On 14-09-20, 09:44, Pierre-Louis Bossart wrote: > > For LSB bits, I dont think this is an issue. I expect it to work, for example: > > #define CONTROL_LSB_MASK GENMASK(2, 0) > > foo |= u32_encode_bits(control, CONTROL_LSB_MASK); > > > > would mask the control value and program that in specific bitfeild. > > > > But for MSB bits, I am not sure above will work so, you may need to extract > > the bits and then use, for example: > > #define CONTROL_MSB_BITS GENMASK(5, 3) > > #define CONTROL_MSB_MASK GENMASK(17, 15) > > > > control = FIELD_GET(CONTROL_MSB_BITS, control); > > foo |= u32_encode_bits(control, CONTROL_MSB_MASK); > > > > > If you have a better suggestion that the FIELD_PREP/FIELD_GET use, I am all > > > ears. At the end of the day, the mapping is pre-defined and we don't have > > > any degree of freedom. What I do want is that this macro/inline function is > > > shared by all codec drivers so that we don't have different interpretations > > > of how the address is constructed. > > > > Absolutely, this need to be defined here and used by everyone else. > > Compare: > > #define SDCA_CONTROL_MSB_BITS GENMASK(5, 3) > #define SDCA_CONTROL_MSB_MASK GENMASK(17, 15) > #define SDCA_CONTROL_LSB_MASK GENMASK(2, 0) > > foo |= u32_encode_bits(control, SDCA_CONTROL_LSB_MASK); > control = FIELD_GET(SDCA_CONTROL_MSB_BITS, control); > foo |= u32_encode_bits(control, SDCA_CONTROL_MSB_MASK); > > with the original proposal: > > foo |= FIELD_GET(GENMASK(2, 0), control)) > foo |= FIELD_PREP(GENMASK(17, 15), FIELD_GET(GENMASK(5, 3), control)) > > it gets worse when the LSB positions don't match, you need another variable > and an additional mask. > > I don't see how this improves readability? I get that hard-coding magic > numbers is a bad thing in general, but in this case there are limited > benefits to the use of additional defines. I think it would be prudent to define the masks and use them rather than magic values. Also it makes it future proof Thanks -- ~Vinod
