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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Tue, Dec 11, 2018 at 03:23:13PM -0600, Pierre-Louis Bossart wrote:
> From: Liam Girdwood <liam.r.girdwood@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`.

> +	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?

> +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.

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

-- 
With Best Regards,
Andy Shevchenko