[PATCH v1 0/5] Improvements for MAX77620 GPIO driver

[PATCH v1 0/5] Improvements for MAX77620 GPIO driver

[Dmitry Osipenko]

Hello!

This series addresses a problem that I discovered on Nexus 7 device where
GPIO interrupts may be left enabled after bootloader and the driver isn't
prepared to this. Secondly, I made few very minor cleanup improvements to
the code.

Dmitry Osipenko (5):
  gpio: max77620: Initialize interrupts state
  gpio: max77620: Replace 8 with MAX77620_GPIO_NR
  gpio: max77620: Replace interrupt-enable array with bitmap
  gpio: max77620: Don't handle disabled interrupts
  gpio: max77620: Move variable declaration

 drivers/gpio/gpio-max77620.c | 44 ++++++++++++++++++++++++++++--------
 1 file changed, 34 insertions(+), 10 deletions(-)

-- 
2.26.0

[PATCH v1 1/5] gpio: max77620: Initialize interrupts state

[Dmitry Osipenko]

I noticed on Nexus 7 that after rebooting from downstream kernel to
upstream, the GPIO interrupt is triggering non-stop despite of interrupts
being disabled for all of GPIOs. This happens because Nexus 7 uses a
soft-reboot, meaning that bootloader should take care of resetting
hardware, but bootloader doesn't do it well. In a result, GPIO interrupt
may be left ON at a boot time. Let's mask all GPIO interrupts at the
driver's probe time in order to resolve the issue.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/gpio/gpio-max77620.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/gpio/gpio-max77620.c b/drivers/gpio/gpio-max77620.c
index 313bd02dd893..473b4e900bbb 100644
--- a/drivers/gpio/gpio-max77620.c
+++ b/drivers/gpio/gpio-max77620.c
@@ -260,6 +260,25 @@ static int max77620_gpio_set_config(struct gpio_chip *gc, unsigned int offset,
 	return -ENOTSUPP;
 }
 
+static void max77620_gpio_initialize(struct max77620_gpio *mgpio)
+{
+	unsigned int i;
+	int err;
+
+	/*
+	 * GPIO interrupts may be left ON after bootloader, hence let's
+	 * pre-initialize hardware to the expected state by disabling all
+	 * interrupts.
+	 */
+	for (i = 0; i < MAX77620_GPIO_NR; i++) {
+		err = regmap_update_bits(mgpio->rmap, GPIO_REG_ADDR(i),
+					 MAX77620_CNFG_GPIO_INT_MASK, 0);
+		if (err < 0)
+			dev_err(mgpio->dev, "failed to disable interrupt: %d\n",
+				err);
+	}
+}
+
 static int max77620_gpio_probe(struct platform_device *pdev)
 {
 	struct max77620_chip *chip =  dev_get_drvdata(pdev->dev.parent);
@@ -292,6 +311,7 @@ static int max77620_gpio_probe(struct platform_device *pdev)
 	mgpio->gpio_chip.of_node = pdev->dev.parent->of_node;
 #endif
 
+	max77620_gpio_initialize(mgpio);
 	platform_set_drvdata(pdev, mgpio);
 
 	ret = devm_gpiochip_add_data(&pdev->dev, &mgpio->gpio_chip, mgpio);
-- 
2.26.0

Re: [PATCH v1 1/5] gpio: max77620: Initialize interrupts state

[Andy Shevchenko]

On Wed, Jul 8, 2020 at 11:29 AM Dmitry Osipenko <digetx@gmail.com> wrote:
>
> I noticed on Nexus 7 that after rebooting from downstream kernel to
> upstream, the GPIO interrupt is triggering non-stop despite of interrupts
> being disabled for all of GPIOs. This happens because Nexus 7 uses a
> soft-reboot, meaning that bootloader should take care of resetting
> hardware, but bootloader doesn't do it well. In a result, GPIO interrupt
> may be left ON at a boot time. Let's mask all GPIO interrupts at the
> driver's probe time in order to resolve the issue.

...

> +               err = regmap_update_bits(mgpio->rmap, GPIO_REG_ADDR(i),
> +                                        MAX77620_CNFG_GPIO_INT_MASK, 0);
> +               if (err < 0)

Does ' < 0' meaningful here?

> +                       dev_err(mgpio->dev, "failed to disable interrupt: %d\n",
> +                               err);

One line.

...

> +       max77620_gpio_initialize(mgpio);

I guess we have special callback for that, i.e.
https://elixir.bootlin.com/linux/v5.8-rc3/C/ident/init_hw.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/5] gpio: max77620: Initialize interrupts state

[Andy Shevchenko]

On Wed, Jul 8, 2020 at 11:51 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Wed, Jul 8, 2020 at 11:29 AM Dmitry Osipenko <digetx@gmail.com> wrote:

...

> > +       max77620_gpio_initialize(mgpio);
>
> I guess we have special callback for that, i.e.
> https://elixir.bootlin.com/linux/v5.8-rc3/C/ident/init_hw.

Sorry, here is correct link

https://elixir.bootlin.com/linux/v5.8-rc3/source/include/linux/gpio/driver.h#L212

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/5] gpio: max77620: Initialize interrupts state

