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/70fe32e776dafb4b03581d62a4569f65...
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