Hello Stefan, Hello Sascha, On 28.02.24 09:46, Stefan Kerkmann wrote: >> Another thing is that the usage of cpu_is() has the tendency to lead to >> cascades of if (cpu_is_x() || cpu_is_y() || cpu_is_z()) which is not >> paticularly nice to read. >> > > That is arguably subjective :-). > > For me as a developer that is new to barebox, it was confusing to find two different styles of arch dependent code. I prefer the `cpu_is_xyz` style approach which is used in barebox proper much more. > > In the case of the TZC380 driver the pseudo (as they are probably optimized away) runtime arguments to the init functions seem unnecessarily complicated, I would suspect they aren't even optimized away, because it's only known at link-time that the function is only called once, so a late inlining would require LTO. This means your approach may have a lower overhead than what we are doing right now... :-) > as does the approach to define aliases to the same function for all arches. The if style is clearer in intend as it keeps the distinction between the arches local to the parts that are actually different. Which is straight forward to read IMHO. I also think it's a good idea that in future we should extend to more places in the i.MX8M init code. The code duplication in ./arch/arm/mach-imx/atf.c is not a pretty sight. Cheers, Ahmad > >> Both are not really strong points, but on the other hand there's not >> much improvement in this patch, so I tend to not take it. >> >>> -bool tzc380_is_enabled(void) >>> +bool imx8m_tzc380_is_enabled(void) >> >> This change is good though as the function is clearly i.MX8 specific. >> >> Sascha >> > > Cheers, > Stefan > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
