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

[Andy Shevchenko]

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.

...

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

...

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

> +		val = (val << 8) | lval;

Is it guaranteed that lval is always less than 256 (if unsigned)?

> +	}

...

> +		regmap_update_bits(regmap, cinfo->regs[desc->ext_sel_idx],
> +				   MT6363_EXT_PURES_MASK, ext_sel);

No  error check?

> +	}

-- 
With Best Regards,
Andy Shevchenko