Date:   Mon, 23 May 2022 01:02:39 +0800
From:   Miles Chen <miles.chen@...iatek.com>
To:     <angelogioacchino.delregno@...labora.com>
CC:     <bgolaszewski@...libre.com>, <chun-jie.chen@...iatek.com>,
        <devicetree@...r.kernel.org>, <ikjn@...omium.org>,
        <krzysztof.kozlowski+dt@...aro.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-clk@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-mediatek@...ts.infradead.org>, <matthias.bgg@...il.com>,
        <miles.chen@...iatek.com>, <mturquette@...libre.com>,
        <p.zabel@...gutronix.de>, <robh+dt@...nel.org>,
        <sam.shih@...iatek.com>, <sboyd@...nel.org>,
        <tinghan.shen@...iatek.com>, <weiyi.lu@...iatek.com>,
        <wenst@...omium.org>, <y.oudjana@...tonmail.com>,
        <yassine.oudjana@...il.com>,
        <~postmarketos/upstreaming@...ts.sr.ht>
Subject: Re: [PATCH v2 4/4] clk: mediatek: Add drivers for MediaTek MT6735 main clock drivers

Hi Angelo, Yassine,

>>>
>>> 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 common style.
>> 
>> How do you think?
>> 
>
>In my opinion, Yassine is definitely right about this one: unrolling these 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 to 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 everything
>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, versatile, etc),
>so it's really "do it as you wish"...
>

Thanks for the explanation and guide. I think we can do that for all MediaTek
clock driver in the future, not having two styles in MediaTek clock driver now.

>
>... *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 MediaTek clock
>drivers harder for some of the community in this moment... especially when we look
>at how many MTK SoCs are out there in the wild, and how many we have upstream:
>something like 10% of them, or less.

and thanks for considering this too.

>
>I see the huge benefit of having a bigger community around MediaTek platforms as
>that's beneficial to get a way better support and solidity for all SoCs as they
>are sharing the same drivers and same framework, and expanding the support to more
>of them will only make it better with highly valuable community contributions.
>
>
>That said, Yassine, you should've understood that you have my full support 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 is, at
>this point, unavoidable... if we are able to get the aid of scripts (cocci and
>others), that will make our life easier in this cleanup, and will also make us
>able to perform the entire cleanup with less effort and in less overall time.
>
>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 am sure - and it is my wish - to see MediaTek clock drivers open-coding stuff
>instead of using macros, but that's something for the future - which will happen
>after the more important cleanups.
>
>After all, it will be just about running "gcc -E xxxx.c" and copy-pasting the
>unrolled macros to the clock drivers, which will be pretty fast and straightforward.
>
>
>Sorry for the wall of text, by the way.

Sounds good and I want to say thank you again, I learned a lot from your post
and patches you submitted.

and I also want to say thank you to Yassine for the patch.

thanks,
Miles
>
>Cheers,
>Angelo
>

Powered by blists - more mailing lists