[PATCH 1/3 v3] Add camera support for A780 and A910 EZX phones

On Wed, 11 Nov 2009 19:02:11 +0100 (CET)
Guennadi Liakhovetski [off-list ref] wrote:

Hi Antonio

Still one more nitpick:

Comments below.

On Wed, 11 Nov 2009, Antonio Ospite wrote:

[...]

quoted

 
+/* camera */
+static int a780_camera_init(void)

This function returns an error but...

quoted

+{
+	int err;
+
+	/*
+	 * GPIO50_nCAM_EN is active low
+	 * GPIO19_GEN1_CAM_RST is active on rising edge
+	 */
+	err = gpio_request(GPIO50_nCAM_EN, "nCAM_EN");
+	if (err) {
+		pr_err("%s: Failed to request nCAM_EN\n", __func__);
+		goto fail;
+	}

[...]

quoted

+	return err;
+}
+

[...]

quoted

 static void __init a780_init(void)

@@ -699,6 +782,9 @@ static void __init a780_init(void)
 
 	pxa_set_keypad_info(&a780_keypad_platform_data);
 
+	a780_camera_init();

...it is not used. So, I would either make it void, or check the return 
code, and maybe even not register the camera since it's going to be 
useless anyway? Same for a910.

Actually I was keeping returning an error just in case there would be a
soc-camera .init() someday. But it's indeed a good idea to check the
return value once that we have it.

I am inlining here only the incremental change for a faster review,
I am going to send v4 of the patch separately for you ACK, hopefully :).

Note, now the return value of platform_device_register is ignored but
this is not grave, I guess.

Thanks for your time,
   Antonio

diff --git a/arch/arm/mach-pxa/ezx.c b/arch/arm/mach-pxa/ezx.c
index 74423a6..1a73b7b 100644
--- a/arch/arm/mach-pxa/ezx.c
+++ b/arch/arm/mach-pxa/ezx.c

@@ -767,7 +767,6 @@ static struct platform_device a780_camera = {
 
 static struct platform_device *a780_devices[] __initdata = {
 	&a780_gpio_keys,
-	&a780_camera,
 };
 
 static void __init a780_init(void)

@@ -782,8 +781,10 @@ static void __init a780_init(void)
 
 	pxa_set_keypad_info(&a780_keypad_platform_data);
 
-	a780_camera_init();
-	pxa_set_camera_info(&a780_pxacamera_platform_data);
+	if (a780_camera_init() == 0) {
+		pxa_set_camera_info(&a780_pxacamera_platform_data);
+		platform_device_register(&a780_camera);
+	}
 
 	platform_add_devices(ARRAY_AND_SIZE(ezx_devices));
 	platform_add_devices(ARRAY_AND_SIZE(a780_devices));

@@ -1026,7 +1027,6 @@ static struct platform_device a910_camera = {
 
 static struct platform_device *a910_devices[] __initdata = {
 	&a910_gpio_keys,
-	&a910_camera,
 };
 
 static void __init a910_init(void)

@@ -1041,8 +1041,10 @@ static void __init a910_init(void)
 
 	pxa_set_keypad_info(&a910_keypad_platform_data);
 
-	a910_camera_init();
-	pxa_set_camera_info(&a910_pxacamera_platform_data);
+	if (a910_camera_init() == 0) {
+		pxa_set_camera_info(&a910_pxacamera_platform_data);
+		platform_device_register(&a910_camera);
+	}
 
 	platform_add_devices(ARRAY_AND_SIZE(ezx_devices));
 	platform_add_devices(ARRAY_AND_SIZE(a910_devices));

-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20091112/ff2fa21b/attachment.sig>