[RFC PATCH 0/5] Support ROHM BD79124 ADC/GPO

[RFC PATCH 0/5] Support ROHM BD79124 ADC/GPO

[Matti Vaittinen]

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

Support ROHM BD79124 ADC.

Quite usual stuff. 12-bit, 8-channel ADC with threshold monitoring.

Except that:
 - each ADC input pin can be configured as a general purpose output.
 - manually starting an ADC conversion and reading the result would
   require the I2C _master_ to do clock stretching(!) for the duration
   of the conversion... Let's just say this is not well supported.
 - IC supports 'autonomous measurement mode' and storing latest results
   to the result registers. This mode is used by the driver due to the
   "peculiar" I2C when doing manual reads.

I sent this as an RFC because I implemented the pin purposing (GPO/ADC)
using pinmux - which I've never done for upstream stuff before. Hence
it's better to ask if this makes sense, or if there is better way to go.
Anyways, resulted drivers spread to 3 subsystems (MFD, pinctrl and IIO).

Furthermore, the GPO functionality has not been (properly) tested. I'll
do more testing for v2 if this pinmux approach is appropriate.

Furthermore, because the ADC uses this continuous autonomous measuring,
and because the IC keeps producing new 'out of window' IRQs if
measurements are out of window - the driver disables the event when
sending one. This prevents generating storm of events, but it also
requires users to reconfigure / re-enable an event if they wish to
continue monitoring after receiving one. Again I am not sure if this is
the best way to handle such HW - so better to ask for an opinion than a
nose bleed, right? Maybe the next version will no longer be a RFC :)

---

Matti Vaittinen (5):
  dt-bindings: ROHM BD79124 ADC/GPO
  mfd: Add ROHM BD79124 ADC/GPO
  iio: adc: Support ROHM BD79124 ADC
  pinctrl: Support ROHM BD79124 pinmux / GPO
  MAINTAINERS: Add ROHM BD79124 ADC/GPO

 .../devicetree/bindings/mfd/rohm,bd79124.yaml | 111 +++
 MAINTAINERS                                   |   8 +
 drivers/iio/adc/Kconfig                       |  10 +
 drivers/iio/adc/Makefile                      |   1 +
 drivers/iio/adc/rohm-bd79124-adc.c            | 890 ++++++++++++++++++
 drivers/mfd/Kconfig                           |  12 +
 drivers/mfd/Makefile                          |   1 +
 drivers/mfd/rohm-bd79124.c                    | 165 ++++
 drivers/pinctrl/Kconfig                       |  11 +
 drivers/pinctrl/Makefile                      |   1 +
 drivers/pinctrl/pinctrl-bd79124.c             | 276 ++++++
 include/linux/mfd/rohm-bd79124.h              |  32 +
 12 files changed, 1518 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/rohm,bd79124.yaml
 create mode 100644 drivers/iio/adc/rohm-bd79124-adc.c
 create mode 100644 drivers/mfd/rohm-bd79124.c
 create mode 100644 drivers/pinctrl/pinctrl-bd79124.c
 create mode 100644 include/linux/mfd/rohm-bd79124.h


base-commit: 5bc55a333a2f7316b58edc7573e8e893f7acb532
-- 
2.48.1


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

[RFC PATCH 1/5] dt-bindings: ROHM BD79124 ADC/GPO

[Matti Vaittinen]

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

Add binding document for the ROHM BD79124 ADC / GPO.

ROHM BD79124 is a 8-channel, 12-bit ADC. The input pins can also be used
as general purpose outputs. Switching between ADC and GPO is described
using as a pinmux.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
 .../devicetree/bindings/mfd/rohm,bd79124.yaml | 111 ++++++++++++++++++
 1 file changed, 111 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/rohm,bd79124.yaml

diff --git a/Documentation/devicetree/bindings/mfd/rohm,bd79124.yaml b/Documentation/devicetree/bindings/mfd/rohm,bd79124.yaml
new file mode 100644
index 000000000000..0d2958e82780
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/rohm,bd79124.yaml
@@ -0,0 +1,111 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/rohm,bd79124.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ROHM BD79124 ADC/GPO
+
+maintainers:
+  - Matti Vaittinen <mazziesaccount@gmail.com>
+
+description: |
+  The ROHM BD79124 is a 12-bit, 8-channel, SAR ADC. The ADC supports
+  an automatic measurement mode, with an alarm interrupt for out-of-window
+  measurements. ADC input pins can be also configured as general purpose
+  outputs.
+
+properties:
+  compatible:
+    const: rohm,bd79124
+
+  reg:
+    description:
+      I2C slave address.
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  gpio-controller: true
+
+  "#gpio-cells":
+    const: 1
+    description:
+      The pin number.
+
+  vdd-supply: true
+
+  iovdd-supply: true
+
+patternProperties:
+  "^channel@[0-9a-f]+$":
+    type: object
+    $ref: /schemas/iio/adc/adc.yaml#
+    description: Represents ADC channel.
+
+    properties:
+      reg:
+        items:
+          minimum: 1
+          maximum: 8
+
+    required:
+      - reg
+
+    additionalProperties: false
+
+  "-pins$":
+    type: object
+    $ref: /schemas/pinctrl/pinmux-node.yaml
+
+    properties:
+      function:
+        enum: [gpo, adc]
+
+      groups:
+        description:
+          List of pin groups affected by the functions specified in this
+          subnode.
+        items:
+          enum: [ain0, ain1, ain2, ain3, ain4, ain5, ain6, ain7]
+
+      pins:
+        items:
+          enum: [ain0, ain1, ain2, ain3, ain4, ain5, ain6, ain7]
+
+    additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - iovdd-supply
+  - vdd-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/leds/common.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        adc: adc@10 {
+            compatible = "rohm,bd79124";
+            reg = <0x10>;
+            interrupt-parent = <&gpio1>;
+            interrupts = <29 8>;
+            vdd-supply = <&dummyreg>;
+            iovdd-supply = <&dummyreg>;
+
+            adcpins: adc-pins {
+                pins = "ain0", "ain1", "ain5", "ain7";
+                function = "adc";
+            };
+            gpopins: gpo-pins {
+                groups = "ain2", "ain3", "ain4", "ain6";
+                function = "gpo";
+            };
+        };
+    };
-- 
2.48.1


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

[RFC PATCH 2/5] mfd: Add ROHM BD79124 ADC/GPO

[Matti Vaittinen]

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

Add core driver for the ROHM BD79124 ADC / GPO.

The core driver launches the sub-drivers for the pinmux/GPO and for the
IIO ADC. It also provides the regmap, and forwards the IRQ resource to
the ADC.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
 drivers/mfd/Kconfig              |  12 +++
 drivers/mfd/Makefile             |   1 +
 drivers/mfd/rohm-bd79124.c       | 165 +++++++++++++++++++++++++++++++
 include/linux/mfd/rohm-bd79124.h |  32 ++++++
 4 files changed, 210 insertions(+)
 create mode 100644 drivers/mfd/rohm-bd79124.c
 create mode 100644 include/linux/mfd/rohm-bd79124.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index ae23b317a64e..f024256fb180 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -2113,6 +2113,18 @@ config MFD_ROHM_BD71828
 	  also a single-cell linear charger, a Coulomb counter, a real-time
 	  clock (RTC), GPIOs and a 32.768 kHz clock gate.
 
+config MFD_ROHM_BD79124
+	tristate "Rohm BD79124 core driver"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  Say yes here to build support for the ROHM BD79124 ADC core. The
+	  ROHM BD79124 is a 12-bit, 8-channel, SAR ADC. The ADC supports
+	  also an automatic measurement mode, with an alarm interrupt for
+	  out-of-window measurements. The window is configurable for each
+	  channel. The ADC inputs can optionally be used as general purpose
+	  outputs.
+
 config MFD_ROHM_BD957XMUF
 	tristate "ROHM BD9576MUF and BD9573MUF Power Management ICs"
 	depends on I2C=y
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index e057d6d6faef..c7d64e933a7d 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -270,6 +270,7 @@ obj-$(CONFIG_MFD_SC27XX_PMIC)	+= sprd-sc27xx-spi.o
 obj-$(CONFIG_RAVE_SP_CORE)	+= rave-sp.o
 obj-$(CONFIG_MFD_ROHM_BD71828)	+= rohm-bd71828.o
 obj-$(CONFIG_MFD_ROHM_BD718XX)	+= rohm-bd718x7.o
+obj-$(CONFIG_MFD_ROHM_BD79124)	+= rohm-bd79124.o
 obj-$(CONFIG_MFD_ROHM_BD957XMUF)	+= rohm-bd9576.o
 obj-$(CONFIG_MFD_ROHM_BD96801)	+= rohm-bd96801.o
 obj-$(CONFIG_MFD_STMFX) 	+= stmfx.o
diff --git a/drivers/mfd/rohm-bd79124.c b/drivers/mfd/rohm-bd79124.c
new file mode 100644
index 000000000000..c35ab0e03b0b
--- /dev/null
+++ b/drivers/mfd/rohm-bd79124.c
@@ -0,0 +1,165 @@
+// SPDX-License-Identifier: GPL-2.0-only
+//
+// Copyright (C) 2025 ROHM Semiconductors
+//
+// ROHM BD79124 ADC / GPO driver
+
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+
+#include <linux/mfd/core.h>
+#include <linux/mfd/rohm-bd79124.h>
+
+static struct resource adc_alert;
+
+enum {
+	CELL_PINMUX,
+	CELL_ADC,
+};
+
+static struct mfd_cell bd79124_cells[] = {
+	[CELL_PINMUX]	= { .name = "bd79124-pinmux", },
+	[CELL_ADC]	= { .name = "bd79124-adc", },
+};
+
+/* Read-only regs */
+static const struct regmap_range bd79124_ro_ranges[] = {
+	{
+		.range_min = BD79124_REG_EVENT_FLAG,
+		.range_max = BD79124_REG_EVENT_FLAG,
+	}, {
+		.range_min = BD79124_REG_RECENT_CH0_LSB,
+		.range_max = BD79124_REG_RECENT_CH7_MSB,
+	},
+};
+
+static const struct regmap_access_table bd79124_ro_regs = {
+	.no_ranges	= &bd79124_ro_ranges[0],
+	.n_no_ranges	= ARRAY_SIZE(bd79124_ro_ranges),
+};
+
+static const struct regmap_range bd79124_volatile_ranges[] = {
+	{
+		.range_min = BD79124_REG_RECENT_CH0_LSB,
+		.range_max = BD79124_REG_RECENT_CH7_MSB,
+	}, {
+		.range_min = BD79124_REG_EVENT_FLAG,
+		.range_max = BD79124_REG_EVENT_FLAG,
+	}, {
+		.range_min = BD79124_REG_EVENT_FLAG_HI,
+		.range_max = BD79124_REG_EVENT_FLAG_HI,
+	}, {
+		.range_min = BD79124_REG_EVENT_FLAG_LO,
+		.range_max = BD79124_REG_EVENT_FLAG_LO,
+	}, {
+		.range_min = BD79124_REG_SYSTEM_STATUS,
+		.range_max = BD79124_REG_SYSTEM_STATUS,
+	},
+};
+
+static const struct regmap_access_table bd79124_volatile_regs = {
+	.yes_ranges	= &bd79124_volatile_ranges[0],
+	.n_yes_ranges	= ARRAY_SIZE(bd79124_volatile_ranges),
+};
+
+static const struct regmap_range bd79124_precious_ranges[] = {
+	{
+		.range_min = BD79124_REG_EVENT_FLAG_HI,
+		.range_max = BD79124_REG_EVENT_FLAG_HI,
+	}, {
+		.range_min = BD79124_REG_EVENT_FLAG_LO,
+		.range_max = BD79124_REG_EVENT_FLAG_LO,
+	},
+};
+
+static const struct regmap_access_table bd79124_precious_regs = {
+	.yes_ranges	= &bd79124_precious_ranges[0],
+	.n_yes_ranges	= ARRAY_SIZE(bd79124_precious_ranges),
+};
+
+static const struct regmap_config bd79124_regmap = {
+	.reg_bits		= 16,
+	.val_bits		= 8,
+	.read_flag_mask		= BD79124_I2C_MULTI_READ,
+	.write_flag_mask	= BD79124_I2C_MULTI_WRITE,
+	.max_register		= BD79124_REG_MAX,
+	.cache_type		= REGCACHE_MAPLE,
+	.volatile_table		= &bd79124_volatile_regs,
+	.wr_table		= &bd79124_ro_regs,
+	.precious_table		= &bd79124_precious_regs,
+};
+
+static int bd79124_probe(struct i2c_client *i2c)
+{
+	int ret;
+	struct regmap *map;
+	struct device *dev = &i2c->dev;
+	int *adc_vref;
+
+	adc_vref = devm_kzalloc(dev, sizeof(*adc_vref), GFP_KERNEL);
+	if (!adc_vref)
+		return -ENOMEM;
+
+	/*
+	 * Better to enable regulators here so we don't need to worry about the
+	 * order of sub-device instantiation. We also need to deliver the
+	 * reference voltage value to the ADC driver. This is done via
+	 * the MFD driver's drvdata.
+	 */
+	*adc_vref = devm_regulator_get_enable_read_voltage(dev, "vdd");
+	if (*adc_vref < 0)
+		return dev_err_probe(dev, ret, "Failed to get the Vdd\n");
+
+	dev_set_drvdata(dev, adc_vref);
+
+	ret = devm_regulator_get_enable(dev, "iovdd");
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Failed to enable I/O voltage\n");
+
+	map = devm_regmap_init_i2c(i2c, &bd79124_regmap);
+	if (IS_ERR(map))
+		return dev_err_probe(dev, PTR_ERR(map),
+				     "Failed to initialize Regmap\n");
+
+	if (i2c->irq) {
+		adc_alert = DEFINE_RES_IRQ_NAMED(i2c->irq, "thresh-alert");
+		bd79124_cells[CELL_ADC].resources = &adc_alert;
+		bd79124_cells[CELL_ADC].num_resources = 1;
+	}
+
+	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, bd79124_cells,
+				   ARRAY_SIZE(bd79124_cells), NULL, 0, NULL);
+	if (ret)
+		dev_err_probe(dev, ret, "Failed to create subdevices\n");
+
+	return ret;
+}
+
+static const struct of_device_id bd79124_of_match[] = {
+	{ .compatible = "rohm,bd79124" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, bd79124_of_match);
+
+static const struct i2c_device_id bd79124_id[] = {
+	{ "bd79124", },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, bd79124_id);
+
+static struct i2c_driver bd79124_driver = {
+	.driver = {
+		.name = "bd79124",
+		.of_match_table = bd79124_of_match,
+	},
+	.probe = bd79124_probe,
+	.id_table = bd79124_id,
+};
+module_i2c_driver(bd79124_driver);
+
+MODULE_AUTHOR("Matti Vaittinen <mazziesaccount@gmail.com>");
+MODULE_DESCRIPTION("Core Driver for ROHM BD79124");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/rohm-bd79124.h b/include/linux/mfd/rohm-bd79124.h
new file mode 100644
index 000000000000..505faeb6f135
--- /dev/null
+++ b/include/linux/mfd/rohm-bd79124.h
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright 2021 ROHM Semiconductors.
+ *
+ * Author: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
+ */
+
+#ifndef _MFD_BD79124_H
+#define _MFD_BD79124_H
+
+#define BD79124_I2C_MULTI_READ		0x30
+#define BD79124_I2C_MULTI_WRITE		0x28
+#define BD79124_REG_MAX			0xaf
+
+#define BD79124_REG_SYSTEM_STATUS	0x0
+#define BD79124_REG_GEN_CFG		0x01
+#define BD79124_REG_OPMODE_CFG		0x04
+#define BD79124_REG_PINCFG		0x05
+#define BD79124_REG_GPO_VAL		0x06
+#define BD79124_REG_SEQUENCE_CFG	0x10
+#define BD79124_REG_MANUAL_CHANNELS	0x11
+#define BD79124_REG_AUTO_CHANNELS	0x12
+#define BD79124_REG_ALERT_CH_SEL	0x14
+#define BD79124_REG_EVENT_FLAG		0x18
+#define BD79124_REG_EVENT_FLAG_HI	0x1a
+#define BD79124_REG_EVENT_FLAG_LO	0x1c
+#define BD79124_REG_HYSTERESIS_CH0	0x20
+#define BD79124_REG_EVENTCOUNT_CH0	0x22
+#define BD79124_REG_RECENT_CH0_LSB	0xa0
+#define BD79124_REG_RECENT_CH7_MSB	0xaf
+
+#endif
-- 
2.48.1


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

[RFC PATCH 3/5] iio: adc: Support ROHM BD79124 ADC

[Matti Vaittinen]

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

The ROHM BD79124 is a 12-bit, 8-channel, SAR ADC. The ADC supports
an automatic measurement mode, with an alarm interrupt for out-of-window
measurements. The window is configurable for each channel.

The I2C protocol for manual start of the measurement and data reading is
somewhat peculiar. It requires the master to do clock stretching after
sending the I2C slave-address until the slave has captured the data.
Needless to say this is not well suopported by the I2C controllers.

Thus the driver does not support the BD79124's manual measurement mode
but implements the measurements using automatic measurement mode relying
on the BD79124's ability of storing latest measurements into register.

The driver does also support configuring the threshold events for
detecting the out-of-window events.

The BD79124 keeps asserting IRQ for as long as the measured voltage is
out of the configured window. Thus the driver disables the event when
first event is handled. This prevents the user-space from choking on the
events - but it also requires the user space to reconfigure and
re-enable the monitored event when it wants to keep monitoring for new
occurrences.

It is worth noting that the ADC input pins can be also configured as
general purpose outputs. The pin mode should be configured using pincmux
driver.

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

---

Regarding disabling the event upon reception - is this totally strange?
Is regular userspace compeletely unprepared for this, and better
prepared for handling large amounts of continuous events?

The BD79124 should not cause a total CPU-blocking IRQ storm because the
driver uses the autonomous sequencer mode - which has minimum of 0.75
millisecond delay between measurements. So, new IRQs can be raised with
this interval. (The 0.75 mS includes handling and acking / status reading
delays - so there is still not much time for things done outside of the
IRQ handling...)
---
 drivers/iio/adc/Kconfig            |  10 +
 drivers/iio/adc/Makefile           |   1 +
 drivers/iio/adc/rohm-bd79124-adc.c | 890 +++++++++++++++++++++++++++++
 3 files changed, 901 insertions(+)
 create mode 100644 drivers/iio/adc/rohm-bd79124-adc.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 849c90203071..195a61ba5cf4 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1188,6 +1188,16 @@ config RN5T618_ADC
 	  This driver can also be built as a module. If so, the module
 	  will be called rn5t618-adc.
 
+config ROHM_BD79124
+	tristate "Rohm BD79124 ADC driver"
+	depends on MFD_ROHM_BD79124
+	help
+	  Say yes here to build support for the ROHM BD79124 ADC. The
+	  ROHM BD79124 is a 12-bit, 8-channel, SAR ADC. The ADC supports
+	  also an automatic measurement mode, with an alarm interrupt for
+	  out-of-window measurements. The window is configurable for each
+	  channel.
+
 config ROCKCHIP_SARADC
 	tristate "Rockchip SARADC driver"
 	depends on ARCH_ROCKCHIP || COMPILE_TEST
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index ee19afba62b7..7049d984682d 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -107,6 +107,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_BD79124) += rohm-bd79124-adc.o
 obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
 obj-$(CONFIG_RZG2L_ADC) += rzg2l_adc.o
 obj-$(CONFIG_SC27XX_ADC) += sc27xx_adc.o
