Message-ID: <0036d44542f8cf45c91c867f0ddd7b45d1904d6b.camel@gmail.com>
Date: Fri, 28 Jun 2024 10:35:08 +0200
From: Nuno Sá <noname.nuno@...il.com>
To: Esteban Blanc <eblanc@...libre.com>, baylibre-upstreaming@...ups.io, 
 Lars-Peter Clausen
	 <lars@...afoo.de>, Michael Hennerich <Michael.Hennerich@...log.com>, 
 Jonathan Cameron
	 <jic23@...nel.org>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
	 <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Nuno Sa
	 <nuno.sa@...log.com>
Cc: linux-iio@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, David Lechner <dlechner@...libre.com>
Subject: Re: [PATCH RFC 2/5] iio: adc: ad4030: add driver for ad4030-24

Hi Esteban,

Main thing that I think we should clear and think about is how to expose the
common mode voltage. I kind if agree with it being a single channel but I still
have some concerns... See below.

On Thu, 2024-06-27 at 13:59 +0200, Esteban Blanc wrote:
> This adds a new driver for the Analog Devices INC. AD4030-24 ADC.
> 
> The driver implements basic support for the AD4030-24 1 channel
> differential ADC with hardware gain and offset control.
> 
> Signed-off-by: Esteban Blanc <eblanc@...libre.com>
> ---
>  MAINTAINERS              |   1 +
>  drivers/iio/adc/Kconfig  |  13 +
>  drivers/iio/adc/Makefile |   1 +
>  drivers/iio/adc/ad4030.c | 822
> +++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 837 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8ca5b2e09b69..8df171c62d37 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -419,6 +419,7 @@ R:	Esteban Blanc <eblanc@...libre.com>
>  S:	Supported
>  W:	https://ez.analog.com/linux-software-drivers
>  F:	Documentation/devicetree/bindings/iio/adc/adi,ad4630.yaml
> +F:	drivers/iio/adc/ad4030.c
>  
>  AD5110 ANALOG DEVICES DIGITAL POTENTIOMETERS DRIVER
>  M:	Mugilraj Dhavachelvan <dmugil2000@...il.com>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 3d91015af6ea..e71ac1e49acb 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -21,6 +21,19 @@ config AD_SIGMA_DELTA
>  	select IIO_BUFFER
>  	select IIO_TRIGGERED_BUFFER
>  
> +config AD4030
> +	tristate "Analog Device AD4630 ADC Driver"
> +	depends on SPI
> +	depends on GPIOLIB
> +	select REGMAP_SPI
> +	select IIO_BUFFER
> +	help
> +	  Say yes here to build support for Analog Devices AD4030 and AD4630
> high speed
> +	  SPI analog to digital converters (ADC).
> +
> +	  To compile this driver as a module, choose M here: the module will
> be
> +	  called ad4030.
> +
>  config AD4130
>  	tristate "Analog Device AD4130 ADC Driver"
>  	depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 37ac689a0209..7a8945559589 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -6,6 +6,7 @@
>  # When adding new entries keep the list in alphabetical order
>  obj-$(CONFIG_AB8500_GPADC) += ab8500-gpadc.o
>  obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
> +obj-$(CONFIG_AD4030) += ad4030.o
>  obj-$(CONFIG_AD4130) += ad4130.o
>  obj-$(CONFIG_AD7091R) += ad7091r-base.o
>  obj-$(CONFIG_AD7091R5) += ad7091r5.o
> diff --git a/drivers/iio/adc/ad4030.c b/drivers/iio/adc/ad4030.c
> new file mode 100644
> index 000000000000..6d537e531d6f
> --- /dev/null
> +++ b/drivers/iio/adc/ad4030.c
> @@ -0,0 +1,822 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Analog Devices AD4030 and AD4630 ADC family driver.
> + *
> + * Copyright 2024 Analog Devices, Inc.
> + * Copyright 2024 BayLibre, SAS
> + *
> + * based on code from:
> + *	Analog Devices, Inc.
> + *	  Sergiu Cuciurean <sergiu.cuciurean@...log.com>
> + *	  Nuno Sa <nuno.sa@...log.com>
> + *	  Marcelo Schmitt <marcelo.schmitt@...log.com>
> + *	  Liviu Adace <liviu.adace@...log.com>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/units.h>
> +#include <linux/clk.h>
> +#include <linux/spi/spi.h>
> +#include <linux/regmap.h>
> +#include <linux/iio/iio.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +
> +#define AD4030_REG_INTERFACE_CONFIG_A				0x00
> +#define     AD4030_REG_INTERFACE_CONFIG_A_SW_RESET		(BIT(0) |
> BIT(7))
> +#define AD4030_REG_INTERFACE_CONFIG_B				0x01
> +#define AD4030_REG_DEVICE_CONFIG				0x02
> +#define AD4030_REG_CHIP_TYPE					0x03
> +#define AD4030_REG_PRODUCT_ID_L					0x04
> +#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_MASK_CHIP_GRADE		GENMASK(7, 3)
> +#define AD4030_REG_SCRATCH_PAD					0x0A
> +#define AD4030_REG_SPI_REVISION					0x0B
> +#define AD4030_REG_VENDOR_L					0x0C
> +#define AD4030_REG_VENDOR_H					0x0D
> +#define AD4030_REG_STREAM_MODE					0x0E
> +#define AD4030_REG_INTERFACE_CONFIG_C				0x10
> +#define AD4030_REG_INTERFACE_STATUS_A				0x11
> +#define AD4030_REG_EXIT_CFG_MODE				0x14
> +#define     AD4030_REG_EXIT_CFG_MODE_MASK_EXIT_CONFIG_MD	BIT(0)
> +#define AD4030_REG_AVG						0x15
> +#define     AD4030_REG_AVG_MASK_AVG_SYNC			BIT(7)
> +#define    
> AD4030_REG_AVG_MASK_AVG_VAL				GENMASK(4, 0)
> +#define AD4030_REG_OFFSET_X0_0					0x16
> +#define AD4030_REG_OFFSET_X0_1					0x17
> +#define AD4030_REG_OFFSET_X0_2					0x18
> +#define AD4030_REG_OFFSET_X1_0					0x19
> +#define AD4030_REG_OFFSET_X1_1					0x1A
> +#define AD4030_REG_OFFSET_X1_2					0x1B
> +#define     AD4030_REG_OFFSET_BYTES_NB				3
> +#define    
> AD4030_REG_OFFSET_CHAN(ch)				(AD4030_REG_OFFSET_X0_2 +	\
> +								(AD4030_REG_O
> FFSET_BYTES_NB *	\
> +								(ch)))
> +#define AD4030_REG_GAIN_X0_LSB					0x1C
> +#define AD4030_REG_GAIN_X0_MSB					0x1D
> +#define AD4030_REG_GAIN_X1_LSB					0x1E
> +#define AD4030_REG_GAIN_X1_MSB					0x1F
> +#define     AD4030_REG_GAIN_MAX_GAIN				1999970
> +#define     AD4030_REG_GAIN_BYTES_NB				2
> +#define    
> AD4030_REG_GAIN_CHAN(ch)				(AD4030_REG_GAIN_X0_MSB +	\
> +								(AD4030_REG_G
> AIN_BYTES_NB *	\
> +								(ch)))
> +#define AD4030_REG_MODES					0x20
> +#define    
> AD4030_REG_MODES_MASK_OUT_DATA_MODE			GENMASK(2, 0)
> +#define     AD4030_REG_MODES_MASK_LANE_MODE			GENMASK(7, 6)
> +#define AD4030_REG_OSCILATOR					0x21
> +#define AD4030_REG_IO						0x22
> +#define     AD4030_REG_IO_MASK_IO2X				BIT(1)
> +#define AD4030_REG_PAT0						0x23
> +#define AD4030_REG_PAT1						0x24
> +#define AD4030_REG_PAT2						0x25
> +#define AD4030_REG_PAT3						0x26
> +#define AD4030_REG_DIG_DIAG					0x34
> +#define AD4030_REG_DIG_ERR					0x35
> +
> +/* Sequence starting with "1 0 1" to enable reg access */
> +#define AD4030_REG_ACCESS		0xa0
> +
> +#define AD4030_MAX_DIFF_CHANNEL_NB	2
> +#define AD4030_MAX_COMMON_CHANNEL_NB	AD4030_MAX_DIFF_CHANNEL_NB
> +#define AD4030_MAX_TIMESTAMP_CHANNEL_NB	1
> +#define AD4030_ALL_CHANNELS_NB		(AD4030_MAX_DIFF_CHANNEL_NB +	\
> +					AD4030_MAX_COMMON_CHANNEL_NB +	\
> +					AD4030_MAX_TIMESTAMP_CHANNEL_NB)
> +#define AD4030_VREF_MIN_UV		(4096 * MILLI)
> +#define AD4030_VREF_MAX_UV		(5000 * MILLI)
> +#define AD4030_VIO_THRESHOLD_UV		(1400 * MILLI)
> +
> +#define AD4030_SPI_MAX_XFER_LEN		8
> +#define AD4030_SPI_MAX_REG_XFER_SPEED	(80 * MEGA)
> +#define AD4030_TCNVH_NS			10
> +#define AD4030_TCNVL_NS			20
> +#define AD4030_TCONV_NS			(300 - (AD4030_TCNVH_NS
> +	\
> +					AD4030_TCNVL_NS))
> +#define AD4030_TRESET_PW_NS		50
> +#define AD4030_TRESET_COM_DELAY_MS	750
> +
> +enum ad4030_out_mode {
> +	AD4030_OUT_DATA_MD_16_DIFF = 0x00,
> +	AD4030_OUT_DATA_MD_24_DIFF = 0x00,
> +	AD4030_OUT_DATA_MD_16_DIFF_8_COM = 0x01,
> +	AD4030_OUT_DATA_MD_24_DIFF_8_COM = 0x02,
> +	AD4030_OUT_DATA_MD_30_AVERAGED_DIFF = 0x03,
> +	AD4030_OUT_DATA_MD_32_PATTERN = 0x04
> +};
> +
> +struct ad4030_chip_info {
> +	const char *name;
> +	const unsigned long *available_masks;
> +	unsigned int available_masks_len;
> +	const struct iio_chan_spec channels[AD4030_ALL_CHANNELS_NB];
> +	u8 grade;
> +	u8 precision_bits;
> +	int num_channels;
> +};
> +
> +struct ad4030_state {
> +	struct spi_device *spi;
> +	struct regmap *regmap;
> +	const struct ad4030_chip_info *chip;
> +	struct gpio_desc *cnv_gpio;
> +	int vref_uv;
> +	int vio_uv;
> +	int min_offset;
> +	int max_offset;
> +	int offset_avail[3];
> +	u32 conversion_speed_hz;
> +	enum ad4030_out_mode mode;
> +
> +	/*
> +	 * DMA (thus cache coherency maintenance) requires the transfer
> buffers
> +	 * to live in their own cache lines.
> +	 */
> +	u8 tx_data[AD4030_SPI_MAX_XFER_LEN] __aligned(IIO_DMA_MINALIGN);
> +	struct {
> +		union {
> +			/*
> +			 * Make the buffer large enough for MAX_NUM_CHANNELS
> 32-bit samples and
> +			 * one 64-bit aligned 64-bit timestamp.
> +			 */
> +			u8 raw[ALIGN(AD4030_MAX_DIFF_CHANNEL_NB * sizeof(u32)
> +
> +			       AD4030_MAX_COMMON_CHANNEL_NB * sizeof(u8),
> +			       sizeof(u64)) + sizeof(u64)];

This is not very readable. I would make this simpler.

> +			struct {
> +				s32 val[AD4030_MAX_DIFF_CHANNEL_NB];
> +				u8 common[AD4030_MAX_COMMON_CHANNEL_NB];

Not sure common makes sense as it comes aggregated with the sample. Maybe this
could as simple as:

struct {
	s32 val;
	u64 timestamp __aligned(8);
} rx_data ...

> +			};
> +		};
> +	} rx_data __aligned(IIO_DMA_MINALIGN);
> +};
> +
> +#define AD4030_CHAN_CMO(_idx)  {					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
> +	.type = IIO_VOLTAGE,						\
> +	.indexed = 1,							\
> +	.channel = _idx,						\
> +	.scan_index = _idx,						\
> +	.scan_type = {							\
> +		.sign = 'u',						\
> +		.storagebits = 8,					\
> +		.realbits = 8,						\
> +		.endianness = IIO_BE,					\
> +	},								\
> +}
> +

