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)