Re: [PATCH 2/7] dt-bindings: mediatek: Add reset controller constants for mt8167

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

Il 11/02/26 20:03, Luca Leonardo Scorcia ha scritto:
> This file comes from the Lenovo Smart Clock kernel sources.
> 
> Signed-off-by: Luca Leonardo Scorcia <l.scorcia@gmail.com>
> ---
>   include/dt-bindings/reset/mt8167-resets.h | 38 +++++++++++++++++++++++

Please rename this to "mediatek,mt8167-resets.h"

>   1 file changed, 38 insertions(+)
>   create mode 100644 include/dt-bindings/reset/mt8167-resets.h
> 
> diff --git a/include/dt-bindings/reset/mt8167-resets.h b/include/dt-bindings/reset/mt8167-resets.h
> new file mode 100644
> index 000000000000..f77c2ca897cf
> --- /dev/null
> +++ b/include/dt-bindings/reset/mt8167-resets.h
> @@ -0,0 +1,38 @@
> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause) */
> +/*
> + * Copyright (c) 2015 MediaTek Inc.
> + *

End the comment here; the extra text is redundant with the specified license
identifier anyway.

> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef _DT_BINDINGS_RESET_CONTROLLER_MT8167
> +#define _DT_BINDINGS_RESET_CONTROLLER_MT8167
> +
> +/* TOPRGU resets */
> +#define MT8167_TOPRGU_DDRPHY_FLASH_RST		0 /* reset ddrphy and flash pad macro */

Please remove all of the extra comments: they're stating the obvious anyway.

> +#define MT8167_TOPRGU_AUD_PAD_RST		1 /* Write 1 to reset audio_tdm_in_pad,audio_tdm_pad,audio_fifo */
> +#define MT8167_TOPRGU_MM_RST			2 /* Write 1 to reset MMSYS */
> +#define MT8167_TOPRGU_MFG_RST			3 /* Write 1 to reset MFG */
> +#define MT8167_TOPRGU_MDSYS_RST			4 /* Write 1 to reset INFRA_AO */
> +#define MT8167_TOPRGU_CONN_RST			5 /* Write 1 to reset CONNSYS WDT reset */
> +#define MT8167_TOPRGU_PAD2CAM_DIG_MIPI_RX_RST	6 /* Write 1 to reset MM and its related pad macro(DPI,MIPI_CFG,MIPI_TX) */
> +#define MT8167_TOPRGU_DIG_MIPI_TX_RST		7 /* Write 1 to reset digi_mipi_tx */
> +#define MT8167_TOPRGU_SPI_PAD_MACRO_RST		8 /* Write 1 to reset SPI_PAD_MACRO */

You're missing reset N.9 - I'm mostly sure that 9 is

MT8167_TOPRGU_INFRA_AO_RST

> +#define MT8167_TOPRGU_APMIXED_RST		10 /* Write 1 to reset APMIXEDSYS */
> +#define MT8167_TOPRGU_VDEC_RST			11 /* Write 1 to reset VDEC module */
> +#define MT8167_TOPRGU_CONN_MCU_RST		12 /* Write 1 to reset CONNSYS */
> +#define MT8167_TOPRGU_EFUSE_RST			13 /* Write 1 to reset efuse */
> +#define MT8167_TOPRGU_PWRAP_SPICTL_RST		14 /* Write 1 to reset pwrap_spictl module */
> +#define MT8167_TOPRGU_SW_RST_NUM		15
> +
> +/* MMSYS resets */
> +#define MT8167_MMSYS_SW0_RST_B_DISP_DSI0	22

That's not ok. All of the bindings must not be hardware specific, and I know
that this "22" is bit(22) of MMSYS.

The definitions must start from 0 and must have no holes (0 1 2 4 6 7 8 is not ok,
but 0 1 2 3 4 5 is ok).
If you have the entire list and it also matches 1:1 with the HW coincidentally,
that'd be okay as well.

Since I have a hunch that you don't actually have the entire list, you should
at this point define your MT8167_SW0_RST_B_DISP_DSI0 as 0, and check the review
on your mmsys patch for more hints.

P.S.:

#define MT8167_MMSYS_SW0_RST_B_SMI_COMMON	0
#define MT8167_MMSYS_SW0_RST_B_SMI_LARB		1
#define MT8167_MMSYS_SW0_RST_B_CAM_MDP		2
#define MT8167_MMSYS_SW0_RST_B_MDP_RDMA0	3
#define MT8167_MMSYS_SW0_RST_B_MDP_RSZ0		4
#define MT8167_MMSYS_SW0_RST_B_MDP_RSZ1		5
#define MT8167_MMSYS_SW0_RST_B_MDP_TDSHP0	6
#define MT8167_MMSYS_SW0_RST_B_MDP_WDMA		7
#define MT8167_MMSYS_SW0_RST_B_MDP_WROT0	8
#define MT8167_MMSYS_SW0_RST_B_DISP_DSI0	9

> +
> +#endif  /* _DT_BINDINGS_RESET_CONTROLLER_MT8167 */