Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1728584 > unrolled thread
| Started by | Himanshi Jain <himshijain.hj@gmail.com> |
|---|---|
| First post | 2017-09-08 06:50 +0200 |
| Last post | 2017-09-08 11:40 +0200 |
| Articles | 9 — 5 participants |
Back to article view | Back to linux.kernel
[PATCH] Staging: iio: adc: Added Space around binary op. Himanshi Jain <himshijain.hj@gmail.com> - 2017-09-08 06:50 +0200
Re: [Outreachy kernel] [PATCH] Staging: iio: adc: Added Space around binary op. Julia Lawall <julia.lawall@lip6.fr> - 2017-09-08 08:10 +0200
Re: [PATCH] Staging: iio: adc: Added Space around binary op. Jonathan Cameron <jic23@jic23.retrosnub.co.uk> - 2017-09-08 08:30 +0200
Re: [PATCH] Staging: iio: adc: Added Space around binary op. Lars-Peter Clausen <lars@metafoo.de> - 2017-09-08 11:40 +0200
Re: [PATCH] Staging: iio: adc: Added Space around binary op. Jonathan Cameron <Jonathan.Cameron@huawei.com> - 2017-09-08 11:50 +0200
Re: [Outreachy kernel] Re: [PATCH] Staging: iio: adc: Added Space around binary op. Julia Lawall <julia.lawall@lip6.fr> - 2017-09-08 12:10 +0200
Re: [Outreachy kernel] Re: [PATCH] Staging: iio: adc: Added Space around binary op. Lars-Peter Clausen <lars@metafoo.de> - 2017-09-08 12:20 +0200
Re: [Outreachy kernel] Re: [PATCH] Staging: iio: adc: Added Space around binary op. Julia Lawall <julia.lawall@lip6.fr> - 2017-09-08 12:40 +0200
Re: [PATCH] Staging: iio: adc: Added Space around binary op. Jonathan Cameron <Jonathan.Cameron@huawei.com> - 2017-09-08 11:40 +0200
| From | Himanshi Jain <himshijain.hj@gmail.com> |
|---|---|
| Date | 2017-09-08 06:50 +0200 |
| Subject | [PATCH] Staging: iio: adc: Added Space around binary op. |
| Message-ID | <unjoR-1Us-1@gated-at.bofh.it> |
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
[toc] | [next] | [standalone]
| From | Julia Lawall <julia.lawall@lip6.fr> |
|---|---|
| Date | 2017-09-08 08:10 +0200 |
| Subject | Re: [Outreachy kernel] [PATCH] Staging: iio: adc: Added Space around binary op. |
| Message-ID | <unkEi-2Qx-33@gated-at.bofh.it> |
| In reply to | #1728584 |
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. >
[toc] | [prev] | [next] | [standalone]
| From | Jonathan Cameron <jic23@jic23.retrosnub.co.uk> |
|---|---|
| Date | 2017-09-08 08:30 +0200 |
| Message-ID | <unkXE-2YQ-7@gated-at.bofh.it> |
| In reply to | #1728584 |
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.
[toc] | [prev] | [next] | [standalone]
| From | Lars-Peter Clausen <lars@metafoo.de> |
|---|---|
| Date | 2017-09-08 11:40 +0200 |
| Message-ID | <unnVw-4Ss-9@gated-at.bofh.it> |
| In reply to | #1728627 |
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, >> >
[toc] | [prev] | [next] | [standalone]
| From | Jonathan Cameron <Jonathan.Cameron@huawei.com> |
|---|---|
| Date | 2017-09-08 11:50 +0200 |
| Message-ID | <uno5b-4XX-5@gated-at.bofh.it> |
| In reply to | #1728722 |
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, > >> > > >
[toc] | [prev] | [next] | [standalone]
| From | Julia Lawall <julia.lawall@lip6.fr> |
|---|---|
| Date | 2017-09-08 12:10 +0200 |
| Subject | Re: [Outreachy kernel] Re: [PATCH] Staging: iio: adc: Added Space around binary op. |
| Message-ID | <unooy-5ko-11@gated-at.bofh.it> |
| In reply to | #1728722 |
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. >
[toc] | [prev] | [next] | [standalone]
| From | Lars-Peter Clausen <lars@metafoo.de> |
|---|---|
| Date | 2017-09-08 12:20 +0200 |
| Subject | Re: [Outreachy kernel] Re: [PATCH] Staging: iio: adc: Added Space around binary op. |
| Message-ID | <unoye-5qa-9@gated-at.bofh.it> |
| In reply to | #1728744 |
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.
[toc] | [prev] | [next] | [standalone]
| From | Julia Lawall <julia.lawall@lip6.fr> |
|---|---|
| Date | 2017-09-08 12:40 +0200 |
| Subject | Re: [Outreachy kernel] Re: [PATCH] Staging: iio: adc: Added Space around binary op. |
| Message-ID | <unoRz-5F0-3@gated-at.bofh.it> |
| In reply to | #1728749 |
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
[toc] | [prev] | [next] | [standalone]
| From | Jonathan Cameron <Jonathan.Cameron@huawei.com> |
|---|---|
| Date | 2017-09-08 11:40 +0200 |
| Message-ID | <unnVw-4Ss-11@gated-at.bofh.it> |
| In reply to | #1728627 |
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, >
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web