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

[Armin Wolf <W_Armin@gmx.de>]

Am 19.06.25 um 22:03 schrieb Hans de Goede:

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

Fine with me.

Armin Wolf