Hi Sam,
On Sat 04 Jan 20, 20:20, Sam Ravnborg wrote:
Good looking driver. Well structured in a number of relevant files. A few comments in the following. Some parts I fail to follow - due to my lack of DRM knowledge. So all in all - only trivial comments.
Thanks for the review and the friendly feedback!
With these fixed you can add: Acked-by: Sam Ravnborg sam@ravnborg.org
I'll take most of your suggestions in for the next version and there's just one point where I disagree:
+struct logicvc_drm {
- const struct logicvc_drm_caps *caps;
- struct logicvc_drm_config config;
- struct drm_device *drm;
Modern drm drivers are expected to embed drm_device. See example in drm_drv.c
Well, I see lots of modern drivers that use drm_dev_alloc, including vc4 that I took as a reference.
My understanding is that embedding the struct is a recommendation but drm_dev_alloc is still quite valid and that the choice is left open. Quoting drm_drv.c:
* It is recommended that drivers embed &struct drm_device into their own device * structure. * * Drivers that do not want to allocate their own device struct * embedding &struct drm_device can call drm_dev_alloc() instead.
In my case, I like the fact that drm_dev_alloc correctly wraps drm_dev_init and drmm_add_final_kfree (and I'd rather not add & all around unless I'm obliged to ;)
Cheers,
Paul