[PATCH v3 0/2] iio: adc: add Nuvoton NCT7201 ADC driver

[PATCH v3 0/2] iio: adc: add Nuvoton NCT7201 ADC driver

[Eason Yang]

Change since version 3:
 - Fix comments
 - Don't put nct720"x" in the name, just call it nct7201
 - Remove differential inputs until conversions are finished
 - Add NCT7201_ prefix in all macros and avoid the tables
 - Correct event threshold values in raw units
 - Add with and without interrupt callback function to have the event
   config part and one that doesn't
 - Remove print an error message if regmap_wirte failed case

Change since version 2:
 - Remvoe read-vin-data-size property, default use read word vin data
 - Use regmap instead of i2c smbus API
 - IIO should be IIO_CHAN_INFO_RAW and _SCALE not _PROCESSED
 - Use dev_xxx_probe in probe function and dev_xxx in other functions
 - Use devm_iio_device_register replace of iio_device_register
 - Use guard(mutex) replace of mutex_lock
 - Use get_unaligned_le16 conversion API

Changes since version 1:
 - Add new property in iio:adc binding document
 - Add new driver for Nuvoton NCT720x driver

Eason Yang (2):
  dt-bindings: iio: adc: Add binding for Nuvoton NCT720x ADCs
  iio: adc: add Nuvoton NCT7201 ADC driver

 .../bindings/iio/adc/nuvoton,nct7201.yaml     |  49 ++
 MAINTAINERS                                   |   2 +
 drivers/iio/adc/Kconfig                       |  10 +
 drivers/iio/adc/Makefile                      |   1 +
 drivers/iio/adc/nct7201.c                     | 449 ++++++++++++++++++
 5 files changed, 511 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/nuvoton,nct7201.yaml
 create mode 100644 drivers/iio/adc/nct7201.c

-- 
2.34.1

[PATCH v3 1/2] dt-bindings: iio: adc: Add binding for Nuvoton NCT720x ADCs

[Eason Yang]

Adds a binding specification for the Nuvoton NCT7201/NCT7202

Signed-off-by: Eason Yang <j2anfernee@gmail.com>
---
 .../bindings/iio/adc/nuvoton,nct7201.yaml     | 49 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 50 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/nuvoton,nct7201.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/nuvoton,nct7201.yaml b/Documentation/devicetree/bindings/iio/adc/nuvoton,nct7201.yaml
new file mode 100644
index 000000000000..08b52258e4af
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/nuvoton,nct7201.yaml
@@ -0,0 +1,49 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/nuvoton,nct7201.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Nuvoton nct7201 and similar ADCs
+
+maintainers:
+  - Eason Yang <j2anfernee@gmail.com>
+
+description: |
+   Family of ADCs with i2c interface.
+
+properties:
+  compatible:
+    enum:
+      - nuvoton,nct7201
+      - nuvoton,nct7202
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  reset-gpios:
+    description:
+      Reset pin for the device.
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        adc@1d {
+            compatible = "nuvoton,nct7202";
+            reg = <0x1d>;
+        };
+    };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 910305c11e8a..9c5328cbef17 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2792,6 +2792,7 @@ L:	openbmc@lists.ozlabs.org (moderated for non-subscribers)
 S:	Supported
 F:	Documentation/devicetree/bindings/*/*/*npcm*
 F:	Documentation/devicetree/bindings/*/*npcm*
+F:	Documentation/devicetree/bindings/iio/adc/nuvoton,nct7201.yaml
 F:	Documentation/devicetree/bindings/rtc/nuvoton,nct3018y.yaml
 F:	arch/arm/boot/dts/nuvoton/nuvoton-npcm*
 F:	arch/arm/mach-npcm/
-- 
2.34.1

Re: [PATCH v3 1/2] dt-bindings: iio: adc: Add binding for Nuvoton NCT720x ADCs

[Krzysztof Kozlowski]

On Thu, Dec 26, 2024 at 01:53:12PM +0800, Eason Yang wrote:
> Adds a binding specification for the Nuvoton NCT7201/NCT7202


I gave you link to exact line with exact text to use. Read it again and
use it, instead inventing your own wording. The documentation does not
say "Adds" but explicitly asks you to say "Add". Why using different?

Subject: nothing improved.

> 
> Signed-off-by: Eason Yang <j2anfernee@gmail.com>
> ---
>  .../bindings/iio/adc/nuvoton,nct7201.yaml     | 49 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 50 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/nuvoton,nct7201.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/nuvoton,nct7201.yaml b/Documentation/devicetree/bindings/iio/adc/nuvoton,nct7201.yaml
> new file mode 100644
> index 000000000000..08b52258e4af
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/nuvoton,nct7201.yaml
> @@ -0,0 +1,49 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/nuvoton,nct7201.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Nuvoton nct7201 and similar ADCs
> +
> +maintainers:
> +  - Eason Yang <j2anfernee@gmail.com>
> +
> +description: |
> +   Family of ADCs with i2c interface.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - nuvoton,nct7201
> +      - nuvoton,nct7202

Devices aren't compatible? Explain in the commit msg why they aren't or
use proper compatibility (oneOf, see numerous other bindings or example-schema).


> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  reset-gpios:
> +    description:
> +      Reset pin for the device.

Drop description, obvious.

> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        adc@1d {
> +            compatible = "nuvoton,nct7202";
> +            reg = <0x1d>;


Make the example complete: add interrupts and reset-gpios.

Best regards,
Krzysztof

Re: [PATCH v3 1/2] dt-bindings: iio: adc: Add binding for Nuvoton NCT720x ADCs

[Yu-Hsian Yang]

Dear Krzysztof,

Thanks for your comments.

Krzysztof Kozlowski <krzk@kernel.org> 於 2024年12月27日 週五 下午4:17寫道：
>
> On Thu, Dec 26, 2024 at 01:53:12PM +0800, Eason Yang wrote:
> > Adds a binding specification for the Nuvoton NCT7201/NCT7202
>
>
> I gave you link to exact line with exact text to use. Read it again and
> use it, instead inventing your own wording. The documentation does not
> say "Adds" but explicitly asks you to say "Add". Why using different?
>
> Subject: nothing improved.
>
> >
> > Signed-off-by: Eason Yang <j2anfernee@gmail.com>
> > ---
> >  .../bindings/iio/adc/nuvoton,nct7201.yaml     | 49 +++++++++++++++++++
> >  MAINTAINERS                                   |  1 +
> >  2 files changed, 50 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/adc/nuvoton,nct7201.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/iio/adc/nuvoton,nct7201.yaml b/Documentation/devicetree/bindings/iio/adc/nuvoton,nct7201.yaml
> > new file mode 100644
> > index 000000000000..08b52258e4af
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/nuvoton,nct7201.yaml
> > @@ -0,0 +1,49 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/adc/nuvoton,nct7201.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Nuvoton nct7201 and similar ADCs
> > +
> > +maintainers:
> > +  - Eason Yang <j2anfernee@gmail.com>
> > +
> > +description: |
> > +   Family of ADCs with i2c interface.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - nuvoton,nct7201
> > +      - nuvoton,nct7202
>
> Devices aren't compatible? Explain in the commit msg why they aren't or
> use proper compatibility (oneOf, see numerous other bindings or example-schema).
>
>

+  compatible:
-    enum:
-      - nuvoton,nct7201
-      - nuvoton,nct7202
+    oneOf:
+      - const: nuvoton,nct7201
+      - const: nuvoton,nct7202

> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  reset-gpios:
> > +    description:
> > +      Reset pin for the device.
>
> Drop description, obvious.
>
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        adc@1d {
> > +            compatible = "nuvoton,nct7202";
> > +            reg = <0x1d>;
>
>
> Make the example complete: add interrupts and reset-gpios.
>

Add interrupts and reset-gpios example,
+ #include <dt-bindings/gpio/gpio.h>

    i2c {
        #address-cells = <1>;
        #size-cells = <0>;
       adc@1d {
            compatible = "nuvoton,nct7202";
            reg = <0x1d>;
+            interrupt-parent = <&gpio3>;
+            interrupts = <30 IRQ_TYPE_LEVEL_LOW>;
+            reset-gpios = <&gpio3 28 GPIO_ACTIVE_LOW>;
        };

> Best regards,
> Krzysztof
>

Re: [PATCH v3 1/2] dt-bindings: iio: adc: Add binding for Nuvoton NCT720x ADCs

[Krzysztof Kozlowski]

On 06/01/2025 08:22, Yu-Hsian Yang wrote:
>>> ---
>>>  .../bindings/iio/adc/nuvoton,nct7201.yaml     | 49 +++++++++++++++++++
>>>  MAINTAINERS                                   |  1 +
>>>  2 files changed, 50 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/nuvoton,nct7201.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/adc/nuvoton,nct7201.yaml b/Documentation/devicetree/bindings/iio/adc/nuvoton,nct7201.yaml
>>> new file mode 100644
>>> index 000000000000..08b52258e4af
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/adc/nuvoton,nct7201.yaml
>>> @@ -0,0 +1,49 @@
>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/iio/adc/nuvoton,nct7201.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Nuvoton nct7201 and similar ADCs
>>> +
>>> +maintainers:
>>> +  - Eason Yang <j2anfernee@gmail.com>
>>> +
>>> +description: |
>>> +   Family of ADCs with i2c interface.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - nuvoton,nct7201
>>> +      - nuvoton,nct7202
>>
>> Devices aren't compatible? Explain in the commit msg why they aren't or
>> use proper compatibility (oneOf, see numerous other bindings or example-schema).
>>
>>
> 
> +  compatible:
> -    enum:
> -      - nuvoton,nct7201
> -      - nuvoton,nct7202
> +    oneOf:
> +      - const: nuvoton,nct7201
> +      - const: nuvoton,nct7202


Nothing improved. I referenced 'oneOf' for the case of compatibility.
Don't use it to code enum in different way.

Address the comment and questions. You did not respond to several
comments, so I assume you are going to implement/fix all of pointed out
things.

Best regards,
Krzysztof

[PATCH v3 2/2] iio: adc: add Nuvoton NCT7201 ADC driver

[Eason Yang]

Add Nuvoton NCT7201/NCT7202 system voltage monitor 12-bit ADC driver

NCT7201/NCT7202 supports up to 12 analog voltage monitor inputs and up to
4 SMBus addresses by ADDR pin. Meanwhile, ALERT# hardware event pins for
independent alarm signals, and the all threshold values could be set for
system protection without any timing delay. It also supports reset input
RSTIN# to recover system from a fault condition.

Currently, only single-edge mode conversion and threshold events support.

Signed-off-by: Eason Yang <j2anfernee@gmail.com>
---
 MAINTAINERS               |   1 +
 drivers/iio/adc/Kconfig   |  10 +
 drivers/iio/adc/Makefile  |   1 +
 drivers/iio/adc/nct7201.c | 449 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 461 insertions(+)
 create mode 100644 drivers/iio/adc/nct7201.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 9c5328cbef17..592ccca88f04 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2799,6 +2799,7 @@ F:	arch/arm/mach-npcm/
 F:	arch/arm64/boot/dts/nuvoton/
 F:	drivers/*/*/*npcm*
 F:	drivers/*/*npcm*
