| Message ID | b5c31f8e-9401-6ec1-cfbf-3b0977df6fc2@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: snd_soc_info_volsw and platfrom_max | expand |
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. > 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 we apply the platform_max override. Keeping platform_max a register value is *probably* the least bad thing here. > 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.
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.
On Tue, Aug 16, 2022 at 02:44:47PM +0100, Srinivas Kandagatla wrote: > On 16/08/2022 14:11, Mark Brown wrote: > > On Mon, Aug 15, 2022 at 10:22:37AM +0100, Srinivas Kandagatla wrote: > > 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. Those macros just shouldn't be setting platform_max at all, the whole goal with platform_max is that it overrides what the driver is doing for platform specific reasons. This is supposed to be overridable by the machine integration, but looking now it looks like it's got lost in the shuffle, I can't see any mechanism to configure it via DT or machine drivers right now though I think there might be some out of tree drivers that do it or perhaps I'm doing the wrong greps. > 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. That's entirely correct in that if it's not been overridden by the platform then we should just use whatever the driver provided. > - max = uinfo->value.integer.max = mc->max - mc->min; > + max = uinfo->value.integer.max = mc->max; Don't do double assignments like this, they're confusing.
On 16/08/2022 16:56, Mark Brown wrote: > On Tue, Aug 16, 2022 at 02:44:47PM +0100, Srinivas Kandagatla wrote: >> On 16/08/2022 14:11, Mark Brown wrote: >>> On Mon, Aug 15, 2022 at 10:22:37AM +0100, Srinivas Kandagatla wrote: > >>> 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. > > Those macros just shouldn't be setting platform_max at all, the whole > goal with platform_max is that it overrides what the driver is doing for > platform specific reasons. This is supposed to be overridable by the Ah, Do you think we should remove setting platform_max from these macros. Am hoping that this should also fix the issues that am seeing. --srini > machine integration, but looking now it looks like it's got lost in the > shuffle, I can't see any mechanism to configure it via DT or machine > drivers right now though I think there might be some out of tree drivers > that do it or perhaps I'm doing the wrong greps. > >> 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. > > That's entirely correct in that if it's not been overridden by the > platform then we should just use whatever the driver provided. > >> - max = uinfo->value.integer.max = mc->max - mc->min; >> + max = uinfo->value.integer.max = mc->max; > > Don't do double assignments like this, they're confusing.
On Tue, Aug 16, 2022 at 05:20:12PM +0100, Srinivas Kandagatla wrote: > On 16/08/2022 16:56, Mark Brown wrote: > > Those macros just shouldn't be setting platform_max at all, the whole > goal with platform_max is that it overrides what the driver is doing for > > platform specific reasons. This is supposed to be overridable by the > Do you think we should remove setting platform_max from these macros. > Am hoping that this should also fix the issues that am seeing. Yes, we should do that regardless of what's going on with your issues.
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; return 0; }
Hi Mark, After patch 30ac49841386 (ASoC: ops: Don't modify the driver's plaform_max when reading state) all the controls that are using signed TLV range like below /* -84dB min - 40dB max */ SOC_SINGLE_S8_TLV("RX0 Digital Volume", WCD934X_CDC_RX0_RX_VOL_CTL, -84, 40, digital_gain), reports max value as 40 instead of 124. 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. 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. Or should we fix the macro to set platform_max to be max - min. thanks, Srini