diff --git a/drivers/iio/adc/rohm-bd79124-adc.c b/drivers/iio/adc/rohm-bd79124-adc.c
new file mode 100644
index 000000000000..7c95a1de1e71
--- /dev/null
+++ b/drivers/iio/adc/rohm-bd79124-adc.c
@@ -0,0 +1,890 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * ROHM ADC driver for BD79124 ADC/GPO device
+ * https://fscdn.rohm.com/en/products/databook/datasheet/ic/data_converter/dac/bd79124muf-c-e.pdf
+ *
+ * Copyright (c) 2025, ROHM Semiconductor.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/byteorder/generic.h>
+#include <linux/device.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/irqreturn.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/types.h>
+
+#include <linux/iio/events.h>
+#include <linux/iio/iio.h>
+#include <linux/mfd/rohm-bd79124.h>
+
+#define BD79124_ADC_BITS 12
+#define BD79124_MASK_CONV_MODE GENMASK(6, 5)
+#define BD79124_MASK_AUTO_INTERVAL GENMASK(1, 0)
+#define BD79124_CONV_MODE_MANSEQ 0
+#define BD79124_CONV_MODE_AUTO 1
+#define BD79124_INTERVAL_075 0
+#define BD79124_INTERVAL_150 1
+#define BD79124_INTERVAL_300 2
+#define BD79124_INTERVAL_600 3
+
+#define BD79124_MASK_DWC_EN BIT(4)
+#define BD79124_MASK_STATS_EN BIT(5)
+#define BD79124_MASK_SEQ_START BIT(4)
+#define BD79124_MASK_SEQ_MODE GENMASK(1, 0)
+#define BD79124_MASK_SEQ_MANUAL 0
+#define BD79124_MASK_SEQ_SEQ 1
+
+#define BD79124_MASK_HYSTERESIS GENMASK(3, 0)
+#define BD79124_LOW_LIMIT_MIN 0
+#define BD79124_HIGH_LIMIT_MAX GENMASK(11, 0)
+
+/*
+ * The high limit, low limit and last measurement result are each stored in
+ * 2 consequtive registers. 4 bits are in the high bits of the 1.st register
+ * and 8 bits in the next register.
+ *
+ * These macros return the address of the 1.st reg for the given channel
+ */
+#define BD79124_GET_HIGH_LIMIT_REG(ch) (BD79124_REG_HYSTERESIS_CH0 + (ch) * 4)
+#define BD79124_GET_LOW_LIMIT_REG(ch) (BD79124_REG_EVENTCOUNT_CH0 + (ch) * 4)
+#define BD79124_GET_RECENT_RES_REG(ch) (BD79124_REG_RECENT_CH0_LSB + (ch) * 2)
+
+/*
+ * The hysteresis for a channel is stored in the same register where the
+ * 4 bits of high limit reside.
+ */
+#define BD79124_GET_HYSTERESIS_REG(ch) BD79124_GET_HIGH_LIMIT_REG(ch)
+
+enum {
+	BD79124_CH_0,
+	BD79124_CH_1,
+	BD79124_CH_2,
+	BD79124_CH_3,
+	BD79124_CH_4,
+	BD79124_CH_5,
+	BD79124_CH_6,
+	BD79124_CH_7,
+	BD79124_MAX_NUM_CHANNELS
+};
+
+struct bd79124_data {
+	s64 timestamp;
+	struct regmap *map;
+	struct device *dev;
+	int vmax;
+	int alarm_monitored[BD79124_MAX_NUM_CHANNELS];
+	/*
+	 * The BD79124 is configured to run the measurements in the background.
+	 * This is done for the event monitoring as well as for the read_raw().
+	 * Protect the measurement starting/stopping using a mutex.
+	 */
+	struct mutex mutex;
+};
+
+struct bd79124_raw {
+	u8 bit0_3; /* Is set in high bits of the byte */
+	u8 bit4_11;
+};
+#define BD79124_RAW_TO_INT(r) ((r.bit4_11 << 4) | (r.bit0_3 >> 4))
+
+/*
+ * The high and low limits as well as the recent result values are stored in
+ * the same way in 2 consequent registers. The first register contains 4 bits
+ * of the value. These bits are stored in the high bits [7:4] of register, but
+ * they represent the low bits [3:0] of the value.
+ * The value bits [11:4] are stored in the next register.
+ *
+ * Read data from register and convert to integer.
+ */
+static int bd79124_read_reg_to_int(struct bd79124_data *d, int reg,
+				   unsigned int *val)
+{
+	int ret;
+	struct bd79124_raw raw;
+
+	ret = regmap_bulk_read(d->map, reg, &raw, sizeof(raw));
+	if (ret)
+		dev_dbg(d->dev, "bulk_read failed %d\n", ret);
+	*val = BD79124_RAW_TO_INT(raw);
+
+	return ret;
+}
+
+/*
+ * The high and low limits as well as the recent result values are stored in
+ * the same way in 2 consequent registers. The first register contains 4 bits
+ * of the value. These bits are stored in the high bits [7:4] of register, but
+ * they represent the low bits [3:0] of the value.
+ * The value bits [11:4] are stored in the next regoster.
+ *
+ * Conver the integer to register format and write it using rmw cycle.
+ */
+static int bd79124_write_int_to_reg(struct bd79124_data *d, int reg,
+				    unsigned int val)
+{
+	struct bd79124_raw raw;
+	int ret, tmp;
+
+	raw.bit4_11 = (u8)(val >> 4);
+	raw.bit0_3 = (u8)(val << 4);
+
+	ret = regmap_read(d->map, reg, &tmp);
+	if (ret)
+		return ret;
+
+	raw.bit0_3 |= (0xf & tmp);
+
+	return regmap_bulk_write(d->map, reg, &raw, sizeof(raw));
+}
+
+static const struct iio_event_spec bd79124_events[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
+				 BIT(IIO_EV_INFO_ENABLE),
+	},
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
+				 BIT(IIO_EV_INFO_ENABLE),
+	},
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_EITHER,
+		.mask_separate = BIT(IIO_EV_INFO_HYSTERESIS),
+	},
+};
+
+#define BD79124_CHAN(idx) {						\
+	.type = IIO_VOLTAGE,						\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),		\
+	.indexed = 1,							\
+	.channel = idx,							\
+}
+
+#define BD79124_CHAN_EV(idx) {						\
+	.type = IIO_VOLTAGE,						\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),		\
+	.indexed = 1,							\
+	.channel = idx,							\
+	.event_spec = bd79124_events,					\
+	.num_event_specs = ARRAY_SIZE(bd79124_events),			\
+}
+
+static const struct iio_chan_spec bd79124_channels[] = {
+	BD79124_CHAN_EV(0),
+	BD79124_CHAN_EV(1),
+	BD79124_CHAN_EV(2),
+	BD79124_CHAN_EV(3),
+	BD79124_CHAN_EV(4),
+	BD79124_CHAN_EV(5),
+	BD79124_CHAN_EV(6),
+	BD79124_CHAN_EV(7),
+};
+
+static const struct iio_chan_spec bd79124_channels_noirq[] = {
+	BD79124_CHAN(0),
+	BD79124_CHAN(1),
+	BD79124_CHAN(2),
+	BD79124_CHAN(3),
+	BD79124_CHAN(4),
+	BD79124_CHAN(5),
+	BD79124_CHAN(6),
+	BD79124_CHAN(7),
+};
+
+/*
+ * The BD79124 supports muxing the pins as ADC inputs or as a general purpose
+ * output. This muxing is handled by a pinmux driver. Here we just check the
+ * settings from the register, and disallow using the pin if pinmux is set to
+ * GPO.
+ *
+ * NOTE: This driver does not perform any locking to ensure the pinmux stays
+ * toggled to ADC for the duration of the whatever operation is being done.
+ * It is responsibility of the user to configure the pinmux.
+ */
+static bool bd79124_chan_is_adc(struct bd79124_data *d, unsigned int offset)
+{
+	int ret, val;
+
+	ret = regmap_read(d->map, BD79124_REG_PINCFG, &val);
+	/* If read fails, don't allow using as AIN (to be on a safe side) */
+	if (ret)
+		return 0;
+
+	return !(val & BIT(offset));
+}
+
+static int bd79124_read_event_value(struct iio_dev *idev,
+				    const struct iio_chan_spec *chan,
+				    enum iio_event_type type,
+				    enum iio_event_direction dir,
+				    enum iio_event_info info, int *val,
+				    int *val2)
+{
+	struct bd79124_data *d = iio_priv(idev);
+	int ret, reg;
+
+	if (chan->channel >= BD79124_MAX_NUM_CHANNELS)
+		return -EINVAL;
+
+	/* ensure pinmux is set to ADC */
+	if (!bd79124_chan_is_adc(d, chan->channel))
+		return -EBUSY;
+
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		if (dir == IIO_EV_DIR_RISING)
+			reg = BD79124_GET_HIGH_LIMIT_REG(chan->channel);
+		else if (dir == IIO_EV_DIR_FALLING)
+			reg = BD79124_GET_LOW_LIMIT_REG(chan->channel);
+		else
+			return -EINVAL;
+
+		ret = bd79124_read_reg_to_int(d, reg, val);
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;
+
+	case IIO_EV_INFO_HYSTERESIS:
+		reg = BD79124_GET_HYSTERESIS_REG(chan->channel);
+		ret = regmap_read(d->map, reg, val);
+		if (ret)
+			return ret;
+		/* Mask the non hysteresis bits */
+		*val &= BD79124_MASK_HYSTERESIS;
+		/*
+		 * The data-sheet says the hysteresis register value needs to be
+		 * sifted left by 3 (or multiplied by 8, depending on the
+		 * page :] )
+		 */
+		*val <<= 3;
+
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int bd79124_start_measurement(struct bd79124_data *d, int chan)
+{
+	int val, ret, regval;
+
+	/* ensure pinmux is set to ADC */
+	if (!bd79124_chan_is_adc(d, chan))
+		return -EBUSY;
+
+	/* See if already started */
+	ret = regmap_read(d->map, BD79124_REG_AUTO_CHANNELS, &val);
+	if (val & BIT(chan))
+		return 0;
+
+	/* Stop the sequencer */
+	ret = regmap_clear_bits(d->map, BD79124_REG_SEQUENCE_CFG,
+				BD79124_MASK_SEQ_START);
+	if (ret)
+		return ret;
+
+	/* Add the channel to measured channels */
+	ret = regmap_write(d->map, BD79124_REG_AUTO_CHANNELS, val | BIT(chan));
+	if (ret)
+		return ret;
+
+	/* Restart the sequencer */
+	ret = regmap_set_bits(d->map, BD79124_REG_SEQUENCE_CFG,
+			      BD79124_MASK_SEQ_START);
+	if (ret)
+		return ret;
+
+	/*
+	 * Start the measurement at the background. Don't bother checking if
+	 * it was started, regmap has cache
+	 */
+	regval = FIELD_PREP(BD79124_MASK_CONV_MODE, BD79124_CONV_MODE_AUTO);
+
+	return regmap_update_bits(d->map, BD79124_REG_OPMODE_CFG,
+				BD79124_MASK_CONV_MODE, regval);
+}
+
+static int bd79124_stop_measurement(struct bd79124_data *d, int chan)
+{
+	int val, ret;
+
+	/* Ensure pinmux is set to ADC */
+	if (!bd79124_chan_is_adc(d, chan))
+		return -EBUSY;
+
+	/* If alarm is requested for the channel we won't stop measurement */
+	if (d->alarm_monitored[chan])
+		return 0;
+
+	/* See if already stopped */
+	ret = regmap_read(d->map, BD79124_REG_AUTO_CHANNELS, &val);
+	if (!(val & BIT(chan)))
+		return 0;
+
+	/* Stop the sequencer */
+	ret = regmap_clear_bits(d->map, BD79124_REG_SEQUENCE_CFG,
+				BD79124_MASK_SEQ_START);
+
+	/* Clear the channel from the measured channels */
+	ret = regmap_write(d->map, BD79124_REG_AUTO_CHANNELS,
+			   (~BIT(chan)) & val);
+	if (ret)
+		return ret;
+
+	/* Stop background conversion if it was the last channel */
+	if (!((~BIT(chan)) & val)) {
+		int regval = FIELD_PREP(BD79124_MASK_CONV_MODE,
+					BD79124_CONV_MODE_MANSEQ);
+
+		ret = regmap_update_bits(d->map, BD79124_REG_OPMODE_CFG,
+					 BD79124_MASK_CONV_MODE, regval);
+		if (ret)
+			return ret;
+	}
+
+	/* Restart the sequencer */
+	return regmap_set_bits(d->map, BD79124_REG_SEQUENCE_CFG,
+			       BD79124_MASK_SEQ_START);
+}
+
+static int bd79124_read_event_config(struct iio_dev *idev,
+				     const struct iio_chan_spec *chan,
+				     enum iio_event_type type,
+				     enum iio_event_direction dir)
+{
+	struct bd79124_data *d = iio_priv(idev);
+	int val, ret, reg, disabled;
+
+	if (chan->channel >= BD79124_MAX_NUM_CHANNELS)
+		return -EINVAL;
+
+	/* ensure pinmux is set to ADC */
+	if (!bd79124_chan_is_adc(d, chan->channel))
+		return -EBUSY;
+
+	ret = regmap_read(d->map, BD79124_REG_ALERT_CH_SEL, &val);
+	if (ret)
+		return ret;
+
+	/* The event is disabled if alerts for the channel are disabled */
+	if (!(val & BIT(chan->channel)))
+		return 0;
+
+	/*
+	 * If alerts are on, then the event may be disabled if limit is set to
+	 * the one extreme. (HW does not support disabling rising/falling
+	 * thresholds independently. Hence we resort to setting high limit to
+	 * MAX, or low limit to 0 to try effectively disable thresholds).
+	 */
+	if (dir == IIO_EV_DIR_RISING) {
+		reg = BD79124_GET_HIGH_LIMIT_REG(chan->channel);
+		disabled = BD79124_HIGH_LIMIT_MAX;
+	} else if (dir == IIO_EV_DIR_FALLING) {
+		reg = BD79124_GET_LOW_LIMIT_REG(chan->channel);
+		disabled = BD79124_LOW_LIMIT_MIN;
+	} else {
+		return -EINVAL;
+	}
+	ret = bd79124_read_reg_to_int(d, reg, &val);
+	if (ret)
+		return ret;
+
+	return val != disabled;
+}
+
+static int bd79124_disable_event(struct bd79124_data *d,
+				 enum iio_event_direction dir, int channel)
+{
+	int dir_bit = BIT(dir), reg;
+	unsigned int limit;
+
+	d->alarm_monitored[channel] &= (~dir_bit);
+	/*
+	 * Set thresholds either to 0 or to 2^12 - 1 as appropriate to prevent
+	 * alerts and thus disable event generation.
+	 */
+	if (dir == IIO_EV_DIR_RISING) {
+		reg = BD79124_GET_HIGH_LIMIT_REG(channel);
+		limit = BD79124_HIGH_LIMIT_MAX;
+	} else if (dir == IIO_EV_DIR_FALLING) {
+		reg = BD79124_GET_LOW_LIMIT_REG(channel);
+		limit = BD79124_LOW_LIMIT_MIN;
+	} else {
+		return -EINVAL;
+	}
+
+	/*
+	 * Stop measurement if there is no more events to monitor.
+	 * We don't bother checking the retval because the limit
+	 * setting should in any case effectively disable the alarm.
+	 */
+	if (!d->alarm_monitored[channel]) {
+		bd79124_stop_measurement(d, channel);
+		regmap_clear_bits(d->map, BD79124_REG_ALERT_CH_SEL,
+			       BIT(channel));
+	}
+
+	return bd79124_write_int_to_reg(d, reg, limit);
+}
+
+/* Do we need to disable the measurement for the duration of the limit conf ? */
+static int bd79124_write_event_config(struct iio_dev *idev,
+				      const struct iio_chan_spec *chan,
+				      enum iio_event_type type,
+				      enum iio_event_direction dir, bool state)
+{
+	struct bd79124_data *d = iio_priv(idev);
+	int dir_bit = BIT(dir);
+
+	guard(mutex)(&d->mutex);
+
+	if (chan->channel >= BD79124_MAX_NUM_CHANNELS)
+		return -EINVAL;
+
+	/* ensure pinmux is set to ADC */
+	if (!bd79124_chan_is_adc(d, chan->channel))
+		return -EBUSY;
+
+	if (state) {
+		int ret;
+
+		/* Set channel to be measured */
+		ret = bd79124_start_measurement(d, chan->channel);
+		if (ret)
+			return ret;
+
+		d->alarm_monitored[chan->channel] |= dir_bit;
+
+		/* Add the channel to the list of monitored channels */
+		ret = regmap_set_bits(d->map, BD79124_REG_ALERT_CH_SEL,
+				      BIT(chan->channel));
+		if (ret)
+			return ret;
+
+		/*
+		 * Enable comparator. Trust the regmap cache, no need to check
+		 * if it was already enabled.
+		 *
+		 * We could do this in the hw-init, but there may be users who
+		 * never enable alarms and for them it makes sense to not
+		 * enable the comparator at probe.
+		 */
+		return regmap_set_bits(d->map, BD79124_REG_GEN_CFG,
+				      BD79124_MASK_DWC_EN);
+	}
+
+	return bd79124_disable_event(d, dir, chan->channel);
+}
+
+static int bd79124_write_event_value(struct iio_dev *idev,
+				     const struct iio_chan_spec *chan,
+				     enum iio_event_type type,
+				     enum iio_event_direction dir,
+				     enum iio_event_info info, int val,
+				     int val2)
+{
+	struct bd79124_data *d = iio_priv(idev);
+	int reg;
+
+	if (chan->channel >= BD79124_MAX_NUM_CHANNELS)
+		return -EINVAL;
+
+	/* ensure pinmux is set to ADC */
+	if (!bd79124_chan_is_adc(d, chan->channel))
+		return -EBUSY;
+
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		if (dir == IIO_EV_DIR_RISING)
+			reg = BD79124_GET_HIGH_LIMIT_REG(chan->channel);
+		else if (dir == IIO_EV_DIR_FALLING)
+			reg = BD79124_GET_LOW_LIMIT_REG(chan->channel);
+		else
+			return -EINVAL;
+
+		return bd79124_write_int_to_reg(d, reg, val);
+
+	case IIO_EV_INFO_HYSTERESIS:
+			reg = BD79124_GET_HYSTERESIS_REG(chan->channel);
+			val >>= 3;
+
+		return regmap_update_bits(d->map, reg, BD79124_MASK_HYSTERESIS,
+					  val);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int bd79124_read_last_result(struct bd79124_data *d, int chan,
+				    int *result)
+{
+	struct bd79124_raw raw;
+	int ret;
+
+	ret = regmap_bulk_read(d->map, BD79124_GET_RECENT_RES_REG(chan), &raw,
+			       sizeof(raw));
+	if (ret)
+		return ret;
+
+	*result = BD79124_RAW_TO_INT(raw);
+
+	return 0;
+}
+
+static int bd79124_single_chan_seq(struct bd79124_data *d, int chan, int *old)
+{
+	int ret;
+
+	/* Stop the sequencer */
+	ret = regmap_clear_bits(d->map, BD79124_REG_SEQUENCE_CFG,
+				BD79124_MASK_SEQ_START);
+	if (ret)
+		return ret;
+
+	/*
+	 * It may be we have some channels monitored for alarms so we want to
+	 * cache the old config and return it when the single channel
+	 * measurement has been completed. Read the old config.
+	 */
+	ret = regmap_read(d->map, BD79124_REG_AUTO_CHANNELS, old);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(d->map, BD79124_REG_AUTO_CHANNELS, BIT(chan));
+	if (ret)
+		return ret;
+
+	/* Restart the sequencer */
+	return regmap_set_bits(d->map, BD79124_REG_SEQUENCE_CFG,
+			      BD79124_MASK_SEQ_START);
+}
+
+static int bd79124_single_chan_seq_end(struct bd79124_data *d, int old)
+{
+	int ret;
+
+	/* Stop the sequencer */
+	ret = regmap_clear_bits(d->map, BD79124_REG_SEQUENCE_CFG,
+				BD79124_MASK_SEQ_START);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(d->map, BD79124_REG_AUTO_CHANNELS, old);
+	if (ret)
+		return ret;
+
+	/* Restart the sequencer */
+	return regmap_set_bits(d->map, BD79124_REG_SEQUENCE_CFG,
+			      BD79124_MASK_SEQ_START);
+}
+
+static int bd79124_read_raw(struct iio_dev *idev,
+			    struct iio_chan_spec const *chan,
+			    int *val, int *val2, long m)
+{
+	struct bd79124_data *d = iio_priv(idev);
+	int ret;
+
+	if (chan->channel >= BD79124_MAX_NUM_CHANNELS)
+		return -EINVAL;
+
+	switch (m) {
+	case IIO_CHAN_INFO_RAW:
+	{
+		int old_chan_cfg, tmp;
+		int regval = FIELD_PREP(BD79124_MASK_CONV_MODE,
+					BD79124_CONV_MODE_AUTO);
+
+		guard(mutex)(&d->mutex);
+
+		/* ensure pinmux is set to ADC */
+		if (!bd79124_chan_is_adc(d, chan->channel))
+			return -EBUSY;
+
+		/*
+		 * Start the automatic conversion. This is needed here if no
+		 * events have been enabled.
+		 */
+		ret = regmap_update_bits(d->map, BD79124_REG_OPMODE_CFG,
+			BD79124_MASK_CONV_MODE, regval);
+		if (ret)
+			return ret;
+
+		ret = bd79124_single_chan_seq(d, chan->channel, &old_chan_cfg);
+		if (ret)
+			return ret;
+
+		/*
+		 * The maximum conversion time is 6 uS. Ensure the sample is
+		 * ready
+		 */
+		udelay(6);
+
+		ret = bd79124_read_last_result(d, chan->channel, val);
+		/* Try unconditionally returning the chan config */
+		tmp = bd79124_single_chan_seq_end(d, old_chan_cfg);
+		if (tmp)
+			dev_err(d->dev,
+				"Failed to return config. Alarms may be disabled\n");
+
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;
+	}
+	case IIO_CHAN_INFO_SCALE:
+		*val = d->vmax / 1000;
+		*val2 = BD79124_ADC_BITS;
+		return IIO_VAL_FRACTIONAL_LOG2;
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info bd79124_info = {
+	.read_raw = bd79124_read_raw,
+	.read_event_config = &bd79124_read_event_config,
+	.write_event_config = &bd79124_write_event_config,
+	.read_event_value = &bd79124_read_event_value,
+	.write_event_value = &bd79124_write_event_value,
+};
+
+static irqreturn_t bd79124_event_handler(int irq, void *priv)
+{
+	int ret, i_hi, i_lo, i;
+	struct iio_dev *idev = priv;
+	struct bd79124_data *d = iio_priv(idev);
+
+	/*
+	 * Return IRQ_NONE if bailing-out without acking. This allows the IRQ
+	 * subsystem to disable the offending IRQ line if we get a hardware
+	 * problem. This behaviour has saved my poor bottom a few times in the
+	 * past as, instead of getting unusably unresponsive, the system has
+	 * spilled out the magic words "...nobody cared".
+	 */
+	ret = regmap_read(d->map, BD79124_REG_EVENT_FLAG_HI, &i_hi);
+	if (ret)
+		return IRQ_NONE;
+
+	ret = regmap_read(d->map, BD79124_REG_EVENT_FLAG_LO, &i_lo);
+	if (ret)
+		return IRQ_NONE;
+
+	if (!i_lo && !i_hi)
+		return IRQ_NONE;
+
+	for (i = 0; i < BD79124_MAX_NUM_CHANNELS; i++) {
+		u64 ecode;
+
+		if (BIT(i) & i_hi) {
+			ecode = IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, i,
+					IIO_EV_TYPE_THRESH, IIO_EV_DIR_RISING);
+
+			iio_push_event(idev, ecode, d->timestamp);
+			/*
+			 * The BD79124 keeps the IRQ asserted for as long as
+			 * the voltage exceeds the threshold. It may not serve
+			 * the purpose to keep the IRQ firing and events
+			 * generated in a loop because it may yield the
+			 * userspace to have some problems when event handling
+			 * there is slow.
+			 *
+			 * Thus, we disable the event for the channel. Userspace
+			 * needs to re-enable the event.
+			 *
+			 * We don't check the result as there is not much to do.
+			 * Also, this should not lead to total IRQ storm
+			 * because the BD79124 is running in autonomous mode,
+			 * which means there is by minimum 0.75 mS idle time
+			 * between changing the channels. That should be
+			 * sufficient to show some life on system, even if the
+			 * event handling couldn't keep up.
+			 */
+			bd79124_disable_event(d, IIO_EV_DIR_RISING, i);
+		}
+		if (BIT(i) & i_lo) {
+			ecode = IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, i,
+					IIO_EV_TYPE_THRESH, IIO_EV_DIR_FALLING);
+
+			iio_push_event(idev, ecode, d->timestamp);
+		}
+	}
+
+	ret = regmap_write(d->map, BD79124_REG_EVENT_FLAG_HI, i_hi);
+	if (ret)
+		return IRQ_NONE;
+
+	ret = regmap_write(d->map, BD79124_REG_EVENT_FLAG_LO, i_lo);
+	if (ret)
+		return IRQ_NONE;
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t bd79124_irq_handler(int irq, void *priv)
+{
+	struct iio_dev *idev = priv;
+	struct bd79124_data *d = iio_priv(idev);
+
+	d->timestamp = iio_get_time_ns(idev);
+
+	return IRQ_WAKE_THREAD;
+}
+
+static int bd79124_hw_init(struct bd79124_data *d)
+{
+	int ret, regval;
+
+	/* Stop auto sequencer */
+	ret = regmap_clear_bits(d->map, BD79124_REG_SEQUENCE_CFG,
+				BD79124_MASK_SEQ_START);
+	if (ret)
+		return ret;
+
+	/* Enable writing the measured values to the regsters */
+	ret = regmap_set_bits(d->map, BD79124_REG_GEN_CFG,
+				 BD79124_MASK_STATS_EN);
+	if (ret)
+		return ret;
+
+	/* Set no channels to be auto-measured */
+	ret = regmap_write(d->map, BD79124_REG_AUTO_CHANNELS, 0x0);
+	if (ret)
+		return ret;
+
+	/* Set no channels to be manually measured */
+	ret = regmap_write(d->map, BD79124_REG_MANUAL_CHANNELS, 0x0);
+	if (ret)
+		return ret;
+
+	/* Set the measurement interval to 0.75 mS */
+
+	regval = FIELD_PREP(BD79124_MASK_AUTO_INTERVAL, BD79124_INTERVAL_075);
+	ret = regmap_update_bits(d->map, BD79124_REG_OPMODE_CFG,
+			BD79124_MASK_AUTO_INTERVAL, regval);
+	if (ret)
+		return ret;
+
+	/* Sequencer mode to auto */
+	ret = regmap_set_bits(d->map, BD79124_REG_SEQUENCE_CFG,
+			      BD79124_MASK_SEQ_SEQ);
+	if (ret)
+		return ret;
+
+	regval = FIELD_PREP(BD79124_MASK_CONV_MODE, BD79124_CONV_MODE_MANSEQ);
+	/* Don't start the measurement */
+	return regmap_update_bits(d->map, BD79124_REG_OPMODE_CFG,
+			BD79124_MASK_CONV_MODE, BD79124_CONV_MODE_MANSEQ);
+
+}
+
+#define BD79124_VDD_MAX 5250000
+#define BD79124_VDD_MIN 2700000
+
+static int bd79124_probe(struct platform_device *pdev)
+{
+	struct bd79124_data *d;
+	struct iio_dev *idev;
+	int ret, irq, *parent_data;
+
+	idev = devm_iio_device_alloc(&pdev->dev, sizeof(*d));
+	if (!idev)
+		return -ENOMEM;
+
+	d = iio_priv(idev);
+
+	parent_data = dev_get_drvdata(pdev->dev.parent);
+	if (!parent_data)
+		return dev_err_probe(&pdev->dev, -EINVAL,
+				     "reference voltage missing\n");
+
+	/*
+	 * Recommended VDD voltage from the data-sheet:
+	 * Analog/Digital Supply Voltage VDD 2.70 - 5.25 V
+	 */
+	d->vmax = *parent_data;
+	if (d->vmax < BD79124_VDD_MIN || d->vmax > BD79124_VDD_MAX) {
+		return dev_err_probe(d->dev, -EINVAL,
+				     "VDD (%d) out of range [%d - %d]\n",
+				     d->vmax, BD79124_VDD_MIN, BD79124_VDD_MAX);
+
+		return -EINVAL;
+	}
+
+	irq = platform_get_irq_byname_optional(pdev, "thresh-alert");
+	if (irq < 0) {
+		if (irq == -EPROBE_DEFER)
+			return irq;
+
+		idev->channels = &bd79124_channels_noirq[0];
+		idev->num_channels = ARRAY_SIZE(bd79124_channels_noirq);
+		dev_dbg(d->dev, "No IRQ found, events disabled\n");
+	} else {
+		idev->channels = &bd79124_channels[0];
+		idev->num_channels = ARRAY_SIZE(bd79124_channels);
+	}
+
+	idev->info = &bd79124_info;
+	idev->name = "bd79124";
+	idev->modes = INDIO_DIRECT_MODE;
+
+	d->dev = &pdev->dev;
+	d->map = dev_get_regmap(d->dev->parent, NULL);
+	if (!d->map)
+		return dev_err_probe(d->dev, -ENODEV, "No regmap\n");
+
+	mutex_init(&d->mutex);
+
+	ret = bd79124_hw_init(d);
+	if (ret)
+		return ret;
+
+	if (irq > 0) {
+		ret = devm_request_threaded_irq(d->dev, irq, bd79124_irq_handler,
+					&bd79124_event_handler, IRQF_ONESHOT,
+					"adc-thresh-alert", idev);
+		if (ret)
+			return dev_err_probe(d->dev, ret,
+					     "Failed to register IRQ\n");
+	}
+
+	return devm_iio_device_register(d->dev, idev);
+}
+
+static const struct platform_device_id bd79124_adc_id[] = {
+	{ "bd79124-adc", },
+	{ }
+};
+MODULE_DEVICE_TABLE(platform, bd79124_adc_id);
+
+static struct platform_driver bd79124_driver = {
+	.driver = {
+		.name = "bd79124-adc",
+		/*
+		 * Probing explicitly requires a few millisecond of sleep.
+		 * Enabling the VDD regulator may include ramp up rates.
+		 */
+		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
+	},
+	.probe = bd79124_probe,
+	.id_table = bd79124_adc_id,
+};
+module_platform_driver(bd79124_driver);
+
+MODULE_AUTHOR("Matti Vaittinen <mazziesaccount@gmail.com>");
+MODULE_DESCRIPTION("Driver for ROHM BD79124 ADC");
+MODULE_LICENSE("GPL");
-- 
2.48.1


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

[RFC PATCH 4/5] pinctrl: Support ROHM BD79124 pinmux / GPO

[Matti Vaittinen]

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

The ROHM BD79124 is a 12-bit, 8-channel, SAR ADC. The AIN pins can be
used as ADC inputs, or as general purpose outputs.

Support changing pin function (GPO / ADC) and the gpo output control.

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

NOTE: This patch is not properly tested. More thorough testing is to be
done prior v2 if this pinmux approach makes sense.

 drivers/pinctrl/Kconfig           |  11 ++
 drivers/pinctrl/Makefile          |   1 +
 drivers/pinctrl/pinctrl-bd79124.c | 276 ++++++++++++++++++++++++++++++
 3 files changed, 288 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-bd79124.c

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 95a8e2b9a614..7dd9bb0d1ab4 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -145,6 +145,17 @@ config PINCTRL_AW9523
 
 	  Say yes to enable pinctrl and GPIO support for the AW9523(B).
 
+config PINCTRL_BD79124
+	tristate "Rohm BD79124 ADC/GPO"
+	depends on MFD_ROHM_BD79124
+	select PINMUX
+	select GPIOLIB
+	help
+	  The Rohm BD79124 is a 12-bit, 8-channel, SAR ADC. The analog input
+	  pins can also be configured to be used as general purpose outputs.
+
+	  Say yes to enable the pinmux and GPOs.
+
 config PINCTRL_BM1880
 	bool "Bitmain BM1880 Pinctrl driver"
 	depends on OF && (ARCH_BITMAIN || COMPILE_TEST)
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index fba1c56624c0..0caf6dc3d2c1 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_PINCTRL_AT91PIO4)	+= pinctrl-at91-pio4.o
 obj-$(CONFIG_PINCTRL_AW9523)	+= pinctrl-aw9523.o
 obj-$(CONFIG_PINCTRL_AXP209)	+= pinctrl-axp209.o
 obj-$(CONFIG_PINCTRL_BM1880)	+= pinctrl-bm1880.o
+obj-$(CONFIG_PINCTRL_BD79124)	+= pinctrl-bd79124.o
 obj-$(CONFIG_PINCTRL_CY8C95X0)	+= pinctrl-cy8c95x0.o
 obj-$(CONFIG_PINCTRL_DA850_PUPD) += pinctrl-da850-pupd.o
 obj-$(CONFIG_PINCTRL_DA9062)	+= pinctrl-da9062.o
diff --git a/drivers/pinctrl/pinctrl-bd79124.c b/drivers/pinctrl/pinctrl-bd79124.c
new file mode 100644
index 000000000000..8d25b1c5345f
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-bd79124.c
@@ -0,0 +1,276 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * ROHM BD79124 ADC / GPO pinmux.
+ *
+ * Copyright (c) 2025, ROHM Semiconductor.
+ *
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/gpio/driver.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/types.h>
+
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/mfd/rohm-bd79124.h>
+#include "pinctrl-utils.h"
+
+/*
+ * The driver expects pins top have 1 to 1 mapping to the groups.
+ * Eg, pin 'ID 0' is AIN0, and can be directly mapped to the group "ain0", which
+ * also uses group ID 0. The driver mix and match the pin and group IDs. This
+ * works because we don't have any specific multi-pin groups. If I knew typical
+ * use cases better I might've been able to create some funtionally meaningful
+ * groups - but as I don't, I just decided to create per-pin groups for toggling
+ * and individual pin to ADC-input or GPO mode. I believe this gives the
+ * flexibility for generic use-cases.
+ *
+ * If this is a false assumption and special groups are needed, then the pin <=>
+ * group mapping in this driver must be reworked. Meanwhile just keep the pin
+ * and group IDs matching!
+ */
+static const struct pinctrl_pin_desc bd79124_pins[] = {
+	PINCTRL_PIN(0, "ain0"),
+	PINCTRL_PIN(1, "ain1"),
+	PINCTRL_PIN(2, "ain2"),
+	PINCTRL_PIN(3, "ain3"),
+	PINCTRL_PIN(4, "ain4"),
+	PINCTRL_PIN(5, "ain5"),
+	PINCTRL_PIN(6, "ain6"),
+	PINCTRL_PIN(7, "ain7"),
+};
+
+static const char * const bd79124_pin_groups[] = {
+	"ain0",
+	"ain1",
+	"ain2",
+	"ain3",
+	"ain4",
+	"ain5",
+	"ain6",
+	"ain7",
+};
+
+static int bd79124_get_groups_count(struct pinctrl_dev *pcdev)
+{
+	return ARRAY_SIZE(bd79124_pin_groups);
+}
+
+static const char *bd79124_get_group_name(struct pinctrl_dev *pctldev,
+					   unsigned int group)
+{
+	return bd79124_pin_groups[group];
+}
+
+enum {
+	BD79124_FUNC_GPO,
+	BD79124_FUNC_ADC,
+	BD79124_FUNC_AMOUNT
+};
+
+static const char * const bd79124_functions[BD79124_FUNC_AMOUNT] = {
+	[BD79124_FUNC_GPO] = "gpo",
+	[BD79124_FUNC_ADC] = "adc",
+};
+
+struct bd79124_mux_data {
+	struct device *dev;
+	struct regmap *map;
+	struct pinctrl_dev *pcdev;
+	struct gpio_chip gc;
+};
+
+static int bd79124_pmx_get_functions_count(struct pinctrl_dev *pcdev)
+{
+	return BD79124_FUNC_AMOUNT;
+}
+
+static const char *bd79124_pmx_get_function_name(struct pinctrl_dev *pcdev,
+						  unsigned int selector)
+{
+	return bd79124_functions[selector];
+}
+
+static int bd79124_pmx_get_function_groups(struct pinctrl_dev *pcdev,
+		unsigned int selector,
+		const char * const **groups,
+		unsigned int * const num_groups)
+{
+	*groups = &bd79124_pin_groups[0];
+	*num_groups = ARRAY_SIZE(bd79124_pin_groups);
+
+	return 0;
+}
+
+static int bd79124_pmx_set(struct pinctrl_dev *pcdev, unsigned int func,
+			   unsigned int group)
+{
+	struct bd79124_mux_data *d = pinctrl_dev_get_drvdata(pcdev);
+
+	/* We use 1 to 1 mapping for grp <=> pin */
+	if (func == BD79124_FUNC_GPO)
+		return regmap_set_bits(d->map, BD79124_REG_PINCFG, BIT(group));
+
+	return regmap_clear_bits(d->map, BD79124_REG_PINCFG, BIT(group));
+}
+
+/*
+ * Check that the pinmux has set this pin as GPO before allowing it to be used.
+ * NOTE: There is no locking in the pinctrl driver to ensure the pin _stays_
+ * appropriately muxed. It is the responsibility of the device using this GPO
+ * (or ADC) to reserve the pin from the pinmux.
+ */
+static bool bd79124_is_gpo(struct bd79124_mux_data *d, unsigned int offset)
+{
+	int ret, val;
+
+	ret = regmap_read(d->map, BD79124_REG_PINCFG, &val);
+	/*
+	 * If read fails, don't allow setting GPO value as we don't know if
+	 * pin is used as AIN. (In which case we might upset the device being
+	 * measured - although I suppose the BD79124 would ignore the set value
+	 * if pin is used as AIN - but better safe than sorry, right?
+	 */
+	if (ret)
+		return 0;
+
+	return (val & BIT(offset));
+}
+
+static int bd79124gpo_direction_get(struct gpio_chip *gc, unsigned int offset)
+{
+	struct bd79124_mux_data *d = gpiochip_get_data(gc);
+
+	if (!bd79124_is_gpo(d, offset))
+		return -EINVAL;
+
+	return GPIO_LINE_DIRECTION_OUT;
+}
+
+static void bd79124gpo_set(struct gpio_chip *gc, unsigned int offset, int value)
+{
+	struct bd79124_mux_data *d = gpiochip_get_data(gc);
+
+	if (!bd79124_is_gpo(d, offset)) {
+		dev_dbg(d->dev, "Bad GPO mux mode\n");
+		return;
+	}
+
+	if (value)
+		regmap_set_bits(d->map, BD79124_REG_GPO_VAL, BIT(offset));
+
+	regmap_clear_bits(d->map, BD79124_REG_GPO_VAL, BIT(offset));
+}
+
+static void bd79124gpo_set_multiple(struct gpio_chip *gc, unsigned long *mask,
+				   unsigned long *bits)
+{
+	int ret, val;
+	struct bd79124_mux_data *d = gpiochip_get_data(gc);
+
+	/* Ensure all GPIOs in 'mask' are set to be GPIOs */
+	ret = regmap_read(d->map, BD79124_REG_PINCFG, &val);
+	if (ret)
+		return;
+
+	if ((val & *mask) != *mask) {
+		dev_dbg(d->dev, "Invalid mux config. Can't set value.\n");
+		/* Do not set value for pins configured as ADC inputs */
+		*mask &= val;
+	}
+
+	regmap_update_bits(d->map, BD79124_REG_GPO_VAL, *mask, *bits);
+}
+
+/* Template for GPIO chip */
+static const struct gpio_chip bd79124gpo_chip = {
+	.label			= "bd79124-gpo",
+	.get_direction		= bd79124gpo_direction_get,
+	.set			= bd79124gpo_set,
+	.set_multiple		= bd79124gpo_set_multiple,
+	.can_sleep		= true,
+	.ngpio			= 8,
+	.base			= -1,
+};
+
+static const struct pinmux_ops bd79124_pmxops = {
+	.get_functions_count = bd79124_pmx_get_functions_count,
+	.get_function_name = bd79124_pmx_get_function_name,
+	.get_function_groups = bd79124_pmx_get_function_groups,
+	.set_mux = bd79124_pmx_set,
+};
+
+static const struct pinctrl_ops bd79124_pctlops = {
+	.get_groups_count = bd79124_get_groups_count,
+	.get_group_name = bd79124_get_group_name,
+	.dt_node_to_map = pinconf_generic_dt_node_to_map_all,
+	.dt_free_map = pinctrl_utils_free_map,
+};
+
+static const struct pinctrl_desc bd79124_pdesc = {
+	.name = "bd79124-pinctrl",
+	.pins = &bd79124_pins[0],
+	.npins = ARRAY_SIZE(bd79124_pins),
+	.pmxops = &bd79124_pmxops,
+	.pctlops = &bd79124_pctlops,
+};
+
+static int bd79124_probe(struct platform_device *pdev)
+{
+	struct bd79124_mux_data *d;
+	int ret;
+
+	d = devm_kzalloc(&pdev->dev, sizeof(*d), GFP_KERNEL);
+	if (!d)
+		return -ENOMEM;
+
+	d->dev = &pdev->dev;
+	d->map = dev_get_regmap(d->dev->parent, NULL);
+	if (!d->map)
+		return dev_err_probe(d->dev, -ENODEV, "No regmap\n");
+
+	d->gc = bd79124gpo_chip;
+
+	ret = devm_pinctrl_register_and_init(d->dev->parent,
+			(struct pinctrl_desc *)&bd79124_pdesc, d, &d->pcdev);
+	if (ret)
+		return dev_err_probe(d->dev, ret, "pincontrol registration failed\n");
+	ret = pinctrl_enable(d->pcdev);
+	if (ret)
+		return dev_err_probe(d->dev, ret, "pincontrol enabling failed\n");
+
+	ret = devm_gpiochip_add_data(d->dev, &d->gc, d);
+	if (ret)
+		return dev_err_probe(d->dev, ret, "gpio init Failed\n");
+
+	return 0;
+}
+
+static const struct platform_device_id bd79124_mux_id[] = {
+	{ "bd79124-pinmux", },
+	{ }
+};
+MODULE_DEVICE_TABLE(platform, bd79124_mux_id);
+
+static struct platform_driver bd79124_mux_driver = {
+	.driver = {
+		.name = "bd79124-pinmux",
+		/*
+		 * Probing explicitly requires a few millisecond of sleep.
+		 * Enabling the VDD regulator may include ramp up rates.
+		 */
+		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
+	},
+	.probe = bd79124_probe,
+	.id_table = bd79124_mux_id,
+};
+module_platform_driver(bd79124_mux_driver);
+
+MODULE_AUTHOR("Matti Vaittinen <mazziesaccount@gmail.com>");
+MODULE_DESCRIPTION("Pinmux/GPO Driver for ROHM BD79124");
+MODULE_LICENSE("GPL");
-- 
2.48.1


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

[RFC PATCH 5/5] MAINTAINERS: Add ROHM BD79124 ADC/GPO

[Matti Vaittinen]

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

Add undersigned as a maintainer for the ROHM BD79124 ADC/GPO driver.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
 MAINTAINERS | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index a87ddad78e26..19314bee95b2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20292,6 +20292,14 @@ S:	Supported
 F:	drivers/power/supply/bd99954-charger.c
 F:	drivers/power/supply/bd99954-charger.h
 
+ROHM BD79124 ADC / GPO IC
+M:	Matti Vaittinen <mazziesaccount@gmail.com>
+S:	Supported
+F:	drivers/iio/adc/rohm-bd79124-adc.c
+F:	drivers/mfd/rohm-bd79124.c
+F:	drivers/pinctrl/pinctrl-bd79124.c
+F:	include/linux/mfd/rohm-bd79124.h
+
 ROHM BH1745 COLOUR SENSOR
 M:	Mudit Sharma <muditsharma.info@gmail.com>
 L:	linux-iio@vger.kernel.org
-- 
2.48.1


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

Re: [RFC PATCH 0/5] Support ROHM BD79124 ADC/GPO

[Jonathan Cameron]

On Fri, 31 Jan 2025 15:34:43 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> Support ROHM BD79124 ADC.
> 
> Quite usual stuff. 12-bit, 8-channel ADC with threshold monitoring.
> 
> Except that:
>  - each ADC input pin can be configured as a general purpose output.
>  - manually starting an ADC conversion and reading the result would
>    require the I2C _master_ to do clock stretching(!) for the duration
>    of the conversion... Let's just say this is not well supported.
>  - IC supports 'autonomous measurement mode' and storing latest results
>    to the result registers. This mode is used by the driver due to the
>    "peculiar" I2C when doing manual reads.
> 
> I sent this as an RFC because I implemented the pin purposing (GPO/ADC)
> using pinmux - which I've never done for upstream stuff before. Hence
> it's better to ask if this makes sense, or if there is better way to go.
> Anyways, resulted drivers spread to 3 subsystems (MFD, pinctrl and IIO)
In principle nothing against pin mux for this.
There are other options though if pin mux ends up being too complex.

- provide ADC channels in the binding channel@x etc.
Anything else is freely available as a GPIO.
Normal GPIO bindings etc for those.

The channel bit is common on SoC ADC anyway where we don't want to
expose channels that aren't wired out.

For combined ADC GPIO chips we normally don't bother with an MFD.
Just host the gpio driver in the ADC one unless there is a strong
reasons someone will put this down for GPIO usage only.

> 
> Furthermore, the GPO functionality has not been (properly) tested. I'll
> do more testing for v2 if this pinmux approach is appropriate.
> 
> Furthermore, because the ADC uses this continuous autonomous measuring,
> and because the IC keeps producing new 'out of window' IRQs if
> measurements are out of window - the driver disables the event when
> sending one. This prevents generating storm of events, but it also
> requires users to reconfigure / re-enable an event if they wish to
> continue monitoring after receiving one. Again I am not sure if this is
> the best way to handle such HW - so better to ask for an opinion than a
> nose bleed, right? Maybe the next version will no longer be a RFC :)

Oddly I thought we had ABI for this but not finding it.
We basically want a thing that lets us say don't allow a repeat event
for X seconds. Then we set a timer and reenable the interrupt after that
time.  I think there are drivers doing this but can't find one right
now :(  It's close to _timeout used for gesture detection.

> 
> ---
> 
> Matti Vaittinen (5):
>   dt-bindings: ROHM BD79124 ADC/GPO
>   mfd: Add ROHM BD79124 ADC/GPO
>   iio: adc: Support ROHM BD79124 ADC
>   pinctrl: Support ROHM BD79124 pinmux / GPO
>   MAINTAINERS: Add ROHM BD79124 ADC/GPO
> 
>  .../devicetree/bindings/mfd/rohm,bd79124.yaml | 111 +++
>  MAINTAINERS                                   |   8 +
>  drivers/iio/adc/Kconfig                       |  10 +
>  drivers/iio/adc/Makefile                      |   1 +
>  drivers/iio/adc/rohm-bd79124-adc.c            | 890 ++++++++++++++++++
>  drivers/mfd/Kconfig                           |  12 +
>  drivers/mfd/Makefile                          |   1 +
>  drivers/mfd/rohm-bd79124.c                    | 165 ++++
>  drivers/pinctrl/Kconfig                       |  11 +
>  drivers/pinctrl/Makefile                      |   1 +
>  drivers/pinctrl/pinctrl-bd79124.c             | 276 ++++++
>  include/linux/mfd/rohm-bd79124.h              |  32 +
>  12 files changed, 1518 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/rohm,bd79124.yaml
>  create mode 100644 drivers/iio/adc/rohm-bd79124-adc.c
>  create mode 100644 drivers/mfd/rohm-bd79124.c
>  create mode 100644 drivers/pinctrl/pinctrl-bd79124.c
>  create mode 100644 include/linux/mfd/rohm-bd79124.h
> 
> 
> base-commit: 5bc55a333a2f7316b58edc7573e8e893f7acb532

Re: [RFC PATCH 2/5] mfd: Add ROHM BD79124 ADC/GPO

[Jonathan Cameron]

On Fri, 31 Jan 2025 15:37:06 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> Add core driver for the ROHM BD79124 ADC / GPO.
> 
> The core driver launches the sub-drivers for the pinmux/GPO and for the
> IIO ADC. It also provides the regmap, and forwards the IRQ resource to
> the ADC.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
As per response in cover letter. This is a common device combination and so
far I don't think we ever bothered with an MFD. Lots of ADCs provide
GPIO chips as well so I'd just squash it into the ADC driver.

> ---
>  drivers/mfd/Kconfig              |  12 +++
>  drivers/mfd/Makefile             |   1 +
>  drivers/mfd/rohm-bd79124.c       | 165 +++++++++++++++++++++++++++++++
>  include/linux/mfd/rohm-bd79124.h |  32 ++++++
>  4 files changed, 210 insertions(+)
>  create mode 100644 drivers/mfd/rohm-bd79124.c
>  create mode 100644 include/linux/mfd/rohm-bd79124.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index ae23b317a64e..f024256fb180 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -2113,6 +2113,18 @@ config MFD_ROHM_BD71828
>  	  also a single-cell linear charger, a Coulomb counter, a real-time
>  	  clock (RTC), GPIOs and a 32.768 kHz clock gate.
>  
> +config MFD_ROHM_BD79124
> +	tristate "Rohm BD79124 core driver"
> +	depends on I2C
> +	select REGMAP_I2C
> +	help
> +	  Say yes here to build support for the ROHM BD79124 ADC core. The
> +	  ROHM BD79124 is a 12-bit, 8-channel, SAR ADC. The ADC supports
> +	  also an automatic measurement mode, with an alarm interrupt for
> +	  out-of-window measurements. The window is configurable for each
> +	  channel. The ADC inputs can optionally be used as general purpose
> +	  outputs.
> +
>  config MFD_ROHM_BD957XMUF
>  	tristate "ROHM BD9576MUF and BD9573MUF Power Management ICs"
>  	depends on I2C=y
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index e057d6d6faef..c7d64e933a7d 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -270,6 +270,7 @@ obj-$(CONFIG_MFD_SC27XX_PMIC)	+= sprd-sc27xx-spi.o
>  obj-$(CONFIG_RAVE_SP_CORE)	+= rave-sp.o
>  obj-$(CONFIG_MFD_ROHM_BD71828)	+= rohm-bd71828.o
>  obj-$(CONFIG_MFD_ROHM_BD718XX)	+= rohm-bd718x7.o
> +obj-$(CONFIG_MFD_ROHM_BD79124)	+= rohm-bd79124.o
>  obj-$(CONFIG_MFD_ROHM_BD957XMUF)	+= rohm-bd9576.o
>  obj-$(CONFIG_MFD_ROHM_BD96801)	+= rohm-bd96801.o
>  obj-$(CONFIG_MFD_STMFX) 	+= stmfx.o
> diff --git a/drivers/mfd/rohm-bd79124.c b/drivers/mfd/rohm-bd79124.c
> new file mode 100644
> index 000000000000..c35ab0e03b0b
> --- /dev/null
> +++ b/drivers/mfd/rohm-bd79124.c
> @@ -0,0 +1,165 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +//
> +// Copyright (C) 2025 ROHM Semiconductors
> +//
> +// ROHM BD79124 ADC / GPO driver
> +
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>

mod_devicetable.h

> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/rohm-bd79124.h>
> +
> +static struct resource adc_alert;

What if we have two of these?

> +
> +enum {
> +	CELL_PINMUX,
> +	CELL_ADC,
> +};
> +
> +static struct mfd_cell bd79124_cells[] = {
> +	[CELL_PINMUX]	= { .name = "bd79124-pinmux", },
> +	[CELL_ADC]	= { .name = "bd79124-adc", },
> +};
> +
> +/* Read-only regs */
> +static const struct regmap_range bd79124_ro_ranges[] = {
> +	{
> +		.range_min = BD79124_REG_EVENT_FLAG,
> +		.range_max = BD79124_REG_EVENT_FLAG,
> +	}, {
> +		.range_min = BD79124_REG_RECENT_CH0_LSB,
> +		.range_max = BD79124_REG_RECENT_CH7_MSB,
> +	},
> +};
> +
> +static const struct regmap_access_table bd79124_ro_regs = {
> +	.no_ranges	= &bd79124_ro_ranges[0],
> +	.n_no_ranges	= ARRAY_SIZE(bd79124_ro_ranges),
> +};
> +
> +static const struct regmap_range bd79124_volatile_ranges[] = {
> +	{
> +		.range_min = BD79124_REG_RECENT_CH0_LSB,
> +		.range_max = BD79124_REG_RECENT_CH7_MSB,
> +	}, {
> +		.range_min = BD79124_REG_EVENT_FLAG,
> +		.range_max = BD79124_REG_EVENT_FLAG,
> +	}, {
> +		.range_min = BD79124_REG_EVENT_FLAG_HI,
> +		.range_max = BD79124_REG_EVENT_FLAG_HI,
> +	}, {
> +		.range_min = BD79124_REG_EVENT_FLAG_LO,
> +		.range_max = BD79124_REG_EVENT_FLAG_LO,
> +	}, {
> +		.range_min = BD79124_REG_SYSTEM_STATUS,
> +		.range_max = BD79124_REG_SYSTEM_STATUS,
> +	},
> +};
> +
> +static const struct regmap_access_table bd79124_volatile_regs = {
> +	.yes_ranges	= &bd79124_volatile_ranges[0],
> +	.n_yes_ranges	= ARRAY_SIZE(bd79124_volatile_ranges),
> +};
> +
> +static const struct regmap_range bd79124_precious_ranges[] = {
> +	{
> +		.range_min = BD79124_REG_EVENT_FLAG_HI,
> +		.range_max = BD79124_REG_EVENT_FLAG_HI,
> +	}, {
> +		.range_min = BD79124_REG_EVENT_FLAG_LO,
> +		.range_max = BD79124_REG_EVENT_FLAG_LO,
> +	},
> +};
> +
> +static const struct regmap_access_table bd79124_precious_regs = {
> +	.yes_ranges	= &bd79124_precious_ranges[0],
> +	.n_yes_ranges	= ARRAY_SIZE(bd79124_precious_ranges),
> +};
> +
> +static const struct regmap_config bd79124_regmap = {
> +	.reg_bits		= 16,
> +	.val_bits		= 8,
> +	.read_flag_mask		= BD79124_I2C_MULTI_READ,
> +	.write_flag_mask	= BD79124_I2C_MULTI_WRITE,
> +	.max_register		= BD79124_REG_MAX,
> +	.cache_type		= REGCACHE_MAPLE,
> +	.volatile_table		= &bd79124_volatile_regs,
> +	.wr_table		= &bd79124_ro_regs,
> +	.precious_table		= &bd79124_precious_regs,
> +};
> +
> +static int bd79124_probe(struct i2c_client *i2c)
> +{
> +	int ret;
> +	struct regmap *map;
> +	struct device *dev = &i2c->dev;
> +	int *adc_vref;

Wrap that in a structure.  It's just a bit too odd to have just
one integer!

> +
> +	adc_vref = devm_kzalloc(dev, sizeof(*adc_vref), GFP_KERNEL);
> +	if (!adc_vref)
> +		return -ENOMEM;
> +
> +	/*
> +	 * Better to enable regulators here so we don't need to worry about the
> +	 * order of sub-device instantiation. We also need to deliver the
> +	 * reference voltage value to the ADC driver. This is done via
> +	 * the MFD driver's drvdata.
> +	 */
> +	*adc_vref = devm_regulator_get_enable_read_voltage(dev, "vdd");
> +	if (*adc_vref < 0)
> +		return dev_err_probe(dev, ret, "Failed to get the Vdd\n");
> +
> +	dev_set_drvdata(dev, adc_vref);
> +
> +	ret = devm_regulator_get_enable(dev, "iovdd");
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Failed to enable I/O voltage\n");
> +
> +	map = devm_regmap_init_i2c(i2c, &bd79124_regmap);
> +	if (IS_ERR(map))
> +		return dev_err_probe(dev, PTR_ERR(map),
> +				     "Failed to initialize Regmap\n");
> +
> +	if (i2c->irq) {
> +		adc_alert = DEFINE_RES_IRQ_NAMED(i2c->irq, "thresh-alert");
> +		bd79124_cells[CELL_ADC].resources = &adc_alert;
> +		bd79124_cells[CELL_ADC].num_resources = 1;
> +	}
> +
> +	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, bd79124_cells,
> +				   ARRAY_SIZE(bd79124_cells), NULL, 0, NULL);
> +	if (ret)
> +		dev_err_probe(dev, ret, "Failed to create subdevices\n");
return dev_err_probe();

Then return 0 in other path.

> +
> +	return ret;
> +}
>
> diff --git a/include/linux/mfd/rohm-bd79124.h b/include/linux/mfd/rohm-bd79124.h
> new file mode 100644
> index 000000000000..505faeb6f135
> --- /dev/null
> +++ b/include/linux/mfd/rohm-bd79124.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright 2021 ROHM Semiconductors.

No update on that date?

> + *
> + * Author: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> + */
> +
> +#ifndef _MFD_BD79124_H
> +#define _MFD_BD79124_H
> +
> +#define BD79124_I2C_MULTI_READ		0x30
> +#define BD79124_I2C_MULTI_WRITE		0x28
> +#define BD79124_REG_MAX			0xaf
> +
> +#define BD79124_REG_SYSTEM_STATUS	0x0

Give it two digits. 0x00 for ever so slight readability advantage.

> +#define BD79124_REG_GEN_CFG		0x01
> +#define BD79124_REG_OPMODE_CFG		0x04
> +#define BD79124_REG_PINCFG		0x05
> +#define BD79124_REG_GPO_VAL		0x06
> +#define BD79124_REG_SEQUENCE_CFG	0x10
> +#define BD79124_REG_MANUAL_CHANNELS	0x11
> +#define BD79124_REG_AUTO_CHANNELS	0x12
> +#define BD79124_REG_ALERT_CH_SEL	0x14
> +#define BD79124_REG_EVENT_FLAG		0x18
> +#define BD79124_REG_EVENT_FLAG_HI	0x1a
> +#define BD79124_REG_EVENT_FLAG_LO	0x1c
> +#define BD79124_REG_HYSTERESIS_CH0	0x20
> +#define BD79124_REG_EVENTCOUNT_CH0	0x22
> +#define BD79124_REG_RECENT_CH0_LSB	0xa0
> +#define BD79124_REG_RECENT_CH7_MSB	0xaf
> +
> +#endif

Re: [RFC PATCH 3/5] iio: adc: Support ROHM BD79124 ADC

[Jonathan Cameron]

On Fri, 31 Jan 2025 15:37:48 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> The ROHM BD79124 is a 12-bit, 8-channel, SAR ADC. The ADC supports
> an automatic measurement mode, with an alarm interrupt for out-of-window
> measurements. The window is configurable for each channel.
> 
> The I2C protocol for manual start of the measurement and data reading is
> somewhat peculiar. It requires the master to do clock stretching after
> sending the I2C slave-address until the slave has captured the data.
> Needless to say this is not well suopported by the I2C controllers.

From what I recall that is in the I2C spec, so in theory should be supported.
Ah well.

> 
> Thus the driver does not support the BD79124's manual measurement mode
> but implements the measurements using automatic measurement mode relying
> on the BD79124's ability of storing latest measurements into register.
> 
> The driver does also support configuring the threshold events for
> detecting the out-of-window events.
> 
> The BD79124 keeps asserting IRQ for as long as the measured voltage is
> out of the configured window. Thus the driver disables the event when
> first event is handled. This prevents the user-space from choking on the
> events - but it also requires the user space to reconfigure and
> re-enable the monitored event when it wants to keep monitoring for new
> occurrences.
> 
> It is worth noting that the ADC input pins can be also configured as
> general purpose outputs. The pin mode should be configured using pincmux
> driver.

We shouldn't be presenting channels that are configure for GPIOs as
ADC channels.  It is very rare that there is a usecase for any
dynamic switching. Normally it's a case of what is wired and
so static.  Hence build the iio_chan_spec array for just the
channels you want, not the the lot.  Channel sub nodes in the
DT are how we most commonly specify what is wired.

Few other comments inline,

Jonathan

> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> 
> ---
> 
> Regarding disabling the event upon reception - is this totally strange?
> Is regular userspace compeletely unprepared for this, and better
> prepared for handling large amounts of continuous events?
> 
> The BD79124 should not cause a total CPU-blocking IRQ storm because the
> driver uses the autonomous sequencer mode - which has minimum of 0.75
> millisecond delay between measurements. So, new IRQs can be raised with
> this interval. (The 0.75 mS includes handling and acking / status reading
> delays - so there is still not much time for things done outside of the
> IRQ handling...)

> diff --git a/drivers/iio/adc/rohm-bd79124-adc.c b/drivers/iio/adc/rohm-bd79124-adc.c
> new file mode 100644
> index 000000000000..7c95a1de1e71
> --- /dev/null
> +++ b/drivers/iio/adc/rohm-bd79124-adc.c
> @@ -0,0 +1,890 @@

> +enum {
> +	BD79124_CH_0,

If they just map to integers of the same name, may not be worth bothering
with the enum!

> +	BD79124_CH_1,
> +	BD79124_CH_2,
> +	BD79124_CH_3,
> +	BD79124_CH_4,
> +	BD79124_CH_5,
> +	BD79124_CH_6,
> +	BD79124_CH_7,
> +	BD79124_MAX_NUM_CHANNELS
> +};

> +struct bd79124_raw {
> +	u8 bit0_3; /* Is set in high bits of the byte */
> +	u8 bit4_11;
> +};
> +#define BD79124_RAW_TO_INT(r) ((r.bit4_11 << 4) | (r.bit0_3 >> 4))
You could do this as an endian conversion and a single shift I think.
Might be slightly simpler.

> +
> +/*
> + * The high and low limits as well as the recent result values are stored in
> + * the same way in 2 consequent registers. The first register contains 4 bits
> + * of the value. These bits are stored in the high bits [7:4] of register, but
> + * they represent the low bits [3:0] of the value.
> + * The value bits [11:4] are stored in the next register.
> + *
> + * Read data from register and convert to integer.
> + */
> +static int bd79124_read_reg_to_int(struct bd79124_data *d, int reg,
> +				   unsigned int *val)
> +{
> +	int ret;
> +	struct bd79124_raw raw;
> +
> +	ret = regmap_bulk_read(d->map, reg, &raw, sizeof(raw));
> +	if (ret)
> +		dev_dbg(d->dev, "bulk_read failed %d\n", ret);

If it failed, then shouldn't set *val.

> +	*val = BD79124_RAW_TO_INT(raw);
> +
> +	return ret;
> +}
> +
> +/*
> + * The high and low limits as well as the recent result values are stored in
> + * the same way in 2 consequent registers. The first register contains 4 bits
> + * of the value. These bits are stored in the high bits [7:4] of register, but
> + * they represent the low bits [3:0] of the value.
> + * The value bits [11:4] are stored in the next regoster.
> + *
> + * Conver the integer to register format and write it using rmw cycle.
> + */
> +static int bd79124_write_int_to_reg(struct bd79124_data *d, int reg,
> +				    unsigned int val)
> +{
> +	struct bd79124_raw raw;
> +	int ret, tmp;
> +
> +	raw.bit4_11 = (u8)(val >> 4);
> +	raw.bit0_3 = (u8)(val << 4);
> +
> +	ret = regmap_read(d->map, reg, &tmp);
> +	if (ret)
> +		return ret;
> +
> +	raw.bit0_3 |= (0xf & tmp);
> +
> +	return regmap_bulk_write(d->map, reg, &raw, sizeof(raw));
> +}
> +
> +static const struct iio_event_spec bd79124_events[] = {
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_RISING,
> +		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
> +				 BIT(IIO_EV_INFO_ENABLE),
> +	},
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_FALLING,
> +		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
> +				 BIT(IIO_EV_INFO_ENABLE),
> +	},
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_EITHER,
> +		.mask_separate = BIT(IIO_EV_INFO_HYSTERESIS),
> +	},
> +};
> +
> +#define BD79124_CHAN(idx) {						\
> +	.type = IIO_VOLTAGE,						\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),		\
> +	.indexed = 1,							\
> +	.channel = idx,							\
> +}
> +
> +#define BD79124_CHAN_EV(idx) {						\
> +	.type = IIO_VOLTAGE,						\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),		\
> +	.indexed = 1,							\
> +	.channel = idx,							\
> +	.event_spec = bd79124_events,					\
> +	.num_event_specs = ARRAY_SIZE(bd79124_events),			\
> +}
> +
> +static const struct iio_chan_spec bd79124_channels[] = {
> +	BD79124_CHAN_EV(0),
> +	BD79124_CHAN_EV(1),
> +	BD79124_CHAN_EV(2),
> +	BD79124_CHAN_EV(3),
> +	BD79124_CHAN_EV(4),
> +	BD79124_CHAN_EV(5),
> +	BD79124_CHAN_EV(6),
> +	BD79124_CHAN_EV(7),
> +};
> +
> +static const struct iio_chan_spec bd79124_channels_noirq[] = {
> +	BD79124_CHAN(0),
> +	BD79124_CHAN(1),
> +	BD79124_CHAN(2),
> +	BD79124_CHAN(3),
> +	BD79124_CHAN(4),
> +	BD79124_CHAN(5),
> +	BD79124_CHAN(6),
> +	BD79124_CHAN(7),
> +};
> +
> +/*
> + * The BD79124 supports muxing the pins as ADC inputs or as a general purpose
> + * output. This muxing is handled by a pinmux driver. Here we just check the
> + * settings from the register, and disallow using the pin if pinmux is set to
> + * GPO.
> + *
> + * NOTE: This driver does not perform any locking to ensure the pinmux stays
> + * toggled to ADC for the duration of the whatever operation is being done.
> + * It is responsibility of the user to configure the pinmux.
> + */
> +static bool bd79124_chan_is_adc(struct bd79124_data *d, unsigned int offset)
> +{
> +	int ret, val;
> +
> +	ret = regmap_read(d->map, BD79124_REG_PINCFG, &val);
> +	/* If read fails, don't allow using as AIN (to be on a safe side) */
> +	if (ret)
> +		return 0;
> +
> +	return !(val & BIT(offset));
> +}
> +
> +static int bd79124_read_event_value(struct iio_dev *idev,
> +				    const struct iio_chan_spec *chan,
> +				    enum iio_event_type type,
> +				    enum iio_event_direction dir,
> +				    enum iio_event_info info, int *val,
> +				    int *val2)
> +{
> +	struct bd79124_data *d = iio_priv(idev);
> +	int ret, reg;
> +
> +	if (chan->channel >= BD79124_MAX_NUM_CHANNELS)
> +		return -EINVAL;
> +
> +	/* ensure pinmux is set to ADC */
> +	if (!bd79124_chan_is_adc(d, chan->channel))
> +		return -EBUSY;
> +
> +	switch (info) {
> +	case IIO_EV_INFO_VALUE:
> +		if (dir == IIO_EV_DIR_RISING)
> +			reg = BD79124_GET_HIGH_LIMIT_REG(chan->channel);
> +		else if (dir == IIO_EV_DIR_FALLING)
> +			reg = BD79124_GET_LOW_LIMIT_REG(chan->channel);
> +		else
> +			return -EINVAL;
> +
> +		ret = bd79124_read_reg_to_int(d, reg, val);
> +		if (ret)
> +			return ret;
> +
> +		return IIO_VAL_INT;
> +
> +	case IIO_EV_INFO_HYSTERESIS:
> +		reg = BD79124_GET_HYSTERESIS_REG(chan->channel);
> +		ret = regmap_read(d->map, reg, val);
> +		if (ret)
> +			return ret;
> +		/* Mask the non hysteresis bits */
> +		*val &= BD79124_MASK_HYSTERESIS;
> +		/*
> +		 * The data-sheet says the hysteresis register value needs to be
> +		 * sifted left by 3 (or multiplied by 8, depending on the
> +		 * page :] )
> +		 */
> +		*val <<= 3;
> +
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int bd79124_start_measurement(struct bd79124_data *d, int chan)
> +{
> +	int val, ret, regval;
> +
> +	/* ensure pinmux is set to ADC */
> +	if (!bd79124_chan_is_adc(d, chan))
> +		return -EBUSY;
> +
> +	/* See if already started */
> +	ret = regmap_read(d->map, BD79124_REG_AUTO_CHANNELS, &val);
> +	if (val & BIT(chan))
> +		return 0;
> +
> +	/* Stop the sequencer */
> +	ret = regmap_clear_bits(d->map, BD79124_REG_SEQUENCE_CFG,
> +				BD79124_MASK_SEQ_START);
> +	if (ret)
> +		return ret;
> +
> +	/* Add the channel to measured channels */
> +	ret = regmap_write(d->map, BD79124_REG_AUTO_CHANNELS, val | BIT(chan));
> +	if (ret)
> +		return ret;
> +
> +	/* Restart the sequencer */
> +	ret = regmap_set_bits(d->map, BD79124_REG_SEQUENCE_CFG,
> +			      BD79124_MASK_SEQ_START);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Start the measurement at the background. Don't bother checking if
> +	 * it was started, regmap has cache
> +	 */
> +	regval = FIELD_PREP(BD79124_MASK_CONV_MODE, BD79124_CONV_MODE_AUTO);
> +
> +	return regmap_update_bits(d->map, BD79124_REG_OPMODE_CFG,
> +				BD79124_MASK_CONV_MODE, regval);
> +}
> +
> +static int bd79124_stop_measurement(struct bd79124_data *d, int chan)
> +{
> +	int val, ret;
> +
> +	/* Ensure pinmux is set to ADC */
> +	if (!bd79124_chan_is_adc(d, chan))
> +		return -EBUSY;
> +
> +	/* If alarm is requested for the channel we won't stop measurement */
> +	if (d->alarm_monitored[chan])
> +		return 0;
> +
> +	/* See if already stopped */
> +	ret = regmap_read(d->map, BD79124_REG_AUTO_CHANNELS, &val);
> +	if (!(val & BIT(chan)))
> +		return 0;
> +
> +	/* Stop the sequencer */
> +	ret = regmap_clear_bits(d->map, BD79124_REG_SEQUENCE_CFG,
> +				BD79124_MASK_SEQ_START);
> +
> +	/* Clear the channel from the measured channels */
> +	ret = regmap_write(d->map, BD79124_REG_AUTO_CHANNELS,
> +			   (~BIT(chan)) & val);
> +	if (ret)
> +		return ret;
> +
> +	/* Stop background conversion if it was the last channel */
> +	if (!((~BIT(chan)) & val)) {
> +		int regval = FIELD_PREP(BD79124_MASK_CONV_MODE,
> +					BD79124_CONV_MODE_MANSEQ);
> +
> +		ret = regmap_update_bits(d->map, BD79124_REG_OPMODE_CFG,
> +					 BD79124_MASK_CONV_MODE, regval);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/* Restart the sequencer */
> +	return regmap_set_bits(d->map, BD79124_REG_SEQUENCE_CFG,
> +			       BD79124_MASK_SEQ_START);
> +}
> +
> +static int bd79124_read_event_config(struct iio_dev *idev,
> +				     const struct iio_chan_spec *chan,
> +				     enum iio_event_type type,
> +				     enum iio_event_direction dir)
> +{
> +	struct bd79124_data *d = iio_priv(idev);
> +	int val, ret, reg, disabled;
> +
> +	if (chan->channel >= BD79124_MAX_NUM_CHANNELS)
> +		return -EINVAL;
> +
> +	/* ensure pinmux is set to ADC */
> +	if (!bd79124_chan_is_adc(d, chan->channel))
> +		return -EBUSY;
> +
> +	ret = regmap_read(d->map, BD79124_REG_ALERT_CH_SEL, &val);
> +	if (ret)
> +		return ret;
> +
> +	/* The event is disabled if alerts for the channel are disabled */
> +	if (!(val & BIT(chan->channel)))
> +		return 0;
> +
> +	/*
> +	 * If alerts are on, then the event may be disabled if limit is set to
> +	 * the one extreme. (HW does not support disabling rising/falling
> +	 * thresholds independently. Hence we resort to setting high limit to
> +	 * MAX, or low limit to 0 to try effectively disable thresholds).
> +	 */
> +	if (dir == IIO_EV_DIR_RISING) {
> +		reg = BD79124_GET_HIGH_LIMIT_REG(chan->channel);
> +		disabled = BD79124_HIGH_LIMIT_MAX;
> +	} else if (dir == IIO_EV_DIR_FALLING) {
> +		reg = BD79124_GET_LOW_LIMIT_REG(chan->channel);
> +		disabled = BD79124_LOW_LIMIT_MIN;
> +	} else {
> +		return -EINVAL;
> +	}
> +	ret = bd79124_read_reg_to_int(d, reg, &val);
> +	if (ret)
> +		return ret;
> +
> +	return val != disabled;
> +}
> +
> +static int bd79124_disable_event(struct bd79124_data *d,
> +				 enum iio_event_direction dir, int channel)
> +{
> +	int dir_bit = BIT(dir), reg;
> +	unsigned int limit;
> +
> +	d->alarm_monitored[channel] &= (~dir_bit);
> +	/*
> +	 * Set thresholds either to 0 or to 2^12 - 1 as appropriate to prevent
> +	 * alerts and thus disable event generation.
> +	 */
> +	if (dir == IIO_EV_DIR_RISING) {
> +		reg = BD79124_GET_HIGH_LIMIT_REG(channel);
> +		limit = BD79124_HIGH_LIMIT_MAX;
> +	} else if (dir == IIO_EV_DIR_FALLING) {
> +		reg = BD79124_GET_LOW_LIMIT_REG(channel);
> +		limit = BD79124_LOW_LIMIT_MIN;
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Stop measurement if there is no more events to monitor.
> +	 * We don't bother checking the retval because the limit
> +	 * setting should in any case effectively disable the alarm.
> +	 */
> +	if (!d->alarm_monitored[channel]) {
> +		bd79124_stop_measurement(d, channel);
> +		regmap_clear_bits(d->map, BD79124_REG_ALERT_CH_SEL,
> +			       BIT(channel));
> +	}
> +
> +	return bd79124_write_int_to_reg(d, reg, limit);
> +}
> +
> +/* Do we need to disable the measurement for the duration of the limit conf ? */
> +static int bd79124_write_event_config(struct iio_dev *idev,
> +				      const struct iio_chan_spec *chan,
> +				      enum iio_event_type type,
> +				      enum iio_event_direction dir, bool state)
> +{
> +	struct bd79124_data *d = iio_priv(idev);
> +	int dir_bit = BIT(dir);
> +
> +	guard(mutex)(&d->mutex);
> +
> +	if (chan->channel >= BD79124_MAX_NUM_CHANNELS)
> +		return -EINVAL;
> +
> +	/* ensure pinmux is set to ADC */
> +	if (!bd79124_chan_is_adc(d, chan->channel))
> +		return -EBUSY;
> +
> +	if (state) {
> +		int ret;
> +
> +		/* Set channel to be measured */
> +		ret = bd79124_start_measurement(d, chan->channel);
> +		if (ret)
> +			return ret;
> +
> +		d->alarm_monitored[chan->channel] |= dir_bit;
> +
> +		/* Add the channel to the list of monitored channels */
> +		ret = regmap_set_bits(d->map, BD79124_REG_ALERT_CH_SEL,
> +				      BIT(chan->channel));
> +		if (ret)
> +			return ret;
> +
> +		/*
> +		 * Enable comparator. Trust the regmap cache, no need to check
> +		 * if it was already enabled.
> +		 *
> +		 * We could do this in the hw-init, but there may be users who
> +		 * never enable alarms and for them it makes sense to not
> +		 * enable the comparator at probe.
> +		 */
> +		return regmap_set_bits(d->map, BD79124_REG_GEN_CFG,
> +				      BD79124_MASK_DWC_EN);
> +	}
> +
> +	return bd79124_disable_event(d, dir, chan->channel);
> +}
> +
> +static int bd79124_write_event_value(struct iio_dev *idev,
> +				     const struct iio_chan_spec *chan,
> +				     enum iio_event_type type,
> +				     enum iio_event_direction dir,
> +				     enum iio_event_info info, int val,
> +				     int val2)
> +{
> +	struct bd79124_data *d = iio_priv(idev);
> +	int reg;
> +
> +	if (chan->channel >= BD79124_MAX_NUM_CHANNELS)
> +		return -EINVAL;
> +
> +	/* ensure pinmux is set to ADC */
> +	if (!bd79124_chan_is_adc(d, chan->channel))
> +		return -EBUSY;
> +
> +	switch (info) {
> +	case IIO_EV_INFO_VALUE:
> +		if (dir == IIO_EV_DIR_RISING)
> +			reg = BD79124_GET_HIGH_LIMIT_REG(chan->channel);
> +		else if (dir == IIO_EV_DIR_FALLING)
> +			reg = BD79124_GET_LOW_LIMIT_REG(chan->channel);
> +		else
> +			return -EINVAL;
> +
> +		return bd79124_write_int_to_reg(d, reg, val);
> +
> +	case IIO_EV_INFO_HYSTERESIS:
> +			reg = BD79124_GET_HYSTERESIS_REG(chan->channel);
> +			val >>= 3;
> +
> +		return regmap_update_bits(d->map, reg, BD79124_MASK_HYSTERESIS,
> +					  val);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int bd79124_read_last_result(struct bd79124_data *d, int chan,
> +				    int *result)
> +{
> +	struct bd79124_raw raw;
> +	int ret;
> +
> +	ret = regmap_bulk_read(d->map, BD79124_GET_RECENT_RES_REG(chan), &raw,
> +			       sizeof(raw));
> +	if (ret)
> +		return ret;
> +
> +	*result = BD79124_RAW_TO_INT(raw);
> +
> +	return 0;
> +}
> +

> +static int bd79124_single_chan_seq_end(struct bd79124_data *d, int old)
> +{
> +	int ret;
> +
> +	/* Stop the sequencer */

Fairly obvious from code. Maybe drop these sequencer related comments.

> +	ret = regmap_clear_bits(d->map, BD79124_REG_SEQUENCE_CFG,
> +				BD79124_MASK_SEQ_START);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(d->map, BD79124_REG_AUTO_CHANNELS, old);
> +	if (ret)
> +		return ret;
> +
> +	/* Restart the sequencer */
> +	return regmap_set_bits(d->map, BD79124_REG_SEQUENCE_CFG,
> +			      BD79124_MASK_SEQ_START);
> +}
> +
> +static int bd79124_read_raw(struct iio_dev *idev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long m)
> +{
> +	struct bd79124_data *d = iio_priv(idev);
> +	int ret;
> +
> +	if (chan->channel >= BD79124_MAX_NUM_CHANNELS)
> +		return -EINVAL;
> +
> +	switch (m) {
> +	case IIO_CHAN_INFO_RAW:
> +	{
> +		int old_chan_cfg, tmp;
> +		int regval = FIELD_PREP(BD79124_MASK_CONV_MODE,
> +					BD79124_CONV_MODE_AUTO);
> +
> +		guard(mutex)(&d->mutex);
> +
> +		/* ensure pinmux is set to ADC */
> +		if (!bd79124_chan_is_adc(d, chan->channel))
> +			return -EBUSY;

If you are exposing the channel in sysfs it better not be set to be
a GPIO.  You need to generate the chan_spec from what is wired
for ADC usage vs what is wired for GPIO.

> +
> +		/*
> +		 * Start the automatic conversion. This is needed here if no
> +		 * events have been enabled.
> +		 */
> +		ret = regmap_update_bits(d->map, BD79124_REG_OPMODE_CFG,
> +			BD79124_MASK_CONV_MODE, regval);
		ret = regmap_update_bits(d->map, BD79124_REG_OPMODE_CFG,
				         BD79124_MASK_CONV_MODE,
					 FIELD_PREP(BD79124_MASK_CONV_MODE,
> +						    BD79124_CONV_MODE_AUTO));

Is a tiny bit long but meh, it keeps the information all in one place so
I think still worth doing it in one go. If not, just set regval the line
above this call.

> +		if (ret)
> +			return ret;
> +
> +		ret = bd79124_single_chan_seq(d, chan->channel, &old_chan_cfg);
> +		if (ret)
> +			return ret;
> +
> +		/*
> +		 * The maximum conversion time is 6 uS. Ensure the sample is
> +		 * ready
The second bit not needed.
		/* Maximum conversion time is 6 usecs */
is fine.

> +		 */
> +		udelay(6);
> +
> +		ret = bd79124_read_last_result(d, chan->channel, val);
> +		/* Try unconditionally returning the chan config */

We can see the return to old config from the code. Comment should say why instead.

> +		tmp = bd79124_single_chan_seq_end(d, old_chan_cfg);
> +		if (tmp)
> +			dev_err(d->dev,
> +				"Failed to return config. Alarms may be disabled\n");
> +
> +		if (ret)
> +			return ret;
> +
> +		return IIO_VAL_INT;
> +	}
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = d->vmax / 1000;
> +		*val2 = BD79124_ADC_BITS;
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	default:
> +		return -EINVAL;
> +	}
> +}

> +static irqreturn_t bd79124_event_handler(int irq, void *priv)
> +{
> +	int ret, i_hi, i_lo, i;
> +	struct iio_dev *idev = priv;
> +	struct bd79124_data *d = iio_priv(idev);
> +
> +	/*
> +	 * Return IRQ_NONE if bailing-out without acking. This allows the IRQ
> +	 * subsystem to disable the offending IRQ line if we get a hardware
> +	 * problem. This behaviour has saved my poor bottom a few times in the
> +	 * past as, instead of getting unusably unresponsive, the system has
> +	 * spilled out the magic words "...nobody cared".
*laughs*.  Maybe the comment isn't strictly necessary but it cheered
up my Friday.
> +	 */
> +	ret = regmap_read(d->map, BD79124_REG_EVENT_FLAG_HI, &i_hi);
> +	if (ret)
> +		return IRQ_NONE;
> +
> +	ret = regmap_read(d->map, BD79124_REG_EVENT_FLAG_LO, &i_lo);
> +	if (ret)
> +		return IRQ_NONE;
> +
> +	if (!i_lo && !i_hi)
> +		return IRQ_NONE;
> +
> +	for (i = 0; i < BD79124_MAX_NUM_CHANNELS; i++) {
> +		u64 ecode;
> +
> +		if (BIT(i) & i_hi) {
Maybe cleaner as a pair of

for_each_set_bit() loops.

> +			ecode = IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, i,
> +					IIO_EV_TYPE_THRESH, IIO_EV_DIR_RISING);
> +
> +			iio_push_event(idev, ecode, d->timestamp);
> +			/*
> +			 * The BD79124 keeps the IRQ asserted for as long as
> +			 * the voltage exceeds the threshold. It may not serve
> +			 * the purpose to keep the IRQ firing and events
> +			 * generated in a loop because it may yield the
> +			 * userspace to have some problems when event handling
> +			 * there is slow.
> +			 *
> +			 * Thus, we disable the event for the channel. Userspace
> +			 * needs to re-enable the event.

That's not pretty. So I'd prefer a timeout and autoreenable if we can.

> +			 *
> +			 * We don't check the result as there is not much to do.
> +			 * Also, this should not lead to total IRQ storm
> +			 * because the BD79124 is running in autonomous mode,
> +			 * which means there is by minimum 0.75 mS idle time
> +			 * between changing the channels. That should be
> +			 * sufficient to show some life on system, even if the
> +			 * event handling couldn't keep up.
> +			 */
> +			bd79124_disable_event(d, IIO_EV_DIR_RISING, i);
> +		}
> +		if (BIT(i) & i_lo) {
> +			ecode = IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, i,
> +					IIO_EV_TYPE_THRESH, IIO_EV_DIR_FALLING);
> +
> +			iio_push_event(idev, ecode, d->timestamp);
The same interrupt thing doesn't apply to falling? That's odd.

> +		}
> +	}
> +
> +	ret = regmap_write(d->map, BD79124_REG_EVENT_FLAG_HI, i_hi);
> +	if (ret)
> +		return IRQ_NONE;
> +
> +	ret = regmap_write(d->map, BD79124_REG_EVENT_FLAG_LO, i_lo);
> +	if (ret)
> +		return IRQ_NONE;
> +
> +	return IRQ_HANDLED;
> +}

> +static int bd79124_hw_init(struct bd79124_data *d)
> +{
> +	int ret, regval;
> +
> +	/* Stop auto sequencer */
> +	ret = regmap_clear_bits(d->map, BD79124_REG_SEQUENCE_CFG,
> +				BD79124_MASK_SEQ_START);
> +	if (ret)
> +		return ret;
> +
> +	/* Enable writing the measured values to the regsters */
> +	ret = regmap_set_bits(d->map, BD79124_REG_GEN_CFG,
> +				 BD79124_MASK_STATS_EN);

Check alignment of that second line. Generally align to just after (

> +	if (ret)
> +		return ret;
> +
> +	/* Set no channels to be auto-measured */
> +	ret = regmap_write(d->map, BD79124_REG_AUTO_CHANNELS, 0x0);
> +	if (ret)
> +		return ret;
> +
> +	/* Set no channels to be manually measured */
> +	ret = regmap_write(d->map, BD79124_REG_MANUAL_CHANNELS, 0x0);
> +	if (ret)
> +		return ret;
> +
> +	/* Set the measurement interval to 0.75 mS */
> +
> +	regval = FIELD_PREP(BD79124_MASK_AUTO_INTERVAL, BD79124_INTERVAL_075);
> +	ret = regmap_update_bits(d->map, BD79124_REG_OPMODE_CFG,
> +			BD79124_MASK_AUTO_INTERVAL, regval);
> +	if (ret)
> +		return ret;
> +
> +	/* Sequencer mode to auto */
> +	ret = regmap_set_bits(d->map, BD79124_REG_SEQUENCE_CFG,
> +			      BD79124_MASK_SEQ_SEQ);
> +	if (ret)
> +		return ret;
> +
> +	regval = FIELD_PREP(BD79124_MASK_CONV_MODE, BD79124_CONV_MODE_MANSEQ);
> +	/* Don't start the measurement */
> +	return regmap_update_bits(d->map, BD79124_REG_OPMODE_CFG,
> +			BD79124_MASK_CONV_MODE, BD79124_CONV_MODE_MANSEQ);
> +
> +}
> +
> +#define BD79124_VDD_MAX 5250000
> +#define BD79124_VDD_MIN 2700000
They are real numbers, so if you need them but them values inline.

> +
> +static int bd79124_probe(struct platform_device *pdev)
> +{
> +	struct bd79124_data *d;
> +	struct iio_dev *idev;
Maybe copy more common naming  iio_dev, indio_dev etc
just for familiarity.  For device data, d is a little brief!
> +	int ret, irq, *parent_data;
> +
> +	idev = devm_iio_device_alloc(&pdev->dev, sizeof(*d));
> +	if (!idev)
> +		return -ENOMEM;
> +
> +	d = iio_priv(idev);
> +
> +	parent_data = dev_get_drvdata(pdev->dev.parent);
> +	if (!parent_data)
> +		return dev_err_probe(&pdev->dev, -EINVAL,
> +				     "reference voltage missing\n");
> +
> +	/*
> +	 * Recommended VDD voltage from the data-sheet:
> +	 * Analog/Digital Supply Voltage VDD 2.70 - 5.25 V
> +	 */
> +	d->vmax = *parent_data;
> +	if (d->vmax < BD79124_VDD_MIN || d->vmax > BD79124_VDD_MAX) {

Normally we consider this 'not our problem'.  If someone has wired
their system wrong we don't bother to tell them.  I don't mind having
the check though if you find it particularly useful.

> +		return dev_err_probe(d->dev, -EINVAL,
> +				     "VDD (%d) out of range [%d - %d]\n",
> +				     d->vmax, BD79124_VDD_MIN, BD79124_VDD_MAX);
> +
> +		return -EINVAL;
Already returned.

> +	}
> +
> +	irq = platform_get_irq_byname_optional(pdev, "thresh-alert");

Is there more than one?  If not why do we need to do it by name?

> +	if (irq < 0) {
> +		if (irq == -EPROBE_DEFER)
> +			return irq;
> +
> +		idev->channels = &bd79124_channels_noirq[0];
> +		idev->num_channels = ARRAY_SIZE(bd79124_channels_noirq);
> +		dev_dbg(d->dev, "No IRQ found, events disabled\n");
> +	} else {
> +		idev->channels = &bd79124_channels[0];
> +		idev->num_channels = ARRAY_SIZE(bd79124_channels);
> +	}
> +
> +	idev->info = &bd79124_info;
> +	idev->name = "bd79124";
> +	idev->modes = INDIO_DIRECT_MODE;
> +
> +	d->dev = &pdev->dev;
> +	d->map = dev_get_regmap(d->dev->parent, NULL);
> +	if (!d->map)
> +		return dev_err_probe(d->dev, -ENODEV, "No regmap\n");
> +
> +	mutex_init(&d->mutex);
> +
> +	ret = bd79124_hw_init(d);
> +	if (ret)
> +		return ret;
> +
> +	if (irq > 0) {
> +		ret = devm_request_threaded_irq(d->dev, irq, bd79124_irq_handler,
> +					&bd79124_event_handler, IRQF_ONESHOT,
> +					"adc-thresh-alert", idev);
> +		if (ret)
> +			return dev_err_probe(d->dev, ret,
> +					     "Failed to register IRQ\n");
> +	}
> +
> +	return devm_iio_device_register(d->dev, idev);
> +}

Re: [RFC PATCH 0/5] Support ROHM BD79124 ADC/GPO

[Matti Vaittinen]

Hi Jonathan,

Thanks a ton for the help! :)

On 31/01/2025 19:08, Jonathan Cameron wrote:
> On Fri, 31 Jan 2025 15:34:43 +0200
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> Support ROHM BD79124 ADC.
>>
>> Quite usual stuff. 12-bit, 8-channel ADC with threshold monitoring.
>>
>> Except that:
>>   - each ADC input pin can be configured as a general purpose output.
>>   - manually starting an ADC conversion and reading the result would
>>     require the I2C _master_ to do clock stretching(!) for the duration
>>     of the conversion... Let's just say this is not well supported.
>>   - IC supports 'autonomous measurement mode' and storing latest results
>>     to the result registers. This mode is used by the driver due to the
>>     "peculiar" I2C when doing manual reads.
>>
>> I sent this as an RFC because I implemented the pin purposing (GPO/ADC)
>> using pinmux - which I've never done for upstream stuff before. Hence
>> it's better to ask if this makes sense, or if there is better way to go.
>> Anyways, resulted drivers spread to 3 subsystems (MFD, pinctrl and IIO)
> In principle nothing against pin mux for this.
> There are other options though if pin mux ends up being too complex.
> 
> - provide ADC channels in the binding channel@x etc.
> Anything else is freely available as a GPIO.
> Normal GPIO bindings etc for those.
> 
> The channel bit is common on SoC ADC anyway where we don't want to
> expose channels that aren't wired out.

Thanks for the insight on how things are usually done :)

I think the only reason for having all the channels visible in IIO, 
could be, if there was a need to provide a runtime configuration.

> For combined ADC GPIO chips we normally don't bother with an MFD.
> Just host the gpio driver in the ADC one unless there is a strong
> reasons someone will put this down for GPIO usage only.

I don't really know about that. I don't like arguing, yet I seem to do 
that all the time XD

I personally like using MFD and having smaller drivers in relevant 
subsystems, because it tends to keep the drivers leaner - and allows 
re-use of drivers when some of the hardware blocks are re-used. In some 
cases this results (much) cleaner drivers.

(Let's assume they did "new" ADC, and just dropped the GPO from it. With 
the MFD the deal is to add new compatible, and have an MFD cell array 
without the pinctrl/GPO matching this new device. And lets imagine they 
later add this ADC to a PMIC. We add yet another MFD cell array for this 
new device, with a cell for the regulators, power-supply and the ADC... 
The same platform subdevice can be re-used to drive ADC (well, with 
added register offsets)).

Allright. I believe you have more experience on this area than I do, but 
I definitely think MFD has it's merits also for ADCs - they do tend to 
put ADCs to all kinds of devices (like in PMICs after all, although 
maybe not with 8 channels and less often without an accumulator).

>> Furthermore, the GPO functionality has not been (properly) tested. I'll
>> do more testing for v2 if this pinmux approach is appropriate.

I took a better look at the pinctrl docs while listening the FOSDEM 
talks :) (Which inevitably means I missed a few things from some of the 
presentations, and also didn't really properly understand what I was 
reading. "Multipasking..." like some rude Finns might say.)

Anyways, I think the pinctrl should have some out-of-the-box support for 
use-cases where pin(s) can be used for GPIO, and for an another 
function. (I think, I saw functions which take care of the pins having 
right state for GPIO use). I don't think I properly used those features.

>> Furthermore, because the ADC uses this continuous autonomous measuring,
>> and because the IC keeps producing new 'out of window' IRQs if
>> measurements are out of window - the driver disables the event when
>> sending one. This prevents generating storm of events, but it also
>> requires users to reconfigure / re-enable an event if they wish to
>> continue monitoring after receiving one. Again I am not sure if this is
>> the best way to handle such HW - so better to ask for an opinion than a
>> nose bleed, right? Maybe the next version will no longer be a RFC :)
> 
> Oddly I thought we had ABI for this but not finding it.
> We basically want a thing that lets us say don't allow a repeat event
> for X seconds. Then we set a timer and reenable the interrupt after that
> time.  I think there are drivers doing this but can't find one right
> now :(  It's close to _timeout used for gesture detection.

So, a good old timer for doing unmasking. I think this makes sense if 
the existing users of ADCs aren't prepared for the events to get 
disabled by driver. Thanks! I'll follow this suggestion :)

Yours,
   -- Matti

Re: [RFC PATCH 3/5] iio: adc: Support ROHM BD79124 ADC

[Matti Vaittinen]

On 31/01/2025 19:41, Jonathan Cameron wrote:
> On Fri, 31 Jan 2025 15:37:48 +0200
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> The ROHM BD79124 is a 12-bit, 8-channel, SAR ADC. The ADC supports
>> an automatic measurement mode, with an alarm interrupt for out-of-window
>> measurements. The window is configurable for each channel.
>>
>> The I2C protocol for manual start of the measurement and data reading is
>> somewhat peculiar. It requires the master to do clock stretching after
>> sending the I2C slave-address until the slave has captured the data.
>> Needless to say this is not well suopported by the I2C controllers.
> 
>  From what I recall that is in the I2C spec, so in theory should be supported.
> Ah well.

Could be I am mistaken then. Or, maybe I just misused the term "master 
to do clock stretching".

I know that it is not rare the slave device is keeping the clock down 
for extended period (in this case so that the measurement would be 
completed) - but at least I am not aware of any APIs which could be used 
to cause the _master_ side to keep the SCL low for an extended period 
after receiving the ACK (after sending the slave address). In this case 
it would require this driver to be able to set a time for how long the 
master would keep SCL low after sensing the slave address, before 
sending the "command" bytes.

|S|ADDRESS+R|a|STRETCH|8-bit-i2c-frame|A|8-bit-i2c-frame|A|STRETCH|8-bit-i2c...

Above denotes this "master stretching". CAPITALs are initiated by 
master, lowercase by slave. S, is start, a is ack and R is read-bit.

If there is a standard way to implement this in Linux side, then I might 
consider using it as it'd allowed much higher capture rates.

>> It is worth noting that the ADC input pins can be also configured as
>> general purpose outputs. The pin mode should be configured using pincmux
>> driver.
> 
> We shouldn't be presenting channels that are configure for GPIOs as
> ADC channels.  It is very rare that there is a usecase for any
> dynamic switching.

Thanks :) If the dynamic switching is rare, then you're definitely 
right. I need to see if using the pinmux still makes sense, and if we 
can implement this while using (separate) pinmux driver.

> Normally it's a case of what is wired and
> so static.

I should implement a device which can be controlled via it's analog 
output line :) If nothing else then a device shutting down when it's 
output is pulled low ;)

