| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <6d12b6fe-85fb-4345-bf32-02c0fbb1a27a@foss.st.com>
Date: Mon, 7 Apr 2025 18:07:17 +0200
From: Olivier MOYSAN <olivier.moysan@...s.st.com>
To: David Lechner <dlechner@...libre.com>,
Jonathan Cameron
<jic23@...nel.org>,
Lars-Peter Clausen <lars@...afoo.de>,
Maxime Coquelin
<mcoquelin.stm32@...il.com>,
Alexandre Torgue <alexandre.torgue@...s.st.com>
CC: Fabrice Gasnier <fabrice.gasnier@...s.st.com>, <linux-iio@...r.kernel.org>,
<linux-stm32@...md-mailman.stormreply.com>,
<linux-arm-kernel@...ts.infradead.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] iio: adc: stm32: add oversampling support
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@...s.st.com>
>> Signed-off-by: Olivier Moysan <olivier.moysan@...s.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
Powered by blists - more mailing lists