Re: [PATCH RFC v2 1/2] platform/x86: quickstart: Add ACPI quickstart button (PNP0C32) driver

From: Arvid Norlander <lkml@vorpal.se>
To: "Barnabás Pőcze" <pobrn@protonmail.com>
Cc: platform-driver-x86@vger.kernel.org,
	Hans de Goede <hdegoede@redhat.com>,
	linux-acpi@vger.kernel.org, Len Brown <lenb@kernel.org>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	linux-input@vger.kernel.org,
	Azael Avalos <coproscefalo@gmail.com>
Subject: Re: [PATCH RFC v2 1/2] platform/x86: quickstart: Add ACPI quickstart button (PNP0C32) driver
Date: Sun, 25 Sep 2022 20:19:34 +0200	[thread overview]
Message-ID: <3095147c-844c-42cf-833b-8a2eae5fcc21@vorpal.se> (raw)
In-Reply-To: <4-mkye9NM7L93IKQAGjd8BmHi1_2zEnx4F8L3AvKk9RsNBtuoS5cpNCKV-nyb1Xpb1jmAZQDdpNlyvjoUfrFKkq4V-EOfXo9b_gRbyC1hSs=@protonmail.com>

Hi,

Thank you, I have incorperated your feedback in my local branch.

On 2022-09-23 21:24, Barnabás Pőcze wrote:
> Hi
> 
> 2022. szeptember 22., csütörtök 20:24 keltezéssel, Arvid Norlander írta:
> 
>> This is loosely based on a previous staging driver that was removed. See
>> links below for more info on that driver. The original commit ID was
>> 0be013e3dc2ee79ffab8a438bbb4e216837e3d52.
>>
>> However, here a completely different approach is taken to the user space
>> API (which should solve the issues the original driver had). Each PNP0C32
>> device is a button, and each such button gets a separate input device
>> associated with it (instead of a shared platform input device).
>>
>> The button ID (as read from ACPI method GHID) is provided via a sysfs file
>> "button_id".
>>
>> If the button caused a wakeup it will "latch" the "wakeup_cause" sysfs file
>> to true. This can be reset by a user space process.
>>
>> Link: https://marc.info/?l=linux-acpi&m=120550727131007
>> Link: https://lkml.org/lkml/2010/5/28/327
>> Signed-off-by: Arvid Norlander <lkml@vorpal.se>
>> ---
>> [...]
>> diff --git a/drivers/platform/x86/quickstart.c b/drivers/platform/x86/quickstart.c
>> new file mode 100644
>> index 000000000000..ce51abe012f7
>> --- /dev/null
>> +++ b/drivers/platform/x86/quickstart.c
>> @@ -0,0 +1,320 @@

<snip>

>> +
>> +static ssize_t wakeup_cause_store(struct device *dev,
>> +				  struct device_attribute *attr,
>> +				  const char *buf, size_t count)
>> +{
>> +	struct quickstart_acpi *quickstart = dev_get_drvdata(dev);
>> +
>> +	if (count < 2)
>> +		return -EINVAL;
>> +
>> +	if (strncasecmp(buf, "false", 4) != 0)
>> +		return -EINVAL;
>> +
> 
> If "true"/"false" will be used in the final version, then I think this check
> currently is too lax. You could use `sysfs_streq()`. And I think the `count < 2`
> check is not needed.

Regarding the user space API I don't know, that is one of the open
questions in the cover letter. I have yet to get any feedback on any of
those questions. That is something that needs to happen before this driver
can be included. I would appreciate your feedback on those.

<snip>

> 
> Regards,
> Barnabás Pőcze

Regards,
Arvid Norlander