...Well, I have no real use-case for dynamic config either.

>  Hence build the iio_chan_spec array for just the
> channels you want, not the the lot.  Channel sub nodes in the
> DT are how we most commonly specify what is wired.

Hmm. That'd mean the ADC channels _must_ be defined in DT in order to be 
usable(?) Well, if this is the usual way, then it should be well known 
by users. Thanks.

...

>> +		if (BIT(i) & i_lo) {
>> +			ecode = IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, i,
>> +					IIO_EV_TYPE_THRESH, IIO_EV_DIR_FALLING);
>> +
>> +			iio_push_event(idev, ecode, d->timestamp);
> The same interrupt thing doesn't apply to falling? That's odd.
> 

It does. So, not odd, just a regular bug :) Thanks!

>> +		}
>> +	}
>> +

...

>> +
>> +	irq = platform_get_irq_byname_optional(pdev, "thresh-alert");
> 
> Is there more than one?  If not why do we need to do it by name?

I kind of like doing it by name when the IRQs come from a MFD device - 
which can name them. It is no real extra cost (well, maybe bytes of the 
added string in kernel, but I guess it's not relevant with only few 
interrupts) - but it makes it very hard to get it wrong, and it 
documents the purpose of the IRQ.

>> +	if (irq < 0) {
>> +		if (irq == -EPROBE_DEFER)
>> +			return irq;

...

I do thank you for, and agree with, all the rest of the comments!

Yours,
	-- Matti

Re: [RFC PATCH 2/5] mfd: Add ROHM BD79124 ADC/GPO

[Matti Vaittinen]

On 31/01/2025 19:14, Jonathan Cameron wrote:
> On Fri, 31 Jan 2025 15:37:06 +0200
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> Add core driver for the ROHM BD79124 ADC / GPO.
>>
>> The core driver launches the sub-drivers for the pinmux/GPO and for the
>> IIO ADC. It also provides the regmap, and forwards the IRQ resource to
>> the ADC.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> As per response in cover letter. This is a common device combination and so
> far I don't think we ever bothered with an MFD. Lots of ADCs provide
> GPIO chips as well so I'd just squash it into the ADC driver.

You may be right with this. I still need to digest this a bit as I 
explaned in the cover letter discussion :)

