[RFC] soc_camera: endianness between camera and its host

[RFC] soc_camera: endianness between camera and its host

[Robert Jarzmik]

Modern camera chips provide ways to invert their data output, as well in colors
swap as in byte order. To be more precise, the one I know (mt9m111) enables :
 - swapping Red and Blue in RGB formats
 - swapping first and second byte in RGB formats
 - swapping Cb and Cr in YUV formats
 - swapping chrominance and luminance in YUV formats

This swap is necessary to adapt the camera chip output to the capture
interface. For example, pxa_camera on pxa27x CPUs expects RGB and YUV formats
in a very specific order. This order is not the default one on mt9m111 chip, so
the mt9m111 driver has to have a way to know how to order its output.

The question I have is where to put such information, so that board specific
code (arch/arm/mach-pxa/xxx.c) can setup this for a dedicated camera chip ?

One easy way would be to put it in soc_camera_link, but is it the right place ?

--
Robert

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [RFC] soc_camera: endianness between camera and its host

[Guennadi Liakhovetski]

On Sat, 2 Aug 2008, Robert Jarzmik wrote:

> Modern camera chips provide ways to invert their data output, as well in colors
> swap as in byte order. To be more precise, the one I know (mt9m111) enables :

To me these look like just different pixel formats:

>  - swapping Red and Blue in RGB formats

This is no longer RGB, but BGR, compare:

#define V4L2_PIX_FMT_BGR24   v4l2_fourcc('B', 'G', 'R', '3') /* 24  BGR-8-8-8     */
#define V4L2_PIX_FMT_RGB24   v4l2_fourcc('R', 'G', 'B', '3') /* 24  RGB-8-8-8     */

>  - swapping first and second byte in RGB formats

hm, no idea about this one

>  - swapping Cb and Cr in YUV formats

I couldn't find it among the defined formats in videodev2.h, but on 
http://www.fourcc.org/ they are defined as YVYU vs. YUYV.

>  - swapping chrominance and luminance in YUV formats

These seem to be these two:

#define V4L2_PIX_FMT_YUYV    v4l2_fourcc('Y', 'U', 'Y', 'V') /* 16  YUV 4:2:2     */
#define V4L2_PIX_FMT_UYVY    v4l2_fourcc('U', 'Y', 'V', 'Y') /* 16  YUV 4:2:2     */

So, I don't see at first any relation to host's endianness. You just 
define respective formats in cameras array of struct 
soc_camera_data_format.

> This swap is necessary to adapt the camera chip output to the capture
> interface. For example, pxa_camera on pxa27x CPUs expects RGB and YUV formats
> in a very specific order. This order is not the default one on mt9m111 chip, so
> the mt9m111 driver has to have a way to know how to order its output.
> 
> The question I have is where to put such information, so that board specific
> code (arch/arm/mach-pxa/xxx.c) can setup this for a dedicated camera chip ?
> 
> One easy way would be to put it in soc_camera_link, but is it the right place ?

Isn't using the existing pixel format negotiation procedure eough?

Thanks
Guennadi
---
Guennadi Liakhovetski

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [RFC] soc_camera: endianness between camera and its host

[Robert Jarzmik]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

> On Sat, 2 Aug 2008, Robert Jarzmik wrote:
>
>> Modern camera chips provide ways to invert their data output, as well in colors
>> swap as in byte order. To be more precise, the one I know (mt9m111) enables :
>
> To me these look like just different pixel formats:
Ah, they look like, but there aren't.

Let me explain the subtle part of it by an example on mio a701 phone :
 - mt9m111 is connected to a pxa272 cpu through an 8bit bus
 - when I select RGB565 as a pixel format, the pxa cpu expects the bits in a
 very precise order :
   - have a look at PXA Developper Manual, chapter 27.4.5.2.1, table 27-10. The
   order the bytes are comming on the bus is important, because of the
   "interpretation" the PXA does, to reorder and store them in memory.
   => the chip must send the bytes in that order
   - if you pay attention closely, you'll notice the pxa doesn't expect RGB but
   inverted BGR.
   - have a look at Micron MT9M111 specification, table 5, page 14. You'll see
   what they consider as RGB565.

>>  - swapping first and second byte in RGB formats
>
> hm, no idea about this one
Explanation above. Have a look at the 2 specification, and the tables I
mentionned. It will be far more clearer.

