[PATCH 0/3] Support ROHM BD79112 ADC

[PATCH 0/3] Support ROHM BD79112 ADC

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 2389 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.

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        | 118 ++++
 MAINTAINERS                                   |   3 +-
 drivers/iio/adc/Kconfig                       |  10 +
 drivers/iio/adc/Makefile                      |   1 +
 drivers/iio/adc/rohm-bd79112.c                | 542 ++++++++++++++++++
 5 files changed, 673 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 1/3] dt-bindings: iio: adc: ROHM BD79112 ADC/GPIO

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 3451 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>
---
 .../bindings/iio/adc/rohm,bd79112.yaml        | 118 ++++++++++++++++++
 1 file changed, 118 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..c340a05fbbda
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/rohm,bd79112.yaml
@@ -0,0 +1,118 @@
+# 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: 1
+    description:
+      The pin number.
+
+  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;
+
+            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 --]

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

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 17850 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>
---
 drivers/iio/adc/Kconfig        |  10 +
 drivers/iio/adc/Makefile       |   1 +
 drivers/iio/adc/rohm-bd79112.c | 542 +++++++++++++++++++++++++++++++++
 3 files changed, 553 insertions(+)
 create mode 100644 drivers/iio/adc/rohm-bd79112.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index e3d3826c3357..4b78929bb257 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_I2C
+	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..84d9768fb343
--- /dev/null
+++ b/drivers/iio/adc/rohm-bd79112.c
@@ -0,0 +1,542 @@
+// 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 <linux/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/err.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/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] __aligned(IIO_DMA_MINALIGN);
+	/* 12-bit of ADC data or 8 bit of reg data */
+	__be16 read_rx __aligned(IIO_DMA_MINALIGN);
+};
+
+/*
+ * The ADC data is read issuing SPI-command matching the channel number.
+ * We treat this as a register address.
+ */
+#define BD79112_REG_AGIO0A		0x0
+#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(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)
+		if (*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;
+	}
+
+	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 int i;
+
+	for (i = 0; i < 4; i++) {
+		unsigned int bank_mask, reg, regval, regmask;
+		int ret;
+
+		bank_mask = 0xff << 8 * i;
+		regmask = (*mask & bank_mask) << 8 * i;
+
+		if (!regmask)
+			continue;
+
+		reg = BD79112_REG_GPO_VALUE_A0_A7 - i;
+		regval = (*bits & bank_mask) >> 8 * i;
+		ret = regmap_update_bits(data->map, reg, regmask, regval);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int bd79112_gpio_dir_set(struct bd79112_data *data, unsigned int offset,
+				int dir)
+{
+	unsigned int set_reg, clear_reg, bit;
+	int ret;
+
+	bit = GET_GPIO_BIT(offset);
+
+	if (dir == GPIO_LINE_DIRECTION_IN) {
+		set_reg = GET_GPI_EN_REG(offset);
+		clear_reg = GET_GPO_EN_REG(offset);
+	} else {
+		set_reg = GET_GPO_EN_REG(offset);
+		clear_reg = GET_GPI_EN_REG(offset);
+	}
+
+	ret = regmap_set_bits(data->map, set_reg, bit);
+	if (ret)
+		return ret;
+
+	return regmap_clear_bits(data->map, clear_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;
+}
+
+static int bd79112_probe(struct spi_device *spi)
+{
+	/* ADC channels as named in the data-sheet */
+	static const char * const chan_names[] = {
+		"AGIO0A", "AGIO1A", "AGIO2A", "AGIO3A", "AGIO4A", "AGIO5A",
+		"AGIO6A", "AGIO7A", "AGIO8A", "AGIO9A", "AGIO10A", "AGIO11A",
+		"AGIO11A", "AGIO12A", "AGIO13A", "AGIO14A", "AGIO15A",
+		"AGIO0B", "AGIO1B", "AGIO2B", "AGIO3B", "AGIO4B", "AGIO5B",
+		"AGIO6B", "AGIO7B", "AGIO8B", "AGIO9B", "AGIO10B", "AGIO11B",
+		"AGIO11B", "AGIO12B", "AGIO13B", "AGIO14B", "AGIO15B",
+	};
+	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;
+	}
+
+	/* 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 = chan_names[ch];
+	}
+
+	iio_dev->channels = cs;
+	iio_dev->num_channels = ret;
+	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);
+
+	/* We're done if all channels are reserved for ADC. */
+	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 --]

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

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 934 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>
---

This patch got some last minute changes after other patches were already
sent. I hope I got the message-IDs right - but if I didin't - I'm sorry.
In that case I'll re-spin series after some delay :)

 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 --]

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

[Andy Shevchenko]

On Tue, Sep 02, 2025 at 03:24:31PM +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.

...

> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/err.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>

Incomplete list. See below (it doesn't mean I caught up all of the missing
inclusions).

...

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

Perhaps _mV to follow the actual unit spelling?
(and yes, I know that both variants are present in the kernel)

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

+ types.h for u8 and indirectly for __aligned.

> +	/* 8-bit address followed by 8-bit data */
> +	u8 reg_write_tx[2] __aligned(IIO_DMA_MINALIGN);
> +	/* 12-bit of ADC data or 8 bit of reg data */
> +	__be16 read_rx __aligned(IIO_DMA_MINALIGN);
> +};

...

> +#define BD79112_REG_AGIO0A		0x0

Keep it fixed-width. i.e. 0x00

> +#define BD79112_REG_AGIO15B		0x1f

...

> +#define BD79112_BIT_IO			BIT(5)

+ bits.h (but see about bitops.h below)

...

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

> + *

Stray empty line.

> + */

...

> +static int _get_gpio_reg(int offset, unsigned int base)
> +{

Why offset is signed?

> +	int regoffset = offset / 8;
> +
> +	if (offset > 31 || offset < 0)
> +		return -EINVAL;

+ errno.h (but since you use IS_ERR(), the err.h should be)

> +	return base - regoffset;
> +}

...

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

asm/byteorder.h

