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

[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@xxxxxx>
>>>>>> +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@xxxxxx>
>>>>>> +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