Hi Angelo, Yassine, >>> >>> 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"... > Thanks for the explanation and guide. I think we can do that for all MediaTek clock driver in the future, not having two styles in MediaTek clock driver now. > >... *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. and thanks for considering this too. > >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. Sounds good and I want to say thank you again, I learned a lot from your post and patches you submitted. and I also want to say thank you to Yassine for the patch. thanks, Miles > >Cheers, >Angelo >
