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

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

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.


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

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