linux-kernel - Re: [PATCH v3 1/1] backlight: hid_bl: Add VESA VCP HID backlight driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 24 Aug 2023 19:35:04 +0200
From:   Julius Zint <julius@...t.sh>
To:     Daniel Thompson <daniel.thompson@...aro.org>
Cc:     Lee Jones <lee@...nel.org>, Jingoo Han <jingoohan1@...il.com>,
        Jiri Kosina <jikos@...nel.org>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        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
Subject: Re: [PATCH v3 1/1] backlight: hid_bl: Add VESA VCP HID backlight
 driver

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