diff mbox series

[v1,1/1] pinctrl: renesas: rzg2l: Replace of_node_to_fwnode() with more suitable API

Message ID 20240822230104.707812-1-andy.shevchenko@gmail.com
State New
Headers show
Series [v1,1/1] pinctrl: renesas: rzg2l: Replace of_node_to_fwnode() with more suitable API | expand

Commit Message

Andy Shevchenko Aug. 22, 2024, 11:01 p.m. UTC
of_node_to_fwnode() is a IRQ domain specific implementation of
of_fwnode_handle(). Replace the former with more suitable API.

Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/pinctrl/renesas/pinctrl-rzg2l.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Geert Uytterhoeven Aug. 23, 2024, 7:23 a.m. UTC | #1
Hi Andy,

On Fri, Aug 23, 2024 at 1:01 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> of_node_to_fwnode() is a IRQ domain specific implementation of
> of_fwnode_handle(). Replace the former with more suitable API.
>
> Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Thanks for your patch!

> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -16,6 +16,7 @@
>  #include <linux/of.h>
>  #include <linux/of_irq.h>
>  #include <linux/platform_device.h>
> +#include <linux/property.h>
>  #include <linux/seq_file.h>
>  #include <linux/spinlock.h>
>
> @@ -2624,7 +2625,7 @@ static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl)
>
>         girq = &chip->irq;
>         gpio_irq_chip_set_chip(girq, &rzg2l_gpio_irqchip);
> -       girq->fwnode = of_node_to_fwnode(np);
> +       girq->fwnode = dev_fwnode(pctrl->dev);

While this looks correct, the new call goes through many more hoops, and
is not a simple inline function.

Perhaps just &np->fwnode? ;-)

>         girq->parent_domain = parent_domain;
>         girq->child_to_parent_hwirq = rzg2l_gpio_child_to_parent_hwirq;
>         girq->populate_parent_alloc_arg = rzg2l_gpio_populate_parent_fwspec;

Gr{oetje,eeting}s,

                        Geert
Andy Shevchenko Aug. 23, 2024, 1:17 p.m. UTC | #2
On Fri, Aug 23, 2024 at 10:23 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Fri, Aug 23, 2024 at 1:01 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > of_node_to_fwnode() is a IRQ domain specific implementation of
> > of_fwnode_handle(). Replace the former with more suitable API.

...

> > -       girq->fwnode = of_node_to_fwnode(np);
> > +       girq->fwnode = dev_fwnode(pctrl->dev);
>
> While this looks correct, the new call goes through many more hoops, and
> is not a simple inline function.

Maybe, but it's not a hot path anyway.

> Perhaps just &np->fwnode? ;-)

Definitely not for a couple of reasons:
- the explicit dereferencing may prevent from easier cleaning up and
moving the fwnode members around in the driver core
- the GPIO library internally doesn't use OF node directly, so the
code that call GPIO library would be better to follow that

Additionally one can call of_fwnode_handle(), but going here and there
with it makes no sense as it much cleaner to see that this fwnode
comes directly from the device. This is not obvious from the original
or any code that uses np.

After all the idea at minimum to isolate of_node_to_fwnode() from all,
but native IRQ chips (basically there are two big users: native IRQ
chips and PCI MSI).

P.S. Also note, it's _the only_ pin control driver that uses that API.
Lad, Prabhakar Aug. 23, 2024, 2:05 p.m. UTC | #3
On Fri, Aug 23, 2024 at 12:42 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> of_node_to_fwnode() is a IRQ domain specific implementation of
> of_fwnode_handle(). Replace the former with more suitable API.
>
> Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> ---
>  drivers/pinctrl/renesas/pinctrl-rzg2l.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Cheers,
Prabhakar

> diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> index 5e3d735a8570..73b55e096106 100644
> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -16,6 +16,7 @@
>  #include <linux/of.h>
>  #include <linux/of_irq.h>
>  #include <linux/platform_device.h>
> +#include <linux/property.h>
>  #include <linux/seq_file.h>
>  #include <linux/spinlock.h>
>
> @@ -2624,7 +2625,7 @@ static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl)
>
>         girq = &chip->irq;
>         gpio_irq_chip_set_chip(girq, &rzg2l_gpio_irqchip);
> -       girq->fwnode = of_node_to_fwnode(np);
> +       girq->fwnode = dev_fwnode(pctrl->dev);
>         girq->parent_domain = parent_domain;
>         girq->child_to_parent_hwirq = rzg2l_gpio_child_to_parent_hwirq;
>         girq->populate_parent_alloc_arg = rzg2l_gpio_populate_parent_fwspec;
> --
> 2.46.0
>
>
Geert Uytterhoeven Aug. 29, 2024, 12:51 p.m. UTC | #4
Hi Andy,

On Fri, Aug 23, 2024 at 3:17 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Fri, Aug 23, 2024 at 10:23 AM Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> > On Fri, Aug 23, 2024 at 1:01 AM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > of_node_to_fwnode() is a IRQ domain specific implementation of
> > > of_fwnode_handle(). Replace the former with more suitable API.
>
> ...
>
> > > -       girq->fwnode = of_node_to_fwnode(np);
> > > +       girq->fwnode = dev_fwnode(pctrl->dev);
> >
> > While this looks correct, the new call goes through many more hoops, and
> > is not a simple inline function.

[...]

> P.S. Also note, it's _the only_ pin control driver that uses that API.

Thanks for the explanation!

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
i.e. will queue in renesas-pinctrl for v6.12.

Gr{oetje,eeting}s,

                        Geert
diff mbox series

Patch

diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
index 5e3d735a8570..73b55e096106 100644
--- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
+++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
@@ -16,6 +16,7 @@ 
 #include <linux/of.h>
 #include <linux/of_irq.h>
 #include <linux/platform_device.h>
+#include <linux/property.h>
 #include <linux/seq_file.h>
 #include <linux/spinlock.h>
 
@@ -2624,7 +2625,7 @@  static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl)
 
 	girq = &chip->irq;
 	gpio_irq_chip_set_chip(girq, &rzg2l_gpio_irqchip);
-	girq->fwnode = of_node_to_fwnode(np);
+	girq->fwnode = dev_fwnode(pctrl->dev);
 	girq->parent_domain = parent_domain;
 	girq->child_to_parent_hwirq = rzg2l_gpio_child_to_parent_hwirq;
 	girq->populate_parent_alloc_arg = rzg2l_gpio_populate_parent_fwspec;