On Wed, 29 Jan 2025 16:29:03 +0200
Alisa-Dariana Roman [off-list ref] 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

+
+#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!

+}
+