Re: [PATCH v4 6/8] iio: adc: stm32-dfsdm: adopt generic channels bindings

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 4 Jul 2024 17:53:34 +0200
Olivier Moysan <olivier.moysan@foss.st.com> wrote:

> Move to generic channels binding to ease new backend framework adoption
> and prepare the convergence with MDF IP support on STM32MP2 SoC family.
> 
> Legacy binding:
> DFSDM is an IIO channel consumer.
> SD modulator is an IIO channels provider.
> The channel phandles are provided in DT through io-channels property
> and channel indexes through st,adc-channels property.
> 
> New binding:
> DFSDM is an IIO channel provider.
> The channel indexes are given by reg property in channel child node.
> 
> This new binding is intended to be used with SD modulator IIO backends.
> It does not support SD modulator legacy IIO devices.
> The st,adc-channels property presence is used to discriminate
> between legacy and backend bindings.
> 
> The support of the DFSDM legacy channels and SD modulator IIO devices
> is kept for backward compatibility.
> 
> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
Hi Olivier

A few minor comments inline given looks like you are going to be doing a v5.

Jonathan


> ---
>  drivers/iio/adc/stm32-dfsdm-adc.c | 200 ++++++++++++++++++++++++------
>  1 file changed, 164 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
> index fabd654245f5..ae5d95e38cd7 100644
> --- a/drivers/iio/adc/stm32-dfsdm-adc.c
> +++ b/drivers/iio/adc/stm32-dfsdm-adc.c
> @@ -666,6 +666,64 @@ static int stm32_dfsdm_channel_parse_of(struct stm32_dfsdm *dfsdm,
>  	return 0;
>  }
>  
> +static int stm32_dfsdm_generic_channel_parse_of(struct stm32_dfsdm *dfsdm,
> +						struct iio_dev *indio_dev,
> +						struct iio_chan_spec *ch,
> +						struct fwnode_handle *node)
> +{
> +	struct stm32_dfsdm_channel *df_ch;
> +	const char *of_str;
> +	int ret, val;
> +
> +	ret = fwnode_property_read_u32(node, "reg", &ch->channel);
> +	if (ret < 0) {
> +		dev_err(&indio_dev->dev, "Missing channel index %d\n", ret);
> +		return ret;

I think this is only called from probe?
If so, return dev_err_probe() throughout is probably appropriate.

> +	}
...

> @@ -1430,43 +1540,61 @@ static int stm32_dfsdm_adc_init(struct device *dev, struct iio_dev *indio_dev)
>  {
...

> -	for (chan_idx = 0; chan_idx < num_ch; chan_idx++) {
> -		ch[chan_idx].scan_index = chan_idx;
> -		ret = stm32_dfsdm_adc_chan_init_one(indio_dev, &ch[chan_idx]);
> -		if (ret < 0) {
> -			dev_err(&indio_dev->dev, "Channels init failed\n");
> -			return ret;
> -		}
> +		adc->backend = devm_kzalloc(&indio_dev->dev, sizeof(*adc->backend) * num_ch,

devm_kcalloc maybe?
We aren't going to overflow here, but perhaps it is also more readable in that it makes
it clear this is an array of pointers.

However I also wanted to check the type of backend. Tricky as you don't introduce
it until 2 patches later.  Fix that. Also on a series like this, make sure to
step patch by patch and ensure it at least builds. Otherwise bisection might not work
and people get very grumpy if that happens.



> +					    GFP_KERNEL);
> +		if (!adc->backend)
> +			return -ENOMEM;
>  	}
>  
> -	indio_dev->num_channels = num_ch;
> +	ch = devm_kcalloc(&indio_dev->dev, num_ch, sizeof(*ch), GFP_KERNEL);
> +	if (!ch)
> +		return -ENOMEM;
>  	indio_dev->channels = ch;
>  
> +	if (legacy)
> +		ret = stm32_dfsdm_chan_init(indio_dev, ch);
> +	else
> +		ret = stm32_dfsdm_generic_chan_init(indio_dev, ch);
> +	if (ret < 0)
> +		return ret;
> +
>  	init_completion(&adc->completion);
>  
>  	/* Optionally request DMA */