All of your inline comments were valid and I agree with them. Noticing 
the single IC limitation was great!

I'll address all the findings if I keep the MFD approach.

Yours,
	-- Matti

Re: [RFC PATCH 3/5] iio: adc: Support ROHM BD79124 ADC

[Jonathan Cameron]

On Sat, 1 Feb 2025 17:38:20 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 31/01/2025 19:41, Jonathan Cameron wrote:
> > On Fri, 31 Jan 2025 15:37:48 +0200
> > Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> >   
> >> The ROHM BD79124 is a 12-bit, 8-channel, SAR ADC. The ADC supports
> >> an automatic measurement mode, with an alarm interrupt for out-of-window
> >> measurements. The window is configurable for each channel.
> >>
> >> The I2C protocol for manual start of the measurement and data reading is
> >> somewhat peculiar. It requires the master to do clock stretching after
> >> sending the I2C slave-address until the slave has captured the data.
> >> Needless to say this is not well suopported by the I2C controllers.  
> > 
> >  From what I recall that is in the I2C spec, so in theory should be supported.
> > Ah well.  
> 
> Could be I am mistaken then. Or, maybe I just misused the term "master 
> to do clock stretching".
> 
> I know that it is not rare the slave device is keeping the clock down 
> for extended period (in this case so that the measurement would be 
> completed) - but at least I am not aware of any APIs which could be used 
> to cause the _master_ side to keep the SCL low for an extended period 
> after receiving the ACK (after sending the slave address). In this case 
> it would require this driver to be able to set a time for how long the 
> master would keep SCL low after sensing the slave address, before 
> sending the "command" bytes.
> 
> |S|ADDRESS+R|a|STRETCH|8-bit-i2c-frame|A|8-bit-i2c-frame|A|STRETCH|8-bit-i2c...
> 
> Above denotes this "master stretching". CAPITALs are initiated by 
> master, lowercase by slave. S, is start, a is ack and R is read-bit.

