Improvements for MAX77620 GPIO driver
Subsystems (2)
v3 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:
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 v3 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
Re: [PATCH v3 1/6] gpio: max77620: Replace 8 with MAX77620_GPIO_NR
Quoted 9 lines · Dmitry Osipenko
External email: Use caution opening links or attachments
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.Reviewed-by Andy Shevchenko <[email protected]>
Looks good to me.
PATCH v3 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
Re: [PATCH v3 2/6] gpio: max77620: Fix missing release of interrupt
Quoted 8 lines · Dmitry Osipenko
External email: Use caution opening links or attachments
The requested interrupt is never released by the driver. Fix this by using the resource-managed variant of
request_threaded_irq().Fixes ab3dd9cc24d4 ("gpio: max77620: Fix interrupt handling")Signed-off-by Dmitry Osipenko <[email protected]>
Looks good to me.
Re: [PATCH v3 2/6] gpio: max77620: Fix missing release of interrupt
09.07.2020 17:57, Laxman Dewangan пишет:
Quoted 9 lines · Dmitry Osipenko
Quoted 6 lines
External email: Use caution opening links or attachments
The requested interrupt is never released by the driver. Fix this by using the resource-managed variant of
request_threaded_irq().
Quoted 6 lines
Quoted 2 lines
Fixes ab3dd9cc24d4 ("gpio: max77620: Fix interrupt handling")Signed-off-by Dmitry Osipenko <[email protected]>Looks good to me.
Acked-by Laxman Dewangan <[email protected]>
Hello, Laxman! Thank you very much for taking a look at this series!
I missed to add the stable tag to this patch, so will prepare a v4 with the corrected tag. I'll also add yours acks and Andy's r-bs to the patches. Thanks!
PATCH v3 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
A message that is not in the archive (sent off-list or not yet received): CAHp75VejftNuSqdYvd1YE1SdRON6=mQ_iD2dEr4K9D8YGgeRBQ@mail.gmail.com
Re: [PATCH v3 3/6] gpio: max77620: Don't set of_node
08.07.2020 23:57, Andy Shevchenko пишет:
Quoted 12 lines
On Wednesday, July 8, 2020, Dmitry Osipenko <[email protected] <mailto:[email protected]>> wrote:
The
gpiochip_add_data()takes care of setting theof_nodeto the parent's
deviceof_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 theof_node.
Quoted 3 lines
I gave a second look and I think my suggestion is wrong. Here is an interesting propagation of the parent device node to its grand son, leaving son’s one untouched. Original code has intentions to do that way.
The [1] says that gpio_chip.parent should point at the "device providing the GPIOs". That's the pdev->dev.parent in the case of this driver. MAX77620 is an MFD PMIC device that has virtual sub-devices like GPIO controller, PINCTRL and RTC. The MFD is the parent device that provides the GPIOs [2].
[1]
https://elixir.bootlin.com/linux/v5.8-rc3/source/include/linux/gpio/driver.h#L276
[2]
https://elixir.bootlin.com/linux/v5.8-rc3/source/arch/arm64/boot/dts/nvidia/tegra210-p2180.dtsi#L48
I think the old code was wrong and this patch is correct, please correct me if I'm missing something.
Quoted 43 lines
Suggested-by: Andy Shevchenko <[email protected] <mailto:[email protected]>> Signed-off-by: Dmitry Osipenko <[email protected] <mailto:[email protected]>> --- drivers/gpio/gpio-max77620.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/drivers/gpio/gpio-max77620.c b/drivers/gpio/gpio-max77620.c index 7f7e8d4bf0d3..39d431da2dbc 100644 --- a/drivers/gpio/gpio-max77620.c +++ b/drivers/gpio/gpio-max77620.c @@ -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); -- 2.26.0Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v3 3/6] gpio: max77620: Don't set of_node
Quoted 3 lines · Dmitry Osipenko
08.07.2020 23:57, Andy Shevchenko пишет:
Quoted 2 lines
On Wednesday, July 8, 2020, Dmitry Osipenko <[email protected] <mailto:[email protected]>> wrote:
...
Quoted 6 lines
Quoted 3 lines
I gave a second look and I think my suggestion is wrong. Here is an interesting propagation of the parent device node to its grand son, leaving son’s one untouched. Original code has intentions to do that way.
The [1] says that gpio_chip.parent should point at the "device providing the GPIOs".
Yes, physical device I believe.
Quoted 5 lines
That's the
pdev->dev.parentin the case of this driver. MAX77620 is an MFD PMIC device that has virtual sub-devices like GPIO controller, PINCTRL and RTC. The MFD is the parent device that provides the GPIOs [2].
Quoted 8 lines
[1]
https://elixir.bootlin.com/linux/v5.8-rc3/source/include/linux/gpio/driver.h#L276[2]
https://elixir.bootlin.com/linux/v5.8-rc3/source/arch/arm64/boot/dts/nvidia/tegra210-p2180.dtsi#L48I think the old code was wrong and this patch is correct, please correct me if I'm missing something.
Hmm... I have checked through GPIO drivers I have knowledge of / care about and PMIC ones do like you suggested in this patch, the rest (which are instantiated from MFD) take a virtual platform device.
Looking at DT excerpt I think you're rather right than wrong, so I leave it to you and maintainers.
Thanks!
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v3 3/6] gpio: max77620: Don't set of_node
09.07.2020 12:07, Andy Shevchenko пишет:
Quoted 30 lines · Dmitry Osipenko
Quoted 3 lines
08.07.2020 23:57, Andy Shevchenko пишет:
Quoted 2 lines
On Wednesday, July 8, 2020, Dmitry Osipenko <[email protected] <mailto:[email protected]>> wrote:
...
Quoted 6 lines
Quoted 3 lines
I gave a second look and I think my suggestion is wrong. Here is an interesting propagation of the parent device node to its grand son, leaving son’s one untouched. Original code has intentions to do that way.
The [1] says that gpio_chip.parent should point at the "device providing the GPIOs".
Yes, physical device I believe.
Quoted 5 lines
That's the
pdev->dev.parentin the case of this driver. MAX77620 is an MFD PMIC device that has virtual sub-devices like GPIO controller, PINCTRL and RTC. The MFD is the parent device that provides the GPIOs [2].Quoted 8 lines
[1]
https://elixir.bootlin.com/linux/v5.8-rc3/source/include/linux/gpio/driver.h#L276[2]
https://elixir.bootlin.com/linux/v5.8-rc3/source/arch/arm64/boot/dts/nvidia/tegra210-p2180.dtsi#L48I think the old code was wrong and this patch is correct, please correct me if I'm missing something.
Quoted 7 lines
Hmm... I have checked through GPIO drivers I have knowledge of / care about and PMIC ones do like you suggested in this patch, the rest (which are instantiated from MFD) take a virtual platform device.
Looking at DT excerpt I think you're rather right than wrong, so I leave it to you and maintainers.
Thanks!
Okay, waiting for the maintainers then :)
Thank you very much for the review!
Re: [PATCH v3 3/6] gpio: max77620: Don't set of_node
In reply to Dmitry OsipenkoQuoted 11 lines · Dmitry Osipenko
External email: Use caution opening links or attachments
The
gpiochip_add_data()takes care of setting theof_nodeto the parent's deviceof_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 theof_node.Suggested-by Andy Shevchenko <[email protected]>Signed-off-by Dmitry Osipenko <[email protected]>
Looks good to me.
PATCH v3 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
Re: [PATCH v3 4/6] gpio: max77620: Don't shadow error code of platform_get_irq()
Quoted 11 lines · Dmitry Osipenko
External email: Use caution opening links or attachments
The
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.Suggested-by Andy Shevchenko <[email protected]>Signed-off-by Dmitry Osipenko <[email protected]>
Looks good to me.
PATCH v3 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 v3 5/6] gpio: max77620: Use irqchip template
Quoted 12 lines · Dmitry Osipenko
External email: Use caution opening links or attachments
This 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_chipstructure is now filled by the driver itself and thengpiochip_add_data()takes care of instantiating the IRQ domain for us.Suggested-by Andy Shevchenko <[email protected]>Signed-off-by Dmitry Osipenko <[email protected]>
Looks good to me.
PATCH v3 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
Re: [PATCH v3 6/6] gpio: max77620: Initialize hardware state of interrupts
Quoted 11 lines · Dmitry Osipenko
External email: Use caution opening links or attachments
I 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.
Quoted 2 lines
Signed-off-by Dmitry Osipenko <[email protected]>
Looks good to me.
A message that is not in the archive (sent off-list or not yet received): CAHp75VcNao84UajMYDJRH2gX7t8n=uo_Th8VVgcxJj3YkiA+pA@mail.gmail.com
09.07.2020 00:00, Andy Shevchenko пишет:
Quoted 16 lines
On Wednesday, July 8, 2020, Dmitry Osipenko <[email protected] <mailto:[email protected]>> wrote:
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.
Quoted 2 lines
Thanks! I commented on one patch, the rest looks good and you may add my Rb tag.
Thank you! :)