Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 13 Aug 2022 10:44:49 +0000
To:     AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>
From:   Yassine Oudjana <y.oudjana@protonmail.com>
Cc:     Miles Chen <miles.chen@mediatek.com>, yassine.oudjana@gmail.com,
        bgolaszewski@baylibre.com, chun-jie.chen@mediatek.com,
        devicetree@vger.kernel.org, ikjn@chromium.org,
        krzysztof.kozlowski+dt@linaro.org,
        linux-arm-kernel@lists.infradead.org, linux-clk@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org,
        matthias.bgg@gmail.com, mturquette@baylibre.com,
        p.zabel@pengutronix.de, robh+dt@kernel.org, sam.shih@mediatek.com,
        sboyd@kernel.org, tinghan.shen@mediatek.com, weiyi.lu@mediatek.com,
        wenst@chromium.org, ~postmarketos/upstreaming@lists.sr.ht
Reply-To: Yassine Oudjana <y.oudjana@protonmail.com>
Subject: Re: [PATCH v2 4/4] clk: mediatek: Add drivers for MediaTek MT6735 main clock drivers
Message-ID: <lAB8fLfWTwUu6FUqPZWoKNEC0ZPYHnvo05u6BGriYQVjanlTzorHaZAflEbzoml-0UVZe-02r6CfzKwGdBCp7E0YeT_hF86P26r-Zeivda4=@protonmail.com>
In-Reply-To: <c7b98ee4-cd4f-d7b7-726d-1acd4fafd50a@collabora.com>
References: <NJC6CR.M4CF312LSXXV1@gmail.com> <20220520093501.28758-1-miles.chen@mediatek.com> <c7b98ee4-cd4f-d7b7-726d-1acd4fafd50a@collabora.com>
Feedback-ID: 6882736:user:proton
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Friday, May 20th, 2022 at 11:26 AM, AngeloGioacchino Del Regno <angelogi=
oacchino.delregno@collabora.com> wrote:

> Il 20/05/22 11:35, Miles Chen ha scritto:
>
> > > > Thanks for submitting this patch.
> > > >
> > > > I compare this with drivers/clk/mediatek/clk-mt7986-apmixed.c,
> > > > and other clk files are using macros to make the mtk_pll_data array
> > > > more readable.
> > >
> > > I'd actually argue that macros make it less readable. While reading
> > > other drivers I had a lot of trouble figuring out which argument
> > > is which field of the struct, and had to constantly go back to the
> > > macro definitions and count arguments to find it. Having it this
> > > way, each value is labeled clearly with the field it's in. I think
> > > the tradeoff between line count and readability here is worth it.
> >
> > It is easier for multiple developers to work together if we have a comm=
on style.
> >
> > How do you think?
>
>
> In my opinion, Yassine is definitely right about this one: unrolling thes=
e macros
> will make the code more readable, even though this has the side effect of=
 making
> it bigger in the source code form (obviously, when compiled, it's going t=
o be the
> exact same size).
>
> I wouldn't mind getting this clock driver in without the usage of macros,=
 as much
> as I wouldn't mind converting all of the existing drivers to open-code ev=
erything
> instead of using macros that you have to find in various headers... this =
practice
> was done in multiple drivers (clock or elsewhere), so I don't think that =
it would
> actually be a bad idea to do it here on MediaTek too, even though I'm not=
 aware of
> any rule that may want us to do that: if you check across drivers/clk/*, =
there's
> a big split in how drivers are made, where some are using macros (davinci=
, renesas,
> samsung, sprd, etc), and some are not (bcm, sunxi-ng, qcom, tegra, versat=
ile, etc),
> so it's really "do it as you wish"...
>
> ... but:
>
> Apart from that, I also don't think that it is a good idea to convert the=
 other
> MTK clock drivers right now, as this would make the upstreaming of MediaT=
ek clock
> drivers harder for some of the community in this moment... especially whe=
n we look
> at how many MTK SoCs are out there in the wild, and how many we have upst=
ream:
> something like 10% of them, or less.
>
> I see the huge benefit of having a bigger community around MediaTek platf=
orms as
> that's beneficial to get a way better support and solidity for all SoCs a=
s they
> are sharing the same drivers and same framework, and expanding the suppor=
t to more
> of them will only make it better with highly valuable community contribut=
ions.
>
>
> That said, Yassine, you should've understood that you have my full suppor=
t on
> unrolling these macros - but it's not time to do that yet: you definitely=
 know
> that MediaTek clock drivers are going through a big cleanup phase which i=
s, at
> this point, unavoidable... if we are able to get the aid of scripts (cocc=
i and
> others), that will make our life easier in this cleanup, and will also ma=
ke us
> able to perform the entire cleanup with less effort and in less overall t=
ime.
>
> With that, I'm sad but I have to support Miles' decision on this one, and=
 I also
> have to ask you to use macros in this driver.

I'm picking up this series again now after taking a long break to allow for
ongoing cleanup and refactoring work to settle down. I was going to make th=
is
change but then I couldn't find the PLL macro defined in any common header.
It seems that it is defined in every driver that uses it, with slight varia=
tions
in some of them. Should I just do the same, or would it be better to define=
 it
in clk-pll.h? Also, would now be a good time to unroll the macros in all dr=
ivers,
or is it still too soon?

Another thing: Since I've been out of touch with the cleanup work for a whi=
le,
it would be great if someone makes me aware of any pending cleanup patches =
that
I should know of so that I base my patches on them and avoid duplicating wo=
rk.

> ...