On 09/27/2011 09:14 AM, Hans Verkuil wrote:
> On Monday, September 26, 2011 23:49:56 Sylwester Nawrocki wrote:
>> Hi Hans,
>>
>> thanks for the comments. It's good to see you back, this mailing list
>> had been much more quiet when you've been away for a while;)
>> I hope everything got well for you.
>>
>> On 09/26/2011 03:21 PM, Hans Verkuil wrote:
>>> On Wednesday, September 21, 2011 19:45:07 Sylwester Nawrocki wrote:
>>>> This driver exposes preview mode operation of the S5K6AAFX sensor with
>>>> embedded SoC ISP. It uses one of the five user predefined configuration
>>>> register sets. There is yet no support for capture (snapshot) operation.
>>>> Following controls are supported:
>>>> manual/auto exposure and gain, power line frequency (anti-flicker),
>>>> saturation, sharpness, brightness, contrast, white balance temperature,
>>>> color effects, horizontal/vertical image flip, frame interval.
>>>>
>>>> Signed-off-by: Sylwester Nawrocki<s.nawro...@samsung.com>
>>>> Signed-off-by: Kyungmin Park<kyungmin.p...@samsung.com>
>>>> ---
>> ...
>>>> +/*
>>>> + * V4L2 subdev core and video operations
>>>> + */
>>>> +static int s5k6aa_set_power(struct v4l2_subdev *sd, int on)
>>>> +{
>>>> + struct s5k6aa *s5k6aa = to_s5k6aa(sd);
>>>> + int ret = 0;
>>>> +
>>>> + mutex_lock(&s5k6aa->lock);
>>>> +
>>>> + if (!on == s5k6aa->power) {
>>>> + if (on) {
>>>> + ret = __s5k6aa_power_enable(s5k6aa);
>>>> + if (!ret)
>>>> + ret = s5k6aa_initialize_isp(sd);
>>>> + } else {
>>>> + ret = __s5k6aa_power_disable(s5k6aa);
>>>> + }
>>>> + }
>>>> + if (!ret&& !WARN_ON(s5k6aa->power< 0))
>>>> + s5k6aa->power += on ? 1 : -1;
>>>> + mutex_unlock(&s5k6aa->lock);
>>>> +
>>>> + if (!ret&& on&& s5k6aa->power == 1)
>>>> + return v4l2_ctrl_handler_setup(sd->ctrl_handler);
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static int __s5k6aa_stream(struct s5k6aa *s5k6aa, int enable)
>>>> +{
>>>> + struct i2c_client *client = v4l2_get_subdevdata(&s5k6aa->sd);
>>>> + int ret;
>>>> +
>>>> + ret = s5k6aa_write(client, REG_G_ENABLE_PREV, enable);
>>>> + if (!ret)
>>>> + ret = s5k6aa_write(client, REG_G_ENABLE_PREV_CHG, 1);
>>>> + if (!ret)
>>>> + ret = s5k6aa_write(client, REG_G_NEW_CFG_SYNC, 1);
>>>> + if (!ret)
>>>> + s5k6aa->streaming = enable;
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static int s5k6aa_s_stream(struct v4l2_subdev *sd, int on)
>>>> +{
>>>> + struct s5k6aa *s5k6aa = to_s5k6aa(sd);
>>>> + int ret = 0;
>>>> +
>>>> + mutex_lock(&s5k6aa->lock);
>>>
>>> Stupid question perhaps, but why do you need a lock? Usually these calls are
>>> serialized by the bridge driver. Most subdevs don't use a lock, unless they
>>> start some thread of their own.
>>
>> I wish I could get rid of the lock, but it seems necessary as long as the
>> device
>> can be accessed through two device nodes: /dev/video? and /dev/v4l-subdev?.
>
> Ah yes, that's true.
>
>> It holds mostly for s_ctrl, which can be called on the subdev node, the other
>> subdev ops generally don't attempt to access I2C.
>
> The control framework has its own locking, so s_ctrl is guaranteed to be
> serialized.
Yup, I was aware of that.
> So you don't need to lock there.
I still need the I2C mutex there, which is used to serialize the device I2C
interface access across various subdev ops, s_ctrl among others.
>
>> So this lock is also an I2C interface mutex ensuring correct configuration
>> registers read/write sequences. Especially nothing should be allowed to
>> interfere
>> with the ISP initialization routine, which is executed through s_power op at
>> the time of /dev/video? open().
>>
>> Yes, adding device node to subdevs made things slightly more complicated :)
>
> I need to look at this some more: see if we can support the core locking
> mechanism for subdev nodes as well.
I think it could be useful to have an option to use the core lock. However
I guess it's not easy to implement as the drivers are quite independent.
Maybe it could be added somewhere around v4l2_device_register_subdev_nodes().
>
>> I wonder how other subdevs resolve the serialization without a lock.
>
> They don't have their own device node :-)
Some of them have, IIRC.
>
>>
>>>
>>>> +
>>>> + if (!s5k6aa->streaming == !on) {
>>>> + mutex_unlock(&s5k6aa->lock);
>>>> + return 0;
>>>> + }
>>>> + if (s5k6aa->apply_new_cfg)
>>>> + ret = s5k6aa_set_preview_preset(s5k6aa, s5k6aa->preset);
>>>> + if (!ret)
>>>> + ret = __s5k6aa_stream(s5k6aa, !!on);
>>>> +
>>>> + mutex_unlock(&s5k6aa->lock);
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static int s5k6aa_g_frame_interval(struct v4l2_subdev *sd,
>>>> + struct v4l2_subdev_frame_interval *fi)
>>>> +{
>>>> + struct s5k6aa *s5k6aa = to_s5k6aa(sd);
>>>> +
>>>> + memset(fi->reserved, 0, sizeof(fi->reserved));
>>>> +
>>>> + mutex_lock(&s5k6aa->lock);
>>>> + fi->interval = s5k6aa->fiv->interval;
>>>> + mutex_unlock(&s5k6aa->lock);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int __s5k6aa_set_frame_interval(struct s5k6aa *s5k6aa,
>>>> + struct v4l2_subdev_frame_interval *fi)
>>>> +{
>>>> + struct v4l2_frmsize_discrete *out_win =&s5k6aa->preset->out_size;
>>>> + const struct s5k6aa_interval *fiv =&s5k6aa_intervals[0];
>>>> + unsigned int err, min_err = UINT_MAX;
>>>> + unsigned int i, fr_time;
>>>> +
>>>> + if (fi->interval.denominator == 0)
>>>> + return -EINVAL;
>>>> +
>>>> + memset(fi->reserved, 0, sizeof(fi->reserved));
>>>> + fr_time = fi->interval.numerator * 10000 / fi->interval.denominator;
>>>> +
>>>> + for (i = 0; i< ARRAY_SIZE(s5k6aa_intervals); i++) {
>>>> + const struct s5k6aa_interval *iv =&s5k6aa_intervals[i];
>>>> +
>>>> + if (out_win->width> iv->size.width ||
>>>> + out_win->height> iv->size.height)
>>>> + continue;
>>>> +
>>>> + err = abs(iv->reg_fr_time - fr_time);
>>>> + if (err< min_err) {
>>>> + fiv = iv;
>>>> + min_err = err;
>>>> + }
>>>> + }
>>>> + s5k6aa->fiv = fiv;
>>>> +
>>>> + v4l2_dbg(1, debug,&s5k6aa->sd, "Changed frame interval to %d us\n",
>>>> + fiv->reg_fr_time * 100);
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int s5k6aa_s_frame_interval(struct v4l2_subdev *sd,
>>>> + struct v4l2_subdev_frame_interval *fi)
>>>> +{
>>>> + struct s5k6aa *s5k6aa = to_s5k6aa(sd);
>>>> + int ret;
>>>> +
>>>> + v4l2_dbg(1, debug, sd, "Setting %d/%d frame interval\n",
>>>> + fi->interval.numerator, fi->interval.denominator);
>>>> +
>>>> + mutex_lock(&s5k6aa->lock);
>>>> + ret = __s5k6aa_set_frame_interval(s5k6aa, fi);
>>>> + s5k6aa->apply_new_cfg = 1;
>>>> +
>>>> + mutex_unlock(&s5k6aa->lock);
>>>> + return ret;
>>>> +}
>>>> +
>> ...
>>>> +static const struct v4l2_subdev_pad_ops s5k6aa_pad_ops = {
>>>> + .enum_mbus_code = s5k6aa_enum_mbus_code,
>>>> + .enum_frame_size = s5k6aa_enum_frame_size,
>>>> + .enum_frame_interval = s5k6aa_enum_frame_interval,
>>>> + .get_fmt = s5k6aa_get_fmt,
>>>> + .set_fmt = s5k6aa_set_fmt,
>>>> +};
>>>> +
>>>> +/*
>>>> + * V4L2 subdev control operations
>>>> + */
>>>> +static int s5k6aa_s_ctrl(struct v4l2_ctrl *ctrl)
>>>> +{
>>>> + struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
>>>> + struct i2c_client *c = v4l2_get_subdevdata(sd);
>>>> + struct s5k6aa *s5k6aa = to_s5k6aa(sd);
>>>> + int pid, err = 0;
>>>> +
>>>> + v4l2_dbg(1, debug, sd, "%s: ctrl: 0x%x, value: %d\n",
>>>> + __func__, ctrl->id, ctrl->val);
>>>> +
>>>> + mutex_lock(&s5k6aa->lock);
>>>> + /*
>>>> + * If the device is not powered up by the host driver do
>>>> + * not apply any controls to H/W at this time. Instead
>>>> + * the controls will be restored right after power-up.
>>>> + */
>>>> + if (s5k6aa->power == 0)
>>>> + goto unlock;
>>>> + pid = s5k6aa->preset->index;
>>>> +
>>>> + switch (ctrl->id) {
>>>> + case V4L2_CID_BRIGHTNESS:
>>>> + err = s5k6aa_write(c, REG_USER_BRIGHTNESS, ctrl->val);
>>>> + break;
>>>> +
>>>> + case V4L2_CID_COLORFX:
>>>> + err = s5k6aa_set_colorfx(s5k6aa, ctrl->val);
>>>> + break;
>>>> +
>>>> + case V4L2_CID_CONTRAST:
>>>> + err = s5k6aa_write(c, REG_USER_CONTRAST, ctrl->val);
>>>> + break;
>>>> +
>>>> + case V4L2_CID_EXPOSURE_AUTO:
>>>> + err = s5k6aa_set_auto_exposure(s5k6aa, ctrl->val);
>>>> + break;
>>>> +
>>>> + case V4L2_CID_HFLIP:
>>>> + err = s5k6aa_set_mirror(s5k6aa, ctrl->val);
>>>> + break;
>>>> +
>>>> + case V4L2_CID_POWER_LINE_FREQUENCY:
>>>> + err = s5k6aa_set_anti_flicker(s5k6aa, ctrl->val);
>>>> + break;
>>>> +
>>>> + case V4L2_CID_SATURATION:
>>>> + err = s5k6aa_write(c, REG_USER_SATURATION, ctrl->val);
>>>> + break;
>>>> +
>>>> + case V4L2_CID_SHARPNESS:
>>>> + err = s5k6aa_write(c, REG_USER_SHARPBLUR, ctrl->val);
>>>> + break;
>>>> +
>>>> + case V4L2_CID_WHITE_BALANCE_TEMPERATURE:
>>>> + err = s5k6aa_write(c, REG_P_COLORTEMP(pid), ctrl->val);
>>>> + break;
>>>> + }
>>>> + /* This should be really called once per all controls update
>>>> + rather than per each control. */
>>>> + if (!err)
>>>> + err = s5k6aa_sync_preview_preset(c, 0);
>>>> +unlock:
>>>> + mutex_unlock(&s5k6aa->lock);
>>>> + return err;
>>>> +}
>>>> +
>>>> +static const struct v4l2_ctrl_ops s5k6aa_ctrl_ops = {
>>>> + .s_ctrl = s5k6aa_s_ctrl,
>>>> +};
>>>> +
>>>> +static int s5k6aa_log_status(struct v4l2_subdev *sd)
>>>> +{
>>>> + v4l2_ctrl_handler_log_status(sd->ctrl_handler, sd->name);
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static const struct v4l2_subdev_video_ops s5k6aa_video_ops = {
>>>> + .g_frame_interval = s5k6aa_g_frame_interval,
>>>> + .s_frame_interval = s5k6aa_s_frame_interval,
>>>> + .s_stream = s5k6aa_s_stream,
>>>> +};
>>>> +
>>>> +/*
>>>> + * V4L2 subdev internal operations
>>>> + */
>>>> +static int s5k6aa_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>>>> +{
>>>> + struct v4l2_mbus_framefmt *mf = v4l2_subdev_get_try_format(fh, 0);
>>>> +
>>>> + s5k6aa_get_preset_fmt(to_s5k6aa(sd)->preset, mf);
>>>> + return 0;
>>>> +}
>>>> +
>>>> +int s5k6aa_check_fw_revision(struct s5k6aa *s5k6aa)
>>>> +{
>>>> + struct i2c_client *client = v4l2_get_subdevdata(&s5k6aa->sd);
>>>> + u16 api_ver = 0, fw_rev = 0;
>>>> +
>>>> + int ret = s5k6aa_set_ahb_address(client);
>>>> +
>>>> + if (!ret)
>>>> + ret = s5k6aa_read(client, REG_FW_APIVER,&api_ver);
>>>> + if (!ret)
>>>> + ret = s5k6aa_read(client, REG_FW_REVISION,&fw_rev);
>>>> + if (ret) {
>>>> + v4l2_err(&s5k6aa->sd, "FW revision check failed!\n");
>>>> + return ret;
>>>> + }
>>>> +
>>>> + v4l2_info(&s5k6aa->sd, "FW API ver.: 0x%X, FW rev.: 0x%X\n",
>>>> + api_ver, fw_rev);
>>>> +
>>>> + return api_ver == S5K6AAFX_FW_APIVER ? 0 : -ENODEV;
>>>> +}
>>>> +
>>>> +static int s5k6aa_registered(struct v4l2_subdev *sd)
>>>> +{
>>>> + struct s5k6aa *s5k6aa = to_s5k6aa(sd);
>>>> + int ret;
>>>> +
>>>> + mutex_lock(&s5k6aa->lock);
>>>> + ret = __s5k6aa_power_enable(s5k6aa);
>>>> + if (!ret) {
>>>> + msleep(100);
>>>> + ret = s5k6aa_check_fw_revision(s5k6aa);
>>>> + __s5k6aa_power_disable(s5k6aa);
>>>> + }
>>>> + mutex_unlock(&s5k6aa->lock);
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static const struct v4l2_subdev_internal_ops s5k6aa_subdev_internal_ops =
>>>> {
>>>> + .registered = s5k6aa_registered,
>>>> + .open = s5k6aa_open,
>>>> +};
>>>> +
>>>> +static const struct v4l2_subdev_core_ops s5k6aa_core_ops = {
>>>> + .s_power = s5k6aa_set_power,
>>>> + .g_ctrl = v4l2_subdev_g_ctrl,
>>>> + .s_ctrl = v4l2_subdev_s_ctrl,
>>>> + .queryctrl = v4l2_subdev_queryctrl,
>>>> + .querymenu = v4l2_subdev_querymenu,
>>>> + .g_ext_ctrls = v4l2_subdev_g_ext_ctrls,
>>>> + .try_ext_ctrls = v4l2_subdev_try_ext_ctrls,
>>>> + .s_ext_ctrls = v4l2_subdev_s_ext_ctrls,
>>>
>>> Don't add these control ops. They are only needed if this subdev driver is
>>> used by bridge drivers that are not yet converted to the control framework.
>>> That's not the case, so just remove these ops here.
>>
>> Great! thanks for the hint. I've just added this to the list of changes for
>> v3.
>>
>>>
>>>> + .log_status = s5k6aa_log_status,
>>
>> Do we plan to support this op directly on subdev nodes ? Or should it be only
>> accessible through bridge drivers ?
>> I was just wondering if we should add VIDIOC_LOG_STATUS to subdev_do_ioctl().
>
> We should. Just one of those things that we didn't get around to.
> Can you add it?
Yeah, I could take care of it. I'll also check the Docbook, if we need any
changes
there.
>
> Regards,
>
> Hans
Thanks,
--
Sylwester Nawrocki
Samsung Poland R&D Center
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