So, from the datasheet, figure 39 we have something like a multiplexer where we
can have:

- averaged data;
- normal differential;
- test pattern (btw, useful to have it in debugfs - but can come later);
- 8 common mode bits;

While the average, normal and test pattern are really mutual exclusive, the
common mode voltage is different in the way that it's appended to differential
sample. Making it kind of an aggregated thingy. Thus I guess it can make sense
for us to see them as different channels from a SW perspective (even more since
gain and offset only apply to the differential data). But there are a couple of
things I don't like (have concerns):

* 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).

Other thing that came to mind is if we could somehow use differential = true
here. Having something like:

in_voltage1_in_voltage0_raw - diff data
...
And the only thing for CM would be:

in_voltage1_raw
in_voltage1_scale

(not sure if the above is doable with ext_info - maybe only with device_attrs)

The downside of the above is that we don't have a way to separate the scan
elements. Meaning that we don't have a way to specify the scan_type for both the
common mode and differential voltage. That said, I wonder if it is that useful
to buffer the common mode stuff? Alternatively, we could just have the scan_type
for the diff data and apps really wanting the CM voltage could still access the
raw data. Not pretty, I know...

However, even if we go with the two separate channels there's one thing that
concerns me. Right now we have diff data with 32 for storage bits and CM data
with 8 storage bits which means the sample will be 40 bits and in reality we
just have 32. Sure, now we have SW buffering so we can make it work but the
ultimate goal is to support HW buffering where we won't be able to touch the
sample and thus we can't lie about the sample size. Could you run any test with
this approach on a HW buffer setup? 

