Re: [PATCH 1/2] staging:iio:adc:added space around '-'

[Joe Perches]

On Sat, 2016-01-30 at 18:12 +0300, Dan Carpenter wrote:
> We could make checkpatch.pl not complain if the line says checkpatch:
> on
> it.  It would look like this.
> 
> -   in_voltage-voltage_thresh_low_value,
> +   in_voltage-voltage_thresh_low_value, /* checkpatch:
> not math */
> 
> I suppose I could have made the explanation longer since the it won't
> complain about the 80 character limit...  What do yo/u guys think?

Maybe use a more generic thing like the checkpatch type

               in_voltage-voltage_thresh_low_value, /* checkpatch-SPACING */

Even so, it might uglify checkpatch code a lot to
check something like this per-line or per-block.

And that likely would have to be per line in the
code as checkpatch couldn't see when a patch block
addition occurs outside the scope of a comment.

I suppose inside checkpatch the "sub report {"
function could be extended to look at the specific
$rawline being tested for any "checkpatch" comment
and if so, test if it's the specific $type.

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] staging:iio:adc:added space around '-'

[Jonathan Cameron]

On 24/01/16 17:14, Lars-Peter Clausen wrote:
> On 01/24/2016 05:36 PM, Jonathan Cameron wrote:
>> On 20/01/16 14:21, Dan Carpenter wrote:
>>> On Fri, Jan 15, 2016 at 09:15:52PM +0100, Lars-Peter Clausen wrote:
 On 01/15/2016 08:42 PM, Bhumika Goyal wrote:
> This patch adds apace around '-' operator.Found using checkpatch.pl
>
> Signed-off-by: Bhumika Goyal 
> ---
>  drivers/staging/iio/adc/ad7280a.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/iio/adc/ad7280a.c 
> b/drivers/staging/iio/adc/ad7280a.c
> index f45ebed..0c73bce 100644
> --- a/drivers/staging/iio/adc/ad7280a.c
> +++ b/drivers/staging/iio/adc/ad7280a.c
> @@ -744,14 +744,14 @@ out:
>  }
>  
>  static IIO_DEVICE_ATTR_NAMED(in_thresh_low_value,
> - in_voltage-voltage_thresh_low_value,
> + in_voltage - voltage_thresh_low_value,

 Hi,

 Thanks for patch. But when sending cleanup patches like this please make
 sure that you a) understand what the code does and how your change affects
 it and b) as a bare minimum of testing perform a compile test, if possible
 also do functional testing.

 The patch as it is, is neither semantically nor syntactically correct. As 
 an
 exercise please make sure you understand why.
>>>
>>> Ugh!
>>>
>>> It took me a long time to figure out the bug in this patch...  Why does
>>> that filename have a mix of dashes and underscores?  Too late to fix it
>>> now...  :/
>>>
>> Very deliberately.  The - is indicating it is a differential channel!
>> Literally A minus B.
>>
>> It's an awfully compact representation for maths ;)
>> This is obscured partly in this case as it's specifying an attribute
>> shared by a set of differential channels so it's the generalization
>> of
>> in_voltage0-voltage1_thresh_low_value
>> which does begin to slightly stretch the argument that it is nice and
>> clear ;(
> 
> One thing we should maybe take a look at is making it explicit that this is
> a string so it does not get picked up by checkpatch.
Make sense. Patches welcome :)
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] staging:iio:adc:added space around '-'

[Dan Carpenter]

We could make checkpatch.pl not complain if the line says checkpatch: on
it.  It would look like this.

-   in_voltage-voltage_thresh_low_value,
+   in_voltage-voltage_thresh_low_value, /* checkpatch: not math */

I suppose I could have made the explanation longer since the it won't
complain about the 80 character limit...  What do you guys think?

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] staging:iio:adc:added space around '-'

[Lars-Peter Clausen]

On 01/30/2016 04:12 PM, Dan Carpenter wrote:
> We could make checkpatch.pl not complain if the line says checkpatch: on
> it.  It would look like this.
> 
> -   in_voltage-voltage_thresh_low_value,
> +   in_voltage-voltage_thresh_low_value, /* checkpatch: not math 
> */
> 
> I suppose I could have made the explanation longer since the it won't
> complain about the 80 character limit...  What do you guys think?

We could add it as a temporary way to silence the checker. But it feels a
bit ugly, there is really no reason why this shouldn't be a string, other
than that the current device attr macros don't support that.

- Lars

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] staging:iio:adc:added space around '-'

[Jonathan Cameron]

On 20/01/16 14:21, Dan Carpenter wrote:
> On Fri, Jan 15, 2016 at 09:15:52PM +0100, Lars-Peter Clausen wrote:
>> On 01/15/2016 08:42 PM, Bhumika Goyal wrote:
>>> This patch adds apace around '-' operator.Found using checkpatch.pl
>>>
>>> Signed-off-by: Bhumika Goyal 
>>> ---
>>>  drivers/staging/iio/adc/ad7280a.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/staging/iio/adc/ad7280a.c 
>>> b/drivers/staging/iio/adc/ad7280a.c
>>> index f45ebed..0c73bce 100644
>>> --- a/drivers/staging/iio/adc/ad7280a.c
>>> +++ b/drivers/staging/iio/adc/ad7280a.c
>>> @@ -744,14 +744,14 @@ out:
>>>  }
>>>  
>>>  static IIO_DEVICE_ATTR_NAMED(in_thresh_low_value,
>>> -   in_voltage-voltage_thresh_low_value,
>>> +   in_voltage - voltage_thresh_low_value,
>>
>> Hi,
>>
>> Thanks for patch. But when sending cleanup patches like this please make
>> sure that you a) understand what the code does and how your change affects
>> it and b) as a bare minimum of testing perform a compile test, if possible
>> also do functional testing.
>>
>> The patch as it is, is neither semantically nor syntactically correct. As an
>> exercise please make sure you understand why.
> 
> Ugh!
> 
> It took me a long time to figure out the bug in this patch...  Why does
> that filename have a mix of dashes and underscores?  Too late to fix it
> now...  :/
> 
Very deliberately.  The - is indicating it is a differential channel!
Literally A minus B.

