devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1 0/5] iio: Add support for MT6363/6373 Auxiliary ADC
@ 2025-06-23 12:00 AngeloGioacchino Del Regno
  2025-06-23 12:00 ` [PATCH v1 1/5] dt-bindings: iio: adc: mt6359: Add bindings for MT6363 PMIC AuxADC AngeloGioacchino Del Regno
                   ` (5 more replies)
  0 siblings, 6 replies; 14+ messages in thread
From: AngeloGioacchino Del Regno @ 2025-06-23 12:00 UTC (permalink / raw)
  To: jic23
  Cc: dlechner, nuno.sa, andy, robh, krzk+dt, conor+dt, matthias.bgg,
	angelogioacchino.delregno, linux-iio, devicetree, linux-kernel,
	linux-arm-kernel, linux-mediatek, kernel

This series adds support for the Auxiliary ADC IP found on the new
MediaTek MT6363 and MT6373 PMICs, found on board designs featuring
the MT8196 Chromebook SoC or the MT6991 Dimensity 9400 Smartphone SoC.

AngeloGioacchino Del Regno (5):
  dt-bindings: iio: adc: mt6359: Add bindings for MT6363 PMIC AuxADC
  dt-bindings: iio: adc: mt6359: Add bindings for MT6373 PMIC AuxADC
  iio: adc: mt6359: Add ready register index and mask to channel data
  iio: adc: mt6359: Add support for MediaTek MT6363 PMIC AUXADC
  iio: adc: mt6359: Add support for MediaTek MT6373 PMIC AUXADC

 .../iio/adc/mediatek,mt6359-auxadc.yaml       |   2 +
 drivers/iio/adc/mt6359-auxadc.c               | 401 ++++++++++++++----
 .../iio/adc/mediatek,mt6363-auxadc.h          |  24 ++
 .../iio/adc/mediatek,mt6373-auxadc.h          |  19 +
 4 files changed, 374 insertions(+), 72 deletions(-)
 create mode 100644 include/dt-bindings/iio/adc/mediatek,mt6363-auxadc.h
 create mode 100644 include/dt-bindings/iio/adc/mediatek,mt6373-auxadc.h

-- 
2.49.0


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v1 1/5] dt-bindings: iio: adc: mt6359: Add bindings for MT6363 PMIC AuxADC
  2025-06-23 12:00 [PATCH v1 0/5] iio: Add support for MT6363/6373 Auxiliary ADC AngeloGioacchino Del Regno
@ 2025-06-23 12:00 ` AngeloGioacchino Del Regno
  2025-06-27 20:02   ` Rob Herring (Arm)
  2025-06-23 12:00 ` [PATCH v1 2/5] dt-bindings: iio: adc: mt6359: Add bindings for MT6373 " AngeloGioacchino Del Regno
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 14+ messages in thread
From: AngeloGioacchino Del Regno @ 2025-06-23 12:00 UTC (permalink / raw)
  To: jic23
  Cc: dlechner, nuno.sa, andy, robh, krzk+dt, conor+dt, matthias.bgg,
	angelogioacchino.delregno, linux-iio, devicetree, linux-kernel,
	linux-arm-kernel, linux-mediatek, kernel

Add a compatible and channel bindings for MediaTek's MT6363 PMIC,
featuring an Auxiliary ADC IP with 15 ADC channels used for both
internal temperatures and voltages and for external voltage inputs.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 .../iio/adc/mediatek,mt6359-auxadc.yaml       |  1 +
 .../iio/adc/mediatek,mt6363-auxadc.h          | 24 +++++++++++++++++++
 2 files changed, 25 insertions(+)
 create mode 100644 include/dt-bindings/iio/adc/mediatek,mt6363-auxadc.h

diff --git a/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml b/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml
index 6497c416094d..a94429477e46 100644
--- a/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml
@@ -22,6 +22,7 @@ properties:
       - mediatek,mt6357-auxadc
       - mediatek,mt6358-auxadc
       - mediatek,mt6359-auxadc
+      - mediatek,mt6363-auxadc
 
   "#io-channel-cells":
     const: 1
diff --git a/include/dt-bindings/iio/adc/mediatek,mt6363-auxadc.h b/include/dt-bindings/iio/adc/mediatek,mt6363-auxadc.h
new file mode 100644
index 000000000000..92d135477d0e
--- /dev/null
+++ b/include/dt-bindings/iio/adc/mediatek,mt6363-auxadc.h
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
+
+#ifndef _DT_BINDINGS_MEDIATEK_MT6363_AUXADC_H
+#define _DT_BINDINGS_MEDIATEK_MT6363_AUXADC_H
+
+/* ADC Channel Index */
+#define MT6363_AUXADC_BATADC		0
+#define MT6363_AUXADC_VCDT		1
+#define MT6363_AUXADC_BAT_TEMP		2
+#define MT6363_AUXADC_CHIP_TEMP		3
+#define MT6363_AUXADC_VSYSSNS		4
+#define MT6363_AUXADC_VTREF		5
+#define MT6363_AUXADC_VCORE_TEMP	6
+#define MT6363_AUXADC_VPROC_TEMP	7
+#define MT6363_AUXADC_VGPU_TEMP		8
+#define MT6363_AUXADC_VIN1		9
+#define MT6363_AUXADC_VIN2		10
+#define MT6363_AUXADC_VIN3		11
+#define MT6363_AUXADC_VIN4		12
+#define MT6363_AUXADC_VIN5		13
+#define MT6363_AUXADC_VIN6		14
+#define MT6363_AUXADC_VIN7		15
+
+#endif
-- 
2.49.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v1 2/5] dt-bindings: iio: adc: mt6359: Add bindings for MT6373 PMIC AuxADC
  2025-06-23 12:00 [PATCH v1 0/5] iio: Add support for MT6363/6373 Auxiliary ADC AngeloGioacchino Del Regno
  2025-06-23 12:00 ` [PATCH v1 1/5] dt-bindings: iio: adc: mt6359: Add bindings for MT6363 PMIC AuxADC AngeloGioacchino Del Regno
