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

[Olivier MOYSAN <olivier.moysan@foss.st.com>]

Hi David,

Thanks for reviewing,

On 4/4/25 18:15, David Lechner wrote:
> 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.
> 

Yes, I agree that the oversampling feature could could be further 
exploited. It could also be used to increase the data resolution.
This additional feature can be managed as a separate patch later.
The main logic here was to keep the resolution aligned with the one 
requested in the DT, through the "assigned-resolution-bits" property.

>> 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.
> 

The oversampling configuration is shared by all the channels of a given 
ADC instance. So, it makes sense to use info_mask_shared_by_all here.
What is the most relevant is to change the commit message to
"- oversampling_ratio
  - oversampling_ratio_available"
which are the attributes actually exposed.

>>
>> 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.
> 

Oversampling bit name is "OSVR" in datasheet H7, while oversampling 
shift is "OVSS". For naming consistency, I used OVSR instead of OSVR,
and highlighted it with a comment. As an alternative, STM32H7_OVSR could 
be renamed, but I would rather keep it unchanged.

>> +#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.
> 

ok

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

ok

>>   };
>>   
> 
> ...
> 
>> @@ -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).
> 

ok, I will add some comments here.

>> @@ -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/
> 

ok

>> +
>> +		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;
>> +	}
>> +}

BRs
Olivier