[PATCH v2 0/2] Improvements for MAX77620 GPIO driver

[PATCH v2 0/2] 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, it also makes a small improvement to the code.

Changelog:

v2: - Addressed review comment that were made by Andy Shevchenko to v1:

        - Generic init_hw() callback is used now for resetting interrupts.

        - These v1 patches are dropped:

           gpio: max77620: Replace interrupt-enable array with bitmap
           gpio: max77620: Don't handle disabled interrupts
           gpio: max77620: Move variable declaration

Dmitry Osipenko (2):
  gpio: max77620: Initialize interrupts state
  gpio: max77620: Replace 8 with MAX77620_GPIO_NR

 drivers/gpio/gpio-max77620.c | 31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

-- 
2.26.0

[PATCH v2 1/2] 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 initialization time in order to resolve the issue.

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

diff --git a/drivers/gpio/gpio-max77620.c b/drivers/gpio/gpio-max77620.c
index 313bd02dd893..970ad6397a56 100644
--- a/drivers/gpio/gpio-max77620.c
+++ b/drivers/gpio/gpio-max77620.c
@@ -260,6 +260,30 @@ static int max77620_gpio_set_config(struct gpio_chip *gc, unsigned int offset,
 	return -ENOTSUPP;
 }
 
+static int max77620_gpio_irq_init_hw(struct gpio_chip *gc)
+{
+	struct max77620_gpio *gpio = gpiochip_get_data(gc);
+	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
+	 * the interrupts.
+	 */
+	for (i = 0; i < MAX77620_GPIO_NR; i++) {
+		err = regmap_update_bits(gpio->rmap, GPIO_REG_ADDR(i),
+					 MAX77620_CNFG_GPIO_INT_MASK, 0);
+		if (err < 0) {
+			dev_err(gpio->dev,
+				"failed to disable interrupt: %d\n", err);
+			return err;
+		}
+	}
+
+	return 0;
+}
+
 static int max77620_gpio_probe(struct platform_device *pdev)
 {
 	struct max77620_chip *chip =  dev_get_drvdata(pdev->dev.parent);
@@ -288,6 +312,7 @@ static int max77620_gpio_probe(struct platform_device *pdev)
 	mgpio->gpio_chip.ngpio = MAX77620_GPIO_NR;
 	mgpio->gpio_chip.can_sleep = 1;
 	mgpio->gpio_chip.base = -1;
+	mgpio->gpio_chip.irq.init_hw = max77620_gpio_irq_init_hw,
 #ifdef CONFIG_OF_GPIO
 	mgpio->gpio_chip.of_node = pdev->dev.parent->of_node;
 #endif
-- 
2.26.0

[PATCH v2 2/2] 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 970ad6397a56..08bd5b141437 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

Re: [PATCH v2 2/2] gpio: max77620: Replace 8 with MAX77620_GPIO_NR

[Andy Shevchenko]

On Wed, Jul 8, 2020 at 6:58 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>
> 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.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> 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 970ad6397a56..08bd5b141437 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
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] gpio: max77620: Initialize interrupts state

[Andy Shevchenko]

On Wed, Jul 8, 2020 at 6:58 PM 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

despite 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

but the bootloader

As a result

> may be left ON at a boot time. Let's mask all GPIO interrupts at the
> driver's initialization time in order to resolve the issue.

...

>         mgpio->gpio_chip.ngpio = MAX77620_GPIO_NR;
>         mgpio->gpio_chip.can_sleep = 1;
>         mgpio->gpio_chip.base = -1;
> +       mgpio->gpio_chip.irq.init_hw = max77620_gpio_irq_init_hw,

Now this seems a bit awkward. Perhaps you need first to switch to use
GPIO IRQ chip structure?
It's also in the TODO of the GPIO subsystem ;-)

...

>  #ifdef CONFIG_OF_GPIO
>         mgpio->gpio_chip.of_node = pdev->dev.parent->of_node;
>  #endif

This seems to be done by GPIO library.

Also point to improve: don't shadow error from platform_get_irq().

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] gpio: max77620: Initialize interrupts state

[Dmitry Osipenko]

08.07.2020 19:58, Andy Shevchenko пишет:
> On Wed, Jul 8, 2020 at 6:58 PM 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
> 
> despite 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
> 
> but the bootloader
> 
> As a result

Thanks!

>> may be left ON at a boot time. Let's mask all GPIO interrupts at the
>> driver's initialization time in order to resolve the issue.
> 
> ...
> 
>>         mgpio->gpio_chip.ngpio = MAX77620_GPIO_NR;
>>         mgpio->gpio_chip.can_sleep = 1;
>>         mgpio->gpio_chip.base = -1;
>> +       mgpio->gpio_chip.irq.init_hw = max77620_gpio_irq_init_hw,
> 
> Now this seems a bit awkward. Perhaps you need first to switch to use
> GPIO IRQ chip structure?
> It's also in the TODO of the GPIO subsystem ;-)

Alright, I'll try to switch it to use the GPIO IRQ chip struct in v3!

> ...
> 
>>  #ifdef CONFIG_OF_GPIO
>>         mgpio->gpio_chip.of_node = pdev->dev.parent->of_node;
>>  #endif
> 
> This seems to be done by GPIO library.

Indeed!

> Also point to improve: don't shadow error from platform_get_irq().
> 

Good catch!