Re: [PATCH v3 1/3] drm/loongson: Add DRM Driver for Loongson 7A1000 bridge chip

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Chenyang,

I browsed the code on lore and noticed a few things and thought it
better to bring it to your attention now.

The general structure of the drivers seems good and coding style is
fine. The feedback is mostly stuff we have decided to do different over
time, so likely because you based the driver on some older driver.
And it can be hard to follow all the refactoring going on - something
that you get for free (almost) when is is mainlined.

1) Use drm_ for logging whereever possible (need drm_device)
2) Do not use irq mid-layer support in drm_driver, it is about to be
   legacy only. In other words avoid using drm_irq* stuff.
3) Look at drm_drv to see code snippet how to use the drmm*
   infrastructure. It will save you some cleanup and in general make the
   driver more stable
4) Sort includes alphabetically, and split them on in blocks.
   <linux/*> is one block
   <newline>
   <drm/drm_*> is next block
5) Put entry in makefile in alphabetical order
6) You most like can use the simple_encoder stuff we have today
7) The *_load and *_unlod names where used in the past. Maybe be
   inspired by some newer driver here. _load functiosn is something used
   by legacy drivers so it confuses me a little.

I look forward to see next revision of the patch-set.
And sorry for not providing these high-level feedback issues before - I
have not had time to look at your driver.

	Sam



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux