[v2,2/2] iio: adc: add Nuvoton NCT720x ADC driver

Commit Message

Yu-Hsian Yang Dec. 3, 2024, 9:15 a.m. UTC

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 <[e-mail]>
---
 MAINTAINERS               |   1 +
 drivers/iio/adc/Kconfig   |  10 +
 drivers/iio/adc/Makefile  |   1 +
 drivers/iio/adc/nct720x.c | 533 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 545 insertions(+)
 create mode 100644 drivers/iio/adc/nct720x.c

Comments

Andy Shevchenko Dec. 3, 2024, 1:50 p.m. UTC | #1

On Tue, Dec 03, 2024 at 05:15:40PM +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.

Please, get rid of explicit castings where the are not needed or implied, like

	u16 foo;
	...
	foo = (u16)bar;

you have a lot of this in the code.

Second, why do you need two regmaps? How debugfs is supposed to work on the
registers that are 16-bit if you access them via 8-bit regmap and vice versa?

Can't you simply use bulk reads/writes when it makes sense and drop 16-bit
regmap completely?

Yu-Hsian Yang Dec. 4, 2024, 3:20 a.m. UTC | #2

Dear Andy Shevchenko,

Thank you for your comment.

Andy Shevchenko <[e-mail]> 於 2024年12月3日 週二 下午9:50寫道：
>
> On Tue, Dec 03, 2024 at 05:15:40PM +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.
>
> Please, get rid of explicit castings where the are not needed or implied, like
>
>         u16 foo;
>         ...
>         foo = (u16)bar;
>
> you have a lot of this in the code.
>

We would  get rid of explicit castings in all codes.

> Second, why do you need two regmaps? How debugfs is supposed to work on the
> registers that are 16-bit if you access them via 8-bit regmap and vice versa?
>
> Can't you simply use bulk reads/writes when it makes sense and drop 16-bit
> regmap completely?
>
>

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

For a reviewer's comment,
If the i2c controller allows word read
then the right thing is to always use it.


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

Andy Shevchenko Dec. 4, 2024, 4:27 a.m. UTC | #3

On Wed, Dec 04, 2024 at 11:20:20AM +0800, Yu-Hsian Yang wrote:
> Andy Shevchenko <[e-mail]> 於 2024年12月3日 週二 下午9:50寫道：
> > On Tue, Dec 03, 2024 at 05:15:40PM +0800, Eason Yang wrote:

...

> > Second, why do you need two regmaps? How debugfs is supposed to work on the
> > registers that are 16-bit if you access them via 8-bit regmap and vice versa?
> >
> > Can't you simply use bulk reads/writes when it makes sense and drop 16-bit
> > regmap completely?
> 
> Read VIN info can use word read or byte read, and other registers
> should use byte read.
> 
> For a reviewer's comment, If the i2c controller allows word read then the
> right thing is to always use it.

But how does this differ to bulk read of two sequential 8-bit offsets?
And if there is a difference, shouldn't this be addressed on regmap level for
all? Like testing for the supported flags and access registers based on the
controller capability and user request.

Yu-Hsian Yang Dec. 4, 2024, 9:05 a.m. UTC | #4

Dear Andy Shevchenko,

Andy Shevchenko <[e-mail]> 於 2024年12月4日 週三 下午12:27寫道：
>
> On Wed, Dec 04, 2024 at 11:20:20AM +0800, Yu-Hsian Yang wrote:
> > Andy Shevchenko <[e-mail]> 於 2024年12月3日 週二 下午9:50寫道：
> > > On Tue, Dec 03, 2024 at 05:15:40PM +0800, Eason Yang wrote:
>
> ...
>
> > > Second, why do you need two regmaps? How debugfs is supposed to work on the
> > > registers that are 16-bit if you access them via 8-bit regmap and vice versa?
> > >
> > > Can't you simply use bulk reads/writes when it makes sense and drop 16-bit
> > > regmap completely?
> >
> > Read VIN info can use word read or byte read, and other registers
> > should use byte read.
> >
> > For a reviewer's comment, If the i2c controller allows word read then the
> > right thing is to always use it.
>
> But how does this differ to bulk read of two sequential 8-bit offsets?
> And if there is a difference, shouldn't this be addressed on regmap level for
> all? Like testing for the supported flags and access registers based on the
> controller capability and user request.
>

We would explain why we use two regmaps.
In the beginning, we declare a property read-vin-data-size for user to
select byte read or word read.
After discuss with reviewers, we don't need this property.
So I get rid of this property and take word read vin data first.
We can't use regmap_bulk_read since the vin data is not sequential.

For Nuvoton NCT7201/NCT7202 chip,
Take an example as to Vin1:
The VIN reading supports Byte read (One Byte) and Word read (Two Byte)

For Byte read:
First read Index 00h to get VIN1 MSB, then read Index 0Fh Bit 3~7 to
get VIN1 LSB.
Index 0Fh is a shared LSB for all VINs.

For Word read:
Read Index 00h and get 2 Byte (VIN1 MSB and VIN1 LSB).

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

David Lechner Dec. 5, 2024, 6:22 p.m. UTC | #5

On 12/3/24 3:15 AM, 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.

In the code, there are channels set up for differential inputs. Should we
remove these until conversion and event support for them is added?

> 
> Signed-off-by: Eason Yang <[e-mail]>
> ---
>  MAINTAINERS               |   1 +
>  drivers/iio/adc/Kconfig   |  10 +
>  drivers/iio/adc/Makefile  |   1 +
>  drivers/iio/adc/nct720x.c | 533 ++++++++++++++++++++++++++++++++++++++
>  4 files changed, 545 insertions(+)
>  create mode 100644 drivers/iio/adc/nct720x.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index bea10a846475..573b12f0cd4d 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/nct720x.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..6eed518efa1c 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 NCT720X
> +	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 nct720x.

Don't put "x" in the name, just call it nct7201. We always try to avoid
using "x" in the IIO subsystem because too often it causes problems in
the future.

> +
>  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..89f5b5d1a567 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_NCT720X) += nct720x.o
>  obj-$(CONFIG_NPCM_ADC) += npcm_adc.o
>  obj-$(CONFIG_PAC1921) += pac1921.o
>  obj-$(CONFIG_PAC1934) += pac1934.o
> diff --git a/drivers/iio/adc/nct720x.c b/drivers/iio/adc/nct720x.c
> new file mode 100644
> index 000000000000..b28b5f4d7d70
> --- /dev/null
> +++ b/drivers/iio/adc/nct720x.c
> @@ -0,0 +1,533 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Driver for Nuvoton nct7201 and nct7202 power monitor chips.
> + *
> + * Copyright (c) 2024 Nuvoton Inc.

If there are datasheets available, it would be helpful to link to them here.

> + */
> +
> +#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>
> +#include <linux/iio/sysfs.h>

Unused header.

> +
> +#define VIN_MAX					12	/* Counted from 1 */
> +#define NCT720X_IN_SCALING			4995
> +#define NCT720X_IN_SCALING_FACTOR		10000
> +
> +#define REG_INTERRUPT_STATUS_1			0x0C
> +#define REG_INTERRUPT_STATUS_2			0x0D
> +#define REG_VOLT_LOW_BYTE			0x0F
> +#define REG_CONFIGURATION			0x10
> +#define  BIT_CONFIGURATION_START		BIT(0)
> +#define  BIT_CONFIGURATION_ALERT_MSK		BIT(1)
> +#define  BIT_CONFIGURATION_CONV_RATE		BIT(2)
> +#define  BIT_CONFIGURATION_RESET		BIT(7)
> +
> +#define REG_ADVANCED_CONFIGURATION		0x11
> +#define  BIT_ADVANCED_CONF_MOD_ALERT		BIT(0)
> +#define  BIT_ADVANCED_CONF_MOD_STS		BIT(1)
> +#define  BIT_ADVANCED_CONF_FAULT_QUEUE		BIT(2)
> +#define  BIT_ADVANCED_CONF_EN_DEEP_SHUTDOWN	BIT(4)
> +#define  BIT_ADVANCED_CONF_EN_SMB_TIMEOUT	BIT(5)
> +#define  BIT_ADVANCED_CONF_MOD_RSTIN		BIT(7)
> +
> +#define REG_CHANNEL_INPUT_MODE			0x12
> +#define REG_CHANNEL_ENABLE_1			0x13
> +#define  REG_CHANNEL_ENABLE_1_MASK		GENMASK(7, 0)
> +#define REG_CHANNEL_ENABLE_2			0x14
> +#define  REG_CHANNEL_ENABLE_2_MASK		GENMASK(3, 0)
> +#define REG_INTERRUPT_MASK_1			0x15
> +#define REG_INTERRUPT_MASK_2			0x16
> +#define REG_BUSY_STATUS				0x1E
> +#define  BIT_BUSY				BIT(0)
> +#define  BIT_PWR_UP				BIT(1)
> +#define REG_ONE_SHOT				0x1F
> +#define REG_SMUS_ADDRESS			0xFC
> +#define REG_VIN_LIMIT_LSB_MASK			GENMASK(4, 0)
> +
> +static const u8 REG_VIN[VIN_MAX] = {

Usually ALL_CAPS is reserved for macros and static const data is
lower_snake_case. Plus, prefer to always add the driver name as
a namespace to help avoid conflics with more generic names.

Example:

static const u8 nct7201_reg_vin[NCT7201_VIN_MAX] = {

Or (even better IMHO) just turn these into macros and avoid
the tables:

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

> +	0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07,	/* 0 -7 */
> +	0x08, 0x09, 0x0A, 0x0B,				/* 8 - 11 */
> +};
> +static const u8 REG_VIN_HIGH_LIMIT[VIN_MAX] = {
> +	0x20, 0x22, 0x24, 0x26, 0x28, 0x2A, 0x2C, 0x2E,
> +	0x30, 0x32, 0x34, 0x36,
> +};
> +static const u8 REG_VIN_LOW_LIMIT[VIN_MAX] = {
> +	0x21, 0x23, 0x25, 0x27, 0x29, 0x2B, 0x2D, 0x2F,
> +	0x31, 0x33, 0x35, 0x37,
> +};
> +static const u8 REG_VIN_HIGH_LIMIT_LSB[VIN_MAX] = {
> +	0x40, 0x42, 0x44, 0x46, 0x48, 0x4A, 0x4C, 0x4E,
> +	0x50, 0x52, 0x54, 0x56,
> +};
> +static const u8 REG_VIN_LOW_LIMIT_LSB[VIN_MAX] = {
> +	0x41, 0x43, 0x45, 0x47, 0x49, 0x4B, 0x4D, 0x4F,
> +	0x51, 0x53, 0x55, 0x57,
> +};
> +static u8 nct720x_chan_to_index[] = {

Should be const. Although, even better, just store this value in
the address field, then we don't need the translation table.

Right now, the address is always the same as the channel, so it
is redundant anyway.

> +	0 /* Not used */, 0, 1, 2, 3, 4, 5, 6,
> +	7, 8, 9, 10, 11,
> +};
> +
> +struct nct720x_chip_info {
> +	struct i2c_client *client;
> +	struct mutex access_lock;	/* for multi-byte read and write operations */
> +	struct regmap *regmap;
> +	struct regmap *regmap16;
> +	int vin_max;			/* number of VIN channels */

We could rename this to num_vin_channels, then we wouldn't need
a comment to explain it.

> +	u32 vin_mask;
> +};
> +
> +struct nct720x_adc_model_data {
> +	const char *model_name;
> +	const struct iio_chan_spec *channels;
> +	const int num_channels;
> +	int vin_max;
> +};
> +
> +static const struct iio_event_spec nct720x_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 NCT720X_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 = nct720x_events,				\
> +		.num_event_specs = ARRAY_SIZE(nct720x_events),		\
> +	}
> +
> +#define NCT720X_VOLTAGE_CHANNEL_DIFF(chan1, chan2, addr)		\
> +	{								\
> +		.type = IIO_VOLTAGE,					\
> +		.indexed = 1,						\
> +		.channel = (chan1),					\
> +		.channel2 = (chan2),					\
> +		.differential = 1,					\
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> +		.address = addr,					\
> +		.event_spec = nct720x_events,				\
> +		.num_event_specs = ARRAY_SIZE(nct720x_events),		\
> +	}
> +
> +static const struct iio_chan_spec nct7201_channels[] = {
> +	NCT720X_VOLTAGE_CHANNEL(1, 1),
> +	NCT720X_VOLTAGE_CHANNEL(2, 2),
> +	NCT720X_VOLTAGE_CHANNEL(3, 3),
> +	NCT720X_VOLTAGE_CHANNEL(4, 4),
> +	NCT720X_VOLTAGE_CHANNEL(5, 5),
> +	NCT720X_VOLTAGE_CHANNEL(6, 6),
> +	NCT720X_VOLTAGE_CHANNEL(7, 7),
> +	NCT720X_VOLTAGE_CHANNEL(8, 8),
> +	NCT720X_VOLTAGE_CHANNEL_DIFF(1, 2, 1),
> +	NCT720X_VOLTAGE_CHANNEL_DIFF(3, 4, 3),
> +	NCT720X_VOLTAGE_CHANNEL_DIFF(5, 6, 5),
> +	NCT720X_VOLTAGE_CHANNEL_DIFF(7, 8, 7),
> +};
> +
> +static const struct iio_chan_spec nct7202_channels[] = {
> +	NCT720X_VOLTAGE_CHANNEL(1, 1),
> +	NCT720X_VOLTAGE_CHANNEL(2, 2),
> +	NCT720X_VOLTAGE_CHANNEL(3, 3),
> +	NCT720X_VOLTAGE_CHANNEL(4, 4),
> +	NCT720X_VOLTAGE_CHANNEL(5, 5),
> +	NCT720X_VOLTAGE_CHANNEL(6, 6),
> +	NCT720X_VOLTAGE_CHANNEL(7, 7),
> +	NCT720X_VOLTAGE_CHANNEL(8, 8),
> +	NCT720X_VOLTAGE_CHANNEL(9, 9),
> +	NCT720X_VOLTAGE_CHANNEL(10, 10),
> +	NCT720X_VOLTAGE_CHANNEL(11, 11),
> +	NCT720X_VOLTAGE_CHANNEL(12, 12),
> +	NCT720X_VOLTAGE_CHANNEL_DIFF(1, 2, 1),
> +	NCT720X_VOLTAGE_CHANNEL_DIFF(3, 4, 3),
> +	NCT720X_VOLTAGE_CHANNEL_DIFF(5, 6, 5),
> +	NCT720X_VOLTAGE_CHANNEL_DIFF(7, 8, 7),
> +	NCT720X_VOLTAGE_CHANNEL_DIFF(9, 10, 9),
> +	NCT720X_VOLTAGE_CHANNEL_DIFF(11, 12, 11),
> +};
> +
> +static int nct720x_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long mask)
> +{
> +	int index = nct720x_chan_to_index[chan->address];
> +	u16 volt;
> +	unsigned int value;
> +	int err;
> +	struct nct720x_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, REG_VIN[index], &value);
> +		if (err < 0)
> +			return err;
> +		volt = (u16)value;
> +		*val = volt >> 3;

