Re: [PATCH 1/2] platform/x86: asus-wmi: keep the lid-flip state when UNKNOWN is set

On 8/10/26 15:20, Hans de Goede wrote:
> Hi Denis,
>
> On 5-Aug-26 2:07 PM, Denis Benato wrote:
>> On 8/5/26 12:15, Robin Everaars wrote:
>>> On some convertibles the lid-flip devstate sets ASUS_WMI_DSTS_UNKNOWN_BIT
>>> alongside the state bit while the machine is folded. Measured on an ASUS
>>> ProArt PX13 (HN7306EAC), ASUS_WMI_DEVID_LID_FLIP_ROG answers 0x00010000
>>> open and 0x00010003 folded, that is presence | status | UNKNOWN.
>>>
>>> asus_wmi_get_devstate_simple() treats that bit as "the state is not known"
>>> and fails the call with -ENODEV, so asus_wmi_tablet_mode_get_state()
>>> discards a perfectly good state sitting in bit 0 and SW_TABLET_MODE never
>>> moves.
>>>
>>> Add asus_wmi_tablet_sw_get_state(), which gates on the presence bit only
>>> and returns the status bit. Use it from the two tablet-switch paths. Every
>>> other caller of asus_wmi_get_devstate_simple() is untouched, so the change
>>> is confined to the tablet switch.
>> Hi Robin,
>>
>> Thanks for looking into this!
>>> Signed-off-by: Robin Everaars <[email protected]>
>>> ---
>>>  drivers/platform/x86/asus-wmi.c | 32 ++++++++++++++++++++++++++++++--
>>>  1 file changed, 30 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86
>>> /asus-wmi.c b/drivers/platform/x86/asus-wmi.c
>>> index 8610663b8..f68fd2bcd 100644
>>> --- a/drivers/platform/x86/asus-wmi.c
>>> +++ b/drivers/platform/x86/asus-wmi.c
>>> @@ -706,12 +706,40 @@ static void asus_wmi_tablet_sw_report(struct asus_wmi *asus, bool value)
>>>  	input_sync(asus->inputdev);
>>>  }
>>>  
>>> +/*
>>> + * Read the lid-flip state directly rather than through
>>> + * asus_wmi_get_devstate_simple().
>>> + *
>>> + * On some convertibles the lid-flip devstate sets ASUS_WMI_DSTS_UNKNOWN_BIT
>>> + * alongside the state bit while folded. Measured on an ASUS ProArt PX13
>>> + * (HN7306EAC), devid ASUS_WMI_DEVID_LID_FLIP_ROG answers 0x00010000 open and
>>> + * 0x00010003 folded, i.e. presence | state | UNKNOWN. The generic helper reads
>>> + * that bit as "the state is not known" and rejects the whole call with -ENODEV,
>>> + * so asus_wmi_tablet_mode_get_state() discards a perfectly good state sitting in
>>> + * bit 0 and the switch never moves. Only presence gates the value here, which is
>>> + * safe because this path serves 
>>> the tablet switch alone.
>>> + */
>>> +static int asus_wmi_tablet_sw_get_state(struct asus_wmi *asus, u32 dev_id)
>>> +{
>>> +	u32 retval;
>>> +	int err;
>>> +
>>> +	err = asus_wmi_get_devstate(asus, dev_id, &retval);
>>> +	if (err < 0)
>>> +		return err;
>>> +
>>> +	if (!(retval & ASUS_WMI_DSTS_PRESENCE_BIT))
>>> +		return -ENODEV;
>> There are FIELD_GET and many more macros to do this,
>> please use those as it makes the code easier to read.
> I appreciate that you are trying to help out with reviewing, but in this case this
> is not good advice.
>
> Using FIELD_GET is good advice for new code, but for an existing driver it is
> more important to be consistent and so far no code in asus-wmi.c is using
> FIELD_GET so adding this just makes the whole driver harder to read since
> now it is mixing 2 styles.
Aaaah, my bad, sorry.

> Regards,
>
> Hans
>
>
>
>>> +
>>> +	return !!(retval & ASUS_WMI_DSTS_STATUS_BIT);
>> Same here
>>> +}
>>> +
>>>  static void asus_wmi_tablet_sw_init(struct asus_wmi *asus, u32 dev_id, int event_code)
>>>  {
>>>  	struct device *dev = &asus->platform_device->dev;
>>>  	int result;
>>>  
>>> -	result = asus_wmi_get_devstate_simple(asus, dev_id);
>>> +	result = asus_wmi_tablet_sw_get_state(asus, dev_id);
>>>  	if (result >= 0) {
>>>  		input_set_capability(asus->inputdev, EV_SW, SW_TABLET_MODE);
>>>  		asus_wmi_tablet_sw_report(asus, result);
>>> @@ -786,7 +814,7 @@ static void asus_wmi_tablet_mode_get_state(struct asus_wmi *asus)
>>>  	if (!asus->tablet_switch_dev_id)
>>>  		return;
>>>  
>>> -	result = asus_wmi_get_devstate_simple(asus, asus->tablet_switch_dev_id);
>>> +	result = asus_wmi
>>> _tablet_sw_get_state(asus, asus->tablet_switch_dev_id);
>>>  	if (result >= 0)
>>>  		asus_wmi_tablet_sw_report(asus, result);
>>>  }