Ah. That is indeed more unusual. You were correct that i was thinking
of the client side doing the stretching!

> 
> If there is a standard way to implement this in Linux side, then I might 
> consider using it as it'd allowed much higher capture rates.
Not that I'm aware of. 

Wolfram, have you seen anything like this?

> 
> >> It is worth noting that the ADC input pins can be also configured as
> >> general purpose outputs. The pin mode should be configured using pincmux
> >> driver.  
> > 
> > We shouldn't be presenting channels that are configure for GPIOs as
> > ADC channels.  It is very rare that there is a usecase for any
> > dynamic switching.  
> 
> Thanks :) If the dynamic switching is rare, then you're definitely 
> right. I need to see if using the pinmux still makes sense, and if we 
> can implement this while using (separate) pinmux driver.
> 
> > Normally it's a case of what is wired and
> > so static.  
> 
> I should implement a device which can be controlled via it's analog 
> output line :) If nothing else then a device shutting down when it's 
> output is pulled low ;)
> 
> ...Well, I have no real use-case for dynamic config either.
> 
> >  Hence build the iio_chan_spec array for just the
> > channels you want, not the the lot.  Channel sub nodes in the
> > DT are how we most commonly specify what is wired.  
> 
> Hmm. That'd mean the ADC channels _must_ be defined in DT in order to be 
> usable(?) Well, if this is the usual way, then it should be well known 
> by users. Thanks.

