On Thu, 4 Jul 2024 17:53:34 +0200
Olivier Moysan <olivier.moysan@xxxxxxxxxxx> 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@xxxxxxxxxxx>
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 */
