Re: [PATCH 1/2] media: zoran: add spaces around '<<'

From: Hans Verkuil <hverkuil@xs4all.nl>
To: Mitali Borkar <mitaliborkar810@gmail.com>
Cc: clabbe@baylibre.com, mchehab@kernel.org,
	gregkh@linuxfoundation.org, linux-media@vger.kernel.org,
	linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org,
	outreachy-kernel@googlegroups.com, mitali_s@me.iitr.ac.in
Subject: Re: [PATCH 1/2] media: zoran: add spaces around '<<'
Date: Fri, 9 Apr 2021 17:48:15 +0200	[thread overview]
Message-ID: <5b187aca-8793-bfcb-e52a-c1e03c94b5c9@xs4all.nl> (raw)
In-Reply-To: <YHBukmdxSiRc+K6I@kali>

On 09/04/2021 17:11, Mitali Borkar wrote:
> On Fri, Apr 09, 2021 at 09:23:22AM +0200, Hans Verkuil wrote:
>> Hi Mitali,
>>
>> On 08/04/2021 22:38, Mitali Borkar wrote:
>>> Added spaces around '<<' operator to improve readability and meet linux
>>> kernel coding style.
>>> Reported by checkpatch
>>>
>>> Signed-off-by: Mitali Borkar <mitaliborkar810@gmail.com>
>>> ---
>>>  drivers/staging/media/zoran/zr36057.h | 14 +++++++-------
>>>  1 file changed, 7 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/staging/media/zoran/zr36057.h b/drivers/staging/media/zoran/zr36057.h
>>> index 71b651add35a..a2a75fd9f535 100644
>>> --- a/drivers/staging/media/zoran/zr36057.h
>>> +++ b/drivers/staging/media/zoran/zr36057.h
>>> @@ -30,13 +30,13 @@
>>>  #define ZR36057_VFESPFR_HOR_DCM          14
>>>  #define ZR36057_VFESPFR_VER_DCM          8
>>>  #define ZR36057_VFESPFR_DISP_MODE        6
>>> -#define ZR36057_VFESPFR_YUV422          (0<<3)
>>> -#define ZR36057_VFESPFR_RGB888          (1<<3)
>>> -#define ZR36057_VFESPFR_RGB565          (2<<3)
>>> -#define ZR36057_VFESPFR_RGB555          (3<<3)
>>> -#define ZR36057_VFESPFR_ERR_DIF          (1<<2)
>>> -#define ZR36057_VFESPFR_PACK24          (1<<1)
>>> -#define ZR36057_VFESPFR_LITTLE_ENDIAN    (1<<0)
>>> +#define ZR36057_VFESPFR_YUV422          (0 << 3)
>>> +#define ZR36057_VFESPFR_RGB888          (1 << 3)
>>> +#define ZR36057_VFESPFR_RGB565          (2 << 3)
>>> +#define ZR36057_VFESPFR_RGB555          (3 << 3)
>>> +#define ZR36057_VFESPFR_ERR_DIF          (1 << 2)
>>> +#define ZR36057_VFESPFR_PACK24          (1 << 1)
>>> +#define ZR36057_VFESPFR_LITTLE_ENDIAN    (1 << 0)
>>>  
>>>  #define ZR36057_VDTR            0x00c	/* Video Display "Top" Register */
>>>  
>>>
>>
>> I looked at that header and it is very messy.
>>
>> Can you make two new patches? The first aligns every nicely, e.g. this:
>>
>> #define ZR36057_VFEHCR          0x000   /* Video Front End, Horizontal Configuration Register */
>> #define ZR36057_VFEHCR_HS_POL             BIT(30)
>> #define ZR36057_VFEHCR_H_START           10
>> #define ZR36057_VFEHCR_H_END            0
>> #define ZR36057_VFEHCR_HMASK            0x3ff
>>
>> should become:
>>
>> /* Video Front End, Horizontal Configuration Register */
>> #define ZR36057_VFEHCR			0x000
>> #define ZR36057_VFEHCR_HS_POL		BIT(30)
>> #define ZR36057_VFEHCR_H_START		10
>> #define ZR36057_VFEHCR_H_END		0
>> #define ZR36057_VFEHCR_HMASK		0x3ff
>>
>> Same for all the other register blocks. Use tabs to do the alignment
>> instead of spaces, as is currently the case.
>>
> Okay Sir, will do this.
> 
>> The second patch can replace the (0<<3) etc. to BIT(0).
>>
> Sir may I know how to write (2<<3) in BIT() form? Like I am still
> learning and not sure how to convert this. Should it be BIT(2)? But this
> is only possible for (1<<nr), PLease help me out. 

The << notation is kept for that. So this would be kept as-is:

#define ZR36057_VFESPFR_YUV422          (0 << 3)
#define ZR36057_VFESPFR_RGB888          (1 << 3)
#define ZR36057_VFESPFR_RGB565          (2 << 3)
#define ZR36057_VFESPFR_RGB555          (3 << 3)

BIT() is only used for single bits.

Regards,

	Hans

> 
> Thanks.
> 
>> That would be a nice cleanup of this rather messy header.
>>
>> Thanks!
>>
>> 	Hans