Yes. We basically have two types of binding wrt to channels.
1) Always there - no explicit binding, but also no way to describe
   anything specific about the channels.
2) Subnode per channel with stuff from adc.yaml and anything device
   specific.  Only channels that that have a node are enabled.

There are a few drivers that for historical reasons support both
options with 'no channels' meaning 'all channels'.

J

Re: [RFC PATCH 0/5] Support ROHM BD79124 ADC/GPO

[Jonathan Cameron]

On Sat, 1 Feb 2025 17:00:51 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> Hi Jonathan,
> 
> Thanks a ton for the help! :)
> 
> On 31/01/2025 19:08, Jonathan Cameron wrote:
> > On Fri, 31 Jan 2025 15:34:43 +0200
> > Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> >   
> >> Support ROHM BD79124 ADC.
> >>
> >> Quite usual stuff. 12-bit, 8-channel ADC with threshold monitoring.
> >>
> >> Except that:
> >>   - each ADC input pin can be configured as a general purpose output.
> >>   - manually starting an ADC conversion and reading the result would
> >>     require the I2C _master_ to do clock stretching(!) for the duration
> >>     of the conversion... Let's just say this is not well supported.
> >>   - IC supports 'autonomous measurement mode' and storing latest results
> >>     to the result registers. This mode is used by the driver due to the
> >>     "peculiar" I2C when doing manual reads.
> >>
> >> I sent this as an RFC because I implemented the pin purposing (GPO/ADC)
> >> using pinmux - which I've never done for upstream stuff before. Hence
> >> it's better to ask if this makes sense, or if there is better way to go.
> >> Anyways, resulted drivers spread to 3 subsystems (MFD, pinctrl and IIO)  
> > In principle nothing against pin mux for this.
> > There are other options though if pin mux ends up being too complex.
> > 
> > - provide ADC channels in the binding channel@x etc.
> > Anything else is freely available as a GPIO.
> > Normal GPIO bindings etc for those.
> > 
> > The channel bit is common on SoC ADC anyway where we don't want to
> > expose channels that aren't wired out.  
> 
> Thanks for the insight on how things are usually done :)
> 
> I think the only reason for having all the channels visible in IIO, 
> could be, if there was a need to provide a runtime configuration.
> 
> > For combined ADC GPIO chips we normally don't bother with an MFD.
> > Just host the gpio driver in the ADC one unless there is a strong
> > reasons someone will put this down for GPIO usage only.  
> 
> I don't really know about that. I don't like arguing, yet I seem to do 
> that all the time XD
> 
> I personally like using MFD and having smaller drivers in relevant 
> subsystems, because it tends to keep the drivers leaner - and allows 
> re-use of drivers when some of the hardware blocks are re-used. In some 
> cases this results (much) cleaner drivers.

