[PATCH] staging: greybus: fixed the coding style, labels should not be indented.

[PATCH] staging: greybus: fixed the coding style, labels should not be indented.

[sh4nnu]

From: Manikishan Ghantasala <manikishanghantasala@gmail.com>

staging: greybus: gpio.c: Clear coding-style problem
"labels should not be indented" by removing indentation.

Signed-off-by: Manikishan Ghantasala <manikishanghantasala@gmail.com>
---
 drivers/staging/greybus/gpio.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/greybus/gpio.c b/drivers/staging/greybus/gpio.c
index 7e6347fe93f9..4661f4a251bd 100644
--- a/drivers/staging/greybus/gpio.c
+++ b/drivers/staging/greybus/gpio.c
@@ -20,9 +20,9 @@
 struct gb_gpio_line {
 	/* The following has to be an array of line_max entries */
 	/* --> make them just a flags field */
-	u8			active:    1,
-				direction: 1,	/* 0 = output, 1 = input */
-				value:     1;	/* 0 = low, 1 = high */
+	u8			active:1,
+				direction:1,	/* 0 = output, 1 = input */
+				value:1;	/* 0 = low, 1 = high */
 	u16			debounce_usec;
 
 	u8			irq_type;
-- 
2.25.1

[PATCH] staging: greybus: fixed the coding style, labels should not be indented.

[sh4nnu]

From: Manikishan Ghantasala <manikishanghantasala@gmail.com>

staging: greybus: gpio.c: Clear coding-style problem
"labels should not be indented" by removing indentation.

Signed-off-by: Manikishan Ghantasala <manikishanghantasala@gmail.com>
---
 drivers/staging/greybus/gpio.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/greybus/gpio.c b/drivers/staging/greybus/gpio.c
index 7e6347fe93f9..4661f4a251bd 100644
--- a/drivers/staging/greybus/gpio.c
+++ b/drivers/staging/greybus/gpio.c
@@ -20,9 +20,9 @@
 struct gb_gpio_line {
 	/* The following has to be an array of line_max entries */
 	/* --> make them just a flags field */
-	u8			active:    1,
-				direction: 1,	/* 0 = output, 1 = input */
-				value:     1;	/* 0 = low, 1 = high */
+	u8			active:1,
+				direction:1,	/* 0 = output, 1 = input */
+				value:1;	/* 0 = low, 1 = high */
 	u16			debounce_usec;
 
 	u8			irq_type;
-- 
2.25.1

Re: [PATCH] staging: greybus: fixed the coding style, labels should not be indented.

[Alex Elder]

On 6/2/21 8:36 AM, sh4nnu wrote:
> From: Manikishan Ghantasala <manikishanghantasala@gmail.com>
> 
> staging: greybus: gpio.c: Clear coding-style problem
> "labels should not be indented" by removing indentation.

These are not labels.

I don't really understand what you're doing here.

Can you please explain why you think this needs changing?

					-Alex

> Signed-off-by: Manikishan Ghantasala <manikishanghantasala@gmail.com>
> ---
>   drivers/staging/greybus/gpio.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/greybus/gpio.c b/drivers/staging/greybus/gpio.c
> index 7e6347fe93f9..4661f4a251bd 100644
> --- a/drivers/staging/greybus/gpio.c
> +++ b/drivers/staging/greybus/gpio.c
> @@ -20,9 +20,9 @@
>   struct gb_gpio_line {
>   	/* The following has to be an array of line_max entries */
>   	/* --> make them just a flags field */
> -	u8			active:    1,
> -				direction: 1,	/* 0 = output, 1 = input */
> -				value:     1;	/* 0 = low, 1 = high */
> +	u8			active:1,
> +				direction:1,	/* 0 = output, 1 = input */
> +				value:1;	/* 0 = low, 1 = high */
>   	u16			debounce_usec;
>   
>   	u8			irq_type;
> 

Re: [PATCH] staging: greybus: fixed the coding style, labels should not be indented.

[Manikishan Ghantasala]

Sending this mail again as I missed to reply to all.
 Hi Alex,

I agree those are called bit-field member names rather than labels.
But the reason I mentioned is because the ./scripts/checkpatch.pl
gave out a warning saying "labels should not be indented".

Sorry for the confusion in the name I referred to. So, I think this
change is needed as I feel this is not following the coding-style by
having indent before the width for bit field member. I went through
other places in source code to make sure this is correct, and sent the
patch after confirmation.

Regards,
Manikishan Ghantasala

On Wed, 2 Jun 2021 at 19:13, Alex Elder <elder@ieee.org> wrote:
>
> On 6/2/21 8:36 AM, sh4nnu wrote:
> > From: Manikishan Ghantasala <manikishanghantasala@gmail.com>
> >
> > staging: greybus: gpio.c: Clear coding-style problem
> > "labels should not be indented" by removing indentation.
>
> These are not labels.
>
> I don't really understand what you're doing here.
>
> Can you please explain why you think this needs changing?
>
>                                         -Alex
>
> > Signed-off-by: Manikishan Ghantasala <manikishanghantasala@gmail.com>
> > ---
> >   drivers/staging/greybus/gpio.c | 6 +++---
> >   1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/staging/greybus/gpio.c b/drivers/staging/greybus/gpio.c
> > index 7e6347fe93f9..4661f4a251bd 100644
> > --- a/drivers/staging/greybus/gpio.c
> > +++ b/drivers/staging/greybus/gpio.c
> > @@ -20,9 +20,9 @@
> >   struct gb_gpio_line {
> >       /* The following has to be an array of line_max entries */
> >       /* --> make them just a flags field */
> > -     u8                      active:    1,
> > -                             direction: 1,   /* 0 = output, 1 = input */
> > -                             value:     1;   /* 0 = low, 1 = high */
> > +     u8                      active:1,
> > +                             direction:1,    /* 0 = output, 1 = input */
> > +                             value:1;        /* 0 = low, 1 = high */
> >       u16                     debounce_usec;
> >
> >       u8                      irq_type;
> >
>

Re: [PATCH] staging: greybus: fixed the coding style, labels should not be indented.

[Manikishan Ghantasala]

Sending this mail again as I missed to reply to all.
 Hi Alex,

I agree those are called bit-field member names rather than labels.
But the reason I mentioned is because the ./scripts/checkpatch.pl
gave out a warning saying "labels should not be indented".

Sorry for the confusion in the name I referred to. So, I think this
change is needed as I feel this is not following the coding-style by
having indent before the width for bit field member. I went through
other places in source code to make sure this is correct, and sent the
patch after confirmation.

Regards,
Manikishan Ghantasala

On Wed, 2 Jun 2021 at 19:13, Alex Elder <elder@ieee.org> wrote:
>
> On 6/2/21 8:36 AM, sh4nnu wrote:
> > From: Manikishan Ghantasala <manikishanghantasala@gmail.com>
> >
> > staging: greybus: gpio.c: Clear coding-style problem
> > "labels should not be indented" by removing indentation.
>
> These are not labels.
>
> I don't really understand what you're doing here.
>
> Can you please explain why you think this needs changing?
>
>                                         -Alex
>
> > Signed-off-by: Manikishan Ghantasala <manikishanghantasala@gmail.com>
> > ---
> >   drivers/staging/greybus/gpio.c | 6 +++---
> >   1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/staging/greybus/gpio.c b/drivers/staging/greybus/gpio.c
> > index 7e6347fe93f9..4661f4a251bd 100644
> > --- a/drivers/staging/greybus/gpio.c
> > +++ b/drivers/staging/greybus/gpio.c
> > @@ -20,9 +20,9 @@
> >   struct gb_gpio_line {
> >       /* The following has to be an array of line_max entries */
> >       /* --> make them just a flags field */
> > -     u8                      active:    1,
> > -                             direction: 1,   /* 0 = output, 1 = input */
> > -                             value:     1;   /* 0 = low, 1 = high */
> > +     u8                      active:1,
> > +                             direction:1,    /* 0 = output, 1 = input */
> > +                             value:1;        /* 0 = low, 1 = high */
> >       u16                     debounce_usec;
> >
> >       u8                      irq_type;
> >
>

Re: [PATCH] staging: greybus: fixed the coding style, labels should not be indented.

[Greg Kroah-Hartman]

On Wed, Jun 02, 2021 at 07:57:35PM +0530, Manikishan Ghantasala wrote:
> Sending this mail again as I missed to reply to all.
>  Hi Alex,
> 
> I agree those are called bit-field member names rather than labels.
> But the reason I mentioned is because the ./scripts/checkpatch.pl
> gave out a warning saying "labels should not be indented".

checkpatch is a perl script that does it's best, but does not always get
it right.  In this case, it is incorrect, the existing code is just
fine.

thanks,

greg k-h

Re: [PATCH] staging: greybus: fixed the coding style, labels should not be indented.

[Alex Elder]

On 6/2/21 9:27 AM, Manikishan Ghantasala wrote:
> Sending this mail again as I missed to reply to all.
>   Hi Alex,
> 
> I agree those are called bit-field member names rather than labels.
> But the reason I mentioned is because the ./scripts/checkpatch.pl
> gave out a warning saying "labels should not be indented".
> 
> Sorry for the confusion in the name I referred to. So, I think this
> change is needed as I feel this is not following the coding-style by
> having indent before the width for bit field member. I went through
> other places in source code to make sure this is correct, and sent the
> patch after confirmation.

I agree that many instances in the kernel source place the width
of a C bit-field immediately after the colon.  But it is not a
universal convention, and I personally prefer the aligned widths
used by the Greybus code here.

So I don't find this patch acceptable.

					-Alex

> Regards,
> Manikishan Ghantasala
> 
> On Wed, 2 Jun 2021 at 19:13, Alex Elder <elder@ieee.org> wrote:
>>
>> On 6/2/21 8:36 AM, sh4nnu wrote:
>>> From: Manikishan Ghantasala <manikishanghantasala@gmail.com>
>>>
>>> staging: greybus: gpio.c: Clear coding-style problem
>>> "labels should not be indented" by removing indentation.
>>
>> These are not labels.
>>
>> I don't really understand what you're doing here.
>>
>> Can you please explain why you think this needs changing?
>>
>>                                          -Alex
>>
>>> Signed-off-by: Manikishan Ghantasala <manikishanghantasala@gmail.com>
>>> ---
>>>    drivers/staging/greybus/gpio.c | 6 +++---
>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/staging/greybus/gpio.c b/drivers/staging/greybus/gpio.c
>>> index 7e6347fe93f9..4661f4a251bd 100644
>>> --- a/drivers/staging/greybus/gpio.c
>>> +++ b/drivers/staging/greybus/gpio.c
>>> @@ -20,9 +20,9 @@
>>>    struct gb_gpio_line {
>>>        /* The following has to be an array of line_max entries */
>>>        /* --> make them just a flags field */
>>> -     u8                      active:    1,
>>> -                             direction: 1,   /* 0 = output, 1 = input */
>>> -                             value:     1;   /* 0 = low, 1 = high */
>>> +     u8                      active:1,
>>> +                             direction:1,    /* 0 = output, 1 = input */
>>> +                             value:1;        /* 0 = low, 1 = high */
>>>        u16                     debounce_usec;
>>>
>>>        u8                      irq_type;
>>>
>>

Re: [PATCH] staging: greybus: fixed the coding style, labels should not be indented.

[Manikishan Ghantasala]

Hi Greg,
Thanks for the clarification.
Regards,
Manikishan Ghantasala

On Wed, 2 Jun 2021 at 20:07, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Jun 02, 2021 at 07:57:35PM +0530, Manikishan Ghantasala wrote:
> > Sending this mail again as I missed to reply to all.
> >  Hi Alex,
> >
> > I agree those are called bit-field member names rather than labels.
> > But the reason I mentioned is because the ./scripts/checkpatch.pl
> > gave out a warning saying "labels should not be indented".
>
> checkpatch is a perl script that does it's best, but does not always get
> it right.  In this case, it is incorrect, the existing code is just
> fine.
>
> thanks,
>
> greg k-h

Re: [PATCH] staging: greybus: fixed the coding style, labels should not be indented.

[Manikishan Ghantasala]

Hi Greg,
Thanks for the clarification.
Regards,
Manikishan Ghantasala

On Wed, 2 Jun 2021 at 20:07, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Jun 02, 2021 at 07:57:35PM +0530, Manikishan Ghantasala wrote:
> > Sending this mail again as I missed to reply to all.
> >  Hi Alex,
> >
> > I agree those are called bit-field member names rather than labels.
> > But the reason I mentioned is because the ./scripts/checkpatch.pl
> > gave out a warning saying "labels should not be indented".
>
> checkpatch is a perl script that does it's best, but does not always get
> it right.  In this case, it is incorrect, the existing code is just
> fine.
>
> thanks,
>
> greg k-h

[PATCH] checkpatch: Improve the indented label test

[Joe Perches]

checkpatch identifies a label only when a terminating colon
immediately follows an identifier.

Bitfield definitions can appear to be labels so ignore any
spaces between the identifier terminating colon and any digit
that may be used to define a bitfield length.

Miscellanea:

o Improve the initial checkpatch comment
o Use the more typical '&&' instead of 'and'
o Require the initial label character to be a non-digit
  (Can't use $Ident here because $Ident allows ## concatenation)
o Use $sline instead of $line to ignore comments
o Use '$sline !~ /.../' instead of '!($line =~ /.../)'

Signed-off-by: Joe Perches <joe@perches.com>
---
 
On Wed, 2021-06-02 at 16:37 +0200, Greg Kroah-Hartman wrote:
> On Wed, Jun 02, 2021 at 07:57:35PM +0530, Manikishan Ghantasala wrote:
> > I agree those are called bit-field member names rather than labels.
> > But the reason I mentioned is because the ./scripts/checkpatch.pl
> > gave out a warning saying "labels should not be indented".
> 
> checkpatch is a perl script that does it's best, but does not always get
> it right.  In this case, it is incorrect, the existing code is just
> fine.

 scripts/checkpatch.pl | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d65334588eb4c..6e7d48f412fb7 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5361,9 +5361,13 @@ sub process {
 			}
 		}
 
-#goto labels aren't indented, allow a single space however
-		if ($line=~/^.\s+[A-Za-z\d_]+:(?![0-9]+)/ and
-		   !($line=~/^. [A-Za-z\d_]+:/) and !($line=~/^.\s+default:/)) {
+# check that goto labels aren't indented (allow a single space indentation)
+# and ignore bitfield definitions like foo:1
+# Strictly, labels can have whitespace after the identifier and before the :
+# but this is not allowed here as many ?: uses would appear to be labels
+		if ($sline =~ /^.\s+[A-Za-z_][A-Za-z\d_]*:(?!\s*\d+)/ &&
+		    $sline !~ /^. [A-Za-z\d_][A-Za-z\d_]*:/ &&
+		    $sline !~ /^.\s+default:/) {
 			if (WARN("INDENTED_LABEL",
 				 "labels should not be indented\n" . $herecurr) &&
 			    $fix) {

[PATCH] checkpatch: Improve the indented label test

[Joe Perches]

checkpatch identifies a label only when a terminating colon
immediately follows an identifier.

Bitfield definitions can appear to be labels so ignore any
spaces between the identifier terminating colon and any digit
that may be used to define a bitfield length.

Miscellanea:

o Improve the initial checkpatch comment
o Use the more typical '&&' instead of 'and'
o Require the initial label character to be a non-digit
  (Can't use $Ident here because $Ident allows ## concatenation)
o Use $sline instead of $line to ignore comments
o Use '$sline !~ /.../' instead of '!($line =~ /.../)'

Signed-off-by: Joe Perches <joe@perches.com>
---
 
On Wed, 2021-06-02 at 16:37 +0200, Greg Kroah-Hartman wrote:
> On Wed, Jun 02, 2021 at 07:57:35PM +0530, Manikishan Ghantasala wrote:
> > I agree those are called bit-field member names rather than labels.
> > But the reason I mentioned is because the ./scripts/checkpatch.pl
> > gave out a warning saying "labels should not be indented".
> 
> checkpatch is a perl script that does it's best, but does not always get
> it right.  In this case, it is incorrect, the existing code is just
> fine.

 scripts/checkpatch.pl | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d65334588eb4c..6e7d48f412fb7 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5361,9 +5361,13 @@ sub process {
 			}
 		}
 
-#goto labels aren't indented, allow a single space however
-		if ($line=~/^.\s+[A-Za-z\d_]+:(?![0-9]+)/ and
-		   !($line=~/^. [A-Za-z\d_]+:/) and !($line=~/^.\s+default:/)) {
+# check that goto labels aren't indented (allow a single space indentation)
+# and ignore bitfield definitions like foo:1
+# Strictly, labels can have whitespace after the identifier and before the :
+# but this is not allowed here as many ?: uses would appear to be labels
+		if ($sline =~ /^.\s+[A-Za-z_][A-Za-z\d_]*:(?!\s*\d+)/ &&
+		    $sline !~ /^. [A-Za-z\d_][A-Za-z\d_]*:/ &&
+		    $sline !~ /^.\s+default:/) {
 			if (WARN("INDENTED_LABEL",
 				 "labels should not be indented\n" . $herecurr) &&
 			    $fix) {

RE: [PATCH] staging: greybus: fixed the coding style, labels should not be indented.

[David Laight]

From: Manikishan Ghantasala
> Sent: 02 June 2021 15:28
> 
> I agree those are called bit-field member names rather than labels.
> But the reason I mentioned is because the ./scripts/checkpatch.pl
> gave out a warning saying "labels should not be indented".
> 
> Sorry for the confusion in the name I referred to. So, I think this
> change is needed as I feel this is not following the coding-style by
> having indent before the width for bit field member. I went through
> other places in source code to make sure this is correct, and sent the
> patch after confirmation.
> 
> Regards,
> Manikishan Ghantasala
> 
> On Wed, 2 Jun 2021 at 19:13, Alex Elder <elder@ieee.org> wrote:
> >
> > On 6/2/21 8:36 AM, sh4nnu wrote:
> > > From: Manikishan Ghantasala <manikishanghantasala@gmail.com>
> > >
> > > staging: greybus: gpio.c: Clear coding-style problem
> > > "labels should not be indented" by removing indentation.
> >
> > These are not labels.
> >
> > I don't really understand what you're doing here.
> >
> > Can you please explain why you think this needs changing?
> >
> >                                         -Alex
> >
> > > Signed-off-by: Manikishan Ghantasala <manikishanghantasala@gmail.com>
> > > ---
> > >   drivers/staging/greybus/gpio.c | 6 +++---
> > >   1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/staging/greybus/gpio.c b/drivers/staging/greybus/gpio.c
> > > index 7e6347fe93f9..4661f4a251bd 100644
> > > --- a/drivers/staging/greybus/gpio.c
> > > +++ b/drivers/staging/greybus/gpio.c
> > > @@ -20,9 +20,9 @@
> > >   struct gb_gpio_line {
> > >       /* The following has to be an array of line_max entries */
> > >       /* --> make them just a flags field */
> > > -     u8                      active:    1,
> > > -                             direction: 1,   /* 0 = output, 1 = input */
> > > -                             value:     1;   /* 0 = low, 1 = high */
> > > +     u8                      active:1,
> > > +                             direction:1,    /* 0 = output, 1 = input */
> > > +                             value:1;        /* 0 = low, 1 = high */

Why are you even using bitfields at all?
If you cared about the structure size you'd not have a byte-size pad here.

Since I doubt many copies of this structure get allocated the
(typical) increase in code size for the bitfields will also
exceed any size saving.

Isn't the kernel style also to repeat the type for every field?

	David


> > >       u16                     debounce_usec;
> > >
> > >       u8                      irq_type;
> > >
> >

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [greybus-dev] [PATCH] staging: greybus: fixed the coding style, labels should not be indented.

[Alex Elder]

On 6/3/21 4:22 PM, David Laight wrote:
> From: Manikishan Ghantasala
>> Sent: 02 June 2021 15:28
>>
>> I agree those are called bit-field member names rather than labels.
>> But the reason I mentioned is because the ./scripts/checkpatch.pl
>> gave out a warning saying "labels should not be indented".
>>
>> Sorry for the confusion in the name I referred to. So, I think this
>> change is needed as I feel this is not following the coding-style by
>> having indent before the width for bit field member. I went through
>> other places in source code to make sure this is correct, and sent the
>> patch after confirmation.
>>
>> Regards,
>> Manikishan Ghantasala
>>
>> On Wed, 2 Jun 2021 at 19:13, Alex Elder <elder@ieee.org> wrote:
>>>
>>> On 6/2/21 8:36 AM, sh4nnu wrote:
>>>> From: Manikishan Ghantasala <manikishanghantasala@gmail.com>
>>>>
>>>> staging: greybus: gpio.c: Clear coding-style problem
>>>> "labels should not be indented" by removing indentation.
>>>
>>> These are not labels.
>>>
>>> I don't really understand what you're doing here.
>>>
>>> Can you please explain why you think this needs changing?
>>>
>>>                                          -Alex
>>>
>>>> Signed-off-by: Manikishan Ghantasala <manikishanghantasala@gmail.com>
>>>> ---
>>>>    drivers/staging/greybus/gpio.c | 6 +++---
>>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/greybus/gpio.c b/drivers/staging/greybus/gpio.c
>>>> index 7e6347fe93f9..4661f4a251bd 100644
>>>> --- a/drivers/staging/greybus/gpio.c
>>>> +++ b/drivers/staging/greybus/gpio.c
>>>> @@ -20,9 +20,9 @@
>>>>    struct gb_gpio_line {
>>>>        /* The following has to be an array of line_max entries */
>>>>        /* --> make them just a flags field */
>>>> -     u8                      active:    1,
>>>> -                             direction: 1,   /* 0 = output, 1 = input */
>>>> -                             value:     1;   /* 0 = low, 1 = high */
>>>> +     u8                      active:1,
>>>> +                             direction:1,    /* 0 = output, 1 = input */
>>>> +                             value:1;        /* 0 = low, 1 = high */
> 
> Why are you even using bitfields at all?
> If you cared about the structure size you'd not have a byte-size pad here.

Apparently I committed this, and it was part of the very first
Greybus drivers...

These would be better defined as Booleans; there are others in
the same structure after all.  That would have avoided the
checkpatch problem in the first place.

I was probably thinking *a little* about structure size when
defining it this way, but I agree with you, the bit-fields
don't really add value.

> Since I doubt many copies of this structure get allocated the
> (typical) increase in code size for the bitfields will also
> exceed any size saving.
> 
> Isn't the kernel style also to repeat the type for every field?

I see that style in many places, but not all.  I personally
like it this way--provided it's done in a way that makes
it clear where the integral boundaries are.

					-Alex

> 	David
> 
> 
>>>>        u16                     debounce_usec;
>>>>
>>>>        u8                      irq_type;
>>>>
>>>
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
> _______________________________________________
> greybus-dev mailing list
> greybus-dev@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/greybus-dev
> 

RE: [greybus-dev] [PATCH] staging: greybus: fixed the coding style, labels should not be indented.

[David Laight]

From: Alex Elder
> Sent: 03 June 2021 22:46
> 
> On 6/3/21 4:22 PM, David Laight wrote:
...
> >>>> --- a/drivers/staging/greybus/gpio.c
> >>>> +++ b/drivers/staging/greybus/gpio.c
> >>>> @@ -20,9 +20,9 @@
> >>>>    struct gb_gpio_line {
> >>>>        /* The following has to be an array of line_max entries */
> >>>>        /* --> make them just a flags field */
> >>>> -     u8                      active:    1,
> >>>> -                             direction: 1,   /* 0 = output, 1 = input */
> >>>> -                             value:     1;   /* 0 = low, 1 = high */
> >>>> +     u8                      active:1,
> >>>> +                             direction:1,    /* 0 = output, 1 = input */
> >>>> +                             value:1;        /* 0 = low, 1 = high */
> >
> > Why are you even using bitfields at all?
> > If you cared about the structure size you'd not have a byte-size pad here.
> 
> Apparently I committed this, and it was part of the very first
> Greybus drivers...
> 
> These would be better defined as Booleans; there are others in
> the same structure after all.  That would have avoided the
> checkpatch problem in the first place.

Using 'u8' can be sensible.
Boolean will be 32bit.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [greybus-dev] [PATCH] staging: greybus: fixed the coding style, labels should not be indented.

[Alex Elder]

On 6/3/21 4:48 PM, David Laight wrote:
> From: Alex Elder
>> Sent: 03 June 2021 22:46
>>
>> On 6/3/21 4:22 PM, David Laight wrote:
> ...
>>>>>> --- a/drivers/staging/greybus/gpio.c
>>>>>> +++ b/drivers/staging/greybus/gpio.c
>>>>>> @@ -20,9 +20,9 @@
>>>>>>     struct gb_gpio_line {
>>>>>>         /* The following has to be an array of line_max entries */
>>>>>>         /* --> make them just a flags field */
>>>>>> -     u8                      active:    1,
>>>>>> -                             direction: 1,   /* 0 = output, 1 = input */
>>>>>> -                             value:     1;   /* 0 = low, 1 = high */
>>>>>> +     u8                      active:1,
>>>>>> +                             direction:1,    /* 0 = output, 1 = input */
>>>>>> +                             value:1;        /* 0 = low, 1 = high */
>>>
>>> Why are you even using bitfields at all?
>>> If you cared about the structure size you'd not have a byte-size pad here.
>>
>> Apparently I committed this, and it was part of the very first
>> Greybus drivers...
>>
>> These would be better defined as Booleans; there are others in
>> the same structure after all.  That would have avoided the
>> checkpatch problem in the first place.
> 
> Using 'u8' can be sensible.
> Boolean will be 32bit.

Not necessarily, sizeof(bool) is implementation defined.
And I thought you didn't think the size of the structure
was very important...

In any case, I'm open to changing the type of these fields,
and my preference would be bool rather than u8, because it
is completely clear what it represents.

					-Alex

> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
> 

RE: [greybus-dev] [PATCH] staging: greybus: fixed the coding style, labels should not be indented.

[David Laight]

From: Alex Elder
> Sent: 03 June 2021 22:55
...
> Not necessarily, sizeof(bool) is implementation defined.
> And I thought you didn't think the size of the structure
> was very important...

It is 'implementation defined' but will be 32 bits on everything
except an old 32bit ARM ABI.

> In any case, I'm open to changing the type of these fields,
> and my preference would be bool rather than u8, because it
> is completely clear what it represents.

Yes, and it isn't at all clear what it actually means.
If the value of a bool memory location isn't 0 or 1
what does 'bool_a & bool_b' mean.
It might be 'undefined behaviour' - but that doesn't actually
exclude an ICBM hitting the coder's house!
I've seen very silly code generated (by an old gcc) for
simple statements like:
	bool_a |= bool_b;
Mostly because it didn't trust the values to be 0 or 1
and wanted to ensure the result was either 0 or 1.

If I use an integer type (as in traditional C) I know
what I'm getting and there are no unexpected comparisons
and conditionals.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [greybus-dev] [PATCH] staging: greybus: fixed the coding style, labels should not be indented.

[Joe Perches]

On Fri, 2021-06-04 at 08:13 +0000, David Laight wrote:
> From: Alex Elder
> > Sent: 03 June 2021 22:55
> ...
> > Not necessarily, sizeof(bool) is implementation defined.
> > And I thought you didn't think the size of the structure
> > was very important...
> 
> It is 'implementation defined' but will be 32 bits on everything
> except an old 32bit ARM ABI.

Really? (x86-64)

$ gcc -x c -
#include <stdio.h>
#include <stdlib.h>

struct foo {
	_Bool b;
};

int main(int argc, char **argv)
{
	printf("sizeof _Bool: %zu\n", sizeof(_Bool));
	printf("sizeof struct foo: %zu\n", sizeof(struct foo));
	return 0;
}
$ ./a.out
sizeof _Bool: 1
sizeof struct foo: 1

Re: [greybus-dev] [PATCH] staging: greybus: fixed the coding style, labels should not be indented.

[Joe Perches]

On Fri, 2021-06-04 at 08:13 +0000, David Laight wrote:
> From: Alex Elder
> > Sent: 03 June 2021 22:55
> ...
> > Not necessarily, sizeof(bool) is implementation defined.
> > And I thought you didn't think the size of the structure
> > was very important...
> 
> It is 'implementation defined' but will be 32 bits on everything
> except an old 32bit ARM ABI.

Really? (x86-64)

$ gcc -x c -
#include <stdio.h>
#include <stdlib.h>

struct foo {
	_Bool b;
};

int main(int argc, char **argv)
{
	printf("sizeof _Bool: %zu\n", sizeof(_Bool));
	printf("sizeof struct foo: %zu\n", sizeof(struct foo));
	return 0;
}
$ ./a.out
sizeof _Bool: 1
sizeof struct foo: 1

RE: [greybus-dev] [PATCH] staging: greybus: fixed the coding style, labels should not be indented.

[David Laight]

From: Joe Perches
> Sent: 04 June 2021 09:27
> 
> On Fri, 2021-06-04 at 08:13 +0000, David Laight wrote:
> > From: Alex Elder
> > > Sent: 03 June 2021 22:55
> > ...
> > > Not necessarily, sizeof(bool) is implementation defined.
> > > And I thought you didn't think the size of the structure
> > > was very important...
> >
> > It is 'implementation defined' but will be 32 bits on everything
> > except an old 32bit ARM ABI.
> 
> Really? (x86-64)
> 
...
> sizeof _Bool: 1
> sizeof struct foo: 1

Gah, not enough coffee.
I said I don't like _Bool :-)

The old arm32 must have had 32bit bool.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [greybus-dev] [PATCH] staging: greybus: fixed the coding style, labels should not be indented.

[Alex Elder]

On 6/4/21 3:13 AM, David Laight wrote:
> Yes, and it isn't at all clear what it actually means.
> If the value of a bool memory location isn't 0 or 1
> what does 'bool_a & bool_b' mean.

I think this discussion is done, but I wanted to point out
that the above expression is incorrect.  It might be valid,
but it would be bad code.  A Boolean, when properly used,
should only be compared with true and false (or the result
of another Boolean expression).  Therefore "&" is not the
right operator, "&&" is.  The reason is similar to why you
would never use ~bool_a, you use !bool_a to invert its value.

					-Alex