ASoC: snd_soc_info_volsw and platfrom_max

[Srinivas Kandagatla]

On 16/08/2022 14:11, Mark Brown wrote:
> On Mon, Aug 15, 2022 at 10:22:37AM +0100, Srinivas Kandagatla wrote:
> 
>> before this patch the controls max value was calculated considering the min
>> value, but with this patch this calculation has changed resulting in low
>> volume on most of the codecs that are using SOC_SINGLE_S8_TLV.
> 
> Right, the whole situation is unclear.  At various points the code
> hasn't agreed with itself ragarding what the platform_max is relative
> to, if it's taken into account and all both within individual control
> types and also between control types.
> 
>> snd_soc_put_volsw does the right thing by considering mc->min, but
>> info_volsw does it differently.
> 
>> Below change fixes the issue for me, but I am bit confused with the first
>> line of this function that calculates max value as max = mc->max - mc->min
>> and then limits it to platform_max.
> 
> The issue is that it's not clear if the platform_max value should be a
> value for the register or a value for the control.  For some reason
> (which frankly is the source of a lot of the problems here) the controls
> adjust the user visible range to be zero based even though the ABI would
> be totally fine with any range.  There were out of tree patches that
> changed things for some of the control types too.

Only Instances where platform_max is set are via kcontrol builder 
macros, which then always sets this to xmax.

And none of these macros have provision to pass platform_max these are 
always assumed to be xmax. Which am not sure is always correct.

> 
>> diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c
>> index bd88de056358..49fb34609202 100644
>> --- a/sound/soc/soc-ops.c
>> +++ b/sound/soc/soc-ops.c
>> @@ -196,7 +196,7 @@ int snd_soc_info_volsw(struct snd_kcontrol *kcontrol,
>>
>>   	uinfo->count = snd_soc_volsw_is_stereo(mc) ? 2 : 1;
>>   	uinfo->value.integer.min = 0;
>> -	uinfo->value.integer.max = max;
>> +	uinfo->value.integer.max = max  - mc->min;
> 
> That'll then break anything that doesn't set platform_max since we'll
> apply mc->min twice, you'd need to do the adjustment at the point where
Yes, I agree.

something like this might work.

diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c
index bd88de056358..cc3b12ace295 100644
--- a/sound/soc/soc-ops.c
+++ b/sound/soc/soc-ops.c
@@ -179,7 +179,7 @@ int snd_soc_info_volsw(struct snd_kcontrol *kcontrol,
  	const char *vol_string = NULL;
  	int max;

-	max = uinfo->value.integer.max = mc->max - mc->min;
+	max = uinfo->value.integer.max = mc->max;
  	if (mc->platform_max && mc->platform_max < max)
  		max = mc->platform_max;

@@ -196,7 +196,7 @@ int snd_soc_info_volsw(struct snd_kcontrol *kcontrol,

  	uinfo->count = snd_soc_volsw_is_stereo(mc) ? 2 : 1;
  	uinfo->value.integer.min = 0;
-	uinfo->value.integer.max = max;
+	uinfo->value.integer.max = max  - mc->min;

  	return 0;
  }

> we apply the platform_max override.  Keeping platform_max a register
> value is *probably* the least bad thing here.

platform_max being a max register value seems more sensible.

But again if agree on that.

Current state of platform_max which is never set correctly if we use any 
helper macros is confusing.

Do you think having an explicit macros that take platform_max argument 
from drivers makes sense?

This will also bring in some clarity.


--srini


> 
>>   	return 0;
>>   }
>>
>>
>> Or should we fix the macro to set platform_max to be max - min.
> 
> We shouldn't be changing the static data at all, that's one of the
> problems.