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.
...
+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.
+ 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?
+ if (err) {
+ pr_err("Unable to set TUF RGB data?\n");
Why not dev_err() ?
+ return err;
+ }
+ return 0;
return err;
+}
...
+ 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.
+ /*
+ * 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;
+ 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.
+ }
--
With Best Regards,
Andy Shevchenko