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->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
