On Thu, Sep 04, 2025 at 03:36:46PM +0300, Matti Vaittinen wrote:

The ROHM BD79112 is an ADC/GPIO with 32 channels. The channel inputs can
be used as ADC or GPIO. Using the GPIOs as IRQ sources isn't supported.

The ADC is 12-bit, supporting input voltages up to 5.7V, and separate I/O
voltage supply. Maximum SPI clock rate is 20 MHz (10 MHz with
daisy-chain configuration) and maximum sampling rate is 1MSPS.

The IC does also support CRC but it is not implemented in the driver.

...

+/*
+ * The data-sheet explains register I/O communication as follows:
+ *
+ * Read, two 16-bit sequences separated by CSB:
+ * MOSI:
+ * SCK:	| 1 | 2 | 3   | 4      | 5 .. 8 | 9 .. 16 |
+ * data:| 0 | 0 |IOSET| RW (1) | ADDR   | 8'b0    |
+ *
+ * MISO:
+ * SCK:	| 1 .. 8 | 9 .. 16 |
+ * data:| 8'b0   | data    |
+ *
+ * Note, CSB is shown to be released between writing the address (MOSI) and
+ * reading the register data (MISO).
+ *
+ * Write, single 16-bit sequence:
+ * MOSI:
+ * SCK:	| 1 | 2 | 3   | 4     | 5 .. 8 |
+ * data:| 0 | 0 |IOSET| RW(0) | ADDR   |
+ *
+ * MISO:
+ * SCK:	| 1 .. 8 |
+ * data:| data   |
+ */

I don't know how to read this comment. In the monospace font the whole block
looks like a mess.

...

+static int _get_gpio_reg(unsigned int offset, unsigned int base)
+{
+	int regoffset = offset / 8;
+
+	if (offset > 31 || offset < 0)

So, < 0 is now unneeded and offset > 31 can be rewritten as

	if (regoffset >= 4)

which is more clear to me (like we have 4 banks and here is the check for
the bank. Maybe you can even call the variable 'bank'.

+		return -EINVAL;
+
+	return base - regoffset;
+}

...

+#define GET_GPIO_BIT(offset) BIT((offset) % 8)

I suggest to make it to be a returned parameter of _get_gpio_reg(). This will
give better code generation on some architectures, see, for example, this
commit: 9b3cd5c7099f regmap: place foo / 8 and foo % 8 closer to each other.

...

+static const struct regmap_access_table bd79112_volatile_regs = {
+	.yes_ranges = &bd71815_volatile_ro_ranges[0],
+	.n_yes_ranges = ARRAY_SIZE(bd71815_volatile_ro_ranges),

+ array_size.h
(and btw we put generic asm/* _after_ generic linux/*, just noticed that).

+};

...

+static int bd79112_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan, int *val,
+			    int *val2, long m)
+{
+	struct bd79112_data *data = iio_priv(indio_dev);
+	int ret;
+
+	switch (m) {
+	case IIO_CHAN_INFO_RAW:
+		ret = regmap_read(data->map, chan->channel, val);
+		if (ret < 0)
+			return ret;
+
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		 *val = data->vref_mv;
+		 *val2 = 12;
+
+		return IIO_VAL_FRACTIONAL_LOG2;
+	default:
+		return -EINVAL;
+	}

+

Unneeded blank line.

+}

...

+static int bd79112_gpio_set_multiple(struct gpio_chip *gc, unsigned long *mask,
+				     unsigned long *bits)
+{
+	struct bd79112_data *data = gpiochip_get_data(gc);
+	unsigned long i, bank_mask;
+
+	for_each_set_clump8(i, bank_mask, mask, /* gc->ngpio */ 32) {

Hmm... Why constant and not gc->ngpio?

+		unsigned long bank_bits;
+		unsigned int reg;
+		int ret;

+		if (bank_mask) {

This is a duplication, the iterator only gives non-zero "clumps".

+			bank_bits = bitmap_get_value8(bits, i);
+			reg = BD79112_REG_GPO_VALUE_A0_A7 - i / 8;
+			ret = regmap_update_bits(data->map, reg, bank_mask,
+						 bank_bits);
+			if (ret)
+				return ret;
+		}
+	}
+
+	return 0;
+}

...

+static int bd79112_get_gpio_pins(const struct iio_chan_spec *cs, int num_channels)
+{
+	int i, gpio_channels;
+
+	/*
+	 * Let's initialize the mux config to say that all 32 channels are
+	 * GPIOs. Then we can just loop through the iio_chan_spec and clear the
+	 * bits for found ADC channels.
+	 */
+	gpio_channels = GENMASK(31, 0);

This is negative number, it might bait one at a surprising time. Hence once
again, why not make them unsigned?

+	for (i = 0; i < num_channels; i++)
+		gpio_channels &= ~BIT(cs[i].channel);
+
+	return gpio_channels;
+}

...

+/* ADC channels as named in the data-sheet */
+static const char * const bd79112_chan_names[] = {
+	"AGIO0A", "AGIO1A", "AGIO2A", "AGIO3A", "AGIO4A",	/* 0 - 4 */
+	"AGIO5A", "AGIO6A", "AGIO7A", "AGIO8A", "AGIO9A",	/* 5 - 9 */
+	"AGIO10A", "AGIO11A", "AGIO12A", "AGIO13A", "AGIO14A",	/* 10 - 14 */
+	"AGIO15A", "AGIO0B", "AGIO1B", "AGIO2B", "AGIO3B",	/* 15 - 19 */
+	"AGIO4B", "AGIO5B", "AGIO6B", "AGIO7B", "AGIO8B",	/* 20 - 24 */
+	"AGIO9B", "AGIO10B", "AGIO11B", "AGIO12B", "AGIO13B",	/* 25 - 29 */
+	"AGIO14B", "AGIO15B",					/* 30 - 31 */

O-o-key, but why not power-of-two per line (esp. taking into account
the whole size)? (Whatever, it's not something I would fight for.)

+};

...

+	data->vref_mv = ret / 1000;

Yeah, mV, (MICRO / MILLI) and other things I leave to other people to discuss.


...

+	ret = devm_iio_adc_device_alloc_chaninfo_se(dev, &bd79112_chan_template,
+						    BD79112_MAX_NUM_CHANNELS - 1,
+						    &cs);
+	if (ret < 0) {

+		/* Register all pins as GPIOs if there are no ADC channels */
+		if (ret == -ENOENT)
+			goto register_gpios;

As I showed this can be checked before other case, but I kinda have an idea why
you are liking to do it this way.

+		return ret;
+	}

...

+register_gpios:
+	gpio_pins = bd79112_get_gpio_pins(iio_dev->channels,
+					  iio_dev->num_channels);
+
+	/* If all channels are reserved for ADC, then we're done. */

I still consider the assignment to be located here is a better place,
but I leave it to maintainers.

+	if (!gpio_pins)
+		return 0;

-- 
With Best Regards,
Andy Shevchenko