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 <[email protected]> wrote:
> >
> > On 6/2/21 8:36 AM, sh4nnu wrote:
> > > From: Manikishan Ghantasala <[email protected]>
> > >
> > > 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 <[email protected]>
> > > ---
> > > 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)
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 <[email protected]> wrote:
>>>
>>> On 6/2/21 8:36 AM, sh4nnu wrote:
>>>> From: Manikishan Ghantasala <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> ---
>>>> 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
> [email protected]
> https://lists.linaro.org/mailman/listinfo/greybus-dev
>