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

Andy Shevchenko
2018-12-11T22:38:29Z

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