[PATCH 24/34] iio: inkern: move to fwnode properties
Jonathan Cameron
jic23 at kernel.org
Sun Jun 12 01:32:57 AEST 2022
On Sat, 11 Jun 2022 16:30:57 +0100
Jonathan Cameron <jic23 at kernel.org> wrote:
> On Fri, 10 Jun 2022 22:01:09 +0200
> Nuno Sá <noname.nuno at 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 at 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 at 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).
Having said that adding a warning comment here, so we remember this is
a potential issue would be great.
>
> 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.
>
>
More information about the openbmc
mailing list