Thanks much Sam for reviewing the code so quickly. I'll address your reviews in v2. Anitha > -----Original Message----- > From: Sam Ravnborg <sam@xxxxxxxxxxxx> > Sent: Wednesday, July 1, 2020 12:01 AM > To: Chrisanthus, Anitha <anitha.chrisanthus@xxxxxxxxx> > Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx; Paauwe, Bob J <bob.j.paauwe@xxxxxxxxx>; > Dea, Edmund J <edmund.j.dea@xxxxxxxxx>; Vetter, Daniel > <daniel.vetter@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Vivi, Rodrigo > <rodrigo.vivi@xxxxxxxxx> > 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
