Re: [PATCH 2/4] ALSA: hda: intel-nhlt: add intel_nhlt_ssp_mclk_mask()

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

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