[alsa-devel] Right place for defining ACPI GPIO mapping for codec

[Hans de Goede]

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
>      {
>      }
> }
>