RE: [PATCH 00/59] Add support for Keem Bay DRM driver

From: "Chrisanthus, Anitha" <anitha.chrisanthus@intel.com>
To: Sam Ravnborg <sam@ravnborg.org>
Cc: "intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"Dea, Edmund J" <edmund.j.dea@intel.com>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	"Vivi,  Rodrigo" <rodrigo.vivi@intel.com>,
	"Vetter, Daniel" <daniel.vetter@intel.com>
Subject: RE: [PATCH 00/59] Add support for Keem Bay DRM driver
Date: Wed, 1 Jul 2020 21:53:40 +0000	[thread overview]
Message-ID: <BY5PR11MB41827EDF1C35E2AEA25906B78C6C0@BY5PR11MB4182.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20200701070102.GA1494555@ravnborg.org>

Thanks much Sam for reviewing the code so quickly. I'll address your reviews in v2.

Anitha

> -----Original Message-----
> From: Sam Ravnborg <sam@ravnborg.org>
> Sent: Wednesday, July 1, 2020 12:01 AM
> To: Chrisanthus, Anitha <anitha.chrisanthus@intel.com>
> Cc: dri-devel@lists.freedesktop.org; Paauwe, Bob J <bob.j.paauwe@intel.com>;
> Dea, Edmund J <edmund.j.dea@intel.com>; Vetter, Daniel
> <daniel.vetter@intel.com>; intel-gfx@lists.freedesktop.org; Vivi, Rodrigo
> <rodrigo.vivi@intel.com>
> Subject: Re: [PATCH 00/59] Add support for Keem Bay DRM driver
> 
> Hi Anitha.
> 
> On Tue, Jun 30, 2020 at 02:27:12PM -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.
> ...
> Nice and informative intro - thanks.
> 
> The patchset looks more like a dump of current state and less like a
> logical set of changes - as the few I looked at changed code introduced
> in earlier patches.
> So I ended up applying all patches to a local branch.
> Very good to post an WIP version so you can capture some early
> feedback.
> It is never fun to think something is finished and then address a lot of
> feedback.
> 
> 
> Some general remarks after reading/browsing some of the code:
> - Embeds drm_device - good
> - Uses SPDX, good. But includes also a license text. Can we
>   get rid of the redundandt license text?
> - Some of the inline functions in kmb_drv.h is too large
>   (kmb_write_bits_mipi())
> - There is stray code commented out using "//" here and there - shall be
>   dropped.
> - Please arrange include files in blocks:
> #include <linux/...>
> 
> #include <video/...>
> 
> #include <drm/...>
> 
> #include "kmb_*"
> 
> Within each block sort alphabetially.
> 
> - Use a kmb_ prefix or similar for global variables - like under_flow
> - no extern in .c files - plane_status
> - Consider using an array for the clk's
> - In general use drm_info(), drm_err() for logging.
>   Yes, you will need to pass kmb_drm_private around to do so
> - Do not use drm_device->dev_private, it is deprecated. Use upclassing
> - kmb_driver can be slimmed a lot. See what recent drivers uses. There is
>   some nice defines so it is obvious what is not standard.
>   DRIVER_HAVE_IRQ is not relevant btw.
> - Start using drmm_* support. The way kmb_drm_private is allocated
>   is sub-optimal
> 
> The above was my fist drive-by comments - but do not be discouraged.
> For the most part they should be easy to address.
> Looking forward for other feedback and for v2.
> 
> Let me know if the above triggers any questions.
> 
> > +--------------+    +---------+    +-----------------------+
> > |LCD controller| -> |Mipi DSI | -> |Mipi to HDMI Converter |
> > +--------------+    +---------+    +-----------------------+
> 
> Question to dri-devel people:
> Would the Mipi DSI be better represented outside the display driver?
> If yes, how?
> 
> 	Sam
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel