Re: [PATCH 3/8] iio: adc: ad7124: Switch from of specific to fwnode based property handling

["Nuno Sá" <noname.nuno@gmail.com>]

On Sun, 2024-02-18 at 17:27 +0000, Jonathan Cameron wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Using the generic firmware data access functions from property.h
> provides a number of advantages:
>  1) Works with different firmware types.
>  2) Doesn't provide a 'bad' example for new IIO drivers.
>  3) Lets us use the new _scoped() loops with automatic reference count
>     cleanup for fwnode_handle
> 
> Cc: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Michael Hennerich <Michael.Hennerich@analog.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---

Just one minor nit from me... Still, fell free to add:

Reviewed-by: Nuno Sa <nuno.sa@analog.com>

>  drivers/iio/adc/ad7124.c | 55 +++++++++++++++++-----------------------
>  1 file changed, 23 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
> index b9b206fcd748..e7b1d517d3de 100644
> --- a/drivers/iio/adc/ad7124.c
> +++ b/drivers/iio/adc/ad7124.c
> @@ -14,7 +14,8 @@
>  #include <linux/kernel.h>
>  #include <linux/kfifo.h>
>  #include <linux/module.h>
> -#include <linux/of.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/property.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/spi/spi.h>
>  
> @@ -807,22 +808,19 @@ static int ad7124_check_chip_id(struct ad7124_state *st)
>  	return 0;
>  }
>  
> -static int ad7124_of_parse_channel_config(struct iio_dev *indio_dev,
> -					  struct device_node *np)
> +static int ad7124_parse_channel_config(struct iio_dev *indio_dev,
> +				       struct device *dev)
>  {
>  	struct ad7124_state *st = iio_priv(indio_dev);
>  	struct ad7124_channel_config *cfg;
>  	struct ad7124_channel *channels;
> -	struct device_node *child;
>  	struct iio_chan_spec *chan;
>  	unsigned int ain[2], channel = 0, tmp;
>  	int ret;
>  
> -	st->num_channels = of_get_available_child_count(np);
> -	if (!st->num_channels) {
> -		dev_err(indio_dev->dev.parent, "no channel children\n");
> -		return -ENODEV;
> -	}
> +	st->num_channels = device_get_child_node_count(dev);
> +	if (!st->num_channels)
> +		return dev_err_probe(dev, -ENODEV, "no channel children\n");
>  
>  	chan = devm_kcalloc(indio_dev->dev.parent, st->num_channels,
>  			    sizeof(*chan), GFP_KERNEL);
> @@ -838,39 +836,38 @@ static int ad7124_of_parse_channel_config(struct iio_dev
> *indio_dev,
>  	indio_dev->num_channels = st->num_channels;
>  	st->channels = channels;
>  
> -	for_each_available_child_of_node(np, child) {
> +	device_for_each_child_node_scoped(dev, child) {
>  		cfg = &st->channels[channel].cfg;
>  
> -		ret = of_property_read_u32(child, "reg", &channel);
> +		ret = fwnode_property_read_u32(child, "reg", &channel);
>  		if (ret)
> -			goto err;
> +			return ret;
>  
> -		if (channel >= indio_dev->num_channels) {
> -			dev_err(indio_dev->dev.parent,
> +		if (channel >= indio_dev->num_channels)
> +			return dev_err_probe(dev, -EINVAL,
>  				"Channel index >= number of channels\n");
> -			ret = -EINVAL;
> -			goto err;
> -		}
>  
> -		ret = of_property_read_u32_array(child, "diff-channels",
> -						 ain, 2);
> +		ret = fwnode_property_read_u32_array(child, "diff-channels",
> +						     ain, 2);
>  		if (ret)
> -			goto err;
> +			return ret;
>  
>  		st->channels[channel].nr = channel;
>  		st->channels[channel].ain = AD7124_CHANNEL_AINP(ain[0]) |
>  						 
> AD7124_CHANNEL_AINM(ain[1]);
>  
> -		cfg->bipolar = of_property_read_bool(child, "bipolar");
> +		cfg->bipolar = fwnode_property_read_bool(child, "bipolar");
>  
> -		ret = of_property_read_u32(child, "adi,reference-select",
> &tmp);
> +		ret = fwnode_property_read_u32(child, "adi,reference-select",
> &tmp);
>  		if (ret)
>  			cfg->refsel = AD7124_INT_REF;
>  		else
>  			cfg->refsel = tmp;
>  
> -		cfg->buf_positive = of_property_read_bool(child,
> "adi,buffered-positive");
> -		cfg->buf_negative = of_property_read_bool(child,
> "adi,buffered-negative");
> +		cfg->buf_positive =
> +			fwnode_property_read_bool(child, "adi,buffered-
> positive");
> +		cfg->buf_negative =
> +			fwnode_property_read_bool(child, "adi,buffered-
> negative");

I think this is one of those cases where 80col limit hurts readability...

- Nuno Sá