Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i5ec1447f:Fastmail
Date:   Tue, 09 Aug 2022 11:27:33 +1200
From:   Luke Jones <luke@ljones.dev>
Subject: Re: [PATCH v2 3/6] asus-wmi: Implement TUF laptop keyboard power
 states
To:     Andy Shevchenko <andy.shevchenko@gmail.com>
Cc:     Hans de Goede <hdegoede@redhat.com>,
        Mark Gross <markgross@kernel.org>,
        Platform Driver <platform-driver-x86@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Message-Id: <X5LBGR.15DO7CCYH6ER@ljones.dev>
In-Reply-To: <CAHp75VdNa8csSF5Je87kOzC3EQ4yetZqpxXFZu1D7C4A6XCTNg@mail.gmail.com>
References: <20220808030420.8633-1-luke@ljones.dev>
        <20220808030420.8633-4-luke@ljones.dev>
        <CAHp75VdNa8csSF5Je87kOzC3EQ4yetZqpxXFZu1D7C4A6XCTNg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Precedence: bulk

Hi Andy

>>  +       flags = 0;
> 
> This can be done before 'if (boot)'

Okay done.

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

> 
>>  +       save = save == 0 ? 0x0100 : 0x0000;
> 
>   if (save)
>     flags = BIT(8);

I didn't know about BIT(). Will do.

> 
>>  +       if (boot)
>>  +               flags |= 0x02;
>>  +       if (awake)
>>  +               flags |= 0x08;
>>  +       if (sleep)
>>  +               flags |= 0x20;
>>  +       if (keyboard)
>>  +               flags |= 0x80;
> 
> Use BIT() for flags.
> 
> ...
> 
>>  +       err = asus_wmi_evaluate_method3(ASUS_WMI_METHODID_DEVS,
>>  +                       ASUS_WMI_DEVID_TUF_RGB_STATE, 0xBD | save | 
>> (flags << 16), 0, &ret);
> 
> Why not provide flags to be a full 32-bit value?
> 
> Also 0xBD can be lower-cased and explained somehow?

Done, as is the rest of comments

Kind regards,
Luke.
>