| Message ID | 20221125163748.1288942-2-ckeepax@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 3d1bb6cc1a654c8693a85b1d262e610196edec8b |
| Headers | show |
| Series | [v2,1/2] ASoC: ops: Correct bounds check for second channel on SX controls | expand |
El vie, 25 nov 2022 a las 17:37, Charles Keepax (<ckeepax@opensource.cirrus.com>) escribió: > > The table in the datasheet actually shows the volume values in the wrong > order, with the two -3dB values being reversed. This appears to have > caused the lower of the two values to be used in the driver when the > higher should have been, correct this mixup. > > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com> > --- > > No changes since v1. > > sound/soc/codecs/cs42l51.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/sound/soc/codecs/cs42l51.c b/sound/soc/codecs/cs42l51.c > index 51721edd8f53c..e88d9ff95cdfc 100644 > --- a/sound/soc/codecs/cs42l51.c > +++ b/sound/soc/codecs/cs42l51.c > @@ -143,7 +143,7 @@ static const struct snd_kcontrol_new cs42l51_snd_controls[] = { > 0, 0xA0, 96, adc_att_tlv), > SOC_DOUBLE_R_SX_TLV("PGA Volume", > CS42L51_ALC_PGA_CTL, CS42L51_ALC_PGB_CTL, > - 0, 0x19, 30, pga_tlv), > + 0, 0x1A, 30, pga_tlv), The original patch where this control was added [1] already used 0x1A, however this was later changed to 0x19 in [2]. Your patch now reverts that change. Does this mean [2] was incorrect? If that is the case, shouldn't the commit message for this patch mention that it fixes [2] ? [1]: https://lore.kernel.org/all/20200918134317.22574-1-guille.rodriguez@gmail.com/ [2]: https://lore.kernel.org/all/20220602162119.3393857-7-ckeepax@opensource.cirrus.com/ Best regards, Guillermo Rodriguez Garcia guille.rodriguez@gmail.com
On Sat, Nov 26, 2022 at 12:15:10PM +0100, Guillermo Rodriguez Garcia wrote: > El vie, 25 nov 2022 a las 17:37, Charles Keepax > (<ckeepax@opensource.cirrus.com>) escribió: > > > > The table in the datasheet actually shows the volume values in the wrong > > order, with the two -3dB values being reversed. This appears to have > > caused the lower of the two values to be used in the driver when the > > higher should have been, correct this mixup. > > > > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com> > > --- > > > > No changes since v1. > > > > sound/soc/codecs/cs42l51.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/sound/soc/codecs/cs42l51.c b/sound/soc/codecs/cs42l51.c > > index 51721edd8f53c..e88d9ff95cdfc 100644 > > --- a/sound/soc/codecs/cs42l51.c > > +++ b/sound/soc/codecs/cs42l51.c > > @@ -143,7 +143,7 @@ static const struct snd_kcontrol_new cs42l51_snd_controls[] = { > > 0, 0xA0, 96, adc_att_tlv), > > SOC_DOUBLE_R_SX_TLV("PGA Volume", > > CS42L51_ALC_PGA_CTL, CS42L51_ALC_PGB_CTL, > > - 0, 0x19, 30, pga_tlv), > > + 0, 0x1A, 30, pga_tlv), > > The original patch where this control was added [1] already used 0x1A, > however this was later changed to 0x19 in [2]. Your patch now reverts > that change. Does this mean [2] was incorrect? If that is the case, > shouldn't the commit message for this patch mention that it fixes [2] > ? > > [1]: https://lore.kernel.org/all/20200918134317.22574-1-guille.rodriguez@gmail.com/ > [2]: https://lore.kernel.org/all/20220602162119.3393857-7-ckeepax@opensource.cirrus.com/ > Hmm... good digging, I didn't realise it was me who broke that. Apologies in that chain I went around and checked a bunch of SX controls to make sure they matched the datasheets, but it seems I got a bit confused by the weird ordering of the values in the datasheet. Since you have hardware would you be able to check, before we merge this revert? A simple check that writing 0 to the control sets the register value to 0x1A and writing 30 sets the register to 0x18 would suffice. Thanks, Charles
El lun, 28 nov 2022 a las 10:48, Charles Keepax (<ckeepax@opensource.cirrus.com>) escribió: > > On Sat, Nov 26, 2022 at 12:15:10PM +0100, Guillermo Rodriguez Garcia wrote: > > El vie, 25 nov 2022 a las 17:37, Charles Keepax > > (<ckeepax@opensource.cirrus.com>) escribió: > > > > > > The table in the datasheet actually shows the volume values in the wrong > > > order, with the two -3dB values being reversed. This appears to have > > > caused the lower of the two values to be used in the driver when the > > > higher should have been, correct this mixup. > > > > > > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com> > > > --- > > > > > > No changes since v1. > > > > > > sound/soc/codecs/cs42l51.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/sound/soc/codecs/cs42l51.c b/sound/soc/codecs/cs42l51.c > > > index 51721edd8f53c..e88d9ff95cdfc 100644 > > > --- a/sound/soc/codecs/cs42l51.c > > > +++ b/sound/soc/codecs/cs42l51.c > > > @@ -143,7 +143,7 @@ static const struct snd_kcontrol_new cs42l51_snd_controls[] = { > > > 0, 0xA0, 96, adc_att_tlv), > > > SOC_DOUBLE_R_SX_TLV("PGA Volume", > > > CS42L51_ALC_PGA_CTL, CS42L51_ALC_PGB_CTL, > > > - 0, 0x19, 30, pga_tlv), > > > + 0, 0x1A, 30, pga_tlv), > > > > The original patch where this control was added [1] already used 0x1A, > > however this was later changed to 0x19 in [2]. Your patch now reverts > > that change. Does this mean [2] was incorrect? If that is the case, > > shouldn't the commit message for this patch mention that it fixes [2] > > ? > > > > [1]: https://lore.kernel.org/all/20200918134317.22574-1-guille.rodriguez@gmail.com/ > > [2]: https://lore.kernel.org/all/20220602162119.3393857-7-ckeepax@opensource.cirrus.com/ > > > > Hmm... good digging, I didn't realise it was me who broke that. > Apologies in that chain I went around and checked a bunch of SX > controls to make sure they matched the datasheets, but it seems > I got a bit confused by the weird ordering of the values in the > datasheet. Since you have hardware would you be able to check, > before we merge this revert? A simple check that writing 0 to the > control sets the register value to 0x1A and writing 30 sets the > register to 0x18 would suffice. Just checked. The values are correct after applying the patch: $ amixer cset name='PGA Volume' '0','0' $ i2cget -y -f 2 0x4A 0x0A 0x1a $ i2cget -y -f 2 0x4A 0x0B 0x1a $ amixer cset name='PGA Volume' '30','30' $ i2cget -y -f 2 0x4A 0x0A 0x18 $ i2cget -y -f 2 0x4A 0x0B 0x18 BR, Guillermo Rodriguez Garcia guille.rodriguez@gmail.com
On Mon, Nov 28, 2022 at 12:14:43PM +0100, Guillermo Rodriguez Garcia wrote: > El lun, 28 nov 2022 a las 10:48, Charles Keepax > (<ckeepax@opensource.cirrus.com>) escribió: > > On Sat, Nov 26, 2022 at 12:15:10PM +0100, Guillermo Rodriguez Garcia wrote: > > > El vie, 25 nov 2022 a las 17:37, Charles Keepax > > > (<ckeepax@opensource.cirrus.com>) escribió: > > > > > > > > The table in the datasheet actually shows the volume values in the wrong > > > > order, with the two -3dB values being reversed. This appears to have > > > > caused the lower of the two values to be used in the driver when the > > > > higher should have been, correct this mixup. > > > > > > > > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com> > > > > --- > > > > @@ -143,7 +143,7 @@ static const struct snd_kcontrol_new cs42l51_snd_controls[] = { > > > > 0, 0xA0, 96, adc_att_tlv), > > > > SOC_DOUBLE_R_SX_TLV("PGA Volume", > > > > CS42L51_ALC_PGA_CTL, CS42L51_ALC_PGB_CTL, > > > > - 0, 0x19, 30, pga_tlv), > > > > + 0, 0x1A, 30, pga_tlv), > > > > > > The original patch where this control was added [1] already used 0x1A, > > > however this was later changed to 0x19 in [2]. Your patch now reverts > > > that change. Does this mean [2] was incorrect? If that is the case, > > > shouldn't the commit message for this patch mention that it fixes [2] > > > ? > > > > > > [1]: https://lore.kernel.org/all/20200918134317.22574-1-guille.rodriguez@gmail.com/ > > > [2]: https://lore.kernel.org/all/20220602162119.3393857-7-ckeepax@opensource.cirrus.com/ > > > > > > > Hmm... good digging, I didn't realise it was me who broke that. > > Apologies in that chain I went around and checked a bunch of SX > > controls to make sure they matched the datasheets, but it seems > > I got a bit confused by the weird ordering of the values in the > > datasheet. Since you have hardware would you be able to check, > > before we merge this revert? A simple check that writing 0 to the > > control sets the register value to 0x1A and writing 30 sets the > > register to 0x18 would suffice. > > Just checked. The values are correct after applying the patch: > > $ amixer cset name='PGA Volume' '0','0' > $ i2cget -y -f 2 0x4A 0x0A > 0x1a > $ i2cget -y -f 2 0x4A 0x0B > 0x1a > $ amixer cset name='PGA Volume' '30','30' > $ i2cget -y -f 2 0x4A 0x0A > 0x18 > $ i2cget -y -f 2 0x4A 0x0B > 0x18 Excellent thank you so much for your help on this. I will send a new version of the patch with the fixes tag applied. Thanks, Charles
diff --git a/sound/soc/codecs/cs42l51.c b/sound/soc/codecs/cs42l51.c index 51721edd8f53c..e88d9ff95cdfc 100644 --- a/sound/soc/codecs/cs42l51.c +++ b/sound/soc/codecs/cs42l51.c @@ -143,7 +143,7 @@ static const struct snd_kcontrol_new cs42l51_snd_controls[] = { 0, 0xA0, 96, adc_att_tlv), SOC_DOUBLE_R_SX_TLV("PGA Volume", CS42L51_ALC_PGA_CTL, CS42L51_ALC_PGB_CTL, - 0, 0x19, 30, pga_tlv), + 0, 0x1A, 30, pga_tlv), SOC_SINGLE("Playback Deemphasis Switch", CS42L51_DAC_CTL, 3, 1, 0), SOC_SINGLE("Auto-Mute Switch", CS42L51_DAC_CTL, 2, 1, 0), SOC_SINGLE("Soft Ramp Switch", CS42L51_DAC_CTL, 1, 1, 0),
The table in the datasheet actually shows the volume values in the wrong order, with the two -3dB values being reversed. This appears to have caused the lower of the two values to be used in the driver when the higher should have been, correct this mixup. Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com> --- No changes since v1. sound/soc/codecs/cs42l51.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)