[PATCH] ASoC: sgtl5000: set DAP_AVC_CTRL register to correct default value on probe

[PATCH] ASoC: sgtl5000: set DAP_AVC_CTRL register to correct default value on probe

[Benjamin Rood]

According to the SGTL5000 datasheet [1], the DAP_AVC_CTRL register has
the following bit field definitions:

| BITS  | FIELD       | RW | RESET | DEFINITION                        |
| 15    | RSVD        | RO | 0x0   | Reserved                          |
| 14    | RSVD        | RW | 0x1   | Reserved                          |
| 13:12 | MAX_GAIN    | RW | 0x1   | Max Gain of AVC in expander mode  |
| 11:10 | RSVD        | RO | 0x0   | Reserved                          |
| 9:8   | LBI_RESP    | RW | 0x1   | Integrator Response               |
| 7:6   | RSVD        | RO | 0x0   | Reserved                          |
| 5     | HARD_LMT_EN | RW | 0x0   | Enable hard limiter mode          |
| 4:1   | RSVD        | RO | 0x0   | Reserved                          |
| 0     | EN          | RW | 0x0   | Enable/Disable AVC                |

The original default value written to the DAP_AVC_CTRL register during
sgtl5000_i2c_probe() was 0x0510.  This would incorrectly write values to
bits 4 and 10, which are defined as RESERVED.  It would also not set
bits 12 and 14 to their correct RESET values of 0x1, and instead set
them to 0x0.  While the DAP_AVC module is effectively disabled because
the EN bit is 0, this default value is still writing invalid values to
registers that are marked as read-only and RESERVED as well as not
setting bits 12 and 14 to their correct default values as defined by the
datasheet.

The correct value that should be written to the DAP_AVC_CTRL register is
0x5100, which configures the register bits to the default values defined
by the datasheet, and prevents any writes to bits defined as
'read-only'.  Generally speaking, it is best practice to NOT attempt to
write values to registers/bits defined as RESERVED, as it generally
produces unwanted/undefined behavior, or errors.

Also, all credit for this patch should go to my colleague Dan MacDonald
<dmacdonald@curbellmedical.com> for finding this error in the first
place.

[1] https://www.nxp.com/docs/en/data-sheet/SGTL5000.pdf

Signed-off-by: Benjamin Rood <benjaminjrood@gmail.com>
---
 sound/soc/codecs/sgtl5000.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
index 4d6ff8114622..4c0e87e22b97 100644
--- a/sound/soc/codecs/sgtl5000.c
+++ b/sound/soc/codecs/sgtl5000.c
@@ -71,7 +71,7 @@ static const struct reg_default sgtl5000_reg_defaults[] = {
 	{ SGTL5000_DAP_EQ_BASS_BAND4,		0x002f },
 	{ SGTL5000_DAP_MAIN_CHAN,		0x8000 },
 	{ SGTL5000_DAP_MIX_CHAN,		0x0000 },
-	{ SGTL5000_DAP_AVC_CTRL,		0x0510 },
+	{ SGTL5000_DAP_AVC_CTRL,		0x5100 },
 	{ SGTL5000_DAP_AVC_THRESHOLD,		0x1473 },
 	{ SGTL5000_DAP_AVC_ATTACK,		0x0028 },
 	{ SGTL5000_DAP_AVC_DECAY,		0x0050 },
-- 
2.25.1

Re: [PATCH] ASoC: sgtl5000: set DAP_AVC_CTRL register to correct default value on probe

[Fabio Estevam]

Hi Benjamin,

On Fri, Feb 19, 2021 at 3:33 PM Benjamin Rood <benjaminjrood@gmail.com> wrote:
>
> According to the SGTL5000 datasheet [1], the DAP_AVC_CTRL register has
> the following bit field definitions:
>
> | BITS  | FIELD       | RW | RESET | DEFINITION                        |
> | 15    | RSVD        | RO | 0x0   | Reserved                          |
> | 14    | RSVD        | RW | 0x1   | Reserved                          |
> | 13:12 | MAX_GAIN    | RW | 0x1   | Max Gain of AVC in expander mode  |
> | 11:10 | RSVD        | RO | 0x0   | Reserved                          |
> | 9:8   | LBI_RESP    | RW | 0x1   | Integrator Response               |
> | 7:6   | RSVD        | RO | 0x0   | Reserved                          |
> | 5     | HARD_LMT_EN | RW | 0x0   | Enable hard limiter mode          |
> | 4:1   | RSVD        | RO | 0x0   | Reserved                          |
> | 0     | EN          | RW | 0x0   | Enable/Disable AVC                |
>
> The original default value written to the DAP_AVC_CTRL register during
> sgtl5000_i2c_probe() was 0x0510.  This would incorrectly write values to
> bits 4 and 10, which are defined as RESERVED.  It would also not set
> bits 12 and 14 to their correct RESET values of 0x1, and instead set
> them to 0x0.  While the DAP_AVC module is effectively disabled because
> the EN bit is 0, this default value is still writing invalid values to
> registers that are marked as read-only and RESERVED as well as not
> setting bits 12 and 14 to their correct default values as defined by the
> datasheet.
>
> The correct value that should be written to the DAP_AVC_CTRL register is
> 0x5100, which configures the register bits to the default values defined
> by the datasheet, and prevents any writes to bits defined as
> 'read-only'.  Generally speaking, it is best practice to NOT attempt to
> write values to registers/bits defined as RESERVED, as it generally
> produces unwanted/undefined behavior, or errors.
>
> Also, all credit for this patch should go to my colleague Dan MacDonald
> <dmacdonald@curbellmedical.com> for finding this error in the first
> place.
>
> [1] https://www.nxp.com/docs/en/data-sheet/SGTL5000.pdf
>
> Signed-off-by: Benjamin Rood <benjaminjrood@gmail.com>

Thanks for the fix:

Reviewed-by: Fabio Estevam <festevam@gmail.com>

Re: [PATCH] ASoC: sgtl5000: set DAP_AVC_CTRL register to correct default value on probe

[Mark Brown]

On Fri, 19 Feb 2021 13:33:08 -0500, Benjamin Rood wrote:
> According to the SGTL5000 datasheet [1], the DAP_AVC_CTRL register has
> the following bit field definitions:
> 
> | BITS  | FIELD       | RW | RESET | DEFINITION                        |
> | 15    | RSVD        | RO | 0x0   | Reserved                          |
> | 14    | RSVD        | RW | 0x1   | Reserved                          |
> | 13:12 | MAX_GAIN    | RW | 0x1   | Max Gain of AVC in expander mode  |
> | 11:10 | RSVD        | RO | 0x0   | Reserved                          |
> | 9:8   | LBI_RESP    | RW | 0x1   | Integrator Response               |
> | 7:6   | RSVD        | RO | 0x0   | Reserved                          |
> | 5     | HARD_LMT_EN | RW | 0x0   | Enable hard limiter mode          |
> | 4:1   | RSVD        | RO | 0x0   | Reserved                          |
> | 0     | EN          | RW | 0x0   | Enable/Disable AVC                |
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/1] ASoC: sgtl5000: set DAP_AVC_CTRL register to correct default value on probe
      commit: d74fcdc51afd431ca9d956e032e14d12f0ee4153

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark