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

[Hans de Goede <hansg@kernel.org>]

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@gmx.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@gmx.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.

Regards,

Hans