Re: [PATCH v2 1/1] asus-wmi: Add support for TUF laptop keyboard states

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Hans,

Thanks for the valuable review,

On Thu, Aug 4 2022 at 15:22:00 +0200, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
Hi Luke,

On 8/4/22 01:13, Luke D. Jones wrote:
 Adds support for the TUF series laptop power states. This means
 control of if the LED's are on while awake, or an animation is
 shown while booting or suspended.

 /sys/devices/platform/asus-nb-wmi/tuf_krgb_state_index provides
 labels for the index fields as "save boot awake sleep keyboard"

 /sys/devices/platform/asus-nb-wmi/tuf_krgb_state has the following
 as input options via boolean "b b b b b":
 - Save or set, if set, then settings revert on cold boot
 - Boot, if true, the keyboard displays animation on boot
 - Awake, if true, the keyboard LED's are on while device is awake
- Sleep, if true, the keyboard shows animation while device is suspended
 - Keybaord, appears to have no effect

Keybaord typo / spelling issue.

Please make this an extra attribute for the led_class_device,
you can do this by adding this attribute to a separate
attribute_group, lets say e.g. "tuf_rgb_attributes" and then
in the "[PATCH] asus-wmi: Add support for TUF laptop keyboard RGB"
code add:

	mc_cdev->led_cdev.groups = tuf_rgb_attributes;

and then the "tuf_krgb_state" file should show up as:
/sys/class/leds/asus::multicolour/tuf_krgb_state

Oh this is very cool! I was honestly wondering about if that could be
done but was unsure how and finding the right docs isn't always easy.


Also I'm not sure what to think of the tuf_krgb_state_index file,
having a sysfs file just to show some help text feels weird / wrong.

I modelled that after the multicolor version in led-class-multicolor.c:

static ssize_t multi_index_show(struct device *dev,
			      struct device_attribute *multi_index_attr,
			      char *buf)
{
	struct led_classdev *led_cdev = dev_get_drvdata(dev);
	struct led_classdev_mc *mcled_cdev = lcdev_to_mccdev(led_cdev);
	int len = 0;
	int index;
	int i;

	for (i = 0; i < mcled_cdev->num_colors; i++) {
		index = mcled_cdev->subled_info[i].color_index;
		len += sprintf(buf + len, "%s", led_colors[index]);
		if (i < mcled_cdev->num_colors - 1)
			len += sprintf(buf + len, " ");
	}

	buf[len++] = '\n';
	return len;
}
static DEVICE_ATTR_RO(multi_index);

Since it was done there it seemed okay?



Please instead document the expected format in the existing:
Documentation/ABI/testing/sysfs-platform-asus-wmi

file; and talking about that file, it seems that this file
could use some love to document other recently addes asus-wmi
features too.


Oof, yep I'll hop on that.

Related to the tuf_krgb_state_index file, please use the
new sysfs_emit helper for all new show functions. And bonus
points for a patch (series?) converting old show functions
over to it.

For sure I'll do both. I guess this will turn in to a series of
patches now. If only I could get paid for the work..


Many thanks,
Luke.


Regards,

Hans





 Signed-off-by: Luke D. Jones <luke@xxxxxxxxxx>
 ---
