| Message ID | 20230820094118.20521-1-julius@zint.sh |
|---|---|
| Headers | show |
| Series | HID backlight driver | expand |
Le 20/08/2023 à 11:41, Julius Zint a écrit : > 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@zint.sh> > --- [...] > +static void hid_bl_remove(struct hid_device *hdev) > +{ > + struct backlight_device *bl; > + struct hid_bl_data *data; > + > + hid_dbg(hdev, "remove\n"); > + bl = hid_get_drvdata(hdev); > + data = bl_get_data(bl); > + > + devm_backlight_device_unregister(&hdev->dev, bl); Hi, If this need to be called here, before the hid_() calls below, there seems to be no real point in using devm_backlight_device_register() / devm_backlight_device_unregister(). The non-devm_ version should be enough. > + hid_hw_close(hdev); > + hid_hw_stop(hdev); > + hid_set_drvdata(hdev, NULL); > + devm_kfree(&hdev->dev, data); 'data' is devm_kzalloc()'ed. It is likely that this explicit devm_kfree() could be saved. It will be freed by the framework. > +} [...] > +static int hid_bl_probe(struct hid_device *hdev, const struct hid_device_id *id) > +{ > + int ret; > + struct hid_field *input_field; > + struct hid_field *feature_field; > + struct hid_bl_data *data; > + struct backlight_properties props; > + struct backlight_device *bl; > + > + hid_dbg(hdev, "probe\n"); > + > + ret = hid_parse(hdev); > + if (ret) { > + hid_err(hdev, "parse failed: %d\n", ret); > + return ret; > + } > + > + ret = hid_hw_start(hdev, HID_CONNECT_DRIVER); > + if (ret) { > + hid_err(hdev, "hw start failed: %d\n", ret); > + return ret; > + } > + > + input_field = hid_get_usage_field(hdev, HID_INPUT_REPORT, > + HID_USAGE_MONITOR_CTRL, > + HID_USAGE_VESA_VCP_BRIGHTNESS); > + if (input_field == NULL) { > + ret = -ENODEV; > + goto exit_stop; > + } > + > + feature_field = hid_get_usage_field(hdev, HID_FEATURE_REPORT, > + HID_USAGE_MONITOR_CTRL, > + HID_USAGE_VESA_VCP_BRIGHTNESS); > + if (feature_field == NULL) { > + ret = -ENODEV; > + goto exit_stop; > + } > + > + if (input_field->logical_minimum != feature_field->logical_minimum) { > + hid_warn(hdev, "minimums do not match: %d / %d\n", > + input_field->logical_minimum, > + feature_field->logical_minimum); > + ret = -ENODEV; > + goto exit_stop; > + } > + > + if (input_field->logical_maximum != feature_field->logical_maximum) { > + hid_warn(hdev, "maximums do not match: %d / %d\n", > + input_field->logical_maximum, > + feature_field->logical_maximum); > + ret = -ENODEV; > + goto exit_stop; > + } > + > + hid_dbg(hdev, "Monitor VESA VCP with brightness control\n"); > + > + ret = hid_hw_open(hdev); > + if (ret) { > + hid_err(hdev, "hw open failed: %d\n", ret); > + goto exit_stop; > + } > + > + data = devm_kzalloc(&hdev->dev, sizeof(*data), GFP_KERNEL); > + if (data == NULL) { > + ret = -ENOMEM; > + goto exit_stop; exit_free? in order to undo the hid_hw_open() just above. > + } > + data->hdev = hdev; > + data->min_brightness = input_field->logical_minimum; > + data->max_brightness = input_field->logical_maximum; > + data->input_field = input_field; > + data->feature_field = feature_field; > + > + memset(&props, 0, sizeof(props)); > + props.type = BACKLIGHT_RAW; > + props.max_brightness = data->max_brightness - data->min_brightness; > + > + bl = devm_backlight_device_register(&hdev->dev, "vesa_vcp", > + &hdev->dev, data, > + &hid_bl_ops, > + &props); > + if (IS_ERR(bl)) { > + ret = PTR_ERR(bl); > + hid_err(hdev, "failed to register backlight: %d\n", ret); > + goto exit_free; > + } > + > + hid_set_drvdata(hdev, bl); > + > + return 0; > + > +exit_free: > + hid_hw_close(hdev); > + devm_kfree(&hdev->dev, data); 'data' is devm_kzalloc()'ed. It is likely that this explicit devm_kfree() could be saved. It will be freed by the framework. > + > +exit_stop: > + hid_hw_stop(hdev); > + return ret; > +} [...]
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@zint.sh> 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.
On 21.08.23 18:36, Daniel Thompson wrote: >> @@ -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. >> 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! You are right, I will remove all hid_dbg calls in v4. Thank you very much for the review. Julius
On 20.08.23 12:06, Christophe JAILLET wrote: > [...] > >> +static void hid_bl_remove(struct hid_device *hdev) >> +{ >> + struct backlight_device *bl; >> + struct hid_bl_data *data; >> + >> + hid_dbg(hdev, "remove\n"); >> + bl = hid_get_drvdata(hdev); >> + data = bl_get_data(bl); >> + >> + devm_backlight_device_unregister(&hdev->dev, bl); > > Hi, > > If this need to be called here, before the hid_() calls below, there > seems to be no real point in using devm_backlight_device_register() / > devm_backlight_device_unregister(). > > The non-devm_ version should be enough. The non-devm_ version is marked deprecated. As for the order, I am not really sure. I am concerned about someone updating the brightness while its being removed. So the HID device would already have been stopped and then I would issue a request and wait for completion. If hid_hw_request and hid_hw_wait can handle this situation we are fine. > >> + hid_hw_close(hdev); >> + hid_hw_stop(hdev); >> + hid_set_drvdata(hdev, NULL); >> + devm_kfree(&hdev->dev, data); > > 'data' is devm_kzalloc()'ed. > It is likely that this explicit devm_kfree() could be saved. It will > be freed by the framework. > >> +} > > [...] > >> + if (input_field->logical_maximum != >> feature_field->logical_maximum) { >> + hid_warn(hdev, "maximums do not match: %d / %d\n", >> + input_field->logical_maximum, >> + feature_field->logical_maximum); >> + ret = -ENODEV; >> + goto exit_stop; >> + } >> + >> + hid_dbg(hdev, "Monitor VESA VCP with brightness control\n"); >> + >> + ret = hid_hw_open(hdev); >> + if (ret) { >> + hid_err(hdev, "hw open failed: %d\n", ret); >> + goto exit_stop; >> + } >> + >> + data = devm_kzalloc(&hdev->dev, sizeof(*data), GFP_KERNEL); >> + if (data == NULL) { >> + ret = -ENOMEM; >> + goto exit_stop; > > exit_free? > in order to undo the hid_hw_open() just above. Yes, my mistake. This does not call hid_hw_close(). I will fix it in v4. > >> + } >> + data->hdev = hdev; >> + data->min_brightness = input_field->logical_minimum; >> + data->max_brightness = input_field->logical_maximum; >> + data->input_field = input_field; >> + data->feature_field = feature_field; >> + >> + memset(&props, 0, sizeof(props)); >> + props.type = BACKLIGHT_RAW; >> + props.max_brightness = data->max_brightness - data->min_brightness; >> + >> + bl = devm_backlight_device_register(&hdev->dev, "vesa_vcp", >> + &hdev->dev, data, >> + &hid_bl_ops, >> + &props); >> + if (IS_ERR(bl)) { >> + ret = PTR_ERR(bl); >> + hid_err(hdev, "failed to register backlight: %d\n", ret); >> + goto exit_free; >> + } >> + >> + hid_set_drvdata(hdev, bl); >> + >> + return 0; >> + >> +exit_free: >> + hid_hw_close(hdev); >> + devm_kfree(&hdev->dev, data); > > 'data' is devm_kzalloc()'ed. > It is likely that this explicit devm_kfree() could be saved. It will > be freed by the framework. > >> + >> +exit_stop: >> + hid_hw_stop(hdev); >> + return ret; >> +} > > [...] I will remove all of the explicit calls to devm_kfree (and others) in v4 (and test it thoroughly). Thank you for the helpful review. Julius