Re: [PATCH v4] ACPI / bus: Introduce a list of ids for "always present" devices

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

On Mon, 2017-04-10 at 17:49 +0200, Hans de Goede wrote:
> Several cherrytrail devices (all of which ship with windows 10) hide
> the
> lpss pwm controller in ACPI, typically the _STA method looks like
> this:

CherryTrail
PWM
LPSS

> 
>     Method (_STA, 0, NotSerialized)  // _STA: Status
>     {
>         If (OSID == One)
>         {
>             Return (Zero)
>         }
> 
>         Return (0x0F)
>     }
> 
> Where OSID is some dark magic seen in all cherrytrail ACPI tables
> making
> the machine behave differently depending on which OS it *thinks* it is
> booting, this gets set in a number of ways which we cannot control, on
> some newer machines it simple hardcoded to "One" aka win10.
> 
> This causes the PWM controller to get hidden, which means Linux cannot
> control the backlight level on cht based tablets / laptops.
> 
> Since loading the driver for this does no harm (the only in kernel
> user
> of it is the i915 driver, which will only use it when it needs it),
> this
> commit makes acpi_bus_get_status() always set status to
> ACPI_STA_DEFAULT
> for the 80862288 device, fixing the lack of backlight control.

> +#ifdef CONFIG_X86
> +/*
> + * Some ACPI devices are hidden (status == 0x0) in recent BIOS-es
> because
> + * some recent windows drivers bind to one device but poke at
> multiple

Windows

> + * devices at the same time, so the others get hidden.
> + * We work around this by always reporting ACPI_STA_DEFAULT for these
> + * devices. Note this MUST only be done for devices where this is
> safe.
> + *
> + * This forcing of devices to be present is limited to specific CPU
> (SoC)
> + * models both to avoid potentially causing trouble on other models
> and
> + * because some HIDs are re-used on different SoCs for completely
> + * different devices.
> + */
> +struct always_present_device_id {
> +	struct acpi_device_id hid[2];
> +	struct x86_cpu_id cpu_id[2];
> +};
> +

> +#define ENTRY(hid, cpu_model) {					

> 	\
> +	{ { hid, }, {} },						
> \

> +	{ { X86_VENDOR_INTEL, 6, cpu_model, X86_FEATURE_ANY, }, {} },
> 	\

Can we use separate macro for this. i.e. ICPU() ?

Perhaps at some point we might switch to set of generic ICPU()-like
macros.

Moreover, it seems you may change it to use only one existing structure

ICPU(model, hid)

> +}
> +
> +static const struct always_present_device_id
> always_present_device_ids[] = {
> +	/*
> +	 * Cherrytrail pwm directly poked by GPU driver in win10,
> +	 * but Linux uses a separate pwm driver, harmless if not
> used.
> +	 */
> +	ENTRY("80862288", INTEL_FAM6_ATOM_AIRMONT),
> +};
> +#endif
> +
> +void acpi_set_device_status(struct acpi_device *adev, u32 sta)
> +{
> +	u32 *status = (u32 *)&adev->status;
> +#ifdef CONFIG_X86
> +	int i;
> +
> +	/* acpi_match_device_ids checks status, so start with default
> */
> +	*status = ACPI_STA_DEFAULT;

> +	for (i = 0; i < ARRAY_SIZE(always_present_device_ids); i++) {
> +		if (acpi_match_device_ids(adev,
> +			always_present_device_ids[i].hid) == 0 &&
> +		    x86_match_cpu(always_present_device_ids[i].cpu_id
> )) {

I don't like this. It looks a bit hackish. If we need more, than one hid
per CPU, we might just supply an array

ICPU(model, hids)

> +			dev_info(&adev->dev, "Device [%s] is in
> always present list setting status [%08x]\n",
> +				 adev->pnp.bus_id, ACPI_STA_DEFAULT);
> +			return;
> +		}
> +	}
> +#endif

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx