Re: [PATCH 3/3] drm/i915: Use transcoder as index to MIPI regs

["Sharma, Shashank" <shashank.sharma@intel.com>]

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.
>