Re: [PATCH v5 23/27] clk: mediatek: Add MT8196 disp-ao clock support

["Jason-JH Lin (林睿祥)" <Jason-JH.Lin@mediatek.com>]

On Fri, 2025-08-29 at 11:19 +0200, Laura Nao wrote:
> Add support for the MT8196 disp-ao clock controller, which provides
> clock gate control for the display system. It is integrated with the
> mtk-mmsys driver, which registers the disp-ao clock driver via
> platform_device_register_data().
> 
> Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> Reviewed-by: AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com>
> Signed-off-by: Laura Nao <laura.nao@collabora.com>
> ---
>  drivers/clk/mediatek/Makefile              |  2 +-
>  drivers/clk/mediatek/clk-mt8196-vdisp_ao.c | 80
> ++++++++++++++++++++++
>  2 files changed, 81 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/clk/mediatek/clk-mt8196-vdisp_ao.c
> 
> diff --git a/drivers/clk/mediatek/Makefile
> b/drivers/clk/mediatek/Makefile
> index fe5699411d8b..5b8969ff1985 100644
> --- a/drivers/clk/mediatek/Makefile
> +++ b/drivers/clk/mediatek/Makefile
> @@ -157,7 +157,7 @@ obj-$(CONFIG_COMMON_CLK_MT8196_IMP_IIC_WRAP) +=
> clk-mt8196-imp_iic_wrap.o
>  obj-$(CONFIG_COMMON_CLK_MT8196_MCUSYS) += clk-mt8196-mcu.o
>  obj-$(CONFIG_COMMON_CLK_MT8196_MDPSYS) += clk-mt8196-mdpsys.o
>  obj-$(CONFIG_COMMON_CLK_MT8196_MFGCFG) += clk-mt8196-mfg.o
> -obj-$(CONFIG_COMMON_CLK_MT8196_MMSYS) += clk-mt8196-disp0.o clk-
> mt8196-disp1.o
> +obj-$(CONFIG_COMMON_CLK_MT8196_MMSYS) += clk-mt8196-disp0.o clk-
> mt8196-disp1.o clk-mt8196-vdisp_ao.o
>  obj-$(CONFIG_COMMON_CLK_MT8196_PEXTPSYS) += clk-mt8196-pextp.o
>  obj-$(CONFIG_COMMON_CLK_MT8196_UFSSYS) += clk-mt8196-ufs_ao.o
>  obj-$(CONFIG_COMMON_CLK_MT8365) += clk-mt8365-apmixedsys.o clk-
> mt8365.o
> diff --git a/drivers/clk/mediatek/clk-mt8196-vdisp_ao.c
> b/drivers/clk/mediatek/clk-mt8196-vdisp_ao.c
> new file mode 100644
> index 000000000000..fddb69d1c3eb
> --- /dev/null
> +++ b/drivers/clk/mediatek/clk-mt8196-vdisp_ao.c
> @@ -0,0 +1,80 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2025 MediaTek Inc.
> + *                    Guangjie Song <guangjie.song@mediatek.com>
> + * Copyright (c) 2025 Collabora Ltd.
> + *                    Laura Nao <laura.nao@collabora.com>
> + */
> +#include <dt-bindings/clock/mediatek,mt8196-clock.h>
> +
> +#include <linux/clk-provider.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +
> +#include "clk-gate.h"
> +#include "clk-mtk.h"
> +
> +static const struct mtk_gate_regs mm_v_cg_regs = {
> +	.set_ofs = 0x104,
> +	.clr_ofs = 0x108,
> +	.sta_ofs = 0x100,
> +};
> +
> +static const struct mtk_gate_regs mm_v_hwv_regs = {
> +	.set_ofs = 0x0030,
> +	.clr_ofs = 0x0034,
> +	.sta_ofs = 0x2c18,
> +};
> +
> +#define GATE_MM_AO_V(_id, _name, _parent, _shift) {	\
> +		.id = _id,				\
> +		.name = _name,				\
> +		.parent_name = _parent,			\
> +		.regs = &mm_v_cg_regs,			\
> +		.shift = _shift,			\
> +		.ops = &mtk_clk_gate_ops_setclr,	\
> +		.flags = CLK_OPS_PARENT_ENABLE |	\
> +			 CLK_IS_CRITICAL,		\
> +	}
> +
> +#define GATE_HWV_MM_V(_id, _name, _parent, _shift) {	\
> +		.id = _id,				\
> +		.name = _name,				\
> +		.parent_name = _parent,			\
> +		.regs = &mm_v_cg_regs,			\
> +		.hwv_regs = &mm_v_hwv_regs,		\
> +		.shift = _shift,			\
> +		.ops = &mtk_clk_gate_hwv_ops_setclr,	\
> +		.flags = CLK_OPS_PARENT_ENABLE,		\
> +	}
> +
> +static const struct mtk_gate mm_v_clks[] = {
> +	GATE_HWV_MM_V(CLK_MM_V_DISP_VDISP_AO_CONFIG,
> "mm_v_disp_vdisp_ao_config", "disp", 0),
> +	GATE_HWV_MM_V(CLK_MM_V_DISP_DPC, "mm_v_disp_dpc", "disp",
> 16),
> +	GATE_MM_AO_V(CLK_MM_V_SMI_SUB_SOMM0, "mm_v_smi_sub_somm0",
> "disp", 2),
> +};
> +
> +static const struct mtk_clk_desc mm_v_mcd = {
> +	.clks = mm_v_clks,
> +	.num_clks = ARRAY_SIZE(mm_v_clks),
> +};
> +
> +static const struct of_device_id of_match_clk_mt8196_vdisp_ao[] = {
> +	{ .compatible = "mediatek,mt8196-vdisp-ao", .data =
> &mm_v_mcd },

Hi Laura,

We are going to send mtk-mmsys driver for MT8196 recently, but we found
the compatible name is used here.

As your commit message, vdisp-ao is integrated with the mtk-mmsys
driver, which registers the vdisp-ao clock driver via 
platform_device_register_data().

Shouldn't this compatible name belong to mmsys driver for MT8196?

> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, of_match_clk_mt8196_vdisp_ao);
> +
> +static struct platform_driver clk_mt8196_vdisp_ao_drv = {
> +	.probe = mtk_clk_pdev_probe,
> +	.remove = mtk_clk_pdev_remove,
> +	.driver = {
> +		.name = "clk-mt8196-vdisp-ao",
> +		.of_match_table = of_match_clk_mt8196_vdisp_ao,

In clk-mt8196-ovl0/ovl1/disp0/disp1.c, they use `.id_table` instead of
`.of_match_table` here. Shouldn't this align to them?

Regards,
Jason-JH Lin

> +	},
> +};
> +module_platform_driver(clk_mt8196_vdisp_ao_drv);
> +
> +MODULE_DESCRIPTION("MediaTek MT8196 vdisp_ao clocks driver");
> +MODULE_LICENSE("GPL");