Re: [PATCH 0/4] fix some checkpatch style issues in atomisp driver
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
Re: [PATCH 0/4] fix some checkpatch style issues in atomisp driver
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...
> > @@ -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?
> > 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.
Thanks for your review!
Riccardo Schirone
[PATCH 0/4] fix some checkpatch style issues in atomisp driver
This patch series fixes some coding style issues reported by checkpatch. It is based on next-20171127. Riccardo Schirone (4): staging: add missing blank line after declarations in atomisp-ov5693 staging: improve comments usage in atomisp-ov5693 staging: improves comparisons readability in atomisp-ov5693 staging: fix indentation in atomisp-ov5693 .../media/atomisp/i2c/ov5693/atomisp-ov5693.c | 63 +++--- 1 file changed, 44 insertions(+), 19 deletions(-) -- 2.14.3
