Re: [PATCH v1 0/1] Added GOP driver for DisplayLink-based Universal USB Docks to edk2-platforms

["Michael D Kinney" <michael.d.kinney@intel.com>]

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

Hi Andy,

Thanks for the contribution.  Is this patch for the edk2-platform repo?  The email subject can help clarify the intended repo.

Also, we prefer the patches to be provided inline instead of an attachment.  You can use the
'git send-email' feature to do this.  Here are a couple links to useful wikis.

https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process
https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers

I also noticed looking at the patch file that there are some inconsistencies with the
EDK II C Coding Standards.  Most are around function headers and indents.  Here is a link
to the latest version:

https://www.gitbook.com/download/pdf/book/edk2-docs/edk-ii-c-coding-standards-specification

I see a few #if 0 and commented out lines of code and TODO comments.  Can you please clean these up
and if needed add Tianocore Bugzillas for future enhancements/work items for this driver.

https://bugzilla.tianocore.org/

I also noticed that you are using named fields to initialize C structures:

STATIC CONST struct VideoMode ModeData[] = {
+  {
+     // 640 x 480 @ 60Hz
+    .Reserved2 = 2,
+    .PixelClock = 2517,
+    .HActive = 640,
+    .HBlanking = 160,
+    .HSyncOffset = 16,
+    .HSyncWidth = 96,

I do like this style because it has better self documentation of the field being
initialized to a value.  However, I do not see this style in the rest of the EDK II
sources, so I am concerned that we may have required compilers for the EDK II
that may not support this syntax.

If we find all the supported compilers do support this syntax, this would be
a great addition to the EDK II C Coding Standards.

I am curious how this driver interacts with the ConSplitter when displays
are hot added and hot removed.  I see a reference to a feature that appears
to sync the GOP content between frame buffers when a display is hot added.
I would be better if the common code in the ConSplitter handled these types
of operations instead of putting that code in individual GOP drivers. I have
added Ray to this thread who may have ideas on how to handle these hot
add events.

Thanks,

Mike

From: Andy Hayes [mailto:andy.hayes@displaylink.com]
Sent: Wednesday, August 14, 2019 7:44 AM
To: devel@edk2.groups.io
Cc: Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: [PATCH v1 0/1] Added GOP driver for DisplayLink-based Universal USB Docks to edk2-platforms

>From 4a42eb997aeb3699217b40bf3bc47dec56458847 Mon Sep 17 00:00:00 2001
From: "Andy Hayes" < andy.hayes@displaylink.com<mailto:andy.hayes@displaylink.com> >
Date: Wed, 14 Aug 2019 15:30:20 +0100
Subject: [PATCH v1 0/1] Added GOP graphics driver for DisplayLink-based Universal USB Docks to edk2-platforms

andy.hayes@displaylink.com<mailto:andy.hayes@displaylink.com> (1):
  Added GOP driver for USB Docks which use DisplayLink chips.

.../DisplayLinkPkg/DisplayLinkPkg.dsc         |   61 +
.../DisplayLinkGop/DisplayLinkGopDxe.inf      |   63 +
.../DisplayLinkPkg/DisplayLinkGop/Edid.h      |  129 ++
.../DisplayLinkGop/UsbDescriptors.h           |  109 ++
.../DisplayLinkGop/UsbDisplayLink.h           |  284 +++++
.../DisplayLinkGop/CapabilityDescriptor.c     |  137 ++
.../DisplayLinkGop/ComponentName.c            |  235 ++++
.../DisplayLinkPkg/DisplayLinkGop/Edid.c      |  598 +++++++++
.../DisplayLinkPkg/DisplayLinkGop/Gop.c       |  587 +++++++++
.../DisplayLinkGop/UsbDescriptors.c           |  144 +++
.../DisplayLinkGop/UsbDisplayLink.c           | 1109 +++++++++++++++++
.../DisplayLinkGop/UsbTransfer.c              |  180 +++
.../DisplayLinkGop/VideoModes.c               |  254 ++++
Drivers/DisplayLink/DisplayLinkPkg/ReadMe.md  |   77 ++
Maintainers.txt                               |    5 +
15 files changed, 3972 insertions(+)
create mode 100644 Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkPkg.dsc
create mode 100644 Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/DisplayLinkGopDxe.inf
create mode 100644 Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/Edid.h
create mode 100644 Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/UsbDescriptors.h
create mode 100644 Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/UsbDisplayLink.h
create mode 100644 Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/CapabilityDescriptor.c
create mode 100644 Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/ComponentName.c
create mode 100644 Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/Edid.c
create mode 100644 Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/Gop.c
create mode 100644 Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/UsbDescriptors.c
create mode 100644 Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/UsbDisplayLink.c
create mode 100644 Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/UsbTransfer.c
create mode 100644 Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/VideoModes.c
create mode 100644 Drivers/DisplayLink/DisplayLinkPkg/ReadMe.md

--
2.17.1

[-- Attachment #2: Type: text/html, Size: 50481 bytes --]