Fixes: 361b79119a4b7 ('gpio: Add Aspeed driver')
Signed-off-by: Rashmica Gupta <[email protected]>
---
drivers/gpio/gpio-aspeed.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
index 9defe25d4721..77752b2624e8 100644
--- a/drivers/gpio/gpio-aspeed.c
+++ b/drivers/gpio/gpio-aspeed.c
@@ -1165,7 +1165,7 @@ static int __init aspeed_gpio_probe(struct platform_device *pdev)
gpio->chip.base = -1;
/* Allocate a cache of the output registers */
- banks = gpio->config->nr_gpios >> 5;
+ banks = (gpio->config->nr_gpios >> 5) + 1;
gpio->dcache = devm_kcalloc(&pdev->dev,
banks, sizeof(u32), GFP_KERNEL);
if (!gpio->dcache)
--
2.20.1
This is in preparation for ast2600 as we will have two gpio drivers
which need their own irqchip.
Signed-off-by: Rashmica Gupta <[email protected]>
---
drivers/gpio/gpio-aspeed.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
index 77752b2624e8..60ab042c9129 100644
--- a/drivers/gpio/gpio-aspeed.c
+++ b/drivers/gpio/gpio-aspeed.c
@@ -52,6 +52,7 @@ struct aspeed_gpio_config {
*/
struct aspeed_gpio {
struct gpio_chip chip;
+ struct irq_chip irqc;
spinlock_t lock;
void __iomem *base;
int irq;
@@ -681,14 +682,6 @@ static void aspeed_gpio_irq_handler(struct irq_desc *desc)
chained_irq_exit(ic, desc);
}
-static struct irq_chip aspeed_gpio_irqchip = {
- .name = "aspeed-gpio",
- .irq_ack = aspeed_gpio_irq_ack,
- .irq_mask = aspeed_gpio_irq_mask,
- .irq_unmask = aspeed_gpio_irq_unmask,
- .irq_set_type = aspeed_gpio_set_type,
-};
-
static void set_irq_valid_mask(struct aspeed_gpio *gpio)
{
const struct aspeed_bank_props *props = gpio->config->props;
@@ -1192,7 +1185,12 @@ static int __init aspeed_gpio_probe(struct platform_device *pdev)
gpio->irq = rc;
girq = &gpio->chip.irq;
- girq->chip = &aspeed_gpio_irqchip;
+ girq->chip = &gpio->irqc;
+ girq->chip->name = dev_name(&pdev->dev);
+ girq->chip->irq_ack = aspeed_gpio_irq_ack;
+ girq->chip->irq_mask = aspeed_gpio_irq_mask;
+ girq->chip->irq_unmask = aspeed_gpio_irq_unmask;
+ girq->chip->irq_set_type = aspeed_gpio_set_type;
girq->parent_handler = aspeed_gpio_irq_handler;
girq->num_parents = 1;
girq->parents = devm_kcalloc(&pdev->dev, 1,
--
2.20.1
The ast2600 has two gpio controllers, one for 3.6V GPIOS and one for 1.8V GPIOS.
Signed-off-by: Rashmica Gupta <[email protected]>
---
drivers/gpio/gpio-aspeed.c | 29 +++++++++++++++++++++++++++--
1 file changed, 27 insertions(+), 2 deletions(-)
diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
index 60ab042c9129..98881c99d0b9 100644
--- a/drivers/gpio/gpio-aspeed.c
+++ b/drivers/gpio/gpio-aspeed.c
@@ -662,12 +662,14 @@ static void aspeed_gpio_irq_handler(struct irq_desc *desc)
struct gpio_chip *gc = irq_desc_get_handler_data(desc);
struct irq_chip *ic = irq_desc_get_chip(desc);
struct aspeed_gpio *data = gpiochip_get_data(gc);
- unsigned int i, p, girq;
+ unsigned int i, p, girq, banks;
unsigned long reg;
+ struct aspeed_gpio *gpio = gpiochip_get_data(gc);
chained_irq_enter(ic, desc);
- for (i = 0; i < ARRAY_SIZE(aspeed_gpio_banks); i++) {
+ banks = (gpio->config->nr_gpios >> 5) + 1;
+ for (i = 0; i < banks; i++) {
const struct aspeed_gpio_bank *bank = &aspeed_gpio_banks[i];
reg = ioread32(bank_reg(data, bank, reg_irq_status));
@@ -1108,9 +1110,32 @@ static const struct aspeed_gpio_config ast2500_config =
/* 232 for simplicity, actual number is 228 (4-GPIO hole in GPIOAB) */
{ .nr_gpios = 232, .props = ast2500_bank_props, };
+static const struct aspeed_bank_props ast2600_bank_props[] = {
+ /* input output */
+ {5, 0xffffffff, 0x0000ffff}, /* U/V/W/X */
+ {6, 0xffff0000, 0x0fff0000}, /* Y/Z */
+ { },
+};
+
+static const struct aspeed_gpio_config ast2600_config =
+ /* 208 3.6V GPIOs */
+ { .nr_gpios = 208, .props = ast2600_bank_props, };
+
+static const struct aspeed_bank_props ast2600_1_8v_bank_props[] = {
+ /* input output */
+ {1, 0x0000000f, 0x0000000f}, /* E */
+ { },
+};
+
+static const struct aspeed_gpio_config ast2600_1_8v_config =
+ /* 36 1.8V GPIOs */
+ { .nr_gpios = 36, .props = ast2600_1_8v_bank_props, };
+
static const struct of_device_id aspeed_gpio_of_table[] = {
{ .compatible = "aspeed,ast2400-gpio", .data = &ast2400_config, },
{ .compatible = "aspeed,ast2500-gpio", .data = &ast2500_config, },
+ { .compatible = "aspeed,ast2600-gpio", .data = &ast2600_config, },
+ { .compatible = "aspeed,ast2600-1-8v-gpio", .data = &ast2600_1_8v_config,},
{}
};
MODULE_DEVICE_TABLE(of, aspeed_gpio_of_table);
--
2.20.1
Signed-off-by: Rashmica Gupta <[email protected]>
---
Documentation/devicetree/bindings/gpio/gpio-aspeed.txt | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/gpio/gpio-aspeed.txt b/Documentation/devicetree/bindings/gpio/gpio-aspeed.txt
index 7e9b586770b0..cd388797e07c 100644
--- a/Documentation/devicetree/bindings/gpio/gpio-aspeed.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio-aspeed.txt
@@ -2,7 +2,8 @@ Aspeed GPIO controller Device Tree Bindings
-------------------------------------------
Required properties:
-- compatible : Either "aspeed,ast2400-gpio" or "aspeed,ast2500-gpio"
+- compatible : Either "aspeed,ast2400-gpio", "aspeed,ast2500-gpio",
+ "aspeed,ast2600-gpio", or "aspeed,ast2600-1-8v-gpio"
- #gpio-cells : Should be two
- First cell is the GPIO line number
--
2.20.1
śr., 4 wrz 2019 o 08:13 Rashmica Gupta <[email protected]> napisał(a):
>
> Fixes: 361b79119a4b7 ('gpio: Add Aspeed driver')
>
Please, add a proper commit description. Checkpatch should have warned
you about it.
Bart
> Signed-off-by: Rashmica Gupta <[email protected]>
> ---
> drivers/gpio/gpio-aspeed.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
> index 9defe25d4721..77752b2624e8 100644
> --- a/drivers/gpio/gpio-aspeed.c
> +++ b/drivers/gpio/gpio-aspeed.c
> @@ -1165,7 +1165,7 @@ static int __init aspeed_gpio_probe(struct platform_device *pdev)
> gpio->chip.base = -1;
>
> /* Allocate a cache of the output registers */
> - banks = gpio->config->nr_gpios >> 5;
> + banks = (gpio->config->nr_gpios >> 5) + 1;
> gpio->dcache = devm_kcalloc(&pdev->dev,
> banks, sizeof(u32), GFP_KERNEL);
> if (!gpio->dcache)
> --
> 2.20.1
>
śr., 4 wrz 2019 o 08:13 Rashmica Gupta <[email protected]> napisał(a):
>
Again, this needs a proper commit description and the subject should
start with "dt-bindings: ...".
You also need to Cc the device-tree maintainers. Use
scripts/get_maintainer.pl to list all people that should get this
patch.
Bart
> Signed-off-by: Rashmica Gupta <[email protected]>
> ---
> Documentation/devicetree/bindings/gpio/gpio-aspeed.txt | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-aspeed.txt b/Documentation/devicetree/bindings/gpio/gpio-aspeed.txt
> index 7e9b586770b0..cd388797e07c 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio-aspeed.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio-aspeed.txt
> @@ -2,7 +2,8 @@ Aspeed GPIO controller Device Tree Bindings
> -------------------------------------------
>
> Required properties:
> -- compatible : Either "aspeed,ast2400-gpio" or "aspeed,ast2500-gpio"
> +- compatible : Either "aspeed,ast2400-gpio", "aspeed,ast2500-gpio",
> + "aspeed,ast2600-gpio", or "aspeed,ast2600-1-8v-gpio"
>
> - #gpio-cells : Should be two
> - First cell is the GPIO line number
> --
> 2.20.1
>
On Wed, Sep 4, 2019 at 9:14 AM Rashmica Gupta <[email protected]> wrote:
>
> Fixes: 361b79119a4b7 ('gpio: Add Aspeed driver')
>
> Signed-off-by: Rashmica Gupta <[email protected]>
> /* Allocate a cache of the output registers */
> - banks = gpio->config->nr_gpios >> 5;
> + banks = (gpio->config->nr_gpios >> 5) + 1;
Shouldn't be rather DIV_ROUND_UP(nr_gpios, sizeof(u32)) ?
> gpio->dcache = devm_kcalloc(&pdev->dev,
> banks, sizeof(u32), GFP_KERNEL);
--
With Best Regards,
Andy Shevchenko
On Wed, Sep 4, 2019 at 9:14 AM Rashmica Gupta <[email protected]> wrote:
>
> The ast2600 has two gpio controllers, one for 3.6V GPIOS and one for 1.8V GPIOS.
>
> Signed-off-by: Rashmica Gupta <[email protected]>
> - for (i = 0; i < ARRAY_SIZE(aspeed_gpio_banks); i++) {
> + banks = (gpio->config->nr_gpios >> 5) + 1;
Same comment as per the other patch.
> + for (i = 0; i < banks; i++) {
> +static const struct aspeed_bank_props ast2600_bank_props[] = {
> + /* input output */
> + {5, 0xffffffff, 0x0000ffff}, /* U/V/W/X */
> + {6, 0xffff0000, 0x0fff0000}, /* Y/Z */
Perhaps GENMASK() for all values?
> + { },
Comma is not needed here.
> +};
> +
> +static const struct aspeed_gpio_config ast2600_config =
> + /* 208 3.6V GPIOs */
> + { .nr_gpios = 208, .props = ast2600_bank_props, };
Seems curly braces missed their places.
> +static const struct aspeed_bank_props ast2600_1_8v_bank_props[] = {
> + /* input output */
> + {1, 0x0000000f, 0x0000000f}, /* E */
GENMASK()?
> + { },
No comma.
> +};
> +static const struct aspeed_gpio_config ast2600_1_8v_config =
> + /* 36 1.8V GPIOs */
> + { .nr_gpios = 36, .props = ast2600_1_8v_bank_props, };
Location of the curly braces?
--
With Best Regards,
Andy Shevchenko
On Wed, 2019-09-04 at 19:27 +0300, Andy Shevchenko wrote:
> On Wed, Sep 4, 2019 at 9:14 AM Rashmica Gupta <[email protected]>
> wrote:
> > Fixes: 361b79119a4b7 ('gpio: Add Aspeed driver')
> >
> > Signed-off-by: Rashmica Gupta <[email protected]>
> > /* Allocate a cache of the output registers */
> > - banks = gpio->config->nr_gpios >> 5;
> > + banks = (gpio->config->nr_gpios >> 5) + 1;
>
> Shouldn't be rather DIV_ROUND_UP(nr_gpios, sizeof(u32)) ?
I agree that DIV_ROUND_UP is the right thing to use here, but wouldn't
it be DIV_ROUND_UP(nr_gpios, 32)?
>
> > gpio->dcache = devm_kcalloc(&pdev->dev,
> > banks, sizeof(u32),
> > GFP_KERNEL);
>
>
On Wed, 2019-09-04 at 09:02 +0200, Bartosz Golaszewski wrote:
> śr., 4 wrz 2019 o 08:13 Rashmica Gupta <[email protected]>
> napisał(a):
>
> Again, this needs a proper commit description
I figured as similar patches have gone through with just the one line
and there isn't anything more to say that the one line message that
this would be ok.
>and the subject should
> start with "dt-bindings: ...".
>
True.
> You also need to Cc the device-tree maintainers. Use
> scripts/get_maintainer.pl to list all people that should get this
> patch.
Must have missed that one somehow.
>
> Bart
>
> > Signed-off-by: Rashmica Gupta <[email protected]>
> > ---
> > Documentation/devicetree/bindings/gpio/gpio-aspeed.txt | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/gpio/gpio-
aspeed.txt
> > b/Documentation/devicetree/bindings/gpio/gpio-aspeed.txt
> > index 7e9b586770b0..cd388797e07c 100644
> > --- a/Documentation/devicetree/bindings/gpio/gpio-aspeed.txt
> > +++ b/Documentation/devicetree/bindings/gpio/gpio-aspeed.txt
> > @@ -2,7 +2,8 @@ Aspeed GPIO controller Device Tree Bindings
> > -------------------------------------------
> >
> > Required properties:
> > -- compatible : Either "aspeed,ast2400-gpio" or
> > "aspeed,ast2500-gpio"
> > +- compatible : Either "aspeed,ast2400-gpio",
> > "aspeed,ast2500-gpio",
> > + "aspeed,ast2600-gpio", or
> > "aspeed,ast2600-1-8v-gpio"
> >
> > - #gpio-cells : Should be two
> > - First cell is the GPIO line number
> > --
> > 2.20.1
> >
On Wed, 2019-09-04 at 19:30 +0300, Andy Shevchenko wrote:
> On Wed, Sep 4, 2019 at 9:14 AM Rashmica Gupta <[email protected]>
> wrote:
> > The ast2600 has two gpio controllers, one for 3.6V GPIOS and one
> > for 1.8V GPIOS.
> >
> > Signed-off-by: Rashmica Gupta <[email protected]>
> > - for (i = 0; i < ARRAY_SIZE(aspeed_gpio_banks); i++) {
> > + banks = (gpio->config->nr_gpios >> 5) + 1;
>
> Same comment as per the other patch.
>
> > + for (i = 0; i < banks; i++) {
> > +static const struct aspeed_bank_props ast2600_bank_props[] = {
> > + /* input output */
> > + {5, 0xffffffff, 0x0000ffff}, /* U/V/W/X */
> > + {6, 0xffff0000, 0x0fff0000}, /* Y/Z */
>
> Perhaps GENMASK() for all values?
Perhaps this and your other comments below would be best addressed in
an additional cleanup patch? This patch follows the formatting of the
existing code and it's not very clean to differ from that or to change
the formatting of the current code in this patch.
>
> > + { },
>
> Comma is not needed here.
>
> > +};
> > +
> > +static const struct aspeed_gpio_config ast2600_config =
> > + /* 208 3.6V GPIOs */
> > + { .nr_gpios = 208, .props = ast2600_bank_props, };
>
> Seems curly braces missed their places.
>
> > +static const struct aspeed_bank_props ast2600_1_8v_bank_props[] =
> > {
> > + /* input output */
> > + {1, 0x0000000f, 0x0000000f}, /* E */
>
> GENMASK()?
>
> > + { },
>
> No comma.
>
> > +};
> > +static const struct aspeed_gpio_config ast2600_1_8v_config =
> > + /* 36 1.8V GPIOs */
> > + { .nr_gpios = 36, .props = ast2600_1_8v_bank_props, };
>
> Location of the curly braces?
>
On Thu, Sep 5, 2019 at 2:34 AM Rashmica Gupta <[email protected]> wrote:>
> On Wed, 2019-09-04 at 19:30 +0300, Andy Shevchenko wrote:
> Perhaps this and your other comments below would be best addressed in
> an additional cleanup patch? This patch follows the formatting of the
> existing code and it's not very clean to differ from that or to change
> the formatting of the current code in this patch.
OK.
--
With Best Regards,
Andy Shevchenko
On Thu, Sep 5, 2019 at 2:17 AM Rashmica Gupta <[email protected]> wrote:
> On Wed, 2019-09-04 at 19:27 +0300, Andy Shevchenko wrote:
> > On Wed, Sep 4, 2019 at 9:14 AM Rashmica Gupta <[email protected]>
> > wrote:
> > > - banks = gpio->config->nr_gpios >> 5;
> > > + banks = (gpio->config->nr_gpios >> 5) + 1;
> >
> > Shouldn't be rather DIV_ROUND_UP(nr_gpios, sizeof(u32)) ?
>
> I agree that DIV_ROUND_UP is the right thing to use here, but wouldn't
> it be DIV_ROUND_UP(nr_gpios, 32)?
Right. Either this or BITS_PER_TYPE(u32).
> > > gpio->dcache = devm_kcalloc(&pdev->dev,
> > > banks, sizeof(u32),
> > > GFP_KERNEL);
--
With Best Regards,
Andy Shevchenko
On 9/11/19, 5:16 AM, "Linux-aspeed on behalf of Rashmica Gupta" <[email protected] on behalf of [email protected]> wrote:
Fixes: 361b79119a4b7 ('gpio: Add Aspeed driver')
Signed-off-by: Rashmica Gupta <[email protected]>
---
drivers/gpio/gpio-aspeed.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
index 9defe25d4721..77752b2624e8 100644
--- a/drivers/gpio/gpio-aspeed.c
+++ b/drivers/gpio/gpio-aspeed.c
@@ -1165,7 +1165,7 @@ static int __init aspeed_gpio_probe(struct platform_device *pdev)
gpio->chip.base = -1;
/* Allocate a cache of the output registers */
- banks = gpio->config->nr_gpios >> 5;
+ banks = (gpio->config->nr_gpios >> 5) + 1;
If number of gpios are 32 then it should be only 1 bank, as per above it is 2 bank.
gpio->dcache = devm_kcalloc(&pdev->dev,
banks, sizeof(u32), GFP_KERNEL);
if (!gpio->dcache)
--
2.20.1