Sekhar, > >Hi Murali, > >Here is a (styling related) review from an non-video person. The [MK] The styling was mostly done by one of our intern who is new to open source coding. So I will fix it based on your comments. >review is neither complete nor exhaustive (the patch is huge!), >but I thought will send across whatever I have for you to take a look. > [MK] Yes Sure! >On Wed, Dec 02, 2009 at 03:08:49, Karicheri, Muralidharan wrote: >> From: Muralidharan Karicheri <m-karicheri2@xxxxxx> >> >> This patch is for adding support for DM365 CCDC. This will allow to >> capture YUV video frames from TVP5146 video decoder on DM365 EVM. The >vpfe >> capture driver will use this module to configure ISIF (a.k.a CCDC) >> module to allow YUV data capture. This driver is written for >ccdc_hw_device >> interface used by vpfe capture driver to configure the ccdc module. >> This patch is tested using NTSC & PAL video sources and verified for >> both formats. >> >> NOTE: This is the initial version for review. > >Typically "RFC" is put instead of "PATCH" in subject line >to convey this. [MK] IMO, RFC is for something that is a proposal to make some changes in the architecture/implementation to get feedback from the reviewers. But here it is a real code that is being reviewed. I just mentioned it to make sure no body complains that it doesn't apply to the latest tree :). This is based on what I see in the mailing list. > >> >> Signed-off-by: Muralidharan Karicheri <m-karicheri2@xxxxxx> >> --- >> drivers/media/video/davinci/dm365_ccdc.c | 1529 >+++++++++++++++++++++++++ >> drivers/media/video/davinci/dm365_ccdc_regs.h | 293 +++++ >> include/media/davinci/dm365_ccdc.h | 555 +++++++++ >> 3 files changed, 2377 insertions(+), 0 deletions(-) >> create mode 100644 drivers/media/video/davinci/dm365_ccdc.c >> create mode 100644 drivers/media/video/davinci/dm365_ccdc_regs.h >> create mode 100644 include/media/davinci/dm365_ccdc.h >> >> diff --git a/drivers/media/video/davinci/dm365_ccdc.c >b/drivers/media/video/davinci/dm365_ccdc.c > >Hopefully it is possible to choose a "generic" name >instead of tying it to an SoC. > [MK]I agree there is problem here. For example, we have same ccdc IP used across DM6446, OMAP34xx and Sitara (earlier Shiva). We have it named currently as dm644x_ccdc.c. I have checked the DM6446 and OMAP34xx PRG and finds that CCDC IP has a Peripheral identification (TID) and class identification (CID). For OMAP & DM6446, they have the same values. So We might be able to rename them as ccdc_<TID><CID>.c. But for CCDC on DM355, we don't have a PID/CID for ccdc. So how do we name it? I don't see a uniform way we can name the driver for these IPs. Probably change dm365_ccdc.c to isif.c since that is the IP name on DM365. ISIF stands for Image Sensor Interface. >> new file mode 100644 >> index 0000000..2f27696 >> --- /dev/null >> +++ b/drivers/media/video/davinci/dm365_ccdc.c >> @@ -0,0 +1,1529 @@ > >[...] > >> +#include <linux/delay.h> >> +#include <linux/platform_device.h> >> +#include <linux/uaccess.h> >> +#include <linux/io.h> >> +#include <linux/videodev2.h> >> +#include <mach/mux.h> >> +#include <media/davinci/dm365_ccdc.h> >> +#include <media/davinci/vpss.h> >> +#include "dm365_ccdc_regs.h" >> +#include "ccdc_hw_device.h" > >Typically the includes are grouped using empty lines >based on the folder "linux", "media" "mach" etc. > [MK]This is not strictly followed in the community, But I think it is good to do this for better readability. >> + >> +static struct device *dev; >> + >> +/* Defauts for module configuation paramaters */ >> +static struct ccdc_config_params_raw ccdc_config_defaults = { >> + .linearize = { >> + .en = 0, >> + .corr_shft = CCDC_NO_SHIFT, >> + .scale_fact = {1, 0}, >> + }, >> + .df_csc = { >> + .df_or_csc = 0, >> + .csc = { >> + .en = 0 > >Should use ',' at the end of line so adding >new members leads to adding just one line. >There are more of these in this static init >below. > [MK] Ok. >> + }, >> + }, >> + .dfc = { >> + .en = 0 >> + }, >> + .bclamp = { >> + .en = 0 >> + }, >> + .gain_offset = { >> + .gain = { >> + .r_ye = {1, 0}, >> + .gr_cy = {1, 0}, >> + .gb_g = {1, 0}, >> + .b_mg = {1, 0}, >> + }, >> + }, >> + .culling = { >> + .hcpat_odd = 0xff, >> + .hcpat_even = 0xff, >> + .vcpat = 0xff >> + }, >> + .compress = { >> + .alg = CCDC_ALAW, >> + }, >> +}; >> + >> +/* ISIF operation configuration */ >> +struct ccdc_oper_config { >> + enum vpfe_hw_if_type if_type; >> + struct ccdc_ycbcr_config ycbcr; >> + struct ccdc_params_raw bayer; >> + enum ccdc_data_pack data_pack; >> + void *__iomem base_addr; >> + void *__iomem linear_tbl0_addr; >> + void *__iomem linear_tbl1_addr; > >Usually it is void __iomem *foo; [MK] Correct. > >> +}; >> + >> +static struct ccdc_oper_config ccdc_cfg = { >> + .ycbcr = { >> + .pix_fmt = CCDC_PIXFMT_YCBCR_8BIT, >> + .frm_fmt = CCDC_FRMFMT_INTERLACED, >> + .win = CCDC_WIN_NTSC, >> + .fid_pol = VPFE_PINPOL_POSITIVE, >> + .vd_pol = VPFE_PINPOL_POSITIVE, >> + .hd_pol = VPFE_PINPOL_POSITIVE, >> + .pix_order = CCDC_PIXORDER_CBYCRY, >> + .buf_type = CCDC_BUFTYPE_FLD_INTERLEAVED, >> + }, >> + .bayer = { >> + .pix_fmt = CCDC_PIXFMT_RAW, >> + .frm_fmt = CCDC_FRMFMT_PROGRESSIVE, >> + .win = CCDC_WIN_VGA, >> + .fid_pol = VPFE_PINPOL_POSITIVE, >> + .vd_pol = VPFE_PINPOL_POSITIVE, >> + .hd_pol = VPFE_PINPOL_POSITIVE, >> + .gain = { >> + .r_ye = {1, 0}, >> + .gr_cy = {1, 0}, >> + .gb_g = {1, 0}, >> + .b_mg = {1, 0}, >> + }, >> + .cfa_pat = CCDC_CFA_PAT_MOSAIC, >> + .data_msb = CCDC_BIT_MSB_11, >> + .config_params = { >> + .data_shift = CCDC_NO_SHIFT, >> + .col_pat_field0 = { >> + .olop = CCDC_GREEN_BLUE, >> + .olep = CCDC_BLUE, >> + .elop = CCDC_RED, >> + .elep = CCDC_GREEN_RED, >> + }, >> + .col_pat_field1 = { >> + .olop = CCDC_GREEN_BLUE, >> + .olep = CCDC_BLUE, >> + .elop = CCDC_RED, >> + .elep = CCDC_GREEN_RED, >> + }, >> + .test_pat_gen = 0, >> + }, >> + }, >> + .data_pack = CCDC_DATA_PACK8, >> +}; >> + >> +/* Raw Bayer formats */ >> +static u32 ccdc_raw_bayer_pix_formats[] = >> + {V4L2_PIX_FMT_SBGGR8, V4L2_PIX_FMT_SBGGR16}; >> + >> +/* Raw YUV formats */ >> +static u32 ccdc_raw_yuv_pix_formats[] = >> + {V4L2_PIX_FMT_UYVY, V4L2_PIX_FMT_YUYV}; >> + >> +/* register access routines */ >> +static inline u32 regr(u32 offset) >> +{ >> + return __raw_readl(ccdc_cfg.base_addr + offset); >> +} >> + >> +static inline void regw(u32 val, u32 offset) >> +{ >> + __raw_writel(val, ccdc_cfg.base_addr + offset); >> +} >> + >> +static inline u32 ccdc_merge(u32 mask, u32 val, u32 offset) > >"merge" was not intuitive until I read the implementation. >How about "modify" (as used in arch/arm/mach-davinci/dma.c)? Merge sounds okay to me. Besides, it is within the same file. So not difficult to check what the function does. I will add a function header. > >> +{ >> + u32 new_val = (regr(offset) & ~mask) | (val & mask); >> + >> + regw(new_val, offset); >> + return new_val; >> +} >> + >> +static inline void regw_lin_tbl(u32 val, u32 offset, int i) >> +{ >> + if (!i) >> + __raw_writel(val, ccdc_cfg.linear_tbl0_addr + offset); >> + else >> + __raw_writel(val, ccdc_cfg.linear_tbl1_addr + offset); >> +} >> + >> +static void ccdc_disable_all_modules(void) >> +{ >> + /* disable BC */ >> + regw(0, CLAMPCFG); >> + /* disable vdfc */ >> + regw(0, DFCCTL); >> + /* disable CSC */ >> + regw(0, CSCCTL); >> + /* disable linearization */ >> + regw(0, LINCFG0); >> + /* disable other modules here as they are supported */ >> +} >> + >> +static void ccdc_enable(int en) >> +{ >> + if (!en) { >> + /* Before disable isif, disable all ISIF modules */ > >Could you use ccdc instead of isif in the comments too? If name of the file to be changed, then I will rename all functions to isif_ and comments will reflect the same. > >> + ccdc_disable_all_modules(); >> + /** >> + * wait for next VD. Assume lowest scan rate is 12 Hz. So >> + * 100 msec delay is good enough >> + */ >> + } > >The comment explaining the msleep seems mis-placed. > [MK] Ok. >> + msleep(100); >> + ccdc_merge(CCDC_SYNCEN_VDHDEN_MASK, en, SYNCEN); >> +} >> + >> +static void ccdc_enable_output_to_sdram(int en) >> +{ >> + ccdc_merge(CCDC_SYNCEN_WEN_MASK, en << CCDC_SYNCEN_WEN_SHIFT, >SYNCEN); >> +} >> + >> +static void ccdc_config_culling(struct ccdc_cul *cul) >> +{ >> + u32 val; >> + >> + /* Horizontal pattern */ >> + val = (cul->hcpat_even) << CULL_PAT_EVEN_LINE_SHIFT; > >No need of parenthesis. [MK] Correct. > >> + val |= cul->hcpat_odd; >> + regw(val, CULH); >> + >> + /* vertical pattern */ >> + regw(cul->vcpat, CULV); >> + >> + /* LPF */ >> + ccdc_merge((CCDC_LPF_MASK << CCDC_LPF_SHIFT), >> + (cul->en_lpf << CCDC_LPF_SHIFT), MODESET); > >.. ditto .. > [MK] Ok. >> +} >> + >> +static void ccdc_config_gain_offset(void) >> +{ >> + struct ccdc_gain_offsets_adj *gain_off_ptr = >> + &ccdc_cfg.bayer.config_params.gain_offset; >> + u32 val; >> + >> + val = ((gain_off_ptr->gain_sdram_en & 1) << GAIN_SDRAM_EN_SHIFT) | >> + ((gain_off_ptr->gain_ipipe_en & 1) << GAIN_IPIPE_EN_SHIFT) | >> + ((gain_off_ptr->gain_h3a_en & 1) << GAIN_H3A_EN_SHIFT) | >> + ((gain_off_ptr->offset_sdram_en & 1) << OFST_SDRAM_EN_SHIFT) | >> + ((gain_off_ptr->offset_ipipe_en & 1) << OFST_IPIPE_EN_SHIFT) | >> + ((gain_off_ptr->offset_h3a_en & 1) << OFST_H3A_EN_SHIFT); > >I think the intent here is to convert an arbitrary >gain_off_ptr->foo to 0 or 1. In that case you can >use !!gain_off_ptr->foo > [MK] That is a good idea ! >> + >> + ccdc_merge(GAIN_OFFSET_EN_MASK, val, CGAMMAWD); >> + >> + val = ((gain_off_ptr->gain.r_ye.integer & GAIN_INTEGER_MASK) >> + << GAIN_INTEGER_SHIFT); >> + val |= (ccdc_cfg.bayer. >> + config_params.gain_offset.gain.r_ye.decimal & >> + GAIN_DECIMAL_MASK); >> + regw(val, CRGAIN); >> + >> + val = ((gain_off_ptr->gain.gr_cy >> + .integer & GAIN_INTEGER_MASK) << GAIN_INTEGER_SHIFT); >> + val |= (gain_off_ptr->gain.gr_cy >> + .decimal & GAIN_DECIMAL_MASK); >> + regw(val, CGRGAIN); >> + >> + val = ((gain_off_ptr->gain.gb_g >> + .integer & GAIN_INTEGER_MASK) << GAIN_INTEGER_SHIFT); >> + val |= (gain_off_ptr->gain.gb_g >> + .decimal & GAIN_DECIMAL_MASK); >> + regw(val, CGBGAIN); >> + >> + val = ((gain_off_ptr->gain.b_mg >> + .integer & GAIN_INTEGER_MASK) << GAIN_INTEGER_SHIFT); >> + val |= (gain_off_ptr->gain.b_mg >> + .decimal & GAIN_DECIMAL_MASK); > >Breaking the line at the . is making reading difficult. >Can you break at '<<' instead? > [MK] Correct. >> + regw(val, CBGAIN); >> + >> + regw((gain_off_ptr->offset & >> + OFFSET_MASK), COFSTA); > >Not sure if we really need to break here. Are you hitting >the 80 char limit even here? > [MK] Looks unnecessary >> +} >> + >> +static void ccdc_restore_defaults(void) >> +{ >> + enum vpss_ccdc_source_sel source = VPSS_CCDCIN; >> + int i; >> + >> + memcpy(&ccdc_cfg.bayer.config_params, &ccdc_config_defaults, >> + sizeof(struct ccdc_config_params_raw)); >> + >> + dev_dbg(dev, "\nstarting ccdc_restore_defaults..."); >> + /* Enable clock to ISIF, IPIPEIF and BL */ >> + vpss_enable_clock(VPSS_CCDC_CLOCK, 1); >> + vpss_enable_clock(VPSS_IPIPEIF_CLOCK, 1); >> + vpss_enable_clock(VPSS_BL_CLOCK, 1); >> + >> + /* set all registers to default value */ >> + for (i = 0; i <= 0x1f8; i += 4) >> + regw(0, i); > >Hmm, something like this is not usually expected. >You would anyway be programming all the relevant >registers again. So, why is this required? > [MK] Will investigate if this can be removed. I think the implementation assumes that all register values are restored to default. >> + >> + /* no culling support */ >> + regw(0xffff, CULH); >> + regw(0xff, CULV); >> + >> + /* Set default offset and gain */ >> + ccdc_config_gain_offset(); >> + >> + vpss_select_ccdc_source(source); >> + >> + dev_dbg(dev, "\nEnd of ccdc_restore_defaults..."); >> +} >> + >> +static int ccdc_open(struct device *device) >> +{ >> + dev = device; >> + ccdc_restore_defaults(); >> + return 0; >> +} >> + >> +/* This function will configure the window size to be capture in CCDC >reg */ >> +static void ccdc_setwin(struct v4l2_rect *image_win, >> + enum ccdc_frmfmt frm_fmt, int ppc) >> +{ >> + int horz_start, horz_nr_pixels; >> + int vert_start, vert_nr_lines; >> + int mid_img = 0; >> + >> + dev_dbg(dev, "\nStarting ccdc_setwin..."); >> + /** >> + * ppc - per pixel count. indicates how many pixels per cell > >Kernel doc style comments are only useful for >API description, I guess. [MK] yes. Will change > >> + * output to SDRAM. example, for ycbcr, it is one y and one c, so 2. >> + * raw capture this is 1 >> + */ >> + horz_start = image_win->left << (ppc - 1); >> + horz_nr_pixels = ((image_win->width) << (ppc - 1)) - 1; >> + >> + /* Writing the horizontal info into the registers */ >> + regw(horz_start & START_PX_HOR_MASK, SPH); >> + regw(horz_nr_pixels & NUM_PX_HOR_MASK, LNH); >> + vert_start = image_win->top; >> + >> + if (frm_fmt == CCDC_FRMFMT_INTERLACED) { >> + vert_nr_lines = (image_win->height >> 1) - 1; >> + vert_start >>= 1; >> + /* To account for VD since line 0 doesn't have any data */ >> + vert_start += 1; >> + } else { >> + /* To account for VD since line 0 doesn't have any data */ >> + vert_start += 1; >> + vert_nr_lines = image_win->height - 1; >> + /* configure VDINT0 and VDINT1 */ >> + mid_img = vert_start + (image_win->height / 2); >> + regw(mid_img, VDINT1); >> + } >> + >> + regw(0, VDINT0); >> + regw(vert_start & START_VER_ONE_MASK, SLV0); >> + regw(vert_start & START_VER_TWO_MASK, SLV1); >> + regw(vert_nr_lines & NUM_LINES_VER, LNV); >> +} >> + >> +static void ccdc_config_bclamp(struct ccdc_black_clamp *bc) >> +{ >> + u32 val; >> + >> + /** >> + * DC Offset is always added to image data irrespective of bc enable >> + * status >> + */ > >.. ditto .. [MK] Will change > >> + val = bc->dc_offset & CCDC_BC_DCOFFSET_MASK; >> + regw(val, CLDCOFST); >> + >> + if (bc->en) { >> + val = (bc->bc_mode_color & CCDC_BC_MODE_COLOR_MASK) << >> + CCDC_BC_MODE_COLOR_SHIFT; >> + >> + /* Enable BC and horizontal clamp caculation paramaters */ >> + val = val | 1 | ((bc->horz.mode & CCDC_HORZ_BC_MODE_MASK) << >> + CCDC_HORZ_BC_MODE_SHIFT); >> + >> + regw(val, CLAMPCFG); >> + >> + if (bc->horz.mode != CCDC_HORZ_BC_DISABLE) { >> + /** >> + * Window count for calculation >> + * Base window selection >> + * pixel limit >> + * Horizontal size of window >> + * vertical size of the window >> + * Horizontal start position of the window >> + * Vertical start position of the window >> + */ >> + val = (bc->horz.win_count_calc & >> + CCDC_HORZ_BC_WIN_COUNT_MASK) | >> + ((bc->horz.base_win_sel_calc & 1) >> + << CCDC_HORZ_BC_WIN_SEL_SHIFT) | >> + ((bc->horz.clamp_pix_limit & 1) >> + << CCDC_HORZ_BC_PIX_LIMIT_SHIFT) | >> + ((bc->horz.win_h_sz_calc & >> + CCDC_HORZ_BC_WIN_H_SIZE_MASK) >> + << CCDC_HORZ_BC_WIN_H_SIZE_SHIFT) | >> + ((bc->horz.win_v_sz_calc & >> + CCDC_HORZ_BC_WIN_V_SIZE_MASK) >> + << CCDC_HORZ_BC_WIN_V_SIZE_SHIFT); >> + >> + regw(val, CLHWIN0); >> + >> + val = (bc->horz.win_start_h_calc & >> + CCDC_HORZ_BC_WIN_START_H_MASK); >> + regw(val, CLHWIN1); >> + >> + val = >> + (bc->horz. >> + win_start_v_calc & CCDC_HORZ_BC_WIN_START_V_MASK); > >Too much broken line. Suggest breaking at & instead. > [MK] Ok. >> + regw(val, CLHWIN2); >> + } >> + >> + /* vertical clamp caculation paramaters */ >> + >> + /* OB H Valid */ >> + val = (bc->vert.ob_h_sz_calc & CCDC_VERT_BC_OB_H_SZ_MASK); >> + >> + /* Reset clamp value sel for previous line */ >> + val |= ((bc->vert.reset_val_sel & >> + CCDC_VERT_BC_RST_VAL_SEL_MASK) >> + << CCDC_VERT_BC_RST_VAL_SEL_SHIFT); >> + >> + /* Line average coefficient */ >> + val |= (bc->vert.line_ave_coef << >> + CCDC_VERT_BC_LINE_AVE_COEF_SHIFT); >> + regw(val, CLVWIN0); >> + >> + /* Configured reset value */ >> + if (bc->vert.reset_val_sel == >> + CCDC_VERT_BC_USE_CONFIG_CLAMP_VAL) { >> + val = >> + (bc->vert. >> + reset_clamp_val & CCDC_VERT_BC_RST_VAL_MASK); > >.. ditto .. There are other places in the patch >where line breaks needs revisit. [MK] Ok. > >> + regw(val, CLVRV); >> + } >> + >> + /* Optical Black horizontal start position */ >> + val = (bc->vert.ob_start_h & CCDC_VERT_BC_OB_START_HORZ_MASK); >> + regw(val, CLVWIN1); >> + >> + /* Optical Black vertical start position */ >> + val = (bc->vert.ob_start_v & CCDC_VERT_BC_OB_START_VERT_MASK); >> + regw(val, CLVWIN2); >> + >> + val = (bc->vert.ob_v_sz_calc & CCDC_VERT_BC_OB_VERT_SZ_MASK); >> + regw(val, CLVWIN3); >> + >> + /* Vertical start position for BC subtraction */ >> + val = (bc->vert_start_sub & CCDC_BC_VERT_START_SUB_V_MASK); >> + regw(val, CLSV); >> + } >> +} >> + >> +static void ccdc_config_linearization(struct ccdc_linearize *linearize) >> +{ >> + u32 val, i; >> + if (!linearize->en) { > >Typically an empty line is used after variable >declarations. > [MK] Ok. >> + regw(0, LINCFG0); >> + return; >> + } >> + >> + /* shift value for correction */ >> + val = (linearize->corr_shft & CCDC_LIN_CORRSFT_MASK) >> + << CCDC_LIN_CORRSFT_SHIFT; >> + /* enable */ >> + val |= 1; >> + regw(val, LINCFG0); >> + >> + /* Scale factor */ >> + val = (linearize->scale_fact.integer & 1) >> + << CCDC_LIN_SCALE_FACT_INTEG_SHIFT; >> + val |= (linearize->scale_fact.decimal & >> + CCDC_LIN_SCALE_FACT_DECIMAL_MASK); >> + regw(val, LINCFG1); >> + >> + for (i = 0; i < CCDC_LINEAR_TAB_SIZE; i++) { >> + val = linearize->table[i] & CCDC_LIN_ENTRY_MASK; >> + if (i%2) >> + regw_lin_tbl(val, ((i >> 1) << 2), 1); >> + else >> + regw_lin_tbl(val, ((i >> 1) << 2), 0); >> + } >> +} >> + >> +static void ccdc_config_dfc(struct ccdc_dfc *vdfc) >> +{ >> +#define DFC_WRITE_WAIT_COUNT 1000 >> + u32 val, count = DFC_WRITE_WAIT_COUNT; >> + int i; >> + >> + if (!vdfc->en) >> + return; >> + >> + /* Correction mode */ >> + val = ((vdfc->corr_mode & CCDC_VDFC_CORR_MOD_MASK) >> + << CCDC_VDFC_CORR_MOD_SHIFT); >> + >> + /* Correct whole line or partial */ >> + if (vdfc->corr_whole_line) >> + val |= 1 << CCDC_VDFC_CORR_WHOLE_LN_SHIFT; >> + >> + /* level shift value */ >> + val |= (vdfc->def_level_shift & CCDC_VDFC_LEVEL_SHFT_MASK) << >> + CCDC_VDFC_LEVEL_SHFT_SHIFT; >> + >> + regw(val, DFCCTL); >> + >> + /* Defect saturation level */ >> + val = vdfc->def_sat_level & CCDC_VDFC_SAT_LEVEL_MASK; >> + regw(val, VDFSATLV); >> + >> + regw(vdfc->table[0].pos_vert & CCDC_VDFC_POS_MASK, DFCMEM0); >> + regw(vdfc->table[0].pos_horz & CCDC_VDFC_POS_MASK, DFCMEM1); >> + if (vdfc->corr_mode == CCDC_VDFC_NORMAL || >> + vdfc->corr_mode == CCDC_VDFC_HORZ_INTERPOL_IF_SAT) { >> + regw(vdfc->table[0].level_at_pos, DFCMEM2); >> + regw(vdfc->table[0].level_up_pixels, DFCMEM3); >> + regw(vdfc->table[0].level_low_pixels, DFCMEM4); >> + } >> + >> + val = regr(DFCMEMCTL); >> + /* set DFCMARST and set DFCMWR */ >> + val |= 1 << CCDC_DFCMEMCTL_DFCMARST_SHIFT; >> + val |= 1; >> + regw(val, DFCMEMCTL); >> + >> + while (count && (regr(DFCMEMCTL) & 0x01)) >> + count--; > >This is CPU speed dependent. Suggest using udelay() >or loops_per_jiffy instead. > [MK] will investigate. >> + >> + val = regr(DFCMEMCTL); >> + if (!count) { >> + dev_dbg(dev, "defect table write timeout !!!\n"); >> + return; >> + } >> + >> + for (i = 1; i < vdfc->num_vdefects; i++) { >> + regw(vdfc->table[i].pos_vert & CCDC_VDFC_POS_MASK, >> + DFCMEM0); >> + regw(vdfc->table[i].pos_horz & CCDC_VDFC_POS_MASK, >> + DFCMEM1); >> + if (vdfc->corr_mode == CCDC_VDFC_NORMAL || >> + vdfc->corr_mode == CCDC_VDFC_HORZ_INTERPOL_IF_SAT) { >> + regw(vdfc->table[i].level_at_pos, DFCMEM2); >> + regw(vdfc->table[i].level_up_pixels, DFCMEM3); >> + regw(vdfc->table[i].level_low_pixels, DFCMEM4); >> + } >> + val = regr(DFCMEMCTL); >> + /* clear DFCMARST and set DFCMWR */ >> + val &= ~(1 << CCDC_DFCMEMCTL_DFCMARST_SHIFT); > >Could use BIT(x) here. [MK] Will check > >> + val |= 1; >> + regw(val, DFCMEMCTL); >> + >> + count = DFC_WRITE_WAIT_COUNT; >> + while (count && (regr(DFCMEMCTL) & 0x01)) >> + count--; >> + >> + val = regr(DFCMEMCTL); >> + if (!count) { >> + dev_err(dev, "defect table write timeout !!!\n"); >> + return; >> + } >> + } >> + if (vdfc->num_vdefects < CCDC_VDFC_TABLE_SIZE) { >> + /* Extra cycle needed */ >> + regw(0, DFCMEM0); >> + regw(0x1FFF, DFCMEM1); >> + val = 1; >> + regw(val, DFCMEMCTL); >> + } >> + >> + /* enable VDFC */ >> + ccdc_merge((1 << CCDC_VDFC_EN_SHIFT), (1 << CCDC_VDFC_EN_SHIFT), >> + DFCCTL); >> + >> + ccdc_merge((1 << CCDC_VDFC_EN_SHIFT), (0 << CCDC_VDFC_EN_SHIFT), >> + DFCCTL); >> + >> + regw(0x6, DFCMEMCTL); >> + for (i = 0 ; i < vdfc->num_vdefects; i++) { >> + count = DFC_WRITE_WAIT_COUNT; >> + while (count && (regr(DFCMEMCTL) & 0x2)) >> + count--; >> + >> + val = regr(DFCMEMCTL); >> + if (!count) { >> + dev_err(dev, "defect table write timeout !!!\n"); >> + return; >> + } >> + >> + val = regr(DFCMEM0) | regr(DFCMEM1) | regr(DFCMEM2) | >> + regr(DFCMEM3) | regr(DFCMEM4); >> + regw(0x2, DFCMEMCTL); >> + } >> +} >> + > >[...] > >> + >> +static int ccdc_config_raw(void) >> +{ >> + struct ccdc_params_raw *params = &ccdc_cfg.bayer; >> + struct ccdc_config_params_raw *module_params = >> + &ccdc_cfg.bayer.config_params; >> + struct vpss_pg_frame_size frame_size; >> + struct vpss_sync_pol sync; >> + u32 val; >> + >> + dev_dbg(dev, "\nStarting ccdc_config_raw..\n"); >> + >> + /* Configure CCDCFG register */ >> + >> + /** >> + * Set CCD Not to swap input since input is RAW data >> + * Set FID detection function to Latch at V-Sync >> + * Set WENLOG - ccdc valid area >> + * Set TRGSEL >> + * Set EXTRG >> + * Packed to 8 or 16 bits >> + */ >> + >> + val = CCDC_YCINSWP_RAW | CCDC_CCDCFG_FIDMD_LATCH_VSYNC | >> + CCDC_CCDCFG_WENLOG_AND | CCDC_CCDCFG_TRGSEL_WEN | >> + CCDC_CCDCFG_EXTRG_DISABLE | (ccdc_cfg.data_pack & >> + CCDC_DATA_PACK_MASK); >> + >> + dev_dbg(dev, "Writing 0x%x to ...CCDCFG \n", val); >> + regw(val, CCDCFG); >> + >> + /** >> + * Configure the vertical sync polarity(MODESET.VDPOL) >> + * Configure the horizontal sync polarity (MODESET.HDPOL) >> + * Configure frame id polarity (MODESET.FLDPOL) >> + * Configure data polarity >> + * Configure External WEN Selection >> + * Configure frame format(progressive or interlace) >> + * Configure pixel format (Input mode) >> + * Configure the data shift >> + */ >> + >> + val = CCDC_VDHDOUT_INPUT | >> + ((params->vd_pol & CCDC_VD_POL_MASK) << CCDC_VD_POL_SHIFT) | >> + ((params->hd_pol & CCDC_HD_POL_MASK) << CCDC_HD_POL_SHIFT) | >> + ((params->fid_pol & CCDC_FID_POL_MASK) << CCDC_FID_POL_SHIFT) | >> + ((CCDC_DATAPOL_NORMAL & CCDC_DATAPOL_MASK) >> + << CCDC_DATAPOL_SHIFT) | >> + ((CCDC_EXWEN_DISABLE & CCDC_EXWEN_MASK) << CCDC_EXWEN_SHIFT) | >> + ((params->frm_fmt & CCDC_FRM_FMT_MASK) << CCDC_FRM_FMT_SHIFT) | >> + ((params->pix_fmt & CCDC_INPUT_MASK) << CCDC_INPUT_SHIFT) | >> + ((params->config_params.data_shift & CCDC_DATASFT_MASK) >> + << CCDC_DATASFT_SHIFT); >> + >> + regw(val, MODESET); >> + dev_dbg(dev, "Writing 0x%x to MODESET...\n", val); >> + >> + /** >> + * Configure GAMMAWD register >> + * CFA pattern setting >> + */ >> + val = (params->cfa_pat & CCDC_GAMMAWD_CFA_MASK) << >> + CCDC_GAMMAWD_CFA_SHIFT; >> + >> + /* Gamma msb */ >> + if (module_params->compress.alg == CCDC_ALAW) >> + val = val | CCDC_ALAW_ENABLE; > >val |= CCDC_ALAW_ENABLE; > >> + >> + val = val | ((params->data_msb & CCDC_ALAW_GAMA_WD_MASK) << >> + CCDC_ALAW_GAMA_WD_SHIFT); > >.. ditto .. > [MK] Will check. >> +static int ccdc_set_pixel_format(unsigned int pixfmt) >> +{ >> + if (ccdc_cfg.if_type == VPFE_RAW_BAYER) { >> + if (pixfmt == V4L2_PIX_FMT_SBGGR8) { >> + if ((ccdc_cfg.bayer.config_params.compress.alg != >> + CCDC_ALAW) && >> + (ccdc_cfg.bayer.config_params.compress.alg != >> + CCDC_DPCM)) { >> + dev_dbg(dev, "Either configure A-Law or" >> + "DPCM\n"); > >Space required before DPCM > >Thanks, >Sekhar [MK] Ok. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html