Re: [PATCH 3/3] drm/crtc-helper: Add a drm_crtc_helper_atomic_check() helper

[Javier Martinez Canillas]

On 10/11/22 15:21, Thomas Zimmermann wrote:
> Hi
> 
> Am 10.10.22 um 19:02 schrieb Javier Martinez Canillas:
>> Provides a default CRTC state check handler for CRTCs that only have one
>> primary plane attached.
>>
>> There are some drivers that duplicate this logic in their helpers, such as
>> simpledrm and ssd130x. Factor out this common code into a CRTC helper and
>> make drivers use it.
>>
>> Signed-off-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>
> 
> Reviewed-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> 
> There really isn't much here for now. I suspect that there are more 
> drivers that could use this helper. If you merge this before ofdrm, I'll 
> rebase ofdrm on top.
>

Sure. I probably won't post a v2 until tomorrow and I believe ofdrm is
ready to be merged, so I'll just rebase this series on top of that once
just push it.

> Please also see my comment below.
> 
> 

[...]

>> +/**
>> + * drm_crtc_helper_atomic_check() - Helper to check CRTC atomic-state
>> + * @crtc: CRTC to check
>> + * @state: atomic state object
>> + *
>> + * Provides a default CRTC-state check handler for CRTCs that only have
>> + * one primary plane attached to it.
>> + *
>> + * This is often the case for the CRTC of simple framebuffers.
> 
> I'd add a reference to drm_plane_helper_atomic_check() to this 
> paragraph. Like
> 
>    See drm_plane_helper_atomic_check() for the respective plane helpers.
> 
> And also reference back from the plane-check helper to the CRTC-check 
> helper.
>

Good idea, I'll do that. Thanks for your review.
 
> Best regards
> Thomas
> 

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat