Re: [PATCH v4 3/3] iio: adc: mt6360: Add ADC driver for MT6360

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 18 Sep 2020 16:04:43 +0800
Gene Chen <gene.chen.richtek@gmail.com> wrote:

> Jonathan Cameron <jic23@kernel.org> 於 2020年9月18日 週五 上午1:53寫道：
> >
> > On Wed, 16 Sep 2020 01:36:09 +0800
> > Gene Chen <gene.chen.richtek@gmail.com> wrote:
> >  
> > > From: Gene Chen <gene_chen@richtek.com>
> > >
> > > Add MT6360 ADC driver include Charger Current, Voltage, and
> > > Temperature.
> > >
> > > Signed-off-by: Gene Chen <gene_chen@richtek.com>  
> > Comments inline.

...

> > > +     if (ret)
> > > +             goto out_adc_lock;
> > > +
> > > +     start_t = ktime_get();
> > > +     predict_end_t = ktime_add_ms(mad->last_off_timestamps[channel], 2 * ADC_WAIT_TIME_MS);
> > > +
> > > +     if (ktime_after(start_t, predict_end_t))
> > > +             pre_wait_time = ADC_WAIT_TIME_MS;
> > > +     else
> > > +             pre_wait_time = 3 * ADC_WAIT_TIME_MS;
> > > +
> > > +     msleep(pre_wait_time);
> > > +
> > > +     while (true) {
> > > +             ret = regmap_raw_read(mad->regmap, MT6360_REG_PMUADCRPT1, rpt, sizeof(rpt));
> > > +             if (ret)
> > > +                     goto out_adc_conv;
> > > +
> > > +             /*
> > > +              * There are two functions, ZCV and TypeC OTP, running ADC VBAT and TS in
> > > +              * background, and ADC samples are taken on a fixed frequency no matter read the
> > > +              * previous one or not.
> > > +              * To avoid conflict, We set minimum time threshold after enable ADC and
> > > +              * check report channel is the same.
> > > +              * The worst case is run the same ADC twice and background function is also running,
> > > +              * ADC conversion sequence is desire channel before start ADC, background ADC,
> > > +              * desire channel after start ADC.
> > > +              * So the minimum correct data is three times of typical conversion time.
> > > +              */
> > > +             if ((rpt[0] & MT6360_RPTCH_MASK) == channel)
> > > +                     break;
> > > +
> > > +             msleep(ADC_WAIT_TIME_MS);
> > > +     }
> > > +
> > > +     /* rpt[1]: ADC_HI_BYTE, rpt[2]: ADC_LOW_BYTE */
> > > +     *val = be16_to_cpup((__be16 *)(rpt + 1));  
> >
> > To be entirely safe, probably need that to be an unaligned read?
> >  
> 
> Maybe I can change to "*val = rpt[1] << 8 | rpt[2];".
> It's more abvious.

That would definitely be safe so do that.

> 
> > > +     ret = IIO_VAL_INT;
> > > +
> > > +out_adc_conv:
> > > +     /* Only keep ADC enable */
> > > +     adc_enable = cpu_to_be16(MT6360_ADCEN_MASK);
> > > +     regmap_raw_write(mad->regmap, MT6360_REG_PMUADCCFG, (void *)&adc_enable, sizeof(__be16));  
> >
> > sizeof(adc_enable)
> >  
> 
> ACK
> 
> > > +     mad->last_off_timestamps[channel] = ktime_get();
> > > +     /* Config prefer channel to NO_PREFER */
> > > +     regmap_update_bits(mad->regmap, MT6360_REG_PMUADCRPT1, MT6360_PREFERCH_MASK,
> > > +                        MT6360_NO_PREFER << MT6360_PREFERCH_SHFT);
> > > +out_adc_lock:
> > > +     mutex_unlock(&mad->adc_lock);
> > > +
> > > +     return ret;
> > > +}
> > > +
> > > +static int mt6360_adc_read_scale(struct mt6360_adc_data *mad, int channel, int *val, int *val2)
> > > +{
> > > +     unsigned int regval;
> > > +     int ret;
> > > +
> > > +     switch (channel) {
> > > +     case MT6360_CHAN_USBID:
> > > +             fallthrough;  
> >
> > I don't think we need fallthrough for cases
> > with nothing in them.
> >  
> 
> Every channel needs set " *val = 2500" for scale.
> Do you mean change as below?
> 
>         switch (channel) {
>         case MT6360_CHAN_USBID:
>         case MT6360_CHAN_VSYS:
>         case MT6360_CHAN_VBAT:
>         case MT6360_CHAN_CHG_VDDP:
>         case MT6360_CHAN_VREF_TS:
>                 fallthrough;
Don't need this fallthrough.

fallthrough is only needed if something is done in the
case statement.  I believe the checker is clever enough to
assume that a set of case statements with nothing inbetween
them are deliberate.

>         case MT6360_CHAN_TS:
>                 *val = 1250;
>                 return IIO_VAL_INT;
> 

...

> > > +
> > > +static irqreturn_t mt6360_adc_trigger_handler(int irq, void *p)
> > > +{
> > > +     struct iio_poll_func *pf = p;
> > > +     struct iio_dev *indio_dev = pf->indio_dev;
> > > +     struct mt6360_adc_data *mad = iio_priv(indio_dev);
> > > +     struct {
> > > +             u16 values[MT6360_CHAN_MAX];  
> >
> > There is a hole in here I think? (MT6360_CHAN_MAX is 11?)
> > If so we need to explicitly memset the structure to avoid any
> > risk of kernel data leakage to userspace.

Make sure you deal with this in v5!

> >  
> > > +             int64_t timestamp;
> > > +     } data;  
> >
> > I guess we know this is on a platform with 64bit alignment for int64_t's
> > (unlike x86_64 for example)
> >  
> 
> Do you mean change as below?
> struct {
>     u16 values[MT6360_CHAN_MAX];
>     int64_t timestamp; __aligned(8)
> } data;

You can do that, or we can rely on the fact this part is never used
on a platform where that isn't guaranteed anyway.

> 
> > > +     int i = 0, bit, val, ret;
...