Re: [PATCH v3 3/6] platform/x86/tuxedo: Use intensity according to HID spec

[Werner Sembach]

Am 26.08.26 um 10:11 schrieb Aaron Erhardt:
> So far, this driver assumed incorrectly that the HID spec requires an
> 8-bit intensity value to be honored. The spec actually allows multiple
> intensities for RGB lamps, but suggest to rather set brightness
> through the individual channels, which is also what the Microsoft
> MacroPad reference implementation does.
>
> Accordingly, this commit simplifies intensity handling by offering
> only two intensity values for turning LEDs on and off. All other color
> values are submitted through the color channels individually, thus
> avoiding duplicated handling of brightness.
>
> Additionally, the incorrect comments explaining the deviation from the
> MacroPad reference implementation were removed.
>
> Signed-off-by: Aaron Erhardt <aer@xxxxxxxxxxxxxxxxxxx>
Reviewed-by: Werner Sembach <wse@xxxxxxxxxxxxxxxxxxx>
> ---
>   drivers/platform/x86/tuxedo/nb04/wmi_ab.c | 25 ++++++++++++-----------
>   1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/platform/x86/tuxedo/nb04/wmi_ab.c b/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
> index 11babc7c7767..8f1ffca0430d 100644
> --- a/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
> +++ b/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
> @@ -553,7 +553,7 @@ static int handle_lamp_attributes_response_report(struct hid_device *hdev,
>   	rep->red_level_count = 0xff;
>   	rep->green_level_count = 0xff;
>   	rep->blue_level_count = 0xff;
> -	rep->intensity_level_count = 0xff;
> +	rep->intensity_level_count = 0x1;
>   	rep->is_programmable = 1;
>   
>   	if (driver_data->kbl_map[lamp_id].code <= 0xe8) {
> @@ -640,22 +640,23 @@ static int handle_lamp_multi_update_report(struct hid_device *hdev,
>   					j + 1;
>   			rgb_configs_j->key_id = key_id;
>   			/*
> -			 * While this driver respects update_channel.intensity
> -			 * according to "HID Usage Tables v1.5" also on RGB
> -			 * leds, the Microsoft MacroPad reference implementation
> +			 * This driver uses update_channel.intensity according to
> +			 * "Color Attributes Examples" in "HID Usage Tables v1.7".
> +			 * Only two intensity values are allowed for turning LEDs
> +			 * on or off, while color and brightness can be controlled
> +			 * through the RGB values. This is also identical to the
> +			 * Microsoft MacroPad reference implementation
>   			 * (https://github.com/microsoft/RP2040MacropadHidSample
> -			 * 1d6c3ad) does not and ignores it. If it turns out
> -			 * that Windows writes intensity = 0 for RGB leds
> -			 * instead of intensity = 255, this driver should also
> -			 * ignore the update_channel.intensity.
> +			 * 1d6c3ad).
>   			 */
> -			intensity_i = rep->update_channels[i].intensity;
> +			intensity_i = min(1, rep->update_channels[i].intensity);
>   			red_i = rep->update_channels[i].red;
>   			green_i = rep->update_channels[i].green;
>   			blue_i = rep->update_channels[i].blue;
> -			rgb_configs_j->red = red_i * intensity_i / 0xff;
> -			rgb_configs_j->green = green_i * intensity_i / 0xff;
> -			rgb_configs_j->blue = blue_i * intensity_i / 0xff;
> +
> +			rgb_configs_j->red = red_i * intensity_i;
> +			rgb_configs_j->green = green_i * intensity_i;
> +			rgb_configs_j->blue = blue_i * intensity_i;
>   
>   			break;
>   		}