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

[Takashi Iwai <tiwai@suse.de>]

On Fri, 30 Oct 2020 16:52:24 +0100,
Pierre-Louis Bossart wrote:
> 
> 
> 
> >>>> +#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.

It's not necessarily mandatory to send the whole series, but if a
relevant code is already available, mentioning a repo URL in the patch
description (or in the comment below the delimiter) would be helpful,
for example.

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

Hrm, OK, in this case the value is masked then shifted, so it's not
trivial to deal with a macro.


thanks,

Takashi