On Sat, 29 Jun 2024 16:06:59 -0300
Marcelo Schmitt <marcelo.schmitt@xxxxxxxxxx> wrote:
> Add support for AD4000 series of low noise, low power, high speed,
> successive approximation register (SAR) ADCs.
>
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@xxxxxxxxxx>
Hi Marcelo
A few comments inline. However, the spi_w8r8 etc can easily be a follow up
optimization patch (if you agree it's a good improvement) and the
other changes are so trivial I could tweak whilst applying.
So unless you have to do a v7 for some other reason this is fine for
merging as is - subject to the fact it's not been on list long enough yet
and I need Mark to pick up the SPI parts and throw me a tag to pull.
Thanks,
Jonathan
> --- /dev/null
> +++ b/drivers/iio/adc/ad4000.c
> @@ -0,0 +1,708 @@
> +
> +struct ad4000_state {
> + struct spi_device *spi;
> + struct gpio_desc *cnv_gpio;
> + struct spi_transfer xfers[2];
> + struct spi_message msg;
> + struct mutex lock; /* Protect read modify write cycle */
> + int vref_mv;
> + enum ad4000_sdi sdi_pin;
> + bool span_comp;
> + u16 gain_milli;
> + int scale_tbl[AD4000_SCALE_OPTIONS][2];
> +
> + /*
> + * DMA (thus cache coherency maintenance) requires the transfer buffers
> + * to live in their own cache lines.
> + */
> + struct {
> + union {
> + __be16 sample_buf16;
> + __be32 sample_buf32;
> + } data;
> + s64 timestamp __aligned(8);
> + } scan __aligned(IIO_DMA_MINALIGN);
> + u8 tx_buf[2];
> + u8 rx_buf[2];
If you made the spi_w8r8() change suggested below (which uses a bounce buffer
in the spi core), rx_buf would be unused and can go away.
Given I think registers accesses on this device are all off the fast path
you could even use spi_write_then_read() with zero read size for the
register writes and rely on the spi core bounce buffers.
That way tx_buf goes away as well leaving you with the dma
safe buffer for only the fast path reads.
> +};
> +
> +static void ad4000_fill_scale_tbl(struct ad4000_state *st,
> + struct iio_chan_spec const *chan)
> +{
> + int val, tmp0, tmp1;
> + int scale_bits;
> + u64 tmp2;
> +
> + /*
> + * ADCs that output two's complement code have one less bit to express
> + * voltage magnitude.
> + */
> + if (chan->scan_type.sign == 's')
> + scale_bits = chan->scan_type.realbits - 1;
> + else
> + scale_bits = chan->scan_type.realbits;
> +
> + /*
> + * The gain is stored as a fraction of 1000 and, as we need to
> + * divide vref_mv by the gain, we invert the gain/1000 fraction.
> + * Also multiply by an extra MILLI to preserve precision.
> + * Thus, we have MILLI * MILLI equals MICRO as fraction numerator.
> + */
> + val = mult_frac(st->vref_mv, MICRO, st->gain_milli);
If you are rolling a v7 for other reasons, stick some line breaks in here!
It's a bit of a mass of text that is hard for my eyes to parse!
> + /* Would multiply by NANO here but we multiplied by extra MILLI */
> + tmp2 = shift_right((u64)val * MICRO, scale_bits);
> + tmp0 = div_s64_rem(tmp2, NANO, &tmp1);
> + /* Store scale for when span compression is disabled */
> + st->scale_tbl[0][0] = tmp0; /* Integer part */
> + st->scale_tbl[0][1] = abs(tmp1); /* Fractional part */
> + /* Store scale for when span compression is enabled */
> + st->scale_tbl[1][0] = tmp0;
> + /* The integer part is always zero so don't bother to divide it. */
> + if (chan->differential)
> + st->scale_tbl[1][1] = DIV_ROUND_CLOSEST(abs(tmp1) * 4, 5);
> + else
> + st->scale_tbl[1][1] = DIV_ROUND_CLOSEST(abs(tmp1) * 9, 10);
> +}
> +static int ad4000_read_reg(struct ad4000_state *st, unsigned int *val)
> +{
> + struct spi_transfer t = {
> + .tx_buf = st->tx_buf,
> + .rx_buf = st->rx_buf,
> + .len = 2,
> + };
> + int ret;
> +
> + st->tx_buf[0] = AD4000_READ_COMMAND;
> + ret = spi_sync_transfer(st->spi, &t, 1);
> + if (ret < 0)
> + return ret;
> +
> + *val = st->rx_buf[1];
> + return ret;
I'd be tempted to do
ssize_t ret;
ret = spi_w8r8(AD4000_READ_COMMAND);
if (ret < 0)
return ret;
*val = ret;
return 0;
> +}
> +static int ad4000_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int val, int val2,
> + long mask)
> +{
> + struct ad4000_state *st = iio_priv(indio_dev);
> + unsigned int reg_val;
> + bool span_comp_en;
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SCALE:
> + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> + guard(mutex)(&st->lock);
> +
> + ret = ad4000_read_reg(st, ®_val);
> + if (ret < 0)
> + return ret;
> +
> + span_comp_en = val2 == st->scale_tbl[1][1];
> + reg_val &= ~AD4000_CFG_SPAN_COMP;
> + reg_val |= FIELD_PREP(AD4000_CFG_SPAN_COMP, span_comp_en);
> +
> + ret = ad4000_write_reg(st, reg_val);
> + if (ret < 0)
> + return ret;
> +
> + st->span_comp = span_comp_en;
> + return ret;
If you are spinning for another reason, make it clear this is always good.
The spi_write() never returns positive so current code is correct but I had
to go check which this would have avoided.
return 0;
If nothing else comes up, I'll probably tweak whilst applying.
J
> + }
> + unreachable();
> + default:
> + return -EINVAL;
> + }
> +}
