Re: [PATCH v2] platform/x86: Add wmi driver for Casper Excalibur laptops. .....
From: Guenter Roeck
Date: Thu Feb 22 2024 - 19:13:49 EST
- Next message: Pierre-Louis Bossart: "Re: [PATCH v9] ASoc: tas2783: Add tas2783 codec driver"
- Previous message: Rob Herring: "Re: [PATCH 1/3] dt-bindings: arm: fsl: add i.MX95 19x19 EVK board"
- In reply to: Mustafa Ekşi: "[PATCH v2] platform/x86: Add wmi driver for Casper Excalibur laptops. Odd line breaks was because I have used scripts/Lindent without checking, I'm sorry for that. And for my weird rgb led API: This kind of design was also used in drivers/platform/x86/dell/alienware-wmi.c:239, but mine differs as it doesn't create different attributes for different leds. That is because driver doesn't know how many leds there are, to know how many leds there are it should check processor information (whether it's 10th gen or 11th). I don't think include/linux/mod_devicetable.h supports that. If there is a way to differentiate cpus, please let me know. And even if it knew how many leds there are, having different attributes can be cumbersome because there's no way of reading leds. And also user can change led state without notifying os (with some hotkey). But I'm open to further discussion. And thanks for all of your careful reviewing. It helped me to learn more."
- Next in thread: Kuppuswamy Sathyanarayanan: "Re: [PATCH v2] platform/x86: Add wmi driver for Casper Excalibur laptops. Odd line breaks was because I have used scripts/Lindent without checking, I'm sorry for that. And for my weird rgb led API: This kind of design was also used in drivers/platform/x86/dell/alienware-wmi.c:239, but mine differs as it doesn't create different attributes for different leds. That is because driver doesn't know how many leds there are, to know how many leds there are it should check processor information (whether it's 10th gen or 11th). I don't think include/linux/mod_devicetable.h supports that. If there is a way to differentiate cpus, please let me know. And even if it knew how many leds there are, having different attributes can be cumbersome because there's no way of reading leds. And also user can change led state without notifying os (with some hotkey). But I'm open to further discussion. And thanks for all of your careful reviewing. It helped me to learn more."
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
On 2/22/24 13:48, Mustafa Ekşi wrote:
Adding wmi driver for Casper Excalibur Laptops:
This driver implements a ledclass_dev device for keyboard backlight
and hwmon driver to read fan speed and (also write) pwm mode. NEW_LEDS is
selected because this driver introduces new leds, and LEDS_CLASS is selected
because this driver implements a led class device. All of Casper Excalibur
Laptops are supported but fan speeds has a bug for older generations.
"Impressive" subject (I dropped most of it). Really, you should not do that.
Signed-off-by: Mustafa Ekşi <mustafa.eskieksi@xxxxxxxxx>
[ ... ]
+
+static int casper_wmi_hwmon_read(struct device *dev,
+ enum hwmon_sensor_types type, u32 attr,
+ int channel, long *val)
+{
+ struct casper_wmi_args out = { 0 };
+ struct wmi_device *wdev = to_wmi_device(dev->parent);
+ int ret;
+
+ switch (type) {
+ case hwmon_fan:
+ ret = casper_query(wdev, CASPER_GET_HARDWAREINFO, &out);
Ignoring errors is unacceptable.
+ /*
+ * a4 and a5 is little endian in older laptops (with 10th gen
+ * cpus or older) and big endian in newer ones. I don't think
+ * dmi has something for cpu information. Also, defining a
+ * dmi_list just for this seems like an overkill. This problem
+ * can be solved in userspace too.
+ */
+ if (channel == 0) // CPU fan
+ *val = out.a4;
+ else if (channel == 1) // GPU fan
+ *val = out.a5;
+ return 0;
+ case hwmon_pwm:
+ ret = casper_query(wdev, CASPER_POWERPLAN, &out);
+ if (ret) // power plan count varies generations.
That comment doesn't explain anything. If it is supposed to mean that it is
not supported on all laptop variants, the attribute should not be instantiated
in the first place. I am sure I mentioned this before.
+ return ret;
+ if (channel == 0)
+ *val = out.a2;
There is only one pwm channel. Checking the channel number is pointless.
+ return 0;
+ default:
+ return -ENODEV;
-EOPNOTSUPP
+ }
+}
+
+static int casper_wmi_hwmon_read_string(struct device *dev,
+ enum hwmon_sensor_types type, u32 attr,
+ int channel, const char **str)
+{
+ switch (type) {
+ case hwmon_fan:
+ switch (channel) {
+ case 0:
+ *str = "cpu_fan_speed";
+ break;
+ case 1:
+ *str = "gpu_fan_speed";
+ break;
+ default:
+ return -ENODEV;
-EOPNOTSUPP
+ }
+ break;
+ default:
+ return -ENODEV;
-EOPNOTSUPP
+ }
+ return 0;
+}
+
+static int casper_wmi_hwmon_write(struct device *dev,
+ enum hwmon_sensor_types type, u32 attr,
+ int channel, long val)
+{
+ acpi_status ret;
+
+ switch (type) {
+ case hwmon_pwm:
+ if (val > 5 || val < 0)
+ return -EINVAL;
pwm range, per ABI, is 0..255, not 0..5. It is expected to be normalized.
Since you are not using the standard ABI, what are the values returned
as fan speed ? Is that RPM or something else ?
+ ret = casper_set(to_wmi_device(dev->parent),
+ CASPER_POWERPLAN, val, 0);
+ if (ret)
+ return ret;
+ return 0;
return ret;
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static const struct hwmon_ops casper_wmi_hwmon_ops = {
+ .is_visible = &casper_wmi_hwmon_is_visible,
+ .read = &casper_wmi_hwmon_read,
+ .read_string = &casper_wmi_hwmon_read_string,
+ .write = &casper_wmi_hwmon_write
+};
+
+static const struct hwmon_channel_info *const casper_wmi_hwmon_info[] = {
+ HWMON_CHANNEL_INFO(fan,
+ HWMON_F_INPUT | HWMON_F_LABEL,
+ HWMON_F_INPUT | HWMON_F_LABEL),
+ HWMON_CHANNEL_INFO(pwm, HWMON_PWM_MODE),
+ NULL
+};
+
+static const struct hwmon_chip_info casper_wmi_hwmon_chip_info = {
+ .ops = &casper_wmi_hwmon_ops,
+ .info = casper_wmi_hwmon_info,
+};
+
+static int casper_wmi_probe(struct wmi_device *wdev, const void *context)
+{
+ struct device *hwmon_dev;
+
+ if (ACPI_FAILURE(led_classdev_register(&wdev->dev, &casper_kbd_led)))
I am quite sure that led_classdev_register() doesn't return ACPI error codes.
+ return -ENODEV;
+ hwmon_dev = devm_hwmon_device_register_with_info(&wdev->dev,
+ "casper_wmi", wdev,
+ &casper_wmi_hwmon_chip_info,
+ NULL);
+ return PTR_ERR_OR_ZERO(hwmon_dev);
Why use devm_hwmon_device_register_with_info() but not
devm_led_classdev_register() ?
+}
+
+static void casper_wmi_remove(struct wmi_device *wdev)
+{
+ led_classdev_unregister(&casper_kbd_led);
+}
+
+static const struct wmi_device_id casper_wmi_id_table[] = {
+ { CASPER_WMI_GUID, NULL },
+ { }
+};
+
+static struct wmi_driver casper_wmi_driver = {
+ .driver = {
+ .name = "casper-wmi",
+ },
+ .id_table = casper_wmi_id_table,
+ .probe = casper_wmi_probe,
+ .remove = &casper_wmi_remove,
+};
+
+module_wmi_driver(casper_wmi_driver);
+MODULE_DEVICE_TABLE(wmi, casper_wmi_id_table);
+
+MODULE_AUTHOR("Mustafa Ekşi <mustafa.eskieksi@xxxxxxxxx>");
+MODULE_DESCRIPTION("Casper Excalibur Laptop WMI driver");
+MODULE_LICENSE("GPL");
- Next message: Pierre-Louis Bossart: "Re: [PATCH v9] ASoc: tas2783: Add tas2783 codec driver"
- Previous message: Rob Herring: "Re: [PATCH 1/3] dt-bindings: arm: fsl: add i.MX95 19x19 EVK board"
- In reply to: Mustafa Ekşi: "[PATCH v2] platform/x86: Add wmi driver for Casper Excalibur laptops. Odd line breaks was because I have used scripts/Lindent without checking, I'm sorry for that. And for my weird rgb led API: This kind of design was also used in drivers/platform/x86/dell/alienware-wmi.c:239, but mine differs as it doesn't create different attributes for different leds. That is because driver doesn't know how many leds there are, to know how many leds there are it should check processor information (whether it's 10th gen or 11th). I don't think include/linux/mod_devicetable.h supports that. If there is a way to differentiate cpus, please let me know. And even if it knew how many leds there are, having different attributes can be cumbersome because there's no way of reading leds. And also user can change led state without notifying os (with some hotkey). But I'm open to further discussion. And thanks for all of your careful reviewing. It helped me to learn more."
- Next in thread: Kuppuswamy Sathyanarayanan: "Re: [PATCH v2] platform/x86: Add wmi driver for Casper Excalibur laptops. Odd line breaks was because I have used scripts/Lindent without checking, I'm sorry for that. And for my weird rgb led API: This kind of design was also used in drivers/platform/x86/dell/alienware-wmi.c:239, but mine differs as it doesn't create different attributes for different leds. That is because driver doesn't know how many leds there are, to know how many leds there are it should check processor information (whether it's 10th gen or 11th). I don't think include/linux/mod_devicetable.h supports that. If there is a way to differentiate cpus, please let me know. And even if it knew how many leds there are, having different attributes can be cumbersome because there's no way of reading leds. And also user can change led state without notifying os (with some hotkey). But I'm open to further discussion. And thanks for all of your careful reviewing. It helped me to learn more."
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]