[PATCH] ASoC: codecs: Max98095: Fix logging of hardware revision.

[PATCH] ASoC: codecs: Max98095: Fix logging of hardware revision.

[Taylor Hutt]

The base hardware revision of the Maxim 98095 part is 0x40; the code
which outputs the revision of the hardware has been updated to
properly use uppercase alphabetic values for the revision numbers.

Also, the use of a constant for the length 'max98095_dai' has been
replaced with ARRAY_SIZE().

Signed-off-by: Taylor Hutt <thutt@chromium.org>
---
 sound/soc/codecs/max98095.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/sound/soc/codecs/max98095.c b/sound/soc/codecs/max98095.c
index e1d282d..9a793b3 100644
--- a/sound/soc/codecs/max98095.c
+++ b/sound/soc/codecs/max98095.c
@@ -2261,11 +2261,11 @@ static int max98095_probe(struct snd_soc_codec *codec)
 
 	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);
 		goto err_access;
 	}
-	dev_info(codec->dev, "revision %c\n", ret + 'A');
+	dev_info(codec->dev, "Hardware revision: %c\n", ret - 0x40 + 'A');
 
 	snd_soc_write(codec, M98095_097_PWR_SYS, M98095_PWRSV);
 
@@ -2342,8 +2342,8 @@ static int max98095_i2c_probe(struct i2c_client *i2c,
 	max98095->control_data = i2c;
 	max98095->pdata = i2c->dev.platform_data;
 
-	ret = snd_soc_register_codec(&i2c->dev,
-			&soc_codec_dev_max98095, &max98095_dai[0], 3);
+	ret = snd_soc_register_codec(&i2c->dev, &soc_codec_dev_max98095,
+				     max98095_dai, ARRAY_SIZE(max98095_dai));
 	if (ret < 0)
 		kfree(max98095);
 	return ret;
-- 
1.7.3.1

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

[Liam Girdwood]

On Mon, 2011-06-20 at 11:54 -0700, Taylor Hutt wrote:
> The base hardware revision of the Maxim 98095 part is 0x40; the code
> which outputs the revision of the hardware has been updated to
> properly use uppercase alphabetic values for the revision numbers.
> 
> Also, the use of a constant for the length 'max98095_dai' has been
> replaced with ARRAY_SIZE().
> 
> Signed-off-by: Taylor Hutt <thutt@chromium.org>

Acked-by: Liam Girdwood <lrg@ti.com>

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

[Mark Brown]

On Mon, Jun 20, 2011 at 11:54:32AM -0700, Taylor Hutt wrote:
> The base hardware revision of the Maxim 98095 part is 0x40; the code
> which outputs the revision of the hardware has been updated to
> properly use uppercase alphabetic values for the revision numbers.

Are you sure that this is true for all devices that might be supported
by the driver (I'm guessing there may be other variants)?  There's often
a drift between silicon and package revisions which gets papered over by
datasheets and ignored by drivers.

> Also, the use of a constant for the length 'max98095_dai' has been
> replaced with ARRAY_SIZE().

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.

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

RE: [PATCH] ASoC: codecs: Max98095: Fix logging of hardware revision.

[Peter Hsiang]

On Mon, Jun 20, 2011, Mark Brown wrote:
> On Mon, Jun 20, 2011 at 11:54:32AM -0700, Taylor Hutt wrote:
> > The base hardware revision of the Maxim 98095 part is 0x40; the code
> > which outputs the revision of the hardware has been updated to
> > properly use uppercase alphabetic values for the revision numbers.
> 
> Are you sure that this is true for all devices that might be supported
> by the driver (I'm guessing there may be other variants)?  There's
> often a drift between silicon and package revisions which gets papered 
> over by datasheets and ignored by drivers.

I checked with hardware engineering and was told 0x40=RevA, 0x41=RevB..

Would a raw value or the use of a 0x3F bit mask be more acceptable?

Peter

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

[Mark Brown]

On Mon, Jun 20, 2011 at 05:43:58PM -0700, Peter Hsiang wrote:

> I checked with hardware engineering and was told 0x40=RevA, 0x41=RevB..

> Would a raw value or the use of a 0x3F bit mask be more acceptable?

I don't mind (though it does seem like the high bit isn't part of the
actual data), it was just that it's a common error to assume that die
revisions and package revisions correspond directly.

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

[Mark Brown]

On Tue, Jun 21, 2011 at 02:10:39AM +0100, Mark Brown wrote:
> On Mon, Jun 20, 2011 at 05:43:58PM -0700, Peter Hsiang wrote:

> > I checked with hardware engineering and was told 0x40=RevA, 0x41=RevB..

> > Would a raw value or the use of a 0x3F bit mask be more acceptable?

> I don't mind (though it does seem like the high bit isn't part of the
> actual data), it was just that it's a common error to assume that die
> revisions and package revisions correspond directly.

This means if you're OK with it I can apply the patch as-is, BTW.

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

[Mark Brown]

On Tue, Jun 21, 2011 at 07:52:41AM -0700, Taylor Hutt wrote:

> Please let me know if you'd like me to continue splitting this patch into
> two changes.

It won't hurt if you don't need to respin for some other reason, but
it's not 100% essential.

RE: [PATCH] ASoC: codecs: Max98095: Fix logging of hardware revision.

[Peter Hsiang]

On Tue, Jun 21, 2011, Mark Brown wrote:
> On Tue, Jun 21, 2011 at 02:10:39AM +0100, Mark Brown wrote:
> > On Mon, Jun 20, 2011 at 05:43:58PM -0700, Peter Hsiang wrote:
> 
> > > I checked with hardware engineering and was told 0x40=RevA,
> 0x41=RevB..
> 
> This means if you're OK with it I can apply the patch as-is, BTW.

Mark, I am ok with this.  Thanks!

RE: [PATCH] ASoC: codecs: Max98095: Fix logging of hardware revision.

[Peter Hsiang]

On Tue, Jun 21, 2011, Mark Brown wrote:
> On Tue, Jun 21, 2011 at 02:10:39AM +0100, Mark Brown wrote:
> > On Mon, Jun 20, 2011 at 05:43:58PM -0700, Peter Hsiang wrote:
> 
> > > I checked with hardware engineering and was told 0x40=RevA,
> 0x41=RevB..
> 
> This means if you're OK with it I can apply the patch as-is, BTW.

Acked-by: Peter Hsiang <peter.hsiang@maxim-ic.com>

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

[Mark Brown]

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.

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

[Mark Brown]

On Mon, Jun 20, 2011 at 11:54:32AM -0700, Taylor Hutt wrote:
> The base hardware revision of the Maxim 98095 part is 0x40; the code
> which outputs the revision of the hardware has been updated to
> properly use uppercase alphabetic values for the revision numbers.
> 
> Also, the use of a constant for the length 'max98095_dai' has been
> replaced with ARRAY_SIZE().

Applied.  Like I said in my previous mail please do try to split your
patches up better and make sure the changelogs are accurate.