lkml.org 
[lkml]   [2022]   [May]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 4/4] clk: mediatek: Add drivers for MediaTek MT6735 main clock drivers

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


\
 
 \ /
  Last update: 2022-05-20 13:10    [W:0.066 / U:0.640 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site