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 <p...@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
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
