[PATCH v5 0/10] Touch Bar support for T2 Macs

[PATCH v5 0/10] Touch Bar support for T2 Macs

[Aditya Garg]

Hi Maintainers

The Touch Bars found on x86 Macs support two USB configurations: one
where the device presents itself as a HID keyboard and can display
predefined sets of keys, and one where the operating system has full
control over what is displayed.

This patch series adds support for both the configurations.

The hid-appletb-bl driver adds support for the backlight of the Touch Bar.
This enables the user to control the brightness of the Touch Bar from
userspace. The Touch Bar supports 3 modes here: Max brightness, Dim and Off.
So, daemons, used to manage Touch Bar can easily manage these modes by writing
to /sys/class/backlight/appletb_backlight/brightness. It is needed by both the
configurations of the Touch Bar.

The hid-appletb-kbd adds support for the first (predefined keys) configuration.
There are 4 modes here: Esc key only, Fn mode, Media keys and No keys.
Mode can be changed by writing to /sys/bus/hid/drivers/hid-appletb-kbd/<dev>/mode
This configuration is what Windows uses with the official Apple Bootcamp drivers.

Rest patches support the second configuration, where the OS has full control
on what's displayed on the Touch Bar. It is achieved by the patching the
hid-multitouch driver to add support for touch feedback from the Touch Bar
and the appletbdrm driver, that displays what we want to on the Touch Bar.
This configuration is what macOS uses.

The appletbdrm driver is based on the similar driver made for Windows by
imbushuo [1].

Currently, a daemon named tiny-dfr [2] is being used to display function keys
and media controls using the second configuration for both Apple Silicon and
T2 Macs.

A daemon for the first configuration is being developed, but that's a userspace
thing.

[1]: https://github.com/imbushuo/DFRDisplayKm
[2]: https://github.com/WhatAmISupposedToPutHere/tiny-dfr

v2:
1. Cleaned up some code in the hid-appletb-kbd driver.
2. Fixed wrong subject in drm/format-helper patch.
3. Fixed Co-developed-by wrongly added to hid-multitouch patch.

v3:
1. Fixed key mapping for Function keys in hid-appletb-kbd driver.

v4:
1. Added support for fn key toggle in the hid-appletb-kbd driver.

v5:
1. Do required changes to hid-appletb-bl as requested by upstream.

Aditya Garg (1):
HID: hid-appletb-kbd: add support for fn toggle between media and
  function mode

Kerem Karabay (9):
HID: hid-appletb-bl: add driver for the backlight of Apple Touch Bars
HID: hid-appletb-kbd: add driver for the keyboard mode of Apple Touch
  Bars
HID: multitouch: support getting the contact ID from
  HID_DG_TRANSDUCER_INDEX fields
HID: multitouch: support getting the tip state from HID_DG_TOUCH
  fields
HID: multitouch: take cls->maxcontacts into account for devices
  without a HID_DG_CONTACTMAX field too
HID: multitouch: allow specifying if a device is direct in a class
HID: multitouch: add device ID for Apple Touch Bars
drm/format-helper: Add conversion from XRGB8888 to BGR888 conversion
drm/tiny: add driver for Apple Touch Bars in x86 Macs

.../ABI/testing/sysfs-driver-hid-appletb-kbd  |  13 +
MAINTAINERS                                   |   6 +
drivers/gpu/drm/drm_format_helper.c           |  54 ++
.../gpu/drm/tests/drm_format_helper_test.c    |  81 +++
drivers/gpu/drm/tiny/Kconfig                  |  12 +
drivers/gpu/drm/tiny/Makefile                 |   1 +
drivers/gpu/drm/tiny/appletbdrm.c             | 624 ++++++++++++++++++
drivers/hid/Kconfig                           |  22 +
drivers/hid/Makefile                          |   2 +
drivers/hid/hid-appletb-bl.c                  | 207 ++++++
drivers/hid/hid-appletb-kbd.c                 | 432 ++++++++++++
drivers/hid/hid-multitouch.c                  |  60 +-
drivers/hid/hid-quirks.c                      |   8 +-
include/drm/drm_format_helper.h               |   3 +
14 files changed, 1509 insertions(+), 16 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-appletb-kbd
create mode 100644 drivers/gpu/drm/tiny/appletbdrm.c
create mode 100644 drivers/hid/hid-appletb-bl.c
create mode 100644 drivers/hid/hid-appletb-kbd.c

-- 
2.43.0

[PATCH v5 1/10] HID: hid-appletb-bl: add driver for the backlight of Apple Touch Bars

[Aditya Garg]

From: Kerem Karabay <kekrby@gmail.com>

This commit adds a driver for the backlight of Apple Touch Bars on x86
Macs. Note that currently only T2 Macs are supported.

This driver is based on previous work done by Ronald Tschalär
<ronald@innovation.ch>.

Signed-off-by: Kerem Karabay <kekrby@gmail.com>
Co-developed-by: Aditya Garg <gargaditya08@live.com>
Signed-off-by: Aditya Garg <gargaditya08@live.com>
---
 drivers/hid/Kconfig          |  10 ++
 drivers/hid/Makefile         |   1 +
 drivers/hid/hid-appletb-bl.c | 207 +++++++++++++++++++++++++++++++++++
 drivers/hid/hid-quirks.c     |   4 +-
 4 files changed, 221 insertions(+), 1 deletion(-)
 create mode 100644 drivers/hid/hid-appletb-bl.c

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 08446c89e..4988c1fb2 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -148,6 +148,16 @@ config HID_APPLEIR
 
 	Say Y here if you want support for Apple infrared remote control.
 
+config HID_APPLETB_BL
+	tristate "Apple Touch Bar Backlight"
+	depends on BACKLIGHT_CLASS_DEVICE
+	help
+	  Say Y here if you want support for the backlight of Touch Bars on x86
+	  MacBook Pros.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called hid-appletb-bl.
+
 config HID_ASUS
 	tristate "Asus"
 	depends on USB_HID
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index e40f1ddeb..1d825a474 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_HID_ALPS)		+= hid-alps.o
 obj-$(CONFIG_HID_ACRUX)		+= hid-axff.o
 obj-$(CONFIG_HID_APPLE)		+= hid-apple.o
 obj-$(CONFIG_HID_APPLEIR)	+= hid-appleir.o
+obj-$(CONFIG_HID_APPLETB_BL)	+= hid-appletb-bl.o
 obj-$(CONFIG_HID_CREATIVE_SB0540)	+= hid-creative-sb0540.o
 obj-$(CONFIG_HID_ASUS)		+= hid-asus.o
 obj-$(CONFIG_HID_AUREAL)	+= hid-aureal.o
diff --git a/drivers/hid/hid-appletb-bl.c b/drivers/hid/hid-appletb-bl.c
new file mode 100644
index 000000000..819157686
--- /dev/null
+++ b/drivers/hid/hid-appletb-bl.c
@@ -0,0 +1,207 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Apple Touch Bar Backlight Driver
+ *
+ * Copyright (c) 2017-2018 Ronald Tschalär
+ * Copyright (c) 2022-2023 Kerem Karabay <kekrby@gmail.com>
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/hid.h>
+#include <linux/backlight.h>
+#include <linux/device.h>
+
+#include "hid-ids.h"
+
+#define APPLETB_BL_ON			1
+#define APPLETB_BL_DIM			3
+#define APPLETB_BL_OFF			4
+
+#define HID_UP_APPLEVENDOR_TB_BL	0xff120000
+
+#define HID_VD_APPLE_TB_BRIGHTNESS	0xff120001
+#define HID_USAGE_AUX1			0xff120020
+#define HID_USAGE_BRIGHTNESS		0xff120021
+
+static int appletb_bl_def_brightness = 2;
+module_param_named(brightness, appletb_bl_def_brightness, int, 0444);
+MODULE_PARM_DESC(brightness, "Default brightness:\n"
+			 "    0 - Touchbar is off\n"
+			 "    1 - Dim brightness\n"
+			 "    [2] - Full brightness");
+
+struct appletb_bl {
+	struct hid_field *aux1_field, *brightness_field;
+	struct backlight_device *bdev;
+
+	bool full_on;
+};
+
+static const u8 appletb_bl_brightness_map[] = {
+	APPLETB_BL_OFF,
+	APPLETB_BL_DIM,
+	APPLETB_BL_ON,
+};
+
+static int appletb_bl_set_brightness(struct appletb_bl *bl, u8 brightness)
+{
+	struct hid_report *report = bl->brightness_field->report;
+	struct hid_device *hdev = report->device;
+	int ret;
+
+	ret = hid_set_field(bl->aux1_field, 0, 1);
+	if (ret) {
+		hid_err(hdev, "Failed to set auxiliary field (%pe)\n", ERR_PTR(ret));
+		return ret;
+	}
+
+	ret = hid_set_field(bl->brightness_field, 0, brightness);
+	if (ret) {
+		hid_err(hdev, "Failed to set brightness field (%pe)\n", ERR_PTR(ret));
+		return ret;
+	}
+
+	if (!bl->full_on) {
+		ret = hid_hw_power(hdev, PM_HINT_FULLON);
+		if (ret < 0) {
+			hid_err(hdev, "Device didn't power on (%pe)\n", ERR_PTR(ret));
+			return ret;
+		}
+
+		bl->full_on = true;
+	}
+
+	hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
+
+	if (brightness == APPLETB_BL_OFF) {
+		hid_hw_power(hdev, PM_HINT_NORMAL);
+		bl->full_on = false;
+	}
+
+	return 0;
+}
+
+static int appletb_bl_update_status(struct backlight_device *bdev)
+{
+	struct appletb_bl *bl = bl_get_data(bdev);
+	u8 brightness;
+
+	if (backlight_is_blank(bdev))
+		brightness = APPLETB_BL_OFF;
+	else
+		brightness = appletb_bl_brightness_map[backlight_get_brightness(bdev)];
+
+	return appletb_bl_set_brightness(bl, brightness);
+}
+
+static const struct backlight_ops appletb_bl_backlight_ops = {
+	.options = BL_CORE_SUSPENDRESUME,
+	.update_status = appletb_bl_update_status,
+};
+
+static int appletb_bl_probe(struct hid_device *hdev, const struct hid_device_id *id)
+{
+	struct hid_field *aux1_field, *brightness_field;
+	struct backlight_properties bl_props = { 0 };
+	struct device *dev = &hdev->dev;
+	struct appletb_bl *bl;
+	int ret;
+
+	ret = hid_parse(hdev);
+	if (ret)
+		return dev_err_probe(dev, ret, "HID parse failed\n");
+
+	aux1_field = hid_find_field(hdev, HID_FEATURE_REPORT,
+				    HID_VD_APPLE_TB_BRIGHTNESS, HID_USAGE_AUX1);
+
+	brightness_field = hid_find_field(hdev, HID_FEATURE_REPORT,
+					  HID_VD_APPLE_TB_BRIGHTNESS, HID_USAGE_BRIGHTNESS);
+
+	if (!aux1_field || !brightness_field)
+		return -ENODEV;
+
+	if (aux1_field->report != brightness_field->report)
+		return dev_err_probe(dev, -ENODEV, "Encountered unexpected report structure\n");
+
+	bl = devm_kzalloc(dev, sizeof(*bl), GFP_KERNEL);
+	if (!bl)
+		return -ENOMEM;
+
+	ret = hid_hw_start(hdev, HID_CONNECT_DRIVER);
+	if (ret)
+		return dev_err_probe(dev, ret, "HID hardware start failed\n");
+
+	ret = hid_hw_open(hdev);
+	if (ret) {
+		dev_err_probe(dev, ret, "HID hardware open failed\n");
+		goto stop_hw;
+	}
+
+	bl->aux1_field = aux1_field;
+	bl->brightness_field = brightness_field;
+
+	if (appletb_bl_def_brightness == 0)
+		ret = appletb_bl_set_brightness(bl, APPLETB_BL_OFF);
+	else if (appletb_bl_def_brightness == 1)
+		ret = appletb_bl_set_brightness(bl, APPLETB_BL_DIM);
+	else
+		ret = appletb_bl_set_brightness(bl, APPLETB_BL_ON);
+
+	if (ret) {
+		dev_err_probe(dev, ret, "Failed to set touch bar brightness to off\n");
+		goto close_hw;
+	}
+
+	bl_props.type = BACKLIGHT_RAW;
+	bl_props.max_brightness = ARRAY_SIZE(appletb_bl_brightness_map) - 1;
+
+	bl->bdev = devm_backlight_device_register(dev, "appletb_backlight", dev, bl,
+						  &appletb_bl_backlight_ops, &bl_props);
+	if (IS_ERR(bl->bdev)) {
+		ret = PTR_ERR(bl->bdev);
+		dev_err_probe(dev, ret, "Failed to register backlight device\n");
+		goto close_hw;
+	}
+
+	hid_set_drvdata(hdev, bl);
+
+	return 0;
+
+close_hw:
+	hid_hw_close(hdev);
+stop_hw:
+	hid_hw_stop(hdev);
+
+	return ret;
+}
+
+static void appletb_bl_remove(struct hid_device *hdev)
+{
+	struct appletb_bl *bl = hid_get_drvdata(hdev);
+
+	appletb_bl_set_brightness(bl, APPLETB_BL_OFF);
+
+	hid_hw_close(hdev);
+	hid_hw_stop(hdev);
+}
+
+static const struct hid_device_id appletb_bl_hid_ids[] = {
+	/* MacBook Pro's 2018, 2019, with T2 chip: iBridge DFR Brightness */
+	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_TOUCHBAR_BACKLIGHT) },
+	{ }
+};
+MODULE_DEVICE_TABLE(hid, appletb_bl_hid_ids);
+
+static struct hid_driver appletb_bl_hid_driver = {
+	.name = "hid-appletb-bl",
+	.id_table = appletb_bl_hid_ids,
+	.probe = appletb_bl_probe,
+	.remove = appletb_bl_remove,
+};
+module_hid_driver(appletb_bl_hid_driver);
+
+MODULE_AUTHOR("Ronald Tschalär");
+MODULE_AUTHOR("Kerem Karabay <kekrby@gmail.com>");
+MODULE_DESCRIPTION("MacBookPro Touch Bar Backlight Driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
index e0bbf0c63..818d41a35 100644
--- a/drivers/hid/hid-quirks.c
+++ b/drivers/hid/hid-quirks.c
@@ -328,7 +328,6 @@ static const struct hid_device_id hid_have_special_driver[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_GEYSER1_TP_ONLY) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MAGIC_KEYBOARD_2021) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MAGIC_KEYBOARD_FINGERPRINT_2021) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_TOUCHBAR_BACKLIGHT) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_TOUCHBAR_DISPLAY) },
 #endif
 #if IS_ENABLED(CONFIG_HID_APPLEIR)
@@ -338,6 +337,9 @@ static const struct hid_device_id hid_have_special_driver[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IRCONTROL4) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IRCONTROL5) },
 #endif
+#if IS_ENABLED(CONFIG_HID_APPLETB_BL)
+	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_TOUCHBAR_BACKLIGHT) },
+#endif
 #if IS_ENABLED(CONFIG_HID_ASUS)
 	{ HID_I2C_DEVICE(USB_VENDOR_ID_ASUSTEK, USB_DEVICE_ID_ASUSTEK_I2C_KEYBOARD) },
 	{ HID_I2C_DEVICE(USB_VENDOR_ID_ASUSTEK, USB_DEVICE_ID_ASUSTEK_I2C_TOUCHPAD) },
-- 
2.43.0

[PATCH v5 2/10] HID: hid-appletb-kbd: add driver for the keyboard mode

[Aditya Garg]

From: Kerem Karabay <kekrby@gmail.com>

The Touch Bars found on x86 Macs support two USB configurations: one
where the device presents itself as a HID keyboard and can display
predefined sets of keys, and one where the operating system has full
control over what is displayed. This commit adds a driver for the
display functionality of the first configuration.

Note that currently only T2 Macs are supported.

This driver is based on previous work done by Ronald Tschalär
<ronald@innovation.ch>.

Signed-off-by: Kerem Karabay <kekrby@gmail.com>
Co-developed-by: Aditya Garg <gargaditya08@live.com>
Signed-off-by: Aditya Garg <gargaditya08@live.com>
---
 .../ABI/testing/sysfs-driver-hid-appletb-kbd  |  13 +
 drivers/hid/Kconfig                           |  11 +
 drivers/hid/Makefile                          |   1 +
 drivers/hid/hid-appletb-kbd.c                 | 304 ++++++++++++++++++
 drivers/hid/hid-quirks.c                      |   4 +-
 5 files changed, 332 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-appletb-kbd
 create mode 100644 drivers/hid/hid-appletb-kbd.c

diff --git a/Documentation/ABI/testing/sysfs-driver-hid-appletb-kbd b/Documentation/ABI/testing/sysfs-driver-hid-appletb-kbd
new file mode 100644
index 000000000..2a19584d0
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-hid-appletb-kbd
@@ -0,0 +1,13 @@
+What:		/sys/bus/hid/drivers/hid-appletb-kbd/<dev>/mode
+Date:		September, 2023
+KernelVersion:	6.5
+Contact:	linux-input@vger.kernel.org
+Description:
+		The set of keys displayed on the Touch Bar.
+		Valid values are:
+		== =================
+		0  Escape key only
+		1  Function keys
+		2  Media/brightness keys
+		3  None
+		== =================
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 4988c1fb2..72b665eda 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -158,6 +158,17 @@ config HID_APPLETB_BL
 	  To compile this driver as a module, choose M here: the
 	  module will be called hid-appletb-bl.
 
+config HID_APPLETB_KBD
+	tristate "Apple Touch Bar Keyboard Mode"
+	depends on USB_HID
+	help
+	  Say Y here if you want support for the keyboard mode (escape,
+	  function, media and brightness keys) of Touch Bars on x86 MacBook
+	  Pros.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called hid-appletb-kbd.
+
 config HID_ASUS
 	tristate "Asus"
 	depends on USB_HID
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index 1d825a474..d903c9a26 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -30,6 +30,7 @@ obj-$(CONFIG_HID_ACRUX)		+= hid-axff.o
 obj-$(CONFIG_HID_APPLE)		+= hid-apple.o
 obj-$(CONFIG_HID_APPLEIR)	+= hid-appleir.o
 obj-$(CONFIG_HID_APPLETB_BL)	+= hid-appletb-bl.o
+obj-$(CONFIG_HID_APPLETB_KBD)	+= hid-appletb-kbd.o
 obj-$(CONFIG_HID_CREATIVE_SB0540)	+= hid-creative-sb0540.o
 obj-$(CONFIG_HID_ASUS)		+= hid-asus.o
 obj-$(CONFIG_HID_AUREAL)	+= hid-aureal.o
diff --git a/drivers/hid/hid-appletb-kbd.c b/drivers/hid/hid-appletb-kbd.c
new file mode 100644
index 000000000..ecac68fc7
--- /dev/null
+++ b/drivers/hid/hid-appletb-kbd.c
@@ -0,0 +1,304 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Apple Touch Bar Keyboard Mode Driver
+ *
+ * Copyright (c) 2017-2018 Ronald Tschalär
+ * Copyright (c) 2022-2023 Kerem Karabay <kekrby@gmail.com>
+ * Copyright (c) 2024 Aditya Garg <gargaditya08@live.com>
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/hid.h>
+#include <linux/usb.h>
+#include <linux/input.h>
+#include <linux/sysfs.h>
+#include <linux/bitops.h>
+#include <linux/module.h>
+#include <linux/string.h>
+#include <linux/input/sparse-keymap.h>
+
+#include "hid-ids.h"
+
+#define APPLETB_KBD_MODE_ESC	0
+#define APPLETB_KBD_MODE_FN	1
+#define APPLETB_KBD_MODE_SPCL	2
+#define APPLETB_KBD_MODE_OFF	3
+#define APPLETB_KBD_MODE_MAX	APPLETB_KBD_MODE_OFF
+
+#define HID_USAGE_MODE		0x00ff0004
+
+static int appletb_tb_def_mode = APPLETB_KBD_MODE_SPCL;
+module_param_named(mode, appletb_tb_def_mode, int, 0444);
+MODULE_PARM_DESC(mode, "Default touchbar mode:\n"
+			 "    0 - escape key only\n"
+			 "    1 - function-keys\n"
+			 "    [2] - special keys");
+
+struct appletb_kbd {
+	struct hid_field *mode_field;
+
+	u8 saved_mode;
+	u8 current_mode;
+};
+
+static const struct key_entry appletb_kbd_keymap[] = {
+	{ KE_KEY, KEY_ESC, { KEY_ESC } },
+	{ KE_KEY, KEY_F1,  { KEY_BRIGHTNESSDOWN } },
+	{ KE_KEY, KEY_F2,  { KEY_BRIGHTNESSUP } },
+	{ KE_KEY, KEY_F3,  { KEY_RESERVED } },
+	{ KE_KEY, KEY_F4,  { KEY_RESERVED } },
+	{ KE_KEY, KEY_F5,  { KEY_KBDILLUMDOWN } },
+	{ KE_KEY, KEY_F6,  { KEY_KBDILLUMUP } },
+	{ KE_KEY, KEY_F7,  { KEY_PREVIOUSSONG } },
+	{ KE_KEY, KEY_F8,  { KEY_PLAYPAUSE } },
+	{ KE_KEY, KEY_F9,  { KEY_NEXTSONG } },
+	{ KE_KEY, KEY_F10, { KEY_MUTE } },
+	{ KE_KEY, KEY_F11, { KEY_VOLUMEDOWN } },
+	{ KE_KEY, KEY_F12, { KEY_VOLUMEUP } },
+	{ KE_END, 0 }
+};
+
+static int appletb_kbd_set_mode(struct appletb_kbd *kbd, u8 mode)
+{
+	struct hid_report *report = kbd->mode_field->report;
+	struct hid_device *hdev = report->device;
+	int ret;
+
+	ret = hid_hw_power(hdev, PM_HINT_FULLON);
+	if (ret) {
+		hid_err(hdev, "Device didn't resume (%pe)\n", ERR_PTR(ret));
+		return ret;
+	}
+
+	ret = hid_set_field(kbd->mode_field, 0, mode);
+	if (ret) {
+		hid_err(hdev, "Failed to set mode field to %u (%pe)\n", mode, ERR_PTR(ret));
+		goto power_normal;
+	}
+
+	hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
+
+	kbd->current_mode = mode;
+
+power_normal:
+	hid_hw_power(hdev, PM_HINT_NORMAL);
+
+	return ret;
+}
+
+static ssize_t mode_show(struct device *dev,
+			 struct device_attribute *attr, char *buf)
+{
+	struct appletb_kbd *kbd = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "%d\n", kbd->current_mode);
+}
+
+static ssize_t mode_store(struct device *dev,
+			  struct device_attribute *attr,
+			  const char *buf, size_t size)
+{
+	struct appletb_kbd *kbd = dev_get_drvdata(dev);
+	u8 mode;
+	int ret;
+
+	ret = kstrtou8(buf, 0, &mode);
+	if (ret)
+		return ret;
+
+	if (mode > APPLETB_KBD_MODE_MAX)
+		return -EINVAL;
+
+	ret = appletb_kbd_set_mode(kbd, mode);
+
+	return ret < 0 ? ret : size;
+}
+static DEVICE_ATTR_RW(mode);
+
+struct attribute *appletb_kbd_attrs[] = {
+	&dev_attr_mode.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(appletb_kbd);
+
+static int appletb_tb_key_to_slot(unsigned int code)
+{
+	switch (code) {
+	case KEY_ESC:
+		return 0;
+	case KEY_F1 ... KEY_F10:
+		return code - KEY_F1 + 1;
+	case KEY_F11 ... KEY_F12:
+		return code - KEY_F11 + 11;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int appletb_kbd_hid_event(struct hid_device *hdev, struct hid_field *field,
+				      struct hid_usage *usage, __s32 value)
+{
+	struct appletb_kbd *kbd = hid_get_drvdata(hdev);
+	struct key_entry *translation;
+	struct input_dev *input;
+	int slot;
+
+	if ((usage->hid & HID_USAGE_PAGE) != HID_UP_KEYBOARD || usage->type != EV_KEY)
+		return 0;
+
+	input = field->hidinput->input;
+
+	/*
+	 * Skip non-touch-bar keys.
+	 *
+	 * Either the touch bar itself or usbhid generate a slew of key-down
+	 * events for all the meta keys. None of which we're at all interested
+	 * in.
+	 */
+	slot = appletb_tb_key_to_slot(usage->code);
+	if (slot < 0)
+		return 0;
+
+	translation = sparse_keymap_entry_from_scancode(input, usage->code);
+
+	if (translation && kbd->current_mode == APPLETB_KBD_MODE_SPCL) {
+		input_event(input, usage->type, translation->keycode, value);
+
+		return 1;
+	}
+
+	return kbd->current_mode == APPLETB_KBD_MODE_OFF;
+}
+
+static int appletb_kbd_input_configured(struct hid_device *hdev, struct hid_input *hidinput)
+{
+	int idx;
+	struct input_dev *input = hidinput->input;
+
+	/*
+	 * Clear various input capabilities that are blindly set by the hid
+	 * driver (usbkbd.c)
+	 */
+	memset(input->evbit, 0, sizeof(input->evbit));
+	memset(input->keybit, 0, sizeof(input->keybit));
+	memset(input->ledbit, 0, sizeof(input->ledbit));
+
+	__set_bit(EV_REP, input->evbit);
+
+	sparse_keymap_setup(input, appletb_kbd_keymap, NULL);
+
+	for (idx = 0; appletb_kbd_keymap[idx].type != KE_END; idx++) {
+		input_set_capability(input, EV_KEY, appletb_kbd_keymap[idx].code);
+	}
+
+	return 0;
+}
+
+static int appletb_kbd_probe(struct hid_device *hdev, const struct hid_device_id *id)
+{
+	struct appletb_kbd *kbd;
+	struct device *dev = &hdev->dev;
+	struct hid_field *mode_field;
+	int ret;
+
+	ret = hid_parse(hdev);
+	if (ret)
+		return dev_err_probe(dev, ret, "HID parse failed\n");
+
+	mode_field = hid_find_field(hdev, HID_OUTPUT_REPORT,
+				    HID_GD_KEYBOARD, HID_USAGE_MODE);
+	if (!mode_field)
+		return -ENODEV;
+
+	kbd = devm_kzalloc(dev, sizeof(*kbd), GFP_KERNEL);
+	if (!kbd)
+		return -ENOMEM;
+
+	kbd->mode_field = mode_field;
+
+	ret = hid_hw_start(hdev, HID_CONNECT_HIDINPUT);
+	if (ret)
+		return dev_err_probe(dev, ret, "HID hw start failed\n");
+
+	ret = hid_hw_open(hdev);
+	if (ret) {
+		dev_err_probe(dev, ret, "HID hw open failed\n");
+		goto stop_hw;
+	}
+
+	ret = appletb_kbd_set_mode(kbd, appletb_tb_def_mode);
+	if (ret) {
+		dev_err_probe(dev, ret, "Failed to set touchbar mode\n");
+		goto close_hw;
+	}
+
+	hid_set_drvdata(hdev, kbd);
+
+	return 0;
+
+close_hw:
+	hid_hw_close(hdev);
+stop_hw:
+	hid_hw_stop(hdev);
+	return ret;
+}
+
+static void appletb_kbd_remove(struct hid_device *hdev)
+{
+	struct appletb_kbd *kbd = hid_get_drvdata(hdev);
+
+	appletb_kbd_set_mode(kbd, APPLETB_KBD_MODE_OFF);
+
+	hid_hw_close(hdev);
+	hid_hw_stop(hdev);
+}
+
+#ifdef CONFIG_PM
+static int appletb_kbd_suspend(struct hid_device *hdev, pm_message_t msg)
+{
+	struct appletb_kbd *kbd = hid_get_drvdata(hdev);
+
+	kbd->saved_mode = kbd->current_mode;
+	appletb_kbd_set_mode(kbd, APPLETB_KBD_MODE_OFF);
+
+	return 0;
+}
+
+static int appletb_kbd_reset_resume(struct hid_device *hdev)
+{
+	struct appletb_kbd *kbd = hid_get_drvdata(hdev);
+
+	appletb_kbd_set_mode(kbd, kbd->saved_mode);
+
+	return 0;
+}
+#endif
+
+static const struct hid_device_id appletb_kbd_hid_ids[] = {
+	/* MacBook Pro's 2018, 2019, with T2 chip: iBridge Display */
+	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_TOUCHBAR_DISPLAY) },
+	{ }
+};
+MODULE_DEVICE_TABLE(hid, appletb_kbd_hid_ids);
+
+static struct hid_driver appletb_kbd_hid_driver = {
+	.name = "hid-appletb-kbd",
+	.id_table = appletb_kbd_hid_ids,
+	.probe = appletb_kbd_probe,
+	.remove = appletb_kbd_remove,
+	.event = appletb_kbd_hid_event,
+	.input_configured = appletb_kbd_input_configured,
+#ifdef CONFIG_PM
+	.suspend = appletb_kbd_suspend,
+	.reset_resume = appletb_kbd_reset_resume,
+#endif
+	.driver.dev_groups = appletb_kbd_groups,
+};
+module_hid_driver(appletb_kbd_hid_driver);
+
+MODULE_AUTHOR("Ronald Tschalär");
+MODULE_AUTHOR("Kerem Karabay <kekrby@gmail.com>");
+MODULE_DESCRIPTION("MacBookPro Touch Bar Keyboard Mode Driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
index 818d41a35..7c576d654 100644
--- a/drivers/hid/hid-quirks.c
+++ b/drivers/hid/hid-quirks.c
@@ -328,7 +328,6 @@ static const struct hid_device_id hid_have_special_driver[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_GEYSER1_TP_ONLY) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MAGIC_KEYBOARD_2021) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MAGIC_KEYBOARD_FINGERPRINT_2021) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_TOUCHBAR_DISPLAY) },
 #endif
 #if IS_ENABLED(CONFIG_HID_APPLEIR)
 	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IRCONTROL) },
@@ -340,6 +339,9 @@ static const struct hid_device_id hid_have_special_driver[] = {
 #if IS_ENABLED(CONFIG_HID_APPLETB_BL)
 	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_TOUCHBAR_BACKLIGHT) },
 #endif