drivers/platform/x86/asus-wmi.c | 95 ++++++++++++++++++++++
  include/linux/platform_data/x86/asus-wmi.h |  3 +
  2 files changed, 98 insertions(+)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
 index 0e7fbed8a50d..bbfb054ff3b2 100644
 --- a/drivers/platform/x86/asus-wmi.c
 +++ b/drivers/platform/x86/asus-wmi.c
 @@ -234,6 +234,8 @@ struct asus_wmi {
  	bool dgpu_disable_available;
  	bool dgpu_disable;

 +	bool tuf_krgb_state_available;
 +
  	bool throttle_thermal_policy_available;
  	u8 throttle_thermal_policy_mode;

@@ -734,6 +736,86 @@ static ssize_t egpu_enable_store(struct device *dev,

  static DEVICE_ATTR_RW(egpu_enable);

+/* TUF Laptop Keyboard RGB States *********************************************/
 +static int tuf_krgb_state_check_present(struct asus_wmi *asus)
 +{
 +	u32 result;
 +	int err;
 +
 +	asus->tuf_krgb_state_available = false;
 +
+ err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_TUF_RGB_STATE, &result);
 +	if (err) {
 +		if (err == -ENODEV)
 +			return 0;
 +		return err;
 +	}
 +
 +	if (result & ASUS_WMI_DSTS_PRESENCE_BIT)
 +		asus->tuf_krgb_state_available = true;
 +
 +	return 0;
 +}
 +
 +static ssize_t tuf_krgb_state_store(struct device *dev,
 +				   struct device_attribute *attr,
 +				   const char *buf, size_t count)
 +{
 +	int err;
 +	u32 ret;
 +	bool tmp;
 +	char *data, *part, *end;
 +	u8 save, flags, res, arg_num;
 +
 +	save = flags = arg_num = 0;
 +	data = end = kstrdup(buf, GFP_KERNEL);
 +
 +	while ((part = strsep(&end, " ")) != NULL) {
 +		if (part == NULL)
 +			return -1;
 +
 +		res = kstrtobool(part, &tmp);
 +		if (res)
 +			return -1;
 +
 +		if (tmp) {
 +			if (arg_num == 0) // save  :  set
 +				save = tmp == 0 ? 0x0100 : 0x0000;
 +			else if (arg_num == 1)
 +				flags |= 0x02; // boot
 +			else if (arg_num == 2)
 +				flags |= 0x08; // awake
 +			else if (arg_num == 3)
 +				flags |= 0x20; // sleep
 +			else if (arg_num == 4)
 +				flags |= 0x80; // keyboard
 +		}
 +
 +		arg_num += 1;
 +	}
 +
 +	err = asus_wmi_evaluate_method3(ASUS_WMI_METHODID_DEVS,
+ ASUS_WMI_DEVID_TUF_RGB_STATE, 0xBD | save | (flags << 16), 0, &ret);
 +	if (err) {
 +			return err;
 +	}
 +
 +	kfree(data);
 +	return count;
 +}
 +
 +static DEVICE_ATTR_WO(tuf_krgb_state);
 +
 +static ssize_t tuf_krgb_state_index_show(struct device *device,
 +						 struct device_attribute *attr,
 +						 char *buf)
 +{
 +	int len = sprintf(buf, "%s", "save boot awake sleep keyboard\n");
 +	return len;
 +}
 +
 +static DEVICE_ATTR_RO(tuf_krgb_state_index);
 +
/* Battery ********************************************************************/

  /* The battery maximum charging percentage */
@@ -3258,6 +3340,8 @@ static struct attribute *platform_attributes[] = {
  	&dev_attr_touchpad.attr,
  	&dev_attr_egpu_enable.attr,
  	&dev_attr_dgpu_disable.attr,
 +	&dev_attr_tuf_krgb_state.attr,
 +	&dev_attr_tuf_krgb_state_index.attr,
  	&dev_attr_lid_resume.attr,
  	&dev_attr_als_enable.attr,
  	&dev_attr_fan_boost_mode.attr,
@@ -3286,6 +3370,12 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
  		devid = ASUS_WMI_DEVID_ALS_ENABLE;
  	else if (attr == &dev_attr_egpu_enable.attr)
  		ok = asus->egpu_enable_available;
 +	else if (attr == &dev_attr_tuf_krgb_state.attr)
 +		ok = asus->tuf_krgb_state_available;
 +	else if (attr == &dev_attr_tuf_krgb_state_index.attr)
 +		ok = asus->tuf_krgb_state_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_fan_boost_mode.attr)
@@ -3557,6 +3647,10 @@ static int asus_wmi_add(struct platform_device *pdev)
  	if (err)
  		goto fail_dgpu_disable;

 +	err = tuf_krgb_state_check_present(asus);
 +	if (err)
 +		goto fail_tuf_krgb_state;
 +
  	err = fan_boost_mode_check_present(asus);
  	if (err)
  		goto fail_fan_boost_mode;
@@ -3671,6 +3765,7 @@ static int asus_wmi_add(struct platform_device *pdev)
  fail_fan_boost_mode:
  fail_egpu_enable:
  fail_dgpu_disable:
 +fail_tuf_krgb_state:
  fail_platform:
  fail_panel_od:
  	kfree(asus);
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
 index a571b47ff362..a95b37385e66 100644
 --- a/include/linux/platform_data/x86/asus-wmi.h
 +++ b/include/linux/platform_data/x86/asus-wmi.h
 @@ -98,6 +98,9 @@
  /* dgpu on/off */
  #define ASUS_WMI_DEVID_DGPU		0x00090020

 +/* TUF laptop RGB power/state */
 +#define ASUS_WMI_DEVID_TUF_RGB_STATE	0x00100057
 +
  /* DSTS masks */
  #define ASUS_WMI_DSTS_STATUS_BIT	0x00000001
  #define ASUS_WMI_DSTS_UNKNOWN_BIT	0x00000002






[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux