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:   Sat, 06 Aug 2022 22:16:35 +1200
From:   Luke Jones <luke@ljones.dev>
Subject: Re: [PATCH 1/5] asus-wmi: Add basic support for TUF laptop keyboard
 RGB
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: <N7V6GR.XFSPFYLCEU9X2@ljones.dev>
In-Reply-To: <CAHp75Ve-oo=pGkpD10kbOVtrYtzQLscFq9HO0-Y5a6M_fpXDNw@mail.gmail.com>
References: <20220805081909.10962-1-luke@ljones.dev>
        <20220805081909.10962-2-luke@ljones.dev>
        <CAHp75Ve-oo=pGkpD10kbOVtrYtzQLscFq9HO0-Y5a6M_fpXDNw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Precedence: bulk

Hi Andy, thanks for the feedback:

On Sat, Aug 6 2022 at 11:44:33 +0200, Andy Shevchenko 
<andy.shevchenko@gmail.com> wrote:
> On Fri, Aug 5, 2022 at 10:20 AM Luke D. Jones <luke@ljones.dev> wrote:
>> 
>>  Adds support for TUF laptop RGB control via the multicolor LED API.
>> 
>>  As this is the base essentials for adjusting the RGB, it sets the
> 
> these are
> ...or...
> essential
> 
>>  default mode of the keyboard to static. This overwrites the booted
>>  state of the keyboard.
> 
> ...
> 
>>   #include <linux/leds.h>
>>  +#include <linux/led-class-multicolor.h>
> 
> Not sure about the ordering ('-' vs. 's') in locale C.
> 

I used hid-playstation.c as a reference and followed that ordering.

> ...
> 
>>  +static int tuf_rgb_brightness_set(struct led_classdev *cdev,
>>  +       enum led_brightness brightness)
>>  +{
>>  +       u8 r, g, b;
>>  +       int err;
>>  +       u32 ret;
> 
>>  +
>>  +       struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
> 
> No need to put blank lines in the definition block. Also it would be
> better to move the longest line to be first.

Okay cool. Done.

> 
>>  +       led_mc_calc_color_components(mc_cdev, brightness);
>>  +       r = mc_cdev->subled_info[0].brightness;
>>  +       g = mc_cdev->subled_info[1].brightness;
>>  +       b = mc_cdev->subled_info[2].brightness;
>>  +
>>  +       /* Writing out requires some defaults. This will overwrite 
>> boot mode */
>>  +       err = asus_wmi_evaluate_method3(ASUS_WMI_METHODID_DEVS, 
>> ASUS_WMI_DEVID_TUF_RGB_MODE,
>>  +                       1 | 0 | (r << 16) | (g << 24), (b) | 0, 
>> &ret);
> 
> What the point in those ' | 0'  additions?

They were place-holders in testing that I forgot to change in the 
second patch which adds mode configuration :(

Should be "save | (mode << 8) | (r << 16) | (g << 24), (b) | (speed << 
8), &ret);", two bytes.

> 
>>  +       if (err) {
>>  +               pr_err("Unable to set TUF RGB data?\n");
> 
> Why not dev_err() ?

I didn't know about it? Is there an example or doc on its use?

> 
>>  +               return err;
>>  +       }
>>  +       return 0;
> 
> return err;

Something like this then?

if (err) {
	pr_err("Unable to set TUF RGB data?\n");
}
return err;

If so, done.

> 
>>  +}
> 
> ...
> 
>>  +       if (asus_wmi_dev_is_present(asus, 
>> ASUS_WMI_DEVID_TUF_RGB_MODE)) {
>>  +               struct led_classdev_mc *mc_cdev;
>>  +               struct mc_subled *mc_led_info;
>>  +               u8 brightness = 127;
> 
>>  +               mc_cdev = &asus->keyboard_rgb_mode.dev;
> 
> Join this with the definition above. It's fine if it's a bit longer
> than 80 characters.

Done.

> 
>>  +               /*
>>  +                * asus::kbd_backlight still controls a base 
>> 3-level backlight and when
>>  +                * it is on 0, the RGB is not visible at all. RGB 
>> should be treated as
>>  +                * an additional step.
>>  +                */
>>  +               mc_cdev->led_cdev.name = 
>> "asus::multicolour::kbd_backlight";
>>  +               mc_cdev->led_cdev.flags = LED_CORE_SUSPENDRESUME | 
>> LED_RETAIN_AT_SHUTDOWN;
>>  +               mc_cdev->led_cdev.brightness_set_blocking = 
>> tuf_rgb_brightness_set;
>>  +               mc_cdev->led_cdev.brightness_get = 
>> tuf_rgb_brightness_get;
>>  +
>>  +               /* Let the multicolour LED own the info */
>>  +               mc_led_info = devm_kmalloc_array(
>>  +                       &asus->platform_device->dev,
> 
> With a temporary variable you may make this one line shorter and 
> nicer looking
> 
>   struct device *dev = &asus->platform_device->dev;
> 

Done.

>>  +                       3,
>>  +                       sizeof(*mc_led_info),
>>  +                       GFP_KERNEL | __GFP_ZERO);
>>  +
>>  +               if (!mc_led_info)
>>  +                       return -ENOMEM;
>>  +
>>  +               mc_led_info[0].color_index = LED_COLOR_ID_RED;
>>  +               mc_led_info[1].color_index = LED_COLOR_ID_GREEN;
>>  +               mc_led_info[2].color_index = LED_COLOR_ID_BLUE;
>>  +
>>  +               /*
>>  +                * It's not possible to get last set data from 
>> device so set defaults
>>  +                * to make it safe for a user to change either RGB 
>> or modes. We don't
>>  +                * write these defaults to the device because they 
>> will overwrite a
>>  +                * users last saved boot setting (in NVRAM).
>>  +                */
>>  +               mc_cdev->led_cdev.brightness = brightness;
>>  +               mc_cdev->led_cdev.max_brightness = brightness;
>>  +               mc_led_info[0].intensity = brightness;
>>  +               mc_led_info[0].brightness = 
>> mc_cdev->led_cdev.brightness;
>>  +               mc_led_info[1].brightness = 
>> mc_cdev->led_cdev.brightness;
>>  +               mc_led_info[2].brightness = 
>> mc_cdev->led_cdev.brightness;
>>  +               led_mc_calc_color_components(mc_cdev, brightness);
>>  +
>>  +               mc_cdev->subled_info = mc_led_info;
>>  +               mc_cdev->num_colors = 3;
>>  +
>>  +               rv = 
>> led_classdev_multicolor_register(&asus->platform_device->dev, 
>> mc_cdev);
> 
> This also becomes shorter.

Done.

> 
>>  +       }
> 
> --
> With Best Regards,
> Andy Shevchenko