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() :-)
> +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);
--
With Best Regards,
Andy Shevchenko
