iio: adc: add Nuvoton NCT7201 ADC driver

Adds a binding specification for the Nuvoton NCT7201/NCT7202

Signed-off-by Eason Yang <[email protected]>

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

Documentation/devicetree/bindings/iio/adc/nuvoton,nct7201.yaml new file +9 -0

new file mode 100644
index 000000000000..08b52258e4af
@@ -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:

Documentation/devicetree/bindings/iio/adc/nuvoton,nct7201.yaml new file +7 -0

+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false

Quoted 1 line

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.

Quoted 28 lines

Signed-off-by Eason Yang <[email protected]>

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

Documentation/devicetree/bindings/iio/adc/nuvoton,nct7201.yaml new file +14 -0

new file mode 100644
index 000000000000..08b52258e4af
@@ -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 <[email protected]>
+
+description: |
+   Family of ADCs with i2c interface.
+

Documentation/devicetree/bindings/iio/adc/nuvoton,nct7201.yaml line 19

Quoted 5 lines

Documentation/devicetree/bindings/iio/adc/nuvoton,nct7201.yaml new file +5 -0

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

Documentation/devicetree/bindings/iio/adc/nuvoton,nct7201.yaml line 29

Quoted 10 lines

Documentation/devicetree/bindings/iio/adc/nuvoton,nct7201.yaml new file +10 -0

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

Drop description, obvious.

Quoted 12 lines

Documentation/devicetree/bindings/iio/adc/nuvoton,nct7201.yaml new file +12 -0

+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;

Documentation/devicetree/bindings/iio/adc/nuvoton,nct7201.yaml line 46

Quoted 5 lines

Documentation/devicetree/bindings/iio/adc/nuvoton,nct7201.yaml new file +5 -0

+        #size-cells = <0>;
+
+        adc@1d {
+            compatible = "nuvoton,nct7202";
+            reg = <0x1d>;

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

Best regards,
Krzysztof

Quoted 36 lines

Quoted 21 lines

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

Documentation/devicetree/bindings/iio/adc/nuvoton,nct7201.yaml new file +9 -0

new file mode 100644
index 000000000000..08b52258e4af
@@ -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:

Documentation/devicetree/bindings/iio/adc/nuvoton,nct7201.yaml line 19

Quoted 10 lines

Documentation/devicetree/bindings/iio/adc/nuvoton,nct7201.yaml new file +10 -0

+  - Eason Yang <[email protected]>
+
+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).

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.

drivers/iio/adc/nct7201.c new file +9 -0

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

(quoted diff) +1 -0

+	}

drivers/iio/adc/nct7201.c new file +28 -0

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

drivers/iio/adc/nct7201.c new file +7 -0

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

Quoted 9 lines

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.

...

Quoted 6 lines

drivers/iio/adc/nct7201.c new file +6 -0

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

Quoted 1 line

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

Quoted 9 lines

drivers/iio/adc/nct7201.c new file +9 -0

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

...

Quoted 1 line

+ *val = volt >> 3;

Ditto.

...

drivers/iio/adc/nct7201.c line 212

Quoted 2 lines

drivers/iio/adc/nct7201.c new file +2 -0

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

Ditto.

Quoted 4 lines

drivers/iio/adc/nct7201.c new file +4 -0

+     if (chan->type != IIO_VOLTAGE)
+             return -EOPNOTSUPP;
+
+     if (info == IIO_EV_INFO_VALUE) {

drivers/iio/adc/nct7201.c line 225

Quoted 8 lines

drivers/iio/adc/nct7201.c new file +8 -0

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

Quoted 1 line

+ }

...

drivers/iio/adc/nct7201.c line 306

Quoted 8 lines

drivers/iio/adc/nct7201.c new file +8 -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);

No error check?

Quoted 28 lines

drivers/iio/adc/nct7201.c new file +28 -0

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

drivers/iio/adc/nct7201.c line 344

Quoted 10 lines

drivers/iio/adc/nct7201.c new file +9 -0

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

Quoted 7 lines

drivers/iio/adc/nct7201.c new file +7 -0

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

...

drivers/iio/adc/nct7201.c line 365

Quoted 13 lines

drivers/iio/adc/nct7201.c new file +13 -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

Dear Andy,

