All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ASoC: DAPM: Cover regression by kctl change notification fix
@ 2021-11-05  9:09 Takashi Iwai
  2021-11-05 11:30 ` Cezary Rojewski
  2021-11-05 15:22 ` Mark Brown
  0 siblings, 2 replies; 5+ messages in thread
From: Takashi Iwai @ 2021-11-05  9:09 UTC (permalink / raw)
  To: Mark Brown; +Cc: alsa-devel, Yu-Hsuan Hsu

The recent fix for DAPM to correct the kctl change notification by the
commit 5af82c81b2c4 ("ASoC: DAPM: Fix missing kctl change
notifications") caused other regressions since it changed the behavior
of snd_soc_dapm_set_pin() that is called from several API functions.
Formerly it returned always 0 for success, but now it returns 0 or 1.

This patch addresses it, restoring the old behavior of
snd_soc_dapm_set_pin() while keeping the fix in
snd_soc_dapm_put_pin_switch().

Fixes: 5af82c81b2c4 ("ASoC: DAPM: Fix missing kctl change notifications")
Reported-by: Yu-Hsuan Hsu <yuhsuan@chromium.org>
Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/soc-dapm.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index 2892b0aba151..b06c5682445c 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -2559,8 +2559,13 @@ static struct snd_soc_dapm_widget *dapm_find_widget(
 	return NULL;
 }
 
-static int snd_soc_dapm_set_pin(struct snd_soc_dapm_context *dapm,
-				const char *pin, int status)
+/*
+ * set the DAPM pin status:
+ * returns 1 when the value has been updated, 0 when unchanged, or a negative
+ * error code; called from kcontrol put callback
+ */
+static int __snd_soc_dapm_set_pin(struct snd_soc_dapm_context *dapm,
+				  const char *pin, int status)
 {
 	struct snd_soc_dapm_widget *w = dapm_find_widget(dapm, pin, true);
 	int ret = 0;
@@ -2586,6 +2591,18 @@ static int snd_soc_dapm_set_pin(struct snd_soc_dapm_context *dapm,
 	return ret;
 }
 
+/*
+ * similar as __snd_soc_dapm_set_pin(), but returns 0 when successful;
+ * called from several API functions below
+ */
+static int snd_soc_dapm_set_pin(struct snd_soc_dapm_context *dapm,
+				const char *pin, int status)
+{
+	int ret = __snd_soc_dapm_set_pin(dapm, pin, status);
+
+	return ret < 0 ? ret : 0;
+}
+
 /**
  * snd_soc_dapm_sync_unlocked - scan and power dapm paths
  * @dapm: DAPM context
@@ -3589,10 +3606,10 @@ int snd_soc_dapm_put_pin_switch(struct snd_kcontrol *kcontrol,
 	const char *pin = (const char *)kcontrol->private_value;
 	int ret;
 
-	if (ucontrol->value.integer.value[0])
-		ret = snd_soc_dapm_enable_pin(&card->dapm, pin);
-	else
-		ret = snd_soc_dapm_disable_pin(&card->dapm, pin);
+	mutex_lock_nested(&card->dapm_mutex, SND_SOC_DAPM_CLASS_RUNTIME);
+	ret = __snd_soc_dapm_set_pin(&card->dapm, pin,
+				     !!ucontrol->value.integer.value[0]);
+	mutex_unlock(&card->dapm_mutex);
 
 	snd_soc_dapm_sync(&card->dapm);
 	return ret;
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] ASoC: DAPM: Cover regression by kctl change notification fix
  2021-11-05  9:09 [PATCH] ASoC: DAPM: Cover regression by kctl change notification fix Takashi Iwai
@ 2021-11-05 11:30 ` Cezary Rojewski
  2021-11-05 11:36   ` Takashi Iwai
  2021-11-05 15:22 ` Mark Brown
  1 sibling, 1 reply; 5+ messages in thread
From: Cezary Rojewski @ 2021-11-05 11:30 UTC (permalink / raw)
  To: Takashi Iwai, Mark Brown; +Cc: alsa-devel, Yu-Hsuan Hsu

On 2021-11-05 10:09 AM, Takashi Iwai wrote:
> The recent fix for DAPM to correct the kctl change notification by the
> commit 5af82c81b2c4 ("ASoC: DAPM: Fix missing kctl change
> notifications") caused other regressions since it changed the behavior
> of snd_soc_dapm_set_pin() that is called from several API functions.
> Formerly it returned always 0 for success, but now it returns 0 or 1.
> 
> This patch addresses it, restoring the old behavior of
> snd_soc_dapm_set_pin() while keeping the fix in
> snd_soc_dapm_put_pin_switch().
> 
> Fixes: 5af82c81b2c4 ("ASoC: DAPM: Fix missing kctl change notifications")
> Reported-by: Yu-Hsuan Hsu <yuhsuan@chromium.org>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

Hello,

 From my research I've found very few places which actually check the 
returned value of functions mentioned by you. And thus we could use this 
opportunity to adjust the kcontrol-put behavior according to 
documentation for all users without adding any additional functions 
which are part of this patch.

Board:
sound/soc/intel/boards/kbl_da7219_max98927.c

seems to be the offending user. We could update its code instead, 
leaving ASoC unchanged. What do you think?


Regards
Czarek

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] ASoC: DAPM: Cover regression by kctl change notification fix
  2021-11-05 11:30 ` Cezary Rojewski
@ 2021-11-05 11:36   ` Takashi Iwai
  2021-11-05 12:03     ` Cezary Rojewski
  0 siblings, 1 reply; 5+ messages in thread
From: Takashi Iwai @ 2021-11-05 11:36 UTC (permalink / raw)
  To: Cezary Rojewski; +Cc: alsa-devel, Mark Brown, Yu-Hsuan Hsu

On Fri, 05 Nov 2021 12:30:48 +0100,
Cezary Rojewski wrote:
> 
> On 2021-11-05 10:09 AM, Takashi Iwai wrote:
> > The recent fix for DAPM to correct the kctl change notification by the
> > commit 5af82c81b2c4 ("ASoC: DAPM: Fix missing kctl change
> > notifications") caused other regressions since it changed the behavior
> > of snd_soc_dapm_set_pin() that is called from several API functions.
> > Formerly it returned always 0 for success, but now it returns 0 or 1.
> >
> > This patch addresses it, restoring the old behavior of
> > snd_soc_dapm_set_pin() while keeping the fix in
> > snd_soc_dapm_put_pin_switch().
> >
> > Fixes: 5af82c81b2c4 ("ASoC: DAPM: Fix missing kctl change notifications")
> > Reported-by: Yu-Hsuan Hsu <yuhsuan@chromium.org>
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> 
> Hello,
> 
> From my research I've found very few places which actually check the
> returned value of functions mentioned by you. And thus we could use
> this opportunity to adjust the kcontrol-put behavior according to
> documentation for all users without adding any additional functions
> which are part of this patch.
> 
> Board:
> sound/soc/intel/boards/kbl_da7219_max98927.c
> 
> seems to be the offending user. We could update its code instead,
> leaving ASoC unchanged. What do you think?

Well, if we're going to that direction, we have to update the
documentation and properly mention about the positive return value.
So the changes in ASoC core would be needed nevertheless.

I find it good to keep the old existing behavior; those API functions
are only for enabling/disabling, so returning 0 or a negative error is
more natural than tri-state, less complex for callers.


thanks,

Takashi

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] ASoC: DAPM: Cover regression by kctl change notification fix
  2021-11-05 11:36   ` Takashi Iwai
@ 2021-11-05 12:03     ` Cezary Rojewski
  0 siblings, 0 replies; 5+ messages in thread
From: Cezary Rojewski @ 2021-11-05 12:03 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: alsa-devel, Mark Brown, Yu-Hsuan Hsu

On 2021-11-05 12:36 PM, Takashi Iwai wrote:
> On Fri, 05 Nov 2021 12:30:48 +0100,
> Cezary Rojewski wrote:
>>
>> On 2021-11-05 10:09 AM, Takashi Iwai wrote:
>>> The recent fix for DAPM to correct the kctl change notification by the
>>> commit 5af82c81b2c4 ("ASoC: DAPM: Fix missing kctl change
>>> notifications") caused other regressions since it changed the behavior
>>> of snd_soc_dapm_set_pin() that is called from several API functions.
>>> Formerly it returned always 0 for success, but now it returns 0 or 1.
>>>
>>> This patch addresses it, restoring the old behavior of
>>> snd_soc_dapm_set_pin() while keeping the fix in
>>> snd_soc_dapm_put_pin_switch().
>>>
>>> Fixes: 5af82c81b2c4 ("ASoC: DAPM: Fix missing kctl change notifications")
>>> Reported-by: Yu-Hsuan Hsu <yuhsuan@chromium.org>
>>> Cc: <stable@vger.kernel.org>
>>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
>>
>> Hello,
>>
>>  From my research I've found very few places which actually check the
>> returned value of functions mentioned by you. And thus we could use
>> this opportunity to adjust the kcontrol-put behavior according to
>> documentation for all users without adding any additional functions
>> which are part of this patch.
>>
>> Board:
>> sound/soc/intel/boards/kbl_da7219_max98927.c
>>
>> seems to be the offending user. We could update its code instead,
>> leaving ASoC unchanged. What do you think?
> 
> Well, if we're going to that direction, we have to update the
> documentation and properly mention about the positive return value.
> So the changes in ASoC core would be needed nevertheless.
> 
> I find it good to keep the old existing behavior; those API functions
> are only for enabling/disabling, so returning 0 or a negative error is
> more natural than tri-state, less complex for callers.

Hmm, indeed ASoC API doc would need an update, ASoC DAPM api != kcontrol 
api after all.
My impression of original patch was the desire to align the behaviour of 
snd_soc_dapm_enable/disable_pin() and similar functions with 
kcontrol-put equivalents - in terms of the returned value. Perhaps 
that's not the case.

While it's preferred the keep the existing behaviour, if we were to 
align DAPM functions with kcontrol-put ones, fact that vast majority of 
users ignore the returned value, helps us out.


Regards,
Czarek

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] ASoC: DAPM: Cover regression by kctl change notification fix
  2021-11-05  9:09 [PATCH] ASoC: DAPM: Cover regression by kctl change notification fix Takashi Iwai
  2021-11-05 11:30 ` Cezary Rojewski
@ 2021-11-05 15:22 ` Mark Brown
  1 sibling, 0 replies; 5+ messages in thread
From: Mark Brown @ 2021-11-05 15:22 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: alsa-devel, Yu-Hsuan Hsu

On Fri, 5 Nov 2021 10:09:25 +0100, Takashi Iwai wrote:
> The recent fix for DAPM to correct the kctl change notification by the
> commit 5af82c81b2c4 ("ASoC: DAPM: Fix missing kctl change
> notifications") caused other regressions since it changed the behavior
> of snd_soc_dapm_set_pin() that is called from several API functions.
> Formerly it returned always 0 for success, but now it returns 0 or 1.
> 
> This patch addresses it, restoring the old behavior of
> snd_soc_dapm_set_pin() while keeping the fix in
> snd_soc_dapm_put_pin_switch().
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-linus

Thanks!

[1/1] ASoC: DAPM: Cover regression by kctl change notification fix
      commit: 827b0913a9d9d07a0c3e559dbb20ca4d6d285a54

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-11-05 15:24 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-05  9:09 [PATCH] ASoC: DAPM: Cover regression by kctl change notification fix Takashi Iwai
2021-11-05 11:30 ` Cezary Rojewski
2021-11-05 11:36   ` Takashi Iwai
2021-11-05 12:03     ` Cezary Rojewski
2021-11-05 15:22 ` Mark Brown

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.