Hello Tomasz, Here's my comments. Thanks, Hyungwoo -----Original Message----- > From: Tomasz Figa [mailto:tfiga@xxxxxxxxxxxx] > Sent: Sunday, May 28, 2017 7:56 PM > To: Yang, Hyungwoo <hyungwoo.yang@xxxxxxxxx> > Cc: Sakari Ailus <sakari.ailus@xxxxxx>; linux-media@xxxxxxxxxxxxxxx; sakari.ailus@xxxxxxxxxxxxxxx; Zheng, Jian Xu <jian.xu.zheng@xxxxxxxxx>; Hsu, Cedric <cedric.hsu@xxxxxxxxx> > Subject: Re: [PATCH v3 1/1] [media] i2c: add support for OV13858 sensor > > Hi Hyungwoo, > > On Mon, May 29, 2017 at 8:26 AM, Yang, Hyungwoo <hyungwoo.yang@xxxxxxxxx> wrote: > > > > Hi Sakari, > > > > Here's my comments. > > > > -Hyungwoo > > > > > > -----Original Message----- > >> From: Sakari Ailus [mailto:sakari.ailus@xxxxxx] > >> Sent: Saturday, May 27, 2017 1:31 PM > >> To: Yang, Hyungwoo <hyungwoo.yang@xxxxxxxxx> > >> Cc: linux-media@xxxxxxxxxxxxxxx; sakari.ailus@xxxxxxxxxxxxxxx; Zheng, > >> Jian Xu <jian.xu.zheng@xxxxxxxxx>; Hsu, Cedric > >> <cedric.hsu@xxxxxxxxx>; tfiga@xxxxxxxxxxxx > >> Subject: Re: [PATCH v3 1/1] [media] i2c: add support for OV13858 > >> sensor > >> > >> Hi Hyungwoo, > >> > >> Thanks for the update. A few comments below. > >> > [snip] > >> > +/* Update VTS that meets expected vertical blanking */ static int > >> > +ov13858_update_vblank(struct ov13858 *ov13858, > >> > + struct v4l2_ctrl *ctrl) { > >> > + return ov13858_write_reg( > >> > + ov13858, OV13858_REG_VTS, > >> > + OV13858_REG_VALUE_16BIT, > >> > + ov13858->cur_mode->height + > >> > +ov13858->vblank->val); } > >> > + > >> > +/* Update analog gain */ > >> > +static int ov13858_update_analog_gain(struct ov13858 *ov13858, > >> > + struct v4l2_ctrl *ctrl) { > >> > + return ov13858_write_reg(ov13858, OV13858_REG_ANALOG_GAIN, > >> > + OV13858_REG_VALUE_16BIT, ctrl->val); > >> > >> I think I'd move what the four above functions do to ov13858_set_ctrl() unless they're used in more than one location. > > > > Why ? Personally I like this. Since there wouldn't be any difference in generated machine code, I want to keep this if there's no strict rule on this. > > Personally I wouldn't probably care about this, but I see one advantage of Sakari's suggestion. > > Namely, it improves code readability, because there is less indirection and the person reading ov13858_set_ctrl() instantly knows that all it does is directly writing the control value to hardware registers. Otherwise, with the indirection in current version, until you read ov13858_update_analog_gain() (or such), you don't know whether it does some extra processing, power management or whatnot. > > If ov13858_update_analog_gain() did more than just a simple register write, it would indeed make sense to separate it, as it's intuitive that a separate function means some more complicated work. (And vice versa, it's counter-intuitive to have a function that is only there to call a register accessor.) > This is my habit for people who doesn't have datasheet for h/w or people who doesn't need(want) to know about detail. Yeah, my habit is especially for those h/w which have many bit-fields in a register and I believe this kind of separation helps these people. I know the registers in this sensor is very much straightforward. > > > >> > >> > +} > >> > + > >> > +static int ov13858_set_ctrl(struct v4l2_ctrl *ctrl) { > >> > + struct ov13858 *ov13858 = container_of(ctrl->handler, > >> > + struct ov13858, ctrl_handler); > >> > + struct i2c_client *client = v4l2_get_subdevdata(&ov13858->sd); > >> > + int ret; > >> > + > >> > + /* Propagate change of current control to all related controls */ > >> > + switch (ctrl->id) { > >> > + case V4L2_CID_VBLANK: > >> > + ov13858_update_exposure_limits(ov13858); > >> > + break; > >> > + }; > >> > + > >> > + /* > >> > + * Applying V4L2 control value only happens > >> > + * when power is up for streaming > >> > + */ > >> > + if (pm_runtime_get_if_in_use(&client->dev) <= 0) > >> > + return 0; > >> > + > >> > + 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; > >> > + case V4L2_CID_VBLANK: > >> > + ret = ov13858_update_vblank(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; > >> > +} > >> > + > > : > > : > >> > +/* > >> > + * Prepare streaming by writing default values and customized values. > >> > + * This should be called with ov13858->mutex acquired. > >> > + */ > >> > +static int ov13858_prepare_streaming(struct ov13858 *ov13858) { > >> > + struct i2c_client *client = v4l2_get_subdevdata(&ov13858->sd); > >> > + const struct ov13858_reg_list *reg_list; > >> > + int ret, link_freq_index; > >> > + > >> > + /* Get out of from software reset */ > >> > + ret = ov13858_write_reg(ov13858, OV13858_REG_SOFTWARE_RST, > >> > + OV13858_REG_VALUE_08BIT, OV13858_SOFTWARE_RST); > >> > + if (ret) { > >> > + dev_err(&client->dev, "%s failed to set powerup registers\n", > >> > + __func__); > >> > + return ret; > >> > + } > >> > + > >> > + /* Setup PLL */ > >> > + link_freq_index = ov13858->cur_mode->link_freq_index; > >> > + reg_list = &link_freq_configs[link_freq_index].reg_list; > >> > + ret = ov13858_write_reg_list(ov13858, reg_list); > >> > + if (ret) { > >> > + dev_err(&client->dev, "%s failed to set plls\n", __func__); > >> > + return ret; > >> > + } > >> > + > >> > + /* Apply default values of current mode */ > >> > + reg_list = &ov13858->cur_mode->reg_list; > >> > + ret = ov13858_write_reg_list(ov13858, reg_list); > >> > + if (ret) { > >> > + dev_err(&client->dev, "%s failed to set mode\n", __func__); > >> > + return ret; > >> > + } > >> > + > >> > + /* Apply customized values from user */ > >> > + return __v4l2_ctrl_handler_setup(ov13858->sd.ctrl_handler); > >> > +} > >> > + > >> > +/* Start streaming */ > >> > +static int ov13858_start_streaming(struct ov13858 *ov13858) { > >> > + int ret; > >> > + > >> > + /* Write default & customized values */ > >> > + ret = ov13858_prepare_streaming(ov13858); > >> > >> Could you merge this with ov13858_prepare_streaming()? > >> > > > > Why ? I want to keep this. If you want to worry about 1 more jump then, if it is really there, I can make this function "inline" > > I doubt it's about the number of jumps. The same argument of code readability as I mentioned above applies here as well. I see no point in having ov13858_start_streaming() separate if all it does on top of > ov13858_prepare_streaming() is a register write, it is counter-intuitive for readers of the code. Here, it's the same. I believe this kind of code very much readable for people who doesn't have much experiences(or dummies like me) and who just wants to know control flow(not detail about h/w). > > Best regards, > Tomasz
