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