Re: [PATCH v2 4/4] clk: mediatek: Add drivers for MediaTek MT6735 main clock drivers

[Miles Chen]

hi Yassine,

> Add drivers for MT6735 apmixedsys, topckgen, infracfg and pericfg
> clock and reset controllers. These provide the base clocks on the
> platform, and should be enough to bring up all essential blocks
> including PWRAP, MSDC and peripherals (UART, I2C, SPI).
> 
> Signed-off-by: Yassine Oudjana <y.oudjana@xxxxxxxxxxxxxx>
> ---
> Dependencies:
> - clk: mediatek: Move to struct clk_hw provider APIs (series)
>   https://patchwork.kernel.org/project/linux-mediatek/cover/20220510104804.544597-1-wenst@xxxxxxxxxxxx/ 
> - Cleanup MediaTek clk reset drivers and support MT8192/MT8195 (series)
>   https://patchwork.kernel.org/project/linux-mediatek/cover/20220503093856.22250-1-rex-bc.chen@xxxxxxxxxxxx/
> - Export required symbols to compile clk drivers as module (single patch)
>   https://patchwork.kernel.org/project/linux-mediatek/patch/20220518111652.223727-7-angelogioacchino.delregno@xxxxxxxxxxxxx/
> - clk: mediatek: Improvements to simple probe/remove and reset controller unregistration
>   https://patchwork.kernel.org/project/linux-clk/cover/20220519134728.456643-1-y.oudjana@xxxxxxxxxxxxxx/
> 
>  MAINTAINERS                                  |    4 +
>  drivers/clk/mediatek/Kconfig                 |    9 +
>  drivers/clk/mediatek/Makefile                |    1 +
>  drivers/clk/mediatek/clk-mt6735-apmixedsys.c |  235 ++++

...snip...

