| 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
| ||
|
Date: Tue, 25 Jun 2024 11:39:20 +0200
From: Olivier MOYSAN <olivier.moysan@...s.st.com>
To: Jonathan Cameron <jic23@...nel.org>
CC: Lars-Peter Clausen <lars@...afoo.de>,
Maxime Coquelin
<mcoquelin.stm32@...il.com>,
Alexandre Torgue <alexandre.torgue@...s.st.com>,
<linux-iio@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<linux-stm32@...md-mailman.stormreply.com>,
<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 8/8] iio: adc: stm32-dfsdm: add scaling support to dfsdm
Hi Jonathan,
On 6/23/24 17:21, Jonathan Cameron wrote:
> On Tue, 18 Jun 2024 18:08:34 +0200
> Olivier Moysan <olivier.moysan@...s.st.com> wrote:
>
>> Add scaling support to STM32 DFSDM.
> Perhaps a short description here of how this works? Where does the scale come
> from, what assumptions are made etc.
>>
Sure. Requires more explanations. Done
>> Signed-off-by: Olivier Moysan <olivier.moysan@...s.st.com>
>
> Some minor stuff.
>
>> diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
>> index 69b4764d7cba..93bf6035bd6d 100644
>> --- a/drivers/iio/adc/stm32-dfsdm-adc.c
>> +++ b/drivers/iio/adc/stm32-dfsdm-adc.c
> urn 0;
>> }
>>
>> @@ -1060,7 +1072,7 @@ static int stm32_dfsdm_update_scan_mode(struct iio_dev *indio_dev,
>> static int stm32_dfsdm_postenable(struct iio_dev *indio_dev)
>> {
>> struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
>> - int ret;
>> + int i = 0, ret;
>
> Don't mix assigned and unassigned variable declarations. Just use a separate line
> as this can mean subtle assignment or lack of assignment issues sneak in.
>
Ack
>>
>> /* Reset adc buffer index */
>> adc->bufi = 0;
>> @@ -1071,6 +1083,15 @@ static int stm32_dfsdm_postenable(struct iio_dev *indio_dev)
>> return ret;
>> }
>>
>> + if (adc->backend) {
>> + while (adc->backend[i]) {
>
> Could do similar to the suggestion below.
> Mostly I don't like the index variable manipulation.
>
>> + ret = iio_backend_enable(&indio_dev->dev, adc->backend[i]);
>> + if (ret < 0)
>> + return ret;
>> + i++;
>> + }
>> + }
>> +
>> ret = stm32_dfsdm_start_dfsdm(adc->dfsdm);
>> if (ret < 0)
>> goto err_stop_hwc;
>> @@ -1103,6 +1124,7 @@ static int stm32_dfsdm_postenable(struct iio_dev *indio_dev)
>> static int stm32_dfsdm_predisable(struct iio_dev *indio_dev)
>> {
>> struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
>> + int i = 0;
>>
>> stm32_dfsdm_stop_conv(indio_dev);
>>
>> @@ -1110,6 +1132,13 @@ static int stm32_dfsdm_predisable(struct iio_dev *indio_dev)
>>
>> stm32_dfsdm_stop_dfsdm(adc->dfsdm);
>>
>> + if (adc->backend) {
>> + while (adc->backend[i]) {
>> + iio_backend_disable(&indio_dev->dev, adc->backend[i]);
>> + i++;
>> + }
> Something like
> struct iio_backend **be = &adc->backend[0];
> do {
> iio_backend_disable(&indio-dev->dev, be);
> } while (be++);
>
> maybe. Up to you.
>
If you don't mind, I will keep indexes. Pointers here looks more
difficult to read, and pre/post incrementation in the loop condition can
easily introduce bugs, imho.
>
>> + }
>
>> @@ -1320,6 +1360,45 @@ static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
>> *val = adc->sample_freq;
>>
>> return IIO_VAL_INT;
>> +
>> + case IIO_CHAN_INFO_SCALE:
>> + /*
>> + * Scale is expressed in mV.
>> + * When fast mode is disabled, actual resolution may be lower
>> + * than 2^n, where n=realbits-1.
>
> As below, use a few more spaces.
>
>> + * This leads to underestimating input voltage. To
>> + * compensate this deviation, the voltage reference can be
>> + * corrected with a factor = realbits resolution / actual max
>> + */
>> + if (adc->backend[idx]) {
>> + iio_backend_read_raw(adc->backend[idx], val, val2, mask);
>> +
>> + *val = div_u64((u64)*val * (u64)BIT(DFSDM_DATA_RES - 1), max);
>> + *val2 = chan->scan_type.realbits;
>> + if (chan->differential)
>> + *val *= 2;
>> + }
>> + return IIO_VAL_FRACTIONAL_LOG2;
>> +
>> + case IIO_CHAN_INFO_OFFSET:
>> + /*
>> + * DFSDM output data are in the range [-2^n,2^n],
> Use a few more spaces. [-2^2, 2^n]
>> + * with n=realbits-1.
> n = realbits - 1
>
> Just to keep it closer to the C coding style.
>
Done
>> + * - Differential modulator:
>> + * Offset correspond to SD modulator offset.
>> + * - Single ended modulator:
>> + * Input is in [0V,Vref] range, where 0V corresponds to -2^n, and Vref to 2^n.
>
> Avoid that long line with a suitable line break.
>
Ack
>> + * Add 2^n to offset. (i.e. middle of input range)
>> + * offset = offset(sd) * vref / res(sd) * max / vref.
>> + */
>> + if (adc->backend[idx]) {
>> + iio_backend_read_raw(adc->backend[idx], val, val2, mask);
>> +
>> + *val = div_u64((u64)max * *val, BIT(*val2 - 1));
>> + if (!chan->differential)
>> + *val += max;
>> + }
>> + return IIO_VAL_INT;
>> }
>>
>> return -EINVAL;
>> @@ -1449,7 +1528,15 @@ static int stm32_dfsdm_adc_chan_init_one(struct iio_dev *indio_dev, struct iio_c
>> * IIO_CHAN_INFO_RAW: used to compute regular conversion
>> * IIO_CHAN_INFO_OVERSAMPLING_RATIO: used to set oversampling
>> */
>> - ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
>> + if (child) {
>> + ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> + BIT(IIO_CHAN_INFO_SCALE) |
>> + BIT(IIO_CHAN_INFO_OFFSET);
>
> Indent looks a little odd. Maybe one more space neede?
>
Ack
>> + } else {
>> + /* Legacy. Scaling not supported */
>> + ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
>> + }
>> +
>> ch->info_mask_shared_by_all = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) |
>> BIT(IIO_CHAN_INFO_SAMP_FREQ);
>>
>> @@ -1816,3 +1903,4 @@ module_platform_driver(stm32_dfsdm_adc_driver);
>> MODULE_DESCRIPTION("STM32 sigma delta ADC");
>> MODULE_AUTHOR("Arnaud Pouliquen <arnaud.pouliquen@...com>");
>> MODULE_LICENSE("GPL v2");
>> +MODULE_IMPORT_NS(IIO_BACKEND);
>
BRs
Olivier
Powered by blists - more mailing lists