[PATCH v2 0/3] Support ROHM BD79112 ADC

[PATCH v2 0/3] Support ROHM BD79112 ADC

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 2617 bytes --]

Support ROHM BD79112 ADC/GPIO

The ROHM BD79112 is a 12-bit, 32 channel SAR ADC / GPIO IC. Or, a "Signal
Monitor Hub IC" as data-sheet describes it.

Data sheet states the maximum sampling rate to be 1 MSPS, but achieving
this would probably require the SPI and samples to be processed by
something else but the CPU running Linux. This could work with the "SPI
offloading" which has recently landed upstream - but I have no HW to test
this so nothing fancy is implemented here. It's still worth mentioning
if someone needs the speed and wants to try implementing it :)

The SPI protocol is slightly peculiar. Accesses are done in 16-bit
sequences, separated by releasing and re-aquiring the chip-select.

Register write takes 1 such sequence. The 8-bit register data to write,
is stored in the last 8 bits. The high 8 bits contain register address
and an I/O-bit which needs to be set for register accesses.

Register read consists of two 16-bit sequences (separated by
chip-select). First sequence has again the register address and an IO
bit in the high byte. Additionally, reads must have a 'read bit' set.
The last 8 bits must be zero. The register data will be carried in the
last 8 bits of the next 16-bit sequence while high bits in reply are zero.

ADC data reading is similar to register reading except:
 - No R/W bit or I/O bit should be set.
 - Register address is replaced by channel number (0 - 31).
 - Reply data is carried in the 12 low bits (instead of 8 bits) of the
   reply sequence.

The protocol is implemented using custom regmap read() and write()
operations.

Other than that, pretty standard device and driver.

Revision history:
v1 => v2:
 - Plenty of fixes to the driver (thanks to reviewers, Andy and David)
 - Add gpio-controller information to the device-tree bindings
 See individual patches for more accurate changelog

---

Matti Vaittinen (3):
  dt-bindings: iio: adc: ROHM BD79112 ADC/GPIO
  iio: adc: Support ROHM BD79112 ADC/GPIO
  MAINTAINERS: Support ROHM BD79112 ADC

 .../bindings/iio/adc/rohm,bd79112.yaml        | 116 ++++
 MAINTAINERS                                   |   3 +-
 drivers/iio/adc/Kconfig                       |  10 +
 drivers/iio/adc/Makefile                      |   1 +
 drivers/iio/adc/rohm-bd79112.c                | 551 ++++++++++++++++++
 5 files changed, 680 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/rohm,bd79112.yaml
 create mode 100644 drivers/iio/adc/rohm-bd79112.c


base-commit: d1487b0b78720b86ec2a2ac7acc683ec90627e5b
-- 
2.51.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v2 1/3] dt-bindings: iio: adc: ROHM BD79112 ADC/GPIO

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 3517 bytes --]

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.

Add a device tree binding document for the ROHM BD79112.

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

---
Revision history:
v1 => v2:
 - BD79112 can act as a GPIO controller.
---
 .../bindings/iio/adc/rohm,bd79112.yaml        | 116 ++++++++++++++++++
 1 file changed, 116 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/rohm,bd79112.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/rohm,bd79112.yaml b/Documentation/devicetree/bindings/iio/adc/rohm,bd79112.yaml
new file mode 100644
index 000000000000..8aeac5b60880
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/rohm,bd79112.yaml
@@ -0,0 +1,116 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/rohm,bd79112.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ROHM BD79112 ADC/GPO
+
+maintainers:
+  - Matti Vaittinen <mazziesaccount@gmail.com>
+
+description: |
+  The ROHM BD79112 is a 12-bit, 32-channel, SAR ADC. ADC input pins can be
+  also configured as general purpose inputs/outputs. SPI should use MODE 3.
+
+properties:
+  compatible:
+    const: rohm,bd79112
+
+  reg:
+    maxItems: 1
+
+  spi-cpha: true
+  spi-cpol: true
+
+  gpio-controller: true
+  "#gpio-cells":
+    const: 2
+
+  vdd-supply: true
+
+  iovdd-supply: true
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+patternProperties:
+  "^channel@([0-9]|[12][0-9]|3[01])$":
+    type: object
+    $ref: /schemas/iio/adc/adc.yaml#
+    description: Represents ADC channel. Omitted channels' inputs are GPIOs.
+
+    properties:
+      reg:
+        description: AIN pin number
+        minimum: 0
+        maximum: 31
+
+    required:
+      - reg
+
+    additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - iovdd-supply
+  - vdd-supply
+  - spi-cpha
+  - spi-cpol
+
+additionalProperties: false
+
+examples:
+  - |
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        adc: adc@0 {
+            compatible = "rohm,bd79112";
+            reg = <0x0>;
+
+            spi-cpha;
+            spi-cpol;
+
+            vdd-supply = <&dummyreg>;
+            iovdd-supply = <&dummyreg>;
+
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            gpio-controller;
+            #gpio-cells = <2>;
+
+            channel@0 {
+                reg = <0>;
+            };
+            channel@1 {
+                reg = <1>;
+            };
+            channel@2 {
+                reg = <2>;
+            };
+            channel@3 {
+                reg = <3>;
+            };
+            channel@4 {
+                reg = <4>;
+            };
+            channel@5 {
+                reg = <5>;
+            };
+            channel@6 {
+                reg = <6>;
+            };
+            channel@16 {
+                reg = <16>;
+            };
+            channel@20 {
+                reg = <20>;
+            };
+        };
+    };
-- 
2.51.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/3] dt-bindings: iio: adc: ROHM BD79112 ADC/GPIO

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 613 bytes --]

On Thu, Sep 04, 2025 at 03:36:30PM +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.
> 
> Add a device tree binding document for the ROHM BD79112.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

Acked-by: Conor Dooley <conor.dooley@microchip.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH v2 2/3] iio: adc: Support ROHM BD79112 ADC/GPIO

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 18586 bytes --]

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/3] iio: adc: Support ROHM BD79112 ADC/GPIO

[Andy Shevchenko]

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

Re: [PATCH v2 2/3] iio: adc: Support ROHM BD79112 ADC/GPIO

[Matti Vaittinen]

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

Re: [PATCH v2 2/3] iio: adc: Support ROHM BD79112 ADC/GPIO

[David Lechner]

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.

Re: [PATCH v2 2/3] iio: adc: Support ROHM BD79112 ADC/GPIO

[Matti Vaittinen]

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

Re: [PATCH v2 2/3] iio: adc: Support ROHM BD79112 ADC/GPIO

[Jonathan Cameron]

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

Re: [PATCH v2 2/3] iio: adc: Support ROHM BD79112 ADC/GPIO

[Matti Vaittinen]

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

[PATCH v2 3/3] MAINTAINERS: Support ROHM BD79112 ADC

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 778 bytes --]

Add the ROHM BD79112 ADC in the list of the BD791xx ADC drivers
which are maintained by undersigned.

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

---
Revision history:
v1 => :
 - no changes
---
 MAINTAINERS | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index af1c8d2bfb3d..8e78a1168c17 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21864,9 +21864,10 @@ S:	Supported
 F:	drivers/power/supply/bd99954-charger.c
 F:	drivers/power/supply/bd99954-charger.h
 
-ROHM BD79124 ADC / GPO IC
+ROHM BD791xx ADC / GPO IC
 M:	Matti Vaittinen <mazziesaccount@gmail.com>
 S:	Supported
+F:	drivers/iio/adc/rohm-bd79112.c
 F:	drivers/iio/adc/rohm-bd79124.c
 
 ROHM BH1745 COLOUR SENSOR
-- 
2.51.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]