Hi, > From: Hans Verkuil [mailto:hverkuil@xxxxxxxxx] <snip> > > + { > > + .id = V4L2_CID_PRIVATE_ISP_COLOR_FX, > > + .type = V4L2_CTRL_TYPE_INTEGER, > > + .name = "Color Effects", > > What does this do? It definitely looks like this should be a menu, not > an integer control. Forcing color to B&W is something that other hardware > can do as well, so I think this is a very good candidate for a generic > user control, rather than driver specific. This basically enables some common color effects on image processing: - Default colors (i.e. original captured colors) - Sepia colored images - B&W colored images. I think its generic enough to propose for a new generic user CID. Ok, so, should I keep the Private CID as a menu type ctrl by now? > > > + .minimum = PREV_DEFAULT_COLOR, > > + .maximum = PREV_BW_COLOR, > > + .step = 1, > > + .default_value = PREV_DEFAULT_COLOR, > > + }, > > + .current_value = PREV_DEFAULT_COLOR, > > + } > > +}; > > + <snip> > > + > > + if (((isp_obj.if_status == ISP_PARLL) && (if_t == ISP_CSIA)) || > > + ((isp_obj.if_status == ISP_CSIA) && > > + (if_t == ISP_PARLL)) || > > + ((isp_obj.if_status == ISP_CSIA) && > > + (if_t == ISP_CSIB)) || > > + ((isp_obj.if_status == ISP_CSIB) && > > + (if_t == ISP_CSIA)) || > > + (isp_obj.if_status == 0)) { > > Hard to understand. Why not: > > if ((isp_obj.if_status == ISP_PARLL && if_t == ISP_CSIA) || > (isp_obj.if_status == ISP_CSIA && if_t == ISP_PARLL) || > (isp_obj.if_status == ISP_CSIA && if_t == ISP_CSIB) || > (isp_obj.if_status == ISP_CSIB && if_t == ISP_CSIA) || > isp_obj.if_status == 0) { > Done > > + isp_obj.if_status |= if_t; > > + return 0; > > + } else { > > + DPRINTK_ISPCTRL("ISP_ERR : Invalid Combination Serial- \ > > + Parallel interface\n"); > > + return -EINVAL; > > + } > > +} > > +EXPORT_SYMBOL(isp_request_interface); <snip> > > + /* CONTROL_ */ > > + omap_writel( > > + /* CSIb receiver data/clock or data/strobe mode */ > > + (config->u.csi.signalling << 10) > > + | BIT(12) /* Enable differential transceiver */ > > + | BIT(13) /* Disable reset */ > > +#ifdef TERM_RESISTOR > > What is this define? It doesn't seem to be documented. This define is to enable/disable an OMAP34xx internal resistor for CSIb (CCP2) camera port. A comment along its declaration should be enough? Thanks for your time. Regards, Sergio > > Regards, > > Hans > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
