On Mon, 23 Sep 2024 13:10:23 +0300
Antoniu Miclaus <antoniu.miclaus@xxxxxxxxxx> wrote:
> Add support for the AD485X DAS familiy.
>
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@xxxxxxxxxx>
A few additional comments from me.
Thanks,
Jonathan
> diff --git a/drivers/iio/adc/ad485x.c b/drivers/iio/adc/ad485x.c
> new file mode 100644
> index 000000000000..3333cad9ed8f
> --- /dev/null
> +++ b/drivers/iio/adc/ad485x.c
> @@ -0,0 +1,1061 @@
> +static int ad485x_setup(struct ad485x_state *st)
> +{
> + unsigned int product_id;
> + int ret;
> +
> + ret = ad485x_set_sampling_freq(st, 1000000);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(st->regmap, AD485X_REG_INTERFACE_CONFIG_A,
> + AD485X_SW_RESET);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(st->regmap, AD485X_REG_INTERFACE_CONFIG_B,
> + AD485X_SINGLE_INSTRUCTION);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(st->regmap, AD485X_REG_INTERFACE_CONFIG_A,
> + AD485X_SDO_ENABLE);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(st->regmap, AD485X_REG_PRODUCT_ID_L, &product_id);
> + if (ret)
> + return ret;
> +
> + if (product_id != st->info->product_id)
> + dev_warn(&st->spi->dev, "Unknown product ID: 0x%02X\n",
> + product_id);
dev_info() for this is probably better. Might be fine afterall if
the new part is fully backwards compatible with this one.
> +
> + ret = regmap_write(st->regmap, AD485X_REG_DEVICE_CTRL,
> + AD485X_ECHO_CLOCK_MODE);
> + if (ret)
> + return ret;
> +
> + return regmap_write(st->regmap, AD485X_REG_PACKET, 0);
> +}
> +
> +static int ad485x_get_packet_format(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan)
> +{
> + struct ad485x_state *st = iio_priv(indio_dev);
> + unsigned int format;
> + int ret;
> +
> + ret = regmap_read(st->regmap, AD485X_REG_PACKET, &format);
> + if (ret)
> + return ret;
> +
> + format = FIELD_GET(AD485X_PACKET_FORMAT_MASK, format);
> +
> + return format;
return FIELD_GET()
> +}
> +
> +static int ad485x_get_calibscale(struct ad485x_state *st, int ch, int *val,
> + int *val2)
> +{
> + unsigned int reg_val;
> + int gain;
> + int ret;
> +
> + guard(mutex)(&st->lock);
> +
> + ret = regmap_read(st->regmap, AD485X_REG_CHX_GAIN_MSB(ch),
> + ®_val);
> + if (ret)
> + return ret;
> +
> + gain = (reg_val & 0xFF) << 8;
Use a byte array into which you write the regval then a get_unalignedb_be16
or similar to get the value.
> +
> + ret = regmap_read(st->regmap, AD485X_REG_CHX_GAIN_LSB(ch),
> + ®_val);
> + if (ret)
> + return ret;
> +
> + gain |= reg_val & 0xFF;
> +
> + *val = gain;
> + *val2 = 32768;
> +
> + return IIO_VAL_FRACTIONAL;
> +}
> +static int ad485x_get_calibbias(struct ad485x_state *st, int ch, int *val,
> + int *val2)
> +{
> + unsigned int lsb, mid, msb;
> + int ret;
> +
> + guard(mutex)(&st->lock);
> +
> + ret = regmap_read(st->regmap, AD485X_REG_CHX_OFFSET_MSB(ch),
> + &msb);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(st->regmap, AD485X_REG_CHX_OFFSET_MID(ch),
> + &mid);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(st->regmap, AD485X_REG_CHX_OFFSET_LSB(ch),
> + &lsb);
> + if (ret)
> + return ret;
> +
> + if (st->info->resolution == 16) {
> + *val = msb << 8;
> + *val |= mid;
> + *val = sign_extend32(*val, 15);
> + } else {
> + *val = msb << 12;
> + *val |= mid << 4;
> + *val |= lsb >> 4;
As below I'd use a byte array then you can do get_unaligned_be24 to
+ a right shift by 4 of the whole thing.
> + *val = sign_extend32(*val, 19);
> + }
> +
> + return IIO_VAL_INT;
> +}
> +
> +static int ad485x_set_calibbias(struct ad485x_state *st, int ch, int val,
> + int val2)
> +{
> + unsigned int lsb, mid, msb;
> + int ret;
> +
> + if (st->info->resolution == 16) {
> + lsb = 0;
> + mid = val & 0xFF;
> + msb = (val >> 8) & 0xFF;
> + } else {
> + lsb = (val << 4) & 0xFF;
Might as well mask by 0xF0 to make it really clear
nothing in bottom few bits.
> + mid = (val >> 4) & 0xFF;
> + msb = (val >> 12) & 0xFF;
I'd be tempted to shift the whole thing left 4 bits and
then use a put_unaligned_be24 on it into a byte array then pick
out the bytes from that array. Will be easier to read.
Above 16 bit case would be a put_unaligned_be16
> + }
> +
> + guard(mutex)(&st->lock);
> +
> + ret = regmap_write(st->regmap, AD485X_REG_CHX_OFFSET_LSB(ch), lsb);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(st->regmap, AD485X_REG_CHX_OFFSET_MID(ch), mid);
> + if (ret)
> + return ret;
> +
> + return regmap_write(st->regmap, AD485X_REG_CHX_OFFSET_MSB(ch), msb);
I think you already got this question, but consider bulk writes.
> +}
> +
> +static const unsigned int ad485x_scale_table[][2] = {
> + {2500, 0x0}, {5000, 0x1}, {5000, 0x2}, {10000, 0x3}, {6250, 0x04},
> + {12500, 0x5}, {10000, 0x6}, {20000, 0x7}, {12500, 0x8}, {25000, 0x9},
> + {20000, 0xA}, {40000, 0xB}, {25000, 0xC}, {50000, 0xD}, {40000, 0xE},
> + {80000, 0xF}
Spaces after { and before } preferred.
Maybe just have this as one item per line. I think that is more readable.
> +};
> +
> +static int ad485x_read_raw(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + int *val, int *val2, long info)
> +{
> + struct ad485x_state *st = iio_priv(indio_dev);
> +
> + switch (info) {
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + *val = st->sampling_freq;
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_CALIBSCALE:
> + return ad485x_get_calibscale(st, chan->channel, val, val2);
> + case IIO_CHAN_INFO_SCALE:
> + return ad485x_get_scale(st, chan, val, val2);
> + case IIO_CHAN_INFO_CALIBBIAS:
> + return ad485x_get_calibbias(st, chan->channel, val, val2);
> + case IIO_CHAN_INFO_OFFSET:
> + scoped_guard(mutex, &st->lock)
> + * val = st->offsets[chan->channel];
*val = ...
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +#define AD485X_IIO_CHANNEL(index, real, storage, info) \
> +{ \
> + .type = IIO_VOLTAGE, \
> + .ext_info = info, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_CALIBSCALE) | \
> + BIT(IIO_CHAN_INFO_CALIBBIAS) | \
> + BIT(IIO_CHAN_INFO_SCALE) | \
> + BIT(IIO_CHAN_INFO_OFFSET), \
> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> + .info_mask_shared_by_type_available = \
> + BIT(IIO_CHAN_INFO_SCALE) | \
> + BIT(IIO_CHAN_INFO_OFFSET), \
Maybe add avail for calibscale and calibbias as well.
> +
> +static struct iio_chan_spec_ext_info ad4858_ext_info[] = {
> + IIO_ENUM("packet_format", IIO_SHARED_BY_ALL, &ad4858_packet_fmt),
> + IIO_ENUM_AVAILABLE("packet_format",
> + IIO_SHARED_BY_ALL, &ad4858_packet_fmt),
> + {},
> +};
> +
> +static struct iio_chan_spec_ext_info ad4857_ext_info[] = {
> + IIO_ENUM("packet_format", IIO_SHARED_BY_ALL, &ad4857_packet_fmt),
> + IIO_ENUM_AVAILABLE("packet_format",
> + IIO_SHARED_BY_ALL, &ad4857_packet_fmt),
> + {},
I think Andy pointed these out. No trailing comma on terminators.
However, I'm not sure this ABI is practical currently.
Basically I have no idea what it is or how to use it!
> +};
> +
> +static const struct ad485x_chip_info ad4858i_info = {
> + .name = "ad4858i",
> + .product_id = AD4858I_PROD_ID,
> + .scale_table = ad485x_scale_table,
> + .num_scales = ARRAY_SIZE(ad485x_scale_table),
> + .offset_table = ad4858_offset_table,
> + .num_offset = ARRAY_SIZE(ad4858_offset_table),
> + .channels = ad4858_channels,
> + .num_channels = ARRAY_SIZE(ad4858_channels),
> + .throughput = 1000000,
Odd you use MEGA above, but not here.
> + .resolution = 20,
> +};
> +static int ad485x_probe(struct spi_device *spi)
> +{
> + struct iio_dev *indio_dev;
> + struct ad485x_state *st;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + st = iio_priv(indio_dev);
> + st->spi = spi;
> +
> + mutex_init(&st->lock);
Ever so slight preference for the new option of.
ret = devm_mutex_init(dev, &st->lock);
if (ret)
return ret;
...
> +}
> +
> +static const struct of_device_id ad485x_of_match[] = {
> + { .compatible = "adi,ad4858", .data = &ad4858_info, },
> + { .compatible = "adi,ad4857", .data = &ad4857_info, },
> + { .compatible = "adi,ad4856", .data = &ad4856_info, },
> + { .compatible = "adi,ad4855", .data = &ad4855_info, },
> + { .compatible = "adi,ad4854", .data = &ad4854_info, },
> + { .compatible = "adi,ad4853", .data = &ad4853_info, },
> + { .compatible = "adi,ad4852", .data = &ad4852_info, },
> + { .compatible = "adi,ad4851", .data = &ad4851_info, },
> + { .compatible = "adi,ad4858i", .data = &ad4858i_info, },
> + {}
{ }
> +};
> +
