soc_camera: match signedness of soc_camera_limit_side()

From: Márton Németh <[email protected]>

The parameters of soc_camera_limit_side() are either a pointer to a structure element from v4l2_rect, or constants. The structure elements of the v4l2_rect are signed (see <linux/videodev2.h>) so do the computations also with signed values.

This will remove the following sparse warning (see "make C=1"):
* incorrect type in argument 1 (different signedness) expected unsigned int *start
got signed int *<noident>

Signed-off-by Márton Németh <[email protected]>

---
diff -r 2a50a0a1c951 linux/include/media/soc_camera.h

linux/include/media/soc_camera.h +2 -3

@@ -264,9 +264,8 @@
 		common_flags;
 }
 
-static inline void soc_camera_limit_side(unsigned int *start,
-		unsigned int *length, unsigned int start_min,
-		unsigned int length_min, unsigned int length_max)
+static inline void soc_camera_limit_side(int *start, int *length,
+		int start_min, int length_min, int length_max)
 {
 	if (*length < length_min)
 		*length = length_min;

diff -r 2a50a0a1c951 linux/drivers/media/video/rj54n1cb0c.c

linux/drivers/media/video/rj54n1cb0c.c +2 -2

@@ -555,15 +555,15 @@
 	return ret;
 }
 
-static int rj54n1_sensor_scale(struct v4l2_subdev *sd, u32 *in_w, u32 *in_h,
-			       u32 *out_w, u32 *out_h);
+static int rj54n1_sensor_scale(struct v4l2_subdev *sd, s32 *in_w, s32 *in_h,
+			       s32 *out_w, s32 *out_h);

linux/drivers/media/video/rj54n1cb0c.c +2 -2

@@ -638,8 +638,8 @@
  * the output one, updates the window sizes and returns an error or the resize
  * coefficient on success. Note: we only use the "Fixed Scaling" on this camera.
  */
-static int rj54n1_sensor_scale(struct v4l2_subdev *sd, u32 *in_w, u32 *in_h,
-			       u32 *out_w, u32 *out_h)
+static int rj54n1_sensor_scale(struct v4l2_subdev *sd, s32 *in_w, s32 *in_h,
+			       s32 *out_w, s32 *out_h)
 {
 	struct i2c_client *client = sd->priv;
 	struct rj54n1 *rj54n1 = to_rj54n1(client);

Quoted 11 lines · Németh Márton

From: Márton Németh <[email protected]>

The parameters of soc_camera_limit_side() are either a pointer to a structure element from v4l2_rect, or constants. The structure elements of the v4l2_rect are signed (see <linux/videodev2.h>) so do the computations also with signed values.

This will remove the following sparse warning (see "make C=1"):
* incorrect type in argument 1 (different signedness) expected unsigned int *start
got signed int *<noident>

Well, it is interesting, but insufficient. And, unfortunately, I don't have a good (and easy) recipe for how to fix this properly.

The problem is, that in soc_camera_limit_side all tests and arithmetics are performed with unsigned in mind, now, if you change them to signed, think what happens, if some of them are negative. No, I don't know when negative members of struct v4l2_rect make sense, having them signed doesn't seem a very good idea to me. But they cannot be changed - that's a part of the user-space API...

Casting all parameters inside that inline to unsigned would be way too ugly. Maybe we could at least keep start_min, length_min, and length_max unsigned, and only change start and length to signed, and only cast those two inside the function. Then, if you grep through all the drivers, there's only one location, where soc_camera_limit_side() is called with the latter 3 parameters not constant - two calls in sh_mobile_ceu_camera.c. So, to keep sparse happy, you'd have to cast there. Ideally, you would also add checks there for negative values...

Quoted 28 lines

Signed-off-by Márton Németh <[email protected]>

---
diff -r 2a50a0a1c951 linux/include/media/soc_camera.h

linux/include/media/soc_camera.h +2 -3

@@ -264,9 +264,8 @@
 		common_flags;
 }
 
