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.1On 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);
> +}
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);
> > +}
<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
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
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
>
>
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.
© 2016 - 2026 Red Hat, Inc.