Messages in this thread | | | Date | Thu, 24 Aug 2023 19:52:51 +0200 | Subject | Re: [PATCH v3 1/1] backlight: hid_bl: Add VESA VCP HID backlight driver | From | Julius Zint <> |
| |
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
| |