@ 2025-06-23 12:00 ` AngeloGioacchino Del Regno
  2025-06-27 20:04   ` Rob Herring
  2025-06-23 12:00 ` [PATCH v1 3/5] iio: adc: mt6359: Add ready register index and mask to channel data AngeloGioacchino Del Regno
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 14+ messages in thread
From: AngeloGioacchino Del Regno @ 2025-06-23 12:00 UTC (permalink / raw)
  To: jic23
  Cc: dlechner, nuno.sa, andy, robh, krzk+dt, conor+dt, matthias.bgg,
	angelogioacchino.delregno, linux-iio, devicetree, linux-kernel,
	linux-arm-kernel, linux-mediatek, kernel

Add a compatible and channel bindings for MediaTek's MT6373 PMIC,
featuring an Auxiliary ADC IP with 15 ADC channels for external
(SoC) temperatures and external voltage inputs.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 .../iio/adc/mediatek,mt6359-auxadc.yaml       |  1 +
 .../iio/adc/mediatek,mt6373-auxadc.h          | 19 +++++++++++++++++++
 2 files changed, 20 insertions(+)
 create mode 100644 include/dt-bindings/iio/adc/mediatek,mt6373-auxadc.h

diff --git a/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml b/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml
index a94429477e46..5d4ab701f51a 100644
--- a/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml
@@ -23,6 +23,7 @@ properties:
       - mediatek,mt6358-auxadc
       - mediatek,mt6359-auxadc
       - mediatek,mt6363-auxadc
+      - mediatek,mt6373-auxadc
 
   "#io-channel-cells":
     const: 1
diff --git a/include/dt-bindings/iio/adc/mediatek,mt6373-auxadc.h b/include/dt-bindings/iio/adc/mediatek,mt6373-auxadc.h
new file mode 100644
index 000000000000..17cab86d355e
--- /dev/null
+++ b/include/dt-bindings/iio/adc/mediatek,mt6373-auxadc.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
+
+#ifndef _DT_BINDINGS_MEDIATEK_MT6373_AUXADC_H
+#define _DT_BINDINGS_MEDIATEK_MT6373_AUXADC_H
+
+/* ADC Channel Index */
+#define MT6373_AUXADC_CHIP_TEMP		0
+#define MT6373_AUXADC_VCORE_TEMP	1
+#define MT6373_AUXADC_VPROC_TEMP	2
+#define MT6373_AUXADC_VGPU_TEMP		3
+#define MT6373_AUXADC_VIN1		4
+#define MT6373_AUXADC_VIN2		5
+#define MT6373_AUXADC_VIN3		6
+#define MT6373_AUXADC_VIN4		7
+#define MT6373_AUXADC_VIN5		8
+#define MT6373_AUXADC_VIN6		9
+#define MT6373_AUXADC_VIN7		10
+
+#endif
-- 
2.49.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v1 3/5] iio: adc: mt6359: Add ready register index and mask to channel data
  2025-06-23 12:00 [PATCH v1 0/5] iio: Add support for MT6363/6373 Auxiliary ADC AngeloGioacchino Del Regno
  2025-06-23 12:00 ` [PATCH v1 1/5] dt-bindings: iio: adc: mt6359: Add bindings for MT6363 PMIC AuxADC AngeloGioacchino Del Regno
  2025-06-23 12:00 ` [PATCH v1 2/5] dt-bindings: iio: adc: mt6359: Add bindings for MT6373 " AngeloGioacchino Del Regno
@ 2025-06-23 12:00 ` AngeloGioacchino Del Regno
  2025-06-23 12:00 ` [PATCH v1 4/5] iio: adc: mt6359: Add support for MediaTek MT6363 PMIC AUXADC AngeloGioacchino Del Regno
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: AngeloGioacchino Del Regno @ 2025-06-23 12:00 UTC (permalink / raw)
  To: jic23
  Cc: dlechner, nuno.sa, andy, robh, krzk+dt, conor+dt, matthias.bgg,
	angelogioacchino.delregno, linux-iio, devicetree, linux-kernel,
	linux-arm-kernel, linux-mediatek, kernel

In preparation for adding support for the AUXADC block found in
the MT6363 PMIC, add the ready register index and mask to the
mtk_pmic_auxadc_chan structure, populate those in the channel
description for all of the already supported SoCs and make use
of them in the .read_imp() callbacks.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/iio/adc/mt6359-auxadc.c | 118 ++++++++++++++++++--------------
 1 file changed, 65 insertions(+), 53 deletions(-)

diff --git a/drivers/iio/adc/mt6359-auxadc.c b/drivers/iio/adc/mt6359-auxadc.c
index eecf88b05c6f..ae7b65f5f551 100644
--- a/drivers/iio/adc/mt6359-auxadc.c
+++ b/drivers/iio/adc/mt6359-auxadc.c
@@ -101,12 +101,16 @@ struct mt6359_auxadc {
  * struct mtk_pmic_auxadc_chan - PMIC AUXADC channel data
  * @req_idx:       Request register number
  * @req_mask:      Bitmask to activate a channel
+ * @rdy_idx:       Readiness register number
+ * @rdy_mask:      Bitmask to determine channel readiness
  * @num_samples:   Number of AUXADC samples for averaging
  * @r_ratio:       Resistance ratio fractional
  */
 struct mtk_pmic_auxadc_chan {
 	u8 req_idx;
 	u16 req_mask;
+	u8 rdy_idx;
+	u16 rdy_mask;
 	u16 num_samples;
 	struct u8_fract r_ratio;
 };
@@ -130,13 +134,17 @@ struct mtk_pmic_auxadc_info {
 	const u16 *regs;
 	u16 sec_unlock_key;
 	u8 imp_adc_num;
-	int (*read_imp)(struct mt6359_auxadc *adc_dev, int *vbat, int *ibat);
+	int (*read_imp)(struct mt6359_auxadc *adc_dev,
+			const struct iio_chan_spec *chan, int *vbat, int *ibat);
 };
 
-#define MTK_PMIC_ADC_CHAN(_ch_idx, _req_idx, _req_bit, _samples, _rnum, _rdiv)	\
+#define MTK_PMIC_ADC_CHAN(_ch_idx, _req_idx, _req_bit, _rdy_idx, _rdy_bit,	\
+			  _samples, _rnum, _rdiv)				\
 	[PMIC_AUXADC_CHAN_##_ch_idx] = {					\
 		.req_idx = _req_idx,						\
 		.req_mask = BIT(_req_bit),					\
+		.rdy_idx = _rdy_idx,						\
+		.rdy_mask = BIT(_rdy_bit),					\
 		.num_samples = _samples,					\
 		.r_ratio = { _rnum, _rdiv }					\
 	}
@@ -177,21 +185,21 @@ static const struct iio_chan_spec mt6357_auxadc_channels[] = {
 };
 
 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),
+	MTK_PMIC_ADC_CHAN(BATADC, PMIC_AUXADC_RQST0, 0, PMIC_AUXADC_IMP0, 8, 128, 3, 1),
+	MTK_PMIC_ADC_CHAN(ISENSE, PMIC_AUXADC_RQST0, 0, PMIC_AUXADC_IMP0, 8, 128, 3, 1),
+	MTK_PMIC_ADC_CHAN(VCDT, PMIC_AUXADC_RQST0, 0, PMIC_AUXADC_IMP0, 8, 8, 1, 1),
+	MTK_PMIC_ADC_CHAN(BAT_TEMP, PMIC_AUXADC_RQST0, 3, PMIC_AUXADC_IMP0, 8, 8, 1, 1),
+	MTK_PMIC_ADC_CHAN(CHIP_TEMP, PMIC_AUXADC_RQST0, 4, PMIC_AUXADC_IMP0, 8, 8, 1, 1),
+	MTK_PMIC_ADC_CHAN(ACCDET, PMIC_AUXADC_RQST0, 5, PMIC_AUXADC_IMP0, 8, 8, 1, 1),
+	MTK_PMIC_ADC_CHAN(TSX_TEMP, PMIC_AUXADC_RQST0, 7, PMIC_AUXADC_IMP0, 8, 128, 1, 1),
+	MTK_PMIC_ADC_CHAN(HPOFS_CAL, PMIC_AUXADC_RQST0, 9, PMIC_AUXADC_IMP0, 8, 256, 1, 1),
+	MTK_PMIC_ADC_CHAN(DCXO_TEMP, PMIC_AUXADC_RQST0, 10, PMIC_AUXADC_IMP0, 8, 16, 1, 1),
+	MTK_PMIC_ADC_CHAN(VBIF, PMIC_AUXADC_RQST0, 11, PMIC_AUXADC_IMP0, 8, 8, 1, 1),
+	MTK_PMIC_ADC_CHAN(VCORE_TEMP, PMIC_AUXADC_RQST1, 5, PMIC_AUXADC_IMP0, 8, 8, 1, 1),
+	MTK_PMIC_ADC_CHAN(VPROC_TEMP, PMIC_AUXADC_RQST1, 6, PMIC_AUXADC_IMP0, 8, 8, 1, 1),
 
 	/* Battery impedance channels */
-	MTK_PMIC_ADC_CHAN(VBAT, 0, 0, 128, 3, 1),
+	MTK_PMIC_ADC_CHAN(VBAT, 0, 0, 0, 0, 128, 3, 1),
 };
 
 static const u16 mt6357_auxadc_regs[] = {
@@ -224,22 +232,22 @@ static const struct iio_chan_spec mt6358_auxadc_channels[] = {
 };
 
 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),
+	MTK_PMIC_ADC_CHAN(BATADC, PMIC_AUXADC_RQST0, 0, PMIC_AUXADC_IMP0, 8, 128, 3, 1),
+	MTK_PMIC_ADC_CHAN(VCDT, PMIC_AUXADC_RQST0, 0, PMIC_AUXADC_IMP0, 8, 8, 1, 1),
+	MTK_PMIC_ADC_CHAN(BAT_TEMP, PMIC_AUXADC_RQST0, 3, PMIC_AUXADC_IMP0, 8, 8, 2, 1),
+	MTK_PMIC_ADC_CHAN(CHIP_TEMP, PMIC_AUXADC_RQST0, 4, PMIC_AUXADC_IMP0, 8, 8, 1, 1),
+	MTK_PMIC_ADC_CHAN(ACCDET, PMIC_AUXADC_RQST0, 5, PMIC_AUXADC_IMP0, 8, 8, 1, 1),
+	MTK_PMIC_ADC_CHAN(VDCXO, PMIC_AUXADC_RQST0, 6, PMIC_AUXADC_IMP0, 8, 8, 3, 2),
+	MTK_PMIC_ADC_CHAN(TSX_TEMP, PMIC_AUXADC_RQST0, 7, PMIC_AUXADC_IMP0, 8, 128, 1, 1),
+	MTK_PMIC_ADC_CHAN(HPOFS_CAL, PMIC_AUXADC_RQST0, 9, PMIC_AUXADC_IMP0, 8, 256, 1, 1),
+	MTK_PMIC_ADC_CHAN(DCXO_TEMP, PMIC_AUXADC_RQST0, 10, PMIC_AUXADC_IMP0, 8, 16, 1, 1),
+	MTK_PMIC_ADC_CHAN(VBIF, PMIC_AUXADC_RQST0, 11, PMIC_AUXADC_IMP0, 8, 8, 2, 1),
+	MTK_PMIC_ADC_CHAN(VCORE_TEMP, PMIC_AUXADC_RQST1, 8, PMIC_AUXADC_IMP0, 8, 8, 1, 1),
+	MTK_PMIC_ADC_CHAN(VPROC_TEMP, PMIC_AUXADC_RQST1, 9, PMIC_AUXADC_IMP0, 8, 8, 1, 1),
+	MTK_PMIC_ADC_CHAN(VGPU_TEMP, PMIC_AUXADC_RQST1, 10, PMIC_AUXADC_IMP0, 8, 8, 1, 1),
 
 	/* Battery impedance channels */
-	MTK_PMIC_ADC_CHAN(VBAT, 0, 0, 128, 7, 2),
+	MTK_PMIC_ADC_CHAN(VBAT, 0, 0, 0, 0, 128, 7, 2),
 };
 
 static const u16 mt6358_auxadc_regs[] = {
@@ -272,22 +280,22 @@ static const struct iio_chan_spec mt6359_auxadc_channels[] = {
 };
 
 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),
+	MTK_PMIC_ADC_CHAN(BATADC, PMIC_AUXADC_RQST0, 0, PMIC_AUXADC_IMP1, 15, 128, 7, 2),
+	MTK_PMIC_ADC_CHAN(BAT_TEMP, PMIC_AUXADC_RQST0, 3, PMIC_AUXADC_IMP1, 15, 8, 5, 2),
+	MTK_PMIC_ADC_CHAN(CHIP_TEMP, PMIC_AUXADC_RQST0, 4, PMIC_AUXADC_IMP1, 15, 8, 1, 1),
+	MTK_PMIC_ADC_CHAN(ACCDET, PMIC_AUXADC_RQST0, 5, PMIC_AUXADC_IMP1, 15 ,8, 1, 1),
+	MTK_PMIC_ADC_CHAN(VDCXO, PMIC_AUXADC_RQST0, 6, PMIC_AUXADC_IMP1, 15, 8, 3, 2),
+	MTK_PMIC_ADC_CHAN(TSX_TEMP, PMIC_AUXADC_RQST0, 7, PMIC_AUXADC_IMP1, 15, 128, 1, 1),
+	MTK_PMIC_ADC_CHAN(HPOFS_CAL, PMIC_AUXADC_RQST0, 9, PMIC_AUXADC_IMP1, 15, 256, 1, 1),
+	MTK_PMIC_ADC_CHAN(DCXO_TEMP, PMIC_AUXADC_RQST0, 10, PMIC_AUXADC_IMP1, 15, 16, 1, 1),
+	MTK_PMIC_ADC_CHAN(VBIF, PMIC_AUXADC_RQST0, 11, PMIC_AUXADC_IMP1, 15, 8, 5, 2),
+	MTK_PMIC_ADC_CHAN(VCORE_TEMP, PMIC_AUXADC_RQST1, 8, PMIC_AUXADC_IMP1, 15, 8, 1, 1),
+	MTK_PMIC_ADC_CHAN(VPROC_TEMP, PMIC_AUXADC_RQST1, 9, PMIC_AUXADC_IMP1, 15, 8, 1, 1),
+	MTK_PMIC_ADC_CHAN(VGPU_TEMP, PMIC_AUXADC_RQST1, 10, PMIC_AUXADC_IMP1, 15, 8, 1, 1),
 
 	/* Battery impedance channels */
-	MTK_PMIC_ADC_CHAN(VBAT, 0, 0, 128, 7, 2),
-	MTK_PMIC_ADC_CHAN(IBAT, 0, 0, 128, 7, 2),
+	MTK_PMIC_ADC_CHAN(VBAT, 0, 0, 0, 0, 128, 7, 2),
+	MTK_PMIC_ADC_CHAN(IBAT, 0, 0, 0, 0, 128, 7, 2),
 };
 
 static const u16 mt6359_auxadc_regs[] = {
@@ -313,9 +321,10 @@ static void mt6358_stop_imp_conv(struct mt6359_auxadc *adc_dev)
 	regmap_clear_bits(regmap, cinfo->regs[PMIC_AUXADC_DCM_CON], MT6358_DCM_CK_SW_EN);
 }
 
-static int mt6358_start_imp_conv(struct mt6359_auxadc *adc_dev)
+static int mt6358_start_imp_conv(struct mt6359_auxadc *adc_dev, const struct iio_chan_spec *chan)
 {
 	const struct mtk_pmic_auxadc_info *cinfo = adc_dev->chip_info;
+	const struct mtk_pmic_auxadc_chan *desc = &cinfo->desc[chan->scan_index];
 	struct regmap *regmap = adc_dev->regmap;
 	u32 val;
 	int ret;
@@ -323,8 +332,8 @@ static int mt6358_start_imp_conv(struct mt6359_auxadc *adc_dev)
 	regmap_set_bits(regmap, cinfo->regs[PMIC_AUXADC_DCM_CON], MT6358_DCM_CK_SW_EN);
 	regmap_set_bits(regmap, cinfo->regs[PMIC_AUXADC_IMP1], MT6358_IMP1_AUTOREPEAT_EN);
 
-	ret = regmap_read_poll_timeout(adc_dev->regmap, cinfo->regs[PMIC_AUXADC_IMP0],
-				       val, val & MT6358_IMP0_IRQ_RDY,
+	ret = regmap_read_poll_timeout(regmap, cinfo->regs[desc->rdy_idx],
+				       val, val & desc->rdy_mask,
 				       IMP_POLL_DELAY_US, AUXADC_TIMEOUT_US);
 	if (ret) {
 		mt6358_stop_imp_conv(adc_dev);
@@ -334,7 +343,8 @@ static int mt6358_start_imp_conv(struct mt6359_auxadc *adc_dev)
 	return 0;
 }
 
-static int mt6358_read_imp(struct mt6359_auxadc *adc_dev, int *vbat, int *ibat)
+static int mt6358_read_imp(struct mt6359_auxadc *adc_dev,
+			   const struct iio_chan_spec *chan, int *vbat, int *ibat)
 {
 	const struct mtk_pmic_auxadc_info *cinfo = adc_dev->chip_info;
 	struct regmap *regmap = adc_dev->regmap;
@@ -342,7 +352,7 @@ static int mt6358_read_imp(struct mt6359_auxadc *adc_dev, int *vbat, int *ibat)
 	u32 val_v;
 	int ret;
 
-	ret = mt6358_start_imp_conv(adc_dev);
+	ret = mt6358_start_imp_conv(adc_dev, chan);
 	if (ret)
 		return ret;
 
@@ -359,17 +369,19 @@ static int mt6358_read_imp(struct mt6359_auxadc *adc_dev, int *vbat, int *ibat)
 	return 0;
 }
 
-static int mt6359_read_imp(struct mt6359_auxadc *adc_dev, int *vbat, int *ibat)
+static int mt6359_read_imp(struct mt6359_auxadc *adc_dev,
+			   const struct iio_chan_spec *chan, int *vbat, int *ibat)
 {
 	const struct mtk_pmic_auxadc_info *cinfo = adc_dev->chip_info;
+	const struct mtk_pmic_auxadc_chan *desc = &cinfo->desc[chan->scan_index];
 	struct regmap *regmap = adc_dev->regmap;
 	u32 val, val_v, val_i;
 	int ret;
 
 	/* Start conversion */
 	regmap_write(regmap, cinfo->regs[PMIC_AUXADC_IMP0], MT6359_IMP0_CONV_EN);
-	ret = regmap_read_poll_timeout(regmap, cinfo->regs[PMIC_AUXADC_IMP1],
-				       val, val & MT6359_IMP1_IRQ_RDY,
+	ret = regmap_read_poll_timeout(regmap, cinfo->regs[desc->rdy_idx],
+				       val, val & desc->rdy_mask,
 				       IMP_POLL_DELAY_US, AUXADC_TIMEOUT_US);
 
 	/* Stop conversion regardless of the result */
@@ -506,10 +518,10 @@ static int mt6359_auxadc_read_raw(struct iio_dev *indio_dev,
 	scoped_guard(mutex, &adc_dev->lock) {
 		switch (chan->scan_index) {
 		case PMIC_AUXADC_CHAN_IBAT:
-			ret = adc_dev->chip_info->read_imp(adc_dev, NULL, val);
+			ret = adc_dev->chip_info->read_imp(adc_dev, chan, NULL, val);
 			break;
 		case PMIC_AUXADC_CHAN_VBAT:
-			ret = adc_dev->chip_info->read_imp(adc_dev, val, NULL);
+			ret = adc_dev->chip_info->read_imp(adc_dev, chan, val, NULL);
 			break;
 		default:
 			ret = mt6359_auxadc_read_adc(adc_dev, chan, val);
-- 
2.49.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v1 4/5] iio: adc: mt6359: Add support for MediaTek MT6363 PMIC AUXADC
  2025-06-23 12:00 [PATCH v1 0/5] iio: Add support for MT6363/6373 Auxiliary ADC AngeloGioacchino Del Regno
                   ` (2 preceding siblings ...)
  2025-06-23 12:00 ` [PATCH v1 3/5] iio: adc: mt6359: Add ready register index and mask to channel data AngeloGioacchino Del Regno
@ 2025-06-23 12:00 ` AngeloGioacchino Del Regno
  2025-06-23 14:30   ` Andy Shevchenko
  2025-06-28 16:01   ` Jonathan Cameron
  2025-06-23 12:00 ` [PATCH v1 5/5] iio: adc: mt6359: Add support for MediaTek MT6373 " AngeloGioacchino Del Regno
  2025-06-24 15:08 ` [PATCH v1 0/5] iio: Add support for MT6363/6373 Auxiliary ADC Nícolas F. R. A. Prado
  5 siblings, 2 replies; 14+ messages in thread