I'm fully in agreement with MFD being useful, but for very simple
parts of a device it can be overkill. 
> 
> (Let's assume they did "new" ADC, and just dropped the GPO from it. With 
> the MFD the deal is to add new compatible, and have an MFD cell array 
> without the pinctrl/GPO matching this new device. And lets imagine they 
> later add this ADC to a PMIC. We add yet another MFD cell array for this 
> new device, with a cell for the regulators, power-supply and the ADC... 
> The same platform subdevice can be re-used to drive ADC (well, with 
> added register offsets)).
> 
> Allright. I believe you have more experience on this area than I do, but 
> I definitely think MFD has it's merits also for ADCs - they do tend to 
> put ADCs to all kinds of devices (like in PMICs after all, although 
> maybe not with 8 channels and less often without an accumulator).

It's a trade off.  Sometimes we just have a little code duplication
to the need for a more complex design.

Enjoy the rest of Fosdem

Jonathan

Re: [RFC PATCH 0/5] Support ROHM BD79124 ADC/GPO

[Matti Vaittinen]

On 01/02/2025 18:30, Jonathan Cameron wrote:
>
> Enjoy the rest of Fosdem

Thanks! Just watching the live streams though :(

Yours,
	-- Matti

Re: [RFC PATCH 2/5] mfd: Add ROHM BD79124 ADC/GPO

[Matti Vaittinen]

On 31/01/2025 15:37, Matti Vaittinen wrote:
> Add core driver for the ROHM BD79124 ADC / GPO.
> 
> The core driver launches the sub-drivers for the pinmux/GPO and for the
> IIO ADC. It also provides the regmap, and forwards the IRQ resource to
> the ADC.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

Just a note to reviewers - I dropped the MFD from v2. No need to review 
this any further.

Thanks!

Yours,
	-- Matti

Re: [RFC PATCH 4/5] pinctrl: Support ROHM BD79124 pinmux / GPO

[Matti Vaittinen]

On 31/01/2025 15:38, Matti Vaittinen wrote:
> The ROHM BD79124 is a 12-bit, 8-channel, SAR ADC. The AIN pins can be
> used as ADC inputs, or as general purpose outputs.
> 
> Support changing pin function (GPO / ADC) and the gpo output control.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> ---
> 
> NOTE: This patch is not properly tested. More thorough testing is to be
> done prior v2 if this pinmux approach makes sense.

Just a note to reviewers - I dropped the pinmux from v2. No need to 
review this any further.

Thanks!

Yours,
	-- Matti

Re: [RFC PATCH 3/5] iio: adc: Support ROHM BD79124 ADC

[Matti Vaittinen]

On 31/01/2025 19:41, Jonathan Cameron wrote:
> On Fri, 31 Jan 2025 15:37:48 +0200
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> The ROHM BD79124 is a 12-bit, 8-channel, SAR ADC. The ADC supports
>> an automatic measurement mode, with an alarm interrupt for out-of-window
>> measurements. The window is configurable for each channel.
>>

Hi Jonathan,

I just sent the v2, where I (think I) addressed all comments except ones 
below. Just wanted to point out what was not changed and why :)

