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.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

---
Revision history:
v1 => v2 (mainly based on reviews by Andy and David, thanks!)
- Fix Kconfig dependency to REGMAP_SPI instead of REGMAP_I2C
- Add a few header includes
- Drop unnecessary alignments
- plenty of styling
- use for_each_set_clump8 instead of open-coding it
- change order of direction setting writes to avoid receiving 'event'
  when direction is changed from input to output.
- fix data-sheet names and assigning of them to iio_dev
---
 drivers/iio/adc/Kconfig        |  10 +
 drivers/iio/adc/Makefile       |   1 +
 drivers/iio/adc/rohm-bd79112.c | 551 +++++++++++++++++++++++++++++++++
 3 files changed, 562 insertions(+)
 create mode 100644 drivers/iio/adc/rohm-bd79112.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index e3d3826c3357..64ce1eda78c4 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1309,6 +1309,16 @@ config RN5T618_ADC
 	  This driver can also be built as a module. If so, the module
 	  will be called rn5t618-adc.
 
+config ROHM_BD79112
+	tristate "Rohm BD79112 ADC driver"
+	depends on I2C && GPIOLIB
+	select REGMAP_SPI
+	select IIO_ADC_HELPER
+	help
+	  Say yes here to build support for the ROHM BD79112 ADC. The
+	  ROHM BD79112 is a 12-bit, 32-channel, SAR ADC, which analog
+	  inputs can also be used for GPIO.
+
 config ROHM_BD79124
 	tristate "Rohm BD79124 ADC driver"
 	depends on I2C
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 89d72bf9ce70..34b40c34cf71 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -117,6 +117,7 @@ obj-$(CONFIG_QCOM_VADC_COMMON) += qcom-vadc-common.o
 obj-$(CONFIG_RCAR_GYRO_ADC) += rcar-gyroadc.o
 obj-$(CONFIG_RICHTEK_RTQ6056) += rtq6056.o
 obj-$(CONFIG_RN5T618_ADC) += rn5t618-adc.o
+obj-$(CONFIG_ROHM_BD79112) += rohm-bd79112.o
 obj-$(CONFIG_ROHM_BD79124) += rohm-bd79124.o
 obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
 obj-$(CONFIG_RZG2L_ADC) += rzg2l_adc.o
