From patchwork Fri Nov 6 17:29:10 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Antonio Ospite X-Patchwork-Id: 1979 Return-path: Envelope-to: mchehab@bombadil.infradead.org Delivery-date: Fri, 06 Nov 2009 18:20:38 +0000 Received: from bombadil.infradead.org [18.85.46.34] by pedra.chehab.org with IMAP (fetchmail-6.3.6) for (single-drop); Fri, 06 Nov 2009 16:22:05 -0200 (BRST) Received: from casper.infradead.org ([2001:770:15f::2]) by bombadil.infradead.org with esmtps (Exim 4.69 #1 (Red Hat Linux)) id 1N6TQM-0001fA-B7 for mchehab@bombadil.infradead.org; Fri, 06 Nov 2009 18:20:38 +0000 Received: from vger.kernel.org ([209.132.176.167]) by casper.infradead.org with esmtp (Exim 4.69 #1 (Red Hat Linux)) id 1N6T6f-0000Ub-1E; Fri, 06 Nov 2009 18:00:17 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758397AbZKFR3V (ORCPT + 1 other); Fri, 6 Nov 2009 12:29:21 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1754959AbZKFR3T (ORCPT ); Fri, 6 Nov 2009 12:29:19 -0500 Received: from smtp-out114.alice.it ([85.37.17.114]:4189 "EHLO smtp-out114.alice.it" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755186AbZKFR3O (ORCPT ); Fri, 6 Nov 2009 12:29:14 -0500 Received: from FBCMMO02.fbc.local ([192.168.68.196]) by smtp-out114.alice.it with Microsoft SMTPSVC(6.0.3790.3959); Fri, 6 Nov 2009 18:29:18 +0100 Received: from FBCMCL01B03.fbc.local ([192.168.69.84]) by FBCMMO02.fbc.local with Microsoft SMTPSVC(6.0.3790.3959); Fri, 6 Nov 2009 18:29:18 +0100 Received: from badebec ([79.3.140.214]) by FBCMCL01B03.fbc.local with Microsoft SMTPSVC(6.0.3790.3959); Fri, 6 Nov 2009 18:29:17 +0100 Date: Fri, 6 Nov 2009 18:29:10 +0100 From: Antonio Ospite To: Guennadi Liakhovetski Cc: linux-arm-kernel@lists.infradead.org, Eric Miao , openezx-devel@lists.openezx.org, Bart Visscher , Linux Media Mailing List Subject: Re: [PATCH 1/3 v2] ezx: Add camera support for A780 and A910 EZX phones Message-Id: <20091106182910.a3b48c41.ospite@studenti.unina.it> In-Reply-To: References: <1257367650-15056-1-git-send-email-ospite@studenti.unina.it> <20091105234429.ef855e2d.ospite@studenti.unina.it> X-Mailer: Sylpheed 2.7.1 (GTK+ 2.18.3; i686-pc-linux-gnu) X-Face: z*RaLf`X<@C75u6Ig9}{oW$H; 1_\2t5)({*|jhM/Vb; ]yA5\I~93>J<_`<4)A{':UrE Mime-Version: 1.0 X-OriginalArrivalTime: 06 Nov 2009 17:29:17.0231 (UTC) FILETIME=[AABB13F0:01CA5F06] Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org On Fri, 6 Nov 2009 15:11:55 +0100 (CET) Guennadi Liakhovetski wrote: > On Thu, 5 Nov 2009, Antonio Ospite wrote: > > > On Thu, 5 Nov 2009 00:53:46 +0100 (CET) > > Guennadi Liakhovetski wrote: > > > > > On Wed, 4 Nov 2009, Antonio Ospite wrote: > > > > > > > Signed-off-by: Antonio Ospite > > > > Signed-off-by: Bart Visscher > > > > > > 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. > > [...] > > > > +/* camera */ > > > > +static int a780_pxacamera_init(struct device *dev) > > > > +{ > > > > + int err; > > > > + > > > > + /* > > > > + * GPIO50_nCAM_EN is active low > > > > + * GPIO19_GEN1_CAM_RST is active high > > > > + */ [...] > > > > + gpio_direction_output(GPIO50_nCAM_EN, 0); > > > > + gpio_direction_output(GPIO19_GEN1_CAM_RST, 1); > > > [...] > > 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? > > 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))) {