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,
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.
From: Paulius Zaleckas <paulius.zaleckas@xxxxxxxxxxxx>
Driver for i.MX1/L camera (CSI) host.
Signed-off-by: Paulius Zaleckas <paulius.zaleckas@xxxxxxxxxxxx>
You are forwarding his patch, so, you have to sign-off under it. Why isn't
he submitting it himself?
---
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 <paulius.zaleckas@xxxxxxxxxxxx>
> + *
> + * Based on PXA SoC camera driver
> + * Copyright (C) 2006, Sascha Hauer, Pengutronix
> + * Copyright (C) 2008, Guennadi Liakhovetski <kernel@xxxxxxxxxxxxxx>
> + *
> + * 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 <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/io.h>
> +#include <linux/delay.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/errno.h>
> +#include <linux/fs.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/mm.h>
> +#include <linux/moduleparam.h>
> +#include <linux/time.h>
> +#include <linux/version.h>
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +#include <linux/mutex.h>
> +#include <linux/clk.h>
> +
> +#include <media/v4l2-common.h>
> +#include <media/v4l2-dev.h>
> +#include <media/videobuf-dma-contig.h>
> +#include <media/soc_camera.h>
> +
> +#include <linux/videodev2.h>
> +
> +#include <asm/dma.h>
> +#include <asm/fiq.h>
> +#include <mach/hardware.h>
> +#include <mach/dma-mx1-mx2.h>
> +#include <mach/camera.h>
It makes patching a bit easier, if includes are alphabetically ordered.
So, if in the future you get two patches each adding one <linux/...>
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. <SoF FIQ> enable DMA, DMA IRQ, disable SoF FIQ
3. <DMA done IRQ> 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 <paulius.zaleckas@xxxxxxxxxxxx>");
> +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 <paulius.zaleckas@xxxxxxxxxxxx>
> + *
> + * Based on PXA camera.h file:
> + * Copyright (C) 2003, Intel Corporation
> + * Copyright (C) 2008, Guennadi Liakhovetski <kernel@xxxxxxxxxxxxxx>
> + *
> + * 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 <paulius.zaleckas@xxxxxxxxxxxx>
> + *
> + * 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 <linux/linkage.h>
> +#include <asm/assembler.h>
> +
> + .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 <augulis.darius@xxxxxxxxx>
> + *
> + *
> + * 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 <linux/platform_device.h>
> +#include <linux/module.h>
> +
> +
> +#if defined(CONFIG_VIDEO_IMX) || defined(CONFIG_VIDEO_IMX_MODULE)
> +#include <mach/camera.h>
> +
> +/* IMX camera FIQ handler */
> +EXPORT_SYMBOL(imx_camera_sof_fiq_start);
> +EXPORT_SYMBOL(imx_camera_sof_fiq_end);
> +#endif
Thanks
Guennadi
---
Guennadi Liakhovetski
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
