| Message ID | 20220610084545.547700-25-nuno.sa@analog.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | make iio inkern interface firmware agnostic | expand |
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. > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > --- > 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() :-) > +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. > - 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). > 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? > *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; ? (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. > -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. > struct iio_dev; > struct iio_chan_spec; > struct device; > struct device_node; > +struct fwnode_handle; > > /** > * struct iio_channel - everything needed for a consumer to use a channel > @@ -99,26 +101,20 @@ void iio_channel_release_all(struct iio_channel *chan); > struct iio_channel *devm_iio_channel_get_all(struct device *dev); > > /** > - * of_iio_channel_get_by_name() - get description of all that is needed to access channel. > - * @np: Pointer to consumer device tree node > + * fwnode_iio_channel_get_by_name() - get description of all that is needed to access channel. > + * @fwnode: Pointer to consumer Firmware node > * @consumer_channel: Unique name to identify the channel on the consumer > * side. This typically describes the channels use within > * the consumer. E.g. 'battery_voltage' > */ > -#ifdef CONFIG_OF > -struct iio_channel *of_iio_channel_get_by_name(struct device_node *np, const char *name); > -#else > -static inline struct iio_channel * > -of_iio_channel_get_by_name(struct device_node *np, const char *name) > -{ > - return NULL; > -} > -#endif > +struct iio_channel *fwnode_iio_channel_get_by_name(struct fwnode_handle *fwnode, > + const char *name); > > /** > - * devm_of_iio_channel_get_by_name() - Resource managed version of of_iio_channel_get_by_name(). > + * devm_fwnode_iio_channel_get_by_name() - Resource managed version of > + * fwnode_iio_channel_get_by_name(). > * @dev: Pointer to consumer device. > - * @np: Pointer to consumer device tree node > + * @fwnode: Pointer to consumer Firmware node > * @consumer_channel: Unique name to identify the channel on the consumer > * side. This typically describes the channels use within > * the consumer. E.g. 'battery_voltage' > @@ -129,9 +125,17 @@ of_iio_channel_get_by_name(struct device_node *np, const char *name) > * The allocated iio channel is automatically released when the device is > * unbound. > */ > -struct iio_channel *devm_of_iio_channel_get_by_name(struct device *dev, > - struct device_node *np, > - const char *consumer_channel); > +struct iio_channel *devm_fwnode_iio_channel_get_by_name(struct device *dev, > + struct fwnode_handle *fwnode, > + const char *consumer_channel); > + > +static inline struct iio_channel > +*devm_of_iio_channel_get_by_name(struct device *dev, struct device_node *np, > + const char *consumer_channel) > +{ > + return devm_fwnode_iio_channel_get_by_name(dev, of_fwnode_handle(np), > + consumer_channel); > +} > > struct iio_cb_buffer; > /** > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h > index d9b4a9ca9a0f..494abb63406e 100644 > --- a/include/linux/iio/iio.h > +++ b/include/linux/iio/iio.h > @@ -18,6 +18,7 @@ > */ > > struct of_phandle_args; > +struct fwnode_reference_args; > > enum iio_shared_by { > IIO_SEPARATE, > @@ -429,6 +430,8 @@ struct iio_trigger; /* forward declaration */ > * provide a custom of_xlate function that reads the > * *args* and returns the appropriate index in registered > * IIO channels array. > + * @fwnode_xlate: fwnode based function pointer to obtain channel specifier index. > + * Functionally the same as @of_xlate. > * @hwfifo_set_watermark: function pointer to set the current hardware > * fifo watermark level; see hwfifo_* entries in > * Documentation/ABI/testing/sysfs-bus-iio for details on > @@ -510,6 +513,8 @@ struct iio_info { > unsigned *readval); > int (*of_xlate)(struct iio_dev *indio_dev, > const struct of_phandle_args *iiospec); > + int (*fwnode_xlate)(struct iio_dev *indio_dev, > + const struct fwnode_reference_args *iiospec); > int (*hwfifo_set_watermark)(struct iio_dev *indio_dev, unsigned val); > int (*hwfifo_flush_to_buffer)(struct iio_dev *indio_dev, > unsigned count);
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. > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > --- > > 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á
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.
On Sat, 11 Jun 2022 16:30:57 +0100 Jonathan Cameron <jic23@kernel.org> wrote: > 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). 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. > >
On Sat, 2022-06-11 at 16:32 +0100, Jonathan Cameron wrote: > On Sat, 11 Jun 2022 16:30:57 +0100 > Jonathan Cameron <jic23@kernel.org> wrote: > > > 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). > > Having said that adding a warning comment here, so we remember this > is > a potential issue would be great. > can add the warning... And AFAIU, we can go with Andy suggestion right? - Nuno Sá
On Mon, 13 Jun 2022 09:13:00 +0200 Nuno Sá <noname.nuno@gmail.com> wrote: > On Sat, 2022-06-11 at 16:32 +0100, Jonathan Cameron wrote: > > On Sat, 11 Jun 2022 16:30:57 +0100 > > Jonathan Cameron <jic23@kernel.org> wrote: > > > > > 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). > > > > Having said that adding a warning comment here, so we remember this > > is > > a potential issue would be great. > > > > can add the warning... And AFAIU, we can go with Andy suggestion right? Yes, I think so... Might chance my mind when I see the code though :) Jonathan > > - Nuno Sá >
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); +} + +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; - 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); 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); *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; + } - 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; - - np = np->parent; + } } return chan; } -EXPORT_SYMBOL_GPL(of_iio_channel_get_by_name); +EXPORT_SYMBOL_GPL(fwnode_iio_channel_get_by_name); -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> struct iio_dev; struct iio_chan_spec; struct device; struct device_node; +struct fwnode_handle; /** * struct iio_channel - everything needed for a consumer to use a channel @@ -99,26 +101,20 @@ void iio_channel_release_all(struct iio_channel *chan); struct iio_channel *devm_iio_channel_get_all(struct device *dev); /** - * of_iio_channel_get_by_name() - get description of all that is needed to access channel. - * @np: Pointer to consumer device tree node + * fwnode_iio_channel_get_by_name() - get description of all that is needed to access channel. + * @fwnode: Pointer to consumer Firmware node * @consumer_channel: Unique name to identify the channel on the consumer * side. This typically describes the channels use within * the consumer. E.g. 'battery_voltage' */ -#ifdef CONFIG_OF -struct iio_channel *of_iio_channel_get_by_name(struct device_node *np, const char *name); -#else -static inline struct iio_channel * -of_iio_channel_get_by_name(struct device_node *np, const char *name) -{ - return NULL; -} -#endif +struct iio_channel *fwnode_iio_channel_get_by_name(struct fwnode_handle *fwnode, + const char *name); /** - * devm_of_iio_channel_get_by_name() - Resource managed version of of_iio_channel_get_by_name(). + * devm_fwnode_iio_channel_get_by_name() - Resource managed version of + * fwnode_iio_channel_get_by_name(). * @dev: Pointer to consumer device. - * @np: Pointer to consumer device tree node + * @fwnode: Pointer to consumer Firmware node * @consumer_channel: Unique name to identify the channel on the consumer * side. This typically describes the channels use within * the consumer. E.g. 'battery_voltage' @@ -129,9 +125,17 @@ of_iio_channel_get_by_name(struct device_node *np, const char *name) * The allocated iio channel is automatically released when the device is * unbound. */ -struct iio_channel *devm_of_iio_channel_get_by_name(struct device *dev, - struct device_node *np, - const char *consumer_channel); +struct iio_channel *devm_fwnode_iio_channel_get_by_name(struct device *dev, + struct fwnode_handle *fwnode, + const char *consumer_channel); + +static inline struct iio_channel +*devm_of_iio_channel_get_by_name(struct device *dev, struct device_node *np, + const char *consumer_channel) +{ + return devm_fwnode_iio_channel_get_by_name(dev, of_fwnode_handle(np), + consumer_channel); +} struct iio_cb_buffer; /** diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h index d9b4a9ca9a0f..494abb63406e 100644 --- a/include/linux/iio/iio.h +++ b/include/linux/iio/iio.h @@ -18,6 +18,7 @@ */ struct of_phandle_args; +struct fwnode_reference_args; enum iio_shared_by { IIO_SEPARATE, @@ -429,6 +430,8 @@ struct iio_trigger; /* forward declaration */ * provide a custom of_xlate function that reads the * *args* and returns the appropriate index in registered * IIO channels array. + * @fwnode_xlate: fwnode based function pointer to obtain channel specifier index. + * Functionally the same as @of_xlate. * @hwfifo_set_watermark: function pointer to set the current hardware * fifo watermark level; see hwfifo_* entries in * Documentation/ABI/testing/sysfs-bus-iio for details on @@ -510,6 +513,8 @@ struct iio_info { unsigned *readval); int (*of_xlate)(struct iio_dev *indio_dev, const struct of_phandle_args *iiospec); + int (*fwnode_xlate)(struct iio_dev *indio_dev, + const struct fwnode_reference_args *iiospec); int (*hwfifo_set_watermark)(struct iio_dev *indio_dev, unsigned val); int (*hwfifo_flush_to_buffer)(struct iio_dev *indio_dev, unsigned count);
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. Signed-off-by: Nuno Sá <nuno.sa@analog.com> --- drivers/iio/inkern.c | 145 +++++++++++++++++++---------------- include/linux/iio/consumer.h | 36 +++++---- include/linux/iio/iio.h | 5 ++ 3 files changed, 105 insertions(+), 81 deletions(-)