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

On Fri, 6 Nov 2009 15:11:55 +0100 (CET)
Guennadi Liakhovetski [off-list ref] wrote:

On Thu, 5 Nov 2009, Antonio Ospite wrote:

quoted

On Thu, 5 Nov 2009 00:53:46 +0100 (CET)
Guennadi Liakhovetski [off-list ref] wrote:

quoted

On Wed, 4 Nov 2009, Antonio Ospite wrote:

quoted

Signed-off-by: Antonio Ospite <redacted>
Signed-off-by: Bart Visscher <redacted>

Is this patch going via Bart? Or should this be an Acked-by?

Bart did the initial research and wrote a first, partially working,
version of the patch for A780, then I made it work and refactored it,
adding code for A910. So I put two SOBs here as we are both the
authors, in a sense.

Then, I think, his Sob should be the first.

Ok.

quoted

[...] 

quoted

quoted

+/* camera */
+static int a780_pxacamera_init(struct device *dev)
+{
+	int err;
+
+	/*
+	 * GPIO50_nCAM_EN is active low
+	 * GPIO19_GEN1_CAM_RST is active high
+	 */

[...]

quoted

quoted

quoted

+	gpio_direction_output(GPIO50_nCAM_EN, 0);
+	gpio_direction_output(GPIO19_GEN1_CAM_RST, 1);

[...]

quoted

The reset happens when bringing the signal from low to high, so
holding it high or low it's basically the same here, and given how 
a780_pxacamera_reset() works I decided to keep it high.

Then it is not level- but edge-sensitive. Maybe put reset - rising edge to 
activate, or something like that.

You mean I fix the comment, right?

quoted

I also need to put CAM_EN active in init(), because of how
soc_camera_probe() works, adding some debug I get this call sequence
(with CAM_EN not active):

Linux video capture interface: v2.00
pxa27x-camera pxa27x-camera.0: Limiting master clock to 26000000
pxa27x-camera pxa27x-camera.0: LCD clock 104000000Hz, target freq 26000000Hz, divisor 1
pxa27x-camera pxa27x-camera.0: got DMA channel 1
pxa27x-camera pxa27x-camera.0: got DMA channel (U) 2
pxa27x-camera pxa27x-camera.0: got DMA channel (V) 3
camera 0-0: Probing 0-0
camera 0-0: soc_camera_probe: power 1
--> a780_pxacamera_power: called. on: 1  !on: 0
camera 0-0: soc_camera_probe: reset
--> a780_pxacamera_reset: called
pxa27x-camera pxa27x-camera.0: Registered platform device at cc8a5380 data c03a40a8
pxa27x-camera pxa27x-camera.0: pxa_camera_activate: Init gpios
--> a780_pxacamera_init: called
pxa27x-camera pxa27x-camera.0: PXA Camera driver attached to camera 0
camera 0-0: mt9m111_video_probe: called
i2c: error: exhausted retries
i2c: msg_num: 0 msg_idx: -2000 msg_ptr: 0
i2c: ICR: 000007e0 ISR: 00000002
i2c: log: [00000446:000007e0] 
mt9m111 0-005d: read  reg.00d -> ffffff87
mt9m111 0-005d: mt9m11x init failed: -121
mt9m111: probe of 0-005d failed with error -121
pxa27x-camera pxa27x-camera.0: PXA Camera driver detached from camera 0
camera 0-0: soc_camera_probe: power 0
a780_pxacamera_power: called. on: 0  !on: 1
camera: probe of 0-0 failed with error -12

See? It's power(), reset(), init().
Maybe the problem is in soc_camera_probe()?

Sorry, you'd have to elaborate more on this. So, what's wrong with that 
sequence? it doesn't make sense to reset a powered down device or reset 
after init or whatever...

I mean, when probing (or even opening) the device, pxacamera.init()
is now called *after* the power ON and reset. If I kept disabled (high)
nCAM_EN in init(), as it should've been, this would have overridden
the previous power(1) call.

Isn't init() in pxacamera platform data meant to initialize the device
before it can be powered ON? In fact I am requesting the gpios in
a780_pxacamera_init, and if power() or reset() are called before it, then
they will be invalid, because the gpios have not been requested yet.

Moreover, pxacamera.init() is called in pxa_camera_activate, which is
called in pxa_camera_add_device, which in turn is invoked by
soc_camera_open() every time.
Shouldn't the init() method, where I request gpios, be called
only on probe?

Let me be more schematic, when probing the camera we have:

soc_camera_probe()         /* same in soc_camera_open! */
|-	icl->power(1)
|-	icl->reset()
|-	icd->ops->add()
	|-	pxacamera.init()  /* requesting gpios here! */
|-	video_dev_create(icd)
|-	...

Maybe we should have something like:

pxacamera.init()           /* request gpios only once! on probe. */
soc_camera_probe()         /* same in soc_camera_open */
|-	icl->power(1)
|-	icl->reset()
|-	icd->ops->add()
|-	video_dev_create(icd)
|-	...

Or, I'm missing what init() is supposed to do :)
Does a patch like this make sense to you?
(I've read the other mail about removing .init just before hitting Send,
this can be an alternative to removing it, having GPIOs setup in the
user driver seems clearer to me.)

diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c
index 6952e96..3101bcb 100644
--- a/drivers/media/video/pxa_camera.c
+++ b/drivers/media/video/pxa_camera.c

@@ -881,18 +882,8 @@ static void recalculate_fifo_timeout(struct pxa_camera_dev *pcdev,
 
 static void pxa_camera_activate(struct pxa_camera_dev *pcdev)
 {
-	struct pxacamera_platform_data *pdata = pcdev->pdata;
-	struct device *dev = pcdev->soc_host.v4l2_dev.dev;
 	u32 cicr4 = 0;
 
-	dev_dbg(dev, "Registered platform device at %p data %p\n",
-		pcdev, pdata);
-
-	if (pdata && pdata->init) {
-		dev_dbg(dev, "%s: Init gpios\n", __func__);
-		pdata->init(dev);
-	}
-
 	/* disable all interrupts */
 	__raw_writel(0x3ff, pcdev->base + CICR0);
 

@@ -1651,6 +1644,17 @@ static int __devinit pxa_camera_probe(struct platform_device *pdev)
 	pcdev->res = res;
 
 	pcdev->pdata = pdev->dev.platform_data;
+
+	dev_dbg(&pdev->dev, "Registered platform device at %p data %p\n",
+		pcdev, pcdev->pdata);
+
+	if (pcdev->pdata && pcdev->pdata->init) {
+		dev_dbg(&pdev->dev, "%s: Init gpios\n", __func__);
+		err = pcdev->pdata->init(&pdev->dev);
+		if (err)
+			goto exit_clk;
+	}
+
 	pcdev->platform_flags = pcdev->pdata->flags;
 	if (!(pcdev->platform_flags & (PXA_CAMERA_DATAWIDTH_8 |
 			PXA_CAMERA_DATAWIDTH_9 | PXA_CAMERA_DATAWIDTH_10))) {

-- 
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/20091106/3e43f2e4/attachment.sig>