Adds support for changing the laptop keyboard LED modes. These
are visible effects such as static, rainbow, pulsing, colour cycles.

Two sysfs attributes are added to asus-nb-wmi:
- keyboard_rgb_mode
- keyboard_rgb_mode_index

Signed-off-by: Luke D. Jones <luke@ljones.dev>
---
 drivers/platform/x86/asus-wmi.c | 83 +++++++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 233e73f4313d..4c2bdd9dac30 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -246,6 +246,7 @@ struct asus_wmi {
 	bool dgpu_disable_available;
 	bool dgpu_disable;
 
+	bool keyboard_rgb_mode_available;
 	struct keyboard_rgb_led keyboard_rgb_mode;
 
 	bool throttle_thermal_policy_available;
@@ -748,6 +749,77 @@ static ssize_t egpu_enable_store(struct device *dev,
 
 static DEVICE_ATTR_RW(egpu_enable);
 
+/* TUF Laptop Keyboard RGB Modes **********************************************/
+static int keyboard_rgb_mode_check_present(struct asus_wmi *asus)
+{
+	u32 result;
+	int err;
+
+	asus->keyboard_rgb_mode_available = false;
+
+	err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_TUF_RGB_MODE, &result);
+	if (err) {
+		if (err == -ENODEV)
+			return 0;
+		return err;
+	}
+
+	if (result & ASUS_WMI_DSTS_PRESENCE_BIT)
+		asus->keyboard_rgb_mode_available = true;
+
+	return 0;
+}
+
+static ssize_t keyboard_rgb_mode_store(struct device *device,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	u8 save, mode, speed;
+	int err;
+
+	struct asus_wmi *asus = dev_get_drvdata(device);
+	struct led_classdev *cdev = &asus->keyboard_rgb_mode.dev.led_cdev;
+
+	if (sscanf(buf, "%hhd %hhd %hhd", &save, &mode, &speed) != 3)
+		return -EINVAL;
+
+	asus->keyboard_rgb_mode.save = !!save;
+
+	/* These are the known usable modes across all TUF/ROG */
+	asus->keyboard_rgb_mode.mode = mode < 12 && mode != 9 ? mode : 0x0a;
+
+	switch (speed) {
+	case 0:
+		asus->keyboard_rgb_mode.speed = 0xe1;
+		break;
+	case 1:
+		asus->keyboard_rgb_mode.speed = 0xeb;
+		break;
+	case 2:
+		asus->keyboard_rgb_mode.speed = 0xf5;
+		break;
+	default:
+		asus->keyboard_rgb_mode.speed = 0xeb;
+	}
+
+	err = tuf_rgb_brightness_set(cdev, cdev->brightness);
+	if (err)
+		return err;
+
+	return count;
+}
+
+static DEVICE_ATTR_WO(keyboard_rgb_mode);
+
+static ssize_t keyboard_rgb_mode_index_show(struct device *device,
+						 struct device_attribute *attr,
+						 char *buf)
+{
+	return sysfs_emit(buf, "%s\n", "save mode speed");
+}
+
+static DEVICE_ATTR_RO(keyboard_rgb_mode_index);
+
 /* Battery ********************************************************************/
 
 /* The battery maximum charging percentage */
@@ -3338,6 +3410,8 @@ static struct attribute *platform_attributes[] = {
 	&dev_attr_touchpad.attr,
 	&dev_attr_egpu_enable.attr,
 	&dev_attr_dgpu_disable.attr,
+	&dev_attr_keyboard_rgb_mode.attr,
+	&dev_attr_keyboard_rgb_mode_index.attr,
 	&dev_attr_lid_resume.attr,
 	&dev_attr_als_enable.attr,
 	&dev_attr_fan_boost_mode.attr,
@@ -3368,6 +3442,10 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
 		ok = asus->egpu_enable_available;
 	else if (attr == &dev_attr_dgpu_disable.attr)
 		ok = asus->dgpu_disable_available;
+	else if (attr == &dev_attr_keyboard_rgb_mode.attr)
+		ok = asus->keyboard_rgb_mode_available;
+	else if (attr == &dev_attr_keyboard_rgb_mode_index.attr)
+		ok = asus->keyboard_rgb_mode_available;
 	else if (attr == &dev_attr_fan_boost_mode.attr)
 		ok = asus->fan_boost_mode_available;
 	else if (attr == &dev_attr_throttle_thermal_policy.attr)
@@ -3637,6 +3715,10 @@ static int asus_wmi_add(struct platform_device *pdev)
 	if (err)
 		goto fail_dgpu_disable;
 
+	err = keyboard_rgb_mode_check_present(asus);
+	if (err)
+		goto fail_keyboard_rgb_mode;
+
 	err = fan_boost_mode_check_present(asus);
 	if (err)
 		goto fail_fan_boost_mode;
@@ -3751,6 +3833,7 @@ static int asus_wmi_add(struct platform_device *pdev)
 fail_fan_boost_mode:
 fail_egpu_enable:
 fail_dgpu_disable:
+fail_keyboard_rgb_mode:
 fail_platform:
 fail_panel_od:
 	kfree(asus);
-- 
2.37.1

On Mon, Aug 8, 2022 at 5:07 AM Luke D. Jones <luke@ljones.dev> wrote:
>
> Adds support for changing the laptop keyboard LED modes. These
> are visible effects such as static, rainbow, pulsing, colour cycles.
>
> Two sysfs attributes are added to asus-nb-wmi:
> - keyboard_rgb_mode
> - keyboard_rgb_mode_index

...

> +       if (sscanf(buf, "%hhd %hhd %hhd", &save, &mode, &speed) != 3)
> +               return -EINVAL;

Same comment as per v1.

...

> +       asus->keyboard_rgb_mode.mode = mode < 12 && mode != 9 ? mode : 0x0a;

Same comment as per v1.

...

> +       switch (speed) {
> +       case 0:
> +               asus->keyboard_rgb_mode.speed = 0xe1;
> +               break;
> +       case 1:
> +               asus->keyboard_rgb_mode.speed = 0xeb;
> +               break;
> +       case 2:
> +               asus->keyboard_rgb_mode.speed = 0xf5;
> +               break;
> +       default:
> +               asus->keyboard_rgb_mode.speed = 0xeb;

break;

> +       }

...

> +

A blank line is not needed here.

> +static DEVICE_ATTR_WO(keyboard_rgb_mode);

-- 
With Best Regards,
Andy Shevchenko

Hi Andy,

> 
>>  +       if (sscanf(buf, "%hhd %hhd %hhd", &save, &mode, &speed) != 
>> 3)
>>  +               return -EINVAL;
> 
> Same comment as per v1.
> 

You wrote:

 > Usually we have three separate nodes for that, but they are kinda
 > hidden in one driver, so I don't care much.

I think that is the wrong direction to take. Doing so would mean that 
every write to one of these values has to write-out to device. I don't 
know how long writes on an i2c device take, but on the USB connected 
versions they are 1ms, which means that to individually set colour, 
save, mode, speed (also direction and sometimes a 2nd colour on USB) 
adds up to 4-6ms - and I don't know what sort of impact that has in the 
kernel itself, but I do know that users expect there to be fancy 
effects available on par with Windows (like audio equalizer visuals on 
the RGB, something that is in progress in asusctl).

Using multicolor LED class already breaks away from having a single 
packet write, but the gain in API scope was worth the compromise. 
Hopefully we can keep the single set of parameters here?

Pavel suggested using triggers, I've yet to look at that, but will do 
so after finalising this.

I suppose one alternative would be to store speed and mode as 
attributes, but not write out until the "save" node is written to? So 
this raises the question of: we can't read from device, and speed+mode 
must be saved in module for use with "save" now, should I then allow 
showing these values in a _show? On fresh boot they will be incorrect..

I'm going to go ahead and split those parameters in to individual nodes 
now anyway - it may help with later work using triggers.


> ...
> 
>>  +       asus->keyboard_rgb_mode.mode = mode < 12 && mode != 9 ? 
>> mode : 0x0a;
> 
> Same comment as per v1.
> 

I missed it sorry. Done now.

> ...
> 
>>  +       switch (speed) {
>>  +       case 0:
>>  +               asus->keyboard_rgb_mode.speed = 0xe1;
>>  +               break;
>>  +       case 1:
>>  +               asus->keyboard_rgb_mode.speed = 0xeb;
>>  +               break;
>>  +       case 2:
>>  +               asus->keyboard_rgb_mode.speed = 0xf5;
>>  +               break;
>>  +       default:
>>  +               asus->keyboard_rgb_mode.speed = 0xeb;
> 
> break;

Done

> 
>>  +       }
> 
> ...
> 
>>  +
> 
> A blank line is not needed here.

Okay thanks, I'll go through previous patches and check this.

Kind regards,
Luke.
>

On Mon, Aug 8, 2022 at 11:43 PM Luke Jones <luke@ljones.dev> wrote:

...

> >>  +       if (sscanf(buf, "%hhd %hhd %hhd", &save, &mode, &speed) !=
> >> 3)
> >>  +               return -EINVAL;
> >
> > Same comment as per v1.
>
> You wrote:
>
>  > Usually we have three separate nodes for that, but they are kinda
>  > hidden in one driver, so I don't care much.
>
> I think that is the wrong direction to take. Doing so would mean that
> every write to one of these values has to write-out to device. I don't
> know how long writes on an i2c device take, but on the USB connected
> versions they are 1ms, which means that to individually set colour,
> save, mode, speed (also direction and sometimes a 2nd colour on USB)
> adds up to 4-6ms - and I don't know what sort of impact that has in the
> kernel itself, but I do know that users expect there to be fancy
> effects available on par with Windows (like audio equalizer visuals on
> the RGB, something that is in progress in asusctl).

This is a good justification, but easy to workaround by using another
node like "submit" or unifying it with one of the existing nodes
implicitly (like setting savee will submit all changes at once).

> Using multicolor LED class already breaks away from having a single
> packet write, but the gain in API scope was worth the compromise.
> Hopefully we can keep the single set of parameters here?
>
> Pavel suggested using triggers, I've yet to look at that, but will do
> so after finalising this.

The thing is you can't "finalize" this and go to another type of ABI,
because we come with two ABIs - we may not drop the old one once it's
established (yes, there are exceptions, but it's rare). So, knowing
that we would drop an ABI we mustn't introduce it in the first place.

> I suppose one alternative would be to store speed and mode as
> attributes, but not write out until the "save" node is written to?

Yes!

> So
> this raises the question of: we can't read from device, and speed+mode
> must be saved in module for use with "save" now, should I then allow
> showing these values in a _show? On fresh boot they will be incorrect..

Yep, they should be reset either by hardware, or provided somehow
(device tree?) from the platform knowing what firmware sets and what
kernel may use if you don't want to change the settings.

> I'm going to go ahead and split those parameters in to individual nodes
> now anyway - it may help with later work using triggers.

Okay!

-- 
With Best Regards,
Andy Shevchenko