+#if IS_ENABLED(CONFIG_HID_APPLETB_KBD)
+	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_TOUCHBAR_DISPLAY) },
+#endif
 #if IS_ENABLED(CONFIG_HID_ASUS)
 	{ HID_I2C_DEVICE(USB_VENDOR_ID_ASUSTEK, USB_DEVICE_ID_ASUSTEK_I2C_KEYBOARD) },
 	{ HID_I2C_DEVICE(USB_VENDOR_ID_ASUSTEK, USB_DEVICE_ID_ASUSTEK_I2C_TOUCHPAD) },
-- 
2.43.0

Re: [PATCH v5 2/10] HID: hid-appletb-kbd: add driver for the keyboard mode

[Benjamin Tissoires]

On Aug 17 2024, Aditya Garg wrote:
> From: Kerem Karabay <kekrby@gmail.com>
> 
> The Touch Bars found on x86 Macs support two USB configurations: one
> where the device presents itself as a HID keyboard and can display
> predefined sets of keys, and one where the operating system has full
> control over what is displayed. This commit adds a driver for the
> display functionality of the first configuration.
> 
> Note that currently only T2 Macs are supported.
> 
> This driver is based on previous work done by Ronald Tschalär
> <ronald@innovation.ch>.
> 
> Signed-off-by: Kerem Karabay <kekrby@gmail.com>
> Co-developed-by: Aditya Garg <gargaditya08@live.com>
> Signed-off-by: Aditya Garg <gargaditya08@live.com>
> ---
>  .../ABI/testing/sysfs-driver-hid-appletb-kbd  |  13 +
>  drivers/hid/Kconfig                           |  11 +
>  drivers/hid/Makefile                          |   1 +
>  drivers/hid/hid-appletb-kbd.c                 | 304 ++++++++++++++++++
>  drivers/hid/hid-quirks.c                      |   4 +-
>  5 files changed, 332 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-appletb-kbd
>  create mode 100644 drivers/hid/hid-appletb-kbd.c
> 
> diff --git a/Documentation/ABI/testing/sysfs-driver-hid-appletb-kbd b/Documentation/ABI/testing/sysfs-driver-hid-appletb-kbd
> new file mode 100644
> index 000000000..2a19584d0
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-hid-appletb-kbd
> @@ -0,0 +1,13 @@
> +What:		/sys/bus/hid/drivers/hid-appletb-kbd/<dev>/mode
> +Date:		September, 2023
> +KernelVersion:	6.5
> +Contact:	linux-input@vger.kernel.org
> +Description:
> +		The set of keys displayed on the Touch Bar.
> +		Valid values are:
> +		== =================
> +		0  Escape key only
> +		1  Function keys
> +		2  Media/brightness keys
> +		3  None
> +		== =================
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 4988c1fb2..72b665eda 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -158,6 +158,17 @@ config HID_APPLETB_BL
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called hid-appletb-bl.
>  
> +config HID_APPLETB_KBD
> +	tristate "Apple Touch Bar Keyboard Mode"
> +	depends on USB_HID
> +	help
> +	  Say Y here if you want support for the keyboard mode (escape,
> +	  function, media and brightness keys) of Touch Bars on x86 MacBook
> +	  Pros.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called hid-appletb-kbd.
> +
>  config HID_ASUS
>  	tristate "Asus"
>  	depends on USB_HID
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index 1d825a474..d903c9a26 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -30,6 +30,7 @@ obj-$(CONFIG_HID_ACRUX)		+= hid-axff.o
>  obj-$(CONFIG_HID_APPLE)		+= hid-apple.o
>  obj-$(CONFIG_HID_APPLEIR)	+= hid-appleir.o
>  obj-$(CONFIG_HID_APPLETB_BL)	+= hid-appletb-bl.o
> +obj-$(CONFIG_HID_APPLETB_KBD)	+= hid-appletb-kbd.o
>  obj-$(CONFIG_HID_CREATIVE_SB0540)	+= hid-creative-sb0540.o
>  obj-$(CONFIG_HID_ASUS)		+= hid-asus.o
>  obj-$(CONFIG_HID_AUREAL)	+= hid-aureal.o
> diff --git a/drivers/hid/hid-appletb-kbd.c b/drivers/hid/hid-appletb-kbd.c
> new file mode 100644
> index 000000000..ecac68fc7
> --- /dev/null
> +++ b/drivers/hid/hid-appletb-kbd.c
> @@ -0,0 +1,304 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Apple Touch Bar Keyboard Mode Driver
> + *
> + * Copyright (c) 2017-2018 Ronald Tschalär
> + * Copyright (c) 2022-2023 Kerem Karabay <kekrby@gmail.com>
> + * Copyright (c) 2024 Aditya Garg <gargaditya08@live.com>
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/hid.h>
> +#include <linux/usb.h>
> +#include <linux/input.h>
> +#include <linux/sysfs.h>
> +#include <linux/bitops.h>
> +#include <linux/module.h>
> +#include <linux/string.h>
> +#include <linux/input/sparse-keymap.h>
> +
> +#include "hid-ids.h"
> +
> +#define APPLETB_KBD_MODE_ESC	0
> +#define APPLETB_KBD_MODE_FN	1
> +#define APPLETB_KBD_MODE_SPCL	2
> +#define APPLETB_KBD_MODE_OFF	3
> +#define APPLETB_KBD_MODE_MAX	APPLETB_KBD_MODE_OFF
> +
> +#define HID_USAGE_MODE		0x00ff0004
> +
> +static int appletb_tb_def_mode = APPLETB_KBD_MODE_SPCL;
> +module_param_named(mode, appletb_tb_def_mode, int, 0444);
> +MODULE_PARM_DESC(mode, "Default touchbar mode:\n"
> +			 "    0 - escape key only\n"
> +			 "    1 - function-keys\n"
> +			 "    [2] - special keys");
> +
> +struct appletb_kbd {
> +	struct hid_field *mode_field;
> +
> +	u8 saved_mode;
> +	u8 current_mode;
> +};
> +
> +static const struct key_entry appletb_kbd_keymap[] = {
> +	{ KE_KEY, KEY_ESC, { KEY_ESC } },
> +	{ KE_KEY, KEY_F1,  { KEY_BRIGHTNESSDOWN } },
> +	{ KE_KEY, KEY_F2,  { KEY_BRIGHTNESSUP } },
> +	{ KE_KEY, KEY_F3,  { KEY_RESERVED } },
> +	{ KE_KEY, KEY_F4,  { KEY_RESERVED } },
> +	{ KE_KEY, KEY_F5,  { KEY_KBDILLUMDOWN } },
> +	{ KE_KEY, KEY_F6,  { KEY_KBDILLUMUP } },
> +	{ KE_KEY, KEY_F7,  { KEY_PREVIOUSSONG } },
> +	{ KE_KEY, KEY_F8,  { KEY_PLAYPAUSE } },
> +	{ KE_KEY, KEY_F9,  { KEY_NEXTSONG } },
> +	{ KE_KEY, KEY_F10, { KEY_MUTE } },
> +	{ KE_KEY, KEY_F11, { KEY_VOLUMEDOWN } },
> +	{ KE_KEY, KEY_F12, { KEY_VOLUMEUP } },
> +	{ KE_END, 0 }
> +};
> +
> +static int appletb_kbd_set_mode(struct appletb_kbd *kbd, u8 mode)
> +{
> +	struct hid_report *report = kbd->mode_field->report;
> +	struct hid_device *hdev = report->device;
> +	int ret;
> +
> +	ret = hid_hw_power(hdev, PM_HINT_FULLON);
> +	if (ret) {
> +		hid_err(hdev, "Device didn't resume (%pe)\n", ERR_PTR(ret));
> +		return ret;
> +	}
> +
> +	ret = hid_set_field(kbd->mode_field, 0, mode);
> +	if (ret) {
> +		hid_err(hdev, "Failed to set mode field to %u (%pe)\n", mode, ERR_PTR(ret));
> +		goto power_normal;
> +	}
> +
> +	hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
> +
> +	kbd->current_mode = mode;
> +
> +power_normal:
> +	hid_hw_power(hdev, PM_HINT_NORMAL);
> +
> +	return ret;
> +}
> +
> +static ssize_t mode_show(struct device *dev,
> +			 struct device_attribute *attr, char *buf)
> +{
> +	struct appletb_kbd *kbd = dev_get_drvdata(dev);
> +
> +	return sysfs_emit(buf, "%d\n", kbd->current_mode);
> +}
> +
> +static ssize_t mode_store(struct device *dev,
> +			  struct device_attribute *attr,
> +			  const char *buf, size_t size)
> +{
> +	struct appletb_kbd *kbd = dev_get_drvdata(dev);
> +	u8 mode;
> +	int ret;
> +
> +	ret = kstrtou8(buf, 0, &mode);
> +	if (ret)
> +		return ret;
> +
> +	if (mode > APPLETB_KBD_MODE_MAX)
> +		return -EINVAL;
> +
> +	ret = appletb_kbd_set_mode(kbd, mode);
> +
> +	return ret < 0 ? ret : size;
> +}
> +static DEVICE_ATTR_RW(mode);
> +
> +struct attribute *appletb_kbd_attrs[] = {
> +	&dev_attr_mode.attr,
> +	NULL
> +};
> +ATTRIBUTE_GROUPS(appletb_kbd);
> +
> +static int appletb_tb_key_to_slot(unsigned int code)
> +{
> +	switch (code) {
> +	case KEY_ESC:
> +		return 0;
> +	case KEY_F1 ... KEY_F10:
> +		return code - KEY_F1 + 1;
> +	case KEY_F11 ... KEY_F12:
> +		return code - KEY_F11 + 11;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int appletb_kbd_hid_event(struct hid_device *hdev, struct hid_field *field,
> +				      struct hid_usage *usage, __s32 value)
> +{
> +	struct appletb_kbd *kbd = hid_get_drvdata(hdev);
> +	struct key_entry *translation;
> +	struct input_dev *input;
> +	int slot;
> +
> +	if ((usage->hid & HID_USAGE_PAGE) != HID_UP_KEYBOARD || usage->type != EV_KEY)
> +		return 0;
> +
> +	input = field->hidinput->input;
> +
> +	/*
> +	 * Skip non-touch-bar keys.
> +	 *
> +	 * Either the touch bar itself or usbhid generate a slew of key-down
> +	 * events for all the meta keys. None of which we're at all interested
> +	 * in.
> +	 */
> +	slot = appletb_tb_key_to_slot(usage->code);
> +	if (slot < 0)
> +		return 0;
> +
> +	translation = sparse_keymap_entry_from_scancode(input, usage->code);

You are missing a Kconfig depends or select here because when enabling
this module, we can get

ERROR: modpost: "sparse_keymap_setup" [drivers/hid/hid-appletb-kbd.ko] undefined!
ERROR: modpost: "sparse_keymap_entry_from_scancode" [drivers/hid/hid-appletb-kbd.ko] undefined!

FWIW, I tried "depend on INPUT_SPARSEKMAP" and "select
INPUT_SPARSEKMAP", but in both cases the error was there, so I suspect
there is one extra config to select (could be my script but if I can
enter this state, I believe others will get into it as well).

Cheers,
Benjamin

> +
> +	if (translation && kbd->current_mode == APPLETB_KBD_MODE_SPCL) {
> +		input_event(input, usage->type, translation->keycode, value);
> +
> +		return 1;
> +	}
> +
> +	return kbd->current_mode == APPLETB_KBD_MODE_OFF;
> +}
> +
> +static int appletb_kbd_input_configured(struct hid_device *hdev, struct hid_input *hidinput)
> +{
> +	int idx;
> +	struct input_dev *input = hidinput->input;
> +
> +	/*
> +	 * Clear various input capabilities that are blindly set by the hid
> +	 * driver (usbkbd.c)
> +	 */
> +	memset(input->evbit, 0, sizeof(input->evbit));
> +	memset(input->keybit, 0, sizeof(input->keybit));
> +	memset(input->ledbit, 0, sizeof(input->ledbit));
> +
> +	__set_bit(EV_REP, input->evbit);
> +
> +	sparse_keymap_setup(input, appletb_kbd_keymap, NULL);
> +
> +	for (idx = 0; appletb_kbd_keymap[idx].type != KE_END; idx++) {
> +		input_set_capability(input, EV_KEY, appletb_kbd_keymap[idx].code);
> +	}
> +
> +	return 0;
> +}
> +
> +static int appletb_kbd_probe(struct hid_device *hdev, const struct hid_device_id *id)
> +{
> +	struct appletb_kbd *kbd;
> +	struct device *dev = &hdev->dev;
> +	struct hid_field *mode_field;
> +	int ret;
> +
> +	ret = hid_parse(hdev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "HID parse failed\n");
> +
> +	mode_field = hid_find_field(hdev, HID_OUTPUT_REPORT,
> +				    HID_GD_KEYBOARD, HID_USAGE_MODE);
> +	if (!mode_field)
> +		return -ENODEV;
> +
> +	kbd = devm_kzalloc(dev, sizeof(*kbd), GFP_KERNEL);
> +	if (!kbd)
> +		return -ENOMEM;
> +
> +	kbd->mode_field = mode_field;
> +
> +	ret = hid_hw_start(hdev, HID_CONNECT_HIDINPUT);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "HID hw start failed\n");
> +
> +	ret = hid_hw_open(hdev);
> +	if (ret) {
> +		dev_err_probe(dev, ret, "HID hw open failed\n");
> +		goto stop_hw;
> +	}
> +
> +	ret = appletb_kbd_set_mode(kbd, appletb_tb_def_mode);
> +	if (ret) {
> +		dev_err_probe(dev, ret, "Failed to set touchbar mode\n");
> +		goto close_hw;
> +	}
> +
> +	hid_set_drvdata(hdev, kbd);
> +
> +	return 0;
> +
> +close_hw:
> +	hid_hw_close(hdev);
> +stop_hw:
> +	hid_hw_stop(hdev);
> +	return ret;
> +}
> +
> +static void appletb_kbd_remove(struct hid_device *hdev)
> +{
> +	struct appletb_kbd *kbd = hid_get_drvdata(hdev);
> +
> +	appletb_kbd_set_mode(kbd, APPLETB_KBD_MODE_OFF);
> +
> +	hid_hw_close(hdev);
> +	hid_hw_stop(hdev);
> +}
> +
> +#ifdef CONFIG_PM
> +static int appletb_kbd_suspend(struct hid_device *hdev, pm_message_t msg)
> +{
> +	struct appletb_kbd *kbd = hid_get_drvdata(hdev);
> +
> +	kbd->saved_mode = kbd->current_mode;
> +	appletb_kbd_set_mode(kbd, APPLETB_KBD_MODE_OFF);
> +
> +	return 0;
> +}
> +
> +static int appletb_kbd_reset_resume(struct hid_device *hdev)
> +{
> +	struct appletb_kbd *kbd = hid_get_drvdata(hdev);
> +
> +	appletb_kbd_set_mode(kbd, kbd->saved_mode);
> +
> +	return 0;
> +}
> +#endif
> +
> +static const struct hid_device_id appletb_kbd_hid_ids[] = {
> +	/* MacBook Pro's 2018, 2019, with T2 chip: iBridge Display */
> +	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_TOUCHBAR_DISPLAY) },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(hid, appletb_kbd_hid_ids);
> +
> +static struct hid_driver appletb_kbd_hid_driver = {
> +	.name = "hid-appletb-kbd",
> +	.id_table = appletb_kbd_hid_ids,
> +	.probe = appletb_kbd_probe,
> +	.remove = appletb_kbd_remove,
> +	.event = appletb_kbd_hid_event,
> +	.input_configured = appletb_kbd_input_configured,
> +#ifdef CONFIG_PM
> +	.suspend = appletb_kbd_suspend,
> +	.reset_resume = appletb_kbd_reset_resume,
> +#endif
> +	.driver.dev_groups = appletb_kbd_groups,
> +};
> +module_hid_driver(appletb_kbd_hid_driver);
> +
> +MODULE_AUTHOR("Ronald Tschalär");
> +MODULE_AUTHOR("Kerem Karabay <kekrby@gmail.com>");
> +MODULE_DESCRIPTION("MacBookPro Touch Bar Keyboard Mode Driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
> index 818d41a35..7c576d654 100644
> --- a/drivers/hid/hid-quirks.c
> +++ b/drivers/hid/hid-quirks.c
> @@ -328,7 +328,6 @@ static const struct hid_device_id hid_have_special_driver[] = {
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_GEYSER1_TP_ONLY) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MAGIC_KEYBOARD_2021) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MAGIC_KEYBOARD_FINGERPRINT_2021) },
> -	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_TOUCHBAR_DISPLAY) },
>  #endif
>  #if IS_ENABLED(CONFIG_HID_APPLEIR)
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IRCONTROL) },
> @@ -340,6 +339,9 @@ static const struct hid_device_id hid_have_special_driver[] = {
>  #if IS_ENABLED(CONFIG_HID_APPLETB_BL)
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_TOUCHBAR_BACKLIGHT) },
>  #endif
> +#if IS_ENABLED(CONFIG_HID_APPLETB_KBD)
> +	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_TOUCHBAR_DISPLAY) },
> +#endif
>  #if IS_ENABLED(CONFIG_HID_ASUS)
>  	{ HID_I2C_DEVICE(USB_VENDOR_ID_ASUSTEK, USB_DEVICE_ID_ASUSTEK_I2C_KEYBOARD) },
>  	{ HID_I2C_DEVICE(USB_VENDOR_ID_ASUSTEK, USB_DEVICE_ID_ASUSTEK_I2C_TOUCHPAD) },
> -- 
> 2.43.0
> 

Re: [PATCH v5 2/10] HID: hid-appletb-kbd: add driver for the keyboard mode

[Aditya Garg]

> On 27 Sep 2024, at 9:09 PM, Benjamin Tissoires <bentiss@kernel.org> wrote:
> 
> On Aug 17 2024, Aditya Garg wrote:
>> From: Kerem Karabay <kekrby@gmail.com>
>> 
>> The Touch Bars found on x86 Macs support two USB configurations: one
>> where the device presents itself as a HID keyboard and can display
>> predefined sets of keys, and one where the operating system has full
>> control over what is displayed. This commit adds a driver for the
>> display functionality of the first configuration.
>> 
>> Note that currently only T2 Macs are supported.
>> 
>> This driver is based on previous work done by Ronald Tschalär
>> <ronald@innovation.ch>.
>> 
>> Signed-off-by: Kerem Karabay <kekrby@gmail.com>
>> Co-developed-by: Aditya Garg <gargaditya08@live.com>
>> Signed-off-by: Aditya Garg <gargaditya08@live.com>
>> ---
>> .../ABI/testing/sysfs-driver-hid-appletb-kbd  |  13 +
>> drivers/hid/Kconfig                           |  11 +
>> drivers/hid/Makefile                          |   1 +
>> drivers/hid/hid-appletb-kbd.c                 | 304 ++++++++++++++++++
>> drivers/hid/hid-quirks.c                      |   4 +-
>> 5 files changed, 332 insertions(+), 1 deletion(-)
>> create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-appletb-kbd
>> create mode 100644 drivers/hid/hid-appletb-kbd.c
>> 
>> diff --git a/Documentation/ABI/testing/sysfs-driver-hid-appletb-kbd b/Documentation/ABI/testing/sysfs-driver-hid-appletb-kbd
>> new file mode 100644
>> index 000000000..2a19584d0
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-driver-hid-appletb-kbd
>> @@ -0,0 +1,13 @@
>> +What:        /sys/bus/hid/drivers/hid-appletb-kbd/<dev>/mode
>> +Date:        September, 2023
>> +KernelVersion:    6.5
>> +Contact:    linux-input@vger.kernel.org
>> +Description:
>> +        The set of keys displayed on the Touch Bar.
>> +        Valid values are:
>> +        == =================
>> +        0  Escape key only
>> +        1  Function keys
>> +        2  Media/brightness keys
>> +        3  None
>> +        == =================
>> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
>> index 4988c1fb2..72b665eda 100644
>> --- a/drivers/hid/Kconfig
>> +++ b/drivers/hid/Kconfig
>> @@ -158,6 +158,17 @@ config HID_APPLETB_BL
>>      To compile this driver as a module, choose M here: the
>>      module will be called hid-appletb-bl.
>> 
>> +config HID_APPLETB_KBD
>> +    tristate "Apple Touch Bar Keyboard Mode"
>> +    depends on USB_HID
>> +    help
>> +      Say Y here if you want support for the keyboard mode (escape,
>> +      function, media and brightness keys) of Touch Bars on x86 MacBook
>> +      Pros.
>> +
>> +      To compile this driver as a module, choose M here: the
>> +      module will be called hid-appletb-kbd.
>> +
>> config HID_ASUS
>>    tristate "Asus"
>>    depends on USB_HID
>> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
>> index 1d825a474..d903c9a26 100644
>> --- a/drivers/hid/Makefile
>> +++ b/drivers/hid/Makefile
>> @@ -30,6 +30,7 @@ obj-$(CONFIG_HID_ACRUX)        += hid-axff.o
>> obj-$(CONFIG_HID_APPLE)        += hid-apple.o
>> obj-$(CONFIG_HID_APPLEIR)    += hid-appleir.o
>> obj-$(CONFIG_HID_APPLETB_BL)    += hid-appletb-bl.o
>> +obj-$(CONFIG_HID_APPLETB_KBD)    += hid-appletb-kbd.o
>> obj-$(CONFIG_HID_CREATIVE_SB0540)    += hid-creative-sb0540.o
>> obj-$(CONFIG_HID_ASUS)        += hid-asus.o
>> obj-$(CONFIG_HID_AUREAL)    += hid-aureal.o
>> diff --git a/drivers/hid/hid-appletb-kbd.c b/drivers/hid/hid-appletb-kbd.c
>> new file mode 100644
>> index 000000000..ecac68fc7
>> --- /dev/null
>> +++ b/drivers/hid/hid-appletb-kbd.c
>> @@ -0,0 +1,304 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Apple Touch Bar Keyboard Mode Driver
>> + *
>> + * Copyright (c) 2017-2018 Ronald Tschalär
>> + * Copyright (c) 2022-2023 Kerem Karabay <kekrby@gmail.com>
>> + * Copyright (c) 2024 Aditya Garg <gargaditya08@live.com>
>> + */
>> +
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> +
>> +#include <linux/hid.h>
>> +#include <linux/usb.h>
>> +#include <linux/input.h>
>> +#include <linux/sysfs.h>
>> +#include <linux/bitops.h>
>> +#include <linux/module.h>
>> +#include <linux/string.h>
>> +#include <linux/input/sparse-keymap.h>
>> +
>> +#include "hid-ids.h"
>> +
>> +#define APPLETB_KBD_MODE_ESC    0
>> +#define APPLETB_KBD_MODE_FN    1
>> +#define APPLETB_KBD_MODE_SPCL    2
>> +#define APPLETB_KBD_MODE_OFF    3
>> +#define APPLETB_KBD_MODE_MAX    APPLETB_KBD_MODE_OFF
>> +
>> +#define HID_USAGE_MODE        0x00ff0004
>> +
>> +static int appletb_tb_def_mode = APPLETB_KBD_MODE_SPCL;
>> +module_param_named(mode, appletb_tb_def_mode, int, 0444);
>> +MODULE_PARM_DESC(mode, "Default touchbar mode:\n"
>> +             "    0 - escape key only\n"
>> +             "    1 - function-keys\n"
>> +             "    [2] - special keys");
>> +
>> +struct appletb_kbd {
>> +    struct hid_field *mode_field;
>> +
>> +    u8 saved_mode;
>> +    u8 current_mode;
>> +};
>> +
>> +static const struct key_entry appletb_kbd_keymap[] = {
>> +    { KE_KEY, KEY_ESC, { KEY_ESC } },
>> +    { KE_KEY, KEY_F1,  { KEY_BRIGHTNESSDOWN } },
>> +    { KE_KEY, KEY_F2,  { KEY_BRIGHTNESSUP } },
>> +    { KE_KEY, KEY_F3,  { KEY_RESERVED } },
>> +    { KE_KEY, KEY_F4,  { KEY_RESERVED } },
>> +    { KE_KEY, KEY_F5,  { KEY_KBDILLUMDOWN } },
>> +    { KE_KEY, KEY_F6,  { KEY_KBDILLUMUP } },
>> +    { KE_KEY, KEY_F7,  { KEY_PREVIOUSSONG } },
>> +    { KE_KEY, KEY_F8,  { KEY_PLAYPAUSE } },
>> +    { KE_KEY, KEY_F9,  { KEY_NEXTSONG } },
>> +    { KE_KEY, KEY_F10, { KEY_MUTE } },
>> +    { KE_KEY, KEY_F11, { KEY_VOLUMEDOWN } },
>> +    { KE_KEY, KEY_F12, { KEY_VOLUMEUP } },
>> +    { KE_END, 0 }
>> +};
>> +
>> +static int appletb_kbd_set_mode(struct appletb_kbd *kbd, u8 mode)
>> +{
>> +    struct hid_report *report = kbd->mode_field->report;
>> +    struct hid_device *hdev = report->device;
>> +    int ret;
>> +
>> +    ret = hid_hw_power(hdev, PM_HINT_FULLON);
>> +    if (ret) {
>> +        hid_err(hdev, "Device didn't resume (%pe)\n", ERR_PTR(ret));
>> +        return ret;
>> +    }
>> +
>> +    ret = hid_set_field(kbd->mode_field, 0, mode);
>> +    if (ret) {
>> +        hid_err(hdev, "Failed to set mode field to %u (%pe)\n", mode, ERR_PTR(ret));
>> +        goto power_normal;
>> +    }
>> +
>> +    hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
>> +
>> +    kbd->current_mode = mode;
>> +
>> +power_normal:
>> +    hid_hw_power(hdev, PM_HINT_NORMAL);
>> +
>> +    return ret;
>> +}
>> +
>> +static ssize_t mode_show(struct device *dev,
>> +             struct device_attribute *attr, char *buf)
>> +{
>> +    struct appletb_kbd *kbd = dev_get_drvdata(dev);
>> +
>> +    return sysfs_emit(buf, "%d\n", kbd->current_mode);
>> +}
>> +
>> +static ssize_t mode_store(struct device *dev,
>> +              struct device_attribute *attr,
>> +              const char *buf, size_t size)
>> +{
>> +    struct appletb_kbd *kbd = dev_get_drvdata(dev);
>> +    u8 mode;
>> +    int ret;
>> +
>> +    ret = kstrtou8(buf, 0, &mode);
>> +    if (ret)
>> +        return ret;
>> +
>> +    if (mode > APPLETB_KBD_MODE_MAX)
>> +        return -EINVAL;
>> +
>> +    ret = appletb_kbd_set_mode(kbd, mode);
>> +
>> +    return ret < 0 ? ret : size;
>> +}
>> +static DEVICE_ATTR_RW(mode);
>> +
>> +struct attribute *appletb_kbd_attrs[] = {
>> +    &dev_attr_mode.attr,
>> +    NULL
>> +};
>> +ATTRIBUTE_GROUPS(appletb_kbd);
>> +
>> +static int appletb_tb_key_to_slot(unsigned int code)
>> +{
>> +    switch (code) {
>> +    case KEY_ESC:
>> +        return 0;
>> +    case KEY_F1 ... KEY_F10:
>> +        return code - KEY_F1 + 1;
>> +    case KEY_F11 ... KEY_F12:
>> +        return code - KEY_F11 + 11;
>> +
>> +    default:
>> +        return -EINVAL;
>> +    }
>> +}
>> +
>> +static int appletb_kbd_hid_event(struct hid_device *hdev, struct hid_field *field,
>> +                      struct hid_usage *usage, __s32 value)
>> +{
>> +    struct appletb_kbd *kbd = hid_get_drvdata(hdev);
>> +    struct key_entry *translation;
>> +    struct input_dev *input;
>> +    int slot;
>> +
>> +    if ((usage->hid & HID_USAGE_PAGE) != HID_UP_KEYBOARD || usage->type != EV_KEY)
>> +        return 0;
>> +
>> +    input = field->hidinput->input;
>> +
>> +    /*
>> +     * Skip non-touch-bar keys.
>> +     *
>> +     * Either the touch bar itself or usbhid generate a slew of key-down
>> +     * events for all the meta keys. None of which we're at all interested
>> +     * in.
>> +     */
>> +    slot = appletb_tb_key_to_slot(usage->code);
>> +    if (slot < 0)
>> +        return 0;
>> +
>> +    translation = sparse_keymap_entry_from_scancode(input, usage->code);
> 
> You are missing a Kconfig depends or select here because when enabling
> this module, we can get
> 
> ERROR: modpost: "sparse_keymap_setup" [drivers/hid/hid-appletb-kbd.ko] undefined!
> ERROR: modpost: "sparse_keymap_entry_from_scancode" [drivers/hid/hid-appletb-kbd.ko] undefined!
> 

Hi

It's strange that "select INPUT_SPARSEKMAP" is causing the same error.

Can you please see if the error is still there with "depends on INPUT" as well as "select INPUT_SPARSEKMAP"

Thanks
Aditya

> FWIW, I tried "depend on INPUT_SPARSEKMAP" and "select
> INPUT_SPARSEKMAP", but in both cases the error was there, so I suspect
> there is one extra config to select (could be my script but if I can
> enter this state, I believe others will get into it as well).
> Cheers,
> Benjamin
> 
>> +
>> +    if (translation && kbd->current_mode == APPLETB_KBD_MODE_SPCL) {
>> +        input_event(input, usage->type, translation->keycode, value);
>> +
>> +        return 1;
>> +    }
>> +
>> +    return kbd->current_mode == APPLETB_KBD_MODE_OFF;
>> +}
>> +
>> +static int appletb_kbd_input_configured(struct hid_device *hdev, struct hid_input *hidinput)
>> +{
>> +    int idx;
>> +    struct input_dev *input = hidinput->input;
>> +
>> +    /*
>> +     * Clear various input capabilities that are blindly set by the hid
>> +     * driver (usbkbd.c)
>> +     */
>> +    memset(input->evbit, 0, sizeof(input->evbit));
>> +    memset(input->keybit, 0, sizeof(input->keybit));
>> +    memset(input->ledbit, 0, sizeof(input->ledbit));
>> +
>> +    __set_bit(EV_REP, input->evbit);
>> +
>> +    sparse_keymap_setup(input, appletb_kbd_keymap, NULL);
>> +
>> +    for (idx = 0; appletb_kbd_keymap[idx].type != KE_END; idx++) {
>> +        input_set_capability(input, EV_KEY, appletb_kbd_keymap[idx].code);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int appletb_kbd_probe(struct hid_device *hdev, const struct hid_device_id *id)
>> +{
>> +    struct appletb_kbd *kbd;
>> +    struct device *dev = &hdev->dev;
>> +    struct hid_field *mode_field;
>> +    int ret;
>> +
>> +    ret = hid_parse(hdev);
>> +    if (ret)
>> +        return dev_err_probe(dev, ret, "HID parse failed\n");
>> +
>> +    mode_field = hid_find_field(hdev, HID_OUTPUT_REPORT,
>> +                    HID_GD_KEYBOARD, HID_USAGE_MODE);
>> +    if (!mode_field)
>> +        return -ENODEV;
>> +
>> +    kbd = devm_kzalloc(dev, sizeof(*kbd), GFP_KERNEL);
>> +    if (!kbd)
>> +        return -ENOMEM;
>> +
>> +    kbd->mode_field = mode_field;
>> +
>> +    ret = hid_hw_start(hdev, HID_CONNECT_HIDINPUT);
>> +    if (ret)
>> +        return dev_err_probe(dev, ret, "HID hw start failed\n");
>> +
>> +    ret = hid_hw_open(hdev);
>> +    if (ret) {
>> +        dev_err_probe(dev, ret, "HID hw open failed\n");
>> +        goto stop_hw;
>> +    }
>> +
>> +    ret = appletb_kbd_set_mode(kbd, appletb_tb_def_mode);
>> +    if (ret) {
>> +        dev_err_probe(dev, ret, "Failed to set touchbar mode\n");
>> +        goto close_hw;
>> +    }
>> +
>> +    hid_set_drvdata(hdev, kbd);
>> +
>> +    return 0;
>> +
>> +close_hw:
>> +    hid_hw_close(hdev);
>> +stop_hw:
>> +    hid_hw_stop(hdev);
>> +    return ret;
>> +}
>> +
>> +static void appletb_kbd_remove(struct hid_device *hdev)
>> +{
>> +    struct appletb_kbd *kbd = hid_get_drvdata(hdev);
>> +
>> +    appletb_kbd_set_mode(kbd, APPLETB_KBD_MODE_OFF);
>> +
>> +    hid_hw_close(hdev);
>> +    hid_hw_stop(hdev);
>> +}
>> +
>> +#ifdef CONFIG_PM
>> +static int appletb_kbd_suspend(struct hid_device *hdev, pm_message_t msg)
>> +{
>> +    struct appletb_kbd *kbd = hid_get_drvdata(hdev);
>> +
>> +    kbd->saved_mode = kbd->current_mode;
>> +    appletb_kbd_set_mode(kbd, APPLETB_KBD_MODE_OFF);
>> +
>> +    return 0;
>> +}
>> +
>> +static int appletb_kbd_reset_resume(struct hid_device *hdev)
>> +{
>> +    struct appletb_kbd *kbd = hid_get_drvdata(hdev);
>> +
>> +    appletb_kbd_set_mode(kbd, kbd->saved_mode);
>> +
>> +    return 0;
>> +}
>> +#endif
>> +
>> +static const struct hid_device_id appletb_kbd_hid_ids[] = {
>> +    /* MacBook Pro's 2018, 2019, with T2 chip: iBridge Display */
>> +    { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_TOUCHBAR_DISPLAY) },
>> +    { }
>> +};
>> +MODULE_DEVICE_TABLE(hid, appletb_kbd_hid_ids);
>> +
>> +static struct hid_driver appletb_kbd_hid_driver = {
>> +    .name = "hid-appletb-kbd",
>> +    .id_table = appletb_kbd_hid_ids,
>> +    .probe = appletb_kbd_probe,
>> +    .remove = appletb_kbd_remove,
>> +    .event = appletb_kbd_hid_event,
>> +    .input_configured = appletb_kbd_input_configured,
>> +#ifdef CONFIG_PM
>> +    .suspend = appletb_kbd_suspend,
>> +    .reset_resume = appletb_kbd_reset_resume,
>> +#endif
>> +    .driver.dev_groups = appletb_kbd_groups,
>> +};
>> +module_hid_driver(appletb_kbd_hid_driver);
>> +
>> +MODULE_AUTHOR("Ronald Tschalär");
>> +MODULE_AUTHOR("Kerem Karabay <kekrby@gmail.com>");
>> +MODULE_DESCRIPTION("MacBookPro Touch Bar Keyboard Mode Driver");
>> +MODULE_LICENSE("GPL");
>> diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
>> index 818d41a35..7c576d654 100644
>> --- a/drivers/hid/hid-quirks.c
>> +++ b/drivers/hid/hid-quirks.c
>> @@ -328,7 +328,6 @@ static const struct hid_device_id hid_have_special_driver[] = {
>>    { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_GEYSER1_TP_ONLY) },
>>    { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MAGIC_KEYBOARD_2021) },
>>    { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MAGIC_KEYBOARD_FINGERPRINT_2021) },
>> -    { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_TOUCHBAR_DISPLAY) },
>> #endif
>> #if IS_ENABLED(CONFIG_HID_APPLEIR)
>>    { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IRCONTROL) },
>> @@ -340,6 +339,9 @@ static const struct hid_device_id hid_have_special_driver[] = {
>> #if IS_ENABLED(CONFIG_HID_APPLETB_BL)
>>    { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_TOUCHBAR_BACKLIGHT) },
>> #endif
>> +#if IS_ENABLED(CONFIG_HID_APPLETB_KBD)
>> +    { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_TOUCHBAR_DISPLAY) },
>> +#endif
>> #if IS_ENABLED(CONFIG_HID_ASUS)
>>    { HID_I2C_DEVICE(USB_VENDOR_ID_ASUSTEK, USB_DEVICE_ID_ASUSTEK_I2C_KEYBOARD) },
>>    { HID_I2C_DEVICE(USB_VENDOR_ID_ASUSTEK, USB_DEVICE_ID_ASUSTEK_I2C_TOUCHPAD) },
>> --
>> 2.43.0
>> 

