Re: [PATCH v2] iio: adc: stm32: add oversampling support

[David Lechner <dlechner@baylibre.com>]

On 4/3/25 11:23 AM, Olivier Moysan wrote:
> Add oversampling support for STM32H7, STM32MP15 & STM32MP13.
> STM32F4 ADC has no oversampling feature.
> 
> The current support of the oversampling feature aims at increasing
> the data SNR, without changing the data resolution.
> As the oversampling by itself increases data resolution,
> a right shift is applied to keep initial resolution.

Why do we not want the extra bits too? I guess if we wanted the extra bits
in the future we could make the in_voltage_scale attribute writable to
select the resolution.

> Only the oversampling ratio corresponding to a power of two are
> supported here, to get a direct link between right shift and
> oversampling ratio. (2exp(n) ratio <=> n right shift)
> 
> The oversampling ratio is shared by all channels, whatever channel type.
> (e.g. single ended or differential).
> 
> Oversampling can be configured using IIO ABI:
> - in_voltage_oversampling_ratio_available
> - in_voltage_oversampling_ratio

This would require info_mask_shared_by_type but the patch uses
info_mask_shared_by_all, so the attributes will be:

- oversampling_ratio
- oversampling_ratio_available

I guess currently it doesn't matter which one gets used if there are only
voltage channels, but it could make a difference, e.g. if a temperature
channel was ever added.

In any case, the description should match what is actually implemented.

> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
> 
> ---
> Changes in v2:
> - Remove useless header files
> - Use FIELD_PREP macro
> - Reorder stm32_adc_write_raw() function

Link to v1? (for the lazy reviewer :-p)

> ---
>  drivers/iio/adc/stm32-adc-core.h |  14 ++++
>  drivers/iio/adc/stm32-adc.c      | 137 +++++++++++++++++++++++++++++++
>  2 files changed, 151 insertions(+)
> 
> diff --git a/drivers/iio/adc/stm32-adc-core.h b/drivers/iio/adc/stm32-adc-core.h
> index 73b2c2e91c08..bfd42c5456bf 100644
> --- a/drivers/iio/adc/stm32-adc-core.h
> +++ b/drivers/iio/adc/stm32-adc-core.h
> @@ -91,6 +91,7 @@
>  #define STM32H7_ADC_IER			0x04
>  #define STM32H7_ADC_CR			0x08
>  #define STM32H7_ADC_CFGR		0x0C
> +#define STM32H7_ADC_CFGR2		0x10
>  #define STM32H7_ADC_SMPR1		0x14
>  #define STM32H7_ADC_SMPR2		0x18
>  #define STM32H7_ADC_PCSEL		0x1C
> @@ -160,6 +161,13 @@
>  #define STM32H7_DMNGT_SHIFT		0
>  #define STM32H7_DMNGT_MASK		GENMASK(1, 0)
>  
> +/* STM32H7_ADC_CFGR2 bit fields */
> +#define STM32H7_OVSR_MASK		GENMASK(25, 16) /* Correspond to OSVR field in datasheet */

nit: Comment seems obvious and can be left out.

> +#define STM32H7_OVSR(v)			FIELD_PREP(STM32H7_OVSR_MASK, v)
> +#define STM32H7_OVSS_MASK		GENMASK(8, 5)
> +#define STM32H7_OVSS(v)			FIELD_PREP(STM32H7_OVSS_MASK, v)
> +#define STM32H7_ROVSE			BIT(0)
> +

...

> +static const unsigned int stm32h7_adc_oversampling_avail[] = {
> +1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024

Normal style is to have 1 tab indent here.

> +};
> +
> +static const unsigned int stm32mp13_adc_oversampling_avail[] = {
> +1, 2, 4, 8, 16, 32, 64, 128, 256

And here.

>  };
>  

...

