Re: [PATCH v1] drm/kms/crtc: Improving the func drm_mode_setcrtc

[Maarten Lankhorst]

Op 03-08-18 om 13:43 schreef Satendra Singh Thakur:
> Following changes are done to this func:
> 1. The declaration of plane and it's assignment plane = crtc->primary
> are only used when mode_valid is set. Therefore, moved it inside
> the if(mode_valid) statement.
>
> 2. The declaration of connector and set_connectors_ptr and out_id
> are moved inside the for loop, as their scope is limited within
> that block.
>
> 3. Currently, there are 3 checks on count_connectors
> and 4 checks on mode related params (mode_valid, mode, fb).
> if (crtc_req->mode_valid) {
> if (crtc_req->count_connectors == 0 && mode) {
> if (crtc_req->count_connectors > 0 && (!mode || !fb)) {
> if (crtc_req->count_connectors > 0) {
>
> In the modified code, there are just 1 check on mode_valid and
> 2 checks  count_connectors.
> Checks on mode and fb are not needed as these variables will
> be non-NULL by the end of if(mode_valid) statement if mode_valid is set.
> If mode_valid is clear, mode and fb will be NULL.
> Therefore, we just check mode_valid and NOT mode or fb.
>
> 4. Moved kfree inside if statement
>
> Signed-off-by: Satendra Singh Thakur <satendra.t@xxxxxxxxxxx>
>
> ---
>  
>  v1: Hi Mr Maarten, Thanks for the comments.
>      I have fixed some of them and done more modifications to the patch.
>      Please review.
Could you read the suggestions on https://patchwork.freedesktop.org/patch/241508/ ?
I'm not saying this code is incorrect, I think that if you want to improve drm then you should tackle something bigger than just function readability. :)

Cheers,
~Maarten