Hi Sakari,
I've submitted V2 yesterday. If possible, can you review that one also ?
I'm learning many things from your review comments.
I think in V2, I've addressed most of comments except raw bayer format.
For ray bayer format, for now, I intentionally don't support crop since it requires more complexity to meet request from _set_pad_format() while keeping FOV for the resolutions with the same ratio(4:3 or 16:9).
Yes, it is hacky but I thought it's OK unless there's a need to support crop. Hm..... I'm thinking drop "bayer order change" since it is not that meaningful. Should I ?
For VBLANK, I realized I made wrong comments just after I send it. Yeas, it shouldn't be read-only. So you can see that VBLANK I added in V2 is NOT read-only.
Thanks,
Hyungwoo
> Hi Hyungwoo,
>
> On Wed, May 24, 2017 at 11:13:50PM +0000, Yang, Hyungwoo wrote:
> ...
> > > > +static inline int ov13858_write_reg_list(struct ov13858 *ov13858,
> > >
> > > I'd drop inline.
> >
> > if it's not mandatory for upstream, I prefer to keep inline for people
> > who want to port this with a not-good-compiler. Is it mandatory ?
>
> I don't think you'd really lose anything if the compiler didn't inline it.
> It's a non-issue anyway.
>
> ...
>
> > > > +/*
> > > > + * Change the bayer order to meet the requested one.
> > > > + */
> > > > +static int ov13858_apply_bayer_order(struct ov13858 *ov13858) {
> > > > + int ret;
> > > > +
> > > > + switch (ov13858->cur_bayer_format) {
> > > > + case MEDIA_BUS_FMT_SGRBG10_1X10:
> > > > + break;
> > > > + case MEDIA_BUS_FMT_SRGGB10_1X10:
> > > > + return ov13858_increase_offset(ov13858, OV13858_REG_H_OFFSET);
> > > > + case MEDIA_BUS_FMT_SGBRG10_1X10:
> > > > + ret = ov13858_increase_offset(ov13858, OV13858_REG_H_OFFSET);
> > >
> > > The bayer pixel order is defined by cropping the pixel array. If the sensor can do that, you should implement support for the crop selection rectangle instead.
> >
> > Sorry, I'm new to imaging world but, as you can see, bayer order in
> > this sensor IS DEFINED by both cropping and offset(where you start to
> > read). Is there a strict (implicit or explicit) rule or specific
> > reason that we should use only crop to apply expected bayer order,
> > even though the bayer order in the sensor is defined by both crop and offset ?
> >
> > Anyway, I changed H_/V_OFFSET(0x3810, 0x3812) to
> > H_/V_CROP_START(0x3800,
> > 0x3802) with no changes in initial values.
>
> The CROP selection rectangle is the interface to configure crop an area from the parent rectangle (NATIVE_SIZE in this case). Using the format to change cropping in pre-defined ways is quite hackish.
>
> ...
>
> > > > +/* Exposure control */
> > > > +static int ov13858_update_exposure(struct ov13858 *ov13858,
> > > > + struct v4l2_ctrl *ctrl)
> > > > +{
> > > > + int ret;
> > > > + u32 exposure, new_vts = 0;
> > > > +
> > > > + exposure = ctrl->val;
> > > > + if (exposure > ov13858->cur_mode->vts - 8)
> > > > + new_vts = exposure + 8;
> > > > + else
> > > > + new_vts = ov13858->cur_mode->vts;
> > >
> > > Instead of changing the vertical blanking interval implicitly, could
> > > you do it explicitly though the VBLANK control instead?
> > >
> > > As you do already control the vertical sync and provide the pixel
> > > rate control, how about adding a HBLANK control as well? I suppose
> > > it could be added later on as well. And presumably will be read only.
> >
> > I'll introduce VBLANK control with READ ONLY and the value of the
> > control will be updated here.
>
> HBLANK would be read-only since the register list that you have might contain dependencies to the horizontal blanking so you can't change that.
> The VBLANK control, instead, should not be read-only to allow controlling the frame rate and also controlling the exposure without affecting the frame rate.
>
> >
> > >
> > > > +
> > > > + ret = ov13858_group_hold_start(ov13858, 0);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + ret = ov13858_write_reg(ov13858, OV13858_REG_VTS,
> > > > + OV13858_REG_VALUE_16BIT, new_vts);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > >
> > > If you want group hold for that, too, we need a new callback (or
> > > two) for the control handler I believe.
> >
> > I don't understand wht this means. Can you give me detail ?
>
> The V4L2 control framework calls the s_ctrl() callback in the driver to set control values. The driver however doesn't know how many controls there will be to set or when the last control of the set would be conveyed to the driver. To use the grouped parameter hold meaningfully this information is needed.
>
> ...
>
> > > > + /* Values of V4L2 controls will be applied only when power is up */
> > > > + if (atomic_read(&client->dev.power.usage_count) == 0)
> > >
> > > I wonder if using pm_runtime_active() would work for this. Checking
> > > the usage_count directly does not look like something a driver
> > > should be doing.
> >
> > Agree, I really wanted to use any helper(accesor) method for this but
> > when I checked the pm_runtime_active() it wasn't good enough. Anyway I
> > just found better one for this case. I'll not use using usage_count
> > and instread of using pm_runtime_get_sync, I'll use
> > pm_runtime_get_if_in_use()
>
> Ah, that seems much better indeed!
>
> >
> > >
> > > > + return 0;
> > > > +
> > > > + ret = pm_runtime_get_sync(&client->dev);
> > > > + if (ret < 0) {
> > > > + pm_runtime_put_noidle(&client->dev);
> > > > + return ret;
> > > > + }
> > > > +
> > > > + ret = 0;
> > > > + switch (ctrl->id) {
> > > > + case V4L2_CID_ANALOGUE_GAIN:
> > > > + ret = ov13858_update_analog_gain(ov13858, ctrl);
> > > > + break;
> > > > + case V4L2_CID_EXPOSURE:
> > > > + ret = ov13858_update_exposure(ov13858, ctrl);
> > > > + break;
> > > > + default:
> > > > + dev_info(&client->dev,
> > > > + "ctrl(id:0x%x,val:0x%x) is not handled\n",
> > > > + ctrl->id, ctrl->val);
> > > > + break;
> > > > + };
> > > > +
> > > > + pm_runtime_put(&client->dev);
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static const struct v4l2_ctrl_ops ov13858_ctrl_ops = {
> > > > + .s_ctrl = ov13858_set_ctrl,
> > > > +};
> > > > +
> > > > +/* Initialize control handlers */ static int
> > > > +ov13858_init_controls(struct ov13858 *ov13858) {
> > > > + struct i2c_client *client = v4l2_get_subdevdata(&ov13858->sd);
> > > > + struct v4l2_ctrl_handler *ctrl_hdlr;
> > > > + int ret;
> > > > +
> > > > + ctrl_hdlr = &ov13858->ctrl_handler;
> > > > + ret = v4l2_ctrl_handler_init(ctrl_hdlr, 4);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + ctrl_hdlr->lock = &ov13858->mutex;
> > > > + ov13858->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr,
> > > > + &ov13858_ctrl_ops,
> > > > + V4L2_CID_LINK_FREQ,
> > > > + OV13858_NUM_OF_LINK_FREQS - 1,
> > > > + 0,
> > > > + link_freq_menu_items);
> > > > + ov13858->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > > > +
> > > > + /* By default, PIXEL_RATE is read only */
> > > > + ov13858->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &ov13858_ctrl_ops,
> > > > + V4L2_CID_PIXEL_RATE, 0,
> > > > + OV13858_GET_PIXEL_RATE(0), 1,
> > > > + OV13858_GET_PIXEL_RATE(0));
> > > > +
> > > > + v4l2_ctrl_new_std(ctrl_hdlr, &ov13858_ctrl_ops, V4L2_CID_ANALOGUE_GAIN,
> > > > + OV13858_ANA_GAIN_MIN, OV13858_ANA_GAIN_MAX,
> > > > + OV13858_ANA_GAIN_STEP, OV13858_ANA_GAIN_DEFAULT);
> > > > +
> > > > + v4l2_ctrl_new_std(ctrl_hdlr, &ov13858_ctrl_ops, V4L2_CID_EXPOSURE,
> > > > + OV13858_EXP_GAIN_MIN, OV13858_EXP_GAIN_MAX,
> > > > + OV13858_EXP_GAIN_STEP, OV13858_EXP_GAIN_DEFAULT);
> > >
> > > Are the minimum and maximum values dependent on the register list chosen?
> >
> > We are going to use the same HTS for all reolutions.
> > The supports 4 lines as minimum and MAX_VTS - 8 as maximum.
> >
> > >
> > > > + if (ctrl_hdlr->error) {
> > > > + ret = ctrl_hdlr->error;
> > > > + dev_err(&client->dev, "%s control init failed (%d)\n",
> > > > + __func__, ret);
> > > > + goto error;
> > > > + }
> > > > +
> > > > + ov13858->sd.ctrl_handler = ctrl_hdlr;
> > > > +
> > > > + return 0;
> > > > +
> > > > +error:
> > > > + v4l2_ctrl_handler_free(ctrl_hdlr);
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static void ov13858_update_pad_format(struct ov13858 *ov13858,
> > > > + const struct ov13858_mode *mode,
> > > > + struct v4l2_subdev_format *fmt) {
> > > > + fmt->format.width = mode->width;
> > > > + fmt->format.height = mode->height;
> > > > + fmt->format.code = ov13858->cur_bayer_format;
> > > > + fmt->format.field = V4L2_FIELD_NONE; }
> > > > +
> > > > +static int ov13858_do_get_pad_format(struct ov13858 *ov13858,
> > > > + struct v4l2_subdev_pad_config *cfg,
> > > > + struct v4l2_subdev_format *fmt) {
> > > > + struct v4l2_mbus_framefmt *framefmt;
> > > > + struct v4l2_subdev *sd = &ov13858->sd;
> > > > +
> > > > + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> > > > + framefmt = v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
> > > > + fmt->format = *framefmt;
> > >
> > > You could write this as :
> > >
> > > fmt->format = *v4l2_subdev_get_try_format(&ov13858->sd, cfg,
> > > fmt->fmt->pad);
> > >
> >
> > Personally I don't like this since I believe readablity of this kind
> > of code is not good. If there's no stric rule for this, I want to keep
> > this since believe there's no difference in generated code.
>
> I don't think the extra local variable assigned and used once really helps, but ok for me.
>
> --
> Regards,
>
> Sakari Ailus
> e-mail: sakari.ailus@xxxxxx XMPP: sailus@xxxxxxxxxxxxxx
>
