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