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
