* [PATCH v3 0/2] iio: adc: mt6360: Add ADC driver for MT6360
@ 2020-08-24 9:06 Gene Chen
2020-08-24 9:06 ` [PATCH v3 1/2] " Gene Chen
2020-08-24 9:06 ` [PATCH v3 2/2] Documentation: ABI: testing: mt6360: Add ADC sysfs guideline Gene Chen
0 siblings, 2 replies; 15+ messages in thread
From: Gene Chen @ 2020-08-24 9:06 UTC (permalink / raw)
To: jic23, matthias.bgg
Cc: knaack.h, lars, pmeerw, linux-iio, linux-arm-kernel,
linux-mediatek, linux-kernel, gene_chen, Wilma.Wu, shufan_lee,
cy_huang, benjamin.chao
In-Reply-To:
This patch series add MT6360 ADC support contains driver and testing document
Gene Chen (2)
iio: adc: mt6360: Add ADC driver for MT6360
Documentation: ABI: testing: mt6360: Add ADC sysfs guideline
Documentation/ABI/testing/sysfs-bus-iio-adc-mt6360 | 83 ++++
drivers/iio/adc/Kconfig | 11
drivers/iio/adc/Makefile | 1
drivers/iio/adc/mt6360-adc.c | 359 +++++++++++++++++++++
4 files changed, 454 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 1/2] iio: adc: mt6360: Add ADC driver for MT6360
2020-08-24 9:06 [PATCH v3 0/2] iio: adc: mt6360: Add ADC driver for MT6360 Gene Chen
@ 2020-08-24 9:06 ` Gene Chen
2020-08-27 12:16 ` kernel test robot
` (2 more replies)
2020-08-24 9:06 ` [PATCH v3 2/2] Documentation: ABI: testing: mt6360: Add ADC sysfs guideline Gene Chen
1 sibling, 3 replies; 15+ messages in thread
From: Gene Chen @ 2020-08-24 9:06 UTC (permalink / raw)
To: jic23, matthias.bgg
Cc: knaack.h, lars, pmeerw, linux-iio, linux-arm-kernel,
linux-mediatek, linux-kernel, gene_chen, Wilma.Wu, shufan_lee,
cy_huang, benjamin.chao
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>
---
drivers/iio/adc/Kconfig | 11 ++
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/mt6360-adc.c | 366 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 378 insertions(+)
create mode 100644 drivers/iio/adc/mt6360-adc.c
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 66d9cc0..07dcea7 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -703,6 +703,17 @@ config MCP3911
This driver can also be built as a module. If so, the module will be
called mcp3911.
+config MEDIATEK_MT6360_ADC
+ tristate "Mediatek MT6360 ADC Part"
+ depends on MFD_MT6360
+ select IIO_BUFFER
+ select IIO_TRIGGERED_BUFFER
+ help
+ Say Y here to enable MT6360 ADC Part.
+ Integrated for System Monitoring include
+ Charger and Battery Current, Voltage and
+ Temperature
+
config MEDIATEK_MT6577_AUXADC
tristate "MediaTek AUXADC driver"
depends on ARCH_MEDIATEK || COMPILE_TEST
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 90f94ad..5fca90a 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -65,6 +65,7 @@ obj-$(CONFIG_MAX9611) += max9611.o
obj-$(CONFIG_MCP320X) += mcp320x.o
obj-$(CONFIG_MCP3422) += mcp3422.o
obj-$(CONFIG_MCP3911) += mcp3911.o
+obj-$(CONFIG_MEDIATEK_MT6360_ADC) += mt6360-adc.o
obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o
obj-$(CONFIG_MEN_Z188_ADC) += men_z188_adc.o
obj-$(CONFIG_MESON_SARADC) += meson_saradc.o
diff --git a/drivers/iio/adc/mt6360-adc.c b/drivers/iio/adc/mt6360-adc.c
new file mode 100644
index 0000000..5eed812
--- /dev/null
+++ b/drivers/iio/adc/mt6360-adc.c
@@ -0,0 +1,366 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2020 MediaTek Inc.
+ *
+ * Author: Gene Chen <gene_chen@richtek.com>
+ */
+
+#include <linux/completion.h>
+#include <linux/interrupt.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/irq.h>
+#include <linux/kernel.h>
+#include <linux/ktime.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define MT6360_REG_PMUCHGCTRL3 0x313
+#define MT6360_REG_PMUADCCFG 0x356
+#define MT6360_REG_PMUADCRPT1 0x35A
+
+/* PMUCHGCTRL3 0x313 */
+#define MT6360_AICR_MASK 0xFC
+#define MT6360_AICR_SHFT 2
+#define MT6360_AICR_400MA 0x6
+/* PMUADCCFG 0x356 */
+#define MT6360_ADCEN_MASK 0x8000
+/* PMUADCRPT1 0x35A */
+#define MT6360_PREFERCH_MASK 0xF0
+#define MT6360_PREFERCH_SHFT 4
+#define MT6360_RPTCH_MASK 0x0F
+
+enum {
+ MT6360_CHAN_USBID = 0,
+ MT6360_CHAN_VBUSDIV5,
+ MT6360_CHAN_VBUSDIV2,
+ MT6360_CHAN_VSYS,
+ MT6360_CHAN_VBAT,
+ MT6360_CHAN_IBUS,
+ MT6360_CHAN_IBAT,
+ MT6360_CHAN_CHG_VDDP,
+ MT6360_CHAN_TEMP_JC,
+ MT6360_CHAN_VREF_TS,
+ MT6360_CHAN_TS,
+ MT6360_CHAN_MAX,
+};
+
+struct mt6360_adc_data {
+ struct device *dev;
+ struct regmap *regmap;
+ struct completion adc_complete;
+ struct mutex adc_lock;
+ ktime_t last_off_timestamps[MT6360_CHAN_MAX];
+ int irq;
+};
+
+static inline int mt6360_adc_val_converter(int val, int multiplier, int offset, int divisor)
+{
+ return ((val * multiplier) + offset) / divisor;
+}
+
+static int mt6360_adc_convert_processed_val(struct mt6360_adc_data *info, int chan_idx, int *val)
+{
+ unsigned int regval = 0;
+ const struct converter {
+ int multiplier;
+ int offset;
+ int divisor;
+ } adc_converter[MT6360_CHAN_MAX] = {
+ { 1250, 0, 1}, /* USBID */
+ { 6250, 0, 1}, /* VBUSDIV5 */
+ { 2500, 0, 1}, /* VBUSDIV2 */
+ { 1250, 0, 1}, /* VSYS */
+ { 1250, 0, 1}, /* VBAT */
+ { 2500, 0, 1}, /* IBUS */
+ { 2500, 0, 1}, /* IBAT */
+ { 1250, 0, 1}, /* CHG_VDDP */
+ { 105, -8000, 100}, /* TEMP_JC */
+ { 1250, 0, 1}, /* VREF_TS */
+ { 1250, 0, 1}, /* TS */
+ }, sp_ibus_adc_converter = { 1900, 0, 1 }, *sel_converter;
+ int ret;
+
+ sel_converter = adc_converter + chan_idx;
+ if (chan_idx == MT6360_CHAN_IBUS) {
+ /* ibus chan will be affected by aicr config */
+ /* if aicr < 400, apply the special ibus converter */
+ ret = regmap_read(info->regmap, MT6360_REG_PMUCHGCTRL3, ®val);
+ if (ret)
+ return ret;
+
+ regval = (regval & MT6360_AICR_MASK) >> MT6360_AICR_SHFT;
+ if (regval < MT6360_AICR_400MA)
+ sel_converter = &sp_ibus_adc_converter;
+ }
+
+ *val = mt6360_adc_val_converter(*val, sel_converter->multiplier, sel_converter->offset,
+ sel_converter->divisor);
+
+ return 0;
+}
+
+static int mt6360_adc_read_processed(struct mt6360_adc_data *mad, int channel, int *val)
+{
+ u16 adc_enable;
+ u8 rpt[3];
+ ktime_t start_t, predict_end_t;
+ long timeout;
+ int value, ret;
+
+ mutex_lock(&mad->adc_lock);
+
+ /* select preferred channel that we want */
+ ret = regmap_update_bits(mad->regmap, MT6360_REG_PMUADCRPT1, MT6360_PREFERCH_MASK,
+ channel << MT6360_PREFERCH_SHFT);
+ if (ret)
+ goto out_adc;
+
+ /* enable adc channel we want and adc_en */
+ adc_enable = MT6360_ADCEN_MASK | BIT(channel);
+ adc_enable = cpu_to_be16(adc_enable);
+ ret = regmap_raw_write(mad->regmap, MT6360_REG_PMUADCCFG, (void *)&adc_enable, sizeof(u16));
+ if (ret)
+ goto out_adc;
+
+ start_t = ktime_get();
+ predict_end_t = ktime_add_ms(mad->last_off_timestamps[channel], 50);
+
+ if (ktime_after(start_t, predict_end_t))
+ predict_end_t = ktime_add_ms(start_t, 25);
+ else
+ predict_end_t = ktime_add_ms(start_t, 75);
+
+ enable_irq(mad->irq);
+adc_retry:
+ reinit_completion(&mad->adc_complete);
+
+ /* wait for conversion to complete */
+ timeout = wait_for_completion_timeout(&mad->adc_complete, msecs_to_jiffies(200));
+ if (timeout == 0) {
+ ret = -ETIMEDOUT;
+ goto out_adc_conv;
+ } else if (timeout < 0) {
+ ret = -EINTR;
+ goto out_adc_conv;
+ }
+
+ ret = regmap_raw_read(mad->regmap, MT6360_REG_PMUADCRPT1, rpt, sizeof(rpt));
+ if (ret)
+ goto out_adc_conv;
+
+ /* check the current reported channel */
+ if ((rpt[0] & MT6360_RPTCH_MASK) != channel) {
+ dev_dbg(mad->dev, "not wanted channel report [%02x]\n", rpt[0]);
+ goto adc_retry;
+ }
+
+ if (!ktime_after(ktime_get(), predict_end_t)) {
+ dev_dbg(mad->dev, "time is not after one adc_conv_t\n");
+ goto adc_retry;
+ }
+
+ value = (rpt[1] << 8) | rpt[2];
+
+ ret = mt6360_adc_convert_processed_val(mad, channel, &value);
+ if (ret)
+ goto out_adc_conv;
+
+ *val = value;
+ ret = IIO_VAL_INT;
+
+out_adc_conv:
+ disable_irq(mad->irq);
+ adc_enable = MT6360_ADCEN_MASK;
+ adc_enable = cpu_to_be16(adc_enable);
+ regmap_raw_write(mad->regmap, MT6360_REG_PMUADCCFG, (void *)&adc_enable, sizeof(u16));
+ mad->last_off_timestamps[channel] = ktime_get();
+ /* set prefer channel to 0xf */
+ regmap_update_bits(mad->regmap, MT6360_REG_PMUADCRPT1, MT6360_PREFERCH_MASK,
+ 0xF << MT6360_PREFERCH_SHFT);
+out_adc:
+ mutex_unlock(&mad->adc_lock);
+
+ return ret;
+}
+
+static int mt6360_adc_read_raw(struct iio_dev *iio_dev, const struct iio_chan_spec *chan,
+ int *val, int *val2, long mask)
+{
+ struct mt6360_adc_data *mad = iio_priv(iio_dev);
+
+ if (mask == IIO_CHAN_INFO_PROCESSED)
+ return mt6360_adc_read_processed(mad, chan->channel, val);
+
+ return -EINVAL;
+}
+
+static const struct iio_info mt6360_adc_iio_info = {
+ .read_raw = mt6360_adc_read_raw,
+};
+
+#define MT6360_ADC_CHAN(_idx, _type) { \
+ .type = _type, \
+ .channel = MT6360_CHAN_##_idx, \
+ .scan_index = MT6360_CHAN_##_idx, \
+ .extend_name = #_idx, \
+ .datasheet_name = #_idx, \
+ .scan_type = { \
+ .sign = 's', \
+ .realbits = 32, \
+ .storagebits = 32, \
+ .endianness = IIO_CPU, \
+ }, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
+ .indexed = 1, \
+}
+
+static const struct iio_chan_spec mt6360_adc_channels[] = {
+ MT6360_ADC_CHAN(USBID, IIO_VOLTAGE),
+ MT6360_ADC_CHAN(VBUSDIV5, IIO_VOLTAGE),
+ MT6360_ADC_CHAN(VBUSDIV2, IIO_VOLTAGE),
+ MT6360_ADC_CHAN(VSYS, IIO_VOLTAGE),
+ MT6360_ADC_CHAN(VBAT, IIO_VOLTAGE),
+ MT6360_ADC_CHAN(IBUS, IIO_CURRENT),
+ MT6360_ADC_CHAN(IBAT, IIO_CURRENT),
+ MT6360_ADC_CHAN(CHG_VDDP, IIO_VOLTAGE),
+ MT6360_ADC_CHAN(TEMP_JC, IIO_TEMP),
+ MT6360_ADC_CHAN(VREF_TS, IIO_VOLTAGE),
+ MT6360_ADC_CHAN(TS, IIO_VOLTAGE),
+ IIO_CHAN_SOFT_TIMESTAMP(MT6360_CHAN_MAX),
+};
+
+static irqreturn_t mt6360_pmu_adc_donei_handler(int irq, void *data)
+{
+ struct mt6360_adc_data *mad = iio_priv(data);
+
+ complete(&mad->adc_complete);
+ return IRQ_HANDLED;
+}
+
+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;
+ /* 11 ch s32 numbers + 1 s64 timestamp */
+ s32 data[MT6360_CHAN_MAX + 2] __aligned(8);
+ int i = 0, bit, val, ret;
+
+ for_each_set_bit(bit, indio_dev->active_scan_mask, indio_dev->masklength) {
+ const struct iio_chan_spec *chan = indio_dev->channels + bit;
+
+ ret = mt6360_adc_read_raw(indio_dev, chan, &val, NULL, IIO_CHAN_INFO_PROCESSED);
+ if (ret != IIO_VAL_INT) {
+ dev_warn(&indio_dev->dev, "Failed to get %d conversion val\n", bit);
+ goto out;
+ }
+
+ data[i++] = val;
+ }
+ iio_push_to_buffers_with_timestamp(indio_dev, data, iio_get_time_ns(indio_dev));
+out:
+ iio_trigger_notify_done(indio_dev->trig);
+
+ return IRQ_HANDLED;
+}
+
+static inline int mt6360_adc_reset(struct mt6360_adc_data *info)
+{
+ u8 configs[3] = {0x80, 0, 0};
+ ktime_t all_off_time;
+ int i;
+
+ all_off_time = ktime_get();
+ for (i = 0; i < MT6360_CHAN_MAX; i++)
+ info->last_off_timestamps[i] = all_off_time;
+
+ /* enable adc_en, clear adc_chn_en/zcv_en/adc_wait_t/adc_idle_t */
+ return regmap_raw_write(info->regmap, MT6360_REG_PMUADCCFG, configs, sizeof(configs));
+}
+
+static int mt6360_adc_probe(struct platform_device *pdev)
+{
+ struct mt6360_adc_data *mad;
+ struct iio_dev *indio_dev;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*mad));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ mad = iio_priv(indio_dev);
+ mad->dev = &pdev->dev;
+ init_completion(&mad->adc_complete);
+ mutex_init(&mad->adc_lock);
+
+ mad->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+ if (!mad->regmap) {
+ dev_err(&pdev->dev, "Failed to get parent regmap\n");
+ return -ENODEV;
+ }
+
+ ret = mt6360_adc_reset(mad);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "Failed to reset adc\n");
+ return ret;
+ }
+
+ mad->irq = platform_get_irq_byname(pdev, "adc_donei");
+ if (mad->irq < 0) {
+ dev_err(&pdev->dev, "Failed to get adc_done irq\n");
+ return mad->irq;
+ }
+
+ irq_set_status_flags(mad->irq, IRQ_NOAUTOEN);
+ ret = devm_request_threaded_irq(&pdev->dev, mad->irq, NULL, mt6360_pmu_adc_donei_handler, 0,
+ "adc_donei", indio_dev);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed to register adc_done irq\n");
+ return ret;
+ }
+
+ indio_dev->name = dev_name(&pdev->dev);
+ indio_dev->dev.parent = &pdev->dev;
+ indio_dev->info = &mt6360_adc_iio_info;
+ indio_dev->modes = INDIO_DIRECT_MODE;
+ indio_dev->channels = mt6360_adc_channels;
+ indio_dev->num_channels = ARRAY_SIZE(mt6360_adc_channels);
+
+ ret = devm_iio_triggered_buffer_setup(&pdev->dev, indio_dev, NULL,
+ mt6360_adc_trigger_handler, NULL);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed to allocate iio trigger buffer\n");
+ return ret;
+ }
+
+ ret = devm_iio_device_register(&pdev->dev, indio_dev);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed to register iio device\n");
+ return ret;
+ }
+
+ return 0;
+}
+
+static const struct of_device_id __maybe_unused mt6360_adc_of_id[] = {
+ { .compatible = "mediatek,mt6360-adc", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, mt6360_adc_of_id);
+
+static struct platform_driver mt6360_adc_driver = {
+ .driver = {
+ .name = "mt6360-adc",
+ .of_match_table = mt6360_adc_of_id,
+ },
+ .probe = mt6360_adc_probe,
+};
+module_platform_driver(mt6360_adc_driver);
+
+MODULE_AUTHOR("Gene Chen <gene_chen@richtek.com>");
+MODULE_DESCRIPTION("MT6360 ADC Driver");
+MODULE_LICENSE("GPL v2");
--
2.7.4
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v3 2/2] Documentation: ABI: testing: mt6360: Add ADC sysfs guideline
2020-08-24 9:06 [PATCH v3 0/2] iio: adc: mt6360: Add ADC driver for MT6360 Gene Chen
2020-08-24 9:06 ` [PATCH v3 1/2] " Gene Chen
@ 2020-08-24 9:06 ` Gene Chen
2020-08-29 17:15 ` Jonathan Cameron
1 sibling, 1 reply; 15+ messages in thread
From: Gene Chen @ 2020-08-24 9:06 UTC (permalink / raw)
To: jic23, matthias.bgg
Cc: knaack.h, lars, pmeerw, linux-iio, linux-arm-kernel,
linux-mediatek, linux-kernel, gene_chen, Wilma.Wu, shufan_lee,
cy_huang, benjamin.chao
From: Gene Chen <gene_chen@richtek.com>
Add ABI documentation for mt6360 ADC sysfs interfaces.
Signed-off-by: Gene Chen <gene_chen@richtek.com>
---
Documentation/ABI/testing/sysfs-bus-iio-adc-mt6360 | 83 ++++++++++++++++++++++
1 file changed, 83 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-mt6360
diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-mt6360 b/Documentation/ABI/testing/sysfs-bus-iio-adc-mt6360
new file mode 100644
index 0000000..9dab17e
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-mt6360
@@ -0,0 +1,83 @@
+What: /sys/bus/iio/devices/iio:deviceX/usbid_input
+KernelVersion: 5.8.0
+Contact: gene_chen@richtek.com
+Description:
+ Indicated MT6360 USBID ADC
+ Reading returns current voltage in uV
+
+What: /sys/bus/iio/devices/iio:deviceX/vbusdiv5_input
+KernelVersion: 5.8.0
+Contact: gene_chen@richtek.com
+Description:
+ Indicated MT6360 VBUS ADC with high accuracy
+ Reading returns current voltage in uV
+
+What: /sys/bus/iio/devices/iio:deviceX/vbusdiv2_input
+KernelVersion: 5.8.0
+Contact: gene_chen@richtek.com
+Description:
+ Indicated MT6360 VBUS ADC with low accuracy
+ Reading returns current voltage in uV
+
+What: /sys/bus/iio/devices/iio:deviceX/vsys_input
+KernelVersion: 5.8.0
+Contact: gene_chen@richtek.com
+Description:
+ Indicated MT6360 VSYS ADC
+ Reading returns current voltage in uV
+
+What: /sys/bus/iio/devices/iio:deviceX/vbat_input
+KernelVersion: 5.8.0
+Contact: gene_chen@richtek.com
+Description:
+ Indicated MT6360 VBAT ADC
+ Reading returns current voltage in uV
+
+What: /sys/bus/iio/devices/iio:deviceX/ibus_input
+KernelVersion: 5.8.0
+Contact: gene_chen@richtek.com
+Description:
+ Indicated MT6360 IBUS ADC
+ Reading returns current in uA
+
+What: /sys/bus/iio/devices/iio:deviceX/ibat_input
+KernelVersion: 5.8.0
+Contact: gene_chen@richtek.com
+Description:
+ Indicated MT6360 IBAT ADC
+ Reading returns current in uA
+
+What: /sys/bus/iio/devices/iio:deviceX/chg_vddp_input
+KernelVersion: 5.8.0
+Contact: gene_chen@richtek.com
+Description:
+ Indicated MT6360 CHG_VDDP ADC
+ Reading returns current voltage in uV
+
+What: /sys/bus/iio/devices/iio:deviceX/temp_jc_input
+KernelVersion: 5.8.0
+Contact: gene_chen@richtek.com
+Description:
+ Indicated MT6360 IC junction temperature
+ Reading returns current temperature in degree
+
+What: /sys/bus/iio/devices/iio:deviceX/vref_ts_input
+KernelVersion: 5.8.0
+Contact: gene_chen@richtek.com
+Description:
+ Indicated MT6360 VREF_TS ADC
+ Reading returns current voltage in uV
+
+What: /sys/bus/iio/devices/iio:deviceX/ts_input
+KernelVersion: 5.8.0
+Contact: gene_chen@richtek.com
+Description:
+ Indicated MT6360 TS ADC
+ Reading returns current voltage in uV
+
+What: /sys/bus/iio/devices/iio:deviceX/timestamp
+KernelVersion: 5.8.0
+Contact: gene_chen@richtek.com
+Description:
+ Indicated MT6360 timestamp
+ Reading returns current timestamp in ms
--
2.7.4
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v3 1/2] iio: adc: mt6360: Add ADC driver for MT6360
2020-08-24 9:06 ` [PATCH v3 1/2] " Gene Chen
@ 2020-08-27 12:16 ` kernel test robot
2020-08-29 17:11 ` Jonathan Cameron
2020-08-30 17:39 ` Andy Shevchenko
2 siblings, 0 replies; 15+ messages in thread
From: kernel test robot @ 2020-08-27 12:16 UTC (permalink / raw)
To: Gene Chen, jic23, matthias.bgg
Cc: kbuild-all, knaack.h, lars, pmeerw, linux-iio, linux-arm-kernel,
linux-mediatek, linux-kernel, gene_chen
[-- Attachment #1: Type: text/plain, Size: 5178 bytes --]
Hi Gene,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on iio/togreg]
[also build test WARNING on v5.9-rc2 next-20200827]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Gene-Chen/iio-adc-mt6360-Add-ADC-driver-for-MT6360/20200824-170948
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: i386-randconfig-s031-20200827 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.2-191-g10164920-dirty
# save the attached .config to linux build tree
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
>> drivers/iio/adc/mt6360-adc.c:125:20: sparse: sparse: incorrect type in assignment (different base types) @@ expected unsigned short [assigned] [usertype] adc_enable @@ got restricted __be16 [usertype] @@
>> drivers/iio/adc/mt6360-adc.c:125:20: sparse: expected unsigned short [assigned] [usertype] adc_enable
>> drivers/iio/adc/mt6360-adc.c:125:20: sparse: got restricted __be16 [usertype]
>> drivers/iio/adc/mt6360-adc.c:179:20: sparse: sparse: incorrect type in assignment (different base types) @@ expected unsigned short [addressable] [assigned] [usertype] adc_enable @@ got restricted __be16 [usertype] @@
>> drivers/iio/adc/mt6360-adc.c:179:20: sparse: expected unsigned short [addressable] [assigned] [usertype] adc_enable
drivers/iio/adc/mt6360-adc.c:179:20: sparse: got restricted __be16 [usertype]
# https://github.com/0day-ci/linux/commit/0e757f71bbdf50d4dad3b79e5f30c7f2386ae082
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Gene-Chen/iio-adc-mt6360-Add-ADC-driver-for-MT6360/20200824-170948
git checkout 0e757f71bbdf50d4dad3b79e5f30c7f2386ae082
vim +125 drivers/iio/adc/mt6360-adc.c
106
107 static int mt6360_adc_read_processed(struct mt6360_adc_data *mad, int channel, int *val)
108 {
109 u16 adc_enable;
110 u8 rpt[3];
111 ktime_t start_t, predict_end_t;
112 long timeout;
113 int value, ret;
114
115 mutex_lock(&mad->adc_lock);
116
117 /* select preferred channel that we want */
118 ret = regmap_update_bits(mad->regmap, MT6360_REG_PMUADCRPT1, MT6360_PREFERCH_MASK,
119 channel << MT6360_PREFERCH_SHFT);
120 if (ret)
121 goto out_adc;
122
123 /* enable adc channel we want and adc_en */
124 adc_enable = MT6360_ADCEN_MASK | BIT(channel);
> 125 adc_enable = cpu_to_be16(adc_enable);
126 ret = regmap_raw_write(mad->regmap, MT6360_REG_PMUADCCFG, (void *)&adc_enable, sizeof(u16));
127 if (ret)
128 goto out_adc;
129
130 start_t = ktime_get();
131 predict_end_t = ktime_add_ms(mad->last_off_timestamps[channel], 50);
132
133 if (ktime_after(start_t, predict_end_t))
134 predict_end_t = ktime_add_ms(start_t, 25);
135 else
136 predict_end_t = ktime_add_ms(start_t, 75);
137
138 enable_irq(mad->irq);
139 adc_retry:
140 reinit_completion(&mad->adc_complete);
141
142 /* wait for conversion to complete */
143 timeout = wait_for_completion_timeout(&mad->adc_complete, msecs_to_jiffies(200));
144 if (timeout == 0) {
145 ret = -ETIMEDOUT;
146 goto out_adc_conv;
147 } else if (timeout < 0) {
148 ret = -EINTR;
149 goto out_adc_conv;
150 }
151
152 ret = regmap_raw_read(mad->regmap, MT6360_REG_PMUADCRPT1, rpt, sizeof(rpt));
153 if (ret)
154 goto out_adc_conv;
155
156 /* check the current reported channel */
157 if ((rpt[0] & MT6360_RPTCH_MASK) != channel) {
158 dev_dbg(mad->dev, "not wanted channel report [%02x]\n", rpt[0]);
159 goto adc_retry;
160 }
161
162 if (!ktime_after(ktime_get(), predict_end_t)) {
163 dev_dbg(mad->dev, "time is not after one adc_conv_t\n");
164 goto adc_retry;
165 }
166
167 value = (rpt[1] << 8) | rpt[2];
168
169 ret = mt6360_adc_convert_processed_val(mad, channel, &value);
170 if (ret)
171 goto out_adc_conv;
172
173 *val = value;
174 ret = IIO_VAL_INT;
175
176 out_adc_conv:
177 disable_irq(mad->irq);
178 adc_enable = MT6360_ADCEN_MASK;
> 179 adc_enable = cpu_to_be16(adc_enable);
180 regmap_raw_write(mad->regmap, MT6360_REG_PMUADCCFG, (void *)&adc_enable, sizeof(u16));
181 mad->last_off_timestamps[channel] = ktime_get();
182 /* set prefer channel to 0xf */
183 regmap_update_bits(mad->regmap, MT6360_REG_PMUADCRPT1, MT6360_PREFERCH_MASK,
184 0xF << MT6360_PREFERCH_SHFT);
185 out_adc:
186 mutex_unlock(&mad->adc_lock);
187
188 return ret;
189 }
190
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 34835 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 1/2] iio: adc: mt6360: Add ADC driver for MT6360
2020-08-24 9:06 ` [PATCH v3 1/2] " Gene Chen
2020-08-27 12:16 ` kernel test robot
@ 2020-08-29 17:11 ` Jonathan Cameron
2020-09-08 6:17 ` Gene Chen
2020-08-30 17:39 ` Andy Shevchenko
2 siblings, 1 reply; 15+ messages in thread
From: Jonathan Cameron @ 2020-08-29 17:11 UTC (permalink / raw)
To: Gene Chen
Cc: matthias.bgg, knaack.h, lars, pmeerw, linux-iio,
linux-arm-kernel, linux-mediatek, linux-kernel, gene_chen,
Wilma.Wu, shufan_lee, cy_huang, benjamin.chao
On Mon, 24 Aug 2020 17:06:24 +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>
Hi Gene,
A few comments inline. The big one centres on why we can't
expose the channels as _raw, _offset and _scale?
Thanks,
Jonathan
> ---
> drivers/iio/adc/Kconfig | 11 ++
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/mt6360-adc.c | 366 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 378 insertions(+)
> create mode 100644 drivers/iio/adc/mt6360-adc.c
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 66d9cc0..07dcea7 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -703,6 +703,17 @@ config MCP3911
> This driver can also be built as a module. If so, the module will be
> called mcp3911.
>
> +config MEDIATEK_MT6360_ADC
> + tristate "Mediatek MT6360 ADC Part"
> + depends on MFD_MT6360
> + select IIO_BUFFER
> + select IIO_TRIGGERED_BUFFER
> + help
> + Say Y here to enable MT6360 ADC Part.
> + Integrated for System Monitoring include
> + Charger and Battery Current, Voltage and
> + Temperature
> +
> config MEDIATEK_MT6577_AUXADC
> tristate "MediaTek AUXADC driver"
> depends on ARCH_MEDIATEK || COMPILE_TEST
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 90f94ad..5fca90a 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -65,6 +65,7 @@ obj-$(CONFIG_MAX9611) += max9611.o
> obj-$(CONFIG_MCP320X) += mcp320x.o
> obj-$(CONFIG_MCP3422) += mcp3422.o
> obj-$(CONFIG_MCP3911) += mcp3911.o
> +obj-$(CONFIG_MEDIATEK_MT6360_ADC) += mt6360-adc.o
> obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o
> obj-$(CONFIG_MEN_Z188_ADC) += men_z188_adc.o
> obj-$(CONFIG_MESON_SARADC) += meson_saradc.o
> diff --git a/drivers/iio/adc/mt6360-adc.c b/drivers/iio/adc/mt6360-adc.c
> new file mode 100644
> index 0000000..5eed812
> --- /dev/null
> +++ b/drivers/iio/adc/mt6360-adc.c
> @@ -0,0 +1,366 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2020 MediaTek Inc.
> + *
> + * Author: Gene Chen <gene_chen@richtek.com>
> + */
> +
> +#include <linux/completion.h>
> +#include <linux/interrupt.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/irq.h>
> +#include <linux/kernel.h>
> +#include <linux/ktime.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#define MT6360_REG_PMUCHGCTRL3 0x313
> +#define MT6360_REG_PMUADCCFG 0x356
> +#define MT6360_REG_PMUADCRPT1 0x35A
> +
> +/* PMUCHGCTRL3 0x313 */
> +#define MT6360_AICR_MASK 0xFC
> +#define MT6360_AICR_SHFT 2
> +#define MT6360_AICR_400MA 0x6
> +/* PMUADCCFG 0x356 */
> +#define MT6360_ADCEN_MASK 0x8000
> +/* PMUADCRPT1 0x35A */
> +#define MT6360_PREFERCH_MASK 0xF0
> +#define MT6360_PREFERCH_SHFT 4
> +#define MT6360_RPTCH_MASK 0x0F
> +
> +enum {
> + MT6360_CHAN_USBID = 0,
> + MT6360_CHAN_VBUSDIV5,
> + MT6360_CHAN_VBUSDIV2,
> + MT6360_CHAN_VSYS,
> + MT6360_CHAN_VBAT,
> + MT6360_CHAN_IBUS,
> + MT6360_CHAN_IBAT,
> + MT6360_CHAN_CHG_VDDP,
> + MT6360_CHAN_TEMP_JC,
> + MT6360_CHAN_VREF_TS,
> + MT6360_CHAN_TS,
> + MT6360_CHAN_MAX,
> +};
> +
> +struct mt6360_adc_data {
> + struct device *dev;
> + struct regmap *regmap;
> + struct completion adc_complete;
> + struct mutex adc_lock;
> + ktime_t last_off_timestamps[MT6360_CHAN_MAX];
> + int irq;
> +};
> +
> +static inline int mt6360_adc_val_converter(int val, int multiplier, int offset, int divisor)
> +{
> + return ((val * multiplier) + offset) / divisor;
Why could we not report these values to userspace or consumer drivers and let
them deal with the conversion if they actually needed it?
Mapping this to
(val + new_offset) * multiplier would be a little messy, but not too bad.
The advantage would be that we would then be providing the data needed
to get real units for values read from the buffers without having to
do all the maths in kernel (without access to floating point).
> +}
> +
> +static int mt6360_adc_convert_processed_val(struct mt6360_adc_data *info, int chan_idx, int *val)
> +{
> + unsigned int regval = 0;
> + const struct converter {
> + int multiplier;
> + int offset;
> + int divisor;
> + } adc_converter[MT6360_CHAN_MAX] = {
> + { 1250, 0, 1}, /* USBID */
> + { 6250, 0, 1}, /* VBUSDIV5 */
> + { 2500, 0, 1}, /* VBUSDIV2 */
> + { 1250, 0, 1}, /* VSYS */
> + { 1250, 0, 1}, /* VBAT */
> + { 2500, 0, 1}, /* IBUS */
> + { 2500, 0, 1}, /* IBAT */
> + { 1250, 0, 1}, /* CHG_VDDP */
> + { 105, -8000, 100}, /* TEMP_JC */
> + { 1250, 0, 1}, /* VREF_TS */
> + { 1250, 0, 1}, /* TS */
> + }, sp_ibus_adc_converter = { 1900, 0, 1 }, *sel_converter;
> + int ret;
> +
> + sel_converter = adc_converter + chan_idx;
> + if (chan_idx == MT6360_CHAN_IBUS) {
> + /* ibus chan will be affected by aicr config */
> + /* if aicr < 400, apply the special ibus converter */
> + ret = regmap_read(info->regmap, MT6360_REG_PMUCHGCTRL3, ®val);
> + if (ret)
> + return ret;
> +
> + regval = (regval & MT6360_AICR_MASK) >> MT6360_AICR_SHFT;
> + if (regval < MT6360_AICR_400MA)
> + sel_converter = &sp_ibus_adc_converter;
> + }
> +
> + *val = mt6360_adc_val_converter(*val, sel_converter->multiplier, sel_converter->offset,
> + sel_converter->divisor);
> +
> + return 0;
> +}
> +
> +static int mt6360_adc_read_processed(struct mt6360_adc_data *mad, int channel, int *val)
> +{
> + u16 adc_enable;
> + u8 rpt[3];
> + ktime_t start_t, predict_end_t;
> + long timeout;
> + int value, ret;
> +
> + mutex_lock(&mad->adc_lock);
> +
> + /* select preferred channel that we want */
> + ret = regmap_update_bits(mad->regmap, MT6360_REG_PMUADCRPT1, MT6360_PREFERCH_MASK,
> + channel << MT6360_PREFERCH_SHFT);
> + if (ret)
> + goto out_adc;
> +
> + /* enable adc channel we want and adc_en */
> + adc_enable = MT6360_ADCEN_MASK | BIT(channel);
> + adc_enable = cpu_to_be16(adc_enable);
Use a local be16 to store that. It will make it a little clearer
that we are doing something 'unusual' here. Perhaps a comment on
why this odd code exists would also help?
> + ret = regmap_raw_write(mad->regmap, MT6360_REG_PMUADCCFG, (void *)&adc_enable, sizeof(u16));
> + if (ret)
> + goto out_adc;
> +
> + start_t = ktime_get();
> + predict_end_t = ktime_add_ms(mad->last_off_timestamps[channel], 50);
> +
> + if (ktime_after(start_t, predict_end_t))
> + predict_end_t = ktime_add_ms(start_t, 25);
> + else
> + predict_end_t = ktime_add_ms(start_t, 75);
> +
> + enable_irq(mad->irq);
> +adc_retry:
> + reinit_completion(&mad->adc_complete);
> +
> + /* wait for conversion to complete */
> + timeout = wait_for_completion_timeout(&mad->adc_complete, msecs_to_jiffies(200));
> + if (timeout == 0) {
> + ret = -ETIMEDOUT;
> + goto out_adc_conv;
> + } else if (timeout < 0) {
> + ret = -EINTR;
> + goto out_adc_conv;
> + }
> +
> + ret = regmap_raw_read(mad->regmap, MT6360_REG_PMUADCRPT1, rpt, sizeof(rpt));
> + if (ret)
> + goto out_adc_conv;
> +
> + /* check the current reported channel */
> + if ((rpt[0] & MT6360_RPTCH_MASK) != channel) {
> + dev_dbg(mad->dev, "not wanted channel report [%02x]\n", rpt[0]);
This and the one below feel like error messages rather than debug ones.
> + goto adc_retry;
> + }
> +
> + if (!ktime_after(ktime_get(), predict_end_t)) {
> + dev_dbg(mad->dev, "time is not after one adc_conv_t\n");
Does this actually happen? If feels like we are being a bit over protective
here. I'd definitely like to see a comment saying why this protection
might be needed.
> + goto adc_retry;
> + }
> +
> + value = (rpt[1] << 8) | rpt[2];
> +
> + ret = mt6360_adc_convert_processed_val(mad, channel, &value);
> + if (ret)
> + goto out_adc_conv;
> +
> + *val = value;
> + ret = IIO_VAL_INT;
> +
> +out_adc_conv:
> + disable_irq(mad->irq);
> + adc_enable = MT6360_ADCEN_MASK;
> + adc_enable = cpu_to_be16(adc_enable);
> + regmap_raw_write(mad->regmap, MT6360_REG_PMUADCCFG, (void *)&adc_enable, sizeof(u16));
> + mad->last_off_timestamps[channel] = ktime_get();
> + /* set prefer channel to 0xf */
> + regmap_update_bits(mad->regmap, MT6360_REG_PMUADCRPT1, MT6360_PREFERCH_MASK,
> + 0xF << MT6360_PREFERCH_SHFT);
> +out_adc:
> + mutex_unlock(&mad->adc_lock);
> +
> + return ret;
> +}
> +
> +static int mt6360_adc_read_raw(struct iio_dev *iio_dev, const struct iio_chan_spec *chan,
> + int *val, int *val2, long mask)
> +{
> + struct mt6360_adc_data *mad = iio_priv(iio_dev);
> +
> + if (mask == IIO_CHAN_INFO_PROCESSED)
> + return mt6360_adc_read_processed(mad, chan->channel, val);
> +
> + return -EINVAL;
> +}
> +
> +static const struct iio_info mt6360_adc_iio_info = {
> + .read_raw = mt6360_adc_read_raw,
> +};
> +
> +#define MT6360_ADC_CHAN(_idx, _type) { \
> + .type = _type, \
> + .channel = MT6360_CHAN_##_idx, \
> + .scan_index = MT6360_CHAN_##_idx, \
> + .extend_name = #_idx, \
> + .datasheet_name = #_idx, \
> + .scan_type = { \
> + .sign = 's', \
> + .realbits = 32, \
> + .storagebits = 32, \
> + .endianness = IIO_CPU, \
> + }, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
> + .indexed = 1, \
> +}
> +
> +static const struct iio_chan_spec mt6360_adc_channels[] = {
> + MT6360_ADC_CHAN(USBID, IIO_VOLTAGE),
> + MT6360_ADC_CHAN(VBUSDIV5, IIO_VOLTAGE),
> + MT6360_ADC_CHAN(VBUSDIV2, IIO_VOLTAGE),
> + MT6360_ADC_CHAN(VSYS, IIO_VOLTAGE),
> + MT6360_ADC_CHAN(VBAT, IIO_VOLTAGE),
> + MT6360_ADC_CHAN(IBUS, IIO_CURRENT),
> + MT6360_ADC_CHAN(IBAT, IIO_CURRENT),
> + MT6360_ADC_CHAN(CHG_VDDP, IIO_VOLTAGE),
> + MT6360_ADC_CHAN(TEMP_JC, IIO_TEMP),
> + MT6360_ADC_CHAN(VREF_TS, IIO_VOLTAGE),
> + MT6360_ADC_CHAN(TS, IIO_VOLTAGE),
> + IIO_CHAN_SOFT_TIMESTAMP(MT6360_CHAN_MAX),
> +};
> +
> +static irqreturn_t mt6360_pmu_adc_donei_handler(int irq, void *data)
> +{
> + struct mt6360_adc_data *mad = iio_priv(data);
> +
> + complete(&mad->adc_complete);
> + return IRQ_HANDLED;
> +}
> +
...
> +
> +static int mt6360_adc_probe(struct platform_device *pdev)
> +{
> + struct mt6360_adc_data *mad;
> + struct iio_dev *indio_dev;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*mad));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + mad = iio_priv(indio_dev);
> + mad->dev = &pdev->dev;
> + init_completion(&mad->adc_complete);
> + mutex_init(&mad->adc_lock);
> +
> + mad->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> + if (!mad->regmap) {
> + dev_err(&pdev->dev, "Failed to get parent regmap\n");
> + return -ENODEV;
> + }
> +
> + ret = mt6360_adc_reset(mad);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "Failed to reset adc\n");
> + return ret;
> + }
> +
> + mad->irq = platform_get_irq_byname(pdev, "adc_donei");
> + if (mad->irq < 0) {
> + dev_err(&pdev->dev, "Failed to get adc_done irq\n");
> + return mad->irq;
> + }
> +
> + irq_set_status_flags(mad->irq, IRQ_NOAUTOEN);
As we are going to have a v5 anyway to clean up that endian warning,
please could you add a comment to explain the need for IRQ_NOAUTOEN?
> + ret = devm_request_threaded_irq(&pdev->dev, mad->irq, NULL, mt6360_pmu_adc_donei_handler, 0,
> + "adc_donei", indio_dev);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to register adc_done irq\n");
> + return ret;
> + }
> +
> + indio_dev->name = dev_name(&pdev->dev);
> + indio_dev->dev.parent = &pdev->dev;
> + indio_dev->info = &mt6360_adc_iio_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->channels = mt6360_adc_channels;
> + indio_dev->num_channels = ARRAY_SIZE(mt6360_adc_channels);
> +
> + ret = devm_iio_triggered_buffer_setup(&pdev->dev, indio_dev, NULL,
> + mt6360_adc_trigger_handler, NULL);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to allocate iio trigger buffer\n");
> + return ret;
> + }
> +
> + ret = devm_iio_device_register(&pdev->dev, indio_dev);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to register iio device\n");
> + return ret;
> + }
> +
> + return 0;
> +}
...
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 2/2] Documentation: ABI: testing: mt6360: Add ADC sysfs guideline
2020-08-24 9:06 ` [PATCH v3 2/2] Documentation: ABI: testing: mt6360: Add ADC sysfs guideline Gene Chen
@ 2020-08-29 17:15 ` Jonathan Cameron
0 siblings, 0 replies; 15+ messages in thread
From: Jonathan Cameron @ 2020-08-29 17:15 UTC (permalink / raw)
To: Gene Chen
Cc: matthias.bgg, knaack.h, lars, pmeerw, linux-iio,
linux-arm-kernel, linux-mediatek, linux-kernel, gene_chen,
Wilma.Wu, shufan_lee, cy_huang, benjamin.chao
On Mon, 24 Aug 2020 17:06:25 +0800
Gene Chen <gene.chen.richtek@gmail.com> wrote:
> From: Gene Chen <gene_chen@richtek.com>
>
> Add ABI documentation for mt6360 ADC sysfs interfaces.
Please check this. The actual filenames don't look correct
to me.
>
> Signed-off-by: Gene Chen <gene_chen@richtek.com>
> ---
> Documentation/ABI/testing/sysfs-bus-iio-adc-mt6360 | 83 ++++++++++++++++++++++
> 1 file changed, 83 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-mt6360
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-mt6360 b/Documentation/ABI/testing/sysfs-bus-iio-adc-mt6360
> new file mode 100644
> index 0000000..9dab17e
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-mt6360
> @@ -0,0 +1,83 @@
> +What: /sys/bus/iio/devices/iio:deviceX/usbid_input
> +KernelVersion: 5.8.0
> +Contact: gene_chen@richtek.com
> +Description:
> + Indicated MT6360 USBID ADC
Can we give a bit of text saying what that is?
It's nice if people writing userspace software looking at this don't have
to go find the datasheet just to give a human readable description of the
channel.
> + Reading returns current voltage in uV
drop _current_ as that's a bit confusing :)
Reading returns voltage in uV
> +
> +What: /sys/bus/iio/devices/iio:deviceX/vbusdiv5_input
> +KernelVersion: 5.8.0
> +Contact: gene_chen@richtek.com
> +Description:
> + Indicated MT6360 VBUS ADC with high accuracy
> + Reading returns current voltage in uV
Is this measuring output of a regulator which has taken vbus and divided
it by 5? I'm rather confused.
> +
> +What: /sys/bus/iio/devices/iio:deviceX/vbusdiv2_input
> +KernelVersion: 5.8.0
> +Contact: gene_chen@richtek.com
> +Description:
> + Indicated MT6360 VBUS ADC with low accuracy
> + Reading returns current voltage in uV
> +
> +What: /sys/bus/iio/devices/iio:deviceX/vsys_input
> +KernelVersion: 5.8.0
> +Contact: gene_chen@richtek.com
> +Description:
> + Indicated MT6360 VSYS ADC
> + Reading returns current voltage in uV
> +
> +What: /sys/bus/iio/devices/iio:deviceX/vbat_input
> +KernelVersion: 5.8.0
> +Contact: gene_chen@richtek.com
> +Description:
> + Indicated MT6360 VBAT ADC
> + Reading returns current voltage in uV
> +
> +What: /sys/bus/iio/devices/iio:deviceX/ibus_input
> +KernelVersion: 5.8.0
> +Contact: gene_chen@richtek.com
> +Description:
> + Indicated MT6360 IBUS ADC
> + Reading returns current in uA
> +
> +What: /sys/bus/iio/devices/iio:deviceX/ibat_input
> +KernelVersion: 5.8.0
> +Contact: gene_chen@richtek.com
> +Description:
> + Indicated MT6360 IBAT ADC
> + Reading returns current in uA
> +
> +What: /sys/bus/iio/devices/iio:deviceX/chg_vddp_input
> +KernelVersion: 5.8.0
> +Contact: gene_chen@richtek.com
> +Description:
> + Indicated MT6360 CHG_VDDP ADC
> + Reading returns current voltage in uV
> +
> +What: /sys/bus/iio/devices/iio:deviceX/temp_jc_input
> +KernelVersion: 5.8.0
> +Contact: gene_chen@richtek.com
> +Description:
> + Indicated MT6360 IC junction temperature
> + Reading returns current temperature in degree
> +
> +What: /sys/bus/iio/devices/iio:deviceX/vref_ts_input
> +KernelVersion: 5.8.0
> +Contact: gene_chen@richtek.com
> +Description:
> + Indicated MT6360 VREF_TS ADC
> + Reading returns current voltage in uV
> +
> +What: /sys/bus/iio/devices/iio:deviceX/ts_input
> +KernelVersion: 5.8.0
> +Contact: gene_chen@richtek.com
> +Description:
> + Indicated MT6360 TS ADC
> + Reading returns current voltage in uV
> +
> +What: /sys/bus/iio/devices/iio:deviceX/timestamp
> +KernelVersion: 5.8.0
> +Contact: gene_chen@richtek.com
> +Description:
> + Indicated MT6360 timestamp
> + Reading returns current timestamp in ms
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 1/2] iio: adc: mt6360: Add ADC driver for MT6360
2020-08-24 9:06 ` [PATCH v3 1/2] " Gene Chen
2020-08-27 12:16 ` kernel test robot
2020-08-29 17:11 ` Jonathan Cameron
@ 2020-08-30 17:39 ` Andy Shevchenko
2 siblings, 0 replies; 15+ messages in thread
From: Andy Shevchenko @ 2020-08-30 17:39 UTC (permalink / raw)
To: Gene Chen
Cc: Jonathan Cameron, Matthias Brugger, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald, linux-iio,
linux-arm Mailing List, moderated list:ARM/Mediatek SoC support,
Linux Kernel Mailing List, gene_chen, Wilma.Wu, shufan_lee,
cy_huang, benjamin.chao
On Mon, Aug 24, 2020 at 12:07 PM Gene Chen <gene.chen.richtek@gmail.com> wrote:
>
> Add MT6360 ADC driver include Charger Current, Voltage, and
include -> that includes
> Temperature.
...
> + help
> + Say Y here to enable MT6360 ADC Part.
> + Integrated for System Monitoring include
> + Charger and Battery Current, Voltage and
> + Temperature
We have a wider field for this text. Also, use proper punctuation.
...
> +#include <linux/completion.h>
> +#include <linux/interrupt.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
I would rather move this block after linux/* headers because it's
subsystem related (not so generic).
> +#include <linux/irq.h>
Are you sure about this?
> +#include <linux/kernel.h>
> +#include <linux/ktime.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
...
> +#define MT6360_AICR_MASK 0xFC
GENMASK() (and include bits.h for that).
> +#define MT6360_ADCEN_MASK 0x8000
> +#define MT6360_PREFERCH_MASK 0xF0
> +#define MT6360_RPTCH_MASK 0x0F
Ditto for all above.
...
> +enum {
> + MT6360_CHAN_USBID = 0,
> + MT6360_CHAN_VBUSDIV5,
> + MT6360_CHAN_VBUSDIV2,
> + MT6360_CHAN_VSYS,
> + MT6360_CHAN_VBAT,
> + MT6360_CHAN_IBUS,
> + MT6360_CHAN_IBAT,
> + MT6360_CHAN_CHG_VDDP,
> + MT6360_CHAN_TEMP_JC,
> + MT6360_CHAN_VREF_TS,
> + MT6360_CHAN_TS,
> + MT6360_CHAN_MAX,
No comma for terminator.
> +};
...
> + const struct converter {
> + int multiplier;
> + int offset;
> + int divisor;
> + } adc_converter[MT6360_CHAN_MAX] = {
> + { 1250, 0, 1}, /* USBID */
> + { 6250, 0, 1}, /* VBUSDIV5 */
> + { 2500, 0, 1}, /* VBUSDIV2 */
> + { 1250, 0, 1}, /* VSYS */
> + { 1250, 0, 1}, /* VBAT */
> + { 2500, 0, 1}, /* IBUS */
> + { 2500, 0, 1}, /* IBAT */
> + { 1250, 0, 1}, /* CHG_VDDP */
> + { 105, -8000, 100}, /* TEMP_JC */
> + { 1250, 0, 1}, /* VREF_TS */
> + { 1250, 0, 1}, /* TS */
> + }, sp_ibus_adc_converter = { 1900, 0, 1 }, *sel_converter;
This is quite hidden in the code. Better to move out from the function at least.
...
> + start_t = ktime_get();
> + predict_end_t = ktime_add_ms(mad->last_off_timestamps[channel], 50);
> +
> + if (ktime_after(start_t, predict_end_t))
> + predict_end_t = ktime_add_ms(start_t, 25);
> + else
> + predict_end_t = ktime_add_ms(start_t, 75);
> +
> + timeout = wait_for_completion_timeout(&mad->adc_complete, msecs_to_jiffies(200));
Above code full of magic numbers.
...
> + value = (rpt[1] << 8) | rpt[2];
put_unaligned_be16() (or what is this?)
...
> + /* set prefer channel to 0xf */
What 0x0f is?
> + regmap_update_bits(mad->regmap, MT6360_REG_PMUADCRPT1, MT6360_PREFERCH_MASK,
> + 0xF << MT6360_PREFERCH_SHFT);
0xf should be GENMASK() and have its descriptive name.
> +out_adc:
The rule of thumb is to explain in the label what is going to do,
rather than some abstract words. Here, i.e.,
out_mutex_unlock:
> + mutex_unlock(&mad->adc_lock);
...
> + if (mask == IIO_CHAN_INFO_PROCESSED)
> + return mt6360_adc_read_processed(mad, chan->channel, val);
> +
> + return -EINVAL;
Usual pattern is
if (err_condition)
...handle error...
...
> + /* 11 ch s32 numbers + 1 s64 timestamp */
> + s32 data[MT6360_CHAN_MAX + 2] __aligned(8);
We have a better approach now (with a struct being used).
...
> + u8 configs[3] = {0x80, 0, 0};
Magic.
...
> +static int mt6360_adc_probe(struct platform_device *pdev)
> +{
> + mad->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> + if (!mad->regmap) {
> + dev_err(&pdev->dev, "Failed to get parent regmap\n");
> + return -ENODEV;
> + }
You may do this before allocation happens. Also consider to introduce
temporary variable to simplify below code, i.e.
struct device *dev = &pdev->dev;
struct regmap *regmap;
...
> + mad->irq = platform_get_irq_byname(pdev, "adc_donei");
> + if (mad->irq < 0) {
> + dev_err(&pdev->dev, "Failed to get adc_done irq\n");
This duplicates the core message.
> + return mad->irq;
> + }
...
> + irq_set_status_flags(mad->irq, IRQ_NOAUTOEN);
Why?
> + ret = devm_iio_device_register(&pdev->dev, indio_dev);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to register iio device\n");
> + return ret;
> + }
> +
> + return 0;
return ret; (At least, but I would go even further and do return
devm_iio_device_register(); directly)
> +}
...
> +static const struct of_device_id __maybe_unused mt6360_adc_of_id[] = {
> + { .compatible = "mediatek,mt6360-adc", },
> + {},
No comma for terminator line.
> +};
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 1/2] iio: adc: mt6360: Add ADC driver for MT6360
2020-08-29 17:11 ` Jonathan Cameron
@ 2020-09-08 6:17 ` Gene Chen
2020-09-08 7:43 ` Andy Shevchenko
2020-09-08 9:07 ` Jonathan Cameron
0 siblings, 2 replies; 15+ messages in thread
From: Gene Chen @ 2020-09-08 6:17 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Matthias Brugger, knaack.h, lars, pmeerw, linux-iio,
linux-arm-kernel, linux-mediatek, linux-kernel, Gene Chen,
Wilma.Wu, shufan_lee, cy_huang, benjamin.chao
Jonathan Cameron <jic23@kernel.org> 於 2020年8月30日 週日 上午1:12寫道：
>
> On Mon, 24 Aug 2020 17:06:24 +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>
> Hi Gene,
>
> A few comments inline. The big one centres on why we can't
> expose the channels as _raw, _offset and _scale?
>
I think i have 3 reason for use real value,
ADC is used to get real value rather than raw data which is not meaningful.
And I can decide which formula needs apply according to different condition.
Also the junction temperature channel _scale is floating point 1.05
which is not easy to express.
> Thanks,
>
> Jonathan
>
> > ---
> > drivers/iio/adc/Kconfig | 11 ++
> > drivers/iio/adc/Makefile | 1 +
> > drivers/iio/adc/mt6360-adc.c | 366 +++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 378 insertions(+)
> > create mode 100644 drivers/iio/adc/mt6360-adc.c
> >
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index 66d9cc0..07dcea7 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -703,6 +703,17 @@ config MCP3911
> > This driver can also be built as a module. If so, the module will be
> > called mcp3911.
> >
> > +config MEDIATEK_MT6360_ADC
> > + tristate "Mediatek MT6360 ADC Part"
> > + depends on MFD_MT6360
> > + select IIO_BUFFER
> > + select IIO_TRIGGERED_BUFFER
> > + help
> > + Say Y here to enable MT6360 ADC Part.
> > + Integrated for System Monitoring include
> > + Charger and Battery Current, Voltage and
> > + Temperature
> > +
> > config MEDIATEK_MT6577_AUXADC
> > tristate "MediaTek AUXADC driver"
> > depends on ARCH_MEDIATEK || COMPILE_TEST
> > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > index 90f94ad..5fca90a 100644
> > --- a/drivers/iio/adc/Makefile
> > +++ b/drivers/iio/adc/Makefile
> > @@ -65,6 +65,7 @@ obj-$(CONFIG_MAX9611) += max9611.o
> > obj-$(CONFIG_MCP320X) += mcp320x.o
> > obj-$(CONFIG_MCP3422) += mcp3422.o
> > obj-$(CONFIG_MCP3911) += mcp3911.o
> > +obj-$(CONFIG_MEDIATEK_MT6360_ADC) += mt6360-adc.o
> > obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o
> > obj-$(CONFIG_MEN_Z188_ADC) += men_z188_adc.o
> > obj-$(CONFIG_MESON_SARADC) += meson_saradc.o
> > diff --git a/drivers/iio/adc/mt6360-adc.c b/drivers/iio/adc/mt6360-adc.c
> > new file mode 100644
> > index 0000000..5eed812
> > --- /dev/null
> > +++ b/drivers/iio/adc/mt6360-adc.c
> > @@ -0,0 +1,366 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2020 MediaTek Inc.
> > + *
> > + * Author: Gene Chen <gene_chen@richtek.com>
> > + */
> > +
> > +#include <linux/completion.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/iio/buffer.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/trigger_consumer.h>
> > +#include <linux/iio/triggered_buffer.h>
> > +#include <linux/irq.h>
> > +#include <linux/kernel.h>
> > +#include <linux/ktime.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +
> > +#define MT6360_REG_PMUCHGCTRL3 0x313
> > +#define MT6360_REG_PMUADCCFG 0x356
> > +#define MT6360_REG_PMUADCRPT1 0x35A
> > +
> > +/* PMUCHGCTRL3 0x313 */
> > +#define MT6360_AICR_MASK 0xFC
> > +#define MT6360_AICR_SHFT 2
> > +#define MT6360_AICR_400MA 0x6
> > +/* PMUADCCFG 0x356 */
> > +#define MT6360_ADCEN_MASK 0x8000
> > +/* PMUADCRPT1 0x35A */
> > +#define MT6360_PREFERCH_MASK 0xF0
> > +#define MT6360_PREFERCH_SHFT 4
> > +#define MT6360_RPTCH_MASK 0x0F
> > +
> > +enum {
> > + MT6360_CHAN_USBID = 0,
> > + MT6360_CHAN_VBUSDIV5,
> > + MT6360_CHAN_VBUSDIV2,
> > + MT6360_CHAN_VSYS,
> > + MT6360_CHAN_VBAT,
> > + MT6360_CHAN_IBUS,
> > + MT6360_CHAN_IBAT,
> > + MT6360_CHAN_CHG_VDDP,
> > + MT6360_CHAN_TEMP_JC,
> > + MT6360_CHAN_VREF_TS,
> > + MT6360_CHAN_TS,
> > + MT6360_CHAN_MAX,
> > +};
> > +
> > +struct mt6360_adc_data {
> > + struct device *dev;
> > + struct regmap *regmap;
> > + struct completion adc_complete;
> > + struct mutex adc_lock;
> > + ktime_t last_off_timestamps[MT6360_CHAN_MAX];
> > + int irq;
> > +};
> > +
> > +static inline int mt6360_adc_val_converter(int val, int multiplier, int offset, int divisor)
> > +{
> > + return ((val * multiplier) + offset) / divisor;
>
> Why could we not report these values to userspace or consumer drivers and let
> them deal with the conversion if they actually needed it?
> Mapping this to
>
> (val + new_offset) * multiplier would be a little messy, but not too bad.
>
> The advantage would be that we would then be providing the data needed
> to get real units for values read from the buffers without having to
> do all the maths in kernel (without access to floating point).
>
>
As above, if I use formula "(val + new_offset) * multiplier",
the junction temperature channel multiplier will be floating point
1.05, i don't know how to express.
> > +}
> > +
> > +static int mt6360_adc_convert_processed_val(struct mt6360_adc_data *info, int chan_idx, int *val)
> > +{
> > + unsigned int regval = 0;
> > + const struct converter {
> > + int multiplier;
> > + int offset;
> > + int divisor;
> > + } adc_converter[MT6360_CHAN_MAX] = {
> > + { 1250, 0, 1}, /* USBID */
> > + { 6250, 0, 1}, /* VBUSDIV5 */
> > + { 2500, 0, 1}, /* VBUSDIV2 */
> > + { 1250, 0, 1}, /* VSYS */
> > + { 1250, 0, 1}, /* VBAT */
> > + { 2500, 0, 1}, /* IBUS */
> > + { 2500, 0, 1}, /* IBAT */
> > + { 1250, 0, 1}, /* CHG_VDDP */
> > + { 105, -8000, 100}, /* TEMP_JC */
> > + { 1250, 0, 1}, /* VREF_TS */
> > + { 1250, 0, 1}, /* TS */
> > + }, sp_ibus_adc_converter = { 1900, 0, 1 }, *sel_converter;
> > + int ret;
> > +
> > + sel_converter = adc_converter + chan_idx;
> > + if (chan_idx == MT6360_CHAN_IBUS) {
> > + /* ibus chan will be affected by aicr config */
> > + /* if aicr < 400, apply the special ibus converter */
> > + ret = regmap_read(info->regmap, MT6360_REG_PMUCHGCTRL3, ®val);
> > + if (ret)
> > + return ret;
> > +
> > + regval = (regval & MT6360_AICR_MASK) >> MT6360_AICR_SHFT;
> > + if (regval < MT6360_AICR_400MA)
> > + sel_converter = &sp_ibus_adc_converter;
> > + }
> > +
> > + *val = mt6360_adc_val_converter(*val, sel_converter->multiplier, sel_converter->offset,
> > + sel_converter->divisor);
> > +
> > + return 0;
> > +}
> > +
> > +static int mt6360_adc_read_processed(struct mt6360_adc_data *mad, int channel, int *val)
> > +{
> > + u16 adc_enable;
> > + u8 rpt[3];
> > + ktime_t start_t, predict_end_t;
> > + long timeout;
> > + int value, ret;
> > +
> > + mutex_lock(&mad->adc_lock);
> > +
> > + /* select preferred channel that we want */
> > + ret = regmap_update_bits(mad->regmap, MT6360_REG_PMUADCRPT1, MT6360_PREFERCH_MASK,
> > + channel << MT6360_PREFERCH_SHFT);
> > + if (ret)
> > + goto out_adc;
> > +
> > + /* enable adc channel we want and adc_en */
> > + adc_enable = MT6360_ADCEN_MASK | BIT(channel);
> > + adc_enable = cpu_to_be16(adc_enable);
>
> Use a local be16 to store that. It will make it a little clearer
> that we are doing something 'unusual' here. Perhaps a comment on
> why this odd code exists would also help?
>
ACK
> > + ret = regmap_raw_write(mad->regmap, MT6360_REG_PMUADCCFG, (void *)&adc_enable, sizeof(u16));
> > + if (ret)
> > + goto out_adc;
> > +
> > + start_t = ktime_get();
> > + predict_end_t = ktime_add_ms(mad->last_off_timestamps[channel], 50);
> > +
> > + if (ktime_after(start_t, predict_end_t))
> > + predict_end_t = ktime_add_ms(start_t, 25);
> > + else
> > + predict_end_t = ktime_add_ms(start_t, 75);
> > +
> > + enable_irq(mad->irq);
> > +adc_retry:
> > + reinit_completion(&mad->adc_complete);
> > +
> > + /* wait for conversion to complete */
> > + timeout = wait_for_completion_timeout(&mad->adc_complete, msecs_to_jiffies(200));
> > + if (timeout == 0) {
> > + ret = -ETIMEDOUT;
> > + goto out_adc_conv;
> > + } else if (timeout < 0) {
> > + ret = -EINTR;
> > + goto out_adc_conv;
> > + }
> > +
> > + ret = regmap_raw_read(mad->regmap, MT6360_REG_PMUADCRPT1, rpt, sizeof(rpt));
> > + if (ret)
> > + goto out_adc_conv;
> > +
> > + /* check the current reported channel */
> > + if ((rpt[0] & MT6360_RPTCH_MASK) != channel) {
> > + dev_dbg(mad->dev, "not wanted channel report [%02x]\n", rpt[0]);
>
> This and the one below feel like error messages rather than debug ones.
>
We have two function "battery zero current voltage(ZCV)" and "TypeC
OTP" will auto run ADC at background.
ZCV_EN will run VBAT_ADC when TA plug in, TypeC OTP will run TS_ADC
when TypeC attach.
We need to check report channel for ADC report data match is our desire channel.
> > + goto adc_retry;
> > + }
> > +
> > + if (!ktime_after(ktime_get(), predict_end_t)) {
> > + dev_dbg(mad->dev, "time is not after one adc_conv_t\n");
>
> Does this actually happen? If feels like we are being a bit over protective
> here. I'd definitely like to see a comment saying why this protection
> might be needed.
>
When ADC_EN and MT6360_CHANx_EN is enable, the channel x will keep
running again and again
I supposed to get immediate data which is generated after I start it.
When I disable ADC_CHANx_EN, the H/W logical ADC is still running.
If I run the same ADC immediately, I may get the old result about this channel.
MT6360 ADC typical conversation time is about 25ms.
So We need ignore which irq trigger below 25ms.
> > + goto adc_retry;
> > + }
> > +
> > + value = (rpt[1] << 8) | rpt[2];
> > +
> > + ret = mt6360_adc_convert_processed_val(mad, channel, &value);
> > + if (ret)
> > + goto out_adc_conv;
> > +
> > + *val = value;
> > + ret = IIO_VAL_INT;
> > +
> > +out_adc_conv:
> > + disable_irq(mad->irq);
> > + adc_enable = MT6360_ADCEN_MASK;
> > + adc_enable = cpu_to_be16(adc_enable);
> > + regmap_raw_write(mad->regmap, MT6360_REG_PMUADCCFG, (void *)&adc_enable, sizeof(u16));
> > + mad->last_off_timestamps[channel] = ktime_get();
> > + /* set prefer channel to 0xf */
> > + regmap_update_bits(mad->regmap, MT6360_REG_PMUADCRPT1, MT6360_PREFERCH_MASK,
> > + 0xF << MT6360_PREFERCH_SHFT);
> > +out_adc:
> > + mutex_unlock(&mad->adc_lock);
> > +
> > + return ret;
> > +}
> > +
> > +static int mt6360_adc_read_raw(struct iio_dev *iio_dev, const struct iio_chan_spec *chan,
> > + int *val, int *val2, long mask)
> > +{
> > + struct mt6360_adc_data *mad = iio_priv(iio_dev);
> > +
> > + if (mask == IIO_CHAN_INFO_PROCESSED)
> > + return mt6360_adc_read_processed(mad, chan->channel, val);
> > +
> > + return -EINVAL;
> > +}
> > +
> > +static const struct iio_info mt6360_adc_iio_info = {
> > + .read_raw = mt6360_adc_read_raw,
> > +};
> > +
> > +#define MT6360_ADC_CHAN(_idx, _type) { \
> > + .type = _type, \
> > + .channel = MT6360_CHAN_##_idx, \
> > + .scan_index = MT6360_CHAN_##_idx, \
> > + .extend_name = #_idx, \
> > + .datasheet_name = #_idx, \
> > + .scan_type = { \
> > + .sign = 's', \
> > + .realbits = 32, \
> > + .storagebits = 32, \
> > + .endianness = IIO_CPU, \
> > + }, \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
> > + .indexed = 1, \
> > +}
> > +
> > +static const struct iio_chan_spec mt6360_adc_channels[] = {
> > + MT6360_ADC_CHAN(USBID, IIO_VOLTAGE),
> > + MT6360_ADC_CHAN(VBUSDIV5, IIO_VOLTAGE),
> > + MT6360_ADC_CHAN(VBUSDIV2, IIO_VOLTAGE),
> > + MT6360_ADC_CHAN(VSYS, IIO_VOLTAGE),
> > + MT6360_ADC_CHAN(VBAT, IIO_VOLTAGE),
> > + MT6360_ADC_CHAN(IBUS, IIO_CURRENT),
> > + MT6360_ADC_CHAN(IBAT, IIO_CURRENT),
> > + MT6360_ADC_CHAN(CHG_VDDP, IIO_VOLTAGE),
> > + MT6360_ADC_CHAN(TEMP_JC, IIO_TEMP),
> > + MT6360_ADC_CHAN(VREF_TS, IIO_VOLTAGE),
> > + MT6360_ADC_CHAN(TS, IIO_VOLTAGE),
> > + IIO_CHAN_SOFT_TIMESTAMP(MT6360_CHAN_MAX),
> > +};
> > +
> > +static irqreturn_t mt6360_pmu_adc_donei_handler(int irq, void *data)
> > +{
> > + struct mt6360_adc_data *mad = iio_priv(data);
> > +
> > + complete(&mad->adc_complete);
> > + return IRQ_HANDLED;
> > +}
> > +
> ...
>
> > +
> > +static int mt6360_adc_probe(struct platform_device *pdev)
> > +{
> > + struct mt6360_adc_data *mad;
> > + struct iio_dev *indio_dev;
> > + int ret;
> > +
> > + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*mad));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + mad = iio_priv(indio_dev);
> > + mad->dev = &pdev->dev;
> > + init_completion(&mad->adc_complete);
> > + mutex_init(&mad->adc_lock);
> > +
> > + mad->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> > + if (!mad->regmap) {
> > + dev_err(&pdev->dev, "Failed to get parent regmap\n");
> > + return -ENODEV;
> > + }
> > +
> > + ret = mt6360_adc_reset(mad);
> > + if (ret < 0) {
> > + dev_err(&pdev->dev, "Failed to reset adc\n");
> > + return ret;
> > + }
> > +
> > + mad->irq = platform_get_irq_byname(pdev, "adc_donei");
> > + if (mad->irq < 0) {
> > + dev_err(&pdev->dev, "Failed to get adc_done irq\n");
> > + return mad->irq;
> > + }
> > +
> > + irq_set_status_flags(mad->irq, IRQ_NOAUTOEN);
>
> As we are going to have a v5 anyway to clean up that endian warning,
> please could you add a comment to explain the need for IRQ_NOAUTOEN?
>
Same as above "Enable ADC will run again and again until clear
ADC__CHANx_EN bit"
So After I get the ADC result, I disable irq in order to handle only
oneshot data.
> > + ret = devm_request_threaded_irq(&pdev->dev, mad->irq, NULL, mt6360_pmu_adc_donei_handler, 0,
> > + "adc_donei", indio_dev);
> > + if (ret) {
> > + dev_err(&pdev->dev, "Failed to register adc_done irq\n");
> > + return ret;
> > + }
> > +
> > + indio_dev->name = dev_name(&pdev->dev);
> > + indio_dev->dev.parent = &pdev->dev;
> > + indio_dev->info = &mt6360_adc_iio_info;
> > + indio_dev->modes = INDIO_DIRECT_MODE;
> > + indio_dev->channels = mt6360_adc_channels;
> > + indio_dev->num_channels = ARRAY_SIZE(mt6360_adc_channels);
> > +
> > + ret = devm_iio_triggered_buffer_setup(&pdev->dev, indio_dev, NULL,
> > + mt6360_adc_trigger_handler, NULL);
> > + if (ret) {
> > + dev_err(&pdev->dev, "Failed to allocate iio trigger buffer\n");
> > + return ret;
> > + }
> > +
> > + ret = devm_iio_device_register(&pdev->dev, indio_dev);
> > + if (ret) {
> > + dev_err(&pdev->dev, "Failed to register iio device\n");
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> ...
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 1/2] iio: adc: mt6360: Add ADC driver for MT6360
2020-09-08 6:17 ` Gene Chen
@ 2020-09-08 7:43 ` Andy Shevchenko
2020-09-08 9:07 ` Jonathan Cameron
1 sibling, 0 replies; 15+ messages in thread
From: Andy Shevchenko @ 2020-09-08 7:43 UTC (permalink / raw)
To: Gene Chen
Cc: Jonathan Cameron, Matthias Brugger, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald, linux-iio,
linux-arm Mailing List, moderated list:ARM/Mediatek SoC support,
Linux Kernel Mailing List, Gene Chen, Wilma.Wu, shufan_lee,
cy_huang, benjamin.chao
On Tue, Sep 8, 2020 at 9:19 AM Gene Chen <gene.chen.richtek@gmail.com> wrote:
> Jonathan Cameron <jic23@kernel.org> 於 2020年8月30日 週日 上午1:12寫道：
> > On Mon, 24 Aug 2020 17:06:24 +0800
> > Gene Chen <gene.chen.richtek@gmail.com> wrote:
> > A few comments inline. The big one centres on why we can't
> > expose the channels as _raw, _offset and _scale?
> >
>
> I think i have 3 reason for use real value,
> ADC is used to get real value rather than raw data which is not meaningful.
> And I can decide which formula needs apply according to different condition.
> Also the junction temperature channel _scale is floating point 1.05
> which is not easy to express.
It's easy to express. Like other values in IIO subsystem which are float.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 1/2] iio: adc: mt6360: Add ADC driver for MT6360
2020-09-08 6:17 ` Gene Chen
2020-09-08 7:43 ` Andy Shevchenko
@ 2020-09-08 9:07 ` Jonathan Cameron
2020-09-08 11:08 ` Gene Chen
1 sibling, 1 reply; 15+ messages in thread
From: Jonathan Cameron @ 2020-09-08 9:07 UTC (permalink / raw)
To: Gene Chen
Cc: Jonathan Cameron, Matthias Brugger, knaack.h, lars, pmeerw,
linux-iio, linux-arm-kernel, linux-mediatek, linux-kernel,
Gene Chen, Wilma.Wu, shufan_lee, cy_huang, benjamin.chao
On Tue, 8 Sep 2020 14:17:42 +0800
Gene Chen <gene.chen.richtek@gmail.com> wrote:
> Jonathan Cameron <jic23@kernel.org> 於 2020年8月30日 週日 上午1:12寫道：
> >
> > On Mon, 24 Aug 2020 17:06:24 +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>
> > Hi Gene,
> >
> > A few comments inline. The big one centres on why we can't
> > expose the channels as _raw, _offset and _scale?
> >
>
> I think i have 3 reason for use real value,
> ADC is used to get real value rather than raw data which is not meaningful.
> And I can decide which formula needs apply according to different condition.
> Also the junction temperature channel _scale is floating point 1.05
> which is not easy to express.
See below.
>
> > Thanks,
> >
> > Jonathan
> >
> > > ---
> > > drivers/iio/adc/Kconfig | 11 ++
> > > drivers/iio/adc/Makefile | 1 +
> > > drivers/iio/adc/mt6360-adc.c | 366 +++++++++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 378 insertions(+)
> > > create mode 100644 drivers/iio/adc/mt6360-adc.c
> > >
> > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > > index 66d9cc0..07dcea7 100644
> > > --- a/drivers/iio/adc/Kconfig
> > > +++ b/drivers/iio/adc/Kconfig
> > > @@ -703,6 +703,17 @@ config MCP3911
> > > This driver can also be built as a module. If so, the module will be
> > > called mcp3911.
> > >
> > > +config MEDIATEK_MT6360_ADC
> > > + tristate "Mediatek MT6360 ADC Part"
> > > + depends on MFD_MT6360
> > > + select IIO_BUFFER
> > > + select IIO_TRIGGERED_BUFFER
> > > + help
> > > + Say Y here to enable MT6360 ADC Part.
> > > + Integrated for System Monitoring include
> > > + Charger and Battery Current, Voltage and
> > > + Temperature
> > > +
> > > config MEDIATEK_MT6577_AUXADC
> > > tristate "MediaTek AUXADC driver"
> > > depends on ARCH_MEDIATEK || COMPILE_TEST
> > > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > > index 90f94ad..5fca90a 100644
> > > --- a/drivers/iio/adc/Makefile
> > > +++ b/drivers/iio/adc/Makefile
> > > @@ -65,6 +65,7 @@ obj-$(CONFIG_MAX9611) += max9611.o
> > > obj-$(CONFIG_MCP320X) += mcp320x.o
> > > obj-$(CONFIG_MCP3422) += mcp3422.o
> > > obj-$(CONFIG_MCP3911) += mcp3911.o
> > > +obj-$(CONFIG_MEDIATEK_MT6360_ADC) += mt6360-adc.o
> > > obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o
> > > obj-$(CONFIG_MEN_Z188_ADC) += men_z188_adc.o
> > > obj-$(CONFIG_MESON_SARADC) += meson_saradc.o
> > > diff --git a/drivers/iio/adc/mt6360-adc.c b/drivers/iio/adc/mt6360-adc.c
> > > new file mode 100644
> > > index 0000000..5eed812
> > > --- /dev/null
> > > +++ b/drivers/iio/adc/mt6360-adc.c
> > > @@ -0,0 +1,366 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (c) 2020 MediaTek Inc.
> > > + *
> > > + * Author: Gene Chen <gene_chen@richtek.com>
> > > + */
> > > +
> > > +#include <linux/completion.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/iio/buffer.h>
> > > +#include <linux/iio/iio.h>
> > > +#include <linux/iio/trigger_consumer.h>
> > > +#include <linux/iio/triggered_buffer.h>
> > > +#include <linux/irq.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/ktime.h>
> > > +#include <linux/module.h>
> > > +#include <linux/mutex.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/regmap.h>
> > > +
> > > +#define MT6360_REG_PMUCHGCTRL3 0x313
> > > +#define MT6360_REG_PMUADCCFG 0x356
> > > +#define MT6360_REG_PMUADCRPT1 0x35A
> > > +
> > > +/* PMUCHGCTRL3 0x313 */
> > > +#define MT6360_AICR_MASK 0xFC
> > > +#define MT6360_AICR_SHFT 2
> > > +#define MT6360_AICR_400MA 0x6
> > > +/* PMUADCCFG 0x356 */
> > > +#define MT6360_ADCEN_MASK 0x8000
> > > +/* PMUADCRPT1 0x35A */
> > > +#define MT6360_PREFERCH_MASK 0xF0
> > > +#define MT6360_PREFERCH_SHFT 4
> > > +#define MT6360_RPTCH_MASK 0x0F
> > > +
> > > +enum {
> > > + MT6360_CHAN_USBID = 0,
> > > + MT6360_CHAN_VBUSDIV5,
> > > + MT6360_CHAN_VBUSDIV2,
> > > + MT6360_CHAN_VSYS,
> > > + MT6360_CHAN_VBAT,
> > > + MT6360_CHAN_IBUS,
> > > + MT6360_CHAN_IBAT,
> > > + MT6360_CHAN_CHG_VDDP,
> > > + MT6360_CHAN_TEMP_JC,
> > > + MT6360_CHAN_VREF_TS,
> > > + MT6360_CHAN_TS,
> > > + MT6360_CHAN_MAX,
> > > +};
> > > +
> > > +struct mt6360_adc_data {
> > > + struct device *dev;
> > > + struct regmap *regmap;
> > > + struct completion adc_complete;
> > > + struct mutex adc_lock;
> > > + ktime_t last_off_timestamps[MT6360_CHAN_MAX];
> > > + int irq;
> > > +};
> > > +
> > > +static inline int mt6360_adc_val_converter(int val, int multiplier, int offset, int divisor)
> > > +{
> > > + return ((val * multiplier) + offset) / divisor;
> >
> > Why could we not report these values to userspace or consumer drivers and let
> > them deal with the conversion if they actually needed it?
> > Mapping this to
> >
> > (val + new_offset) * multiplier would be a little messy, but not too bad.
> >
> > The advantage would be that we would then be providing the data needed
> > to get real units for values read from the buffers without having to
> > do all the maths in kernel (without access to floating point).
> >
> >
>
> As above, if I use formula "(val + new_offset) * multiplier",
> the junction temperature channel multiplier will be floating point
> 1.05, i don't know how to express.
As Andy mentioned, we do this all over the place.
IIO_VAL_INT_PLUS_MICRO
The key is that we want to push the burden of doing this maths to the user
not the source.
Often what is actually of interest is whether a temperature passed a threshold.
In that case, you can transform the threshold into the units of the ADC (so the
reverse directly to you would do with processed data) and only have to do the
maths once per change of the threshold instead of for every sample.
There are helper functions to do the maths for you, should you actually
need SI units.
>
> > > +}
> > > +
> > > +static int mt6360_adc_convert_processed_val(struct mt6360_adc_data *info, int chan_idx, int *val)
> > > +{
> > > + unsigned int regval = 0;
> > > + const struct converter {
> > > + int multiplier;
> > > + int offset;
> > > + int divisor;
> > > + } adc_converter[MT6360_CHAN_MAX] = {
> > > + { 1250, 0, 1}, /* USBID */
> > > + { 6250, 0, 1}, /* VBUSDIV5 */
> > > + { 2500, 0, 1}, /* VBUSDIV2 */
> > > + { 1250, 0, 1}, /* VSYS */
> > > + { 1250, 0, 1}, /* VBAT */
> > > + { 2500, 0, 1}, /* IBUS */
> > > + { 2500, 0, 1}, /* IBAT */
> > > + { 1250, 0, 1}, /* CHG_VDDP */
> > > + { 105, -8000, 100}, /* TEMP_JC */
> > > + { 1250, 0, 1}, /* VREF_TS */
> > > + { 1250, 0, 1}, /* TS */
> > > + }, sp_ibus_adc_converter = { 1900, 0, 1 }, *sel_converter;
> > > + int ret;
> > > +
> > > + sel_converter = adc_converter + chan_idx;
> > > + if (chan_idx == MT6360_CHAN_IBUS) {
> > > + /* ibus chan will be affected by aicr config */
> > > + /* if aicr < 400, apply the special ibus converter */
> > > + ret = regmap_read(info->regmap, MT6360_REG_PMUCHGCTRL3, ®val);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + regval = (regval & MT6360_AICR_MASK) >> MT6360_AICR_SHFT;
> > > + if (regval < MT6360_AICR_400MA)
> > > + sel_converter = &sp_ibus_adc_converter;
> > > + }
> > > +
> > > + *val = mt6360_adc_val_converter(*val, sel_converter->multiplier, sel_converter->offset,
> > > + sel_converter->divisor);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int mt6360_adc_read_processed(struct mt6360_adc_data *mad, int channel, int *val)
> > > +{
> > > + u16 adc_enable;
> > > + u8 rpt[3];
> > > + ktime_t start_t, predict_end_t;
> > > + long timeout;
> > > + int value, ret;
> > > +
> > > + mutex_lock(&mad->adc_lock);
> > > +
> > > + /* select preferred channel that we want */
> > > + ret = regmap_update_bits(mad->regmap, MT6360_REG_PMUADCRPT1, MT6360_PREFERCH_MASK,
> > > + channel << MT6360_PREFERCH_SHFT);
> > > + if (ret)
> > > + goto out_adc;
> > > +
> > > + /* enable adc channel we want and adc_en */
> > > + adc_enable = MT6360_ADCEN_MASK | BIT(channel);
> > > + adc_enable = cpu_to_be16(adc_enable);
> >
> > Use a local be16 to store that. It will make it a little clearer
> > that we are doing something 'unusual' here. Perhaps a comment on
> > why this odd code exists would also help?
> >
>
> ACK
>
> > > + ret = regmap_raw_write(mad->regmap, MT6360_REG_PMUADCCFG, (void *)&adc_enable, sizeof(u16));
> > > + if (ret)
> > > + goto out_adc;
> > > +
> > > + start_t = ktime_get();
> > > + predict_end_t = ktime_add_ms(mad->last_off_timestamps[channel], 50);
> > > +
> > > + if (ktime_after(start_t, predict_end_t))
> > > + predict_end_t = ktime_add_ms(start_t, 25);
> > > + else
> > > + predict_end_t = ktime_add_ms(start_t, 75);
> > > +
> > > + enable_irq(mad->irq);
> > > +adc_retry:
> > > + reinit_completion(&mad->adc_complete);
> > > +
> > > + /* wait for conversion to complete */
> > > + timeout = wait_for_completion_timeout(&mad->adc_complete, msecs_to_jiffies(200));
> > > + if (timeout == 0) {
> > > + ret = -ETIMEDOUT;
> > > + goto out_adc_conv;
> > > + } else if (timeout < 0) {
> > > + ret = -EINTR;
> > > + goto out_adc_conv;
> > > + }
> > > +
> > > + ret = regmap_raw_read(mad->regmap, MT6360_REG_PMUADCRPT1, rpt, sizeof(rpt));
> > > + if (ret)
> > > + goto out_adc_conv;
> > > +
> > > + /* check the current reported channel */
> > > + if ((rpt[0] & MT6360_RPTCH_MASK) != channel) {
> > > + dev_dbg(mad->dev, "not wanted channel report [%02x]\n", rpt[0]);
> >
> > This and the one below feel like error messages rather than debug ones.
> >
>
> We have two function "battery zero current voltage(ZCV)" and "TypeC
> OTP" will auto run ADC at background.
> ZCV_EN will run VBAT_ADC when TA plug in, TypeC OTP will run TS_ADC
> when TypeC attach.
> We need to check report channel for ADC report data match is our desire channel.
So there is firmware messing with it underneath? Oh goody.
Add a comment explaining this.
>
> > > + goto adc_retry;
> > > + }
> > > +
> > > + if (!ktime_after(ktime_get(), predict_end_t)) {
> > > + dev_dbg(mad->dev, "time is not after one adc_conv_t\n");
> >
> > Does this actually happen? If feels like we are being a bit over protective
> > here. I'd definitely like to see a comment saying why this protection
> > might be needed.
> >
>
> When ADC_EN and MT6360_CHANx_EN is enable, the channel x will keep
> running again and again
> I supposed to get immediate data which is generated after I start it.
Just to check my understanding.
This is an edge triggered interrupt and it triggers every time a new sample
is taken. Those samples are taken on a fixed frequency irrespective of whether
we have read the previous one?
>
> When I disable ADC_CHANx_EN, the H/W logical ADC is still running.
> If I run the same ADC immediately, I may get the old result about this channel.
> MT6360 ADC typical conversation time is about 25ms.
> So We need ignore which irq trigger below 25ms.
Normal trick for this sort of case is to just not use the interrupt.
Just read after 25+delta msecs and you are guaranteed to get the right answer.
>
> > > + goto adc_retry;
> > > + }
> > > +
> > > + value = (rpt[1] << 8) | rpt[2];
> > > +
> > > + ret = mt6360_adc_convert_processed_val(mad, channel, &value);
> > > + if (ret)
> > > + goto out_adc_conv;
> > > +
> > > + *val = value;
> > > + ret = IIO_VAL_INT;
> > > +
> > > +out_adc_conv:
> > > + disable_irq(mad->irq);
> > > + adc_enable = MT6360_ADCEN_MASK;
> > > + adc_enable = cpu_to_be16(adc_enable);
> > > + regmap_raw_write(mad->regmap, MT6360_REG_PMUADCCFG, (void *)&adc_enable, sizeof(u16));
> > > + mad->last_off_timestamps[channel] = ktime_get();
> > > + /* set prefer channel to 0xf */
> > > + regmap_update_bits(mad->regmap, MT6360_REG_PMUADCRPT1, MT6360_PREFERCH_MASK,
> > > + 0xF << MT6360_PREFERCH_SHFT);
> > > +out_adc:
> > > + mutex_unlock(&mad->adc_lock);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static int mt6360_adc_read_raw(struct iio_dev *iio_dev, const struct iio_chan_spec *chan,
> > > + int *val, int *val2, long mask)
> > > +{
> > > + struct mt6360_adc_data *mad = iio_priv(iio_dev);
> > > +
> > > + if (mask == IIO_CHAN_INFO_PROCESSED)
> > > + return mt6360_adc_read_processed(mad, chan->channel, val);
> > > +
> > > + return -EINVAL;
> > > +}
> > > +
> > > +static const struct iio_info mt6360_adc_iio_info = {
> > > + .read_raw = mt6360_adc_read_raw,
> > > +};
> > > +
> > > +#define MT6360_ADC_CHAN(_idx, _type) { \
> > > + .type = _type, \
> > > + .channel = MT6360_CHAN_##_idx, \
> > > + .scan_index = MT6360_CHAN_##_idx, \
> > > + .extend_name = #_idx, \
> > > + .datasheet_name = #_idx, \
> > > + .scan_type = { \
> > > + .sign = 's', \
> > > + .realbits = 32, \
> > > + .storagebits = 32, \
> > > + .endianness = IIO_CPU, \
> > > + }, \
> > > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
> > > + .indexed = 1, \
> > > +}
> > > +
> > > +static const struct iio_chan_spec mt6360_adc_channels[] = {
> > > + MT6360_ADC_CHAN(USBID, IIO_VOLTAGE),
> > > + MT6360_ADC_CHAN(VBUSDIV5, IIO_VOLTAGE),
> > > + MT6360_ADC_CHAN(VBUSDIV2, IIO_VOLTAGE),
> > > + MT6360_ADC_CHAN(VSYS, IIO_VOLTAGE),
> > > + MT6360_ADC_CHAN(VBAT, IIO_VOLTAGE),
> > > + MT6360_ADC_CHAN(IBUS, IIO_CURRENT),
> > > + MT6360_ADC_CHAN(IBAT, IIO_CURRENT),
> > > + MT6360_ADC_CHAN(CHG_VDDP, IIO_VOLTAGE),
> > > + MT6360_ADC_CHAN(TEMP_JC, IIO_TEMP),
> > > + MT6360_ADC_CHAN(VREF_TS, IIO_VOLTAGE),
> > > + MT6360_ADC_CHAN(TS, IIO_VOLTAGE),
> > > + IIO_CHAN_SOFT_TIMESTAMP(MT6360_CHAN_MAX),
> > > +};
> > > +
> > > +static irqreturn_t mt6360_pmu_adc_donei_handler(int irq, void *data)
> > > +{
> > > + struct mt6360_adc_data *mad = iio_priv(data);
> > > +
> > > + complete(&mad->adc_complete);
> > > + return IRQ_HANDLED;
> > > +}
> > > +
> > ...
> >
> > > +
> > > +static int mt6360_adc_probe(struct platform_device *pdev)
> > > +{
> > > + struct mt6360_adc_data *mad;
> > > + struct iio_dev *indio_dev;
> > > + int ret;
> > > +
> > > + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*mad));
> > > + if (!indio_dev)
> > > + return -ENOMEM;
> > > +
> > > + mad = iio_priv(indio_dev);
> > > + mad->dev = &pdev->dev;
> > > + init_completion(&mad->adc_complete);
> > > + mutex_init(&mad->adc_lock);
> > > +
> > > + mad->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> > > + if (!mad->regmap) {
> > > + dev_err(&pdev->dev, "Failed to get parent regmap\n");
> > > + return -ENODEV;
> > > + }
> > > +
> > > + ret = mt6360_adc_reset(mad);
> > > + if (ret < 0) {
> > > + dev_err(&pdev->dev, "Failed to reset adc\n");
> > > + return ret;
> > > + }
> > > +
> > > + mad->irq = platform_get_irq_byname(pdev, "adc_donei");
> > > + if (mad->irq < 0) {
> > > + dev_err(&pdev->dev, "Failed to get adc_done irq\n");
> > > + return mad->irq;
> > > + }
> > > +
> > > + irq_set_status_flags(mad->irq, IRQ_NOAUTOEN);
> >
> > As we are going to have a v5 anyway to clean up that endian warning,
> > please could you add a comment to explain the need for IRQ_NOAUTOEN?
> >
>
> Same as above "Enable ADC will run again and again until clear
> ADC__CHANx_EN bit"
> So After I get the ADC result, I disable irq in order to handle only
> oneshot data.
As mentioned a above I suspect you may be better off just not using
the interrupt at all.
>
> > > + ret = devm_request_threaded_irq(&pdev->dev, mad->irq, NULL, mt6360_pmu_adc_donei_handler, 0,
> > > + "adc_donei", indio_dev);
> > > + if (ret) {
> > > + dev_err(&pdev->dev, "Failed to register adc_done irq\n");
> > > + return ret;
> > > + }
> > > +
> > > + indio_dev->name = dev_name(&pdev->dev);
> > > + indio_dev->dev.parent = &pdev->dev;
> > > + indio_dev->info = &mt6360_adc_iio_info;
> > > + indio_dev->modes = INDIO_DIRECT_MODE;
> > > + indio_dev->channels = mt6360_adc_channels;
> > > + indio_dev->num_channels = ARRAY_SIZE(mt6360_adc_channels);
> > > +
> > > + ret = devm_iio_triggered_buffer_setup(&pdev->dev, indio_dev, NULL,
> > > + mt6360_adc_trigger_handler, NULL);
> > > + if (ret) {
> > > + dev_err(&pdev->dev, "Failed to allocate iio trigger buffer\n");
> > > + return ret;
> > > + }
> > > +
> > > + ret = devm_iio_device_register(&pdev->dev, indio_dev);
> > > + if (ret) {
> > > + dev_err(&pdev->dev, "Failed to register iio device\n");
> > > + return ret;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > ...
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 1/2] iio: adc: mt6360: Add ADC driver for MT6360
2020-09-08 9:07 ` Jonathan Cameron
@ 2020-09-08 11:08 ` Gene Chen
2020-09-08 12:58 ` Jonathan Cameron
0 siblings, 1 reply; 15+ messages in thread
From: Gene Chen @ 2020-09-08 11:08 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Jonathan Cameron, Matthias Brugger, knaack.h, lars, pmeerw,
linux-iio, linux-arm-kernel, linux-mediatek, linux-kernel,
Gene Chen, Wilma.Wu, shufan_lee, cy_huang, benjamin.chao
Jonathan Cameron <Jonathan.Cameron@huawei.com> 於 2020年9月8日 週二 下午5:08寫道：
>
> On Tue, 8 Sep 2020 14:17:42 +0800
> Gene Chen <gene.chen.richtek@gmail.com> wrote:
>
> > Jonathan Cameron <jic23@kernel.org> 於 2020年8月30日 週日 上午1:12寫道：
> > >
> > > On Mon, 24 Aug 2020 17:06:24 +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>
> > > Hi Gene,
> > >
> > > A few comments inline. The big one centres on why we can't
> > > expose the channels as _raw, _offset and _scale?
> > >
> >
> > I think i have 3 reason for use real value,
> > ADC is used to get real value rather than raw data which is not meaningful.
> > And I can decide which formula needs apply according to different condition.
> > Also the junction temperature channel _scale is floating point 1.05
> > which is not easy to express.
>
> See below.
>
> >
> > > Thanks,
> > >
> > > Jonathan
> > >
> > > > ---
> > > > drivers/iio/adc/Kconfig | 11 ++
> > > > drivers/iio/adc/Makefile | 1 +
> > > > drivers/iio/adc/mt6360-adc.c | 366 +++++++++++++++++++++++++++++++++++++++++++
> > > > 3 files changed, 378 insertions(+)
> > > > create mode 100644 drivers/iio/adc/mt6360-adc.c
> > > >
> > > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > > > index 66d9cc0..07dcea7 100644
> > > > --- a/drivers/iio/adc/Kconfig
> > > > +++ b/drivers/iio/adc/Kconfig
> > > > @@ -703,6 +703,17 @@ config MCP3911
> > > > This driver can also be built as a module. If so, the module will be
> > > > called mcp3911.
> > > >
> > > > +config MEDIATEK_MT6360_ADC
> > > > + tristate "Mediatek MT6360 ADC Part"
> > > > + depends on MFD_MT6360
> > > > + select IIO_BUFFER
> > > > + select IIO_TRIGGERED_BUFFER
> > > > + help
> > > > + Say Y here to enable MT6360 ADC Part.
> > > > + Integrated for System Monitoring include
> > > > + Charger and Battery Current, Voltage and
> > > > + Temperature
> > > > +
> > > > config MEDIATEK_MT6577_AUXADC
> > > > tristate "MediaTek AUXADC driver"
> > > > depends on ARCH_MEDIATEK || COMPILE_TEST
> > > > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > > > index 90f94ad..5fca90a 100644
> > > > --- a/drivers/iio/adc/Makefile
> > > > +++ b/drivers/iio/adc/Makefile
> > > > @@ -65,6 +65,7 @@ obj-$(CONFIG_MAX9611) += max9611.o
> > > > obj-$(CONFIG_MCP320X) += mcp320x.o
> > > > obj-$(CONFIG_MCP3422) += mcp3422.o
> > > > obj-$(CONFIG_MCP3911) += mcp3911.o
> > > > +obj-$(CONFIG_MEDIATEK_MT6360_ADC) += mt6360-adc.o
> > > > obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o
> > > > obj-$(CONFIG_MEN_Z188_ADC) += men_z188_adc.o
> > > > obj-$(CONFIG_MESON_SARADC) += meson_saradc.o
> > > > diff --git a/drivers/iio/adc/mt6360-adc.c b/drivers/iio/adc/mt6360-adc.c
> > > > new file mode 100644
> > > > index 0000000..5eed812
> > > > --- /dev/null
> > > > +++ b/drivers/iio/adc/mt6360-adc.c
> > > > @@ -0,0 +1,366 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * Copyright (c) 2020 MediaTek Inc.
> > > > + *
> > > > + * Author: Gene Chen <gene_chen@richtek.com>
> > > > + */
> > > > +
> > > > +#include <linux/completion.h>
> > > > +#include <linux/interrupt.h>
> > > > +#include <linux/iio/buffer.h>
> > > > +#include <linux/iio/iio.h>
> > > > +#include <linux/iio/trigger_consumer.h>
> > > > +#include <linux/iio/triggered_buffer.h>
> > > > +#include <linux/irq.h>
> > > > +#include <linux/kernel.h>
> > > > +#include <linux/ktime.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/mutex.h>
> > > > +#include <linux/platform_device.h>
> > > > +#include <linux/regmap.h>
> > > > +
> > > > +#define MT6360_REG_PMUCHGCTRL3 0x313
> > > > +#define MT6360_REG_PMUADCCFG 0x356
> > > > +#define MT6360_REG_PMUADCRPT1 0x35A
> > > > +
> > > > +/* PMUCHGCTRL3 0x313 */
> > > > +#define MT6360_AICR_MASK 0xFC
> > > > +#define MT6360_AICR_SHFT 2
> > > > +#define MT6360_AICR_400MA 0x6
> > > > +/* PMUADCCFG 0x356 */
> > > > +#define MT6360_ADCEN_MASK 0x8000
> > > > +/* PMUADCRPT1 0x35A */
> > > > +#define MT6360_PREFERCH_MASK 0xF0
> > > > +#define MT6360_PREFERCH_SHFT 4
> > > > +#define MT6360_RPTCH_MASK 0x0F
> > > > +
> > > > +enum {
> > > > + MT6360_CHAN_USBID = 0,
> > > > + MT6360_CHAN_VBUSDIV5,
> > > > + MT6360_CHAN_VBUSDIV2,
> > > > + MT6360_CHAN_VSYS,
> > > > + MT6360_CHAN_VBAT,
> > > > + MT6360_CHAN_IBUS,
> > > > + MT6360_CHAN_IBAT,
> > > > + MT6360_CHAN_CHG_VDDP,
> > > > + MT6360_CHAN_TEMP_JC,
> > > > + MT6360_CHAN_VREF_TS,
> > > > + MT6360_CHAN_TS,
> > > > + MT6360_CHAN_MAX,
> > > > +};
> > > > +
> > > > +struct mt6360_adc_data {
> > > > + struct device *dev;
> > > > + struct regmap *regmap;
> > > > + struct completion adc_complete;
> > > > + struct mutex adc_lock;
> > > > + ktime_t last_off_timestamps[MT6360_CHAN_MAX];
> > > > + int irq;
> > > > +};
> > > > +
> > > > +static inline int mt6360_adc_val_converter(int val, int multiplier, int offset, int divisor)
> > > > +{
> > > > + return ((val * multiplier) + offset) / divisor;
> > >
> > > Why could we not report these values to userspace or consumer drivers and let
> > > them deal with the conversion if they actually needed it?
> > > Mapping this to
> > >
> > > (val + new_offset) * multiplier would be a little messy, but not too bad.
> > >
> > > The advantage would be that we would then be providing the data needed
> > > to get real units for values read from the buffers without having to
> > > do all the maths in kernel (without access to floating point).
> > >
> > >
> >
> > As above, if I use formula "(val + new_offset) * multiplier",
> > the junction temperature channel multiplier will be floating point
> > 1.05, i don't know how to express.
>
> As Andy mentioned, we do this all over the place.
> IIO_VAL_INT_PLUS_MICRO
>
> The key is that we want to push the burden of doing this maths to the user
> not the source.
ACK.
Can I keep IIO_CHAN_INFO_PROCESSED function be reserved for user in
kernel space?
>
> Often what is actually of interest is whether a temperature passed a threshold.
> In that case, you can transform the threshold into the units of the ADC (so the
> reverse directly to you would do with processed data) and only have to do the
> maths once per change of the threshold instead of for every sample.
>
> There are helper functions to do the maths for you, should you actually
> need SI units.
>
ACK
> >
> > > > +}
> > > > +
> > > > +static int mt6360_adc_convert_processed_val(struct mt6360_adc_data *info, int chan_idx, int *val)
> > > > +{
> > > > + unsigned int regval = 0;
> > > > + const struct converter {
> > > > + int multiplier;
> > > > + int offset;
> > > > + int divisor;
> > > > + } adc_converter[MT6360_CHAN_MAX] = {
> > > > + { 1250, 0, 1}, /* USBID */
> > > > + { 6250, 0, 1}, /* VBUSDIV5 */
> > > > + { 2500, 0, 1}, /* VBUSDIV2 */
> > > > + { 1250, 0, 1}, /* VSYS */
> > > > + { 1250, 0, 1}, /* VBAT */
> > > > + { 2500, 0, 1}, /* IBUS */
> > > > + { 2500, 0, 1}, /* IBAT */
> > > > + { 1250, 0, 1}, /* CHG_VDDP */
> > > > + { 105, -8000, 100}, /* TEMP_JC */
> > > > + { 1250, 0, 1}, /* VREF_TS */
> > > > + { 1250, 0, 1}, /* TS */
> > > > + }, sp_ibus_adc_converter = { 1900, 0, 1 }, *sel_converter;
> > > > + int ret;
> > > > +
> > > > + sel_converter = adc_converter + chan_idx;
> > > > + if (chan_idx == MT6360_CHAN_IBUS) {
> > > > + /* ibus chan will be affected by aicr config */
> > > > + /* if aicr < 400, apply the special ibus converter */
> > > > + ret = regmap_read(info->regmap, MT6360_REG_PMUCHGCTRL3, ®val);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + regval = (regval & MT6360_AICR_MASK) >> MT6360_AICR_SHFT;
> > > > + if (regval < MT6360_AICR_400MA)
> > > > + sel_converter = &sp_ibus_adc_converter;
> > > > + }
> > > > +
> > > > + *val = mt6360_adc_val_converter(*val, sel_converter->multiplier, sel_converter->offset,
> > > > + sel_converter->divisor);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int mt6360_adc_read_processed(struct mt6360_adc_data *mad, int channel, int *val)
> > > > +{
> > > > + u16 adc_enable;
> > > > + u8 rpt[3];
> > > > + ktime_t start_t, predict_end_t;
> > > > + long timeout;
> > > > + int value, ret;
> > > > +
> > > > + mutex_lock(&mad->adc_lock);
> > > > +
> > > > + /* select preferred channel that we want */
> > > > + ret = regmap_update_bits(mad->regmap, MT6360_REG_PMUADCRPT1, MT6360_PREFERCH_MASK,
> > > > + channel << MT6360_PREFERCH_SHFT);
> > > > + if (ret)
> > > > + goto out_adc;
> > > > +
> > > > + /* enable adc channel we want and adc_en */
> > > > + adc_enable = MT6360_ADCEN_MASK | BIT(channel);
> > > > + adc_enable = cpu_to_be16(adc_enable);
> > >
> > > Use a local be16 to store that. It will make it a little clearer
> > > that we are doing something 'unusual' here. Perhaps a comment on
> > > why this odd code exists would also help?
> > >
> >
> > ACK
> >
> > > > + ret = regmap_raw_write(mad->regmap, MT6360_REG_PMUADCCFG, (void *)&adc_enable, sizeof(u16));
> > > > + if (ret)
> > > > + goto out_adc;
> > > > +
> > > > + start_t = ktime_get();
> > > > + predict_end_t = ktime_add_ms(mad->last_off_timestamps[channel], 50);
> > > > +
> > > > + if (ktime_after(start_t, predict_end_t))
> > > > + predict_end_t = ktime_add_ms(start_t, 25);
> > > > + else
> > > > + predict_end_t = ktime_add_ms(start_t, 75);
> > > > +
> > > > + enable_irq(mad->irq);
> > > > +adc_retry:
> > > > + reinit_completion(&mad->adc_complete);
> > > > +
> > > > + /* wait for conversion to complete */
> > > > + timeout = wait_for_completion_timeout(&mad->adc_complete, msecs_to_jiffies(200));
> > > > + if (timeout == 0) {
> > > > + ret = -ETIMEDOUT;
> > > > + goto out_adc_conv;
> > > > + } else if (timeout < 0) {
> > > > + ret = -EINTR;
> > > > + goto out_adc_conv;
> > > > + }
> > > > +
> > > > + ret = regmap_raw_read(mad->regmap, MT6360_REG_PMUADCRPT1, rpt, sizeof(rpt));
> > > > + if (ret)
> > > > + goto out_adc_conv;
> > > > +
> > > > + /* check the current reported channel */
> > > > + if ((rpt[0] & MT6360_RPTCH_MASK) != channel) {
> > > > + dev_dbg(mad->dev, "not wanted channel report [%02x]\n", rpt[0]);
> > >
> > > This and the one below feel like error messages rather than debug ones.
> > >
> >
> > We have two function "battery zero current voltage(ZCV)" and "TypeC
> > OTP" will auto run ADC at background.
> > ZCV_EN will run VBAT_ADC when TA plug in, TypeC OTP will run TS_ADC
> > when TypeC attach.
> > We need to check report channel for ADC report data match is our desire channel.
>
> So there is firmware messing with it underneath? Oh goody.
> Add a comment explaining this.
>
ACK, I try to write a comment as below
/*
* 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 need 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.
*/
> >
> > > > + goto adc_retry;
> > > > + }
> > > > +
> > > > + if (!ktime_after(ktime_get(), predict_end_t)) {
> > > > + dev_dbg(mad->dev, "time is not after one adc_conv_t\n");
> > >
> > > Does this actually happen? If feels like we are being a bit over protective
> > > here. I'd definitely like to see a comment saying why this protection
> > > might be needed.
> > >
> >
> > When ADC_EN and MT6360_CHANx_EN is enable, the channel x will keep
> > running again and again
> > I supposed to get immediate data which is generated after I start it.
>
> Just to check my understanding.
>
> This is an edge triggered interrupt and it triggers every time a new sample
> is taken. Those samples are taken on a fixed frequency irrespective of whether
> we have read the previous one?
>
Yes.
I use LEVEL_LOW trigger in latest review MFD patch.
> >
> > When I disable ADC_CHANx_EN, the H/W logical ADC is still running.
> > If I run the same ADC immediately, I may get the old result about this channel.
> > MT6360 ADC typical conversation time is about 25ms.
> > So We need ignore which irq trigger below 25ms.
>
> Normal trick for this sort of case is to just not use the interrupt.
> Just read after 25+delta msecs and you are guaranteed to get the right answer.
>
>
ACK, I will try to use polling
Is the pseudocode correct?
mdelay(predict_end_t);
while (true) {
read adc event is occured
check report channel is the same
if the same, read report ADC data and break while loop
else msleep(per ADC conversion time)
}
> >
> > > > + goto adc_retry;
> > > > + }
> > > > +
> > > > + value = (rpt[1] << 8) | rpt[2];
> > > > +
> > > > + ret = mt6360_adc_convert_processed_val(mad, channel, &value);
> > > > + if (ret)
> > > > + goto out_adc_conv;
> > > > +
> > > > + *val = value;
> > > > + ret = IIO_VAL_INT;
> > > > +
> > > > +out_adc_conv:
> > > > + disable_irq(mad->irq);
> > > > + adc_enable = MT6360_ADCEN_MASK;
> > > > + adc_enable = cpu_to_be16(adc_enable);
> > > > + regmap_raw_write(mad->regmap, MT6360_REG_PMUADCCFG, (void *)&adc_enable, sizeof(u16));
> > > > + mad->last_off_timestamps[channel] = ktime_get();
> > > > + /* set prefer channel to 0xf */
> > > > + regmap_update_bits(mad->regmap, MT6360_REG_PMUADCRPT1, MT6360_PREFERCH_MASK,
> > > > + 0xF << MT6360_PREFERCH_SHFT);
> > > > +out_adc:
> > > > + mutex_unlock(&mad->adc_lock);
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static int mt6360_adc_read_raw(struct iio_dev *iio_dev, const struct iio_chan_spec *chan,
> > > > + int *val, int *val2, long mask)
> > > > +{
> > > > + struct mt6360_adc_data *mad = iio_priv(iio_dev);
> > > > +
> > > > + if (mask == IIO_CHAN_INFO_PROCESSED)
> > > > + return mt6360_adc_read_processed(mad, chan->channel, val);
> > > > +
> > > > + return -EINVAL;
> > > > +}
> > > > +
> > > > +static const struct iio_info mt6360_adc_iio_info = {
> > > > + .read_raw = mt6360_adc_read_raw,
> > > > +};
> > > > +
> > > > +#define MT6360_ADC_CHAN(_idx, _type) { \
> > > > + .type = _type, \
> > > > + .channel = MT6360_CHAN_##_idx, \
> > > > + .scan_index = MT6360_CHAN_##_idx, \
> > > > + .extend_name = #_idx, \
> > > > + .datasheet_name = #_idx, \
> > > > + .scan_type = { \
> > > > + .sign = 's', \
> > > > + .realbits = 32, \
> > > > + .storagebits = 32, \
> > > > + .endianness = IIO_CPU, \
> > > > + }, \
> > > > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
> > > > + .indexed = 1, \
> > > > +}
> > > > +
> > > > +static const struct iio_chan_spec mt6360_adc_channels[] = {
> > > > + MT6360_ADC_CHAN(USBID, IIO_VOLTAGE),
> > > > + MT6360_ADC_CHAN(VBUSDIV5, IIO_VOLTAGE),
> > > > + MT6360_ADC_CHAN(VBUSDIV2, IIO_VOLTAGE),
> > > > + MT6360_ADC_CHAN(VSYS, IIO_VOLTAGE),
> > > > + MT6360_ADC_CHAN(VBAT, IIO_VOLTAGE),
> > > > + MT6360_ADC_CHAN(IBUS, IIO_CURRENT),
> > > > + MT6360_ADC_CHAN(IBAT, IIO_CURRENT),
> > > > + MT6360_ADC_CHAN(CHG_VDDP, IIO_VOLTAGE),
> > > > + MT6360_ADC_CHAN(TEMP_JC, IIO_TEMP),
> > > > + MT6360_ADC_CHAN(VREF_TS, IIO_VOLTAGE),
> > > > + MT6360_ADC_CHAN(TS, IIO_VOLTAGE),
> > > > + IIO_CHAN_SOFT_TIMESTAMP(MT6360_CHAN_MAX),
> > > > +};
> > > > +
> > > > +static irqreturn_t mt6360_pmu_adc_donei_handler(int irq, void *data)
> > > > +{
> > > > + struct mt6360_adc_data *mad = iio_priv(data);
> > > > +
> > > > + complete(&mad->adc_complete);
> > > > + return IRQ_HANDLED;
> > > > +}
> > > > +
> > > ...
> > >
> > > > +
> > > > +static int mt6360_adc_probe(struct platform_device *pdev)
> > > > +{
> > > > + struct mt6360_adc_data *mad;
> > > > + struct iio_dev *indio_dev;
> > > > + int ret;
> > > > +
> > > > + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*mad));
> > > > + if (!indio_dev)
> > > > + return -ENOMEM;
> > > > +
> > > > + mad = iio_priv(indio_dev);
> > > > + mad->dev = &pdev->dev;
> > > > + init_completion(&mad->adc_complete);
> > > > + mutex_init(&mad->adc_lock);
> > > > +
> > > > + mad->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> > > > + if (!mad->regmap) {
> > > > + dev_err(&pdev->dev, "Failed to get parent regmap\n");
> > > > + return -ENODEV;
> > > > + }
> > > > +
> > > > + ret = mt6360_adc_reset(mad);
> > > > + if (ret < 0) {
> > > > + dev_err(&pdev->dev, "Failed to reset adc\n");
> > > > + return ret;
> > > > + }
> > > > +
> > > > + mad->irq = platform_get_irq_byname(pdev, "adc_donei");
> > > > + if (mad->irq < 0) {
> > > > + dev_err(&pdev->dev, "Failed to get adc_done irq\n");
> > > > + return mad->irq;
> > > > + }
> > > > +
> > > > + irq_set_status_flags(mad->irq, IRQ_NOAUTOEN);
> > >
> > > As we are going to have a v5 anyway to clean up that endian warning,
> > > please could you add a comment to explain the need for IRQ_NOAUTOEN?
> > >
> >
> > Same as above "Enable ADC will run again and again until clear
> > ADC__CHANx_EN bit"
> > So After I get the ADC result, I disable irq in order to handle only
> > oneshot data.
>
> As mentioned a above I suspect you may be better off just not using
> the interrupt at all.
>
ACK, I try to fixed by polling ADC event.
> >
> > > > + ret = devm_request_threaded_irq(&pdev->dev, mad->irq, NULL, mt6360_pmu_adc_donei_handler, 0,
> > > > + "adc_donei", indio_dev);
> > > > + if (ret) {
> > > > + dev_err(&pdev->dev, "Failed to register adc_done irq\n");
> > > > + return ret;
> > > > + }
> > > > +
> > > > + indio_dev->name = dev_name(&pdev->dev);
> > > > + indio_dev->dev.parent = &pdev->dev;
> > > > + indio_dev->info = &mt6360_adc_iio_info;
> > > > + indio_dev->modes = INDIO_DIRECT_MODE;
> > > > + indio_dev->channels = mt6360_adc_channels;
> > > > + indio_dev->num_channels = ARRAY_SIZE(mt6360_adc_channels);
> > > > +
> > > > + ret = devm_iio_triggered_buffer_setup(&pdev->dev, indio_dev, NULL,
> > > > + mt6360_adc_trigger_handler, NULL);
> > > > + if (ret) {
> > > > + dev_err(&pdev->dev, "Failed to allocate iio trigger buffer\n");
> > > > + return ret;
> > > > + }
> > > > +
> > > > + ret = devm_iio_device_register(&pdev->dev, indio_dev);
> > > > + if (ret) {
> > > > + dev_err(&pdev->dev, "Failed to register iio device\n");
> > > > + return ret;
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > ...
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 1/2] iio: adc: mt6360: Add ADC driver for MT6360
2020-09-08 11:08 ` Gene Chen
@ 2020-09-08 12:58 ` Jonathan Cameron
2020-09-08 23:39 ` Gene Chen
0 siblings, 1 reply; 15+ messages in thread
From: Jonathan Cameron @ 2020-09-08 12:58 UTC (permalink / raw)
To: Gene Chen
Cc: Jonathan Cameron, Matthias Brugger, knaack.h, lars, pmeerw,
linux-iio, linux-arm-kernel, linux-mediatek, linux-kernel,
Gene Chen, Wilma.Wu, shufan_lee, cy_huang, benjamin.chao
...
> > > > > +#include <linux/completion.h>
> > > > > +#include <linux/interrupt.h>
> > > > > +#include <linux/iio/buffer.h>
> > > > > +#include <linux/iio/iio.h>
> > > > > +#include <linux/iio/trigger_consumer.h>
> > > > > +#include <linux/iio/triggered_buffer.h>
> > > > > +#include <linux/irq.h>
> > > > > +#include <linux/kernel.h>
> > > > > +#include <linux/ktime.h>
> > > > > +#include <linux/module.h>
> > > > > +#include <linux/mutex.h>
> > > > > +#include <linux/platform_device.h>
> > > > > +#include <linux/regmap.h>
> > > > > +
> > > > > +#define MT6360_REG_PMUCHGCTRL3 0x313
> > > > > +#define MT6360_REG_PMUADCCFG 0x356
> > > > > +#define MT6360_REG_PMUADCRPT1 0x35A
> > > > > +
> > > > > +/* PMUCHGCTRL3 0x313 */
> > > > > +#define MT6360_AICR_MASK 0xFC
> > > > > +#define MT6360_AICR_SHFT 2
> > > > > +#define MT6360_AICR_400MA 0x6
> > > > > +/* PMUADCCFG 0x356 */
> > > > > +#define MT6360_ADCEN_MASK 0x8000
> > > > > +/* PMUADCRPT1 0x35A */
> > > > > +#define MT6360_PREFERCH_MASK 0xF0
> > > > > +#define MT6360_PREFERCH_SHFT 4
> > > > > +#define MT6360_RPTCH_MASK 0x0F
> > > > > +
> > > > > +enum {
> > > > > + MT6360_CHAN_USBID = 0,
> > > > > + MT6360_CHAN_VBUSDIV5,
> > > > > + MT6360_CHAN_VBUSDIV2,
> > > > > + MT6360_CHAN_VSYS,
> > > > > + MT6360_CHAN_VBAT,
> > > > > + MT6360_CHAN_IBUS,
> > > > > + MT6360_CHAN_IBAT,
> > > > > + MT6360_CHAN_CHG_VDDP,
> > > > > + MT6360_CHAN_TEMP_JC,
> > > > > + MT6360_CHAN_VREF_TS,
> > > > > + MT6360_CHAN_TS,
> > > > > + MT6360_CHAN_MAX,
> > > > > +};
> > > > > +
> > > > > +struct mt6360_adc_data {
> > > > > + struct device *dev;
> > > > > + struct regmap *regmap;
> > > > > + struct completion adc_complete;
> > > > > + struct mutex adc_lock;
> > > > > + ktime_t last_off_timestamps[MT6360_CHAN_MAX];
> > > > > + int irq;
> > > > > +};
> > > > > +
> > > > > +static inline int mt6360_adc_val_converter(int val, int multiplier, int offset, int divisor)
> > > > > +{
> > > > > + return ((val * multiplier) + offset) / divisor;
> > > >
> > > > Why could we not report these values to userspace or consumer drivers and let
> > > > them deal with the conversion if they actually needed it?
> > > > Mapping this to
> > > >
> > > > (val + new_offset) * multiplier would be a little messy, but not too bad.
> > > >
> > > > The advantage would be that we would then be providing the data needed
> > > > to get real units for values read from the buffers without having to
> > > > do all the maths in kernel (without access to floating point).
> > > >
> > > >
> > >
> > > As above, if I use formula "(val + new_offset) * multiplier",
> > > the junction temperature channel multiplier will be floating point
> > > 1.05, i don't know how to express.
> >
> > As Andy mentioned, we do this all over the place.
> > IIO_VAL_INT_PLUS_MICRO
> >
> > The key is that we want to push the burden of doing this maths to the user
> > not the source.
>
> ACK.
> Can I keep IIO_CHAN_INFO_PROCESSED function be reserved for user in
> kernel space?
>
No. We have utility functions that will apply the multiplier as needed so
there is no significant advantage in doing this and it won't be consistent
with the majority of other drivers.
> >
> > Often what is actually of interest is whether a temperature passed a threshold.
> > In that case, you can transform the threshold into the units of the ADC (so the
> > reverse directly to you would do with processed data) and only have to do the
> > maths once per change of the threshold instead of for every sample.
> >
> > There are helper functions to do the maths for you, should you actually
> > need SI units.
> >
>
> ACK
>
> > >
> > > > > +}
> > > > > +
> > > > > +static int mt6360_adc_convert_processed_val(struct mt6360_adc_data *info, int chan_idx, int *val)
> > > > > +{
> > > > > + unsigned int regval = 0;
> > > > > + const struct converter {
> > > > > + int multiplier;
> > > > > + int offset;
> > > > > + int divisor;
> > > > > + } adc_converter[MT6360_CHAN_MAX] = {
> > > > > + { 1250, 0, 1}, /* USBID */
> > > > > + { 6250, 0, 1}, /* VBUSDIV5 */
> > > > > + { 2500, 0, 1}, /* VBUSDIV2 */
> > > > > + { 1250, 0, 1}, /* VSYS */
> > > > > + { 1250, 0, 1}, /* VBAT */
> > > > > + { 2500, 0, 1}, /* IBUS */
> > > > > + { 2500, 0, 1}, /* IBAT */
> > > > > + { 1250, 0, 1}, /* CHG_VDDP */
> > > > > + { 105, -8000, 100}, /* TEMP_JC */
> > > > > + { 1250, 0, 1}, /* VREF_TS */
> > > > > + { 1250, 0, 1}, /* TS */
> > > > > + }, sp_ibus_adc_converter = { 1900, 0, 1 }, *sel_converter;
> > > > > + int ret;
> > > > > +
> > > > > + sel_converter = adc_converter + chan_idx;
> > > > > + if (chan_idx == MT6360_CHAN_IBUS) {
> > > > > + /* ibus chan will be affected by aicr config */
> > > > > + /* if aicr < 400, apply the special ibus converter */
> > > > > + ret = regmap_read(info->regmap, MT6360_REG_PMUCHGCTRL3, ®val);
> > > > > + if (ret)
> > > > > + return ret;
> > > > > +
> > > > > + regval = (regval & MT6360_AICR_MASK) >> MT6360_AICR_SHFT;
> > > > > + if (regval < MT6360_AICR_400MA)
> > > > > + sel_converter = &sp_ibus_adc_converter;
> > > > > + }
> > > > > +
> > > > > + *val = mt6360_adc_val_converter(*val, sel_converter->multiplier, sel_converter->offset,
> > > > > + sel_converter->divisor);
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > +static int mt6360_adc_read_processed(struct mt6360_adc_data *mad, int channel, int *val)
> > > > > +{
> > > > > + u16 adc_enable;
> > > > > + u8 rpt[3];
> > > > > + ktime_t start_t, predict_end_t;
> > > > > + long timeout;
> > > > > + int value, ret;
> > > > > +
> > > > > + mutex_lock(&mad->adc_lock);
> > > > > +
> > > > > + /* select preferred channel that we want */
> > > > > + ret = regmap_update_bits(mad->regmap, MT6360_REG_PMUADCRPT1, MT6360_PREFERCH_MASK,
> > > > > + channel << MT6360_PREFERCH_SHFT);
> > > > > + if (ret)
> > > > > + goto out_adc;
> > > > > +
> > > > > + /* enable adc channel we want and adc_en */
> > > > > + adc_enable = MT6360_ADCEN_MASK | BIT(channel);
> > > > > + adc_enable = cpu_to_be16(adc_enable);
> > > >
> > > > Use a local be16 to store that. It will make it a little clearer
> > > > that we are doing something 'unusual' here. Perhaps a comment on
> > > > why this odd code exists would also help?
> > > >
> > >
> > > ACK
> > >
> > > > > + ret = regmap_raw_write(mad->regmap, MT6360_REG_PMUADCCFG, (void *)&adc_enable, sizeof(u16));
> > > > > + if (ret)
> > > > > + goto out_adc;
> > > > > +
> > > > > + start_t = ktime_get();
> > > > > + predict_end_t = ktime_add_ms(mad->last_off_timestamps[channel], 50);
> > > > > +
> > > > > + if (ktime_after(start_t, predict_end_t))
> > > > > + predict_end_t = ktime_add_ms(start_t, 25);
> > > > > + else
> > > > > + predict_end_t = ktime_add_ms(start_t, 75);
> > > > > +
> > > > > + enable_irq(mad->irq);
> > > > > +adc_retry:
> > > > > + reinit_completion(&mad->adc_complete);
> > > > > +
> > > > > + /* wait for conversion to complete */
> > > > > + timeout = wait_for_completion_timeout(&mad->adc_complete, msecs_to_jiffies(200));
> > > > > + if (timeout == 0) {
> > > > > + ret = -ETIMEDOUT;
> > > > > + goto out_adc_conv;
> > > > > + } else if (timeout < 0) {
> > > > > + ret = -EINTR;
> > > > > + goto out_adc_conv;
> > > > > + }
> > > > > +
> > > > > + ret = regmap_raw_read(mad->regmap, MT6360_REG_PMUADCRPT1, rpt, sizeof(rpt));
> > > > > + if (ret)
> > > > > + goto out_adc_conv;
> > > > > +
> > > > > + /* check the current reported channel */
> > > > > + if ((rpt[0] & MT6360_RPTCH_MASK) != channel) {
> > > > > + dev_dbg(mad->dev, "not wanted channel report [%02x]\n", rpt[0]);
> > > >
> > > > This and the one below feel like error messages rather than debug ones.
> > > >
> > >
> > > We have two function "battery zero current voltage(ZCV)" and "TypeC
> > > OTP" will auto run ADC at background.
> > > ZCV_EN will run VBAT_ADC when TA plug in, TypeC OTP will run TS_ADC
> > > when TypeC attach.
> > > We need to check report channel for ADC report data match is our desire channel.
> >
> > So there is firmware messing with it underneath? Oh goody.
> > Add a comment explaining this.
> >
>
> ACK, I try to write a comment as below
>
> /*
> * 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 need 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.
> */
Looks good.
>
> > >
> > > > > + goto adc_retry;
> > > > > + }
> > > > > +
> > > > > + if (!ktime_after(ktime_get(), predict_end_t)) {
> > > > > + dev_dbg(mad->dev, "time is not after one adc_conv_t\n");
> > > >
> > > > Does this actually happen? If feels like we are being a bit over protective
> > > > here. I'd definitely like to see a comment saying why this protection
> > > > might be needed.
> > > >
> > >
> > > When ADC_EN and MT6360_CHANx_EN is enable, the channel x will keep
> > > running again and again
> > > I supposed to get immediate data which is generated after I start it.
> >
> > Just to check my understanding.
> >
> > This is an edge triggered interrupt and it triggers every time a new sample
> > is taken. Those samples are taken on a fixed frequency irrespective of whether
> > we have read the previous one?
> >
>
> Yes.
> I use LEVEL_LOW trigger in latest review MFD patch.
I'm not sure I follow that comment. How can you do that if it's a repeating
edge trigger?
>
> > >
> > > When I disable ADC_CHANx_EN, the H/W logical ADC is still running.
> > > If I run the same ADC immediately, I may get the old result about this channel.
> > > MT6360 ADC typical conversation time is about 25ms.
> > > So We need ignore which irq trigger below 25ms.
> >
> > Normal trick for this sort of case is to just not use the interrupt.
> > Just read after 25+delta msecs and you are guaranteed to get the right answer.
> >
> >
>
> ACK, I will try to use polling
> Is the pseudocode correct?
>
> mdelay(predict_end_t);
> while (true) {
> read adc event is occured
> check report channel is the same
> if the same, read report ADC data and break while loop
> else msleep(per ADC conversion time)
> }
Looks correct to me. We should 'know' the event has happened but
still need to check the channel is the expected one.
...
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 1/2] iio: adc: mt6360: Add ADC driver for MT6360
2020-09-08 12:58 ` Jonathan Cameron
@ 2020-09-08 23:39 ` Gene Chen
2020-09-09 9:34 ` Jonathan Cameron
2020-09-10 0:14 ` Gene Chen
0 siblings, 2 replies; 15+ messages in thread
From: Gene Chen @ 2020-09-08 23:39 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Jonathan Cameron, Matthias Brugger, knaack.h, lars, pmeerw,
linux-iio, linux-arm-kernel, linux-mediatek, linux-kernel,
Gene Chen, Wilma.Wu, shufan_lee, cy_huang, benjamin.chao
Jonathan Cameron <Jonathan.Cameron@huawei.com> 於 2020年9月8日 週二 下午9:00寫道：
>
> ...
>
> > > > > > +#include <linux/completion.h>
> > > > > > +#include <linux/interrupt.h>
> > > > > > +#include <linux/iio/buffer.h>
> > > > > > +#include <linux/iio/iio.h>
> > > > > > +#include <linux/iio/trigger_consumer.h>
> > > > > > +#include <linux/iio/triggered_buffer.h>
> > > > > > +#include <linux/irq.h>
> > > > > > +#include <linux/kernel.h>
> > > > > > +#include <linux/ktime.h>
> > > > > > +#include <linux/module.h>
> > > > > > +#include <linux/mutex.h>
> > > > > > +#include <linux/platform_device.h>
> > > > > > +#include <linux/regmap.h>
> > > > > > +
> > > > > > +#define MT6360_REG_PMUCHGCTRL3 0x313
> > > > > > +#define MT6360_REG_PMUADCCFG 0x356
> > > > > > +#define MT6360_REG_PMUADCRPT1 0x35A
> > > > > > +
> > > > > > +/* PMUCHGCTRL3 0x313 */
> > > > > > +#define MT6360_AICR_MASK 0xFC
> > > > > > +#define MT6360_AICR_SHFT 2
> > > > > > +#define MT6360_AICR_400MA 0x6
> > > > > > +/* PMUADCCFG 0x356 */
> > > > > > +#define MT6360_ADCEN_MASK 0x8000
> > > > > > +/* PMUADCRPT1 0x35A */
> > > > > > +#define MT6360_PREFERCH_MASK 0xF0
> > > > > > +#define MT6360_PREFERCH_SHFT 4
> > > > > > +#define MT6360_RPTCH_MASK 0x0F
> > > > > > +
> > > > > > +enum {
> > > > > > + MT6360_CHAN_USBID = 0,
> > > > > > + MT6360_CHAN_VBUSDIV5,
> > > > > > + MT6360_CHAN_VBUSDIV2,
> > > > > > + MT6360_CHAN_VSYS,
> > > > > > + MT6360_CHAN_VBAT,
> > > > > > + MT6360_CHAN_IBUS,
> > > > > > + MT6360_CHAN_IBAT,
> > > > > > + MT6360_CHAN_CHG_VDDP,
> > > > > > + MT6360_CHAN_TEMP_JC,
> > > > > > + MT6360_CHAN_VREF_TS,
> > > > > > + MT6360_CHAN_TS,
> > > > > > + MT6360_CHAN_MAX,
> > > > > > +};
> > > > > > +
> > > > > > +struct mt6360_adc_data {
> > > > > > + struct device *dev;
> > > > > > + struct regmap *regmap;
> > > > > > + struct completion adc_complete;
> > > > > > + struct mutex adc_lock;
> > > > > > + ktime_t last_off_timestamps[MT6360_CHAN_MAX];
> > > > > > + int irq;
> > > > > > +};
> > > > > > +
> > > > > > +static inline int mt6360_adc_val_converter(int val, int multiplier, int offset, int divisor)
> > > > > > +{
> > > > > > + return ((val * multiplier) + offset) / divisor;
> > > > >
> > > > > Why could we not report these values to userspace or consumer drivers and let
> > > > > them deal with the conversion if they actually needed it?
> > > > > Mapping this to
> > > > >
> > > > > (val + new_offset) * multiplier would be a little messy, but not too bad.
> > > > >
> > > > > The advantage would be that we would then be providing the data needed
> > > > > to get real units for values read from the buffers without having to
> > > > > do all the maths in kernel (without access to floating point).
> > > > >
> > > > >
> > > >
> > > > As above, if I use formula "(val + new_offset) * multiplier",
> > > > the junction temperature channel multiplier will be floating point
> > > > 1.05, i don't know how to express.
> > >
> > > As Andy mentioned, we do this all over the place.
> > > IIO_VAL_INT_PLUS_MICRO
> > >
> > > The key is that we want to push the burden of doing this maths to the user
> > > not the source.
> >
> > ACK.
> > Can I keep IIO_CHAN_INFO_PROCESSED function be reserved for user in
> > kernel space?
> >
>
> No. We have utility functions that will apply the multiplier as needed so
> there is no significant advantage in doing this and it won't be consistent
> with the majority of other drivers.
>
ACK, I will remove IIO_CHAN_INFO_PROCESSED.
> > >
> > > Often what is actually of interest is whether a temperature passed a threshold.
> > > In that case, you can transform the threshold into the units of the ADC (so the
> > > reverse directly to you would do with processed data) and only have to do the
> > > maths once per change of the threshold instead of for every sample.
> > >
> > > There are helper functions to do the maths for you, should you actually
> > > need SI units.
> > >
> >
> > ACK
> >
> > > >
> > > > > > +}
> > > > > > +
> > > > > > +static int mt6360_adc_convert_processed_val(struct mt6360_adc_data *info, int chan_idx, int *val)
> > > > > > +{
> > > > > > + unsigned int regval = 0;
> > > > > > + const struct converter {
> > > > > > + int multiplier;
> > > > > > + int offset;
> > > > > > + int divisor;
> > > > > > + } adc_converter[MT6360_CHAN_MAX] = {
> > > > > > + { 1250, 0, 1}, /* USBID */
> > > > > > + { 6250, 0, 1}, /* VBUSDIV5 */
> > > > > > + { 2500, 0, 1}, /* VBUSDIV2 */
> > > > > > + { 1250, 0, 1}, /* VSYS */
> > > > > > + { 1250, 0, 1}, /* VBAT */
> > > > > > + { 2500, 0, 1}, /* IBUS */
> > > > > > + { 2500, 0, 1}, /* IBAT */
> > > > > > + { 1250, 0, 1}, /* CHG_VDDP */
> > > > > > + { 105, -8000, 100}, /* TEMP_JC */
> > > > > > + { 1250, 0, 1}, /* VREF_TS */
> > > > > > + { 1250, 0, 1}, /* TS */
> > > > > > + }, sp_ibus_adc_converter = { 1900, 0, 1 }, *sel_converter;
> > > > > > + int ret;
> > > > > > +
> > > > > > + sel_converter = adc_converter + chan_idx;
> > > > > > + if (chan_idx == MT6360_CHAN_IBUS) {
> > > > > > + /* ibus chan will be affected by aicr config */
> > > > > > + /* if aicr < 400, apply the special ibus converter */
> > > > > > + ret = regmap_read(info->regmap, MT6360_REG_PMUCHGCTRL3, ®val);
> > > > > > + if (ret)
> > > > > > + return ret;
> > > > > > +
> > > > > > + regval = (regval & MT6360_AICR_MASK) >> MT6360_AICR_SHFT;
> > > > > > + if (regval < MT6360_AICR_400MA)
> > > > > > + sel_converter = &sp_ibus_adc_converter;
> > > > > > + }
> > > > > > +
> > > > > > + *val = mt6360_adc_val_converter(*val, sel_converter->multiplier, sel_converter->offset,
> > > > > > + sel_converter->divisor);
> > > > > > +
> > > > > > + return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static int mt6360_adc_read_processed(struct mt6360_adc_data *mad, int channel, int *val)
> > > > > > +{
> > > > > > + u16 adc_enable;
> > > > > > + u8 rpt[3];
> > > > > > + ktime_t start_t, predict_end_t;
> > > > > > + long timeout;
> > > > > > + int value, ret;
> > > > > > +
> > > > > > + mutex_lock(&mad->adc_lock);
> > > > > > +
> > > > > > + /* select preferred channel that we want */
> > > > > > + ret = regmap_update_bits(mad->regmap, MT6360_REG_PMUADCRPT1, MT6360_PREFERCH_MASK,
> > > > > > + channel << MT6360_PREFERCH_SHFT);
> > > > > > + if (ret)
> > > > > > + goto out_adc;
> > > > > > +
> > > > > > + /* enable adc channel we want and adc_en */
> > > > > > + adc_enable = MT6360_ADCEN_MASK | BIT(channel);
> > > > > > + adc_enable = cpu_to_be16(adc_enable);
> > > > >
> > > > > Use a local be16 to store that. It will make it a little clearer
> > > > > that we are doing something 'unusual' here. Perhaps a comment on
> > > > > why this odd code exists would also help?
> > > > >
> > > >
> > > > ACK
> > > >
> > > > > > + ret = regmap_raw_write(mad->regmap, MT6360_REG_PMUADCCFG, (void *)&adc_enable, sizeof(u16));
> > > > > > + if (ret)
> > > > > > + goto out_adc;
> > > > > > +
> > > > > > + start_t = ktime_get();
> > > > > > + predict_end_t = ktime_add_ms(mad->last_off_timestamps[channel], 50);
> > > > > > +
> > > > > > + if (ktime_after(start_t, predict_end_t))
> > > > > > + predict_end_t = ktime_add_ms(start_t, 25);
> > > > > > + else
> > > > > > + predict_end_t = ktime_add_ms(start_t, 75);
> > > > > > +
> > > > > > + enable_irq(mad->irq);
> > > > > > +adc_retry:
> > > > > > + reinit_completion(&mad->adc_complete);
> > > > > > +
> > > > > > + /* wait for conversion to complete */
> > > > > > + timeout = wait_for_completion_timeout(&mad->adc_complete, msecs_to_jiffies(200));
> > > > > > + if (timeout == 0) {
> > > > > > + ret = -ETIMEDOUT;
> > > > > > + goto out_adc_conv;
> > > > > > + } else if (timeout < 0) {
> > > > > > + ret = -EINTR;
> > > > > > + goto out_adc_conv;
> > > > > > + }
> > > > > > +
> > > > > > + ret = regmap_raw_read(mad->regmap, MT6360_REG_PMUADCRPT1, rpt, sizeof(rpt));
> > > > > > + if (ret)
> > > > > > + goto out_adc_conv;
> > > > > > +
> > > > > > + /* check the current reported channel */
> > > > > > + if ((rpt[0] & MT6360_RPTCH_MASK) != channel) {
> > > > > > + dev_dbg(mad->dev, "not wanted channel report [%02x]\n", rpt[0]);
> > > > >
> > > > > This and the one below feel like error messages rather than debug ones.
> > > > >
> > > >
> > > > We have two function "battery zero current voltage(ZCV)" and "TypeC
> > > > OTP" will auto run ADC at background.
> > > > ZCV_EN will run VBAT_ADC when TA plug in, TypeC OTP will run TS_ADC
> > > > when TypeC attach.
> > > > We need to check report channel for ADC report data match is our desire channel.
> > >
> > > So there is firmware messing with it underneath? Oh goody.
> > > Add a comment explaining this.
> > >
> >
> > ACK, I try to write a comment as below
> >
> > /*
> > * 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 need 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.
> > */
>
> Looks good.
>
ACK
> >
> > > >
> > > > > > + goto adc_retry;
> > > > > > + }
> > > > > > +
> > > > > > + if (!ktime_after(ktime_get(), predict_end_t)) {
> > > > > > + dev_dbg(mad->dev, "time is not after one adc_conv_t\n");
> > > > >
> > > > > Does this actually happen? If feels like we are being a bit over protective
> > > > > here. I'd definitely like to see a comment saying why this protection
> > > > > might be needed.
> > > > >
> > > >
> > > > When ADC_EN and MT6360_CHANx_EN is enable, the channel x will keep
> > > > running again and again
> > > > I supposed to get immediate data which is generated after I start it.
> > >
> > > Just to check my understanding.
> > >
> > > This is an edge triggered interrupt and it triggers every time a new sample
> > > is taken. Those samples are taken on a fixed frequency irrespective of whether
> > > we have read the previous one?
> > >
> >
> > Yes.
> > I use LEVEL_LOW trigger in latest review MFD patch.
>
> I'm not sure I follow that comment. How can you do that if it's a repeating
> edge trigger?
>
I implement "struct regmap_irq_chip" handle_post_irq ops,
In the end of handle irq, I set the re-trigger bit which will pull irq
high to low again if irq pin is low.
-static int mt6360_pmu_handle_post_irq(void *irq_drv_data)
-{
- struct mt6360_pmu_info *mpi = irq_drv_data;
-
- return regmap_update_bits(mpi->regmap,
- MT6360_PMU_IRQ_SET, MT6360_IRQ_RETRIG, MT6360_IRQ_RETRIG);
-}
-
> >
> > > >
> > > > When I disable ADC_CHANx_EN, the H/W logical ADC is still running.
> > > > If I run the same ADC immediately, I may get the old result about this channel.
> > > > MT6360 ADC typical conversation time is about 25ms.
> > > > So We need ignore which irq trigger below 25ms.
> > >
> > > Normal trick for this sort of case is to just not use the interrupt.
> > > Just read after 25+delta msecs and you are guaranteed to get the right answer.
> > >
> > >
> >
> > ACK, I will try to use polling
> > Is the pseudocode correct?
> >
> > mdelay(predict_end_t);
> > while (true) {
> > read adc event is occured
> > check report channel is the same
> > if the same, read report ADC data and break while loop
> > else msleep(per ADC conversion time)
> > }
>
> Looks correct to me. We should 'know' the event has happened but
> still need to check the channel is the expected one.
>
There is a comment in our internal discuss.
If I use msleep as polling interval, the worst case will cause
additional wait time nearly one polling interval.
Can I keep using interrupt for saving time?
> ...
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 1/2] iio: adc: mt6360: Add ADC driver for MT6360
2020-09-08 23:39 ` Gene Chen
@ 2020-09-09 9:34 ` Jonathan Cameron
2020-09-10 0:14 ` Gene Chen
1 sibling, 0 replies; 15+ messages in thread
From: Jonathan Cameron @ 2020-09-09 9:34 UTC (permalink / raw)
To: Gene Chen
Cc: Jonathan Cameron, Matthias Brugger, knaack.h, lars, pmeerw,
linux-iio, linux-arm-kernel, linux-mediatek, linux-kernel,
Gene Chen, Wilma.Wu, shufan_lee, cy_huang, benjamin.chao
On Wed, 9 Sep 2020 07:39:14 +0800
Gene Chen <gene.chen.richtek@gmail.com> wrote:
> Jonathan Cameron <Jonathan.Cameron@huawei.com> 於 2020年9月8日 週二 下午9:00寫道：
> >
> > ...
> >
> > > > > > > +#include <linux/completion.h>
> > > > > > > +#include <linux/interrupt.h>
> > > > > > > +#include <linux/iio/buffer.h>
> > > > > > > +#include <linux/iio/iio.h>
> > > > > > > +#include <linux/iio/trigger_consumer.h>
> > > > > > > +#include <linux/iio/triggered_buffer.h>
> > > > > > > +#include <linux/irq.h>
> > > > > > > +#include <linux/kernel.h>
> > > > > > > +#include <linux/ktime.h>
> > > > > > > +#include <linux/module.h>
> > > > > > > +#include <linux/mutex.h>
> > > > > > > +#include <linux/platform_device.h>
> > > > > > > +#include <linux/regmap.h>
> > > > > > > +
> > > > > > > +#define MT6360_REG_PMUCHGCTRL3 0x313
> > > > > > > +#define MT6360_REG_PMUADCCFG 0x356
> > > > > > > +#define MT6360_REG_PMUADCRPT1 0x35A
> > > > > > > +
> > > > > > > +/* PMUCHGCTRL3 0x313 */
> > > > > > > +#define MT6360_AICR_MASK 0xFC
> > > > > > > +#define MT6360_AICR_SHFT 2
> > > > > > > +#define MT6360_AICR_400MA 0x6
> > > > > > > +/* PMUADCCFG 0x356 */
> > > > > > > +#define MT6360_ADCEN_MASK 0x8000
> > > > > > > +/* PMUADCRPT1 0x35A */
> > > > > > > +#define MT6360_PREFERCH_MASK 0xF0
> > > > > > > +#define MT6360_PREFERCH_SHFT 4
> > > > > > > +#define MT6360_RPTCH_MASK 0x0F
> > > > > > > +
> > > > > > > +enum {
> > > > > > > + MT6360_CHAN_USBID = 0,
> > > > > > > + MT6360_CHAN_VBUSDIV5,
> > > > > > > + MT6360_CHAN_VBUSDIV2,
> > > > > > > + MT6360_CHAN_VSYS,
> > > > > > > + MT6360_CHAN_VBAT,
> > > > > > > + MT6360_CHAN_IBUS,
> > > > > > > + MT6360_CHAN_IBAT,
> > > > > > > + MT6360_CHAN_CHG_VDDP,
> > > > > > > + MT6360_CHAN_TEMP_JC,
> > > > > > > + MT6360_CHAN_VREF_TS,
> > > > > > > + MT6360_CHAN_TS,
> > > > > > > + MT6360_CHAN_MAX,
> > > > > > > +};
> > > > > > > +
> > > > > > > +struct mt6360_adc_data {
> > > > > > > + struct device *dev;
> > > > > > > + struct regmap *regmap;
> > > > > > > + struct completion adc_complete;
> > > > > > > + struct mutex adc_lock;
> > > > > > > + ktime_t last_off_timestamps[MT6360_CHAN_MAX];
> > > > > > > + int irq;
> > > > > > > +};
> > > > > > > +
> > > > > > > +static inline int mt6360_adc_val_converter(int val, int multiplier, int offset, int divisor)
> > > > > > > +{
> > > > > > > + return ((val * multiplier) + offset) / divisor;
> > > > > >
> > > > > > Why could we not report these values to userspace or consumer drivers and let
> > > > > > them deal with the conversion if they actually needed it?
> > > > > > Mapping this to
> > > > > >
> > > > > > (val + new_offset) * multiplier would be a little messy, but not too bad.
> > > > > >
> > > > > > The advantage would be that we would then be providing the data needed
> > > > > > to get real units for values read from the buffers without having to
> > > > > > do all the maths in kernel (without access to floating point).
> > > > > >
> > > > > >
> > > > >
> > > > > As above, if I use formula "(val + new_offset) * multiplier",
> > > > > the junction temperature channel multiplier will be floating point
> > > > > 1.05, i don't know how to express.
> > > >
> > > > As Andy mentioned, we do this all over the place.
> > > > IIO_VAL_INT_PLUS_MICRO
> > > >
> > > > The key is that we want to push the burden of doing this maths to the user
> > > > not the source.
> > >
> > > ACK.
> > > Can I keep IIO_CHAN_INFO_PROCESSED function be reserved for user in
> > > kernel space?
> > >
> >
> > No. We have utility functions that will apply the multiplier as needed so
> > there is no significant advantage in doing this and it won't be consistent
> > with the majority of other drivers.
> >
>
> ACK, I will remove IIO_CHAN_INFO_PROCESSED.
>
> > > >
> > > > Often what is actually of interest is whether a temperature passed a threshold.
> > > > In that case, you can transform the threshold into the units of the ADC (so the
> > > > reverse directly to you would do with processed data) and only have to do the
> > > > maths once per change of the threshold instead of for every sample.
> > > >
> > > > There are helper functions to do the maths for you, should you actually
> > > > need SI units.
> > > >
> > >
> > > ACK
> > >
> > > > >
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int mt6360_adc_convert_processed_val(struct mt6360_adc_data *info, int chan_idx, int *val)
> > > > > > > +{
> > > > > > > + unsigned int regval = 0;
> > > > > > > + const struct converter {
> > > > > > > + int multiplier;
> > > > > > > + int offset;
> > > > > > > + int divisor;
> > > > > > > + } adc_converter[MT6360_CHAN_MAX] = {
> > > > > > > + { 1250, 0, 1}, /* USBID */
> > > > > > > + { 6250, 0, 1}, /* VBUSDIV5 */
> > > > > > > + { 2500, 0, 1}, /* VBUSDIV2 */
> > > > > > > + { 1250, 0, 1}, /* VSYS */
> > > > > > > + { 1250, 0, 1}, /* VBAT */
> > > > > > > + { 2500, 0, 1}, /* IBUS */
> > > > > > > + { 2500, 0, 1}, /* IBAT */
> > > > > > > + { 1250, 0, 1}, /* CHG_VDDP */
> > > > > > > + { 105, -8000, 100}, /* TEMP_JC */
> > > > > > > + { 1250, 0, 1}, /* VREF_TS */
> > > > > > > + { 1250, 0, 1}, /* TS */
> > > > > > > + }, sp_ibus_adc_converter = { 1900, 0, 1 }, *sel_converter;
> > > > > > > + int ret;
> > > > > > > +
> > > > > > > + sel_converter = adc_converter + chan_idx;
> > > > > > > + if (chan_idx == MT6360_CHAN_IBUS) {
> > > > > > > + /* ibus chan will be affected by aicr config */
> > > > > > > + /* if aicr < 400, apply the special ibus converter */
> > > > > > > + ret = regmap_read(info->regmap, MT6360_REG_PMUCHGCTRL3, ®val);
> > > > > > > + if (ret)
> > > > > > > + return ret;
> > > > > > > +
> > > > > > > + regval = (regval & MT6360_AICR_MASK) >> MT6360_AICR_SHFT;
> > > > > > > + if (regval < MT6360_AICR_400MA)
> > > > > > > + sel_converter = &sp_ibus_adc_converter;
> > > > > > > + }
> > > > > > > +
> > > > > > > + *val = mt6360_adc_val_converter(*val, sel_converter->multiplier, sel_converter->offset,
> > > > > > > + sel_converter->divisor);
> > > > > > > +
> > > > > > > + return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int mt6360_adc_read_processed(struct mt6360_adc_data *mad, int channel, int *val)
> > > > > > > +{
> > > > > > > + u16 adc_enable;
> > > > > > > + u8 rpt[3];
> > > > > > > + ktime_t start_t, predict_end_t;
> > > > > > > + long timeout;
> > > > > > > + int value, ret;
> > > > > > > +
> > > > > > > + mutex_lock(&mad->adc_lock);
> > > > > > > +
> > > > > > > + /* select preferred channel that we want */
> > > > > > > + ret = regmap_update_bits(mad->regmap, MT6360_REG_PMUADCRPT1, MT6360_PREFERCH_MASK,
> > > > > > > + channel << MT6360_PREFERCH_SHFT);
> > > > > > > + if (ret)
> > > > > > > + goto out_adc;
> > > > > > > +
> > > > > > > + /* enable adc channel we want and adc_en */
> > > > > > > + adc_enable = MT6360_ADCEN_MASK | BIT(channel);
> > > > > > > + adc_enable = cpu_to_be16(adc_enable);
> > > > > >
> > > > > > Use a local be16 to store that. It will make it a little clearer
> > > > > > that we are doing something 'unusual' here. Perhaps a comment on
> > > > > > why this odd code exists would also help?
> > > > > >
> > > > >
> > > > > ACK
> > > > >
> > > > > > > + ret = regmap_raw_write(mad->regmap, MT6360_REG_PMUADCCFG, (void *)&adc_enable, sizeof(u16));
> > > > > > > + if (ret)
> > > > > > > + goto out_adc;
> > > > > > > +
> > > > > > > + start_t = ktime_get();
> > > > > > > + predict_end_t = ktime_add_ms(mad->last_off_timestamps[channel], 50);
> > > > > > > +
> > > > > > > + if (ktime_after(start_t, predict_end_t))
> > > > > > > + predict_end_t = ktime_add_ms(start_t, 25);
> > > > > > > + else
> > > > > > > + predict_end_t = ktime_add_ms(start_t, 75);
> > > > > > > +
> > > > > > > + enable_irq(mad->irq);
> > > > > > > +adc_retry:
> > > > > > > + reinit_completion(&mad->adc_complete);
> > > > > > > +
> > > > > > > + /* wait for conversion to complete */
> > > > > > > + timeout = wait_for_completion_timeout(&mad->adc_complete, msecs_to_jiffies(200));
> > > > > > > + if (timeout == 0) {
> > > > > > > + ret = -ETIMEDOUT;
> > > > > > > + goto out_adc_conv;
> > > > > > > + } else if (timeout < 0) {
> > > > > > > + ret = -EINTR;
> > > > > > > + goto out_adc_conv;
> > > > > > > + }
> > > > > > > +
> > > > > > > + ret = regmap_raw_read(mad->regmap, MT6360_REG_PMUADCRPT1, rpt, sizeof(rpt));
> > > > > > > + if (ret)
> > > > > > > + goto out_adc_conv;
> > > > > > > +
> > > > > > > + /* check the current reported channel */
> > > > > > > + if ((rpt[0] & MT6360_RPTCH_MASK) != channel) {
> > > > > > > + dev_dbg(mad->dev, "not wanted channel report [%02x]\n", rpt[0]);
> > > > > >
> > > > > > This and the one below feel like error messages rather than debug ones.
> > > > > >
> > > > >
> > > > > We have two function "battery zero current voltage(ZCV)" and "TypeC
> > > > > OTP" will auto run ADC at background.
> > > > > ZCV_EN will run VBAT_ADC when TA plug in, TypeC OTP will run TS_ADC
> > > > > when TypeC attach.
> > > > > We need to check report channel for ADC report data match is our desire channel.
> > > >
> > > > So there is firmware messing with it underneath? Oh goody.
> > > > Add a comment explaining this.
> > > >
> > >
> > > ACK, I try to write a comment as below
> > >
> > > /*
> > > * 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 need 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.
> > > */
> >
> > Looks good.
> >
>
> ACK
>
> > >
> > > > >
> > > > > > > + goto adc_retry;
> > > > > > > + }
> > > > > > > +
> > > > > > > + if (!ktime_after(ktime_get(), predict_end_t)) {
> > > > > > > + dev_dbg(mad->dev, "time is not after one adc_conv_t\n");
> > > > > >
> > > > > > Does this actually happen? If feels like we are being a bit over protective
> > > > > > here. I'd definitely like to see a comment saying why this protection
> > > > > > might be needed.
> > > > > >
> > > > >
> > > > > When ADC_EN and MT6360_CHANx_EN is enable, the channel x will keep
> > > > > running again and again
> > > > > I supposed to get immediate data which is generated after I start it.
> > > >
> > > > Just to check my understanding.
> > > >
> > > > This is an edge triggered interrupt and it triggers every time a new sample
> > > > is taken. Those samples are taken on a fixed frequency irrespective of whether
> > > > we have read the previous one?
> > > >
> > >
> > > Yes.
> > > I use LEVEL_LOW trigger in latest review MFD patch.
> >
> > I'm not sure I follow that comment. How can you do that if it's a repeating
> > edge trigger?
> >
>
> I implement "struct regmap_irq_chip" handle_post_irq ops,
> In the end of handle irq, I set the re-trigger bit which will pull irq
> high to low again if irq pin is low.
>
> -static int mt6360_pmu_handle_post_irq(void *irq_drv_data)
> -{
> - struct mt6360_pmu_info *mpi = irq_drv_data;
> -
> - return regmap_update_bits(mpi->regmap,
> - MT6360_PMU_IRQ_SET, MT6360_IRQ_RETRIG, MT6360_IRQ_RETRIG);
> -}
> -
Ah understood I think.
>
> > >
> > > > >
> > > > > When I disable ADC_CHANx_EN, the H/W logical ADC is still running.
> > > > > If I run the same ADC immediately, I may get the old result about this channel.
> > > > > MT6360 ADC typical conversation time is about 25ms.
> > > > > So We need ignore which irq trigger below 25ms.
> > > >
> > > > Normal trick for this sort of case is to just not use the interrupt.
> > > > Just read after 25+delta msecs and you are guaranteed to get the right answer.
> > > >
> > > >
> > >
> > > ACK, I will try to use polling
> > > Is the pseudocode correct?
> > >
> > > mdelay(predict_end_t);
> > > while (true) {
> > > read adc event is occured
> > > check report channel is the same
> > > if the same, read report ADC data and break while loop
> > > else msleep(per ADC conversion time)
> > > }
> >
> > Looks correct to me. We should 'know' the event has happened but
> > still need to check the channel is the expected one.
> >
>
> There is a comment in our internal discuss.
> If I use msleep as polling interval, the worst case will cause
> additional wait time nearly one polling interval.
> Can I keep using interrupt for saving time?
You could but it is complicating the code to deal with frankly stupid
hardware design which I'm not that keen on.
If it ends up clean enough with a lot comments on why the odd parts
are there, then maybe it will be fine.
Jonathan
>
> > ...
> >
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 1/2] iio: adc: mt6360: Add ADC driver for MT6360
2020-09-08 23:39 ` Gene Chen
2020-09-09 9:34 ` Jonathan Cameron
@ 2020-09-10 0:14 ` Gene Chen
1 sibling, 0 replies; 15+ messages in thread
From: Gene Chen @ 2020-09-10 0:14 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Jonathan Cameron, Matthias Brugger, knaack.h, lars, pmeerw,
linux-iio, linux-arm-kernel, linux-mediatek, linux-kernel,
Gene Chen, Wilma.Wu, shufan_lee, cy_huang, benjamin.chao
Gene Chen <gene.chen.richtek@gmail.com> 於 2020年9月9日 週三 上午7:39寫道：
>
> Jonathan Cameron <Jonathan.Cameron@huawei.com> 於 2020年9月8日 週二 下午9:00寫道：
> >
> > ...
> >
> > > > > > > +#include <linux/completion.h>
> > > > > > > +#include <linux/interrupt.h>
> > > > > > > +#include <linux/iio/buffer.h>
> > > > > > > +#include <linux/iio/iio.h>
> > > > > > > +#include <linux/iio/trigger_consumer.h>
> > > > > > > +#include <linux/iio/triggered_buffer.h>
> > > > > > > +#include <linux/irq.h>
> > > > > > > +#include <linux/kernel.h>
> > > > > > > +#include <linux/ktime.h>
> > > > > > > +#include <linux/module.h>
> > > > > > > +#include <linux/mutex.h>
> > > > > > > +#include <linux/platform_device.h>
> > > > > > > +#include <linux/regmap.h>
> > > > > > > +
> > > > > > > +#define MT6360_REG_PMUCHGCTRL3 0x313
> > > > > > > +#define MT6360_REG_PMUADCCFG 0x356
> > > > > > > +#define MT6360_REG_PMUADCRPT1 0x35A
> > > > > > > +
> > > > > > > +/* PMUCHGCTRL3 0x313 */
> > > > > > > +#define MT6360_AICR_MASK 0xFC
> > > > > > > +#define MT6360_AICR_SHFT 2
> > > > > > > +#define MT6360_AICR_400MA 0x6
> > > > > > > +/* PMUADCCFG 0x356 */
> > > > > > > +#define MT6360_ADCEN_MASK 0x8000
> > > > > > > +/* PMUADCRPT1 0x35A */
> > > > > > > +#define MT6360_PREFERCH_MASK 0xF0
> > > > > > > +#define MT6360_PREFERCH_SHFT 4
> > > > > > > +#define MT6360_RPTCH_MASK 0x0F
> > > > > > > +
> > > > > > > +enum {
> > > > > > > + MT6360_CHAN_USBID = 0,
> > > > > > > + MT6360_CHAN_VBUSDIV5,
> > > > > > > + MT6360_CHAN_VBUSDIV2,
> > > > > > > + MT6360_CHAN_VSYS,
> > > > > > > + MT6360_CHAN_VBAT,
> > > > > > > + MT6360_CHAN_IBUS,
> > > > > > > + MT6360_CHAN_IBAT,
> > > > > > > + MT6360_CHAN_CHG_VDDP,
> > > > > > > + MT6360_CHAN_TEMP_JC,
> > > > > > > + MT6360_CHAN_VREF_TS,
> > > > > > > + MT6360_CHAN_TS,
> > > > > > > + MT6360_CHAN_MAX,
> > > > > > > +};
> > > > > > > +
> > > > > > > +struct mt6360_adc_data {
> > > > > > > + struct device *dev;
> > > > > > > + struct regmap *regmap;
> > > > > > > + struct completion adc_complete;
> > > > > > > + struct mutex adc_lock;
> > > > > > > + ktime_t last_off_timestamps[MT6360_CHAN_MAX];
> > > > > > > + int irq;
> > > > > > > +};
> > > > > > > +
> > > > > > > +static inline int mt6360_adc_val_converter(int val, int multiplier, int offset, int divisor)
> > > > > > > +{
> > > > > > > + return ((val * multiplier) + offset) / divisor;
> > > > > >
> > > > > > Why could we not report these values to userspace or consumer drivers and let
> > > > > > them deal with the conversion if they actually needed it?
> > > > > > Mapping this to
> > > > > >
> > > > > > (val + new_offset) * multiplier would be a little messy, but not too bad.
> > > > > >
> > > > > > The advantage would be that we would then be providing the data needed
> > > > > > to get real units for values read from the buffers without having to
> > > > > > do all the maths in kernel (without access to floating point).
> > > > > >
> > > > > >
> > > > >
> > > > > As above, if I use formula "(val + new_offset) * multiplier",
> > > > > the junction temperature channel multiplier will be floating point
> > > > > 1.05, i don't know how to express.
> > > >
> > > > As Andy mentioned, we do this all over the place.
> > > > IIO_VAL_INT_PLUS_MICRO
> > > >
> > > > The key is that we want to push the burden of doing this maths to the user
> > > > not the source.
> > >
> > > ACK.
> > > Can I keep IIO_CHAN_INFO_PROCESSED function be reserved for user in
> > > kernel space?
> > >
> >
> > No. We have utility functions that will apply the multiplier as needed so
> > there is no significant advantage in doing this and it won't be consistent
> > with the majority of other drivers.
> >
>
> ACK, I will remove IIO_CHAN_INFO_PROCESSED.
>
> > > >
> > > > Often what is actually of interest is whether a temperature passed a threshold.
> > > > In that case, you can transform the threshold into the units of the ADC (so the
> > > > reverse directly to you would do with processed data) and only have to do the
> > > > maths once per change of the threshold instead of for every sample.
> > > >
> > > > There are helper functions to do the maths for you, should you actually
> > > > need SI units.
> > > >
> > >
> > > ACK
> > >
> > > > >
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int mt6360_adc_convert_processed_val(struct mt6360_adc_data *info, int chan_idx, int *val)
> > > > > > > +{
> > > > > > > + unsigned int regval = 0;
> > > > > > > + const struct converter {
> > > > > > > + int multiplier;
> > > > > > > + int offset;
> > > > > > > + int divisor;
> > > > > > > + } adc_converter[MT6360_CHAN_MAX] = {
> > > > > > > + { 1250, 0, 1}, /* USBID */
> > > > > > > + { 6250, 0, 1}, /* VBUSDIV5 */
> > > > > > > + { 2500, 0, 1}, /* VBUSDIV2 */
> > > > > > > + { 1250, 0, 1}, /* VSYS */
> > > > > > > + { 1250, 0, 1}, /* VBAT */
> > > > > > > + { 2500, 0, 1}, /* IBUS */
> > > > > > > + { 2500, 0, 1}, /* IBAT */
> > > > > > > + { 1250, 0, 1}, /* CHG_VDDP */
> > > > > > > + { 105, -8000, 100}, /* TEMP_JC */
> > > > > > > + { 1250, 0, 1}, /* VREF_TS */
> > > > > > > + { 1250, 0, 1}, /* TS */
> > > > > > > + }, sp_ibus_adc_converter = { 1900, 0, 1 }, *sel_converter;
> > > > > > > + int ret;
> > > > > > > +
> > > > > > > + sel_converter = adc_converter + chan_idx;
> > > > > > > + if (chan_idx == MT6360_CHAN_IBUS) {
> > > > > > > + /* ibus chan will be affected by aicr config */
> > > > > > > + /* if aicr < 400, apply the special ibus converter */
> > > > > > > + ret = regmap_read(info->regmap, MT6360_REG_PMUCHGCTRL3, ®val);
> > > > > > > + if (ret)
> > > > > > > + return ret;
> > > > > > > +
> > > > > > > + regval = (regval & MT6360_AICR_MASK) >> MT6360_AICR_SHFT;
> > > > > > > + if (regval < MT6360_AICR_400MA)
> > > > > > > + sel_converter = &sp_ibus_adc_converter;
> > > > > > > + }
> > > > > > > +
> > > > > > > + *val = mt6360_adc_val_converter(*val, sel_converter->multiplier, sel_converter->offset,
> > > > > > > + sel_converter->divisor);
> > > > > > > +
> > > > > > > + return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int mt6360_adc_read_processed(struct mt6360_adc_data *mad, int channel, int *val)
> > > > > > > +{
> > > > > > > + u16 adc_enable;
> > > > > > > + u8 rpt[3];
> > > > > > > + ktime_t start_t, predict_end_t;
> > > > > > > + long timeout;
> > > > > > > + int value, ret;
> > > > > > > +
> > > > > > > + mutex_lock(&mad->adc_lock);
> > > > > > > +
> > > > > > > + /* select preferred channel that we want */
> > > > > > > + ret = regmap_update_bits(mad->regmap, MT6360_REG_PMUADCRPT1, MT6360_PREFERCH_MASK,
> > > > > > > + channel << MT6360_PREFERCH_SHFT);
> > > > > > > + if (ret)
> > > > > > > + goto out_adc;
> > > > > > > +
> > > > > > > + /* enable adc channel we want and adc_en */
> > > > > > > + adc_enable = MT6360_ADCEN_MASK | BIT(channel);
> > > > > > > + adc_enable = cpu_to_be16(adc_enable);
> > > > > >
> > > > > > Use a local be16 to store that. It will make it a little clearer
> > > > > > that we are doing something 'unusual' here. Perhaps a comment on
> > > > > > why this odd code exists would also help?
> > > > > >
> > > > >
> > > > > ACK
> > > > >
> > > > > > > + ret = regmap_raw_write(mad->regmap, MT6360_REG_PMUADCCFG, (void *)&adc_enable, sizeof(u16));
> > > > > > > + if (ret)
> > > > > > > + goto out_adc;
> > > > > > > +
> > > > > > > + start_t = ktime_get();
> > > > > > > + predict_end_t = ktime_add_ms(mad->last_off_timestamps[channel], 50);
> > > > > > > +
> > > > > > > + if (ktime_after(start_t, predict_end_t))
> > > > > > > + predict_end_t = ktime_add_ms(start_t, 25);
> > > > > > > + else
> > > > > > > + predict_end_t = ktime_add_ms(start_t, 75);
> > > > > > > +
> > > > > > > + enable_irq(mad->irq);
> > > > > > > +adc_retry:
> > > > > > > + reinit_completion(&mad->adc_complete);
> > > > > > > +
> > > > > > > + /* wait for conversion to complete */
> > > > > > > + timeout = wait_for_completion_timeout(&mad->adc_complete, msecs_to_jiffies(200));
> > > > > > > + if (timeout == 0) {
> > > > > > > + ret = -ETIMEDOUT;
> > > > > > > + goto out_adc_conv;
> > > > > > > + } else if (timeout < 0) {
> > > > > > > + ret = -EINTR;
> > > > > > > + goto out_adc_conv;
> > > > > > > + }
> > > > > > > +
> > > > > > > + ret = regmap_raw_read(mad->regmap, MT6360_REG_PMUADCRPT1, rpt, sizeof(rpt));
> > > > > > > + if (ret)
> > > > > > > + goto out_adc_conv;
> > > > > > > +
> > > > > > > + /* check the current reported channel */
> > > > > > > + if ((rpt[0] & MT6360_RPTCH_MASK) != channel) {
> > > > > > > + dev_dbg(mad->dev, "not wanted channel report [%02x]\n", rpt[0]);
> > > > > >
> > > > > > This and the one below feel like error messages rather than debug ones.
> > > > > >
> > > > >
> > > > > We have two function "battery zero current voltage(ZCV)" and "TypeC
> > > > > OTP" will auto run ADC at background.
> > > > > ZCV_EN will run VBAT_ADC when TA plug in, TypeC OTP will run TS_ADC
> > > > > when TypeC attach.
> > > > > We need to check report channel for ADC report data match is our desire channel.
> > > >
> > > > So there is firmware messing with it underneath? Oh goody.
> > > > Add a comment explaining this.
> > > >
> > >
> > > ACK, I try to write a comment as below
> > >
> > > /*
> > > * 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 need 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.
> > > */
> >
> > Looks good.
> >
>
> ACK
>
> > >
> > > > >
> > > > > > > + goto adc_retry;
> > > > > > > + }
> > > > > > > +
> > > > > > > + if (!ktime_after(ktime_get(), predict_end_t)) {
> > > > > > > + dev_dbg(mad->dev, "time is not after one adc_conv_t\n");
> > > > > >
> > > > > > Does this actually happen? If feels like we are being a bit over protective
> > > > > > here. I'd definitely like to see a comment saying why this protection
> > > > > > might be needed.
> > > > > >
> > > > >
> > > > > When ADC_EN and MT6360_CHANx_EN is enable, the channel x will keep
> > > > > running again and again
> > > > > I supposed to get immediate data which is generated after I start it.
> > > >
> > > > Just to check my understanding.
> > > >
> > > > This is an edge triggered interrupt and it triggers every time a new sample
> > > > is taken. Those samples are taken on a fixed frequency irrespective of whether
> > > > we have read the previous one?
> > > >
> > >
> > > Yes.
> > > I use LEVEL_LOW trigger in latest review MFD patch.
> >
> > I'm not sure I follow that comment. How can you do that if it's a repeating
> > edge trigger?
> >
>
> I implement "struct regmap_irq_chip" handle_post_irq ops,
> In the end of handle irq, I set the re-trigger bit which will pull irq
> high to low again if irq pin is low.
>
> -static int mt6360_pmu_handle_post_irq(void *irq_drv_data)
> -{
> - struct mt6360_pmu_info *mpi = irq_drv_data;
> -
> - return regmap_update_bits(mpi->regmap,
> - MT6360_PMU_IRQ_SET, MT6360_IRQ_RETRIG, MT6360_IRQ_RETRIG);
> -}
> -
>
> > >
> > > > >
> > > > > When I disable ADC_CHANx_EN, the H/W logical ADC is still running.
> > > > > If I run the same ADC immediately, I may get the old result about this channel.
> > > > > MT6360 ADC typical conversation time is about 25ms.
> > > > > So We need ignore which irq trigger below 25ms.
> > > >
> > > > Normal trick for this sort of case is to just not use the interrupt.
> > > > Just read after 25+delta msecs and you are guaranteed to get the right answer.
> > > >
> > > >
> > >
> > > ACK, I will try to use polling
> > > Is the pseudocode correct?
> > >
> > > mdelay(predict_end_t);
> > > while (true) {
> > > read adc event is occured
> > > check report channel is the same
> > > if the same, read report ADC data and break while loop
> > > else msleep(per ADC conversion time)
> > > }
> >
> > Looks correct to me. We should 'know' the event has happened but
> > still need to check the channel is the expected one.
> >
>
> There is a comment in our internal discuss.
> If I use msleep as polling interval, the worst case will cause
> additional wait time nearly one polling interval.
> Can I keep using interrupt for saving time?
>
ACK, I will use polling only.
This is our IC limitation which will be fixed in next generation.
> > ...
> >
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2020-09-10 8:41 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-24 9:06 [PATCH v3 0/2] iio: adc: mt6360: Add ADC driver for MT6360 Gene Chen
2020-08-24 9:06 ` [PATCH v3 1/2] " Gene Chen
2020-08-27 12:16 ` kernel test robot
2020-08-29 17:11 ` Jonathan Cameron
2020-09-08 6:17 ` Gene Chen
2020-09-08 7:43 ` Andy Shevchenko
2020-09-08 9:07 ` Jonathan Cameron
2020-09-08 11:08 ` Gene Chen
2020-09-08 12:58 ` Jonathan Cameron
2020-09-08 23:39 ` Gene Chen
2020-09-09 9:34 ` Jonathan Cameron
2020-09-10 0:14 ` Gene Chen
2020-08-30 17:39 ` Andy Shevchenko
2020-08-24 9:06 ` [PATCH v3 2/2] Documentation: ABI: testing: mt6360: Add ADC sysfs guideline Gene Chen
2020-08-29 17:15 ` Jonathan Cameron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).