I did not gave too much thought on it but I'm not sure there's a sane way to
have multiple scan_types associated with the same channel. 

...

> +#define AD4030_CHAN_IN(_idx, _storage, _real, _shift)
> {			\
> 
> +
> +static int ad4030_get_chan_gain(struct iio_dev *indio_dev, int ch, int *val,
> +				int *val2)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +	u16 gain;
> +	int ret;
> +
> +	ret = regmap_bulk_read(st->regmap, AD4030_REG_GAIN_CHAN(ch), &gain,
> +			       AD4030_REG_GAIN_BYTES_NB);

Even though it's just 2 bytes, Jonathan typically has a strict policy regarding
this not being DMA safe.

> +	if (ret)
> +		return ret;
> +
> +	gain = be16_to_cpu(gain);
> +
> +	/* From datasheet: multiplied output = input × gain word/0x8000 */
> +	*val = gain / 0x8000;
> +	*val2 = mul_u64_u32_div(gain % 0x8000, MICRO, 0x8000);
> +
> +	return 0;
> +}
> +
> +/*
> + * @brief Returns the offset where 1 LSB = (VREF/2^precision_bits - 1)/gain
> + */
> +static int ad4030_get_chan_offset(struct iio_dev *indio_dev, int ch, int
> *val)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +	__be32 offset;

