Hi,

On 06-01-2020 11:21, Yauhen Kharuzhy wrote:

Hello,

I am working now to get sound working at Lenovo Yoga Book tablet. It is Intel CherryTrail-based device and has RT5677 sound codec.

The RT5677 codec driver uses two GPIOs to reset and enable the codec, with names 'realtek,reset' and 'realtek,pow-ldo2'. The ACPI definition lacks a _DSD section with GPIO name->CRS ID definition, so I need to manually define this mapping somewhere using existing devm_acpi_dev_add_driver_gpios() method (various devices has various _CRS definitions order and content, so this is cannot be placed into rt5677.c codec driver).

The most obvious place for this is ASoC machine driver for my device, but the codec driver is binded to the ACPI device and initializes before machine driver.

Can somebody advice how to define such GPIOs mapping for codec in my case?

Hmm, so normally I would say to move the devm_gpiod_get_optional calls into the component probe part of the codec driver (rt5677_probe) which does run after the machine driver, but it seems that the GPIOs must be driven to the correct values before we can do any i2c transfers.

You could move everything below (and including) the devm_gpiod_get_optional calls from rt5677_i2c_probe to the top of rt5677_probe, but then you will also be moving these calls:

rt5677_init_gpio(i2c); ret = rt5677_init_irq(i2c);

Which register a GPIO chip themselves, which may be a dependency for probing other bits of the sound stack so moving those 2 calls is a bad idea.

This means that the codec-driver itself and specifically the rt5677_i2c_probe() function is pretty much the only remaining place where you can add the devm_acpi_dev_add_driver_gpios() call. Note that you may also need to set some pdata settings. For the rt5640 and rt5651 drivers we set some device properties from the machine driver and check those in the component probe function. But rt5677 already depends on various props inside the i2c probe function.

Note that taking care of machine specific bits in the codec driver is not unheard of, the rt5645.c also does this and includes a DMI table for this even though typically this would be more appropriate for the machine driver.

So in this case given the constraints I think it is fine to de a DMI match and add the devm_acpi_dev_add_driver_gpios() call based on that to the codec driver itself, like we are doing in sound/soc/codecs/rt5645.c, you can then also set some of the pdata based on the DMI match as needed.

For now I would not worry about making this generic, my suggestion would be to add a "rt5677_lenovo_yogabook_fixup" function which stars with the DMI check (and bails if it fails) and then does whatever you need wrt both the devm_acpi_dev_add_driver_gpios() call and the pdata settings.

And then add a call to rt5677_lenovo_yogabook_fixup() directly under the rt5677_read_device_properties() call in rt5677_i2c_probe().

We can worry about making the X86 machine specific fixups more generic when we need to add them for a second X86 device.

Regards,

Hans

The ACPI definition of sound device is below. Second I2C address definition is for TS3A277E jack detector. The first GPIO in the _CRS is codec reset, the second is pow-ldo2.

Device (RTEK) { Name (_ADR, Zero) // _ADR: Address Name (_HID, "10EC5677") // _HID: Hardware ID Name (_CID, "10EC5677") // _CID: Compatible ID Name (_DDN, "Realtek IIS Audio Codec") // _DDN: DOS Device Name Name (_SUB, "17AA7005") // _SUB: Subsystem ID Name (_UID, One) // _UID: Unique ID Name (_PR0, Package (0x01) // _PR0: Power Resources for D0 { CLK3 }) Name (CHAN, Package (0x02) { One, 0x0124F800 }) Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings { Name (SBUF, ResourceTemplate () { I2cSerialBusV2 (0x002C, ControllerInitiated, 0x000186A0, AddressingMode7Bit, "\_SB.PCI0.I2C1", 0x00, ResourceConsumer, , Exclusive, ) I2cSerialBusV2 (0x003B, ControllerInitiated, 0x000186A0, AddressingMode7Bit, "\_SB.PCI0.I2C1", 0x00, ResourceConsumer, , Exclusive, ) GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly, "\_SB.GPO3", 0x00, ResourceConsumer, , ) { // Pin list 0x0019 } GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly, "\_SB.GPO3", 0x00, ResourceConsumer, , ) { // Pin list 0x0012 } GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly, "\_SB.GPO3", 0x00, ResourceConsumer, , ) { // Pin list 0x0030 } GpioInt (Edge, ActiveLow, Exclusive, PullNone, 0x0000, "\_SB.GPO0", 0x00, ResourceConsumer, , ) { // Pin list 0x005B } GpioInt (Edge, ActiveLow, Exclusive, PullNone, 0x0000, "\_SB.GPO0", 0x00, ResourceConsumer, , ) { // Pin list 0x004D } SpiSerialBusV2 (0x0001, PolarityLow, FourWireMode, 0x08, ControllerInitiated, 0x003D0900, ClockPolarityHigh, ClockPhaseSecond, "\_SB.PCI0.SPI1", 0x00, ResourceConsumer, , Exclusive, ) }) Return (SBUF) /* _SB_.PCI0.I2C1.RTEK._CRS.SBUF */ }

 Method (_STA, 0, NotSerialized)  // _STA: Status
 {
     Return (0x0F)
 }

 Method (_DIS, 0, NotSerialized)  // _DIS: Disable Device
 {
 }

}