Message-ID: <20240823201913.5bded18f@jic23-huawei>
Date: Fri, 23 Aug 2024 20:19:13 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Alexandru Ardelean <aardelean@...libre.com>
Cc: linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org, krzk+dt@...nel.org, robh@...nel.org,
 lars@...afoo.de, michael.hennerich@...log.com, gstols@...libre.com
Subject: Re: [PATCH 7/7] iio: adc: ad7606: add support for AD7606C-{16,18}
 parts

On Mon, 19 Aug 2024 09:47:17 +0300
Alexandru Ardelean <aardelean@...libre.com> wrote:

> The AD7606C-16 and AD7606C-18 are pretty similar with the AD7606B.
> The main difference between AD7606C-16 & AD7606C-18 is the precision in
> bits (16 vs 18).
> Because of that, some scales need to be defined for the 18-bit variants, as
> they need to be computed against 2**18 (vs 2**16 for the 16 bit-variants).
> 
> Because the AD7606C-16,18 also supports bipolar & differential channels,
> for SW-mode, the default range of 10 V or ±10V should be set at probe.
> On reset, the default range (in the registers) is set to value 0x3 which
> corresponds to '±10 V single-ended range', regardless of bipolar or
> differential configuration.
> 
> Aside from the scale/ranges, the AD7606C-16 is similar to the AD7606B.
> 
> And the AD7606C-18 variant offers 18-bit precision. The unfortunate effect
> of this 18-bit sample size, is that there is no simple/neat way to get the
> samples into a 32-bit array without having to do a home-brewed bit-buffer.
> The ADC must read all samples (from all 8 channels) in order to get the
> N-th sample (this could be reworked to do up-to-N-th sample for scan-direct).
> There doesn't seem to be any quick-trick to be usable to pad the samples
> up to at least 24 bits.
> Even the optional status-header is 8-bits, which would mean 26-bits of data
> per sample.
> That means that when using a simple SPI controller (which can usually read
> 8 bit multiples) a simple bit-buffer trick is required.
> 
> Datasheet links:
>   https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-16.pdf
>   https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-18.pdf
> 
> Signed-off-by: Alexandru Ardelean <aardelean@...libre.com>

A few minor things. If we can just start with 18 bit word spi controllers only
maybe that's worth doing to make things simpler.

> +static int ad7606c_sw_mode_setup_channels(struct iio_dev *indio_dev,
> +					  ad7606c_chan_setup_cb_t chan_setup_cb)
> +{
> +	unsigned int num_channels = indio_dev->num_channels - 1;
> +	struct ad7606_state *st = iio_priv(indio_dev);
> +	bool chan_configured[AD760X_MAX_CHANNELS];
= {};
and drop the memset.

> +	struct device *dev = st->dev;
> +	int ret;
> +	u32 ch;
> +
> +	/* We need to hook this first */
> +	ret = st->bops->sw_mode_config(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->info = &ad7606c_info_sw_mode;
> +
> +	memset(chan_configured, 0, sizeof(chan_configured));
> +
> +	device_for_each_child_node_scoped(dev, child) {
> +		bool bipolar, differential;
> +
> +		ret = fwnode_property_read_u32(child, "reg", &ch);
> +		if (ret)
> +			continue;
> +
> +		if (ch >= num_channels) {
> +			dev_warn(st->dev,
> +				 "Invalid channel number (ignoring): %d\n", ch);
> +			continue;
> +		}
> +
> +		bipolar = fwnode_property_present(child, "bipolar");
> +		differential = fwnode_property_present(child, "diff-channel");
> +
> +		chan_setup_cb(st, ch, bipolar, differential);
> +		chan_configured[ch] = true;
> +	}
> +
> +	/* Apply default configuration to unconfigured (via DT) channels */
> +	for (ch = 0; ch < num_channels; ch++) {
> +		struct ad7606_chan_scale *cs;
> +		unsigned int *scale_avail_show;
> +		int i;
> +
> +		if (!chan_configured[ch])
> +			chan_setup_cb(st, ch, false, false);
> +
> +		/* AD7606C supports different scales per channel */
> +		cs = &st->chan_scales[ch];
> +
> +		scale_avail_show = devm_kcalloc(st->dev, cs->num_scales * 2,
> +						sizeof(*scale_avail_show),
> +						GFP_KERNEL);

Maybe just make it big enough for worst case and stick it in st always?
How big can it get?



> +		if (!scale_avail_show)
> +			return -ENOMEM;
> +
> +		/* Generate a scale_avail list for showing to userspace */
> +		for (i = 0; i < cs->num_scales; i++) {
> +			scale_avail_show[i * 2] = 0;
> +			scale_avail_show[i * 2 + 1] = cs->scale_avail[i];
> +		}
> +
> +		cs->scale_avail_show = scale_avail_show;
> +	}
> +
> +	return 0;
> +}
>

> diff --git a/drivers/iio/adc/ad7606_spi.c b/drivers/iio/adc/ad7606_spi.c
> index dd0075c97c24..73a7b0007bf8 100644
> --- a/drivers/iio/adc/ad7606_spi.c
> +++ b/drivers/iio/adc/ad7606_spi.c
> @@ -45,6 +45,8 @@

>  
> +static int ad7606_spi_read_block18to32(struct device *dev,
> +				       int count, void *buf)
> +{
> +	struct spi_device *spi = to_spi_device(dev);
> +	u32 i, bit_buffer, buf_size, bit_buf_size;
> +	u32 *data = buf;
> +	u8 *bdata = buf;
> +	int j, ret;
> +
> +	/**
Not kernel doc.  /*
> +	 * With the 18 bit ADC variants (here) is that we can't assume that all
> +	 * SPI controllers will pad 18-bit sequences into 32-bit arrays,
> +	 * so we need to do a bit of buffer magic here.
> +	 * Alternatively, we can have a variant of this function that works
> +	 * for SPI controllers that can pad 18-bit samples into 32-bit arrays.
> +	 */
> +
> +	/* Write 'count' bytes to the right, to not overwrite samples */
> +	bdata += count;
> +
> +	/* Read 24 bits only, as we'll only get samples of 18 bits each */
> +	buf_size = count * 3;
> +	ret = spi_read(spi, bdata, buf_size);
> +	if (ret < 0) {
> +		dev_err(&spi->dev, "SPI read error\n");
> +		return ret;
> +	}
> +
> +	bit_buffer = 0;
> +	bit_buf_size = 0;
> +	for (j = 0, i = 0; i < buf_size; i++) {
> +		u32 sample;
> +
> +		bit_buffer = (bit_buffer << 8) | bdata[i];
> +		bit_buf_size += 8;
> +
> +		if (bit_buf_size < 18)
> +			continue;
> +
> +		bit_buf_size -= 18;
> +		sample = (bit_buffer >> bit_buf_size) & AD7606C_18_SAMPLE_MASK;
> +		data[j++] = sign_extend32(sample, 17);
> +
> +		if (j == count)
> +			break;
> +	}
> +
> +	return 0;
> +}

Powered by blists - more mailing lists