On 21.08.23 18:36, Daniel Thompson wrote:

quoted

@@ -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!

Will remove it in v4.

quoted

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

+	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!

You are right, I will remove all hid_dbg calls in v4.

Thank you very much for the review.

Julius