diff --git a/drivers/iio/adc/rohm-bd79112.c b/drivers/iio/adc/rohm-bd79112.c
new file mode 100644
index 000000000000..8107d6b79fa4
--- /dev/null
+++ b/drivers/iio/adc/rohm-bd79112.c
@@ -0,0 +1,551 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * ROHM ADC driver for BD79112 signal monitoring hub.
+ * Copyright (C) 2025, ROHM Semiconductor.
+ *
+ * SPI communication derived from ad7923.c and ti-ads7950.c
+ */
+
+#include <asm/byteorder.h>
+#include <linux/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/bits.h>
+#include <linux/dev_printk.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/gpio/driver.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+#include <linux/types.h>
+
+#include <linux/iio/adc-helpers.h>
+#include <linux/iio/iio.h>
+
+#define BD79112_MAX_NUM_CHANNELS 32
+
+struct bd79112_data {
+	struct spi_device *spi;
+	struct regmap *map;
+	struct device *dev;
+	struct gpio_chip gc;
+	unsigned long gpio_valid_mask;
+	unsigned int vref_mv;
+	struct spi_transfer read_xfer[2];
+	struct spi_transfer write_xfer;
+	struct spi_message read_msg;
+	struct spi_message write_msg;
+	/* 16-bit TX, valid data in high byte */
+	u8 read_tx[2] __aligned(IIO_DMA_MINALIGN);
+	/* 8-bit address followed by 8-bit data */
+	u8 reg_write_tx[2];
+	/* 12-bit of ADC data or 8 bit of reg data */
+	__be16 read_rx;
+};
+
+/*
+ * The ADC data is read issuing SPI-command matching the channel number.
+ * We treat this as a register address.
+ */
+#define BD79112_REG_AGIO0A		0x00
+#define BD79112_REG_AGIO15B		0x1f
+
+/*
+ * ADC STATUS_FLAG appended to ADC data will be set, if the ADC result is being
+ * read for a channel, which input pin is muxed to be a GPIO.
+ */
+#define BD79112_ADC_STATUS_FLAG BIT(14)
+
+/*
+ * The BD79112 requires "R/W bit" to be set for SPI register (not ADC data)
+ * reads and an "IO bit" to be set for read/write operations (which aren't
+ * reading the ADC data).
+ */
+#define BD79112_BIT_RW			BIT(4)
+#define BD79112_BIT_IO			BIT(5)
+
+/*
+ * 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   |
+ */
+
+#define BD79112_REG_GPI_VALUE_B8_15	(BD79112_BIT_IO | 0x0)
+#define BD79112_REG_GPI_VALUE_B0_B7	(BD79112_BIT_IO | 0x1)
+#define BD79112_REG_GPI_VALUE_A8_15	(BD79112_BIT_IO | 0x2)
+#define BD79112_REG_GPI_VALUE_A0_A7	(BD79112_BIT_IO | 0x3)
+
+#define BD79112_REG_GPI_EN_B7_B15	(BD79112_BIT_IO | 0x4)
+#define BD79112_REG_GPI_EN_B0_B7	(BD79112_BIT_IO | 0x5)
+#define BD79112_REG_GPI_EN_A8_A15	(BD79112_BIT_IO | 0x6)
+#define BD79112_REG_GPI_EN_A0_A7	(BD79112_BIT_IO | 0x7)
+
+#define BD79112_REG_GPO_EN_B7_B15	(BD79112_BIT_IO | 0x8)
+#define BD79112_REG_GPO_EN_B0_B7	(BD79112_BIT_IO | 0x9)
+#define BD79112_REG_GPO_EN_A8_A15	(BD79112_BIT_IO | 0xa)
+#define BD79112_REG_GPO_EN_A0_A7	(BD79112_BIT_IO | 0xb)
+
+#define BD79112_NUM_GPIO_EN_REGS	8
+#define BD79112_FIRST_GPIO_EN_REG	BD79112_REG_GPI_EN_B7_B15
+
+#define BD79112_REG_GPO_VALUE_B8_15	(BD79112_BIT_IO | 0xc)
+#define BD79112_REG_GPO_VALUE_B0_B7	(BD79112_BIT_IO | 0xd)
+#define BD79112_REG_GPO_VALUE_A8_15	(BD79112_BIT_IO | 0xe)
+#define BD79112_REG_GPO_VALUE_A0_A7	(BD79112_BIT_IO | 0xf)
+
+#define BD79112_REG_MAX BD79112_REG_GPO_VALUE_A0_A7
+
+static int _get_gpio_reg(unsigned int offset, unsigned int base)
+{
+	int regoffset = offset / 8;
+
+	if (offset > 31 || offset < 0)
+		return -EINVAL;
+
+	return base - regoffset;
+}
+
+#define GET_GPIO_BIT(offset) BIT((offset) % 8)
+#define GET_GPO_EN_REG(offset)  _get_gpio_reg((offset), BD79112_REG_GPO_EN_A0_A7)
+#define GET_GPI_EN_REG(offset)  _get_gpio_reg((offset), BD79112_REG_GPI_EN_A0_A7)
+#define GET_GPO_VAL_REG(offset)  _get_gpio_reg((offset), BD79112_REG_GPO_VALUE_A0_A7)
+#define GET_GPI_VAL_REG(offset)  _get_gpio_reg((offset), BD79112_REG_GPI_VALUE_A0_A7)
+
+static const struct regmap_range bd71815_volatile_ro_ranges[] = {
+	{
+		/* Read ADC data */
+		.range_min = BD79112_REG_AGIO0A,
+		.range_max = BD79112_REG_AGIO15B,
+	}, {
+		/* GPI state */
+		.range_min = BD79112_REG_GPI_VALUE_B8_15,
+		.range_max = BD79112_REG_GPI_VALUE_A0_A7,
+	},
+};
+
+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),
+};
+
+static const struct regmap_access_table bd79112_ro_regs = {
+	.no_ranges = &bd71815_volatile_ro_ranges[0],
+	.n_no_ranges = ARRAY_SIZE(bd71815_volatile_ro_ranges),
+};
+
+static int bd79112_reg_read(void *context, unsigned int reg, unsigned int *val)
+{
+	struct bd79112_data *data = context;
+	int ret;
+
+	if (reg & BD79112_BIT_IO)
+		reg |= BD79112_BIT_RW;
+
+	data->read_tx[0] = reg;
+
+	ret = spi_sync(data->spi, &data->read_msg);
+	if (!ret)
+		*val = be16_to_cpu(data->read_rx);
+
+	if (reg & BD79112_BIT_IO && *val & BD79112_ADC_STATUS_FLAG)
+		dev_err(data->dev, "ADC pin configured as GPIO\n");
+
+	return ret;
+}
+
+static int bd79112_reg_write(void *context, unsigned int reg, unsigned int val)
+{
+	struct bd79112_data *data = context;
+
+	data->reg_write_tx[0] = reg;
+	data->reg_write_tx[1] = val;
+
+	return spi_sync(data->spi, &data->write_msg);
+}
+
+static const struct regmap_config bd79112_regmap = {
+	.reg_read = bd79112_reg_read,
+	.reg_write = bd79112_reg_write,
+	.volatile_table = &bd79112_volatile_regs,
+	.wr_table = &bd79112_ro_regs,
+	.cache_type = REGCACHE_MAPLE,
+	.max_register = BD79112_REG_MAX,
+};
+
+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;
+	}
+
+}
+
+static const struct iio_info bd79112_info = {
+	.read_raw = bd79112_read_raw,
+};
+
+static const struct iio_chan_spec bd79112_chan_template = {
+	.type = IIO_VOLTAGE,
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
+	.indexed = 1,
+};
+
+static int bd79112_gpio_init_valid_mask(struct gpio_chip *gc,
+					unsigned long *valid_mask,
+					unsigned int ngpios)
+{
+	struct bd79112_data *data = gpiochip_get_data(gc);
+
+	*valid_mask = data->gpio_valid_mask;
+
+	return 0;
+}
+
+static int bd79112_gpio_dir_get(struct gpio_chip *gc, unsigned int offset)
+{
+	struct bd79112_data *data = gpiochip_get_data(gc);
+	unsigned int reg, bit, val;
+	int ret;
+
+	bit = GET_GPIO_BIT(offset);
+	reg = GET_GPO_EN_REG(offset);
+
+	ret = regmap_read(data->map, reg, &val);
+	if (ret)
+		return ret;
+
+	if (bit & val)
+		return GPIO_LINE_DIRECTION_OUT;
+
+	reg = GET_GPI_EN_REG(offset);
+	ret = regmap_read(data->map, reg, &val);
+	if (ret)
+		return ret;
+
+	if (bit & val)
+		return GPIO_LINE_DIRECTION_IN;
+
+	/*
+	 * Ouch. Seems the pin is ADC input - shouldn't happen as changing mux
+	 * at runtime is not supported and non GPIO pins should be invalidated
+	 * by the valid_mask at probe. Maybe someone wrote register bypassing
+	 * the driver?
+	 */
+	dev_err(data->dev, "Pin not a GPIO\n");
+
+	return -EINVAL;
+}
+
+static int bd79112_gpio_get(struct gpio_chip *gc, unsigned int offset)
+{
+	struct bd79112_data *data = gpiochip_get_data(gc);
+	unsigned int reg, bit, val;
+	int ret;
+
+	bit = GET_GPIO_BIT(offset);
+	reg = GET_GPI_VAL_REG(offset);
+
+	ret = regmap_read(data->map, reg, &val);
+	if (ret)
+		return ret;
+
+	return !!(val & bit);
+}
+
+static int bd79112_gpio_set(struct gpio_chip *gc, unsigned int offset,
+			    int value)
+{
+	struct bd79112_data *data = gpiochip_get_data(gc);
+	unsigned int reg, bit;
+
+	bit = GET_GPIO_BIT(offset);
+	reg = GET_GPO_VAL_REG(offset);
+
+	return regmap_assign_bits(data->map, reg, bit, value);
+}
+
+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) {
+		unsigned long bank_bits;
+		unsigned int reg;
+		int ret;
+
+		if (bank_mask) {
+			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_gpio_dir_set(struct bd79112_data *data, unsigned int offset,
+				int dir)
+{
+	unsigned int gpi_reg, gpo_reg, bit;
+	int ret;
+
+	bit = GET_GPIO_BIT(offset);
+	gpi_reg = GET_GPI_EN_REG(offset);
+	gpo_reg =  GET_GPO_EN_REG(offset);
+
+	if (dir == GPIO_LINE_DIRECTION_OUT) {
+		ret = regmap_clear_bits(data->map, gpi_reg, bit);
+		if (ret)
+			return ret;
+
+		return regmap_set_bits(data->map, gpo_reg, bit);
+	}
+
+	ret = regmap_set_bits(data->map, gpi_reg, bit);
+	if (ret)
+		return ret;
+
+	return regmap_clear_bits(data->map, gpo_reg, bit);
+}
+
+static int bd79112_gpio_input(struct gpio_chip *gc, unsigned int offset)
+{
+	struct bd79112_data *data = gpiochip_get_data(gc);
+
+	return bd79112_gpio_dir_set(data, offset, GPIO_LINE_DIRECTION_IN);
+}
+
+static int bd79112_gpio_output(struct gpio_chip *gc, unsigned int offset,
+			       int value)
+{
+	struct bd79112_data *data = gpiochip_get_data(gc);
+	int ret;
+
+	ret = bd79112_gpio_set(gc, offset, value);
+	if (ret)
+		return ret;
+
+	return bd79112_gpio_dir_set(data, offset, GPIO_LINE_DIRECTION_OUT);
+}
+
+static const struct gpio_chip bd79112_gpio_chip = {
+	.label			= "bd79112-gpio",
+	.get_direction		= bd79112_gpio_dir_get,
+	.direction_input	= bd79112_gpio_input,
+	.direction_output	= bd79112_gpio_output,
+	.get			= bd79112_gpio_get,
+	.set			= bd79112_gpio_set,
+	.set_multiple		= bd79112_gpio_set_multiple,
+	.init_valid_mask	= bd79112_gpio_init_valid_mask,
+	.can_sleep		= true,
+	.ngpio			= 32,
+	.base			= -1,
+};
+
+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);
+	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 */
+};
+
+static int bd79112_probe(struct spi_device *spi)
+{
+	struct bd79112_data *data;
+	struct iio_dev *iio_dev;
+	struct iio_chan_spec *cs;
+	struct device *dev = &spi->dev;
+	unsigned long gpio_pins, pin;
+	unsigned int i;
+	int ret;
+
+	iio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!iio_dev)
+		return -ENOMEM;
+
+	data = iio_priv(iio_dev);
+	data->spi = spi;
+	data->dev = dev;
+	data->map = devm_regmap_init(&spi->dev, NULL, data, &bd79112_regmap);
+	if (IS_ERR(data->map))
+		return dev_err_probe(dev, PTR_ERR(data->map),
+				     "Failed to initialize Regmap\n");
+
+	ret = devm_regulator_get_enable_read_voltage(dev, "vdd");
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Failed to get the Vdd\n");
+
+	data->vref_mv = ret / 1000;
+
+	ret = devm_regulator_get_enable(dev, "iovdd");
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Failed to enable I/O voltage\n");
+
+	data->read_xfer[0].tx_buf = &data->read_tx[0];
+	data->read_xfer[0].len = sizeof(data->read_tx);
+	data->read_xfer[0].cs_change = 1;
+	data->read_xfer[1].rx_buf = &data->read_rx;
+	data->read_xfer[1].len = sizeof(data->read_rx);
+	spi_message_init_with_transfers(&data->read_msg, data->read_xfer, 2);
+
+	data->write_xfer.tx_buf = &data->reg_write_tx[0];
+	data->write_xfer.len = sizeof(data->reg_write_tx);
+	spi_message_init_with_transfers(&data->write_msg, &data->write_xfer, 1);
+
+	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;
+
+		return ret;
+	}
+
+	iio_dev->num_channels = ret;
+	iio_dev->channels = cs;
+
+	/* Let's assign data-sheet names to channels */
+	for (i = 0; i < iio_dev->num_channels; i++) {
+		unsigned int ch = cs[i].channel;
+
+		cs[i].datasheet_name = bd79112_chan_names[ch];
+	}
+
+	iio_dev->info = &bd79112_info;
+	iio_dev->name = "bd79112";
+	iio_dev->modes = INDIO_DIRECT_MODE;
+
+	/*
+	 * Ensure all channels are ADCs. This allows us to register the IIO
+	 * device early (before checking which pins are to be used for GPIO)
+	 * without having to worry about some pins being initially used for
+	 * GPIO.
+	 */
+	for (i = 0; i < BD79112_NUM_GPIO_EN_REGS; i++) {
+		ret = regmap_write(data->map, BD79112_FIRST_GPIO_EN_REG + i, 0);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "Failed to initialize channels\n");
+	}
+
+	ret = devm_iio_device_register(data->dev, iio_dev);
+	if (ret)
+		return dev_err_probe(data->dev, ret, "Failed to register ADC\n");
+
+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. */
+	if (!gpio_pins)
+		return 0;
+
+	/* Default all the GPIO pins to GPI */
+	for_each_set_bit(pin, &gpio_pins, BD79112_MAX_NUM_CHANNELS) {
+		ret = bd79112_gpio_dir_set(data, pin, GPIO_LINE_DIRECTION_IN);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "Failed to mark pin as GPI\n");
+	}
+
+	data->gpio_valid_mask = gpio_pins;
+	data->gc = bd79112_gpio_chip;
+	data->gc.parent = dev;
+
+	return devm_gpiochip_add_data(dev, &data->gc, data);
+}
+
+static const struct of_device_id bd79112_of_match[] = {
+	{ .compatible = "rohm,bd79112" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, bd79112_of_match);
+
+static const struct spi_device_id bd79112_id[] = {
+	{ "bd79112" },
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, bd79112_id);
+
+static struct spi_driver bd79112_driver = {
+	.driver = {
+		.name = "bd79112",
+		.of_match_table = bd79112_of_match,
+	},
+	.probe = bd79112_probe,
+	.id_table = bd79112_id,
+};
+module_spi_driver(bd79112_driver);
+
+MODULE_AUTHOR("Matti Vaittinen <mazziesaccount@gmail.com>");
+MODULE_DESCRIPTION("Driver for ROHM BD79112 ADC/GPIO");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS("IIO_DRIVER");
-- 
2.51.0

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

Hi dee Ho Andy!

Thanks again. I really appreciate the effort you put in these reviews! :)