ditto...

> +	int ret;
> +
> +	ret = regmap_bulk_read(st->regmap, AD4030_REG_OFFSET_CHAN(ch),
> &offset,
> +			       AD4030_REG_OFFSET_BYTES_NB);
> +	if (ret)
> +		return ret;
> +
> +	if (st->chip->precision_bits == 16)
> +		offset <<= 16;
> +	else
> +		offset <<= 8;
> +
> +	*val = be32_to_cpu(offset);
> +
> +	return 0;
> +}
> +
> +static int ad4030_set_chan_gain(struct iio_dev *indio_dev, int ch, int
> gain_int,
> +				int gain_frac)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +	__be16 val;
> +	u64 gain;
> +
> +	gain = mul_u32_u32(gain_int, MICRO) + gain_frac;
> +
> +	if (gain > AD4030_REG_GAIN_MAX_GAIN)
> +		return -EINVAL;
> +
> +	val = cpu_to_be16(DIV_ROUND_CLOSEST_ULL(gain * 0x8000, MICRO));
> +
> +	return regmap_bulk_write(st->regmap, AD4030_REG_GAIN_CHAN(ch), &val,
> +			  AD4030_REG_GAIN_BYTES_NB);

ditto

> +}
> +
> +static int ad4030_set_chan_offset(struct iio_dev *indio_dev, int ch, int
> offset)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +	__be32 val;
> +
> +	if (offset < st->min_offset || offset > st->max_offset)
> +		return -EINVAL;
> +
> +	val = cpu_to_be32(offset);
> +	if (st->chip->precision_bits == 16)
> +		val >>= 16;
> +	else
> +		val >>= 8;
> +
> +	return regmap_bulk_write(st->regmap, AD4030_REG_OFFSET_CHAN(ch),
> &val, 3);

...

> +}
> +
> +static bool ad4030_is_common_byte_asked(struct ad4030_state *st,
> +					unsigned int mask)
> +{
> +	return mask & BIT(st->chip->num_channels);
> +}
> +
> +static int ad4030_set_mode(struct iio_dev *indio_dev, unsigned long mask)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +
> +	if (ad4030_is_common_byte_asked(st, mask))
> +		st->mode = st->chip->precision_bits == 24 ?
> +			AD4030_OUT_DATA_MD_24_DIFF_8_COM :
> +			AD4030_OUT_DATA_MD_16_DIFF_8_COM;

At this point you still don't really have AD4030_OUT_DATA_MD_16_DIFF_8_COM
support so I would keep it simple until you need to support it.

