| Message ID | 20240822-eblanc-ad4630_v1-v1-4-5c68f3327fdd@baylibre.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: adc: ad4030: new driver for AD4030 and similar ADCs | expand |
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@baylibre.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); >
On Thu, 22 Aug 2024 14:45:20 +0200 Esteban Blanc <eblanc@baylibre.com> 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@baylibre.com> Ah. I should have looked on in the patch set. This obviously answers question about plans to support more parts. It feels like you made various improvements to naming etc in this patch and then forgot to push them back to the earlier patches. Jonathan > --- > 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 Unless you are going to do something complex later, a union of unions is just a union so can flatten this I think. Also no need for __packed for array of single size as those are always packed. > + buffered_common[AD4030_MAX_HARDWARE_CHANNEL_NB]; This style is confusing to read. If you did need the __packed then it would be worth pushing to previous line, but you don't. In general feels like some of these renames should have been in the earlier patch. > + }; > }; > } 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, \ Odd change. Wrong patch? > .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 */ Just use that comment in earlier patch to avoid noise. Each can incorporate 1 even if it's a bit odd to read. > return mask & AD4030_COMMON_BYTE_CHANNELS_FILTER; > } > > +/* > + * @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]; > + } I wonder if doing it in place is actually worthwhile. Maybe unpack into a second array? That is still fairly small and may make code easier to read. > > return 0; > } ... > > +static const unsigned long ad4630_channel_masks[] = { > + /* Differential only */ > + BIT(0) | BIT(2), > + /* Differential with common byte */ > + GENMASK(3, 0), The packing of data isn't going to be good. How bad to shuffle to put the two small channels next to each other? Seems like it means you will want to combine your deinterleave and channel specific handling above, which is a bit fiddly but not much worse than current code. It only matters if you have timestamps though as otherwise the 8 byte alignment will force 6 bytes of packing in one place vs the 3 this gives in two places. I guess maybe not worth doing unless you plan to combine this with an offload engine in which case the timestamps will probably be disabled and the 12 byte scan length will be preferable to 16 bytes. > + 0, > +}; > + > static const struct iio_scan_type ad4030_24_scan_types[] = { > - [AD4030_SCAN_TYPE_NORMAL] = { > + [AD4030_OUT_DATA_MD_24_DIFF] = { Name it this way in the first place. > .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, > + } > +}; > +
On Mon Aug 26, 2024 at 9:27 AM UTC, Jonathan Cameron wrote: > On Thu, 22 Aug 2024 14:45:20 +0200 > Esteban Blanc <eblanc@baylibre.com> wrote: > > @@ -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]; > > + } > > I wonder if doing it in place is actually worthwhile. Maybe unpack into a second > array? That is still fairly small and may make code easier to read. Okay sure > > +static const unsigned long ad4630_channel_masks[] = { > > + /* Differential only */ > > + BIT(0) | BIT(2), > > + /* Differential with common byte */ > > + GENMASK(3, 0), > The packing of data isn't going to be good. How bad to shuffle > to put the two small channels next to each other? > Seems like it means you will want to combine your deinterleave > and channel specific handling above, which is a bit fiddly but > not much worse than current code. I can do it since that was what I had done in the RFC in the first place. Nuno asked for in this email https://lore.kernel.org/r/0036d44542f8cf45c91c867f0ddd7b45d1904d6b.camel@gmail.com/: >>> * You're pushing the CM channels into the end. So when we a 2 channel device >>> we'll have: >>> in_voltage0 - diff >>> in_voltage1 - diff >>> in_voltage2 - CM associated with chan0 >>> in_voltage0 - CM associated with chan1 >>> >>> I think we could make it so the CM channel comes right after the channel where >>> it's data belongs too. So for example, odd channels would be CM channels (and >>> labels could also make sense). So that's what I did here :D For the software side off things here it doesn't change a lot of things since we have to manipulate the data anyway, putting the extra byte at the end or in between is no extra work. For the offload engine however, it should be easier to ask for 24 bits then 8 bits for each channel as it would return two u32 per "hardware channel". In order to avoid having two different layouts, I was kind of sold by Nuno's idea of having the CM in between each diff channel. Thanks for your time,
On Fri, 2024-09-13 at 09:55 +0000, Esteban Blanc wrote: > On Mon Aug 26, 2024 at 9:27 AM UTC, Jonathan Cameron wrote: > > On Thu, 22 Aug 2024 14:45:20 +0200 > > Esteban Blanc <eblanc@baylibre.com> wrote: > > > @@ -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]; > > > + } > > > > I wonder if doing it in place is actually worthwhile. Maybe unpack into a second > > array? That is still fairly small and may make code easier to read. > > Okay sure > > > > +static const unsigned long ad4630_channel_masks[] = { > > > + /* Differential only */ > > > + BIT(0) | BIT(2), > > > + /* Differential with common byte */ > > > + GENMASK(3, 0), > > The packing of data isn't going to be good. How bad to shuffle > > to put the two small channels next to each other? > > Seems like it means you will want to combine your deinterleave > > and channel specific handling above, which is a bit fiddly but > > not much worse than current code. > > I can do it since that was what I had done in the RFC in the first place. > Nuno asked for in this email > https://lore.kernel.org/r/0036d44542f8cf45c91c867f0ddd7b45d1904d6b.camel@gmail.com/ > : > > > > > * You're pushing the CM channels into the end. So when we a 2 channel device > > > > we'll have: > > > > > in_voltage0 - diff > > > > in_voltage1 - diff > > > > in_voltage2 - CM associated with chan0 > > > > in_voltage0 - CM associated with chan1 > > > > > > > > I think we could make it so the CM channel comes right after the channel > > > > where > > > > it's data belongs too. So for example, odd channels would be CM channels (and > > > > labels could also make sense). > > So that's what I did here :D > > For the software side off things here it doesn't change a lot of things > since we have to manipulate the data anyway, putting the extra byte at the > end or in between is no extra work. > For the offload engine however, it should be easier to ask for 24 bits > then 8 bits for each channel as it would return two u32 per "hardware > channel". > > In order to avoid having two different layouts, I was kind of sold by > Nuno's idea of having the CM in between each diff channel. > Tbh, I was not even thinking about the layout when I proposed the arrangement. Just made sense to me (from a logical point of view) to have them together as they relate to the same physical channel. FWIW, we're also speaking bytes in here so not sure if it's that important (or bad). That said, as we should have labels anyways, I'm not being the blocker if everyone else prefers to have the RFC layout :) - Nuno Sá
On Fri Sep 13, 2024 at 10:18 AM UTC, Nuno Sá wrote: > On Fri, 2024-09-13 at 09:55 +0000, Esteban Blanc wrote: > > On Mon Aug 26, 2024 at 9:27 AM UTC, Jonathan Cameron wrote: > > > On Thu, 22 Aug 2024 14:45:20 +0200 > > > Esteban Blanc <eblanc@baylibre.com> wrote: > > > > +static const unsigned long ad4630_channel_masks[] = { > > > > + /* Differential only */ > > > > + BIT(0) | BIT(2), > > > > + /* Differential with common byte */ > > > > + GENMASK(3, 0), > > > The packing of data isn't going to be good. How bad to shuffle > > > to put the two small channels next to each other? > > > Seems like it means you will want to combine your deinterleave > > > and channel specific handling above, which is a bit fiddly but > > > not much worse than current code. > > > > I can do it since that was what I had done in the RFC in the first place. > > Nuno asked for in this email > > https://lore.kernel.org/r/0036d44542f8cf45c91c867f0ddd7b45d1904d6b.camel@gmail.com/ > > : > > > > > > > * You're pushing the CM channels into the end. So when we a 2 channel device > > > > > we'll have: > > > > > > > in_voltage0 - diff > > > > > in_voltage1 - diff > > > > > in_voltage2 - CM associated with chan0 > > > > > in_voltage0 - CM associated with chan1 > > > > > > > > > > I think we could make it so the CM channel comes right after the channel > > > > > where > > > > > it's data belongs too. So for example, odd channels would be CM channels (and > > > > > labels could also make sense). > > > > So that's what I did here :D > > > > For the software side off things here it doesn't change a lot of things > > since we have to manipulate the data anyway, putting the extra byte at the > > end or in between is no extra work. > > For the offload engine however, it should be easier to ask for 24 bits > > then 8 bits for each channel as it would return two u32 per "hardware > > channel". > > > > In order to avoid having two different layouts, I was kind of sold by > > Nuno's idea of having the CM in between each diff channel. > > > > Tbh, I was not even thinking about the layout when I proposed the arrangement. Just > made sense to me (from a logical point of view) to have them together as they relate > to the same physical channel. FWIW, we're also speaking bytes in here so not sure if > it's that important (or bad). The best we can do (if we managed to do it HDL wise) is to reorder the data to get both CM byte in a single u32 after the 2 u32 of both diff channel. That would be 3 u32 instead of 4. I don't have a strong opinion other than what we have with the V1 should be simpler to rework for the offload engine since we can manage to get the same layout from the offload engine. And if there is some performance issue we could still try to rework the HDL but I can't say for sure if there will be some drawback with that. If you or Jonathan prefers the RFC version let's do that, no problem I already have the code for it :D Best regards,
On Fri, 2024-09-13 at 12:55 +0000, Esteban Blanc wrote: > On Fri Sep 13, 2024 at 10:18 AM UTC, Nuno Sá wrote: > > On Fri, 2024-09-13 at 09:55 +0000, Esteban Blanc wrote: > > > On Mon Aug 26, 2024 at 9:27 AM UTC, Jonathan Cameron wrote: > > > > On Thu, 22 Aug 2024 14:45:20 +0200 > > > > Esteban Blanc <eblanc@baylibre.com> wrote: > > > > > +static const unsigned long ad4630_channel_masks[] = { > > > > > + /* Differential only */ > > > > > + BIT(0) | BIT(2), > > > > > + /* Differential with common byte */ > > > > > + GENMASK(3, 0), > > > > The packing of data isn't going to be good. How bad to shuffle > > > > to put the two small channels next to each other? > > > > Seems like it means you will want to combine your deinterleave > > > > and channel specific handling above, which is a bit fiddly but > > > > not much worse than current code. > > > > > > I can do it since that was what I had done in the RFC in the first place. > > > Nuno asked for in this email > > > https://lore.kernel.org/r/0036d44542f8cf45c91c867f0ddd7b45d1904d6b.camel@gmail.com/ > > > : > > > > > > > > > * You're pushing the CM channels into the end. So when we a 2 channel > > > > > > device > > > > > > we'll have: > > > > > > > > > in_voltage0 - diff > > > > > > in_voltage1 - diff > > > > > > in_voltage2 - CM associated with chan0 > > > > > > in_voltage0 - CM associated with chan1 > > > > > > > > > > > > I think we could make it so the CM channel comes right after the channel > > > > > > where > > > > > > it's data belongs too. So for example, odd channels would be CM channels > > > > > > (and > > > > > > labels could also make sense). > > > > > > So that's what I did here :D > > > > > > For the software side off things here it doesn't change a lot of things > > > since we have to manipulate the data anyway, putting the extra byte at the > > > end or in between is no extra work. > > > For the offload engine however, it should be easier to ask for 24 bits > > > then 8 bits for each channel as it would return two u32 per "hardware > > > channel". > > > > > > In order to avoid having two different layouts, I was kind of sold by > > > Nuno's idea of having the CM in between each diff channel. > > > > > > > Tbh, I was not even thinking about the layout when I proposed the arrangement. > > Just > > made sense to me (from a logical point of view) to have them together as they > > relate > > to the same physical channel. FWIW, we're also speaking bytes in here so not sure > > if > > it's that important (or bad). > > The best we can do (if we managed to do it HDL wise) is to reorder the > data to get both CM byte in a single u32 after the 2 u32 of both diff > channel. That would be 3 u32 instead of 4. > We are starting to see more and more devices that do stuff like this. Have one physical channel that reflects in more than one IIO channel. For SW buffering it's not really a big deal but for HW buffering it's not ideal. I feel that at some point we should think about having a way to map a channel scan element (being kind of a virtual scan element) into the storage_bits of another one. So in this case, one sample (for one channel) would be the 32bits and things should work the same either in SW or HW buffering. That said, it's probably easier said than done in practice :) - Nuno Sá
On Fri, 13 Sep 2024 15:46:17 +0200 Nuno Sá <noname.nuno@gmail.com> wrote: > On Fri, 2024-09-13 at 12:55 +0000, Esteban Blanc wrote: > > On Fri Sep 13, 2024 at 10:18 AM UTC, Nuno Sá wrote: > > > On Fri, 2024-09-13 at 09:55 +0000, Esteban Blanc wrote: > > > > On Mon Aug 26, 2024 at 9:27 AM UTC, Jonathan Cameron wrote: > > > > > On Thu, 22 Aug 2024 14:45:20 +0200 > > > > > Esteban Blanc <eblanc@baylibre.com> wrote: > > > > > > +static const unsigned long ad4630_channel_masks[] = { > > > > > > + /* Differential only */ > > > > > > + BIT(0) | BIT(2), > > > > > > + /* Differential with common byte */ > > > > > > + GENMASK(3, 0), > > > > > The packing of data isn't going to be good. How bad to shuffle > > > > > to put the two small channels next to each other? > > > > > Seems like it means you will want to combine your deinterleave > > > > > and channel specific handling above, which is a bit fiddly but > > > > > not much worse than current code. > > > > > > > > I can do it since that was what I had done in the RFC in the first place. > > > > Nuno asked for in this email > > > > https://lore.kernel.org/r/0036d44542f8cf45c91c867f0ddd7b45d1904d6b.camel@gmail.com/ > > > > : > > > > > > > > > > > * You're pushing the CM channels into the end. So when we a 2 channel > > > > > > > device > > > > > > > we'll have: > > > > > > > > > > > in_voltage0 - diff > > > > > > > in_voltage1 - diff > > > > > > > in_voltage2 - CM associated with chan0 > > > > > > > in_voltage0 - CM associated with chan1 > > > > > > > > > > > > > > I think we could make it so the CM channel comes right after the channel > > > > > > > where > > > > > > > it's data belongs too. So for example, odd channels would be CM channels > > > > > > > (and > > > > > > > labels could also make sense). > > > > > > > > So that's what I did here :D > > > > > > > > For the software side off things here it doesn't change a lot of things > > > > since we have to manipulate the data anyway, putting the extra byte at the > > > > end or in between is no extra work. > > > > For the offload engine however, it should be easier to ask for 24 bits > > > > then 8 bits for each channel as it would return two u32 per "hardware > > > > channel". > > > > > > > > In order to avoid having two different layouts, I was kind of sold by > > > > Nuno's idea of having the CM in between each diff channel. > > > > > > > > > > Tbh, I was not even thinking about the layout when I proposed the arrangement. > > > Just > > > made sense to me (from a logical point of view) to have them together as they > > > relate > > > to the same physical channel. FWIW, we're also speaking bytes in here so not sure > > > if > > > it's that important (or bad). > > > > The best we can do (if we managed to do it HDL wise) is to reorder the > > data to get both CM byte in a single u32 after the 2 u32 of both diff > > channel. That would be 3 u32 instead of 4. Entirely up to you. :) > > > > We are starting to see more and more devices that do stuff like this. Have one > physical channel that reflects in more than one IIO channel. For SW buffering it's > not really a big deal but for HW buffering it's not ideal. > > I feel that at some point we should think about having a way to map a channel scan > element (being kind of a virtual scan element) into the storage_bits of another one. > So in this case, one sample (for one channel) would be the 32bits and things should > work the same either in SW or HW buffering. > > That said, it's probably easier said than done in practice :) Yeah. That could get ugly fast + All existing userspace will fail to handle it so I'm not keen. Maybe it's doable if we assume the 'virtual channels' are all meta data we don't mind loosing with existing software stacks and define a non overlapping ABI to identify the metadata. Still smells bad to me so I'll take quite a bit of convincing! Adding something to clearly 'associate' multiple related channels would be fine as that wouldn't change the data interpretation, just provide more info on top. Kind of a structured _label Maybe a _channelgroup attribute? Would be const and all the channels with the same index would reflect that they were measured on same 'thing'. Typically thing might be a pin or differential pair, but we might be measuring different types of signals - e.g. current and power. Joanthan > > - Nuno Sá >
On Sat, 2024-09-14 at 12:25 +0100, Jonathan Cameron wrote: > On Fri, 13 Sep 2024 15:46:17 +0200 > Nuno Sá <noname.nuno@gmail.com> wrote: > > > On Fri, 2024-09-13 at 12:55 +0000, Esteban Blanc wrote: > > > On Fri Sep 13, 2024 at 10:18 AM UTC, Nuno Sá wrote: > > > > On Fri, 2024-09-13 at 09:55 +0000, Esteban Blanc wrote: > > > > > On Mon Aug 26, 2024 at 9:27 AM UTC, Jonathan Cameron wrote: > > > > > > On Thu, 22 Aug 2024 14:45:20 +0200 > > > > > > Esteban Blanc <eblanc@baylibre.com> wrote: > > > > > > > +static const unsigned long ad4630_channel_masks[] = { > > > > > > > + /* Differential only */ > > > > > > > + BIT(0) | BIT(2), > > > > > > > + /* Differential with common byte */ > > > > > > > + GENMASK(3, 0), > > > > > > The packing of data isn't going to be good. How bad to shuffle > > > > > > to put the two small channels next to each other? > > > > > > Seems like it means you will want to combine your deinterleave > > > > > > and channel specific handling above, which is a bit fiddly but > > > > > > not much worse than current code. > > > > > > > > > > I can do it since that was what I had done in the RFC in the first place. > > > > > Nuno asked for in this email > > > > > https://lore.kernel.org/r/0036d44542f8cf45c91c867f0ddd7b45d1904d6b.camel@gmail.com/ > > > > > : > > > > > > > > > > > > > * You're pushing the CM channels into the end. So when we a 2 channel > > > > > > > > device > > > > > > > > we'll have: > > > > > > > > > > > > > in_voltage0 - diff > > > > > > > > in_voltage1 - diff > > > > > > > > in_voltage2 - CM associated with chan0 > > > > > > > > in_voltage0 - CM associated with chan1 > > > > > > > > > > > > > > > > I think we could make it so the CM channel comes right after the > > > > > > > > channel > > > > > > > > where > > > > > > > > it's data belongs too. So for example, odd channels would be CM > > > > > > > > channels > > > > > > > > (and > > > > > > > > labels could also make sense). > > > > > > > > > > So that's what I did here :D > > > > > > > > > > For the software side off things here it doesn't change a lot of things > > > > > since we have to manipulate the data anyway, putting the extra byte at the > > > > > end or in between is no extra work. > > > > > For the offload engine however, it should be easier to ask for 24 bits > > > > > then 8 bits for each channel as it would return two u32 per "hardware > > > > > channel". > > > > > > > > > > In order to avoid having two different layouts, I was kind of sold by > > > > > Nuno's idea of having the CM in between each diff channel. > > > > > > > > > > > > > Tbh, I was not even thinking about the layout when I proposed the > > > > arrangement. > > > > Just > > > > made sense to me (from a logical point of view) to have them together as they > > > > relate > > > > to the same physical channel. FWIW, we're also speaking bytes in here so not > > > > sure > > > > if > > > > it's that important (or bad). > > > > > > The best we can do (if we managed to do it HDL wise) is to reorder the > > > data to get both CM byte in a single u32 after the 2 u32 of both diff > > > channel. That would be 3 u32 instead of 4. > > Entirely up to you. :) > > > > > > > We are starting to see more and more devices that do stuff like this. Have one > > physical channel that reflects in more than one IIO channel. For SW buffering > > it's > > not really a big deal but for HW buffering it's not ideal. > > > > I feel that at some point we should think about having a way to map a channel > > scan > > element (being kind of a virtual scan element) into the storage_bits of another > > one. > > So in this case, one sample (for one channel) would be the 32bits and things > > should > > work the same either in SW or HW buffering. > > > > That said, it's probably easier said than done in practice :) > > Yeah. That could get ugly fast + All existing userspace will fail to handle it > so I'm not keen. Maybe it's doable if we assume the 'virtual channels' are all > meta data we don't mind loosing with existing software stacks and define > a non overlapping ABI to identify the metadata. Still smells bad to me so > I'll take quite a bit of convincing! Naturally it would have to be done in a way that drivers not defining the "special" scan elements would not be affected. > > Adding something to clearly 'associate' multiple related channels would be fine > as that wouldn't change the data interpretation, just provide more info on top. > Kind of a structured _label > > Maybe a _channelgroup attribute? Would be const and all the channels with > the same index would reflect that they were measured on same 'thing'. > Typically thing might be a pin or differential pair, but we might be measuring > different types of signals - e.g. current and power. > Sounds reasonable but I think the tricky part is always to have a sane way of saying that multiple scan elements relate to just one storage_bits so we could say something like (taking this as example): scan0: //diff channel which describing the physical HW in terms of real size .storage_bits = 32 .real_bits = 24 .shift = 8 scan1: //CM data //.storage - relates to scan0 so should add nothing to the sample size if both enabled .real_bits = 8 Likely not what you meant but one thing I took from your '_channelgroup' idea was to have something similar to extended_info maybe with a small top level description and then an array of channels (that would form the group/aggregated channel). Only on the top level description we would be allowed to define the size of the scan element (in case of buffering). Still seems tricky to me :). Anyways, Right now, I have no time for something like this but eventually would like to try something. But if someone wants to propose something sooner, please :) - Nuno Sá
On Sat Sep 14, 2024 at 11:25 AM UTC, Jonathan Cameron wrote: > On Fri, 13 Sep 2024 15:46:17 +0200 > Nuno Sá <noname.nuno@gmail.com> wrote: > > > On Fri, 2024-09-13 at 12:55 +0000, Esteban Blanc wrote: > > > On Fri Sep 13, 2024 at 10:18 AM UTC, Nuno Sá wrote: > > > > On Fri, 2024-09-13 at 09:55 +0000, Esteban Blanc wrote: > > > > > On Mon Aug 26, 2024 at 9:27 AM UTC, Jonathan Cameron wrote: > > > > > > On Thu, 22 Aug 2024 14:45:20 +0200 > > > > > > Esteban Blanc <eblanc@baylibre.com> wrote: > > > > > > > +static const unsigned long ad4630_channel_masks[] = { > > > > > > > + /* Differential only */ > > > > > > > + BIT(0) | BIT(2), > > > > > > > + /* Differential with common byte */ > > > > > > > + GENMASK(3, 0), > > > > > > The packing of data isn't going to be good. How bad to shuffle > > > > > > to put the two small channels next to each other? > > > > > > Seems like it means you will want to combine your deinterleave > > > > > > and channel specific handling above, which is a bit fiddly but > > > > > > not much worse than current code. > > > > > > > > > > I can do it since that was what I had done in the RFC in the first place. > > > > > Nuno asked for in this email > > > > > https://lore.kernel.org/r/0036d44542f8cf45c91c867f0ddd7b45d1904d6b.camel@gmail.com/ > > > > > : > > > > > > > > > > > > > * You're pushing the CM channels into the end. So when we a 2 channel > > > > > > > > device > > > > > > > > we'll have: > > > > > > > > > > > > > in_voltage0 - diff > > > > > > > > in_voltage1 - diff > > > > > > > > in_voltage2 - CM associated with chan0 > > > > > > > > in_voltage0 - CM associated with chan1 > > > > > > > > > > > > > > > > I think we could make it so the CM channel comes right after the channel > > > > > > > > where > > > > > > > > it's data belongs too. So for example, odd channels would be CM channels > > > > > > > > (and > > > > > > > > labels could also make sense). > > > > > > > > > > So that's what I did here :D > > > > > > > > > > For the software side off things here it doesn't change a lot of things > > > > > since we have to manipulate the data anyway, putting the extra byte at the > > > > > end or in between is no extra work. > > > > > For the offload engine however, it should be easier to ask for 24 bits > > > > > then 8 bits for each channel as it would return two u32 per "hardware > > > > > channel". > > > > > > > > > > In order to avoid having two different layouts, I was kind of sold by > > > > > Nuno's idea of having the CM in between each diff channel. > > > > > > > > > > > > > Tbh, I was not even thinking about the layout when I proposed the arrangement. > > > > Just > > > > made sense to me (from a logical point of view) to have them together as they > > > > relate > > > > to the same physical channel. FWIW, we're also speaking bytes in here so not sure > > > > if > > > > it's that important (or bad). > > > > > > The best we can do (if we managed to do it HDL wise) is to reorder the > > > data to get both CM byte in a single u32 after the 2 u32 of both diff > > > channel. That would be 3 u32 instead of 4. > > Entirely up to you. :) Ok so here is the plan I propose: 1. Use the layout of this patch (common byte channels just after their respective diff channel) as it should work out of the box for the offload engine (once it's merged [1]). 2. In case of performance issue, switch to the RFC layout (both diff channels then both common byte channels) and try to modify the HDL for the offload engine to reduce the memory footprint by one byte for the 2 hardware channels case. [1]: https://lore.kernel.org/lkml/20240722-dlech-mainline-spi-engine-offload-2-v3-0-7420e45df69b@baylibre.com/ Best regards,
On Fri Sep 13, 2024 at 9:55 AM UTC, Esteban Blanc wrote: > On Mon Aug 26, 2024 at 9:27 AM UTC, Jonathan Cameron wrote: > > On Thu, 22 Aug 2024 14:45:20 +0200 > > Esteban Blanc <eblanc@baylibre.com> wrote: > > > @@ -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]; > > > + } > > > > I wonder if doing it in place is actually worthwhile. Maybe unpack into a second > > array? That is still fairly small and may make code easier to read. > > Okay sure Actually I can't consolidate the differential only mode and the common byte mode without having to create a bunch of if/else or having a memcpy. The best I can do is this, but I don't like it: ``` static int ad4030_conversion(struct ad4030_state *st, const struct iio_chan_spec *chan) { ... u32 tmp[AD4030_MAX_HARDWARE_CHANNEL_NB]; u32 *diff; ... if (st->mode != AD4030_OUT_DATA_MD_16_DIFF_8_COM && st->mode != AD4030_OUT_DATA_MD_24_DIFF_8_COM) { if (st->chip->num_voltage_inputs == 2) ad4030_extract_interleaved(st->rx_data.raw, &st->rx_data.diff[0], &st->rx_data.diff[1]); return 0; } if (st->chip->num_voltage_inputs == 2) { ad4030_extract_interleaved(st->rx_data.raw, &tmp[0], &tmp[1]); diff = tmp; } else { diff = st->rx_data.diff; } common_byte_mask = BITS_TO_BYTES(chan->scan_type.realbits); for (i = 0; i < st->chip->num_voltage_inputs; i++) { st->rx_data.buffered[i].val = diff[i]; st->rx_data.buffered[i].common = ((u8 *)(diff + i))[common_byte_mask]; } return 0; ``` The root cause is that when we are in a differential only mode we are leaving before the for loop and we want the data to be in the rx_data. It fells clunky and brain consuming IMAO, I prefer a reversed loop with a good comment (the one we have now is not explicit enough, I will update it).
On Mon, 16 Sep 2024 09:19:02 +0000 "Esteban Blanc" <eblanc@baylibre.com> wrote: > On Sat Sep 14, 2024 at 11:25 AM UTC, Jonathan Cameron wrote: > > On Fri, 13 Sep 2024 15:46:17 +0200 > > Nuno Sá <noname.nuno@gmail.com> wrote: > > > > > On Fri, 2024-09-13 at 12:55 +0000, Esteban Blanc wrote: > > > > On Fri Sep 13, 2024 at 10:18 AM UTC, Nuno Sá wrote: > > > > > On Fri, 2024-09-13 at 09:55 +0000, Esteban Blanc wrote: > > > > > > On Mon Aug 26, 2024 at 9:27 AM UTC, Jonathan Cameron wrote: > > > > > > > On Thu, 22 Aug 2024 14:45:20 +0200 > > > > > > > Esteban Blanc <eblanc@baylibre.com> wrote: > > > > > > > > +static const unsigned long ad4630_channel_masks[] = { > > > > > > > > + /* Differential only */ > > > > > > > > + BIT(0) | BIT(2), > > > > > > > > + /* Differential with common byte */ > > > > > > > > + GENMASK(3, 0), > > > > > > > The packing of data isn't going to be good. How bad to shuffle > > > > > > > to put the two small channels next to each other? > > > > > > > Seems like it means you will want to combine your deinterleave > > > > > > > and channel specific handling above, which is a bit fiddly but > > > > > > > not much worse than current code. > > > > > > > > > > > > I can do it since that was what I had done in the RFC in the first place. > > > > > > Nuno asked for in this email > > > > > > https://lore.kernel.org/r/0036d44542f8cf45c91c867f0ddd7b45d1904d6b.camel@gmail.com/ > > > > > > : > > > > > > > > > > > > > > > * You're pushing the CM channels into the end. So when we a 2 channel > > > > > > > > > device > > > > > > > > > we'll have: > > > > > > > > > > > > > > > in_voltage0 - diff > > > > > > > > > in_voltage1 - diff > > > > > > > > > in_voltage2 - CM associated with chan0 > > > > > > > > > in_voltage0 - CM associated with chan1 > > > > > > > > > > > > > > > > > > I think we could make it so the CM channel comes right after the channel > > > > > > > > > where > > > > > > > > > it's data belongs too. So for example, odd channels would be CM channels > > > > > > > > > (and > > > > > > > > > labels could also make sense). > > > > > > > > > > > > So that's what I did here :D > > > > > > > > > > > > For the software side off things here it doesn't change a lot of things > > > > > > since we have to manipulate the data anyway, putting the extra byte at the > > > > > > end or in between is no extra work. > > > > > > For the offload engine however, it should be easier to ask for 24 bits > > > > > > then 8 bits for each channel as it would return two u32 per "hardware > > > > > > channel". > > > > > > > > > > > > In order to avoid having two different layouts, I was kind of sold by > > > > > > Nuno's idea of having the CM in between each diff channel. > > > > > > > > > > > > > > > > Tbh, I was not even thinking about the layout when I proposed the arrangement. > > > > > Just > > > > > made sense to me (from a logical point of view) to have them together as they > > > > > relate > > > > > to the same physical channel. FWIW, we're also speaking bytes in here so not sure > > > > > if > > > > > it's that important (or bad). > > > > > > > > The best we can do (if we managed to do it HDL wise) is to reorder the > > > > data to get both CM byte in a single u32 after the 2 u32 of both diff > > > > channel. That would be 3 u32 instead of 4. > > > > Entirely up to you. :) > > Ok so here is the plan I propose: > 1. Use the layout of this patch (common byte channels just after their > respective diff channel) as it should work out of the box for the offload > engine (once it's merged [1]). > 2. In case of performance issue, switch to the RFC layout (both diff > channels then both common byte channels) and try to modify the HDL for > the offload engine to reduce the memory footprint by one byte for the 2 > hardware channels case. It's a bit of a risk as you might get someone making too many assumptions about channel ordering in some specific purpose code - at that point this change is an ABI break that someone noticed. So I'd guess we'll stay with 1 for ever which is find. Jonathan > > [1]: https://lore.kernel.org/lkml/20240722-dlech-mainline-spi-engine-offload-2-v3-0-7420e45df69b@baylibre.com/ > > Best regards, >
On Mon, 16 Sep 2024 08:12:24 +0200 Nuno Sá <noname.nuno@gmail.com> wrote: > On Sat, 2024-09-14 at 12:25 +0100, Jonathan Cameron wrote: > > On Fri, 13 Sep 2024 15:46:17 +0200 > > Nuno Sá <noname.nuno@gmail.com> wrote: > > > > > On Fri, 2024-09-13 at 12:55 +0000, Esteban Blanc wrote: > > > > On Fri Sep 13, 2024 at 10:18 AM UTC, Nuno Sá wrote: > > > > > On Fri, 2024-09-13 at 09:55 +0000, Esteban Blanc wrote: > > > > > > On Mon Aug 26, 2024 at 9:27 AM UTC, Jonathan Cameron wrote: > > > > > > > On Thu, 22 Aug 2024 14:45:20 +0200 > > > > > > > Esteban Blanc <eblanc@baylibre.com> wrote: > > > > > > > > +static const unsigned long ad4630_channel_masks[] = { > > > > > > > > + /* Differential only */ > > > > > > > > + BIT(0) | BIT(2), > > > > > > > > + /* Differential with common byte */ > > > > > > > > + GENMASK(3, 0), > > > > > > > The packing of data isn't going to be good. How bad to shuffle > > > > > > > to put the two small channels next to each other? > > > > > > > Seems like it means you will want to combine your deinterleave > > > > > > > and channel specific handling above, which is a bit fiddly but > > > > > > > not much worse than current code. > > > > > > > > > > > > I can do it since that was what I had done in the RFC in the first place. > > > > > > Nuno asked for in this email > > > > > > https://lore.kernel.org/r/0036d44542f8cf45c91c867f0ddd7b45d1904d6b.camel@gmail.com/ > > > > > > : > > > > > > > > > > > > > > > * You're pushing the CM channels into the end. So when we a 2 channel > > > > > > > > > device > > > > > > > > > we'll have: > > > > > > > > > > > > > > > in_voltage0 - diff > > > > > > > > > in_voltage1 - diff > > > > > > > > > in_voltage2 - CM associated with chan0 > > > > > > > > > in_voltage0 - CM associated with chan1 > > > > > > > > > > > > > > > > > > I think we could make it so the CM channel comes right after the > > > > > > > > > channel > > > > > > > > > where > > > > > > > > > it's data belongs too. So for example, odd channels would be CM > > > > > > > > > channels > > > > > > > > > (and > > > > > > > > > labels could also make sense). > > > > > > > > > > > > So that's what I did here :D > > > > > > > > > > > > For the software side off things here it doesn't change a lot of things > > > > > > since we have to manipulate the data anyway, putting the extra byte at the > > > > > > end or in between is no extra work. > > > > > > For the offload engine however, it should be easier to ask for 24 bits > > > > > > then 8 bits for each channel as it would return two u32 per "hardware > > > > > > channel". > > > > > > > > > > > > In order to avoid having two different layouts, I was kind of sold by > > > > > > Nuno's idea of having the CM in between each diff channel. > > > > > > > > > > > > > > > > Tbh, I was not even thinking about the layout when I proposed the > > > > > arrangement. > > > > > Just > > > > > made sense to me (from a logical point of view) to have them together as they > > > > > relate > > > > > to the same physical channel. FWIW, we're also speaking bytes in here so not > > > > > sure > > > > > if > > > > > it's that important (or bad). > > > > > > > > The best we can do (if we managed to do it HDL wise) is to reorder the > > > > data to get both CM byte in a single u32 after the 2 u32 of both diff > > > > channel. That would be 3 u32 instead of 4. > > > > Entirely up to you. :) > > > > > > > > > > We are starting to see more and more devices that do stuff like this. Have one > > > physical channel that reflects in more than one IIO channel. For SW buffering > > > it's > > > not really a big deal but for HW buffering it's not ideal. > > > > > > I feel that at some point we should think about having a way to map a channel > > > scan > > > element (being kind of a virtual scan element) into the storage_bits of another > > > one. > > > So in this case, one sample (for one channel) would be the 32bits and things > > > should > > > work the same either in SW or HW buffering. > > > > > > That said, it's probably easier said than done in practice :) > > > > Yeah. That could get ugly fast + All existing userspace will fail to handle it > > so I'm not keen. Maybe it's doable if we assume the 'virtual channels' are all > > meta data we don't mind loosing with existing software stacks and define > > a non overlapping ABI to identify the metadata. Still smells bad to me so > > I'll take quite a bit of convincing! > > Naturally it would have to be done in a way that drivers not defining the "special" > scan elements would not be affected. It's worse than that - it would need be defined so userspace running against the devices with the special channels would have to work without knowing anything about them. So we couldn't do the really nasty thing of setting scan_index the same for both of them with same storage size and different shifts and real_bits. That would be the sort of things that might crash userspace code. Driver effects are less of an issue than ABI breakage - or even just ABI a userspace author would not expect. > > > > > Adding something to clearly 'associate' multiple related channels would be fine > > as that wouldn't change the data interpretation, just provide more info on top. > > Kind of a structured _label > > > > Maybe a _channelgroup attribute? Would be const and all the channels with > > the same index would reflect that they were measured on same 'thing'. > > Typically thing might be a pin or differential pair, but we might be measuring > > different types of signals - e.g. current and power. > > > > Sounds reasonable but I think the tricky part is always to have a sane way of saying > that multiple scan elements relate to just one storage_bits so we could say something > like (taking this as example): > > scan0: //diff channel which describing the physical HW in terms of real size > .storage_bits = 32 > .real_bits = 24 > .shift = 8 > > scan1: //CM data > //.storage - relates to scan0 so should add nothing to the sample size if both > enabled > .real_bits = 8 > Indeed - I get the concept, but don't like it. In general it's a dead end for general purpose channels - because of that pile of legacy userspace. It 'might' just about be acceptable for 'meta data' channels or where we are adding significant new interface for functionality purposes (e.g. when we did the newer DMA buffer stuff). > Likely not what you meant but one thing I took from your '_channelgroup' idea was to > have something similar to extended_info maybe with a small top level description and > then an array of channels (that would form the group/aggregated channel). Only on the > top level description we would be allowed to define the size of the scan element (in > case of buffering). Still seems tricky to me :). Yeah. The channel group thing was for normal naturally aligned packing, not data backed tighter than that. > > Anyways, Right now, I have no time for something like this but eventually would like > to try something. But if someone wants to propose something sooner, please :) *looks doubtful* Maybe I can be convinced. We'll see. Jonathan > > - Nuno Sá >
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, \ .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; + } 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]; 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, 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), + /* 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] = { .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);
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@baylibre.com> --- drivers/iio/adc/ad4030.c | 197 +++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 173 insertions(+), 24 deletions(-)