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.) > >> >> > +} >> > + >> > +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. Best regards, Tomasz
