| Message ID | 20241221094122.27325-3-ryan@testtoast.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: sun4i-codec: add headphone dectection for Anbernic RG35XX devices | expand |
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 > >
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
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 >
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
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,