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 20:58:12 +1200
From:   Luke Jones <luke@ljones.dev>
Subject: Re: [PATCH v3 3/6] asus-wmi: Implement TUF laptop keyboard power
 states
To:     Andy Shevchenko <andy.shevchenko@gmail.com>
Cc:     Hans de Goede <hdegoede@redhat.com>,
        =?iso-8859-2?q?Barnab=E1s_P=F5cze?= <pobrn@protonmail.com>,
        Pavel Machek <pavel@ucw.cz>,
        Platform Driver <platform-driver-x86@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Message-Id: <0LBCGR.NDLPJAR4JTJQ@ljones.dev>
In-Reply-To: <CAHp75VenHd6JREvxjmcNbeshvgv-S_th9pqBBgQJ+FBgPzgWqA@mail.gmail.com>
References: <20220809025054.1626339-1-luke@ljones.dev>
        <20220809025054.1626339-4-luke@ljones.dev>
        <CAHp75VenHd6JREvxjmcNbeshvgv-S_th9pqBBgQJ+FBgPzgWqA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Precedence: bulk

Hi Andy,

On Tue, Aug 9 2022 at 10:52:21 +0200, Andy Shevchenko 
<andy.shevchenko@gmail.com> wrote:
> On Tue, Aug 9, 2022 at 4:51 AM Luke D. Jones <luke@ljones.dev> wrote:
>> 
>>  Adds support for setting various power states of TUF keyboards.
>>  These states are combinations of:
>>  - boot, set if a boot animation is shown on keyboard
>>  - awake, set if the keyboard LEDs are visible while laptop is on
>>  - sleep, set if an animation is displayed while the laptop is 
>> suspended
>>  - keyboard (unknown effect)
>> 
>>  Adds two sysfs attributes to asus-nb-wmi:
>>  - keyboard_rgb_state
>>  - keyboard_rgb_state_index
> 
> ...
> 
>>                  until the keyboard_rgb_save attribute is set 
>> (write-only):
>>                          * 0 - slow
>>                          * 1 - medium
>>  -                       * 2 - fast
>>  \ No newline at end of file
> 
> ^^^
> 
>>  +                       * 2 - fast
>>  +
>>  +What:          /sys/devices/platform/<platform>/keyboard_rgb_state
>>  +Date:          Aug 2022
>>  +KernelVersion: 6.1
>>  +Contact:       "Luke Jones" <luke@ljones.dev>
>>  +Description:
>>  +               Set some RGB keyboard power states (write-only).
>>  +
>>  +               The accepted input is "boot awake sleep keyboard", 
>> where "n n n n n"
>>  +               options are:
>>  +                       * save - 0 or 1, if 0 then settings are not 
>> retained on boot
>>  +                       * boot - 0 or 1, controls if a boot 
>> animation is shown
>>  +                       * awake - 0 or 1, controls if the keyboard 
>> LED are on during awake
>>  +                       * sleep - 0 or 1, controls if a suspended 
>> animation is shown.
>>  +                                               This is only active 
>> if the AC is connected.
>>  +                       * keyboard - 0 or 1, unknown what effect 
>> this really has
>>  \ No newline at end of file
> 
> ^^^
> 
> Be sure of not repeating the same mistake. I.e. add a newline as 
> suggested.
> 
> 
>>  +       flags = 0;
> 
> Seems you ignored my comment here...
> 
>>  +       if (sscanf(buf, "%hhd %hhd %hhd %hhd %hhd", &save, &boot, 
>> &awake, &sleep, &keyboard) != 5
>>  +               return -EINVAL;
> 
>>  +       save = save == 0 ? 0x0100 : 0x0000;
> 
> ...and here...
> 
>>  +       if (boot)
>>  +               flags |= 0x02;
>>  +       if (awake)
>>  +               flags |= 0x08;
>>  +       if (sleep)
>>  +               flags |= 0x20;
>>  +       if (keyboard)
>>  +               flags |= 0x80;
> 
> ...and here.
> 

Umm... I know for sure I fixed all those. I must have screwed up a git 
rebase :(
I'll fix again.