Re: [PATCH 24/34] iio: inkern: move to fwnode properties

From: Jonathan Cameron <jic23@kernel.org>
To: "Nuno Sá" <noname.nuno@gmail.com>
Cc: "Andy Shevchenko" <andy.shevchenko@gmail.com>,
	"Nuno Sá" <nuno.sa@analog.com>, dl-linux-imx <linux-imx@nxp.com>,
	Linux-Renesas <linux-renesas-soc@vger.kernel.org>,
	"open list:BROADCOM NVRAM DRIVER" <linux-mips@vger.kernel.org>,
	"linux-arm Mailing List" <linux-arm-kernel@lists.infradead.org>,
	chrome-platform@lists.linux.dev,
	"Lad Prabhakar" <prabhakar.mahadev-lad.rj@bp.renesas.com>,
	"moderated list:ARM/Mediatek SoC support"
	<linux-mediatek@lists.infradead.org>,
	linux-stm32@st-md-mailman.stormreply.com,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	linux-iio <linux-iio@vger.kernel.org>,
	"OpenBMC Maillist" <openbmc@lists.ozlabs.org>,
	"Cai Huoqing" <cai.huoqing@linux.dev>,
	"Benjamin Fair" <benjaminfair@google.com>,
	"Jishnu Prakash" <quic_jprakash@quicinc.com>,
	"Linus Walleij" <linus.walleij@linaro.org>,
	"Lars-Peter Clausen" <lars@metafoo.de>,
	"Alexandre Torgue" <alexandre.torgue@foss.st.com>,
	"Amit Kucheria" <amitk@kernel.org>,
	"Andy Gross" <agross@kernel.org>,
	"Michael Hennerich" <Michael.Hennerich@analog.com>,
	"Haibo Chen" <haibo.chen@nxp.com>,
	"Benson Leung" <bleung@chromium.org>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	"Alexandre Belloni" <alexandre.belloni@bootlin.com>,
	"Christophe Branchereau" <cbranchereau@gmail.com>,
	"Patrick Venture" <venture@google.com>,
	"Arnd Bergmann" <arnd@arndb.de>, "Nancy Yuen" <yuenn@google.com>,
	"Sascha Hauer" <s.hauer@pengutronix.de>,
	"Daniel Lezcano" <daniel.lezcano@linaro.org>,
	"Gwendal Grignou" <gwendal@chromium.org>,
	"Saravanan Sekar" <sravanhome@gmail.com>,
	"Tali Perry" <tali.perry1@gmail.com>,
	"Maxime Coquelin" <mcoquelin.stm32@gmail.com>,
	"Paul Cercueil" <paul@crapouillou.net>,
	"Thara Gopinath" <thara.gopinath@linaro.org>,
	"Avi Fishman" <avifishman70@gmail.com>,
	"Lorenzo Bianconi" <lorenzo@kernel.org>,
	"Claudiu Beznea" <claudiu.beznea@microchip.com>,
	"Pengutronix Kernel Team" <kernel@pengutronix.de>,
	"Fabrice Gasnier" <fabrice.gasnier@foss.st.com>,
	"Matthias Brugger" <matthias.bgg@gmail.com>,
	"Tomer Maimon" <tmaimon77@gmail.com>,
	"Bjorn Andersson" <bjorn.andersson@linaro.org>,
	"Nicolas Ferre" <nicolas.ferre@microchip.com>,
	"Zhang Rui" <rui.zhang@intel.com>,
	"Shawn Guo" <shawnguo@kernel.org>,
	"Guenter Roeck" <groeck@chromium.org>,
	"Fabio Estevam" <festevam@gmail.com>,
	"Olivier Moysan" <olivier.moysan@foss.st.com>,
	"Eugen Hristev" <eugen.hristev@microchip.com>,
	"Miquel Raynal" <miquel.raynal@bootlin.com>
Subject: Re: [PATCH 24/34] iio: inkern: move to fwnode properties
Date: Sat, 11 Jun 2022 16:30:57 +0100	[thread overview]
Message-ID: <20220611163057.2e2606aa@jic23-huawei> (raw)
In-Reply-To: <97b9278953d923008a4c1230ca9bd354117e7213.camel@gmail.com>

On Fri, 10 Jun 2022 22:01:09 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Fri, 2022-06-10 at 17:19 +0200, Andy Shevchenko wrote:
> > On Fri, Jun 10, 2022 at 10:48 AM Nuno Sá <nuno.sa@analog.com> wrote:  
> > > 
> > > This moves the IIO in kernel interface to use fwnode properties and
> > > thus
> > > be firmware agnostic.
> > > 
> > > Note that the interface is still not firmware agnostic. At this
> > > point we
> > > have both OF and fwnode interfaces so that we don't break any user.
> > > On
> > > top of this we also want to have a per driver conversion and that
> > > is the
> > > main reason we have both of_xlate() and fwnode_xlate() support.  
> > 
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Thanks!
> > 
> > A few nit-picks below, though.
> >   
...

