Hi Hans, Thanks for the review. On Mon, Dec 1, 2014 at 10:11 AM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote: > Hi all, > > Thanks for the patch, review comments are below. > > For the next version I would appreciate if someone can test this driver with > the latest v4l2-compliance from the v4l-utils git repo. If possible test streaming > as well (v4l2-compliance -s), ideally with both a sensor and a STD receiver input. > But that depends on the available hardware of course. > > I'd like to see the v4l2-compliance output. It's always good to have that on > record. > Following is the compliance output, missed it post it along with patch: root@am437x-evm:~# ./v4l2-compliance -s -i 0 -v Driver Info: Driver name : vpfe Card type :[ 70.363908] vpfe 48326000.vpfe: ================= START STATUS ================= TI AM437x VPFE Bus info : platform:vpfe [ 70.375576] vpfe 48326000.vpfe: ================== END STATUS ================== 48326000.vpfe Driver version: 3.18.0 Capabil[ 70.388485] vpfe 48326000.vpfe: invalid input index: 1 ities : 0x85200001 Video Capture Read/Write Streaming Extended Pix Format Device Capabilities Device Caps : 0x05200001 Video Capture Read/Write Streaming Extended Pix Format Compliance test for device /dev/video0 (not using libv4l2): Required ioctls: test VIDIOC_QUERYCAP: OK Allow for multiple opens: test second video open: OK test VIDIOC_QUERYCAP: OK test VIDIOC_G/S_PRIORITY: OK Debug ioctls: test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported) test VIDIOC_LOG_STATUS: OK Input ioctls: test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) test VIDIOC_ENUMAUDIO: OK (Not Supported) test VIDIOC_G/S/ENUMINPUT: OK test VIDIOC_G/S_AUDIO: OK (Not Supported) Inputs: 1 Audio Inputs: 0 Tuners: 0 Output ioctls: test VIDIOC_G/S_MODULATOR: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_ENUMAUDOUT: OK (Not Supported) test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) test VIDIOC_G/S_AUDOUT: OK (Not Supported) Outputs: 0 Audio Outputs: 0 Modulators: 0 Input/Output configuration ioctls: test VIDIOC_ENUM/G/S/QUERY_STD: OK test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported) test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported) test VIDIOC_G/S_EDID: OK (Not Supported) Test input 0: Control ioctls: test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK (Not Supported) test VIDIOC_QUERYCTRL: OK (Not Supported) test VIDIOC_G/S_CTRL: OK (Not Supported) test VIDIOC_G/S/TRY_EXT_CTRLS: OK (Not Supported) test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported) test VIDIOC_G/S_JPEGCOMP: OK (Not Supported) Standard Controls: 0 Private Controls: 0 Format ioctls: info: found 7 framesizes for pixel format 56595559 info: found 7 framesizes for pixel format 59565955 info: found 7 framesizes for pixel format 52424752 info: found 7 framesizes for pixel format 31384142 info: found 4 formats for buftype 1 test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK test VIDIOC_G/S_PARM: OK test VIDIOC_G_FBUF: OK (Not Supported) test VIDIOC_G_FMT: OK test VIDIOC_TRY_FMT: OK info: Global format check succeeded for type 1 test VIDIOC_S_FMT: OK test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported) Codec ioctls: test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported) test VIDIOC_G_ENC_INDEX: OK (Not Supported) test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported) Buffer ioctls: info: test buftype Video Capture test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK test VIDIOC_EXPBUF: OK Streaming ioctls: test read/write: OK Video Capture: Buffer: 0 Sequence: 0 Field: None Timestamp: 74.956437s Buffer: 1 Sequence: 0 Field: None Timestamp: 74.979310s Buffer: 2 Sequence: 0 Field: None Timestamp: 75.002191s Buffer: 3 Sequence: 0 Field: None Timestamp: 75.208114s Buffer: 0 Sequence: 0 Field: None Timestamp: 75.230998s Buffer: 1 Sequence: 0 Field: None Timestamp: 75.253877s Buffer: 2 Sequence: 0 Field: None Timestamp: 75.276756s Buffer: 3 Sequence: 0 Field: None Timestamp: 75.299637s Buffer: 0 Sequence: 0 Field: None Timestamp: 75.322517s Buffer: 1 Sequence: 0 Field: None Timestamp: 75.345398s Buffer: 2 Sequence: 0 Field: None Timestamp: 75.368279s Buffer: 3 Sequence: 0 Field: None Timestamp: 75.391159s Buffer: 0 Sequence: 0 Field: None Timestamp: 75.414039s Buffer: 1 Sequence: 0 Field: None Timestamp: 75.436919s Buffer: 2 Sequence: 0 Field: None Timestamp: 75.459800s Buffer: 3 Sequence: 0 Field: None Timestamp: 75.482680s Buffer: 0 Sequence: 0 Field: None Timestamp: 75.505560s Buffer: 1 Sequence: 0 Field: None Timestamp: 75.528442s Buffer: 2 Sequence: 0 Field: None Timestamp: 75.551322s Buffer: 3 Sequence: 0 Field: None Timestamp: 75.574202s Buffer: 0 Sequence: 0 Field: None Timestamp: 75.597082s Buffer: 1 Sequence: 0 Field: None Timestamp: 75.619962s Buffer: 2 Sequence: 0 Field: None Timestamp: 75.642842s Buffer: 3 Sequence: 0 Field: None Timestamp: 75.665723s Buffer: 0 Sequence: 0 Field: None Timestamp: 75.688603s Buffer: 1 Sequence: 0 Field: None Timestamp: 75.711485s Buffer: 2 Sequence: 0 Field: None Timestamp: 75.734364s Buffer: 3 Sequence: 0 Field: None Timestamp: 75.757244s Buffer: 0 Sequence: 0 Field: None Timestamp: 75.780126s Buffer: 1 Sequence: 0 Field: None Timestamp: 75.803005s Buffer: 2 Sequence: 0 Field: None Timestamp: 75.825885s Buffer: 3 Sequence: 0 Field: None Timestamp: 75.848766s Buffer: 0 Sequence: 0 Field: None Timestamp: 75.871648s Buffer: 1 Sequence: 0 Field: None Timestamp: 75.894527s Buffer: 2 Sequence: 0 Field: None Timestamp: 75.917407s Buffer: 3 Sequence: 0 Field: None Timestamp: 75.940288s Buffer: 0 Sequence: 0 Field: None Timestamp: 75.963168s Buffer: 1 Sequence: 0 Field: None Timestamp: 75.986048s Buffer: 2 Sequence: 0 Field: None Timestamp: 76.008926s Buffer: 3 Sequence: 0 Field: None Timestamp: 76.031809s Buffer: 0 Sequence: 0 Field: None Timestamp: 76.054691s Buffer: 1 Sequence: 0 Field: None Timestamp: 76.077568s Buffer: 2 Sequence: 0 Field: None Timestamp: 76.100451s Buffer: 3 Sequence: 0 Field: None Timestamp: 76.123330s Buffer: 0 Sequence: 0 Field: None Timestamp: 76.146210s Buffer: 1 Sequence: 0 Field: None Timestamp: 76.169091s Buffer: 2 Sequence: 0 Field: None Timestamp: 76.191970s Buffer: 3 Sequence: 0 Field: None Timestamp: 76.214851s Buffer: 0 Sequence: 0 Field: None Timestamp: 76.237732s Buffer: 1 Sequence: 0 Field: None Timestamp: 76.260613s Buffer: 2 Sequence: 0 Field: None Timestamp: 76.283493s Buffer: 3 Sequence: 0 Field: None Timestamp: 76.306373s Buffer: 0 Sequence: 0 Field: None Timestamp: 76.329253s Buffer: 1 Sequence: 0 Field: None Timestamp: 76.352133s Buffer: 2 Sequence: 0 Field: None Timestamp: 76.375014s Buffer: 3 Sequence: 0 Field: None Timestamp: 76.397894s Buffer: 0 Sequence: 0 Field: None Timestamp: 76.420776s Buffer: 1 Sequence: 0 Field: None Timestamp: 76.443655s Buffer: 2 Sequence: 0 Field: None Timestamp: 76.466535s Buffer: 3 Sequence: 0 Field: None Timestamp: 76.489415s Video Capture (polling): Buffer: 0 Sequence: 0 Field: None Timestamp: 76.512295s Buffer: 1 Sequence: 0 Field: None Timestamp: 76.535177s Buffer: 2 Sequence: 0 Field: None Timestamp: 76.558057s Buffer: 3 Sequence: 0 Field: None Timestamp: 76.580938s Buffer: 0 Sequence: 0 Field: None Timestamp: 76.603818s Buffer: 1 Sequence: 0 Field: None Timestamp: 76.626698s Buffer: 2 Sequence: 0 Field: None Timestamp: 76.649578s Buffer: 3 Sequence: 0 Field: None Timestamp: 76.672459s Buffer: 0 Sequence: 0 Field: None Timestamp: 76.695339s Buffer: 1 Sequence: 0 Field: None Timestamp: 76.718219s Buffer: 2 Sequence: 0 Field: None Timestamp: 76.741102s Buffer: 3 Sequence: 0 Field: None Timestamp: 76.763980s Buffer: 0 Sequence: 0 Field: None Timestamp: 76.786860s Buffer: 1 Sequence: 0 Field: None Timestamp: 76.809741s Buffer: 2 Sequence: 0 Field: None Timestamp: 76.832621s Buffer: 3 Sequence: 0 Field: None Timestamp: 76.855502s Buffer: 0 Sequence: 0 Field: None Timestamp: 76.878382s Buffer: 1 Sequence: 0 Field: None Timestamp: 76.901263s Buffer: 2 Sequence: 0 Field: None Timestamp: 76.924143s Buffer: 3 Sequence: 0 Field: None Timestamp: 76.947023s Buffer: 0 Sequence: 0 Field: None Timestamp: 76.969903s Buffer: 1 Sequence: 0 Field: None Timestamp: 76.992784s Buffer: 2 Sequence: 0 Field: None Timestamp: 77.015664s Buffer: 3 Sequence: 0 Field: None Timestamp: 77.038544s Buffer: 0 Sequence: 0 Field: None Timestamp: 77.061427s Buffer: 1 Sequence: 0 Field: None Timestamp: 77.084305s Buffer: 2 Sequence: 0 Field: None Timestamp: 77.107185s Buffer: 3 Sequence: 0 Field: None Timestamp: 77.130067s Buffer: 0 Sequence: 0 Field: None Timestamp: 77.152946s Buffer: 1 Sequence: 0 Field: None Timestamp: 77.175827s Buffer: 2 Sequence: 0 Field: None Timestamp: 77.198707s Buffer: 3 Sequence: 0 Field: None Timestamp: 77.221589s Buffer: 0 Sequence: 0 Field: None Timestamp: 77.244468s Buffer: 1 Sequence: 0 Field: None Timestamp: 77.267348s Buffer: 2 Sequence: 0 Field: None Timestamp: 77.290229s Buffer: 3 Sequence: 0 Field: None Timestamp: 77.313109s Buffer: 0 Sequence: 0 Field: None Timestamp: 77.335989s Buffer: 1 Sequence: 0 Field: None Timestamp: 77.358870s Buffer: 2 Sequence: 0 Field: None Timestamp: 77.381750s Buffer: 3 Sequence: 0 Field: None Timestamp: 77.404630s Buffer: 0 Sequence: 0 Field: None Timestamp: 77.427510s Buffer: 1 Sequence: 0 Field: None Timestamp: 77.450391s Buffer: 2 Sequence: 0 Field: None Timestamp: 77.473271s Buffer: 3 Sequence: 0 Field: None Timestamp: 77.496151s Buffer: 0 Sequence: 0 Field: None Timestamp: 77.519032s Buffer: 1 Sequence: 0 Field: None Timestamp: 77.541912s Buffer: 2 Sequence: 0 Field: None Timestamp: 77.564793s Buffer: 3 Sequence: 0 Field: None Timestamp: 77.587673s Buffer: 0 Sequence: 0 Field: None Timestamp: 77.610554s Buffer: 1 Sequence: 0 Field: None Timestamp: 77.633434s Buffer: 2 Sequence: 0 Field: None Timestamp: 77.656314s Buffer: 3 Sequence: 0 Field: None Timestamp: 77.679194s Buffer: 0 Sequence: 0 Field: None Timestamp: 77.702075s Buffer: 1 Sequence: 0 Field: None Timestamp: 77.724955s Buffer: 2 Sequence: 0 Field: None Timestamp: 77.747836s Buffer: 3 Sequence: 0 Field: None Timestamp: 77.770717s Buffer: 0 Sequence: 0 Field: None Timestamp: 77.793596s Buffer: 1 Sequence: 0 Field: None Timestamp: 77.816477s Buffer: 2 Sequence: 0 Field: None Timestamp: 77.839357s Buffer: 3 Sequence: 0 Field: None Timestamp: 77.862237s test MMAP: OK test USERPTR: OK (Not Supported) test DMABUF: Cannot test, specify --expbuf-device Total: 42, Succeeded: 42, Failed: 0, Warnings: 0 > [Snip] >> +/* Print Four-character-code (FOURCC) */ >> +static char *print_fourcc(u32 fmt) >> +{ >> + static char code[5]; >> + >> + code[0] = (unsigned char)(fmt & 0xff); >> + code[1] = (unsigned char)((fmt>>8) & 0xff); >> + code[2] = (unsigned char)((fmt>>16) & 0xff); >> + code[3] = (unsigned char)((fmt>>24) & 0xff); > > I prefer an extra space around '>>' > OK >> + code[4] = '\0'; [Snip] >> + >> + if (ccdcparam->alaw.gamma_wd > VPFE_CCDC_GAMMA_BITS_09_0 || >> + ccdcparam->alaw.gamma_wd < VPFE_CCDC_GAMMA_BITS_15_6 || >> + max_gamma > max_data) { >> + vpfe_dbg(1, dev, "\nInvalid data line select"); > > Newline at the start instead of at the end? > Fixed it. >> + return -EINVAL; >> + } >> + >> + return 0; >> +} >> + >> +static int >> +vpfe_ccdc_update_raw_params(struct vpfe_ccdc *ccdc, >> + struct vpfe_ccdc_config_params_raw *raw_params) >> +{ >> + struct vpfe_ccdc_config_params_raw *config_params = >> + &ccdc->ccdc_cfg.bayer.config_params; >> + >> + memcpy(config_params, raw_params, sizeof(*raw_params)); > > config_params = *raw_params; > >> + >> + return 0; > > Any reason why this can't be a void function? > Fixed it. >> +} >> + >> +/* >> + * vpfe_ccdc_restore_defaults() > > ditto > >> + vpfe_reg_write(ccdc, VPFE_INTERLACED_NO_IMAGE_INVERT, >> + VPFE_SDOFST); >> + } >> + } else if (params->frm_fmt == CCDC_FRMFMT_PROGRESSIVE) { >> + vpfe_reg_write(ccdc, VPFE_PROGRESSIVE_NO_IMAGE_INVERT, >> + VPFE_SDOFST); >> + } >> + >> + vpfe_reg_write(ccdc, syn_mode, VPFE_SYNMODE); >> + >> + vpfe_reg_dump(ccdc); >> +} >> + >> +static inline int >> +vpfe_ccdc_set_buftype(struct vpfe_ccdc *ccdc, >> + enum ccdc_buftype buf_type) >> +{ >> + if (ccdc->ccdc_cfg.if_type == VPFE_RAW_BAYER) >> + ccdc->ccdc_cfg.bayer.buf_type = buf_type; >> + else >> + ccdc->ccdc_cfg.ycbcr.buf_type = buf_type; >> + >> + return 0; >> +} >> + >> +static inline enum ccdc_buftype vpfe_ccdc_get_buftype(struct vpfe_ccdc *ccdc) >> +{ >> + if (ccdc->ccdc_cfg.if_type == VPFE_RAW_BAYER) >> + return ccdc->ccdc_cfg.bayer.buf_type; >> + >> + return ccdc->ccdc_cfg.ycbcr.buf_type; >> +} >> + >> +static int vpfe_ccdc_set_pixel_format(struct vpfe_ccdc *ccdc, u32 pixfmt) >> +{ >> + struct vpfe_device *dev = container_of(ccdc, struct vpfe_device, ccdc); >> + >> + vpfe_dbg(1, dev, "vpfe_ccdc_set_pixel_format: if_type: %d, pixfmt:%s\n", >> + ccdc->ccdc_cfg.if_type, print_fourcc(pixfmt)); >> + >> + if (ccdc->ccdc_cfg.if_type == VPFE_RAW_BAYER) { >> + ccdc->ccdc_cfg.bayer.pix_fmt = CCDC_PIXFMT_RAW; >> + /* >> + * Need to clear it in case it was left on >> + * after the last capture. >> + */ >> + ccdc->ccdc_cfg.bayer.config_params.alaw.enable = 0; >> + >> + switch (pixfmt) { >> + case V4L2_PIX_FMT_SBGGR8: >> + ccdc->ccdc_cfg.bayer.config_params.alaw.enable = 1; >> + break; >> + case V4L2_PIX_FMT_YUYV: >> + case V4L2_PIX_FMT_UYVY: >> + case V4L2_PIX_FMT_YUV420: >> + case V4L2_PIX_FMT_NV12: >> + case V4L2_PIX_FMT_RGB565X: >> + break; /* nothing for now */ >> + case V4L2_PIX_FMT_SBGGR16: >> + default: >> + return -EINVAL; >> + } >> + } else { >> + switch (pixfmt) { >> + case V4L2_PIX_FMT_YUYV: >> + ccdc->ccdc_cfg.ycbcr.pix_order = CCDC_PIXORDER_YCBYCR; >> + break; >> + case V4L2_PIX_FMT_UYVY: >> + ccdc->ccdc_cfg.ycbcr.pix_order = CCDC_PIXORDER_CBYCRY; >> + break; >> + default: >> + return -EINVAL; >> + } >> + } >> + >> + return 0; >> +} >> + >> +static u32 vpfe_ccdc_get_pixel_format(struct vpfe_ccdc *ccdc) >> +{ >> + u32 pixfmt; >> + >> + if (ccdc->ccdc_cfg.if_type == VPFE_RAW_BAYER) { >> + pixfmt = V4L2_PIX_FMT_YUYV; >> + } else { >> + if (ccdc->ccdc_cfg.ycbcr.pix_order == CCDC_PIXORDER_YCBYCR) >> + pixfmt = V4L2_PIX_FMT_YUYV; >> + else >> + pixfmt = V4L2_PIX_FMT_UYVY; >> + } >> + >> + return pixfmt; >> +} >> + >> +static int >> +vpfe_ccdc_set_image_window(struct vpfe_ccdc *ccdc, >> + struct v4l2_rect *win, unsigned int bpp) >> +{ >> + if (ccdc->ccdc_cfg.if_type == VPFE_RAW_BAYER) { >> + ccdc->ccdc_cfg.bayer.win = *win; >> + ccdc->ccdc_cfg.bayer.bytesperpixel = bpp; >> + ccdc->ccdc_cfg.bayer.bytesperline = ALIGN(win->width * bpp, 32); >> + } else { >> + ccdc->ccdc_cfg.ycbcr.win = *win; >> + ccdc->ccdc_cfg.ycbcr.bytesperpixel = bpp; >> + ccdc->ccdc_cfg.ycbcr.bytesperline = ALIGN(win->width * bpp, 32); >> + } >> + >> + return 0; >> +} >> + >> +static inline void >> +vpfe_ccdc_get_image_window(struct vpfe_ccdc *ccdc, >> + struct v4l2_rect *win) >> +{ >> + if (ccdc->ccdc_cfg.if_type == VPFE_RAW_BAYER) >> + *win = ccdc->ccdc_cfg.bayer.win; >> + else >> + *win = ccdc->ccdc_cfg.ycbcr.win; >> +} >> + >> +static inline unsigned int vpfe_ccdc_get_line_length(struct vpfe_ccdc *ccdc) >> +{ >> + if (ccdc->ccdc_cfg.if_type == VPFE_RAW_BAYER) >> + return ccdc->ccdc_cfg.bayer.bytesperline; >> + >> + return ccdc->ccdc_cfg.ycbcr.bytesperline; >> +} >> + >> +static inline int >> +vpfe_ccdc_set_frame_format(struct vpfe_ccdc *ccdc, >> + enum ccdc_frmfmt frm_fmt) >> +{ >> + if (ccdc->ccdc_cfg.if_type == VPFE_RAW_BAYER) >> + ccdc->ccdc_cfg.bayer.frm_fmt = frm_fmt; >> + else >> + ccdc->ccdc_cfg.ycbcr.frm_fmt = frm_fmt; >> + >> + return 0; >> +} >> + >> +static inline enum ccdc_frmfmt >> +vpfe_ccdc_get_frame_format(struct vpfe_ccdc *ccdc) >> +{ >> + if (ccdc->ccdc_cfg.if_type == VPFE_RAW_BAYER) >> + return ccdc->ccdc_cfg.bayer.frm_fmt; >> + >> + return ccdc->ccdc_cfg.ycbcr.frm_fmt; >> +} >> + >> +static inline int vpfe_ccdc_getfid(struct vpfe_ccdc *ccdc) >> +{ >> + return (vpfe_reg_read(ccdc, VPFE_SYNMODE) >> 15) & 1; >> +} >> + >> +static inline void vpfe_set_sdr_addr(struct vpfe_ccdc *ccdc, unsigned long addr) >> +{ >> + vpfe_reg_write(ccdc, addr & 0xffffffe0, VPFE_SDR_ADDR); >> +} >> + >> +static int vpfe_ccdc_set_hw_if_params(struct vpfe_ccdc *ccdc, >> + struct vpfe_hw_if_param *params) >> +{ >> + struct vpfe_device *vpfe = container_of(ccdc, struct vpfe_device, ccdc); >> + >> + ccdc->ccdc_cfg.if_type = params->if_type; >> + >> + switch (params->if_type) { >> + case VPFE_BT656: >> + case VPFE_YCBCR_SYNC_16: >> + case VPFE_YCBCR_SYNC_8: >> + case VPFE_BT656_10BIT: >> + ccdc->ccdc_cfg.ycbcr.vd_pol = params->vdpol; >> + ccdc->ccdc_cfg.ycbcr.hd_pol = params->hdpol; >> + break; >> + case VPFE_RAW_BAYER: >> + ccdc->ccdc_cfg.bayer.vd_pol = params->vdpol; >> + ccdc->ccdc_cfg.bayer.hd_pol = params->hdpol; >> + if (params->bus_width == 10) >> + ccdc->ccdc_cfg.bayer.config_params.data_sz = >> + VPFE_CCDC_DATA_10BITS; >> + else >> + ccdc->ccdc_cfg.bayer.config_params.data_sz = >> + VPFE_CCDC_DATA_8BITS; >> + vpfe_dbg(1, vpfe, "params.bus_width: %d\n", >> + params->bus_width); >> + vpfe_dbg(1, vpfe, "config_params.data_sz: %d\n", >> + ccdc->ccdc_cfg.bayer.config_params.data_sz); >> + break; >> + >> + default: >> + return -EINVAL; >> + } >> + >> + return 0; >> +} >> + >> +static void vpfe_clear_intr(struct vpfe_ccdc *ccdc, int vdint) >> +{ >> + unsigned int vpfe_int_status; >> + >> + vpfe_int_status = vpfe_reg_read(ccdc, VPFE_IRQ_STS); >> + >> + switch (vdint) { >> + /* VD0 interrupt */ >> + case VPFE_VDINT0: >> + vpfe_int_status &= ~VPFE_VDINT0; >> + vpfe_int_status |= VPFE_VDINT0; >> + break; >> + /* VD1 interrupt */ >> + case VPFE_VDINT1: >> + vpfe_int_status &= ~VPFE_VDINT1; >> + vpfe_int_status |= VPFE_VDINT1; >> + break; >> + /* VD2 interrupt */ >> + case VPFE_VDINT2: >> + vpfe_int_status &= ~VPFE_VDINT2; >> + vpfe_int_status |= VPFE_VDINT2; >> + break; >> + /* Clear all interrupts */ >> + default: >> + vpfe_int_status &= ~(VPFE_VDINT0 | >> + VPFE_VDINT1 | >> + VPFE_VDINT2); >> + vpfe_int_status |= (VPFE_VDINT0 | >> + VPFE_VDINT1 | >> + VPFE_VDINT2); >> + break; >> + } >> + /* Clear specific VDINT from the status register */ >> + vpfe_reg_write(ccdc, vpfe_int_status, VPFE_IRQ_STS); >> + >> + vpfe_int_status = vpfe_reg_read(ccdc, VPFE_IRQ_STS); >> + >> + /* Acknowledge that we are done with all interrupts */ >> + vpfe_reg_write(ccdc, 1, VPFE_IRQ_EOI); >> +} >> + >> +static void vpfe_ccdc_config_defaults(struct vpfe_ccdc *ccdc) >> +{ >> + ccdc->ccdc_cfg.if_type = VPFE_RAW_BAYER; >> + >> + ccdc->ccdc_cfg.ycbcr.pix_fmt = CCDC_PIXFMT_YCBCR_8BIT; >> + ccdc->ccdc_cfg.ycbcr.frm_fmt = CCDC_FRMFMT_INTERLACED; >> + ccdc->ccdc_cfg.ycbcr.fid_pol = VPFE_PINPOL_POSITIVE; >> + ccdc->ccdc_cfg.ycbcr.vd_pol = VPFE_PINPOL_POSITIVE; >> + ccdc->ccdc_cfg.ycbcr.hd_pol = VPFE_PINPOL_POSITIVE; >> + ccdc->ccdc_cfg.ycbcr.pix_order = CCDC_PIXORDER_CBYCRY; >> + ccdc->ccdc_cfg.ycbcr.buf_type = CCDC_BUFTYPE_FLD_INTERLEAVED; >> + >> + ccdc->ccdc_cfg.ycbcr.win.left = 0; >> + ccdc->ccdc_cfg.ycbcr.win.top = 0; >> + ccdc->ccdc_cfg.ycbcr.win.width = 720; >> + ccdc->ccdc_cfg.ycbcr.win.height = 576; >> + ccdc->ccdc_cfg.ycbcr.bt656_enable = 1; >> + >> + ccdc->ccdc_cfg.bayer.pix_fmt = CCDC_PIXFMT_RAW; >> + ccdc->ccdc_cfg.bayer.frm_fmt = CCDC_FRMFMT_PROGRESSIVE; >> + ccdc->ccdc_cfg.bayer.fid_pol = VPFE_PINPOL_POSITIVE; >> + ccdc->ccdc_cfg.bayer.vd_pol = VPFE_PINPOL_POSITIVE; >> + ccdc->ccdc_cfg.bayer.hd_pol = VPFE_PINPOL_POSITIVE; >> + >> + ccdc->ccdc_cfg.bayer.win.left = 0; >> + ccdc->ccdc_cfg.bayer.win.top = 0; >> + ccdc->ccdc_cfg.bayer.win.width = 800; >> + ccdc->ccdc_cfg.bayer.win.height = 600; >> + ccdc->ccdc_cfg.bayer.config_params.data_sz = VPFE_CCDC_DATA_8BITS; >> + ccdc->ccdc_cfg.bayer.config_params.alaw.gamma_wd = >> + VPFE_CCDC_GAMMA_BITS_09_0; >> +} >> + >> +/* >> + * vpfe_get_ccdc_image_format - Get image parameters based on CCDC settings >> + */ >> +static int vpfe_get_ccdc_image_format(struct vpfe_device *dev, >> + struct v4l2_format *f) >> +{ >> + struct v4l2_rect image_win; >> + enum ccdc_buftype buf_type; >> + enum ccdc_frmfmt frm_fmt; >> + >> + memset(f, 0, sizeof(*f)); >> + f->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; >> + vpfe_ccdc_get_image_window(&dev->ccdc, &image_win); >> + f->fmt.pix.width = image_win.width; >> + f->fmt.pix.height = image_win.height; >> + f->fmt.pix.bytesperline = vpfe_ccdc_get_line_length(&dev->ccdc); >> + f->fmt.pix.sizeimage = f->fmt.pix.bytesperline * >> + f->fmt.pix.height; >> + buf_type = vpfe_ccdc_get_buftype(&dev->ccdc); >> + f->fmt.pix.pixelformat = vpfe_ccdc_get_pixel_format(&dev->ccdc); >> + frm_fmt = vpfe_ccdc_get_frame_format(&dev->ccdc); >> + if (frm_fmt == CCDC_FRMFMT_PROGRESSIVE) { >> + f->fmt.pix.field = V4L2_FIELD_NONE; >> + } else if (frm_fmt == CCDC_FRMFMT_INTERLACED) { >> + if (buf_type == CCDC_BUFTYPE_FLD_INTERLEAVED) { >> + f->fmt.pix.field = V4L2_FIELD_INTERLACED; >> + } else if (buf_type == CCDC_BUFTYPE_FLD_SEPARATED) { >> + f->fmt.pix.field = V4L2_FIELD_SEQ_TB; >> + } else { >> + vpfe_err(dev, "Invalid buf_type\n"); >> + return -EINVAL; >> + } >> + } else { >> + vpfe_err(dev, "Invalid frm_fmt\n"); >> + return -EINVAL; >> + } >> + return 0; >> +} >> + >> +static int vpfe_config_ccdc_image_format(struct vpfe_device *dev) >> +{ >> + enum ccdc_frmfmt frm_fmt = CCDC_FRMFMT_INTERLACED; >> + int ret = 0; >> + >> + vpfe_dbg(2, dev, "vpfe_config_ccdc_image_format\n"); >> + >> + vpfe_dbg(1, dev, "pixelformat: %s\n", >> + print_fourcc(dev->fmt.fmt.pix.pixelformat)); >> + >> + if (vpfe_ccdc_set_pixel_format( >> + &dev->ccdc, >> + dev->fmt.fmt.pix.pixelformat) < 0) { >> + vpfe_err(dev, >> + "couldn't set pix format in ccdc\n"); >> + return -EINVAL; >> + } >> + /* configure the image window */ >> + vpfe_ccdc_set_image_window(&dev->ccdc, &dev->crop, dev->bpp); >> + >> + switch (dev->fmt.fmt.pix.field) { >> + case V4L2_FIELD_INTERLACED: >> + /* do nothing, since it is default */ >> + ret = vpfe_ccdc_set_buftype( >> + &dev->ccdc, >> + CCDC_BUFTYPE_FLD_INTERLEAVED); >> + break; >> + case V4L2_FIELD_NONE: >> + frm_fmt = CCDC_FRMFMT_PROGRESSIVE; >> + /* buffer type only applicable for interlaced scan */ >> + break; >> + case V4L2_FIELD_SEQ_TB: >> + ret = vpfe_ccdc_set_buftype( >> + &dev->ccdc, >> + CCDC_BUFTYPE_FLD_SEPARATED); >> + break; >> + default: >> + return -EINVAL; >> + } >> + >> + /* set the frame format */ >> + if (!ret) >> + ret = vpfe_ccdc_set_frame_format(&dev->ccdc, frm_fmt); >> + return ret; >> +} >> + >> +/* >> + * vpfe_config_image_format() >> + * For a given standard, this functions sets up the default >> + * pix format & crop values in the vpfe device and ccdc. It first >> + * starts with defaults based values from the standard table. >> + * It then checks if sub device support g_mbus_fmt and then override the >> + * values based on that.Sets crop values to match with scan resolution >> + * starting at 0,0. It calls vpfe_config_ccdc_image_format() set the >> + * values in ccdc >> + */ >> +static int vpfe_config_image_format(struct vpfe_device *dev, >> + v4l2_std_id std_id) >> +{ >> + struct v4l2_pix_format *pix = &dev->fmt.fmt.pix; >> + int i, ret; >> + >> + for (i = 0; i < ARRAY_SIZE(vpfe_standards); i++) { >> + if (vpfe_standards[i].std_id & std_id) { >> + dev->std_info.active_pixels = >> + vpfe_standards[i].width; >> + dev->std_info.active_lines = >> + vpfe_standards[i].height; >> + dev->std_info.frame_format = >> + vpfe_standards[i].frame_format; >> + dev->std_index = i; >> + break; >> + } >> + } >> + >> + if (i == ARRAY_SIZE(vpfe_standards)) { >> + vpfe_err(dev, "standard not supported\n"); >> + return -EINVAL; >> + } >> + >> + dev->crop.top = 0; >> + dev->crop.left = 0; >> + dev->crop.width = dev->std_info.active_pixels; >> + dev->crop.height = dev->std_info.active_lines; >> + pix->width = dev->crop.width; >> + pix->height = dev->crop.height; >> + pix->pixelformat = V4L2_PIX_FMT_YUYV; >> + >> + /* first field and frame format based on standard frame format */ >> + if (dev->std_info.frame_format) >> + pix->field = V4L2_FIELD_INTERLACED; >> + else >> + pix->field = V4L2_FIELD_NONE; >> + >> + ret = __vpfe_get_format(dev, &dev->fmt, &dev->bpp); >> + if (ret) >> + return ret; >> + >> + /* Update the crop window based on found values */ >> + dev->crop.width = pix->width; >> + dev->crop.height = pix->height; >> + >> + return vpfe_config_ccdc_image_format(dev); >> +} >> + >> +static int vpfe_initialize_device(struct vpfe_device *vpfe) >> +{ >> + struct vpfe_subdev_info *sdinfo; >> + int ret; >> + >> + sdinfo = &vpfe->cfg->sub_devs[0]; >> + sdinfo->sd = vpfe->sd[0]; >> + vpfe->current_input = 0; >> + vpfe->std_index = 0; >> + /* Configure the default format information */ >> + ret = vpfe_config_image_format(vpfe, >> + vpfe_standards[vpfe->std_index].std_id); >> + if (ret) >> + return ret; >> + >> + pm_runtime_get_sync(vpfe->pdev); >> + >> + vpfe_config_enable(&vpfe->ccdc, 1); >> + >> + vpfe_ccdc_restore_defaults(&vpfe->ccdc); >> + >> + /* Clear all VPFE interrupts */ >> + vpfe_clear_intr(&vpfe->ccdc, -1); >> + >> + return ret; >> +} >> + >> +/* >> + * vpfe_release : This function is based on the vb2_fop_release >> + * helper function. >> + * It has been augmented to handle module power management, >> + * by disabling/enabling h/w module fcntl clock when necessary. >> + */ >> +static int vpfe_release(struct file *file) >> +{ >> + struct vpfe_device *dev = video_drvdata(file); >> + int ret; >> + >> + vpfe_dbg(2, dev, "vpfe_release\n"); >> + >> + ret = _vb2_fop_release(file, NULL); >> + >> + if (v4l2_fh_is_singular_file(file)) { >> + mutex_lock(&dev->lock); >> + vpfe_ccdc_close(&dev->ccdc, dev->pdev); >> + mutex_unlock(&dev->lock); >> + } >> + >> + return ret; >> +} >> + >> +/* >> + * vpfe_open : This function is based on the v4l2_fh_open helper function. >> + * It has been augmented to handle module power management, >> + * by disabling/enabling h/w module fcntl clock when necessary. >> + */ >> + >> +static int vpfe_open(struct file *file) >> +{ >> + struct vpfe_device *vpfe = video_drvdata(file); >> + >> + v4l2_fh_open(file); > > Check error code! > >> + >> + if (!v4l2_fh_is_singular_file(file)) >> + return 0; >> + >> + mutex_lock(&vpfe->lock); >> + if (vpfe_initialize_device(vpfe)) { >> + mutex_unlock(&vpfe->lock); > > Call v4l2_fh_release(), otherwise you have a memory leak. > >> + return -ENODEV; >> + } >> + mutex_unlock(&vpfe->lock); >> + >> + return 0; >> +} >> + >> +/** >> + * vpfe_schedule_next_buffer: set next buffer address for capture >> + * @vpfe : ptr to vpfe device >> + * >> + * This function will get next buffer from the dma queue and >> + * set the buffer address in the vpfe register for capture. >> + * the buffer is marked active >> + * >> + * Assumes caller is holding vpfe->dma_queue_lock already >> + */ >> +static inline void vpfe_schedule_next_buffer(struct vpfe_device *vpfe) >> +{ >> + vpfe->next_frm = list_entry(vpfe->dma_queue.next, >> + struct vpfe_cap_buffer, list); >> + list_del(&vpfe->next_frm->list); >> + >> + vpfe_set_sdr_addr(&vpfe->ccdc, >> + vb2_dma_contig_plane_dma_addr(&vpfe->next_frm->vb, 0)); >> +} >> + >> +static inline void vpfe_schedule_bottom_field(struct vpfe_device *vpfe) >> +{ >> + unsigned long addr; >> + >> + addr = vb2_dma_contig_plane_dma_addr(&vpfe->next_frm->vb, 0) + >> + vpfe->field_off; >> + >> + vpfe_set_sdr_addr(&vpfe->ccdc, addr); >> +} >> + >> +/* >> + * vpfe_process_buffer_complete: process a completed buffer >> + * @vpfe : ptr to vpfe device >> + * >> + * This function time stamp the buffer and mark it as DONE. It also >> + * wake up any process waiting on the QUEUE and set the next buffer >> + * as current >> + */ >> +static inline void vpfe_process_buffer_complete(struct vpfe_device *vpfe) >> +{ >> + v4l2_get_timestamp(&vpfe->cur_frm->vb.v4l2_buf.timestamp); >> + vb2_buffer_done(&vpfe->cur_frm->vb, VB2_BUF_STATE_DONE); >> + vpfe->cur_frm = vpfe->next_frm; >> +} >> + >> +/* >> + * vpfe_isr : ISR handler for vpfe capture (VINT0) >> + * @irq: irq number >> + * @dev_id: dev_id ptr >> + * >> + * It changes status of the captured buffer, takes next buffer from the queue >> + * and sets its address in VPFE registers >> + */ >> +static irqreturn_t vpfe_isr(int irq, void *dev) >> +{ >> + struct vpfe_device *vpfe = (struct vpfe_device *)dev; >> + enum v4l2_field field; >> + int intr_status; >> + int fid; >> + >> + intr_status = vpfe_reg_read(&vpfe->ccdc, VPFE_IRQ_STS); >> + >> + if (intr_status & VPFE_VDINT0) { >> + field = vpfe->fmt.fmt.pix.field; >> + >> + if (field == V4L2_FIELD_NONE) { >> + /* handle progressive frame capture */ >> + if (vpfe->cur_frm != vpfe->next_frm) >> + vpfe_process_buffer_complete(vpfe); >> + goto next_intr; >> + } >> + >> + /* interlaced or TB capture check which field >> + we are in hardware */ >> + fid = vpfe_ccdc_getfid(&vpfe->ccdc); >> + >> + /* switch the software maintained field id */ >> + vpfe->field_id ^= 1; >> + if (fid == vpfe->field_id) { >> + /* we are in-sync here,continue */ >> + if (fid == 0) { >> + /* >> + * One frame is just being captured. If the >> + * next frame is available, release the >> + * current frame and move on >> + */ >> + if (vpfe->cur_frm != vpfe->next_frm) >> + vpfe_process_buffer_complete(vpfe); >> + /* >> + * based on whether the two fields are stored >> + * interleavely or separately in memory, >> + * reconfigure the CCDC memory address >> + */ >> + if (field == V4L2_FIELD_SEQ_TB) >> + vpfe_schedule_bottom_field(vpfe); >> + >> + goto next_intr; >> + } >> + /* >> + * if one field is just being captured configure >> + * the next frame get the next frame from the empty >> + * queue if no frame is available hold on to the >> + * current buffer >> + */ >> + spin_lock(&vpfe->dma_queue_lock); >> + if (!list_empty(&vpfe->dma_queue) && >> + vpfe->cur_frm == vpfe->next_frm) >> + vpfe_schedule_next_buffer(vpfe); >> + spin_unlock(&vpfe->dma_queue_lock); >> + } else if (fid == 0) { >> + /* >> + * out of sync. Recover from any hardware out-of-sync. >> + * May loose one frame >> + */ >> + vpfe->field_id = fid; >> + } >> + } >> + >> +next_intr: >> + if (intr_status & VPFE_VDINT1) { >> + spin_lock(&vpfe->dma_queue_lock); >> + if (vpfe->fmt.fmt.pix.field == V4L2_FIELD_NONE && >> + !list_empty(&vpfe->dma_queue) && >> + vpfe->cur_frm == vpfe->next_frm) >> + vpfe_schedule_next_buffer(vpfe); >> + spin_unlock(&vpfe->dma_queue_lock); >> + } >> + >> + vpfe_clear_intr(&vpfe->ccdc, intr_status); >> + >> + return IRQ_HANDLED; >> +} >> + >> +static inline void vpfe_detach_irq(struct vpfe_device *vpfe) >> +{ >> + unsigned int intr = VPFE_VDINT0; >> + enum ccdc_frmfmt frame_format; >> + >> + frame_format = vpfe_ccdc_get_frame_format(&vpfe->ccdc); >> + if (frame_format == CCDC_FRMFMT_PROGRESSIVE) >> + intr |= VPFE_VDINT1; >> + >> + vpfe_reg_write(&vpfe->ccdc, intr, VPFE_IRQ_EN_CLR); >> +} >> + >> +static inline void vpfe_attach_irq(struct vpfe_device *dev) >> +{ >> + unsigned int intr = VPFE_VDINT0; >> + enum ccdc_frmfmt frame_format; >> + >> + frame_format = vpfe_ccdc_get_frame_format(&dev->ccdc); >> + if (frame_format == CCDC_FRMFMT_PROGRESSIVE) >> + intr |= VPFE_VDINT1; >> + >> + vpfe_reg_write(&dev->ccdc, intr, VPFE_IRQ_EN_SET); >> +} >> + >> +static int vpfe_querycap(struct file *file, void *priv, >> + struct v4l2_capability *cap) >> +{ >> + struct vpfe_device *dev = video_drvdata(file); >> + >> + vpfe_dbg(2, dev, "vpfe_querycap\n"); >> + >> + strlcpy(cap->driver, VPFE_MODULE_NAME, sizeof(cap->driver)); >> + strlcpy(cap->card, "TI AM437x VPFE", sizeof(cap->card)); >> + snprintf(cap->bus_info, sizeof(cap->bus_info), >> + "platform:%s", dev->v4l2_dev.name); >> + cap->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING | >> + V4L2_CAP_READWRITE; >> + cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS; >> + >> + return 0; >> +} >> + >> +/* get the format set at output pad of the adjacent subdev */ >> +static int __vpfe_get_format(struct vpfe_device *vpfe, >> + struct v4l2_format *format, unsigned int *bpp) >> +{ >> + struct v4l2_mbus_framefmt mbus_fmt; >> + struct vpfe_subdev_info *sdinfo; >> + struct v4l2_subdev_format fmt; >> + int ret; >> + >> + sdinfo = vpfe->current_subdev; >> + if (!sdinfo->sd) >> + return -EINVAL; >> + >> + fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE; >> + fmt.pad = 0; >> + >> + ret = v4l2_subdev_call(sdinfo->sd, pad, get_fmt, NULL, &fmt); >> + if (ret && ret != -ENOIOCTLCMD && ret != -ENODEV) >> + return ret; >> + >> + if (!ret) { >> + v4l2_fill_pix_format(&format->fmt.pix, &fmt.format); >> + mbus_to_pix(vpfe, &fmt.format, &format->fmt.pix, bpp); >> + } else { >> + ret = v4l2_device_call_until_err(&vpfe->v4l2_dev, >> + sdinfo->grp_id, >> + video, g_mbus_fmt, >> + &mbus_fmt); >> + if (ret && ret != -ENOIOCTLCMD && ret != -ENODEV) >> + return ret; >> + v4l2_fill_pix_format(&format->fmt.pix, &mbus_fmt); >> + mbus_to_pix(vpfe, &mbus_fmt, &format->fmt.pix, bpp); >> + } >> + >> + format->type = vpfe->fmt.type; >> + >> + vpfe_dbg(1, vpfe, "__vpfe_get_format size %dx%d (%s) bytesperline = %d, sizeimage = %d, bpp = %d\n", >> + format->fmt.pix.width, format->fmt.pix.height, >> + print_fourcc(format->fmt.pix.pixelformat), >> + format->fmt.pix.bytesperline, format->fmt.pix.sizeimage, *bpp); >> + >> + return 0; >> +} >> + >> +/* set the format at output pad of the adjacent subdev */ >> +static int __vpfe_set_format(struct vpfe_device *vpfe, >> + struct v4l2_format *format, unsigned int *bpp) >> +{ >> + struct vpfe_subdev_info *sdinfo; >> + struct v4l2_subdev_format fmt; >> + int ret; >> + >> + vpfe_dbg(2, vpfe, "__vpfe_set_format\n"); >> + >> + sdinfo = vpfe->current_subdev; >> + if (!sdinfo->sd) >> + return -EINVAL; >> + >> + fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE; >> + fmt.pad = 0; >> + >> + ret = pix_to_mbus(vpfe, &format->fmt.pix, &fmt.format); >> + if (ret) >> + return ret; >> + >> + ret = v4l2_subdev_call(sdinfo->sd, pad, set_fmt, NULL, &fmt); >> + if (ret == -ENOIOCTLCMD) >> + return -EINVAL; >> + >> + format->type = vpfe->fmt.type; >> + >> + /* convert mbus_format to v4l2_format */ >> + v4l2_fill_pix_format(&format->fmt.pix, &fmt.format); >> + mbus_to_pix(vpfe, &fmt.format, &format->fmt.pix, bpp); >> + vpfe_dbg(1, vpfe, "__vpfe_set_format size %dx%d (%s) bytesperline = %d, sizeimage = %d, bpp = %d\n", >> + format->fmt.pix.width, format->fmt.pix.height, >> + print_fourcc(format->fmt.pix.pixelformat), >> + format->fmt.pix.bytesperline, format->fmt.pix.sizeimage, *bpp); >> + >> + return 0; >> +} >> + >> +static int vpfe_g_fmt(struct file *file, void *priv, >> + struct v4l2_format *fmt) >> +{ >> + struct vpfe_device *dev = video_drvdata(file); >> + struct v4l2_format format; >> + unsigned int bpp; >> + int ret = 0; >> + >> + vpfe_dbg(2, dev, "vpfe_g_fmt\n"); >> + >> + ret = __vpfe_get_format(dev, &format, &bpp); >> + if (ret) >> + return ret; > > Why do this? > >> + >> + /* Fill in the information about format */ >> + *fmt = dev->fmt; > > If you are always going to fill in the info from dev->fmt? > >> + >> + return ret; >> +} >> + >> +static int vpfe_enum_fmt(struct file *file, void *priv, >> + struct v4l2_fmtdesc *f) >> +{ >> + struct vpfe_device *vpfe = video_drvdata(file); >> + struct v4l2_subdev_mbus_code_enum mbus_code; >> + struct vpfe_subdev_info *sdinfo; >> + struct vpfe_fmt *fmt; >> + int ret; >> + >> + vpfe_dbg(2, vpfe, "vpfe_enum_format index:%d\n", >> + f->index); >> + >> + sdinfo = vpfe->current_subdev; >> + if (!sdinfo->sd) >> + return -EINVAL; >> + >> + mbus_code.index = f->index; >> + ret = v4l2_subdev_call(sdinfo->sd, pad, enum_mbus_code, >> + NULL, &mbus_code); >> + if (ret) >> + return -EINVAL; >> + >> + /* convert mbus_format to v4l2_format */ >> + fmt = find_format_by_code(mbus_code.code); >> + if (!fmt) { >> + vpfe_err(vpfe, "mbus code 0x%08x not found\n", >> + mbus_code.code); >> + return -EINVAL; >> + } > > Hmm. If a subdev supports more media bus codes then are supported by this > driver, then the enumeration will stop as soon as such an unsupported code > is found. > > What you want to do here is enumerate over the pixelformats that are supported > by both this driver and the subdev. It is probably something you want to > determine at the time the subdev is loaded and just mark unsupported formats > at that time so that they can be skipped here. > So probably populate the supported pixelformats in s_input call , by calling enm_mbus_code ? >> + >> + strncpy(f->description, fmt->name, sizeof(f->description) - 1); >> + f->pixelformat = fmt->fourcc; >> + f->type = vpfe->fmt.type; >> + >> + vpfe_dbg(1, vpfe, "vpfe_enum_format: mbus index: %d code: %x pixelformat: %s [%s]\n", >> + f->index, fmt->code, print_fourcc(fmt->fourcc), fmt->name); >> + >> + return 0; >> +} >> + >> +static int vpfe_s_fmt(struct file *file, void *priv, >> + struct v4l2_format *fmt) >> +{ >> + struct vpfe_device *dev = video_drvdata(file); >> + struct vb2_queue *q = &dev->buffer_queue; >> + struct v4l2_format format; >> + unsigned int bpp; >> + int ret = 0; >> + >> + vpfe_dbg(2, dev, "vpfe_s_fmt\n"); >> + >> + /* Check for valid frame format */ >> + ret = __vpfe_get_format(dev, &format, &bpp); >> + if (ret) >> + return ret; > > Usually s_fmt calls try_fmt first. I recommend doing that here as well. > OK >> + >> + /* If streaming is started, return error */ >> + if (vb2_is_busy(q)) { >> + vpfe_err(dev, "%s device busy\n", __func__); >> + return -EBUSY; >> + } >> + >> + if (!cmp_v4l2_format(fmt, &format)) { >> + /* Sensor format is different from the requested format >> + * so we need to change it >> + */ >> + ret = __vpfe_set_format(dev, fmt, &bpp); >> + if (ret) >> + return ret; >> + } else /* Just make sure all of the fields are consistent */ >> + *fmt = format; >> + >> + fmt->fmt.pix.priv = 0; > > No longer needed, remove it. > OK >> + >> + /* First detach any IRQ if currently attached */ >> + vpfe_detach_irq(dev); >> + dev->fmt = *fmt; >> + dev->bpp = bpp; >> + >> + /* Update the crop window based on found values */ >> + dev->crop.width = fmt->fmt.pix.width; >> + dev->crop.height = fmt->fmt.pix.height; >> + >> + /* set image capture parameters in the ccdc */ >> + return vpfe_config_ccdc_image_format(dev); >> +} >> + >> +static int vpfe_try_fmt(struct file *file, void *priv, >> + struct v4l2_format *fmt) >> +{ >> + struct vpfe_device *dev = video_drvdata(file); >> + struct v4l2_format format; >> + unsigned int bpp; >> + int ret = 0; >> + >> + vpfe_dbg(2, dev, "vpfe_try_fmt\n"); >> + >> + memset(&format, 0x00, sizeof(format)); >> + ret = __vpfe_get_format(dev, &format, &bpp); > > Why not use fmt as __vpfe_get_format argument? That way there is no > need for the local format variable. > > Also, this works fine for a sensor which has a fixed format, but what > about a video receiver? You might want to switch the format from e.g. > PAL to NTSC resolution, but this code will always return the current > format. > Currently the driver is intended to support sensor only. >> + if (ret) >> + return ret; >> + >> + *fmt = format; >> + fmt->fmt.pix.priv = 0; > > Remove this. > Ok >> + >> + return 0; >> +} >> + >> +static int vpfe_enum_size(struct file *file, void *priv, >> + struct v4l2_frmsizeenum *fsize) >> +{ >> + struct vpfe_device *vpfe = video_drvdata(file); >> + struct v4l2_subdev_frame_size_enum fse; >> + struct vpfe_subdev_info *sdinfo; >> + struct v4l2_mbus_framefmt mbus; >> + struct v4l2_pix_format pix; >> + struct vpfe_fmt *fmt; >> + int ret; >> + >> + vpfe_dbg(2, vpfe, "vpfe_enum_size\n"); >> + >> + /* check for valid format */ >> + fmt = find_format_by_pix(fsize->pixel_format); >> + if (!fmt) { >> + vpfe_dbg(3, vpfe, "Invalid pixel code: %x, default used instead\n", >> + fsize->pixel_format); >> + return -EINVAL; >> + } >> + >> + memset(fsize->reserved, 0x00, sizeof(fsize->reserved)); >> + >> + sdinfo = vpfe->current_subdev; >> + if (!sdinfo->sd) >> + return -EINVAL; >> + >> + /* Construct pix from parameter and use default for the rest */ >> + pix.pixelformat = fsize->pixel_format; >> + pix.width = 640; >> + pix.height = 480; >> + pix.colorspace = V4L2_COLORSPACE_JPEG; > > I would probably choose COLORSPACE_SRGB here, as that's most common for sensors. > OK >> + [snip] >> + if (sdinfo->inputs[0].capabilities != V4L2_IN_CAP_STD) > > Use '&'. Please check the code if this is used elsewhere as well. > >> + return -ENODATA; > > Do this check before the vb2_is_busy check. > OK >> + >> + ret = v4l2_device_call_until_err(&vpfe->v4l2_dev, sdinfo->grp_id, >> + video, s_std, std_id); >> + if (ret < 0) { >> + vpfe_err(vpfe, "Failed to set standard\n"); >> + return ret; >> + } >> + ret = vpfe_config_image_format(vpfe, std_id); >> + >> + return ret; >> +} >> + >> +static int vpfe_g_std(struct file *file, void *priv, v4l2_std_id *std_id) >> +{ >> + struct vpfe_device *vpfe = video_drvdata(file); >> + struct vpfe_subdev_info *sdinfo; >> + >> + vpfe_dbg(2, vpfe, "vpfe_g_std\n"); >> + >> + sdinfo = vpfe->current_subdev; >> + if (sdinfo->inputs[0].capabilities != V4L2_IN_CAP_STD) >> + return -ENODATA; >> + >> + *std_id = vpfe_standards[vpfe->std_index].std_id; >> + >> + return 0; >> +} >> + >> +/* >> + * vpfe_calculate_offsets : This function calculates buffers offset >> + * for top and bottom field >> + */ >> +static void vpfe_calculate_offsets(struct vpfe_device *vpfe) >> +{ >> + struct v4l2_rect image_win; >> + >> + vpfe_dbg(2, vpfe, "vpfe_calculate_offsets\n"); >> + >> + vpfe_ccdc_get_image_window(&vpfe->ccdc, &image_win); >> + vpfe->field_off = image_win.height * image_win.width; >> +} >> + >> +/* >> + * vpfe_queue_setup - Callback function for buffer setup. >> + * @vq: vb2_queue ptr >> + * @fmt: v4l2 format >> + * @nbuffers: ptr to number of buffers requested by application >> + * @nplanes:: contains number of distinct video planes needed to hold a frame >> + * @sizes[]: contains the size (in bytes) of each plane. >> + * @alloc_ctxs: ptr to allocation context >> + * >> + * This callback function is called when reqbuf() is called to adjust >> + * the buffer count and buffer size >> + */ >> +static int vpfe_queue_setup(struct vb2_queue *vq, >> + const struct v4l2_format *fmt, >> + unsigned int *nbuffers, unsigned int *nplanes, >> + unsigned int sizes[], void *alloc_ctxs[]) >> +{ >> + struct vpfe_device *vpfe = vb2_get_drv_priv(vq); >> + >> + if (fmt && fmt->fmt.pix.sizeimage < vpfe->fmt.fmt.pix.sizeimage) >> + return -EINVAL; >> + >> + if (vq->num_buffers + *nbuffers < 3) >> + *nbuffers = 3 - vq->num_buffers; >> + >> + *nplanes = 1; >> + sizes[0] = fmt ? fmt->fmt.pix.sizeimage : vpfe->fmt.fmt.pix.sizeimage; >> + alloc_ctxs[0] = vpfe->alloc_ctx; >> + >> + vpfe_dbg(1, vpfe, >> + "nbuffers=%d, size=%u\n", *nbuffers, sizes[0]); >> + >> + /* Calculate field offset */ >> + vpfe_calculate_offsets(vpfe); >> + >> + return 0; >> +} >> + >> +/* >> + * vpfe_buffer_prepare : callback function for buffer prepare >> + * @vb: ptr to vb2_buffer >> + * >> + * This is the callback function for buffer prepare when vb2_qbuf() >> + * function is called. The buffer is prepared and user space virtual address >> + * or user address is converted into physical address >> + */ >> +static int vpfe_buffer_prepare(struct vb2_buffer *vb) >> +{ >> + struct vpfe_device *vpfe = vb2_get_drv_priv(vb->vb2_queue); >> + >> + vb2_set_plane_payload(vb, 0, vpfe->fmt.fmt.pix.sizeimage); >> + >> + if (vb2_get_plane_payload(vb, 0) > vb2_plane_size(vb, 0)) >> + return -EINVAL; >> + >> + vb->v4l2_buf.field = vpfe->fmt.fmt.pix.field; >> + >> + return 0; >> +} >> + >> +/* >> + * vpfe_buffer_queue : Callback function to add buffer to DMA queue >> + * @vb: ptr to vb2_buffer >> + */ >> +static void vpfe_buffer_queue(struct vb2_buffer *vb) >> +{ >> + struct vpfe_device *vpfe = vb2_get_drv_priv(vb->vb2_queue); >> + struct vpfe_cap_buffer *buf = to_vpfe_buffer(vb); >> + unsigned long flags = 0; >> + >> + /* add the buffer to the DMA queue */ >> + spin_lock_irqsave(&vpfe->dma_queue_lock, flags); >> + list_add_tail(&buf->list, &vpfe->dma_queue); >> + spin_unlock_irqrestore(&vpfe->dma_queue_lock, flags); >> +} >> + >> +/* >> + * vpfe_start_streaming : Starts the DMA engine for streaming >> + * @vb: ptr to vb2_buffer >> + * @count: number of buffers >> + */ >> +static int vpfe_start_streaming(struct vb2_queue *vq, unsigned int count) >> +{ >> + struct vpfe_device *vpfe = vb2_get_drv_priv(vq); >> + struct vpfe_cap_buffer *buf, *tmp; >> + struct vpfe_subdev_info *sdinfo; >> + unsigned long addr = 0; >> + unsigned long flags; >> + int ret = 0; >> + >> + spin_lock_irqsave(&vpfe->dma_queue_lock, flags); >> + >> + vpfe->field_id = 0; >> + >> + sdinfo = vpfe->current_subdev; >> + ret = v4l2_subdev_call(sdinfo->sd, video, s_stream, 1); >> + if (ret < 0) { >> + vpfe_err(vpfe, "Error in attaching interrupt handle\n"); >> + goto err; >> + } >> + >> + vpfe_attach_irq(vpfe); >> + >> + if (vpfe->ccdc.ccdc_cfg.if_type == VPFE_RAW_BAYER) >> + vpfe_ccdc_config_raw(&vpfe->ccdc); >> + else >> + vpfe_ccdc_config_ycbcr(&vpfe->ccdc); >> + >> + /* Get the next frame from the buffer queue */ >> + vpfe->next_frm = list_entry(vpfe->dma_queue.next, >> + struct vpfe_cap_buffer, list); >> + vpfe->cur_frm = vpfe->next_frm; >> + /* Remove buffer from the buffer queue */ >> + list_del(&vpfe->cur_frm->list); >> + spin_unlock_irqrestore(&vpfe->dma_queue_lock, flags); >> + >> + addr = vb2_dma_contig_plane_dma_addr(&vpfe->cur_frm->vb, 0); >> + >> + v4l2_get_timestamp(&vpfe->cur_frm->vb.v4l2_buf.timestamp); > > That can't be right. You haven't captured the frame yet, so why set > the timestamp now? > Dropped it not needed. >> + >> + vpfe_set_sdr_addr(&vpfe->ccdc, (unsigned long)(addr)); >> + >> + vpfe_pcr_enable(&vpfe->ccdc, 1); >> + >> + return 0; >> + >> +err: >> + list_for_each_entry_safe(buf, tmp, &vpfe->dma_queue, list) { >> + list_del(&buf->list); >> + vb2_buffer_done(&buf->vb, VB2_BUF_STATE_QUEUED); >> + } >> + spin_unlock_irqrestore(&vpfe->dma_queue_lock, flags); >> + >> + return ret; >> +} >> + >> +/* >> + * vpfe_stop_streaming : Stop the DMA engine >> + * @vq: ptr to vb2_queue >> + * >> + * This callback stops the DMA engine and any remaining buffers >> + * in the DMA queue are released. >> + */ >> +static void vpfe_stop_streaming(struct vb2_queue *vq) >> +{ >> + struct vpfe_device *vpfe = vb2_get_drv_priv(vq); >> + struct vpfe_subdev_info *sdinfo; >> + unsigned long flags; >> + int ret; >> + >> + vpfe_pcr_enable(&vpfe->ccdc, 0); >> + >> + vpfe_detach_irq(vpfe); >> + >> + sdinfo = vpfe->current_subdev; >> + ret = v4l2_subdev_call(sdinfo->sd, video, s_stream, 0); >> + if (ret && ret != -ENOIOCTLCMD && ret != -ENODEV) >> + vpfe_dbg(1, vpfe, "stream off failed in subdev\n"); >> + >> + /* release all active buffers */ >> + spin_lock_irqsave(&vpfe->dma_queue_lock, flags); >> + if (vpfe->cur_frm == vpfe->next_frm) { >> + vb2_buffer_done(&vpfe->cur_frm->vb, VB2_BUF_STATE_ERROR); >> + } else { >> + if (vpfe->cur_frm != NULL) >> + vb2_buffer_done(&vpfe->cur_frm->vb, >> + VB2_BUF_STATE_ERROR); >> + if (vpfe->next_frm != NULL) >> + vb2_buffer_done(&vpfe->next_frm->vb, >> + VB2_BUF_STATE_ERROR); >> + } >> + >> + while (!list_empty(&vpfe->dma_queue)) { >> + vpfe->next_frm = list_entry(vpfe->dma_queue.next, >> + struct vpfe_cap_buffer, list); >> + list_del(&vpfe->next_frm->list); >> + vb2_buffer_done(&vpfe->next_frm->vb, VB2_BUF_STATE_ERROR); >> + } >> + spin_unlock_irqrestore(&vpfe->dma_queue_lock, flags); >> +} >> + >> +static int vpfe_cropcap(struct file *file, void *priv, >> + struct v4l2_cropcap *crop) >> +{ >> + struct vpfe_device *vpfe = video_drvdata(file); >> + >> + vpfe_dbg(2, vpfe, "vpfe_cropcap\n"); >> + >> + if (vpfe->std_index >= ARRAY_SIZE(vpfe_standards)) >> + return -EINVAL; >> + >> + memset(crop, 0, sizeof(struct v4l2_cropcap)); >> + >> + crop->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; >> + crop->defrect.width = vpfe_standards[vpfe->std_index].width; >> + crop->bounds.width = crop->defrect.width; >> + crop->defrect.height = vpfe_standards[vpfe->std_index].height; >> + crop->bounds.height = crop->defrect.height; >> + crop->pixelaspect = vpfe_standards[vpfe->std_index].pixelaspect; >> + >> + return 0; >> +} >> + >> +static int >> +vpfe_g_selection(struct file *file, void *fh, struct v4l2_selection *s) >> +{ >> + struct vpfe_device *vpfe = video_drvdata(file); >> + >> + switch (s->target) { >> + case V4L2_SEL_TGT_COMPOSE_DEFAULT: >> + case V4L2_SEL_TGT_COMPOSE_BOUNDS: >> + case V4L2_SEL_TGT_CROP_BOUNDS: >> + case V4L2_SEL_TGT_CROP_DEFAULT: >> + s->r.left = s->r.top = 0; >> + s->r.width = vpfe->crop.width; >> + s->r.height = vpfe->crop.height; >> + break; >> + >> + case V4L2_SEL_TGT_COMPOSE: >> + case V4L2_SEL_TGT_CROP: >> + s->r = vpfe->crop; >> + break; >> + >> + default: >> + return -EINVAL; >> + } >> + >> + return 0; >> +} >> + >> +static int enclosed_rectangle(struct v4l2_rect *a, struct v4l2_rect *b) >> +{ >> + if (a->left < b->left || a->top < b->top) >> + return 0; >> + >> + if (a->left + a->width > b->left + b->width) >> + return 0; >> + >> + if (a->top + a->height > b->top + b->height) >> + return 0; >> + >> + return 1; >> +} >> + >> +static int >> +vpfe_s_selection(struct file *file, void *fh, struct v4l2_selection *s) >> +{ >> + struct vpfe_device *vpfe = video_drvdata(file); >> + struct v4l2_rect cr = vpfe->crop; >> + struct v4l2_rect r = s->r; >> + > > I would expect to see a vb2_is_busy check here. > > s->target isn't checked either. > Fixed it. >> + v4l_bound_align_image(&r.width, 0, cr.width, 0, >> + &r.height, 0, cr.height, 0, 0); >> + >> + r.left = clamp_t(unsigned int, r.left, 0, cr.width - r.width); >> + r.top = clamp_t(unsigned int, r.top, 0, cr.height - r.height); >> + >> + if (s->flags & V4L2_SEL_FLAG_LE && !enclosed_rectangle(&r, &s->r)) >> + return -ERANGE; >> + >> + if (s->flags & V4L2_SEL_FLAG_GE && !enclosed_rectangle(&s->r, &r)) >> + return -ERANGE; >> + >> + s->r = vpfe->crop = r; >> + >> + vpfe_ccdc_set_image_window(&vpfe->ccdc, &r, vpfe->bpp); >> + vpfe->fmt.fmt.pix.width = r.width; >> + vpfe->fmt.fmt.pix.height = r.height; >> + vpfe->fmt.fmt.pix.bytesperline = vpfe_ccdc_get_line_length(&vpfe->ccdc); >> + vpfe->fmt.fmt.pix.sizeimage = vpfe->fmt.fmt.pix.bytesperline * >> + vpfe->fmt.fmt.pix.height; >> + >> + vpfe_dbg(1, vpfe, "cropped (%d,%d)/%dx%d of %dx%d\n", >> + r.left, r.top, r.width, r.height, cr.width, cr.height); >> + >> + return 0; >> +} >> + >> +static long vpfe_ioctl_default(struct file *file, void *priv, >> + bool valid_prio, unsigned int cmd, void *param) >> +{ >> + struct vpfe_device *vpfe = video_drvdata(file); >> + int ret; >> + >> + vpfe_dbg(2, vpfe, "vpfe_ioctl_default\n"); >> + >> + if (!valid_prio) { >> + vpfe_err(vpfe, "%s device busy\n", __func__); >> + return -EBUSY; >> + } >> + >> + /* If streaming is started, return error */ >> + if (vb2_is_busy(&vpfe->buffer_queue)) { >> + vpfe_err(vpfe, "%s device busy\n", __func__); >> + return -EBUSY; >> + } >> + >> + switch (cmd) { >> + case VIDIOC_AM437X_CCDC_CFG: >> + ret = vpfe_ccdc_set_params(&vpfe->ccdc, param); >> + if (ret) { >> + vpfe_dbg(2, vpfe, >> + "Error setting parameters in CCDC\n"); >> + return ret; >> + } >> + ret = vpfe_get_ccdc_image_format(vpfe, >> + &vpfe->fmt); >> + if (ret < 0) { >> + vpfe_dbg(2, vpfe, >> + "Invalid image format at CCDC\n"); >> + return ret; >> + } >> + break; >> + default: >> + ret = -ENOTTY; > > Add a 'break' here. > > Or just do 'return -ENOTTY;' > >> + } >> + >> + return ret; >> +} >> + >> +static const struct vb2_ops vpfe_video_qops = { >> + .wait_prepare = vb2_ops_wait_prepare, >> + .wait_finish = vb2_ops_wait_finish, >> + .queue_setup = vpfe_queue_setup, >> + .buf_prepare = vpfe_buffer_prepare, >> + .buf_queue = vpfe_buffer_queue, >> + .start_streaming = vpfe_start_streaming, >> + .stop_streaming = vpfe_stop_streaming, >> +}; >> + >> +/* vpfe capture driver file operations */ >> +static const struct v4l2_file_operations vpfe_fops = { >> + .owner = THIS_MODULE, >> + .open = vpfe_open, >> + .release = vpfe_release, >> + .read = vb2_fop_read, >> + .poll = vb2_fop_poll, >> + .unlocked_ioctl = video_ioctl2, >> + .mmap = vb2_fop_mmap, >> + >> +}; >> + >> +/* vpfe capture ioctl operations */ >> +static const struct v4l2_ioctl_ops vpfe_ioctl_ops = { >> + .vidioc_querycap = vpfe_querycap, >> + .vidioc_enum_fmt_vid_cap = vpfe_enum_fmt, >> + .vidioc_g_fmt_vid_cap = vpfe_g_fmt, >> + .vidioc_s_fmt_vid_cap = vpfe_s_fmt, >> + .vidioc_try_fmt_vid_cap = vpfe_try_fmt, >> + >> + .vidioc_enum_framesizes = vpfe_enum_size, >> + >> + .vidioc_enum_input = vpfe_enum_input, >> + .vidioc_g_input = vpfe_g_input, >> + .vidioc_s_input = vpfe_s_input, >> + >> + .vidioc_querystd = vpfe_querystd, >> + .vidioc_s_std = vpfe_s_std, >> + .vidioc_g_std = vpfe_g_std, >> + >> + .vidioc_reqbufs = vb2_ioctl_reqbufs, >> + .vidioc_create_bufs = vb2_ioctl_create_bufs, >> + .vidioc_prepare_buf = vb2_ioctl_prepare_buf, >> + .vidioc_querybuf = vb2_ioctl_querybuf, >> + .vidioc_qbuf = vb2_ioctl_qbuf, >> + .vidioc_dqbuf = vb2_ioctl_dqbuf, >> + .vidioc_expbuf = vb2_ioctl_expbuf, >> + .vidioc_streamon = vb2_ioctl_streamon, >> + .vidioc_streamoff = vb2_ioctl_streamoff, >> + >> + .vidioc_log_status = v4l2_ctrl_log_status, >> + .vidioc_subscribe_event = v4l2_ctrl_subscribe_event, >> + .vidioc_unsubscribe_event = v4l2_event_unsubscribe, >> + >> + .vidioc_cropcap = vpfe_cropcap, >> + .vidioc_g_selection = vpfe_g_selection, >> + .vidioc_s_selection = vpfe_s_selection, >> + >> + .vidioc_default = vpfe_ioctl_default, >> +}; >> + >> +static const struct video_device vpfe_videodev = { >> + .name = VPFE_MODULE_NAME, >> + .fops = &vpfe_fops, >> + .ioctl_ops = &vpfe_ioctl_ops, >> + .minor = -1, >> + .release = video_device_release, >> + .tvnorms = 0, >> +}; >> + >> +static int >> +vpfe_async_bound(struct v4l2_async_notifier *notifier, >> + struct v4l2_subdev *subdev, >> + struct v4l2_async_subdev *asd) >> +{ >> + struct vpfe_device *vpfe = container_of(notifier->v4l2_dev, >> + struct vpfe_device, v4l2_dev); >> + struct vpfe_subdev_info *sdinfo; >> + int i, j; >> + >> + vpfe_dbg(1, vpfe, "vpfe_async_bound\n"); >> + >> + for (i = 0; i < ARRAY_SIZE(vpfe->cfg->asd); i++) { >> + sdinfo = &vpfe->cfg->sub_devs[i]; >> + >> + if (!strcmp(sdinfo->name, subdev->name)) { >> + vpfe->sd[i] = subdev; >> + vpfe_info(vpfe, >> + "v4l2 sub device %s registered\n", >> + subdev->name); >> + vpfe->sd[i]->grp_id = >> + sdinfo->grp_id; >> + /* update tvnorms from the sub devices */ >> + for (j = 0; j < 1; j++) >> + vpfe->video_dev->tvnorms |= >> + sdinfo->inputs[j].std; >> + return 0; >> + } >> + } >> + >> + vpfe_info(vpfe, "vpfe_async_bound sub device (%s) not matched\n", >> + subdev->name); >> + >> + return -EINVAL; >> +} >> + >> +static int vpfe_probe_complete(struct vpfe_device *vpfe) >> +{ >> + struct video_device *vfd; >> + struct vb2_queue *q; >> + int err; >> + >> + spin_lock_init(&vpfe->dma_queue_lock); >> + mutex_init(&vpfe->lock); >> + >> + vpfe->fmt.type = V4L2_BUF_TYPE_VIDEO_CAPTURE; >> + >> + /* set first sub device as current one */ >> + vpfe->current_subdev = &vpfe->cfg->sub_devs[0]; >> + vpfe->v4l2_dev.ctrl_handler = vpfe->sd[0]->ctrl_handler; >> + >> + /* Initialize videobuf2 queue as per the buffer type */ >> + vpfe->alloc_ctx = vb2_dma_contig_init_ctx(vpfe->pdev); >> + if (IS_ERR(vpfe->alloc_ctx)) { >> + vpfe_err(vpfe, "Failed to get the context\n"); >> + err = PTR_ERR(vpfe->alloc_ctx); >> + goto probe_out; >> + } >> + >> + q = &vpfe->buffer_queue; >> + q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; >> + q->io_modes = VB2_MMAP | VB2_DMABUF | VB2_READ; >> + q->drv_priv = vpfe; >> + q->ops = &vpfe_video_qops; >> + q->mem_ops = &vb2_dma_contig_memops; >> + q->buf_struct_size = sizeof(struct vpfe_cap_buffer); >> + q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC; >> + q->lock = &vpfe->lock; >> + q->min_buffers_needed = 1; >> + >> + err = vb2_queue_init(q); >> + if (err) { >> + vpfe_err(vpfe, "vb2_queue_init() failed\n"); >> + vb2_dma_contig_cleanup_ctx(vpfe->alloc_ctx); >> + goto probe_out; >> + } >> + >> + INIT_LIST_HEAD(&vpfe->dma_queue); >> + >> + vfd = vpfe->video_dev; >> + /* Pass on debug flag if it is high enough */ >> + vfd->debug = (debug >= 4)?1:0; > > Don't set this. It can be enabled dynamically by 'echo 1 >/sys/class/video4linux/video0/debug'. > Drivers shouldn't touch vfd->debug. > Dropped. >> + vfd->queue = q; >> + >> + /* >> + * Provide a mutex to v4l2 core. It will be used to protect >> + * all fops and v4l2 ioctls. >> + */ >> + vfd->lock = &vpfe->lock; >> + /* set video driver private data */ >> + video_set_drvdata(vfd, vpfe); >> + >> + /* select input 0 */ >> + err = vpfe_set_input(vpfe, 0); >> + if (err) >> + goto probe_out; >> + >> + err = video_register_device(vpfe->video_dev, VFL_TYPE_GRABBER, -1); >> + if (err) { >> + vpfe_err(vpfe, >> + "Unable to register video device.\n"); >> + goto probe_out; >> + } >> + >> + return 0; >> + >> +probe_out: >> + v4l2_device_unregister(&vpfe->v4l2_dev); >> + return err; >> +} >> + >> +static int vpfe_async_complete(struct v4l2_async_notifier *notifier) >> +{ >> + struct vpfe_device *vpfe = container_of(notifier->v4l2_dev, >> + struct vpfe_device, v4l2_dev); >> + >> + return vpfe_probe_complete(vpfe); >> +} >> + >> +static struct vpfe_config * >> +vpfe_get_pdata(struct platform_device *pdev) >> +{ >> + struct device_node *endpoint = NULL, *rem = NULL; >> + struct v4l2_of_endpoint bus_cfg; >> + struct vpfe_subdev_info *sdinfo; >> + struct vpfe_config *pdata; >> + unsigned int flags; >> + unsigned int i; >> + int err; >> + >> + dev_dbg(&pdev->dev, "vpfe_get_pdata\n"); >> + >> + if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node) >> + return pdev->dev.platform_data; >> + >> + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL); >> + if (!pdata) >> + return NULL; >> + >> + for (i = 0; ; i++) { >> + endpoint = of_graph_get_next_endpoint(pdev->dev.of_node, >> + endpoint); >> + if (!endpoint) >> + break; >> + >> + sdinfo = &pdata->sub_devs[i]; >> + sdinfo->grp_id = 0; >> + >> + /* we only support camera */ >> + sdinfo->inputs[0].index = i; >> + strcpy(sdinfo->inputs[0].name, "camera"); > > Use 'Camera' (capital C) > OK fixed it. >> + sdinfo->inputs[0].type = V4L2_INPUT_TYPE_CAMERA; >> + sdinfo->inputs[0].std = V4L2_STD_ALL; >> + sdinfo->inputs[0].capabilities = V4L2_IN_CAP_STD; >> + >> + sdinfo->can_route = 0; >> + sdinfo->routes = NULL; >> + >> + of_property_read_u32(endpoint, "ti,am437x-vpfe-interface", >> + &sdinfo->vpfe_param.if_type); >> + if (sdinfo->vpfe_param.if_type < 0 || >> + sdinfo->vpfe_param.if_type > 4) { >> + sdinfo->vpfe_param.if_type = VPFE_RAW_BAYER; >> + } >> + >> + err = v4l2_of_parse_endpoint(endpoint, &bus_cfg); >> + if (err) { >> + dev_err(&pdev->dev, "Could not parse the endpoint\n"); >> + goto done; >> + } >> + >> + sdinfo->vpfe_param.bus_width = bus_cfg.bus.parallel.bus_width; >> + >> + if (sdinfo->vpfe_param.bus_width < 8 || >> + sdinfo->vpfe_param.bus_width > 16) { >> + dev_err(&pdev->dev, "Invalid bus width.\n"); >> + goto done; >> + } >> + >> + flags = bus_cfg.bus.parallel.flags; >> + >> + if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH) >> + sdinfo->vpfe_param.hdpol = 1; >> + >> + if (flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH) >> + sdinfo->vpfe_param.vdpol = 1; >> + >> + rem = of_graph_get_remote_port_parent(endpoint); >> + if (!rem) { >> + dev_err(&pdev->dev, "Remote device at %s not found\n", >> + endpoint->full_name); >> + goto done; >> + } >> + >> + strncpy(sdinfo->name, rem->name, sizeof(sdinfo->name)); >> + >> + pdata->asd[i] = devm_kzalloc(&pdev->dev, >> + sizeof(struct v4l2_async_subdev), >> + GFP_KERNEL); >> + pdata->asd[i]->match_type = V4L2_ASYNC_MATCH_OF; >> + pdata->asd[i]->match.of.node = rem; >> + of_node_put(endpoint); >> + of_node_put(rem); >> + } >> + >> + of_node_put(endpoint); >> + return pdata; >> + >> +done: >> + of_node_put(endpoint); >> + of_node_put(rem); >> + return NULL; >> +} >> + >> +/* >> + * vpfe_probe : This function creates device entries by register >> + * itself to the V4L2 driver and initializes fields of each >> + * device objects >> + */ >> +static int vpfe_probe(struct platform_device *pdev) >> +{ >> + struct vpfe_config *vpfe_cfg = vpfe_get_pdata(pdev); >> + struct video_device *vfd; >> + struct vpfe_device *vpfe; >> + struct vpfe_ccdc *ccdc; >> + struct resource *res; >> + int ret; >> + >> + if (!vpfe_cfg) { >> + dev_err(&pdev->dev, "No platform data\n"); >> + return -EINVAL; >> + } >> + >> + vpfe = devm_kzalloc(&pdev->dev, sizeof(*vpfe), GFP_KERNEL); >> + if (!vpfe) >> + return -ENOMEM; >> + >> + vpfe->pdev = &pdev->dev; >> + vpfe->cfg = vpfe_cfg; >> + ccdc = &vpfe->ccdc; >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + ccdc->ccdc_cfg.base_addr = devm_ioremap_resource(&pdev->dev, res); >> + if (IS_ERR(ccdc->ccdc_cfg.base_addr)) >> + return PTR_ERR(ccdc->ccdc_cfg.base_addr); >> + >> + vpfe->irq = platform_get_irq(pdev, 0); >> + if (vpfe->irq <= 0) { >> + dev_err(&pdev->dev, "No IRQ resource\n"); >> + return -ENODEV; >> + } >> + >> + ret = devm_request_irq(vpfe->pdev, vpfe->irq, vpfe_isr, 0, >> + "vpfe_capture0", vpfe); >> + if (ret) { >> + dev_err(&pdev->dev, "Unable to request interrupt\n"); >> + return -EINVAL; >> + } >> + >> + vfd = video_device_alloc(); > > I recommend that struct video_device is embedded in struct vpfe_device. > If nothing else, it avoids the !vfd check below. > OK >> + if (!vfd) { >> + dev_err(&pdev->dev, "Unable to alloc video device\n"); >> + return -ENOMEM; >> + } >> + >> + /* Initialize field of video device */ >> + *vfd = vpfe_videodev; >> + >> + /* Set video_dev to the video device */ >> + vpfe->video_dev = vfd; >> + >> + ret = v4l2_device_register(&pdev->dev, &vpfe->v4l2_dev); >> + if (ret) { >> + vpfe_err(vpfe, >> + "Unable to register v4l2 device.\n"); >> + goto probe_out_video_release; >> + } >> + >> + /* set the driver data in platform device */ >> + platform_set_drvdata(pdev, vpfe); >> + >> + vfd->v4l2_dev = &vpfe->v4l2_dev; >> + >> + /* Enabling module functional clock */ >> + pm_runtime_enable(&pdev->dev); >> + >> + /* for now just enable it here instead of waiting for the open */ >> + pm_runtime_get_sync(&pdev->dev); >> + >> + vpfe_ccdc_config_defaults(ccdc); >> + >> + pm_runtime_put_sync(&pdev->dev); >> + >> + vpfe->sd = kmalloc(sizeof(struct v4l2_subdev *) * > > Wouldn't kzalloc be better? > OK. Thanks, --Prabhakar Lad -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html