> @@ -889,6 +912,41 @@ static void stm32mp13_adc_start_conv(struct iio_dev *indio_dev, bool dma)
>  	stm32_adc_set_bits(adc, STM32H7_ADC_CR, STM32H7_ADSTART);
>  }
>  
> +static void stm32h7_adc_set_ovs(struct iio_dev *indio_dev, u32 ovs_idx)
> +{
> +	struct stm32_adc *adc = iio_priv(indio_dev);
> +	u32 ovsr_bits, bits, msk;
> +
> +	msk = STM32H7_ROVSE | STM32H7_OVSR_MASK | STM32H7_OVSS_MASK;
> +	stm32_adc_clr_bits(adc, STM32H7_ADC_CFGR2, msk);
> +
> +	if (!ovs_idx)
> +		return;
> +
> +	ovsr_bits = (1 << ovs_idx) - 1;
> +	bits = STM32H7_ROVSE | STM32H7_OVSS(ovs_idx) | STM32H7_OVSR(ovsr_bits);
> +
> +	stm32_adc_set_bits(adc, STM32H7_ADC_CFGR2, bits & msk);
> +}
> +
> +static void stm32mp13_adc_set_ovs(struct iio_dev *indio_dev, u32 ovs_idx)
> +{
> +	struct stm32_adc *adc = iio_priv(indio_dev);
> +	u32 bits, msk;
> +
> +	msk = STM32H7_ROVSE | STM32MP13_OVSR_MASK | STM32MP13_OVSS_MASK;
> +	stm32_adc_clr_bits(adc, STM32H7_ADC_CFGR2, msk);
> +
> +	if (!ovs_idx)
> +		return;
> +
> +	bits = STM32H7_ROVSE | STM32MP13_OVSS(ovs_idx);
> +	if (ovs_idx - 1)
> +		bits |= STM32MP13_OVSR(ovs_idx - 1);
> +
> +	stm32_adc_set_bits(adc, STM32H7_ADC_CFGR2, bits & msk);
> +}


Some comments in these functions could be useful to avoid needing the
datasheet to understand all the different things that are happening here
and more importantly, why it was decided to do it this way when there are
many other possibilities (i.e. repeat the bit from commit message about
always using 12-bit output).

> @@ -1461,6 +1519,69 @@ static int stm32_adc_single_conv(struct iio_dev *indio_dev,
>  	return ret;
>  }
>  
> +static int stm32_adc_write_raw(struct iio_dev *indio_dev,
> +			       struct iio_chan_spec const *chan,
> +			       int val, int val2, long mask)
> +{
> +	struct stm32_adc *adc = iio_priv(indio_dev);
> +	struct device *dev = indio_dev->dev.parent;
> +	int nb = adc->cfg->adc_info->num_ovs;
> +	u32 idx;
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		if (val2)
> +			return -EINVAL;
> +
> +		for (idx = 0; idx < nb; idx++)
> +			if (adc->cfg->adc_info->oversampling[idx] == val)
> +				break;
> +
> +		if (idx >= nb)
> +			return -EINVAL;
> +
> +		ret = iio_device_claim_direct_mode(indio_dev);
> +		if (ret)
> +			return ret;

We've been replacing this everywhere with:

		if (!iio_device_claim_direct(indio_dev))
			return -EBUSY;

See: https://lore.kernel.org/linux-iio/20250331121317.1694135-1-jic23@kernel.org/

> +
> +		ret = pm_runtime_resume_and_get(dev);
> +		if (ret < 0)
> +			goto err;
> +
> +		adc->cfg->set_ovs(indio_dev, idx);
> +
> +		pm_runtime_mark_last_busy(dev);
> +		pm_runtime_put_autosuspend(dev);
> +
> +		adc->ovs_idx = idx;
> +
> +err:
> +		iio_device_release_direct_mode(indio_dev);

		iio_device_release_direct(indio_dev);

> +
> +		return ret;
> +	default:
> +		return -EINVAL;
> +	}
> +}