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

[Jonathan Cameron]

On Mon, 28 Sep 2026 15:57:20 -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
> 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@xxxxxxxxx>
Hi Ryan

Sorry for the delay.

Just one thing inline. If I wasn't tired from a long day
of review I might just have made the change and asked you
to check it. So bad luck on that!

Thanks,

Jonathan

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

> +static void mt6397_auxadc_release(struct mt6397_auxadc *adc,
> +				  const struct iio_chan_spec *chan)
> +{
> +	/* Lowering START leaves the converter unarmed 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_off(adc);
> +}
> +
> +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);
> +	int ret;
> +
> +	mutex_lock(&adc->lock);
> +
> +	if (chan->channel == MT6397_AUXADC_ISENSE) {
> +		ret = mt6397_auxadc_isense_enable(adc);
> +		if (ret)
> +			goto out_release;
> +
> +		fsleep(MT6397_AUXADC_ISENSE_SETTLE_US);
> +	} else {
> +		ret = mt6397_auxadc_battemp_bias_on(adc);
> +		if (ret)
> +			goto out_release;
> +
> +		fsleep(MT6397_AUXADC_BATTEMP_SETTLE_US);
> +	}
> +
> +	ret = mt6397_auxadc_read_burst(adc, chan, val);
> +
> +out_release:
> +	mt6397_auxadc_release(adc, chan);
This ends up rather confusing.  Would you mind instead doing it as a pair of helpers
and a top level bit of code that does.

	guard(mutex)(&adc->lock);
	if (chan->channel == MT6397_AUXADC_ISENSE) {
		return mt6397_auxadc_isense_read();
	else
		return mt6397_auxadc_battemp_read();


The only thing that get duplicated is the call to read_burst()
which seems simpler than having a pair of if / else on the channel
(one hidden in this release).

> +	mutex_unlock(&adc->lock);
> +
> +	return ret;
> +}