| Message ID | 20210608102547.4880-7-steven_lee@aspeedtech.com |
|---|---|
| State | New |
| Headers | show |
| Series | ASPEED sgpio driver enhancement. | expand |
On Tue, 8 Jun 2021, at 19:55, Steven Lee wrote: > We use platform data to store GPIO pin mask and the max number of > available GPIO pins for AST2600. > Refactor driver to also add the platform data for AST2400/AST2500 and > remove unused MAX_NR_HW_SGPIO and ASPEED_SGPIO_PINS_MASK macros. > > Signed-off-by: Steven Lee <steven_lee@aspeedtech.com> > --- > drivers/gpio/gpio-aspeed-sgpio.c | 34 +++++++++++--------------------- > 1 file changed, 12 insertions(+), 22 deletions(-) > > diff --git a/drivers/gpio/gpio-aspeed-sgpio.c b/drivers/gpio/gpio-aspeed-sgpio.c > index ea20a0127748..7d0a4f6fd9d1 100644 > --- a/drivers/gpio/gpio-aspeed-sgpio.c > +++ b/drivers/gpio/gpio-aspeed-sgpio.c > @@ -17,21 +17,8 @@ > #include <linux/spinlock.h> > #include <linux/string.h> > > -/* > - * MAX_NR_HW_GPIO represents the number of actual hardware-supported GPIOs (ie, > - * slots within the clocked serial GPIO data). Since each HW GPIO is both an > - * input and an output, we provide MAX_NR_HW_GPIO * 2 lines on our gpiochip > - * device. > - * > - * We use SGPIO_OUTPUT_OFFSET to define the split between the inputs and > - * outputs; the inputs start at line 0, the outputs start at OUTPUT_OFFSET. > - */ > -#define MAX_NR_HW_SGPIO 80 > -#define SGPIO_OUTPUT_OFFSET MAX_NR_HW_SGPIO > - > #define ASPEED_SGPIO_CTRL 0x54 > > -#define ASPEED_SGPIO_PINS_MASK GENMASK(9, 6) > #define ASPEED_SGPIO_CLK_DIV_MASK GENMASK(31, 16) > #define ASPEED_SGPIO_ENABLE BIT(0) > #define ASPEED_SGPIO_PINS_SHIFT 6 > @@ -484,6 +471,11 @@ static int aspeed_sgpio_setup_irqs(struct > aspeed_sgpio *gpio, > return 0; > } > > +static const struct aspeed_sgpio_pdata ast2400_sgpio_pdata = { > + .max_ngpios = 80, > + .pin_mask = GENMASK(9, 6), > +}; > + > static const struct aspeed_sgpio_pdata ast2600_sgpiom_128_pdata = { > .max_ngpios = 128, > .pin_mask = GENMASK(10, 6), > @@ -495,8 +487,8 @@ static const struct aspeed_sgpio_pdata > ast2600_sgpiom_80_pdata = { > }; > > static const struct of_device_id aspeed_sgpio_of_table[] = { > - { .compatible = "aspeed,ast2400-sgpio" }, > - { .compatible = "aspeed,ast2500-sgpio" }, > + { .compatible = "aspeed,ast2400-sgpio", .data = &ast2400_sgpio_pdata, > }, > + { .compatible = "aspeed,ast2500-sgpio", .data = &ast2400_sgpio_pdata, > }, > { .compatible = "aspeed,ast2600-sgpiom-128", .data = > &ast2600_sgpiom_128_pdata, }, > { .compatible = "aspeed,ast2600-sgpiom-80", .data = > &ast2600_sgpiom_80_pdata, }, > {} > @@ -521,13 +513,11 @@ static int __init aspeed_sgpio_probe(struct > platform_device *pdev) > return PTR_ERR(gpio->base); > > pdata = device_get_match_data(&pdev->dev); > - if (pdata) { > - gpio->max_ngpios = pdata->max_ngpios; > - pin_mask = pdata->pin_mask; > - } else { > - gpio->max_ngpios = MAX_NR_HW_SGPIO; > - pin_mask = ASPEED_SGPIO_PINS_MASK; > - } > + if (!pdata) > + return -EINVAL; > + > + gpio->max_ngpios = pdata->max_ngpios; > + pin_mask = pdata->pin_mask; Hmm, okay, maybe just re-order the patches so this commit comes before the previous one. That way we don't immediately rip out this condition that we just introduced in the previous patch. I think I suggested squashing it into the previous patch, but with the removal of the comments and macros I think it's worth leaving it separate, just reordered. Andrew
The 06/09/2021 08:55, Andrew Jeffery wrote: > > > On Tue, 8 Jun 2021, at 19:55, Steven Lee wrote: > > We use platform data to store GPIO pin mask and the max number of > > available GPIO pins for AST2600. > > Refactor driver to also add the platform data for AST2400/AST2500 and > > remove unused MAX_NR_HW_SGPIO and ASPEED_SGPIO_PINS_MASK macros. > > > > Signed-off-by: Steven Lee <steven_lee@aspeedtech.com> > > --- > > drivers/gpio/gpio-aspeed-sgpio.c | 34 +++++++++++--------------------- > > 1 file changed, 12 insertions(+), 22 deletions(-) > > > > diff --git a/drivers/gpio/gpio-aspeed-sgpio.c b/drivers/gpio/gpio-aspeed-sgpio.c > > index ea20a0127748..7d0a4f6fd9d1 100644 > > --- a/drivers/gpio/gpio-aspeed-sgpio.c > > +++ b/drivers/gpio/gpio-aspeed-sgpio.c > > @@ -17,21 +17,8 @@ > > #include <linux/spinlock.h> > > #include <linux/string.h> > > > > -/* > > - * MAX_NR_HW_GPIO represents the number of actual hardware-supported GPIOs (ie, > > - * slots within the clocked serial GPIO data). Since each HW GPIO is both an > > - * input and an output, we provide MAX_NR_HW_GPIO * 2 lines on our gpiochip > > - * device. > > - * > > - * We use SGPIO_OUTPUT_OFFSET to define the split between the inputs and > > - * outputs; the inputs start at line 0, the outputs start at OUTPUT_OFFSET. > > - */ > > -#define MAX_NR_HW_SGPIO 80 > > -#define SGPIO_OUTPUT_OFFSET MAX_NR_HW_SGPIO > > - > > #define ASPEED_SGPIO_CTRL 0x54 > > > > -#define ASPEED_SGPIO_PINS_MASK GENMASK(9, 6) > > #define ASPEED_SGPIO_CLK_DIV_MASK GENMASK(31, 16) > > #define ASPEED_SGPIO_ENABLE BIT(0) > > #define ASPEED_SGPIO_PINS_SHIFT 6 > > @@ -484,6 +471,11 @@ static int aspeed_sgpio_setup_irqs(struct > > aspeed_sgpio *gpio, > > return 0; > > } > > > > +static const struct aspeed_sgpio_pdata ast2400_sgpio_pdata = { > > + .max_ngpios = 80, > > + .pin_mask = GENMASK(9, 6), > > +}; > > + > > static const struct aspeed_sgpio_pdata ast2600_sgpiom_128_pdata = { > > .max_ngpios = 128, > > .pin_mask = GENMASK(10, 6), > > @@ -495,8 +487,8 @@ static const struct aspeed_sgpio_pdata > > ast2600_sgpiom_80_pdata = { > > }; > > > > static const struct of_device_id aspeed_sgpio_of_table[] = { > > - { .compatible = "aspeed,ast2400-sgpio" }, > > - { .compatible = "aspeed,ast2500-sgpio" }, > > + { .compatible = "aspeed,ast2400-sgpio", .data = &ast2400_sgpio_pdata, > > }, > > + { .compatible = "aspeed,ast2500-sgpio", .data = &ast2400_sgpio_pdata, > > }, > > { .compatible = "aspeed,ast2600-sgpiom-128", .data = > > &ast2600_sgpiom_128_pdata, }, > > { .compatible = "aspeed,ast2600-sgpiom-80", .data = > > &ast2600_sgpiom_80_pdata, }, > > {} > > @@ -521,13 +513,11 @@ static int __init aspeed_sgpio_probe(struct > > platform_device *pdev) > > return PTR_ERR(gpio->base); > > > > pdata = device_get_match_data(&pdev->dev); > > - if (pdata) { > > - gpio->max_ngpios = pdata->max_ngpios; > > - pin_mask = pdata->pin_mask; > > - } else { > > - gpio->max_ngpios = MAX_NR_HW_SGPIO; > > - pin_mask = ASPEED_SGPIO_PINS_MASK; > > - } > > + if (!pdata) > > + return -EINVAL; > > + > > + gpio->max_ngpios = pdata->max_ngpios; > > + pin_mask = pdata->pin_mask; > > Hmm, okay, maybe just re-order the patches so this commit comes before the previous one. That way we don't immediately rip out this condition that we just introduced in the previous patch. > > I think I suggested squashing it into the previous patch, but with the removal of the comments and macros I think it's worth leaving it separate, just reordered. > I was wondering if I can squash patch-05 and patch-06 into one patch as this patch(patch-06) requires macros, structures, and functions that modified in the previous patch(patch-05). Thanks, Steven > Andrew
On Wed, 9 Jun 2021, at 13:42, Steven Lee wrote: > The 06/09/2021 08:55, Andrew Jeffery wrote: > > > > > > On Tue, 8 Jun 2021, at 19:55, Steven Lee wrote: > > > We use platform data to store GPIO pin mask and the max number of > > > available GPIO pins for AST2600. > > > Refactor driver to also add the platform data for AST2400/AST2500 and > > > remove unused MAX_NR_HW_SGPIO and ASPEED_SGPIO_PINS_MASK macros. > > > > > > Signed-off-by: Steven Lee <steven_lee@aspeedtech.com> > > > --- > > > drivers/gpio/gpio-aspeed-sgpio.c | 34 +++++++++++--------------------- > > > 1 file changed, 12 insertions(+), 22 deletions(-) > > > > > > diff --git a/drivers/gpio/gpio-aspeed-sgpio.c b/drivers/gpio/gpio-aspeed-sgpio.c > > > index ea20a0127748..7d0a4f6fd9d1 100644 > > > --- a/drivers/gpio/gpio-aspeed-sgpio.c > > > +++ b/drivers/gpio/gpio-aspeed-sgpio.c > > > @@ -17,21 +17,8 @@ > > > #include <linux/spinlock.h> > > > #include <linux/string.h> > > > > > > -/* > > > - * MAX_NR_HW_GPIO represents the number of actual hardware-supported GPIOs (ie, > > > - * slots within the clocked serial GPIO data). Since each HW GPIO is both an > > > - * input and an output, we provide MAX_NR_HW_GPIO * 2 lines on our gpiochip > > > - * device. > > > - * > > > - * We use SGPIO_OUTPUT_OFFSET to define the split between the inputs and > > > - * outputs; the inputs start at line 0, the outputs start at OUTPUT_OFFSET. > > > - */ > > > -#define MAX_NR_HW_SGPIO 80 > > > -#define SGPIO_OUTPUT_OFFSET MAX_NR_HW_SGPIO > > > - > > > #define ASPEED_SGPIO_CTRL 0x54 > > > > > > -#define ASPEED_SGPIO_PINS_MASK GENMASK(9, 6) > > > #define ASPEED_SGPIO_CLK_DIV_MASK GENMASK(31, 16) > > > #define ASPEED_SGPIO_ENABLE BIT(0) > > > #define ASPEED_SGPIO_PINS_SHIFT 6 > > > @@ -484,6 +471,11 @@ static int aspeed_sgpio_setup_irqs(struct > > > aspeed_sgpio *gpio, > > > return 0; > > > } > > > > > > +static const struct aspeed_sgpio_pdata ast2400_sgpio_pdata = { > > > + .max_ngpios = 80, > > > + .pin_mask = GENMASK(9, 6), > > > +}; > > > + > > > static const struct aspeed_sgpio_pdata ast2600_sgpiom_128_pdata = { > > > .max_ngpios = 128, > > > .pin_mask = GENMASK(10, 6), > > > @@ -495,8 +487,8 @@ static const struct aspeed_sgpio_pdata > > > ast2600_sgpiom_80_pdata = { > > > }; > > > > > > static const struct of_device_id aspeed_sgpio_of_table[] = { > > > - { .compatible = "aspeed,ast2400-sgpio" }, > > > - { .compatible = "aspeed,ast2500-sgpio" }, > > > + { .compatible = "aspeed,ast2400-sgpio", .data = &ast2400_sgpio_pdata, > > > }, > > > + { .compatible = "aspeed,ast2500-sgpio", .data = &ast2400_sgpio_pdata, > > > }, > > > { .compatible = "aspeed,ast2600-sgpiom-128", .data = > > > &ast2600_sgpiom_128_pdata, }, > > > { .compatible = "aspeed,ast2600-sgpiom-80", .data = > > > &ast2600_sgpiom_80_pdata, }, > > > {} > > > @@ -521,13 +513,11 @@ static int __init aspeed_sgpio_probe(struct > > > platform_device *pdev) > > > return PTR_ERR(gpio->base); > > > > > > pdata = device_get_match_data(&pdev->dev); > > > - if (pdata) { > > > - gpio->max_ngpios = pdata->max_ngpios; > > > - pin_mask = pdata->pin_mask; > > > - } else { > > > - gpio->max_ngpios = MAX_NR_HW_SGPIO; > > > - pin_mask = ASPEED_SGPIO_PINS_MASK; > > > - } > > > + if (!pdata) > > > + return -EINVAL; > > > + > > > + gpio->max_ngpios = pdata->max_ngpios; > > > + pin_mask = pdata->pin_mask; > > > > Hmm, okay, maybe just re-order the patches so this commit comes before the previous one. That way we don't immediately rip out this condition that we just introduced in the previous patch. > > > > I think I suggested squashing it into the previous patch, but with the removal of the comments and macros I think it's worth leaving it separate, just reordered. > > > > I was wondering if I can squash patch-05 and patch-06 into one patch > as this patch(patch-06) requires macros, structures, and functions that > modified in the previous patch(patch-05). Yeah, fair enough. Just squash them. Cheers, Andrew
The 06/12/2021 03:02, Bartosz Golaszewski wrote: > On Wed, Jun 9, 2021 at 8:46 AM Andrew Jeffery <andrew@aj.id.au> wrote: > > > > > > > > On Wed, 9 Jun 2021, at 13:42, Steven Lee wrote: > > > The 06/09/2021 08:55, Andrew Jeffery wrote: > > > > > > > > > > > > On Tue, 8 Jun 2021, at 19:55, Steven Lee wrote: > > > > > We use platform data to store GPIO pin mask and the max number of > > > > > available GPIO pins for AST2600. > > > > > Refactor driver to also add the platform data for AST2400/AST2500 and > > > > > remove unused MAX_NR_HW_SGPIO and ASPEED_SGPIO_PINS_MASK macros. > > > > > > > > > > Signed-off-by: Steven Lee <steven_lee@aspeedtech.com> > > > > > --- > > > > > drivers/gpio/gpio-aspeed-sgpio.c | 34 +++++++++++--------------------- > > > > > 1 file changed, 12 insertions(+), 22 deletions(-) > > > > > > > > > > diff --git a/drivers/gpio/gpio-aspeed-sgpio.c b/drivers/gpio/gpio-aspeed-sgpio.c > > > > > index ea20a0127748..7d0a4f6fd9d1 100644 > > > > > --- a/drivers/gpio/gpio-aspeed-sgpio.c > > > > > +++ b/drivers/gpio/gpio-aspeed-sgpio.c > > > > > @@ -17,21 +17,8 @@ > > > > > #include <linux/spinlock.h> > > > > > #include <linux/string.h> > > > > > > > > > > -/* > > > > > - * MAX_NR_HW_GPIO represents the number of actual hardware-supported GPIOs (ie, > > > > > - * slots within the clocked serial GPIO data). Since each HW GPIO is both an > > > > > - * input and an output, we provide MAX_NR_HW_GPIO * 2 lines on our gpiochip > > > > > - * device. > > > > > - * > > > > > - * We use SGPIO_OUTPUT_OFFSET to define the split between the inputs and > > > > > - * outputs; the inputs start at line 0, the outputs start at OUTPUT_OFFSET. > > > > > - */ > > > > > -#define MAX_NR_HW_SGPIO 80 > > > > > -#define SGPIO_OUTPUT_OFFSET MAX_NR_HW_SGPIO > > > > > - > > > > > #define ASPEED_SGPIO_CTRL 0x54 > > > > > > > > > > -#define ASPEED_SGPIO_PINS_MASK GENMASK(9, 6) > > > > > #define ASPEED_SGPIO_CLK_DIV_MASK GENMASK(31, 16) > > > > > #define ASPEED_SGPIO_ENABLE BIT(0) > > > > > #define ASPEED_SGPIO_PINS_SHIFT 6 > > > > > @@ -484,6 +471,11 @@ static int aspeed_sgpio_setup_irqs(struct > > > > > aspeed_sgpio *gpio, > > > > > return 0; > > > > > } > > > > > > > > > > +static const struct aspeed_sgpio_pdata ast2400_sgpio_pdata = { > > > > > + .max_ngpios = 80, > > > > > + .pin_mask = GENMASK(9, 6), > > > > > +}; > > > > > + > > > > > static const struct aspeed_sgpio_pdata ast2600_sgpiom_128_pdata = { > > > > > .max_ngpios = 128, > > > > > .pin_mask = GENMASK(10, 6), > > > > > @@ -495,8 +487,8 @@ static const struct aspeed_sgpio_pdata > > > > > ast2600_sgpiom_80_pdata = { > > > > > }; > > > > > > > > > > static const struct of_device_id aspeed_sgpio_of_table[] = { > > > > > - { .compatible = "aspeed,ast2400-sgpio" }, > > > > > - { .compatible = "aspeed,ast2500-sgpio" }, > > > > > + { .compatible = "aspeed,ast2400-sgpio", .data = &ast2400_sgpio_pdata, > > > > > }, > > > > > + { .compatible = "aspeed,ast2500-sgpio", .data = &ast2400_sgpio_pdata, > > > > > }, > > > > > { .compatible = "aspeed,ast2600-sgpiom-128", .data = > > > > > &ast2600_sgpiom_128_pdata, }, > > > > > { .compatible = "aspeed,ast2600-sgpiom-80", .data = > > > > > &ast2600_sgpiom_80_pdata, }, > > > > > {} > > > > > @@ -521,13 +513,11 @@ static int __init aspeed_sgpio_probe(struct > > > > > platform_device *pdev) > > > > > return PTR_ERR(gpio->base); > > > > > > > > > > pdata = device_get_match_data(&pdev->dev); > > > > > - if (pdata) { > > > > > - gpio->max_ngpios = pdata->max_ngpios; > > > > > - pin_mask = pdata->pin_mask; > > > > > - } else { > > > > > - gpio->max_ngpios = MAX_NR_HW_SGPIO; > > > > > - pin_mask = ASPEED_SGPIO_PINS_MASK; > > > > > - } > > > > > + if (!pdata) > > > > > + return -EINVAL; > > > > > + > > > > > + gpio->max_ngpios = pdata->max_ngpios; > > > > > + pin_mask = pdata->pin_mask; > > > > > > > > Hmm, okay, maybe just re-order the patches so this commit comes before the previous one. That way we don't immediately rip out this condition that we just introduced in the previous patch. > > > > > > > > I think I suggested squashing it into the previous patch, but with the removal of the comments and macros I think it's worth leaving it separate, just reordered. > > > > > > > > > > I was wondering if I can squash patch-05 and patch-06 into one patch > > > as this patch(patch-06) requires macros, structures, and functions that > > > modified in the previous patch(patch-05). > > > > Yeah, fair enough. Just squash them. > > > > Cheers, > > > > Andrew > > I'm ready to pick this up as soon as you respin the series. > Hi Bart, Per the discussion in the following mail threads, I may redesign aspeed sgpio driver for the new solution. https://lkml.org/lkml/2021/6/3/1507 https://lkml.org/lkml/2021/6/10/240 Patch02- Patch06 of this patch series will need to be modified for the new solution, although some of them have Reviewed-by tag. Thanks, Steven > Bart
diff --git a/drivers/gpio/gpio-aspeed-sgpio.c b/drivers/gpio/gpio-aspeed-sgpio.c index ea20a0127748..7d0a4f6fd9d1 100644 --- a/drivers/gpio/gpio-aspeed-sgpio.c +++ b/drivers/gpio/gpio-aspeed-sgpio.c @@ -17,21 +17,8 @@ #include <linux/spinlock.h> #include <linux/string.h> -/* - * MAX_NR_HW_GPIO represents the number of actual hardware-supported GPIOs (ie, - * slots within the clocked serial GPIO data). Since each HW GPIO is both an - * input and an output, we provide MAX_NR_HW_GPIO * 2 lines on our gpiochip - * device. - * - * We use SGPIO_OUTPUT_OFFSET to define the split between the inputs and - * outputs; the inputs start at line 0, the outputs start at OUTPUT_OFFSET. - */ -#define MAX_NR_HW_SGPIO 80 -#define SGPIO_OUTPUT_OFFSET MAX_NR_HW_SGPIO - #define ASPEED_SGPIO_CTRL 0x54 -#define ASPEED_SGPIO_PINS_MASK GENMASK(9, 6) #define ASPEED_SGPIO_CLK_DIV_MASK GENMASK(31, 16) #define ASPEED_SGPIO_ENABLE BIT(0) #define ASPEED_SGPIO_PINS_SHIFT 6 @@ -484,6 +471,11 @@ static int aspeed_sgpio_setup_irqs(struct aspeed_sgpio *gpio, return 0; } +static const struct aspeed_sgpio_pdata ast2400_sgpio_pdata = { + .max_ngpios = 80, + .pin_mask = GENMASK(9, 6), +}; + static const struct aspeed_sgpio_pdata ast2600_sgpiom_128_pdata = { .max_ngpios = 128, .pin_mask = GENMASK(10, 6), @@ -495,8 +487,8 @@ static const struct aspeed_sgpio_pdata ast2600_sgpiom_80_pdata = { }; static const struct of_device_id aspeed_sgpio_of_table[] = { - { .compatible = "aspeed,ast2400-sgpio" }, - { .compatible = "aspeed,ast2500-sgpio" }, + { .compatible = "aspeed,ast2400-sgpio", .data = &ast2400_sgpio_pdata, }, + { .compatible = "aspeed,ast2500-sgpio", .data = &ast2400_sgpio_pdata, }, { .compatible = "aspeed,ast2600-sgpiom-128", .data = &ast2600_sgpiom_128_pdata, }, { .compatible = "aspeed,ast2600-sgpiom-80", .data = &ast2600_sgpiom_80_pdata, }, {} @@ -521,13 +513,11 @@ static int __init aspeed_sgpio_probe(struct platform_device *pdev) return PTR_ERR(gpio->base); pdata = device_get_match_data(&pdev->dev); - if (pdata) { - gpio->max_ngpios = pdata->max_ngpios; - pin_mask = pdata->pin_mask; - } else { - gpio->max_ngpios = MAX_NR_HW_SGPIO; - pin_mask = ASPEED_SGPIO_PINS_MASK; - } + if (!pdata) + return -EINVAL; + + gpio->max_ngpios = pdata->max_ngpios; + pin_mask = pdata->pin_mask; rc = of_property_read_u32(pdev->dev.of_node, "ngpios", &nr_gpios); if (rc < 0) {
We use platform data to store GPIO pin mask and the max number of available GPIO pins for AST2600. Refactor driver to also add the platform data for AST2400/AST2500 and remove unused MAX_NR_HW_SGPIO and ASPEED_SGPIO_PINS_MASK macros. Signed-off-by: Steven Lee <steven_lee@aspeedtech.com> --- drivers/gpio/gpio-aspeed-sgpio.c | 34 +++++++++++--------------------- 1 file changed, 12 insertions(+), 22 deletions(-)