Message-ID: <43ca98e5a51d30291efc96ef4758ff508ee143a4.camel@ljones.dev>
Date:   Wed, 10 Aug 2022 10:25:29 +1200
From:   Luke Jones <luke@...nes.dev>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Hans de Goede <hdegoede@...hat.com>,
        Mark Gross <markgross@...nel.org>,
        Platform Driver <platform-driver-x86@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 3/6] asus-wmi: Implement TUF laptop keyboard power
 states

G'day Andy,

On Tue, 2022-08-09 at 10:29 +0200, Andy Shevchenko wrote:
> On Tue, Aug 9, 2022 at 1:27 AM Luke Jones <luke@...nes.dev> wrote:
> 
> ...
> 
> > > >  +       if (sscanf(buf, "%hhd %hhd %hhd %hhd %hhd", &save,
> > > > &boot,
> > > > &awake, &sleep, &keyboard) != 5)
> > > >  +               return -EINVAL;
> > > 
> > > Same Q here: wouldn't it be better to put each of the parameters
> > > to a
> > > separate sysfs node? Or look at the LED ABI (that what Pavel
> > > mentioned
> > > for multi-color patterns) and see if there are already some
> > > established ways of how to represent necessary information?
> > 
> > Same argument I make for the RGB mode nodes. But here I think it's
> > probably even more pertinent. The reasons I would like to keep this
> > as
> > one node are:
> > 
> > - It's separate to the RGB part
> > - We can't read the device to set defaults on boot
> 
> Hmm... Maybe it's done via one of the WMI calls?

That was my hope, but I'm unable to find one. I'm fairly certain that
this set of keyboards uses the same controller as the USB connected one
(the USB one has two versions in circulation also), and I've not been
able to find any packet data that indicates the USB ones send a
"supported".

Checking with `fwts wmi -` reveals nothing (all passes).

I've emailed my contact in the ROG engineering team at ASUS to see if
they can provide any insight.

> 
> > - Because of the above, if we set a default and the user wants to
> > change perhaps "sleep", then we're going to have to write some
> > incorrect guess data since the write requires all the flags at once
> > - One way to improve the UX is to add _show, but then this has to
> > display incorrect data on boot
> > - We end up with 5 more nodes
> > 
> > The same reasons above apply to the RGB nodes, which right now I'm
> > of
> > two minds about. We'll see which way the RGB mode patch goes after
> > some
> > daily use.
> 
> I just realized that in previous mail I mentioned Device Tree which
> is
> irrelevant here. We can't use it on x86 traditional platforms, so it
> means that platform should somehow pass the data to the OS one way or
> another. If there is no way to read back (bad designed interfaces),
> then we can only reset to whatever user provides.
> 

Umm.. Do you mean:
- load module
- module sets a default (all on)
- user or userspace-util sets user preference?

Given that the system daemon I develop (asusd + asusctl) is in very
wide use, I guess it's not such a big issue to both split these nodes
out and set a default.. I guess I'll go ahead and keep the expectation
that the reworked RGB-mode patch sets.

It seems to me that the appropriate way to do the "write-out" for both
mode and state is to have nodes:
- keyboard_rgb_mode_apply
- keyboard_rgb_state_apply
- Input 0 = set (doesn't stick on boot), 1 = save

going with the above I should rename existing nodes, especially the
current *_save node. And this brings me to my next issue: currently
behaviour for the *_apply is:
- write 0 applies, but doesn't stick
- write 1 applies, and sticks on boot
- reading the *_apply nodes will show 0/1
- if already "1", you still need to overwrite with "1" to apply.

This doesn't seem appropriate does it?
Should there be a WO node for *_apply, and another node for *_save
(which would store/show the setting)? I'm inclined to think so, and
that this will add quite a bit of clutter (6 nodes for state, 4 for
mode).

Kind regards,
Luke.

Powered by blists - more mailing lists