[Dmitry Osipenko]

08.07.2020 11:53, Andy Shevchenko пишет:
> On Wed, Jul 8, 2020 at 11:51 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Wed, Jul 8, 2020 at 11:29 AM Dmitry Osipenko <digetx@gmail.com> wrote:
> 
> ...
> 
>>> +       max77620_gpio_initialize(mgpio);
>>
>> I guess we have special callback for that, i.e.
>> https://elixir.bootlin.com/linux/v5.8-rc3/C/ident/init_hw.
> 
> Sorry, here is correct link
> 
> https://elixir.bootlin.com/linux/v5.8-rc3/source/include/linux/gpio/driver.h#L212
> 

Indeed, I missed the init_hw(), thank you!

Re: [PATCH v1 1/5] gpio: max77620: Initialize interrupts state

[Dmitry Osipenko]

08.07.2020 11:51, Andy Shevchenko пишет:
> On Wed, Jul 8, 2020 at 11:29 AM Dmitry Osipenko <digetx@gmail.com> wrote:
>>
>> I noticed on Nexus 7 that after rebooting from downstream kernel to
>> upstream, the GPIO interrupt is triggering non-stop despite of interrupts
>> being disabled for all of GPIOs. This happens because Nexus 7 uses a
>> soft-reboot, meaning that bootloader should take care of resetting
>> hardware, but bootloader doesn't do it well. In a result, GPIO interrupt
>> may be left ON at a boot time. Let's mask all GPIO interrupts at the
>> driver's probe time in order to resolve the issue.
> 
> ...
> 
>> +               err = regmap_update_bits(mgpio->rmap, GPIO_REG_ADDR(i),
>> +                                        MAX77620_CNFG_GPIO_INT_MASK, 0);
>> +               if (err < 0)
> 
> Does ' < 0' meaningful here?

Not really, although [1] explicitly says that regmap_update_bits()
returns either 0 or a negative error code. The positive value will be an
unexpected return code here.

[1]
https://elixir.bootlin.com/linux/v5.8-rc3/source/drivers/base/regmap/regmap.c#L2910

This variant of ' < 0' is consistent with all other similar occurrences
in the driver's code, so should be better to keep it as-is, IMO.

>> +                       dev_err(mgpio->dev, "failed to disable interrupt: %d\n",
>> +                               err);
> 
> One line.

This will make this line inconsistent with the rest of the driver's code.

Secondly, this line won't fit to display using my multi-file view-edit
setup.

I know that 80 chars isn't warned by checkpatch anymore, but still it's
a preferred width for all cases where it doesn't hurt readability, which
is the case here, IMO.

[PATCH v1 2/5] gpio: max77620: Replace 8 with MAX77620_GPIO_NR

[Dmitry Osipenko]

The MAX77620_GPIO_NR enum value represents the total number of GPIOs,
let's use it instead of a raw value in order to improve the code's
readability a tad.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/gpio/gpio-max77620.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-max77620.c b/drivers/gpio/gpio-max77620.c
index 473b4e900bbb..6a7ede6b8b74 100644
--- a/drivers/gpio/gpio-max77620.c
+++ b/drivers/gpio/gpio-max77620.c
@@ -19,8 +19,8 @@ struct max77620_gpio {
 	struct regmap		*rmap;
 	struct device		*dev;
 	struct mutex		buslock; /* irq_bus_lock */
-	unsigned int		irq_type[8];
-	bool			irq_enabled[8];
+	unsigned int		irq_type[MAX77620_GPIO_NR];
+	bool			irq_enabled[MAX77620_GPIO_NR];
 };
 
 static irqreturn_t max77620_gpio_irqhandler(int irq, void *data)
@@ -38,7 +38,7 @@ static irqreturn_t max77620_gpio_irqhandler(int irq, void *data)
 
 	pending = value;
 
