* Re: [PATCH 1/1] backlight: apple_bl_usb: Add Apple Studio Display support
2023-07-01 12:08 ` [PATCH 1/1] backlight: apple_bl_usb: Add Apple Studio Display support Julius Zint
@ 2023-07-01 13:32 ` Rafał Miłecki
2023-07-01 14:00 ` kernel test robot
2023-07-01 22:00 ` Sam Ravnborg
2 siblings, 0 replies; 8+ messages in thread
From: Rafał Miłecki @ 2023-07-01 13:32 UTC (permalink / raw)
To: Julius Zint, Lee Jones, Daniel Thompson, Jingoo Han
Cc: Helge Deller, linux-kernel, dri-devel, linux-fbdev
On 1.07.2023 14:08, Julius Zint wrote:
> The Apple Studio Display does not have any physical buttons and the only
> way to get or set the brightness is by sending USB control transfers to a
> HID device exposed by the display.
>
> These control transfers take the form of a HID_(GET|SET)_REPORT request
> and the payload looks like this:
>
> struct brightness_ctrl_message_data {
> u8 unknown_1;
> __le16 brightness;
> u8 unkown_2[4];
> } __packed;
>
> When compiled as a module this driver needs to be part of the early boot
> environment, otherwise the generic USB HID driver will claim the device.
>
> Signed-off-by: Julius Zint <julius@zint.sh>
Few comments below.
> ---
> drivers/video/backlight/Kconfig | 8 +
> drivers/video/backlight/Makefile | 1 +
> drivers/video/backlight/apple_bl_usb.c | 264 +++++++++++++++++++++++++
> 3 files changed, 273 insertions(+)
> create mode 100644 drivers/video/backlight/apple_bl_usb.c
>
> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> index 51387b1ef012..9383d402ebed 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -290,6 +290,14 @@ config BACKLIGHT_APPLE
> If you have an Intel-based Apple say Y to enable a driver for its
> backlight.
>
> +config BACKLIGHT_APPLE_USB
> + tristate "Apple USB Backlight Driver"
> + depends on USB
> + help
> + If you have an external display from Apple that is attached via USB
> + say Y to enable a driver for its backlight. Currently it supports the
> + Apple Studio Display.
> +
> config BACKLIGHT_QCOM_WLED
> tristate "Qualcomm PMIC WLED Driver"
> select REGMAP
> diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
> index f72e1c3c59e9..c42880655113 100644
> --- a/drivers/video/backlight/Makefile
> +++ b/drivers/video/backlight/Makefile
> @@ -23,6 +23,7 @@ obj-$(CONFIG_BACKLIGHT_ADP5520) += adp5520_bl.o
> obj-$(CONFIG_BACKLIGHT_ADP8860) += adp8860_bl.o
> obj-$(CONFIG_BACKLIGHT_ADP8870) += adp8870_bl.o
> obj-$(CONFIG_BACKLIGHT_APPLE) += apple_bl.o
> +obj-$(CONFIG_BACKLIGHT_APPLE_USB) += apple_bl_usb.o
> obj-$(CONFIG_BACKLIGHT_AS3711) += as3711_bl.o
> obj-$(CONFIG_BACKLIGHT_BD6107) += bd6107.o
> obj-$(CONFIG_BACKLIGHT_CARILLO_RANCH) += cr_bllcd.o
> diff --git a/drivers/video/backlight/apple_bl_usb.c b/drivers/video/backlight/apple_bl_usb.c
> new file mode 100644
> index 000000000000..b746b7822974
> --- /dev/null
> +++ b/drivers/video/backlight/apple_bl_usb.c
> @@ -0,0 +1,264 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/usb.h>
> +#include <linux/backlight.h>
> +#include <asm/byteorder.h>
> +
> +#define APPLE_STUDIO_DISPLAY_VENDOR_ID 0x05ac
> +#define APPLE_STUDIO_DISPLAY_PRODUCT_ID 0x1114
> +
> +#define HID_GET_REPORT 0x01
> +#define HID_SET_REPORT 0x09
> +
> +#define HID_REPORT_TYPE_FEATURE 0x0300
> +
> +struct apple_bl_usb_data {
> + struct usb_interface *usb_interface;
> + struct usb_device *usb_dev;
> +};
> +
> +struct brightness_ctrl_message_data {
> + u8 unknown_1;
> + __le16 brightness;
> + u8 unkown_2[4];
Typo
> +} __packed;
> +
> +void init_ctrl_msg_data(struct brightness_ctrl_message_data *msg)
> +{
> + memset(msg, 0, sizeof(struct brightness_ctrl_message_data));
> + msg->unknown_1 = 0x01;
> +}
> +
> +void set_ctrl_message_brightness(struct brightness_ctrl_message_data *msg,
> + u16 brightness_value)
> +{
> + msg->brightness = cpu_to_le16(brightness_value + 400);
> +}
> +
> +u16 get_ctrl_message_brightness(struct brightness_ctrl_message_data *msg)
> +{
> + return le16_to_cpu(msg->brightness) - 400;
> +}
> +
> +int apple_bl_usb_usb_get_brightness(struct usb_interface *interface,
> + struct usb_device *usb_dev,
> + int *brightness)
> +{
> + int err;
> + u16 interface_nr;
> + int msg_data_size;
> + struct brightness_ctrl_message_data *msg_data;
> +
> + msg_data_size = sizeof(struct brightness_ctrl_message_data);
> + msg_data = kzalloc(msg_data_size, GFP_KERNEL);
Check for NULL as kzalloc() may fail
> + memset(msg_data, 0x00, msg_data_size);
> + interface_nr = interface->cur_altsetting->desc.bInterfaceNumber;
> +
> + err = usb_control_msg(usb_dev,
> + usb_rcvctrlpipe(usb_dev, 0),
> + HID_GET_REPORT,
> + USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> + /* wValue: HID-Report Type and Report ID */
> + HID_REPORT_TYPE_FEATURE | 0x01,
> + interface_nr /* wIndex */,
> + msg_data,
> + msg_data_size,
> + HZ);
> + if (err < 0) {
> + dev_err(&interface->dev,
> + "get: usb control message err: %d\n",
> + err);
Shouldn't you kfree() and return err here?
> + }
> + *brightness = get_ctrl_message_brightness(msg_data);
> + kfree(msg_data);
> + dev_dbg(&interface->dev, "get brightness: %d\n", *brightness);
> + return 0;
> +}
> +
> +int apple_bl_usb_usb_set_brightness(struct usb_interface *interface,
> + struct usb_device *usb_dev,
> + int brightness)
> +{
> + int err;
> + u16 interface_nr;
> + struct brightness_ctrl_message_data *msg_data;
> +
> + msg_data = kzalloc(sizeof(struct brightness_ctrl_message_data), GFP_KERNEL);
Same here
> + interface_nr = interface->cur_altsetting->desc.bInterfaceNumber;
> + init_ctrl_msg_data(msg_data);
> + set_ctrl_message_brightness(msg_data, brightness);
> +
> + err = usb_control_msg(usb_dev,
> + usb_sndctrlpipe(usb_dev, 0),
> + HID_SET_REPORT,
> + USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> + /* wValue: HID-Report Type and Report ID */
> + HID_REPORT_TYPE_FEATURE | 0x01,
> + interface_nr /* wIndex */,
> + msg_data,
> + sizeof(struct brightness_ctrl_message_data),
> + HZ);
> + kfree(msg_data);
> + if (err < 0) {
> + dev_err(&interface->dev,
> + "set: usb control message err: %d\n",
> + err);
> + return err;
> + }
> + dev_dbg(&interface->dev, "set brightness: %d\n", brightness);
> + return 0;
> +}
> +
> +int apple_bl_usb_check_fb(struct backlight_device *bd, struct fb_info *info)
> +{
> + dev_info(&bd->dev, "check fb\n");
> + return 0;
> +}
Do we need that function / print?
> +
> +int apple_bl_usb_get_brightness(struct backlight_device *bl)
> +{
> + int ret;
> + struct apple_bl_usb_data *data;
> + int hw_brightness;
> +
> + data = bl_get_data(bl);
> + ret = apple_bl_usb_usb_get_brightness(data->usb_interface,
> + data->usb_dev,
> + &hw_brightness);
> + if (!ret)
> + ret = hw_brightness;
> +
> + return ret;
> +}
> +
> +int apple_bl_usb_update_status(struct backlight_device *bl)
> +{
> + int err;
> + struct apple_bl_usb_data *data;
> +
> + data = bl_get_data(bl);
> + err = apple_bl_usb_usb_set_brightness(data->usb_interface,
> + data->usb_dev,
> + bl->props.brightness);
> + return err;
> +}
> +
> +static const struct backlight_ops apple_bl_usb_ops = {
> + .update_status = apple_bl_usb_update_status,
> + .get_brightness = apple_bl_usb_get_brightness,
> + .check_fb = apple_bl_usb_check_fb,
> +};
> +
> +static void apple_bl_usb_disconnect(struct usb_interface *interface)
> +{
> + struct backlight_device *bl;
> +
> + dev_dbg(&interface->dev, "disconnect\n");
> +
> + bl = usb_get_intfdata(interface);
> + usb_set_intfdata(interface, NULL);
> + backlight_device_unregister(bl);
> +}
> +
> +static int apple_bl_usb_probe(struct usb_interface *interface,
> + const struct usb_device_id *id)
> +{
> + struct backlight_properties props;
> + struct backlight_device *bl;
> + struct usb_device *usb_dev;
> + struct device *dev;
> + struct apple_bl_usb_data *data;
> + int brightness_interface_nr;
> +
> + dev_dbg(&interface->dev, "probe\n");
> +
> + dev = &interface->dev;
> + usb_dev = interface_to_usbdev(interface);
> +
> + switch (usb_dev->config->desc.bConfigurationValue) {
> + case 1:
> + brightness_interface_nr = 0x7;
> + break;
> + case 2:
> + brightness_interface_nr = 0x9;
> + break;
> + case 3:
> + brightness_interface_nr = 0xc;
> + break;
> + default:
> + dev_err(dev,
> + "unexpected configuration value: %d\n",
> + usb_dev->config->desc.bConfigurationValue);
> + return -EINVAL;
> + }
> +
> + if (interface->cur_altsetting->desc.bInterfaceNumber != brightness_interface_nr)
> + return -ENODEV;
> +
> + data = devm_kzalloc(dev,
> + sizeof(struct apple_bl_usb_data),
> + GFP_KERNEL);
I don't think devm_kzalloc() can return error pointer. Just check for NULL below.
> + if (IS_ERR(data)) {
> + dev_err(dev, "failed to allocate memory\n");
> + return PTR_ERR(bl);
> + }
> + data->usb_interface = interface;
> + data->usb_dev = usb_dev;
> +
> + // Valid brightness values for the apple studio display range from 400
> + // to 60000. Since the backlight subsystem´s brightness value starts
> + // from 0, we use 0 to 59600 and offset it by the minimum value.
> + memset(&props, 0, sizeof(props));
> + props.type = BACKLIGHT_RAW;
> + props.max_brightness = 59600;
> +
> + bl = backlight_device_register("apple_studio_display",
> + dev,
> + data,
> + &apple_bl_usb_ops,
> + &props);
> + if (IS_ERR(bl)) {
> + dev_err(dev, "failed to register backlight\n");
> + return PTR_ERR(bl);
> + }
> + usb_set_intfdata(interface, bl);
> + return 0;
> +}
> +
> +static int apple_bl_usb_suspend(struct usb_interface *interface,
> + pm_message_t message)
> +{
> + dev_dbg(&interface->dev, "suspend\n");
> + return 0;
> +}
> +
> +static int apple_bl_usb_resume(struct usb_interface *interface)
> +{
> + dev_dbg(&interface->dev, "resume\n");
> + return 0;
> +}
> +
> +static const struct usb_device_id id_table[] = {
> + {
> + .idVendor = APPLE_STUDIO_DISPLAY_VENDOR_ID,
> + .idProduct = APPLE_STUDIO_DISPLAY_PRODUCT_ID,
> + },
> + {},
> +};
> +MODULE_DEVICE_TABLE(usb, id_table);
> +
> +static struct usb_driver usb_asdbl_driver = {
> + .name = "apple_bl_usb",
> + .probe = apple_bl_usb_probe,
> + .disconnect = apple_bl_usb_disconnect,
> + .id_table = id_table,
> + .suspend = apple_bl_usb_suspend,
> + .resume = apple_bl_usb_resume,
> + .reset_resume = apple_bl_usb_resume
> +};
> +module_usb_driver(usb_asdbl_driver);
> +
> +MODULE_AUTHOR("Julius Zint <julius@zint.sh>");
> +MODULE_LICENSE("Dual MIT/GPL");
> +MODULE_DESCRIPTION("Backlight control for USB-C Thunderbolt Apple displays");
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/1] backlight: apple_bl_usb: Add Apple Studio Display support
2023-07-01 12:08 ` [PATCH 1/1] backlight: apple_bl_usb: Add Apple Studio Display support Julius Zint
2023-07-01 13:32 ` Rafał Miłecki
@ 2023-07-01 14:00 ` kernel test robot
2023-07-01 22:00 ` Sam Ravnborg
2 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2023-07-01 14:00 UTC (permalink / raw)
To: Julius Zint, Lee Jones, Daniel Thompson, Jingoo Han
Cc: oe-kbuild-all, Helge Deller, linux-kernel, dri-devel, linux-fbdev,
Julius Zint
Hi Julius,
kernel test robot noticed the following build warnings:
[auto build test WARNING on lee-backlight/for-backlight-next]
[also build test WARNING on lee-leds/for-leds-next drm-misc/drm-misc-next drm-tip/drm-tip linus/master v6.4 next-20230630]
[cannot apply to lee-backlight/for-backlight-fixes]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Julius-Zint/backlight-apple_bl_usb-Add-Apple-Studio-Display-support/20230701-202142
base: https://git.kernel.org/pub/scm/linux/kernel/git/lee/backlight.git for-backlight-next
patch link: https://lore.kernel.org/r/20230701120806.11812-2-julius%40zint.sh
patch subject: [PATCH 1/1] backlight: apple_bl_usb: Add Apple Studio Display support
config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20230701/202307012107.OW4d1gBR-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230701/202307012107.OW4d1gBR-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202307012107.OW4d1gBR-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/video/backlight/apple_bl_usb.c:27:6: warning: no previous prototype for 'init_ctrl_msg_data' [-Wmissing-prototypes]
27 | void init_ctrl_msg_data(struct brightness_ctrl_message_data *msg)
| ^~~~~~~~~~~~~~~~~~
>> drivers/video/backlight/apple_bl_usb.c:33:6: warning: no previous prototype for 'set_ctrl_message_brightness' [-Wmissing-prototypes]
33 | void set_ctrl_message_brightness(struct brightness_ctrl_message_data *msg,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/video/backlight/apple_bl_usb.c:39:5: warning: no previous prototype for 'get_ctrl_message_brightness' [-Wmissing-prototypes]
39 | u16 get_ctrl_message_brightness(struct brightness_ctrl_message_data *msg)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/video/backlight/apple_bl_usb.c:44:5: warning: no previous prototype for 'apple_bl_usb_usb_get_brightness' [-Wmissing-prototypes]
44 | int apple_bl_usb_usb_get_brightness(struct usb_interface *interface,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/video/backlight/apple_bl_usb.c:79:5: warning: no previous prototype for 'apple_bl_usb_usb_set_brightness' [-Wmissing-prototypes]
79 | int apple_bl_usb_usb_set_brightness(struct usb_interface *interface,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/video/backlight/apple_bl_usb.c:113:5: warning: no previous prototype for 'apple_bl_usb_check_fb' [-Wmissing-prototypes]
113 | int apple_bl_usb_check_fb(struct backlight_device *bd, struct fb_info *info)
| ^~~~~~~~~~~~~~~~~~~~~
>> drivers/video/backlight/apple_bl_usb.c:119:5: warning: no previous prototype for 'apple_bl_usb_get_brightness' [-Wmissing-prototypes]
119 | int apple_bl_usb_get_brightness(struct backlight_device *bl)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/video/backlight/apple_bl_usb.c:135:5: warning: no previous prototype for 'apple_bl_usb_update_status' [-Wmissing-prototypes]
135 | int apple_bl_usb_update_status(struct backlight_device *bl)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/init_ctrl_msg_data +27 drivers/video/backlight/apple_bl_usb.c
26
> 27 void init_ctrl_msg_data(struct brightness_ctrl_message_data *msg)
28 {
29 memset(msg, 0, sizeof(struct brightness_ctrl_message_data));
30 msg->unknown_1 = 0x01;
31 }
32
> 33 void set_ctrl_message_brightness(struct brightness_ctrl_message_data *msg,
34 u16 brightness_value)
35 {
36 msg->brightness = cpu_to_le16(brightness_value + 400);
37 }
38
> 39 u16 get_ctrl_message_brightness(struct brightness_ctrl_message_data *msg)
40 {
41 return le16_to_cpu(msg->brightness) - 400;
42 }
43
> 44 int apple_bl_usb_usb_get_brightness(struct usb_interface *interface,
45 struct usb_device *usb_dev,
46 int *brightness)
47 {
48 int err;
49 u16 interface_nr;
50 int msg_data_size;
51 struct brightness_ctrl_message_data *msg_data;
52
53 msg_data_size = sizeof(struct brightness_ctrl_message_data);
54 msg_data = kzalloc(msg_data_size, GFP_KERNEL);
55 memset(msg_data, 0x00, msg_data_size);
56 interface_nr = interface->cur_altsetting->desc.bInterfaceNumber;
57
58 err = usb_control_msg(usb_dev,
59 usb_rcvctrlpipe(usb_dev, 0),
60 HID_GET_REPORT,
61 USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
62 /* wValue: HID-Report Type and Report ID */
63 HID_REPORT_TYPE_FEATURE | 0x01,
64 interface_nr /* wIndex */,
65 msg_data,
66 msg_data_size,
67 HZ);
68 if (err < 0) {
69 dev_err(&interface->dev,
70 "get: usb control message err: %d\n",
71 err);
72 }
73 *brightness = get_ctrl_message_brightness(msg_data);
74 kfree(msg_data);
75 dev_dbg(&interface->dev, "get brightness: %d\n", *brightness);
76 return 0;
77 }
78
> 79 int apple_bl_usb_usb_set_brightness(struct usb_interface *interface,
80 struct usb_device *usb_dev,
81 int brightness)
82 {
83 int err;
84 u16 interface_nr;
85 struct brightness_ctrl_message_data *msg_data;
86
87 msg_data = kzalloc(sizeof(struct brightness_ctrl_message_data), GFP_KERNEL);
88 interface_nr = interface->cur_altsetting->desc.bInterfaceNumber;
89 init_ctrl_msg_data(msg_data);
90 set_ctrl_message_brightness(msg_data, brightness);
91
92 err = usb_control_msg(usb_dev,
93 usb_sndctrlpipe(usb_dev, 0),
94 HID_SET_REPORT,
95 USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
96 /* wValue: HID-Report Type and Report ID */
97 HID_REPORT_TYPE_FEATURE | 0x01,
98 interface_nr /* wIndex */,
99 msg_data,
100 sizeof(struct brightness_ctrl_message_data),
101 HZ);
102 kfree(msg_data);
103 if (err < 0) {
104 dev_err(&interface->dev,
105 "set: usb control message err: %d\n",
106 err);
107 return err;
108 }
109 dev_dbg(&interface->dev, "set brightness: %d\n", brightness);
110 return 0;
111 }
112
> 113 int apple_bl_usb_check_fb(struct backlight_device *bd, struct fb_info *info)
114 {
115 dev_info(&bd->dev, "check fb\n");
116 return 0;
117 }
118
> 119 int apple_bl_usb_get_brightness(struct backlight_device *bl)
120 {
121 int ret;
122 struct apple_bl_usb_data *data;
123 int hw_brightness;
124
125 data = bl_get_data(bl);
126 ret = apple_bl_usb_usb_get_brightness(data->usb_interface,
127 data->usb_dev,
128 &hw_brightness);
129 if (!ret)
130 ret = hw_brightness;
131
132 return ret;
133 }
134
> 135 int apple_bl_usb_update_status(struct backlight_device *bl)
136 {
137 int err;
138 struct apple_bl_usb_data *data;
139
140 data = bl_get_data(bl);
141 err = apple_bl_usb_usb_set_brightness(data->usb_interface,
142 data->usb_dev,
143 bl->props.brightness);
144 return err;
145 }
146
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/1] backlight: apple_bl_usb: Add Apple Studio Display support
2023-07-01 12:08 ` [PATCH 1/1] backlight: apple_bl_usb: Add Apple Studio Display support Julius Zint
2023-07-01 13:32 ` Rafał Miłecki
2023-07-01 14:00 ` kernel test robot
@ 2023-07-01 22:00 ` Sam Ravnborg
2 siblings, 0 replies; 8+ messages in thread
From: Sam Ravnborg @ 2023-07-01 22:00 UTC (permalink / raw)
To: Julius Zint
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Helge Deller,
linux-kernel, dri-devel, linux-fbdev
Hi Julius.
Thanks for posting this. I few nits in the following where you as the
author decide what to ignore and what to update.
Sam
On Sat, Jul 01, 2023 at 02:08:03PM +0200, Julius Zint wrote:
> The Apple Studio Display does not have any physical buttons and the only
> way to get or set the brightness is by sending USB control transfers to a
> HID device exposed by the display.
>
> These control transfers take the form of a HID_(GET|SET)_REPORT request
> and the payload looks like this:
>
> struct brightness_ctrl_message_data {
> u8 unknown_1;
> __le16 brightness;
> u8 unkown_2[4];
> } __packed;
>
> When compiled as a module this driver needs to be part of the early boot
> environment, otherwise the generic USB HID driver will claim the device.
I hope someone else can help here, as I have no clue.
>
> Signed-off-by: Julius Zint <julius@zint.sh>
> ---
> drivers/video/backlight/Kconfig | 8 +
> drivers/video/backlight/Makefile | 1 +
> drivers/video/backlight/apple_bl_usb.c | 264 +++++++++++++++++++++++++
> 3 files changed, 273 insertions(+)
> create mode 100644 drivers/video/backlight/apple_bl_usb.c
>
> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> index 51387b1ef012..9383d402ebed 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -290,6 +290,14 @@ config BACKLIGHT_APPLE
> If you have an Intel-based Apple say Y to enable a driver for its
> backlight.
>
> +config BACKLIGHT_APPLE_USB
> + tristate "Apple USB Backlight Driver"
> + depends on USB
> + help
> + If you have an external display from Apple that is attached via USB
> + say Y to enable a driver for its backlight. Currently it supports the
> + Apple Studio Display.
> +
> config BACKLIGHT_QCOM_WLED
> tristate "Qualcomm PMIC WLED Driver"
> select REGMAP
> diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
> index f72e1c3c59e9..c42880655113 100644
> --- a/drivers/video/backlight/Makefile
> +++ b/drivers/video/backlight/Makefile
> @@ -23,6 +23,7 @@ obj-$(CONFIG_BACKLIGHT_ADP5520) += adp5520_bl.o
> obj-$(CONFIG_BACKLIGHT_ADP8860) += adp8860_bl.o
> obj-$(CONFIG_BACKLIGHT_ADP8870) += adp8870_bl.o
> obj-$(CONFIG_BACKLIGHT_APPLE) += apple_bl.o
> +obj-$(CONFIG_BACKLIGHT_APPLE_USB) += apple_bl_usb.o
> obj-$(CONFIG_BACKLIGHT_AS3711) += as3711_bl.o
> obj-$(CONFIG_BACKLIGHT_BD6107) += bd6107.o
> obj-$(CONFIG_BACKLIGHT_CARILLO_RANCH) += cr_bllcd.o
> diff --git a/drivers/video/backlight/apple_bl_usb.c b/drivers/video/backlight/apple_bl_usb.c
> new file mode 100644
> index 000000000000..b746b7822974
> --- /dev/null
> +++ b/drivers/video/backlight/apple_bl_usb.c
> @@ -0,0 +1,264 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/usb.h>
> +#include <linux/backlight.h>
> +#include <asm/byteorder.h>
> +
> +#define APPLE_STUDIO_DISPLAY_VENDOR_ID 0x05ac
> +#define APPLE_STUDIO_DISPLAY_PRODUCT_ID 0x1114
> +
> +#define HID_GET_REPORT 0x01
> +#define HID_SET_REPORT 0x09
> +
> +#define HID_REPORT_TYPE_FEATURE 0x0300
> +
> +struct apple_bl_usb_data {
> + struct usb_interface *usb_interface;
> + struct usb_device *usb_dev;
> +};
> +
> +struct brightness_ctrl_message_data {
> + u8 unknown_1;
> + __le16 brightness;
> + u8 unkown_2[4];
> +} __packed;
A different way to set the brightness value could be:
struct brightness_ctrl_message_data {
u8 cmd;
u8[2] brightness;
u8 unknown[4];
} __packed;
static void set_ctrl_message_brightness(struct brightness_ctrl_message_data *msg,
u16 brightness_value)
{
u16 brightness = brightness_value + 400;
msg->brightness[0] = brightness & 0xff;
msg->brightness[2] = brightness >> 8;
}
This is similar to what is done in drm_mipi_dsi.
Other backlight drivers (except one) uses similar tricks to handle when
the brightness is more than one byte.
The magic number 400 would be better represented by a constant like:
#define BACKLIGHT_INTENSITY_OFFSET 400
Or something like this.
It also from the code looks like unknown_1 is a command byte.
#define GET_BACKLIGHT_INTENSITY 0x0
#define SET_BACKLIGHT_INTENSITY 0x1
Looks more descriptive than the current hard coding, implicit via memset
or explicit.
> +void init_ctrl_msg_data(struct brightness_ctrl_message_data *msg)
> +{
> + memset(msg, 0, sizeof(struct brightness_ctrl_message_data));
> + msg->unknown_1 = 0x01;
> +}
As the build bot already told you, please use static everywhere
possible.
In this case just drop the helper as it has only one user.
> +
> +void set_ctrl_message_brightness(struct brightness_ctrl_message_data *msg,
> + u16 brightness_value)
> +{
> + msg->brightness = cpu_to_le16(brightness_value + 400);
> +}
> +
> +u16 get_ctrl_message_brightness(struct brightness_ctrl_message_data *msg)
> +{
> + return le16_to_cpu(msg->brightness) - 400;
> +}
> +
> +int apple_bl_usb_usb_get_brightness(struct usb_interface *interface,
> + struct usb_device *usb_dev,
> + int *brightness)
> +{
> + int err;
> + u16 interface_nr;
> + int msg_data_size;
> + struct brightness_ctrl_message_data *msg_data;
> +
> + msg_data_size = sizeof(struct brightness_ctrl_message_data);
> + msg_data = kzalloc(msg_data_size, GFP_KERNEL);
The struct is so small that you can safely have it as a local variable.
The pointer is almost the same size. And then there is no need to check
for failing allocations.
> + memset(msg_data, 0x00, msg_data_size);
> + interface_nr = interface->cur_altsetting->desc.bInterfaceNumber;
> +
> + err = usb_control_msg(usb_dev,
> + usb_rcvctrlpipe(usb_dev, 0),
> + HID_GET_REPORT,
> + USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> + /* wValue: HID-Report Type and Report ID */
> + HID_REPORT_TYPE_FEATURE | 0x01,
> + interface_nr /* wIndex */,
> + msg_data,
> + msg_data_size,
> + HZ);
Consider specifying the number of msec to wait, rather than the platform
dependent HZ value that may or may not work.
I found:
#define USB_CTRL_GET_TIMEOUT 5000
#define USB_CTRL_SET_TIMEOUT 5000
They look like the right choices here.
> + if (err < 0) {
> + dev_err(&interface->dev,
> + "get: usb control message err: %d\n",
> + err);
> + }
> + *brightness = get_ctrl_message_brightness(msg_data);
Should this check the first byte that I assume is a command byte?
> + kfree(msg_data);
> + dev_dbg(&interface->dev, "get brightness: %d\n", *brightness);
> + return 0;
> +}
> +
> +int apple_bl_usb_usb_set_brightness(struct usb_interface *interface,
> + struct usb_device *usb_dev,
> + int brightness)
> +{
> + int err;
> + u16 interface_nr;
> + struct brightness_ctrl_message_data *msg_data;
> +
> + msg_data = kzalloc(sizeof(struct brightness_ctrl_message_data), GFP_KERNEL);
As above, declare it on the stack.
> + interface_nr = interface->cur_altsetting->desc.bInterfaceNumber;
> + init_ctrl_msg_data(msg_data);
> + set_ctrl_message_brightness(msg_data, brightness);
> +
> + err = usb_control_msg(usb_dev,
> + usb_sndctrlpipe(usb_dev, 0),
> + HID_SET_REPORT,
> + USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> + /* wValue: HID-Report Type and Report ID */
> + HID_REPORT_TYPE_FEATURE | 0x01,
> + interface_nr /* wIndex */,
> + msg_data,
> + sizeof(struct brightness_ctrl_message_data),
> + HZ);
> + kfree(msg_data);
> + if (err < 0) {
> + dev_err(&interface->dev,
> + "set: usb control message err: %d\n",
> + err);
> + return err;
> + }
> + dev_dbg(&interface->dev, "set brightness: %d\n", brightness);
> + return 0;
> +}
> +
> +int apple_bl_usb_check_fb(struct backlight_device *bd, struct fb_info *info)
> +{
> + dev_info(&bd->dev, "check fb\n");
> + return 0;
> +}
> +
> +int apple_bl_usb_get_brightness(struct backlight_device *bl)
> +{
> + int ret;
> + struct apple_bl_usb_data *data;
> + int hw_brightness;
> +
> + data = bl_get_data(bl);
> + ret = apple_bl_usb_usb_get_brightness(data->usb_interface,
> + data->usb_dev,
> + &hw_brightness);
> + if (!ret)
> + ret = hw_brightness;
> +
> + return ret;
> +}
> +
> +int apple_bl_usb_update_status(struct backlight_device *bl)
> +{
> + int err;
> + struct apple_bl_usb_data *data;
> +
> + data = bl_get_data(bl);
> + err = apple_bl_usb_usb_set_brightness(data->usb_interface,
> + data->usb_dev,
> + bl->props.brightness);
Here you should replace bl->props.brightness with
backlight_get_brightness(bl).
This will give you the value 0 when the backlight device is power down
or blank which is often the right thing.
> + return err;
> +}
> +
> +static const struct backlight_ops apple_bl_usb_ops = {
> + .update_status = apple_bl_usb_update_status,
> + .get_brightness = apple_bl_usb_get_brightness,
> + .check_fb = apple_bl_usb_check_fb,
> +};
> +
> +static void apple_bl_usb_disconnect(struct usb_interface *interface)
> +{
> + struct backlight_device *bl;
> +
> + dev_dbg(&interface->dev, "disconnect\n");
> +
> + bl = usb_get_intfdata(interface);
> + usb_set_intfdata(interface, NULL);
> + backlight_device_unregister(bl);
> +}
> +
> +static int apple_bl_usb_probe(struct usb_interface *interface,
> + const struct usb_device_id *id)
> +{
> + struct backlight_properties props;
> + struct backlight_device *bl;
> + struct usb_device *usb_dev;
> + struct device *dev;
> + struct apple_bl_usb_data *data;
> + int brightness_interface_nr;
> +
> + dev_dbg(&interface->dev, "probe\n");
> +
> + dev = &interface->dev;
> + usb_dev = interface_to_usbdev(interface);
> +
> + switch (usb_dev->config->desc.bConfigurationValue) {
> + case 1:
> + brightness_interface_nr = 0x7;
> + break;
> + case 2:
> + brightness_interface_nr = 0x9;
> + break;
> + case 3:
> + brightness_interface_nr = 0xc;
> + break;
> + default:
> + dev_err(dev,
> + "unexpected configuration value: %d\n",
> + usb_dev->config->desc.bConfigurationValue);
> + return -EINVAL;
Use dev_err_probe() here.
> + }
> +
> + if (interface->cur_altsetting->desc.bInterfaceNumber != brightness_interface_nr)
> + return -ENODEV;
Same here, so you also log something.
> +
> + data = devm_kzalloc(dev,
> + sizeof(struct apple_bl_usb_data),
> + GFP_KERNEL);
> + if (IS_ERR(data)) {
> + dev_err(dev, "failed to allocate memory\n");
> + return PTR_ERR(bl);
Same here.
> + }
> + data->usb_interface = interface;
> + data->usb_dev = usb_dev;
> +
> + // Valid brightness values for the apple studio display range from 400
> + // to 60000. Since the backlight subsystem´s brightness value starts
> + // from 0, we use 0 to 59600 and offset it by the minimum value.
> + memset(&props, 0, sizeof(props));
> + props.type = BACKLIGHT_RAW;
> + props.max_brightness = 59600;
> +
> + bl = backlight_device_register("apple_studio_display",
> + dev,
> + data,
> + &apple_bl_usb_ops,
> + &props);
Any particular reason NOT to use devm_backlight_device_register()?
> + if (IS_ERR(bl)) {
> + dev_err(dev, "failed to register backlight\n");
> + return PTR_ERR(bl);
> + }
> + usb_set_intfdata(interface, bl);
> + return 0;
> +}
> +
> +static int apple_bl_usb_suspend(struct usb_interface *interface,
> + pm_message_t message)
> +{
> + dev_dbg(&interface->dev, "suspend\n");
> + return 0;
> +}
> +
> +static int apple_bl_usb_resume(struct usb_interface *interface)
> +{
> + dev_dbg(&interface->dev, "resume\n");
> + return 0;
> +}
> +
> +static const struct usb_device_id id_table[] = {
> + {
> + .idVendor = APPLE_STUDIO_DISPLAY_VENDOR_ID,
> + .idProduct = APPLE_STUDIO_DISPLAY_PRODUCT_ID,
> + },
> + {},
> +};
> +MODULE_DEVICE_TABLE(usb, id_table);
> +
> +static struct usb_driver usb_asdbl_driver = {
> + .name = "apple_bl_usb",
> + .probe = apple_bl_usb_probe,
> + .disconnect = apple_bl_usb_disconnect,
> + .id_table = id_table,
> + .suspend = apple_bl_usb_suspend,
> + .resume = apple_bl_usb_resume,
> + .reset_resume = apple_bl_usb_resume
> +};
> +module_usb_driver(usb_asdbl_driver);
> +
> +MODULE_AUTHOR("Julius Zint <julius@zint.sh>");
> +MODULE_LICENSE("Dual MIT/GPL");
> +MODULE_DESCRIPTION("Backlight control for USB-C Thunderbolt Apple displays");
> --
> 2.41.0
^ permalink raw reply [flat|nested] 8+ messages in thread