Hi Sam and Daniel, Thank you very much for reviewing the code. I will squish all the commits and send version 3, so it will be easier for you to review.
Anitha
-----Original Message----- From: Sam Ravnborg sam@ravnborg.org Sent: Wednesday, July 15, 2020 10:07 AM To: Daniel Vetter daniel@ffwll.ch Cc: Chrisanthus, Anitha anitha.chrisanthus@intel.com; Vetter, Daniel daniel.vetter@intel.com; intel-gfx@lists.freedesktop.org; Dea, Edmund J edmund.j.dea@intel.com; dri-devel@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH v2 00/59] Add support for KeemBay DRM driver
On Wed, Jul 15, 2020 at 05:05:49PM +0200, Daniel Vetter wrote:
Hi Anitha
On Tue, Jul 14, 2020 at 01:56:46PM -0700, Anitha Chrisanthus wrote:
This is a new DRM driver for Intel's KeemBay SOC. The SoC couples an ARM Cortex A53 CPU with an Intel Movidius VPU.
This driver is tested with the KMB EVM board which is the refernce baord for Keem Bay SOC. The SOC's display pipeline is as follows
+--------------+ +---------+ +-----------------------+ |LCD controller| -> |Mipi DSI | -> |Mipi to HDMI Converter | +--------------+ +---------+ +-----------------------+
LCD controller and Mipi DSI transmitter are part of the SOC and mipi to HDMI converter is ADV7535 for KMB EVM board.
The DRM driver is a basic KMS atomic modesetting display driver and has no 2D or 3D graphics.It calls into the ADV bridge driver at the connector level.
Only 1080p resolution and single plane is supported at this time. The usecase is for debugging video and camera outputs.
Device tree patches are under review here https://lore.kernel.org/linux-arm-kernel/20200708175020.194436-1-
daniele.alessandrelli@linux.intel.com/T/
Cool, new driver, thanks a lot for submitting.
Changes since v1:
- Removed redundant license text, updated license
- Rearranged include blocks
- renamed global vars and removed extern in c
- Used upclassing for dev_private
- Used drm_dev_init in drm device create (will be updated to use devm_drm_dev_alloc() in a separate patch later as kmb driver is currently developed on 5.4 kernel)
drm moves fairly quickly, please develop the upstream submission on top of linux-next or similar. We constantly add new helpers to simplify drivers, and we expect new driver submissions to be up to date with all that.
Seconded!
Another thing: From your description it sounds like it's a very simple driver, just a single plane/crtc, nothing fancy, plus adv bridge output. Is the driver already using simple display pipeline helpers? I think that would be an ideal fit and probably greatly simplifies the code.
- minor cleanups
The patch series looks like it contains the entire development history, or at least large chunks of it. That's useful for you, but for upstreaming the main focues (especially for smaller drivers) is whether your driver uses all the available helpers and integrations correctly. And for that it's much easier if the history is cleaned up, and all intermediate steps removed.
And also agree on this point. The submission could be split up in a few patches where the split is file based. So only with the latest patch, containing Makefile + Kconfig,the driver i buildable. This would ease review as one looses focus when trying to review 1000+ lines in one mail.
You will loose some of the internal history - but if important keep relevant parts in sensible comments.
Sam