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. > ...
