Date:   Mon, 27 Feb 2023 13:39:46 +0300
From:   Yassine Oudjana <yassine.oudjana@gmail.com>
Subject: Re: [PATCH v3 4/4] clk: mediatek: Add drivers for MediaTek MT6735
 main clock and reset drivers
To:     AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.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
Message-Id: <AYIQQR.5R1644EUJNRV2@gmail.com>
In-Reply-To: <cd634833-b28e-d6d3-692d-5a391b85ad34@collabora.com>
References: <20230225094246.261697-1-y.oudjana@protonmail.com>
        <20230225094246.261697-5-y.oudjana@protonmail.com>
        <cd634833-b28e-d6d3-692d-5a391b85ad34@collabora.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Precedence: bulk


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/