> +
> +	if (reg & BD79112_BIT_IO)
> +		if (*val & BD79112_ADC_STATUS_FLAG)
> +			dev_err(data->dev, "ADC pin configured as GPIO\n");

Missing {}, I think one needs to refresh a memory of kernel coding style.

> +	return ret;
> +}

...

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

> +	return -EINVAL;

Why not making it default case? This is how most of the IIO drivers do.

> +}

...

> +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 int i;
> +
> +	for (i = 0; i < 4; i++) {
> +		unsigned int bank_mask, reg, regval, regmask;
> +		int ret;
> +
> +		bank_mask = 0xff << 8 * i;
> +		regmask = (*mask & bank_mask) << 8 * i;

Why all this?

We have for_each_set_clump8().

> +		if (!regmask)
> +			continue;
> +
> +		reg = BD79112_REG_GPO_VALUE_A0_A7 - i;
> +		regval = (*bits & bank_mask) >> 8 * i;
> +		ret = regmap_update_bits(data->map, reg, regmask, regval);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}

...

> +static int bd79112_gpio_dir_set(struct bd79112_data *data, unsigned int offset,
> +				int dir)

Why dir is int? And why in negative case or other than _IN we switch pin to
output mode. It's dangerous default.

> +{
> +	unsigned int set_reg, clear_reg, bit;
> +	int ret;
> +
> +	bit = GET_GPIO_BIT(offset);
> +
> +	if (dir == GPIO_LINE_DIRECTION_IN) {
> +		set_reg = GET_GPI_EN_REG(offset);
> +		clear_reg = GET_GPO_EN_REG(offset);
> +	} else {
> +		set_reg = GET_GPO_EN_REG(offset);
> +		clear_reg = GET_GPI_EN_REG(offset);
> +	}

> +	ret = regmap_set_bits(data->map, set_reg, bit);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_clear_bits(data->map, clear_reg, bit);

I believe the order depends on the out-in or in-out switch.
Otherwise it might be potential glitches on input (hw) buffer.
Right now when it's not an interrupt it may be okay to don't
bother, but in general I see a potential issues with that.

> +}

...

> +static int bd79112_gpio_output(struct gpio_chip *gc, unsigned int offset,
> +			       int value)

Why value is signed?

...

> +static int bd79112_get_gpio_pins(const struct iio_chan_spec *cs, int num_channels)
> +{
> +	int i, gpio_channels;

Why signed?

...

> +static int bd79112_probe(struct spi_device *spi)
> +{
> +	/* ADC channels as named in the data-sheet */
> +	static const char * const chan_names[] = {
> +		"AGIO0A", "AGIO1A", "AGIO2A", "AGIO3A", "AGIO4A", "AGIO5A",
> +		"AGIO6A", "AGIO7A", "AGIO8A", "AGIO9A", "AGIO10A", "AGIO11A",
> +		"AGIO11A", "AGIO12A", "AGIO13A", "AGIO14A", "AGIO15A",
> +		"AGIO0B", "AGIO1B", "AGIO2B", "AGIO3B", "AGIO4B", "AGIO5B",
> +		"AGIO6B", "AGIO7B", "AGIO8B", "AGIO9B", "AGIO10B", "AGIO11B",
> +		"AGIO11B", "AGIO12B", "AGIO13B", "AGIO14B", "AGIO15B",

Can you make all of the lines to be the same in terms of amount of entries?

> +	};

This seems to be hidden in the function while it's used for the whole life time
f the device. Why not move it outside of the function?

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

You have dev, use it.

> +	if (IS_ERR(data->map))
> +		return dev_err_probe(dev, PTR_ERR(data->map),
> +				     "Failed to initialize Regmap\n");

+ dev_printk.h

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

(MICRO / MILLI)

> +	ret = devm_regulator_get_enable(dev, "iovdd");
> +	if (ret < 0)

Does it return positive or zero on success?

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

Hmm... Indentation can be amended.

> +	if (ret < 0) {

Why ' < 0' ?

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

Assuming ret can't be positive this can be refactored as:

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

I find it easier to follow.

> +	/* 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 = chan_names[ch];
> +	}
> +
> +	iio_dev->channels = cs;
> +	iio_dev->num_channels = ret;
> +	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);

> +

Instead of leaving this rather unneeded blank line I would move above...

> +	/* We're done if all channels are reserved for ADC. */

...to be here

	gpio_pins = bd79112_get_gpio_pins(iio_dev->channels,
					  iio_dev->num_channels);

> +	if (!gpio_pins)
> +		return 0;
> +
> +	/* Default all the GPIO pins to GPI */
> +	for_each_set_bit(pin, &gpio_pins, BD79112_MAX_NUM_CHANNELS) {

+ bitops.h

> +		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);
> +}

-- 
With Best Regards,
Andy Shevchenko

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

[David Lechner]

On 9/2/25 7:24 AM, 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.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> ---
>  drivers/iio/adc/Kconfig        |  10 +
>  drivers/iio/adc/Makefile       |   1 +
>  drivers/iio/adc/rohm-bd79112.c | 542 +++++++++++++++++++++++++++++++++
>  3 files changed, 553 insertions(+)
>  create mode 100644 drivers/iio/adc/rohm-bd79112.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index e3d3826c3357..4b78929bb257 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_I2C

I think you want SPI rather than I2C. :-)

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



> +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] __aligned(IIO_DMA_MINALIGN);
> +	/* 12-bit of ADC data or 8 bit of reg data */
> +	__be16 read_rx __aligned(IIO_DMA_MINALIGN);

Usually, we only need one __aligned(IIO_DMA_MINALIGN) (on the first
field). Since these are only used for SPI messages and we can only
send one message at a time, there isn't a way for there to be a
problem that would require them to each need to be in their own
cache line.

> +};
> +