> +	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);
> +}
> +
> +static int ad4030_conversion(struct ad4030_state *st, const struct
> iio_chan_spec *chan)
> +{
> +	unsigned int bytes_to_read = (BITS_TO_BYTES(chan->scan_type.realbits)
> +
> +			     ((st->mode == AD4030_OUT_DATA_MD_24_DIFF_8_COM
> ||
> +			     st->mode == AD4030_OUT_DATA_MD_16_DIFF_8_COM) ?
> 1 : 0)) *
> +			     st->chip->num_channels;
> +	struct spi_transfer xfer = {
> +		.rx_buf = st->rx_data.raw,
> +		.len = bytes_to_read,
> +	};
> +	unsigned char byte_index;
> +	unsigned int i;
> +	int ret;
> +
> +	gpiod_set_value_cansleep(st->cnv_gpio, 1);
> +	ndelay(AD4030_TCNVH_NS);
> +	gpiod_set_value_cansleep(st->cnv_gpio, 0);
> +	ndelay(AD4030_TCNVL_NS + AD4030_TCONV_NS);
> +
> +	ret = spi_sync_transfer(st->spi, &xfer, 1);
> +	if (ret || (st->mode != AD4030_OUT_DATA_MD_16_DIFF_8_COM &&
> +		    st->mode != AD4030_OUT_DATA_MD_24_DIFF_8_COM))

You should guarantee that st->mode is not invalid...

> +		return ret;
> +
> +	byte_index = st->chip->precision_bits == 16 ? 3 : 4;
> +	for (i = 0; i < st->chip->num_channels; i++)

So even for a single channel conversion we are going through all?

> +		st->rx_data.common[i] = ((u8 *)&st-
> >rx_data.val[i])[byte_index];
> +
> +	return 0;
> +}
> +
> +static int ad4030_single_conversion(struct iio_dev *indio_dev,
> +				    const struct iio_chan_spec *chan, int
> *val)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = ad4030_set_mode(indio_dev, BIT(chan->channel));
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4030_exit_config_mode(st);
> +	if (ret)
> +		goto out_error;
> +
> +	ret = ad4030_conversion(st, chan);
> +	if (ret)
> +		goto out_error;
> +
> +	if (chan->channel < st->chip->num_channels)
> +		*val = st->rx_data.val[chan->channel];
> +	else
> +		*val = st->rx_data.common[chan->channel - st->chip-
> >num_channels];
> +
> +out_error:
> +	ad4030_enter_config_mode(st);
> +
> +	return IIO_VAL_INT;
> +}
> +
> +static irqreturn_t ad4030_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = ad4030_conversion(st, indio_dev->channels);
> +	if (ret)
> +		goto out;
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, st->rx_data.raw,
> +					   pf->timestamp);
> +
> +out:
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static const int *ad4030_get_offset_avail(struct ad4030_state *st)
> +{
> +	return st->offset_avail;
> +}
> +
> +static const int ad4030_gain_avail[3][2] = {
> +	{0, 0},
> +	{0, 30},
> +	{1, 999969},
> +};
> +
> +static int ad4030_read_avail(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *channel,
> +			     const int **vals, int *type,
> +			     int *length, long mask)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		*vals = ad4030_get_offset_avail(st);
> +		*type = IIO_VAL_INT;
> +		return IIO_AVAIL_RANGE;
> +
> +	case IIO_CHAN_INFO_CALIBSCALE:
> +		*vals = (void *)ad4030_gain_avail;
> +		*type = IIO_VAL_INT_PLUS_MICRO;
> +		return IIO_AVAIL_RANGE;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ad4030_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan, int *val,
> +			   int *val2, long info)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {

Oh this is not neat :(. I guess there's still no work to make conditional guards
to look more as the typical pattern...


> +		switch (info) {
> +		case IIO_CHAN_INFO_RAW:
> +			return ad4030_single_conversion(indio_dev, chan,
> val);
> +
> +		case IIO_CHAN_INFO_SCALE:
> +			*val = (st->vref_uv * 2) / MILLI;
> +			*val2 = st->chip->precision_bits;
> +			return IIO_VAL_FRACTIONAL_LOG2;

I don't think this applies to CM?

...

> 
> +static int ad4030_detect_chip_info(const struct ad4030_state *st)
> +{
> +	unsigned int grade;
> +	int ret;
> +
> +	ret = regmap_read(st->regmap, AD4030_REG_CHIP_GRADE, &grade);
> +	if (ret)
> +		return ret;
> +
> +	grade = FIELD_GET(AD4030_REG_CHIP_GRADE_MASK_CHIP_GRADE, grade);
> +	if (grade != st->chip->grade)
> +		return dev_err_probe(&st->spi->dev, -EINVAL,
> +				"Unknown grade(%u) for %s\n", grade,
> +				st->chip->name);

I think in here we still want to proceed and just print a warning...

- Nuno Sá

Powered by blists - more mailing lists