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