Re: [PATCH v9 18/22] clk: mediatek: Add MT8192 mmsys clock support

[Matthias Brugger]

Hi Chun-Jie,

On 09/06/2021 00:38, Chun-Jie Chen wrote:
> On Tue, 2021-06-08 at 16:44 +0200, Matthias Brugger wrote:
>>
>> On 24/05/2021 14:20, Chun-Jie Chen wrote:
>>> Add MT8192 mmsys clock provider
>>>
>>> Signed-off-by: Weiyi Lu <weiyi.lu@xxxxxxxxxxxx>
>>> Signed-off-by: chun-jie.chen <chun-jie.chen@xxxxxxxxxxxx>
>>> ---
>>>  drivers/clk/mediatek/Kconfig         |   6 ++
>>>  drivers/clk/mediatek/Makefile        |   1 +
>>>  drivers/clk/mediatek/clk-mt8192-mm.c | 108
>>> +++++++++++++++++++++++++++
>>>  3 files changed, 115 insertions(+)
>>>  create mode 100644 drivers/clk/mediatek/clk-mt8192-mm.c
>>>
>>
>> [...]
>>> +
>>> +static int clk_mt8192_mm_probe(struct platform_device *pdev)
>>> +{
>>> +	struct device *dev = &pdev->dev;
>>> +	struct device_node *node = dev->parent->of_node;
>>> +	struct clk_onecell_data *clk_data;
>>> +	int r;
>>> +
>>> +	clk_data = mtk_alloc_clk_data(CLK_MM_NR_CLK);
>>> +	if (!clk_data)
>>> +		return -ENOMEM;
>>> +
>>> +	r = mtk_clk_register_gates(node, mm_clks, ARRAY_SIZE(mm_clks),
>>> clk_data);
>>> +	if (r)
>>> +		return r;
>>> +
>>> +	return of_clk_add_provider(node, of_clk_src_onecell_get,
>>> clk_data);
>>> +}
>>> +
>>> +static struct platform_driver clk_mt8192_mm_drv = {
>>> +	.probe = clk_mt8192_mm_probe,
>>> +	.driver = {
>>> +		.name = "clk-mt8192-mm",
>>> +	},
>>> +};
>>
>> Did you had a look at drivers/soc/mediatek/mtk-mmsys.c? How is the
>> MMSYS
>> different from all the other SoCs? I suppose it is not. Please don't
>> just
>> implement the clock drivers, but check in existing code how they play
>> together
>> with the HW they are for. MediaTek unfortunately has the design to
>> add the clock
>> registers in the address space of the IP block that needs this
>> registers. Which
>> makes it more complicated to implement clock driver in the first
>> place.
>>
>> Regards,
>> Matthias
> 
> Did you means binding the mm clock driver by creating a platform device
> in drivers/soc/mediatek/mtk-mmsys.c? There is 8192 mmsys compatible
> data in patch [1] but lack of it in the latest patch [2], I will check
> it.
> Thanks for your kind reminder.
> 

Yes, the clock driver should be a platform driver. Binding should be done
through the soc driver.


Thanks a lot,
Matthias

> [1] 
> https://patchwork.kernel.org/project/linux-mediatek/patch/1609815993-22744-11-git-send-email-yongqiang.niu@xxxxxxxxxxxx/
> [2] 
> https://patchwork.kernel.org/project/linux-mediatek/patch/1618236288-1617-5-git-send-email-yongqiang.niu@xxxxxxxxxxxx/
> 
> Best Regards,
> Chun-Jie
>