On Monday 12 January 2009 21:16:28 Aguirre Rodriguez, Sergio Alberto
wrote:
> Hi,
>
> > From: Hans Verkuil [mailto:hverk...@xs4all.nl]
>
> <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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
