Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34771 )
Change subject: soc/mediatek: dsi: Refactor PHY timing calculation ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34771/6/src/soc/mediatek/common/dsi... File src/soc/mediatek/common/dsi.c:
https://review.coreboot.org/c/coreboot/+/34771/6/src/soc/mediatek/common/dsi... PS6, Line 100: timcon0 = phy_timing->lpx | phy_timing->da_hs_prepare << 8 | : phy_timing->da_hs_zero << 16 | phy_timing->da_hs_trail << 24;
Does it make sense to define a macro similar to the following? Then much code under soc/mediatek can […]
That's an interesting idea, but I think the intent was different.
What they did there was really to encode a string into a word (for mark, signal, or magic identifiers). However here we are constructing values for a register. it's simply coincident that many of the calls have 4 fields, each in 8 bytes. But if we want to really write this in a better way, you should deal with field, shift, length, mask, ... etc and usually in the end it's probably not better than v = a | b << 8 | c << 16 | d << 24, or
write32(&v, a << REG_FIELD_A_SHIFT | b << REG_FIELD_B_SHIFT, ...)
So I think we're probably staying with this until something better is done.
-
Hung-Te Lin (Code Review)