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

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

[Armin Wolf]

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.

Thanks,
Armin Wolf

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

[Lee Jones]

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 Jones [李琼斯]

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

[Hans de Goede]

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

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

[Lee Jones]

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?

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

-- 
Lee Jones [李琼斯]

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

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

[Armin Wolf]

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