Messages in this thread | | | Subject | Re: [PATCH v2 2/3] soundwire: SDCA: add helper macro to access controls | From | Pierre-Louis Bossart <> | Date | Wed, 16 Sep 2020 08:11:34 -0500 |
| |
On 9/16/20 7:35 AM, Vinod Koul wrote: > 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
I don't see your point at all. The values cannot be modified, a different macro would be needed for a standard change.
Anyways, I am not going to argue further, I'll use your code example as is and move on.
| |