It's an awfully compact representation for maths ;)
This is obscured partly in this case as it's specifying an attribute
shared by a set of differential channels so it's the generalization
of
in_voltage0-voltage1_thresh_low_value
which does begin to slightly stretch the argument that it is nice and
clear ;(

Jonathan
> regards,
> dan carpenter
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] staging:iio:adc:added space around '-'

[Lars-Peter Clausen]

On 01/24/2016 05:36 PM, Jonathan Cameron wrote:
> On 20/01/16 14:21, Dan Carpenter wrote:
>> On Fri, Jan 15, 2016 at 09:15:52PM +0100, Lars-Peter Clausen wrote:
>>> On 01/15/2016 08:42 PM, Bhumika Goyal wrote:
 This patch adds apace around '-' operator.Found using checkpatch.pl

 Signed-off-by: Bhumika Goyal 
 ---
  drivers/staging/iio/adc/ad7280a.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

 diff --git a/drivers/staging/iio/adc/ad7280a.c 
 b/drivers/staging/iio/adc/ad7280a.c
 index f45ebed..0c73bce 100644
 --- a/drivers/staging/iio/adc/ad7280a.c
 +++ b/drivers/staging/iio/adc/ad7280a.c
 @@ -744,14 +744,14 @@ out:
  }
  
  static IIO_DEVICE_ATTR_NAMED(in_thresh_low_value,
 -  in_voltage-voltage_thresh_low_value,
 +  in_voltage - voltage_thresh_low_value,
>>>
>>> Hi,
>>>
>>> Thanks for patch. But when sending cleanup patches like this please make
>>> sure that you a) understand what the code does and how your change affects
>>> it and b) as a bare minimum of testing perform a compile test, if possible
>>> also do functional testing.
>>>
>>> The patch as it is, is neither semantically nor syntactically correct. As an
>>> exercise please make sure you understand why.
>>
>> Ugh!
>>
>> It took me a long time to figure out the bug in this patch...  Why does
>> that filename have a mix of dashes and underscores?  Too late to fix it
>> now...  :/
>>
> Very deliberately.  The - is indicating it is a differential channel!
> Literally A minus B.
> 
> It's an awfully compact representation for maths ;)
> This is obscured partly in this case as it's specifying an attribute
> shared by a set of differential channels so it's the generalization
> of
> in_voltage0-voltage1_thresh_low_value
> which does begin to slightly stretch the argument that it is nice and
> clear ;(

One thing we should maybe take a look at is making it explicit that this is
a string so it does not get picked up by checkpatch.

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] staging:iio:adc:added space around '-'

[Dan Carpenter]

On Fri, Jan 15, 2016 at 09:15:52PM +0100, Lars-Peter Clausen wrote:
> On 01/15/2016 08:42 PM, Bhumika Goyal wrote:
> > This patch adds apace around '-' operator.Found using checkpatch.pl
> > 
> > Signed-off-by: Bhumika Goyal 
> > ---
> >  drivers/staging/iio/adc/ad7280a.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/adc/ad7280a.c 
> > b/drivers/staging/iio/adc/ad7280a.c
> > index f45ebed..0c73bce 100644
> > --- a/drivers/staging/iio/adc/ad7280a.c
> > +++ b/drivers/staging/iio/adc/ad7280a.c
> > @@ -744,14 +744,14 @@ out:
> >  }
> >  
> >  static IIO_DEVICE_ATTR_NAMED(in_thresh_low_value,
> > -   in_voltage-voltage_thresh_low_value,
> > +   in_voltage - voltage_thresh_low_value,
> 
> Hi,
> 
> Thanks for patch. But when sending cleanup patches like this please make
> sure that you a) understand what the code does and how your change affects
> it and b) as a bare minimum of testing perform a compile test, if possible
> also do functional testing.
> 
> The patch as it is, is neither semantically nor syntactically correct. As an
> exercise please make sure you understand why.

Ugh!

It took me a long time to figure out the bug in this patch...  Why does
that filename have a mix of dashes and underscores?  Too late to fix it
now...  :/

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] staging:iio:adc:added space around '-'

[Lars-Peter Clausen]

On 01/15/2016 08:42 PM, Bhumika Goyal wrote:
> This patch adds apace around '-' operator.Found using checkpatch.pl
> 
> Signed-off-by: Bhumika Goyal 
> ---
>  drivers/staging/iio/adc/ad7280a.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7280a.c 
> b/drivers/staging/iio/adc/ad7280a.c
> index f45ebed..0c73bce 100644
> --- a/drivers/staging/iio/adc/ad7280a.c
> +++ b/drivers/staging/iio/adc/ad7280a.c
> @@ -744,14 +744,14 @@ out:
>  }
>  
>  static IIO_DEVICE_ATTR_NAMED(in_thresh_low_value,
> - in_voltage-voltage_thresh_low_value,
> + in_voltage - voltage_thresh_low_value,

Hi,

Thanks for patch. But when sending cleanup patches like this please make
sure that you a) understand what the code does and how your change affects
it and b) as a bare minimum of testing perform a compile test, if possible
also do functional testing.

The patch as it is, is neither semantically nor syntactically correct. As an
exercise please make sure you understand why.

Same for the second patch.

- Lars
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel