[PATCH v4 0/6] Improvements for MAX77620 GPIO driver

[PATCH v4 0/6] Improvements for MAX77620 GPIO driver

[Dmitry Osipenko]

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(-)

-- 
2.26.0

[PATCH v4 1/6] gpio: max77620: Replace 8 with MAX77620_GPIO_NR

[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.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Acked-by: Laxman Dewangan <ldewangan@nvidia.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/gpio/gpio-max77620.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-max77620.c b/drivers/gpio/gpio-max77620.c
index 313bd02dd893..4c0c9ec2587d 100644
--- a/drivers/gpio/gpio-max77620.c
+++ b/drivers/gpio/gpio-max77620.c
@@ -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

[PATCH v4 2/6] gpio: max77620: Fix missing release of interrupt

[Dmitry Osipenko]

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")
Cc: <stable@vger.kernel.org> # 5.5+
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Acked-by: Laxman Dewangan <ldewangan@nvidia.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/gpio/gpio-max77620.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-max77620.c b/drivers/gpio/gpio-max77620.c
index 4c0c9ec2587d..7f7e8d4bf0d3 100644
--- a/drivers/gpio/gpio-max77620.c
+++ b/drivers/gpio/gpio-max77620.c
@@ -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;
-- 
2.26.0

[PATCH v4 3/6] gpio: max77620: Don't set of_node

[Dmitry Osipenko]

The 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.

Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Acked-by: Laxman Dewangan <ldewangan@nvidia.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 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.0

[PATCH v4 4/6] gpio: max77620: Don't shadow error code of platform_get_irq()

[Dmitry Osipenko]

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 <andy.shevchenko@gmail.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Acked-by: Laxman Dewangan <ldewangan@nvidia.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/gpio/gpio-max77620.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpio-max77620.c b/drivers/gpio/gpio-max77620.c
index 39d431da2dbc..9121d2507f60 100644
--- a/drivers/gpio/gpio-max77620.c
+++ b/drivers/gpio/gpio-max77620.c
@@ -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)
-- 
2.26.0

[PATCH v4 5/6] gpio: max77620: Use irqchip template

[Dmitry Osipenko]

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_chip structure is now filled by the driver itself and then
gpiochip_add_data() takes care of instantiating the IRQ domain for us.

Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Acked-by: Laxman Dewangan <ldewangan@nvidia.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/gpio/gpio-max77620.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/gpio/gpio-max77620.c b/drivers/gpio/gpio-max77620.c
index 9121d2507f60..6c516aa7732d 100644
--- a/drivers/gpio/gpio-max77620.c
+++ b/drivers/gpio/gpio-max77620.c
@@ -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;
 }
 
-- 
2.26.0

Re: [PATCH v4 5/6] gpio: max77620: Use irqchip template

[Linus Walleij]

On Thu, Jul 9, 2020 at 7:12 PM Dmitry Osipenko <digetx@gmail.com> wrote:

> +       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.

> -       gpiochip_irqchip_add_nested(&mgpio->gpio_chip, &max77620_gpio_irqchip,
> -                                   0, handle_edge_irq, IRQ_TYPE_NONE);
> -
(...)
> -       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

[Dmitry Osipenko]

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.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Acked-by: Laxman Dewangan <ldewangan@nvidia.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/gpio/gpio-max77620.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/gpio/gpio-max77620.c b/drivers/gpio/gpio-max77620.c
index 6c516aa7732d..e090979659eb 100644
--- a/drivers/gpio/gpio-max77620.c
+++ b/drivers/gpio/gpio-max77620.c
@@ -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);
-- 
2.26.0

Re: [PATCH v4 0/6] Improvements for MAX77620 GPIO driver

[Linus Walleij]

On Thu, Jul 9, 2020 at 7:12 PM Dmitry Osipenko <digetx@gmail.com> wrote:

> 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

Re: [PATCH v4 0/6] Improvements for MAX77620 GPIO driver

[Dmitry Osipenko]

16.07.2020 11:52, Linus Walleij пишет:
> On Thu, Jul 9, 2020 at 7:12 PM Dmitry Osipenko <digetx@gmail.com> wrote:
> 
>> 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!

Hello, Linus! Thank you for applying the patches!

> 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! :)