...

> 
>> +struct bd79124_raw {
>> +	u8 bit0_3; /* Is set in high bits of the byte */
>> +	u8 bit4_11;
>> +};
>> +#define BD79124_RAW_TO_INT(r) ((r.bit4_11 << 4) | (r.bit0_3 >> 4))
> You could do this as an endian conversion and a single shift I think.
> Might be slightly simpler.

I kept this struct with bytes matching the register spec. Doing the 
endian conversion and then shifting would probably have worked, but my 
head hurts when I try thinking how the bits settle there. Especially if 
this is done on a big-endian machine. I can rework this for v3 if you 
feel very strongly about this.

...

> 
>> +static irqreturn_t bd79124_event_handler(int irq, void *priv)
>> +{
>> +	int ret, i_hi, i_lo, i;
>> +	struct iio_dev *idev = priv;
>> +	struct bd79124_data *d = iio_priv(idev);
>> +
>> +	/*
>> +	 * Return IRQ_NONE if bailing-out without acking. This allows the IRQ
>> +	 * subsystem to disable the offending IRQ line if we get a hardware
>> +	 * problem. This behaviour has saved my poor bottom a few times in the
>> +	 * past as, instead of getting unusably unresponsive, the system has
>> +	 * spilled out the magic words "...nobody cared".
> *laughs*.  Maybe the comment isn't strictly necessary but it cheered
> up my Friday.
>> +	 */
>> +	ret = regmap_read(d->map, BD79124_REG_EVENT_FLAG_HI, &i_hi);
>> +	if (ret)
>> +		return IRQ_NONE;
>> +
>> +	ret = regmap_read(d->map, BD79124_REG_EVENT_FLAG_LO, &i_lo);
>> +	if (ret)
>> +		return IRQ_NONE;
>> +
>> +	if (!i_lo && !i_hi)
>> +		return IRQ_NONE;
>> +
>> +	for (i = 0; i < BD79124_MAX_NUM_CHANNELS; i++) {
>> +		u64 ecode;
>> +
>> +		if (BIT(i) & i_hi) {
> Maybe cleaner as a pair of
> 
> for_each_set_bit() loops.
> 

I kept the original for 2 reasons.

1. the main reason is that the for_each_set_bit() would want the value 
read from a register to be in long. Regmap wants to use int. Solving 
this produced (in my 'humblish' opinion) less readable code.

2. The current implementation has only one loop, which should perhaps be 
a tiny bit more efficient.

>> +			ecode = IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, i,
>> +					IIO_EV_TYPE_THRESH, IIO_EV_DIR_RISING);
>> +
>> +			iio_push_event(idev, ecode, d->timestamp);
>> +			/*
>> +			 * The BD79124 keeps the IRQ asserted for as long as
>> +			 * the voltage exceeds the threshold. It may not serve
>> +			 * the purpose to keep the IRQ firing and events
>> +			 * generated in a loop because it may yield the
>> +			 * userspace to have some problems when event handling
>> +			 * there is slow.
>> +			 *
>> +			 * Thus, we disable the event for the channel. Userspace
>> +			 * needs to re-enable the event.
> 
> That's not pretty. So I'd prefer a timeout and autoreenable if we can.

And I did this, but with constant 1 sec 'grace time' instead of 
modifiable time-out. I believe this suffices and keeps it simpler.


Yours,
	-- Matti

Re: [RFC PATCH 4/5] pinctrl: Support ROHM BD79124 pinmux / GPO

[Linus Walleij]

On Wed, Feb 5, 2025 at 2:40 PM Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> On 31/01/2025 15:38, Matti Vaittinen wrote:
> > The ROHM BD79124 is a 12-bit, 8-channel, SAR ADC. The AIN pins can be
> > used as ADC inputs, or as general purpose outputs.
> >
> > Support changing pin function (GPO / ADC) and the gpo output control.
> >
> > Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> > ---
> >
> > NOTE: This patch is not properly tested. More thorough testing is to be
> > done prior v2 if this pinmux approach makes sense.
>
> Just a note to reviewers - I dropped the pinmux from v2. No need to
> review this any further.

Why? Gave up on the idea or want to pursue it later?

Yours,
Linus Walleij

Re: [RFC PATCH 4/5] pinctrl: Support ROHM BD79124 pinmux / GPO

[Matti Vaittinen]

Hi Linus,

On 06/02/2025 11:39, Linus Walleij wrote:
> On Wed, Feb 5, 2025 at 2:40 PM Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>> On 31/01/2025 15:38, Matti Vaittinen wrote:
>>> The ROHM BD79124 is a 12-bit, 8-channel, SAR ADC. The AIN pins can be
>>> used as ADC inputs, or as general purpose outputs.
>>>
>>> Support changing pin function (GPO / ADC) and the gpo output control.
>>>
>>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>>> ---
>>>
>>> NOTE: This patch is not properly tested. More thorough testing is to be
>>> done prior v2 if this pinmux approach makes sense.
>>
>> Just a note to reviewers - I dropped the pinmux from v2. No need to
>> review this any further.
> 
> Why? Gave up on the idea or want to pursue it later?

I took the (simpler) approach suggested by Jonathan. Eg, I just required 
all the ADC pins (channels) to be presented in the device-tree - and 
then treat rest of the pins as GPO. Hence, the pinmux driver is not 
really needed as the mux registers are initialized at the ADC driver 
probe when the DT is parsed. (I did also drop the MFD and bundled the 
GPO control in ADC driver).

Yours,
	-- Matti

Re: [RFC PATCH 4/5] pinctrl: Support ROHM BD79124 pinmux / GPO

[Matti Vaittinen]

On 06/02/2025 11:39, Linus Walleij wrote:
> On Wed, Feb 5, 2025 at 2:40 PM Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>> On 31/01/2025 15:38, Matti Vaittinen wrote:
>>> The ROHM BD79124 is a 12-bit, 8-channel, SAR ADC. The AIN pins can be
>>> used as ADC inputs, or as general purpose outputs.
>>>
>>> Support changing pin function (GPO / ADC) and the gpo output control.
>>>
>>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>>> ---
>>>
>>> NOTE: This patch is not properly tested. More thorough testing is to be
>>> done prior v2 if this pinmux approach makes sense.
>>
>> Just a note to reviewers - I dropped the pinmux from v2. No need to
>> review this any further.
> 
> Why? Gave up on the idea or want to pursue it later?

I just realized I should've shared the link to the v2 - which may not 
include all the recipients (because it no longer touches all the 
subsystems - and the get_maintainer.pl probably reduced the list of 
recipients). So, for anyone interested, here's the v2:

https://lore.kernel.org/all/cover.1738761899.git.mazziesaccount@gmail.com/

I do still appreciate all the reviews of the v2 even if it does not 
target subsystem you're specifically watching ;) But reviewing the RFC 
v1 patches does not make sense because the v2 dropped a few of them.

Yours,
	-- Matti

Re: [RFC PATCH 3/5] iio: adc: Support ROHM BD79124 ADC

[Jonathan Cameron]

On Wed, 5 Feb 2025 15:58:18 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 31/01/2025 19:41, Jonathan Cameron wrote:
> > On Fri, 31 Jan 2025 15:37:48 +0200
> > Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> >   
> >> The ROHM BD79124 is a 12-bit, 8-channel, SAR ADC. The ADC supports
> >> an automatic measurement mode, with an alarm interrupt for out-of-window
> >> measurements. The window is configurable for each channel.
> >>  
> 
> Hi Jonathan,
> 
> I just sent the v2, where I (think I) addressed all comments except ones 
> below. Just wanted to point out what was not changed and why :)
> 
> ...
> 
> >   
> >> +struct bd79124_raw {
> >> +	u8 bit0_3; /* Is set in high bits of the byte */
> >> +	u8 bit4_11;
> >> +};
> >> +#define BD79124_RAW_TO_INT(r) ((r.bit4_11 << 4) | (r.bit0_3 >> 4))  
> > You could do this as an endian conversion and a single shift I think.
> > Might be slightly simpler.  
> 
> I kept this struct with bytes matching the register spec. Doing the 
> endian conversion and then shifting would probably have worked, but my 
> head hurts when I try thinking how the bits settle there. Especially if 
> this is done on a big-endian machine. I can rework this for v3 if you 
> feel very strongly about this.

The key is that an endian conversion is always the same as OR + SHIFT
because that is being done in the system endiannes.

Doesn't matter that much, but we may see follow up patches switching
this over to the endian handlers.

From datasheet point of view it tends to depend on whether they show
an illustration of a bulk read or not to whether it's described
as a multi byte value, or as bits in smaller registers.

> 
> ...
> 
> >   
> >> +static irqreturn_t bd79124_event_handler(int irq, void *priv)
> >> +{
> >> +	int ret, i_hi, i_lo, i;
> >> +	struct iio_dev *idev = priv;
> >> +	struct bd79124_data *d = iio_priv(idev);
> >> +
> >> +	/*
> >> +	 * Return IRQ_NONE if bailing-out without acking. This allows the IRQ
> >> +	 * subsystem to disable the offending IRQ line if we get a hardware
> >> +	 * problem. This behaviour has saved my poor bottom a few times in the
> >> +	 * past as, instead of getting unusably unresponsive, the system has
> >> +	 * spilled out the magic words "...nobody cared".  
> > *laughs*.  Maybe the comment isn't strictly necessary but it cheered
> > up my Friday.  
> >> +	 */
> >> +	ret = regmap_read(d->map, BD79124_REG_EVENT_FLAG_HI, &i_hi);
> >> +	if (ret)
> >> +		return IRQ_NONE;
> >> +
> >> +	ret = regmap_read(d->map, BD79124_REG_EVENT_FLAG_LO, &i_lo);
> >> +	if (ret)
> >> +		return IRQ_NONE;
> >> +
> >> +	if (!i_lo && !i_hi)
> >> +		return IRQ_NONE;
> >> +
> >> +	for (i = 0; i < BD79124_MAX_NUM_CHANNELS; i++) {
> >> +		u64 ecode;
> >> +
> >> +		if (BIT(i) & i_hi) {  
> > Maybe cleaner as a pair of
> > 
> > for_each_set_bit() loops.
> >   
> 
> I kept the original for 2 reasons.
> 
> 1. the main reason is that the for_each_set_bit() would want the value 
> read from a register to be in long. Regmap wants to use int. Solving 
> this produced (in my 'humblish' opinion) less readable code.
> 
> 2. The current implementation has only one loop, which should perhaps be 
> a tiny bit more efficient.

OK.
> 
> >> +			ecode = IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, i,
> >> +					IIO_EV_TYPE_THRESH, IIO_EV_DIR_RISING);
> >> +
> >> +			iio_push_event(idev, ecode, d->timestamp);
> >> +			/*
> >> +			 * The BD79124 keeps the IRQ asserted for as long as
> >> +			 * the voltage exceeds the threshold. It may not serve
> >> +			 * the purpose to keep the IRQ firing and events
> >> +			 * generated in a loop because it may yield the
> >> +			 * userspace to have some problems when event handling
> >> +			 * there is slow.
> >> +			 *
> >> +			 * Thus, we disable the event for the channel. Userspace
> >> +			 * needs to re-enable the event.  
> > 
> > That's not pretty. So I'd prefer a timeout and autoreenable if we can.  
> 
> And I did this, but with constant 1 sec 'grace time' instead of 
> modifiable time-out. I believe this suffices and keeps it simpler.

We might want to present that value to userspace anyway at somepoint, but
a fixed value is fine.

Jonathan

> 
> 
> Yours,
> 	-- Matti

Re: [RFC PATCH 4/5] pinctrl: Support ROHM BD79124 pinmux / GPO

[Linus Walleij]

On Thu, Feb 6, 2025 at 11:09 AM Matti Vaittinen
<mazziesaccount@gmail.com> wrote:

> I just realized I should've shared the link to the v2 - which may not
> include all the recipients (because it no longer touches all the
> subsystems - and the get_maintainer.pl probably reduced the list of
> recipients). So, for anyone interested, here's the v2:
>
> https://lore.kernel.org/all/cover.1738761899.git.mazziesaccount@gmail.com/

Well it touches (uses) the gpio subsystem so the GPIO maintainers
should have been on CC...

This is one of the shortcomings of get_maintainers.pl really (also what
b4 is using): it does not know that if you use some specific APIs from
some specific .h files then some specific maintainers need to be on
CC.

It's because there is no hard rule: <linux/slab.h> - who cares? It's not
like the memory management people want to look at every user of
kmalloc()... <linux/gpio/driver.h> - this is a different story because
it's possible to get the semantics wrong.

That said, I looked at the patch in lore:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
for patch 4/5!

Linus

Re: [RFC PATCH 4/5] pinctrl: Support ROHM BD79124 pinmux / GPO

[Matti Vaittinen]

On 13/02/2025 13:53, Linus Walleij wrote:
> On Thu, Feb 6, 2025 at 11:09 AM Matti Vaittinen
> <mazziesaccount@gmail.com> wrote:
> 
>> I just realized I should've shared the link to the v2 - which may not
>> include all the recipients (because it no longer touches all the
>> subsystems - and the get_maintainer.pl probably reduced the list of
>> recipients). So, for anyone interested, here's the v2:
>>
>> https://lore.kernel.org/all/cover.1738761899.git.mazziesaccount@gmail.com/
> 
> Well it touches (uses) the gpio subsystem so the GPIO maintainers
> should have been on CC...
 > > This is one of the shortcomings of get_maintainers.pl really (also what
> b4 is using): it does not know that if you use some specific APIs from
> some specific .h files then some specific maintainers need to be on
> CC.
> 
> It's because there is no hard rule: <linux/slab.h> - who cares? It's not
> like the memory management people want to look at every user of
> kmalloc()... <linux/gpio/driver.h> - this is a different story because
> it's possible to get the semantics wrong.

This is a tough one. There are also a few other subsystems (besides mm) 
where maintainers can't stay on track for all the users. Also, AFAIR, 
some maintainers don't want to be CC'd by users of their subsystems, but 
only care the subsystem core changes. It's hard for an occasional 
contributor to know who to CC - it's often safest to just go with the 
get_maintainer.pl.

Still, I recognize the problem. I'm also trying to review users of some 
of the APIs I've added to IIO, lib or regulator subsystems. What I use 
is for this is lore + lei which fetch me mails with specific keywords 
(APIs / API prefixes / headers which I'm interested in) from the mail 
lists. Regular email filters could also do the job, but it'd required 
subscribing the lists which tend to quickly fill ones mailboxes.

Anyways, I believe it'd be best if maintainers who want to review users 
of their APIs did pick the mails with specific keywords from the lists. 
Maintainers know what they want to pick, (occasional) patch senders can 
only guess this ;)

> That said, I looked at the patch in lore:
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> for patch 4/5!

Thanks! I do appreciate all the reviews as usual!

Yours,
   -- Matti

Re: [RFC PATCH 4/5] pinctrl: Support ROHM BD79124 pinmux / GPO

[David Lechner]

On 2/13/25 5:53 AM, Linus Walleij wrote:
> On Thu, Feb 6, 2025 at 11:09 AM Matti Vaittinen
> <mazziesaccount@gmail.com> wrote:
> 
>> I just realized I should've shared the link to the v2 - which may not
>> include all the recipients (because it no longer touches all the
>> subsystems - and the get_maintainer.pl probably reduced the list of
>> recipients). So, for anyone interested, here's the v2:
>>
>> https://lore.kernel.org/all/cover.1738761899.git.mazziesaccount@gmail.com/
> 
> Well it touches (uses) the gpio subsystem so the GPIO maintainers
> should have been on CC...
> 
> This is one of the shortcomings of get_maintainers.pl really (also what
> b4 is using): it does not know that if you use some specific APIs from
> some specific .h files then some specific maintainers need to be on
> CC.

Can't we do that with `K:` in MAINTAINERS? 

I see:

K:	(devm_)?gpio_(request|free|direction|get|set)

to catch use of old gpio apis. Maybe should add 

K:	(devm_)?gpiochip_add_data

to catch anyone registering a gpio controller?