Message-ID: <04acf661-06b5-22b5-ca5d-50078495f7e8@collabora.com>
Date:   Mon, 27 Feb 2023 13:42:20 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH v3 4/4] clk: mediatek: Add drivers for MediaTek MT6735
 main clock and reset drivers
Content-Language: en-US
To:     Yassine Oudjana <yassine.oudjana@gmail.com>
Cc:     Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Michael Turquette <mturquette@baylibre.com>,
        Stephen Boyd <sboyd@kernel.org>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        Daniel Golle <daniel@makrotopia.org>,
        Allen-KH Cheng <allen-kh.cheng@mediatek.com>,
        Tinghan Shen <tinghan.shen@mediatek.com>,
        Chen-Yu Tsai <wenst@chromium.org>,
        Edward-JW Yang <edward-jw.yang@mediatek.com>,
        Johnson Wang <johnson.wang@mediatek.com>,
        Fabien Parent <fparent@baylibre.com>,
        Chun-Jie Chen <chun-jie.chen@mediatek.com>,
        Miles Chen <miles.chen@mediatek.com>,
        Bartosz Golaszewski <bgolaszewski@baylibre.com>,
        Yassine Oudjana <y.oudjana@protonmail.com>,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-clk@vger.kernel.org, linux-mediatek@lists.infradead.org,
        linux-arm-kernel@lists.infradead.org
References: <20230225094246.261697-1-y.oudjana@protonmail.com>
 <20230225094246.261697-5-y.oudjana@protonmail.com>
 <cd634833-b28e-d6d3-692d-5a391b85ad34@collabora.com>
 <AYIQQR.5R1644EUJNRV2@gmail.com>
From:   AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>
In-Reply-To: <AYIQQR.5R1644EUJNRV2@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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