It seems strange that this is 13 bits when the chips are 8 and 12 bit.

> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		/* From the datasheet, we have to multiply by 0.0004995 */

The scale is the same for both 8 bit and 12 bit chips?

> +		*val = 0;
> +		*val2 = 499500;
> +		return IIO_VAL_INT_PLUS_NANO;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int nct720x_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 nct720x_chip_info *chip = iio_priv(indio_dev);
> +	u16 volt;
> +	unsigned int value;
> +	int tmp, err;
> +	int index = nct720x_chan_to_index[chan->address];
> +
> +	if (chan->type != IIO_VOLTAGE)
> +		return -EOPNOTSUPP;
> +
> +	if (info != IIO_EV_INFO_VALUE)
> +		return -EINVAL;

Do we need guard(mutex)(&chip->access_lock); here?

> +
> +	if (dir == IIO_EV_DIR_FALLING) {
> +		err = regmap_read(chip->regmap16, REG_VIN_LOW_LIMIT[index], &value);
> +		if (err < 0)
> +			return err;
> +		volt = (u16)value;
> +	} else {
> +		err = regmap_read(chip->regmap16, REG_VIN_HIGH_LIMIT[index], &value);
> +		if (err < 0)
> +			return err;
> +		volt = (u16)value;
> +	}
> +
> +	/* Voltage(V) = 13bitCountValue * 0.0004995 */
> +	tmp = (volt >> 3) * NCT720X_IN_SCALING;
> +	*val = tmp / NCT720X_IN_SCALING_FACTOR;

I'm pretty sure event threshold values need to be in raw units to match
the `in_voltageY_raw` attributes, so the scaling factor would not be
applied here.

> +
> +	return IIO_VAL_INT;
> +}
> +
> +static int nct720x_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 nct720x_chip_info *chip = iio_priv(indio_dev);
> +	int index, err = 0;
> +	long v1, v2, volt;
> +
> +	index = nct720x_chan_to_index[chan->address];
> +	volt = (val * NCT720X_IN_SCALING_FACTOR) / NCT720X_IN_SCALING;

Same applies here.

> +	v1 = volt >> 5;
> +	v2 = (volt & REG_VIN_LIMIT_LSB_MASK) << 3;

Looks like FIELD_PREP() could be used here.

