[PATCH v2 2/4] iio: adc: mt6323-auxadc: add mt6323 PMIC AUXADC driver

[Roman Vivchar]

Hi Jonathan,

On Sunday, June 14th, 2026 at 8:22 PM, Jonathan Cameron <jic23 at kernel.org> wrote:
> On Tue, 09 Jun 2026 16:31:59 +0300
> Roman Vivchar via B4 Relay <devnull+rva333.protonmail.com at kernel.org> wrote:

...
 
> > +
> > +#define MTK_PMIC_IIO_CHAN(_name, _chan, _addr)                  \
> > +{                                                               \
> > +	.type = IIO_VOLTAGE,                                    \
> > +	.indexed = 1,                                           \
> > +	.channel = _chan,                                       \
> > +	.address = _addr,                                       \
> > +	.datasheet_name = __stringify(_name),                   \
> > +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |          \
> > +			      BIT(IIO_CHAN_INFO_SCALE),         \
> > +}
> > +
> > +static const struct iio_chan_spec mt6323_auxadc_channels[] = {
> > +	MTK_PMIC_IIO_CHAN(baton2,    MT6323_AUXADC_BATON2,    MT6323_AUXADC_ADC6),
> > +	MTK_PMIC_IIO_CHAN(ch6,       MT6323_AUXADC_CH6,       MT6323_AUXADC_ADC11),
> > +	MTK_PMIC_IIO_CHAN(bat_temp,  MT6323_AUXADC_BAT_TEMP,  MT6323_AUXADC_ADC5),
> 
> Reasonable query from Sashiko on why temperature channels are presented as voltages.
> If for some reason that is the right choice, then maybe a comment here.

mt6323 ADC always returns voltage. The thermal driver (which was in the
previous series and will be sent later) is required to map these to the
actual temperature. Ack.

...

> > +/*
> > + * The MediaTek MT6323 (as well as a lot of other PMICs) has the following hierarchy:
> > + * PMIC AUXADC <- PMIC MFD <- SoC PWRAP (wrapper for PWRAP FSM)
> > + *
> > + * Therefore, PWRAP regmap should be obtained using dev->parent->parent.
> > + */
> > +struct mt6323_auxadc {
> > +	struct regmap *regmap;
> > +	struct mutex lock;
> Locks should always have a comment on what data they are protecting.
> I think this one is about protecting the state of a device during a channel read
> by serializing those reads.

Nuno said kerneldoc looks unnecessary on v1 [1]. How the comment should
look?

...

> > +static int mt6323_auxadc_request(struct mt6323_auxadc *auxadc,
> > +				 unsigned long channel)
> > +{
> > +	struct regmap *map = auxadc->regmap;
> > +	int ret;
> > +
> > +	ret = regmap_set_bits(map, MT6323_AUXADC_CON11, AUXADC_CON11_VBUF_EN);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return regmap_set_bits(map, MT6323_AUXADC_CON22, BIT(channel));
> 
> I'm not sure whether the sashiko question on this is valid or not. Make sure to take
> a look.
> 
> https://sashiko.dev/#/patchset/20260609-mt6323-adc-v2-0-aa93a22309f9%40protonmail.com
> You may have carefully selected the numbering so the channel numbering matches
> the bits in this register.  If so, it is probably worth a comment in the header
> to provide a cross reference.  No idea if Sashiko will notice that, but at least
> humans should!

The hardware is pretty weird, but dt-bindings have correct numbers.
I have double checked with the vendor driver and the logic is the same.

'If regmap_set_bits() fails to set MT6323_AUXADC_CON22, does this leave the
AUXADC voltage buffer (VBUF) permanently enabled?' - if this happens,
then there's something really wrong with PWRAP and disabling VBUF may
not be possible. Same about the 'mt6323_auxadc_release' comment.

...

> > +	case IIO_CHAN_INFO_RAW:
> 
> What Andy suggested here is the preferred path in IIO at least.
> Mainly because it reduced indent without hurting readability.
> Just be careful to define the scope with { }

Ack.

> 
> 
> > +		scoped_guard(mutex, &auxadc->lock) {
> > +			ret = mt6323_auxadc_prepare_channel(auxadc);
> > +			if (ret)
> > +				return ret;
> > +
> > +			ret = mt6323_auxadc_request(auxadc, chan->channel);
> > +			if (ret)
> > +				return ret;
> > +
> > +			/* Hardware limitation: the AUXADC needs a delay to become ready. */
> > +			fsleep(300);
> > +
> > +			ret = mt6323_auxadc_read(auxadc, chan, val);
> > +
> > +			if (mt6323_auxadc_release(auxadc, chan->channel))
> > +				dev_err(&indio_dev->dev,
> > +					"failed to release channel %d\n", chan->channel);
> > +
> > +			if (ret)
> > +				return ret;
> > +		}
> > +		return IIO_VAL_INT;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> 
> 

After these changes, should I keep or drop Andy's Reviewed-by?

[1]: https://lore.kernel.org/linux-iio/2df4cad5e29fbcb4c5c5f59ea0bf322c7a301bdc.camel@gmail.com/

Best regards,
Roman