From: AngeloGioacchino Del Regno @ 2025-06-23 12:00 UTC (permalink / raw)
  To: jic23
  Cc: dlechner, nuno.sa, andy, robh, krzk+dt, conor+dt, matthias.bgg,
	angelogioacchino.delregno, linux-iio, devicetree, linux-kernel,
	linux-arm-kernel, linux-mediatek, kernel

MediaTek MT6363 is a PMIC found on MT8196/MT6991 board designs
and communicates with the SoC over SPMI.

This PMIC integrates an Auxiliary ADC (AUXADC) which has a grand
total of 54 ADC channels: 49 PMIC-internal channels, 2 external
NTC thermistor channels and 2 generic ADC channels (mapped to 7
PMIC ADC external inputs).

To use a generic ADC channel it is necessary to enable one of
the PMIC ADC inputs at a time and only then start the reading,
so in this case it is possible to read only one external input
for each generic ADC channel.

Due to the lack of documentation, this implementation supports
using only one generic ADC channel, hence supports reading only
one external input at a time.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/iio/adc/mt6359-auxadc.c | 238 +++++++++++++++++++++++++++++---
 1 file changed, 217 insertions(+), 21 deletions(-)

diff --git a/drivers/iio/adc/mt6359-auxadc.c b/drivers/iio/adc/mt6359-auxadc.c
index ae7b65f5f551..f49b0b6e78da 100644
--- a/drivers/iio/adc/mt6359-auxadc.c
+++ b/drivers/iio/adc/mt6359-auxadc.c
@@ -7,6 +7,7 @@
  * Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
  */
 
+#include <linux/bitfield.h>
 #include <linux/bits.h>
 #include <linux/cleanup.h>
 #include <linux/delay.h>
@@ -24,6 +25,7 @@
 #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>
+#include <dt-bindings/iio/adc/mediatek,mt6363-auxadc.h>
 
 #define AUXADC_AVG_TIME_US		10
 #define AUXADC_POLL_DELAY_US		100
@@ -46,6 +48,18 @@
 #define MT6359_IMP0_CONV_EN		BIT(0)
 #define MT6359_IMP1_IRQ_RDY		BIT(15)
 
