Message-ID: <7d083fb7-74e9-37b6-0fa4-4f97d55ef619@linux.intel.com>
Date: Tue, 15 Apr 2025 20:32:01 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Werner Sembach <wse@...edocomputers.com>
cc: Hans de Goede <hdegoede@...hat.com>, bentiss@...nel.org, 
    linux-input@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>, 
    platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH v6 1/1] platform/x86/tuxedo: Add virtual LampArray for
 TUXEDO NB04 devices

On Tue, 15 Apr 2025, Werner Sembach wrote:
> Am 11.04.25 um 15:48 schrieb Ilpo Järvinen:
> > On Thu, 3 Apr 2025, Werner Sembach wrote:
> > 
> > > The TUXEDO Sirius 16 Gen1 and TUXEDO Sirius 16 Gen2 devices have a per-key
> > > controllable RGB keyboard backlight. The firmware API for it is
> > > implemented
> > > via WMI.
> > > 
> > > To make the backlight userspace configurable this driver emulates a
> > > LampArray HID device and translates the input from hidraw to the
> > > corresponding WMI calls. This is a new approach as the leds subsystem
> > > lacks
> > > a suitable UAPI for per-key keyboard backlights, and like this no new UAPI
> > > needs to be established.
> > > 
> > > Signed-off-by: Werner Sembach <wse@...edocomputers.com>
> > > ---
> > >   MAINTAINERS                                 |   6 +
> > >   drivers/platform/x86/Kconfig                |   2 +
> > >   drivers/platform/x86/Makefile               |   3 +
> > >   drivers/platform/x86/tuxedo/Kconfig         |   8 +
> > >   drivers/platform/x86/tuxedo/Makefile        |   8 +
> > >   drivers/platform/x86/tuxedo/nb04/Kconfig    |  15 +
> > >   drivers/platform/x86/tuxedo/nb04/Makefile   |  10 +
> > >   drivers/platform/x86/tuxedo/nb04/wmi_ab.c   | 847 ++++++++++++++++++++
> > >   drivers/platform/x86/tuxedo/nb04/wmi_util.c |  95 +++
> > >   drivers/platform/x86/tuxedo/nb04/wmi_util.h | 109 +++
> > >   10 files changed, 1103 insertions(+)
> > >   create mode 100644 drivers/platform/x86/tuxedo/Kconfig
> > >   create mode 100644 drivers/platform/x86/tuxedo/Makefile
> > >   create mode 100644 drivers/platform/x86/tuxedo/nb04/Kconfig
> > >   create mode 100644 drivers/platform/x86/tuxedo/nb04/Makefile
> > >   create mode 100644 drivers/platform/x86/tuxedo/nb04/wmi_ab.c
> > >   create mode 100644 drivers/platform/x86/tuxedo/nb04/wmi_util.c
> > >   create mode 100644 drivers/platform/x86/tuxedo/nb04/wmi_util.h
> > > 
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 00e94bec401e1..c1f7460c246ad 100644


