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

[Mark Brown <broonie@kernel.org>]

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

On Tue, Dec 11, 2018 at 05:54:02PM -0600, Pierre-Louis Bossart wrote:
> On 12/11/18 4:38 PM, Andy Shevchenko wrote:
> > On Tue, Dec 11, 2018 at 03:23:13PM -0600, Pierre-Louis Bossart wrote:

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

> Darn, we missed this one. I thought we were using W=1 on github but we
> aren't, this will be fixed.

> Though W=1 doesn't report this one, so need to re-inspect this. Thanks for
> the sighting.

This is one reason not to unconditionally init stuff on declaration -
it masks some warnings.

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

> it's not that bad, most people understand what dmesg is, it happens once and
> only if you have dynamic debug.

Then everyone adds their print on boot and the log gets huge (and slow
if you push debug through serial during development).  As a rule of
thumb I tend to suggest that if you're not reporting something you
discovered at runtime there's probably already other ways of getting
equivalent trace.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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