[PATCH 0/3] ES8316 audio codec fixes on Rock5B

[PATCH 0/3] ES8316 audio codec fixes on Rock5B

[Cristian Ciocaltea]

This patch series handles a few issues related to the ES8316 audio 
codec, discovered while doing some testing on the Rock 5B board.

Cristian Ciocaltea (3):
  ASoC: es8316: Increment max value for ALC Capture Target Volume
    control
  ASoC: es8316: Do not set rate constraints for unsupported MCLKs
  arm64: dts: rockchip: Assign ES8316 MCLK rate on rk3588-rock-5b

 arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts | 2 ++
 sound/soc/codecs/es8316.c                       | 4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)

-- 
2.40.1

[PATCH 1/3] ASoC: es8316: Increment max value for ALC Capture Target Volume control

[Cristian Ciocaltea]

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(-)

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),
-- 
2.40.1

Re: [PATCH 1/3] ASoC: es8316: Increment max value for ALC Capture Target Volume control

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1194 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] ASoC: es8316: Increment max value for ALC Capture Target Volume control

[Cristian Ciocaltea]

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

Re: [PATCH 1/3] ASoC: es8316: Increment max value for ALC Capture Target Volume control

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 809 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] ASoC: es8316: Increment max value for ALC Capture Target Volume control

[Cristian Ciocaltea]

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

Re: [PATCH 1/3] ASoC: es8316: Increment max value for ALC Capture Target Volume control

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 522 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] ASoC: es8316: Increment max value for ALC Capture Target Volume control

[Cristian Ciocaltea]

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.

Re: [PATCH 1/3] ASoC: es8316: Increment max value for ALC Capture Target Volume control

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 534 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 2/3] ASoC: es8316: Do not set rate constraints for unsupported MCLKs

[Cristian Ciocaltea]

When using the codec through the generic audio graph card, there are at
least two calls of es8316_set_dai_sysclk(), with the effect of limiting
the allowed sample rates according to the MCLK/LRCK ratios supported by
the codec:

1. During audio card setup, to set the initial MCLK - see
   asoc_simple_init_dai().

2. Before opening a stream, to update MCLK, according to the stream
   sample rate and the multiplication factor - see
   asoc_simple_hw_params().

In some cases the initial MCLK might be set to a frequency that doesn't
match any of the supported ratios, e.g. 12287999 instead of 12288000,
which is only 1 Hz below the supported clock, as that is what the
hardware reports. This creates an empty list of rate constraints, which
is further passed to snd_pcm_hw_constraint_list() via
es8316_pcm_startup(), and causes the following error on the very first
access of the sound card:

  $ speaker-test -D hw:Analog,0 -F S16_LE -c 2 -t wav
  Broken configuration for playback: no configurations available: Invalid argument
  Setting of hwparams failed: Invalid argument

Note that all subsequent retries succeed thanks to the updated MCLK set
at point 2 above, which uses a computed frequency value instead of a
reading from the hardware registers. Normally this would have mitigated
the issue, but es8316_pcm_startup() executes before the 2nd call to
es8316_set_dai_sysclk(), hence it cannot make use of the updated
constraints.

Since es8316_pcm_hw_params() performs anyway a final validation of MCLK
against the stream sample rate and the supported MCLK/LRCK ratios, fix
the issue by ensuring that sysclk_constraints list is only set when at
least one supported sample rate is autodetected by the codec.

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(-)

diff --git a/sound/soc/codecs/es8316.c b/sound/soc/codecs/es8316.c
index 773c94fd3547..a78c0049a30c 100644
--- a/sound/soc/codecs/es8316.c
+++ b/sound/soc/codecs/es8316.c
@@ -386,7 +386,7 @@ static int es8316_set_dai_sysclk(struct snd_soc_dai *codec_dai,
 			es8316->allowed_rates[count++] = freq / ratio;
 	}
 
-	es8316->sysclk_constraints.list = es8316->allowed_rates;
+	es8316->sysclk_constraints.list = count ? es8316->allowed_rates : NULL;
 	es8316->sysclk_constraints.count = count;
 
 	return 0;
-- 
2.40.1

Re: [PATCH 2/3] ASoC: es8316: Do not set rate constraints for unsupported MCLKs

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 273 bytes --]

On Wed, May 24, 2023 at 10:41:55AM +0300, Cristian Ciocaltea wrote:

> -	es8316->sysclk_constraints.list = es8316->allowed_rates;
> +	es8316->sysclk_constraints.list = count ? es8316->allowed_rates : NULL;