+#define MT6363_EXT_CHAN_MASK		GENMASK(2, 0)
+#define MT6363_EXT_PURES_MASK		GENMASK(4, 3)
+ #define MT6363_PULLUP_RES_100K		0
+ #define MT6363_PULLUP_RES_OPEN		3
+
+#define MTK_AUXADC_HAS_FLAG(pdata, msk)	((pdata)->flags & (MTK_PMIC_AUXADC_##msk))
+
+enum mtk_pmic_auxadc_flags {
+	MTK_PMIC_AUXADC_IS_SPMI = BIT(0),
+	MTK_PMIC_AUXADC_NO_RESET = BIT(1),
+};
+
 enum mtk_pmic_auxadc_regs {
 	PMIC_AUXADC_ADC0,
 	PMIC_AUXADC_DCM_CON,
@@ -54,6 +68,8 @@ enum mtk_pmic_auxadc_regs {
 	PMIC_AUXADC_IMP3,
 	PMIC_AUXADC_RQST0,
 	PMIC_AUXADC_RQST1,
+	PMIC_AUXADC_RQST3,
+	PMIC_AUXADC_SDMADC_CON0,
 	PMIC_HK_TOP_WKEY,
 	PMIC_HK_TOP_RST_CON0,
 	PMIC_FGADC_R_CON0,
@@ -75,7 +91,17 @@ enum mtk_pmic_auxadc_channels {
 	PMIC_AUXADC_CHAN_TSX_TEMP,
 	PMIC_AUXADC_CHAN_HPOFS_CAL,
 	PMIC_AUXADC_CHAN_DCXO_TEMP,
+	PMIC_AUXADC_CHAN_VTREF,
 	PMIC_AUXADC_CHAN_VBIF,
+	PMIC_AUXADC_CHAN_IMIX_R,
+	PMIC_AUXADC_CHAN_VSYSSNS,
+	PMIC_AUXADC_CHAN_VIN1,
+	PMIC_AUXADC_CHAN_VIN2,
+	PMIC_AUXADC_CHAN_VIN3,
+	PMIC_AUXADC_CHAN_VIN4,
+	PMIC_AUXADC_CHAN_VIN5,
+	PMIC_AUXADC_CHAN_VIN6,
+	PMIC_AUXADC_CHAN_VIN7,
 	PMIC_AUXADC_CHAN_IBAT,
 	PMIC_AUXADC_CHAN_VBAT,
 	PMIC_AUXADC_CHAN_MAX
@@ -103,6 +129,9 @@ struct mt6359_auxadc {
  * @req_mask:      Bitmask to activate a channel
  * @rdy_idx:       Readiness register number
  * @rdy_mask:      Bitmask to determine channel readiness
+ * @ext_sel_idx:   PMIC GPIO channel register number
+ * @ext_sel_ch:    PMIC GPIO number
+ * @ext_sel_pu:    PMIC GPIO channel pullup resistor selector
  * @num_samples:   Number of AUXADC samples for averaging
  * @r_ratio:       Resistance ratio fractional
  */
@@ -111,6 +140,9 @@ struct mtk_pmic_auxadc_chan {
 	u16 req_mask;
 	u8 rdy_idx;
 	u16 rdy_mask;
+	s8 ext_sel_idx;
+	u8 ext_sel_ch;
+	u8 ext_sel_pu;
 	u16 num_samples;
 	struct u8_fract r_ratio;
 };
@@ -123,7 +155,9 @@ struct mtk_pmic_auxadc_chan {
  * @desc:           PMIC AUXADC channel data
  * @regs:           List of PMIC specific registers
  * @sec_unlock_key: Security unlock key for HK_TOP writes
+ * @vref_mv:        AUXADC Reference Voltage (VREF) in millivolts
  * @imp_adc_num:    ADC channel for battery impedance readings
+ * @flags:          Feature flags
  * @read_imp:       Callback to read impedance channels
  */
 struct mtk_pmic_auxadc_info {
@@ -133,22 +167,33 @@ struct mtk_pmic_auxadc_info {
 	const struct mtk_pmic_auxadc_chan *desc;
 	const u16 *regs;
 	u16 sec_unlock_key;
+	u16 vref_mv;
 	u8 imp_adc_num;
+	u8 flags;
 	int (*read_imp)(struct mt6359_auxadc *adc_dev,
 			const struct iio_chan_spec *chan, int *vbat, int *ibat);
 };
 
-#define MTK_PMIC_ADC_CHAN(_ch_idx, _req_idx, _req_bit, _rdy_idx, _rdy_bit,	\
-			  _samples, _rnum, _rdiv)				\
+#define MTK_PMIC_ADC_EXT_CHAN(_ch_idx, _req_idx, _req_bit, _rdy_idx, _rdy_bit,	\
+			      _ext_sel_idx, _ext_sel_ch, _ext_sel_pu,		\
+			      _samples, _rnum, _rdiv)				\
 	[PMIC_AUXADC_CHAN_##_ch_idx] = {					\
 		.req_idx = _req_idx,						\
 		.req_mask = BIT(_req_bit),					\
 		.rdy_idx = _rdy_idx,						\
 		.rdy_mask = BIT(_rdy_bit),					\
+		.ext_sel_idx = _ext_sel_idx,					\
+		.ext_sel_ch = _ext_sel_ch,					\
+		.ext_sel_pu = _ext_sel_pu,					\
 		.num_samples = _samples,					\
 		.r_ratio = { _rnum, _rdiv }					\
 	}
 
+#define MTK_PMIC_ADC_CHAN(_ch_idx, _req_idx, _req_bit, _rdy_idx, _rdy_bit,	\
+			  _samples, _rnum, _rdiv)				\
+	MTK_PMIC_ADC_EXT_CHAN(_ch_idx, _req_idx, _req_bit, _rdy_idx, _rdy_bit,	\
+			      -1, 0, 0, _samples, _rnum, _rdiv)
+
 #define MTK_PMIC_IIO_CHAN(_model, _name, _ch_idx, _adc_idx, _nbits, _ch_type)	\
 {										\
 	.type = _ch_type,							\
@@ -310,6 +355,70 @@ static const u16 mt6359_auxadc_regs[] = {
 	[PMIC_AUXADC_IMP3]	= 0x120e,
 };
 
+static const struct iio_chan_spec mt6363_auxadc_channels[] = {
+	MTK_PMIC_IIO_CHAN(MT6363, bat_adc, BATADC, 0, 15, IIO_RESISTANCE),
+	MTK_PMIC_IIO_CHAN(MT6363, cdt_v, VCDT, 2, 12, IIO_TEMP),/**/
+	MTK_PMIC_IIO_CHAN(MT6363, batt_temp, BAT_TEMP, 3, 12, IIO_TEMP),
+	MTK_PMIC_IIO_CHAN(MT6363, chip_temp, CHIP_TEMP, 4, 12, IIO_TEMP),
+	MTK_PMIC_IIO_CHAN(MT6363, sys_sns_v, VSYSSNS, 6, 15, IIO_VOLTAGE),
+	MTK_PMIC_IIO_CHAN(MT6363, tref_v, VTREF, 11, 12, IIO_VOLTAGE),
+	MTK_PMIC_IIO_CHAN(MT6363, vcore_temp, VCORE_TEMP, 38, 12, IIO_TEMP),
+	MTK_PMIC_IIO_CHAN(MT6363, vproc_temp, VPROC_TEMP, 39, 12, IIO_TEMP),
+	MTK_PMIC_IIO_CHAN(MT6363, vgpu_temp, VGPU_TEMP, 40, 12, IIO_TEMP),
+
+	/* For VIN, ADC12 holds the result depending on which GPIO was activated */
+	MTK_PMIC_IIO_CHAN(MT6363, in1_v, VIN1, 45, 15, IIO_VOLTAGE),
+	MTK_PMIC_IIO_CHAN(MT6363, in2_v, VIN2, 45, 15, IIO_VOLTAGE),
+	MTK_PMIC_IIO_CHAN(MT6363, in3_v, VIN3, 45, 15, IIO_VOLTAGE),
+	MTK_PMIC_IIO_CHAN(MT6363, in4_v, VIN4, 45, 15, IIO_VOLTAGE),
+	MTK_PMIC_IIO_CHAN(MT6363, in5_v, VIN5, 45, 15, IIO_VOLTAGE),
+	MTK_PMIC_IIO_CHAN(MT6363, in6_v, VIN6, 45, 15, IIO_VOLTAGE),
+	MTK_PMIC_IIO_CHAN(MT6363, in7_v, VIN7, 45, 15, IIO_VOLTAGE),
+};
+
+static const struct mtk_pmic_auxadc_chan mt6363_auxadc_ch_desc[] = {
+	MTK_PMIC_ADC_CHAN(BATADC, PMIC_AUXADC_RQST0, 0, PMIC_AUXADC_ADC0, 15, 64, 4, 1),
+	MTK_PMIC_ADC_CHAN(VCDT, PMIC_AUXADC_RQST0, 2, PMIC_AUXADC_ADC0, 15, 32, 1, 1),
+	MTK_PMIC_ADC_CHAN(BAT_TEMP, PMIC_AUXADC_RQST0, 3, PMIC_AUXADC_ADC0, 15, 32, 3, 2),
+	MTK_PMIC_ADC_CHAN(CHIP_TEMP, PMIC_AUXADC_RQST0, 4, PMIC_AUXADC_ADC0, 15, 32, 1, 1),
+	MTK_PMIC_ADC_CHAN(VSYSSNS, PMIC_AUXADC_RQST1, 6, PMIC_AUXADC_ADC0, 15, 64, 3, 1),
+	MTK_PMIC_ADC_CHAN(VTREF, PMIC_AUXADC_RQST1, 3, PMIC_AUXADC_ADC0, 15, 32, 3, 2),
+	MTK_PMIC_ADC_CHAN(VCORE_TEMP, PMIC_AUXADC_RQST3, 0, PMIC_AUXADC_ADC0, 15, 32, 1, 1),
+	MTK_PMIC_ADC_CHAN(VPROC_TEMP, PMIC_AUXADC_RQST3, 1, PMIC_AUXADC_ADC0, 15, 32, 1, 1),
+	MTK_PMIC_ADC_CHAN(VGPU_TEMP, PMIC_AUXADC_RQST3, 2, PMIC_AUXADC_ADC0, 15, 32, 1, 1),
+
+	MTK_PMIC_ADC_EXT_CHAN(VIN1,
+			      PMIC_AUXADC_RQST1, 4, PMIC_AUXADC_ADC0, 15,
+			      PMIC_AUXADC_SDMADC_CON0, 1, MT6363_PULLUP_RES_100K, 32, 1, 1),
+	MTK_PMIC_ADC_EXT_CHAN(VIN2,
+			      PMIC_AUXADC_RQST1, 4, PMIC_AUXADC_ADC0, 15,
+			      PMIC_AUXADC_SDMADC_CON0, 2, MT6363_PULLUP_RES_100K, 32, 1, 1),
+	MTK_PMIC_ADC_EXT_CHAN(VIN3,
+			      PMIC_AUXADC_RQST1, 4, PMIC_AUXADC_ADC0, 15,
+			      PMIC_AUXADC_SDMADC_CON0, 3, MT6363_PULLUP_RES_100K, 32, 1, 1),
+	MTK_PMIC_ADC_EXT_CHAN(VIN4,
+			      PMIC_AUXADC_RQST1, 4, PMIC_AUXADC_ADC0, 15,
+			      PMIC_AUXADC_SDMADC_CON0, 4, MT6363_PULLUP_RES_100K, 32, 1, 1),
+	MTK_PMIC_ADC_EXT_CHAN(VIN5,
+			      PMIC_AUXADC_RQST1, 4, PMIC_AUXADC_ADC0, 15,
+			      PMIC_AUXADC_SDMADC_CON0, 5, MT6363_PULLUP_RES_100K, 32, 1, 1),
+	MTK_PMIC_ADC_EXT_CHAN(VIN6,
+			      PMIC_AUXADC_RQST1, 4, PMIC_AUXADC_ADC0, 15,
+			      PMIC_AUXADC_SDMADC_CON0, 6, MT6363_PULLUP_RES_100K, 32, 1, 1),
+	MTK_PMIC_ADC_EXT_CHAN(VIN7,
+			      PMIC_AUXADC_RQST1, 4, PMIC_AUXADC_ADC0, 15,
+			      PMIC_AUXADC_SDMADC_CON0, 7, MT6363_PULLUP_RES_100K, 32, 1, 1),
+};
+
+static const u16 mt6363_auxadc_regs[] = {
+	[PMIC_AUXADC_RQST0]	= 0x1108,
+	[PMIC_AUXADC_RQST1]	= 0x1109,
+	[PMIC_AUXADC_RQST3]	= 0x110c,
+	[PMIC_AUXADC_ADC0]	= 0x1088,
+	[PMIC_AUXADC_IMP0]	= 0x1208,
+	[PMIC_AUXADC_IMP1]	= 0x1209,
+};
+
 static void mt6358_stop_imp_conv(struct mt6359_auxadc *adc_dev)
 {
 	const struct mtk_pmic_auxadc_info *cinfo = adc_dev->chip_info;
@@ -379,13 +488,13 @@ static int mt6359_read_imp(struct mt6359_auxadc *adc_dev,
 	int ret;
 
 	/* Start conversion */
-	regmap_write(regmap, cinfo->regs[PMIC_AUXADC_IMP0], MT6359_IMP0_CONV_EN);
+	regmap_write(regmap, cinfo->regs[desc->req_idx], desc->req_mask);
 	ret = regmap_read_poll_timeout(regmap, cinfo->regs[desc->rdy_idx],
 				       val, val & desc->rdy_mask,
 				       IMP_POLL_DELAY_US, AUXADC_TIMEOUT_US);
 
 	/* Stop conversion regardless of the result */
-	regmap_write(regmap, cinfo->regs[PMIC_AUXADC_IMP0], 0);
+	regmap_write(regmap, cinfo->regs[desc->req_idx], 0);
 	if (ret)
 		return ret;
 
@@ -416,6 +525,7 @@ static const struct mtk_pmic_auxadc_info mt6357_chip_info = {
 	.regs = mt6357_auxadc_regs,
 	.imp_adc_num = MT6357_IMP_ADC_NUM,
 	.read_imp = mt6358_read_imp,
+	.vref_mv = 1800,
 };
 
 static const struct mtk_pmic_auxadc_info mt6358_chip_info = {
@@ -426,6 +536,7 @@ static const struct mtk_pmic_auxadc_info mt6358_chip_info = {
 	.regs = mt6358_auxadc_regs,
 	.imp_adc_num = MT6358_IMP_ADC_NUM,
 	.read_imp = mt6358_read_imp,
+	.vref_mv = 1800,
 };
 
 static const struct mtk_pmic_auxadc_info mt6359_chip_info = {
@@ -436,6 +547,17 @@ static const struct mtk_pmic_auxadc_info mt6359_chip_info = {
 	.regs = mt6359_auxadc_regs,
 	.sec_unlock_key = 0x6359,
 	.read_imp = mt6359_read_imp,
+	.vref_mv = 1800,
+};
+
+static const struct mtk_pmic_auxadc_info mt6363_chip_info = {
+	.model_name = "MT6363",
+	.channels = mt6363_auxadc_channels,
+	.num_channels = ARRAY_SIZE(mt6363_auxadc_channels),
+	.desc = mt6363_auxadc_ch_desc,
+	.regs = mt6363_auxadc_regs,
+	.flags = MTK_PMIC_AUXADC_IS_SPMI | MTK_PMIC_AUXADC_NO_RESET,
+	.vref_mv = 1840,
 };
 
 static void mt6359_auxadc_reset(struct mt6359_auxadc *adc_dev)
@@ -464,27 +586,74 @@ static int mt6359_auxadc_read_adc(struct mt6359_auxadc *adc_dev,
 	const struct mtk_pmic_auxadc_info *cinfo = adc_dev->chip_info;
 	const struct mtk_pmic_auxadc_chan *desc = &cinfo->desc[chan->scan_index];
 	struct regmap *regmap = adc_dev->regmap;
-	u32 val;
+	u32 reg, rdy_mask, val, lval;
+	u8 ext_sel;
 	int ret;
 
+	if (desc->ext_sel_idx >= 0) {
+		ext_sel = FIELD_PREP(MT6363_EXT_PURES_MASK, desc->ext_sel_pu);
+		ext_sel |= FIELD_PREP(MT6363_EXT_CHAN_MASK, desc->ext_sel_ch);
+
+		ret = regmap_update_bits(regmap, cinfo->regs[desc->ext_sel_idx],
+					 MT6363_EXT_PURES_MASK | MT6363_EXT_CHAN_MASK,
+					 ext_sel);
+		if (ret)
+			return ret;
+	}
+
 	/* Request to start sampling for ADC channel */
 	ret = regmap_write(regmap, cinfo->regs[desc->req_idx], desc->req_mask);
 	if (ret)
-		return ret;
+		goto end;
 
 	/* Wait until all samples are averaged */
 	fsleep(desc->num_samples * AUXADC_AVG_TIME_US);
 
-	ret = regmap_read_poll_timeout(regmap,
-				       cinfo->regs[PMIC_AUXADC_ADC0] + (chan->address << 1),
-				       val, val & PMIC_AUXADC_RDY_BIT,
+	reg = cinfo->regs[PMIC_AUXADC_ADC0] + (chan->address << 1);
+	rdy_mask = PMIC_AUXADC_RDY_BIT;
+
+	/*
+	 * Even though for both PWRAP and SPMI cases the ADC HW signals that
+	 * the data is ready by setting AUXADC_RDY_BIT, for SPMI the register
+	 * read is only 8 bits long: for this case, the check has to be done
+	 * on the ADC(x)_H register (high bits) and the rdy_mask needs to be
+	 * shifted to the right by the same 8 bits.
+	 */
+	if (MTK_AUXADC_HAS_FLAG(cinfo, IS_SPMI)) {
+		rdy_mask >>= 8;
+		reg += 1;
+	}
+
+	ret = regmap_read_poll_timeout(regmap, reg, val, val & rdy_mask,
 				       AUXADC_POLL_DELAY_US, AUXADC_TIMEOUT_US);
-	if (ret)
-		return ret;
+	if (ret) {
+		dev_dbg(adc_dev->dev, "ADC read timeout for chan %lu\n", chan->address);
+		goto end;
+	}
+
+	if (MTK_AUXADC_HAS_FLAG(cinfo, IS_SPMI)) {
+		/* If the previous read succeeded, this can't fail */
+		regmap_read(regmap, reg - 1, &lval);
+		val = (val << 8) | lval;
+	}
 
-	/* Stop sampling */
+end:
+	/* Stop sampling unconditionally... */
 	regmap_write(regmap, cinfo->regs[desc->req_idx], 0);
 
+	/* ...and deactivate the ADC GPIO if previously done */
+	if (desc->ext_sel_idx >= 0) {
+		ext_sel = FIELD_PREP(MT6363_EXT_PURES_MASK, MT6363_PULLUP_RES_OPEN);
+
+		regmap_update_bits(regmap, cinfo->regs[desc->ext_sel_idx],
+				   MT6363_EXT_PURES_MASK, ext_sel);
+	}
+
+	/* Check if we reached this point because of an error or regular flow */
+	if (ret)
+		return ret;
+
+	/* Everything went fine, give back the ADC reading */
 	*out = val & GENMASK(chan->scan_type.realbits - 1, 0);
 	return 0;
 }
@@ -505,7 +674,7 @@ static int mt6359_auxadc_read_raw(struct iio_dev *indio_dev,
 	int ret;
 
 	if (mask == IIO_CHAN_INFO_SCALE) {
-		*val = desc->r_ratio.numerator * AUXADC_VOLT_FULL;
+		*val = desc->r_ratio.numerator * (u32)cinfo->vref_mv;
 
 		if (desc->r_ratio.denominator > 1) {
 			*val2 = desc->r_ratio.denominator;
@@ -518,9 +687,15 @@ static int mt6359_auxadc_read_raw(struct iio_dev *indio_dev,
 	scoped_guard(mutex, &adc_dev->lock) {
 		switch (chan->scan_index) {
 		case PMIC_AUXADC_CHAN_IBAT:
+			if (!adc_dev->chip_info->read_imp)
+				return -EOPNOTSUPP;
+
 			ret = adc_dev->chip_info->read_imp(adc_dev, chan, NULL, val);
 			break;
 		case PMIC_AUXADC_CHAN_VBAT:
+			if (!adc_dev->chip_info->read_imp)
+				return -EOPNOTSUPP;
+
 			ret = adc_dev->chip_info->read_imp(adc_dev, chan, val, NULL);
 			break;
 		default:
@@ -535,7 +710,8 @@ static int mt6359_auxadc_read_raw(struct iio_dev *indio_dev,
 		 * AUXADC is stuck: perform a full reset to recover it.
 		 */
 		if (ret == -ETIMEDOUT) {
-			if (adc_dev->timed_out) {
+			if (!MTK_AUXADC_HAS_FLAG(cinfo, NO_RESET) &&
+			    adc_dev->timed_out) {
 				dev_warn(adc_dev->dev, "Resetting stuck ADC!\r\n");
 				mt6359_auxadc_reset(adc_dev);
 			}
@@ -555,15 +731,36 @@ static const struct iio_info mt6359_auxadc_iio_info = {
 
 static int mt6359_auxadc_probe(struct platform_device *pdev)
 {
+	const struct mtk_pmic_auxadc_info *chip_info;
 	struct device *dev = &pdev->dev;
-	struct device *mt6397_mfd_dev = dev->parent;
+	struct device *mfd_dev = dev->parent;
 	struct mt6359_auxadc *adc_dev;
 	struct iio_dev *indio_dev;
+	struct device *regmap_dev;
 	struct regmap *regmap;
 	int ret;
 
+	chip_info = device_get_match_data(dev);
+	if (!chip_info)
+		return -EINVAL;
+	/*
+	 * The regmap for this device has to be acquired differently for
+	 * SoC PMIC Wrapper and SPMI PMIC cases:
+	 *
+	 * If this is under SPMI, the regmap comes from the direct parent of
+	 * this driver: this_device->parent(mfd).
+	 *                            ... or ...
+	 * If this is under the SoC PMIC Wrapper, the regmap comes from the
+	 * parent of the MT6397 MFD: this_device->parent(mfd)->parent(pwrap)
+	 */
+	if (MTK_AUXADC_HAS_FLAG(chip_info, IS_SPMI))
+		regmap_dev = mfd_dev;
+	else
+		regmap_dev = mfd_dev->parent;
+
+
 	/* Regmap is from SoC PMIC Wrapper, parent of the mt6397 MFD */
-	regmap = dev_get_regmap(mt6397_mfd_dev->parent, NULL);
+	regmap = dev_get_regmap(regmap_dev, NULL);
 	if (!regmap)
 		return dev_err_probe(dev, -ENODEV, "Failed to get regmap\n");
 
@@ -574,14 +771,12 @@ static int mt6359_auxadc_probe(struct platform_device *pdev)
 	adc_dev = iio_priv(indio_dev);
 	adc_dev->regmap = regmap;
 	adc_dev->dev = dev;
-
-	adc_dev->chip_info = device_get_match_data(dev);
-	if (!adc_dev->chip_info)
-		return -EINVAL;
+	adc_dev->chip_info = chip_info;
 
 	mutex_init(&adc_dev->lock);
 
-	mt6359_auxadc_reset(adc_dev);
+	if (!MTK_AUXADC_HAS_FLAG(chip_info, NO_RESET))
+		mt6359_auxadc_reset(adc_dev);
 
 	indio_dev->name = adc_dev->chip_info->model_name;
 	indio_dev->info = &mt6359_auxadc_iio_info;
@@ -600,6 +795,7 @@ static const struct of_device_id mt6359_auxadc_of_match[] = {
 	{ .compatible = "mediatek,mt6357-auxadc", .data = &mt6357_chip_info },
 	{ .compatible = "mediatek,mt6358-auxadc", .data = &mt6358_chip_info },
 	{ .compatible = "mediatek,mt6359-auxadc", .data = &mt6359_chip_info },
+	{ .compatible = "mediatek,mt6363-auxadc", .data = &mt6363_chip_info },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, mt6359_auxadc_of_match);
-- 
2.49.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v1 5/5] iio: adc: mt6359: Add support for MediaTek MT6373 PMIC AUXADC
  2025-06-23 12:00 [PATCH v1 0/5] iio: Add support for MT6363/6373 Auxiliary ADC AngeloGioacchino Del Regno
                   ` (3 preceding siblings ...)
  2025-06-23 12:00 ` [PATCH v1 4/5] iio: adc: mt6359: Add support for MediaTek MT6363 PMIC AUXADC AngeloGioacchino Del Regno
@ 2025-06-23 12:00 ` AngeloGioacchino Del Regno
  2025-06-24 15:08 ` [PATCH v1 0/5] iio: Add support for MT6363/6373 Auxiliary ADC Nícolas F. R. A. Prado
  5 siblings, 0 replies; 14+ messages in thread
From: AngeloGioacchino Del Regno @ 2025-06-23 12:00 UTC (permalink / raw)
  To: jic23
  Cc: dlechner, nuno.sa, andy, robh, krzk+dt, conor+dt, matthias.bgg,
	angelogioacchino.delregno, linux-iio, devicetree, linux-kernel,
	linux-arm-kernel, linux-mediatek, kernel

MediaTek MT6373 is a PMIC found on MT8196/MT6991 board designs
and communicates with the SoC over SPMI.

This PMIC integrates an Auxiliary ADC (AUXADC) which has a grand
total of 54 channels, of which usually only 9 are used as this
is usually paired with MT6363 on the same board.

For the Auxiliary ADC part, this reuses the same register layout
as the MT6363 PMIC, but exposes only a subset of the ADC chans.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/iio/adc/mt6359-auxadc.c | 49 +++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/drivers/iio/adc/mt6359-auxadc.c b/drivers/iio/adc/mt6359-auxadc.c
index f49b0b6e78da..909586282c1e 100644
--- a/drivers/iio/adc/mt6359-auxadc.c
+++ b/drivers/iio/adc/mt6359-auxadc.c
@@ -51,6 +51,7 @@
 #define MT6363_EXT_CHAN_MASK		GENMASK(2, 0)
 #define MT6363_EXT_PURES_MASK		GENMASK(4, 3)
  #define MT6363_PULLUP_RES_100K		0
+ #define MT6363_PULLUP_RES_30K		1
  #define MT6363_PULLUP_RES_OPEN		3
 
 #define MTK_AUXADC_HAS_FLAG(pdata, msk)	((pdata)->flags & (MTK_PMIC_AUXADC_##msk))
@@ -419,6 +420,43 @@ static const u16 mt6363_auxadc_regs[] = {
 	[PMIC_AUXADC_IMP1]	= 0x1209,
 };
 
+static const struct iio_chan_spec mt6373_auxadc_channels[] = {
+	MTK_PMIC_IIO_CHAN(MT6363, chip_temp, CHIP_TEMP, 4, 12, IIO_TEMP),
+	MTK_PMIC_IIO_CHAN(MT6363, vcore_temp, VCORE_TEMP, 38, 12, IIO_TEMP),
+	MTK_PMIC_IIO_CHAN(MT6363, vproc_temp, VPROC_TEMP, 39, 12, IIO_TEMP),
+	MTK_PMIC_IIO_CHAN(MT6363, vgpu_temp, VGPU_TEMP, 40, 12, IIO_TEMP),
+
+	/* For VIN, ADC12 holds the result depending on which GPIO was activated */
+	MTK_PMIC_IIO_CHAN(MT6363, in1_v, VIN1, 45, 15, IIO_VOLTAGE),
+	MTK_PMIC_IIO_CHAN(MT6363, in2_v, VIN2, 45, 15, IIO_VOLTAGE),
+	MTK_PMIC_IIO_CHAN(MT6363, in3_v, VIN3, 45, 15, IIO_VOLTAGE),
+	MTK_PMIC_IIO_CHAN(MT6363, in4_v, VIN4, 45, 15, IIO_VOLTAGE),
+	MTK_PMIC_IIO_CHAN(MT6363, in5_v, VIN5, 45, 15, IIO_VOLTAGE),
+};
+
+static const struct mtk_pmic_auxadc_chan mt6373_auxadc_ch_desc[] = {
+	MTK_PMIC_ADC_CHAN(CHIP_TEMP, PMIC_AUXADC_RQST0, 4, PMIC_AUXADC_ADC0, 15, 32, 1, 1),
+	MTK_PMIC_ADC_CHAN(VCORE_TEMP, PMIC_AUXADC_RQST3, 0, PMIC_AUXADC_ADC0, 15, 32, 1, 1),
+	MTK_PMIC_ADC_CHAN(VPROC_TEMP, PMIC_AUXADC_RQST3, 1, PMIC_AUXADC_ADC0, 15, 32, 1, 1),
+	MTK_PMIC_ADC_CHAN(VGPU_TEMP, PMIC_AUXADC_RQST3, 2, PMIC_AUXADC_ADC0, 15, 32, 1, 1),
+
+	MTK_PMIC_ADC_EXT_CHAN(VIN1,
+			      PMIC_AUXADC_RQST1, 4, PMIC_AUXADC_ADC0, 15,
+			      PMIC_AUXADC_SDMADC_CON0, 1, MT6363_PULLUP_RES_30K, 32, 1, 1),
+	MTK_PMIC_ADC_EXT_CHAN(VIN2,
+			      PMIC_AUXADC_RQST1, 4, PMIC_AUXADC_ADC0, 15,
+			      PMIC_AUXADC_SDMADC_CON0, 2, MT6363_PULLUP_RES_OPEN, 32, 1, 1),
+	MTK_PMIC_ADC_EXT_CHAN(VIN3,
+			      PMIC_AUXADC_RQST1, 4, PMIC_AUXADC_ADC0, 15,
+			      PMIC_AUXADC_SDMADC_CON0, 3, MT6363_PULLUP_RES_OPEN, 32, 1, 1),
+	MTK_PMIC_ADC_EXT_CHAN(VIN4,
+			      PMIC_AUXADC_RQST1, 4, PMIC_AUXADC_ADC0, 15,
+			      PMIC_AUXADC_SDMADC_CON0, 4, MT6363_PULLUP_RES_OPEN, 32, 1, 1),
+	MTK_PMIC_ADC_EXT_CHAN(VIN5,
+			      PMIC_AUXADC_RQST1, 4, PMIC_AUXADC_ADC0, 15,
+			      PMIC_AUXADC_SDMADC_CON0, 5, MT6363_PULLUP_RES_OPEN, 32, 1, 1),
+};
+
 static void mt6358_stop_imp_conv(struct mt6359_auxadc *adc_dev)
 {
 	const struct mtk_pmic_auxadc_info *cinfo = adc_dev->chip_info;
@@ -560,6 +598,16 @@ static const struct mtk_pmic_auxadc_info mt6363_chip_info = {
 	.vref_mv = 1840,
 };
 
+static const struct mtk_pmic_auxadc_info mt6373_chip_info = {
+	.model_name = "MT6373",
+	.channels = mt6373_auxadc_channels,
+	.num_channels = ARRAY_SIZE(mt6373_auxadc_channels),
+	.desc = mt6373_auxadc_ch_desc,
+	.regs = mt6363_auxadc_regs,
+	.flags = MTK_PMIC_AUXADC_IS_SPMI | MTK_PMIC_AUXADC_NO_RESET,
+	.vref_mv = 1840,
+};
+
 static void mt6359_auxadc_reset(struct mt6359_auxadc *adc_dev)
 {
 	const struct mtk_pmic_auxadc_info *cinfo = adc_dev->chip_info;
@@ -796,6 +844,7 @@ static const struct of_device_id mt6359_auxadc_of_match[] = {
 	{ .compatible = "mediatek,mt6358-auxadc", .data = &mt6358_chip_info },
 	{ .compatible = "mediatek,mt6359-auxadc", .data = &mt6359_chip_info },
 	{ .compatible = "mediatek,mt6363-auxadc", .data = &mt6363_chip_info },
+	{ .compatible = "mediatek,mt6373-auxadc", .data = &mt6373_chip_info },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, mt6359_auxadc_of_match);
-- 
2.49.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v1 4/5] iio: adc: mt6359: Add support for MediaTek MT6363 PMIC AUXADC
  2025-06-23 12:00 ` [PATCH v1 4/5] iio: adc: mt6359: Add support for MediaTek MT6363 PMIC AUXADC AngeloGioacchino Del Regno
@ 2025-06-23 14:30   ` Andy Shevchenko
  2025-06-25 13:29     ` AngeloGioacchino Del Regno
  2025-06-28 16:01   ` Jonathan Cameron
  1 sibling, 1 reply; 14+ messages in thread
From: Andy Shevchenko @ 2025-06-23 14:30 UTC (permalink / raw)
  To: AngeloGioacchino Del Regno
  Cc: jic23, dlechner, nuno.sa, andy, robh, krzk+dt, conor+dt,
	matthias.bgg, linux-iio, devicetree, linux-kernel,
	linux-arm-kernel, linux-mediatek, kernel

On Mon, Jun 23, 2025 at 02:00:27PM +0200, AngeloGioacchino Del Regno wrote:
> MediaTek MT6363 is a PMIC found on MT8196/MT6991 board designs
> and communicates with the SoC over SPMI.
> 
> This PMIC integrates an Auxiliary ADC (AUXADC) which has a grand
> total of 54 ADC channels: 49 PMIC-internal channels, 2 external
> NTC thermistor channels and 2 generic ADC channels (mapped to 7
> PMIC ADC external inputs).
> 
> To use a generic ADC channel it is necessary to enable one of
> the PMIC ADC inputs at a time and only then start the reading,
> so in this case it is possible to read only one external input
> for each generic ADC channel.
> 
> Due to the lack of documentation, this implementation supports
> using only one generic ADC channel, hence supports reading only
> one external input at a time.

> +#define MT6363_EXT_CHAN_MASK		GENMASK(2, 0)
> +#define MT6363_EXT_PURES_MASK		GENMASK(4, 3)
> + #define MT6363_PULLUP_RES_100K		0
> + #define MT6363_PULLUP_RES_OPEN		3

I would rather expect the two spaces after #define. This most likely will break
syntax highlighting in (some of) the editors.

...

> +#define MTK_PMIC_ADC_EXT_CHAN(_ch_idx, _req_idx, _req_bit, _rdy_idx, _rdy_bit,	\
> +			      _ext_sel_idx, _ext_sel_ch, _ext_sel_pu,		\
> +			      _samples, _rnum, _rdiv)				\

Wondering, and it's out of scope here, if we can go to use a macro for
initialization of struct *_fract.

>  	[PMIC_AUXADC_CHAN_##_ch_idx] = {					\
>  		.req_idx = _req_idx,						\
>  		.req_mask = BIT(_req_bit),					\
>  		.rdy_idx = _rdy_idx,						\
>  		.rdy_mask = BIT(_rdy_bit),					\
> +		.ext_sel_idx = _ext_sel_idx,					\
> +		.ext_sel_ch = _ext_sel_ch,					\
> +		.ext_sel_pu = _ext_sel_pu,					\
>  		.num_samples = _samples,					\
>  		.r_ratio = { _rnum, _rdiv }					\
>  	}

Perhaps something in math.h as

#define INIT_STRUCT_FRACT_UXX(n, d) ...

...

> +	if (MTK_AUXADC_HAS_FLAG(cinfo, IS_SPMI)) {
> +		/* If the previous read succeeded, this can't fail */
> +		regmap_read(regmap, reg - 1, &lval);

No error check? lval may contain garbage here, right?

> +		val = (val << 8) | lval;

Is it guaranteed that lval is always less than 256 (if unsigned)?

> +	}

...

> +		regmap_update_bits(regmap, cinfo->regs[desc->ext_sel_idx],
> +				   MT6363_EXT_PURES_MASK, ext_sel);

No  error check?

> +	}

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v1 0/5] iio: Add support for MT6363/6373 Auxiliary ADC
  2025-06-23 12:00 [PATCH v1 0/5] iio: Add support for MT6363/6373 Auxiliary ADC AngeloGioacchino Del Regno
                   ` (4 preceding siblings ...)
  2025-06-23 12:00 ` [PATCH v1 5/5] iio: adc: mt6359: Add support for MediaTek MT6373 " AngeloGioacchino Del Regno
@ 2025-06-24 15:08 ` Nícolas F. R. A. Prado
  5 siblings, 0 replies; 14+ messages in thread
From: Nícolas F. R. A. Prado @ 2025-06-24 15:08 UTC (permalink / raw)
  To: AngeloGioacchino Del Regno, jic23
  Cc: dlechner, nuno.sa, andy, robh, krzk+dt, conor+dt, matthias.bgg,
	linux-iio, devicetree, linux-kernel, linux-arm-kernel,
	linux-mediatek, kernel

On Mon, 2025-06-23 at 14:00 +0200, AngeloGioacchino Del Regno wrote:
> This series adds support for the Auxiliary ADC IP found on the new
> MediaTek MT6363 and MT6373 PMICs, found on board designs featuring
> the MT8196 Chromebook SoC or the MT6991 Dimensity 9400 Smartphone
> SoC.
> 
> AngeloGioacchino Del Regno (5):
>   dt-bindings: iio: adc: mt6359: Add bindings for MT6363 PMIC AuxADC
>   dt-bindings: iio: adc: mt6359: Add bindings for MT6373 PMIC AuxADC
>   iio: adc: mt6359: Add ready register index and mask to channel data
>   iio: adc: mt6359: Add support for MediaTek MT6363 PMIC AUXADC
>   iio: adc: mt6359: Add support for MediaTek MT6373 PMIC AUXADC

For the entire series:

Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

(as I internally reviewed this before submission)

-- 
Thanks,

Nícolas

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v1 4/5] iio: adc: mt6359: Add support for MediaTek MT6363 PMIC AUXADC
  2025-06-23 14:30   ` Andy Shevchenko
@ 2025-06-25 13:29     ` AngeloGioacchino Del Regno
  2025-06-25 14:27       ` Andy Shevchenko
  0 siblings, 1 reply; 14+ messages in thread
From: AngeloGioacchino Del Regno @ 2025-06-25 13:29 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: jic23, dlechner, nuno.sa, andy, robh, krzk+dt, conor+dt,
	matthias.bgg, linux-iio, devicetree, linux-kernel,
	linux-arm-kernel, linux-mediatek, kernel

Il 23/06/25 16:30, Andy Shevchenko ha scritto:
> On Mon, Jun 23, 2025 at 02:00:27PM +0200, AngeloGioacchino Del Regno wrote:
>> MediaTek MT6363 is a PMIC found on MT8196/MT6991 board designs
>> and communicates with the SoC over SPMI.
>>
>> This PMIC integrates an Auxiliary ADC (AUXADC) which has a grand
>> total of 54 ADC channels: 49 PMIC-internal channels, 2 external
>> NTC thermistor channels and 2 generic ADC channels (mapped to 7
>> PMIC ADC external inputs).
>>
>> To use a generic ADC channel it is necessary to enable one of
>> the PMIC ADC inputs at a time and only then start the reading,
>> so in this case it is possible to read only one external input
>> for each generic ADC channel.
>>
>> Due to the lack of documentation, this implementation supports
>> using only one generic ADC channel, hence supports reading only
>> one external input at a time.
> 
>> +#define MT6363_EXT_CHAN_MASK		GENMASK(2, 0)
>> +#define MT6363_EXT_PURES_MASK		GENMASK(4, 3)
>> + #define MT6363_PULLUP_RES_100K		0
>> + #define MT6363_PULLUP_RES_OPEN		3
> 
> I would rather expect the two spaces after #define. This most likely will break
> syntax highlighting in (some of) the editors.
> 

I can change that no problem (or if this can be changed while applying, that'd
buy me some time and I'd appreciate that a lot)

> ...
> 
>> +#define MTK_PMIC_ADC_EXT_CHAN(_ch_idx, _req_idx, _req_bit, _rdy_idx, _rdy_bit,	\
>> +			      _ext_sel_idx, _ext_sel_ch, _ext_sel_pu,		\
>> +			      _samples, _rnum, _rdiv)				\
> 
> Wondering, and it's out of scope here, if we can go to use a macro for
> initialization of struct *_fract.
> 
>>   	[PMIC_AUXADC_CHAN_##_ch_idx] = {					\
>>   		.req_idx = _req_idx,						\
>>   		.req_mask = BIT(_req_bit),					\
>>   		.rdy_idx = _rdy_idx,						\
>>   		.rdy_mask = BIT(_rdy_bit),					\
>> +		.ext_sel_idx = _ext_sel_idx,					\
>> +		.ext_sel_ch = _ext_sel_ch,					\
>> +		.ext_sel_pu = _ext_sel_pu,					\
>>   		.num_samples = _samples,					\
>>   		.r_ratio = { _rnum, _rdiv }					\
>>   	}
> 
> Perhaps something in math.h as
> 
> #define INIT_STRUCT_FRACT_UXX(n, d) ...

Not sure... honestly, at a first glance it looks like a macro would only make
a longer line and nothing else...

...but - effectively - I can see a benefit for a INIT_CONST_STRUCT_FRACT_Uxx(n, d)
where we could perform a build-time check for division by zero.

I'm not sure how many users would there be of such a macro, ideas?

> 
> ...
> 
>> +	if (MTK_AUXADC_HAS_FLAG(cinfo, IS_SPMI)) {
>> +		/* If the previous read succeeded, this can't fail */
>> +		regmap_read(regmap, reg - 1, &lval);
> 
> No error check? lval may contain garbage here, right?
> 

No, because if the previous read succeeded, this can't fail, and also cannot ever
possibly contain garbage (and if it does, - but again, that can't happen - there is
no way to validate that because valid values are [0x00..0xff] anyway).

>> +		val = (val << 8) | lval;
> 
> Is it guaranteed that lval is always less than 256 (if unsigned)?
> 

Yes, with SPMI that is guaranteed.

>> +	}
> 
> ...
> 
>> +		regmap_update_bits(regmap, cinfo->regs[desc->ext_sel_idx],
>> +				   MT6363_EXT_PURES_MASK, ext_sel);
> 
> No  error check?
> 

No, because if the previous reads and/or writes succeeded, it is impossible for
this to fail :-)

Cheers,
Angelo


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v1 4/5] iio: adc: mt6359: Add support for MediaTek MT6363 PMIC AUXADC
  2025-06-25 13:29     ` AngeloGioacchino Del Regno
@ 2025-06-25 14:27       ` Andy Shevchenko
  0 siblings, 0 replies; 14+ messages in thread
From: Andy Shevchenko @ 2025-06-25 14:27 UTC (permalink / raw)
  To: AngeloGioacchino Del Regno
  Cc: jic23, dlechner, nuno.sa, andy, robh, krzk+dt, conor+dt,
	matthias.bgg, linux-iio, devicetree, linux-kernel,
	linux-arm-kernel, linux-mediatek, kernel

On Wed, Jun 25, 2025 at 03:29:47PM +0200, AngeloGioacchino Del Regno wrote:
> Il 23/06/25 16:30, Andy Shevchenko ha scritto:
> > On Mon, Jun 23, 2025 at 02:00:27PM +0200, AngeloGioacchino Del Regno wrote:

...

> > > +	if (MTK_AUXADC_HAS_FLAG(cinfo, IS_SPMI)) {
> > > +		/* If the previous read succeeded, this can't fail */
> > > +		regmap_read(regmap, reg - 1, &lval);
> > 
> > No error check? lval may contain garbage here, right?
> 
> No, because if the previous read succeeded, this can't fail, and also cannot ever
> possibly contain garbage (and if it does, - but again, that can't happen - there is
> no way to validate that because valid values are [0x00..0xff] anyway).

Never say never. Any regmap_*() call that performs I/O might fail. You can't
predict with 100% guarantee the HW behaviour in all possible scenarios.

> > > +		val = (val << 8) | lval;
> > 
> > Is it guaranteed that lval is always less than 256 (if unsigned)?
> 
> Yes, with SPMI that is guaranteed.
> 
> > > +	}

...

> > > +		regmap_update_bits(regmap, cinfo->regs[desc->ext_sel_idx],
> > > +				   MT6363_EXT_PURES_MASK, ext_sel);
> > 
> > No  error check?
> 
> No, because if the previous reads and/or writes succeeded, it is impossible for
> this to fail :-)

Ditto.

I.o.w. the failed regmap_*() call can be a signal that something on the
communication channel with the HW went wrong, Depending on the severity of this
call the device driver may decide what to do next.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v1 1/5] dt-bindings: iio: adc: mt6359: Add bindings for MT6363 PMIC AuxADC
  2025-06-23 12:00 ` [PATCH v1 1/5] dt-bindings: iio: adc: mt6359: Add bindings for MT6363 PMIC AuxADC AngeloGioacchino Del Regno
@ 2025-06-27 20:02   ` Rob Herring (Arm)
  0 siblings, 0 replies; 14+ messages in thread
From: Rob Herring (Arm) @ 2025-06-27 20:02 UTC (permalink / raw)
  To: AngeloGioacchino Del Regno
  Cc: linux-iio, linux-kernel, devicetree, matthias.bgg, dlechner,
	kernel, krzk+dt, jic23, linux-arm-kernel, linux-mediatek,
	conor+dt, andy, nuno.sa


On Mon, 23 Jun 2025 14:00:24 +0200, AngeloGioacchino Del Regno wrote:
> Add a compatible and channel bindings for MediaTek's MT6363 PMIC,
> featuring an Auxiliary ADC IP with 15 ADC channels used for both
> internal temperatures and voltages and for external voltage inputs.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  .../iio/adc/mediatek,mt6359-auxadc.yaml       |  1 +
>  .../iio/adc/mediatek,mt6363-auxadc.h          | 24 +++++++++++++++++++
>  2 files changed, 25 insertions(+)
>  create mode 100644 include/dt-bindings/iio/adc/mediatek,mt6363-auxadc.h
> 

Acked-by: Rob Herring (Arm) <robh@kernel.org>


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v1 2/5] dt-bindings: iio: adc: mt6359: Add bindings for MT6373 PMIC AuxADC
  2025-06-23 12:00 ` [PATCH v1 2/5] dt-bindings: iio: adc: mt6359: Add bindings for MT6373 " AngeloGioacchino Del Regno
@ 2025-06-27 20:04   ` Rob Herring
  0 siblings, 0 replies; 14+ messages in thread
From: Rob Herring @ 2025-06-27 20:04 UTC (permalink / raw)
  To: AngeloGioacchino Del Regno
  Cc: jic23, dlechner, nuno.sa, andy, krzk+dt, conor+dt, matthias.bgg,
	linux-iio, devicetree, linux-kernel, linux-arm-kernel,
	linux-mediatek, kernel

On Mon, Jun 23, 2025 at 02:00:25PM +0200, AngeloGioacchino Del Regno wrote:
> Add a compatible and channel bindings for MediaTek's MT6373 PMIC,
> featuring an Auxiliary ADC IP with 15 ADC channels for external
> (SoC) temperatures and external voltage inputs.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  .../iio/adc/mediatek,mt6359-auxadc.yaml       |  1 +
>  .../iio/adc/mediatek,mt6373-auxadc.h          | 19 +++++++++++++++++++
>  2 files changed, 20 insertions(+)
>  create mode 100644 include/dt-bindings/iio/adc/mediatek,mt6373-auxadc.h

You can drop 'bindings for ' in the subject. We already said 'bindings' 
once.

Acked-by: Rob Herring (Arm) <robh@kernel.org>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v1 4/5] iio: adc: mt6359: Add support for MediaTek MT6363 PMIC AUXADC
  2025-06-23 12:00 ` [PATCH v1 4/5] iio: adc: mt6359: Add support for MediaTek MT6363 PMIC AUXADC AngeloGioacchino Del Regno
  2025-06-23 14:30   ` Andy Shevchenko
@ 2025-06-28 16:01   ` Jonathan Cameron
  2025-07-03 13:05     ` AngeloGioacchino Del Regno
  1 sibling, 1 reply; 14+ messages in thread
From: Jonathan Cameron @ 2025-06-28 16:01 UTC (permalink / raw)
  To: AngeloGioacchino Del Regno
  Cc: dlechner, nuno.sa, andy, robh, krzk+dt, conor+dt, matthias.bgg,
	linux-iio, devicetree, linux-kernel, linux-arm-kernel,
	linux-mediatek, kernel

On Mon, 23 Jun 2025 14:00:27 +0200
AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote:

> MediaTek MT6363 is a PMIC found on MT8196/MT6991 board designs
> and communicates with the SoC over SPMI.
> 
> This PMIC integrates an Auxiliary ADC (AUXADC) which has a grand
> total of 54 ADC channels: 49 PMIC-internal channels, 2 external
> NTC thermistor channels and 2 generic ADC channels (mapped to 7
> PMIC ADC external inputs).
> 
> To use a generic ADC channel it is necessary to enable one of
> the PMIC ADC inputs at a time and only then start the reading,
> so in this case it is possible to read only one external input
> for each generic ADC channel.
> 
> Due to the lack of documentation, this implementation supports
> using only one generic ADC channel, hence supports reading only
> one external input at a time.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Hi,

A few comments that may or may not overlap with Andy's review.

thanks,

Jonathan

> ---
>  drivers/iio/adc/mt6359-auxadc.c | 238 +++++++++++++++++++++++++++++---
>  1 file changed, 217 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/iio/adc/mt6359-auxadc.c b/drivers/iio/adc/mt6359-auxadc.c
> index ae7b65f5f551..f49b0b6e78da 100644
> --- a/drivers/iio/adc/mt6359-auxadc.c
> +++ b/drivers/iio/adc/mt6359-auxadc.c

> +enum mtk_pmic_auxadc_flags {
> +	MTK_PMIC_AUXADC_IS_SPMI = BIT(0),
> +	MTK_PMIC_AUXADC_NO_RESET = BIT(1),
> +};

With just two bits I think flags obscures what is going on over
a pair of separate booleans.

>  };
> @@ -123,7 +155,9 @@ struct mtk_pmic_auxadc_chan {
>   * @desc:           PMIC AUXADC channel data
>   * @regs:           List of PMIC specific registers
>   * @sec_unlock_key: Security unlock key for HK_TOP writes
> + * @vref_mv:        AUXADC Reference Voltage (VREF) in millivolts
>   * @imp_adc_num:    ADC channel for battery impedance readings
> + * @flags:          Feature flags
>   * @read_imp:       Callback to read impedance channels
>   */
>  struct mtk_pmic_auxadc_info {
> @@ -133,22 +167,33 @@ struct mtk_pmic_auxadc_info {
>  	const struct mtk_pmic_auxadc_chan *desc;
>  	const u16 *regs;
>  	u16 sec_unlock_key;
> +	u16 vref_mv;
I'd not worry about the space saving here and instead make this a u32 so that
can avoid the casting when using this.

>  	u8 imp_adc_num;
> +	u8 flags;

As above. Pair of bool preferred.

>  	int (*read_imp)(struct mt6359_auxadc *adc_dev,
>  			const struct iio_chan_spec *chan, int *vbat, int *ibat);
>  };

>  static void mt6358_stop_imp_conv(struct mt6359_auxadc *adc_dev)
>  {
>  	const struct mtk_pmic_auxadc_info *cinfo = adc_dev->chip_info;
> @@ -379,13 +488,13 @@ static int mt6359_read_imp(struct mt6359_auxadc *adc_dev,
>  	int ret;
>  
>  	/* Start conversion */
> -	regmap_write(regmap, cinfo->regs[PMIC_AUXADC_IMP0], MT6359_IMP0_CONV_EN);
> +	regmap_write(regmap, cinfo->regs[desc->req_idx], desc->req_mask);

Given desc->req_idx is not introduced in this patch, why is this needed now
but not previously?  Maybe this change belongs in a separate patch with
a description to explain that.

>  	ret = regmap_read_poll_timeout(regmap, cinfo->regs[desc->rdy_idx],
>  				       val, val & desc->rdy_mask,
>  				       IMP_POLL_DELAY_US, AUXADC_TIMEOUT_US);
>  
>  	/* Stop conversion regardless of the result */
> -	regmap_write(regmap, cinfo->regs[PMIC_AUXADC_IMP0], 0);
> +	regmap_write(regmap, cinfo->regs[desc->req_idx], 0);
>  	if (ret)
>  		return ret;
>  
> @@ -416,6 +525,7 @@ static const struct mtk_pmic_auxadc_info mt6357_chip_info = {
>  	.regs = mt6357_auxadc_regs,
>  	.imp_adc_num = MT6357_IMP_ADC_NUM,
>  	.read_imp = mt6358_read_imp,
> +	.vref_mv = 1800,
>  };
>  
>  static const struct mtk_pmic_auxadc_info mt6358_chip_info = {
> @@ -426,6 +536,7 @@ static const struct mtk_pmic_auxadc_info mt6358_chip_info = {
>  	.regs = mt6358_auxadc_regs,
>  	.imp_adc_num = MT6358_IMP_ADC_NUM,
>  	.read_imp = mt6358_read_imp,
> +	.vref_mv = 1800,
>  };
>  
>  static const struct mtk_pmic_auxadc_info mt6359_chip_info = {
> @@ -436,6 +547,17 @@ static const struct mtk_pmic_auxadc_info mt6359_chip_info = {
>  	.regs = mt6359_auxadc_regs,
>  	.sec_unlock_key = 0x6359,
>  	.read_imp = mt6359_read_imp,
> +	.vref_mv = 1800,

Add vref_mv and code using it in a precursor patch.  Not a problem that all
vref_mv will be 1800 at that point.  That way we can quickly see that it
has no affect on existing parts, and simplify what is present in this patch.

> +};
> +
> +static const struct mtk_pmic_auxadc_info mt6363_chip_info = {
> +	.model_name = "MT6363",
> +	.channels = mt6363_auxadc_channels,
> +	.num_channels = ARRAY_SIZE(mt6363_auxadc_channels),
> +	.desc = mt6363_auxadc_ch_desc,
> +	.regs = mt6363_auxadc_regs,
> +	.flags = MTK_PMIC_AUXADC_IS_SPMI | MTK_PMIC_AUXADC_NO_RESET,
> +	.vref_mv = 1840,
>  };
>  
>  static void mt6359_auxadc_reset(struct mt6359_auxadc *adc_dev)
> @@ -464,27 +586,74 @@ static int mt6359_auxadc_read_adc(struct mt6359_auxadc *adc_dev,
>  	const struct mtk_pmic_auxadc_info *cinfo = adc_dev->chip_info;
>  	const struct mtk_pmic_auxadc_chan *desc = &cinfo->desc[chan->scan_index];
>  	struct regmap *regmap = adc_dev->regmap;
> -	u32 val;
> +	u32 reg, rdy_mask, val, lval;
> +	u8 ext_sel;
>  	int ret;
>  
> +	if (desc->ext_sel_idx >= 0) {
> +		ext_sel = FIELD_PREP(MT6363_EXT_PURES_MASK, desc->ext_sel_pu);
> +		ext_sel |= FIELD_PREP(MT6363_EXT_CHAN_MASK, desc->ext_sel_ch);
> +
> +		ret = regmap_update_bits(regmap, cinfo->regs[desc->ext_sel_idx],
> +					 MT6363_EXT_PURES_MASK | MT6363_EXT_CHAN_MASK,
> +					 ext_sel);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	/* Request to start sampling for ADC channel */
>  	ret = regmap_write(regmap, cinfo->regs[desc->req_idx], desc->req_mask);
>  	if (ret)
> -		return ret;
> +		goto end;
>  
>  	/* Wait until all samples are averaged */
>  	fsleep(desc->num_samples * AUXADC_AVG_TIME_US);
>  
> -	ret = regmap_read_poll_timeout(regmap,
> -				       cinfo->regs[PMIC_AUXADC_ADC0] + (chan->address << 1),
> -				       val, val & PMIC_AUXADC_RDY_BIT,
> +	reg = cinfo->regs[PMIC_AUXADC_ADC0] + (chan->address << 1);
> +	rdy_mask = PMIC_AUXADC_RDY_BIT;
> +
> +	/*
> +	 * Even though for both PWRAP and SPMI cases the ADC HW signals that
> +	 * the data is ready by setting AUXADC_RDY_BIT, for SPMI the register
> +	 * read is only 8 bits long: for this case, the check has to be done
> +	 * on the ADC(x)_H register (high bits) and the rdy_mask needs to be
> +	 * shifted to the right by the same 8 bits.
> +	 */
> +	if (MTK_AUXADC_HAS_FLAG(cinfo, IS_SPMI)) {

This is getting close to the point where the complexity for the IS_SPMI case
is compled enough you'd be better off just splitting the code.  I'd try that
and see if it ends up neater than this.

> +		rdy_mask >>= 8;
> +		reg += 1;
> +	}
> +
> +	ret = regmap_read_poll_timeout(regmap, reg, val, val & rdy_mask,
>  				       AUXADC_POLL_DELAY_US, AUXADC_TIMEOUT_US);
> -	if (ret)
> -		return ret;
> +	if (ret) {
> +		dev_dbg(adc_dev->dev, "ADC read timeout for chan %lu\n", chan->address);
> +		goto end;
> +	}
> +
> +	if (MTK_AUXADC_HAS_FLAG(cinfo, IS_SPMI)) {
> +		/* If the previous read succeeded, this can't fail */

As per discussion with Andy, I don't think we can ever assume that.

> +		regmap_read(regmap, reg - 1, &lval);
> +		val = (val << 8) | lval;
> +	}
>  
> -	/* Stop sampling */

If you have code that ends up with an internal goto for a specific
block, that often suggests you should be factoring some code out to simplify
the flow.

I would take everything between the activiate ADC GPIO and deactivate out
as another function.  That will still need a goto to get to the stop
sampling but then we won't have the dance below where we do some
stuff from the main code flow on error and then exit (with more after
that not run).

> +end:
> +	/* Stop sampling unconditionally... */
>  	regmap_write(regmap, cinfo->regs[desc->req_idx], 0);
>  
> +	/* ...and deactivate the ADC GPIO if previously done */
> +	if (desc->ext_sel_idx >= 0) {
> +		ext_sel = FIELD_PREP(MT6363_EXT_PURES_MASK, MT6363_PULLUP_RES_OPEN);
> +
> +		regmap_update_bits(regmap, cinfo->regs[desc->ext_sel_idx],
> +				   MT6363_EXT_PURES_MASK, ext_sel);
> +	}
> +
> +	/* Check if we reached this point because of an error or regular flow */
> +	if (ret)
> +		return ret;
> +
> +	/* Everything went fine, give back the ADC reading */
>  	*out = val & GENMASK(chan->scan_type.realbits - 1, 0);
>  	return 0;
>  }
> @@ -505,7 +674,7 @@ static int mt6359_auxadc_read_raw(struct iio_dev *indio_dev,
>  	int ret;
>  
>  	if (mask == IIO_CHAN_INFO_SCALE) {
> -		*val = desc->r_ratio.numerator * AUXADC_VOLT_FULL;
> +		*val = desc->r_ratio.numerator * (u32)cinfo->vref_mv;

As above.  If vref_mv was already a (u32) no need to cast here.


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v1 4/5] iio: adc: mt6359: Add support for MediaTek MT6363 PMIC AUXADC
  2025-06-28 16:01   ` Jonathan Cameron
@ 2025-07-03 13:05     ` AngeloGioacchino Del Regno
  0 siblings, 0 replies; 14+ messages in thread
From: AngeloGioacchino Del Regno @ 2025-07-03 13:05 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: dlechner, nuno.sa, andy, robh, krzk+dt, conor+dt, matthias.bgg,
	linux-iio, devicetree, linux-kernel, linux-arm-kernel,
	linux-mediatek, kernel

Il 28/06/25 18:01, Jonathan Cameron ha scritto:
> On Mon, 23 Jun 2025 14:00:27 +0200
> AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote:
> 
>> MediaTek MT6363 is a PMIC found on MT8196/MT6991 board designs
>> and communicates with the SoC over SPMI.
>>
>> This PMIC integrates an Auxiliary ADC (AUXADC) which has a grand
>> total of 54 ADC channels: 49 PMIC-internal channels, 2 external
>> NTC thermistor channels and 2 generic ADC channels (mapped to 7
>> PMIC ADC external inputs).
>>
>> To use a generic ADC channel it is necessary to enable one of
>> the PMIC ADC inputs at a time and only then start the reading,
>> so in this case it is possible to read only one external input
>> for each generic ADC channel.
>>
>> Due to the lack of documentation, this implementation supports
>> using only one generic ADC channel, hence supports reading only
>> one external input at a time.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Hi,
> 
> A few comments that may or may not overlap with Andy's review.
> 
> thanks,
> 
> Jonathan
> 
>> ---
>>   drivers/iio/adc/mt6359-auxadc.c | 238 +++++++++++++++++++++++++++++---
>>   1 file changed, 217 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/iio/adc/mt6359-auxadc.c b/drivers/iio/adc/mt6359-auxadc.c
>> index ae7b65f5f551..f49b0b6e78da 100644
>> --- a/drivers/iio/adc/mt6359-auxadc.c
>> +++ b/drivers/iio/adc/mt6359-auxadc.c
> 
>> +enum mtk_pmic_auxadc_flags {
>> +	MTK_PMIC_AUXADC_IS_SPMI = BIT(0),
>> +	MTK_PMIC_AUXADC_NO_RESET = BIT(1),
>> +};
> 
> With just two bits I think flags obscures what is going on over
> a pair of separate booleans.
> 

Okay, I'll change this to two booleans then!

>>   };
>> @@ -123,7 +155,9 @@ struct mtk_pmic_auxadc_chan {
>>    * @desc:           PMIC AUXADC channel data
>>    * @regs:           List of PMIC specific registers
>>    * @sec_unlock_key: Security unlock key for HK_TOP writes
>> + * @vref_mv:        AUXADC Reference Voltage (VREF) in millivolts
>>    * @imp_adc_num:    ADC channel for battery impedance readings
>> + * @flags:          Feature flags
>>    * @read_imp:       Callback to read impedance channels
>>    */
>>   struct mtk_pmic_auxadc_info {
>> @@ -133,22 +167,33 @@ struct mtk_pmic_auxadc_info {
>>   	const struct mtk_pmic_auxadc_chan *desc;
>>   	const u16 *regs;
>>   	u16 sec_unlock_key;
>> +	u16 vref_mv;
> I'd not worry about the space saving here and instead make this a u32 so that
> can avoid the casting when using this.
> 

Okay, will do.

>>   	u8 imp_adc_num;
>> +	u8 flags;
> 
> As above. Pair of bool preferred.
> 
>>   	int (*read_imp)(struct mt6359_auxadc *adc_dev,
>>   			const struct iio_chan_spec *chan, int *vbat, int *ibat);
>>   };
> 
>>   static void mt6358_stop_imp_conv(struct mt6359_auxadc *adc_dev)
>>   {
>>   	const struct mtk_pmic_auxadc_info *cinfo = adc_dev->chip_info;
>> @@ -379,13 +488,13 @@ static int mt6359_read_imp(struct mt6359_auxadc *adc_dev,
>>   	int ret;
>>   
>>   	/* Start conversion */
>> -	regmap_write(regmap, cinfo->regs[PMIC_AUXADC_IMP0], MT6359_IMP0_CONV_EN);
>> +	regmap_write(regmap, cinfo->regs[desc->req_idx], desc->req_mask);
> 
> Given desc->req_idx is not introduced in this patch, why is this needed now
> but not previously?  Maybe this change belongs in a separate patch with
> a description to explain that.
> 

Oh wow, many many many... many thanks for catching this!!!!

This change was not supposed to be in this commit, as I had implemented both
the IBAT and VBAT "IMP" for the 6363 but then left them out because I was in
doubt whether to add them here or if they are read from the fuel gauge chip
transparently and in firmware; I wouldn't even be able to test that on the
MT8196 Chromebook that I'm bringing up, because the battery is managed by EC
instead, unlike smartphones.

I have to remove that from mt6359_read_imp, the addition was unintentional;
only mt6359_auxadc_read_adc() should use it (and already did before this commit).

>>   	ret = regmap_read_poll_timeout(regmap, cinfo->regs[desc->rdy_idx],
>>   				       val, val & desc->rdy_mask,
>>   				       IMP_POLL_DELAY_US, AUXADC_TIMEOUT_US);
>>   
>>   	/* Stop conversion regardless of the result */
>> -	regmap_write(regmap, cinfo->regs[PMIC_AUXADC_IMP0], 0);
>> +	regmap_write(regmap, cinfo->regs[desc->req_idx], 0);
>>   	if (ret)
>>   		return ret;
>>   
>> @@ -416,6 +525,7 @@ static const struct mtk_pmic_auxadc_info mt6357_chip_info = {
>>   	.regs = mt6357_auxadc_regs,
>>   	.imp_adc_num = MT6357_IMP_ADC_NUM,
>>   	.read_imp = mt6358_read_imp,
>> +	.vref_mv = 1800,
>>   };
>>   
>>   static const struct mtk_pmic_auxadc_info mt6358_chip_info = {
>> @@ -426,6 +536,7 @@ static const struct mtk_pmic_auxadc_info mt6358_chip_info = {
>>   	.regs = mt6358_auxadc_regs,
>>   	.imp_adc_num = MT6358_IMP_ADC_NUM,
>>   	.read_imp = mt6358_read_imp,
>> +	.vref_mv = 1800,
>>   };
>>   
>>   static const struct mtk_pmic_auxadc_info mt6359_chip_info = {
>> @@ -436,6 +547,17 @@ static const struct mtk_pmic_auxadc_info mt6359_chip_info = {
>>   	.regs = mt6359_auxadc_regs,
>>   	.sec_unlock_key = 0x6359,
>>   	.read_imp = mt6359_read_imp,
>> +	.vref_mv = 1800,
> 
> Add vref_mv and code using it in a precursor patch.  Not a problem that all
> vref_mv will be 1800 at that point.  That way we can quickly see that it
> has no affect on existing parts, and simplify what is present in this patch.
> 

Right, I agree. I'll move that addition to a separate patch.

>> +};
>> +
>> +static const struct mtk_pmic_auxadc_info mt6363_chip_info = {
>> +	.model_name = "MT6363",
>> +	.channels = mt6363_auxadc_channels,
>> +	.num_channels = ARRAY_SIZE(mt6363_auxadc_channels),
>> +	.desc = mt6363_auxadc_ch_desc,
>> +	.regs = mt6363_auxadc_regs,
>> +	.flags = MTK_PMIC_AUXADC_IS_SPMI | MTK_PMIC_AUXADC_NO_RESET,
>> +	.vref_mv = 1840,
>>   };
>>   
>>   static void mt6359_auxadc_reset(struct mt6359_auxadc *adc_dev)
>> @@ -464,27 +586,74 @@ static int mt6359_auxadc_read_adc(struct mt6359_auxadc *adc_dev,
>>   	const struct mtk_pmic_auxadc_info *cinfo = adc_dev->chip_info;
>>   	const struct mtk_pmic_auxadc_chan *desc = &cinfo->desc[chan->scan_index];
>>   	struct regmap *regmap = adc_dev->regmap;
>> -	u32 val;
>> +	u32 reg, rdy_mask, val, lval;
>> +	u8 ext_sel;
>>   	int ret;
>>   
>> +	if (desc->ext_sel_idx >= 0) {
>> +		ext_sel = FIELD_PREP(MT6363_EXT_PURES_MASK, desc->ext_sel_pu);
>> +		ext_sel |= FIELD_PREP(MT6363_EXT_CHAN_MASK, desc->ext_sel_ch);
>> +
>> +		ret = regmap_update_bits(regmap, cinfo->regs[desc->ext_sel_idx],
>> +					 MT6363_EXT_PURES_MASK | MT6363_EXT_CHAN_MASK,
>> +					 ext_sel);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>>   	/* Request to start sampling for ADC channel */
>>   	ret = regmap_write(regmap, cinfo->regs[desc->req_idx], desc->req_mask);
>>   	if (ret)
>> -		return ret;
>> +		goto end;
>>   
>>   	/* Wait until all samples are averaged */
>>   	fsleep(desc->num_samples * AUXADC_AVG_TIME_US);
>>   
>> -	ret = regmap_read_poll_timeout(regmap,
>> -				       cinfo->regs[PMIC_AUXADC_ADC0] + (chan->address << 1),
>> -				       val, val & PMIC_AUXADC_RDY_BIT,
>> +	reg = cinfo->regs[PMIC_AUXADC_ADC0] + (chan->address << 1);
>> +	rdy_mask = PMIC_AUXADC_RDY_BIT;
>> +
>> +	/*
>> +	 * Even though for both PWRAP and SPMI cases the ADC HW signals that
>> +	 * the data is ready by setting AUXADC_RDY_BIT, for SPMI the register
>> +	 * read is only 8 bits long: for this case, the check has to be done
>> +	 * on the ADC(x)_H register (high bits) and the rdy_mask needs to be
>> +	 * shifted to the right by the same 8 bits.
>> +	 */
>> +	if (MTK_AUXADC_HAS_FLAG(cinfo, IS_SPMI)) {
> 
> This is getting close to the point where the complexity for the IS_SPMI case
> is compled enough you'd be better off just splitting the code.  I'd try that
> and see if it ends up neater than this.
> 

That was the first thing I did... but it's not so many changes... I'd be ending
up almost completely duplicating this driver for no reason.

I'm mostly sure that there won't be any more spmi-specific differences in the
AuxADC, but if one day turns out I'm wrong, I guess we can always split the
driver in two and move the SPMI platforms away... but again, I'm mostly sure
that won't happen.

>> +		rdy_mask >>= 8;
>> +		reg += 1;
>> +	}
>> +
>> +	ret = regmap_read_poll_timeout(regmap, reg, val, val & rdy_mask,
>>   				       AUXADC_POLL_DELAY_US, AUXADC_TIMEOUT_US);
>> -	if (ret)
>> -		return ret;
>> +	if (ret) {
>> +		dev_dbg(adc_dev->dev, "ADC read timeout for chan %lu\n", chan->address);
>> +		goto end;
>> +	}
>> +
>> +	if (MTK_AUXADC_HAS_FLAG(cinfo, IS_SPMI)) {
>> +		/* If the previous read succeeded, this can't fail */
> 
> As per discussion with Andy, I don't think we can ever assume that.
> 

I'll add all the checks around :-)

>> +		regmap_read(regmap, reg - 1, &lval);
>> +		val = (val << 8) | lval;
>> +	}
>>   
>> -	/* Stop sampling */
> 
> If you have code that ends up with an internal goto for a specific
> block, that often suggests you should be factoring some code out to simplify
> the flow.
> 
> I would take everything between the activiate ADC GPIO and deactivate out
> as another function.  That will still need a goto to get to the stop
> sampling but then we won't have the dance below where we do some
> stuff from the main code flow on error and then exit (with more after
> that not run).
> 

Actually, I have removed the goto as well - I moved everything but the write
to stop the sampling to a new function; I'm calling it, then stopping the
sampling unconditionally, as was already done before.

To let you understand, this is the description:
/**
  * mt6359_auxadc_sample_value() - Start ADC channel sampling and read value
  * @adc_dev: Main driver structure
  * @out:     Preallocated variable to store the value read from HW
  *
  * This function starts the sampling for an ADC channel, waits until all
  * of the samples are averaged and then reads the value from the HW.
  *
  * Note that the caller must stop the ADC sampling on its own, as this
  * function *never* stops it.
  *
  * Return:
  * Negative number for error;
  * Upon success returns zero and writes the read value to *out.
  */

...you can imagine the rest, or anyway I'm sending a v2 shortly :-)

>> +end:
>> +	/* Stop sampling unconditionally... */
>>   	regmap_write(regmap, cinfo->regs[desc->req_idx], 0);
>>   
>> +	/* ...and deactivate the ADC GPIO if previously done */
>> +	if (desc->ext_sel_idx >= 0) {
>> +		ext_sel = FIELD_PREP(MT6363_EXT_PURES_MASK, MT6363_PULLUP_RES_OPEN);
>> +
>> +		regmap_update_bits(regmap, cinfo->regs[desc->ext_sel_idx],
>> +				   MT6363_EXT_PURES_MASK, ext_sel);
>> +	}
>> +
>> +	/* Check if we reached this point because of an error or regular flow */
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Everything went fine, give back the ADC reading */
>>   	*out = val & GENMASK(chan->scan_type.realbits - 1, 0);
>>   	return 0;
>>   }
>> @@ -505,7 +674,7 @@ static int mt6359_auxadc_read_raw(struct iio_dev *indio_dev,
>>   	int ret;
>>   
>>   	if (mask == IIO_CHAN_INFO_SCALE) {
>> -		*val = desc->r_ratio.numerator * AUXADC_VOLT_FULL;
>> +		*val = desc->r_ratio.numerator * (u32)cinfo->vref_mv;
> 
> As above.  If vref_mv was already a (u32) no need to cast here.

Changed!

Cheers,
Angelo




^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2025-07-03 13:05 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-23 12:00 [PATCH v1 0/5] iio: Add support for MT6363/6373 Auxiliary ADC AngeloGioacchino Del Regno
2025-06-23 12:00 ` [PATCH v1 1/5] dt-bindings: iio: adc: mt6359: Add bindings for MT6363 PMIC AuxADC AngeloGioacchino Del Regno
2025-06-27 20:02   ` Rob Herring (Arm)
2025-06-23 12:00 ` [PATCH v1 2/5] dt-bindings: iio: adc: mt6359: Add bindings for MT6373 " AngeloGioacchino Del Regno
2025-06-27 20:04   ` Rob Herring
2025-06-23 12:00 ` [PATCH v1 3/5] iio: adc: mt6359: Add ready register index and mask to channel data AngeloGioacchino Del Regno
2025-06-23 12:00 ` [PATCH v1 4/5] iio: adc: mt6359: Add support for MediaTek MT6363 PMIC AUXADC AngeloGioacchino Del Regno
2025-06-23 14:30   ` Andy Shevchenko
2025-06-25 13:29     ` AngeloGioacchino Del Regno
2025-06-25 14:27       ` Andy Shevchenko
2025-06-28 16:01   ` Jonathan Cameron
2025-07-03 13:05     ` AngeloGioacchino Del Regno
2025-06-23 12:00 ` [PATCH v1 5/5] iio: adc: mt6359: Add support for MediaTek MT6373 " AngeloGioacchino Del Regno
2025-06-24 15:08 ` [PATCH v1 0/5] iio: Add support for MT6363/6373 Auxiliary ADC Nícolas F. R. A. Prado

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