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

On Sun, 20 Sep 2026 00:31:52 -0500
Ryan Brue [off-list ref] 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
indexes 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 factory-calibrated copy.

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 <redacted>

Hi Ryan,

A few things related to code flow and readability inline.

+static int mt6397_auxadc_isense_enable(struct mt6397_auxadc *adc)
+{
+	struct regmap *map = adc->regmap;
+	int ret;
+
+	/*
+	 * Two of the five independent input enables, not a select field:
+	 * BATSNS is cleared first so that the two inputs are never enabled
+	 * onto the battery channel at the same time.

I'm not following this comment. Perhaps consider a rewrite?

+	 */
+	ret = regmap_clear_bits(map, MT6397_CHR_CON16, MT6397_CHR_CON16_ADCIN_VBAT_EN);
+	if (ret)
+		return ret;
+
+	ret = regmap_set_bits(map, MT6397_CHR_CON16, 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_read_channel(struct iio_dev *indio_dev,
+				      const struct iio_chan_spec *chan,
+				      int *val)
+{
+	struct mt6397_auxadc *adc = iio_priv(indio_dev);
+	bool isense = chan->channel == MT6397_AUXADC_ISENSE;
+	int ret, stop_err, release_err;
+
+	guard(mutex)(&adc->lock);

Go back to conventional mutex_lock() / mutex_unlock() or wrap
everything under the guard so you can avoid mixing goto and 
guards() (see cleanup.h docs for some of the reason why).

One option would be to just move them up in to the switch statement
from which this is called with scope carefully defined there.
It's not ideal but having a wrapper for just taking a lock
isn't pretty either and you need to do one or the other if you
don't want to fallback to non guard() approaches.

+
+	if (isense) {
+		ret = mt6397_auxadc_isense_enable(adc);
+		if (!ret)
+			fsleep(MT6397_AUXADC_ISENSE_SETTLE_US);

Keep to errors out of line 
		if (ret)
			goto err_stop;

		fsleep(MT6397_AUXADC_ISENSE_SETTLE_US);

+	} else {
+		ret = mt6397_auxadc_battemp_bias_on(adc);
+		if (!ret)
+			fsleep(MT6397_AUXADC_BATTEMP_SETTLE_US);
+	}
+
+	if (!ret)

With changes above you won't get here with ret not 0 so this can go.

+		ret = mt6397_auxadc_read_burst(adc, chan, val);
+
+	/*
+	 * The teardown is unconditional: once any part of the setup has been
+	 * written it has to run, even if the setup itself failed part-way.
+	 * Lowering START also leaves the converter unarmed between reads.
+	 * A caller has nothing to do with a teardown error, so it is logged
+	 * rather than returned.
+	 */
+	stop_err = regmap_clear_bits(adc->regmap, MT6397_AUXADC_CON1,
+				     MT6397_AUXADC_CON1_START);
+
+	if (isense)
+		release_err = mt6397_auxadc_isense_disable(adc);
+	else
+		release_err = mt6397_auxadc_battemp_bias_off(adc);
+
+	if (stop_err || release_err)
+		dev_err(&indio_dev->dev, "failed to release channel %d\n",
+			chan->channel);

I would make a little helper for the stuff that has to run.
So everything from the comment onwards. Then

	return mt6397_read_channel_tear_down();

err_stop:
	mt6397_read_channel_tear_down();

	return ret;
(with locks if you don't move those up into a wrapper.

+
+	return ret;
+}