Re: [RESEND 10/14] iio: adc: mt6370: Add Mediatek MT6370 support

[Jonathan Cameron <jic23@kernel.org>]

> >  
> > > +
> > > +#define MT6370_AICR_400MA            0x6
> > > +#define MT6370_ICHG_500MA            0x4
> > > +#define MT6370_ICHG_900MA            0x8
> > > +
> > > +#define ADC_CONV_TIME_US             35000
> > > +#define ADC_CONV_POLLING_TIME                1000
> > > +
> > > +struct mt6370_adc_data {
> > > +     struct device *dev;
> > > +     struct regmap *regmap;
> > > +     struct mutex lock;  
> >
> > All locks need documentation.  What is the scope of the lock?
> > Looks like it protects device state when doing setup, wait for read, read
> > cycles.  
> 
> This mutex lock is for preventing the different adc channel from being
> read at the same time.
> So, if I just change its name to adc_chan_lock or adc_lock and add the
> comment for this mutex lock, does this change meet your requirement

Yes

> 
> >  
> > > +};
> > > +
> > > +static int mt6370_adc_read_scale(struct mt6370_adc_data *priv,
> > > +                              int chan, int *val1, int *val2)
> > > +{
> > > +     unsigned int reg_val;
> > > +     int ret;
> > > +
> > > +     switch (chan) {
> > > +     case MT6370_CHAN_VBAT:
> > > +     case MT6370_CHAN_VSYS:
> > > +     case MT6370_CHAN_CHG_VDDP:
> > > +             *val1 = 5000;  
> >
> > This seems very large.  Voltages are in millivolts
> > as per Documentation/ABI/testing/sysfs-bus-iio
> > and this means each step is 5 volts.
> >
> > So value in mv is currently 5 * _raw
> >  
> 
> OK, I got it. Also, I will add the ABI file in the next version. Thanks!
Only add ABI documentation for anything non-standard.

The documentation scripts really don't like repeats!

> 
> > > +static const char * const mt6370_channel_labels[MT6370_CHAN_MAX] = {  
> >
> > Perhaps define an enum with which to index this and the chan spec
> > and hence ensure they end up matching.
> >  [vbusdiv5] = "vbusdiv5", etc
> >  
> 
> Do you mean that I can refine this const char array to the following array??
> 
> static const char * const mt6370_channel_labels[MT6370_CHAN_MAX] = {
>     [MT6370_CHAN_VBUSDIV5] =  "vbusdiv5",
>     [MT6370_CHAN_VBUSDIV2] =  "vbusdiv2",
>     ...
>     ...
>     [MT6370_CHAN_TEMP_JC] =  "temp_jc",
> };

Yes

thanks,

Jonathan