[PATCH] ASoC: rt5682: Add jack kcontrol

[PATCH] ASoC: rt5682: Add jack kcontrol

[Akihiko Odaki]

Create a jack kcontrol manually as rt5682 does not use DAPM pins
for jack and will not have kcontrols for them.

Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>
---
 include/sound/jack.h      | 2 +-
 sound/core/jack.c         | 3 +--
 sound/soc/codecs/rt5682.c | 5 +++++
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/include/sound/jack.h b/include/sound/jack.h
index 1181f536557e..bb5fcbd70d7c 100644
--- a/include/sound/jack.h
+++ b/include/sound/jack.h
@@ -60,10 +60,10 @@ struct snd_jack {
 	struct list_head kctl_list;
 	struct snd_card *card;
 	const char *id;
+	int type;
 #ifdef CONFIG_SND_JACK_INPUT_DEV
 	struct input_dev *input_dev;
 	int registered;
-	int type;
 	char name[100];
 	unsigned int key[6];   /* Keep in sync with definitions above */
 #endif /* CONFIG_SND_JACK_INPUT_DEV */
diff --git a/sound/core/jack.c b/sound/core/jack.c
index d1e3055f2b6a..967a9769ea24 100644
--- a/sound/core/jack.c
+++ b/sound/core/jack.c
@@ -530,8 +530,6 @@ int snd_jack_new(struct snd_card *card, const char *id, int type,
 
 		jack->input_dev->phys = "ALSA";
 
-		jack->type = type;
-
 		for (i = 0; i < SND_JACK_SWITCH_TYPES; i++)
 			if (type & (1 << i))
 				input_set_capability(jack->input_dev, EV_SW,
@@ -545,6 +543,7 @@ int snd_jack_new(struct snd_card *card, const char *id, int type,
 		goto fail_input;
 
 	jack->card = card;
+	jack->type = type;
 	INIT_LIST_HEAD(&jack->kctl_list);
 
 	if (initial_kctl)
diff --git a/sound/soc/codecs/rt5682.c b/sound/soc/codecs/rt5682.c
index be68d573a490..583dc80f08d0 100644
--- a/sound/soc/codecs/rt5682.c
+++ b/sound/soc/codecs/rt5682.c
@@ -1012,6 +1012,7 @@ static int rt5682_set_jack_detect(struct snd_soc_component *component,
 		struct snd_soc_jack *hs_jack, void *data)
 {
 	struct rt5682_priv *rt5682 = snd_soc_component_get_drvdata(component);
+	int ret;
 
 	rt5682->hs_jack = hs_jack;
 
@@ -1025,6 +1026,10 @@ static int rt5682_set_jack_detect(struct snd_soc_component *component,
 		return 0;
 	}
 
+	ret = snd_jack_add_new_kctl(hs_jack->jack, hs_jack->jack->id, hs_jack->jack->type);
+	if (ret)
+		dev_warn(component->dev, "Failed to create jack kcontrol - %d\n", ret);
+
 	if (!rt5682->is_sdw) {
 		switch (rt5682->pdata.jd_src) {
 		case RT5682_JD1:
-- 
2.35.1

[PATCH] ASoC: rt5682: Add jack kcontrol

[Akihiko Odaki]

Create a jack kcontrol manually as rt5682 does not use DAPM pins
for jack and will not have kcontrols for them.

Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>
---
 include/sound/jack.h      | 2 +-
 sound/core/jack.c         | 3 +--
 sound/soc/codecs/rt5682.c | 5 +++++
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/include/sound/jack.h b/include/sound/jack.h
index 1181f536557e..bb5fcbd70d7c 100644
--- a/include/sound/jack.h
+++ b/include/sound/jack.h
@@ -60,10 +60,10 @@ struct snd_jack {
 	struct list_head kctl_list;
 	struct snd_card *card;
 	const char *id;
+	int type;
 #ifdef CONFIG_SND_JACK_INPUT_DEV
 	struct input_dev *input_dev;
 	int registered;
-	int type;
 	char name[100];
 	unsigned int key[6];   /* Keep in sync with definitions above */
 #endif /* CONFIG_SND_JACK_INPUT_DEV */
diff --git a/sound/core/jack.c b/sound/core/jack.c
index d1e3055f2b6a..967a9769ea24 100644
--- a/sound/core/jack.c
+++ b/sound/core/jack.c
@@ -530,8 +530,6 @@ int snd_jack_new(struct snd_card *card, const char *id, int type,
 
 		jack->input_dev->phys = "ALSA";
 
-		jack->type = type;
-
 		for (i = 0; i < SND_JACK_SWITCH_TYPES; i++)
 			if (type & (1 << i))
 				input_set_capability(jack->input_dev, EV_SW,
@@ -545,6 +543,7 @@ int snd_jack_new(struct snd_card *card, const char *id, int type,
 		goto fail_input;
 
 	jack->card = card;
+	jack->type = type;
 	INIT_LIST_HEAD(&jack->kctl_list);
 
 	if (initial_kctl)
diff --git a/sound/soc/codecs/rt5682.c b/sound/soc/codecs/rt5682.c
index be68d573a490..583dc80f08d0 100644
--- a/sound/soc/codecs/rt5682.c
+++ b/sound/soc/codecs/rt5682.c
@@ -1012,6 +1012,7 @@ static int rt5682_set_jack_detect(struct snd_soc_component *component,
 		struct snd_soc_jack *hs_jack, void *data)
 {
 	struct rt5682_priv *rt5682 = snd_soc_component_get_drvdata(component);
+	int ret;
 
 	rt5682->hs_jack = hs_jack;
 
@@ -1025,6 +1026,10 @@ static int rt5682_set_jack_detect(struct snd_soc_component *component,
 		return 0;
 	}
 
+	ret = snd_jack_add_new_kctl(hs_jack->jack, hs_jack->jack->id, hs_jack->jack->type);
+	if (ret)
+		dev_warn(component->dev, "Failed to create jack kcontrol - %d\n", ret);
+
 	if (!rt5682->is_sdw) {
 		switch (rt5682->pdata.jd_src) {
 		case RT5682_JD1:
-- 
2.35.1

Re: [PATCH] ASoC: rt5682: Add jack kcontrol

[Mark Brown]

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

On Thu, Apr 07, 2022 at 11:13:16PM +0900, Akihiko Odaki wrote:

> Create a jack kcontrol manually as rt5682 does not use DAPM pins
> for jack and will not have kcontrols for them.

Why not use DAPM for the jack?  Note that normally the jack is part of
the machine driver not the CODEC - there's no way the CODEC can know how
it's been wired in on any given system.

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

Re: [PATCH] ASoC: rt5682: Add jack kcontrol

[Mark Brown]

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

On Thu, Apr 07, 2022 at 11:13:16PM +0900, Akihiko Odaki wrote:

> Create a jack kcontrol manually as rt5682 does not use DAPM pins
> for jack and will not have kcontrols for them.

Why not use DAPM for the jack?  Note that normally the jack is part of
the machine driver not the CODEC - there's no way the CODEC can know how
it's been wired in on any given system.

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

Re: [PATCH] ASoC: rt5682: Add jack kcontrol

[Akihiko Odaki]

On 2022/04/08 0:09, Mark Brown wrote:
> On Thu, Apr 07, 2022 at 11:13:16PM +0900, Akihiko Odaki wrote:
> 
>> Create a jack kcontrol manually as rt5682 does not use DAPM pins
>> for jack and will not have kcontrols for them.
> 
> Why not use DAPM for the jack?  Note that normally the jack is part of
> the machine driver not the CODEC - there's no way the CODEC can know how
> it's been wired in on any given system.

It seems it is an unsual case where the codec knows the source of the 
jack. RT5682 has interrupts and registers for the jack; see e.g. 
rt5682_button_detect in sound/soc/codecs/rt5682.c for details.

Regards,
Akihiko Odaki

Re: [PATCH] ASoC: rt5682: Add jack kcontrol

[Akihiko Odaki]

On 2022/04/08 0:09, Mark Brown wrote:
> On Thu, Apr 07, 2022 at 11:13:16PM +0900, Akihiko Odaki wrote:
> 
>> Create a jack kcontrol manually as rt5682 does not use DAPM pins
>> for jack and will not have kcontrols for them.
> 
> Why not use DAPM for the jack?  Note that normally the jack is part of
> the machine driver not the CODEC - there's no way the CODEC can know how
> it's been wired in on any given system.

It seems it is an unsual case where the codec knows the source of the 
jack. RT5682 has interrupts and registers for the jack; see e.g. 
rt5682_button_detect in sound/soc/codecs/rt5682.c for details.

Regards,
Akihiko Odaki

Re: [PATCH] ASoC: rt5682: Add jack kcontrol

[Mark Brown]

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

On Fri, Apr 08, 2022 at 12:46:04AM +0900, Akihiko Odaki wrote:
> On 2022/04/08 0:09, Mark Brown wrote:

> > Why not use DAPM for the jack?  Note that normally the jack is part of
> > the machine driver not the CODEC - there's no way the CODEC can know how
> > it's been wired in on any given system.

> It seems it is an unsual case where the codec knows the source of the jack.
> RT5682 has interrupts and registers for the jack; see e.g.
> rt5682_button_detect in sound/soc/codecs/rt5682.c for details.

That bit is very common but there's still machine specific aspects - is
the required hardware wired up, if it is wired up how exactly are things
wired (separate microphone jack, headset jack, one of many jacks?).  A
lot of the machine driver part of things is about labeling things so
that it can be displayed in a way that's easy to connect to the physical
system.  Generally the machine driver would define a jack and then
connect the CODEC to it.

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

Re: [PATCH] ASoC: rt5682: Add jack kcontrol

[Mark Brown]

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

On Fri, Apr 08, 2022 at 12:46:04AM +0900, Akihiko Odaki wrote:
> On 2022/04/08 0:09, Mark Brown wrote:

> > Why not use DAPM for the jack?  Note that normally the jack is part of
> > the machine driver not the CODEC - there's no way the CODEC can know how
> > it's been wired in on any given system.

> It seems it is an unsual case where the codec knows the source of the jack.
> RT5682 has interrupts and registers for the jack; see e.g.
> rt5682_button_detect in sound/soc/codecs/rt5682.c for details.

That bit is very common but there's still machine specific aspects - is
the required hardware wired up, if it is wired up how exactly are things
wired (separate microphone jack, headset jack, one of many jacks?).  A
lot of the machine driver part of things is about labeling things so
that it can be displayed in a way that's easy to connect to the physical
system.  Generally the machine driver would define a jack and then
connect the CODEC to it.

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

Re: [PATCH] ASoC: rt5682: Add jack kcontrol

[Akihiko Odaki]

On 2022/04/08 1:00, Mark Brown wrote:
> On Fri, Apr 08, 2022 at 12:46:04AM +0900, Akihiko Odaki wrote:
>> On 2022/04/08 0:09, Mark Brown wrote:
> 
>>> Why not use DAPM for the jack?  Note that normally the jack is part of
>>> the machine driver not the CODEC - there's no way the CODEC can know how
>>> it's been wired in on any given system.
> 
>> It seems it is an unsual case where the codec knows the source of the jack.
>> RT5682 has interrupts and registers for the jack; see e.g.
>> rt5682_button_detect in sound/soc/codecs/rt5682.c for details.
> 
> That bit is very common but there's still machine specific aspects - is
> the required hardware wired up, if it is wired up how exactly are things
> wired (separate microphone jack, headset jack, one of many jacks?).  A
> lot of the machine driver part of things is about labeling things so
> that it can be displayed in a way that's easy to connect to the physical
> system.  Generally the machine driver would define a jack and then
> connect the CODEC to it.

Whether the required hardware wired is told from the user of the codec 
via jack's type specified with snd_soc_card_jack_new(). The other 
details live in the codec.

Re: [PATCH] ASoC: rt5682: Add jack kcontrol

[Akihiko Odaki]

On 2022/04/08 1:00, Mark Brown wrote:
> On Fri, Apr 08, 2022 at 12:46:04AM +0900, Akihiko Odaki wrote:
>> On 2022/04/08 0:09, Mark Brown wrote:
> 
>>> Why not use DAPM for the jack?  Note that normally the jack is part of
>>> the machine driver not the CODEC - there's no way the CODEC can know how
>>> it's been wired in on any given system.
> 
>> It seems it is an unsual case where the codec knows the source of the jack.
>> RT5682 has interrupts and registers for the jack; see e.g.
>> rt5682_button_detect in sound/soc/codecs/rt5682.c for details.
> 
> That bit is very common but there's still machine specific aspects - is
> the required hardware wired up, if it is wired up how exactly are things
> wired (separate microphone jack, headset jack, one of many jacks?).  A
> lot of the machine driver part of things is about labeling things so
> that it can be displayed in a way that's easy to connect to the physical
> system.  Generally the machine driver would define a jack and then
> connect the CODEC to it.

Whether the required hardware wired is told from the user of the codec 
via jack's type specified with snd_soc_card_jack_new(). The other 
details live in the codec.

Re: [PATCH] ASoC: rt5682: Add jack kcontrol

[Mark Brown]

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

j
On Fri, Apr 08, 2022 at 01:11:22AM +0900, Akihiko Odaki wrote:
> On 2022/04/08 1:00, Mark Brown wrote:

> > That bit is very common but there's still machine specific aspects - is
> > the required hardware wired up, if it is wired up how exactly are things
> > wired (separate microphone jack, headset jack, one of many jacks?).  A
> > lot of the machine driver part of things is about labeling things so
> > that it can be displayed in a way that's easy to connect to the physical
> > system.  Generally the machine driver would define a jack and then
> > connect the CODEC to it.

> Whether the required hardware wired is told from the user of the codec via
> jack's type specified with snd_soc_card_jack_new(). The other details live
> in the codec.

So I'm confused about what problem this patch is intended to fix.  It
really sounds like there's some issue with the driver not using standard
interfaces that you're trying to work around but the changelog is not at
all clear.  The "doesn't use DAPM" bit is a bit of a warning sign, it
sounds like the audio signals to and from the CODEC aren't being
connected to the jack properly.

Look at how other devices with jack detection hardware handle this and
follow a similar pattern.

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

Re: [PATCH] ASoC: rt5682: Add jack kcontrol

[Mark Brown]

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

j
On Fri, Apr 08, 2022 at 01:11:22AM +0900, Akihiko Odaki wrote:
> On 2022/04/08 1:00, Mark Brown wrote:

> > That bit is very common but there's still machine specific aspects - is
> > the required hardware wired up, if it is wired up how exactly are things
> > wired (separate microphone jack, headset jack, one of many jacks?).  A
> > lot of the machine driver part of things is about labeling things so
> > that it can be displayed in a way that's easy to connect to the physical
> > system.  Generally the machine driver would define a jack and then
> > connect the CODEC to it.

> Whether the required hardware wired is told from the user of the codec via
> jack's type specified with snd_soc_card_jack_new(). The other details live
> in the codec.

So I'm confused about what problem this patch is intended to fix.  It
really sounds like there's some issue with the driver not using standard
interfaces that you're trying to work around but the changelog is not at
all clear.  The "doesn't use DAPM" bit is a bit of a warning sign, it
sounds like the audio signals to and from the CODEC aren't being
connected to the jack properly.

Look at how other devices with jack detection hardware handle this and
follow a similar pattern.

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

Re: [PATCH] ASoC: rt5682: Add jack kcontrol

[Akihiko Odaki]

On 2022/04/08 1:37, Mark Brown wrote:
> j
> On Fri, Apr 08, 2022 at 01:11:22AM +0900, Akihiko Odaki wrote:
>> On 2022/04/08 1:00, Mark Brown wrote:
> 
>>> That bit is very common but there's still machine specific aspects - is
>>> the required hardware wired up, if it is wired up how exactly are things
>>> wired (separate microphone jack, headset jack, one of many jacks?).  A
>>> lot of the machine driver part of things is about labeling things so
>>> that it can be displayed in a way that's easy to connect to the physical
>>> system.  Generally the machine driver would define a jack and then
>>> connect the CODEC to it.
> 
>> Whether the required hardware wired is told from the user of the codec via
>> jack's type specified with snd_soc_card_jack_new(). The other details live
>> in the codec.
> 
> So I'm confused about what problem this patch is intended to fix.  It
> really sounds like there's some issue with the driver not using standard
> interfaces that you're trying to work around but the changelog is not at
> all clear.  The "doesn't use DAPM" bit is a bit of a warning sign, it
> sounds like the audio signals to and from the CODEC aren't being
> connected to the jack properly.
> 
> Look at how other devices with jack detection hardware handle this and
> follow a similar pattern.

The situation actually seems quite a mess. You can find many drivers not 
using DAPM pins by searching for snd_soc_card_jack_new() calls with 
num_pins argument is 0. ams-delta-audio is exceptional as it adds DAPM 
pins later with snd_soc_jack_add_pins().

They do not have kcontrols for the jacks. The only exception is 
skl_hda_dsp_generic which calls snd_jack_add_new_kctl() as my patch 
does. Looking at other devices is probably not helpful to find an 
alternative in this case.

Regards,
Akihiko Odaki

Re: [PATCH] ASoC: rt5682: Add jack kcontrol

[Akihiko Odaki]

On 2022/04/08 1:37, Mark Brown wrote:
> j
> On Fri, Apr 08, 2022 at 01:11:22AM +0900, Akihiko Odaki wrote:
>> On 2022/04/08 1:00, Mark Brown wrote:
> 
>>> That bit is very common but there's still machine specific aspects - is
>>> the required hardware wired up, if it is wired up how exactly are things
>>> wired (separate microphone jack, headset jack, one of many jacks?).  A
>>> lot of the machine driver part of things is about labeling things so
>>> that it can be displayed in a way that's easy to connect to the physical
>>> system.  Generally the machine driver would define a jack and then
>>> connect the CODEC to it.
> 
>> Whether the required hardware wired is told from the user of the codec via
>> jack's type specified with snd_soc_card_jack_new(). The other details live
>> in the codec.
> 
> So I'm confused about what problem this patch is intended to fix.  It
> really sounds like there's some issue with the driver not using standard
> interfaces that you're trying to work around but the changelog is not at
> all clear.  The "doesn't use DAPM" bit is a bit of a warning sign, it
> sounds like the audio signals to and from the CODEC aren't being
> connected to the jack properly.
> 
> Look at how other devices with jack detection hardware handle this and
> follow a similar pattern.

The situation actually seems quite a mess. You can find many drivers not 
using DAPM pins by searching for snd_soc_card_jack_new() calls with 
num_pins argument is 0. ams-delta-audio is exceptional as it adds DAPM 
pins later with snd_soc_jack_add_pins().

They do not have kcontrols for the jacks. The only exception is 
skl_hda_dsp_generic which calls snd_jack_add_new_kctl() as my patch 
does. Looking at other devices is probably not helpful to find an 
alternative in this case.

Regards,
Akihiko Odaki

Re: [PATCH] ASoC: rt5682: Add jack kcontrol

[Mark Brown]

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

On Fri, Apr 08, 2022 at 02:16:41AM +0900, Akihiko Odaki wrote:
> On 2022/04/08 1:37, Mark Brown wrote:

> > Look at how other devices with jack detection hardware handle this and
> > follow a similar pattern.

> The situation actually seems quite a mess. You can find many drivers not
> using DAPM pins by searching for snd_soc_card_jack_new() calls with num_pins
> argument is 0. ams-delta-audio is exceptional as it adds DAPM pins later
> with snd_soc_jack_add_pins().

Sure, I'm not surprised there's some buggy drivers.

> They do not have kcontrols for the jacks. The only exception is
> skl_hda_dsp_generic which calls snd_jack_add_new_kctl() as my patch does.
> Looking at other devices is probably not helpful to find an alternative in
> this case.

The first driver I randomly picked when searching was
sound/soc/intel/boards/skl_rt286.c which seems to DTRT here, you can
also see sound/soc/samsung/speyside.c for another example.

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

Re: [PATCH] ASoC: rt5682: Add jack kcontrol

[Mark Brown]

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

On Fri, Apr 08, 2022 at 02:16:41AM +0900, Akihiko Odaki wrote:
> On 2022/04/08 1:37, Mark Brown wrote:

> > Look at how other devices with jack detection hardware handle this and
> > follow a similar pattern.

> The situation actually seems quite a mess. You can find many drivers not
> using DAPM pins by searching for snd_soc_card_jack_new() calls with num_pins
> argument is 0. ams-delta-audio is exceptional as it adds DAPM pins later
> with snd_soc_jack_add_pins().

Sure, I'm not surprised there's some buggy drivers.

> They do not have kcontrols for the jacks. The only exception is
> skl_hda_dsp_generic which calls snd_jack_add_new_kctl() as my patch does.
> Looking at other devices is probably not helpful to find an alternative in
> this case.

The first driver I randomly picked when searching was
sound/soc/intel/boards/skl_rt286.c which seems to DTRT here, you can
also see sound/soc/samsung/speyside.c for another example.

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