Skip to contentSkip to conversation

Improvements for MAX77620 GPIO driver

PATCH Superseded Dmitry Osipenko 2 participants 19 messages Jul 8, 2020

Subsystems (2)

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore
#0 lore raw

cover Improvements for MAX77620 GPIO driver

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(-)
Signature · 1 line
2.26.0
#1 lore raw

PATCH 1/5 gpio: max77620: Initialize interrupts state

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.

---
 drivers/gpio/gpio-max77620.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
drivers/gpio/gpio-max77620.c +20 -0
index 313bd02dd893..473b4e900bbb 100644
@@ -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);
Signature · 1 line
2.26.0
#2 lore raw

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

Quoted 8 lines · 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.

...

drivers/gpio/gpio-max77620.c static int max77620_gpio_set_config(struct gpio_chip *gc, unsigned int offset, · line 276

Quoted 3 lines
drivers/gpio/gpio-max77620.c +3 -0
+               err = regmap_update_bits(mgpio->rmap, GPIO_REG_ADDR(i),
+                                        MAX77620_CNFG_GPIO_INT_MASK, 0);
+               if (err < 0)

Does ' < 0' meaningful here?

drivers/gpio/gpio-max77620.c static int max77620_gpio_set_config(struct gpio_chip *gc, unsigned int offset, · line 278

Quoted 2 lines
drivers/gpio/gpio-max77620.c +2 -0
+                       dev_err(mgpio->dev, "failed to disable interrupt: %d\n",
+                               err);

One line.

...

Quoted 1 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.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#3 lore raw

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

Quoted 1 line · Andy Shevchenko

On Wed, Jul 8, 2020 at 11:29 AM Dmitry Osipenko <[email protected]> wrote:

...

Quoted 4 lines
Quoted 1 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.

Sorry, here is correct link

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

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#4 lore raw

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

08.07.2020 11:53, Andy Shevchenko пишет:

Quoted 6 lines
Quoted 1 line · Andy Shevchenko

On Wed, Jul 8, 2020 at 11:29 AM Dmitry Osipenko <[email protected]> wrote:

...

Quoted 9 lines
Quoted 4 lines
Quoted 1 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.

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!

#5 lore raw

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

In reply to Andy Shevchenko

08.07.2020 11:51, Andy Shevchenko пишет:

Quoted 10 lines · Dmitry Osipenko
Quoted 8 lines

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.

Quoted 7 lines

...

drivers/gpio/gpio-max77620.c static int max77620_gpio_set_config(struct gpio_chip *gc, unsigned int offset, · line 276

Quoted 3 lines
drivers/gpio/gpio-max77620.c +3 -0
+               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.

Quoted 4 lines

drivers/gpio/gpio-max77620.c static int max77620_gpio_set_config(struct gpio_chip *gc, unsigned int offset, · line 278

Quoted 2 lines
drivers/gpio/gpio-max77620.c +2 -0
+                       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.

#6 lore raw

PATCH 2/5 gpio: max77620: Replace 8 with MAX77620_GPIO_NR

In reply to 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.

---
 drivers/gpio/gpio-max77620.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/gpio/gpio-max77620.c +3 -3
index 473b4e900bbb..6a7ede6b8b74 100644
@@ -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);
Signature · 1 line
2.26.0
#7 lore raw

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

In reply to 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.

---
 drivers/gpio/gpio-max77620.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)
drivers/gpio/gpio-max77620.c +8 -4
index 6a7ede6b8b74..dd83c16a1ec6 100644
@@ -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);
Signature · 1 line
2.26.0
#8 lore raw

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

Quoted 4 lines · 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.

...

drivers/gpio/gpio-max77620.c struct max77620_gpio { · line 24

Quoted 5 lines
drivers/gpio/gpio-max77620.c +2 -1
+#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)

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#9 lore raw

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

08.07.2020 11:44, Andy Shevchenko пишет:

Quoted 8 lines · Dmitry Osipenko
Quoted 4 lines

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.

...

Quoted 9 lines

drivers/gpio/gpio-max77620.c struct max77620_gpio { · line 24

Quoted 5 lines
drivers/gpio/gpio-max77620.c +2 -1
+#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.

#10 lore raw

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

In reply to 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.

---
 drivers/gpio/gpio-max77620.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/gpio/gpio-max77620.c +3 -1
index dd83c16a1ec6..8d54bc4307c2 100644
@@ -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;
Signature · 1 line
2.26.0
#11 lore raw

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

Quoted 4 lines · 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.

...

drivers/gpio/gpio-max77620.c static irqreturn_t max77620_gpio_irqhandler(int irq, void *data)

Quoted 5 lines
drivers/gpio/gpio-max77620.c +3 -1
-       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)

Quoted 2 lines
        for_each_set_bit(offset, &pending, MAX77620_GPIO_NR) {
                unsigned int virq;
Signature · 2 lines
With Best Regards,
Andy Shevchenko
#12 lore raw

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

08.07.2020 11:46, Andy Shevchenko пишет:

Quoted 8 lines · Dmitry Osipenko
Quoted 4 lines

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.

...

Quoted 7 lines

drivers/gpio/gpio-max77620.c static irqreturn_t max77620_gpio_irqhandler(int irq, void *data)

Quoted 5 lines
drivers/gpio/gpio-max77620.c +3 -1
-       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!

Quoted 5 lines

(and probably return with IRQ_RETVAL() macro)

Quoted 2 lines
        for_each_set_bit(offset, &pending, MAX77620_GPIO_NR) {
                unsigned int virq;
#13 lore raw

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

Quoted 12 lines · Dmitry Osipenko

08.07.2020 11:46, Andy Shevchenko пишет:

Quoted 11 lines · Dmitry Osipenko
Quoted 4 lines

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.

...

Quoted 2 lines
drivers/gpio/gpio-max77620.c +1 -1
-       pending = value;
+       pending = value & gpio->irq_enb_mask;
Quoted 8 lines
Quoted 4 lines

drivers/gpio/gpio-max77620.c static irqreturn_t max77620_gpio_irqhandler(int irq, void *data) · line 42

Quoted 2 lines
drivers/gpio/gpio-max77620.c +2 -0
+       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.

Quoted 4 lines
Quoted 4 lines

(and probably return with IRQ_RETVAL() macro)

Quoted 2 lines
        for_each_set_bit(offset, &pending, MAX77620_GPIO_NR) {
                unsigned int virq;
Signature · 2 lines
With Best Regards,
Andy Shevchenko
#14 lore raw

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

08.07.2020 13:11, Andy Shevchenko пишет:

Quoted 18 lines · Dmitry Osipenko
Quoted 17 lines

08.07.2020 11:46, Andy Shevchenko пишет:

Quoted 15 lines · Dmitry Osipenko
Quoted 4 lines

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.

...

drivers/gpio/gpio-max77620.c static irqreturn_t max77620_gpio_irqhandler(int irq, void *data)

Quoted 5 lines
drivers/gpio/gpio-max77620.c +3 -1
-       pending = value;
+       pending = value & gpio->irq_enb_mask;
 
+       if (!pending)
+               return IRQ_NONE;

for_each_set_bit() should take care of it, no?

Quoted 7 lines
Quoted 3 lines

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.

#15 lore raw

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

Quoted 1 line · Dmitry Osipenko

08.07.2020 13:11, Andy Shevchenko пишет:

...

Quoted 9 lines

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!

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#16 lore raw

PATCH 5/5 gpio: max77620: Move variable declaration

In reply to Dmitry Osipenko

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

---
 drivers/gpio/gpio-max77620.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/gpio/gpio-max77620.c +1 -3
index 8d54bc4307c2..6861980da0d8 100644
@@ -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);
 	}
Signature · 1 line
2.26.0
#17 lore raw

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

Quoted 3 lines · Dmitry Osipenko

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.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#18 lore raw

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

08.07.2020 11:47, Andy Shevchenko пишет:

Quoted 7 lines · Dmitry Osipenko
Quoted 3 lines

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!