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