+F:	drivers/iio/adc/nct7201.c
 F:	drivers/rtc/rtc-nct3018y.c
 F:	include/dt-bindings/clock/nuvoton,npcm7xx-clock.h
 F:	include/dt-bindings/clock/nuvoton,npcm845-clk.h
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 849c90203071..abd8b21fdbac 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1048,6 +1048,16 @@ config NAU7802
 	  To compile this driver as a module, choose M here: the
 	  module will be called nau7802.
 
+config NCT7201
+	tristate "Nuvoton Instruments NCT7201 and NCT7202 Power Monitor"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  If you say yes here you get support for the Nuvoton NCT7201 and
+	  NCT7202 Voltage Monitor.
+	  This driver can also be built as a module. If so, the module
+	  will be called nct7201.
+
 config NPCM_ADC
 	tristate "Nuvoton NPCM ADC driver"
 	depends on ARCH_NPCM || COMPILE_TEST
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index ee19afba62b7..0108472e141c 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -94,6 +94,7 @@ obj-$(CONFIG_MESON_SARADC) += meson_saradc.o
 obj-$(CONFIG_MP2629_ADC) += mp2629_adc.o
 obj-$(CONFIG_MXS_LRADC_ADC) += mxs-lradc-adc.o
 obj-$(CONFIG_NAU7802) += nau7802.o
+obj-$(CONFIG_NCT7201) += nct7201.o
 obj-$(CONFIG_NPCM_ADC) += npcm_adc.o
 obj-$(CONFIG_PAC1921) += pac1921.o
 obj-$(CONFIG_PAC1934) += pac1934.o