> So, I don't see at first any relation to host's endianness. You just 
> define respective formats in cameras array of struct 
> soc_camera_data_format.
That would be true if the host didn't interpret and reorder the bytes, which pxa
does.

> Isn't using the existing pixel format negotiation procedure eough?
If you still think that after looking at the tables, well .. we'll discuss
further. Maybe there's something I don't see ... You tell me your opinion once
you had looked at the tables.

You have all my code now, so you know what I'm facing here :)

--
Robert

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [RFC] soc_camera: endianness between camera and its host

[Guennadi Liakhovetski]

Sorry for a late reply. Looking at your mt9m111 patch, I realised we 
didn't finish this discussion:-)

On Sun, 3 Aug 2008, Robert Jarzmik wrote:

> Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:
> 
> > On Sat, 2 Aug 2008, Robert Jarzmik wrote:
> >
> >> Modern camera chips provide ways to invert their data output, as well in colors
> >> swap as in byte order. To be more precise, the one I know (mt9m111) enables :
> >
> > To me these look like just different pixel formats:
> Ah, they look like, but there aren't.
> 
> Let me explain the subtle part of it by an example on mio a701 phone :
>  - mt9m111 is connected to a pxa272 cpu through an 8bit bus
>  - when I select RGB565 as a pixel format, the pxa cpu expects the bits in a
>  very precise order :
>    - have a look at PXA Developper Manual, chapter 27.4.5.2.1, table 27-10. The
>    order the bytes are comming on the bus is important, because of the
>    "interpretation" the PXA does, to reorder and store them in memory.
>    => the chip must send the bytes in that order
>    - if you pay attention closely, you'll notice the pxa doesn't expect RGB but
>    inverted BGR.
>    - have a look at Micron MT9M111 specification, table 5, page 14. You'll see
>    what they consider as RGB565.

Ok, I looked at them, I really did:-) Still, doesn't the following 
describe the situation:

mt9m111 supported formats:
rgb 565				(as specified in mt9m111 manual)
rgb 565 swapped			(ditto swapped to match pxa270)
...

pxa270 supports formats
rgb 565 swapped			(pxa manual doesn't call it swapped)
...

So, when a user enumerates supported formats, we should report rgb 565 
swapped, but not report rgb 565. If you connect a mt9m111 to another host, 
maybe the non-swapped rgb 565 will be supported. So, mt9m111 should report 
both. The problem currently is, soc-camera doesn't ask the host controller 
whether it supports a specific pixel format. It only has a chance to fail 
an attempted VIDIOC_S_FMT, which is a bit too late. So, would adding pixel 
format negotiation with the camera host driver sufficiently fix the 
problem for you? One of us could try to cook a patch then.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [RFC] soc_camera: endianness between camera and its host

[robert.jarzmik]

Selon Guennadi Liakhovetski <g.liakhovetski@gmx.de>:

> Sorry for a late reply. Looking at your mt9m111 patch, I realised we
> didn't finish this discussion:-)
No problem.

> Ok, I looked at them, I really did:-) Still, doesn't the following
> describe the situation:
>
> mt9m111 supported formats:
> rgb 565				(as specified in mt9m111 manual)
> rgb 565 swapped			(ditto swapped to match pxa270)
> ...
>
> pxa270 supports formats
> rgb 565 swapped			(pxa manual doesn't call it swapped)
> ...
Yes, that's exactly my point of view.

> So, when a user enumerates supported formats, we should report rgb 565
> swapped, but not report rgb 565. If you connect a mt9m111 to another host,
> maybe the non-swapped rgb 565 will be supported. So, mt9m111 should report
> both. The problem currently is, soc-camera doesn't ask the host controller
> whether it supports a specific pixel format. It only has a chance to fail
> an attempted VIDIOC_S_FMT, which is a bit too late. So, would adding pixel
> format negotiation with the camera host driver sufficiently fix the
> problem for you? One of us could try to cook a patch then.

Yes, pixel format negotiation is the key, that's the clean solution.
It will have impacts on existing camera drivers, like mt9m001, ..., and camera
hosts, but you must already be aware of it and ready to pay the price :)

