Re: [PATCH 04/13] iio: adc: mediatek: add mt6323 PMIC AUXADC driver

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 04 May 2026 21:24:56 +0300
Roman Vivchar via B4 Relay <devnull+rva333.protonmail.com@kernel.org> wrote:

> From: Roman Vivchar <rva333@protonmail.com>
> 
> The mt6323 AUXADC is a 15-bit ADC used for system monitoring. This driver
> provides support for reading various channels including battery and
> charger voltages, battery and chip temperature, current sensing and
> accessory detection.
> 
> Add a driver for the AUXADC found in the MediaTek mt6323 PMIC.
> 
> Tested-by: Ben Grisdale <bengris32@protonmail.ch> # Amazon Echo Dot (2nd Generation)
> Signed-off-by: Roman Vivchar <rva333@protonmail.com>

A few things inline that might not entirely overlap with other reviews

Also make sure to check:
https://sashiko.dev/#/patchset/20260504-mt6323-v1-0-799b58b355ff%40protonmail.com
It may well have made some stuff up, but in general it does find stuff
humans have missed. I've called out a few more interesting ones inline
(I looked after doing my initial review)


Thanks,

Jonathan

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

> +/**
> + * struct mt6323_auxadc - Main driver structure
> + * @dev:           Device pointer
> + * @regmap:        Regmap from PWRAP
> + * @lock:          Mutex to serialize AUXADC reading vs configuration
> + */
> +struct mt6323_auxadc {
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct mutex lock;
> +};

