Re: [PATCH v3 1/1] backlight: hid_bl: Add VESA VCP HID backlight driver

[Daniel Thompson]

On Sun, Aug 20, 2023 at 11:41:18AM +0200, Julius Zint wrote:
> The HID spec defines the following Usage IDs (p. 345 ff):
>
> - Monitor Page (0x80) -> Monitor Control (0x01)
> - VESA Virtual Controls Page (0x82) -> Brightness (0x10)
>
> Apple made use of them in their Apple Studio Display and most likely on
> other external displays (LG UltraFine 5k, Pro Display XDR).
>
> The driver will work for any HID device with a report, where the
> application matches the Monitor Control Usage ID and:
>
> 1. An Input field in this report with the Brightness Usage ID (to get
>    the current brightness)
> 2. A Feature field in this report with the Brightness Usage ID (to
>    set the current brightness)
>
> This driver has been developed and tested with the Apple Studio Display.
> Here is a small excerpt from the decoded HID descriptor showing the
> feature field for setting the brightness:
>
>   Usage Page (Monitor VESA VCP),  ; Monitor VESA VPC (82h, monitor page)
>   Usage (10h, Brightness),
>   Logical Minimum (400),
>   Logical Maximum (60000),
>   Unit (Centimeter^-2 * Candela),
>   Unit Exponent (14),
>   Report Size (32),
>   Report Count (1),
>   Feature (Variable, Null State),
>
> The full HID descriptor dump is available as a comment in the source
> code.
>
> Signed-off-by: Julius Zint <julius@xxxxxxx>

I saw Christophe's review (thanks) and won't repeat anything from
there...

> @@ -472,6 +472,14 @@ config BACKLIGHT_LED
>  	  If you have a LCD backlight adjustable by LED class driver, say Y
>  	  to enable this driver.
>
> +config BACKLIGHT_HID
> +	tristate "VESA VCP HID Backlight Driver"
> +	depends on HID
> +	help
> +	  If you have an external display with VESA compliant HID brightness
> +	  controls then say Y to enable this backlight driver. Currently the
> +	  only supported device is the Apple Studio Display.

This contradicts the description which says you write the driver to the
standard but only tested on Apple Studio Display. There is no need to
spell what has been tested in the Kconfig text. Remove the final
sentence!

> diff --git a/drivers/video/backlight/hid_bl.c b/drivers/video/backlight/hid_bl.c
> new file mode 100644
> index 000000000000..b40f8f412ee2
> --- /dev/null
> +++ b/drivers/video/backlight/hid_bl.c
> <snip>
> +static void hid_bl_remove(struct hid_device *hdev)
> +{
> +	struct backlight_device *bl;
> +	struct hid_bl_data *data;
> +
> +	hid_dbg(hdev, "remove\n");

This message probably should be removed (if you want to know if a function was
executed use ftrace).


> +	bl = hid_get_drvdata(hdev);
> +	data = bl_get_data(bl);
> +
> +	devm_backlight_device_unregister(&hdev->dev, bl);
> +	hid_hw_close(hdev);
> +	hid_hw_stop(hdev);
> +	hid_set_drvdata(hdev, NULL);
> +	devm_kfree(&hdev->dev, data);
> +}
> +
> +static int hid_bl_get_brightness_raw(struct hid_bl_data *data)
> +{
> +	struct hid_field *field;
> +	int result;
> +
> +	field = data->input_field;
> +	hid_hw_request(data->hdev, field->report, HID_REQ_GET_REPORT);
> +	hid_hw_wait(data->hdev);
> +	result = *field->new_value;
> +	hid_dbg(data->hdev, "get brightness: %d\n", result);

To be honest I'm a little dubious about *all* the hid_dbg() calls. They
add very little value (e.g. they are useful to get the driver working
but not that important to keeping it working). As such I don't think
they are worth the clutter in a CONFIG_DYNAMIC_DEBUG kernel.

Note this is strictly for the hid_dbg() stuff... the hid_err() stuff in
the probe error paths are much more useful!


Daniel.