Now, let's talk schedule. Until the end of the week, I'll be a bit busy. If I
don't see a patch you submitted by then, I'll cook one up. I only need to know
at which point you wish the format negociation should be performed, and on which
ground.

[RFC]
Would that be something like :
 - all begins which the binding of both a camera driver and host driver
 - soc_camera asks host controller which format it provides
 - soc_camera asks camera driver which format it supports
 - soc_camera make a table of possible pixel formats (which would be the common
subset of host and camera pixel formats)
 - soc_camera uses that table for format enumeration
 - soc_camera uses that table for preliminary check on VIDIOC_S_FMT

--
Robert

PS: I use an awfull web based mailer this morning, which I never use. Sorry if
it messes up linebreaks ...

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [RFC] soc_camera: endianness between camera and its host

[Guennadi Liakhovetski]

On Wed, 13 Aug 2008, robert.jarzmik@free.fr wrote:

> Selon Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
> 
> > So, when a user enumerates supported formats, we should report rgb 565
> > swapped, but not report rgb 565. If you connect a mt9m111 to another host,
> > maybe the non-swapped rgb 565 will be supported. So, mt9m111 should report
> > both. The problem currently is, soc-camera doesn't ask the host controller
> > whether it supports a specific pixel format. It only has a chance to fail
> > an attempted VIDIOC_S_FMT, which is a bit too late. So, would adding pixel
> > format negotiation with the camera host driver sufficiently fix the
> > problem for you? One of us could try to cook a patch then.
> 
> Yes, pixel format negotiation is the key, that's the clean solution.
> It will have impacts on existing camera drivers, like mt9m001, ..., and camera
> hosts, but you must already be aware of it and ready to pay the price :)

Yes, fortunately, there are not too many yet:-)

> Now, let's talk schedule. Until the end of the week, I'll be a bit busy. If I
> don't see a patch you submitted by then, I'll cook one up. I only need to know
> at which point you wish the format negociation should be performed, and on which
> ground.

Hm, I would just do this during format-enumeration... I'll try to sketch 
something maybe today.

> [RFC]
> Would that be something like :
>  - all begins which the binding of both a camera driver and host driver
>  - soc_camera asks host controller which format it provides
>  - soc_camera asks camera driver which format it supports
>  - soc_camera make a table of possible pixel formats (which would be the common
> subset of host and camera pixel formats)
>  - soc_camera uses that table for format enumeration
>  - soc_camera uses that table for preliminary check on VIDIOC_S_FMT

I think, we might manage to get it a bit simpler.

As for your mt9m111 patch - unfortunately, during my first quick review I 
missed a few more minor formatting issues, so, because it is kinda my 
fault, my plan is to apply your patch as it is, and then I can just post 
for your ack a clean-up patch. And then, as we get format negotiation in 
place, you can extend it with further supported formats. What do you 
think?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [RFC] soc_camera: endianness between camera and its host

[robert.jarzmik]

Selon Guennadi Liakhovetski <g.liakhovetski@gmx.de>:

> Hm, I would just do this during format-enumeration... I'll try to sketch
> something maybe today.
Great !

> > [RFC]
> I think, we might manage to get it a bit simpler.
Even better :)

> As for your mt9m111 patch - unfortunately, during my first quick review I
> missed a few more minor formatting issues, so, because it is kinda my
> fault, my plan is to apply your patch as it is, and then I can just post
> for your ack a clean-up patch. And then, as we get format negotiation in
> place, you can extend it with further supported formats. What do you
> think?
Very good idea. Let's do it that way, it's perfect for me.

Cheers.

--
Robert

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

[PATCH] mt9m111: style cleanup

[Guennadi Liakhovetski]

Fix a typo in Kconfig, simplify error checking, further minor cleanup.

Signed-off-by: Guennadi Liakhovetski <g.iakhovetski@gmx.de>

---

On Wed, 13 Aug 2008, robert.jarzmik@free.fr wrote:

> Selon Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
> 
> > As for your mt9m111 patch - unfortunately, during my first quick review I
> > missed a few more minor formatting issues, so, because it is kinda my
> > fault, my plan is to apply your patch as it is, and then I can just post
> > for your ack a clean-up patch. And then, as we get format negotiation in
> > place, you can extend it with further supported formats. What do you
> > think?
> Very good idea. Let's do it that way, it's perfect for me.

