Re: [PATCH v3 3/3] ACPI / LPSS: Add dmi quirk for skipping _DEP check for some device-links

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Thu, Oct 24, 2019 at 11:57:23PM +0200, Hans de Goede wrote:
> The iGPU / GFX0 device's _PS0 method on the ASUS T200TA depends on the
> I2C1 controller (which is connected to the embedded controller). But unlike
> in the T100TA/T100CHI this dependency is not listed in the _DEP of the GFX0
> device.
> 
> This results in the dev_WARN_ONCE(..., "Transfer while suspended\n") call
> in i2c-designware-master.c triggering and the AML code not working as it
> should.
> 
> This commit fixes this by adding a dmi based quirk mechanism for devices
> which miss a _DEP, and adding a quirk for the LNXVIDEO depending on the
> I2C1 device on the Asus T200TA.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

One comment below.

> Cc: stable@vger.kernel.org
> Fixes: 2d71ee0ce72f ("ACPI / LPSS: Add a device link from the GPU to the BYT I2C5 controller")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -Add Fixes: tag
> 
> Changes in v3:
> -Point Fixes tag to a more apropriate commit
> ---
>  drivers/acpi/acpi_lpss.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
> index cd8cf3333f04..751ed38f2a10 100644
> --- a/drivers/acpi/acpi_lpss.c
> +++ b/drivers/acpi/acpi_lpss.c
> @@ -10,6 +10,7 @@
>  #include <linux/acpi.h>
>  #include <linux/clkdev.h>
>  #include <linux/clk-provider.h>
> +#include <linux/dmi.h>
>  #include <linux/err.h>
>  #include <linux/io.h>
>  #include <linux/mutex.h>
> @@ -463,6 +464,18 @@ struct lpss_device_links {
>  	const char *consumer_hid;
>  	const char *consumer_uid;
>  	u32 flags;
> +	const struct dmi_system_id *dep_missing_ids;
> +};
> +
> +/* Please keep this list sorted alphabetically by vendor and model */
> +static const struct dmi_system_id i2c1_dep_missing_dmi_ids[] = {
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "T200TA"),
> +		},
> +	},
> +	{}
>  };
>  
>  /*
> @@ -478,7 +491,8 @@ static const struct lpss_device_links lpss_device_links[] = {
>  	/* CHT iGPU depends on PMIC I2C controller */
>  	{"808622C1", "7", "LNXVIDEO", NULL, DL_FLAG_PM_RUNTIME},
>  	/* BYT iGPU depends on the Embedded Controller I2C controller (UID 1) */
> -	{"80860F41", "1", "LNXVIDEO", NULL, DL_FLAG_PM_RUNTIME},
> +	{"80860F41", "1", "LNXVIDEO", NULL, DL_FLAG_PM_RUNTIME,
> +	 i2c1_dep_missing_dmi_ids},
>  	/* BYT CR iGPU depends on PMIC I2C controller (UID 5 on CR) */
>  	{"80860F41", "5", "LNXVIDEO", NULL, DL_FLAG_PM_RUNTIME},
>  	/* BYT iGPU depends on PMIC I2C controller (UID 7 on non CR) */
> @@ -577,7 +591,8 @@ static void acpi_lpss_link_consumer(struct device *dev1,
>  	if (!dev2)
>  		return;
>  

> -	if (acpi_lpss_dep(ACPI_COMPANION(dev2), ACPI_HANDLE(dev1)))
> +	if ((link->dep_missing_ids && dmi_check_system(link->dep_missing_ids))
> +	    || acpi_lpss_dep(ACPI_COMPANION(dev2), ACPI_HANDLE(dev1)))
>  		device_link_add(dev2, dev1, link->flags);

Perhaps a helper?

>  
>  	put_device(dev2);
> @@ -592,7 +607,8 @@ static void acpi_lpss_link_supplier(struct device *dev1,
>  	if (!dev2)
>  		return;
>  
> -	if (acpi_lpss_dep(ACPI_COMPANION(dev1), ACPI_HANDLE(dev2)))
> +	if ((link->dep_missing_ids && dmi_check_system(link->dep_missing_ids))
> +	    || acpi_lpss_dep(ACPI_COMPANION(dev1), ACPI_HANDLE(dev2)))
>  		device_link_add(dev1, dev2, link->flags);

And use it here?

>  
>  	put_device(dev2);
> -- 
> 2.23.0
> 

-- 
With Best Regards,
Andy Shevchenko