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:   Fri, 26 Aug 2022 21:56:57 +1200
From:   Luke Jones <luke@ljones.dev>
Subject: Re: [PATCH v4 1/2] asus-wmi: Implement TUF laptop keyboard LED modes
To:     Hans de Goede <hdegoede@redhat.com>
Cc:     markgross@kernel.org, platform-driver-x86@vger.kernel.org,
        linux-kernel@vger.kernel.org, andy.shevchenko@gmail.com,
        pobrn@protonmail.com, pavel@ucw.cz
Message-Id: <XMV7HR.FTECLY4PXEQX@ljones.dev>
In-Reply-To: <0a45a991-4123-a76c-174f-435523f6d348@redhat.com>
References: <20220825232251.345893-1-luke@ljones.dev>
        <20220825232251.345893-2-luke@ljones.dev>
        <0a45a991-4123-a76c-174f-435523f6d348@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Precedence: bulk


On Fri, Aug 26 2022 at 11:47:37 +0200, Hans de Goede 
<hdegoede@redhat.com> wrote:
> Hi Luke,
> 
> On 8/26/22 01:22, Luke D. Jones wrote:
>>  Adds support for changing the laptop keyboard LED mode and colour.
>> 
>>  The modes are visible effects such as static, rainbow, pulsing,
>>  colour cycles.
>> 
>>  These sysfs attributes are added to asus::kbd_backlight:
>>  - kbd_rgb_mode
>>  - kbd_rgb_mode_index
>> 
>>  Signed-off-by: Luke D. Jones <luke@ljones.dev>
>>  ---
>>   drivers/platform/x86/asus-wmi.c            | 76 
>> +++++++++++++++++++++-
>>   include/linux/platform_data/x86/asus-wmi.h |  3 +
>>   2 files changed, 78 insertions(+), 1 deletion(-)
>> 
>>  diff --git a/drivers/platform/x86/asus-wmi.c 
>> b/drivers/platform/x86/asus-wmi.c
>>  index 0f9f79f249c7..92f16bb9b4ef 100644
>>  --- a/drivers/platform/x86/asus-wmi.c
>>  +++ b/drivers/platform/x86/asus-wmi.c
>>  @@ -237,6 +237,8 @@ struct asus_wmi {
>>   	bool dgpu_disable_available;
>>   	bool gpu_mux_mode_available;
>> 
>>  +	bool kbd_rgb_mode_available;
>>  +
>>   	bool throttle_thermal_policy_available;
>>   	u8 throttle_thermal_policy_mode;
>> 
>>  @@ -720,6 +722,69 @@ static ssize_t gpu_mux_mode_store(struct 
>> device *dev,
>>   }
>>   static DEVICE_ATTR_RW(gpu_mux_mode);
>> 
>>  +/* TUF Laptop Keyboard RGB Modes 
>> **********************************************/
>>  +static ssize_t kbd_rgb_mode_store(struct device *dev,
>>  +				 struct device_attribute *attr,
>>  +				 const char *buf, size_t count)
>>  +{
>>  +	u32 cmd, mode, r, g,  b,  speed;
>>  +	int err;
>>  +
>>  +	if (sscanf(buf, "%d %d %d %d %d %d", &cmd, &mode, &r, &g, &b, 
>> &speed) != 6)
>>  +		return -EINVAL;
>>  +
>>  +	cmd = !!cmd;
>>  +
>>  +	/* These are the known usable modes across all TUF/ROG */
>>  +	if (mode >= 12 || mode == 9)
>>  +		mode = 10;
>>  +
>>  +	switch (speed) {
>>  +	case 0:
>>  +		speed = 0xe1;
>>  +		break;
>>  +	case 1:
>>  +		speed = 0xeb;
>>  +		break;
>>  +	case 2:
>>  +		speed = 0xf5;
>>  +		break;
>>  +	default:
>>  +		speed = 0xeb;
>>  +	}
>>  +
>>  +	err = asus_wmi_evaluate_method3(ASUS_WMI_METHODID_DEVS, 
>> ASUS_WMI_DEVID_TUF_RGB_MODE,
>>  +			cmd | (mode << 8) | (r << 16) | (g << 24), b | (speed << 8), 
>> NULL);
>>  +	if (err)
>>  +		return err;
>>  +
>>  +	return count;
>>  +}
>>  +static DEVICE_ATTR_WO(kbd_rgb_mode);
>>  +
>>  +static ssize_t kbd_rgb_mode_index_show(struct device *device,
>>  +						 struct device_attribute *attr,
>>  +						 char *buf)
>>  +{
>>  +	return sysfs_emit(buf, "%s\n", "cmd mode red green blue speed");
>>  +}
>>  +static DEVICE_ATTR_RO(kbd_rgb_mode_index);
>>  +
>>  +static struct attribute *kbd_rgb_mode_attrs[] = {
>>  +	&dev_attr_kbd_rgb_mode.attr,
>>  +	&dev_attr_kbd_rgb_mode_index.attr,
>>  +	NULL,
>>  +};
>>  +
>>  +static const struct attribute_group kbd_rgb_mode_group = {
>>  +	.attrs = kbd_rgb_mode_attrs,
>>  +};
>>  +
>>  +const struct attribute_group *kbd_rgb_mode_groups[] = {
>>  +	NULL,
>>  +	NULL,
>>  +};
>>  +
>>   /* Battery 
>> ********************************************************************/
>> 
>>   /* The battery maximum charging percentage */
>>  @@ -1038,7 +1103,10 @@ static void asus_wmi_led_exit(struct 
>> asus_wmi *asus)
>> 
>>   static int asus_wmi_led_init(struct asus_wmi *asus)
>>   {
>>  -	int rv = 0, led_val;
>>  +	int rv = 0, num_rgb_groups = 0, led_val;
>>  +
>>  +	if (asus->kbd_rgb_mode_available)
>>  +		kbd_rgb_mode_groups[num_rgb_groups++] = &kbd_rgb_mode_group;
>> 
>>   	asus->led_workqueue = 
>> create_singlethread_workqueue("led_workqueue");
>>   	if (!asus->led_workqueue)
>>  @@ -1066,6 +1134,9 @@ static int asus_wmi_led_init(struct asus_wmi 
>> *asus)
>>   		asus->kbd_led.brightness_get = kbd_led_get;
>>   		asus->kbd_led.max_brightness = 3;
>> 
>>  +		if (num_rgb_groups != 0)
>>  +			asus->kbd_led.groups = kbd_rgb_mode_groups;
>>  +
>>   		rv = led_classdev_register(&asus->platform_device->dev,
>>   					   &asus->kbd_led);
>>   		if (rv)
>>  @@ -3253,6 +3324,8 @@ 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_dgpu_disable.attr)
>>  +		ok = asus->dgpu_disable_available;
>>   	else if (attr == &dev_attr_gpu_mux_mode.attr)
>>   		ok = asus->gpu_mux_mode_available;
>>   	else if (attr == &dev_attr_fan_boost_mode.attr)
> 
> This patch-hunk looks like it is a (mangled) leftover from previous 
> versions
> of the patch.

Do you mean:

 +	else if (attr == &dev_attr_dgpu_disable.attr)
 +		ok = asus->dgpu_disable_available;


> 
> Other then that this latest version of the series looks nice and 
> clean,
> so I've added the series to my review-hans branch minus this 
> patch-hunk.
> 
> One request can you do a follow up patch documenting the new
> attributes in Documentation/ABI/testing/sysfs-platform-asus-wmi
> I know the attributes sit under the LED class device, but this
> still seems the best place for documenting them.

Yes for sure. I wasn't actually certain where I should put the doc.

> 
> Regards,
> 
> Hans
> 
> 
> 
> 
>>  @@ -3519,6 +3592,7 @@ static int asus_wmi_add(struct 
>> platform_device *pdev)
>>   	asus->egpu_enable_available = asus_wmi_dev_is_present(asus, 
>> ASUS_WMI_DEVID_EGPU);
>>   	asus->dgpu_disable_available = asus_wmi_dev_is_present(asus, 
>> ASUS_WMI_DEVID_DGPU);
>>   	asus->gpu_mux_mode_available = asus_wmi_dev_is_present(asus, 
>> ASUS_WMI_DEVID_GPU_MUX);
>>  +	asus->kbd_rgb_mode_available = asus_wmi_dev_is_present(asus, 
>> ASUS_WMI_DEVID_TUF_RGB_MODE);
>>   	asus->panel_overdrive_available = asus_wmi_dev_is_present(asus, 
>> ASUS_WMI_DEVID_PANEL_OD);
>> 
>>   	err = fan_boost_mode_check_present(asus);
>>  diff --git a/include/linux/platform_data/x86/asus-wmi.h 
>> b/include/linux/platform_data/x86/asus-wmi.h
>>  index 6e8a95c10d17..3d861477cb20 100644
>>  --- a/include/linux/platform_data/x86/asus-wmi.h
>>  +++ b/include/linux/platform_data/x86/asus-wmi.h
>>  @@ -103,6 +103,9 @@
>>   /* gpu mux switch, 0 = dGPU, 1 = Optimus */
>>   #define ASUS_WMI_DEVID_GPU_MUX		0x00090016
>> 
>>  +/* TUF laptop RGB modes/colours */
>>  +#define ASUS_WMI_DEVID_TUF_RGB_MODE	0x00100056
>>  +
>>   /* DSTS masks */
>>   #define ASUS_WMI_DSTS_STATUS_BIT	0x00000001
>>   #define ASUS_WMI_DSTS_UNKNOWN_BIT	0x00000002
>