> +static int bd79112_probe(struct spi_device *spi)
> +{

...

> +	iio_dev->channels = cs;
> +	iio_dev->num_channels = ret;

This is quite far from where it is assigned. Better to have a dedicated
local variable for this.

> +	iio_dev->info = &bd79112_info;
> +	iio_dev->name = "bd79112";
> +	iio_dev->modes = INDIO_DIRECT_MODE;
> +

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

[Rob Herring (Arm)]

On Tue, 02 Sep 2025 15:23:54 +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>
> ---
>  .../bindings/iio/adc/rohm,bd79112.yaml        | 118 ++++++++++++++++++
>  1 file changed, 118 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/rohm,bd79112.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/rohm,bd79112.example.dtb: adc@0 (rohm,bd79112): '#gpio-cells' is a dependency of 'gpio-controller'
	from schema $id: http://devicetree.org/schemas/gpio/gpio.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/77c36ecaf5992ebcabf6ce862bf2a6ec72d9f606.1756813980.git.mazziesaccount@gmail.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

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

[Linus Walleij]

Hi Matti,

On Tue, Sep 2, 2025 at 2:24 PM Matti Vaittinen <mazziesaccount@gmail.com> 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.
>
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

> +static int bd79112_gpio_dir_get(struct gpio_chip *gc, unsigned int offset)
> +static int bd79112_gpio_get(struct gpio_chip *gc, unsigned int offset)
> +static int bd79112_gpio_set(struct gpio_chip *gc, unsigned int offset,
> +                           int value)
> +static int bd79112_gpio_set_multiple(struct gpio_chip *gc, unsigned long *mask,
> +                                    unsigned long *bits)
> +static int bd79112_gpio_dir_set(struct bd79112_data *data, unsigned int offset,
> +                               int dir)
> +static int bd79112_gpio_input(struct gpio_chip *gc, unsigned int offset)
> +static int bd79112_gpio_output(struct gpio_chip *gc, unsigned int offset,
> +                              int value)

This looks like it could use

select GPIO_REGMAP

#include <linux/gpio/regmap.h>

struct gpio_regmap_config config = {};

etc. Did you check out the GPIO_REGMAP
helper library? It can work wonders to reduce code
footprint, just git grep and look a bit at what the drivers
using it does.

Yours,
Linus Walleij

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

[Matti Vaittinen]

Hi deee Ho Linus,

Long time no chat. Thanks for the review!

On 03/09/2025 01:34, Linus Walleij wrote:
> Hi Matti,
> 
> On Tue, Sep 2, 2025 at 2:24 PM Matti Vaittinen <mazziesaccount@gmail.com> 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.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> 
>> +static int bd79112_gpio_dir_get(struct gpio_chip *gc, unsigned int offset)
>> +static int bd79112_gpio_get(struct gpio_chip *gc, unsigned int offset)
>> +static int bd79112_gpio_set(struct gpio_chip *gc, unsigned int offset,
>> +                           int value)
>> +static int bd79112_gpio_set_multiple(struct gpio_chip *gc, unsigned long *mask,
>> +                                    unsigned long *bits)
>> +static int bd79112_gpio_dir_set(struct bd79112_data *data, unsigned int offset,
>> +                               int dir)
>> +static int bd79112_gpio_input(struct gpio_chip *gc, unsigned int offset)
>> +static int bd79112_gpio_output(struct gpio_chip *gc, unsigned int offset,
>> +                              int value)
> 
> This looks like it could use
> 
> select GPIO_REGMAP
> 
> #include <linux/gpio/regmap.h>
> 
> struct gpio_regmap_config config = {};
> 
> etc. Did you check out the GPIO_REGMAP
> helper library?

I did - but that was couple of years ago :) I was very excited about it 
back then. I actually tried (tried hard, fingers almost bleeding) to 
write a patch to make it cover all of the ROHM PMIC GPIOs which I had to 
deal with. I thought it won't get it's full potential due to it's 
somewhat inflexible design. (And to tell the truth, I still believe so).

Anyways, fast-forward to this day, I don't see it handling valid_mask. I 
think it is a must for this device/driver, where pins can be either 
GPIOs or ADC inputs.

Yours,
	-- Matti

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

[Linus Walleij]

On Wed, Sep 3, 2025 at 7:23 AM Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> Anyways, fast-forward to this day, I don't see it handling valid_mask. I
> think it is a must for this device/driver, where pins can be either
> GPIOs or ADC inputs.

Why not just add a .init_valid_mask() to
struct gpio_regmap_config so it can just pass that
down to its gpio_chip?

OK I don't want to load you with too much extra work for
the driver, but it seems such a small thing for a blocker,
and Michael who wrote the library is really helpful
with extending the code, so consider it!

Yours,
Linus Walleij

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

[Matti Vaittinen]

Hi deee Ho Andy,

On 02/09/2025 17:15, Andy Shevchenko wrote:
> On Tue, Sep 02, 2025 at 03:24:31PM +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.
> 
> ...
> 
>> +#include <linux/array_size.h>
>> +#include <linux/bitfield.h>
>> +#include <linux/err.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>
> 
> Incomplete list. See below (it doesn't mean I caught up all of the missing
> inclusions).

Thanks!

> ...
> 
>> +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;
> 
> Perhaps _mV to follow the actual unit spelling?
> (and yes, I know that both variants are present in the kernel)

The 'mv' is a deliberate choise. I still remember our previous 
discussion about this same topic [1].

>> +	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);
> 
> + types.h for u8 and indirectly for __aligned.

Thanks! It's super helpful to list the missing headers. (And no, there 
is no sarcasm, it really is hard to pick the right headers at times! I 
find your review _very_ helpful on that).

I won't repeat this for other header comments you made.