On 04/09/2025 16:36, Andy Shevchenko wrote:
> 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.

What do you mean by a mess? Don't you have the '|' -characters aligned? 
That's very odd because they are aligned for me. Or, is this otherwise 
unclear?

> 
> ...
> 
>> +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'.

Ah, thanks for pointing out the pointlessness of the < 0! Will fix this.

I still prefer checking that the offset doesn't exceed the pin count. 
The amount of pins is clear for anyone using the device, where as amount 
of 'banks' or even 'registers' isn't quite as obvious but requires one 
to know the internals of the IC.

> 
>> +		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.

Interesting micro optimization. I had no idea about this - thanks for 
sharing it :) It's always good to learn something!

I don't like it here though. There are a few call sites where we only 
need to get the offset once, but more than one register. (Like the 
direction setting). Hence, I like to keep these operations decoupled. I 
think the benefit from this optimization is quite small. Furthermore, 
architecture specific optimizations aren't really something we should 
add in generic drivers. It may be this will never get used on the 
architecture we are optimizing for. [Sure it can make sense for 
something like regmap, which is heavily used (probably) on all 
architectures, especially if cost is just placing some lines closer to 
each others. So, nice change there!]

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

Thanks!

>> +};
> 
> ...
> 
>> +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?

Oh, well spotted! That's just a leftover from my testing where I called 
this without the gpio-chip! It should indeed be the gc->ngpio! Thanks!