Robert, please have a look at and test this patch. It fixes a typo, cleans 
up a couple of places, the big bulk of the change is replacing (ret >= 0) 
tests with (!ret), since smbus functions return a negative error or 0, 
just as we need it. I will do the same to mt9m001 and mt9v022.

diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index 68e7e90..68d5810 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -939,7 +939,7 @@ config MT9M001_PCA9536_SWITCH
 	  extender to switch between 8 and 10 bit datawidth modes
 
 config SOC_CAMERA_MT9M111
-	tristate "mt9m001 support"
+	tristate "mt9m111 support"
 	depends on SOC_CAMERA && I2C
 	help
 	  This driver supports MT9M111 cameras from Micron
diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
index 0c88e5d..537cff0 100644
--- a/drivers/media/video/mt9m111.c
+++ b/drivers/media/video/mt9m111.c
@@ -173,7 +173,7 @@ static int reg_page_map_set(struct i2c_client *client, const u16 reg)
 		return -EINVAL;
 
 	ret = i2c_smbus_write_word_data(client, MT9M111_PAGE_MAP, swab16(page));
-	if (ret >= 0)
+	if (!ret)
 		lastpage = page;
 	return ret;
 }
@@ -200,7 +200,7 @@ static int mt9m111_reg_write(struct soc_camera_device *icd, const u16 reg,
 	int ret;
 
 	ret = reg_page_map_set(client, reg);
-	if (ret >= 0)
+	if (!ret)
 		ret = i2c_smbus_write_word_data(mt9m111->client, (reg & 0xff),
 						swab16(data));
 	dev_dbg(&icd->dev, "write reg.%03x = %04x -> %d\n", reg, data, ret);
@@ -246,7 +246,7 @@ static int mt9m111_set_context(struct soc_camera_device *icd,
 static int mt9m111_setup_rect(struct soc_camera_device *icd)
 {
 	struct mt9m111 *mt9m111 = container_of(icd, struct mt9m111, icd);
-	int ret = 0, is_raw_format;
+	int ret, is_raw_format;
 	int width = mt9m111->width;
 	int height = mt9m111->height;
 
@@ -256,32 +256,31 @@ static int mt9m111_setup_rect(struct soc_camera_device *icd)
 	else
 		is_raw_format = 0;
 
-	if (ret >= 0)
-		ret = reg_write(COLUMN_START, mt9m111->left);
-	if (ret >= 0)
+	ret = reg_write(COLUMN_START, mt9m111->left);
+	if (!ret)
 		ret = reg_write(ROW_START, mt9m111->top);
 
 	if (is_raw_format) {
-		if (ret >= 0)
+		if (!ret)
 			ret = reg_write(WINDOW_WIDTH, width);
-		if (ret >= 0)
+		if (!ret)
 			ret = reg_write(WINDOW_HEIGHT, height);
 	} else {
-		if (ret >= 0)
+		if (!ret)
 			ret = reg_write(REDUCER_XZOOM_B, MT9M111_MAX_WIDTH);
-		if (ret >= 0)
+		if (!ret)
 			ret = reg_write(REDUCER_YZOOM_B, MT9M111_MAX_HEIGHT);
-		if (ret >= 0)
+		if (!ret)
 			ret = reg_write(REDUCER_XSIZE_B, width);
-		if (ret >= 0)
+		if (!ret)
 			ret = reg_write(REDUCER_YSIZE_B, height);
-		if (ret >= 0)
+		if (!ret)
 			ret = reg_write(REDUCER_XZOOM_A, MT9M111_MAX_WIDTH);
-		if (ret >= 0)
+		if (!ret)
 			ret = reg_write(REDUCER_YZOOM_A, MT9M111_MAX_HEIGHT);
-		if (ret >= 0)
+		if (!ret)
 			ret = reg_write(REDUCER_XSIZE_A, width);
-		if (ret >= 0)
+		if (!ret)
 			ret = reg_write(REDUCER_YSIZE_A, height);
 	}
 
@@ -293,7 +292,7 @@ static int mt9m111_setup_pixfmt(struct soc_camera_device *icd, u16 outfmt)
 	int ret;
 
 	ret = reg_write(OUTPUT_FORMAT_CTRL2_A, outfmt);
-	if (ret >= 0)
+	if (!ret)
 		ret = reg_write(OUTPUT_FORMAT_CTRL2_B, outfmt);
 	return ret;
 }
@@ -305,7 +304,6 @@ static int mt9m111_setfmt_bayer8(struct soc_camera_device *icd)
 
 static int mt9m111_setfmt_bayer10(struct soc_camera_device *icd)
 {
-
 	return mt9m111_setup_pixfmt(icd, MT9M111_OUTFMT_BYPASS_IFP);
 }
 
@@ -356,7 +354,7 @@ static int mt9m111_enable(struct soc_camera_device *icd)
 	int ret;
 
 	ret = reg_set(RESET, MT9M111_RESET_CHIP_ENABLE);
-	if (ret >= 0)
+	if (!ret)
 		mt9m111->powered = 1;
 	return ret;
 }
@@ -367,7 +365,7 @@ static int mt9m111_disable(struct soc_camera_device *icd)
 	int ret;
 
 	ret = reg_clear(RESET, MT9M111_RESET_CHIP_ENABLE);
-	if (ret >= 0)
+	if (!ret)
 		mt9m111->powered = 0;
 	return ret;
 }
