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

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

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.