Re: [PATCH v3 04/15] iio: inkern: split of_iio_channel_get_by_name()

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 15 Jul 2022 14:28:52 +0200
Nuno Sá <nuno.sa@analog.com> wrote:

> This change splits of_iio_channel_get_by_name() so that it decouples
> looking for channels in the current node from looking in it's parents
> nodes. This will be helpful when moving to fwnode properties where we
> need to release the handles when looking for channels in parent's nodes.
> 
> No functional change intended...
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
A few things from a build test (after hand applying this because of
issues with the comment thing following through the series).

Seem obvious so I fixed them up.

> ---
>  drivers/iio/inkern.c | 109 ++++++++++++++++++++++++++-----------------
>  1 file changed, 65 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index c6f1cfe09bd3..f97b7967d3d9 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -211,61 +211,82 @@ 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,
static 
> +						 const char *name)
> +{
> +	struct iio_channel *chan;
> +	int index = 0;
> +
> +	/*
> +	 * 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()
> +	 * will fail.
> +	 */
> +	if (name)
> +		index = of_property_match_string(np, "io-channel-names", name);
> +
> +	chan = of_iio_channel_get(np, index);
> +	if (!IS_ERR(chan) || PTR_ERR(chan) == -EPROBE_DEFER)
> +		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;
> +	}
> +
> +	/* so we continue the lookup */
> +	return ERR_PTR(-ENODEV);
> +}
> +
>  struct iio_channel *of_iio_channel_get_by_name(struct device_node *np,
>  					       const char *name)
>  {
>  	struct iio_channel *chan;
>  
>  	/* Walk up the tree of devices looking for a matching iio channel */
> +	chan = __of_iio_channel_get_by_name(np, name);
> +	if (!IS_ERR(chan) || PTR_ERR(chan) != -ENODEV)
> +		return chan;
> +
> +	/*
> +	 * No matching IIO channel found on this node.
> +	 * If the parent node has a "io-channel-ranges" property,
> +	 * then we can try one of its channels.
> +	 */
> +	np = np->parent;
>  	while (np) {
>  		int index = 0;
Not used.

>  
> -		/*
> -		 * 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()
> -		 * will fail.
> -		 */
> -		if (name)
> -			index = of_property_match_string(np, "io-channel-names",
> -							 name);
> -		chan = of_iio_channel_get(np, index);
> -		if (!IS_ERR(chan) || PTR_ERR(chan) == -EPROBE_DEFER)
> -			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.
> -			 */
> +		if (!of_get_property(np, "io-channel-ranges", NULL))
> +			return ERR_PTR(-ENODEV);
> +
> +		chan = __of_iio_channel_get_by_name(np, name);
> +		if (!IS_ERR(chan) || PTR_ERR(chan) != -ENODEV)
>  			return chan;
> -		}
> -		/*
> -		 * No matching IIO channel found on this node.
> -		 * If the parent node has a "io-channel-ranges" property,
> -		 * then we can try one of its channels.
> -		 */
> +
>  		np = np->parent;
> -		if (np && !of_get_property(np, "io-channel-ranges", NULL))
> -			return ERR_PTR(-ENODEV);
>  	}
>  
>  	return ERR_PTR(-ENODEV);