Re: [PATCH 1/3 v2] ezx: Add camera support for A780 and A910 EZX phones
Ping.
Guennadi, did you see the patch below? Or I should completely remove
the .init() callback like you said in another message?
As I said, my humble preference would be to keep GPIOs setup local to
the driver somehow, but you just tell me what to do :)
On Fri, 6 Nov 2009 18:29:10 +0100
Antonio Ospite osp...@studenti.unina.it wrote:
On Fri, 6 Nov 2009 15:11:55 +0100 (CET)
Guennadi Liakhovetski g.liakhovet...@gmx.de wrote:
On Thu, 5 Nov 2009, Antonio Ospite wrote:
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?
pgp7eMfkgWJNi.pgp
Description: PGP signature
Re: [PATCH 1/3 v2] ezx: Add camera support for A780 and A910 EZX phones
On Tue, 10 Nov 2009, Antonio Ospite wrote: Ping. Guennadi, did you see the patch below? Or I should completely remove the .init() callback like you said in another message? As I said, my humble preference would be to keep GPIOs setup local to the driver somehow, but you just tell me what to do :) Yes, please make GPIO config static and remove .init. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3 v2] ezx: Add camera support for A780 and A910 EZX phones
On Thu, 5 Nov 2009 00:53:46 +0100 (CET)
Guennadi Liakhovetski g.liakhovet...@gmx.de wrote:
On Wed, 4 Nov 2009, Antonio Ospite wrote:
Signed-off-by: Antonio Ospite osp...@studenti.unina.it
Signed-off-by: Bart Visscher ba...@thisnet.nl
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.
Changes since previous version:
Addressed all the comments from Eric and Guennadi.
I said I still wanted to have a better look at it, so, basically, I just
think you have a typo in two comments:
Or the code is wrong, even if it works :)
Let's sort this out.
[...]
+/* camera */
+static int a780_pxacamera_init(struct device *dev)
+{
+ int err;
+
+ /*
+* GPIO50_nCAM_EN is active low
+* GPIO19_GEN1_CAM_RST is active high
+*/
+ err = gpio_request(GPIO50_nCAM_EN, nCAM_EN);
+ if (err) {
+ pr_err(%s: Failed to request nCAM_EN\n, __func__);
+ goto fail;
+ }
+
+ err = gpio_request(GPIO19_GEN1_CAM_RST, CAM_RST);
+ if (err) {
+ pr_err(%s: Failed to request CAM_RST\n, __func__);
+ goto fail_gpio_cam_rst;
+ }
+
+ gpio_direction_output(GPIO50_nCAM_EN, 0);
+ gpio_direction_output(GPIO19_GEN1_CAM_RST, 1);
Don't understand this, are you sure your comments are correct? This would
mean, that in init() you enable the camera and hold it in reset.
I am pretty confident the comments are right,
they came from runtime analysis with gpiotool on original firmware.
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.
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 2600
pxa27x-camera pxa27x-camera.0: LCD clock 10400Hz, target freq 2600Hz,
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: 07e0 ISR: 0002
i2c: log: [0446:07e0]
mt9m111 0-005d: read reg.00d - ff87
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()?
+
+ return 0;
+
+fail_gpio_cam_rst:
+ gpio_free(GPIO50_nCAM_EN);
+fail:
+ return err;
+}
+
+static int a780_pxacamera_power(struct device *dev, int on)
+{
+ gpio_set_value(GPIO50_nCAM_EN, !on);
This agrees with the comment above, but then why do you enable it
immediately in init?
See above.
+ return 0;
+}
+
+static int a780_pxacamera_reset(struct device *dev)
+{
+ gpio_set_value(GPIO19_GEN1_CAM_RST, 0);
+ msleep(10);
+ gpio_set_value(GPIO19_GEN1_CAM_RST, 1);
This, however, seems to contradict the comment and confirm the code above
- looks like your reset pin is active low too?
Well, reset happens when bringing the GPIO to HIGH but only after some
time it was LOW, so I consider it active high but maybe I am messing up
with terminology here?
[...]
+static struct soc_camera_link a780_iclink = {
+ .bus_id = 0,
+ .flags = SOCAM_SENSOR_INVERT_PCLK,
Do you have schematics or have you found this out experimentally? What
happens if you don't invert pclk?
I've found this experimentally, and I think I was the reason why you
introduced SOCAM_SENSOR_INVERT_PCLK, see
http://thread.gmane.org/gmane.comp.video.video4linux/40592
If I don't invert pixelclock I get a picture like this:
http://people.openezx.org/ao2/a780-not-yet.jpg
[...]
A general question for the ezx.c: wouldn't it be better to convert that
full-of-ifdef's file to a mach-pxa/ezx/ directory?
Actually we are fine using a single file, there are many parts shared
between different