> > > +struct tux_driver_data_t {
> > > +	struct hid_device *hdev;
> > > +};
> > > +
> > > +struct tux_hdev_driver_data_t {
> > > +	u8 keyboard_type;
> > > +	u8 lamp_count;
> > > +	u8 next_lamp_id;
> > > +	union tux_wmi_xx_496in_80out_in_t next_kbl_set_multiple_keys_in;
> > > +};
> > > +
> > > +static int tux_ll_start(struct hid_device *hdev)
> > > +{
> > > +	struct wmi_device *wdev = to_wmi_device(hdev->dev.parent);
> > > +	struct tux_hdev_driver_data_t *driver_data;
> > > +	union tux_wmi_xx_8in_80out_out_t out;
> > > +	union tux_wmi_xx_8in_80out_in_t in;
> > > +	int ret;
> > > +
> > > +	driver_data = devm_kzalloc(&hdev->dev, sizeof(*driver_data),
> > > GFP_KERNEL);
> > > +	if (!driver_data)
> > > +		return -ENOMEM;
> > > +
> > > +	in.get_device_status_in.device_type =
> > > WMI_AB_GET_DEVICE_STATUS_DEVICE_ID_KEYBOARD;
> > > +	ret = tux_wmi_xx_8in_80out(wdev, WMI_AB_GET_DEVICE_STATUS, &in, &out);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	driver_data->keyboard_type =
> > > out.get_device_status_out.keyboard_physical_layout;
> > > +	if (driver_data->keyboard_type ==
> > > WMI_AB_GET_DEVICE_STATUS_KEYBOARD_LAYOUT_ANSII)
> > > +		driver_data->lamp_count = sizeof(sirius_16_ansii_kbl_mapping);
> > > +	else if (driver_data->keyboard_type ==
> > > WMI_AB_GET_DEVICE_STATUS_KEYBOARD_LAYOUT_ISO)
> > > +		driver_data->lamp_count = sizeof(sirius_16_iso_kbl_mapping);
> > > +	else
> > > +		return -EINVAL;
> > > +	driver_data->next_lamp_id = 0;
> > > +
> > > +	dev_set_drvdata(&hdev->dev, driver_data);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static void tux_ll_stop(struct hid_device __always_unused *hdev)
> > > +{
> > > +}
> > > +
> > > +static int tux_ll_open(struct hid_device __always_unused *hdev)
> > > +{
> > > +	return 0;
> > > +}
> > > +
> > > +static void tux_ll_close(struct hid_device __always_unused *hdev)
> > > +{
> > > +}
> > > +
> > > +static int tux_ll_parse(struct hid_device *hdev)
> > > +{
> > > +	return hid_parse_report(hdev, tux_report_descriptor,
> > > +				sizeof(tux_report_descriptor));
> > > +}
> > > +
> > > +struct __packed lamp_array_attributes_report_t {
> > > +	const u8 report_id;
> > > +	u16 lamp_count;
> > > +	u32 bounding_box_width_in_micrometers;
> > > +	u32 bounding_box_height_in_micrometers;
> > > +	u32 bounding_box_depth_in_micrometers;
> > > +	u32 lamp_array_kind;
> > > +	u32 min_update_interval_in_microseconds;
> > > +};
> > > +
> > > +static int handle_lamp_array_attributes_report(struct hid_device *hdev,
> > > +					       struct
> > > lamp_array_attributes_report_t *rep)
> > > +{
> > > +	struct tux_hdev_driver_data_t *driver_data =
> > > dev_get_drvdata(&hdev->dev);
> > > +
> > > +	rep->lamp_count = driver_data->lamp_count;
> > > +	rep->bounding_box_width_in_micrometers = 368000;
> > > +	rep->bounding_box_height_in_micrometers = 266000;
> > > +	rep->bounding_box_depth_in_micrometers = 30000;
> > > +	/*
> > > +	 * LampArrayKindKeyboard, see "26.2.1 LampArrayKind Values" of
> > > +	 * "HID Usage Tables v1.5"
> > > +	 */
> > > +	rep->lamp_array_kind = 1;
> > > +	// Some guessed value for interval microseconds
> > > +	rep->min_update_interval_in_microseconds = 500;
> > > +
> > > +	return sizeof(*rep);
> > > +}
> > > +
> > > +struct __packed lamp_attributes_request_report_t {
> > > +	const u8 report_id;
> > > +	u16 lamp_id;
> > > +};
> > > +
> > > +static int handle_lamp_attributes_request_report(struct hid_device *hdev,
> > > +						 struct
> > > lamp_attributes_request_report_t *rep)
> > > +{
> > > +	struct tux_hdev_driver_data_t *driver_data =
> > > dev_get_drvdata(&hdev->dev);
> > > +
> > > +	if (rep->lamp_id < driver_data->lamp_count)
> > > +		driver_data->next_lamp_id = rep->lamp_id;
> > > +	else
> > > +		driver_data->next_lamp_id = 0;
> > > +
> > > +	return sizeof(*rep);
> > > +}
> > > +
> > > +struct __packed lamp_attributes_response_report_t {
> > > +	const u8 report_id;
> > > +	u16 lamp_id;
> > > +	u32 position_x_in_micrometers;
> > > +	u32 position_y_in_micrometers;
> > > +	u32 position_z_in_micrometers;
> > > +	u32 update_latency_in_microseconds;
> > > +	u32 lamp_purpose;
> > > +	u8 red_level_count;
> > > +	u8 green_level_count;
> > > +	u8 blue_level_count;
> > > +	u8 intensity_level_count;
> > > +	u8 is_programmable;
> > > +	u8 input_binding;
> > > +};
> > > +
> > > +static int handle_lamp_attributes_response_report(struct hid_device
> > > *hdev,
> > > +						  struct
> > > lamp_attributes_response_report_t *rep)
> > > +{
> > > +	struct tux_hdev_driver_data_t *driver_data =
> > > dev_get_drvdata(&hdev->dev);
> > > +	u16 lamp_id = driver_data->next_lamp_id;
> > > +
> > Don't leave empty lines into the variable declaration block.
> ack, an oversight
> > 
> > > +	const u32 *kbl_mapping_pos_x, *kbl_mapping_pos_y, *kbl_mapping_pos_z;
> > > +	const u8 *kbl_mapping;
> > > +
> > > +	rep->lamp_id = lamp_id;
> > > +	// Some guessed value for latency microseconds
> > > +	rep->update_latency_in_microseconds = 100;
> > > +	/*
> > > +	 * LampPurposeControl, see "26.3.1 LampPurposes Flags" of
> > > +	 * "HID Usage Tables v1.5"
> > > +	 */
> > > +	rep->lamp_purpose = 1;
> > > +	rep->red_level_count = 0xff;
> > > +	rep->green_level_count = 0xff;
> > > +	rep->blue_level_count = 0xff;
> > > +	rep->intensity_level_count = 0xff;
> > > +	rep->is_programmable = 1;
> > > +
> > > +	if (driver_data->keyboard_type ==
> > > WMI_AB_GET_DEVICE_STATUS_KEYBOARD_LAYOUT_ANSII) {
> > > +		kbl_mapping = &sirius_16_ansii_kbl_mapping[0];
> > > +		kbl_mapping_pos_x = &sirius_16_ansii_kbl_mapping_pos_x[0];
> > > +		kbl_mapping_pos_y = &sirius_16_ansii_kbl_mapping_pos_y[0];
> > > +		kbl_mapping_pos_z = &sirius_16_ansii_kbl_mapping_pos_z[0];
> > > +	} else if (driver_data->keyboard_type ==
> > > WMI_AB_GET_DEVICE_STATUS_KEYBOARD_LAYOUT_ISO) {
> > > +		kbl_mapping = &sirius_16_iso_kbl_mapping[0];
> > > +		kbl_mapping_pos_x = &sirius_16_iso_kbl_mapping_pos_x[0];
> > > +		kbl_mapping_pos_y = &sirius_16_iso_kbl_mapping_pos_y[0];
> > > +		kbl_mapping_pos_z = &sirius_16_iso_kbl_mapping_pos_z[0];
> > Could these components be put inside another struct so you don't need 3
> > variables to store them?
> 
> Reworked that a little bit, is now stored in struct tux_hdev_driver_data_t
> 
> struct tux_hdev_driver_data_t {
>     u8 lamp_count;
>     const u8 *kbl_map;
>     const u32 *kbl_map_pos_x;
>     const u32 *kbl_map_pos_y;
>     const u32 *kbl_map_pos_z;
>     u8 next_lamp_id;
>     union tux_wmi_xx_496in_80out_in_t next_kbl_set_multiple_keys_in;
> };
> 
> initialized only once during ll_start
> 
> or should go another level and do a nested struct?

I'd prefer the pos ones to be nested as they seem strongly related. It 
also makes it possible to store it into temporary pointer variable if
that's beneficial somewhere to make the code more readable.

> > > +		for (unsigned int j = 0; j <
> > > lamp_multi_update_report.lamp_count; ++j) {
> > > +			lamp_multi_update_report.lamp_id[j] = i + j;
> > > +			lamp_multi_update_report.update_channels[j].red =
> > > +				rep->red_update_channel;
> > > +			lamp_multi_update_report.update_channels[j].green =
> > > +				rep->green_update_channel;
> > > +			lamp_multi_update_report.update_channels[j].blue =
> > > +				rep->blue_update_channel;
> > > +			lamp_multi_update_report.update_channels[j].intensity
> > > =
> > > +				rep->intensity_update_channel;
> > > +		}
> > > +
> > > +		ret = handle_lamp_multi_update_report(hdev,
> > > &lamp_multi_update_report);
> > > +		if (ret < 0)
> > > +			return ret;
> > > +		else if (ret != sizeof(struct lamp_multi_update_report_t))
> > Unnecessary "else".
> 
> practically handle_lamp_multi_update_report can only return <0 or the correct
> size the was i implemented it, but theoretically other values would be wrong
> and this code would document that if in the future other values are possible
> 
> I know a somewhat philosophical train of thought. If you want i can just
> delete the else and optionally replace it with a comment about the return
> value.

I think you misunderstood why, I didn't mean remove the whole else if 
thing.

There's return statement in the if block so else is not required, you can 
do the same without explicit "else":

		if (ret < 0)
			return ret;
		if (ret != sizeof(...))
			...

> > Take sizeof directly from the related struct variable.
> ack
> > 
> > > +			return -EIO;
> > > +	}
> > > +
> > > +	return sizeof(*rep);
> > > +}


> > > +{
> > > +	/*
> > > +	 * The keyboards firmware doesn't have any built in controls and the
> > > +	 * built in effects are not implemented so this is a NOOP.
> > > +	 * According to the HID Documentation (HID Usage Tables v1.5) this
> > > +	 * function is optional and can be removed from the HID Report
> > > +	 * Descriptor, but it should first be confirmed that userspace
> > > respects
> > > +	 * this possibility too. The Microsoft MacroPad reference
> > > implementation
> > > +	 * (https://github.com/microsoft/RP2040MacropadHidSample 1d6c3ad)
> > > +	 * already deviates from the spec at another point, see
> > > +	 * handle_lamp_*_update_report.
> > > +	 */
> > > +
> > > +	return sizeof(*rep);
> > > +}
> > > +
> > > +static int tux_ll_raw_request(struct hid_device *hdev, unsigned char
> > > reportnum,
> > > +			      __u8 *buf, size_t len, unsigned char rtype,
> > For in-kernel usage, always use non-__ variants, so u8.
> copied that over from the definition in hid.h, but can change it ofc

Yes, use u8.


-- 
 i.

Powered by blists - more mailing lists