Improvements for MAX77620 GPIO driver
Subsystems (2)
v4 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 improvements to the code, fixes the non-released interrupt bug and converts driver to use irqchip template.
Changelog:
v4: - Added stable-tag to the patch "Fix missing release of interrupt".
- Added acks and r-bs from Laxman Dewangan and Andy Shevchenko.
v3: - Addressed review comment that were made by Andy Shevchenko to v2:
- Improved the commit message of the "Initialize hardware state of interrupts" patch.
- Added these new patches:
gpio: max77620: Don't set of_node
gpio: max77620: Don't shadow error code of platform_get_irq()
gpio: max77620: Use irqchip template
- Added "Fix missing release of interrupt" patch.
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 (6):
gpio: max77620: Replace 8 with MAX77620_GPIO_NR
gpio: max77620: Fix missing release of interrupt
gpio: max77620: Don't set of_node
gpio: max77620: Don't shadow error code of platform_get_irq()
gpio: max77620: Use irqchip template
gpio: max77620: Initialize hardware state of interrupts
drivers/gpio/gpio-max77620.c | 65 ++++++++++++++++++++++++------------ 1 file changed, 44 insertions(+), 21 deletions(-)
Signature · 1 line
2.26.0
PATCH v4 1/6 gpio: max77620: Replace 8 with MAX77620_GPIO_NR
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
@@ -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 v4 2/6 gpio: max77620: Fix missing release of interrupt
In reply to Dmitry OsipenkoThe requested interrupt is never released by the driver. Fix this by using the resource-managed variant of request_threaded_irq().
--- drivers/gpio/gpio-max77620.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/gpio/gpio-max77620.c +3 -2
@@ -305,8 +305,9 @@ static int max77620_gpio_probe(struct platform_device *pdev)gpiochip_irqchip_add_nested (&mgpio->gpio_chip, &max77620_gpio_irqchip,0 , handle_edge_irq,IRQ_TYPE_NONE ); - ret =request_threaded_irq (gpio_irq,NULL , max77620_gpio_irqhandler, -IRQF_ONESHOT ,"max77620-gpio" , mgpio); + ret =devm_request_threaded_irq (&pdev->dev, gpio_irq,NULL , + max77620_gpio_irqhandler,IRQF_ONESHOT , +"max77620-gpio" , mgpio);if (ret <0 ) {dev_err (&pdev->dev,"failed to request IRQ: %d\n" , ret);return ret;
Signature · 1 line
2.26.0
PATCH v4 3/6 gpio: max77620: Don't set of_node
In reply to Dmitry OsipenkoThe gpiochip_add_data() takes care of setting the of_node to the parent's device of_node, hence there is no need to do it manually in the driver's code. This patch corrects the parent's device pointer and removes the unnecessary setting of the of_node.
--- drivers/gpio/gpio-max77620.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
drivers/gpio/gpio-max77620.c +1 -4
@@ -279,7 +279,7 @@ static int max77620_gpio_probe(struct platform_device *pdev) mgpio->dev = &pdev->dev; mgpio->gpio_chip.label = pdev->name; - mgpio->gpio_chip.parent = &pdev->dev; + mgpio->gpio_chip.parent = pdev->dev.parent; mgpio->gpio_chip.direction_input = max77620_gpio_dir_input; mgpio->gpio_chip.get = max77620_gpio_get; mgpio->gpio_chip.direction_output = max77620_gpio_dir_output;
@@ -288,9 +288,6 @@ 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 ; -#ifdef CONFIG_OF_GPIO - mgpio->gpio_chip.of_node = pdev->dev.parent->of_node; -#endif platform_set_drvdata (pdev, mgpio);
Signature · 1 line
2.26.0
PATCH v4 4/6 gpio: max77620: Don't shadow error code of platform_get_irq()
In reply to Dmitry OsipenkoThe platform_get_irq() returns a positive interrupt number on success and negative error code on failure (zero shouldn't ever happen in practice, it would produce a noisy warning). Hence let's return the error code directly instead of overriding it with -ENODEV.
--- drivers/gpio/gpio-max77620.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
drivers/gpio/gpio-max77620.c +6 -4
@@ -264,12 +264,14 @@ static int max77620_gpio_probe(struct platform_device *pdev) {struct max77620_chip *chip =dev_get_drvdata (pdev->dev.parent);struct max77620_gpio *mgpio; -int gpio_irq; +unsigned int gpio_irq;int ret; - gpio_irq =platform_get_irq (pdev,0 ); -if (gpio_irq <=0 ) -return -ENODEV ; + ret =platform_get_irq (pdev,0 ); +if (ret <0 ) +return ret; + + gpio_irq = ret; mgpio =devm_kzalloc (&pdev->dev,sizeof (*mgpio),GFP_KERNEL );if (!mgpio)
Signature · 1 line
2.26.0
PATCH v4 5/6 gpio: max77620: Use irqchip template
In reply to Dmitry OsipenkoThis change addresses one of the GPIO-core TODOs for the MAX77620 driver which requires modern drivers to use the irqchip template. Instead of using the GPIO's irqchip-helpers for creating the IRQ domain, the gpio_irq_chip structure is now filled by the driver itself and then gpiochip_add_data() takes care of instantiating the IRQ domain for us.
--- drivers/gpio/gpio-max77620.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)
drivers/gpio/gpio-max77620.c +6 -8
@@ -277,6 +277,7 @@ static int max77620_gpio_probe(struct platform_device *pdev)if (!mgpio)return -ENOMEM ; +mutex_init (&mgpio->buslock); mgpio->rmap = chip->rmap; mgpio->dev = &pdev->dev;
@@ -291,6 +292,11 @@ static int max77620_gpio_probe(struct platform_device *pdev) mgpio->gpio_chip.can_sleep =1 ; mgpio->gpio_chip.base =-1 ; + mgpio->gpio_chip.irq.chip = &max77620_gpio_irqchip; + mgpio->gpio_chip.irq.default_type =IRQ_TYPE_NONE ; + mgpio->gpio_chip.irq.handler = handle_edge_irq; + mgpio->gpio_chip.irq.threaded = true; +platform_set_drvdata (pdev, mgpio); ret =devm_gpiochip_add_data (&pdev->dev, &mgpio->gpio_chip, mgpio);
@@ -299,11 +305,6 @@ static int max77620_gpio_probe(struct platform_device *pdev)return ret; } -mutex_init (&mgpio->buslock); - -gpiochip_irqchip_add_nested (&mgpio->gpio_chip, &max77620_gpio_irqchip, -0 , handle_edge_irq,IRQ_TYPE_NONE ); - ret =devm_request_threaded_irq (&pdev->dev, gpio_irq,NULL , max77620_gpio_irqhandler,IRQF_ONESHOT ,"max77620-gpio" , mgpio);
@@ -312,9 +313,6 @@ static int max77620_gpio_probe(struct platform_device *pdev)return ret; } -gpiochip_set_nested_irqchip (&mgpio->gpio_chip, &max77620_gpio_irqchip, - gpio_irq); -return 0 ; }
Signature · 1 line
2.26.0
Re: [PATCH v4 5/6] gpio: max77620: Use irqchip template
drivers/gpio/gpio-max77620.c static int max77620_gpio_probe(struct platform_device *pdev) · line 298
Quoted 4 lines · Dmitry Osipenko
drivers/gpio/gpio-max77620.c +4
-0+ mgpio->gpio_chip.irq.chip = &max77620_gpio_irqchip; + mgpio->gpio_chip.irq.default_type =IRQ_TYPE_NONE ; + mgpio->gpio_chip.irq.handler = handle_edge_irq; + mgpio->gpio_chip.irq.threaded = true;
And I didn't even realize one could do this.
Quoted 3 lines
drivers/gpio/gpio-max77620.c +0
-3-gpiochip_irqchip_add_nested (&mgpio->gpio_chip, &max77620_gpio_irqchip, -0 , handle_edge_irq,IRQ_TYPE_NONE ); -
(...)
drivers/gpio/gpio-max77620.c static int max77620_gpio_probe(struct platform_device *pdev) · line 316
Quoted 2 lines
drivers/gpio/gpio-max77620.c +0
-2-gpiochip_set_nested_irqchip (&mgpio->gpio_chip, &max77620_gpio_irqchip, - gpio_irq);
And get rid of these two.
I suppose I can just do the same for the remaining users of gpiochip_irqchip_add_nested() and gpiochip_set_nested_irqchip() and get rid of these two functions altogether!
Yours,
Linus Walleij
PATCH v4 6/6 gpio: max77620: Initialize hardware state of interrupts
In reply to Dmitry OsipenkoI noticed on Nexus 7 that after rebooting from downstream kernel to upstream, the GPIO interrupt is triggering non-stop 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 the bootloader doesn't do it well. As 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);
@@ -295,6 +319,7 @@ static int max77620_gpio_probe(struct platform_device *pdev) mgpio->gpio_chip.irq.chip = &max77620_gpio_irqchip; mgpio->gpio_chip.irq.default_type =IRQ_TYPE_NONE ; mgpio->gpio_chip.irq.handler = handle_edge_irq; + mgpio->gpio_chip.irq.init_hw = max77620_gpio_irq_init_hw, mgpio->gpio_chip.irq.threaded = true;platform_set_drvdata (pdev, mgpio);
Signature · 1 line
2.26.0
Quoted 8 lines · Dmitry Osipenko
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 improvements to the code, fixes the non-released interrupt bug and converts driver to use irqchip template.
Changelog:
v4: - Added stable-tag to the patch "Fix missing release of interrupt".
This v4 series applied, thanks a *LOT* for your patient work on this!
I need to fix the USB port on my Nexus 7 so I can test how the mainline support is working these days!
Yours,
Linus Walleij
16.07.2020 11:52, Linus Walleij пишет:
Quoted 7 lines · Dmitry Osipenko
Quoted 6 lines
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 improvements to the code, fixes the non-released interrupt bug and converts driver to use irqchip template.
Quoted 5 lines
Quoted 3 lines
Changelog:
v4: - Added stable-tag to the patch "Fix missing release of interrupt".
This v4 series applied, thanks a *LOT* for your patient work on this!
Hello, Linus! Thank you for applying the patches!
Quoted 2 lines
I need to fix the USB port on my Nexus 7 so I can test how the mainline support is working these days!
Please notice that this should be a Nexus 7 2012, which is NVIDIA Tegra30-based Nexus 7.
The mainline support just starting to emerge for the 2012 model, although not much is missed in the upstream. There are couple patches currently under review which are necessary for Nexus 7 2012 in order to make it ready for everyday use, like a tiny patch for the touchscreen support and DRM bridges/panel orientation improvements for the Tegra DRM driver. We're pretty close! :)