> +
> +	if (chan->type != IIO_VOLTAGE)
> +		return -EOPNOTSUPP;
> +
> +	if (info == IIO_EV_INFO_VALUE) {
> +		if (dir == IIO_EV_DIR_FALLING) {
> +			guard(mutex)(&chip->access_lock);
> +			err = regmap_write(chip->regmap, REG_VIN_LOW_LIMIT[index], v1);
> +			if (err < 0)
> +				dev_err(&indio_dev->dev, "Failed to write REG_VIN%d_LOW_LIMIT\n",
> +					index + 1);
> +
> +			err = regmap_write(chip->regmap, REG_VIN_LOW_LIMIT_LSB[index], v2);
> +			if (err < 0)
> +				dev_err(&indio_dev->dev, "Failed to write REG_VIN%d_LOW_LIMIT_LSB\n",
> +					index + 1);
> +
> +		} else {
> +			guard(mutex)(&chip->access_lock);
> +			err = regmap_write(chip->regmap, REG_VIN_HIGH_LIMIT[index], v1);
> +			if (err < 0)
> +				dev_err(&indio_dev->dev, "Failed to write REG_VIN%d_HIGH_LIMIT\n",
> +					index + 1);
> +
> +			err = regmap_write(chip->regmap, REG_VIN_HIGH_LIMIT_LSB[index], v2);
> +			if (err < 0)
> +				dev_err(&indio_dev->dev, "Failed to write REG_VIN%d_HIGH_LIMIT_LSB\n",
> +					index + 1);
> +		}
> +	}
> +	return err;
> +}
> +
> +static int nct720x_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 nct720x_chip_info *chip = iio_priv(indio_dev);
> +	int index = nct720x_chan_to_index[chan->address];
> +
> +	if (chan->type != IIO_VOLTAGE)
> +		return -EOPNOTSUPP;
> +
> +	return !!(chip->vin_mask & BIT(index));
> +}
> +
> +static int nct720x_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)
> +{
> +	int err = 0;
> +	struct nct720x_chip_info *chip = iio_priv(indio_dev);
> +	int index = nct720x_chan_to_index[chan->address];
> +	unsigned int mask;
> +
> +	if (chan->type != IIO_VOLTAGE)
> +		return -EOPNOTSUPP;
> +
> +	mask = BIT(index);
> +
> +	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);
> +
> +	err = regmap_write(chip->regmap, REG_CHANNEL_ENABLE_1,
> +			   chip->vin_mask & REG_CHANNEL_ENABLE_1_MASK);
> +	if (err < 0)
> +		dev_err(&indio_dev->dev, "Failed to write REG_CHANNEL_ENABLE_1\n");
> +
> +	if (chip->vin_max == 12) {
> +		err = regmap_write(chip->regmap, REG_CHANNEL_ENABLE_2, chip->vin_mask >> 8);
> +		if (err < 0)
> +			dev_err(&indio_dev->dev, "Failed to write REG_CHANNEL_ENABLE_2\n");
> +	}
> +	return err;
> +}
> +
> +static const struct iio_info nct720x_info = {
> +	.read_raw = nct720x_read_raw,
> +	.read_event_config = nct720x_read_event_config,
> +	.write_event_config = nct720x_write_event_config,
> +	.read_event_value = nct720x_read_event_value,
> +	.write_event_value = nct720x_write_event_value,
> +};
> +
> +static const struct nct720x_adc_model_data nct7201_model_data = {
> +	.model_name = "nct7201",
> +	.channels = nct7201_channels,
> +	.num_channels = ARRAY_SIZE(nct7201_channels),
> +	.vin_max = 8,
> +};
> +
> +static const struct nct720x_adc_model_data nct7202_model_data = {
> +	.model_name = "nct7202",
> +	.channels = nct7202_channels,
> +	.num_channels = ARRAY_SIZE(nct7202_channels),
> +	.vin_max = 12,
> +};
> +
> +static int nct720x_init_chip(struct nct720x_chip_info *chip)
> +{
> +	u8 data[2];
> +	unsigned int value;
> +	int err;
> +
> +	err = regmap_write(chip->regmap, REG_CONFIGURATION, BIT_CONFIGURATION_RESET);
> +	if (err) {
> +		dev_err(&chip->client->dev, "Failed to write REG_CONFIGURATION\n");
> +		return err;

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.

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

I don't see anything that looks like waiting after checking the power
up bit.

> +	 */
> +	mdelay(25);
> +	err  = regmap_read(chip->regmap, REG_BUSY_STATUS, &value);
> +	if (err < 0)
> +		return err;
> +	if (!(value & BIT_PWR_UP))
> +		return err;

Won't this return 0? It seems like it should be returning an error code.

Better would be something like:

		return dev_err_probe(dev, -EIO, "failed to power up after reset\n");

> +
> +	/* Enable Channel */
> +	err = regmap_write(chip->regmap, REG_CHANNEL_ENABLE_1, REG_CHANNEL_ENABLE_1_MASK);
> +	if (err) {
> +		dev_err(&chip->client->dev, "Failed to write REG_CHANNEL_ENABLE_1\n");
> +		return err;
> +	}
> +
> +	if (chip->vin_max == 12) {
> +		err = regmap_write(chip->regmap, REG_CHANNEL_ENABLE_2, REG_CHANNEL_ENABLE_2_MASK);
> +		if (err) {
> +			dev_err(&chip->client->dev, "Failed to write REG_CHANNEL_ENABLE_2\n");
> +			return err;
> +		}
> +	}
> +
> +	guard(mutex)(&chip->access_lock);

Why guard here and not before other regmap access in this function?

Since this is only called during probe, we probably don't need the guard.

> +	err  = regmap_read(chip->regmap, REG_CHANNEL_ENABLE_1, &value);
> +	if (err < 0)
> +		return err;
> +	data[0] = (u8)value;
> +
> +	err  = regmap_read(chip->regmap, REG_CHANNEL_ENABLE_2, &value);
> +	if (err < 0)
> +		return err;
> +	data[1] = (u8)value;
> +
> +	value = get_unaligned_le16(data);
> +	chip->vin_mask = value;
> +
> +	/* Start monitoring if needed */
> +	err = regmap_read(chip->regmap, REG_CONFIGURATION, &value);
> +	if (err < 0) {
> +		dev_err(&chip->client->dev, "Failed to read REG_CONFIGURATION\n");
> +		return value;
> +	}
> +
> +	value |= BIT_CONFIGURATION_START;
> +	err = regmap_write(chip->regmap, REG_CONFIGURATION, value);
> +	if (err < 0) {
> +		dev_err(&chip->client->dev, "Failed to write REG_CONFIGURATION\n");
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct regmap_config nct720x_regmap8_config = {
> +	.name = "vin-data-read-byte",
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = 0xff,
> +	.cache_type = REGCACHE_NONE,
> +};
> +
> +static const struct regmap_config nct720x_regmap16_config = {
> +	.name = "vin-data-read-word",
> +	.reg_bits = 8,
> +	.val_bits = 16,
> +	.max_register = 0xff,
> +	.cache_type = REGCACHE_NONE,

REGCACHE_NONE is the default, so can be omitted.

> +};
> +
> +static int nct720x_probe(struct i2c_client *client)
> +{
> +	const struct nct720x_adc_model_data *model_data;
> +	struct nct720x_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, &nct720x_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, &nct720x_regmap16_config);
> +	if (IS_ERR(chip->regmap16))
> +		return dev_err_probe(&client->dev, PTR_ERR(chip->regmap16),
> +			"Failed to init regmap16\n");
> +
> +	chip->vin_max = model_data->vin_max;
> +
> +	ret = devm_mutex_init(&client->dev, &chip->access_lock);
> +	if (ret)
> +		return ret;
> +
> +	chip->client = client;
> +
> +	ret = nct720x_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;
> +	indio_dev->info = &nct720x_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	return devm_iio_device_register(&client->dev, indio_dev);
> +}
> +
> +static const struct i2c_device_id nct720x_id[] = {
> +	{ "nct7201", (kernel_ulong_t)&nct7201_model_data },
> +	{ "nct7202", (kernel_ulong_t)&nct7202_model_data },

To be consistent with [1], please add .name = and .driver_data = in this table.

[1]: https://lore.kernel.org/linux-iio/[e-mail]/

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, nct720x_id);
> +
> +static const struct of_device_id nct720x_of_match[] = {
> +	{
> +		.compatible = "nuvoton,nct7201",
> +		.data = &nct7201_model_data,
> +	},
> +	{
> +		.compatible = "nuvoton,nct7202",
> +		.data = &nct7202_model_data,
> +	},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, nct720x_of_match);
> +
> +static struct i2c_driver nct720x_driver = {
> +	.driver = {
> +		.name	= "nct720x",
> +		.of_match_table = nct720x_of_match,
> +	},
> +	.probe = nct720x_probe,
> +	.id_table = nct720x_id,
> +};
> +module_i2c_driver(nct720x_driver);
> +
> +MODULE_AUTHOR("Eason Yang <[e-mail]>");
> +MODULE_DESCRIPTION("Nuvoton NCT720x voltage monitor driver");
> +MODULE_LICENSE("GPL");

David Lechner Dec. 5, 2024, 7:01 p.m. UTC | #6

On 12/5/24 12:22 PM, David Lechner wrote:
> On 12/3/24 3:15 AM, Eason Yang wrote:


>> +static int nct720x_read_raw(struct iio_dev *indio_dev,
>> +			    struct iio_chan_spec const *chan,
>> +			    int *val, int *val2, long mask)
>> +{
>> +	int index = nct720x_chan_to_index[chan->address];
>> +	u16 volt;
>> +	unsigned int value;
>> +	int err;
>> +	struct nct720x_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, REG_VIN[index], &value);
>> +		if (err < 0)
>> +			return err;
>> +		volt = (u16)value;
>> +		*val = volt >> 3;
> 
> It seems strange that this is 13 bits when the chips are 8 and 12 bit.
> 
>> +		return IIO_VAL_INT;
>> +	case IIO_CHAN_INFO_SCALE:
>> +		/* From the datasheet, we have to multiply by 0.0004995 */
> 
> The scale is the same for both 8 bit and 12 bit chips?
> 
>> +		*val = 0;
>> +		*val2 = 499500;
>> +		return IIO_VAL_INT_PLUS_NANO;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +

Sorry, I got confused. The difference between the two chips is the
number of channels, not the number of bits. Please ignore these two
comments.

Jonathan Cameron Dec. 8, 2024, 5:16 p.m. UTC | #7

On Wed, 4 Dec 2024 17:05:19 +0800
Yu-Hsian Yang <[e-mail]> wrote:

> Dear Andy Shevchenko,
> 
> Andy Shevchenko <[e-mail]> 於 2024年12月4日 週三 下午12:27寫道：
> >
> > On Wed, Dec 04, 2024 at 11:20:20AM +0800, Yu-Hsian Yang wrote:  
> > > Andy Shevchenko <[e-mail]> 於 2024年12月3日 週二 下午9:50寫道：  
> > > > On Tue, Dec 03, 2024 at 05:15:40PM +0800, Eason Yang wrote:  
> >
> > ...
> >  
> > > > Second, why do you need two regmaps? How debugfs is supposed to work on the
> > > > registers that are 16-bit if you access them via 8-bit regmap and vice versa?
> > > >
> > > > Can't you simply use bulk reads/writes when it makes sense and drop 16-bit
> > > > regmap completely?  
> > >
> > > Read VIN info can use word read or byte read, and other registers
> > > should use byte read.
> > >
> > > For a reviewer's comment, If the i2c controller allows word read then the
> > > right thing is to always use it.  
> >
> > But how does this differ to bulk read of two sequential 8-bit offsets?
> > And if there is a difference, shouldn't this be addressed on regmap level for
> > all? Like testing for the supported flags and access registers based on the
> > controller capability and user request.
> >  
> 
> We would explain why we use two regmaps.
> In the beginning, we declare a property read-vin-data-size for user to
> select byte read or word read.
> After discuss with reviewers, we don't need this property.
> So I get rid of this property and take word read vin data first.
> We can't use regmap_bulk_read since the vin data is not sequential.
> 
> For Nuvoton NCT7201/NCT7202 chip,
> Take an example as to Vin1:
> The VIN reading supports Byte read (One Byte) and Word read (Two Byte)
> 
> For Byte read:
> First read Index 00h to get VIN1 MSB, then read Index 0Fh Bit 3~7 to
> get VIN1 LSB.
> Index 0Fh is a shared LSB for all VINs.
> 
> For Word read:
> Read Index 00h and get 2 Byte (VIN1 MSB and VIN1 LSB).

Yeah. This is a really weird device.  2 regmaps is probably
the best option.  The regmap access tables or functions can be used to
avoid the debugfs problem Andy mentioned.

Jonathan

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

Jonathan Cameron Dec. 8, 2024, 5:22 p.m. UTC | #8

On Tue,  3 Dec 2024 17:15:40 +0800
Eason Yang <[e-mail]> 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 <[e-mail]>
Hi Eason,

Given you have some good reviews already I only took a very quick glance
through.  A few things inline

Jonathan

> diff --git a/drivers/iio/adc/nct720x.c b/drivers/iio/adc/nct720x.c
> new file mode 100644
> index 000000000000..b28b5f4d7d70
> --- /dev/null
> +++ b/drivers/iio/adc/nct720x.c

> +
> +static int nct720x_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 nct720x_chip_info *chip = iio_priv(indio_dev);
> +	int index, err = 0;
> +	long v1, v2, volt;
> +
> +	index = nct720x_chan_to_index[chan->address];
> +	volt = (val * NCT720X_IN_SCALING_FACTOR) / NCT720X_IN_SCALING;
> +	v1 = volt >> 5;
> +	v2 = (volt & REG_VIN_LIMIT_LSB_MASK) << 3;
> +
> +	if (chan->type != IIO_VOLTAGE)
> +		return -EOPNOTSUPP;
> +
> +	if (info == IIO_EV_INFO_VALUE) {
> +		if (dir == IIO_EV_DIR_FALLING) {
> +			guard(mutex)(&chip->access_lock);

Might as well move this up one level as it is called in both legs.

> +			err = regmap_write(chip->regmap, REG_VIN_LOW_LIMIT[index], v1);
> +			if (err < 0)
> +				dev_err(&indio_dev->dev, "Failed to write REG_VIN%d_LOW_LIMIT\n",
> +					index + 1);
> +
> +			err = regmap_write(chip->regmap, REG_VIN_LOW_LIMIT_LSB[index], v2);
> +			if (err < 0)
> +				dev_err(&indio_dev->dev, "Failed to write REG_VIN%d_LOW_LIMIT_LSB\n",
> +					index + 1);
> +
> +		} else {
> +			guard(mutex)(&chip->access_lock);
> +			err = regmap_write(chip->regmap, REG_VIN_HIGH_LIMIT[index], v1);
> +			if (err < 0)
> +				dev_err(&indio_dev->dev, "Failed to write REG_VIN%d_HIGH_LIMIT\n",
> +					index + 1);
> +
> +			err = regmap_write(chip->regmap, REG_VIN_HIGH_LIMIT_LSB[index], v2);
> +			if (err < 0)
> +				dev_err(&indio_dev->dev, "Failed to write REG_VIN%d_HIGH_LIMIT_LSB\n",
> +					index + 1);
> +		}
> +	}
> +	return err;
> +}

> +
> +static const struct iio_info nct720x_info = {
> +	.read_raw = nct720x_read_raw,
> +	.read_event_config = nct720x_read_event_config,
> +	.write_event_config = nct720x_write_event_config,
> +	.read_event_value = nct720x_read_event_value,
> +	.write_event_value = nct720x_write_event_value,

Given you are supporting with and without interrupts, should probably pick between
versions of this that have the event config part and one that doesn't.

> +};
> +
> +static const struct nct720x_adc_model_data nct7201_model_data = {
> +	.model_name = "nct7201",
> +	.channels = nct7201_channels,
> +	.num_channels = ARRAY_SIZE(nct7201_channels),
> +	.vin_max = 8,
> +};
> +
> +static const struct nct720x_adc_model_data nct7202_model_data = {
> +	.model_name = "nct7202",
> +	.channels = nct7202_channels,
> +	.num_channels = ARRAY_SIZE(nct7202_channels),
> +	.vin_max = 12,
> +};
> +
> +static int nct720x_init_chip(struct nct720x_chip_info *chip)
> +{
> +	u8 data[2];
> +	unsigned int value;
> +	int err;
> +
> +	err = regmap_write(chip->regmap, REG_CONFIGURATION, BIT_CONFIGURATION_RESET);
> +	if (err) {
> +		dev_err(&chip->client->dev, "Failed to write REG_CONFIGURATION\n");
> +		return err;
> +	}
> +
> +	/*
> +	 * 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, REG_BUSY_STATUS, &value);
> +	if (err < 0)
> +		return err;
> +	if (!(value & BIT_PWR_UP))
> +		return err;
> +
> +	/* Enable Channel */
> +	err = regmap_write(chip->regmap, REG_CHANNEL_ENABLE_1, REG_CHANNEL_ENABLE_1_MASK);
> +	if (err) {
> +		dev_err(&chip->client->dev, "Failed to write REG_CHANNEL_ENABLE_1\n");
> +		return err;
> +	}
> +
> +	if (chip->vin_max == 12) {
> +		err = regmap_write(chip->regmap, REG_CHANNEL_ENABLE_2, REG_CHANNEL_ENABLE_2_MASK);
> +		if (err) {
> +			dev_err(&chip->client->dev, "Failed to write REG_CHANNEL_ENABLE_2\n");
> +			return err;
> +		}
> +	}
> +
> +	guard(mutex)(&chip->access_lock);
> +	err  = regmap_read(chip->regmap, REG_CHANNEL_ENABLE_1, &value);
> +	if (err < 0)
> +		return err;
> +	data[0] = (u8)value;
> +
> +	err  = regmap_read(chip->regmap, REG_CHANNEL_ENABLE_2, &value);
> +	if (err < 0)
> +		return err;

Here I think you can use a bulk read as the registers are next to each other.

> +	data[1] = (u8)value;
> +
> +	value = get_unaligned_le16(data);
> +	chip->vin_mask = value;
> +
> +	/* Start monitoring if needed */
> +	err = regmap_read(chip->regmap, REG_CONFIGURATION, &value);
> +	if (err < 0) {
> +		dev_err(&chip->client->dev, "Failed to read REG_CONFIGURATION\n");
> +		return value;
> +	}
> +
> +	value |= BIT_CONFIGURATION_START;
> +	err = regmap_write(chip->regmap, REG_CONFIGURATION, value);
> +	if (err < 0) {
> +		dev_err(&chip->client->dev, "Failed to write REG_CONFIGURATION\n");
> +		return err;
> +	}
> +
> +	return 0;
> +}

Christophe JAILLET Dec. 8, 2024, 5:47 p.m. UTC | #9

Le 03/12/2024 à 10:15, Eason Yang a écrit :
> 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 <[e-mail]>
> ---

...

> +static const u8 REG_VIN_HIGH_LIMIT_LSB[VIN_MAX] = {
> +	0x40, 0x42, 0x44, 0x46, 0x48, 0x4A, 0x4C, 0x4E,
> +	0x50, 0x52, 0x54, 0x56,
> +};
> +static const u8 REG_VIN_LOW_LIMIT_LSB[VIN_MAX] = {
> +	0x41, 0x43, 0x45, 0x47, 0x49, 0x4B, 0x4D, 0x4F,
> +	0x51, 0x53, 0x55, 0x57,
> +};
> +static u8 nct720x_chan_to_index[] = {

const as well here?

> +	0 /* Not used */, 0, 1, 2, 3, 4, 5, 6,
> +	7, 8, 9, 10, 11,
> +};

...

> +static int nct720x_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long mask)
> +{
> +	int index = nct720x_chan_to_index[chan->address];
> +	u16 volt;
> +	unsigned int value;
> +	int err;
> +	struct nct720x_chip_info *chip = iio_priv(indio_dev);
> +
> +	if (chan->type != IIO_VOLTAGE)
> +		return -EOPNOTSUPP;
> +
> +	guard(mutex)(&chip->access_lock);

The IIO_CHAN_INFO_SCALE case does not seem to need the lock. Would it 
make sense to move it only in the IIO_CHAN_INFO_RAW case?

> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		err = regmap_read(chip->regmap16, REG_VIN[index], &value);
> +		if (err < 0)
> +			return err;
> +		volt = (u16)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 nct720x_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)
> +{
> +	int err = 0;

Harmless but useless initialisation.

> +	struct nct720x_chip_info *chip = iio_priv(indio_dev);
> +	int index = nct720x_chan_to_index[chan->address];
> +	unsigned int mask;
> +
> +	if (chan->type != IIO_VOLTAGE)
> +		return -EOPNOTSUPP;

...

> +static int nct720x_init_chip(struct nct720x_chip_info *chip)
> +{
> +	u8 data[2];
> +	unsigned int value;
> +	int err;
> +
> +	err = regmap_write(chip->regmap, REG_CONFIGURATION, BIT_CONFIGURATION_RESET);
> +	if (err) {
> +		dev_err(&chip->client->dev, "Failed to write REG_CONFIGURATION\n");
> +		return err;
> +	}
> +
> +	/*
> +	 * 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, REG_BUSY_STATUS, &value);

double space after err.

> +	if (err < 0)
> +		return err;
> +	if (!(value & BIT_PWR_UP))
> +		return err;
> +
> +	/* Enable Channel */
> +	err = regmap_write(chip->regmap, REG_CHANNEL_ENABLE_1, REG_CHANNEL_ENABLE_1_MASK);
> +	if (err) {
> +		dev_err(&chip->client->dev, "Failed to write REG_CHANNEL_ENABLE_1\n");
> +		return err;
> +	}
> +
> +	if (chip->vin_max == 12) {
> +		err = regmap_write(chip->regmap, REG_CHANNEL_ENABLE_2, REG_CHANNEL_ENABLE_2_MASK);
> +		if (err) {
> +			dev_err(&chip->client->dev, "Failed to write REG_CHANNEL_ENABLE_2\n");
> +			return err;
> +		}
> +	}
> +
> +	guard(mutex)(&chip->access_lock);
> +	err  = regmap_read(chip->regmap, REG_CHANNEL_ENABLE_1, &value);

double space after err.

> +	if (err < 0)
> +		return err;
> +	data[0] = (u8)value;
> +
> +	err  = regmap_read(chip->regmap, REG_CHANNEL_ENABLE_2, &value);

double space after err.

> +	if (err < 0)
> +		return err;
> +	data[1] = (u8)value;
> +
> +	value = get_unaligned_le16(data);
> +	chip->vin_mask = value;
> +
> +	/* Start monitoring if needed */

...

CJ

Yu-Hsian Yang Dec. 10, 2024, 5:20 a.m. UTC | #10

Dear David Lechner,

Thanks for your comment.

David Lechner <[e-mail]> 於 2024年12月6日 週五 上午2:22寫道：
>
> On 12/3/24 3:15 AM, 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.
>
> In the code, there are channels set up for differential inputs. Should we
> remove these until conversion and event support for them is added?
>

Okay, I would remove differential inputs until conversions are finished.

> >
> > Signed-off-by: Eason Yang <[e-mail]>
> > ---
> >  MAINTAINERS               |   1 +
> >  drivers/iio/adc/Kconfig   |  10 +
> >  drivers/iio/adc/Makefile  |   1 +
> >  drivers/iio/adc/nct720x.c | 533 ++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 545 insertions(+)
> >  create mode 100644 drivers/iio/adc/nct720x.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index bea10a846475..573b12f0cd4d 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/nct720x.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..6eed518efa1c 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 NCT720X
> > +     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 nct720x.
>
> Don't put "x" in the name, just call it nct7201. We always try to avoid
> using "x" in the IIO subsystem because too often it causes problems in
> the future.
>

Understand it, we would use nct7201 to replace nct720x for all parts.

> > +
> >  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..89f5b5d1a567 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_NCT720X) += nct720x.o
> >  obj-$(CONFIG_NPCM_ADC) += npcm_adc.o
> >  obj-$(CONFIG_PAC1921) += pac1921.o
> >  obj-$(CONFIG_PAC1934) += pac1934.o
> > diff --git a/drivers/iio/adc/nct720x.c b/drivers/iio/adc/nct720x.c
> > new file mode 100644
> > index 000000000000..b28b5f4d7d70
> > --- /dev/null
> > +++ b/drivers/iio/adc/nct720x.c
> > @@ -0,0 +1,533 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Driver for Nuvoton nct7201 and nct7202 power monitor chips.
> > + *
> > + * Copyright (c) 2024 Nuvoton Inc.
>
> If there are datasheets available, it would be helpful to link to them here.
>

We would check the chip vendor.

> > + */
> > +
> > +#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>
> > +#include <linux/iio/sysfs.h>
>
> Unused header.
>
- #include <linux/iio/sysfs.h>

> > +
> > +#define VIN_MAX                                      12      /* Counted from 1 */
> > +#define NCT720X_IN_SCALING                   4995
> > +#define NCT720X_IN_SCALING_FACTOR            10000
> > +
> > +#define REG_INTERRUPT_STATUS_1                       0x0C
> > +#define REG_INTERRUPT_STATUS_2                       0x0D
> > +#define REG_VOLT_LOW_BYTE                    0x0F
> > +#define REG_CONFIGURATION                    0x10
> > +#define  BIT_CONFIGURATION_START             BIT(0)
> > +#define  BIT_CONFIGURATION_ALERT_MSK         BIT(1)
> > +#define  BIT_CONFIGURATION_CONV_RATE         BIT(2)
> > +#define  BIT_CONFIGURATION_RESET             BIT(7)
> > +
> > +#define REG_ADVANCED_CONFIGURATION           0x11
> > +#define  BIT_ADVANCED_CONF_MOD_ALERT         BIT(0)
> > +#define  BIT_ADVANCED_CONF_MOD_STS           BIT(1)
> > +#define  BIT_ADVANCED_CONF_FAULT_QUEUE               BIT(2)
> > +#define  BIT_ADVANCED_CONF_EN_DEEP_SHUTDOWN  BIT(4)
> > +#define  BIT_ADVANCED_CONF_EN_SMB_TIMEOUT    BIT(5)
> > +#define  BIT_ADVANCED_CONF_MOD_RSTIN         BIT(7)
> > +
> > +#define REG_CHANNEL_INPUT_MODE                       0x12
> > +#define REG_CHANNEL_ENABLE_1                 0x13
> > +#define  REG_CHANNEL_ENABLE_1_MASK           GENMASK(7, 0)
> > +#define REG_CHANNEL_ENABLE_2                 0x14
> > +#define  REG_CHANNEL_ENABLE_2_MASK           GENMASK(3, 0)
> > +#define REG_INTERRUPT_MASK_1                 0x15
> > +#define REG_INTERRUPT_MASK_2                 0x16
> > +#define REG_BUSY_STATUS                              0x1E
> > +#define  BIT_BUSY                            BIT(0)
> > +#define  BIT_PWR_UP                          BIT(1)
> > +#define REG_ONE_SHOT                         0x1F
> > +#define REG_SMUS_ADDRESS                     0xFC
> > +#define REG_VIN_LIMIT_LSB_MASK                       GENMASK(4, 0)
> > +
> > +static const u8 REG_VIN[VIN_MAX] = {
>
> Usually ALL_CAPS is reserved for macros and static const data is
> lower_snake_case. Plus, prefer to always add the driver name as
> a namespace to help avoid conflics with more generic names.
>
> Example:
>
> static const u8 nct7201_reg_vin[NCT7201_VIN_MAX] = {
>
> Or (even better IMHO) just turn these into macros and avoid
> the tables:
>
> #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)
>

Add prefix NCT7201_ in above all above define and use macros to avoid
the tables, like below
#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)

> > +     0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, /* 0 -7 */
> > +     0x08, 0x09, 0x0A, 0x0B,                         /* 8 - 11 */
> > +};
> > +static const u8 REG_VIN_HIGH_LIMIT[VIN_MAX] = {
> > +     0x20, 0x22, 0x24, 0x26, 0x28, 0x2A, 0x2C, 0x2E,
> > +     0x30, 0x32, 0x34, 0x36,
> > +};
> > +static const u8 REG_VIN_LOW_LIMIT[VIN_MAX] = {
> > +     0x21, 0x23, 0x25, 0x27, 0x29, 0x2B, 0x2D, 0x2F,
> > +     0x31, 0x33, 0x35, 0x37,
> > +};
> > +static const u8 REG_VIN_HIGH_LIMIT_LSB[VIN_MAX] = {
> > +     0x40, 0x42, 0x44, 0x46, 0x48, 0x4A, 0x4C, 0x4E,
> > +     0x50, 0x52, 0x54, 0x56,
> > +};
> > +static const u8 REG_VIN_LOW_LIMIT_LSB[VIN_MAX] = {
> > +     0x41, 0x43, 0x45, 0x47, 0x49, 0x4B, 0x4D, 0x4F,
> > +     0x51, 0x53, 0x55, 0x57,
> > +};
> > +static u8 nct720x_chan_to_index[] = {
>
> Should be const. Although, even better, just store this value in
> the address field, then we don't need the translation table.
>
> Right now, the address is always the same as the channel, so it
> is redundant anyway.
>

Remove nct720x_chan_to_index tables.

> > +     0 /* Not used */, 0, 1, 2, 3, 4, 5, 6,
> > +     7, 8, 9, 10, 11,
> > +};
> > +
> > +struct nct720x_chip_info {
> > +     struct i2c_client *client;
> > +     struct mutex access_lock;       /* for multi-byte read and write operations */
> > +     struct regmap *regmap;
> > +     struct regmap *regmap16;
> > +     int vin_max;                    /* number of VIN channels */
>
> We could rename this to num_vin_channels, then we wouldn't need
> a comment to explain it.
>

Okay, rename vin_max to num_vin_channels

> > +     u32 vin_mask;
> > +};
> > +
> > +struct nct720x_adc_model_data {
> > +     const char *model_name;
> > +     const struct iio_chan_spec *channels;
> > +     const int num_channels;
> > +     int vin_max;
> > +};

Rename vin_max to num_vin_channels

> > +
> > +static const struct iio_event_spec nct720x_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 NCT720X_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 = nct720x_events,                           \
> > +             .num_event_specs = ARRAY_SIZE(nct720x_events),          \
> > +     }
> > +
> > +#define NCT720X_VOLTAGE_CHANNEL_DIFF(chan1, chan2, addr)             \
> > +     {                                                               \
> > +             .type = IIO_VOLTAGE,                                    \
> > +             .indexed = 1,                                           \
> > +             .channel = (chan1),                                     \
> > +             .channel2 = (chan2),                                    \
> > +             .differential = 1,                                      \
> > +             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),           \
> > +             .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),   \
> > +             .address = addr,                                        \
> > +             .event_spec = nct720x_events,                           \
> > +             .num_event_specs = ARRAY_SIZE(nct720x_events),          \
> > +     }
> > +
> > +static const struct iio_chan_spec nct7201_channels[] = {
> > +     NCT720X_VOLTAGE_CHANNEL(1, 1),
> > +     NCT720X_VOLTAGE_CHANNEL(2, 2),
> > +     NCT720X_VOLTAGE_CHANNEL(3, 3),
> > +     NCT720X_VOLTAGE_CHANNEL(4, 4),
> > +     NCT720X_VOLTAGE_CHANNEL(5, 5),
> > +     NCT720X_VOLTAGE_CHANNEL(6, 6),
> > +     NCT720X_VOLTAGE_CHANNEL(7, 7),
> > +     NCT720X_VOLTAGE_CHANNEL(8, 8),
> > +     NCT720X_VOLTAGE_CHANNEL_DIFF(1, 2, 1),
> > +     NCT720X_VOLTAGE_CHANNEL_DIFF(3, 4, 3),
> > +     NCT720X_VOLTAGE_CHANNEL_DIFF(5, 6, 5),
> > +     NCT720X_VOLTAGE_CHANNEL_DIFF(7, 8, 7),
> > +};