-static inline void soc_camera_limit_side(unsigned int *start,
-		unsigned int *length, unsigned int start_min,
-		unsigned int length_min, unsigned int length_max)
+static inline void soc_camera_limit_side(int *start, int *length,
+		int start_min, int length_min, int length_max)
 {
 	if (*length < length_min)
 		*length = length_min;

diff -r 2a50a0a1c951 linux/drivers/media/video/rj54n1cb0c.c

linux/drivers/media/video/rj54n1cb0c.c +2 -2

@@ -555,15 +555,15 @@
 	return ret;
 }
 
-static int rj54n1_sensor_scale(struct v4l2_subdev *sd, u32 *in_w, u32 *in_h,
-			       u32 *out_w, u32 *out_h);
+static int rj54n1_sensor_scale(struct v4l2_subdev *sd, s32 *in_w, s32 *in_h,
+			       s32 *out_w, s32 *out_h);

linux/drivers/media/video/rj54n1cb0c.c line 568

Quoted 10 lines

linux/drivers/media/video/rj54n1cb0c.c +1 -1

 
 static int rj54n1_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
 {
 	struct i2c_client *client = sd->priv;
 	struct rj54n1 *rj54n1 = to_rj54n1(client);
 	struct v4l2_rect *rect = &a->c;
-	unsigned int dummy = 0, output_w, output_h,
+	int dummy = 0, output_w, output_h,
 		input_w = rect->width, input_h = rect->height;
 	int ret;

And these:

	if (output_w > max(512U, input_w / 2)) {
	if (output_h > max(384U, input_h / 2)) {

would now produce compiler warnings... Have you actually tried to compile your patch? You'll also have to change formats in dev_dbg() calls here...

Quoted 13 lines

linux/drivers/media/video/rj54n1cb0c.c +2 -2

@@ -638,8 +638,8 @@
  * the output one, updates the window sizes and returns an error or the resize
  * coefficient on success. Note: we only use the "Fixed Scaling" on this camera.
  */
-static int rj54n1_sensor_scale(struct v4l2_subdev *sd, u32 *in_w, u32 *in_h,
-			       u32 *out_w, u32 *out_h)
+static int rj54n1_sensor_scale(struct v4l2_subdev *sd, s32 *in_w, s32 *in_h,
+			       s32 *out_w, s32 *out_h)
 {
 	struct i2c_client *client = sd->priv;
 	struct rj54n1 *rj54n1 = to_rj54n1(client);

linux/drivers/media/video/rj54n1cb0c.c line 1022

Quoted 7 lines

linux/drivers/media/video/rj54n1cb0c.c +1 -1

@@ -1017,7 +1017,7 @@
 	struct i2c_client *client = sd->priv;
 	struct rj54n1 *rj54n1 = to_rj54n1(client);
 	const struct rj54n1_datafmt *fmt;
-	unsigned int output_w, output_h, max_w, max_h,
+	int output_w, output_h, max_w, max_h,
 		input_w = rj54n1->rect.width, input_h = rj54n1->rect.height;
 	int ret;

and here.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Guennadi Liakhovetski wrote:

Quoted 7 lines

And these:

	if (output_w > max(512U, input_w / 2)) {
	if (output_h > max(384U, input_h / 2)) {

would now produce compiler warnings... Have you actually tried to compile your patch? You'll also have to change formats in dev_dbg() calls here...

Interesting to hear about compiler warnings. I don't get them if I apply the patch on top of version 14064:31eaa9423f98 of http://linuxtv.org/hg/v4l-dvb/ . What is your compiler version?

diff -r 31eaa9423f98 linux/include/media/soc_camera.h

linux/include/media/soc_camera.h +0 -0

@@ -264,9 +264,8 @@
 		common_flags;

You didn't reply to my most important objection:

Quoted 7 lines · Németh Márton

diff -r 31eaa9423f98 linux/include/media/soc_camera.h

linux/include/media/soc_camera.h +0 -0

@@ -264,9 +264,8 @@
 		common_flags;
 }
 

linux/include/media/soc_camera.h line 271

Quoted 8 lines

linux/include/media/soc_camera.h +2 -3

-static inline void soc_camera_limit_side(unsigned int *start,
-		unsigned int *length, unsigned int start_min,
-		unsigned int length_min, unsigned int length_max)
+static inline void soc_camera_limit_side(int *start, int *length,
+		int start_min, int length_min, int length_max)
 {
 	if (*length < length_min)
 		*length = length_min;

I still do not believe this function will work equally well with signed parameters, as it works with unsigned ones.

Quoted 11 lines · Guennadi Liakhovetski

You didn't reply to my most important objection:

Quoted 7 lines · Németh Márton

diff -r 31eaa9423f98 linux/include/media/soc_camera.h

linux/include/media/soc_camera.h +0 -0

@@ -264,9 +264,8 @@
 		common_flags;
 }
 

Quoted 11 lines

linux/include/media/soc_camera.h line 271

Quoted 8 lines

linux/include/media/soc_camera.h +2 -3

-static inline void soc_camera_limit_side(unsigned int *start,
-		unsigned int *length, unsigned int start_min,
-		unsigned int length_min, unsigned int length_max)
+static inline void soc_camera_limit_side(int *start, int *length,
+		int start_min, int length_min, int length_max)
 {
 	if (*length < length_min)
 		*length = length_min;

I still do not believe this function will work equally well with signed parameters, as it works with unsigned ones.

I implemented some test cases to find out whether the soc_camera_limit_side() works correctly or not. My biggest problem is that I'm not sure what is the expected working of the soc_camera_limit_side() function.

Nevertheless I tried expect some values, you can probably verify whether my expectations are correct or not (see the test attached).

The signed and unsigned version of the function works differently because the unsigned version cannot accept negative values. These values will be implicitly casted to an unsigned value which means that they will be interpreted as a big positive value.

Here are the test results:

Test Case 1: PASSED
Test Case 2: PASSED
Test Case 3: FAILED: start=50, length=8, start_unsigned=0, length_unsigned=1600 Test Case 4: PASSED
Test Case 5: PASSED
Test Case 6: PASSED
Test Case 7: PASSED
Test Case 8: PASSED

There is a difference in case 3, but which is the correct one?

Regard,

Márton Németh

Quoted 22 lines · Németh Márton

Quoted 21 lines · Guennadi Liakhovetski

You didn't reply to my most important objection:

Quoted 5 lines · Németh Márton

diff -r 31eaa9423f98 linux/include/media/soc_camera.h

linux/include/media/soc_camera.h +0 -0

@@ -264,9 +264,8 @@
 		common_flags;

linux/include/media/soc_camera.h line 271

Quoted 10 lines

linux/include/media/soc_camera.h +2 -3

 }
 
-static inline void soc_camera_limit_side(unsigned int *start,
-		unsigned int *length, unsigned int start_min,
-		unsigned int length_min, unsigned int length_max)
+static inline void soc_camera_limit_side(int *start, int *length,
+		int start_min, int length_min, int length_max)
 {
 	if (*length < length_min)
 		*length = length_min;

I still do not believe this function will work equally well with signed parameters, as it works with unsigned ones.

Quoted 3 lines

I implemented some test cases to find out whether the soc_camera_limit_side() works correctly or not. My biggest problem is that I'm not sure what is the expected working of the soc_camera_limit_side() function.

Well, the expected behaviour is simple: the function treats all its parameters as unsigned, and puts the former two input/output parameters within the limits, provided by the latter three parameters. Well, taking

into account, that when comparing a signed and an unsigned integers, the comparison is performed unsigned, I think, it should be ok to do what I suggested in the last email: change prototype to

(quoted diff) +3 -0

+static inline void soc_camera_limit_side(int *start, int *length,
+		unsigned int start_min, unsigned int length_min, 
+		unsigned int length_max)

Maybe also provide a comment above the function explaining, why the first two parameters are signed. And cast explicitly in sh_mobile_ceu_camera.c:

	soc_camera_limit_side(&rect->left, &rect->width,
			      (unsigned int)cap.bounds.left, 2,
			      (unsigned int)cap.bounds.width);
	soc_camera_limit_side(&rect->top, &rect->height,
			      (unsigned int)cap.bounds.top, 4,
			      (unsigned int)cap.bounds.height);

Could you check if this would make both sparse and the compiler happy?

Quoted 28 lines · Guennadi Liakhovetski

Quoted 25 lines · Németh Márton

Quoted 11 lines · Guennadi Liakhovetski

You didn't reply to my most important objection:

Quoted 7 lines · Németh Márton

diff -r 31eaa9423f98 linux/include/media/soc_camera.h

linux/include/media/soc_camera.h +0 -0

@@ -264,9 +264,8 @@
 		common_flags;
 }
 

Quoted 10 lines

linux/include/media/soc_camera.h line 271

Quoted 8 lines

linux/include/media/soc_camera.h +2 -3

-static inline void soc_camera_limit_side(unsigned int *start,
-		unsigned int *length, unsigned int start_min,
-		unsigned int length_min, unsigned int length_max)
+static inline void soc_camera_limit_side(int *start, int *length,
+		int start_min, int length_min, int length_max)
 {
 	if (*length < length_min)
 		*length = length_min;

I still do not believe this function will work equally well with signed parameters, as it works with unsigned ones.

I implemented some test cases to find out whether the soc_camera_limit_side() works correctly or not. My biggest problem is that I'm not sure what is the expected working of the soc_camera_limit_side() function.

Quoted 3 lines

Well, the expected behaviour is simple: the function treats all its parameters as unsigned, and puts the former two input/output parameters within the limits, provided by the latter three parameters. Well, taking

For the length parameter it is clear to put them between length_min and length_max. But for start there is only one limit given: start_min. Does this mean that any *start value bigger than start_min is acceptable?

(I would like to find out the meaning, not to read back what is written in the source code because it is no use to define test cases out of the source code.)

Quoted 11 lines

into account, that when comparing a signed and an unsigned integers, the comparison is performed unsigned, I think, it should be ok to do what I suggested in the last email: change prototype to

(quoted diff) +3 -0

+static inline void soc_camera_limit_side(int *start, int *length,
+		unsigned int start_min, unsigned int length_min, 
+		unsigned int length_max)

Maybe also provide a comment above the function explaining, why the first two parameters are signed. And cast explicitly in sh_mobile_ceu_camera.c:

Quoted 6 lines

	soc_camera_limit_side(&rect->left, &rect->width,
			      (unsigned int)cap.bounds.left, 2,
			      (unsigned int)cap.bounds.width);
	soc_camera_limit_side(&rect->top, &rect->height,
			      (unsigned int)cap.bounds.top, 4,
			      (unsigned int)cap.bounds.height);

I'm afraid that casting __s32 to unsigned int just cannot work. Let's take an example on a 32bit machine:

               -1 = 0xFFFFFFFF

(unsigned int)-1 = 0xFFFFFFFF = 4294967295

and

               -2147483648 = 0x80000000

(unsigned int)-2147483648 = 0x80000000 = 2147483648

This means that any negative number will be mapped to a large positive number when casting to (unsigned int) and I think this is not the wanted behaviour.

Quoted 1 line

Could you check if this would make both sparse and the compiler happy?

There is no compiler warning nor sparse warning when applying the following version of the patch. I'm not sure, however, that the simple cast will do the right thing here.

Ok, I modified your patch a bit, how about the below version? If you agree, I'll commit it in that form (after converting to utf-8):

From: Márton Németh <[email protected]>

The first two parameters of soc_camera_limit_side() are usually pointers to struct v4l2_rect elements. They are signed, so adjust the prototype accordingly.

This will remove the following sparse warning (see "make C=1"):

* incorrect type in argument 1 (different signedness) expected unsigned int *start
got signed int *<noident>

as well as a couple more signedness mismatches.

Signed-off-by Márton Németh <[email protected]>

Signed-off-by Guennadi Liakhovetski <[email protected]>

---

drivers/media/video/mt9v022.c +1 -1

index 1a34d29..e5bae4c 100644
@@ -325,7 +325,7 @@ static int mt9v022_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
 	if (ret < 0)
 		return ret;
 
-	dev_dbg(&client->dev, "Frame %ux%u pixel\n", rect.width, rect.height);
+	dev_dbg(&client->dev, "Frame %dx%d pixel\n", rect.width, rect.height);
 
 	mt9v022->rect = rect;
 

drivers/media/video/mx3_camera.c +2 -2

index bd297f5..d477e30 100644
@@ -796,7 +796,7 @@ static int acquire_dma_channel(struct mx3_camera_dev *mx3_cam)
  * FIXME: learn to use stride != width, then we can keep stride properly aligned
  * and support arbitrary (even) widths.
  */
-static inline void stride_align(__s32 *width)
+static inline void stride_align(__u32 *width)
 {
 	if (((*width + 7) &  ~7) < 4096)
 		*width = (*width + 7) &  ~7;

@@ -844,7 +844,7 @@ static int mx3_camera_set_crop(struct soc_camera_device *icd,
 		 * So far only direct camera-to-memory is supported
 		 */
 		if (channel_change_requested(icd, rect)) {
-			int ret = acquire_dma_channel(mx3_cam);
+			ret = acquire_dma_channel(mx3_cam);
 			if (ret < 0)
 				return ret;
 		}

drivers/media/video/rj54n1cb0c.c +9 -9

index 9277194..bbd9c11 100644
@@ -555,15 +555,15 @@ static int rj54n1_commit(struct i2c_client *client)
 	return ret;
 }
 
-static int rj54n1_sensor_scale(struct v4l2_subdev *sd, u32 *in_w, u32 *in_h,
-			       u32 *out_w, u32 *out_h);
+static int rj54n1_sensor_scale(struct v4l2_subdev *sd, s32 *in_w, s32 *in_h,
+			       s32 *out_w, s32 *out_h);
 
 static int rj54n1_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
 {
 	struct i2c_client *client = sd->priv;
 	struct rj54n1 *rj54n1 = to_rj54n1(client);
 	struct v4l2_rect *rect = &a->c;
-	unsigned int dummy = 0, output_w, output_h,
+	int dummy = 0, output_w, output_h,
 		input_w = rect->width, input_h = rect->height;
 	int ret;
 

@@ -577,7 +577,7 @@ static int rj54n1_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
 	output_w = (input_w * 1024 + rj54n1->resize / 2) / rj54n1->resize;
 	output_h = (input_h * 1024 + rj54n1->resize / 2) / rj54n1->resize;
 
-	dev_dbg(&client->dev, "Scaling for %ux%u : %u = %ux%u\n",
+	dev_dbg(&client->dev, "Scaling for %dx%d : %u = %dx%d\n",
 		input_w, input_h, rj54n1->resize, output_w, output_h);
 
 	ret = rj54n1_sensor_scale(sd, &input_w, &input_h, &output_w, &output_h);

@@ -638,8 +638,8 @@ static int rj54n1_g_fmt(struct v4l2_subdev *sd,
  * the output one, updates the window sizes and returns an error or the resize
  * coefficient on success. Note: we only use the "Fixed Scaling" on this camera.
  */
-static int rj54n1_sensor_scale(struct v4l2_subdev *sd, u32 *in_w, u32 *in_h,
-			       u32 *out_w, u32 *out_h)
+static int rj54n1_sensor_scale(struct v4l2_subdev *sd, s32 *in_w, s32 *in_h,
+			       s32 *out_w, s32 *out_h)
 {
 	struct i2c_client *client = sd->priv;
 	struct rj54n1 *rj54n1 = to_rj54n1(client);

@@ -749,7 +749,7 @@ static int rj54n1_sensor_scale(struct v4l2_subdev *sd, u32 *in_w, u32 *in_h,
 	 * improve the image quality or stability for larger frames (see comment
 	 * above), but I didn't check the framerate.
 	 */
-	skip = min(resize / 1024, (unsigned)15);
+	skip = min(resize / 1024, 15U);
 
 	inc_sel = 1 << skip;
 

@@ -819,7 +819,7 @@ static int rj54n1_sensor_scale(struct v4l2_subdev *sd, u32 *in_w, u32 *in_h,
 	*out_w = output_w;
 	*out_h = output_h;
 
-	dev_dbg(&client->dev, "Scaled for %ux%u : %u = %ux%u, skip %u\n",
+	dev_dbg(&client->dev, "Scaled for %dx%d : %u = %ux%u, skip %u\n",
 		*in_w, *in_h, resize, output_w, output_h, skip);
 
 	return resize;

@@ -1017,7 +1017,7 @@ static int rj54n1_s_fmt(struct v4l2_subdev *sd,
 	struct i2c_client *client = sd->priv;
 	struct rj54n1 *rj54n1 = to_rj54n1(client);
 	const struct rj54n1_datafmt *fmt;
-	unsigned int output_w, output_h, max_w, max_h,
+	int output_w, output_h, max_w, max_h,
 		input_w = rj54n1->rect.width, input_h = rj54n1->rect.height;
 	int ret;
 

drivers/media/video/sh_mobile_ceu_camera.c +4 -4

index f09c714..af506bc 100644
@@ -1040,13 +1040,13 @@ static int client_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *crop,
 	 */
 	if (!memcmp(rect, cam_rect, sizeof(*rect))) {
 		/* Even if camera S_CROP failed, but camera rectangle matches */
-		dev_dbg(dev, "Camera S_CROP successful for %ux%u@%u:%u\n",
+		dev_dbg(dev, "Camera S_CROP successful for %dx%d@%d:%d\n",
 			rect->width, rect->height, rect->left, rect->top);
 		return 0;
 	}
 
 	/* Try to fix cropping, that camera hasn't managed to set */
-	dev_geo(dev, "Fix camera S_CROP for %ux%u@%u:%u to %ux%u@%u:%u\n",
+	dev_geo(dev, "Fix camera S_CROP for %dx%d@%d:%d to %dx%d@%d:%d\n",
 		cam_rect->width, cam_rect->height,
 		cam_rect->left, cam_rect->top,
 		rect->width, rect->height, rect->left, rect->top);

@@ -1102,7 +1102,7 @@ static int client_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *crop,
 
 		v4l2_subdev_call(sd, video, s_crop, cam_crop);
 		ret = client_g_rect(sd, cam_rect);
-		dev_geo(dev, "Camera S_CROP %d for %ux%u@%u:%u\n", ret,
+		dev_geo(dev, "Camera S_CROP %d for %dx%d@%d:%d\n", ret,
 			cam_rect->width, cam_rect->height,
 			cam_rect->left, cam_rect->top);
 	}

@@ -1116,7 +1116,7 @@ static int client_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *crop,
 		*cam_rect = cap.bounds;
 		v4l2_subdev_call(sd, video, s_crop, cam_crop);
 		ret = client_g_rect(sd, cam_rect);
-		dev_geo(dev, "Camera S_CROP %d for max %ux%u@%u:%u\n", ret,
+		dev_geo(dev, "Camera S_CROP %d for max %dx%d@%d:%d\n", ret,
 			cam_rect->width, cam_rect->height,
 			cam_rect->left, cam_rect->top);
 	}

include/media/soc_camera.h +0 -0

index dcc5b86..5a17365 100644

Hi Németh

Quoted 15 lines · Guennadi Liakhovetski

Ok, I modified your patch a bit, how about the below version? If you agree, I'll commit it in that form (after converting to utf-8):

From: Márton Németh <[email protected]>

The first two parameters of soc_camera_limit_side() are usually pointers to struct v4l2_rect elements. They are signed, so adjust the prototype accordingly.

This will remove the following sparse warning (see "make C=1"):

* incorrect type in argument 1 (different signedness) expected unsigned int *start
got signed int *<noident>

Quoted 4 lines

as well as a couple more signedness mismatches.

Signed-off-by Márton Németh <[email protected]>

Signed-off-by Guennadi Liakhovetski <[email protected]>

please confirm, if you agree with my version of your patch, or I'll have to leave it out from my pull request.

Quoted 150 lines

---

drivers/media/video/mt9v022.c +1 -1

index 1a34d29..e5bae4c 100644
@@ -325,7 +325,7 @@ static int mt9v022_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
 	if (ret < 0)
 		return ret;
 
-	dev_dbg(&client->dev, "Frame %ux%u pixel\n", rect.width, rect.height);
+	dev_dbg(&client->dev, "Frame %dx%d pixel\n", rect.width, rect.height);
 
 	mt9v022->rect = rect;
 

drivers/media/video/mx3_camera.c +2 -2

index bd297f5..d477e30 100644
@@ -796,7 +796,7 @@ static int acquire_dma_channel(struct mx3_camera_dev *mx3_cam)
  * FIXME: learn to use stride != width, then we can keep stride properly aligned
  * and support arbitrary (even) widths.
  */
-static inline void stride_align(__s32 *width)
+static inline void stride_align(__u32 *width)
 {
 	if (((*width + 7) &  ~7) < 4096)
 		*width = (*width + 7) &  ~7;

@@ -844,7 +844,7 @@ static int mx3_camera_set_crop(struct soc_camera_device *icd,
 		 * So far only direct camera-to-memory is supported
 		 */
 		if (channel_change_requested(icd, rect)) {
-			int ret = acquire_dma_channel(mx3_cam);
+			ret = acquire_dma_channel(mx3_cam);
 			if (ret < 0)
 				return ret;
 		}

drivers/media/video/rj54n1cb0c.c +9 -9

index 9277194..bbd9c11 100644
@@ -555,15 +555,15 @@ static int rj54n1_commit(struct i2c_client *client)
 	return ret;
 }
 
-static int rj54n1_sensor_scale(struct v4l2_subdev *sd, u32 *in_w, u32 *in_h,
-			       u32 *out_w, u32 *out_h);
+static int rj54n1_sensor_scale(struct v4l2_subdev *sd, s32 *in_w, s32 *in_h,
+			       s32 *out_w, s32 *out_h);
 
 static int rj54n1_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
 {
 	struct i2c_client *client = sd->priv;
 	struct rj54n1 *rj54n1 = to_rj54n1(client);
 	struct v4l2_rect *rect = &a->c;
-	unsigned int dummy = 0, output_w, output_h,
+	int dummy = 0, output_w, output_h,
 		input_w = rect->width, input_h = rect->height;
 	int ret;
 

@@ -577,7 +577,7 @@ static int rj54n1_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
 	output_w = (input_w * 1024 + rj54n1->resize / 2) / rj54n1->resize;
 	output_h = (input_h * 1024 + rj54n1->resize / 2) / rj54n1->resize;
 
-	dev_dbg(&client->dev, "Scaling for %ux%u : %u = %ux%u\n",
+	dev_dbg(&client->dev, "Scaling for %dx%d : %u = %dx%d\n",
 		input_w, input_h, rj54n1->resize, output_w, output_h);
 
 	ret = rj54n1_sensor_scale(sd, &input_w, &input_h, &output_w, &output_h);

@@ -638,8 +638,8 @@ static int rj54n1_g_fmt(struct v4l2_subdev *sd,
  * the output one, updates the window sizes and returns an error or the resize
  * coefficient on success. Note: we only use the "Fixed Scaling" on this camera.
  */
-static int rj54n1_sensor_scale(struct v4l2_subdev *sd, u32 *in_w, u32 *in_h,
-			       u32 *out_w, u32 *out_h)
+static int rj54n1_sensor_scale(struct v4l2_subdev *sd, s32 *in_w, s32 *in_h,
+			       s32 *out_w, s32 *out_h)
 {
 	struct i2c_client *client = sd->priv;
 	struct rj54n1 *rj54n1 = to_rj54n1(client);

@@ -749,7 +749,7 @@ static int rj54n1_sensor_scale(struct v4l2_subdev *sd, u32 *in_w, u32 *in_h,
 	 * improve the image quality or stability for larger frames (see comment
 	 * above), but I didn't check the framerate.
 	 */
-	skip = min(resize / 1024, (unsigned)15);
+	skip = min(resize / 1024, 15U);
 
 	inc_sel = 1 << skip;
 

@@ -819,7 +819,7 @@ static int rj54n1_sensor_scale(struct v4l2_subdev *sd, u32 *in_w, u32 *in_h,
 	*out_w = output_w;
 	*out_h = output_h;
 
-	dev_dbg(&client->dev, "Scaled for %ux%u : %u = %ux%u, skip %u\n",
+	dev_dbg(&client->dev, "Scaled for %dx%d : %u = %ux%u, skip %u\n",
 		*in_w, *in_h, resize, output_w, output_h, skip);
 
 	return resize;

@@ -1017,7 +1017,7 @@ static int rj54n1_s_fmt(struct v4l2_subdev *sd,
 	struct i2c_client *client = sd->priv;
 	struct rj54n1 *rj54n1 = to_rj54n1(client);
 	const struct rj54n1_datafmt *fmt;
-	unsigned int output_w, output_h, max_w, max_h,
+	int output_w, output_h, max_w, max_h,
 		input_w = rj54n1->rect.width, input_h = rj54n1->rect.height;
 	int ret;
 

drivers/media/video/sh_mobile_ceu_camera.c +4 -4

index f09c714..af506bc 100644
@@ -1040,13 +1040,13 @@ static int client_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *crop,
 	 */
 	if (!memcmp(rect, cam_rect, sizeof(*rect))) {
 		/* Even if camera S_CROP failed, but camera rectangle matches */
-		dev_dbg(dev, "Camera S_CROP successful for %ux%u@%u:%u\n",
+		dev_dbg(dev, "Camera S_CROP successful for %dx%d@%d:%d\n",
 			rect->width, rect->height, rect->left, rect->top);
 		return 0;
 	}
 
 	/* Try to fix cropping, that camera hasn't managed to set */
-	dev_geo(dev, "Fix camera S_CROP for %ux%u@%u:%u to %ux%u@%u:%u\n",
+	dev_geo(dev, "Fix camera S_CROP for %dx%d@%d:%d to %dx%d@%d:%d\n",
 		cam_rect->width, cam_rect->height,
 		cam_rect->left, cam_rect->top,
 		rect->width, rect->height, rect->left, rect->top);

@@ -1102,7 +1102,7 @@ static int client_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *crop,
 
 		v4l2_subdev_call(sd, video, s_crop, cam_crop);
 		ret = client_g_rect(sd, cam_rect);
-		dev_geo(dev, "Camera S_CROP %d for %ux%u@%u:%u\n", ret,
+		dev_geo(dev, "Camera S_CROP %d for %dx%d@%d:%d\n", ret,
 			cam_rect->width, cam_rect->height,
 			cam_rect->left, cam_rect->top);
 	}

@@ -1116,7 +1116,7 @@ static int client_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *crop,
 		*cam_rect = cap.bounds;
 		v4l2_subdev_call(sd, video, s_crop, cam_crop);
 		ret = client_g_rect(sd, cam_rect);
-		dev_geo(dev, "Camera S_CROP %d for max %ux%u@%u:%u\n", ret,
+		dev_geo(dev, "Camera S_CROP %d for max %dx%d@%d:%d\n", ret,
 			cam_rect->width, cam_rect->height,
 			cam_rect->left, cam_rect->top);
 	}

include/media/soc_camera.h +0 -0

index dcc5b86..5a17365 100644

include/media/soc_camera.h static inline unsigned long soc_camera_bus_param_compatible( · line 273

Quoted 10 lines

include/media/soc_camera.h +2 -2

@@ -264,8 +266,8 @@ static inline unsigned long soc_camera_bus_param_compatible(
 		common_flags;
 }
 
-static inline void soc_camera_limit_side(unsigned int *start,
-		unsigned int *length, unsigned int start_min,
+static inline void soc_camera_limit_side(int *start, int *length,
+		unsigned int start_min,
 		unsigned int length_min, unsigned int length_max)
 {
 	if (*length < length_min)

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/