> +#define APLL2_CON0		0x284
> +#define APLL2_CON1		0x288
> +#define APLL2_CON2		0x28c
> +#define APLL2_PWR_CON0		0x294
> +
> +#define CON0_RST_BAR		BIT(24)
> +
> +static const struct mtk_pll_data apmixedsys_plls[] = {
> +	{
> +		.id = ARMPLL,
> +		.name = "armpll",
> +		.parent_name = "clk26m",
> +
> +		.reg = ARMPLL_CON0,
> +		.pwr_reg = ARMPLL_PWR_CON0,
> +		.en_mask = 0x00000001,
> +
> +		.pd_reg = ARMPLL_CON1,
> +		.pd_shift = 24,
> +
> +		.pcw_reg = ARMPLL_CON1,
> +		.pcw_chg_reg = ARMPLL_CON1,
> +		.pcwbits = 21,
> +
> +		.flags = PLL_AO

Thanks for submitting this patch.

I compare this with drivers/clk/mediatek/clk-mt7986-apmixed.c,
and other clk files are using macros to make the mtk_pll_data array
more readable.

Would you mind following the same style for all c files, please?

e.g.,
	static const struct mtk_pll_data plls[] = {
		PLL(CLK_APMIXED_ARMPLL, "armpll", 0x0200, 0x020C, 0x00000001, 0, 32,
				0x0200, 4, 0, 0x0204, 0),
		PLL(CLK_APMIXED_NET2PLL, "net2pll", 0x0210, 0x021C, 0x00000001, 0, 32,
				0x0210, 4, 0, 0x0214, 0),                                           
		...
	};

> +	},
> +	{
> +		.id = MAINPLL,
> +		.name = "mainpll",
> +		.parent_name = "clk26m",
> +
> +		.reg = MAINPLL_CON0,
> +		.pwr_reg = MAINPLL_PWR_CON0,
> +		.en_mask = 0xf0000101,
> +
> +		.pd_reg = MAINPLL_CON1,
> +		.pd_shift = 24,
> +
> +		.pcw_reg = MAINPLL_CON1,
> +		.pcw_chg_reg = MAINPLL_CON1,
> +		.pcwbits = 21,
> +
> +		.flags = HAVE_RST_BAR,
> +		.rst_bar_mask = CON0_RST_BAR
> +	},
> +	{
> +		.id = UNIVPLL,
> +		.name = "univpll",
> +		.parent_name = "clk26m",
> +
> +		.reg = UNIVPLL_CON0,
> +		.pwr_reg = UNIVPLL_PWR_CON0,
> +		.en_mask = 0xfc000001,
> +
> +		.pd_reg = UNIVPLL_CON1,
> +		.pd_shift = 24,
> +
> +		.pcw_reg = UNIVPLL_CON1,
> +		.pcw_chg_reg = UNIVPLL_CON1,
> +		.pcwbits = 21,
> +
> +		.flags = HAVE_RST_BAR,
> +		.rst_bar_mask = CON0_RST_BAR
> +	},
> +	{
> +		.id = MMPLL,
> +		.name = "mmpll",
> +		.parent_name = "clk26m",
> +
> +		.reg = MMPLL_CON0,
> +		.pwr_reg = MMPLL_PWR_CON0,
> +		.en_mask = 0x00000001,
> +
> +		.pd_reg = MMPLL_CON1,
> +		.pd_shift = 24,
> +
> +		.pcw_reg = MMPLL_CON1,
> +		.pcw_chg_reg = MMPLL_CON1,
> +		.pcwbits = 21
> +	},
> +	{
> +		.id = MSDCPLL,
> +		.name = "msdcpll",
> +		.parent_name = "clk26m",
> +
> +		.reg = MSDCPLL_CON0,
> +		.pwr_reg = MSDCPLL_PWR_CON0,
> +		.en_mask = 0x00000001,
> +
> +		.pd_reg = MSDCPLL_CON1,
> +		.pd_shift = 24,
> +
> +		.pcw_reg = MSDCPLL_CON1,
> +		.pcw_chg_reg = MSDCPLL_CON1,
> +		.pcwbits = 21,
> +	},
> +	{
> +		.id = VENCPLL,
> +		.name = "vencpll",
> +		.parent_name = "clk26m",
> +
> +		.reg = VENCPLL_CON0,
> +		.pwr_reg = VENCPLL_PWR_CON0,
> +		.en_mask = 0x00000001,
> +
> +		.pd_reg = VENCPLL_CON1,
> +		.pd_shift = 24,
> +
> +		.pcw_reg = VENCPLL_CON1,
> +		.pcw_chg_reg = VENCPLL_CON1,
> +		.pcwbits = 21,
> +
> +		.flags = HAVE_RST_BAR,
> +		.rst_bar_mask = CON0_RST_BAR
> +	},
> +	{
> +		.id = TVDPLL,
> +		.name = "tvdpll",
> +		.parent_name = "clk26m",
> +
> +		.reg = TVDPLL_CON0,
> +		.pwr_reg = TVDPLL_PWR_CON0,
> +		.en_mask = 0x00000001,
> +
> +		.pd_reg = TVDPLL_CON1,
> +		.pd_shift = 24,
> +
> +		.pcw_reg = TVDPLL_CON1,
> +		.pcw_chg_reg = TVDPLL_CON1,
> +		.pcwbits = 21
> +	},
> +	{
> +		.id = APLL1,
> +		.name = "apll1",
> +		.parent_name = "clk26m",
> +
> +		.reg = APLL1_CON0,
> +		.pwr_reg = APLL1_PWR_CON0,
> +module_platform_driver(clk_mt6735_apmixedsys);
> +
> +MODULE_AUTHOR("Yassine Oudjana <y.oudjana@xxxxxxxxxxxxxx>");
> +MODULE_DESCRIPTION("Mediatek MT6735 apmixedsys clock driver");

Would you mind changing all Mediatek to MediaTek?
i.e.,

s/Mediatek/MediaTek/


thanks,
Miles
> +MODULE_LICENSE("GPL");