Messages in this thread | | | Date | Mon, 8 Apr 2024 12:01:12 +0200 | Subject | Re: [PATCH v5 1/1] platform/x86: Add wmi driver for Casper Excalibur laptops | From | Hans de Goede <> |
| |
Hi Mustafa,
On 3/24/24 7:12 PM, mustafa wrote: > From: Mustafa Ekşi <mustafa.eskieksi@gmail.com> > > This wmi driver supports Casper Excalibur laptops' changing keyboard > backlight, reading fan speeds and changing power profiles. Multicolor > led device is used for backlight, platform_profile for power management > and hwmon for fan speeds. It supports both old (10th gen or older) and > new (11th gen or newer) laptops. It uses x86_match_cpu to check if the > laptop is old or new. > This driver's Multicolor keyboard backlight API is very similar to Rishit > Bansal's proposed API. > > Signed-off-by: Mustafa Ekşi <mustafa.eskieksi@gmail.com>
Thank you for your patch.
Overall this looks pretty good I have one important remark about the LED names and some small remarks inline.
After those are addressed I believe this is ready for merging.
<snip>
> +#define CASPER_LED_COUNT 4 > + > +static const char * const zone_names[CASPER_LED_COUNT] = { > + "casper::kbd_zoned_backlight-right", > + "casper::kbd_zoned_backlight-middle", > + "casper::kbd_zoned_backlight-left", > + "casper::kbd_zoned_backlight-corners", > +};
So these names should be:
static const char * const zone_names[CASPER_LED_COUNT] = { "casper:rgb:kbd_zoned_backlight-right", "casper:rgb:kbd_zoned_backlight-middle", "casper:rgb:kbd_zoned_backlight-left", "casper:rgb:kbd_zoned_backlight-corners", };
with that change I think this is fine, but we really need to get an ack for this from the LED subsys maintainers.
Please add a second patch to this patch-serieas adding some documentation about the use of these names for zoned RGB backlit kbds in a new paragraph / subsection of the "LED Device Naming" section of:
Documentation/leds/leds-class.rst
It seems there are 2 possible sets which we should probably document in one go:
The set of 4 zones from this patch:
:rgb:kbd_zoned_backlight-right :rgb:kbd_zoned_backlight-middle :rgb:kbd_zoned_backlight-left :rgb:kbd_zoned_backlight-corners
As well as:
:rgb:kbd_zoned_backlight-main :rgb:kbd_zoned_backlight-wasd :rgb:kbd_zoned_backlight-cursor :rgb:kbd_zoned_backlight-numpad
Maybe with a comment that in the future different zone names are possible if keyboards with a different zoning scheme show up.
> +static int casper_set(struct casper_drv *drv, u16 a1, u8 led_id, u32 data) > +{ > + acpi_status ret = 0; > + struct casper_wmi_args wmi_args; > + struct acpi_buffer input;
Please use a separate acpi_status and ret variables here with the correct types:
struct casper_wmi_args wmi_args; struct acpi_buffer input; acpi_status status; int ret = 0;
> + > + wmi_args = (struct casper_wmi_args) { > + .a0 = CASPER_WRITE, > + .a1 = a1, > + .a2 = led_id, > + .a3 = data > + }; > + > + input = (struct acpi_buffer) { > + (acpi_size) sizeof(struct casper_wmi_args), > + &wmi_args > + }; > + > + mutex_lock(&drv->casper_mutex); > +
And then here:
status = wmidev_block_set(drv->wdev, 0, &input); if (ACPI_FAILURE(status)) ret = -EIO;
> + > + mutex_unlock(&drv->casper_mutex); > + return ret; > +} > + > +static int casper_query(struct casper_drv *drv, u16 a1, > + struct casper_wmi_args *out) > +{
Same here, also please sort variable declarations in reverse christmas tree order, so longest lines first:
struct casper_wmi_args wmi_args; struct acpi_buffer input; union acpi_object *obj; acpi_status status; int ret = 0;
> + wmi_args = (struct casper_wmi_args) { > + .a0 = CASPER_READ, > + .a1 = a1 > + }; > + input = (struct acpi_buffer) { > + (acpi_size) sizeof(struct casper_wmi_args), > + &wmi_args > + }; > + > + mutex_lock(&drv->casper_mutex); > +
And use status here to store the acpi_status type returned by wmidev_block_set().
> + ret = wmidev_block_set(drv->wdev, 0, &input); > + if (ACPI_FAILURE(ret)) { > + ret = -EIO; > + goto unlock; > + } > + > + obj = wmidev_block_query(drv->wdev, 0); > + if (!obj) { > + ret = -EIO; > + goto unlock; > + } > + > + if (obj->type != ACPI_TYPE_BUFFER) { // obj will be 0x10 on failure > + ret = -EINVAL; > + goto freeobj; > + } > + if (obj->buffer.length != sizeof(struct casper_wmi_args)) { > + ret = -EIO; > + goto freeobj; > + } > + > + memcpy(out, obj->buffer.pointer, sizeof(struct casper_wmi_args)); > + > +freeobj: > + kfree(obj); > +unlock: > + mutex_unlock(&drv->casper_mutex); > + return ret; > +}
<snip>
The MODULE_DEVICE_TABLE() line should be directly below the declaration of the table like this:
static const struct wmi_device_id casper_wmi_id_table[] = { { CASPER_WMI_GUID, NULL }, { } }; MODULE_DEVICE_TABLE(wmi, casper_wmi_id_table);
Regards,
Hans
| |