| Message ID | 1374679278-9856-1-git-send-email-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Andy, Thanks for the patch. (Drop Mauro from cc.) On Wed, Jul 24, 2013 at 06:21:18PM +0300, Andy Shevchenko wrote: > clamp_t does the job to put a variable into the given range. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/media/i2c/smiapp/smiapp-core.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c > index 7ac7580..914e52f 100644 > --- a/drivers/media/i2c/smiapp/smiapp-core.c > +++ b/drivers/media/i2c/smiapp/smiapp-core.c > @@ -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]); Do you need clamp_t()? Wouldn't plain clamp() do? I can change it if you're ok with that.
On Wed, Jul 24, 2013 at 6:45 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote: [] >> + 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). > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 24, 2013 at 06:49:24PM +0300, Andy Shevchenko wrote: > On Wed, Jul 24, 2013 at 6:45 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote: > > [] > > >> + 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). > > > 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.
On Wed, Jul 24, 2013 at 6:55 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote: > On Wed, Jul 24, 2013 at 06:49:24PM +0300, Andy Shevchenko wrote: >> On Wed, Jul 24, 2013 at 6:45 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote: >> >> [] >> >> >> + 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). >> >> > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sakari, On Wednesday 24 July 2013 18:55:38 Sakari Ailus wrote: > On Wed, Jul 24, 2013 at 06:49:24PM +0300, Andy Shevchenko wrote: > > On Wed, Jul 24, 2013 at 6:45 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote: > > > > [] > > > > >> + 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). > > > > > 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 ?
diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c index 7ac7580..914e52f 100644 --- a/drivers/media/i2c/smiapp/smiapp-core.c +++ b/drivers/media/i2c/smiapp/smiapp-core.c @@ -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);
clamp_t does the job to put a variable into the given range. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/media/i2c/smiapp/smiapp-core.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)