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 <ospite@xxxxxxxxxxxxxxxxx> wrote:
> On Fri, 6 Nov 2009 15:11:55 +0100 (CET)
> Guennadi Liakhovetski <g.liakhovetski@xxxxxx> 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?
Attachment:
pgpy9qpHYe3Np.pgp
Description: PGP signature
