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

[Andy Shevchenko]

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

...

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

> +		*val = volt >> 3;

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

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

...

> +	*val = volt >> 3;

Ditto.


...

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

Ditto.

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

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

> +	}

...

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

No error check?

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

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

You already used

	return dev_err_probe(...);

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

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

...

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

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

-- 
With Best Regards,
Andy Shevchenko