Re: [PATCH v2 2/3] soundwire: SDCA: add helper macro to access controls

From: Vinod Koul
Date: Wed Sep 16 2020 - 16:58:12 EST


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