[PATCH] ak4642: show error if register write fails

[PATCH] ak4642: show error if register write fails

[Ben Dooks]

The calls to snd_soc_update_bits() and snd_soc_write() return error
codes if the calls fails. The ak4642 driver discards these errors
which means that the user is not informed if there is an issue with
the bus providing the register access.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 sound/soc/codecs/ak4642.c | 62 ++++++++++++++++++++++++++++++++++-------------
 1 file changed, 45 insertions(+), 17 deletions(-)

diff --git a/sound/soc/codecs/ak4642.c b/sound/soc/codecs/ak4642.c
index 4717fa9..15795f6 100644
--- a/sound/soc/codecs/ak4642.c
+++ b/sound/soc/codecs/ak4642.c
@@ -225,6 +225,34 @@ static const struct reg_default ak4648_reg[] = {
 	{ 36, 0x00 }, { 37, 0x88 }, { 38, 0x88 }, { 39, 0x08 },
 };
 
+/* wrapper functions to show any errors to updating register values */
+
+static inline int ak4642_update_bits(struct snd_soc_codec *codec,
+				     unsigned int reg,
+				     unsigned int mask, unsigned int val)
+{
+	int ret = snd_soc_update_bits(codec, reg, mask, val);
+
+	if (ret < 0) {
+		pr_info("%s: error %d writing %04x (%08x, mask %08x)\n",
+			codec->name, ret, reg, mask, val);
+	}
+
+	return ret;
+}
+
+static inline int ak4642_soc_write(struct snd_soc_codec *codec,
+				   unsigned int reg, unsigned int value)
+{
+	int ret = snd_soc_write(codec, reg, value);
+
+	if (ret < 0)
+		pr_info("%s: error %d writing %04x to %08x\n",
+			codec->name, ret, reg, value);
+
+	return ret;
+}
+
 static int ak4642_dai_startup(struct snd_pcm_substream *substream,
 			      struct snd_soc_dai *dai)
 {
@@ -242,8 +270,8 @@ static int ak4642_dai_startup(struct snd_pcm_substream *substream,
 		 * This operation came from example code of
 		 * "ASAHI KASEI AK4642" (japanese) manual p97.
 		 */
-		snd_soc_write(codec, L_IVC, 0x91); /* volume */
-		snd_soc_write(codec, R_IVC, 0x91); /* volume */
+		ak4642_soc_write(codec, L_IVC, 0x91); /* volume */
+		ak4642_soc_write(codec, R_IVC, 0x91); /* volume */
 	} else {
 		/*
 		 * start stereo input
@@ -258,11 +286,11 @@ static int ak4642_dai_startup(struct snd_pcm_substream *substream,
 		 * This operation came from example code of
 		 * "ASAHI KASEI AK4642" (japanese) manual p94.
 		 */
-		snd_soc_update_bits(codec, SG_SL1, PMMP | MGAIN0, PMMP | MGAIN0);
-		snd_soc_write(codec, TIMER, ZTM(0x3) | WTM(0x3));
-		snd_soc_write(codec, ALC_CTL1, ALC | LMTH0);
-		snd_soc_update_bits(codec, PW_MGMT1, PMADL, PMADL);
-		snd_soc_update_bits(codec, PW_MGMT3, PMADR, PMADR);
+		ak4642_update_bits(codec, SG_SL1, PMMP | MGAIN0, PMMP | MGAIN0);
+		ak4642_soc_write(codec, TIMER, ZTM(0x3) | WTM(0x3));
+		ak4642_soc_write(codec, ALC_CTL1, ALC | LMTH0);
+		ak4642_update_bits(codec, PW_MGMT1, PMADL, PMADL);
+		ak4642_update_bits(codec, PW_MGMT3, PMADR, PMADR);
 	}
 
 	return 0;
@@ -277,9 +305,9 @@ static void ak4642_dai_shutdown(struct snd_pcm_substream *substream,
 	if (is_play) {
 	} else {
 		/* stop stereo input */
-		snd_soc_update_bits(codec, PW_MGMT1, PMADL, 0);
-		snd_soc_update_bits(codec, PW_MGMT3, PMADR, 0);
-		snd_soc_update_bits(codec, ALC_CTL1, ALC, 0);
+		ak4642_update_bits(codec, PW_MGMT1, PMADL, 0);
+		ak4642_update_bits(codec, PW_MGMT3, PMADR, 0);
+		ak4642_update_bits(codec, ALC_CTL1, ALC, 0);
 	}
 }
 
@@ -312,7 +340,7 @@ static int ak4642_dai_set_sysclk(struct snd_soc_dai *codec_dai,
 	default:
 		return -EINVAL;
 	}
-	snd_soc_update_bits(codec, MD_CTL1, PLL_MASK, pll);
+	ak4642_update_bits(codec, MD_CTL1, PLL_MASK, pll);
 
 	return 0;
 }
