by Jonathan Cameron

[permalink] [raw]

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

On Fri, 18 Sep 2020 16:04:43 +0800
Gene Chen <[email protected]> wrote:

> Jonathan Cameron <[email protected]> 於 2020年9月18日週五上午1:53寫道：
> >
> > On Wed, 16 Sep 2020 01:36:09 +0800
> > Gene Chen <[email protected]> wrote:
> >
> > > From: Gene Chen <[email protected]>
> > >
> > > Add MT6360 ADC driver include Charger Current, Voltage, and
> > > Temperature.
> > >
> > > Signed-off-by: Gene Chen <[email protected]>
> > 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;
...