Messages in this thread | | | Date | Fri, 3 Apr 2020 11:13:44 +0200 | From | Paul Kocialkowski <> | Subject | Re: [PATCH v4 2/3] drm: Add support for the LogiCVC display controller |
| |
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
-- Paul Kocialkowski, Bootlin Embedded Linux and kernel engineering https://bootlin.com [unhandled content-type:application/pgp-signature] | |