Re: [PATCH v1 4/5] iio: adc: mt6359: Add support for MediaTek MT6363 PMIC AUXADC

[AngeloGioacchino Del Regno]

Il 23/06/25 16:30, Andy Shevchenko ha scritto:
> On Mon, Jun 23, 2025 at 02:00:27PM +0200, AngeloGioacchino Del Regno wrote:
> > MediaTek MT6363 is a PMIC found on MT8196/MT6991 board designs
> > and communicates with the SoC over SPMI.
> >
> > This PMIC integrates an Auxiliary ADC (AUXADC) which has a grand
> > total of 54 ADC channels: 49 PMIC-internal channels, 2 external
> > NTC thermistor channels and 2 generic ADC channels (mapped to 7
> > PMIC ADC external inputs).
> >
> > To use a generic ADC channel it is necessary to enable one of
> > the PMIC ADC inputs at a time and only then start the reading,
> > so in this case it is possible to read only one external input
> > for each generic ADC channel.
> >
> > Due to the lack of documentation, this implementation supports
> > using only one generic ADC channel, hence supports reading only
> > one external input at a time.
>
> > +#define MT6363_EXT_CHAN_MASK		GENMASK(2, 0)
> > +#define MT6363_EXT_PURES_MASK		GENMASK(4, 3)
> > + #define MT6363_PULLUP_RES_100K		0
> > + #define MT6363_PULLUP_RES_OPEN		3
>
> I would rather expect the two spaces after #define. This most likely will break
> syntax highlighting in (some of) the editors.

I can change that no problem (or if this can be changed while applying, that'd
buy me some time and I'd appreciate that a lot)

> ...
>
> > +#define MTK_PMIC_ADC_EXT_CHAN(_ch_idx, _req_idx, _req_bit, _rdy_idx, _rdy_bit,	\
> > +			      _ext_sel_idx, _ext_sel_ch, _ext_sel_pu,		\
> > +			      _samples, _rnum, _rdiv)				\
>
> Wondering, and it's out of scope here, if we can go to use a macro for
> initialization of struct *_fract.
>
> >   	[PMIC_AUXADC_CHAN_##_ch_idx] = {					\
> >   		.req_idx = _req_idx,						\
> >   		.req_mask = BIT(_req_bit),					\
> >   		.rdy_idx = _rdy_idx,						\
> >   		.rdy_mask = BIT(_rdy_bit),					\
> > +		.ext_sel_idx = _ext_sel_idx,					\
> > +		.ext_sel_ch = _ext_sel_ch,					\
> > +		.ext_sel_pu = _ext_sel_pu,					\
> >   		.num_samples = _samples,					\
> >   		.r_ratio = { _rnum, _rdiv }					\
> >   	}
>
> Perhaps something in math.h as
>
> #define INIT_STRUCT_FRACT_UXX(n, d) ...

Not sure... honestly, at a first glance it looks like a macro would only make
a longer line and nothing else...

...but - effectively - I can see a benefit for a INIT_CONST_STRUCT_FRACT_Uxx(n, d)
where we could perform a build-time check for division by zero.

I'm not sure how many users would there be of such a macro, ideas?

> ...
>
> > +	if (MTK_AUXADC_HAS_FLAG(cinfo, IS_SPMI)) {
> > +		/* If the previous read succeeded, this can't fail */
> > +		regmap_read(regmap, reg - 1, &lval);
>
> No error check? lval may contain garbage here, right?

No, because if the previous read succeeded, this can't fail, and also cannot ever
possibly contain garbage (and if it does, - but again, that can't happen - there is
no way to validate that because valid values are [0x00..0xff] anyway).

> > +		val = (val << 8) | lval;
>
> Is it guaranteed that lval is always less than 256 (if unsigned)?

Yes, with SPMI that is guaranteed.

> > +	}
>
> ...
>
> > +		regmap_update_bits(regmap, cinfo->regs[desc->ext_sel_idx],
> > +				   MT6363_EXT_PURES_MASK, ext_sel);
>
> No  error check?

No, because if the previous reads and/or writes succeeded, it is impossible for
this to fail :-)

Cheers,
Angelo