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

[Julius Zint]

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