Remove differential inputs.
-     NCT720X_VOLTAGE_CHANNEL_DIFF(1, 2, 1),
-     NCT720X_VOLTAGE_CHANNEL_DIFF(3, 4, 3),
-     NCT720X_VOLTAGE_CHANNEL_DIFF(5, 6, 5),
-     NCT720X_VOLTAGE_CHANNEL_DIFF(7, 8, 7),

> > +
> > +static const struct iio_chan_spec nct7202_channels[] = {
> > +     NCT720X_VOLTAGE_CHANNEL(1, 1),
> > +     NCT720X_VOLTAGE_CHANNEL(2, 2),
> > +     NCT720X_VOLTAGE_CHANNEL(3, 3),
> > +     NCT720X_VOLTAGE_CHANNEL(4, 4),
> > +     NCT720X_VOLTAGE_CHANNEL(5, 5),
> > +     NCT720X_VOLTAGE_CHANNEL(6, 6),
> > +     NCT720X_VOLTAGE_CHANNEL(7, 7),
> > +     NCT720X_VOLTAGE_CHANNEL(8, 8),
> > +     NCT720X_VOLTAGE_CHANNEL(9, 9),
> > +     NCT720X_VOLTAGE_CHANNEL(10, 10),
> > +     NCT720X_VOLTAGE_CHANNEL(11, 11),
> > +     NCT720X_VOLTAGE_CHANNEL(12, 12),
> > +     NCT720X_VOLTAGE_CHANNEL_DIFF(1, 2, 1),
> > +     NCT720X_VOLTAGE_CHANNEL_DIFF(3, 4, 3),
> > +     NCT720X_VOLTAGE_CHANNEL_DIFF(5, 6, 5),
> > +     NCT720X_VOLTAGE_CHANNEL_DIFF(7, 8, 7),
> > +     NCT720X_VOLTAGE_CHANNEL_DIFF(9, 10, 9),
> > +     NCT720X_VOLTAGE_CHANNEL_DIFF(11, 12, 11),
> > +};

Remove differential inputs.
-     NCT720X_VOLTAGE_CHANNEL_DIFF(1, 2, 1),
-     NCT720X_VOLTAGE_CHANNEL_DIFF(3, 4, 3),
-     NCT720X_VOLTAGE_CHANNEL_DIFF(5, 6, 5),
-     NCT720X_VOLTAGE_CHANNEL_DIFF(7, 8, 7),
-     NCT720X_VOLTAGE_CHANNEL_DIFF(9, 10, 9),
-     NCT720X_VOLTAGE_CHANNEL_DIFF(11, 12, 11),

> > +static int nct720x_read_raw(struct iio_dev *indio_dev,
> > +                         struct iio_chan_spec const *chan,
> > +                         int *val, int *val2, long mask)
> > +{
> > +     int index = nct720x_chan_to_index[chan->address];
> > +     u16 volt;
> > +     unsigned int value;
> > +     int err;
> > +     struct nct720x_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, REG_VIN[index], &value);
> > +             if (err < 0)
> > +                     return err;
> > +             volt = (u16)value;
> > +             *val = volt >> 3;
>
> It seems strange that this is 13 bits when the chips are 8 and 12 bit.
>
> > +             return IIO_VAL_INT;
> > +     case IIO_CHAN_INFO_SCALE:
> > +             /* From the datasheet, we have to multiply by 0.0004995 */
>
> The scale is the same for both 8 bit and 12 bit chips?
>
> > +             *val = 0;
> > +             *val2 = 499500;
> > +             return IIO_VAL_INT_PLUS_NANO;
> > +     default:
> > +             return -EINVAL;
> > +     }
> > +}
> > +
> > +static int nct720x_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 nct720x_chip_info *chip = iio_priv(indio_dev);
> > +     u16 volt;
> > +     unsigned int value;
> > +     int tmp, err;
> > +     int index = nct720x_chan_to_index[chan->address];
> > +
> > +     if (chan->type != IIO_VOLTAGE)
> > +             return -EOPNOTSUPP;
> > +
> > +     if (info != IIO_EV_INFO_VALUE)
> > +             return -EINVAL;
>
> Do we need guard(mutex)(&chip->access_lock); here?
>

No, if read word, we don't need mutex here.

> > +
> > +     if (dir == IIO_EV_DIR_FALLING) {
> > +             err = regmap_read(chip->regmap16, REG_VIN_LOW_LIMIT[index], &value);
> > +             if (err < 0)
> > +                     return err;
> > +             volt = (u16)value;
> > +     } else {
> > +             err = regmap_read(chip->regmap16, REG_VIN_HIGH_LIMIT[index], &value);
> > +             if (err < 0)
> > +                     return err;
> > +             volt = (u16)value;
> > +     }
> > +
> > +     /* Voltage(V) = 13bitCountValue * 0.0004995 */
> > +     tmp = (volt >> 3) * NCT720X_IN_SCALING;
> > +     *val = tmp / NCT720X_IN_SCALING_FACTOR;
>
> I'm pretty sure event threshold values need to be in raw units to match
> the `in_voltageY_raw` attributes, so the scaling factor would not be
> applied here.
>

-  /* Voltage(V) = 13bitCountValue * 0.0004995 */
- tmp = (volt >> 3) * NCT720X_IN_SCALING;
- val = tmp / NCT720X_IN_SCALING_FACTOR;
+ *val = volt >> 3;

> > +
> > +     return IIO_VAL_INT;
> > +}
> > +
> > +static int nct720x_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 nct720x_chip_info *chip = iio_priv(indio_dev);
> > +     int index, err = 0;
> > +     long v1, v2, volt;
> > +
> > +     index = nct720x_chan_to_index[chan->address];
> > +     volt = (val * NCT720X_IN_SCALING_FACTOR) / NCT720X_IN_SCALING;
>
> Same applies here.
>

- long v1, v2, volt;
+ long v1, v2;
- volt = (val * NCT720X_IN_SCALING_FACTOR) / NCT720X_IN_SCALING;

> > +     v1 = volt >> 5;
> > +     v2 = (volt & REG_VIN_LIMIT_LSB_MASK) << 3;
>
> Looks like FIELD_PREP() could be used here.
>

