Dear Andy,
Thanks for your comments.
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> 於 2024年12月27日 週五 下午8:14寫道：
>
> On Thu, Dec 26, 2024 at 01:53:13PM +0800, Eason Yang wrote:
> > Add Nuvoton NCT7201/NCT7202 system voltage monitor 12-bit ADC driver
> >
> > NCT7201/NCT7202 supports up to 12 analog voltage monitor inputs and up to
> > 4 SMBus addresses by ADDR pin. Meanwhile, ALERT# hardware event pins for
> > independent alarm signals, and the all threshold values could be set for
> > system protection without any timing delay. It also supports reset input
> > RSTIN# to recover system from a fault condition.
> >
> > Currently, only single-edge mode conversion and threshold events support.
>
> ...
>
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
> > + err = regmap_read(chip->regmap16, NCT7201_REG_VIN(chan->address), &value);
> > + if (err < 0)
> > + return err;
> > + volt = value;
>
> > + *val = volt >> 3;
>
> I am not sure I understand this. If it's a shifted field, you rather
> should fix it by using FIELD_*() macros from bitfield.h, otherwise
> it's even more confusing.
>
+ #define NCT7201_REG_VIN_MASK GENMASK(15, 3)
- *val = volt >> 3;
+ *val = FIELD_GET(NCT7201_REG_VIN_MASK, volt);
> > + return IIO_VAL_INT;
> > + case IIO_CHAN_INFO_SCALE:
> > + /* From the datasheet, we have to multiply by 0.0004995 */
> > + *val = 0;
> > + *val2 = 499500;
> > + return IIO_VAL_INT_PLUS_NANO;
> > + default:
> > + return -EINVAL;
> > + }
>
> ...
>
> > + *val = volt >> 3;
>
> Ditto.
>
>
> ...
>
> > + v1 = val >> 5;
> > + v2 = FIELD_PREP(NCT7201_REG_VIN_LIMIT_LSB_MASK, val) << 3;
>
> Ditto.
>
The VIN reading supports Byte read (One Byte) and Word read (Two
Byte), the same as the VIN writing.
We don't need to separate 13-bit val into two bytes.
We can use 16-bit regmap to write val with right shift 3 bit to
replace write 8-bit regmap register twice.
> > + if (chan->type != IIO_VOLTAGE)
> > + return -EOPNOTSUPP;
> > +
> > + if (info == IIO_EV_INFO_VALUE) {
> > + guard(mutex)(&chip->access_lock);
> > + if (dir == IIO_EV_DIR_FALLING) {
> > + regmap_write(chip->regmap, NCT7201_REG_VIN_LOW_LIMIT(chan->address), v1);
> > + regmap_write(chip->regmap, NCT7201_REG_VIN_LOW_LIMIT_LSB(chan->address), v2);
> > + } else {
> > + regmap_write(chip->regmap, NCT7201_REG_VIN_HIGH_LIMIT(chan->address), v1);
> > + regmap_write(chip->regmap, NCT7201_REG_VIN_HIGH_LIMIT_LSB(chan->address), v2);
> > + }
if (dir == IIO_EV_DIR_FALLING)
- regmap_write(chip->regmap,
NCT7201_REG_VIN_LOW_LIMIT(chan->address), v1);
- regmap_write(chip->regmap,
NCT7201_REG_VIN_LOW_LIMIT_LSB(chan->address), v2);
+ regmap_write(chip->regmap16,
NCT7201_REG_VIN_LOW_LIMIT(chan->address),
FIELD_PREP(NCT7201_REG_VIN_MASK, val));
else
- regmap_write(chip->regmap,
NCT7201_REG_VIN_HIGH_LIMIT(chan->address), v1);
- regmap_write(chip->regmap,
NCT7201_REG_VIN_HIGH_LIMIT_LSB(chan->address), v2);
+ regmap_write(chip->regmap16,
NCT7201_REG_VIN_HIGH_LIMIT(chan->address),
FIELD_PREP(NCT7201_REG_VIN_MASK, val));
>
> This needs a comment why regmap_bulk_write() can't be used.
>
We can't use regmap_bulk_write() since the chip limit.
regmap_bulk_write(chip->regmap, ..., ..., 2) ,
the first byte is well written, but the second byte don't changed.
> > + }
>
> ...
>
> > +static int nct7201_init_chip(struct nct7201_chip_info *chip)
> > +{
> > + u8 data[2];
> > + unsigned int value;
> > + int err;
> > +
> > + regmap_write(chip->regmap, NCT7201_REG_CONFIGURATION,
> > + NCT7201_BIT_CONFIGURATION_RESET);
>
> No error check?
>
As David Lechner mentioned,
Better would be `return dev_err_probe()`. But it is very rare for
regmap_write() to fail so usually we don't print an error message
for these.
So we would remove print an error message for all remap_write.
> > + /*
> > + * After about 25 msecs, the device should be ready and then
> > + * the Power Up bit will be set to 1. If not, wait for it.
> > + */
> > + mdelay(25);
> > + err = regmap_read(chip->regmap, NCT7201_REG_BUSY_STATUS, &value);
> > + if (err < 0)
> > + return err;
> > + if (!(value & NCT7201_BIT_PWR_UP))
> > + return dev_err_probe(&chip->client->dev, -EIO, "failed to power up after reset\n");
> > +
> > + /* Enable Channel */
> > + guard(mutex)(&chip->access_lock);
> > + regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1,
> > + NCT7201_REG_CHANNEL_ENABLE_1_MASK);
> > + if (chip->num_vin_channels == 12)
> > + regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_2,
> > + NCT7201_REG_CHANNEL_ENABLE_2_MASK);
> > +
> > + err = regmap_read(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1, &value);
> > + if (err < 0)
> > + return err;
> > + data[0] = value;
> > +
> > + err = regmap_read(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_2, &value);
> > + if (err < 0)
> > + return err;
> > + data[1] = value;
> > +
> > + value = get_unaligned_le16(data);
> > + chip->vin_mask = value;
> > +
> > + /* Start monitoring if needed */
> > + err = regmap_read(chip->regmap, NCT7201_REG_CONFIGURATION, &value);
> > + if (err < 0) {
>
> > + dev_err_probe(&chip->client->dev, -EIO, "Failed to read REG_CONFIGURATION\n");
> > + return value;
>
> You already used
>
> return dev_err_probe(...);
>
> above, why here it's different? You want return something in addition to the
> error code? Why?
>
It should use
return dev_err_probe(&chip->client->dev, -EIO, "Failed to read
REG_CONFIGURATION\n");
> > + }
> > +
> > + value |= NCT7201_BIT_CONFIGURATION_START;
> > + regmap_write(chip->regmap, NCT7201_REG_CONFIGURATION, value);
> > +
> > + return 0;
> > +}
>
> ...
>
> > +static const struct regmap_config nct7201_regmap8_config = {
> > + .name = "vin-data-read-byte",
> > + .reg_bits = 8,
> > + .val_bits = 8,
> > + .max_register = 0xff,
> > +};
> > +
> > +static const struct regmap_config nct7201_regmap16_config = {
> > + .name = "vin-data-read-word",
> > + .reg_bits = 8,
> > + .val_bits = 16,
> > + .max_register = 0xff,
> > +};
>
> I don't see how these configurations will prevent, e.g., debugfs to access
> 16-bit registers via 8-bit IO and vice versa.
>
Read VIN info can use word read or byte read, and other registers
should use byte read.
The design is that VIN info registers are used 16-bit debugfs to access and
other registers are used 8-bit debugfs to access.
We need to probe 8-bit regmap and 16-bit regmap,
but I have no idea how to prevent 8-bit IO to access 16-bit registers
and vice versa.
> --
> With Best Regards,
> Andy Shevchenko
>
>
