2011/8/31 Bastian Hecht <hechtb@xxxxxxxxxxxxxx>: > 2011/8/31 Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>: >> Hi Bastian, >> >> On Wednesday 31 August 2011 17:27:49 Bastian Hecht wrote: >>> 2011/8/28 Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>: >>> > 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 :-) >> >> Guennadi, do you have any update on this ? Hans posted patches early this >> year, do you know if there has been any work on them since then ? >> >>> >> Signed-off-by: Bastian Hecht <hechtb@xxxxxxxxx> >>> >> --- >>> >> >>> >> diff --git a/drivers/media/video/ov5642.c b/drivers/media/video/ov5642.c >>> >> index 1b40d90..069a720 100644 >>> >> --- a/drivers/media/video/ov5642.c >>> >> +++ b/drivers/media/video/ov5642.c >>> >> @@ -74,6 +74,34 @@ >>> >> #define REG_AVG_WINDOW_END_Y_HIGH 0x5686 >>> >> #define REG_AVG_WINDOW_END_Y_LOW 0x5687 >>> >> >>> >> +/* default values in native space */ >>> >> +#define DEFAULT_RBBALANCE 0x400 >>> >> +#define DEFAULT_CONTRAST 0x20 >>> >> +#define DEFAULT_SATURATION 0x40 >>> >> + >>> >> +#define MAX_EXP_NATIVE 0x01ffff >>> >> +#define MAX_GAIN_NATIVE 0x1f >>> >> +#define MAX_RBBALANCE_NATIVE 0x0fff >>> >> +#define MAX_EXP 0xffff >>> >> +#define MAX_GAIN 0xff >>> >> +#define MAX_RBBALANCE 0xff >>> >> +#define MAX_HUE_TRIG_NATIVE 0x80 >>> >> + >>> >> +#define OV5642_CONTROL_BLUE_SATURATION (V4L2_CID_PRIVATE_BASE + >>> >> 0) +#define OV5642_CONTROL_RED_SATURATION (V4L2_CID_PRIVATE_BASE >>> >> + 1) +#define OV5642_CONTROL_GRAY_SCALE (V4L2_CID_PRIVATE_BASE + 2) >>> >> +#define OV5642_CONTROL_SOLARIZE (V4L2_CID_PRIVATE_BASE + >>> >> 3) >>> > >>> > If I'm not mistaken V4L2_CID_PRIVATE_BASE is deprecated. >>> >>> I checked at http://v4l2spec.bytesex.org/spec/x542.htm, googled >>> "V4L2_CID_PRIVATE_BASE deprecated" and read >>> Documentation/feature-removal-schedule.txt. I couldn't find anything. >>> If it is deprecated, do you have an idea how to offer device specific >>> features to the user? >> >> The basic idea is that controls should be standardized, or at least documented >> and added to the V4L2 spec. Controls should belong to a class, so you should >> select the proper base class and add a big offset (I've used 0x1000) in the >> meantime if you want to export private controls. > > Is this code accessable? Then I can just copy the scheme. > >>> >> + >>> >> +#define EXP_V4L2_TO_NATIVE(x) ((x) << 4) >>> >> +#define EXP_NATIVE_TO_V4L2(x) ((x) >> 4) >>> >> +#define GAIN_V4L2_TO_NATIVE(x) ((x) * MAX_GAIN_NATIVE / MAX_GAIN) >>> >> +#define GAIN_NATIVE_TO_V4L2(x) ((x) * MAX_GAIN / MAX_GAIN_NATIVE) >>> >> +#define RBBALANCE_V4L2_TO_NATIVE(x) ((x) * MAX_RBBALANCE_NATIVE / >>> >> MAX_RBBALANCE) +#define RBBALANCE_NATIVE_TO_V4L2(x) ((x) * MAX_RBBALANCE >>> >> / MAX_RBBALANCE_NATIVE) + >>> >> +/* flaw in the datasheet. we need some extra lines */ >>> >> +#define MANUAL_LONG_EXP_SAFETY_DISTANCE 20 >>> >> + >>> >> /* active pixel array size */ >>> >> #define OV5642_SENSOR_SIZE_X 2592 >>> >> #define OV5642_SENSOR_SIZE_Y 1944 >>> > >>> > [snip] >>> > >>> >> @@ -804,6 +1013,100 @@ static int ov5642_set_resolution(struct >>> >> v4l2_subdev *sd) return ret; >>> >> } >>> >> >>> >> +static int ov5642_restore_state(struct v4l2_subdev *sd) >>> >> +{ >>> >> + struct i2c_client *client = v4l2_get_subdevdata(sd); >>> >> + struct ov5642 *priv = to_ov5642(client); >>> >> + struct v4l2_control set_ctrl; >>> >> + int tmp_red_balance = priv->red_balance; >>> >> + int tmp_blue_balance = priv->blue_balance; >>> >> + int tmp_gain = priv->gain; >>> >> + int tmp_exp = priv->exposure; >>> >> + int ret; >>> >> + >>> >> + set_ctrl.id = V4L2_CID_AUTOGAIN; >>> >> + set_ctrl.value = priv->agc; >>> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >>> > >>> > What about writing to registers directly ? >>> >>> I thought about that too, but code duplication would be very large >>> (e.g. take the hue control). I considered the speedup of avoiding >>> function calls less than the error-proness of this duplication. It is >>> just cleaner imho. >> >> OK. This will be replaced when soc-camera will use the control framework >> anyway. >> >>> >> + >>> >> + if (!priv->agc) { >>> >> + set_ctrl.id = V4L2_CID_GAIN; >>> >> + set_ctrl.value = tmp_gain; >>> >> + if (!ret) >>> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >>> >> + } >>> >> + >>> >> + set_ctrl.id = V4L2_CID_AUTO_WHITE_BALANCE; >>> >> + set_ctrl.value = priv->awb; >>> >> + if (!ret) >>> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >>> >> + >>> >> + if (!priv->awb) { >>> >> + set_ctrl.id = V4L2_CID_RED_BALANCE; >>> >> + set_ctrl.value = tmp_red_balance; >>> >> + if (!ret) >>> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >>> >> + >>> >> + set_ctrl.id = V4L2_CID_BLUE_BALANCE; >>> >> + set_ctrl.value = tmp_blue_balance; >>> >> + if (!ret) >>> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >>> >> + } >>> >> + >>> >> + set_ctrl.id = V4L2_CID_EXPOSURE_AUTO; >>> >> + set_ctrl.value = priv->aec; >>> >> + if (!ret) >>> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >>> >> + >>> >> + if (priv->aec == V4L2_EXPOSURE_MANUAL) { >>> >> + set_ctrl.id = V4L2_CID_EXPOSURE; >>> >> + set_ctrl.value = tmp_exp; >>> >> + if (!ret) >>> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >>> >> + } >>> >> + >>> >> + set_ctrl.id = V4L2_CID_VFLIP; >>> >> + set_ctrl.value = priv->vflip; >>> >> + if (!ret) >>> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >>> >> + >>> >> + set_ctrl.id = OV5642_CONTROL_GRAY_SCALE; >>> >> + set_ctrl.value = priv->grayscale; >>> >> + if (!ret) >>> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >>> >> + >>> >> + set_ctrl.id = OV5642_CONTROL_SOLARIZE; >>> >> + set_ctrl.value = priv->solarize; >>> >> + if (!ret) >>> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >>> >> + >>> >> + set_ctrl.id = OV5642_CONTROL_BLUE_SATURATION; >>> >> + set_ctrl.value = priv->blue_saturation; >>> >> + if (!ret) >>> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >>> >> + >>> >> + set_ctrl.id = OV5642_CONTROL_RED_SATURATION; >>> >> + set_ctrl.value = priv->red_saturation; >>> >> + if (!ret) >>> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >>> >> + >>> >> + set_ctrl.id = V4L2_CID_BRIGHTNESS; >>> >> + set_ctrl.value = priv->brightness; >>> >> + if (!ret) >>> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >>> >> + >>> >> + set_ctrl.id = V4L2_CID_CONTRAST; >>> >> + set_ctrl.value = priv->contrast; >>> >> + if (!ret) >>> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >>> >> + >>> >> + set_ctrl.id = V4L2_CID_HUE; >>> >> + set_ctrl.value = priv->hue; >>> >> + if (!ret) >>> >> + ret = ov5642_s_ctrl(sd, &set_ctrl); >>> >> + >>> >> + return ret; >>> >> +} >>> >> + >>> >> static int ov5642_try_fmt(struct v4l2_subdev *sd, struct >>> >> v4l2_mbus_framefmt *mf) { >>> >> const struct ov5642_datafmt *fmt = >>> >> ov5642_find_datafmt(mf->code); @@ -856,6 +1159,9 @@ static int >>> >> ov5642_s_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf) if >>> >> (!ret) >>> >> ret = ov5642_write_array(client, >>> >> ov5642_default_regs_finalise); >>> >> >>> >> + /* the chip has been reset, so configure it again */ >>> >> + if (!ret) >>> >> + ret = ov5642_restore_state(sd); >>> > >>> > I suppose there's no way to avoid resetting the chip ? >>> >>> Whenever the clock is down, the chip looses its state. >> >> But the clock isn't turned down at every s_fmt call. Would it be possible >> reinit the chip in the .s_power operation instead ? > > Guennadi had the same idea. I tried it out already to do it in > s_power, but the chip hangs most times then. Even when I use mplayer > and the s_power() is closely followed by the s_fmt() the chip crashes. > Witch the same register writes but a small time gap. The chip has > suuuch a strange behavior that I gave up trying to solve it. It sounds > quite unbelievable I must admit, but meantime I stopped being amazed > by the ov5642. To be more precise I tried to separate it to inialise in s_power, set_resolution in s_crop() and s_fmt() and finalise in streamon(). >>> >> return ret; >>> >> } >>> > >>> > [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. >>> >>> >> + break; >> >> -- >> Regards, >> >> Laurent Pinchart >> > -- 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
