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

[Yassine Oudjana <yassine.oudjana@gmail.com>]

On Fri, May 20 2022 at 12:26:25 +0200, AngeloGioacchino Del Regno 
<angelogioacchino.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 
>> 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"...
> 
> ... *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.
> 
> 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.
> 
> Cheers,
> Angelo

Fair enough. I'll switch to macros in the next version.

Thanks,
Yassine