[PATCH 0/3] iio: adc: add mt6397 PMIC AUXADC support

[PATCH 0/3] iio: adc: add mt6397 PMIC AUXADC support

[Ryan Brue]

The MediaTek mt6397 PMIC has a 10-bit AUXADC that nothing in-tree can
reach. On boards built around it that ADC is the only path to the battery:
the SoC's AUXADC is wired to board thermistors, and the charger ICs these
boards use have no ADC at all, so without it there is no pack voltage and
no state of charge.

Patch 1 adds the compatible to the existing MediaTek PMIC AUXADC schema and
the header naming the channels, patch 2 the driver, patch 3 the MFD cell
that instantiates it. The cell comes last so that no commit in between
registers a child no driver can claim.

The mt6397 is not given mediatek,mt6359-auxadc as a fallback because the
two are not register compatible; patch 1 has the detail.

Only the two channels a board needs for its pack are described, so patch 1
ships a header naming them: a channel ID is an index into the driver's array
and not the PMIC's channel number, as in mt6323-auxadc. The register
sequences are the ones the vendor kernel's pmic_auxadc.c programs.

One note on patch 3: it adds a cell to mt6397_devs[], which Luca Leonardo
Scorcia's MT6392 series converts to the MFD_CELL_* macros in its patch
4/9 [1]. Whichever of the two lands second needs a respin. Mine would
become a single MFD_CELL_OF(), and I can do a respin if that patch arrives
first.
Cc'd Luca.

[1] https://lore.kernel.org/all/20260902193351.185771-5-l.scorcia@gmail.com/

Tested on an Amazon Fire HD 10 (2017), an mt8173 with an mt6397. Both
channels are exercised: the battery channel by a fuel gauge, which reads 817
counts, 3830 mV with the pack at rest, and the thermistor channel by an
ntc-thermistor consuming it.

I have two things in the driver different from the vendor (Amazon
Fire OS based on a 3.18 kernel). Verified with over 150 interleaved reads
per setting:

 - Leaving the ISENSE input enabled between reads, as the vendor does,
   makes no difference to the value, so the driver switches it off again
   and leaves both ADC input enables clear when idle.

 - The chip's sample accumulator makes no difference at 4, 8, 16 or 31
   samples and costs no measurable time, so it is left at one. Software
   averaging helps, but only by about a quarter: 0.92 LSB standard
   deviation for one conversion against 0.67 for sixteen, where independent
   samples would have given 0.23. Conversions in a burst are correlated.

The reading was checked against the charger, which regulates the pack to
a programmed voltage in constant-voltage mode. Over twelve such voltages
from 3904 to 4080 mV, and charge currents from 111 to 778 mA, the driver's
reading is a mean of 0.9 mV from the regulated value, well inside
the charger's regulation tolerance. That also settles reading the chip's
trimmed result register rather than the raw one - the raw register is
20.7 mV low at every one of the twelve points.

Signed-off-by: Ryan Brue <ryanbrue.dev@gmail.com>
---
Ryan Brue (3):
      dt-bindings: iio: adc: mediatek,mt6359-auxadc: add mt6397 PMIC AUXADC
      iio: adc: mt6397-auxadc: add mt6397 PMIC AUXADC driver
      mfd: mt6397-core: Add mt6397 AUXADC support

 .../bindings/iio/adc/mediatek,mt6359-auxadc.yaml   |   1 +
 MAINTAINERS                                        |   7 +
 drivers/iio/adc/Kconfig                            |  11 +
 drivers/iio/adc/Makefile                           |   1 +
 drivers/iio/adc/mt6397-auxadc.c                    | 340 +++++++++++++++++++++
 drivers/mfd/mt6397-core.c                          |   3 +
 .../dt-bindings/iio/adc/mediatek,mt6397-auxadc.h   |   9 +
 7 files changed, 372 insertions(+)
---
base-commit: fd73f4a6659897191fa0d40695fe370925dd3780
change-id: 20260914-rbrue-suez-upstreaming-mt6397-auxadc-5e1228025c50

Best regards,
--  
Ryan Brue <ryanbrue.dev@gmail.com>

[PATCH 1/3] dt-bindings: iio: adc: mediatek,mt6359-auxadc: add mt6397 PMIC AUXADC

[Ryan Brue]

The MediaTek mt6397 PMIC includes a 10-bit AUXADC used for battery voltage
and battery temperature measurements. It is not register compatible with
mt6359: a conversion is requested by writing a channel number to a select
field and pulsing an edge triggered start bit rather than by setting the
channel's bit in a request register, and the results live elsewhere.
It therefore gets its own compatible rather than a fallback.

Add the binding and the header naming the channels a consumer puts in
io-channels. The battery channel is named for the input it measures,
ISENSE, which is what mt6323-auxadc calls the same input.

Assisted-by: LLM
Signed-off-by: Ryan Brue <ryanbrue.dev@gmail.com>
---
 .../devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml      | 1 +
 include/dt-bindings/iio/adc/mediatek,mt6397-auxadc.h             | 9 +++++++++
 2 files changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml b/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml
index c2f7387e4bfc..2847bd5d5bc4 100644
--- a/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml
@@ -26,6 +26,7 @@ properties:
           - mediatek,mt6359-auxadc
           - mediatek,mt6363-auxadc
           - mediatek,mt6373-auxadc
+          - mediatek,mt6397-auxadc
       - items:
           - enum:
               - mediatek,mt6365-auxadc
diff --git a/include/dt-bindings/iio/adc/mediatek,mt6397-auxadc.h b/include/dt-bindings/iio/adc/mediatek,mt6397-auxadc.h
new file mode 100644
index 000000000000..b696c1cb5ebe
--- /dev/null
+++ b/include/dt-bindings/iio/adc/mediatek,mt6397-auxadc.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+
+#ifndef _DT_BINDINGS_MEDIATEK_MT6397_AUXADC_H
+#define _DT_BINDINGS_MEDIATEK_MT6397_AUXADC_H
+
+#define MT6397_AUXADC_ISENSE		0
+#define MT6397_AUXADC_BAT_TEMP		1
+
+#endif

-- 
2.55.0

[PATCH 2/3] iio: adc: mt6397-auxadc: add mt6397 PMIC AUXADC driver

[Ryan Brue]

The mt6397 AUXADC is a 10-bit ADC behind the SoC's PMIC wrapper. On boards
built around this PMIC it is the only way to read the battery: the SoC's
AUXADC is wired to board thermistors and the charger ICs these boards use
have no ADC of their own.

Add a driver exposing the battery voltage and battery temperature
channels. Only those two are described, so a channel ID in the device tree
is an index into the driver's channel array rather than the PMIC's channel
number, as mt6323-auxadc does. The ready bit lives in a channel's raw
result register, but the value comes from the chip's trimmed copy of it,
which is what the vendor driver reads for a measurement.

Both channels need more than that, as the vendor programs them. The
battery voltage is measured through ISENSE, because a board with a
switching charger in the power path leaves BATSNS on the charger's system
rail instead of on the pack. The thermistor only reads correctly with the
PMIC's battery-detect bias and input buffer enabled, which take 20 ms to
settle. Both are switched back off afterwards.

Reads average sixteen conversions in software; the chip's sample
accumulator makes no measurable difference at any setting, so it is left
at one sample per conversion.

Assisted-by: LLM
Signed-off-by: Ryan Brue <ryanbrue.dev@gmail.com>
---
 MAINTAINERS                     |   7 +
 drivers/iio/adc/Kconfig         |  11 ++
 drivers/iio/adc/Makefile        |   1 +
 drivers/iio/adc/mt6397-auxadc.c | 340 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 359 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index c2414447892c..9b254f98b5d7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16713,6 +16713,13 @@ S:	Maintained
 F:	drivers/iio/adc/mt6323-auxadc.c
 F:	include/dt-bindings/iio/adc/mediatek,mt6323-auxadc.h
 
+MEDIATEK MT6397 PMIC AUXADC DRIVER
+M:	Ryan Brue <ryanbrue.dev@gmail.com>
+L:	linux-iio@vger.kernel.org
+S:	Maintained
+F:	drivers/iio/adc/mt6397-auxadc.c
+F:	include/dt-bindings/iio/adc/mediatek,mt6397-auxadc.h
+
 MEDIATEK MT6735 CLOCK & RESET DRIVERS
 M:	Yassine Oudjana <y.oudjana@protonmail.com>
 L:	linux-clk@vger.kernel.org
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 415e519ad4eb..9a1d36ed53cb 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1233,6 +1233,17 @@ config MEDIATEK_MT6370_ADC
 	  This driver can also be built as a module. If so, the module
 	  will be called "mt6370-adc".
 
