Re: [PATCH] media: Add camera controls for the ov5642 driver

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

 



2011/8/31 Guennadi Liakhovetski <g.liakhovetski@xxxxxx>:
> On Wed, 31 Aug 2011, Bastian Hecht wrote:
>
>> 2011/8/28 Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>:
>> > Hi Bastian,
>> >
>> > Thanks for the patch.
>> >
>> > On Wednesday 17 August 2011 18:02:07 Bastian Hecht wrote:
>> >> The driver now supports automatic/manual gain, automatic/manual white
>> >> balance, automatic/manual exposure control, vertical flip, brightness
>> >> control, contrast control and saturation control. Additionally the
>> >> following effects are available now: rotating the hue in the colorspace,
>> >> gray scale image and solarize effect.
>> >
>> > Any chance to port soc-camera to the control framework ? :-)
>>
>> I redirect that to Guennadi :-)
>
> Hans is prepaing an update of his port, then we'll integrate it... This
> all will take time, so, it's better to do this driver now the "old
> soc-camera" way, and port it later.
>
> [snip]
>
>> >> +static int ov5642_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control
>> >> *ctrl) +{
>> >> +     struct i2c_client *client = v4l2_get_subdevdata(sd);
>> >> +     struct ov5642 *priv = to_ov5642(client);
>> >> +     int ret = 0;
>> >> +     u8 val8;
>> >> +     u16 val16;
>> >> +     u32 val32;
>> >> +     int trig;
>> >> +     struct v4l2_control aux_ctrl;
>> >> +
>> >> +     switch (ctrl->id) {
>> >> +     case V4L2_CID_AUTOGAIN:
>> >> +             if (!ctrl->value) {
>> >> +                     aux_ctrl.id = V4L2_CID_GAIN;
>> >> +                     ret = ov5642_g_ctrl(sd, &aux_ctrl);
>> >> +                     if (ret)
>> >> +                             break;
>> >> +                     priv->gain = aux_ctrl.value;
>> >> +             }
>> >> +
>> >> +             ret = reg_read(client, REG_EXP_GAIN_CTRL, &val8);
>> >> +             if (ret)
>> >> +                     break;
>> >> +             val8 = ctrl->value ? val8 & ~BIT(1) : val8 | BIT(1);
>> >> +             ret = reg_write(client, REG_EXP_GAIN_CTRL, val8);
>> >> +             if (!ret)
>> >> +                     priv->agc = ctrl->value;
>> >
>> > What about caching the content of this register (and of other registers below)
>> > instead of reading it back ? If you can't do that, a reg_clr_set() function
>> > would make the code simpler.
>>
>> Ok I will do the caching.
>
> Wasn't the reason for reading those registers from the hardware, that the
> sensor changes them in auto* modes (autogain in this case)?

There are different cases. E.g. this register sets the autogain and
autoexposure on/off not the gain value itself. So indeed one should
cache it (sometimes I registered side-effects on the ov5642 - setting
1 register changed others. I hope I won't run into trouble with
these).

best,

 Bastian


> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux