[Intel-gfx] [PATCH 3/3] drm/i915: Use transcoder as index to MIPI regs
Sharma, Shashank
shashank.sharma at intel.com
Mon Jun 2 17:08:03 CEST 2014
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.
>
More information about the Intel-gfx
mailing list