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

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 10 Dec 2024 13:38:20 +0800
Yu-Hsian Yang <j2anfernee@gmail.com> wrote:

> Dear Jonathan Cameron,
> 
> Thanks for your comment.
> 
> Jonathan Cameron <jic23@kernel.org> 於 2024年12月9日 週一 上午1:22寫道：
> >
> > On Tue,  3 Dec 2024 17:15:40 +0800
> > Eason Yang <j2anfernee@gmail.com> 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.
> > >
> > > Signed-off-by: Eason Yang <j2anfernee@gmail.com>  
> > Hi Eason,
> >
> > Given you have some good reviews already I only took a very quick glance
> > through.  A few things inline
> >
> > Jonathan
> >  
> > > diff --git a/drivers/iio/adc/nct720x.c b/drivers/iio/adc/nct720x.c
> > > new file mode 100644
> > > index 000000000000..b28b5f4d7d70
> > > --- /dev/null
> > > +++ b/drivers/iio/adc/nct720x.c  
> >  
> > > +
> > > +static int nct720x_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 nct720x_chip_info *chip = iio_priv(indio_dev);
> > > +     int index, err = 0;
> > > +     long v1, v2, volt;
> > > +
> > > +     index = nct720x_chan_to_index[chan->address];
> > > +     volt = (val * NCT720X_IN_SCALING_FACTOR) / NCT720X_IN_SCALING;
> > > +     v1 = volt >> 5;
> > > +     v2 = (volt & REG_VIN_LIMIT_LSB_MASK) << 3;
> > > +
> > > +     if (chan->type != IIO_VOLTAGE)
> > > +             return -EOPNOTSUPP;
> > > +
> > > +     if (info == IIO_EV_INFO_VALUE) {
> > > +             if (dir == IIO_EV_DIR_FALLING) {
> > > +                     guard(mutex)(&chip->access_lock);  
> >
> > Might as well move this up one level as it is called in both legs.
> >  
> 
> I would remove guard(mutex) up one level.
A small process thing.  There is no need to reply to parts of a review
where you agree.  It just means more to read for everyone!

I assume if you didn't comment you are fine with the feedback.
Just crop down to the bits where discussion is needed.

> 
> > > +                     err = regmap_write(chip->regmap, REG_VIN_LOW_LIMIT[index], v1);
> > > +                     if (err < 0)
> > > +                             dev_err(&indio_dev->dev, "Failed to write REG_VIN%d_LOW_LIMIT\n",
> > > +                                     index + 1);
> > > +
> > > +                     err = regmap_write(chip->regmap, REG_VIN_LOW_LIMIT_LSB[index], v2);
> > > +                     if (err < 0)
> > > +                             dev_err(&indio_dev->dev, "Failed to write REG_VIN%d_LOW_LIMIT_LSB\n",
> > > +                                     index + 1);
> > > +
> > > +             } else {
> > > +                     guard(mutex)(&chip->access_lock);
> > > +                     err = regmap_write(chip->regmap, REG_VIN_HIGH_LIMIT[index], v1);
> > > +                     if (err < 0)
> > > +                             dev_err(&indio_dev->dev, "Failed to write REG_VIN%d_HIGH_LIMIT\n",
> > > +                                     index + 1);
> > > +
> > > +                     err = regmap_write(chip->regmap, REG_VIN_HIGH_LIMIT_LSB[index], v2);
> > > +                     if (err < 0)
> > > +                             dev_err(&indio_dev->dev, "Failed to write REG_VIN%d_HIGH_LIMIT_LSB\n",
> > > +                                     index + 1);
> > > +             }
> > > +     }
> > > +     return err;
> > > +}  
> >  
> > > +
> > > +static const struct iio_info nct720x_info = {
> > > +     .read_raw = nct720x_read_raw,
> > > +     .read_event_config = nct720x_read_event_config,
> > > +     .write_event_config = nct720x_write_event_config,
> > > +     .read_event_value = nct720x_read_event_value,
> > > +     .write_event_value = nct720x_write_event_value,  
> >
> > Given you are supporting with and without interrupts, should probably pick between
> > versions of this that have the event config part and one that doesn't.
> >  
> 
> Sorry, could you give some examples for us to refer.
Sure, something like:

static const struct iio_info nct720x_info = {
     .read_raw = nct720x_read_raw,
     .read_event_config = nct720x_read_event_config,
     .write_event_config = nct720x_write_event_config,
     .read_event_value = nct720x_read_event_value,
     .write_event_value = nct720x_write_event_value,  
};

static const struct iio_info nct720x_info_no_irq = {
     .read_raw = nct720x_read_raw,
};

if (irq)
	indio_dev->info = nct720x_info;
else
	indio_dev->info = nct720x_info_no_irq;

It isn't strictly necessary I think, but it is cleaner to not provide
callbacks that should not be called.
   }
> > > +
> > > +     guard(mutex)(&chip->access_lock);
> > > +     err  = regmap_read(chip->regmap, REG_CHANNEL_ENABLE_1, &value);
> > > +     if (err < 0)
> > > +             return err;
> > > +     data[0] = (u8)value;
> > > +
> > > +     err  = regmap_read(chip->regmap, REG_CHANNEL_ENABLE_2, &value);
> > > +     if (err < 0)
> > > +             return err;  
> >
> > Here I think you can use a bulk read as the registers are next to each other.
> >  
> 
> Generally, registers with 8 bits support Byte format, and registers
> with more than 8 bits support Word format.
> If transmission a Word command to a register that supports Byte
> format, the second byte will get 0xFF.
> Here, if we use regmap_bulk_read(), we would get first byte correct
> and second byte is wrong 0xff.
> I use i2ctransfer to demo it.
> root@evb-npcm845:~# i2ctransfer -f -y 5 w1@0x1d 0x13 r1
> 0xff
> root@evb-npcm845:~# i2ctransfer -f -y 5 w1@0x1d 0x14 r1
> 0x0f

As your regmap for these registers is 8 bit one the function here:
https://elixir.bootlin.com/linux/v6.12.4/source/drivers/base/regmap/regmap-i2c.c#L306
should have picked you a regmap bus config that only does 8 bit reads.

Thus when you use a regmap_bulk_read() it should issue two of those to neighbouring
registers, not use a word read.  So that should work in this specific case.


Thanks,

Jonathan