| 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: <48ad0f2b-e937-43f0-a620-164eeb45723e@baylibre.com>
Date: Thu, 22 Aug 2024 14:43:19 -0500
From: David Lechner <dlechner@...libre.com>
To: Esteban Blanc <eblanc@...libre.com>, Lars-Peter Clausen
<lars@...afoo.de>, Michael Hennerich <Michael.Hennerich@...log.com>,
Jonathan Cameron <jic23@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Nuno Sa <nuno.sa@...log.com>,
Jonathan Corbet <corbet@....net>
Cc: linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org
Subject: Re: [PATCH 4/6] iio: adc: ad4030: add support for ad4630-24 and
ad4630-16
On 8/22/24 7:45 AM, Esteban Blanc wrote:
> AD4630-24 and AD4630-16 are 2 channels ADCs. Both channels are
> interleaved bit per bit on SDO line.
>
> Signed-off-by: Esteban Blanc <eblanc@...libre.com>
> ---
> drivers/iio/adc/ad4030.c | 197 +++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 173 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/iio/adc/ad4030.c b/drivers/iio/adc/ad4030.c
> index e1e1dbf0565c..dbba5287b630 100644
> --- a/drivers/iio/adc/ad4030.c
> +++ b/drivers/iio/adc/ad4030.c
> @@ -32,6 +32,8 @@
> #define AD4030_REG_PRODUCT_ID_H 0x05
> #define AD4030_REG_CHIP_GRADE 0x06
> #define AD4030_REG_CHIP_GRADE_AD4030_24_GRADE 0x10
> +#define AD4030_REG_CHIP_GRADE_AD4630_16_GRADE 0x03
> +#define AD4030_REG_CHIP_GRADE_AD4630_24_GRADE 0x00
> #define AD4030_REG_CHIP_GRADE_MASK_CHIP_GRADE GENMASK(7, 3)
> #define AD4030_REG_SCRATCH_PAD 0x0A
> #define AD4030_REG_SPI_REVISION 0x0B
> @@ -159,10 +161,14 @@ struct ad4030_state {
> struct {
> union {
> u8 raw[AD4030_MAXIMUM_RX_BUFFER_SIZE];
> - struct {
> - s32 val;
> - u32 common;
> - } __packed buffered[AD4030_MAX_HARDWARE_CHANNEL_NB];
> + union {
> + s32 diff[AD4030_MAX_HARDWARE_CHANNEL_NB];
> + struct {
> + s32 diff;
> + u32 common;
> + } __packed
> + buffered_common[AD4030_MAX_HARDWARE_CHANNEL_NB];
> + };
> };
> } rx_data __aligned(IIO_DMA_MINALIGN);
> };
> @@ -171,7 +177,7 @@ struct ad4030_state {
> .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> .type = IIO_VOLTAGE, \
> .indexed = 1, \
> - .channel = _idx * 2 + 2, \
> + .channel = _idx * 3 + 2, \
I'm guessing * 3 should have actually been in the first patch?
Seems odd to change it now.
> .scan_index = _idx * 2 + 1, \
> .extend_name = "Channel" #_idx " common byte part", \
> .scan_type = { \
> @@ -194,8 +200,8 @@ struct ad4030_state {
> BIT(IIO_CHAN_INFO_CALIBSCALE), \
> .type = IIO_VOLTAGE, \
> .indexed = 1, \
> - .channel = _idx * 2, \
> - .channel2 = _idx * 2 + 1, \
> + .channel = _idx * 3, \
> + .channel2 = _idx * 3 + 1, \
> .scan_index = _idx * 2, \
> .extend_name = "Channel" #_idx " differential part", \
> .differential = true, \
> @@ -412,7 +418,7 @@ static int ad4030_set_avg_frame_len(struct iio_dev *dev, unsigned int avg_len)
> static bool ad4030_is_common_byte_asked(struct ad4030_state *st,
> unsigned int mask)
> {
> - /* Common byte channel is after the "real" differential sample channel */
> + /* Common byte channels are after each differential channel */
> return mask & AD4030_COMMON_BYTE_CHANNELS_FILTER;
> }
>
> @@ -420,18 +426,69 @@ static int ad4030_set_mode(struct iio_dev *indio_dev, unsigned long mask)
> {
> struct ad4030_state *st = iio_priv(indio_dev);
>
> - if (st->avg_len)
> + if (st->avg_len) {
> st->mode = AD4030_OUT_DATA_MD_30_AVERAGED_DIFF;
> - else if (ad4030_is_common_byte_asked(st, mask))
> - st->mode = AD4030_OUT_DATA_MD_24_DIFF_8_COM;
> - else
> + } else if (ad4030_is_common_byte_asked(st, mask)) {
> + switch (st->chip->precision_bits) {
> + case 16:
> + st->mode = AD4030_OUT_DATA_MD_16_DIFF_8_COM;
> + break;
> +
> + case 24:
> + st->mode = AD4030_OUT_DATA_MD_24_DIFF_8_COM;
> + break;
> +
> + default:
> + return -EINVAL;
> + }
> + } else {
> st->mode = AD4030_OUT_DATA_MD_24_DIFF;
nit: maybe better to rename this one to AD4030_OUT_DATA_MD_DIFF
or AD4030_OUT_DATA_MD_DIFF_ONLY since it can be 16 or 24 bits
depending on the chip?
> + }
>
> return regmap_update_bits(st->regmap, AD4030_REG_MODES,
> AD4030_REG_MODES_MASK_OUT_DATA_MODE,
> st->mode);
> }
>
> +/*
> + * @brief Descramble 2 32bits numbers out of a 64bits. The bits are interleaved:
> + * 1 bit for first number, 1 bit for the second, and so on...
> + */
> +static void ad4030_extract_interleaved(u8 *src, u32 *ch0, u32 *ch1)
> +{
> + u8 h0, h1, l0, l1;
> + u32 out0, out1;
> + u8 *out0_raw = (u8 *)&out0;
> + u8 *out1_raw = (u8 *)&out1;
> +
> + for (int i = 0; i < 4; i++) {
> + h0 = src[i * 2];
> + l1 = src[i * 2 + 1];
> + h1 = h0 << 1;
> + l0 = l1 >> 1;
> +
> + h0 &= 0xAA;
> + l0 &= 0x55;
> + h1 &= 0xAA;
> + l1 &= 0x55;
> +
> + h0 = (h0 | h0 << 001) & 0xCC;
> + h1 = (h1 | h1 << 001) & 0xCC;
> + l0 = (l0 | l0 >> 001) & 0x33;
> + l1 = (l1 | l1 >> 001) & 0x33;
> + h0 = (h0 | h0 << 002) & 0xF0;
> + h1 = (h1 | h1 << 002) & 0xF0;
> + l0 = (l0 | l0 >> 002) & 0x0F;
> + l1 = (l1 | l1 >> 002) & 0x0F;
> +
> + out0_raw[i] = h0 | l0;
> + out1_raw[i] = h1 | l1;
> + }
> +
> + *ch0 = out0;
> + *ch1 = out1;
> +}
> +
> static int ad4030_conversion(struct ad4030_state *st,
> const struct iio_chan_spec *chan)
> {
> @@ -460,12 +517,21 @@ static int ad4030_conversion(struct ad4030_state *st,
> if (ret)
> return ret;
>
> - if (st->mode != AD4030_OUT_DATA_MD_24_DIFF_8_COM)
> + if (st->chip->num_channels == 2)
> + ad4030_extract_interleaved(st->rx_data.raw,
> + &st->rx_data.diff[0],
> + &st->rx_data.diff[1]);
> +
> + if (st->mode != AD4030_OUT_DATA_MD_16_DIFF_8_COM &&
> + st->mode != AD4030_OUT_DATA_MD_24_DIFF_8_COM)
> return 0;
>
> byte_index = BITS_TO_BYTES(chan->scan_type.realbits);
> - for (i = 0; i < st->chip->num_channels; i++)
> - st->rx_data.buffered[i].common = ((u8 *)&st->rx_data.buffered[i].val)[byte_index];
> + /* Doing it backward to avoid overlap when reordering */
> + for (i = st->chip->num_channels - 1; i > 0; i--) {
> + st->rx_data.buffered_common[i].diff = st->rx_data.diff[i];
> + st->rx_data.buffered_common[i].common = ((u8 *)&st->rx_data.diff[i])[byte_index];
> + }
>
> return 0;
> }
> @@ -489,9 +555,9 @@ static int ad4030_single_conversion(struct iio_dev *indio_dev,
> goto out_error;
>
> if (chan->channel % 2)
> - *val = st->rx_data.buffered[chan->channel / 2].common;
> + *val = st->rx_data.buffered_common[chan->channel / 2].common;
> else
> - *val = st->rx_data.buffered[chan->channel / 2].val;
> + *val = st->rx_data.diff[chan->channel / 2];
Doesn't this need to change since `* 2` was changed to `* 3` for the channel
value?
Better would be to make use of chan->address to store the actual number we need
and use that directly instead of reverse engineering the relation of chan->channel
to input pin number.
>
> out_error:
> ad4030_enter_config_mode(st);
> @@ -582,14 +648,17 @@ static int ad4030_read_raw(struct iio_dev *indio_dev,
> return IIO_VAL_FRACTIONAL_LOG2;
>
> case IIO_CHAN_INFO_CALIBSCALE:
> - ret = ad4030_get_chan_gain(indio_dev, chan->channel,
> - val, val2);
> + ret = ad4030_get_chan_gain(indio_dev,
> + chan->scan_index / 2,
> + val,
> + val2);
> if (ret)
> return ret;
> return IIO_VAL_INT_PLUS_MICRO;
>
> case IIO_CHAN_INFO_CALIBBIAS:
> - ret = ad4030_get_chan_offset(indio_dev, chan->channel,
> + ret = ad4030_get_chan_offset(indio_dev,
> + chan->scan_index / 2,
Similar to above, we could use chan->address here instead of having to
know the relationship between scan_index and input pin.
> val);
> if (ret)
> return ret;
> @@ -614,11 +683,14 @@ static int ad4030_write_raw(struct iio_dev *indio_dev,
> iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> switch (info) {
> case IIO_CHAN_INFO_CALIBSCALE:
> - return ad4030_set_chan_gain(indio_dev, chan->channel,
> - val, val2);
> + return ad4030_set_chan_gain(indio_dev,
> + chan->scan_index / 2,
> + val,
> + val2);
>
> case IIO_CHAN_INFO_CALIBBIAS:
> - return ad4030_set_chan_offset(indio_dev, chan->channel,
> + return ad4030_set_chan_offset(indio_dev,
> + chan->scan_index / 2,
> val);
>
> case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> @@ -801,10 +873,24 @@ static int ad4030_detect_chip_info(const struct ad4030_state *st)
>
> static int ad4030_config(struct ad4030_state *st)
> {
> + int ret;
> + u8 reg_modes;
> +
> st->offset_avail[0] = (int)BIT(st->chip->precision_bits - 1) * -1;
> st->offset_avail[1] = 1;
> st->offset_avail[2] = BIT(st->chip->precision_bits - 1) - 1;
>
> + if (st->chip->num_channels > 1)
> + reg_modes = FIELD_PREP(AD4030_REG_MODES_MASK_LANE_MODE,
> + AD4030_LANE_MD_INTERLEAVED);
> + else
> + reg_modes = FIELD_PREP(AD4030_REG_MODES_MASK_LANE_MODE,
> + AD4030_LANE_MD_1_PER_CH);
> +
> + ret = regmap_write(st->regmap, AD4030_REG_MODES, reg_modes);
> + if (ret)
> + return ret;
> +
> if (st->vio_uv < AD4030_VIO_THRESHOLD_UV)
> return regmap_write(st->regmap, AD4030_REG_IO,
> AD4030_REG_IO_MASK_IO2X);
> @@ -891,8 +977,16 @@ static const unsigned long ad4030_channel_masks[] = {
> 0,
> };
>
> +static const unsigned long ad4630_channel_masks[] = {
> + /* Differential only */
> + BIT(0) | BIT(2),
nit: for order consistency with GENMASK
BIT(2) | BIT(0),
> + /* Differential with common byte */
> + GENMASK(3, 0),
> + 0,
> +};
> +
> static const struct iio_scan_type ad4030_24_scan_types[] = {
> - [AD4030_SCAN_TYPE_NORMAL] = {
> + [AD4030_OUT_DATA_MD_24_DIFF] = {
Accidental replacement?
> .sign = 's',
> .storagebits = 32,
> .realbits = 24,
> @@ -908,6 +1002,23 @@ static const struct iio_scan_type ad4030_24_scan_types[] = {
> },
> };
>
> +static const struct iio_scan_type ad4030_16_scan_types[] = {
> + [AD4030_SCAN_TYPE_NORMAL] = {
> + .sign = 's',
> + .storagebits = 32,
> + .realbits = 16,
> + .shift = 16,
> + .endianness = IIO_BE,
> + },
> + [AD4030_SCAN_TYPE_AVG] = {
> + .sign = 's',
> + .storagebits = 32,
> + .realbits = 30,
> + .shift = 2,
> + .endianness = IIO_BE,
> + }
> +};
> +
> static const struct ad4030_chip_info ad4030_24_chip_info = {
> .name = "ad4030-24",
> .available_masks = ad4030_channel_masks,
> @@ -923,14 +1034,52 @@ static const struct ad4030_chip_info ad4030_24_chip_info = {
> .tcyc = AD4030_TCYC_ADJUSTED_NS,
> };
>
> +static const struct ad4030_chip_info ad4630_16_chip_info = {
> + .name = "ad4630-16",
> + .available_masks = ad4630_channel_masks,
> + .available_masks_len = ARRAY_SIZE(ad4630_channel_masks),
> + .channels = {
> + AD4030_CHAN_IN(0, ad4030_16_scan_types),
> + AD4030_CHAN_CMO(0),
> + AD4030_CHAN_IN(1, ad4030_16_scan_types),
> + AD4030_CHAN_CMO(1),
> + IIO_CHAN_SOFT_TIMESTAMP(4),
> + },
> + .grade = AD4030_REG_CHIP_GRADE_AD4630_16_GRADE,
> + .precision_bits = 16,
> + .num_channels = 2,
> + .tcyc = AD4030_TCYC_ADJUSTED_NS,
> +};
> +
> +static const struct ad4030_chip_info ad4630_24_chip_info = {
> + .name = "ad4630-24",
> + .available_masks = ad4630_channel_masks,
> + .available_masks_len = ARRAY_SIZE(ad4630_channel_masks),
> + .channels = {
> + AD4030_CHAN_IN(0, ad4030_24_scan_types),
> + AD4030_CHAN_CMO(0),
> + AD4030_CHAN_IN(1, ad4030_24_scan_types),
> + AD4030_CHAN_CMO(1),
> + IIO_CHAN_SOFT_TIMESTAMP(4),
> + },
> + .grade = AD4030_REG_CHIP_GRADE_AD4630_24_GRADE,
> + .precision_bits = 24,
> + .num_channels = 2,
> + .tcyc = AD4030_TCYC_ADJUSTED_NS,
> +};
> +
> static const struct spi_device_id ad4030_id_table[] = {
> { "ad4030-24", (kernel_ulong_t)&ad4030_24_chip_info },
> + { "ad4630-16", (kernel_ulong_t)&ad4630_16_chip_info },
> + { "ad4630-24", (kernel_ulong_t)&ad4630_24_chip_info },
> {}
> };
> MODULE_DEVICE_TABLE(spi, ad4030_id_table);
>
> static const struct of_device_id ad4030_of_match[] = {
> { .compatible = "adi,ad4030-24", .data = &ad4030_24_chip_info },
> + { .compatible = "adi,ad4630-16", .data = &ad4630_16_chip_info },
> + { .compatible = "adi,ad4630-24", .data = &ad4630_24_chip_info },
> {}
> };
> MODULE_DEVICE_TABLE(of, ad4030_of_match);
>
Powered by blists - more mailing lists