| Message ID | 1601542448-7433-1-git-send-email-gene.chen.richtek@gmail.com |
|---|---|
| Headers | show |
| Series | iio: adc: mt6360: Add ADC driver for MT6360 | expand |
On Thu, 1 Oct 2020 16:54:05 +0800 Gene Chen <gene.chen.richtek@gmail.com> wrote: > This patch series add MT6360 ADC support contains driver, testing document > and binding document > > Gene Chen (2) > dt-bindings: iio: adc: add bindings doc for MT6360 ADC > Documentation: ABI: testing: mt6360: Add ADC sysfs guideline > iio: adc: mt6360: Add ADC driver for MT6360 Hi Gene This looks good to me. I'm just waiting now on some final reviews for the patch that adds the label attribute support. Once those are in I'll pick your driver up as well. If I seem to have lost it, feel free to poke me! Thanks, Jonathan > > Documentation/ABI/testing/sysfs-bus-iio-adc-mt6360 | 78 ++ > Documentation/devicetree/bindings/iio/adc/mediatek,mt6360-adc.yaml | 34 > drivers/iio/adc/Kconfig | 11 > drivers/iio/adc/Makefile | 1 > drivers/iio/adc/mt6360-adc.c | 362 ++++++++++ > 5 files changed, 486 insertions(+) > > changelogs between v1 & v2 > - adc: use IIO_CHAN_INFO_PROCESSED only > - adc: use devm_iio_triggered_buffer_setup > - adc: use use s64 to record timestamp > > changelogs between v2 & v3 > - Rearrange include file order by alphabet > - Set line length constraint below 100 > - Add Document for testing adc sysfs node guideline > - Set compiler 64 bit aligned when handle iio timestamp > > changelogs between v3 & v4 > - Fix sysfs guideline description > - Replace iio channel processed by raw/scale/offset > - Add comment of read adc flow for special HW design > > changelogs between v4 & v5 > - Rename dt-bindings aligned to file name > - Aligned sysfs node name with driver and add VBUSDIVX description > - Add ADC channel sysfs node "*_labels" > > changelogs between v5 & v6 > - Memset aligned adc data > - Remove strong casting void pointer >
On Wed, Sep 30, 2020 at 11:55 AM Gene Chen <gene.chen.richtek@gmail.com> wrote: > > From: Gene Chen <gene_chen@richtek.com> > > Add MT6360 ADC driver include Charger Current, Voltage, and including > Temperature. ... > +#define MT6360_AICR_MASK GENMASK(7, 2) bits.h is missed. ... > +#define MT6360_ADCEN_MASK 0x8000 BIT() ? ... > +static int mt6360_adc_read_channel(struct mt6360_adc_data *mad, int channel, int *val) > +{ > + 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); This looks like NIH of wait_interruptible() or so. ... > + while (true) { Oh, please avoid infinite loops. > + 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); > + } So, a new NIH of regmap_read_poll_timeout() ? ... > + /* rpt[1]: ADC_HI_BYTE, rpt[2]: ADC_LOW_BYTE */ > + *val = rpt[1] << 8 | rpt[2]; get_unalined_be16() and hence redundant comment. ... > +static int mt6360_adc_read_offset(struct mt6360_adc_data *mad, int channel, int *val) > +{ > + *val = (channel == MT6360_CHAN_TEMP_JC) ? -80 : 0; > + return IIO_VAL_INT; > + Misplaced blank line. > +} > +static const char *mt6360_channel_labels[MT6360_CHAN_MAX] = { > + "usbid", "vbusdiv5", "vbusdiv2", "vsys", "vbat", "ibus", "ibat", "chg_vddp", "temp_jc", > + "vref_ts", "ts" Leave comma. And split either by power of two or equally between the lines (seems like 8 + 3 here). > +}; ... > + /* Reset all channel off time to the current one */ > + all_off_time = ktime_get(); > + for (i = 0; i < MT6360_CHAN_MAX; i++) > + info->last_off_timestamps[i] = all_off_time; memset32() / memset64() / memset_l() (Not applicable, but JFYI: memset_p() also present)