Hi Tomasz, I left my comments.
Thanks, Hyungwoo -----Original Message----- > From: Tomasz Figa [mailto:[email protected]] > Sent: Monday, May 29, 2017 12:35 AM > To: Yang, Hyungwoo <[email protected]> > Cc: Sakari Ailus <[email protected]>; [email protected]; > [email protected]; Zheng, Jian Xu <[email protected]>; Hsu, > Cedric <[email protected]> > Subject: Re: [PATCH v3 1/1] [media] i2c: add support for OV13858 sensor > > On Mon, May 29, 2017 at 3:49 PM, Yang, Hyungwoo <[email protected]> > wrote: > > > > Hello Tomasz, > > > > Here's my comments. > > > > Thanks, > > Hyungwoo > > > > -----Original Message----- > >> From: Tomasz Figa [mailto:[email protected]] > >> Sent: Sunday, May 28, 2017 7:56 PM > >> To: Yang, Hyungwoo <[email protected]> > >> Cc: Sakari Ailus <[email protected]>; [email protected]; > >> [email protected]; Zheng, Jian Xu > >> <[email protected]>; Hsu, Cedric <[email protected]> > >> 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 <[email protected]> > >> wrote: > >> > > >> > Hi Sakari, > >> > > >> > Here's my comments. > >> > > >> > -Hyungwoo > >> > > >> > > >> > -----Original Message----- > >> >> From: Sakari Ailus [mailto:[email protected]] > >> >> Sent: Saturday, May 27, 2017 1:31 PM > >> >> To: Yang, Hyungwoo <[email protected]> > >> >> Cc: [email protected]; [email protected]; > >> >> Zheng, Jian Xu <[email protected]>; Hsu, Cedric > >> >> <[email protected]>; [email protected] > >> >> 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 >
