Re: [RFC 04/12] exynos-fimc-is: Adds common driver header files

On 03/08/2013 03:59 PM, Arun Kumar K wrote:

[...]

+#define HOST_SET_INT_BIT	0x00000001
+#define HOST_CLR_INT_BIT	0x00000001
+#define IS_SET_INT_BIT		0x00000001
+#define IS_CLR_INT_BIT		0x00000001
+
+#define HOST_SET_INTERRUPT(base)	(base->uiINTGR0 |= HOST_SET_INT_BIT)
+#define HOST_CLR_INTERRUPT(base)	(base->uiINTCR0 |= HOST_CLR_INT_BIT)
+#define IS_SET_INTERRUPT(base)		(base->uiINTGR1 |= IS_SET_INT_BIT)
+#define IS_CLR_INTERRUPT(base)		(base->uiINTCR1 |= IS_CLR_INT_BIT)

These 8 appear to be all unused unused definitions. Probably can be removed.

nit: It feels an enum would be more appropriate for these.

Hmm, these macros look pretty ugly. They obfuscate what's really being done.
I would try to rewrite the code so we use pointers to target data structures
instead.

+#ifndef BIT0
+#define  BIT0     0x00000001
+#define  BIT1     0x00000002
+#define  BIT2     0x00000004
+#define  BIT3     0x00000008
+#define  BIT4     0x00000010
+#define  BIT5     0x00000020
+#define  BIT6     0x00000040
+#define  BIT7     0x00000080
+#define  BIT8     0x00000100
+#define  BIT9     0x00000200
+#define  BIT10    0x00000400
+#define  BIT11    0x00000800
+#define  BIT12    0x00001000
+#define  BIT13    0x00002000
+#define  BIT14    0x00004000
+#define  BIT15    0x00008000
+#define  BIT16    0x00010000
+#define  BIT17    0x00020000
+#define  BIT18    0x00040000
+#define  BIT19    0x00080000
+#define  BIT20    0x00100000
+#define  BIT21    0x00200000
+#define  BIT22    0x00400000
+#define  BIT23    0x00800000
+#define  BIT24    0x01000000
+#define  BIT25    0x02000000
+#define  BIT26    0x04000000
+#define  BIT27    0x08000000
+#define  BIT28    0x10000000
+#define  BIT29    0x20000000
+#define  BIT30    0x40000000
+#define  BIT31    0x80000000
+#define  BIT32    0x0000000100000000ULL
+#define  BIT33    0x0000000200000000ULL
+#define  BIT34    0x0000000400000000ULL
+#define  BIT35    0x0000000800000000ULL
+#define  BIT36    0x0000001000000000ULL
+#define  BIT37    0x0000002000000000ULL
+#define  BIT38    0x0000004000000000ULL
+#define  BIT39    0x0000008000000000ULL
+#define  BIT40    0x0000010000000000ULL
+#define  BIT41    0x0000020000000000ULL
+#define  BIT42    0x0000040000000000ULL
+#define  BIT43    0x0000080000000000ULL
+#define  BIT44    0x0000100000000000ULL
+#define  BIT45    0x0000200000000000ULL
+#define  BIT46    0x0000400000000000ULL
+#define  BIT47    0x0000800000000000ULL
+#define  BIT48    0x0001000000000000ULL
+#define  BIT49    0x0002000000000000ULL
+#define  BIT50    0x0004000000000000ULL
+#define  BIT51    0x0008000000000000ULL
+#define  BIT52    0x0010000000000000ULL
+#define  BIT53    0x0020000000000000ULL
+#define  BIT54    0x0040000000000000ULL
+#define  BIT55    0x0080000000000000ULL
+#define  BIT56    0x0100000000000000ULL
+#define  BIT57    0x0200000000000000ULL
+#define  BIT58    0x0400000000000000ULL
+#define  BIT59    0x0800000000000000ULL
+#define  BIT60    0x1000000000000000ULL
+#define  BIT61    0x2000000000000000ULL
+#define  BIT62    0x4000000000000000ULL
+#define  BIT63    0x8000000000000000ULL

Are you sure this is a good idea ? What do you think BIT() macro available
in bitops.h does ? And are all these BIT definitions really used anywhere ?

