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

["Thomas Weißschuh" <thomas@t-8ch.de>]

On 2023-08-20 11:41:18+0200, Julius Zint wrote:
> [..]

> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> index 51387b1ef012..b964a820956d 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -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.

Is the last sentence needed?
It will go out of date soon, requiring updates to the Kconfig.

> +
>  endif # BACKLIGHT_CLASS_DEVICE
>  
>  endmenu

> [..]

> 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
> @@ -0,0 +1,269 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/device.h>
> +#include <linux/hid.h>
> +#include <linux/module.h>
> +#include <linux/backlight.h>
> +
> +#define APPLE_STUDIO_DISPLAY_VENDOR_ID  0x05ac
> +#define APPLE_STUDIO_DISPLAY_PRODUCT_ID 0x1114

Use hid-ids.h.  The vendor ID already has an entry.

> +
> +#define HID_USAGE_MONITOR_CTRL			0x800001
> +#define HID_USAGE_VESA_VCP_BRIGHTNESS		0x820010

> [..]

> +static int hid_bl_probe(struct hid_device *hdev, const struct hid_device_id *id)
> +{

> [..]

> +
> +	memset(&props, 0, sizeof(props));
> +	props.type = BACKLIGHT_RAW;

Wouldn't this be more a BACKLIGHT_FIRMWARE?

> +	props.max_brightness = data->max_brightness - data->min_brightness;
> +
> +	bl = devm_backlight_device_register(&hdev->dev, "vesa_vcp",

It's non-obvious that the "vesa_vcp" backlight comes from the
"hid_backlight" driver. Maybe align the names.

What happens when multiple compatible devices are used?
That seems to be possible with external monitors.

Can existing userspace figure out which display the backlight device
belongs to?
(I don't know either)

> +					    &hdev->dev, data,
> +					    &hid_bl_ops,
> +					    &props);

> [..]