Thanks for your comments.

Andy Shevchenko <[email protected]> 於 2024年12月27日 週五 下午8:14寫道：

Quoted 12 lines · Eason Yang

Quoted 9 lines

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.

Quoted 18 lines

Quoted 4 lines

drivers/iio/adc/nct7201.c new file +3 -0

+static const struct regmap_config nct7201_regmap8_config = {
+     .name = "vin-data-read-byte",
+     .reg_bits = 8,

drivers/iio/adc/nct7201.c line 365

Quoted 10 lines

drivers/iio/adc/nct7201.c new file +10 -0

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

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 <[email protected]>

drivers/iio/adc/nct7201.c new file +41 -0

new file mode 100644
index 000000000000..9ad4d2919461
@@ -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)

drivers/iio/adc/nct7201.c new file +10 -0

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

drivers/iio/adc/nct7201.c new file +32 -0

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

drivers/iio/adc/nct7201.c new file +11 -0

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

drivers/iio/adc/nct7201.c new file +24 -0

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

drivers/iio/adc/nct7201.c new file +6 -0

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

drivers/iio/adc/nct7201.c new file +67 -0

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

Quoted 11 lines · On Thu, 26 Dec 2024 13:53:13 +0800 Eason Yang <[email protected]> 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 <[email protected]>

Hi Eason,

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

Jonathan

Quoted 47 lines

drivers/iio/adc/nct7201.c new file +41 -0

new file mode 100644
index 000000000000..9ad4d2919461
@@ -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)

drivers/iio/adc/nct7201.c line 51

Quoted 10 lines

drivers/iio/adc/nct7201.c new file +10 -0

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

Quoted 10 lines

drivers/iio/adc/nct7201.c new file +10 -0

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

drivers/iio/adc/nct7201.c line 154

Quoted 10 lines

drivers/iio/adc/nct7201.c new file +10 -0

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

Quoted 32 lines

drivers/iio/adc/nct7201.c new file +32 -0

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

drivers/iio/adc/nct7201.c line 196

Quoted 10 lines

drivers/iio/adc/nct7201.c new file +10 -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.

Quoted 11 lines

drivers/iio/adc/nct7201.c new file +11 -0

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

drivers/iio/adc/nct7201.c line 217

Quoted 10 lines

drivers/iio/adc/nct7201.c new file +10 -0

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

drivers/iio/adc/nct7201.c line 221

Quoted 4 lines

drivers/iio/adc/nct7201.c new file +4 -0

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

Quoted 30 lines

drivers/iio/adc/nct7201.c new file +29 -0

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

drivers/iio/adc/nct7201.c line 268

Quoted 5 lines

drivers/iio/adc/nct7201.c new file +5 -0

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

drivers/iio/adc/nct7201.c

Quoted 2 lines

drivers/iio/adc/nct7201.c new file +2 -0

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

drivers/iio/adc/nct7201.c line 306

Quoted 12 lines

drivers/iio/adc/nct7201.c new file +11 -0

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

Quoted 6 lines

drivers/iio/adc/nct7201.c new file +6 -0

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

drivers/iio/adc/nct7201.c line 322

Quoted 10 lines

drivers/iio/adc/nct7201.c new file +10 -0

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

drivers/iio/adc/nct7201.c line 335

Quoted 13 lines

drivers/iio/adc/nct7201.c new file +13 -0

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

drivers/iio/adc/nct7201.c line 344

Quoted 9 lines

drivers/iio/adc/nct7201.c new file +9 -0

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

drivers/iio/adc/nct7201.c line 348

Quoted 4 lines

drivers/iio/adc/nct7201.c new file +4 -0

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

regmap_set_bits()

Quoted 67 lines

drivers/iio/adc/nct7201.c new file +67 -0

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

Quoted 28 lines

Quoted 6 lines

drivers/iio/adc/nct7201.c new file +6 -0

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

drivers/iio/adc/nct7201.c line 322

Quoted 10 lines

drivers/iio/adc/nct7201.c new file +10 -0

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

Quoted 3 lines

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.

Quoted 6 lines

drivers/iio/adc/nct7201.c line 326

Quoted 6 lines

drivers/iio/adc/nct7201.c new file +4 -0

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