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

["Ryan Walklin" <ryan@xxxxxxxxxxxxx>]

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.

>> @@ -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