Re: [PATCH 02/10] drm/i915: Added port as parameter to the functions which does read/write of DSI Controller

From: Daniel Vetter <daniel@ffwll.ch>
To: "Singh, Gaurav K" <gaurav.k.singh@intel.com>
Cc: Shobhit Kumar <shobhit.kumar@intel.com>,
	intel-gfx <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 02/10] drm/i915: Added port as parameter to the	functions which does read/write of DSI Controller
Date: Fri, 5 Dec 2014 15:38:46 +0100	[thread overview]
Message-ID: <20141205143846.GP20350@phenom.ffwll.local> (raw)
In-Reply-To: <5481AA23.80308@intel.com>

On Fri, Dec 05, 2014 at 06:20:43PM +0530, Singh, Gaurav K wrote:
> 
> On 12/4/2014 4:52 PM, Daniel Vetter wrote:
> >On Thu, Dec 04, 2014 at 11:14:01AM +0200, Jani Nikula wrote:
> >>On Thu, 04 Dec 2014, Gaurav K Singh <gaurav.k.singh@intel.com> wrote:
> >>>This patch is in preparation of DSI dual link panels. For dual link
> >>>panels, few packets needs to be sent to Port A or Port C or both. Based
> >>>on the portno from MIPI Sequence Block#53, these sequences needs to be
> >>>sent accordingly.
> >>>
> >>>v2: Addressed review comments by Jani
> >>>     - port variables named properly
> >>>
> >>>Signed-off-by: Gaurav K Singh <gaurav.k.singh@intel.com>
> >>Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> >Merged the first two patches, thanks.
> >-Daniel
> Hi Daniel,
> 
> Thanks. I have addressed Jani's review comments too for the rest of the
> patches.

Well looking through the resend patches your changes look a bit minimal.
Which might be the right thing to do, but review is also about
communication and sharing expert knowledge. So please follow up to the
questions from Jani where you didn't adjust the code. And if there's any
follow-up patches from those discussions please submit them.

Another part: Your editor doesn't seem to align continuation lines quite
how we usually do that. checkpatch --strict will catch that. Two big
rules:
- function paramater lists should be aligned to the opening ( on the next
  line.
- Continuations of boolean logic checks (e.g. in if checks) same, with the
  special rule that && or || should be on the previous line as the last
  thing.
- continuation of any other long lines should be indented 1 or two spaces
  (just to make sure it sticks out without looking jarring).

Especially the first two help readability a lot of you're used to them, so
please do a follow-up patch to rectify this in intel_dsi*.c.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx