Re: [PATCH v6 6/7] iio: adc: Add support for AD4000

[Jonathan Cameron <jic23@xxxxxxxxxx>]

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, &reg_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;
> +	}
> +}