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.
>
> > Signed-off-by: Nuno Sá <nuno.sa@xxxxxxxxxx>
> > ---
> > drivers/iio/inkern.c | 145 +++++++++++++++++++------------
> > ----
> > include/linux/iio/consumer.h | 36 +++++----
> > include/linux/iio/iio.h | 5 ++
> > 3 files changed, 105 insertions(+), 81 deletions(-)
> >
> > diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> > index dde47324b826..1d519b0cacea 100644
> > --- a/drivers/iio/inkern.c
> > +++ b/drivers/iio/inkern.c
> > @@ -5,6 +5,7 @@
> > */
> > #include <linux/err.h>
> > #include <linux/export.h>
> > +#include <linux/property.h>
> > #include <linux/slab.h>
> > #include <linux/mutex.h>
> > #include <linux/of.h>
> > @@ -117,15 +118,13 @@ static const struct iio_chan_spec
> > return chan;
> > }
> >
> > -#ifdef CONFIG_OF
> > -
> > static int iio_dev_node_match(struct device *dev, const void
> > *data)
> > {
> > - return dev->of_node == data && dev->type ==
> > &iio_device_type;
> > + return device_match_fwnode(dev, data) && dev->type ==
> > &iio_device_type;
> > }
> >
> > /**
> > - * __of_iio_simple_xlate - translate iiospec to the IIO channel
> > index
> > + * __fwnode_iio_simple_xlate - translate iiospec to the IIO
> > channel index
> > * @indio_dev: pointer to the iio_dev structure
> > * @iiospec: IIO specifier as found in the device tree
> > *
> > @@ -134,14 +133,14 @@ static int iio_dev_node_match(struct device
> > *dev, const void *data)
> > * whether IIO index is less than num_channels (that is specified
> > in the
> > * iio_dev).
> > */
> > -static int __of_iio_simple_xlate(struct iio_dev *indio_dev,
> > - const struct of_phandle_args
> > *iiospec)
> > +static int __fwnode_iio_simple_xlate(struct iio_dev *indio_dev,
> > + const struct
> > fwnode_reference_args *iiospec)
> > {
> > - if (!iiospec->args_count)
> > + if (!iiospec->nargs)
> > return 0;
> >
> > if (iiospec->args[0] >= indio_dev->num_channels) {
> > - dev_err(&indio_dev->dev, "invalid channel index
> > %u\n",
> > + dev_err(&indio_dev->dev, "invalid channel index
> > %llu\n",
> > iiospec->args[0]);
> > return -EINVAL;
> > }
> > @@ -149,34 +148,56 @@ static int __of_iio_simple_xlate(struct
> > iio_dev *indio_dev,
> > return iiospec->args[0];
> > }
> >
> > -static int __of_iio_channel_get(struct iio_channel *channel,
> > - struct device_node *np, int index)
> > +/*
> > + * Simple helper to copy fwnode_reference_args into
> > of_phandle_args so we
> > + * can pass it to of_xlate(). Ultimate goal is to drop this
> > together with
> > + * of_xlate().
> > + */
> > +static int __fwnode_to_of_xlate(struct iio_dev *indio_dev,
> > + const struct fwnode_reference_args
> > *iiospec)
> > +{
> > + struct of_phandle_args of_args;
> > + unsigned int i;
> > +
> > + of_args.args_count = iiospec->nargs;
> > + of_args.np = to_of_node(iiospec->fwnode);
> > +
> > + for (i = 0; i < MAX_PHANDLE_ARGS; i++)
> > + of_args.args[i] = i < iiospec->nargs ? iiospec-
> > >args[i] : 0;
> > +
> > + return indio_dev->info->of_xlate(indio_dev, &of_args);
> > +}
>
> Ah, now I realized that it's a bit more complicated than just
> to_of_node() :-)
>
Yeah, of_fwnode_get_reference_args() was helpfull. But I based myself
too much on it. On a second look, I guess ARRAY_SIZE(of_args.args)
would be better than MAX_PHANDLE_ARGS.
> > +static int __fwnode_iio_channel_get(struct iio_channel *channel,
> > + struct fwnode_handle *fwnode,
> > int index)
> > {
> > struct device *idev;
> > struct iio_dev *indio_dev;
> > int err;
> > - struct of_phandle_args iiospec;
> > + struct fwnode_reference_args iiospec;
>
> At the same point you can move it up in the block to make a long line
> first.
Can do that...
>
> > - 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?
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;
> > }
> >
> > -static struct iio_channel *of_iio_channel_get(struct device_node
> > *np, int index)
> > +static struct iio_channel *fwnode_iio_channel_get(struct
> > fwnode_handle *fwnode,
> > + int index)
> > {
> > struct iio_channel *channel;
> > int err;
> > @@ -200,7 +222,7 @@ static struct iio_channel
> > *of_iio_channel_get(struct device_node *np, int index)
> > if (channel == NULL)
> > return ERR_PTR(-ENOMEM);
> >
> > - err = __of_iio_channel_get(channel, np, index);
> > + err = __fwnode_iio_channel_get(channel, fwnode, index);
> > if (err)
> > goto err_free_channel;
> >
> > @@ -211,9 +233,9 @@ static struct iio_channel
> > *of_iio_channel_get(struct device_node *np, int index)
> > return ERR_PTR(err);
> > }
> >
> > -struct iio_channel *__of_iio_channel_get_by_name(struct
> > device_node *np,
> > - const char *name,
> > - bool
> > *parent_lookup)
> > +struct iio_channel *
> > +__fwnode_iio_channel_get_by_name(struct fwnode_handle *fwnode,
> > const char *name,
> > + bool *parent_lookup)
> > {
> > struct iio_channel *chan;
> > int index = 0;
> > @@ -221,32 +243,34 @@ struct iio_channel
> > *__of_iio_channel_get_by_name(struct device_node *np,
> > /*
> > * For named iio channels, first look up the name in the
> > * "io-channel-names" property. If it cannot be found, the
> > - * index will be an error code, and of_iio_channel_get()
> > + * index will be an error code, and
> > fwnode_iio_channel_get()
> > * will fail.
> > */
> > if (name)
> > - index = of_property_match_string(np, "io-channel-
> > names", name);
> > + index = fwnode_property_match_string(fwnode, "io-
> > channel-names",
> > + name);
> >
> > - chan = of_iio_channel_get(np, index);
> > + chan = fwnode_iio_channel_get(fwnode, index);
> > if (!IS_ERR(chan) || PTR_ERR(chan) == -EPROBE_DEFER) {
> > *parent_lookup = false;
> > } else if (name && index >= 0) {
> > - pr_err("ERROR: could not get IIO channel
> > %pOF:%s(%i)\n",
> > - np, name ? name : "", index);
> > + pr_err("ERROR: could not get IIO channel
> > %pfw:%s(%i)\n",
> > + fwnode, name ? name : "", index);
>
> Since you are touching this line can you switch to name ?: "" and
> possibly move some parameters to the above line?
If it does not cross the 80limit col, sure.
>
> > *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...
>
> (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...
>
> > -static struct iio_channel *of_iio_channel_get_all(struct device
> > *dev)
> > +static struct iio_channel *fwnode_iio_channel_get_all(struct
> > device *dev)
> > {
> > + struct fwnode_handle *fwnode = dev_fwnode(dev);
> > struct iio_channel *chans;
> > int i, mapind, nummaps = 0;
> > int ret;
> >
> > do {
> > - ret = of_parse_phandle_with_args(dev->of_node,
> > - "io-channels",
> > - "#io-channel-
> > cells",
> > - nummaps, NULL);
> > + ret = fwnode_property_get_reference_args(fwnode,
> > "io-channels",
> > + "#io-
> > channel-cells", 0,
> > + nummaps,
> > NULL);
> > if (ret < 0)
> > break;
> > } while (++nummaps);
> > @@ -294,10 +319,9 @@ static struct iio_channel
> > *of_iio_channel_get_all(struct device *dev)
> > if (chans == NULL)
> > return ERR_PTR(-ENOMEM);
> >
> > - /* Search for OF matches */
> > + /* Search for FW matches */
> > for (mapind = 0; mapind < nummaps; mapind++) {
> > - ret = __of_iio_channel_get(&chans[mapind], dev-
> > >of_node,
> > - mapind);
> > + ret = __fwnode_iio_channel_get(&chans[mapind],
> > fwnode, mapind);
> > if (ret)
> > goto error_free_chans;
> > }
> > @@ -310,15 +334,6 @@ static struct iio_channel
> > *of_iio_channel_get_all(struct device *dev)
> > return ERR_PTR(ret);
> > }
> >
> > -#else /* CONFIG_OF */
> > -
> > -static inline struct iio_channel *of_iio_channel_get_all(struct
> > device *dev)
> > -{
> > - return ERR_PTR(-ENODEV);
> > -}
> > -
> > -#endif /* CONFIG_OF */
> > -
> > static struct iio_channel *iio_channel_get_sys(const char *name,
> > const char
> > *channel_name)
> > {
> > @@ -379,8 +394,8 @@ struct iio_channel *iio_channel_get(struct
> > device *dev,
> > struct iio_channel *channel;
> >
> > if (dev) {
> > - channel = of_iio_channel_get_by_name(dev->of_node,
> > - channel_name);
> > + channel =
> > fwnode_iio_channel_get_by_name(dev_fwnode(dev),
> > +
> > channel_name);
> > if (!IS_ERR(channel) || PTR_ERR(channel) == -
> > EPROBE_DEFER)
> > return channel;
> > }
> > @@ -421,14 +436,14 @@ struct iio_channel
> > *devm_iio_channel_get(struct device *dev,
> > }
> > EXPORT_SYMBOL_GPL(devm_iio_channel_get);
> >
> > -struct iio_channel *devm_of_iio_channel_get_by_name(struct device
> > *dev,
> > - struct
> > device_node *np,
> > - const char
> > *channel_name)
> > +struct iio_channel *devm_fwnode_iio_channel_get_by_name(struct
> > device *dev,
> > + struct
> > fwnode_handle *fwnode,
> > + const char
> > *channel_name)
> > {
> > struct iio_channel *channel;
> > int ret;
> >
> > - channel = of_iio_channel_get_by_name(np, channel_name);
> > + channel = fwnode_iio_channel_get_by_name(fwnode,
> > channel_name);
> > if (IS_ERR(channel))
> > return channel;
> >
> > @@ -438,7 +453,7 @@ struct iio_channel
> > *devm_of_iio_channel_get_by_name(struct device *dev,
> >
> > return channel;
> > }
> > -EXPORT_SYMBOL_GPL(devm_of_iio_channel_get_by_name);
> > +EXPORT_SYMBOL_GPL(devm_fwnode_iio_channel_get_by_name);
> >
> > struct iio_channel *iio_channel_get_all(struct device *dev)
> > {
> > @@ -452,7 +467,7 @@ struct iio_channel *iio_channel_get_all(struct
> > device *dev)
> > if (dev == NULL)
> > return ERR_PTR(-EINVAL);
> >
> > - chans = of_iio_channel_get_all(dev);
> > + chans = fwnode_iio_channel_get_all(dev);
> > if (!IS_ERR(chans) || PTR_ERR(chans) == -EPROBE_DEFER)
> > return chans;
> >
> > diff --git a/include/linux/iio/consumer.h
> > b/include/linux/iio/consumer.h
> > index 5fa5957586cf..a96a714b5fdc 100644
> > --- a/include/linux/iio/consumer.h
> > +++ b/include/linux/iio/consumer.h
> > @@ -9,11 +9,13 @@
> >
> > #include <linux/types.h>
> > #include <linux/iio/types.h>
>
> > +#include <linux/of.h>
>
> Ordering. IIO has special meaning here, that's why it's last.
ok...
- Nuno Sá
