linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] vc04_services: Promote bool usage
@ 2022-11-17 16:00 Umang Jain
  2022-11-17 16:00 ` [PATCH 1/3] Revert "staging: mmal-vchiq: Avoid use of bool in structures" Umang Jain
                   ` (3 more replies)
  0 siblings, 4 replies; 16+ messages in thread
From: Umang Jain @ 2022-11-17 16:00 UTC (permalink / raw)
  To: Florian Fainelli, Broadcom internal kernel review list, Ray Jui,
	Dan Carpenter, Greg Kroah-Hartman, Hans Verkuil, Dave Stevenson,
	Mauro Carvalho Chehab, linux-rpi-kernel, linux-arm-kernel,
	linux-staging, linux-kernel
  Cc: kieran.bingham, Umang Jain

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.

Umang Jain (3):
  Revert "staging: mmal-vchiq: Avoid use of bool in structures"
  vc04_services: mmal-vchiq: Use bool for vchiq_mmal_component.in_use
  vc04_services: bcm2835-camera: Use bool values for
    mmal_fmt.remove_padding

 .../bcm2835-camera/bcm2835-camera.c           | 30 +++++++++----------
 .../vc04_services/vchiq-mmal/mmal-vchiq.c     | 18 +++++------
 .../vc04_services/vchiq-mmal/mmal-vchiq.h     |  6 ++--
 3 files changed, 27 insertions(+), 27 deletions(-)

-- 
2.38.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 16+ messages in thread
* [PATCH 1/3] Revert "staging: mmal-vchiq: Avoid use of bool in structures"
  2022-11-17 16:00 [PATCH 0/3] vc04_services: Promote bool usage Umang Jain
@ 2022-11-17 16:00 ` Umang Jain
  2022-11-17 16:09   ` Kieran Bingham
  2022-11-17 16:00 ` [PATCH 2/3] vc04_services: mmal-vchiq: Use bool for vchiq_mmal_component.in_use Umang Jain
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 16+ messages in thread
From: Umang Jain @ 2022-11-17 16:00 UTC (permalink / raw)
  To: Florian Fainelli, Broadcom internal kernel review list, Ray Jui,
	Dan Carpenter, Greg Kroah-Hartman, Hans Verkuil, Dave Stevenson,
	Mauro Carvalho Chehab, linux-rpi-kernel, linux-arm-kernel,
	linux-staging, linux-kernel
  Cc: kieran.bingham, Umang Jain

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;
 	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;
 	u32 handle;  /* VideoCore handle for component */
 	u32 inputs;  /* Number of input ports */
 	u32 outputs; /* Number of output ports */
-- 
2.38.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 16+ messages in thread
* [PATCH 2/3] vc04_services: mmal-vchiq: Use bool for vchiq_mmal_component.in_use
  2022-11-17 16:00 [PATCH 0/3] vc04_services: Promote bool usage Umang Jain
  2022-11-17 16:00 ` [PATCH 1/3] Revert "staging: mmal-vchiq: Avoid use of bool in structures" Umang Jain
@ 2022-11-17 16:00 ` Umang Jain
  2022-11-18  0:23   ` Andrew Lunn
  2022-11-17 16:00 ` [PATCH 3/3] vc04_services: bcm2835-camera: Use bool values for mmal_fmt.remove_padding Umang Jain
  2022-11-17 18:11 ` [PATCH 0/3] vc04_services: Promote bool usage Stefan Wahren
  3 siblings, 1 reply; 16+ messages in thread
From: Umang Jain @ 2022-11-17 16:00 UTC (permalink / raw)
  To: Florian Fainelli, Broadcom internal kernel review list, Ray Jui,
	Dan Carpenter, Greg Kroah-Hartman, Hans Verkuil, Dave Stevenson,
	Mauro Carvalho Chehab, linux-rpi-kernel, linux-arm-kernel,
	linux-staging, linux-kernel
  Cc: kieran.bingham, Umang Jain

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>
---
 drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c | 6 +++---
 drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.h | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
