Re: [PATCH 24/34] iio: inkern: move to fwnode properties

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux