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.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