- v1 = volt >> 5;
- v2 = (volt & REG_VIN_LIMIT_LSB_MASK) << 3;
+ 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) {
> > +             if (dir == IIO_EV_DIR_FALLING) {
> > +                     guard(mutex)(&chip->access_lock);
> > +                     err = regmap_write(chip->regmap, REG_VIN_LOW_LIMIT[index], v1);
> > +                     if (err < 0)
> > +                             dev_err(&indio_dev->dev, "Failed to write REG_VIN%d_LOW_LIMIT\n",
> > +                                     index + 1);
> > +
> > +                     err = regmap_write(chip->regmap, REG_VIN_LOW_LIMIT_LSB[index], v2);
> > +                     if (err < 0)
> > +                             dev_err(&indio_dev->dev, "Failed to write REG_VIN%d_LOW_LIMIT_LSB\n",
> > +                                     index + 1);
> > +
> > +             } else {
> > +                     guard(mutex)(&chip->access_lock);
> > +                     err = regmap_write(chip->regmap, REG_VIN_HIGH_LIMIT[index], v1);
> > +                     if (err < 0)
> > +                             dev_err(&indio_dev->dev, "Failed to write REG_VIN%d_HIGH_LIMIT\n",
> > +                                     index + 1);
> > +
> > +                     err = regmap_write(chip->regmap, REG_VIN_HIGH_LIMIT_LSB[index], v2);
> > +                     if (err < 0)
> > +                             dev_err(&indio_dev->dev, "Failed to write REG_VIN%d_HIGH_LIMIT_LSB\n",
> > +                                     index + 1);
> > +             }
> > +     }
> > +     return err;
> > +}
> > +
> > +static int nct720x_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 nct720x_chip_info *chip = iio_priv(indio_dev);
> > +     int index = nct720x_chan_to_index[chan->address];
> > +
> > +     if (chan->type != IIO_VOLTAGE)
> > +             return -EOPNOTSUPP;
> > +
> > +     return !!(chip->vin_mask & BIT(index));
> > +}
> > +
> > +static int nct720x_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)
> > +{
> > +     int err = 0;
> > +     struct nct720x_chip_info *chip = iio_priv(indio_dev);
> > +     int index = nct720x_chan_to_index[chan->address];
> > +     unsigned int mask;
> > +
> > +     if (chan->type != IIO_VOLTAGE)
> > +             return -EOPNOTSUPP;
> > +
> > +     mask = BIT(index);
> > +
> > +     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);
> > +
> > +     err = regmap_write(chip->regmap, REG_CHANNEL_ENABLE_1,
> > +                        chip->vin_mask & REG_CHANNEL_ENABLE_1_MASK);
> > +     if (err < 0)
> > +             dev_err(&indio_dev->dev, "Failed to write REG_CHANNEL_ENABLE_1\n");
> > +
> > +     if (chip->vin_max == 12) {
> > +             err = regmap_write(chip->regmap, REG_CHANNEL_ENABLE_2, chip->vin_mask >> 8);
> > +             if (err < 0)
> > +                     dev_err(&indio_dev->dev, "Failed to write REG_CHANNEL_ENABLE_2\n");
> > +     }
> > +     return err;
> > +}
> > +
> > +static const struct iio_info nct720x_info = {
> > +     .read_raw = nct720x_read_raw,
> > +     .read_event_config = nct720x_read_event_config,
> > +     .write_event_config = nct720x_write_event_config,
> > +     .read_event_value = nct720x_read_event_value,
> > +     .write_event_value = nct720x_write_event_value,
> > +};
> > +
> > +static const struct nct720x_adc_model_data nct7201_model_data = {
> > +     .model_name = "nct7201",
> > +     .channels = nct7201_channels,
> > +     .num_channels = ARRAY_SIZE(nct7201_channels),
> > +     .vin_max = 8,
> > +};
> > +
> > +static const struct nct720x_adc_model_data nct7202_model_data = {
> > +     .model_name = "nct7202",
> > +     .channels = nct7202_channels,
> > +     .num_channels = ARRAY_SIZE(nct7202_channels),
> > +     .vin_max = 12,
> > +};
> > +
> > +static int nct720x_init_chip(struct nct720x_chip_info *chip)
> > +{
> > +     u8 data[2];
> > +     unsigned int value;
> > +     int err;
> > +
> > +     err = regmap_write(chip->regmap, REG_CONFIGURATION, BIT_CONFIGURATION_RESET);
> > +     if (err) {
> > +             dev_err(&chip->client->dev, "Failed to write REG_CONFIGURATION\n");
> > +             return err;
>
> 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.
>

We would remove print an error message for all remap_write failed.

> > +     }
> > +
> > +     /*
> > +      * 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.
>
> I don't see anything that looks like waiting after checking the power
> up bit.
>

Since 25 msecs is simulated by HW in the lab for all registers can be accessed.
Then we would check one register if it is ready to access,

> > +      */
> > +     mdelay(25);
> > +     err  = regmap_read(chip->regmap, REG_BUSY_STATUS, &value);
> > +     if (err < 0)
> > +             return err;
> > +     if (!(value & BIT_PWR_UP))
> > +             return err;
>
> Won't this return 0? It seems like it should be returning an error code.
>
> Better would be something like:
>
>                 return dev_err_probe(dev, -EIO, "failed to power up after reset\n");
>

Thanks for your comment.
- return err;
+ return dev_err_probe(&chip->client->dev, -EIO, "failed to power up
after reset\n");

> > +
> > +     /* Enable Channel */
> > +     err = regmap_write(chip->regmap, REG_CHANNEL_ENABLE_1, REG_CHANNEL_ENABLE_1_MASK);
> > +     if (err) {
> > +             dev_err(&chip->client->dev, "Failed to write REG_CHANNEL_ENABLE_1\n");
> > +             return err;
> > +     }
> > +
> > +     if (chip->vin_max == 12) {
> > +             err = regmap_write(chip->regmap, REG_CHANNEL_ENABLE_2, REG_CHANNEL_ENABLE_2_MASK);
> > +             if (err) {
> > +                     dev_err(&chip->client->dev, "Failed to write REG_CHANNEL_ENABLE_2\n");
> > +                     return err;
> > +             }
> > +     }
> > +
> > +     guard(mutex)(&chip->access_lock);
>
> Why guard here and not before other regmap access in this function?
>
> Since this is only called during probe, we probably don't need the guard.
>

Okay.

> > +     err  = regmap_read(chip->regmap, REG_CHANNEL_ENABLE_1, &value);
> > +     if (err < 0)
> > +             return err;
> > +     data[0] = (u8)value;
> > +
> > +     err  = regmap_read(chip->regmap, REG_CHANNEL_ENABLE_2, &value);
> > +     if (err < 0)
> > +             return err;
> > +     data[1] = (u8)value;
> > +
> > +     value = get_unaligned_le16(data);
> > +     chip->vin_mask = value;
> > +
> > +     /* Start monitoring if needed */
> > +     err = regmap_read(chip->regmap, REG_CONFIGURATION, &value);
> > +     if (err < 0) {
> > +             dev_err(&chip->client->dev, "Failed to read REG_CONFIGURATION\n");
> > +             return value;
> > +     }
> > +
> > +     value |= BIT_CONFIGURATION_START;
> > +     err = regmap_write(chip->regmap, REG_CONFIGURATION, value);
> > +     if (err < 0) {
> > +             dev_err(&chip->client->dev, "Failed to write REG_CONFIGURATION\n");
> > +             return err;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct regmap_config nct720x_regmap8_config = {
> > +     .name = "vin-data-read-byte",
> > +     .reg_bits = 8,
> > +     .val_bits = 8,
> > +     .max_register = 0xff,
> > +     .cache_type = REGCACHE_NONE,
> > +};
> > +
> > +static const struct regmap_config nct720x_regmap16_config = {
> > +     .name = "vin-data-read-word",
> > +     .reg_bits = 8,
> > +     .val_bits = 16,
> > +     .max_register = 0xff,
> > +     .cache_type = REGCACHE_NONE,
>
> REGCACHE_NONE is the default, so can be omitted.
>

Remove it.

> > +};
> > +
> > +static int nct720x_probe(struct i2c_client *client)
> > +{
> > +     const struct nct720x_adc_model_data *model_data;
> > +     struct nct720x_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, &nct720x_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, &nct720x_regmap16_config);
> > +     if (IS_ERR(chip->regmap16))
> > +             return dev_err_probe(&client->dev, PTR_ERR(chip->regmap16),
> > +                     "Failed to init regmap16\n");
> > +
> > +     chip->vin_max = model_data->vin_max;
> > +
> > +     ret = devm_mutex_init(&client->dev, &chip->access_lock);
> > +     if (ret)
> > +             return ret;
> > +
> > +     chip->client = client;
> > +
> > +     ret = nct720x_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;
> > +     indio_dev->info = &nct720x_info;
> > +     indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > +     return devm_iio_device_register(&client->dev, indio_dev);
> > +}
> > +
> > +static const struct i2c_device_id nct720x_id[] = {
> > +     { "nct7201", (kernel_ulong_t)&nct7201_model_data },
> > +     { "nct7202", (kernel_ulong_t)&nct7202_model_data },
>
> To be consistent with [1], please add .name = and .driver_data = in this table.
>
> [1]: https://lore.kernel.org/linux-iio/[e-mail]/
>

- { "nct7201", (kernel_ulong_t)&nct7201_model_data },
- { "nct7202", (kernel_ulong_t)&nct7202_model_data },
+ { .name = "nct7201", .driver_data = (kernel_ulong_t)&nct7201_model_data },
+ { .name = "nct7202", .driver_data = (kernel_ulong_t)&nct7202_model_data },

> > +     { }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, nct720x_id);
> > +
> > +static const struct of_device_id nct720x_of_match[] = {
> > +     {
> > +             .compatible = "nuvoton,nct7201",
> > +             .data = &nct7201_model_data,
> > +     },
> > +     {
> > +             .compatible = "nuvoton,nct7202",
> > +             .data = &nct7202_model_data,
> > +     },
> > +     { }
> > +};
> > +MODULE_DEVICE_TABLE(of, nct720x_of_match);
> > +
> > +static struct i2c_driver nct720x_driver = {
> > +     .driver = {
> > +             .name   = "nct720x",
> > +             .of_match_table = nct720x_of_match,
> > +     },
> > +     .probe = nct720x_probe,
> > +     .id_table = nct720x_id,
> > +};
> > +module_i2c_driver(nct720x_driver);
> > +
> > +MODULE_AUTHOR("Eason Yang <[e-mail]>");
> > +MODULE_DESCRIPTION("Nuvoton NCT720x voltage monitor driver");
> > +MODULE_LICENSE("GPL");
>

Yu-Hsian Yang Dec. 10, 2024, 5:27 a.m. UTC | #11

Dear David Lechner,

David Lechner <[e-mail]> 於 2024年12月6日 週五 上午3:01寫道：
>
> On 12/5/24 12:22 PM, David Lechner wrote:
> > On 12/3/24 3:15 AM, Eason Yang wrote:
>
>
> >> +static int nct720x_read_raw(struct iio_dev *indio_dev,
> >> +                        struct iio_chan_spec const *chan,
> >> +                        int *val, int *val2, long mask)
> >> +{
> >> +    int index = nct720x_chan_to_index[chan->address];
> >> +    u16 volt;
> >> +    unsigned int value;
> >> +    int err;
> >> +    struct nct720x_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, REG_VIN[index], &value);
> >> +            if (err < 0)
> >> +                    return err;
> >> +            volt = (u16)value;
> >> +            *val = volt >> 3;
> >
> > It seems strange that this is 13 bits when the chips are 8 and 12 bit.
> >
> >> +            return IIO_VAL_INT;
> >> +    case IIO_CHAN_INFO_SCALE:
> >> +            /* From the datasheet, we have to multiply by 0.0004995 */
> >
> > The scale is the same for both 8 bit and 12 bit chips?
> >
> >> +            *val = 0;
> >> +            *val2 = 499500;
> >> +            return IIO_VAL_INT_PLUS_NANO;
> >> +    default:
> >> +            return -EINVAL;
> >> +    }
> >> +}
> >> +
>
> Sorry, I got confused. The difference between the two chips is the
> number of channels, not the number of bits. Please ignore these two
> comments.

Yes, The difference between nct7201 and nct7202 is the vin numbers.
And VOLTAGE SENSE DATA FORMAT is
Voltage(V) =13bitCountValue * 0.0004995

Yu-Hsian Yang Dec. 10, 2024, 5:38 a.m. UTC | #12

Dear Jonathan Cameron,

Thanks for your comment.

