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
