Re: [PATCH v7 2/2] iio: adc: add support for Nuvoton NCT7201

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Thu, Apr 24, 2025 at 04:30:00PM +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 all the 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 are
> supported.

Very good, from my point of view it's almost ready, a few nit-picks below.

...

> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/delay.h>

> +#include <linux/device.h>

It seems this is not used, but missing
dev_printk.h
Am I mistaken?

> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/types.h>
> +#include <linux/unaligned.h>

...

> +static int nct7201_write_event_value(struct iio_dev *indio_dev,
> +				     const struct iio_chan_spec *chan,
> +				     enum iio_event_type type,
> +				     enum iio_event_direction dir,
> +				     enum iio_event_info info,
> +				     int val, int val2)
> +{
> +	struct nct7201_chip_info *chip = iio_priv(indio_dev);
> +	int  err;
> +
> +	if (chan->type != IIO_VOLTAGE)
> +		return -EOPNOTSUPP;
> +
> +	if (info != IIO_EV_INFO_VALUE)
> +		return -EOPNOTSUPP;
> +
> +	if (dir == IIO_EV_DIR_FALLING)
> +		err = regmap_write(chip->regmap16, NCT7201_REG_VIN_LOW_LIMIT(chan->address),
> +				   FIELD_PREP(NCT7201_REG_VIN_MASK, val));
> +	else
> +		err = regmap_write(chip->regmap16, NCT7201_REG_VIN_HIGH_LIMIT(chan->address),
> +				   FIELD_PREP(NCT7201_REG_VIN_MASK, val));

> +	if (err)
> +		return err;
> +
> +	return 0;

	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.
> +	 */
> +	fsleep(25000);

25 * USEC_PER_MSEC ?

...

> +	/* Enable Channel */
> +	if (chip->num_vin_channels <= 8) {
> +		err = regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE,
> +				   GENMASK(chip->num_vin_channels - 1, 0));

> +		if (err)
> +			return dev_err_probe(dev, err, "Failed to enable channel\n");

This...

> +	} else {
> +		err = regmap_bulk_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE,
> +					&data, sizeof(data));

> +		if (err)
> +			return dev_err_probe(dev2, err, "Failed to enable channel\n");

...and this are identical, deduplicate by moving outside of if-else.

> +	}


-- 
With Best Regards,
Andy Shevchenko