diff mbox series

[v4,13/13] ASoC: Intel: bytcr_wm5102: Add jack detect support

Message ID 20210123121720.79863-4-hdegoede@redhat.com (mailing list archive)
State Superseded
Headers show
Series MFD/extcon/ASoC: Rework arizona codec jack-detect support | expand

Commit Message

Hans de Goede Jan. 23, 2021, 12:17 p.m. UTC
Add jack detect support by creating a jack and calling
snd_soc_component_set_jack to register the created jack
with the codec.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 sound/soc/intel/boards/bytcr_wm5102.c | 28 ++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

Comments

Charles Keepax Jan. 30, 2021, 3:40 p.m. UTC | #1
On Sat, Jan 23, 2021 at 01:17:20PM +0100, Hans de Goede wrote:
> Add jack detect support by creating a jack and calling
> snd_soc_component_set_jack to register the created jack
> with the codec.
> 
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> +static struct snd_soc_jack_pin byt_wm5102_pins[] = {
> +	{
> +		.pin	= "Headphone",
> +		.mask	= SND_JACK_HEADPHONE,
> +	},
> +	{
> +		.pin	= "Headset Mic",
> +		.mask	= SND_JACK_MICROPHONE,
> +	},
> +};
> +

This patch looks fine to me, but I did have one small question.
What is the thinking behind punting this to the machine driver?

I guess you can not register it if there is no jack present
on the board, or if you have multiple jacks name them
meaningfully. Although I sort of feel like those applied to
the old extcon approach that just internally registered all
the interfaces.

But to be clear not asking for any changes just more about trying
to refine my understanding of things.

Thanks,
Charles
Hans de Goede Jan. 30, 2021, 6:26 p.m. UTC | #2
Hi,

On 1/30/21 4:40 PM, Charles Keepax wrote:
> On Sat, Jan 23, 2021 at 01:17:20PM +0100, Hans de Goede wrote:
>> Add jack detect support by creating a jack and calling
>> snd_soc_component_set_jack to register the created jack
>> with the codec.
>>
>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> +static struct snd_soc_jack_pin byt_wm5102_pins[] = {
>> +	{
>> +		.pin	= "Headphone",
>> +		.mask	= SND_JACK_HEADPHONE,
>> +	},
>> +	{
>> +		.pin	= "Headset Mic",
>> +		.mask	= SND_JACK_MICROPHONE,
>> +	},
>> +};
>> +
> 
> This patch looks fine to me, but I did have one small question.
> What is the thinking behind punting this to the machine driver?
> 
> I guess you can not register it if there is no jack present
> on the board, or if you have multiple jacks name them
> meaningfully. Although I sort of feel like those applied to
> the old extcon approach that just internally registered all
> the interfaces.

To be honest I'm not 100% sure why this is done this way,
this is how *all* ASoC drivers do it (AFAICT).

I think it is done this way because of 2 reasons:

1. The pins controlled by the jack are what for lack of
a better word I call "end-point" pins. And these get
registered by the machine-driver, so to make sure that
the names match it makes sense to also declare the
snd_soc_jack_pin array in the machine-driver.
For example the "Headphone" pin is a widget registered
by the machine driver as:

        SND_SOC_DAPM_HP("Headphone", NULL),

2. Probe ordering, the jack gets attached to the card and
when the coded driver's probe function runs the card does
not exist yet. But I think that could be worked around by
doing things in the component-probe function.

Regards,

Hans
diff mbox series

Patch

diff --git a/sound/soc/intel/boards/bytcr_wm5102.c b/sound/soc/intel/boards/bytcr_wm5102.c
index f38850eb2eaf..cdfe203ed9fa 100644
--- a/sound/soc/intel/boards/bytcr_wm5102.c
+++ b/sound/soc/intel/boards/bytcr_wm5102.c
@@ -18,6 +18,7 @@ 
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/spi/spi.h>
+#include <sound/jack.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>
 #include <sound/soc.h>
@@ -31,6 +32,7 @@ 
 #define WM5102_MAX_SYSCLK_11025	45158400 /* max sysclk for 11.025K family */
 
 struct byt_wm5102_private {
+	struct snd_soc_jack jack;
 	struct clk *mclk;
 	struct gpio_desc *spkvdd_en_gpio;
 };
@@ -177,11 +179,23 @@  static const struct snd_kcontrol_new byt_wm5102_controls[] = {
 	SOC_DAPM_PIN_SWITCH("Speaker"),
 };
 
+static struct snd_soc_jack_pin byt_wm5102_pins[] = {
+	{
+		.pin	= "Headphone",
+		.mask	= SND_JACK_HEADPHONE,
+	},
+	{
+		.pin	= "Headset Mic",
+		.mask	= SND_JACK_MICROPHONE,
+	},
+};
+
 static int byt_wm5102_init(struct snd_soc_pcm_runtime *runtime)
 {
 	struct snd_soc_card *card = runtime->card;
 	struct byt_wm5102_private *priv = snd_soc_card_get_drvdata(card);
-	int ret;
+	struct snd_soc_component *component = asoc_rtd_to_codec(runtime, 0)->component;
+	int ret, jack_type;
 
 	card->dapm.idle_bias_off = true;
 
@@ -210,6 +224,18 @@  static int byt_wm5102_init(struct snd_soc_pcm_runtime *runtime)
 		return ret;
 	}
 
+	jack_type = ARIZONA_JACK_MASK | SND_JACK_BTN_0 | SND_JACK_BTN_1 |
+		    SND_JACK_BTN_2 | SND_JACK_BTN_3;
+	ret = snd_soc_card_jack_new(card, "Headset", jack_type,
+				    &priv->jack, byt_wm5102_pins,
+				    ARRAY_SIZE(byt_wm5102_pins));
+	if (ret) {
+		dev_err(card->dev, "Error creating jack: %d\n", ret);
+		return ret;
+	}
+
+	snd_soc_component_set_jack(component, &priv->jack, NULL);
+
 	return 0;
 }