| 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: <106d451d-2433-4343-9e22-d9f37b9a24db@baylibre.com>
Date: Sun, 12 Jan 2025 11:51:09 -0600
From: David Lechner <dlechner@...libre.com>
To: Jonathan Cameron <jic23@...nel.org>,
Jonathan Santos <jonath4nns@...il.com>
Cc: 111f571d-1d88-42f7-b9a5-4b1cb328e26b@...libre.com,
Jonathan Santos <Jonathan.Santos@...log.com>, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, lars@...afoo.de,
Michael.Hennerich@...log.com, robh@...nel.org, krzk+dt@...nel.org,
conor+dt@...nel.org, marcelo.schmitt1@...il.com
Subject: Re: [PATCH v1 13/15] iio: adc: ad7768-1: add multiple scan types to
support 16-bits mode
On 1/12/25 6:50 AM, Jonathan Cameron wrote:
> On Sun, 12 Jan 2025 00:21:37 -0300
> Jonathan Santos <jonath4nns@...il.com> wrote:
>
>> On 01/07, David Lechner wrote:
>>> On 1/7/25 9:26 AM, Jonathan Santos wrote:
>>>> When the device is configured to Sinc5 filter and decimation x8,
>>>> output data is reduced to 16-bits in order to support 1 MHz of
>>>> sampling frequency due to clock limitation.
>>>
>>> We aren't going to get a 1 MHz sample rate without SPI offload support so maybe
>>> we should save this patch until then?
>>>
>>> In this patch, we are still reading 24-bits per sample, so we aren't really
>>> getting any benefit. It is probably fine for now to leave it as 24-bit even if
>>> the last 8 bits are all 0 or just noise.
>>
>> Indeed we cannot achieve 1 MHz yet, but I believe it is good have this
>> now so it is more mature for the time SPI offload is supported. Also, will
>> allow us to backport this patch to other repos.
>>
>>>
>>> Also, the datasheet says:
>>>
>>> this path allows viewing of wider bandwidth; however, it is quantization
>>> noise limited so that output data is reduced to 16 bits
>>>
>>> So this doesn't actually seem related to higher sample rates. There is a CONVLEN
>>> bit in the INTERFACE_FORMAT register that globally reduces the output size to
>>> 16-bit, which I suspect would be what we will need for achieving the highest
>>> sample rate when we add SPI offload support.
>>>
>>
>> Right, that is true, but the reason we did this patch was to fix the
>> output size when we configure the filter to sinc5 decx8. The datasheet
>> says:
>>
>> To configure the sinc5 filter for 1.024 MSPS output data rate,
>> write 001 to the FILTER bits [6:4] of the DIGITAL_FILTER register
>> (Register 0x19). The ADAQ7768-1 automatically changes the decimation
>> rate to 8 and output data length is reduced to 16 bits from 24 bits
>> due to the maximum speed limitation of the digital serial interface.
>>
>> In this case we don't even need to change the value of CONVLEN
>>
>>>>
>>>> Use multiple scan types feature to enable the driver to switch
>>>> scan type in runtime, making possible to support both 24-bit and
>>>> 16-bit resolution.
>>>>
>>>> Signed-off-by: Jonathan Santos <Jonathan.Santos@...log.com>
>>>> ---
>>>> drivers/iio/adc/ad7768-1.c | 65 ++++++++++++++++++++++++++++++++------
>>>> 1 file changed, 56 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
>>>> index 9741a6d47942..5e4e7d387f9a 100644
>>>> --- a/drivers/iio/adc/ad7768-1.c
>>>> +++ b/drivers/iio/adc/ad7768-1.c
>>>> @@ -134,6 +134,11 @@ struct ad7768_clk_configuration {
>>>> enum ad7768_pwrmode pwrmode;
>>>> };
>>>>
>>>> +enum ad7768_scan_type {
>>>> + AD7768_SCAN_TYPE_NORMAL,
>>>> + AD7768_SCAN_TYPE_HIGH_SPEED,
>>>> +};
>>>> +
>>>> static const char * const ad7768_vcm_modes[] = {
>>>> "(AVDD1-AVSS)/2",
>>>> "2V5",
>>>> @@ -145,6 +150,10 @@ static const char * const ad7768_vcm_modes[] = {
>>>> "OFF",
>>>> };
>>>>
>>>> +static const int ad7768_mclk_div_rates[4] = {
>>>> + 16, 8, 4, 2,
>>>> +};
>>>> +
>>>> static const struct ad7768_clk_configuration ad7768_clk_config[] = {
>>>> { AD7768_MCLK_DIV_2, AD7768_DEC_RATE_8, 16, AD7768_FAST_MODE },
>>>> { AD7768_MCLK_DIV_2, AD7768_DEC_RATE_16, 32, AD7768_FAST_MODE },
>>>> @@ -159,6 +168,21 @@ static const struct ad7768_clk_configuration ad7768_clk_config[] = {
>>>> { AD7768_MCLK_DIV_16, AD7768_DEC_RATE_1024, 16384, AD7768_ECO_MODE },
>>>> };
>>>>
>>>> +static const struct iio_scan_type ad7768_scan_type[] = {
>>>> + [AD7768_SCAN_TYPE_NORMAL] = {
>>>> + .sign = 's',
>>>> + .realbits = 24,
>>>> + .storagebits = 32,
>>>
>>> What happened to .shift = 8, ? If there is a reason for removing it, please add
>>> that to the commit description.
>>>
>>
>> Sorry, will fix this
>>
>>>> + .endianness = IIO_BE,
>>>> + },
>>>> + [AD7768_SCAN_TYPE_HIGH_SPEED] = {
>>>> + .sign = 's',
>>>> + .realbits = 16,
>>>> + .storagebits = 32,
>>>
>>> I guess it doesn't matter much since we are reading one sample at a time, but
>>> I would expect storagebits to be 16 instead of 32. Or if it really needs to be
>>> 32, does it need shift = 16?
>>>
>>
>> This is because the hw is configured to return the samples in a 32 bits
>> format, so if storage is 16 we will get wrong data.
Ah, right, I was forgetting that the data is already coming separately through
the IIO backend interface on a different bus from SPI. So that would be the
comment Jonathan is looking for. :-)
>
> Currently we only support one channel (daisy chain mode support might change
> that). Not particularly painful to repack and it doubles the data we can fit
> in a fifo of a given size.
>
> If this is tricky because of later patches, throw in a common on why.
>
>
>>
>>>> + .endianness = IIO_BE,
>>>> + },
>>>> +};
>>>> +
>>>> static int ad7768_get_vcm(struct iio_dev *dev, const struct iio_chan_spec *chan);
>>>> static int ad7768_set_vcm(struct iio_dev *dev, const struct iio_chan_spec *chan,
>>>> unsigned int mode);
>>>
>>> ...
>>>
>>>> @@ -308,6 +329,15 @@ static int ad7768_scan_direct(struct iio_dev *indio_dev)
>>>> ret = ad7768_spi_reg_read(st, AD7768_REG_ADC_DATA, &readval, 3);
>>>> if (ret < 0)
>>>> return ret;
>>>> +
>>>> + /*
>>>> + * When the decimation rate is set to x8, the ADC data precision is reduced
>>>> + * from 24 bits to 16 bits. Since the AD7768_REG_ADC_DATA register provides
>>>> + * 24-bit data, the precision is reduced by right-shifting the read value
>>>> + * by 8 bits.
>>>> + */
>>>> + if (st->dec_rate == 8)
>>>> + readval = readval >> 8;
>>>
>>> Why not change size of ad7768_spi_reg_read() instead of reading 3 bytes and
>>> throwing one away?
>>>
>>
>> Right, i will check this and fix in the next version
>>
>>>> /*
>>>> * Any SPI configuration of the AD7768-1 can only be
>>>> * performed in continuous conversion mode.
>
Powered by blists - more mailing lists