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
