Re: [PATCH v3 2/3] iio: adc: ad7191: add AD7191

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Wed, 29 Jan 2025 16:29:03 +0200
Alisa-Dariana Roman <alisadariana@gmail.com> wrote:

> AD7191 is a pin-programmable, ultra-low noise 24-bit sigma-delta ADC
> designed for precision bridge sensor measurements. It features two
> differential analog input channels, selectable output rates,
> programmable gain, internal temperature sensor and simultaneous
> 50Hz/60Hz rejection.
> 
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
Hi Alisa-Dariana

Seeing as a v4 is needed for the available thing you mention
in the cover letter, a few minor things inline I might otherwise
have just tidied up whilst applying.

Thanks,

Jonathan


> diff --git a/drivers/iio/adc/ad7191.c b/drivers/iio/adc/ad7191.c
> new file mode 100644
> index 000000000000..b6c1d5c25783
> --- /dev/null
> +++ b/drivers/iio/adc/ad7191.c

> +
> +#include <linux/iio/adc/ad_sigma_delta.h>
> +#include <linux/iio/iio.h>
> +
> +#define ad_sigma_delta_to_ad7191(sigmad)	container_of((sigmad), struct ad7191_state, sd)
#define ad_sigma_delta_to_ad7191(sigmad)	\
	container_of((sigmad), struct ad7191_state, sd)

Given it is very long otherwise.

> +
> +#define AD7191_TEMP_CODES_PER_DEGREE	2815
> +
> +#define AD7191_EXT_CLK_ENABLE		0
> +#define AD7191_INT_CLK_ENABLE		1
> +
> +#define AD7191_CHAN_MASK		BIT(0)
> +#define AD7191_TEMP_MASK		BIT(1)
> +
> +enum ad7191_channel {
> +	AD7191_CH_AIN1_AIN2,
> +	AD7191_CH_AIN3_AIN4,
> +	AD7191_CH_TEMP
Add a trailing comma. Maybe there will be more channels on some
compatible part that we add after this.

> +};
> +
> +/*
> + * NOTE:
> + * The AD7191 features a dual-use data out ready DOUT/RDY output.
> + * In order to avoid contentions on the SPI bus, it's therefore necessary
> + * to use SPI bus locking.
> + *
> + * The DOUT/RDY output must also be wired to an interrupt-capable GPIO.
> + *
> + * The SPI controller's chip select must be connected to the PDOWN pin
> + * of the ADC. When CS (PDOWN) is high, it powers down the device and
> + * resets the internal circuitry.
> + */
> +
> +struct ad7191_state {
> +	struct ad_sigma_delta		sd;
> +	struct mutex			lock; // to protect sensor state
Only use old style /* */ in IIO code with exception of the SPDX
corner case.  This is just a consistency thing hanging over
from when that was only thing used in the kernel.


> +
> +	struct gpio_descs		*odr_gpios;
> +	struct gpio_descs		*pga_gpios;
> +	struct gpio_desc		*temp_gpio;
> +	struct gpio_desc		*chan_gpio;
> +
> +	u16				int_vref_mv;
> +	u32				pga_index;
> +	u32				scale_avail[4][2];
> +	u32				odr_index;
> +	u32				samp_freq_avail[4];
> +
> +	struct clk			*mclk;
> +};

> +
> +static int ad7191_config_setup(struct iio_dev *indio_dev)
> +{
> +	struct ad7191_state *st = iio_priv(indio_dev);
> +	struct device *dev = &st->sd.spi->dev;
> +	/* Sampling frequencies in Hz, see Table 5 */
> +	const int samp_freq[4] = {120, 60, 50, 10};

Space after { and before } in all such places.
This is just my local IIO preference as nice to pick a standard.
I often just tweak this whilst applying but seeing as you
are going to be doing a v4 anyway please tidy it up!

> +	/* Gain options, see Table 7 */
> +	const int gain[4] = {1, 8, 64, 128};
> +	int odr_value, pga_value, i, ret;
> +	u64 scale_uv;
...


> +
> +static int ad7191_setup(struct iio_dev *indio_dev, struct device *dev)
> +{
> +	struct ad7191_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = ad7191_init_regulators(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad7191_config_setup(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad7191_clock_setup(st);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
	return ad7191_clock_setup(st);
and save a few lines of code.

> +}
> +
> +static int ad7191_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan, int *val,
> +			   int *val2, long m)
> +{
> +	struct ad7191_state *st = iio_priv(indio_dev);
> +
> +	switch (m) {
> +	case IIO_CHAN_INFO_RAW:
> +		return ad_sigma_delta_single_conversion(indio_dev, chan, val);
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->type) {
> +		case IIO_VOLTAGE: {
> +			guard(mutex)(&st->lock);
> +			*val = st->scale_avail[st->pga_index][0];
> +			*val2 = st->scale_avail[st->pga_index][1];
> +			return IIO_VAL_INT_PLUS_NANO;
> +		}
> +		case IIO_TEMP:
> +			*val = 0;
> +			*val2 = NANO / AD7191_TEMP_CODES_PER_DEGREE;
> +			return IIO_VAL_INT_PLUS_NANO;
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_OFFSET:
> +		*val = -(1 << (chan->scan_type.realbits - 1));
> +		switch (chan->type) {
> +		case IIO_VOLTAGE:
> +			return IIO_VAL_INT;
> +		case IIO_TEMP:
> +			*val -= 273 * AD7191_TEMP_CODES_PER_DEGREE;
> +			return IIO_VAL_INT;
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*val = st->samp_freq_avail[st->odr_index];
> +		return IIO_VAL_INT;
> +	}
> +

Can't get here so drop this.  I guess the bot hasn't tested this one yet
or we should have seen unreachable warnings.

> +	return -EINVAL;
> +}
> +
> +static int ad7191_set_gain(struct ad7191_state *st, int gain_index)
> +{
> +	unsigned long value = gain_index;
> +
> +	if (!st->pga_gpios)
> +		return -EPERM;
> +
> +	st->pga_index = gain_index;
> +
> +	return gpiod_set_array_value_cansleep(st->pga_gpios->ndescs,
> +					      st->pga_gpios->desc,
> +					      st->pga_gpios->info, &value);
> +
> +	return 0;

double return.  Drop this bonus one!

> +}
> +