[alsa-devel] [PATCH] ASoC: codecs: Max98095: Fix logging of hardware revision.
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;
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
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.
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
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.
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.
On Tue, Jun 21, 2011 at 4:55 AM, Mark Brown < broonie@opensource.wolfsonmicro.com> 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..
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.
Please let me know if you'd like me to continue splitting this patch into two changes.
(Also, informationally, max98088.c also uses a constant instead of ARRAY_SIZE in the same context. I don't know if it suffers from the same hardware revision output issue, as I don't have that hardware...)
thutt
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!
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
On Mon, Jun 20, 2011 at 5:23 PM, Mark Brown < broonie@opensource.wolfsonmicro.com> 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.
Got this information from Peter, as he notes below. I'm just the messenger on this particular change; I'm merely trying to upstream the information he provided me. Whatever you folks want, I'll be happy to do.
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.
Ok, fine. Seemed trivial enough and didn't seem like the code churn for another patch was warranted. But, ok.
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.
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.
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.
participants (5)
-
Liam Girdwood -
Mark Brown -
Peter Hsiang -
Taylor Hutt -
Taylor Hutt