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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

Il 27/02/23 11:39, Yassine Oudjana ha scritto:
> 
> 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?
> 

*Basically*, yes. *Practically*, no.

To answer all your questions about that, please read my part 1 series that already
landed. The part 2 adds the factor clocks to the mix and performs a full migration
to platform_driver and modularity to 99% of MediaTek clock drivers.

>>
>> 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/
> 
> 
>