Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1322372 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/2] staging:iio:adc:added space around '-' Jonathan Cameron <jic23@kernel.org> - 2016-01-30 15:20 +0100
    Re: [PATCH 1/2] staging:iio:adc:added space around '-' Dan Carpenter <dan.carpenter@oracle.com> - 2016-01-30 16:20 +0100
      Re: [PATCH 1/2] staging:iio:adc:added space around '-' Lars-Peter Clausen <lars@metafoo.de> - 2016-01-30 16:30 +0100
      Re: [PATCH 1/2] staging:iio:adc:added space around '-' Joe Perches <joe@perches.com> - 2016-01-30 18:10 +0100

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

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 <bhumirks@gmail.com>
>>>>> ---
>>>>>  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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[toc] | [next] | [standalone]

#1322386

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

[toc] | [prev] | [next] | [standalone]

#1322390

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

[toc] | [prev] | [next] | [standalone]

#1322435

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web