On Monday 12 January 2009 21:16:28 Aguirre Rodriguez, Sergio Alberto wrote: > 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. Yes. > Ok, so, should I keep the Private CID as a menu type ctrl by now? No, just propose a new control for this. Shouldn't be a problem. The only suggestion that I have is that the user control has B&W as the second entry rather than the third. There are more devices that can choose between color and B&W, but this is the first device I know of that can do sepia. Having B&W as the second entry makes it easier for drivers to omit the sepia entry by just setting the max control value (= menu index) to B&W. Note that in two weeks time I'm going to merge the V4L2 specification into our v4l-dvb repository. That should make it easy to add documentation for this new control as well. > > > > + .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? Yes. But shouldn't this be a module option, perhaps? Up to you, I'm just wondering. > Thanks for your time. No problem, Hans -- Hans Verkuil - video4linux developer - sponsored by TANDBERG -- 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
