On Fri, May 20 2022 at 12:26:25 +0200, 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 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
