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

From: Mark Brown <broonie@kernel.org>
To: Vinod Koul <vinod.koul@intel.com>
Cc: alsa-devel@alsa-project.org, tiwai@suse.de,
	liam.r.girdwood@linux.intel.com, patches.audio@intel.com,
	Jeeja KP <jeeja.kp@intel.com>,
	"Subhransu S. Prusty" <subhransu.s.prusty@intel.com>
Subject: Re: [PATCH 5/9] ASoC: Intel: Skylake: Add topology core init and handlers
Date: Sat, 15 Aug 2015 10:00:58 -0700	[thread overview]
Message-ID: <20150815170058.GK10748@sirena.org.uk> (raw)
In-Reply-To: <20150815141658.GF13546@localhost>

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

On Sat, Aug 15, 2015 at 07:46:58PM +0530, Vinod Koul wrote:
> On Fri, Aug 14, 2015 at 11:03:13PM +0100, Mark Brown wrote:

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

Your comment isn't explaining that at all, sorry - it's making things
less clear.  It's peering into the implementation to translate ID to
index and even with s/id/index/ it's begging the question "ID/index is
what for each component load?".  If you're adding a comment here I'd
expect to see something like "Use the same ID each time because..."
which explains something that isn't in the code.

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

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