Hi Tomasz, I left my comments. Thanks, Hyungwoo -----Original Message----- > From: Tomasz Figa [mailto:tfiga@xxxxxxxxxxxx] > Sent: Monday, May 29, 2017 12:35 AM > 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 > > On Mon, May 29, 2017 at 3:49 PM, Yang, Hyungwoo <hyungwoo.yang@xxxxxxxxx> wrote: > > > > 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. > > I still think it doesn't really add any value. If the code is organized well, i.e. no forward declarations, callees always above callers, then you can simply read the code from top to bottom and have exactly the same understanding of it without having the datasheet, because you can see what the code is supposed to do, in this case you would get to ov13858_set_ctrl(), which is clear to be the function to set a control, then to the switch construct and then to particular switch case and at this point it's already clear that you want to set analog gain or whatever given the V4L2_CID_* enum. So having a function named in exactly the same way (ov13858_update_analog_gain()) is just redundant and actually takes the information about what's going on inside away from the reader. First, although I believe this is nothing related with "forward declaration" or "not easy to to read code", I WILL MOVE the code. As I said, I know the registers in this sensor is very straightforward so people can think it's redundant. For me, it's more like you have macro or not to have different look. > > Best regards, > Tomasz >
