Re: [PATCH v4 2/2] staging: media: zoran: add BIT() macro and align code

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Apr 14, 2021 at 12:38:41PM +0200, zzam@xxxxxxxxxx wrote:
> Am 13.04.21 um 21:50 schrieb Mitali Borkar:
> > Added #include <linux/bitops.h> and replaced bit shifts by BIT() macro.
> > This BIT() macro from linux/bitops.h is used to define ZR36057_VFESPFR_* bitmasks.
> > Use of macro is better and neater. It maintains consistency.
> > Removed comments from the same line and added them to new line above the
> > blocks, aligned everything properly by using tabs to make code neater
> > and improve readability.
> > Reported by checkpatch.
> > 
> > Signed-off-by: Mitali Borkar <mitaliborkar810@xxxxxxxxx>
> > ---
> > 
> > Changes from v3:- No changes.
> > Changes from v2:- Aligned the code using tabs.
> > Changes from v1:- Aligned the code using tabs and readjusted the
> > comments line.
> > 
> >   drivers/staging/media/zoran/zr36057.h | 312 ++++++++++++++------------
> >   1 file changed, 172 insertions(+), 140 deletions(-)
> > 
> 
> General comment:
> The patch is hard to read without applying it and viewing it with ignore
> whitespace.
> 
> > diff --git a/drivers/staging/media/zoran/zr36057.h b/drivers/staging/media/zoran/zr36057.h
> > index a2a75fd9f535..f51e9ea2c3d3 100644
> > --- a/drivers/staging/media/zoran/zr36057.h
> > +++ b/drivers/staging/media/zoran/zr36057.h
> > @@ -7,148 +7,180 @@
> >   #ifndef _ZR36057_H_
> >   #define _ZR36057_H_
> > +#include <linux/bitops.h>
> >   /* Zoran ZR36057 registers */
> > -#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
> > -
> > -#define ZR36057_VFEVCR          0x004	/* Video Front End, Vertical Configuration Register */
> > -#define ZR36057_VFEVCR_VS_POL             BIT(30)
> > -#define ZR36057_VFEVCR_V_START           10
> > -#define ZR36057_VFEVCR_V_END		0
> > -#define ZR36057_VFEVCR_VMASK		0x3ff
> > -
> > -#define ZR36057_VFESPFR         0x008	/* Video Front End, Scaler and Pixel Format Register */
> > -#define ZR36057_VFESPFR_EXT_FL            BIT(26)
> > -#define ZR36057_VFESPFR_TOP_FIELD         BIT(25)
> > -#define ZR36057_VFESPFR_VCLK_POL          BIT(24)
> > -#define ZR36057_VFESPFR_H_FILTER         21
> > -#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_VDTR            0x00c	/* Video Display "Top" Register */
> > -
> > -#define ZR36057_VDBR            0x010	/* Video Display "Bottom" Register */
> > -
> > -#define ZR36057_VSSFGR          0x014	/* Video Stride, Status, and Frame Grab Register */
> > -#define ZR36057_VSSFGR_DISP_STRIDE       16
> > -#define ZR36057_VSSFGR_VID_OVF            BIT(8)
> > -#define ZR36057_VSSFGR_SNAP_SHOT          BIT(1)
> > -#define ZR36057_VSSFGR_FRAME_GRAB         BIT(0)
> > -
> > -#define ZR36057_VDCR            0x018	/* Video Display Configuration Register */
> > -#define ZR36057_VDCR_VID_EN               BIT(31)
> > -#define ZR36057_VDCR_MIN_PIX             24
> > -#define ZR36057_VDCR_TRITON              BIT(24)
> > -#define ZR36057_VDCR_VID_WIN_HT           12
> > -#define ZR36057_VDCR_VID_WIN_WID          0
> > -
> > -#define ZR36057_MMTR            0x01c	/* Masking Map "Top" Register */
> > -
> > -#define ZR36057_MMBR            0x020	/* Masking Map "Bottom" Register */
> > -
> > -#define ZR36057_OCR             0x024	/* Overlay Control Register */
> > -#define ZR36057_OCR_OVL_ENABLE            BIT(15)
> > -#define ZR36057_OCR_MASK_STRIDE          0
> > -
> > -#define ZR36057_SPGPPCR         0x028	/* System, PCI, and General Purpose Pins Control Register */
> > -#define ZR36057_SPGPPCR_SOFT_RESET	 BIT(24)
> > -
> > -#define ZR36057_GPPGCR1         0x02c	/* General Purpose Pins and GuestBus Control Register (1) */
> > -
> > -#define ZR36057_MCSAR           0x030	/* MPEG Code Source Address Register */
> > -
> > -#define ZR36057_MCTCR           0x034	/* MPEG Code Transfer Control Register */
> > -#define ZR36057_MCTCR_COD_TIME            BIT(30)
> > -#define ZR36057_MCTCR_C_EMPTY             BIT(29)
> > -#define ZR36057_MCTCR_C_FLUSH             BIT(28)
> > -#define ZR36057_MCTCR_COD_GUEST_ID	20
> > -#define ZR36057_MCTCR_COD_GUEST_REG	16
> > -
> > -#define ZR36057_MCMPR           0x038	/* MPEG Code Memory Pointer Register */
> > -
> > -#define ZR36057_ISR             0x03c	/* Interrupt Status Register */
> > -#define ZR36057_ISR_GIRQ1                BIT(30)
> > -#define ZR36057_ISR_GIRQ0                BIT(29)
> > -#define ZR36057_ISR_COD_REP_IRQ            BIT(28)
> > -#define ZR36057_ISR_JPEG_REP_IRQ           BIT(27)
> > -
> > -#define ZR36057_ICR             0x040	/* Interrupt Control Register */
> > -#define ZR36057_ICR_GIRQ1                BIT(30)
> > -#define ZR36057_ICR_GIRQ0                BIT(29)
> > -#define ZR36057_ICR_COD_REP_IRQ            BIT(28)
> > -#define ZR36057_ICR_JPEG_REP_IRQ           BIT(27)
> > -#define ZR36057_ICR_INT_PIN_EN             BIT(24)
> > -
> > -#define ZR36057_I2CBR           0x044	/* I2C Bus Register */
> > -#define ZR36057_I2CBR_SDA		 BIT(1)
> > -#define ZR36057_I2CBR_SCL		 BIT(0)
> > -
> > -#define ZR36057_JMC             0x100	/* JPEG Mode and Control */
> > -#define ZR36057_JMC_JPG                  BIT(31)
> > -#define ZR36057_JMC_JPG_EXP_MODE          (0 << 29)
> > -#define ZR36057_JMC_JPG_CMP_MODE           BIT(29)
> > -#define ZR36057_JMC_MJPG_EXP_MODE         (2 << 29)
> > -#define ZR36057_JMC_MJPG_CMP_MODE         (3 << 29)
> > -#define ZR36057_JMC_RTBUSY_FB            BIT(6)
> > -#define ZR36057_JMC_GO_EN                BIT(5)
> > -#define ZR36057_JMC_SYNC_MSTR             BIT(4)
> > -#define ZR36057_JMC_FLD_PER_BUFF         BIT(3)
> > -#define ZR36057_JMC_VFIFO_FB             BIT(2)
> > -#define ZR36057_JMC_CFIFO_FB             BIT(1)
> > -#define ZR36057_JMC_STLL_LIT_ENDIAN       BIT(0)
> > -
> > -#define ZR36057_JPC             0x104	/* JPEG Process Control */
> > -#define ZR36057_JPC_P_RESET              BIT(7)
> > -#define ZR36057_JPC_COD_TRNS_EN            BIT(5)
> > -#define ZR36057_JPC_ACTIVE               BIT(0)
> > -
> > -#define ZR36057_VSP             0x108	/* Vertical Sync Parameters */
> > -#define ZR36057_VSP_VSYNC_SIZE           16
> > -#define ZR36057_VSP_FRM_TOT              0
> > -
> > -#define ZR36057_HSP             0x10c	/* Horizontal Sync Parameters */
> > -#define ZR36057_HSP_HSYNC_START          16
> > -#define ZR36057_HSP_LINE_TOT             0
> > -
> > -#define ZR36057_FHAP            0x110	/* Field Horizontal Active Portion */
> > -#define ZR36057_FHAP_NAX                16
> > -#define ZR36057_FHAP_PAX                0
> > -
> > -#define ZR36057_FVAP            0x114	/* Field Vertical Active Portion */
> > -#define ZR36057_FVAP_NAY                16
> > -#define ZR36057_FVAP_PAY                0
> > -
> > -#define ZR36057_FPP             0x118	/* Field Process Parameters */
> > -#define ZR36057_FPP_ODD_EVEN             BIT(0)
> > -
> > -#define ZR36057_JCBA            0x11c	/* JPEG Code Base Address */
> > -
> > -#define ZR36057_JCFT            0x120	/* JPEG Code FIFO Threshold */
> > -
> > -#define ZR36057_JCGI            0x124	/* JPEG Codec Guest ID */
> > -#define ZR36057_JCGI_JPE_GUEST_ID         4
> > -#define ZR36057_JCGI_JPE_GUEST_REG        0
> > -
> > -#define ZR36057_GCR2            0x12c	/* GuestBus Control Register (2) */
> > -
> > -#define ZR36057_POR             0x200	/* Post Office Register */
> > -#define ZR36057_POR_PO_PEN                BIT(25)
> > -#define ZR36057_POR_PO_TIME               BIT(24)
> > -#define ZR36057_POR_PO_DIR                BIT(23)
> > -
> > -#define ZR36057_STR             0x300	/* "Still" Transfer Register */
> > +/* 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
> > +
> > +/* Video Front End, Vertical Configuration Register */
> > +#define ZR36057_VFEVCR				0x004
> > +#define ZR36057_VFEVCR_VS_POL			BIT(30)
> > +#define ZR36057_VFEVCR_V_START			10
> > +#define ZR36057_VFEVCR_V_END			0
> > +#define ZR36057_VFEVCR_VMASK			0x3ff
> > +
> > +/* Video Front End, Scaler and Pixel Format Register */
> > +#define ZR36057_VFESPFR			0x008
> > +#define ZR36057_VFESPFR_EXT_FL			BIT(26)
> > +#define ZR36057_VFESPFR_TOP_FIELD		BIT(25)
> > +#define ZR36057_VFESPFR_VCLK_POL		BIT(24)
> > +#define ZR36057_VFESPFR_H_FILTER		21
> > +#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			BIT(3)
> > +#define ZR36057_VFESPFR_RGB565			(2 << 3)
> > +#define ZR36057_VFESPFR_RGB555			(3 << 3)
> The single BIT occurrence in this list is confusing at best. Please keep
> ZR36057_VFESPFR_RGB888 to be (1 << 3). It is not a single bit, but defining
> a 2-bit field in the middle with 4 different values. It should be
> consistent.
>
Okay Sir.

