Re: [PATCH 0/4] fix some checkpatch style issues in atomisp driver

[jacopo mondi <jacopo@jmondi.org>]

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