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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aFlk-l5LhgO8dnXK@smile.fi.intel.com>
Date: Mon, 23 Jun 2025 17:30:18 +0300
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
Cc: jic23@...nel.org, dlechner@...libre.com, nuno.sa@...log.com,
	andy@...nel.org, robh@...nel.org, krzk+dt@...nel.org,
	conor+dt@...nel.org, matthias.bgg@...il.com,
	linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org, kernel@...labora.com
Subject: Re: [PATCH v1 4/5] iio: adc: mt6359: Add support for MediaTek MT6363
 PMIC AUXADC

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