* [PATCH 0/1] Backlight driver for the Apple Studio Display
@ 2023-07-01 12:08 ` Julius Zint
0 siblings, 0 replies; 16+ messages in thread
From: Julius Zint @ 2023-07-01 12:08 UTC (permalink / raw)
To: Lee Jones, Daniel Thompson, Jingoo Han
Cc: Helge Deller, linux-kernel, dri-devel, linux-fbdev, Julius Zint
I have been using and testing this as a DKMS for 6 months now without
any known issues. It bothers me, that it needs to be part of the
initramfs instead of just working out of the box. Maybe someone else
here knows, how to tell the USB HID driver, that this is not a HID device
and it should keep its fingers from it.
Julius Zint (1):
backlight: apple_bl_usb: Add Apple Studio Display support
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
--
2.41.0
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 0/1] Backlight driver for the Apple Studio Display
@ 2023-07-01 12:08 ` Julius Zint
0 siblings, 0 replies; 16+ messages in thread
From: Julius Zint @ 2023-07-01 12:08 UTC (permalink / raw)
To: Lee Jones, Daniel Thompson, Jingoo Han
Cc: Julius Zint, Helge Deller, linux-kernel, dri-devel, linux-fbdev
I have been using and testing this as a DKMS for 6 months now without
any known issues. It bothers me, that it needs to be part of the
initramfs instead of just working out of the box. Maybe someone else
here knows, how to tell the USB HID driver, that this is not a HID device
and it should keep its fingers from it.
Julius Zint (1):
backlight: apple_bl_usb: Add Apple Studio Display support
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
--
2.41.0
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/1] backlight: apple_bl_usb: Add Apple Studio Display support
2023-07-01 12:08 ` Julius Zint
@ 2023-07-01 12:08 ` Julius Zint
-1 siblings, 0 replies; 16+ messages in thread
From: Julius Zint @ 2023-07-01 12:08 UTC (permalink / raw)
To: Lee Jones, Daniel Thompson, Jingoo Han
Cc: Helge Deller, linux-kernel, dri-devel, linux-fbdev, Julius Zint
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>
---
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;
+
+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);
+ 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);
+ }
+ *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);
+ 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);
+ 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);
+ 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");
--
2.41.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 1/1] backlight: apple_bl_usb: Add Apple Studio Display support
@ 2023-07-01 12:08 ` Julius Zint
0 siblings, 0 replies; 16+ messages in thread
From: Julius Zint @ 2023-07-01 12:08 UTC (permalink / raw)
To: Lee Jones, Daniel Thompson, Jingoo Han
Cc: Julius Zint, Helge Deller, linux-kernel, dri-devel, linux-fbdev
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>
---
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;
+
+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);
+ 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);
+ }
+ *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);
+ 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);
+ 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);
+ 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");
--
2.41.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] backlight: apple_bl_usb: Add Apple Studio Display support
2023-07-01 12:08 ` Julius Zint
(?)
@ 2023-07-01 13:32 ` Rafał Miłecki
-1 siblings, 0 replies; 16+ 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] 16+ messages in thread
* Re: [PATCH 1/1] backlight: apple_bl_usb: Add Apple Studio Display support
2023-07-01 12:08 ` Julius Zint
@ 2023-07-01 14:00 ` kernel test robot
-1 siblings, 0 replies; 16+ 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] 16+ messages in thread
* Re: [PATCH 1/1] backlight: apple_bl_usb: Add Apple Studio Display support
@ 2023-07-01 14:00 ` kernel test robot
0 siblings, 0 replies; 16+ 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: linux-fbdev, Helge Deller, linux-kernel, dri-devel, Julius Zint,
oe-kbuild-all
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] 16+ messages in thread
* Re: [PATCH 1/1] backlight: apple_bl_usb: Add Apple Studio Display support
2023-07-01 12:08 ` Julius Zint
@ 2023-07-01 22:00 ` Sam Ravnborg
-1 siblings, 0 replies; 16+ 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] 16+ messages in thread
* Re: [PATCH 1/1] backlight: apple_bl_usb: Add Apple Studio Display support
@ 2023-07-01 22:00 ` Sam Ravnborg
0 siblings, 0 replies; 16+ messages in thread
From: Sam Ravnborg @ 2023-07-01 22:00 UTC (permalink / raw)
To: Julius Zint
Cc: Daniel Thompson, Jingoo Han, Helge Deller, Lee Jones,
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] 16+ messages in thread
* Re: [PATCH 0/1] Backlight driver for the Apple Studio Display
2023-07-01 12:08 ` Julius Zint
@ 2023-07-03 10:36 ` Daniel Thompson
-1 siblings, 0 replies; 16+ messages in thread
From: Daniel Thompson @ 2023-07-03 10:36 UTC (permalink / raw)
To: Julius Zint
Cc: Lee Jones, Jingoo Han, Helge Deller, linux-kernel, dri-devel,
linux-fbdev
On Sat, Jul 01, 2023 at 02:08:02PM +0200, Julius Zint wrote:
> I have been using and testing this as a DKMS for 6 months now without
> any known issues. It bothers me, that it needs to be part of the
> initramfs instead of just working out of the box. Maybe someone else
> here knows, how to tell the USB HID driver, that this is not a HID device
> and it should keep its fingers from it.
If is says it is a HID device and is uses HID reports for control then
it *is* a HID device!
In other words you need your driver to register as a HID driver instead
of sending raw HID frames using the USB stack. If you do that then the
HID core infrastructure will ensure the right driver gets loaded (it
has special logic to automatically unregister hid-generic and load the
better driver as soon as one becomes available).
Daniel.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/1] Backlight driver for the Apple Studio Display
@ 2023-07-03 10:36 ` Daniel Thompson
0 siblings, 0 replies; 16+ messages in thread
From: Daniel Thompson @ 2023-07-03 10:36 UTC (permalink / raw)
To: Julius Zint
Cc: linux-fbdev, Jingoo Han, Helge Deller, Lee Jones, linux-kernel,
dri-devel
On Sat, Jul 01, 2023 at 02:08:02PM +0200, Julius Zint wrote:
> I have been using and testing this as a DKMS for 6 months now without
> any known issues. It bothers me, that it needs to be part of the
> initramfs instead of just working out of the box. Maybe someone else
> here knows, how to tell the USB HID driver, that this is not a HID device
> and it should keep its fingers from it.
If is says it is a HID device and is uses HID reports for control then
it *is* a HID device!
In other words you need your driver to register as a HID driver instead
of sending raw HID frames using the USB stack. If you do that then the
HID core infrastructure will ensure the right driver gets loaded (it
has special logic to automatically unregister hid-generic and load the
better driver as soon as one becomes available).
Daniel.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/1] Backlight driver for the Apple Studio Display
2023-07-01 12:08 ` Julius Zint
@ 2023-07-04 18:48 ` Julius Zint
-1 siblings, 0 replies; 16+ messages in thread
From: Julius Zint @ 2023-07-04 18:48 UTC (permalink / raw)
To: Daniel Thompson, Rafał Miłecki, Sam Ravnborg
Cc: Helge Deller, linux-kernel, dri-devel, linux-fbdev, Jingoo Han,
Lee Jones
I appreciate all of the feedback, this should be plenty for a v2.
Thanks,
Julius
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/1] Backlight driver for the Apple Studio Display
@ 2023-07-04 18:48 ` Julius Zint
0 siblings, 0 replies; 16+ messages in thread
From: Julius Zint @ 2023-07-04 18:48 UTC (permalink / raw)
To: Daniel Thompson, Rafał Miłecki, Sam Ravnborg
Cc: linux-fbdev, Jingoo Han, Helge Deller, Lee Jones, linux-kernel,
dri-devel
I appreciate all of the feedback, this should be plenty for a v2.
Thanks,
Julius
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/1] Backlight driver for the Apple Studio Display
2023-11-17 20:46 Sean Aguinaga
@ 2023-11-17 22:21 ` Julius Zint
0 siblings, 0 replies; 16+ messages in thread
From: Julius Zint @ 2023-11-17 22:21 UTC (permalink / raw)
To: Sean Aguinaga
Cc: daniel.thompson, deller, dri-devel, jingoohan1, lee, linux-fbdev,
linux-kernel, sam, zajec5
On Fri, 17 Nov 2023, Sean Aguinaga wrote:
Hi,
> Did you get a chance to implement V2?
Yes, v2 is here [1] and v3 is here [2]. Currently I do have the
a few changes on top of v3 that are appended here as a patch. I use it
with DMKS and it works (mostly). I do see the userspace confusion when
the monitor is plugged in after boot.
Its still possible to set it by writing to the brightness file.
Julius
[1] https://lore.kernel.org/dri-devel/20230806091403.10002-1-julius@zint.sh/
[2] https://lore.kernel.org/dri-devel/20230820094118.20521-1-julius@zint.sh/
diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index b964a820956d..35864cc1afee 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -477,8 +477,7 @@ config BACKLIGHT_HID
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.
+ controls then say Y to enable this backlight driver.
endif # BACKLIGHT_CLASS_DEVICE
diff --git a/drivers/video/backlight/hid_bl.c b/drivers/video/backlight/hid_bl.c
index b40f8f412ee2..38b82de8224f 100644
--- a/drivers/video/backlight/hid_bl.c
+++ b/drivers/video/backlight/hid_bl.c
@@ -4,8 +4,8 @@
#include <linux/module.h>
#include <linux/backlight.h>
-#define APPLE_STUDIO_DISPLAY_VENDOR_ID 0x05ac
-#define APPLE_STUDIO_DISPLAY_PRODUCT_ID 0x1114
+#define APPLE_STUDIO_DISPLAY_VENDOR_ID 0x05ac
+#define APPLE_STUDIO_DISPLAY_PRODUCT_ID 0x1114
#define HID_USAGE_MONITOR_CTRL 0x800001
#define HID_USAGE_VESA_VCP_BRIGHTNESS 0x820010
@@ -59,7 +59,8 @@ struct hid_bl_data {
static struct hid_field *hid_get_usage_field(struct hid_device *hdev,
unsigned int report_type,
- unsigned int application, unsigned int usage)
+ unsigned int application,
+ unsigned int usage)
{
struct hid_report_enum *re = &hdev->report_enum[report_type];
struct hid_report *report;
@@ -82,18 +83,8 @@ static struct hid_field *hid_get_usage_field(struct hid_device *hdev,
static void hid_bl_remove(struct hid_device *hdev)
{
- struct backlight_device *bl;
- struct hid_bl_data *data;
-
- hid_dbg(hdev, "remove\n");
- 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)
@@ -105,7 +96,6 @@ static int hid_bl_get_brightness_raw(struct hid_bl_data *data)
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);
return result;
}
@@ -128,7 +118,6 @@ static void hid_bl_set_brightness_raw(struct hid_bl_data *data, int brightness)
*field->value = brightness;
hid_hw_request(data->hdev, field->report, HID_REQ_SET_REPORT);
hid_hw_wait(data->hdev);
- hid_dbg(data->hdev, "set brightness: %d\n", brightness);
}
static int hid_bl_update_status(struct backlight_device *bl)
@@ -157,8 +146,6 @@ static int hid_bl_probe(struct hid_device *hdev, const struct hid_device_id *id)
struct backlight_properties props;
struct backlight_device *bl;
- hid_dbg(hdev, "probe\n");
-
ret = hid_parse(hdev);
if (ret) {
hid_err(hdev, "parse failed: %d\n", ret);
@@ -203,8 +190,6 @@ static int hid_bl_probe(struct hid_device *hdev, const struct hid_device_id *id)
goto exit_stop;
}
- hid_dbg(hdev, "Monitor VESA VCP with brightness control\n");
-
ret = hid_hw_open(hdev);
if (ret) {
hid_err(hdev, "hw open failed: %d\n", ret);
@@ -214,7 +199,7 @@ static int hid_bl_probe(struct hid_device *hdev, const struct hid_device_id *id)
data = devm_kzalloc(&hdev->dev, sizeof(*data), GFP_KERNEL);
if (data == NULL) {
ret = -ENOMEM;
- goto exit_stop;
+ goto exit_close;
}
data->hdev = hdev;
data->min_brightness = input_field->logical_minimum;
@@ -233,16 +218,15 @@ static int hid_bl_probe(struct hid_device *hdev, const struct hid_device_id *id)
if (IS_ERR(bl)) {
ret = PTR_ERR(bl);
hid_err(hdev, "failed to register backlight: %d\n", ret);
- goto exit_free;
+ goto exit_close;
}
hid_set_drvdata(hdev, bl);
return 0;
-exit_free:
+exit_close:
hid_hw_close(hdev);
- devm_kfree(&hdev->dev, data);
exit_stop:
hid_hw_stop(hdev);
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 0/1] Backlight driver for the Apple Studio Display
@ 2023-11-17 22:21 ` Julius Zint
0 siblings, 0 replies; 16+ messages in thread
From: Julius Zint @ 2023-11-17 22:21 UTC (permalink / raw)
To: Sean Aguinaga
Cc: daniel.thompson, zajec5, jingoohan1, deller, lee, linux-fbdev,
dri-devel, linux-kernel, sam
On Fri, 17 Nov 2023, Sean Aguinaga wrote:
Hi,
> Did you get a chance to implement V2?
Yes, v2 is here [1] and v3 is here [2]. Currently I do have the
a few changes on top of v3 that are appended here as a patch. I use it
with DMKS and it works (mostly). I do see the userspace confusion when
the monitor is plugged in after boot.
Its still possible to set it by writing to the brightness file.
Julius
[1] https://lore.kernel.org/dri-devel/20230806091403.10002-1-julius@zint.sh/
[2] https://lore.kernel.org/dri-devel/20230820094118.20521-1-julius@zint.sh/
diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index b964a820956d..35864cc1afee 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -477,8 +477,7 @@ config BACKLIGHT_HID
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.
+ controls then say Y to enable this backlight driver.
endif # BACKLIGHT_CLASS_DEVICE
diff --git a/drivers/video/backlight/hid_bl.c b/drivers/video/backlight/hid_bl.c
index b40f8f412ee2..38b82de8224f 100644
--- a/drivers/video/backlight/hid_bl.c
+++ b/drivers/video/backlight/hid_bl.c
@@ -4,8 +4,8 @@
#include <linux/module.h>
#include <linux/backlight.h>
-#define APPLE_STUDIO_DISPLAY_VENDOR_ID 0x05ac
-#define APPLE_STUDIO_DISPLAY_PRODUCT_ID 0x1114
+#define APPLE_STUDIO_DISPLAY_VENDOR_ID 0x05ac
+#define APPLE_STUDIO_DISPLAY_PRODUCT_ID 0x1114
#define HID_USAGE_MONITOR_CTRL 0x800001
#define HID_USAGE_VESA_VCP_BRIGHTNESS 0x820010
@@ -59,7 +59,8 @@ struct hid_bl_data {
static struct hid_field *hid_get_usage_field(struct hid_device *hdev,
unsigned int report_type,
- unsigned int application, unsigned int usage)
+ unsigned int application,
+ unsigned int usage)
{
struct hid_report_enum *re = &hdev->report_enum[report_type];
struct hid_report *report;
@@ -82,18 +83,8 @@ static struct hid_field *hid_get_usage_field(struct hid_device *hdev,
static void hid_bl_remove(struct hid_device *hdev)
{
- struct backlight_device *bl;
- struct hid_bl_data *data;
-
- hid_dbg(hdev, "remove\n");
- 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)
@@ -105,7 +96,6 @@ static int hid_bl_get_brightness_raw(struct hid_bl_data *data)
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);
return result;
}
@@ -128,7 +118,6 @@ static void hid_bl_set_brightness_raw(struct hid_bl_data *data, int brightness)
*field->value = brightness;
hid_hw_request(data->hdev, field->report, HID_REQ_SET_REPORT);
hid_hw_wait(data->hdev);
- hid_dbg(data->hdev, "set brightness: %d\n", brightness);
}
static int hid_bl_update_status(struct backlight_device *bl)
@@ -157,8 +146,6 @@ static int hid_bl_probe(struct hid_device *hdev, const struct hid_device_id *id)
struct backlight_properties props;
struct backlight_device *bl;
- hid_dbg(hdev, "probe\n");
-
ret = hid_parse(hdev);
if (ret) {
hid_err(hdev, "parse failed: %d\n", ret);
@@ -203,8 +190,6 @@ static int hid_bl_probe(struct hid_device *hdev, const struct hid_device_id *id)
goto exit_stop;
}
- hid_dbg(hdev, "Monitor VESA VCP with brightness control\n");
-
ret = hid_hw_open(hdev);
if (ret) {
hid_err(hdev, "hw open failed: %d\n", ret);
@@ -214,7 +199,7 @@ static int hid_bl_probe(struct hid_device *hdev, const struct hid_device_id *id)
data = devm_kzalloc(&hdev->dev, sizeof(*data), GFP_KERNEL);
if (data == NULL) {
ret = -ENOMEM;
- goto exit_stop;
+ goto exit_close;
}
data->hdev = hdev;
data->min_brightness = input_field->logical_minimum;
@@ -233,16 +218,15 @@ static int hid_bl_probe(struct hid_device *hdev, const struct hid_device_id *id)
if (IS_ERR(bl)) {
ret = PTR_ERR(bl);
hid_err(hdev, "failed to register backlight: %d\n", ret);
- goto exit_free;
+ goto exit_close;
}
hid_set_drvdata(hdev, bl);
return 0;
-exit_free:
+exit_close:
hid_hw_close(hdev);
- devm_kfree(&hdev->dev, data);
exit_stop:
hid_hw_stop(hdev);
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 0/1] Backlight driver for the Apple Studio Display
@ 2023-11-17 20:46 Sean Aguinaga
2023-11-17 22:21 ` Julius Zint
0 siblings, 1 reply; 16+ messages in thread
From: Sean Aguinaga @ 2023-11-17 20:46 UTC (permalink / raw)
To: julius
Cc: daniel.thompson, zajec5, jingoohan1, deller, lee, linux-fbdev,
dri-devel, linux-kernel, sam
[-- Attachment #1: Type: text/plain, Size: 98 bytes --]
Did you get a chance to implement V2?
I want this in my own install! :)
Thank you
Sean Aguinaga
[-- Attachment #2: Type: text/html, Size: 2606 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2023-11-17 23:05 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-01 12:08 [PATCH 0/1] Backlight driver for the Apple Studio Display Julius Zint
2023-07-01 12:08 ` Julius Zint
2023-07-01 12:08 ` [PATCH 1/1] backlight: apple_bl_usb: Add Apple Studio Display support Julius Zint
2023-07-01 12:08 ` Julius Zint
2023-07-01 13:32 ` Rafał Miłecki
2023-07-01 14:00 ` kernel test robot
2023-07-01 14:00 ` kernel test robot
2023-07-01 22:00 ` Sam Ravnborg
2023-07-01 22:00 ` Sam Ravnborg
2023-07-03 10:36 ` [PATCH 0/1] Backlight driver for the Apple Studio Display Daniel Thompson
2023-07-03 10:36 ` Daniel Thompson
2023-07-04 18:48 ` Julius Zint
2023-07-04 18:48 ` Julius Zint
2023-11-17 20:46 Sean Aguinaga
2023-11-17 22:21 ` Julius Zint
2023-11-17 22:21 ` Julius Zint
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.