> 
>> +		unsigned long bank_bits;
>> +		unsigned int reg;
>> +		int ret;
> 
>> +		if (bank_mask) {
> 
> This is a duplication, the iterator only gives non-zero "clumps".

Indeed. Thanks!

>> +			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?

I think I already explained why it is signed. I don't see how this bites 
us here? Can you please elaborate what's the problem _here_?

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

I just filled the rows to the maximum width, while keeping the item 
count same for each row and adding the comment.

I'm not really sure having 4 items / row, and adding one row more would 
be much better, but not much worse either. I can do this if you think 
it's better. (No need to even fight for that).

>> +};
> 
> ...
> 
>> +	data->vref_mv = ret / 1000;
> 
> Yeah, mV, (MICRO / MILLI) and other things I leave to other people to discuss.

Thanks Andy. I've a feeling we were somewhat stuck on this discussion 
anyways. I suppose it's nice to hear other's opinions if someone else 
really cares. This was approaching bikeshedding.

> ...
> 
>> +	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.

Sorry Andy. I think I didn't consider your suggestion thoroughly as I 
just assumed it was related to the question if the value returned by the 
devm_iio_adc_device_alloc_chaninfo_se() can be positive. I'll re-read 
your suggestion, thanks for pinging me on this.

>> +		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.

