Change in coreboot[master]: mb/google/cyan: Adjust CID for realtek audio codec

[Matt DeVillier (Code Review)]

Matt DeVillier has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40003 )

Change subject: mb/google/cyan: Adjust CID for realtek audio codec
......................................................................

mb/google/cyan: Adjust CID for realtek audio codec

Adjust CID to allow for Windows driver to attach without breaking
functionality under Linux.

Test: build/boot google/edgar, verify working audio under
both Windows 10 and Linux (GalliumOS 3.1)

Change-Id: Idca5cc86ba1f5ef3978cfba291a0c06e56ef5958
Signed-off-by: Matt DeVillier 
---
M src/mainboard/google/cyan/acpi/codec_realtek.asl
1 file changed, 1 insertion(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/40003/1

diff --git a/src/mainboard/google/cyan/acpi/codec_realtek.asl b/src/mainboard/google/cyan/acpi/codec_realtek.asl
index 9ebef19..365f799 100644
--- a/src/mainboard/google/cyan/acpi/codec_realtek.asl
+++ b/src/mainboard/google/cyan/acpi/codec_realtek.asl
@@ -19,7 +19,7 @@
    Device (RTEK)   /* Audio Codec driver I2C */
    {
    	Name (_HID, AUDIO_CODEC_HID)
-		Name (_CID, AUDIO_CODEC_CID)
+		Name (_CID, Package() { AUDIO_CODEC_CID, "INTCCFFD" })
    	Name (_DDN, AUDIO_CODEC_DDN)
    	Name (_UID, 1)

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40003 )

Change subject: mb/google/cyan: Adjust CID for realtek audio codec
......................................................................

Patch Set 1:

(3 comments)

https://review.coreboot.org/c/coreboot/+/40003/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/40003/1//COMMIT_MSG@11 
PS1, Line 11: 
Can you elaborate a little bit, where the 
Windows driver limitation comes from? Please also add the version of the tested driver.

https://review.coreboot.org/c/coreboot/+/40003/1//COMMIT_MSG@13 
PS1, Line 13: Linux
Add version for conveniance.

https://review.coreboot.org/c/coreboot/+/40003/1/src/mainboard/google/cyan/a... 
File src/mainboard/google/cyan/acpi/codec_realtek.asl:

https://review.coreboot.org/c/coreboot/+/40003/1/src/mainboard/google/cyan/a... 
PS1, Line 22: INTCCFFD
Where is this ID from?

$ git grep INTCCFFD
    src/mainboard/google/auron/variants/buddy/include/variant/acpi/mainboard.asl:           Name (_CID, "INTCCFFD")

Sorry for being ignorant, so this is just an array, and the Realtek Windows 10 driver checks these?

[Matt DeVillier (Code Review)]

Hello build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/40003

to look at the new patch set (#2).

Change subject: mb/google/cyan: Adjust CID for realtek audio codec
......................................................................

mb/google/cyan: Adjust CID for realtek audio codec

Adjust CID to allow for Realtek's Windows drivers to attach
without breaking functionality under Linux.

Both Linux and Windows use ACPI HID/CID matching for driver attachment.
Since the Realtek 5650 isn't used in standard Windows laptops, the
'10EC5650' HID/CID isn't contained in the Windows drivers' lookup file
(.inf), but a catch-all 'INTCCFFD' entry does exist, so concatenate
that to the existing CID to allow the drivers to attach.

Test: build/boot google/edgar, verify working audio under
both Windows 10 (with Realtek drivers 10.0.10586.4393) and Linux
(GalliumOS 3.1 / kernel 4.16.18, Manjaro 18.1 / kernel 5.1.x)

Change-Id: Idca5cc86ba1f5ef3978cfba291a0c06e56ef5958
Signed-off-by: Matt DeVillier 
---
M src/mainboard/google/cyan/acpi/codec_realtek.asl
1 file changed, 1 insertion(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/40003/2

[Matt DeVillier (Code Review)]

Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40003 )

Change subject: mb/google/cyan: Adjust CID for realtek audio codec
......................................................................

Patch Set 2:

(3 comments)

https://review.coreboot.org/c/coreboot/+/40003/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/40003/1//COMMIT_MSG@11 
PS1, Line 11:
> Can you elaborate a little bit, where the  […]

Done

https://review.coreboot.org/c/coreboot/+/40003/1//COMMIT_MSG@13 
PS1, Line 13: Linux
> Add version for conveniance.

Done

https://review.coreboot.org/c/coreboot/+/40003/1/src/mainboard/google/cyan/a... 
File src/mainboard/google/cyan/acpi/codec_realtek.asl:

https://review.coreboot.org/c/coreboot/+/40003/1/src/mainboard/google/cyan/a... 
PS1, Line 22: INTCCFFD
> Where is this ID from? […]

I've elaborated in the commit msg. Yes, both Linux and Windows use the HID/CID  for driver attachment matching. The HID (hardware ID) is checked first, then the CID (Compatible ID) which can be a string or an array.

The change in google/buddy was modeled after this one, but got merged first, so the commit msg there is a bit confusing =P

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40003 )

Change subject: mb/google/cyan: Adjust CID for realtek audio codec
......................................................................

Patch Set 2:

Thanks a lot. I understand it now.

Off-topic:

1.  Should the ASL Realtek driver code go into some common location? `src/mainboard/intel/strago/acpi/mainboard.asl` has it too for example.
2.  Should Realtek be contacted to update the drivers?

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40003 )

