[PATCH] Staging: iio: adc: Added Space around binary op.

[PATCH] Staging: iio: adc: Added Space around binary op.

[Himanshi Jain]

Added space around(one on each side of) binary
operator(-) as preferred according to kernel
coding style.

Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com>
---
 drivers/staging/iio/adc/ad7192.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/iio/adc/ad7192.c b/drivers/staging/iio/adc/ad7192.c
index d11c6de..1aee662 100644
--- a/drivers/staging/iio/adc/ad7192.c
+++ b/drivers/staging/iio/adc/ad7192.c
@@ -341,7 +341,7 @@ static int ad7192_setup(struct ad7192_state *st,
 }
 
 static IIO_DEVICE_ATTR_NAMED(in_v_m_v_scale_available,
-			     in_voltage-voltage_scale_available,
+			     in_voltage - voltage_scale_available,
 			     0444, ad7192_show_scale_available, NULL, 0);
 
 static IIO_DEVICE_ATTR(in_voltage_scale_available, 0444,
-- 
1.9.1

Re: [Outreachy kernel] [PATCH] Staging: iio: adc: Added Space around binary op.

[Julia Lawall]

On Fri, 8 Sep 2017, Himanshi Jain wrote:

> Added space around(one on each side of) binary

I think that just around would be clear enough.

In the previous patches on this file, found with git log --oneline, a
subject line of staging: iio: ad7192: seems to be more popular when the
patch affects only this file.  But maybe it doesn't matter either way.

julia

> operator(-) as preferred according to kernel
> coding style.
>
> Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com>
> ---
>  drivers/staging/iio/adc/ad7192.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/iio/adc/ad7192.c b/drivers/staging/iio/adc/ad7192.c
> index d11c6de..1aee662 100644
> --- a/drivers/staging/iio/adc/ad7192.c
> +++ b/drivers/staging/iio/adc/ad7192.c
> @@ -341,7 +341,7 @@ static int ad7192_setup(struct ad7192_state *st,
>  }
>
>  static IIO_DEVICE_ATTR_NAMED(in_v_m_v_scale_available,
> -			     in_voltage-voltage_scale_available,
> +			     in_voltage - voltage_scale_available,
>  			     0444, ad7192_show_scale_available, NULL, 0);
>
>  static IIO_DEVICE_ATTR(in_voltage_scale_available, 0444,
> --
> 1.9.1
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20170908044752.GA6199%40himanshi-Inspiron-5558.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [PATCH] Staging: iio: adc: Added Space around binary op.

[Jonathan Cameron]

On 8 September 2017 05:47:52 BST, Himanshi Jain <himshijain.hj@gmail.com> wrote:
>Added space around(one on each side of) binary
>operator(-) as preferred according to kernel
>coding style.
>
>Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com>

Take a closer look at that macro. It isn't doing what you think...  To give a hint, changing this breaks userspace.

Jonathan


>---
> drivers/staging/iio/adc/ad7192.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/staging/iio/adc/ad7192.c
>b/drivers/staging/iio/adc/ad7192.c
>index d11c6de..1aee662 100644
>--- a/drivers/staging/iio/adc/ad7192.c
>+++ b/drivers/staging/iio/adc/ad7192.c
>@@ -341,7 +341,7 @@ static int ad7192_setup(struct ad7192_state *st,
> }
> 
> static IIO_DEVICE_ATTR_NAMED(in_v_m_v_scale_available,
>-			     in_voltage-voltage_scale_available,
>+			     in_voltage - voltage_scale_available,
> 			     0444, ad7192_show_scale_available, NULL, 0);
> 
> static IIO_DEVICE_ATTR(in_voltage_scale_available, 0444,

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH] Staging: iio: adc: Added Space around binary op.

[Jonathan Cameron]

On Fri, 8 Sep 2017 07:29:06 +0100
Jonathan Cameron <jic23@jic23.retrosnub.co.uk> wrote:

> On 8 September 2017 05:47:52 BST, Himanshi Jain <himshijain.hj@gmail.com> wrote:
> >Added space around(one on each side of) binary
> >operator(-) as preferred according to kernel
> >coding style.
> >
> >Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com>  
> 
> Take a closer look at that macro. It isn't doing what you think...  
> To give a hint, changing this breaks userspace.

Ok, I'm bored of this particular one coming up. When you have
worked out what is going on Himanshi, would you mind putting
together a patch adding a comment describing why it is a bad
idea to 'fix' this?  That would be a very useful patch as
far as I'm concerned :)

There aren't that many cases of this in IIO so adding a comment
on each of them is probably reasonable just to avoid wasting
people's time on fixing them! (I think we have had more than
5 such goes this year so far...)

Jonathan

> 
> Jonathan
> 
> 
> >---
> > drivers/staging/iio/adc/ad7192.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/drivers/staging/iio/adc/ad7192.c
> >b/drivers/staging/iio/adc/ad7192.c
> >index d11c6de..1aee662 100644
> >--- a/drivers/staging/iio/adc/ad7192.c
> >+++ b/drivers/staging/iio/adc/ad7192.c
> >@@ -341,7 +341,7 @@ static int ad7192_setup(struct ad7192_state *st,
> > }
> > 
> > static IIO_DEVICE_ATTR_NAMED(in_v_m_v_scale_available,
> >-			     in_voltage-voltage_scale_available,
> >+			     in_voltage - voltage_scale_available,
> > 			     0444, ad7192_show_scale_available, NULL, 0);
> > 
> > static IIO_DEVICE_ATTR(in_voltage_scale_available, 0444,  
> 

Re: [PATCH] Staging: iio: adc: Added Space around binary op.

[Lars-Peter Clausen]

On 09/08/2017 11:32 AM, Jonathan Cameron wrote:
> On Fri, 8 Sep 2017 07:29:06 +0100
> Jonathan Cameron <jic23@jic23.retrosnub.co.uk> wrote:
> 
>> On 8 September 2017 05:47:52 BST, Himanshi Jain <himshijain.hj@gmail.com> wrote:
>>> Added space around(one on each side of) binary
>>> operator(-) as preferred according to kernel
>>> coding style.
>>>
>>> Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com>  
>>
>> Take a closer look at that macro. It isn't doing what you think...  
>> To give a hint, changing this breaks userspace.
> 
> Ok, I'm bored of this particular one coming up. When you have
> worked out what is going on Himanshi, would you mind putting
> together a patch adding a comment describing why it is a bad
> idea to 'fix' this?  That would be a very useful patch as
> far as I'm concerned :)
> 
> There aren't that many cases of this in IIO so adding a comment
> on each of them is probably reasonable just to avoid wasting
> people's time on fixing them! (I think we have had more than
> 5 such goes this year so far...)
> 

I'd much rather fix this API so that you have to put "" around the name so
that it is clear that it is a string, rather than doing the implicit
conversion to string using preprocessor magic. Better to have a
self-documenting API then having to add a comment to each user of the API.

> Jonathan
> 
>>
>> Jonathan
>>
>>
>>> ---
>>> drivers/staging/iio/adc/ad7192.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/staging/iio/adc/ad7192.c
>>> b/drivers/staging/iio/adc/ad7192.c
>>> index d11c6de..1aee662 100644
>>> --- a/drivers/staging/iio/adc/ad7192.c
>>> +++ b/drivers/staging/iio/adc/ad7192.c
>>> @@ -341,7 +341,7 @@ static int ad7192_setup(struct ad7192_state *st,
>>> }
>>>
>>> static IIO_DEVICE_ATTR_NAMED(in_v_m_v_scale_available,
>>> -			     in_voltage-voltage_scale_available,
>>> +			     in_voltage - voltage_scale_available,
>>> 			     0444, ad7192_show_scale_available, NULL, 0);
>>>
>>> static IIO_DEVICE_ATTR(in_voltage_scale_available, 0444,  
>>
> 

Re: [PATCH] Staging: iio: adc: Added Space around binary op.

[Jonathan Cameron]

On Fri, 8 Sep 2017 11:37:56 +0200
Lars-Peter Clausen <lars@metafoo.de> wrote:

> On 09/08/2017 11:32 AM, Jonathan Cameron wrote:
> > On Fri, 8 Sep 2017 07:29:06 +0100
> > Jonathan Cameron <jic23@jic23.retrosnub.co.uk> wrote:
> >   
> >> On 8 September 2017 05:47:52 BST, Himanshi Jain <himshijain.hj@gmail.com> wrote:  
> >>> Added space around(one on each side of) binary
> >>> operator(-) as preferred according to kernel
> >>> coding style.
> >>>
> >>> Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com>    
> >>
> >> Take a closer look at that macro. It isn't doing what you think...  
> >> To give a hint, changing this breaks userspace.  
> > 
> > Ok, I'm bored of this particular one coming up. When you have
> > worked out what is going on Himanshi, would you mind putting
> > together a patch adding a comment describing why it is a bad
> > idea to 'fix' this?  That would be a very useful patch as
> > far as I'm concerned :)
> > 
> > There aren't that many cases of this in IIO so adding a comment
> > on each of them is probably reasonable just to avoid wasting
> > people's time on fixing them! (I think we have had more than
> > 5 such goes this year so far...)
> >   
> 
> I'd much rather fix this API so that you have to put "" around the name so
> that it is clear that it is a string, rather than doing the implicit
> conversion to string using preprocessor magic. Better to have a
> self-documenting API then having to add a comment to each user of the API.

Good point.

So Himanshi, feel like taking this on?

Jonathan

> 
> > Jonathan
> >   
> >>
> >> Jonathan
> >>
> >>  
> >>> ---
> >>> drivers/staging/iio/adc/ad7192.c | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/staging/iio/adc/ad7192.c
> >>> b/drivers/staging/iio/adc/ad7192.c
> >>> index d11c6de..1aee662 100644
> >>> --- a/drivers/staging/iio/adc/ad7192.c
> >>> +++ b/drivers/staging/iio/adc/ad7192.c
> >>> @@ -341,7 +341,7 @@ static int ad7192_setup(struct ad7192_state *st,
> >>> }
> >>>
> >>> static IIO_DEVICE_ATTR_NAMED(in_v_m_v_scale_available,
> >>> -			     in_voltage-voltage_scale_available,
> >>> +			     in_voltage - voltage_scale_available,
> >>> 			     0444, ad7192_show_scale_available, NULL, 0);
> >>>
> >>> static IIO_DEVICE_ATTR(in_voltage_scale_available, 0444,    
> >>  
> >   
> 

Re: [Outreachy kernel] Re: [PATCH] Staging: iio: adc: Added Space around binary op.

[Julia Lawall]

On Fri, 8 Sep 2017, Lars-Peter Clausen wrote:

> On 09/08/2017 11:32 AM, Jonathan Cameron wrote:
> > On Fri, 8 Sep 2017 07:29:06 +0100
> > Jonathan Cameron <jic23@jic23.retrosnub.co.uk> wrote:
> >
> >> On 8 September 2017 05:47:52 BST, Himanshi Jain <himshijain.hj@gmail.com> wrote:
> >>> Added space around(one on each side of) binary
> >>> operator(-) as preferred according to kernel
> >>> coding style.
> >>>
> >>> Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com>
> >>
> >> Take a closer look at that macro. It isn't doing what you think...
> >> To give a hint, changing this breaks userspace.
> >
> > Ok, I'm bored of this particular one coming up. When you have
> > worked out what is going on Himanshi, would you mind putting
> > together a patch adding a comment describing why it is a bad
> > idea to 'fix' this?  That would be a very useful patch as
> > far as I'm concerned :)
> >
> > There aren't that many cases of this in IIO so adding a comment
> > on each of them is probably reasonable just to avoid wasting
> > people's time on fixing them! (I think we have had more than
> > 5 such goes this year so far...)
> >
>
> I'd much rather fix this API so that you have to put "" around the name so
> that it is clear that it is a string, rather than doing the implicit
> conversion to string using preprocessor magic. Better to have a
> self-documenting API then having to add a comment to each user of the API.

All the DEVICE_ATTR macros use the same strategy.  And the non-IIO ones,
eg DEVICE_ATTR_RW, also use the provided name to construct the names of
the show and store functions, so I don't think a string would be
appropriate.

julia

>
> > Jonathan
> >
> >>
> >> Jonathan
> >>
> >>
> >>> ---
> >>> drivers/staging/iio/adc/ad7192.c | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/staging/iio/adc/ad7192.c
> >>> b/drivers/staging/iio/adc/ad7192.c
> >>> index d11c6de..1aee662 100644
> >>> --- a/drivers/staging/iio/adc/ad7192.c
> >>> +++ b/drivers/staging/iio/adc/ad7192.c
> >>> @@ -341,7 +341,7 @@ static int ad7192_setup(struct ad7192_state *st,
> >>> }
> >>>
> >>> static IIO_DEVICE_ATTR_NAMED(in_v_m_v_scale_available,
> >>> -			     in_voltage-voltage_scale_available,
> >>> +			     in_voltage - voltage_scale_available,
> >>> 			     0444, ad7192_show_scale_available, NULL, 0);
> >>>
> >>> static IIO_DEVICE_ATTR(in_voltage_scale_available, 0444,
> >>
> >
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/2e3476f0-9e7e-2701-220a-5fb178d68d2e%40metafoo.de.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [Outreachy kernel] Re: [PATCH] Staging: iio: adc: Added Space around binary op.

[Lars-Peter Clausen]

On 09/08/2017 11:59 AM, Julia Lawall wrote:
> 
> 
> On Fri, 8 Sep 2017, Lars-Peter Clausen wrote:
> 
>> On 09/08/2017 11:32 AM, Jonathan Cameron wrote:
>>> On Fri, 8 Sep 2017 07:29:06 +0100
>>> Jonathan Cameron <jic23@jic23.retrosnub.co.uk> wrote:
>>>
>>>> On 8 September 2017 05:47:52 BST, Himanshi Jain <himshijain.hj@gmail.com> wrote:
>>>>> Added space around(one on each side of) binary
>>>>> operator(-) as preferred according to kernel
>>>>> coding style.
>>>>>
>>>>> Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com>
>>>>
>>>> Take a closer look at that macro. It isn't doing what you think...
>>>> To give a hint, changing this breaks userspace.
>>>
>>> Ok, I'm bored of this particular one coming up. When you have
>>> worked out what is going on Himanshi, would you mind putting
>>> together a patch adding a comment describing why it is a bad
>>> idea to 'fix' this?  That would be a very useful patch as
>>> far as I'm concerned :)
>>>
>>> There aren't that many cases of this in IIO so adding a comment
>>> on each of them is probably reasonable just to avoid wasting
>>> people's time on fixing them! (I think we have had more than
>>> 5 such goes this year so far...)
>>>
>>
>> I'd much rather fix this API so that you have to put "" around the name so
>> that it is clear that it is a string, rather than doing the implicit
>> conversion to string using preprocessor magic. Better to have a
>> self-documenting API then having to add a comment to each user of the API.
> 
> All the DEVICE_ATTR macros use the same strategy.  And the non-IIO ones,
> eg DEVICE_ATTR_RW, also use the provided name to construct the names of
> the show and store functions, so I don't think a string would be
> appropriate.

I'm only suggesting to use a string for the _NAMED macros where the name is
not used to construct the identifiers.

In the case where the name is used to construct the identifiers we don't
have the issue with false positives since the name must be a valid
identifier on its own.

Re: [Outreachy kernel] Re: [PATCH] Staging: iio: adc: Added Space around binary op.

[Julia Lawall]

On Fri, 8 Sep 2017, Lars-Peter Clausen wrote:

> On 09/08/2017 11:59 AM, Julia Lawall wrote:
> >
> >
> > On Fri, 8 Sep 2017, Lars-Peter Clausen wrote:
> >
> >> On 09/08/2017 11:32 AM, Jonathan Cameron wrote:
> >>> On Fri, 8 Sep 2017 07:29:06 +0100
> >>> Jonathan Cameron <jic23@jic23.retrosnub.co.uk> wrote:
> >>>
> >>>> On 8 September 2017 05:47:52 BST, Himanshi Jain <himshijain.hj@gmail.com> wrote:
> >>>>> Added space around(one on each side of) binary
> >>>>> operator(-) as preferred according to kernel
> >>>>> coding style.
> >>>>>
> >>>>> Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com>
> >>>>
> >>>> Take a closer look at that macro. It isn't doing what you think...
> >>>> To give a hint, changing this breaks userspace.
> >>>
> >>> Ok, I'm bored of this particular one coming up. When you have
> >>> worked out what is going on Himanshi, would you mind putting
> >>> together a patch adding a comment describing why it is a bad
> >>> idea to 'fix' this?  That would be a very useful patch as
> >>> far as I'm concerned :)
> >>>
> >>> There aren't that many cases of this in IIO so adding a comment
> >>> on each of them is probably reasonable just to avoid wasting
> >>> people's time on fixing them! (I think we have had more than
> >>> 5 such goes this year so far...)
> >>>
> >>
> >> I'd much rather fix this API so that you have to put "" around the name so
> >> that it is clear that it is a string, rather than doing the implicit
> >> conversion to string using preprocessor magic. Better to have a
> >> self-documenting API then having to add a comment to each user of the API.
> >
> > All the DEVICE_ATTR macros use the same strategy.  And the non-IIO ones,
> > eg DEVICE_ATTR_RW, also use the provided name to construct the names of
> > the show and store functions, so I don't think a string would be
> > appropriate.
>
> I'm only suggesting to use a string for the _NAMED macros where the name is
> not used to construct the identifiers.
>
> In the case where the name is used to construct the identifiers we don't
> have the issue with false positives since the name must be a valid
> identifier on its own.

OK, it looks like a good project :)

julia