diff mbox series

[v4,11/13] pinctrl: meson: Replace custom code by gpiochip_node_count() call

Message ID 20220401103604.8705-12-andriy.shevchenko@linux.intel.com (mailing list archive)
State Awaiting Upstream
Delegated to: Geert Uytterhoeven
Headers show
Series gpiolib: Two new helpers and way toward fwnode | expand

Commit Message

Andy Shevchenko April 1, 2022, 10:36 a.m. UTC
Since we have generic function to count GPIO controller nodes
under a given device, there is no need to open code it. Replace
custom code by gpiochip_node_count() call.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/pinctrl/meson/pinctrl-meson.c | 28 ++++++++++++---------------
 1 file changed, 12 insertions(+), 16 deletions(-)

Comments

Marek Szyprowski April 14, 2022, 6:38 a.m. UTC | #1
Hi

On 01.04.2022 12:36, Andy Shevchenko wrote:
> Since we have generic function to count GPIO controller nodes
> under a given device, there is no need to open code it. Replace
> custom code by gpiochip_node_count() call.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>

This patch landed in linux next-20220413 as commit 88834c75cae5 
("pinctrl: meson: Replace custom code by gpiochip_node_count() call"). 
Unfortunately it breaks booting of all my Amlogic-based test boards 
(Odroid C4, N2, Khadas VIM3, VIM3l). MMC driver is no longer probed and 
boards are unable to mount rootfs. Reverting this patch on top of 
linux-next fixes the issue.

