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

[Yassine Oudjana <yassine.oudjana@gmail.com>]

On Mon, Feb 27 2023 at 10:28:06 AM +01:00:00, AngeloGioacchino Del 
Regno <angelogioacchino.delregno@collabora.com> wrote:
> Il 25/02/23 10:42, Yassine Oudjana ha scritto:
>> From: Yassine Oudjana <y.oudjana@protonmail.com>
>> 
>> Add drivers for MT6735 apmixedsys, topckgen, infracfg and pericfg
>> clock and reset controllers. These provide the base clocks and resets
>> 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@protonmail.com>
>> ---
>>   MAINTAINERS                                  |   4 +
>>   drivers/clk/mediatek/Kconfig                 |   9 +
>>   drivers/clk/mediatek/Makefile                |   1 +
>>   drivers/clk/mediatek/clk-mt6735-apmixedsys.c | 139 ++++++
>>   drivers/clk/mediatek/clk-mt6735-infracfg.c   |  78 ++++
>>   drivers/clk/mediatek/clk-mt6735-pericfg.c    |  91 ++++
>>   drivers/clk/mediatek/clk-mt6735-topckgen.c   | 450 
>> +++++++++++++++++++
>>   7 files changed, 772 insertions(+)
>>   create mode 100644 drivers/clk/mediatek/clk-mt6735-apmixedsys.c
>>   create mode 100644 drivers/clk/mediatek/clk-mt6735-infracfg.c
>>   create mode 100644 drivers/clk/mediatek/clk-mt6735-pericfg.c
>>   create mode 100644 drivers/clk/mediatek/clk-mt6735-topckgen.c
>> 
> 
> ..snip..
> 
>> diff --git a/drivers/clk/mediatek/clk-mt6735-topckgen.c 
>> b/drivers/clk/mediatek/clk-mt6735-topckgen.c
>> new file mode 100644
>> index 000000000000..5fa743e4b0fc
>> --- /dev/null
>> +++ b/drivers/clk/mediatek/clk-mt6735-topckgen.c
>> @@ -0,0 +1,450 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2022 Yassine Oudjana <y.oudjana@protonmail.com>
>> + */
>> +
>> +#include <linux/clk-provider.h>
>> +#include <linux/platform_device.h>
>> +
>> +#include "clk-mtk.h"
>> +#include "clk-mux.h"
>> +
>> +#include <dt-bindings/clock/mediatek,mt6735-topckgen.h>
>> +
> 
> ..snip..
> 
>> +
>> +int clk_mt6735_topckgen_probe(struct platform_device *pdev)
> 
> It gets *even easier* than that!
> 
> Check out this one:
> https://patchwork.kernel.org/project/linux-mediatek/patch/20230222092543.19187-5-angelogioacchino.delregno@collabora.com/
> 
> ...being part of:
> https://patchwork.kernel.org/project/linux-mediatek/list/?series=724004
> 
> So you can use simple_probe for MT6735's topckgen too!

Isn't this basically what I did in v2[1][2]? What changed now?

> 
> In this case, it would be...
> 
> static const struct mtk_clk_desc topck_desc = {
> 	.clks = topckgen_muxes,
> 	.num_clks = ARRAY_SIZE(topckgen_muxes),
> 	.fixed_clks = topckgen_fixed_clks,
> 	.num_fixed_clks = ARRAY_SIZE(topckgen_fixed_clks),
> 	.factor_clks = topckgen_factors,
> 	.num_factor_clks = ARRAY_SIZE(topckgen_factors),
> 	.clk_lock = &mt6735_topckgen_lock,
> };
> 
> static const struct of_device_id of_match_mt6735_topckgen[] = {
> 	{ .compatible = "mediatek,mt6735-topckgen", .data = &topck_desc },
> 	{ /* sentinel */ }
> };
> 
> MODULE_DEVICE_TABLE(of, of_match_mt6735_topckgen)
>     ^^^^^
> You're missing that on multiple clock drivers ;-)
> 
> ...And you're replacing .probe(), .remove() callbacks with
> 
> static struct platform_driver clk_mt6735_topckgen = {
> 	.probe = mtk_clk_simple_probe,
> 	.remove = mtk_clk_simple_remove,
> 
> 	......
> 
> Other than that, good job!
> 
> After performing these changes, please make sure to mention the 
> dependency on
> my last cleanup series on your cover letter for v4, so that 
> maintainers will
> be aware of what to do.
> 
> Your v4 smells like Reviewed-by tags all over. Keep up the great work!
> 
> Cheers,
> Angelo

[1] 
https://lore.kernel.org/linux-mediatek/20220519142211.458336-5-y.oudjana@protonmail.com/
[2] 
https://patchwork.kernel.org/project/linux-clk/patch/20220519134728.456643-7-y.oudjana@protonmail.com/