diff --git a/drivers/iio/adc/nct7201.c b/drivers/iio/adc/nct7201.c
new file mode 100644
index 000000000000..9ad4d2919461
--- /dev/null
+++ b/drivers/iio/adc/nct7201.c
@@ -0,0 +1,449 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Driver for Nuvoton nct7201 and nct7202 power monitor chips.
+ *
+ * Copyright (c) 2024 Nuvoton Inc.
+ */
+
+#include <linux/array_size.h>
+#include <linux/bits.h>
+#include <linux/cleanup.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+#include <linux/types.h>
+#include <linux/unaligned.h>
+
+#include <linux/iio/events.h>
+#include <linux/iio/iio.h>
+
+#define NCT7201_VIN_MAX					12	/* Counted from 1 */
+#define NCT7201_IN_SCALING				4995
+#define NCT7201_IN_SCALING_FACTOR			10000
+
+#define NCT7201_REG_INTERRUPT_STATUS_1			0x0C
+#define NCT7201_REG_INTERRUPT_STATUS_2			0x0D
+#define NCT7201_REG_VOLT_LOW_BYTE			0x0F
+#define NCT7201_REG_CONFIGURATION			0x10
+#define  NCT7201_BIT_CONFIGURATION_START		BIT(0)
+#define  NCT7201_BIT_CONFIGURATION_ALERT_MSK		BIT(1)
+#define  NCT7201_BIT_CONFIGURATION_CONV_RATE		BIT(2)
+#define  NCT7201_BIT_CONFIGURATION_RESET		BIT(7)
+
+#define NCT7201_REG_ADVANCED_CONFIGURATION		0x11
+#define  NCT7201_BIT_ADVANCED_CONF_MOD_ALERT		BIT(0)
+#define  NCT7201_BIT_ADVANCED_CONF_MOD_STS		BIT(1)
+#define  NCT7201_BIT_ADVANCED_CONF_FAULT_QUEUE		BIT(2)
+#define  NCT7201_BIT_ADVANCED_CONF_EN_DEEP_SHUTDOWN	BIT(4)
+#define  NCT7201_BIT_ADVANCED_CONF_EN_SMB_TIMEOUT	BIT(5)
+#define  NCT7201_BIT_ADVANCED_CONF_MOD_RSTIN		BIT(7)
+
+#define NCT7201_REG_CHANNEL_INPUT_MODE			0x12
+#define NCT7201_REG_CHANNEL_ENABLE_1			0x13
+#define  NCT7201_REG_CHANNEL_ENABLE_1_MASK		GENMASK(7, 0)
+#define NCT7201_REG_CHANNEL_ENABLE_2			0x14
+#define  NCT7201_REG_CHANNEL_ENABLE_2_MASK		GENMASK(3, 0)
+#define NCT7201_REG_INTERRUPT_MASK_1			0x15
+#define NCT7201_REG_INTERRUPT_MASK_2			0x16
+#define NCT7201_REG_BUSY_STATUS				0x1E
+#define  NCT7201_BIT_BUSY				BIT(0)
+#define  NCT7201_BIT_PWR_UP				BIT(1)
+#define NCT7201_REG_ONE_SHOT				0x1F
+#define NCT7201_REG_SMUS_ADDRESS			0xFC
+#define NCT7201_REG_VIN_LIMIT_LSB_MASK			GENMASK(4, 0)
+
+#define NCT7201_REG_VIN(i)				(i)
+#define NCT7201_REG_VIN_HIGH_LIMIT(i)			(0x20 + (i) * 2)
+#define NCT7201_REG_VIN_LOW_LIMIT(i)			(0x21 + (i) * 2)
+#define NCT7201_REG_VIN_HIGH_LIMIT_LSB(i)		(0x40 + (i) * 2)
+#define NCT7201_REG_VIN_LOW_LIMIT_LSB(i)		(0x41 + (i) * 2)
+
+struct nct7201_chip_info {
+	struct i2c_client *client;
+	struct mutex access_lock;	/* for multi-byte read and write operations */
+	struct regmap *regmap;
+	struct regmap *regmap16;
+	int num_vin_channels;
+	u32 vin_mask;
+};
+
+struct nct7201_adc_model_data {
+	const char *model_name;
+	const struct iio_chan_spec *channels;
+	const int num_channels;
+	int num_vin_channels;
+};
+
+static const struct iio_event_spec nct7201_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),
+	},
+};
+
+#define NCT7201_VOLTAGE_CHANNEL(chan, addr)				\
+	{								\
+		.type = IIO_VOLTAGE,					\
+		.indexed = 1,						\
+		.channel = chan,					\
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
+		.address = addr,					\
+		.event_spec = nct7201_events,				\
+		.num_event_specs = ARRAY_SIZE(nct7201_events),		\
+	}
+
+static const struct iio_chan_spec nct7201_channels[] = {
+	NCT7201_VOLTAGE_CHANNEL(1, 0),
+	NCT7201_VOLTAGE_CHANNEL(2, 1),
+	NCT7201_VOLTAGE_CHANNEL(3, 2),
+	NCT7201_VOLTAGE_CHANNEL(4, 3),
+	NCT7201_VOLTAGE_CHANNEL(5, 4),
+	NCT7201_VOLTAGE_CHANNEL(6, 5),
+	NCT7201_VOLTAGE_CHANNEL(7, 6),
+	NCT7201_VOLTAGE_CHANNEL(8, 7),
+};
+
+static const struct iio_chan_spec nct7202_channels[] = {
+	NCT7201_VOLTAGE_CHANNEL(1, 0),
+	NCT7201_VOLTAGE_CHANNEL(2, 1),
+	NCT7201_VOLTAGE_CHANNEL(3, 2),
+	NCT7201_VOLTAGE_CHANNEL(4, 3),
+	NCT7201_VOLTAGE_CHANNEL(5, 4),
+	NCT7201_VOLTAGE_CHANNEL(6, 5),
+	NCT7201_VOLTAGE_CHANNEL(7, 6),
+	NCT7201_VOLTAGE_CHANNEL(8, 7),
+	NCT7201_VOLTAGE_CHANNEL(9, 8),
+	NCT7201_VOLTAGE_CHANNEL(10, 9),
+	NCT7201_VOLTAGE_CHANNEL(11, 10),
+	NCT7201_VOLTAGE_CHANNEL(12, 11),
+};
+
+static int nct7201_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int *val, int *val2, long mask)
+{
+	u16 volt;
+	unsigned int value;
+	int err;
+	struct nct7201_chip_info *chip = iio_priv(indio_dev);
+
+	if (chan->type != IIO_VOLTAGE)
+		return -EOPNOTSUPP;
+
+	guard(mutex)(&chip->access_lock);
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		err = regmap_read(chip->regmap16, NCT7201_REG_VIN(chan->address), &value);
+		if (err < 0)
+			return err;
+		volt = value;
+		*val = volt >> 3;
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		/* From the datasheet, we have to multiply by 0.0004995 */
+		*val = 0;
+		*val2 = 499500;
+		return IIO_VAL_INT_PLUS_NANO;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int nct7201_read_event_value(struct iio_dev *indio_dev,
+				    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 nct7201_chip_info *chip = iio_priv(indio_dev);
+	u16 volt;
+	unsigned int value;
+	int err;
+
+	if (chan->type != IIO_VOLTAGE)
+		return -EOPNOTSUPP;
+
+	if (info != IIO_EV_INFO_VALUE)
+		return -EINVAL;
+
+	if (dir == IIO_EV_DIR_FALLING) {
+		err = regmap_read(chip->regmap16, NCT7201_REG_VIN_LOW_LIMIT(chan->address), &value);
+		if (err < 0)
+			return err;
+		volt = value;
+	} else {
+		err = regmap_read(chip->regmap16, NCT7201_REG_VIN_HIGH_LIMIT(chan->address), &value);
+		if (err < 0)
+			return err;
+		volt = value;
+	}
+
+	*val = volt >> 3;
+
+	return IIO_VAL_INT;
+}
+
+static int nct7201_write_event_value(struct iio_dev *indio_dev,
+				     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 nct7201_chip_info *chip = iio_priv(indio_dev);
+	long v1, v2;
+
+	v1 = val >> 5;
+	v2 = FIELD_PREP(NCT7201_REG_VIN_LIMIT_LSB_MASK, val) << 3;
+
+	if (chan->type != IIO_VOLTAGE)
+		return -EOPNOTSUPP;
+
+	if (info == IIO_EV_INFO_VALUE) {
+		guard(mutex)(&chip->access_lock);
+		if (dir == IIO_EV_DIR_FALLING) {
+			regmap_write(chip->regmap, NCT7201_REG_VIN_LOW_LIMIT(chan->address), v1);
+			regmap_write(chip->regmap, NCT7201_REG_VIN_LOW_LIMIT_LSB(chan->address), v2);
+		} else {
+			regmap_write(chip->regmap, NCT7201_REG_VIN_HIGH_LIMIT(chan->address), v1);
+			regmap_write(chip->regmap, NCT7201_REG_VIN_HIGH_LIMIT_LSB(chan->address), v2);
+		}
+	}
+
+	return 0;
+}
+
+static int nct7201_read_event_config(struct iio_dev *indio_dev,
+				     const struct iio_chan_spec *chan,
+				     enum iio_event_type type,
+				     enum iio_event_direction dir)
+{
+	struct nct7201_chip_info *chip = iio_priv(indio_dev);
+
+	if (chan->type != IIO_VOLTAGE)
+		return -EOPNOTSUPP;
+
+	return !!(chip->vin_mask & BIT(chan->address));
+}
+
+static int nct7201_write_event_config(struct iio_dev *indio_dev,
+				      const struct iio_chan_spec *chan,
+				      enum iio_event_type type,
+				      enum iio_event_direction dir,
+				      bool state)
+{
+	struct nct7201_chip_info *chip = iio_priv(indio_dev);
+	unsigned int mask;
+
+	if (chan->type != IIO_VOLTAGE)
+		return -EOPNOTSUPP;
+
+	mask = BIT(chan->address);
+
+	if (!state && (chip->vin_mask & mask))
+		chip->vin_mask &= ~mask;
+	else if (state && !(chip->vin_mask & mask))
+		chip->vin_mask |= mask;
+
+	guard(mutex)(&chip->access_lock);
+	regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1,
+		     chip->vin_mask & NCT7201_REG_CHANNEL_ENABLE_1_MASK);
+	if (chip->num_vin_channels == 12)
+		regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_2,
+			     chip->vin_mask & NCT7201_REG_CHANNEL_ENABLE_2_MASK);
+
+	return 0;
+}
+
+static const struct iio_info nct7201_info = {
+	.read_raw = nct7201_read_raw,
+	.read_event_config = nct7201_read_event_config,
+	.write_event_config = nct7201_write_event_config,
+	.read_event_value = nct7201_read_event_value,
+	.write_event_value = nct7201_write_event_value,
+};
+
+static const struct iio_info nct7201_info_no_irq = {
+	.read_raw = nct7201_read_raw,
+};
+
+static const struct nct7201_adc_model_data nct7201_model_data = {
+	.model_name = "nct7201",
+	.channels = nct7201_channels,
+	.num_channels = ARRAY_SIZE(nct7201_channels),
+	.num_vin_channels = 8,
+};
+
+static const struct nct7201_adc_model_data nct7202_model_data = {
+	.model_name = "nct7202",
+	.channels = nct7202_channels,
+	.num_channels = ARRAY_SIZE(nct7202_channels),
+	.num_vin_channels = 12,
+};
+
+static int nct7201_init_chip(struct nct7201_chip_info *chip)
+{
+	u8 data[2];
+	unsigned int value;
+	int err;
+
+	regmap_write(chip->regmap, NCT7201_REG_CONFIGURATION,
+		     NCT7201_BIT_CONFIGURATION_RESET);
+
+	/*
+	 * After about 25 msecs, the device should be ready and then
+	 * the Power Up bit will be set to 1. If not, wait for it.
+	 */
+	mdelay(25);
+	err = regmap_read(chip->regmap, NCT7201_REG_BUSY_STATUS, &value);
+	if (err < 0)
+		return err;
+	if (!(value & NCT7201_BIT_PWR_UP))
+		return dev_err_probe(&chip->client->dev, -EIO, "failed to power up after reset\n");
+
+	/* Enable Channel */
+	guard(mutex)(&chip->access_lock);
+	regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1,
+		     NCT7201_REG_CHANNEL_ENABLE_1_MASK);
+	if (chip->num_vin_channels == 12)
+		regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_2,
+			     NCT7201_REG_CHANNEL_ENABLE_2_MASK);
+
+	err = regmap_read(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1, &value);
+	if (err < 0)
+		return err;
+	data[0] = value;
+
+	err = regmap_read(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_2, &value);
+	if (err < 0)
+		return err;
+	data[1] = value;
+
+	value = get_unaligned_le16(data);
+	chip->vin_mask = value;
+
+	/* Start monitoring if needed */
+	err = regmap_read(chip->regmap, NCT7201_REG_CONFIGURATION, &value);
+	if (err < 0) {
+		dev_err_probe(&chip->client->dev, -EIO, "Failed to read REG_CONFIGURATION\n");
+		return value;
+	}
+
+	value |= NCT7201_BIT_CONFIGURATION_START;
+	regmap_write(chip->regmap, NCT7201_REG_CONFIGURATION, value);
+
+	return 0;
+}
+
+static const struct regmap_config nct7201_regmap8_config = {
+	.name = "vin-data-read-byte",
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = 0xff,
+};
+
+static const struct regmap_config nct7201_regmap16_config = {
+	.name = "vin-data-read-word",
+	.reg_bits = 8,
+	.val_bits = 16,
+	.max_register = 0xff,
+};
+
+static int nct7201_probe(struct i2c_client *client)
+{
+	const struct nct7201_adc_model_data *model_data;
+	struct nct7201_chip_info *chip;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	model_data = i2c_get_match_data(client);
+	if (!model_data)
+		return -EINVAL;
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
+	if (!indio_dev)
+		return -ENOMEM;
+	chip = iio_priv(indio_dev);
+
+	chip->regmap = devm_regmap_init_i2c(client, &nct7201_regmap8_config);
+	if (IS_ERR(chip->regmap))
+		return dev_err_probe(&client->dev, PTR_ERR(chip->regmap),
+			"Failed to init regmap\n");
+
+	chip->regmap16 = devm_regmap_init_i2c(client, &nct7201_regmap16_config);
+	if (IS_ERR(chip->regmap16))
+		return dev_err_probe(&client->dev, PTR_ERR(chip->regmap16),
+			"Failed to init regmap16\n");
+
+	chip->num_vin_channels = model_data->num_vin_channels;
+
+	ret = devm_mutex_init(&client->dev, &chip->access_lock);
+	if (ret)
+		return ret;
+
+	chip->client = client;
+
+	ret = nct7201_init_chip(chip);
+	if (ret < 0)
+		return ret;
+
+	indio_dev->name = model_data->model_name;
+	indio_dev->channels = model_data->channels;
+	indio_dev->num_channels = model_data->num_channels;
+	if (client->irq)
+		indio_dev->info = &nct7201_info;
+	else
+		indio_dev->info = &nct7201_info_no_irq;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	return devm_iio_device_register(&client->dev, indio_dev);
+}
+
+static const struct i2c_device_id nct7201_id[] = {
+	{ .name = "nct7201", .driver_data = (kernel_ulong_t)&nct7201_model_data },
+	{ .name = "nct7202", .driver_data = (kernel_ulong_t)&nct7202_model_data },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, nct7201_id);
+
+static const struct of_device_id nct7201_of_match[] = {
+	{
+		.compatible = "nuvoton,nct7201",
+		.data = &nct7201_model_data,
+	},
+	{
+		.compatible = "nuvoton,nct7202",
+		.data = &nct7202_model_data,
+	},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, nct7201_of_match);
+
+static struct i2c_driver nct7201_driver = {
+	.driver = {
+		.name	= "nct7201",
+		.of_match_table = nct7201_of_match,
+	},
+	.probe = nct7201_probe,
+	.id_table = nct7201_id,
+};
+module_i2c_driver(nct7201_driver);
+
+MODULE_AUTHOR("Eason Yang <j2anfernee@gmail.com>");
+MODULE_DESCRIPTION("Nuvoton NCT7201 voltage monitor driver");
+MODULE_LICENSE("GPL");
-- 
2.34.1

