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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

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