* [PATCH 0/3] Driver for the i.MX2x CMOS Sensor Interface
@ 2010-05-06 13:09 Baruch Siach
2010-05-06 13:09 ` [PATCH 1/3] mx2_camera: Add soc_camera support for i.MX25/i.MX27 Baruch Siach
` (3 more replies)
0 siblings, 4 replies; 16+ messages in thread
From: Baruch Siach @ 2010-05-06 13:09 UTC (permalink / raw)
To: linux-media; +Cc: linux-arm-kernel, Sascha Hauer, Baruch Siach
This series contains a soc_camera driver for the i.MX25/i.MX27 CSI device, and
platform code for the i.MX25 and i.MX27 chips. This driver is based on a driver
for i.MX27 CSI from Sascha Hauer, that Alan Carvalho de Assis has posted in
linux-media last December[1]. Since all I have is a i.MX25 PDK paltform I can't
test the mx27 specific code. Testers and comment are welcome.
[1] https://patchwork.kernel.org/patch/67636/
Baruch Siach (3):
mx2_camera: Add soc_camera support for i.MX25/i.MX27
mx27: add support for the CSI device
mx25: add support for the CSI device
arch/arm/mach-mx2/clock_imx27.c | 2 +-
arch/arm/mach-mx2/devices.c | 31 +
arch/arm/mach-mx2/devices.h | 1 +
arch/arm/mach-mx25/clock.c | 14 +-
arch/arm/mach-mx25/devices.c | 22 +
arch/arm/mach-mx25/devices.h | 1 +
arch/arm/plat-mxc/include/mach/memory.h | 4 +-
arch/arm/plat-mxc/include/mach/mx25.h | 2 +
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 ++++++++++++++++++++++++++++++
12 files changed, 1524 insertions(+), 5 deletions(-)
create mode 100644 arch/arm/plat-mxc/include/mach/mx2_cam.h
create mode 100644 drivers/media/video/mx2_camera.c
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH 1/3] mx2_camera: Add soc_camera support for i.MX25/i.MX27 2010-05-06 13:09 [PATCH 0/3] Driver for the i.MX2x CMOS Sensor Interface Baruch Siach @ 2010-05-06 13:09 ` Baruch Siach 2010-05-12 19:02 ` Guennadi Liakhovetski 2010-05-21 7:19 ` Sascha Hauer 2010-05-06 13:09 ` [PATCH 2/3] mx27: add support for the CSI device Baruch Siach ` (2 subsequent siblings) 3 siblings, 2 replies; 16+ messages in thread From: Baruch Siach @ 2010-05-06 13:09 UTC (permalink / raw) To: linux-media; +Cc: linux-arm-kernel, Sascha Hauer, Baruch Siach 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 <baruch@tkos.co.il> --- 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 <s.hauer@pengutronix.de> + Copyright (C) 2010, Baruch Siach <baruch@tkos.co.il> + + 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. +*/ + +#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; +}; + +#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 + 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 + * + * 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> +#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? */ +#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; + + 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__); + +#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; + 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 */ +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); + } + pcdev->discard_buffer = 0; + + 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); + 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) + 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)--; + + 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()); + + 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); + 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 */ + memset((void *)vb->baddr, 0xaa, vb->bsize); +#endif + + 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; + + 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) { + writel(videobuf_to_dma_contig(vb), + pcdev->base_csi + CSIDMASA_FB1); + pcdev->fb1_active = buf; + dma_inten |= CSICR1_FB1_DMA_INTEN; + } 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; + } + + 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; + + 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) + + +static int mx27_camera_emma_prp_reset(struct mx2_camera_dev *pcdev) +{ + unsigned int cntl; + + 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; +} + +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 + * DMA Engine. No color space consversion at the moment. + * We adjust incoming and outgoing pixelformat to rgb16 + * and adjust the bytesperline accordingly. + */ + 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); + + /* 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); +} + +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); + + /* 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 ;) + */ + 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); + 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; +} + +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); + 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); + + 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) +{ + 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); + + 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; + + 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) { + 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); + if (status & (1 << 7)) { + uint32_t cntl; + cntl = readl(pcdev->base_emma + PRP_CNTL); + writel(cntl & ~PRP_CNTL_CH1EN, pcdev->base_emma + PRP_CNTL); + writel(cntl, pcdev->base_emma + PRP_CNTL); + } + + writel(status, pcdev->base_emma + PRP_INTRSTATUS); + + return IRQ_HANDLED; +} + +static int mx27_camera_emma_init(struct mx2_camera_dev *pcdev) +{ + 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) +{ + 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"); + + res_csi = platform_get_resource(pdev, IORESOURCE_MEM, 0); + irq_csi = platform_get_irq(pdev, 0); + if (!res_csi || !irq_csi) { + 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)); + + /* 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); + } + + 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) { + 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), +}; + + +static int __devinit mx2_camera_init(void) +{ + return platform_driver_register(&mx2_camera_driver); +} + +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 <sha@pengutronix.de>"); +MODULE_LICENSE("GPL"); -- 1.7.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] mx2_camera: Add soc_camera support for i.MX25/i.MX27 2010-05-06 13:09 ` [PATCH 1/3] mx2_camera: Add soc_camera support for i.MX25/i.MX27 Baruch Siach @ 2010-05-12 19:02 ` Guennadi Liakhovetski 2010-05-13 5:21 ` Baruch Siach 2010-05-17 7:27 ` Sascha Hauer 2010-05-21 7:19 ` Sascha Hauer 1 sibling, 2 replies; 16+ messages in thread From: Guennadi Liakhovetski @ 2010-05-12 19:02 UTC (permalink / raw) To: Baruch Siach; +Cc: linux-media, linux-arm-kernel, Sascha Hauer 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. 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. 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 <baruch@tkos.co.il> > --- > 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 <s.hauer@pengutronix.de> > + Copyright (C) 2010, Baruch Siach <baruch@tkos.co.il> > + > + 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. > + 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;) > + * 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? > + } > + > + 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 <sha@pengutronix.de>"); > +MODULE_LICENSE("GPL"); > -- > 1.7.0 Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] mx2_camera: Add soc_camera support for i.MX25/i.MX27 2010-05-12 19:02 ` Guennadi Liakhovetski @ 2010-05-13 5:21 ` Baruch Siach 2010-05-17 7:27 ` Sascha Hauer 1 sibling, 0 replies; 16+ messages in thread From: Baruch Siach @ 2010-05-13 5:21 UTC (permalink / raw) To: Guennadi Liakhovetski; +Cc: linux-media, linux-arm-kernel, Sascha Hauer Hi Guennadi, On Wed, May 12, 2010 at 09:02:29PM +0200, Guennadi Liakhovetski wrote: > 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. 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. Thank you very much for your detailed review and informative comments. I'll fix the problems that you've found, and post up updated patch after getting Sascha comments on the mx27 specific code. baruch > 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 <baruch@tkos.co.il> > > --- -- ~. .~ Tk Open Systems =}------------------------------------------------ooO--U--Ooo------------{= - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il - ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] mx2_camera: Add soc_camera support for i.MX25/i.MX27 2010-05-12 19:02 ` Guennadi Liakhovetski 2010-05-13 5:21 ` Baruch Siach @ 2010-05-17 7:27 ` Sascha Hauer 2010-05-17 13:58 ` Baruch Siach 1 sibling, 1 reply; 16+ messages in thread From: Sascha Hauer @ 2010-05-17 7:27 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: Baruch Siach, linux-media, linux-arm-kernel, Sascha Hauer 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 <baruch@tkos.co.il> > > --- > > 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 <s.hauer@pengutronix.de> > > + Copyright (C) 2010, Baruch Siach <baruch@tkos.co.il> > > + > > + 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 <sha@pengutronix.de>"); > > +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 | ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] mx2_camera: Add soc_camera support for i.MX25/i.MX27 2010-05-17 7:27 ` Sascha Hauer @ 2010-05-17 13:58 ` Baruch Siach 2010-05-19 8:55 ` Sascha Hauer 0 siblings, 1 reply; 16+ messages in thread From: Baruch Siach @ 2010-05-17 13:58 UTC (permalink / raw) To: Sascha Hauer Cc: Guennadi Liakhovetski, linux-media, linux-arm-kernel, Sascha Hauer Hi Sascha, Thanks for your comments. On Mon, May 17, 2010 at 09:27:20AM +0200, Sascha Hauer wrote: > 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). So can I remove the non EMMA code from this driver? This will simplify the code quite a bit. [snip] > > > +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; > > > +} Can you shed some light on this? Thanks, baruch -- ~. .~ Tk Open Systems =}------------------------------------------------ooO--U--Ooo------------{= - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il - ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] mx2_camera: Add soc_camera support for i.MX25/i.MX27 2010-05-17 13:58 ` Baruch Siach @ 2010-05-19 8:55 ` Sascha Hauer 0 siblings, 0 replies; 16+ messages in thread From: Sascha Hauer @ 2010-05-19 8:55 UTC (permalink / raw) To: Baruch Siach Cc: Guennadi Liakhovetski, linux-media, linux-arm-kernel, Sascha Hauer On Mon, May 17, 2010 at 04:58:00PM +0300, Baruch Siach wrote: > Hi Sascha, > > Thanks for your comments. > > On Mon, May 17, 2010 at 09:27:20AM +0200, Sascha Hauer wrote: > > 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). > > So can I remove the non EMMA code from this driver? This will simplify the > code quite a bit. Please don't. I had a talk with our customer and it seems I can put some effort into the i.MX27 part. That's good news because I also want this driver mainline. Sascha -- 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 | ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] mx2_camera: Add soc_camera support for i.MX25/i.MX27 2010-05-06 13:09 ` [PATCH 1/3] mx2_camera: Add soc_camera support for i.MX25/i.MX27 Baruch Siach 2010-05-12 19:02 ` Guennadi Liakhovetski @ 2010-05-21 7:19 ` Sascha Hauer 1 sibling, 0 replies; 16+ messages in thread From: Sascha Hauer @ 2010-05-21 7:19 UTC (permalink / raw) To: Baruch Siach; +Cc: linux-media, linux-arm-kernel, Sascha Hauer On Thu, May 06, 2010 at 04:09:39PM +0300, 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 <baruch@tkos.co.il> > --- > 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 > On i.MX27 in DMA mode you pass the kernel virtual address to the DMA engine. Should be a physical address, so please amend the following: >From 8a1e1ac8b2448a5c83ed933e427cccd00d470308 Mon Sep 17 00:00:00 2001 From: Sascha Hauer <s.hauer@pengutronix.de> Date: Fri, 21 May 2010 09:15:22 +0200 Subject: [PATCH 2/2] mx2_camera: pass physical address to DMA engine Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> --- drivers/media/video/mx2_camera.c | 7 +++++-- 1 files changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/media/video/mx2_camera.c b/drivers/media/video/mx2_camera.c index 5d6fb08..6436968 100644 --- a/drivers/media/video/mx2_camera.c +++ b/drivers/media/video/mx2_camera.c @@ -216,6 +216,7 @@ struct mx2_camera_dev { unsigned int irq_csi, irq_emma; void __iomem *base_csi, *base_emma; + unsigned long base_dma; struct mx2_camera_platform_data *pdata; struct resource *res_csi, *res_emma; @@ -550,7 +551,7 @@ static void mx2_videobuf_queue(struct videobuf_queue *vq, if (!pcdev->active) { ret = imx_dma_setup_single(pcdev->dma, videobuf_to_dma_contig(vb), vb->size, - (u32)pcdev->base_csi + 0x10, + (u32)pcdev->base_dma + 0x10, DMA_MODE_READ); if (ret) { vb->state = VIDEOBUF_ERROR; @@ -976,7 +977,8 @@ static void mx27_camera_frame_done(struct mx2_camera_dev *pcdev, int state) 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); + vb->size, (u32)pcdev->base_dma + 0x10, DMA_MODE_READ); + if (ret) { vb->state = VIDEOBUF_ERROR; wake_up(&vb->done); @@ -1273,6 +1275,7 @@ static int mx2_camera_probe(struct platform_device *pdev) } pcdev->irq_csi = irq_csi; pcdev->base_csi = base_csi; + pcdev->base_dma = res_csi->start; pcdev->dev = &pdev->dev; err = request_irq(pcdev->irq_csi, mx2_cam_irq_handler, 0, -- 1.7.0 -- 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 | ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/3] mx27: add support for the CSI device 2010-05-06 13:09 [PATCH 0/3] Driver for the i.MX2x CMOS Sensor Interface Baruch Siach 2010-05-06 13:09 ` [PATCH 1/3] mx2_camera: Add soc_camera support for i.MX25/i.MX27 Baruch Siach @ 2010-05-06 13:09 ` Baruch Siach 2010-05-21 7:17 ` Sascha Hauer 2010-05-06 13:09 ` [PATCH 3/3] mx25: " Baruch Siach 2010-05-21 7:20 ` [PATCH 0/3] Driver for the i.MX2x CMOS Sensor Interface Sascha Hauer 3 siblings, 1 reply; 16+ messages in thread From: Baruch Siach @ 2010-05-06 13:09 UTC (permalink / raw) To: linux-media; +Cc: linux-arm-kernel, Sascha Hauer, Baruch Siach Signed-off-by: Baruch Siach <baruch@tkos.co.il> --- arch/arm/mach-mx2/clock_imx27.c | 2 +- arch/arm/mach-mx2/devices.c | 31 +++++++++++++++++++++++++++++++ arch/arm/mach-mx2/devices.h | 1 + 3 files changed, 33 insertions(+), 1 deletions(-) diff --git a/arch/arm/mach-mx2/clock_imx27.c b/arch/arm/mach-mx2/clock_imx27.c index 0f0823c..5a1aa15 100644 --- a/arch/arm/mach-mx2/clock_imx27.c +++ b/arch/arm/mach-mx2/clock_imx27.c @@ -644,7 +644,7 @@ static struct clk_lookup lookups[] = { _REGISTER_CLOCK("spi_imx.1", NULL, cspi2_clk) _REGISTER_CLOCK("spi_imx.2", NULL, cspi3_clk) _REGISTER_CLOCK("imx-fb.0", NULL, lcdc_clk) - _REGISTER_CLOCK(NULL, "csi", csi_clk) + _REGISTER_CLOCK("mx2-camera.0", NULL, csi_clk) _REGISTER_CLOCK("fsl-usb2-udc", "usb", usb_clk) _REGISTER_CLOCK("fsl-usb2-udc", "usb_ahb", usb_clk1) _REGISTER_CLOCK("mxc-ehci.0", "usb", usb_clk) diff --git a/arch/arm/mach-mx2/devices.c b/arch/arm/mach-mx2/devices.c index b91e412..de501ac 100644 --- a/arch/arm/mach-mx2/devices.c +++ b/arch/arm/mach-mx2/devices.c @@ -40,6 +40,37 @@ #include "devices.h" +#ifdef CONFIG_MACH_MX27 +static struct resource mx27_camera_resources[] = { + { + .start = CSI_BASE_ADDR, + .end = CSI_BASE_ADDR + 0x1f, + .flags = IORESOURCE_MEM, + }, { + .start = EMMA_PRP_BASE_ADDR, + .end = EMMA_PRP_BASE_ADDR + 0x1f, + .flags = IORESOURCE_MEM, + }, { + .start = MXC_INT_CSI, + .end = MXC_INT_CSI, + .flags = IORESOURCE_IRQ, + },{ + .start = MXC_INT_EMMAPRP, + .end = MXC_INT_EMMAPRP, + .flags = IORESOURCE_IRQ, + }, +}; +struct platform_device mx27_camera_device = { + .name = "mx2-camera", + .id = 0, + .num_resources = ARRAY_SIZE(mx27_camera_resources), + .resource = mx27_camera_resources, + .dev = { + .coherent_dma_mask = 0xffffffff, + }, +}; +#endif + /* * SPI master controller * diff --git a/arch/arm/mach-mx2/devices.h b/arch/arm/mach-mx2/devices.h index 84ed513..8bdf018 100644 --- a/arch/arm/mach-mx2/devices.h +++ b/arch/arm/mach-mx2/devices.h @@ -29,6 +29,7 @@ extern struct platform_device mxc_i2c_device1; extern struct platform_device mxc_sdhc_device0; extern struct platform_device mxc_sdhc_device1; extern struct platform_device mxc_otg_udc_device; +extern struct platform_device mx27_camera_device; extern struct platform_device mxc_otg_host; extern struct platform_device mxc_usbh1; extern struct platform_device mxc_usbh2; -- 1.7.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] mx27: add support for the CSI device 2010-05-06 13:09 ` [PATCH 2/3] mx27: add support for the CSI device Baruch Siach @ 2010-05-21 7:17 ` Sascha Hauer 0 siblings, 0 replies; 16+ messages in thread From: Sascha Hauer @ 2010-05-21 7:17 UTC (permalink / raw) To: Baruch Siach; +Cc: linux-media, linux-arm-kernel, Sascha Hauer On Thu, May 06, 2010 at 04:09:40PM +0300, Baruch Siach wrote: > Signed-off-by: Baruch Siach <baruch@tkos.co.il> > --- > arch/arm/mach-mx2/clock_imx27.c | 2 +- > arch/arm/mach-mx2/devices.c | 31 +++++++++++++++++++++++++++++++ > arch/arm/mach-mx2/devices.h | 1 + > 3 files changed, 33 insertions(+), 1 deletions(-) > > diff --git a/arch/arm/mach-mx2/clock_imx27.c b/arch/arm/mach-mx2/clock_imx27.c > index 0f0823c..5a1aa15 100644 > --- a/arch/arm/mach-mx2/clock_imx27.c > +++ b/arch/arm/mach-mx2/clock_imx27.c > @@ -644,7 +644,7 @@ static struct clk_lookup lookups[] = { > _REGISTER_CLOCK("spi_imx.1", NULL, cspi2_clk) > _REGISTER_CLOCK("spi_imx.2", NULL, cspi3_clk) > _REGISTER_CLOCK("imx-fb.0", NULL, lcdc_clk) > - _REGISTER_CLOCK(NULL, "csi", csi_clk) > + _REGISTER_CLOCK("mx2-camera.0", NULL, csi_clk) > _REGISTER_CLOCK("fsl-usb2-udc", "usb", usb_clk) > _REGISTER_CLOCK("fsl-usb2-udc", "usb_ahb", usb_clk1) > _REGISTER_CLOCK("mxc-ehci.0", "usb", usb_clk) > diff --git a/arch/arm/mach-mx2/devices.c b/arch/arm/mach-mx2/devices.c > index b91e412..de501ac 100644 > --- a/arch/arm/mach-mx2/devices.c > +++ b/arch/arm/mach-mx2/devices.c > @@ -40,6 +40,37 @@ > > #include "devices.h" > > +#ifdef CONFIG_MACH_MX27 > +static struct resource mx27_camera_resources[] = { > + { > + .start = CSI_BASE_ADDR, > + .end = CSI_BASE_ADDR + 0x1f, > + .flags = IORESOURCE_MEM, > + }, { > + .start = EMMA_PRP_BASE_ADDR, > + .end = EMMA_PRP_BASE_ADDR + 0x1f, > + .flags = IORESOURCE_MEM, > + }, { > + .start = MXC_INT_CSI, > + .end = MXC_INT_CSI, > + .flags = IORESOURCE_IRQ, > + },{ > + .start = MXC_INT_EMMAPRP, > + .end = MXC_INT_EMMAPRP, > + .flags = IORESOURCE_IRQ, > + }, > +}; > +struct platform_device mx27_camera_device = { > + .name = "mx2-camera", > + .id = 0, > + .num_resources = ARRAY_SIZE(mx27_camera_resources), > + .resource = mx27_camera_resources, > + .dev = { > + .coherent_dma_mask = 0xffffffff, > + }, > +}; > +#endif > + > /* > * SPI master controller > * > diff --git a/arch/arm/mach-mx2/devices.h b/arch/arm/mach-mx2/devices.h > index 84ed513..8bdf018 100644 > --- a/arch/arm/mach-mx2/devices.h > +++ b/arch/arm/mach-mx2/devices.h > @@ -29,6 +29,7 @@ extern struct platform_device mxc_i2c_device1; > extern struct platform_device mxc_sdhc_device0; > extern struct platform_device mxc_sdhc_device1; > extern struct platform_device mxc_otg_udc_device; > +extern struct platform_device mx27_camera_device; > extern struct platform_device mxc_otg_host; > extern struct platform_device mxc_usbh1; > extern struct platform_device mxc_usbh2; > -- > 1.7.0 > > Please amend the following to this patch to make it compile on i.MX27: >From f1dc7e4c35ea0847e5527d9db50b6343c655de8c Mon Sep 17 00:00:00 2001 From: Sascha Hauer <s.hauer@pengutronix.de> Date: Thu, 20 May 2010 13:36:11 +0200 Subject: [PATCH 1/2] mx27 devices: Fix CSI/EMMA base addresses Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> --- arch/arm/mach-mx2/devices.c | 16 ++++++++-------- 1 files changed, 8 insertions(+), 8 deletions(-) diff --git a/arch/arm/mach-mx2/devices.c b/arch/arm/mach-mx2/devices.c index de501ac..6a49c79 100644 --- a/arch/arm/mach-mx2/devices.c +++ b/arch/arm/mach-mx2/devices.c @@ -43,20 +43,20 @@ #ifdef CONFIG_MACH_MX27 static struct resource mx27_camera_resources[] = { { - .start = CSI_BASE_ADDR, - .end = CSI_BASE_ADDR + 0x1f, + .start = MX27_CSI_BASE_ADDR, + .end = MX27_CSI_BASE_ADDR + 0x1f, .flags = IORESOURCE_MEM, }, { - .start = EMMA_PRP_BASE_ADDR, - .end = EMMA_PRP_BASE_ADDR + 0x1f, + .start = MX27_EMMA_PRP_BASE_ADDR, + .end = MX27_EMMA_PRP_BASE_ADDR + 0x1f, .flags = IORESOURCE_MEM, }, { - .start = MXC_INT_CSI, - .end = MXC_INT_CSI, + .start = MX27_INT_CSI, + .end = MX27_INT_CSI, .flags = IORESOURCE_IRQ, },{ - .start = MXC_INT_EMMAPRP, - .end = MXC_INT_EMMAPRP, + .start = MX27_INT_EMMAPRP, + .end = MX27_INT_EMMAPRP, .flags = IORESOURCE_IRQ, }, }; -- 1.7.0 -- 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 | ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 3/3] mx25: add support for the CSI device 2010-05-06 13:09 [PATCH 0/3] Driver for the i.MX2x CMOS Sensor Interface Baruch Siach 2010-05-06 13:09 ` [PATCH 1/3] mx2_camera: Add soc_camera support for i.MX25/i.MX27 Baruch Siach 2010-05-06 13:09 ` [PATCH 2/3] mx27: add support for the CSI device Baruch Siach @ 2010-05-06 13:09 ` Baruch Siach 2010-05-12 19:11 ` Guennadi Liakhovetski 2010-05-21 7:20 ` [PATCH 0/3] Driver for the i.MX2x CMOS Sensor Interface Sascha Hauer 3 siblings, 1 reply; 16+ messages in thread From: Baruch Siach @ 2010-05-06 13:09 UTC (permalink / raw) To: linux-media; +Cc: linux-arm-kernel, Sascha Hauer, Baruch Siach Signed-off-by: Baruch Siach <baruch@tkos.co.il> --- arch/arm/mach-mx25/clock.c | 14 ++++++++++++-- arch/arm/mach-mx25/devices.c | 22 ++++++++++++++++++++++ arch/arm/mach-mx25/devices.h | 1 + arch/arm/plat-mxc/include/mach/mx25.h | 2 ++ 4 files changed, 37 insertions(+), 2 deletions(-) diff --git a/arch/arm/mach-mx25/clock.c b/arch/arm/mach-mx25/clock.c index 1550149..7a98d18 100644 --- a/arch/arm/mach-mx25/clock.c +++ b/arch/arm/mach-mx25/clock.c @@ -129,6 +129,11 @@ static unsigned long get_rate_lcdc(struct clk *clk) return get_rate_per(7); } +static unsigned long get_rate_csi(struct clk *clk) +{ + return get_rate_per(0); +} + static unsigned long get_rate_otg(struct clk *clk) { return 48000000; /* FIXME */ @@ -174,6 +179,8 @@ DEFINE_CLOCK(cspi3_clk, 0, CCM_CGCR1, 7, get_rate_ipg, NULL, NULL); DEFINE_CLOCK(fec_ahb_clk, 0, CCM_CGCR0, 23, NULL, NULL, NULL); DEFINE_CLOCK(lcdc_ahb_clk, 0, CCM_CGCR0, 24, NULL, NULL, NULL); DEFINE_CLOCK(lcdc_per_clk, 0, CCM_CGCR0, 7, NULL, NULL, &lcdc_ahb_clk); +DEFINE_CLOCK(csi_ahb_clk, 0, CCM_CGCR0, 18, get_rate_csi, NULL, NULL); +DEFINE_CLOCK(csi_per_clk, 0, CCM_CGCR0, 0, get_rate_csi, NULL, &csi_ahb_clk); DEFINE_CLOCK(uart1_clk, 0, CCM_CGCR2, 14, get_rate_uart, NULL, &uart_per_clk); DEFINE_CLOCK(uart2_clk, 0, CCM_CGCR2, 15, get_rate_uart, NULL, &uart_per_clk); DEFINE_CLOCK(uart3_clk, 0, CCM_CGCR2, 16, get_rate_uart, NULL, &uart_per_clk); @@ -191,6 +198,7 @@ DEFINE_CLOCK(i2c_clk, 0, CCM_CGCR0, 6, get_rate_i2c, NULL, NULL); DEFINE_CLOCK(fec_clk, 0, CCM_CGCR1, 15, get_rate_ipg, NULL, &fec_ahb_clk); DEFINE_CLOCK(dryice_clk, 0, CCM_CGCR1, 8, get_rate_ipg, NULL, NULL); DEFINE_CLOCK(lcdc_clk, 0, CCM_CGCR1, 29, get_rate_lcdc, NULL, &lcdc_per_clk); +DEFINE_CLOCK(csi_clk, 0, CCM_CGCR1, 4, get_rate_csi, NULL, &csi_per_clk); #define _REGISTER_CLOCK(d, n, c) \ { \ @@ -225,6 +233,7 @@ static struct clk_lookup lookups[] = { _REGISTER_CLOCK("fec.0", NULL, fec_clk) _REGISTER_CLOCK("imxdi_rtc.0", NULL, dryice_clk) _REGISTER_CLOCK("imx-fb.0", NULL, lcdc_clk) + _REGISTER_CLOCK("mx2-camera.0", NULL, csi_clk) }; int __init mx25_clocks_init(void) @@ -239,8 +248,9 @@ int __init mx25_clocks_init(void) __raw_writel((0xf << 16) | (3 << 26), CRM_BASE + CCM_CGCR1); __raw_writel((1 << 5), CRM_BASE + CCM_CGCR2); - /* Clock source for lcdc is upll */ - __raw_writel(__raw_readl(CRM_BASE+0x64) | (1 << 7), CRM_BASE + 0x64); + /* Clock source for lcdc and csi is upll */ + __raw_writel(__raw_readl(CRM_BASE+0x64) | (1 << 7) | (1 << 0), + CRM_BASE + 0x64); mxc_timer_init(&gpt_clk, MX25_IO_ADDRESS(MX25_GPT1_BASE_ADDR), 54); diff --git a/arch/arm/mach-mx25/devices.c b/arch/arm/mach-mx25/devices.c index 3f4b8a0..bc6d189 100644 --- a/arch/arm/mach-mx25/devices.c +++ b/arch/arm/mach-mx25/devices.c @@ -500,3 +500,25 @@ struct platform_device mx25_fb_device = { .coherent_dma_mask = 0xFFFFFFFF, }, }; + +static struct resource mx25_csi_resources[] = { + { + .start = MX25_CSI_BASE_ADDR, + .end = MX25_CSI_BASE_ADDR + 0xfff, + .flags = IORESOURCE_MEM, + }, + { + .start = MX25_INT_CSI, + .flags = IORESOURCE_IRQ + }, +}; + +struct platform_device mx25_csi_device = { + .name = "mx2-camera", + .id = 0, + .num_resources = ARRAY_SIZE(mx25_csi_resources), + .resource = mx25_csi_resources, + .dev = { + .coherent_dma_mask = 0xFFFFFFFF, + }, +}; diff --git a/arch/arm/mach-mx25/devices.h b/arch/arm/mach-mx25/devices.h index 39560e1..1e80ac2 100644 --- a/arch/arm/mach-mx25/devices.h +++ b/arch/arm/mach-mx25/devices.h @@ -21,3 +21,4 @@ extern struct platform_device mx25_fec_device; extern struct platform_device mxc_nand_device; extern struct platform_device mx25_rtc_device; extern struct platform_device mx25_fb_device; +extern struct platform_device mx25_csi_device; diff --git a/arch/arm/plat-mxc/include/mach/mx25.h b/arch/arm/plat-mxc/include/mach/mx25.h index 4eb6e33..5e6a8b5 100644 --- a/arch/arm/plat-mxc/include/mach/mx25.h +++ b/arch/arm/plat-mxc/include/mach/mx25.h @@ -34,11 +34,13 @@ #define MX25_NFC_BASE_ADDR 0xbb000000 #define MX25_DRYICE_BASE_ADDR 0x53ffc000 #define MX25_LCDC_BASE_ADDR 0x53fbc000 +#define MX25_CSI_BASE_ADDR 0x53ff8000 #define MX25_INT_DRYICE 25 #define MX25_INT_FEC 57 #define MX25_INT_NANDFC 33 #define MX25_INT_LCDC 39 +#define MX25_INT_CSI 17 #if defined(IMX_NEEDS_DEPRECATED_SYMBOLS) #define UART1_BASE_ADDR MX25_UART1_BASE_ADDR -- 1.7.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] mx25: add support for the CSI device 2010-05-06 13:09 ` [PATCH 3/3] mx25: " Baruch Siach @ 2010-05-12 19:11 ` Guennadi Liakhovetski 0 siblings, 0 replies; 16+ messages in thread From: Guennadi Liakhovetski @ 2010-05-12 19:11 UTC (permalink / raw) To: Baruch Siach; +Cc: linux-media, linux-arm-kernel, Sascha Hauer On Thu, 6 May 2010, Baruch Siach wrote: > Signed-off-by: Baruch Siach <baruch@tkos.co.il> > --- > arch/arm/mach-mx25/clock.c | 14 ++++++++++++-- > arch/arm/mach-mx25/devices.c | 22 ++++++++++++++++++++++ > arch/arm/mach-mx25/devices.h | 1 + > arch/arm/plat-mxc/include/mach/mx25.h | 2 ++ > 4 files changed, 37 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/mach-mx25/clock.c b/arch/arm/mach-mx25/clock.c > index 1550149..7a98d18 100644 > --- a/arch/arm/mach-mx25/clock.c > +++ b/arch/arm/mach-mx25/clock.c > @@ -129,6 +129,11 @@ static unsigned long get_rate_lcdc(struct clk *clk) > return get_rate_per(7); > } > > +static unsigned long get_rate_csi(struct clk *clk) > +{ > + return get_rate_per(0); > +} > + > static unsigned long get_rate_otg(struct clk *clk) > { > return 48000000; /* FIXME */ > @@ -174,6 +179,8 @@ DEFINE_CLOCK(cspi3_clk, 0, CCM_CGCR1, 7, get_rate_ipg, NULL, NULL); > DEFINE_CLOCK(fec_ahb_clk, 0, CCM_CGCR0, 23, NULL, NULL, NULL); > DEFINE_CLOCK(lcdc_ahb_clk, 0, CCM_CGCR0, 24, NULL, NULL, NULL); > DEFINE_CLOCK(lcdc_per_clk, 0, CCM_CGCR0, 7, NULL, NULL, &lcdc_ahb_clk); > +DEFINE_CLOCK(csi_ahb_clk, 0, CCM_CGCR0, 18, get_rate_csi, NULL, NULL); > +DEFINE_CLOCK(csi_per_clk, 0, CCM_CGCR0, 0, get_rate_csi, NULL, &csi_ahb_clk); > DEFINE_CLOCK(uart1_clk, 0, CCM_CGCR2, 14, get_rate_uart, NULL, &uart_per_clk); > DEFINE_CLOCK(uart2_clk, 0, CCM_CGCR2, 15, get_rate_uart, NULL, &uart_per_clk); > DEFINE_CLOCK(uart3_clk, 0, CCM_CGCR2, 16, get_rate_uart, NULL, &uart_per_clk); > @@ -191,6 +198,7 @@ DEFINE_CLOCK(i2c_clk, 0, CCM_CGCR0, 6, get_rate_i2c, NULL, NULL); > DEFINE_CLOCK(fec_clk, 0, CCM_CGCR1, 15, get_rate_ipg, NULL, &fec_ahb_clk); > DEFINE_CLOCK(dryice_clk, 0, CCM_CGCR1, 8, get_rate_ipg, NULL, NULL); > DEFINE_CLOCK(lcdc_clk, 0, CCM_CGCR1, 29, get_rate_lcdc, NULL, &lcdc_per_clk); > +DEFINE_CLOCK(csi_clk, 0, CCM_CGCR1, 4, get_rate_csi, NULL, &csi_per_clk); > > #define _REGISTER_CLOCK(d, n, c) \ > { \ > @@ -225,6 +233,7 @@ static struct clk_lookup lookups[] = { > _REGISTER_CLOCK("fec.0", NULL, fec_clk) > _REGISTER_CLOCK("imxdi_rtc.0", NULL, dryice_clk) > _REGISTER_CLOCK("imx-fb.0", NULL, lcdc_clk) > + _REGISTER_CLOCK("mx2-camera.0", NULL, csi_clk) > }; > > int __init mx25_clocks_init(void) > @@ -239,8 +248,9 @@ int __init mx25_clocks_init(void) > __raw_writel((0xf << 16) | (3 << 26), CRM_BASE + CCM_CGCR1); > __raw_writel((1 << 5), CRM_BASE + CCM_CGCR2); > > - /* Clock source for lcdc is upll */ > - __raw_writel(__raw_readl(CRM_BASE+0x64) | (1 << 7), CRM_BASE + 0x64); > + /* Clock source for lcdc and csi is upll */ > + __raw_writel(__raw_readl(CRM_BASE+0x64) | (1 << 7) | (1 << 0), > + CRM_BASE + 0x64); > > mxc_timer_init(&gpt_clk, MX25_IO_ADDRESS(MX25_GPT1_BASE_ADDR), 54); > > diff --git a/arch/arm/mach-mx25/devices.c b/arch/arm/mach-mx25/devices.c > index 3f4b8a0..bc6d189 100644 > --- a/arch/arm/mach-mx25/devices.c > +++ b/arch/arm/mach-mx25/devices.c > @@ -500,3 +500,25 @@ struct platform_device mx25_fb_device = { > .coherent_dma_mask = 0xFFFFFFFF, > }, > }; > + > +static struct resource mx25_csi_resources[] = { > + { > + .start = MX25_CSI_BASE_ADDR, > + .end = MX25_CSI_BASE_ADDR + 0xfff, > + .flags = IORESOURCE_MEM, > + }, > + { > + .start = MX25_INT_CSI, > + .flags = IORESOURCE_IRQ Please, also specify .end (= .start). > + }, > +}; > + > +struct platform_device mx25_csi_device = { > + .name = "mx2-camera", > + .id = 0, > + .num_resources = ARRAY_SIZE(mx25_csi_resources), > + .resource = mx25_csi_resources, > + .dev = { > + .coherent_dma_mask = 0xFFFFFFFF, > + }, > +}; > diff --git a/arch/arm/mach-mx25/devices.h b/arch/arm/mach-mx25/devices.h > index 39560e1..1e80ac2 100644 > --- a/arch/arm/mach-mx25/devices.h > +++ b/arch/arm/mach-mx25/devices.h > @@ -21,3 +21,4 @@ extern struct platform_device mx25_fec_device; > extern struct platform_device mxc_nand_device; > extern struct platform_device mx25_rtc_device; > extern struct platform_device mx25_fb_device; > +extern struct platform_device mx25_csi_device; > diff --git a/arch/arm/plat-mxc/include/mach/mx25.h b/arch/arm/plat-mxc/include/mach/mx25.h > index 4eb6e33..5e6a8b5 100644 > --- a/arch/arm/plat-mxc/include/mach/mx25.h > +++ b/arch/arm/plat-mxc/include/mach/mx25.h > @@ -34,11 +34,13 @@ > #define MX25_NFC_BASE_ADDR 0xbb000000 > #define MX25_DRYICE_BASE_ADDR 0x53ffc000 > #define MX25_LCDC_BASE_ADDR 0x53fbc000 > +#define MX25_CSI_BASE_ADDR 0x53ff8000 > > #define MX25_INT_DRYICE 25 > #define MX25_INT_FEC 57 > #define MX25_INT_NANDFC 33 > #define MX25_INT_LCDC 39 > +#define MX25_INT_CSI 17 > > #if defined(IMX_NEEDS_DEPRECATED_SYMBOLS) > #define UART1_BASE_ADDR MX25_UART1_BASE_ADDR > -- > 1.7.0 Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] Driver for the i.MX2x CMOS Sensor Interface 2010-05-06 13:09 [PATCH 0/3] Driver for the i.MX2x CMOS Sensor Interface Baruch Siach ` (2 preceding siblings ...) 2010-05-06 13:09 ` [PATCH 3/3] mx25: " Baruch Siach @ 2010-05-21 7:20 ` Sascha Hauer 2010-05-21 7:27 ` Baruch Siach 3 siblings, 1 reply; 16+ messages in thread From: Sascha Hauer @ 2010-05-21 7:20 UTC (permalink / raw) To: Baruch Siach; +Cc: linux-media, linux-arm-kernel, Sascha Hauer On Thu, May 06, 2010 at 04:09:38PM +0300, Baruch Siach wrote: > This series contains a soc_camera driver for the i.MX25/i.MX27 CSI device, and > platform code for the i.MX25 and i.MX27 chips. This driver is based on a driver > for i.MX27 CSI from Sascha Hauer, that Alan Carvalho de Assis has posted in > linux-media last December[1]. Since all I have is a i.MX25 PDK paltform I can't > test the mx27 specific code. Testers and comment are welcome. > > [1] https://patchwork.kernel.org/patch/67636/ > > Baruch Siach (3): > mx2_camera: Add soc_camera support for i.MX25/i.MX27 > mx27: add support for the CSI device > mx25: add support for the CSI device With the two additions I sent I can confirm this working on i.MX27, so no need to remove the related code. Sascha -- 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 | ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] Driver for the i.MX2x CMOS Sensor Interface 2010-05-21 7:20 ` [PATCH 0/3] Driver for the i.MX2x CMOS Sensor Interface Sascha Hauer @ 2010-05-21 7:27 ` Baruch Siach 2010-05-21 18:33 ` Guennadi Liakhovetski 0 siblings, 1 reply; 16+ messages in thread From: Baruch Siach @ 2010-05-21 7:27 UTC (permalink / raw) To: Sascha Hauer; +Cc: linux-media, linux-arm-kernel, Sascha Hauer Hi Sascha, On Fri, May 21, 2010 at 09:20:45AM +0200, Sascha Hauer wrote: > On Thu, May 06, 2010 at 04:09:38PM +0300, Baruch Siach wrote: > > This series contains a soc_camera driver for the i.MX25/i.MX27 CSI device, and > > platform code for the i.MX25 and i.MX27 chips. This driver is based on a driver > > for i.MX27 CSI from Sascha Hauer, that Alan Carvalho de Assis has posted in > > linux-media last December[1]. Since all I have is a i.MX25 PDK paltform I can't > > test the mx27 specific code. Testers and comment are welcome. > > > > [1] https://patchwork.kernel.org/patch/67636/ > > > > Baruch Siach (3): > > mx2_camera: Add soc_camera support for i.MX25/i.MX27 > > mx27: add support for the CSI device > > mx25: add support for the CSI device > > With the two additions I sent I can confirm this working on i.MX27, so > no need to remove the related code. Thanks. I'll add your patches to my queue and resend the series next week. baruch -- ~. .~ Tk Open Systems =}------------------------------------------------ooO--U--Ooo------------{= - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il - ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] Driver for the i.MX2x CMOS Sensor Interface 2010-05-21 7:27 ` Baruch Siach @ 2010-05-21 18:33 ` Guennadi Liakhovetski 2010-05-25 7:20 ` Sascha Hauer 0 siblings, 1 reply; 16+ messages in thread From: Guennadi Liakhovetski @ 2010-05-21 18:33 UTC (permalink / raw) To: Baruch Siach; +Cc: Sascha Hauer, Linux Media Mailing List, linux-arm-kernel On Fri, 21 May 2010, Baruch Siach wrote: > Hi Sascha, > > On Fri, May 21, 2010 at 09:20:45AM +0200, Sascha Hauer wrote: > > On Thu, May 06, 2010 at 04:09:38PM +0300, Baruch Siach wrote: > > > This series contains a soc_camera driver for the i.MX25/i.MX27 CSI device, and > > > platform code for the i.MX25 and i.MX27 chips. This driver is based on a driver > > > for i.MX27 CSI from Sascha Hauer, that Alan Carvalho de Assis has posted in > > > linux-media last December[1]. Since all I have is a i.MX25 PDK paltform I can't > > > test the mx27 specific code. Testers and comment are welcome. > > > > > > [1] https://patchwork.kernel.org/patch/67636/ > > > > > > Baruch Siach (3): > > > mx2_camera: Add soc_camera support for i.MX25/i.MX27 > > > mx27: add support for the CSI device > > > mx25: add support for the CSI device > > > > With the two additions I sent I can confirm this working on i.MX27, so > > no need to remove the related code. > > Thanks. I'll add your patches to my queue and resend the series next week. Firstly, Sascha, unfortunately, you've forgotten to CC the maintainer, that will have to deal with these patches. Secondly, I don't think that's a good idea to submit mx27 fixes as incremental patches. I'd prefer to have them rolled into the actual driver submission patches, where Sascha would just add his Sob / acked-by / tested-by / whatever... Or you can first submit an mx25-only driver and let Sascha add mx27 to it, in which case this would be a functionality extension, but not a fix of a broken driver. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] Driver for the i.MX2x CMOS Sensor Interface 2010-05-21 18:33 ` Guennadi Liakhovetski @ 2010-05-25 7:20 ` Sascha Hauer 0 siblings, 0 replies; 16+ messages in thread From: Sascha Hauer @ 2010-05-25 7:20 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: Baruch Siach, Linux Media Mailing List, linux-arm-kernel On Fri, May 21, 2010 at 08:33:40PM +0200, Guennadi Liakhovetski wrote: > On Fri, 21 May 2010, Baruch Siach wrote: > > > Hi Sascha, > > > > On Fri, May 21, 2010 at 09:20:45AM +0200, Sascha Hauer wrote: > > > On Thu, May 06, 2010 at 04:09:38PM +0300, Baruch Siach wrote: > > > > This series contains a soc_camera driver for the i.MX25/i.MX27 CSI device, and > > > > platform code for the i.MX25 and i.MX27 chips. This driver is based on a driver > > > > for i.MX27 CSI from Sascha Hauer, that Alan Carvalho de Assis has posted in > > > > linux-media last December[1]. Since all I have is a i.MX25 PDK paltform I can't > > > > test the mx27 specific code. Testers and comment are welcome. > > > > > > > > [1] https://patchwork.kernel.org/patch/67636/ > > > > > > > > Baruch Siach (3): > > > > mx2_camera: Add soc_camera support for i.MX25/i.MX27 > > > > mx27: add support for the CSI device > > > > mx25: add support for the CSI device > > > > > > With the two additions I sent I can confirm this working on i.MX27, so > > > no need to remove the related code. > > > > Thanks. I'll add your patches to my queue and resend the series next week. > > Firstly, Sascha, unfortunately, you've forgotten to CC the maintainer, > that will have to deal with these patches. > > Secondly, I don't think that's a good idea to submit mx27 fixes as > incremental patches. I'd prefer to have them rolled into the actual driver > submission patches, where Sascha would just add his Sob / acked-by / > tested-by / whatever... Or you can first submit an mx25-only driver and > let Sascha add mx27 to it, in which case this would be a functionality > extension, but not a fix of a broken driver. My intention with these fixes was that Baruch integrates them into his patch (which he did). Sascha -- 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 | ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2010-05-25 7:20 UTC | newest] Thread overview: 16+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-05-06 13:09 [PATCH 0/3] Driver for the i.MX2x CMOS Sensor Interface Baruch Siach 2010-05-06 13:09 ` [PATCH 1/3] mx2_camera: Add soc_camera support for i.MX25/i.MX27 Baruch Siach 2010-05-12 19:02 ` Guennadi Liakhovetski 2010-05-13 5:21 ` Baruch Siach 2010-05-17 7:27 ` Sascha Hauer 2010-05-17 13:58 ` Baruch Siach 2010-05-19 8:55 ` Sascha Hauer 2010-05-21 7:19 ` Sascha Hauer 2010-05-06 13:09 ` [PATCH 2/3] mx27: add support for the CSI device Baruch Siach 2010-05-21 7:17 ` Sascha Hauer 2010-05-06 13:09 ` [PATCH 3/3] mx25: " Baruch Siach 2010-05-12 19:11 ` Guennadi Liakhovetski 2010-05-21 7:20 ` [PATCH 0/3] Driver for the i.MX2x CMOS Sensor Interface Sascha Hauer 2010-05-21 7:27 ` Baruch Siach 2010-05-21 18:33 ` Guennadi Liakhovetski 2010-05-25 7:20 ` Sascha Hauer
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).