Re: [PATCH v3 2/2] iio: adc: add Nuvoton NCT7201 ADC driver

[Andy Shevchenko]

On Thu, Dec 26, 2024 at 01:53:13PM +0800, Eason Yang wrote:
> Add Nuvoton NCT7201/NCT7202 system voltage monitor 12-bit ADC driver
> 
> NCT7201/NCT7202 supports up to 12 analog voltage monitor inputs and up to
> 4 SMBus addresses by ADDR pin. Meanwhile, ALERT# hardware event pins for
> independent alarm signals, and the all threshold values could be set for
> system protection without any timing delay. It also supports reset input
> RSTIN# to recover system from a fault condition.
> 
> Currently, only single-edge mode conversion and threshold events support.

...

> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		err = regmap_read(chip->regmap16, NCT7201_REG_VIN(chan->address), &value);
> +		if (err < 0)
> +			return err;
> +		volt = value;

> +		*val = volt >> 3;

I am not sure I understand this. If it's a shifted field, you rather
should fix it by using FIELD_*() macros from bitfield.h, otherwise
it's even more confusing.

> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		/* From the datasheet, we have to multiply by 0.0004995 */
> +		*val = 0;
> +		*val2 = 499500;
> +		return IIO_VAL_INT_PLUS_NANO;
> +	default:
> +		return -EINVAL;
> +	}

...

> +	*val = volt >> 3;

Ditto.


...

> +	v1 = val >> 5;
> +	v2 = FIELD_PREP(NCT7201_REG_VIN_LIMIT_LSB_MASK, val) << 3;

Ditto.

> +	if (chan->type != IIO_VOLTAGE)
> +		return -EOPNOTSUPP;
> +
> +	if (info == IIO_EV_INFO_VALUE) {
> +		guard(mutex)(&chip->access_lock);
> +		if (dir == IIO_EV_DIR_FALLING) {
> +			regmap_write(chip->regmap, NCT7201_REG_VIN_LOW_LIMIT(chan->address), v1);
> +			regmap_write(chip->regmap, NCT7201_REG_VIN_LOW_LIMIT_LSB(chan->address), v2);
> +		} else {
> +			regmap_write(chip->regmap, NCT7201_REG_VIN_HIGH_LIMIT(chan->address), v1);
> +			regmap_write(chip->regmap, NCT7201_REG_VIN_HIGH_LIMIT_LSB(chan->address), v2);
> +		}

This needs a comment why regmap_bulk_write() can't be used.

> +	}

...

> +static int nct7201_init_chip(struct nct7201_chip_info *chip)
> +{
> +	u8 data[2];
> +	unsigned int value;
> +	int err;
> +
> +	regmap_write(chip->regmap, NCT7201_REG_CONFIGURATION,
> +		     NCT7201_BIT_CONFIGURATION_RESET);

No error check?

> +	/*
> +	 * After about 25 msecs, the device should be ready and then
> +	 * the Power Up bit will be set to 1. If not, wait for it.
> +	 */
> +	mdelay(25);
> +	err = regmap_read(chip->regmap, NCT7201_REG_BUSY_STATUS, &value);
> +	if (err < 0)
> +		return err;
> +	if (!(value & NCT7201_BIT_PWR_UP))
> +		return dev_err_probe(&chip->client->dev, -EIO, "failed to power up after reset\n");
> +
> +	/* Enable Channel */
> +	guard(mutex)(&chip->access_lock);
> +	regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1,
> +		     NCT7201_REG_CHANNEL_ENABLE_1_MASK);
> +	if (chip->num_vin_channels == 12)
> +		regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_2,
> +			     NCT7201_REG_CHANNEL_ENABLE_2_MASK);
> +
> +	err = regmap_read(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1, &value);
> +	if (err < 0)
> +		return err;
> +	data[0] = value;
> +
> +	err = regmap_read(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_2, &value);
> +	if (err < 0)
> +		return err;
> +	data[1] = value;
> +
> +	value = get_unaligned_le16(data);
> +	chip->vin_mask = value;
> +
> +	/* Start monitoring if needed */
> +	err = regmap_read(chip->regmap, NCT7201_REG_CONFIGURATION, &value);
> +	if (err < 0) {

> +		dev_err_probe(&chip->client->dev, -EIO, "Failed to read REG_CONFIGURATION\n");
> +		return value;

You already used

	return dev_err_probe(...);

above, why here it's different? You want return something in addition to the
error code? Why?

> +	}
> +
> +	value |= NCT7201_BIT_CONFIGURATION_START;
> +	regmap_write(chip->regmap, NCT7201_REG_CONFIGURATION, value);
> +
> +	return 0;
> +}

...

> +static const struct regmap_config nct7201_regmap8_config = {
> +	.name = "vin-data-read-byte",
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = 0xff,
> +};
> +
> +static const struct regmap_config nct7201_regmap16_config = {
> +	.name = "vin-data-read-word",
> +	.reg_bits = 8,
> +	.val_bits = 16,
> +	.max_register = 0xff,
> +};

I don't see how these configurations will prevent, e.g., debugfs to access
16-bit registers via 8-bit IO and vice versa.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/2] iio: adc: add Nuvoton NCT7201 ADC driver

[Yu-Hsian Yang]

Dear Andy,

Thanks for your comments.

Andy Shevchenko <andriy.shevchenko@linux.intel.com> 於 2024年12月27日 週五 下午8:14寫道：
>
> On Thu, Dec 26, 2024 at 01:53:13PM +0800, Eason Yang wrote:
> > Add Nuvoton NCT7201/NCT7202 system voltage monitor 12-bit ADC driver
> >
> > NCT7201/NCT7202 supports up to 12 analog voltage monitor inputs and up to
> > 4 SMBus addresses by ADDR pin. Meanwhile, ALERT# hardware event pins for
> > independent alarm signals, and the all threshold values could be set for
> > system protection without any timing delay. It also supports reset input
> > RSTIN# to recover system from a fault condition.
> >
> > Currently, only single-edge mode conversion and threshold events support.
>
> ...
>
> > +     switch (mask) {
> > +     case IIO_CHAN_INFO_RAW:
> > +             err = regmap_read(chip->regmap16, NCT7201_REG_VIN(chan->address), &value);
> > +             if (err < 0)
> > +                     return err;
> > +             volt = value;
>
> > +             *val = volt >> 3;
>
> I am not sure I understand this. If it's a shifted field, you rather
> should fix it by using FIELD_*() macros from bitfield.h, otherwise
> it's even more confusing.
>

+ #define NCT7201_REG_VIN_MASK                GENMASK(15, 3)

- *val = volt >> 3;
+ *val = FIELD_GET(NCT7201_REG_VIN_MASK, volt);

> > +             return IIO_VAL_INT;
> > +     case IIO_CHAN_INFO_SCALE:
> > +             /* From the datasheet, we have to multiply by 0.0004995 */
> > +             *val = 0;
> > +             *val2 = 499500;
> > +             return IIO_VAL_INT_PLUS_NANO;
> > +     default:
> > +             return -EINVAL;
> > +     }
>
> ...
>
> > +     *val = volt >> 3;
>
> Ditto.
>
>
> ...
>
> > +     v1 = val >> 5;
> > +     v2 = FIELD_PREP(NCT7201_REG_VIN_LIMIT_LSB_MASK, val) << 3;
>
> Ditto.
>

The VIN reading supports Byte read (One Byte) and Word read (Two
Byte), the same as the VIN writing.
We don't need to separate 13-bit val into two bytes.
We can use 16-bit regmap to write val with right shift 3 bit to
replace write 8-bit regmap register twice.

