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:   Sat, 26 Aug 2023 12:15:49 +0200
From:   Thomas Weißschuh <thomas@...ch.de>
To:     Julius Zint <julius@...t.sh>
Cc:     Lee Jones <lee@...nel.org>,
        Daniel Thompson <daniel.thompson@...aro.org>,
        Jingoo Han <jingoohan1@...il.com>,
        Jiri Kosina <jikos@...nel.org>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        Helge Deller <deller@....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 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);

> [..]