[PATCH 0/3] ASoC: sun4i-codec: add headphone dectection for Anbernic RG35XX devices

[PATCH 0/3] ASoC: sun4i-codec: add headphone dectection for Anbernic RG35XX devices

[Ryan Walklin]

Add support for headphone detection on the Anbernic RG35XX series. 

This series adds the required device tree bindings to describe GPIOs for jack detection in the sun4i-codec driver, adds support for jack detection to the codec machine driver, and describes the hardware configuration in the RG35XX DTS. The existing speaker amplifier GPIO pin can then be used in concert with jack detection to enable userspace sound servers (via an ALSA UCM configuration) to disable the speaker route when headphones are connected.

Thanks to Chris Morgan for his assistance putting this series together.

Regards,

Ryan

Chris Morgan (3):
  ASoC: dt-bindings: sun4i-a10-codec: add hp-det-gpios
  ASoC: sun4i-codec: support hp-det-gpios property
  arm64: dts: allwinner: h700: Add hp-det-gpios for Anbernic RG35XX

 .../sound/allwinner,sun4i-a10-codec.yaml      |  6 ++
 .../sun50i-h700-anbernic-rg35xx-2024.dts      |  5 +-
 sound/soc/sunxi/sun4i-codec.c                 | 59 ++++++++++++++++++-
 3 files changed, 67 insertions(+), 3 deletions(-)

-- 
2.47.1

[PATCH 1/3] ASoC: dt-bindings: sun4i-a10-codec: add hp-det-gpios

[Ryan Walklin]

From: Chris Morgan <macromorgan@hotmail.com>

Devices integrating Allwinner SoCs may use line-out or headphone jacks
with jack detection circuits attached to a GPIO. Support defining these
in DTs.

A number of Anbernic devices featuring the H700 SoC use this mechanism
to switch between a headphone jack and an internal speaker, so add these
to the allowed routing items.

Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
Signed-off-by: Ryan Walklin <ryan@testtoast.com>
---
 .../bindings/sound/allwinner,sun4i-a10-codec.yaml           | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/sound/allwinner,sun4i-a10-codec.yaml b/Documentation/devicetree/bindings/sound/allwinner,sun4i-a10-codec.yaml
index ebc9097f936ad..b4b711e80b65a 100644
--- a/Documentation/devicetree/bindings/sound/allwinner,sun4i-a10-codec.yaml
+++ b/Documentation/devicetree/bindings/sound/allwinner,sun4i-a10-codec.yaml
@@ -98,6 +98,10 @@ properties:
     maxItems: 1
     description: GPIO to enable the external amplifier
 
+  allwinner,hp-det-gpios:
+    maxItems: 1
+    description: GPIO for headphone/line-out detection
+
 required:
   - "#sound-dai-cells"
   - compatible
@@ -247,8 +251,10 @@ allOf:
         allwinner,audio-routing:
           items:
             enum:
+              - Headphone
               - LINEOUT
               - Line Out
+              - Speaker
 
         dmas:
           items:
-- 
2.47.1

[PATCH 2/3] ASoC: sun4i-codec: support hp-det-gpios property

[Ryan Walklin]

From: Chris Morgan <macromorgan@hotmail.com>

Add support for GPIO headphone detection with the hp-det-gpios
property. In order for this to properly disable the path upon
removal of headphones, the output must be labelled Headphone which
is a common sink in the driver. Note this patch also adds the output
of Headphone for the H616 codec.

Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
Signed-off-by: Ryan Walklin <ryan@testtoast.com>
---
 sound/soc/sunxi/sun4i-codec.c | 59 +++++++++++++++++++++++++++++++++--
 1 file changed, 57 insertions(+), 2 deletions(-)

diff --git a/sound/soc/sunxi/sun4i-codec.c b/sound/soc/sunxi/sun4i-codec.c
index 933a0913237c8..5f718dd3fcf0a 100644
--- a/sound/soc/sunxi/sun4i-codec.c
+++ b/sound/soc/sunxi/sun4i-codec.c
@@ -21,6 +21,7 @@
 #include <linux/gpio/consumer.h>
 
 #include <sound/core.h>
+#include <sound/jack.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>
 #include <sound/soc.h>