> > +     if (chan->type != IIO_VOLTAGE)
> > +             return -EOPNOTSUPP;
> > +
> > +     if (info == IIO_EV_INFO_VALUE) {
> > +             guard(mutex)(&chip->access_lock);
> > +             if (dir == IIO_EV_DIR_FALLING) {
> > +                     regmap_write(chip->regmap, NCT7201_REG_VIN_LOW_LIMIT(chan->address), v1);
> > +                     regmap_write(chip->regmap, NCT7201_REG_VIN_LOW_LIMIT_LSB(chan->address), v2);
> > +             } else {
> > +                     regmap_write(chip->regmap, NCT7201_REG_VIN_HIGH_LIMIT(chan->address), v1);
> > +                     regmap_write(chip->regmap, NCT7201_REG_VIN_HIGH_LIMIT_LSB(chan->address), v2);
> > +             }

    if (dir == IIO_EV_DIR_FALLING)
-         regmap_write(chip->regmap,
NCT7201_REG_VIN_LOW_LIMIT(chan->address), v1);
-         regmap_write(chip->regmap,
NCT7201_REG_VIN_LOW_LIMIT_LSB(chan->address), v2);
+        regmap_write(chip->regmap16,
NCT7201_REG_VIN_LOW_LIMIT(chan->address),
FIELD_PREP(NCT7201_REG_VIN_MASK, val));
    else
-         regmap_write(chip->regmap,
NCT7201_REG_VIN_HIGH_LIMIT(chan->address), v1);
-         regmap_write(chip->regmap,
NCT7201_REG_VIN_HIGH_LIMIT_LSB(chan->address), v2);
+        regmap_write(chip->regmap16,
NCT7201_REG_VIN_HIGH_LIMIT(chan->address),
FIELD_PREP(NCT7201_REG_VIN_MASK, val));

>
> This needs a comment why regmap_bulk_write() can't be used.
>

We can't use regmap_bulk_write() since the chip limit.
regmap_bulk_write(chip->regmap, ..., ..., 2) ,
the first byte is well written, but the second byte don't changed.

> > +     }
>
> ...
>
> > +static int nct7201_init_chip(struct nct7201_chip_info *chip)
> > +{
> > +     u8 data[2];
> > +     unsigned int value;
> > +     int err;
> > +
> > +     regmap_write(chip->regmap, NCT7201_REG_CONFIGURATION,
> > +                  NCT7201_BIT_CONFIGURATION_RESET);
>
> No error check?
>

As David Lechner mentioned,
Better would be `return dev_err_probe()`. But it is very rare for
regmap_write() to fail so usually we don't print an error message
for these.

So we would remove print an error message for all remap_write.

> > +     /*
> > +      * After about 25 msecs, the device should be ready and then
> > +      * the Power Up bit will be set to 1. If not, wait for it.
> > +      */
> > +     mdelay(25);
> > +     err = regmap_read(chip->regmap, NCT7201_REG_BUSY_STATUS, &value);
> > +     if (err < 0)
> > +             return err;
> > +     if (!(value & NCT7201_BIT_PWR_UP))
> > +             return dev_err_probe(&chip->client->dev, -EIO, "failed to power up after reset\n");
> > +
> > +     /* Enable Channel */
> > +     guard(mutex)(&chip->access_lock);
> > +     regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1,
> > +                  NCT7201_REG_CHANNEL_ENABLE_1_MASK);
> > +     if (chip->num_vin_channels == 12)
> > +             regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_2,
> > +                          NCT7201_REG_CHANNEL_ENABLE_2_MASK);
> > +
> > +     err = regmap_read(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1, &value);
> > +     if (err < 0)
> > +             return err;
> > +     data[0] = value;
> > +
> > +     err = regmap_read(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_2, &value);
> > +     if (err < 0)
> > +             return err;
> > +     data[1] = value;
> > +
> > +     value = get_unaligned_le16(data);
> > +     chip->vin_mask = value;
> > +
> > +     /* Start monitoring if needed */
> > +     err = regmap_read(chip->regmap, NCT7201_REG_CONFIGURATION, &value);
> > +     if (err < 0) {
>
> > +             dev_err_probe(&chip->client->dev, -EIO, "Failed to read REG_CONFIGURATION\n");
> > +             return value;
>
> You already used
>
>         return dev_err_probe(...);
>
> above, why here it's different? You want return something in addition to the
> error code? Why?
>

It should use
return dev_err_probe(&chip->client->dev, -EIO, "Failed to read
REG_CONFIGURATION\n");

> > +     }
> > +
> > +     value |= NCT7201_BIT_CONFIGURATION_START;
> > +     regmap_write(chip->regmap, NCT7201_REG_CONFIGURATION, value);
> > +
> > +     return 0;
> > +}
>
> ...
>
> > +static const struct regmap_config nct7201_regmap8_config = {
> > +     .name = "vin-data-read-byte",
> > +     .reg_bits = 8,
> > +     .val_bits = 8,
> > +     .max_register = 0xff,
> > +};
> > +
> > +static const struct regmap_config nct7201_regmap16_config = {
> > +     .name = "vin-data-read-word",
> > +     .reg_bits = 8,
> > +     .val_bits = 16,
> > +     .max_register = 0xff,
> > +};
>
> I don't see how these configurations will prevent, e.g., debugfs to access
> 16-bit registers via 8-bit IO and vice versa.
>

Read VIN info can use word read or byte read, and other registers
should use byte read.

The design is that VIN info registers are used 16-bit debugfs to access and
other registers are used 8-bit debugfs to access.

We need to probe 8-bit regmap and 16-bit regmap,
but I have no idea how to prevent 8-bit IO to access 16-bit registers
and vice versa.

> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v3 2/2] iio: adc: add Nuvoton NCT7201 ADC driver

[David Lechner]

On 1/6/25 2:33 AM, Yu-Hsian Yang wrote:
> Dear Andy,
> 
> Thanks for your comments.
> 
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> 於 2024年12月27日 週五 下午8:14寫道：
>>
>> On Thu, Dec 26, 2024 at 01:53:13PM +0800, Eason Yang wrote:
>>> Add Nuvoton NCT7201/NCT7202 system voltage monitor 12-bit ADC driver
>>>
>>> NCT7201/NCT7202 supports up to 12 analog voltage monitor inputs and up to
>>> 4 SMBus addresses by ADDR pin. Meanwhile, ALERT# hardware event pins for
>>> independent alarm signals, and the all threshold values could be set for
>>> system protection without any timing delay. It also supports reset input
>>> RSTIN# to recover system from a fault condition.
>>>
>>> Currently, only single-edge mode conversion and threshold events support.
>>

...

>>
>>> +static const struct regmap_config nct7201_regmap8_config = {
>>> +     .name = "vin-data-read-byte",
>>> +     .reg_bits = 8,
>>> +     .val_bits = 8,
>>> +     .max_register = 0xff,
>>> +};
>>> +
>>> +static const struct regmap_config nct7201_regmap16_config = {
>>> +     .name = "vin-data-read-word",
>>> +     .reg_bits = 8,
>>> +     .val_bits = 16,
>>> +     .max_register = 0xff,
>>> +};
>>
>> I don't see how these configurations will prevent, e.g., debugfs to access
>> 16-bit registers via 8-bit IO and vice versa.
>>
> 
> Read VIN info can use word read or byte read, and other registers
> should use byte read.
> 
> The design is that VIN info registers are used 16-bit debugfs to access and
> other registers are used 8-bit debugfs to access.
> 
> We need to probe 8-bit regmap and 16-bit regmap,
> but I have no idea how to prevent 8-bit IO to access 16-bit registers
> and vice versa.

You can do this with struct regmap_access_table via wr_table and rd_table
in the struct regmap_config.

Re: [PATCH v3 2/2] iio: adc: add Nuvoton NCT7201 ADC driver

[Jonathan Cameron]

On Thu, 26 Dec 2024 13:53:13 +0800
Eason Yang <j2anfernee@gmail.com> wrote:

> Add Nuvoton NCT7201/NCT7202 system voltage monitor 12-bit ADC driver
> 
> NCT7201/NCT7202 supports up to 12 analog voltage monitor inputs and up to
> 4 SMBus addresses by ADDR pin. Meanwhile, ALERT# hardware event pins for
> independent alarm signals, and the all threshold values could be set for
> system protection without any timing delay. It also supports reset input
> RSTIN# to recover system from a fault condition.
> 
> Currently, only single-edge mode conversion and threshold events support.
> 
> Signed-off-by: Eason Yang <j2anfernee@gmail.com>
Hi Eason,

Various minor comments in addition to what Andy has
posted already.

Jonathan

> diff --git a/drivers/iio/adc/nct7201.c b/drivers/iio/adc/nct7201.c
> new file mode 100644
> index 000000000000..9ad4d2919461
> --- /dev/null
> +++ b/drivers/iio/adc/nct7201.c
> @@ -0,0 +1,449 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Driver for Nuvoton nct7201 and nct7202 power monitor chips.
> + *
> + * Copyright (c) 2024 Nuvoton Inc.
> + */
> +
> +#include <linux/array_size.h>
> +#include <linux/bits.h>
> +#include <linux/cleanup.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/types.h>
> +#include <linux/unaligned.h>
> +
> +#include <linux/iio/events.h>
> +#include <linux/iio/iio.h>
> +
> +#define NCT7201_VIN_MAX					12	/* Counted from 1 */
> +#define NCT7201_IN_SCALING				4995
> +#define NCT7201_IN_SCALING_FACTOR			10000
> +
> +#define NCT7201_REG_INTERRUPT_STATUS_1			0x0C
> +#define NCT7201_REG_INTERRUPT_STATUS_2			0x0D
> +#define NCT7201_REG_VOLT_LOW_BYTE			0x0F
> +#define NCT7201_REG_CONFIGURATION			0x10
> +#define  NCT7201_BIT_CONFIGURATION_START		BIT(0)
> +#define  NCT7201_BIT_CONFIGURATION_ALERT_MSK		BIT(1)
> +#define  NCT7201_BIT_CONFIGURATION_CONV_RATE		BIT(2)
> +#define  NCT7201_BIT_CONFIGURATION_RESET		BIT(7)
> +
> +#define NCT7201_REG_ADVANCED_CONFIGURATION		0x11
> +#define  NCT7201_BIT_ADVANCED_CONF_MOD_ALERT		BIT(0)
> +#define  NCT7201_BIT_ADVANCED_CONF_MOD_STS		BIT(1)
> +#define  NCT7201_BIT_ADVANCED_CONF_FAULT_QUEUE		BIT(2)
> +#define  NCT7201_BIT_ADVANCED_CONF_EN_DEEP_SHUTDOWN	BIT(4)
> +#define  NCT7201_BIT_ADVANCED_CONF_EN_SMB_TIMEOUT	BIT(5)
> +#define  NCT7201_BIT_ADVANCED_CONF_MOD_RSTIN		BIT(7)
> +
> +#define NCT7201_REG_CHANNEL_INPUT_MODE			0x12
> +#define NCT7201_REG_CHANNEL_ENABLE_1			0x13
> +#define  NCT7201_REG_CHANNEL_ENABLE_1_MASK		GENMASK(7, 0)
> +#define NCT7201_REG_CHANNEL_ENABLE_2			0x14
> +#define  NCT7201_REG_CHANNEL_ENABLE_2_MASK		GENMASK(3, 0)