+#define  INC_BIT(bit) (bit<<1)
+#define  INC_NUM(bit) (bit + 1)
+#endif
+
+#define MAGIC_NUMBER 0x01020304
+
+#define PARAMETER_MAX_SIZE    128  /* in byte */
+#define PARAMETER_MAX_MEMBER  (PARAMETER_MAX_SIZE/4)
+
+enum is_entry {
+	ENTRY_GLOBAL,
+	ENTRY_BUFFER,
+	ENTRY_SENSOR,
+	ENTRY_ISP,
+	ENTRY_DRC,
+	ENTRY_SCALERC,
+	ENTRY_ODC,
+	ENTRY_DIS,
+	ENTRY_TDNR,
+	ENTRY_SCALERP,
+	ENTRY_LHFD, /* 10 */
+	ENTRY_END

Are these used somewhere ?

+};
+
+enum is_param_set_bit {
+	PARAM_GLOBAL_SHOTMODE = 0,
+	PARAM_SENSOR_CONTROL,
+	PARAM_SENSOR_OTF_INPUT,
+	PARAM_SENSOR_OTF_OUTPUT,
+	PARAM_SENSOR_FRAME_RATE,
+	PARAM_SENSOR_DMA_OUTPUT,
+	PARAM_BUFFER_CONTROL,
+	PARAM_BUFFER_OTF_INPUT,
+	PARAM_BUFFER_OTF_OUTPUT,
+	PARAM_ISP_CONTROL,
+	PARAM_ISP_OTF_INPUT = 10,
+	PARAM_ISP_DMA1_INPUT,
+	PARAM_ISP_DMA2_INPUT,
+	PARAM_ISP_AA,
+	PARAM_ISP_FLASH,
+	PARAM_ISP_AWB,
+	PARAM_ISP_IMAGE_EFFECT,
+	PARAM_ISP_ISO,
+	PARAM_ISP_ADJUST,
+	PARAM_ISP_METERING,
+	PARAM_ISP_AFC = 20,
+	PARAM_ISP_OTF_OUTPUT,
+	PARAM_ISP_DMA1_OUTPUT,
+	PARAM_ISP_DMA2_OUTPUT,
+	PARAM_DRC_CONTROL,
+	PARAM_DRC_OTF_INPUT,
+	PARAM_DRC_DMA_INPUT,
+	PARAM_DRC_OTF_OUTPUT,
+	PARAM_SCALERC_CONTROL,
+	PARAM_SCALERC_OTF_INPUT,
+	PARAM_SCALERC_IMAGE_EFFECT = 30,
+	PARAM_SCALERC_INPUT_CROP,
+	PARAM_SCALERC_OUTPUT_CROP,
+	PARAM_SCALERC_OTF_OUTPUT,
+	PARAM_SCALERC_DMA_OUTPUT = 34,
+	PARAM_ODC_CONTROL,
+	PARAM_ODC_OTF_INPUT,
+	PARAM_ODC_OTF_OUTPUT,
+	PARAM_DIS_CONTROL,
+	PARAM_DIS_OTF_INPUT,
+	PARAM_DIS_OTF_OUTPUT = 40,
+	PARAM_TDNR_CONTROL,
+	PARAM_TDNR_OTF_INPUT,
+	PARAM_TDNR_1ST_FRAME,
+	PARAM_TDNR_OTF_OUTPUT,
+	PARAM_TDNR_DMA_OUTPUT,
+	PARAM_SCALERP_CONTROL,
+	PARAM_SCALERP_OTF_INPUT,
+	PARAM_SCALERP_IMAGE_EFFECT,
+	PARAM_SCALERP_INPUT_CROP,
+	PARAM_SCALERP_OUTPUT_CROP = 50,
+	PARAM_SCALERP_ROTATION,
+	PARAM_SCALERP_FLIP,
+	PARAM_SCALERP_OTF_OUTPUT,
+	PARAM_SCALERP_DMA_OUTPUT,
+	PARAM_FD_CONTROL,
+	PARAM_FD_OTF_INPUT,
+	PARAM_FD_DMA_INPUT,
+	PARAM_FD_CONFIG = 58,
+	PARAM_END,
+};
+
+#define ADDRESS_TO_OFFSET(start, end)	((uint32)end - (uint32)start)
+#define OFFSET_TO_NUM(offset)		((offset)>>6)
+#define IS_OFFSET_LOWBIT(offset)	(OFFSET_TO_NUM(offset)>= \
+						32 ? false : true)
+#define OFFSET_TO_BIT(offset) \
+		{(IS_OFFSET_LOWBIT(offset) ? (1<<OFFSET_TO_NUM(offset)) \
+			: (1<<(OFFSET_TO_NUM(offset)-32))}
+#define LOWBIT_OF_NUM(num)		(num>= 32 ? 0 : BIT0<<num)
+#define HIGHBIT_OF_NUM(num)		(num>= 32 ? BIT0<<(num-32) : 0)
+
+/* 0~31 */
+#define PARAM_GLOBAL_SHOTMODE		0
+#define PARAM_SENSOR_CONTROL		INC_NUM(PARAM_GLOBAL_SHOTMODE)
+#define PARAM_SENSOR_OTF_INPUT		INC_NUM(PARAM_SENSOR_CONTROL)
+#define PARAM_SENSOR_OTF_OUTPUT		INC_NUM(PARAM_SENSOR_OTF_INPUT)
+#define PARAM_SENSOR_FRAME_RATE		INC_NUM(PARAM_SENSOR_OTF_OUTPUT)
+#define PARAM_SENSOR_DMA_OUTPUT		INC_NUM(PARAM_SENSOR_FRAME_RATE)
+#define PARAM_BUFFER_CONTROL		INC_NUM(PARAM_SENSOR_DMA_OUTPUT)
+#define PARAM_BUFFER_OTF_INPUT		INC_NUM(PARAM_BUFFER_CONTROL)
+#define PARAM_BUFFER_OTF_OUTPUT		INC_NUM(PARAM_BUFFER_OTF_INPUT)
+#define PARAM_ISP_CONTROL		INC_NUM(PARAM_BUFFER_OTF_OUTPUT)
+#define PARAM_ISP_OTF_INPUT		INC_NUM(PARAM_ISP_CONTROL)
+#define PARAM_ISP_DMA1_INPUT		INC_NUM(PARAM_ISP_OTF_INPUT)
+#define PARAM_ISP_DMA2_INPUT		INC_NUM(PARAM_ISP_DMA1_INPUT)
+#define PARAM_ISP_AA			INC_NUM(PARAM_ISP_DMA2_INPUT)
+#define PARAM_ISP_FLASH			INC_NUM(PARAM_ISP_AA)
+#define PARAM_ISP_AWB			INC_NUM(PARAM_ISP_FLASH)
+#define PARAM_ISP_IMAGE_EFFECT		INC_NUM(PARAM_ISP_AWB)
+#define PARAM_ISP_ISO			INC_NUM(PARAM_ISP_IMAGE_EFFECT)
+#define PARAM_ISP_ADJUST		INC_NUM(PARAM_ISP_ISO)
+#define PARAM_ISP_METERING		INC_NUM(PARAM_ISP_ADJUST)
+#define PARAM_ISP_AFC			INC_NUM(PARAM_ISP_METERING)
+#define PARAM_ISP_OTF_OUTPUT		INC_NUM(PARAM_ISP_AFC)
+#define PARAM_ISP_DMA1_OUTPUT		INC_NUM(PARAM_ISP_OTF_OUTPUT)
+#define PARAM_ISP_DMA2_OUTPUT		INC_NUM(PARAM_ISP_DMA1_OUTPUT)
+#define PARAM_DRC_CONTROL		INC_NUM(PARAM_ISP_DMA2_OUTPUT)
+#define PARAM_DRC_OTF_INPUT		INC_NUM(PARAM_DRC_CONTROL)
+#define PARAM_DRC_DMA_INPUT		INC_NUM(PARAM_DRC_OTF_INPUT)
+#define PARAM_DRC_OTF_OUTPUT		INC_NUM(PARAM_DRC_DMA_INPUT)
+#define PARAM_SCALERC_CONTROL		INC_NUM(PARAM_DRC_OTF_OUTPUT)
+#define PARAM_SCALERC_OTF_INPUT		INC_NUM(PARAM_SCALERC_CONTROL)
+#define PARAM_SCALERC_IMAGE_EFFECT	INC_NUM(PARAM_SCALERC_OTF_INPUT)
+#define PARAM_SCALERC_INPUT_CROP	INC_NUM(PARAM_SCALERC_IMAGE_EFFECT)
+#define PARAM_SCALERC_OUTPUT_CROP	INC_NUM(PARAM_SCALERC_INPUT_CROP)
+#define PARAM_SCALERC_OTF_OUTPUT	INC_NUM(PARAM_SCALERC_OUTPUT_CROP)

+/* 32~63 */
+#define PARAM_SCALERC_DMA_OUTPUT	INC_NUM(PARAM_SCALERC_OTF_OUTPUT)
+#define PARAM_ODC_CONTROL		INC_NUM(PARAM_SCALERC_DMA_OUTPUT)
+#define PARAM_ODC_OTF_INPUT		INC_NUM(PARAM_ODC_CONTROL)
+#define PARAM_ODC_OTF_OUTPUT		INC_NUM(PARAM_ODC_OTF_INPUT)
+#define PARAM_DIS_CONTROL		INC_NUM(PARAM_ODC_OTF_OUTPUT)
+#define PARAM_DIS_OTF_INPUT		INC_NUM(PARAM_DIS_CONTROL)
+#define PARAM_DIS_OTF_OUTPUT		INC_NUM(PARAM_DIS_OTF_INPUT)
+#define PARAM_TDNR_CONTROL		INC_NUM(PARAM_DIS_OTF_OUTPUT)
+#define PARAM_TDNR_OTF_INPUT		INC_NUM(PARAM_TDNR_CONTROL)
+#define PARAM_TDNR_1ST_FRAME		INC_NUM(PARAM_TDNR_OTF_INPUT)
+#define PARAM_TDNR_OTF_OUTPUT		INC_NUM(PARAM_TDNR_1ST_FRAME)
+#define PARAM_TDNR_DMA_OUTPUT		INC_NUM(PARAM_TDNR_OTF_OUTPUT)
+#define PARAM_SCALERP_CONTROL		INC_NUM(PARAM_TDNR_DMA_OUTPUT)
+#define PARAM_SCALERP_OTF_INPUT		INC_NUM(PARAM_SCALERP_CONTROL)
+#define PARAM_SCALERP_IMAGE_EFFECT	INC_NUM(PARAM_SCALERP_OTF_INPUT)
+#define PARAM_SCALERP_INPUT_CROP	INC_NUM(PARAM_SCALERP_IMAGE_EFFECT)
+#define PARAM_SCALERP_OUTPUT_CROP	INC_NUM(PARAM_SCALERP_INPUT_CROP)
+#define PARAM_SCALERP_ROTATION		INC_NUM(PARAM_SCALERP_OUTPUT_CROP)
+#define PARAM_SCALERP_FLIP		INC_NUM(PARAM_SCALERP_ROTATION)
+#define PARAM_SCALERP_OTF_OUTPUT	INC_NUM(PARAM_SCALERP_FLIP)
+#define PARAM_SCALERP_DMA_OUTPUT	INC_NUM(PARAM_SCALERP_OTF_OUTPUT)
+#define PARAM_FD_CONTROL		INC_NUM(PARAM_SCALERP_DMA_OUTPUT)
+#define PARAM_FD_OTF_INPUT		INC_NUM(PARAM_FD_CONTROL)
+#define PARAM_FD_DMA_INPUT		INC_NUM(PARAM_FD_OTF_INPUT)
+#define PARAM_FD_CONFIG			INC_NUM(PARAM_FD_DMA_INPUT)
+#define PARAM_END			INC_NUM(PARAM_FD_CONFIG)

Isn't it much easier to define these as enum ?

+#define PARAM_STRNUM_GLOBAL		(PARAM_GLOBAL_SHOTMODE)
+#define PARAM_RANGE_GLOBAL		1
+#define PARAM_STRNUM_SENSOR		(PARAM_SENSOR_BYPASS)
+#define PARAM_RANGE_SENSOR		5
+#define PARAM_STRNUM_BUFFER		(PARAM_BUFFER_BYPASS)
+#define PARAM_RANGE_BUFFER		3
+#define PARAM_STRNUM_ISP		(PARAM_ISP_BYPASS)
+#define PARAM_RANGE_ISP			15
+#define PARAM_STRNUM_DRC		(PARAM_DRC_BYPASS)
+#define PARAM_RANGE_DRC			4
+#define PARAM_STRNUM_SCALERC		(PARAM_SCALERC_BYPASS)
+#define PARAM_RANGE_SCALERC		7
+#define PARAM_STRNUM_ODC		(PARAM_ODC_BYPASS)
+#define PARAM_RANGE_ODC			3
+#define PARAM_STRNUM_DIS		(PARAM_DIS_BYPASS)
+#define PARAM_RANGE_DIS			3
+#define PARAM_STRNUM_TDNR		(PARAM_TDNR_BYPASS)
+#define PARAM_RANGE_TDNR		5
+#define PARAM_STRNUM_SCALERP		(PARAM_SCALERP_BYPASS)
+#define PARAM_RANGE_SCALERP		9
+#define PARAM_STRNUM_LHFD		(PARAM_FD_BYPASS)
+#define PARAM_RANGE_LHFD		4

Are these definitions used anywhere ?

+#define PARAM_LOW_MASK		(0xFFFFFFFF)
+#define PARAM_HIGH_MASK		(0x07FFFFFF)
+

[...]

+struct param_dma_input {
+	u32	cmd;
+	u32	width;
+	u32	height;
+	u32	format;
+	u32	bitwidth;
+	u32	plane;
+	u32	order;
+	u32	buffer_number;
+	u32	buffer_address;
+	u32	uibayercropoffsetx;
+	u32	uibayercropoffsety;
+	u32	uibayercropwidth;
+	u32	uibayercropheight;
+	u32	uidmacropoffsetx;
+	u32	uidmacropoffsety;
+	u32	uidmacropwidth;
+	u32	uidmacropheight;
+	u32	uiuserminframetime;
+	u32	uiusermaxframetime;

That is really hard to read. Perhaps we can have some underscores
added so it looks like ui_user_max_frametime ?

+	u32	uiwideframegap;
+	u32	uiframegap;
+	u32	uilinegap;
+	u32	uireserved[PARAMETER_MAX_MEMBER-23];
+	u32	err;
+};