[PATCH v5 3/10] HID: hid-appletb-kbd: add support for fn toggle between media and function mode

[Aditya Garg]

From: Aditya Garg <gargaditya08@live.com>

This patch adds support for the switching between the Media and Function
keys on the touchbar by pressing the Fn key on Apple Internal Keyboard.

Signed-off-by: Aditya Garg <gargaditya08@live.com>
---
 drivers/hid/hid-appletb-kbd.c | 128 ++++++++++++++++++++++++++++++++++
 1 file changed, 128 insertions(+)

diff --git a/drivers/hid/hid-appletb-kbd.c b/drivers/hid/hid-appletb-kbd.c
index ecac68fc7..442c4d884 100644
--- a/drivers/hid/hid-appletb-kbd.c
+++ b/drivers/hid/hid-appletb-kbd.c
@@ -26,6 +26,8 @@
 #define APPLETB_KBD_MODE_OFF	3
 #define APPLETB_KBD_MODE_MAX	APPLETB_KBD_MODE_OFF
 
+#define APPLETB_DEVID_KEYBOARD	1
+
 #define HID_USAGE_MODE		0x00ff0004
 
 static int appletb_tb_def_mode = APPLETB_KBD_MODE_SPCL;
@@ -35,11 +37,18 @@ MODULE_PARM_DESC(mode, "Default touchbar mode:\n"
 			 "    1 - function-keys\n"
 			 "    [2] - special keys");
 
