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

[Sam Ravnborg]

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