> +static int mt6323_auxadc_check_if_stuck(struct mt6323_auxadc *auxadc)
> +{
> +	int i, ret;
> +	u32 val;
> +
> +	for (i = 0; i < 50; i++) {
> +		ret = regmap_read(auxadc->regmap, MT6323_AUXADC_CON19, &val);
> +		if (ret)
> +			return ret;
> +
> +		if (FIELD_GET(AUXADC_DECI_GDLY_MASK, val)) {
Flip to reduce indent.
		if (!FIELD_GET(AUXADC_DECI_GDLY_MASK, val))
			return 0;

		ret = regmap_read(auxadc->regmap, MT6323_AUXADC_ADC19, &val);
		if (ret)
			return ret;

		if (!FIELD_GET(AUXADC_ADC19_BUSY_MASK, val)) {
			ret = regmap_update_bits(auxadc->regmap,
						 MT6323_AUXADC_CON19,
						 FIELD_PREP(AUXADC_DECI_GDLY_MASK, 3),
// no idea what the magic 3 is. Might need a define to make that clear.
// fine to go a bit long on code like this for readability.  Also rather
// odd for a regmap mask to be made up of some bits of a larger mask.
// rather implies that a field needs breaking up into two.

						 0x0);
			if (ret)
				return ret;
		}
		fsleep(10);
	}

> +			ret = regmap_read(auxadc->regmap, MT6323_AUXADC_ADC19,
> +					  &val);
> +			if (ret)
> +				return ret;
> +
> +			if (!FIELD_GET(AUXADC_ADC19_BUSY_MASK, val)) {
> +				ret = regmap_update_bits(
> +					auxadc->regmap, MT6323_AUXADC_CON19,
> +					FIELD_PREP(AUXADC_DECI_GDLY_MASK, 3),
> +					0x0);
> +				if (ret)
> +					return ret;
> +			}
> +		} else {
> +			return 0;
> +		}
> +
> +		fsleep(10);
> +	}
> +
> +	return -ETIMEDOUT;
> +}
> +
> +static int mt6323_auxadc_request(struct mt6323_auxadc *auxadc,
> +				 unsigned long channel)
> +{
> +	int ret;
> +	u32 pmic_val, adc_val;
> +
> +	if (channel < 9) {
> +		ret = regmap_update_bits(auxadc->regmap, MT6323_AUXADC_CON11,
> +					 AUXADC_VBUF_EN, AUXADC_VBUF_EN);
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_read(auxadc->regmap, MT6323_AUXADC_CON22,
> +				  &pmic_val);
> +		if (ret)
> +			return ret;
> +
> +		adc_val = FIELD_GET(AUXADC_LOW_CHANNEL_MASK, pmic_val);
> +		adc_val &= ~BIT(channel);
> +
> +		ret = regmap_update_bits(auxadc->regmap, MT6323_AUXADC_CON22,
> +					 AUXADC_LOW_CHANNEL_MASK, adc_val);

Given you have the full register value I'd do regmap_write() rather than
update_bits() that may trigger another read. Matters less if you having
caching enabled.

> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_read(auxadc->regmap, MT6323_AUXADC_CON22,
> +				  &pmic_val);
> +		if (ret)
> +			return ret;
> +
> +		adc_val = FIELD_GET(AUXADC_LOW_CHANNEL_MASK, pmic_val);
> +		adc_val |= BIT(channel);
> +
> +		ret = regmap_update_bits(auxadc->regmap, MT6323_AUXADC_CON22,
> +					 AUXADC_LOW_CHANNEL_MASK, adc_val);

as above for a full write.

		return regmap_update_bits() so we don't have to read on
to see what else happens.  You could drop the else but I think it
does have some value in showing they are of 'equal' likelihood.

> +
> +	} else {
Sashiko points out that we only have channels 0-9 defined, so how do
we get here?

> +		ret = regmap_read(auxadc->regmap, MT6323_AUXADC_CON23,
> +				  &pmic_val);
> +		if (ret)
> +			return ret;
> +
> +		adc_val = FIELD_GET(AUXADC_AUDIO_CHANNEL_MASK, pmic_val);
> +		adc_val &= ~BIT(channel - 9);
> +
> +		ret = regmap_update_bits(auxadc->regmap, MT6323_AUXADC_CON23,
> +					 AUXADC_AUDIO_CHANNEL_MASK, adc_val);
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_read(auxadc->regmap, MT6323_AUXADC_CON23,
> +				  &pmic_val);
> +		if (ret)
> +			return ret;
> +
> +		adc_val = FIELD_GET(AUXADC_AUDIO_CHANNEL_MASK, pmic_val);
> +		adc_val |= BIT(channel - 9);
> +
> +		ret = regmap_update_bits(auxadc->regmap, MT6323_AUXADC_CON23,
> +					 AUXADC_AUDIO_CHANNEL_MASK, adc_val);
return regmap_update_bits() here as well because we might as well.

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

> +static int mt6323_auxadc_read_raw(struct iio_dev *indio_dev,
> +				  const struct iio_chan_spec *chan, int *val,
> +				  int *val2, long mask)
> +{
> +	struct mt6323_auxadc *auxadc = iio_priv(indio_dev);
> +	int ret, mult = 1;

Move declaration of mult into the scope where it's used.
That will mean this default is near the point of use. Mind you
I'd probably but the assignment in and else to make it even
more obvious.

> +
> +	if (mask == IIO_CHAN_INFO_RAW) {
> +		scoped_guard(mutex, &auxadc->lock)
> +		{
		guard(mutex)(&auxadc->lock);
preferred where not necessary to use scoped_guard() as it
means we don't end up with large code indents.

Though if you did { on the line above.

As sashiko notes, this will trip up compilers anyway due
to an oddity of how scoped_guard() is implemented. You have
to have a return after it.

> +			ret = mt6323_auxadc_check_if_stuck(auxadc);
> +			if (ret)
> +				return ret;
> +
> +			ret = mt6323_auxadc_request(auxadc, chan->address);
> +			if (ret)
> +				return ret;
> +
> +			usleep_range(300, 500);

Smells like fsleep() is appropriate.

> +
> +			ret = mt6323_auxadc_read(auxadc, chan, val);
> +			if (ret)
> +				return ret;
> +			return IIO_VAL_INT;
> +		}
> +	} else if (mask == IIO_CHAN_INFO_SCALE) {
> +		if (chan->channel == MT6323_AUXADC_ISENSE ||
> +		    chan->address == MT6323_AUXADC_BATSNS)
Probably pick channel or address.  Mind you not much point in
setting them both to the same thing (another one sashiko got
that I missed)

> +			mult = 4;
> +
> +		*val = mult * 1800;
> +		*val2 = 32768;
> +
> +		return IIO_VAL_FRACTIONAL;
> +	} else 

{} for all legs if any need it.

> +		return -EINVAL;
> +}
> +
> +static int mt6323_auxadc_init(struct mt6323_auxadc *auxadc)
> +{

> +
> +	ret = regmap_update_bits(auxadc->regmap, MT6323_AUXADC_CON9,
> +				 AUXADC_OSR_MASK,
> +				 FIELD_PREP(AUXADC_OSR_MASK, 3));

I'm going to guess something to do with sampling rate?  That 3 is non
obvious so I'd suggest defines for the values AUXADC_OSR can take.

> +	return ret;
> +}