Subject: Re: [PATCH v4 01/14] drm/atomic: export
 drm_atomic_replace_property_blob
To: linux-kernel@vger.kernel.org,
        Boris Brezillon <boris.brezillon@free-electrons.com>,
        dri-devel@lists.freedesktop.org,
        Daniel Vetter <daniel.vetter@intel.com>
References: <1499343648-29695-1-git-send-email-peda@axentia.se>
 <1499343648-29695-2-git-send-email-peda@axentia.se>
 <20170711080136.7ywol2i272oq2mkk@phenom.ffwll.local>
From: Peter Rosin <peda@axentia.se>
Organization: Axentia Technologies AB
Message-ID: <464dad2f-84fd-69bc-9c4b-e51db33e4349@axentia.se>
Date: Tue, 11 Jul 2017 14:06:10 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
 Thunderbird/52.2.1
MIME-Version: 1.0
In-Reply-To: <20170711080136.7ywol2i272oq2mkk@phenom.ffwll.local>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Jul 2017 12:06:14.2718 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM5PR0202MB2546
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 3719
Lines: 98

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
>