+static bool appletb_tb_fn_toggle = true;
+module_param_named(fntoggle, appletb_tb_fn_toggle, bool, 0644);
+MODULE_PARM_DESC(fntoggle, "Switch between Fn and media controls on pressing Fn key");
+
 struct appletb_kbd {
 	struct hid_field *mode_field;
 
 	u8 saved_mode;
 	u8 current_mode;
+	struct input_handler inp_handler;
+	struct input_handle kbd_handle;
+
 };
 
 static const struct key_entry appletb_kbd_keymap[] = {
@@ -172,6 +181,75 @@ static int appletb_kbd_hid_event(struct hid_device *hdev, struct hid_field *fiel
 	return kbd->current_mode == APPLETB_KBD_MODE_OFF;
 }
 
+static void appletb_kbd_inp_event(struct input_handle *handle, unsigned int type,
+			      unsigned int code, int value)
+{
+	struct appletb_kbd *kbd = handle->private;
+
+	if (type == EV_KEY && code == KEY_FN && appletb_tb_fn_toggle) {
+		if (value == 1) {
+			kbd->saved_mode = kbd->current_mode;
+			if (kbd->current_mode == APPLETB_KBD_MODE_SPCL)
+				appletb_kbd_set_mode(kbd, APPLETB_KBD_MODE_FN);
+			else if (kbd->current_mode == APPLETB_KBD_MODE_FN)
+				appletb_kbd_set_mode(kbd, APPLETB_KBD_MODE_SPCL);
+		} else if (value == 0) {
+			if (kbd->saved_mode != kbd->current_mode)
+				appletb_kbd_set_mode(kbd, kbd->saved_mode);
+		}
+	}
+}
+
+static int appletb_kbd_inp_connect(struct input_handler *handler,
+			       struct input_dev *dev,
+			       const struct input_device_id *id)
+{
+	struct appletb_kbd *kbd = handler->private;
+	struct input_handle *handle;
+	int rc;
+
+	if (id->driver_info == APPLETB_DEVID_KEYBOARD) {
+		handle = &kbd->kbd_handle;
+		handle->name = "tbkbd";
+	} else {
+		return -ENOENT;
+	}
+
+	if (handle->dev)
+		return -EEXIST;
+
+	handle->open = 0;
+	handle->dev = input_get_device(dev);
+	handle->handler = handler;
+	handle->private = kbd;
+
+	rc = input_register_handle(handle);
+	if (rc)
+		goto err_free_dev;
+
+	rc = input_open_device(handle);
+	if (rc)
+		goto err_unregister_handle;
+
+	return 0;
+
+ err_unregister_handle:
+	input_unregister_handle(handle);
+ err_free_dev:
+	input_put_device(handle->dev);
+	handle->dev = NULL;
+	return rc;
+}
+
+static void appletb_kbd_inp_disconnect(struct input_handle *handle)
+{
+	input_close_device(handle);
+	input_unregister_handle(handle);
+
+	input_put_device(handle->dev);
+	handle->dev = NULL;
+}
+
 static int appletb_kbd_input_configured(struct hid_device *hdev, struct hid_input *hidinput)
 {
 	int idx;
@@ -196,6 +274,40 @@ static int appletb_kbd_input_configured(struct hid_device *hdev, struct hid_inpu
 	return 0;
 }
 
+static const struct input_device_id appletb_kbd_input_devices[] = {
+	{
+		.flags = INPUT_DEVICE_ID_MATCH_BUS |
+			INPUT_DEVICE_ID_MATCH_VENDOR |
+			INPUT_DEVICE_ID_MATCH_KEYBIT,
+		.bustype = BUS_USB,
+		.vendor = USB_VENDOR_ID_APPLE,
+		.keybit = { [BIT_WORD(KEY_FN)] = BIT_MASK(KEY_FN) },
+		.driver_info = APPLETB_DEVID_KEYBOARD,
+	},
+	{ }
+};
+
+static bool appletb_kbd_match_internal_device(struct input_handler *handler,
+					  struct input_dev *inp_dev)
+{
+	struct device *dev = &inp_dev->dev;
+
+	/* in kernel: dev && !is_usb_device(dev) */
+	while (dev && !(dev->type && dev->type->name &&
+			!strcmp(dev->type->name, "usb_device")))
+		dev = dev->parent;
+
+	/*
+	 * Apple labels all their internal keyboards and trackpads as such,
+	 * instead of maintaining an ever expanding list of product-id's we
+	 * just look at the device's product name.
+	 */
+	if (dev)
+		return !!strstr(to_usb_device(dev)->product, "Internal Keyboard");
+
+	return false;
+}
+
 static int appletb_kbd_probe(struct hid_device *hdev, const struct hid_device_id *id)
 {
 	struct appletb_kbd *kbd;
@@ -228,6 +340,20 @@ static int appletb_kbd_probe(struct hid_device *hdev, const struct hid_device_id
 		goto stop_hw;
 	}
 
+	kbd->inp_handler.event = appletb_kbd_inp_event;
+	kbd->inp_handler.connect = appletb_kbd_inp_connect;
+	kbd->inp_handler.disconnect = appletb_kbd_inp_disconnect;
+	kbd->inp_handler.name = "appletb";
+	kbd->inp_handler.id_table = appletb_kbd_input_devices;
+	kbd->inp_handler.match = appletb_kbd_match_internal_device;
+	kbd->inp_handler.private = kbd;
+
+	ret = input_register_handler(&kbd->inp_handler);
+	if (ret) {
+		dev_err_probe(dev, ret, "Unable to register keyboard handler\n");
+		goto close_hw;
+	}
+
 	ret = appletb_kbd_set_mode(kbd, appletb_tb_def_mode);
 	if (ret) {
 		dev_err_probe(dev, ret, "Failed to set touchbar mode\n");
@@ -251,6 +377,8 @@ static void appletb_kbd_remove(struct hid_device *hdev)
 
 	appletb_kbd_set_mode(kbd, APPLETB_KBD_MODE_OFF);
 
+	input_unregister_handler(&kbd->inp_handler);
+
 	hid_hw_close(hdev);
 	hid_hw_stop(hdev);
 }
-- 
2.43.0

[PATCH v5 4/10] HID: multitouch: support getting the contact ID from HID_DG_TRANSDUCER_INDEX fields

[Aditya Garg]

From: Kerem Karabay <kekrby@gmail.com>

This is needed to support Apple Touch Bars, where the contact ID is
contained in fields with the HID_DG_TRANSDUCER_INDEX usage.

Signed-off-by: Kerem Karabay <kekrby@gmail.com>
Signed-off-by: Aditya Garg <gargaditya08@live.com>
---
 drivers/hid/hid-multitouch.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 56fc78841..3e92789ed 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -635,7 +635,9 @@ static struct mt_report_data *mt_allocate_report_data(struct mt_device *td,
 
 		if (field->logical == HID_DG_FINGER || td->hdev->group != HID_GROUP_MULTITOUCH_WIN_8) {
 			for (n = 0; n < field->report_count; n++) {
-				if (field->usage[n].hid == HID_DG_CONTACTID) {
+				unsigned int hid = field->usage[n].hid;
+
+				if (hid == HID_DG_CONTACTID || hid == HID_DG_TRANSDUCER_INDEX) {
 					rdata->is_mt_collection = true;
 					break;
 				}
@@ -814,6 +816,7 @@ static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 			MT_STORE_FIELD(tip_state);
 			return 1;
 		case HID_DG_CONTACTID:
+		case HID_DG_TRANSDUCER_INDEX:
 			MT_STORE_FIELD(contactid);
 			app->touches_by_report++;
 			return 1;
-- 
2.43.0

Re: [PATCH v5 4/10] HID: multitouch: support getting the contact ID from HID_DG_TRANSDUCER_INDEX fields

[Benjamin Tissoires]

On Aug 17 2024, Aditya Garg wrote:
> From: Kerem Karabay <kekrby@gmail.com>
> 
> This is needed to support Apple Touch Bars, where the contact ID is
> contained in fields with the HID_DG_TRANSDUCER_INDEX usage.
> 
> Signed-off-by: Kerem Karabay <kekrby@gmail.com>
> Signed-off-by: Aditya Garg <gargaditya08@live.com>
> ---
>  drivers/hid/hid-multitouch.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 56fc78841..3e92789ed 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -635,7 +635,9 @@ static struct mt_report_data *mt_allocate_report_data(struct mt_device *td,
>  
>  		if (field->logical == HID_DG_FINGER || td->hdev->group != HID_GROUP_MULTITOUCH_WIN_8) {
>  			for (n = 0; n < field->report_count; n++) {
> -				if (field->usage[n].hid == HID_DG_CONTACTID) {
> +				unsigned int hid = field->usage[n].hid;
> +
> +				if (hid == HID_DG_CONTACTID || hid == HID_DG_TRANSDUCER_INDEX) {

I'm not super happy about this. The HID spec specifically mentions
CONTACTID to be related to multitouch, when TRANSDUCER is not
specifically for multitouch. I would rather have this hidden behind a
quirk, because I don't think this is standard (the device comes from an
environment where both the hardware and the software stack is
controlled, which already gave some fun decisions from Apple).

Cheers,
Benjamin

>  					rdata->is_mt_collection = true;
>  					break;
>  				}
> @@ -814,6 +816,7 @@ static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>  			MT_STORE_FIELD(tip_state);
>  			return 1;
>  		case HID_DG_CONTACTID:
> +		case HID_DG_TRANSDUCER_INDEX:
>  			MT_STORE_FIELD(contactid);
>  			app->touches_by_report++;
>  			return 1;
> -- 
> 2.43.0
> 

[PATCH v5 5/10] HID: multitouch: support getting the tip state from HID_DG_TOUCH fields

[Aditya Garg]

From: Kerem Karabay <kekrby@gmail.com>

This is necessary on Apple Touch Bars, where the tip state is contained
in fields with the HID_DG_TOUCH usage. This feature is gated by a quirk
in order to prevent breaking other devices, see commit c2ef8f21ea8f
("HID: multitouch: add support for trackpads").

Signed-off-by: Kerem Karabay <kekrby@gmail.com>
Signed-off-by: Aditya Garg <gargaditya08@live.com>
---
 drivers/hid/hid-multitouch.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 3e92789ed..571435da5 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -72,6 +72,7 @@ MODULE_LICENSE("GPL");
 #define MT_QUIRK_FORCE_MULTI_INPUT	BIT(20)
 #define MT_QUIRK_DISABLE_WAKEUP		BIT(21)
 #define MT_QUIRK_ORIENTATION_INVERT	BIT(22)
+#define MT_QUIRK_TOUCH_IS_TIPSTATE	BIT(23)
 
 #define MT_INPUTMODE_TOUCHSCREEN	0x02
 #define MT_INPUTMODE_TOUCHPAD		0x03
@@ -809,6 +810,15 @@ static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 
 			MT_STORE_FIELD(confidence_state);
 			return 1;
+		case HID_DG_TOUCH:
+			/*
+			 * Legacy devices use TIPSWITCH and not TOUCH.
+			 * Let's just ignore this field unless the quirk is set.
+			 */
+			if (!(cls->quirks & MT_QUIRK_TOUCH_IS_TIPSTATE))
+				return -1;
+
+			fallthrough;
 		case HID_DG_TIPSWITCH:
 			if (field->application != HID_GD_SYSTEM_MULTIAXIS)
 				input_set_capability(hi->input,
@@ -872,10 +882,6 @@ static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 		case HID_DG_CONTACTMAX:
 			/* contact max are global to the report */
 			return -1;
-		case HID_DG_TOUCH:
-			/* Legacy devices use TIPSWITCH and not TOUCH.
-			 * Let's just ignore this field. */
-			return -1;
 		}
 		/* let hid-input decide for the others */
 		return 0;
-- 
2.43.0

[PATCH v5 6/10] HID: multitouch: take cls->maxcontacts into account for devices without a HID_DG_CONTACTMAX field too

[Aditya Garg]

From: Kerem Karabay <kekrby@gmail.com>

This is needed for Apple Touch Bars, where no HID_DG_CONTACTMAX field is
present and the maximum contact count is greater than the default.

Signed-off-by: Kerem Karabay <kekrby@gmail.com>
Signed-off-by: Aditya Garg <gargaditya08@live.com>
---
 drivers/hid/hid-multitouch.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 571435da5..60b675cd1 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -490,9 +490,6 @@ static void mt_feature_mapping(struct hid_device *hdev,
 		if (!td->maxcontacts &&
 		    field->logical_maximum <= MT_MAX_MAXCONTACT)
 			td->maxcontacts = field->logical_maximum;
-		if (td->mtclass.maxcontacts)
-			/* check if the maxcontacts is given by the class */
-			td->maxcontacts = td->mtclass.maxcontacts;
 
 		break;
 	case HID_DG_BUTTONTYPE:
@@ -1309,6 +1306,10 @@ static int mt_touch_input_configured(struct hid_device *hdev,
 	struct input_dev *input = hi->input;
 	int ret;
 
+	/* check if the maxcontacts is given by the class */
+	if (cls->maxcontacts)
+		td->maxcontacts = cls->maxcontacts;
+
 	if (!td->maxcontacts)
 		td->maxcontacts = MT_DEFAULT_MAXCONTACT;
 
-- 
2.43.0

[PATCH v5 7/10] HID: multitouch: allow specifying if a device is direct in a class

[Aditya Garg]

From: Kerem Karabay <kekrby@gmail.com>

Currently the driver determines the device type based on the
application, but this value is not reliable on Apple Touch Bars, where
the application is HID_DG_TOUCHPAD even though the devices are direct,
so allow setting it in classes.

Signed-off-by: Kerem Karabay <kekrby@gmail.com>
Signed-off-by: Aditya Garg <gargaditya08@live.com>
---
 drivers/hid/hid-multitouch.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 60b675cd1..2948fbcbc 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -146,6 +146,7 @@ struct mt_class {
 	__s32 sn_height;	/* Signal/noise ratio for height events */
 	__s32 sn_pressure;	/* Signal/noise ratio for pressure events */
 	__u8 maxcontacts;
+	bool is_direct;	/* true for touchscreens */
 	bool is_indirect;	/* true for touchpads */
 	bool export_all_inputs;	/* do not ignore mouse, keyboards, etc... */
 };
@@ -563,13 +564,13 @@ static struct mt_application *mt_allocate_application(struct mt_device *td,
 	mt_application->application = application;
 	INIT_LIST_HEAD(&mt_application->mt_usages);
 
-	if (application == HID_DG_TOUCHSCREEN)
+	if (application == HID_DG_TOUCHSCREEN && !td->mtclass.is_indirect)
 		mt_application->mt_flags |= INPUT_MT_DIRECT;
 
 	/*
 	 * Model touchscreens providing buttons as touchpads.
 	 */
-	if (application == HID_DG_TOUCHPAD) {
+	if (application == HID_DG_TOUCHPAD && !td->mtclass.is_direct) {
 		mt_application->mt_flags |= INPUT_MT_POINTER;
 		td->inputmode_value = MT_INPUTMODE_TOUCHPAD;
 	}
@@ -1317,6 +1318,9 @@ static int mt_touch_input_configured(struct hid_device *hdev,
 	if (td->serial_maybe)
 		mt_post_parse_default_settings(td, app);
 
+	if (cls->is_direct)
+		app->mt_flags |= INPUT_MT_DIRECT;
+
 	if (cls->is_indirect)
 		app->mt_flags |= INPUT_MT_POINTER;
 
-- 
2.43.0

Re: [PATCH v5 7/10] HID: multitouch: allow specifying if a device is direct in a class

[Benjamin Tissoires]

On Aug 17 2024, Aditya Garg wrote:
> From: Kerem Karabay <kekrby@gmail.com>
> 
> Currently the driver determines the device type based on the
> application, but this value is not reliable on Apple Touch Bars, where
> the application is HID_DG_TOUCHPAD even though the devices are direct,
> so allow setting it in classes.
> 
> Signed-off-by: Kerem Karabay <kekrby@gmail.com>
> Signed-off-by: Aditya Garg <gargaditya08@live.com>

So that one commit breaks the HID selftests.

For a reproducer:

./tools/testing/selftests/hid/vmtest.sh -- \
  "pip3 install hid-tools attrs pyroute2; \
   pytest -v tools/testing/selftests/hid/tests/ --udevd -k Topseed"

In summary: Topseed devices are weird, and I guess this patch makes them
weirder. Please ensure you do not regress on any known devices (so by
removing '-k Topseed' once this is fixed).

Cheers,
Benjamin

> ---
>  drivers/hid/hid-multitouch.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 60b675cd1..2948fbcbc 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -146,6 +146,7 @@ struct mt_class {
>  	__s32 sn_height;	/* Signal/noise ratio for height events */
>  	__s32 sn_pressure;	/* Signal/noise ratio for pressure events */
>  	__u8 maxcontacts;
> +	bool is_direct;	/* true for touchscreens */
>  	bool is_indirect;	/* true for touchpads */
>  	bool export_all_inputs;	/* do not ignore mouse, keyboards, etc... */
>  };
> @@ -563,13 +564,13 @@ static struct mt_application *mt_allocate_application(struct mt_device *td,
>  	mt_application->application = application;
>  	INIT_LIST_HEAD(&mt_application->mt_usages);
>  
> -	if (application == HID_DG_TOUCHSCREEN)
> +	if (application == HID_DG_TOUCHSCREEN && !td->mtclass.is_indirect)
>  		mt_application->mt_flags |= INPUT_MT_DIRECT;
>  
>  	/*
>  	 * Model touchscreens providing buttons as touchpads.
>  	 */
> -	if (application == HID_DG_TOUCHPAD) {
> +	if (application == HID_DG_TOUCHPAD && !td->mtclass.is_direct) {
>  		mt_application->mt_flags |= INPUT_MT_POINTER;
>  		td->inputmode_value = MT_INPUTMODE_TOUCHPAD;
>  	}
> @@ -1317,6 +1318,9 @@ static int mt_touch_input_configured(struct hid_device *hdev,
>  	if (td->serial_maybe)
>  		mt_post_parse_default_settings(td, app);
>  
> +	if (cls->is_direct)
> +		app->mt_flags |= INPUT_MT_DIRECT;
> +
>  	if (cls->is_indirect)
>  		app->mt_flags |= INPUT_MT_POINTER;
>  
> -- 
> 2.43.0
> 

[PATCH v5 8/10] HID: multitouch: add device ID for Apple Touch Bars

[Aditya Garg]

From: Kerem Karabay <kekrby@gmail.com>

Note that this is device ID is for T2 Macs. Testing on T1 Macs would be
appreciated.

Signed-off-by: Kerem Karabay <kekrby@gmail.com>
Signed-off-by: Aditya Garg <gargaditya08@live.com>
---
 drivers/hid/Kconfig          |  1 +
 drivers/hid/hid-multitouch.c | 26 ++++++++++++++++++++++----
 2 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 72b665eda..35ef5d4ef 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -744,6 +744,7 @@ config HID_MULTITOUCH
 	  Say Y here if you have one of the following devices:
 	  - 3M PCT touch screens
 	  - ActionStar dual touch panels
+	  - Touch Bars on x86 MacBook Pros
 	  - Atmel panels
 	  - Cando dual touch panels
 	  - Chunghwa panels
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 2948fbcbc..0fed95536 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -214,6 +214,7 @@ static void mt_post_parse(struct mt_device *td, struct mt_application *app);
 #define MT_CLS_GOOGLE				0x0111
 #define MT_CLS_RAZER_BLADE_STEALTH		0x0112
 #define MT_CLS_SMART_TECH			0x0113
+#define MT_CLS_APPLE_TOUCHBAR			0x0114
 
 #define MT_DEFAULT_MAXCONTACT	10
 #define MT_MAX_MAXCONTACT	250
@@ -398,6 +399,13 @@ static const struct mt_class mt_classes[] = {
 			MT_QUIRK_CONTACT_CNT_ACCURATE |
 			MT_QUIRK_SEPARATE_APP_REPORT,
 	},
+	{ .name = MT_CLS_APPLE_TOUCHBAR,
+		.quirks = MT_QUIRK_HOVERING |
+			MT_QUIRK_TOUCH_IS_TIPSTATE |
+			MT_QUIRK_SLOT_IS_CONTACTID_MINUS_ONE,
+		.is_direct = true,
+		.maxcontacts = 11,
+	},
 	{ }
 };
 
@@ -1747,6 +1755,15 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		}
 	}
 
+	ret = hid_parse(hdev);
+	if (ret != 0)
+		return ret;
+
+	if (mtclass->name == MT_CLS_APPLE_TOUCHBAR &&
+	    !hid_find_field(hdev, HID_INPUT_REPORT,
+			    HID_DG_TOUCHPAD, HID_DG_TRANSDUCER_INDEX))
+		return -ENODEV;
+
 	td = devm_kzalloc(&hdev->dev, sizeof(struct mt_device), GFP_KERNEL);
 	if (!td) {
 		dev_err(&hdev->dev, "cannot allocate multitouch data\n");
@@ -1794,10 +1811,6 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 
 	timer_setup(&td->release_timer, mt_expired_timeout, 0);
 
-	ret = hid_parse(hdev);
-	if (ret != 0)
-		return ret;
-
 	if (mtclass->quirks & MT_QUIRK_FIX_CONST_CONTACT_ID)
 		mt_fix_const_fields(hdev, HID_DG_CONTACTID);
 
@@ -2249,6 +2262,11 @@ static const struct hid_device_id mt_devices[] = {
 		MT_USB_DEVICE(USB_VENDOR_ID_XIROKU,
 			USB_DEVICE_ID_XIROKU_CSR2) },
 
+	/* Apple Touch Bars */
+	{ .driver_data = MT_CLS_APPLE_TOUCHBAR,
+		HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
+			       USB_DEVICE_ID_APPLE_TOUCHBAR_DISPLAY) },
+
 	/* Google MT devices */
 	{ .driver_data = MT_CLS_GOOGLE,
 		HID_DEVICE(HID_BUS_ANY, HID_GROUP_ANY, USB_VENDOR_ID_GOOGLE,
-- 
2.43.0

[PATCH v5 9/10] drm/format-helper: Add conversion from XRGB8888 to BGR888

[Aditya Garg]

From: Kerem Karabay <kekrby@gmail.com>

Add XRGB8888 emulation helper for devices that only support BGR888.

Signed-off-by: Kerem Karabay <kekrby@gmail.com>
Signed-off-by: Aditya Garg <gargaditya08@live.com>
---
 drivers/gpu/drm/drm_format_helper.c           | 54 +++++++++++++
 .../gpu/drm/tests/drm_format_helper_test.c    | 81 +++++++++++++++++++
 include/drm/drm_format_helper.h               |  3 +
 3 files changed, 138 insertions(+)

diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
index b1be458ed..28c0e76a1 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -702,6 +702,57 @@ void drm_fb_xrgb8888_to_rgb888(struct iosys_map *dst, const unsigned int *dst_pi
 }
 EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb888);
 
+static void drm_fb_xrgb8888_to_bgr888_line(void *dbuf, const void *sbuf, unsigned int pixels)
+{
+	u8 *dbuf8 = dbuf;
+	const __le32 *sbuf32 = sbuf;
+	unsigned int x;
+	u32 pix;
+
+	for (x = 0; x < pixels; x++) {
+		pix = le32_to_cpu(sbuf32[x]);
+		/* write red-green-blue to output in little endianness */
+		*dbuf8++ = (pix & 0x00FF0000) >> 16;
+		*dbuf8++ = (pix & 0x0000FF00) >> 8;
+		*dbuf8++ = (pix & 0x000000FF) >> 0;
+	}
+}
+
+/**
+ * drm_fb_xrgb8888_to_bgr888 - Convert XRGB8888 to BGR888 clip buffer
+ * @dst: Array of BGR888 destination buffers
+ * @dst_pitch: Array of numbers of bytes between the start of two consecutive scanlines
+ *             within @dst; can be NULL if scanlines are stored next to each other.
+ * @src: Array of XRGB8888 source buffers
+ * @fb: DRM framebuffer
+ * @clip: Clip rectangle area to copy
+ * @state: Transform and conversion state
+ *
+ * This function copies parts of a framebuffer to display memory and converts the
+ * color format during the process. Destination and framebuffer formats must match. The
+ * parameters @dst, @dst_pitch and @src refer to arrays. Each array must have at
+ * least as many entries as there are planes in @fb's format. Each entry stores the
+ * value for the format's respective color plane at the same index.
+ *
+ * This function does not apply clipping on @dst (i.e. the destination is at the
+ * top-left corner).
+ *
+ * Drivers can use this function for BGR888 devices that don't natively
+ * support XRGB8888.
+ */
+void drm_fb_xrgb8888_to_bgr888(struct iosys_map *dst, const unsigned int *dst_pitch,
+			       const struct iosys_map *src, const struct drm_framebuffer *fb,
+			       const struct drm_rect *clip, struct drm_format_conv_state *state)
+{
+	static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
+		3,
+	};
+
+	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false, state,
+		    drm_fb_xrgb8888_to_bgr888_line);
+}
+EXPORT_SYMBOL(drm_fb_xrgb8888_to_bgr888);
+
 static void drm_fb_xrgb8888_to_argb8888_line(void *dbuf, const void *sbuf, unsigned int pixels)
 {
 	__le32 *dbuf32 = dbuf;
@@ -1035,6 +1086,9 @@ int drm_fb_blit(struct iosys_map *dst, const unsigned int *dst_pitch, uint32_t d
 		} else if (dst_format == DRM_FORMAT_RGB888) {
 			drm_fb_xrgb8888_to_rgb888(dst, dst_pitch, src, fb, clip, state);
 			return 0;
+		} else if (dst_format == DRM_FORMAT_BGR888) {
+			drm_fb_xrgb8888_to_bgr888(dst, dst_pitch, src, fb, clip, state);
+			return 0;
 		} else if (dst_format == DRM_FORMAT_ARGB8888) {
 			drm_fb_xrgb8888_to_argb8888(dst, dst_pitch, src, fb, clip, state);
 			return 0;
diff --git a/drivers/gpu/drm/tests/drm_format_helper_test.c b/drivers/gpu/drm/tests/drm_format_helper_test.c
index 08992636e..35cd3405d 100644
--- a/drivers/gpu/drm/tests/drm_format_helper_test.c
+++ b/drivers/gpu/drm/tests/drm_format_helper_test.c
@@ -60,6 +60,11 @@ struct convert_to_rgb888_result {
 	const u8 expected[TEST_BUF_SIZE];
 };
 
+struct convert_to_bgr888_result {
+	unsigned int dst_pitch;
+	const u8 expected[TEST_BUF_SIZE];
+};
+
 struct convert_to_argb8888_result {
 	unsigned int dst_pitch;
 	const u32 expected[TEST_BUF_SIZE];
@@ -107,6 +112,7 @@ struct convert_xrgb8888_case {
 	struct convert_to_argb1555_result argb1555_result;
 	struct convert_to_rgba5551_result rgba5551_result;
 	struct convert_to_rgb888_result rgb888_result;
+	struct convert_to_bgr888_result bgr888_result;
 	struct convert_to_argb8888_result argb8888_result;
 	struct convert_to_xrgb2101010_result xrgb2101010_result;
 	struct convert_to_argb2101010_result argb2101010_result;
@@ -151,6 +157,10 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
 			.dst_pitch = TEST_USE_DEFAULT_PITCH,
 			.expected = { 0x00, 0x00, 0xFF },
 		},
+		.bgr888_result = {
+			.dst_pitch = TEST_USE_DEFAULT_PITCH,
+			.expected = { 0xFF, 0x00, 0x00 },
+		},
 		.argb8888_result = {
 			.dst_pitch = TEST_USE_DEFAULT_PITCH,
 			.expected = { 0xFFFF0000 },
@@ -217,6 +227,10 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
 			.dst_pitch = TEST_USE_DEFAULT_PITCH,
 			.expected = { 0x00, 0x00, 0xFF },
 		},
+		.bgr888_result = {
+			.dst_pitch = TEST_USE_DEFAULT_PITCH,
+			.expected = { 0xFF, 0x00, 0x00 },
+		},
 		.argb8888_result = {
 			.dst_pitch = TEST_USE_DEFAULT_PITCH,
 			.expected = { 0xFFFF0000 },
@@ -330,6 +344,15 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
 				0x00, 0xFF, 0xFF, 0xFF, 0xFF, 0x00,
 			},
 		},
+		.bgr888_result = {
+			.dst_pitch = TEST_USE_DEFAULT_PITCH,
+			.expected = {
+				0xFF, 0xFF, 0xFF, 0x00, 0x00, 0x00,
+				0xFF, 0x00, 0x00, 0x00, 0xFF, 0x00,
+				0x00, 0x00, 0xFF, 0xFF, 0x00, 0xFF,
+				0xFF, 0xFF, 0x00, 0x00, 0xFF, 0xFF,
+			},
+		},
 		.argb8888_result = {
 			.dst_pitch = TEST_USE_DEFAULT_PITCH,
 			.expected = {
@@ -468,6 +491,17 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
 				0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
 			},
 		},
+		.bgr888_result = {
+			.dst_pitch = 15,
+			.expected = {
+				0x0E, 0x44, 0x9C, 0x11, 0x4D, 0x05, 0xA8, 0xF3, 0x03,
+				0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+				0x6C, 0xF0, 0x73, 0x0E, 0x44, 0x9C, 0x11, 0x4D, 0x05,
+				0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+				0xA8, 0x03, 0x03, 0x6C, 0xF0, 0x73, 0x0E, 0x44, 0x9C,
+				0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+			},
+		},
 		.argb8888_result = {
 			.dst_pitch = 20,
 			.expected = {
@@ -914,6 +948,52 @@ static void drm_test_fb_xrgb8888_to_rgb888(struct kunit *test)
 	KUNIT_EXPECT_MEMEQ(test, buf, result->expected, dst_size);
 }
 
+static void drm_test_fb_xrgb8888_to_bgr888(struct kunit *test)
+{
+	const struct convert_xrgb8888_case *params = test->param_value;
+	const struct convert_to_bgr888_result *result = &params->bgr888_result;
+	size_t dst_size;
+	u8 *buf = NULL;
+	__le32 *xrgb8888 = NULL;
+	struct iosys_map dst, src;
+
+	struct drm_framebuffer fb = {
+		.format = drm_format_info(DRM_FORMAT_XRGB8888),
+		.pitches = { params->pitch, 0, 0 },
+	};
+
+	dst_size = conversion_buf_size(DRM_FORMAT_BGR888, result->dst_pitch,
+				       &params->clip, 0);
+	KUNIT_ASSERT_GT(test, dst_size, 0);
+
+	buf = kunit_kzalloc(test, dst_size, GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, buf);
+	iosys_map_set_vaddr(&dst, buf);
+
+	xrgb8888 = cpubuf_to_le32(test, params->xrgb8888, TEST_BUF_SIZE);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, xrgb8888);
+	iosys_map_set_vaddr(&src, xrgb8888);
+
+	/*
+	 * BGR888 expected results are already in little-endian
+	 * order, so there's no need to convert the test output.
+	 */
+	drm_fb_xrgb8888_to_bgr888(&dst, &result->dst_pitch, &src, &fb, &params->clip,
+				  &fmtcnv_state);
+	KUNIT_EXPECT_MEMEQ(test, buf, result->expected, dst_size);
+
+	buf = dst.vaddr; /* restore original value of buf */
+	memset(buf, 0, dst_size);
+
+	int blit_result = 0;
+
+	blit_result = drm_fb_blit(&dst, &result->dst_pitch, DRM_FORMAT_BGR888, &src, &fb, &params->clip,
+				  &fmtcnv_state);
+
+	KUNIT_EXPECT_FALSE(test, blit_result);
+	KUNIT_EXPECT_MEMEQ(test, buf, result->expected, dst_size);
+}
+
 static void drm_test_fb_xrgb8888_to_argb8888(struct kunit *test)
 {
 	const struct convert_xrgb8888_case *params = test->param_value;
@@ -1851,6 +1931,7 @@ static struct kunit_case drm_format_helper_test_cases[] = {
 	KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_argb1555, convert_xrgb8888_gen_params),
 	KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_rgba5551, convert_xrgb8888_gen_params),
 	KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_rgb888, convert_xrgb8888_gen_params),
+	KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_bgr888, convert_xrgb8888_gen_params),
 	KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_argb8888, convert_xrgb8888_gen_params),
 	KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_xrgb2101010, convert_xrgb8888_gen_params),
 	KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_argb2101010, convert_xrgb8888_gen_params),
diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
index 428d81afe..aa1604d92 100644
--- a/include/drm/drm_format_helper.h
+++ b/include/drm/drm_format_helper.h
@@ -96,6 +96,9 @@ void drm_fb_xrgb8888_to_rgba5551(struct iosys_map *dst, const unsigned int *dst_
 void drm_fb_xrgb8888_to_rgb888(struct iosys_map *dst, const unsigned int *dst_pitch,
 			       const struct iosys_map *src, const struct drm_framebuffer *fb,
 			       const struct drm_rect *clip, struct drm_format_conv_state *state);
+void drm_fb_xrgb8888_to_bgr888(struct iosys_map *dst, const unsigned int *dst_pitch,
+			       const struct iosys_map *src, const struct drm_framebuffer *fb,
+			       const struct drm_rect *clip, struct drm_format_conv_state *state);
 void drm_fb_xrgb8888_to_argb8888(struct iosys_map *dst, const unsigned int *dst_pitch,
 				 const struct iosys_map *src, const struct drm_framebuffer *fb,
 				 const struct drm_rect *clip, struct drm_format_conv_state *state);
-- 
2.43.0

Re: [PATCH v5 9/10] drm/format-helper: Add conversion from XRGB8888 to BGR888

[Thomas Zimmermann]

Hi

Am 17.08.24 um 13:51 schrieb Aditya Garg:
> From: Kerem Karabay <kekrby@gmail.com>
>
> Add XRGB8888 emulation helper for devices that only support BGR888.
>
> Signed-off-by: Kerem Karabay <kekrby@gmail.com>
> Signed-off-by: Aditya Garg <gargaditya08@live.com>
> ---
>   drivers/gpu/drm/drm_format_helper.c           | 54 +++++++++++++
>   .../gpu/drm/tests/drm_format_helper_test.c    | 81 +++++++++++++++++++
>   include/drm/drm_format_helper.h               |  3 +
>   3 files changed, 138 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
> index b1be458ed..28c0e76a1 100644
> --- a/drivers/gpu/drm/drm_format_helper.c
> +++ b/drivers/gpu/drm/drm_format_helper.c
> @@ -702,6 +702,57 @@ void drm_fb_xrgb8888_to_rgb888(struct iosys_map *dst, const unsigned int *dst_pi
>   }
>   EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb888);
>   
> +static void drm_fb_xrgb8888_to_bgr888_line(void *dbuf, const void *sbuf, unsigned int pixels)
> +{
> +	u8 *dbuf8 = dbuf;
> +	const __le32 *sbuf32 = sbuf;
> +	unsigned int x;
> +	u32 pix;
> +
> +	for (x = 0; x < pixels; x++) {
> +		pix = le32_to_cpu(sbuf32[x]);
> +		/* write red-green-blue to output in little endianness */
> +		*dbuf8++ = (pix & 0x00FF0000) >> 16;
> +		*dbuf8++ = (pix & 0x0000FF00) >> 8;
> +		*dbuf8++ = (pix & 0x000000FF) >> 0;

I know the file is inconsistent, but I'd prefer lower-case numbers 
(0xff) here. Apart from that

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

Best regards
Thomas

> +	}
> +}
> +
> +/**
> + * drm_fb_xrgb8888_to_bgr888 - Convert XRGB8888 to BGR888 clip buffer
> + * @dst: Array of BGR888 destination buffers
> + * @dst_pitch: Array of numbers of bytes between the start of two consecutive scanlines
> + *             within @dst; can be NULL if scanlines are stored next to each other.
> + * @src: Array of XRGB8888 source buffers
> + * @fb: DRM framebuffer
> + * @clip: Clip rectangle area to copy
> + * @state: Transform and conversion state
> + *
> + * This function copies parts of a framebuffer to display memory and converts the
> + * color format during the process. Destination and framebuffer formats must match. The
> + * parameters @dst, @dst_pitch and @src refer to arrays. Each array must have at
> + * least as many entries as there are planes in @fb's format. Each entry stores the
> + * value for the format's respective color plane at the same index.
> + *
> + * This function does not apply clipping on @dst (i.e. the destination is at the
> + * top-left corner).
> + *
> + * Drivers can use this function for BGR888 devices that don't natively
> + * support XRGB8888.
> + */
> +void drm_fb_xrgb8888_to_bgr888(struct iosys_map *dst, const unsigned int *dst_pitch,
> +			       const struct iosys_map *src, const struct drm_framebuffer *fb,
> +			       const struct drm_rect *clip, struct drm_format_conv_state *state)
> +{
> +	static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
> +		3,
> +	};
> +
> +	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false, state,
> +		    drm_fb_xrgb8888_to_bgr888_line);
> +}
> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_bgr888);
> +
>   static void drm_fb_xrgb8888_to_argb8888_line(void *dbuf, const void *sbuf, unsigned int pixels)
>   {
>   	__le32 *dbuf32 = dbuf;
> @@ -1035,6 +1086,9 @@ int drm_fb_blit(struct iosys_map *dst, const unsigned int *dst_pitch, uint32_t d
>   		} else if (dst_format == DRM_FORMAT_RGB888) {
>   			drm_fb_xrgb8888_to_rgb888(dst, dst_pitch, src, fb, clip, state);
>   			return 0;
> +		} else if (dst_format == DRM_FORMAT_BGR888) {
> +			drm_fb_xrgb8888_to_bgr888(dst, dst_pitch, src, fb, clip, state);
> +			return 0;
>   		} else if (dst_format == DRM_FORMAT_ARGB8888) {
>   			drm_fb_xrgb8888_to_argb8888(dst, dst_pitch, src, fb, clip, state);
>   			return 0;
> diff --git a/drivers/gpu/drm/tests/drm_format_helper_test.c b/drivers/gpu/drm/tests/drm_format_helper_test.c
> index 08992636e..35cd3405d 100644
> --- a/drivers/gpu/drm/tests/drm_format_helper_test.c
> +++ b/drivers/gpu/drm/tests/drm_format_helper_test.c
> @@ -60,6 +60,11 @@ struct convert_to_rgb888_result {
>   	const u8 expected[TEST_BUF_SIZE];
>   };
>   
> +struct convert_to_bgr888_result {
> +	unsigned int dst_pitch;
> +	const u8 expected[TEST_BUF_SIZE];
> +};
> +
>   struct convert_to_argb8888_result {
>   	unsigned int dst_pitch;
>   	const u32 expected[TEST_BUF_SIZE];
> @@ -107,6 +112,7 @@ struct convert_xrgb8888_case {
>   	struct convert_to_argb1555_result argb1555_result;
>   	struct convert_to_rgba5551_result rgba5551_result;
>   	struct convert_to_rgb888_result rgb888_result;
> +	struct convert_to_bgr888_result bgr888_result;
>   	struct convert_to_argb8888_result argb8888_result;
>   	struct convert_to_xrgb2101010_result xrgb2101010_result;
>   	struct convert_to_argb2101010_result argb2101010_result;
> @@ -151,6 +157,10 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
>   			.dst_pitch = TEST_USE_DEFAULT_PITCH,
>   			.expected = { 0x00, 0x00, 0xFF },
>   		},
> +		.bgr888_result = {
> +			.dst_pitch = TEST_USE_DEFAULT_PITCH,
> +			.expected = { 0xFF, 0x00, 0x00 },
> +		},
>   		.argb8888_result = {
>   			.dst_pitch = TEST_USE_DEFAULT_PITCH,
>   			.expected = { 0xFFFF0000 },
> @@ -217,6 +227,10 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
>   			.dst_pitch = TEST_USE_DEFAULT_PITCH,
>   			.expected = { 0x00, 0x00, 0xFF },
>   		},
> +		.bgr888_result = {
> +			.dst_pitch = TEST_USE_DEFAULT_PITCH,
> +			.expected = { 0xFF, 0x00, 0x00 },
> +		},
>   		.argb8888_result = {
>   			.dst_pitch = TEST_USE_DEFAULT_PITCH,
>   			.expected = { 0xFFFF0000 },
> @@ -330,6 +344,15 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
>   				0x00, 0xFF, 0xFF, 0xFF, 0xFF, 0x00,
>   			},
>   		},
> +		.bgr888_result = {
> +			.dst_pitch = TEST_USE_DEFAULT_PITCH,
> +			.expected = {
> +				0xFF, 0xFF, 0xFF, 0x00, 0x00, 0x00,
> +				0xFF, 0x00, 0x00, 0x00, 0xFF, 0x00,
> +				0x00, 0x00, 0xFF, 0xFF, 0x00, 0xFF,
> +				0xFF, 0xFF, 0x00, 0x00, 0xFF, 0xFF,
> +			},
> +		},
>   		.argb8888_result = {
>   			.dst_pitch = TEST_USE_DEFAULT_PITCH,
>   			.expected = {
> @@ -468,6 +491,17 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
>   				0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
>   			},
>   		},
> +		.bgr888_result = {
> +			.dst_pitch = 15,
> +			.expected = {
> +				0x0E, 0x44, 0x9C, 0x11, 0x4D, 0x05, 0xA8, 0xF3, 0x03,
> +				0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +				0x6C, 0xF0, 0x73, 0x0E, 0x44, 0x9C, 0x11, 0x4D, 0x05,
> +				0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +				0xA8, 0x03, 0x03, 0x6C, 0xF0, 0x73, 0x0E, 0x44, 0x9C,
> +				0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +			},
> +		},
>   		.argb8888_result = {
>   			.dst_pitch = 20,
>   			.expected = {
> @@ -914,6 +948,52 @@ static void drm_test_fb_xrgb8888_to_rgb888(struct kunit *test)
>   	KUNIT_EXPECT_MEMEQ(test, buf, result->expected, dst_size);
>   }
>   
> +static void drm_test_fb_xrgb8888_to_bgr888(struct kunit *test)
> +{
> +	const struct convert_xrgb8888_case *params = test->param_value;
> +	const struct convert_to_bgr888_result *result = &params->bgr888_result;
> +	size_t dst_size;
> +	u8 *buf = NULL;
> +	__le32 *xrgb8888 = NULL;
> +	struct iosys_map dst, src;
> +
> +	struct drm_framebuffer fb = {
> +		.format = drm_format_info(DRM_FORMAT_XRGB8888),
> +		.pitches = { params->pitch, 0, 0 },
> +	};
> +
> +	dst_size = conversion_buf_size(DRM_FORMAT_BGR888, result->dst_pitch,
> +				       &params->clip, 0);
> +	KUNIT_ASSERT_GT(test, dst_size, 0);
> +
> +	buf = kunit_kzalloc(test, dst_size, GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, buf);
> +	iosys_map_set_vaddr(&dst, buf);
> +
> +	xrgb8888 = cpubuf_to_le32(test, params->xrgb8888, TEST_BUF_SIZE);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, xrgb8888);
> +	iosys_map_set_vaddr(&src, xrgb8888);
> +
> +	/*
> +	 * BGR888 expected results are already in little-endian
> +	 * order, so there's no need to convert the test output.
> +	 */
> +	drm_fb_xrgb8888_to_bgr888(&dst, &result->dst_pitch, &src, &fb, &params->clip,
> +				  &fmtcnv_state);
> +	KUNIT_EXPECT_MEMEQ(test, buf, result->expected, dst_size);
> +
> +	buf = dst.vaddr; /* restore original value of buf */
> +	memset(buf, 0, dst_size);
> +
> +	int blit_result = 0;
> +
> +	blit_result = drm_fb_blit(&dst, &result->dst_pitch, DRM_FORMAT_BGR888, &src, &fb, &params->clip,
> +				  &fmtcnv_state);
> +
> +	KUNIT_EXPECT_FALSE(test, blit_result);
> +	KUNIT_EXPECT_MEMEQ(test, buf, result->expected, dst_size);
> +}
> +
>   static void drm_test_fb_xrgb8888_to_argb8888(struct kunit *test)
>   {
>   	const struct convert_xrgb8888_case *params = test->param_value;
> @@ -1851,6 +1931,7 @@ static struct kunit_case drm_format_helper_test_cases[] = {
>   	KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_argb1555, convert_xrgb8888_gen_params),
>   	KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_rgba5551, convert_xrgb8888_gen_params),
>   	KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_rgb888, convert_xrgb8888_gen_params),
> +	KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_bgr888, convert_xrgb8888_gen_params),
>   	KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_argb8888, convert_xrgb8888_gen_params),
>   	KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_xrgb2101010, convert_xrgb8888_gen_params),
>   	KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_argb2101010, convert_xrgb8888_gen_params),
> diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
> index 428d81afe..aa1604d92 100644
> --- a/include/drm/drm_format_helper.h
> +++ b/include/drm/drm_format_helper.h
> @@ -96,6 +96,9 @@ void drm_fb_xrgb8888_to_rgba5551(struct iosys_map *dst, const unsigned int *dst_
>   void drm_fb_xrgb8888_to_rgb888(struct iosys_map *dst, const unsigned int *dst_pitch,
>   			       const struct iosys_map *src, const struct drm_framebuffer *fb,
>   			       const struct drm_rect *clip, struct drm_format_conv_state *state);
> +void drm_fb_xrgb8888_to_bgr888(struct iosys_map *dst, const unsigned int *dst_pitch,
> +			       const struct iosys_map *src, const struct drm_framebuffer *fb,
> +			       const struct drm_rect *clip, struct drm_format_conv_state *state);
>   void drm_fb_xrgb8888_to_argb8888(struct iosys_map *dst, const unsigned int *dst_pitch,
>   				 const struct iosys_map *src, const struct drm_framebuffer *fb,
>   				 const struct drm_rect *clip, struct drm_format_conv_state *state);

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[PATCH v5 10/10] drm/tiny: add driver for Apple Touch Bars in x86 Macs

[Aditya Garg]

From: Kerem Karabay <kekrby@gmail.com>

The Touch Bars found on x86 Macs support two USB configurations: one
where the device presents itself as a HID keyboard and can display
predefined sets of keys, and one where the operating system has full
control over what is displayed. This commit adds support for the display
functionality of the second configuration.

Note that this driver has only been tested on T2 Macs, and only includes
the USB device ID for these devices. Testing on T1 Macs would be
appreciated.

Credit goes to @imbushuo on GitHub for reverse engineering most of the
protocol.

Signed-off-by: Kerem Karabay <kekrby@gmail.com>
Signed-off-by: Aditya Garg <gargaditya08@live.com>
---
 MAINTAINERS                       |   6 +
 drivers/gpu/drm/tiny/Kconfig      |  12 +
 drivers/gpu/drm/tiny/Makefile     |   1 +
 drivers/gpu/drm/tiny/appletbdrm.c | 624 ++++++++++++++++++++++++++++++
 4 files changed, 643 insertions(+)
 create mode 100644 drivers/gpu/drm/tiny/appletbdrm.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 8766f3e5e..2665e6c57 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6889,6 +6889,12 @@ S:	Supported
 T:	git https://gitlab.freedesktop.org/drm/misc/kernel.git
 F:	drivers/gpu/drm/sun4i/sun8i*
 
+DRM DRIVER FOR APPLE TOUCH BARS
+M:	Kerem Karabay <kekrby@gmail.com>
+L:	dri-devel@lists.freedesktop.org
+S:	Maintained
+F:	drivers/gpu/drm/tiny/appletbdrm.c
+
 DRM DRIVER FOR ARM PL111 CLCD
 S:	Orphan
 T:	git https://gitlab.freedesktop.org/drm/misc/kernel.git
diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
index f6889f649..559a97bce 100644
--- a/drivers/gpu/drm/tiny/Kconfig
+++ b/drivers/gpu/drm/tiny/Kconfig
@@ -1,5 +1,17 @@
 # SPDX-License-Identifier: GPL-2.0-only
 
+config DRM_APPLETBDRM
+	tristate "DRM support for Apple Touch Bars"
+	depends on DRM && USB && MMU
+	select DRM_KMS_HELPER
+	select DRM_GEM_SHMEM_HELPER
+	help
+	  Say Y here if you want support for the display of Touch Bars on x86
+	  MacBook Pros.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called appletbdrm.
+
 config DRM_ARCPGU
 	tristate "ARC PGU"
 	depends on DRM && OF
diff --git a/drivers/gpu/drm/tiny/Makefile b/drivers/gpu/drm/tiny/Makefile
index 76dde89a0..9a1b412e7 100644
--- a/drivers/gpu/drm/tiny/Makefile
+++ b/drivers/gpu/drm/tiny/Makefile
@@ -1,5 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0-only
 
+obj-$(CONFIG_DRM_APPLETBDRM)		+= appletbdrm.o
 obj-$(CONFIG_DRM_ARCPGU)		+= arcpgu.o
 obj-$(CONFIG_DRM_BOCHS)			+= bochs.o
 obj-$(CONFIG_DRM_CIRRUS_QEMU)		+= cirrus.o
diff --git a/drivers/gpu/drm/tiny/appletbdrm.c b/drivers/gpu/drm/tiny/appletbdrm.c
new file mode 100644
index 000000000..b9440ce00
--- /dev/null
+++ b/drivers/gpu/drm/tiny/appletbdrm.c
@@ -0,0 +1,624 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Apple Touch Bar DRM Driver
+ *
+ * Copyright (c) 2023 Kerem Karabay <kekrby@gmail.com>
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <asm/unaligned.h>
+
+#include <linux/usb.h>
+#include <linux/module.h>
+
+#include <drm/drm_drv.h>
+#include <drm/drm_fourcc.h>
+#include <drm/drm_probe_helper.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_damage_helper.h>
+#include <drm/drm_format_helper.h>
+#include <drm/drm_gem_shmem_helper.h>
+#include <drm/drm_gem_atomic_helper.h>
+#include <drm/drm_simple_kms_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>
+
+#define _APPLETBDRM_FOURCC(s)		(((s)[0] << 24) | ((s)[1] << 16) | ((s)[2] << 8) | (s)[3])
+#define APPLETBDRM_FOURCC(s)		_APPLETBDRM_FOURCC(#s)
+
+#define APPLETBDRM_PIXEL_FORMAT		APPLETBDRM_FOURCC(RGBA) /* The actual format is BGR888 */
+#define APPLETBDRM_BITS_PER_PIXEL	24
+
+#define APPLETBDRM_MSG_CLEAR_DISPLAY	APPLETBDRM_FOURCC(CLRD)
+#define APPLETBDRM_MSG_GET_INFORMATION	APPLETBDRM_FOURCC(GINF)
+#define APPLETBDRM_MSG_UPDATE_COMPLETE	APPLETBDRM_FOURCC(UDCL)
+#define APPLETBDRM_MSG_SIGNAL_READINESS	APPLETBDRM_FOURCC(REDY)
+
+#define APPLETBDRM_BULK_MSG_TIMEOUT	1000
+
+#define drm_to_adev(_drm)		container_of(_drm, struct appletbdrm_device, drm)
+#define adev_to_udev(adev)		interface_to_usbdev(to_usb_interface(adev->dev))
+
+struct appletbdrm_device {
+	struct device *dev;
+
+	u8 in_ep;
+	u8 out_ep;
+
+	u32 width;
+	u32 height;
+
+	struct drm_device drm;
+	struct drm_display_mode mode;
+	struct drm_connector connector;
+	struct drm_simple_display_pipe pipe;
+
+	bool readiness_signal_received;
+};
+
+struct appletbdrm_request_header {
+	__le16 unk_00;
+	__le16 unk_02;
+	__le32 unk_04;
+	__le32 unk_08;
+	__le32 size;
+} __packed;
+
+struct appletbdrm_response_header {
+	u8 unk_00[16];
+	u32 msg;
+} __packed;
+
+struct appletbdrm_simple_request {
+	struct appletbdrm_request_header header;
+	u32 msg;
+	u8 unk_14[8];
+	__le32 size;
+} __packed;
+
+struct appletbdrm_information {
+	struct appletbdrm_response_header header;
+	u8 unk_14[12];
+	__le32 width;
+	__le32 height;
+	u8 bits_per_pixel;
+	__le32 bytes_per_row;
+	__le32 orientation;
+	__le32 bitmap_info;
+	u32 pixel_format;
+	__le32 width_inches;	/* floating point */
+	__le32 height_inches;	/* floating point */
+} __packed;
+
+struct appletbdrm_frame {
+	__le16 begin_x;
+	__le16 begin_y;
+	__le16 width;
+	__le16 height;
+	__le32 buf_size;
+	u8 buf[];
+} __packed;
+
+struct appletbdrm_fb_request_footer {
+	u8 unk_00[12];
+	__le32 unk_0c;
+	u8 unk_10[12];
+	__le32 unk_1c;
+	__le64 timestamp;
+	u8 unk_28[12];
+	__le32 unk_34;
+	u8 unk_38[20];
+	__le32 unk_4c;
+} __packed;
+
+struct appletbdrm_fb_request {
+	struct appletbdrm_request_header header;
+	__le16 unk_10;
+	u8 msg_id;
+	u8 unk_13[29];
+	/*
+	 * Contents of `data`:
+	 * - struct appletbdrm_frame frames[];
+	 * - struct appletbdrm_fb_request_footer footer;
+	 * - padding to make the total size a multiple of 16
+	 */
+	u8 data[];
+} __packed;
+
+struct appletbdrm_fb_request_response {
+	struct appletbdrm_response_header header;
+	u8 unk_14[12];
+	__le64 timestamp;
+} __packed;
+
+static int appletbdrm_send_request(struct appletbdrm_device *adev,
+				   struct appletbdrm_request_header *request, size_t size)
+{
+	struct usb_device *udev = adev_to_udev(adev);
+	struct drm_device *drm = &adev->drm;
+	int ret, actual_size;
+
+	ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, adev->out_ep),
+			   request, size, &actual_size, APPLETBDRM_BULK_MSG_TIMEOUT);
+	if (ret) {
+		drm_err(drm, "Failed to send message (%pe)\n", ERR_PTR(ret));
+		return ret;
+	}
+
+	if (actual_size != size) {
+		drm_err(drm, "Actual size (%d) doesn't match expected size (%lu)\n",
+			actual_size, size);
+		return -EIO;
+	}
+
+	return ret;
+}
+
+static int appletbdrm_read_response(struct appletbdrm_device *adev,
+				    struct appletbdrm_response_header *response,
+				    size_t size, u32 expected_response)
+{
+	struct usb_device *udev = adev_to_udev(adev);
+	struct drm_device *drm = &adev->drm;
+	int ret, actual_size;
+
+retry:
+	ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, adev->in_ep),
+			   response, size, &actual_size, APPLETBDRM_BULK_MSG_TIMEOUT);
+	if (ret) {
+		drm_err(drm, "Failed to read response (%pe)\n", ERR_PTR(ret));
+		return ret;
+	}
+
+	/*
+	 * The device responds to the first request sent in a particular
+	 * timeframe after the USB device configuration is set with a readiness
+	 * signal, in which case the response should be read again
+	 */
+	if (response->msg == APPLETBDRM_MSG_SIGNAL_READINESS) {
+		if (!adev->readiness_signal_received) {
+			adev->readiness_signal_received = true;
+			goto retry;
+		}
+
+		drm_err(drm, "Encountered unexpected readiness signal\n");
+		return -EIO;
+	}
+
+	if (actual_size != size) {
+		drm_err(drm, "Actual size (%d) doesn't match expected size (%lu)\n",
+			actual_size, size);
+		return -EIO;
+	}
+
+	if (response->msg != expected_response) {
+		drm_err(drm, "Unexpected response from device (expected %p4ch found %p4ch)\n",
+			&expected_response, &response->msg);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int appletbdrm_send_msg(struct appletbdrm_device *adev, u32 msg)
+{
+	struct appletbdrm_simple_request *request;
+	int ret;
+
+	request = kzalloc(sizeof(*request), GFP_KERNEL);
+	if (!request)
+		return -ENOMEM;
+
+	request->header.unk_00 = cpu_to_le16(2);
+	request->header.unk_02 = cpu_to_le16(0x1512);
+	request->header.size = cpu_to_le32(sizeof(*request) - sizeof(request->header));
+	request->msg = msg;
+	request->size = request->header.size;
+
+	ret = appletbdrm_send_request(adev, &request->header, sizeof(*request));
+
+	kfree(request);
+
+	return ret;
+}
+
+static int appletbdrm_clear_display(struct appletbdrm_device *adev)
+{
+	return appletbdrm_send_msg(adev, APPLETBDRM_MSG_CLEAR_DISPLAY);
+}
+
+static int appletbdrm_signal_readiness(struct appletbdrm_device *adev)
+{
+	return appletbdrm_send_msg(adev, APPLETBDRM_MSG_SIGNAL_READINESS);
+}
+
+static int appletbdrm_get_information(struct appletbdrm_device *adev)
+{
+	struct appletbdrm_information *info;
+	struct drm_device *drm = &adev->drm;
+	u8 bits_per_pixel;
+	u32 pixel_format;
+	int ret;
+
+	info = kzalloc(sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+
+	ret = appletbdrm_send_msg(adev, APPLETBDRM_MSG_GET_INFORMATION);
+	if (ret)
+		return ret;
+
+	ret = appletbdrm_read_response(adev, &info->header, sizeof(*info),
+				       APPLETBDRM_MSG_GET_INFORMATION);
+	if (ret)
+		goto free_info;
+
+	bits_per_pixel = info->bits_per_pixel;
+	pixel_format = get_unaligned(&info->pixel_format);
+
+	adev->width = get_unaligned_le32(&info->width);
+	adev->height = get_unaligned_le32(&info->height);
+
+	if (bits_per_pixel != APPLETBDRM_BITS_PER_PIXEL) {
+		drm_err(drm, "Encountered unexpected bits per pixel value (%d)\n", bits_per_pixel);
+		ret = -EINVAL;
+		goto free_info;
+	}
+
+	if (pixel_format != APPLETBDRM_PIXEL_FORMAT) {
+		drm_err(drm, "Encountered unknown pixel format (%p4ch)\n", &pixel_format);
+		ret = -EINVAL;
+		goto free_info;
+	}
+
+free_info:
+	kfree(info);
+
+	return ret;
+}
+
+static u32 rect_size(struct drm_rect *rect)
+{
+	return drm_rect_width(rect) * drm_rect_height(rect) * (APPLETBDRM_BITS_PER_PIXEL / 8);
+}
+
+static int appletbdrm_flush_damage(struct appletbdrm_device *adev,
+				   struct drm_plane_state *old_state,
+				   struct drm_plane_state *state)
+{
+	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(state);
+	struct appletbdrm_fb_request_response *response;
+	struct appletbdrm_fb_request_footer *footer;
+	struct drm_atomic_helper_damage_iter iter;
+	struct drm_framebuffer *fb = state->fb;
+	struct appletbdrm_fb_request *request;
+	struct drm_device *drm = &adev->drm;
+	struct appletbdrm_frame *frame;
+	u64 timestamp = ktime_get_ns();
+	struct drm_rect damage;
+	size_t frames_size = 0;
+	size_t request_size;
+	int ret;
+
+	drm_atomic_helper_damage_iter_init(&iter, old_state, state);
+	drm_atomic_for_each_plane_damage(&iter, &damage) {
+		frames_size += struct_size(frame, buf, rect_size(&damage));
+	}
+
+	if (!frames_size)
+		return 0;
+
+	request_size = ALIGN(sizeof(*request) + frames_size + sizeof(*footer), 16);
+
+	request = kzalloc(request_size, GFP_KERNEL);
+	if (!request)
+		return -ENOMEM;
+
+	response = kzalloc(sizeof(*response), GFP_KERNEL);
+	if (!response) {
+		ret = -ENOMEM;
+		goto free_request;
+	}
+
+	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
+	if (ret) {
+		drm_err(drm, "Failed to start CPU framebuffer access (%pe)\n", ERR_PTR(ret));
+		goto free_response;
+	}
+
+	request->header.unk_00 = cpu_to_le16(2);
+	request->header.unk_02 = cpu_to_le16(0x12);
+	request->header.unk_04 = cpu_to_le32(9);
+	request->header.size = cpu_to_le32(request_size - sizeof(request->header));
+	request->unk_10 = cpu_to_le16(1);
+	request->msg_id = timestamp & 0xff;
+
+	frame = (struct appletbdrm_frame *)request->data;
+
+	drm_atomic_helper_damage_iter_init(&iter, old_state, state);
+	drm_atomic_for_each_plane_damage(&iter, &damage) {
+		struct iosys_map dst = IOSYS_MAP_INIT_VADDR(frame->buf);
+		u32 buf_size = rect_size(&damage);
+
+		/*
+		 * The coordinates need to be translated to the coordinate
+		 * system the device expects, see the comment in
+		 * appletbdrm_setup_mode_config
+		 */
+		frame->begin_x = cpu_to_le16(damage.y1);
+		frame->begin_y = cpu_to_le16(adev->height - damage.x2);
+		frame->width = cpu_to_le16(drm_rect_height(&damage));
+		frame->height = cpu_to_le16(drm_rect_width(&damage));
+		frame->buf_size = cpu_to_le32(buf_size);
+
+		ret = drm_fb_blit(&dst, NULL, DRM_FORMAT_BGR888,
+				  &shadow_plane_state->data[0], fb, &damage, &shadow_plane_state->fmtcnv_state);
+		if (ret) {
+			drm_err(drm, "Failed to copy damage clip (%pe)\n", ERR_PTR(ret));
+			goto end_fb_cpu_access;
+		}
+
+		frame = (void *)frame + struct_size(frame, buf, buf_size);
+	}
+
+	footer = (struct appletbdrm_fb_request_footer *)&request->data[frames_size];
+
+	footer->unk_0c = cpu_to_le32(0xfffe);
+	footer->unk_1c = cpu_to_le32(0x80001);
+	footer->unk_34 = cpu_to_le32(0x80002);
+	footer->unk_4c = cpu_to_le32(0xffff);
+	footer->timestamp = cpu_to_le64(timestamp);
+
+	ret = appletbdrm_send_request(adev, &request->header, request_size);
+	if (ret)
+		goto end_fb_cpu_access;
+
+	ret = appletbdrm_read_response(adev, &response->header, sizeof(*response),
+				       APPLETBDRM_MSG_UPDATE_COMPLETE);
+	if (ret)
+		goto end_fb_cpu_access;
+
+	if (response->timestamp != footer->timestamp) {
+		drm_err(drm, "Response timestamp (%llu) doesn't match request timestamp (%llu)\n",
+			le64_to_cpu(response->timestamp), timestamp);
+		goto end_fb_cpu_access;
+	}
+
+end_fb_cpu_access:
+	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
+free_response:
+	kfree(response);
+free_request:
+	kfree(request);
+
+	return ret;
+}
+
+static int appletbdrm_connector_helper_get_modes(struct drm_connector *connector)
+{
+	struct appletbdrm_device *adev = drm_to_adev(connector->dev);
+
+	return drm_connector_helper_get_modes_fixed(connector, &adev->mode);
+}
+
+static enum drm_mode_status appletbdrm_pipe_mode_valid(struct drm_simple_display_pipe *pipe,
+						       const struct drm_display_mode *mode)
+{
+	struct drm_crtc *crtc = &pipe->crtc;
+	struct appletbdrm_device *adev = drm_to_adev(crtc->dev);
+
+	return drm_crtc_helper_mode_valid_fixed(crtc, mode, &adev->mode);
+}
+
+static void appletbdrm_pipe_disable(struct drm_simple_display_pipe *pipe)
+{
+	struct appletbdrm_device *adev = drm_to_adev(pipe->crtc.dev);
+	int idx;
+
+	if (!drm_dev_enter(&adev->drm, &idx))
+		return;
+
+	appletbdrm_clear_display(adev);
+
+	drm_dev_exit(idx);
+}
+
+static void appletbdrm_pipe_update(struct drm_simple_display_pipe *pipe,
+				   struct drm_plane_state *old_state)
+{
+	struct drm_crtc *crtc = &pipe->crtc;
+	struct appletbdrm_device *adev = drm_to_adev(crtc->dev);
+	int idx;
+
+	if (!crtc->state->active || !drm_dev_enter(&adev->drm, &idx))
+		return;
+
+	appletbdrm_flush_damage(adev, old_state, pipe->plane.state);
+
+	drm_dev_exit(idx);
+}
+
+static const u32 appletbdrm_formats[] = {
+	DRM_FORMAT_BGR888,
+	DRM_FORMAT_XRGB8888, /* emulated */
+};
+
+static const struct drm_mode_config_funcs appletbdrm_mode_config_funcs = {
+	.fb_create = drm_gem_fb_create_with_dirty,
+	.atomic_check = drm_atomic_helper_check,
+	.atomic_commit = drm_atomic_helper_commit,
+};
+
+static const struct drm_connector_funcs appletbdrm_connector_funcs = {
+	.reset = drm_atomic_helper_connector_reset,
+	.destroy = drm_connector_cleanup,
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+};
+
+static const struct drm_connector_helper_funcs appletbdrm_connector_helper_funcs = {
+	.get_modes = appletbdrm_connector_helper_get_modes,
+};
+
+static const struct drm_simple_display_pipe_funcs appletbdrm_pipe_funcs = {
+	DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS,
+	.update = appletbdrm_pipe_update,
+	.disable = appletbdrm_pipe_disable,
+	.mode_valid = appletbdrm_pipe_mode_valid,
+};
+
+DEFINE_DRM_GEM_FOPS(appletbdrm_drm_fops);
+
+static const struct drm_driver appletbdrm_drm_driver = {
+	DRM_GEM_SHMEM_DRIVER_OPS,
+	.name			= "appletbdrm",
+	.desc			= "Apple Touch Bar DRM Driver",
+	.date			= "20230910",
+	.major			= 1,
+	.minor			= 0,
+	.driver_features	= DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC,
+	.fops			= &appletbdrm_drm_fops,
+};
+
+static int appletbdrm_setup_mode_config(struct appletbdrm_device *adev)
+{
+	struct drm_connector *connector = &adev->connector;
+	struct drm_device *drm = &adev->drm;
+	struct device *dev = adev->dev;
+	int ret;
+
+	ret = drmm_mode_config_init(drm);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to initialize mode configuration\n");
+
+	/*
+	 * The coordinate system used by the device is different from the
+	 * coordinate system of the framebuffer in that the x and y axes are
+	 * swapped, and that the y axis is inverted; so what the device reports
+	 * as the height is actually the width of the framebuffer and vice
+	 * versa
+	 */
+	drm->mode_config.min_width = 0;
+	drm->mode_config.min_height = 0;
+	drm->mode_config.max_width = max(adev->height, DRM_SHADOW_PLANE_MAX_WIDTH);
+	drm->mode_config.max_height = max(adev->width, DRM_SHADOW_PLANE_MAX_HEIGHT);
+	drm->mode_config.preferred_depth = APPLETBDRM_BITS_PER_PIXEL;
+	drm->mode_config.funcs = &appletbdrm_mode_config_funcs;
+
+	adev->mode = (struct drm_display_mode) {
+		DRM_MODE_INIT(60, adev->height, adev->width,
+			      DRM_MODE_RES_MM(adev->height, 218),
+			      DRM_MODE_RES_MM(adev->width, 218))
+	};
+
+	ret = drm_connector_init(drm, connector,
+				 &appletbdrm_connector_funcs, DRM_MODE_CONNECTOR_USB);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to initialize connector\n");
+
+	drm_connector_helper_add(connector, &appletbdrm_connector_helper_funcs);
+
+	ret = drm_connector_set_panel_orientation(connector,
+						  DRM_MODE_PANEL_ORIENTATION_RIGHT_UP);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to set panel orientation\n");
+
+	connector->display_info.non_desktop = true;
+	ret = drm_object_property_set_value(&connector->base,
+					    drm->mode_config.non_desktop_property, true);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to set non-desktop property\n");
+
+	ret = drm_simple_display_pipe_init(drm, &adev->pipe, &appletbdrm_pipe_funcs,
+					   appletbdrm_formats, ARRAY_SIZE(appletbdrm_formats),
+					   NULL, &adev->connector);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to initialize simple display pipe\n");
+
+	drm_plane_enable_fb_damage_clips(&adev->pipe.plane);
+
+	drm_mode_config_reset(drm);
+
+	ret = drm_dev_register(drm, 0);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to register DRM device\n");
+
+	return 0;
+}
+
+static int appletbdrm_probe(struct usb_interface *intf,
+			    const struct usb_device_id *id)
+{
+	struct usb_endpoint_descriptor *bulk_in, *bulk_out;
+	struct device *dev = &intf->dev;
+	struct appletbdrm_device *adev;
+	int ret;
+
+	ret = usb_find_common_endpoints(intf->cur_altsetting, &bulk_in, &bulk_out, NULL, NULL);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to find bulk endpoints\n");
+
+	adev = devm_drm_dev_alloc(dev, &appletbdrm_drm_driver, struct appletbdrm_device, drm);
+	if (IS_ERR(adev))
+		return PTR_ERR(adev);
+
+	adev->dev = dev;
+	adev->in_ep = bulk_in->bEndpointAddress;
+	adev->out_ep = bulk_out->bEndpointAddress;
+
+	usb_set_intfdata(intf, adev);
+
+	ret = appletbdrm_get_information(adev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to get display information\n");
+
+	ret = appletbdrm_signal_readiness(adev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to signal readiness\n");
+
+	ret = appletbdrm_clear_display(adev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to clear display\n");
+
+	return appletbdrm_setup_mode_config(adev);
+}
+
+static void appletbdrm_disconnect(struct usb_interface *intf)
+{
+	struct appletbdrm_device *adev = usb_get_intfdata(intf);
+	struct drm_device *drm = &adev->drm;
+
+	drm_dev_unplug(drm);
+	drm_atomic_helper_shutdown(drm);
+}
+
+static void appletbdrm_shutdown(struct usb_interface *intf)
+{
+	struct appletbdrm_device *adev = usb_get_intfdata(intf);
+
+	/*
+	 * The framebuffer needs to be cleared on shutdown since its content
+	 * persists across boots
+	 */
+	drm_atomic_helper_shutdown(&adev->drm);
+}
+
+static const struct usb_device_id appletbdrm_usb_id_table[] = {
+	{ USB_DEVICE_INTERFACE_CLASS(0x05ac, 0x8302, USB_CLASS_AUDIO_VIDEO) },
+	{}
+};
+MODULE_DEVICE_TABLE(usb, appletbdrm_usb_id_table);
+
+static struct usb_driver appletbdrm_usb_driver = {
+	.name		= "appletbdrm",
+	.probe		= appletbdrm_probe,
+	.disconnect	= appletbdrm_disconnect,
+	.shutdown	= appletbdrm_shutdown,
+	.id_table	= appletbdrm_usb_id_table,
+};
+module_usb_driver(appletbdrm_usb_driver);
+
+MODULE_AUTHOR("Kerem Karabay <kekrby@gmail.com>");
+MODULE_DESCRIPTION("Apple Touch Bar DRM Driver");
+MODULE_LICENSE("GPL");
-- 
2.43.0

Re: [PATCH v5 10/10] drm/tiny: add driver for Apple Touch Bars in x86 Macs

[mripard]

[-- Attachment #1: Type: text/plain, Size: 1928 bytes --]

On Sat, Aug 17, 2024 at 11:52:22AM GMT, Aditya Garg wrote:
> From: Kerem Karabay <kekrby@gmail.com>
> 
> The Touch Bars found on x86 Macs support two USB configurations: one
> where the device presents itself as a HID keyboard and can display
> predefined sets of keys, and one where the operating system has full
> control over what is displayed. This commit adds support for the display
> functionality of the second configuration.
> 
> Note that this driver has only been tested on T2 Macs, and only includes
> the USB device ID for these devices. Testing on T1 Macs would be
> appreciated.
> 
> Credit goes to @imbushuo on GitHub for reverse engineering most of the
> protocol.
> 
> Signed-off-by: Kerem Karabay <kekrby@gmail.com>
> Signed-off-by: Aditya Garg <gargaditya08@live.com>
> ---
>  MAINTAINERS                       |   6 +
>  drivers/gpu/drm/tiny/Kconfig      |  12 +
>  drivers/gpu/drm/tiny/Makefile     |   1 +
>  drivers/gpu/drm/tiny/appletbdrm.c | 624 ++++++++++++++++++++++++++++++
>  4 files changed, 643 insertions(+)
>  create mode 100644 drivers/gpu/drm/tiny/appletbdrm.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8766f3e5e..2665e6c57 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6889,6 +6889,12 @@ S:	Supported
>  T:	git https://gitlab.freedesktop.org/drm/misc/kernel.git
>  F:	drivers/gpu/drm/sun4i/sun8i*
>  
> +DRM DRIVER FOR APPLE TOUCH BARS
> +M:	Kerem Karabay <kekrby@gmail.com>
> +L:	dri-devel@lists.freedesktop.org
> +S:	Maintained
> +F:	drivers/gpu/drm/tiny/appletbdrm.c

How do you plan on maintaining it? If you want to do so through drm-misc
(and you should), you need to list the gitlab repo here.

Also, I haven't seen Kerem take part of the discussion at all. Are they
ok with taking on the maintainer's role?

It's really clear to me either why this needs to be going through hid at
all. Is it not standalone?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v5 10/10] drm/tiny: add driver for Apple Touch Bars in x86 Macs

[Aditya Garg]

> On 27 Sep 2024, at 1:12 PM, mripard@kernel.org wrote:
> 
> On Sat, Aug 17, 2024 at 11:52:22AM GMT, Aditya Garg wrote:
>> From: Kerem Karabay <kekrby@gmail.com>
>> 
>> The Touch Bars found on x86 Macs support two USB configurations: one
>> where the device presents itself as a HID keyboard and can display
>> predefined sets of keys, and one where the operating system has full
>> control over what is displayed. This commit adds support for the display
>> functionality of the second configuration.
>> 
>> Note that this driver has only been tested on T2 Macs, and only includes
>> the USB device ID for these devices. Testing on T1 Macs would be
>> appreciated.
>> 
>> Credit goes to @imbushuo on GitHub for reverse engineering most of the
>> protocol.
>> 
>> Signed-off-by: Kerem Karabay <kekrby@gmail.com>
>> Signed-off-by: Aditya Garg <gargaditya08@live.com>
>> ---
>> MAINTAINERS                       |   6 +
>> drivers/gpu/drm/tiny/Kconfig      |  12 +
>> drivers/gpu/drm/tiny/Makefile     |   1 +
>> drivers/gpu/drm/tiny/appletbdrm.c | 624 ++++++++++++++++++++++++++++++
>> 4 files changed, 643 insertions(+)
>> create mode 100644 drivers/gpu/drm/tiny/appletbdrm.c
>> 
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 8766f3e5e..2665e6c57 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -6889,6 +6889,12 @@ S: Supported
>> T: git https://gitlab.freedesktop.org/drm/misc/kernel.git
>> F: drivers/gpu/drm/sun4i/sun8i*
>> 
>> +DRM DRIVER FOR APPLE TOUCH BARS
>> +M: Kerem Karabay <kekrby@gmail.com>
>> +L: dri-devel@lists.freedesktop.org
>> +S: Maintained
>> +F: drivers/gpu/drm/tiny/appletbdrm.c

Hi Maxime

> 
> How do you plan on maintaining it? If you want to do so through drm-misc
> (and you should), you need to list the gitlab repo here.

Sorry for the oversight. Indeed the maintainer needs to be changed here, since Kerem is no longer active on t2linux, the organisation I am working with.

If a maintainer is really needed, I’m fine with taking the role. I can also request other people from t2linux to take up the role as well. If its optional, I’ll remove it
> 
> Also, I haven't seen Kerem take part of the discussion at all. Are they
> ok with taking on the maintainer's role?
> 
> It's really clear to me either why this needs to be going through hid at
> all. Is it not standalone?

The hid patchset has various patches to hid-multitouch with are needed along with the drm driver.

Thanks
Aditya

Re: [PATCH v5 10/10] drm/tiny: add driver for Apple Touch Bars in x86 Macs

[Thomas Zimmermann]

Hi

Am 17.08.24 um 13:52 schrieb Aditya Garg:
> From: Kerem Karabay <kekrby@gmail.com>
>
> The Touch Bars found on x86 Macs support two USB configurations: one
> where the device presents itself as a HID keyboard and can display
> predefined sets of keys, and one where the operating system has full
> control over what is displayed. This commit adds support for the display
> functionality of the second configuration.
>
> Note that this driver has only been tested on T2 Macs, and only includes
> the USB device ID for these devices. Testing on T1 Macs would be
> appreciated.
>
> Credit goes to @imbushuo on GitHub for reverse engineering most of the
> protocol.
>
> Signed-off-by: Kerem Karabay <kekrby@gmail.com>
> Signed-off-by: Aditya Garg <gargaditya08@live.com>
> ---
>   MAINTAINERS                       |   6 +
>   drivers/gpu/drm/tiny/Kconfig      |  12 +
>   drivers/gpu/drm/tiny/Makefile     |   1 +
>   drivers/gpu/drm/tiny/appletbdrm.c | 624 ++++++++++++++++++++++++++++++
>   4 files changed, 643 insertions(+)
>   create mode 100644 drivers/gpu/drm/tiny/appletbdrm.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8766f3e5e..2665e6c57 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6889,6 +6889,12 @@ S:	Supported
>   T:	git https://gitlab.freedesktop.org/drm/misc/kernel.git
>   F:	drivers/gpu/drm/sun4i/sun8i*
>   
> +DRM DRIVER FOR APPLE TOUCH BARS
> +M:	Kerem Karabay <kekrby@gmail.com>
> +L:	dri-devel@lists.freedesktop.org
> +S:	Maintained
> +F:	drivers/gpu/drm/tiny/appletbdrm.c
> +
>   DRM DRIVER FOR ARM PL111 CLCD
>   S:	Orphan
>   T:	git https://gitlab.freedesktop.org/drm/misc/kernel.git
> diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
> index f6889f649..559a97bce 100644
> --- a/drivers/gpu/drm/tiny/Kconfig
> +++ b/drivers/gpu/drm/tiny/Kconfig
> @@ -1,5 +1,17 @@
>   # SPDX-License-Identifier: GPL-2.0-only
>   
> +config DRM_APPLETBDRM
> +	tristate "DRM support for Apple Touch Bars"
> +	depends on DRM && USB && MMU
> +	select DRM_KMS_HELPER
> +	select DRM_GEM_SHMEM_HELPER

nit: Selects sorted alphabetically.

> +	help
> +	  Say Y here if you want support for the display of Touch Bars on x86
> +	  MacBook Pros.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called appletbdrm.
> +
>   config DRM_ARCPGU
>   	tristate "ARC PGU"
>   	depends on DRM && OF
> diff --git a/drivers/gpu/drm/tiny/Makefile b/drivers/gpu/drm/tiny/Makefile
> index 76dde89a0..9a1b412e7 100644
> --- a/drivers/gpu/drm/tiny/Makefile
> +++ b/drivers/gpu/drm/tiny/Makefile
> @@ -1,5 +1,6 @@
>   # SPDX-License-Identifier: GPL-2.0-only
>   
> +obj-$(CONFIG_DRM_APPLETBDRM)		+= appletbdrm.o
>   obj-$(CONFIG_DRM_ARCPGU)		+= arcpgu.o
>   obj-$(CONFIG_DRM_BOCHS)			+= bochs.o
>   obj-$(CONFIG_DRM_CIRRUS_QEMU)		+= cirrus.o
> diff --git a/drivers/gpu/drm/tiny/appletbdrm.c b/drivers/gpu/drm/tiny/appletbdrm.c
> new file mode 100644
> index 000000000..b9440ce00
> --- /dev/null
> +++ b/drivers/gpu/drm/tiny/appletbdrm.c
> @@ -0,0 +1,624 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Apple Touch Bar DRM Driver
> + *
> + * Copyright (c) 2023 Kerem Karabay <kekrby@gmail.com>
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <asm/unaligned.h>
> +
> +#include <linux/usb.h>
> +#include <linux/module.h>

Include statements should be sorted alphabetically.

> +
> +#include <drm/drm_drv.h>
> +#include <drm/drm_fourcc.h>
> +#include <drm/drm_probe_helper.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_damage_helper.h>
> +#include <drm/drm_format_helper.h>
> +#include <drm/drm_gem_shmem_helper.h>
> +#include <drm/drm_gem_atomic_helper.h>
> +#include <drm/drm_simple_kms_helper.h>
> +#include <drm/drm_gem_framebuffer_helper.h>

Sorting.

> +
> +#define _APPLETBDRM_FOURCC(s)		(((s)[0] << 24) | ((s)[1] << 16) | ((s)[2] << 8) | (s)[3])
> +#define APPLETBDRM_FOURCC(s)		_APPLETBDRM_FOURCC(#s)
> +
> +#define APPLETBDRM_PIXEL_FORMAT		APPLETBDRM_FOURCC(RGBA) /* The actual format is BGR888 */

Pleasedon't call it FOURCC (because it isn't). Rather do something like

#define __APPLETBDRM_MSG_STR4(str4)    ((__le32 __force) /* bit shifting 
here */)
#define __APPLETBDRM_MSG_TOK4(tok4)    __APPLETBDRM_MSG_STR4(#tok4)

(two underscores)

> +#define APPLETBDRM_BITS_PER_PIXEL	24
> +
> +#define APPLETBDRM_MSG_CLEAR_DISPLAY	APPLETBDRM_FOURCC(CLRD)
> +#define APPLETBDRM_MSG_GET_INFORMATION	APPLETBDRM_FOURCC(GINF)
> +#define APPLETBDRM_MSG_UPDATE_COMPLETE	APPLETBDRM_FOURCC(UDCL)
> +#define APPLETBDRM_MSG_SIGNAL_READINESS	APPLETBDRM_FOURCC(REDY)

Maybe infix all such constants with _MSG_ or _PROTO_ so that it's clear 
that these are part of the device protocol.

I'd also change the style a bit to something like
APPLETBDRM_MSG_REDY __APPLETBDRM_MSG_TOK4(REDY) /* device signals 
readiness */

> +
> +#define APPLETBDRM_BULK_MSG_TIMEOUT	1000
> +
> +#define drm_to_adev(_drm)		container_of(_drm, struct appletbdrm_device, drm)
> +#define adev_to_udev(adev)		interface_to_usbdev(to_usb_interface(adev->dev))
> +
> +struct appletbdrm_device {
> +	struct device *dev;
> +
> +	u8 in_ep;
> +	u8 out_ep;
> +
> +	u32 width;
> +	u32 height;

For software state, it's common to use regular types (unsigned int) 
without a specific size.

> +
> +	struct drm_device drm;
> +	struct drm_display_mode mode;
> +	struct drm_connector connector;
> +	struct drm_simple_display_pipe pipe;

Simple-display pipe is deprecated. Please don't add any new drivers 
using it. You can easily replace it by taking its code and data 
structures and inline them into the driver. Change the naming to fit the 
driver and it should be good.

> +
> +	bool readiness_signal_received;
> +};
> +
> +struct appletbdrm_request_header {
> +	__le16 unk_00;
> +	__le16 unk_02;
> +	__le32 unk_04;
> +	__le32 unk_08;
> +	__le32 size;
> +} __packed;
> +
> +struct appletbdrm_response_header {
> +	u8 unk_00[16];
> +	u32 msg;

These u32 are in host byte order. The protocol uses little endian? Then 
it's __le32. My _MSG_STR4 macro above adds a forced cast to __le32 already.

> +} __packed;
> +
> +struct appletbdrm_simple_request {
> +	struct appletbdrm_request_header header;
> +	u32 msg;

Same here and below.

> +	u8 unk_14[8];
> +	__le32 size;
> +} __packed;
> +
> +struct appletbdrm_information {
> +	struct appletbdrm_response_header header;
> +	u8 unk_14[12];
> +	__le32 width;
> +	__le32 height;
> +	u8 bits_per_pixel;
> +	__le32 bytes_per_row;
> +	__le32 orientation;
> +	__le32 bitmap_info;
> +	u32 pixel_format;
> +	__le32 width_inches;	/* floating point */
> +	__le32 height_inches;	/* floating point */
> +} __packed;
> +
> +struct appletbdrm_frame {
> +	__le16 begin_x;
> +	__le16 begin_y;
> +	__le16 width;
> +	__le16 height;
> +	__le32 buf_size;
> +	u8 buf[];
> +} __packed;
> +
> +struct appletbdrm_fb_request_footer {
> +	u8 unk_00[12];
> +	__le32 unk_0c;
> +	u8 unk_10[12];
> +	__le32 unk_1c;
> +	__le64 timestamp;
> +	u8 unk_28[12];
> +	__le32 unk_34;
> +	u8 unk_38[20];
> +	__le32 unk_4c;
> +} __packed;
> +
> +struct appletbdrm_fb_request {
> +	struct appletbdrm_request_header header;
> +	__le16 unk_10;
> +	u8 msg_id;
> +	u8 unk_13[29];
> +	/*
> +	 * Contents of `data`:
> +	 * - struct appletbdrm_frame frames[];
> +	 * - struct appletbdrm_fb_request_footer footer;
> +	 * - padding to make the total size a multiple of 16
> +	 */
> +	u8 data[];
> +} __packed;
> +
> +struct appletbdrm_fb_request_response {
> +	struct appletbdrm_response_header header;
> +	u8 unk_14[12];
> +	__le64 timestamp;
> +} __packed;

You should probably move all these structs and the related constants to 
the top of the file and add an _msg_ infix. So it's clear that this is 
protocol and not DRM driver state.

> +
> +static int appletbdrm_send_request(struct appletbdrm_device *adev,
> +				   struct appletbdrm_request_header *request, size_t size)
> +{
> +	struct usb_device *udev = adev_to_udev(adev);
> +	struct drm_device *drm = &adev->drm;
> +	int ret, actual_size;
> +
> +	ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, adev->out_ep),
> +			   request, size, &actual_size, APPLETBDRM_BULK_MSG_TIMEOUT);
> +	if (ret) {
> +		drm_err(drm, "Failed to send message (%pe)\n", ERR_PTR(ret));

Is there a reason for %pe? %d with ret should be fine. (?)

> +		return ret;
> +	}
> +
> +	if (actual_size != size) {
> +		drm_err(drm, "Actual size (%d) doesn't match expected size (%lu)\n",
> +			actual_size, size);
> +		return -EIO;
> +	}
> +
> +	return ret;
> +}
> +
> +static int appletbdrm_read_response(struct appletbdrm_device *adev,
> +				    struct appletbdrm_response_header *response,
> +				    size_t size, u32 expected_response)
> +{
> +	struct usb_device *udev = adev_to_udev(adev);
> +	struct drm_device *drm = &adev->drm;
> +	int ret, actual_size;
> +
> +retry:
> +	ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, adev->in_ep),
> +			   response, size, &actual_size, APPLETBDRM_BULK_MSG_TIMEOUT);
> +	if (ret) {
> +		drm_err(drm, "Failed to read response (%pe)\n", ERR_PTR(ret));
> +		return ret;
> +	}
> +
> +	/*
> +	 * The device responds to the first request sent in a particular
> +	 * timeframe after the USB device configuration is set with a readiness
> +	 * signal, in which case the response should be read again
> +	 */

IIUC the device signals (1) readiness at certain intervals? Or (2) just 
at the beginning after appletbdrm_signal_readiness() If (1), I'd make 
readiness_signal_received a local variable. If you receive REDY try 
again once. No need for per-driver state. If (2), wait for REDY after 
appletbdrm_signal_readiness() and remove the whole logic form this function.
> +	if (response->msg == APPLETBDRM_MSG_SIGNAL_READINESS) {
> +		if (!adev->readiness_signal_received) {
> +			adev->readiness_signal_received = true;
> +			goto retry;
> +		}
> +
> +		drm_err(drm, "Encountered unexpected readiness signal\n");
> +		return -EIO;
> +	}
> +
> +	if (actual_size != size) {
> +		drm_err(drm, "Actual size (%d) doesn't match expected size (%lu)\n",
> +			actual_size, size);
> +		return -EIO;
> +	}
> +
> +	if (response->msg != expected_response) {
> +		drm_err(drm, "Unexpected response from device (expected %p4ch found %p4ch)\n",
> +			&expected_response, &response->msg);
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static int appletbdrm_send_msg(struct appletbdrm_device *adev, u32 msg)
> +{
> +	struct appletbdrm_simple_request *request;
> +	int ret;
> +
> +	request = kzalloc(sizeof(*request), GFP_KERNEL);
> +	if (!request)
> +		return -ENOMEM;
> +
> +	request->header.unk_00 = cpu_to_le16(2);
> +	request->header.unk_02 = cpu_to_le16(0x1512);

Magic numbers? Do you know what they mean?

> +	request->header.size = cpu_to_le32(sizeof(*request) - sizeof(request->header));
> +	request->msg = msg;
> +	request->size = request->header.size;
> +
> +	ret = appletbdrm_send_request(adev, &request->header, sizeof(*request));
> +
> +	kfree(request);
> +
> +	return ret;
> +}
> +
> +static int appletbdrm_clear_display(struct appletbdrm_device *adev)
> +{
> +	return appletbdrm_send_msg(adev, APPLETBDRM_MSG_CLEAR_DISPLAY);
> +}
> +
> +static int appletbdrm_signal_readiness(struct appletbdrm_device *adev)
> +{
> +	return appletbdrm_send_msg(adev, APPLETBDRM_MSG_SIGNAL_READINESS);
> +}
> +
> +static int appletbdrm_get_information(struct appletbdrm_device *adev)
> +{
> +	struct appletbdrm_information *info;
> +	struct drm_device *drm = &adev->drm;
> +	u8 bits_per_pixel;
> +	u32 pixel_format;
> +	int ret;
> +
> +	info = kzalloc(sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return -ENOMEM;
> +
> +	ret = appletbdrm_send_msg(adev, APPLETBDRM_MSG_GET_INFORMATION);
> +	if (ret)
> +		return ret;
> +
> +	ret = appletbdrm_read_response(adev, &info->header, sizeof(*info),
> +				       APPLETBDRM_MSG_GET_INFORMATION);
> +	if (ret)
> +		goto free_info;
> +
> +	bits_per_pixel = info->bits_per_pixel;
> +	pixel_format = get_unaligned(&info->pixel_format);
> +
> +	adev->width = get_unaligned_le32(&info->width);
> +	adev->height = get_unaligned_le32(&info->height);
> +
> +	if (bits_per_pixel != APPLETBDRM_BITS_PER_PIXEL) {
> +		drm_err(drm, "Encountered unexpected bits per pixel value (%d)\n", bits_per_pixel);
> +		ret = -EINVAL;
> +		goto free_info;
> +	}
> +
> +	if (pixel_format != APPLETBDRM_PIXEL_FORMAT) {
> +		drm_err(drm, "Encountered unknown pixel format (%p4ch)\n", &pixel_format);
> +		ret = -EINVAL;
> +		goto free_info;
> +	}
> +
> +free_info:
> +	kfree(info);
> +
> +	return ret;
> +}
> +
> +static u32 rect_size(struct drm_rect *rect)
> +{
> +	return drm_rect_width(rect) * drm_rect_height(rect) * (APPLETBDRM_BITS_PER_PIXEL / 8);
> +}
> +
> +static int appletbdrm_flush_damage(struct appletbdrm_device *adev,
> +				   struct drm_plane_state *old_state,
> +				   struct drm_plane_state *state)
> +{
> +	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(state);
> +	struct appletbdrm_fb_request_response *response;
> +	struct appletbdrm_fb_request_footer *footer;
> +	struct drm_atomic_helper_damage_iter iter;
> +	struct drm_framebuffer *fb = state->fb;
> +	struct appletbdrm_fb_request *request;
> +	struct drm_device *drm = &adev->drm;
> +	struct appletbdrm_frame *frame;
> +	u64 timestamp = ktime_get_ns();
> +	struct drm_rect damage;
> +	size_t frames_size = 0;
> +	size_t request_size;
> +	int ret;
> +
> +	drm_atomic_helper_damage_iter_init(&iter, old_state, state);
> +	drm_atomic_for_each_plane_damage(&iter, &damage) {
> +		frames_size += struct_size(frame, buf, rect_size(&damage));
> +	}
> +
> +	if (!frames_size)
> +		return 0;
> +
> +	request_size = ALIGN(sizeof(*request) + frames_size + sizeof(*footer), 16);
> +
> +	request = kzalloc(request_size, GFP_KERNEL);
> +	if (!request)
> +		return -ENOMEM;
> +
> +	response = kzalloc(sizeof(*response), GFP_KERNEL);
> +	if (!response) {
> +		ret = -ENOMEM;
> +		goto free_request;
> +	}
> +
> +	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
> +	if (ret) {
> +		drm_err(drm, "Failed to start CPU framebuffer access (%pe)\n", ERR_PTR(ret));
> +		goto free_response;
> +	}
> +
> +	request->header.unk_00 = cpu_to_le16(2);
> +	request->header.unk_02 = cpu_to_le16(0x12);
> +	request->header.unk_04 = cpu_to_le32(9);
> +	request->header.size = cpu_to_le32(request_size - sizeof(request->header));
> +	request->unk_10 = cpu_to_le16(1);
> +	request->msg_id = timestamp & 0xff;
> +
> +	frame = (struct appletbdrm_frame *)request->data;
> +
> +	drm_atomic_helper_damage_iter_init(&iter, old_state, state);
> +	drm_atomic_for_each_plane_damage(&iter, &damage) {
> +		struct iosys_map dst = IOSYS_MAP_INIT_VADDR(frame->buf);
> +		u32 buf_size = rect_size(&damage);
> +
> +		/*
> +		 * The coordinates need to be translated to the coordinate
> +		 * system the device expects, see the comment in
> +		 * appletbdrm_setup_mode_config
> +		 */
> +		frame->begin_x = cpu_to_le16(damage.y1);
> +		frame->begin_y = cpu_to_le16(adev->height - damage.x2);
> +		frame->width = cpu_to_le16(drm_rect_height(&damage));
> +		frame->height = cpu_to_le16(drm_rect_width(&damage));
> +		frame->buf_size = cpu_to_le32(buf_size);
> +
> +		ret = drm_fb_blit(&dst, NULL, DRM_FORMAT_BGR888,
> +				  &shadow_plane_state->data[0], fb, &damage, &shadow_plane_state->fmtcnv_state);
> +		if (ret) {
> +			drm_err(drm, "Failed to copy damage clip (%pe)\n", ERR_PTR(ret));
> +			goto end_fb_cpu_access;
> +		}
> +
> +		frame = (void *)frame + struct_size(frame, buf, buf_size);
> +	}
> +
> +	footer = (struct appletbdrm_fb_request_footer *)&request->data[frames_size];
> +
> +	footer->unk_0c = cpu_to_le32(0xfffe);
> +	footer->unk_1c = cpu_to_le32(0x80001);
> +	footer->unk_34 = cpu_to_le32(0x80002);
> +	footer->unk_4c = cpu_to_le32(0xffff);
> +	footer->timestamp = cpu_to_le64(timestamp);
> +
> +	ret = appletbdrm_send_request(adev, &request->header, request_size);
> +	if (ret)
> +		goto end_fb_cpu_access;
> +
> +	ret = appletbdrm_read_response(adev, &response->header, sizeof(*response),
> +				       APPLETBDRM_MSG_UPDATE_COMPLETE);
> +	if (ret)
> +		goto end_fb_cpu_access;
> +
> +	if (response->timestamp != footer->timestamp) {
> +		drm_err(drm, "Response timestamp (%llu) doesn't match request timestamp (%llu)\n",
> +			le64_to_cpu(response->timestamp), timestamp);
> +		goto end_fb_cpu_access;
> +	}
> +
> +end_fb_cpu_access:
> +	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
> +free_response:
> +	kfree(response);
> +free_request:
> +	kfree(request);
> +
> +	return ret;
> +}
> +
> +static int appletbdrm_connector_helper_get_modes(struct drm_connector *connector)
> +{
> +	struct appletbdrm_device *adev = drm_to_adev(connector->dev);
> +
> +	return drm_connector_helper_get_modes_fixed(connector, &adev->mode);
> +}
> +
> +static enum drm_mode_status appletbdrm_pipe_mode_valid(struct drm_simple_display_pipe *pipe,
> +						       const struct drm_display_mode *mode)
> +{
> +	struct drm_crtc *crtc = &pipe->crtc;
> +	struct appletbdrm_device *adev = drm_to_adev(crtc->dev);
> +
> +	return drm_crtc_helper_mode_valid_fixed(crtc, mode, &adev->mode);
> +}
> +
> +static void appletbdrm_pipe_disable(struct drm_simple_display_pipe *pipe)
> +{
> +	struct appletbdrm_device *adev = drm_to_adev(pipe->crtc.dev);
> +	int idx;
> +
> +	if (!drm_dev_enter(&adev->drm, &idx))
> +		return;
> +
> +	appletbdrm_clear_display(adev);
> +
> +	drm_dev_exit(idx);
> +}
> +
> +static void appletbdrm_pipe_update(struct drm_simple_display_pipe *pipe,
> +				   struct drm_plane_state *old_state)
> +{
> +	struct drm_crtc *crtc = &pipe->crtc;
> +	struct appletbdrm_device *adev = drm_to_adev(crtc->dev);
> +	int idx;
> +
> +	if (!crtc->state->active || !drm_dev_enter(&adev->drm, &idx))
> +		return;
> +
> +	appletbdrm_flush_damage(adev, old_state, pipe->plane.state);
> +
> +	drm_dev_exit(idx);
> +}
> +
> +static const u32 appletbdrm_formats[] = {
> +	DRM_FORMAT_BGR888,
> +	DRM_FORMAT_XRGB8888, /* emulated */
> +};
> +
> +static const struct drm_mode_config_funcs appletbdrm_mode_config_funcs = {
> +	.fb_create = drm_gem_fb_create_with_dirty,
> +	.atomic_check = drm_atomic_helper_check,
> +	.atomic_commit = drm_atomic_helper_commit,
> +};
> +
> +static const struct drm_connector_funcs appletbdrm_connector_funcs = {
> +	.reset = drm_atomic_helper_connector_reset,
> +	.destroy = drm_connector_cleanup,
> +	.fill_modes = drm_helper_probe_single_connector_modes,
> +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> +};
> +
> +static const struct drm_connector_helper_funcs appletbdrm_connector_helper_funcs = {
> +	.get_modes = appletbdrm_connector_helper_get_modes,
> +};
> +
> +static const struct drm_simple_display_pipe_funcs appletbdrm_pipe_funcs = {
> +	DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS,
> +	.update = appletbdrm_pipe_update,
> +	.disable = appletbdrm_pipe_disable,
> +	.mode_valid = appletbdrm_pipe_mode_valid,
> +};
> +
> +DEFINE_DRM_GEM_FOPS(appletbdrm_drm_fops);
> +
> +static const struct drm_driver appletbdrm_drm_driver = {
> +	DRM_GEM_SHMEM_DRIVER_OPS,
> +	.name			= "appletbdrm",
> +	.desc			= "Apple Touch Bar DRM Driver",
> +	.date			= "20230910",
> +	.major			= 1,
> +	.minor			= 0,
> +	.driver_features	= DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC,
> +	.fops			= &appletbdrm_drm_fops,
> +};
> +
> +static int appletbdrm_setup_mode_config(struct appletbdrm_device *adev)
> +{
> +	struct drm_connector *connector = &adev->connector;
> +	struct drm_device *drm = &adev->drm;
> +	struct device *dev = adev->dev;
> +	int ret;
> +
> +	ret = drmm_mode_config_init(drm);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to initialize mode configuration\n");
> +
> +	/*
> +	 * The coordinate system used by the device is different from the
> +	 * coordinate system of the framebuffer in that the x and y axes are
> +	 * swapped, and that the y axis is inverted; so what the device reports
> +	 * as the height is actually the width of the framebuffer and vice
> +	 * versa
> +	 */
> +	drm->mode_config.min_width = 0;
> +	drm->mode_config.min_height = 0;
> +	drm->mode_config.max_width = max(adev->height, DRM_SHADOW_PLANE_MAX_WIDTH);
> +	drm->mode_config.max_height = max(adev->width, DRM_SHADOW_PLANE_MAX_HEIGHT);
> +	drm->mode_config.preferred_depth = APPLETBDRM_BITS_PER_PIXEL;
> +	drm->mode_config.funcs = &appletbdrm_mode_config_funcs;
> +
> +	adev->mode = (struct drm_display_mode) {
> +		DRM_MODE_INIT(60, adev->height, adev->width,
> +			      DRM_MODE_RES_MM(adev->height, 218),
> +			      DRM_MODE_RES_MM(adev->width, 218))
> +	};
> +
> +	ret = drm_connector_init(drm, connector,
> +				 &appletbdrm_connector_funcs, DRM_MODE_CONNECTOR_USB);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to initialize connector\n");
> +
> +	drm_connector_helper_add(connector, &appletbdrm_connector_helper_funcs);
> +
> +	ret = drm_connector_set_panel_orientation(connector,
> +						  DRM_MODE_PANEL_ORIENTATION_RIGHT_UP);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to set panel orientation\n");
> +
> +	connector->display_info.non_desktop = true;
> +	ret = drm_object_property_set_value(&connector->base,
> +					    drm->mode_config.non_desktop_property, true);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to set non-desktop property\n");
> +
> +	ret = drm_simple_display_pipe_init(drm, &adev->pipe, &appletbdrm_pipe_funcs,
> +					   appletbdrm_formats, ARRAY_SIZE(appletbdrm_formats),
> +					   NULL, &adev->connector);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to initialize simple display pipe\n");
> +
> +	drm_plane_enable_fb_damage_clips(&adev->pipe.plane);
> +
> +	drm_mode_config_reset(drm);
> +
> +	ret = drm_dev_register(drm, 0);

This line belongs into the caller.

I'll do another review for the next iteration.

Best regards
Thomas

> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to register DRM device\n");
> +
> +	return 0;
> +}
> +
> +static int appletbdrm_probe(struct usb_interface *intf,
> +			    const struct usb_device_id *id)
> +{
> +	struct usb_endpoint_descriptor *bulk_in, *bulk_out;
> +	struct device *dev = &intf->dev;
> +	struct appletbdrm_device *adev;
> +	int ret;
> +
> +	ret = usb_find_common_endpoints(intf->cur_altsetting, &bulk_in, &bulk_out, NULL, NULL);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to find bulk endpoints\n");
> +
> +	adev = devm_drm_dev_alloc(dev, &appletbdrm_drm_driver, struct appletbdrm_device, drm);
> +	if (IS_ERR(adev))
> +		return PTR_ERR(adev);
> +
> +	adev->dev = dev;
> +	adev->in_ep = bulk_in->bEndpointAddress;
> +	adev->out_ep = bulk_out->bEndpointAddress;
> +
> +	usb_set_intfdata(intf, adev);
> +
> +	ret = appletbdrm_get_information(adev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to get display information\n");
> +
> +	ret = appletbdrm_signal_readiness(adev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to signal readiness\n");
> +
> +	ret = appletbdrm_clear_display(adev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to clear display\n");
> +
> +	return appletbdrm_setup_mode_config(adev);
> +}
> +
> +static void appletbdrm_disconnect(struct usb_interface *intf)
> +{
> +	struct appletbdrm_device *adev = usb_get_intfdata(intf);
> +	struct drm_device *drm = &adev->drm;
> +
> +	drm_dev_unplug(drm);
> +	drm_atomic_helper_shutdown(drm);
> +}
> +
> +static void appletbdrm_shutdown(struct usb_interface *intf)
> +{
> +	struct appletbdrm_device *adev = usb_get_intfdata(intf);
> +
> +	/*
> +	 * The framebuffer needs to be cleared on shutdown since its content
> +	 * persists across boots
> +	 */
> +	drm_atomic_helper_shutdown(&adev->drm);
> +}
> +
> +static const struct usb_device_id appletbdrm_usb_id_table[] = {
> +	{ USB_DEVICE_INTERFACE_CLASS(0x05ac, 0x8302, USB_CLASS_AUDIO_VIDEO) },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(usb, appletbdrm_usb_id_table);
> +
> +static struct usb_driver appletbdrm_usb_driver = {
> +	.name		= "appletbdrm",
> +	.probe		= appletbdrm_probe,
> +	.disconnect	= appletbdrm_disconnect,
> +	.shutdown	= appletbdrm_shutdown,
> +	.id_table	= appletbdrm_usb_id_table,
> +};
> +module_usb_driver(appletbdrm_usb_driver);
> +
> +MODULE_AUTHOR("Kerem Karabay <kekrby@gmail.com>");
> +MODULE_DESCRIPTION("Apple Touch Bar DRM Driver");
> +MODULE_LICENSE("GPL");

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH v5 10/10] drm/tiny: add driver for Apple Touch Bars in x86 Macs

[Aditya Garg]

Hi Thomas

Thanks for the review, I’ll soon get to work.

> On 27 Sep 2024, at 1:18 PM, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> 
> Hi
> 
> Am 17.08.24 um 13:52 schrieb Aditya Garg:
>> From: Kerem Karabay <kekrby@gmail.com>
>> 
>> The Touch Bars found on x86 Macs support two USB configurations: one
>> where the device presents itself as a HID keyboard and can display
>> predefined sets of keys, and one where the operating system has full
>> control over what is displayed. This commit adds support for the display
>> functionality of the second configuration.
>> 
>> Note that this driver has only been tested on T2 Macs, and only includes
>> the USB device ID for these devices. Testing on T1 Macs would be
>> appreciated.
>> 
>> Credit goes to @imbushuo on GitHub for reverse engineering most of the
>> protocol.
>> 
>> Signed-off-by: Kerem Karabay <kekrby@gmail.com>
>> Signed-off-by: Aditya Garg <gargaditya08@live.com>
>> ---
>>  MAINTAINERS                       |   6 +
>>  drivers/gpu/drm/tiny/Kconfig      |  12 +
>>  drivers/gpu/drm/tiny/Makefile     |   1 +
>>  drivers/gpu/drm/tiny/appletbdrm.c | 624 ++++++++++++++++++++++++++++++
>>  4 files changed, 643 insertions(+)
>>  create mode 100644 drivers/gpu/drm/tiny/appletbdrm.c
>> 
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 8766f3e5e..2665e6c57 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -6889,6 +6889,12 @@ S: Supported
>>  T: git https://gitlab.freedesktop.org/drm/misc/kernel.git
>>  F: drivers/gpu/drm/sun4i/sun8i*
>>  +DRM DRIVER FOR APPLE TOUCH BARS
>> +M: Kerem Karabay <kekrby@gmail.com>
>> +L: dri-devel@lists.freedesktop.org
>> +S: Maintained
>> +F: drivers/gpu/drm/tiny/appletbdrm.c
>> +
>>  DRM DRIVER FOR ARM PL111 CLCD
>>  S: Orphan
>>  T: git https://gitlab.freedesktop.org/drm/misc/kernel.git
>> diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
>> index f6889f649..559a97bce 100644
>> --- a/drivers/gpu/drm/tiny/Kconfig
>> +++ b/drivers/gpu/drm/tiny/Kconfig
>> @@ -1,5 +1,17 @@
>>  # SPDX-License-Identifier: GPL-2.0-only
>>  +config DRM_APPLETBDRM
>> + tristate "DRM support for Apple Touch Bars"
>> + depends on DRM && USB && MMU
>> + select DRM_KMS_HELPER
>> + select DRM_GEM_SHMEM_HELPER
> 
> nit: Selects sorted alphabetically.
> 
>> + help
>> +   Say Y here if you want support for the display of Touch Bars on x86
>> +   MacBook Pros.
>> +
>> +   To compile this driver as a module, choose M here: the
>> +   module will be called appletbdrm.
>> +
>>  config DRM_ARCPGU
>>   tristate "ARC PGU"
>>   depends on DRM && OF
>> diff --git a/drivers/gpu/drm/tiny/Makefile b/drivers/gpu/drm/tiny/Makefile
>> index 76dde89a0..9a1b412e7 100644
>> --- a/drivers/gpu/drm/tiny/Makefile
>> +++ b/drivers/gpu/drm/tiny/Makefile
>> @@ -1,5 +1,6 @@
>>  # SPDX-License-Identifier: GPL-2.0-only
>>  +obj-$(CONFIG_DRM_APPLETBDRM) += appletbdrm.o
>>  obj-$(CONFIG_DRM_ARCPGU) += arcpgu.o
>>  obj-$(CONFIG_DRM_BOCHS) += bochs.o
>>  obj-$(CONFIG_DRM_CIRRUS_QEMU) += cirrus.o
>> diff --git a/drivers/gpu/drm/tiny/appletbdrm.c b/drivers/gpu/drm/tiny/appletbdrm.c
>> new file mode 100644
>> index 000000000..b9440ce00
>> --- /dev/null
>> +++ b/drivers/gpu/drm/tiny/appletbdrm.c
>> @@ -0,0 +1,624 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Apple Touch Bar DRM Driver
>> + *
>> + * Copyright (c) 2023 Kerem Karabay <kekrby@gmail.com>
>> + */
>> +
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> +
>> +#include <asm/unaligned.h>
>> +
>> +#include <linux/usb.h>
>> +#include <linux/module.h>
> 
> Include statements should be sorted alphabetically.
> 
>> +
>> +#include <drm/drm_drv.h>
>> +#include <drm/drm_fourcc.h>
>> +#include <drm/drm_probe_helper.h>
>> +#include <drm/drm_atomic_helper.h>
>> +#include <drm/drm_damage_helper.h>
>> +#include <drm/drm_format_helper.h>
>> +#include <drm/drm_gem_shmem_helper.h>
>> +#include <drm/drm_gem_atomic_helper.h>
>> +#include <drm/drm_simple_kms_helper.h>
>> +#include <drm/drm_gem_framebuffer_helper.h>
> 
> Sorting.
> 
>> +
>> +#define _APPLETBDRM_FOURCC(s) (((s)[0] << 24) | ((s)[1] << 16) | ((s)[2] << 8) | (s)[3])
>> +#define APPLETBDRM_FOURCC(s) _APPLETBDRM_FOURCC(#s)
>> +
>> +#define APPLETBDRM_PIXEL_FORMAT APPLETBDRM_FOURCC(RGBA) /* The actual format is BGR888 */
> 
> Pleasedon't call it FOURCC (because it isn't). Rather do something like
> 
> #define __APPLETBDRM_MSG_STR4(str4)    ((__le32 __force) /* bit shifting here */)
> #define __APPLETBDRM_MSG_TOK4(tok4)    __APPLETBDRM_MSG_STR4(#tok4)
> 
> (two underscores)
> 
>> +#define APPLETBDRM_BITS_PER_PIXEL 24
>> +
>> +#define APPLETBDRM_MSG_CLEAR_DISPLAY APPLETBDRM_FOURCC(CLRD)
>> +#define APPLETBDRM_MSG_GET_INFORMATION APPLETBDRM_FOURCC(GINF)
>> +#define APPLETBDRM_MSG_UPDATE_COMPLETE APPLETBDRM_FOURCC(UDCL)
>> +#define APPLETBDRM_MSG_SIGNAL_READINESS APPLETBDRM_FOURCC(REDY)
> 
> Maybe infix all such constants with _MSG_ or _PROTO_ so that it's clear that these are part of the device protocol.
> 
> I'd also change the style a bit to something like
> APPLETBDRM_MSG_REDY __APPLETBDRM_MSG_TOK4(REDY) /* device signals readiness */
> 
>> +
>> +#define APPLETBDRM_BULK_MSG_TIMEOUT 1000
>> +
>> +#define drm_to_adev(_drm) container_of(_drm, struct appletbdrm_device, drm)
>> +#define adev_to_udev(adev) interface_to_usbdev(to_usb_interface(adev->dev))
>> +
>> +struct appletbdrm_device {
>> + struct device *dev;
>> +
>> + u8 in_ep;
>> + u8 out_ep;
>> +
>> + u32 width;
>> + u32 height;
> 
> For software state, it's common to use regular types (unsigned int) without a specific size.
> 
>> +
>> + struct drm_device drm;
>> + struct drm_display_mode mode;
>> + struct drm_connector connector;
>> + struct drm_simple_display_pipe pipe;
> 
> Simple-display pipe is deprecated. Please don't add any new drivers using it. You can easily replace it by taking its code and data structures and inline them into the driver. Change the naming to fit the driver and it should be good.
> 
>> +
>> + bool readiness_signal_received;
>> +};
>> +
>> +struct appletbdrm_request_header {
>> + __le16 unk_00;
>> + __le16 unk_02;
>> + __le32 unk_04;
>> + __le32 unk_08;
>> + __le32 size;
>> +} __packed;
>> +
>> +struct appletbdrm_response_header {
>> + u8 unk_00[16];
>> + u32 msg;
> 
> These u32 are in host byte order. The protocol uses little endian? Then it's __le32. My _MSG_STR4 macro above adds a forced cast to __le32 already.
> 
>> +} __packed;
>> +
>> +struct appletbdrm_simple_request {
>> + struct appletbdrm_request_header header;
>> + u32 msg;
> 
> Same here and below.
> 
>> + u8 unk_14[8];
>> + __le32 size;
>> +} __packed;
>> +
>> +struct appletbdrm_information {
>> + struct appletbdrm_response_header header;
>> + u8 unk_14[12];
>> + __le32 width;
>> + __le32 height;
>> + u8 bits_per_pixel;
>> + __le32 bytes_per_row;
>> + __le32 orientation;
>> + __le32 bitmap_info;
>> + u32 pixel_format;
>> + __le32 width_inches; /* floating point */
>> + __le32 height_inches; /* floating point */
>> +} __packed;
>> +
>> +struct appletbdrm_frame {
>> + __le16 begin_x;
>> + __le16 begin_y;
>> + __le16 width;
>> + __le16 height;
>> + __le32 buf_size;
>> + u8 buf[];
>> +} __packed;
>> +
>> +struct appletbdrm_fb_request_footer {
>> + u8 unk_00[12];
>> + __le32 unk_0c;
>> + u8 unk_10[12];
>> + __le32 unk_1c;
>> + __le64 timestamp;
>> + u8 unk_28[12];
>> + __le32 unk_34;
>> + u8 unk_38[20];
>> + __le32 unk_4c;
>> +} __packed;
>> +
>> +struct appletbdrm_fb_request {
>> + struct appletbdrm_request_header header;
>> + __le16 unk_10;
>> + u8 msg_id;
>> + u8 unk_13[29];
>> + /*
>> +  * Contents of `data`:
>> +  * - struct appletbdrm_frame frames[];
>> +  * - struct appletbdrm_fb_request_footer footer;
>> +  * - padding to make the total size a multiple of 16
>> +  */
>> + u8 data[];
>> +} __packed;
>> +
>> +struct appletbdrm_fb_request_response {
>> + struct appletbdrm_response_header header;
>> + u8 unk_14[12];
>> + __le64 timestamp;
>> +} __packed;
> 
> You should probably move all these structs and the related constants to the top of the file and add an _msg_ infix. So it's clear that this is protocol and not DRM driver state.
> 
>> +
>> +static int appletbdrm_send_request(struct appletbdrm_device *adev,
>> +    struct appletbdrm_request_header *request, size_t size)
>> +{
>> + struct usb_device *udev = adev_to_udev(adev);
>> + struct drm_device *drm = &adev->drm;
>> + int ret, actual_size;
>> +
>> + ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, adev->out_ep),
>> +    request, size, &actual_size, APPLETBDRM_BULK_MSG_TIMEOUT);
>> + if (ret) {
>> + drm_err(drm, "Failed to send message (%pe)\n", ERR_PTR(ret));
> 
> Is there a reason for %pe? %d with ret should be fine. (?)
> 
>> + return ret;
>> + }
>> +
>> + if (actual_size != size) {
>> + drm_err(drm, "Actual size (%d) doesn't match expected size (%lu)\n",
>> + actual_size, size);
>> + return -EIO;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static int appletbdrm_read_response(struct appletbdrm_device *adev,
>> +     struct appletbdrm_response_header *response,
>> +     size_t size, u32 expected_response)
>> +{
>> + struct usb_device *udev = adev_to_udev(adev);
>> + struct drm_device *drm = &adev->drm;
>> + int ret, actual_size;
>> +
>> +retry:
>> + ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, adev->in_ep),
>> +    response, size, &actual_size, APPLETBDRM_BULK_MSG_TIMEOUT);
>> + if (ret) {
>> + drm_err(drm, "Failed to read response (%pe)\n", ERR_PTR(ret));
>> + return ret;
>> + }
>> +
>> + /*
>> +  * The device responds to the first request sent in a particular
>> +  * timeframe after the USB device configuration is set with a readiness
>> +  * signal, in which case the response should be read again
>> +  */
> 
> IIUC the device signals (1) readiness at certain intervals? Or (2) just at the beginning after appletbdrm_signal_readiness() If (1), I'd make readiness_signal_received a local variable. If you receive REDY try again once. No need for per-driver state. If (2), wait for REDY after appletbdrm_signal_readiness() and remove the whole logic form this function.
>> + if (response->msg == APPLETBDRM_MSG_SIGNAL_READINESS) {
>> + if (!adev->readiness_signal_received) {
>> + adev->readiness_signal_received = true;
>> + goto retry;
>> + }
>> +
>> + drm_err(drm, "Encountered unexpected readiness signal\n");
>> + return -EIO;
>> + }
>> +
>> + if (actual_size != size) {
>> + drm_err(drm, "Actual size (%d) doesn't match expected size (%lu)\n",
>> + actual_size, size);
>> + return -EIO;
>> + }
>> +
>> + if (response->msg != expected_response) {
>> + drm_err(drm, "Unexpected response from device (expected %p4ch found %p4ch)\n",
>> + &expected_response, &response->msg);
>> + return -EIO;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int appletbdrm_send_msg(struct appletbdrm_device *adev, u32 msg)
>> +{
>> + struct appletbdrm_simple_request *request;
>> + int ret;
>> +
>> + request = kzalloc(sizeof(*request), GFP_KERNEL);
>> + if (!request)
>> + return -ENOMEM;
>> +
>> + request->header.unk_00 = cpu_to_le16(2);
>> + request->header.unk_02 = cpu_to_le16(0x1512);
> 
> Magic numbers? Do you know what they mean?
> 
>> + request->header.size = cpu_to_le32(sizeof(*request) - sizeof(request->header));
>> + request->msg = msg;
>> + request->size = request->header.size;
>> +
>> + ret = appletbdrm_send_request(adev, &request->header, sizeof(*request));
>> +
>> + kfree(request);
>> +
>> + return ret;
>> +}
>> +
>> +static int appletbdrm_clear_display(struct appletbdrm_device *adev)
>> +{
>> + return appletbdrm_send_msg(adev, APPLETBDRM_MSG_CLEAR_DISPLAY);
>> +}
>> +
>> +static int appletbdrm_signal_readiness(struct appletbdrm_device *adev)
>> +{
>> + return appletbdrm_send_msg(adev, APPLETBDRM_MSG_SIGNAL_READINESS);
>> +}
>> +
>> +static int appletbdrm_get_information(struct appletbdrm_device *adev)
>> +{
>> + struct appletbdrm_information *info;
>> + struct drm_device *drm = &adev->drm;
>> + u8 bits_per_pixel;
>> + u32 pixel_format;
>> + int ret;
>> +
>> + info = kzalloc(sizeof(*info), GFP_KERNEL);
>> + if (!info)
>> + return -ENOMEM;
>> +
>> + ret = appletbdrm_send_msg(adev, APPLETBDRM_MSG_GET_INFORMATION);
>> + if (ret)
>> + return ret;
>> +
>> + ret = appletbdrm_read_response(adev, &info->header, sizeof(*info),
>> +        APPLETBDRM_MSG_GET_INFORMATION);
>> + if (ret)
>> + goto free_info;
>> +
>> + bits_per_pixel = info->bits_per_pixel;
>> + pixel_format = get_unaligned(&info->pixel_format);
>> +
>> + adev->width = get_unaligned_le32(&info->width);
>> + adev->height = get_unaligned_le32(&info->height);
>> +
>> + if (bits_per_pixel != APPLETBDRM_BITS_PER_PIXEL) {
>> + drm_err(drm, "Encountered unexpected bits per pixel value (%d)\n", bits_per_pixel);
>> + ret = -EINVAL;
>> + goto free_info;
>> + }
>> +
>> + if (pixel_format != APPLETBDRM_PIXEL_FORMAT) {
>> + drm_err(drm, "Encountered unknown pixel format (%p4ch)\n", &pixel_format);
>> + ret = -EINVAL;
>> + goto free_info;
>> + }
>> +
>> +free_info:
>> + kfree(info);
>> +
>> + return ret;
>> +}
>> +
>> +static u32 rect_size(struct drm_rect *rect)
>> +{
>> + return drm_rect_width(rect) * drm_rect_height(rect) * (APPLETBDRM_BITS_PER_PIXEL / 8);
>> +}
>> +
>> +static int appletbdrm_flush_damage(struct appletbdrm_device *adev,
>> +    struct drm_plane_state *old_state,
>> +    struct drm_plane_state *state)
>> +{
>> + struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(state);
>> + struct appletbdrm_fb_request_response *response;
>> + struct appletbdrm_fb_request_footer *footer;
>> + struct drm_atomic_helper_damage_iter iter;
>> + struct drm_framebuffer *fb = state->fb;
>> + struct appletbdrm_fb_request *request;
>> + struct drm_device *drm = &adev->drm;
>> + struct appletbdrm_frame *frame;
>> + u64 timestamp = ktime_get_ns();
>> + struct drm_rect damage;
>> + size_t frames_size = 0;
>> + size_t request_size;
>> + int ret;
>> +
>> + drm_atomic_helper_damage_iter_init(&iter, old_state, state);
>> + drm_atomic_for_each_plane_damage(&iter, &damage) {
>> + frames_size += struct_size(frame, buf, rect_size(&damage));
>> + }
>> +
>> + if (!frames_size)
>> + return 0;
>> +
>> + request_size = ALIGN(sizeof(*request) + frames_size + sizeof(*footer), 16);
>> +
>> + request = kzalloc(request_size, GFP_KERNEL);
>> + if (!request)
>> + return -ENOMEM;
>> +
>> + response = kzalloc(sizeof(*response), GFP_KERNEL);
>> + if (!response) {
>> + ret = -ENOMEM;
>> + goto free_request;
>> + }
>> +
>> + ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
>> + if (ret) {
>> + drm_err(drm, "Failed to start CPU framebuffer access (%pe)\n", ERR_PTR(ret));
>> + goto free_response;
>> + }
>> +
>> + request->header.unk_00 = cpu_to_le16(2);
>> + request->header.unk_02 = cpu_to_le16(0x12);
>> + request->header.unk_04 = cpu_to_le32(9);
>> + request->header.size = cpu_to_le32(request_size - sizeof(request->header));
>> + request->unk_10 = cpu_to_le16(1);
>> + request->msg_id = timestamp & 0xff;
>> +
>> + frame = (struct appletbdrm_frame *)request->data;
>> +
>> + drm_atomic_helper_damage_iter_init(&iter, old_state, state);
>> + drm_atomic_for_each_plane_damage(&iter, &damage) {
>> + struct iosys_map dst = IOSYS_MAP_INIT_VADDR(frame->buf);
>> + u32 buf_size = rect_size(&damage);
>> +
>> + /*
>> +  * The coordinates need to be translated to the coordinate
>> +  * system the device expects, see the comment in
>> +  * appletbdrm_setup_mode_config
>> +  */
>> + frame->begin_x = cpu_to_le16(damage.y1);
>> + frame->begin_y = cpu_to_le16(adev->height - damage.x2);
>> + frame->width = cpu_to_le16(drm_rect_height(&damage));
>> + frame->height = cpu_to_le16(drm_rect_width(&damage));
>> + frame->buf_size = cpu_to_le32(buf_size);
>> +
>> + ret = drm_fb_blit(&dst, NULL, DRM_FORMAT_BGR888,
>> +   &shadow_plane_state->data[0], fb, &damage, &shadow_plane_state->fmtcnv_state);
>> + if (ret) {
>> + drm_err(drm, "Failed to copy damage clip (%pe)\n", ERR_PTR(ret));
>> + goto end_fb_cpu_access;
>> + }
>> +
>> + frame = (void *)frame + struct_size(frame, buf, buf_size);
>> + }
>> +
>> + footer = (struct appletbdrm_fb_request_footer *)&request->data[frames_size];
>> +
>> + footer->unk_0c = cpu_to_le32(0xfffe);
>> + footer->unk_1c = cpu_to_le32(0x80001);
>> + footer->unk_34 = cpu_to_le32(0x80002);
>> + footer->unk_4c = cpu_to_le32(0xffff);
>> + footer->timestamp = cpu_to_le64(timestamp);
>> +
>> + ret = appletbdrm_send_request(adev, &request->header, request_size);
>> + if (ret)
>> + goto end_fb_cpu_access;
>> +
>> + ret = appletbdrm_read_response(adev, &response->header, sizeof(*response),
>> +        APPLETBDRM_MSG_UPDATE_COMPLETE);
>> + if (ret)
>> + goto end_fb_cpu_access;
>> +
>> + if (response->timestamp != footer->timestamp) {
>> + drm_err(drm, "Response timestamp (%llu) doesn't match request timestamp (%llu)\n",
>> + le64_to_cpu(response->timestamp), timestamp);
>> + goto end_fb_cpu_access;
>> + }
>> +
>> +end_fb_cpu_access:
>> + drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>> +free_response:
>> + kfree(response);
>> +free_request:
>> + kfree(request);
>> +
>> + return ret;
>> +}
>> +
>> +static int appletbdrm_connector_helper_get_modes(struct drm_connector *connector)
>> +{
>> + struct appletbdrm_device *adev = drm_to_adev(connector->dev);
>> +
>> + return drm_connector_helper_get_modes_fixed(connector, &adev->mode);
>> +}
>> +
>> +static enum drm_mode_status appletbdrm_pipe_mode_valid(struct drm_simple_display_pipe *pipe,
>> +        const struct drm_display_mode *mode)
>> +{
>> + struct drm_crtc *crtc = &pipe->crtc;
>> + struct appletbdrm_device *adev = drm_to_adev(crtc->dev);
>> +
>> + return drm_crtc_helper_mode_valid_fixed(crtc, mode, &adev->mode);
>> +}
>> +
>> +static void appletbdrm_pipe_disable(struct drm_simple_display_pipe *pipe)
>> +{
>> + struct appletbdrm_device *adev = drm_to_adev(pipe->crtc.dev);
>> + int idx;
>> +
>> + if (!drm_dev_enter(&adev->drm, &idx))
>> + return;
>> +
>> + appletbdrm_clear_display(adev);
>> +
>> + drm_dev_exit(idx);
>> +}
>> +
>> +static void appletbdrm_pipe_update(struct drm_simple_display_pipe *pipe,
>> +    struct drm_plane_state *old_state)
>> +{
>> + struct drm_crtc *crtc = &pipe->crtc;
>> + struct appletbdrm_device *adev = drm_to_adev(crtc->dev);
>> + int idx;
>> +
>> + if (!crtc->state->active || !drm_dev_enter(&adev->drm, &idx))
>> + return;
>> +
>> + appletbdrm_flush_damage(adev, old_state, pipe->plane.state);
>> +
>> + drm_dev_exit(idx);
>> +}
>> +
>> +static const u32 appletbdrm_formats[] = {
>> + DRM_FORMAT_BGR888,
>> + DRM_FORMAT_XRGB8888, /* emulated */
>> +};
>> +
>> +static const struct drm_mode_config_funcs appletbdrm_mode_config_funcs = {
>> + .fb_create = drm_gem_fb_create_with_dirty,
>> + .atomic_check = drm_atomic_helper_check,
>> + .atomic_commit = drm_atomic_helper_commit,
>> +};
>> +
>> +static const struct drm_connector_funcs appletbdrm_connector_funcs = {
>> + .reset = drm_atomic_helper_connector_reset,
>> + .destroy = drm_connector_cleanup,
>> + .fill_modes = drm_helper_probe_single_connector_modes,
>> + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>> + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
>> +};
>> +
>> +static const struct drm_connector_helper_funcs appletbdrm_connector_helper_funcs = {
>> + .get_modes = appletbdrm_connector_helper_get_modes,
>> +};
>> +
>> +static const struct drm_simple_display_pipe_funcs appletbdrm_pipe_funcs = {
>> + DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS,
>> + .update = appletbdrm_pipe_update,
>> + .disable = appletbdrm_pipe_disable,
>> + .mode_valid = appletbdrm_pipe_mode_valid,
>> +};
>> +
>> +DEFINE_DRM_GEM_FOPS(appletbdrm_drm_fops);
>> +
>> +static const struct drm_driver appletbdrm_drm_driver = {
>> + DRM_GEM_SHMEM_DRIVER_OPS,
>> + .name = "appletbdrm",
>> + .desc = "Apple Touch Bar DRM Driver",
>> + .date = "20230910",
>> + .major = 1,
>> + .minor = 0,
>> + .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC,
>> + .fops = &appletbdrm_drm_fops,
>> +};
>> +
>> +static int appletbdrm_setup_mode_config(struct appletbdrm_device *adev)
>> +{
>> + struct drm_connector *connector = &adev->connector;
>> + struct drm_device *drm = &adev->drm;
>> + struct device *dev = adev->dev;
>> + int ret;
>> +
>> + ret = drmm_mode_config_init(drm);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to initialize mode configuration\n");
>> +
>> + /*
>> +  * The coordinate system used by the device is different from the
>> +  * coordinate system of the framebuffer in that the x and y axes are
>> +  * swapped, and that the y axis is inverted; so what the device reports
>> +  * as the height is actually the width of the framebuffer and vice
>> +  * versa
>> +  */
>> + drm->mode_config.min_width = 0;
>> + drm->mode_config.min_height = 0;
>> + drm->mode_config.max_width = max(adev->height, DRM_SHADOW_PLANE_MAX_WIDTH);
>> + drm->mode_config.max_height = max(adev->width, DRM_SHADOW_PLANE_MAX_HEIGHT);
>> + drm->mode_config.preferred_depth = APPLETBDRM_BITS_PER_PIXEL;
>> + drm->mode_config.funcs = &appletbdrm_mode_config_funcs;
>> +
>> + adev->mode = (struct drm_display_mode) {
>> + DRM_MODE_INIT(60, adev->height, adev->width,
>> +       DRM_MODE_RES_MM(adev->height, 218),
>> +       DRM_MODE_RES_MM(adev->width, 218))
>> + };
>> +
>> + ret = drm_connector_init(drm, connector,
>> +  &appletbdrm_connector_funcs, DRM_MODE_CONNECTOR_USB);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to initialize connector\n");
>> +
>> + drm_connector_helper_add(connector, &appletbdrm_connector_helper_funcs);
>> +
>> + ret = drm_connector_set_panel_orientation(connector,
>> +   DRM_MODE_PANEL_ORIENTATION_RIGHT_UP);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to set panel orientation\n");
>> +
>> + connector->display_info.non_desktop = true;
>> + ret = drm_object_property_set_value(&connector->base,
>> +     drm->mode_config.non_desktop_property, true);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to set non-desktop property\n");
>> +
>> + ret = drm_simple_display_pipe_init(drm, &adev->pipe, &appletbdrm_pipe_funcs,
>> +    appletbdrm_formats, ARRAY_SIZE(appletbdrm_formats),
>> +    NULL, &adev->connector);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to initialize simple display pipe\n");
>> +
>> + drm_plane_enable_fb_damage_clips(&adev->pipe.plane);
>> +
>> + drm_mode_config_reset(drm);
>> +
>> + ret = drm_dev_register(drm, 0);
> 
> This line belongs into the caller.
> 
> I'll do another review for the next iteration.
> 
> Best regards
> Thomas
> 
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to register DRM device\n");
>> +
>> + return 0;
>> +}
>> +
>> +static int appletbdrm_probe(struct usb_interface *intf,
>> +     const struct usb_device_id *id)
>> +{
>> + struct usb_endpoint_descriptor *bulk_in, *bulk_out;
>> + struct device *dev = &intf->dev;
>> + struct appletbdrm_device *adev;
>> + int ret;
>> +
>> + ret = usb_find_common_endpoints(intf->cur_altsetting, &bulk_in, &bulk_out, NULL, NULL);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to find bulk endpoints\n");
>> +
>> + adev = devm_drm_dev_alloc(dev, &appletbdrm_drm_driver, struct appletbdrm_device, drm);
>> + if (IS_ERR(adev))
>> + return PTR_ERR(adev);
>> +
>> + adev->dev = dev;
>> + adev->in_ep = bulk_in->bEndpointAddress;
>> + adev->out_ep = bulk_out->bEndpointAddress;
>> +
>> + usb_set_intfdata(intf, adev);
>> +
>> + ret = appletbdrm_get_information(adev);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to get display information\n");
>> +
>> + ret = appletbdrm_signal_readiness(adev);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to signal readiness\n");
>> +
>> + ret = appletbdrm_clear_display(adev);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to clear display\n");
>> +
>> + return appletbdrm_setup_mode_config(adev);
>> +}
>> +
>> +static void appletbdrm_disconnect(struct usb_interface *intf)
>> +{
>> + struct appletbdrm_device *adev = usb_get_intfdata(intf);
>> + struct drm_device *drm = &adev->drm;
>> +
>> + drm_dev_unplug(drm);
>> + drm_atomic_helper_shutdown(drm);
>> +}
>> +
>> +static void appletbdrm_shutdown(struct usb_interface *intf)
>> +{
>> + struct appletbdrm_device *adev = usb_get_intfdata(intf);
>> +
>> + /*
>> +  * The framebuffer needs to be cleared on shutdown since its content
>> +  * persists across boots
>> +  */
>> + drm_atomic_helper_shutdown(&adev->drm);
>> +}
>> +
>> +static const struct usb_device_id appletbdrm_usb_id_table[] = {
>> + { USB_DEVICE_INTERFACE_CLASS(0x05ac, 0x8302, USB_CLASS_AUDIO_VIDEO) },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(usb, appletbdrm_usb_id_table);
>> +
>> +static struct usb_driver appletbdrm_usb_driver = {
>> + .name = "appletbdrm",
>> + .probe = appletbdrm_probe,
>> + .disconnect = appletbdrm_disconnect,
>> + .shutdown = appletbdrm_shutdown,
>> + .id_table = appletbdrm_usb_id_table,
>> +};
>> +module_usb_driver(appletbdrm_usb_driver);
>> +
>> +MODULE_AUTHOR("Kerem Karabay <kekrby@gmail.com>");
>> +MODULE_DESCRIPTION("Apple Touch Bar DRM Driver");
>> +MODULE_LICENSE("GPL");
> 
> -- 
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Frankenstrasse 146, 90461 Nuernberg, Germany
> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
> HRB 36809 (AG Nuernberg)

Re: [RFC PATCH v5 10/10] drm/tiny: add driver for Apple Touch Bars in x86 Macs

[Aditya Garg]

Hi Thomas

>> From: Kerem Karabay <kekrby@gmail.com>
>> 
>> The Touch Bars found on x86 Macs support two USB configurations: one
>> where the device presents itself as a HID keyboard and can display
>> predefined sets of keys, and one where the operating system has full
>> control over what is displayed. This commit adds support for the display
>> functionality of the second configuration.
>> 
>> Note that this driver has only been tested on T2 Macs, and only includes
>> the USB device ID for these devices. Testing on T1 Macs would be
>> appreciated.
>> 
>> Credit goes to @imbushuo on GitHub for reverse engineering most of the
>> protocol.
>> 
>> Signed-off-by: Kerem Karabay <kekrby@gmail.com>
>> Signed-off-by: Aditya Garg <gargaditya08@live.com>
>> ---
>> MAINTAINERS                       |   6 +
>> drivers/gpu/drm/tiny/Kconfig      |  12 +
>> drivers/gpu/drm/tiny/Makefile     |   1 +
>> drivers/gpu/drm/tiny/appletbdrm.c | 624 ++++++++++++++++++++++++++++++
>> 4 files changed, 643 insertions(+)
>> create mode 100644 drivers/gpu/drm/tiny/appletbdrm.c
>> 
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 8766f3e5e..2665e6c57 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -6889,6 +6889,12 @@ S: Supported
>> T: git https://gitlab.freedesktop.org/drm/misc/kernel.git
>> F: drivers/gpu/drm/sun4i/sun8i*
>> +DRM DRIVER FOR APPLE TOUCH BARS
>> +M: Kerem Karabay <kekrby@gmail.com>
>> +L: dri-devel@lists.freedesktop.org
>> +S: Maintained
>> +F: drivers/gpu/drm/tiny/appletbdrm.c
>> +
>> DRM DRIVER FOR ARM PL111 CLCD
>> S: Orphan
>> T: git https://gitlab.freedesktop.org/drm/misc/kernel.git
>> diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
>> index f6889f649..559a97bce 100644
>> --- a/drivers/gpu/drm/tiny/Kconfig
>> +++ b/drivers/gpu/drm/tiny/Kconfig
>> @@ -1,5 +1,17 @@
>> # SPDX-License-Identifier: GPL-2.0-only
>> +config DRM_APPLETBDRM
>> + tristate "DRM support for Apple Touch Bars"
>> + depends on DRM && USB && MMU
>> + select DRM_KMS_HELPER
>> + select DRM_GEM_SHMEM_HELPER
> 
> nit: Selects sorted alphabetically.
> 
>> + help
>> +   Say Y here if you want support for the display of Touch Bars on x86
>> +   MacBook Pros.
>> +
>> +   To compile this driver as a module, choose M here: the
>> +   module will be called appletbdrm.
>> +
>> config DRM_ARCPGU
>>  tristate "ARC PGU"
>>  depends on DRM && OF
>> diff --git a/drivers/gpu/drm/tiny/Makefile b/drivers/gpu/drm/tiny/Makefile
>> index 76dde89a0..9a1b412e7 100644
>> --- a/drivers/gpu/drm/tiny/Makefile
>> +++ b/drivers/gpu/drm/tiny/Makefile
>> @@ -1,5 +1,6 @@
>> # SPDX-License-Identifier: GPL-2.0-only
>> +obj-$(CONFIG_DRM_APPLETBDRM) += appletbdrm.o
>> obj-$(CONFIG_DRM_ARCPGU) += arcpgu.o
>> obj-$(CONFIG_DRM_BOCHS) += bochs.o
>> obj-$(CONFIG_DRM_CIRRUS_QEMU) += cirrus.o
>> diff --git a/drivers/gpu/drm/tiny/appletbdrm.c b/drivers/gpu/drm/tiny/appletbdrm.c
>> new file mode 100644
>> index 000000000..b9440ce00
>> --- /dev/null
>> +++ b/drivers/gpu/drm/tiny/appletbdrm.c
>> @@ -0,0 +1,624 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Apple Touch Bar DRM Driver
>> + *
>> + * Copyright (c) 2023 Kerem Karabay <kekrby@gmail.com>
>> + */
>> +
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> +
>> +#include <asm/unaligned.h>
>> +
>> +#include <linux/usb.h>
>> +#include <linux/module.h>
> 
> Include statements should be sorted alphabetically.
> 
>> +
>> +#include <drm/drm_drv.h>
>> +#include <drm/drm_fourcc.h>
>> +#include <drm/drm_probe_helper.h>
>> +#include <drm/drm_atomic_helper.h>
>> +#include <drm/drm_damage_helper.h>
>> +#include <drm/drm_format_helper.h>
>> +#include <drm/drm_gem_shmem_helper.h>
>> +#include <drm/drm_gem_atomic_helper.h>
>> +#include <drm/drm_simple_kms_helper.h>
>> +#include <drm/drm_gem_framebuffer_helper.h>
> 
> Sorting.
> 
>> +
>> +#define _APPLETBDRM_FOURCC(s) (((s)[0] << 24) | ((s)[1] << 16) | ((s)[2] << 8) | (s)[3])
>> +#define APPLETBDRM_FOURCC(s) _APPLETBDRM_FOURCC(#s)
>> +
>> +#define APPLETBDRM_PIXEL_FORMAT APPLETBDRM_FOURCC(RGBA) /* The actual format is BGR888 */
> 
> Pleasedon't call it FOURCC (because it isn't). Rather do something like
> 
> #define __APPLETBDRM_MSG_STR4(str4)    ((__le32 __force) /* bit shifting here */)
> #define __APPLETBDRM_MSG_TOK4(tok4)    __APPLETBDRM_MSG_STR4(#tok4)
> 
> (two underscores)
> 
>> +#define APPLETBDRM_BITS_PER_PIXEL 24
>> +
>> +#define APPLETBDRM_MSG_CLEAR_DISPLAY APPLETBDRM_FOURCC(CLRD)
>> +#define APPLETBDRM_MSG_GET_INFORMATION APPLETBDRM_FOURCC(GINF)
>> +#define APPLETBDRM_MSG_UPDATE_COMPLETE APPLETBDRM_FOURCC(UDCL)
>> +#define APPLETBDRM_MSG_SIGNAL_READINESS APPLETBDRM_FOURCC(REDY)
> 
> Maybe infix all such constants with _MSG_ or _PROTO_ so that it's clear that these are part of the device protocol.
> 
> I'd also change the style a bit to something like
> APPLETBDRM_MSG_REDY __APPLETBDRM_MSG_TOK4(REDY) /* device signals readiness */
> 
>> +
>> +#define APPLETBDRM_BULK_MSG_TIMEOUT 1000
>> +
>> +#define drm_to_adev(_drm) container_of(_drm, struct appletbdrm_device, drm)
>> +#define adev_to_udev(adev) interface_to_usbdev(to_usb_interface(adev->dev))
>> +
>> +struct appletbdrm_device {
>> + struct device *dev;
>> +
>> + u8 in_ep;
>> + u8 out_ep;
>> +
>> + u32 width;
>> + u32 height;
> 
> For software state, it's common to use regular types (unsigned int) without a specific size.
> 
>> +
>> + struct drm_device drm;
>> + struct drm_display_mode mode;
>> + struct drm_connector connector;
>> + struct drm_simple_display_pipe pipe;
> 
> Simple-display pipe is deprecated. Please don't add any new drivers using it. You can easily replace it by taking its code and data structures and inline them into the driver. Change the naming to fit the driver and it should be good.

I attempted to use atomic helper instead of simple display helpers for the driver, but for some reason, modprobing the driver results in “killed” message. I request you to comment upon what’s wrong here. The changes done are here:

—>8—

diff --git a/usr/src/apple-touchbar-advanced-0.1/appletbdrm.c b/usr/src/apple-touchbar-advanced-0.1/appletbdrm.c
index 7a74c8a..8b11a34 100644
--- a/usr/src/apple-touchbar-advanced-0.1/appletbdrm.c
+++ b/usr/src/apple-touchbar-advanced-0.1/appletbdrm.c
@@ -13,6 +13,8 @@
#include <linux/module.h>

#include <drm/drm_drv.h>
+#include <drm/drm_atomic.h>
+#include <drm/drm_plane_helper.h>
#include <drm/drm_fourcc.h>
#include <drm/drm_probe_helper.h>
#include <drm/drm_atomic_helper.h>
@@ -51,7 +53,9 @@ struct appletbdrm_device {
	struct drm_device drm;
	struct drm_display_mode mode;
	struct drm_connector connector;
-	struct drm_simple_display_pipe pipe;
+	struct drm_plane primary_plane;
+	struct drm_crtc crtc;
+	struct drm_encoder encoder;

	bool readiness_signal_received;
};
@@ -401,44 +405,82 @@ static int appletbdrm_connector_helper_get_modes(struct drm_connector *connector
	return drm_connector_helper_get_modes_fixed(connector, &adev->mode);
}

-static enum drm_mode_status appletbdrm_pipe_mode_valid(struct drm_simple_display_pipe *pipe,
-						       const struct drm_display_mode *mode)
+static int appletbdrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
+						   struct drm_atomic_state *state)
{
-	struct drm_crtc *crtc = &pipe->crtc;
-	struct appletbdrm_device *adev = drm_to_adev(crtc->dev);
+	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane);
+	struct drm_crtc *new_crtc = new_plane_state->crtc;
+	struct drm_crtc_state *new_crtc_state = NULL;
+	int ret;

-	return drm_crtc_helper_mode_valid_fixed(crtc, mode, &adev->mode);
+	if (new_crtc)
+		new_crtc_state = drm_atomic_get_new_crtc_state(state, new_crtc);
+
+	ret = drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
+						  DRM_PLANE_NO_SCALING,
+						  DRM_PLANE_NO_SCALING,
+						  false, false);
+	if (ret)
+		return ret;
+	else if (!new_plane_state->visible)
+		return 0;
+
+	return 0;
}

-static void appletbdrm_pipe_disable(struct drm_simple_display_pipe *pipe)
+static void appletbdrm_primary_plane_helper_atomic_update(struct drm_plane *plane,
+						     struct drm_atomic_state *old_state)
{
-	struct appletbdrm_device *adev = drm_to_adev(pipe->crtc.dev);
+	struct appletbdrm_device *adev = drm_to_adev(plane->dev);
+	struct drm_device *drm = plane->dev;
+	struct drm_plane_state *plane_state = plane->state;
+	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(old_state, plane);
	int idx;

-	if (!drm_dev_enter(&adev->drm, &idx))
+	if (!drm_dev_enter(drm, &idx))
		return;

-	appletbdrm_clear_display(adev);
+	appletbdrm_flush_damage(adev, old_plane_state, plane_state);

	drm_dev_exit(idx);
}

-static void appletbdrm_pipe_update(struct drm_simple_display_pipe *pipe,
-				   struct drm_plane_state *old_state)
+static const struct drm_plane_helper_funcs appletbdrm_primary_plane_helper_funcs = {
+	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
+	.atomic_check = appletbdrm_primary_plane_helper_atomic_check,
+	.atomic_update = appletbdrm_primary_plane_helper_atomic_update,
+};
+
+static const struct drm_plane_funcs appletbdrm_primary_plane_funcs = {
+	.update_plane = drm_atomic_helper_update_plane,
+	.disable_plane = drm_atomic_helper_disable_plane,
+	.destroy = drm_plane_cleanup,
+	DRM_GEM_SHADOW_PLANE_FUNCS,
+};
+
+static enum drm_mode_status appletbdrm_crtc_helper_mode_valid(struct drm_crtc *crtc,
+							  const struct drm_display_mode *mode)
+{
+	struct appletbdrm_device *adev = drm_to_adev(crtc->dev);
+
+	return drm_crtc_helper_mode_valid_fixed(crtc, mode, &adev->mode);
+}
+
+static void appletbdrm_crtc_helper_atomic_disable(struct drm_crtc *crtc,
+					     struct drm_atomic_state *crtc_state)
{
-	struct drm_crtc *crtc = &pipe->crtc;
	struct appletbdrm_device *adev = drm_to_adev(crtc->dev);
	int idx;

-	if (!crtc->state->active || !drm_dev_enter(&adev->drm, &idx))
+	if (!drm_dev_enter(&adev->drm, &idx))
		return;

-	appletbdrm_flush_damage(adev, old_state, pipe->plane.state);
+	appletbdrm_clear_display(adev);

	drm_dev_exit(idx);
}

-static const u32 appletbdrm_formats[] = {
+static const u32 appletbdrm_primary_plane_formats[] = {
	DRM_FORMAT_BGR888,
	DRM_FORMAT_XRGB8888, /* emulated */
};
@@ -461,11 +503,22 @@ static const struct drm_connector_helper_funcs appletbdrm_connector_helper_funcs
	.get_modes = appletbdrm_connector_helper_get_modes,
};

-static const struct drm_simple_display_pipe_funcs appletbdrm_pipe_funcs = {
-	DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS,
-	.update = appletbdrm_pipe_update,
-	.disable = appletbdrm_pipe_disable,
-	.mode_valid = appletbdrm_pipe_mode_valid,
+static const struct drm_crtc_helper_funcs appletbdrm_crtc_helper_funcs = {
+	.mode_valid = appletbdrm_crtc_helper_mode_valid,
+	.atomic_disable = appletbdrm_crtc_helper_atomic_disable,
+};
+
+static const struct drm_crtc_funcs appletbdrm_crtc_funcs = {
+	.reset = drm_atomic_helper_crtc_reset,
+	.destroy = drm_crtc_cleanup,
+	.set_config = drm_atomic_helper_set_config,
+	.page_flip = drm_atomic_helper_page_flip,
+	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
+};
+
+static const struct drm_encoder_funcs appletbdrm_encoder_funcs = {
+	.destroy = drm_encoder_cleanup,
};

DEFINE_DRM_GEM_FOPS(appletbdrm_drm_fops);
@@ -484,10 +537,38 @@ static const struct drm_driver appletbdrm_drm_driver = {
static int appletbdrm_setup_mode_config(struct appletbdrm_device *adev)
{
	struct drm_connector *connector = &adev->connector;
+	struct drm_plane *primary_plane;
+	struct drm_crtc *crtc;
+	struct drm_encoder *encoder;
	struct drm_device *drm = &adev->drm;
	struct device *dev = adev->dev;
	int ret;

+	primary_plane = &adev->primary_plane;
+	ret = drm_universal_plane_init(drm, primary_plane, 0,
+				       &appletbdrm_primary_plane_funcs,
+				       appletbdrm_primary_plane_formats,
+				       ARRAY_SIZE(appletbdrm_primary_plane_formats),
+				       NULL,
+				       DRM_PLANE_TYPE_PRIMARY, NULL);
+	if (ret)
+		return ret;
+	drm_plane_helper_add(primary_plane, &appletbdrm_primary_plane_helper_funcs);
+
+	crtc = &adev->crtc;
+	ret = drm_crtc_init_with_planes(drm, crtc, primary_plane, NULL,
+					&appletbdrm_crtc_funcs, NULL);
+	if (ret)
+		return ret;
+	drm_crtc_helper_add(crtc, &appletbdrm_crtc_helper_funcs);
+
+	encoder = &adev->encoder;
+	ret = drm_encoder_init(drm, encoder, &appletbdrm_encoder_funcs,
+			       DRM_MODE_ENCODER_DAC, NULL);
+	if (ret)
+		return ret;
+	encoder->possible_crtcs = drm_crtc_mask(crtc);
+
	ret = drmm_mode_config_init(drm);
	if (ret)
		return dev_err_probe(dev, ret, "Failed to initialize mode configuration\n");
@@ -530,13 +611,13 @@ static int appletbdrm_setup_mode_config(struct appletbdrm_device *adev)
	if (ret)
		return dev_err_probe(dev, ret, "Failed to set non-desktop property\n");

-	ret = drm_simple_display_pipe_init(drm, &adev->pipe, &appletbdrm_pipe_funcs,
-					   appletbdrm_formats, ARRAY_SIZE(appletbdrm_formats),
-					   NULL, &adev->connector);
+	ret = drm_connector_attach_encoder(connector, encoder);
+
	if (ret)
		return dev_err_probe(dev, ret, "Failed to initialize simple display pipe\n");

-	drm_plane_enable_fb_damage_clips(&adev->pipe.plane);
+	drm_plane_helper_add(primary_plane, &appletbdrm_primary_plane_helper_funcs);
+	drm_plane_enable_fb_damage_clips(&adev->primary_plane);

	drm_mode_config_reset(drm);


The commit history having both old and new revisions of the driver is here:

https://github.com/AdityaGarg8/apple-touchbar-drv/blob/atomic/usr/src/apple-touchbar-advanced-0.1/appletbdrm.c

Thanks
Aditya

Re: [RFC PATCH v5 10/10] drm/tiny: add driver for Apple Touch Bars in x86 Macs

[Thomas Zimmermann]

Hi Aditya,

the code looks correct overall. There's one place where I think it 
fails. See below.

Am 15.02.25 um 14:43 schrieb Aditya Garg:
[...]
> DEFINE_DRM_GEM_FOPS(appletbdrm_drm_fops);
> @@ -484,10 +537,38 @@ static const struct drm_driver appletbdrm_drm_driver = {
> static int appletbdrm_setup_mode_config(struct appletbdrm_device *adev)
> {
> 	struct drm_connector *connector = &adev->connector;
> +	struct drm_plane *primary_plane;
> +	struct drm_crtc *crtc;
> +	struct drm_encoder *encoder;
> 	struct drm_device *drm = &adev->drm;
> 	struct device *dev = adev->dev;
> 	int ret;
>
> +	primary_plane = &adev->primary_plane;
> +	ret = drm_universal_plane_init(drm, primary_plane, 0,
> +				       &appletbdrm_primary_plane_funcs,
> +				       appletbdrm_primary_plane_formats,
> +				       ARRAY_SIZE(appletbdrm_primary_plane_formats),
> +				       NULL,
> +				       DRM_PLANE_TYPE_PRIMARY, NULL);
> +	if (ret)
> +		return ret;
> +	drm_plane_helper_add(primary_plane, &appletbdrm_primary_plane_helper_funcs);
> +
> +	crtc = &adev->crtc;
> +	ret = drm_crtc_init_with_planes(drm, crtc, primary_plane, NULL,
> +					&appletbdrm_crtc_funcs, NULL);
> +	if (ret)
> +		return ret;
> +	drm_crtc_helper_add(crtc, &appletbdrm_crtc_helper_funcs);
> +
> +	encoder = &adev->encoder;
> +	ret = drm_encoder_init(drm, encoder, &appletbdrm_encoder_funcs,
> +			       DRM_MODE_ENCODER_DAC, NULL);
> +	if (ret)
> +		return ret;
> +	encoder->possible_crtcs = drm_crtc_mask(crtc);
> +
> 	ret = drmm_mode_config_init(drm);

Try to do drmm_mode_config_init() first. The initialization of planes, 
crtcs and encoders requires it. See [1] for how other drivers order 
these calls.

> 	if (ret)
> 		return dev_err_probe(dev, ret, "Failed to initialize mode configuration\n");
> @@ -530,13 +611,13 @@ static int appletbdrm_setup_mode_config(struct appletbdrm_device *adev)
> 	if (ret)
> 		return dev_err_probe(dev, ret, "Failed to set non-desktop property\n");
>
> -	ret = drm_simple_display_pipe_init(drm, &adev->pipe, &appletbdrm_pipe_funcs,
> -					   appletbdrm_formats, ARRAY_SIZE(appletbdrm_formats),
> -					   NULL, &adev->connector);
> +	ret = drm_connector_attach_encoder(connector, encoder);
> +
> 	if (ret)
> 		return dev_err_probe(dev, ret, "Failed to initialize simple display pipe\n");
>
> -	drm_plane_enable_fb_damage_clips(&adev->pipe.plane);

> +	drm_plane_helper_add(primary_plane, &appletbdrm_primary_plane_helper_funcs);

This line can be removed. You've already set the plane helpers a few 
lines above.

> +	drm_plane_enable_fb_damage_clips(&adev->primary_plane);

And this call should better be done next to the plane init. The code at 
[1] again gives you an example of the preferable order.

Best regards
Thomas

[1] 
https://elixir.bootlin.com/linux/v6.13.2/source/drivers/gpu/drm/tiny/bochs.c#L606

>
> 	drm_mode_config_reset(drm);
>
>
> The commit history having both old and new revisions of the driver is here:
>
> https://github.com/AdityaGarg8/apple-touchbar-drv/blob/atomic/usr/src/apple-touchbar-advanced-0.1/appletbdrm.c
>
> Thanks
> Aditya

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [RFC PATCH v5 10/10] drm/tiny: add driver for Apple Touch Bars in x86 Macs

[Aditya Garg]

Hi Thomas

> On 17 Feb 2025, at 1:34 PM, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> 
> Hi Aditya,
> 
> the code looks correct overall. There's one place where I think it fails. See below.
> 
> Am 15.02.25 um 14:43 schrieb Aditya Garg:
> [...]
>> DEFINE_DRM_GEM_FOPS(appletbdrm_drm_fops);
>> @@ -484,10 +537,38 @@ static const struct drm_driver appletbdrm_drm_driver = {
>> static int appletbdrm_setup_mode_config(struct appletbdrm_device *adev)
>> {
>>    struct drm_connector *connector = &adev->connector;
>> +    struct drm_plane *primary_plane;
>> +    struct drm_crtc *crtc;
>> +    struct drm_encoder *encoder;
>>    struct drm_device *drm = &adev->drm;
>>    struct device *dev = adev->dev;
>>    int ret;
>> 
>> +    primary_plane = &adev->primary_plane;
>> +    ret = drm_universal_plane_init(drm, primary_plane, 0,
>> +                       &appletbdrm_primary_plane_funcs,
>> +                       appletbdrm_primary_plane_formats,
>> +                       ARRAY_SIZE(appletbdrm_primary_plane_formats),
>> +                       NULL,
>> +                       DRM_PLANE_TYPE_PRIMARY, NULL);
>> +    if (ret)
>> +        return ret;
>> +    drm_plane_helper_add(primary_plane, &appletbdrm_primary_plane_helper_funcs);
>> +
>> +    crtc = &adev->crtc;
>> +    ret = drm_crtc_init_with_planes(drm, crtc, primary_plane, NULL,
>> +                    &appletbdrm_crtc_funcs, NULL);
>> +    if (ret)
>> +        return ret;
>> +    drm_crtc_helper_add(crtc, &appletbdrm_crtc_helper_funcs);
>> +
>> +    encoder = &adev->encoder;
>> +    ret = drm_encoder_init(drm, encoder, &appletbdrm_encoder_funcs,
>> +                   DRM_MODE_ENCODER_DAC, NULL);
>> +    if (ret)
>> +        return ret;
>> +    encoder->possible_crtcs = drm_crtc_mask(crtc);
>> +
>>    ret = drmm_mode_config_init(drm);
> 
> Try to do drmm_mode_config_init() first. The initialization of planes, crtcs and encoders requires it. See [1] for how other drivers order these calls.

Indeed that was the issue. Thanks a lot for giving me time to fix this. You are a lifesaver!
> 
>>    if (ret)
>>        return dev_err_probe(dev, ret, "Failed to initialize mode configuration\n");
>> @@ -530,13 +611,13 @@ static int appletbdrm_setup_mode_config(struct appletbdrm_device *adev)
>>    if (ret)
>>        return dev_err_probe(dev, ret, "Failed to set non-desktop property\n");
>> 
>> -    ret = drm_simple_display_pipe_init(drm, &adev->pipe, &appletbdrm_pipe_funcs,
>> -                       appletbdrm_formats, ARRAY_SIZE(appletbdrm_formats),
>> -                       NULL, &adev->connector);
>> +    ret = drm_connector_attach_encoder(connector, encoder);
>> +
>>    if (ret)
>>        return dev_err_probe(dev, ret, "Failed to initialize simple display pipe\n");
>> 
>> -    drm_plane_enable_fb_damage_clips(&adev->pipe.plane);
> 
>> +    drm_plane_helper_add(primary_plane, &appletbdrm_primary_plane_helper_funcs);
> 
> This line can be removed. You've already set the plane helpers a few lines above.
> 
>> +    drm_plane_enable_fb_damage_clips(&adev->primary_plane);
> 
> And this call should better be done next to the plane init. The code at [1] again gives you an example of the preferable order.
> 
> Best regards
> Thomas
> 
> [1] https://elixir.bootlin.com/linux/v6.13.2/source/drivers/gpu/drm/tiny/bochs.c#L606
> 
>> 
>>    drm_mode_config_reset(drm);
>> 
>> 
>> The commit history having both old and new revisions of the driver is here:
>> 
>> https://github.com/AdityaGarg8/apple-touchbar-drv/blob/atomic/usr/src/apple-touchbar-advanced-0.1/appletbdrm.c
>> 
>> Thanks
>> Aditya
> 
> --
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Frankenstrasse 146, 90461 Nuernberg, Germany
> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
> HRB 36809 (AG Nuernberg)
> 

Re: [PATCH v5 0/10] Touch Bar support for T2 Macs

[Aditya Garg]

Hi Maintainers

It has been 2 weeks but I still haven't received a single reply on this version of the patch series. Consider this email as a friendly reminder.

> On 17 Aug 2024, at 5:15 PM, Aditya Garg <gargaditya08@live.com> wrote:
> 
> Hi Maintainers
> 
> The Touch Bars found on x86 Macs support two USB configurations: one
> where the device presents itself as a HID keyboard and can display
> predefined sets of keys, and one where the operating system has full
> control over what is displayed.
> 
> This patch series adds support for both the configurations.
> 
> The hid-appletb-bl driver adds support for the backlight of the Touch Bar.
> This enables the user to control the brightness of the Touch Bar from
> userspace. The Touch Bar supports 3 modes here: Max brightness, Dim and Off.
> So, daemons, used to manage Touch Bar can easily manage these modes by writing
> to /sys/class/backlight/appletb_backlight/brightness. It is needed by both the
> configurations of the Touch Bar.
> 
> The hid-appletb-kbd adds support for the first (predefined keys) configuration.
> There are 4 modes here: Esc key only, Fn mode, Media keys and No keys.
> Mode can be changed by writing to /sys/bus/hid/drivers/hid-appletb-kbd/<dev>/mode
> This configuration is what Windows uses with the official Apple Bootcamp drivers.
> 
> Rest patches support the second configuration, where the OS has full control
> on what's displayed on the Touch Bar. It is achieved by the patching the
> hid-multitouch driver to add support for touch feedback from the Touch Bar
> and the appletbdrm driver, that displays what we want to on the Touch Bar.
> This configuration is what macOS uses.
> 
> The appletbdrm driver is based on the similar driver made for Windows by
> imbushuo [1].
> 
> Currently, a daemon named tiny-dfr [2] is being used to display function keys
> and media controls using the second configuration for both Apple Silicon and
> T2 Macs.
> 
> A daemon for the first configuration is being developed, but that's a userspace
> thing.
> 
> [1]: https://github.com/imbushuo/DFRDisplayKm
> [2]: https://github.com/WhatAmISupposedToPutHere/tiny-dfr
> 
> v2:
> 1. Cleaned up some code in the hid-appletb-kbd driver.
> 2. Fixed wrong subject in drm/format-helper patch.
> 3. Fixed Co-developed-by wrongly added to hid-multitouch patch.
> 
> v3:
> 1. Fixed key mapping for Function keys in hid-appletb-kbd driver.
> 
> v4:
> 1. Added support for fn key toggle in the hid-appletb-kbd driver.
> 
> v5:
> 1. Do required changes to hid-appletb-bl as requested by upstream.
> 
> Aditya Garg (1):
> HID: hid-appletb-kbd: add support for fn toggle between media and
>  function mode
> 
> Kerem Karabay (9):
> HID: hid-appletb-bl: add driver for the backlight of Apple Touch Bars
> HID: hid-appletb-kbd: add driver for the keyboard mode of Apple Touch
>  Bars
> HID: multitouch: support getting the contact ID from
>  HID_DG_TRANSDUCER_INDEX fields
> HID: multitouch: support getting the tip state from HID_DG_TOUCH
>  fields
> HID: multitouch: take cls->maxcontacts into account for devices
>  without a HID_DG_CONTACTMAX field too
> HID: multitouch: allow specifying if a device is direct in a class
> HID: multitouch: add device ID for Apple Touch Bars
> drm/format-helper: Add conversion from XRGB8888 to BGR888 conversion
> drm/tiny: add driver for Apple Touch Bars in x86 Macs
> 
> .../ABI/testing/sysfs-driver-hid-appletb-kbd  |  13 +
> MAINTAINERS                                   |   6 +
> drivers/gpu/drm/drm_format_helper.c           |  54 ++
> .../gpu/drm/tests/drm_format_helper_test.c    |  81 +++
> drivers/gpu/drm/tiny/Kconfig                  |  12 +
> drivers/gpu/drm/tiny/Makefile                 |   1 +
> drivers/gpu/drm/tiny/appletbdrm.c             | 624 ++++++++++++++++++
> drivers/hid/Kconfig                           |  22 +
> drivers/hid/Makefile                          |   2 +
> drivers/hid/hid-appletb-bl.c                  | 207 ++++++
> drivers/hid/hid-appletb-kbd.c                 | 432 ++++++++++++
> drivers/hid/hid-multitouch.c                  |  60 +-
> drivers/hid/hid-quirks.c                      |   8 +-
> include/drm/drm_format_helper.h               |   3 +
> 14 files changed, 1509 insertions(+), 16 deletions(-)
> create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-appletb-kbd
> create mode 100644 drivers/gpu/drm/tiny/appletbdrm.c
> create mode 100644 drivers/hid/hid-appletb-bl.c
> create mode 100644 drivers/hid/hid-appletb-kbd.c
> 
> --
> 2.43.0
> 

Re: [PATCH v5 0/10] Touch Bar support for T2 Macs

[Jiri Kosina]

On Sat, 31 Aug 2024, Aditya Garg wrote:

> Hi Maintainers
> 
> It has been 2 weeks but I still haven't received a single reply on this 
> version of the patch series. Consider this email as a friendly reminder.

I think it makes most sense to take this whole set through hid.git, but 
for that, I'd like to get Acked-by/Reviewed-by for patches 9 and 10 (drm 
bits).

Dave, Daniel, .. ?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v5 0/10] [DRM REVIEW NEEDED] Touch Bar support for T2 Macs

[Aditya Garg]

Hi Jiri

Do I have to send the drm patches again to get their Acked-by/Reviewed-by?
Although I have already cced the DRM maintainers in this patchset.

> On 11 Sep 2024, at 5:51 PM, Jiri Kosina <jikos@kernel.org> wrote:
> 
> On Sat, 31 Aug 2024, Aditya Garg wrote:
> 
>> Hi Maintainers
>> 
>> It has been 2 weeks but I still haven't received a single reply on this 
>> version of the patch series. Consider this email as a friendly reminder.
> 
> I think it makes most sense to take this whole set through hid.git, but 
> for that, I'd like to get Acked-by/Reviewed-by for patches 9 and 10 (drm 
> bits).
> 
> Dave, Daniel, .. ?
> 
> Thanks,
> 
> -- 
> Jiri Kosina
> SUSE Labs
> 

[WHY SUCH DELAY!] Touch Bar support for T2 Macs

[Aditya Garg]

It has been more than a month since I've sent this patch set and I haven't got a clear yes or not for the same. I understand maintainers are busy people, but I'd really appreciate if I get some response for this series of patches from the HID and DRM maintainers.

Thanks
Aditya

> On 11 Sep 2024, at 5:51 PM, Jiri Kosina <jikos@kernel.org> wrote:
> 
> On Sat, 31 Aug 2024, Aditya Garg wrote:
> 
>> Hi Maintainers
>> 
>> It has been 2 weeks but I still haven't received a single reply on this
>> version of the patch series. Consider this email as a friendly reminder.
> 
> I think it makes most sense to take this whole set through hid.git, but
> for that, I'd like to get Acked-by/Reviewed-by for patches 9 and 10 (drm
> bits).
> 
> Dave, Daniel, .. ?
> 
> Thanks,
> 
> --
> Jiri Kosina
> SUSE Labs
> 

Re: [WHY SUCH DELAY!] Touch Bar support for T2 Macs

[Jiri Kosina]

On Thu, 26 Sep 2024, Aditya Garg wrote:

> It has been more than a month since I've sent this patch set and I 
> haven't got a clear yes or not for the same. I understand maintainers 
> are busy people, but I'd really appreciate if I get some response for 
> this series of patches from the HID and DRM maintainers.

Just to reiterate -- I am waiting for Ack from the DRM people and will 
then take it through hid.git.

Dave, who'd be the best person to do this from the DRM side please?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [WHY SUCH DELAY!] Touch Bar support for T2 Macs

[Aditya Garg]

> On 26 Sep 2024, at 11:33 PM, Jiri Kosina <jikos@kernel.org> wrote:
> 
> On Thu, 26 Sep 2024, Aditya Garg wrote:
> 
>> It has been more than a month since I've sent this patch set and I
>> haven't got a clear yes or not for the same. I understand maintainers
>> are busy people, but I'd really appreciate if I get some response for
>> this series of patches from the HID and DRM maintainers.
> 
> Just to reiterate -- I am waiting for Ack from the DRM people and will
> then take it through hid.git.

So I should assume the HID portion is fit to be merged now?

> 
> Dave, who'd be the best person to do this from the DRM side please?
> 
> Thanks,
> 
> --
> Jiri Kosina
> SUSE Labs
> 

Re: [WHY SUCH DELAY!] Touch Bar support for T2 Macs

[Jiri Kosina]

On Thu, 26 Sep 2024, Aditya Garg wrote:

> >> It has been more than a month since I've sent this patch set and I
> >> haven't got a clear yes or not for the same. I understand maintainers
> >> are busy people, but I'd really appreciate if I get some response for
> >> this series of patches from the HID and DRM maintainers.
> > 
> > Just to reiterate -- I am waiting for Ack from the DRM people and will
> > then take it through hid.git.
> 
> So I should assume the HID portion is fit to be merged now?

hid-appletb-* I have gone through, and it looks all good to me.

Benjamin, could you please explicitly provide your Ack for the multitouch 
changes?

-- 
Jiri Kosina
SUSE Labs

Re: [WHY SUCH DELAY!] Touch Bar support for T2 Macs

[Dave Airlie]

On Fri, 27 Sept 2024 at 04:03, Jiri Kosina <jikos@kernel.org> wrote:
>
> On Thu, 26 Sep 2024, Aditya Garg wrote:
>
> > It has been more than a month since I've sent this patch set and I
> > haven't got a clear yes or not for the same. I understand maintainers
> > are busy people, but I'd really appreciate if I get some response for
> > this series of patches from the HID and DRM maintainers.
>
> Just to reiterate -- I am waiting for Ack from the DRM people and will
> then take it through hid.git.
>
> Dave, who'd be the best person to do this from the DRM side please?
>

I think Thomas or Maxime could take a look, though it might be easier
to submit this in pieces, does it really need to all go in at once?

Dave.

Re: [WHY SUCH DELAY!] Touch Bar support for T2 Macs

[Jiri Kosina]

On Fri, 27 Sep 2024, Dave Airlie wrote:

> > > It has been more than a month since I've sent this patch set and I
> > > haven't got a clear yes or not for the same. I understand maintainers
> > > are busy people, but I'd really appreciate if I get some response for
> > > this series of patches from the HID and DRM maintainers.
> >
> > Just to reiterate -- I am waiting for Ack from the DRM people and will
> > then take it through hid.git.
> >
> > Dave, who'd be the best person to do this from the DRM side please?
> >
> 
> I think Thomas or Maxime could take a look, though it might be easier
> to submit this in pieces, does it really need to all go in at once?

I'm fine either way, once DRM folks either give their Ack, or express 
their desire to separate it out, I'll follow :)

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [WHY SUCH DELAY!] Touch Bar support for T2 Macs

[Benjamin Tissoires]

On Sep 26 2024, Aditya Garg wrote:
> It has been more than a month since I've sent this patch set and I haven't got a clear yes or not for the same. I understand maintainers are busy people, but I'd really appreciate if I get some response for this series of patches from the HID and DRM maintainers.

Please look at the time. You sent this a month ago, while v6.11-rc4 was
out the next day.

I was personally taking time off and came back at the end of August
(roughly at rc6).
This is then a problematic time to merge new drivers because they won't
have enough time to be in linux-next before they are sent to Linus.

Also some subsystem are more strict in term of what can go in and when,
and IIRC drm had been strict regarding that because it is heavily making
use of sub-subsystems, and they need time to put back everything
together for sending it to Linus.

Then, when -rc7 is out, I bet no maintainers will take new drivers for
the next 3 weeks:
- the final version will be out the next week, meaning not enough time
  to test in linux-next
- while the merge window is opened, we are not allowed to take any N+1
  material, or this will break everybody testing system.

Merge window is closing this Sunday, but I realized that there was a
regression in HID-BPF which breaks the CI, so at the earliest your new
drivers will be taken at the end of next week.

So yeah, I understand it can be frustrating somehow, but please avoid
all caps in your subject prefix, this really put other people on their
nerves for nothing.

I have a few objections to your series, I'm going to answer individually
in the patches.

Cheers,
Benjamin

> 
> Thanks
> Aditya
> 
> > On 11 Sep 2024, at 5:51 PM, Jiri Kosina <jikos@kernel.org> wrote:
> > 
> > On Sat, 31 Aug 2024, Aditya Garg wrote:
> > 
> >> Hi Maintainers
> >> 
> >> It has been 2 weeks but I still haven't received a single reply on this
> >> version of the patch series. Consider this email as a friendly reminder.
> > 
> > I think it makes most sense to take this whole set through hid.git, but
> > for that, I'd like to get Acked-by/Reviewed-by for patches 9 and 10 (drm
> > bits).
> > 
> > Dave, Daniel, .. ?
> > 
> > Thanks,
> > 
> > --
> > Jiri Kosina
> > SUSE Labs
> > 

Re: [WHY SUCH DELAY!] Touch Bar support for T2 Macs

[Aditya Garg]

Hi Benjamin

I’ve noticed the DRM folks also have requested a lot of changes in the drm bit.

I really appreciate that finally we got some review on the patches, irrespective of whether it was acceptance/rejection/change.

Anyways, since the hid-multitouch changes are needed only for the DRM driver, I’ll for the time being fix up the basic Touch Bar mode driver for upstream. Let’s get it done first, and at least make the upstream kernels be able to at least use the Touch Bar, even if it is in basic mode. I’ll figure out the other macOS mode driver later, and will send it some time later then.

> On 27 Sep 2024, at 8:52 PM, Benjamin Tissoires <bentiss@kernel.org> wrote:
> 
> On Sep 26 2024, Aditya Garg wrote:
>> It has been more than a month since I've sent this patch set and I haven't got a clear yes or not for the same. I understand maintainers are busy people, but I'd really appreciate if I get some response for this series of patches from the HID and DRM maintainers.
> 
> Please look at the time. You sent this a month ago, while v6.11-rc4 was
> out the next day.
> 
> I was personally taking time off and came back at the end of August
> (roughly at rc6).
> This is then a problematic time to merge new drivers because they won't
> have enough time to be in linux-next before they are sent to Linus.
> 
> Also some subsystem are more strict in term of what can go in and when,
> and IIRC drm had been strict regarding that because it is heavily making
> use of sub-subsystems, and they need time to put back everything
> together for sending it to Linus.
> 
> Then, when -rc7 is out, I bet no maintainers will take new drivers for
> the next 3 weeks:
> - the final version will be out the next week, meaning not enough time
> to test in linux-next
> - while the merge window is opened, we are not allowed to take any N+1
> material, or this will break everybody testing system.
> 
> Merge window is closing this Sunday, but I realized that there was a
> regression in HID-BPF which breaks the CI, so at the earliest your new
> drivers will be taken at the end of next week.
> 
> So yeah, I understand it can be frustrating somehow, but please avoid
> all caps in your subject prefix, this really put other people on their
> nerves for nothing.

For the above, I understand and was well aware of the rc6/7 merge windows during those days, and that was the reason I was waiting quite patiently.

Talking about caps in the subject, I apologise for that, my intention was definitely not to "put other people on their nerves for nothing.”

Cheers

Aditya