Hi Manjunath,

few more comments below..

On 08/29/2011 05:07 PM, Manjunath Hadli wrote:
> add support for ccdc on dm365 SoC. ccdc is responsible for
> capturing video data- both raw bayer through sync seperate
> signals and through BT656/1120 interfaces. This driver implements
> the hardware functionality. Mainly- setting of hardware, validation
> of parameters, and isr configuration.
> 
> Signed-off-by: Manjunath Hadli<manjunath.ha...@ti.com>
> Signed-off-by: Nagabhushana Netagunte<nagabhushana.netagu...@ti.com>
> ---
>   drivers/media/video/davinci/ccdc_types.h      |   43 +
>   drivers/media/video/davinci/dm365_ccdc.c      | 1519 
> +++++++++++++++++++++++++
>   drivers/media/video/davinci/dm365_ccdc.h      |   88 ++
>   drivers/media/video/davinci/dm365_ccdc_regs.h |  309 +++++
>   include/linux/dm365_ccdc.h                    |  664 +++++++++++
>   5 files changed, 2623 insertions(+), 0 deletions(-)
>   create mode 100644 drivers/media/video/davinci/ccdc_types.h
>   create mode 100644 drivers/media/video/davinci/dm365_ccdc.c
>   create mode 100644 drivers/media/video/davinci/dm365_ccdc.h
>   create mode 100644 drivers/media/video/davinci/dm365_ccdc_regs.h
>   create mode 100644 include/linux/dm365_ccdc.h
...
> +#define CCDC_LINEAR_LUT0_ADDR                        0x1c7c000
> +#define CCDC_LINEAR_LUT1_ADDR                        0x1c7c400
> +
> +/* Masks&  Shifts below */
> +#define START_PX_HOR_MASK                    (0x7fff)
> +#define NUM_PX_HOR_MASK                              (0x7fff)
> +#define START_VER_ONE_MASK                   (0x7fff)
> +#define START_VER_TWO_MASK                   (0x7fff)
> +#define NUM_LINES_VER                                (0x7fff)
> +
> +/* gain - offset masks */
> +#define GAIN_INTEGER_MASK                    (0x7)
> +#define GAIN_INTEGER_SHIFT                   (0x9)
> +#define GAIN_DECIMAL_MASK                    (0x1ff)
> +#define OFFSET_MASK                          (0xfff)
> +#define GAIN_SDRAM_EN_SHIFT                  (12)
> +#define GAIN_IPIPE_EN_SHIFT                  (13)
> +#define GAIN_H3A_EN_SHIFT                    (14)
> +#define OFST_SDRAM_EN_SHIFT                  (8)
> +#define OFST_IPIPE_EN_SHIFT                  (9)
> +#define OFST_H3A_EN_SHIFT                    (10)
> +#define GAIN_OFFSET_EN_MASK                  (0x7700)
> +
> +/* Culling */
> +#define CULL_PAT_EVEN_LINE_SHIFT             (8)
> +
> +/* CCDCFG register */
> +#define CCDC_YCINSWP_RAW                     (0x00<<  4)
> +#define CCDC_YCINSWP_YCBCR                   (0x01<<  4)
> +#define CCDC_CCDCFG_FIDMD_LATCH_VSYNC                (0x00<<  6)
> +#define CCDC_CCDCFG_WENLOG_AND                       (0x00<<  8)
> +#define CCDC_CCDCFG_TRGSEL_WEN                       (0x00<<  9)
> +#define CCDC_CCDCFG_EXTRG_DISABLE            (0x00<<  10)
> +#define CCDC_LATCH_ON_VSYNC_DISABLE          (0x01<<  15)
> +#define CCDC_LATCH_ON_VSYNC_ENABLE           (0x00<<  15)
> +#define CCDC_DATA_PACK_MASK                  (0x03)
> +#define CCDC_DATA_PACK16                     (0x0)
> +#define CCDC_DATA_PACK12                     (0x1)
> +#define CCDC_DATA_PACK8                              (0x2)
> +#define CCDC_PIX_ORDER_SHIFT                 (11)
> +#define CCDC_PIX_ORDER_MASK                  (0x01)
> +#define CCDC_BW656_ENABLE                    (0x01<<  5)
> +
> +/* MODESET registers */
> +#define CCDC_VDHDOUT_INPUT                   (0x00<<  0)
> +#define CCDC_INPUT_MASK                              (0x03)
> +#define CCDC_INPUT_SHIFT                     (12)
> +#define CCDC_RAW_INPUT_MODE                  (0x00)
> +#define CCDC_FID_POL_MASK                    (0x01)
> +#define CCDC_FID_POL_SHIFT                   (4)
> +#define CCDC_HD_POL_MASK                     (0x01)
> +#define CCDC_HD_POL_SHIFT                    (3)
> +#define CCDC_VD_POL_MASK                     (0x01)
> +#define CCDC_VD_POL_SHIFT                    (2)
> +#define CCDC_DATAPOL_NORMAL                  (0x00)
> +#define CCDC_DATAPOL_MASK                    (0x01)
> +#define CCDC_DATAPOL_SHIFT                   (6)
> +#define CCDC_EXWEN_DISABLE                   (0x00)
> +#define CCDC_EXWEN_MASK                              (0x01)
> +#define CCDC_EXWEN_SHIFT                     (5)
> +#define CCDC_FRM_FMT_MASK                    (0x01)
> +#define CCDC_FRM_FMT_SHIFT                   (7)
> +#define CCDC_DATASFT_MASK                    (0x07)
> +#define CCDC_DATASFT_SHIFT                   (8)
> +#define CCDC_LPF_SHIFT                               (14)
> +#define CCDC_LPF_MASK                                (0x1)
> +
> +/* GAMMAWD registers */
> +#define CCDC_ALAW_GAMA_WD_MASK                       (0xf)
> +#define CCDC_ALAW_GAMA_WD_SHIFT                      (1)
> +#define CCDC_ALAW_ENABLE                     (0x01)
> +#define CCDC_GAMMAWD_CFA_MASK                        (0x01)
> +#define CCDC_GAMMAWD_CFA_SHIFT                       (5)
> +
> +/* HSIZE registers */
> +#define CCDC_HSIZE_FLIP_MASK                 (0x01)
> +#define CCDC_HSIZE_FLIP_SHIFT                        (12)
> +#define CCDC_LINEOFST_MASK                   (0xfff)
> +
> +/* MISC registers */
> +#define CCDC_DPCM_EN_SHIFT                   (12)
> +#define CCDC_DPCM_EN_MASK                    (1)
> +#define CCDC_DPCM_PREDICTOR_SHIFT            (13)
> +#define CCDC_DPCM_PREDICTOR_MASK             (1)
> +
> +/* Black clamp related */
> +#define CCDC_BC_DCOFFSET_MASK                        (0x1fff)
> +#define CCDC_BC_MODE_COLOR_MASK                      (1)
> +#define CCDC_BC_MODE_COLOR_SHIFT             (4)
> +#define CCDC_HORZ_BC_MODE_MASK                       (3)
> +#define CCDC_HORZ_BC_MODE_SHIFT                      (1)
> +#define CCDC_HORZ_BC_WIN_COUNT_MASK          (0x1f)
> +#define CCDC_HORZ_BC_WIN_SEL_SHIFT           (5)
> +#define CCDC_HORZ_BC_PIX_LIMIT_SHIFT         (6)
> +#define CCDC_HORZ_BC_WIN_H_SIZE_MASK         (3)
> +#define CCDC_HORZ_BC_WIN_H_SIZE_SHIFT                (8)
> +#define CCDC_HORZ_BC_WIN_V_SIZE_MASK         (3)
> +#define CCDC_HORZ_BC_WIN_V_SIZE_SHIFT                (12)
> +#define CCDC_HORZ_BC_WIN_START_H_MASK                (0x1fff)
> +#define CCDC_HORZ_BC_WIN_START_V_MASK                (0x1fff)
> +#define CCDC_VERT_BC_OB_H_SZ_MASK            (7)
> +#define CCDC_VERT_BC_RST_VAL_SEL_MASK                (3)
> +#define      CCDC_VERT_BC_RST_VAL_SEL_SHIFT          (4)
> +#define CCDC_VERT_BC_LINE_AVE_COEF_SHIFT     (8)
> +#define      CCDC_VERT_BC_OB_START_HORZ_MASK         (0x1fff)
> +#define CCDC_VERT_BC_OB_START_VERT_MASK              (0x1fff)
> +#define CCDC_VERT_BC_OB_VERT_SZ_MASK         (0x1fff)
> +#define CCDC_VERT_BC_RST_VAL_MASK            (0xfff)
> +#define CCDC_BC_VERT_START_SUB_V_MASK                (0x1fff)
> +
> +/* VDFC registers */
> +#define CCDC_VDFC_EN_SHIFT                   (4)
> +#define CCDC_VDFC_CORR_MOD_MASK                      (3)
> +#define CCDC_VDFC_CORR_MOD_SHIFT             (5)
> +#define CCDC_VDFC_CORR_WHOLE_LN_SHIFT                (7)
> +#define CCDC_VDFC_LEVEL_SHFT_MASK            (7)
> +#define CCDC_VDFC_LEVEL_SHFT_SHIFT           (8)
> +#define CCDC_VDFC_SAT_LEVEL_MASK             (0xfff)
> +#define CCDC_VDFC_POS_MASK                   (0x1fff)
> +#define CCDC_DFCMEMCTL_DFCMARST_SHIFT                (2)
> +
> +/* CSC registers */
> +#define CCDC_CSC_COEF_INTEG_MASK             (7)
> +#define CCDC_CSC_COEF_DECIMAL_MASK           (0x1f)
> +#define CCDC_CSC_COEF_INTEG_SHIFT            (5)
> +#define CCDC_CSCM_MSB_SHIFT                  (8)
> +#define CCDC_DF_CSC_SPH_MASK                 (0x1fff)
> +#define CCDC_DF_CSC_LNH_MASK                 (0x1fff)
> +#define CCDC_DF_CSC_SLV_MASK                 (0x1fff)
> +#define CCDC_DF_CSC_LNV_MASK                 (0x1fff)
> +#define CCDC_DF_NUMLINES                     (0x7fff)
> +#define CCDC_DF_NUMPIX                               (0x1fff)
> +
> +/* Offsets for LSC/DFC/Gain */
> +#define CCDC_DATA_H_OFFSET_MASK                      (0x1fff)
> +#define CCDC_DATA_V_OFFSET_MASK                      (0x1fff)
> +
> +/* Linearization */
> +#define CCDC_LIN_CORRSFT_MASK                        (7)
> +#define CCDC_LIN_CORRSFT_SHIFT                       (4)
> +#define CCDC_LIN_SCALE_FACT_INTEG_SHIFT              (10)
> +#define CCDC_LIN_SCALE_FACT_DECIMAL_MASK     (0x3ff)
> +#define CCDC_LIN_ENTRY_MASK                  (0x3ff)
> +
> +#define CCDC_DF_FMTRLEN_MASK                 (0x1fff)
> +#define CCDC_DF_FMTHCNT_MASK                 (0x1fff)
> +
> +/* Pattern registers */
> +#define CCDC_PG_EN                           (1<<  3)
> +#define CCDC_SEL_PG_SRC                              (3<<  4)
> +#define CCDC_PG_VD_POL_SHIFT                 (0)
> +#define CCDC_PG_HD_POL_SHIFT                 (1)
> +
> +/*masks and shifts*/
> +#define CCDC_SYNCEN_VDHDEN_MASK                      (1<<  0)
> +#define CCDC_SYNCEN_WEN_MASK                 (1<<  1)
> +#define CCDC_SYNCEN_WEN_SHIFT                        1
> +
> +#endif
> diff --git a/include/linux/dm365_ccdc.h b/include/linux/dm365_ccdc.h
> new file mode 100644
> index 0000000..4e50529
> --- /dev/null
> +++ b/include/linux/dm365_ccdc.h
> @@ -0,0 +1,664 @@
...
> +#define VPFE_CMD_S_CCDC_RAW_PARAMS _IOW('V', 1, \
> +                                     struct ccdc_config_params_raw)
> +#define VPFE_CMD_G_CCDC_RAW_PARAMS _IOR('V', 2, \
> +                                     struct ccdc_config_params_raw)
> +/**
> + * ccdc float type S8Q8/U8Q8
> + */
> +struct ccdc_float_8 {
> +     /* 8 bit integer part */
> +     unsigned char integer;
> +     /* 8 bit decimal part */
> +     unsigned char decimal;
> +};

