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

[Vinod Koul <vinod.koul@intel.com>]

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

On Fri, Aug 14, 2015 at 11:03:13PM +0100, Mark Brown wrote:
> 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.
sorry about that, will fix

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

yes that a good idea, will drop this here.

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

Not yet. So we will add this and few other stuff in topology manifest vendor
data for Intel. So Topology Kernel ABI will not get modified with this.
Yes we need to start adding these data sets to Kernel ABI, but I am planning
to add that, one shot at the end of SKL cycle so that we don't have to modify
ABI defines.

> 
> > +	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 :(

Will fix this

> 
> > +	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 :(

Yes, will fix

> 
> > +	/* Index is for each config load */
> > +	ret = snd_soc_tplg_component_load(&platform->component, &skl_tplg_ops, fw, 0);
> 
> Which index?
The last arg of snd_soc_tplg_component_load() is id which is set as
tplg.req_index = id;

So the comment tries to explain how last 0 index is added. We have only one
load so we will be always 0 index


Thanks
-- 
~Vinod

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

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