[1/3] Revert "staging: mmal-vchiq: Avoid use of bool in structures"

  This reverts commit 640e77466e69d9c28de227bc76881f5501f532ca.

In commit 7967656ffbfa ("coding-style: Clarify the expectations around
bool") the check to dis-allow bool structure members was removed from
checkpatch.pl. It promotes bool structure members to store boolean
values. This enhances code readability.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../staging/vc04_services/vchiq-mmal/mmal-vchiq.c    | 12 ++++++------
 .../staging/vc04_services/vchiq-mmal/mmal-vchiq.h    |  4 ++--
 2 files changed, 8 insertions(+), 8 deletions(-)
  

Quoting Umang Jain (2022-11-17 16:00:13)
> This reverts commit 640e77466e69d9c28de227bc76881f5501f532ca.
> 
> In commit 7967656ffbfa ("coding-style: Clarify the expectations around
> bool") the check to dis-allow bool structure members was removed from
> checkpatch.pl. It promotes bool structure members to store boolean
> values. This enhances code readability.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  .../staging/vc04_services/vchiq-mmal/mmal-vchiq.c    | 12 ++++++------
>  .../staging/vc04_services/vchiq-mmal/mmal-vchiq.h    |  4 ++--
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
> index cb921c94996a..4abb6178cb9f 100644
> --- a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
> +++ b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
> @@ -863,9 +863,9 @@ static int port_info_get(struct vchiq_mmal_instance *instance,
>                 goto release_msg;
>  
>         if (rmsg->u.port_info_get_reply.port.is_enabled == 0)
> -               port->enabled = 0;
> +               port->enabled = false;
>         else
> -               port->enabled = 1;
> +               port->enabled = true;
>  
>         /* copy the values out of the message */
>         port->handle = rmsg->u.port_info_get_reply.port_handle;
> @@ -1304,7 +1304,7 @@ static int port_disable(struct vchiq_mmal_instance *instance,
>         if (!port->enabled)
>                 return 0;
>  
> -       port->enabled = 0;
> +       port->enabled = false;
>  
>         ret = port_action_port(instance, port,
>                                MMAL_MSG_PORT_ACTION_TYPE_DISABLE);
> @@ -1359,7 +1359,7 @@ static int port_enable(struct vchiq_mmal_instance *instance,
>         if (ret)
>                 goto done;
>  
> -       port->enabled = 1;
> +       port->enabled = true;
>  
>         if (port->buffer_cb) {
>                 /* send buffer headers to videocore */
> @@ -1531,7 +1531,7 @@ int vchiq_mmal_port_connect_tunnel(struct vchiq_mmal_instance *instance,
>                         pr_err("failed disconnecting src port\n");
>                         goto release_unlock;
>                 }
> -               src->connected->enabled = 0;
> +               src->connected->enabled = false;
>                 src->connected = NULL;
>         }
>  
> @@ -1799,7 +1799,7 @@ int vchiq_mmal_component_disable(struct vchiq_mmal_instance *instance,
>  
>         ret = disable_component(instance, component);
>         if (ret == 0)
> -               component->enabled = 0;
> +               component->enabled = false;
>  
>         mutex_unlock(&instance->vchiq_mutex);
>  
> diff --git a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.h b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.h
> index 6006e29232b3..70eda6cac1c6 100644
> --- a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.h
> +++ b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.h
> @@ -48,7 +48,7 @@ typedef void (*vchiq_mmal_buffer_cb)(
>                 int status, struct mmal_buffer *buffer);
>  
>  struct vchiq_mmal_port {
> -       u32 enabled:1;
> +       bool enabled:1;

Is this a direct revert with 'git revert' ?

I would expect this to be
	bool enabled;


>         u32 handle;
>         u32 type; /* port type, cached to use on port info set */
>         u32 index; /* port index, cached to use on port info set */
> @@ -83,7 +83,7 @@ struct vchiq_mmal_port {
>  
>  struct vchiq_mmal_component {
>         u32 in_use:1;
> -       u32 enabled:1;
> +       bool enabled:1;

Same here of course.

>         u32 handle;  /* VideoCore handle for component */
>         u32 inputs;  /* Number of input ports */
>         u32 outputs; /* Number of output ports */
> -- 
> 2.38.1
>
  

Hi Kieran,

On 11/17/22 9:39 PM, Kieran Bingham wrote:
> Quoting Umang Jain (2022-11-17 16:00:13)
>> This reverts commit 640e77466e69d9c28de227bc76881f5501f532ca.
>>
>> In commit 7967656ffbfa ("coding-style: Clarify the expectations around
>> bool") the check to dis-allow bool structure members was removed from
>> checkpatch.pl. It promotes bool structure members to store boolean
>> values. This enhances code readability.
>>
>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>> ---
>>   .../staging/vc04_services/vchiq-mmal/mmal-vchiq.c    | 12 ++++++------
>>   .../staging/vc04_services/vchiq-mmal/mmal-vchiq.h    |  4 ++--
>>   2 files changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
>> index cb921c94996a..4abb6178cb9f 100644
>> --- a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
>> +++ b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
>> @@ -863,9 +863,9 @@ static int port_info_get(struct vchiq_mmal_instance *instance,
>>                  goto release_msg;
>>   
>>          if (rmsg->u.port_info_get_reply.port.is_enabled == 0)
>> -               port->enabled = 0;
>> +               port->enabled = false;
>>          else
>> -               port->enabled = 1;
>> +               port->enabled = true;
>>   
>>          /* copy the values out of the message */
>>          port->handle = rmsg->u.port_info_get_reply.port_handle;
>> @@ -1304,7 +1304,7 @@ static int port_disable(struct vchiq_mmal_instance *instance,
>>          if (!port->enabled)
>>                  return 0;
>>   
>> -       port->enabled = 0;
>> +       port->enabled = false;
>>   
>>          ret = port_action_port(instance, port,
>>                                 MMAL_MSG_PORT_ACTION_TYPE_DISABLE);
>> @@ -1359,7 +1359,7 @@ static int port_enable(struct vchiq_mmal_instance *instance,
>>          if (ret)
>>                  goto done;
>>   
>> -       port->enabled = 1;
>> +       port->enabled = true;
>>   
>>          if (port->buffer_cb) {
>>                  /* send buffer headers to videocore */
>> @@ -1531,7 +1531,7 @@ int vchiq_mmal_port_connect_tunnel(struct vchiq_mmal_instance *instance,
>>                          pr_err("failed disconnecting src port\n");
>>                          goto release_unlock;
>>                  }
>> -               src->connected->enabled = 0;
>> +               src->connected->enabled = false;
>>                  src->connected = NULL;
>>          }
>>   
>> @@ -1799,7 +1799,7 @@ int vchiq_mmal_component_disable(struct vchiq_mmal_instance *instance,
>>   
>>          ret = disable_component(instance, component);
>>          if (ret == 0)
>> -               component->enabled = 0;
>> +               component->enabled = false;
>>   
>>          mutex_unlock(&instance->vchiq_mutex);
>>   
>> diff --git a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.h b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.h
>> index 6006e29232b3..70eda6cac1c6 100644
>> --- a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.h
>> +++ b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.h
>> @@ -48,7 +48,7 @@ typedef void (*vchiq_mmal_buffer_cb)(
>>                  int status, struct mmal_buffer *buffer);
>>   
>>   struct vchiq_mmal_port {
>> -       u32 enabled:1;
>> +       bool enabled:1;
> Is this a direct revert with 'git revert' ?


No. It had conflicts plus I added the ':1' initialization to keep the 
logic same (in case 'enabled' gets used directly). Similar pattern come 
up with:
     ($) git grep 'bool' -- '*.[h]' | grep '\:1'

So it shouldn't be an issue.

>
> I would expect this to be
> 	bool enabled;


True but it won't functionally not be the same in matter of 
initialization. Should the initialization be split to separate patch?

>
>
>>          u32 handle;
>>          u32 type; /* port type, cached to use on port info set */
>>          u32 index; /* port index, cached to use on port info set */
>> @@ -83,7 +83,7 @@ struct vchiq_mmal_port {
>>   
>>   struct vchiq_mmal_component {
>>          u32 in_use:1;
>> -       u32 enabled:1;
>> +       bool enabled:1;
> Same here of course.
>
>>          u32 handle;  /* VideoCore handle for component */
>>          u32 inputs;  /* Number of input ports */
>>          u32 outputs; /* Number of output ports */
>> -- 
>> 2.38.1
>>
  

On Thu, Nov 17, 2022 at 11:25:48PM +0530, Umang Jain wrote:
> Hi Kieran,
> 
> On 11/17/22 9:39 PM, Kieran Bingham wrote:
> > Quoting Umang Jain (2022-11-17 16:00:13)
> > > This reverts commit 640e77466e69d9c28de227bc76881f5501f532ca.
> > > 
> > > In commit 7967656ffbfa ("coding-style: Clarify the expectations around
> > > bool") the check to dis-allow bool structure members was removed from
> > > checkpatch.pl. It promotes bool structure members to store boolean
> > > values. This enhances code readability.
> > > 
> > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > > ---
> > >   .../staging/vc04_services/vchiq-mmal/mmal-vchiq.c    | 12 ++++++------
> > >   .../staging/vc04_services/vchiq-mmal/mmal-vchiq.h    |  4 ++--
> > >   2 files changed, 8 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
> > > index cb921c94996a..4abb6178cb9f 100644
> > > --- a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
> > > +++ b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
> > > @@ -863,9 +863,9 @@ static int port_info_get(struct vchiq_mmal_instance *instance,
> > >                  goto release_msg;
> > >          if (rmsg->u.port_info_get_reply.port.is_enabled == 0)
> > > -               port->enabled = 0;
> > > +               port->enabled = false;
> > >          else
> > > -               port->enabled = 1;
> > > +               port->enabled = true;
> > >          /* copy the values out of the message */
> > >          port->handle = rmsg->u.port_info_get_reply.port_handle;
> > > @@ -1304,7 +1304,7 @@ static int port_disable(struct vchiq_mmal_instance *instance,
> > >          if (!port->enabled)
> > >                  return 0;
> > > -       port->enabled = 0;
> > > +       port->enabled = false;
> > >          ret = port_action_port(instance, port,
> > >                                 MMAL_MSG_PORT_ACTION_TYPE_DISABLE);
> > > @@ -1359,7 +1359,7 @@ static int port_enable(struct vchiq_mmal_instance *instance,
> > >          if (ret)
> > >                  goto done;
> > > -       port->enabled = 1;
> > > +       port->enabled = true;
> > >          if (port->buffer_cb) {
> > >                  /* send buffer headers to videocore */
> > > @@ -1531,7 +1531,7 @@ int vchiq_mmal_port_connect_tunnel(struct vchiq_mmal_instance *instance,
> > >                          pr_err("failed disconnecting src port\n");
> > >                          goto release_unlock;
> > >                  }
> > > -               src->connected->enabled = 0;
> > > +               src->connected->enabled = false;
> > >                  src->connected = NULL;
> > >          }
> > > @@ -1799,7 +1799,7 @@ int vchiq_mmal_component_disable(struct vchiq_mmal_instance *instance,
> > >          ret = disable_component(instance, component);
> > >          if (ret == 0)
> > > -               component->enabled = 0;
> > > +               component->enabled = false;
> > >          mutex_unlock(&instance->vchiq_mutex);
> > > diff --git a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.h b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.h
> > > index 6006e29232b3..70eda6cac1c6 100644
> > > --- a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.h
> > > +++ b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.h
> > > @@ -48,7 +48,7 @@ typedef void (*vchiq_mmal_buffer_cb)(
> > >                  int status, struct mmal_buffer *buffer);
> > >   struct vchiq_mmal_port {
> > > -       u32 enabled:1;
> > > +       bool enabled:1;
> > Is this a direct revert with 'git revert' ?
> 
> 
> No. It had conflicts plus I added the ':1' initialization to keep the logic
> same (in case 'enabled' gets used directly). Similar pattern come up with:
>     ($) git grep 'bool' -- '*.[h]' | grep '\:1'
> 
> So it shouldn't be an issue.

Please don't do that "bool foo:1" makes no sense.  Drop the ":1"
please.

thanks,

greg k-h
  

Hi Greg,

On 11/17/22 11:32 PM, Greg Kroah-Hartman wrote:
> On Thu, Nov 17, 2022 at 11:25:48PM +0530, Umang Jain wrote:
>> Hi Kieran,
>>
>> On 11/17/22 9:39 PM, Kieran Bingham wrote:
>>> Quoting Umang Jain (2022-11-17 16:00:13)
>>>> This reverts commit 640e77466e69d9c28de227bc76881f5501f532ca.
>>>>
>>>> In commit 7967656ffbfa ("coding-style: Clarify the expectations around
>>>> bool") the check to dis-allow bool structure members was removed from
>>>> checkpatch.pl. It promotes bool structure members to store boolean
>>>> values. This enhances code readability.
>>>>
>>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>>>> ---
>>>>    .../staging/vc04_services/vchiq-mmal/mmal-vchiq.c    | 12 ++++++------
>>>>    .../staging/vc04_services/vchiq-mmal/mmal-vchiq.h    |  4 ++--
>>>>    2 files changed, 8 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
>>>> index cb921c94996a..4abb6178cb9f 100644
>>>> --- a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
>>>> +++ b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
>>>> @@ -863,9 +863,9 @@ static int port_info_get(struct vchiq_mmal_instance *instance,
>>>>                   goto release_msg;
>>>>           if (rmsg->u.port_info_get_reply.port.is_enabled == 0)
>>>> -               port->enabled = 0;
>>>> +               port->enabled = false;
>>>>           else
>>>> -               port->enabled = 1;
>>>> +               port->enabled = true;
>>>>           /* copy the values out of the message */
>>>>           port->handle = rmsg->u.port_info_get_reply.port_handle;
>>>> @@ -1304,7 +1304,7 @@ static int port_disable(struct vchiq_mmal_instance *instance,
>>>>           if (!port->enabled)
>>>>                   return 0;
>>>> -       port->enabled = 0;
>>>> +       port->enabled = false;
>>>>           ret = port_action_port(instance, port,
>>>>                                  MMAL_MSG_PORT_ACTION_TYPE_DISABLE);
>>>> @@ -1359,7 +1359,7 @@ static int port_enable(struct vchiq_mmal_instance *instance,
>>>>           if (ret)
>>>>                   goto done;
>>>> -       port->enabled = 1;
>>>> +       port->enabled = true;
>>>>           if (port->buffer_cb) {
>>>>                   /* send buffer headers to videocore */
>>>> @@ -1531,7 +1531,7 @@ int vchiq_mmal_port_connect_tunnel(struct vchiq_mmal_instance *instance,
>>>>                           pr_err("failed disconnecting src port\n");
>>>>                           goto release_unlock;
>>>>                   }
>>>> -               src->connected->enabled = 0;
>>>> +               src->connected->enabled = false;
>>>>                   src->connected = NULL;
>>>>           }
>>>> @@ -1799,7 +1799,7 @@ int vchiq_mmal_component_disable(struct vchiq_mmal_instance *instance,
>>>>           ret = disable_component(instance, component);
>>>>           if (ret == 0)
>>>> -               component->enabled = 0;
>>>> +               component->enabled = false;
>>>>           mutex_unlock(&instance->vchiq_mutex);
>>>> diff --git a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.h b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.h
>>>> index 6006e29232b3..70eda6cac1c6 100644
>>>> --- a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.h
>>>> +++ b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.h
>>>> @@ -48,7 +48,7 @@ typedef void (*vchiq_mmal_buffer_cb)(
>>>>                   int status, struct mmal_buffer *buffer);
>>>>    struct vchiq_mmal_port {
>>>> -       u32 enabled:1;
>>>> +       bool enabled:1;
>>> Is this a direct revert with 'git revert' ?
>>
>> No. It had conflicts plus I added the ':1' initialization to keep the logic
>> same (in case 'enabled' gets used directly). Similar pattern come up with:
>>      ($) git grep 'bool' -- '*.[h]' | grep '\:1'
>>
>> So it shouldn't be an issue.
> Please don't do that "bool foo:1" makes no sense.  Drop the ":1"
> please.

It won't affect this patch but if you take a look at 2/3 - you'll see a 
bool flag 'in_use' that needs to be initialized (as it's getting used 
directly).

I can move the initialization part in the function (_init() or 
something) and drop the ":1" as you mentioned. That's  fine as well but 
I do find patterns of 'bool foo:1' in the codebase so I assumed it would 
be safe to use.


>   
>
> thanks,
>
> greg k-h
  

> > > > >    struct vchiq_mmal_port {
> > > > > -       u32 enabled:1;
> > > > > +       bool enabled:1;
> > > > Is this a direct revert with 'git revert' ?
> > > 
> > > No. It had conflicts plus I added the ':1' initialization to keep the logic
> > > same (in case 'enabled' gets used directly). Similar pattern come up with:
> > >      ($) git grep 'bool' -- '*.[h]' | grep '\:1'
> > > 
> > > So it shouldn't be an issue.
> > Please don't do that "bool foo:1" makes no sense.  Drop the ":1"
> > please.
> 
> It won't affect this patch but if you take a look at 2/3 - you'll see a bool
> flag 'in_use' that needs to be initialized (as it's getting used directly).
> 
> I can move the initialization part in the function (_init() or something)
> and drop the ":1" as you mentioned. That's  fine as well but I do find
> patterns of 'bool foo:1' in the codebase so I assumed it would be safe to
> use.

Does :1 really initialise the variable? In "u32 enabled:1" it means
this is a 1 bit wide bit field. It seems odd that bool is somehow
special and :1 means something else.

	Andrew
  

Hi,

On 11/18/22 5:42 AM, Andrew Lunn wrote:
>>>>>>     struct vchiq_mmal_port {
>>>>>> -       u32 enabled:1;
>>>>>> +       bool enabled:1;
>>>>> Is this a direct revert with 'git revert' ?
>>>> No. It had conflicts plus I added the ':1' initialization to keep the logic
>>>> same (in case 'enabled' gets used directly). Similar pattern come up with:
>>>>       ($) git grep 'bool' -- '*.[h]' | grep '\:1'
>>>>
>>>> So it shouldn't be an issue.
>>> Please don't do that "bool foo:1" makes no sense.  Drop the ":1"
>>> please.
>> It won't affect this patch but if you take a look at 2/3 - you'll see a bool
>> flag 'in_use' that needs to be initialized (as it's getting used directly).
>>
>> I can move the initialization part in the function (_init() or something)
>> and drop the ":1" as you mentioned. That's  fine as well but I do find
>> patterns of 'bool foo:1' in the codebase so I assumed it would be safe to
>> use.
> Does :1 really initialise the variable? In "u32 enabled:1" it means
> this is a 1 bit wide bit field. It seems odd that bool is somehow
> special and :1 means something else.


Yup you are correct - seems I mis-read :1 as initialization

>
> 	Andrew
  

On Fri, Nov 18, 2022 at 01:12:32AM +0100, Andrew Lunn wrote:
> 
> Does :1 really initialise the variable?

Obviously not.

> In "u32 enabled:1" it means
> this is a 1 bit wide bit field. It seems odd that bool is somehow
> special and :1 means something else.

If you have a bunch of consecutive bool a:1; bool b:1; then GCC will
squeeze them into the same byte.  But if you have bool a; bool b; then
they each take a byte with GCC.  I have specified GCC because the size
of bool types are a bit vague in the C standard.

regards,
dan carpenter