[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <dc966b56-9139-4e8b-b667-051c167f6bee@gmx.de>
Date: Thu, 22 Feb 2024 11:26:45 +0100
From: Armin Wolf <W_Armin@....de>
To: Guenter Roeck <linux@...ck-us.net>,
Mustafa Ekşi <mustafa.eskieksi@...il.com>,
hdegoede@...hat.com, ilpo.jarvinen@...ux.intel.com,
linux-kernel@...r.kernel.org, platform-driver-x86@...r.kernel.org,
linux-hwmon@...r.kernel.org
Cc: jdelvare@...e.com
Subject: Re: [PATCH] Add wmi driver support for Casper Excalibur laptops.
Am 21.02.24 um 23:55 schrieb Guenter Roeck:
> On 2/21/24 14:15, Mustafa Ekşi wrote:
>> Signed-off-by: Mustafa Ekşi <mustafa.eskieksi@...il.com>
>> ---
>> MAINTAINERS | 6 +
>> drivers/platform/x86/Kconfig | 14 ++
>> drivers/platform/x86/Makefile | 1 +
>> drivers/platform/x86/casper-wmi.c | 344 ++++++++++++++++++++++++++++++
>> 4 files changed, 365 insertions(+)
>> create mode 100644 drivers/platform/x86/casper-wmi.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 9ed4d386853..d0142a75d2c 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -4723,6 +4723,12 @@ S: Maintained
>> W: https://wireless.wiki.kernel.org/en/users/Drivers/carl9170
>> F: drivers/net/wireless/ath/carl9170/
>> +CASPER EXCALIBUR WMI DRIVER
>> +M: Mustafa Ekşi <mustafa.eskieksi@...il.com>
>> +L: platform-driver-x86@...r.kernel.org
>> +S: Maintained
>> +F: drivers/platform/x86/casper-wmi.c
>> +
>> CAVIUM I2C DRIVER
>> M: Robert Richter <rric@...nel.org>
>> S: Odd Fixes
>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>> index bdd302274b9..ebef9c9dfb6 100644
>> --- a/drivers/platform/x86/Kconfig
>> +++ b/drivers/platform/x86/Kconfig
>> @@ -1127,6 +1127,20 @@ config SEL3350_PLATFORM
>> To compile this driver as a module, choose M here: the module
>> will be called sel3350-platform.
>> +config CASPER_WMI
>> + tristate "Casper Excalibur Laptop WMI driver"
>> + depends on ACPI_WMI
>> + depends on HWMON
>> + select NEW_LEDS
>> + select LEDS_CLASS
>> + help
>> + Say Y here if you want to support WMI-based fan speed reporting,
>> + power management and keyboard backlight support on Casper
>> Excalibur
>> + Laptops.
>> +
>> + To compile this driver as a module, choose M here: the module
>> will
>> + be called casper-wmi.
>> +
>> endif # X86_PLATFORM_DEVICES
>> config P2SB
>> diff --git a/drivers/platform/x86/Makefile
>> b/drivers/platform/x86/Makefile
>> index 1de432e8861..4b527dd44ad 100644
>> --- a/drivers/platform/x86/Makefile
>> +++ b/drivers/platform/x86/Makefile
>> @@ -14,6 +14,7 @@ obj-$(CONFIG_MXM_WMI) += mxm-wmi.o
>> obj-$(CONFIG_NVIDIA_WMI_EC_BACKLIGHT) += nvidia-wmi-ec-backlight.o
>> obj-$(CONFIG_XIAOMI_WMI) += xiaomi-wmi.o
>> obj-$(CONFIG_GIGABYTE_WMI) += gigabyte-wmi.o
>> +obj-$(CONFIG_CASPER_WMI) += casper-wmi.o
>> # Acer
>> obj-$(CONFIG_ACERHDF) += acerhdf.o
>> diff --git a/drivers/platform/x86/casper-wmi.c
>> b/drivers/platform/x86/casper-wmi.c
>> new file mode 100644
>> index 00000000000..aae08202b19
>> --- /dev/null
>> +++ b/drivers/platform/x86/casper-wmi.c
>> @@ -0,0 +1,344 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/leds.h>
>> +#include <linux/slab.h>
>> +#include <linux/module.h>
>> +#include <linux/wmi.h>
>> +#include <linux/device.h>
>> +#include <linux/dev_printk.h>
>> +#include <linux/hwmon.h>
>> +#include <linux/sysfs.h>
>> +#include <linux/types.h>
>> +#include <linux/dmi.h>
>> +#include <acpi/acexcep.h>
>> +
>> +MODULE_AUTHOR("Mustafa Ekşi <mustafa.eskieksi@...il.com>");
>> +MODULE_DESCRIPTION("Casper Excalibur Laptop WMI driver");
>> +MODULE_LICENSE("GPL");
>> +
>> +#define CASPER_WMI_GUID "644C5791-B7B0-4123-A90B-E93876E0DAAD"
>> +
>> +#define CASPER_READ 0xfa00
>> +#define CASPER_WRITE 0xfb00
>> +#define CASPER_GET_HARDWAREINFO 0x0200
>> +#define CASPER_SET_LED 0x0100
>> +#define CASPER_POWERPLAN 0x0300
>> +
>> +#define CASPER_KEYBOARD_LED_1 0x03
>> +#define CASPER_KEYBOARD_LED_2 0x04
>> +#define CASPER_KEYBOARD_LED_3 0x05
>> +#define CASPER_ALL_KEYBOARD_LEDS 0x06
>> +#define CASPER_CORNER_LEDS 0x07
>> +
>> +struct casper_wmi_args {
>> + u16 a0, a1;
>> + u32 a2, a3, a4, a5, a6, a7, a8;
>> +};
>> +
>> +static u32 casper_last_color;
>> +static u8 casper_last_led;
>> +
>> +static acpi_status casper_set(struct wmi_device *wdev, u16 a1, u8
>> led_id,
>> + u32 data)
>> +{
>> + struct casper_wmi_args wmi_args = {
>> + .a0 = CASPER_WRITE,
>> + .a1 = a1,
>> + .a2 = led_id,
>> + .a3 = data
>> + };
>> + struct acpi_buffer input = {
>> + (acpi_size) sizeof(struct casper_wmi_args),
>> + &wmi_args
>> + };
>> + return wmidev_block_set(wdev, 0, &input);
>> +}
>> +
>> +static ssize_t led_control_show(struct device *dev, struct
>> device_attribute
>> + *attr, char *buf)
>> +{
>> + return sprintf("%u%08x\n", buf, casper_last_led,
>> + casper_last_color);
>> +}
>> +
>> +
>> +// input is formatted as "IMARRGGBB", I: led_id, M: mode, A:
>> brightness, ...
>> +static ssize_t led_control_store(struct device *dev, struct
>> device_attribute
>> + *attr, const char *buf, size_t count)
>> +{
>> + u64 tmp;
>> + int ret;
>> +
>> + ret = kstrtou64(buf, 16, &tmp);
>
> What exatly is the point of u64 and kstrtou64() ?
>
>> +
>> + if (ret)
>> + return ret;
>> +
>> + u8 led_id = (tmp >> (8 * 4))&0xFF;
>
> This will result in interesting LED IDs based on u64 input. To me it
> looks
> very much like a poor random number generator. Does this follow wome kind
> of LED subsystem API ?
>
>> +
>> + ret =
>> + casper_set(to_wmi_device(dev->parent), CASPER_SET_LED, led_id,
>> + (u32) tmp
>> + );
>
> Odd line breaks. Does this pass checkpatch ?
>
>> + if (ACPI_FAILURE(ret)) {
>> + dev_err(dev, "casper-wmi ACPI status: %d\n", ret);
>> + return ret;
>
> The function return code is supposed to be a Linux error code.
> As for the functions below, I would reject this patch due to its
> logging noise.
>
>> + }
>> + if (led_id != CASPER_CORNER_LEDS) {
>> + casper_last_color = (u32) tmp;
>> + casper_last_led = led_id;
>> + }
>> + return count;
>> +}
>> +
>> +static DEVICE_ATTR_RW(led_control);
>> +
>> +static struct attribute *casper_kbd_led_attrs[] = {
>> + &dev_attr_led_control.attr,
>> + NULL,
>> +};
>> +
>> +ATTRIBUTE_GROUPS(casper_kbd_led);
>> +
>> +static void set_casper_backlight_brightness(struct led_classdev
>> *led_cdev,
>> + enum led_brightness brightness)
>> +{
>> + // Setting any of the keyboard leds' brightness sets brightness
>> of all
>> + acpi_status ret =
>> + casper_set(to_wmi_device(led_cdev->dev->parent),
>> CASPER_SET_LED,
>> + CASPER_KEYBOARD_LED_1,
>> + (casper_last_color & 0xF0FFFFFF) |
>> + (((u32) brightness) << 24)
>> + );
>> +
>> + if (ret != 0)
>> + dev_err(led_cdev->dev,
>> + "Couldn't set brightness acpi status: %d\n", ret);
>> +}
>> +
>> +static enum led_brightness get_casper_backlight_brightness(struct
>> led_classdev
>> + *led_cdev)
>> +{
>> + return (casper_last_color&0x0F000000)>>24;
>> +}
>> +
>> +static struct led_classdev casper_kbd_led = {
>> + .name = "casper::kbd_backlight",
>> + .brightness = 0,
>> + .brightness_set = set_casper_backlight_brightness,
>> + .brightness_get = get_casper_backlight_brightness,
>> + .max_brightness = 2,
>> + .groups = casper_kbd_led_groups,
>> +};
>> +
>> +static acpi_status casper_query(struct wmi_device *wdev, u16 a1,
>> + struct casper_wmi_args *out)
>> +{
>> + struct casper_wmi_args wmi_args = {
>> + .a0 = CASPER_READ,
>> + .a1 = a1
>> + };
>> + struct acpi_buffer input = {
>> + (acpi_size) sizeof(struct casper_wmi_args),
>> + &wmi_args
>> + };
>> +
>> + acpi_status ret = wmidev_block_set(wdev, 0, &input);
>> +
>> + if (ACPI_FAILURE(ret)) {
>> + dev_err(&wdev->dev,
>> + "Could not query acpi status: %u", ret);
>
> This code generates _way_ too much logging noise for my liking.
>
>> + return ret;
>
> Is there any value in having this function return acpi error
> codes instead of Linux error codes ?
>
>> + }
>> +
>> + union acpi_object *obj = wmidev_block_query(wdev, 0);
>> +
>> + if (obj == NULL) {
>> + dev_err(&wdev->dev,
>> + "Could not query hardware information");
>> + return AE_ERROR;
>> + }
>> + if (obj->type != ACPI_TYPE_BUFFER) {
>> + dev_err(&wdev->dev, "Return type is not a buffer");
>> + return AE_TYPE;
>> + }
>> +
>> + if (obj->buffer.length != 32) {
>> + dev_err(&wdev->dev, "Return buffer is not long enough");
>> + return AE_ERROR;
>> + }
>> + memcpy(out, obj->buffer.pointer, 32);
>> + kfree(obj);
>> + return ret;
>> +}
>> +
>> +static umode_t casper_wmi_hwmon_is_visible(const void *drvdata,
>> + enum hwmon_sensor_types type,
>> + u32 attr, int channel)
>> +{
>> + switch (type) {
>> + case hwmon_fan:
>> + return 0444;
>> + case hwmon_pwm:
>> + return 0644;
>> + default:
>> + return 0;
>> + }
>> + return 0;
>> +}
>> +
>> +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 };
>> +
>> + switch (type) {
>> + case hwmon_fan:
>> + acpi_status ret = casper_query(to_wmi_device(dev->parent),
>> + CASPER_GET_HARDWAREINFO, &out);
>> +
>> + if (ACPI_FAILURE(ret))
>> + return ret;
>
> This function is expected to return a Linux error code, not an acpi
> error code.
>
> Also, if CASPER_GET_HARDWAREINFO is not always available, the attributes
> needing it should not be created in the first place.
>
>> +
>> + if (channel == 0) { // CPU fan
>> + u32 cpu_fanspeed = out.a4;
>> +
>> + cpu_fanspeed <<= 8;
>> + cpu_fanspeed += out.a4 >> 8;
>> + *val = (long) cpu_fanspeed;
>> + } else if (channel == 1) { // GPU fan
>> + u32 gpu_fanspeed = out.a5;
>> +
>> + gpu_fanspeed <<= 8;
>> + gpu_fanspeed += out.a5 >> 8;
>
> I don't know what this is supposed to be doing, but it will return
> odd values. For example, if out.a5 is 0xabcd, the returned value
> will be 0xabcdab. That seems to be unlikely I suspect this is supposed
> to be
> *val = ((out.a5 & 0xff) << 8) | ((out.a5 >> 8) & 0xff);
> but I am not even sure about that because a5 is u32 and the above
> would suggest
> a 16-bit unsigned short in big endian format. Please check return values
> and implement any necessary endiannes conversion correctly.
>
>> + *val = (long) gpu_fanspeed;
>
> FWIW, those type casts are unnecessary.
>
>> + }
>> + return 0;
>> + case hwmon_pwm:
>> + casper_query(to_wmi_device(dev->parent), CASPER_POWERPLAN,
>> + &out);
>
> Why no error check here ?
>
>> + if (channel == 0)
>> + *val = (long)out.a2;
>> + else
>> + return -EOPNOTSUPP;
>
> The coonditional and else case is unnecessary since only
> a single pwm channel is declared.
>
>> + return 0;
>> + default:
>> + return -EOPNOTSUPP;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +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 -EOPNOTSUPP;
>> + }
>> + break;
>> + default:
>> + return -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 (channel != 0)
>> + return -EOPNOTSUPP;
>
> This is unnecessary. Only a single pwm channel is declared,
> so channel will never be != 0.
>
>> + ret =
>> + casper_set(to_wmi_device(dev->parent), CASPER_POWERPLAN,
>> + val, 0);
>> +
> The first line split is unnecessary.
>
>> + if (ACPI_FAILURE(ret)) {
>> + dev_err(dev, "Couldn't set power plan, acpi_status: %d",
>> + ret);
>
> Drivers should not generate such logging noise, even more so after
> user input.
> Also, the valid range (0..255) should be checked before trying to set it.
>
>> + return -EINVAL;
>> + }
>> + return 0;
>> + 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;
>> +
>> + // All Casper Excalibur Laptops use this GUID
>> + if (!wmi_has_guid(CASPER_WMI_GUID))
>> + return -ENODEV;
>> +
> How would the device ever be instantiated with a different GUID,
> making this check necessary ?
>
Hi,
this is indeed unnecessary, the WMI driver core already takes care that the WMI driver is
only bound to WMI devices with a matching GUID.
I think this was copied from the dell-wmi-privacy driver, i will send a patch to remove this.
Armin Wolf
>> + hwmon_dev =
>> + devm_hwmon_device_register_with_info(&wdev->dev,
>> "casper_wmi", wdev,
>> + &casper_wmi_hwmon_chip_info,
>> + NULL);
>> +
>> + acpi_status result = led_classdev_register(&wdev->dev,
>> &casper_kbd_led);
>> +
>> + if (result != 0)
>> + return -ENODEV;
>> +
>> + return PTR_ERR_OR_ZERO(hwmon_dev);
>
> This would leave the LED device registered if instantiating the hwmon
> device
> failed. However, the probe function would return an error, meaning the
> driver
> core will believe that instantiation failed. Is that intentional ? I
> am quite
> sure that this would result in interesting crashes.
>
>> + }
>> +
>> +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);
>
>
Powered by blists - more mailing lists