* [PATCH] soc_camera: match signedness of soc_camera_limit_side()
@ 2010-01-23 13:43 Németh Márton
2010-01-27 16:05 ` Guennadi Liakhovetski
0 siblings, 1 reply; 12+ messages in thread
From: Németh Márton @ 2010-01-23 13:43 UTC (permalink / raw)
To: Guennadi Liakhovetski; +Cc: V4L Mailing List
From: Márton Németh <nm127@freemail.hu>
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 <nm127@freemail.hu>
---
diff -r 2a50a0a1c951 linux/include/media/soc_camera.h
--- a/linux/include/media/soc_camera.h Sat Jan 23 00:14:32 2010 -0200
+++ b/linux/include/media/soc_camera.h Sat Jan 23 10:09:41 2010 +0100
@@ -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
--- a/linux/drivers/media/video/rj54n1cb0c.c Sat Jan 23 00:14:32 2010 -0200
+++ b/linux/drivers/media/video/rj54n1cb0c.c Sat Jan 23 10:09:41 2010 +0100
@@ -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);
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;
@@ -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);
@@ -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;
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] soc_camera: match signedness of soc_camera_limit_side() 2010-01-23 13:43 [PATCH] soc_camera: match signedness of soc_camera_limit_side() Németh Márton @ 2010-01-27 16:05 ` Guennadi Liakhovetski 2010-01-27 18:11 ` Németh Márton 0 siblings, 1 reply; 12+ messages in thread From: Guennadi Liakhovetski @ 2010-01-27 16:05 UTC (permalink / raw) To: Németh Márton; +Cc: V4L Mailing List On Sat, 23 Jan 2010, Németh Márton wrote: > From: Márton Németh <nm127@freemail.hu> > > 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... > > Signed-off-by: Márton Németh <nm127@freemail.hu> > --- > diff -r 2a50a0a1c951 linux/include/media/soc_camera.h > --- a/linux/include/media/soc_camera.h Sat Jan 23 00:14:32 2010 -0200 > +++ b/linux/include/media/soc_camera.h Sat Jan 23 10:09:41 2010 +0100 > @@ -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 > --- a/linux/drivers/media/video/rj54n1cb0c.c Sat Jan 23 00:14:32 2010 -0200 > +++ b/linux/drivers/media/video/rj54n1cb0c.c Sat Jan 23 10:09:41 2010 +0100 > @@ -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); > > 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... > > @@ -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); > @@ -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/ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] soc_camera: match signedness of soc_camera_limit_side() 2010-01-27 16:05 ` Guennadi Liakhovetski @ 2010-01-27 18:11 ` Németh Márton 2010-01-27 18:22 ` Guennadi Liakhovetski 0 siblings, 1 reply; 12+ messages in thread From: Németh Márton @ 2010-01-27 18:11 UTC (permalink / raw) To: Guennadi Liakhovetski; +Cc: V4L Mailing List [-- Attachment #1: Type: text/plain, Size: 8235 bytes --] Guennadi Liakhovetski wrote: > On Sat, 23 Jan 2010, Németh Márton wrote: > >> From: Márton Németh <nm127@freemail.hu> >> >> 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... I'll have a look to see what can be done to handle the negative values properly. >> Signed-off-by: Márton Németh <nm127@freemail.hu> >> --- >> diff -r 2a50a0a1c951 linux/include/media/soc_camera.h >> --- a/linux/include/media/soc_camera.h Sat Jan 23 00:14:32 2010 -0200 >> +++ b/linux/include/media/soc_camera.h Sat Jan 23 10:09:41 2010 +0100 >> @@ -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 >> --- a/linux/drivers/media/video/rj54n1cb0c.c Sat Jan 23 00:14:32 2010 -0200 >> +++ b/linux/drivers/media/video/rj54n1cb0c.c Sat Jan 23 10:09:41 2010 +0100 >> @@ -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); >> >> 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... 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? I used the attached configuration. Maybe some other options has to be turned on? localhost:/usr/src/linuxtv.org/v4l-dvb$ patch -p1 <../soc_camera_signedness.patch patching file linux/include/media/soc_camera.h patching file linux/drivers/media/video/rj54n1cb0c.c localhost:/usr/src/linuxtv.org/v4l-dvb$ make C=1 2>&1 |tee result1.txt make -C /usr/src/linuxtv.org/v4l-dvb/v4l make[1]: Entering directory `/usr/src/linuxtv.org/v4l-dvb/v4l' creating symbolic links... make -C firmware prep make[2]: Entering directory `/usr/src/linuxtv.org/v4l-dvb/v4l/firmware' make[2]: Leaving directory `/usr/src/linuxtv.org/v4l-dvb/v4l/firmware' make -C firmware make[2]: Entering directory `/usr/src/linuxtv.org/v4l-dvb/v4l/firmware' make[2]: Nothing to be done for `default'. make[2]: Leaving directory `/usr/src/linuxtv.org/v4l-dvb/v4l/firmware' Kernel build directory is /lib/modules/2.6.33-rc4/build make -C /lib/modules/2.6.33-rc4/build SUBDIRS=/usr/src/linuxtv.org/v4l-dvb/v4l modules make[2]: Entering directory `/usr/src/linuxtv.org/pinchartl/uvcvideo/v4l-dvb' CHECK /usr/src/linuxtv.org/v4l-dvb/v4l/mt9m001.c CC [M] /usr/src/linuxtv.org/v4l-dvb/v4l/mt9m001.o CHECK /usr/src/linuxtv.org/v4l-dvb/v4l/mt9m111.c CC [M] /usr/src/linuxtv.org/v4l-dvb/v4l/mt9m111.o CHECK /usr/src/linuxtv.org/v4l-dvb/v4l/mt9t031.c CC [M] /usr/src/linuxtv.org/v4l-dvb/v4l/mt9t031.o CHECK /usr/src/linuxtv.org/v4l-dvb/v4l/mt9t112.c CC [M] /usr/src/linuxtv.org/v4l-dvb/v4l/mt9t112.o CHECK /usr/src/linuxtv.org/v4l-dvb/v4l/mt9v022.c CC [M] /usr/src/linuxtv.org/v4l-dvb/v4l/mt9v022.o CHECK /usr/src/linuxtv.org/v4l-dvb/v4l/ov772x.c CC [M] /usr/src/linuxtv.org/v4l-dvb/v4l/ov772x.o CHECK /usr/src/linuxtv.org/v4l-dvb/v4l/ov9640.c CC [M] /usr/src/linuxtv.org/v4l-dvb/v4l/ov9640.o CHECK /usr/src/linuxtv.org/v4l-dvb/v4l/rj54n1cb0c.c CC [M] /usr/src/linuxtv.org/v4l-dvb/v4l/rj54n1cb0c.o CHECK /usr/src/linuxtv.org/v4l-dvb/v4l/tw9910.c CC [M] /usr/src/linuxtv.org/v4l-dvb/v4l/tw9910.o CHECK /usr/src/linuxtv.org/v4l-dvb/v4l/soc_camera.c CC [M] /usr/src/linuxtv.org/v4l-dvb/v4l/soc_camera.o CHECK /usr/src/linuxtv.org/v4l-dvb/v4l/soc_camera_platform.c CC [M] /usr/src/linuxtv.org/v4l-dvb/v4l/soc_camera_platform.o Building modules, stage 2. MODPOST 28 modules LD [M] /usr/src/linuxtv.org/v4l-dvb/v4l/mt9m001.ko LD [M] /usr/src/linuxtv.org/v4l-dvb/v4l/mt9m111.ko LD [M] /usr/src/linuxtv.org/v4l-dvb/v4l/mt9t031.ko LD [M] /usr/src/linuxtv.org/v4l-dvb/v4l/mt9t112.ko LD [M] /usr/src/linuxtv.org/v4l-dvb/v4l/mt9v022.ko LD [M] /usr/src/linuxtv.org/v4l-dvb/v4l/ov772x.ko LD [M] /usr/src/linuxtv.org/v4l-dvb/v4l/ov9640.ko LD [M] /usr/src/linuxtv.org/v4l-dvb/v4l/rj54n1cb0c.ko LD [M] /usr/src/linuxtv.org/v4l-dvb/v4l/soc_camera.ko LD [M] /usr/src/linuxtv.org/v4l-dvb/v4l/soc_camera_platform.ko LD [M] /usr/src/linuxtv.org/v4l-dvb/v4l/tw9910.ko make[2]: Leaving directory `/usr/src/linuxtv.org/pinchartl/uvcvideo/v4l-dvb' ./scripts/rmmod.pl check found 28 modules make[1]: Leaving directory `/usr/src/linuxtv.org/v4l-dvb/v4l' nmarci@asus-eeepc:/usr/src/linuxtv.org/v4l-dvb$ gcc --version gcc (Debian 4.3.4-6) 4.3.4 Copyright (C) 2008 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. > >> @@ -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); >> @@ -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/ > > Regards, Márton Németh [-- Attachment #2: .config --] [-- Type: text/plain, Size: 4716 bytes --] # # Automatically generated make config: don't edit # Linux kernel version: KERNELVERSION # Wed Jan 27 18:58:24 2010 # # CONFIG_SND_MIRO is not set CONFIG_INPUT=y CONFIG_USB=m CONFIG_FW_LOADER=y # CONFIG_SPARC64 is not set # CONFIG_PLAT_M32700UT is not set # CONFIG_SND_FM801 is not set CONFIG_FB_CFB_IMAGEBLIT=y # CONFIG_GPIO_PCA953X is not set # CONFIG_HAVE_CLK is not set # CONFIG_FIQ is not set CONFIG_SND=m # CONFIG_MT9M001_PCA9536_SWITCH is not set # CONFIG_ARCH_OMAP2 is not set # CONFIG_SPARC32 is not set CONFIG_I2C_ALGOBIT=m # CONFIG_SND_ISA is not set CONFIG_INET=y CONFIG_CRC32=y CONFIG_SYSFS=y CONFIG_MMC=m CONFIG_ISA=y CONFIG_PCI=y CONFIG_PARPORT_1284=y CONFIG_FB_CFB_FILLRECT=y # CONFIG_MT9V022_PCA9536_SWITCH is not set CONFIG_VIRT_TO_BUS=y CONFIG_PARPORT=m # CONFIG_ARCH_DAVINCI_DM644x is not set # CONFIG_FIREWIRE is not set CONFIG_NET=y # CONFIG_ARCH_DAVINCI is not set CONFIG_FB_CFB_COPYAREA=y # CONFIG_PXA27x is not set # CONFIG_SGI_IP22 is not set CONFIG_I2C=m # CONFIG_ARCH_DAVINCI_DM355 is not set CONFIG_MODULES=y # CONFIG_TUNER_XC2028 is not set CONFIG_HAS_IOMEM=y # CONFIG_MACH_DAVINCI_DM6467_EVM is not set CONFIG_HAS_DMA=y CONFIG_FB=y # CONFIG_ARCH_MX1 is not set # CONFIG_SONY_LAPTOP is not set # CONFIG_MX3_IPU is not set CONFIG_SND_PCM=m CONFIG_EXPERIMENTAL=y # CONFIG_M32R is not set # CONFIG_IEEE1394 is not set # CONFIG_VIDEO_KERNEL_VERSION is not set CONFIG_MEDIA_SUPPORT=m # # Multimedia core support # CONFIG_VIDEO_DEV=m CONFIG_VIDEO_V4L2_COMMON=m # CONFIG_VIDEO_ALLOW_V4L1 is not set # CONFIG_VIDEO_V4L1_COMPAT is not set # CONFIG_DVB_CORE is not set CONFIG_VIDEO_MEDIA=m # # Multimedia drivers # CONFIG_IR_CORE=m CONFIG_VIDEO_IR=m # CONFIG_MEDIA_ATTACH is not set CONFIG_MEDIA_TUNER=m # CONFIG_MEDIA_TUNER_CUSTOMISE is not set CONFIG_MEDIA_TUNER_SIMPLE=m CONFIG_MEDIA_TUNER_TDA8290=m CONFIG_MEDIA_TUNER_TDA9887=m CONFIG_MEDIA_TUNER_TEA5761=m CONFIG_MEDIA_TUNER_TEA5767=m CONFIG_MEDIA_TUNER_MT20XX=m CONFIG_MEDIA_TUNER_XC2028=m CONFIG_MEDIA_TUNER_XC5000=m CONFIG_MEDIA_TUNER_MC44S803=m CONFIG_VIDEO_V4L2=m CONFIG_VIDEOBUF_GEN=m CONFIG_VIDEO_CAPTURE_DRIVERS=y # CONFIG_VIDEO_ADV_DEBUG is not set # CONFIG_VIDEO_FIXED_MINOR_RANGES is not set # CONFIG_VIDEO_HELPER_CHIPS_AUTO is not set # CONFIG_VIDEO_IR_I2C is not set # # Encoders/decoders and other helper chips # # # Audio decoders # # CONFIG_VIDEO_TVAUDIO is not set # CONFIG_VIDEO_TDA7432 is not set # CONFIG_VIDEO_TDA9840 is not set # CONFIG_VIDEO_TDA9875 is not set # CONFIG_VIDEO_TEA6415C is not set # CONFIG_VIDEO_TEA6420 is not set # CONFIG_VIDEO_MSP3400 is not set # CONFIG_VIDEO_CS5345 is not set # CONFIG_VIDEO_CS53L32A is not set # CONFIG_VIDEO_M52790 is not set # CONFIG_VIDEO_TLV320AIC23B is not set # CONFIG_VIDEO_WM8775 is not set # CONFIG_VIDEO_WM8739 is not set # CONFIG_VIDEO_VP27SMPX is not set # # RDS decoders # # CONFIG_VIDEO_SAA6588 is not set # # Video decoders # # CONFIG_VIDEO_ADV7180 is not set # CONFIG_VIDEO_BT819 is not set # CONFIG_VIDEO_BT856 is not set # CONFIG_VIDEO_BT866 is not set # CONFIG_VIDEO_KS0127 is not set # CONFIG_VIDEO_OV7670 is not set # CONFIG_VIDEO_MT9V011 is not set # CONFIG_VIDEO_TCM825X is not set # CONFIG_VIDEO_SAA7110 is not set # CONFIG_VIDEO_SAA711X is not set # CONFIG_VIDEO_SAA717X is not set # CONFIG_VIDEO_TVP514X is not set # CONFIG_VIDEO_TVP5150 is not set # CONFIG_VIDEO_VPX3220 is not set # # Video and audio decoders # # CONFIG_VIDEO_CX25840 is not set # # MPEG video encoders # # CONFIG_VIDEO_CX2341X is not set # # Video encoders # # CONFIG_VIDEO_SAA7127 is not set # CONFIG_VIDEO_SAA7185 is not set # CONFIG_VIDEO_ADV7170 is not set # CONFIG_VIDEO_ADV7175 is not set # CONFIG_VIDEO_THS7303 is not set # CONFIG_VIDEO_ADV7343 is not set # # Video improvement chips # # CONFIG_VIDEO_UPD64031A is not set # CONFIG_VIDEO_UPD64083 is not set # CONFIG_VIDEO_VIVI is not set # CONFIG_VIDEO_BT848 is not set # CONFIG_VIDEO_PMS is not set # CONFIG_VIDEO_SAA5246A is not set # CONFIG_VIDEO_SAA5249 is not set # CONFIG_VIDEO_ZORAN is not set # CONFIG_VIDEO_SAA7134 is not set # CONFIG_VIDEO_HEXIUM_ORION is not set # CONFIG_VIDEO_HEXIUM_GEMINI is not set # CONFIG_VIDEO_CX88 is not set # CONFIG_VIDEO_IVTV is not set # CONFIG_VIDEO_CAFE_CCIC is not set CONFIG_SOC_CAMERA=m CONFIG_SOC_CAMERA_MT9M001=m CONFIG_SOC_CAMERA_MT9M111=m CONFIG_SOC_CAMERA_MT9T031=m CONFIG_SOC_CAMERA_MT9T112=m CONFIG_SOC_CAMERA_MT9V022=m CONFIG_SOC_CAMERA_RJ54N1=m CONFIG_SOC_CAMERA_TW9910=m CONFIG_SOC_CAMERA_PLATFORM=m CONFIG_SOC_CAMERA_OV772X=m CONFIG_SOC_CAMERA_OV9640=m # CONFIG_V4L_USB_DRIVERS is not set # CONFIG_RADIO_ADAPTERS is not set # CONFIG_DAB is not set # # Audio devices for multimedia # # # ALSA sound # # CONFIG_SND_BT87X is not set # CONFIG_STAGING is not set ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] soc_camera: match signedness of soc_camera_limit_side() 2010-01-27 18:11 ` Németh Márton @ 2010-01-27 18:22 ` Guennadi Liakhovetski 2010-01-27 19:58 ` Németh Márton 0 siblings, 1 reply; 12+ messages in thread From: Guennadi Liakhovetski @ 2010-01-27 18:22 UTC (permalink / raw) To: Németh Márton; +Cc: V4L Mailing List On Wed, 27 Jan 2010, Németh Márton wrote: > Guennadi Liakhovetski wrote: [snip] > > 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? Well, it's my built-in mental compiler, I haven't started versioning it yet;) Strange, that (your) compiler doesn't complain - max() does type-checking and now it's signed against unsigned, hm... In any case, that one is easy to fix - just remove the "U"s, but I'm wondering why the compiler didn't shout and whether there can be other similar mismatches... Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] soc_camera: match signedness of soc_camera_limit_side() 2010-01-27 18:22 ` Guennadi Liakhovetski @ 2010-01-27 19:58 ` Németh Márton 2010-01-27 20:12 ` Guennadi Liakhovetski 0 siblings, 1 reply; 12+ messages in thread From: Németh Márton @ 2010-01-27 19:58 UTC (permalink / raw) To: Guennadi Liakhovetski; +Cc: V4L Mailing List From: Márton Németh <nm127@freemail.hu> 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. The *s_crop() functions may receive negative numbers through the c field of struct v4l2_crop. These negative values then limited by the start_min and length_min parameters of soc_camera_limit_side(). 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 <nm127@freemail.hu> --- diff -r 31eaa9423f98 linux/drivers/media/video/mt9v022.c --- a/linux/drivers/media/video/mt9v022.c Mon Jan 25 15:04:15 2010 -0200 +++ b/linux/drivers/media/video/mt9v022.c Wed Jan 27 20:49:57 2010 +0100 @@ -326,7 +326,7 @@ 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; diff -r 31eaa9423f98 linux/drivers/media/video/rj54n1cb0c.c --- a/linux/drivers/media/video/rj54n1cb0c.c Mon Jan 25 15:04:15 2010 -0200 +++ b/linux/drivers/media/video/rj54n1cb0c.c Wed Jan 27 20:49:57 2010 +0100 @@ -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); 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 @@ 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 : %d = %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,12 +638,12 @@ * 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); - unsigned int skip, resize, input_w = *in_w, input_h = *in_h, + int skip, resize, input_w = *in_w, input_h = *in_h, output_w = *out_w, output_h = *out_h; u16 inc_sel, wb_bit8, wb_left, wb_right, wb_top, wb_bottom; unsigned int peak, peak_50, peak_60; @@ -655,7 +655,7 @@ * case we have to either reduce the input window to equal or below * 512x384 or the output window to equal or below 1/2 of the input. */ - if (output_w > max(512U, input_w / 2)) { + if (output_w > max(512, input_w / 2)) { if (2 * output_w > RJ54N1_MAX_WIDTH) { input_w = RJ54N1_MAX_WIDTH; output_w = RJ54N1_MAX_WIDTH / 2; @@ -663,11 +663,11 @@ input_w = output_w * 2; } - dev_dbg(&client->dev, "Adjusted output width: in %u, out %u\n", + dev_dbg(&client->dev, "Adjusted output width: in %d, out %d\n", input_w, output_w); } - if (output_h > max(384U, input_h / 2)) { + if (output_h > max(384, input_h / 2)) { if (2 * output_h > RJ54N1_MAX_HEIGHT) { input_h = RJ54N1_MAX_HEIGHT; output_h = RJ54N1_MAX_HEIGHT / 2; @@ -675,7 +675,7 @@ input_h = output_h * 2; } - dev_dbg(&client->dev, "Adjusted output height: in %u, out %u\n", + dev_dbg(&client->dev, "Adjusted output height: in %d, out %d\n", input_h, output_h); } @@ -749,7 +749,7 @@ * 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, 15); inc_sel = 1 << skip; @@ -819,7 +819,7 @@ *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 : %d = %dx%d, skip %d\n", *in_w, *in_h, resize, output_w, output_h, skip); return resize; @@ -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; diff -r 31eaa9423f98 linux/drivers/media/video/sh_mobile_ceu_camera.c --- a/linux/drivers/media/video/sh_mobile_ceu_camera.c Mon Jan 25 15:04:15 2010 -0200 +++ b/linux/drivers/media/video/sh_mobile_ceu_camera.c Wed Jan 27 20:49:57 2010 +0100 @@ -1041,13 +1041,13 @@ */ 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); @@ -1103,7 +1103,7 @@ 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); } @@ -1117,7 +1117,7 @@ *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); } diff -r 31eaa9423f98 linux/include/media/soc_camera.h --- a/linux/include/media/soc_camera.h Mon Jan 25 15:04:15 2010 -0200 +++ b/linux/include/media/soc_camera.h Wed Jan 27 20:49:57 2010 +0100 @@ -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; ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] soc_camera: match signedness of soc_camera_limit_side() 2010-01-27 19:58 ` Németh Márton @ 2010-01-27 20:12 ` Guennadi Liakhovetski 2010-01-27 21:42 ` Németh Márton 0 siblings, 1 reply; 12+ messages in thread From: Guennadi Liakhovetski @ 2010-01-27 20:12 UTC (permalink / raw) To: Németh Márton; +Cc: V4L Mailing List You didn't reply to my most important objection: On Wed, 27 Jan 2010, Németh Márton wrote: > diff -r 31eaa9423f98 linux/include/media/soc_camera.h > --- a/linux/include/media/soc_camera.h Mon Jan 25 15:04:15 2010 -0200 > +++ b/linux/include/media/soc_camera.h Wed Jan 27 20:49:57 2010 +0100 > @@ -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; I still do not believe this function will work equally well with signed parameters, as it works with unsigned ones. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] soc_camera: match signedness of soc_camera_limit_side() 2010-01-27 20:12 ` Guennadi Liakhovetski @ 2010-01-27 21:42 ` Németh Márton 2010-01-28 20:52 ` Guennadi Liakhovetski 0 siblings, 1 reply; 12+ messages in thread From: Németh Márton @ 2010-01-27 21:42 UTC (permalink / raw) To: Guennadi Liakhovetski; +Cc: V4L Mailing List [-- Attachment #1: Type: text/plain, Size: 1810 bytes --] Guennadi Liakhovetski wrote: > You didn't reply to my most important objection: > > On Wed, 27 Jan 2010, Németh Márton wrote: > >> diff -r 31eaa9423f98 linux/include/media/soc_camera.h >> --- a/linux/include/media/soc_camera.h Mon Jan 25 15:04:15 2010 -0200 >> +++ b/linux/include/media/soc_camera.h Wed Jan 27 20:49:57 2010 +0100 >> @@ -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; > > 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 [-- Attachment #2: test.c --] [-- Type: text/x-csrc, Size: 4920 bytes --] #include <stdio.h> static inline void soc_camera_limit_side_UNSIGNED(unsigned int *start, unsigned int *length, unsigned int start_min, unsigned int length_min, unsigned int length_max) { if (*length < length_min) *length = length_min; else if (*length > length_max) *length = length_max; if (*start < start_min) *start = start_min; else if (*start > start_min + length_max - *length) *start = start_min + length_max - *length; } 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; else if (*length > length_max) *length = length_max; if (*start < start_min) *start = start_min; else if (*start > start_min + length_max - *length) *start = start_min + length_max - *length; } int main() { int start, length; unsigned int start_unsigned, length_unsigned; printf("Test Case 1: "); start = 0; length = 8; start_unsigned = start; length_unsigned = length; soc_camera_limit_side(&start, &length, 0, 8, 1600); soc_camera_limit_side_UNSIGNED(&start_unsigned, &length_unsigned, 0, 8, 1600); if (start == 0 && length == 8 && start_unsigned == start && length_unsigned == length) { printf("PASSED\n"); } else { printf("FAILED: start=%i, length=%i, start_unsigned=%i, length_unsigned=%i\n", start, length, start_unsigned, length_unsigned); } printf("Test Case 2: "); start = -5; length = 1600; start_unsigned = start; length_unsigned = length; soc_camera_limit_side(&start, &length, 0, 8, 1600); soc_camera_limit_side_UNSIGNED(&start_unsigned, &length_unsigned, 0, 8, 1600); if (start == 0 && length == 1600 && start_unsigned == start && length_unsigned == length) { printf("PASSED\n"); } else { printf("FAILED: start=%i, length=%i, start_unsigned=%i, length_unsigned=%i\n", start, length, start_unsigned, length_unsigned); } printf("Test Case 3: "); start = 50; length = -15; start_unsigned = start; length_unsigned = length; soc_camera_limit_side(&start, &length, 0, 8, 1600); soc_camera_limit_side_UNSIGNED(&start_unsigned, &length_unsigned, 0, 8, 1600); if (start == 50 && length == 8 && start_unsigned == start && length_unsigned == length) { printf("PASSED\n"); } else { printf("FAILED: start=%i, length=%i, start_unsigned=%i, length_unsigned=%i\n", start, length, start_unsigned, length_unsigned); } printf("Test Case 4: "); start = 500; length = 2000; start_unsigned = start; length_unsigned = length; soc_camera_limit_side(&start, &length, 0, 8, 1600); soc_camera_limit_side_UNSIGNED(&start_unsigned, &length_unsigned, 0, 8, 1600); if (start == 0 && length == 1600 && start_unsigned == start && length_unsigned == length) { printf("PASSED\n"); } else { printf("FAILED: start=%i, length=%i, start_unsigned=%i, length_unsigned=%i\n", start, length, start_unsigned, length_unsigned); } printf("Test Case 5: "); start = -20; length = 1600; start_unsigned = start; length_unsigned = length; soc_camera_limit_side(&start, &length, 100, 8, 1600); soc_camera_limit_side_UNSIGNED(&start_unsigned, &length_unsigned, 100, 8, 1600); if (start == 100 && length == 1600 && start_unsigned == start && length_unsigned == length) { printf("PASSED\n"); } else { printf("FAILED: start=%i, length=%i, start_unsigned=%i, length_unsigned=%i\n", start, length, start_unsigned, length_unsigned); } printf("Test Case 6: "); start = 1600; length = 1600; start_unsigned = start; length_unsigned = length; soc_camera_limit_side(&start, &length, 100, 8, 1600); soc_camera_limit_side_UNSIGNED(&start_unsigned, &length_unsigned, 100, 8, 1600); if (start == 100 && length == 1600 && start_unsigned == start && length_unsigned == length) { printf("PASSED\n"); } else { printf("FAILED: start=%i, length=%i, start_unsigned=%i, length_unsigned=%i\n", start, length, start_unsigned, length_unsigned); } printf("Test Case 7: "); start = 5; length = 300; start_unsigned = start; length_unsigned = length; soc_camera_limit_side(&start, &length, 100, 8, 1600); soc_camera_limit_side_UNSIGNED(&start_unsigned, &length_unsigned, 100, 8, 1600); if (start == 100 && length == 300 && start_unsigned == start && length_unsigned == length) { printf("PASSED\n"); } else { printf("FAILED: start=%i, length=%i, start_unsigned=%i, length_unsigned=%i\n", start, length, start_unsigned, length_unsigned); } printf("Test Case 8: "); start = 200; length = 4; start_unsigned = start; length_unsigned = length; soc_camera_limit_side(&start, &length, 100, 8, 1600); soc_camera_limit_side_UNSIGNED(&start_unsigned, &length_unsigned, 100, 8, 1600); if (start == 200 && length == 8 && start_unsigned == start && length_unsigned == length) { printf("PASSED\n"); } else { printf("FAILED: start=%i, length=%i, start_unsigned=%i, length_unsigned=%i\n", start, length, start_unsigned, length_unsigned); } return 0; } ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] soc_camera: match signedness of soc_camera_limit_side() 2010-01-27 21:42 ` Németh Márton @ 2010-01-28 20:52 ` Guennadi Liakhovetski 2010-01-28 21:30 ` Németh Márton 0 siblings, 1 reply; 12+ messages in thread From: Guennadi Liakhovetski @ 2010-01-28 20:52 UTC (permalink / raw) To: Németh Márton; +Cc: V4L Mailing List On Wed, 27 Jan 2010, Németh Márton wrote: > Guennadi Liakhovetski wrote: > > You didn't reply to my most important objection: > > > > On Wed, 27 Jan 2010, Németh Márton wrote: > > > >> diff -r 31eaa9423f98 linux/include/media/soc_camera.h > >> --- a/linux/include/media/soc_camera.h Mon Jan 25 15:04:15 2010 -0200 > >> +++ b/linux/include/media/soc_camera.h Wed Jan 27 20:49:57 2010 +0100 > >> @@ -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; > > > > 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. 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 +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? Thanks Guennadi > > 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 > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] soc_camera: match signedness of soc_camera_limit_side() 2010-01-28 20:52 ` Guennadi Liakhovetski @ 2010-01-28 21:30 ` Németh Márton 2010-02-09 9:05 ` Guennadi Liakhovetski 0 siblings, 1 reply; 12+ messages in thread From: Németh Márton @ 2010-01-28 21:30 UTC (permalink / raw) To: Guennadi Liakhovetski; +Cc: V4L Mailing List Guennadi Liakhovetski wrote: > On Wed, 27 Jan 2010, Németh Márton wrote: > >> Guennadi Liakhovetski wrote: >>> You didn't reply to my most important objection: >>> >>> On Wed, 27 Jan 2010, Németh Márton wrote: >>> >>>> diff -r 31eaa9423f98 linux/include/media/soc_camera.h >>>> --- a/linux/include/media/soc_camera.h Mon Jan 25 15:04:15 2010 -0200 >>>> +++ b/linux/include/media/soc_camera.h Wed Jan 27 20:49:57 2010 +0100 >>>> @@ -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; >>> 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. > > 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.) > 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 > > +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); 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. > 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. Regards, Márton Németh --- From: Márton Németh <nm127@freemail.hu> 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. The *s_crop() functions may receive negative numbers through the c field of struct v4l2_crop. These negative values then limited by the start_min and length_min parameters of soc_camera_limit_side(). 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 <nm127@freemail.hu> --- diff -r 31eaa9423f98 linux/drivers/media/video/mt9v022.c --- a/linux/drivers/media/video/mt9v022.c Mon Jan 25 15:04:15 2010 -0200 +++ b/linux/drivers/media/video/mt9v022.c Thu Jan 28 22:24:35 2010 +0100 @@ -326,7 +326,7 @@ 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; diff -r 31eaa9423f98 linux/drivers/media/video/rj54n1cb0c.c --- a/linux/drivers/media/video/rj54n1cb0c.c Mon Jan 25 15:04:15 2010 -0200 +++ b/linux/drivers/media/video/rj54n1cb0c.c Thu Jan 28 22:24:35 2010 +0100 @@ -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); 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 @@ 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 : %d = %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,12 +638,12 @@ * 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); - unsigned int skip, resize, input_w = *in_w, input_h = *in_h, + int skip, resize, input_w = *in_w, input_h = *in_h, output_w = *out_w, output_h = *out_h; u16 inc_sel, wb_bit8, wb_left, wb_right, wb_top, wb_bottom; unsigned int peak, peak_50, peak_60; @@ -655,7 +655,7 @@ * case we have to either reduce the input window to equal or below * 512x384 or the output window to equal or below 1/2 of the input. */ - if (output_w > max(512U, input_w / 2)) { + if (output_w > max(512, input_w / 2)) { if (2 * output_w > RJ54N1_MAX_WIDTH) { input_w = RJ54N1_MAX_WIDTH; output_w = RJ54N1_MAX_WIDTH / 2; @@ -663,11 +663,11 @@ input_w = output_w * 2; } - dev_dbg(&client->dev, "Adjusted output width: in %u, out %u\n", + dev_dbg(&client->dev, "Adjusted output width: in %d, out %d\n", input_w, output_w); } - if (output_h > max(384U, input_h / 2)) { + if (output_h > max(384, input_h / 2)) { if (2 * output_h > RJ54N1_MAX_HEIGHT) { input_h = RJ54N1_MAX_HEIGHT; output_h = RJ54N1_MAX_HEIGHT / 2; @@ -675,7 +675,7 @@ input_h = output_h * 2; } - dev_dbg(&client->dev, "Adjusted output height: in %u, out %u\n", + dev_dbg(&client->dev, "Adjusted output height: in %d, out %d\n", input_h, output_h); } @@ -749,7 +749,7 @@ * 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, 15); inc_sel = 1 << skip; @@ -819,7 +819,7 @@ *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 : %d = %dx%d, skip %d\n", *in_w, *in_h, resize, output_w, output_h, skip); return resize; @@ -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; diff -r 31eaa9423f98 linux/drivers/media/video/sh_mobile_ceu_camera.c --- a/linux/drivers/media/video/sh_mobile_ceu_camera.c Mon Jan 25 15:04:15 2010 -0200 +++ b/linux/drivers/media/video/sh_mobile_ceu_camera.c Thu Jan 28 22:24:35 2010 +0100 @@ -1041,13 +1041,13 @@ */ 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); @@ -1057,10 +1057,12 @@ if (ret < 0) return ret; - soc_camera_limit_side(&rect->left, &rect->width, cap.bounds.left, 2, - cap.bounds.width); - soc_camera_limit_side(&rect->top, &rect->height, cap.bounds.top, 4, - cap.bounds.height); + 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); /* * Popular special case - some cameras can only handle fixed sizes like @@ -1103,7 +1105,7 @@ 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); } @@ -1117,7 +1119,7 @@ *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); } diff -r 31eaa9423f98 linux/include/media/soc_camera.h --- a/linux/include/media/soc_camera.h Mon Jan 25 15:04:15 2010 -0200 +++ b/linux/include/media/soc_camera.h Thu Jan 28 22:24:35 2010 +0100 @@ -264,8 +264,8 @@ 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) ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] soc_camera: match signedness of soc_camera_limit_side() 2010-01-28 21:30 ` Németh Márton @ 2010-02-09 9:05 ` Guennadi Liakhovetski 2010-02-12 9:32 ` Guennadi Liakhovetski 0 siblings, 1 reply; 12+ messages in thread From: Guennadi Liakhovetski @ 2010-02-09 9:05 UTC (permalink / raw) To: Németh Márton; +Cc: V4L Mailing List On Thu, 28 Jan 2010, Németh Márton wrote: > Guennadi Liakhovetski wrote: > > On Wed, 27 Jan 2010, Németh Márton wrote: > > > >> Guennadi Liakhovetski wrote: > >>> You didn't reply to my most important objection: > >>> > >>> On Wed, 27 Jan 2010, Németh Márton wrote: > >>> > >>>> diff -r 31eaa9423f98 linux/include/media/soc_camera.h > >>>> --- a/linux/include/media/soc_camera.h Mon Jan 25 15:04:15 2010 -0200 > >>>> +++ b/linux/include/media/soc_camera.h Wed Jan 27 20:49:57 2010 +0100 > >>>> @@ -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; > >>> 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. > > > > 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? Isn't this not clear enough? if (*start < start_min) *start = start_min; else if (*start > start_min + length_max - *length) *start = start_min + length_max - *length; > (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.) > > > 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 > > > > +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); > > 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. That's exactly the expected behaviour. > > 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 <nm127@freemail.hu> 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 <nm127@freemail.hu> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> --- diff --git a/drivers/media/video/mt9v022.c b/drivers/media/video/mt9v022.c index 1a34d29..e5bae4c 100644 --- a/drivers/media/video/mt9v022.c +++ b/drivers/media/video/mt9v022.c @@ -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; diff --git a/drivers/media/video/mx3_camera.c b/drivers/media/video/mx3_camera.c index bd297f5..d477e30 100644 --- a/drivers/media/video/mx3_camera.c +++ b/drivers/media/video/mx3_camera.c @@ -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; } diff --git a/drivers/media/video/rj54n1cb0c.c b/drivers/media/video/rj54n1cb0c.c index 9277194..bbd9c11 100644 --- a/drivers/media/video/rj54n1cb0c.c +++ b/drivers/media/video/rj54n1cb0c.c @@ -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; diff --git a/drivers/media/video/sh_mobile_ceu_camera.c b/drivers/media/video/sh_mobile_ceu_camera.c index f09c714..af506bc 100644 --- a/drivers/media/video/sh_mobile_ceu_camera.c +++ b/drivers/media/video/sh_mobile_ceu_camera.c @@ -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); } diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h index dcc5b86..5a17365 100644 --- a/include/media/soc_camera.h +++ b/include/media/soc_camera.h @@ -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) ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] soc_camera: match signedness of soc_camera_limit_side() 2010-02-09 9:05 ` Guennadi Liakhovetski @ 2010-02-12 9:32 ` Guennadi Liakhovetski 2010-02-24 6:32 ` Németh Márton 0 siblings, 1 reply; 12+ messages in thread From: Guennadi Liakhovetski @ 2010-02-12 9:32 UTC (permalink / raw) To: Németh Márton; +Cc: V4L Mailing List Hi Németh On Tue, 9 Feb 2010, Guennadi Liakhovetski wrote: > 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 <nm127@freemail.hu> > > 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 <nm127@freemail.hu> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> please confirm, if you agree with my version of your patch, or I'll have to leave it out from my pull request. Thanks Guennadi > --- > diff --git a/drivers/media/video/mt9v022.c b/drivers/media/video/mt9v022.c > index 1a34d29..e5bae4c 100644 > --- a/drivers/media/video/mt9v022.c > +++ b/drivers/media/video/mt9v022.c > @@ -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; > > diff --git a/drivers/media/video/mx3_camera.c b/drivers/media/video/mx3_camera.c > index bd297f5..d477e30 100644 > --- a/drivers/media/video/mx3_camera.c > +++ b/drivers/media/video/mx3_camera.c > @@ -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; > } > diff --git a/drivers/media/video/rj54n1cb0c.c b/drivers/media/video/rj54n1cb0c.c > index 9277194..bbd9c11 100644 > --- a/drivers/media/video/rj54n1cb0c.c > +++ b/drivers/media/video/rj54n1cb0c.c > @@ -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; > > diff --git a/drivers/media/video/sh_mobile_ceu_camera.c b/drivers/media/video/sh_mobile_ceu_camera.c > index f09c714..af506bc 100644 > --- a/drivers/media/video/sh_mobile_ceu_camera.c > +++ b/drivers/media/video/sh_mobile_ceu_camera.c > @@ -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); > } > diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h > index dcc5b86..5a17365 100644 > --- a/include/media/soc_camera.h > +++ b/include/media/soc_camera.h > @@ -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/ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] soc_camera: match signedness of soc_camera_limit_side() 2010-02-12 9:32 ` Guennadi Liakhovetski @ 2010-02-24 6:32 ` Németh Márton 0 siblings, 0 replies; 12+ messages in thread From: Németh Márton @ 2010-02-24 6:32 UTC (permalink / raw) To: Guennadi Liakhovetski; +Cc: V4L Mailing List Hi Guennadi, Guennadi Liakhovetski wrote: > Hi Németh > > On Tue, 9 Feb 2010, Guennadi Liakhovetski wrote: > >> 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 <nm127@freemail.hu> >> >> 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 <nm127@freemail.hu> >> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> > > please confirm, if you agree with my version of your patch, or I'll have > to leave it out from my pull request. I confirm. Sorry about the late response, I was not able to access my emails for a while. Regards, Márton Németh >> --- >> diff --git a/drivers/media/video/mt9v022.c b/drivers/media/video/mt9v022.c >> index 1a34d29..e5bae4c 100644 >> --- a/drivers/media/video/mt9v022.c >> +++ b/drivers/media/video/mt9v022.c >> @@ -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; >> >> diff --git a/drivers/media/video/mx3_camera.c b/drivers/media/video/mx3_camera.c >> index bd297f5..d477e30 100644 >> --- a/drivers/media/video/mx3_camera.c >> +++ b/drivers/media/video/mx3_camera.c >> @@ -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; >> } >> diff --git a/drivers/media/video/rj54n1cb0c.c b/drivers/media/video/rj54n1cb0c.c >> index 9277194..bbd9c11 100644 >> --- a/drivers/media/video/rj54n1cb0c.c >> +++ b/drivers/media/video/rj54n1cb0c.c >> @@ -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; >> >> diff --git a/drivers/media/video/sh_mobile_ceu_camera.c b/drivers/media/video/sh_mobile_ceu_camera.c >> index f09c714..af506bc 100644 >> --- a/drivers/media/video/sh_mobile_ceu_camera.c >> +++ b/drivers/media/video/sh_mobile_ceu_camera.c >> @@ -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); >> } >> diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h >> index dcc5b86..5a17365 100644 >> --- a/include/media/soc_camera.h >> +++ b/include/media/soc_camera.h >> @@ -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/ > > ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2010-02-24 6:59 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-01-23 13:43 [PATCH] soc_camera: match signedness of soc_camera_limit_side() Németh Márton 2010-01-27 16:05 ` Guennadi Liakhovetski 2010-01-27 18:11 ` Németh Márton 2010-01-27 18:22 ` Guennadi Liakhovetski 2010-01-27 19:58 ` Németh Márton 2010-01-27 20:12 ` Guennadi Liakhovetski 2010-01-27 21:42 ` Németh Márton 2010-01-28 20:52 ` Guennadi Liakhovetski 2010-01-28 21:30 ` Németh Márton 2010-02-09 9:05 ` Guennadi Liakhovetski 2010-02-12 9:32 ` Guennadi Liakhovetski 2010-02-24 6:32 ` Németh Márton
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox