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

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 20 Sep 2026 00:31:52 -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
> 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 <ryanbrue.dev@gmail.com>
> 
Hi Ryan,

A few things related to code flow and readability inline.

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


> +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;
> +}