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