-	for_each_set_bit(offset, &pending, 8) {
+	for_each_set_bit(offset, &pending, MAX77620_GPIO_NR) {
 		unsigned int virq;
 
 		virq = irq_find_mapping(gpio->gpio_chip.irq.domain, offset);
-- 
2.26.0

[PATCH v1 3/5] gpio: max77620: Replace interrupt-enable array with bitmap

[Dmitry Osipenko]

There is no need to dedicate an array where a bitmap could be used.
Let's replace the interrupt's enable-array with the enable-mask in order
to improve the code a tad.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/gpio/gpio-max77620.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpio-max77620.c b/drivers/gpio/gpio-max77620.c
index 6a7ede6b8b74..dd83c16a1ec6 100644
--- a/drivers/gpio/gpio-max77620.c
+++ b/drivers/gpio/gpio-max77620.c
@@ -5,6 +5,7 @@
  * Copyright (c) 2016, NVIDIA CORPORATION.  All rights reserved.
  */
 
+#include <linux/bitops.h>
 #include <linux/gpio/driver.h>
 #include <linux/interrupt.h>
 #include <linux/mfd/max77620.h>
@@ -20,7 +21,7 @@ struct max77620_gpio {
 	struct device		*dev;
 	struct mutex		buslock; /* irq_bus_lock */
 	unsigned int		irq_type[MAX77620_GPIO_NR];
-	bool			irq_enabled[MAX77620_GPIO_NR];
+	unsigned long		irq_enb_mask;
 };
 
 static irqreturn_t max77620_gpio_irqhandler(int irq, void *data)
@@ -53,7 +54,7 @@ static void max77620_gpio_irq_mask(struct irq_data *data)
 	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
 	struct max77620_gpio *gpio = gpiochip_get_data(chip);
 
-	gpio->irq_enabled[data->hwirq] = false;
+	clear_bit(data->hwirq, &gpio->irq_enb_mask);
 }
 
 static void max77620_gpio_irq_unmask(struct irq_data *data)
@@ -61,7 +62,7 @@ static void max77620_gpio_irq_unmask(struct irq_data *data)
 	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
 	struct max77620_gpio *gpio = gpiochip_get_data(chip);
 
-	gpio->irq_enabled[data->hwirq] = true;
+	set_bit(data->hwirq, &gpio->irq_enb_mask);
 }
 
 static int max77620_gpio_set_irq_type(struct irq_data *data, unsigned int type)
@@ -108,7 +109,10 @@ static void max77620_gpio_bus_sync_unlock(struct irq_data *data)
 	unsigned int value, offset = data->hwirq;
 	int err;
 
-	value = gpio->irq_enabled[offset] ? gpio->irq_type[offset] : 0;
+	if (test_bit(offset, &gpio->irq_enb_mask))
+		value = gpio->irq_type[offset];
+	else
+		value = 0;
 
 	err = regmap_update_bits(gpio->rmap, GPIO_REG_ADDR(offset),
 				 MAX77620_CNFG_GPIO_INT_MASK, value);
-- 
2.26.0

Re: [PATCH v1 3/5] gpio: max77620: Replace interrupt-enable array with bitmap

[Andy Shevchenko]

On Wed, Jul 8, 2020 at 11:30 AM Dmitry Osipenko <digetx@gmail.com> wrote:
>
> There is no need to dedicate an array where a bitmap could be used.
> Let's replace the interrupt's enable-array with the enable-mask in order
> to improve the code a tad.

...

> +#include <linux/bitops.h>

>         unsigned int            irq_type[MAX77620_GPIO_NR];
> -       bool                    irq_enabled[MAX77620_GPIO_NR];
> +       unsigned long           irq_enb_mask;

I would rather to move to DECLARE_BITMAP()
(the macro is defined in types.h IIRC)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 3/5] gpio: max77620: Replace interrupt-enable array with bitmap

[Dmitry Osipenko]

08.07.2020 11:44, Andy Shevchenko пишет:
> On Wed, Jul 8, 2020 at 11:30 AM Dmitry Osipenko <digetx@gmail.com> wrote:
>>
>> There is no need to dedicate an array where a bitmap could be used.
>> Let's replace the interrupt's enable-array with the enable-mask in order
>> to improve the code a tad.
> 
> ...
> 
>> +#include <linux/bitops.h>
> 
>>         unsigned int            irq_type[MAX77620_GPIO_NR];
>> -       bool                    irq_enabled[MAX77620_GPIO_NR];
>> +       unsigned long           irq_enb_mask;
> 
> I would rather to move to DECLARE_BITMAP()
> (the macro is defined in types.h IIRC)
> 

Hello, Andy! I know about DECLARE_BITMAP(), it is a very useful macro
for bitmaps that are over 32 bits, which is absolutely not the case
here. This macro will make code more difficult to read and then we will
have to use the bitmap API, which is unnecessary overhead for this case,
and thus, it won't be an improvement anymore, IMO.

