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

From: Ryan Brue

Date: Thu Sep 17 2026 - 19:04:24 EST

On 9/16/26 10:48 PM, Jonathan Cameron wrote:

On Tue, 15 Sep 2026 23:15:27 -0500
Ryan Brue <ryanbrue.dev@xxxxxxxxx> 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@xxxxxxxxx>

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@xxxxxxxxx/

Best regards,
Ryan