Hello Riccardo,
On Wed, Nov 29, 2017 at 10:31:17AM +0100, Riccardo S. wrote:
> Hi Jacopo,
>
> for some reason your comment about "[PATCH 3/4] staging: improves
> comparisons readability in atomisp-ov5693" did not reach my inbox.
>
> Unfortunately I already sent PATCHv2 and it has been accepted.
> Anyway...
No worries!
>
> > > @@ -780,7 +780,7 @@ static int __ov5693_otp_read(struct v4l2_subdev *sd,
> > > u8 *buf)
> > > b = buf;
> > > continue;
> > > }
> > > - } else if (27 == i) { //if the prvious 32bytes data
> > > doesn't exist, try to read the next 32bytes data again.
> > > + } else if (i == 27) { //if the prvious 32bytes data
> > > doesn't exist, try to read the next 32bytes data again.
> >
> > I wonder why checkpatch does not complain about these C++ style
> > comments clearly exceeding 80 columns...
> >
>
> It complained, but I didn't put that fix in this series. Should I have
> cleaned those lines in the same commit since I was already touching
> that part of the code? Or better in a separate patch?
As you wish.. This is a cleanup series, and fixing comments is
really a minor issues, so if you like to change them in this single
patch you can do that, imo, and mention it in the commit message:
"Fix C++ style comments exceeding 80 columns while at there."
or similar.
>
> > > if ((*b) == 0) {
> > > dev->otp_size = 32;
> > > break;
> > > @@ -1351,7 +1351,7 @@ static int __power_up(struct v4l2_subdev *sd)
> > > struct i2c_client *client = v4l2_get_subdevdata(sd);
> > > int ret;
> > >
> > > - if (NULL == dev->platform_data) {
> > > + if (!dev->platform_data) {
>
> > Please mention in changelog that you're also substituting a comparison to
> > NULL with this.
> >
> > Checkpatch points this out, didn't it?
>
> It actually warned about the comparison that should place the constant
> on the right side of the test. When fixing this, I used the "!foo"
> syntax. I got your point though.
Oh, ok, I thought it gave you back a different warning for
comparisons with NULL!
>
> Thanks for your review!
You're welcome!
Cheers
j
>
>
> Riccardo Schirone