Jonathan Cameron <[e-mail]> 於 2024年12月9日 週一 上午1:22寫道：
>
> On Tue,  3 Dec 2024 17:15:40 +0800
> Eason Yang <[e-mail]> 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 <[e-mail]>
> Hi Eason,
>
> Given you have some good reviews already I only took a very quick glance
> through.  A few things inline
>
> Jonathan
>
> > diff --git a/drivers/iio/adc/nct720x.c b/drivers/iio/adc/nct720x.c
> > new file mode 100644
> > index 000000000000..b28b5f4d7d70
> > --- /dev/null
> > +++ b/drivers/iio/adc/nct720x.c
>
> > +
> > +static int nct720x_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 nct720x_chip_info *chip = iio_priv(indio_dev);
> > +     int index, err = 0;
> > +     long v1, v2, volt;
> > +
> > +     index = nct720x_chan_to_index[chan->address];
> > +     volt = (val * NCT720X_IN_SCALING_FACTOR) / NCT720X_IN_SCALING;
> > +     v1 = volt >> 5;
> > +     v2 = (volt & REG_VIN_LIMIT_LSB_MASK) << 3;
> > +
> > +     if (chan->type != IIO_VOLTAGE)
> > +             return -EOPNOTSUPP;
> > +
> > +     if (info == IIO_EV_INFO_VALUE) {
> > +             if (dir == IIO_EV_DIR_FALLING) {
> > +                     guard(mutex)(&chip->access_lock);
>
> Might as well move this up one level as it is called in both legs.
>

I would remove guard(mutex) up one level.

> > +                     err = regmap_write(chip->regmap, REG_VIN_LOW_LIMIT[index], v1);
> > +                     if (err < 0)
> > +                             dev_err(&indio_dev->dev, "Failed to write REG_VIN%d_LOW_LIMIT\n",
> > +                                     index + 1);
> > +
> > +                     err = regmap_write(chip->regmap, REG_VIN_LOW_LIMIT_LSB[index], v2);
> > +                     if (err < 0)
> > +                             dev_err(&indio_dev->dev, "Failed to write REG_VIN%d_LOW_LIMIT_LSB\n",
> > +                                     index + 1);
> > +
> > +             } else {
> > +                     guard(mutex)(&chip->access_lock);
> > +                     err = regmap_write(chip->regmap, REG_VIN_HIGH_LIMIT[index], v1);
> > +                     if (err < 0)
> > +                             dev_err(&indio_dev->dev, "Failed to write REG_VIN%d_HIGH_LIMIT\n",
> > +                                     index + 1);
> > +
> > +                     err = regmap_write(chip->regmap, REG_VIN_HIGH_LIMIT_LSB[index], v2);
> > +                     if (err < 0)
> > +                             dev_err(&indio_dev->dev, "Failed to write REG_VIN%d_HIGH_LIMIT_LSB\n",
> > +                                     index + 1);
> > +             }
> > +     }
> > +     return err;
> > +}
>
> > +
> > +static const struct iio_info nct720x_info = {
> > +     .read_raw = nct720x_read_raw,
> > +     .read_event_config = nct720x_read_event_config,
> > +     .write_event_config = nct720x_write_event_config,
> > +     .read_event_value = nct720x_read_event_value,
> > +     .write_event_value = nct720x_write_event_value,
>
> Given you are supporting with and without interrupts, should probably pick between
> versions of this that have the event config part and one that doesn't.
>

Sorry, could you give some examples for us to refer.

> > +};
> > +
> > +static const struct nct720x_adc_model_data nct7201_model_data = {
> > +     .model_name = "nct7201",
> > +     .channels = nct7201_channels,
> > +     .num_channels = ARRAY_SIZE(nct7201_channels),
> > +     .vin_max = 8,
> > +};
> > +
> > +static const struct nct720x_adc_model_data nct7202_model_data = {
> > +     .model_name = "nct7202",
> > +     .channels = nct7202_channels,
> > +     .num_channels = ARRAY_SIZE(nct7202_channels),
> > +     .vin_max = 12,
> > +};
> > +
> > +static int nct720x_init_chip(struct nct720x_chip_info *chip)
> > +{
> > +     u8 data[2];
> > +     unsigned int value;
> > +     int err;
> > +
> > +     err = regmap_write(chip->regmap, REG_CONFIGURATION, BIT_CONFIGURATION_RESET);
> > +     if (err) {
> > +             dev_err(&chip->client->dev, "Failed to write REG_CONFIGURATION\n");
> > +             return err;
> > +     }
> > +
> > +     /*
> > +      * 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, REG_BUSY_STATUS, &value);
> > +     if (err < 0)
> > +             return err;
> > +     if (!(value & BIT_PWR_UP))
> > +             return err;
> > +
> > +     /* Enable Channel */
> > +     err = regmap_write(chip->regmap, REG_CHANNEL_ENABLE_1, REG_CHANNEL_ENABLE_1_MASK);
> > +     if (err) {
> > +             dev_err(&chip->client->dev, "Failed to write REG_CHANNEL_ENABLE_1\n");
> > +             return err;
> > +     }
> > +
> > +     if (chip->vin_max == 12) {
> > +             err = regmap_write(chip->regmap, REG_CHANNEL_ENABLE_2, REG_CHANNEL_ENABLE_2_MASK);
> > +             if (err) {
> > +                     dev_err(&chip->client->dev, "Failed to write REG_CHANNEL_ENABLE_2\n");
> > +                     return err;
> > +             }
> > +     }
> > +
> > +     guard(mutex)(&chip->access_lock);
> > +     err  = regmap_read(chip->regmap, REG_CHANNEL_ENABLE_1, &value);
> > +     if (err < 0)
> > +             return err;
> > +     data[0] = (u8)value;
> > +
> > +     err  = regmap_read(chip->regmap, REG_CHANNEL_ENABLE_2, &value);
> > +     if (err < 0)
> > +             return err;
>
> Here I think you can use a bulk read as the registers are next to each other.
>

Generally, registers with 8 bits support Byte format, and registers
with more than 8 bits support Word format.
If transmission a Word command to a register that supports Byte
format, the second byte will get 0xFF.
Here, if we use regmap_bulk_read(), we would get first byte correct
and second byte is wrong 0xff.
I use i2ctransfer to demo it.
root@evb-npcm845:~# i2ctransfer -f -y 5 w1@0x1d 0x13 r1
0xff
root@evb-npcm845:~# i2ctransfer -f -y 5 w1@0x1d 0x14 r1
0x0f



> > +     data[1] = (u8)value;
> > +
> > +     value = get_unaligned_le16(data);
> > +     chip->vin_mask = value;
> > +
> > +     /* Start monitoring if needed */
> > +     err = regmap_read(chip->regmap, REG_CONFIGURATION, &value);
> > +     if (err < 0) {
> > +             dev_err(&chip->client->dev, "Failed to read REG_CONFIGURATION\n");
> > +             return value;
> > +     }
> > +
> > +     value |= BIT_CONFIGURATION_START;
> > +     err = regmap_write(chip->regmap, REG_CONFIGURATION, value);
> > +     if (err < 0) {
> > +             dev_err(&chip->client->dev, "Failed to write REG_CONFIGURATION\n");
> > +             return err;
> > +     }
> > +
> > +     return 0;
> > +}

Yu-Hsian Yang Dec. 10, 2024, 5:47 a.m. UTC | #13

Dear Jonathan Cameron,

Sorry the above mail is not finished and just sent it.
I would explain why we can't use bulk read sequential bytes in our chips.

Yu-Hsian Yang <[e-mail]> 於 2024年12月10日 週二 下午1:38寫道：
>
> Dear Jonathan Cameron,
>
> Thanks for your comment.
>
> Jonathan Cameron <[e-mail]> 於 2024年12月9日 週一 上午1:22寫道：
> >
> > On Tue,  3 Dec 2024 17:15:40 +0800
> > Eason Yang <[e-mail]> 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 <[e-mail]>
> > Hi Eason,
> >
> > Given you have some good reviews already I only took a very quick glance
> > through.  A few things inline
> >
> > Jonathan
> >
> > > diff --git a/drivers/iio/adc/nct720x.c b/drivers/iio/adc/nct720x.c
> > > new file mode 100644
> > > index 000000000000..b28b5f4d7d70
> > > --- /dev/null
> > > +++ b/drivers/iio/adc/nct720x.c
> >
> > > +
> > > +static int nct720x_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 nct720x_chip_info *chip = iio_priv(indio_dev);
> > > +     int index, err = 0;
> > > +     long v1, v2, volt;
> > > +
> > > +     index = nct720x_chan_to_index[chan->address];
> > > +     volt = (val * NCT720X_IN_SCALING_FACTOR) / NCT720X_IN_SCALING;
> > > +     v1 = volt >> 5;
> > > +     v2 = (volt & REG_VIN_LIMIT_LSB_MASK) << 3;
> > > +
> > > +     if (chan->type != IIO_VOLTAGE)
> > > +             return -EOPNOTSUPP;
> > > +
> > > +     if (info == IIO_EV_INFO_VALUE) {
> > > +             if (dir == IIO_EV_DIR_FALLING) {
> > > +                     guard(mutex)(&chip->access_lock);
> >
> > Might as well move this up one level as it is called in both legs.
> >
>
> I would remove guard(mutex) up one level.
>
> > > +                     err = regmap_write(chip->regmap, REG_VIN_LOW_LIMIT[index], v1);
> > > +                     if (err < 0)
> > > +                             dev_err(&indio_dev->dev, "Failed to write REG_VIN%d_LOW_LIMIT\n",
> > > +                                     index + 1);
> > > +
> > > +                     err = regmap_write(chip->regmap, REG_VIN_LOW_LIMIT_LSB[index], v2);
> > > +                     if (err < 0)
> > > +                             dev_err(&indio_dev->dev, "Failed to write REG_VIN%d_LOW_LIMIT_LSB\n",
> > > +                                     index + 1);
> > > +
> > > +             } else {
> > > +                     guard(mutex)(&chip->access_lock);
> > > +                     err = regmap_write(chip->regmap, REG_VIN_HIGH_LIMIT[index], v1);
> > > +                     if (err < 0)
> > > +                             dev_err(&indio_dev->dev, "Failed to write REG_VIN%d_HIGH_LIMIT\n",
> > > +                                     index + 1);
> > > +
> > > +                     err = regmap_write(chip->regmap, REG_VIN_HIGH_LIMIT_LSB[index], v2);
> > > +                     if (err < 0)
> > > +                             dev_err(&indio_dev->dev, "Failed to write REG_VIN%d_HIGH_LIMIT_LSB\n",
> > > +                                     index + 1);
> > > +             }
> > > +     }
> > > +     return err;
> > > +}
> >
> > > +
> > > +static const struct iio_info nct720x_info = {
> > > +     .read_raw = nct720x_read_raw,
> > > +     .read_event_config = nct720x_read_event_config,
> > > +     .write_event_config = nct720x_write_event_config,
> > > +     .read_event_value = nct720x_read_event_value,
> > > +     .write_event_value = nct720x_write_event_value,
> >
> > Given you are supporting with and without interrupts, should probably pick between
> > versions of this that have the event config part and one that doesn't.
> >
>
> Sorry, could you give some examples for us to refer.
>
> > > +};
> > > +
> > > +static const struct nct720x_adc_model_data nct7201_model_data = {
> > > +     .model_name = "nct7201",
> > > +     .channels = nct7201_channels,
> > > +     .num_channels = ARRAY_SIZE(nct7201_channels),
> > > +     .vin_max = 8,
> > > +};
> > > +
> > > +static const struct nct720x_adc_model_data nct7202_model_data = {
> > > +     .model_name = "nct7202",
> > > +     .channels = nct7202_channels,
> > > +     .num_channels = ARRAY_SIZE(nct7202_channels),
> > > +     .vin_max = 12,
> > > +};
> > > +
> > > +static int nct720x_init_chip(struct nct720x_chip_info *chip)
> > > +{
> > > +     u8 data[2];
> > > +     unsigned int value;
> > > +     int err;
> > > +
> > > +     err = regmap_write(chip->regmap, REG_CONFIGURATION, BIT_CONFIGURATION_RESET);
> > > +     if (err) {
> > > +             dev_err(&chip->client->dev, "Failed to write REG_CONFIGURATION\n");
> > > +             return err;
> > > +     }
> > > +
> > > +     /*
> > > +      * 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, REG_BUSY_STATUS, &value);
> > > +     if (err < 0)
> > > +             return err;
> > > +     if (!(value & BIT_PWR_UP))
> > > +             return err;
> > > +
> > > +     /* Enable Channel */
> > > +     err = regmap_write(chip->regmap, REG_CHANNEL_ENABLE_1, REG_CHANNEL_ENABLE_1_MASK);
> > > +     if (err) {
> > > +             dev_err(&chip->client->dev, "Failed to write REG_CHANNEL_ENABLE_1\n");
> > > +             return err;
> > > +     }
> > > +
> > > +     if (chip->vin_max == 12) {
> > > +             err = regmap_write(chip->regmap, REG_CHANNEL_ENABLE_2, REG_CHANNEL_ENABLE_2_MASK);
> > > +             if (err) {
> > > +                     dev_err(&chip->client->dev, "Failed to write REG_CHANNEL_ENABLE_2\n");
> > > +                     return err;
> > > +             }
> > > +     }
> > > +
> > > +     guard(mutex)(&chip->access_lock);
> > > +     err  = regmap_read(chip->regmap, REG_CHANNEL_ENABLE_1, &value);
> > > +     if (err < 0)
> > > +             return err;
> > > +     data[0] = (u8)value;
> > > +
> > > +     err  = regmap_read(chip->regmap, REG_CHANNEL_ENABLE_2, &value);
> > > +     if (err < 0)
> > > +             return err;
> >
> > Here I think you can use a bulk read as the registers are next to each other.
> >
>
Generally, registers with 8 bits support Byte format, and registers
with more than 8 bits support Word format.
If transmission a Word command to a register that supports Byte
format, the second byte will get 0xFF.
Here, if we use regmap_bulk_read(), we would get first byte correct
and second byte is wrong 0xff.

I use i2ctransfer command to demo it.
root@evb-npcm845:~# i2ctransfer -f -y 5 w1@0x1d 0x13 r1
0xff
root@evb-npcm845:~# i2ctransfer -f -y 5 w1@0x1d 0x14 r1
0x0f

root@evb-npcm845:~# i2ctransfer -f -y 5 w1@0x1d 0x13 r2
0xff 0xff
And if we read four bytes, you can see the first and third byte as we wanted.
root@evb-npcm845:~# i2ctransfer -f -y 5 w1@0x1d 0x13 r4
0xff 0xff 0x0f 0xff

so we can't use bulk read directly since it would get a second byte 0xff.
The safe method is to  use read byte twice.

>
> > > +     data[1] = (u8)value;
> > > +
> > > +     value = get_unaligned_le16(data);
> > > +     chip->vin_mask = value;
> > > +
> > > +     /* Start monitoring if needed */
> > > +     err = regmap_read(chip->regmap, REG_CONFIGURATION, &value);
> > > +     if (err < 0) {
> > > +             dev_err(&chip->client->dev, "Failed to read REG_CONFIGURATION\n");
> > > +             return value;
> > > +     }
> > > +
> > > +     value |= BIT_CONFIGURATION_START;
> > > +     err = regmap_write(chip->regmap, REG_CONFIGURATION, value);
> > > +     if (err < 0) {
> > > +             dev_err(&chip->client->dev, "Failed to write REG_CONFIGURATION\n");
> > > +             return err;
> > > +     }
> > > +
> > > +     return 0;
> > > +}

Yu-Hsian Yang Dec. 10, 2024, 5:59 a.m. UTC | #14

Dear Christophe JAILLET,

Thanks for your comment.

Christophe JAILLET <[e-mail]> 於 2024年12月9日 週一 上午1:47寫道：
>
> Le 03/12/2024 à 10:15, Eason Yang a écrit :
> > 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 <[e-mail]>
> > ---
>
> ...
>
> > +static const u8 REG_VIN_HIGH_LIMIT_LSB[VIN_MAX] = {
> > +     0x40, 0x42, 0x44, 0x46, 0x48, 0x4A, 0x4C, 0x4E,
> > +     0x50, 0x52, 0x54, 0x56,
> > +};
> > +static const u8 REG_VIN_LOW_LIMIT_LSB[VIN_MAX] = {
> > +     0x41, 0x43, 0x45, 0x47, 0x49, 0x4B, 0x4D, 0x4F,
> > +     0x51, 0x53, 0x55, 0x57,
> > +};
> > +static u8 nct720x_chan_to_index[] = {
>
> const as well here?
>
> > +     0 /* Not used */, 0, 1, 2, 3, 4, 5, 6,
> > +     7, 8, 9, 10, 11,
> > +};
>

Yes, it should add const here,
Finally we would remove nct720x_chan_to_index tables.
We would just store this value in the address field.