>> +	/* 8-bit address followed by 8-bit data */
>> +	u8 reg_write_tx[2] __aligned(IIO_DMA_MINALIGN);
>> +	/* 12-bit of ADC data or 8 bit of reg data */
>> +	__be16 read_rx __aligned(IIO_DMA_MINALIGN);
>> +};
> ...
> 
>> +static int _get_gpio_reg(int offset, unsigned int base)
>> +{
> 
> Why offset is signed?

Just out of habit. Can be unsigned.

>> +	int regoffset = offset / 8;
>> +
>> +	if (offset > 31 || offset < 0)
>> +		return -EINVAL;

...

>> +
>> +	if (reg & BD79112_BIT_IO)
>> +		if (*val & BD79112_ADC_STATUS_FLAG)
>> +			dev_err(data->dev, "ADC pin configured as GPIO\n");
> 
> Missing {}, I think one needs to refresh a memory of kernel coding style.

Could be. I've never been overly enthusiastic what comes to memorizing 
styling rules. (I believe that if there are hard styling rules, those 
should be enforced/checked by tooling like checkpatch.) Still, I've 
myself never really been fond of having too strict set of styling rules.

Thus, I really thought useless brackets were useless.

TLDR; I can combine the conditions in one if (), thanks.

> 
>> +	return ret;
>> +}
> 
> ...
> 
>> +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;
>> +	}
> 
>> +	return -EINVAL;
> 
> Why not making it default case? This is how most of the IIO drivers do.

Hmm. Thanks. I wonder why I didn't see nagging from unhandled cases. I 
think some tooling used to emit those warnings...

> 
>> +}
> 
> ...
> 
>> +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 int i;
>> +
>> +	for (i = 0; i < 4; i++) {
>> +		unsigned int bank_mask, reg, regval, regmask;
>> +		int ret;
>> +
>> +		bank_mask = 0xff << 8 * i;
>> +		regmask = (*mask & bank_mask) << 8 * i;
> 
> Why all this?
> 
> We have for_each_set_clump8().

Haven't yet checked the for_each_set_clump8() - but I assume because I 
had no memory trace about kernel having such function :) (Thanks!)

>> +		if (!regmask)
>> +			continue;
>> +
>> +		reg = BD79112_REG_GPO_VALUE_A0_A7 - i;
>> +		regval = (*bits & bank_mask) >> 8 * i;
>> +		ret = regmap_update_bits(data->map, reg, regmask, regval);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	return 0;
>> +}
> 
> ...
> 
>> +static int bd79112_gpio_dir_set(struct bd79112_data *data, unsigned int offset,
>> +				int dir)
> 
> Why dir is int? 

Because it is expected to be either GPIO_LINE_DIRECTION_IN or 
GPIO_LINE_DIRECTION_OUT, which are integers, right?

> And why in negative case or other than _IN we switch pin to
> output mode. It's dangerous default.

This function is only called with GPIO_LINE_DIRECTION_IN or 
GPIO_LINE_DIRECTION_OUT, but I agree it's better to invert the default.

>> +{
>> +	unsigned int set_reg, clear_reg, bit;
>> +	int ret;
>> +
>> +	bit = GET_GPIO_BIT(offset);
>> +
>> +	if (dir == GPIO_LINE_DIRECTION_IN) {
>> +		set_reg = GET_GPI_EN_REG(offset);
>> +		clear_reg = GET_GPO_EN_REG(offset);
>> +	} else {
>> +		set_reg = GET_GPO_EN_REG(offset);
>> +		clear_reg = GET_GPI_EN_REG(offset);
>> +	}
> 
>> +	ret = regmap_set_bits(data->map, set_reg, bit);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return regmap_clear_bits(data->map, clear_reg, bit);
> 
> I believe the order depends on the out-in or in-out switch.
> Otherwise it might be potential glitches on input (hw) buffer.
> Right now when it's not an interrupt it may be okay to don't
> bother, but in general I see a potential issues with that.

Can you please explain what you mean. I am not sure I can follow you here.

>> +}
> 
> ...
> 
>> +static int bd79112_gpio_output(struct gpio_chip *gc, unsigned int offset,
>> +			       int value)
> 
> Why value is signed?

Because the direction_output is defined as:
int                     (*direction_output)(struct gpio_chip *gc,
					unsigned int offset, int value);
in a tree this series is based on?

> 
> ...
> 
>> +static int bd79112_get_gpio_pins(const struct iio_chan_spec *cs, int num_channels)
>> +{
>> +	int i, gpio_channels;
> 
> Why signed?

For 'i'? Because it matches with the num_channels, which is int, just as 
it is in the struct iio_dev. This is not a problem because we know the 
maximum number of channels is 32.

For 'gpio_channels'? Mostly because int works and it was handy to just 
use same type as for 'i'. It works as we just need enough memory to fit 
in 32 bits. The integer representation of the value is meaningless and 
thus the sign does not play any role here.

> ...
> 
>> +static int bd79112_probe(struct spi_device *spi)
>> +{
>> +	/* ADC channels as named in the data-sheet */
>> +	static const char * const chan_names[] = {
>> +		"AGIO0A", "AGIO1A", "AGIO2A", "AGIO3A", "AGIO4A", "AGIO5A",
>> +		"AGIO6A", "AGIO7A", "AGIO8A", "AGIO9A", "AGIO10A", "AGIO11A",
>> +		"AGIO11A", "AGIO12A", "AGIO13A", "AGIO14A", "AGIO15A",
>> +		"AGIO0B", "AGIO1B", "AGIO2B", "AGIO3B", "AGIO4B", "AGIO5B",
>> +		"AGIO6B", "AGIO7B", "AGIO8B", "AGIO9B", "AGIO10B", "AGIO11B",
>> +		"AGIO11B", "AGIO12B", "AGIO13B", "AGIO14B", "AGIO15B",
> 
> Can you make all of the lines to be the same in terms of amount of entries?

Maybe :) I would like to know why? As you know, I prefer to keep lines 
short to fit multiple terminals in parallel, so this will probably make 
the entry to consume more rows. Thus, I would like to have a solid reason.

>> +	};
> 
> This seems to be hidden in the function while it's used for the whole life time
> f the device. Why not move it outside of the function?

Mostly just because this is not directly referred to outside the probe 
by this driver and I don't think it should be referred. OTOH, having 
this global would allow squeezing the indentiation - which would make it 
tad more compact - so I think I'll just go with your suggestion for the 
next revision :) Thanks.

..

