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 |
