Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v1] drm/kms/crtc: Improving the func drm_mode_setcrtc
To:     Satendra Singh Thakur <satendra.t@samsung.com>,
        Gustavo Padovan <gustavo@padovan.org>,
        Sean Paul <seanpaul@chromium.org>,
        David Airlie <airlied@linux.ie>,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
Cc:     vineet.j@samsung.com, hemanshu.s@samsung.com, sst2005@gmail.com
References: <66ead941-3c64-45e4-7835-949360e2b01f@linux.intel.com>
 <CGME20180803114404epcas5p1c5aaec7ca27e520b5cba52b0d7f09124@epcas5p1.samsung.com>
 <20180803114404epcas5p1c5aaec7ca27e520b5cba52b0d7f09124~HXSyqLTJ81698916989epcas5p1G@epcas5p1.samsung.com>
From:   Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Message-ID: <419670d5-bba9-d5e3-9ad6-f7b3324099c5@linux.intel.com>
Date:   Fri, 3 Aug 2018 14:24:25 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.9.1
MIME-Version: 1.0
In-Reply-To: <20180803114404epcas5p1c5aaec7ca27e520b5cba52b0d7f09124~HXSyqLTJ81698916989epcas5p1G@epcas5p1.samsung.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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@samsung.com>
>
> ---
>  
>  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