[PATCH 0/2] platform/x86: intel-vbtn: Fix ASUS VivoBook boot state

[PATCH 0/2] platform/x86: intel-vbtn: Fix ASUS VivoBook boot state

[Gwendal Grignou]

While qualifying ASUS VivoBook Flip 14 (TP401NAS) for ChromeOS Plex,
we notice it always boot in tablet mode, with the keyboard and touchpad
disabled. We have to rotate its lid over 180 degree and back to get into
clamshell mode, or put it into sleep and wake it up.

Disassembling the ACPI table, the virtual button/switch ACPI device is
implemented as follow:

  Device (VGBI)
  {
      Name (_HID, EisaId ("INT33D6") /* Intel Virtual Buttons Device */)  // _HID: Hardware ID
      Name (VBDS, Zero)
      Method (_STA, 0, Serialized)  // _STA: Status
      {
          PB1E |= 0x20
          If ((OSYS >= 0x07DD))
          {
              Return (0x0F)
          }

          Return (Zero)
      }

      Method (VBDL, 0, Serialized)
      {
          PB1E |= 0x20
          VBDS |= 0x40
      }

      Method (VGBS, 0, Serialized)
      {
          Return (VBDS) /* \_SB_.PCI0.SBRG.EC0_.VGBI.VBDS */
      }

      Method (UPBT, 2, Serialized)
      {
          Local0 = (One << Arg0)
          If (Arg1)
          {
              VBDS |= Local0
          }
          Else
          {
              VBDS &= ~Local0
          }
      }
  }

Method UBPT is called when lid angle cross 180 degree boundary or when
the device is woken up.

At boot, VBDS is set to 0 ("tablet mode") until UBPT or VBDL are called.

VBDL used to be evaluated before VGBS by the intel-vbtn driver probe
routine, but since commit 26173179fae1 ("platform/x86: intel-vbtn: Eval VBDL after registering our notifier"),
call to VGBS is delayed until after the notifier is register.

To bring back the expected behavior (device booting in clamshell
mode), make sure we evaluate VGBS after VBDL.

While at it, use function acpi_has_method() when we only need to know if a
method exist, as commit 26173179fae1 does.

Gwendal Grignou (2):
  platform/x86: intel-vbtn: Use acpi_has_method to check for switch
  platform/x86: intel-vbtn: Update tablet mode switch at end of probe

 drivers/platform/x86/intel/vbtn.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

-- 
2.44.0.478.gd926399ef9-goog

[PATCH 1/2] platform/x86: intel-vbtn: Use acpi_has_method to check for switch

[Gwendal Grignou]

To mimic how we check if the device has virtual buttons,
acpi_has_method(..."VBDL"), use the same method for checking virtual
switch presence.

Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
---
 drivers/platform/x86/intel/vbtn.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/platform/x86/intel/vbtn.c b/drivers/platform/x86/intel/vbtn.c
