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@xxxxxxx> wrote:
> 
> Hi
> 
> Am 17.08.24 um 13:52 schrieb Aditya Garg:
>> From: Kerem Karabay <kekrby@xxxxxxxxx>
>> 
>> 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@xxxxxxxxx>
>> Signed-off-by: Aditya Garg <gargaditya08@xxxxxxxx>
>> ---
>>  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@xxxxxxxxx>
>> +L: dri-devel@xxxxxxxxxxxxxxxxxxxxx
>> +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@xxxxxxxxx>
>> + */
>> +
>> +#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@xxxxxxxxx>");
>> +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)