[Sound-open-firmware] [alsa-devel] [PATCH v3 09/14] ASoC: SOF: Add firmware loader support

[Pierre-Louis Bossart]

On 12/11/18 4:38 PM, Andy Shevchenko wrote:
> On Tue, Dec 11, 2018 at 03:23:13PM -0600, Pierre-Louis Bossart wrote:
>> From: Liam Girdwood <liam.r.girdwood at linux.intel.com>
>>
>> The firmware loader exports APIs that can be called by core to load and
>> process multiple different file formats.
>> +static int get_ext_windows(struct snd_sof_dev *sdev,
>> +			   struct sof_ipc_ext_data_hdr *ext_hdr)
>> +{
>> +	struct sof_ipc_window *w = (struct sof_ipc_window *)ext_hdr;
>> +
>> +	int ret = 0;
> I don't see how it's used. Perhaps you need to check code with `make W=1`.

Darn, we missed this one. I thought we were using W=1 on github but we 
aren't, this will be fixed.

Though W=1 doesn't report this one, so need to re-inspect this. Thanks 
for the sighting.

>
>> +	size_t size;
>> +
>> +	if (w->num_windows == 0 || w->num_windows > SOF_IPC_MAX_ELEMS)
>> +		return -EINVAL;
>> +
>> +	size = sizeof(*w) + sizeof(struct sof_ipc_window_elem) * w->num_windows;
>> +
>> +	/* keep a local copy of the data */
>> +	sdev->info_window = kmemdup(w, size, GFP_KERNEL);
>> +	if (!sdev->info_window)
>> +		return -ENOMEM;
>> +
>> +	return ret;
>> +}
>> +			dev_warn(sdev->dev,
>> +				 "warning: block %d size zero\n", count);
>> +			dev_warn(sdev->dev, " type 0x%x offset 0x%x\n",
>> +				 block->type, block->offset);
> Hmm... Why do we need a kernel level duplication in words?
Not sure I get this one, it's just a warning message where you don't 
copy a block of size zero into the target memory.
>
>> +int snd_sof_load_firmware(struct snd_sof_dev *sdev)
>> +{
>> +	dev_dbg(sdev->dev, "loading firmware\n");
> Noise.
> Better to introduce a trace points and drop all these kind of messages.

it's not that bad, most people understand what dmesg is, it happens once 
and only if you have dynamic debug.

At some point we also thought about something like drm_debug where you 
can specify which parts you are interested in, in addition to the 
verbosity level.

>
>> +
>> +	if (sdev->ops->load_firmware)
>> +		return sdev->ops->load_firmware(sdev);
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(snd_sof_load_firmware);