On Mon, Dec 23, 2024 at 5:20 AM Ryan Walklin <ryan@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> 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
>
