Re: [PATCH v3 1/1] backlight: hid_bl: Add VESA VCP HID backlight driver
From: Daniel Thompson <hidden>
Date: 2023-08-21 16:36:39
Also in:
dri-devel, linux-fbdev, lkml
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 <redacted>
I saw Christophe's review (thanks) and won't repeat anything from there...
quoted hunk ↗ jump to hunk
@@ -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!
quoted hunk ↗ jump to hunk
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).
quoted hunk ↗ jump to hunk
+ 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.