From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mx2.mikrovisata.net ([217.17.85.7]:37098 "EHLO mx2.mikrovisata.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932542AbZCZTbm (ORCPT ); Thu, 26 Mar 2009 15:31:42 -0400 Message-ID: <49CBD53C.6060700@gmail.com> Date: Thu, 26 Mar 2009 21:19:24 +0200 From: Darius Augulis MIME-Version: 1.0 To: Guennadi Liakhovetski CC: linux-arm-kernel@lists.arm.linux.org.uk, Linux Media Mailing List , Paulius Zaleckas , Sascha Hauer Subject: Re: [PATCH 1/5] CSI camera interface driver for MX1 References: <49C89F00.1020402@gmail.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-media-owner@vger.kernel.org List-ID: Guennadi Liakhovetski wrote: > Sascha, > > would you prefer me to pull this via soc-camera or you'd prefer to handle > it in your mxc tree? I think it's better to pull it via v4l, so, I'd need > your acks for platform parts, especially for the assembly, ksyms and FIQ > code. > > Hi Darius, > Hi Guennadi, Sascha, > On Tue, 24 Mar 2009, Darius wrote: > > Please, send your patches inline next time. Also, as noticed inline, > you'll have to rebase this onto a current v4l stack, e.g., linux-next. > ok, I just started to use stgit now. > From: Paulius Zaleckas > > Driver for i.MX1/L camera (CSI) host. > > Signed-off-by: Paulius Zaleckas > > You are forwarding his patch, so, you have to sign-off under it. Why isn't > he submitting it himself? > clear. This is because we work together on two archs - MXC and Gemini. Paulius will maintain all our Gemini patches and I will take care about MXC, also old patches from Paulius. I will need some time to study this CSI and driver code, then I could fix your comments. Thank you for review and notes! > --- > > Index: linux-2.6.29/drivers/media/video/imx_camera.c > =================================================================== > >> --- /dev/null >> +++ linux-2.6.29/drivers/media/video/imx_camera.c >> @@ -0,0 +1,872 @@ >> +/* >> + * V4L2 Driver for i.MX camera (CSI) host >> + * >> + * Copyright (C) 2008, Paulius Zaleckas >> + * >> + * Based on PXA SoC camera driver >> + * Copyright (C) 2006, Sascha Hauer, Pengutronix >> + * Copyright (C) 2008, Guennadi Liakhovetski >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; either version 2 of the License, or >> + * (at your option) any later version. >> > > Do you really want to allow later GPL versions? I think, v2 only is > preferred currently. > > >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include >> +#include >> +#include >> +#include >> + >> +#include >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> > > It makes patching a bit easier, if includes are alphabetically ordered. > So, if in the future you get two patches each adding one > include, if entries are alphabetically sorted, they might lie apart and > patches will still apply. Whereas if entries are not sorted, people will > just append their entries and patches will conflict. > > >> + >> +/* >> + * CSI registers >> + */ >> + >> +#define DMA_DIMR 0x08 /* Interrupt mask Register */ >> +#define DMA_CCR(x) (0x8c + ((x) << 6)) /* Control Registers */ >> + >> +#define CSICR1 0x00 /* CSI Control Register 1 */ >> +#define CSISR 0x08 /* CSI Status Register */ >> +#define CSIRXR 0x10 /* CSI RxFIFO Register */ >> + >> +#define CSICR1_RXFF_LEVEL(x) (((x) & 0x3) << 19) >> +#define CSICR1_SOF_POL (1 << 17) >> +#define CSICR1_SOF_INTEN (1 << 16) >> +#define CSICR1_MCLKDIV(x) (((x) & 0xf) << 12) >> +#define CSICR1_MCLKEN (1 << 9) >> +#define CSICR1_FCC (1 << 8) >> +#define CSICR1_BIG_ENDIAN (1 << 7) >> +#define CSICR1_CLR_RXFIFO (1 << 5) >> +#define CSICR1_GCLK_MODE (1 << 4) >> +#define CSICR1_REDGE (1 << 1) >> +#define CSICR1_EN (1 << 0) >> + >> +#define CSISR_SFF_OR_INT (1 << 25) >> +#define CSISR_RFF_OR_INT (1 << 24) >> +#define CSISR_STATFF_INT (1 << 21) >> +#define CSISR_RXFF_INT (1 << 18) >> +#define CSISR_SOF_INT (1 << 16) >> +#define CSISR_DRDY (1 << 0) >> + >> +#define VERSION_CODE KERNEL_VERSION(0, 0, 1) >> +#define DRIVER_NAME "imx-csi" >> + >> +#define CSI_IRQ_MASK (CSISR_SFF_OR_INT | CSISR_RFF_OR_INT | CSISR_STATFF_INT | \ >> + CSISR_RXFF_INT | CSISR_SOF_INT) >> + >> +#define CSI_BUS_FLAGS (SOCAM_MASTER | SOCAM_HSYNC_ACTIVE_HIGH | \ >> + SOCAM_VSYNC_ACTIVE_HIGH | SOCAM_VSYNC_ACTIVE_LOW | \ >> + SOCAM_PCLK_SAMPLE_RISING | SOCAM_PCLK_SAMPLE_FALLING | \ >> + SOCAM_DATAWIDTH_8) >> + >> +static DEFINE_MUTEX(camera_lock); >> + >> +static int mclk; >> + >> +/* >> + * Structures >> + */ >> + >> +/* buffer for one video frame */ >> +struct imx_buffer { >> + /* common v4l buffer stuff -- must be first */ >> + struct videobuf_buffer vb; >> > > Here you have one space > > >> + >> + const struct soc_camera_data_format *fmt; >> > > Here you have 8 spaces > > >> + >> + int inwork; >> > > Here you have tabs. Please, unify. > > >> +}; >> + >> +struct imx_camera_dev { >> + struct device *dev; >> + /* i.MX is only supposed to handle one camera on its Camera Sensor >> + * Interface. If anyone ever builds hardware to enable more than >> + * one camera, they will have to modify this driver too */ >> + struct soc_camera_device *icd; >> + >> + unsigned int irq; >> + void __iomem *base; >> + >> + int dma_chan; >> + >> + struct imxcamera_platform_data *pdata; >> + struct resource *res; >> + struct clk *clk; >> + unsigned long platform_mclk_10khz; >> + >> + struct list_head capture; >> + >> + spinlock_t lock; >> + >> + struct imx_buffer *active; >> +}; >> + >> +static const char *imx_cam_driver_description = "i.MX_Camera"; >> + >> +/* >> + * Videobuf operations >> + */ >> +static int imx_videobuf_setup(struct videobuf_queue *vq, unsigned int *count, >> + unsigned int *size) >> +{ >> + struct soc_camera_device *icd = vq->priv_data; >> + >> + *size = icd->width * icd->height * >> + ((icd->current_fmt->depth + 7) >> 3); >> + >> + if (0 == *count) >> + *count = 32; >> > > Doesn't look like a good idea to me. You don't restrict picture sizes in > your try_fmt / set_fmt and here you default to 32 buffers... > > >> + >> + dev_dbg(&icd->dev, "count=%d, size=%d\n", *count, *size); >> + >> + return 0; >> +} >> + >> +static void free_buffer(struct videobuf_queue *vq, struct imx_buffer *buf) >> +{ >> + struct soc_camera_device *icd = vq->priv_data; >> > > The compiler should do this for you, but the code would look a bit cleaner > with a > > struct videobuf_buffer *vb = &buf->vb; > > >> + >> + BUG_ON(in_interrupt()); >> + >> + dev_dbg(&icd->dev, "%s (vb=0x%p) 0x%08lx %d\n", __func__, >> + &buf->vb, buf->vb.baddr, buf->vb.bsize); >> + >> + /* This waits until this buffer is out of danger, i.e., until it is no >> + * longer in STATE_QUEUED or STATE_ACTIVE */ >> + videobuf_waiton(&buf->vb, 0, 0); >> + videobuf_dma_contig_free(vq, &buf->vb); >> + >> + buf->vb.state = VIDEOBUF_NEEDS_INIT; >> +} >> + >> +static int imx_videobuf_prepare(struct videobuf_queue *vq, >> + struct videobuf_buffer *vb, enum v4l2_field field) >> +{ >> + struct soc_camera_device *icd = vq->priv_data; >> + struct imx_buffer *buf = container_of(vb, struct imx_buffer, vb); >> + int ret; >> + >> + dev_dbg(&icd->dev, "%s (vb=0x%p) 0x%08lx %d\n", __func__, >> + vb, vb->baddr, vb->bsize); >> + >> + /* Added list head initialization on alloc */ >> + WARN_ON(!list_empty(&vb->queue)); >> + >> + BUG_ON(NULL == icd->current_fmt); >> + >> + /* I think, in buf_prepare you only have to protect global data, >> + * the actual buffer is yours */ >> + buf->inwork = 1; >> + >> + if (buf->fmt != icd->current_fmt || >> + vb->width != icd->width || >> + vb->height != icd->height || >> + vb->field != field) { >> + buf->fmt = icd->current_fmt; >> + vb->width = icd->width; >> + vb->height = icd->height; >> + vb->field = field; >> + vb->state = VIDEOBUF_NEEDS_INIT; >> + } >> + >> + vb->size = vb->width * vb->height * ((buf->fmt->depth + 7) >> 3); >> + if (0 != vb->baddr && vb->bsize < vb->size) { >> + ret = -EINVAL; >> + goto out; >> + } >> + >> + if (vb->state == VIDEOBUF_NEEDS_INIT) { >> + ret = videobuf_iolock(vq, vb, NULL); >> + if (ret) >> + goto fail; >> + >> + vb->state = VIDEOBUF_PREPARED; >> + } >> + >> + buf->inwork = 0; >> + >> + return 0; >> + >> +fail: >> + free_buffer(vq, buf); >> +out: >> + buf->inwork = 0; >> + return ret; >> +} >> + >> +static int imx_camera_setup_dma(struct imx_camera_dev *pcdev) >> +{ >> + struct videobuf_buffer *vbuf = &pcdev->active->vb; >> + int ret; >> + >> + if (unlikely(!pcdev->active)) { >> + dev_err(pcdev->dev, "DMA End IRQ with no active buffer\n"); >> + return -EFAULT; >> + } >> + >> + /* setup sg list for future DMA */ >> + ret = imx_dma_setup_single(pcdev->dma_chan, >> + videobuf_to_dma_contig(vbuf), >> + vbuf->size, pcdev->res->start + >> + CSIRXR, DMA_MODE_READ); >> + if(unlikely(ret)) >> > > Space missing > > >> + dev_err(pcdev->dev, "Failed to setup DMA sg list\n"); >> + >> + return ret; >> +} >> + >> +static void imx_videobuf_queue(struct videobuf_queue *vq, >> + struct videobuf_buffer *vb) >> +{ >> + struct soc_camera_device *icd = vq->priv_data; >> + struct soc_camera_host *ici = >> + to_soc_camera_host(icd->dev.parent); >> > > No need to break the line. > > >> + struct imx_camera_dev *pcdev = ici->priv; >> + struct imx_buffer *buf = container_of(vb, struct imx_buffer, vb); >> + unsigned long flags; >> + >> + dev_dbg(&icd->dev, "%s (vb=0x%p) 0x%08lx %d\n", __func__, >> + vb, vb->baddr, vb->bsize); >> + spin_lock_irqsave(&pcdev->lock, flags); >> > > You use an FIQ for SoF, and spin_lock_irqsave() to protect. Don't they > only disable IRQs and not FIQs? But it seems your FIQ cannot cause any > trouble, so, it should be fine. Do you really need an FIQ? > > >> + >> + list_add_tail(&vb->queue, &pcdev->capture); >> + >> + vb->state = VIDEOBUF_ACTIVE; >> + >> + if (!pcdev->active) { >> + pcdev->active = buf; >> + >> + /* setup sg list for future DMA */ >> + if (!imx_camera_setup_dma(pcdev)) { >> + unsigned int temp; >> + /* enable SOF irq */ >> + temp = __raw_readl(pcdev->base + CSICR1) | >> + CSICR1_SOF_INTEN; >> > > Hm, looks like an error in the datasheet: > > SOF_INTEN Start Of Frame Interrupt--SOF interrupt status bit; set Read: > Bit 16 when interrupt occurs. 0 = No interrupt > 1 = SOF interrupt occurred > Write: > 0 = No action > 1 = Clears bit > > This is not a status bit, but a control "SoF interrupt enable" bit, right? > > >> + __raw_writel(temp, pcdev->base + CSICR1); >> + } >> + } >> + >> + spin_unlock_irqrestore(&pcdev->lock, flags); >> +} >> + >> +static void imx_videobuf_release(struct videobuf_queue *vq, >> + struct videobuf_buffer *vb) >> +{ >> + struct imx_buffer *buf = container_of(vb, struct imx_buffer, vb); >> +#ifdef DEBUG >> + struct soc_camera_device *icd = vq->priv_data; >> + >> + dev_dbg(&icd->dev, "%s (vb=0x%p) 0x%08lx %d\n", __func__, >> + vb, vb->baddr, vb->bsize); >> + >> + switch (vb->state) { >> + case VIDEOBUF_ACTIVE: >> + dev_dbg(&icd->dev, "%s (active)\n", __func__); >> + break; >> + case VIDEOBUF_QUEUED: >> + dev_dbg(&icd->dev, "%s (queued)\n", __func__); >> + break; >> + case VIDEOBUF_PREPARED: >> + dev_dbg(&icd->dev, "%s (prepared)\n", __func__); >> + break; >> + default: >> + dev_dbg(&icd->dev, "%s (unknown)\n", __func__); >> + break; >> + } >> +#endif >> + >> + free_buffer(vq, buf); >> +} >> + >> +static inline void imx_camera_wakeup(struct imx_camera_dev *pcdev, >> > > No need for "inline" > > >> + struct videobuf_buffer *vb, >> + struct imx_buffer *buf) >> +{ >> + /* _init is used to debug races, see comment in imx_camera_reqbufs() */ >> + list_del_init(&vb->queue); >> + vb->state = VIDEOBUF_DONE; >> + do_gettimeofday(&vb->ts); >> + vb->field_count++; >> + wake_up(&vb->done); >> + >> + if (list_empty(&pcdev->capture)) { >> + pcdev->active = NULL; >> + return; >> + } >> + >> + pcdev->active = list_entry(pcdev->capture.next, >> + struct imx_buffer, vb.queue); >> + >> + /* setup sg list for future DMA */ >> + if (likely(!imx_camera_setup_dma(pcdev))) { >> + unsigned int temp; >> + >> + /* enable SOF irq */ >> + temp = __raw_readl(pcdev->base + CSICR1) | CSICR1_SOF_INTEN; >> + __raw_writel(temp, pcdev->base + CSICR1); >> + } >> +} >> + >> +static void imx_camera_dma_irq(int channel, void *data) >> +{ >> + struct imx_camera_dev *pcdev = data; >> + struct imx_buffer *buf; >> + unsigned long flags; >> + struct videobuf_buffer *vb; >> + >> + spin_lock_irqsave(&pcdev->lock, flags); >> + >> + imx_dma_disable(channel); >> + >> + if (unlikely(!pcdev->active)) { >> + dev_err(pcdev->dev, "DMA End IRQ with no active buffer\n"); >> + goto out; >> + } >> + >> + vb = &pcdev->active->vb; >> + buf = container_of(vb, struct imx_buffer, vb); >> + WARN_ON(buf->inwork || list_empty(&vb->queue)); >> + dev_dbg(pcdev->dev, "%s (vb=0x%p) 0x%08lx %d\n", __func__, >> + vb, vb->baddr, vb->bsize); >> + >> + imx_camera_wakeup(pcdev, vb, buf); >> > > AFAIU, your flow looks as follows: > > 1. configure DMA, enable Start of Frame FIQ > 2. enable DMA, DMA IRQ, disable SoF FIQ > 3. disable DMA, report completed buffer, goto 1 > > >> + >> +out: >> + spin_unlock_irqrestore(&pcdev->lock, flags); >> +} >> + >> +static struct videobuf_queue_ops imx_videobuf_ops = { >> + .buf_setup = imx_videobuf_setup, >> + .buf_prepare = imx_videobuf_prepare, >> + .buf_queue = imx_videobuf_queue, >> + .buf_release = imx_videobuf_release, >> > > You use spaces before "=", but you're not aligning on "longest line plus > one space," but on the next tab position... This is not critical, just > seems strange to me. > > >> +}; >> + >> +static void imx_camera_init_videobuf(struct videobuf_queue *q, >> + struct soc_camera_device *icd) >> +{ >> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent); >> + struct imx_camera_dev *pcdev = ici->priv; >> + >> + videobuf_queue_dma_contig_init(q, &imx_videobuf_ops, pcdev->dev, >> + &pcdev->lock, >> + V4L2_BUF_TYPE_VIDEO_CAPTURE, >> + V4L2_FIELD_NONE, >> + sizeof(struct imx_buffer), icd); >> +} >> + >> +static int mclk_get_divisor(struct imx_camera_dev *pcdev) >> +{ >> + unsigned int mclk_10khz = pcdev->platform_mclk_10khz; >> + unsigned long div; >> + unsigned long lcdclk; >> + >> + lcdclk = clk_get_rate(pcdev->clk) / 10000; >> + >> + /* We verify platform_mclk_10khz != 0, so if anyone breaks it, here >> + * they get a nice Oops */ >> + div = (lcdclk + 2 * mclk_10khz - 1) / (2 * mclk_10khz) - 1; >> + >> + dev_dbg(pcdev->dev, "System clock %lukHz, target freq %dkHz, " >> + "divisor %lu\n", lcdclk * 10, mclk_10khz * 10, div); >> + >> + return div; >> +} >> + >> +static void imx_camera_activate(struct imx_camera_dev *pcdev) >> +{ >> + struct imxcamera_platform_data *pdata = pcdev->pdata; >> + unsigned int csicr1 = CSICR1_EN; >> + >> + dev_dbg(pcdev->dev, "Registered platform device at %p\n", >> + pcdev); >> + >> + if (pdata && pdata->init) { >> + dev_dbg(pcdev->dev, "%s: Init gpios\n", __func__); >> + pdata->init(pcdev->dev); >> + } >> + >> + if (pdata && pdata->power) { >> + dev_dbg(pcdev->dev, "%s: Power on camera\n", __func__); >> + pdata->power(pcdev->dev, 1); >> + } >> > > No, cameras should use .power and .reset from their struct > soc_camera_link, please, drop this. > > >> + >> + clk_enable(pcdev->clk); >> + >> + /* enable CSI before doing anything else */ >> + __raw_writel(csicr1, pcdev->base + CSICR1); >> + >> + csicr1 |= CSICR1_MCLKEN | CSICR1_FCC | CSICR1_GCLK_MODE; >> + csicr1 |= CSICR1_MCLKDIV(mclk_get_divisor(pcdev)); >> + csicr1 |= CSICR1_RXFF_LEVEL(2); /* 16 words */ >> + >> + __raw_writel(csicr1, pcdev->base + CSICR1); >> + >> + if (pdata && pdata->reset) { >> + dev_dbg(pcdev->dev, "%s: Releasing camera reset\n", >> + __func__); >> + pdata->reset(pcdev->dev, 1); >> > > Same here. > > >> + } >> +} >> + >> +static void imx_camera_deactivate(struct imx_camera_dev *pcdev) >> +{ >> + struct imxcamera_platform_data *board = pcdev->pdata; >> + >> + /* Disable all CSI interface */ >> + __raw_writel(0x00, pcdev->base + CSICR1); >> + >> + clk_disable(pcdev->clk); >> + >> + if (board && board->reset) { >> + dev_dbg(pcdev->dev, "%s: Asserting camera reset\n", >> + __func__); >> + board->reset(pcdev->dev, 0); >> + } >> + >> + if (board && board->power) { >> + dev_dbg(pcdev->dev, "%s: Power off camera\n", __func__); >> + board->power(pcdev->dev, 0); >> + } >> > > These two too. > > >> + >> + if (board && board->exit) { >> + dev_dbg(pcdev->dev, "%s: Release gpios\n", __func__); >> + board->exit(pcdev->dev); >> + } >> +} >> + >> +/* The following two functions absolutely depend on the fact, that >> + * there can be only one camera on i.MX camera sensor interface */ >> +static int imx_camera_add_device(struct soc_camera_device *icd) >> +{ >> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent); >> + struct imx_camera_dev *pcdev = ici->priv; >> + int ret; >> + >> + mutex_lock(&camera_lock); >> + >> + if (pcdev->icd) { >> + ret = -EBUSY; >> + goto ebusy; >> + } >> + >> + dev_info(&icd->dev, "i.MX Camera driver attached to camera %d\n", >> + icd->devnum); >> + >> + imx_camera_activate(pcdev); >> + ret = icd->ops->init(icd); >> + >> + if (!ret) >> + pcdev->icd = icd; >> + >> +ebusy: >> + mutex_unlock(&camera_lock); >> + >> + return ret; >> +} >> + >> +static void imx_camera_remove_device(struct soc_camera_device *icd) >> +{ >> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent); >> + struct imx_camera_dev *pcdev = ici->priv; >> + unsigned int csicr1; >> + >> + BUG_ON(icd != pcdev->icd); >> + >> + /* disable interrupts */ >> + csicr1 = __raw_readl(pcdev->base + CSICR1) & ~CSI_IRQ_MASK; >> + __raw_writel(csicr1, pcdev->base + CSICR1); >> + >> + /* Stop DMA engine */ >> + imx_dma_disable(pcdev->dma_chan); >> + >> + dev_info(&icd->dev, "i.MX Camera driver detached from camera %d\n", >> + icd->devnum); >> + >> + icd->ops->release(icd); >> + >> + imx_camera_deactivate(pcdev); >> + >> + pcdev->icd = NULL; >> +} >> + >> +static int imx_camera_set_bus_param(struct soc_camera_device *icd, __u32 pixfmt) >> +{ >> + struct soc_camera_host *ici = >> + to_soc_camera_host(icd->dev.parent); >> + struct imx_camera_dev *pcdev = ici->priv; >> + unsigned long camera_flags, common_flags; >> + unsigned int csicr1; >> + int ret; >> + >> + camera_flags = icd->ops->query_bus_param(icd); >> + >> + common_flags = soc_camera_bus_param_compatible(camera_flags, >> + CSI_BUS_FLAGS); >> + if (!common_flags) >> + return -EINVAL; >> + >> + if (!(common_flags & SOCAM_DATAWIDTH_8)) { >> > > I don't understand this. In your CSI_BUS_FLAGS you only support 8 bits. If > the camera doesn't support it, you get a 0 back in common_flags and return > -EINVAL above. So, this test seems redundant. > > >> + dev_warn(&icd->dev, "Camera sensor doesn't support 8-bit bus " >> + "width\n"); >> + /* set it so sensor set_bus_param could fail */ >> + common_flags |= SOCAM_DATAWIDTH_8; >> + } >> + >> + icd->buswidth = 8; >> + >> + /* Make choises, based on platform defaults */ >> + if ((common_flags & SOCAM_VSYNC_ACTIVE_HIGH) && >> + (common_flags & SOCAM_VSYNC_ACTIVE_LOW)) >> + common_flags &= ~SOCAM_VSYNC_ACTIVE_HIGH; >> + >> + if ((common_flags & SOCAM_PCLK_SAMPLE_RISING) && >> + (common_flags & SOCAM_PCLK_SAMPLE_FALLING)) >> + common_flags &= ~SOCAM_PCLK_SAMPLE_RISING; >> > > Hm, these are not "platform defaults," this is just your hard-coded > preference. > > >> + >> + ret = icd->ops->set_bus_param(icd, common_flags); >> + if (ret < 0) >> + return ret; >> + >> + csicr1 = __raw_readl(pcdev->base + CSICR1); >> + >> + if (common_flags & SOCAM_PCLK_SAMPLE_RISING) >> + csicr1 |= CSICR1_REDGE; >> + if (common_flags & SOCAM_VSYNC_ACTIVE_HIGH) >> + csicr1 |= CSICR1_SOF_POL; >> + >> + __raw_writel(csicr1, pcdev->base + CSICR1); >> + >> + return 0; >> +} >> + >> +static int imx_camera_set_fmt(struct soc_camera_device *icd, >> + __u32 pixfmt, struct v4l2_rect *rect) >> +{ >> > > You must set current_fmt here > > >> + return icd->ops->set_fmt(icd, pixfmt, rect); >> +} >> + >> +static int imx_camera_try_fmt(struct soc_camera_device *icd, >> + struct v4l2_format *f) >> +{ >> + /* TODO: limit to imx hardware capabilities */ >> + >> + /* limit to sensor capabilities */ >> + return icd->ops->try_fmt(icd, f); >> +} >> + >> +static int imx_camera_reqbufs(struct soc_camera_file *icf, >> + struct v4l2_requestbuffers *p) >> +{ >> + int i; >> + >> + /* This is for locking debugging only. I removed spinlocks and now I >> + * check whether .prepare is ever called on a linked buffer, or whether >> + * a dma IRQ can occur for an in-work or unlinked buffer. Until now >> + * it hadn't triggered */ >> + for (i = 0; i < p->count; i++) { >> + struct imx_buffer *buf = container_of(icf->vb_vidq.bufs[i], >> + struct imx_buffer, vb); >> + buf->inwork = 0; >> + INIT_LIST_HEAD(&buf->vb.queue); >> + } >> + >> + return 0; >> +} >> + >> +static unsigned int imx_camera_poll(struct file *file, poll_table *pt) >> +{ >> + struct soc_camera_file *icf = file->private_data; >> + struct imx_buffer *buf; >> + >> + buf = list_entry(icf->vb_vidq.stream.next, struct imx_buffer, >> + vb.stream); >> + >> + poll_wait(file, &buf->vb.done, pt); >> + >> + if (buf->vb.state == VIDEOBUF_DONE || >> + buf->vb.state == VIDEOBUF_ERROR) >> + return POLLIN | POLLRDNORM; >> + >> + return 0; >> +} >> + >> +static int imx_camera_querycap(struct soc_camera_host *ici, >> + struct v4l2_capability *cap) >> +{ >> + /* cap->name is set by the firendly caller:-> */ >> > > Typo: friendly > > >> + strlcpy(cap->card, imx_cam_driver_description, sizeof(cap->card)); >> + cap->version = VERSION_CODE; >> + cap->capabilities = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING; >> + >> + return 0; >> +} >> + >> +static struct soc_camera_host_ops imx_soc_camera_host_ops = { >> + .owner = THIS_MODULE, >> + .add = imx_camera_add_device, >> + .remove = imx_camera_remove_device, >> + .set_fmt = imx_camera_set_fmt, >> + .try_fmt = imx_camera_try_fmt, >> + .init_videobuf = imx_camera_init_videobuf, >> + .reqbufs = imx_camera_reqbufs, >> + .poll = imx_camera_poll, >> + .querycap = imx_camera_querycap, >> + .set_bus_param = imx_camera_set_bus_param, >> > > You are not implementing this against a current v4l tree. Please, rebase > onto, e.g., linux-next. You will have to at least implement a .set_crop > method too. > > >> +}; >> + >> +/* Should be allocated dynamically too, but we have only one. */ >> +static struct soc_camera_host imx_soc_camera_host = { >> + .drv_name = DRIVER_NAME, >> + .ops = &imx_soc_camera_host_ops, >> +}; >> + >> +static struct fiq_handler fh = { >> + .name = "csi_sof" >> +}; >> + >> +static int __init imx_camera_probe(struct platform_device *pdev) >> +{ >> + struct imx_camera_dev *pcdev; >> + struct resource *res; >> + struct pt_regs regs; >> + struct clk *clk; >> + void __iomem *base; >> + unsigned int irq; >> + int err = 0; >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + irq = platform_get_irq(pdev, 0); >> + if (!res || !irq) { >> + err = -ENODEV; >> + goto exit; >> + } >> + >> + clk = clk_get(&pdev->dev, "csi_clk"); >> > > I think, the preferred method to get a clock now is to use NULL for its > name, as long as there's only one clock attached to this device. However, > it looks like mx1 doesn't implement clkdev yet, am I right? > > >> + if (IS_ERR(clk)) { >> + err = PTR_ERR(clk); >> + goto exit; >> + } >> + >> + pcdev = kzalloc(sizeof(*pcdev), GFP_KERNEL); >> + if (!pcdev) { >> + dev_err(&pdev->dev, "Could not allocate pcdev\n"); >> + err = -ENOMEM; >> + goto exit; >> + } >> + >> + dev_set_drvdata(&pdev->dev, pcdev); >> + pcdev->res = res; >> + pcdev->clk = clk; >> + >> + pcdev->pdata = pdev->dev.platform_data; >> + >> + if (pcdev->pdata) >> + pcdev->platform_mclk_10khz = pcdev->pdata->mclk_10khz; >> + else >> + dev_warn(&pdev->dev, "No platform data provided!\n"); >> + >> + /* override if user entered mclk frequency as module param */ >> + if (mclk) >> + pcdev->platform_mclk_10khz = mclk / 10000; >> + >> + if (!pcdev->platform_mclk_10khz) { >> + dev_warn(&pdev->dev, >> + "mclk_10khz == 0! Please, fix your platform data. " >> + "Using default 20MHz\n"); >> + pcdev->platform_mclk_10khz = 2000; >> + } >> + >> + INIT_LIST_HEAD(&pcdev->capture); >> + spin_lock_init(&pcdev->lock); >> + >> + /* >> + * Request the regions. >> + */ >> + if (!request_mem_region(res->start, res->end - res->start + 1, >> + DRIVER_NAME)) { >> + err = -EBUSY; >> + goto exit_kfree; >> + } >> + >> + base = ioremap(res->start, res->end - res->start + 1); >> > > Use resource_size() > > >> + if (!base) { >> + err = -ENOMEM; >> + goto exit_release; >> + } >> + pcdev->irq = irq; >> + pcdev->base = base; >> + pcdev->dev = &pdev->dev; >> + >> + /* request dma */ >> + pcdev->dma_chan = imx_dma_request_by_prio(DRIVER_NAME, DMA_PRIO_HIGH); >> + if (pcdev->dma_chan < 0) { >> + dev_err(pcdev->dev, "Can't request DMA for i.MX CSI\n"); >> + err = -ENOMEM; >> > > ENOMEM doesn't seem to fit well here, maybe EBUSY or something? > > >> + goto exit_iounmap; >> + } >> + dev_dbg(pcdev->dev, "got DMA channel %d\n", pcdev->dma_chan); >> + >> + imx_dma_setup_handlers(pcdev->dma_chan, imx_camera_dma_irq, NULL, >> + pcdev); >> + >> + imx_dma_config_channel(pcdev->dma_chan, IMX_DMA_TYPE_FIFO, >> + IMX_DMA_MEMSIZE_32, DMA_REQ_CSI_R, 0); >> + /* burst length : 16 words = 64 bytes */ >> + imx_dma_config_burstlen(pcdev->dma_chan, 0); >> + >> + /* request irq */ >> + err = claim_fiq(&fh); >> + if (err) { >> + dev_err(pcdev->dev, "Camera interrupt register failed \n"); >> + goto exit_free_dma; >> + } >> + >> + set_fiq_handler(&imx_camera_sof_fiq_start, &imx_camera_sof_fiq_end - >> + &imx_camera_sof_fiq_start); >> + >> + regs.ARM_r8 = DMA_BASE + DMA_DIMR; >> + regs.ARM_r9 = DMA_BASE + DMA_CCR(pcdev->dma_chan); >> + regs.ARM_r10 = (long)pcdev->base + CSICR1; >> + regs.ARM_fp = (long)pcdev->base + CSISR; >> + regs.ARM_sp = 1 << pcdev->dma_chan; >> + set_fiq_regs(®s); >> + >> + mxc_set_irq_fiq(irq, 1); >> + enable_fiq(irq); >> + >> + imx_soc_camera_host.priv = pcdev; >> + imx_soc_camera_host.dev.parent = &pdev->dev; >> + imx_soc_camera_host.nr = pdev->id; >> + err = soc_camera_host_register(&imx_soc_camera_host); >> + if (err) >> + goto exit_free_irq; >> + >> + dev_info(&pdev->dev, "i.MX Camera driver loaded\n"); >> + >> + return 0; >> + >> +exit_free_irq: >> + disable_fiq(irq); >> + mxc_set_irq_fiq(irq, 0); >> + release_fiq(&fh); >> +exit_free_dma: >> + imx_dma_free(pcdev->dma_chan); >> +exit_iounmap: >> + iounmap(base); >> +exit_release: >> + release_mem_region(res->start, res->end - res->start + 1); >> +exit_kfree: >> + kfree(pcdev); >> +exit: >> + return err; >> +} >> + >> +static int __exit imx_camera_remove(struct platform_device *pdev) >> +{ >> + struct imx_camera_dev *pcdev = platform_get_drvdata(pdev); >> + struct resource *res; >> + >> + imx_dma_free(pcdev->dma_chan); >> + disable_fiq(pcdev->irq); >> + mxc_set_irq_fiq(pcdev->irq, 0); >> + release_fiq(&fh); >> + >> + soc_camera_host_unregister(&imx_soc_camera_host); >> + >> + iounmap(pcdev->base); >> + >> + res = pcdev->res; >> + release_mem_region(res->start, res->end - res->start + 1); >> + >> + kfree(pcdev); >> + >> + dev_info(&pdev->dev, "i.MX Camera driver unloaded\n"); >> + >> + return 0; >> +} >> + >> +static int imx_camera_suspend(struct platform_device *pdev, pm_message_t state) >> +{ >> + struct imx_camera_dev *pcdev = platform_get_drvdata(pdev); >> + struct imxcamera_platform_data *pdata = pcdev->pdata; >> + >> + if (pcdev->active) >> + return -EBUSY; >> + >> + if (pdata && pdata->power) { >> + dev_dbg(pcdev->dev, "%s: Power off camera\n", __func__); >> + pdata->power(pcdev->dev, 0); >> > > No pdata->power please. See pxa_camera.c or other host drivers for suspend > / resume handling and calling camera client power-management hooks. > > >> + } >> + >> + return 0; >> +} >> + >> +static int imx_camera_resume(struct platform_device *pdev) >> +{ >> + struct imx_camera_dev *pcdev = platform_get_drvdata(pdev); >> + struct imxcamera_platform_data *pdata = pcdev->pdata; >> + >> + if (pdata && pdata->power) { >> + dev_dbg(pcdev->dev, "%s: Power on camera\n", __func__); >> + pdata->power(pcdev->dev, 1); >> > > Same here > > >> + } >> + >> + return 0; >> +} >> + >> +static struct platform_driver imx_camera_driver = { >> + .driver = { >> + .name = DRIVER_NAME, >> + }, >> + .remove = __exit_p(imx_camera_remove), >> + .suspend = imx_camera_suspend, >> + .resume = imx_camera_resume, >> > > Any reason to not be using .suspend and .resume from struct > soc_camera_host_ops? > > >> +}; >> + >> +static int __init imx_camera_init(void) >> +{ >> + return platform_driver_probe(&imx_camera_driver, imx_camera_probe); >> +} >> + >> +static void __exit imx_camera_exit(void) >> +{ >> + return platform_driver_unregister(&imx_camera_driver); >> +} >> + >> +module_init(imx_camera_init); >> +module_exit(imx_camera_exit); >> + >> +MODULE_DESCRIPTION("i.MX SoC Camera Host driver"); >> +MODULE_AUTHOR("Paulius Zaleckas "); >> +MODULE_LICENSE("GPL"); >> +MODULE_ALIAS("platform:" DRIVER_NAME); >> + >> +module_param(mclk, int, 0); >> +MODULE_PARM_DESC(mclk, "MCLK value in Hz"); >> > > Hm, do you really think anyone needs mclk as a parameter? > > >> Index: linux-2.6.29/drivers/media/video/Kconfig >> =================================================================== >> --- linux-2.6.29.orig/drivers/media/video/Kconfig >> +++ linux-2.6.29/drivers/media/video/Kconfig >> @@ -929,4 +929,13 @@ config USB_S2255 >> >> endif # V4L_USB_DRIVERS >> >> +config VIDEO_IMX >> + tristate "i.MX CMOS Sensor Interface driver" >> + depends on VIDEO_DEV && ARCH_MX1 >> + select FIQ >> + select SOC_CAMERA >> + select VIDEOBUF_DMA_CONTIG >> > > It has been decided they have to depend on soc_camera, e.g., > > depends on VIDEO_DEV && SOC_CAMERA && HAS_DMA && HAVE_CLK > select VIDEOBUF_DMA_CONTIG > > >> + ---help--- >> + This is a v4l2 driver for the i.MX CMOS Sensor Interface >> + >> endif # VIDEO_CAPTURE_DRIVERS >> Index: linux-2.6.29/drivers/media/video/Makefile >> =================================================================== >> --- linux-2.6.29.orig/drivers/media/video/Makefile >> +++ linux-2.6.29/drivers/media/video/Makefile >> @@ -134,6 +134,7 @@ obj-$(CONFIG_VIDEO_CX18) += cx18/ >> obj-$(CONFIG_VIDEO_VIVI) += vivi.o >> obj-$(CONFIG_VIDEO_CX23885) += cx23885/ >> >> +obj-$(CONFIG_VIDEO_IMX) += imx_camera.o >> obj-$(CONFIG_VIDEO_PXA27x) += pxa_camera.o >> obj-$(CONFIG_VIDEO_SH_MOBILE_CEU) += sh_mobile_ceu_camera.o >> obj-$(CONFIG_VIDEO_OMAP2) += omap2cam.o >> Index: linux-2.6.29/arch/arm/mach-mx1/Makefile >> =================================================================== >> --- linux-2.6.29.orig/arch/arm/mach-mx1/Makefile >> +++ linux-2.6.29/arch/arm/mach-mx1/Makefile >> @@ -4,7 +4,7 @@ >> >> # Object file lists. >> >> -obj-y += generic.o clock.o devices.o >> +obj-y += generic.o clock.o devices.o ksym_mx1.o >> >> # Power management >> obj-$(CONFIG_PM) += pm.o >> @@ -13,5 +13,10 @@ ifeq ($(CONFIG_PM_DEBUG),y) >> CFLAGS_pm.o += -DDEBUG >> endif >> >> +# Support for CMOS sensor interface >> +ifdef CONFIG_VIDEO_IMX >> +obj-y += imx_camera_fiq.o >> +endif >> + >> # Specific board support >> obj-$(CONFIG_ARCH_MX1ADS) += mx1ads.o >> Index: linux-2.6.29/arch/arm/plat-mxc/include/mach/memory.h >> =================================================================== >> --- linux-2.6.29.orig/arch/arm/plat-mxc/include/mach/memory.h >> +++ linux-2.6.29/arch/arm/plat-mxc/include/mach/memory.h >> @@ -19,4 +19,12 @@ >> #define PHYS_OFFSET UL(0x80000000) >> #endif >> >> +#if defined(CONFIG_VIDEO_IMX) || defined(CONFIG_VIDEO_IMX_MODULE) >> +/* >> + * Increase size of DMA-consistent memory region. >> + * This is required for i.MX camera driver to capture at least four VGA frames. >> + */ >> +#define CONSISTENT_DMA_SIZE SZ_8M >> +#endif /* CONFIG_VIDEO_IMX */ >> + >> #endif /* __ASM_ARCH_MXC_MEMORY_H__ */ >> Index: linux-2.6.29/arch/arm/plat-mxc/include/mach/camera.h >> =================================================================== >> --- /dev/null >> +++ linux-2.6.29/arch/arm/plat-mxc/include/mach/camera.h >> > > No, you have to select a more specific name for it. There is already an > mx3_camera.h there. > > >> @@ -0,0 +1,27 @@ >> +/* >> + * camera.h - i.MX camera driver header file >> + * >> + * Copyright (c) 2008, Paulius Zaleckas >> + * >> + * Based on PXA camera.h file: >> + * Copyright (C) 2003, Intel Corporation >> + * Copyright (C) 2008, Guennadi Liakhovetski >> + * >> + * This file is released under the GPLv2 >> + */ >> + >> +#ifndef __ASM_ARCH_CAMERA_H_ >> +#define __ASM_ARCH_CAMERA_H_ >> + >> +extern unsigned char imx_camera_sof_fiq_start, imx_camera_sof_fiq_end; >> + >> +struct imxcamera_platform_data { >> + int (*init)(struct device *); >> + int (*exit)(struct device *); >> + int (*power)(struct device *, int); >> + int (*reset)(struct device *, int); >> > > Just remove power and reset here. Remember, you might have more than one > camera attached, even if only one of them can be active at any given time. > > >> + >> + unsigned long mclk_10khz; >> +}; >> + >> +#endif /* __ASM_ARCH_CAMERA_H_ */ >> Index: linux-2.6.29/arch/arm/mach-mx1/imx_camera_fiq.S >> =================================================================== >> --- /dev/null >> +++ linux-2.6.29/arch/arm/mach-mx1/imx_camera_fiq.S >> @@ -0,0 +1,35 @@ >> +/* >> + * Copyright (C) 2008 Paulius Zaleckas >> + * >> + * Based on linux/arch/arm/lib/floppydma.S >> + * Copyright (C) 1995, 1996 Russell King >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> > > Here you have GPL v2 only > > >> + */ >> +#include >> +#include >> + >> + .text >> + .global imx_camera_sof_fiq_end >> + .global imx_camera_sof_fiq_start >> +imx_camera_sof_fiq_start: >> + @ enable dma >> + ldr r12, [r9] >> + orr r12, r12, #0x00000001 >> + str r12, [r9] >> + @ unmask DMA interrupt >> + ldr r12, [r8] >> + bic r12, r12, r13 >> + str r12, [r8] >> + @ disable SOF interrupt >> + ldr r12, [r10] >> + bic r12, r12, #0x00010000 >> + str r12, [r10] >> + @ clear SOF flag >> + mov r12, #0x00010000 >> + str r12, [r11] >> + @ return from FIQ >> + subs pc, lr, #4 >> +imx_camera_sof_fiq_end: >> Index: linux-2.6.29/arch/arm/mach-mx1/ksym_mx1.c >> =================================================================== >> --- /dev/null >> +++ linux-2.6.29/arch/arm/mach-mx1/ksym_mx1.c >> @@ -0,0 +1,23 @@ >> +/* >> + * Exported ksyms of ARCH_MX1 >> + * >> + * Copyright (C) 2008, Darius Augulis >> + * >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; either version 2 of the License, or >> + * (at your option) any later version. >> > > Here >= 2 again > > >> + */ >> + >> +#include >> +#include >> + >> + >> +#if defined(CONFIG_VIDEO_IMX) || defined(CONFIG_VIDEO_IMX_MODULE) >> +#include >> + >> +/* IMX camera FIQ handler */ >> +EXPORT_SYMBOL(imx_camera_sof_fiq_start); >> +EXPORT_SYMBOL(imx_camera_sof_fiq_end); >> +#endif >> > > Thanks > Guennadi > --- > Guennadi Liakhovetski > >