As below. I'd treat these two registers as one larger register.

> +static int nct7201_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long mask)
> +{
> +	u16 volt;
> +	unsigned int value;
> +	int err;
> +	struct nct7201_chip_info *chip = iio_priv(indio_dev);
> +
> +	if (chan->type != IIO_VOLTAGE)
> +		return -EOPNOTSUPP;
> +
> +	guard(mutex)(&chip->access_lock);
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		err = regmap_read(chip->regmap16, NCT7201_REG_VIN(chan->address), &value);
> +		if (err < 0)
> +			return err;
> +		volt = value;
> +		*val = volt >> 3;

As below, likely a FIELD_GET() is appropriate here.

> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		/* From the datasheet, we have to multiply by 0.0004995 */
> +		*val = 0;
> +		*val2 = 499500;
> +		return IIO_VAL_INT_PLUS_NANO;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int nct7201_read_event_value(struct iio_dev *indio_dev,
> +				    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 nct7201_chip_info *chip = iio_priv(indio_dev);
> +	u16 volt;
> +	unsigned int value;
> +	int err;
> +
> +	if (chan->type != IIO_VOLTAGE)
> +		return -EOPNOTSUPP;
> +
> +	if (info != IIO_EV_INFO_VALUE)
> +		return -EINVAL;
> +
> +	if (dir == IIO_EV_DIR_FALLING) {
> +		err = regmap_read(chip->regmap16, NCT7201_REG_VIN_LOW_LIMIT(chan->address), &value);
> +		if (err < 0)
> +			return err;
> +		volt = value;
> +	} else {
> +		err = regmap_read(chip->regmap16, NCT7201_REG_VIN_HIGH_LIMIT(chan->address), &value);
> +		if (err < 0)
> +			return err;
> +		volt = value;
> +	}
> +
> +	*val = volt >> 3;
As Andy pointed out, likely a FIELD_GET() makes sense here.

> +
> +	return IIO_VAL_INT;
> +}
> +
> +static int nct7201_write_event_value(struct iio_dev *indio_dev,
> +				     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 nct7201_chip_info *chip = iio_priv(indio_dev);
> +	long v1, v2;
> +
> +	v1 = val >> 5;
> +	v2 = FIELD_PREP(NCT7201_REG_VIN_LIMIT_LSB_MASK, val) << 3;
> +
> +	if (chan->type != IIO_VOLTAGE)
> +		return -EOPNOTSUPP;
> +
> +	if (info == IIO_EV_INFO_VALUE) {
I'd flip this to
	if (info != IIO_EV_INFO_VALUE)
		return -EOPNOTSUPP;

	guard().

> +		guard(mutex)(&chip->access_lock);
> +		if (dir == IIO_EV_DIR_FALLING) {
> +			regmap_write(chip->regmap, NCT7201_REG_VIN_LOW_LIMIT(chan->address), v1);
> +			regmap_write(chip->regmap, NCT7201_REG_VIN_LOW_LIMIT_LSB(chan->address), v2);

Check for errors.

> +		} else {
> +			regmap_write(chip->regmap, NCT7201_REG_VIN_HIGH_LIMIT(chan->address), v1);
> +			regmap_write(chip->regmap, NCT7201_REG_VIN_HIGH_LIMIT_LSB(chan->address), v2);
> +		}
> +	}
> +
> +	return 0;
> +}

> +
> +static int nct7201_write_event_config(struct iio_dev *indio_dev,
> +				      const struct iio_chan_spec *chan,
> +				      enum iio_event_type type,
> +				      enum iio_event_direction dir,
> +				      bool state)
> +{
> +	struct nct7201_chip_info *chip = iio_priv(indio_dev);
> +	unsigned int mask;
> +
> +	if (chan->type != IIO_VOLTAGE)
> +		return -EOPNOTSUPP;
> +
> +	mask = BIT(chan->address);
> +
> +	if (!state && (chip->vin_mask & mask))
> +		chip->vin_mask &= ~mask;
> +	else if (state && !(chip->vin_mask & mask))
> +		chip->vin_mask |= mask;
> +
> +	guard(mutex)(&chip->access_lock);
> +	regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1,
> +		     chip->vin_mask & NCT7201_REG_CHANNEL_ENABLE_1_MASK);
> +	if (chip->num_vin_channels == 12)
> +		regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_2,
> +			     chip->vin_mask & NCT7201_REG_CHANNEL_ENABLE_2_MASK);

This feels odd.  Don't you need to shift that vin_mask to get rid of channels
that are enabled via ENABLE_1?

> +
> +	return 0;
> +}

> +static int nct7201_init_chip(struct nct7201_chip_info *chip)
> +{
> +	u8 data[2];
> +	unsigned int value;
> +	int err;
> +
> +	regmap_write(chip->regmap, NCT7201_REG_CONFIGURATION,
> +		     NCT7201_BIT_CONFIGURATION_RESET);

Add a comment on why you don't check return value (or do check it if appropriate).

> +
> +	/*
> +	 * After about 25 msecs, the device should be ready and then
> +	 * the Power Up bit will be set to 1. If not, wait for it.
> +	 */
> +	mdelay(25);
> +	err = regmap_read(chip->regmap, NCT7201_REG_BUSY_STATUS, &value);
> +	if (err < 0)
> +		return err;
> +	if (!(value & NCT7201_BIT_PWR_UP))
> +		return dev_err_probe(&chip->client->dev, -EIO, "failed to power up after reset\n");
> +
> +	/* Enable Channel */
> +	guard(mutex)(&chip->access_lock);
> +	regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1,
> +		     NCT7201_REG_CHANNEL_ENABLE_1_MASK);

Check return value.  This is over an I2C bus, not the most reliable of
transports!

Consider doing this differently and using a bulk write for the larger
case.

	if (chip->num_vin_channels <= 8)
		ret = regmap_write();
	else
		ret = regmap_bulk_write();

However as you read ENABLE_2 unconditionally below, can you instead just
always use a bulk write here?

> +	if (chip->num_vin_channels == 12)
> +		regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_2,
> +			     NCT7201_REG_CHANNEL_ENABLE_2_MASK);
> +
> +	err = regmap_read(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1, &value);
> +	if (err < 0)
> +		return err;
> +	data[0] = value;
> +
> +	err = regmap_read(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_2, &value);
> +	if (err < 0)
> +		return err;
> +	data[1] = value;

Why does reading these back make sense?  Is there a reason the write
above might not work if the I2C transfer is Acked?

If this is part of discovery protocol for how many channels are there, then
add comments.

> +
> +	value = get_unaligned_le16(data);
> +	chip->vin_mask = value;
> +
> +	/* Start monitoring if needed */
> +	err = regmap_read(chip->regmap, NCT7201_REG_CONFIGURATION, &value);
> +	if (err < 0) {
> +		dev_err_probe(&chip->client->dev, -EIO, "Failed to read REG_CONFIGURATION\n");
> +		return value;

Why return value if an error occurred?
		retuen dev_err_probe();

> +	}
> +
> +	value |= NCT7201_BIT_CONFIGURATION_START;
> +	regmap_write(chip->regmap, NCT7201_REG_CONFIGURATION, value);

regmap_set_bits()

> +
> +	return 0;
> +}
> +
> +static const struct regmap_config nct7201_regmap8_config = {
> +	.name = "vin-data-read-byte",
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = 0xff,
> +};
> +
> +static const struct regmap_config nct7201_regmap16_config = {
> +	.name = "vin-data-read-word",
> +	.reg_bits = 8,
> +	.val_bits = 16,
> +	.max_register = 0xff,
> +};
> +
> +static int nct7201_probe(struct i2c_client *client)
> +{
> +	const struct nct7201_adc_model_data *model_data;
> +	struct nct7201_chip_info *chip;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	model_data = i2c_get_match_data(client);
> +	if (!model_data)
> +		return -EINVAL;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +	chip = iio_priv(indio_dev);
> +
> +	chip->regmap = devm_regmap_init_i2c(client, &nct7201_regmap8_config);
> +	if (IS_ERR(chip->regmap))
> +		return dev_err_probe(&client->dev, PTR_ERR(chip->regmap),
> +			"Failed to init regmap\n");
> +
> +	chip->regmap16 = devm_regmap_init_i2c(client, &nct7201_regmap16_config);
> +	if (IS_ERR(chip->regmap16))
> +		return dev_err_probe(&client->dev, PTR_ERR(chip->regmap16),
> +			"Failed to init regmap16\n");
> +
> +	chip->num_vin_channels = model_data->num_vin_channels;
> +
> +	ret = devm_mutex_init(&client->dev, &chip->access_lock);
> +	if (ret)
> +		return ret;
> +
> +	chip->client = client;
> +
> +	ret = nct7201_init_chip(chip);
> +	if (ret < 0)
> +		return ret;
> +
> +	indio_dev->name = model_data->model_name;
> +	indio_dev->channels = model_data->channels;
> +	indio_dev->num_channels = model_data->num_channels;
> +	if (client->irq)
> +		indio_dev->info = &nct7201_info;
> +	else
> +		indio_dev->info = &nct7201_info_no_irq;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	return devm_iio_device_register(&client->dev, indio_dev);
> +}