Isn't it better to use explicit width type, like u8, u16, etc. ?
Then we could just have:

+struct ccdc_float_8 {
+       u8 integer;
+       u8 decimal;
+};


> +
> +/**
> + * brief ccdc float type U16Q16/S16Q16

> + */
> +struct ccdc_float_16 {
> +     /* 16 bit integer part */
> +     unsigned short integer;
> +     /* 16 bit decimal part */
> +     unsigned short decimal;
> +};

and 

+struct ccdc_float_16 {
+       u16 integer;
+       u16 decimal;
+};

> +
> +/*
> + * ccdc image(target) window parameters
> + */
> +struct ccdc_cropwin {
> +     /* horzontal offset of the top left corner in pixels */
> +     unsigned int left;
> +     /* vertical offset of the top left corner in pixels */
> +     unsigned int top;
> +     /* width in pixels of the rectangle */
> +     unsigned int width;
> +     /* height in lines of the rectangle */
> +     unsigned int height;
> +};

How about using struct v4l2_rect instead ?

...
> +/**
> + * CCDC image data size
> + */
> +enum ccdc_data_size {
> +     /* 8 bits */
> +     CCDC_8_BITS,
> +     /* 9 bits */
> +     CCDC_9_BITS,
> +     /* 10 bits */
> +     CCDC_10_BITS,
> +     /* 11 bits */
> +     CCDC_11_BITS,
> +     /* 12 bits */
> +     CCDC_12_BITS,
> +     /* 13 bits */
> +     CCDC_13_BITS,
> +     /* 14 bits */
> +     CCDC_14_BITS,
> +     /* 15 bits */
> +     CCDC_15_BITS,
> +     /* 16 bits */
> +     CCDC_16_BITS
> +};
> +
> +/**
> + * CCDC image data shift to right
> + */
> +enum ccdc_datasft {
> +     /* No Shift */
> +     CCDC_NO_SHIFT,
> +     /* 1 bit Shift */
> +     CCDC_1BIT_SHIFT,
> +     /* 2 bit Shift */
> +     CCDC_2BIT_SHIFT,
> +     /* 3 bit Shift */
> +     CCDC_3BIT_SHIFT,
> +     /* 4 bit Shift */
> +     CCDC_4BIT_SHIFT,
> +     /* 5 bit Shift */
> +     CCDC_5BIT_SHIFT,
> +     /* 6 bit Shift */
> +     CCDC_6BIT_SHIFT
> +};
> +
> +/**
> + * MSB of image data connected to sensor port
> + */
> +enum ccdc_data_msb {
> +     /* MSB b15 */
> +     CCDC_BIT_MSB_15,
> +     /* MSB b14 */
> +     CCDC_BIT_MSB_14,
> +     /* MSB b13 */
> +     CCDC_BIT_MSB_13,
> +     /* MSB b12 */
> +     CCDC_BIT_MSB_12,
> +     /* MSB b11 */
> +     CCDC_BIT_MSB_11,
> +     /* MSB b10 */
> +     CCDC_BIT_MSB_10,
> +     /* MSB b9 */
> +     CCDC_BIT_MSB_9,
> +     /* MSB b8 */
> +     CCDC_BIT_MSB_8,
> +     /* MSB b7 */
> +     CCDC_BIT_MSB_7

Could you live without the comments in these 3 enum declarations ? 
They don't seem to add any information.


--
Regards,
Sylwester

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to