I'd either keep this patch as-is or drop it.

[PATCH v1 4/5] gpio: max77620: Don't handle disabled interrupts

[Dmitry Osipenko]

Check whether GPIO IRQ is enabled before proceeding with handling the
interrupt request. The interrupt handler now returns IRQ_NONE if none
of interrupts were handled, which is usually a sign of a problem.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/gpio/gpio-max77620.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-max77620.c b/drivers/gpio/gpio-max77620.c
index dd83c16a1ec6..8d54bc4307c2 100644
--- a/drivers/gpio/gpio-max77620.c
+++ b/drivers/gpio/gpio-max77620.c
@@ -37,7 +37,9 @@ static irqreturn_t max77620_gpio_irqhandler(int irq, void *data)
 		return IRQ_NONE;
 	}
 
-	pending = value;
+	pending = value & gpio->irq_enb_mask;
+	if (!pending)
+		return IRQ_NONE;
 
 	for_each_set_bit(offset, &pending, MAX77620_GPIO_NR) {
 		unsigned int virq;
-- 
2.26.0

Re: [PATCH v1 4/5] gpio: max77620: Don't handle disabled interrupts

[Andy Shevchenko]

On Wed, Jul 8, 2020 at 11:29 AM Dmitry Osipenko <digetx@gmail.com> wrote:
>
> Check whether GPIO IRQ is enabled before proceeding with handling the
> interrupt request. The interrupt handler now returns IRQ_NONE if none
> of interrupts were handled, which is usually a sign of a problem.

...

> -       pending = value;
> +       pending = value & gpio->irq_enb_mask;

> +       if (!pending)
> +               return IRQ_NONE;

for_each_set_bit() should take care of it, no?
(and probably return with IRQ_RETVAL() macro)

>         for_each_set_bit(offset, &pending, MAX77620_GPIO_NR) {
>                 unsigned int virq;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 4/5] gpio: max77620: Don't handle disabled interrupts

[Dmitry Osipenko]

08.07.2020 11:46, Andy Shevchenko пишет:
> On Wed, Jul 8, 2020 at 11:29 AM Dmitry Osipenko <digetx@gmail.com> wrote:
>>
>> Check whether GPIO IRQ is enabled before proceeding with handling the
>> interrupt request. The interrupt handler now returns IRQ_NONE if none
>> of interrupts were handled, which is usually a sign of a problem.
> 
> ...
> 
>> -       pending = value;
>> +       pending = value & gpio->irq_enb_mask;
> 
>> +       if (!pending)
>> +               return IRQ_NONE;
> 
> for_each_set_bit() should take care of it, no?

Do you mean that the handle_nested_irq() takes care of handling
unrequested interrupts? Actually, looks like it cares. Alright, I'll
drop this patch since it should be unnecessary. Thank you for the comment!

> (and probably return with IRQ_RETVAL() macro)
> 
>>         for_each_set_bit(offset, &pending, MAX77620_GPIO_NR) {
>>                 unsigned int virq;
> 

Re: [PATCH v1 4/5] gpio: max77620: Don't handle disabled interrupts

[Andy Shevchenko]

On Wed, Jul 8, 2020 at 12:19 PM Dmitry Osipenko <digetx@gmail.com> wrote:
> 08.07.2020 11:46, Andy Shevchenko пишет:
> > On Wed, Jul 8, 2020 at 11:29 AM Dmitry Osipenko <digetx@gmail.com> wrote:
> >>
> >> Check whether GPIO IRQ is enabled before proceeding with handling the
> >> interrupt request. The interrupt handler now returns IRQ_NONE if none
> >> of interrupts were handled, which is usually a sign of a problem.
> >
> > ...
> >
> >> -       pending = value;
> >> +       pending = value & gpio->irq_enb_mask;
> >
> >> +       if (!pending)
> >> +               return IRQ_NONE;
> >
> > for_each_set_bit() should take care of it, no?
>
> Do you mean that the handle_nested_irq() takes care of handling
> unrequested interrupts? Actually, looks like it cares. Alright, I'll
> drop this patch since it should be unnecessary. Thank you for the comment!

I think it's still good to have reduced IRQs to handle by dropping not
enabled ones, my comment was about the case when pending == 0. Sorry
if it was unclear.

> > (and probably return with IRQ_RETVAL() macro)
> >
> >>         for_each_set_bit(offset, &pending, MAX77620_GPIO_NR) {
> >>                 unsigned int virq;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 4/5] gpio: max77620: Don't handle disabled interrupts

[Dmitry Osipenko]

08.07.2020 13:11, Andy Shevchenko пишет:
> On Wed, Jul 8, 2020 at 12:19 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>> 08.07.2020 11:46, Andy Shevchenko пишет:
>>> On Wed, Jul 8, 2020 at 11:29 AM Dmitry Osipenko <digetx@gmail.com> wrote:
>>>>
>>>> Check whether GPIO IRQ is enabled before proceeding with handling the
>>>> interrupt request. The interrupt handler now returns IRQ_NONE if none
>>>> of interrupts were handled, which is usually a sign of a problem.
>>>
>>> ...
>>>
>>>> -       pending = value;
>>>> +       pending = value & gpio->irq_enb_mask;
>>>
>>>> +       if (!pending)
>>>> +               return IRQ_NONE;
>>>
>>> for_each_set_bit() should take care of it, no?
>>
>> Do you mean that the handle_nested_irq() takes care of handling
>> unrequested interrupts? Actually, looks like it cares. Alright, I'll
>> drop this patch since it should be unnecessary. Thank you for the comment!
> 
> I think it's still good to have reduced IRQs to handle by dropping not
> enabled ones, my comment was about the case when pending == 0. Sorry
> if it was unclear.

It should be unnecessary since we now see that the handle_nested_irq()
checks whether interrupt was requested and if it wasn't, then particular
GPIO interrupt will be treated as spurious [1]. The pending == 0
condition is an extreme case, I don't think that there is a need to
optimize it without any good reason.

[1] https://elixir.bootlin.com/linux/v5.8-rc3/source/kernel/irq/chip.c#L485

Hence it should be better to drop this patch.

Re: [PATCH v1 4/5] gpio: max77620: Don't handle disabled interrupts

[Andy Shevchenko]

On Wed, Jul 8, 2020 at 1:54 PM Dmitry Osipenko <digetx@gmail.com> wrote:
> 08.07.2020 13:11, Andy Shevchenko пишет:

...

> It should be unnecessary since we now see that the handle_nested_irq()
> checks whether interrupt was requested and if it wasn't, then particular
> GPIO interrupt will be treated as spurious [1]. The pending == 0
> condition is an extreme case, I don't think that there is a need to
> optimize it without any good reason.
>
> [1] https://elixir.bootlin.com/linux/v5.8-rc3/source/kernel/irq/chip.c#L485
>
> Hence it should be better to drop this patch.

Fine with me, thanks!

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 5/5] gpio: max77620: Move variable declaration

[Dmitry Osipenko]

Let's move the virq variable declaration to a top-level scope just to
make the code a bit more visually appealing.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/gpio/gpio-max77620.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-max77620.c b/drivers/gpio/gpio-max77620.c
index 8d54bc4307c2..6861980da0d8 100644
--- a/drivers/gpio/gpio-max77620.c
+++ b/drivers/gpio/gpio-max77620.c
@@ -27,7 +27,7 @@ struct max77620_gpio {
 static irqreturn_t max77620_gpio_irqhandler(int irq, void *data)
 {
 	struct max77620_gpio *gpio = data;
-	unsigned int value, offset;
+	unsigned int value, offset, virq;
 	unsigned long pending;
 	int err;
 
@@ -42,8 +42,6 @@ static irqreturn_t max77620_gpio_irqhandler(int irq, void *data)
 		return IRQ_NONE;
 
 	for_each_set_bit(offset, &pending, MAX77620_GPIO_NR) {
-		unsigned int virq;
-
 		virq = irq_find_mapping(gpio->gpio_chip.irq.domain, offset);
 		handle_nested_irq(virq);
 	}
-- 
2.26.0

Re: [PATCH v1 5/5] gpio: max77620: Move variable declaration

[Andy Shevchenko]

On Wed, Jul 8, 2020 at 11:29 AM Dmitry Osipenko <digetx@gmail.com> wrote:
>
> Let's move the virq variable declaration to a top-level scope just to
> make the code a bit more visually appealing.

To me it sounds like unneeded churn, but it's up to maintainers.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 5/5] gpio: max77620: Move variable declaration

[Dmitry Osipenko]

08.07.2020 11:47, Andy Shevchenko пишет:
> On Wed, Jul 8, 2020 at 11:29 AM Dmitry Osipenko <digetx@gmail.com> wrote:
>>
>> Let's move the virq variable declaration to a top-level scope just to
>> make the code a bit more visually appealing.
> 
> To me it sounds like unneeded churn, but it's up to maintainers.
> 

I agree that this is an unnecessary change, but I couldn't resist :)
Alright, I'll skip the patches 3-5 in the v2. Thank you very much for
the review!