| 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: <cb81cba4-0fa3-431a-924f-b362fd0c4638@gmail.com>
Date: Mon, 14 Apr 2025 09:40:03 +0300
From: Matti Vaittinen <mazziesaccount@...il.com>
To: Sukrut Bellary <sbellary@...libre.com>,
Jonathan Cameron <jic23@...nel.org>, Lars-Peter Clausen <lars@...afoo.de>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Angelo Compagnucci <angelo.compagnucci@...il.com>
Cc: Nishanth Menon <nm@...com>, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] iio: adc: ti-adc128s052: Add lower resolution devices
support
On 08/04/2025 16:21, Sukrut Bellary wrote:
> The adcxx4s communicates with a host processor via an SPI/Microwire Bus
> interface. The device family responds with 12-bit data, of which the LSB
> bits are transmitted by the lower resolution devices as 0.
> The unavailable bits are 0 in LSB.
> Shift is calculated per resolution and used in scaling and
> raw data read.
>
> Lets reuse the driver to support the family of devices with name
> ADC<bb><c>S<sss>, where
> * bb is the resolution in number of bits (8, 10, 12)
> * c is the number of channels (1, 2, 4, 8)
> * sss is the maximum conversion speed (021 for 200 kSPS, 051 for 500 kSPS
> and 101 for 1 MSPS)
>
> Complete datasheets are available at TI's website here:
> https://www.ti.com/lit/gpn/adc<bb><c>s<sss>.pdf
I tried looking up:
https://www.ti.com/lit/gpn/adc102s051.pdf
>
> Tested only with ti-adc102s051 on BegalePlay SBC.
> https://www.beagleboard.org/boards/beagleplay
>
> Co-developed-by: Nishanth Menon <nm@...com>
> Signed-off-by: Nishanth Menon <nm@...com>
> Signed-off-by: Sukrut Bellary <sbellary@...libre.com>
> ---
> Changes in v3:
> - used be16_to_cpu() for the endian conversion.
> - used config index enum while setting up the adc128_config[]
>
> - Link to v2:
> https://lore.kernel.org/lkml/20231022031203.632153-1-sukrut.bellary@linux.com/
>
> Changes in v2:
> - Arranged of_device_id and spi_device_id in numeric order.
> - Used enum to index into adc128_config.
> - Reorder adc128_config in alphabetical.
> - Include channel resolution information.
> - Shift is calculated per resolution and used in scaling and
> raw data read.
>
> - Link to v1: https://lore.kernel.org/all/20220701042919.18180-1-nm@ti.com/
> ---
> drivers/iio/adc/ti-adc128s052.c | 149 ++++++++++++++++++++++++--------
> 1 file changed, 112 insertions(+), 37 deletions(-)
>
Hi dee Ho,
Thanks for improving this! It's always nice to be able to support more
devices with small(ish) changes!
This looks good to me. I will take another, hopefully more in-depth look
at the rebased version when available though.
I have just one comment for now, but it's not strictly related to this
change. If you wish to go the extra mile, then I'd appreciated it. If
not, then it can be re-worked later. Anyways, please, see below.
> diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
> index a456ea78462f..d4b76fd85abd 100644
> --- a/drivers/iio/adc/ti-adc128s052.c
> +++ b/drivers/iio/adc/ti-adc128s052.c
> @@ -7,6 +7,22 @@
> * https://www.ti.com/lit/ds/symlink/adc128s052.pdf
> * https://www.ti.com/lit/ds/symlink/adc122s021.pdf
> * https://www.ti.com/lit/ds/symlink/adc124s021.pdf
> + *
> + * The adcxx4s communicates with a host processor via an SPI/Microwire Bus
> + * interface. This driver supports the whole family of devices with a name
> + * ADC<bb><c>S<sss>, where
> + * bb is the resolution in number of bits (8, 10, 12)
> + * c is the number of channels (1, 2, 4, 8)
> + * sss is the maximum conversion speed (021 for 200 kSPS, 051 for 500 kSPS
> + * and 101 for 1 MSPS)
> + *
> + * Complete datasheets are available at TI's website here:
> + * https://www.ti.com/lit/gpn/adc<bb><c>s<sss>.pdf
> + *
> + * 8, 10, and 12 bits converters send 12-bit data with
> + * unavailable bits set to 0 in LSB.
> + * Shift is calculated per resolution and used in scaling and
> + * raw data read.
> */
>
> #include <linux/err.h>
> @@ -53,7 +69,7 @@ static int adc128_adc_conversion(struct adc128 *adc, u8 channel)
> if (ret < 0)
> return ret;
>
> - return ((adc->buffer[0] << 8 | adc->buffer[1]) & 0xFFF);
> + return be16_to_cpu(*((__be16 *)adc->buffer));
> }
>
> static int adc128_read_raw(struct iio_dev *indio_dev,
> @@ -70,7 +86,8 @@ static int adc128_read_raw(struct iio_dev *indio_dev,
> if (ret < 0)
> return ret;
>
> - *val = ret;
> + *val = (ret >> channel->scan_type.shift) &
> + GENMASK(channel->scan_type.realbits - 1, 0);
> return IIO_VAL_INT;
>
> case IIO_CHAN_INFO_SCALE:
> @@ -80,7 +97,7 @@ static int adc128_read_raw(struct iio_dev *indio_dev,
> return ret;
>
> *val = ret / 1000;
> - *val2 = 12;
> + *val2 = channel->scan_type.realbits;
> return IIO_VAL_FRACTIONAL_LOG2;
>
> default:
> @@ -89,24 +106,34 @@ static int adc128_read_raw(struct iio_dev *indio_dev,
>
> }
>
> -#define ADC128_VOLTAGE_CHANNEL(num) \
> - { \
> - .type = IIO_VOLTAGE, \
> - .indexed = 1, \
> - .channel = (num), \
> - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> - .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) \
> +#define _ADC128_VOLTAGE_CHANNEL(num, real_bits, store_bits) \
> + { \
> + .type = IIO_VOLTAGE, \
> + .indexed = 1, \
> + .channel = (num), \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
> + .scan_index = (num), \
> + .scan_type = { \
> + .sign = 'u', \
> + .realbits = (real_bits), \
> + .storagebits = (store_bits), \
> + .shift = (12 - real_bits), \
> + }, \
> }
>
> -static const struct iio_chan_spec adc128s052_channels[] = {
> - ADC128_VOLTAGE_CHANNEL(0),
> - ADC128_VOLTAGE_CHANNEL(1),
> - ADC128_VOLTAGE_CHANNEL(2),
> - ADC128_VOLTAGE_CHANNEL(3),
> - ADC128_VOLTAGE_CHANNEL(4),
> - ADC128_VOLTAGE_CHANNEL(5),
> - ADC128_VOLTAGE_CHANNEL(6),
> - ADC128_VOLTAGE_CHANNEL(7),
> +#define ADC082_VOLTAGE_CHANNEL(num) _ADC128_VOLTAGE_CHANNEL(num, 8, 16)
> +#define ADC102_VOLTAGE_CHANNEL(num) _ADC128_VOLTAGE_CHANNEL(num, 10, 16)
> +#define ADC128_VOLTAGE_CHANNEL(num) _ADC128_VOLTAGE_CHANNEL(num, 12, 16)
> +
> +static const struct iio_chan_spec adc082s021_channels[] = {
> + ADC082_VOLTAGE_CHANNEL(0),
> + ADC082_VOLTAGE_CHANNEL(1),
> +};
> +
> +static const struct iio_chan_spec adc102s021_channels[] = {
> + ADC102_VOLTAGE_CHANNEL(0),
> + ADC102_VOLTAGE_CHANNEL(1),
> };
>
> static const struct iio_chan_spec adc122s021_channels[] = {
> @@ -121,10 +148,46 @@ static const struct iio_chan_spec adc124s021_channels[] = {
> ADC128_VOLTAGE_CHANNEL(3),
> };
>
> +static const struct iio_chan_spec adc128s052_channels[] = {
> + ADC128_VOLTAGE_CHANNEL(0),
> + ADC128_VOLTAGE_CHANNEL(1),
> + ADC128_VOLTAGE_CHANNEL(2),
> + ADC128_VOLTAGE_CHANNEL(3),
> + ADC128_VOLTAGE_CHANNEL(4),
> + ADC128_VOLTAGE_CHANNEL(5),
> + ADC128_VOLTAGE_CHANNEL(6),
> + ADC128_VOLTAGE_CHANNEL(7),
> +};
> +
> +enum adc128_configuration_index {
> + ADC128_CONFIG_INDEX_082S,
> + ADC128_CONFIG_INDEX_102S,
> + ADC128_CONFIG_INDEX_122S,
> + ADC128_CONFIG_INDEX_124S,
> + ADC128_CONFIG_INDEX_128S,
> +};
I like the fact you added these indexes as it makes this a lot clearer.
But...
> +
> static const struct adc128_configuration adc128_config[] = {
> - { adc128s052_channels, ARRAY_SIZE(adc128s052_channels) },
> - { adc122s021_channels, ARRAY_SIZE(adc122s021_channels) },
> - { adc124s021_channels, ARRAY_SIZE(adc124s021_channels) },
> + [ADC128_CONFIG_INDEX_082S] = {
> + .channels = adc082s021_channels,
> + .num_channels = ARRAY_SIZE(adc082s021_channels)
> + },
> + [ADC128_CONFIG_INDEX_102S] = {
> + .channels = adc102s021_channels,
> + .num_channels = ARRAY_SIZE(adc102s021_channels)
> + },
> + [ADC128_CONFIG_INDEX_122S] = {
> + .channels = adc122s021_channels,
> + .num_channels = ARRAY_SIZE(adc122s021_channels)
> + },
> + [ADC128_CONFIG_INDEX_124S] = {
> + .channels = adc124s021_channels,
> + .num_channels = ARRAY_SIZE(adc124s021_channels)
> + },
> + [ADC128_CONFIG_INDEX_128S] = {
> + .channels = adc128s052_channels,
> + .num_channels = ARRAY_SIZE(adc128s052_channels)
> + },
> };
... I don't really love this array. I believe the code would be clearer
if this array was changed to individual structs because ...
>
> static const struct iio_info adc128_info = {
> @@ -177,31 +240,43 @@ static int adc128_probe(struct spi_device *spi)
> }
>
> static const struct of_device_id adc128_of_match[] = {
> - { .compatible = "ti,adc128s052", .data = &adc128_config[0] },
> - { .compatible = "ti,adc122s021", .data = &adc128_config[1] },
> - { .compatible = "ti,adc122s051", .data = &adc128_config[1] },
> - { .compatible = "ti,adc122s101", .data = &adc128_config[1] },
> - { .compatible = "ti,adc124s021", .data = &adc128_config[2] },
> - { .compatible = "ti,adc124s051", .data = &adc128_config[2] },
> - { .compatible = "ti,adc124s101", .data = &adc128_config[2] },
> + { .compatible = "ti,adc082s021", .data = &adc128_config[ADC128_CONFIG_INDEX_082S] },
> + { .compatible = "ti,adc082s051", .data = &adc128_config[ADC128_CONFIG_INDEX_082S] },
> + { .compatible = "ti,adc082s101", .data = &adc128_config[ADC128_CONFIG_INDEX_082S] },
> + { .compatible = "ti,adc102s021", .data = &adc128_config[ADC128_CONFIG_INDEX_102S] },
> + { .compatible = "ti,adc102s051", .data = &adc128_config[ADC128_CONFIG_INDEX_102S] },
> + { .compatible = "ti,adc102s101", .data = &adc128_config[ADC128_CONFIG_INDEX_102S] },
> + { .compatible = "ti,adc122s021", .data = &adc128_config[ADC128_CONFIG_INDEX_122S] },
> + { .compatible = "ti,adc122s051", .data = &adc128_config[ADC128_CONFIG_INDEX_122S] },
> + { .compatible = "ti,adc122s101", .data = &adc128_config[ADC128_CONFIG_INDEX_122S] },
> + { .compatible = "ti,adc124s021", .data = &adc128_config[ADC128_CONFIG_INDEX_124S] },
> + { .compatible = "ti,adc124s051", .data = &adc128_config[ADC128_CONFIG_INDEX_124S] },
> + { .compatible = "ti,adc124s101", .data = &adc128_config[ADC128_CONFIG_INDEX_124S] },
> + { .compatible = "ti,adc128s052", .data = &adc128_config[ADC128_CONFIG_INDEX_128S] },
... here we could then directly refer to individual structs. That way we
would not need to define the names for the array indexes (for clarity),
or look up the individual array members based on magic numbers.
> { /* sentinel */ },
> };
> MODULE_DEVICE_TABLE(of, adc128_of_match);
Yours,
-- Matti
Powered by blists - more mailing lists