>>>>>> + } >>>>>> + >>>>>> + cfg = (struct nhlt_fmt_cfg *)(cfg->config.caps + >>>>>> cfg->config.size); >>>>>> + } >>>>>> + } >>>>>> + epnt = (struct nhlt_endpoint *)((u8 *)epnt + epnt->length); >>>>>> + } >>>>>> + >>>>>> + return mclk_mask; >>>>> >>>>> Although I understand that it is relegated to the caller, but if both >>>>> mclk being set is considered an error maybe add some kind of check >>>>> here >>>>> instead and free callers from having to remember about it? >>>>> >>>>> if (hweight_long(mclk_mask) != 1) >>>>> return -EINVAL; >>>>> >>>>> return mclk_mask; >>>> >>>> I went back and forth multiple times on this one. I can't figure out if >>>> this would be a bug or a feature, it could be e.g. a test capability >>>> and >>>> it's supported in hardware. I decided to make the decision in the >>>> caller >>>> rather than a lower level in the library. >>>> >>>> If the tools used to generate NHLT don't support this multi-MCLK mode >>>> then we could indeed move the test here. >>>> >>> >>> Considering comment I added above I've asked Czarek to also check this >>> series. I'm not sure it even makes sense to name the field "_mask" when >>> it is one bit... >> >> it's two bits, see above. > > So I've spend a bit talking with FW team, and you are right, I got > confused by one of the tables and some code that specified it as 1 bit > field and rest as reserved, while other documents do specify it as a > variable range of bits. Yeah, I had to ask multiple times as well. It's far from self-explanatory and all the findings were based on NHLT shared with me. See https://github.com/thesofproject/linux/issues/3336#issuecomment-1206176141 for two examples with MCLK0 and MCLK1 used. > Going back to return value, the tool I have access to only has support > for MCLK0. I guess we can make the assumption for now that everyone > connects codec to one clock source and if someone later implements HW > where somehow 2 different clocks are used (depending on format) we can > refine the check later? Indeed it seems that depending on tools versions and targeted silicon, MCLK1 may or may not be supported. I guess it'd be fine to throw a big fat error in case this two-MCLK configuration ever shows-up. That way we'll know for sure what was deployed. Thanks for the feedback, I'll update this shortly.
