Re: [PATCH 5/9] ASoC: Intel: Skylake: Add topology core init and handlers

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 1386 bytes --]

On Sat, Aug 08, 2015 at 01:06:20AM +0530, Subhransu S. Prusty wrote:

> +	struct skl_pipeline  *ppl;

> +	pipe  = devm_kzalloc(dev, sizeof(*pipe), GFP_KERNEL);

There's lots of random spaces in this code so far and some further down
too.

> +	struct skl_dfw_module *dfw_config = (struct skl_dfw_module *) tplg_w->priv.data;
> +
> +	skl_tplg_dump_widget_info(bus->dev, dfw_config, tplg_w);

Consider implementing debugfs for this...

> +/* This will be read from topology manifest, currently defined here */
> +#define SKL_MAX_MCPS 30000000
> +#define SKL_FW_MAX_MEM 1000000

Oh dear, this sounds like we need another ABI update to add these
manifests?

> +	dev_dbg(bus->dev, "In%s req firmware topology bin\n",  __func__);

Those "In%s" are going to come out as the function name prefixed with
In.  What's that for?  It's just going to make the logs harder to read
as far as I can tell :(

> +	const struct firmware *fw;

> +	ret = request_firmware(&fw, "dfw_sst.bin", bus->dev);
> +	if (fw == NULL) {
> +		dev_err(bus->dev, "config firmware request failed with %d\n", ret);
> +		return ret;
> +	}

We're ignoring the return value (which is what we should be paying
attention to here) and checking to see if fw is NULL but fw wasn't
initialized :(

> +	/* Index is for each config load */
> +	ret = snd_soc_tplg_component_load(&platform->component, &skl_tplg_ops, fw, 0);

Which index?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]