Re: [PATCH 1/5] CSI camera interface driver for MX1

Re: [PATCH 1/5] CSI camera interface driver for MX1

[Guennadi Liakhovetski]

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@teltonika.lt>

Driver for i.MX1/L camera (CSI) host.

Signed-off-by: Paulius Zaleckas <paulius.zaleckas@teltonika.lt>

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@teltonika.lt>
> + *
> + * Based on PXA SoC camera driver
> + * Copyright (C) 2006, Sascha Hauer, Pengutronix
> + * Copyright (C) 2008, Guennadi Liakhovetski <kernel@pengutronix.de>
> + *
> + * 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(&regs);
> +
> +	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@teltonika.lt>");
> +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@teltonika.lt>
> + *
> + * Based on PXA camera.h file:
> + * Copyright (C) 2003, Intel Corporation
> + * Copyright (C) 2008, Guennadi Liakhovetski <kernel@pengutronix.de>
> + *
> + * 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@teltonika.lt>
> + *
> + *  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@gmail.com>
> + *
> + *
> + * 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

Re: [PATCH 1/5] CSI camera interface driver for MX1

[Darius Augulis]

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 <paulius.zaleckas@teltonika.lt>
>
> Driver for i.MX1/L camera (CSI) host.
>
> Signed-off-by: Paulius Zaleckas <paulius.zaleckas@teltonika.lt>
>
> 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 <paulius.zaleckas@teltonika.lt>
>> + *
>> + * Based on PXA SoC camera driver
>> + * Copyright (C) 2006, Sascha Hauer, Pengutronix
>> + * Copyright (C) 2008, Guennadi Liakhovetski <kernel@pengutronix.de>
>> + *
>> + * 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(&regs);
>> +
>> +	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@teltonika.lt>");
>> +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@teltonika.lt>
>> + *
>> + * Based on PXA camera.h file:
>> + * Copyright (C) 2003, Intel Corporation
>> + * Copyright (C) 2008, Guennadi Liakhovetski <kernel@pengutronix.de>
>> + *
>> + * 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@teltonika.lt>
>> + *
>> + *  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@gmail.com>
>> + *
>> + *
>> + * 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
>
>   

Re: [PATCH 1/5] CSI camera interface driver for MX1

[Mauro Carvalho Chehab]

Hi Darius,

On Thu, 26 Mar 2009 21:19:24 +0200
Darius Augulis <augulis.darius@gmail.com> wrote:

> 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.

Please always base your patches against the last v4l-dvb tree or linux-next.
This is specially important those days, where v4l core is suffering several
changes.
> 
> > From: Paulius Zaleckas <paulius.zaleckas@teltonika.lt>
> >
> > Driver for i.MX1/L camera (CSI) host.
> >
> > Signed-off-by: Paulius Zaleckas <paulius.zaleckas@teltonika.lt>
> >
> > 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!