I do also still think placing the comment before assignment can give 
reader an idea that the amount of pins is fetched for this check, which 
it isn't. We fetch it for other purposes, but optimize things bailing 
out if there are no GPIOs. So, we comment to explain early exit. But 
yeah, let's see what others think of it as well.

>> +	if (!gpio_pins)
>> +		return 0;
> 

Yours,
	-- Matti

> >> +/* 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.)  
> 
> I just filled the rows to the maximum width, while keeping the item 
> count same for each row and adding the comment.
> 
> I'm not really sure having 4 items / row, and adding one row more would 
> be much better, but not much worse either. I can do this if you think 
> it's better. (No need to even fight for that).

In this case I'd do it in 4s purely because then the B ones start on
a new line and that looks nicer ;)

> 
> >> +};  
> > 
> > ...
> >   
> >> +	data->vref_mv = ret / 1000;  
> > 
> > Yeah, mV, (MICRO / MILLI) and other things I leave to other people to discuss.  
> 
> Thanks Andy. I've a feeling we were somewhat stuck on this discussion 
> anyways. I suppose it's nice to hear other's opinions if someone else 
> really cares. This was approaching bikeshedding.
> 
I'll just throw a quick comment in here.  

I'm absolutely in favour of the defines as they help in several ways.
- Stop counting 0s when there are lots of them. 
- Provide information on the unit conversion not otherwise visible
  where they happen to be used (often avoids need for local variables etc).
- Consistency across a code base.

In this particular case none apply strongly enough to force the issue.
Even so I'd prefer the units.h macros are used for the consistency reason
but I won't refuse to merge the driver over this.

One of those perfect (to me) being the enemy of good cases.

p.s. Matti, 3 versions in 4 days?  Perhaps slow down a little.

On 07/09/2025 14:16, Jonathan Cameron wrote:
> 
>>>> +/* 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.)
>>
>> I just filled the rows to the maximum width, while keeping the item
>> count same for each row and adding the comment.
>>
>> I'm not really sure having 4 items / row, and adding one row more would
>> be much better, but not much worse either. I can do this if you think
>> it's better. (No need to even fight for that).
> 
> In this case I'd do it in 4s purely because then the B ones start on
> a new line and that looks nicer ;)

Ok.

>>
>>>> +};
>>>
>>> ...
>>>    
>>>> +	data->vref_mv = ret / 1000;
>>>
>>> Yeah, mV, (MICRO / MILLI) and other things I leave to other people to discuss.
>>
>> Thanks Andy. I've a feeling we were somewhat stuck on this discussion
>> anyways. I suppose it's nice to hear other's opinions if someone else
>> really cares. This was approaching bikeshedding.
>>
> I'll just throw a quick comment in here.
> 
> I'm absolutely in favour of the defines as they help in several ways.
> - Stop counting 0s when there are lots of them.

I do agree on this when there is more than 3 zeroes. Although, for 
example 100 * 1000 is still clear to me. 100 * MILLI is not quite as 
obvious (again, to me).

> - Provide information on the unit conversion not otherwise visible
>    where they happen to be used (often avoids need for local variables etc).

Speaking only on my behalf but I'd rather see the local variables 
instead of constructs like (MICRO / MILLI) - which does not really feel 
right to me.

> - Consistency across a code base.

This probably means much more to a subsystem maintainer than to me! :) 
I'd still claim that consistency could be also achieved by consistently 
using dividers like / 1000 :)

> In this particular case none apply strongly enough to force the issue.
> Even so I'd prefer the units.h macros are used for the consistency reason
> but I won't refuse to merge the driver over this.
> 
> One of those perfect (to me) being the enemy of good cases.
> 
> p.s. Matti, 3 versions in 4 days?  Perhaps slow down a little.

I should've indeed throttled sending the versions a bit. I just did 
fixes suggested by Andy/David immediately - and then I thought I could 
just do a decent change-log for reviewers, and send them while at it.

I didn't consider someone like You who will probably skim through all 
the versions and discussion. Sorry!

Thanks for the input!

Yours,
	-- Matti

On 9/5/25 12:41 AM, Matti Vaittinen wrote:
> Hi dee Ho Andy!
> 
> Thanks again. I really appreciate the effort you put in these reviews! :)
> 
> On 04/09/2025 16:36, Andy Shevchenko wrote:
>> 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.
> 
> What do you mean by a mess? Don't you have the '|' -characters aligned? That's very odd because they are aligned for me. Or, is this otherwise unclear?

I find these diagrams very hard to read as well. I would just drop this part
and let people look it up in the datasheet. I don't think it adds anything
essential to understanding how the driver works.

On 05/09/2025 16:18, David Lechner wrote:
> On 9/5/25 12:41 AM, Matti Vaittinen wrote:
>> Hi dee Ho Andy!
>>
>> Thanks again. I really appreciate the effort you put in these reviews! :)
>>
>> On 04/09/2025 16:36, Andy Shevchenko wrote:
>>> 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.
>>
>> What do you mean by a mess? Don't you have the '|' -characters aligned? That's very odd because they are aligned for me. Or, is this otherwise unclear?
> 
> I find these diagrams very hard to read as well. I would just drop this part
> and let people look it up in the datasheet. I don't think it adds anything
> essential to understanding how the driver works.
> 

I am a bit reluctant to drop this because I don't see public data-sheet 
anywhere... :/ I think explaining the protocol is somewhat useful so 
readers can understand why we have custom regmap read/write functions, 
and hopefully also the I/O and RW bits.

I am all for improving this documentation though! Starting by dropping 
those tabs, and seeing if I can think of something else too :)

Yours,
	-- Matti