[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1813ff8c-e192-4873-a22b-5ee48a6f9185@zint.sh>
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
Powered by blists - more mailing lists