Re: [REVIEW PATCH 08/14] OMAP: CAM: Add ISP Core

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux