linux-kernel - Re: [RFC PATCH 2/3] platform/x86: Add Uniwill laptop driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <7c5b6512-1374-41c9-be9a-ac05b573e2cd@kernel.org>
Date: Thu, 19 Jun 2025 22:03:44 +0200
From: Hans de Goede <hansg@...nel.org>
To: Lee Jones <lee@...nel.org>
Cc: Armin Wolf <W_Armin@....de>, Werner Sembach <wse@...edocomputers.com>,
 ilpo.jarvinen@...ux.intel.com, chumuzero@...il.com, corbet@....net,
 cs@...edo.de, ggo@...edocomputers.com, linux-doc@...r.kernel.org,
 linux-kernel@...r.kernel.org, platform-driver-x86@...r.kernel.org,
 linux-leds@...r.kernel.org
Subject: Re: [RFC PATCH 2/3] platform/x86: Add Uniwill laptop driver

Hi Lee,

On 19-Jun-25 5:17 PM, Lee Jones wrote:
> On Thu, 19 Jun 2025, Hans de Goede wrote:
> 
>> Hi Lee,
>>
>> On 19-Jun-25 11:47 AM, Lee Jones wrote:
>>> On Tue, 17 Jun 2025, Armin Wolf wrote:
>>>
>>>> Am 16.06.25 um 14:46 schrieb Werner Sembach:
>>>>
>>>>> Hi, small additon
>>>>>
>>>>> Am 15.06.25 um 19:59 schrieb Armin Wolf:
>>>>>> +        functionality.
>>>>>> +
>>>>>> +What: /sys/bus/wmi/devices/ABBC0F6F-8EA1-11D1-00A0-C90629100000[-X]/rainbow_animation
>>>>>> +Date:        Juni 2025
>>>>>> +KernelVersion:    6.17
>>>>>> +Contact:    Armin Wolf <W_Armin@....de>
>>>>>> +Description:
>>>>>> +        Forces the integrated lightbar to display a rainbow
>>>>>> animation when the machine
>>>>>> +        is not suspended. Writing "enable"/"disable" into this file
>>>>>> enables/disables
>>>>>> +        this functionality.
>>>>>> +
>>>>>> +        Reading this file returns the current status of the rainbow
>>>>>> animation functionality.
>>>>>> +
>>>>>> +What: /sys/bus/wmi/devices/ABBC0F6F-8EA1-11D1-00A0-C90629100000[-X]/breathing_in_suspend
>>>>>> +Date:        Juni 2025
>>>>>> +KernelVersion:    6.17
>>>>>> +Contact:    Armin Wolf <W_Armin@....de>
>>>>>> +Description:
>>>>>> +        Causes the integrated lightbar to display a breathing
>>>>>> animation when the machine
>>>>>> +        has been suspended and is running on AC power. Writing
>>>>>> "enable"/"disable" into
>>>>>> +        this file enables/disables this functionality.
>>>>>> +
>>>>>> +        Reading this file returns the current status of the
>>>>>> breathing animation
>>>>>> +        functionality.
>>>>>
>>>>> maybe this would be better under the /sys/class/leds/*/ tree if possible
>>>>
>>>> I CCed the LED mailing list so that they can give us advice on which location is the preferred one for new drivers.
>>>
>>> No need to involve the LED subsystem for a hardware function controlled
>>> by a single register value just because the interface involves an LED.
>>
>> Lee, the question here is where put the sysfs attribute to put the lightbar
>> in breathing mode e.g. which of these 2 should be used?  :
>>
>> 1. /sys/bus/wmi/devices/ABBC0F6F-8EA1-11D1-00A0-C90629100000[-X]/breathing_in_suspend
>> 2. /sys/class/leds/uniwill-lightbar/breathing_in_suspend
>>
>> I think this is a fair question and since 2. involves the LED class userspace
>> API I also think that asking for the LED maintainers input is reasonable.
>>
>> FWIW I'm not sure myself. 2. is the more logical place / path. But 2. adds
>> a custom sysfs attr the LED class device. Whereas 1. adds a custom sysfs attr
>> in a place where these are more or less expected.
> 
> Right.  It was a reasonable question.  Did I imply otherwise?

Sorry, my bad, I interpreted your "No need to involve the LED
subsystem for a hardware function ..." remark as meaning that
you did not understand why you were Cc-ed.

I now realize that you meant that you believe the control for
this does not need to be under /sys/class/leds/

> If it wasn't clear, my vote (this is not a dictatorship) is for 1.

Ok, 1. works for me and that is what the patch is already doing,
so lets keep it as as.

Regards,

Hans