Re: [PATCH v3 17/17] ASoC: Intel: avs: Code loading over HDA

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

>>> Compared to SKL and KBL, younger cAVS platforms are meant to re-use
>>> one
>> Younger? you mean newer?
> 
> 
> Isn't the meaning of the two quite similar in this context?

younger doesn't sound quite right to me.

'cAVS platforms more recent that SKL and KBL...'

> 
>>> of HDAudio streams during boot procedure causing CLDMA to become
>>> obsolete. Once transferred, given stream is returned to pool
>>> available
>>> for audio streaming.
>>>
>>> Module loading handler is dummy as library and module code became
>>> inseparable in later firmware generations.
>> replace dummy with "stub" maybe? lets use inclusive terms
> 
> 
> Is 'dummy' categorized as non-inclusive? We have several usages of 
> 'dummy' even within /sound (e.g. soc-utils.c).

Intel and plenty of other companies recommend a different wording. 
mock-up, stub, placeholder, etc. see e.g.

https://developers.google.com/style/inclusive-documentation

Yes we have plenty of existing uses of 'dummy', but that doesn't mean we 
should add new ones.

>>> +static int
>>> +avs_hda_init_rom(struct avs_dev *adev, unsigned int dma_id, bool
>>> purge)
>>> +{
>>> +    const struct avs_spec *const spec = adev->spec;
>>> +    unsigned int corex_mask, reg;
>>> +    int ret;
>>> +
>>> +    corex_mask = spec->core_init_mask & ~AVS_MAIN_CORE_MASK;
>>> +
>>> +    ret = avs_dsp_op(adev, power, spec->core_init_mask, true);
>>> +    if (ret < 0)
>>> +        goto err;
>>> +
>>> +    ret = avs_dsp_op(adev, reset, AVS_MAIN_CORE_MASK, false);
>>> +    if (ret < 0)
>>> +        goto err;
>>> +
>>> +    reinit_completion(&adev->fw_ready);
>>> +    avs_dsp_op(adev, int_control, true);
>>> +
>>> +    /* set boot config */
>>> +    ret = avs_ipc_set_boot_config(adev, dma_id, purge);
>>> +    if (ret) {
>>> +        ret = AVS_IPC_RET(ret);
>>> +        goto err;
>>> +    }
>>> +
>>> +    /* await ROM init */
>>> +    ret = snd_hdac_adsp_readq_poll(adev, spec->rom_status, reg,
>>> +                       (reg & 0xF) == AVS_ROM_INIT_DONE
>>> ||
>>> +                       (reg & 0xF) ==
>>> APL_ROM_FW_ENTERED,
>>> +                       AVS_ROM_INIT_POLLING_US,
>>> APL_ROM_INIT_TIMEOUT_US);
>>> +    if (ret < 0) {
>>> +        dev_err(adev->dev, "rom init timeout: %d\n", ret);
>>> +        goto err;
>>> +    }
>>> +
>>> +    /* power down non-main cores */
>>> +    if (corex_mask)
>>> +        avs_dsp_op(adev, power, corex_mask, false);
>> What if this fails?
> 
> 
> We are still in happy path, worst thing could happen here is increased 
> power consumption.

'happy path'?