[PATCH 1/4] gpio/aspeed: Fix incorrect number of banks

[PATCH 1/4] gpio/aspeed: Fix incorrect number of banks

[Rashmica Gupta]

Fixes: 361b79119a4b7 ('gpio: Add Aspeed driver')

Signed-off-by: Rashmica Gupta <rashmica.g@gmail.com>
---
 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

[PATCH 2/4] gpio/aspeed: Setup irqchip dynamically

[Rashmica Gupta]

This is in preparation for ast2600 as we will have two gpio drivers
which need their own irqchip.

Signed-off-by: Rashmica Gupta <rashmica.g@gmail.com>
---
 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

[PATCH 3/4] gpio: Add in ast2600 details to Aspeed driver

[Rashmica Gupta]

The ast2600 has two gpio controllers, one for 3.6V GPIOS and one for 1.8V GPIOS.

Signed-off-by: Rashmica Gupta <rashmica.g@gmail.com>
---
 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

[PATCH 4/4] gpio: Update documentation with ast2600 controllers

[Rashmica Gupta]

Signed-off-by: Rashmica Gupta <rashmica.g@gmail.com>
---
 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

Re: [PATCH 1/4] gpio/aspeed: Fix incorrect number of banks

[Bartosz Golaszewski]

śr., 4 wrz 2019 o 08:13 Rashmica Gupta <rashmica.g@gmail.com> 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 <rashmica.g@gmail.com>
> ---
>  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
>

Re: [PATCH 4/4] gpio: Update documentation with ast2600 controllers

[Bartosz Golaszewski]

śr., 4 wrz 2019 o 08:13 Rashmica Gupta <rashmica.g@gmail.com> 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 <rashmica.g@gmail.com>
> ---
>  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
>

Re: [PATCH 1/4] gpio/aspeed: Fix incorrect number of banks

[Andy Shevchenko]

On Wed, Sep 4, 2019 at 9:14 AM Rashmica Gupta <rashmica.g@gmail.com> wrote:
>
> Fixes: 361b79119a4b7 ('gpio: Add Aspeed driver')
>
> Signed-off-by: Rashmica Gupta <rashmica.g@gmail.com>

>         /* 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

Re: [PATCH 3/4] gpio: Add in ast2600 details to Aspeed driver

[Andy Shevchenko]

On Wed, Sep 4, 2019 at 9:14 AM Rashmica Gupta <rashmica.g@gmail.com> wrote:
>
> The ast2600 has two gpio controllers, one for 3.6V GPIOS and one for 1.8V GPIOS.
>
> Signed-off-by: Rashmica Gupta <rashmica.g@gmail.com>

> -       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

Re: [PATCH 1/4] gpio/aspeed: Fix incorrect number of banks

[Rashmica Gupta]

On Wed, 2019-09-04 at 19:27 +0300, Andy Shevchenko wrote:
> On Wed, Sep 4, 2019 at 9:14 AM Rashmica Gupta <rashmica.g@gmail.com>
> wrote:
> > Fixes: 361b79119a4b7 ('gpio: Add Aspeed driver')
> > 
> > Signed-off-by: Rashmica Gupta <rashmica.g@gmail.com>
> >         /* 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);
> 
> 

Re: [PATCH 4/4] gpio: Update documentation with ast2600 controllers

[Rashmica Gupta]

On Wed, 2019-09-04 at 09:02 +0200, Bartosz Golaszewski wrote:
> śr., 4 wrz 2019 o 08:13 Rashmica Gupta <rashmica.g@gmail.com>
> 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 <rashmica.g@gmail.com>
> > ---
> >  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
> > 

Re: [PATCH 3/4] gpio: Add in ast2600 details to Aspeed driver

[Rashmica Gupta]

On Wed, 2019-09-04 at 19:30 +0300, Andy Shevchenko wrote:
> On Wed, Sep 4, 2019 at 9:14 AM Rashmica Gupta <rashmica.g@gmail.com>
> wrote:
> > The ast2600 has two gpio controllers, one for 3.6V GPIOS and one
> > for 1.8V GPIOS.
> > 
> > Signed-off-by: Rashmica Gupta <rashmica.g@gmail.com>
> > -       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?
> 

Re: [PATCH 1/4] gpio/aspeed: Fix incorrect number of banks

[Andy Shevchenko]

On Thu, Sep 5, 2019 at 2:17 AM Rashmica Gupta <rashmica.g@gmail.com> wrote:
> On Wed, 2019-09-04 at 19:27 +0300, Andy Shevchenko wrote:
> > On Wed, Sep 4, 2019 at 9:14 AM Rashmica Gupta <rashmica.g@gmail.com>
> > 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

Re: [PATCH 3/4] gpio: Add in ast2600 details to Aspeed driver

[Andy Shevchenko]

On Thu, Sep 5, 2019 at 2:34 AM Rashmica Gupta <rashmica.g@gmail.com> 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

Re: [PATCH 1/4] gpio/aspeed: Fix incorrect number of banks

[Vijay Khemka]

On 9/11/19, 5:16 AM, "Linux-aspeed on behalf of Rashmica Gupta" <linux-aspeed-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of rashmica.g@gmail.com> wrote:

    Fixes: 361b79119a4b7 ('gpio: Add Aspeed driver')
    
    Signed-off-by: Rashmica Gupta <rashmica.g@gmail.com>
    ---
     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