linux-kernel - Re: [PATCH] ASoC: codecs: Max98095: Fix logging of hardware revision.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <20110621100614.GA22177@opensource.wolfsonmicro.com>
Date:	Tue, 21 Jun 2011 11:06:15 +0100
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	Taylor Hutt <thutt@...gle.com>
Cc:	Liam Girdwood <lrg@...mlogic.co.uk>,
	Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.de>,
	Peter Hsiang <Peter.Hsiang@...im-ic.com>,
	Olof Johansson <olofj@...omium.org>,
	alsa-devel@...a-project.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ASoC: codecs: Max98095: Fix logging of hardware revision.

On Mon, Jun 20, 2011 at 08:22:32PM -0700, Taylor Hutt wrote:
> On Mon, Jun 20, 2011 at 5:23 PM, Mark Brown <

> > Don't include a series of random unrelated changes in a single patch,
> > split them up into separate patches.  This makes review much easier if
> > nothing else.  There's no overlap at all between this change and the one
> > above.  The change is sensible.

> Ok, fine.  Seemed trivial enough and didn't seem like the code churn for
> another patch was warranted.
> But, ok.

Like I say it's for review - it's much easier to look at a diff and
verify that it does exactly one thing than it is to verify that it does
a series of unrelated things, that all of them got covered, that there's
no unexpected additional changes and that all of them are complete.

> > >       ret = snd_soc_read(codec, M98095_0FF_REV_ID);
> > >       if (ret < 0) {
> > > -             dev_err(codec->dev, "Failed to read device revision: %d\n",
> > > +             dev_err(codec->dev, "Failure reading hardware revision:
> > %d\n",
> > >                       ret);

> > You've also got this again unrelated change which isn't mentioned in the
> > changelog at all.
> This is part of the change for the hardware revision, and it seems pretty

> clear that they're related to me.  The text of the two output messages are
> now more aligned, and they are both related to reporting the hardware
> revision.

You're doing three things here - you're changing the revision that's
printed, you're rewriting the text that's output for some reason and
you're changing the way the number of DAIs is stored and you only
mentioned two of those.  My first thought when I saw this change, just
looking at the shape of the diff rather than reading the text of the
message, was that it didn't belong and I needed to slow down and look in
more detail.  Had it been mentioned in the log I would have been
expecting to see some random text only updates, making this less of a
surprise.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/