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

From: Fabio Estevam <festevam@gmail.com>
To: Benjamin Rood <benjaminjrood@gmail.com>
Cc: Linux-ALSA <alsa-devel@alsa-project.org>,
	Takashi Iwai <tiwai@suse.com>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Mark Brown <broonie@kernel.org>,
	dmacdonald@curbellmedical.com
Subject: Re: [PATCH] ASoC: sgtl5000: set DAP_AVC_CTRL register to correct default value on probe
Date: Fri, 19 Feb 2021 15:37:51 -0300	[thread overview]
Message-ID: <CAOMZO5CtE9OrthMpGAcTHrSy_+np_yLUO1M9SVjTrsjz-gWbNA@mail.gmail.com> (raw)
In-Reply-To: <20210219183308.GA2117@ubuntu-dev>

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>