> > +#define ZR36057_VFESPFR_ERR_DIF		BIT(2)
> > +#define ZR36057_VFESPFR_PACK24			BIT(1)
> > +#define ZR36057_VFESPFR_LITTLE_ENDIAN		BIT(0)
> > +
> > +/* Video Display "Top" Register */
> > +#define ZR36057_VDTR				0x00c
> > +
> > +/* Video Display "Bottom" Register */
> > +#define ZR36057_VDBR				0x010
> > +
> > +/* Video Stride, Status, and Frame Grab Register */
> > +#define ZR36057_VSSFGR				0x014
> > +#define ZR36057_VSSFGR_DISP_STRIDE		16
> > +#define ZR36057_VSSFGR_VID_OVF			BIT(8)
> > +#define ZR36057_VSSFGR_SNAP_SHOT		BIT(1)
> > +#define ZR36057_VSSFGR_FRAME_GRAB		BIT(0)
> > +
> > +/* Video Display Configuration Register */
> > +#define ZR36057_VDCR				0x018
> > +#define ZR36057_VDCR_VID_EN			BIT(31)
> > +#define ZR36057_VDCR_MIN_PIX			24
> > +#define ZR36057_VDCR_TRITON			BIT(24)
> > +#define ZR36057_VDCR_VID_WIN_HT		12
> > +#define ZR36057_VDCR_VID_WIN_WID		0
> > +
> > +/* Masking Map "Top" Register */
> > +#define ZR36057_MMTR				0x01c
> > +
> > +/* Masking Map "Bottom" Register */
> > +#define ZR36057_MMBR				0x020
> > +
> > +/* Overlay Control Register */
> > +#define ZR36057_OCR				0x024
> > +#define ZR36057_OCR_OVL_ENABLE			BIT(15)
> > +#define ZR36057_OCR_MASK_STRIDE		0
> > +
> > +/* System, PCI, and General Purpose Pins Control Register */
> > +#define ZR36057_SPGPPCR			0x028
> > +#define ZR36057_SPGPPCR_SOFT_RESET		BIT(24)
> > +
> > +/* General Purpose Pins and GuestBus Control Register (1) */
> > +#define ZR36057_GPPGCR1			0x02c
> > +
> > +/* MPEG Code Source Address Register */
> > +#define ZR36057_MCSAR				0x030
> > +
> > +/* MPEG Code Transfer Control Register */
> > +#define ZR36057_MCTCR				0x034
> > +#define ZR36057_MCTCR_COD_TIME			BIT(30)
> > +#define ZR36057_MCTCR_C_EMPTY			BIT(29)
> > +#define ZR36057_MCTCR_C_FLUSH			BIT(28)
> > +#define ZR36057_MCTCR_COD_GUEST_ID		20
> > +#define ZR36057_MCTCR_COD_GUEST_REG		16
> > +
> > +/* MPEG Code Memory Pointer Register */
> > +#define ZR36057_MCMPR				0x038
> > +
> > +/* Interrupt Status Register */
> > +#define ZR36057_ISR				0x03c
> > +#define ZR36057_ISR_GIRQ1			BIT(30)
> > +#define ZR36057_ISR_GIRQ0			BIT(29)
> > +#define ZR36057_ISR_COD_REP_IRQ		BIT(28)
> > +#define ZR36057_ISR_JPEG_REP_IRQ		BIT(27)
> > +
> > +/* Interrupt Control Register */
> > +#define ZR36057_ICR				0x040
> > +#define ZR36057_ICR_GIRQ1			BIT(30)
> > +#define ZR36057_ICR_GIRQ0			BIT(29)
> > +#define ZR36057_ICR_COD_REP_IRQ		BIT(28)
> > +#define ZR36057_ICR_JPEG_REP_IRQ		BIT(27)
> > +#define ZR36057_ICR_INT_PIN_EN			BIT(24)
> > +
> > +/* I2C Bus Register */
> > +#define ZR36057_I2CBR				0x044
> > +#define ZR36057_I2CBR_SDA			BIT(1)
> > +#define ZR36057_I2CBR_SCL			BIT(0)
> > +
> > +/* JPEG Mode and Control */
> > +#define ZR36057_JMC				0x100
> > +#define ZR36057_JMC_JPG			BIT(31)
> 
> > +#define ZR36057_JMC_JPG_EXP_MODE		(0 << 29)
> > +#define ZR36057_JMC_JPG_CMP_MODE		BIT(29)
> > +#define ZR36057_JMC_MJPG_EXP_MODE		(2 << 29)
> > +#define ZR36057_JMC_MJPG_CMP_MODE		(3 << 29)
> Same as above. Please change back ZR36057_JMC_JPG_CMP_MODE to be (1 << 29).
> Then this 2 bit field is consistent.
> 
Sir, I guess this BIT(29) was already present. I didn't changed this. 
I will change this as you said.