Re: [PATCH v3 2/2] iio: adc: add Nuvoton NCT7201 ADC driver

[Yu-Hsian Yang]

Dear Jonathan,

Thanks for your comments.

Some questions are replied in Andy's comments.

We use FIELD_GET and FIELD_PREP to handle bit shift operations.

About regmap_write case, we remove print an error message according to
David Lechner mentioned.
If check for errors are needed, we would recovery it.

Jonathan Cameron <jic23@kernel.org> 於 2024年12月28日 週六 下午9:35寫道：
>
> On Thu, 26 Dec 2024 13:53:13 +0800
> Eason Yang <j2anfernee@gmail.com> wrote:
>
> > Add Nuvoton NCT7201/NCT7202 system voltage monitor 12-bit ADC driver
> >
> > NCT7201/NCT7202 supports up to 12 analog voltage monitor inputs and up to
> > 4 SMBus addresses by ADDR pin. Meanwhile, ALERT# hardware event pins for
> > independent alarm signals, and the all threshold values could be set for
> > system protection without any timing delay. It also supports reset input
> > RSTIN# to recover system from a fault condition.
> >
> > Currently, only single-edge mode conversion and threshold events support.
> >
> > Signed-off-by: Eason Yang <j2anfernee@gmail.com>
> Hi Eason,
>
> Various minor comments in addition to what Andy has
> posted already.
>
> Jonathan
>
> > diff --git a/drivers/iio/adc/nct7201.c b/drivers/iio/adc/nct7201.c
> > new file mode 100644
> > index 000000000000..9ad4d2919461
> > --- /dev/null
> > +++ b/drivers/iio/adc/nct7201.c
> > @@ -0,0 +1,449 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Driver for Nuvoton nct7201 and nct7202 power monitor chips.
> > + *
> > + * Copyright (c) 2024 Nuvoton Inc.
> > + */
> > +
> > +#include <linux/array_size.h>
> > +#include <linux/bits.h>
> > +#include <linux/cleanup.h>
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/i2c.h>
> > +#include <linux/init.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/regmap.h>
> > +#include <linux/types.h>
> > +#include <linux/unaligned.h>
> > +
> > +#include <linux/iio/events.h>
> > +#include <linux/iio/iio.h>
> > +
> > +#define NCT7201_VIN_MAX                                      12      /* Counted from 1 */
> > +#define NCT7201_IN_SCALING                           4995
> > +#define NCT7201_IN_SCALING_FACTOR                    10000
> > +
> > +#define NCT7201_REG_INTERRUPT_STATUS_1                       0x0C
> > +#define NCT7201_REG_INTERRUPT_STATUS_2                       0x0D
> > +#define NCT7201_REG_VOLT_LOW_BYTE                    0x0F
> > +#define NCT7201_REG_CONFIGURATION                    0x10
> > +#define  NCT7201_BIT_CONFIGURATION_START             BIT(0)
> > +#define  NCT7201_BIT_CONFIGURATION_ALERT_MSK         BIT(1)
> > +#define  NCT7201_BIT_CONFIGURATION_CONV_RATE         BIT(2)
> > +#define  NCT7201_BIT_CONFIGURATION_RESET             BIT(7)
> > +
> > +#define NCT7201_REG_ADVANCED_CONFIGURATION           0x11
> > +#define  NCT7201_BIT_ADVANCED_CONF_MOD_ALERT         BIT(0)
> > +#define  NCT7201_BIT_ADVANCED_CONF_MOD_STS           BIT(1)
> > +#define  NCT7201_BIT_ADVANCED_CONF_FAULT_QUEUE               BIT(2)
> > +#define  NCT7201_BIT_ADVANCED_CONF_EN_DEEP_SHUTDOWN  BIT(4)
> > +#define  NCT7201_BIT_ADVANCED_CONF_EN_SMB_TIMEOUT    BIT(5)
> > +#define  NCT7201_BIT_ADVANCED_CONF_MOD_RSTIN         BIT(7)
> > +
> > +#define NCT7201_REG_CHANNEL_INPUT_MODE                       0x12
> > +#define NCT7201_REG_CHANNEL_ENABLE_1                 0x13
> > +#define  NCT7201_REG_CHANNEL_ENABLE_1_MASK           GENMASK(7, 0)
> > +#define NCT7201_REG_CHANNEL_ENABLE_2                 0x14
> > +#define  NCT7201_REG_CHANNEL_ENABLE_2_MASK           GENMASK(3, 0)
>
> As below. I'd treat these two registers as one larger register.
>
> > +static int nct7201_read_raw(struct iio_dev *indio_dev,
> > +                         struct iio_chan_spec const *chan,
> > +                         int *val, int *val2, long mask)
> > +{
> > +     u16 volt;
> > +     unsigned int value;
> > +     int err;
> > +     struct nct7201_chip_info *chip = iio_priv(indio_dev);
> > +
> > +     if (chan->type != IIO_VOLTAGE)
> > +             return -EOPNOTSUPP;
> > +
> > +     guard(mutex)(&chip->access_lock);
> > +     switch (mask) {
> > +     case IIO_CHAN_INFO_RAW:
> > +             err = regmap_read(chip->regmap16, NCT7201_REG_VIN(chan->address), &value);
> > +             if (err < 0)
> > +                     return err;
> > +             volt = value;
> > +             *val = volt >> 3;
>
> As below, likely a FIELD_GET() is appropriate here.
>
> > +             return IIO_VAL_INT;
> > +     case IIO_CHAN_INFO_SCALE:
> > +             /* From the datasheet, we have to multiply by 0.0004995 */
> > +             *val = 0;
> > +             *val2 = 499500;
> > +             return IIO_VAL_INT_PLUS_NANO;
> > +     default:
> > +             return -EINVAL;
> > +     }
> > +}
> > +
> > +static int nct7201_read_event_value(struct iio_dev *indio_dev,
> > +                                 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 nct7201_chip_info *chip = iio_priv(indio_dev);
> > +     u16 volt;
> > +     unsigned int value;
> > +     int err;
> > +
> > +     if (chan->type != IIO_VOLTAGE)
> > +             return -EOPNOTSUPP;
> > +
> > +     if (info != IIO_EV_INFO_VALUE)
> > +             return -EINVAL;
> > +
> > +     if (dir == IIO_EV_DIR_FALLING) {
> > +             err = regmap_read(chip->regmap16, NCT7201_REG_VIN_LOW_LIMIT(chan->address), &value);
> > +             if (err < 0)
> > +                     return err;
> > +             volt = value;
> > +     } else {
> > +             err = regmap_read(chip->regmap16, NCT7201_REG_VIN_HIGH_LIMIT(chan->address), &value);
> > +             if (err < 0)
> > +                     return err;
> > +             volt = value;
> > +     }
> > +
> > +     *val = volt >> 3;
> As Andy pointed out, likely a FIELD_GET() makes sense here.
>
> > +
> > +     return IIO_VAL_INT;
> > +}
> > +
> > +static int nct7201_write_event_value(struct iio_dev *indio_dev,
> > +                                  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 nct7201_chip_info *chip = iio_priv(indio_dev);
> > +     long v1, v2;
> > +
> > +     v1 = val >> 5;
> > +     v2 = FIELD_PREP(NCT7201_REG_VIN_LIMIT_LSB_MASK, val) << 3;
> > +
> > +     if (chan->type != IIO_VOLTAGE)
> > +             return -EOPNOTSUPP;
> > +
> > +     if (info == IIO_EV_INFO_VALUE) {
> I'd flip this to
>         if (info != IIO_EV_INFO_VALUE)
>                 return -EOPNOTSUPP;
>
>         guard().
>
> > +             guard(mutex)(&chip->access_lock);
> > +             if (dir == IIO_EV_DIR_FALLING) {
> > +                     regmap_write(chip->regmap, NCT7201_REG_VIN_LOW_LIMIT(chan->address), v1);
> > +                     regmap_write(chip->regmap, NCT7201_REG_VIN_LOW_LIMIT_LSB(chan->address), v2);
>
> Check for errors.
>
> > +             } else {
> > +                     regmap_write(chip->regmap, NCT7201_REG_VIN_HIGH_LIMIT(chan->address), v1);
> > +                     regmap_write(chip->regmap, NCT7201_REG_VIN_HIGH_LIMIT_LSB(chan->address), v2);
> > +             }
> > +     }
> > +
> > +     return 0;
> > +}
>
> > +
> > +static int nct7201_write_event_config(struct iio_dev *indio_dev,
> > +                                   const struct iio_chan_spec *chan,
> > +                                   enum iio_event_type type,
> > +                                   enum iio_event_direction dir,
> > +                                   bool state)
> > +{
> > +     struct nct7201_chip_info *chip = iio_priv(indio_dev);
> > +     unsigned int mask;
> > +
> > +     if (chan->type != IIO_VOLTAGE)
> > +             return -EOPNOTSUPP;
> > +
> > +     mask = BIT(chan->address);
> > +
> > +     if (!state && (chip->vin_mask & mask))
> > +             chip->vin_mask &= ~mask;
> > +     else if (state && !(chip->vin_mask & mask))
> > +             chip->vin_mask |= mask;
> > +
> > +     guard(mutex)(&chip->access_lock);
> > +     regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1,
> > +                  chip->vin_mask & NCT7201_REG_CHANNEL_ENABLE_1_MASK);
> > +     if (chip->num_vin_channels == 12)
> > +             regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_2,
> > +                          chip->vin_mask & NCT7201_REG_CHANNEL_ENABLE_2_MASK);
>
> This feels odd.  Don't you need to shift that vin_mask to get rid of channels
> that are enabled via ENABLE_1?
>

