On Tue, 18 Jun 2024 18:08:32 +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 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;
> }
>