Change subject: mb/google/cyan: Adjust CID for realtek audio codec
......................................................................

Patch Set 2: Code-Review+1

[Matt DeVillier (Code Review)]

Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40003 )

Change subject: mb/google/cyan: Adjust CID for realtek audio codec
......................................................................

Patch Set 2:
> Patch Set 2:
>
> Thanks a lot. I understand it now.
>
> Off-topic:
> Should the ASL Realtek driver code go into some common location? `src/mainboard/intel/strago/acpi/mainboard.asl` has it too for example.
> Should Realtek be contacted to update the drivers?

the hookup is somewhat board/platform specific, so not sure there's a benefit. Strago is just a reference board for some of the cyan variants (all except cyan proper I believe), so we can either change it too or not, but don't believe anyone would care either way

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40003 )

Change subject: mb/google/cyan: Adjust CID for realtek audio codec
......................................................................

Patch Set 2: Code-Review+2

[Matt DeVillier (Code Review)]

Matt DeVillier has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40003 )

Change subject: mb/google/cyan: Adjust CID for realtek audio codec
......................................................................

mb/google/cyan: Adjust CID for realtek audio codec

Adjust CID to allow for Realtek's Windows drivers to attach
without breaking functionality under Linux.

Both Linux and Windows use ACPI HID/CID matching for driver attachment.
Since the Realtek 5650 isn't used in standard Windows laptops, the
'10EC5650' HID/CID isn't contained in the Windows drivers' lookup file
(.inf), but a catch-all 'INTCCFFD' entry does exist, so concatenate
that to the existing CID to allow the drivers to attach.

Test: build/boot google/edgar, verify working audio under
both Windows 10 (with Realtek drivers 10.0.10586.4393) and Linux
(GalliumOS 3.1 / kernel 4.16.18, Manjaro 18.1 / kernel 5.1.x)

Change-Id: Idca5cc86ba1f5ef3978cfba291a0c06e56ef5958
Signed-off-by: Matt DeVillier 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/40003
Reviewed-by: Paul Menzel 
Reviewed-by: Angel Pons 
Tested-by: build bot (Jenkins) 
---
M src/mainboard/google/cyan/acpi/codec_realtek.asl
1 file changed, 1 insertion(+), 1 deletion(-)

Approvals:
  build bot (Jenkins): Verified
  Paul Menzel: Looks good to me, but someone else must approve
  Angel Pons: Looks good to me, approved

diff --git a/src/mainboard/google/cyan/acpi/codec_realtek.asl b/src/mainboard/google/cyan/acpi/codec_realtek.asl
index 9ebef19..365f799 100644
--- a/src/mainboard/google/cyan/acpi/codec_realtek.asl
+++ b/src/mainboard/google/cyan/acpi/codec_realtek.asl
@@ -19,7 +19,7 @@
    Device (RTEK)   /* Audio Codec driver I2C */
    {
    	Name (_HID, AUDIO_CODEC_HID)
-		Name (_CID, AUDIO_CODEC_CID)
+		Name (_CID, Package() { AUDIO_CODEC_CID, "INTCCFFD" })
    	Name (_DDN, AUDIO_CODEC_DDN)
    	Name (_UID, 1)