On Friday, April 21, 2017 11:59:34 AM Hans de Goede wrote:
> Hi,
>
> On 19-04-17 22:14, Rafael J. Wysocki wrote:
> > On Wed, Apr 19, 2017 at 2:04 PM, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> >> Several Bay / Cherry Trail devices (all of which ship with Windows 10) hide
> >> the LPSS PWM controller in ACPI, typically the _STA method looks like this:
> >>
> >> Method (_STA, 0, NotSerialized) // _STA: Status
> >> {
> >> If (OSID == One)
> >> {
> >> Return (Zero)
> >> }
> >>
> >> Return (0x0F)
> >> }
> >>
> >> Where OSID is some dark magic seen in all Cherry Trail 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 uses it when it needs it), this
> >> commit makes acpi_bus_get_status() always set status to ACPI_STA_DEFAULT
> >> for the LPSS PWM device, fixing the lack of backlight control.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> >> ---
> >> Changes in v2:
> >> -Use pr_debug instead of ACPI_DEBUG_PRINT
> >> Changes in v3:
> >> -Un-inline acpi_set_device_status and do the always_present_device_ids
> >> table check inside the un-inlined version of it
> >> Changes in v4:
> >> -Use dev_info instead of pr_debug
> >> -Not only check for ACPI HID but also for CPU (SoC) model so as to not
> >> for devices present on other models then for which the quirk is intended and
> >> to avoid enabling unrelated ACPI devices which happen to use the same HID
> >> Changes in v5:
> >> -Only do the dev_info once per device (acpi_set_device_status gets called
> >> multiple times per device during boot)
> >> Changes in v6:
> >> -Allow specifying more then one CPU-model for a single HID
> >> -Not only match the HID but also the UID, like on Cherry Trail, on some Bay
> >> Trail Windows 10 tablets we need to enable the PWM controller to get working
> >> backlight even though _STA returns 0. The Bay Trail SoC has 2 PWM controllers
> >> and we only need the first one. UID matching will allows adding an entry for
> >> Bay Trail which only enables the first PWM controller
> >> Changes in v7:
> >> -Put the actual x86 specific matching code and table with always present
> >> device HID + UID + CPU model entries in a new x86/x86_utils.c file which
> >> provides an acpi_device_always_present() helper function on x86, on
> >> non x86 a stub which always returns false is provided
> >> -Squash in the addition of the Bay Trail PWM entry in the table as it
> >> is there for the same reasons as the Cherry Trail entry is there
>
> <snip>
>
> >> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> >> index 34fbe02..4d952cc 100644
> >> --- a/drivers/acpi/bus.c
> >> +++ b/drivers/acpi/bus.c
> >> @@ -132,6 +132,16 @@ int acpi_bus_get_status(struct acpi_device *device)
> >> }
> >> EXPORT_SYMBOL(acpi_bus_get_status);
> >>
> >> +void acpi_set_device_status(struct acpi_device *adev, u32 sta)
> >> +{
> >> + u32 *status = (u32 *)&adev->status;
> >> +
> >> + if (acpi_device_always_present(adev))
> >> + *status = ACPI_STA_DEFAULT;
> >> + else
> >> + *status = sta;
> >
> > *((u32 *)&adev->status) = acpi_device_always_present(adev) ?
> > ACPI_STA_DEFAULT : sta;
> >
> > and I guess it could still be static inline?
> >
> > But that said, evaluating _STA for the always present devices is
> > pointless (as Lukas noticed), so why not to modify
> > acpi_bus_get_status() to do something like:
> >
> > if (acpi_device_always_present(adev)) {
> > acpi_set_device_status(adev, ACPI_STA_DEFAULT);
> > return 0;
> > }
> >
> > upfront
>
> Hehe, my v1 of this patch actually did the check in acpi_bus_get_status()
> I moved it to acpi_set_device_status() on your request. Moving it back
> is fine with me and indeed seems cleaner.
>
> > and modify the other path invoking acpi_set_device_status() accordingly.
> >
> > And in that path, which I seem to have overlooked before, the
> > acpi_set_device_status() call is too early for invoking
> > acpi_device_always_present(adev), so the latter should be called
> > directly from acpi_add_single_object() like
> >
> > acpi_init_device_object(device, handle, type, sta);
> > if (acpi_device_always_present(adev))
> > acpi_set_device_status(adev, ACPI_STA_DEFAULT);
>
> That is not necessary, the place(s) where we care about status being
> fixed-up for always-present devices, so that they get scanned / their
> platform device initiated, is in acpi_bus_attach() which
> already calls acpi_bus_get_status() and thus gets the
> acpi_device_always_present() check applied before checking.
>
> For hotplugged devices there are acpi_scan_bus_check and
> acpi_scan_device_check but those both also already call
> acpi_bus_get_status() before checking adev->status.
OK
Which probably means that we don't need to initialize adev->status
in acpi_init_device_object() to anything meaningful, do we?
> So just adding the check to acpi_bus_get_status(), as I did in
> v1 is sufficient.
>
> Note that the end result is still significantly cleaner then v1
> as we're now using the acpi_device_always_present() which makes
> the drivers/acpi/bus.c changes a lot cleaner.
>
> I'll go and test this and then send a v8.
OK
Thanks,
Rafael
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
