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

[Jonathan Cameron]

On Tue, 18 Jun 2024 18:08:32 +0200
Olivier Moysan <olivier.moysan at 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 at foss.st.com>
Hi Oliver

A few minor things inline.

Jonathan

> ---
>  drivers/iio/adc/stm32-dfsdm-adc.c | 208 ++++++++++++++++++++++++------
>  1 file changed, 171 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
> index 9a47d2c87f05..69b4764d7cba 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;
> +	}
> +
> +	if (ch->channel >= dfsdm->num_chs) {
> +		dev_err(&indio_dev->dev, " Error bad channel number %d (max = %d)\n",
> +			ch->channel, dfsdm->num_chs);
> +		return -EINVAL;
> +	}
> +
> +	ret = fwnode_property_read_string(node, "label", &ch->datasheet_name);
> +	if (ret < 0) {
> +		dev_err(&indio_dev->dev,
> +			" Error parsing 'label' for idx %d\n", ch->channel);
> +		return ret;
> +	}
> +
> +	df_ch =  &dfsdm->ch_list[ch->channel];
> +	df_ch->id = ch->channel;
> +
> +	ret = fwnode_property_read_string(node, "st,adc-channel-types", &of_str);
> +	if (!ret) {
> +		val = stm32_dfsdm_str2val(of_str, stm32_dfsdm_chan_type);
> +		if (val < 0)
> +			return val;
> +	} else {
> +		val = 0;

Sometimes better to set a default, then override if if the property is read
successfully.
	df_ch->type = 0;
	if (!fwnode_property_read_string()) {
		int val = ...

		df_ch->type = val;
	}
	
etc
	
> +	}
> +	df_ch->type = val;
> +
> +	ret = fwnode_property_read_string(node, "st,adc-channel-clk-src", &of_str);
> +	if (!ret) {
> +		val = stm32_dfsdm_str2val(of_str, stm32_dfsdm_chan_src);
> +		if (val < 0)
> +			return val;
> +	} else {
> +		val = 0;
> +	}
> +	df_ch->src = val;
> +
> +	ret = fwnode_property_read_u32(node, "st,adc-alt-channel", &df_ch->alt_si);
> +	if (ret != -EINVAL)
> +		df_ch->alt_si = 0;

I'm confused. If it does == EINVAL we just silently carry on with alt_si sort
of undefined. I guess 0?

> +
> +	return 0;
> +}
> +

...

> +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;
> +
> +	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 ret;
return 0;    I don't think it's ever positive and can't get here with it negative.

> +}
> +
> +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() and direct returns should tidy this up a tiny bit.


> +		/* 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;

This is consistent with existing code, but would be nice to make extensive
use of dev_err_probe() in this driver and this is a gone place for that.
			return dev_err_probe(indio_dev->dev, ret, "...);


> +		}
> +
> +		chan_idx++;
> +	}
> +	return chan_idx;
> +
> +err:
> +	fwnode_handle_put(child);
> +
> +	return ret;
>  }
>