> ---
>   drivers/pinctrl/meson/pinctrl-meson.c | 28 ++++++++++++---------------
>   1 file changed, 12 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/pinctrl/meson/pinctrl-meson.c b/drivers/pinctrl/meson/pinctrl-meson.c
> index 5b46a0979db7..1b078da81523 100644
> --- a/drivers/pinctrl/meson/pinctrl-meson.c
> +++ b/drivers/pinctrl/meson/pinctrl-meson.c
> @@ -49,6 +49,7 @@
>   #include <linux/pinctrl/pinctrl.h>
>   #include <linux/pinctrl/pinmux.h>
>   #include <linux/platform_device.h>
> +#include <linux/property.h>
>   #include <linux/regmap.h>
>   #include <linux/seq_file.h>
>   
> @@ -662,27 +663,22 @@ static struct regmap *meson_map_resource(struct meson_pinctrl *pc,
>   	return devm_regmap_init_mmio(pc->dev, base, &meson_regmap_config);
>   }
>   
> -static int meson_pinctrl_parse_dt(struct meson_pinctrl *pc,
> -				  struct device_node *node)
> +static int meson_pinctrl_parse_dt(struct meson_pinctrl *pc)
>   {
> -	struct device_node *np, *gpio_np = NULL;
> +	struct device_node *gpio_np;
> +	unsigned int chips;
>   
> -	for_each_child_of_node(node, np) {
> -		if (!of_find_property(np, "gpio-controller", NULL))
> -			continue;
> -		if (gpio_np) {
> -			dev_err(pc->dev, "multiple gpio nodes\n");
> -			of_node_put(np);
> -			return -EINVAL;
> -		}
> -		gpio_np = np;
> -	}
> -
> -	if (!gpio_np) {
> +	chips = gpiochip_node_count(pc->dev);
> +	if (!chips) {
>   		dev_err(pc->dev, "no gpio node found\n");
>   		return -EINVAL;
>   	}
> +	if (chips > 1) {
> +		dev_err(pc->dev, "multiple gpio nodes\n");
> +		return -EINVAL;
> +	}
>   
> +	gpio_np = to_of_node(device_get_named_child_node(pc->dev, "gpio-controller"));
>   	pc->of_node = gpio_np;
>   
>   	pc->reg_mux = meson_map_resource(pc, gpio_np, "mux");
> @@ -751,7 +747,7 @@ int meson_pinctrl_probe(struct platform_device *pdev)
>   	pc->dev = dev;
>   	pc->data = (struct meson_pinctrl_data *) of_device_get_match_data(dev);
>   
> -	ret = meson_pinctrl_parse_dt(pc, dev->of_node);
> +	ret = meson_pinctrl_parse_dt(pc);
>   	if (ret)
>   		return ret;
>   

Best regards
Andy Shevchenko April 14, 2022, 1:51 p.m. UTC | #2
On Thu, Apr 14, 2022 at 12:44 PM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
> On 01.04.2022 12:36, Andy Shevchenko wrote:
> > Since we have generic function to count GPIO controller nodes
> > under a given device, there is no need to open code it. Replace
> > custom code by gpiochip_node_count() call.

...

> This patch landed in linux next-20220413 as commit 88834c75cae5
> ("pinctrl: meson: Replace custom code by gpiochip_node_count() call").
> Unfortunately it breaks booting of all my Amlogic-based test boards
> (Odroid C4, N2, Khadas VIM3, VIM3l). MMC driver is no longer probed and
> boards are unable to mount rootfs. Reverting this patch on top of
> linux-next fixes the issue.

Thank you for letting me know, I'll withdraw it and investigate.
Martin Blumenstingl April 14, 2022, 3:32 p.m. UTC | #3
Hi Andy,

On Thu, Apr 14, 2022 at 3:51 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
[...]
> > This patch landed in linux next-20220413 as commit 88834c75cae5
> > ("pinctrl: meson: Replace custom code by gpiochip_node_count() call").
> > Unfortunately it breaks booting of all my Amlogic-based test boards
> > (Odroid C4, N2, Khadas VIM3, VIM3l). MMC driver is no longer probed and
> > boards are unable to mount rootfs. Reverting this patch on top of
> > linux-next fixes the issue.
>
> Thank you for letting me know, I'll withdraw it and investigate.
If needed I can investigate further later today/tomorrow. I think the
problem is that our node name doesn't follow the .dts recommendation.

For GXL (arch/arm64/boot/dts/amlogic/meson-gxl.dtsi) the GPIO
controller nodes are for example:
  gpio: bank@4b0 {
      ...
  }
and
  gpio_ao: bank@14 {
      ...
  }

See also:
$ git grep -C6 gpio-controller arch/arm64/boot/dts/amlogic/*.dtsi

Marek did not state which error he's getting but I suspect it fails
with "no gpio node found".


Best regards,
Martin
Andy Shevchenko April 14, 2022, 4:06 p.m. UTC | #4
On Thu, Apr 14, 2022 at 6:32 PM Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
> On Thu, Apr 14, 2022 at 3:51 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> [...]
> > > This patch landed in linux next-20220413 as commit 88834c75cae5
> > > ("pinctrl: meson: Replace custom code by gpiochip_node_count() call").
> > > Unfortunately it breaks booting of all my Amlogic-based test boards
> > > (Odroid C4, N2, Khadas VIM3, VIM3l). MMC driver is no longer probed and
> > > boards are unable to mount rootfs. Reverting this patch on top of
> > > linux-next fixes the issue.
> >
> > Thank you for letting me know, I'll withdraw it and investigate.
> If needed I can investigate further later today/tomorrow. I think the
> problem is that our node name doesn't follow the .dts recommendation.
>
> For GXL (arch/arm64/boot/dts/amlogic/meson-gxl.dtsi) the GPIO
> controller nodes are for example:
>   gpio: bank@4b0 {
>       ...
>   }
> and
>   gpio_ao: bank@14 {
>       ...
>   }
>
> See also:
> $ git grep -C6 gpio-controller arch/arm64/boot/dts/amlogic/*.dtsi
>
> Marek did not state which error he's getting but I suspect it fails
> with "no gpio node found".

Would be interesting to know that, yeah.

The subtle difference between the patched and unpatched version is
that the former uses only available nodes, it means that node is not
available by some reason and then the error would be the one you
guessed.
Andy Shevchenko April 14, 2022, 6:28 p.m. UTC | #5
On Thu, Apr 14, 2022 at 07:06:21PM +0300, Andy Shevchenko wrote:
> On Thu, Apr 14, 2022 at 6:32 PM Martin Blumenstingl
> <martin.blumenstingl@googlemail.com> wrote:
> > On Thu, Apr 14, 2022 at 3:51 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > [...]
> > > > This patch landed in linux next-20220413 as commit 88834c75cae5
> > > > ("pinctrl: meson: Replace custom code by gpiochip_node_count() call").
> > > > Unfortunately it breaks booting of all my Amlogic-based test boards
> > > > (Odroid C4, N2, Khadas VIM3, VIM3l). MMC driver is no longer probed and
> > > > boards are unable to mount rootfs. Reverting this patch on top of
> > > > linux-next fixes the issue.
> > >
> > > Thank you for letting me know, I'll withdraw it and investigate.
> > If needed I can investigate further later today/tomorrow. I think the
> > problem is that our node name doesn't follow the .dts recommendation.
> >
> > For GXL (arch/arm64/boot/dts/amlogic/meson-gxl.dtsi) the GPIO
> > controller nodes are for example:
> >   gpio: bank@4b0 {
> >       ...
> >   }
> > and
> >   gpio_ao: bank@14 {
> >       ...
> >   }
> >
> > See also:
> > $ git grep -C6 gpio-controller arch/arm64/boot/dts/amlogic/*.dtsi
> >
> > Marek did not state which error he's getting but I suspect it fails
> > with "no gpio node found".
> 
> Would be interesting to know that, yeah.
> 
> The subtle difference between the patched and unpatched version is
> that the former uses only available nodes, it means that node is not
> available by some reason and then the error would be the one you
> guessed.

Looking into the difference between iterating via available nodes I have found
nothing suspicious. Your DTSes do not have status property, so it assumes the
node is available.

I'm quite puzzled what's going on there. Because I can't see what the logical
difference the patch brought in.

P.S. In any case it's withdrawn now and shouldn't appear in the next Linux
Next. But I would really appreciate more input on this.
Andy Shevchenko April 14, 2022, 6:33 p.m. UTC | #6
On Thu, Apr 14, 2022 at 09:28:30PM +0300, Andy Shevchenko wrote:
> On Thu, Apr 14, 2022 at 07:06:21PM +0300, Andy Shevchenko wrote:
> > On Thu, Apr 14, 2022 at 6:32 PM Martin Blumenstingl
> > <martin.blumenstingl@googlemail.com> wrote:
> > > On Thu, Apr 14, 2022 at 3:51 PM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > > [...]
> > > > > This patch landed in linux next-20220413 as commit 88834c75cae5
> > > > > ("pinctrl: meson: Replace custom code by gpiochip_node_count() call").
> > > > > Unfortunately it breaks booting of all my Amlogic-based test boards
> > > > > (Odroid C4, N2, Khadas VIM3, VIM3l). MMC driver is no longer probed and
> > > > > boards are unable to mount rootfs. Reverting this patch on top of
> > > > > linux-next fixes the issue.
> > > >
> > > > Thank you for letting me know, I'll withdraw it and investigate.
> > > If needed I can investigate further later today/tomorrow. I think the
> > > problem is that our node name doesn't follow the .dts recommendation.
> > >
> > > For GXL (arch/arm64/boot/dts/amlogic/meson-gxl.dtsi) the GPIO
> > > controller nodes are for example:
> > >   gpio: bank@4b0 {
> > >       ...
> > >   }
> > > and
> > >   gpio_ao: bank@14 {
> > >       ...
> > >   }
> > >
> > > See also:
> > > $ git grep -C6 gpio-controller arch/arm64/boot/dts/amlogic/*.dtsi
> > >
> > > Marek did not state which error he's getting but I suspect it fails
> > > with "no gpio node found".
> > 
> > Would be interesting to know that, yeah.
> > 
> > The subtle difference between the patched and unpatched version is
> > that the former uses only available nodes, it means that node is not
> > available by some reason and then the error would be the one you
> > guessed.
> 
> Looking into the difference between iterating via available nodes I have found
> nothing suspicious. Your DTSes do not have status property, so it assumes the
> node is available.
> 
> I'm quite puzzled what's going on there. Because I can't see what the logical
> difference the patch brought in.
> 
> P.S. In any case it's withdrawn now and shouldn't appear in the next Linux
> Next. But I would really appreciate more input on this.

Okay, now I got it. The "name" of the node is not the same as containing the
property with a given name. So, the faulting line of the code is this one:

gpio_np = to_of_node(device_get_named_child_node(pc->dev, "gpio-controller"));
diff mbox series

Patch

diff --git a/drivers/pinctrl/meson/pinctrl-meson.c b/drivers/pinctrl/meson/pinctrl-meson.c
index 5b46a0979db7..1b078da81523 100644
--- a/drivers/pinctrl/meson/pinctrl-meson.c
+++ b/drivers/pinctrl/meson/pinctrl-meson.c
@@ -49,6 +49,7 @@ 
 #include <linux/pinctrl/pinctrl.h>
 #include <linux/pinctrl/pinmux.h>
 #include <linux/platform_device.h>
+#include <linux/property.h>
 #include <linux/regmap.h>
 #include <linux/seq_file.h>
 
@@ -662,27 +663,22 @@  static struct regmap *meson_map_resource(struct meson_pinctrl *pc,
 	return devm_regmap_init_mmio(pc->dev, base, &meson_regmap_config);
 }
 
-static int meson_pinctrl_parse_dt(struct meson_pinctrl *pc,
-				  struct device_node *node)
+static int meson_pinctrl_parse_dt(struct meson_pinctrl *pc)
 {
-	struct device_node *np, *gpio_np = NULL;
+	struct device_node *gpio_np;
+	unsigned int chips;
 
-	for_each_child_of_node(node, np) {
-		if (!of_find_property(np, "gpio-controller", NULL))
-			continue;
-		if (gpio_np) {
-			dev_err(pc->dev, "multiple gpio nodes\n");
-			of_node_put(np);
-			return -EINVAL;
-		}
-		gpio_np = np;
-	}
-
-	if (!gpio_np) {
+	chips = gpiochip_node_count(pc->dev);
+	if (!chips) {
 		dev_err(pc->dev, "no gpio node found\n");
 		return -EINVAL;
 	}
+	if (chips > 1) {
+		dev_err(pc->dev, "multiple gpio nodes\n");
+		return -EINVAL;
+	}
 
+	gpio_np = to_of_node(device_get_named_child_node(pc->dev, "gpio-controller"));
 	pc->of_node = gpio_np;
 
 	pc->reg_mux = meson_map_resource(pc, gpio_np, "mux");
@@ -751,7 +747,7 @@  int meson_pinctrl_probe(struct platform_device *pdev)
 	pc->dev = dev;
 	pc->data = (struct meson_pinctrl_data *) of_device_get_match_data(dev);
 
-	ret = meson_pinctrl_parse_dt(pc, dev->of_node);
+	ret = meson_pinctrl_parse_dt(pc);
 	if (ret)
 		return ret;