* Re: [PATCH v2 2/2] ASoC: cs42l51: Correct PGA Volume minimum value [not found] ` <20221128094829.GH105268@ediswmail.ad.cirrus.com> @ 2022-11-28 11:14 ` Guillermo Rodriguez Garcia 2022-11-28 17:16 ` Charles Keepax 0 siblings, 1 reply; 2+ messages in thread From: Guillermo Rodriguez Garcia @ 2022-11-28 11:14 UTC (permalink / raw) To: Charles Keepax Cc: broonie, lgirdwood, alsa-devel, patches, james.schulman, david.rhodes, linux-kernel 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 ^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH v2 2/2] ASoC: cs42l51: Correct PGA Volume minimum value 2022-11-28 11:14 ` [PATCH v2 2/2] ASoC: cs42l51: Correct PGA Volume minimum value Guillermo Rodriguez Garcia @ 2022-11-28 17:16 ` Charles Keepax 0 siblings, 0 replies; 2+ messages in thread From: Charles Keepax @ 2022-11-28 17:16 UTC (permalink / raw) To: Guillermo Rodriguez Garcia Cc: broonie, lgirdwood, alsa-devel, patches, james.schulman, david.rhodes, linux-kernel 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 ^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-11-28 17:16 UTC | newest] Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20221125163748.1288942-1-ckeepax@opensource.cirrus.com> [not found] ` <20221125163748.1288942-2-ckeepax@opensource.cirrus.com> [not found] ` <CABDcavbtBBU7ySPK32104nZB5RHf2=LFuOOYOF-Ze1EDoM+etw@mail.gmail.com> [not found] ` <20221128094829.GH105268@ediswmail.ad.cirrus.com> 2022-11-28 11:14 ` [PATCH v2 2/2] ASoC: cs42l51: Correct PGA Volume minimum value Guillermo Rodriguez Garcia 2022-11-28 17:16 ` Charles Keepax
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).