> 
>> +	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;
> 
> (MICRO / MILLI)

I find this much more confusing than plain 1000. (I know we had this 
type of discussion before. See [1] again).

> 
>> +	ret = devm_regulator_get_enable(dev, "iovdd");
>> +	if (ret < 0)
> 
> Does it return positive or zero on success?

Zero, thanks!

>> +		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);
> 
> Hmm... Indentation can be amended.

Sorry but I am not sure I understand what you mean by amended? Can you 
please go an extra mile and explain :)

> 
>> +	if (ret < 0) {
> 
> Why ' < 0' ?

because the devm_iio_adc_device_alloc_chaninfo_se() returns number of 
found channels.

>> +	/* 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 = chan_names[ch];
>> +	}
>> +
>> +	iio_dev->channels = cs;
>> +	iio_dev->num_channels = ret;
>> +	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);
> 
>> +
> 
> Instead of leaving this rather unneeded blank line I would move above...
> 
>> +	/* We're done if all channels are reserved for ADC. */
> 
> ...to be here
> 
> 	gpio_pins = bd79112_get_gpio_pins(iio_dev->channels,
> 					  iio_dev->num_channels);

I suppose you mean something like:

register_gpios:
	/* We're done if all channels are reserved for ADC. */
	gpio_pins = bd79112_get_gpio_pins(iio_dev->channels,
                                           iio_dev->num_channels);
	if (!gpio_pins)
		return 0;

right?

I don't like this because now the comment suggests we do call 
bd79112_get_gpio_pins() only to see if all channels were for ADCs. This, 
however, is not THE reason for this call, only an optimization. I think: 
having:

         /* We're done if all channels are reserved for ADC. */
         if (!gpio_pins)
                 return 0;

is clearer.



Thanks (again) for the review! I always appreciate your work, even if I 
don't always agree on everything :)

[1]: https://lore.kernel.org/all/20250505173157.57aa16f9@jic23-huawei/


Yours,
	-- Matti

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

[Matti Vaittinen]

On 02/09/2025 18:14, David Lechner wrote:
> On 9/2/25 7:24 AM, 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.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>> ---
>>   drivers/iio/adc/Kconfig        |  10 +
>>   drivers/iio/adc/Makefile       |   1 +
>>   drivers/iio/adc/rohm-bd79112.c | 542 +++++++++++++++++++++++++++++++++
>>   3 files changed, 553 insertions(+)
>>   create mode 100644 drivers/iio/adc/rohm-bd79112.c
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index e3d3826c3357..4b78929bb257 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_I2C
> 
> I think you want SPI rather than I2C. :-)

Ouch! :) Well spotted! Thanks!

> 
>> +	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.
>> +
> 
> 
> 
>> +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] __aligned(IIO_DMA_MINALIGN);
>> +	/* 12-bit of ADC data or 8 bit of reg data */
>> +	__be16 read_rx __aligned(IIO_DMA_MINALIGN);
> 
> Usually, we only need one __aligned(IIO_DMA_MINALIGN) (on the first
> field). Since these are only used for SPI messages and we can only
> send one message at a time, there isn't a way for there to be a
> problem that would require them to each need to be in their own
> cache line.

I was wondering about this and hoping to get a good comment explaining 
it :) I noticed I don't really know how different SPI controllers handle 
DMA or cache. Hence I just went with what felt like safest option - and 
hoped to get a comment like yours if it wasn't needed ;) So, Thanks!

> 
>> +};
>> +
> 
> 
> 
>> +static int bd79112_probe(struct spi_device *spi)
>> +{
> 
> ...
> 
>> +	iio_dev->channels = cs;
>> +	iio_dev->num_channels = ret;
> 
> This is quite far from where it is assigned. Better to have a dedicated
> local variable for this.

Gah! I agree. Actually there is now a bug where the 
iio_dev->num_channels is used before it is set. So, datasheet names 
won't be assigned correctly. (I did some re-ordering of stuff in probe 
to cover the 'all ADCs and all GPIOs cases. I must've messed this at 
that point!).

Thanks! I feel like I owe you a beer :] Just remind me if we meet! ;)

Yours,
	-- Matti

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

[Matti Vaittinen]

On 03/09/2025 09:47, Linus Walleij wrote:
> On Wed, Sep 3, 2025 at 7:23 AM Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> Anyways, fast-forward to this day, I don't see it handling valid_mask. I
>> think it is a must for this device/driver, where pins can be either
>> GPIOs or ADC inputs.
> 
> Why not just add a .init_valid_mask() to
> struct gpio_regmap_config so it can just pass that
> down to its gpio_chip?

Sigh. I suppose that would technically make sense. (So would allowing 
other IC-specific callbacks ;) ).

> OK I don't want to load you with too much extra work for
> the driver, but it seems such a small thing for a blocker,
> and Michael who wrote the library is really helpful
> with extending the code, so consider it!

I suppose I can see how that works out. I am not a fan of maintaining 
the extra code. Thanks for the suggestion.

Yours,
	-- Matti

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

[Matti Vaittinen]

On 03/09/2025 09:47, Linus Walleij wrote:
> On Wed, Sep 3, 2025 at 7:23 AM Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> Anyways, fast-forward to this day, I don't see it handling valid_mask. I
>> think it is a must for this device/driver, where pins can be either
>> GPIOs or ADC inputs.
> 
> Why not just add a .init_valid_mask() to
> struct gpio_regmap_config so it can just pass that
> down to its gpio_chip?

Sigh. I suppose that would technically make sense. (So would allowing 
other IC-specific callbacks... ;) ).

> OK I don't want to load you with too much extra work for
> the driver, but it seems such a small thing for a blocker,
> and Michael who wrote the library is really helpful
> with extending the code, so consider it!

I suppose I can see how that works out. I am not a fan of maintaining 
the extra code. Thanks for the suggestion.

Yours,
	-- Matti

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

[Matti Vaittinen]

