On Fri, 18 Sep 2020 16:04:43 +0800
Gene Chen [off-list ref] wrote:

Jonathan Cameron [off-list ref] 於 2020年9月18日 週五 上午1:53寫道：

quoted

On Wed, 16 Sep 2020 01:36:09 +0800
Gene Chen [off-list ref] wrote:
 

quoted

From: Gene Chen <redacted>

Add MT6360 ADC driver include Charger Current, Voltage, and
Temperature.

Signed-off-by: Gene Chen <redacted>  

Comments inline.

...

quoted

quoted

+     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.

quoted

quoted

+     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

quoted

quoted

+     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;

...

quoted

quoted

+
+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!

quoted

quoted

+             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.

quoted

quoted

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

...


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel