This patch is to enable Atmel Image Sensor Interface (ISI) driver support.
- Using soc-camera framework with videobuf2 dma-contig allocator
- Supporting video streaming of YUV packed format
- Tested on AT91SAM9M10G45-EK with OV2640
Signed-off-by: Josh Wu <[email protected]>
---
base on branch staging/for_v2.6.40
arch/arm/mach-at91/include/mach/at91_isi.h | 454 ++++++++++++
drivers/media/video/Kconfig | 10 +
drivers/media/video/Makefile | 1 +
drivers/media/video/atmel-isi.c | 1089 ++++++++++++++++++++++++++++
4 files changed, 1554 insertions(+), 0 deletions(-)
create mode 100644 arch/arm/mach-at91/include/mach/at91_isi.h
create mode 100644 drivers/media/video/atmel-isi.c
diff --git a/arch/arm/mach-at91/include/mach/at91_isi.h b/arch/arm/mach-at91/include/mach/at91_isi.h
new file mode 100644
index 0000000..3219358
--- /dev/null
+++ b/arch/arm/mach-at91/include/mach/at91_isi.h
@@ -0,0 +1,454 @@
+/*
+ * Register definitions for the Atmel Image Sensor Interface.
+ *
+ * Copyright (C) 2011 Atmel Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#ifndef __AT91_ISI_H__
+#define __AT91_ISI_H__
+
+#include <linux/videodev2.h>
+#include <linux/i2c.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-common.h>
+#include <media/v4l2-ioctl.h>
+#include <media/v4l2-chip-ident.h>
+
+/* ISI register offsets */
+#define ISI_CR1 0x0000
+#define ISI_CR2 0x0004
+#define ISI_SR 0x0008
+#define ISI_IER 0x000c
+#define ISI_IDR 0x0010
+#define ISI_IMR 0x0014
+#define ISI_PSIZE 0x0020
+#define ISI_PDECF 0x0024
+#define ISI_PPFBD 0x0028
+#define ISI_CDBA 0x002c
+#define ISI_Y2R_SET0 0x0030
+#define ISI_Y2R_SET1 0x0034
+#define ISI_R2Y_SET0 0x0038
+#define ISI_R2Y_SET1 0x003c
+#define ISI_R2Y_SET2 0x0040
+
+/* ISI_V2 register offsets */
+#define ISI_V2_CFG1 0x0000
+#define ISI_V2_CFG2 0x0004
+#define ISI_V2_PSIZE 0x0008
+#define ISI_V2_PDECF 0x000c
+#define ISI_V2_Y2R_SET0 0x0010
+#define ISI_V2_Y2R_SET1 0x0014
+#define ISI_V2_R2Y_SET0 0x0018
+#define ISI_V2_R2Y_SET1 0x001C
+#define ISI_V2_R2Y_SET2 0x0020
+#define ISI_V2_CTRL 0x0024
+#define ISI_V2_STATUS 0x0028
+#define ISI_V2_INTEN 0x002C
+#define ISI_V2_INTDIS 0x0030
+#define ISI_V2_INTMASK 0x0034
+#define ISI_V2_DMA_CHER 0x0038
+#define ISI_V2_DMA_CHDR 0x003C
+#define ISI_V2_DMA_CHSR 0x0040
+#define ISI_V2_DMA_P_ADDR 0x0044
+#define ISI_V2_DMA_P_CTRL 0x0048
+#define ISI_V2_DMA_P_DSCR 0x004C
+#define ISI_V2_DMA_C_ADDR 0x0050
+#define ISI_V2_DMA_C_CTRL 0x0054
+#define ISI_V2_DMA_C_DSCR 0x0058
+
+/* Bitfields in CR1 */
+#define ISI_RST_OFFSET 0
+#define ISI_RST_SIZE 1
+#define ISI_DIS_OFFSET 1
+#define ISI_DIS_SIZE 1
+#define ISI_HSYNC_POL_OFFSET 2
+#define ISI_HSYNC_POL_SIZE 1
+#define ISI_VSYNC_POL_OFFSET 3
+#define ISI_VSYNC_POL_SIZE 1
+#define ISI_PIXCLK_POL_OFFSET 4
+#define ISI_PIXCLK_POL_SIZE 1
+#define ISI_EMB_SYNC_OFFSET 6
+#define ISI_EMB_SYNC_SIZE 1
+#define ISI_CRC_SYNC_OFFSET 7
+#define ISI_CRC_SYNC_SIZE 1
+#define ISI_FRATE_OFFSET 8
+#define ISI_FRATE_SIZE 3
+#define ISI_FULL_OFFSET 12
+#define ISI_FULL_SIZE 1
+#define ISI_THMASK_OFFSET 13
+#define ISI_THMASK_SIZE 2
+#define ISI_CODEC_ON_OFFSET 15
+#define ISI_CODEC_ON_SIZE 1
+#define ISI_SLD_OFFSET 16
+#define ISI_SLD_SIZE 8
+#define ISI_SFD_OFFSET 24
+#define ISI_SFD_SIZE 8
+
+/* Bitfields in CFG1 */
+#define ISI_V2_HSYNC_POL_OFFSET 2
+#define ISI_V2_HSYNC_POL_SIZE 1
+#define ISI_V2_VSYNC_POL_OFFSET 3
+#define ISI_V2_VSYNC_POL_SIZE 1
+#define ISI_V2_PIXCLK_POL_OFFSET 4
+#define ISI_V2_PIXCLK_POL_SIZE 1
+#define ISI_V2_EMB_SYNC_OFFSET 6
+#define ISI_V2_EMB_SYNC_SIZE 1
+#define ISI_V2_CRC_SYNC_OFFSET 7
+#define ISI_V2_CRC_SYNC_SIZE 1
+#define ISI_V2_FRATE_OFFSET 8
+#define ISI_V2_FRATE_SIZE 3
+#define ISI_V2_DISCR_OFFSET 11
+#define ISI_V2_DISCR_SIZE 1
+#define ISI_V2_FULL_OFFSET 12
+#define ISI_V2_FULL_SIZE 1
+#define ISI_V2_THMASK_OFFSET 13
+#define ISI_V2_THMASK_SIZE 2
+#define ISI_V2_SLD_OFFSET 16
+#define ISI_V2_SLD_SIZE 8
+#define ISI_V2_SFD_OFFSET 24
+#define ISI_V2_SFD_SIZE 8
+
+/* Bitfields in CR2 */
+#define ISI_IM_VSIZE_OFFSET 0
+#define ISI_IM_VSIZE_SIZE 11
+#define ISI_GS_MODE_OFFSET 11
+#define ISI_GS_MODE_SIZE 1
+#define ISI_RGB_MODE_OFFSET 12
+#define ISI_RGB_MODE_SIZE 1
+#define ISI_GRAYSCALE_OFFSET 13
+#define ISI_GRAYSCALE_SIZE 1
+#define ISI_RGB_SWAP_OFFSET 14
+#define ISI_RGB_SWAP_SIZE 1
+#define ISI_COL_SPACE_OFFSET 15
+#define ISI_COL_SPACE_SIZE 1
+#define ISI_IM_HSIZE_OFFSET 16
+#define ISI_IM_HSIZE_SIZE 11
+#define ISI_YCC_SWAP_OFFSET 28
+#define ISI_YCC_SWAP_SIZE 2
+#define ISI_RGB_CFG_OFFSET 30
+#define ISI_RGB_CFG_SIZE 2
+
+/* Bitfields in CFG2 */
+#define ISI_V2_IM_VSIZE_OFFSET 0
+#define ISI_V2_IM_VSIZE_SIZE 11
+#define ISI_V2_GS_MODE_OFFSET 11
+#define ISI_V2_GS_MODE_SIZE 1
+#define ISI_V2_RGB_MODE_OFFSET 12
+#define ISI_V2_RGB_MODE_SIZE 1
+#define ISI_V2_GRAYSCALE_OFFSET 13
+#define ISI_V2_GRAYSCALE_SIZE 1
+#define ISI_V2_RGB_SWAP_OFFSET 14
+#define ISI_V2_RGB_SWAP_SIZE 1
+#define ISI_V2_COL_SPACE_OFFSET 15
+#define ISI_V2_COL_SPACE_SIZE 1
+#define ISI_V2_IM_HSIZE_OFFSET 16
+#define ISI_V2_IM_HSIZE_SIZE 11
+#define ISI_V2_YCC_SWAP_OFFSET 28
+#define ISI_V2_YCC_SWAP_SIZE 2
+#define ISI_V2_RGB_CFG_OFFSET 30
+#define ISI_V2_RGB_CFG_SIZE 2
+
+/* Bitfields in CTRL */
+#define ISI_V2_EN_OFFSET 0
+#define ISI_V2_EN_SIZE 1
+#define ISI_V2_DIS_OFFSET 1
+#define ISI_V2_DIS_SIZE 1
+#define ISI_V2_SRST_OFFSET 2
+#define ISI_V2_SRST_SIZE 1
+#define ISI_V2_CDC_OFFSET 8
+#define ISI_V2_CDC_SIZE 1
+
+/* Bitfields in SR/IER/IDR/IMR */
+#define ISI_SOF_OFFSET 0
+#define ISI_SOF_SIZE 1
+#define ISI_SOFTRST_OFFSET 2
+#define ISI_SOFTRST_SIZE 1
+#define ISI_CDC_STATUS_OFFSET 3
+#define ISI_CDC_STATUS_SIZE 1
+#define ISI_CRC_ERR_OFFSET 4
+#define ISI_CRC_ERR_SIZE 1
+#define ISI_FO_C_OVF_OFFSET 5
+#define ISI_FO_C_OVF_SIZE 1
+#define ISI_FO_P_OVF_OFFSET 6
+#define ISI_FO_P_OVF_SIZE 1
+#define ISI_FO_P_EMP_OFFSET 7
+#define ISI_FO_P_EMP_SIZE 1
+#define ISI_FO_C_EMP_OFFSET 8
+#define ISI_FO_C_EMP_SIZE 1
+#define ISI_FR_OVR_OFFSET 9
+#define ISI_FR_OVR_SIZE 1
+
+/* Bitfields in SR/IER/IDR/IMR(ISI_V2) */
+#define ISI_V2_ENABLE_OFFSET 0
+#define ISI_V2_ENABLE_SIZE 1
+#define ISI_V2_DIS_DONE_OFFSET 1
+#define ISI_V2_DIS_DONE_SIZE 1
+#define ISI_V2_SRST_OFFSET 2
+#define ISI_V2_SRST_SIZE 1
+#define ISI_V2_CDC_STATUS_OFFSET 8
+#define ISI_V2_CDC_STATUS_SIZE 1
+#define ISI_V2_VSYNC_OFFSET 10
+#define ISI_V2_VSYNC_SIZE 1
+#define ISI_V2_PXFR_DONE_OFFSET 16
+#define ISI_V2_PXFR_DONE_SIZE 1
+#define ISI_V2_CXFR_DONE_OFFSET 17
+#define ISI_V2_CXFR_DONE_SIZE 1
+#define ISI_V2_P_OVR_OFFSET 24
+#define ISI_V2_P_OVR_SIZE 1
+#define ISI_V2_C_OVR_OFFSET 25
+#define ISI_V2_C_OVR_SIZE 1
+#define ISI_V2_CRC_ERR_OFFSET 26
+#define ISI_V2_CRC_ERR_SIZE 1
+#define ISI_V2_FR_OVR_OFFSET 27
+#define ISI_V2_FR_OVR_SIZE 1
+
+/* Bitfields in PSIZE */
+#define ISI_PREV_VSIZE_OFFSET 0
+#define ISI_PREV_VSIZE_SIZE 10
+#define ISI_PREV_HSIZE_OFFSET 16
+#define ISI_PREV_HSIZE_SIZE 10
+
+/* Bitfields in PSIZE(ISI_V2) */
+#define ISI_V2_PREV_VSIZE_OFFSET 0
+#define ISI_V2_PREV_VSIZE_SIZE 10
+#define ISI_V2_PREV_HSIZE_OFFSET 16
+#define ISI_V2_PREV_HSIZE_SIZE 10
+
+/* Bitfields in PCDEF */
+#define ISI_DEC_FACTOR_OFFSET 0
+#define ISI_DEC_FACTOR_SIZE 8
+
+/* Bitfields in PCDEF */
+#define ISI_V2_DEC_FACTOR_OFFSET 0
+#define ISI_V2_DEC_FACTOR_SIZE 8
+
+/* Bitfields in PPFBD */
+#define ISI_PREV_FBD_ADDR_OFFSET 0
+#define ISI_PREV_FBD_ADDR_SIZE 32
+
+/* Bitfields in CDBA */
+#define ISI_CODEC_DMA_ADDR_OFFSET 0
+#define ISI_CODEC_DMA_ADDR_SIZE 32
+
+/* Bitfields in DMA_C_ADDR */
+#define ISI_V2_DMA_ADDR_OFFSET 0
+#define ISI_V2_DMA_ADDR_SIZE 32
+
+/* Bitfields in DMA_C_CTRL & in DMA_P_CTRL */
+#define ISI_V2_DMA_FETCH_OFFSET 0
+#define ISI_V2_DMA_FETCH_SIZE 1
+#define ISI_V2_DMA_WB_OFFSET 1
+#define ISI_V2_DMA_WB_SIZE 1
+#define ISI_V2_DMA_IEN_OFFSET 2
+#define ISI_V2_DMA_IEN_SIZE 1
+#define ISI_V2_DMA_DONE_OFFSET 3
+#define ISI_V2_DMA_DONE_SIZE 1
+
+/* Bitfields in DMA_CHER */
+#define ISI_V2_DMA_P_CH_EN_OFFSET 0
+#define ISI_V2_DMA_P_CH_EN_SIZE 1
+#define ISI_V2_DMA_C_CH_EN_OFFSET 1
+#define ISI_V2_DMA_C_CH_EN_SIZE 1
+
+/* Bitfields in Y2R_SET0 */
+#define ISI_Y2R_SET0_C3_OFFSET 24
+#define ISI_Y2R_SET0_C3_SIZE 8
+
+/* Bitfields in Y2R_SET0(ISI_V2) */
+#define ISI_V2_Y2R_SET0_C3_OFFSET 24
+#define ISI_V2_Y2R_SET0_C3_SIZE 8
+
+/* Bitfields in Y2R_SET1 */
+#define ISI_Y2R_SET1_C4_OFFSET 0
+#define ISI_Y2R_SET1_C4_SIZE 9
+#define ISI_YOFF_OFFSET 12
+#define ISI_YOFF_SIZE 1
+#define ISI_CROFF_OFFSET 13
+#define ISI_CROFF_SIZE 1
+#define ISI_CBOFF_OFFSET 14
+#define ISI_CBOFF_SIZE 1
+
+/* Bitfields in Y2R_SET1(ISI_V2) */
+#define ISI_V2_Y2R_SET1_C4_OFFSET 0
+#define ISI_V2_Y2R_SET1_C4_SIZE 9
+#define ISI_V2_YOFF_OFFSET 12
+#define ISI_V2_YOFF_SIZE 1
+#define ISI_V2_CROFF_OFFSET 13
+#define ISI_V2_CROFF_SIZE 1
+#define ISI_V2_CBOFF_OFFSET 14
+#define ISI_V2_CBOFF_SIZE 1
+
+/* Bitfields in R2Y_SET0 */
+#define ISI_C0_OFFSET 0
+#define ISI_C0_SIZE 8
+#define ISI_C1_OFFSET 8
+#define ISI_C1_SIZE 8
+#define ISI_C2_OFFSET 16
+#define ISI_C2_SIZE 8
+#define ISI_ROFF_OFFSET 24
+#define ISI_ROFF_SIZE 1
+
+/* Bitfields in R2Y_SET0(ISI_V2) */
+#define ISI_V2_C0_OFFSET 0
+#define ISI_V2_C0_SIZE 8
+#define ISI_V2_C1_OFFSET 8
+#define ISI_V2_C1_SIZE 8
+#define ISI_V2_C2_OFFSET 16
+#define ISI_V2_C2_SIZE 8
+#define ISI_V2_ROFF_OFFSET 24
+#define ISI_V2_ROFF_SIZE 1
+
+/* Bitfields in R2Y_SET1 */
+#define ISI_R2Y_SET1_C3_OFFSET 0
+#define ISI_R2Y_SET1_C3_SIZE 8
+#define ISI_R2Y_SET1_C4_OFFSET 8
+#define ISI_R2Y_SET1_C4_SIZE 8
+#define ISI_C5_OFFSET 16
+#define ISI_C5_SIZE 8
+#define ISI_GOFF_OFFSET 24
+#define ISI_GOFF_SIZE 1
+
+/* Bitfields in R2Y_SET1(ISI_V2) */
+#define ISI_V2_R2Y_SET1_C3_OFFSET 0
+#define ISI_V2_R2Y_SET1_C3_SIZE 8
+#define ISI_V2_R2Y_SET1_C4_OFFSET 8
+#define ISI_V2_R2Y_SET1_C4_SIZE 8
+#define ISI_V2_C5_OFFSET 16
+#define ISI_V2_C5_SIZE 8
+#define ISI_V2_GOFF_OFFSET 24
+#define ISI_V2_GOFF_SIZE 1
+
+/* Bitfields in R2Y_SET2 */
+#define ISI_C6_OFFSET 0
+#define ISI_C6_SIZE 8
+#define ISI_C7_OFFSET 8
+#define ISI_C7_SIZE 8
+#define ISI_C8_OFFSET 16
+#define ISI_C8_SIZE 8
+#define ISI_BOFF_OFFSET 24
+#define ISI_BOFF_SIZE 1
+
+/* Bitfields in R2Y_SET2(ISI_V2) */
+#define ISI_V2_C6_OFFSET 0
+#define ISI_V2_C6_SIZE 8
+#define ISI_V2_C7_OFFSET 8
+#define ISI_V2_C7_SIZE 8
+#define ISI_V2_C8_OFFSET 16
+#define ISI_V2_C8_SIZE 8
+#define ISI_V2_BOFF_OFFSET 24
+#define ISI_V2_BOFF_SIZE 1
+
+/* Constants for FRATE */
+#define ISI_FRATE_CAPTURE_ALL 0
+
+/* Constants for FRATE(ISI_V2) */
+#define ISI_V2_FRATE_CAPTURE_ALL 0
+
+/* Constants for YCC_SWAP */
+#define ISI_YCC_SWAP_DEFAULT 0
+#define ISI_YCC_SWAP_MODE_1 1
+#define ISI_YCC_SWAP_MODE_2 2
+#define ISI_YCC_SWAP_MODE_3 3
+
+/* Constants for YCC_SWAP(ISI_V2) */
+#define ISI_V2_YCC_SWAP_DEFAULT 0
+#define ISI_V2_YCC_SWAP_MODE_1 1
+#define ISI_V2_YCC_SWAP_MODE_2 2
+#define ISI_V2_YCC_SWAP_MODE_3 3
+
+/* Constants for RGB_CFG */
+#define ISI_RGB_CFG_DEFAULT 0
+#define ISI_RGB_CFG_MODE_1 1
+#define ISI_RGB_CFG_MODE_2 2
+#define ISI_RGB_CFG_MODE_3 3
+
+/* Constants for RGB_CFG(ISI_V2) */
+#define ISI_V2_RGB_CFG_DEFAULT 0
+#define ISI_V2_RGB_CFG_MODE_1 1
+#define ISI_V2_RGB_CFG_MODE_2 2
+#define ISI_V2_RGB_CFG_MODE_3 3
+
+/* Bit manipulation macros */
+#define ISI_BIT(name) \
+ (1 << ISI_##name##_OFFSET)
+#define ISI_BF(name, value) \
+ (((value) & ((1 << ISI_##name##_SIZE) - 1)) \
+ << ISI_##name##_OFFSET)
+#define ISI_BFEXT(name, value) \
+ (((value) >> ISI_##name##_OFFSET) \
+ & ((1 << ISI_##name##_SIZE) - 1))
+#define ISI_BFINS(name, value, old) \
+ (((old) & ~(((1 << ISI_##name##_SIZE) - 1) \
+ << ISI_##name##_OFFSET))\
+ | ISI_BF(name, value))
+
+/* Register access macros */
+#define isi_readl(port, reg) \
+ __raw_readl((port)->regs + ISI_##reg)
+#define isi_writel(port, reg, value) \
+ __raw_writel((value), (port)->regs + ISI_##reg)
+
+#define ATMEL_V4L2_VID_FLAGS (V4L2_CAP_VIDEO_OUTPUT)
+
+struct atmel_isi;
+
+enum atmel_isi_pixfmt {
+ ATMEL_ISI_PIXFMT_GREY, /* Greyscale */
+ ATMEL_ISI_PIXFMT_CbYCrY,
+ ATMEL_ISI_PIXFMT_CrYCbY,
+ ATMEL_ISI_PIXFMT_YCbYCr,
+ ATMEL_ISI_PIXFMT_YCrYCb,
+ ATMEL_ISI_PIXFMT_RGB24,
+ ATMEL_ISI_PIXFMT_BGR24,
+ ATMEL_ISI_PIXFMT_RGB16,
+ ATMEL_ISI_PIXFMT_BGR16,
+ ATMEL_ISI_PIXFMT_GRB16, /* G[2:0] R[4:0]/B[4:0] G[5:3] */
+ ATMEL_ISI_PIXFMT_GBR16, /* G[2:0] B[4:0]/R[4:0] G[5:3] */
+ ATMEL_ISI_PIXFMT_RGB24_REV,
+ ATMEL_ISI_PIXFMT_BGR24_REV,
+ ATMEL_ISI_PIXFMT_RGB16_REV,
+ ATMEL_ISI_PIXFMT_BGR16_REV,
+ ATMEL_ISI_PIXFMT_GRB16_REV, /* G[2:0] R[4:0]/B[4:0] G[5:3] */
+ ATMEL_ISI_PIXFMT_GBR16_REV, /* G[2:0] B[4:0]/R[4:0] G[5:3] */
+};
+
+struct isi_platform_data {
+ u8 has_emb_sync;
+ u8 emb_crc_sync;
+ u8 hsync_act_low;
+ u8 vsync_act_low;
+ u8 pclk_act_falling;
+ u8 isi_full_mode;
+#define ISI_HSYNC_ACT_LOW 0x01
+#define ISI_VSYNC_ACT_LOW 0x02
+#define ISI_PXCLK_ACT_FALLING 0x04
+#define ISI_EMB_SYNC 0x08
+#define ISI_CRC_SYNC 0x10
+#define ISI_FULL 0x20
+#define ISI_DATAWIDTH_8 0x40
+#define ISI_DATAWIDTH_10 0x80
+ u32 flags;
+ u8 gs_mode;
+#define ISI_GS_2PIX_PER_WORD 0x00
+#define ISI_GS_1PIX_PER_WORD 0x01
+ u8 pixfmt;
+ u8 sfd;
+ u8 sld;
+ u8 thmask;
+#define ISI_BURST_4_8_16 0x00
+#define ISI_BURST_8_16 0x01
+#define ISI_BURST_16 0x02
+ u8 frate;
+#define ISI_FRATE_DIV_2 0x01
+#define ISI_FRATE_DIV_3 0x02
+#define ISI_FRATE_DIV_4 0x03
+#define ISI_FRATE_DIV_5 0x04
+#define ISI_FRATE_DIV_6 0x05
+#define ISI_FRATE_DIV_7 0x06
+#define ISI_FRATE_DIV_8 0x07
+};
+
+#endif /* __AT91_ISI_H__ */
diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index d61414e..eae6005 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -80,6 +80,16 @@ menuconfig VIDEO_CAPTURE_DRIVERS
Some of those devices also supports FM radio.
if VIDEO_CAPTURE_DRIVERS && VIDEO_V4L2
+config VIDEO_ATMEL_ISI
+ tristate "ATMEL Image Sensor Interface (ISI) support"
+ depends on VIDEO_DEV && SOC_CAMERA
+ select VIDEOBUF2_DMA_CONTIG
+ default n
+ ---help---
+ This module makes the ATMEL Image Sensor Interface available
+ as a v4l2 device.
+ Say Y here to enable selecting the Image Sensor Interface.
+ When in doubt, say N.
config VIDEO_ADV_DEBUG
bool "Enable advanced debug functionality"
diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index a10e4c3..f734a65 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -166,6 +166,7 @@ obj-$(CONFIG_VIDEO_SH_MOBILE_CSI2) += sh_mobile_csi2.o
obj-$(CONFIG_VIDEO_SH_MOBILE_CEU) += sh_mobile_ceu_camera.o
obj-$(CONFIG_VIDEO_OMAP1) += omap1_camera.o
obj-$(CONFIG_VIDEO_SAMSUNG_S5P_FIMC) += s5p-fimc/
+obj-$(CONFIG_VIDEO_ATMEL_ISI) += atmel-isi.o
obj-$(CONFIG_ARCH_DAVINCI) += davinci/
diff --git a/drivers/media/video/atmel-isi.c b/drivers/media/video/atmel-isi.c
new file mode 100644
index 0000000..33d0b83
--- /dev/null
+++ b/drivers/media/video/atmel-isi.c
@@ -0,0 +1,1089 @@
+/*
+ * Copyright (c) 2011 Atmel Corporation
+ *
+ * Based on previous work by Lars Haring and Sedji Gaouaou
+ *
+ * Based on the bttv driver for Bt848 with respective copyright holders
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/fs.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/version.h>
+#include <linux/kfifo.h>
+
+#include <mach/board.h>
+#include <mach/cpu.h>
+#include <mach/at91_isi.h>
+
+#include <media/videobuf2-dma-contig.h>
+#include <media/soc_camera.h>
+#include <media/soc_mediabus.h>
+
+#define ATMEL_ISI_VERSION KERNEL_VERSION(1, 0, 0)
+#define MAX_BUFFER_NUMS 32
+#define MAX_SUPPORT_WIDTH 2048
+#define MAX_SUPPORT_HEIGHT 2048
+
+static unsigned int vid_limit = 16;
+
+enum isi_buffer_state {
+ ISI_BUF_NEEDS_INIT,
+ ISI_BUF_PREPARED,
+};
+
+/* Single frame capturing states */
+enum {
+ STATE_IDLE = 0,
+ STATE_CAPTURE_READY,
+ STATE_CAPTURE_WAIT_SOF,
+ STATE_CAPTURE_IN_PROGRESS,
+ STATE_CAPTURE_DONE,
+ STATE_CAPTURE_ERROR,
+};
+
+/* Frame buffer descriptor
+ * Used by the ISI module as a linked list for the DMA controller.
+ */
+struct fbd {
+ /* Physical address of the frame buffer */
+ u32 fb_address;
+#if defined(CONFIG_ARCH_AT91SAM9G45) ||\
+ defined(CONFIG_ARCH_AT91SAM9X5)
+ /* DMA Control Register(only in HISI2) */
+ u32 dma_ctrl;
+#endif
+ /* Physical address of the next fbd */
+ u32 next_fbd_address;
+};
+
+#if defined(CONFIG_ARCH_AT91SAM9G45) ||\
+ defined(CONFIG_ARCH_AT91SAM9X5)
+static void set_dma_ctrl(struct fbd *fb_desc, u32 ctrl)
+{
+ fb_desc->dma_ctrl = ctrl;
+}
+#else
+static void set_dma_ctrl(struct fbd *fb_desc, u32 ctrl) { }
+#endif
+
+/* Frame buffer data
+ */
+struct frame_buffer {
+ struct vb2_buffer vb;
+ struct fbd fb_desc;
+ /* Frame number of the frame */
+ unsigned long sequence;
+
+ enum isi_buffer_state dma_desc_status;
+ struct list_head list;
+};
+
+struct atmel_isi {
+ /* ISI module spin lock. Protects against concurrent access of variables
+ * that are shared with the ISR */
+ spinlock_t lock;
+ void __iomem *regs;
+
+ /* If set ISI is in still capture mode */
+ int still_capture;
+ int sequence;
+ /* State of the ISI module in capturing mode */
+ int state;
+
+ /* Capture/streaming wait queue for waiting for SOF */
+ wait_queue_head_t capture_wq;
+
+ struct v4l2_device v4l2_dev;
+
+ struct vb2_alloc_ctx *alloc_ctx;
+
+ struct clk *pclk;
+ struct platform_device *pdev;
+ unsigned int irq;
+
+ struct isi_platform_data *pdata;
+ unsigned long platform_flags;
+
+ struct list_head video_buffer_list;
+ struct frame_buffer *active;
+
+ struct soc_camera_device *icd;
+ struct soc_camera_host soc_host;
+};
+
+static int configure_geometry(struct atmel_isi *isi, u32 width,
+ u32 height, enum v4l2_mbus_pixelcode code)
+{
+ u32 cfg2, cr, ctrl;
+
+ cr = 0;
+ switch (code) {
+ /* YUV, including grey */
+ case V4L2_MBUS_FMT_Y8_1X8:
+ cr = ISI_BIT(GRAYSCALE);
+ break;
+ case V4L2_MBUS_FMT_UYVY8_2X8:
+ cr = ISI_BF(V2_YCC_SWAP, 3);
+ break;
+ case V4L2_MBUS_FMT_VYUY8_2X8:
+ cr = ISI_BF(V2_YCC_SWAP, 2);
+ break;
+ case V4L2_MBUS_FMT_YUYV8_2X8:
+ cr = ISI_BF(V2_YCC_SWAP, 1);
+ break;
+ case V4L2_MBUS_FMT_YVYU8_2X8:
+ cr = ISI_BF(V2_YCC_SWAP, 0);
+ break;
+ /* RGB, TODO */
+ default:
+ return -EINVAL;
+ }
+
+ ctrl = ISI_BIT(DIS);
+ isi_writel(isi, V2_CTRL, ctrl);
+ /* Check if module properly disable */
+ while (isi_readl(isi, V2_STATUS) & ISI_BIT(V2_DIS_DONE))
+ cpu_relax();
+
+ cfg2 = isi_readl(isi, V2_CFG2);
+ cfg2 |= cr;
+ cfg2 = ISI_BFINS(V2_IM_VSIZE, height - 1, cfg2);
+ cfg2 = ISI_BFINS(V2_IM_HSIZE, width - 1, cfg2);
+ isi_writel(isi, V2_CFG2, cfg2);
+
+ return 0;
+}
+
+static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
+{
+ if (isi->active) {
+ struct vb2_buffer *vb = &isi->active->vb;
+ struct frame_buffer *buf = isi->active;
+
+ list_del_init(&buf->list);
+ do_gettimeofday(&vb->v4l2_buf.timestamp);
+ vb->v4l2_buf.sequence = isi->sequence++;
+ vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
+ }
+
+ if (list_empty(&isi->video_buffer_list)) {
+ isi->active = NULL;
+ } else {
+ /* start next dma frame. */
+ isi->active = list_entry(isi->video_buffer_list.next,
+ struct frame_buffer, list);
+ isi_writel(isi, V2_DMA_C_DSCR, (__pa(&(isi->active->fb_desc))));
+ isi_writel(isi, V2_DMA_C_CTRL,
+ ISI_BIT(V2_DMA_FETCH) | ISI_BIT(V2_DMA_DONE));
+ isi_writel(isi, V2_DMA_CHER, ISI_BIT(V2_DMA_C_CH_EN));
+ }
+ return IRQ_HANDLED;
+}
+
+/* ISI interrupt service routine */
+static irqreturn_t isi_interrupt(int irq, void *dev_id)
+{
+ struct atmel_isi *isi = dev_id;
+ u32 status, mask, pending;
+ irqreturn_t ret = IRQ_NONE;
+
+ spin_lock(&isi->lock);
+
+ status = isi_readl(isi, V2_STATUS);
+ mask = isi_readl(isi, V2_INTMASK);
+ pending = status & mask;
+
+ if (!isi->still_capture) {
+ if (pending & (ISI_BIT(V2_VSYNC))) {
+ switch (isi->state) {
+ case STATE_IDLE:
+ isi->state = STATE_CAPTURE_READY;
+ wake_up_interruptible(&isi->capture_wq);
+ break;
+ }
+ } else if (likely(pending & (ISI_BIT(V2_CXFR_DONE)))) {
+ ret = atmel_isi_handle_streaming(isi);
+ }
+ } else {
+ while (pending) {
+ if (pending & (ISI_BIT(V2_C_OVR) | ISI_BIT(V2_FR_OVR)))
+ printk(KERN_ERR "Overrun (status = 0x%x)\n",
+ status);
+ else if (pending &
+ (ISI_BIT(V2_VSYNC) | ISI_BIT(V2_CDC))) {
+ switch (isi->state) {
+ case STATE_IDLE:
+ isi->state = STATE_CAPTURE_READY;
+ wake_up_interruptible(&isi->capture_wq);
+ break;
+ case STATE_CAPTURE_READY:
+ break;
+ case STATE_CAPTURE_WAIT_SOF:
+ isi->state = STATE_CAPTURE_IN_PROGRESS;
+ break;
+ }
+ }
+
+ if (likely(pending & (ISI_BIT(V2_CXFR_DONE))))
+ ret = atmel_isi_handle_streaming(isi);
+
+ status = isi_readl(isi, V2_STATUS);
+ mask = isi_readl(isi, V2_INTMASK);
+ pending = status & mask;
+ ret = IRQ_HANDLED;
+ }
+ }
+ spin_unlock(&isi->lock);
+
+ return ret;
+}
+
+static int atmel_isi_init(struct atmel_isi *isi)
+{
+ /*
+ * The reset will only succeed if we have a
+ * pixel clock from the camera.
+ */
+ isi_writel(isi, V2_CTRL, ISI_BIT(V2_SRST));
+ isi_writel(isi, V2_INTDIS, ~0UL);
+
+ return 0;
+}
+
+/* ------------------------------------------------------------------
+ Videobuf operations
+ ------------------------------------------------------------------*/
+static int queue_setup(struct vb2_queue *vq, unsigned int *nbuffers,
+ unsigned int *nplanes, unsigned long sizes[],
+ void *alloc_ctxs[])
+{
+ struct soc_camera_device *icd = soc_camera_from_vb2q(vq);
+ struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+ struct atmel_isi *dev = ici->priv;
+ unsigned long size;
+
+ int bytes_per_line = soc_mbus_bytes_per_line(icd->user_width,
+ icd->current_fmt->host_fmt);
+
+ if (bytes_per_line < 0)
+ return bytes_per_line;
+
+ size = bytes_per_line * icd->user_height;
+
+ if (0 == *nbuffers)
+ *nbuffers = MAX_BUFFER_NUMS;
+ if (*nbuffers > MAX_BUFFER_NUMS)
+ *nbuffers = MAX_BUFFER_NUMS;
+
+ while (size * *nbuffers > vid_limit * 1024 * 1024)
+ (*nbuffers)--;
+
+ *nplanes = 1;
+ sizes[0] = size;
+ alloc_ctxs[0] = dev->alloc_ctx;
+
+ dev->sequence = 0;
+ dev->active = NULL;
+
+ dev_dbg(icd->dev.parent, "%s, count=%d, size=%ld\n", __func__,
+ *nbuffers, size);
+
+ return 0;
+}
+
+static int buffer_init(struct vb2_buffer *vb)
+{
+ struct frame_buffer *buf = container_of(vb, struct frame_buffer, vb);
+
+ buf->dma_desc_status = ISI_BUF_NEEDS_INIT;
+ INIT_LIST_HEAD(&buf->list);
+
+ return 0;
+}
+
+static int buffer_prepare(struct vb2_buffer *vb)
+{
+ struct soc_camera_device *icd = soc_camera_from_vb2q(vb->vb2_queue);
+ struct frame_buffer *buf = container_of(vb, struct frame_buffer, vb);
+ unsigned long size;
+ int bytes_per_line = soc_mbus_bytes_per_line(icd->user_width,
+ icd->current_fmt->host_fmt);
+
+ if (bytes_per_line < 0)
+ return bytes_per_line;
+
+ size = bytes_per_line * icd->user_height;
+
+ if (vb2_plane_size(vb, 0) < size) {
+ dev_err(icd->dev.parent, "%s data will not fit into plane (%lu < %lu)\n",
+ __func__, vb2_plane_size(vb, 0), size);
+ return -EINVAL;
+ }
+
+ vb2_set_plane_payload(&buf->vb, 0, size);
+
+ if (buf->dma_desc_status == ISI_BUF_NEEDS_INIT) {
+ buf->dma_desc_status = ISI_BUF_PREPARED;
+
+ /* initialize the dma descriptor */
+ buf->fb_desc.fb_address = vb2_dma_contig_plane_paddr(vb, 0);
+ buf->fb_desc.next_fbd_address = 0;
+ set_dma_ctrl(&buf->fb_desc, ISI_BIT(V2_DMA_WB));
+ }
+
+ return 0;
+}
+
+static int buffer_finish(struct vb2_buffer *vb)
+{
+ return 0;
+}
+
+static void buffer_cleanup(struct vb2_buffer *vb)
+{
+ struct soc_camera_device *icd = soc_camera_from_vb2q(vb->vb2_queue);
+ struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+ struct atmel_isi *isi = ici->priv;
+ struct frame_buffer *buf = container_of(vb, struct frame_buffer, vb);
+ unsigned long flags = 0;
+
+ spin_lock_irqsave(&isi->lock, flags);
+ buf->dma_desc_status = ISI_BUF_NEEDS_INIT;
+ spin_unlock_irqrestore(&isi->lock, flags);
+}
+
+static void start_dma(struct atmel_isi *isi, struct frame_buffer *buffer)
+{
+ u32 ctrl, cfg1;
+ ctrl = isi_readl(isi, V2_CTRL);
+ cfg1 = isi_readl(isi, V2_CFG1);
+ /* Enable irq: cxfr for the codec path, pxfr for the preview path */
+ isi_writel(isi, V2_INTEN,
+ ISI_BIT(V2_CXFR_DONE) | ISI_BIT(V2_PXFR_DONE));
+
+ /* Enable codec path */
+ ctrl |= ISI_BIT(V2_CDC);
+ /* Check if already in a frame */
+ while (isi_readl(isi, V2_STATUS) & ISI_BIT(V2_CDC))
+ cpu_relax();
+
+ /* Write the address of the first frame buffer in the C_ADDR reg
+ * write the address of the first descriptor(link list of buffer)
+ * in the C_DSCR reg, and enable dma channel.
+ */
+ isi_writel(isi, V2_DMA_C_DSCR, (__pa(&(buffer->fb_desc))));
+ isi_writel(isi, V2_DMA_C_CTRL,
+ ISI_BIT(V2_DMA_FETCH) | ISI_BIT(V2_DMA_DONE));
+ isi_writel(isi, V2_DMA_CHER, ISI_BIT(V2_DMA_C_CH_EN));
+
+ /* Enable linked list */
+ cfg1 |= ISI_BF(V2_FRATE, isi->pdata->frate) | ISI_BIT(V2_DISCR);
+
+ /* Enable ISI module*/
+ ctrl |= ISI_BIT(V2_ENABLE);
+ isi_writel(isi, V2_CTRL, ctrl);
+ isi_writel(isi, V2_CFG1, cfg1);
+}
+
+static void buffer_queue(struct vb2_buffer *vb)
+{
+ struct soc_camera_device *icd = soc_camera_from_vb2q(vb->vb2_queue);
+ struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+ struct atmel_isi *isi = ici->priv;
+ struct frame_buffer *buf = container_of(vb, struct frame_buffer, vb);
+ unsigned long flags = 0;
+
+ spin_lock_irqsave(&isi->lock, flags);
+ list_add_tail(&buf->list, &isi->video_buffer_list);
+
+ if (isi->active == NULL) {
+ isi->active = buf;
+ start_dma(isi, buf);
+ }
+ spin_unlock_irqrestore(&isi->lock, flags);
+}
+
+static int start_streaming(struct vb2_queue *vq)
+{
+ struct soc_camera_device *icd = soc_camera_from_vb2q(vq);
+ struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+ struct atmel_isi *isi = ici->priv;
+
+ u32 sr = 0;
+ int ret;
+
+ spin_lock_irq(&isi->lock);
+ isi->state = STATE_IDLE;
+
+ /* Clear any pending SOF interrupt */
+ sr = isi_readl(isi, V2_STATUS);
+ /* Enable VSYNC interrupt for SOF */
+ isi_writel(isi, V2_INTEN, ISI_BIT(V2_VSYNC));
+ isi_writel(isi, V2_CTRL, isi_readl(isi, V2_CTRL) | ISI_BIT(V2_EN));
+
+ spin_unlock_irq(&isi->lock);
+ dev_dbg(icd->dev.parent, "isi: waiting for SOF\n");
+ ret = wait_event_interruptible(isi->capture_wq,
+ isi->state != STATE_IDLE);
+ if (ret)
+ return ret;
+
+ if (isi->state != STATE_CAPTURE_READY)
+ return -EIO;
+
+ spin_lock_irq(&isi->lock);
+ isi->state = STATE_CAPTURE_WAIT_SOF;
+ isi_writel(isi, V2_INTDIS, ISI_BIT(V2_VSYNC));
+ spin_unlock_irq(&isi->lock);
+
+ return 0;
+}
+
+/* abort streaming and wait for last buffer */
+static int stop_streaming(struct vb2_queue *vq)
+{
+ struct soc_camera_device *icd = soc_camera_from_vb2q(vq);
+ struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+ struct atmel_isi *isi = ici->priv;
+
+ spin_lock_irq(&isi->lock);
+ isi->still_capture = 0;
+ isi->active = NULL;
+
+ while (isi_readl(isi, V2_STATUS) & ISI_BIT(V2_CDC))
+ cpu_relax();
+
+ /* Disble codec path */
+ isi_writel(isi, V2_CTRL, isi_readl(isi, V2_CTRL) & (~ISI_BIT(V2_CDC)));
+ /* Disable interrupts */
+ isi_writel(isi, V2_INTDIS,
+ ISI_BIT(V2_CXFR_DONE) | ISI_BIT(V2_PXFR_DONE));
+ /* Disable ISI module*/
+ isi_writel(isi, V2_CTRL, isi_readl(isi, V2_CTRL) | ISI_BIT(V2_DIS));
+
+ /* Release all active buffers */
+ while (!list_empty(&isi->video_buffer_list)) {
+ struct frame_buffer *buf;
+ buf = list_entry(isi->video_buffer_list.next,
+ struct frame_buffer, list);
+ list_del_init(&buf->list);
+ vb2_buffer_done(&buf->vb, VB2_BUF_STATE_ERROR);
+ }
+
+ spin_unlock_irq(&isi->lock);
+ return 0;
+}
+
+static struct vb2_ops isi_video_qops = {
+ .queue_setup = queue_setup,
+ .buf_init = buffer_init,
+ .buf_prepare = buffer_prepare,
+ .buf_finish = buffer_finish,
+ .buf_cleanup = buffer_cleanup,
+ .buf_queue = buffer_queue,
+ .start_streaming = start_streaming,
+ .stop_streaming = stop_streaming,
+ .wait_prepare = soc_camera_unlock,
+ .wait_finish = soc_camera_lock,
+};
+
+/* ------------------------------------------------------------------
+ SOC camera operations for the device
+ ------------------------------------------------------------------*/
+static int isi_camera_init_videobuf(struct vb2_queue *q,
+ struct soc_camera_device *icd)
+{
+ q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+ q->io_modes = VB2_MMAP | VB2_USERPTR | VB2_READ;
+ q->drv_priv = icd;
+ q->buf_struct_size = sizeof(struct frame_buffer);
+ q->ops = &isi_video_qops;
+ q->mem_ops = &vb2_dma_contig_memops;
+
+ return vb2_queue_init(q);
+}
+static inline void stride_align(u32 *width)
+{
+ if (((*width + 7) & ~7) < 4096)
+ *width = (*width + 7) & ~7;
+ else
+ *width = *width & ~7;
+}
+
+static int isi_camera_set_fmt(struct soc_camera_device *icd,
+ struct v4l2_format *f)
+{
+ struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+ struct atmel_isi *isi = ici->priv;
+ 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;
+ }
+
+ stride_align(&pix->width);
+ dev_dbg(icd->dev.parent, "plan to set format %dx%d\n",
+ pix->width, pix->height);
+
+ 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)
+ return ret;
+
+ if (mf.code != xlate->code)
+ return -EINVAL;
+
+ ret = configure_geometry(isi, pix->width, pix->height, xlate->code);
+ if (ret < 0)
+ return ret;
+
+ pix->width = mf.width;
+ pix->height = mf.height;
+ pix->field = mf.field;
+ pix->colorspace = mf.colorspace;
+ icd->current_fmt = xlate;
+
+ pix->bytesperline = soc_mbus_bytes_per_line(pix->width,
+ xlate->host_fmt);
+ if (pix->bytesperline < 0)
+ return pix->bytesperline;
+ pix->sizeimage = pix->height * pix->bytesperline;
+
+ dev_dbg(icd->dev.parent, "Finally set format %dx%d, sizeimage = %d\n",
+ pix->width, pix->height, pix->sizeimage);
+
+ return ret;
+}
+
+static int isi_camera_try_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;
+ __u32 pixfmt = pix->pixelformat;
+ int ret;
+
+ xlate = soc_camera_xlate_by_fourcc(icd, pixfmt);
+ if (pixfmt && !xlate) {
+ dev_warn(icd->dev.parent, "Format %x not found\n", pixfmt);
+ return -EINVAL;
+ }
+
+ /* limit to Atmel ISI hardware capabilities */
+ if (pix->height > MAX_SUPPORT_HEIGHT)
+ pix->height = MAX_SUPPORT_HEIGHT;
+ if (pix->width > MAX_SUPPORT_WIDTH)
+ pix->width = MAX_SUPPORT_WIDTH;
+
+ pix->bytesperline = soc_mbus_bytes_per_line(pix->width,
+ xlate->host_fmt);
+ if (pix->bytesperline < 0)
+ return pix->bytesperline;
+ pix->sizeimage = pix->height * pix->bytesperline;
+
+ /* limit to sensor capabilities */
+ 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, try_mbus_fmt, &mf);
+ if (ret < 0)
+ return ret;
+
+ pix->width = mf.width;
+ pix->height = mf.height;
+ pix->colorspace = mf.colorspace;
+
+ switch (mf.field) {
+ case V4L2_FIELD_ANY:
+ pix->field = V4L2_FIELD_NONE;
+ break;
+ case V4L2_FIELD_NONE:
+ break;
+ default:
+ dev_err(icd->dev.parent, "Field type %d unsupported.\n",
+ mf.field);
+ ret = -EINVAL;
+ }
+
+ return ret;
+}
+
+static const struct soc_mbus_pixelfmt isi_camera_formats[] = {
+ {
+ .fourcc = V4L2_PIX_FMT_YUYV,
+ .name = "Packed YUV422 16 bit",
+ .bits_per_sample = 8,
+ .packing = SOC_MBUS_PACKING_2X8_PADHI,
+ .order = SOC_MBUS_ORDER_LE,
+ },
+};
+
+/* This will be corrected as we get more formats */
+static bool isi_camera_packing_supported(const struct soc_mbus_pixelfmt *fmt)
+{
+ return fmt->packing == SOC_MBUS_PACKING_NONE ||
+ (fmt->bits_per_sample == 8 &&
+ fmt->packing == SOC_MBUS_PACKING_2X8_PADHI) ||
+ (fmt->bits_per_sample > 8 &&
+ fmt->packing == SOC_MBUS_PACKING_EXTEND16);
+}
+
+static int test_platform_param(struct atmel_isi *isi,
+ unsigned char buswidth, unsigned long *flags)
+{
+ /*
+ * Platform specified synchronization and pixel clock polarities are
+ * only a recommendation and are only used during probing. Atmel ISI
+ * camera interface only works in master mode, i.e., uses HSYNC and
+ * VSYNC signals from the sensor
+ */
+ *flags = SOCAM_MASTER |
+ SOCAM_HSYNC_ACTIVE_HIGH |
+ SOCAM_HSYNC_ACTIVE_LOW |
+ SOCAM_VSYNC_ACTIVE_HIGH |
+ SOCAM_VSYNC_ACTIVE_LOW |
+ SOCAM_PCLK_SAMPLE_RISING |
+ SOCAM_PCLK_SAMPLE_FALLING |
+ SOCAM_DATA_ACTIVE_HIGH;
+
+ /* If requested data width is supported by the platform, use it */
+ switch (buswidth) {
+ case 10:
+ if (!(isi->platform_flags & ISI_DATAWIDTH_10))
+ return -EINVAL;
+ *flags |= SOCAM_DATAWIDTH_10;
+ break;
+ case 8:
+ if (!(isi->platform_flags & ISI_DATAWIDTH_8))
+ return -EINVAL;
+ *flags |= SOCAM_DATAWIDTH_8;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int isi_camera_try_bus_param(struct soc_camera_device *icd,
+ unsigned char buswidth)
+{
+ struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+ struct atmel_isi *isi = ici->priv;
+ unsigned long bus_flags, camera_flags;
+ int ret = test_platform_param(isi, buswidth, &bus_flags);
+
+ if (ret < 0)
+ return ret;
+
+ camera_flags = icd->ops->query_bus_param(icd);
+
+ ret = soc_camera_bus_param_compatible(camera_flags, bus_flags);
+ if (!ret)
+ return -EINVAL;
+ return 0;
+}
+
+
+static int isi_camera_get_formats(struct soc_camera_device *icd,
+ unsigned int idx,
+ struct soc_camera_format_xlate *xlate)
+{
+ struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
+ struct device *dev = icd->dev.parent;
+ int formats = 0, ret;
+ /* sensor format */
+ enum v4l2_mbus_pixelcode code;
+ /* soc camera host format */
+ const struct soc_mbus_pixelfmt *fmt;
+
+ ret = v4l2_subdev_call(sd, video, enum_mbus_fmt, idx, &code);
+ if (ret < 0)
+ /* No more formats */
+ return 0;
+
+ fmt = soc_mbus_get_fmtdesc(code);
+ if (!fmt) {
+ dev_err(icd->dev.parent,
+ "Invalid format code #%u: %d\n", idx, code);
+ return 0;
+ }
+
+ /* This also checks support for the requested bits-per-sample */
+ ret = isi_camera_try_bus_param(icd, fmt->bits_per_sample);
+ if (ret < 0) {
+ dev_err(icd->dev.parent,
+ "Fail to try the bus parameters.\n");
+ return 0;
+ }
+
+ switch (code) {
+ case V4L2_MBUS_FMT_UYVY8_2X8:
+ case V4L2_MBUS_FMT_VYUY8_2X8:
+ case V4L2_MBUS_FMT_YUYV8_2X8:
+ case V4L2_MBUS_FMT_YVYU8_2X8:
+ formats++;
+ if (xlate) {
+ xlate->host_fmt = &isi_camera_formats[0];
+ xlate->code = code;
+ xlate++;
+ dev_dbg(icd->dev.parent, "Providing format %s using code %d\n",
+ isi_camera_formats[0].name, code);
+ }
+ break;
+ default:
+ if (!isi_camera_packing_supported(fmt))
+ return 0;
+ if (xlate)
+ dev_dbg(dev,
+ "Providing format %s in pass-through mode\n",
+ fmt->name);
+ }
+
+ /* Generic pass-through */
+ formats++;
+ if (xlate) {
+ xlate->host_fmt = fmt;
+ xlate->code = code;
+ xlate++;
+ }
+
+ return formats;
+}
+
+/* Called with .video_lock held */
+static int isi_camera_add_device(struct soc_camera_device *icd)
+{
+ struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+ struct atmel_isi *isi = ici->priv;
+ int ret;
+
+ if (isi->icd)
+ return -EBUSY;
+
+ ret = atmel_isi_init(isi);
+ if (ret)
+ return ret;
+
+ isi->icd = icd;
+ dev_info(icd->dev.parent, "Atmel ISI Camera driver attached to camera %d\n",
+ icd->devnum);
+ return 0;
+}
+/* Called with .video_lock held */
+static void isi_camera_remove_device(struct soc_camera_device *icd)
+{
+ struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+ struct atmel_isi *isi = ici->priv;
+
+ BUG_ON(icd != isi->icd);
+
+ isi_writel(isi, V2_CTRL, ISI_BIT(V2_DIS));
+ /* Check if module disable */
+ while (isi_readl(isi, V2_STATUS) & ISI_BIT(V2_DIS))
+ cpu_relax();
+
+ isi->icd = NULL;
+
+ dev_info(icd->dev.parent, "Atmel ISI Camera driver detached from camera %d\n",
+ icd->devnum);
+}
+
+static unsigned int isi_camera_poll(struct file *file, poll_table *pt)
+{
+ struct soc_camera_device *icd = file->private_data;
+
+ return vb2_poll(&icd->vb2_vidq, file, pt);
+}
+
+static int isi_camera_querycap(struct soc_camera_host *ici,
+ struct v4l2_capability *cap)
+{
+ strcpy(cap->driver, "atmel-isi");
+ strcpy(cap->card, "Atmel Image Sensor Interface");
+ cap->version = ATMEL_ISI_VERSION;
+
+ cap->capabilities = (V4L2_CAP_VIDEO_CAPTURE
+ | V4L2_CAP_READWRITE
+ | V4L2_CAP_STREAMING
+ );
+ return 0;
+}
+
+static int isi_camera_set_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
+{
+ struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+ struct atmel_isi *isi = ici->priv;
+ unsigned long bus_flags, camera_flags, common_flags;
+ const struct soc_mbus_pixelfmt *fmt;
+ int ret;
+ u32 cfg1, ctrl;
+
+ fmt = soc_mbus_get_fmtdesc(icd->current_fmt->code);
+ if (!fmt)
+ return -EINVAL;
+
+ ret = test_platform_param(isi, fmt->bits_per_sample, &bus_flags);
+ if (ret < 0)
+ return ret;
+
+ camera_flags = icd->ops->query_bus_param(icd);
+
+ common_flags = soc_camera_bus_param_compatible(camera_flags, bus_flags);
+ dev_dbg(icd->dev.parent, "Flags cam: 0x%lx host: 0x%lx common: 0x%lx\n",
+ camera_flags, bus_flags, common_flags);
+ if (!common_flags)
+ return -EINVAL;
+
+ /* Make choises, based on platform preferences */
+ if ((common_flags & SOCAM_HSYNC_ACTIVE_HIGH) &&
+ (common_flags & SOCAM_HSYNC_ACTIVE_LOW)) {
+ if (isi->pdata->hsync_act_low)
+ common_flags &= ~SOCAM_HSYNC_ACTIVE_HIGH;
+ else
+ common_flags &= ~SOCAM_HSYNC_ACTIVE_LOW;
+ }
+
+ if ((common_flags & SOCAM_VSYNC_ACTIVE_HIGH) &&
+ (common_flags & SOCAM_VSYNC_ACTIVE_LOW)) {
+ if (isi->pdata->vsync_act_low)
+ common_flags &= ~SOCAM_VSYNC_ACTIVE_HIGH;
+ else
+ common_flags &= ~SOCAM_VSYNC_ACTIVE_LOW;
+ }
+
+ if ((common_flags & SOCAM_PCLK_SAMPLE_RISING) &&
+ (common_flags & SOCAM_PCLK_SAMPLE_FALLING)) {
+ if (isi->pdata->pclk_act_falling)
+ common_flags &= ~SOCAM_PCLK_SAMPLE_RISING;
+ else
+ common_flags &= ~SOCAM_PCLK_SAMPLE_FALLING;
+ }
+
+ ret = icd->ops->set_bus_param(icd, common_flags);
+ if (ret < 0) {
+ dev_dbg(icd->dev.parent, "camera set_bus_param(%lx) returned %d\n",
+ common_flags, ret);
+ return ret;
+ }
+
+ /* set bus param for ISI */
+ if (common_flags & SOCAM_PCLK_SAMPLE_FALLING)
+ isi->pdata->pclk_act_falling = 1;
+ if (common_flags & SOCAM_HSYNC_ACTIVE_LOW)
+ isi->pdata->hsync_act_low = 1;
+ if (common_flags & SOCAM_VSYNC_ACTIVE_LOW)
+ isi->pdata->vsync_act_low = 1;
+
+ cfg1 = ISI_BF(V2_EMB_SYNC, (isi->pdata->has_emb_sync))
+ | ISI_BF(V2_HSYNC_POL, isi->pdata->hsync_act_low)
+ | ISI_BF(V2_VSYNC_POL, isi->pdata->vsync_act_low)
+ | ISI_BF(V2_PIXCLK_POL, isi->pdata->pclk_act_falling)
+ | ISI_BF(V2_FULL, isi->pdata->isi_full_mode);
+
+ ctrl = ISI_BIT(DIS);
+ isi_writel(isi, V2_CFG1, cfg1);
+ isi_writel(isi, V2_CTRL, ctrl);
+
+ return 0;
+}
+
+static struct soc_camera_host_ops isi_soc_camera_host_ops = {
+ .owner = THIS_MODULE,
+ .add = isi_camera_add_device,
+ .remove = isi_camera_remove_device,
+ .set_fmt = isi_camera_set_fmt,
+ .try_fmt = isi_camera_try_fmt,
+ .get_formats = isi_camera_get_formats,
+ .init_videobuf2 = isi_camera_init_videobuf,
+ .poll = isi_camera_poll,
+ .querycap = isi_camera_querycap,
+ .set_bus_param = isi_camera_set_bus_param,
+};
+
+/* -----------------------------------------------------------------------*/
+static int __exit atmel_isi_remove(struct platform_device *pdev)
+{
+ struct soc_camera_host *soc_host = to_soc_camera_host(&pdev->dev);
+ struct atmel_isi *isi = container_of(soc_host,
+ struct atmel_isi, soc_host);
+
+ soc_camera_host_unregister(soc_host);
+ vb2_dma_contig_cleanup_ctx(isi->alloc_ctx);
+
+ free_irq(isi->irq, isi);
+ iounmap(isi->regs);
+ clk_disable(isi->pclk);
+ clk_put(isi->pclk);
+
+ return 0;
+}
+
+static int __init atmel_isi_probe(struct platform_device *pdev)
+{
+ unsigned int irq;
+ struct atmel_isi *isi;
+ struct clk *pclk;
+ struct resource *regs;
+ int ret;
+ struct device *dev = &pdev->dev;
+ struct isi_platform_data *pdata;
+ struct soc_camera_host *soc_host;
+
+ regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!regs)
+ return -ENXIO;
+
+ pclk = clk_get(&pdev->dev, "isi_clk");
+ if (IS_ERR(pclk))
+ return PTR_ERR(pclk);
+
+ clk_enable(pclk);
+
+ isi = kzalloc(sizeof(struct atmel_isi), GFP_KERNEL);
+ if (!isi) {
+ ret = -ENOMEM;
+ dev_err(&pdev->dev, "can't allocate interface!\n");
+ goto err_alloc_isi;
+ }
+
+ isi->pclk = pclk;
+
+ spin_lock_init(&isi->lock);
+ init_waitqueue_head(&isi->capture_wq);
+
+ isi->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev);
+ if (IS_ERR(isi->alloc_ctx)) {
+ ret = PTR_ERR(isi->alloc_ctx);
+ goto err_alloc_isi;
+ }
+
+ isi->regs = ioremap(regs->start, resource_size(regs));
+ if (!isi->regs) {
+ ret = -ENOMEM;
+ goto err_ioremap;
+ }
+
+ if (dev->platform_data)
+ pdata = (struct isi_platform_data *) dev->platform_data;
+ else {
+ static struct isi_platform_data isi_default_data = {
+ .frate = 0,
+ .has_emb_sync = 0,
+ .emb_crc_sync = 0,
+ .hsync_act_low = 0,
+ .vsync_act_low = 0,
+ .pclk_act_falling = 0,
+ .isi_full_mode = 1,
+ /* to use codec and preview path simultaneously */
+ .flags = ISI_DATAWIDTH_8 |
+ ISI_DATAWIDTH_10,
+ };
+ dev_info(&pdev->dev,
+ "No config available using default values\n");
+ pdata = &isi_default_data;
+ }
+
+ isi->pdata = pdata;
+ isi->platform_flags = pdata->flags;
+ if (isi->platform_flags == 0)
+ isi->platform_flags = ISI_DATAWIDTH_8;
+
+ isi_writel(isi, V2_CTRL, ISI_BIT(V2_DIS));
+ /* Check if module disable */
+ while (isi_readl(isi, V2_STATUS) & ISI_BIT(V2_DIS))
+ cpu_relax();
+
+ irq = platform_get_irq(pdev, 0);
+ ret = request_irq(irq, isi_interrupt, 0, "isi", isi);
+ if (ret) {
+ dev_err(&pdev->dev, "unable to request irq %d\n", irq);
+ goto err_req_irq;
+ }
+ isi->irq = irq;
+
+ INIT_LIST_HEAD(&isi->video_buffer_list);
+ isi->still_capture = 0;
+ isi->active = NULL;
+
+ soc_host = &isi->soc_host;
+ soc_host->drv_name = "isi-camera";
+ soc_host->ops = &isi_soc_camera_host_ops;
+ soc_host->priv = isi;
+ soc_host->v4l2_dev.dev = &pdev->dev;
+ soc_host->nr = pdev->id;
+
+ ret = soc_camera_host_register(soc_host);
+ if (ret) {
+ dev_err(&pdev->dev, "unable to register soc camera host\n");
+ goto err_register_soc_camera_host;
+ }
+ return 0;
+
+err_register_soc_camera_host:
+ free_irq(isi->irq, isi);
+err_req_irq:
+ iounmap(isi->regs);
+err_ioremap:
+ vb2_dma_contig_cleanup_ctx(isi->alloc_ctx);
+ kfree(isi);
+err_alloc_isi:
+ clk_disable(pclk);
+
+ return ret;
+}
+
+static struct platform_driver atmel_isi_driver = {
+ .probe = atmel_isi_probe,
+ .remove = __exit_p(atmel_isi_remove),
+ .driver = {
+ .name = "atmel_isi",
+ .owner = THIS_MODULE,
+ },
+};
+
+static int __init atmel_isi_init_module(void)
+{
+ return platform_driver_probe(&atmel_isi_driver, &atmel_isi_probe);
+}
+
+static void __exit atmel_isi_exit(void)
+{
+ platform_driver_unregister(&atmel_isi_driver);
+}
+module_init(atmel_isi_init_module);
+module_exit(atmel_isi_exit);
+
+MODULE_AUTHOR("Josh Wu <[email protected]>");
+MODULE_DESCRIPTION("The V4L2 driver for Atmel Linux");
+MODULE_LICENSE("GPL");
+MODULE_SUPPORTED_DEVICE("video");
--
1.6.3.3
On Thu, May 12, 2011 at 03:42:18PM +0800, Josh Wu wrote:
> +err_alloc_isi:
> + clk_disable(pclk);
clk_put() ?
Hi, Russell
From: Russell King - ARM Linux [mailto:[email protected]] Sent: Thursday, May 12, 2011 3:47 PM
> On Thu, May 12, 2011 at 03:42:18PM +0800, Josh Wu wrote:
>> +err_alloc_isi:
>> + clk_disable(pclk);
> clk_put() ?
Ok, will be fixed in V2 patch. Thanks.
Best Regards,
Josh Wu
On Thu, 12 May 2011, Wu, Josh wrote:
> Hi, Russell
>
> From: Russell King - ARM Linux [mailto:[email protected]] Sent: Thursday, May 12, 2011 3:47 PM
> > On Thu, May 12, 2011 at 03:42:18PM +0800, Josh Wu wrote:
> >> +err_alloc_isi:
> >> + clk_disable(pclk);
> > clk_put() ?
> Ok, will be fixed in V2 patch. Thanks.
You might wait with v2 until I find time to review your patch. Will take a
couple of days, I think. A general question, though: how compatible is
this driver with the AVR32?
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
On Thu, May 12, 2011 at 03:42:18PM +0800, Josh Wu wrote:
> This patch is to enable Atmel Image Sensor Interface (ISI) driver support.
> - Using soc-camera framework with videobuf2 dma-contig allocator
> - Supporting video streaming of YUV packed format
> - Tested on AT91SAM9M10G45-EK with OV2640
A few more points...
> +static int __init atmel_isi_probe(struct platform_device *pdev)
Should be __devinit otherwise you'll have section errors.
> +{
> + unsigned int irq;
> + struct atmel_isi *isi;
> + struct clk *pclk;
> + struct resource *regs;
> + int ret;
> + struct device *dev = &pdev->dev;
> + struct isi_platform_data *pdata;
> + struct soc_camera_host *soc_host;
> +
> + regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!regs)
> + return -ENXIO;
> +
> + pclk = clk_get(&pdev->dev, "isi_clk");
> + if (IS_ERR(pclk))
> + return PTR_ERR(pclk);
> +
> + clk_enable(pclk);
Return value of clk_enable() should be checked.
> +
> + isi = kzalloc(sizeof(struct atmel_isi), GFP_KERNEL);
> + if (!isi) {
> + ret = -ENOMEM;
> + dev_err(&pdev->dev, "can't allocate interface!\n");
> + goto err_alloc_isi;
> + }
> +
> + isi->pclk = pclk;
> +
> + spin_lock_init(&isi->lock);
> + init_waitqueue_head(&isi->capture_wq);
> +
> + isi->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev);
> + if (IS_ERR(isi->alloc_ctx)) {
> + ret = PTR_ERR(isi->alloc_ctx);
> + goto err_alloc_isi;
> + }
> +
> + isi->regs = ioremap(regs->start, resource_size(regs));
> + if (!isi->regs) {
> + ret = -ENOMEM;
> + goto err_ioremap;
> + }
> +
> + if (dev->platform_data)
> + pdata = (struct isi_platform_data *) dev->platform_data;
> + else {
> + static struct isi_platform_data isi_default_data = {
> + .frate = 0,
> + .has_emb_sync = 0,
> + .emb_crc_sync = 0,
> + .hsync_act_low = 0,
> + .vsync_act_low = 0,
> + .pclk_act_falling = 0,
> + .isi_full_mode = 1,
> + /* to use codec and preview path simultaneously */
> + .flags = ISI_DATAWIDTH_8 |
> + ISI_DATAWIDTH_10,
> + };
> + dev_info(&pdev->dev,
> + "No config available using default values\n");
> + pdata = &isi_default_data;
> + }
> +
> + isi->pdata = pdata;
> + isi->platform_flags = pdata->flags;
> + if (isi->platform_flags == 0)
> + isi->platform_flags = ISI_DATAWIDTH_8;
> +
> + isi_writel(isi, V2_CTRL, ISI_BIT(V2_DIS));
> + /* Check if module disable */
> + while (isi_readl(isi, V2_STATUS) & ISI_BIT(V2_DIS))
> + cpu_relax();
> +
> + irq = platform_get_irq(pdev, 0);
This should also be checked.
From: Guennadi Liakhovetski [mailto:[email protected]] Sent: Thursday, May 12, 2011 5:32 PM
> On Thu, 12 May 2011, Wu, Josh wrote:
>> Hi, Russell
>>
>> From: Russell King - ARM Linux [mailto:[email protected]] Sent: Thursday, May 12, 2011 3:47 PM
>> > On Thu, May 12, 2011 at 03:42:18PM +0800, Josh Wu wrote:
>> >> +err_alloc_isi:
>> >> + clk_disable(pclk);
>> > clk_put() ?
>> Ok, will be fixed in V2 patch. Thanks.
> You might wait with v2 until I find time to review your patch. Will take a
> couple of days, I think. A general question, though: how compatible is
> this driver with the AVR32?
That is ok.
For AVR32, I think I need time to check with AVR team. I will update the status when I got more information.
Best Regards,
Josh Wu
> +struct atmel_isi;
do we really this here?
> +
> +enum atmel_isi_pixfmt {
> + ATMEL_ISI_PIXFMT_GREY, /* Greyscale */
> + ATMEL_ISI_PIXFMT_CbYCrY,
> + ATMEL_ISI_PIXFMT_CrYCbY,
> + ATMEL_ISI_PIXFMT_YCbYCr,
> + ATMEL_ISI_PIXFMT_YCrYCb,
> + ATMEL_ISI_PIXFMT_RGB24,
> + ATMEL_ISI_PIXFMT_BGR24,
> + ATMEL_ISI_PIXFMT_RGB16,
> + ATMEL_ISI_PIXFMT_BGR16,
> + ATMEL_ISI_PIXFMT_GRB16, /* G[2:0] R[4:0]/B[4:0] G[5:3] */
> + ATMEL_ISI_PIXFMT_GBR16, /* G[2:0] B[4:0]/R[4:0] G[5:3] */
> + ATMEL_ISI_PIXFMT_RGB24_REV,
> + ATMEL_ISI_PIXFMT_BGR24_REV,
> + ATMEL_ISI_PIXFMT_RGB16_REV,
> + ATMEL_ISI_PIXFMT_BGR16_REV,
> + ATMEL_ISI_PIXFMT_GRB16_REV, /* G[2:0] R[4:0]/B[4:0] G[5:3] */
> + ATMEL_ISI_PIXFMT_GBR16_REV, /* G[2:0] B[4:0]/R[4:0] G[5:3] */
> +};
> +
> +struct isi_platform_data {
> + u8 has_emb_sync;
> + u8 emb_crc_sync;
> + u8 hsync_act_low;
> + u8 vsync_act_low;
> + u8 pclk_act_falling;
> + u8 isi_full_mode;
> +#define ISI_HSYNC_ACT_LOW 0x01
> +#define ISI_VSYNC_ACT_LOW 0x02
> +#define ISI_PXCLK_ACT_FALLING 0x04
> +#define ISI_EMB_SYNC 0x08
> +#define ISI_CRC_SYNC 0x10
> +#define ISI_FULL 0x20
> +#define ISI_DATAWIDTH_8 0x40
> +#define ISI_DATAWIDTH_10 0x80
> + u32 flags;
> + u8 gs_mode;
> +#define ISI_GS_2PIX_PER_WORD 0x00
> +#define ISI_GS_1PIX_PER_WORD 0x01
> + u8 pixfmt;
> + u8 sfd;
> + u8 sld;
> + u8 thmask;
> +#define ISI_BURST_4_8_16 0x00
> +#define ISI_BURST_8_16 0x01
> +#define ISI_BURST_16 0x02
> + u8 frate;
> +#define ISI_FRATE_DIV_2 0x01
> +#define ISI_FRATE_DIV_3 0x02
> +#define ISI_FRATE_DIV_4 0x03
> +#define ISI_FRATE_DIV_5 0x04
> +#define ISI_FRATE_DIV_6 0x05
> +#define ISI_FRATE_DIV_7 0x06
> +#define ISI_FRATE_DIV_8 0x07
> +};
> +
> +#endif /* __AT91_ISI_H__ */
> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
> index d61414e..eae6005 100644
> --- a/drivers/media/video/Kconfig
> +++ b/drivers/media/video/Kconfig
> @@ -80,6 +80,16 @@ menuconfig VIDEO_CAPTURE_DRIVERS
> Some of those devices also supports FM radio.
>
> if VIDEO_CAPTURE_DRIVERS && VIDEO_V4L2
> +config VIDEO_ATMEL_ISI
> + tristate "ATMEL Image Sensor Interface (ISI) support"
> + depends on VIDEO_DEV && SOC_CAMERA
depends on AT91 if the drivers is at91 specific or avr32 otherwise
> + select VIDEOBUF2_DMA_CONTIG
> + default n
it's n by default please remove
> + ---help---
> + This module makes the ATMEL Image Sensor Interface available
> + as a v4l2 device.
> + Say Y here to enable selecting the Image Sensor Interface.
> + When in doubt, say N.
>
> config VIDEO_ADV_DEBUG
> bool "Enable advanced debug functionality"
> diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
> index a10e4c3..f734a65 100644
> --- a/drivers/media/video/Makefile
> +++ b/drivers/media/video/Makefile
> @@ -166,6 +166,7 @@ obj-$(CONFIG_VIDEO_SH_MOBILE_CSI2) += sh_mobile_csi2.o
> obj-$(CONFIG_VIDEO_SH_MOBILE_CEU) += sh_mobile_ceu_camera.o
> obj-$(CONFIG_VIDEO_OMAP1) += omap1_camera.o
> obj-$(CONFIG_VIDEO_SAMSUNG_S5P_FIMC) += s5p-fimc/
> +obj-$(CONFIG_VIDEO_ATMEL_ISI) += atmel-isi.o
>
> obj-$(CONFIG_ARCH_DAVINCI) += davinci/
>
> diff --git a/drivers/media/video/atmel-isi.c b/drivers/media/video/atmel-isi.c
> new file mode 100644
> index 0000000..33d0b83
> --- /dev/null
> +++ b/drivers/media/video/atmel-isi.c
> @@ -0,0 +1,1089 @@
> +/*
> + * Copyright (c) 2011 Atmel Corporation
> + *
> + * Based on previous work by Lars Haring and Sedji Gaouaou
> + *
> + * Based on the bttv driver for Bt848 with respective copyright holders
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/completion.h>
> +#include <linux/fs.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/version.h>
> +#include <linux/kfifo.h>
> +
> +#include <mach/board.h>
> +#include <mach/cpu.h>
> +#include <mach/at91_isi.h>
> +
> +#include <media/videobuf2-dma-contig.h>
> +#include <media/soc_camera.h>
> +#include <media/soc_mediabus.h>
> +
> +#define ATMEL_ISI_VERSION KERNEL_VERSION(1, 0, 0)
> +#define MAX_BUFFER_NUMS 32
> +#define MAX_SUPPORT_WIDTH 2048
> +#define MAX_SUPPORT_HEIGHT 2048
> +
> +static unsigned int vid_limit = 16;
> +
> +enum isi_buffer_state {
> + ISI_BUF_NEEDS_INIT,
> + ISI_BUF_PREPARED,
> +};
> +
> +/* Single frame capturing states */
> +enum {
> + STATE_IDLE = 0,
> + STATE_CAPTURE_READY,
> + STATE_CAPTURE_WAIT_SOF,
> + STATE_CAPTURE_IN_PROGRESS,
> + STATE_CAPTURE_DONE,
> + STATE_CAPTURE_ERROR,
> +};
> +
> +/* Frame buffer descriptor
> + * Used by the ISI module as a linked list for the DMA controller.
> + */
> +struct fbd {
> + /* Physical address of the frame buffer */
> + u32 fb_address;
> +#if defined(CONFIG_ARCH_AT91SAM9G45) ||\
> + defined(CONFIG_ARCH_AT91SAM9X5)
> + /* DMA Control Register(only in HISI2) */
> + u32 dma_ctrl;
> +#endif
no ifdef in the struct
> + /* Physical address of the next fbd */
> + u32 next_fbd_address;
> +};
> +
> +#if defined(CONFIG_ARCH_AT91SAM9G45) ||\
> + defined(CONFIG_ARCH_AT91SAM9X5)
> +static void set_dma_ctrl(struct fbd *fb_desc, u32 ctrl)
> +{
> + fb_desc->dma_ctrl = ctrl;
> +}
> +#else
> +static void set_dma_ctrl(struct fbd *fb_desc, u32 ctrl) { }
> +#endif
no ifdef here also as we want to have multi soc support
> +
> +/* Frame buffer data
> + */
> +struct frame_buffer {
> + struct vb2_buffer vb;
> + struct fbd fb_desc;
> + /* Frame number of the frame */
> + unsigned long sequence;
> +
> + enum isi_buffer_state dma_desc_status;
> + struct list_head list;
> +};
> +
> +struct atmel_isi {
> + /* ISI module spin lock. Protects against concurrent access of variables
> + * that are shared with the ISR */
> + spinlock_t lock;
> + void __iomem *regs;
> +
> + /* If set ISI is in still capture mode */
> + int still_capture;
> + int sequence;
> + /* State of the ISI module in capturing mode */
> + int state;
> +
> + /* Capture/streaming wait queue for waiting for SOF */
> + wait_queue_head_t capture_wq;
> +
> + struct v4l2_device v4l2_dev;
> +
> + struct vb2_alloc_ctx *alloc_ctx;
> +
> + struct clk *pclk;
> + struct platform_device *pdev;
do you really need to store the pdev?
> + unsigned int irq;
> +
> + struct isi_platform_data *pdata;
> + unsigned long platform_flags;
> +
> + struct list_head video_buffer_list;
> + struct frame_buffer *active;
> +
> + struct soc_camera_device *icd;
> + struct soc_camera_host soc_host;
> +};
> +
> +static int configure_geometry(struct atmel_isi *isi, u32 width,
> + u32 height, enum v4l2_mbus_pixelcode code)
> +{
> + u32 cfg2, cr, ctrl;
> +
> + cr = 0;
please move this in default
> + switch (code) {
> + /* YUV, including grey */
> + case V4L2_MBUS_FMT_Y8_1X8:
> + cr = ISI_BIT(GRAYSCALE);
> + break;
> + case V4L2_MBUS_FMT_UYVY8_2X8:
> + cr = ISI_BF(V2_YCC_SWAP, 3);
> + break;
> + case V4L2_MBUS_FMT_VYUY8_2X8:
> + cr = ISI_BF(V2_YCC_SWAP, 2);
> + break;
> + case V4L2_MBUS_FMT_YUYV8_2X8:
> + cr = ISI_BF(V2_YCC_SWAP, 1);
> + break;
> + case V4L2_MBUS_FMT_YVYU8_2X8:
> + cr = ISI_BF(V2_YCC_SWAP, 0);
> + break;
> + /* RGB, TODO */
> + default:
> + return -EINVAL;
> + }
> +
> + ctrl = ISI_BIT(DIS);
> + isi_writel(isi, V2_CTRL, ctrl);
> + /* Check if module properly disable */
> + while (isi_readl(isi, V2_STATUS) & ISI_BIT(V2_DIS_DONE))
> + cpu_relax();
> +
> + cfg2 = isi_readl(isi, V2_CFG2);
> + cfg2 |= cr;
> + cfg2 = ISI_BFINS(V2_IM_VSIZE, height - 1, cfg2);
> + cfg2 = ISI_BFINS(V2_IM_HSIZE, width - 1, cfg2);
> + isi_writel(isi, V2_CFG2, cfg2);
> +
> + return 0;
> +}
> +
> +
> + size = bytes_per_line * icd->user_height;
> +
> + if (0 == *nbuffers)
please invert the test
> + *nbuffers = MAX_BUFFER_NUMS;
> + if (*nbuffers > MAX_BUFFER_NUMS)
> + *nbuffers = MAX_BUFFER_NUMS;
> +
> + while (size * *nbuffers > vid_limit * 1024 * 1024)
> + (*nbuffers)--;
> +
> + *nplanes = 1;
> + sizes[0] = size;
> + alloc_ctxs[0] = dev->alloc_ctx;
> +
> + dev->sequence = 0;
> + dev->active = NULL;
> +
> + dev_dbg(icd->dev.parent, "%s, count=%d, size=%ld\n", __func__,
> + *nbuffers, size);
> +
> + return 0;
> +}
> +
> +
> +static void start_dma(struct atmel_isi *isi, struct frame_buffer *buffer)
> +{
> + u32 ctrl, cfg1;
please add ine ligne here
> + ctrl = isi_readl(isi, V2_CTRL);
> + cfg1 = isi_readl(isi, V2_CFG1);
> + /* Enable irq: cxfr for the codec path, pxfr for the preview path */
> + isi_writel(isi, V2_INTEN,
> + ISI_BIT(V2_CXFR_DONE) | ISI_BIT(V2_PXFR_DONE));
> +
> + /* Enable codec path */
> + ctrl |= ISI_BIT(V2_CDC);
> + /* Check if already in a frame */
> + while (isi_readl(isi, V2_STATUS) & ISI_BIT(V2_CDC))
> + cpu_relax();
no timeout?
> +
> + /* Write the address of the first frame buffer in the C_ADDR reg
> + * write the address of the first descriptor(link list of buffer)
> + * in the C_DSCR reg, and enable dma channel.
> + */
> + isi_writel(isi, V2_DMA_C_DSCR, (__pa(&(buffer->fb_desc))));
> + isi_writel(isi, V2_DMA_C_CTRL,
> + ISI_BIT(V2_DMA_FETCH) | ISI_BIT(V2_DMA_DONE));
> + isi_writel(isi, V2_DMA_CHER, ISI_BIT(V2_DMA_C_CH_EN));
> +
> + /* Enable linked list */
> + cfg1 |= ISI_BF(V2_FRATE, isi->pdata->frate) | ISI_BIT(V2_DISCR);
> +
> + /* Enable ISI module*/
> + ctrl |= ISI_BIT(V2_ENABLE);
> + isi_writel(isi, V2_CTRL, ctrl);
> + isi_writel(isi, V2_CFG1, cfg1);
> +}
> +
> +
> +/* abort streaming and wait for last buffer */
> +static int stop_streaming(struct vb2_queue *vq)
> +{
> + struct soc_camera_device *icd = soc_camera_from_vb2q(vq);
> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> + struct atmel_isi *isi = ici->priv;
> +
> + spin_lock_irq(&isi->lock);
> + isi->still_capture = 0;
> + isi->active = NULL;
> +
> + while (isi_readl(isi, V2_STATUS) & ISI_BIT(V2_CDC))
> + cpu_relax();
ditto
> +
> + /* Disble codec path */
> + isi_writel(isi, V2_CTRL, isi_readl(isi, V2_CTRL) & (~ISI_BIT(V2_CDC)));
> + /* Disable interrupts */
> + isi_writel(isi, V2_INTDIS,
> + ISI_BIT(V2_CXFR_DONE) | ISI_BIT(V2_PXFR_DONE));
> + /* Disable ISI module*/
> + isi_writel(isi, V2_CTRL, isi_readl(isi, V2_CTRL) | ISI_BIT(V2_DIS));
> +
> +
> +static int __init atmel_isi_probe(struct platform_device *pdev)
> +{
> + unsigned int irq;
> + struct atmel_isi *isi;
> + struct clk *pclk;
> + struct resource *regs;
> + int ret;
> + struct device *dev = &pdev->dev;
> + struct isi_platform_data *pdata;
> + struct soc_camera_host *soc_host;
> +
> + regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!regs)
> + return -ENXIO;
> +
> + pclk = clk_get(&pdev->dev, "isi_clk");
> + if (IS_ERR(pclk))
> + return PTR_ERR(pclk);
> +
> + clk_enable(pclk);
do we really need to always enable the clock?
normally we need to enable it just when we use the device and disable asap
do you plane toadd the pm?
Best Regards,
J.
On Thu, 12 May 2011, Jean-Christophe PLAGNIOL-VILLARD wrote:
[snip]
> > + if (0 == *nbuffers)
> please invert the test
Don't think this is required by CodingStyle or anything like that. If it
were, you'd have to revamp half of the kernel.
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
On 05/12/2011 07:42 PM, Josh Wu wrote:
> This patch is to enable Atmel Image Sensor Interface (ISI) driver support.
> - Using soc-camera framework with videobuf2 dma-contig allocator
> - Supporting video streaming of YUV packed format
> - Tested on AT91SAM9M10G45-EK with OV2640
Hi Josh,
Thansk for doing this. Overall the patch looks really good. A few
comments below.
~Ryan
>
> Signed-off-by: Josh Wu <[email protected]>
> ---
> base on branch staging/for_v2.6.40
>
> arch/arm/mach-at91/include/mach/at91_isi.h | 454 ++++++++++++
> drivers/media/video/Kconfig | 10 +
> drivers/media/video/Makefile | 1 +
> drivers/media/video/atmel-isi.c | 1089 ++++++++++++++++++++++++++++
> 4 files changed, 1554 insertions(+), 0 deletions(-)
> create mode 100644 arch/arm/mach-at91/include/mach/at91_isi.h
> create mode 100644 drivers/media/video/atmel-isi.c
>
> diff --git a/arch/arm/mach-at91/include/mach/at91_isi.h b/arch/arm/mach-at91/include/mach/at91_isi.h
> new file mode 100644
> index 0000000..3219358
> --- /dev/null
> +++ b/arch/arm/mach-at91/include/mach/at91_isi.h
> @@ -0,0 +1,454 @@
> +/*
> + * Register definitions for the Atmel Image Sensor Interface.
> + *
> + * Copyright (C) 2011 Atmel Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#ifndef __AT91_ISI_H__
> +#define __AT91_ISI_H__
> +
> +#include <linux/videodev2.h>
> +#include <linux/i2c.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-common.h>
> +#include <media/v4l2-ioctl.h>
> +#include <media/v4l2-chip-ident.h>
> +
> +/* ISI register offsets */
> +#define ISI_CR1 0x0000
> +#define ISI_CR2 0x0004
> +#define ISI_SR 0x0008
> +#define ISI_IER 0x000c
> +#define ISI_IDR 0x0010
> +#define ISI_IMR 0x0014
> +#define ISI_PSIZE 0x0020
> +#define ISI_PDECF 0x0024
> +#define ISI_PPFBD 0x0028
> +#define ISI_CDBA 0x002c
> +#define ISI_Y2R_SET0 0x0030
> +#define ISI_Y2R_SET1 0x0034
> +#define ISI_R2Y_SET0 0x0038
> +#define ISI_R2Y_SET1 0x003c
> +#define ISI_R2Y_SET2 0x0040
> +
> +/* ISI_V2 register offsets */
> +#define ISI_V2_CFG1 0x0000
> +#define ISI_V2_CFG2 0x0004
> +#define ISI_V2_PSIZE 0x0008
> +#define ISI_V2_PDECF 0x000c
> +#define ISI_V2_Y2R_SET0 0x0010
> +#define ISI_V2_Y2R_SET1 0x0014
> +#define ISI_V2_R2Y_SET0 0x0018
> +#define ISI_V2_R2Y_SET1 0x001C
> +#define ISI_V2_R2Y_SET2 0x0020
> +#define ISI_V2_CTRL 0x0024
> +#define ISI_V2_STATUS 0x0028
> +#define ISI_V2_INTEN 0x002C
> +#define ISI_V2_INTDIS 0x0030
> +#define ISI_V2_INTMASK 0x0034
> +#define ISI_V2_DMA_CHER 0x0038
> +#define ISI_V2_DMA_CHDR 0x003C
> +#define ISI_V2_DMA_CHSR 0x0040
> +#define ISI_V2_DMA_P_ADDR 0x0044
> +#define ISI_V2_DMA_P_CTRL 0x0048
> +#define ISI_V2_DMA_P_DSCR 0x004C
> +#define ISI_V2_DMA_C_ADDR 0x0050
> +#define ISI_V2_DMA_C_CTRL 0x0054
> +#define ISI_V2_DMA_C_DSCR 0x0058
> +
> +/* Bitfields in CR1 */
> +#define ISI_RST_OFFSET 0
> +#define ISI_RST_SIZE 1
> +#define ISI_DIS_OFFSET 1
> +#define ISI_DIS_SIZE 1
> +#define ISI_HSYNC_POL_OFFSET 2
> +#define ISI_HSYNC_POL_SIZE 1
> +#define ISI_VSYNC_POL_OFFSET 3
> +#define ISI_VSYNC_POL_SIZE 1
> +#define ISI_PIXCLK_POL_OFFSET 4
> +#define ISI_PIXCLK_POL_SIZE 1
> +#define ISI_EMB_SYNC_OFFSET 6
> +#define ISI_EMB_SYNC_SIZE 1
> +#define ISI_CRC_SYNC_OFFSET 7
> +#define ISI_CRC_SYNC_SIZE 1
> +#define ISI_FRATE_OFFSET 8
> +#define ISI_FRATE_SIZE 3
> +#define ISI_FULL_OFFSET 12
> +#define ISI_FULL_SIZE 1
> +#define ISI_THMASK_OFFSET 13
> +#define ISI_THMASK_SIZE 2
> +#define ISI_CODEC_ON_OFFSET 15
> +#define ISI_CODEC_ON_SIZE 1
> +#define ISI_SLD_OFFSET 16
> +#define ISI_SLD_SIZE 8
> +#define ISI_SFD_OFFSET 24
> +#define ISI_SFD_SIZE 8
> +
> +/* Bitfields in CFG1 */
> +#define ISI_V2_HSYNC_POL_OFFSET 2
> +#define ISI_V2_HSYNC_POL_SIZE 1
> +#define ISI_V2_VSYNC_POL_OFFSET 3
> +#define ISI_V2_VSYNC_POL_SIZE 1
> +#define ISI_V2_PIXCLK_POL_OFFSET 4
> +#define ISI_V2_PIXCLK_POL_SIZE 1
> +#define ISI_V2_EMB_SYNC_OFFSET 6
> +#define ISI_V2_EMB_SYNC_SIZE 1
> +#define ISI_V2_CRC_SYNC_OFFSET 7
> +#define ISI_V2_CRC_SYNC_SIZE 1
> +#define ISI_V2_FRATE_OFFSET 8
> +#define ISI_V2_FRATE_SIZE 3
> +#define ISI_V2_DISCR_OFFSET 11
> +#define ISI_V2_DISCR_SIZE 1
> +#define ISI_V2_FULL_OFFSET 12
> +#define ISI_V2_FULL_SIZE 1
> +#define ISI_V2_THMASK_OFFSET 13
> +#define ISI_V2_THMASK_SIZE 2
> +#define ISI_V2_SLD_OFFSET 16
> +#define ISI_V2_SLD_SIZE 8
> +#define ISI_V2_SFD_OFFSET 24
> +#define ISI_V2_SFD_SIZE 8
> +
> +/* Bitfields in CR2 */
> +#define ISI_IM_VSIZE_OFFSET 0
> +#define ISI_IM_VSIZE_SIZE 11
> +#define ISI_GS_MODE_OFFSET 11
> +#define ISI_GS_MODE_SIZE 1
> +#define ISI_RGB_MODE_OFFSET 12
> +#define ISI_RGB_MODE_SIZE 1
> +#define ISI_GRAYSCALE_OFFSET 13
> +#define ISI_GRAYSCALE_SIZE 1
> +#define ISI_RGB_SWAP_OFFSET 14
> +#define ISI_RGB_SWAP_SIZE 1
> +#define ISI_COL_SPACE_OFFSET 15
> +#define ISI_COL_SPACE_SIZE 1
> +#define ISI_IM_HSIZE_OFFSET 16
> +#define ISI_IM_HSIZE_SIZE 11
> +#define ISI_YCC_SWAP_OFFSET 28
> +#define ISI_YCC_SWAP_SIZE 2
> +#define ISI_RGB_CFG_OFFSET 30
> +#define ISI_RGB_CFG_SIZE 2
> +
> +/* Bitfields in CFG2 */
> +#define ISI_V2_IM_VSIZE_OFFSET 0
> +#define ISI_V2_IM_VSIZE_SIZE 11
> +#define ISI_V2_GS_MODE_OFFSET 11
> +#define ISI_V2_GS_MODE_SIZE 1
> +#define ISI_V2_RGB_MODE_OFFSET 12
> +#define ISI_V2_RGB_MODE_SIZE 1
> +#define ISI_V2_GRAYSCALE_OFFSET 13
> +#define ISI_V2_GRAYSCALE_SIZE 1
> +#define ISI_V2_RGB_SWAP_OFFSET 14
> +#define ISI_V2_RGB_SWAP_SIZE 1
> +#define ISI_V2_COL_SPACE_OFFSET 15
> +#define ISI_V2_COL_SPACE_SIZE 1
> +#define ISI_V2_IM_HSIZE_OFFSET 16
> +#define ISI_V2_IM_HSIZE_SIZE 11
> +#define ISI_V2_YCC_SWAP_OFFSET 28
> +#define ISI_V2_YCC_SWAP_SIZE 2
> +#define ISI_V2_RGB_CFG_OFFSET 30
> +#define ISI_V2_RGB_CFG_SIZE 2
> +
> +/* Bitfields in CTRL */
> +#define ISI_V2_EN_OFFSET 0
> +#define ISI_V2_EN_SIZE 1
> +#define ISI_V2_DIS_OFFSET 1
> +#define ISI_V2_DIS_SIZE 1
> +#define ISI_V2_SRST_OFFSET 2
> +#define ISI_V2_SRST_SIZE 1
> +#define ISI_V2_CDC_OFFSET 8
> +#define ISI_V2_CDC_SIZE 1
> +
> +/* Bitfields in SR/IER/IDR/IMR */
> +#define ISI_SOF_OFFSET 0
> +#define ISI_SOF_SIZE 1
> +#define ISI_SOFTRST_OFFSET 2
> +#define ISI_SOFTRST_SIZE 1
> +#define ISI_CDC_STATUS_OFFSET 3
> +#define ISI_CDC_STATUS_SIZE 1
> +#define ISI_CRC_ERR_OFFSET 4
> +#define ISI_CRC_ERR_SIZE 1
> +#define ISI_FO_C_OVF_OFFSET 5
> +#define ISI_FO_C_OVF_SIZE 1
> +#define ISI_FO_P_OVF_OFFSET 6
> +#define ISI_FO_P_OVF_SIZE 1
> +#define ISI_FO_P_EMP_OFFSET 7
> +#define ISI_FO_P_EMP_SIZE 1
> +#define ISI_FO_C_EMP_OFFSET 8
> +#define ISI_FO_C_EMP_SIZE 1
> +#define ISI_FR_OVR_OFFSET 9
> +#define ISI_FR_OVR_SIZE 1
> +
> +/* Bitfields in SR/IER/IDR/IMR(ISI_V2) */
> +#define ISI_V2_ENABLE_OFFSET 0
> +#define ISI_V2_ENABLE_SIZE 1
> +#define ISI_V2_DIS_DONE_OFFSET 1
> +#define ISI_V2_DIS_DONE_SIZE 1
> +#define ISI_V2_SRST_OFFSET 2
> +#define ISI_V2_SRST_SIZE 1
> +#define ISI_V2_CDC_STATUS_OFFSET 8
> +#define ISI_V2_CDC_STATUS_SIZE 1
> +#define ISI_V2_VSYNC_OFFSET 10
> +#define ISI_V2_VSYNC_SIZE 1
> +#define ISI_V2_PXFR_DONE_OFFSET 16
> +#define ISI_V2_PXFR_DONE_SIZE 1
> +#define ISI_V2_CXFR_DONE_OFFSET 17
> +#define ISI_V2_CXFR_DONE_SIZE 1
> +#define ISI_V2_P_OVR_OFFSET 24
> +#define ISI_V2_P_OVR_SIZE 1
> +#define ISI_V2_C_OVR_OFFSET 25
> +#define ISI_V2_C_OVR_SIZE 1
> +#define ISI_V2_CRC_ERR_OFFSET 26
> +#define ISI_V2_CRC_ERR_SIZE 1
> +#define ISI_V2_FR_OVR_OFFSET 27
> +#define ISI_V2_FR_OVR_SIZE 1
> +
> +/* Bitfields in PSIZE */
> +#define ISI_PREV_VSIZE_OFFSET 0
> +#define ISI_PREV_VSIZE_SIZE 10
> +#define ISI_PREV_HSIZE_OFFSET 16
> +#define ISI_PREV_HSIZE_SIZE 10
> +
> +/* Bitfields in PSIZE(ISI_V2) */
> +#define ISI_V2_PREV_VSIZE_OFFSET 0
> +#define ISI_V2_PREV_VSIZE_SIZE 10
> +#define ISI_V2_PREV_HSIZE_OFFSET 16
> +#define ISI_V2_PREV_HSIZE_SIZE 10
> +
> +/* Bitfields in PCDEF */
> +#define ISI_DEC_FACTOR_OFFSET 0
> +#define ISI_DEC_FACTOR_SIZE 8
> +
> +/* Bitfields in PCDEF */
> +#define ISI_V2_DEC_FACTOR_OFFSET 0
> +#define ISI_V2_DEC_FACTOR_SIZE 8
> +
> +/* Bitfields in PPFBD */
> +#define ISI_PREV_FBD_ADDR_OFFSET 0
> +#define ISI_PREV_FBD_ADDR_SIZE 32
> +
> +/* Bitfields in CDBA */
> +#define ISI_CODEC_DMA_ADDR_OFFSET 0
> +#define ISI_CODEC_DMA_ADDR_SIZE 32
> +
> +/* Bitfields in DMA_C_ADDR */
> +#define ISI_V2_DMA_ADDR_OFFSET 0
> +#define ISI_V2_DMA_ADDR_SIZE 32
> +
> +/* Bitfields in DMA_C_CTRL & in DMA_P_CTRL */
> +#define ISI_V2_DMA_FETCH_OFFSET 0
> +#define ISI_V2_DMA_FETCH_SIZE 1
> +#define ISI_V2_DMA_WB_OFFSET 1
> +#define ISI_V2_DMA_WB_SIZE 1
> +#define ISI_V2_DMA_IEN_OFFSET 2
> +#define ISI_V2_DMA_IEN_SIZE 1
> +#define ISI_V2_DMA_DONE_OFFSET 3
> +#define ISI_V2_DMA_DONE_SIZE 1
> +
> +/* Bitfields in DMA_CHER */
> +#define ISI_V2_DMA_P_CH_EN_OFFSET 0
> +#define ISI_V2_DMA_P_CH_EN_SIZE 1
> +#define ISI_V2_DMA_C_CH_EN_OFFSET 1
> +#define ISI_V2_DMA_C_CH_EN_SIZE 1
> +
> +/* Bitfields in Y2R_SET0 */
> +#define ISI_Y2R_SET0_C3_OFFSET 24
> +#define ISI_Y2R_SET0_C3_SIZE 8
> +
> +/* Bitfields in Y2R_SET0(ISI_V2) */
> +#define ISI_V2_Y2R_SET0_C3_OFFSET 24
> +#define ISI_V2_Y2R_SET0_C3_SIZE 8
> +
> +/* Bitfields in Y2R_SET1 */
> +#define ISI_Y2R_SET1_C4_OFFSET 0
> +#define ISI_Y2R_SET1_C4_SIZE 9
> +#define ISI_YOFF_OFFSET 12
> +#define ISI_YOFF_SIZE 1
> +#define ISI_CROFF_OFFSET 13
> +#define ISI_CROFF_SIZE 1
> +#define ISI_CBOFF_OFFSET 14
> +#define ISI_CBOFF_SIZE 1
> +
> +/* Bitfields in Y2R_SET1(ISI_V2) */
> +#define ISI_V2_Y2R_SET1_C4_OFFSET 0
> +#define ISI_V2_Y2R_SET1_C4_SIZE 9
> +#define ISI_V2_YOFF_OFFSET 12
> +#define ISI_V2_YOFF_SIZE 1
> +#define ISI_V2_CROFF_OFFSET 13
> +#define ISI_V2_CROFF_SIZE 1
> +#define ISI_V2_CBOFF_OFFSET 14
> +#define ISI_V2_CBOFF_SIZE 1
> +
> +/* Bitfields in R2Y_SET0 */
> +#define ISI_C0_OFFSET 0
> +#define ISI_C0_SIZE 8
> +#define ISI_C1_OFFSET 8
> +#define ISI_C1_SIZE 8
> +#define ISI_C2_OFFSET 16
> +#define ISI_C2_SIZE 8
> +#define ISI_ROFF_OFFSET 24
> +#define ISI_ROFF_SIZE 1
> +
> +/* Bitfields in R2Y_SET0(ISI_V2) */
> +#define ISI_V2_C0_OFFSET 0
> +#define ISI_V2_C0_SIZE 8
> +#define ISI_V2_C1_OFFSET 8
> +#define ISI_V2_C1_SIZE 8
> +#define ISI_V2_C2_OFFSET 16
> +#define ISI_V2_C2_SIZE 8
> +#define ISI_V2_ROFF_OFFSET 24
> +#define ISI_V2_ROFF_SIZE 1
> +
> +/* Bitfields in R2Y_SET1 */
> +#define ISI_R2Y_SET1_C3_OFFSET 0
> +#define ISI_R2Y_SET1_C3_SIZE 8
> +#define ISI_R2Y_SET1_C4_OFFSET 8
> +#define ISI_R2Y_SET1_C4_SIZE 8
> +#define ISI_C5_OFFSET 16
> +#define ISI_C5_SIZE 8
> +#define ISI_GOFF_OFFSET 24
> +#define ISI_GOFF_SIZE 1
> +
> +/* Bitfields in R2Y_SET1(ISI_V2) */
> +#define ISI_V2_R2Y_SET1_C3_OFFSET 0
> +#define ISI_V2_R2Y_SET1_C3_SIZE 8
> +#define ISI_V2_R2Y_SET1_C4_OFFSET 8
> +#define ISI_V2_R2Y_SET1_C4_SIZE 8
> +#define ISI_V2_C5_OFFSET 16
> +#define ISI_V2_C5_SIZE 8
> +#define ISI_V2_GOFF_OFFSET 24
> +#define ISI_V2_GOFF_SIZE 1
> +
> +/* Bitfields in R2Y_SET2 */
> +#define ISI_C6_OFFSET 0
> +#define ISI_C6_SIZE 8
> +#define ISI_C7_OFFSET 8
> +#define ISI_C7_SIZE 8
> +#define ISI_C8_OFFSET 16
> +#define ISI_C8_SIZE 8
> +#define ISI_BOFF_OFFSET 24
> +#define ISI_BOFF_SIZE 1
> +
> +/* Bitfields in R2Y_SET2(ISI_V2) */
> +#define ISI_V2_C6_OFFSET 0
> +#define ISI_V2_C6_SIZE 8
> +#define ISI_V2_C7_OFFSET 8
> +#define ISI_V2_C7_SIZE 8
> +#define ISI_V2_C8_OFFSET 16
> +#define ISI_V2_C8_SIZE 8
> +#define ISI_V2_BOFF_OFFSET 24
> +#define ISI_V2_BOFF_SIZE 1
> +
> +/* Constants for FRATE */
> +#define ISI_FRATE_CAPTURE_ALL 0
> +
> +/* Constants for FRATE(ISI_V2) */
> +#define ISI_V2_FRATE_CAPTURE_ALL 0
> +
> +/* Constants for YCC_SWAP */
> +#define ISI_YCC_SWAP_DEFAULT 0
> +#define ISI_YCC_SWAP_MODE_1 1
> +#define ISI_YCC_SWAP_MODE_2 2
> +#define ISI_YCC_SWAP_MODE_3 3
> +
> +/* Constants for YCC_SWAP(ISI_V2) */
> +#define ISI_V2_YCC_SWAP_DEFAULT 0
> +#define ISI_V2_YCC_SWAP_MODE_1 1
> +#define ISI_V2_YCC_SWAP_MODE_2 2
> +#define ISI_V2_YCC_SWAP_MODE_3 3
> +
> +/* Constants for RGB_CFG */
> +#define ISI_RGB_CFG_DEFAULT 0
> +#define ISI_RGB_CFG_MODE_1 1
> +#define ISI_RGB_CFG_MODE_2 2
> +#define ISI_RGB_CFG_MODE_3 3
> +
> +/* Constants for RGB_CFG(ISI_V2) */
> +#define ISI_V2_RGB_CFG_DEFAULT 0
> +#define ISI_V2_RGB_CFG_MODE_1 1
> +#define ISI_V2_RGB_CFG_MODE_2 2
> +#define ISI_V2_RGB_CFG_MODE_3 3
> +
> +/* Bit manipulation macros */
> +#define ISI_BIT(name) \
> + (1 << ISI_##name##_OFFSET)
> +#define ISI_BF(name, value) \
> + (((value) & ((1 << ISI_##name##_SIZE) - 1)) \
> + << ISI_##name##_OFFSET)
> +#define ISI_BFEXT(name, value) \
> + (((value) >> ISI_##name##_OFFSET) \
> + & ((1 << ISI_##name##_SIZE) - 1))
> +#define ISI_BFINS(name, value, old) \
> + (((old) & ~(((1 << ISI_##name##_SIZE) - 1) \
> + << ISI_##name##_OFFSET))\
> + | ISI_BF(name, value))
I really dislike this kind of register access magic. Not sure how others
feel about it. These macros are really ugly.
> +/* Register access macros */
> +#define isi_readl(port, reg) \
> + __raw_readl((port)->regs + ISI_##reg)
> +#define isi_writel(port, reg, value) \
> + __raw_writel((value), (port)->regs + ISI_##reg)
If the token pasting stuff gets dropped then these can be static inline
functions which is preferred.
> +
> +#define ATMEL_V4L2_VID_FLAGS (V4L2_CAP_VIDEO_OUTPUT)
> +
> +struct atmel_isi;
> +
> +enum atmel_isi_pixfmt {
> + ATMEL_ISI_PIXFMT_GREY, /* Greyscale */
> + ATMEL_ISI_PIXFMT_CbYCrY,
> + ATMEL_ISI_PIXFMT_CrYCbY,
> + ATMEL_ISI_PIXFMT_YCbYCr,
> + ATMEL_ISI_PIXFMT_YCrYCb,
> + ATMEL_ISI_PIXFMT_RGB24,
> + ATMEL_ISI_PIXFMT_BGR24,
> + ATMEL_ISI_PIXFMT_RGB16,
> + ATMEL_ISI_PIXFMT_BGR16,
> + ATMEL_ISI_PIXFMT_GRB16, /* G[2:0] R[4:0]/B[4:0] G[5:3] */
> + ATMEL_ISI_PIXFMT_GBR16, /* G[2:0] B[4:0]/R[4:0] G[5:3] */
> + ATMEL_ISI_PIXFMT_RGB24_REV,
> + ATMEL_ISI_PIXFMT_BGR24_REV,
> + ATMEL_ISI_PIXFMT_RGB16_REV,
> + ATMEL_ISI_PIXFMT_BGR16_REV,
> + ATMEL_ISI_PIXFMT_GRB16_REV, /* G[2:0] R[4:0]/B[4:0] G[5:3] */
> + ATMEL_ISI_PIXFMT_GBR16_REV, /* G[2:0] B[4:0]/R[4:0] G[5:3] */
> +};
> +
> +struct isi_platform_data {
> + u8 has_emb_sync;
> + u8 emb_crc_sync;
> + u8 hsync_act_low;
> + u8 vsync_act_low;
> + u8 pclk_act_falling;
> + u8 isi_full_mode;
> +#define ISI_HSYNC_ACT_LOW 0x01
> +#define ISI_VSYNC_ACT_LOW 0x02
> +#define ISI_PXCLK_ACT_FALLING 0x04
> +#define ISI_EMB_SYNC 0x08
> +#define ISI_CRC_SYNC 0x10
> +#define ISI_FULL 0x20
> +#define ISI_DATAWIDTH_8 0x40
> +#define ISI_DATAWIDTH_10 0x80
> + u32 flags;
> + u8 gs_mode;
> +#define ISI_GS_2PIX_PER_WORD 0x00
> +#define ISI_GS_1PIX_PER_WORD 0x01
> + u8 pixfmt;
> + u8 sfd;
> + u8 sld;
> + u8 thmask;
> +#define ISI_BURST_4_8_16 0x00
> +#define ISI_BURST_8_16 0x01
> +#define ISI_BURST_16 0x02
> + u8 frate;
> +#define ISI_FRATE_DIV_2 0x01
> +#define ISI_FRATE_DIV_3 0x02
> +#define ISI_FRATE_DIV_4 0x03
> +#define ISI_FRATE_DIV_5 0x04
> +#define ISI_FRATE_DIV_6 0x05
> +#define ISI_FRATE_DIV_7 0x06
> +#define ISI_FRATE_DIV_8 0x07
> +};
Might need some comments in this structure so that board file writers
know what each of the fields are for.
> +
> +#endif /* __AT91_ISI_H__ */
> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
> index d61414e..eae6005 100644
> --- a/drivers/media/video/Kconfig
> +++ b/drivers/media/video/Kconfig
> @@ -80,6 +80,16 @@ menuconfig VIDEO_CAPTURE_DRIVERS
> Some of those devices also supports FM radio.
>
> if VIDEO_CAPTURE_DRIVERS && VIDEO_V4L2
> +config VIDEO_ATMEL_ISI
> + tristate "ATMEL Image Sensor Interface (ISI) support"
> + depends on VIDEO_DEV && SOC_CAMERA
Depends on AT91/AVR32?
> + select VIDEOBUF2_DMA_CONTIG
> + default n
> + ---help---
> + This module makes the ATMEL Image Sensor Interface available
> + as a v4l2 device.
> + Say Y here to enable selecting the Image Sensor Interface.
> + When in doubt, say N.
>
> config VIDEO_ADV_DEBUG
> bool "Enable advanced debug functionality"
> diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
> index a10e4c3..f734a65 100644
> --- a/drivers/media/video/Makefile
> +++ b/drivers/media/video/Makefile
> @@ -166,6 +166,7 @@ obj-$(CONFIG_VIDEO_SH_MOBILE_CSI2) += sh_mobile_csi2.o
> obj-$(CONFIG_VIDEO_SH_MOBILE_CEU) += sh_mobile_ceu_camera.o
> obj-$(CONFIG_VIDEO_OMAP1) += omap1_camera.o
> obj-$(CONFIG_VIDEO_SAMSUNG_S5P_FIMC) += s5p-fimc/
> +obj-$(CONFIG_VIDEO_ATMEL_ISI) += atmel-isi.o
>
> obj-$(CONFIG_ARCH_DAVINCI) += davinci/
>
> diff --git a/drivers/media/video/atmel-isi.c b/drivers/media/video/atmel-isi.c
> new file mode 100644
> index 0000000..33d0b83
> --- /dev/null
> +++ b/drivers/media/video/atmel-isi.c
> @@ -0,0 +1,1089 @@
> +/*
> + * Copyright (c) 2011 Atmel Corporation
> + *
> + * Based on previous work by Lars Haring and Sedji Gaouaou
> + *
> + * Based on the bttv driver for Bt848 with respective copyright holders
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/completion.h>
> +#include <linux/fs.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/version.h>
> +#include <linux/kfifo.h>
> +
> +#include <mach/board.h>
> +#include <mach/cpu.h>
> +#include <mach/at91_isi.h>
> +
> +#include <media/videobuf2-dma-contig.h>
> +#include <media/soc_camera.h>
> +#include <media/soc_mediabus.h>
> +
> +#define ATMEL_ISI_VERSION KERNEL_VERSION(1, 0, 0)
Do we really need this version?
> +#define MAX_BUFFER_NUMS 32
> +#define MAX_SUPPORT_WIDTH 2048
> +#define MAX_SUPPORT_HEIGHT 2048
> +
> +static unsigned int vid_limit = 16;
This never gets changed so it can become a const/define. The value is
MB, which is not clear from the name, and it gets multiplied out to
bytes in its only usage, so maybe:
#define VID_LIMIT_BYTES (16 * 1024 * 1024)
> +
> +enum isi_buffer_state {
> + ISI_BUF_NEEDS_INIT,
> + ISI_BUF_PREPARED,
> +};
Aren't there v4l2 constants for this already?
VIDEOBUF_NEEDS_INIT,
VIDEOBUF_PREPARED,
Just reuse those.
> +
> +/* Single frame capturing states */
> +enum {
> + STATE_IDLE = 0,
> + STATE_CAPTURE_READY,
> + STATE_CAPTURE_WAIT_SOF,
> + STATE_CAPTURE_IN_PROGRESS,
> + STATE_CAPTURE_DONE,
> + STATE_CAPTURE_ERROR,
> +};
> +
> +/* Frame buffer descriptor
> + * Used by the ISI module as a linked list for the DMA controller.
> + */
> +struct fbd {
> + /* Physical address of the frame buffer */
> + u32 fb_address;
> +#if defined(CONFIG_ARCH_AT91SAM9G45) ||\
> + defined(CONFIG_ARCH_AT91SAM9X5)
> + /* DMA Control Register(only in HISI2) */
> + u32 dma_ctrl;
> +#endif
I wouldn't bother with this ifdef. The memory cost is pretty
insignificant and it makes the code easier to read without it. It also
means you don't need to patch it up whenever a new at91 platform with
isi dma support appears.
> + /* Physical address of the next fbd */
> + u32 next_fbd_address;
> +};
> +
> +#if defined(CONFIG_ARCH_AT91SAM9G45) ||\
> + defined(CONFIG_ARCH_AT91SAM9X5)
> +static void set_dma_ctrl(struct fbd *fb_desc, u32 ctrl)
> +{
> + fb_desc->dma_ctrl = ctrl;
> +}
> +#else
> +static void set_dma_ctrl(struct fbd *fb_desc, u32 ctrl) { }
> +#endif
Same here, get rid of the ifdefs.
> +/* Frame buffer data
> + */
> +struct frame_buffer {
> + struct vb2_buffer vb;
> + struct fbd fb_desc;
> + /* Frame number of the frame */
> + unsigned long sequence;
> +
> + enum isi_buffer_state dma_desc_status;
> + struct list_head list;
> +};
> +
> +struct atmel_isi {
> + /* ISI module spin lock. Protects against concurrent access of variables
> + * that are shared with the ISR */
> + spinlock_t lock;
> + void __iomem *regs;
> +
> + /* If set ISI is in still capture mode */
> + int still_capture;
> + int sequence;
> + /* State of the ISI module in capturing mode */
> + int state;
> +
> + /* Capture/streaming wait queue for waiting for SOF */
> + wait_queue_head_t capture_wq;
> +
> + struct v4l2_device v4l2_dev;
> +
> + struct vb2_alloc_ctx *alloc_ctx;
> +
> + struct clk *pclk;
> + struct platform_device *pdev;
> + unsigned int irq;
> +
> + struct isi_platform_data *pdata;
> + unsigned long platform_flags;
> +
> + struct list_head video_buffer_list;
> + struct frame_buffer *active;
> +
> + struct soc_camera_device *icd;
> + struct soc_camera_host soc_host;
> +};
> +
> +static int configure_geometry(struct atmel_isi *isi, u32 width,
> + u32 height, enum v4l2_mbus_pixelcode code)
> +{
> + u32 cfg2, cr, ctrl;
> +
> + cr = 0;
> + switch (code) {
> + /* YUV, including grey */
> + case V4L2_MBUS_FMT_Y8_1X8:
> + cr = ISI_BIT(GRAYSCALE);
> + break;
> + case V4L2_MBUS_FMT_UYVY8_2X8:
> + cr = ISI_BF(V2_YCC_SWAP, 3);
> + break;
> + case V4L2_MBUS_FMT_VYUY8_2X8:
> + cr = ISI_BF(V2_YCC_SWAP, 2);
> + break;
> + case V4L2_MBUS_FMT_YUYV8_2X8:
> + cr = ISI_BF(V2_YCC_SWAP, 1);
> + break;
> + case V4L2_MBUS_FMT_YVYU8_2X8:
> + cr = ISI_BF(V2_YCC_SWAP, 0);
> + break;
> + /* RGB, TODO */
> + default:
> + return -EINVAL;
> + }
> +
> + ctrl = ISI_BIT(DIS);
> + isi_writel(isi, V2_CTRL, ctrl);
> + /* Check if module properly disable */
> + while (isi_readl(isi, V2_STATUS) & ISI_BIT(V2_DIS_DONE))
> + cpu_relax();
> +
> + cfg2 = isi_readl(isi, V2_CFG2);
> + cfg2 |= cr;
> + cfg2 = ISI_BFINS(V2_IM_VSIZE, height - 1, cfg2);
> + cfg2 = ISI_BFINS(V2_IM_HSIZE, width - 1, cfg2);
> + isi_writel(isi, V2_CFG2, cfg2);
> +
> + return 0;
> +}
> +
> +static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
> +{
> + if (isi->active) {
> + struct vb2_buffer *vb = &isi->active->vb;
> + struct frame_buffer *buf = isi->active;
> +
> + list_del_init(&buf->list);
> + do_gettimeofday(&vb->v4l2_buf.timestamp);
> + vb->v4l2_buf.sequence = isi->sequence++;
> + vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
> + }
> +
> + if (list_empty(&isi->video_buffer_list)) {
> + isi->active = NULL;
> + } else {
> + /* start next dma frame. */
> + isi->active = list_entry(isi->video_buffer_list.next,
> + struct frame_buffer, list);
> + isi_writel(isi, V2_DMA_C_DSCR, (__pa(&(isi->active->fb_desc))));
> + isi_writel(isi, V2_DMA_C_CTRL,
> + ISI_BIT(V2_DMA_FETCH) | ISI_BIT(V2_DMA_DONE));
> + isi_writel(isi, V2_DMA_CHER, ISI_BIT(V2_DMA_C_CH_EN));
> + }
> + return IRQ_HANDLED;
> +}
> +
> +/* ISI interrupt service routine */
> +static irqreturn_t isi_interrupt(int irq, void *dev_id)
> +{
> + struct atmel_isi *isi = dev_id;
> + u32 status, mask, pending;
> + irqreturn_t ret = IRQ_NONE;
> +
> + spin_lock(&isi->lock);
> +
> + status = isi_readl(isi, V2_STATUS);
> + mask = isi_readl(isi, V2_INTMASK);
> + pending = status & mask;
> +
> + if (!isi->still_capture) {
> + if (pending & (ISI_BIT(V2_VSYNC))) {
> + switch (isi->state) {
> + case STATE_IDLE:
> + isi->state = STATE_CAPTURE_READY;
> + wake_up_interruptible(&isi->capture_wq);
> + break;
> + }
> + } else if (likely(pending & (ISI_BIT(V2_CXFR_DONE)))) {
> + ret = atmel_isi_handle_streaming(isi);
> + }
> + } else {
> + while (pending) {
> + if (pending & (ISI_BIT(V2_C_OVR) | ISI_BIT(V2_FR_OVR)))
> + printk(KERN_ERR "Overrun (status = 0x%x)\n",
> + status);
dev_err(isi->v4l2_dev.dev, "Overrun...");
> + else if (pending &
> + (ISI_BIT(V2_VSYNC) | ISI_BIT(V2_CDC))) {
> + switch (isi->state) {
> + case STATE_IDLE:
> + isi->state = STATE_CAPTURE_READY;
> + wake_up_interruptible(&isi->capture_wq);
> + break;
> + case STATE_CAPTURE_READY:
> + break;
> + case STATE_CAPTURE_WAIT_SOF:
> + isi->state = STATE_CAPTURE_IN_PROGRESS;
> + break;
> + }
> + }
> +
> + if (likely(pending & (ISI_BIT(V2_CXFR_DONE))))
> + ret = atmel_isi_handle_streaming(isi);
> +
> + status = isi_readl(isi, V2_STATUS);
> + mask = isi_readl(isi, V2_INTMASK);
> + pending = status & mask;
> + ret = IRQ_HANDLED;
> + }
> + }
> + spin_unlock(&isi->lock);
> +
> + return ret;
> +}
> +
> +static int atmel_isi_init(struct atmel_isi *isi)
> +{
> + /*
> + * The reset will only succeed if we have a
> + * pixel clock from the camera.
> + */
> + isi_writel(isi, V2_CTRL, ISI_BIT(V2_SRST));
> + isi_writel(isi, V2_INTDIS, ~0UL);
Don't you need to wait for the reset bit to clear? Other implementations
I have used of the Atmel ISI driver do a wait_for_completition and have
the interrupt handler set a reset_complete flag.
> + return 0;
> +}
> +
> +/* ------------------------------------------------------------------
> + Videobuf operations
> + ------------------------------------------------------------------*/
> +static int queue_setup(struct vb2_queue *vq, unsigned int *nbuffers,
> + unsigned int *nplanes, unsigned long sizes[],
> + void *alloc_ctxs[])
> +{
> + struct soc_camera_device *icd = soc_camera_from_vb2q(vq);
> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> + struct atmel_isi *dev = ici->priv;
> + unsigned long size;
> +
> + int bytes_per_line = soc_mbus_bytes_per_line(icd->user_width,
> + icd->current_fmt->host_fmt);
> +
> + if (bytes_per_line < 0)
> + return bytes_per_line;
> +
> + size = bytes_per_line * icd->user_height;
> +
> + if (0 == *nbuffers)
if (*nbuffers == 0)
is the more commonly preferred style I believe.
> + *nbuffers = MAX_BUFFER_NUMS;
> + if (*nbuffers > MAX_BUFFER_NUMS)
> + *nbuffers = MAX_BUFFER_NUMS;
> +
> + while (size * *nbuffers > vid_limit * 1024 * 1024)
> + (*nbuffers)--;
> +
> + *nplanes = 1;
> + sizes[0] = size;
> + alloc_ctxs[0] = dev->alloc_ctx;
> +
> + dev->sequence = 0;
> + dev->active = NULL;
> +
> + dev_dbg(icd->dev.parent, "%s, count=%d, size=%ld\n", __func__,
> + *nbuffers, size);
> +
> + return 0;
> +}
> +
> +static int buffer_init(struct vb2_buffer *vb)
> +{
> + struct frame_buffer *buf = container_of(vb, struct frame_buffer, vb);
> +
> + buf->dma_desc_status = ISI_BUF_NEEDS_INIT;
> + INIT_LIST_HEAD(&buf->list);
> +
> + return 0;
> +}
> +
> +static int buffer_prepare(struct vb2_buffer *vb)
> +{
> + struct soc_camera_device *icd = soc_camera_from_vb2q(vb->vb2_queue);
> + struct frame_buffer *buf = container_of(vb, struct frame_buffer, vb);
> + unsigned long size;
> + int bytes_per_line = soc_mbus_bytes_per_line(icd->user_width,
> + icd->current_fmt->host_fmt);
> +
> + if (bytes_per_line < 0)
> + return bytes_per_line;
> +
> + size = bytes_per_line * icd->user_height;
> +
> + if (vb2_plane_size(vb, 0) < size) {
> + dev_err(icd->dev.parent, "%s data will not fit into plane (%lu < %lu)\n",
> + __func__, vb2_plane_size(vb, 0), size);
> + return -EINVAL;
> + }
> +
> + vb2_set_plane_payload(&buf->vb, 0, size);
> +
> + if (buf->dma_desc_status == ISI_BUF_NEEDS_INIT) {
> + buf->dma_desc_status = ISI_BUF_PREPARED;
> +
> + /* initialize the dma descriptor */
> + buf->fb_desc.fb_address = vb2_dma_contig_plane_paddr(vb, 0);
> + buf->fb_desc.next_fbd_address = 0;
> + set_dma_ctrl(&buf->fb_desc, ISI_BIT(V2_DMA_WB));
> + }
> +
> + return 0;
> +}
> +
> +static int buffer_finish(struct vb2_buffer *vb)
> +{
> + return 0;
> +}
Does soc-camera support having NULL function callbacks for functions
that are not required?
> +
> +static void buffer_cleanup(struct vb2_buffer *vb)
> +{
> + struct soc_camera_device *icd = soc_camera_from_vb2q(vb->vb2_queue);
> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> + struct atmel_isi *isi = ici->priv;
> + struct frame_buffer *buf = container_of(vb, struct frame_buffer, vb);
> + unsigned long flags = 0;
> +
> + spin_lock_irqsave(&isi->lock, flags);
> + buf->dma_desc_status = ISI_BUF_NEEDS_INIT;
> + spin_unlock_irqrestore(&isi->lock, flags);
> +}
> +
> +static void start_dma(struct atmel_isi *isi, struct frame_buffer *buffer)
> +{
> + u32 ctrl, cfg1;
> + ctrl = isi_readl(isi, V2_CTRL);
> + cfg1 = isi_readl(isi, V2_CFG1);
> + /* Enable irq: cxfr for the codec path, pxfr for the preview path */
> + isi_writel(isi, V2_INTEN,
> + ISI_BIT(V2_CXFR_DONE) | ISI_BIT(V2_PXFR_DONE));
> +
> + /* Enable codec path */
> + ctrl |= ISI_BIT(V2_CDC);
> + /* Check if already in a frame */
> + while (isi_readl(isi, V2_STATUS) & ISI_BIT(V2_CDC))
> + cpu_relax();
> +
> + /* Write the address of the first frame buffer in the C_ADDR reg
> + * write the address of the first descriptor(link list of buffer)
> + * in the C_DSCR reg, and enable dma channel.
> + */
> + isi_writel(isi, V2_DMA_C_DSCR, (__pa(&(buffer->fb_desc))));
> + isi_writel(isi, V2_DMA_C_CTRL,
> + ISI_BIT(V2_DMA_FETCH) | ISI_BIT(V2_DMA_DONE));
> + isi_writel(isi, V2_DMA_CHER, ISI_BIT(V2_DMA_C_CH_EN));
> +
> + /* Enable linked list */
> + cfg1 |= ISI_BF(V2_FRATE, isi->pdata->frate) | ISI_BIT(V2_DISCR);
> +
> + /* Enable ISI module*/
> + ctrl |= ISI_BIT(V2_ENABLE);
> + isi_writel(isi, V2_CTRL, ctrl);
> + isi_writel(isi, V2_CFG1, cfg1);
> +}
> +
> +static void buffer_queue(struct vb2_buffer *vb)
> +{
> + struct soc_camera_device *icd = soc_camera_from_vb2q(vb->vb2_queue);
> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> + struct atmel_isi *isi = ici->priv;
> + struct frame_buffer *buf = container_of(vb, struct frame_buffer, vb);
> + unsigned long flags = 0;
> +
> + spin_lock_irqsave(&isi->lock, flags);
> + list_add_tail(&buf->list, &isi->video_buffer_list);
> +
> + if (isi->active == NULL) {
> + isi->active = buf;
> + start_dma(isi, buf);
> + }
> + spin_unlock_irqrestore(&isi->lock, flags);
> +}
> +
> +static int start_streaming(struct vb2_queue *vq)
> +{
> + struct soc_camera_device *icd = soc_camera_from_vb2q(vq);
> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> + struct atmel_isi *isi = ici->priv;
> +
> + u32 sr = 0;
> + int ret;
> +
> + spin_lock_irq(&isi->lock);
> + isi->state = STATE_IDLE;
> +
> + /* Clear any pending SOF interrupt */
> + sr = isi_readl(isi, V2_STATUS);
> + /* Enable VSYNC interrupt for SOF */
> + isi_writel(isi, V2_INTEN, ISI_BIT(V2_VSYNC));
> + isi_writel(isi, V2_CTRL, isi_readl(isi, V2_CTRL) | ISI_BIT(V2_EN));
> +
> + spin_unlock_irq(&isi->lock);
> + dev_dbg(icd->dev.parent, "isi: waiting for SOF\n");
> + ret = wait_event_interruptible(isi->capture_wq,
> + isi->state != STATE_IDLE);
> + if (ret)
> + return ret;
> +
> + if (isi->state != STATE_CAPTURE_READY)
> + return -EIO;
> +
> + spin_lock_irq(&isi->lock);
> + isi->state = STATE_CAPTURE_WAIT_SOF;
> + isi_writel(isi, V2_INTDIS, ISI_BIT(V2_VSYNC));
> + spin_unlock_irq(&isi->lock);
> +
> + return 0;
> +}
> +
> +/* abort streaming and wait for last buffer */
> +static int stop_streaming(struct vb2_queue *vq)
> +{
> + struct soc_camera_device *icd = soc_camera_from_vb2q(vq);
> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> + struct atmel_isi *isi = ici->priv;
> +
> + spin_lock_irq(&isi->lock);
> + isi->still_capture = 0;
> + isi->active = NULL;
> +
> + while (isi_readl(isi, V2_STATUS) & ISI_BIT(V2_CDC))
> + cpu_relax();
> +
> + /* Disble codec path */
> + isi_writel(isi, V2_CTRL, isi_readl(isi, V2_CTRL) & (~ISI_BIT(V2_CDC)));
> + /* Disable interrupts */
> + isi_writel(isi, V2_INTDIS,
> + ISI_BIT(V2_CXFR_DONE) | ISI_BIT(V2_PXFR_DONE));
> + /* Disable ISI module*/
> + isi_writel(isi, V2_CTRL, isi_readl(isi, V2_CTRL) | ISI_BIT(V2_DIS));
> +
> + /* Release all active buffers */
> + while (!list_empty(&isi->video_buffer_list)) {
> + struct frame_buffer *buf;
> + buf = list_entry(isi->video_buffer_list.next,
> + struct frame_buffer, list);
> + list_del_init(&buf->list);
> + vb2_buffer_done(&buf->vb, VB2_BUF_STATE_ERROR);
> + }
> +
> + spin_unlock_irq(&isi->lock);
> + return 0;
> +}
> +
> +static struct vb2_ops isi_video_qops = {
> + .queue_setup = queue_setup,
> + .buf_init = buffer_init,
> + .buf_prepare = buffer_prepare,
> + .buf_finish = buffer_finish,
> + .buf_cleanup = buffer_cleanup,
> + .buf_queue = buffer_queue,
> + .start_streaming = start_streaming,
> + .stop_streaming = stop_streaming,
> + .wait_prepare = soc_camera_unlock,
> + .wait_finish = soc_camera_lock,
> +};
> +
> +/* ------------------------------------------------------------------
> + SOC camera operations for the device
> + ------------------------------------------------------------------*/
> +static int isi_camera_init_videobuf(struct vb2_queue *q,
> + struct soc_camera_device *icd)
> +{
> + q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> + q->io_modes = VB2_MMAP | VB2_USERPTR | VB2_READ;
> + q->drv_priv = icd;
> + q->buf_struct_size = sizeof(struct frame_buffer);
> + q->ops = &isi_video_qops;
> + q->mem_ops = &vb2_dma_contig_memops;
> +
> + return vb2_queue_init(q);
> +}
> +static inline void stride_align(u32 *width)
> +{
> + if (((*width + 7) & ~7) < 4096)
> + *width = (*width + 7) & ~7;
> + else
> + *width = *width & ~7;
This looks like something that may already have a function to do it?
> +}
> +
> +static int isi_camera_set_fmt(struct soc_camera_device *icd,
> + struct v4l2_format *f)
> +{
> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> + struct atmel_isi *isi = ici->priv;
> + 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;
> + }
> +
> + stride_align(&pix->width);
> + dev_dbg(icd->dev.parent, "plan to set format %dx%d\n",
> + pix->width, pix->height);
> +
> + 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)
> + return ret;
> +
> + if (mf.code != xlate->code)
> + return -EINVAL;
> +
> + ret = configure_geometry(isi, pix->width, pix->height, xlate->code);
> + if (ret < 0)
> + return ret;
> +
> + pix->width = mf.width;
> + pix->height = mf.height;
> + pix->field = mf.field;
> + pix->colorspace = mf.colorspace;
> + icd->current_fmt = xlate;
> +
> + pix->bytesperline = soc_mbus_bytes_per_line(pix->width,
> + xlate->host_fmt);
> + if (pix->bytesperline < 0)
> + return pix->bytesperline;
> + pix->sizeimage = pix->height * pix->bytesperline;
> +
> + dev_dbg(icd->dev.parent, "Finally set format %dx%d, sizeimage = %d\n",
> + pix->width, pix->height, pix->sizeimage);
> +
> + return ret;
> +}
> +
> +static int isi_camera_try_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;
> + __u32 pixfmt = pix->pixelformat;
u32 inside the kernel, __u32 is for code which is exposed to userspace
(i.e. API headers).
> + int ret;
> +
> + xlate = soc_camera_xlate_by_fourcc(icd, pixfmt);
> + if (pixfmt && !xlate) {
> + dev_warn(icd->dev.parent, "Format %x not found\n", pixfmt);
> + return -EINVAL;
> + }
> +
> + /* limit to Atmel ISI hardware capabilities */
> + if (pix->height > MAX_SUPPORT_HEIGHT)
> + pix->height = MAX_SUPPORT_HEIGHT;
> + if (pix->width > MAX_SUPPORT_WIDTH)
> + pix->width = MAX_SUPPORT_WIDTH;
> +
> + pix->bytesperline = soc_mbus_bytes_per_line(pix->width,
> + xlate->host_fmt);
> + if (pix->bytesperline < 0)
> + return pix->bytesperline;
> + pix->sizeimage = pix->height * pix->bytesperline;
> +
> + /* limit to sensor capabilities */
> + 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, try_mbus_fmt, &mf);
> + if (ret < 0)
> + return ret;
> +
> + pix->width = mf.width;
> + pix->height = mf.height;
> + pix->colorspace = mf.colorspace;
> +
> + switch (mf.field) {
> + case V4L2_FIELD_ANY:
> + pix->field = V4L2_FIELD_NONE;
> + break;
> + case V4L2_FIELD_NONE:
> + break;
> + default:
> + dev_err(icd->dev.parent, "Field type %d unsupported.\n",
> + mf.field);
> + ret = -EINVAL;
> + }
> +
> + return ret;
> +}
> +
> +static const struct soc_mbus_pixelfmt isi_camera_formats[] = {
> + {
> + .fourcc = V4L2_PIX_FMT_YUYV,
> + .name = "Packed YUV422 16 bit",
> + .bits_per_sample = 8,
> + .packing = SOC_MBUS_PACKING_2X8_PADHI,
> + .order = SOC_MBUS_ORDER_LE,
> + },
> +};
> +
> +/* This will be corrected as we get more formats */
> +static bool isi_camera_packing_supported(const struct soc_mbus_pixelfmt *fmt)
> +{
> + return fmt->packing == SOC_MBUS_PACKING_NONE ||
> + (fmt->bits_per_sample == 8 &&
> + fmt->packing == SOC_MBUS_PACKING_2X8_PADHI) ||
> + (fmt->bits_per_sample > 8 &&
> + fmt->packing == SOC_MBUS_PACKING_EXTEND16);
> +}
> +
> +static int test_platform_param(struct atmel_isi *isi,
> + unsigned char buswidth, unsigned long *flags)
> +{
> + /*
> + * Platform specified synchronization and pixel clock polarities are
> + * only a recommendation and are only used during probing. Atmel ISI
> + * camera interface only works in master mode, i.e., uses HSYNC and
> + * VSYNC signals from the sensor
> + */
> + *flags = SOCAM_MASTER |
> + SOCAM_HSYNC_ACTIVE_HIGH |
> + SOCAM_HSYNC_ACTIVE_LOW |
> + SOCAM_VSYNC_ACTIVE_HIGH |
> + SOCAM_VSYNC_ACTIVE_LOW |
> + SOCAM_PCLK_SAMPLE_RISING |
> + SOCAM_PCLK_SAMPLE_FALLING |
> + SOCAM_DATA_ACTIVE_HIGH;
> +
> + /* If requested data width is supported by the platform, use it */
> + switch (buswidth) {
> + case 10:
> + if (!(isi->platform_flags & ISI_DATAWIDTH_10))
> + return -EINVAL;
> + *flags |= SOCAM_DATAWIDTH_10;
> + break;
If you have specified the platform data width as 10 bits can you not do
8 bit transfers on it?
> + case 8:
> + if (!(isi->platform_flags & ISI_DATAWIDTH_8))
> + return -EINVAL;
> + *flags |= SOCAM_DATAWIDTH_8;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int isi_camera_try_bus_param(struct soc_camera_device *icd,
> + unsigned char buswidth)
> +{
> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> + struct atmel_isi *isi = ici->priv;
> + unsigned long bus_flags, camera_flags;
> + int ret = test_platform_param(isi, buswidth, &bus_flags);
> +
> + if (ret < 0)
> + return ret;
> +
> + camera_flags = icd->ops->query_bus_param(icd);
> +
> + ret = soc_camera_bus_param_compatible(camera_flags, bus_flags);
> + if (!ret)
> + return -EINVAL;
> + return 0;
> +}
> +
> +
> +static int isi_camera_get_formats(struct soc_camera_device *icd,
> + unsigned int idx,
> + struct soc_camera_format_xlate *xlate)
> +{
> + struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
> + struct device *dev = icd->dev.parent;
> + int formats = 0, ret;
> + /* sensor format */
> + enum v4l2_mbus_pixelcode code;
> + /* soc camera host format */
> + const struct soc_mbus_pixelfmt *fmt;
> +
> + ret = v4l2_subdev_call(sd, video, enum_mbus_fmt, idx, &code);
> + if (ret < 0)
> + /* No more formats */
> + return 0;
> +
> + fmt = soc_mbus_get_fmtdesc(code);
> + if (!fmt) {
> + dev_err(icd->dev.parent,
> + "Invalid format code #%u: %d\n", idx, code);
> + return 0;
> + }
> +
> + /* This also checks support for the requested bits-per-sample */
> + ret = isi_camera_try_bus_param(icd, fmt->bits_per_sample);
> + if (ret < 0) {
> + dev_err(icd->dev.parent,
> + "Fail to try the bus parameters.\n");
> + return 0;
> + }
> +
> + switch (code) {
> + case V4L2_MBUS_FMT_UYVY8_2X8:
> + case V4L2_MBUS_FMT_VYUY8_2X8:
> + case V4L2_MBUS_FMT_YUYV8_2X8:
> + case V4L2_MBUS_FMT_YVYU8_2X8:
> + formats++;
> + if (xlate) {
> + xlate->host_fmt = &isi_camera_formats[0];
> + xlate->code = code;
> + xlate++;
> + dev_dbg(icd->dev.parent, "Providing format %s using code %d\n",
> + isi_camera_formats[0].name, code);
> + }
> + break;
> + default:
> + if (!isi_camera_packing_supported(fmt))
> + return 0;
> + if (xlate)
> + dev_dbg(dev,
> + "Providing format %s in pass-through mode\n",
> + fmt->name);
> + }
> +
> + /* Generic pass-through */
> + formats++;
> + if (xlate) {
> + xlate->host_fmt = fmt;
> + xlate->code = code;
> + xlate++;
> + }
> +
> + return formats;
> +}
> +
> +/* Called with .video_lock held */
> +static int isi_camera_add_device(struct soc_camera_device *icd)
> +{
> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> + struct atmel_isi *isi = ici->priv;
> + int ret;
> +
> + if (isi->icd)
> + return -EBUSY;
> +
> + ret = atmel_isi_init(isi);
> + if (ret)
> + return ret;
> +
> + isi->icd = icd;
> + dev_info(icd->dev.parent, "Atmel ISI Camera driver attached to camera %d\n",
> + icd->devnum);
> + return 0;
> +}
> +/* Called with .video_lock held */
> +static void isi_camera_remove_device(struct soc_camera_device *icd)
> +{
> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> + struct atmel_isi *isi = ici->priv;
> +
> + BUG_ON(icd != isi->icd);
> +
> + isi_writel(isi, V2_CTRL, ISI_BIT(V2_DIS));
> + /* Check if module disable */
> + while (isi_readl(isi, V2_STATUS) & ISI_BIT(V2_DIS))
> + cpu_relax();
> +
> + isi->icd = NULL;
> +
> + dev_info(icd->dev.parent, "Atmel ISI Camera driver detached from camera %d\n",
> + icd->devnum);
> +}
> +
> +static unsigned int isi_camera_poll(struct file *file, poll_table *pt)
> +{
> + struct soc_camera_device *icd = file->private_data;
> +
> + return vb2_poll(&icd->vb2_vidq, file, pt);
> +}
> +
> +static int isi_camera_querycap(struct soc_camera_host *ici,
> + struct v4l2_capability *cap)
> +{
> + strcpy(cap->driver, "atmel-isi");
> + strcpy(cap->card, "Atmel Image Sensor Interface");
> + cap->version = ATMEL_ISI_VERSION;
> +
> + cap->capabilities = (V4L2_CAP_VIDEO_CAPTURE
> + | V4L2_CAP_READWRITE
> + | V4L2_CAP_STREAMING
> + );
In other places you have the |'s at the end of the line. Should be
consistent within a file.
> + return 0;
> +}
> +
> +static int isi_camera_set_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
u32.
> +{
> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> + struct atmel_isi *isi = ici->priv;
> + unsigned long bus_flags, camera_flags, common_flags;
> + const struct soc_mbus_pixelfmt *fmt;
> + int ret;
> + u32 cfg1, ctrl;
> +
> + fmt = soc_mbus_get_fmtdesc(icd->current_fmt->code);
> + if (!fmt)
> + return -EINVAL;
> +
> + ret = test_platform_param(isi, fmt->bits_per_sample, &bus_flags);
> + if (ret < 0)
> + return ret;
> +
> + camera_flags = icd->ops->query_bus_param(icd);
> +
> + common_flags = soc_camera_bus_param_compatible(camera_flags, bus_flags);
> + dev_dbg(icd->dev.parent, "Flags cam: 0x%lx host: 0x%lx common: 0x%lx\n",
> + camera_flags, bus_flags, common_flags);
> + if (!common_flags)
> + return -EINVAL;
> +
> + /* Make choises, based on platform preferences */
> + if ((common_flags & SOCAM_HSYNC_ACTIVE_HIGH) &&
> + (common_flags & SOCAM_HSYNC_ACTIVE_LOW)) {
> + if (isi->pdata->hsync_act_low)
> + common_flags &= ~SOCAM_HSYNC_ACTIVE_HIGH;
> + else
> + common_flags &= ~SOCAM_HSYNC_ACTIVE_LOW;
> + }
> +
> + if ((common_flags & SOCAM_VSYNC_ACTIVE_HIGH) &&
> + (common_flags & SOCAM_VSYNC_ACTIVE_LOW)) {
> + if (isi->pdata->vsync_act_low)
> + common_flags &= ~SOCAM_VSYNC_ACTIVE_HIGH;
> + else
> + common_flags &= ~SOCAM_VSYNC_ACTIVE_LOW;
> + }
> +
> + if ((common_flags & SOCAM_PCLK_SAMPLE_RISING) &&
> + (common_flags & SOCAM_PCLK_SAMPLE_FALLING)) {
> + if (isi->pdata->pclk_act_falling)
> + common_flags &= ~SOCAM_PCLK_SAMPLE_RISING;
> + else
> + common_flags &= ~SOCAM_PCLK_SAMPLE_FALLING;
> + }
> +
> + ret = icd->ops->set_bus_param(icd, common_flags);
> + if (ret < 0) {
> + dev_dbg(icd->dev.parent, "camera set_bus_param(%lx) returned %d\n",
> + common_flags, ret);
> + return ret;
> + }
> +
> + /* set bus param for ISI */
> + if (common_flags & SOCAM_PCLK_SAMPLE_FALLING)
> + isi->pdata->pclk_act_falling = 1;
> + if (common_flags & SOCAM_HSYNC_ACTIVE_LOW)
> + isi->pdata->hsync_act_low = 1;
> + if (common_flags & SOCAM_VSYNC_ACTIVE_LOW)
> + isi->pdata->vsync_act_low = 1;
> +
> + cfg1 = ISI_BF(V2_EMB_SYNC, (isi->pdata->has_emb_sync))
> + | ISI_BF(V2_HSYNC_POL, isi->pdata->hsync_act_low)
> + | ISI_BF(V2_VSYNC_POL, isi->pdata->vsync_act_low)
> + | ISI_BF(V2_PIXCLK_POL, isi->pdata->pclk_act_falling)
> + | ISI_BF(V2_FULL, isi->pdata->isi_full_mode);
> +
> + ctrl = ISI_BIT(DIS);
> + isi_writel(isi, V2_CFG1, cfg1);
> + isi_writel(isi, V2_CTRL, ctrl);
> +
> + return 0;
> +}
> +
> +static struct soc_camera_host_ops isi_soc_camera_host_ops = {
> + .owner = THIS_MODULE,
> + .add = isi_camera_add_device,
> + .remove = isi_camera_remove_device,
> + .set_fmt = isi_camera_set_fmt,
> + .try_fmt = isi_camera_try_fmt,
> + .get_formats = isi_camera_get_formats,
> + .init_videobuf2 = isi_camera_init_videobuf,
> + .poll = isi_camera_poll,
> + .querycap = isi_camera_querycap,
> + .set_bus_param = isi_camera_set_bus_param,
> +};
> +
> +/* -----------------------------------------------------------------------*/
> +static int __exit atmel_isi_remove(struct platform_device *pdev)
> +{
> + struct soc_camera_host *soc_host = to_soc_camera_host(&pdev->dev);
> + struct atmel_isi *isi = container_of(soc_host,
> + struct atmel_isi, soc_host);
> +
> + soc_camera_host_unregister(soc_host);
> + vb2_dma_contig_cleanup_ctx(isi->alloc_ctx);
> +
> + free_irq(isi->irq, isi);
> + iounmap(isi->regs);
> + clk_disable(isi->pclk);
> + clk_put(isi->pclk);
> +
> + return 0;
> +}
> +
> +static int __init atmel_isi_probe(struct platform_device *pdev)
> +{
> + unsigned int irq;
> + struct atmel_isi *isi;
> + struct clk *pclk;
> + struct resource *regs;
> + int ret;
> + struct device *dev = &pdev->dev;
> + struct isi_platform_data *pdata;
> + struct soc_camera_host *soc_host;
> +
> + regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!regs)
> + return -ENXIO;
> +
> + pclk = clk_get(&pdev->dev, "isi_clk");
> + if (IS_ERR(pclk))
> + return PTR_ERR(pclk);
> +
> + clk_enable(pclk);
> +
> + isi = kzalloc(sizeof(struct atmel_isi), GFP_KERNEL);
> + if (!isi) {
> + ret = -ENOMEM;
> + dev_err(&pdev->dev, "can't allocate interface!\n");
> + goto err_alloc_isi;
> + }
> +
> + isi->pclk = pclk;
> +
> + spin_lock_init(&isi->lock);
> + init_waitqueue_head(&isi->capture_wq);
> +
> + isi->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev);
> + if (IS_ERR(isi->alloc_ctx)) {
> + ret = PTR_ERR(isi->alloc_ctx);
> + goto err_alloc_isi;
> + }
> +
> + isi->regs = ioremap(regs->start, resource_size(regs));
> + if (!isi->regs) {
> + ret = -ENOMEM;
> + goto err_ioremap;
> + }
> +
> + if (dev->platform_data)
> + pdata = (struct isi_platform_data *) dev->platform_data;
> + else {
> + static struct isi_platform_data isi_default_data = {
> + .frate = 0,
> + .has_emb_sync = 0,
> + .emb_crc_sync = 0,
> + .hsync_act_low = 0,
> + .vsync_act_low = 0,
> + .pclk_act_falling = 0,
> + .isi_full_mode = 1,
> + /* to use codec and preview path simultaneously */
> + .flags = ISI_DATAWIDTH_8 |
> + ISI_DATAWIDTH_10,
> + };
Move this struct definition outside of this function.
> + dev_info(&pdev->dev,
> + "No config available using default values\n");
> + pdata = &isi_default_data;
> + }
> +
> + isi->pdata = pdata;
> + isi->platform_flags = pdata->flags;
> + if (isi->platform_flags == 0)
> + isi->platform_flags = ISI_DATAWIDTH_8;
We could be mean here an require that people get the flags correct, e.g.
if (!((isi->platform_flags & ISI_DATA_WIDTH_8) ||
(isi->platform_flags & ISI_DATA_WIDTH_8))) {
dev_err(&pdev->dev, "No data width specified\n");
err = -ENOMEM;
goto fail;
}
Which discourages sloppy coding in the board files.
> +
> + isi_writel(isi, V2_CTRL, ISI_BIT(V2_DIS));
> + /* Check if module disable */
> + while (isi_readl(isi, V2_STATUS) & ISI_BIT(V2_DIS))
> + cpu_relax();
> +
> + irq = platform_get_irq(pdev, 0);
> + ret = request_irq(irq, isi_interrupt, 0, "isi", isi);
> + if (ret) {
> + dev_err(&pdev->dev, "unable to request irq %d\n", irq);
> + goto err_req_irq;
> + }
> + isi->irq = irq;
> +
> + INIT_LIST_HEAD(&isi->video_buffer_list);
> + isi->still_capture = 0;
> + isi->active = NULL;
> +
> + soc_host = &isi->soc_host;
> + soc_host->drv_name = "isi-camera";
> + soc_host->ops = &isi_soc_camera_host_ops;
> + soc_host->priv = isi;
> + soc_host->v4l2_dev.dev = &pdev->dev;
> + soc_host->nr = pdev->id;
> +
> + ret = soc_camera_host_register(soc_host);
> + if (ret) {
> + dev_err(&pdev->dev, "unable to register soc camera host\n");
> + goto err_register_soc_camera_host;
> + }
> + return 0;
> +
> +err_register_soc_camera_host:
> + free_irq(isi->irq, isi);
> +err_req_irq:
> + iounmap(isi->regs);
> +err_ioremap:
> + vb2_dma_contig_cleanup_ctx(isi->alloc_ctx);
> + kfree(isi);
> +err_alloc_isi:
> + clk_disable(pclk);
> +
> + return ret;
> +}
> +
> +static struct platform_driver atmel_isi_driver = {
> + .probe = atmel_isi_probe,
> + .remove = __exit_p(atmel_isi_remove),
> + .driver = {
> + .name = "atmel_isi",
> + .owner = THIS_MODULE,
> + },
> +};
> +
> +static int __init atmel_isi_init_module(void)
> +{
> + return platform_driver_probe(&atmel_isi_driver, &atmel_isi_probe);
> +}
> +
> +static void __exit atmel_isi_exit(void)
> +{
> + platform_driver_unregister(&atmel_isi_driver);
> +}
> +module_init(atmel_isi_init_module);
> +module_exit(atmel_isi_exit);
> +
> +MODULE_AUTHOR("Josh Wu <[email protected]>");
> +MODULE_DESCRIPTION("The V4L2 driver for Atmel Linux");
> +MODULE_LICENSE("GPL");
> +MODULE_SUPPORTED_DEVICE("video");
--
Bluewater Systems Ltd - ARM Technology Solution Centre
Ryan Mallon 5 Amuri Park, 404 Barbadoes St
[email protected] PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com New Zealand
Phone: +64 3 3779127 Freecall: Australia 1800 148 751
Fax: +64 3 3779135 USA 1800 261 2934
On 05/13/2011 12:14 AM, Guennadi Liakhovetski wrote:
> On Thu, 12 May 2011, Jean-Christophe PLAGNIOL-VILLARD wrote:
>
> [snip]
>
>>> + if (0 == *nbuffers)
>> please invert the test
>
> Don't think this is required by CodingStyle or anything like that. If it
> were, you'd have to revamp half of the kernel.
It should at least be consistent within a file, which it is not true in
this case. I think the preferred style is to have the variable on the left.
~Ryan
--
Bluewater Systems Ltd - ARM Technology Solution Centre
Ryan Mallon 5 Amuri Park, 404 Barbadoes St
[email protected] PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com New Zealand
Phone: +64 3 3779127 Freecall: Australia 1800 148 751
Fax: +64 3 3779135 USA 1800 261 2934
> > +
> > +/* Constants for RGB_CFG(ISI_V2) */
> > +#define ISI_V2_RGB_CFG_DEFAULT 0
> > +#define ISI_V2_RGB_CFG_MODE_1 1
> > +#define ISI_V2_RGB_CFG_MODE_2 2
> > +#define ISI_V2_RGB_CFG_MODE_3 3
> > +
> > +/* Bit manipulation macros */
> > +#define ISI_BIT(name) \
> > + (1 << ISI_##name##_OFFSET)
> > +#define ISI_BF(name, value) \
> > + (((value) & ((1 << ISI_##name##_SIZE) - 1)) \
> > + << ISI_##name##_OFFSET)
> > +#define ISI_BFEXT(name, value) \
> > + (((value) >> ISI_##name##_OFFSET) \
> > + & ((1 << ISI_##name##_SIZE) - 1))
> > +#define ISI_BFINS(name, value, old) \
> > + (((old) & ~(((1 << ISI_##name##_SIZE) - 1) \
> > + << ISI_##name##_OFFSET))\
> > + | ISI_BF(name, value))
>
> I really dislike this kind of register access magic. Not sure how others
> feel about it. These macros are really ugly.
>
> > +/* Register access macros */
> > +#define isi_readl(port, reg) \
> > + __raw_readl((port)->regs + ISI_##reg)
> > +#define isi_writel(port, reg, value) \
> > + __raw_writel((value), (port)->regs + ISI_##reg)
>
> If the token pasting stuff gets dropped then these can be static inline
> functions which is preferred.
Sorry this make the C code much readable
and this can not be done as a inline
so please keep it as is
>
> > +
> > +#define ATMEL_V4L2_VID_FLAGS (V4L2_CAP_VIDEO_OUTPUT)
> > +
> > +struct atmel_isi;
> > +
> > +enum atmel_isi_pixfmt {
> > + ATMEL_ISI_PIXFMT_GREY, /* Greyscale */
> > + dev_info(&pdev->dev,
> > + "No config available using default values\n");
> > + pdata = &isi_default_data;
> > + }
> > +
> > + isi->pdata = pdata;
> > + isi->platform_flags = pdata->flags;
> > + if (isi->platform_flags == 0)
> > + isi->platform_flags = ISI_DATAWIDTH_8;
>
> We could be mean here an require that people get the flags correct, e.g.
>
> if (!((isi->platform_flags & ISI_DATA_WIDTH_8) ||
> (isi->platform_flags & ISI_DATA_WIDTH_8))) {
> dev_err(&pdev->dev, "No data width specified\n");
> err = -ENOMEM;
> goto fail;
> }
>
> Which discourages sloppy coding in the board files.
if this means default configuration so not need to have all board to set it
>
> > +
> > + isi_writel(isi, V2_CTRL, ISI_BIT(V2_DIS));
> > + /* Check if module disable */
> > + while (isi_readl(isi, V2_STATUS) & ISI_BIT(V2_DIS))
> > + cpu_relax();
> > +
> > + irq = platform_get_irq(pdev, 0);
Best Regards,
J.
On Thursday, May 12, 2011 5:35 PM, Russell King wrote:
> A few more points...
>> +static int __init atmel_isi_probe(struct platform_device *pdev)
> Should be __devinit otherwise you'll have section errors.
Ok, will be fixed in V2 patch.
>> +{
>> + unsigned int irq;
>> + struct atmel_isi *isi;
>> + struct clk *pclk;
>> + struct resource *regs;
>> + int ret;
>> + struct device *dev = &pdev->dev;
>> + struct isi_platform_data *pdata;
>> + struct soc_camera_host *soc_host;
>> +
>> + regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (!regs)
>> + return -ENXIO;
>> +
>> + pclk = clk_get(&pdev->dev, "isi_clk");
>> + if (IS_ERR(pclk))
>> + return PTR_ERR(pclk);
>> +
>> + clk_enable(pclk);
> Return value of clk_enable() should be checked.
Yes. I will add code to check the return value in V2 patch.
>> +
>> + isi = kzalloc(sizeof(struct atmel_isi), GFP_KERNEL);
>> + if (!isi) {
>> + ret = -ENOMEM;
>> [...]
>> + isi_writel(isi, V2_CTRL, ISI_BIT(V2_DIS));
>> + /* Check if module disable */
>> + while (isi_readl(isi, V2_STATUS) & ISI_BIT(V2_DIS))
>> + cpu_relax();
>> +
>> + irq = platform_get_irq(pdev, 0);
> This should also be checked.
Ditto, thank you.
On Thu, 12 May 2011, Josh Wu wrote:
> This patch is to enable Atmel Image Sensor Interface (ISI) driver support.
> - Using soc-camera framework with videobuf2 dma-contig allocator
> - Supporting video streaming of YUV packed format
> - Tested on AT91SAM9M10G45-EK with OV2640
>
> Signed-off-by: Josh Wu <[email protected]>
> ---
> base on branch staging/for_v2.6.40
>
> arch/arm/mach-at91/include/mach/at91_isi.h | 454 ++++++++++++
> drivers/media/video/Kconfig | 10 +
> drivers/media/video/Makefile | 1 +
> drivers/media/video/atmel-isi.c | 1089 ++++++++++++++++++++++++++++
> 4 files changed, 1554 insertions(+), 0 deletions(-)
> create mode 100644 arch/arm/mach-at91/include/mach/at91_isi.h
> create mode 100644 drivers/media/video/atmel-isi.c
>
> diff --git a/arch/arm/mach-at91/include/mach/at91_isi.h b/arch/arm/mach-at91/include/mach/at91_isi.h
> new file mode 100644
> index 0000000..3219358
> --- /dev/null
> +++ b/arch/arm/mach-at91/include/mach/at91_isi.h
> @@ -0,0 +1,454 @@
> +/*
> + * Register definitions for the Atmel Image Sensor Interface.
Why do you put ISI register definitions in a header under arch/arm/...? It
seems only your driver needs them. Then at most put it directly under
drivers/media/video or even consider inlining all these definitions in the
driver itself, although, I admit, in such quantities it probably looks
cleaner in a separate header. But please put it under drivers/... Any
common stuff, that you will need in your board code to configure the
driver should go in a header under include/media/.
> + *
> + * Copyright (C) 2011 Atmel Corporation
Consider adding an author with an email address?
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#ifndef __AT91_ISI_H__
> +#define __AT91_ISI_H__
> +
> +#include <linux/videodev2.h>
> +#include <linux/i2c.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-common.h>
> +#include <media/v4l2-ioctl.h>
> +#include <media/v4l2-chip-ident.h>
None of these headers seems to be needed in the header itself. OTOH, you
do use __raw_readl()/writel(), which need something like linux/io.h. But
you'll, probably, remove those __raw_* from the header.
> +
> +/* ISI register offsets */
> +#define ISI_CR1 0x0000
> +#define ISI_CR2 0x0004
> +#define ISI_SR 0x0008
> +#define ISI_IER 0x000c
> +#define ISI_IDR 0x0010
> +#define ISI_IMR 0x0014
> +#define ISI_PSIZE 0x0020
> +#define ISI_PDECF 0x0024
> +#define ISI_PPFBD 0x0028
> +#define ISI_CDBA 0x002c
> +#define ISI_Y2R_SET0 0x0030
> +#define ISI_Y2R_SET1 0x0034
> +#define ISI_R2Y_SET0 0x0038
> +#define ISI_R2Y_SET1 0x003c
> +#define ISI_R2Y_SET2 0x0040
> +
> +/* ISI_V2 register offsets */
> +#define ISI_V2_CFG1 0x0000
> +#define ISI_V2_CFG2 0x0004
> +#define ISI_V2_PSIZE 0x0008
> +#define ISI_V2_PDECF 0x000c
> +#define ISI_V2_Y2R_SET0 0x0010
> +#define ISI_V2_Y2R_SET1 0x0014
> +#define ISI_V2_R2Y_SET0 0x0018
> +#define ISI_V2_R2Y_SET1 0x001C
> +#define ISI_V2_R2Y_SET2 0x0020
> +#define ISI_V2_CTRL 0x0024
> +#define ISI_V2_STATUS 0x0028
> +#define ISI_V2_INTEN 0x002C
> +#define ISI_V2_INTDIS 0x0030
> +#define ISI_V2_INTMASK 0x0034
> +#define ISI_V2_DMA_CHER 0x0038
> +#define ISI_V2_DMA_CHDR 0x003C
> +#define ISI_V2_DMA_CHSR 0x0040
> +#define ISI_V2_DMA_P_ADDR 0x0044
> +#define ISI_V2_DMA_P_CTRL 0x0048
> +#define ISI_V2_DMA_P_DSCR 0x004C
> +#define ISI_V2_DMA_C_ADDR 0x0050
> +#define ISI_V2_DMA_C_CTRL 0x0054
> +#define ISI_V2_DMA_C_DSCR 0x0058
> +
> +/* Bitfields in CR1 */
> +#define ISI_RST_OFFSET 0
> +#define ISI_RST_SIZE 1
> +#define ISI_DIS_OFFSET 1
> +#define ISI_DIS_SIZE 1
> +#define ISI_HSYNC_POL_OFFSET 2
> +#define ISI_HSYNC_POL_SIZE 1
> +#define ISI_VSYNC_POL_OFFSET 3
> +#define ISI_VSYNC_POL_SIZE 1
> +#define ISI_PIXCLK_POL_OFFSET 4
> +#define ISI_PIXCLK_POL_SIZE 1
> +#define ISI_EMB_SYNC_OFFSET 6
> +#define ISI_EMB_SYNC_SIZE 1
> +#define ISI_CRC_SYNC_OFFSET 7
> +#define ISI_CRC_SYNC_SIZE 1
> +#define ISI_FRATE_OFFSET 8
> +#define ISI_FRATE_SIZE 3
> +#define ISI_FULL_OFFSET 12
> +#define ISI_FULL_SIZE 1
> +#define ISI_THMASK_OFFSET 13
> +#define ISI_THMASK_SIZE 2
> +#define ISI_CODEC_ON_OFFSET 15
> +#define ISI_CODEC_ON_SIZE 1
> +#define ISI_SLD_OFFSET 16
> +#define ISI_SLD_SIZE 8
> +#define ISI_SFD_OFFSET 24
> +#define ISI_SFD_SIZE 8
> +
> +/* Bitfields in CFG1 */
> +#define ISI_V2_HSYNC_POL_OFFSET 2
> +#define ISI_V2_HSYNC_POL_SIZE 1
> +#define ISI_V2_VSYNC_POL_OFFSET 3
> +#define ISI_V2_VSYNC_POL_SIZE 1
> +#define ISI_V2_PIXCLK_POL_OFFSET 4
> +#define ISI_V2_PIXCLK_POL_SIZE 1
> +#define ISI_V2_EMB_SYNC_OFFSET 6
> +#define ISI_V2_EMB_SYNC_SIZE 1
> +#define ISI_V2_CRC_SYNC_OFFSET 7
> +#define ISI_V2_CRC_SYNC_SIZE 1
> +#define ISI_V2_FRATE_OFFSET 8
> +#define ISI_V2_FRATE_SIZE 3
> +#define ISI_V2_DISCR_OFFSET 11
> +#define ISI_V2_DISCR_SIZE 1
> +#define ISI_V2_FULL_OFFSET 12
> +#define ISI_V2_FULL_SIZE 1
> +#define ISI_V2_THMASK_OFFSET 13
> +#define ISI_V2_THMASK_SIZE 2
> +#define ISI_V2_SLD_OFFSET 16
> +#define ISI_V2_SLD_SIZE 8
> +#define ISI_V2_SFD_OFFSET 24
> +#define ISI_V2_SFD_SIZE 8
> +
> +/* Bitfields in CR2 */
> +#define ISI_IM_VSIZE_OFFSET 0
> +#define ISI_IM_VSIZE_SIZE 11
> +#define ISI_GS_MODE_OFFSET 11
> +#define ISI_GS_MODE_SIZE 1
> +#define ISI_RGB_MODE_OFFSET 12
> +#define ISI_RGB_MODE_SIZE 1
> +#define ISI_GRAYSCALE_OFFSET 13
> +#define ISI_GRAYSCALE_SIZE 1
> +#define ISI_RGB_SWAP_OFFSET 14
> +#define ISI_RGB_SWAP_SIZE 1
> +#define ISI_COL_SPACE_OFFSET 15
> +#define ISI_COL_SPACE_SIZE 1
> +#define ISI_IM_HSIZE_OFFSET 16
> +#define ISI_IM_HSIZE_SIZE 11
> +#define ISI_YCC_SWAP_OFFSET 28
> +#define ISI_YCC_SWAP_SIZE 2
> +#define ISI_RGB_CFG_OFFSET 30
> +#define ISI_RGB_CFG_SIZE 2
> +
> +/* Bitfields in CFG2 */
> +#define ISI_V2_IM_VSIZE_OFFSET 0
> +#define ISI_V2_IM_VSIZE_SIZE 11
> +#define ISI_V2_GS_MODE_OFFSET 11
> +#define ISI_V2_GS_MODE_SIZE 1
> +#define ISI_V2_RGB_MODE_OFFSET 12
> +#define ISI_V2_RGB_MODE_SIZE 1
> +#define ISI_V2_GRAYSCALE_OFFSET 13
> +#define ISI_V2_GRAYSCALE_SIZE 1
> +#define ISI_V2_RGB_SWAP_OFFSET 14
> +#define ISI_V2_RGB_SWAP_SIZE 1
> +#define ISI_V2_COL_SPACE_OFFSET 15
> +#define ISI_V2_COL_SPACE_SIZE 1
> +#define ISI_V2_IM_HSIZE_OFFSET 16
> +#define ISI_V2_IM_HSIZE_SIZE 11
> +#define ISI_V2_YCC_SWAP_OFFSET 28
> +#define ISI_V2_YCC_SWAP_SIZE 2
> +#define ISI_V2_RGB_CFG_OFFSET 30
> +#define ISI_V2_RGB_CFG_SIZE 2
> +
> +/* Bitfields in CTRL */
> +#define ISI_V2_EN_OFFSET 0
> +#define ISI_V2_EN_SIZE 1
> +#define ISI_V2_DIS_OFFSET 1
> +#define ISI_V2_DIS_SIZE 1
> +#define ISI_V2_SRST_OFFSET 2
> +#define ISI_V2_SRST_SIZE 1
> +#define ISI_V2_CDC_OFFSET 8
> +#define ISI_V2_CDC_SIZE 1
> +
> +/* Bitfields in SR/IER/IDR/IMR */
> +#define ISI_SOF_OFFSET 0
> +#define ISI_SOF_SIZE 1
> +#define ISI_SOFTRST_OFFSET 2
> +#define ISI_SOFTRST_SIZE 1
> +#define ISI_CDC_STATUS_OFFSET 3
> +#define ISI_CDC_STATUS_SIZE 1
> +#define ISI_CRC_ERR_OFFSET 4
> +#define ISI_CRC_ERR_SIZE 1
> +#define ISI_FO_C_OVF_OFFSET 5
> +#define ISI_FO_C_OVF_SIZE 1
> +#define ISI_FO_P_OVF_OFFSET 6
> +#define ISI_FO_P_OVF_SIZE 1
> +#define ISI_FO_P_EMP_OFFSET 7
> +#define ISI_FO_P_EMP_SIZE 1
> +#define ISI_FO_C_EMP_OFFSET 8
> +#define ISI_FO_C_EMP_SIZE 1
> +#define ISI_FR_OVR_OFFSET 9
> +#define ISI_FR_OVR_SIZE 1
> +
> +/* Bitfields in SR/IER/IDR/IMR(ISI_V2) */
> +#define ISI_V2_ENABLE_OFFSET 0
> +#define ISI_V2_ENABLE_SIZE 1
> +#define ISI_V2_DIS_DONE_OFFSET 1
> +#define ISI_V2_DIS_DONE_SIZE 1
> +#define ISI_V2_SRST_OFFSET 2
> +#define ISI_V2_SRST_SIZE 1
> +#define ISI_V2_CDC_STATUS_OFFSET 8
> +#define ISI_V2_CDC_STATUS_SIZE 1
> +#define ISI_V2_VSYNC_OFFSET 10
> +#define ISI_V2_VSYNC_SIZE 1
> +#define ISI_V2_PXFR_DONE_OFFSET 16
> +#define ISI_V2_PXFR_DONE_SIZE 1
> +#define ISI_V2_CXFR_DONE_OFFSET 17
> +#define ISI_V2_CXFR_DONE_SIZE 1
> +#define ISI_V2_P_OVR_OFFSET 24
> +#define ISI_V2_P_OVR_SIZE 1
> +#define ISI_V2_C_OVR_OFFSET 25
> +#define ISI_V2_C_OVR_SIZE 1
> +#define ISI_V2_CRC_ERR_OFFSET 26
> +#define ISI_V2_CRC_ERR_SIZE 1
> +#define ISI_V2_FR_OVR_OFFSET 27
> +#define ISI_V2_FR_OVR_SIZE 1
> +
> +/* Bitfields in PSIZE */
> +#define ISI_PREV_VSIZE_OFFSET 0
> +#define ISI_PREV_VSIZE_SIZE 10
> +#define ISI_PREV_HSIZE_OFFSET 16
> +#define ISI_PREV_HSIZE_SIZE 10
> +
> +/* Bitfields in PSIZE(ISI_V2) */
> +#define ISI_V2_PREV_VSIZE_OFFSET 0
> +#define ISI_V2_PREV_VSIZE_SIZE 10
> +#define ISI_V2_PREV_HSIZE_OFFSET 16
> +#define ISI_V2_PREV_HSIZE_SIZE 10
> +
> +/* Bitfields in PCDEF */
> +#define ISI_DEC_FACTOR_OFFSET 0
> +#define ISI_DEC_FACTOR_SIZE 8
> +
> +/* Bitfields in PCDEF */
> +#define ISI_V2_DEC_FACTOR_OFFSET 0
> +#define ISI_V2_DEC_FACTOR_SIZE 8
> +
> +/* Bitfields in PPFBD */
> +#define ISI_PREV_FBD_ADDR_OFFSET 0
> +#define ISI_PREV_FBD_ADDR_SIZE 32
> +
> +/* Bitfields in CDBA */
> +#define ISI_CODEC_DMA_ADDR_OFFSET 0
> +#define ISI_CODEC_DMA_ADDR_SIZE 32
> +
> +/* Bitfields in DMA_C_ADDR */
> +#define ISI_V2_DMA_ADDR_OFFSET 0
> +#define ISI_V2_DMA_ADDR_SIZE 32
> +
> +/* Bitfields in DMA_C_CTRL & in DMA_P_CTRL */
> +#define ISI_V2_DMA_FETCH_OFFSET 0
> +#define ISI_V2_DMA_FETCH_SIZE 1
> +#define ISI_V2_DMA_WB_OFFSET 1
> +#define ISI_V2_DMA_WB_SIZE 1
> +#define ISI_V2_DMA_IEN_OFFSET 2
> +#define ISI_V2_DMA_IEN_SIZE 1
> +#define ISI_V2_DMA_DONE_OFFSET 3
> +#define ISI_V2_DMA_DONE_SIZE 1
> +
> +/* Bitfields in DMA_CHER */
> +#define ISI_V2_DMA_P_CH_EN_OFFSET 0
> +#define ISI_V2_DMA_P_CH_EN_SIZE 1
> +#define ISI_V2_DMA_C_CH_EN_OFFSET 1
> +#define ISI_V2_DMA_C_CH_EN_SIZE 1
> +
> +/* Bitfields in Y2R_SET0 */
> +#define ISI_Y2R_SET0_C3_OFFSET 24
> +#define ISI_Y2R_SET0_C3_SIZE 8
> +
> +/* Bitfields in Y2R_SET0(ISI_V2) */
> +#define ISI_V2_Y2R_SET0_C3_OFFSET 24
> +#define ISI_V2_Y2R_SET0_C3_SIZE 8
> +
> +/* Bitfields in Y2R_SET1 */
> +#define ISI_Y2R_SET1_C4_OFFSET 0
> +#define ISI_Y2R_SET1_C4_SIZE 9
> +#define ISI_YOFF_OFFSET 12
> +#define ISI_YOFF_SIZE 1
> +#define ISI_CROFF_OFFSET 13
> +#define ISI_CROFF_SIZE 1
> +#define ISI_CBOFF_OFFSET 14
> +#define ISI_CBOFF_SIZE 1
> +
> +/* Bitfields in Y2R_SET1(ISI_V2) */
> +#define ISI_V2_Y2R_SET1_C4_OFFSET 0
> +#define ISI_V2_Y2R_SET1_C4_SIZE 9
> +#define ISI_V2_YOFF_OFFSET 12
> +#define ISI_V2_YOFF_SIZE 1
> +#define ISI_V2_CROFF_OFFSET 13
> +#define ISI_V2_CROFF_SIZE 1
> +#define ISI_V2_CBOFF_OFFSET 14
> +#define ISI_V2_CBOFF_SIZE 1
> +
> +/* Bitfields in R2Y_SET0 */
> +#define ISI_C0_OFFSET 0
> +#define ISI_C0_SIZE 8
> +#define ISI_C1_OFFSET 8
> +#define ISI_C1_SIZE 8
> +#define ISI_C2_OFFSET 16
> +#define ISI_C2_SIZE 8
> +#define ISI_ROFF_OFFSET 24
> +#define ISI_ROFF_SIZE 1
> +
> +/* Bitfields in R2Y_SET0(ISI_V2) */
> +#define ISI_V2_C0_OFFSET 0
> +#define ISI_V2_C0_SIZE 8
> +#define ISI_V2_C1_OFFSET 8
> +#define ISI_V2_C1_SIZE 8
> +#define ISI_V2_C2_OFFSET 16
> +#define ISI_V2_C2_SIZE 8
> +#define ISI_V2_ROFF_OFFSET 24
> +#define ISI_V2_ROFF_SIZE 1
> +
> +/* Bitfields in R2Y_SET1 */
> +#define ISI_R2Y_SET1_C3_OFFSET 0
> +#define ISI_R2Y_SET1_C3_SIZE 8
> +#define ISI_R2Y_SET1_C4_OFFSET 8
> +#define ISI_R2Y_SET1_C4_SIZE 8
> +#define ISI_C5_OFFSET 16
> +#define ISI_C5_SIZE 8
> +#define ISI_GOFF_OFFSET 24
> +#define ISI_GOFF_SIZE 1
> +
> +/* Bitfields in R2Y_SET1(ISI_V2) */
> +#define ISI_V2_R2Y_SET1_C3_OFFSET 0
> +#define ISI_V2_R2Y_SET1_C3_SIZE 8
> +#define ISI_V2_R2Y_SET1_C4_OFFSET 8
> +#define ISI_V2_R2Y_SET1_C4_SIZE 8
> +#define ISI_V2_C5_OFFSET 16
> +#define ISI_V2_C5_SIZE 8
> +#define ISI_V2_GOFF_OFFSET 24
> +#define ISI_V2_GOFF_SIZE 1
> +
> +/* Bitfields in R2Y_SET2 */
> +#define ISI_C6_OFFSET 0
> +#define ISI_C6_SIZE 8
> +#define ISI_C7_OFFSET 8
> +#define ISI_C7_SIZE 8
> +#define ISI_C8_OFFSET 16
> +#define ISI_C8_SIZE 8
> +#define ISI_BOFF_OFFSET 24
> +#define ISI_BOFF_SIZE 1
> +
> +/* Bitfields in R2Y_SET2(ISI_V2) */
> +#define ISI_V2_C6_OFFSET 0
> +#define ISI_V2_C6_SIZE 8
> +#define ISI_V2_C7_OFFSET 8
> +#define ISI_V2_C7_SIZE 8
> +#define ISI_V2_C8_OFFSET 16
> +#define ISI_V2_C8_SIZE 8
> +#define ISI_V2_BOFF_OFFSET 24
> +#define ISI_V2_BOFF_SIZE 1
> +
> +/* Constants for FRATE */
> +#define ISI_FRATE_CAPTURE_ALL 0
> +
> +/* Constants for FRATE(ISI_V2) */
> +#define ISI_V2_FRATE_CAPTURE_ALL 0
> +
> +/* Constants for YCC_SWAP */
> +#define ISI_YCC_SWAP_DEFAULT 0
> +#define ISI_YCC_SWAP_MODE_1 1
> +#define ISI_YCC_SWAP_MODE_2 2
> +#define ISI_YCC_SWAP_MODE_3 3
> +
> +/* Constants for YCC_SWAP(ISI_V2) */
> +#define ISI_V2_YCC_SWAP_DEFAULT 0
> +#define ISI_V2_YCC_SWAP_MODE_1 1
> +#define ISI_V2_YCC_SWAP_MODE_2 2
> +#define ISI_V2_YCC_SWAP_MODE_3 3
> +
> +/* Constants for RGB_CFG */
> +#define ISI_RGB_CFG_DEFAULT 0
> +#define ISI_RGB_CFG_MODE_1 1
> +#define ISI_RGB_CFG_MODE_2 2
> +#define ISI_RGB_CFG_MODE_3 3
> +
> +/* Constants for RGB_CFG(ISI_V2) */
> +#define ISI_V2_RGB_CFG_DEFAULT 0
> +#define ISI_V2_RGB_CFG_MODE_1 1
> +#define ISI_V2_RGB_CFG_MODE_2 2
> +#define ISI_V2_RGB_CFG_MODE_3 3
> +
Let's see if we can get rid of these without cluttering the driver too
much. The idea would be to use full register names in the code and use
inlines instead of macros, as also others have suggested. In any case all
these accessors, whether as macros or as inlines can and should fo into
the driver .c file.
> +/* Bit manipulation macros */
> +#define ISI_BIT(name) \
> + (1 << ISI_##name##_OFFSET)
This macro is used quite a lot, but it only uses the *_OFFSET to the
bitfield.
> +#define ISI_BF(name, value) \
> + (((value) & ((1 << ISI_##name##_SIZE) - 1)) \
> + << ISI_##name##_OFFSET)
> +#define ISI_BFEXT(name, value) \
> + (((value) >> ISI_##name##_OFFSET) \
> + & ((1 << ISI_##name##_SIZE) - 1))
This one is not used at all. Please, remove.
> +#define ISI_BFINS(name, value, old) \
> + (((old) & ~(((1 << ISI_##name##_SIZE) - 1) \
> + << ISI_##name##_OFFSET))\
> + | ISI_BF(name, value))
This one is only used twice, would be easy to convert.
Ok, put it this way: I will not be the one, who will block this driver just
because of these macros. It would be nicer to replace them with inlines,
but this doesn't look like a serious enough issue for me. At least, please,
move them to the .c file and remove unused ones.
> +
> +/* Register access macros */
> +#define isi_readl(port, reg) \
> + __raw_readl((port)->regs + ISI_##reg)
> +#define isi_writel(port, reg, value) \
> + __raw_writel((value), (port)->regs + ISI_##reg)
Same for these two: inlines would be nicer, but I won't fight. At least
move them to the .c.
> +
> +#define ATMEL_V4L2_VID_FLAGS (V4L2_CAP_VIDEO_OUTPUT)
??? What is this macro for? and it is never used. Please, remove.
> +
> +struct atmel_isi;
Seems unneeded.
> +
> +enum atmel_isi_pixfmt {
> + ATMEL_ISI_PIXFMT_GREY, /* Greyscale */
> + ATMEL_ISI_PIXFMT_CbYCrY,
> + ATMEL_ISI_PIXFMT_CrYCbY,
> + ATMEL_ISI_PIXFMT_YCbYCr,
> + ATMEL_ISI_PIXFMT_YCrYCb,
> + ATMEL_ISI_PIXFMT_RGB24,
> + ATMEL_ISI_PIXFMT_BGR24,
> + ATMEL_ISI_PIXFMT_RGB16,
> + ATMEL_ISI_PIXFMT_BGR16,
> + ATMEL_ISI_PIXFMT_GRB16, /* G[2:0] R[4:0]/B[4:0] G[5:3] */
> + ATMEL_ISI_PIXFMT_GBR16, /* G[2:0] B[4:0]/R[4:0] G[5:3] */
> + ATMEL_ISI_PIXFMT_RGB24_REV,
> + ATMEL_ISI_PIXFMT_BGR24_REV,
> + ATMEL_ISI_PIXFMT_RGB16_REV,
> + ATMEL_ISI_PIXFMT_BGR16_REV,
> + ATMEL_ISI_PIXFMT_GRB16_REV, /* G[2:0] R[4:0]/B[4:0] G[5:3] */
> + ATMEL_ISI_PIXFMT_GBR16_REV, /* G[2:0] B[4:0]/R[4:0] G[5:3] */
> +};
Huh... none of these are used. Please, remove.
> +
> +struct isi_platform_data {
> + u8 has_emb_sync;
> + u8 emb_crc_sync;
> + u8 hsync_act_low;
> + u8 vsync_act_low;
> + u8 pclk_act_falling;
> + u8 isi_full_mode;
> +#define ISI_HSYNC_ACT_LOW 0x01
> +#define ISI_VSYNC_ACT_LOW 0x02
> +#define ISI_PXCLK_ACT_FALLING 0x04
> +#define ISI_EMB_SYNC 0x08
> +#define ISI_CRC_SYNC 0x10
> +#define ISI_FULL 0x20
> +#define ISI_DATAWIDTH_8 0x40
> +#define ISI_DATAWIDTH_10 0x80
> + u32 flags;
> + u8 gs_mode;
> +#define ISI_GS_2PIX_PER_WORD 0x00
> +#define ISI_GS_1PIX_PER_WORD 0x01
> + u8 pixfmt;
> + u8 sfd;
> + u8 sld;
> + u8 thmask;
> +#define ISI_BURST_4_8_16 0x00
> +#define ISI_BURST_8_16 0x01
> +#define ISI_BURST_16 0x02
> + u8 frate;
> +#define ISI_FRATE_DIV_2 0x01
> +#define ISI_FRATE_DIV_3 0x02
> +#define ISI_FRATE_DIV_4 0x03
> +#define ISI_FRATE_DIV_5 0x04
> +#define ISI_FRATE_DIV_6 0x05
> +#define ISI_FRATE_DIV_7 0x06
> +#define ISI_FRATE_DIV_8 0x07
> +};
This is the only struct, that is needed in your external header, please,
put it in include/media/atmel-isi.h or similar. And you'll have to
#include <linux/types.h> in it.
> +
> +#endif /* __AT91_ISI_H__ */
> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
> index d61414e..eae6005 100644
> --- a/drivers/media/video/Kconfig
> +++ b/drivers/media/video/Kconfig
> @@ -80,6 +80,16 @@ menuconfig VIDEO_CAPTURE_DRIVERS
> Some of those devices also supports FM radio.
>
> if VIDEO_CAPTURE_DRIVERS && VIDEO_V4L2
> +config VIDEO_ATMEL_ISI
> + tristate "ATMEL Image Sensor Interface (ISI) support"
> + depends on VIDEO_DEV && SOC_CAMERA
Other reviewers asked you to add a dependency on AT91 / AVR32. At the first
glance your driver doesn't have any arch dependencies, which is good! I.e.,
it should compile on any platform (even if it won't work;)). So, I would
verify this, just try to compile it in a x86 config. If this is possible -
please, keep it arch-independent to allow for broad and easy
compile-testing.
But you probably need a couple more depends: HAVE_CLK and, maybe, HAS_DMA?
> + select VIDEOBUF2_DMA_CONTIG
> + default n
> + ---help---
> + This module makes the ATMEL Image Sensor Interface available
> + as a v4l2 device.
> + Say Y here to enable selecting the Image Sensor Interface.
> + When in doubt, say N.
I would drop the last two lines. The "doubt" statement makes sense for
generic options, where you can have doubts:) For specific hardware drivers
you usually know, whether you have them and need drivers for them.
>
> config VIDEO_ADV_DEBUG
> bool "Enable advanced debug functionality"
> diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
> index a10e4c3..f734a65 100644
> --- a/drivers/media/video/Makefile
> +++ b/drivers/media/video/Makefile
> @@ -166,6 +166,7 @@ obj-$(CONFIG_VIDEO_SH_MOBILE_CSI2) += sh_mobile_csi2.o
> obj-$(CONFIG_VIDEO_SH_MOBILE_CEU) += sh_mobile_ceu_camera.o
> obj-$(CONFIG_VIDEO_OMAP1) += omap1_camera.o
> obj-$(CONFIG_VIDEO_SAMSUNG_S5P_FIMC) += s5p-fimc/
[OT] hm, wow, who has decided to put a generic V4L driver (set) in the
Makefile together with other soc-camera drivers? It has to be converted now;)
> +obj-$(CONFIG_VIDEO_ATMEL_ISI) += atmel-isi.o
>
> obj-$(CONFIG_ARCH_DAVINCI) += davinci/
>
> diff --git a/drivers/media/video/atmel-isi.c b/drivers/media/video/atmel-isi.c
> new file mode 100644
> index 0000000..33d0b83
> --- /dev/null
> +++ b/drivers/media/video/atmel-isi.c
> @@ -0,0 +1,1089 @@
> +/*
> + * Copyright (c) 2011 Atmel Corporation
Same: Author / email would be nice.
> + *
> + * Based on previous work by Lars Haring and Sedji Gaouaou
> + *
> + * Based on the bttv driver for Bt848 with respective copyright holders
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/completion.h>
completion unused.
> +#include <linux/fs.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
moduleparam unused
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/version.h>
Maybe unused, if you remove KERNEL_VERSION.
> +#include <linux/kfifo.h>
Probably unused
> +
> +#include <mach/board.h>
> +#include <mach/cpu.h>
Why do you need these two?
> +#include <mach/at91_isi.h>
> +
> +#include <media/videobuf2-dma-contig.h>
> +#include <media/soc_camera.h>
> +#include <media/soc_mediabus.h>
> +
> +#define ATMEL_ISI_VERSION KERNEL_VERSION(1, 0, 0)
Some suggested removing this. Well, you have to put something into
cap->version, and other drivers do this. So, I don't mind that much.
> +#define MAX_BUFFER_NUMS 32
> +#define MAX_SUPPORT_WIDTH 2048
> +#define MAX_SUPPORT_HEIGHT 2048
> +
> +static unsigned int vid_limit = 16;
As someone pointed out: only used once, is constant, a macro would do.
> +
> +enum isi_buffer_state {
> + ISI_BUF_NEEDS_INIT,
> + ISI_BUF_PREPARED,
> +};
You only verify your buffer state once: in your .buf_prepare(). I think,
you can just remove these.
> +
> +/* Single frame capturing states */
> +enum {
> + STATE_IDLE = 0,
> + STATE_CAPTURE_READY,
> + STATE_CAPTURE_WAIT_SOF,
> + STATE_CAPTURE_IN_PROGRESS,
> + STATE_CAPTURE_DONE,
> + STATE_CAPTURE_ERROR,
> +};
V4L2 doesn't support still image capture yet, and in your it is just a
dummy, probably, left over from earlier versions, that implemented it,
using proprietary ioctl()s. Good, that you removed those, now, please,
also remove all traces of the still image mode. If these states are only
needed for it, they should be removed too. They seem to be used for normal
streaming too, then the comment should be changed.
> +
> +/* Frame buffer descriptor
> + * Used by the ISI module as a linked list for the DMA controller.
> + */
> +struct fbd {
> + /* Physical address of the frame buffer */
> + u32 fb_address;
> +#if defined(CONFIG_ARCH_AT91SAM9G45) ||\
> + defined(CONFIG_ARCH_AT91SAM9X5)
> + /* DMA Control Register(only in HISI2) */
> + u32 dma_ctrl;
> +#endif
As others have pointed out - there's nothing special in these dma_ctrl
fields, #ifd's should be just removed.
> + /* Physical address of the next fbd */
> + u32 next_fbd_address;
> +};
> +
> +#if defined(CONFIG_ARCH_AT91SAM9G45) ||\
> + defined(CONFIG_ARCH_AT91SAM9X5)
ditto
> +static void set_dma_ctrl(struct fbd *fb_desc, u32 ctrl)
> +{
> + fb_desc->dma_ctrl = ctrl;
> +}
> +#else
> +static void set_dma_ctrl(struct fbd *fb_desc, u32 ctrl) { }
> +#endif
> +
> +/* Frame buffer data
> + */
> +struct frame_buffer {
> + struct vb2_buffer vb;
> + struct fbd fb_desc;
> + /* Frame number of the frame */
> + unsigned long sequence;
This "sequence" seems to be unused. Please, remove.
> +
> + enum isi_buffer_state dma_desc_status;
This will go, I think.
> + struct list_head list;
> +};
> +
> +struct atmel_isi {
> + /* ISI module spin lock. Protects against concurrent access of variables
> + * that are shared with the ISR */
multiline comment style.
> + spinlock_t lock;
> + void __iomem *regs;
> +
> + /* If set ISI is in still capture mode */
> + int still_capture;
please, remove.
> + int sequence;
> + /* State of the ISI module in capturing mode */
> + int state;
> +
> + /* Capture/streaming wait queue for waiting for SOF */
> + wait_queue_head_t capture_wq;
> +
> + struct v4l2_device v4l2_dev;
Unneeded. Remove.
> +
> + struct vb2_alloc_ctx *alloc_ctx;
> +
> + struct clk *pclk;
> + struct platform_device *pdev;
Seems unused.
> + unsigned int irq;
> +
> + struct isi_platform_data *pdata;
> + unsigned long platform_flags;
> +
> + struct list_head video_buffer_list;
> + struct frame_buffer *active;
> +
> + struct soc_camera_device *icd;
> + struct soc_camera_host soc_host;
> +};
> +
> +static int configure_geometry(struct atmel_isi *isi, u32 width,
> + u32 height, enum v4l2_mbus_pixelcode code)
> +{
> + u32 cfg2, cr, ctrl;
> +
> + cr = 0;
initialisation superfluous.
> + switch (code) {
> + /* YUV, including grey */
> + case V4L2_MBUS_FMT_Y8_1X8:
> + cr = ISI_BIT(GRAYSCALE);
> + break;
> + case V4L2_MBUS_FMT_UYVY8_2X8:
> + cr = ISI_BF(V2_YCC_SWAP, 3);
> + break;
> + case V4L2_MBUS_FMT_VYUY8_2X8:
> + cr = ISI_BF(V2_YCC_SWAP, 2);
> + break;
> + case V4L2_MBUS_FMT_YUYV8_2X8:
> + cr = ISI_BF(V2_YCC_SWAP, 1);
> + break;
> + case V4L2_MBUS_FMT_YVYU8_2X8:
> + cr = ISI_BF(V2_YCC_SWAP, 0);
> + break;
> + /* RGB, TODO */
> + default:
> + return -EINVAL;
> + }
> +
> + ctrl = ISI_BIT(DIS);
> + isi_writel(isi, V2_CTRL, ctrl);
> + /* Check if module properly disable */
> + while (isi_readl(isi, V2_STATUS) & ISI_BIT(V2_DIS_DONE))
> + cpu_relax();
As others pointed out - busy loops are better with a timeout / a counter.
> +
> + cfg2 = isi_readl(isi, V2_CFG2);
> + cfg2 |= cr;
> + cfg2 = ISI_BFINS(V2_IM_VSIZE, height - 1, cfg2);
> + cfg2 = ISI_BFINS(V2_IM_HSIZE, width - 1, cfg2);
> + isi_writel(isi, V2_CFG2, cfg2);
> +
> + return 0;
> +}
> +
> +static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
> +{
> + if (isi->active) {
> + struct vb2_buffer *vb = &isi->active->vb;
> + struct frame_buffer *buf = isi->active;
> +
> + list_del_init(&buf->list);
> + do_gettimeofday(&vb->v4l2_buf.timestamp);
> + vb->v4l2_buf.sequence = isi->sequence++;
> + vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
> + }
> +
> + if (list_empty(&isi->video_buffer_list)) {
> + isi->active = NULL;
> + } else {
> + /* start next dma frame. */
> + isi->active = list_entry(isi->video_buffer_list.next,
> + struct frame_buffer, list);
> + isi_writel(isi, V2_DMA_C_DSCR, (__pa(&(isi->active->fb_desc))));
Ouch... No __pa(), please. Use proper remapping.
> + isi_writel(isi, V2_DMA_C_CTRL,
> + ISI_BIT(V2_DMA_FETCH) | ISI_BIT(V2_DMA_DONE));
> + isi_writel(isi, V2_DMA_CHER, ISI_BIT(V2_DMA_C_CH_EN));
> + }
> + return IRQ_HANDLED;
> +}
> +
> +/* ISI interrupt service routine */
> +static irqreturn_t isi_interrupt(int irq, void *dev_id)
> +{
> + struct atmel_isi *isi = dev_id;
> + u32 status, mask, pending;
> + irqreturn_t ret = IRQ_NONE;
> +
> + spin_lock(&isi->lock);
> +
> + status = isi_readl(isi, V2_STATUS);
> + mask = isi_readl(isi, V2_INTMASK);
> + pending = status & mask;
> +
> + if (!isi->still_capture) {
> + if (pending & (ISI_BIT(V2_VSYNC))) {
> + switch (isi->state) {
> + case STATE_IDLE:
> + isi->state = STATE_CAPTURE_READY;
> + wake_up_interruptible(&isi->capture_wq);
> + break;
> + }
> + } else if (likely(pending & (ISI_BIT(V2_CXFR_DONE)))) {
> + ret = atmel_isi_handle_streaming(isi);
> + }
> + } else {
This whole case should go - snapshot is not supported.
> + while (pending) {
> + if (pending & (ISI_BIT(V2_C_OVR) | ISI_BIT(V2_FR_OVR)))
> + printk(KERN_ERR "Overrun (status = 0x%x)\n",
> + status);
> + else if (pending &
> + (ISI_BIT(V2_VSYNC) | ISI_BIT(V2_CDC))) {
> + switch (isi->state) {
> + case STATE_IDLE:
> + isi->state = STATE_CAPTURE_READY;
> + wake_up_interruptible(&isi->capture_wq);
> + break;
> + case STATE_CAPTURE_READY:
> + break;
Just omit this case.
> + case STATE_CAPTURE_WAIT_SOF:
> + isi->state = STATE_CAPTURE_IN_PROGRESS;
> + break;
> + }
> + }
> +
> + if (likely(pending & (ISI_BIT(V2_CXFR_DONE))))
> + ret = atmel_isi_handle_streaming(isi);
> +
> + status = isi_readl(isi, V2_STATUS);
> + mask = isi_readl(isi, V2_INTMASK);
> + pending = status & mask;
> + ret = IRQ_HANDLED;
> + }
> + }
> + spin_unlock(&isi->lock);
> +
> + return ret;
> +}
> +
> +static int atmel_isi_init(struct atmel_isi *isi)
> +{
> + /*
> + * The reset will only succeed if we have a
> + * pixel clock from the camera.
> + */
And what happens if you don't?
> + isi_writel(isi, V2_CTRL, ISI_BIT(V2_SRST));
> + isi_writel(isi, V2_INTDIS, ~0UL);
> +
> + return 0;
It cannot fail, so, make it void.
> +}
> +
> +/* ------------------------------------------------------------------
> + Videobuf operations
> + ------------------------------------------------------------------*/
> +static int queue_setup(struct vb2_queue *vq, unsigned int *nbuffers,
> + unsigned int *nplanes, unsigned long sizes[],
> + void *alloc_ctxs[])
> +{
> + struct soc_camera_device *icd = soc_camera_from_vb2q(vq);
> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> + struct atmel_isi *dev = ici->priv;
> + unsigned long size;
> +
> + int bytes_per_line = soc_mbus_bytes_per_line(icd->user_width,
> + icd->current_fmt->host_fmt);
> +
> + if (bytes_per_line < 0)
> + return bytes_per_line;
> +
> + size = bytes_per_line * icd->user_height;
> +
> + if (0 == *nbuffers)
> + *nbuffers = MAX_BUFFER_NUMS;
> + if (*nbuffers > MAX_BUFFER_NUMS)
> + *nbuffers = MAX_BUFFER_NUMS;
> +
> + while (size * *nbuffers > vid_limit * 1024 * 1024)
> + (*nbuffers)--;
Please, use a division.
> +
> + *nplanes = 1;
> + sizes[0] = size;
> + alloc_ctxs[0] = dev->alloc_ctx;
> +
> + dev->sequence = 0;
> + dev->active = NULL;
> +
> + dev_dbg(icd->dev.parent, "%s, count=%d, size=%ld\n", __func__,
> + *nbuffers, size);
> +
> + return 0;
> +}
> +
> +static int buffer_init(struct vb2_buffer *vb)
> +{
> + struct frame_buffer *buf = container_of(vb, struct frame_buffer, vb);
> +
> + buf->dma_desc_status = ISI_BUF_NEEDS_INIT;
> + INIT_LIST_HEAD(&buf->list);
> +
> + return 0;
> +}
> +
> +static int buffer_prepare(struct vb2_buffer *vb)
> +{
> + struct soc_camera_device *icd = soc_camera_from_vb2q(vb->vb2_queue);
> + struct frame_buffer *buf = container_of(vb, struct frame_buffer, vb);
> + unsigned long size;
> + int bytes_per_line = soc_mbus_bytes_per_line(icd->user_width,
> + icd->current_fmt->host_fmt);
> +
> + if (bytes_per_line < 0)
> + return bytes_per_line;
> +
> + size = bytes_per_line * icd->user_height;
> +
> + if (vb2_plane_size(vb, 0) < size) {
> + dev_err(icd->dev.parent, "%s data will not fit into plane (%lu < %lu)\n",
> + __func__, vb2_plane_size(vb, 0), size);
> + return -EINVAL;
> + }
> +
> + vb2_set_plane_payload(&buf->vb, 0, size);
> +
> + if (buf->dma_desc_status == ISI_BUF_NEEDS_INIT) {
> + buf->dma_desc_status = ISI_BUF_PREPARED;
> +
> + /* initialize the dma descriptor */
> + buf->fb_desc.fb_address = vb2_dma_contig_plane_paddr(vb, 0);
> + buf->fb_desc.next_fbd_address = 0;
> + set_dma_ctrl(&buf->fb_desc, ISI_BIT(V2_DMA_WB));
This your descriptor: you either have to allocate it yourself in
.buf_init(), using dma_alloc_coherent(), and free in .buf_cleanup(),
using dma_free_coherent(), or you could try to calculate .sizeimage on your
own, adding the size of your descriptor with required alignment, and rely
on the DMA contiguous buffer allocator to allocate the extra space for you,
and then just calculate offset to it.
An interesting question is also: have you really tested it with USERPTR? If
not, maybe it's better to also drop that capability from the driver for now.
> + }
> +
> + return 0;
> +}
> +
> +static int buffer_finish(struct vb2_buffer *vb)
> +{
> + return 0;
> +}
Don't think you need this one.
> +
> +static void buffer_cleanup(struct vb2_buffer *vb)
> +{
> + struct soc_camera_device *icd = soc_camera_from_vb2q(vb->vb2_queue);
> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> + struct atmel_isi *isi = ici->priv;
> + struct frame_buffer *buf = container_of(vb, struct frame_buffer, vb);
> + unsigned long flags = 0;
No need to initialise.
> +
> + spin_lock_irqsave(&isi->lock, flags);
> + buf->dma_desc_status = ISI_BUF_NEEDS_INIT;
> + spin_unlock_irqrestore(&isi->lock, flags);
This seems bogus... Hope, .dma_desc_status will disappear completely.
> +}
> +
> +static void start_dma(struct atmel_isi *isi, struct frame_buffer *buffer)
> +{
> + u32 ctrl, cfg1;
> + ctrl = isi_readl(isi, V2_CTRL);
> + cfg1 = isi_readl(isi, V2_CFG1);
> + /* Enable irq: cxfr for the codec path, pxfr for the preview path */
> + isi_writel(isi, V2_INTEN,
> + ISI_BIT(V2_CXFR_DONE) | ISI_BIT(V2_PXFR_DONE));
> +
> + /* Enable codec path */
> + ctrl |= ISI_BIT(V2_CDC);
> + /* Check if already in a frame */
> + while (isi_readl(isi, V2_STATUS) & ISI_BIT(V2_CDC))
> + cpu_relax();
Again - don't run endlessly, especially under spin_lock_irqsave()...
> +
> + /* Write the address of the first frame buffer in the C_ADDR reg
> + * write the address of the first descriptor(link list of buffer)
> + * in the C_DSCR reg, and enable dma channel.
> + */
> + isi_writel(isi, V2_DMA_C_DSCR, (__pa(&(buffer->fb_desc))));
Use remapping
> + isi_writel(isi, V2_DMA_C_CTRL,
> + ISI_BIT(V2_DMA_FETCH) | ISI_BIT(V2_DMA_DONE));
> + isi_writel(isi, V2_DMA_CHER, ISI_BIT(V2_DMA_C_CH_EN));
> +
> + /* Enable linked list */
> + cfg1 |= ISI_BF(V2_FRATE, isi->pdata->frate) | ISI_BIT(V2_DISCR);
> +
> + /* Enable ISI module*/
> + ctrl |= ISI_BIT(V2_ENABLE);
> + isi_writel(isi, V2_CTRL, ctrl);
> + isi_writel(isi, V2_CFG1, cfg1);
> +}
> +
> +static void buffer_queue(struct vb2_buffer *vb)
> +{
> + struct soc_camera_device *icd = soc_camera_from_vb2q(vb->vb2_queue);
> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> + struct atmel_isi *isi = ici->priv;
> + struct frame_buffer *buf = container_of(vb, struct frame_buffer, vb);
> + unsigned long flags = 0;
> +
> + spin_lock_irqsave(&isi->lock, flags);
> + list_add_tail(&buf->list, &isi->video_buffer_list);
> +
> + if (isi->active == NULL) {
> + isi->active = buf;
> + start_dma(isi, buf);
> + }
> + spin_unlock_irqrestore(&isi->lock, flags);
> +}
> +
> +static int start_streaming(struct vb2_queue *vq)
> +{
> + struct soc_camera_device *icd = soc_camera_from_vb2q(vq);
> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> + struct atmel_isi *isi = ici->priv;
> +
> + u32 sr = 0;
> + int ret;
> +
> + spin_lock_irq(&isi->lock);
> + isi->state = STATE_IDLE;
> +
> + /* Clear any pending SOF interrupt */
> + sr = isi_readl(isi, V2_STATUS);
> + /* Enable VSYNC interrupt for SOF */
> + isi_writel(isi, V2_INTEN, ISI_BIT(V2_VSYNC));
> + isi_writel(isi, V2_CTRL, isi_readl(isi, V2_CTRL) | ISI_BIT(V2_EN));
> +
> + spin_unlock_irq(&isi->lock);
> + dev_dbg(icd->dev.parent, "isi: waiting for SOF\n");
> + ret = wait_event_interruptible(isi->capture_wq,
> + isi->state != STATE_IDLE);
> + if (ret)
> + return ret;
> +
> + if (isi->state != STATE_CAPTURE_READY)
> + return -EIO;
> +
> + spin_lock_irq(&isi->lock);
> + isi->state = STATE_CAPTURE_WAIT_SOF;
> + isi_writel(isi, V2_INTDIS, ISI_BIT(V2_VSYNC));
> + spin_unlock_irq(&isi->lock);
> +
> + return 0;
> +}
> +
> +/* abort streaming and wait for last buffer */
> +static int stop_streaming(struct vb2_queue *vq)
> +{
> + struct soc_camera_device *icd = soc_camera_from_vb2q(vq);
> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> + struct atmel_isi *isi = ici->priv;
> +
> + spin_lock_irq(&isi->lock);
> + isi->still_capture = 0;
> + isi->active = NULL;
> +
> + while (isi_readl(isi, V2_STATUS) & ISI_BIT(V2_CDC))
> + cpu_relax();
timeout / counter - under spinlock...
> +
> + /* Disble codec path */
> + isi_writel(isi, V2_CTRL, isi_readl(isi, V2_CTRL) & (~ISI_BIT(V2_CDC)));
> + /* Disable interrupts */
> + isi_writel(isi, V2_INTDIS,
> + ISI_BIT(V2_CXFR_DONE) | ISI_BIT(V2_PXFR_DONE));
> + /* Disable ISI module*/
> + isi_writel(isi, V2_CTRL, isi_readl(isi, V2_CTRL) | ISI_BIT(V2_DIS));
> +
> + /* Release all active buffers */
> + while (!list_empty(&isi->video_buffer_list)) {
Use some kind of list_for_each(_entry)_safe()?
> + struct frame_buffer *buf;
> + buf = list_entry(isi->video_buffer_list.next,
> + struct frame_buffer, list);
> + list_del_init(&buf->list);
> + vb2_buffer_done(&buf->vb, VB2_BUF_STATE_ERROR);
> + }
> +
> + spin_unlock_irq(&isi->lock);
> + return 0;
> +}
> +
> +static struct vb2_ops isi_video_qops = {
> + .queue_setup = queue_setup,
> + .buf_init = buffer_init,
> + .buf_prepare = buffer_prepare,
> + .buf_finish = buffer_finish,
> + .buf_cleanup = buffer_cleanup,
> + .buf_queue = buffer_queue,
> + .start_streaming = start_streaming,
> + .stop_streaming = stop_streaming,
> + .wait_prepare = soc_camera_unlock,
> + .wait_finish = soc_camera_lock,
> +};
> +
> +/* ------------------------------------------------------------------
> + SOC camera operations for the device
> + ------------------------------------------------------------------*/
> +static int isi_camera_init_videobuf(struct vb2_queue *q,
> + struct soc_camera_device *icd)
> +{
> + q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> + q->io_modes = VB2_MMAP | VB2_USERPTR | VB2_READ;
I don't think VB2_READ makes sense there.
> + q->drv_priv = icd;
> + q->buf_struct_size = sizeof(struct frame_buffer);
> + q->ops = &isi_video_qops;
> + q->mem_ops = &vb2_dma_contig_memops;
> +
> + return vb2_queue_init(q);
> +}
> +static inline void stride_align(u32 *width)
> +{
> + if (((*width + 7) & ~7) < 4096)
> + *width = (*width + 7) & ~7;
Use ALIGN(*width, 8). Yes, mx3_camera.c should do the same;) But do you
actually need 8-byte width alignment?
> + else
> + *width = *width & ~7;
> +}
> +
> +static int isi_camera_set_fmt(struct soc_camera_device *icd,
> + struct v4l2_format *f)
> +{
> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> + struct atmel_isi *isi = ici->priv;
> + 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;
> + }
> +
> + stride_align(&pix->width);
> + dev_dbg(icd->dev.parent, "plan to set format %dx%d\n",
> + pix->width, pix->height);
> +
> + 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)
> + return ret;
> +
> + if (mf.code != xlate->code)
> + return -EINVAL;
> +
> + ret = configure_geometry(isi, pix->width, pix->height, xlate->code);
> + if (ret < 0)
> + return ret;
> +
> + pix->width = mf.width;
> + pix->height = mf.height;
> + pix->field = mf.field;
> + pix->colorspace = mf.colorspace;
> + icd->current_fmt = xlate;
> +
> + pix->bytesperline = soc_mbus_bytes_per_line(pix->width,
> + xlate->host_fmt);
> + if (pix->bytesperline < 0)
> + return pix->bytesperline;
> + pix->sizeimage = pix->height * pix->bytesperline;
.bytesperline and .sizeimage should be calculated automatically as of
commit b12795e1dd07e0fc3cb1030b4860d0a4e1c17e87 (in next).
> +
> + dev_dbg(icd->dev.parent, "Finally set format %dx%d, sizeimage = %d\n",
> + pix->width, pix->height, pix->sizeimage);
> +
> + return ret;
> +}
> +
> +static int isi_camera_try_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;
> + __u32 pixfmt = pix->pixelformat;
> + int ret;
> +
> + xlate = soc_camera_xlate_by_fourcc(icd, pixfmt);
> + if (pixfmt && !xlate) {
> + dev_warn(icd->dev.parent, "Format %x not found\n", pixfmt);
> + return -EINVAL;
> + }
> +
> + /* limit to Atmel ISI hardware capabilities */
> + if (pix->height > MAX_SUPPORT_HEIGHT)
> + pix->height = MAX_SUPPORT_HEIGHT;
> + if (pix->width > MAX_SUPPORT_WIDTH)
> + pix->width = MAX_SUPPORT_WIDTH;
> +
> + pix->bytesperline = soc_mbus_bytes_per_line(pix->width,
> + xlate->host_fmt);
> + if (pix->bytesperline < 0)
> + return pix->bytesperline;
> + pix->sizeimage = pix->height * pix->bytesperline;
No need for ->bytesperline and ->sizeimage either.
> +
> + /* limit to sensor capabilities */
> + 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, try_mbus_fmt, &mf);
> + if (ret < 0)
> + return ret;
> +
> + pix->width = mf.width;
> + pix->height = mf.height;
> + pix->colorspace = mf.colorspace;
> +
> + switch (mf.field) {
> + case V4L2_FIELD_ANY:
> + pix->field = V4L2_FIELD_NONE;
> + break;
> + case V4L2_FIELD_NONE:
> + break;
> + default:
> + dev_err(icd->dev.parent, "Field type %d unsupported.\n",
> + mf.field);
> + ret = -EINVAL;
> + }
> +
> + return ret;
> +}
> +
> +static const struct soc_mbus_pixelfmt isi_camera_formats[] = {
> + {
> + .fourcc = V4L2_PIX_FMT_YUYV,
> + .name = "Packed YUV422 16 bit",
> + .bits_per_sample = 8,
> + .packing = SOC_MBUS_PACKING_2X8_PADHI,
> + .order = SOC_MBUS_ORDER_LE,
> + },
> +};
> +
> +/* This will be corrected as we get more formats */
> +static bool isi_camera_packing_supported(const struct soc_mbus_pixelfmt *fmt)
> +{
> + return fmt->packing == SOC_MBUS_PACKING_NONE ||
> + (fmt->bits_per_sample == 8 &&
> + fmt->packing == SOC_MBUS_PACKING_2X8_PADHI) ||
> + (fmt->bits_per_sample > 8 &&
> + fmt->packing == SOC_MBUS_PACKING_EXTEND16);
> +}
> +
> +static int test_platform_param(struct atmel_isi *isi,
> + unsigned char buswidth, unsigned long *flags)
> +{
> + /*
> + * Platform specified synchronization and pixel clock polarities are
> + * only a recommendation and are only used during probing. Atmel ISI
> + * camera interface only works in master mode, i.e., uses HSYNC and
> + * VSYNC signals from the sensor
> + */
> + *flags = SOCAM_MASTER |
> + SOCAM_HSYNC_ACTIVE_HIGH |
> + SOCAM_HSYNC_ACTIVE_LOW |
> + SOCAM_VSYNC_ACTIVE_HIGH |
> + SOCAM_VSYNC_ACTIVE_LOW |
> + SOCAM_PCLK_SAMPLE_RISING |
> + SOCAM_PCLK_SAMPLE_FALLING |
> + SOCAM_DATA_ACTIVE_HIGH;
> +
> + /* If requested data width is supported by the platform, use it */
> + switch (buswidth) {
> + case 10:
> + if (!(isi->platform_flags & ISI_DATAWIDTH_10))
> + return -EINVAL;
> + *flags |= SOCAM_DATAWIDTH_10;
> + break;
> + case 8:
> + if (!(isi->platform_flags & ISI_DATAWIDTH_8))
> + return -EINVAL;
> + *flags |= SOCAM_DATAWIDTH_8;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int isi_camera_try_bus_param(struct soc_camera_device *icd,
> + unsigned char buswidth)
> +{
> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> + struct atmel_isi *isi = ici->priv;
> + unsigned long bus_flags, camera_flags;
> + int ret = test_platform_param(isi, buswidth, &bus_flags);
> +
> + if (ret < 0)
> + return ret;
> +
> + camera_flags = icd->ops->query_bus_param(icd);
> +
> + ret = soc_camera_bus_param_compatible(camera_flags, bus_flags);
> + if (!ret)
> + return -EINVAL;
> + return 0;
> +}
> +
> +
> +static int isi_camera_get_formats(struct soc_camera_device *icd,
> + unsigned int idx,
> + struct soc_camera_format_xlate *xlate)
> +{
> + struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
> + struct device *dev = icd->dev.parent;
> + int formats = 0, ret;
> + /* sensor format */
> + enum v4l2_mbus_pixelcode code;
> + /* soc camera host format */
> + const struct soc_mbus_pixelfmt *fmt;
> +
> + ret = v4l2_subdev_call(sd, video, enum_mbus_fmt, idx, &code);
> + if (ret < 0)
> + /* No more formats */
> + return 0;
> +
> + fmt = soc_mbus_get_fmtdesc(code);
> + if (!fmt) {
> + dev_err(icd->dev.parent,
> + "Invalid format code #%u: %d\n", idx, code);
> + return 0;
> + }
> +
> + /* This also checks support for the requested bits-per-sample */
> + ret = isi_camera_try_bus_param(icd, fmt->bits_per_sample);
> + if (ret < 0) {
> + dev_err(icd->dev.parent,
> + "Fail to try the bus parameters.\n");
> + return 0;
> + }
> +
> + switch (code) {
> + case V4L2_MBUS_FMT_UYVY8_2X8:
> + case V4L2_MBUS_FMT_VYUY8_2X8:
> + case V4L2_MBUS_FMT_YUYV8_2X8:
> + case V4L2_MBUS_FMT_YVYU8_2X8:
> + formats++;
> + if (xlate) {
> + xlate->host_fmt = &isi_camera_formats[0];
> + xlate->code = code;
> + xlate++;
> + dev_dbg(icd->dev.parent, "Providing format %s using code %d\n",
> + isi_camera_formats[0].name, code);
> + }
> + break;
> + default:
> + if (!isi_camera_packing_supported(fmt))
> + return 0;
> + if (xlate)
> + dev_dbg(dev,
> + "Providing format %s in pass-through mode\n",
> + fmt->name);
> + }
> +
> + /* Generic pass-through */
> + formats++;
> + if (xlate) {
> + xlate->host_fmt = fmt;
> + xlate->code = code;
> + xlate++;
> + }
> +
> + return formats;
> +}
> +
> +/* Called with .video_lock held */
> +static int isi_camera_add_device(struct soc_camera_device *icd)
> +{
> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> + struct atmel_isi *isi = ici->priv;
> + int ret;
> +
> + if (isi->icd)
> + return -EBUSY;
> +
> + ret = atmel_isi_init(isi);
> + if (ret)
> + return ret;
> +
> + isi->icd = icd;
> + dev_info(icd->dev.parent, "Atmel ISI Camera driver attached to camera %d\n",
> + icd->devnum);
> + return 0;
> +}
> +/* Called with .video_lock held */
> +static void isi_camera_remove_device(struct soc_camera_device *icd)
> +{
> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> + struct atmel_isi *isi = ici->priv;
> +
> + BUG_ON(icd != isi->icd);
> +
> + isi_writel(isi, V2_CTRL, ISI_BIT(V2_DIS));
> + /* Check if module disable */
> + while (isi_readl(isi, V2_STATUS) & ISI_BIT(V2_DIS))
> + cpu_relax();
Timeout
> +
> + isi->icd = NULL;
> +
> + dev_info(icd->dev.parent, "Atmel ISI Camera driver detached from camera %d\n",
> + icd->devnum);
> +}
> +
> +static unsigned int isi_camera_poll(struct file *file, poll_table *pt)
> +{
> + struct soc_camera_device *icd = file->private_data;
> +
> + return vb2_poll(&icd->vb2_vidq, file, pt);
> +}
> +
> +static int isi_camera_querycap(struct soc_camera_host *ici,
> + struct v4l2_capability *cap)
> +{
> + strcpy(cap->driver, "atmel-isi");
> + strcpy(cap->card, "Atmel Image Sensor Interface");
> + cap->version = ATMEL_ISI_VERSION;
> +
> + cap->capabilities = (V4L2_CAP_VIDEO_CAPTURE
> + | V4L2_CAP_READWRITE
> + | V4L2_CAP_STREAMING
> + );
As someone commented: stay consistent, put '|' at the end of th line. And
you don't want to mention V4L2_CAP_READWRITE here.
> + return 0;
> +}
> +
> +static int isi_camera_set_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
> +{
> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> + struct atmel_isi *isi = ici->priv;
> + unsigned long bus_flags, camera_flags, common_flags;
> + const struct soc_mbus_pixelfmt *fmt;
> + int ret;
> + u32 cfg1, ctrl;
> +
> + fmt = soc_mbus_get_fmtdesc(icd->current_fmt->code);
> + if (!fmt)
> + return -EINVAL;
Hm, there seems to be a problem with this one. soc_mbus_get_fmtdesc() only
looks for the requested code in the internal mbus_fmt[] array. But it only
contains formats, for which we know their pass-through formats. E.g., This
does not work with driver additional formats. Drivers, that implement
native formats cannot use this function like here int their
.set_bus_param() methods. See sh_mobile_ceu_camera.c for a correct
implementation. But the soc_mbus_get_fmtdesc() and its other existing users
have to be fixed too...
> +
> + ret = test_platform_param(isi, fmt->bits_per_sample, &bus_flags);
> + if (ret < 0)
> + return ret;
> +
> + camera_flags = icd->ops->query_bus_param(icd);
> +
> + common_flags = soc_camera_bus_param_compatible(camera_flags, bus_flags);
> + dev_dbg(icd->dev.parent, "Flags cam: 0x%lx host: 0x%lx common: 0x%lx\n",
> + camera_flags, bus_flags, common_flags);
> + if (!common_flags)
> + return -EINVAL;
> +
> + /* Make choises, based on platform preferences */
> + if ((common_flags & SOCAM_HSYNC_ACTIVE_HIGH) &&
> + (common_flags & SOCAM_HSYNC_ACTIVE_LOW)) {
> + if (isi->pdata->hsync_act_low)
> + common_flags &= ~SOCAM_HSYNC_ACTIVE_HIGH;
> + else
> + common_flags &= ~SOCAM_HSYNC_ACTIVE_LOW;
> + }
> +
> + if ((common_flags & SOCAM_VSYNC_ACTIVE_HIGH) &&
> + (common_flags & SOCAM_VSYNC_ACTIVE_LOW)) {
> + if (isi->pdata->vsync_act_low)
> + common_flags &= ~SOCAM_VSYNC_ACTIVE_HIGH;
> + else
> + common_flags &= ~SOCAM_VSYNC_ACTIVE_LOW;
> + }
> +
> + if ((common_flags & SOCAM_PCLK_SAMPLE_RISING) &&
> + (common_flags & SOCAM_PCLK_SAMPLE_FALLING)) {
> + if (isi->pdata->pclk_act_falling)
> + common_flags &= ~SOCAM_PCLK_SAMPLE_RISING;
> + else
> + common_flags &= ~SOCAM_PCLK_SAMPLE_FALLING;
> + }
> +
> + ret = icd->ops->set_bus_param(icd, common_flags);
> + if (ret < 0) {
> + dev_dbg(icd->dev.parent, "camera set_bus_param(%lx) returned %d\n",
> + common_flags, ret);
> + return ret;
> + }
> +
> + /* set bus param for ISI */
> + if (common_flags & SOCAM_PCLK_SAMPLE_FALLING)
> + isi->pdata->pclk_act_falling = 1;
> + if (common_flags & SOCAM_HSYNC_ACTIVE_LOW)
> + isi->pdata->hsync_act_low = 1;
> + if (common_flags & SOCAM_VSYNC_ACTIVE_LOW)
> + isi->pdata->vsync_act_low = 1;
This modifies platform data, which is persistent even across module loading
and unloading. This will break set ups with multiple sensors. You should
make separate copies of these flags in your driver private data, if you
have to store them. But in fact, you only use these flags here, so, you
don't have to save them at all.
> +
> + cfg1 = ISI_BF(V2_EMB_SYNC, (isi->pdata->has_emb_sync))
> + | ISI_BF(V2_HSYNC_POL, isi->pdata->hsync_act_low)
> + | ISI_BF(V2_VSYNC_POL, isi->pdata->vsync_act_low)
> + | ISI_BF(V2_PIXCLK_POL, isi->pdata->pclk_act_falling)
> + | ISI_BF(V2_FULL, isi->pdata->isi_full_mode);
> +
> + ctrl = ISI_BIT(DIS);
> + isi_writel(isi, V2_CFG1, cfg1);
> + isi_writel(isi, V2_CTRL, ctrl);
> +
> + return 0;
> +}
> +
> +static struct soc_camera_host_ops isi_soc_camera_host_ops = {
> + .owner = THIS_MODULE,
> + .add = isi_camera_add_device,
> + .remove = isi_camera_remove_device,
> + .set_fmt = isi_camera_set_fmt,
> + .try_fmt = isi_camera_try_fmt,
> + .get_formats = isi_camera_get_formats,
> + .init_videobuf2 = isi_camera_init_videobuf,
> + .poll = isi_camera_poll,
> + .querycap = isi_camera_querycap,
> + .set_bus_param = isi_camera_set_bus_param,
> +};
> +
> +/* -----------------------------------------------------------------------*/
> +static int __exit atmel_isi_remove(struct platform_device *pdev)
> +{
> + struct soc_camera_host *soc_host = to_soc_camera_host(&pdev->dev);
> + struct atmel_isi *isi = container_of(soc_host,
> + struct atmel_isi, soc_host);
> +
> + soc_camera_host_unregister(soc_host);
> + vb2_dma_contig_cleanup_ctx(isi->alloc_ctx);
> +
> + free_irq(isi->irq, isi);
> + iounmap(isi->regs);
> + clk_disable(isi->pclk);
> + clk_put(isi->pclk);
> +
> + return 0;
> +}
> +
> +static int __init atmel_isi_probe(struct platform_device *pdev)
> +{
> + unsigned int irq;
> + struct atmel_isi *isi;
> + struct clk *pclk;
> + struct resource *regs;
> + int ret;
> + struct device *dev = &pdev->dev;
> + struct isi_platform_data *pdata;
> + struct soc_camera_host *soc_host;
> +
> + regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!regs)
> + return -ENXIO;
> +
> + pclk = clk_get(&pdev->dev, "isi_clk");
> + if (IS_ERR(pclk))
> + return PTR_ERR(pclk);
> +
> + clk_enable(pclk);
You've got already comments to the clock API usage.
> +
> + isi = kzalloc(sizeof(struct atmel_isi), GFP_KERNEL);
> + if (!isi) {
> + ret = -ENOMEM;
> + dev_err(&pdev->dev, "can't allocate interface!\n");
> + goto err_alloc_isi;
> + }
> +
> + isi->pclk = pclk;
> +
> + spin_lock_init(&isi->lock);
> + init_waitqueue_head(&isi->capture_wq);
> +
> + isi->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev);
> + if (IS_ERR(isi->alloc_ctx)) {
> + ret = PTR_ERR(isi->alloc_ctx);
> + goto err_alloc_isi;
> + }
> +
> + isi->regs = ioremap(regs->start, resource_size(regs));
> + if (!isi->regs) {
> + ret = -ENOMEM;
> + goto err_ioremap;
> + }
> +
> + if (dev->platform_data)
> + pdata = (struct isi_platform_data *) dev->platform_data;
> + else {
Missing braces
> + static struct isi_platform_data isi_default_data = {
> + .frate = 0,
> + .has_emb_sync = 0,
> + .emb_crc_sync = 0,
> + .hsync_act_low = 0,
> + .vsync_act_low = 0,
> + .pclk_act_falling = 0,
> + .isi_full_mode = 1,
> + /* to use codec and preview path simultaneously */
> + .flags = ISI_DATAWIDTH_8 |
> + ISI_DATAWIDTH_10,
> + };
Hm, I'm not in favour of hard-coding every single parameter, but I'm not
sure I like allowing platforms to completely omit platform data. And I
agree with a previous reviewer: a static struct doesn't look very nice
here.
> + dev_info(&pdev->dev,
> + "No config available using default values\n");
> + pdata = &isi_default_data;
> + }
> +
> + isi->pdata = pdata;
> + isi->platform_flags = pdata->flags;
> + if (isi->platform_flags == 0)
> + isi->platform_flags = ISI_DATAWIDTH_8;
> +
> + isi_writel(isi, V2_CTRL, ISI_BIT(V2_DIS));
> + /* Check if module disable */
> + while (isi_readl(isi, V2_STATUS) & ISI_BIT(V2_DIS))
> + cpu_relax();
> +
> + irq = platform_get_irq(pdev, 0);
> + ret = request_irq(irq, isi_interrupt, 0, "isi", isi);
> + if (ret) {
> + dev_err(&pdev->dev, "unable to request irq %d\n", irq);
> + goto err_req_irq;
> + }
> + isi->irq = irq;
> +
> + INIT_LIST_HEAD(&isi->video_buffer_list);
> + isi->still_capture = 0;
> + isi->active = NULL;
> +
> + soc_host = &isi->soc_host;
> + soc_host->drv_name = "isi-camera";
> + soc_host->ops = &isi_soc_camera_host_ops;
> + soc_host->priv = isi;
> + soc_host->v4l2_dev.dev = &pdev->dev;
> + soc_host->nr = pdev->id;
> +
> + ret = soc_camera_host_register(soc_host);
> + if (ret) {
> + dev_err(&pdev->dev, "unable to register soc camera host\n");
> + goto err_register_soc_camera_host;
> + }
> + return 0;
> +
> +err_register_soc_camera_host:
> + free_irq(isi->irq, isi);
> +err_req_irq:
> + iounmap(isi->regs);
> +err_ioremap:
> + vb2_dma_contig_cleanup_ctx(isi->alloc_ctx);
> + kfree(isi);
> +err_alloc_isi:
> + clk_disable(pclk);
> +
> + return ret;
> +}
> +
> +static struct platform_driver atmel_isi_driver = {
> + .probe = atmel_isi_probe,
> + .remove = __exit_p(atmel_isi_remove),
> + .driver = {
> + .name = "atmel_isi",
> + .owner = THIS_MODULE,
> + },
> +};
> +
> +static int __init atmel_isi_init_module(void)
> +{
> + return platform_driver_probe(&atmel_isi_driver, &atmel_isi_probe);
> +}
> +
> +static void __exit atmel_isi_exit(void)
> +{
> + platform_driver_unregister(&atmel_isi_driver);
> +}
> +module_init(atmel_isi_init_module);
> +module_exit(atmel_isi_exit);
> +
> +MODULE_AUTHOR("Josh Wu <[email protected]>");
> +MODULE_DESCRIPTION("The V4L2 driver for Atmel Linux");
> +MODULE_LICENSE("GPL");
> +MODULE_SUPPORTED_DEVICE("video");
> --
> 1.6.3.3
>
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
On Thursday 12 May 2011, Josh Wu wrote:
> This patch is to enable Atmel Image Sensor Interface (ISI) driver support.
> - Using soc-camera framework with videobuf2 dma-contig allocator
> - Supporting video streaming of YUV packed format
> - Tested on AT91SAM9M10G45-EK with OV2640
>
> Signed-off-by: Josh Wu <[email protected]>
This looks like a very well written driver for the most part. I have a few
comments, mainly regarding maintainability that you should probably look
into.
> arch/arm/mach-at91/include/mach/at91_isi.h | 454 ++++++++++++
> drivers/media/video/Kconfig | 10 +
> drivers/media/video/Makefile | 1 +
> drivers/media/video/atmel-isi.c | 1089 ++++++++++++++++++++++++++++
So you have a mach specific header file that is used by a single driver?
That does not lean itself it code reuse. Better move the header file into
the same directory as the driver, or (better) merge its contents into the
same file.
This is especially important if the driver is possibly shared with avr32
socs.
> +/* Constants for RGB_CFG */
> +#define ISI_RGB_CFG_DEFAULT 0
> +#define ISI_RGB_CFG_MODE_1 1
> +#define ISI_RGB_CFG_MODE_2 2
> +#define ISI_RGB_CFG_MODE_3 3
> +
> +/* Constants for RGB_CFG(ISI_V2) */
> +#define ISI_V2_RGB_CFG_DEFAULT 0
> +#define ISI_V2_RGB_CFG_MODE_1 1
> +#define ISI_V2_RGB_CFG_MODE_2 2
> +#define ISI_V2_RGB_CFG_MODE_3 3
> +
> +/* Bit manipulation macros */
> +#define ISI_BIT(name) \
> + (1 << ISI_##name##_OFFSET)
> +#define ISI_BF(name, value) \
> + (((value) & ((1 << ISI_##name##_SIZE) - 1)) \
> + << ISI_##name##_OFFSET)
> +#define ISI_BFEXT(name, value) \
> + (((value) >> ISI_##name##_OFFSET) \
> + & ((1 << ISI_##name##_SIZE) - 1))
> +#define ISI_BFINS(name, value, old) \
> + (((old) & ~(((1 << ISI_##name##_SIZE) - 1) \
> + << ISI_##name##_OFFSET))\
> + | ISI_BF(name, value))
A much better way to express the above would be to define the constants as
mask values rather than using the macros with bit numbers. There are
conflicting views on how bits are counted, plus the use of macros
makes it harder to grep for the idenfiers.
For example, do
#define ISI_RGB_CFG_DEFAULT 0x0001
#define ISI_RGB_CFG_MODE_1 0x0002
#define ISI_RGB_CFG_MODE_2 0x0004
#define ISI_RGB_CFG_MODE_3 0x0008
For the two examples I quoted, the values are actually the same,
so you might also want to name them so that you don't have to
define multiple versions but can simply reuse the same macros.
Some people also prefer the use of enum over #define, but I
leave that to your judgement
> +/* Register access macros */
> +#define isi_readl(port, reg) \
> + __raw_readl((port)->regs + ISI_##reg)
> +#define isi_writel(port, reg, value) \
> + __raw_writel((value), (port)->regs + ISI_##reg)
Please explain why you use __raw_* instead of the proper
accessors in the comment. Normal drivers should always
use readl/writel.
Better don't concatenate identifiers, in order to make
grep and ctags work on the arguments.
> +
> +#define ATMEL_ISI_VERSION KERNEL_VERSION(1, 0, 0)
Don't use KERNEL_VERSION() here, it means something else.
Better yet, don't define a version at all. It is not acceptable
to make any user space interface depend on specific versions,
this is a compatibility nightmare.
> +/* Frame buffer descriptor
> + * Used by the ISI module as a linked list for the DMA controller.
> + */
> +struct fbd {
> + /* Physical address of the frame buffer */
> + u32 fb_address;
> +#if defined(CONFIG_ARCH_AT91SAM9G45) ||\
> + defined(CONFIG_ARCH_AT91SAM9X5)
> + /* DMA Control Register(only in HISI2) */
> + u32 dma_ctrl;
> +#endif
> + /* Physical address of the next fbd */
> + u32 next_fbd_address;
> +};
> +
> +#if defined(CONFIG_ARCH_AT91SAM9G45) ||\
> + defined(CONFIG_ARCH_AT91SAM9X5)
> +static void set_dma_ctrl(struct fbd *fb_desc, u32 ctrl)
> +{
> + fb_desc->dma_ctrl = ctrl;
> +}
> +#else
> +static void set_dma_ctrl(struct fbd *fb_desc, u32 ctrl) { }
> +#endif
Make these runtime checks, not compile-time. Best define different
identifiers for the platform device, then key the differences off
the device, not the platform.
> +struct atmel_isi {
> + /* ISI module spin lock. Protects against concurrent access of variables
> + * that are shared with the ISR */
> + spinlock_t lock;
> + void __iomem *regs;
> +
> + /* If set ISI is in still capture mode */
> + int still_capture;
> + int sequence;
> + /* State of the ISI module in capturing mode */
> + int state;
> +
> + /* Capture/streaming wait queue for waiting for SOF */
> + wait_queue_head_t capture_wq;
> +
> + struct v4l2_device v4l2_dev;
> +
> + struct vb2_alloc_ctx *alloc_ctx;
> +
> + struct clk *pclk;
> + struct platform_device *pdev;
pdev is unused?
> + spin_lock_irq(&isi->lock);
> + isi->still_capture = 0;
> + isi->active = NULL;
> +
> + while (isi_readl(isi, V2_STATUS) & ISI_BIT(V2_CDC))
> + cpu_relax();
A busy loop with interrupts disabled is rather nasty.
Is there a guaranteed upper bound on how long this can take?
You have the same loop in other places, maybe you can move
it into a separate function with a comment explaining what
you wait for and how long it can take.
Arnd
On 05/13/2011 03:50 PM, Guennadi Liakhovetski wrote:
> On Thu, 12 May 2011, Josh Wu wrote:
>
>> This patch is to enable Atmel Image Sensor Interface (ISI) driver support.
>> - Using soc-camera framework with videobuf2 dma-contig allocator
>> - Supporting video streaming of YUV packed format
>> - Tested on AT91SAM9M10G45-EK with OV2640
>>
>> Signed-off-by: Josh Wu<[email protected]>
>> ---
...
>> diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
>> index a10e4c3..f734a65 100644
>> --- a/drivers/media/video/Makefile
>> +++ b/drivers/media/video/Makefile
>> @@ -166,6 +166,7 @@ obj-$(CONFIG_VIDEO_SH_MOBILE_CSI2) += sh_mobile_csi2.o
>> obj-$(CONFIG_VIDEO_SH_MOBILE_CEU) += sh_mobile_ceu_camera.o
>> obj-$(CONFIG_VIDEO_OMAP1) += omap1_camera.o
>> obj-$(CONFIG_VIDEO_SAMSUNG_S5P_FIMC) += s5p-fimc/
>
> [OT] hm, wow, who has decided to put a generic V4L driver (set) in the
> Makefile together with other soc-camera drivers? It has to be converted now;)
In fact I've already converted that driver, but soc-camera wasn't unfortunately
my choice, just the media controller ;-) I'll probably move the entry when
preparing upstream patches, it just doesn't feel safe here;)
>
>> +obj-$(CONFIG_VIDEO_ATMEL_ISI) += atmel-isi.o
>>
>> obj-$(CONFIG_ARCH_DAVINCI) += davinci/
--
Regards,
Sylwester Nawrocki
On Fri, 13 May 2011, Sylwester Nawrocki wrote:
> On 05/13/2011 03:50 PM, Guennadi Liakhovetski wrote:
> > On Thu, 12 May 2011, Josh Wu wrote:
> >
> >> This patch is to enable Atmel Image Sensor Interface (ISI) driver support.
> >> - Using soc-camera framework with videobuf2 dma-contig allocator
> >> - Supporting video streaming of YUV packed format
> >> - Tested on AT91SAM9M10G45-EK with OV2640
> >>
> >> Signed-off-by: Josh Wu<[email protected]>
> >> ---
> ...
> >> diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
> >> index a10e4c3..f734a65 100644
> >> --- a/drivers/media/video/Makefile
> >> +++ b/drivers/media/video/Makefile
> >> @@ -166,6 +166,7 @@ obj-$(CONFIG_VIDEO_SH_MOBILE_CSI2) += sh_mobile_csi2.o
> >> obj-$(CONFIG_VIDEO_SH_MOBILE_CEU) += sh_mobile_ceu_camera.o
> >> obj-$(CONFIG_VIDEO_OMAP1) += omap1_camera.o
> >> obj-$(CONFIG_VIDEO_SAMSUNG_S5P_FIMC) += s5p-fimc/
> >
> > [OT] hm, wow, who has decided to put a generic V4L driver (set) in the
> > Makefile together with other soc-camera drivers? It has to be converted now;)
>
> In fact I've already converted that driver, but soc-camera wasn't unfortunately
> my choice, just the media controller ;-) I'll probably move the entry when
> preparing upstream patches, it just doesn't feel safe here;)
Yeah, you're right - it's a boilerplate here, nothing like safe;)
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
Hi, JC
>> +struct atmel_isi;
> do we really this here?
Not really. I'll remove this.
>> +
>> [snip]
>>
>> if VIDEO_CAPTURE_DRIVERS && VIDEO_V4L2
>> +config VIDEO_ATMEL_ISI
>> + tristate "ATMEL Image Sensor Interface (ISI) support"
>> + depends on VIDEO_DEV && SOC_CAMERA
> depends on AT91 if the drivers is at91 specific or avr32 otherwise
I'll add that. I think now it is only supported AT91. I am not sure for the AVR32 part
>> + select VIDEOBUF2_DMA_CONTIG
>> + default n
> it's n by default please remove
I'll change that.
>> [snip]
>> +
>> +/* Frame buffer descriptor
>> + * Used by the ISI module as a linked list for the DMA controller.
>> + */
>> +struct fbd {
>> + /* Physical address of the frame buffer */
>> + u32 fb_address;
>> +#if defined(CONFIG_ARCH_AT91SAM9G45) ||\
>> + defined(CONFIG_ARCH_AT91SAM9X5)
>> + /* DMA Control Register(only in HISI2) */
>> + u32 dma_ctrl;
>> +#endif
> no ifdef in the struct
I'll remove this #if. I think for the non-HISI2 version, like AT91SAM9263, we should define another FBD structure which not includes dma_ctrl.
>> + /* Physical address of the next fbd */
>> + u32 next_fbd_address;
>> +};
>> +
>> +#if defined(CONFIG_ARCH_AT91SAM9G45) ||\
>> + defined(CONFIG_ARCH_AT91SAM9X5)
>> +static void set_dma_ctrl(struct fbd *fb_desc, u32 ctrl) {
>> + fb_desc->dma_ctrl = ctrl;
>> +}
>> +#else
>> +static void set_dma_ctrl(struct fbd *fb_desc, u32 ctrl) { } #endif
> no ifdef here also as we want to have multi soc support
I'll remove this #if also.
>> [snip]
>> + /* State of the ISI module in capturing mode */
>> + int state;
>> +
>> + /* Capture/streaming wait queue for waiting for SOF */
>> + wait_queue_head_t capture_wq;
>> +
>> + struct v4l2_device v4l2_dev;
>> +
>> + struct vb2_alloc_ctx *alloc_ctx;
>> +
>> + struct clk *pclk;
>> + struct platform_device *pdev;
> do you really need to store the pdev?
I'll remove this. It is no use.
>> + unsigned int irq;
>> +
>> + struct isi_platform_data *pdata;
>> + unsigned long platform_flags;
>> +
>> + struct list_head video_buffer_list;
>> + struct frame_buffer *active;
>> +
>> + struct soc_camera_device *icd;
>> + struct soc_camera_host soc_host;
>> +};
>> +
>> +static int configure_geometry(struct atmel_isi *isi, u32 width,
>> + u32 height, enum v4l2_mbus_pixelcode code) {
>> + u32 cfg2, cr, ctrl;
>> +
>> + cr = 0;
> please move this in default
I'll remove this cr line. Seems it is not needed. cr will be initialized by the following code.
>> [snip]
>> +
>> + size = bytes_per_line * icd->user_height;
>> +
>> + if (0 == *nbuffers)
> please invert the test
I'll fix it.
>> + *nbuffers = MAX_BUFFER_NUMS;
>> + if (*nbuffers > MAX_BUFFER_NUMS)
>> + *nbuffers = MAX_BUFFER_NUMS;
>> +
>> + while (size * *nbuffers > vid_limit * 1024 * 1024)
>> + (*nbuffers)--;
>> +
>> + *nplanes = 1;
>> + sizes[0] = size;
>> + alloc_ctxs[0] = dev->alloc_ctx;
>> +
>> + dev->sequence = 0;
>> + dev->active = NULL;
>> +
>> + dev_dbg(icd->dev.parent, "%s, count=%d, size=%ld\n", __func__,
>> + *nbuffers, size);
>> +
>> + return 0;
>> +}
>> +
>> +
>> +static void start_dma(struct atmel_isi *isi, struct frame_buffer
>> +*buffer) {
>> + u32 ctrl, cfg1;
> please add ine ligne here
OK. I'll change that.
>> + ctrl = isi_readl(isi, V2_CTRL);
>> + cfg1 = isi_readl(isi, V2_CFG1);
>> + /* Enable irq: cxfr for the codec path, pxfr for the preview path */
>> + isi_writel(isi, V2_INTEN,
>> + ISI_BIT(V2_CXFR_DONE) | ISI_BIT(V2_PXFR_DONE));
>> +
>> + /* Enable codec path */
>> + ctrl |= ISI_BIT(V2_CDC);
>> + /* Check if already in a frame */
>> + while (isi_readl(isi, V2_STATUS) & ISI_BIT(V2_CDC))
>> + cpu_relax();
> no timeout?
I'll add time out code and test it.
>> +
>> + /* Write the address of the first frame buffer in the C_ADDR reg
>> + * write the address of the first descriptor(link list of buffer)
>> + * in the C_DSCR reg, and enable dma channel.
>> + */
>> + isi_writel(isi, V2_DMA_C_DSCR, (__pa(&(buffer->fb_desc))));
>> + isi_writel(isi, V2_DMA_C_CTRL,
>> + ISI_BIT(V2_DMA_FETCH) | ISI_BIT(V2_DMA_DONE));
>> + isi_writel(isi, V2_DMA_CHER, ISI_BIT(V2_DMA_C_CH_EN));
>> +
>> + /* Enable linked list */
>> + cfg1 |= ISI_BF(V2_FRATE, isi->pdata->frate) | ISI_BIT(V2_DISCR);
>> +
>> + /* Enable ISI module*/
>> + ctrl |= ISI_BIT(V2_ENABLE);
>> + isi_writel(isi, V2_CTRL, ctrl);
>> + isi_writel(isi, V2_CFG1, cfg1);
>> +}
>> +
>> +
>> +/* abort streaming and wait for last buffer */ static int
>> +stop_streaming(struct vb2_queue *vq) {
>> + struct soc_camera_device *icd = soc_camera_from_vb2q(vq);
>> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
>> + struct atmel_isi *isi = ici->priv;
>> +
>> + spin_lock_irq(&isi->lock);
>> + isi->still_capture = 0;
>> + isi->active = NULL;
>> +
>> + while (isi_readl(isi, V2_STATUS) & ISI_BIT(V2_CDC))
>> + cpu_relax();
> ditto
I'll fix it too.
>> +
>> + /* Disble codec path */
>> + isi_writel(isi, V2_CTRL, isi_readl(isi, V2_CTRL) & (~ISI_BIT(V2_CDC)));
>> + /* Disable interrupts */
>> + isi_writel(isi, V2_INTDIS,
>> + ISI_BIT(V2_CXFR_DONE) | ISI_BIT(V2_PXFR_DONE));
>> + /* Disable ISI module*/
>> + isi_writel(isi, V2_CTRL, isi_readl(isi, V2_CTRL) | ISI_BIT(V2_DIS));
>> +
>> +
>> +static int __init atmel_isi_probe(struct platform_device *pdev) {
>> + unsigned int irq;
>> + struct atmel_isi *isi;
>> + struct clk *pclk;
>> + struct resource *regs;
>> + int ret;
>> + struct device *dev = &pdev->dev;
>> + struct isi_platform_data *pdata;
>> + struct soc_camera_host *soc_host;
>> +
>> + regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (!regs)
>> + return -ENXIO;
>> +
>> + pclk = clk_get(&pdev->dev, "isi_clk");
>> + if (IS_ERR(pclk))
>> + return PTR_ERR(pclk);
>> +
>> + clk_enable(pclk);
> do we really need to always enable the clock?
> normally we need to enable it just when we use the device and disable asap
Yes, you are right. I will move such code to the camera_add_device/camera_remove_device functions
> do you plane toadd the pm?
You mean the power resume/suspend function for ISI, right? Not planned yet. But if I have time I will try this.
Thank you for the comments. I will fix it in V2 patch.
Best Regards,
Josh Wu
On Friday, May 13, 2011 5:25 AM, Ryan Mallon wrote
> On 05/12/2011 07:42 PM, Josh Wu wrote:
>> This patch is to enable Atmel Image Sensor Interface (ISI) driver support.
>> - Using soc-camera framework with videobuf2 dma-contig allocator
>> - Supporting video streaming of YUV packed format
>> - Tested on AT91SAM9M10G45-EK with OV2640
> Hi Josh,
> Thansk for doing this. Overall the patch looks really good. A few
> comments below.
Hi, Ryan
Thank you for the comments.
>>
>> Signed-off-by: Josh Wu <[email protected]>
>> ---
>> base on branch staging/for_v2.6.40
>>
>> arch/arm/mach-at91/include/mach/at91_isi.h | 454 ++++++++++++
>> drivers/media/video/Kconfig | 10 +
>> drivers/media/video/Makefile | 1 +
>> drivers/media/video/atmel-isi.c | 1089 ++++++++++++++++++++++++++++
>> 4 files changed, 1554 insertions(+), 0 deletions(-)
>> create mode 100644 arch/arm/mach-at91/include/mach/at91_isi.h
>> create mode 100644 drivers/media/video/atmel-isi.c
>> [snip]
>> +
>> +/* Bit manipulation macros */
>> +#define ISI_BIT(name) \
>> + (1 << ISI_##name##_OFFSET)
>> +#define ISI_BF(name, value) \
>> + (((value) & ((1 << ISI_##name##_SIZE) - 1)) \
>> + << ISI_##name##_OFFSET)
>> +#define ISI_BFEXT(name, value) \
>> + (((value) >> ISI_##name##_OFFSET) \
>> + & ((1 << ISI_##name##_SIZE) - 1))
>> +#define ISI_BFINS(name, value, old) \
>> + (((old) & ~(((1 << ISI_##name##_SIZE) - 1) \
>> + << ISI_##name##_OFFSET))\
>> + | ISI_BF(name, value))
> I really dislike this kind of register access magic. Not sure how others
> feel about it. These macros are really ugly.
I understand this. So I will try to find a better way (static inline function) to solve this. :)
>> +/* Register access macros */
>> +#define isi_readl(port, reg) \
>> + __raw_readl((port)->regs + ISI_##reg)
>> +#define isi_writel(port, reg, value) \
>> + __raw_writel((value), (port)->regs + ISI_##reg)
> If the token pasting stuff gets dropped then these can be static inline
> functions which is preferred.
sure, I'll try.
>> [snip]
>> +#define ISI_GS_2PIX_PER_WORD 0x00
>> +#define ISI_GS_1PIX_PER_WORD 0x01
>> + u8 pixfmt;
>> + u8 sfd;
>> + u8 sld;
>> + u8 thmask;
>> +#define ISI_BURST_4_8_16 0x00
>> +#define ISI_BURST_8_16 0x01
>> +#define ISI_BURST_16 0x02
>> + u8 frate;
>> +#define ISI_FRATE_DIV_2 0x01
>> +#define ISI_FRATE_DIV_3 0x02
>> +#define ISI_FRATE_DIV_4 0x03
>> +#define ISI_FRATE_DIV_5 0x04
>> +#define ISI_FRATE_DIV_6 0x05
>> +#define ISI_FRATE_DIV_7 0x06
>> +#define ISI_FRATE_DIV_8 0x07
>> +};
> Might need some comments in this structure so that board file writers
> know what each of the fields are for.
I think this part will be change a little bit. Also I will add the updated comments.
>> +
>> +#endif /* __AT91_ISI_H__ */
>> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
>> index d61414e..eae6005 100644
>> --- a/drivers/media/video/Kconfig
>> +++ b/drivers/media/video/Kconfig
>> @@ -80,6 +80,16 @@ menuconfig VIDEO_CAPTURE_DRIVERS
>> Some of those devices also supports FM radio.
>>
>> if VIDEO_CAPTURE_DRIVERS && VIDEO_V4L2
>> +config VIDEO_ATMEL_ISI
>> + tristate "ATMEL Image Sensor Interface (ISI) support"
>> + depends on VIDEO_DEV && SOC_CAMERA
> Depends on AT91/AVR32?
I think I will use AT91
>> [snip]
>> +
>> +#include <media/videobuf2-dma-contig.h>
>> +#include <media/soc_camera.h>
>> +#include <media/soc_mediabus.h>
>> +
>> +#define ATMEL_ISI_VERSION KERNEL_VERSION(1, 0, 0)
> Do we really need this version?
Since we need set a version for v4l2_capability.version in function isi_camera_querycap(). So we use this macro.
How about this?
static int isi_camera_querycap(struct soc_camera_host *ici,
struct v4l2_capability *cap)
{
strcpy(cap->driver, "atmel-isi");
strcpy(cap->card, "Atmel Image Sensor Interface");
cap->version = KERNEL_VERSION(1, 0, 0);
cap->capabilities = (V4L2_CAP_VIDEO_CAPTURE |
V4L2_CAP_STREAMING);
return 0;
}
>> +#define MAX_BUFFER_NUMS 32
>> +#define MAX_SUPPORT_WIDTH 2048
>> +#define MAX_SUPPORT_HEIGHT 2048
>> +
>> +static unsigned int vid_limit = 16;
> This never gets changed so it can become a const/define. The value is
> MB, which is not clear from the name, and it gets multiplied out to
> bytes in its only usage, so maybe:
> #define VID_LIMIT_BYTES (16 * 1024 * 1024)
Your code is good. I will change according to your suggestion.
>> +
>> +enum isi_buffer_state {
>> + ISI_BUF_NEEDS_INIT,
>> + ISI_BUF_PREPARED,
>> +};
> Aren't there v4l2 constants for this already?
> VIDEOBUF_NEEDS_INIT,
> VIDEOBUF_PREPARED,
> Just reuse those.
I checked the code, above constants are used in videobuf not videobuf2. So I think I cannot use it.
>> +
>> +/* Single frame capturing states */
>> +enum {
>> + STATE_IDLE = 0,
>> + STATE_CAPTURE_READY,
>> + STATE_CAPTURE_WAIT_SOF,
>> + STATE_CAPTURE_IN_PROGRESS,
>> + STATE_CAPTURE_DONE,
>> + STATE_CAPTURE_ERROR,
>> +};
>> +
>> +/* Frame buffer descriptor
>> + * Used by the ISI module as a linked list for the DMA controller.
>> + */
>> +struct fbd {
>> + /* Physical address of the frame buffer */
>> + u32 fb_address;
>> +#if defined(CONFIG_ARCH_AT91SAM9G45) ||\
>> + defined(CONFIG_ARCH_AT91SAM9X5)
>> + /* DMA Control Register(only in HISI2) */
>> + u32 dma_ctrl;
>> +#endif
> I wouldn't bother with this ifdef. The memory cost is pretty
> insignificant and it makes the code easier to read without it. It also
> means you don't need to patch it up whenever a new at91 platform with
> isi dma support appears.
I will remove this #if. It is very disappointed to add some ARCH macro every time when any new platform support ISI added.
>> + /* Physical address of the next fbd */
>> + u32 next_fbd_address;
>> +};
>> +
>> +#if defined(CONFIG_ARCH_AT91SAM9G45) ||\
>> + defined(CONFIG_ARCH_AT91SAM9X5)
>> +static void set_dma_ctrl(struct fbd *fb_desc, u32 ctrl)
>> +{
>> + fb_desc->dma_ctrl = ctrl;
>> +}
>> +#else
>> +static void set_dma_ctrl(struct fbd *fb_desc, u32 ctrl) { }
>> +#endif
> Same here, get rid of the ifdefs.
I'll remove this.
>> [snip]
>> + if (!isi->still_capture) {
>> + if (pending & (ISI_BIT(V2_VSYNC))) {
>> + switch (isi->state) {
>> + case STATE_IDLE:
>> + isi->state = STATE_CAPTURE_READY;
>> + wake_up_interruptible(&isi->capture_wq);
>> + break;
>> + }
>> + } else if (likely(pending & (ISI_BIT(V2_CXFR_DONE)))) {
>> + ret = atmel_isi_handle_streaming(isi);
>> + }
>> + } else {
>> + while (pending) {
>> + if (pending & (ISI_BIT(V2_C_OVR) | ISI_BIT(V2_FR_OVR)))
>> + printk(KERN_ERR "Overrun (status = 0x%x)\n",
>> + status);
> dev_err(isi->v4l2_dev.dev, "Overrun...");
I will fix it.
>> [snip]
>> +
>> +static int atmel_isi_init(struct atmel_isi *isi)
>> +{
>> + /*
>> + * The reset will only succeed if we have a
>> + * pixel clock from the camera.
>> + */
>> + isi_writel(isi, V2_CTRL, ISI_BIT(V2_SRST));
>> + isi_writel(isi, V2_INTDIS, ~0UL);
> Don't you need to wait for the reset bit to clear? Other implementations
> I have used of the Atmel ISI driver do a wait_for_completition and have
> the interrupt handler set a reset_complete flag.
You are right. I just remove the reset_complete waiting part in this version.
Actually I am thinking whether I need add such code again.
>> [snip]
>> + if (bytes_per_line < 0)
>> + return bytes_per_line;
>> +
>> + size = bytes_per_line * icd->user_height;
>> +
>> + if (0 == *nbuffers)
> if (*nbuffers == 0)
> is the more commonly preferred style I believe.
I will fix it for consistency.
>> [snip]
>> +
>> +static int buffer_finish(struct vb2_buffer *vb)
>> +{
>> + return 0;
>> +}
> Does soc-camera support having NULL function callbacks for functions
> that are not required?
I will remove this function at all. soc-camera can use default callback.
>> [snip]
>> +static inline void stride_align(u32 *width)
>> +{
>> + if (((*width + 7) & ~7) < 4096)
>> + *width = (*width + 7) & ~7;
>> + else
>> + *width = *width & ~7;
> This looks like something that may already have a function to do it?
Yes. you are right. There is a function ALIGN(*width, 8).
And I think I can remove this function since ISI is not sensetive for this width alignment.
>> [snip]
>> +
>> +static int isi_camera_try_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;
>> + __u32 pixfmt = pix->pixelformat;
> u32 inside the kernel, __u32 is for code which is exposed to userspace
>(i.e. API headers).
I will fixed it. no __u32 anymore.
>> [snip]
>> +static int test_platform_param(struct atmel_isi *isi,
>> + unsigned char buswidth, unsigned long *flags)
>> +{
>> + /*
>> + * Platform specified synchronization and pixel clock polarities are
>> + * only a recommendation and are only used during probing. Atmel ISI
>> + * camera interface only works in master mode, i.e., uses HSYNC and
>> + * VSYNC signals from the sensor
>> + */
>> + *flags = SOCAM_MASTER |
>> + SOCAM_HSYNC_ACTIVE_HIGH |
>> + SOCAM_HSYNC_ACTIVE_LOW |
>> + SOCAM_VSYNC_ACTIVE_HIGH |
>> + SOCAM_VSYNC_ACTIVE_LOW |
>> + SOCAM_PCLK_SAMPLE_RISING |
>> + SOCAM_PCLK_SAMPLE_FALLING |
>> + SOCAM_DATA_ACTIVE_HIGH;
>> +
>> + /* If requested data width is supported by the platform, use it */
>> + switch (buswidth) {
>> + case 10:
>> + if (!(isi->platform_flags & ISI_DATAWIDTH_10))
>> + return -EINVAL;
>> + *flags |= SOCAM_DATAWIDTH_10;
>> + break;
> If you have specified the platform data width as 10 bits can you not do
> 8 bit transfers on it?
no, you cannot use 8 bit transfers.
But normally we can specify the platform data width to support both 10 and 8.
>> [snip]
>> +static int isi_camera_querycap(struct soc_camera_host *ici,
>> + struct v4l2_capability *cap)
>> +{
>> + strcpy(cap->driver, "atmel-isi");
>> + strcpy(cap->card, "Atmel Image Sensor Interface");
>> + cap->version = ATMEL_ISI_VERSION;
>> +
>> + cap->capabilities = (V4L2_CAP_VIDEO_CAPTURE
>> + | V4L2_CAP_READWRITE
>> + | V4L2_CAP_STREAMING
>> + );
> In other places you have the |'s at the end of the line. Should be
> consistent within a file.
I will fix it. And I find the same mistake in other place. Thank you.
>> + return 0;
>> +}
>> +
>> +static int isi_camera_set_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
> u32.
will be fix.
>> [snip]
>> + if (dev->platform_data)
>> + pdata = (struct isi_platform_data *) dev->platform_data;
>> + else {
>> + static struct isi_platform_data isi_default_data = {
>> + .frate = 0,
>> + .has_emb_sync = 0,
>> + .emb_crc_sync = 0,
>> + .hsync_act_low = 0,
>> + .vsync_act_low = 0,
>> + .pclk_act_falling = 0,
>> + .isi_full_mode = 1,
>> + /* to use codec and preview path simultaneously */
>> + .flags = ISI_DATAWIDTH_8 |
>> + ISI_DATAWIDTH_10,
>> + };
> Move this struct definition outside of this function.
OK. I will change that.
>> + dev_info(&pdev->dev,
>> + "No config available using default values\n");
>> + pdata = &isi_default_data;
>> + }
>> +
>> + isi->pdata = pdata;
>> + isi->platform_flags = pdata->flags;
>> + if (isi->platform_flags == 0)
>> + isi->platform_flags = ISI_DATAWIDTH_8;
> We could be mean here an require that people get the flags correct, e.g.
> if (!((isi->platform_flags & ISI_DATA_WIDTH_8) ||
> (isi->platform_flags & ISI_DATA_WIDTH_8))) {
> dev_err(&pdev->dev, "No data width specified\n");
> err = -ENOMEM;
> goto fail;
> }
> Which discourages sloppy coding in the board files.
now the isi->platform_flags only use for data width, so if it is 0, I will set is as default data width(ISI_DATA_WIDTH_8).
I think if we use platform_flags for more option (not only data width), then we need test it with a WIDTH_MASK.
>> [snip]
Best Regards,
Josh Wu
On 05/17/2011 08:59 PM, Wu, Josh wrote:
>
> On Friday, May 13, 2011 5:25 AM, Ryan Mallon wrote
>
>> On 05/12/2011 07:42 PM, Josh Wu wrote:
>>> This patch is to enable Atmel Image Sensor Interface (ISI) driver support.
>>> - Using soc-camera framework with videobuf2 dma-contig allocator
>>> - Supporting video streaming of YUV packed format
>>> - Tested on AT91SAM9M10G45-EK with OV2640
>
>> Hi Josh,
>
>> Thansk for doing this. Overall the patch looks really good. A few
>> comments below.
> Hi, Ryan
>
> Thank you for the comments.
>
>>>
>>> Signed-off-by: Josh Wu <[email protected]>
>>> ---
>>> base on branch staging/for_v2.6.40
>>>
>>> arch/arm/mach-at91/include/mach/at91_isi.h | 454 ++++++++++++
>>> drivers/media/video/Kconfig | 10 +
>>> drivers/media/video/Makefile | 1 +
>>> drivers/media/video/atmel-isi.c | 1089 ++++++++++++++++++++++++++++
>>> 4 files changed, 1554 insertions(+), 0 deletions(-)
>>> create mode 100644 arch/arm/mach-at91/include/mach/at91_isi.h
>>> create mode 100644 drivers/media/video/atmel-isi.c
>
>>> [snip]
>>> +
>>> +/* Bit manipulation macros */
>>> +#define ISI_BIT(name) \
>>> + (1 << ISI_##name##_OFFSET)
>>> +#define ISI_BF(name, value) \
>>> + (((value) & ((1 << ISI_##name##_SIZE) - 1)) \
>>> + << ISI_##name##_OFFSET)
>>> +#define ISI_BFEXT(name, value) \
>>> + (((value) >> ISI_##name##_OFFSET) \
>>> + & ((1 << ISI_##name##_SIZE) - 1))
>>> +#define ISI_BFINS(name, value, old) \
>>> + (((old) & ~(((1 << ISI_##name##_SIZE) - 1) \
>>> + << ISI_##name##_OFFSET))\
>>> + | ISI_BF(name, value))
>
>> I really dislike this kind of register access magic. Not sure how others
>> feel about it. These macros are really ugly.
> I understand this. So I will try to find a better way (static inline function) to solve this. :)
>>> +/* Register access macros */
>>> +#define isi_readl(port, reg) \
>>> + __raw_readl((port)->regs + ISI_##reg)
>>> +#define isi_writel(port, reg, value) \
>>> + __raw_writel((value), (port)->regs + ISI_##reg)
>> If the token pasting stuff gets dropped then these can be static inline
>> functions which is preferred.
> sure, I'll try.
Something like this is pretty common (should be moved into the .c file):
static inline unsigned atmel_isi_readl(struct atmel_isi *isi,
unsigned reg)
{
return readl(isi->regs + reg);
}
static inline void atmel_isi_writel(struct atmel_isi *isi,
unsigned reg, unsigned val)
{
writel(val, isi->regs + reg);
}
Then for single bit values you can just do:
#define ISI_REG_CR 0x0000
#define ISI_CR_GRAYSCALE (1 << 13)
cr = isi_readl(isi, ISI_REG_CR);
cr |= ISI_CR_GRAYSCALE;
isi_writel(isi, ISI_REG_CR, cr);
For bit-fields you could do something like:
static void atmel_isi_set_bitfield(struct atmel_isi *isi, unsigned reg,
unsigned offset, unsigned mask,
unsigned val)
{
unsigned tmp;
tmp = atmel_isi_readl(isi, reg);
tmp &= ~(mask << offset);
tmp |= (val & mask) << offset;
atmel_isi_writel(isi, reg, tmp);
}
#define ISI_V2_VCC_SWAP_OFFSET 28
#define ISI_V2_VCC_SWAP_MASK 0x3
atmel_isi_set_bitfield(isi, ISI_REG_CR, ISI_V2_VCC_SWAP_OFFSET,
ISI_V2_SWAP_MASK, 2);
There are only a handful of bit-field accesses in the driver so I don't
think this will make the driver much more verbose and it will remove a
number of _SIZE definitions for the single bit values.
<snip>
>>> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
>>> index d61414e..eae6005 100644
>>> --- a/drivers/media/video/Kconfig
>>> +++ b/drivers/media/video/Kconfig
>>> @@ -80,6 +80,16 @@ menuconfig VIDEO_CAPTURE_DRIVERS
>>> Some of those devices also supports FM radio.
>>>
>>> if VIDEO_CAPTURE_DRIVERS && VIDEO_V4L2
>>> +config VIDEO_ATMEL_ISI
>>> + tristate "ATMEL Image Sensor Interface (ISI) support"
>>> + depends on VIDEO_DEV && SOC_CAMERA
>
>> Depends on AT91/AVR32?
> I think I will use AT91
Somebody else suggested leaving out the AT91 dependency to allow better
build coverage. The reason for having the AT91 dependency is so that it
doesn't show up in menuconfig for people on other platforms and
architectures who cannot use the driver. I've always made SoC drivers
depend on their architecture. Not sure what the correct answer is here?
>>> [snip]
>>> +
>>> +#include <media/videobuf2-dma-contig.h>
>>> +#include <media/soc_camera.h>
>>> +#include <media/soc_mediabus.h>
>>> +
>>> +#define ATMEL_ISI_VERSION KERNEL_VERSION(1, 0, 0)
>
>> Do we really need this version?
> Since we need set a version for v4l2_capability.version in function isi_camera_querycap(). So we use this macro.
> How about this?
> static int isi_camera_querycap(struct soc_camera_host *ici,
> struct v4l2_capability *cap)
> {
> strcpy(cap->driver, "atmel-isi");
> strcpy(cap->card, "Atmel Image Sensor Interface");
> cap->version = KERNEL_VERSION(1, 0, 0);
> cap->capabilities = (V4L2_CAP_VIDEO_CAPTURE |
> V4L2_CAP_STREAMING);
> return 0;
> }
Not sure. Is cap->version meant to be a KERNEL_VERSION value or is it
arbitrary? If nothing from user-space cares then it could just be left
as zero?
<snip>
>>> +static int atmel_isi_init(struct atmel_isi *isi)
>>> +{
>>> + /*
>>> + * The reset will only succeed if we have a
>>> + * pixel clock from the camera.
>>> + */
>>> + isi_writel(isi, V2_CTRL, ISI_BIT(V2_SRST));
>>> + isi_writel(isi, V2_INTDIS, ~0UL);
>
>> Don't you need to wait for the reset bit to clear? Other implementations
>> I have used of the Atmel ISI driver do a wait_for_completition and have
>> the interrupt handler set a reset_complete flag.
> You are right. I just remove the reset_complete waiting part in this version.
> Actually I am thinking whether I need add such code again.
The driver should do the check since the reset can fail, and the caller
should be notified. This function should probably be called
atmel_isi_reset rather than atmel_isi_init to better reflect what it does.
~Ryan
--
Bluewater Systems Ltd - ARM Technology Solution Centre
Ryan Mallon 5 Amuri Park, 404 Barbadoes St
[email protected] PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com New Zealand
Phone: +64 3 3779127 Freecall: Australia 1800 148 751
Fax: +64 3 3779135 USA 1800 261 2934
On 08:58 Wed 18 May , Ryan Mallon wrote:
> On 05/17/2011 08:59 PM, Wu, Josh wrote:
> >
> > On Friday, May 13, 2011 5:25 AM, Ryan Mallon wrote
> >
> >> On 05/12/2011 07:42 PM, Josh Wu wrote:
> >>> This patch is to enable Atmel Image Sensor Interface (ISI) driver support.
> >>> - Using soc-camera framework with videobuf2 dma-contig allocator
> >>> - Supporting video streaming of YUV packed format
> >>> - Tested on AT91SAM9M10G45-EK with OV2640
> >
> >> Hi Josh,
> >
> >> Thansk for doing this. Overall the patch looks really good. A few
> >> comments below.
> > Hi, Ryan
> >
> > Thank you for the comments.
> >
> >>>
> >>> Signed-off-by: Josh Wu <[email protected]>
> >>> ---
> >>> base on branch staging/for_v2.6.40
> >>>
> >>> arch/arm/mach-at91/include/mach/at91_isi.h | 454 ++++++++++++
> >>> drivers/media/video/Kconfig | 10 +
> >>> drivers/media/video/Makefile | 1 +
> >>> drivers/media/video/atmel-isi.c | 1089 ++++++++++++++++++++++++++++
> >>> 4 files changed, 1554 insertions(+), 0 deletions(-)
> >>> create mode 100644 arch/arm/mach-at91/include/mach/at91_isi.h
> >>> create mode 100644 drivers/media/video/atmel-isi.c
> >
> >>> [snip]
> >>> +
> >>> +/* Bit manipulation macros */
> >>> +#define ISI_BIT(name) \
> >>> + (1 << ISI_##name##_OFFSET)
> >>> +#define ISI_BF(name, value) \
> >>> + (((value) & ((1 << ISI_##name##_SIZE) - 1)) \
> >>> + << ISI_##name##_OFFSET)
> >>> +#define ISI_BFEXT(name, value) \
> >>> + (((value) >> ISI_##name##_OFFSET) \
> >>> + & ((1 << ISI_##name##_SIZE) - 1))
> >>> +#define ISI_BFINS(name, value, old) \
> >>> + (((old) & ~(((1 << ISI_##name##_SIZE) - 1) \
> >>> + << ISI_##name##_OFFSET))\
> >>> + | ISI_BF(name, value))
> >
> >> I really dislike this kind of register access magic. Not sure how others
> >> feel about it. These macros are really ugly.
> > I understand this. So I will try to find a better way (static inline function) to solve this. :)
>
> >>> +/* Register access macros */
> >>> +#define isi_readl(port, reg) \
> >>> + __raw_readl((port)->regs + ISI_##reg)
> >>> +#define isi_writel(port, reg, value) \
> >>> + __raw_writel((value), (port)->regs + ISI_##reg)
>
> >> If the token pasting stuff gets dropped then these can be static inline
> >> functions which is preferred.
> > sure, I'll try.
>
> Something like this is pretty common (should be moved into the .c file):
>
> static inline unsigned atmel_isi_readl(struct atmel_isi *isi,
> unsigned reg)
> {
> return readl(isi->regs + reg);
> }
>
> static inline void atmel_isi_writel(struct atmel_isi *isi,
> unsigned reg, unsigned val)
> {
> writel(val, isi->regs + reg);
> }
really do not like it
and prefer the first implemetation
NACK for me
>
> Then for single bit values you can just do:
>
> #define ISI_REG_CR 0x0000
> #define ISI_CR_GRAYSCALE (1 << 13)
>
> cr = isi_readl(isi, ISI_REG_CR);
> cr |= ISI_CR_GRAYSCALE;
> isi_writel(isi, ISI_REG_CR, cr);
>
> For bit-fields you could do something like:
>
> static void atmel_isi_set_bitfield(struct atmel_isi *isi, unsigned reg,
> unsigned offset, unsigned mask,
> unsigned val)
> {
> unsigned tmp;
>
> tmp = atmel_isi_readl(isi, reg);
> tmp &= ~(mask << offset);
> tmp |= (val & mask) << offset;
> atmel_isi_writel(isi, reg, tmp);
> }
>
stop to reinvent thinks
use the bitops of the kernel
> #define ISI_V2_VCC_SWAP_OFFSET 28
> #define ISI_V2_VCC_SWAP_MASK 0x3
>
> atmel_isi_set_bitfield(isi, ISI_REG_CR, ISI_V2_VCC_SWAP_OFFSET,
> ISI_V2_SWAP_MASK, 2);
>
> There are only a handful of bit-field accesses in the driver so I don't
> think this will make the driver much more verbose and it will remove a
> number of _SIZE definitions for the single bit values.
>
> <snip>
>
> >>> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
> >>> index d61414e..eae6005 100644
> >>> --- a/drivers/media/video/Kconfig
> >>> +++ b/drivers/media/video/Kconfig
> >>> @@ -80,6 +80,16 @@ menuconfig VIDEO_CAPTURE_DRIVERS
> >>> Some of those devices also supports FM radio.
> >>>
> >>> if VIDEO_CAPTURE_DRIVERS && VIDEO_V4L2
> >>> +config VIDEO_ATMEL_ISI
> >>> + tristate "ATMEL Image Sensor Interface (ISI) support"
> >>> + depends on VIDEO_DEV && SOC_CAMERA
> >
> >> Depends on AT91/AVR32?
> > I think I will use AT91
>
> Somebody else suggested leaving out the AT91 dependency to allow better
> build coverage. The reason for having the AT91 dependency is so that it
> doesn't show up in menuconfig for people on other platforms and
> architectures who cannot use the driver. I've always made SoC drivers
> depend on their architecture. Not sure what the correct answer is here?
no if the drivers is soc specific we MUST not enabled it on other soc
and avoid maintainancne issue
Best Regards,
J.
Hi, Guennadi
Sorry to answer the question so later,
From: Guennadi Liakhovetski Sent: Thursday, May 12, 2011 5:32 PM
> On Thu, 12 May 2011, Wu, Josh wrote:
>> Hi, Russell
>>
>> From: Russell King - ARM Linux [mailto:[email protected]] Sent: Thursday, May 12, 2011 3:47 PM
>> > On Thu, May 12, 2011 at 03:42:18PM +0800, Josh Wu wrote:
>> >> +err_alloc_isi:
>> >> + clk_disable(pclk);
>> > clk_put() ?
>> Ok, will be fixed in V2 patch. Thanks.
> You might wait with v2 until I find time to review your patch. Will take a
> couple of days, I think. A general question, though: how compatible is
> this driver with the AVR32?
Now from the information I got from AVR team, The AVR32 is not compatible with ISI driver patch v2. Since AVR32 use the previous version of ISI IP core. (AT91SAM9M10 using ISI_V2 IP, which has different hardware registers from V1 IP core, such like dma control, etc)
But I think in future we will add ISI V1 IP support since AT91SAM9263 also using ISI V1 IP core.
Best Regards,
Josh Wu