Re: [PATCH v3 09/14] ASoC: SOF: Add firmware loader support

Pierre-Louis Bossart
2019-01-09T21:24:43Z

Thanks for the reviews Mark, much appreciated.

+int snd_sof_load_firmware_memcpy(struct snd_sof_dev *sdev)
>> +{
>> + struct snd_sof_pdata *plat_data = dev_get_platdata(sdev->dev);
>> + const char *fw_filename;
>> + int ret;
> This never actually calls the load_firmware() operation for the DSP
> AFAICT?

it's actually the implementation of the load_firmware callback, see e.g.
for Baytrail/Broadwell

/*Firmware loading */
.load_firmware    = snd_sof_load_firmware_memcpy,

>
>> + ret = request_firmware(&plat_data->fw, fw_filename, sdev->dev);
>> +
>> + if (ret < 0) {
>> + dev_err(sdev->dev, "error: request firmware failed err: %d\n",
>> + ret);
>> + return ret;
>> + }
> I'd suggest logging the name of the firmware we tried to load, users
> will thank you.
yes indeed, thanks for the suggestion, will add this right away.
>
>> + /* create fw_version debugfs to store boot version info */
>> + if (sdev->first_boot) {
>> + ret = snd_sof_debugfs_buf_create_item(sdev, &sdev->fw_version,
>> + sizeof(sdev->fw_version),
>> + "fw_version");
>> +
>> + if (ret < 0) {
>> + dev_err(sdev->dev, "error: cannot create debugfs for fw_version\n");
>> + return ret;
>> + }
>> + }
> As Andy said elsewhere debugfs stuff like that probably shouldn't be
> fatal.
yes, we've fixed this already.