> > +#define ZR36057_JMC_RTBUSY_FB			BIT(6)
> > +#define ZR36057_JMC_GO_EN			BIT(5)
> > +#define ZR36057_JMC_SYNC_MSTR			BIT(4)
> > +#define ZR36057_JMC_FLD_PER_BUFF		BIT(3)
> > +#define ZR36057_JMC_VFIFO_FB			BIT(2)
> > +#define ZR36057_JMC_CFIFO_FB			BIT(1)
> > +#define ZR36057_JMC_STLL_LIT_ENDIAN		BIT(0)
> > +
> > +/* JPEG Process Control */
> > +#define ZR36057_JPC				0x104
> > +#define ZR36057_JPC_P_RESET			BIT(7)
> > +#define ZR36057_JPC_COD_TRNS_EN		BIT(5)
> > +#define ZR36057_JPC_ACTIVE			BIT(0)
> > +
> > +/* Vertical Sync Parameters */
> > +#define ZR36057_VSP				0x108
> > +#define ZR36057_VSP_VSYNC_SIZE			16
> > +#define ZR36057_VSP_FRM_TOT			0
> > +
> > +/* Horizontal Sync Parameters */
> > +#define ZR36057_HSP				0x10c
> > +#define ZR36057_HSP_HSYNC_START		16
> > +#define ZR36057_HSP_LINE_TOT			0
> > +
> > +/* Field Horizontal Active Portion */
> > +#define ZR36057_FHAP				0x110
> > +#define ZR36057_FHAP_NAX			16
> > +#define ZR36057_FHAP_PAX			0
> > +
> > +/* Field Vertical Active Portion */
> > +#define ZR36057_FVAP				0x114
> > +#define ZR36057_FVAP_NAY			16
> > +#define ZR36057_FVAP_PAY			0
> > +
> > +/* Field Process Parameters */
> > +#define ZR36057_FPP				0x118
> > +#define ZR36057_FPP_ODD_EVEN			BIT(0)
> > +
> > +/* JPEG Code Base Address */
> > +#define ZR36057_JCBA				0x11c
> > +
> > +/* JPEG Code FIFO Threshold */
> > +#define ZR36057_JCFT				0x120
> > +
> > +/* JPEG Codec Guest ID */
> > +#define ZR36057_JCGI				0x124
> > +#define ZR36057_JCGI_JPE_GUEST_ID		4
> > +#define ZR36057_JCGI_JPE_GUEST_REG		0
> > +
> > +/* GuestBus Control Register (2) */
> > +#define ZR36057_GCR2				0x12c
> > +
> > +/* Post Office Register */
> > +#define ZR36057_POR				0x200
> > +#define ZR36057_POR_PO_PEN			BIT(25)
> > +#define ZR36057_POR_PO_TIME			BIT(24)
> > +#define ZR36057_POR_PO_DIR			BIT(23)
> > +
> > +/* "Still" Transfer Register */
> > +#define ZR36057_STR				0x300
> >   #endif
> > 
> 
> Regards
> Matthias



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux