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

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 25 Jun 2024 17:07:14 +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

Some minor comments inline.

thanks,

Jonathan



> @@ -1362,15 +1422,20 @@ static int stm32_dfsdm_dma_request(struct device *dev,
>  	return 0;
>  }
>  
> -static int stm32_dfsdm_adc_chan_init_one(struct iio_dev *indio_dev,
> -					 struct iio_chan_spec *ch)
> +static int stm32_dfsdm_adc_chan_init_one(struct iio_dev *indio_dev, struct iio_chan_spec *ch,
> +					 struct fwnode_handle *child)
>  {
>  	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
>  	int ret;
>  
> -	ret = stm32_dfsdm_channel_parse_of(adc->dfsdm, indio_dev, ch);
> -	if (ret < 0)
> +	if (child)
> +		ret = stm32_dfsdm_generic_channel_parse_of(adc->dfsdm, indio_dev, ch, child);
> +	else /* Legacy binding */
> +		ret = stm32_dfsdm_channel_parse_of(adc->dfsdm, indio_dev, ch);
> +	if (ret < 0) {
> +		dev_err(&indio_dev->dev, "Failed to parse channel\n");
>  		return ret;

return dev_err_probe() assuming only called from probe() which I think is the case
but haven't actually checked.


> +	}
>  
>  	ch->type = IIO_VOLTAGE;
>  	ch->indexed = 1;
> @@ -1385,6 +1450,7 @@ static int stm32_dfsdm_adc_chan_init_one(struct iio_dev *indio_dev,
>  
>  	if (adc->dev_data->type == DFSDM_AUDIO) {
>  		ch->ext_info = dfsdm_adc_audio_ext_info;
> +		ch->scan_index = 0;
>  	} else {
>  		ch->scan_type.shift = 8;
>  	}
> @@ -1392,8 +1458,51 @@ static int stm32_dfsdm_adc_chan_init_one(struct iio_dev *indio_dev,
>  	ch->scan_type.realbits = 24;
>  	ch->scan_type.storagebits = 32;
>  
> -	return stm32_dfsdm_chan_configure(adc->dfsdm,
> -					  &adc->dfsdm->ch_list[ch->channel]);
> +	return stm32_dfsdm_chan_configure(adc->dfsdm, &adc->dfsdm->ch_list[ch->channel]);
Is there a change here? If it's just a line wrap don't do that in same patch
making real changes.

> +}
> +
> +static int stm32_dfsdm_chan_init(struct iio_dev *indio_dev, struct iio_chan_spec *channels)
> +{
> +	int num_ch = indio_dev->num_channels;
> +	int chan_idx = 0, ret = 0;

	int ret;

> +
> +	for (chan_idx = 0; chan_idx < num_ch; chan_idx++) {
> +		channels[chan_idx].scan_index = chan_idx;
> +		ret = stm32_dfsdm_adc_chan_init_one(indio_dev, &channels[chan_idx], NULL);
> +		if (ret < 0) {
> +			dev_err(&indio_dev->dev, "Channels init failed\n");
> +			return ret;
			return dev_err_probe()
(I think this is only called from probe?)

> +		}
> +	}
> +
> +	return ret;
return 0;
at least I assume it can't be positive? 
> +}
> +
> +static int stm32_dfsdm_generic_chan_init(struct iio_dev *indio_dev, struct iio_chan_spec *channels)
> +{
> +	struct fwnode_handle *child;
> +	int chan_idx = 0, ret;
> +
> +	device_for_each_child_node(&indio_dev->dev, child) {

device_for_each_child_node_scoped()  as then you can do direct returns.


> +		/* Skip DAI node in DFSDM audio nodes */
> +		if (fwnode_property_present(child, "compatible"))
> +			continue;
> +
> +		channels[chan_idx].scan_index = chan_idx;
> +		ret = stm32_dfsdm_adc_chan_init_one(indio_dev, &channels[chan_idx], child);
> +		if (ret < 0) {
> +			dev_err(&indio_dev->dev, "Channels init failed\n");
> +			goto err;
return dev_err_probe() once using scoped above.


> +		}
> +
> +		chan_idx++;
> +	}
> +	return chan_idx;
> +
> +err:
> +	fwnode_handle_put(child);
> +
> +	return ret;
>  }
>
>  
> @@ -1430,43 +1547,60 @@ static int stm32_dfsdm_adc_init(struct device *dev, struct iio_dev *indio_dev)

> -	ch = devm_kcalloc(&indio_dev->dev, num_ch, sizeof(*ch),
> -			  GFP_KERNEL);
> -	if (!ch)
> -		return -ENOMEM;
> +	if (legacy) {
> +		/* Bind to SD modulator IIO device. */
> +		adc->hwc = devm_iio_hw_consumer_alloc(&indio_dev->dev);
> +		if (IS_ERR(adc->hwc))
> +			return -EPROBE_DEFER;

Obviously in the legacy path, but worth a dev_err_probe() because
if we defer, debug info gets stashed away so it is easy to see
why a driver is continuing to defer.

> +	} else {
> +		/* Generic binding. SD modulator IIO device not used. Use SD modulator backend. */
> +		adc->hwc = NULL;