On 03/09/2025 10:17, Matti Vaittinen wrote:
> On 03/09/2025 09:47, Linus Walleij wrote:
>> On Wed, Sep 3, 2025 at 7:23 AM Matti Vaittinen 
>> <mazziesaccount@gmail.com> wrote:
>>
>>> Anyways, fast-forward to this day, I don't see it handling valid_mask. I
>>> think it is a must for this device/driver, where pins can be either
>>> GPIOs or ADC inputs.
>>
>> Why not just add a .init_valid_mask() to
>> struct gpio_regmap_config so it can just pass that
>> down to its gpio_chip?
> 
> Sigh. I suppose that would technically make sense. (So would allowing 
> other IC-specific callbacks... ;) ).
> 
>> OK I don't want to load you with too much extra work for
>> the driver, but it seems such a small thing for a blocker,
>> and Michael who wrote the library is really helpful
>> with extending the code, so consider it!
> 
> I suppose I can see how that works out. I am not a fan of maintaining 
> the extra code. Thanks for the suggestion.

After more thorough look, I don't think the gpio-regmap is flexible 
enough for this IC (either). The BD79112 direction setting requires 
accessing two registers (one for input, other for output), and I don't 
see how this could be done with the current gpio-regmap.

...If only I was able to override the direction setting callback with IC 
specific one, then I could use the regular gpio-regmap for all the rest...

Yours,
	-- Matti

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

[Nuno Sá]

On Wed, 2025-09-03 at 08:47 +0200, Linus Walleij wrote:
> On Wed, Sep 3, 2025 at 7:23 AM Matti Vaittinen <mazziesaccount@gmail.com>
> wrote:
> 
> > Anyways, fast-forward to this day, I don't see it handling valid_mask. I
> > think it is a must for this device/driver, where pins can be either
> > GPIOs or ADC inputs.
> 
> Why not just add a .init_valid_mask() to
> struct gpio_regmap_config so it can just pass that
> down to its gpio_chip?
> 

Hmm I guess I'll get a similar comment on the series I just sent [1] :)


[1]: https://lore.kernel.org/linux-hwmon/20250903-ltc4283-support-v2-0-6bce091510bf@analog.com/
- Nuno Sá

> OK I don't want to load you with too much extra work for
> the driver, but it seems such a small thing for a blocker,
> and Michael who wrote the library is really helpful
> with extending the code, so consider it!
> 
> Yours,
> Linus Walleij

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

[Andy Shevchenko]

On Wed, Sep 03, 2025 at 09:52:02AM +0300, Matti Vaittinen wrote:
> On 02/09/2025 17:15, Andy Shevchenko wrote:
> > On Tue, Sep 02, 2025 at 03:24:31PM +0300, Matti Vaittinen wrote:

...

> > > +	unsigned int vref_mv;
> > 
> > Perhaps _mV to follow the actual unit spelling?
> > (and yes, I know that both variants are present in the kernel)
> 
> The 'mv' is a deliberate choise. I still remember our previous discussion
> about this same topic [1].

Rings a bell. And I still think the proper spelling is more important when
we talk about physics.

...

> TLDR; I can combine the conditions in one if (), thanks.

Works for me.

...

> > > +static int bd79112_gpio_dir_set(struct bd79112_data *data, unsigned int offset,
> > > +				int dir)
> > > +{
> > > +	unsigned int set_reg, clear_reg, bit;
> > > +	int ret;
> > > +
> > > +	bit = GET_GPIO_BIT(offset);
> > > +
> > > +	if (dir == GPIO_LINE_DIRECTION_IN) {
> > > +		set_reg = GET_GPI_EN_REG(offset);
> > > +		clear_reg = GET_GPO_EN_REG(offset);
> > > +	} else {
> > > +		set_reg = GET_GPO_EN_REG(offset);
> > > +		clear_reg = GET_GPI_EN_REG(offset);
> > > +	}
> > 
> > > +	ret = regmap_set_bits(data->map, set_reg, bit);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	return regmap_clear_bits(data->map, clear_reg, bit);
> > 
> > I believe the order depends on the out-in or in-out switch.
> > Otherwise it might be potential glitches on input (hw) buffer.
> > Right now when it's not an interrupt it may be okay to don't
> > bother, but in general I see a potential issues with that.
> 
> Can you please explain what you mean. I am not sure I can follow you here.

If we set out first with enabled input, input can get a value from the output.
If the value before was different, it will be an event condition (of course
it all depends on the HW and I haven't read datasheet for this one). So when

1) turn out on
2) ...possible event condition... (if external bias, or similar)
3) turn in off

Compare to

1) turn in off
2) ...possible glitch on the wire... (if no external bias, or similar)
3) turn out on

> > > +}

...

