Messages in this thread | | | Date | Sun, 20 Aug 2023 12:06:40 +0200 | Subject | Re: [PATCH v3 1/1] backlight: hid_bl: Add VESA VCP HID backlight driver | From | Christophe JAILLET <> |
| |
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; > +}
[...]
| |