Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755380AbdGKIBm (ORCPT ); Tue, 11 Jul 2017 04:01:42 -0400 Received: from mail-lf0-f68.google.com ([209.85.215.68]:36142 "EHLO mail-lf0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754126AbdGKIBk (ORCPT ); Tue, 11 Jul 2017 04:01:40 -0400 Date: Tue, 11 Jul 2017 10:01:36 +0200 From: Daniel Vetter To: Peter Rosin Cc: linux-kernel@vger.kernel.org, Boris Brezillon , dri-devel@lists.freedesktop.org, Daniel Vetter Subject: Re: [PATCH v4 01/14] drm/atomic: export drm_atomic_replace_property_blob Message-ID: <20170711080136.7ywol2i272oq2mkk@phenom.ffwll.local> Mail-Followup-To: Peter Rosin , linux-kernel@vger.kernel.org, Boris Brezillon , dri-devel@lists.freedesktop.org, Daniel Vetter References: <1499343648-29695-1-git-send-email-peda@axentia.se> <1499343648-29695-2-git-send-email-peda@axentia.se> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1499343648-29695-2-git-send-email-peda@axentia.se> X-Operating-System: Linux phenom 4.11.0-1-amd64 User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3457 Lines: 91 On Thu, Jul 06, 2017 at 02:20:35PM +0200, Peter Rosin wrote: > While at it, add some words in the kernel-doc about the 'replaced' arg and > remove a faulty kernel-doc comment on the return value. > > Also remove a redundant return statement. > > Signed-off-by: Peter Rosin > --- > drivers/gpu/drm/drm_atomic.c | 17 +++++++++-------- > include/drm/drm_atomic.h | 4 ++++ > 2 files changed, 13 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 09ca662..b7d9696 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -414,13 +414,15 @@ EXPORT_SYMBOL(drm_atomic_set_mode_prop_for_crtc); > * @new_blob: the new blob to replace with > * @replaced: whether the blob has been replaced > * > - * RETURNS: > - * Zero on success, error code on failure > + * Note that you are required to initialize @replaced to false before the > + * call, since it is only set to true when the blob property is changed and > + * not set to false when the property is not changed. This enables a series > + * of calls to be made where you are interested in if any property is > + * replaced, but not care so much about exactly which of them was replaced. > */ > -static void > -drm_atomic_replace_property_blob(struct drm_property_blob **blob, > - struct drm_property_blob *new_blob, > - bool *replaced) > +void drm_atomic_replace_property_blob(struct drm_property_blob **blob, > + struct drm_property_blob *new_blob, > + bool *replaced) Yes I know I'm super-annoying, but this function now feels misplaced. It has nothing to do with atomic, it just replaces a pointer to a blob with anther pointer. I think it'd be much better if we move this function to drm_property.c, and rename it to drm_property_replace_blob. Second, instead of typing a huge paragraph explaining how replaced works, I think the better option would be to drop that parameter and instead return a boolean indicating whether the blob was replaced or not. That's a bit more work, but imo functions which are exported need to pass a higher barrier wrt api polish. Thanks, Daniel > { > struct drm_property_blob *old_blob = *blob; > > @@ -432,9 +434,8 @@ drm_atomic_replace_property_blob(struct drm_property_blob **blob, > drm_property_blob_get(new_blob); > *blob = new_blob; > *replaced = true; > - > - return; > } > +EXPORT_SYMBOL(drm_atomic_replace_property_blob); > > static int > drm_atomic_replace_property_blob_from_id(struct drm_device *dev, > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h > index dcc8e0c..8b32ea5 100644 > --- a/include/drm/drm_atomic.h > +++ b/include/drm/drm_atomic.h > @@ -321,6 +321,10 @@ int drm_atomic_connector_set_property(struct drm_connector *connector, > struct drm_connector_state *state, struct drm_property *property, > uint64_t val); > > +void drm_atomic_replace_property_blob(struct drm_property_blob **blob, > + struct drm_property_blob *new_blob, > + bool *replaced); > + > void * __must_check > drm_atomic_get_private_obj_state(struct drm_atomic_state *state, > void *obj, > -- > 2.1.4 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch