Re: [PATCH v2 2/4] soc: mediatek: pwrap: Add support for MT6735 and MT6328 SoC/PMIC pair

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] PMIC_MT6328 is missing from the slave cipher initialization switch statement, which will cause the driver probe to time out and fail.
- [Medium] Unsafe write to PMIC register 0x0 if MT6735 is paired with a PMIC lacking `PWRAP_DEW_RDDMY_NO`.
--

commit 21ea916ac0163eee8daf299b310759cc6d675f2a
Author: Yassine Oudjana <y.oudjana@protonmail.com>

soc: mediatek: pwrap: Add support for MT6735 and MT6328 SoC/PMIC pair

This commit adds register definitions and configuration for the MT6735 SoC
and the MT6328 PMIC to the MediaTek PMIC wrapper driver. It defines their
specific register layouts and capabilities to enable communication between
the two chips.

> diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
> index 0bcd858263755..0f14dca5609d1 100644
> --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
> +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
[ ... ]
> @@ -1733,6 +1877,10 @@ static void pwrap_init_chip_select_ext(struct pmic_wrapper *wrp, u8 hext_write,
>  static int pwrap_common_init_reg_clock(struct pmic_wrapper *wrp)
>  {
>  	switch (wrp->master->type) {
> +	case PWRAP_MT6735:
> +		pwrap_write(wrp, wrp->slave->dew_regs[PWRAP_DEW_RDDMY_NO], 0x8);

[Severity: Medium]
Will this write to register 0x0 if MT6735 is paired with a PMIC that lacks
PWRAP_DEW_RDDMY_NO?

If MT6735 is paired with a PMIC like MT6397 that doesn't define this register
in its array, the read will return 0 and the driver will write 0x8 to PMIC
register 0x0. Should this verify the PMIC type first, similar to how it is
handled for MT6795 below?

> +		pwrap_init_chip_select_ext(wrp, 0, 6, 0, 0);
> +		break;
>  	case PWRAP_MT6795:
>  		if (wrp->slave->type == PMIC_MT6331) {
[ ... ]
> @@ -2194,6 +2343,14 @@ static const struct pwrap_slv_type pmic_mt6323 = {
>  		PWRAP_SLV_CAP_SECURITY,
>  };
>  
> +static const struct pwrap_slv_type pmic_mt6328 = {
> +	.dew_regs = mt6328_regs,
> +	.type = PMIC_MT6328,
> +	.regops = &pwrap_regops16,
> +	.caps = PWRAP_SLV_CAP_SPI | PWRAP_SLV_CAP_DUALIO |
> +		PWRAP_SLV_CAP_SECURITY,

[Severity: High]
Does pwrap_init_cipher() need to be updated to handle PMIC_MT6328?

Since PMIC_MT6328 has PWRAP_SLV_CAP_SECURITY, pwrap_init_cipher() will run.
However, PMIC_MT6328 is missing from the switch (wrp->slave->type) block in
pwrap_init_cipher().

Without writing 1 to PWRAP_DEW_CIPHER_EN for this PMIC, the cipher engine
remains disabled, which will cause the subsequent readx_poll_timeout() for
pwrap_is_pmic_cipher_ready to time out and fail the driver probe.

> +};
> +
>  static const struct pwrap_slv_type pmic_mt6331 = {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260531-mt6735-pwrap-mt6328-pmic-v2-0-dac37bf92894@protonmail.com?part=2