| Message ID | 1394475355-7069-1-git-send-email-ben.dooks@codethink.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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.
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
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.
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 ;)
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.
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
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.
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;
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(-)