> ...
>
> > +static int nct720x_read_raw(struct iio_dev *indio_dev,
> > +                         struct iio_chan_spec const *chan,
> > +                         int *val, int *val2, long mask)
> > +{
> > +     int index = nct720x_chan_to_index[chan->address];
> > +     u16 volt;
> > +     unsigned int value;
> > +     int err;
> > +     struct nct720x_chip_info *chip = iio_priv(indio_dev);
> > +
> > +     if (chan->type != IIO_VOLTAGE)
> > +             return -EOPNOTSUPP;
> > +
> > +     guard(mutex)(&chip->access_lock);
>
> The IIO_CHAN_INFO_SCALE case does not seem to need the lock. Would it
> make sense to move it only in the IIO_CHAN_INFO_RAW case?
>

Remove guard(mutex) here.

> > +     switch (mask) {
> > +     case IIO_CHAN_INFO_RAW:
> > +             err = regmap_read(chip->regmap16, REG_VIN[index], &value);
> > +             if (err < 0)
> > +                     return err;
> > +             volt = (u16)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 nct720x_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)
> > +{
> > +     int err = 0;
>
> Harmless but useless initialisation.
>

We would remove unused err variables.
Since it is very rare for regmap_write() to fail so usually we don't
print an error message for these.

> > +     struct nct720x_chip_info *chip = iio_priv(indio_dev);
> > +     int index = nct720x_chan_to_index[chan->address];
> > +     unsigned int mask;
> > +
> > +     if (chan->type != IIO_VOLTAGE)
> > +             return -EOPNOTSUPP;
>
> ...
>
> > +static int nct720x_init_chip(struct nct720x_chip_info *chip)
> > +{
> > +     u8 data[2];
> > +     unsigned int value;
> > +     int err;
> > +
> > +     err = regmap_write(chip->regmap, REG_CONFIGURATION, BIT_CONFIGURATION_RESET);
> > +     if (err) {
> > +             dev_err(&chip->client->dev, "Failed to write REG_CONFIGURATION\n");
> > +             return err;
> > +     }
> > +
> > +     /*
> > +      * 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, REG_BUSY_STATUS, &value);
>
> double space after err.
>

Okay.

> > +     if (err < 0)
> > +             return err;
> > +     if (!(value & BIT_PWR_UP))
> > +             return err;
> > +
> > +     /* Enable Channel */
> > +     err = regmap_write(chip->regmap, REG_CHANNEL_ENABLE_1, REG_CHANNEL_ENABLE_1_MASK);
> > +     if (err) {
> > +             dev_err(&chip->client->dev, "Failed to write REG_CHANNEL_ENABLE_1\n");
> > +             return err;
> > +     }
> > +
> > +     if (chip->vin_max == 12) {
> > +             err = regmap_write(chip->regmap, REG_CHANNEL_ENABLE_2, REG_CHANNEL_ENABLE_2_MASK);
> > +             if (err) {
> > +                     dev_err(&chip->client->dev, "Failed to write REG_CHANNEL_ENABLE_2\n");
> > +                     return err;
> > +             }
> > +     }
> > +
> > +     guard(mutex)(&chip->access_lock);
> > +     err  = regmap_read(chip->regmap, REG_CHANNEL_ENABLE_1, &value);
>
> double space after err.
>

Okay.

> > +     if (err < 0)
> > +             return err;
> > +     data[0] = (u8)value;
> > +
> > +     err  = regmap_read(chip->regmap, REG_CHANNEL_ENABLE_2, &value);
>
> double space after err.
>

Okay.

> > +     if (err < 0)
> > +             return err;
> > +     data[1] = (u8)value;
> > +
> > +     value = get_unaligned_le16(data);
> > +     chip->vin_mask = value;
> > +
> > +     /* Start monitoring if needed */
>
> ...
>
> CJ

Jonathan Cameron Dec. 11, 2024, 6:26 p.m. UTC | #15

On Tue, 10 Dec 2024 13:38:20 +0800
Yu-Hsian Yang <[e-mail]> wrote:

> Dear Jonathan Cameron,
> 
> Thanks for your comment.
> 
> Jonathan Cameron <[e-mail]> 於 2024年12月9日 週一 上午1:22寫道：
> >
> > On Tue,  3 Dec 2024 17:15:40 +0800
> > Eason Yang <[e-mail]> 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 <[e-mail]>  
> > Hi Eason,
> >
> > Given you have some good reviews already I only took a very quick glance
> > through.  A few things inline
> >
> > Jonathan
> >  
> > > diff --git a/drivers/iio/adc/nct720x.c b/drivers/iio/adc/nct720x.c
> > > new file mode 100644
> > > index 000000000000..b28b5f4d7d70
> > > --- /dev/null
> > > +++ b/drivers/iio/adc/nct720x.c  
> >  
> > > +
> > > +static int nct720x_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 nct720x_chip_info *chip = iio_priv(indio_dev);
> > > +     int index, err = 0;
> > > +     long v1, v2, volt;
> > > +
> > > +     index = nct720x_chan_to_index[chan->address];
> > > +     volt = (val * NCT720X_IN_SCALING_FACTOR) / NCT720X_IN_SCALING;
> > > +     v1 = volt >> 5;
> > > +     v2 = (volt & REG_VIN_LIMIT_LSB_MASK) << 3;
> > > +
> > > +     if (chan->type != IIO_VOLTAGE)
> > > +             return -EOPNOTSUPP;
> > > +
> > > +     if (info == IIO_EV_INFO_VALUE) {
> > > +             if (dir == IIO_EV_DIR_FALLING) {
> > > +                     guard(mutex)(&chip->access_lock);  
> >
> > Might as well move this up one level as it is called in both legs.
> >  
> 
> I would remove guard(mutex) up one level.
A small process thing.  There is no need to reply to parts of a review
where you agree.  It just means more to read for everyone!

I assume if you didn't comment you are fine with the feedback.
Just crop down to the bits where discussion is needed.

> 
> > > +                     err = regmap_write(chip->regmap, REG_VIN_LOW_LIMIT[index], v1);
> > > +                     if (err < 0)
> > > +                             dev_err(&indio_dev->dev, "Failed to write REG_VIN%d_LOW_LIMIT\n",
> > > +                                     index + 1);
> > > +
> > > +                     err = regmap_write(chip->regmap, REG_VIN_LOW_LIMIT_LSB[index], v2);
> > > +                     if (err < 0)
> > > +                             dev_err(&indio_dev->dev, "Failed to write REG_VIN%d_LOW_LIMIT_LSB\n",
> > > +                                     index + 1);
> > > +
> > > +             } else {
> > > +                     guard(mutex)(&chip->access_lock);
> > > +                     err = regmap_write(chip->regmap, REG_VIN_HIGH_LIMIT[index], v1);
> > > +                     if (err < 0)
> > > +                             dev_err(&indio_dev->dev, "Failed to write REG_VIN%d_HIGH_LIMIT\n",
> > > +                                     index + 1);
> > > +
> > > +                     err = regmap_write(chip->regmap, REG_VIN_HIGH_LIMIT_LSB[index], v2);
> > > +                     if (err < 0)
> > > +                             dev_err(&indio_dev->dev, "Failed to write REG_VIN%d_HIGH_LIMIT_LSB\n",
> > > +                                     index + 1);
> > > +             }
> > > +     }
> > > +     return err;
> > > +}  
> >  
> > > +
> > > +static const struct iio_info nct720x_info = {
> > > +     .read_raw = nct720x_read_raw,
> > > +     .read_event_config = nct720x_read_event_config,
> > > +     .write_event_config = nct720x_write_event_config,
> > > +     .read_event_value = nct720x_read_event_value,
> > > +     .write_event_value = nct720x_write_event_value,  
> >
> > Given you are supporting with and without interrupts, should probably pick between
> > versions of this that have the event config part and one that doesn't.
> >  
> 
> Sorry, could you give some examples for us to refer.
Sure, something like:

static const struct iio_info nct720x_info = {
     .read_raw = nct720x_read_raw,
     .read_event_config = nct720x_read_event_config,
     .write_event_config = nct720x_write_event_config,
     .read_event_value = nct720x_read_event_value,
     .write_event_value = nct720x_write_event_value,  
};

static const struct iio_info nct720x_info_no_irq = {
     .read_raw = nct720x_read_raw,
};

if (irq)
	indio_dev->info = nct720x_info;
else
	indio_dev->info = nct720x_info_no_irq;

It isn't strictly necessary I think, but it is cleaner to not provide
callbacks that should not be called.
   }
> > > +
> > > +     guard(mutex)(&chip->access_lock);
> > > +     err  = regmap_read(chip->regmap, REG_CHANNEL_ENABLE_1, &value);
> > > +     if (err < 0)
> > > +             return err;
> > > +     data[0] = (u8)value;
> > > +
> > > +     err  = regmap_read(chip->regmap, REG_CHANNEL_ENABLE_2, &value);
> > > +     if (err < 0)
> > > +             return err;  
> >
> > Here I think you can use a bulk read as the registers are next to each other.
> >  
> 
> Generally, registers with 8 bits support Byte format, and registers
> with more than 8 bits support Word format.
> If transmission a Word command to a register that supports Byte
> format, the second byte will get 0xFF.
> Here, if we use regmap_bulk_read(), we would get first byte correct
> and second byte is wrong 0xff.
> I use i2ctransfer to demo it.
> root@evb-npcm845:~# i2ctransfer -f -y 5 w1@0x1d 0x13 r1
> 0xff
> root@evb-npcm845:~# i2ctransfer -f -y 5 w1@0x1d 0x14 r1
> 0x0f

As your regmap for these registers is 8 bit one the function here:
https://elixir.bootlin.com/linux/v6.12.4/source/drivers/base/regmap/regmap-i2c.c#L306
should have picked you a regmap bus config that only does 8 bit reads.

Thus when you use a regmap_bulk_read() it should issue two of those to neighbouring
registers, not use a word read.  So that should work in this specific case.


Thanks,

Jonathan

Jonathan Cameron Dec. 11, 2024, 6:28 p.m. UTC | #16

On Tue, 10 Dec 2024 13:47:25 +0800
Yu-Hsian Yang <[e-mail]> wrote:

> Dear Jonathan Cameron,
> 
> Sorry the above mail is not finished and just sent it.
> I would explain why we can't use bulk read sequential bytes in our chips.
Ah! I replied to previous. Let me see what you added.


> > > > +
> > > > +     guard(mutex)(&chip->access_lock);
> > > > +     err  = regmap_read(chip->regmap, REG_CHANNEL_ENABLE_1, &value);
> > > > +     if (err < 0)
> > > > +             return err;
> > > > +     data[0] = (u8)value;
> > > > +
> > > > +     err  = regmap_read(chip->regmap, REG_CHANNEL_ENABLE_2, &value);
> > > > +     if (err < 0)
> > > > +             return err;  
> > >
> > > Here I think you can use a bulk read as the registers are next to each other.
> > >  
> >  
> Generally, registers with 8 bits support Byte format, and registers
> with more than 8 bits support Word format.
> If transmission a Word command to a register that supports Byte
> format, the second byte will get 0xFF.
> Here, if we use regmap_bulk_read(), we would get first byte correct
> and second byte is wrong 0xff.
> 
> I use i2ctransfer command to demo it.
> root@evb-npcm845:~# i2ctransfer -f -y 5 w1@0x1d 0x13 r1
> 0xff
> root@evb-npcm845:~# i2ctransfer -f -y 5 w1@0x1d 0x14 r1
> 0x0f
> 
> root@evb-npcm845:~# i2ctransfer -f -y 5 w1@0x1d 0x13 r2
> 0xff 0xff
> And if we read four bytes, you can see the first and third byte as we wanted.
> root@evb-npcm845:~# i2ctransfer -f -y 5 w1@0x1d 0x13 r4
> 0xff 0xff 0x0f 0xff
> 
> so we can't use bulk read directly since it would get a second byte 0xff.
> The safe method is to  use read byte twice.
That command does not do the same thing as regmap_bulk_read() will here.
It will issue a series of byte reads.

Jonathan

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index bea10a846475..573b12f0cd4d 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/nct720x.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..6eed518efa1c 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 NCT720X
+	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 nct720x.
+
 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..89f5b5d1a567 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_NCT720X) += nct720x.o
 obj-$(CONFIG_NPCM_ADC) += npcm_adc.o
 obj-$(CONFIG_PAC1921) += pac1921.o
 obj-$(CONFIG_PAC1934) += pac1934.o
diff --git a/drivers/iio/adc/nct720x.c b/drivers/iio/adc/nct720x.c
new file mode 100644
index 000000000000..b28b5f4d7d70
--- /dev/null
+++ b/drivers/iio/adc/nct720x.c
@@ -0,0 +1,533 @@ 
+// 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>
+#include <linux/iio/sysfs.h>
+
+#define VIN_MAX					12	/* Counted from 1 */
+#define NCT720X_IN_SCALING			4995
+#define NCT720X_IN_SCALING_FACTOR		10000
+
+#define REG_INTERRUPT_STATUS_1			0x0C
+#define REG_INTERRUPT_STATUS_2			0x0D
+#define REG_VOLT_LOW_BYTE			0x0F
+#define REG_CONFIGURATION			0x10
+#define  BIT_CONFIGURATION_START		BIT(0)
+#define  BIT_CONFIGURATION_ALERT_MSK		BIT(1)
+#define  BIT_CONFIGURATION_CONV_RATE		BIT(2)
+#define  BIT_CONFIGURATION_RESET		BIT(7)
+
+#define REG_ADVANCED_CONFIGURATION		0x11
+#define  BIT_ADVANCED_CONF_MOD_ALERT		BIT(0)
+#define  BIT_ADVANCED_CONF_MOD_STS		BIT(1)
+#define  BIT_ADVANCED_CONF_FAULT_QUEUE		BIT(2)
+#define  BIT_ADVANCED_CONF_EN_DEEP_SHUTDOWN	BIT(4)
+#define  BIT_ADVANCED_CONF_EN_SMB_TIMEOUT	BIT(5)
+#define  BIT_ADVANCED_CONF_MOD_RSTIN		BIT(7)
+
+#define REG_CHANNEL_INPUT_MODE			0x12
+#define REG_CHANNEL_ENABLE_1			0x13
+#define  REG_CHANNEL_ENABLE_1_MASK		GENMASK(7, 0)
+#define REG_CHANNEL_ENABLE_2			0x14
+#define  REG_CHANNEL_ENABLE_2_MASK		GENMASK(3, 0)
+#define REG_INTERRUPT_MASK_1			0x15
+#define REG_INTERRUPT_MASK_2			0x16
+#define REG_BUSY_STATUS				0x1E
+#define  BIT_BUSY				BIT(0)
+#define  BIT_PWR_UP				BIT(1)
+#define REG_ONE_SHOT				0x1F
+#define REG_SMUS_ADDRESS			0xFC
+#define REG_VIN_LIMIT_LSB_MASK			GENMASK(4, 0)
+
+static const u8 REG_VIN[VIN_MAX] = {
+	0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07,	/* 0 -7 */
+	0x08, 0x09, 0x0A, 0x0B,				/* 8 - 11 */
+};
+static const u8 REG_VIN_HIGH_LIMIT[VIN_MAX] = {
+	0x20, 0x22, 0x24, 0x26, 0x28, 0x2A, 0x2C, 0x2E,
+	0x30, 0x32, 0x34, 0x36,
+};
+static const u8 REG_VIN_LOW_LIMIT[VIN_MAX] = {
+	0x21, 0x23, 0x25, 0x27, 0x29, 0x2B, 0x2D, 0x2F,
+	0x31, 0x33, 0x35, 0x37,
+};
+static const u8 REG_VIN_HIGH_LIMIT_LSB[VIN_MAX] = {
+	0x40, 0x42, 0x44, 0x46, 0x48, 0x4A, 0x4C, 0x4E,
+	0x50, 0x52, 0x54, 0x56,
+};
+static const u8 REG_VIN_LOW_LIMIT_LSB[VIN_MAX] = {
+	0x41, 0x43, 0x45, 0x47, 0x49, 0x4B, 0x4D, 0x4F,
+	0x51, 0x53, 0x55, 0x57,
+};
+static u8 nct720x_chan_to_index[] = {
+	0 /* Not used */, 0, 1, 2, 3, 4, 5, 6,
+	7, 8, 9, 10, 11,
+};
+
+struct nct720x_chip_info {
+	struct i2c_client *client;
+	struct mutex access_lock;	/* for multi-byte read and write operations */
+	struct regmap *regmap;
+	struct regmap *regmap16;
+	int vin_max;			/* number of VIN channels */
+	u32 vin_mask;
+};
+
+struct nct720x_adc_model_data {
+	const char *model_name;
+	const struct iio_chan_spec *channels;
+	const int num_channels;
+	int vin_max;
+};
+
+static const struct iio_event_spec nct720x_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 NCT720X_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 = nct720x_events,				\
+		.num_event_specs = ARRAY_SIZE(nct720x_events),		\
+	}
+
+#define NCT720X_VOLTAGE_CHANNEL_DIFF(chan1, chan2, addr)		\
+	{								\
+		.type = IIO_VOLTAGE,					\
+		.indexed = 1,						\
+		.channel = (chan1),					\
+		.channel2 = (chan2),					\
+		.differential = 1,					\
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
+		.address = addr,					\
+		.event_spec = nct720x_events,				\
+		.num_event_specs = ARRAY_SIZE(nct720x_events),		\
+	}
+
+static const struct iio_chan_spec nct7201_channels[] = {
+	NCT720X_VOLTAGE_CHANNEL(1, 1),
+	NCT720X_VOLTAGE_CHANNEL(2, 2),
+	NCT720X_VOLTAGE_CHANNEL(3, 3),
+	NCT720X_VOLTAGE_CHANNEL(4, 4),
+	NCT720X_VOLTAGE_CHANNEL(5, 5),
+	NCT720X_VOLTAGE_CHANNEL(6, 6),
+	NCT720X_VOLTAGE_CHANNEL(7, 7),
+	NCT720X_VOLTAGE_CHANNEL(8, 8),
+	NCT720X_VOLTAGE_CHANNEL_DIFF(1, 2, 1),
+	NCT720X_VOLTAGE_CHANNEL_DIFF(3, 4, 3),
+	NCT720X_VOLTAGE_CHANNEL_DIFF(5, 6, 5),
+	NCT720X_VOLTAGE_CHANNEL_DIFF(7, 8, 7),
+};
+
+static const struct iio_chan_spec nct7202_channels[] = {
+	NCT720X_VOLTAGE_CHANNEL(1, 1),
+	NCT720X_VOLTAGE_CHANNEL(2, 2),
+	NCT720X_VOLTAGE_CHANNEL(3, 3),
+	NCT720X_VOLTAGE_CHANNEL(4, 4),
+	NCT720X_VOLTAGE_CHANNEL(5, 5),
+	NCT720X_VOLTAGE_CHANNEL(6, 6),
+	NCT720X_VOLTAGE_CHANNEL(7, 7),
+	NCT720X_VOLTAGE_CHANNEL(8, 8),
+	NCT720X_VOLTAGE_CHANNEL(9, 9),
+	NCT720X_VOLTAGE_CHANNEL(10, 10),
+	NCT720X_VOLTAGE_CHANNEL(11, 11),
+	NCT720X_VOLTAGE_CHANNEL(12, 12),
+	NCT720X_VOLTAGE_CHANNEL_DIFF(1, 2, 1),
+	NCT720X_VOLTAGE_CHANNEL_DIFF(3, 4, 3),
+	NCT720X_VOLTAGE_CHANNEL_DIFF(5, 6, 5),
+	NCT720X_VOLTAGE_CHANNEL_DIFF(7, 8, 7),
+	NCT720X_VOLTAGE_CHANNEL_DIFF(9, 10, 9),
+	NCT720X_VOLTAGE_CHANNEL_DIFF(11, 12, 11),
+};
+
+static int nct720x_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int *val, int *val2, long mask)
+{
+	int index = nct720x_chan_to_index[chan->address];
+	u16 volt;
+	unsigned int value;
+	int err;
+	struct nct720x_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, REG_VIN[index], &value);
+		if (err < 0)
+			return err;
+		volt = (u16)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 nct720x_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 nct720x_chip_info *chip = iio_priv(indio_dev);
+	u16 volt;
+	unsigned int value;
+	int tmp, err;
+	int index = nct720x_chan_to_index[chan->address];
+
+	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, REG_VIN_LOW_LIMIT[index], &value);
+		if (err < 0)
+			return err;
+		volt = (u16)value;
+	} else {
+		err = regmap_read(chip->regmap16, REG_VIN_HIGH_LIMIT[index], &value);
+		if (err < 0)
+			return err;
+		volt = (u16)value;
+	}
+
+	/* Voltage(V) = 13bitCountValue * 0.0004995 */
+	tmp = (volt >> 3) * NCT720X_IN_SCALING;
+	*val = tmp / NCT720X_IN_SCALING_FACTOR;
+
+	return IIO_VAL_INT;
+}
+
+static int nct720x_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 nct720x_chip_info *chip = iio_priv(indio_dev);
+	int index, err = 0;
+	long v1, v2, volt;
+
+	index = nct720x_chan_to_index[chan->address];
+	volt = (val * NCT720X_IN_SCALING_FACTOR) / NCT720X_IN_SCALING;
+	v1 = volt >> 5;
+	v2 = (volt & REG_VIN_LIMIT_LSB_MASK) << 3;
+
+	if (chan->type != IIO_VOLTAGE)
+		return -EOPNOTSUPP;
+
+	if (info == IIO_EV_INFO_VALUE) {
+		if (dir == IIO_EV_DIR_FALLING) {
+			guard(mutex)(&chip->access_lock);
+			err = regmap_write(chip->regmap, REG_VIN_LOW_LIMIT[index], v1);
+			if (err < 0)
+				dev_err(&indio_dev->dev, "Failed to write REG_VIN%d_LOW_LIMIT\n",
+					index + 1);
+
+			err = regmap_write(chip->regmap, REG_VIN_LOW_LIMIT_LSB[index], v2);
+			if (err < 0)
+				dev_err(&indio_dev->dev, "Failed to write REG_VIN%d_LOW_LIMIT_LSB\n",
+					index + 1);
+
+		} else {
+			guard(mutex)(&chip->access_lock);
+			err = regmap_write(chip->regmap, REG_VIN_HIGH_LIMIT[index], v1);
+			if (err < 0)
+				dev_err(&indio_dev->dev, "Failed to write REG_VIN%d_HIGH_LIMIT\n",
+					index + 1);
+
+			err = regmap_write(chip->regmap, REG_VIN_HIGH_LIMIT_LSB[index], v2);
+			if (err < 0)
+				dev_err(&indio_dev->dev, "Failed to write REG_VIN%d_HIGH_LIMIT_LSB\n",
+					index + 1);
+		}
+	}
+	return err;
+}
+
+static int nct720x_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 nct720x_chip_info *chip = iio_priv(indio_dev);
+	int index = nct720x_chan_to_index[chan->address];
+
+	if (chan->type != IIO_VOLTAGE)
+		return -EOPNOTSUPP;
+
+	return !!(chip->vin_mask & BIT(index));
+}
+
+static int nct720x_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)
+{
+	int err = 0;
+	struct nct720x_chip_info *chip = iio_priv(indio_dev);
+	int index = nct720x_chan_to_index[chan->address];
+	unsigned int mask;
+
+	if (chan->type != IIO_VOLTAGE)
+		return -EOPNOTSUPP;
+
+	mask = BIT(index);
+
+	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);
+
+	err = regmap_write(chip->regmap, REG_CHANNEL_ENABLE_1,
+			   chip->vin_mask & REG_CHANNEL_ENABLE_1_MASK);
+	if (err < 0)
+		dev_err(&indio_dev->dev, "Failed to write REG_CHANNEL_ENABLE_1\n");
+
+	if (chip->vin_max == 12) {
+		err = regmap_write(chip->regmap, REG_CHANNEL_ENABLE_2, chip->vin_mask >> 8);
+		if (err < 0)
+			dev_err(&indio_dev->dev, "Failed to write REG_CHANNEL_ENABLE_2\n");
+	}
+	return err;
+}
+
+static const struct iio_info nct720x_info = {
+	.read_raw = nct720x_read_raw,
+	.read_event_config = nct720x_read_event_config,
+	.write_event_config = nct720x_write_event_config,
+	.read_event_value = nct720x_read_event_value,
+	.write_event_value = nct720x_write_event_value,
+};
+
+static const struct nct720x_adc_model_data nct7201_model_data = {
+	.model_name = "nct7201",
+	.channels = nct7201_channels,
+	.num_channels = ARRAY_SIZE(nct7201_channels),
+	.vin_max = 8,
+};
+
+static const struct nct720x_adc_model_data nct7202_model_data = {
+	.model_name = "nct7202",
+	.channels = nct7202_channels,
+	.num_channels = ARRAY_SIZE(nct7202_channels),
+	.vin_max = 12,
+};
+
+static int nct720x_init_chip(struct nct720x_chip_info *chip)
+{
+	u8 data[2];
+	unsigned int value;
+	int err;
+
+	err = regmap_write(chip->regmap, REG_CONFIGURATION, BIT_CONFIGURATION_RESET);
+	if (err) {
+		dev_err(&chip->client->dev, "Failed to write REG_CONFIGURATION\n");
+		return err;
+	}
+
+	/*
+	 * 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, REG_BUSY_STATUS, &value);
+	if (err < 0)
+		return err;
+	if (!(value & BIT_PWR_UP))
+		return err;
+
+	/* Enable Channel */
+	err = regmap_write(chip->regmap, REG_CHANNEL_ENABLE_1, REG_CHANNEL_ENABLE_1_MASK);
+	if (err) {
+		dev_err(&chip->client->dev, "Failed to write REG_CHANNEL_ENABLE_1\n");
+		return err;
+	}
+
+	if (chip->vin_max == 12) {
+		err = regmap_write(chip->regmap, REG_CHANNEL_ENABLE_2, REG_CHANNEL_ENABLE_2_MASK);
+		if (err) {
+			dev_err(&chip->client->dev, "Failed to write REG_CHANNEL_ENABLE_2\n");
+			return err;
+		}
+	}
+
+	guard(mutex)(&chip->access_lock);
+	err  = regmap_read(chip->regmap, REG_CHANNEL_ENABLE_1, &value);
+	if (err < 0)
+		return err;
+	data[0] = (u8)value;
+
+	err  = regmap_read(chip->regmap, REG_CHANNEL_ENABLE_2, &value);
+	if (err < 0)
+		return err;
+	data[1] = (u8)value;
+
+	value = get_unaligned_le16(data);
+	chip->vin_mask = value;
+
+	/* Start monitoring if needed */
+	err = regmap_read(chip->regmap, REG_CONFIGURATION, &value);
+	if (err < 0) {
+		dev_err(&chip->client->dev, "Failed to read REG_CONFIGURATION\n");
+		return value;
+	}
+
+	value |= BIT_CONFIGURATION_START;
+	err = regmap_write(chip->regmap, REG_CONFIGURATION, value);
+	if (err < 0) {
+		dev_err(&chip->client->dev, "Failed to write REG_CONFIGURATION\n");
+		return err;
+	}
+
+	return 0;
+}
+
+static const struct regmap_config nct720x_regmap8_config = {
+	.name = "vin-data-read-byte",
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = 0xff,
+	.cache_type = REGCACHE_NONE,
+};
+
+static const struct regmap_config nct720x_regmap16_config = {
+	.name = "vin-data-read-word",
+	.reg_bits = 8,
+	.val_bits = 16,
+	.max_register = 0xff,
+	.cache_type = REGCACHE_NONE,
+};
+
+static int nct720x_probe(struct i2c_client *client)
+{
+	const struct nct720x_adc_model_data *model_data;
+	struct nct720x_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, &nct720x_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, &nct720x_regmap16_config);
+	if (IS_ERR(chip->regmap16))
+		return dev_err_probe(&client->dev, PTR_ERR(chip->regmap16),
+			"Failed to init regmap16\n");
+
+	chip->vin_max = model_data->vin_max;
+
+	ret = devm_mutex_init(&client->dev, &chip->access_lock);
+	if (ret)
+		return ret;
+
+	chip->client = client;
+
+	ret = nct720x_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;
+	indio_dev->info = &nct720x_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	return devm_iio_device_register(&client->dev, indio_dev);
+}
+
+static const struct i2c_device_id nct720x_id[] = {
+	{ "nct7201", (kernel_ulong_t)&nct7201_model_data },
+	{ "nct7202", (kernel_ulong_t)&nct7202_model_data },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, nct720x_id);
+
+static const struct of_device_id nct720x_of_match[] = {
+	{
+		.compatible = "nuvoton,nct7201",
+		.data = &nct7201_model_data,
+	},
+	{
+		.compatible = "nuvoton,nct7202",
+		.data = &nct7202_model_data,
+	},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, nct720x_of_match);
+
+static struct i2c_driver nct720x_driver = {
+	.driver = {
+		.name	= "nct720x",
+		.of_match_table = nct720x_of_match,
+	},
+	.probe = nct720x_probe,
+	.id_table = nct720x_id,
+};
+module_i2c_driver(nct720x_driver);
+
+MODULE_AUTHOR("Eason Yang <[e-mail]>");
+MODULE_DESCRIPTION("Nuvoton NCT720x voltage monitor driver");
+MODULE_LICENSE("GPL");