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

[Yassine Oudjana]

On Friday, May 20th, 2022 at 11:26 AM, AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx> 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'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 this
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 variations
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 drivers,
or is it still too soon?

Another thing: Since I've been out of touch with the cleanup work for a while,
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 work.

> ...