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

[Amadeusz Sławiński]

On 8/23/2022 10:52 AM, Pierre-Louis Bossart wrote:
> Hi Amadeusz,
> 
>>> +int intel_nhlt_ssp_mclk_mask(struct nhlt_acpi_table *nhlt, int ssp_num)
>>> +{
>>> +    struct nhlt_endpoint *epnt;
>>> +    struct nhlt_fmt *fmt;
>>> +    struct nhlt_fmt_cfg *cfg;
>>> +    int mclk_mask = 0;
>>> +    int i, j;
>>> +
>>> +    if (!nhlt)
>>> +        return 0;
>>> +
>>> +    epnt = (struct nhlt_endpoint *)nhlt->desc;
>>> +    for (i = 0; i < nhlt->endpoint_count; i++) {
>>> +
>>> +        /* we only care about endpoints connected to an audio codec
>>> over SSP */
>>> +        if (epnt->linktype == NHLT_LINK_SSP &&
>>> +            epnt->device_type == NHLT_DEVICE_I2S &&
>>> +            epnt->virtual_bus_id == ssp_num) {
>>
>> if (epnt->linktype != NHLT_LINK_SSP ||
>>      epnt->device_type != NHLT_DEVICE_I2S ||
>>      epnt->virtual_bus_id != ssp_num)
>>      continue;
>>
>> and then you can remove one indentation level below?
> 
> 
> Would that work? We still need to move the epnt pointer:
> 
> epnt = (struct nhlt_endpoint *)((u8 *)epnt + epnt->length);
> 
> and moving this in the endpoint_count loop would be ugly as well.
> 
> 

Another solution would be goto to skip, but that also seems ugly :/
I guess it can stay as it is then.

>>> +
>>> +            fmt = (struct nhlt_fmt *)(epnt->config.caps +
>>> epnt->config.size);
>>> +            cfg = fmt->fmt_config;
>>> +
>>> +            /*
>>> +             * In theory all formats should use the same MCLK but it
>>> doesn't hurt to
>>> +             * double-check that the configuration is consistent
>>> +             */
>>> +            for (j = 0; j < fmt->fmt_count; j++) {
>>> +                u32 *blob;
>>> +                int mdivc_offset;
>>> +
>>> +                if (cfg->config.size >= SSP_BLOB_V1_0_SIZE) {
>>> +                    blob = (u32 *)cfg->config.caps;
>>> +
>>> +                    if (blob[1] == SSP_BLOB_VER_2_0)
>>> +                        mdivc_offset = SSP_BLOB_V2_0_MDIVC_OFFSET;
>>> +                    else if (blob[1] == SSP_BLOB_VER_1_5)
>>> +                        mdivc_offset = SSP_BLOB_V1_5_MDIVC_OFFSET;
>>> +                    else
>>> +                        mdivc_offset = SSP_BLOB_V1_0_MDIVC_OFFSET;
>>> +
>>> +                    mclk_mask |=  blob[mdivc_offset] & GENMASK(1, 0);

One more thing, where does this GENMASK come from, as far as I can tell 
HW specifies and FW uses one bit field to signal that MCLK is enabled? 
(mdivc is simply a value written to HW register to configure it).

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

>>
>>> +}
>>> +EXPORT_SYMBOL(intel_nhlt_ssp_mclk_mask);
>>> +
>>>    static struct nhlt_specific_cfg *
>>>    nhlt_get_specific_cfg(struct device *dev, struct nhlt_fmt *fmt, u8
>>> num_ch,
>>>                  u32 rate, u8 vbps, u8 bps)
>>