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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

I think something may be slightly broken. I got the below email twice, once in
reply to where it should be, once as a reply to the cover letter.

Best regards,
Arvid

On 2022-09-27 15:49, Hans de Goede wrote:
> Hi,
> 
> On 9/25/22 20:19, Arvid Norlander wrote:
>> 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@xxxxxxxxx>
>>>> ---
>>>> [...]
>>>> 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.
> 
> I will reply to this question in my general review of the driver.
> 
> Regards,
> 
> Hans
> 
> 
> 
>>
>> <snip>
>>
>>>
>>> Regards,
>>> Barnabás Pőcze
>>
>> Regards,
>> Arvid Norlander
>>
> 




[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux