[lkml]   [2020]   [Sep]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH v2 2/3] soundwire: SDCA: add helper macro to access controls

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:
>> 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.

 \ /
  Last update: 2020-09-16 22:36    [W:0.050 / U:4.928 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site