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

[Yu-Hsian Yang]

Dear Christophe JAILLET,

Thanks for your comment.

Christophe JAILLET <christophe.jaillet at wanadoo.fr> 於 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 <j2anfernee-Re5JQEeQqe8AvxtiuMwx3w at public.gmane.org>
> > ---
>
> ...
>
> > +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