index 4abb6178cb9f..294b184d4a49 100644
--- a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
+++ b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
@@ -1648,7 +1648,7 @@ int vchiq_mmal_component_init(struct vchiq_mmal_instance *instance,
 	for (idx = 0; idx < VCHIQ_MMAL_MAX_COMPONENTS; idx++) {
 		if (!instance->component[idx].in_use) {
 			component = &instance->component[idx];
-			component->in_use = 1;
+			component->in_use = true;
 			break;
 		}
 	}
@@ -1724,7 +1724,7 @@ int vchiq_mmal_component_init(struct vchiq_mmal_instance *instance,
 	destroy_component(instance, component);
 unlock:
 	if (component)
-		component->in_use = 0;
+		component->in_use = false;
 	mutex_unlock(&instance->vchiq_mutex);
 
 	return ret;
@@ -1747,7 +1747,7 @@ int vchiq_mmal_component_finalise(struct vchiq_mmal_instance *instance,
 
 	ret = destroy_component(instance, component);
 
-	component->in_use = 0;
+	component->in_use = 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 70eda6cac1c6..c5be86b0479d 100644
--- a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.h
+++ b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.h
@@ -82,7 +82,7 @@ struct vchiq_mmal_port {
 };
 
 struct vchiq_mmal_component {
-	u32 in_use:1;
+	bool in_use:1;
 	bool enabled:1;
 	u32 handle;  /* VideoCore handle for component */
 	u32 inputs;  /* Number of input ports */
-- 
2.38.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 16+ messages in thread
* [PATCH 3/3] vc04_services: bcm2835-camera: Use bool values for mmal_fmt.remove_padding
  2022-11-17 16:00 [PATCH 0/3] vc04_services: Promote bool usage Umang Jain
  2022-11-17 16:00 ` [PATCH 1/3] Revert "staging: mmal-vchiq: Avoid use of bool in structures" Umang Jain
  2022-11-17 16:00 ` [PATCH 2/3] vc04_services: mmal-vchiq: Use bool for vchiq_mmal_component.in_use Umang Jain
@ 2022-11-17 16:00 ` Umang Jain
  2022-11-17 18:11 ` [PATCH 0/3] vc04_services: Promote bool usage Stefan Wahren
  3 siblings, 0 replies; 16+ messages in thread
From: Umang Jain @ 2022-11-17 16:00 UTC (permalink / raw)
  To: Florian Fainelli, Broadcom internal kernel review list, Ray Jui,
	Dan Carpenter, Greg Kroah-Hartman, Hans Verkuil, Dave Stevenson,
	Mauro Carvalho Chehab, linux-rpi-kernel, linux-arm-kernel,
	linux-staging, linux-kernel
  Cc: kieran.bingham, Umang Jain

struct mmal_fmt.remove_padding is defined as a boolean type hence,
use boolean values for it instead of 0/1 integers. This enhances
code readability.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../bcm2835-camera/bcm2835-camera.c           | 30 +++++++++----------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
index fd456d1f7061..797ebe2a973a 100644
--- a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
+++ b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
@@ -87,21 +87,21 @@ static struct mmal_fmt formats[] = {
 		.depth = 12,
 		.mmal_component = COMP_CAMERA,
 		.ybbp = 1,
-		.remove_padding = 1,
+		.remove_padding = true,
 	}, {
 		.fourcc = V4L2_PIX_FMT_YUYV,
 		.mmal = MMAL_ENCODING_YUYV,
 		.depth = 16,
 		.mmal_component = COMP_CAMERA,
 		.ybbp = 2,
-		.remove_padding = 0,
+		.remove_padding = false,
 	}, {
 		.fourcc = V4L2_PIX_FMT_RGB24,
 		.mmal = MMAL_ENCODING_RGB24,
 		.depth = 24,
 		.mmal_component = COMP_CAMERA,
 		.ybbp = 3,
-		.remove_padding = 0,
+		.remove_padding = false,
 	}, {
 		.fourcc = V4L2_PIX_FMT_JPEG,
 		.flags = V4L2_FMT_FLAG_COMPRESSED,
@@ -109,7 +109,7 @@ static struct mmal_fmt formats[] = {
 		.depth = 8,
 		.mmal_component = COMP_IMAGE_ENCODE,
 		.ybbp = 0,
-		.remove_padding = 0,
+		.remove_padding = false,
 	}, {
 		.fourcc = V4L2_PIX_FMT_H264,
 		.flags = V4L2_FMT_FLAG_COMPRESSED,
@@ -117,7 +117,7 @@ static struct mmal_fmt formats[] = {
 		.depth = 8,
 		.mmal_component = COMP_VIDEO_ENCODE,
 		.ybbp = 0,
-		.remove_padding = 0,
+		.remove_padding = false,
 	}, {
 		.fourcc = V4L2_PIX_FMT_MJPEG,
 		.flags = V4L2_FMT_FLAG_COMPRESSED,
@@ -125,63 +125,63 @@ static struct mmal_fmt formats[] = {
 		.depth = 8,
 		.mmal_component = COMP_VIDEO_ENCODE,
 		.ybbp = 0,
-		.remove_padding = 0,
+		.remove_padding = false,
 	}, {
 		.fourcc = V4L2_PIX_FMT_YVYU,
 		.mmal = MMAL_ENCODING_YVYU,
 		.depth = 16,
 		.mmal_component = COMP_CAMERA,
 		.ybbp = 2,
-		.remove_padding = 0,
+		.remove_padding = false,
 	}, {
 		.fourcc = V4L2_PIX_FMT_VYUY,
 		.mmal = MMAL_ENCODING_VYUY,
 		.depth = 16,
 		.mmal_component = COMP_CAMERA,
 		.ybbp = 2,
-		.remove_padding = 0,
+		.remove_padding = false,
 	}, {
 		.fourcc = V4L2_PIX_FMT_UYVY,
 		.mmal = MMAL_ENCODING_UYVY,
 		.depth = 16,
 		.mmal_component = COMP_CAMERA,
 		.ybbp = 2,
-		.remove_padding = 0,
+		.remove_padding = false,
 	}, {
 		.fourcc = V4L2_PIX_FMT_NV12,
 		.mmal = MMAL_ENCODING_NV12,
 		.depth = 12,
 		.mmal_component = COMP_CAMERA,
 		.ybbp = 1,
-		.remove_padding = 1,
+		.remove_padding = true,
 	}, {
 		.fourcc = V4L2_PIX_FMT_BGR24,
 		.mmal = MMAL_ENCODING_BGR24,
 		.depth = 24,
 		.mmal_component = COMP_CAMERA,
 		.ybbp = 3,
-		.remove_padding = 0,
+		.remove_padding = false,
 	}, {
 		.fourcc = V4L2_PIX_FMT_YVU420,
 		.mmal = MMAL_ENCODING_YV12,
 		.depth = 12,
 		.mmal_component = COMP_CAMERA,
 		.ybbp = 1,
-		.remove_padding = 1,
+		.remove_padding = true,
 	}, {
 		.fourcc = V4L2_PIX_FMT_NV21,
 		.mmal = MMAL_ENCODING_NV21,
 		.depth = 12,
 		.mmal_component = COMP_CAMERA,
 		.ybbp = 1,
-		.remove_padding = 1,
+		.remove_padding = true,
 	}, {
 		.fourcc = V4L2_PIX_FMT_BGR32,
 		.mmal = MMAL_ENCODING_BGRA,
 		.depth = 32,
 		.mmal_component = COMP_CAMERA,
 		.ybbp = 4,
-		.remove_padding = 0,
+		.remove_padding = false,
 	},
 };
 
@@ -1147,7 +1147,7 @@ static int mmal_setup_components(struct bcm2835_mmal_dev *dev,
 	struct vchiq_mmal_port *port = NULL, *camera_port = NULL;
 	struct vchiq_mmal_component *encode_component = NULL;
 	struct mmal_fmt *mfmt = get_format(f);
-	u32 remove_padding;
+	bool remove_padding;
 
 	if (!mfmt)
 		return -EINVAL;
-- 
2.38.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] Revert "staging: mmal-vchiq: Avoid use of bool in structures"
  2022-11-17 16:00 ` [PATCH 1/3] Revert "staging: mmal-vchiq: Avoid use of bool in structures" Umang Jain
@ 2022-11-17 16:09   ` Kieran Bingham
  2022-11-17 17:55     ` Umang Jain
  0 siblings, 1 reply; 16+ messages in thread
From: Kieran Bingham @ 2022-11-17 16:09 UTC (permalink / raw)
  To: Broadcom internal kernel review list, Dan Carpenter,
	Dave Stevenson, Florian Fainelli, Greg Kroah-Hartman,
	Hans Verkuil, Mauro Carvalho Chehab, Ray Jui, Umang Jain,
	linux-arm-kernel, linux-kernel, linux-rpi-kernel, linux-staging
  Cc: Umang Jain

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
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] Revert "staging: mmal-vchiq: Avoid use of bool in structures"
  2022-11-17 16:09   ` Kieran Bingham
@ 2022-11-17 17:55     ` Umang Jain
  2022-11-17 18:02       ` Greg Kroah-Hartman
  0 siblings, 1 reply; 16+ messages in thread
From: Umang Jain @ 2022-11-17 17:55 UTC (permalink / raw)
  To: Kieran Bingham, Broadcom internal kernel review list,
	Dan Carpenter, Dave Stevenson, Florian Fainelli,
	Greg Kroah-Hartman, Hans Verkuil, Mauro Carvalho Chehab, Ray Jui,
	linux-arm-kernel, linux-kernel, linux-rpi-kernel, linux-staging

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
>>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] Revert "staging: mmal-vchiq: Avoid use of bool in structures"
  2022-11-17 17:55     ` Umang Jain
@ 2022-11-17 18:02       ` Greg Kroah-Hartman
  2022-11-17 18:22         ` Umang Jain
  0 siblings, 1 reply; 16+ messages in thread
From: Greg Kroah-Hartman @ 2022-11-17 18:02 UTC (permalink / raw)
  To: Umang Jain
  Cc: Kieran Bingham, Broadcom internal kernel review list,
	Dan Carpenter, Dave Stevenson, Florian Fainelli, Hans Verkuil,
	Mauro Carvalho Chehab, Ray Jui, linux-arm-kernel, linux-kernel,
	linux-rpi-kernel, linux-staging

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] vc04_services: Promote bool usage
  2022-11-17 16:00 [PATCH 0/3] vc04_services: Promote bool usage Umang Jain
                   ` (2 preceding siblings ...)
  2022-11-17 16:00 ` [PATCH 3/3] vc04_services: bcm2835-camera: Use bool values for mmal_fmt.remove_padding Umang Jain
@ 2022-11-17 18:11 ` Stefan Wahren
  2022-11-18  8:47   ` Dan Carpenter
  2022-11-18 17:25   ` Dave Stevenson
  3 siblings, 2 replies; 16+ messages in thread
From: Stefan Wahren @ 2022-11-17 18:11 UTC (permalink / raw)
  To: Dave Stevenson
  Cc: kieran.bingham, Mauro Carvalho Chehab, Hans Verkuil, linux-kernel,
	linux-rpi-kernel, linux-arm-kernel, Umang Jain, Ray Jui,
	Broadcom internal kernel review list, Dan Carpenter,
	Florian Fainelli, Greg Kroah-Hartman, linux-staging

Hi Dave,

Am 17.11.22 um 17:00 schrieb Umang Jain:
> 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.
>
> Umang Jain (3):
>    Revert "staging: mmal-vchiq: Avoid use of bool in structures"
>    vc04_services: mmal-vchiq: Use bool for vchiq_mmal_component.in_use
>    vc04_services: bcm2835-camera: Use bool values for
>      mmal_fmt.remove_padding
>
>   .../bcm2835-camera/bcm2835-camera.c           | 30 +++++++++----------
>   .../vc04_services/vchiq-mmal/mmal-vchiq.c     | 18 +++++------
>   .../vc04_services/vchiq-mmal/mmal-vchiq.h     |  6 ++--
could you please check these changes to be safe? I'm not sure that the 
affected declarations are really internal. I'm afraid this might affect 
firmware or userspace.
>   3 files changed, 27 insertions(+), 27 deletions(-)
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] Revert "staging: mmal-vchiq: Avoid use of bool in structures"
  2022-11-17 18:02       ` Greg Kroah-Hartman
@ 2022-11-17 18:22         ` Umang Jain
  2022-11-18  0:12           ` Andrew Lunn
  0 siblings, 1 reply; 16+ messages in thread
From: Umang Jain @ 2022-11-17 18:22 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Kieran Bingham, Broadcom internal kernel review list,
	Dan Carpenter, Dave Stevenson, Florian Fainelli, Hans Verkuil,
	Mauro Carvalho Chehab, Ray Jui, linux-arm-kernel, linux-kernel,
	linux-rpi-kernel, linux-staging

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] Revert "staging: mmal-vchiq: Avoid use of bool in structures"
  2022-11-17 18:22         ` Umang Jain
@ 2022-11-18  0:12           ` Andrew Lunn
  2022-11-18  3:58             ` Umang Jain
  2022-11-18  8:35             ` Dan Carpenter
  0 siblings, 2 replies; 16+ messages in thread
From: Andrew Lunn @ 2022-11-18  0:12 UTC (permalink / raw)
  To: Umang Jain
  Cc: Greg Kroah-Hartman, Kieran Bingham,
	Broadcom internal kernel review list, Dan Carpenter,
	Dave Stevenson, Florian Fainelli, Hans Verkuil,
	Mauro Carvalho Chehab, Ray Jui, linux-arm-kernel, linux-kernel,
	linux-rpi-kernel, linux-staging

> > > > >    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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] vc04_services: mmal-vchiq: Use bool for vchiq_mmal_component.in_use
  2022-11-17 16:00 ` [PATCH 2/3] vc04_services: mmal-vchiq: Use bool for vchiq_mmal_component.in_use Umang Jain
@ 2022-11-18  0:23   ` Andrew Lunn
  2022-11-18  9:01     ` Dan Carpenter
  0 siblings, 1 reply; 16+ messages in thread
From: Andrew Lunn @ 2022-11-18  0:23 UTC (permalink / raw)
  To: Umang Jain
  Cc: Florian Fainelli, Broadcom internal kernel review list, Ray Jui,
	Dan Carpenter, Greg Kroah-Hartman, Hans Verkuil, Dave Stevenson,
	Mauro Carvalho Chehab, linux-rpi-kernel, linux-arm-kernel,
	linux-staging, linux-kernel, kieran.bingham

>  struct vchiq_mmal_component {
> -	u32 in_use:1;
> +	bool in_use:1;
>  	bool enabled:1;

The patch you referenced says:

+If a structure has many true/false values, consider consolidating them into a
+bitfield with 1 bit members, or using an appropriate fixed width type, such as
+u8.

The code did exactly this, using two bits fields, in one u32. A bool
probably takes up 4 bytes, maybe 8 bytes, so this change probably
doubles the storage size for these two fields. Are these fields on the
hot path, where an extra AND instruction would make a difference?

    Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] Revert "staging: mmal-vchiq: Avoid use of bool in structures"
  2022-11-18  0:12           ` Andrew Lunn
@ 2022-11-18  3:58             ` Umang Jain
  2022-11-18  8:35             ` Dan Carpenter
  1 sibling, 0 replies; 16+ messages in thread
From: Umang Jain @ 2022-11-18  3:58 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Greg Kroah-Hartman, Kieran Bingham,
	Broadcom internal kernel review list, Dan Carpenter,
	Dave Stevenson, Florian Fainelli, Hans Verkuil,
	Mauro Carvalho Chehab, Ray Jui, linux-arm-kernel, linux-kernel,
	linux-rpi-kernel, linux-staging

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] Revert "staging: mmal-vchiq: Avoid use of bool in structures"
  2022-11-18  0:12           ` Andrew Lunn
  2022-11-18  3:58             ` Umang Jain
@ 2022-11-18  8:35             ` Dan Carpenter
  1 sibling, 0 replies; 16+ messages in thread
From: Dan Carpenter @ 2022-11-18  8:35 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Umang Jain, Greg Kroah-Hartman, Kieran Bingham,
	Broadcom internal kernel review list, Dave Stevenson,
	Florian Fainelli, Hans Verkuil, Mauro Carvalho Chehab, Ray Jui,
	linux-arm-kernel, linux-kernel, linux-rpi-kernel, linux-staging

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] vc04_services: Promote bool usage
  2022-11-17 18:11 ` [PATCH 0/3] vc04_services: Promote bool usage Stefan Wahren
@ 2022-11-18  8:47   ` Dan Carpenter
  2022-11-18 17:25   ` Dave Stevenson
  1 sibling, 0 replies; 16+ messages in thread
From: Dan Carpenter @ 2022-11-18  8:47 UTC (permalink / raw)
  To: Stefan Wahren
  Cc: Dave Stevenson, kieran.bingham, Mauro Carvalho Chehab,
	Hans Verkuil, linux-kernel, linux-rpi-kernel, linux-arm-kernel,
	Umang Jain, Ray Jui, Broadcom internal kernel review list,
	Florian Fainelli, Greg Kroah-Hartman, linux-staging

On Thu, Nov 17, 2022 at 07:11:41PM +0100, Stefan Wahren wrote:
> Hi Dave,
> 
> Am 17.11.22 um 17:00 schrieb Umang Jain:
> > 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.
> > 
> > Umang Jain (3):
> >    Revert "staging: mmal-vchiq: Avoid use of bool in structures"
> >    vc04_services: mmal-vchiq: Use bool for vchiq_mmal_component.in_use
> >    vc04_services: bcm2835-camera: Use bool values for
> >      mmal_fmt.remove_padding
> > 
> >   .../bcm2835-camera/bcm2835-camera.c           | 30 +++++++++----------
> >   .../vc04_services/vchiq-mmal/mmal-vchiq.c     | 18 +++++------
> >   .../vc04_services/vchiq-mmal/mmal-vchiq.h     |  6 ++--
> could you please check these changes to be safe? I'm not sure that the
> affected declarations are really internal. I'm afraid this might affect
> firmware or userspace.

The structs have a bunch of kernel pointers in them so hopefully they
are internal.  Otherwise we have a different sort of problem.

regards,
dan carpenter


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] vc04_services: mmal-vchiq: Use bool for vchiq_mmal_component.in_use
  2022-11-18  0:23   ` Andrew Lunn
@ 2022-11-18  9:01     ` Dan Carpenter
  0 siblings, 0 replies; 16+ messages in thread
From: Dan Carpenter @ 2022-11-18  9:01 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Umang Jain, Florian Fainelli,
	Broadcom internal kernel review list, Ray Jui, Greg Kroah-Hartman,
	Hans Verkuil, Dave Stevenson, Mauro Carvalho Chehab,
	linux-rpi-kernel, linux-arm-kernel, linux-staging, linux-kernel,
	kieran.bingham

On Fri, Nov 18, 2022 at 01:23:36AM +0100, Andrew Lunn wrote:
> >  struct vchiq_mmal_component {
> > -	u32 in_use:1;
> > +	bool in_use:1;
> >  	bool enabled:1;
> 
> The patch you referenced says:
> 
> +If a structure has many true/false values, consider consolidating them into a
> +bitfield with 1 bit members, or using an appropriate fixed width type, such as
> +u8.
> 
> The code did exactly this, using two bits fields, in one u32. A bool
> probably takes up 4 bytes, maybe 8 bytes, so this change probably
> doubles the storage size for these two fields.

In GCC and Clang bools take a byte, but the C language is vague and
other compilers are free to do it differently.

> Are these fields on the
> hot path, where an extra AND instruction would make a difference?

This patch takes the first u32 for "in_use" and squeezes it into the
same byte as "enabled" so it makes the struct four bytes smaller.  There
is still a 3 byte struct hole between "enabled" and "handle" so we could
add more 62 bool bitfields if we wanted.

In the v2 patch these become:

	bool in_use;
	bool enabled;

One byte each and there is a two byte gap before "handle".

regards,
dan carpenter


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] vc04_services: Promote bool usage
  2022-11-17 18:11 ` [PATCH 0/3] vc04_services: Promote bool usage Stefan Wahren
  2022-11-18  8:47   ` Dan Carpenter
@ 2022-11-18 17:25   ` Dave Stevenson
  1 sibling, 0 replies; 16+ messages in thread
From: Dave Stevenson @ 2022-11-18 17:25 UTC (permalink / raw)
  To: Stefan Wahren
  Cc: kieran.bingham, Mauro Carvalho Chehab, Hans Verkuil, linux-kernel,
	linux-rpi-kernel, linux-arm-kernel, Umang Jain, Ray Jui,
	Broadcom internal kernel review list, Dan Carpenter,
	Florian Fainelli, Greg Kroah-Hartman, linux-staging

Hi Stefan

On Thu, 17 Nov 2022 at 18:11, Stefan Wahren <stefan.wahren@i2se.com> wrote:
>
> Hi Dave,
>
> Am 17.11.22 um 17:00 schrieb Umang Jain:
> > 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.
> >
> > Umang Jain (3):
> >    Revert "staging: mmal-vchiq: Avoid use of bool in structures"
> >    vc04_services: mmal-vchiq: Use bool for vchiq_mmal_component.in_use
> >    vc04_services: bcm2835-camera: Use bool values for
> >      mmal_fmt.remove_padding
> >
> >   .../bcm2835-camera/bcm2835-camera.c           | 30 +++++++++----------
> >   .../vc04_services/vchiq-mmal/mmal-vchiq.c     | 18 +++++------
> >   .../vc04_services/vchiq-mmal/mmal-vchiq.h     |  6 ++--
> could you please check these changes to be safe? I'm not sure that the
> affected declarations are really internal. I'm afraid this might affect
> firmware or userspace.

No problem. These are totally safe.
As I've commented on the v2 patch, if it were in the mmal-msg*.h files
then I'd be more concerned as those are matching the firmware
structures, but these are just internal state.

  Dave

> >   3 files changed, 27 insertions(+), 27 deletions(-)
> >

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 16+ messages in thread
end of thread, other threads:[~2022-11-18 17:27 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-11-17 16:00 [PATCH 0/3] vc04_services: Promote bool usage Umang Jain
2022-11-17 16:00 ` [PATCH 1/3] Revert "staging: mmal-vchiq: Avoid use of bool in structures" Umang Jain
2022-11-17 16:09   ` Kieran Bingham
2022-11-17 17:55     ` Umang Jain
2022-11-17 18:02       ` Greg Kroah-Hartman
2022-11-17 18:22         ` Umang Jain
2022-11-18  0:12           ` Andrew Lunn
2022-11-18  3:58             ` Umang Jain
2022-11-18  8:35             ` Dan Carpenter
2022-11-17 16:00 ` [PATCH 2/3] vc04_services: mmal-vchiq: Use bool for vchiq_mmal_component.in_use Umang Jain
2022-11-18  0:23   ` Andrew Lunn
2022-11-18  9:01     ` Dan Carpenter
2022-11-17 16:00 ` [PATCH 3/3] vc04_services: bcm2835-camera: Use bool values for mmal_fmt.remove_padding Umang Jain
2022-11-17 18:11 ` [PATCH 0/3] vc04_services: Promote bool usage Stefan Wahren
2022-11-18  8:47   ` Dan Carpenter
2022-11-18 17:25   ` Dave Stevenson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).