| Message ID | 20230524074156.147387-2-cristian.ciocaltea@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ES8316 audio codec fixes on Rock5B | expand |
On Wed, May 24, 2023 at 10:41:54AM +0300, Cristian Ciocaltea wrote: > This means that either the hardware default is wrongly set to 0xB > instead of 0xA, or the specs are incorrect and instead of having the > range 0xA-0xF mapped to -1.5 dB, the single value 0xA should have been > mapped to -1.5 dB and the remaining range 0xB-0xF to 0 dB. > Increment the max value allowed for ALC Capture Target Volume control, > so that it matches the hardware default. > - SOC_SINGLE_TLV("ALC Capture Target Volume", ES8316_ADC_ALC3, 4, 10, 0, > + SOC_SINGLE_TLV("ALC Capture Target Volume", ES8316_ADC_ALC3, 4, 11, 0, > alc_target_tlv), The description above of what the control does doesn't seem to match what alc_target_tlv specifies - it is: static const SNDRV_CTL_TLVD_DECLARE_DB_SCALE(alc_target_tlv, -1650, 150, 0); which is saying that the value goes from -16.5dB up in steps of 1.5dB but your description above says that 0-10 map to -1.5dB and other values are 0dB. Presumably you can check the effects of changing the value? It seems plausible that what's written in the code might be accurate and the higher values might actually change the gain but it'd be better to check.
On 5/24/23 13:30, Mark Brown wrote: > On Wed, May 24, 2023 at 10:41:54AM +0300, Cristian Ciocaltea wrote: > >> This means that either the hardware default is wrongly set to 0xB >> instead of 0xA, or the specs are incorrect and instead of having the >> range 0xA-0xF mapped to -1.5 dB, the single value 0xA should have been >> mapped to -1.5 dB and the remaining range 0xB-0xF to 0 dB. > >> Increment the max value allowed for ALC Capture Target Volume control, >> so that it matches the hardware default. > >> - SOC_SINGLE_TLV("ALC Capture Target Volume", ES8316_ADC_ALC3, 4, 10, 0, >> + SOC_SINGLE_TLV("ALC Capture Target Volume", ES8316_ADC_ALC3, 4, 11, 0, >> alc_target_tlv), > > The description above of what the control does doesn't seem to match > what alc_target_tlv specifies - it is: > > static const SNDRV_CTL_TLVD_DECLARE_DB_SCALE(alc_target_tlv, -1650, 150, 0); > > which is saying that the value goes from -16.5dB up in steps of 1.5dB > but your description above says that 0-10 map to -1.5dB and other values > are 0dB. My description above mentioned only the 0xA-0xF (10-15) range, anything before that is fine and the implementation matches the specs: 0000 –> -16.5 dB 0001 –> -15.0 dB 0010 –> -13.5 dB ... 0111 –> -6.0 dB 1000 –> -4.5 dB 1001 –> -3.0 dB The inconsistency is here: 1010-1111 –> -1.5 dB Since the hardware default is 1011 (11) instead of 1010 (10), I assumed the specs could be wrong and probably should have provided the following mappings: 1010 -> -1.5 dB 1011-1111 -> 0 dB > Presumably you can check the effects of changing the value? It seems > plausible that what's written in the code might be accurate and the > higher values might actually change the gain but it'd be better to > check. I haven't noticed a (measurable) change in gain when switching between 10 and 11, but my testing equipment is also not that great. Will try to improve the tests accuracy. Thanks, Cristian
On Wed, May 24, 2023 at 04:49:37PM +0300, Cristian Ciocaltea wrote: > On 5/24/23 13:30, Mark Brown wrote: > > Presumably you can check the effects of changing the value? It seems > > plausible that what's written in the code might be accurate and the > > higher values might actually change the gain but it'd be better to > > check. > I haven't noticed a (measurable) change in gain when switching between > 10 and 11, but my testing equipment is also not that great. Will try to > improve the tests accuracy. I'd expect it should be really obvious with a scope if you've got one? Testing with something consistent like a sine wave (eg, from speaker-test) should also make a 1.5dB difference noticable enough to check if there's at least a volume change by ear even if you can't specifically quantify it.
On 5/24/23 16:59, Mark Brown wrote: > On Wed, May 24, 2023 at 04:49:37PM +0300, Cristian Ciocaltea wrote: >> On 5/24/23 13:30, Mark Brown wrote: > >>> Presumably you can check the effects of changing the value? It seems >>> plausible that what's written in the code might be accurate and the >>> higher values might actually change the gain but it'd be better to >>> check. > >> I haven't noticed a (measurable) change in gain when switching between >> 10 and 11, but my testing equipment is also not that great. Will try to >> improve the tests accuracy. > > I'd expect it should be really obvious with a scope if you've got one? > Testing with something consistent like a sine wave (eg, from > speaker-test) should also make a 1.5dB difference noticable enough to > check if there's at least a volume change by ear even if you can't > specifically quantify it. Luckily arecord & aplay provide VU meter support (via -V, --vumeter=TYPE'), so I could easily verify this without using any additional tools: Volume | VU meter ---------+---------- -6.0 dB | 30-31 % -4.5 dB | 35-36 % -3.0 dB | 42-43 % -1.5 dB | 50-51 % -0.0 dB | 50-51 % So it seems the specs are correct, and the problem is the hardware default. Is there a better approach to handle this than extending the volume range? Regards, Cristian
On Fri, May 26, 2023 at 09:11:49PM +0300, Cristian Ciocaltea wrote: > -1.5 dB | 50-51 % > -0.0 dB | 50-51 % > So it seems the specs are correct, and the problem is the hardware default. > Is there a better approach to handle this than extending the volume range? The other option would be to change the value in the register during probe to one that's in range, that wouldn't stop any existing saved settings from generating errors but would mean there wouldn't be any new ones. Either approach is probably fine.
On 5/30/23 14:36, Mark Brown wrote: > On Fri, May 26, 2023 at 09:11:49PM +0300, Cristian Ciocaltea wrote: > >> -1.5 dB | 50-51 % >> -0.0 dB | 50-51 % > >> So it seems the specs are correct, and the problem is the hardware default. > >> Is there a better approach to handle this than extending the volume range? > > The other option would be to change the value in the register during > probe to one that's in range, that wouldn't stop any existing saved > settings from generating errors but would mean there wouldn't be any new > ones. Either approach is probably fine. Thanks, I will prepare v2 and keep the current approach.
On Tue, May 30, 2023 at 03:52:52PM +0300, Cristian Ciocaltea wrote: > On 5/30/23 14:36, Mark Brown wrote: > > The other option would be to change the value in the register during > > probe to one that's in range, that wouldn't stop any existing saved > > settings from generating errors but would mean there wouldn't be any new > > ones. Either approach is probably fine. > Thanks, I will prepare v2 and keep the current approach. OK. Remember that the TLV will need to be updated to show the two values having identical effect.
diff --git a/sound/soc/codecs/es8316.c b/sound/soc/codecs/es8316.c index 9e033fb320a0..773c94fd3547 100644 --- a/sound/soc/codecs/es8316.c +++ b/sound/soc/codecs/es8316.c @@ -115,7 +115,7 @@ static const struct snd_kcontrol_new es8316_snd_controls[] = { alc_max_gain_tlv), SOC_SINGLE_TLV("ALC Capture Min Volume", ES8316_ADC_ALC2, 0, 28, 0, alc_min_gain_tlv), - SOC_SINGLE_TLV("ALC Capture Target Volume", ES8316_ADC_ALC3, 4, 10, 0, + SOC_SINGLE_TLV("ALC Capture Target Volume", ES8316_ADC_ALC3, 4, 11, 0, alc_target_tlv), SOC_SINGLE("ALC Capture Hold Time", ES8316_ADC_ALC3, 0, 10, 0), SOC_SINGLE("ALC Capture Decay Time", ES8316_ADC_ALC4, 4, 10, 0),
The following error occurs when trying to restore a previously saved ALSA mixer state (tested on a Rock 5B board): $ alsactl --no-ucm -f /tmp/asound.state store hw:Analog $ alsactl --no-ucm -I -f /tmp/asound.state restore hw:Analog alsactl: set_control:1475: Cannot write control '2:0:0:ALC Capture Target Volume:0' : Invalid argument According to ES8316 datasheet, the register at address 0x2B, which is related to the above mixer control, contains by default the value 0xB0. Considering the corresponding ALC target bits are 7:4, the control is initialized with 0xB, which is one step above the maximum value 0xA allowed by the driver. This means that either the hardware default is wrongly set to 0xB instead of 0xA, or the specs are incorrect and instead of having the range 0xA-0xF mapped to -1.5 dB, the single value 0xA should have been mapped to -1.5 dB and the remaining range 0xB-0xF to 0 dB. Increment the max value allowed for ALC Capture Target Volume control, so that it matches the hardware default. Fixes: b8b88b70875a ("ASoC: add es8316 codec driver") Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> --- sound/soc/codecs/es8316.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)