[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <730f3107-f7b6-4260-837e-5ddad3a40e35@zint.sh>
Date: Thu, 24 Aug 2023 19:52:51 +0200
From: Julius Zint <julius@...t.sh>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>
Cc: Helge Deller <deller@....de>,
Thomas Weißschuh <thomas@...ch.de>,
linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
linux-input@...r.kernel.org, linux-fbdev@...r.kernel.org,
Benjamin Tissoires <benjamin.tissoires@...hat.com>,
Jiri Kosina <jikos@...nel.org>,
Jingoo Han <jingoohan1@...il.com>,
Daniel Thompson <daniel.thompson@...aro.org>,
Lee Jones <lee@...nel.org>
Subject: Re: [PATCH v3 1/1] backlight: hid_bl: Add VESA VCP HID backlight
driver
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
Powered by blists - more mailing lists