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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Thu, Sep 17, 2026 at 09:19:51PM -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.

...

The below should be part of the comment block, no commit message needs to
be polluted with this.

> A new driver was created here, instead of modifying an existing driver
> such as mt6323-auxadc or mt6359-auxadc, for the following reasons:
> 
>  - 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).
> 
>  - 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.
> 
> Assisted-by: LLM
> Signed-off-by: Ryan Brue <ryanbrue.dev@gmail.com>
> ---

...here is the comment block...

>  MAINTAINERS                     |   1 +
>  drivers/iio/adc/Kconfig         |  11 ++
>  drivers/iio/adc/Makefile        |   1 +

...

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

Make it two a bit long lines rather than four.

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

Does it need any settling timeout (in case it was set before)?

> +	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)

There is nothing common between on==false and on==true cases. Make it two
distinct functions and drop bool parameter. It's actually a recommended
pattern.

> +{
> +	struct regmap *map = adc->regmap;
> +	int ret, err;
> +
> +	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;
> +
> +		ret = regmap_set_bits(map, MT6397_CHR_CON7,
> +				      MT6397_CHR_CON7_BATON_TDET_EN);
> +	} else {
> +		/*
> +		 * Every step of the teardown is attempted even if an earlier
> +		 * one failed, so that one failing write cannot leave the bias
> +		 * or the input buffer powered. The first error is reported.
> +		 */
> +		ret = regmap_clear_bits(map, MT6397_CHR_CON7,
> +					MT6397_CHR_CON7_BATON_TDET_EN);
> +
> +		err = regmap_clear_bits(map, MT6397_AUXADC_CON0,
> +					MT6397_AUXADC_CON0_BUF_PWD_B);
> +		if (!ret)
> +			ret = err;
> +
> +		err = regmap_clear_bits(map, MT6397_AUXADC_CON0,
> +					MT6397_AUXADC_CON0_BUF_PWD_ON);
> +		if (!ret)
> +			ret = err;

These 'if (!ret)' bug me. What can we do if the ret == 0 and err != 0
on the caller's level? In other words, what can caller do in such a case?

> +	}
> +
> +	return ret;
> +}

...

> +static int mt6397_auxadc_isense_disable(struct mt6397_auxadc *adc)
> +{
> +	struct regmap *map = adc->regmap;
> +	int ret, err;
> +
> +	/* As above: the routing is undone even if the select write failed. */
> +	ret = regmap_clear_bits(map, MT6397_AUXADC_CON14,
> +				MT6397_AUXADC_CON14_CH0_NORM_SEL |
> +				MT6397_AUXADC_CON14_CH0_LBAT_SEL);
> +
> +	err = regmap_clear_bits(map, MT6397_CHR_CON16,
> +				MT6397_CHR_CON16_ADCIN_VSEN_EN |
> +				MT6397_CHR_CON16_ADCIN_VBAT_EN);
> +	if (!ret)
> +		ret = err;
> +
> +	return ret;

This can be written as

	if (ret)
		return ret;

	return err;


But the same Q as per above remains.

> +}

...

> +static int mt6397_auxadc_read_channel(struct mt6397_auxadc *adc,
> +				      const struct iio_chan_spec *chan,
> +				      int *val)
> +{
> +	bool isense = chan->channel == MT6397_AUXADC_ISENSE;
> +	unsigned int sum = 0;
> +	int sample;
> +	int ret;

> +	/* Held across the whole burst: the channel select is shared state. */

Unneeded comment. It's obvious that guard()() takes the whole scope.

> +	guard(mutex)(&adc->lock);
> +
> +	/*
> +	 * Once any part of the per-channel setup has been written, the
> +	 * teardown has to run, so every exit below goes through it.
> +	 */
> +	if (isense) {
> +		ret = mt6397_auxadc_isense_enable(adc);
> +		if (ret)
> +			goto out_teardown;

Have you compiled this?

> +		fsleep(MT6397_AUXADC_ISENSE_SETTLE_US);
> +	} else {
> +		ret = mt6397_auxadc_battemp_bias(adc, true);
> +		if (ret)
> +			goto out_teardown;
> +		fsleep(MT6397_AUXADC_BATTEMP_SETTLE_US);
> +	}
> +
> +	for (unsigned int i = 0; i < MT6397_AUXADC_SAMPLES; i++) {
> +		ret = mt6397_auxadc_read_once(adc, chan, &sample);
> +		if (ret)
> +			goto out_teardown;
> +
> +		sum += sample;
> +	}
> +
> +	*val = DIV_ROUND_CLOSEST(sum, MT6397_AUXADC_SAMPLES);
> +
> +out_teardown:
> +	/* Lower START so the converter is not left armed between reads. */
> +	regmap_clear_bits(adc->regmap, MT6397_AUXADC_CON1,
> +			  MT6397_AUXADC_CON1_START);
> +
> +	if (isense)
> +		mt6397_auxadc_isense_disable(adc);
> +	else
> +		mt6397_auxadc_battemp_bias(adc, false);
> +
> +	return ret;
> +}

So, this function has to refactored. And please, compile and test the code
*each* time you update it.

...

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

Make it if-else.

> +		*val2 = 10;
> +
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}

-- 
With Best Regards,
Andy Shevchenko