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

["Ryan Walklin" <ryan@testtoast.com>]

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