Subject: [PATCH 1/3] mx2_camera: Add soc_camera support for i.MX25/i.MX27

From: [email-masked] (Sascha Hauer)
Date: Mon, 17 May 2010 09:27:20 +0200

On Wed, May 12, 2010 at 09:02:29PM +0200, Guennadi Liakhovetski wrote:
> Hi Baruch
> 
> Thanks for eventually mainlining this driver! A couple of comments below. 
> Sascha, would be great, if you could get it tested on imx27 with and 
> without emma.

I will see what I can do. Testing and probably breathing life into a
camera driver usually takes me two days given that the platform support
is very outdated. I hope our customer is interested in this, then it
would be possible to test it.

> BTW, if you say, that you use emma to avoid using the 
> standard DMA controller, why would anyone want not to use emma? Resource 
> conflict? There is also a question for you down in the comments, please, 
> skim over.

I originally did not know how all the components should work together.
Now I think it's the right way to use the EMMA to be able to scale
images and to do colour conversions (which does not work with our Bayer
format cameras, so I cannot test it).

> 
> On Thu, 6 May 2010, Baruch Siach wrote:
> 
> > This is the soc_camera support developed by Sascha Hauer for the i.MX27.  Alan
> > Carvalho de Assis modified the original driver to get it working on more recent
> > kernels. I modified it further to add support for i.MX25. This driver has only
> > been tested on the i.MX25 platform.
> > 
> > Signed-off-by: Baruch Siach <[email-masked]>
> > ---
> >  arch/arm/plat-mxc/include/mach/memory.h  |    4 +-
> >  arch/arm/plat-mxc/include/mach/mx2_cam.h |   41 +
> >  drivers/media/video/Kconfig              |   14 +
> >  drivers/media/video/Makefile             |    1 +
> >  drivers/media/video/mx2_camera.c         | 1396 ++++++++++++++++++++++++++++++
> >  5 files changed, 1454 insertions(+), 2 deletions(-)
> >  create mode 100644 arch/arm/plat-mxc/include/mach/mx2_cam.h
> >  create mode 100644 drivers/media/video/mx2_camera.c
> > 
> > diff --git a/arch/arm/plat-mxc/include/mach/memory.h b/arch/arm/plat-mxc/include/mach/memory.h
> > index c4b40c3..5803836 100644
> > --- a/arch/arm/plat-mxc/include/mach/memory.h
> > +++ b/arch/arm/plat-mxc/include/mach/memory.h
> > @@ -44,12 +44,12 @@
> >   */
> >  #define CONSISTENT_DMA_SIZE SZ_8M
> >  
> > -#elif defined(CONFIG_MX1_VIDEO)
> > +#elif defined(CONFIG_MX1_VIDEO) || defined(CONFIG_MX2_VIDEO)
> >  /*
> >   * 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_4M
> > -#endif /* CONFIG_MX1_VIDEO */
> > +#endif /* CONFIG_MX1_VIDEO || CONFIG_MX2_VIDEO */
> >  
> >  #endif /* __ASM_ARCH_MXC_MEMORY_H__ */
> > diff --git a/arch/arm/plat-mxc/include/mach/mx2_cam.h b/arch/arm/plat-mxc/include/mach/mx2_cam.h
> > new file mode 100644
> > index 0000000..01b3bdc
> > --- /dev/null
> > +++ b/arch/arm/plat-mxc/include/mach/mx2_cam.h
> > @@ -0,0 +1,41 @@
> > +/*
> > +    mx2-cam.h - i.MX27/i.MX25 camera driver header file
> > +
> > +    Copyright (C) 2003, Intel Corporation
> > +    Copyright (C) 2008, Sascha Hauer <[email-masked]>
> > +    Copyright (C) 2010, Baruch Siach <[email-masked]>
> > +
> > +    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.
> > +
> > +    This program is distributed in the hope that it will be useful,
> > +    but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +    GNU General Public License for more details.
> > +
> > +    You should have received a copy of the GNU General Public License
> > +    along with this program; if not, write to the Free Software
> > +    Foundation, Inc., 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
> > +*/
> 
> Please, follow the kernel multiline comment style:
> 
> /*
>  * this is a
>  * multiline comment
>  */
> 
> > +
> > +#ifndef __MACH_MX2_CAM_H_
> > +#define __MACH_MX2_CAM_H_
> > +
> > +#define MX2_CAMERA_SWAP16		(1 << 0)
> > +#define MX2_CAMERA_EXT_VSYNC		(1 << 1)
> > +#define MX2_CAMERA_CCIR			(1 << 2)
> > +#define MX2_CAMERA_CCIR_INTERLACE	(1 << 3)
> > +#define MX2_CAMERA_HSYNC_HIGH		(1 << 4)
> > +#define MX2_CAMERA_GATED_CLOCK		(1 << 5)
> > +#define MX2_CAMERA_INV_DATA		(1 << 6)
> > +#define MX2_CAMERA_PCLK_SAMPLE_RISING	(1 << 7)
> > +#define MX2_CAMERA_PACK_DIR_MSB		(1 << 8)
> > +
> > +struct mx2_camera_platform_data {
> > +	unsigned long flags;
> > +	unsigned long clk;
> > +};
> 
> Since this is an exported API, please, document this struct, at least 
> "unsigned long clk" is not self-explanatory, IMHO.
> 
> > +
> > +#endif /* __MACH_MX2_CAM_H_ */
> > diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
> > index f8fc865..4e230b8 100644
> > --- a/drivers/media/video/Kconfig
> > +++ b/drivers/media/video/Kconfig
> > @@ -949,6 +949,20 @@ config VIDEO_OMAP2
> >  	---help---
> >  	  This is a v4l2 driver for the TI OMAP2 camera capture interface
> >  
> > +config MX2_VIDEO
> > +        bool
> > +
> > +config VIDEO_MX2
> > +	tristate "i.MX27/i.MX25 Camera Sensor Interface driver"
> > +	depends on VIDEO_DEV && (MACH_MX27 || ARCH_MX25)
> > +	select SOC_CAMERA
> 
> Please follow other drivers and add SOC_CAMERA to "depends on"
> 
> > +	select VIDEOBUF_DMA_CONTIG
> > +	select MX2_VIDEO
> > +	---help---
> > +	  This is a v4l2 driver for the i.MX27 and the i.MX25 Camera Sensor
> > +	  Interface
> > +
> > +
> >  #
> >  # USB Multimedia device configuration
> >  #
> > diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
> > index b88b617..177af1a 100644
> > --- a/drivers/media/video/Makefile
> > +++ b/drivers/media/video/Makefile
> > @@ -156,6 +156,7 @@ obj-$(CONFIG_SOC_CAMERA)		+= soc_camera.o soc_mediabus.o
> >  obj-$(CONFIG_SOC_CAMERA_PLATFORM)	+= soc_camera_platform.o
> >  # soc-camera host drivers have to be linked after camera drivers
> >  obj-$(CONFIG_VIDEO_MX1)			+= mx1_camera.o
> > +obj-$(CONFIG_VIDEO_MX2)			+= mx2_camera.o
> >  obj-$(CONFIG_VIDEO_MX3)			+= mx3_camera.o
> >  obj-$(CONFIG_VIDEO_PXA27x)		+= pxa_camera.o
> >  obj-$(CONFIG_VIDEO_SH_MOBILE_CEU)	+= sh_mobile_ceu_camera.o
> > diff --git a/drivers/media/video/mx2_camera.c b/drivers/media/video/mx2_camera.c
> > new file mode 100644
> > index 0000000..5d6fb08
> > --- /dev/null
> > +++ b/drivers/media/video/mx2_camera.c
> > @@ -0,0 +1,1396 @@
> > +/*
> > + * V4L2 Driver for i.MX27/i.MX25 camera host
> > + *
> > + * Copyright (C) 2008, Sascha Hauer, Pengutronix
> 
> Baruch, depending on how you estimate your effort on porting Sascha's 
> driver, you might want to add your copyright here too.
> 
> > + *
> > + * 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.
> > + */
> > +
> > +#include <linux/init.h>
> > +#include <linux/module.h>
> > +#include <linux/io.h>
> > +#include <linux/delay.h>
> > +#include <linux/slab.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 <media/soc_mediabus.h>
> > +
> > +#include <linux/videodev2.h>
> > +
> > +#include <mach/mx2_cam.h>
> > +#include <asm/dma.h>
> 
> This is duplicated below, please, remove this copy.
> 
> > +#include <mach/dma-mx1-mx2.h>
> > +#include <mach/hardware.h>
> > +
> > +#include <asm/dma.h>
> > +
> > +#define MX2_CAM_DRV_NAME "mx2-camera"
> > +#define MX2_CAM_VERSION_CODE KERNEL_VERSION(0, 0, 5) /* FIXME: Whats this? */
> 
> Well, I think, you can remove this "FIXME"... This is just some arbitrary 
> driver version, that noone ever remembers to update;) or am I wrong?
> 
> > +#define MX2_CAM_DRIVER_DESCRIPTION "i.MX2x_Camera"
> > +
> > +/* reset values */
> > +#define CSICR1_RESET_VAL	0x40000800
> > +#define CSICR2_RESET_VAL	0x0
> > +#define CSICR3_RESET_VAL	0x0
> > +
> > +/* csi control reg 1 */
> > +#define CSICR1_SWAP16_EN	(1 << 31)
> > +#define CSICR1_EXT_VSYNC	(1 << 30)
> > +#define CSICR1_EOF_INTEN	(1 << 29)
> > +#define CSICR1_PRP_IF_EN	(1 << 28)
> > +#define CSICR1_CCIR_MODE	(1 << 27)
> > +#define CSICR1_COF_INTEN	(1 << 26)
> > +#define CSICR1_SF_OR_INTEN	(1 << 25)
> > +#define CSICR1_RF_OR_INTEN	(1 << 24)
> > +#define CSICR1_STATFF_LEVEL	(3 << 22)
> > +#define CSICR1_STATFF_INTEN	(1 << 21)
> > +#define CSICR1_RXFF_LEVEL(l)	(((l) & 3) << 19)	/* MX27 */
> > +#define CSICR1_FB2_DMA_INTEN	(1 << 20)		/* MX25 */
> > +#define CSICR1_FB1_DMA_INTEN	(1 << 19)		/* MX25 */
> > +#define CSICR1_RXFF_INTEN	(1 << 18)
> > +#define CSICR1_SOF_POL		(1 << 17)
> > +#define CSICR1_SOF_INTEN	(1 << 16)
> > +#define CSICR1_MCLKDIV(d)	(((d) & 0xF) << 12)
> > +#define CSICR1_HSYNC_POL	(1 << 11)
> > +#define CSICR1_CCIR_EN		(1 << 10)
> > +#define CSICR1_MCLKEN		(1 << 9)
> > +#define CSICR1_FCC		(1 << 8)
> > +#define CSICR1_PACK_DIR		(1 << 7)
> > +#define CSICR1_CLR_STATFIFO	(1 << 6)
> > +#define CSICR1_CLR_RXFIFO	(1 << 5)
> > +#define CSICR1_GCLK_MODE	(1 << 4)
> > +#define CSICR1_INV_DATA		(1 << 3)
> > +#define CSICR1_INV_PCLK		(1 << 2)
> > +#define CSICR1_REDGE		(1 << 1)
> > +
> > +#define SHIFT_STATFF_LEVEL	22
> > +#define SHIFT_RXFF_LEVEL	19
> > +#define SHIFT_MCLKDIV		12
> > +
> > +/* control reg 3 */
> > +#define CSICR3_FRMCNT		(0xFFFF << 16)
> > +#define CSICR3_FRMCNT_RST	(1 << 15)
> > +#define CSICR3_DMA_REFLASH_RFF	(1 << 14)
> > +#define CSICR3_DMA_REFLASH_SFF	(1 << 13)
> > +#define CSICR3_DMA_REQ_EN_RFF	(1 << 12)
> > +#define CSICR3_DMA_REQ_EN_SFF	(1 << 11)
> > +#define CSICR3_RXFF_LEVEL(l)	(((l) & 7) << 4)	/* MX25 */
> > +#define CSICR3_CSI_SUP		(1 << 3)
> > +#define CSICR3_ZERO_PACK_EN	(1 << 2)
> > +#define CSICR3_ECC_INT_EN	(1 << 1)
> > +#define CSICR3_ECC_AUTO_EN	(1 << 0)
> > +
> > +#define SHIFT_FRMCNT		16
> > +
> > +/* csi status reg */
> > +#define CSISR_SFF_OR_INT	(1 << 25)
> > +#define CSISR_RFF_OR_INT	(1 << 24)
> > +#define CSISR_STATFF_INT	(1 << 21)
> > +#define CSISR_DMA_TSF_FB2_INT	(1 << 20)	/* MX25 */
> > +#define CSISR_DMA_TSF_FB1_INT	(1 << 19)	/* MX25 */
> > +#define CSISR_RXFF_INT		(1 << 18)
> > +#define CSISR_EOF_INT		(1 << 17)
> > +#define CSISR_SOF_INT		(1 << 16)
> > +#define CSISR_F2_INT		(1 << 15)
> > +#define CSISR_F1_INT		(1 << 14)
> > +#define CSISR_COF_INT		(1 << 13)
> > +#define CSISR_ECC_INT		(1 << 1)
> > +#define CSISR_DRDY		(1 << 0)
> > +
> > +#define CSICR1			0x00
> > +#define CSICR2			0x04
> > +#define CSISR			(cpu_is_mx27() ? 0x08 : 0x18)
> > +#define CSISTATFIFO		0x0c
> > +#define CSIRFIFO		0x10
> > +#define CSIRXCNT		0x14
> > +#define CSICR3			(cpu_is_mx27() ? 0x1C : 0x08)
> > +#define CSIDMASA_STATFIFO	0x20
> > +#define CSIDMATA_STATFIFO	0x24
> > +#define CSIDMASA_FB1		0x28
> > +#define CSIDMASA_FB2		0x2c
> > +#define CSIFBUF_PARA		0x30
> > +#define CSIIMAG_PARA		0x34
> > +
> > +/* EMMA PrP */
> > +#define PRP_CNTL			0x00
> > +#define PRP_INTR_CNTL			0x04
> > +#define PRP_INTRSTATUS			0x08
> > +#define PRP_SOURCE_Y_PTR		0x0c
> > +#define PRP_SOURCE_CB_PTR		0x10
> > +#define PRP_SOURCE_CR_PTR		0x14
> > +#define PRP_DEST_RGB1_PTR		0x18
> > +#define PRP_DEST_RGB2_PTR		0x1c
> > +#define PRP_DEST_Y_PTR			0x20
> > +#define PRP_DEST_CB_PTR			0x24
> > +#define PRP_DEST_CR_PTR			0x28
> > +#define PRP_SRC_FRAME_SIZE		0x2c
> > +#define PRP_DEST_CH1_LINE_STRIDE	0x30
> > +#define PRP_SRC_PIXEL_FORMAT_CNTL	0x34
> > +#define PRP_CH1_PIXEL_FORMAT_CNTL	0x38
> > +#define PRP_CH1_OUT_IMAGE_SIZE		0x3c
> > +#define PRP_CH2_OUT_IMAGE_SIZE		0x40
> > +#define PRP_SRC_LINE_STRIDE		0x44
> > +#define PRP_CSC_COEF_012		0x48
> > +#define PRP_CSC_COEF_345		0x4c
> > +#define PRP_CSC_COEF_678		0x50
> > +#define PRP_CH1_RZ_HORI_COEF1		0x54
> > +#define PRP_CH1_RZ_HORI_COEF2		0x58
> > +#define PRP_CH1_RZ_HORI_VALID		0x5c
> > +#define PRP_CH1_RZ_VERT_COEF1		0x60
> > +#define PRP_CH1_RZ_VERT_COEF2		0x64
> > +#define PRP_CH1_RZ_VERT_VALID		0x68
> > +#define PRP_CH2_RZ_HORI_COEF1		0x6c
> > +#define PRP_CH2_RZ_HORI_COEF2		0x70
> > +#define PRP_CH2_RZ_HORI_VALID		0x74
> > +#define PRP_CH2_RZ_VERT_COEF1		0x78
> > +#define PRP_CH2_RZ_VERT_COEF2		0x7c
> > +#define PRP_CH2_RZ_VERT_VALID		0x80
> > +
> > +#define PRP_CNTL_CH1EN		(1 << 0)
> > +#define PRP_CNTL_CH2EN		(1 << 1)
> > +#define PRP_CNTL_CSIEN		(1 << 2)
> > +#define PRP_CNTL_DATA_IN_YUV420	(0 << 3)
> > +#define PRP_CNTL_DATA_IN_YUV422	(1 << 3)
> > +#define PRP_CNTL_DATA_IN_RGB16	(2 << 3)
> > +#define PRP_CNTL_DATA_IN_RGB32	(3 << 3)
> > +#define PRP_CNTL_CH1_OUT_RGB8	(0 << 5)
> > +#define PRP_CNTL_CH1_OUT_RGB16	(1 << 5)
> > +#define PRP_CNTL_CH1_OUT_RGB32	(2 << 5)
> > +#define PRP_CNTL_CH1_OUT_YUV422	(3 << 5)
> > +#define PRP_CNTL_CH2_OUT_YUV420	(0 << 7)
> > +#define PRP_CNTL_CH2_OUT_YUV422 (1 << 7)
> > +#define PRP_CNTL_CH2_OUT_YUV444	(2 << 7)
> > +#define PRP_CNTL_CH1_LEN	(1 << 9)
> > +#define PRP_CNTL_CH2_LEN	(1 << 10)
> > +#define PRP_CNTL_SKIP_FRAME	(1 << 11)
> > +#define PRP_CNTL_SWRST		(1 << 12)
> > +#define PRP_CNTL_CLKEN		(1 << 13)
> > +#define PRP_CNTL_WEN		(1 << 14)
> > +#define PRP_CNTL_CH1BYP		(1 << 15)
> > +#define PRP_CNTL_IN_TSKIP(x)	((x) << 16)
> > +#define PRP_CNTL_CH1_TSKIP(x)	((x) << 19)
> > +#define PRP_CNTL_CH2_TSKIP(x)	((x) << 22)
> > +#define PRP_CNTL_INPUT_FIFO_LEVEL(x)	((x) << 25)
> > +#define PRP_CNTL_RZ_FIFO_LEVEL(x)	((x) << 27)
> > +#define PRP_CNTL_CH2B1EN	(1 << 29)
> > +#define PRP_CNTL_CH2B2EN	(1 << 30)
> > +#define PRP_CNTL_CH2FEN		(1 << 31)
> > +
> > +/* IRQ Enable and status register */
> > +#define PRP_INTR_RDERR		(1 << 0)
> > +#define PRP_INTR_CH1WERR	(1 << 1)
> > +#define PRP_INTR_CH2WERR	(1 << 2)
> > +#define PRP_INTR_CH1FC		(1 << 3)
> > +#define PRP_INTR_CH2FC		(1 << 5)
> > +#define PRP_INTR_LBOVF		(1 << 7)
> > +#define PRP_INTR_CH2OVF		(1 << 8)
> > +
> > +#define mx27_camera_emma(pcdev)	(cpu_is_mx27() && pcdev->use_emma)
> > +
> > +#define MAX_VIDEO_MEM	16
> > +
> > +struct mx2_camera_dev {
> > +	struct device		*dev;
> > +	struct soc_camera_host	soc_host;
> > +	struct soc_camera_device *icd;
> > +	struct clk		*clk_csi, *clk_emma;
> > +
> > +	unsigned int		irq_csi, irq_emma;
> > +	void __iomem		*base_csi, *base_emma;
> > +
> > +	struct mx2_camera_platform_data *pdata;
> > +	struct resource		*res_csi, *res_emma;
> > +	unsigned long		platform_flags;
> > +
> > +	struct list_head	capture;
> > +	struct list_head	active_bufs;
> > +
> > +	spinlock_t		lock;
> > +
> > +	int			dma;
> > +	struct mx2_buffer	*active;
> > +	struct mx2_buffer	*fb1_active;
> > +	struct mx2_buffer	*fb2_active;
> > +
> > +	int			use_emma;
> > +
> > +	unsigned int		csicr1;
> 
> This is a hardware register, u32 (also see comment below).
> 
> > +
> > +	void __iomem		*discard_buffer;
> > +	dma_addr_t		discard_buffer_dma;
> > +	size_t			discard_size;
> > +};
> > +
> > +/* buffer for one video frame */
> > +struct mx2_buffer {
> > +	/* common v4l buffer stuff -- must be first */
> > +	struct videobuf_buffer		vb;
> > +
> > +	enum v4l2_mbus_pixelcode	code;
> > +
> > +	int bufnum;
> > +};
> > +
> > +static int mclk_get_divisor(struct mx2_camera_dev *pcdev)
> > +{
> > +	dev_info(pcdev->dev, "%s not implemented. Running at max speed\n",
> > +			__func__);
> 
> Hm, why is this unimplemented?
> 
> > +
> > +#if 0
> > +	unsigned int mclk = pcdev->pdata->clk_csi;
> > +	unsigned int pclk = clk_get_rate(pcdev->clk_csi);
> > +	int i;
> > +
> > +	dev_dbg(pcdev->dev, "%s: %ld %ld\n", __func__, mclk, pclk);
> > +
> > +	for (i = 0; i < 0xf; i++)
> > +		if ((i + 1) * 2 * mclk <= pclk)
> > +			break;
> 
> This doesn't look right. You increment the counter i, and terminate the
> loop as soon as "(i + 1) * 2 * mclk <= pclk". Obviously, if 2 * mclk <= pclk,
> this will terminate immediately, otherwise it will run until the end and
> return 0xf without satisfying the condition. What exactly are you trying to
> achieve? Find the _largest_ i, such that "(i + 1) * 2 * mclk <= pclk"? Then
> why not just do "i = pclk / 2 / mclk - 1"?
> 
> > +	return i;
> > +#endif
> > +	return 0;
> > +}
> > +
> > +static void mx2_camera_deactivate(struct mx2_camera_dev *pcdev)
> > +{
> > +	clk_disable(pcdev->clk_csi);
> > +	writel(0, pcdev->base_csi + CSICR1);
> > +	if (mx27_camera_emma(pcdev))
> > +		writel(0, pcdev->base_emma + PRP_CNTL);
> > +}
> > +
> > +/* The following two functions absolutely depend on the fact, that
> > + * there can be only one camera on mx2 camera sensor interface */
> 
> Multiline comment
> 
> > +static int mx2_camera_add_device(struct soc_camera_device *icd)
> > +{
> > +	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> > +	struct mx2_camera_dev *pcdev = ici->priv;
> > +	int ret;
> > +	u32 csicr1;
> > +
> > +	if (pcdev->icd)
> > +		return -EBUSY;
> > +
> > +	ret = clk_enable(pcdev->clk_csi);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	csicr1 = CSICR1_MCLKDIV(mclk_get_divisor(pcdev)) |
> > +			CSICR1_MCLKEN;
> > +
> > +	if (mx27_camera_emma(pcdev)) {
> > +		csicr1 |= CSICR1_PRP_IF_EN | CSICR1_FCC |
> > +			CSICR1_RXFF_LEVEL(0);
> > +	} else if (cpu_is_mx27())
> > +		csicr1 |= CSICR1_SOF_INTEN | CSICR1_RXFF_LEVEL(2);
> > +
> > +	pcdev->csicr1 = csicr1;
> > +	writel(pcdev->csicr1, pcdev->base_csi + CSICR1);
> > +
> > +	pcdev->icd = icd;
> > +
> > +	dev_info(icd->dev.parent, "Camera driver attached to camera %d\n",
> > +		 icd->devnum);
> > +
> > +	return 0;
> > +}
> > +
> > +static void mx2_camera_remove_device(struct soc_camera_device *icd)
> > +{
> > +	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> > +	struct mx2_camera_dev *pcdev = ici->priv;
> > +
> > +	BUG_ON(icd != pcdev->icd);
> > +
> > +	dev_info(icd->dev.parent, "Camera driver detached from camera %d\n",
> > +		 icd->devnum);
> > +
> > +	mx2_camera_deactivate(pcdev);
> > +
> > +	if (pcdev->discard_buffer) {
> > +		dma_free_coherent(NULL, pcdev->discard_size,
> > +				pcdev->discard_buffer,
> > +				pcdev->discard_buffer_dma);
> 
> use ici->v4l2_dev.dev for device?
> 
> > +	}
> > +	pcdev->discard_buffer = 0;
> 
> discard_buffer is a pointer, so, " = NULL".
> 
> > +
> > +	pcdev->icd = NULL;
> > +}
> > +
> > +static void mx27_camera_dma_enable(struct mx2_camera_dev *pcdev)
> > +{
> > +	u32 tmp;
> > +
> > +	imx_dma_enable(pcdev->dma);
> > +
> > +	tmp = readl(pcdev->base_csi + CSICR1);
> > +	tmp |= CSICR1_RF_OR_INTEN;
> > +	writel(tmp, pcdev->base_csi + CSICR1);
> > +}
> > +
> > +static irqreturn_t mx27_camera_irq(int irq_csi, void *data)
> > +{
> > +	struct mx2_camera_dev *pcdev = data;
> > +	u32 status = readl(pcdev->base_csi + CSISR);
> > +
> > +	if (status & CSISR_SOF_INT && pcdev->active) {
> > +		u32 tmp;
> > +
> > +		tmp = readl(pcdev->base_csi + CSICR1);
> > +		tmp |= CSICR1_CLR_RXFIFO;
> > +		writel(tmp, pcdev->base_csi + CSICR1);
> 
> To me this always looks like that extra assignment to "tmp" must have some 
> meaning, whereas it doesn't. So, I find something like
> 
> +		tmp = readl(pcdev->base_csi + CSICR1) |
> +			CSICR1_CLR_RXFIFO;
> +		writel(tmp, pcdev->base_csi + CSICR1);
> 
> or
> 
> +		tmp = readl(pcdev->base_csi + CSICR1);
> +		writel(tmp | CSICR1_CLR_RXFIFO, pcdev->base_csi + CSICR1);
> 
> nicer.

I don't. I really like the three step read-modify-write better. The two
line style may be simple enough to read for a writel(tmp | VALUE, reg) case,
but I really find something like writel((tmp &= ~MASK) | VALUE, reg)
horrible to read, especially for long register and bit defines.
Therefore I always use the three line style.

> 
> > +		mx27_camera_dma_enable(pcdev);
> > +	}
> > +
> > +	writel(CSISR_SOF_INT | CSISR_RFF_OR_INT, pcdev->base_csi + CSISR);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static void mx25_camera_frame_done(struct mx2_camera_dev *pcdev, int fb,
> > +		int state)
> > +{
> > +	struct videobuf_buffer *vb;
> > +	struct mx2_buffer *buf;
> > +	struct mx2_buffer **fb_active = fb == 1 ? &pcdev->fb1_active :
> > +		&pcdev->fb2_active;
> > +	u32 fb_reg = fb == 1 ? CSIDMASA_FB1 : CSIDMASA_FB2;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&pcdev->lock, flags);
> > +
> > +	if ((*fb_active) == NULL)
> 
> Superfluous parenthesis. Maybe even just
> 
> +	if (!*fb_active)
> 
> > +		goto out;
> > +	vb = &(*fb_active)->vb;
> > +	dev_dbg(pcdev->dev, "%s (vb=0x%p) 0x%08lx %d\n", __func__,
> > +		vb, vb->baddr, vb->bsize);
> > +
> > +	vb->state = state;
> > +	do_gettimeofday(&vb->ts);
> > +	vb->field_count++;
> > +
> > +	wake_up(&vb->done);
> > +
> > +	if (list_empty(&pcdev->capture)) {
> > +		buf = NULL;
> > +		writel(0, pcdev->base_csi + fb_reg);
> > +	} else {
> > +		buf = list_entry(pcdev->capture.next, struct mx2_buffer,
> > +				vb.queue);
> > +		vb = &buf->vb;
> > +		list_del(&vb->queue);
> > +		vb->state = VIDEOBUF_ACTIVE;
> > +		writel(videobuf_to_dma_contig(vb), pcdev->base_csi + fb_reg);
> > +	}
> > +
> > +	*fb_active = buf;
> > +
> > +out:
> > +	spin_unlock_irqrestore(&pcdev->lock, flags);
> > +}
> > +
> > +static irqreturn_t mx25_camera_irq(int irq_csi, void *data)
> > +{
> > +	struct mx2_camera_dev *pcdev = data;
> > +	u32 status = readl(pcdev->base_csi + CSISR);
> > +
> > +	if (status & CSISR_DMA_TSF_FB1_INT)
> > +		mx25_camera_frame_done(pcdev, 1, VIDEOBUF_DONE);
> > +	else if (status & CSISR_DMA_TSF_FB2_INT)
> > +		mx25_camera_frame_done(pcdev, 2, VIDEOBUF_DONE);
> > +
> > +	/* FIXME: handle CSISR_RFF_OR_INT */
> > +
> > +	writel(status, pcdev->base_csi + CSISR);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +/*
> > + *  Videobuf operations
> > + */
> > +static int mx2_videobuf_setup(struct videobuf_queue *vq, unsigned int *count,
> > +			      unsigned int *size)
> > +{
> > +	struct soc_camera_device *icd = vq->priv_data;
> > +	int bytes_per_line = soc_mbus_bytes_per_line(icd->user_width,
> > +			icd->current_fmt->host_fmt);
> > +
> > +	dev_dbg(&icd->dev, "count=%d, size=%d\n", *count, *size);
> > +
> > +	if (bytes_per_line < 0)
> > +		return bytes_per_line;
> > +
> > +	*size = bytes_per_line * icd->user_height;
> > +
> > +	if (0 == *count)
> > +		*count = 32;
> > +	while (*size * *count > MAX_VIDEO_MEM * 1024 * 1024)
> > +		(*count)--;
> 
> Have a look at this:
> 
> http://git.linuxtv.org/v4l-dvb.git?a=commitdiff;h=f7a6936428c11b8bd33e7c438236142fd20cbf8b
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static void free_buffer(struct videobuf_queue *vq, struct mx2_buffer *buf)
> > +{
> > +	struct soc_camera_device *icd = vq->priv_data;
> > +
> > +	BUG_ON(in_interrupt());
> 
> Is there a need for this? I see this being copy-pasted across multiple 
> drivers, but maybe it's time to stop?:)
> 
> > +
> > +	dev_dbg(&icd->dev, "%s (vb=0x%p) 0x%08lx %d\n", __func__,
> > +		&buf->vb, buf->vb.baddr, buf->vb.bsize);
> 
> You reference buf->vb 6 times in this function, so, maybr it's time for an 
> own variable
> 
> 	struct videobuf_buffer *vb = &buf->vb;
> 
> ?
> 
> > +
> > +	/* This waits until this buffer is out of danger, i.e., until it is no
> > +	 * longer in STATE_QUEUED or STATE_ACTIVE */
> 
> multiline comment style
> 
> > +	videobuf_waiton(&buf->vb, 0, 0);
> > +
> > +	videobuf_dma_contig_free(vq, &buf->vb);
> > +	dev_dbg(&icd->dev, "%s freed\n", __func__);
> > +
> > +	buf->vb.state = VIDEOBUF_NEEDS_INIT;
> > +}
> > +
> > +static int mx2_videobuf_prepare(struct videobuf_queue *vq,
> > +		struct videobuf_buffer *vb, enum v4l2_field field)
> > +{
> > +	struct soc_camera_device *icd = vq->priv_data;
> > +	struct mx2_buffer *buf = container_of(vb, struct mx2_buffer, vb);
> > +	int bytes_per_line = soc_mbus_bytes_per_line(icd->user_width,
> > +			icd->current_fmt->host_fmt);
> > +	int ret = 0;
> > +
> > +#ifdef DEBUG
> > +	/* This can be useful if you want to see if we actually fill
> > +	 * the buffer with something */
> 
> comment style
> 
> > +	memset((void *)vb->baddr, 0xaa, vb->bsize);
> > +#endif
> 
> Move this DEBUG block below bytes_per_line check?
> 
> > +
> > +	dev_dbg(&icd->dev, "%s (vb=0x%p) 0x%08lx %d\n", __func__,
> > +		vb, vb->baddr, vb->bsize);
> > +
> > +	if (bytes_per_line < 0)
> > +		return bytes_per_line;
> > +
> > +	if (buf->code	!= icd->current_fmt->code ||
> > +	    vb->width	!= icd->user_width ||
> > +	    vb->height	!= icd->user_height ||
> > +	    vb->field	!= field) {
> > +		buf->code	= icd->current_fmt->code;
> > +		vb->width	= icd->user_width;
> > +		vb->height	= icd->user_height;
> > +		vb->field	= field;
> > +		vb->state	= VIDEOBUF_NEEDS_INIT;
> > +	}
> > +
> > +	vb->size = bytes_per_line * vb->height;
> > +	if (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;
> > +	}
> > +
> > +	return 0;
> > +
> > +fail:
> > +	free_buffer(vq, buf);
> > +out:
> > +	return ret;
> > +}
> > +
> > +static void mx2_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);
> > +	struct mx2_camera_dev *pcdev = ici->priv;
> > +	struct mx2_buffer *buf = container_of(vb, struct mx2_buffer, vb);
> > +	unsigned long flags;
> > +	int ret;
> > +
> > +	dev_dbg(&icd->dev, "%s (vb=0x%p) 0x%08lx %d\n", __func__,
> > +		vb, vb->baddr, vb->bsize);
> > +
> > +	spin_lock_irqsave(&pcdev->lock, flags);
> > +
> > +	vb->state = VIDEOBUF_QUEUED;
> > +	list_add_tail(&vb->queue, &pcdev->capture);
> > +
> > +	if (mx27_camera_emma(pcdev))
> > +		goto out;
> > +	else if (cpu_is_mx27()) {
> > +		vb->state = VIDEOBUF_ACTIVE;
> 
> You could put this below the "if" to avoid setting state twice in case of 
> an error.
> 
> > +
> > +		if (!pcdev->active) {
> > +			ret = imx_dma_setup_single(pcdev->dma,
> > +					videobuf_to_dma_contig(vb), vb->size,
> > +					(u32)pcdev->base_csi + 0x10,
> > +					DMA_MODE_READ);
> > +			if (ret) {
> > +				vb->state = VIDEOBUF_ERROR;
> > +				wake_up(&vb->done);
> > +				goto out;
> > +			}
> > +
> > +			pcdev->active = buf;
> > +		}
> > +	} else { /* cpu_is_mx25() */
> > +		u32 csicr3, dma_inten = 0;
> > +
> > +		if (pcdev->fb1_active == NULL) {
> 
> I find consistent coding nicer. Just a few lines above you test a pointer 
> as "if (!x)", and here it's already "if (x == NULL)", please, select one 
> style and follow it across the driver.
> 
> > +			writel(videobuf_to_dma_contig(vb),
> > +					pcdev->base_csi + CSIDMASA_FB1);
> > +			pcdev->fb1_active = buf;
> > +			dma_inten |= CSICR1_FB1_DMA_INTEN;
> 
> No need for "|", just a "=" would do.
> 
> > +		} else if (pcdev->fb2_active == NULL) {
> > +			writel(videobuf_to_dma_contig(vb),
> > +					pcdev->base_csi + CSIDMASA_FB2);
> > +			pcdev->fb2_active = buf;
> > +			dma_inten |= CSICR1_FB2_DMA_INTEN;
> 
> ditto
> 
> > +		}
> > +
> > +		if (dma_inten) {
> > +			list_del(&vb->queue);
> > +			vb->state = VIDEOBUF_ACTIVE;
> > +
> > +			csicr3 = readl(pcdev->base_csi + CSICR3);
> > +
> > +			/* Reflash DMA */
> > +			writel(csicr3 | CSICR3_DMA_REFLASH_RFF,
> > +					pcdev->base_csi + CSICR3);
> > +
> > +			/* clear & enable interrupts */
> > +			writel(dma_inten, pcdev->base_csi + CSISR);
> > +			pcdev->csicr1 |= dma_inten;
> > +			writel(pcdev->csicr1, pcdev->base_csi + CSICR1);
> > +
> > +			/* enable DMA */
> > +			csicr3 |= CSICR3_DMA_REQ_EN_RFF | CSICR3_RXFF_LEVEL(1);
> > +			writel(csicr3, pcdev->base_csi + CSICR3);
> > +		}
> > +	}
> > +
> > +out:
> > +	spin_unlock_irqrestore(&pcdev->lock, flags);
> > +}
> > +
> > +static void mx2_videobuf_release(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);
> > +	struct mx2_camera_dev *pcdev = ici->priv;
> > +	struct mx2_buffer *buf = container_of(vb, struct mx2_buffer, vb);
> > +	unsigned long flags;
> > +
> > +#ifdef DEBUG
> > +	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_info(&icd->dev, "%s (active)\n", __func__);
> > +		break;
> > +	case VIDEOBUF_QUEUED:
> > +		dev_info(&icd->dev, "%s (queued)\n", __func__);
> > +		break;
> > +	case VIDEOBUF_PREPARED:
> > +		dev_info(&icd->dev, "%s (prepared)\n", __func__);
> > +		break;
> > +	default:
> > +		dev_info(&icd->dev, "%s (unknown) %d\n", __func__,
> > +				vb->state);
> > +		break;
> > +	}
> > +#endif
> > +
> > +	spin_lock_irqsave(&pcdev->lock, flags);
> > +	if (vb->state == VIDEOBUF_QUEUED)
> > +		list_del(&vb->queue);
> > +	else if (pcdev->fb1_active == buf)
> > +		pcdev->fb1_active = NULL;
> > +	else if (pcdev->fb2_active == buf)
> > +		pcdev->fb2_active = NULL;
> > +	else
> > +		goto done;
> 
> Don't you also have to check for pcdev->active for i.mx27?
> 
> > +
> > +	vb->state = VIDEOBUF_ERROR;
> > +
> > +done:
> > +	spin_unlock_irqrestore(&pcdev->lock, flags);
> > +	free_buffer(vq, buf);
> > +}
> > +
> > +static struct videobuf_queue_ops mx2_videobuf_ops = {
> > +	.buf_setup      = mx2_videobuf_setup,
> > +	.buf_prepare    = mx2_videobuf_prepare,
> > +	.buf_queue      = mx2_videobuf_queue,
> > +	.buf_release    = mx2_videobuf_release,
> > +};
> > +
> > +static void mx2_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 mx2_camera_dev *pcdev = ici->priv;
> > +
> > +	videobuf_queue_dma_contig_init(q, &mx2_videobuf_ops, pcdev->dev,
> > +			&pcdev->lock, V4L2_BUF_TYPE_VIDEO_CAPTURE,
> > +			V4L2_FIELD_NONE, sizeof(struct mx2_buffer), icd);
> > +}
> > +
> > +#define MX2_BUS_FLAGS	(SOCAM_DATAWIDTH_8 | \
> > +			SOCAM_MASTER | \
> > +			SOCAM_VSYNC_ACTIVE_HIGH | \
> > +			SOCAM_VSYNC_ACTIVE_LOW | \
> > +			SOCAM_HSYNC_ACTIVE_HIGH | \
> > +			SOCAM_HSYNC_ACTIVE_LOW | \
> > +			SOCAM_PCLK_SAMPLE_RISING | \
> > +			SOCAM_PCLK_SAMPLE_FALLING | \
> > +			SOCAM_DATA_ACTIVE_HIGH | \
> > +			SOCAM_DATA_ACTIVE_LOW)
> > +
> > +
> 
> Well, you normally do one blank line everywhere, don't you? Then this one 
> should better go too.
> 
> > +static int mx27_camera_emma_prp_reset(struct mx2_camera_dev *pcdev)
> > +{
> > +	unsigned int cntl;
> 
> I find it better to use fixed-length variables for hardware registers, 
> i.e., u32 in this case. Possibly, this is not the only location.
> 
> > +
> > +	cntl = readl(pcdev->base_emma + PRP_CNTL);
> > +	writel(PRP_CNTL_SWRST, pcdev->base_emma + PRP_CNTL);
> > +	while (readl(pcdev->base_emma + PRP_CNTL) & PRP_CNTL_SWRST)
> > +		barrier();
> > +
> > +	return 0;
> 
> The function returns an int and it's checked, but it always returns 0. 
> Whereas it'd be better to count-limit the loop and return something like 
> -ETIMEDOUT if counter runs out.
> 
> > +}
> > +
> > +static void mx27_camera_emma_buf_init(struct soc_camera_device *icd,
> > +		int bytesperline)
> > +{
> > +	struct soc_camera_host *ici =
> > +		to_soc_camera_host(icd->dev.parent);
> > +	struct mx2_camera_dev *pcdev = ici->priv;
> > +
> > +	writel(pcdev->discard_buffer_dma,
> > +			pcdev->base_emma + PRP_DEST_RGB1_PTR);
> > +	writel(pcdev->discard_buffer_dma,
> > +			pcdev->base_emma + PRP_DEST_RGB2_PTR);
> > +
> > +	/* We only use the EMMA engine to get rid of the f**king
> 
> maybe you can select a more technical and less emotional comment;)

My bad, I felt very emotional that day ;)

> 
> > +	 * DMA Engine. No color space consversion at the moment.
> > +	 * We adjust incoming and outgoing pixelformat to rgb16
> > +	 * and adjust the bytesperline accordingly.
> > +	 */
> 
> comment-style
> 
> > +	writel(PRP_CNTL_CH1EN |
> > +			PRP_CNTL_CSIEN |
> > +			PRP_CNTL_DATA_IN_RGB16 |
> > +			PRP_CNTL_CH1_OUT_RGB16 |
> > +			PRP_CNTL_CH1_LEN |
> > +			PRP_CNTL_CH1BYP |
> > +			PRP_CNTL_CH1_TSKIP(0) |
> > +			PRP_CNTL_IN_TSKIP(0),
> > +			pcdev->base_emma + PRP_CNTL);
> > +
> > +	writel(((bytesperline >> 1) << 16) | icd->user_height,
> > +			pcdev->base_emma + PRP_SRC_FRAME_SIZE);
> > +	writel(((bytesperline >> 1) << 16) | icd->user_height,
> > +			pcdev->base_emma + PRP_CH1_OUT_IMAGE_SIZE);
> > +	writel(bytesperline,
> > +			pcdev->base_emma + PRP_DEST_CH1_LINE_STRIDE);
> > +	writel(0x2ca00565,
> > +			pcdev->base_emma + PRP_SRC_PIXEL_FORMAT_CNTL);
> > +	writel(0x2ca00565,
> > +			pcdev->base_emma + PRP_CH1_PIXEL_FORMAT_CNTL);
> 
> I don't mind using hard-coded numbers, but please, add a comment, 
> explaining.
> 
> > +
> > +	/* Enable interrupts */
> > +	writel(PRP_INTR_RDERR |
> > +			PRP_INTR_CH1WERR |
> > +			PRP_INTR_CH2WERR |
> > +			PRP_INTR_CH1FC |
> > +			PRP_INTR_CH2FC |
> > +			PRP_INTR_LBOVF |
> > +			PRP_INTR_CH2OVF
> > +			, pcdev->base_emma + PRP_INTR_CNTL);
> 
> misplaced comma
> 
> > +}
> > +
> > +static int mx2_camera_set_bus_param(struct soc_camera_device *icd,
> > +		__u32 pixfmt)
> > +{
> > +	struct soc_camera_host *ici =
> > +		to_soc_camera_host(icd->dev.parent);
> > +	struct mx2_camera_dev *pcdev = ici->priv;
> > +	unsigned long camera_flags, common_flags;
> > +	int ret = 0;
> > +	int bytesperline;
> > +	u32 csicr1 = pcdev->csicr1;
> > +
> > +	camera_flags = icd->ops->query_bus_param(icd);
> > +
> > +	common_flags = soc_camera_bus_param_compatible(camera_flags,
> > +				MX2_BUS_FLAGS);
> > +	if (!common_flags)
> > +		return -EINVAL;
> > +
> > +	if ((common_flags & SOCAM_HSYNC_ACTIVE_HIGH) &&
> > +	    (common_flags & SOCAM_HSYNC_ACTIVE_LOW)) {
> > +		if (pcdev->platform_flags & MX2_CAMERA_HSYNC_HIGH)
> > +			common_flags &= ~SOCAM_HSYNC_ACTIVE_LOW;
> > +		else
> > +			common_flags &= ~SOCAM_HSYNC_ACTIVE_HIGH;
> > +	}
> > +
> > +	if ((common_flags & SOCAM_PCLK_SAMPLE_RISING) &&
> > +	    (common_flags & SOCAM_PCLK_SAMPLE_FALLING)) {
> > +		if (pcdev->platform_flags & MX2_CAMERA_PCLK_SAMPLE_RISING)
> > +			common_flags &= ~SOCAM_PCLK_SAMPLE_FALLING;
> > +		else
> > +			common_flags &= ~SOCAM_PCLK_SAMPLE_RISING;
> > +	}
> > +
> > +	ret = icd->ops->set_bus_param(icd, common_flags);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if (common_flags & SOCAM_PCLK_SAMPLE_FALLING)
> > +		csicr1 |= CSICR1_INV_PCLK;
> > +	if (common_flags & SOCAM_VSYNC_ACTIVE_HIGH)
> > +		csicr1 |= CSICR1_SOF_POL;
> > +	if (common_flags & SOCAM_HSYNC_ACTIVE_HIGH)
> > +		csicr1 |= CSICR1_HSYNC_POL;
> > +	if (pcdev->platform_flags & MX2_CAMERA_SWAP16)
> > +		csicr1 |= CSICR1_SWAP16_EN;
> > +	if (pcdev->platform_flags & MX2_CAMERA_EXT_VSYNC)
> > +		csicr1 |= CSICR1_EXT_VSYNC;
> > +	if (pcdev->platform_flags & MX2_CAMERA_CCIR)
> > +		csicr1 |= CSICR1_CCIR_EN;
> > +	if (pcdev->platform_flags & MX2_CAMERA_CCIR_INTERLACE)
> > +		csicr1 |= CSICR1_CCIR_MODE;
> > +	if (pcdev->platform_flags & MX2_CAMERA_GATED_CLOCK)
> > +		csicr1 |= CSICR1_GCLK_MODE;
> > +	if (pcdev->platform_flags & MX2_CAMERA_INV_DATA)
> > +		csicr1 |= CSICR1_INV_DATA;
> > +	if (pcdev->platform_flags & MX2_CAMERA_PACK_DIR_MSB)
> > +		csicr1 |= CSICR1_PACK_DIR;
> > +
> > +	pcdev->csicr1 = csicr1;
> > +
> > +	bytesperline = soc_mbus_bytes_per_line(icd->user_width,
> > +			icd->current_fmt->host_fmt);
> > +	if (bytesperline < 0)
> > +		return bytesperline;
> > +
> > +	if (mx27_camera_emma(pcdev)) {
> > +		if (mx27_camera_emma_prp_reset(pcdev))
> > +			return -ENODEV;
> > +
> > +		if (pcdev->discard_buffer)
> > +			dma_free_coherent(NULL, pcdev->discard_size,
> > +				pcdev->discard_buffer,
> > +				pcdev->discard_buffer_dma);
> 
> ici->v4l2_dev.dev and below
> 
> > +
> > +		/* I didn't manage to properly enable/disable the prp
> > +		 * on a per frame basis during running transfers,
> > +		 * thus we allocate a buffer here and use it to
> > +		 * discard frames when no buffer is available.
> > +		 * Feel free to work on this ;)
> > +		 */
> 
> comment style
> 
> > +		pcdev->discard_size = icd->user_height * bytesperline;
> > +		pcdev->discard_buffer = dma_alloc_coherent(NULL,
> > +				pcdev->discard_size, &pcdev->discard_buffer_dma,
> > +				GFP_KERNEL);
> > +		if (!pcdev->discard_buffer)
> > +			return -ENOMEM;
> > +
> > +		mx27_camera_emma_buf_init(icd, bytesperline);
> > +	} else if (cpu_is_mx25()) {
> > +		writel((bytesperline * icd->user_height) >> 2,
> > +				pcdev->base_csi + CSIRXCNT);
> > +		writel((bytesperline << 16) | icd->user_height,
> > +				pcdev->base_csi + CSIIMAG_PARA);
> > +	}
> > +
> > +	writel(pcdev->csicr1, pcdev->base_csi + CSICR1);
> > +
> > +	return 0;
> > +}
> > +
> > +static int mx2_camera_set_crop(struct soc_camera_device *icd,
> > +				struct v4l2_crop *a)
> > +{
> > +	struct v4l2_rect *rect = &a->c;
> > +	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
> > +	struct v4l2_format f = {.type = V4L2_BUF_TYPE_VIDEO_CAPTURE};
> > +	struct v4l2_pix_format *pix = &f.fmt.pix;
> > +	int ret;
> > +
> > +	soc_camera_limit_side(&rect->left, &rect->width, 0, 2, 4096);
> > +	soc_camera_limit_side(&rect->top, &rect->height, 0, 2, 4096);
> > +
> > +	ret = v4l2_subdev_call(sd, video, s_crop, a);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/* The capture device might have changed its output  */
> > +	ret = v4l2_subdev_call(sd, video, g_fmt, &f);
> 
> Have you tested cropping?;) You should be using g_mbus_fmt here and a 
> different (fourth) format parameter, of course, too. Since you're 
> supporting a changed output geometry, you should also verify, that the new 
> geometry is still acceptable for you and adjust your configuration.
> 
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	dev_dbg(icd->dev.parent, "Sensor cropped %dx%d\n",
> > +		pix->width, pix->height);
> > +
> > +	icd->user_width = pix->width;
> > +	icd->user_height = pix->height;
> > +
> > +	return ret;
> > +}
> > +
> > +static int mx2_camera_set_fmt(struct soc_camera_device *icd,
> > +			       struct v4l2_format *f)
> > +{
> > +	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
> > +	const struct soc_camera_format_xlate *xlate;
> > +	struct v4l2_pix_format *pix = &f->fmt.pix;
> > +	struct v4l2_mbus_framefmt mf;
> > +	int ret;
> > +
> > +	xlate = soc_camera_xlate_by_fourcc(icd, pix->pixelformat);
> > +	if (!xlate) {
> > +		dev_warn(icd->dev.parent, "Format %x not found\n",
> > +				pix->pixelformat);
> > +		return -EINVAL;
> > +	}
> > +
> > +	mf.width	= pix->width;
> > +	mf.height	= pix->height;
> > +	mf.field	= pix->field;
> > +	mf.colorspace	= pix->colorspace;
> > +	mf.code		= xlate->code;
> > +
> > +	ret = v4l2_subdev_call(sd, video, s_mbus_fmt, &mf);
> > +	if (ret < 0 && ret != -ENOIOCTLCMD)
> > +		return ret;
> > +
> > +	if (mf.code != xlate->code)
> > +		return -EINVAL;
> > +
> > +	pix->width		= mf.width;
> > +	pix->height		= mf.height;
> > +	pix->field		= mf.field;
> > +	pix->colorspace		= mf.colorspace;
> > +	icd->current_fmt	= xlate;
> > +
> > +	return 0;
> > +}
> > +
> > +static int mx2_camera_try_fmt(struct soc_camera_device *icd,
> > +				  struct v4l2_format *f)
> > +{
> > +	return 0;
> 
> Oops, no, no good. Please, add relevant checks and parameter adjustments.
> 
> > +}
> > +
> > +static int mx2_camera_querycap(struct soc_camera_host *ici,
> > +			       struct v4l2_capability *cap)
> > +{
> > +	/* cap->name is set by the friendly caller:-> */
> > +	strlcpy(cap->card, MX2_CAM_DRIVER_DESCRIPTION, sizeof(cap->card));
> > +	cap->version = MX2_CAM_VERSION_CODE;
> > +	cap->capabilities = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
> > +
> > +	return 0;
> > +}
> > +
> > +static int mx2_camera_reqbufs(struct soc_camera_file *icf,
> > +			      struct v4l2_requestbuffers *p)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < p->count; i++) {
> > +		struct mx2_buffer *buf = container_of(icf->vb_vidq.bufs[i],
> > +						      struct mx2_buffer, vb);
> > +		INIT_LIST_HEAD(&buf->vb.queue);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void mx27_camera_frame_done(struct mx2_camera_dev *pcdev, int state)
> > +{
> > +	struct videobuf_buffer *vb;
> > +	struct mx2_buffer *buf;
> > +	int ret;
> > +
> > +	if (!pcdev->active) {
> > +		dev_err(pcdev->dev, "%s called with no active buffer!\n",
> > +				__func__);
> > +		return;
> > +	}
> > +
> > +	vb = &pcdev->active->vb;
> > +	buf = container_of(vb, struct mx2_buffer, vb);
> > +	WARN_ON(list_empty(&vb->queue));
> > +	dev_dbg(pcdev->dev, "%s (vb=0x%p) 0x%08lx %d\n", __func__,
> > +		vb, vb->baddr, vb->bsize);
> > +
> > +	/* _init is used to debug races, see comment in pxa_camera_reqbufs() */
> > +	list_del_init(&vb->queue);
> > +	vb->state = state;
> > +	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 mx2_buffer, vb.queue);
> > +
> > +	vb = &pcdev->active->vb;
> > +	vb->state = VIDEOBUF_ACTIVE;
> > +
> > +	ret = imx_dma_setup_single(pcdev->dma, videobuf_to_dma_contig(vb),
> > +			vb->size, (u32)pcdev->base_csi + 0x10, DMA_MODE_READ);
> > +	if (ret) {
> > +		vb->state = VIDEOBUF_ERROR;
> > +		wake_up(&vb->done);
> 
> I think, in this case you don't want pcdev->active to be set.
> 
> > +		return;
> 
> superfluous "return."
> 
> > +	}
> > +}
> > +
> > +static void mx27_camera_dma_err_callback(int channel, void *data, int err)
> > +{
> > +	struct mx2_camera_dev *pcdev = data;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&pcdev->lock, flags);
> > +
> > +	mx27_camera_frame_done(pcdev, VIDEOBUF_ERROR);
> 
> I think, it would be better to take and release the spinlock inside the 
> frame_done function, similar to mx25. This would improve consistency and 
> reduce the number of spinlock manipulating locations (here and below) by 
> one.
> 
> > +
> > +	spin_unlock_irqrestore(&pcdev->lock, flags);
> > +}
> > +
> > +static void mx27_camera_dma_callback(int channel, void *data)
> > +{
> > +	struct mx2_camera_dev *pcdev = data;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&pcdev->lock, flags);
> > +
> > +	mx27_camera_frame_done(pcdev, VIDEOBUF_DONE);
> > +
> > +	spin_unlock_irqrestore(&pcdev->lock, flags);
> > +}
> > +
> > +#define DMA_REQ_CSI_RX          31 /* FIXME: Add this to a resource */
> > +
> > +static int mx27_camera_dma_init(struct platform_device *pdev,
> > +		struct mx2_camera_dev *pcdev)
> 
> This is only called from mx2_camera_probe(), so, you can make this one 
> "__devinit" too.
> 
> > +{
> > +	int err;
> > +
> > +	pcdev->dma = imx_dma_request_by_prio("CSI RX DMA", DMA_PRIO_HIGH);
> > +	if (pcdev->dma < 0) {
> > +		dev_err(&pdev->dev, "%s failed to request DMA channel\n",
> > +				__func__);
> > +		return pcdev->dma;
> > +	}
> > +
> > +	err = imx_dma_setup_handlers(pcdev->dma, mx27_camera_dma_callback,
> > +					mx27_camera_dma_err_callback, pcdev);
> > +	if (err != 0) {
> > +		dev_err(&pdev->dev, "%s failed to set DMA callback\n",
> > +				__func__);
> > +		imx_dma_free(pcdev->dma);
> > +		return err;
> > +	}
> > +
> > +	imx_dma_config_channel(pcdev->dma,
> > +			IMX_DMA_MEMSIZE_32 | IMX_DMA_TYPE_FIFO,
> > +			IMX_DMA_MEMSIZE_32 | IMX_DMA_TYPE_LINEAR,
> > +			DMA_REQ_CSI_RX, 1);
> 
> Ok, looking at the function implementation, it cannot really fail in your 
> case - on i.mx27, but it'd be better to either check for the return code 
> in any case - e.g., in case the function changes, or at least add a 
> comment.
> 
> > +
> > +	imx_dma_config_burstlen(pcdev->dma, 64);
> > +
> > +	return 0;
> > +}
> > +
> > +static unsigned int mx2_camera_poll(struct file *file, poll_table *pt)
> > +{
> > +	struct soc_camera_file *icf = file->private_data;
> > +	struct mx2_buffer *buf;
> > +
> > +	buf = list_entry(icf->vb_vidq.stream.next, struct mx2_buffer,
> > +			 vb.stream);
> > +
> > +	poll_wait(file, &buf->vb.done, pt);
> > +
> > +	if (buf->vb.state == VIDEOBUF_DONE ||
> > +	    buf->vb.state == VIDEOBUF_ERROR)
> > +		return POLLIN | POLLRDNORM;
> 
> Any specific reason not to use videobuf_poll_stream() (see mx3_camera.c)? 
> Apart from copy-pasting from other soc-camera drivers;)
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static struct soc_camera_host_ops mx2_soc_camera_host_ops = {
> > +	.owner		= THIS_MODULE,
> > +	.add		= mx2_camera_add_device,
> > +	.remove		= mx2_camera_remove_device,
> > +	.set_fmt	= mx2_camera_set_fmt,
> > +	.set_crop	= mx2_camera_set_crop,
> > +	.try_fmt	= mx2_camera_try_fmt,
> > +	.init_videobuf	= mx2_camera_init_videobuf,
> > +	.reqbufs	= mx2_camera_reqbufs,
> > +	.poll		= mx2_camera_poll,
> > +	.querycap	= mx2_camera_querycap,
> > +	.set_bus_param	= mx2_camera_set_bus_param,
> > +};
> > +
> > +static void mx27_camera_frame_done_emma(struct mx2_camera_dev *pcdev,
> > +		int bufnum, int state)
> > +{
> > +	struct mx2_buffer *buf;
> > +	struct videobuf_buffer *vb;
> > +	unsigned long phys;
> > +
> > +	if (!list_empty(&pcdev->active_bufs)) {
> > +		buf = list_entry(pcdev->active_bufs.next,
> > +			struct mx2_buffer, vb.queue);
> > +
> > +		if (buf->bufnum == bufnum) {
> 
> Hmmm... In the ISR below bits for both channels are tested in the status 
> non-exclusively, and this function is called first for channel 1, then for 
> channel 2. This means, the ISR is prepared for the (unlikely) case, that 
> both bits are set, if we missed one interrupt. Now think what happens, if 
> you're expecting completion on channel 2, then you leave channel 1 
> unprocessed... This is not very trivial to fix without testing, but I 
> think, there is a relatively easy fix - see below.
> 
> After you've done that proposed change, I'd suggest to change this "if" to 
> a "BUG_ON(buf->bufnum != bufnum)", so that the first person that uses this 
> driver with eMMA sees immediately, if our fix was wrong;)
> 
> > +			vb = &buf->vb;
> > +#ifdef DEBUG
> > +			phys = videobuf_to_dma_contig(vb);
> > +			if (readl(pcdev->base_emma + PRP_DEST_RGB1_PTR +
> > +						4 * bufnum) != phys) {
> > +				dev_err(pcdev->dev, "%p != %p\n", phys,
> > +						readl(pcdev->base_emma +
> > +						PRP_DEST_RGB1_PTR +
> > +						4 * bufnum));
> > +			}
> > +#endif
> > +			dev_dbg(pcdev->dev, "%s (vb=0x%p) 0x%08lx %d\n",
> > +					__func__, vb, vb->baddr, vb->bsize);
> > +
> > +			list_del(&vb->queue);
> > +			vb->state = state;
> > +			do_gettimeofday(&vb->ts);
> > +			vb->field_count++;
> > +
> > +			wake_up(&vb->done);
> > +		}
> > +	}
> > +
> > +	if (list_empty(&pcdev->capture)) {
> > +		writel(pcdev->discard_buffer_dma, pcdev->base_emma +
> > +				PRP_DEST_RGB1_PTR + 4 * bufnum);
> > +		return;
> > +	}
> > +
> > +	buf = list_entry(pcdev->capture.next,
> > +			struct mx2_buffer, vb.queue);
> > +
> > +	buf->bufnum = bufnum;
> > +
> > +	list_move_tail(pcdev->capture.next, &pcdev->active_bufs);
> > +
> > +	vb = &buf->vb;
> > +	vb->state = VIDEOBUF_ACTIVE;
> > +
> > +	phys = videobuf_to_dma_contig(vb);
> > +	writel(phys, pcdev->base_emma + PRP_DEST_RGB1_PTR + 4 * bufnum);
> > +}
> > +
> > +static irqreturn_t mx27_camera_emma_irq(int irq_emma, void *data)
> > +{
> > +	struct mx2_camera_dev *pcdev = data;
> > +	unsigned int status = readl(pcdev->base_emma + PRP_INTRSTATUS);
> > +
> > +	if (status & (1 << 6))
> > +		mx27_camera_frame_done_emma(pcdev, 0, VIDEOBUF_DONE);
> > +	if (status & (1 << 5))
> > +		mx27_camera_frame_done_emma(pcdev, 1, VIDEOBUF_DONE);
> 
> So, the proposed fix is the following: look which channel is expected to 
> complete and call mx27_camera_frame_done_emma() for it first. Only then 
> check the other channel.
> 
> > +	if (status & (1 << 7)) {
> > +		uint32_t cntl;
> 
> grrrr.... consistency - use u32.
> 
> > +		cntl = readl(pcdev->base_emma + PRP_CNTL);
> > +		writel(cntl & ~PRP_CNTL_CH1EN, pcdev->base_emma + PRP_CNTL);
> > +		writel(cntl, pcdev->base_emma + PRP_CNTL);
> 
> Hm, this doesn't look right to me. Why disable hard-coded channel 1? 
> cannot the line buffer overflow on channel 2? Sascha?

We only have channel one enabled. The driver will need some rework if we
wish to enable the second channel. I can't remember though if this ever
triggered.

> 
> > +	}
> > +
> > +	writel(status, pcdev->base_emma + PRP_INTRSTATUS);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int mx27_camera_emma_init(struct mx2_camera_dev *pcdev)
> 
> This is only called from mx2_camera_probe(), so, you can make this one 
> "__devinit" too.
> 
> > +{
> > +	struct resource *res_emma = pcdev->res_emma;
> > +	int err = 0;
> > +
> > +	if (!request_mem_region(res_emma->start, resource_size(res_emma),
> > +				MX2_CAM_DRV_NAME)) {
> > +		err = -EBUSY;
> > +		goto out;
> > +	}
> > +
> > +	pcdev->base_emma = ioremap(res_emma->start, resource_size(res_emma));
> > +	if (!pcdev->base_emma) {
> > +		err = -ENOMEM;
> > +		goto exit_release;
> > +	}
> > +
> > +	err = request_irq(pcdev->irq_emma, mx27_camera_emma_irq, 0,
> > +			MX2_CAM_DRV_NAME, pcdev);
> > +	if (err) {
> > +		dev_err(pcdev->dev, "Camera EMMA interrupt register failed \n");
> > +		goto exit_iounmap;
> > +	}
> > +
> > +	pcdev->clk_emma = clk_get(NULL, "emma");
> > +	if (IS_ERR(pcdev->clk_emma)) {
> > +		err = PTR_ERR(pcdev->clk_emma);
> > +		goto exit_free_irq;
> > +	}
> > +
> > +	clk_enable(pcdev->clk_emma);
> > +
> > +	err = mx27_camera_emma_prp_reset(pcdev);
> > +	if (err)
> > +		goto exit_clk_emma_put;
> > +
> > +	return err;
> > +
> > +exit_clk_emma_put:
> > +	clk_disable(pcdev->clk_emma);
> > +	clk_put(pcdev->clk_emma);
> > +exit_free_irq:
> > +	free_irq(pcdev->irq_emma, pcdev);
> > +exit_iounmap:
> > +	iounmap(pcdev->base_emma);
> > +exit_release:
> > +	release_mem_region(res_emma->start, resource_size(res_emma));
> > +out:
> > +	return err;
> > +}
> > +
> > +static int mx2_camera_probe(struct platform_device *pdev)
> 
> You use "__devexit" for mx2_camera_remove() below, so, you should 
> "__devinit" here then too.
> 
> > +{
> > +	struct mx2_camera_dev *pcdev;
> > +	struct resource *res_csi, *res_emma;
> > +	void __iomem *base_csi;
> > +	unsigned int irq_csi, irq_emma;
> > +	irq_handler_t mx2_cam_irq_handler = cpu_is_mx25() ? mx25_camera_irq
> > +		: mx27_camera_irq;
> > +	int err = 0;
> > +
> > +	dev_info(&pdev->dev, "initialising\n");
> 
> This is not very informative...
> 
> > +
> > +	res_csi = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	irq_csi = platform_get_irq(pdev, 0);
> > +	if (!res_csi || !irq_csi) {
> 
> Wrong error condition: platform_get_irq() returns a negative error code on 
> failure
> 
> > +		dev_err(&pdev->dev, "Missing platform resources data\n");
> > +		err = -ENODEV;
> > +		goto exit;
> > +	}
> > +
> > +	pcdev = kzalloc(sizeof(*pcdev), GFP_KERNEL);
> > +	if (!pcdev) {
> > +		dev_err(&pdev->dev, "Could not allocate pcdev\n");
> > +		err = -ENOMEM;
> > +		goto exit;
> > +	}
> > +
> > +	pcdev->clk_csi = clk_get(&pdev->dev, NULL);
> > +	if (IS_ERR(pcdev->clk_csi)) {
> > +		err = PTR_ERR(pcdev->clk_csi);
> > +		goto exit_kfree;
> > +	}
> > +
> > +	dev_info(&pdev->dev, "Camera clock frequency: %ld\n",
> > +			clk_get_rate(pcdev->clk_csi));
> 
> nor is this. If you really want to see a kernel message on probe, try to 
> come up with something optimistically-informative:)
> 
> > +
> > +	/* Initialize DMA */
> > +	if (cpu_is_mx27()) {
> > +		err = mx27_camera_dma_init(pdev, pcdev);
> > +		if (err)
> > +			goto exit_clk_put;
> > +	}
> > +
> > +	pcdev->res_csi = res_csi;
> > +	pcdev->pdata = pdev->dev.platform_data;
> > +	if (pcdev->pdata) {
> > +		long rate;
> > +
> > +		pcdev->platform_flags = pcdev->pdata->flags;
> > +
> > +		rate = clk_round_rate(pcdev->clk_csi, pcdev->pdata->clk * 2);
> > +		clk_set_rate(pcdev->clk_csi, rate);
> 
> At least check, that rate is positive, before calling set_rate. And if you 
> indeed want to continue with these two calls failing, maybe at least issue 
> a warning "failed to program frequency %u, continuing with current 
> frequency %u" or something similar.
> 
> > +	}
> > +
> > +	INIT_LIST_HEAD(&pcdev->capture);
> > +	INIT_LIST_HEAD(&pcdev->active_bufs);
> > +	spin_lock_init(&pcdev->lock);
> > +
> > +	/*
> > +	 * Request the regions.
> > +	 */
> > +	if (!request_mem_region(res_csi->start, resource_size(res_csi),
> > +				MX2_CAM_DRV_NAME)) {
> > +		err = -EBUSY;
> > +		goto exit_dma_free;
> > +	}
> > +
> > +	base_csi = ioremap(res_csi->start, resource_size(res_csi));
> > +	if (!base_csi) {
> > +		err = -ENOMEM;
> > +		goto exit_release;
> > +	}
> > +	pcdev->irq_csi = irq_csi;
> > +	pcdev->base_csi = base_csi;
> > +	pcdev->dev = &pdev->dev;
> > +
> > +	err = request_irq(pcdev->irq_csi, mx2_cam_irq_handler, 0,
> > +			MX2_CAM_DRV_NAME, pcdev);
> > +	if (err) {
> > +		dev_err(pcdev->dev, "Camera interrupt register failed \n");
> > +		goto exit_iounmap;
> > +	}
> > +
> > +	if (cpu_is_mx27()) {
> > +		/* EMMA support */
> > +		res_emma = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > +		irq_emma = platform_get_irq(pdev, 1);
> > +
> > +		if (res_emma && irq_emma) {
> 
> Same here, check for "(int)irq_emma >= 0"
> 
> > +			dev_info(&pdev->dev, "Using EMMA\n");
> > +			pcdev->use_emma = 1;
> > +			pcdev->res_emma = res_emma;
> > +			pcdev->irq_emma = irq_emma;
> > +			if (mx27_camera_emma_init(pcdev))
> > +				goto exit_free_irq;
> > +		}
> > +	}
> > +
> > +	pcdev->soc_host.drv_name	= MX2_CAM_DRV_NAME,
> > +	pcdev->soc_host.ops		= &mx2_soc_camera_host_ops,
> > +	pcdev->soc_host.priv		= pcdev;
> > +	pcdev->soc_host.v4l2_dev.dev	= &pdev->dev;
> > +	pcdev->soc_host.nr		= pdev->id;
> > +	err = soc_camera_host_register(&pcdev->soc_host);
> > +	if (err)
> > +		goto exit_free_emma;
> > +
> > +	return 0;
> > +
> > +exit_free_emma:
> > +	if (mx27_camera_emma(pcdev)) {
> > +		free_irq(pcdev->irq_emma, pcdev);
> > +		clk_disable(pcdev->clk_emma);
> > +		clk_put(pcdev->clk_emma);
> > +		iounmap(pcdev->base_emma);
> > +		release_mem_region(res_emma->start, resource_size(res_emma));
> > +	}
> > +exit_free_irq:
> > +	free_irq(pcdev->irq_csi, pcdev);
> > +exit_iounmap:
> > +	iounmap(base_csi);
> > +exit_release:
> > +	release_mem_region(res_csi->start, resource_size(res_csi));
> > +exit_dma_free:
> > +	if (cpu_is_mx27())
> > +		imx_dma_free(pcdev->dma);
> > +exit_clk_put:
> > +	clk_put(pcdev->clk_csi);
> > +exit_kfree:
> > +	kfree(pcdev);
> > +exit:
> > +	return err;
> > +}
> > +
> > +static int __devexit mx2_camera_remove(struct platform_device *pdev)
> > +{
> > +	struct soc_camera_host *soc_host = to_soc_camera_host(&pdev->dev);
> > +	struct mx2_camera_dev *pcdev = container_of(soc_host,
> > +			struct mx2_camera_dev, soc_host);
> > +	struct resource *res;
> > +
> > +	clk_put(pcdev->clk_csi);
> > +	if (cpu_is_mx27())
> > +		imx_dma_free(pcdev->dma);
> > +	free_irq(pcdev->irq_csi, pcdev);
> > +	if (mx27_camera_emma(pcdev))
> > +		free_irq(pcdev->irq_emma, pcdev);
> > +
> > +	soc_camera_host_unregister(&pcdev->soc_host);
> > +
> > +	iounmap(pcdev->base_csi);
> > +
> > +	if (mx27_camera_emma(pcdev)) {
> > +		clk_disable(pcdev->clk_emma);
> > +		clk_put(pcdev->clk_emma);
> > +		iounmap(pcdev->base_emma);
> > +		res = pcdev->res_emma;
> > +		release_mem_region(res->start, resource_size(res));
> > +	}
> > +
> > +	res = pcdev->res_csi;
> > +	release_mem_region(res->start, resource_size(res));
> > +
> > +	kfree(pcdev);
> > +
> > +	dev_info(&pdev->dev, "MX2 Camera driver unloaded\n");
> > +
> > +	return 0;
> > +}
> > +
> > +static struct platform_driver mx2_camera_driver = {
> > +	.driver 	= {
> > +		.name	= MX2_CAM_DRV_NAME,
> > +	},
> > +	.probe		= mx2_camera_probe,
> > +	.remove		= __exit_p(mx2_camera_remove),
> 
> "__devexit_p"
> 
> > +};
> > +
> > +
> > +static int __devinit mx2_camera_init(void)
> 
> No, this should be "__init"
> 
> > +{
> > +	return platform_driver_register(&mx2_camera_driver);
> 
> Since this hardware is not hot-pluggable, you might want to just use 
> platform_driver_probe().
> 
> > +}
> > +
> > +static void __exit mx2_camera_exit(void)
> > +{
> > +	return platform_driver_unregister(&mx2_camera_driver);
> > +}
> > +
> > +module_init(mx2_camera_init);
> > +module_exit(mx2_camera_exit);
> > +
> > +MODULE_DESCRIPTION("i.MX27/i.MX25 SoC Camera Host driver");
> > +MODULE_AUTHOR("Sascha Hauer <[email-masked]>");
> > +MODULE_LICENSE("GPL");
> > -- 
> > 1.7.0
> 
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |