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

[Yauhen Kharuzhy]

On Mon, Jan 06, 2020 at 12:09:32PM +0100, Hans de Goede wrote:
> 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

OK, thanks for the answer. This sounds not ideal but reasonable, I will go
by such way.


-- 
Yauhen Kharuzhy