Improvements for MAX77620 GPIO driver
Subsystems (2)
v2 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, 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(-)
Signature · 1 line
2.26.0
PATCH v2 1/2 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 initialization time in order to resolve the issue.
--- drivers/gpio/gpio-max77620.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)
drivers/gpio/gpio-max77620.c +25 -0
@@ -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
Signature · 1 line
2.26.0
Re: [PATCH v2 1/2] gpio: max77620: Initialize interrupts state
Quoted 3 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
despite interrupts
Quoted 3 lines
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
Quoted 2 lines
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.
...
Quoted 4 lines
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 ;-)
...
Quoted 3 lines
#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().
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v2 1/2] gpio: max77620: Initialize interrupts state
08.07.2020 19:58, Andy Shevchenko пишет:
Quoted 7 lines · Dmitry Osipenko
Quoted 3 lines
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
Quoted 7 lines
Quoted 3 lines
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!
Quoted 5 lines
Quoted 2 lines
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.
...
Quoted 8 lines
Quoted 4 lines
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!
Quoted 7 lines
...
Quoted 3 lines
#ifdef CONFIG_OF_GPIO mgpio->gpio_chip.of_node = pdev->dev.parent->of_node;#endif
This seems to be done by GPIO library.
Indeed!
Quoted 2 lines
Also point to improve: don't shadow error from
platform_get_irq().
Good catch!
PATCH v2 2/2 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
Re: [PATCH v2 2/2] gpio: max77620: Replace 8 with MAX77620_GPIO_NR
Quoted 4 lines · Dmitry Osipenko
The
MAX77620_GPIO_NRenum 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.
Quoted 32 lines
Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/gpio/gpio-max77620.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)drivers/gpio/gpio-max77620.c +3
-4@@ -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
Signature · 2 lines
With Best Regards, Andy Shevchenko