Re: [PATCH v2] iio: adc: stm32: add oversampling support
From: Olivier MOYSAN
Date: Mon Apr 07 2025 - 12:20:19 EST
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@xxxxxxxxxxx>
Signed-off-by: Olivier Moysan <olivier.moysan@xxxxxxxxxxx>
---
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@xxxxxxxxxx/
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