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

[Chrisanthus, Anitha]

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

Anitha

> -----Original Message-----
> From: Sam Ravnborg <sam at ravnborg.org>
> Sent: Wednesday, July 1, 2020 12:01 AM
> To: Chrisanthus, Anitha <anitha.chrisanthus at intel.com>
> Cc: dri-devel at lists.freedesktop.org; Paauwe, Bob J <bob.j.paauwe at intel.com>;
> Dea, Edmund J <edmund.j.dea at intel.com>; Vetter, Daniel
> <daniel.vetter at intel.com>; intel-gfx at lists.freedesktop.org; Vivi, Rodrigo
> <rodrigo.vivi at 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