Re: [PATCH v3] platform/x86: asus-wmi: Add support for SW_TABLET_MODE on UX360

From: Hans de Goede <hdegoede@redhat.com>
To: "Samuel Čavoj" <samuel@cavoj.net>
Cc: Mark Gross <mgross@linux.intel.com>,
	Corentin Chary <corentin.chary@gmail.com>,
	platform-driver-x86@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] platform/x86: asus-wmi: Add support for SW_TABLET_MODE on UX360
Date: Wed, 28 Oct 2020 12:44:32 +0100	[thread overview]
Message-ID: <a108d447-8bbe-dcc1-4ca5-1af4015418bd@redhat.com> (raw)
In-Reply-To: <20201020222914.6k2pkxiirzbefjml@fastboi.localdomain>

Hi,

On 10/21/20 12:29 AM, Samuel Čavoj wrote:
> Hi,
> 
> naturally I notice this right after I send the patch, but my whitespace
> is wrong. Time to set a pre-commit hook up. I guess that means a v4,
> unless you would fix it on your end? It's just a single line.
> 
> Sorry about all the noise,
> Sam
> 
> On 21.10.2020 00:09, Samuel Čavoj wrote:
>> @@ -375,6 +376,20 @@ static int asus_wmi_input_init(struct asus_wmi *asus)
>>  		}
>>  	}
>>  
>> +	if (asus->driver->quirks->use_lid_flip_devid) {
>> +		result = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_LID_FLIP);
>> +        if (result < 0)
> 
> Right ^here.

No worries I will fix this when merging it.

> 
>> +			asus->driver->quirks->use_lid_flip_devid = 0;
>> +		if (result >= 0) {
>> +			input_set_capability(asus->inputdev, EV_SW, SW_TABLET_MODE);
>> +			input_report_switch(asus->inputdev, SW_TABLET_MODE, result);
>> +		} else if (result == -ENODEV) {
>> +			pr_err("This device has lid_flip quirk but got ENODEV checking it. This is a bug.");
>> +		} else {
>> +			pr_err("Error checking for lid-flip: %d\n", result);
>> +		}
>> +	}
> 

>> +	if (asus->driver->quirks->use_lid_flip_devid) {
>> +		result = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_LID_FLIP);
>> +        if (result < 0)
>> +			asus->driver->quirks->use_lid_flip_devid = 0;
> 
> I had the idea of doing this, not sure if it is actually a good idea
> though. The idea is that it would prevent querying the device later
> during runtime, if the first get_devstate fails. Also I assume doing a
> input_report_switch without a corresponding input_set_capability is not
> great either, this would prevent that, if for some reason the
> get_devstate fails in the beginning but later fixes itself.
> 
> However, I can also imagine that writing to the quirks structure is
> frowned upon, please tell if that is the case. Thanks.

The quirk_entry structs are not defined const; and they are already written
to in other places (e.g. the wapf module param handling). So this is fine.

Note FWIW, that calling input_report_switch() with a switch which
has not been declared before through input_set_capability() before
is allowed and is simply treated as a no-op. But just skipping the
handling of the event alltogether is a bit cleaner, so your approach is
fine.

Regards,

Hans