+config MEDIATEK_MT6397_AUXADC
+	tristate "MediaTek MT6397 PMIC AUXADC driver"
+	depends on MFD_MT6397
+	help
+	  Say yes here to enable support for MediaTek MT6397 PMIC Auxiliary ADC.
+	  This driver provides the battery voltage and battery temperature
+	  channels used by the fuel gauge on boards built around this PMIC.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called mt6397-auxadc.
+
 config MEDIATEK_MT6577_AUXADC
 	tristate "MediaTek AUXADC driver"
 	depends on ARCH_MEDIATEK || COMPILE_TEST
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index dcec0abb03b7..13cb4206bdb7 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -105,6 +105,7 @@ obj-$(CONFIG_MEDIATEK_MT6323_AUXADC) += mt6323-auxadc.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_MT6397_AUXADC) += mt6397-auxadc.o
 obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o
 obj-$(CONFIG_MEN_Z188_ADC) += men_z188_adc.o
 obj-$(CONFIG_MESON_SARADC) += meson_saradc.o
diff --git a/drivers/iio/adc/mt6397-auxadc.c b/drivers/iio/adc/mt6397-auxadc.c
new file mode 100644
index 000000000000..dbfa82341d6c
--- /dev/null
+++ b/drivers/iio/adc/mt6397-auxadc.c
@@ -0,0 +1,340 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * MediaTek MT6397 PMIC AUXADC IIO driver
+ *
+ * Copyright (c) 2026 Ryan Brue <ryanbrue.dev@gmail.com>
+ *
+ * Based on drivers/iio/adc/mt6323-auxadc.c
+ */
+
+#include <linux/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/cleanup.h>
+#include <linux/delay.h>
+#include <linux/iio/iio.h>
+#include <linux/math.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/stringify.h>
+#include <linux/time.h>
+#include <linux/types.h>
+
+#include <linux/mfd/mt6397/core.h>
+
+#include <dt-bindings/iio/adc/mediatek,mt6397-auxadc.h>
+
+/* The ready bit is in the raw result register, the value in the trimmed one. */
+#define MT6397_AUXADC_ADC(n)		(0x0514 + 2 * (n))
+#define MT6397_AUXADC_ADC_RDY		BIT(15)
+#define MT6397_AUXADC_ADC_VAL		GENMASK(9, 0)
+#define MT6397_AUXADC_ADC_TRIM(n)	(0x052a + 2 * (n))	/* ADC11 + n */
+
+/* The thermistor needs the battery-detect bias and the input buffer on. */
+#define MT6397_AUXADC_CON0		0x0542
+#define MT6397_AUXADC_CON0_BUF_PWD_B	BIT(1)
+#define MT6397_AUXADC_CON0_BUF_PWD_ON	BIT(3)
+#define MT6397_CHR_CON7			0x000e
+#define MT6397_CHR_CON7_BATON_TDET_EN	BIT(2)
+#define MT6397_AUXADC_BATTEMP_SETTLE_US	(20 * USEC_PER_MSEC)
+
+#define MT6397_AUXADC_CON1		0x0544
+#define MT6397_AUXADC_CON1_CHSEL	GENMASK(10, 7)
+#define MT6397_AUXADC_CON1_START	BIT(0)
+
+/*
+ * CHR_CON16 and the two SOURCE_CH0 selects route either BATSNS or ISENSE onto
+ * the battery channel. Only ISENSE is used: with a switching charger in the
+ * power path BATSNS sits on the system rail rather than on the pack.
+ */
+#define MT6397_CHR_CON16		0x0020
+#define MT6397_CHR_CON16_ADCIN_VSEN_EN	BIT(11)
+#define MT6397_CHR_CON16_ADCIN_VBAT_EN	BIT(10)
+#define MT6397_CHR_CON16_ADCIN_SEL	(MT6397_CHR_CON16_ADCIN_VSEN_EN | \
+					 MT6397_CHR_CON16_ADCIN_VBAT_EN)
+#define MT6397_AUXADC_CON14		0x055e
+#define MT6397_AUXADC_CON14_CH0_NORM_SEL BIT(2)
+#define MT6397_AUXADC_CON14_CH0_LBAT_SEL BIT(0)
+#define MT6397_AUXADC_ISENSE_SETTLE_US	USEC_PER_MSEC
+
+/* Samples accumulated per conversion; the chip's accumulator is left off. */
+#define MT6397_AUXADC_CON0_SPL_NUM	GENMASK(11, 7)
+
+/* Conversions averaged in software per read. */
+#define MT6397_AUXADC_SAMPLES		16
+
+/* Hardware limitation: the result is not ready for this long after START. */
+#define MT6397_AUXADC_START_US		30
+
+/* The battery channel is divided down by 4; the thermistor is not divided. */
+#define MT6397_AUXADC_ISENSE_DIVIDER	4
+
+/* The PMIC's own channel numbers, as the CON1 select field takes them. */
+#define MT6397_AUXADC_HWCHAN_BATSNS	0
+#define MT6397_AUXADC_HWCHAN_BAT_TEMP	3
+
+struct mt6397_auxadc {
+	struct regmap *regmap;
+	/* AUXADC doesn't support reading multiple channels simultaneously. */
+	struct mutex lock;
+};
+
+#define MTK_PMIC_IIO_CHAN(_name, _chan, _hwchan)                \
+{                                                               \
+	.type = IIO_VOLTAGE,                                    \
+	.indexed = 1,                                           \
+	.channel = _chan,                                       \
+	.address = _hwchan,                                     \
+	.datasheet_name = __stringify(_name),                   \
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |          \
+			      BIT(IIO_CHAN_INFO_SCALE),         \
+}
+
+/*
+ * A device tree channel ID indexes this array; .address holds the PMIC channel
+ * number. The thermistor reports as a voltage because that is all the PMIC
+ * measures: the divider across an NTC whose curve belongs to the board.
+ */
+static const struct iio_chan_spec mt6397_auxadc_channels[] = {
+	MTK_PMIC_IIO_CHAN(isense,   MT6397_AUXADC_ISENSE,
+			  MT6397_AUXADC_HWCHAN_BATSNS),
+	MTK_PMIC_IIO_CHAN(bat_temp, MT6397_AUXADC_BAT_TEMP,
+			  MT6397_AUXADC_HWCHAN_BAT_TEMP),
+};
+
+static int mt6397_auxadc_read_once(struct mt6397_auxadc *adc,
+				   const struct iio_chan_spec *chan, int *val)
+{
+	struct regmap *map = adc->regmap;
+	unsigned int reg;
+	int ret;
+
+	ret = regmap_update_bits(map, MT6397_AUXADC_CON1,
+				 MT6397_AUXADC_CON1_CHSEL,
+				 FIELD_PREP(MT6397_AUXADC_CON1_CHSEL,
+					    chan->address));
+	if (ret)
+		return ret;
+
+	/* START is edge triggered: it has to be lowered before being raised. */
+	ret = regmap_clear_bits(map, MT6397_AUXADC_CON1,
+				MT6397_AUXADC_CON1_START);
+	if (ret)
+		return ret;
+
+	ret = regmap_set_bits(map, MT6397_AUXADC_CON1,
+			      MT6397_AUXADC_CON1_START);
+	if (ret)
+		return ret;
+
+	fsleep(MT6397_AUXADC_START_US);
+
+	ret = regmap_read_poll_timeout(map, MT6397_AUXADC_ADC(chan->address),
+				       reg, reg & MT6397_AUXADC_ADC_RDY,
+				       100, 100 * USEC_PER_MSEC);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(map, MT6397_AUXADC_ADC_TRIM(chan->address), &reg);
+	if (ret)
+		return ret;
+
+	*val = FIELD_GET(MT6397_AUXADC_ADC_VAL, reg);
+
+	return 0;
+}
+
+static int mt6397_auxadc_battemp_bias(struct mt6397_auxadc *adc, bool on)
+{
+	struct regmap *map = adc->regmap;
+	int ret;
+
+	if (on) {
+		ret = regmap_set_bits(map, MT6397_AUXADC_CON0,
+				      MT6397_AUXADC_CON0_BUF_PWD_ON);
+		if (ret)
+			return ret;
+		ret = regmap_set_bits(map, MT6397_AUXADC_CON0,
+				      MT6397_AUXADC_CON0_BUF_PWD_B);
+		if (ret)
+			return ret;
+		return regmap_set_bits(map, MT6397_CHR_CON7,
+				       MT6397_CHR_CON7_BATON_TDET_EN);
+	}
+
+	ret = regmap_clear_bits(map, MT6397_CHR_CON7,
+				MT6397_CHR_CON7_BATON_TDET_EN);
+	ret = ret ?: regmap_clear_bits(map, MT6397_AUXADC_CON0,
+				       MT6397_AUXADC_CON0_BUF_PWD_B);
+	return ret ?: regmap_clear_bits(map, MT6397_AUXADC_CON0,
+					MT6397_AUXADC_CON0_BUF_PWD_ON);
+}
+
+static int mt6397_auxadc_isense_enable(struct mt6397_auxadc *adc)
+{
+	struct regmap *map = adc->regmap;
+	int ret;
+
+	ret = regmap_update_bits(map, MT6397_CHR_CON16,
+				 MT6397_CHR_CON16_ADCIN_SEL,
+				 MT6397_CHR_CON16_ADCIN_VSEN_EN);
+	if (ret)
+		return ret;
+
+	return regmap_set_bits(map, MT6397_AUXADC_CON14,
+			       MT6397_AUXADC_CON14_CH0_NORM_SEL |
+			       MT6397_AUXADC_CON14_CH0_LBAT_SEL);
+}
+
+static int mt6397_auxadc_isense_disable(struct mt6397_auxadc *adc)
+{
+	struct regmap *map = adc->regmap;
+	int ret;
+
+	ret = regmap_clear_bits(map, MT6397_AUXADC_CON14,
+				MT6397_AUXADC_CON14_CH0_NORM_SEL |
+				MT6397_AUXADC_CON14_CH0_LBAT_SEL);
+
+	return ret ?: regmap_clear_bits(map, MT6397_CHR_CON16,
+					MT6397_CHR_CON16_ADCIN_SEL);
+}
+
+static int mt6397_auxadc_read_channel(struct mt6397_auxadc *adc,
+				      const struct iio_chan_spec *chan,
+				      int *val)
+{
+	unsigned int i, sum = 0;
+	int ret, sample;
+
+	/* Held across the whole burst: the channel select is shared state. */
+	guard(mutex)(&adc->lock);
+
+	if (chan->channel == MT6397_AUXADC_ISENSE) {
+		ret = mt6397_auxadc_isense_enable(adc);
+		if (ret)
+			return ret;
+		fsleep(MT6397_AUXADC_ISENSE_SETTLE_US);
+	} else {
+		ret = mt6397_auxadc_battemp_bias(adc, true);
+		if (ret)
+			return ret;
+		fsleep(MT6397_AUXADC_BATTEMP_SETTLE_US);
+	}
+
+	for (i = 0; i < MT6397_AUXADC_SAMPLES; i++) {
+		ret = mt6397_auxadc_read_once(adc, chan, &sample);
+		if (ret)
+			break;
+
+		sum += sample;
+	}
+
+	/* Lower START so the converter is not left armed between reads. */
+	regmap_clear_bits(adc->regmap, MT6397_AUXADC_CON1,
+			  MT6397_AUXADC_CON1_START);
+
+	if (chan->channel == MT6397_AUXADC_ISENSE)
+		mt6397_auxadc_isense_disable(adc);
+	else
+		mt6397_auxadc_battemp_bias(adc, false);
+
+	if (ret)
+		return ret;
+
+	*val = DIV_ROUND_CLOSEST(sum, MT6397_AUXADC_SAMPLES);
+
+	return 0;
+}
+
+static int mt6397_auxadc_read_raw(struct iio_dev *indio_dev,
+				  const struct iio_chan_spec *chan,
+				  int *val, int *val2, long mask)
+{
+	struct mt6397_auxadc *adc = iio_priv(indio_dev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = mt6397_auxadc_read_channel(adc, chan, val);
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		/* 1200 mV full range with 10-bit resolution. */
+		*val = 1200;
+		if (chan->channel == MT6397_AUXADC_ISENSE)
+			*val *= MT6397_AUXADC_ISENSE_DIVIDER;
+		*val2 = 10;
+
+		return IIO_VAL_FRACTIONAL_LOG2;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int mt6397_auxadc_init(struct mt6397_auxadc *adc)
+{
+	return regmap_update_bits(adc->regmap, MT6397_AUXADC_CON0,
+				  MT6397_AUXADC_CON0_SPL_NUM,
+				  FIELD_PREP(MT6397_AUXADC_CON0_SPL_NUM, 1));
+}
+
+static const struct iio_info mt6397_auxadc_info = {
+	.read_raw = mt6397_auxadc_read_raw,
+};
+
+static int mt6397_auxadc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct mt6397_chip *chip = dev_get_drvdata(dev->parent);
+	struct mt6397_auxadc *adc;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*adc));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	adc = iio_priv(indio_dev);
+	adc->regmap = chip->regmap;
+
+	ret = devm_mutex_init(dev, &adc->lock);
+	if (ret)
+		return ret;
+
+	ret = mt6397_auxadc_init(adc);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to initialize auxadc\n");
+
+	indio_dev->name = "mt6397-auxadc";
+	indio_dev->info = &mt6397_auxadc_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = mt6397_auxadc_channels;
+	indio_dev->num_channels = ARRAY_SIZE(mt6397_auxadc_channels);
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static const struct of_device_id mt6397_auxadc_of_match[] = {
+	{ .compatible = "mediatek,mt6397-auxadc" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, mt6397_auxadc_of_match);
+
+static struct platform_driver mt6397_auxadc_driver = {
+	.driver = {
+		.name = "mt6397-auxadc",
+		.of_match_table = mt6397_auxadc_of_match,
+	},
+	.probe = mt6397_auxadc_probe,
+};
+module_platform_driver(mt6397_auxadc_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Ryan Brue <ryanbrue.dev@gmail.com>");
+MODULE_DESCRIPTION("MediaTek MT6397 PMIC AUXADC Driver");

-- 
2.55.0

Re: [PATCH 2/3] iio: adc: mt6397-auxadc: add mt6397 PMIC AUXADC driver

[Andy Shevchenko]

On Tue, Sep 15, 2026 at 11:15:27PM -0500, Ryan Brue wrote:
> The mt6397 AUXADC is a 10-bit ADC behind the SoC's PMIC wrapper. On boards
> built around this PMIC it is the only way to read the battery: the SoC's
> AUXADC is wired to board thermistors and the charger ICs these boards use
> have no ADC of their own.
> 
> Add a driver exposing the battery voltage and battery temperature
> channels. Only those two are described, so a channel ID in the device tree
> is an index into the driver's channel array rather than the PMIC's channel
> number, as mt6323-auxadc does. The ready bit lives in a channel's raw
> result register, but the value comes from the chip's trimmed copy of it,
> which is what the vendor driver reads for a measurement.
> 
> Both channels need more than that, as the vendor programs them. The
> battery voltage is measured through ISENSE, because a board with a
> switching charger in the power path leaves BATSNS on the charger's system
> rail instead of on the pack. The thermistor only reads correctly with the
> PMIC's battery-detect bias and input buffer enabled, which take 20 ms to
> settle. Both are switched back off afterwards.
> 
> Reads average sixteen conversions in software; the chip's sample
> accumulator makes no measurable difference at any setting, so it is left
> at one sample per conversion.

...

> +/*
> + * MediaTek MT6397 PMIC AUXADC IIO driver
> + *
> + * Copyright (c) 2026 Ryan Brue <ryanbrue.dev@gmail.com>
> + *
> + * Based on drivers/iio/adc/mt6323-auxadc.c

Why not add this device support into that driver?

> + */

...

> +#define MT6397_AUXADC_ISENSE_SETTLE_US	USEC_PER_MSEC

(1 * USEC_PER_MSEC)

...

> +static const struct iio_chan_spec mt6397_auxadc_channels[] = {
> +	MTK_PMIC_IIO_CHAN(isense,   MT6397_AUXADC_ISENSE,

One space too many.

> +			  MT6397_AUXADC_HWCHAN_BATSNS),
> +	MTK_PMIC_IIO_CHAN(bat_temp, MT6397_AUXADC_BAT_TEMP,
> +			  MT6397_AUXADC_HWCHAN_BAT_TEMP),
> +};

...

> +static int mt6397_auxadc_battemp_bias(struct mt6397_auxadc *adc, bool on)
> +{
> +	struct regmap *map = adc->regmap;
> +	int ret;
> +
> +	if (on) {
> +		ret = regmap_set_bits(map, MT6397_AUXADC_CON0,
> +				      MT6397_AUXADC_CON0_BUF_PWD_ON);
> +		if (ret)
> +			return ret;
> +		ret = regmap_set_bits(map, MT6397_AUXADC_CON0,
> +				      MT6397_AUXADC_CON0_BUF_PWD_B);
> +		if (ret)
> +			return ret;
> +		return regmap_set_bits(map, MT6397_CHR_CON7,
> +				       MT6397_CHR_CON7_BATON_TDET_EN);
> +	}
> +
> +	ret = regmap_clear_bits(map, MT6397_CHR_CON7,
> +				MT6397_CHR_CON7_BATON_TDET_EN);
> +	ret = ret ?: regmap_clear_bits(map, MT6397_AUXADC_CON0,
> +				       MT6397_AUXADC_CON0_BUF_PWD_B);
> +	return ret ?: regmap_clear_bits(map, MT6397_AUXADC_CON0,
> +					MT6397_AUXADC_CON0_BUF_PWD_ON);

Huh?! Please, use standard pattern with 'if (ret) return ret;'.
Ditto for other weird cases like this.

> +}

...

> +{
> +	unsigned int i, sum = 0;
> +	int ret, sample;

It's preferred not to mix ret with other semantically different variables.

> +	/* Held across the whole burst: the channel select is shared state. */
> +	guard(mutex)(&adc->lock);
> +
> +	if (chan->channel == MT6397_AUXADC_ISENSE) {
> +		ret = mt6397_auxadc_isense_enable(adc);
> +		if (ret)
> +			return ret;
> +		fsleep(MT6397_AUXADC_ISENSE_SETTLE_US);
> +	} else {
> +		ret = mt6397_auxadc_battemp_bias(adc, true);
> +		if (ret)
> +			return ret;
> +		fsleep(MT6397_AUXADC_BATTEMP_SETTLE_US);
> +	}

> +	for (i = 0; i < MT6397_AUXADC_SAMPLES; i++) {

	for (unsigned int i = 0; i < MT6397_AUXADC_SAMPLES; i++) {


> +		ret = mt6397_auxadc_read_once(adc, chan, &sample);
> +		if (ret)
> +			break;
> +
> +		sum += sample;
> +	}
> +
> +	/* Lower START so the converter is not left armed between reads. */
> +	regmap_clear_bits(adc->regmap, MT6397_AUXADC_CON1,
> +			  MT6397_AUXADC_CON1_START);
> +
> +	if (chan->channel == MT6397_AUXADC_ISENSE)
> +		mt6397_auxadc_isense_disable(adc);
> +	else
> +		mt6397_auxadc_battemp_bias(adc, false);
> +
> +	if (ret)
> +		return ret;
> +
> +	*val = DIV_ROUND_CLOSEST(sum, MT6397_AUXADC_SAMPLES);
> +
> +	return 0;
> +}

...

Otherwise nice and small driver.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] iio: adc: mt6397-auxadc: add mt6397 PMIC AUXADC driver

[Ryan Brue]

On 9/16/26 4:55 AM, Andy Shevchenko wrote:
> On Tue, Sep 15, 2026 at 11:15:27PM -0500, Ryan Brue wrote:
>> The mt6397 AUXADC is a 10-bit ADC behind the SoC's PMIC wrapper. On boards
>> built around this PMIC it is the only way to read the battery: the SoC's
>> AUXADC is wired to board thermistors and the charger ICs these boards use
>> have no ADC of their own.
>>
>> Add a driver exposing the battery voltage and battery temperature
>> channels. Only those two are described, so a channel ID in the device tree
>> is an index into the driver's channel array rather than the PMIC's channel
>> number, as mt6323-auxadc does. The ready bit lives in a channel's raw
>> result register, but the value comes from the chip's trimmed copy of it,
>> which is what the vendor driver reads for a measurement.
>>
>> Both channels need more than that, as the vendor programs them. The
>> battery voltage is measured through ISENSE, because a board with a
>> switching charger in the power path leaves BATSNS on the charger's system
>> rail instead of on the pack. The thermistor only reads correctly with the
>> PMIC's battery-detect bias and input buffer enabled, which take 20 ms to
>> settle. Both are switched back off afterwards.
>>
>> Reads average sixteen conversions in software; the chip's sample
>> accumulator makes no measurable difference at any setting, so it is left
>> at one sample per conversion.
> ...
>
>> +/*
>> + * MediaTek MT6397 PMIC AUXADC IIO driver
>> + *
>> + * Copyright (c) 2026 Ryan Brue <ryanbrue.dev@gmail.com>
>> + *
>> + * Based on drivers/iio/adc/mt6323-auxadc.c
> Why not add this device support into that driver?
Please see [1].
>> + */
> ...
>
>> +#define MT6397_AUXADC_ISENSE_SETTLE_US	USEC_PER_MSEC
> (1 * USEC_PER_MSEC)
Ack, fixed in v2
> ...
>
>> +static const struct iio_chan_spec mt6397_auxadc_channels[] = {
>> +	MTK_PMIC_IIO_CHAN(isense,   MT6397_AUXADC_ISENSE,
> One space too many.
Ack, fixed in v2
>> +			  MT6397_AUXADC_HWCHAN_BATSNS),
>> +	MTK_PMIC_IIO_CHAN(bat_temp, MT6397_AUXADC_BAT_TEMP,
>> +			  MT6397_AUXADC_HWCHAN_BAT_TEMP),
>> +};
> ...
>
>> +static int mt6397_auxadc_battemp_bias(struct mt6397_auxadc *adc, bool on)
>> +{
>> +	struct regmap *map = adc->regmap;
>> +	int ret;
>> +
>> +	if (on) {
>> +		ret = regmap_set_bits(map, MT6397_AUXADC_CON0,
>> +				      MT6397_AUXADC_CON0_BUF_PWD_ON);
>> +		if (ret)
>> +			return ret;
>> +		ret = regmap_set_bits(map, MT6397_AUXADC_CON0,
>> +				      MT6397_AUXADC_CON0_BUF_PWD_B);
>> +		if (ret)
>> +			return ret;
>> +		return regmap_set_bits(map, MT6397_CHR_CON7,
>> +				       MT6397_CHR_CON7_BATON_TDET_EN);
>> +	}
>> +
>> +	ret = regmap_clear_bits(map, MT6397_CHR_CON7,
>> +				MT6397_CHR_CON7_BATON_TDET_EN);
>> +	ret = ret ?: regmap_clear_bits(map, MT6397_AUXADC_CON0,
>> +				       MT6397_AUXADC_CON0_BUF_PWD_B);
>> +	return ret ?: regmap_clear_bits(map, MT6397_AUXADC_CON0,
>> +					MT6397_AUXADC_CON0_BUF_PWD_ON);
> Huh?! Please, use standard pattern with 'if (ret) return ret;'.
> Ditto for other weird cases like this.
Fixed in v2, and also modified to allow the regmap clears and sets to 
fall through, so one failure doesn't leave some of those bits in the 
wrong state.
>> +}
> ...
>
>> +{
>> +	unsigned int i, sum = 0;
>> +	int ret, sample;
> It's preferred not to mix ret with other semantically different variables.
Ack, thanks! Fixed in v2
>> +	/* Held across the whole burst: the channel select is shared state. */
>> +	guard(mutex)(&adc->lock);
>> +
>> +	if (chan->channel == MT6397_AUXADC_ISENSE) {
>> +		ret = mt6397_auxadc_isense_enable(adc);
>> +		if (ret)
>> +			return ret;
>> +		fsleep(MT6397_AUXADC_ISENSE_SETTLE_US);
>> +	} else {
>> +		ret = mt6397_auxadc_battemp_bias(adc, true);
>> +		if (ret)
>> +			return ret;
>> +		fsleep(MT6397_AUXADC_BATTEMP_SETTLE_US);
>> +	}
>> +	for (i = 0; i < MT6397_AUXADC_SAMPLES; i++) {
> 	for (unsigned int i = 0; i < MT6397_AUXADC_SAMPLES; i++) {
>
>
>> +		ret = mt6397_auxadc_read_once(adc, chan, &sample);
>> +		if (ret)
>> +			break;
>> +
>> +		sum += sample;
>> +	}
>> +
>> +	/* Lower START so the converter is not left armed between reads. */
>> +	regmap_clear_bits(adc->regmap, MT6397_AUXADC_CON1,
>> +			  MT6397_AUXADC_CON1_START);
>> +
>> +	if (chan->channel == MT6397_AUXADC_ISENSE)
>> +		mt6397_auxadc_isense_disable(adc);
>> +	else
>> +		mt6397_auxadc_battemp_bias(adc, false);
>> +
>> +	if (ret)
>> +		return ret;
>> +
>> +	*val = DIV_ROUND_CLOSEST(sum, MT6397_AUXADC_SAMPLES);
>> +
>> +	return 0;
>> +}
> ...
>
> Otherwise nice and small driver.
Thanks for the review, Andy!

Best regards,
Ryan

Re: [PATCH 2/3] iio: adc: mt6397-auxadc: add mt6397 PMIC AUXADC driver

[Ryan Brue]

On 9/17/26 4:01 PM, Ryan Brue wrote:
> On 9/16/26 4:55 AM, Andy Shevchenko wrote:
>> Why not add this device support into that driver?
> Please see [1].
>
My apologies for double-replying, I forgot my link here.

[1] 
https://lore.kernel.org/all/2d1b80f2-53f6-4ed6-81bf-e35c0a9efb26@gmail.com/

Best regards,
Ryan

Re: [PATCH 2/3] iio: adc: mt6397-auxadc: add mt6397 PMIC AUXADC driver

[Jonathan Cameron]

On Tue, 15 Sep 2026 23:15:27 -0500
Ryan Brue <ryanbrue.dev@gmail.com> wrote:

> The mt6397 AUXADC is a 10-bit ADC behind the SoC's PMIC wrapper. On boards
> built around this PMIC it is the only way to read the battery: the SoC's
> AUXADC is wired to board thermistors and the charger ICs these boards use
> have no ADC of their own.
> 
> Add a driver exposing the battery voltage and battery temperature
> channels. Only those two are described, so a channel ID in the device tree
> is an index into the driver's channel array rather than the PMIC's channel
> number, as mt6323-auxadc does. The ready bit lives in a channel's raw
> result register, but the value comes from the chip's trimmed copy of it,
> which is what the vendor driver reads for a measurement.
> 
> Both channels need more than that, as the vendor programs them. The
> battery voltage is measured through ISENSE, because a board with a
> switching charger in the power path leaves BATSNS on the charger's system
> rail instead of on the pack. The thermistor only reads correctly with the
> PMIC's battery-detect bias and input buffer enabled, which take 20 ms to
> settle. Both are switched back off afterwards.
> 
> Reads average sixteen conversions in software; the chip's sample
> accumulator makes no measurable difference at any setting, so it is left
> at one sample per conversion.
> 
> Assisted-by: LLM
> Signed-off-by: Ryan Brue <ryanbrue.dev@gmail.com>

I took a quick look but Andy called out the biggest question,
why doesn't it make sense to extend an existing driver to
cover this part?  Might not make sense but you need to talk
through why not in the cover letter.

> diff --git a/drivers/iio/adc/mt6397-auxadc.c b/drivers/iio/adc/mt6397-auxadc.c
> new file mode 100644
> index 000000000000..dbfa82341d6c
> --- /dev/null
> +++ b/drivers/iio/adc/mt6397-auxadc.c
...

> +/*
> + * CHR_CON16 and the two SOURCE_CH0 selects route either BATSNS or ISENSE onto
> + * the battery channel. Only ISENSE is used: with a switching charger in the
> + * power path BATSNS sits on the system rail rather than on the pack.
> + */
> +#define MT6397_CHR_CON16		0x0020
> +#define MT6397_CHR_CON16_ADCIN_VSEN_EN	BIT(11)
> +#define MT6397_CHR_CON16_ADCIN_VBAT_EN	BIT(10)

These are used as values for the ADCIN_SEL.
I think they should have names to make that clear and be
1 and 2 rather than bits.

> +#define MT6397_CHR_CON16_ADCIN_SEL	(MT6397_CHR_CON16_ADCIN_VSEN_EN | \
> +					 MT6397_CHR_CON16_ADCIN_VBAT_EN)

This would then be a register field mask from GENMASK()

> +#define MT6397_AUXADC_CON14		0x055e
> +#define MT6397_AUXADC_CON14_CH0_NORM_SEL BIT(2)
> +#define MT6397_AUXADC_CON14_CH0_LBAT_SEL BIT(0)

A common visual trick in IIO drivers separates register addresses from fields
and from values.
#define MT6397_AUXADC_CON14		0x055e
#define   MT6397_AUXADC_CON14_CH0_NORM_SEL BIT(2)
#define   MT6397_AUXADC_CON14_CH0_LBAT_SEL BIT(0)

Also blank line here as the settle time is not directly related to the
register.
> +#define MT6397_AUXADC_ISENSE_SETTLE_US	USEC_PER_MSEC
> +
> +struct mt6397_auxadc {
> +	struct regmap *regmap;
> +	/* AUXADC doesn't support reading multiple channels simultaneously. */

I'd rather you said why than simply not supported. Seems
that it's a multi register sequence so needs to be atomic wrt
to other channel reads.

> +	struct mutex lock;
> +};
> +
> +#define MTK_PMIC_IIO_CHAN(_name, _chan, _hwchan)                \
> +{                                                               \
> +	.type = IIO_VOLTAGE,                                    \
> +	.indexed = 1,                                           \
> +	.channel = _chan,                                       \
> +	.address = _hwchan,                                     \
> +	.datasheet_name = __stringify(_name),                   \
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |          \
> +			      BIT(IIO_CHAN_INFO_SCALE),         \
> +}
For two the macro is of marginal benefit. I'd just put the structure
values in directly.  Then we know which parameter is which etc.
> +
> +/*
> + * A device tree channel ID indexes this array; .address holds the PMIC channel
> + * number. The thermistor reports as a voltage because that is all the PMIC
> + * measures: the divider across an NTC whose curve belongs to the board.
> + */
> +static const struct iio_chan_spec mt6397_auxadc_channels[] = {
> +	MTK_PMIC_IIO_CHAN(isense,   MT6397_AUXADC_ISENSE,

If you do keep this, one space is enough. Aligning when there
are lines in between brings little value that I can see.

> +			  MT6397_AUXADC_HWCHAN_BATSNS),
> +	MTK_PMIC_IIO_CHAN(bat_temp, MT6397_AUXADC_BAT_TEMP,
> +			  MT6397_AUXADC_HWCHAN_BAT_TEMP),
> +};
> +
> +static int mt6397_auxadc_read_once(struct mt6397_auxadc *adc,
> +				   const struct iio_chan_spec *chan, int *val)
> +{
> +	struct regmap *map = adc->regmap;
> +	unsigned int reg;
> +	int ret;
> +
> +	ret = regmap_update_bits(map, MT6397_AUXADC_CON1,
> +				 MT6397_AUXADC_CON1_CHSEL,
> +				 FIELD_PREP(MT6397_AUXADC_CON1_CHSEL,
> +					    chan->address));
> +	if (ret)
> +		return ret;
> +
> +	/* START is edge triggered: it has to be lowered before being raised. */
> +	ret = regmap_clear_bits(map, MT6397_AUXADC_CON1,
> +				MT6397_AUXADC_CON1_START);
For lines that go not much over 80 chars and where it helps readability it
is fine to break that limit.  I think that applies here.
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_set_bits(map, MT6397_AUXADC_CON1,
> +			      MT6397_AUXADC_CON1_START);
and here.

> +	if (ret)
> +		return ret;
> +
> +	fsleep(MT6397_AUXADC_START_US);
> +
> +	ret = regmap_read_poll_timeout(map, MT6397_AUXADC_ADC(chan->address),
> +				       reg, reg & MT6397_AUXADC_ADC_RDY,
> +				       100, 100 * USEC_PER_MSEC);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(map, MT6397_AUXADC_ADC_TRIM(chan->address), &reg);
> +	if (ret)
> +		return ret;
> +
> +	*val = FIELD_GET(MT6397_AUXADC_ADC_VAL, reg);
> +
> +	return 0;
> +}
> +
> +static int mt6397_auxadc_battemp_bias(struct mt6397_auxadc *adc, bool on)
> +{
> +	struct regmap *map = adc->regmap;
> +	int ret;
> +
> +	if (on) {
> +		ret = regmap_set_bits(map, MT6397_AUXADC_CON0,
> +				      MT6397_AUXADC_CON0_BUF_PWD_ON);
> +		if (ret)
> +			return ret;
Add a blank line here 
> +		ret = regmap_set_bits(map, MT6397_AUXADC_CON0,
> +				      MT6397_AUXADC_CON0_BUF_PWD_B);
> +		if (ret)
> +			return ret;
and here.  Small readability improvement but worth having!

> +		return regmap_set_bits(map, MT6397_CHR_CON7,
> +				       MT6397_CHR_CON7_BATON_TDET_EN);
> +	}

For cases like this I'd use and else.
Not strictly necessary but makes it visually obvious it is an either or
question.

> +
> +	ret = regmap_clear_bits(map, MT6397_CHR_CON7,
> +				MT6397_CHR_CON7_BATON_TDET_EN);
> +	ret = ret ?: regmap_clear_bits(map, MT6397_AUXADC_CON0,
> +				       MT6397_AUXADC_CON0_BUF_PWD_B);
> +	return ret ?: regmap_clear_bits(map, MT6397_AUXADC_CON0,
> +					MT6397_AUXADC_CON0_BUF_PWD_ON);
> +}

> +
> +static int mt6397_auxadc_isense_disable(struct mt6397_auxadc *adc)
> +{
> +	struct regmap *map = adc->regmap;
> +	int ret;
> +
> +	ret = regmap_clear_bits(map, MT6397_AUXADC_CON14,
> +				MT6397_AUXADC_CON14_CH0_NORM_SEL |
> +				MT6397_AUXADC_CON14_CH0_LBAT_SEL);
> +
> +	return ret ?: regmap_clear_bits(map, MT6397_CHR_CON16,
> +					MT6397_CHR_CON16_ADCIN_SEL);

Andy called this out already.  LLM being too clever maybe?
Anyhow, just burn a few lines of code for readability!

> +}
> +
> +static int mt6397_auxadc_read_channel(struct mt6397_auxadc *adc,
> +				      const struct iio_chan_spec *chan,
> +				      int *val)
> +{
> +	unsigned int i, sum = 0;

Please split variables that are initialized from ones that aren't.
Can be hard to spot when just one is.

> +	int ret, sample;
> +
> +	/* Held across the whole burst: the channel select is shared state. */
> +	guard(mutex)(&adc->lock);
> +
> +	if (chan->channel == MT6397_AUXADC_ISENSE) {
> +		ret = mt6397_auxadc_isense_enable(adc);
> +		if (ret)
> +			return ret;
> +		fsleep(MT6397_AUXADC_ISENSE_SETTLE_US);
> +	} else {
> +		ret = mt6397_auxadc_battemp_bias(adc, true);
> +		if (ret)
> +			return ret;
> +		fsleep(MT6397_AUXADC_BATTEMP_SETTLE_US);
> +	}
> +
> +	for (i = 0; i < MT6397_AUXADC_SAMPLES; i++) {

	for (unsigned int i = 0; ...

> +		ret = mt6397_auxadc_read_once(adc, chan, &sample);
> +		if (ret)
> +			break;
> +
> +		sum += sample;
> +	}
> +
> +	/* Lower START so the converter is not left armed between reads. */
> +	regmap_clear_bits(adc->regmap, MT6397_AUXADC_CON1,
> +			  MT6397_AUXADC_CON1_START);
> +
> +	if (chan->channel == MT6397_AUXADC_ISENSE)
> +		mt6397_auxadc_isense_disable(adc);
> +	else
> +		mt6397_auxadc_battemp_bias(adc, false);
> +
> +	if (ret)
> +		return ret;
> +
> +	*val = DIV_ROUND_CLOSEST(sum, MT6397_AUXADC_SAMPLES);
> +
> +	return 0;
> +}

> +
> +static int mt6397_auxadc_init(struct mt6397_auxadc *adc)
> +{
> +	return regmap_update_bits(adc->regmap, MT6397_AUXADC_CON0,
> +				  MT6397_AUXADC_CON0_SPL_NUM,
> +				  FIELD_PREP(MT6397_AUXADC_CON0_SPL_NUM, 1));

Not worth having a helper to call just one thing. Put this regmap
call inline.

> +}

> +
> +static const struct of_device_id mt6397_auxadc_of_match[] = {
> +	{ .compatible = "mediatek,mt6397-auxadc" },
> +	{ /* sentinel */ }
	{ }

Is enough.  The sentinel nature of that is kind of obvious so
a while back we removed all those comments from IIO.


> +};
> +MODULE_DEVICE_TABLE(of, mt6397_auxadc_of_match);

Re: [PATCH 2/3] iio: adc: mt6397-auxadc: add mt6397 PMIC AUXADC driver

[Ryan Brue]

On 9/16/26 10:48 PM, Jonathan Cameron wrote:
> On Tue, 15 Sep 2026 23:15:27 -0500
> Ryan Brue <ryanbrue.dev@gmail.com> wrote:
>
>> The mt6397 AUXADC is a 10-bit ADC behind the SoC's PMIC wrapper. On boards
>> built around this PMIC it is the only way to read the battery: the SoC's
>> AUXADC is wired to board thermistors and the charger ICs these boards use
>> have no ADC of their own.
>>
>> Add a driver exposing the battery voltage and battery temperature
>> channels. Only those two are described, so a channel ID in the device tree
>> is an index into the driver's channel array rather than the PMIC's channel
>> number, as mt6323-auxadc does. The ready bit lives in a channel's raw
>> result register, but the value comes from the chip's trimmed copy of it,
>> which is what the vendor driver reads for a measurement.
>>
>> Both channels need more than that, as the vendor programs them. The
>> battery voltage is measured through ISENSE, because a board with a
>> switching charger in the power path leaves BATSNS on the charger's system
>> rail instead of on the pack. The thermistor only reads correctly with the
>> PMIC's battery-detect bias and input buffer enabled, which take 20 ms to
>> settle. Both are switched back off afterwards.
>>
>> Reads average sixteen conversions in software; the chip's sample
>> accumulator makes no measurable difference at any setting, so it is left
>> at one sample per conversion.
>>
>> Assisted-by: LLM
>> Signed-off-by: Ryan Brue <ryanbrue.dev@gmail.com>
> I took a quick look but Andy called out the biggest question,
> why doesn't it make sense to extend an existing driver to
> cover this part?  Might not make sense but you need to talk
> through why not in the cover letter.
I replied to Andy with an explanation [1], and I will justify in my 
cover letter for v2. In short, I think the new driver is justified, but 
if it's desired to roll in the mt6397 with another driver, let me know.
>> diff --git a/drivers/iio/adc/mt6397-auxadc.c b/drivers/iio/adc/mt6397-auxadc.c
>> new file mode 100644
>> index 000000000000..dbfa82341d6c
>> --- /dev/null
>> +++ b/drivers/iio/adc/mt6397-auxadc.c
> ...
>
>> +/*
>> + * CHR_CON16 and the two SOURCE_CH0 selects route either BATSNS or ISENSE onto
>> + * the battery channel. Only ISENSE is used: with a switching charger in the
>> + * power path BATSNS sits on the system rail rather than on the pack.
>> + */
>> +#define MT6397_CHR_CON16		0x0020
>> +#define MT6397_CHR_CON16_ADCIN_VSEN_EN	BIT(11)
>> +#define MT6397_CHR_CON16_ADCIN_VBAT_EN	BIT(10)
> These are used as values for the ADCIN_SEL.
> I think they should have names to make that clear and be
> 1 and 2 rather than bits.
The OR between VSEN_EN and VBAT_EN was a mistake. These two bits are 
part of a larger set of 5 mutually exclusive enable bits (CON16 bits 
12:8). I fixed this in v2, and also added the #define s for the others. 
Let me know if I shouldn't add those because we don't use them, I'm on 
the fence for that.
>> +#define MT6397_CHR_CON16_ADCIN_SEL	(MT6397_CHR_CON16_ADCIN_VSEN_EN | \
>> +					 MT6397_CHR_CON16_ADCIN_VBAT_EN)
> This would then be a register field mask from GENMASK()
Mentioned above, but these are mutually exclusive bits, not a field, but 
it's my fault that it looked like one. Fixed in v2.
>
>> +#define MT6397_AUXADC_CON14		0x055e
>> +#define MT6397_AUXADC_CON14_CH0_NORM_SEL BIT(2)
>> +#define MT6397_AUXADC_CON14_CH0_LBAT_SEL BIT(0)
> A common visual trick in IIO drivers separates register addresses from fields
> and from values.
> #define MT6397_AUXADC_CON14		0x055e
> #define   MT6397_AUXADC_CON14_CH0_NORM_SEL BIT(2)
> #define   MT6397_AUXADC_CON14_CH0_LBAT_SEL BIT(0)
>
> Also blank line here as the settle time is not directly related to the
> register.
>> +#define MT6397_AUXADC_ISENSE_SETTLE_US	USEC_PER_MSEC
>> +
>> +struct mt6397_auxadc {
>> +	struct regmap *regmap;
>> +	/* AUXADC doesn't support reading multiple channels simultaneously. */
> I'd rather you said why than simply not supported. Seems
> that it's a multi register sequence so needs to be atomic wrt
> to other channel reads.
Ack, I'll justify it in v2. You're right about the reason. In 
particular, all the channel reads need CON1.
>> +	struct mutex lock;
>> +};
>> +
>> +#define MTK_PMIC_IIO_CHAN(_name, _chan, _hwchan)                \
>> +{                                                               \
>> +	.type = IIO_VOLTAGE,                                    \
>> +	.indexed = 1,                                           \
>> +	.channel = _chan,                                       \
>> +	.address = _hwchan,                                     \
>> +	.datasheet_name = __stringify(_name),                   \
>> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |          \
>> +			      BIT(IIO_CHAN_INFO_SCALE),         \
>> +}
> For two the macro is of marginal benefit. I'd just put the structure
> values in directly.  Then we know which parameter is which etc.
Done in v2. If I had to guess why the LLM might have chosen the macro, 
it might be because there are other channels that exist, just not 
channels that my device uses (referencing the vendor's Amazon Fire OS 
which is based on Linux 3.18). Making it a macro would make it maybe 
cleaner for anybody who comes in and needs to add more channels? Who knows.
>> +
>> +/*
>> + * A device tree channel ID indexes this array; .address holds the PMIC channel
>> + * number. The thermistor reports as a voltage because that is all the PMIC
>> + * measures: the divider across an NTC whose curve belongs to the board.
>> + */
>> +static const struct iio_chan_spec mt6397_auxadc_channels[] = {
>> +	MTK_PMIC_IIO_CHAN(isense,   MT6397_AUXADC_ISENSE,
> If you do keep this, one space is enough. Aligning when there
> are lines in between brings little value that I can see.
>
>> +			  MT6397_AUXADC_HWCHAN_BATSNS),
>> +	MTK_PMIC_IIO_CHAN(bat_temp, MT6397_AUXADC_BAT_TEMP,
>> +			  MT6397_AUXADC_HWCHAN_BAT_TEMP),
>> +};
>> +
>> +static int mt6397_auxadc_read_once(struct mt6397_auxadc *adc,
>> +				   const struct iio_chan_spec *chan, int *val)
>> +{
>> +	struct regmap *map = adc->regmap;
>> +	unsigned int reg;
>> +	int ret;
>> +
>> +	ret = regmap_update_bits(map, MT6397_AUXADC_CON1,
>> +				 MT6397_AUXADC_CON1_CHSEL,
>> +				 FIELD_PREP(MT6397_AUXADC_CON1_CHSEL,
>> +					    chan->address));
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* START is edge triggered: it has to be lowered before being raised. */
>> +	ret = regmap_clear_bits(map, MT6397_AUXADC_CON1,
>> +				MT6397_AUXADC_CON1_START);
> For lines that go not much over 80 chars and where it helps readability it
> is fine to break that limit.  I think that applies here.
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = regmap_set_bits(map, MT6397_AUXADC_CON1,
>> +			      MT6397_AUXADC_CON1_START);
> and here.
Both done in v2.
>> +	if (ret)
>> +		return ret;
>> +
>> +	fsleep(MT6397_AUXADC_START_US);
>> +
>> +	ret = regmap_read_poll_timeout(map, MT6397_AUXADC_ADC(chan->address),
>> +				       reg, reg & MT6397_AUXADC_ADC_RDY,
>> +				       100, 100 * USEC_PER_MSEC);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = regmap_read(map, MT6397_AUXADC_ADC_TRIM(chan->address), &reg);
>> +	if (ret)
>> +		return ret;
>> +
>> +	*val = FIELD_GET(MT6397_AUXADC_ADC_VAL, reg);
>> +
>> +	return 0;
>> +}
>> +
>> +static int mt6397_auxadc_battemp_bias(struct mt6397_auxadc *adc, bool on)
>> +{
>> +	struct regmap *map = adc->regmap;
>> +	int ret;
>> +
>> +	if (on) {
>> +		ret = regmap_set_bits(map, MT6397_AUXADC_CON0,
>> +				      MT6397_AUXADC_CON0_BUF_PWD_ON);
>> +		if (ret)
>> +			return ret;
> Add a blank line here
>> +		ret = regmap_set_bits(map, MT6397_AUXADC_CON0,
>> +				      MT6397_AUXADC_CON0_BUF_PWD_B);
>> +		if (ret)
>> +			return ret;
> and here.  Small readability improvement but worth having!
Done in v2
>> +		return regmap_set_bits(map, MT6397_CHR_CON7,
>> +				       MT6397_CHR_CON7_BATON_TDET_EN);
>> +	}
> For cases like this I'd use and else.
> Not strictly necessary but makes it visually obvious it is an either or
> question.
Done in v2
>> +
>> +	ret = regmap_clear_bits(map, MT6397_CHR_CON7,
>> +				MT6397_CHR_CON7_BATON_TDET_EN);
>> +	ret = ret ?: regmap_clear_bits(map, MT6397_AUXADC_CON0,
>> +				       MT6397_AUXADC_CON0_BUF_PWD_B);
>> +	return ret ?: regmap_clear_bits(map, MT6397_AUXADC_CON0,
>> +					MT6397_AUXADC_CON0_BUF_PWD_ON);
>> +}
>> +
>> +static int mt6397_auxadc_isense_disable(struct mt6397_auxadc *adc)
>> +{
>> +	struct regmap *map = adc->regmap;
>> +	int ret;
>> +
>> +	ret = regmap_clear_bits(map, MT6397_AUXADC_CON14,
>> +				MT6397_AUXADC_CON14_CH0_NORM_SEL |
>> +				MT6397_AUXADC_CON14_CH0_LBAT_SEL);
>> +
>> +	return ret ?: regmap_clear_bits(map, MT6397_CHR_CON16,
>> +					MT6397_CHR_CON16_ADCIN_SEL);
> Andy called this out already.  LLM being too clever maybe?
> Anyhow, just burn a few lines of code for readability!
Oh yeah, almost certainly. These are the kinds of lessons I learn when 
I'm new to kernel development and have no clue how simple or complex 
certain blocks should be, haha.
Done in v2, Sashiko also pointed out that the early return could leave 
some register bits set that should be cleared, so that will be fixed in 
v2 as well.
>> +}
>> +
>> +static int mt6397_auxadc_read_channel(struct mt6397_auxadc *adc,
>> +				      const struct iio_chan_spec *chan,
>> +				      int *val)
>> +{
>> +	unsigned int i, sum = 0;
> Please split variables that are initialized from ones that aren't.
> Can be hard to spot when just one is.
Done in v2.
>> +	int ret, sample;
>> +
>> +	/* Held across the whole burst: the channel select is shared state. */
>> +	guard(mutex)(&adc->lock);
>> +
>> +	if (chan->channel == MT6397_AUXADC_ISENSE) {
>> +		ret = mt6397_auxadc_isense_enable(adc);
>> +		if (ret)
>> +			return ret;
>> +		fsleep(MT6397_AUXADC_ISENSE_SETTLE_US);
>> +	} else {
>> +		ret = mt6397_auxadc_battemp_bias(adc, true);
>> +		if (ret)
>> +			return ret;
>> +		fsleep(MT6397_AUXADC_BATTEMP_SETTLE_US);
>> +	}
>> +
>> +	for (i = 0; i < MT6397_AUXADC_SAMPLES; i++) {
> 	for (unsigned int i = 0; ...
>
>> +		ret = mt6397_auxadc_read_once(adc, chan, &sample);
>> +		if (ret)
>> +			break;
>> +
>> +		sum += sample;
>> +	}
>> +
>> +	/* Lower START so the converter is not left armed between reads. */
>> +	regmap_clear_bits(adc->regmap, MT6397_AUXADC_CON1,
>> +			  MT6397_AUXADC_CON1_START);
>> +
>> +	if (chan->channel == MT6397_AUXADC_ISENSE)
>> +		mt6397_auxadc_isense_disable(adc);
>> +	else
>> +		mt6397_auxadc_battemp_bias(adc, false);
>> +
>> +	if (ret)
>> +		return ret;
>> +
>> +	*val = DIV_ROUND_CLOSEST(sum, MT6397_AUXADC_SAMPLES);
>> +
>> +	return 0;
>> +}
>> +
>> +static int mt6397_auxadc_init(struct mt6397_auxadc *adc)
>> +{
>> +	return regmap_update_bits(adc->regmap, MT6397_AUXADC_CON0,
>> +				  MT6397_AUXADC_CON0_SPL_NUM,
>> +				  FIELD_PREP(MT6397_AUXADC_CON0_SPL_NUM, 1));
> Not worth having a helper to call just one thing. Put this regmap
> call inline.
Done in v2.
>> +}
>> +
>> +static const struct of_device_id mt6397_auxadc_of_match[] = {
>> +	{ .compatible = "mediatek,mt6397-auxadc" },
>> +	{ /* sentinel */ }
> 	{ }
>
> Is enough.  The sentinel nature of that is kind of obvious so
> a while back we removed all those comments from IIO.
>
Done in v2.
>> +};
>> +MODULE_DEVICE_TABLE(of, mt6397_auxadc_of_match);
>>

Thanks for the review!

[1] 
https://lore.kernel.org/all/2d1b80f2-53f6-4ed6-81bf-e35c0a9efb26@gmail.com/

Best regards,
Ryan

[PATCH 3/3] mfd: mt6397-core: Add mt6397 AUXADC support

[Ryan Brue]

The mt6397 PMIC includes an AUXADC. Register the AUXADC in the mt6397
devices array to allow the corresponding driver to probe using compatible
string.

Assisted-by: LLM
Signed-off-by: Ryan Brue <ryanbrue.dev@gmail.com>
---
 drivers/mfd/mt6397-core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mfd/mt6397-core.c b/drivers/mfd/mt6397-core.c
index 9ec951996588..5d5f816492aa 100644
--- a/drivers/mfd/mt6397-core.c
+++ b/drivers/mfd/mt6397-core.c
@@ -261,6 +261,9 @@ static const struct mfd_cell mt6359_devs[] = {
 
 static const struct mfd_cell mt6397_devs[] = {
 	{
+		.name = "mt6397-auxadc",
+		.of_compatible = "mediatek,mt6397-auxadc",
+	}, {
 		.name = "mt6397-rtc",
 		.num_resources = ARRAY_SIZE(mt6397_rtc_resources),
 		.resources = mt6397_rtc_resources,

-- 
2.55.0

Re: [PATCH 0/3] iio: adc: add mt6397 PMIC AUXADC support

[Andy Shevchenko]

On Tue, Sep 15, 2026 at 11:15:25PM -0500, Ryan Brue wrote:
> The MediaTek mt6397 PMIC has a 10-bit AUXADC that nothing in-tree can
> reach. On boards built around it that ADC is the only path to the battery:
> the SoC's AUXADC is wired to board thermistors, and the charger ICs these
> boards use have no ADC at all, so without it there is no pack voltage and
> no state of charge.

This doesn't explain why a brand new driver? Perhaps we have existing code that
may be updated to support this device?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/3] iio: adc: add mt6397 PMIC AUXADC support

[Ryan Brue]

On 9/16/26 4:50 AM, Andy Shevchenko wrote:
> On Tue, Sep 15, 2026 at 11:15:25PM -0500, Ryan Brue wrote:
>> The MediaTek mt6397 PMIC has a 10-bit AUXADC that nothing in-tree can
>> reach. On boards built around it that ADC is the only path to the battery:
>> the SoC's AUXADC is wired to board thermistors, and the charger ICs these
>> boards use have no ADC at all, so without it there is no pack voltage and
>> no state of charge.
> This doesn't explain why a brand new driver? Perhaps we have existing code that
> may be updated to support this device?
I considered adding mt6397 support to either mt6323-auxadc or 
mt6359-auxadc, and both had problems.

Both mt6323-auxadc and mt6359-auxadc select channels through a request 
register (1 bit per channel), while mt6397 uses a 4-bit numeric field 
CHSEL in CON1 (10:7), and then pulses a START bit (CON1 bit 0). That was 
the biggest reason I made the new driver.

For mt6323-auxadc, which is the closest I could find to the mt6397 
(CON0..CON27), it has 13 more registers than the mt6397 (CON0..CON14). 
It uses CON22 for its request register, and reads the result value from 
the same register as the ready bit. We don't do that - the mt6397 has a 
factory calibrated value for each channel at 0x16 higher than the raw 
value. mt6323 also has a 1800 mV / 15 bit scale / resolution while we 
have 1200 mV / 10 bits. We also have some per-channel preparation that 
we have to do before the burst, that the mt6323 doesn't have to do.

For mt6359-auxadc, it has a more generic framework for describing the 
AUXADC, but it assumes requests are channel-per-bit, and so we would 
have to basically ignore req_idx, req_mask, rdy_idx, and rdy_mask.

We also have our own software sampling, which the vendor does too 
(Amazon Fire OS based on Linux 3.18). We'd have to have our own sampling 
callback to do it.

I drafted two other versions of these patches adding mt6397 support to 
both of those drivers, but the differences meant I had to add a lot of 
extra boilerplate to each driver and to me it didn't make sense.

In v2 I will add the justification to the cover letter and commits for 
why I chose a new driver.

If you'd like me to instead send the exploratory patches I made adapting 
mt6323-auxadc or mt6359-auxadc, let me know. I'm fine if it ends up 
seeming like we should adapt one of the existing drivers, but I think 
the mechanism for controlling this AUXADC is unique and merits its own 
driver.

Thanks again for the review, I am going through each one, and sorry for 
the delay. I'm rather new to kernel development.

Best regards,
Ryan

Re: [PATCH 0/3] iio: adc: add mt6397 PMIC AUXADC support

[Andy Shevchenko]

On Thu, Sep 17, 2026 at 03:24:05PM -0500, Ryan Brue wrote:
> On 9/16/26 4:50 AM, Andy Shevchenko wrote:
> > On Tue, Sep 15, 2026 at 11:15:25PM -0500, Ryan Brue wrote:
> > > The MediaTek mt6397 PMIC has a 10-bit AUXADC that nothing in-tree can
> > > reach. On boards built around it that ADC is the only path to the battery:
> > > the SoC's AUXADC is wired to board thermistors, and the charger ICs these
> > > boards use have no ADC at all, so without it there is no pack voltage and
> > > no state of charge.
> > This doesn't explain why a brand new driver? Perhaps we have existing code that
> > may be updated to support this device?
> I considered adding mt6397 support to either mt6323-auxadc or mt6359-auxadc,
> and both had problems.
> 
> Both mt6323-auxadc and mt6359-auxadc select channels through a request
> register (1 bit per channel), while mt6397 uses a 4-bit numeric field CHSEL
> in CON1 (10:7), and then pulses a START bit (CON1 bit 0). That was the
> biggest reason I made the new driver.
> 
> For mt6323-auxadc, which is the closest I could find to the mt6397
> (CON0..CON27), it has 13 more registers than the mt6397 (CON0..CON14). It
> uses CON22 for its request register, and reads the result value from the
> same register as the ready bit. We don't do that - the mt6397 has a factory
> calibrated value for each channel at 0x16 higher than the raw value. mt6323
> also has a 1800 mV / 15 bit scale / resolution while we have 1200 mV / 10
> bits. We also have some per-channel preparation that we have to do before
> the burst, that the mt6323 doesn't have to do.
> 
> For mt6359-auxadc, it has a more generic framework for describing the
> AUXADC, but it assumes requests are channel-per-bit, and so we would have to
> basically ignore req_idx, req_mask, rdy_idx, and rdy_mask.

> We also have our own software sampling, which the vendor does too (Amazon
> Fire OS based on Linux 3.18). We'd have to have our own sampling callback to
> do it.

If it's not an upstream, we do not care.

> I drafted two other versions of these patches adding mt6397 support to both
> of those drivers, but the differences meant I had to add a lot of extra
> boilerplate to each driver and to me it didn't make sense.
> 
> In v2 I will add the justification to the cover letter and commits for why I
> chose a new driver.

Yes, please. Always do that when submitting a brand new driver that might sound
like a (partially) compatible with something already existing.

> If you'd like me to instead send the exploratory patches I made adapting
> mt6323-auxadc or mt6359-auxadc, let me know. I'm fine if it ends up seeming
> like we should adapt one of the existing drivers, but I think the mechanism
> for controlling this AUXADC is unique and merits its own driver.
> 
> Thanks again for the review, I am going through each one, and sorry for the
> delay. I'm rather new to kernel development.

No problem, and welcome!

-- 
With Best Regards,
Andy Shevchenko