Still, it is better to send this via v4l-dvb patch, to avoid merge conflicts
and breakages due to API differences.
> >> +/* 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.

Please always check your patches with checkpatch.pl. This will point such issues.

> >> +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));

Hmm... why do you need such warning?

> >> +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

I haven't seen where you are defining DEBUG. if those debug stuff are needed
only during development, it is better to remove it, to avoid polluting upstream
with useless code.

> >> +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));

Why do you need a warning here?

> >> + * Copyright (C) 2008, Darius Augulis <augulis.darius@gmail.com>
> >> + *
> >> + *
> >> + * 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

About the licensing, you can use both GPLv2 only or GPLv2 or later. It is
better to use the same for all drivers.

Cheers,
Mauro

Re: [PATCH 1/5] CSI camera interface driver for MX1

[Guennadi Liakhovetski]

On Thu, 26 Mar 2009, Mauro Carvalho Chehab wrote:

> > >> +	/* 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.
> 
> Please always check your patches with checkpatch.pl. This will point such issues.

No, I did check the patch with checkpatch.pl and it didn't complain about 
this. It checks _indentation_ of lines, that _must_ be done with TABs, but 
it doesn't check what is used for alignment _inside_ lines, like

	xxx     = 0;
	y	= 1;
	zzzzz   = 2;

where first and third lines have spaces before "=", and the second one has 
a TAB. This is not checked by checkpatch.pl.

Thanks
Guennadi
---
Guennadi Liakhovetski

Re: [PATCH 1/5] CSI camera interface driver for MX1

[Dave Strauss]

Guennadi Liakhovetski wrote:
> On Thu, 26 Mar 2009, Mauro Carvalho Chehab wrote:
> 
>>>>> +	/* 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.
>> Please always check your patches with checkpatch.pl. This will point such issues.
> 
> No, I did check the patch with checkpatch.pl and it didn't complain about 
> this. It checks _indentation_ of lines, that _must_ be done with TABs, but 
> it doesn't check what is used for alignment _inside_ lines, like
> 
> 	xxx     = 0;
> 	y	= 1;
> 	zzzzz   = 2;
> 
> where first and third lines have spaces before "=", and the second one has 
> a TAB. This is not checked by checkpatch.pl.
> 
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski
> 

Newbie question -- where does one find checkpatch.pl?  And are there any other
tools we should be running on patches before we submit them?

Thanks.

  - Dave Strauss

Re: [PATCH 1/5] CSI camera interface driver for MX1

[Mauro Carvalho Chehab]

On Thu, 26 Mar 2009 17:31:35 -0400
Dave Strauss <Dave.Strauss@zoran.com> wrote:

> Guennadi Liakhovetski wrote:
> > On Thu, 26 Mar 2009, Mauro Carvalho Chehab wrote:
> > 
> >>>>> +	/* 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.
> >> Please always check your patches with checkpatch.pl. This will point such issues.
> > 
> > No, I did check the patch with checkpatch.pl and it didn't complain about 
> > this. It checks _indentation_ of lines, that _must_ be done with TABs, but 
> > it doesn't check what is used for alignment _inside_ lines, like
> > 
> > 	xxx     = 0;
> > 	y	= 1;
> > 	zzzzz   = 2;
> > 
> > where first and third lines have spaces before "=", and the second one has 
> > a TAB. This is not checked by checkpatch.pl.
> > 
> > Thanks
> > Guennadi
> > ---
> > Guennadi Liakhovetski
> > 
> 
> Newbie question -- where does one find checkpatch.pl?  And are there any other
> tools we should be running on patches before we submit them?


It is at kernel tree, under script directory. You shouldn't copy it from kernel
tree, but to use it from there, to get optimum results, since it will read some
files a kernel documentation, for checking the usage of legacy functions.
> 
> Thanks.
> 
>   - Dave Strauss




Cheers,
Mauro

Re: [PATCH 1/5] CSI camera interface driver for MX1

[Russell King - ARM Linux]

On Thu, Mar 26, 2009 at 05:31:35PM -0400, Dave Strauss wrote:
> Newbie question -- where does one find checkpatch.pl?  And are there any other
> tools we should be running on patches before we submit them?

scripts/checkpatch.pl in the kernel source.

Sparse is another tool which can be used while building the kernel to
increase the build time checking, but it can be quite noisy, so please
only look at stuff which pops up for your specific area.

Also, avoid using __force to shut up sparse warnings - sparse's address
space separation via use of __user and __iomem tags are supposed to _only_
be casted away by the final level of code (iow, the bits which really
do the accesses.)  It's preferred to leave the warnings in place rather
than silence them.

In other words:

static int blah(void __iomem *ptr)
{
	void *foo = ptr;
	...
}

will generate a sparse warning.  The right solution to this is:

static int blah(void __iomem *ptr)
{
	void __iomem *foo = ptr;
	...
}

but if that's not possible for whatever reason, leave it as is and
definitely do *not* silence it like this:

static int blah(void __iomem *ptr)
{
	void *foo = (void __force *)ptr;
	...
}

The point of __iomem is to allow static checking that pointers for MMIO
aren't dereferenced without using the correct accessors, and adding
such __force casts negates the purpose of sparse.

Re: [PATCH 1/5] CSI camera interface driver for MX1

[Holger Schurig]

> Sparse is another tool which can be used while building the
> kernel to increase the build time checking, but it can be
> quite noisy, so please only look at stuff which pops up for
> your specific area.

To get rid of some of the warnings, you can analyze only the 
parts of the source that you're working on. You just need sparse 
in your PATH and issue:

$ make SUBDIRS=arch/arm/mach-mx2 C=2


Unfortunately, the arm tree is a bit different to mainline linux, 
because

$ make SUBDIRS=arch/arm C=2
arch/arm/Makefile:31: *** Recursive variable `KBUILD_CFLAGS' 
references itself (eventually).  Stop.
make: *** [_module_arch/arm] Error 2

breaks, with and without sparse:

Re: [PATCH 1/5] CSI camera interface driver for MX1

[Russell King - ARM Linux]

On Fri, Mar 27, 2009 at 08:33:27AM +0100, Holger Schurig wrote:
> > Sparse is another tool which can be used while building the
> > kernel to increase the build time checking, but it can be
> > quite noisy, so please only look at stuff which pops up for
> > your specific area.
> 
> To get rid of some of the warnings, you can analyze only the 
> parts of the source that you're working on. You just need sparse 
> in your PATH and issue:
> 
> $ make SUBDIRS=arch/arm/mach-mx2 C=2
> 
> 
> Unfortunately, the arm tree is a bit different to mainline linux, 
> because
> 
> $ make SUBDIRS=arch/arm C=2
> arch/arm/Makefile:31: *** Recursive variable `KBUILD_CFLAGS' 
> references itself (eventually).  Stop.
> make: *** [_module_arch/arm] Error 2

Line 31 is the KBUILD_CFLAGS line of:

ifeq ($(CONFIG_FRAME_POINTER),y)
KBUILD_CFLAGS   +=-fno-omit-frame-pointer -mapcs -mno-sched-prolog
endif

which is _not_ a recursive definition.  Either your make is broken or
you have local changes to that line.

Re: [PATCH 1/5] CSI camera interface driver for MX1

[Darius Augulis]

Mauro Carvalho Chehab wrote:
> Hi Darius,
> 
> Please always base your patches against the last v4l-dvb tree or linux-next.
> This is specially important those days, where v4l core is suffering several
> changes.

Hi,

could you please advice which v4l-dvb Git repository I should pull from?
Because git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/devel.git does not have any new stuff now.
git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-next.git is marked as "unstable" for testing purposes.
What is better to base my patches on?

Darius.

Re: [PATCH 1/5] CSI camera interface driver for MX1

[Mauro Carvalho Chehab]

On Fri, 27 Mar 2009 11:37:23 +0200
Darius Augulis <augulis.darius@gmail.com> wrote:

> Mauro Carvalho Chehab wrote:
> > Hi Darius,
> > 
> > Please always base your patches against the last v4l-dvb tree or linux-next.
> > This is specially important those days, where v4l core is suffering several
> > changes.
> 
> Hi,
> 
> could you please advice which v4l-dvb Git repository I should pull from?
> Because git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/devel.git does not have any new stuff now.

During the merge window, all patches that are ready for submission are moved to:

	git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-2.6.git

> git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-next.git is marked as "unstable" for testing purposes.
> What is better to base my patches on?
> 
> Darius.




Cheers,
Mauro

Re: [PATCH 1/5] CSI camera interface driver for MX1

[Mauro Carvalho Chehab]

On Fri, 27 Mar 2009 11:37:23 +0200
Darius Augulis <augulis.darius@gmail.com> wrote:

> Mauro Carvalho Chehab wrote:
> > Hi Darius,
> > 
> > Please always base your patches against the last v4l-dvb tree or linux-next.
> > This is specially important those days, where v4l core is suffering several
> > changes.
>

Btw, you shouldn't be c/c a list that requires subscription. Every time I send
something, I got such errors:

From: linux-arm-kernel-bounces@lists.arm.linux.org.uk
To: mchehab@infradead.org
Subject: Your message to Linux-arm-kernel awaits moderator approval
Date: Fri, 27 Mar 2009 10:34:51 +0000
Sender: linux-arm-kernel-bounces@lists.arm.linux.org.uk

Your mail to 'Linux-arm-kernel' with the subject

    Re: [PATCH 1/5] CSI camera interface driver for MX1

Is being held until the list moderator can review it for approval.

The reason it is being held:

    Post by non-member to a members-only list

Either the message will get posted to the list, or you will receive
notification of the moderator's decision.  If you would like to cancel
this posting, please visit the following URL:

    http://lists.arm.linux.org.uk/mailman/confirm/linux-arm-kernel/f04eda1d528be13a0d486acf4663a17ca96b05bd


Cheers,
Mauro

Re: [PATCH 1/5] CSI camera interface driver for MX1

[Darius Augulis]

Mauro Carvalho Chehab wrote:
 > On Fri, 27 Mar 2009 11:37:23 +0200
 > Darius Augulis <augulis.darius@gmail.com> wrote:
 >
 >> Mauro Carvalho Chehab wrote:
 >>> Hi Darius,
 >>>
 >>> Please always base your patches against the last v4l-dvb tree or linux-next.
 >>> This is specially important those days, where v4l core is suffering several
 >>> changes.
 >
 > Btw, you shouldn't be c/c a list that requires subscription. Every time I send
 > something, I got such errors:

I sent it to ARM Linux ML, because it has lot of ARM stuff and there are people who maintain ARM/MXC.
You probably could remove some CC from your reply message?

Re: [PATCH 1/5] CSI camera interface driver for MX1

[Darius Augulis]

Hi Guennadi,

>> +/*
>> + *  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...

I'm not sure about this one. Should I leave this unchanged?


>> +	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?

yes, FIQ is necessary. Because IRQ is to slow. When SoF is detected, DMA must be activated immediately, but not to early.
Whe tried to use IRQ, and many starts of frames were missed.
May I remove these spin_lock_irqsave's?

> 
>> +
>> +	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?

Yes, probably it's only 'small' error in datasheet :)


>> +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

is it ok?


>> +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.

yes, this is uneeded. I removed this already.


>> +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.

could you please explain shortly what exactly should be implemented in .set_crop and .get_formats
methods?


>> +	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?

in case mx1 doesn't implement clkdev yet, I will leave clock name there, ok?
We could remove it in the future.


> 
> Any reason to not be using .suspend and .resume from struct 
> soc_camera_host_ops?

no reason, moved already.
But I don't know is there anything to do in suspend and resume functions?
Methods .reset and .power are removed from pdata.


>> +module_param(mclk, int, 0);
>> +MODULE_PARM_DESC(mclk, "MCLK value in Hz");
> 
> Hm, do you really think anyone needs mclk as a parameter?

We needed it before, but not anymore since yet.
Micron and Omnivision sensors usually need different MCLK.
During development time it was not very convenient to re-build kernel with different MCLK's in pdata.
Therefore Paulius added this module param.

Darius A.

Re: [PATCH 1/5] CSI camera interface driver for MX1

[Russell King - ARM Linux]

On Fri, Mar 27, 2009 at 05:18:18PM +0200, Darius Augulis wrote:
> > 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?

This is precisely the area where FIQs can't be used.  You can't take
spinlocks (even IRQ-safe spinlocks) from FIQs.  Why?  You'll deadlock.

Consider:

	spin_lock_irqsave(lock, flags);
	...
FIQ happens
FIQ:	spin_lock_irqsave(lock, flags); <=== deadlock

And there's nothing you can do about that.  And no, converting this
locking primitive to also disable FIQs means that then FIQs will be
subject to the same latency as normal IRQs.

In fact, in uniprocessor mode, you might as well completely forget the
spinlock, because the lock part is a no-op, and the IRQ disable has no
effect on FIQs.

If you're going to be using FIQs in C code, you need to be _very_ _very_
careful about what you do.  Calling normal kernel functions is generally
not going to be safe in any way.

Re: [PATCH 1/5] CSI camera interface driver for MX1

[Guennadi Liakhovetski]

On Fri, 27 Mar 2009, Russell King - ARM Linux wrote:

> On Fri, Mar 27, 2009 at 05:18:18PM +0200, Darius Augulis wrote:
> > > 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?
> 
> This is precisely the area where FIQs can't be used.  You can't take
> spinlocks (even IRQ-safe spinlocks) from FIQs.  Why?  You'll deadlock.
> 
> Consider:
> 
> 	spin_lock_irqsave(lock, flags);
> 	...
> FIQ happens
> FIQ:	spin_lock_irqsave(lock, flags); <=== deadlock
> 
> And there's nothing you can do about that.  And no, converting this
> locking primitive to also disable FIQs means that then FIQs will be
> subject to the same latency as normal IRQs.
> 
> In fact, in uniprocessor mode, you might as well completely forget the
> spinlock, because the lock part is a no-op, and the IRQ disable has no
> effect on FIQs.
> 
> If you're going to be using FIQs in C code, you need to be _very_ _very_
> careful about what you do.  Calling normal kernel functions is generally
> not going to be safe in any way.

No, they are not calling C from FIQs and they are not protecting against 
FIQs with a spinlock_irq*, that was my misinterpretation, sorry.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: [PATCH 1/5] CSI camera interface driver for MX1

[Mauro Carvalho Chehab]

On Fri, 27 Mar 2009 15:39:59 +0200
Darius Augulis <augulis.darius@gmail.com> wrote:

> Mauro Carvalho Chehab wrote:
>  > On Fri, 27 Mar 2009 11:37:23 +0200
>  > Darius Augulis <augulis.darius@gmail.com> wrote:
>  >
>  >> Mauro Carvalho Chehab wrote:
>  >>> Hi Darius,
>  >>>
>  >>> Please always base your patches against the last v4l-dvb tree or linux-next.
>  >>> This is specially important those days, where v4l core is suffering several
>  >>> changes.
>  >
>  > Btw, you shouldn't be c/c a list that requires subscription. Every time I send
>  > something, I got such errors:
> 
> I sent it to ARM Linux ML, because it has lot of ARM stuff and there are people who maintain ARM/MXC.
> You probably could remove some CC from your reply message?

If the subject is important to ARM people, the reply messages should be there
as well. Otherwise you shouldn't c/c it since the beginning ;)

Subscribers only list are not good for patches discussion, and aren't
recommended by Linux practices. 

The issues become evident on such discussions where more than one
subsystem is envolved. 

We've switched this year to linux-media@vger.kernel.org mainly due to that: the
anti-spam filters at VGER are so efficient that we don't need to be
subscribers-only anymore. I suggest that you try to argue with ARM list
maintainer to do the same. 

At the mean time, please c/c only lists that don't require subscriptions, since
people shouldn't be forced to subscribe just to reply an email, and it is not
polite to send emails refusing their comments.

Cheers,
Mauro