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

Fine with me.

Armin Wolf