index 084c355c86f5f..48f0ac19d6ddf 100644
--- a/drivers/platform/x86/intel/vbtn.c
+++ b/drivers/platform/x86/intel/vbtn.c
@@ -258,9 +258,6 @@ static const struct dmi_system_id dmi_switches_allow_list[] = {
 
 static bool intel_vbtn_has_switches(acpi_handle handle, bool dual_accel)
 {
-	unsigned long long vgbs;
-	acpi_status status;
-
 	/* See dual_accel_detect.h for more info */
 	if (dual_accel)
 		return false;
@@ -268,8 +265,7 @@ static bool intel_vbtn_has_switches(acpi_handle handle, bool dual_accel)
 	if (!dmi_check_system(dmi_switches_allow_list))
 		return false;
 
-	status = acpi_evaluate_integer(handle, "VGBS", NULL, &vgbs);
-	return ACPI_SUCCESS(status);
+	return acpi_has_method(handle, "VGBS");
 }
 
 static int intel_vbtn_probe(struct platform_device *device)
-- 
2.44.0.478.gd926399ef9-goog

[PATCH 2/2] platform/x86: intel-vbtn: Update tablet mode switch at end of probe

[Gwendal Grignou]

ACER Vivobook Flip (TP401NAS) virtual intel switch is implemented as
follow:

   Device (VGBI)
   {
       Name (_HID, EisaId ("INT33D6") ...
       Name (VBDS, Zero)
       Method (_STA, 0, Serialized)  // _STA: Status ...
       Method (VBDL, 0, Serialized)
       {
           PB1E |= 0x20
           VBDS |= 0x40
       }
       Method (VGBS, 0, Serialized)
       {
           Return (VBDS) /* \_SB_.PCI0.SBRG.EC0_.VGBI.VBDS */
       }
       ...
    }

By default VBDS is set to 0. At boot it is set to clamshell (bit 6 set)
only after method VBDL is executed.

Since VBDL is now evaluated in the probe routine later, after the device
is registered, the retrieved value of VBDS was still 0 ("tablet mode")
when setting up the virtual switch.

Make sure to evaluate VGBS after VBDL, to ensure the
convertible boots in clamshell mode, the expected default.

Fixes: 26173179fae1 ("platform/x86: intel-vbtn: Eval VBDL after registering our notifier")
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
---
 drivers/platform/x86/intel/vbtn.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/intel/vbtn.c b/drivers/platform/x86/intel/vbtn.c
index 48f0ac19d6ddf..79bb2c801daa9 100644
--- a/drivers/platform/x86/intel/vbtn.c
+++ b/drivers/platform/x86/intel/vbtn.c
@@ -136,8 +136,6 @@ static int intel_vbtn_input_setup(struct platform_device *device)
 	priv->switches_dev->id.bustype = BUS_HOST;
 
 	if (priv->has_switches) {
-		detect_tablet_mode(&device->dev);
-
 		ret = input_register_device(priv->switches_dev);
 		if (ret)
 			return ret;
@@ -312,6 +310,9 @@ static int intel_vbtn_probe(struct platform_device *device)
 		if (ACPI_FAILURE(status))
 			dev_err(&device->dev, "Error VBDL failed with ACPI status %d\n", status);
 	}
+	// Check switches after buttons since VBDL may have side effects.
+	if (has_switches)
+		detect_tablet_mode(&device->dev);
 
 	device_init_wakeup(&device->dev, true);
 	/*
-- 
2.44.0.478.gd926399ef9-goog

Re: [PATCH 1/2] platform/x86: intel-vbtn: Use acpi_has_method to check for switch

[Kuppuswamy Sathyanarayanan]

On 3/29/24 7:32 AM, Gwendal Grignou wrote:
> To mimic how we check if the device has virtual buttons,
> acpi_has_method(..."VBDL"), use the same method for checking virtual
> switch presence.

if possible don't use words like we/I in the commit log.

Other wise, it looks fine.

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

>
> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
> ---
>  drivers/platform/x86/intel/vbtn.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/platform/x86/intel/vbtn.c b/drivers/platform/x86/intel/vbtn.c
> index 084c355c86f5f..48f0ac19d6ddf 100644
> --- a/drivers/platform/x86/intel/vbtn.c
> +++ b/drivers/platform/x86/intel/vbtn.c
> @@ -258,9 +258,6 @@ static const struct dmi_system_id dmi_switches_allow_list[] = {
>  
>  static bool intel_vbtn_has_switches(acpi_handle handle, bool dual_accel)
>  {
> -	unsigned long long vgbs;
> -	acpi_status status;
> -
>  	/* See dual_accel_detect.h for more info */
>  	if (dual_accel)
>  		return false;
> @@ -268,8 +265,7 @@ static bool intel_vbtn_has_switches(acpi_handle handle, bool dual_accel)
>  	if (!dmi_check_system(dmi_switches_allow_list))
>  		return false;
>  
> -	status = acpi_evaluate_integer(handle, "VGBS", NULL, &vgbs);
> -	return ACPI_SUCCESS(status);
> +	return acpi_has_method(handle, "VGBS");
>  }
>  
>  static int intel_vbtn_probe(struct platform_device *device)

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH 2/2] platform/x86: intel-vbtn: Update tablet mode switch at end of probe

[Kuppuswamy Sathyanarayanan]