We need to right shift 8 bit chip->vin_mask and write into ENABLE_2.
> > +     if (chip->num_vin_channels == 12)
> > +             regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_2,
-                          chip->vin_mask & NCT7201_REG_CHANNEL_ENABLE_2_MASK);
+                         FIELD_GET(GENMASK(15, 8), chip->vin_mask) &
NCT7201_REG_CHANNEL_ENABLE_2_MASK);

> > +
> > +     return 0;
> > +}
>
> > +static int nct7201_init_chip(struct nct7201_chip_info *chip)
> > +{
> > +     u8 data[2];
> > +     unsigned int value;
> > +     int err;
> > +
> > +     regmap_write(chip->regmap, NCT7201_REG_CONFIGURATION,
> > +                  NCT7201_BIT_CONFIGURATION_RESET);
>
> Add a comment on why you don't check return value (or do check it if appropriate).
>
> > +
> > +     /*
> > +      * After about 25 msecs, the device should be ready and then
> > +      * the Power Up bit will be set to 1. If not, wait for it.
> > +      */
> > +     mdelay(25);
> > +     err = regmap_read(chip->regmap, NCT7201_REG_BUSY_STATUS, &value);
> > +     if (err < 0)
> > +             return err;
> > +     if (!(value & NCT7201_BIT_PWR_UP))
> > +             return dev_err_probe(&chip->client->dev, -EIO, "failed to power up after reset\n");
> > +
> > +     /* Enable Channel */
> > +     guard(mutex)(&chip->access_lock);
> > +     regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1,
> > +                  NCT7201_REG_CHANNEL_ENABLE_1_MASK);
>
> Check return value.  This is over an I2C bus, not the most reliable of
> transports!
>
> Consider doing this differently and using a bulk write for the larger
> case.
>
>         if (chip->num_vin_channels <= 8)
>                 ret = regmap_write();
>         else
>                 ret = regmap_bulk_write();
>
> However as you read ENABLE_2 unconditionally below, can you instead just
> always use a bulk write here?
>

We can't use regmap_bulk_write() due to the chip's limit.
regmap_bulk_write(chip->regmap, ..., ..., 2) ,
the first byte is well written, but the second byte don't changed.


> > +     if (chip->num_vin_channels == 12)
> > +             regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_2,
> > +                          NCT7201_REG_CHANNEL_ENABLE_2_MASK);
> > +
> > +     err = regmap_read(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1, &value);
> > +     if (err < 0)
> > +             return err;
> > +     data[0] = value;
> > +
> > +     err = regmap_read(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_2, &value);
> > +     if (err < 0)
> > +             return err;
> > +     data[1] = value;
>
> Why does reading these back make sense?  Is there a reason the write
> above might not work if the I2C transfer is Acked?
>
> If this is part of discovery protocol for how many channels are there, then
> add comments.
>

We remove read back enable registers, just assign data value after
regmap_write() successfully.
Like,
    err = regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1,
             NCT7201_REG_CHANNEL_ENABLE_1_MASK);
    if (err) {
        dev_err(&chip->client->dev, "Failed to write REG_CHANNEL_ENABLE_1\n");
        return err;
    }
    data[0] = NCT7201_REG_CHANNEL_ENABLE_1_MASK;

> > +
> > +     value = get_unaligned_le16(data);
> > +     chip->vin_mask = value;
> > +
> > +     /* Start monitoring if needed */
> > +     err = regmap_read(chip->regmap, NCT7201_REG_CONFIGURATION, &value);
> > +     if (err < 0) {
> > +             dev_err_probe(&chip->client->dev, -EIO, "Failed to read REG_CONFIGURATION\n");
> > +             return value;
>
> Why return value if an error occurred?
>                 retuen dev_err_probe();
>
> > +     }
> > +
> > +     value |= NCT7201_BIT_CONFIGURATION_START;
> > +     regmap_write(chip->regmap, NCT7201_REG_CONFIGURATION, value);
>
> regmap_set_bits()
>

-     value |= NCT7201_BIT_CONFIGURATION_START;
-     regmap_write(chip->regmap, NCT7201_REG_CONFIGURATION, value);
+     regmap_set_bits(chip->regmap, NCT7201_REG_CONFIGURATION,
NCT7201_BIT_CONFIGURATION_START);

> > +
> > +     return 0;
> > +}
> > +
> > +static const struct regmap_config nct7201_regmap8_config = {
> > +     .name = "vin-data-read-byte",
> > +     .reg_bits = 8,
> > +     .val_bits = 8,
> > +     .max_register = 0xff,
> > +};
> > +
> > +static const struct regmap_config nct7201_regmap16_config = {
> > +     .name = "vin-data-read-word",
> > +     .reg_bits = 8,
> > +     .val_bits = 16,
> > +     .max_register = 0xff,
> > +};
> > +
> > +static int nct7201_probe(struct i2c_client *client)
> > +{
> > +     const struct nct7201_adc_model_data *model_data;
> > +     struct nct7201_chip_info *chip;
> > +     struct iio_dev *indio_dev;
> > +     int ret;
> > +
> > +     model_data = i2c_get_match_data(client);
> > +     if (!model_data)
> > +             return -EINVAL;
> > +
> > +     indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
> > +     if (!indio_dev)
> > +             return -ENOMEM;
> > +     chip = iio_priv(indio_dev);
> > +
> > +     chip->regmap = devm_regmap_init_i2c(client, &nct7201_regmap8_config);
> > +     if (IS_ERR(chip->regmap))
> > +             return dev_err_probe(&client->dev, PTR_ERR(chip->regmap),
> > +                     "Failed to init regmap\n");
> > +
> > +     chip->regmap16 = devm_regmap_init_i2c(client, &nct7201_regmap16_config);
> > +     if (IS_ERR(chip->regmap16))
> > +             return dev_err_probe(&client->dev, PTR_ERR(chip->regmap16),
> > +                     "Failed to init regmap16\n");
> > +
> > +     chip->num_vin_channels = model_data->num_vin_channels;
> > +
> > +     ret = devm_mutex_init(&client->dev, &chip->access_lock);
> > +     if (ret)
> > +             return ret;
> > +
> > +     chip->client = client;
> > +
> > +     ret = nct7201_init_chip(chip);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     indio_dev->name = model_data->model_name;
> > +     indio_dev->channels = model_data->channels;
> > +     indio_dev->num_channels = model_data->num_channels;
> > +     if (client->irq)
> > +             indio_dev->info = &nct7201_info;
> > +     else
> > +             indio_dev->info = &nct7201_info_no_irq;
> > +     indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > +     return devm_iio_device_register(&client->dev, indio_dev);
> > +}

Re: [PATCH v3 2/2] iio: adc: add Nuvoton NCT7201 ADC driver

[Jonathan Cameron]

<snip>

> > > +
> > > +     /*
> > > +      * After about 25 msecs, the device should be ready and then
> > > +      * the Power Up bit will be set to 1. If not, wait for it.
> > > +      */
> > > +     mdelay(25);
> > > +     err = regmap_read(chip->regmap, NCT7201_REG_BUSY_STATUS, &value);
> > > +     if (err < 0)
> > > +             return err;
> > > +     if (!(value & NCT7201_BIT_PWR_UP))
> > > +             return dev_err_probe(&chip->client->dev, -EIO, "failed to power up after reset\n");
> > > +
> > > +     /* Enable Channel */
> > > +     guard(mutex)(&chip->access_lock);
> > > +     regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1,
> > > +                  NCT7201_REG_CHANNEL_ENABLE_1_MASK);  
> >
> > Check return value.  This is over an I2C bus, not the most reliable of
> > transports!
> >
> > Consider doing this differently and using a bulk write for the larger
> > case.
> >
> >         if (chip->num_vin_channels <= 8)
> >                 ret = regmap_write();
> >         else
> >                 ret = regmap_bulk_write();
> >
> > However as you read ENABLE_2 unconditionally below, can you instead just
> > always use a bulk write here?
> >  
> 
> We can't use regmap_bulk_write() due to the chip's limit.
> regmap_bulk_write(chip->regmap, ..., ..., 2) ,
> the first byte is well written, but the second byte don't changed.

Find out why.  You may well need to set a few more parameters for
the configuration of the regmap to ensure the correct form of bulk
write. 

> 
> 
> > > +     if (chip->num_vin_channels == 12)
> > > +             regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_2,
> > > +                          NCT7201_REG_CHANNEL_ENABLE_2_MASK);
> > > +

Jonathan