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

On Wed, Nov 4, 2009 at 5:14 PM, Antonio Ospite [off-list ref] wrote:

On Wed, 4 Nov 2009 14:38:40 +0800
Eric Miao [off-list ref] wrote:

quoted

Hi Antonio,

Patch looks generally OK except for the MFP/GPIO usage, check my
comments below, thanks.

Ok, will resend ASAP. Some questions inlined below after your comments.

quoted

quoted

+/* camera */
+static int a780_pxacamera_init(struct device *dev)
+{
+ ? ? ? int err;
+
+ ? ? ? /*
+ ? ? ? ?* GPIO50_GPIO is CAM_EN: active low
+ ? ? ? ?* GPIO19_GPIO is CAM_RST: active high
+ ? ? ? ?*/
+ ? ? ? err = gpio_request(MFP_PIN_GPIO50, "nCAM_EN");

Mmm... MFP != GPIO, so this probably should be written simply as:

#define GPIO_nCAM_EN ?(50)

If the use of parentheses here is recommended, should I send another
patch to add them to current defines for GPIOs in ezx.c, for style
consistency?

I don't actually care about that - with or without parentheses are both OK
to me, though Guennadi recommends removing them, so it would be
just OK to left them untouched.

quoted

or (which tends to be more accurate but not necessary)

#define GPIO_nCAM_EN ?mfp_to_gpio(MFP_PIN_GPIO50)

For me it is the same, just tell me if you really prefer this one.

OK, the first/simple one pls

quoted

quoted

+
+static int a780_pxacamera_power(struct device *dev, int on)
+{
+ ? ? ? gpio_set_value(MFP_PIN_GPIO50, on ? 0 : 1);

? ? ? gpio_set_value(GPIO_nCAM_EN, on ? 0 : 1);

quoted

+
+#if 0
+ ? ? ? /*
+ ? ? ? ?* This is reported to resolve the "vertical line in view finder"
+ ? ? ? ?* issue (LIBff11930), in the original source code released by
+ ? ? ? ?* Motorola, but we never experienced the problem, so we don't use
+ ? ? ? ?* this for now.
+ ? ? ? ?*
+ ? ? ? ?* AP Kernel camera driver: set TC_MM_EN to low when camera is running
+ ? ? ? ?* and TC_MM_EN to high when camera stops.
+ ? ? ? ?*
+ ? ? ? ?* BP Software: if TC_MM_EN is low, BP do not shut off 26M clock, but
+ ? ? ? ?* BP can sleep itself.
+ ? ? ? ?*/
+ ? ? ? gpio_set_value(MFP_PIN_GPIO99, on ? 0 : 1);
+#endif

This is a little bit confusing - can we remove this for this stage?

Ok, I am removing it for now. I might put this note in again in
future, hopefully with a better description.

That would be good.