Hi Mustafa,
On 3/24/24 7:12 PM, mustafa wrote:
> From: Mustafa Ekşi <mustafa.eskieksi@xxxxxxxxx>
>
> 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@xxxxxxxxx>
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
