On Wed, Jun 05, 2013 at 04:13:01AM +0200, Laurent Pinchart wrote:
Hi Ville,
Thank you for the patch.
On Tuesday 04 June 2013 10:58:35 ville.syrjala@linux.intel.com wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
drivers/gpu/drm/drm_crtc.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index f00ba75..f1f11e1 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -795,6 +795,21 @@ void drm_encoder_cleanup(struct drm_encoder *encoder) } EXPORT_SYMBOL(drm_encoder_cleanup);
+/**
- drm_plane_init - Initialise a new plane object
- @dev: DRM device
- @plane: plane object to init
- @possible_crtcs: bitmask of possible CRTCs
- @funcs: callbacks for the new plane
- @formats: array of supported formats (%DRM_FORMAT_*)
- @format_count: number of elements in @formats
- @priv: plane is private (hidden from userspace)?
- Inits a new object created as base part of an driver plane object.
s/an driver/a driver/
You can blame the guy who wrote the drm_crtc_init() docs :)
- RETURNS:
- Zero on success, error code on failure.
- */
int drm_plane_init(struct drm_device *dev, struct drm_plane *plane, unsigned long possible_crtcs, const struct drm_plane_funcs *funcs, @@ -843,6 +858,13 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane, } EXPORT_SYMBOL(drm_plane_init);
+/**
- drm_plane_cleanup - Cleans up the core plane usage.
Nitpicking, you could remove the full stop at the end of the line to be consistent with the other two kerneldoc blocks.
And s/Cleans/Clean/
Same deal here. I'll fix up the originals as well...
- @plane: plane to cleanup
- Cleanup @plane. Removes from drm modesetting space
- does NOT free object, caller does that.
As this is documentation, I'd use a more verbose style.
This function clean up @plane and removes it from the DRM mode setting core. Note that the function does *not* free the plane structure itself, this is the responsibility of the caller.
Again just copy-pasted from somewhere.
- */
void drm_plane_cleanup(struct drm_plane *plane) { struct drm_device *dev = plane->dev; @@ -859,6 +881,15 @@ void drm_plane_cleanup(struct drm_plane *plane) } EXPORT_SYMBOL(drm_plane_cleanup);
+/**
- drm_plane_force_disable - Forcibly disable a plane
- @plane: plane to disable
- Forces the plane to be disabled.
This feels a bit unclear to me. In particular, how is "force_disable" different from just disabling the plane ? Maybe the function should be renamed to drm_plane_disable(), and the documentation updated to mention that the function just disables the plane and disassociate with from its frame buffer.
Normal disable would happen in response to the setplane ioctl w/ NULL fb, whereas this guy is meant more for unsolicited disable.
I'm afraid if I call it drm_plane_disable() someone will send a patch to call it from setplane, or people start to call it from drivers' disable_plane hook.