> 
> >   
> > > -       err = of_parse_phandle_with_args(np, "io-channels",
> > > -                                        "#io-channel-cells",
> > > -                                        index, &iiospec);
> > > +       err = fwnode_property_get_reference_args(fwnode, "io-
> > > channels",
> > > +                                                "#io-channel-
> > > cells", 0,
> > > +                                                index, &iiospec);
> > >         if (err)
> > >                 return err;
> > > 
> > > -       idev = bus_find_device(&iio_bus_type, NULL, iiospec.np,
> > > +       idev = bus_find_device(&iio_bus_type, NULL, iiospec.fwnode,
> > >                                iio_dev_node_match);  
> > 
> > Wondering if this
> > https://elixir.bootlin.com/linux/v5.19-rc1/C/ident/bus_find_device_by_fwnode
> > can be utilized (yes, I noticed iio_device_type above).  
> 
> Hmm, at first glance I would say we can use it. AFAICT, we are already
> grabbing a node which contains "#io-channel-cells" which is very
> indicative that is an IIO device. I also find it very unlikely to have
> two IIO devices with the same fwnode (I guess it would be an issue even
> in the old code) and even more unlikely two devices of diferent types
> with the same fwnode?

If we are talking struct iio_dev instances, then there are quite a few cases
where there are multiple with the same fwnode.  We had to do that pre
multiple buffers being introduced so it's fairly common, though not in
ADCs which is probably why we haven't seen breakage here. Not sure how
broken things already are as a result or if any of those devices (most
IMUs) provide #io-channel-cells etc.  I'd put that breakage down as
one to look into if anyone every hits it or one of us is bored enough
to poke at it.  (superficially I think we'd have to check all matches
for an xlate success).

Also, possible (I'm not totally sure) that we have other subdevices using
the same firmware node, such as triggers.  I can't immediately think
of why they would need it, but I'd rather we were at least partly protected
against that.

> 
> Anyways, I guess Jonathan can help in here...
> 
> 
> >   
> > >         if (idev == NULL) {
> > > -               of_node_put(iiospec.np);
> > > +               fwnode_handle_put(iiospec.fwnode);
> > >                 return -EPROBE_DEFER;
> > >         }
> > > 
> > >         indio_dev = dev_to_iio_dev(idev);
> > >         channel->indio_dev = indio_dev;
> > >         if (indio_dev->info->of_xlate)
> > > -               index = indio_dev->info->of_xlate(indio_dev,
> > > &iiospec);
> > > +               index = __fwnode_to_of_xlate(indio_dev, &iiospec);
> > > +       else if (indio_dev->info->fwnode_xlate)
> > > +               index = indio_dev->info->fwnode_xlate(indio_dev,
> > > &iiospec);
> > >         else
> > > -               index = __of_iio_simple_xlate(indio_dev, &iiospec);
> > > -       of_node_put(iiospec.np);
> > > +               index = __fwnode_iio_simple_xlate(indio_dev,
> > > &iiospec);
> > > +       fwnode_handle_put(iiospec.fwnode);
> > >         if (index < 0)
> > >                 goto err_put;
> > >         channel->channel = &indio_dev->channels[index];
> > > @@ -188,7 +209,8 @@ static int __of_iio_channel_get(struct
> > > iio_channel *channel,
> > >         return index;
> > >  }

> >   
> > >                 *parent_lookup = false;
> > >         }
> > > 
> > >         return chan;
> > >  }
> > > 
> > > -struct iio_channel *of_iio_channel_get_by_name(struct device_node
> > > *np,
> > > -                                              const char *name)
> > > +struct iio_channel *fwnode_iio_channel_get_by_name(struct
> > > fwnode_handle *fwnode,
> > > +                                                  const char
> > > *name)
> > >  {
> > >         struct iio_channel *chan;
> > > +       struct fwnode_handle *parent;
> > >         bool parent_lookup = true;
> > > 
> > >         /* Walk up the tree of devices looking for a matching iio
> > > channel */
> > > -       chan = __of_iio_channel_get_by_name(np, name,
> > > &parent_lookup);
> > > +       chan = __fwnode_iio_channel_get_by_name(fwnode, name,
> > > &parent_lookup);
> > >         if (!parent_lookup)
> > >                 return chan;
> > > 
> > > @@ -255,33 +279,34 @@ struct iio_channel
> > > *of_iio_channel_get_by_name(struct device_node *np,
> > >          * If the parent node has a "io-channel-ranges" property,
> > >          * then we can try one of its channels.
> > >          */
> > > -       np = np->parent;
> > > -       while (np) {
> > > -               if (!of_get_property(np, "io-channel-ranges",
> > > NULL))
> > > +       fwnode_for_each_parent_node(fwnode, parent) {
> > > +               if (!fwnode_property_present(parent, "io-channel-
> > > ranges")) {
> > > +                       fwnode_handle_put(parent);
> > >                         return chan;  
> > 
> > break; ?  
> 
> The return in place was a request from Jonathan in the RFC...

:)  I prefer not having to scroll down when we can get out quickly.

> 
> > 
> > (Yes, I understand pros and cons of each variant, up to you)
> >   
> > > +               }
> > > 
> > > -               chan = __of_iio_channel_get_by_name(np, name,
> > > &parent_lookup);
> > > -               if (!parent_lookup)
> > > +               chan = __fwnode_iio_channel_get_by_name(parent,
> > > name, &parent_lookup);
> > > +               if (!parent_lookup) {
> > > +                       fwnode_handle_put(parent);
> > >                         return chan;  
> > 
> > Ditto.
> >   
> > > -               np = np->parent;
> > > +               }
> > >         }
> > > 
> > >         return chan;
> > >  }
> > > -EXPORT_SYMBOL_GPL(of_iio_channel_get_by_name);
> > > +EXPORT_SYMBOL_GPL(fwnode_iio_channel_get_by_name);  
> > 
> > Wondering if we may move this to the IIO namespace.  
> 
> I guess it makes sense but surely on a different patch...

Yup - moving to a IIO namespace is a work in progress (got snarled
up by the PM related macros needed for some of the sub namespaces
which is now sorted)  Let's do it after this.