From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Sharma, Shashank" Subject: Re: [PATCH 3/3] drm/i915: Use transcoder as index to MIPI regs Date: Mon, 02 Jun 2014 20:38:03 +0530 Message-ID: <538C9353.4090306@intel.com> References: <20140602112326.GB17233@strange.amr.corp.intel.com> <1401712668-12514-1-git-send-email-shashank.sharma@intel.com> <1401712668-12514-2-git-send-email-shashank.sharma@intel.com> <20140602125153.GB20418@strange.amr.corp.intel.com> <20140602132653.GE20418@strange.amr.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by gabe.freedesktop.org (Postfix) with ESMTP id D524F6E5A0 for ; Mon, 2 Jun 2014 08:08:57 -0700 (PDT) In-Reply-To: <20140602132653.GE20418@strange.amr.corp.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Damien Lespiau Cc: "Vetter, Daniel" , "intel-gfx@lists.freedesktop.org" List-Id: intel-gfx@lists.freedesktop.org Hi Damien, Thanks for providing the pointers. In my first patch I tried to aligned all the registers definitions, and I got my first review comment for not required formatting changes. Since then, I just replaced _PIPE with _TRANSCODER, so there are no changes at all. So I have just maintained the alignment as it is from the previous MIPI reg definitions and there is no extra/unnecessary tab or space inserted. This line: #define MIPI_READ_DATA_VALID(tc) _TRANSCODER(tc, \ > _MIPIA_READ_DATA_VALID, _MIPIB_READ_DATA_VALID) has a different alignment, just to keep the second line < 80 char. If you insert one more tab in front of _MIPIA_READ_DATA_VALID, its going beyond 80 char, so I had to pull it up. Similarly, #define _MIPIA_CLK_LANE_SWITCH_TIME_CNT (dev_priv->mipi_mmio_base \ > + 0xb088) > There were only two options, either a checkpatch warning, or push to next line. > #define MIPI_READ_DATA_RETURN(tc, n) \ > (_TRANSCODER(tc, _MIPIA_READ_DATA_RETURN0, _MIPIB_READ_DATA_RETURN0) \ > + 4 * (n)) /* n: 0...7 */ > This line was maintained as original alignment, just replacing PIPE with TRANSCODER, no tabs/space inserted. So, you have to agree that, I might have symptoms of OCD, but definitely not uncontrollable :). Going forward I will keep this mind that we can play around checkpatch rules it it gives good readability. Thanks for your time and patience for the review, and thanks a lot for R-B. Regards Shashank On 6/2/2014 6:56 PM, Damien Lespiau wrote: > On Mon, Jun 02, 2014 at 01:55:13PM +0100, Sharma, Shashank wrote: >> Hi Damien, >> >> Can you please point out these, as this patch is re-based on latest >> 2/3, I was expecting this to be without any inconsistency. >> I personally checked for any <80 char formatting, which is not >> required. But if I missed any, I can again fix this, please let me >> know. > > At this point, there's no "rule". As Daniel said earlier the 80 chars > limit is a soft one, esp. in headers declaring list of registers. > > For the inconsistencies, it's just a personal preference, I would try to > make all defines look alike, right now you have: > > #define MIPI_DPI_CONTROL(tc) _TRANSCODER(tc, _MIPIA_DPI_CONTROL, \ > _MIPIB_DPI_CONTROL) > > > #define MIPI_GEN_FIFO_STAT(tc) _TRANSCODER(tc, _MIPIA_GEN_FIFO_STAT, \ > _MIPIB_GEN_FIFO_STAT) > > > #define MIPI_READ_DATA_VALID(tc) _TRANSCODER(tc, \ > _MIPIA_READ_DATA_VALID, _MIPIB_READ_DATA_VALID) > > > All different alignments. Not something I would ever do, but there's no rule > against it per se, hence the r-b. > > You have a couple more of debatable splits: > > #define _MIPIA_CLK_LANE_SWITCH_TIME_CNT (dev_priv->mipi_mmio_base \ > + 0xb088) > > #define MIPI_READ_DATA_RETURN(tc, n) \ > (_TRANSCODER(tc, _MIPIA_READ_DATA_RETURN0, _MIPIB_READ_DATA_RETURN0) \ > + 4 * (n)) /* n: 0...7 */ > > Esp. for the first one, these are cases where the "< 80 chars" split goes > against readibility. > > Someone may ask you to fix those "bad" splits, not me this time though. >