Re: [PATCH 2/3] soc:mediatek mt8189: Porting driver for spmi/pwrap

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

Il 01/08/25 08:39, niklaus.liu ha scritto:
> Modify spmi/pwrap driver for mt8189
> 
> Signed-off-by: niklaus.liu <niklaus.liu@mediatek.com>
> ---
>   drivers/soc/mediatek/mtk-pmic-wrap.c | 27 +++++++++++++++++++++++++++
>   drivers/spmi/spmi-mtk-pmif.c         |  3 +++
>   2 files changed, 30 insertions(+)
> 
> diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
> index 0bcd85826375..e3e8234e29a0 100644
> --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
> +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
> @@ -1087,6 +1087,17 @@ static const int mt8183_regs[] = {
>   	[PWRAP_WACS2_VLDCLR] =			0xC28,
>   };
>   
> +static int mt8189_regs[] = {
> +	[PWRAP_INIT_DONE2] =		0x0,
> +	[PWRAP_TIMER_EN] =		0x3e4,
> +	[PWRAP_INT_EN] =		0x450,
> +	[PWRAP_WACS2_CMD] =		0x880,
> +	[PWRAP_SWINF_2_WDATA_31_0] =	0x884,
> +	[PWRAP_SWINF_2_RDATA_31_0] =	0x894,
> +	[PWRAP_WACS2_VLDCLR] =		0x8a4,
> +	[PWRAP_WACS2_RDATA] =		0x8a8,
> +};

You can fully reuse mt8195_regs, as mt8189 has the same layout.

> +
>   static const int mt8195_regs[] = {
>   	[PWRAP_INIT_DONE2] =		0x0,
>   	[PWRAP_STAUPD_CTRL] =		0x4C,
> @@ -1324,6 +1335,7 @@ enum pwrap_type {
>   	PWRAP_MT8173,
>   	PWRAP_MT8183,
>   	PWRAP_MT8186,
> +	PWRAP_MT8189,
>   	PWRAP_MT8195,
>   	PWRAP_MT8365,
>   	PWRAP_MT8516,
> @@ -1854,6 +1866,7 @@ static int pwrap_init_cipher(struct pmic_wrapper *wrp)
>   		break;
>   	case PWRAP_MT6873:
>   	case PWRAP_MT8183:
> +	case PWRAP_MT8189:
>   	case PWRAP_MT8195:
>   		break;
>   	}
> @@ -2393,6 +2406,19 @@ static const struct pmic_wrapper_type pwrap_mt8183 = {
>   	.init_soc_specific = pwrap_mt8183_init_soc_specific,
>   };
>   
> +static struct pmic_wrapper_type pwrap_mt8189 = {
> +	.regs = mt8189_regs,
> +	.type = PWRAP_MT8189,
> +	.arb_en_all = 0x777f,
> +	.int_en_all = 0x180000,
> +	.int1_en_all = 0,
> +	.spi_w = PWRAP_MAN_CMD_SPI_WRITE,
> +	.wdt_src = PWRAP_WDT_SRC_MASK_ALL,
> +	.caps = PWRAP_CAP_ARB,

Why are you avoiding to enable the INT1 interrupt on MT8189?

Is this working around a hardware bug, or did you simply forget to enable it?
I think you should really enable it, which means....

> +	.init_reg_clock = pwrap_common_init_reg_clock,
> +	.init_soc_specific = NULL,
> +};
> +
>   static const struct pmic_wrapper_type pwrap_mt8195 = {
>   	.regs = mt8195_regs,
>   	.type = PWRAP_MT8195,
> @@ -2456,6 +2482,7 @@ static const struct of_device_id of_pwrap_match_tbl[] = {
>   	{ .compatible = "mediatek,mt8173-pwrap", .data = &pwrap_mt8173 },
>   	{ .compatible = "mediatek,mt8183-pwrap", .data = &pwrap_mt8183 },
>   	{ .compatible = "mediatek,mt8186-pwrap", .data = &pwrap_mt8186 },
> +	{ .compatible = "mediatek,mt8189-pwrap", .data = &pwrap_mt8189 },

...means that you don't even need to add a new compatible in this list, because
the MT8195 compatible can be reused.

You only have to add the MT8189 compatible to the bindings, so that you are
allowed to specify in your devicetree node

compatible = "mediatek,mt8189-pwrap", "mediatek,mt8195-pwrap";

>   	{ .compatible = "mediatek,mt8195-pwrap", .data = &pwrap_mt8195 },
>   	{ .compatible = "mediatek,mt8365-pwrap", .data = &pwrap_mt8365 },
>   	{ .compatible = "mediatek,mt8516-pwrap", .data = &pwrap_mt8516 },
> diff --git a/drivers/spmi/spmi-mtk-pmif.c b/drivers/spmi/spmi-mtk-pmif.c
> index 160d36f7d238..00420568afef 100644
> --- a/drivers/spmi/spmi-mtk-pmif.c
> +++ b/drivers/spmi/spmi-mtk-pmif.c
> @@ -530,6 +530,9 @@ static const struct of_device_id mtk_spmi_match_table[] = {
>   	{
>   		.compatible = "mediatek,mt6873-spmi",
>   		.data = &mt6873_pmif_arb,
> +	}, {
> +		.compatible = "mediatek,mt8189-spmi",
> +		.data = &mt8195_pmif_arb,

This change is useless. Just add the compatible to the bindings so that you
can specify

compatible = "mediatek,mt8189-spmi", "mediatek,mt8195-spmi";

Regards,
Angelo