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

[Julius Zint <julius@zint.sh>]

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