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