> > > +static int bd79112_probe(struct spi_device *spi)
> > > +{
> > > +	/* ADC channels as named in the data-sheet */
> > > +	static const char * const chan_names[] = {
> > > +		"AGIO0A", "AGIO1A", "AGIO2A", "AGIO3A", "AGIO4A", "AGIO5A",
> > > +		"AGIO6A", "AGIO7A", "AGIO8A", "AGIO9A", "AGIO10A", "AGIO11A",
> > > +		"AGIO11A", "AGIO12A", "AGIO13A", "AGIO14A", "AGIO15A",
> > > +		"AGIO0B", "AGIO1B", "AGIO2B", "AGIO3B", "AGIO4B", "AGIO5B",
> > > +		"AGIO6B", "AGIO7B", "AGIO8B", "AGIO9B", "AGIO10B", "AGIO11B",
> > > +		"AGIO11B", "AGIO12B", "AGIO13B", "AGIO14B", "AGIO15B",
> > 
> > Can you make all of the lines to be the same in terms of amount of entries?
> 
> Maybe :) I would like to know why? As you know, I prefer to keep lines short
> to fit multiple terminals in parallel, so this will probably make the entry
> to consume more rows. Thus, I would like to have a solid reason.

Sure, the array above is unindexed. It's prone to errors and typos.
Moreover, it's really hard to follow in case one needs to debug such
a typo and see which value needs to be fixed (imagine you typed twice
the same name).

Recommended way is to use power-of-two per line (and even add a comment
at the end), like

static const char * const chan_names[] = {
	"AGIO0A", "AGIO1A", "AGIO2A", "AGIO3A",		/*  0 -  3 */
	"AGIO4A", "AGIO5A", "AGIO6A", "AGIO7A",		/*  4 -  7 */
	"AGIO8A", "AGIO9A", "AGIO10A", "AGIO11A",	/*  8 - 11 */
	...

(or hexadecimal offsets, whatever is better and more in accordance with
 the SW / data sheet).

> > > +	};

...

> > > +	data->vref_mv = ret / 1000;
> > 
> > (MICRO / MILLI)
> 
> I find this much more confusing than plain 1000. (I know we had this type of
> discussion before. See [1] again).

Rings a bell, but that's what IIO reviewers suggest to do nowadays as a
compromise between creating a new bunch of unit (V) related definitions.

> > > +	ret = devm_iio_adc_device_alloc_chaninfo_se(dev, &bd79112_chan_template,
> > > +		BD79112_MAX_NUM_CHANNELS - 1, &cs);
> > 
> > Hmm... Indentation can be amended.
> 
> Sorry but I am not sure I understand what you mean by amended? Can you
> please go an extra mile and explain :)

	ret = devm_iio_adc_device_alloc_chaninfo_se(dev,
						    &bd79112_chan_template,
						    BD79112_MAX_NUM_CHANNELS - 1,
						    &cs);

...

> > > +	gpio_pins = bd79112_get_gpio_pins(iio_dev->channels,
> > > +					  iio_dev->num_channels);
> > 
> > > +
> > 
> > Instead of leaving this rather unneeded blank line I would move above...
> > 
> > > +	/* We're done if all channels are reserved for ADC. */
> > 
> > ...to be here
> > 
> > 	gpio_pins = bd79112_get_gpio_pins(iio_dev->channels,
> > 					  iio_dev->num_channels);
> 
> I suppose you mean something like:
> 
> register_gpios:
> 	/* We're done if all channels are reserved for ADC. */
> 	gpio_pins = bd79112_get_gpio_pins(iio_dev->channels,
>                                           iio_dev->num_channels);
> 	if (!gpio_pins)
> 		return 0;
> 
> right?

Yes.

> I don't like this because now the comment suggests we do call
> bd79112_get_gpio_pins() only to see if all channels were for ADCs. This,
> however, is not THE reason for this call, only an optimization. I think:
> having:
> 
>         /* We're done if all channels are reserved for ADC. */

Then you can amend the comment

         /* If all channels are reserved for ADC, we are done. */

>         if (!gpio_pins)
>                 return 0;
> 
> is clearer.

Which makes my approach sustainable.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Wed, Sep 03, 2025 at 12:02:56PM +0100, Nuno Sá wrote:
> On Wed, 2025-09-03 at 08:47 +0200, Linus Walleij wrote:
> > On Wed, Sep 3, 2025 at 7:23 AM Matti Vaittinen <mazziesaccount@gmail.com>
> > wrote:

...

> > > Anyways, fast-forward to this day, I don't see it handling valid_mask. I
> > > think it is a must for this device/driver, where pins can be either
> > > GPIOs or ADC inputs.
> > 
> > Why not just add a .init_valid_mask() to
> > struct gpio_regmap_config so it can just pass that
> > down to its gpio_chip?
> 
> Hmm I guess I'll get a similar comment on the series I just sent [1] :)

Right, you have the mask but you don't use gpio-regmap!

> [1]: https://lore.kernel.org/linux-hwmon/20250903-ltc4283-support-v2-0-6bce091510bf@analog.com/

-- 
With Best Regards,
Andy Shevchenko

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

[Matti Vaittinen]

On 03/09/2025 14:23, Andy Shevchenko wrote:
> On Wed, Sep 03, 2025 at 09:52:02AM +0300, Matti Vaittinen wrote:
>> On 02/09/2025 17:15, Andy Shevchenko wrote:
>>> On Tue, Sep 02, 2025 at 03:24:31PM +0300, Matti Vaittinen wrote:
> 
> ...
> 
>>>> +	unsigned int vref_mv;
>>>
>>> Perhaps _mV to follow the actual unit spelling?
>>> (and yes, I know that both variants are present in the kernel)
>>
>> The 'mv' is a deliberate choise. I still remember our previous discussion
>> about this same topic [1].
> 
> Rings a bell. And I still think the proper spelling is more important when
> we talk about physics.
> 
> ...
> 
>> TLDR; I can combine the conditions in one if (), thanks.
> 
> Works for me.
> 
> ...
> 
>>>> +static int bd79112_gpio_dir_set(struct bd79112_data *data, unsigned int offset,
>>>> +				int dir)
>>>> +{
>>>> +	unsigned int set_reg, clear_reg, bit;
>>>> +	int ret;
>>>> +
>>>> +	bit = GET_GPIO_BIT(offset);
>>>> +
>>>> +	if (dir == GPIO_LINE_DIRECTION_IN) {
>>>> +		set_reg = GET_GPI_EN_REG(offset);
>>>> +		clear_reg = GET_GPO_EN_REG(offset);
>>>> +	} else {
>>>> +		set_reg = GET_GPO_EN_REG(offset);
>>>> +		clear_reg = GET_GPI_EN_REG(offset);
>>>> +	}
>>>
>>>> +	ret = regmap_set_bits(data->map, set_reg, bit);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	return regmap_clear_bits(data->map, clear_reg, bit);
>>>
>>> I believe the order depends on the out-in or in-out switch.
>>> Otherwise it might be potential glitches on input (hw) buffer.
>>> Right now when it's not an interrupt it may be okay to don't
>>> bother, but in general I see a potential issues with that.
>>
>> Can you please explain what you mean. I am not sure I can follow you here.
> 
> If we set out first with enabled input, input can get a value from the output.
> If the value before was different, it will be an event condition (of course
> it all depends on the HW and I haven't read datasheet for this one).

Ah. I guess I see what you mean. I have a feeling there is a problem at 
the GPIO user side, if it keeps listening for inputs after it calls 
direction to be switched to output.

But yes. I believe turning off the 'input listening' first, and toggling 
direction to output only afterwards makes sense. So, thanks. And thanks 
for explaining this further.

> ...
> 
>>>> +static int bd79112_probe(struct spi_device *spi)
>>>> +{
>>>> +	/* ADC channels as named in the data-sheet */
>>>> +	static const char * const chan_names[] = {
>>>> +		"AGIO0A", "AGIO1A", "AGIO2A", "AGIO3A", "AGIO4A", "AGIO5A",
>>>> +		"AGIO6A", "AGIO7A", "AGIO8A", "AGIO9A", "AGIO10A", "AGIO11A",
>>>> +		"AGIO11A", "AGIO12A", "AGIO13A", "AGIO14A", "AGIO15A",
>>>> +		"AGIO0B", "AGIO1B", "AGIO2B", "AGIO3B", "AGIO4B", "AGIO5B",
>>>> +		"AGIO6B", "AGIO7B", "AGIO8B", "AGIO9B", "AGIO10B", "AGIO11B",
>>>> +		"AGIO11B", "AGIO12B", "AGIO13B", "AGIO14B", "AGIO15B",
>>>
>>> Can you make all of the lines to be the same in terms of amount of entries?
>>
>> Maybe :) I would like to know why? As you know, I prefer to keep lines short
>> to fit multiple terminals in parallel, so this will probably make the entry
>> to consume more rows. Thus, I would like to have a solid reason.
> 
> Sure, the array above is unindexed. It's prone to errors and typos.

Ha. Thanks :) I see it now when I counted the entries :) Should be 32, 
was 34. I agree this would have been easier to spot!

> Moreover, it's really hard to follow in case one needs to debug such
> a typo and see which value needs to be fixed (imagine you typed twice
> the same name).

Or, if I typed twice the same name twice ;) Thanks!

