Add a driver to support reading the Auxiliary ADC IP found in the
MediaTek MT6357, MT6358 and MT6359 Power Management ICs.
This driver provides multiple ADC channels for system monitoring,
such as battery voltage, PMIC temperature, PMIC-internal voltage
regulators temperature, and others.
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
drivers/iio/adc/Kconfig | 12 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/mt6359-auxadc.c | 598 ++++++++++++++++++++++++++++++++
3 files changed, 611 insertions(+)
create mode 100644 drivers/iio/adc/mt6359-auxadc.c
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 8db68b80b391..1c3df21beaf3 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -870,6 +870,18 @@ config MCP3911
This driver can also be built as a module. If so, the module will be
called mcp3911.
+config MEDIATEK_MT6359_AUXADC
+ tristate "MediaTek MT6359 PMIC AUXADC driver"
+ depends on MFD_MT6397
+ help
+ Say yes here to enable support for MediaTek MT6357, MT6358 and
+ MT6359 PMICs Auxiliary ADC.
+ This driver provides multiple channels for system monitoring,
+ such as battery voltage, PMIC temperature, and others.
+
+ This driver can also be built as a module. If so, the module will be
+ called mt6359-auxadc.
+
config MEDIATEK_MT6360_ADC
tristate "Mediatek MT6360 ADC driver"
depends on MFD_MT6360
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index edb32ce2af02..da7d4452b1e0 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -79,6 +79,7 @@ obj-$(CONFIG_MCP320X) += mcp320x.o
obj-$(CONFIG_MCP3422) += mcp3422.o
obj-$(CONFIG_MCP3564) += mcp3564.o
obj-$(CONFIG_MCP3911) += mcp3911.o
+obj-$(CONFIG_MEDIATEK_MT6359_AUXADC) += mt6359-auxadc.o
obj-$(CONFIG_MEDIATEK_MT6360_ADC) += mt6360-adc.o
obj-$(CONFIG_MEDIATEK_MT6370_ADC) += mt6370-adc.o
obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o
diff --git a/drivers/iio/adc/mt6359-auxadc.c b/drivers/iio/adc/mt6359-auxadc.c
new file mode 100644
index 000000000000..0481bd3f0144
--- /dev/null
+++ b/drivers/iio/adc/mt6359-auxadc.c
@@ -0,0 +1,598 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * MediaTek MT6359 PMIC AUXADC IIO driver
+ *
+ * Copyright (c) 2021 MediaTek Inc.
+ * Copyright (c) 2024 Collabora Ltd
+ * Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
+ */
+
+#include <linux/delay.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/iio/iio.h>
+#include <linux/mfd/mt6397/core.h>
+
+#include <dt-bindings/iio/adc/mediatek,mt6357-auxadc.h>
+#include <dt-bindings/iio/adc/mediatek,mt6358-auxadc.h>
+#include <dt-bindings/iio/adc/mediatek,mt6359-auxadc.h>
+
+#define AUXADC_AVG_TIME_US 10
+#define AUXADC_POLL_DELAY_US 100
+#define AUXADC_TIMEOUT_US 32000
+#define AUXADC_VOLT_FULL 1800
+#define IMP_STOP_DELAY_US 150
+#define IMP_POLL_DELAY_US 1000
+
+#define PMIC_RG_RESET_VAL (BIT(0) | BIT(3))
+#define PMIC_AUXADC_RDY_BIT BIT(15)
+#define PMIC_AUXADC_ADCx(x) ((x) << 1)
+#define MT6357_IMP_ADC_NUM 30
+#define MT6358_IMP_ADC_NUM 28
+
+#define MT6358_DCM_CK_SW_EN GENMASK(1, 0)
+#define MT6358_IMP0_CLEAR (BIT(14) | BIT(7))
+#define MT6358_IMP0_IRQ_RDY BIT(8)
+#define MT6358_IMP1_AUTOREPEAT_EN BIT(15)
+
+#define MT6359_IMP0_CONV_EN BIT(0)
+#define MT6359_IMP1_IRQ_RDY BIT(15)
+
+enum mtk_pmic_auxadc_regs {
+ PMIC_AUXADC_ADC0,
+ PMIC_AUXADC_DCM_CON,
+ PMIC_AUXADC_IMP0,
+ PMIC_AUXADC_IMP1,
+ PMIC_AUXADC_IMP3,
+ PMIC_AUXADC_RQST0,
+ PMIC_AUXADC_RQST1,
+ PMIC_HK_TOP_WKEY,
+ PMIC_HK_TOP_RST_CON0,
+ PMIC_FGADC_R_CON0,
+ PMIC_AUXADC_REGS_MAX
+};
+
+enum mtk_pmic_auxadc_channels {
+ PMIC_AUXADC_CHAN_BATADC,
+ PMIC_AUXADC_CHAN_ISENSE,
+ PMIC_AUXADC_CHAN_VCDT,
+ PMIC_AUXADC_CHAN_BAT_TEMP,
+ PMIC_AUXADC_CHAN_BATID,
+ PMIC_AUXADC_CHAN_CHIP_TEMP,
+ PMIC_AUXADC_CHAN_VCORE_TEMP,
+ PMIC_AUXADC_CHAN_VPROC_TEMP,
+ PMIC_AUXADC_CHAN_VGPU_TEMP,
+ PMIC_AUXADC_CHAN_ACCDET,
+ PMIC_AUXADC_CHAN_VDCXO,
+ PMIC_AUXADC_CHAN_TSX_TEMP,
+ PMIC_AUXADC_CHAN_HPOFS_CAL,
+ PMIC_AUXADC_CHAN_DCXO_TEMP,
+ PMIC_AUXADC_CHAN_VBIF,
+ PMIC_AUXADC_CHAN_IBAT,
+ PMIC_AUXADC_CHAN_VBAT,
+ PMIC_AUXADC_CHAN_MAX
+};
+
+/**
+ * struct mt6359_auxadc - Main driver structure
+ * @dev: Device pointer
+ * @regmap: Regmap from SoC PMIC Wrapper
+ * @pdata: PMIC specific platform data
+ * @lock: Mutex lock for AUXADC reads
+ * @timed_out: Signals whether the last read timed out
+ */
+struct mt6359_auxadc {
+ struct device *dev;
+ struct regmap *regmap;
+ const struct mtk_pmic_auxadc_pdata *pdata;
+ struct mutex lock;
+ bool timed_out;
+};
+
+/**
+ * struct mtk_pmic_auxadc_chan - PMIC AUXADC channel data
+ * @req_idx: Request register number
+ * @req_mask: Bitmask to activate a channel
+ * @num_samples: Number of AUXADC samples for averaging
+ * @r_numerator: Resistance ratio numerator
+ * @r_denominator: Resistance ratio denominator
+ */
+struct mtk_pmic_auxadc_chan {
+ u8 req_idx;
+ u16 req_mask;
+ u16 num_samples;
+ u8 r_numerator;
+ u8 r_denominator;
+};
+
+/**
+ * struct mtk_pmic_auxadc_pdata - PMIC specific platform data
+ * @channels: IIO specification of ADC channels
+ * @num_channels: Number of ADC channels
+ * @desc: PMIC AUXADC channel data
+ * @regs: List of PMIC specific registers
+ * @sec_unlock_key: Security unlock key for HK_TOP writes
+ * @imp_adc_num: ADC channel for IMP readings
+ * @read_imp: Callback to read PMIC IMP channels
+ */
+struct mtk_pmic_auxadc_pdata {
+ const struct iio_chan_spec *channels;
+ int num_channels;
+ const struct mtk_pmic_auxadc_chan *desc;
+ const u16 *regs;
+ u16 sec_unlock_key;
+ u8 imp_adc_num;
+ int (*read_imp)(struct mt6359_auxadc *adc_dev, int *vbat, int *ibat);
+};
+
+#define MTK_PMIC_ADC_CHAN(_ch_idx, _req_idx, _req_bit, _samples, _rnum, _rdiv) \
+ [PMIC_AUXADC_CHAN_##_ch_idx] = { \
+ .req_idx = _req_idx, \
+ .req_mask = BIT(_req_bit), \
+ .num_samples = _samples, \
+ .r_numerator = _rnum, \
+ .r_denominator = _rdiv, \
+ }
+
+#define MTK_PMIC_IIO_CHAN(_model, _name, _ch_idx, _adc_idx, _nbits, _ch_type) \
+{ \
+ .type = _ch_type, \
+ .channel = _model##_AUXADC_##_ch_idx, \
+ .address = _adc_idx, \
+ .scan_index = PMIC_AUXADC_CHAN_##_ch_idx, \
+ .datasheet_name = __stringify(_name), \
+ .scan_type = { \
+ .sign = 'u', \
+ .realbits = _nbits, \
+ .storagebits = 16, \
+ .endianness = IIO_CPU \
+ }, \
+ .indexed = 1, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE) \
+}
+
+static const struct iio_chan_spec mt6357_auxadc_channels[] = {
+ MTK_PMIC_IIO_CHAN(MT6357, bat_adc, BATADC, 0, 15, IIO_RESISTANCE),
+ MTK_PMIC_IIO_CHAN(MT6357, isense, ISENSE, 1, 12, IIO_CURRENT),
+ MTK_PMIC_IIO_CHAN(MT6357, cdt_v, VCDT, 2, 12, IIO_TEMP),
+ MTK_PMIC_IIO_CHAN(MT6357, batt_temp, BAT_TEMP, 3, 12, IIO_TEMP),
+ MTK_PMIC_IIO_CHAN(MT6357, chip_temp, CHIP_TEMP, 4, 12, IIO_TEMP),
+ MTK_PMIC_IIO_CHAN(MT6357, acc_det, ACCDET, 5, 12, IIO_RESISTANCE),
+ MTK_PMIC_IIO_CHAN(MT6357, dcxo_v, VDCXO, 6, 12, IIO_VOLTAGE),
+ MTK_PMIC_IIO_CHAN(MT6357, tsx_temp, TSX_TEMP, 7, 15, IIO_TEMP),
+ MTK_PMIC_IIO_CHAN(MT6357, hp_ofs_cal, HPOFS_CAL, 9, 15, IIO_RESISTANCE),
+ MTK_PMIC_IIO_CHAN(MT6357, dcxo_temp, DCXO_TEMP, 36, 15, IIO_TEMP),
+ MTK_PMIC_IIO_CHAN(MT6357, vcore_temp, VCORE_TEMP, 40, 12, IIO_TEMP),
+ MTK_PMIC_IIO_CHAN(MT6357, vproc_temp, VPROC_TEMP, 41, 12, IIO_TEMP),
+
+ /* IMP channels */
+ MTK_PMIC_IIO_CHAN(MT6357, batt_v, VBAT, 0, 15, IIO_VOLTAGE),
+};
+
+static const struct mtk_pmic_auxadc_chan mt6357_auxadc_ch_desc[] = {
+ MTK_PMIC_ADC_CHAN(BATADC, PMIC_AUXADC_RQST0, 0, 128, 3, 1),
+ MTK_PMIC_ADC_CHAN(ISENSE, PMIC_AUXADC_RQST0, 0, 128, 3, 1),
+ MTK_PMIC_ADC_CHAN(VCDT, PMIC_AUXADC_RQST0, 0, 8, 1, 1),
+ MTK_PMIC_ADC_CHAN(BAT_TEMP, PMIC_AUXADC_RQST0, 3, 8, 1, 1),
+ MTK_PMIC_ADC_CHAN(CHIP_TEMP, PMIC_AUXADC_RQST0, 4, 8, 1, 1),
+ MTK_PMIC_ADC_CHAN(ACCDET, PMIC_AUXADC_RQST0, 5, 8, 1, 1),
+ MTK_PMIC_ADC_CHAN(TSX_TEMP, PMIC_AUXADC_RQST0, 7, 128, 1, 1),
+ MTK_PMIC_ADC_CHAN(HPOFS_CAL, PMIC_AUXADC_RQST0, 9, 256, 1, 1),
+ MTK_PMIC_ADC_CHAN(DCXO_TEMP, PMIC_AUXADC_RQST0, 10, 16, 1, 1),
+ MTK_PMIC_ADC_CHAN(VBIF, PMIC_AUXADC_RQST0, 11, 8, 1, 1),
+ MTK_PMIC_ADC_CHAN(VCORE_TEMP, PMIC_AUXADC_RQST1, 5, 8, 1, 1),
+ MTK_PMIC_ADC_CHAN(VPROC_TEMP, PMIC_AUXADC_RQST1, 6, 8, 1, 1),
+
+ /* IMP channels */
+ MTK_PMIC_ADC_CHAN(VBAT, 0, 0, 128, 3, 1),
+};
+
+static const u16 mt6357_auxadc_regs[] = {
+ [PMIC_HK_TOP_RST_CON0] = 0xf90,
+ [PMIC_AUXADC_DCM_CON] = 0x122e,
+ [PMIC_AUXADC_ADC0] = 0x1088,
+ [PMIC_AUXADC_IMP0] = 0x119c,
+ [PMIC_AUXADC_IMP1] = 0x119e,
+ [PMIC_AUXADC_RQST0] = 0x110e,
+ [PMIC_AUXADC_RQST1] = 0x1114,
+};
+
+static const struct iio_chan_spec mt6358_auxadc_channels[] = {
+ MTK_PMIC_IIO_CHAN(MT6358, bat_adc, BATADC, 0, 15, IIO_RESISTANCE),
+ MTK_PMIC_IIO_CHAN(MT6358, cdt_v, VCDT, 2, 12, IIO_TEMP),
+ MTK_PMIC_IIO_CHAN(MT6358, batt_temp, BAT_TEMP, 3, 12, IIO_TEMP),
+ MTK_PMIC_IIO_CHAN(MT6358, chip_temp, CHIP_TEMP, 4, 12, IIO_TEMP),
+ MTK_PMIC_IIO_CHAN(MT6358, acc_det, ACCDET, 5, 12, IIO_RESISTANCE),
+ MTK_PMIC_IIO_CHAN(MT6358, dcxo_v, VDCXO, 6, 12, IIO_VOLTAGE),
+ MTK_PMIC_IIO_CHAN(MT6358, tsx_temp, TSX_TEMP, 7, 15, IIO_TEMP),
+ MTK_PMIC_IIO_CHAN(MT6358, hp_ofs_cal, HPOFS_CAL, 9, 15, IIO_RESISTANCE),
+ MTK_PMIC_IIO_CHAN(MT6358, dcxo_temp, DCXO_TEMP, 10, 15, IIO_TEMP),
+ MTK_PMIC_IIO_CHAN(MT6358, bif_v, VBIF, 11, 12, IIO_VOLTAGE),
+ MTK_PMIC_IIO_CHAN(MT6358, vcore_temp, VCORE_TEMP, 38, 12, IIO_TEMP),
+ MTK_PMIC_IIO_CHAN(MT6358, vproc_temp, VPROC_TEMP, 39, 12, IIO_TEMP),
+ MTK_PMIC_IIO_CHAN(MT6358, vgpu_temp, VGPU_TEMP, 40, 12, IIO_TEMP),
+
+ /* IMP channels */
+ MTK_PMIC_IIO_CHAN(MT6358, batt_v, VBAT, 0, 15, IIO_VOLTAGE),
+};
+
+static const struct mtk_pmic_auxadc_chan mt6358_auxadc_ch_desc[] = {
+ MTK_PMIC_ADC_CHAN(BATADC, PMIC_AUXADC_RQST0, 0, 128, 3, 1),
+ MTK_PMIC_ADC_CHAN(VCDT, PMIC_AUXADC_RQST0, 0, 8, 1, 1),
+ MTK_PMIC_ADC_CHAN(BAT_TEMP, PMIC_AUXADC_RQST0, 3, 8, 2, 1),
+ MTK_PMIC_ADC_CHAN(CHIP_TEMP, PMIC_AUXADC_RQST0, 4, 8, 1, 1),
+ MTK_PMIC_ADC_CHAN(ACCDET, PMIC_AUXADC_RQST0, 5, 8, 1, 1),
+ MTK_PMIC_ADC_CHAN(VDCXO, PMIC_AUXADC_RQST0, 6, 8, 3, 2),
+ MTK_PMIC_ADC_CHAN(TSX_TEMP, PMIC_AUXADC_RQST0, 7, 128, 1, 1),
+ MTK_PMIC_ADC_CHAN(HPOFS_CAL, PMIC_AUXADC_RQST0, 9, 256, 1, 1),
+ MTK_PMIC_ADC_CHAN(DCXO_TEMP, PMIC_AUXADC_RQST0, 10, 16, 1, 1),
+ MTK_PMIC_ADC_CHAN(VBIF, PMIC_AUXADC_RQST0, 11, 8, 2, 1),
+ MTK_PMIC_ADC_CHAN(VCORE_TEMP, PMIC_AUXADC_RQST1, 8, 8, 1, 1),
+ MTK_PMIC_ADC_CHAN(VPROC_TEMP, PMIC_AUXADC_RQST1, 9, 8, 1, 1),
+ MTK_PMIC_ADC_CHAN(VGPU_TEMP, PMIC_AUXADC_RQST1, 10, 8, 1, 1),
+
+ /* IMP channels */
+ MTK_PMIC_ADC_CHAN(VBAT, 0, 0, 128, 7, 2),
+};
+
+static const u16 mt6358_auxadc_regs[] = {
+ [PMIC_HK_TOP_RST_CON0] = 0xf90,
+ [PMIC_AUXADC_DCM_CON] = 0x1260,
+ [PMIC_AUXADC_ADC0] = 0x1088,
+ [PMIC_AUXADC_IMP0] = 0x1208,
+ [PMIC_AUXADC_IMP1] = 0x120a,
+ [PMIC_AUXADC_RQST0] = 0x1108,
+ [PMIC_AUXADC_RQST1] = 0x110a,
+};
+
+static const struct iio_chan_spec mt6359_auxadc_channels[] = {
+ MTK_PMIC_IIO_CHAN(MT6359, bat_adc, BATADC, 0, 15, IIO_RESISTANCE),
+ MTK_PMIC_IIO_CHAN(MT6359, batt_temp, BAT_TEMP, 3, 12, IIO_TEMP),
+ MTK_PMIC_IIO_CHAN(MT6359, chip_temp, CHIP_TEMP, 4, 12, IIO_TEMP),
+ MTK_PMIC_IIO_CHAN(MT6359, acc_det, ACCDET, 5, 12, IIO_RESISTANCE),
+ MTK_PMIC_IIO_CHAN(MT6359, dcxo_v, VDCXO, 6, 12, IIO_VOLTAGE),
+ MTK_PMIC_IIO_CHAN(MT6359, tsx_temp, TSX_TEMP, 7, 15, IIO_TEMP),
+ MTK_PMIC_IIO_CHAN(MT6359, hp_ofs_cal, HPOFS_CAL, 9, 15, IIO_RESISTANCE),
+ MTK_PMIC_IIO_CHAN(MT6359, dcxo_temp, DCXO_TEMP, 10, 15, IIO_TEMP),
+ MTK_PMIC_IIO_CHAN(MT6359, bif_v, VBIF, 11, 12, IIO_VOLTAGE),
+ MTK_PMIC_IIO_CHAN(MT6359, vcore_temp, VCORE_TEMP, 30, 12, IIO_TEMP),
+ MTK_PMIC_IIO_CHAN(MT6359, vproc_temp, VPROC_TEMP, 31, 12, IIO_TEMP),
+ MTK_PMIC_IIO_CHAN(MT6359, vgpu_temp, VGPU_TEMP, 32, 12, IIO_TEMP),
+
+ /* IMP channels */
+ MTK_PMIC_IIO_CHAN(MT6359, batt_v, VBAT, 0, 15, IIO_VOLTAGE),
+ MTK_PMIC_IIO_CHAN(MT6359, batt_i, IBAT, 0, 15, IIO_CURRENT),
+};
+
+static const struct mtk_pmic_auxadc_chan mt6359_auxadc_ch_desc[] = {
+ MTK_PMIC_ADC_CHAN(BATADC, PMIC_AUXADC_RQST0, 0, 128, 7, 2),
+ MTK_PMIC_ADC_CHAN(BAT_TEMP, PMIC_AUXADC_RQST0, 3, 8, 5, 2),
+ MTK_PMIC_ADC_CHAN(CHIP_TEMP, PMIC_AUXADC_RQST0, 4, 8, 1, 1),
+ MTK_PMIC_ADC_CHAN(ACCDET, PMIC_AUXADC_RQST0, 5, 8, 1, 1),
+ MTK_PMIC_ADC_CHAN(VDCXO, PMIC_AUXADC_RQST0, 6, 8, 3, 2),
+ MTK_PMIC_ADC_CHAN(TSX_TEMP, PMIC_AUXADC_RQST0, 7, 128, 1, 1),
+ MTK_PMIC_ADC_CHAN(HPOFS_CAL, PMIC_AUXADC_RQST0, 9, 256, 1, 1),
+ MTK_PMIC_ADC_CHAN(DCXO_TEMP, PMIC_AUXADC_RQST0, 10, 16, 1, 1),
+ MTK_PMIC_ADC_CHAN(VBIF, PMIC_AUXADC_RQST0, 11, 8, 5, 2),
+ MTK_PMIC_ADC_CHAN(VCORE_TEMP, PMIC_AUXADC_RQST1, 8, 8, 1, 1),
+ MTK_PMIC_ADC_CHAN(VPROC_TEMP, PMIC_AUXADC_RQST1, 9, 8, 1, 1),
+ MTK_PMIC_ADC_CHAN(VGPU_TEMP, PMIC_AUXADC_RQST1, 10, 8, 1, 1),
+
+ /* IMP channels */
+ MTK_PMIC_ADC_CHAN(VBAT, 0, 0, 128, 7, 2),
+ MTK_PMIC_ADC_CHAN(IBAT, 0, 0, 128, 7, 2),
+};
+
+static const u16 mt6359_auxadc_regs[] = {
+ [PMIC_FGADC_R_CON0] = 0xd88,
+ [PMIC_HK_TOP_WKEY] = 0xfb4,
+ [PMIC_HK_TOP_RST_CON0] = 0xf90,
+ [PMIC_AUXADC_RQST0] = 0x1108,
+ [PMIC_AUXADC_RQST1] = 0x110a,
+ [PMIC_AUXADC_ADC0] = 0x1088,
+ [PMIC_AUXADC_IMP0] = 0x1208,
+ [PMIC_AUXADC_IMP1] = 0x120a,
+ [PMIC_AUXADC_IMP3] = 0x120e,
+};
+
+static void mt6358_stop_imp_conv(struct mt6359_auxadc *adc_dev)
+{
+ const struct mtk_pmic_auxadc_pdata *pdata = adc_dev->pdata;
+ struct regmap *regmap = adc_dev->regmap;
+
+ regmap_set_bits(regmap, pdata->regs[PMIC_AUXADC_IMP0], MT6358_IMP0_CLEAR);
+ regmap_clear_bits(regmap, pdata->regs[PMIC_AUXADC_IMP0], MT6358_IMP0_CLEAR);
+ regmap_clear_bits(regmap, pdata->regs[PMIC_AUXADC_IMP1], MT6358_IMP1_AUTOREPEAT_EN);
+ regmap_clear_bits(regmap, pdata->regs[PMIC_AUXADC_DCM_CON], MT6358_DCM_CK_SW_EN);
+}
+
+static int mt6358_start_imp_conv(struct mt6359_auxadc *adc_dev)
+{
+ const struct mtk_pmic_auxadc_pdata *pdata = adc_dev->pdata;
+ struct regmap *regmap = adc_dev->regmap;
+ u32 val;
+ int ret;
+
+ regmap_set_bits(regmap, pdata->regs[PMIC_AUXADC_DCM_CON], MT6358_DCM_CK_SW_EN);
+ regmap_set_bits(regmap, pdata->regs[PMIC_AUXADC_IMP1], MT6358_IMP1_AUTOREPEAT_EN);
+
+ ret = regmap_read_poll_timeout(adc_dev->regmap, pdata->regs[PMIC_AUXADC_IMP0],
+ val, !!(val & MT6358_IMP0_IRQ_RDY),
+ IMP_POLL_DELAY_US, AUXADC_TIMEOUT_US);
+ if (ret) {
+ mt6358_stop_imp_conv(adc_dev);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int mt6358_read_imp(struct mt6359_auxadc *adc_dev, int *vbat, int *ibat)
+{
+ const struct mtk_pmic_auxadc_pdata *pdata = adc_dev->pdata;
+ struct regmap *regmap = adc_dev->regmap;
+ u16 reg_adc0 = pdata->regs[PMIC_AUXADC_ADC0];
+ int val_v, ret;
+
+ ret = mt6358_start_imp_conv(adc_dev);
+ if (ret)
+ return ret;
+
+ /* Read the params before stopping */
+ regmap_read(regmap, reg_adc0 + PMIC_AUXADC_ADCx(pdata->imp_adc_num), &val_v);
+
+ mt6358_stop_imp_conv(adc_dev);
+
+ *vbat = val_v;
+ *ibat = 0;
+
+ return 0;
+}
+
+static int mt6359_read_imp(struct mt6359_auxadc *adc_dev, int *vbat, int *ibat)
+{
+ const struct mtk_pmic_auxadc_pdata *pdata = adc_dev->pdata;
+ struct regmap *regmap = adc_dev->regmap;
+ int val_v, val_i, ret;
+ u32 val;
+
+ /* Start conversion */
+ regmap_write(regmap, pdata->regs[PMIC_AUXADC_IMP0], MT6359_IMP0_CONV_EN);
+ ret = regmap_read_poll_timeout(regmap, pdata->regs[PMIC_AUXADC_IMP1],
+ val, !!(val & MT6359_IMP1_IRQ_RDY),
+ IMP_POLL_DELAY_US, AUXADC_TIMEOUT_US);
+
+ /* Stop conversion regardless of the result */
+ regmap_write(regmap, pdata->regs[PMIC_AUXADC_IMP0], 0);
+ if (ret)
+ return ret;
+
+ /* If it succeeded, wait for the registers to be populated */
+ usleep_range(IMP_STOP_DELAY_US, IMP_STOP_DELAY_US + 50);
+
+ ret = regmap_read(regmap, pdata->regs[PMIC_AUXADC_IMP3], &val_v);
+ if (ret)
+ return ret;
+
+ ret = regmap_read(regmap, pdata->regs[PMIC_FGADC_R_CON0], &val_i);
+ if (ret)
+ return ret;
+
+ *vbat = val_v;
+ *ibat = val_i;
+
+ return 0;
+}
+
+static const struct mtk_pmic_auxadc_pdata mt6357_pdata = {
+ .channels = mt6357_auxadc_channels,
+ .num_channels = ARRAY_SIZE(mt6357_auxadc_channels),
+ .desc = mt6357_auxadc_ch_desc,
+ .regs = mt6357_auxadc_regs,
+ .imp_adc_num = MT6357_IMP_ADC_NUM,
+ .read_imp = mt6358_read_imp,
+};
+
+static const struct mtk_pmic_auxadc_pdata mt6358_pdata = {
+ .channels = mt6358_auxadc_channels,
+ .num_channels = ARRAY_SIZE(mt6358_auxadc_channels),
+ .desc = mt6358_auxadc_ch_desc,
+ .regs = mt6358_auxadc_regs,
+ .imp_adc_num = MT6358_IMP_ADC_NUM,
+ .read_imp = mt6358_read_imp,
+};
+
+static const struct mtk_pmic_auxadc_pdata mt6359_pdata = {
+ .channels = mt6359_auxadc_channels,
+ .num_channels = ARRAY_SIZE(mt6359_auxadc_channels),
+ .desc = mt6359_auxadc_ch_desc,
+ .regs = mt6359_auxadc_regs,
+ .sec_unlock_key = 0x6359,
+ .read_imp = mt6359_read_imp,
+};
+
+static void mt6359_auxadc_reset(struct mt6359_auxadc *adc_dev)
+{
+ const struct mtk_pmic_auxadc_pdata *pdata = adc_dev->pdata;
+ struct regmap *regmap = adc_dev->regmap;
+
+ /* Unlock HK_TOP writes */
+ if (pdata->sec_unlock_key)
+ regmap_write(regmap, pdata->regs[PMIC_HK_TOP_WKEY], pdata->sec_unlock_key);
+
+ /* Assert ADC reset */
+ regmap_set_bits(regmap, pdata->regs[PMIC_HK_TOP_RST_CON0], PMIC_RG_RESET_VAL);
+
+ /* De-assert ADC reset */
+ regmap_clear_bits(regmap, pdata->regs[PMIC_HK_TOP_RST_CON0], PMIC_RG_RESET_VAL);
+
+ /* Lock HK_TOP writes again */
+ if (pdata->sec_unlock_key)
+ regmap_write(regmap, pdata->regs[PMIC_HK_TOP_WKEY], 0);
+}
+
+static int mt6359_auxadc_read_adc(struct mt6359_auxadc *adc_dev,
+ const struct iio_chan_spec *chan, int *out)
+{
+ const struct mtk_pmic_auxadc_pdata *pdata = adc_dev->pdata;
+ const struct mtk_pmic_auxadc_chan *desc = &pdata->desc[chan->scan_index];
+ struct regmap *regmap = adc_dev->regmap;
+ u32 val;
+ int ret;
+
+ /* Request to start sampling for ADC channel */
+ ret = regmap_write(regmap, pdata->regs[desc->req_idx], desc->req_mask);
+ if (ret)
+ return ret;
+
+ /* Wait until all samples are averaged */
+ usleep_range(desc->num_samples * AUXADC_AVG_TIME_US,
+ (desc->num_samples + 1) * AUXADC_AVG_TIME_US);
+
+ ret = regmap_read_poll_timeout(regmap,
+ (pdata->regs[PMIC_AUXADC_ADC0] +
+ PMIC_AUXADC_ADCx(chan->address)),
+ val, (val & PMIC_AUXADC_RDY_BIT),
+ AUXADC_POLL_DELAY_US, AUXADC_TIMEOUT_US);
+ if (ret)
+ return ret;
+
+ /* Stop sampling */
+ regmap_write(regmap, pdata->regs[desc->req_idx], 0);
+
+ *out = val & GENMASK(chan->scan_type.realbits - 1, 0);
+ return 0;
+}
+
+static int mt6359_auxadc_read_label(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan, char *label)
+{
+ return sysfs_emit(label, "%s\n", chan->datasheet_name);
+}
+
+static int mt6359_auxadc_read_raw(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ int *val, int *val2, long mask)
+{
+ struct mt6359_auxadc *adc_dev = iio_priv(indio_dev);
+ const struct mtk_pmic_auxadc_pdata *pdata = adc_dev->pdata;
+ const struct mtk_pmic_auxadc_chan *desc = &pdata->desc[chan->scan_index];
+ int ret;
+
+ if (mask == IIO_CHAN_INFO_SCALE) {
+ *val = desc->r_numerator * AUXADC_VOLT_FULL;
+
+ if (desc->r_denominator > 1) {
+ *val2 = desc->r_denominator;
+ return IIO_VAL_FRACTIONAL;
+ }
+
+ return IIO_VAL_INT;
+ }
+
+ mutex_lock(&adc_dev->lock);
+
+ switch (chan->scan_index) {
+ case PMIC_AUXADC_CHAN_IBAT:
+ ret = adc_dev->pdata->read_imp(adc_dev, val2, val);
+ break;
+ case PMIC_AUXADC_CHAN_VBAT:
+ ret = adc_dev->pdata->read_imp(adc_dev, val, val2);
+ break;
+ default:
+ ret = mt6359_auxadc_read_adc(adc_dev, chan, val);
+ break;
+ }
+
+ mutex_unlock(&adc_dev->lock);
+
+ if (ret) {
+ /*
+ * If we get more than one timeout, it's possible that the
+ * AUXADC is stuck: perform a full reset to recover it.
+ */
+ if (ret == -ETIMEDOUT) {
+ if (adc_dev->timed_out) {
+ dev_warn(adc_dev->dev, "Resetting stuck ADC!\r\n");
+ mt6359_auxadc_reset(adc_dev);
+ }
+ adc_dev->timed_out = true;
+ }
+ return ret;
+ }
+ adc_dev->timed_out = false;
+
+ return IIO_VAL_INT;
+}
+
+static const struct iio_info mt6359_auxadc_info = {
+ .read_label = mt6359_auxadc_read_label,
+ .read_raw = mt6359_auxadc_read_raw,
+};
+
+static int mt6359_auxadc_probe(struct platform_device *pdev)
+{
+ struct device *mt6397_mfd_dev = pdev->dev.parent;
+ struct mt6359_auxadc *adc_dev;
+ struct iio_dev *indio_dev;
+ struct regmap *regmap;
+ int ret;
+
+ /* Regmap is from SoC PMIC Wrapper, parent of the mt6397 MFD */
+ regmap = dev_get_regmap(mt6397_mfd_dev->parent, NULL);
+ if (!regmap)
+ return dev_err_probe(&pdev->dev, -ENODEV, "Failed to get regmap\n");
+
+ indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*adc_dev));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ adc_dev = iio_priv(indio_dev);
+ adc_dev->regmap = regmap;
+ adc_dev->dev = &pdev->dev;
+
+ adc_dev->pdata = device_get_match_data(&pdev->dev);
+ if (!adc_dev->pdata)
+ return -EINVAL;
+
+ mutex_init(&adc_dev->lock);
+
+ mt6359_auxadc_reset(adc_dev);
+
+ indio_dev->dev.parent = &pdev->dev;
+ indio_dev->name = dev_name(&pdev->dev);
+ indio_dev->info = &mt6359_auxadc_info;
+ indio_dev->modes = INDIO_DIRECT_MODE;
+ indio_dev->channels = adc_dev->pdata->channels;
+ indio_dev->num_channels = adc_dev->pdata->num_channels;
+
+ ret = devm_iio_device_register(&pdev->dev, indio_dev);
+ if (ret < 0)
+ return dev_err_probe(&pdev->dev, ret, "failed to register iio device\n");
+
+ return 0;
+}
+
+static const struct of_device_id mt6359_auxadc_of_match[] = {
+ { .compatible = "mediatek,mt6357-auxadc", .data = &mt6357_pdata },
+ { .compatible = "mediatek,mt6358-auxadc", .data = &mt6358_pdata },
+ { .compatible = "mediatek,mt6359-auxadc", .data = &mt6359_pdata },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, mt6359_auxadc_of_match);
+
+static struct platform_driver mt6359_auxadc_driver = {
+ .driver = {
+ .name = "mt6359-auxadc",
+ .of_match_table = mt6359_auxadc_of_match,
+ },
+ .probe = mt6359_auxadc_probe,
+};
+module_platform_driver(mt6359_auxadc_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>");
+MODULE_DESCRIPTION("MediaTek MT6359 PMIC AUXADC Driver");
--
2.45.1On Thu, 30 May 2024 11:34:08 +0200
AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote:
> Add a driver to support reading the Auxiliary ADC IP found in the
> MediaTek MT6357, MT6358 and MT6359 Power Management ICs.
>
> This driver provides multiple ADC channels for system monitoring,
> such as battery voltage, PMIC temperature, PMIC-internal voltage
> regulators temperature, and others.
>
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
I'll leave you to answer the 'why a new driver' in response to Andy's review
and just assume it makes sense whilst reviewing this.
What are IMP channels?
A few additional comments inline.
Thanks,
Jonathan
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index edb32ce2af02..da7d4452b1e0 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -79,6 +79,7 @@ obj-$(CONFIG_MCP320X) += mcp320x.o
> obj-$(CONFIG_MCP3422) += mcp3422.o
> obj-$(CONFIG_MCP3564) += mcp3564.o
> obj-$(CONFIG_MCP3911) += mcp3911.o
> +obj-$(CONFIG_MEDIATEK_MT6359_AUXADC) += mt6359-auxadc.o
> obj-$(CONFIG_MEDIATEK_MT6360_ADC) += mt6360-adc.o
> obj-$(CONFIG_MEDIATEK_MT6370_ADC) += mt6370-adc.o
> obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o
> diff --git a/drivers/iio/adc/mt6359-auxadc.c b/drivers/iio/adc/mt6359-auxadc.c
> new file mode 100644
> index 000000000000..0481bd3f0144
> --- /dev/null
> +++ b/drivers/iio/adc/mt6359-auxadc.c
> @@ -0,0 +1,598 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * MediaTek MT6359 PMIC AUXADC IIO driver
> + *
> + * Copyright (c) 2021 MediaTek Inc.
> + * Copyright (c) 2024 Collabora Ltd
> + * Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/iio/iio.h>
> +#include <linux/mfd/mt6397/core.h>
> +
> +#include <dt-bindings/iio/adc/mediatek,mt6357-auxadc.h>
> +#include <dt-bindings/iio/adc/mediatek,mt6358-auxadc.h>
> +#include <dt-bindings/iio/adc/mediatek,mt6359-auxadc.h>
So I 'guess' these headers are dt-bindings because you want
to consume them from other drivers? That's fine, but if so please
add info on that to the DT binding patch.
> +/**
> + * struct mt6359_auxadc - Main driver structure
> + * @dev: Device pointer
> + * @regmap: Regmap from SoC PMIC Wrapper
> + * @pdata: PMIC specific platform data
> + * @lock: Mutex lock for AUXADC reads
Expand on this mutex comment. What is it protecting?
I think it's about ensuring they are serialized to ensure configuration
is not changed during the read sequence.
> + * @timed_out: Signals whether the last read timed out
> + */
> +struct mt6359_auxadc {
> + struct device *dev;
> + struct regmap *regmap;
> + const struct mtk_pmic_auxadc_pdata *pdata;
> + struct mutex lock;
> + bool timed_out;
> +};
> +
> +/**
> + * struct mtk_pmic_auxadc_pdata - PMIC specific platform data
I'm not sure this is conventionally what we think of as platform
data. This is chip specific stuff. Platform data tends to be
more about how things are wired up etc. A common term for this
stuff is chip_info.
> + * @channels: IIO specification of ADC channels
> + * @num_channels: Number of ADC channels
> + * @desc: PMIC AUXADC channel data
> + * @regs: List of PMIC specific registers
> + * @sec_unlock_key: Security unlock key for HK_TOP writes
> + * @imp_adc_num: ADC channel for IMP readings
> + * @read_imp: Callback to read PMIC IMP channels
> + */
> +struct mtk_pmic_auxadc_pdata {
> + const struct iio_chan_spec *channels;
> + int num_channels;
> + const struct mtk_pmic_auxadc_chan *desc;
> + const u16 *regs;
> + u16 sec_unlock_key;
> + u8 imp_adc_num;
> + int (*read_imp)(struct mt6359_auxadc *adc_dev, int *vbat, int *ibat);
> +};
> +
> +
> +static const struct mtk_pmic_auxadc_chan mt6359_auxadc_ch_desc[] = {
> + MTK_PMIC_ADC_CHAN(BATADC, PMIC_AUXADC_RQST0, 0, 128, 7, 2),
> + MTK_PMIC_ADC_CHAN(BAT_TEMP, PMIC_AUXADC_RQST0, 3, 8, 5, 2),
> + MTK_PMIC_ADC_CHAN(CHIP_TEMP, PMIC_AUXADC_RQST0, 4, 8, 1, 1),
> + MTK_PMIC_ADC_CHAN(ACCDET, PMIC_AUXADC_RQST0, 5, 8, 1, 1),
> + MTK_PMIC_ADC_CHAN(VDCXO, PMIC_AUXADC_RQST0, 6, 8, 3, 2),
> + MTK_PMIC_ADC_CHAN(TSX_TEMP, PMIC_AUXADC_RQST0, 7, 128, 1, 1),
> + MTK_PMIC_ADC_CHAN(HPOFS_CAL, PMIC_AUXADC_RQST0, 9, 256, 1, 1),
> + MTK_PMIC_ADC_CHAN(DCXO_TEMP, PMIC_AUXADC_RQST0, 10, 16, 1, 1),
> + MTK_PMIC_ADC_CHAN(VBIF, PMIC_AUXADC_RQST0, 11, 8, 5, 2),
> + MTK_PMIC_ADC_CHAN(VCORE_TEMP, PMIC_AUXADC_RQST1, 8, 8, 1, 1),
> + MTK_PMIC_ADC_CHAN(VPROC_TEMP, PMIC_AUXADC_RQST1, 9, 8, 1, 1),
> + MTK_PMIC_ADC_CHAN(VGPU_TEMP, PMIC_AUXADC_RQST1, 10, 8, 1, 1),
> +
> + /* IMP channels */
What are these? Expand IMP perhaps!
> + MTK_PMIC_ADC_CHAN(VBAT, 0, 0, 128, 7, 2),
> + MTK_PMIC_ADC_CHAN(IBAT, 0, 0, 128, 7, 2),
> +};
> +static int mt6359_read_imp(struct mt6359_auxadc *adc_dev, int *vbat, int *ibat)
> +{
> + const struct mtk_pmic_auxadc_pdata *pdata = adc_dev->pdata;
> + struct regmap *regmap = adc_dev->regmap;
> + int val_v, val_i, ret;
> + u32 val;
> +
> + /* Start conversion */
> + regmap_write(regmap, pdata->regs[PMIC_AUXADC_IMP0], MT6359_IMP0_CONV_EN);
> + ret = regmap_read_poll_timeout(regmap, pdata->regs[PMIC_AUXADC_IMP1],
> + val, !!(val & MT6359_IMP1_IRQ_RDY),
The condition is just as true or false without the !! so drop those.
> + IMP_POLL_DELAY_US, AUXADC_TIMEOUT_US);
> +
> + /* Stop conversion regardless of the result */
> + regmap_write(regmap, pdata->regs[PMIC_AUXADC_IMP0], 0);
> + if (ret)
> + return ret;
> +
> + /* If it succeeded, wait for the registers to be populated */
> + usleep_range(IMP_STOP_DELAY_US, IMP_STOP_DELAY_US + 50);
> +
> + ret = regmap_read(regmap, pdata->regs[PMIC_AUXADC_IMP3], &val_v);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(regmap, pdata->regs[PMIC_FGADC_R_CON0], &val_i);
> + if (ret)
> + return ret;
Why read them both if only one is wanted? Do you need to read the data
for some reason - i.e. to allow for fresh reads or clear some status bit
or similar? If so add a comment. Otherwise easy to do
if (vbat) {
int val_v;
ret = regmap_read(regmap, pdata->regs[PMIC_AUXADC_IMP3], &val_v);
if (ret)
return ret;
*vbat = val_v;
}
etc
> +
> + *vbat = val_v;
> + *ibat = val_i;
> +
> + return 0;
> +}
> +
> +static const struct mtk_pmic_auxadc_pdata mt6357_pdata = {
> + .channels = mt6357_auxadc_channels,
> + .num_channels = ARRAY_SIZE(mt6357_auxadc_channels),
> + .desc = mt6357_auxadc_ch_desc,
> + .regs = mt6357_auxadc_regs,
> + .imp_adc_num = MT6357_IMP_ADC_NUM,
> + .read_imp = mt6358_read_imp,
> +};
> +
> +static const struct mtk_pmic_auxadc_pdata mt6358_pdata = {
> + .channels = mt6358_auxadc_channels,
> + .num_channels = ARRAY_SIZE(mt6358_auxadc_channels),
> + .desc = mt6358_auxadc_ch_desc,
> + .regs = mt6358_auxadc_regs,
> + .imp_adc_num = MT6358_IMP_ADC_NUM,
> + .read_imp = mt6358_read_imp,
> +};
> +
> +static const struct mtk_pmic_auxadc_pdata mt6359_pdata = {
> + .channels = mt6359_auxadc_channels,
> + .num_channels = ARRAY_SIZE(mt6359_auxadc_channels),
> + .desc = mt6359_auxadc_ch_desc,
> + .regs = mt6359_auxadc_regs,
> + .sec_unlock_key = 0x6359,
> + .read_imp = mt6359_read_imp,
> +};
>
> +
> +static int mt6359_auxadc_read_label(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan, char *label)
> +{
> + return sysfs_emit(label, "%s\n", chan->datasheet_name);
> +}
> +
> +static int mt6359_auxadc_read_raw(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + int *val, int *val2, long mask)
> +{
> + struct mt6359_auxadc *adc_dev = iio_priv(indio_dev);
> + const struct mtk_pmic_auxadc_pdata *pdata = adc_dev->pdata;
> + const struct mtk_pmic_auxadc_chan *desc = &pdata->desc[chan->scan_index];
> + int ret;
> +
> + if (mask == IIO_CHAN_INFO_SCALE) {
> + *val = desc->r_numerator * AUXADC_VOLT_FULL;
> +
> + if (desc->r_denominator > 1) {
> + *val2 = desc->r_denominator;
> + return IIO_VAL_FRACTIONAL;
> + }
> +
> + return IIO_VAL_INT;
> + }
> +
> + mutex_lock(&adc_dev->lock);
> +
> + switch (chan->scan_index) {
> + case PMIC_AUXADC_CHAN_IBAT:
> + ret = adc_dev->pdata->read_imp(adc_dev, val2, val);
This is very odd. Why reverse the parameters between the IBAT and VBAT
channels? I suspect you want to throw away a parameter. That's fine but
don't use val2 for it. Either make that function handle NULL pointers
or add a local int temp or similar for this purpose.
> + break;
> + case PMIC_AUXADC_CHAN_VBAT:
> + ret = adc_dev->pdata->read_imp(adc_dev, val, val2);
> + break;
> + default:
> + ret = mt6359_auxadc_read_adc(adc_dev, chan, val);
> + break;
> + }
> +
> + mutex_unlock(&adc_dev->lock);
> +
> + if (ret) {
> + /*
> + * If we get more than one timeout, it's possible that the
> + * AUXADC is stuck: perform a full reset to recover it.
> + */
> + if (ret == -ETIMEDOUT) {
> + if (adc_dev->timed_out) {
> + dev_warn(adc_dev->dev, "Resetting stuck ADC!\r\n");
> + mt6359_auxadc_reset(adc_dev);
> + }
> + adc_dev->timed_out = true;
> + }
> + return ret;
> + }
> + adc_dev->timed_out = false;
> +
> + return IIO_VAL_INT;
> +}
> +
> +static const struct iio_info mt6359_auxadc_info = {
> + .read_label = mt6359_auxadc_read_label,
> + .read_raw = mt6359_auxadc_read_raw,
> +};
> +
> +static int mt6359_auxadc_probe(struct platform_device *pdev)
> +{
> + struct device *mt6397_mfd_dev = pdev->dev.parent;
> + struct mt6359_auxadc *adc_dev;
> + struct iio_dev *indio_dev;
> + struct regmap *regmap;
> + int ret;
> +
> + /* Regmap is from SoC PMIC Wrapper, parent of the mt6397 MFD */
> + regmap = dev_get_regmap(mt6397_mfd_dev->parent, NULL);
> + if (!regmap)
> + return dev_err_probe(&pdev->dev, -ENODEV, "Failed to get regmap\n");
> +
> + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*adc_dev));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + adc_dev = iio_priv(indio_dev);
> + adc_dev->regmap = regmap;
> + adc_dev->dev = &pdev->dev;
> +
> + adc_dev->pdata = device_get_match_data(&pdev->dev);
> + if (!adc_dev->pdata)
> + return -EINVAL;
> +
> + mutex_init(&adc_dev->lock);
> +
> + mt6359_auxadc_reset(adc_dev);
> +
> + indio_dev->dev.parent = &pdev->dev;
No need to set that, the IIO core does it for you in
devm_iio_device_alloc()
> + indio_dev->name = dev_name(&pdev->dev);
This tends to be fragile at best. The name should be the part number, best
way to reliably get that is either to query a whoami type register if there
is one or put it in your pdata.
> + indio_dev->info = &mt6359_auxadc_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->channels = adc_dev->pdata->channels;
> + indio_dev->num_channels = adc_dev->pdata->num_channels;
> +
> + ret = devm_iio_device_register(&pdev->dev, indio_dev);
> + if (ret < 0)
> + return dev_err_probe(&pdev->dev, ret, "failed to register iio device\n");
> +
> + return 0;
> +}
Il 02/06/24 12:11, Jonathan Cameron ha scritto:
> On Thu, 30 May 2024 11:34:08 +0200
> AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote:
>
>> Add a driver to support reading the Auxiliary ADC IP found in the
>> MediaTek MT6357, MT6358 and MT6359 Power Management ICs.
>>
>> This driver provides multiple ADC channels for system monitoring,
>> such as battery voltage, PMIC temperature, PMIC-internal voltage
>> regulators temperature, and others.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>
> I'll leave you to answer the 'why a new driver' in response to Andy's review
> and just assume it makes sense whilst reviewing this.
>
Simply, the Auxiliary ADC IP in the MT635x PMIC series is different, and handled
differently from the others that are supported by other drivers, other than having
a different register layout.
Adding this to any other driver, such as mt6577_auxadc (a SoC - not PMIC - auxadc
IP driver!), would be like having two different drivers in one and wouldn't make
any sense :-)
> What are IMP channels?
>
Honestly? Well, it's called like that. I don't have any clear description of that
and not even datasheets are unrolling the meaning of "IMP". So.. I don't know.
What I know is what I wrote in the driver, and this is:
* IMP has IBAT and VBAT ADCs
* It needs different handling from the other ADCs, as shown.
...and nothing else :-(
> A few additional comments inline.
>
> Thanks,
>
> Jonathan
>
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>> index edb32ce2af02..da7d4452b1e0 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -79,6 +79,7 @@ obj-$(CONFIG_MCP320X) += mcp320x.o
>> obj-$(CONFIG_MCP3422) += mcp3422.o
>> obj-$(CONFIG_MCP3564) += mcp3564.o
>> obj-$(CONFIG_MCP3911) += mcp3911.o
>> +obj-$(CONFIG_MEDIATEK_MT6359_AUXADC) += mt6359-auxadc.o
>> obj-$(CONFIG_MEDIATEK_MT6360_ADC) += mt6360-adc.o
>> obj-$(CONFIG_MEDIATEK_MT6370_ADC) += mt6370-adc.o
>> obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o
>> diff --git a/drivers/iio/adc/mt6359-auxadc.c b/drivers/iio/adc/mt6359-auxadc.c
>> new file mode 100644
>> index 000000000000..0481bd3f0144
>> --- /dev/null
>> +++ b/drivers/iio/adc/mt6359-auxadc.c
>> @@ -0,0 +1,598 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * MediaTek MT6359 PMIC AUXADC IIO driver
>> + *
>> + * Copyright (c) 2021 MediaTek Inc.
>> + * Copyright (c) 2024 Collabora Ltd
>> + * Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> + */
>> +
>> +#include <linux/delay.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/mfd/mt6397/core.h>
>> +
>> +#include <dt-bindings/iio/adc/mediatek,mt6357-auxadc.h>
>> +#include <dt-bindings/iio/adc/mediatek,mt6358-auxadc.h>
>> +#include <dt-bindings/iio/adc/mediatek,mt6359-auxadc.h>
>
> So I 'guess' these headers are dt-bindings because you want
> to consume them from other drivers? That's fine, but if so please
> add info on that to the DT binding patch.
>
...consume from devicetrees. And the purpose of including those headers is to
index the channels as per the bindings, of course.
That was a *very* light explanation, but I really don't think that I have to
expand at all...
>> +/**
>> + * struct mt6359_auxadc - Main driver structure
>> + * @dev: Device pointer
>> + * @regmap: Regmap from SoC PMIC Wrapper
>> + * @pdata: PMIC specific platform data
>> + * @lock: Mutex lock for AUXADC reads
>
> Expand on this mutex comment. What is it protecting?
> I think it's about ensuring they are serialized to ensure configuration
> is not changed during the read sequence.
>
Yes, exactly that.
"Mutex to serialize AUXADC reading vs configuration"
Looks good? :-)
>> + * @timed_out: Signals whether the last read timed out
>> + */
>> +struct mt6359_auxadc {
>> + struct device *dev;
>> + struct regmap *regmap;
>> + const struct mtk_pmic_auxadc_pdata *pdata;
>> + struct mutex lock;
>> + bool timed_out;
>> +};
>> +
>
>> +/**
>> + * struct mtk_pmic_auxadc_pdata - PMIC specific platform data
>
> I'm not sure this is conventionally what we think of as platform
> data. This is chip specific stuff. Platform data tends to be
> more about how things are wired up etc. A common term for this
> stuff is chip_info.
>
Alright, I can do...
/**
* struct mtk_pmic_auxadc_info - PMIC specific chip info
......
struct mtk_pmic_auxadc_info {
members;
}
struct mtk_pmic_auxadc_info *cinfo = adc_dev->chip_info;
"cinfo" because otherwise it's going to impact code readability later on
>> + * @channels: IIO specification of ADC channels
>> + * @num_channels: Number of ADC channels
>> + * @desc: PMIC AUXADC channel data
>> + * @regs: List of PMIC specific registers
>> + * @sec_unlock_key: Security unlock key for HK_TOP writes
>> + * @imp_adc_num: ADC channel for IMP readings
>> + * @read_imp: Callback to read PMIC IMP channels
>> + */
>> +struct mtk_pmic_auxadc_pdata {
>> + const struct iio_chan_spec *channels;
>> + int num_channels;
>> + const struct mtk_pmic_auxadc_chan *desc;
>> + const u16 *regs;
>> + u16 sec_unlock_key;
>> + u8 imp_adc_num;
>> + int (*read_imp)(struct mt6359_auxadc *adc_dev, int *vbat, int *ibat);
>> +};
>> +
>
>> +
>> +static const struct mtk_pmic_auxadc_chan mt6359_auxadc_ch_desc[] = {
>> + MTK_PMIC_ADC_CHAN(BATADC, PMIC_AUXADC_RQST0, 0, 128, 7, 2),
>> + MTK_PMIC_ADC_CHAN(BAT_TEMP, PMIC_AUXADC_RQST0, 3, 8, 5, 2),
>> + MTK_PMIC_ADC_CHAN(CHIP_TEMP, PMIC_AUXADC_RQST0, 4, 8, 1, 1),
>> + MTK_PMIC_ADC_CHAN(ACCDET, PMIC_AUXADC_RQST0, 5, 8, 1, 1),
>> + MTK_PMIC_ADC_CHAN(VDCXO, PMIC_AUXADC_RQST0, 6, 8, 3, 2),
>> + MTK_PMIC_ADC_CHAN(TSX_TEMP, PMIC_AUXADC_RQST0, 7, 128, 1, 1),
>> + MTK_PMIC_ADC_CHAN(HPOFS_CAL, PMIC_AUXADC_RQST0, 9, 256, 1, 1),
>> + MTK_PMIC_ADC_CHAN(DCXO_TEMP, PMIC_AUXADC_RQST0, 10, 16, 1, 1),
>> + MTK_PMIC_ADC_CHAN(VBIF, PMIC_AUXADC_RQST0, 11, 8, 5, 2),
>> + MTK_PMIC_ADC_CHAN(VCORE_TEMP, PMIC_AUXADC_RQST1, 8, 8, 1, 1),
>> + MTK_PMIC_ADC_CHAN(VPROC_TEMP, PMIC_AUXADC_RQST1, 9, 8, 1, 1),
>> + MTK_PMIC_ADC_CHAN(VGPU_TEMP, PMIC_AUXADC_RQST1, 10, 8, 1, 1),
>> +
>> + /* IMP channels */
> What are these? Expand IMP perhaps!
>
Yeah, well.. it's... "imp"... can't do anything about that, sorry. :-(
>> + MTK_PMIC_ADC_CHAN(VBAT, 0, 0, 128, 7, 2),
>> + MTK_PMIC_ADC_CHAN(IBAT, 0, 0, 128, 7, 2),
>> +};
>
>
>> +static int mt6359_read_imp(struct mt6359_auxadc *adc_dev, int *vbat, int *ibat)
>> +{
>> + const struct mtk_pmic_auxadc_pdata *pdata = adc_dev->pdata;
>> + struct regmap *regmap = adc_dev->regmap;
>> + int val_v, val_i, ret;
>> + u32 val;
>> +
>> + /* Start conversion */
>> + regmap_write(regmap, pdata->regs[PMIC_AUXADC_IMP0], MT6359_IMP0_CONV_EN);
>> + ret = regmap_read_poll_timeout(regmap, pdata->regs[PMIC_AUXADC_IMP1],
>> + val, !!(val & MT6359_IMP1_IRQ_RDY),
>
> The condition is just as true or false without the !! so drop those.
>
Heh yes, I forgot to clean that up before sending, thanks for catching that!
>> + IMP_POLL_DELAY_US, AUXADC_TIMEOUT_US);
>> +
>> + /* Stop conversion regardless of the result */
>> + regmap_write(regmap, pdata->regs[PMIC_AUXADC_IMP0], 0);
>> + if (ret)
>> + return ret;
>> +
>> + /* If it succeeded, wait for the registers to be populated */
>> + usleep_range(IMP_STOP_DELAY_US, IMP_STOP_DELAY_US + 50);
>> +
>> + ret = regmap_read(regmap, pdata->regs[PMIC_AUXADC_IMP3], &val_v);
>> + if (ret)
>> + return ret;
>> +
>> + ret = regmap_read(regmap, pdata->regs[PMIC_FGADC_R_CON0], &val_i);
>> + if (ret)
>> + return ret;
>
> Why read them both if only one is wanted? Do you need to read the data
> for some reason - i.e. to allow for fresh reads or clear some status bit
> or similar? If so add a comment. Otherwise easy to do
>
Yeah, the state machine inside of the AUXADC IP starts the conversion, takes as
many samples as you allow it to take, then stores an average of those in those
registers that I'm reading here.
Reading the registers resets the state machine, so the next time you start it
you won't start averaging current reads versus old ones...
> if (vbat) {
> int val_v;
> ret = regmap_read(regmap, pdata->regs[PMIC_AUXADC_IMP3], &val_v);
> if (ret)
> return ret;
> *vbat = val_v;
> }
> etc
>
>
>> +
>> + *vbat = val_v;
>> + *ibat = val_i;
>> +
>> + return 0;
>> +}
>> +
>> +static const struct mtk_pmic_auxadc_pdata mt6357_pdata = {
>> + .channels = mt6357_auxadc_channels,
>> + .num_channels = ARRAY_SIZE(mt6357_auxadc_channels),
>> + .desc = mt6357_auxadc_ch_desc,
>> + .regs = mt6357_auxadc_regs,
>> + .imp_adc_num = MT6357_IMP_ADC_NUM,
>> + .read_imp = mt6358_read_imp,
>> +};
>> +
>> +static const struct mtk_pmic_auxadc_pdata mt6358_pdata = {
>> + .channels = mt6358_auxadc_channels,
>> + .num_channels = ARRAY_SIZE(mt6358_auxadc_channels),
>> + .desc = mt6358_auxadc_ch_desc,
>> + .regs = mt6358_auxadc_regs,
>> + .imp_adc_num = MT6358_IMP_ADC_NUM,
>> + .read_imp = mt6358_read_imp,
>> +};
>> +
>> +static const struct mtk_pmic_auxadc_pdata mt6359_pdata = {
>> + .channels = mt6359_auxadc_channels,
>> + .num_channels = ARRAY_SIZE(mt6359_auxadc_channels),
>> + .desc = mt6359_auxadc_ch_desc,
>> + .regs = mt6359_auxadc_regs,
>> + .sec_unlock_key = 0x6359,
>> + .read_imp = mt6359_read_imp,
>> +};
>>
>
>> +
>> +static int mt6359_auxadc_read_label(struct iio_dev *indio_dev,
>> + const struct iio_chan_spec *chan, char *label)
>> +{
>> + return sysfs_emit(label, "%s\n", chan->datasheet_name);
>> +}
>> +
>> +static int mt6359_auxadc_read_raw(struct iio_dev *indio_dev,
>> + const struct iio_chan_spec *chan,
>> + int *val, int *val2, long mask)
>> +{
>> + struct mt6359_auxadc *adc_dev = iio_priv(indio_dev);
>> + const struct mtk_pmic_auxadc_pdata *pdata = adc_dev->pdata;
>> + const struct mtk_pmic_auxadc_chan *desc = &pdata->desc[chan->scan_index];
>> + int ret;
>> +
>> + if (mask == IIO_CHAN_INFO_SCALE) {
>> + *val = desc->r_numerator * AUXADC_VOLT_FULL;
>> +
>> + if (desc->r_denominator > 1) {
>> + *val2 = desc->r_denominator;
>> + return IIO_VAL_FRACTIONAL;
>> + }
>> +
>> + return IIO_VAL_INT;
>> + }
>> +
>> + mutex_lock(&adc_dev->lock);
>> +
>> + switch (chan->scan_index) {
>> + case PMIC_AUXADC_CHAN_IBAT:
>> + ret = adc_dev->pdata->read_imp(adc_dev, val2, val);
>
> This is very odd. Why reverse the parameters between the IBAT and VBAT
> channels? I suspect you want to throw away a parameter. That's fine but
> don't use val2 for it. Either make that function handle NULL pointers
> or add a local int temp or similar for this purpose.
>
I honestly prefer to have a simple variable swap instead of adding handling
for NULL pointers, even though it's just a "if NULL don't use me".
But okay, I also understand why you don't want me to use val2 as a "waste bin"...
I will add NULL pointer handling in the read_imp callback, as that's the shortest
and lightest option that I have here.
case PMIC_AUXADC_CHAN_IBAT:
ret = adc_dev->pdata->read_imp(adc_dev, NULL, val);
break;
case PMIC_AUXADC_CHAN_VBAT:
ret = adc_dev->pdata->read_imp(adc_dev, val, NULL);
break;
read_imp
{
....
if (vbat)
*vbat = val_v;
if (ibat)
*ibat = val_i;
return 0;
}
>> + break;
>> + case PMIC_AUXADC_CHAN_VBAT:
>> + ret = adc_dev->pdata->read_imp(adc_dev, val, val2);
>> + break;
>> + default:
>> + ret = mt6359_auxadc_read_adc(adc_dev, chan, val);
>> + break;
>> + }
>> +
>> + mutex_unlock(&adc_dev->lock);
>> +
>> + if (ret) {
>> + /*
>> + * If we get more than one timeout, it's possible that the
>> + * AUXADC is stuck: perform a full reset to recover it.
>> + */
>> + if (ret == -ETIMEDOUT) {
>> + if (adc_dev->timed_out) {
>> + dev_warn(adc_dev->dev, "Resetting stuck ADC!\r\n");
>> + mt6359_auxadc_reset(adc_dev);
>> + }
>> + adc_dev->timed_out = true;
>> + }
>> + return ret;
>> + }
>> + adc_dev->timed_out = false;
>> +
>> + return IIO_VAL_INT;
>> +}
>> +
>> +static const struct iio_info mt6359_auxadc_info = {
>> + .read_label = mt6359_auxadc_read_label,
>> + .read_raw = mt6359_auxadc_read_raw,
>> +};
>> +
>> +static int mt6359_auxadc_probe(struct platform_device *pdev)
>> +{
>> + struct device *mt6397_mfd_dev = pdev->dev.parent;
>> + struct mt6359_auxadc *adc_dev;
>> + struct iio_dev *indio_dev;
>> + struct regmap *regmap;
>> + int ret;
>> +
>> + /* Regmap is from SoC PMIC Wrapper, parent of the mt6397 MFD */
>> + regmap = dev_get_regmap(mt6397_mfd_dev->parent, NULL);
>> + if (!regmap)
>> + return dev_err_probe(&pdev->dev, -ENODEV, "Failed to get regmap\n");
>> +
>> + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*adc_dev));
>> + if (!indio_dev)
>> + return -ENOMEM;
>> +
>> + adc_dev = iio_priv(indio_dev);
>> + adc_dev->regmap = regmap;
>> + adc_dev->dev = &pdev->dev;
>> +
>> + adc_dev->pdata = device_get_match_data(&pdev->dev);
>> + if (!adc_dev->pdata)
>> + return -EINVAL;
>> +
>> + mutex_init(&adc_dev->lock);
>> +
>> + mt6359_auxadc_reset(adc_dev);
>> +
>> + indio_dev->dev.parent = &pdev->dev;
> No need to set that, the IIO core does it for you in
> devm_iio_device_alloc()
Ah, cool, thank you!
>> + indio_dev->name = dev_name(&pdev->dev);
>
> This tends to be fragile at best. The name should be the part number, best
> way to reliably get that is either to query a whoami type register if there
> is one or put it in your pdata.
>
Okay. I don't want to deal with unknown revision IDs and such, as the auxadc
IP is always the same between all of them and I don't have a clear list of
numbers here, so I'll just throw in the MT6357 MT6358 MT6359 names in the
chip_info and call it a day :-)
Thanks for the review btw!
Cheers,
Angelo
On Tue, 4 Jun 2024 11:42:05 +0200
AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote:
> Il 02/06/24 12:11, Jonathan Cameron ha scritto:
> > On Thu, 30 May 2024 11:34:08 +0200
> > AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote:
> >
> >> Add a driver to support reading the Auxiliary ADC IP found in the
> >> MediaTek MT6357, MT6358 and MT6359 Power Management ICs.
> >>
> >> This driver provides multiple ADC channels for system monitoring,
> >> such as battery voltage, PMIC temperature, PMIC-internal voltage
> >> regulators temperature, and others.
> >>
> >> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> >
> > I'll leave you to answer the 'why a new driver' in response to Andy's review
> > and just assume it makes sense whilst reviewing this.
> >
>
> Simply, the Auxiliary ADC IP in the MT635x PMIC series is different, and handled
> differently from the others that are supported by other drivers, other than having
> a different register layout.
>
> Adding this to any other driver, such as mt6577_auxadc (a SoC - not PMIC - auxadc
> IP driver!), would be like having two different drivers in one and wouldn't make
> any sense :-)
That's fine. I missed the PMIC bit!
>
> > What are IMP channels?
> >
>
> Honestly? Well, it's called like that. I don't have any clear description of that
> and not even datasheets are unrolling the meaning of "IMP". So.. I don't know.
>
> What I know is what I wrote in the driver, and this is:
> * IMP has IBAT and VBAT ADCs
> * It needs different handling from the other ADCs, as shown.
>
> ...and nothing else :-(
This is where kernel developers are rescued by sarcasm.
/* IMP - A small type of devil maybe? */
Let your creativity run wild ;)
Joking really. Just add a note to say that we have no idea what it means.
>
> > A few additional comments inline.
> >
> > Thanks,
> >
> > Jonathan
> >
> >> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> >> index edb32ce2af02..da7d4452b1e0 100644
> >> --- a/drivers/iio/adc/Makefile
> >> +++ b/drivers/iio/adc/Makefile
> >> @@ -79,6 +79,7 @@ obj-$(CONFIG_MCP320X) += mcp320x.o
> >> obj-$(CONFIG_MCP3422) += mcp3422.o
> >> obj-$(CONFIG_MCP3564) += mcp3564.o
> >> obj-$(CONFIG_MCP3911) += mcp3911.o
> >> +obj-$(CONFIG_MEDIATEK_MT6359_AUXADC) += mt6359-auxadc.o
> >> obj-$(CONFIG_MEDIATEK_MT6360_ADC) += mt6360-adc.o
> >> obj-$(CONFIG_MEDIATEK_MT6370_ADC) += mt6370-adc.o
> >> obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o
> >> diff --git a/drivers/iio/adc/mt6359-auxadc.c b/drivers/iio/adc/mt6359-auxadc.c
> >> new file mode 100644
> >> index 000000000000..0481bd3f0144
> >> --- /dev/null
> >> +++ b/drivers/iio/adc/mt6359-auxadc.c
> >> @@ -0,0 +1,598 @@
> >> +// SPDX-License-Identifier: GPL-2.0-only
> >> +/*
> >> + * MediaTek MT6359 PMIC AUXADC IIO driver
> >> + *
> >> + * Copyright (c) 2021 MediaTek Inc.
> >> + * Copyright (c) 2024 Collabora Ltd
> >> + * Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> >> + */
> >> +
> >> +#include <linux/delay.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/module.h>
> >> +#include <linux/of.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/regmap.h>
> >> +#include <linux/iio/iio.h>
> >> +#include <linux/mfd/mt6397/core.h>
> >> +
> >> +#include <dt-bindings/iio/adc/mediatek,mt6357-auxadc.h>
> >> +#include <dt-bindings/iio/adc/mediatek,mt6358-auxadc.h>
> >> +#include <dt-bindings/iio/adc/mediatek,mt6359-auxadc.h>
> >
> > So I 'guess' these headers are dt-bindings because you want
> > to consume them from other drivers? That's fine, but if so please
> > add info on that to the DT binding patch.
> >
>
> ...consume from devicetrees. And the purpose of including those headers is to
> index the channels as per the bindings, of course.
> That was a *very* light explanation, but I really don't think that I have to
> expand at all...
That's fine. Stick it in the patch description for patch 1.
>
> >> +/**
> >> + * struct mt6359_auxadc - Main driver structure
> >> + * @dev: Device pointer
> >> + * @regmap: Regmap from SoC PMIC Wrapper
> >> + * @pdata: PMIC specific platform data
> >> + * @lock: Mutex lock for AUXADC reads
> >
> > Expand on this mutex comment. What is it protecting?
> > I think it's about ensuring they are serialized to ensure configuration
> > is not changed during the read sequence.
> >
>
> Yes, exactly that.
>
> "Mutex to serialize AUXADC reading vs configuration"
> Looks good? :-)
Mutex is obvious - so perhaps.
Serialize multi access AUXADC reading against configuration changes.
>
> >> + * @timed_out: Signals whether the last read timed out
> >> + */
> >> +struct mt6359_auxadc {
> >> + struct device *dev;
> >> + struct regmap *regmap;
> >> + const struct mtk_pmic_auxadc_pdata *pdata;
> >> + struct mutex lock;
> >> + bool timed_out;
> >> +};
> >> +
> >
> >> +/**
> >> + * struct mtk_pmic_auxadc_pdata - PMIC specific platform data
> >
> > I'm not sure this is conventionally what we think of as platform
> > data. This is chip specific stuff. Platform data tends to be
> > more about how things are wired up etc. A common term for this
> > stuff is chip_info.
> >
>
> Alright, I can do...
>
> /**
> * struct mtk_pmic_auxadc_info - PMIC specific chip info
> ......
> struct mtk_pmic_auxadc_info {
> members;
> }
>
>
> struct mtk_pmic_auxadc_info *cinfo = adc_dev->chip_info;
>
> "cinfo" because otherwise it's going to impact code readability later on
>
> >> + * @channels: IIO specification of ADC channels
> >> + * @num_channels: Number of ADC channels
> >> + * @desc: PMIC AUXADC channel data
> >> + * @regs: List of PMIC specific registers
> >> + * @sec_unlock_key: Security unlock key for HK_TOP writes
> >> + * @imp_adc_num: ADC channel for IMP readings
> >> + * @read_imp: Callback to read PMIC IMP channels
> >> + */
> >> +struct mtk_pmic_auxadc_pdata {
> >> + const struct iio_chan_spec *channels;
> >> + int num_channels;
> >> + const struct mtk_pmic_auxadc_chan *desc;
> >> + const u16 *regs;
> >> + u16 sec_unlock_key;
> >> + u8 imp_adc_num;
> >> + int (*read_imp)(struct mt6359_auxadc *adc_dev, int *vbat, int *ibat);
> >> +};
> >> +
> >
> >> +
> >> +static const struct mtk_pmic_auxadc_chan mt6359_auxadc_ch_desc[] = {
> >> + MTK_PMIC_ADC_CHAN(BATADC, PMIC_AUXADC_RQST0, 0, 128, 7, 2),
> >> + MTK_PMIC_ADC_CHAN(BAT_TEMP, PMIC_AUXADC_RQST0, 3, 8, 5, 2),
> >> + MTK_PMIC_ADC_CHAN(CHIP_TEMP, PMIC_AUXADC_RQST0, 4, 8, 1, 1),
> >> + MTK_PMIC_ADC_CHAN(ACCDET, PMIC_AUXADC_RQST0, 5, 8, 1, 1),
> >> + MTK_PMIC_ADC_CHAN(VDCXO, PMIC_AUXADC_RQST0, 6, 8, 3, 2),
> >> + MTK_PMIC_ADC_CHAN(TSX_TEMP, PMIC_AUXADC_RQST0, 7, 128, 1, 1),
> >> + MTK_PMIC_ADC_CHAN(HPOFS_CAL, PMIC_AUXADC_RQST0, 9, 256, 1, 1),
> >> + MTK_PMIC_ADC_CHAN(DCXO_TEMP, PMIC_AUXADC_RQST0, 10, 16, 1, 1),
> >> + MTK_PMIC_ADC_CHAN(VBIF, PMIC_AUXADC_RQST0, 11, 8, 5, 2),
> >> + MTK_PMIC_ADC_CHAN(VCORE_TEMP, PMIC_AUXADC_RQST1, 8, 8, 1, 1),
> >> + MTK_PMIC_ADC_CHAN(VPROC_TEMP, PMIC_AUXADC_RQST1, 9, 8, 1, 1),
> >> + MTK_PMIC_ADC_CHAN(VGPU_TEMP, PMIC_AUXADC_RQST1, 10, 8, 1, 1),
> >> +
> >> + /* IMP channels */
> > What are these? Expand IMP perhaps!
> >
>
> Yeah, well.. it's... "imp"... can't do anything about that, sorry. :-(
It's growing horns and it's definitely a red one...
>
> >> + MTK_PMIC_ADC_CHAN(VBAT, 0, 0, 128, 7, 2),
> >> + MTK_PMIC_ADC_CHAN(IBAT, 0, 0, 128, 7, 2),
> >> +};
> >
> >
> >> +static int mt6359_read_imp(struct mt6359_auxadc *adc_dev, int *vbat, int *ibat)
> >> +{
> >> + const struct mtk_pmic_auxadc_pdata *pdata = adc_dev->pdata;
> >> + struct regmap *regmap = adc_dev->regmap;
> >> + int val_v, val_i, ret;
> >> + u32 val;
> >> +
> >> + /* Start conversion */
> >> + regmap_write(regmap, pdata->regs[PMIC_AUXADC_IMP0], MT6359_IMP0_CONV_EN);
> >> + ret = regmap_read_poll_timeout(regmap, pdata->regs[PMIC_AUXADC_IMP1],
> >> + val, !!(val & MT6359_IMP1_IRQ_RDY),
> >
> > The condition is just as true or false without the !! so drop those.
> >
>
> Heh yes, I forgot to clean that up before sending, thanks for catching that!
>
> >> + IMP_POLL_DELAY_US, AUXADC_TIMEOUT_US);
> >> +
> >> + /* Stop conversion regardless of the result */
> >> + regmap_write(regmap, pdata->regs[PMIC_AUXADC_IMP0], 0);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + /* If it succeeded, wait for the registers to be populated */
> >> + usleep_range(IMP_STOP_DELAY_US, IMP_STOP_DELAY_US + 50);
> >> +
> >> + ret = regmap_read(regmap, pdata->regs[PMIC_AUXADC_IMP3], &val_v);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + ret = regmap_read(regmap, pdata->regs[PMIC_FGADC_R_CON0], &val_i);
> >> + if (ret)
> >> + return ret;
> >
> > Why read them both if only one is wanted? Do you need to read the data
> > for some reason - i.e. to allow for fresh reads or clear some status bit
> > or similar? If so add a comment. Otherwise easy to do
> >
>
> Yeah, the state machine inside of the AUXADC IP starts the conversion, takes as
> many samples as you allow it to take, then stores an average of those in those
> registers that I'm reading here.
>
> Reading the registers resets the state machine, so the next time you start it
> you won't start averaging current reads versus old ones...
Ah ok. Comment please on need to read to reset the register.
Everyone loves read to clear registers... *Sigh*
>
> > if (vbat) {
> > int val_v;
> > ret = regmap_read(regmap, pdata->regs[PMIC_AUXADC_IMP3], &val_v);
> > if (ret)
> > return ret;
> > *vbat = val_v;
> > }
> > etc
> >
> >
> >> +
> >> + *vbat = val_v;
> >> + *ibat = val_i;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static const struct mtk_pmic_auxadc_pdata mt6357_pdata = {
> >> + .channels = mt6357_auxadc_channels,
> >> + .num_channels = ARRAY_SIZE(mt6357_auxadc_channels),
> >> + .desc = mt6357_auxadc_ch_desc,
> >> + .regs = mt6357_auxadc_regs,
> >> + .imp_adc_num = MT6357_IMP_ADC_NUM,
> >> + .read_imp = mt6358_read_imp,
> >> +};
> >> +
> >> +static const struct mtk_pmic_auxadc_pdata mt6358_pdata = {
> >> + .channels = mt6358_auxadc_channels,
> >> + .num_channels = ARRAY_SIZE(mt6358_auxadc_channels),
> >> + .desc = mt6358_auxadc_ch_desc,
> >> + .regs = mt6358_auxadc_regs,
> >> + .imp_adc_num = MT6358_IMP_ADC_NUM,
> >> + .read_imp = mt6358_read_imp,
> >> +};
> >> +
> >> +static const struct mtk_pmic_auxadc_pdata mt6359_pdata = {
> >> + .channels = mt6359_auxadc_channels,
> >> + .num_channels = ARRAY_SIZE(mt6359_auxadc_channels),
> >> + .desc = mt6359_auxadc_ch_desc,
> >> + .regs = mt6359_auxadc_regs,
> >> + .sec_unlock_key = 0x6359,
> >> + .read_imp = mt6359_read_imp,
> >> +};
> >>
> >
> >> +
> >> +static int mt6359_auxadc_read_label(struct iio_dev *indio_dev,
> >> + const struct iio_chan_spec *chan, char *label)
> >> +{
> >> + return sysfs_emit(label, "%s\n", chan->datasheet_name);
> >> +}
> >> +
> >> +static int mt6359_auxadc_read_raw(struct iio_dev *indio_dev,
> >> + const struct iio_chan_spec *chan,
> >> + int *val, int *val2, long mask)
> >> +{
> >> + struct mt6359_auxadc *adc_dev = iio_priv(indio_dev);
> >> + const struct mtk_pmic_auxadc_pdata *pdata = adc_dev->pdata;
> >> + const struct mtk_pmic_auxadc_chan *desc = &pdata->desc[chan->scan_index];
> >> + int ret;
> >> +
> >> + if (mask == IIO_CHAN_INFO_SCALE) {
> >> + *val = desc->r_numerator * AUXADC_VOLT_FULL;
> >> +
> >> + if (desc->r_denominator > 1) {
> >> + *val2 = desc->r_denominator;
> >> + return IIO_VAL_FRACTIONAL;
> >> + }
> >> +
> >> + return IIO_VAL_INT;
> >> + }
> >> +
> >> + mutex_lock(&adc_dev->lock);
> >> +
> >> + switch (chan->scan_index) {
> >> + case PMIC_AUXADC_CHAN_IBAT:
> >> + ret = adc_dev->pdata->read_imp(adc_dev, val2, val);
> >
> > This is very odd. Why reverse the parameters between the IBAT and VBAT
> > channels? I suspect you want to throw away a parameter. That's fine but
> > don't use val2 for it. Either make that function handle NULL pointers
> > or add a local int temp or similar for this purpose.
> >
>
> I honestly prefer to have a simple variable swap instead of adding handling
> for NULL pointers, even though it's just a "if NULL don't use me".
>
> But okay, I also understand why you don't want me to use val2 as a "waste bin"...
Indeed not!
>
> I will add NULL pointer handling in the read_imp callback, as that's the shortest
> and lightest option that I have here.
>
> case PMIC_AUXADC_CHAN_IBAT:
> ret = adc_dev->pdata->read_imp(adc_dev, NULL, val);
> break;
> case PMIC_AUXADC_CHAN_VBAT:
> ret = adc_dev->pdata->read_imp(adc_dev, val, NULL);
that works, or just use a locally defined variable to dump the value you don't
want in if you prefer. int garbage; or similar.
> break;
>
> read_imp
> {
> ....
>
> if (vbat)
> *vbat = val_v;
> if (ibat)
> *ibat = val_i;
>
> return 0;
> }
>
> >> + break;
> >> + case PMIC_AUXADC_CHAN_VBAT:
> >> + ret = adc_dev->pdata->read_imp(adc_dev, val, val2);
> >> + break;
> >> + default:
> >> + ret = mt6359_auxadc_read_adc(adc_dev, chan, val);
> >> + break;
> >> + }
> >> +
> >> + mutex_unlock(&adc_dev->lock);
> >> +
> >> + if (ret) {
> >> + /*
> >> + * If we get more than one timeout, it's possible that the
> >> + * AUXADC is stuck: perform a full reset to recover it.
> >> + */
> >> + if (ret == -ETIMEDOUT) {
> >> + if (adc_dev->timed_out) {
> >> + dev_warn(adc_dev->dev, "Resetting stuck ADC!\r\n");
> >> + mt6359_auxadc_reset(adc_dev);
> >> + }
> >> + adc_dev->timed_out = true;
> >> + }
> >> + return ret;
> >> + }
> >> + adc_dev->timed_out = false;
> >> +
> >> + return IIO_VAL_INT;
> >> +}
> >> +
> >> +static const struct iio_info mt6359_auxadc_info = {
> >> + .read_label = mt6359_auxadc_read_label,
> >> + .read_raw = mt6359_auxadc_read_raw,
> >> +};
> >> +
> >> +static int mt6359_auxadc_probe(struct platform_device *pdev)
> >> +{
> >> + struct device *mt6397_mfd_dev = pdev->dev.parent;
> >> + struct mt6359_auxadc *adc_dev;
> >> + struct iio_dev *indio_dev;
> >> + struct regmap *regmap;
> >> + int ret;
> >> +
> >> + /* Regmap is from SoC PMIC Wrapper, parent of the mt6397 MFD */
> >> + regmap = dev_get_regmap(mt6397_mfd_dev->parent, NULL);
> >> + if (!regmap)
> >> + return dev_err_probe(&pdev->dev, -ENODEV, "Failed to get regmap\n");
> >> +
> >> + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*adc_dev));
> >> + if (!indio_dev)
> >> + return -ENOMEM;
> >> +
> >> + adc_dev = iio_priv(indio_dev);
> >> + adc_dev->regmap = regmap;
> >> + adc_dev->dev = &pdev->dev;
> >> +
> >> + adc_dev->pdata = device_get_match_data(&pdev->dev);
> >> + if (!adc_dev->pdata)
> >> + return -EINVAL;
> >> +
> >> + mutex_init(&adc_dev->lock);
> >> +
> >> + mt6359_auxadc_reset(adc_dev);
> >> +
> >> + indio_dev->dev.parent = &pdev->dev;
> > No need to set that, the IIO core does it for you in
> > devm_iio_device_alloc()
>
> Ah, cool, thank you!
>
> >> + indio_dev->name = dev_name(&pdev->dev);
> >
> > This tends to be fragile at best. The name should be the part number, best
> > way to reliably get that is either to query a whoami type register if there
> > is one or put it in your pdata.
> >
>
> Okay. I don't want to deal with unknown revision IDs and such, as the auxadc
> IP is always the same between all of them and I don't have a clear list of
> numbers here, so I'll just throw in the MT6357 MT6358 MT6359 names in the
> chip_info and call it a day :-)
ok. I think. Note we should allow for the mess of fallback compatibles with
at most an info print if we can tell something is different from expected.
Not sure that is relevant here though.
>
> Thanks for the review btw!
np
Jonathan
>
> Cheers,
> Angelo
>
>
On Tue, Jun 4, 2024 at 12:42 PM AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote: > Il 02/06/24 12:11, Jonathan Cameron ha scritto: > > On Thu, 30 May 2024 11:34:08 +0200 > > AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote: ... > > What are IMP channels? > > Honestly? Well, it's called like that. I don't have any clear description of that > and not even datasheets are unrolling the meaning of "IMP". So.. I don't know. > > What I know is what I wrote in the driver, and this is: > * IMP has IBAT and VBAT ADCs > * It needs different handling from the other ADCs, as shown. > > ...and nothing else :-( I could speculate with confidence that this means IMPedance (since it's ADC). From MTK6329 datasheet: "The hardware also includes necessary modes to allow for simultaneous current and voltage measurement which can be utilized to estimate the battery impedance." And googling in vendor trees also suggests the same. -- With Best Regards, Andy Shevchenko
Il 04/06/24 12:55, Andy Shevchenko ha scritto: > On Tue, Jun 4, 2024 at 12:42 PM AngeloGioacchino Del Regno > <angelogioacchino.delregno@collabora.com> wrote: >> Il 02/06/24 12:11, Jonathan Cameron ha scritto: >>> On Thu, 30 May 2024 11:34:08 +0200 >>> AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote: > > ... > >>> What are IMP channels? >> >> Honestly? Well, it's called like that. I don't have any clear description of that >> and not even datasheets are unrolling the meaning of "IMP". So.. I don't know. >> >> What I know is what I wrote in the driver, and this is: >> * IMP has IBAT and VBAT ADCs >> * It needs different handling from the other ADCs, as shown. >> >> ...and nothing else :-( > > I could speculate with confidence that this means IMPedance (since it's ADC). > > From MTK6329 datasheet: > "The hardware also includes necessary modes to allow for simultaneous > current and voltage measurement > which can be utilized to estimate the battery impedance." > And googling in vendor trees also suggests the same. > That does make a lot of sense. Thanks for that, I don't have the 6329 datasheet. Great, one more piece added to the puzzle! Cheers, Angelo
On Tue, Jun 4, 2024 at 1:55 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Tue, Jun 4, 2024 at 12:42 PM AngeloGioacchino Del Regno > <angelogioacchino.delregno@collabora.com> wrote: > > Il 02/06/24 12:11, Jonathan Cameron ha scritto: > > > On Thu, 30 May 2024 11:34:08 +0200 > > > AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote: ... > > > What are IMP channels? > > > > Honestly? Well, it's called like that. I don't have any clear description of that > > and not even datasheets are unrolling the meaning of "IMP". So.. I don't know. > > > > What I know is what I wrote in the driver, and this is: > > * IMP has IBAT and VBAT ADCs > > * It needs different handling from the other ADCs, as shown. > > > > ...and nothing else :-( > > I could speculate with confidence that this means IMPedance (since it's ADC). > > From MTK6329 datasheet: The datasheet for MTK6359 I found on https://ebin.pub/qdownload/mt6359-pmic-datasheet-15nbsped.html also has the same wording in "3.2.7 Fuel Gauge". > "The hardware also includes necessary modes to allow for simultaneous > current and voltage measurement > which can be utilized to estimate the battery impedance." > And googling vendor trees also suggests the same. -- With Best Regards, Andy Shevchenko
On Thu, May 30, 2024 at 12:34 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Add a driver to support reading the Auxiliary ADC IP found in the
> MediaTek MT6357, MT6358 and MT6359 Power Management ICs.
>
> This driver provides multiple ADC channels for system monitoring,
> such as battery voltage, PMIC temperature, PMIC-internal voltage
> regulators temperature, and others.
> ---
Here is no explanation on how this is differ to any of the three
existing drivers? I.o.w. why do we need a brand new one?
...
+ bits.h
> +#include <linux/delay.h>
> +#include <linux/kernel.h>
Why?
+ mod_devicetable.h
> +#include <linux/module.h>
> +#include <linux/of.h>
Why?
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
+ types.h
+ blank line
> +#include <linux/iio/iio.h>
+ Blank line
> +#include <linux/mfd/mt6397/core.h>
...
> +#define PMIC_RG_RESET_VAL (BIT(0) | BIT(3))
In this form it requires a comment explaining each mentioned bit.
> +#define PMIC_AUXADC_ADCx(x) ((x) << 1)
Seems like a useless macro, it occupies much more space than in-place shift.
...
> +#define MT6358_IMP0_CLEAR (BIT(14) | BIT(7))
As per above.
...
> +/**
> + * struct mtk_pmic_auxadc_chan - PMIC AUXADC channel data
> + * @req_idx: Request register number
> + * @req_mask: Bitmask to activate a channel
> + * @num_samples: Number of AUXADC samples for averaging
> + * @r_numerator: Resistance ratio numerator
> + * @r_denominator: Resistance ratio denominator
> + */
> +struct mtk_pmic_auxadc_chan {
> + u8 req_idx;
> + u16 req_mask;
> + u16 num_samples;
> + u8 r_numerator;
> + u8 r_denominator;
Can you add struct u8_fract to the math.h and use it? I will Ack/R the
respective patch.
> +};
...
> +struct mtk_pmic_auxadc_pdata {
> + const struct iio_chan_spec *channels;
> + int num_channels;
Why signed?
> + const struct mtk_pmic_auxadc_chan *desc;
> + const u16 *regs;
> + u16 sec_unlock_key;
> + u8 imp_adc_num;
> + int (*read_imp)(struct mt6359_auxadc *adc_dev, int *vbat, int *ibat);
> +};
...
> +static const u16 mt6357_auxadc_regs[] = {
> + [PMIC_HK_TOP_RST_CON0] = 0xf90,
Can we use the fixed-width values so all of them will look nice and
easy to parse?
> + [PMIC_AUXADC_DCM_CON] = 0x122e,
> + [PMIC_AUXADC_ADC0] = 0x1088,
> + [PMIC_AUXADC_IMP0] = 0x119c,
> + [PMIC_AUXADC_IMP1] = 0x119e,
> + [PMIC_AUXADC_RQST0] = 0x110e,
> + [PMIC_AUXADC_RQST1] = 0x1114,
> +};
...
> +static const u16 mt6358_auxadc_regs[] = {
Ditto.
> + [PMIC_HK_TOP_RST_CON0] = 0xf90,
> + [PMIC_AUXADC_DCM_CON] = 0x1260,
> + [PMIC_AUXADC_ADC0] = 0x1088,
> + [PMIC_AUXADC_IMP0] = 0x1208,
> + [PMIC_AUXADC_IMP1] = 0x120a,
> + [PMIC_AUXADC_RQST0] = 0x1108,
> + [PMIC_AUXADC_RQST1] = 0x110a,
> +};
...
> +static const u16 mt6359_auxadc_regs[] = {
Ditto.
> + [PMIC_FGADC_R_CON0] = 0xd88,
> + [PMIC_HK_TOP_WKEY] = 0xfb4,
> + [PMIC_HK_TOP_RST_CON0] = 0xf90,
> + [PMIC_AUXADC_RQST0] = 0x1108,
> + [PMIC_AUXADC_RQST1] = 0x110a,
> + [PMIC_AUXADC_ADC0] = 0x1088,
> + [PMIC_AUXADC_IMP0] = 0x1208,
> + [PMIC_AUXADC_IMP1] = 0x120a,
> + [PMIC_AUXADC_IMP3] = 0x120e,
> +};
...
> + ret = regmap_read_poll_timeout(adc_dev->regmap, pdata->regs[PMIC_AUXADC_IMP0],
> + val, !!(val & MT6358_IMP0_IRQ_RDY),
> + IMP_POLL_DELAY_US, AUXADC_TIMEOUT_US);
> + if (ret) {
> + mt6358_stop_imp_conv(adc_dev);
> + return ret;
> + }
> +
> + return 0;
if (ret)
foo()
return ret;
...
> + int val_v, ret;
Why is val_v signed?
...
> + int val_v, val_i, ret;
Ditto for all val_*.
...
> + /* If it succeeded, wait for the registers to be populated */
> + usleep_range(IMP_STOP_DELAY_US, IMP_STOP_DELAY_US + 50);
fsleep() ?
...
> + /* Assert ADC reset */
> + regmap_set_bits(regmap, pdata->regs[PMIC_HK_TOP_RST_CON0], PMIC_RG_RESET_VAL);
No required delay in between?
> + /* De-assert ADC reset */
> + regmap_clear_bits(regmap, pdata->regs[PMIC_HK_TOP_RST_CON0], PMIC_RG_RESET_VAL);
...
> + /* Wait until all samples are averaged */
> + usleep_range(desc->num_samples * AUXADC_AVG_TIME_US,
> + (desc->num_samples + 1) * AUXADC_AVG_TIME_US);
fsleep()
...
> + ret = regmap_read_poll_timeout(regmap,
> + (pdata->regs[PMIC_AUXADC_ADC0] +
> + PMIC_AUXADC_ADCx(chan->address)),
Drop unneeded parentheses and possibly make it one line.
> + val, (val & PMIC_AUXADC_RDY_BIT),
Unneeded parentheses.
> + AUXADC_POLL_DELAY_US, AUXADC_TIMEOUT_US);
> + if (ret)
> + return ret;
...
> + mutex_lock(&adc_dev->lock);
Why not use cleanup.h?
...
> +static int mt6359_auxadc_probe(struct platform_device *pdev)
> +{
struct device *dev = &pdev->dev;
> + struct device *mt6397_mfd_dev = pdev->dev.parent;
... = dev->parent;
> + struct mt6359_auxadc *adc_dev;
> + struct iio_dev *indio_dev;
> + struct regmap *regmap;
> + int ret;
> +
> + /* Regmap is from SoC PMIC Wrapper, parent of the mt6397 MFD */
> + regmap = dev_get_regmap(mt6397_mfd_dev->parent, NULL);
> + if (!regmap)
> + return dev_err_probe(&pdev->dev, -ENODEV, "Failed to get regmap\n");
> +
> + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*adc_dev));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + adc_dev = iio_priv(indio_dev);
> + adc_dev->regmap = regmap;
> + adc_dev->dev = &pdev->dev;
> +
> + adc_dev->pdata = device_get_match_data(&pdev->dev);
> + if (!adc_dev->pdata)
> + return -EINVAL;
> +
> + mutex_init(&adc_dev->lock);
> +
> + mt6359_auxadc_reset(adc_dev);
> +
> + indio_dev->dev.parent = &pdev->dev;
> + indio_dev->name = dev_name(&pdev->dev);
> + indio_dev->info = &mt6359_auxadc_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->channels = adc_dev->pdata->channels;
> + indio_dev->num_channels = adc_dev->pdata->num_channels;
> +
> + ret = devm_iio_device_register(&pdev->dev, indio_dev);
> + if (ret < 0)
Why ' < 0' ?
> + return dev_err_probe(&pdev->dev, ret, "failed to register iio device\n");
> +
> + return 0;
> +}
--
With Best Regards,
Andy Shevchenko
Il 30/05/24 15:34, Andy Shevchenko ha scritto:
> On Thu, May 30, 2024 at 12:34 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>>
>> Add a driver to support reading the Auxiliary ADC IP found in the
>> MediaTek MT6357, MT6358 and MT6359 Power Management ICs.
>>
>> This driver provides multiple ADC channels for system monitoring,
>> such as battery voltage, PMIC temperature, PMIC-internal voltage
>> regulators temperature, and others.
>
>> ---
>
> Here is no explanation on how this is differ to any of the three
> existing drivers? I.o.w. why do we need a brand new one?
>
Not a SoC AUXADC but a PMIC AUXADC, different register layout and different
handling for configuration, reset, and reading.
So okay I'm adding a nicer text of what I just wrote to the commit description.
> ...
>
> + bits.h
>
>> +#include <linux/delay.h>
>
>> +#include <linux/kernel.h>
>
> Why?
>
Because I forgot to cleanup the headers :-\
> + mod_devicetable.h
>> +#include <linux/module.h>
>
>> +#include <linux/of.h>
>
> Why?
>
Same reason :')
...And yes that should be linux/property.h instead, for device_get_match_data().
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>
> + types.h
>
> + blank line
>
>> +#include <linux/iio/iio.h>
>
> + Blank line
>
>> +#include <linux/mfd/mt6397/core.h>
>
> ...
>
>> +#define PMIC_RG_RESET_VAL (BIT(0) | BIT(3))
>
> In this form it requires a comment explaining each mentioned bit.
>
I don't have an explanation for this, I know it's two different bits from some
reveng, but the downstream driver declares that simply as 0x9.
Should I just "mask" this as 0x9 instead?
>> +#define PMIC_AUXADC_ADCx(x) ((x) << 1)
>
> Seems like a useless macro, it occupies much more space than in-place shift.
>
Well that was done to enhance human readability, but okay I will just use an
in-place shift.
> ...
>
>> +#define MT6358_IMP0_CLEAR (BIT(14) | BIT(7))
>
> As per above.
>
Same, I don't have any explanation for that.
If you prefer, I can define this as 0x4080, but honestly I prefer keeping
it as-is since I am sure it's not a magic number but really two bits to flip
in a register.
> ...
>
>> +/**
>> + * struct mtk_pmic_auxadc_chan - PMIC AUXADC channel data
>> + * @req_idx: Request register number
>> + * @req_mask: Bitmask to activate a channel
>> + * @num_samples: Number of AUXADC samples for averaging
>> + * @r_numerator: Resistance ratio numerator
>> + * @r_denominator: Resistance ratio denominator
>> + */
>> +struct mtk_pmic_auxadc_chan {
>> + u8 req_idx;
>> + u16 req_mask;
>> + u16 num_samples;
>
>> + u8 r_numerator;
>> + u8 r_denominator;
>
> Can you add struct u8_fract to the math.h and use it? I will Ack/R the
> respective patch.
>
Yeah, I did that exactly because u8_fract wasn't there and I didn't want
to waste more bits, but since you just asked for it... well, I'm happier :-)
>> +};
>
> ...
>
>> +struct mtk_pmic_auxadc_pdata {
>> + const struct iio_chan_spec *channels;
>> + int num_channels;
>
> Why signed?
>
Yeah, that doesn't make any sense, that's going to be u8.
>> + const struct mtk_pmic_auxadc_chan *desc;
>> + const u16 *regs;
>> + u16 sec_unlock_key;
>> + u8 imp_adc_num;
>> + int (*read_imp)(struct mt6359_auxadc *adc_dev, int *vbat, int *ibat);
>> +};
>
> ...
>
>> +static const u16 mt6357_auxadc_regs[] = {
>> + [PMIC_HK_TOP_RST_CON0] = 0xf90,
>
> Can we use the fixed-width values so all of them will look nice and
> easy to parse?
>
We obviously can, let's do that.
>> + [PMIC_AUXADC_DCM_CON] = 0x122e,
>> + [PMIC_AUXADC_ADC0] = 0x1088,
>> + [PMIC_AUXADC_IMP0] = 0x119c,
>> + [PMIC_AUXADC_IMP1] = 0x119e,
>> + [PMIC_AUXADC_RQST0] = 0x110e,
>> + [PMIC_AUXADC_RQST1] = 0x1114,
>> +};
>
> ...
>
>> +static const u16 mt6358_auxadc_regs[] = {
>
> Ditto.
>
>> + [PMIC_HK_TOP_RST_CON0] = 0xf90,
>> + [PMIC_AUXADC_DCM_CON] = 0x1260,
>> + [PMIC_AUXADC_ADC0] = 0x1088,
>> + [PMIC_AUXADC_IMP0] = 0x1208,
>> + [PMIC_AUXADC_IMP1] = 0x120a,
>> + [PMIC_AUXADC_RQST0] = 0x1108,
>> + [PMIC_AUXADC_RQST1] = 0x110a,
>> +};
>
> ...
>
>> +static const u16 mt6359_auxadc_regs[] = {
>
> Ditto.
>
>> + [PMIC_FGADC_R_CON0] = 0xd88,
>> + [PMIC_HK_TOP_WKEY] = 0xfb4,
>> + [PMIC_HK_TOP_RST_CON0] = 0xf90,
>> + [PMIC_AUXADC_RQST0] = 0x1108,
>> + [PMIC_AUXADC_RQST1] = 0x110a,
>> + [PMIC_AUXADC_ADC0] = 0x1088,
>> + [PMIC_AUXADC_IMP0] = 0x1208,
>> + [PMIC_AUXADC_IMP1] = 0x120a,
>> + [PMIC_AUXADC_IMP3] = 0x120e,
>> +};
>
> ...
>
>> + ret = regmap_read_poll_timeout(adc_dev->regmap, pdata->regs[PMIC_AUXADC_IMP0],
>> + val, !!(val & MT6358_IMP0_IRQ_RDY),
>> + IMP_POLL_DELAY_US, AUXADC_TIMEOUT_US);
>> + if (ret) {
>> + mt6358_stop_imp_conv(adc_dev);
>
>> + return ret;
>> + }
>> +
>> + return 0;
>
> if (ret)
> foo()
>
> return ret;
>
>
> ...
>
>> + int val_v, ret;
>
> Why is val_v signed?
>
> ...
>
>> + int val_v, val_i, ret;
>
> Ditto for all val_*.
>
Yup, changed to u32.
> ...
>
>> + /* If it succeeded, wait for the registers to be populated */
>> + usleep_range(IMP_STOP_DELAY_US, IMP_STOP_DELAY_US + 50);
>
> fsleep() ?
>
Okay
> ...
>
>> + /* Assert ADC reset */
>> + regmap_set_bits(regmap, pdata->regs[PMIC_HK_TOP_RST_CON0], PMIC_RG_RESET_VAL);
>
> No required delay in between?
>
No, as strange as it may look, there is no delay required in between: this is
because the register R/W is behind the PMIC Wrapper as much as all of the other
MediaTek PMIC (sub)devices, so, missing delays was intentional here, yes.
>> + /* De-assert ADC reset */
>> + regmap_clear_bits(regmap, pdata->regs[PMIC_HK_TOP_RST_CON0], PMIC_RG_RESET_VAL);
>
> ...
>
>> + /* Wait until all samples are averaged */
>> + usleep_range(desc->num_samples * AUXADC_AVG_TIME_US,
>> + (desc->num_samples + 1) * AUXADC_AVG_TIME_US);
>
> fsleep()
>
> ...
>
>> + ret = regmap_read_poll_timeout(regmap,
>> + (pdata->regs[PMIC_AUXADC_ADC0] +
>> + PMIC_AUXADC_ADCx(chan->address)),
>
> Drop unneeded parentheses and possibly make it one line.
>
>> + val, (val & PMIC_AUXADC_RDY_BIT),
>
> Unneeded parentheses.
>
Yeah, forgot those there during cleanup, whoops!
>> + AUXADC_POLL_DELAY_US, AUXADC_TIMEOUT_US);
>> + if (ret)
>> + return ret;
>
> ...
>
>> + mutex_lock(&adc_dev->lock);
>
> Why not use cleanup.h?
>
I want to unlock the mutex immediately right after executing read_imp() or
mt6359_auxadc_read_adc(), and I don't want the reset to be done while a mutex
is being held, as that makes no sense for this driver.
Besides, I find the macros in cleanup.h to be cryptic - in my opinion, they
require better documentation as, for example, I don't understand when the
guard(mutex)(my_mutex) is supposed to acquire the lock and when it's supposed
to release it.
> ...
>
>> +static int mt6359_auxadc_probe(struct platform_device *pdev)
>> +{
>
> struct device *dev = &pdev->dev;
Yeah without that it simply looked more "beautiful" to read, but okay, added.
>
>> + struct device *mt6397_mfd_dev = pdev->dev.parent;
>
> ... = dev->parent;
>
>> + struct mt6359_auxadc *adc_dev;
>> + struct iio_dev *indio_dev;
>> + struct regmap *regmap;
>> + int ret;
>> +
>> + /* Regmap is from SoC PMIC Wrapper, parent of the mt6397 MFD */
>> + regmap = dev_get_regmap(mt6397_mfd_dev->parent, NULL);
>> + if (!regmap)
>> + return dev_err_probe(&pdev->dev, -ENODEV, "Failed to get regmap\n");
>> +
>> + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*adc_dev));
>> + if (!indio_dev)
>> + return -ENOMEM;
>> +
>> + adc_dev = iio_priv(indio_dev);
>> + adc_dev->regmap = regmap;
>> + adc_dev->dev = &pdev->dev;
>> +
>> + adc_dev->pdata = device_get_match_data(&pdev->dev);
>> + if (!adc_dev->pdata)
>> + return -EINVAL;
>> +
>> + mutex_init(&adc_dev->lock);
>> +
>> + mt6359_auxadc_reset(adc_dev);
>> +
>> + indio_dev->dev.parent = &pdev->dev;
>> + indio_dev->name = dev_name(&pdev->dev);
>> + indio_dev->info = &mt6359_auxadc_info;
>> + indio_dev->modes = INDIO_DIRECT_MODE;
>> + indio_dev->channels = adc_dev->pdata->channels;
>> + indio_dev->num_channels = adc_dev->pdata->num_channels;
>> +
>> + ret = devm_iio_device_register(&pdev->dev, indio_dev);
>> + if (ret < 0)
>
> Why ' < 0' ?
>
Uuuuuh.. well... I have no idea why this "< 0" is in there, I must have gremlins
living inside my keyboard, secretly inserting those checks when I'm not looking.
Must definitely be that.
Jokes apart, thanks for the review!
...a v2 will come asap, along with the math.h change :-)
Cheers!
Angelo
On Tue, Jun 4, 2024 at 1:38 PM AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote: > Il 30/05/24 15:34, Andy Shevchenko ha scritto: > > On Thu, May 30, 2024 at 12:34 PM AngeloGioacchino Del Regno > > <angelogioacchino.delregno@collabora.com> wrote: ... > >> +#define PMIC_RG_RESET_VAL (BIT(0) | BIT(3)) > > > > In this form it requires a comment explaining each mentioned bit. > > I don't have an explanation for this, I know it's two different bits from some > reveng, but the downstream driver declares that simply as 0x9. > > Should I just "mask" this as 0x9 instead? In this case for all of the questionable forms, please add a oneline comment suggesting that "these are different bits without known purpose of each." or something like that. ... > >> +#define MT6358_IMP0_CLEAR (BIT(14) | BIT(7)) > > > > As per above. > > > > Same, I don't have any explanation for that. > > If you prefer, I can define this as 0x4080, but honestly I prefer keeping > it as-is since I am sure it's not a magic number but really two bits to flip > in a register. As per above. ... > >> + u8 r_numerator; > >> + u8 r_denominator; > > > > Can you add struct u8_fract to the math.h and use it? I will Ack/R the > > respective patch. > > Yeah, I did that exactly because u8_fract wasn't there and I didn't want > to waste more bits, but since you just asked for it... well, I'm happier :-) Note, it's enough to have my Rb tag and route that change via IIO tree. We have done similar way for other changes in math.h (or aline) in the past. ... > >> + /* Assert ADC reset */ > >> + regmap_set_bits(regmap, pdata->regs[PMIC_HK_TOP_RST_CON0], PMIC_RG_RESET_VAL); > > > > No required delay in between? > > No, as strange as it may look, there is no delay required in between: this is > because the register R/W is behind the PMIC Wrapper as much as all of the other > MediaTek PMIC (sub)devices, so, missing delays was intentional here, yes. Maybe a comment? ... > >> + mutex_lock(&adc_dev->lock); > > > > Why not use cleanup.h? > > I want to unlock the mutex immediately right after executing read_imp() or > mt6359_auxadc_read_adc(), and I don't want the reset to be done while a mutex > is being held, as that makes no sense for this driver. That's why we have scoped_guard(). Exactly for such cases. > Besides, I find the macros in cleanup.h to be cryptic - in my opinion, they > require better documentation as, for example, I don't understand when the > guard(mutex)(my_mutex) is supposed to acquire the lock and when it's supposed > to release it. They are cryptic due to limitations in C language. But for the end user it doesn't matter. The behaviour is well understandable and makes code cleaner and less prone for errors such as missing unlocks. So, please use cleanup.h. -- With Best Regards, Andy Shevchenko
Il 04/06/24 13:05, Andy Shevchenko ha scritto: > On Tue, Jun 4, 2024 at 1:38 PM AngeloGioacchino Del Regno > <angelogioacchino.delregno@collabora.com> wrote: >> Il 30/05/24 15:34, Andy Shevchenko ha scritto: >>> On Thu, May 30, 2024 at 12:34 PM AngeloGioacchino Del Regno >>> <angelogioacchino.delregno@collabora.com> wrote: > > ... > >>>> +#define PMIC_RG_RESET_VAL (BIT(0) | BIT(3)) >>> >>> In this form it requires a comment explaining each mentioned bit. >> >> I don't have an explanation for this, I know it's two different bits from some >> reveng, but the downstream driver declares that simply as 0x9. >> >> Should I just "mask" this as 0x9 instead? > > In this case for all of the questionable forms, please add a oneline > comment suggesting that "these are different bits without known > purpose of each." or something like that. > Perfect. Comment added. > ... > >>>> +#define MT6358_IMP0_CLEAR (BIT(14) | BIT(7)) >>> >>> As per above. >>> >> >> Same, I don't have any explanation for that. >> >> If you prefer, I can define this as 0x4080, but honestly I prefer keeping >> it as-is since I am sure it's not a magic number but really two bits to flip >> in a register. > > As per above. > > ... > >>>> + u8 r_numerator; >>>> + u8 r_denominator; >>> >>> Can you add struct u8_fract to the math.h and use it? I will Ack/R the >>> respective patch. >> >> Yeah, I did that exactly because u8_fract wasn't there and I didn't want >> to waste more bits, but since you just asked for it... well, I'm happier :-) > > Note, it's enough to have my Rb tag and route that change via IIO > tree. We have done similar way for other changes in math.h (or aline) > in the past. > Sure. > ... > >>>> + /* Assert ADC reset */ >>>> + regmap_set_bits(regmap, pdata->regs[PMIC_HK_TOP_RST_CON0], PMIC_RG_RESET_VAL); >>> >>> No required delay in between? >> >> No, as strange as it may look, there is no delay required in between: this is >> because the register R/W is behind the PMIC Wrapper as much as all of the other >> MediaTek PMIC (sub)devices, so, missing delays was intentional here, yes. > > Maybe a comment? > Done :-) /* De-assert ADC reset. No wait required, as pwrap takes care of that for us. */ > ... > >>>> + mutex_lock(&adc_dev->lock); >>> >>> Why not use cleanup.h? >> >> I want to unlock the mutex immediately right after executing read_imp() or >> mt6359_auxadc_read_adc(), and I don't want the reset to be done while a mutex >> is being held, as that makes no sense for this driver. > > That's why we have scoped_guard(). Exactly for such cases. > Thanks for the hint, looking at other usages that was straightforward. >> Besides, I find the macros in cleanup.h to be cryptic - in my opinion, they >> require better documentation as, for example, I don't understand when the >> guard(mutex)(my_mutex) is supposed to acquire the lock and when it's supposed >> to release it. > > They are cryptic due to limitations in C language. But for the end > user it doesn't matter. The behaviour is well understandable and makes > code cleaner and less prone for errors such as missing unlocks. So, > please use cleanup.h. > Indeed, but my point was that the documentation can (and probably should) be improved. Cheers, Angelo
© 2016 - 2024 Red Hat, Inc.