Hi Benjamin, On Fri, Feb 19, 2021 at 3:33 PM Benjamin Rood <benjaminjrood@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxx> Thanks for the fix: Reviewed-by: Fabio Estevam <festevam@xxxxxxxxx>