> 
> Recommended way is to use power-of-two per line (and even add a comment
> at the end), like
> 
> static const char * const chan_names[] = {
> 	"AGIO0A", "AGIO1A", "AGIO2A", "AGIO3A",		/*  0 -  3 */
> 	"AGIO4A", "AGIO5A", "AGIO6A", "AGIO7A",		/*  4 -  7 */
> 	"AGIO8A", "AGIO9A", "AGIO10A", "AGIO11A",	/*  8 - 11 */
> 	...
> 
> (or hexadecimal offsets, whatever is better and more in accordance with
>   the SW / data sheet).

Ok, This makes sense now.

> 
>>>> +	};
> 
> ...
> 
>>>> +	data->vref_mv = ret / 1000;
>>>
>>> (MICRO / MILLI)
>>
>> I find this much more confusing than plain 1000. (I know we had this type of
>> discussion before. See [1] again).
> 
> Rings a bell, but that's what IIO reviewers suggest to do nowadays as a
> compromise between creating a new bunch of unit (V) related definitions.

I am sorry, but this just seems stupid to me. I'd say that it is very 
obvious for most of the readers dividing microvolts by 1000 results 
millivolts. And if it is not, then having this MICRO / MILLI is likely 
to just cause more confusion.

I _really_ dislike these defines. Why is MILLI 1000? Why it isn't 0.001? 
It makes no sense that KILO and MILLI are the same. Especially not when 
we are dealing with physics.

This is just an obfuscation compared to using plain 1000. (I kind of 
understand having a define for a value like 100000 - where counting the 
zeros gets cumbersome, although 100 * 1000 would be equally clear. But 
1000 _is_ really 100% clear, whereas MICRO / MILLI is not).

>>>> +	ret = devm_iio_adc_device_alloc_chaninfo_se(dev, &bd79112_chan_template,
>>>> +		BD79112_MAX_NUM_CHANNELS - 1, &cs);
>>>
>>> Hmm... Indentation can be amended.
>>
>> Sorry but I am not sure I understand what you mean by amended? Can you
>> please go an extra mile and explain :)
> 
> 	ret = devm_iio_adc_device_alloc_chaninfo_se(dev,
> 						    &bd79112_chan_template,
> 						    BD79112_MAX_NUM_CHANNELS - 1,
> 						    &cs);
> 

Ah, Ok. I'll do this, thanks.

> ...
> 
>>>> +	gpio_pins = bd79112_get_gpio_pins(iio_dev->channels,
>>>> +					  iio_dev->num_channels);
>>>
>>>> +
>>>
>>> Instead of leaving this rather unneeded blank line I would move above...
>>>
>>>> +	/* We're done if all channels are reserved for ADC. */
>>>
>>> ...to be here
>>>
>>> 	gpio_pins = bd79112_get_gpio_pins(iio_dev->channels,
>>> 					  iio_dev->num_channels);
>>
>> I suppose you mean something like:
>>
>> register_gpios:
>> 	/* We're done if all channels are reserved for ADC. */
>> 	gpio_pins = bd79112_get_gpio_pins(iio_dev->channels,
>>                                            iio_dev->num_channels);
>> 	if (!gpio_pins)
>> 		return 0;
>>
>> right?
> 
> Yes.
> 
>> I don't like this because now the comment suggests we do call
>> bd79112_get_gpio_pins() only to see if all channels were for ADCs. This,
>> however, is not THE reason for this call, only an optimization. I think:
>> having:
>>
>>          /* We're done if all channels are reserved for ADC. */
> 
> Then you can amend the comment
> 
>           /* If all channels are reserved for ADC, we are done. */
> 
>>          if (!gpio_pins)
>>                  return 0;
>>
>> is clearer.
> 
> Which makes my approach sustainable.

I like your wording better, but placing this comment before the call to 
bd79112_get_gpio_pins() is still more confusing that placing it before 
the actual check:
	if (!gpio_pins)
is still misleading. Comment applies to the check, not the retrieval.


Yours,
	-- Matti