Hi Andy, thanks for the feedback:
On Sat, Aug 6 2022 at 11:44:33 +0200, Andy Shevchenko
<andy.shevchenko@xxxxxxxxx> wrote:
On Fri, Aug 5, 2022 at 10:20 AM Luke D. Jones <luke@xxxxxxxxxx> 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
