On Wed, May 12, 2010 at 09:02:29PM +0200, Guennadi Liakhovetski wrote:
> Hi Baruch
>
> Thanks for eventually mainlining this driver! A couple of comments below.
> Sascha, would be great, if you could get it tested on imx27 with and
> without emma.
I will see what I can do. Testing and probably breathing life into a
camera driver usually takes me two days given that the platform support
is very outdated. I hope our customer is interested in this, then it
would be possible to test it.
> BTW, if you say, that you use emma to avoid using the
> standard DMA controller, why would anyone want not to use emma? Resource
> conflict? There is also a question for you down in the comments, please,
> skim over.
I originally did not know how all the components should work together.
Now I think it's the right way to use the EMMA to be able to scale
images and to do colour conversions (which does not work with our Bayer
format cameras, so I cannot test it).
>
> On Thu, 6 May 2010, Baruch Siach wrote:
>
> > This is the soc_camera support developed by Sascha Hauer for the i.MX27. Alan
> > Carvalho de Assis modified the original driver to get it working on more recent
> > kernels. I modified it further to add support for i.MX25. This driver has only
> > been tested on the i.MX25 platform.
> >
> > Signed-off-by: Baruch Siach <baruch@xxxxxxxxxx>
> > ---
> > arch/arm/plat-mxc/include/mach/memory.h | 4 +-
> > arch/arm/plat-mxc/include/mach/mx2_cam.h | 41 +
> > drivers/media/video/Kconfig | 14 +
> > drivers/media/video/Makefile | 1 +
> > drivers/media/video/mx2_camera.c | 1396 ++++++++++++++++++++++++++++++
> > 5 files changed, 1454 insertions(+), 2 deletions(-)
> > create mode 100644 arch/arm/plat-mxc/include/mach/mx2_cam.h
> > create mode 100644 drivers/media/video/mx2_camera.c
> >
> > diff --git a/arch/arm/plat-mxc/include/mach/memory.h b/arch/arm/plat-mxc/include/mach/memory.h
> > index c4b40c3..5803836 100644
> > --- a/arch/arm/plat-mxc/include/mach/memory.h
> > +++ b/arch/arm/plat-mxc/include/mach/memory.h
> > @@ -44,12 +44,12 @@
> > */
> > #define CONSISTENT_DMA_SIZE SZ_8M
> >
> > -#elif defined(CONFIG_MX1_VIDEO)
> > +#elif defined(CONFIG_MX1_VIDEO) || defined(CONFIG_MX2_VIDEO)
> > /*
> > * Increase size of DMA-consistent memory region.
> > * This is required for i.MX camera driver to capture at least four VGA frames.
> > */
> > #define CONSISTENT_DMA_SIZE SZ_4M
> > -#endif /* CONFIG_MX1_VIDEO */
> > +#endif /* CONFIG_MX1_VIDEO || CONFIG_MX2_VIDEO */
> >
> > #endif /* __ASM_ARCH_MXC_MEMORY_H__ */
> > diff --git a/arch/arm/plat-mxc/include/mach/mx2_cam.h b/arch/arm/plat-mxc/include/mach/mx2_cam.h
> > new file mode 100644
> > index 0000000..01b3bdc
> > --- /dev/null
> > +++ b/arch/arm/plat-mxc/include/mach/mx2_cam.h
> > @@ -0,0 +1,41 @@
> > +/*
> > + mx2-cam.h - i.MX27/i.MX25 camera driver header file
> > +
> > + Copyright (C) 2003, Intel Corporation
> > + Copyright (C) 2008, Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> > + Copyright (C) 2010, Baruch Siach <baruch@xxxxxxxxxx>
> > +
> > + This program is free software; you can redistribute it and/or modify
> > + it under the terms of the GNU General Public License as published by
> > + the Free Software Foundation; either version 2 of the License, or
> > + (at your option) any later version.
> > +
> > + This program is distributed in the hope that it will be useful,
> > + but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + GNU General Public License for more details.
> > +
> > + You should have received a copy of the GNU General Public License
> > + along with this program; if not, write to the Free Software
> > + Foundation, Inc., 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
> > +*/
>
> Please, follow the kernel multiline comment style:
>
> /*
> * this is a
> * multiline comment
> */
>
> > +
> > +#ifndef __MACH_MX2_CAM_H_
> > +#define __MACH_MX2_CAM_H_
> > +
> > +#define MX2_CAMERA_SWAP16 (1 << 0)
> > +#define MX2_CAMERA_EXT_VSYNC (1 << 1)
> > +#define MX2_CAMERA_CCIR (1 << 2)
> > +#define MX2_CAMERA_CCIR_INTERLACE (1 << 3)
> > +#define MX2_CAMERA_HSYNC_HIGH (1 << 4)
> > +#define MX2_CAMERA_GATED_CLOCK (1 << 5)
> > +#define MX2_CAMERA_INV_DATA (1 << 6)
> > +#define MX2_CAMERA_PCLK_SAMPLE_RISING (1 << 7)
> > +#define MX2_CAMERA_PACK_DIR_MSB (1 << 8)
> > +
> > +struct mx2_camera_platform_data {
> > + unsigned long flags;
> > + unsigned long clk;
> > +};
>
> Since this is an exported API, please, document this struct, at least
> "unsigned long clk" is not self-explanatory, IMHO.
>
> > +
> > +#endif /* __MACH_MX2_CAM_H_ */
> > diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
> > index f8fc865..4e230b8 100644
> > --- a/drivers/media/video/Kconfig
> > +++ b/drivers/media/video/Kconfig
> > @@ -949,6 +949,20 @@ config VIDEO_OMAP2
> > ---help---
> > This is a v4l2 driver for the TI OMAP2 camera capture interface
> >
> > +config MX2_VIDEO
> > + bool
> > +
> > +config VIDEO_MX2
> > + tristate "i.MX27/i.MX25 Camera Sensor Interface driver"
> > + depends on VIDEO_DEV && (MACH_MX27 || ARCH_MX25)
> > + select SOC_CAMERA
>
> Please follow other drivers and add SOC_CAMERA to "depends on"
>
> > + select VIDEOBUF_DMA_CONTIG
> > + select MX2_VIDEO
> > + ---help---
> > + This is a v4l2 driver for the i.MX27 and the i.MX25 Camera Sensor
> > + Interface
> > +
> > +
> > #
> > # USB Multimedia device configuration
> > #
> > diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
> > index b88b617..177af1a 100644
> > --- a/drivers/media/video/Makefile
> > +++ b/drivers/media/video/Makefile
> > @@ -156,6 +156,7 @@ obj-$(CONFIG_SOC_CAMERA) += soc_camera.o soc_mediabus.o
> > obj-$(CONFIG_SOC_CAMERA_PLATFORM) += soc_camera_platform.o
> > # soc-camera host drivers have to be linked after camera drivers
> > obj-$(CONFIG_VIDEO_MX1) += mx1_camera.o
> > +obj-$(CONFIG_VIDEO_MX2) += mx2_camera.o
> > obj-$(CONFIG_VIDEO_MX3) += mx3_camera.o
> > obj-$(CONFIG_VIDEO_PXA27x) += pxa_camera.o
> > obj-$(CONFIG_VIDEO_SH_MOBILE_CEU) += sh_mobile_ceu_camera.o
> > diff --git a/drivers/media/video/mx2_camera.c b/drivers/media/video/mx2_camera.c
> > new file mode 100644
> > index 0000000..5d6fb08
> > --- /dev/null
> > +++ b/drivers/media/video/mx2_camera.c
> > @@ -0,0 +1,1396 @@
> > +/*
> > + * V4L2 Driver for i.MX27/i.MX25 camera host
> > + *
> > + * Copyright (C) 2008, Sascha Hauer, Pengutronix
>
> Baruch, depending on how you estimate your effort on porting Sascha's
> driver, you might want to add your copyright here too.
>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + */
> > +
> > +#include <linux/init.h>
> > +#include <linux/module.h>
> > +#include <linux/io.h>
> > +#include <linux/delay.h>
> > +#include <linux/slab.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/errno.h>
> > +#include <linux/fs.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mm.h>
> > +#include <linux/moduleparam.h>
> > +#include <linux/time.h>
> > +#include <linux/version.h>
> > +#include <linux/device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/mutex.h>
> > +#include <linux/clk.h>
> > +
> > +#include <media/v4l2-common.h>
> > +#include <media/v4l2-dev.h>
> > +#include <media/videobuf-dma-contig.h>
> > +#include <media/soc_camera.h>
> > +#include <media/soc_mediabus.h>
> > +
> > +#include <linux/videodev2.h>
> > +
> > +#include <mach/mx2_cam.h>
> > +#include <asm/dma.h>
>
> This is duplicated below, please, remove this copy.
>
> > +#include <mach/dma-mx1-mx2.h>
> > +#include <mach/hardware.h>
> > +
> > +#include <asm/dma.h>
> > +
> > +#define MX2_CAM_DRV_NAME "mx2-camera"
> > +#define MX2_CAM_VERSION_CODE KERNEL_VERSION(0, 0, 5) /* FIXME: Whats this? */
>
> Well, I think, you can remove this "FIXME"... This is just some arbitrary
> driver version, that noone ever remembers to update;) or am I wrong?
>
> > +#define MX2_CAM_DRIVER_DESCRIPTION "i.MX2x_Camera"
> > +
> > +/* reset values */
> > +#define CSICR1_RESET_VAL 0x40000800
> > +#define CSICR2_RESET_VAL 0x0
> > +#define CSICR3_RESET_VAL 0x0
> > +
> > +/* csi control reg 1 */
> > +#define CSICR1_SWAP16_EN (1 << 31)
> > +#define CSICR1_EXT_VSYNC (1 << 30)
> > +#define CSICR1_EOF_INTEN (1 << 29)
> > +#define CSICR1_PRP_IF_EN (1 << 28)
> > +#define CSICR1_CCIR_MODE (1 << 27)
> > +#define CSICR1_COF_INTEN (1 << 26)
> > +#define CSICR1_SF_OR_INTEN (1 << 25)
> > +#define CSICR1_RF_OR_INTEN (1 << 24)
> > +#define CSICR1_STATFF_LEVEL (3 << 22)
> > +#define CSICR1_STATFF_INTEN (1 << 21)
> > +#define CSICR1_RXFF_LEVEL(l) (((l) & 3) << 19) /* MX27 */
> > +#define CSICR1_FB2_DMA_INTEN (1 << 20) /* MX25 */
> > +#define CSICR1_FB1_DMA_INTEN (1 << 19) /* MX25 */
> > +#define CSICR1_RXFF_INTEN (1 << 18)
> > +#define CSICR1_SOF_POL (1 << 17)
> > +#define CSICR1_SOF_INTEN (1 << 16)
> > +#define CSICR1_MCLKDIV(d) (((d) & 0xF) << 12)
> > +#define CSICR1_HSYNC_POL (1 << 11)
> > +#define CSICR1_CCIR_EN (1 << 10)
> > +#define CSICR1_MCLKEN (1 << 9)
> > +#define CSICR1_FCC (1 << 8)
> > +#define CSICR1_PACK_DIR (1 << 7)
> > +#define CSICR1_CLR_STATFIFO (1 << 6)
> > +#define CSICR1_CLR_RXFIFO (1 << 5)
> > +#define CSICR1_GCLK_MODE (1 << 4)
> > +#define CSICR1_INV_DATA (1 << 3)
> > +#define CSICR1_INV_PCLK (1 << 2)
> > +#define CSICR1_REDGE (1 << 1)
> > +
> > +#define SHIFT_STATFF_LEVEL 22
> > +#define SHIFT_RXFF_LEVEL 19
> > +#define SHIFT_MCLKDIV 12
> > +
> > +/* control reg 3 */
> > +#define CSICR3_FRMCNT (0xFFFF << 16)
> > +#define CSICR3_FRMCNT_RST (1 << 15)
> > +#define CSICR3_DMA_REFLASH_RFF (1 << 14)
> > +#define CSICR3_DMA_REFLASH_SFF (1 << 13)
> > +#define CSICR3_DMA_REQ_EN_RFF (1 << 12)
> > +#define CSICR3_DMA_REQ_EN_SFF (1 << 11)
> > +#define CSICR3_RXFF_LEVEL(l) (((l) & 7) << 4) /* MX25 */
> > +#define CSICR3_CSI_SUP (1 << 3)
> > +#define CSICR3_ZERO_PACK_EN (1 << 2)
> > +#define CSICR3_ECC_INT_EN (1 << 1)
> > +#define CSICR3_ECC_AUTO_EN (1 << 0)
> > +
> > +#define SHIFT_FRMCNT 16
> > +
> > +/* csi status reg */
> > +#define CSISR_SFF_OR_INT (1 << 25)
> > +#define CSISR_RFF_OR_INT (1 << 24)
> > +#define CSISR_STATFF_INT (1 << 21)
> > +#define CSISR_DMA_TSF_FB2_INT (1 << 20) /* MX25 */
> > +#define CSISR_DMA_TSF_FB1_INT (1 << 19) /* MX25 */
> > +#define CSISR_RXFF_INT (1 << 18)
> > +#define CSISR_EOF_INT (1 << 17)
> > +#define CSISR_SOF_INT (1 << 16)
> > +#define CSISR_F2_INT (1 << 15)
> > +#define CSISR_F1_INT (1 << 14)
> > +#define CSISR_COF_INT (1 << 13)
> > +#define CSISR_ECC_INT (1 << 1)
> > +#define CSISR_DRDY (1 << 0)
> > +
> > +#define CSICR1 0x00
> > +#define CSICR2 0x04
> > +#define CSISR (cpu_is_mx27() ? 0x08 : 0x18)
> > +#define CSISTATFIFO 0x0c
> > +#define CSIRFIFO 0x10
> > +#define CSIRXCNT 0x14
> > +#define CSICR3 (cpu_is_mx27() ? 0x1C : 0x08)
> > +#define CSIDMASA_STATFIFO 0x20
> > +#define CSIDMATA_STATFIFO 0x24
> > +#define CSIDMASA_FB1 0x28
> > +#define CSIDMASA_FB2 0x2c
> > +#define CSIFBUF_PARA 0x30
> > +#define CSIIMAG_PARA 0x34
> > +
> > +/* EMMA PrP */
> > +#define PRP_CNTL 0x00
> > +#define PRP_INTR_CNTL 0x04
> > +#define PRP_INTRSTATUS 0x08
> > +#define PRP_SOURCE_Y_PTR 0x0c
> > +#define PRP_SOURCE_CB_PTR 0x10
> > +#define PRP_SOURCE_CR_PTR 0x14
> > +#define PRP_DEST_RGB1_PTR 0x18
> > +#define PRP_DEST_RGB2_PTR 0x1c
> > +#define PRP_DEST_Y_PTR 0x20
> > +#define PRP_DEST_CB_PTR 0x24
> > +#define PRP_DEST_CR_PTR 0x28
> > +#define PRP_SRC_FRAME_SIZE 0x2c
> > +#define PRP_DEST_CH1_LINE_STRIDE 0x30
> > +#define PRP_SRC_PIXEL_FORMAT_CNTL 0x34
> > +#define PRP_CH1_PIXEL_FORMAT_CNTL 0x38
> > +#define PRP_CH1_OUT_IMAGE_SIZE 0x3c
> > +#define PRP_CH2_OUT_IMAGE_SIZE 0x40
> > +#define PRP_SRC_LINE_STRIDE 0x44
> > +#define PRP_CSC_COEF_012 0x48
> > +#define PRP_CSC_COEF_345 0x4c
> > +#define PRP_CSC_COEF_678 0x50
> > +#define PRP_CH1_RZ_HORI_COEF1 0x54
> > +#define PRP_CH1_RZ_HORI_COEF2 0x58
> > +#define PRP_CH1_RZ_HORI_VALID 0x5c
> > +#define PRP_CH1_RZ_VERT_COEF1 0x60
> > +#define PRP_CH1_RZ_VERT_COEF2 0x64
> > +#define PRP_CH1_RZ_VERT_VALID 0x68
> > +#define PRP_CH2_RZ_HORI_COEF1 0x6c
> > +#define PRP_CH2_RZ_HORI_COEF2 0x70
> > +#define PRP_CH2_RZ_HORI_VALID 0x74
> > +#define PRP_CH2_RZ_VERT_COEF1 0x78
> > +#define PRP_CH2_RZ_VERT_COEF2 0x7c
> > +#define PRP_CH2_RZ_VERT_VALID 0x80
> > +
> > +#define PRP_CNTL_CH1EN (1 << 0)
> > +#define PRP_CNTL_CH2EN (1 << 1)
> > +#define PRP_CNTL_CSIEN (1 << 2)
> > +#define PRP_CNTL_DATA_IN_YUV420 (0 << 3)
> > +#define PRP_CNTL_DATA_IN_YUV422 (1 << 3)
> > +#define PRP_CNTL_DATA_IN_RGB16 (2 << 3)
> > +#define PRP_CNTL_DATA_IN_RGB32 (3 << 3)
> > +#define PRP_CNTL_CH1_OUT_RGB8 (0 << 5)
> > +#define PRP_CNTL_CH1_OUT_RGB16 (1 << 5)
> > +#define PRP_CNTL_CH1_OUT_RGB32 (2 << 5)
> > +#define PRP_CNTL_CH1_OUT_YUV422 (3 << 5)
> > +#define PRP_CNTL_CH2_OUT_YUV420 (0 << 7)
> > +#define PRP_CNTL_CH2_OUT_YUV422 (1 << 7)
> > +#define PRP_CNTL_CH2_OUT_YUV444 (2 << 7)
> > +#define PRP_CNTL_CH1_LEN (1 << 9)
> > +#define PRP_CNTL_CH2_LEN (1 << 10)
> > +#define PRP_CNTL_SKIP_FRAME (1 << 11)
> > +#define PRP_CNTL_SWRST (1 << 12)
> > +#define PRP_CNTL_CLKEN (1 << 13)
> > +#define PRP_CNTL_WEN (1 << 14)
> > +#define PRP_CNTL_CH1BYP (1 << 15)
> > +#define PRP_CNTL_IN_TSKIP(x) ((x) << 16)
> > +#define PRP_CNTL_CH1_TSKIP(x) ((x) << 19)
> > +#define PRP_CNTL_CH2_TSKIP(x) ((x) << 22)
> > +#define PRP_CNTL_INPUT_FIFO_LEVEL(x) ((x) << 25)
> > +#define PRP_CNTL_RZ_FIFO_LEVEL(x) ((x) << 27)
> > +#define PRP_CNTL_CH2B1EN (1 << 29)
> > +#define PRP_CNTL_CH2B2EN (1 << 30)
> > +#define PRP_CNTL_CH2FEN (1 << 31)
> > +
> > +/* IRQ Enable and status register */
> > +#define PRP_INTR_RDERR (1 << 0)
> > +#define PRP_INTR_CH1WERR (1 << 1)
> > +#define PRP_INTR_CH2WERR (1 << 2)
> > +#define PRP_INTR_CH1FC (1 << 3)
> > +#define PRP_INTR_CH2FC (1 << 5)
> > +#define PRP_INTR_LBOVF (1 << 7)
> > +#define PRP_INTR_CH2OVF (1 << 8)
> > +
> > +#define mx27_camera_emma(pcdev) (cpu_is_mx27() && pcdev->use_emma)
> > +
> > +#define MAX_VIDEO_MEM 16
> > +
> > +struct mx2_camera_dev {
> > + struct device *dev;
> > + struct soc_camera_host soc_host;
> > + struct soc_camera_device *icd;
> > + struct clk *clk_csi, *clk_emma;
> > +
> > + unsigned int irq_csi, irq_emma;
> > + void __iomem *base_csi, *base_emma;
> > +
> > + struct mx2_camera_platform_data *pdata;
> > + struct resource *res_csi, *res_emma;
> > + unsigned long platform_flags;
> > +
> > + struct list_head capture;
> > + struct list_head active_bufs;
> > +
> > + spinlock_t lock;
> > +
> > + int dma;
> > + struct mx2_buffer *active;
> > + struct mx2_buffer *fb1_active;
> > + struct mx2_buffer *fb2_active;
> > +
> > + int use_emma;
> > +
> > + unsigned int csicr1;
>
> This is a hardware register, u32 (also see comment below).
>
> > +
> > + void __iomem *discard_buffer;
> > + dma_addr_t discard_buffer_dma;
> > + size_t discard_size;
> > +};
> > +
> > +/* buffer for one video frame */
> > +struct mx2_buffer {
> > + /* common v4l buffer stuff -- must be first */
> > + struct videobuf_buffer vb;
> > +
> > + enum v4l2_mbus_pixelcode code;
> > +
> > + int bufnum;
> > +};
> > +
> > +static int mclk_get_divisor(struct mx2_camera_dev *pcdev)
> > +{
> > + dev_info(pcdev->dev, "%s not implemented. Running at max speed\n",
> > + __func__);
>
> Hm, why is this unimplemented?
>
> > +
> > +#if 0
> > + unsigned int mclk = pcdev->pdata->clk_csi;
> > + unsigned int pclk = clk_get_rate(pcdev->clk_csi);
> > + int i;
> > +
> > + dev_dbg(pcdev->dev, "%s: %ld %ld\n", __func__, mclk, pclk);
> > +
> > + for (i = 0; i < 0xf; i++)
> > + if ((i + 1) * 2 * mclk <= pclk)
> > + break;
>
> This doesn't look right. You increment the counter i, and terminate the
> loop as soon as "(i + 1) * 2 * mclk <= pclk". Obviously, if 2 * mclk <= pclk,
> this will terminate immediately, otherwise it will run until the end and
> return 0xf without satisfying the condition. What exactly are you trying to
> achieve? Find the _largest_ i, such that "(i + 1) * 2 * mclk <= pclk"? Then
> why not just do "i = pclk / 2 / mclk - 1"?
>
> > + return i;
> > +#endif
> > + return 0;
> > +}
> > +
> > +static void mx2_camera_deactivate(struct mx2_camera_dev *pcdev)
> > +{
> > + clk_disable(pcdev->clk_csi);
> > + writel(0, pcdev->base_csi + CSICR1);
> > + if (mx27_camera_emma(pcdev))
> > + writel(0, pcdev->base_emma + PRP_CNTL);
> > +}
> > +
> > +/* The following two functions absolutely depend on the fact, that
> > + * there can be only one camera on mx2 camera sensor interface */
>
> Multiline comment
>
> > +static int mx2_camera_add_device(struct soc_camera_device *icd)
> > +{
> > + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> > + struct mx2_camera_dev *pcdev = ici->priv;
> > + int ret;
> > + u32 csicr1;
> > +
> > + if (pcdev->icd)
> > + return -EBUSY;
> > +
> > + ret = clk_enable(pcdev->clk_csi);
> > + if (ret < 0)
> > + return ret;
> > +
> > + csicr1 = CSICR1_MCLKDIV(mclk_get_divisor(pcdev)) |
> > + CSICR1_MCLKEN;
> > +
> > + if (mx27_camera_emma(pcdev)) {
> > + csicr1 |= CSICR1_PRP_IF_EN | CSICR1_FCC |
> > + CSICR1_RXFF_LEVEL(0);
> > + } else if (cpu_is_mx27())
> > + csicr1 |= CSICR1_SOF_INTEN | CSICR1_RXFF_LEVEL(2);
> > +
> > + pcdev->csicr1 = csicr1;
> > + writel(pcdev->csicr1, pcdev->base_csi + CSICR1);
> > +
> > + pcdev->icd = icd;
> > +
> > + dev_info(icd->dev.parent, "Camera driver attached to camera %d\n",
> > + icd->devnum);
> > +
> > + return 0;
> > +}
> > +
> > +static void mx2_camera_remove_device(struct soc_camera_device *icd)
> > +{
> > + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> > + struct mx2_camera_dev *pcdev = ici->priv;
> > +
> > + BUG_ON(icd != pcdev->icd);
> > +
> > + dev_info(icd->dev.parent, "Camera driver detached from camera %d\n",
> > + icd->devnum);
> > +
> > + mx2_camera_deactivate(pcdev);
> > +
> > + if (pcdev->discard_buffer) {
> > + dma_free_coherent(NULL, pcdev->discard_size,
> > + pcdev->discard_buffer,
> > + pcdev->discard_buffer_dma);
>
> use ici->v4l2_dev.dev for device?
>
> > + }
> > + pcdev->discard_buffer = 0;
>
> discard_buffer is a pointer, so, " = NULL".
>
> > +
> > + pcdev->icd = NULL;
> > +}
> > +
> > +static void mx27_camera_dma_enable(struct mx2_camera_dev *pcdev)
> > +{
> > + u32 tmp;
> > +
> > + imx_dma_enable(pcdev->dma);
> > +
> > + tmp = readl(pcdev->base_csi + CSICR1);
> > + tmp |= CSICR1_RF_OR_INTEN;
> > + writel(tmp, pcdev->base_csi + CSICR1);
> > +}
> > +
> > +static irqreturn_t mx27_camera_irq(int irq_csi, void *data)
> > +{
> > + struct mx2_camera_dev *pcdev = data;
> > + u32 status = readl(pcdev->base_csi + CSISR);
> > +
> > + if (status & CSISR_SOF_INT && pcdev->active) {
> > + u32 tmp;
> > +
> > + tmp = readl(pcdev->base_csi + CSICR1);
> > + tmp |= CSICR1_CLR_RXFIFO;
> > + writel(tmp, pcdev->base_csi + CSICR1);
>
> To me this always looks like that extra assignment to "tmp" must have some
> meaning, whereas it doesn't. So, I find something like
>
> + tmp = readl(pcdev->base_csi + CSICR1) |
> + CSICR1_CLR_RXFIFO;
> + writel(tmp, pcdev->base_csi + CSICR1);
>
> or
>
> + tmp = readl(pcdev->base_csi + CSICR1);
> + writel(tmp | CSICR1_CLR_RXFIFO, pcdev->base_csi + CSICR1);
>
> nicer.
I don't. I really like the three step read-modify-write better. The two
line style may be simple enough to read for a writel(tmp | VALUE, reg) case,
but I really find something like writel((tmp &= ~MASK) | VALUE, reg)
horrible to read, especially for long register and bit defines.
Therefore I always use the three line style.
>
> > + mx27_camera_dma_enable(pcdev);
> > + }
> > +
> > + writel(CSISR_SOF_INT | CSISR_RFF_OR_INT, pcdev->base_csi + CSISR);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static void mx25_camera_frame_done(struct mx2_camera_dev *pcdev, int fb,
> > + int state)
> > +{
> > + struct videobuf_buffer *vb;
> > + struct mx2_buffer *buf;
> > + struct mx2_buffer **fb_active = fb == 1 ? &pcdev->fb1_active :
> > + &pcdev->fb2_active;
> > + u32 fb_reg = fb == 1 ? CSIDMASA_FB1 : CSIDMASA_FB2;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&pcdev->lock, flags);
> > +
> > + if ((*fb_active) == NULL)
>
> Superfluous parenthesis. Maybe even just
>
> + if (!*fb_active)
>
> > + goto out;
> > + vb = &(*fb_active)->vb;
> > + dev_dbg(pcdev->dev, "%s (vb=0x%p) 0x%08lx %d\n", __func__,
> > + vb, vb->baddr, vb->bsize);
> > +
> > + vb->state = state;
> > + do_gettimeofday(&vb->ts);
> > + vb->field_count++;
> > +
> > + wake_up(&vb->done);
> > +
> > + if (list_empty(&pcdev->capture)) {
> > + buf = NULL;
> > + writel(0, pcdev->base_csi + fb_reg);
> > + } else {
> > + buf = list_entry(pcdev->capture.next, struct mx2_buffer,
> > + vb.queue);
> > + vb = &buf->vb;
> > + list_del(&vb->queue);
> > + vb->state = VIDEOBUF_ACTIVE;
> > + writel(videobuf_to_dma_contig(vb), pcdev->base_csi + fb_reg);
> > + }
> > +
> > + *fb_active = buf;
> > +
> > +out:
> > + spin_unlock_irqrestore(&pcdev->lock, flags);
> > +}
> > +
> > +static irqreturn_t mx25_camera_irq(int irq_csi, void *data)
> > +{
> > + struct mx2_camera_dev *pcdev = data;
> > + u32 status = readl(pcdev->base_csi + CSISR);
> > +
> > + if (status & CSISR_DMA_TSF_FB1_INT)
> > + mx25_camera_frame_done(pcdev, 1, VIDEOBUF_DONE);
> > + else if (status & CSISR_DMA_TSF_FB2_INT)
> > + mx25_camera_frame_done(pcdev, 2, VIDEOBUF_DONE);
> > +
> > + /* FIXME: handle CSISR_RFF_OR_INT */
> > +
> > + writel(status, pcdev->base_csi + CSISR);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +/*
> > + * Videobuf operations
> > + */
> > +static int mx2_videobuf_setup(struct videobuf_queue *vq, unsigned int *count,
> > + unsigned int *size)
> > +{
> > + struct soc_camera_device *icd = vq->priv_data;
> > + int bytes_per_line = soc_mbus_bytes_per_line(icd->user_width,
> > + icd->current_fmt->host_fmt);
> > +
> > + dev_dbg(&icd->dev, "count=%d, size=%d\n", *count, *size);
> > +
> > + if (bytes_per_line < 0)
> > + return bytes_per_line;
> > +
> > + *size = bytes_per_line * icd->user_height;
> > +
> > + if (0 == *count)
> > + *count = 32;
> > + while (*size * *count > MAX_VIDEO_MEM * 1024 * 1024)
> > + (*count)--;
>
> Have a look at this:
>
> http://git.linuxtv.org/v4l-dvb.git?a=commitdiff;h=f7a6936428c11b8bd33e7c438236142fd20cbf8b
>
> > +
> > + return 0;
> > +}
> > +
> > +static void free_buffer(struct videobuf_queue *vq, struct mx2_buffer *buf)
> > +{
> > + struct soc_camera_device *icd = vq->priv_data;
> > +
> > + BUG_ON(in_interrupt());
>
> Is there a need for this? I see this being copy-pasted across multiple
> drivers, but maybe it's time to stop?:)
>
> > +
> > + dev_dbg(&icd->dev, "%s (vb=0x%p) 0x%08lx %d\n", __func__,
> > + &buf->vb, buf->vb.baddr, buf->vb.bsize);
>
> You reference buf->vb 6 times in this function, so, maybr it's time for an
> own variable
>
> struct videobuf_buffer *vb = &buf->vb;
>
> ?
>
> > +
> > + /* This waits until this buffer is out of danger, i.e., until it is no
> > + * longer in STATE_QUEUED or STATE_ACTIVE */
>
> multiline comment style
>
> > + videobuf_waiton(&buf->vb, 0, 0);
> > +
> > + videobuf_dma_contig_free(vq, &buf->vb);
> > + dev_dbg(&icd->dev, "%s freed\n", __func__);
> > +
> > + buf->vb.state = VIDEOBUF_NEEDS_INIT;
> > +}
> > +
> > +static int mx2_videobuf_prepare(struct videobuf_queue *vq,
> > + struct videobuf_buffer *vb, enum v4l2_field field)
> > +{
> > + struct soc_camera_device *icd = vq->priv_data;
> > + struct mx2_buffer *buf = container_of(vb, struct mx2_buffer, vb);
> > + int bytes_per_line = soc_mbus_bytes_per_line(icd->user_width,
> > + icd->current_fmt->host_fmt);
> > + int ret = 0;
> > +
> > +#ifdef DEBUG
> > + /* This can be useful if you want to see if we actually fill
> > + * the buffer with something */
>
> comment style
>
> > + memset((void *)vb->baddr, 0xaa, vb->bsize);
> > +#endif
>
> Move this DEBUG block below bytes_per_line check?
>
> > +
> > + dev_dbg(&icd->dev, "%s (vb=0x%p) 0x%08lx %d\n", __func__,
> > + vb, vb->baddr, vb->bsize);
> > +
> > + if (bytes_per_line < 0)
> > + return bytes_per_line;
> > +
> > + if (buf->code != icd->current_fmt->code ||
> > + vb->width != icd->user_width ||
> > + vb->height != icd->user_height ||
> > + vb->field != field) {
> > + buf->code = icd->current_fmt->code;
> > + vb->width = icd->user_width;
> > + vb->height = icd->user_height;
> > + vb->field = field;
> > + vb->state = VIDEOBUF_NEEDS_INIT;
> > + }
> > +
> > + vb->size = bytes_per_line * vb->height;
> > + if (vb->baddr && vb->bsize < vb->size) {
> > + ret = -EINVAL;
> > + goto out;
> > + }
> > +
> > + if (vb->state == VIDEOBUF_NEEDS_INIT) {
> > + ret = videobuf_iolock(vq, vb, NULL);
> > + if (ret)
> > + goto fail;
> > +
> > + vb->state = VIDEOBUF_PREPARED;
> > + }
> > +
> > + return 0;
> > +
> > +fail:
> > + free_buffer(vq, buf);
> > +out:
> > + return ret;
> > +}
> > +
> > +static void mx2_videobuf_queue(struct videobuf_queue *vq,
> > + struct videobuf_buffer *vb)
> > +{
> > + struct soc_camera_device *icd = vq->priv_data;
> > + struct soc_camera_host *ici =
> > + to_soc_camera_host(icd->dev.parent);
> > + struct mx2_camera_dev *pcdev = ici->priv;
> > + struct mx2_buffer *buf = container_of(vb, struct mx2_buffer, vb);
> > + unsigned long flags;
> > + int ret;
> > +
> > + dev_dbg(&icd->dev, "%s (vb=0x%p) 0x%08lx %d\n", __func__,
> > + vb, vb->baddr, vb->bsize);
> > +
> > + spin_lock_irqsave(&pcdev->lock, flags);
> > +
> > + vb->state = VIDEOBUF_QUEUED;
> > + list_add_tail(&vb->queue, &pcdev->capture);
> > +
> > + if (mx27_camera_emma(pcdev))
> > + goto out;
> > + else if (cpu_is_mx27()) {
> > + vb->state = VIDEOBUF_ACTIVE;
>
> You could put this below the "if" to avoid setting state twice in case of
> an error.
>
> > +
> > + if (!pcdev->active) {
> > + ret = imx_dma_setup_single(pcdev->dma,
> > + videobuf_to_dma_contig(vb), vb->size,
> > + (u32)pcdev->base_csi + 0x10,
> > + DMA_MODE_READ);
> > + if (ret) {
> > + vb->state = VIDEOBUF_ERROR;
> > + wake_up(&vb->done);
> > + goto out;
> > + }
> > +
> > + pcdev->active = buf;
> > + }
> > + } else { /* cpu_is_mx25() */
> > + u32 csicr3, dma_inten = 0;
> > +
> > + if (pcdev->fb1_active == NULL) {
>
> I find consistent coding nicer. Just a few lines above you test a pointer
> as "if (!x)", and here it's already "if (x == NULL)", please, select one
> style and follow it across the driver.
>
> > + writel(videobuf_to_dma_contig(vb),
> > + pcdev->base_csi + CSIDMASA_FB1);
> > + pcdev->fb1_active = buf;
> > + dma_inten |= CSICR1_FB1_DMA_INTEN;
>
> No need for "|", just a "=" would do.
>
> > + } else if (pcdev->fb2_active == NULL) {
> > + writel(videobuf_to_dma_contig(vb),
> > + pcdev->base_csi + CSIDMASA_FB2);
> > + pcdev->fb2_active = buf;
> > + dma_inten |= CSICR1_FB2_DMA_INTEN;
>
> ditto
>
> > + }
> > +
> > + if (dma_inten) {
> > + list_del(&vb->queue);
> > + vb->state = VIDEOBUF_ACTIVE;
> > +
> > + csicr3 = readl(pcdev->base_csi + CSICR3);
> > +
> > + /* Reflash DMA */
> > + writel(csicr3 | CSICR3_DMA_REFLASH_RFF,
> > + pcdev->base_csi + CSICR3);
> > +
> > + /* clear & enable interrupts */
> > + writel(dma_inten, pcdev->base_csi + CSISR);
> > + pcdev->csicr1 |= dma_inten;
> > + writel(pcdev->csicr1, pcdev->base_csi + CSICR1);
> > +
> > + /* enable DMA */
> > + csicr3 |= CSICR3_DMA_REQ_EN_RFF | CSICR3_RXFF_LEVEL(1);
> > + writel(csicr3, pcdev->base_csi + CSICR3);
> > + }
> > + }
> > +
> > +out:
> > + spin_unlock_irqrestore(&pcdev->lock, flags);
> > +}
> > +
> > +static void mx2_videobuf_release(struct videobuf_queue *vq,
> > + struct videobuf_buffer *vb)
> > +{
> > + struct soc_camera_device *icd = vq->priv_data;
> > + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> > + struct mx2_camera_dev *pcdev = ici->priv;
> > + struct mx2_buffer *buf = container_of(vb, struct mx2_buffer, vb);
> > + unsigned long flags;
> > +
> > +#ifdef DEBUG
> > + dev_dbg(&icd->dev, "%s (vb=0x%p) 0x%08lx %d\n", __func__,
> > + vb, vb->baddr, vb->bsize);
> > +
> > + switch (vb->state) {
> > + case VIDEOBUF_ACTIVE:
> > + dev_info(&icd->dev, "%s (active)\n", __func__);
> > + break;
> > + case VIDEOBUF_QUEUED:
> > + dev_info(&icd->dev, "%s (queued)\n", __func__);
> > + break;
> > + case VIDEOBUF_PREPARED:
> > + dev_info(&icd->dev, "%s (prepared)\n", __func__);
> > + break;
> > + default:
> > + dev_info(&icd->dev, "%s (unknown) %d\n", __func__,
> > + vb->state);
> > + break;
> > + }
> > +#endif
> > +
> > + spin_lock_irqsave(&pcdev->lock, flags);
> > + if (vb->state == VIDEOBUF_QUEUED)
> > + list_del(&vb->queue);
> > + else if (pcdev->fb1_active == buf)
> > + pcdev->fb1_active = NULL;
> > + else if (pcdev->fb2_active == buf)
> > + pcdev->fb2_active = NULL;
> > + else
> > + goto done;
>
> Don't you also have to check for pcdev->active for i.mx27?
>
> > +
> > + vb->state = VIDEOBUF_ERROR;
> > +
> > +done:
> > + spin_unlock_irqrestore(&pcdev->lock, flags);
> > + free_buffer(vq, buf);
> > +}
> > +
> > +static struct videobuf_queue_ops mx2_videobuf_ops = {
> > + .buf_setup = mx2_videobuf_setup,
> > + .buf_prepare = mx2_videobuf_prepare,
> > + .buf_queue = mx2_videobuf_queue,
> > + .buf_release = mx2_videobuf_release,
> > +};
> > +
> > +static void mx2_camera_init_videobuf(struct videobuf_queue *q,
> > + struct soc_camera_device *icd)
> > +{
> > + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> > + struct mx2_camera_dev *pcdev = ici->priv;
> > +
> > + videobuf_queue_dma_contig_init(q, &mx2_videobuf_ops, pcdev->dev,
> > + &pcdev->lock, V4L2_BUF_TYPE_VIDEO_CAPTURE,
> > + V4L2_FIELD_NONE, sizeof(struct mx2_buffer), icd);
> > +}
> > +
> > +#define MX2_BUS_FLAGS (SOCAM_DATAWIDTH_8 | \
> > + SOCAM_MASTER | \
> > + SOCAM_VSYNC_ACTIVE_HIGH | \
> > + SOCAM_VSYNC_ACTIVE_LOW | \
> > + SOCAM_HSYNC_ACTIVE_HIGH | \
> > + SOCAM_HSYNC_ACTIVE_LOW | \
> > + SOCAM_PCLK_SAMPLE_RISING | \
> > + SOCAM_PCLK_SAMPLE_FALLING | \
> > + SOCAM_DATA_ACTIVE_HIGH | \
> > + SOCAM_DATA_ACTIVE_LOW)
> > +
> > +
>
> Well, you normally do one blank line everywhere, don't you? Then this one
> should better go too.
>
> > +static int mx27_camera_emma_prp_reset(struct mx2_camera_dev *pcdev)
> > +{
> > + unsigned int cntl;
>
> I find it better to use fixed-length variables for hardware registers,
> i.e., u32 in this case. Possibly, this is not the only location.
>
> > +
> > + cntl = readl(pcdev->base_emma + PRP_CNTL);
> > + writel(PRP_CNTL_SWRST, pcdev->base_emma + PRP_CNTL);
> > + while (readl(pcdev->base_emma + PRP_CNTL) & PRP_CNTL_SWRST)
> > + barrier();
> > +
> > + return 0;
>
> The function returns an int and it's checked, but it always returns 0.
> Whereas it'd be better to count-limit the loop and return something like
> -ETIMEDOUT if counter runs out.
>
> > +}
> > +
> > +static void mx27_camera_emma_buf_init(struct soc_camera_device *icd,
> > + int bytesperline)
> > +{
> > + struct soc_camera_host *ici =
> > + to_soc_camera_host(icd->dev.parent);
> > + struct mx2_camera_dev *pcdev = ici->priv;
> > +
> > + writel(pcdev->discard_buffer_dma,
> > + pcdev->base_emma + PRP_DEST_RGB1_PTR);
> > + writel(pcdev->discard_buffer_dma,
> > + pcdev->base_emma + PRP_DEST_RGB2_PTR);
> > +
> > + /* We only use the EMMA engine to get rid of the f**king
>
> maybe you can select a more technical and less emotional comment;)
My bad, I felt very emotional that day ;)
>
> > + * DMA Engine. No color space consversion at the moment.
> > + * We adjust incoming and outgoing pixelformat to rgb16
> > + * and adjust the bytesperline accordingly.
> > + */
>
> comment-style
>
> > + writel(PRP_CNTL_CH1EN |
> > + PRP_CNTL_CSIEN |
> > + PRP_CNTL_DATA_IN_RGB16 |
> > + PRP_CNTL_CH1_OUT_RGB16 |
> > + PRP_CNTL_CH1_LEN |
> > + PRP_CNTL_CH1BYP |
> > + PRP_CNTL_CH1_TSKIP(0) |
> > + PRP_CNTL_IN_TSKIP(0),
> > + pcdev->base_emma + PRP_CNTL);
> > +
> > + writel(((bytesperline >> 1) << 16) | icd->user_height,
> > + pcdev->base_emma + PRP_SRC_FRAME_SIZE);
> > + writel(((bytesperline >> 1) << 16) | icd->user_height,
> > + pcdev->base_emma + PRP_CH1_OUT_IMAGE_SIZE);
> > + writel(bytesperline,
> > + pcdev->base_emma + PRP_DEST_CH1_LINE_STRIDE);
> > + writel(0x2ca00565,
> > + pcdev->base_emma + PRP_SRC_PIXEL_FORMAT_CNTL);
> > + writel(0x2ca00565,
> > + pcdev->base_emma + PRP_CH1_PIXEL_FORMAT_CNTL);
>
> I don't mind using hard-coded numbers, but please, add a comment,
> explaining.
>
> > +
> > + /* Enable interrupts */
> > + writel(PRP_INTR_RDERR |
> > + PRP_INTR_CH1WERR |
> > + PRP_INTR_CH2WERR |
> > + PRP_INTR_CH1FC |
> > + PRP_INTR_CH2FC |
> > + PRP_INTR_LBOVF |
> > + PRP_INTR_CH2OVF
> > + , pcdev->base_emma + PRP_INTR_CNTL);
>
> misplaced comma
>
> > +}
> > +
> > +static int mx2_camera_set_bus_param(struct soc_camera_device *icd,
> > + __u32 pixfmt)
> > +{
> > + struct soc_camera_host *ici =
> > + to_soc_camera_host(icd->dev.parent);
> > + struct mx2_camera_dev *pcdev = ici->priv;
> > + unsigned long camera_flags, common_flags;
> > + int ret = 0;
> > + int bytesperline;
> > + u32 csicr1 = pcdev->csicr1;
> > +
> > + camera_flags = icd->ops->query_bus_param(icd);
> > +
> > + common_flags = soc_camera_bus_param_compatible(camera_flags,
> > + MX2_BUS_FLAGS);
> > + if (!common_flags)
> > + return -EINVAL;
> > +
> > + if ((common_flags & SOCAM_HSYNC_ACTIVE_HIGH) &&
> > + (common_flags & SOCAM_HSYNC_ACTIVE_LOW)) {
> > + if (pcdev->platform_flags & MX2_CAMERA_HSYNC_HIGH)
> > + common_flags &= ~SOCAM_HSYNC_ACTIVE_LOW;
> > + else
> > + common_flags &= ~SOCAM_HSYNC_ACTIVE_HIGH;
> > + }
> > +
> > + if ((common_flags & SOCAM_PCLK_SAMPLE_RISING) &&
> > + (common_flags & SOCAM_PCLK_SAMPLE_FALLING)) {
> > + if (pcdev->platform_flags & MX2_CAMERA_PCLK_SAMPLE_RISING)
> > + common_flags &= ~SOCAM_PCLK_SAMPLE_FALLING;
> > + else
> > + common_flags &= ~SOCAM_PCLK_SAMPLE_RISING;
> > + }
> > +
> > + ret = icd->ops->set_bus_param(icd, common_flags);
> > + if (ret < 0)
> > + return ret;
> > +
> > + if (common_flags & SOCAM_PCLK_SAMPLE_FALLING)
> > + csicr1 |= CSICR1_INV_PCLK;
> > + if (common_flags & SOCAM_VSYNC_ACTIVE_HIGH)
> > + csicr1 |= CSICR1_SOF_POL;
> > + if (common_flags & SOCAM_HSYNC_ACTIVE_HIGH)
> > + csicr1 |= CSICR1_HSYNC_POL;
> > + if (pcdev->platform_flags & MX2_CAMERA_SWAP16)
> > + csicr1 |= CSICR1_SWAP16_EN;
> > + if (pcdev->platform_flags & MX2_CAMERA_EXT_VSYNC)
> > + csicr1 |= CSICR1_EXT_VSYNC;
> > + if (pcdev->platform_flags & MX2_CAMERA_CCIR)
> > + csicr1 |= CSICR1_CCIR_EN;
> > + if (pcdev->platform_flags & MX2_CAMERA_CCIR_INTERLACE)
> > + csicr1 |= CSICR1_CCIR_MODE;
> > + if (pcdev->platform_flags & MX2_CAMERA_GATED_CLOCK)
> > + csicr1 |= CSICR1_GCLK_MODE;
> > + if (pcdev->platform_flags & MX2_CAMERA_INV_DATA)
> > + csicr1 |= CSICR1_INV_DATA;
> > + if (pcdev->platform_flags & MX2_CAMERA_PACK_DIR_MSB)
> > + csicr1 |= CSICR1_PACK_DIR;
> > +
> > + pcdev->csicr1 = csicr1;
> > +
> > + bytesperline = soc_mbus_bytes_per_line(icd->user_width,
> > + icd->current_fmt->host_fmt);
> > + if (bytesperline < 0)
> > + return bytesperline;
> > +
> > + if (mx27_camera_emma(pcdev)) {
> > + if (mx27_camera_emma_prp_reset(pcdev))
> > + return -ENODEV;
> > +
> > + if (pcdev->discard_buffer)
> > + dma_free_coherent(NULL, pcdev->discard_size,
> > + pcdev->discard_buffer,
> > + pcdev->discard_buffer_dma);
>
> ici->v4l2_dev.dev and below
>
> > +
> > + /* I didn't manage to properly enable/disable the prp
> > + * on a per frame basis during running transfers,
> > + * thus we allocate a buffer here and use it to
> > + * discard frames when no buffer is available.
> > + * Feel free to work on this ;)
> > + */
>
> comment style
>
> > + pcdev->discard_size = icd->user_height * bytesperline;
> > + pcdev->discard_buffer = dma_alloc_coherent(NULL,
> > + pcdev->discard_size, &pcdev->discard_buffer_dma,
> > + GFP_KERNEL);
> > + if (!pcdev->discard_buffer)
> > + return -ENOMEM;
> > +
> > + mx27_camera_emma_buf_init(icd, bytesperline);
> > + } else if (cpu_is_mx25()) {
> > + writel((bytesperline * icd->user_height) >> 2,
> > + pcdev->base_csi + CSIRXCNT);
> > + writel((bytesperline << 16) | icd->user_height,
> > + pcdev->base_csi + CSIIMAG_PARA);
> > + }
> > +
> > + writel(pcdev->csicr1, pcdev->base_csi + CSICR1);
> > +
> > + return 0;
> > +}
> > +
> > +static int mx2_camera_set_crop(struct soc_camera_device *icd,
> > + struct v4l2_crop *a)
> > +{
> > + struct v4l2_rect *rect = &a->c;
> > + struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
> > + struct v4l2_format f = {.type = V4L2_BUF_TYPE_VIDEO_CAPTURE};
> > + struct v4l2_pix_format *pix = &f.fmt.pix;
> > + int ret;
> > +
> > + soc_camera_limit_side(&rect->left, &rect->width, 0, 2, 4096);
> > + soc_camera_limit_side(&rect->top, &rect->height, 0, 2, 4096);
> > +
> > + ret = v4l2_subdev_call(sd, video, s_crop, a);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /* The capture device might have changed its output */
> > + ret = v4l2_subdev_call(sd, video, g_fmt, &f);
>
> Have you tested cropping?;) You should be using g_mbus_fmt here and a
> different (fourth) format parameter, of course, too. Since you're
> supporting a changed output geometry, you should also verify, that the new
> geometry is still acceptable for you and adjust your configuration.
>
> > + if (ret < 0)
> > + return ret;
> > +
> > + dev_dbg(icd->dev.parent, "Sensor cropped %dx%d\n",
> > + pix->width, pix->height);
> > +
> > + icd->user_width = pix->width;
> > + icd->user_height = pix->height;
> > +
> > + return ret;
> > +}
> > +
> > +static int mx2_camera_set_fmt(struct soc_camera_device *icd,
> > + struct v4l2_format *f)
> > +{
> > + struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
> > + const struct soc_camera_format_xlate *xlate;
> > + struct v4l2_pix_format *pix = &f->fmt.pix;
> > + struct v4l2_mbus_framefmt mf;
> > + int ret;
> > +
> > + xlate = soc_camera_xlate_by_fourcc(icd, pix->pixelformat);
> > + if (!xlate) {
> > + dev_warn(icd->dev.parent, "Format %x not found\n",
> > + pix->pixelformat);
> > + return -EINVAL;
> > + }
> > +
> > + mf.width = pix->width;
> > + mf.height = pix->height;
> > + mf.field = pix->field;
> > + mf.colorspace = pix->colorspace;
> > + mf.code = xlate->code;
> > +
> > + ret = v4l2_subdev_call(sd, video, s_mbus_fmt, &mf);
> > + if (ret < 0 && ret != -ENOIOCTLCMD)
> > + return ret;
> > +
> > + if (mf.code != xlate->code)
> > + return -EINVAL;
> > +
> > + pix->width = mf.width;
> > + pix->height = mf.height;
> > + pix->field = mf.field;
> > + pix->colorspace = mf.colorspace;
> > + icd->current_fmt = xlate;
> > +
> > + return 0;
> > +}
> > +
> > +static int mx2_camera_try_fmt(struct soc_camera_device *icd,
> > + struct v4l2_format *f)
> > +{
> > + return 0;
>
> Oops, no, no good. Please, add relevant checks and parameter adjustments.
>
> > +}
> > +
> > +static int mx2_camera_querycap(struct soc_camera_host *ici,
> > + struct v4l2_capability *cap)
> > +{
> > + /* cap->name is set by the friendly caller:-> */
> > + strlcpy(cap->card, MX2_CAM_DRIVER_DESCRIPTION, sizeof(cap->card));
> > + cap->version = MX2_CAM_VERSION_CODE;
> > + cap->capabilities = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
> > +
> > + return 0;
> > +}
> > +
> > +static int mx2_camera_reqbufs(struct soc_camera_file *icf,
> > + struct v4l2_requestbuffers *p)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < p->count; i++) {
> > + struct mx2_buffer *buf = container_of(icf->vb_vidq.bufs[i],
> > + struct mx2_buffer, vb);
> > + INIT_LIST_HEAD(&buf->vb.queue);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void mx27_camera_frame_done(struct mx2_camera_dev *pcdev, int state)
> > +{
> > + struct videobuf_buffer *vb;
> > + struct mx2_buffer *buf;
> > + int ret;
> > +
> > + if (!pcdev->active) {
> > + dev_err(pcdev->dev, "%s called with no active buffer!\n",
> > + __func__);
> > + return;
> > + }
> > +
> > + vb = &pcdev->active->vb;
> > + buf = container_of(vb, struct mx2_buffer, vb);
> > + WARN_ON(list_empty(&vb->queue));
> > + dev_dbg(pcdev->dev, "%s (vb=0x%p) 0x%08lx %d\n", __func__,
> > + vb, vb->baddr, vb->bsize);
> > +
> > + /* _init is used to debug races, see comment in pxa_camera_reqbufs() */
> > + list_del_init(&vb->queue);
> > + vb->state = state;
> > + do_gettimeofday(&vb->ts);
> > + vb->field_count++;
> > +
> > + wake_up(&vb->done);
> > +
> > + if (list_empty(&pcdev->capture)) {
> > + pcdev->active = NULL;
> > + return;
> > + }
> > +
> > + pcdev->active = list_entry(pcdev->capture.next,
> > + struct mx2_buffer, vb.queue);
> > +
> > + vb = &pcdev->active->vb;
> > + vb->state = VIDEOBUF_ACTIVE;
> > +
> > + ret = imx_dma_setup_single(pcdev->dma, videobuf_to_dma_contig(vb),
> > + vb->size, (u32)pcdev->base_csi + 0x10, DMA_MODE_READ);
> > + if (ret) {
> > + vb->state = VIDEOBUF_ERROR;
> > + wake_up(&vb->done);
>
> I think, in this case you don't want pcdev->active to be set.
>
> > + return;
>
> superfluous "return."
>
> > + }
> > +}
> > +
> > +static void mx27_camera_dma_err_callback(int channel, void *data, int err)
> > +{
> > + struct mx2_camera_dev *pcdev = data;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&pcdev->lock, flags);
> > +
> > + mx27_camera_frame_done(pcdev, VIDEOBUF_ERROR);
>
> I think, it would be better to take and release the spinlock inside the
> frame_done function, similar to mx25. This would improve consistency and
> reduce the number of spinlock manipulating locations (here and below) by
> one.
>
> > +
> > + spin_unlock_irqrestore(&pcdev->lock, flags);
> > +}
> > +
> > +static void mx27_camera_dma_callback(int channel, void *data)
> > +{
> > + struct mx2_camera_dev *pcdev = data;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&pcdev->lock, flags);
> > +
> > + mx27_camera_frame_done(pcdev, VIDEOBUF_DONE);
> > +
> > + spin_unlock_irqrestore(&pcdev->lock, flags);
> > +}
> > +
> > +#define DMA_REQ_CSI_RX 31 /* FIXME: Add this to a resource */
> > +
> > +static int mx27_camera_dma_init(struct platform_device *pdev,
> > + struct mx2_camera_dev *pcdev)
>
> This is only called from mx2_camera_probe(), so, you can make this one
> "__devinit" too.
>
> > +{
> > + int err;
> > +
> > + pcdev->dma = imx_dma_request_by_prio("CSI RX DMA", DMA_PRIO_HIGH);
> > + if (pcdev->dma < 0) {
> > + dev_err(&pdev->dev, "%s failed to request DMA channel\n",
> > + __func__);
> > + return pcdev->dma;
> > + }
> > +
> > + err = imx_dma_setup_handlers(pcdev->dma, mx27_camera_dma_callback,
> > + mx27_camera_dma_err_callback, pcdev);
> > + if (err != 0) {
> > + dev_err(&pdev->dev, "%s failed to set DMA callback\n",
> > + __func__);
> > + imx_dma_free(pcdev->dma);
> > + return err;
> > + }
> > +
> > + imx_dma_config_channel(pcdev->dma,
> > + IMX_DMA_MEMSIZE_32 | IMX_DMA_TYPE_FIFO,
> > + IMX_DMA_MEMSIZE_32 | IMX_DMA_TYPE_LINEAR,
> > + DMA_REQ_CSI_RX, 1);
>
> Ok, looking at the function implementation, it cannot really fail in your
> case - on i.mx27, but it'd be better to either check for the return code
> in any case - e.g., in case the function changes, or at least add a
> comment.
>
> > +
> > + imx_dma_config_burstlen(pcdev->dma, 64);
> > +
> > + return 0;
> > +}
> > +
> > +static unsigned int mx2_camera_poll(struct file *file, poll_table *pt)
> > +{
> > + struct soc_camera_file *icf = file->private_data;
> > + struct mx2_buffer *buf;
> > +
> > + buf = list_entry(icf->vb_vidq.stream.next, struct mx2_buffer,
> > + vb.stream);
> > +
> > + poll_wait(file, &buf->vb.done, pt);
> > +
> > + if (buf->vb.state == VIDEOBUF_DONE ||
> > + buf->vb.state == VIDEOBUF_ERROR)
> > + return POLLIN | POLLRDNORM;
>
> Any specific reason not to use videobuf_poll_stream() (see mx3_camera.c)?
> Apart from copy-pasting from other soc-camera drivers;)
>
> > +
> > + return 0;
> > +}
> > +
> > +static struct soc_camera_host_ops mx2_soc_camera_host_ops = {
> > + .owner = THIS_MODULE,
> > + .add = mx2_camera_add_device,
> > + .remove = mx2_camera_remove_device,
> > + .set_fmt = mx2_camera_set_fmt,
> > + .set_crop = mx2_camera_set_crop,
> > + .try_fmt = mx2_camera_try_fmt,
> > + .init_videobuf = mx2_camera_init_videobuf,
> > + .reqbufs = mx2_camera_reqbufs,
> > + .poll = mx2_camera_poll,
> > + .querycap = mx2_camera_querycap,
> > + .set_bus_param = mx2_camera_set_bus_param,
> > +};
> > +
> > +static void mx27_camera_frame_done_emma(struct mx2_camera_dev *pcdev,
> > + int bufnum, int state)
> > +{
> > + struct mx2_buffer *buf;
> > + struct videobuf_buffer *vb;
> > + unsigned long phys;
> > +
> > + if (!list_empty(&pcdev->active_bufs)) {
> > + buf = list_entry(pcdev->active_bufs.next,
> > + struct mx2_buffer, vb.queue);
> > +
> > + if (buf->bufnum == bufnum) {
>
> Hmmm... In the ISR below bits for both channels are tested in the status
> non-exclusively, and this function is called first for channel 1, then for
> channel 2. This means, the ISR is prepared for the (unlikely) case, that
> both bits are set, if we missed one interrupt. Now think what happens, if
> you're expecting completion on channel 2, then you leave channel 1
> unprocessed... This is not very trivial to fix without testing, but I
> think, there is a relatively easy fix - see below.
>
> After you've done that proposed change, I'd suggest to change this "if" to
> a "BUG_ON(buf->bufnum != bufnum)", so that the first person that uses this
> driver with eMMA sees immediately, if our fix was wrong;)
>
> > + vb = &buf->vb;
> > +#ifdef DEBUG
> > + phys = videobuf_to_dma_contig(vb);
> > + if (readl(pcdev->base_emma + PRP_DEST_RGB1_PTR +
> > + 4 * bufnum) != phys) {
> > + dev_err(pcdev->dev, "%p != %p\n", phys,
> > + readl(pcdev->base_emma +
> > + PRP_DEST_RGB1_PTR +
> > + 4 * bufnum));
> > + }
> > +#endif
> > + dev_dbg(pcdev->dev, "%s (vb=0x%p) 0x%08lx %d\n",
> > + __func__, vb, vb->baddr, vb->bsize);
> > +
> > + list_del(&vb->queue);
> > + vb->state = state;
> > + do_gettimeofday(&vb->ts);
> > + vb->field_count++;
> > +
> > + wake_up(&vb->done);
> > + }
> > + }
> > +
> > + if (list_empty(&pcdev->capture)) {
> > + writel(pcdev->discard_buffer_dma, pcdev->base_emma +
> > + PRP_DEST_RGB1_PTR + 4 * bufnum);
> > + return;
> > + }
> > +
> > + buf = list_entry(pcdev->capture.next,
> > + struct mx2_buffer, vb.queue);
> > +
> > + buf->bufnum = bufnum;
> > +
> > + list_move_tail(pcdev->capture.next, &pcdev->active_bufs);
> > +
> > + vb = &buf->vb;
> > + vb->state = VIDEOBUF_ACTIVE;
> > +
> > + phys = videobuf_to_dma_contig(vb);
> > + writel(phys, pcdev->base_emma + PRP_DEST_RGB1_PTR + 4 * bufnum);
> > +}
> > +
> > +static irqreturn_t mx27_camera_emma_irq(int irq_emma, void *data)
> > +{
> > + struct mx2_camera_dev *pcdev = data;
> > + unsigned int status = readl(pcdev->base_emma + PRP_INTRSTATUS);
> > +
> > + if (status & (1 << 6))
> > + mx27_camera_frame_done_emma(pcdev, 0, VIDEOBUF_DONE);
> > + if (status & (1 << 5))
> > + mx27_camera_frame_done_emma(pcdev, 1, VIDEOBUF_DONE);
>
> So, the proposed fix is the following: look which channel is expected to
> complete and call mx27_camera_frame_done_emma() for it first. Only then
> check the other channel.
>
> > + if (status & (1 << 7)) {
> > + uint32_t cntl;
>
> grrrr.... consistency - use u32.
>
> > + cntl = readl(pcdev->base_emma + PRP_CNTL);
> > + writel(cntl & ~PRP_CNTL_CH1EN, pcdev->base_emma + PRP_CNTL);
> > + writel(cntl, pcdev->base_emma + PRP_CNTL);
>
> Hm, this doesn't look right to me. Why disable hard-coded channel 1?
> cannot the line buffer overflow on channel 2? Sascha?
We only have channel one enabled. The driver will need some rework if we
wish to enable the second channel. I can't remember though if this ever
triggered.
>
> > + }
> > +
> > + writel(status, pcdev->base_emma + PRP_INTRSTATUS);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static int mx27_camera_emma_init(struct mx2_camera_dev *pcdev)
>
> This is only called from mx2_camera_probe(), so, you can make this one
> "__devinit" too.
>
> > +{
> > + struct resource *res_emma = pcdev->res_emma;
> > + int err = 0;
> > +
> > + if (!request_mem_region(res_emma->start, resource_size(res_emma),
> > + MX2_CAM_DRV_NAME)) {
> > + err = -EBUSY;
> > + goto out;
> > + }
> > +
> > + pcdev->base_emma = ioremap(res_emma->start, resource_size(res_emma));
> > + if (!pcdev->base_emma) {
> > + err = -ENOMEM;
> > + goto exit_release;
> > + }
> > +
> > + err = request_irq(pcdev->irq_emma, mx27_camera_emma_irq, 0,
> > + MX2_CAM_DRV_NAME, pcdev);
> > + if (err) {
> > + dev_err(pcdev->dev, "Camera EMMA interrupt register failed \n");
> > + goto exit_iounmap;
> > + }
> > +
> > + pcdev->clk_emma = clk_get(NULL, "emma");
> > + if (IS_ERR(pcdev->clk_emma)) {
> > + err = PTR_ERR(pcdev->clk_emma);
> > + goto exit_free_irq;
> > + }
> > +
> > + clk_enable(pcdev->clk_emma);
> > +
> > + err = mx27_camera_emma_prp_reset(pcdev);
> > + if (err)
> > + goto exit_clk_emma_put;
> > +
> > + return err;
> > +
> > +exit_clk_emma_put:
> > + clk_disable(pcdev->clk_emma);
> > + clk_put(pcdev->clk_emma);
> > +exit_free_irq:
> > + free_irq(pcdev->irq_emma, pcdev);
> > +exit_iounmap:
> > + iounmap(pcdev->base_emma);
> > +exit_release:
> > + release_mem_region(res_emma->start, resource_size(res_emma));
> > +out:
> > + return err;
> > +}
> > +
> > +static int mx2_camera_probe(struct platform_device *pdev)
>
> You use "__devexit" for mx2_camera_remove() below, so, you should
> "__devinit" here then too.
>
> > +{
> > + struct mx2_camera_dev *pcdev;
> > + struct resource *res_csi, *res_emma;
> > + void __iomem *base_csi;
> > + unsigned int irq_csi, irq_emma;
> > + irq_handler_t mx2_cam_irq_handler = cpu_is_mx25() ? mx25_camera_irq
> > + : mx27_camera_irq;
> > + int err = 0;
> > +
> > + dev_info(&pdev->dev, "initialising\n");
>
> This is not very informative...
>
> > +
> > + res_csi = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + irq_csi = platform_get_irq(pdev, 0);
> > + if (!res_csi || !irq_csi) {
>
> Wrong error condition: platform_get_irq() returns a negative error code on
> failure
>
> > + dev_err(&pdev->dev, "Missing platform resources data\n");
> > + err = -ENODEV;
> > + goto exit;
> > + }
> > +
> > + pcdev = kzalloc(sizeof(*pcdev), GFP_KERNEL);
> > + if (!pcdev) {
> > + dev_err(&pdev->dev, "Could not allocate pcdev\n");
> > + err = -ENOMEM;
> > + goto exit;
> > + }
> > +
> > + pcdev->clk_csi = clk_get(&pdev->dev, NULL);
> > + if (IS_ERR(pcdev->clk_csi)) {
> > + err = PTR_ERR(pcdev->clk_csi);
> > + goto exit_kfree;
> > + }
> > +
> > + dev_info(&pdev->dev, "Camera clock frequency: %ld\n",
> > + clk_get_rate(pcdev->clk_csi));
>
> nor is this. If you really want to see a kernel message on probe, try to
> come up with something optimistically-informative:)
>
> > +
> > + /* Initialize DMA */
> > + if (cpu_is_mx27()) {
> > + err = mx27_camera_dma_init(pdev, pcdev);
> > + if (err)
> > + goto exit_clk_put;
> > + }
> > +
> > + pcdev->res_csi = res_csi;
> > + pcdev->pdata = pdev->dev.platform_data;
> > + if (pcdev->pdata) {
> > + long rate;
> > +
> > + pcdev->platform_flags = pcdev->pdata->flags;
> > +
> > + rate = clk_round_rate(pcdev->clk_csi, pcdev->pdata->clk * 2);
> > + clk_set_rate(pcdev->clk_csi, rate);
>
> At least check, that rate is positive, before calling set_rate. And if you
> indeed want to continue with these two calls failing, maybe at least issue
> a warning "failed to program frequency %u, continuing with current
> frequency %u" or something similar.
>
> > + }
> > +
> > + INIT_LIST_HEAD(&pcdev->capture);
> > + INIT_LIST_HEAD(&pcdev->active_bufs);
> > + spin_lock_init(&pcdev->lock);
> > +
> > + /*
> > + * Request the regions.
> > + */
> > + if (!request_mem_region(res_csi->start, resource_size(res_csi),
> > + MX2_CAM_DRV_NAME)) {
> > + err = -EBUSY;
> > + goto exit_dma_free;
> > + }
> > +
> > + base_csi = ioremap(res_csi->start, resource_size(res_csi));
> > + if (!base_csi) {
> > + err = -ENOMEM;
> > + goto exit_release;
> > + }
> > + pcdev->irq_csi = irq_csi;
> > + pcdev->base_csi = base_csi;
> > + pcdev->dev = &pdev->dev;
> > +
> > + err = request_irq(pcdev->irq_csi, mx2_cam_irq_handler, 0,
> > + MX2_CAM_DRV_NAME, pcdev);
> > + if (err) {
> > + dev_err(pcdev->dev, "Camera interrupt register failed \n");
> > + goto exit_iounmap;
> > + }
> > +
> > + if (cpu_is_mx27()) {
> > + /* EMMA support */
> > + res_emma = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > + irq_emma = platform_get_irq(pdev, 1);
> > +
> > + if (res_emma && irq_emma) {
>
> Same here, check for "(int)irq_emma >= 0"
>
> > + dev_info(&pdev->dev, "Using EMMA\n");
> > + pcdev->use_emma = 1;
> > + pcdev->res_emma = res_emma;
> > + pcdev->irq_emma = irq_emma;
> > + if (mx27_camera_emma_init(pcdev))
> > + goto exit_free_irq;
> > + }
> > + }
> > +
> > + pcdev->soc_host.drv_name = MX2_CAM_DRV_NAME,
> > + pcdev->soc_host.ops = &mx2_soc_camera_host_ops,
> > + pcdev->soc_host.priv = pcdev;
> > + pcdev->soc_host.v4l2_dev.dev = &pdev->dev;
> > + pcdev->soc_host.nr = pdev->id;
> > + err = soc_camera_host_register(&pcdev->soc_host);
> > + if (err)
> > + goto exit_free_emma;
> > +
> > + return 0;
> > +
> > +exit_free_emma:
> > + if (mx27_camera_emma(pcdev)) {
> > + free_irq(pcdev->irq_emma, pcdev);
> > + clk_disable(pcdev->clk_emma);
> > + clk_put(pcdev->clk_emma);
> > + iounmap(pcdev->base_emma);
> > + release_mem_region(res_emma->start, resource_size(res_emma));
> > + }
> > +exit_free_irq:
> > + free_irq(pcdev->irq_csi, pcdev);
> > +exit_iounmap:
> > + iounmap(base_csi);
> > +exit_release:
> > + release_mem_region(res_csi->start, resource_size(res_csi));
> > +exit_dma_free:
> > + if (cpu_is_mx27())
> > + imx_dma_free(pcdev->dma);
> > +exit_clk_put:
> > + clk_put(pcdev->clk_csi);
> > +exit_kfree:
> > + kfree(pcdev);
> > +exit:
> > + return err;
> > +}
> > +
> > +static int __devexit mx2_camera_remove(struct platform_device *pdev)
> > +{
> > + struct soc_camera_host *soc_host = to_soc_camera_host(&pdev->dev);
> > + struct mx2_camera_dev *pcdev = container_of(soc_host,
> > + struct mx2_camera_dev, soc_host);
> > + struct resource *res;
> > +
> > + clk_put(pcdev->clk_csi);
> > + if (cpu_is_mx27())
> > + imx_dma_free(pcdev->dma);
> > + free_irq(pcdev->irq_csi, pcdev);
> > + if (mx27_camera_emma(pcdev))
> > + free_irq(pcdev->irq_emma, pcdev);
> > +
> > + soc_camera_host_unregister(&pcdev->soc_host);
> > +
> > + iounmap(pcdev->base_csi);
> > +
> > + if (mx27_camera_emma(pcdev)) {
> > + clk_disable(pcdev->clk_emma);
> > + clk_put(pcdev->clk_emma);
> > + iounmap(pcdev->base_emma);
> > + res = pcdev->res_emma;
> > + release_mem_region(res->start, resource_size(res));
> > + }
> > +
> > + res = pcdev->res_csi;
> > + release_mem_region(res->start, resource_size(res));
> > +
> > + kfree(pcdev);
> > +
> > + dev_info(&pdev->dev, "MX2 Camera driver unloaded\n");
> > +
> > + return 0;
> > +}
> > +
> > +static struct platform_driver mx2_camera_driver = {
> > + .driver = {
> > + .name = MX2_CAM_DRV_NAME,
> > + },
> > + .probe = mx2_camera_probe,
> > + .remove = __exit_p(mx2_camera_remove),
>
> "__devexit_p"
>
> > +};
> > +
> > +
> > +static int __devinit mx2_camera_init(void)
>
> No, this should be "__init"
>
> > +{
> > + return platform_driver_register(&mx2_camera_driver);
>
> Since this hardware is not hot-pluggable, you might want to just use
> platform_driver_probe().
>
> > +}
> > +
> > +static void __exit mx2_camera_exit(void)
> > +{
> > + return platform_driver_unregister(&mx2_camera_driver);
> > +}
> > +
> > +module_init(mx2_camera_init);
> > +module_exit(mx2_camera_exit);
> > +
> > +MODULE_DESCRIPTION("i.MX27/i.MX25 SoC Camera Host driver");
> > +MODULE_AUTHOR("Sascha Hauer <sha@xxxxxxxxxxxxxx>");
> > +MODULE_LICENSE("GPL");
> > --
> > 1.7.0
>
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
>
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