On 3/29/24 7:32 AM, Gwendal Grignou wrote:
> ACER Vivobook Flip (TP401NAS) virtual intel switch is implemented as
> follow:
>
>    Device (VGBI)
>    {
>        Name (_HID, EisaId ("INT33D6") ...
>        Name (VBDS, Zero)
>        Method (_STA, 0, Serialized)  // _STA: Status ...
>        Method (VBDL, 0, Serialized)
>        {
>            PB1E |= 0x20
>            VBDS |= 0x40
>        }
>        Method (VGBS, 0, Serialized)
>        {
>            Return (VBDS) /* \_SB_.PCI0.SBRG.EC0_.VGBI.VBDS */
>        }
>        ...
>     }
>
> By default VBDS is set to 0. At boot it is set to clamshell (bit 6 set)
> only after method VBDL is executed.
>
> Since VBDL is now evaluated in the probe routine later, after the device
> is registered, the retrieved value of VBDS was still 0 ("tablet mode")
> when setting up the virtual switch.
>
> Make sure to evaluate VGBS after VBDL, to ensure the
> convertible boots in clamshell mode, the expected default.
>
> Fixes: 26173179fae1 ("platform/x86: intel-vbtn: Eval VBDL after registering our notifier")
> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
> ---

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

>  drivers/platform/x86/intel/vbtn.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/intel/vbtn.c b/drivers/platform/x86/intel/vbtn.c
> index 48f0ac19d6ddf..79bb2c801daa9 100644
> --- a/drivers/platform/x86/intel/vbtn.c
> +++ b/drivers/platform/x86/intel/vbtn.c
> @@ -136,8 +136,6 @@ static int intel_vbtn_input_setup(struct platform_device *device)
>  	priv->switches_dev->id.bustype = BUS_HOST;
>  
>  	if (priv->has_switches) {
> -		detect_tablet_mode(&device->dev);
> -
>  		ret = input_register_device(priv->switches_dev);
>  		if (ret)
>  			return ret;
> @@ -312,6 +310,9 @@ static int intel_vbtn_probe(struct platform_device *device)
>  		if (ACPI_FAILURE(status))
>  			dev_err(&device->dev, "Error VBDL failed with ACPI status %d\n", status);
>  	}
> +	// Check switches after buttons since VBDL may have side effects.
> +	if (has_switches)
> +		detect_tablet_mode(&device->dev);
>  
>  	device_init_wakeup(&device->dev, true);
>  	/*

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH 0/2] platform/x86: intel-vbtn: Fix ASUS VivoBook boot state

[Hans de Goede]

Hi,

On 3/29/24 3:32 PM, Gwendal Grignou wrote:
> While qualifying ASUS VivoBook Flip 14 (TP401NAS) for ChromeOS Plex,
> we notice it always boot in tablet mode, with the keyboard and touchpad
> disabled. We have to rotate its lid over 180 degree and back to get into
> clamshell mode, or put it into sleep and wake it up.
> 
> Disassembling the ACPI table, the virtual button/switch ACPI device is
> implemented as follow:
> 
>   Device (VGBI)
>   {
>       Name (_HID, EisaId ("INT33D6") /* Intel Virtual Buttons Device */)  // _HID: Hardware ID
>       Name (VBDS, Zero)
>       Method (_STA, 0, Serialized)  // _STA: Status
>       {
>           PB1E |= 0x20
>           If ((OSYS >= 0x07DD))
>           {
>               Return (0x0F)
>           }
> 
>           Return (Zero)
>       }
> 
>       Method (VBDL, 0, Serialized)
>       {
>           PB1E |= 0x20
>           VBDS |= 0x40
>       }
> 
>       Method (VGBS, 0, Serialized)
>       {
>           Return (VBDS) /* \_SB_.PCI0.SBRG.EC0_.VGBI.VBDS */
>       }
> 
>       Method (UPBT, 2, Serialized)
>       {
>           Local0 = (One << Arg0)
>           If (Arg1)
>           {
>               VBDS |= Local0
>           }
>           Else
>           {
>               VBDS &= ~Local0
>           }
>       }
>   }
> 
> Method UBPT is called when lid angle cross 180 degree boundary or when
> the device is woken up.
> 
> At boot, VBDS is set to 0 ("tablet mode") until UBPT or VBDL are called.
> 
> VBDL used to be evaluated before VGBS by the intel-vbtn driver probe
> routine, but since commit 26173179fae1 ("platform/x86: intel-vbtn: Eval VBDL after registering our notifier"),
> call to VGBS is delayed until after the notifier is register.
> 
> To bring back the expected behavior (device booting in clamshell
> mode), make sure we evaluate VGBS after VBDL.
> 
> While at it, use function acpi_has_method() when we only need to know if a
> method exist, as commit 26173179fae1 does.
> 
> Gwendal Grignou (2):
>   platform/x86: intel-vbtn: Use acpi_has_method to check for switch
>   platform/x86: intel-vbtn: Update tablet mode switch at end of probe

Thanks, the entire series looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

for the series.

Ilpo I consider this series a bugfix so can you pick this up as a fix for the
6.9 cycle please ?

Regards,

Hans

Re: [PATCH 1/2] platform/x86: intel-vbtn: Use acpi_has_method to check for switch

[Ilpo Järvinen]

On Fri, 29 Mar 2024, Kuppuswamy Sathyanarayanan wrote:
> On 3/29/24 7:32 AM, Gwendal Grignou wrote:
> > To mimic how we check if the device has virtual buttons,
> > acpi_has_method(..."VBDL"), use the same method for checking virtual
> > switch presence.
> 
> if possible don't use words like we/I in the commit log.
> 
> Other wise, it looks fine.
> 
> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> 
> >
> > Signed-off-by: Gwendal Grignou <gwendal@chromium.org>

Thanks all.

I've applied these into review-ilpo branch. I edited the commit message 
to get rid of "we" while applying.

-- 
 i.