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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <NJC6CR.M4CF312LSXXV1@gmail.com>
Date:   Fri, 20 May 2022 13:18:59 +0400
From:   Yassine Oudjana <yassine.oudjana@...il.com>
To:     Miles Chen <miles.chen@...iatek.com>
Cc:     angelogioacchino.delregno@...labora.com, bgolaszewski@...libre.com,
        chun-jie.chen@...iatek.com, devicetree@...r.kernel.org,
        ikjn@...omium.org, krzysztof.kozlowski+dt@...aro.org,
        linux-arm-kernel@...ts.infradead.org, linux-clk@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-mediatek@...ts.infradead.org,
        matthias.bgg@...il.com, mturquette@...libre.com,
        p.zabel@...gutronix.de, robh+dt@...nel.org, sam.shih@...iatek.com,
        sboyd@...nel.org, tinghan.shen@...iatek.com, weiyi.lu@...iatek.com,
        wenst@...omium.org, y.oudjana@...tonmail.com,
        ~postmarketos/upstreaming@...ts.sr.ht
Subject: Re: [PATCH v2 4/4] clk: mediatek: Add drivers for MediaTek MT6735
 main clock drivers


On Fri, May 20 2022 at 16:35:14 +0800, Miles Chen 
<miles.chen@...iatek.com> wrote:
> 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@...tonmail.com>
>>  ---
>>  Dependencies:
>>  - clk: mediatek: Move to struct clk_hw provider APIs (series)
>>    
>> https://patchwork.kernel.org/project/linux-mediatek/cover/20220510104804.544597-1-wenst@chromium.org/
>>  - Cleanup MediaTek clk reset drivers and support MT8192/MT8195 
>> (series)
>>    
>> https://patchwork.kernel.org/project/linux-mediatek/cover/20220503093856.22250-1-rex-bc.chen@mediatek.com/
>>  - Export required symbols to compile clk drivers as module (single 
>> patch)
>>    
>> https://patchwork.kernel.org/project/linux-mediatek/patch/20220518111652.223727-7-angelogioacchino.delregno@collabora.com/
>>  - clk: mediatek: Improvements to simple probe/remove and reset 
>> controller unregistration
>>    
>> https://patchwork.kernel.org/project/linux-clk/cover/20220519134728.456643-1-y.oudjana@protonmail.com/
>> 
>>   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.

I'd actually argue that macros make it less readable. While reading
other drivers I had a lot of trouble figuring out which argument
is which field of the struct, and had to constantly go back to the
macro definitions and count arguments to find it. Having it this
way, each value is labeled clearly with the field it's in. I think
the tradeoff between line count and readability here is worth it.

> 
> 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@...tonmail.com>");
>>  +MODULE_DESCRIPTION("Mediatek MT6735 apmixedsys clock driver");
> 
> Would you mind changing all Mediatek to MediaTek?
> i.e.,
> 
> s/Mediatek/MediaTek/
> 

Sure. Will fix it.

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

Thanks,
Yassine