Please write normal conditional statements to improve legibility.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 3/3] arm64: dts: rockchip: Assign ES8316 MCLK rate on rk3588-rock-5b

[Cristian Ciocaltea]

The I2S0_8CH_MCLKOUT clock rate on Rock 5B board defaults to 12 MHz and
it is used to provide the master clock (MCLK) for the ES8316 audio
codec.

On sound card initialization, this limits the allowed sample rates
according to the MCLK/LRCK ratios supported by the codec, which results
in the following non-standard rates: 15625, 30000, 31250, 46875.

Hence, the very first access of the sound card fails:

  Broken configuration for playback: no configurations available: Invalid argument
  Setting of hwparams failed: Invalid argument

However, all subsequent attempts will succeed, as the audio graph card
will request a correct clock frequency, based on the stream sample rate
and the multiplication factor.

Assign MCLK to 12.288 MHz, which allows the codec to advertise most of
the standard sample rates.

Fixes: 55529fe3f32d ("arm64: dts: rockchip: Add rk3588-rock-5b analog audio")
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
index 3e4aee8f70c1..30cdd366813f 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
@@ -133,6 +133,8 @@ es8316: audio-codec@11 {
 		reg = <0x11>;
 		clocks = <&cru I2S0_8CH_MCLKOUT>;
 		clock-names = "mclk";
+		assigned-clocks = <&cru I2S0_8CH_MCLKOUT>;
+		assigned-clock-rates = <12288000>;
 		#sound-dai-cells = <0>;
 
 		port {
-- 
2.40.1

Re: [PATCH 3/3] arm64: dts: rockchip: Assign ES8316 MCLK rate on rk3588-rock-5b

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1142 bytes --]

On Wed, May 24, 2023 at 10:41:56AM +0300, Cristian Ciocaltea wrote:
> The I2S0_8CH_MCLKOUT clock rate on Rock 5B board defaults to 12 MHz and
> it is used to provide the master clock (MCLK) for the ES8316 audio
> codec.
> 
> On sound card initialization, this limits the allowed sample rates
> according to the MCLK/LRCK ratios supported by the codec, which results
> in the following non-standard rates: 15625, 30000, 31250, 46875.
> 
> Hence, the very first access of the sound card fails:
> 
>   Broken configuration for playback: no configurations available: Invalid argument
>   Setting of hwparams failed: Invalid argument
> 
> However, all subsequent attempts will succeed, as the audio graph card
> will request a correct clock frequency, based on the stream sample rate
> and the multiplication factor.
> 
> Assign MCLK to 12.288 MHz, which allows the codec to advertise most of
> the standard sample rates.

Surely this is irrelevant with your previous change, and if the clock is
freely reprogrammable as it sounds even harmful given that it'll
restrict rates that are not available with the selected MCLK?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/3] arm64: dts: rockchip: Assign ES8316 MCLK rate on rk3588-rock-5b

[Cristian Ciocaltea]

On 5/24/23 13:39, Mark Brown wrote:
> On Wed, May 24, 2023 at 10:41:56AM +0300, Cristian Ciocaltea wrote:
>> The I2S0_8CH_MCLKOUT clock rate on Rock 5B board defaults to 12 MHz and
>> it is used to provide the master clock (MCLK) for the ES8316 audio
>> codec.
>>
>> On sound card initialization, this limits the allowed sample rates
>> according to the MCLK/LRCK ratios supported by the codec, which results
>> in the following non-standard rates: 15625, 30000, 31250, 46875.
>>
>> Hence, the very first access of the sound card fails:
>>
>>   Broken configuration for playback: no configurations available: Invalid argument
>>   Setting of hwparams failed: Invalid argument
>>
>> However, all subsequent attempts will succeed, as the audio graph card
>> will request a correct clock frequency, based on the stream sample rate
>> and the multiplication factor.
>>
>> Assign MCLK to 12.288 MHz, which allows the codec to advertise most of
>> the standard sample rates.
> 
> Surely this is irrelevant with your previous change, and if the clock is
> freely reprogrammable as it sounds even harmful given that it'll
> restrict rates that are not available with the selected MCLK?

As mentioned in the previous patch description, there's a bad timing
with es8316_pcm_startup() being executed before the 2nd call to
es8316_set_dai_sysclk(), with the effect that the new/updated
rate constraints won't be used until the next playback attempt.

Hence the approach here was to ensure the initial list of restricted
rates is sane, by (pre)assigning a proper MCLK. Alternatively, we could
have used an unsupported MCLK and, with the help of the previous patch,
we would have ended up with no restrictions applied on es8316_pcm_startup().