| Message ID | 1499343648-29695-2-git-send-email-peda@axentia.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <peda@axentia.se> > --- > 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
On 2017-07-11 10:01, Daniel Vetter wrote: > 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 <peda@axentia.se> >> --- >> 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. Right, good judgement. Regarding incremental reviewing, I had it coming because I am guilty too... :-) Anyway, no problem! > 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. Right. And again, good judgement. > That's a bit more work, but imo functions which are exported need to pass > a higher barrier wrt api polish. Will fix these issues in v5. > Thanks, Daniel Cheers, Peter >> { >> 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 >
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) { 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,
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 <peda@axentia.se> --- drivers/gpu/drm/drm_atomic.c | 17 +++++++++-------- include/drm/drm_atomic.h | 4 ++++ 2 files changed, 13 insertions(+), 8 deletions(-)