On Mon, Jul 11, 2022 at 2:38 PM Nuno Sá <nuno.sa@xxxxxxxxxx> wrote:
>
> APIs like of_iio_channel_get_by_name() and of_iio_channel_get_all() were
> returning a mix of NULL and pointers with NULL being the way to
> "notify" that we should do a "system" lookup for channels. This make
> it very confusing and prone to errors as commit 9f63cc0921ec
> ("iio: inkern: fix return value in devm_of_iio_channel_get_by_name()")
> proves. On top of this, patterns like 'if (channel != NULL) return
> channel' were being used where channel could actually be an error code
> which makes the code hard to read.
>
> This change also makes some functional changes on how errors were being
> handled. In the original behavior, even if we get an error like '-ENOMEM',
> we still continue with the search. We should only continue to lookup for
> the channel when it makes sense to do so. Hence, the main error handling
> in 'of_iio_channel_get_by_name()' is changed to the following logic:
>
> * If a channel 'name' is provided and we do find it via
> 'io-channel-names', we should be able to get it. If we get any error,
> we should not proceed with the lookup. Moreover, we should return an error
> so that callers won't proceed with a system lookup.
> * If a channel 'name' is provided and we cannot find it ('index < 0'),
> 'of_parse_phandle_with_args()' is expected to fail with '-EINVAL'. Hence,
> we should only continue if we get that error.
> * If a channel 'name' is not provided we should only carry on with the
> search if 'of_parse_phandle_with_args()' returns '-ENOENT'.
>
> Also note that a system channel lookup is only done if the returned
> error code (from 'of_iio_channel_get_by_name()' or
> 'of_iio_channel_get_all()' is -ENODEV.
LGTM (but I might miss something, it's a bit too many conditionals),
Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> Signed-off-by: Nuno Sá <nuno.sa@xxxxxxxxxx>
> ---
> drivers/iio/inkern.c | 54 +++++++++++++++++++++++++++++++-------------
> 1 file changed, 38 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index 87fd2a0d44f2..c6f1cfe09bd3 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -214,7 +214,7 @@ static struct iio_channel *of_iio_channel_get(struct device_node *np, int index)
> struct iio_channel *of_iio_channel_get_by_name(struct device_node *np,
> const char *name)
> {
> - struct iio_channel *chan = NULL;
> + struct iio_channel *chan;
>
> /* Walk up the tree of devices looking for a matching iio channel */
> while (np) {
> @@ -231,13 +231,33 @@ struct iio_channel *of_iio_channel_get_by_name(struct device_node *np,
> name);
> chan = of_iio_channel_get(np, index);
> if (!IS_ERR(chan) || PTR_ERR(chan) == -EPROBE_DEFER)
> - break;
> - else if (name && index >= 0) {
> - pr_err("ERROR: could not get IIO channel %pOF:%s(%i)\n",
> - np, name ? name : "", index);
> - return NULL;
> + return chan;
> + if (name) {
> + if (index >= 0) {
> + pr_err("ERROR: could not get IIO channel %pOF:%s(%i)\n",
> + np, name, index);
> + /*
> + * In this case, we found 'name' in 'io-channel-names'
> + * but somehow we still fail so that we should not proceed
> + * with any other lookup. Hence, explicitly return -EINVAL
> + * (maybe not the better error code) so that the caller
> + * won't do a system lookup.
> + */
> + return ERR_PTR(-EINVAL);
> + }
> + /* If index < 0, then of_parse_phandle_with_args() fails
> + * with -EINVAL which is expected. We should not proceed
> + * if we get any other error.
> + */
> + if (PTR_ERR(chan) != -EINVAL)
> + return chan;
> + } else if (PTR_ERR(chan) != -ENOENT) {
> + /*
> + * if !name, then we should only proceed the lookup if
> + * of_parse_phandle_with_args() returns -ENOENT.
> + */
> + return chan;
> }
> -
> /*
> * No matching IIO channel found on this node.
> * If the parent node has a "io-channel-ranges" property,
> @@ -245,10 +265,10 @@ struct iio_channel *of_iio_channel_get_by_name(struct device_node *np,
> */
> np = np->parent;
> if (np && !of_get_property(np, "io-channel-ranges", NULL))
> - return NULL;
> + return ERR_PTR(-ENODEV);
> }
>
> - return chan;
> + return ERR_PTR(-ENODEV);
> }
> EXPORT_SYMBOL_GPL(of_iio_channel_get_by_name);
>
> @@ -267,8 +287,8 @@ static struct iio_channel *of_iio_channel_get_all(struct device *dev)
> break;
> } while (++nummaps);
>
> - if (nummaps == 0) /* no error, return NULL to search map table */
> - return NULL;
> + if (nummaps == 0)
> + return ERR_PTR(-ENODEV);
>
> /* NULL terminated array to save passing size */
> chans = kcalloc(nummaps + 1, sizeof(*chans), GFP_KERNEL);
> @@ -295,7 +315,7 @@ static struct iio_channel *of_iio_channel_get_all(struct device *dev)
>
> static inline struct iio_channel *of_iio_channel_get_all(struct device *dev)
> {
> - return NULL;
> + return ERR_PTR(-ENODEV);
> }
>
> #endif /* CONFIG_OF */
> @@ -362,7 +382,7 @@ struct iio_channel *iio_channel_get(struct device *dev,
> if (dev) {
> channel = of_iio_channel_get_by_name(dev->of_node,
> channel_name);
> - if (channel != NULL)
> + if (!IS_ERR(channel) || PTR_ERR(channel) != -ENODEV)
> return channel;
> }
>
> @@ -412,8 +432,6 @@ struct iio_channel *devm_of_iio_channel_get_by_name(struct device *dev,
> channel = of_iio_channel_get_by_name(np, channel_name);
> if (IS_ERR(channel))
> return channel;
> - if (!channel)
> - return ERR_PTR(-ENODEV);
>
> ret = devm_add_action_or_reset(dev, devm_iio_channel_free, channel);
> if (ret)
> @@ -436,7 +454,11 @@ struct iio_channel *iio_channel_get_all(struct device *dev)
> return ERR_PTR(-EINVAL);
>
> chans = of_iio_channel_get_all(dev);
> - if (chans)
> + /*
> + * We only want to carry on if the error is -ENODEV. Anything else
> + * should be reported up the stack.
> + */
> + if (!IS_ERR(chans) || PTR_ERR(chans) != -ENODEV)
> return chans;
>
> name = dev_name(dev);
> --
> 2.37.0
>
--
With Best Regards,
Andy Shevchenko
