Skip to contentSkip to conversation

i.MX8 ISI crossbar: simplify a couple of error messages

PATCH In review Krzysztof Hałasa 2 participants 4 messages May 9, 2025 – May 21, 2025

Subsystems (3)
Media (V4L/DVB) Arm i.MX and Layerscape NXP i.MX 8M ISI driver
lore
Share & tools
lore

Krzysztof Hałasa

author
#0 lore raw

PATCH 1/1 i.MX8 ISI crossbar: simplify a couple of error messages

I guess these messages were split from a single function. Or was it for saving memory? :-)

Signed-off-by Krzysztof Hałasa <[email protected]>
drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c +4 -6
index 93a55c97cd17..b6c7f114f9ed 100644
@@ -352,9 +352,8 @@ static int mxc_isi_crossbar_enable_streams(struct v4l2_subdev *sd,
 						 sink_streams);
 		if (ret) {
 			dev_err(xbar->isi->dev,
-				"failed to %s streams 0x%llx on '%s':%u: %d\n",
-				"enable", sink_streams, remote_sd->name,
-				remote_pad, ret);
+				"failed to enable streams 0x%llx on '%s':%u: %d\n",
+				sink_streams, remote_sd->name, remote_pad, ret);
 			mxc_isi_crossbar_gasket_disable(xbar, sink_pad);
 			return ret;
 		}

@@ -392,9 +391,8 @@ static int mxc_isi_crossbar_disable_streams(struct v4l2_subdev *sd,
 						  sink_streams);
 		if (ret)
 			dev_err(xbar->isi->dev,
-				"failed to %s streams 0x%llx on '%s':%u: %d\n",
-				"disable", sink_streams, remote_sd->name,
-				remote_pad, ret);
+				"failed to disable streams 0x%llx on '%s':%u: %d\n",
+				sink_streams, remote_sd->name, remote_pad, ret);
 
 		mxc_isi_crossbar_gasket_disable(xbar, sink_pad);
 	}
Signature · 5 lines
Krzysztof "Chris" Hałasa

Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa
maintainer
#1 lore raw

Hi Krzysztof,

Quoted 2 lines · Krzysztof Hałasa

I guess these messages were split from a single function. Or was it for saving memory? :-)

The goal was indeed to save memory.

Quoted 30 lines
Signed-off-by Krzysztof Hałasa <[email protected]>
drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c +4 -6
index 93a55c97cd17..b6c7f114f9ed 100644
@@ -352,9 +352,8 @@ static int mxc_isi_crossbar_enable_streams(struct v4l2_subdev *sd,
 						 sink_streams);
 		if (ret) {
 			dev_err(xbar->isi->dev,
-				"failed to %s streams 0x%llx on '%s':%u: %d\n",
-				"enable", sink_streams, remote_sd->name,
-				remote_pad, ret);
+				"failed to enable streams 0x%llx on '%s':%u: %d\n",
+				sink_streams, remote_sd->name, remote_pad, ret);
 			mxc_isi_crossbar_gasket_disable(xbar, sink_pad);
 			return ret;
 		}

@@ -392,9 +391,8 @@ static int mxc_isi_crossbar_disable_streams(struct v4l2_subdev *sd,
 						  sink_streams);
 		if (ret)
 			dev_err(xbar->isi->dev,
-				"failed to %s streams 0x%llx on '%s':%u: %d\n",
-				"disable", sink_streams, remote_sd->name,
-				remote_pad, ret);
+				"failed to disable streams 0x%llx on '%s':%u: %d\n",
+				sink_streams, remote_sd->name, remote_pad, ret);
 
 		mxc_isi_crossbar_gasket_disable(xbar, sink_pad);
 	}
Signature · 3 lines
Regards,

Laurent Pinchart
author
#2 lore raw

Hi Laurent,

Quoted 17 lines · Laurent Pinchart

The goal was indeed to save memory.

Quoted 15 lines
drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c +2 -3
@@ -352,9 +352,8 @@ static int mxc_isi_crossbar_enable_streams(struct v4l2_subdev *sd,
                                               sink_streams);
              if (ret) {
                      dev_err(xbar->isi->dev,
-                             "failed to %s streams 0x%llx on '%s':%u: %d\n",
-                             "enable", sink_streams, remote_sd->name,
-                             remote_pad, ret);
+                             "failed to enable streams 0x%llx on '%s':%u: %d\n",
+                             sink_streams, remote_sd->name, remote_pad, ret);
                      mxc_isi_crossbar_gasket_disable(xbar, sink_pad);
                      return ret;
              }

