Received-SPF: pass (google.com: domain of linux-kernel+bounces-135193-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <0a5ea42f-cc57-492a-b0a9-18fe5045b5ee@redhat.com>
Date: Mon, 8 Apr 2024 12:01:12 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v5 1/1] platform/x86: Add wmi driver for Casper Excalibur
 laptops
To: mustafa <mustafa.eskieksi@gmail.com>, ilpo.jarvinen@linux.intel.com,
 jdelvare@suse.com, linux@roeck-us.net, pavel@ucw.cz, lee@kernel.org,
 linux-kernel@vger.kernel.org, platform-driver-x86@vger.kernel.org,
 linux-hwmon@vger.kernel.org, linux-leds@vger.kernel.org,
 Werner Sembach <wse@tuxedocomputers.com>
References: <20240324181201.87882-1-mustafa.eskieksi@gmail.com>
 <20240324181201.87882-2-mustafa.eskieksi@gmail.com>
Content-Language: en-US, nl
From: Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <20240324181201.87882-2-mustafa.eskieksi@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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