@@ -272,6 +273,7 @@ struct sun4i_codec {
 	struct clk	*clk_module;
 	struct reset_control *rst;
 	struct gpio_desc *gpio_pa;
+	struct gpio_desc *gpio_hp;
 
 	/* ADC_FIFOC register is at different offset on different SoCs */
 	struct regmap_field *reg_adc_fifoc;
@@ -1302,6 +1304,49 @@ static struct snd_soc_dai_driver dummy_cpu_dai = {
 	.ops = &dummy_dai_ops,
 };
 
+static struct snd_soc_jack sun4i_headphone_jack;
+
+static struct snd_soc_jack_pin sun4i_headphone_jack_pins[] = {
+	{ .pin = "Headphone", .mask = SND_JACK_HEADPHONE},
+};
+
+static struct snd_soc_jack_gpio sun4i_headphone_jack_gpio = {
+	.name = "hp-det",
+	.report = SND_JACK_HEADPHONE,
+	.debounce_time = 150,
+};
+
+static int sun4i_codec_machine_init(struct snd_soc_pcm_runtime *rtd)
+{
+	struct snd_soc_card *card = rtd->card;
+	struct sun4i_codec *scodec = snd_soc_card_get_drvdata(card);
+	int ret;
+
+	if (scodec->gpio_hp) {
+		ret = snd_soc_card_jack_new_pins(card, "Headphone Jack",
+						 SND_JACK_HEADPHONE,
+						 &sun4i_headphone_jack,
+						 sun4i_headphone_jack_pins,
+						 ARRAY_SIZE(sun4i_headphone_jack_pins));
+		if (ret) {
+			dev_err(rtd->dev,
+			        "Headphone jack creation failed: %d\n", ret);
+			return ret;
+		}
+
+		sun4i_headphone_jack_gpio.desc = scodec->gpio_hp;
+		ret = snd_soc_jack_add_gpios(&sun4i_headphone_jack, 1,
+					     &sun4i_headphone_jack_gpio);
+
+		if (ret) {
+			dev_err(rtd->dev, "Headphone GPIO not added: %d\n", ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
 static struct snd_soc_dai_link *sun4i_codec_create_link(struct device *dev,
 							int *num_links)
 {
@@ -1327,6 +1372,7 @@ static struct snd_soc_dai_link *sun4i_codec_create_link(struct device *dev,
 	link->codecs->name	= dev_name(dev);
 	link->platforms->name	= dev_name(dev);
 	link->dai_fmt		= SND_SOC_DAIFMT_I2S;
+	link->init		= sun4i_codec_machine_init;
 
 	*num_links = 1;
 
@@ -1635,10 +1681,11 @@ static const struct snd_soc_component_driver sun50i_h616_codec_codec = {
 };
 
 static const struct snd_kcontrol_new sun50i_h616_card_controls[] = {
-	SOC_DAPM_PIN_SWITCH("LINEOUT"),
+	SOC_DAPM_PIN_SWITCH("Speaker"),
 };
 
 static const struct snd_soc_dapm_widget sun50i_h616_codec_card_dapm_widgets[] = {
+	SND_SOC_DAPM_HP("Headphone", NULL),
 	SND_SOC_DAPM_LINE("Line Out", NULL),
 	SND_SOC_DAPM_SPK("Speaker", sun4i_codec_spk_event),
 };
@@ -1684,7 +1731,7 @@ static struct snd_soc_card *sun50i_h616_codec_create_card(struct device *dev)
 
 	card->dev		= dev;
 	card->owner		= THIS_MODULE;
-	card->name		= "H616 Audio Codec";
+	card->name		= "h616-audio-codec";
 	card->driver_name	= "sun4i-codec";
 	card->controls		= sun50i_h616_card_controls;
 	card->num_controls	= ARRAY_SIZE(sun50i_h616_card_controls);
@@ -1940,6 +1987,14 @@ static int sun4i_codec_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	scodec->gpio_hp = devm_gpiod_get_optional(&pdev->dev, "allwinner,hp-det",
+	                                          GPIOD_IN);
+	if (IS_ERR(scodec->gpio_hp)) {
+		ret = PTR_ERR(scodec->gpio_hp);
+		dev_err_probe(&pdev->dev, ret, "Failed to get hp-det gpio\n");
+		return ret;
+	}
+
 	/* reg_field setup */
 	scodec->reg_adc_fifoc = devm_regmap_field_alloc(&pdev->dev,
 							scodec->regmap,
-- 
2.47.1

[PATCH 3/3] arm64: dts: allwinner: h700: Add hp-det-gpios for Anbernic RG35XX

[Ryan Walklin]

From: Chris Morgan <macromorgan@hotmail.com>

Add support for headphone insertion detection via GPIO for the
RG35XX series.

Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
Signed-off-by: Ryan Walklin <macromorgan@hotmail.com>
---
 .../boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts  | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts
index 89de86b442155..cbab10de760c9 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts
@@ -238,8 +238,11 @@ panel_in_rgb: endpoint {
 };
 
 &codec {
-	allwinner,audio-routing = "Line Out", "LINEOUT";
+	/* Both speakers and headphone jack connected to 74HC4052D analog mux*/
+	allwinner,audio-routing = "Speaker", "LINEOUT", 
+				  "Headphone", "LINEOUT";
 	allwinner,pa-gpios = <&pio 8 5 GPIO_ACTIVE_HIGH>; // PI5
+	allwinner,hp-det-gpios = <&pio 8 3 GPIO_ACTIVE_HIGH>; // PI3
 	status = "okay";
 };
 
-- 
2.47.1

Re: [PATCH 1/3] ASoC: dt-bindings: sun4i-a10-codec: add hp-det-gpios

[Chris Morgan]

On Sat, Dec 21, 2024 at 10:26:32PM +1300, Ryan Walklin wrote:
> From: Chris Morgan <macromorgan@hotmail.com>
> 
> Devices integrating Allwinner SoCs may use line-out or headphone jacks
> with jack detection circuits attached to a GPIO. Support defining these
> in DTs.
> 
> A number of Anbernic devices featuring the H700 SoC use this mechanism
> to switch between a headphone jack and an internal speaker, so add these
> to the allowed routing items.
> 
> Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> Signed-off-by: Ryan Walklin <ryan@testtoast.com>
> ---
>  .../bindings/sound/allwinner,sun4i-a10-codec.yaml           | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/sound/allwinner,sun4i-a10-codec.yaml b/Documentation/devicetree/bindings/sound/allwinner,sun4i-a10-codec.yaml
> index ebc9097f936ad..b4b711e80b65a 100644
> --- a/Documentation/devicetree/bindings/sound/allwinner,sun4i-a10-codec.yaml
> +++ b/Documentation/devicetree/bindings/sound/allwinner,sun4i-a10-codec.yaml
> @@ -98,6 +98,10 @@ properties:
>      maxItems: 1
>      description: GPIO to enable the external amplifier
>  
> +  allwinner,hp-det-gpios:
> +    maxItems: 1
> +    description: GPIO for headphone/line-out detection
> +

If possible, I wonder if we can keep this without the vendor prefix?
It looks like for now Nvidia and some Rockchip codecs have the vendor
prefix, but audio-graph bindings and Freescale codec bindings have this
as a non-specific property (and it looks like simple-audio-card does it
either as "hp-det-gpios" or "simple-audio-card,hp-det-gpios" depending
upon the circumstances).

Also, the behavior of this is very specific to just the Headphone sink,
so we should drop the line-out text from the description. If someone
sets the routing as `"Headphone", "LINEOUT"` the state of the GPIO will
affect the audio path, but `"Line Out", "LINEOUT"` will not be impacted
by the state of the GPIO.

Thank you,
Chris

>  required:
>    - "#sound-dai-cells"
>    - compatible
> @@ -247,8 +251,10 @@ allOf:
>          allwinner,audio-routing:
>            items:
>              enum:
> +              - Headphone
>                - LINEOUT
>                - Line Out
> +              - Speaker
>  
>          dmas:
>            items:
> -- 
> 2.47.1
> 
> 

Re: [PATCH 2/3] ASoC: sun4i-codec: support hp-det-gpios property

[Chen-Yu Tsai]

On Sat, Dec 21, 2024 at 5:41 PM Ryan Walklin <ryan@testtoast.com> wrote:
>
> From: Chris Morgan <macromorgan@hotmail.com>
>
> Add support for GPIO headphone detection with the hp-det-gpios
> property. In order for this to properly disable the path upon
> removal of headphones, the output must be labelled Headphone which
> is a common sink in the driver. Note this patch also adds the output
> of Headphone for the H616 codec.
>
> Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> Signed-off-by: Ryan Walklin <ryan@testtoast.com>
> ---
>  sound/soc/sunxi/sun4i-codec.c | 59 +++++++++++++++++++++++++++++++++--
>  1 file changed, 57 insertions(+), 2 deletions(-)
>
> diff --git a/sound/soc/sunxi/sun4i-codec.c b/sound/soc/sunxi/sun4i-codec.c
> index 933a0913237c8..5f718dd3fcf0a 100644
> --- a/sound/soc/sunxi/sun4i-codec.c
> +++ b/sound/soc/sunxi/sun4i-codec.c
> @@ -21,6 +21,7 @@
>  #include <linux/gpio/consumer.h>
>
>  #include <sound/core.h>
> +#include <sound/jack.h>
>  #include <sound/pcm.h>
>  #include <sound/pcm_params.h>
>  #include <sound/soc.h>
> @@ -272,6 +273,7 @@ struct sun4i_codec {
>         struct clk      *clk_module;
>         struct reset_control *rst;
>         struct gpio_desc *gpio_pa;
> +       struct gpio_desc *gpio_hp;
>
>         /* ADC_FIFOC register is at different offset on different SoCs */
>         struct regmap_field *reg_adc_fifoc;
> @@ -1302,6 +1304,49 @@ static struct snd_soc_dai_driver dummy_cpu_dai = {
>         .ops = &dummy_dai_ops,
>  };
>
> +static struct snd_soc_jack sun4i_headphone_jack;
> +
> +static struct snd_soc_jack_pin sun4i_headphone_jack_pins[] = {
> +       { .pin = "Headphone", .mask = SND_JACK_HEADPHONE},
> +};
> +
> +static struct snd_soc_jack_gpio sun4i_headphone_jack_gpio = {
> +       .name = "hp-det",
> +       .report = SND_JACK_HEADPHONE,
> +       .debounce_time = 150,
> +};
> +
> +static int sun4i_codec_machine_init(struct snd_soc_pcm_runtime *rtd)
> +{
> +       struct snd_soc_card *card = rtd->card;
> +       struct sun4i_codec *scodec = snd_soc_card_get_drvdata(card);
> +       int ret;
> +
> +       if (scodec->gpio_hp) {
> +               ret = snd_soc_card_jack_new_pins(card, "Headphone Jack",
> +                                                SND_JACK_HEADPHONE,
> +                                                &sun4i_headphone_jack,
> +                                                sun4i_headphone_jack_pins,
> +                                                ARRAY_SIZE(sun4i_headphone_jack_pins));
> +               if (ret) {
> +                       dev_err(rtd->dev,
> +                               "Headphone jack creation failed: %d\n", ret);
> +                       return ret;
> +               }
> +
> +               sun4i_headphone_jack_gpio.desc = scodec->gpio_hp;
> +               ret = snd_soc_jack_add_gpios(&sun4i_headphone_jack, 1,
> +                                            &sun4i_headphone_jack_gpio);
> +
> +               if (ret) {
> +                       dev_err(rtd->dev, "Headphone GPIO not added: %d\n", ret);
> +                       return ret;
> +               }
> +       }
> +
> +       return 0;
> +}
> +
>  static struct snd_soc_dai_link *sun4i_codec_create_link(struct device *dev,
>                                                         int *num_links)
>  {
> @@ -1327,6 +1372,7 @@ static struct snd_soc_dai_link *sun4i_codec_create_link(struct device *dev,
>         link->codecs->name      = dev_name(dev);
>         link->platforms->name   = dev_name(dev);
>         link->dai_fmt           = SND_SOC_DAIFMT_I2S;
> +       link->init              = sun4i_codec_machine_init;
>
>         *num_links = 1;
>
> @@ -1635,10 +1681,11 @@ static const struct snd_soc_component_driver sun50i_h616_codec_codec = {
>  };
>
>  static const struct snd_kcontrol_new sun50i_h616_card_controls[] = {
> -       SOC_DAPM_PIN_SWITCH("LINEOUT"),
> +       SOC_DAPM_PIN_SWITCH("Speaker"),

Why?

>  };
>
>  static const struct snd_soc_dapm_widget sun50i_h616_codec_card_dapm_widgets[] = {
> +       SND_SOC_DAPM_HP("Headphone", NULL),
>         SND_SOC_DAPM_LINE("Line Out", NULL),
>         SND_SOC_DAPM_SPK("Speaker", sun4i_codec_spk_event),
>  };
> @@ -1684,7 +1731,7 @@ static struct snd_soc_card *sun50i_h616_codec_create_card(struct device *dev)
>
>         card->dev               = dev;
>         card->owner             = THIS_MODULE;
> -       card->name              = "H616 Audio Codec";
> +       card->name              = "h616-audio-codec";

Why?

>         card->driver_name       = "sun4i-codec";
>         card->controls          = sun50i_h616_card_controls;
>         card->num_controls      = ARRAY_SIZE(sun50i_h616_card_controls);
> @@ -1940,6 +1987,14 @@ static int sun4i_codec_probe(struct platform_device *pdev)
>                 return ret;
>         }
>
> +       scodec->gpio_hp = devm_gpiod_get_optional(&pdev->dev, "allwinner,hp-det",
> +                                                 GPIOD_IN);

Just put it on the same line. Lines can go up to 100 characters.

> +       if (IS_ERR(scodec->gpio_hp)) {
> +               ret = PTR_ERR(scodec->gpio_hp);
> +               dev_err_probe(&pdev->dev, ret, "Failed to get hp-det gpio\n");
> +               return ret;
> +       }
> +
>         /* reg_field setup */
>         scodec->reg_adc_fifoc = devm_regmap_field_alloc(&pdev->dev,
>                                                         scodec->regmap,
> --
> 2.47.1
>
>

Re: [PATCH 2/3] ASoC: sun4i-codec: support hp-det-gpios property

[Ryan Walklin]

Hi Chen-Yu,

Thanks for the review!

On Mon, 23 Dec 2024, at 6:15 AM, Chen-Yu Tsai wrote:
> On Sat, Dec 21, 2024 at 5:41 PM Ryan Walklin <ryan@testtoast.com> wrote:

>> @@ -1635,10 +1681,11 @@ static const struct snd_soc_component_driver sun50i_h616_codec_codec = {
>>  };
>>
>>  static const struct snd_kcontrol_new sun50i_h616_card_controls[] = {
>> -       SOC_DAPM_PIN_SWITCH("LINEOUT"),
>> +       SOC_DAPM_PIN_SWITCH("Speaker"),
>
> Why?

The speaker amp is controlled by a GPIO, this needs a specific card control to toggle on and off, discrete from the line-out volume. Muting the output entirely is not what is required, hence the separate control. I also understand (although it is IMO not well documented on the user-space side) that "Speaker" has a specific meaning to the user-space routing. Can add this to the merge message.

>> @@ -1684,7 +1731,7 @@ static struct snd_soc_card *sun50i_h616_codec_create_card(struct device *dev)
>>
>>         card->dev               = dev;
>>         card->owner             = THIS_MODULE;
>> -       card->name              = "H616 Audio Codec";
>> +       card->name              = "h616-audio-codec";
>
> Why?

As mentioned in the cover letter, ALSA UCM in user space uses the card name to detect and apply the relevant configuration, by loading /usr/share/alsa/ucm2/conf.d/<driver_name>/<card_name>.conf. Spaces are therefore removed in the card name to make this easier to manage. Happy to add this to the commit message too, and this could be changed to card->long_name if it is desired to maintain a human-readable card->name.

See https://github.com/alsa-project/alsa-ucm-conf/pull/491/files for the UCM patch.

>> @@ -1940,6 +1987,14 @@ static int sun4i_codec_probe(struct platform_device *pdev)
>>                 return ret;
>>         }
>>
>> +       scodec->gpio_hp = devm_gpiod_get_optional(&pdev->dev, "allwinner,hp-det",
>> +                                                 GPIOD_IN);
>
> Just put it on the same line. Lines can go up to 100 characters.

Thanks, will do.

Regards,

Ryan

Re: [PATCH 1/3] ASoC: dt-bindings: sun4i-a10-codec: add hp-det-gpios

[Ryan Walklin]

On Mon, 23 Dec 2024, at 5:51 AM, Chris Morgan wrote:
> On Sat, Dec 21, 2024 at 10:26:32PM +1300, Ryan Walklin wrote:
>>  
>> +  allwinner,hp-det-gpios:
>> +    maxItems: 1
>> +    description: GPIO for headphone/line-out detection
>> +
>
> If possible, I wonder if we can keep this without the vendor prefix?
> It looks like for now Nvidia and some Rockchip codecs have the vendor
> prefix, but audio-graph bindings and Freescale codec bindings have this
> as a non-specific property (and it looks like simple-audio-card does it
> either as "hp-det-gpios" or "simple-audio-card,hp-det-gpios" depending
> upon the circumstances).

I'm relaxed about either approach, it does work with the vendor prefix but agree just "hp-det-gpios" is probably appropriate given the intent is the same across vendors and devices.

> Also, the behavior of this is very specific to just the Headphone sink,
> so we should drop the line-out text from the description. If someone
> sets the routing as `"Headphone", "LINEOUT"` the state of the GPIO will
> affect the audio path, but `"Line Out", "LINEOUT"` will not be impacted
> by the state of the GPIO.

Agreed for this device, but this is a more generic description for the H616 generally, so I'm not sure we should remove the "Line Out" given that it may make sense for other H616 boards (Orange Pi etc) where someone may be using a 3.5mm jack as a line-level output rather than via an amp to headphones.

>
> Thank you,
> Chris
>
Regards,

Ryan

Re: [PATCH 1/3] ASoC: dt-bindings: sun4i-a10-codec: add hp-det-gpios

[Rob Herring]

On Sun, Dec 22, 2024 at 10:51:24AM -0600, Chris Morgan wrote:
> On Sat, Dec 21, 2024 at 10:26:32PM +1300, Ryan Walklin wrote:
> > From: Chris Morgan <macromorgan@hotmail.com>
> > 
> > Devices integrating Allwinner SoCs may use line-out or headphone jacks
> > with jack detection circuits attached to a GPIO. Support defining these
> > in DTs.
> > 
> > A number of Anbernic devices featuring the H700 SoC use this mechanism
> > to switch between a headphone jack and an internal speaker, so add these
> > to the allowed routing items.
> > 
> > Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> > Signed-off-by: Ryan Walklin <ryan@testtoast.com>
> > ---
> >  .../bindings/sound/allwinner,sun4i-a10-codec.yaml           | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/sound/allwinner,sun4i-a10-codec.yaml b/Documentation/devicetree/bindings/sound/allwinner,sun4i-a10-codec.yaml
> > index ebc9097f936ad..b4b711e80b65a 100644
> > --- a/Documentation/devicetree/bindings/sound/allwinner,sun4i-a10-codec.yaml
> > +++ b/Documentation/devicetree/bindings/sound/allwinner,sun4i-a10-codec.yaml
> > @@ -98,6 +98,10 @@ properties:
> >      maxItems: 1
> >      description: GPIO to enable the external amplifier
> >  
> > +  allwinner,hp-det-gpios:
> > +    maxItems: 1
> > +    description: GPIO for headphone/line-out detection
> > +
> 
> If possible, I wonder if we can keep this without the vendor prefix?
> It looks like for now Nvidia and some Rockchip codecs have the vendor
> prefix, but audio-graph bindings and Freescale codec bindings have this
> as a non-specific property (and it looks like simple-audio-card does it
> either as "hp-det-gpios" or "simple-audio-card,hp-det-gpios" depending
> upon the circumstances).

Yes, drop the vendor prefix.

Rob

Re: [PATCH 2/3] ASoC: sun4i-codec: support hp-det-gpios property

[Chen-Yu Tsai]

On Mon, Dec 23, 2024 at 5:20 AM Ryan Walklin <ryan@testtoast.com> wrote:
>
> Hi Chen-Yu,
>
> Thanks for the review!
>
> On Mon, 23 Dec 2024, at 6:15 AM, Chen-Yu Tsai wrote:
> > On Sat, Dec 21, 2024 at 5:41 PM Ryan Walklin <ryan@testtoast.com> wrote:
>
> >> @@ -1635,10 +1681,11 @@ static const struct snd_soc_component_driver sun50i_h616_codec_codec = {
> >>  };
> >>
> >>  static const struct snd_kcontrol_new sun50i_h616_card_controls[] = {
> >> -       SOC_DAPM_PIN_SWITCH("LINEOUT"),
> >> +       SOC_DAPM_PIN_SWITCH("Speaker"),
> >
> > Why?
>
> The speaker amp is controlled by a GPIO, this needs a specific card control to toggle on and off, discrete from the line-out volume. Muting the output entirely is not what is required, hence the separate control. I also understand (although it is IMO not well documented on the user-space side) that "Speaker" has a specific meaning to the user-space routing. Can add this to the merge message.

It should be documented, and probably a separate patch. This patch is
for the headphone. Also, "Speaker" is a DAPM widget name and that widget
is what toggles the GPIO. So it's actually the kernel side routing.

And also, you can't remove the "LINEOUT" pin because it is referenced from
existing device trees.


> >> @@ -1684,7 +1731,7 @@ static struct snd_soc_card *sun50i_h616_codec_create_card(struct device *dev)
> >>
> >>         card->dev               = dev;
> >>         card->owner             = THIS_MODULE;
> >> -       card->name              = "H616 Audio Codec";
> >> +       card->name              = "h616-audio-codec";
> >
> > Why?
>
> As mentioned in the cover letter, ALSA UCM in user space uses the card name to detect and apply the relevant configuration, by loading /usr/share/alsa/ucm2/conf.d/<driver_name>/<card_name>.conf. Spaces are therefore removed in the card name to make this easier to manage. Happy to add this to the commit message too, and this could be changed to card->long_name if it is desired to maintain a human-readable card->name.

This should also be a separate patch. And yes please add `long_name` to
keep the human friendly name.

ChenYu

> See https://github.com/alsa-project/alsa-ucm-conf/pull/491/files for the UCM patch.
>
> >> @@ -1940,6 +1987,14 @@ static int sun4i_codec_probe(struct platform_device *pdev)
> >>                 return ret;
> >>         }
> >>
> >> +       scodec->gpio_hp = devm_gpiod_get_optional(&pdev->dev, "allwinner,hp-det",
> >> +                                                 GPIOD_IN);
> >
> > Just put it on the same line. Lines can go up to 100 characters.
>
> Thanks, will do.
>
> Regards,
>
> Ryan
>

Re: [PATCH 2/3] ASoC: sun4i-codec: support hp-det-gpios property

[Ryan Walklin]

On Wed, 1 Jan 2025, at 9:56 PM, Chen-Yu Tsai wrote:
> On Mon, Dec 23, 2024 at 5:20 AM Ryan Walklin <ryan@testtoast.com> wrote:

>> >> @@ -1635,10 +1681,11 @@ static const struct snd_soc_component_driver sun50i_h616_codec_codec = {
>> >>  };
>> >>
>> >>  static const struct snd_kcontrol_new sun50i_h616_card_controls[] = {
>> >> -       SOC_DAPM_PIN_SWITCH("LINEOUT"),
>> >> +       SOC_DAPM_PIN_SWITCH("Speaker"),
>> >
>> > Why?
>>
>> The speaker amp is controlled by a GPIO, this needs a specific card control to toggle on and off, discrete from the line-out volume. Muting the output entirely is not what is required, hence the separate control. I also understand (although it is IMO not well documented on the user-space side) that "Speaker" has a specific meaning to the user-space routing. Can add this to the merge message.
>
> It should be documented, and probably a separate patch. This patch is
> for the headphone. Also, "Speaker" is a DAPM widget name and that widget
> is what toggles the GPIO. So it's actually the kernel side routing.

Thanks, happy to add a separate patch, but think it is appropriate included in this series as it only makes sense to power off the amplifier if there is no sound output or if we plug in headphones.

> And also, you can't remove the "LINEOUT" pin because it is referenced from
> existing device trees.

I am not entirely sure and not an expert here, but I think the LINEOUT reference in the existing trees is for the output route, not a DAPM switch? This was added to the prior H616 enablement patch largely because it was in the vendor code, but given the H616 only has a single audio route also called "LINEOUT in the manual) this is somewhat redundant, as the analog and digital parts of the codec already have DAPM widgets, and my understanding was that ALSA would then correctly enable and disable components based on the routing, not that the routing had to refer to specific DAPM controls/widgets.

>> >> @@ -1684,7 +1731,7 @@ static struct snd_soc_card *sun50i_h616_codec_create_card(struct device *dev)
>> >>
>> >>         card->dev               = dev;
>> >>         card->owner             = THIS_MODULE;
>> >> -       card->name              = "H616 Audio Codec";
>> >> +       card->name              = "h616-audio-codec";
>> >
>> > Why?
>>
>> As mentioned in the cover letter, ALSA UCM in user space uses the card name to detect and apply the relevant configuration, by loading /usr/share/alsa/ucm2/conf.d/<driver_name>/<card_name>.conf. Spaces are therefore removed in the card name to make this easier to manage. Happy to add this to the commit message too, and this could be changed to card->long_name if it is desired to maintain a human-readable card->name.
>
> This should also be a separate patch. And yes please add `long_name` to
> keep the human friendly name.

Thanks, will do.

Regards,

Ryan