smiapp: re-use clamp_t instead of min(..., max(...))

PATCH In review Andy Shevchenko 4 participants 6 messages Jul 24, 2013 – Aug 8, 2013 Media (V4L/DVB)

lore

Share & tools

lore

Andy Shevchenko

author

2013-07-24 15:22 UTC #0 lore raw

PATCH 1/1 smiapp: re-use clamp_t instead of min(..., max(...))

clamp_t does the job to put a variable into the given range.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/media/i2c/smiapp/smiapp-core.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

drivers/media/i2c/smiapp/smiapp-core.c +6 -6

index 7ac7580..914e52f 100644
@@ -1835,12 +1835,12 @@ static void smiapp_set_compose_scaler(struct v4l2_subdev *subdev,
 		* sensor->limits[SMIAPP_LIMIT_SCALER_N_MIN]
 		/ sensor->limits[SMIAPP_LIMIT_MIN_X_OUTPUT_SIZE];
 
-	a = min(sensor->limits[SMIAPP_LIMIT_SCALER_M_MAX],
-		max(a, sensor->limits[SMIAPP_LIMIT_SCALER_M_MIN]));
-	b = min(sensor->limits[SMIAPP_LIMIT_SCALER_M_MAX],
-		max(b, sensor->limits[SMIAPP_LIMIT_SCALER_M_MIN]));
-	max_m = min(sensor->limits[SMIAPP_LIMIT_SCALER_M_MAX],
-		    max(max_m, sensor->limits[SMIAPP_LIMIT_SCALER_M_MIN]));
+	a = clamp_t(u32, a, sensor->limits[SMIAPP_LIMIT_SCALER_M_MIN],
+		    sensor->limits[SMIAPP_LIMIT_SCALER_M_MAX]);
+	b = clamp_t(u32, b, sensor->limits[SMIAPP_LIMIT_SCALER_M_MIN],
+		    sensor->limits[SMIAPP_LIMIT_SCALER_M_MAX]);
+	max_m = clamp_t(u32, max_m, sensor->limits[SMIAPP_LIMIT_SCALER_M_MIN],
+			sensor->limits[SMIAPP_LIMIT_SCALER_M_MAX]);
 
 	dev_dbg(&client->dev, "scaling: a %d b %d max_m %d\n", a, b, max_m);

Signature · 1 line

1.8.3.2

Sakari Ailus

2013-07-24 15:46 UTC #1 lore raw

Hi Andy,

Thanks for the patch.

(Drop Mauro from cc.)

Quoted 17 lines · Andy Shevchenko

clamp_t does the job to put a variable into the given range.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/media/i2c/smiapp/smiapp-core.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

drivers/media/i2c/smiapp/smiapp-core.c +0 -2

index 7ac7580..914e52f 100644
@@ -1835,12 +1835,12 @@ static void smiapp_set_compose_scaler(struct v4l2_subdev *subdev,
 		* sensor->limits[SMIAPP_LIMIT_SCALER_N_MIN]
 		/ sensor->limits[SMIAPP_LIMIT_MIN_X_OUTPUT_SIZE];
 
-	a = min(sensor->limits[SMIAPP_LIMIT_SCALER_M_MAX],
-		max(a, sensor->limits[SMIAPP_LIMIT_SCALER_M_MIN]));

drivers/media/i2c/smiapp/smiapp-core.c static void smiapp_set_compose_scaler(struct v4l2_subdev *subdev, · line 1843

Quoted 10 lines

drivers/media/i2c/smiapp/smiapp-core.c +6 -4

-	b = min(sensor->limits[SMIAPP_LIMIT_SCALER_M_MAX],
-		max(b, sensor->limits[SMIAPP_LIMIT_SCALER_M_MIN]));
-	max_m = min(sensor->limits[SMIAPP_LIMIT_SCALER_M_MAX],
-		    max(max_m, sensor->limits[SMIAPP_LIMIT_SCALER_M_MIN]));
+	a = clamp_t(u32, a, sensor->limits[SMIAPP_LIMIT_SCALER_M_MIN],
+		    sensor->limits[SMIAPP_LIMIT_SCALER_M_MAX]);
+	b = clamp_t(u32, b, sensor->limits[SMIAPP_LIMIT_SCALER_M_MIN],
+		    sensor->limits[SMIAPP_LIMIT_SCALER_M_MAX]);
+	max_m = clamp_t(u32, max_m, sensor->limits[SMIAPP_LIMIT_SCALER_M_MIN],
+			sensor->limits[SMIAPP_LIMIT_SCALER_M_MAX]);

Do you need clamp_t()? Wouldn't plain clamp() do?

I can change it if you're ok with that.

Signature · 4 lines

Cheers,

Sakari Ailus
e-mail: [email protected]	XMPP: [email protected]

Andy Shevchenko

2013-07-24 15:56 UTC #2 lore raw

On Wed, Jul 24, 2013 at 6:45 PM, Sakari Ailus <[email protected]> wrote:

[]

Quoted 4 lines

drivers/media/i2c/smiapp/smiapp-core.c static void smiapp_set_compose_scaler(struct v4l2_subdev *subdev, · line 1843
Quoted 2 lines
drivers/media/i2c/smiapp/smiapp-core.c +2 -0
+     max_m = clamp_t(u32, max_m, sensor->limits[SMIAPP_LIMIT_SCALER_M_MIN],
+                     sensor->limits[SMIAPP_LIMIT_SCALER_M_MAX]);
Do you need clamp_t()? Wouldn't plain clamp() do?

The *_t variants are preferred due to they are faster (no type checking).

Quoted 1 line

I can change it if you're ok with that.

I don't know why you may choose clamp instead of clamp_t here. Are you going to change variable types?

--
With Best Regards,
Andy Shevchenko

Sakari Ailus

2013-07-24 15:56 UTC #3 lore raw

Quoted 7 lines · Andy Shevchenko

On Wed, Jul 24, 2013 at 6:45 PM, Sakari Ailus <[email protected]> wrote:
[]
Quoted 3 lines
Quoted 2 lines
drivers/media/i2c/smiapp/smiapp-core.c +2 -0
+     max_m = clamp_t(u32, max_m, sensor->limits[SMIAPP_LIMIT_SCALER_M_MIN],
+                     sensor->limits[SMIAPP_LIMIT_SCALER_M_MAX]);

Quoted 8 lines

Quoted 1 line
Do you need clamp_t()? Wouldn't plain clamp() do?
The *_t variants are preferred due to they are faster (no type checking).
Quoted 1 line
I can change it if you're ok with that.
I don't know why you may choose clamp instead of clamp_t here. Are you going to change variable types?

Probably not. But clamp() would serve as a sanity check vs. clamp_t() which just does the thing. I'd prefer clamp() --- the compiler will not spend much time on it anyway.

Signature · 4 lines

Cheers,

Sakari Ailus
e-mail: [email protected]	XMPP: [email protected]

Andy Shevchenko

2013-07-25 07:47 UTC #4 lore raw

Quoted 12 lines · Sakari Ailus

Quoted 11 lines · Andy Shevchenko
On Wed, Jul 24, 2013 at 6:45 PM, Sakari Ailus <[email protected]> wrote:
[]
Quoted 4 lines
drivers/media/i2c/smiapp/smiapp-core.c static void smiapp_set_compose_scaler(struct v4l2_subdev *subdev, · line 1843
Quoted 2 lines
drivers/media/i2c/smiapp/smiapp-core.c +2 -0
+     max_m = clamp_t(u32, max_m, sensor->limits[SMIAPP_LIMIT_SCALER_M_MIN],
+                     sensor->limits[SMIAPP_LIMIT_SCALER_M_MAX]);
Do you need clamp_t()? Wouldn't plain clamp() do?
The *_t variants are preferred due to they are faster (no type checking).

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
I can change it if you're ok with that.
I don't know why you may choose clamp instead of clamp_t here. Are you going to change variable types?
Probably not. But clamp() would serve as a sanity check vs. clamp_t() which just does the thing. I'd prefer clamp()

You may adjust original patch if you want.

--
With Best Regards,
Andy Shevchenko

Laurent Pinchart

2013-08-08 22:42 UTC #5 lore raw

In reply to Sakari Ailus

Hi Sakari,

Quoted 13 lines · On Wednesday 24 July 2013 18:55:38 Sakari Ailus wrote:

Quoted 12 lines · Andy Shevchenko
On Wed, Jul 24, 2013 at 6:45 PM, Sakari Ailus <[email protected]> wrote:
[]
Quoted 5 lines
Quoted 3 lines
+ max_m = clamp_t(u32, max_m,
sensor->limits[SMIAPP_LIMIT_SCALER_M_MIN], +
sensor->limits[SMIAPP_LIMIT_SCALER_M_MAX]);
Do you need clamp_t()? Wouldn't plain clamp() do?
The *_t variants are preferred due to they are faster (no type checking).

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
I can change it if you're ok with that.
I don't know why you may choose clamp instead of clamp_t here. Are you going to change variable types?
Probably not. But clamp() would serve as a sanity check vs. clamp_t() which just does the thing. I'd prefer clamp() --- the compiler will not spend much time on it anyway.

Should I take this patch in my tree ? If so, could you please repost it with clamp() instead of clamp_t(), and your SoB or Acked-by ?

Signature · 3 lines

Regards,

Laurent Pinchart