Re: [PATCH v3] soundwire: SDCA: add helper macro to access controls

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

>>>> +#define SDW_SDCA_CTL(fun, ent, ctl, ch)		(BIT(30) |
>>> 		\
>>>> +						 (((fun) & 0x7) << 22) |	\
>>>> +						 (((ent) & 0x40) << 15) |	\
>>>> +						 (((ent) & 0x3f) << 7) |	\
>>>> +						 (((ctl) & 0x30) << 15) |	\
>>>> +						 (((ctl) & 0x0f) << 3) |	\
>>>> +						 (((ch) & 0x38) << 12) |	\
>>>> +						 ((ch) & 0x07))
>>>> +
>>>> +#define SDW_SDCA_MBQ_CTL(reg)			((reg) | BIT(13))
>>>> +#define SDW_SDCA_NEXT_CTL(reg)			((reg) | BIT(14))
>>>> +
>>>>   #endif /* __SDW_REGISTERS_H */
>>>
>>>
>>> No users of these macros?
>>
>> SDW_SDCA_CTL is used in sdca codec drivers which are not upstream yet.
>> SDW_SDCA_MBQ_CTL will be used in a new regmap method.
>> SDW_SDCA_NEXT_CTL can be used in sdca codec drivers, too.
> 
> Well, the point is that it's hard to review without seeing how the
> code of actual users are.

Agree, but our job is not made easy by the three-way dependency on 
regmap, SoundWire before we can submit ASoC codec drivers (developed by 
Realtek and tested by Intel).

If you prefer us to send all patches for SDCA codec support in one shot, 
that would be fine with us.
> BTW, the bit definitions can be simplified with GENMASK().
> I personally don't think GENMASK() necessarily good, but it may fit
> better in a case like this.

we use this macro in switch cases, e.g. for regmap properties to define 
read/volatile registers:

case SDW_SDCA_CTL(FUN_JACK_CODEC, RT711_SDCA_ENT_GE49, 
RT711_SDCA_CTL_SELECTED_MODE, 0):
	case SDW_SDCA_CTL(FUN_JACK_CODEC, RT711_SDCA_ENT_GE49, 
RT711_SDCA_CTL_DETECTED_MODE, 0):
	case SDW_SDCA_CTL(FUN_HID, RT711_SDCA_ENT_HID01, 
RT711_SDCA_CTL_HIDTX_CURRENT_OWNER, 0) ...
		SDW_SDCA_CTL(FUN_HID, RT711_SDCA_ENT_HID01, 
RT711_SDCA_CTL_HIDTX_MESSAGE_LENGTH, 0):
	case RT711_BUF_ADDR_HID1 ... RT711_BUF_ADDR_HID2:
		return true;

https://github.com/thesofproject/linux/blob/70fe32e776dafb4b03581d62a4569f65c2f13ada/sound/soc/codecs/rt711-sdca-sdw.c#L35

and unfortunately all our attempts to use FIELD_PREP, FIELD_GET, 
u32_encode, as suggested by Vinod, failed for this case due to 
compilation issues (can't use these macros outside of a function scope). 
The errors were shared with Vinod.

That's why we went back to the initial suggestion to deal with the 
shifts/masks by hand. For now we don't have a better solution that works 
in all cases were the macro is used.

Thanks
-Pierre