@@ -392,9 +391,8 @@ static int mxc_isi_crossbar_disable_streams(struct v4l2_subdev *sd,
Quoted 8 lines

drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c static int mxc_isi_crossbar_disable_streams(struct v4l2_subdev *sd, · line 395

Quoted 8 lines
drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c +2 -3
                                                sink_streams);
              if (ret)
                      dev_err(xbar->isi->dev,
-                             "failed to %s streams 0x%llx on '%s':%u: %d\n",
-                             "disable", sink_streams, remote_sd->name,
-                             remote_pad, ret);
+                             "failed to disable streams 0x%llx on '%s':%u: %d\n",
+                             sink_streams, remote_sd->name, remote_pad, ret);

It appears the current code saves (in my default build) 8 bytes of memory, at the cost of readability and inability to search with grep:

Current:

Name          Size      File off
.text         00000bf4  00000040
.data         00000000  00000c34
.rodata.str1.8 000001b8 00000c38
__jump_table  00000030  00000df0
.rodata       000001b8  00000e20

With patch:

Name          Size      File off
.text         00000bd4  00000040
.data         00000000  00000c14
.rodata.str1.8 000001e0 00000c18
__jump_table  00000030  00000df8
.rodata       000001b8  00000e28

Built as a module it doesn't even save that - the number of pages stays the same.

Just FYI.

Signature · 5 lines
Krzysztof "Chris" Hałasa

Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa
maintainer Reviewed-by
#3 lore raw

Hi Krzysztof,

Quoted 47 lines · Krzysztof Hałasa
Quoted 17 lines · Laurent Pinchart

The goal was indeed to save memory.

Quoted 15 lines
drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c +2 -3
@@ -352,9 +352,8 @@ static int mxc_isi_crossbar_enable_streams(struct v4l2_subdev *sd,
                                               sink_streams);
              if (ret) {
                      dev_err(xbar->isi->dev,
-                             "failed to %s streams 0x%llx on '%s':%u: %d\n",
-                             "enable", sink_streams, remote_sd->name,
-                             remote_pad, ret);
+                             "failed to enable streams 0x%llx on '%s':%u: %d\n",
+                             sink_streams, remote_sd->name, remote_pad, ret);
                      mxc_isi_crossbar_gasket_disable(xbar, sink_pad);
                      return ret;
              }

@@ -392,9 +391,8 @@ static int mxc_isi_crossbar_disable_streams(struct v4l2_subdev *sd,
Quoted 8 lines

drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c static int mxc_isi_crossbar_disable_streams(struct v4l2_subdev *sd, · line 395

Quoted 8 lines
drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c +2 -3
                                                sink_streams);
              if (ret)
                      dev_err(xbar->isi->dev,
-                             "failed to %s streams 0x%llx on '%s':%u: %d\n",
-                             "disable", sink_streams, remote_sd->name,
-                             remote_pad, ret);
+                             "failed to disable streams 0x%llx on '%s':%u: %d\n",
+                             sink_streams, remote_sd->name, remote_pad, ret);

It appears the current code saves (in my default build) 8 bytes of memory, at the cost of readability and inability to search with grep:

Current:

Name          Size      File off
.text         00000bf4  00000040
.data         00000000  00000c34
.rodata.str1.8 000001b8 00000c38
__jump_table  00000030  00000df0
.rodata       000001b8  00000e20

With patch:

Name          Size      File off
.text         00000bd4  00000040
.data         00000000  00000c14
.rodata.str1.8 000001e0 00000c18
__jump_table  00000030  00000df8
.rodata       000001b8  00000e28
Quoted 4 lines

Built as a module it doesn't even save that - the number of pages stays the same.

Just FYI.

Fair enough. I'll take the patch, with an update to the commit message:

media: nxp: imx8-isi: Simplify a couple of error messages

The error messages in the mxc_isi_crossbar_enable_streams() and mxc_isi_crossbar_disable_streams() functions are similar, with a single word difference between them. The word is moved out of the format string to a separate dev_err() argument to try and save memory through string de-duplication. The total savings are however small, as the .data size reduction is partly offset by a .text size increase, with a total saving of 8 bytes in total on an ARM64 platforms. They also come at the cost of making the error message difficult to grep, which outweights the gains.

Simplify the error messages to make them grep-able.

Reviewed-by Laurent Pinchart <[email protected]>
Signature · 3 lines
Regards,

Laurent Pinchart