@@ -377,9 +375,9 @@ static int mt9m111_reset(struct soc_camera_device *icd)
 	int ret;
 
 	ret = reg_set(RESET, MT9M111_RESET_RESET_MODE);
-	if (ret >= 0)
+	if (!ret)
 		ret = reg_set(RESET, MT9M111_RESET_RESET_SOC);
-	if (ret >= 0)
+	if (!ret)
 		ret = reg_clear(RESET, MT9M111_RESET_RESET_MODE
 				| MT9M111_RESET_RESET_SOC);
 	return ret;
@@ -410,7 +408,7 @@ static int mt9m111_set_bus_param(struct soc_camera_device *icd, unsigned long f)
 static int mt9m111_set_pixfmt(struct soc_camera_device *icd, u32 pixfmt)
 {
 	struct mt9m111 *mt9m111 = container_of(icd, struct mt9m111, icd);
-	int ret = 0;
+	int ret;
 
 	switch (pixfmt) {
 	case V4L2_PIX_FMT_SBGGR8:
@@ -433,7 +431,7 @@ static int mt9m111_set_pixfmt(struct soc_camera_device *icd, u32 pixfmt)
 		ret = -EINVAL;
 	}
 
-	if (ret >= 0)
+	if (!ret)
 		mt9m111->pixfmt = pixfmt;
 
 	return ret;
@@ -443,7 +441,7 @@ static int mt9m111_set_fmt_cap(struct soc_camera_device *icd,
 			       __u32 pixfmt, struct v4l2_rect *rect)
 {
 	struct mt9m111 *mt9m111 = container_of(icd, struct mt9m111, icd);
-	int ret = 0;
+	int ret;
 
 	mt9m111->left = rect->left;
 	mt9m111->top = rect->top;
@@ -455,9 +453,9 @@ static int mt9m111_set_fmt_cap(struct soc_camera_device *icd,
 		mt9m111->height);
 
 	ret = mt9m111_setup_rect(icd);
-	if (ret >= 0)
+	if (!ret)
 		ret = mt9m111_set_pixfmt(icd, pixfmt);
-	return ret < 0 ? ret : 0;
+	return ret;
 }
 
 static int mt9m111_try_fmt_cap(struct soc_camera_device *icd,
@@ -644,7 +642,7 @@ static int mt9m111_set_global_gain(struct soc_camera_device *icd, int gain)
 	if ((gain >= 64 * 2) && (gain < 63 * 2 * 2))
 		val = (1 << 10) | (1 << 9) | (gain / 4);
 	else if ((gain >= 64) && (gain < 64 * 2))
-		val = (1<<9) | (gain / 2);
+		val = (1 << 9) | (gain / 2);
 	else
 		val = gain;
 
@@ -661,7 +659,7 @@ static int mt9m111_set_autoexposure(struct soc_camera_device *icd, int on)
 	else
 		ret = reg_clear(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOEXPO_EN);
 
-	if (ret >= 0)
+	if (!ret)
 		mt9m111->autoexposure = on;
 
 	return ret;
@@ -711,7 +709,7 @@ static int mt9m111_set_control(struct soc_camera_device *icd,
 {
 	struct mt9m111 *mt9m111 = container_of(icd, struct mt9m111, icd);
 	const struct v4l2_queryctrl *qctrl;
-	int ret = 0;
+	int ret;
 
 	qctrl = soc_camera_find_qctrl(&mt9m111_ops, ctrl->id);
 
@@ -739,7 +737,7 @@ static int mt9m111_set_control(struct soc_camera_device *icd,
 		ret = -EINVAL;
 	}
 
-	return ret < 0 ? -EIO : 0;
+	return ret;
 }
 
 int mt9m111_restore_state(struct soc_camera_device *icd)
@@ -763,10 +761,10 @@ static int mt9m111_resume(struct soc_camera_device *icd)
 
 	if (mt9m111->powered) {
 		ret = mt9m111_enable(icd);
-		if (ret >= 0)
+		if (!ret) {
 			mt9m111_reset(icd);
-		if (ret >= 0)
 			mt9m111_restore_state(icd);
+		}
 	}
 	return ret;
 }
@@ -778,15 +776,13 @@ static int mt9m111_init(struct soc_camera_device *icd)
 
 	mt9m111->context = HIGHPOWER;
 	ret = mt9m111_enable(icd);
-	if (ret >= 0)
+	if (!ret) {
 		mt9m111_reset(icd);
-	if (ret >= 0)
 		mt9m111_set_context(icd, mt9m111->context);
-	if (ret >= 0)
 		mt9m111_set_autoexposure(icd, mt9m111->autoexposure);
-	if (ret < 0)
+	} else
 		dev_err(&icd->dev, "mt9m111 init failed: %d\n", ret);
-	return ret ? -EIO : 0;
+	return ret;
 }
 
 static int mt9m111_release(struct soc_camera_device *icd)
@@ -797,7 +793,7 @@ static int mt9m111_release(struct soc_camera_device *icd)
 	if (ret < 0)
 		dev_err(&icd->dev, "mt9m111 release failed: %d\n", ret);
 
-	return ret ? -EIO : 0;
+	return ret;
 }
 
 /*

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] mt9m111: style cleanup

[Robert Jarzmik]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

> Fix a typo in Kconfig, simplify error checking, further minor cleanup.
>
> Signed-off-by: Guennadi Liakhovetski <g.iakhovetski@gmx.de>
Tested-by: Robert Jarzmik <robert.jarzmik@free.fr>

But ...

> @@ -778,15 +776,13 @@ static int mt9m111_init(struct soc_camera_device *icd)
>  
>  	mt9m111->context = HIGHPOWER;
>  	ret = mt9m111_enable(icd);
> -	if (ret >= 0)
> +	if (!ret) {
>  		mt9m111_reset(icd);
> -	if (ret >= 0)
>  		mt9m111_set_context(icd, mt9m111->context);
> -	if (ret >= 0)
>  		mt9m111_set_autoexposure(icd, mt9m111->autoexposure);
> -	if (ret < 0)
> +	} else
>  		dev_err(&icd->dev, "mt9m111 init failed: %d\n", ret);
> -	return ret ? -EIO : 0;
> +	return ret;
>  }
You changed the fault path here : you don't check if every call succeeds. I
don't think it's very important though ... I certainly don't care that much.

Plus one style issue : IIRC, if there are braces on the if statement, there must
be braces on the else statement, even if it's a one line else block =>
+	} else {
 		dev_err(&icd->dev, "mt9m111 init failed: %d\n", ret);
+	}

--
Robert

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] mt9m111: style cleanup

[Guennadi Liakhovetski]

On Wed, 13 Aug 2008, Robert Jarzmik wrote:

> Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:
> 
> > Fix a typo in Kconfig, simplify error checking, further minor cleanup.
> >
> > Signed-off-by: Guennadi Liakhovetski <g.iakhovetski@gmx.de>
> Tested-by: Robert Jarzmik <robert.jarzmik@free.fr>
> 
> But ...
> 
> > @@ -778,15 +776,13 @@ static int mt9m111_init(struct soc_camera_device *icd)
> >  
> >  	mt9m111->context = HIGHPOWER;
> >  	ret = mt9m111_enable(icd);
> > -	if (ret >= 0)
> > +	if (!ret) {
> >  		mt9m111_reset(icd);
> > -	if (ret >= 0)
> >  		mt9m111_set_context(icd, mt9m111->context);
> > -	if (ret >= 0)
> >  		mt9m111_set_autoexposure(icd, mt9m111->autoexposure);
> > -	if (ret < 0)
> > +	} else
> >  		dev_err(&icd->dev, "mt9m111 init failed: %d\n", ret);
> > -	return ret ? -EIO : 0;
> > +	return ret;
> >  }
> You changed the fault path here : you don't check if every call succeeds. I
> don't think it's very important though ... I certainly don't care that much.

Sorry, don't understand, you don't set "ret" in the above calls, so, I 
don't think I changed anything.

> Plus one style issue : IIRC, if there are braces on the if statement, there must
> be braces on the else statement, even if it's a one line else block =>
> +	} else {
>  		dev_err(&icd->dev, "mt9m111 init failed: %d\n", ret);
> +	}

Yeah, I think this style is preferred...

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] mt9m111: style cleanup

[Robert Jarzmik]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

>> > @@ -778,15 +776,13 @@ static int mt9m111_init(struct soc_camera_device *icd)
>> >  
>> >  	mt9m111->context = HIGHPOWER;
>> >  	ret = mt9m111_enable(icd);
>> > -	if (ret >= 0)
>> > +	if (!ret) {
>> >  		mt9m111_reset(icd);
>> > -	if (ret >= 0)
>> >  		mt9m111_set_context(icd, mt9m111->context);
>> > -	if (ret >= 0)
>> >  		mt9m111_set_autoexposure(icd, mt9m111->autoexposure);
>> > -	if (ret < 0)
>> > +	} else
>> >  		dev_err(&icd->dev, "mt9m111 init failed: %d\n", ret);
>> > -	return ret ? -EIO : 0;
>> > +	return ret;
>> >  }
>> You changed the fault path here : you don't check if every call succeeds. I
>> don't think it's very important though ... I certainly don't care that much.
>
> Sorry, don't understand, you don't set "ret" in the above calls, so, I 
> don't think I changed anything.
That is because I'm a fool :)
The correct behaviour would be something like :

@@ -778,15 +776,13 @@ static int mt9m111_init(struct soc_camera_device *icd)
 
 	mt9m111->context = HIGHPOWER;
 	ret = mt9m111_enable(icd);
-	if (ret >= 0)
+	if (!ret)
 		ret = mt9m111_reset(icd);
-	if (ret >= 0)
+	if (!ret)
 		ret = mt9m111_set_context(icd, mt9m111->context);
-	if (ret >= 0)
+	if (!ret)
 		ret = mt9m111_set_autoexposure(icd, mt9m111->autoexposure);
-	if (ret < 0)
+	if (ret)
 		dev_err(&icd->dev, "mt9m111 init failed: %d\n", ret);
-	return ret ? -EIO : 0;
+	return ret;
 }

--
Robert

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] mt9m111: style cleanup

[Guennadi Liakhovetski]

On Wed, 13 Aug 2008, Robert Jarzmik wrote:

> The correct behaviour would be something like :
> 
> @@ -778,15 +776,13 @@ static int mt9m111_init(struct soc_camera_device *icd)
>  
>  	mt9m111->context = HIGHPOWER;
>  	ret = mt9m111_enable(icd);
> -	if (ret >= 0)
> +	if (!ret)
>  		ret = mt9m111_reset(icd);
> -	if (ret >= 0)
> +	if (!ret)
>  		ret = mt9m111_set_context(icd, mt9m111->context);
> -	if (ret >= 0)
> +	if (!ret)
>  		ret = mt9m111_set_autoexposure(icd, mt9m111->autoexposure);
> -	if (ret < 0)
> +	if (ret)
>  		dev_err(&icd->dev, "mt9m111 init failed: %d\n", ret);
> -	return ret ? -EIO : 0;
> +	return ret;
>  }

And in mt9m111_resume()? Do you want to check return codes of each 
function there too?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] mt9m111: style cleanup

[Robert Jarzmik]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

> And in mt9m111_resume()? Do you want to check return codes of each 
> function there too?
Yes, in mt9m111_resume() too, please.

Cheers.

--
Robert

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list