Messages in this thread | | | Date | Fri, 20 May 2022 15:08:26 +0400 | From | Yassine Oudjana <> | Subject | Re: [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
| |