Improvements for MAX77620 GPIO driver
Subsystems (2)
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
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
@@ -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
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
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
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!
Re: [PATCH v1 1/5] gpio: max77620: Initialize interrupts state
In reply to Andy Shevchenko08.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.
PATCH 2/5 gpio: max77620: Replace 8 with MAX77620_GPIO_NR
In reply to Dmitry OsipenkoThe 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
@@ -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
PATCH 3/5 gpio: max77620: Replace interrupt-enable array with bitmap
In reply to Dmitry OsipenkoThere 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
@@ -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
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
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 intypes.hIIRC)
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 4/5 gpio: max77620: Don't handle disabled interrupts
In reply to Dmitry OsipenkoCheck 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
@@ -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
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_NONEif 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
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_NONEif 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;
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_NONEif 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
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_NONEif 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.
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
PATCH 5/5 gpio: max77620: Move variable declaration
In reply to Dmitry OsipenkoLet'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
@@ -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
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
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!