Subject: Re: [PATCH v2 08/19] clk: mediatek: Add MT8188 imgsys clock support

Il 24/10/22 11:42, Garmin.Chang ha scritto:
> Add MT8188 imgsys clock controllers which provide clock gate
> control for image IP blocks.
>
> Signed-off-by: Garmin.Chang <[email protected]>
> ---
> drivers/clk/mediatek/Makefile | 2 +-
> drivers/clk/mediatek/clk-mt8188-img.c | 124 ++++++++++++++++++++++++++
> 2 files changed, 125 insertions(+), 1 deletion(-)
> create mode 100644 drivers/clk/mediatek/clk-mt8188-img.c
>
> diff --git a/drivers/clk/mediatek/Makefile b/drivers/clk/mediatek/Makefile
> index bd0a2aa5b6fa..242b49bafa9e 100644
> --- a/drivers/clk/mediatek/Makefile
> +++ b/drivers/clk/mediatek/Makefile
> @@ -84,7 +84,7 @@ obj-$(CONFIG_COMMON_CLK_MT8186) += clk-mt8186-mcu.o clk-mt8186-topckgen.o clk-mt
> clk-mt8186-cam.o clk-mt8186-mdp.o clk-mt8186-ipe.o
> obj-$(CONFIG_COMMON_CLK_MT8188) += clk-mt8188-apmixedsys.o clk-mt8188-topckgen.o \
> clk-mt8188-peri_ao.o clk-mt8188-infra_ao.o \
> - clk-mt8188-cam.o clk-mt8188-ccu.o
> + clk-mt8188-cam.o clk-mt8188-ccu.o clk-mt8188-img.o
> obj-$(CONFIG_COMMON_CLK_MT8192) += clk-mt8192.o
> obj-$(CONFIG_COMMON_CLK_MT8192_AUDSYS) += clk-mt8192-aud.o
> obj-$(CONFIG_COMMON_CLK_MT8192_CAMSYS) += clk-mt8192-cam.o
> diff --git a/drivers/clk/mediatek/clk-mt8188-img.c b/drivers/clk/mediatek/clk-mt8188-img.c
> new file mode 100644
> index 000000000000..00f3bbf4d502
> --- /dev/null
> +++ b/drivers/clk/mediatek/clk-mt8188-img.c
> @@ -0,0 +1,124 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +//
> +// Copyright (c) 2022 MediaTek Inc.
> +// Author: Garmin Chang <[email protected]>
> +
> +#include <linux/clk-provider.h>
> +#include <linux/platform_device.h>
> +#include <dt-bindings/clock/mediatek,mt8188-clk.h>
> +
> +#include "clk-gate.h"
> +#include "clk-mtk.h"
> +
> +static const struct mtk_gate_regs imgsys_cg_regs = {
> + .set_ofs = 0x4,
> + .clr_ofs = 0x8,
> + .sta_ofs = 0x0,
> +};
> +
> +#define GATE_IMGSYS(_id, _name, _parent, _shift) \
> + GATE_MTK(_id, _name, _parent, &imgsys_cg_regs, _shift, &mtk_clk_gate_ops_setclr)
> +
> +static const struct mtk_gate imgsys_main_clks[] = {
> + GATE_IMGSYS(CLK_IMGSYS_MAIN_LARB9, "imgsys_main_larb9", "top_img", 0),
> + GATE_IMGSYS(CLK_IMGSYS_MAIN_TRAW0, "imgsys_main_traw0", "top_img", 1),
> + GATE_IMGSYS(CLK_IMGSYS_MAIN_TRAW1, "imgsys_main_traw1", "top_img", 2),
> + GATE_IMGSYS(CLK_IMGSYS_MAIN_VCORE_GALS, "imgsys_main_vcore_gals", "top_img", 3),
> + GATE_IMGSYS(CLK_IMGSYS_MAIN_DIP0, "imgsys_main_dip0", "top_img", 8),
> + GATE_IMGSYS(CLK_IMGSYS_MAIN_WPE0, "imgsys_main_wpe0", "top_img", 9),
> + GATE_IMGSYS(CLK_IMGSYS_MAIN_IPE, "imgsys_main_ipe", "top_img", 10),
> + GATE_IMGSYS(CLK_IMGSYS_MAIN_WPE1, "imgsys_main_wpe1", "top_img", 12),
> + GATE_IMGSYS(CLK_IMGSYS_MAIN_WPE2, "imgsys_main_wpe2", "top_img", 13),
> + GATE_IMGSYS(CLK_IMGSYS_MAIN_GALS, "imgsys_main_gals", "top_img", 31),
> +};
> +
> +static const struct mtk_gate imgsys_wpe1_clks[] = {
> + GATE_IMGSYS(CLK_IMGSYS_WPE1_LARB11, "imgsys_wpe1_larb11", "top_img", 0),
> + GATE_IMGSYS(CLK_IMGSYS_WPE1, "imgsys_wpe1", "top_img", 1),
> +};
> +
> +static const struct mtk_gate imgsys_wpe2_clks[] = {
> + GATE_IMGSYS(CLK_IMGSYS_WPE2_LARB11, "imgsys_wpe2_larb11", "top_img", 0),
> + GATE_IMGSYS(CLK_IMGSYS_WPE2, "imgsys_wpe2", "top_img", 1),
> +};
> +
> +static const struct mtk_gate imgsys_wpe3_clks[] = {
> + GATE_IMGSYS(CLK_IMGSYS_WPE3_LARB11, "imgsys_wpe3_larb11", "top_img", 0),
> + GATE_IMGSYS(CLK_IMGSYS_WPE3, "imgsys_wpe3", "top_img", 1),
> +};
> +
> +static const struct mtk_gate imgsys1_dip_top_clks[] = {
> + GATE_IMGSYS(CLK_IMGSYS1_DIP_TOP_LARB10, "imgsys1_dip_larb10", "top_img", 0),
> + GATE_IMGSYS(CLK_IMGSYS1_DIP_TOP_DIP_TOP, "imgsys1_dip_dip_top", "top_img", 1),
> +};
> +
> +static const struct mtk_gate imgsys1_dip_nr_clks[] = {
> + GATE_IMGSYS(CLK_IMGSYS1_DIP_NR_LARB15, "imgsys1_dip_nr_larb15", "top_img", 0),
> + GATE_IMGSYS(CLK_IMGSYS1_DIP_NR_DIP_NR, "imgsys1_dip_nr_dip_nr", "top_img", 1),
> +};
> +
> +static const struct mtk_clk_desc imgsys_main_desc = {
> + .clks = imgsys_main_clks,
> + .num_clks = ARRAY_SIZE(imgsys_main_clks),
> +};
> +
> +static const struct mtk_clk_desc imgsys_wpe1_desc = {
> + .clks = imgsys_wpe1_clks,
> + .num_clks = ARRAY_SIZE(imgsys_wpe1_clks),
> +};
> +
> +static const struct mtk_clk_desc imgsys_wpe2_desc = {
> + .clks = imgsys_wpe2_clks,
> + .num_clks = ARRAY_SIZE(imgsys_wpe2_clks),
> +};
> +
> +static const struct mtk_clk_desc imgsys_wpe3_desc = {
> + .clks = imgsys_wpe3_clks,
> + .num_clks = ARRAY_SIZE(imgsys_wpe3_clks),
> +};
> +
> +static const struct mtk_clk_desc imgsys1_dip_top_desc = {
> + .clks = imgsys1_dip_top_clks,
> + .num_clks = ARRAY_SIZE(imgsys1_dip_top_clks),
> +};
> +
> +static const struct mtk_clk_desc imgsys1_dip_nr_desc = {
> + .clks = imgsys1_dip_nr_clks,
> + .num_clks = ARRAY_SIZE(imgsys1_dip_nr_clks),
> +};
> +
> +static const struct of_device_id of_match_clk_mt8188_imgsys_main[] = {
> + {
> + .compatible = "mediatek,mt8188-imgsys",
> + .data = &imgsys_main_desc,
> + }, {
> + .compatible = "mediatek,mt8188-imgsys_wpe1",

I know that this was done in other clock drivers as well, but can we please
stop using underscores in devicetree compatibles?
That makes them look more consistent with the rest of the DT.

"mediatek,mt8188-imgsys-wpe1", as an example, would look a bit better.

P.S.: Please do the same on all of the other drivers that you are introducing
with this series.

Thanks,
Angelo


2022-12-12 08:30:30

by Garmin Chang (張家銘)

[permalink] [raw]
Subject: Re: [PATCH v2 08/19] clk: mediatek: Add MT8188 imgsys clock support

On Thu, 2022-10-27 at 10:25 +0200, AngeloGioacchino Del Regno wrote:
> Il 24/10/22 11:42, Garmin.Chang ha scritto:
> > Add MT8188 imgsys clock controllers which provide clock gate
> > control for image IP blocks.
> >
> > Signed-off-by: Garmin.Chang <[email protected]>
> > ---
> > drivers/clk/mediatek/Makefile | 2 +-
> > drivers/clk/mediatek/clk-mt8188-img.c | 124
> > ++++++++++++++++++++++++++
> > 2 files changed, 125 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/clk/mediatek/clk-mt8188-img.c
> >
> > diff --git a/drivers/clk/mediatek/Makefile
> > b/drivers/clk/mediatek/Makefile
> > index bd0a2aa5b6fa..242b49bafa9e 100644
> > --- a/drivers/clk/mediatek/Makefile
> > +++ b/drivers/clk/mediatek/Makefile
> > @@ -84,7 +84,7 @@ obj-$(CONFIG_COMMON_CLK_MT8186) += clk-mt8186-
> > mcu.o clk-mt8186-topckgen.o clk-mt
> > clk-mt8186-cam.o clk-mt8186-mdp.o
> > clk-mt8186-ipe.o
> > obj-$(CONFIG_COMMON_CLK_MT8188) += clk-mt8188-apmixedsys.o clk-
> > mt8188-topckgen.o \
> > clk-mt8188-peri_ao.o clk-mt8188-
> > infra_ao.o \
> > - clk-mt8188-cam.o clk-mt8188-ccu.o
> > + clk-mt8188-cam.o clk-mt8188-ccu.o
> > clk-mt8188-img.o
> > obj-$(CONFIG_COMMON_CLK_MT8192) += clk-mt8192.o
> > obj-$(CONFIG_COMMON_CLK_MT8192_AUDSYS) += clk-mt8192-aud.o
> > obj-$(CONFIG_COMMON_CLK_MT8192_CAMSYS) += clk-mt8192-cam.o
> > diff --git a/drivers/clk/mediatek/clk-mt8188-img.c
> > b/drivers/clk/mediatek/clk-mt8188-img.c
> > new file mode 100644
> > index 000000000000..00f3bbf4d502
> > --- /dev/null
> > +++ b/drivers/clk/mediatek/clk-mt8188-img.c
> > @@ -0,0 +1,124 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +//
> > +// Copyright (c) 2022 MediaTek Inc.
> > +// Author: Garmin Chang <[email protected]>
> > +
> > +#include <linux/clk-provider.h>
> > +#include <linux/platform_device.h>
> > +#include <dt-bindings/clock/mediatek,mt8188-clk.h>
> > +
> > +#include "clk-gate.h"
> > +#include "clk-mtk.h"
> > +
> > +static const struct mtk_gate_regs imgsys_cg_regs = {
> > + .set_ofs = 0x4,
> > + .clr_ofs = 0x8,
> > + .sta_ofs = 0x0,
> > +};
> > +
> > +#define GATE_IMGSYS(_id, _name, _parent, _shift)
> > \
> > + GATE_MTK(_id, _name, _parent, &imgsys_cg_regs, _shift,
> > &mtk_clk_gate_ops_setclr)
> > +
> > +static const struct mtk_gate imgsys_main_clks[] = {
> > + GATE_IMGSYS(CLK_IMGSYS_MAIN_LARB9, "imgsys_main_larb9",
> > "top_img", 0),
> > + GATE_IMGSYS(CLK_IMGSYS_MAIN_TRAW0, "imgsys_main_traw0",
> > "top_img", 1),
> > + GATE_IMGSYS(CLK_IMGSYS_MAIN_TRAW1, "imgsys_main_traw1",
> > "top_img", 2),
> > + GATE_IMGSYS(CLK_IMGSYS_MAIN_VCORE_GALS,
> > "imgsys_main_vcore_gals", "top_img", 3),
> > + GATE_IMGSYS(CLK_IMGSYS_MAIN_DIP0, "imgsys_main_dip0",
> > "top_img", 8),
> > + GATE_IMGSYS(CLK_IMGSYS_MAIN_WPE0, "imgsys_main_wpe0",
> > "top_img", 9),
> > + GATE_IMGSYS(CLK_IMGSYS_MAIN_IPE, "imgsys_main_ipe", "top_img",
> > 10),
> > + GATE_IMGSYS(CLK_IMGSYS_MAIN_WPE1, "imgsys_main_wpe1",
> > "top_img", 12),
> > + GATE_IMGSYS(CLK_IMGSYS_MAIN_WPE2, "imgsys_main_wpe2",
> > "top_img", 13),
> > + GATE_IMGSYS(CLK_IMGSYS_MAIN_GALS, "imgsys_main_gals",
> > "top_img", 31),
> > +};
> > +
> > +static const struct mtk_gate imgsys_wpe1_clks[] = {
> > + GATE_IMGSYS(CLK_IMGSYS_WPE1_LARB11, "imgsys_wpe1_larb11",
> > "top_img", 0),
> > + GATE_IMGSYS(CLK_IMGSYS_WPE1, "imgsys_wpe1", "top_img", 1),
> > +};
> > +
> > +static const struct mtk_gate imgsys_wpe2_clks[] = {
> > + GATE_IMGSYS(CLK_IMGSYS_WPE2_LARB11, "imgsys_wpe2_larb11",
> > "top_img", 0),
> > + GATE_IMGSYS(CLK_IMGSYS_WPE2, "imgsys_wpe2", "top_img", 1),
> > +};
> > +
> > +static const struct mtk_gate imgsys_wpe3_clks[] = {
> > + GATE_IMGSYS(CLK_IMGSYS_WPE3_LARB11, "imgsys_wpe3_larb11",
> > "top_img", 0),
> > + GATE_IMGSYS(CLK_IMGSYS_WPE3, "imgsys_wpe3", "top_img", 1),
> > +};
> > +
> > +static const struct mtk_gate imgsys1_dip_top_clks[] = {
> > + GATE_IMGSYS(CLK_IMGSYS1_DIP_TOP_LARB10, "imgsys1_dip_larb10",
> > "top_img", 0),
> > + GATE_IMGSYS(CLK_IMGSYS1_DIP_TOP_DIP_TOP, "imgsys1_dip_dip_top",
> > "top_img", 1),
> > +};
> > +
> > +static const struct mtk_gate imgsys1_dip_nr_clks[] = {
> > + GATE_IMGSYS(CLK_IMGSYS1_DIP_NR_LARB15, "imgsys1_dip_nr_larb15",
> > "top_img", 0),
> > + GATE_IMGSYS(CLK_IMGSYS1_DIP_NR_DIP_NR, "imgsys1_dip_nr_dip_nr",
> > "top_img", 1),
> > +};
> > +
> > +static const struct mtk_clk_desc imgsys_main_desc = {
> > + .clks = imgsys_main_clks,
> > + .num_clks = ARRAY_SIZE(imgsys_main_clks),
> > +};
> > +
> > +static const struct mtk_clk_desc imgsys_wpe1_desc = {
> > + .clks = imgsys_wpe1_clks,
> > + .num_clks = ARRAY_SIZE(imgsys_wpe1_clks),
> > +};
> > +
> > +static const struct mtk_clk_desc imgsys_wpe2_desc = {
> > + .clks = imgsys_wpe2_clks,
> > + .num_clks = ARRAY_SIZE(imgsys_wpe2_clks),
> > +};
> > +
> > +static const struct mtk_clk_desc imgsys_wpe3_desc = {
> > + .clks = imgsys_wpe3_clks,
> > + .num_clks = ARRAY_SIZE(imgsys_wpe3_clks),
> > +};
> > +
> > +static const struct mtk_clk_desc imgsys1_dip_top_desc = {
> > + .clks = imgsys1_dip_top_clks,
> > + .num_clks = ARRAY_SIZE(imgsys1_dip_top_clks),
> > +};
> > +
> > +static const struct mtk_clk_desc imgsys1_dip_nr_desc = {
> > + .clks = imgsys1_dip_nr_clks,
> > + .num_clks = ARRAY_SIZE(imgsys1_dip_nr_clks),
> > +};
> > +
> > +static const struct of_device_id of_match_clk_mt8188_imgsys_main[]
> > = {
> > + {
> > + .compatible = "mediatek,mt8188-imgsys",
> > + .data = &imgsys_main_desc,
> > + }, {
> > + .compatible = "mediatek,mt8188-imgsys_wpe1",
>
> I know that this was done in other clock drivers as well, but can we
> please
> stop using underscores in devicetree compatibles?
> That makes them look more consistent with the rest of the DT.
>
> "mediatek,mt8188-imgsys-wpe1", as an example, would look a bit
> better.
>
> P.S.: Please do the same on all of the other drivers that you are
> introducing
> with this series.
>
> Thanks,
> Angelo

Thank you for your suggestion.
Ok, I'll check all drivers for this underscores problem and fix them in
V3.

Thanks,
Best Regards,
Garmin