@@ -339,8 +367,8 @@ static int ak4642_dai_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 	default:
 		return -EINVAL;
 	}
-	snd_soc_update_bits(codec, PW_MGMT2, MS | MCKO | PMPLL, data);
-	snd_soc_update_bits(codec, MD_CTL1, BCKO_MASK, bcko);
+	ak4642_update_bits(codec, PW_MGMT2, MS | MCKO | PMPLL, data);
+	ak4642_update_bits(codec, MD_CTL1, BCKO_MASK, bcko);
 
 	/* format type */
 	data = 0;
@@ -357,7 +385,7 @@ static int ak4642_dai_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 	default:
 		return -EINVAL;
 	}
-	snd_soc_update_bits(codec, MD_CTL1, DIF_MASK, data);
+	ak4642_update_bits(codec, MD_CTL1, DIF_MASK, data);
 
 	return 0;
 }
@@ -411,7 +439,7 @@ static int ak4642_dai_hw_params(struct snd_pcm_substream *substream,
 	default:
 		return -EINVAL;
 	}
-	snd_soc_update_bits(codec, MD_CTL2, FS_MASK, rate);
+	ak4642_update_bits(codec, MD_CTL2, FS_MASK, rate);
 
 	return 0;
 }
@@ -421,10 +449,10 @@ static int ak4642_set_bias_level(struct snd_soc_codec *codec,
 {
 	switch (level) {
 	case SND_SOC_BIAS_OFF:
-		snd_soc_write(codec, PW_MGMT1, 0x00);
+		ak4642_soc_write(codec, PW_MGMT1, 0x00);
 		break;
 	default:
-		snd_soc_update_bits(codec, PW_MGMT1, PMVCM, PMVCM);
+		ak4642_update_bits(codec, PW_MGMT1, PMVCM, PMVCM);
 		break;
 	}
 	codec->dapm.bias_level = level;
-- 
1.9.0

Re: [PATCH] ak4642: show error if register write fails

[Mark Brown]

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

On Mon, Mar 10, 2014 at 06:15:55PM +0000, Ben Dooks wrote:

> +/* wrapper functions to show any errors to updating register values */
> +
> +static inline int ak4642_update_bits(struct snd_soc_codec *codec,
> +				     unsigned int reg,
> +				     unsigned int mask, unsigned int val)
> +{
> +	int ret = snd_soc_update_bits(codec, reg, mask, val);
> +
> +	if (ret < 0) {
> +		pr_info("%s: error %d writing %04x (%08x, mask %08x)\n",
> +			codec->name, ret, reg, mask, val);

Two things here.  One is that this should be a dev_err() and the other
is that if this is worth doing shouldn't it just be in the core - I see
nothing driver specific here?

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

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

Re: [PATCH] ak4642: show error if register write fails

[Ben Dooks]

On 10/03/14 23:40, Mark Brown wrote:
> On Mon, Mar 10, 2014 at 06:15:55PM +0000, Ben Dooks wrote:
>
>> +/* wrapper functions to show any errors to updating register values */
>> +
>> +static inline int ak4642_update_bits(struct snd_soc_codec *codec,
>> +				     unsigned int reg,
>> +				     unsigned int mask, unsigned int val)
>> +{
>> +	int ret = snd_soc_update_bits(codec, reg, mask, val);
>> +
>> +	if (ret < 0) {
>> +		pr_info("%s: error %d writing %04x (%08x, mask %08x)\n",
>> +			codec->name, ret, reg, mask, val);
>
> Two things here.  One is that this should be a dev_err() and the other
> is that if this is worth doing shouldn't it just be in the core - I see
> nothing driver specific here?

Sorry, didn't see a device in "struct snd_soc_codec *codec" so I went
for a printk (although it was pr_info instead of pr_err).

I did think if this should be something in the snd_soc_update_bits
and snd_soc_write() calls after sending the patch.

If you think that changing the two snd_soc calls to print errors
when anything bad happens then that would also be a good idea then
I can send a patch for that.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH] ak4642: show error if register write fails

[Mark Brown]

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

On Tue, Mar 11, 2014 at 11:18:30AM +0000, Ben Dooks wrote:
> On 10/03/14 23:40, Mark Brown wrote:

> >Two things here.  One is that this should be a dev_err() and the other
> >is that if this is worth doing shouldn't it just be in the core - I see
> >nothing driver specific here?

> Sorry, didn't see a device in "struct snd_soc_codec *codec" so I went
> for a printk (although it was pr_info instead of pr_err).

codec->dev.

> If you think that changing the two snd_soc calls to print errors
> when anything bad happens then that would also be a good idea then
> I can send a patch for that.

That would be better, yes.

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

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

Re: [PATCH] ak4642: show error if register write fails

[Lars-Peter Clausen]

On 03/11/2014 12:21 PM, Mark Brown wrote:
> On Tue, Mar 11, 2014 at 11:18:30AM +0000, Ben Dooks wrote:
>> On 10/03/14 23:40, Mark Brown wrote:
>
>>> Two things here.  One is that this should be a dev_err() and the other
>>> is that if this is worth doing shouldn't it just be in the core - I see
>>> nothing driver specific here?
>
>> Sorry, didn't see a device in "struct snd_soc_codec *codec" so I went
>> for a printk (although it was pr_info instead of pr_err).
>
> codec->dev.
>
>> If you think that changing the two snd_soc calls to print errors
>> when anything bad happens then that would also be a good idea then
>> I can send a patch for that.
>
> That would be better, yes.

In my opinion it's better to pass the error on to the upper levels. E.g. if 
userspace opens the PCM device and there is an IO error in the startup 
callback then that error should be passed on to the userspace application 
rather than doing a out of band error reporting and adding a entry to the 
kernel log.

- Lars

Re: [PATCH] ak4642: show error if register write fails

[Ben Dooks]

On 11/03/14 13:59, Lars-Peter Clausen wrote:
> On 03/11/2014 12:21 PM, Mark Brown wrote:
>> On Tue, Mar 11, 2014 at 11:18:30AM +0000, Ben Dooks wrote:
>>> On 10/03/14 23:40, Mark Brown wrote:
>>
>>>> Two things here.  One is that this should be a dev_err() and the other
>>>> is that if this is worth doing shouldn't it just be in the core - I see
>>>> nothing driver specific here?
>>
>>> Sorry, didn't see a device in "struct snd_soc_codec *codec" so I went
>>> for a printk (although it was pr_info instead of pr_err).
>>
>> codec->dev.
>>
>>> If you think that changing the two snd_soc calls to print errors
>>> when anything bad happens then that would also be a good idea then
>>> I can send a patch for that.
>>
>> That would be better, yes.
>
> In my opinion it's better to pass the error on to the upper levels. E.g.
> if userspace opens the PCM device and there is an IO error in the
> startup callback then that error should be passed on to the userspace
> application rather than doing a out of band error reporting and adding a
> entry to the kernel log.

 From a grep, there doesn't seem to be much in the way of error
checking in a number of the codec drivers.


-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH] ak4642: show error if register write fails

[Lars-Peter Clausen]

On 03/11/2014 03:17 PM, Ben Dooks wrote:
> On 11/03/14 13:59, Lars-Peter Clausen wrote:
>> On 03/11/2014 12:21 PM, Mark Brown wrote:
>>> On Tue, Mar 11, 2014 at 11:18:30AM +0000, Ben Dooks wrote:
>>>> On 10/03/14 23:40, Mark Brown wrote:
>>>
>>>>> Two things here.  One is that this should be a dev_err() and the other
>>>>> is that if this is worth doing shouldn't it just be in the core - I see
>>>>> nothing driver specific here?
>>>
>>>> Sorry, didn't see a device in "struct snd_soc_codec *codec" so I went
>>>> for a printk (although it was pr_info instead of pr_err).
>>>
>>> codec->dev.
>>>
>>>> If you think that changing the two snd_soc calls to print errors
>>>> when anything bad happens then that would also be a good idea then
>>>> I can send a patch for that.
>>>
>>> That would be better, yes.
>>
>> In my opinion it's better to pass the error on to the upper levels. E.g.
>> if userspace opens the PCM device and there is an IO error in the
>> startup callback then that error should be passed on to the userspace
>> application rather than doing a out of band error reporting and adding a
>> entry to the kernel log.
>
>  From a grep, there doesn't seem to be much in the way of error
> checking in a number of the codec drivers.
>
>

That doesn't mean it's the right thing to do no error checking ;)

Re: [PATCH] ak4642: show error if register write fails

[Mark Brown]

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

On Tue, Mar 11, 2014 at 02:59:55PM +0100, Lars-Peter Clausen wrote:
> On 03/11/2014 12:21 PM, Mark Brown wrote:

> >>If you think that changing the two snd_soc calls to print errors
> >>when anything bad happens then that would also be a good idea then
> >>I can send a patch for that.

> >That would be better, yes.

> In my opinion it's better to pass the error on to the upper levels.
> E.g. if userspace opens the PCM device and there is an IO error in
> the startup callback then that error should be passed on to the
> userspace application rather than doing a out of band error
> reporting and adding a entry to the kernel log.

It would, overall, be much better to be passing errors back.  However
what's actually happening now is I/O errors are routinely ignored and
our error handling is somewhat shaky (and realistically it's hard to
know what to do a lot of the time when I/O with the device is failing).
Given that improving the diagnostics seems like it's going in the right
direction, we can always remove this later.

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

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

Re: [PATCH] ak4642: show error if register write fails

[Takashi Iwai]

At Tue, 11 Mar 2014 14:21:38 +0000,
Mark Brown wrote:
> 
> On Tue, Mar 11, 2014 at 02:59:55PM +0100, Lars-Peter Clausen wrote:
> > On 03/11/2014 12:21 PM, Mark Brown wrote:
> 
> > >>If you think that changing the two snd_soc calls to print errors
> > >>when anything bad happens then that would also be a good idea then
> > >>I can send a patch for that.
> 
> > >That would be better, yes.
> 
> > In my opinion it's better to pass the error on to the upper levels.
> > E.g. if userspace opens the PCM device and there is an IO error in
> > the startup callback then that error should be passed on to the
> > userspace application rather than doing a out of band error
> > reporting and adding a entry to the kernel log.
> 
> It would, overall, be much better to be passing errors back.  However
> what's actually happening now is I/O errors are routinely ignored and
> our error handling is somewhat shaky (and realistically it's hard to
> know what to do a lot of the time when I/O with the device is failing).
> Given that improving the diagnostics seems like it's going in the right
> direction, we can always remove this later.

This should be decided for each case, and I agree with the usefulness
of error logging for this particular case.  The I/O errors are
basically exceptions, and shouldn't happen in normal operations.

That said, if the driver behavior depends on such lowlevel errors,
its design is wrong, must be fixed in anyway.

Still an open question is whether it should be dev_err() or
dev_dbg().  Ideally speaking, dev_dbg() should be used, while
dev_err() would be more practical.


Takashi

Re: [PATCH] ak4642: show error if register write fails

[Mark Brown]

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

On Tue, Mar 11, 2014 at 04:15:01PM +0100, Takashi Iwai wrote:

> This should be decided for each case, and I agree with the usefulness
> of error logging for this particular case.  The I/O errors are
> basically exceptions, and shouldn't happen in normal operations.

Quite.

> That said, if the driver behavior depends on such lowlevel errors,
> its design is wrong, must be fixed in anyway.

Indeed, this shouldn't be something that happens in normal operation.
Usually you'd see this during board bringup if you'd got something like
I2C, regulators or reset GPIOs hooked up wrongly or missing, though you
can see things like incorrectly handled ramp or reset times triggering
it too.  By the time the system is actually in use it shouldn't happen
which is why we've not been having problems with ignoring errors.

> Still an open question is whether it should be dev_err() or
> dev_dbg().  Ideally speaking, dev_dbg() should be used, while
> dev_err() would be more practical.

I think given that it's generally a sign of substantial hardware failure
dev_err() is justified, probably the user will be noticing other issues
and it'll help a lot with root causing.  I'd guess Ben had something
like one of the situations above and needed this to figure it out.

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

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