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 >

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 (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