On Fri, 10 Jun 2022 22:01:09 +0200 Nuno Sá <noname.nuno@xxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxx> > > 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.
