Hi Janusz Thanks once more for the patches, from your patches it is obvious, that you've spent quite a bit of time on them and you have not just copy-pasted various bits and pieces from other drivers, just filling your hardware details, but you also actually understand a lot of what is actually going on in there. So, I think, fixing remaining mostly minor issues shouldn't be too difficult for you either. On Sun, 18 Jul 2010, Janusz Krzysztofik wrote:

This is a V4L2 driver for TI OMAP1 SoC camera interface.

Two versions of the driver are provided, using either videobuf-dma-contig or videobuf-dma-sg. The former uses less processing power, but often fails to allocate contignuous buffer memory. The latter is free of this problem, but generates tens of DMA interrupts per frame.

Hm, would it be difficult to first try contig, and if it fails - fall back to sg? In its present state buffer management in your driver is pretty difficult to follow. At the VERY least you have to extensively document your buffer manipulations. Better yet clean it up. AFAIU, your "ready" pointer is pretty much unused in the contiguous case. Or you can easily get rid of it. So, I think, a very welcome improvement to the driver would be a cleaner separation between the two cases. Don't try that much to reuse the code as much as possible. Would be much better to have clean separation between the two implementations - whether dynamically switchable at runtime or at config time - would be best to separate the two implementations to the point, where each of them becomes understandable and maintainable. So, I will do a pretty formal review this time and wait for v2. And in v2 I'd like to at the very least see detailed buffer-management documentation, or - better yet - a rework with a clean separation.

Both versions work stable for me, even under heavy load, on my OMAP1510 based Amstrad Delta videophone, that is the oldest, least powerfull OMAP1 implementation.

The interface generally works in pass-through mode. Since input data byte endianess can be swapped, it provides up to two v4l2 pixel formats per each of several soc_mbus formats that have their swapped endian counterparts.

Boards using this driver can provide it with the followning information: - if and what freqency clock is expected by an on-board camera sensor, - what is the maximum pixel clock that should be accepted from the sensor, - what is the polarity of the sensor provided pixel clock, - if the interface GPIO line is connected to a sensor reset/powerdown input and what is the input polarity.

Created and tested against linux-2.6.35-rc3 on Amstrad Delta.

Signed-off-by: Janusz Krzysztofik <jkrzyszt@tis.icnet.pl> --- drivers/media/video/Kconfig | 14 drivers/media/video/Makefile | 1 drivers/media/video/omap1_camera.c | 1656 +++++++++++++++++++++++++++++++++++++ include/media/omap1_camera.h | 16 4 files changed, 1687 insertions(+)

--- linux-2.6.35-rc3.orig/include/media/omap1_camera.h 1970-01-01 01:00:00.000000000 +0100 +++ linux-2.6.35-rc3/include/media/omap1_camera.h 2010-07-18 01:31:57.000000000 +0200 @@ -0,0 +1,16 @@

A copyright / licence header here, please

+#ifndef __MEDIA_OMAP1_CAMERA_H_ +#define __MEDIA_OMAP1_CAMERA_H_ + +#define OMAP1_CAMERA_IOSIZE 0x1c + +struct omap1_cam_platform_data { + unsigned long camexclk_khz; + unsigned long lclk_khz_max; + unsigned long flags; +}; + +#define OMAP1_CAMERA_LCLK_RISING BIT(0) +#define OMAP1_CAMERA_RST_LOW BIT(1) +#define OMAP1_CAMERA_RST_HIGH BIT(2)

Then you need to #include <linux/bitops.h>

+ +#endif /* __MEDIA_OMAP1_CAMERA_H_ */ --- linux-2.6.35-rc3.orig/drivers/media/video/Kconfig 2010-06-26 15:55:29.000000000 +0200 +++ linux-2.6.35-rc3/drivers/media/video/Kconfig 2010-07-02 04:12:02.000000000 +0200 @@ -962,6 +962,20 @@ config VIDEO_SH_MOBILE_CEU ---help--- This is a v4l2 driver for the SuperH Mobile CEU Interface

+config VIDEO_OMAP1 + tristate "OMAP1 Camera Interface driver" + depends on VIDEO_DEV && ARCH_OMAP1 && SOC_CAMERA + select VIDEOBUF_DMA_CONTIG if !VIDEO_OMAP1_SG + ---help--- + This is a v4l2 driver for the TI OMAP1 camera interface + +if VIDEO_OMAP1

Don't think you need this "if," the "depends on" below should suffice.

+config VIDEO_OMAP1_SG + bool "Scatter-gather mode" + depends on VIDEO_OMAP1 && EXPERIMENTAL + select VIDEOBUF_DMA_SG +endif + config VIDEO_OMAP2 tristate "OMAP2 Camera Capture Interface driver" depends on VIDEO_DEV && ARCH_OMAP2 --- linux-2.6.35-rc3.orig/drivers/media/video/Makefile 2010-06-26 15:55:29.000000000 +0200 +++ linux-2.6.35-rc3/drivers/media/video/Makefile 2010-06-26 17:28:09.000000000 +0200 @@ -165,6 +165,7 @@ obj-$(CONFIG_VIDEO_MX1) += mx1_camera. 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 +obj-$(CONFIG_VIDEO_OMAP1) += omap1_camera.o

obj-$(CONFIG_ARCH_DAVINCI) += davinci/

--- linux-2.6.35-rc3.orig/drivers/media/video/omap1_camera.c 1970-01-01 01:00:00.000000000 +0100 +++ linux-2.6.35-rc3/drivers/media/video/omap1_camera.c 2010-07-18 01:32:48.000000000 +0200 @@ -0,0 +1,1656 @@ +/* + * V4L2 SoC Camera driver for OMAP1 Camera Interface + * + * Copyright (C) 2010, Janusz Krzysztofik <jkrzyszt@tis.icnet.pl> + * + * Based on V4L2 Driver for i.MXL/i.MXL camera (CSI) host + * Copyright (C) 2008, Paulius Zaleckas <paulius.zaleckas@teltonika.lt> + * Copyright (C) 2009, Darius Augulis <augulis.darius@gmail.com> + * + * Based on PXA SoC camera driver + * Copyright (C) 2006, Sascha Hauer, Pengutronix + * Copyright (C) 2008, Guennadi Liakhovetski <kernel@pengutronix.de> + * + * Hardware specific bits initialy based on former work by Matt Callow + * drivers/media/video/omap/omap1510cam.c + * Copyright (C) 2006 Matt Callow + * + * 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/dma-mapping.h> +#include <linux/interrupt.h> +#include <linux/platform_device.h> +#include <linux/slab.h> +#include <linux/version.h> + +#include <media/omap1_camera.h> +#include <media/soc_camera.h> +#include <media/soc_mediabus.h> +#ifndef CONFIG_VIDEO_OMAP1_SG +#include <media/videobuf-dma-contig.h> +#else +#include <media/videobuf-dma-sg.h> +#endif

I think, you can just include both headers.

+ +#include <plat/dma.h> + + +#define DRIVER_NAME "omap1-camera" +#define VERSION_CODE KERNEL_VERSION(0, 0, 1) + + +/* + * --------------------------------------------------------------------------- + * OMAP1 Camera Interface registers + * --------------------------------------------------------------------------- + */ + +#define REG_CTRLCLOCK 0x00 +#define REG_IT_STATUS 0x04 +#define REG_MODE 0x08 +#define REG_STATUS 0x0C +#define REG_CAMDATA 0x10 +#define REG_GPIO 0x14 +#define REG_PEAK_COUNTER 0x18 + +/* CTRLCLOCK bit shifts */ +#define LCLK_EN BIT(7) +#define DPLL_EN BIT(6) +#define MCLK_EN BIT(5) +#define CAMEXCLK_EN BIT(4) +#define POLCLK BIT(3) +#define FOSCMOD_SHIFT 0 +#define FOSCMOD_MASK (0x7 << FOSCMOD_SHIFT) +#define FOSCMOD_12MHz 0x0 +#define FOSCMOD_6MHz 0x2 +#define FOSCMOD_9_6MHz 0x4 +#define FOSCMOD_24MHz 0x5 +#define FOSCMOD_8MHz 0x6 + +/* IT_STATUS bit shifts */ +#define DATA_TRANSFER BIT(5) +#define FIFO_FULL BIT(4) +#define H_DOWN BIT(3) +#define H_UP BIT(2) +#define V_DOWN BIT(1) +#define V_UP BIT(0) + +/* MODE bit shifts */ +#define RAZ_FIFO BIT(18) +#define EN_FIFO_FULL BIT(17) +#define EN_NIRQ BIT(16) +#define THRESHOLD_SHIFT 9 +#define THRESHOLD_MASK (0x7f << THRESHOLD_SHIFT) +#define DMA BIT(8) +#define EN_H_DOWN BIT(7) +#define EN_H_UP BIT(6) +#define EN_V_DOWN BIT(5) +#define EN_V_UP BIT(4) +#define ORDERCAMD BIT(3) + +#define IRQ_MASK (EN_V_UP | EN_V_DOWN | EN_H_UP | EN_H_DOWN | \ + EN_NIRQ | EN_FIFO_FULL) + +/* STATUS bit shifts */ +#define HSTATUS BIT(1) +#define VSTATUS BIT(0) + +/* GPIO bit shifts */ +#define CAM_RST BIT(0) + +/* end of OMAP1 Camera Interface registers */ + + +#define SOCAM_BUS_FLAGS (SOCAM_MASTER | \ + SOCAM_HSYNC_ACTIVE_HIGH | SOCAM_VSYNC_ACTIVE_HIGH | \ + SOCAM_PCLK_SAMPLE_RISING | SOCAM_PCLK_SAMPLE_FALLING | \ + SOCAM_DATA_ACTIVE_HIGH | SOCAM_DATAWIDTH_8) + + +#define CAM_EXCLK_6MHz 6000000 +#define CAM_EXCLK_8MHz 8000000 +#define CAM_EXCLK_9_6MHz 9600000 +#define CAM_EXCLK_12MHz 12000000 +#define CAM_EXCLK_24MHz 24000000

Why do you need this? Your platform specifies the EXCLK frequency directly in kHz, then you multiplly it by 1000 to get pcdev->camexclk in Hz, then you compare it to these macros. Don't think you need them - just use numbers directly.

+ + +#define FIFO_SIZE ((THRESHOLD_MASK >> THRESHOLD_SHIFT) + 1) +#define FIFO_SHIFT __fls(FIFO_SIZE) + +#define DMA_BURST_SHIFT (1 + OMAP_DMA_DATA_BURST_4) +#define DMA_BURST_SIZE (1 << DMA_BURST_SHIFT) + +#define DMA_ELEMENT_SHIFT OMAP_DMA_DATA_TYPE_S32 +#define DMA_ELEMENT_SIZE (1 << DMA_ELEMENT_SHIFT) + +#ifndef CONFIG_VIDEO_OMAP1_SG +#define DMA_FRAME_SHIFT (FIFO_SHIFT - 1) +#define MIN_BUF_COUNT 3 +#else +#define DMA_FRAME_SHIFT DMA_BURST_SHIFT +#define MIN_BUF_COUNT 2 +#endif + +#define DMA_FRAME_SIZE (1 << DMA_FRAME_SHIFT) +#define DMA_SYNC OMAP_DMA_SYNC_FRAME +#define THRESHOLD_LEVEL DMA_FRAME_SIZE + + +#define MAX_VIDEO_MEM 4 /* arbitrary video memory limit in MB */ + + +/* + * Structures + */ + +/* buffer for one video frame */ +struct omap1_cam_buf { + struct videobuf_buffer vb; + enum v4l2_mbus_pixelcode code; + int inwork; +#ifdef CONFIG_VIDEO_OMAP1_SG + struct scatterlist *sgbuf; + int sgcount; + int bytes_left; + enum videobuf_state result; +#endif +}; + +struct omap1_cam_dev { + struct soc_camera_host soc_host; + struct soc_camera_device *icd; + struct clk *clk; + + unsigned int irq; + void __iomem *base; + + int dma_ch; + + struct omap1_cam_platform_data *pdata; + struct resource *res; + unsigned long pflags; + unsigned long camexclk; + + struct list_head capture; + + spinlock_t lock; + + /* Pointers to DMA buffers */ + struct omap1_cam_buf *active; + struct omap1_cam_buf *ready;

Well, I must confess, as it stands, you managed to confuse me with your "active" and "ready" buffer management. Is the reason for these two pointers some hardware feature? Maybe that while one buffer is being filled by DMA you can already configure the DMA engine with the next one?

+ + u32 reg_cache[OMAP1_CAMERA_IOSIZE / sizeof(u32)];

Don't think you'd lose much performance without cache, for that the code would become less error-prone.

+}; + + +void cam_write(struct omap1_cam_dev *pcdev, u16 reg, u32 val)

static

+{ + pcdev->reg_cache[reg / sizeof(u32)] = val; + __raw_writel(val, pcdev->base + reg); +} + +int cam_read(struct omap1_cam_dev *pcdev, u16 reg, bool from_cache)

static u32

+{ + return !from_cache ? __raw_readl(pcdev->base + reg) : + pcdev->reg_cache[reg / sizeof(u32)];

hmmm.... it's never simple with caches;) Some registers (e.g., CTRLCLOCK) you read both from the cache and from the hardware directly. Don't you have to update the cache on a direct read?

+} + +#define CAM_READ(pcdev, reg) \ + cam_read(pcdev, REG_##reg, 0)

last parameter is a bool, so, use "false"

+#define CAM_WRITE(pcdev, reg, val) \ + cam_write(pcdev, REG_##reg, val) +#define CAM_READ_CACHE(pcdev, reg) \ + cam_read(pcdev, REG_##reg, 1)

"true"

+ +/* + * Videobuf operations + */ +static int omap1_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); + + if (bytes_per_line < 0) + return bytes_per_line; + + *size = bytes_per_line * icd->user_height; + + if (!*count || *count < MIN_BUF_COUNT) + *count = MIN_BUF_COUNT; + + if (*size * *count > MAX_VIDEO_MEM * 1024 * 1024) + *count = (MAX_VIDEO_MEM * 1024 * 1024) / *size; + + dev_dbg(icd->dev.parent, + "%s: count=%d, size=%d\n", __func__, *count, *size); + + return 0; +} + +static void free_buffer(struct videobuf_queue *vq, struct omap1_cam_buf *buf) +{ + struct videobuf_buffer *vb = &buf->vb; +#ifdef CONFIG_VIDEO_OMAP1_SG + struct videobuf_dmabuf *dma = videobuf_to_dma(vb); +#endif + + BUG_ON(in_interrupt()); + + videobuf_waiton(vb, 0, 0); +#ifndef CONFIG_VIDEO_OMAP1_SG + videobuf_dma_contig_free(vq, vb); +#else + videobuf_dma_unmap(vq, dma); + videobuf_dma_free(dma); +#endif + + vb->state = VIDEOBUF_NEEDS_INIT; +} + +static int omap1_videobuf_prepare(struct videobuf_queue *vq, + struct videobuf_buffer *vb, enum v4l2_field field) +{ + struct soc_camera_device *icd = vq->priv_data; + struct omap1_cam_buf *buf = container_of(vb, struct omap1_cam_buf, vb); + int bytes_per_line = soc_mbus_bytes_per_line(icd->user_width, + icd->current_fmt->host_fmt); + int ret; + + if (bytes_per_line < 0) + return bytes_per_line; + + WARN_ON(!list_empty(&vb->queue)); + + BUG_ON(NULL == icd->current_fmt); + + buf->inwork = 1; + + 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; + } + buf->inwork = 0; + + return 0; +fail: + free_buffer(vq, buf); +out: + buf->inwork = 0; + return ret; +} + +static void set_dma_dest_params(int dma_ch, struct omap1_cam_buf *buf) +{ +#ifndef CONFIG_VIDEO_OMAP1_SG + dma_addr_t dma_addr = videobuf_to_dma_contig(&buf->vb); + unsigned int block_size = buf->vb.size; +#else + dma_addr_t dma_addr; + unsigned int block_size; + + if (WARN_ON(!buf->sgbuf)) { + buf->result = VIDEOBUF_ERROR; + return; + } + dma_addr = sg_dma_address(buf->sgbuf); + if (WARN_ON(!dma_addr)) { + buf->sgbuf = NULL; + buf->result = VIDEOBUF_ERROR; + return; + } + block_size = sg_dma_len(buf->sgbuf); + if (WARN_ON(!block_size)) { + buf->sgbuf = NULL; + buf->result = VIDEOBUF_ERROR; + return; + } + if (unlikely(buf->bytes_left < block_size)) + block_size = buf->bytes_left; + if (WARN_ON(dma_addr & (DMA_FRAME_SIZE * DMA_ELEMENT_SIZE - 1))) { + dma_addr = ALIGN(dma_addr, DMA_FRAME_SIZE * DMA_ELEMENT_SIZE); + block_size &= ~(DMA_FRAME_SIZE * DMA_ELEMENT_SIZE - 1); + } + buf->bytes_left -= block_size; + buf->sgcount++; +#endif + omap_set_dma_dest_params(dma_ch, OMAP_DMA_PORT_EMIFF, + OMAP_DMA_AMODE_POST_INC, dma_addr, 0, 0); + omap_set_dma_transfer_params(dma_ch, OMAP_DMA_DATA_TYPE_S32, + DMA_FRAME_SIZE, block_size >> (DMA_FRAME_SHIFT + + DMA_ELEMENT_SHIFT), DMA_SYNC, 0, 0); +} + +#ifdef CONFIG_VIDEO_OMAP1_SG +static struct scatterlist *try_next_sgbuf(int dma_ch, struct omap1_cam_buf *buf) +{ + struct scatterlist *sgbuf; + + if (likely(buf->sgbuf)) { + + if (unlikely(!buf->bytes_left)) { + /* indicate sglist complete */ + sgbuf = NULL; + } else { + sgbuf = sg_next(buf->sgbuf); + if (WARN_ON(!sgbuf)) { + buf->result = VIDEOBUF_ERROR; + } else if (WARN_ON(!sg_dma_len(sgbuf))) { + sgbuf = NULL; + buf->result = VIDEOBUF_ERROR; + } + } + buf->sgbuf = sgbuf; + } else { + struct videobuf_dmabuf *dma = videobuf_to_dma(&buf->vb); + + buf->sgbuf = sgbuf = dma->sglist; + if (sgbuf) { + buf->sgcount = 0; + buf->bytes_left = buf->vb.size; + buf->result = VIDEOBUF_DONE; + } + } + if (sgbuf) + set_dma_dest_params(dma_ch, buf); + + return sgbuf; +} +#endif + +static struct omap1_cam_buf *prepare_next_vb(struct omap1_cam_dev *pcdev) +{ + struct omap1_cam_buf *buf; + + buf = pcdev->ready; + if (!buf) { + if (list_empty(&pcdev->capture)) + return buf; + buf = list_entry(pcdev->capture.next, + struct omap1_cam_buf, vb.queue); + buf->vb.state = VIDEOBUF_ACTIVE; + pcdev->ready = buf; + list_del_init(&buf->vb.queue); + } +#ifndef CONFIG_VIDEO_OMAP1_SG + set_dma_dest_params(pcdev->dma_ch, buf); +#else + buf->sgbuf = NULL; +#endif + return buf; +} + +static void start_capture(struct omap1_cam_dev *pcdev) +{ + struct omap1_cam_buf *buf = pcdev->active; + unsigned long ctrlclock = CAM_READ_CACHE(pcdev, CTRLCLOCK); + unsigned long mode = CAM_READ_CACHE(pcdev, MODE);

I would make all variables, that are used specifically for various registers u32.

+ + if (WARN_ON(!buf)) + return; + +#ifndef CONFIG_VIDEO_OMAP1_SG + /* don't enable end of frame interrupts before capture autostart */ + mode &= ~EN_V_DOWN; +#endif + if (WARN_ON(mode & RAZ_FIFO)) + /* clean up possibly insane reset condition */ + CAM_WRITE(pcdev, MODE, mode &= ~RAZ_FIFO);

It is usually better to avoid assigning variables inside other constructs, please split into two lines. Besides, in omap1_cam_probe() you initialise MODE to "THRESHOLD_LEVEL << THRESHOLD_SHIFT," i.e., RAZ_FIFO is cleared. So, don't think you need that test at all.

+ + if (unlikely(ctrlclock & LCLK_EN)) + /* stop pixel clock before FIFO reset */ + CAM_WRITE(pcdev, CTRLCLOCK, ctrlclock & ~LCLK_EN); + /* reset FIFO */ + CAM_WRITE(pcdev, MODE, mode | RAZ_FIFO); + + omap_start_dma(pcdev->dma_ch); + + /* (re)enable pixel clock */ + CAM_WRITE(pcdev, CTRLCLOCK, ctrlclock | LCLK_EN); + /* release FIFO reset */ + CAM_WRITE(pcdev, MODE, mode); + +#ifdef CONFIG_VIDEO_OMAP1_SG + try_next_sgbuf(pcdev->dma_ch, buf); +#endif +} + +static void suspend_capture(struct omap1_cam_dev *pcdev) +{ + unsigned long ctrlclock = CAM_READ_CACHE(pcdev, CTRLCLOCK);

u32

+ + CAM_WRITE(pcdev, CTRLCLOCK, ctrlclock & ~LCLK_EN); + omap_stop_dma(pcdev->dma_ch); +} + +static void disable_capture(struct omap1_cam_dev *pcdev) +{ + unsigned long mode = CAM_READ_CACHE(pcdev, MODE);

u32 (etc.)

+ + CAM_WRITE(pcdev, MODE, mode & ~(IRQ_MASK | DMA)); +} + +static void omap1_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 omap1_cam_dev *pcdev = ici->priv; + struct omap1_cam_buf *buf; + unsigned long mode; + + list_add_tail(&vb->queue, &pcdev->capture); + vb->state = VIDEOBUF_QUEUED; + + if (pcdev->active) + return; + + WARN_ON(pcdev->ready); + + buf = prepare_next_vb(pcdev); + if (WARN_ON(!buf)) + return; + + pcdev->active = buf; + pcdev->ready = NULL; + + dev_dbg(icd->dev.parent, "%s: capture not active, setup FIFO, start DMA" + "\n", __func__);

No need to split "\n" to a new line, don't worry about > 80 characters...

+ mode = CAM_READ_CACHE(pcdev, MODE) & (THRESHOLD_MASK | ORDERCAMD); + mode |= EN_FIFO_FULL | DMA; +#ifndef CONFIG_VIDEO_OMAP1_SG + CAM_WRITE(pcdev, MODE, mode | EN_V_UP); +#else + CAM_WRITE(pcdev, MODE, mode | EN_V_DOWN); + + try_next_sgbuf(pcdev->dma_ch, buf); +#endif + start_capture(pcdev); +} + +static void omap1_videobuf_release(struct videobuf_queue *vq, + struct videobuf_buffer *vb) +{ + struct omap1_cam_buf *buf = + container_of(vb, struct omap1_cam_buf, vb); + struct soc_camera_device *icd = vq->priv_data; + struct device *dev = icd->dev.parent; + + switch (vb->state) { + case VIDEOBUF_DONE: + dev_dbg(dev, "%s (done)\n", __func__); + break; + case VIDEOBUF_ACTIVE: + dev_dbg(dev, "%s (active)\n", __func__); + break; + case VIDEOBUF_QUEUED: + dev_dbg(dev, "%s (queued)\n", __func__); + break; + case VIDEOBUF_PREPARED: + dev_dbg(dev, "%s (prepared)\n", __func__); + break; + default: + dev_dbg(dev, "%s (unknown)\n", __func__); + break; + } + + free_buffer(vq, buf); +} + +static void videobuf_done(struct omap1_cam_dev *pcdev, + enum videobuf_state result) +{ + struct omap1_cam_buf *buf = pcdev->active; + struct videobuf_buffer *vb; + struct device *dev = pcdev->icd->dev.parent; + + if (WARN_ON(!buf)) { + suspend_capture(pcdev); + disable_capture(pcdev); + return; + } + + if (result == VIDEOBUF_ERROR) + suspend_capture(pcdev); + + vb = &buf->vb; + if (waitqueue_active(&vb->done)) {

I'm not sure this is a good idea. Why are you reusing the buffer, if noone is waiting for it _now_? It can well be, that the task is doing something else atm. Like processing the previous frame. Or it has been preempted by another process, before it called poll() or select() on your device?

+ if (!pcdev->ready && result != VIDEOBUF_ERROR) + suspend_capture(pcdev); + vb->state = result; + do_gettimeofday(&vb->ts); + vb->field_count++;

I don't think you're supposed to increment field_count if you've got a VIDEOBUF_ERROR.

+ wake_up(&vb->done); + + pcdev->active = buf = pcdev->ready; + pcdev->ready = NULL; + + if (!buf) { + result = VIDEOBUF_ERROR; + prepare_next_vb(pcdev); + + pcdev->active = buf = pcdev->ready; + pcdev->ready = NULL; + } + } else if (pcdev->ready) { + dev_dbg(dev, "%s: nobody waiting on videobuf, swap with next\n", + __func__);

Not sure this is a wise decision... If there are more buffers in the queue, they stay there waiting. Best remove this waitqueue_active() check entirely, just complete buffers and wake up the user regardless. If no more buffers - go to sleep.

+ pcdev->active = pcdev->ready; +#ifdef CONFIG_VIDEO_OMAP1_SG + buf->sgbuf = NULL; +#endif + pcdev->ready = buf; + + buf = pcdev->active; + } else { +#ifndef CONFIG_VIDEO_OMAP1_SG + dev_dbg(dev, "%s: nobody waiting on videobuf, reuse it\n", + __func__); +#else + if (result != VIDEOBUF_ERROR) { + suspend_capture(pcdev); + result = VIDEOBUF_ERROR; + } + prepare_next_vb(pcdev); +#endif + } + + if (!buf) { + dev_dbg(dev, "%s: no more videobufs, stop capture\n", __func__); + disable_capture(pcdev); + return; + } + +#ifdef CONFIG_VIDEO_OMAP1_SG + if (result == VIDEOBUF_ERROR) + buf->sgbuf = NULL; + + try_next_sgbuf(pcdev->dma_ch, buf); +#endif + + if (result == VIDEOBUF_ERROR) { + dev_dbg(dev, "%s: videobuf error; reset FIFO, restart DMA\n", + __func__); + start_capture(pcdev); + }

This all doesn't look quite right, but I'll wait for documentation, before commenting.

+ + prepare_next_vb(pcdev); +} + +static void dma_isr(int channel, unsigned short status, void *data) +{ + struct omap1_cam_dev *pcdev = data; + struct omap1_cam_buf *buf = pcdev->active; + enum videobuf_state result; + unsigned long flags; + + spin_lock_irqsave(&pcdev->lock, flags); + + if (WARN_ON(!buf)) { + suspend_capture(pcdev); + disable_capture(pcdev); + goto out; + } + +#ifndef CONFIG_VIDEO_OMAP1_SG + /* videobuf complete, disable end of frame interrupt for this frame */ + CAM_WRITE(pcdev, MODE, CAM_READ_CACHE(pcdev, MODE) & ~EN_V_DOWN); + result = VIDEOBUF_DONE; +#else + if (buf->sgbuf) { + /* current sglist not complete yet */ + try_next_sgbuf(pcdev->dma_ch, buf); + if (buf->sgbuf) + goto out; + + if (buf->result != VIDEOBUF_ERROR) { + buf = prepare_next_vb(pcdev); + if (!buf) + goto out; + + try_next_sgbuf(pcdev->dma_ch, buf); + goto out; + } + } + /* end of videobuf */ + result = buf->result; +#endif + videobuf_done(pcdev, result); +out: + spin_unlock_irqrestore(&pcdev->lock, flags); +} + +static irqreturn_t cam_isr(int irq, void *data) +{ + struct omap1_cam_dev *pcdev = data; + struct device *dev = pcdev->icd->dev.parent; + struct omap1_cam_buf *buf = pcdev->active; + unsigned long it_status; + unsigned long flags; + + it_status = CAM_READ(pcdev, IT_STATUS); + if (!it_status) + return IRQ_NONE; + + spin_lock_irqsave(&pcdev->lock, flags); + + if (WARN_ON(!buf)) { + suspend_capture(pcdev); + disable_capture(pcdev); + goto out; + } + + if (unlikely(it_status & FIFO_FULL)) { + dev_warn(dev, "%s: FIFO overflow\n", __func__); + + } else if (it_status & V_DOWN) { +#ifdef CONFIG_VIDEO_OMAP1_SG + /* + * if exactly 2 sgbufs of the next sglist has be used, + * then we are in sync + */

You mean the hardware is supposed to interrupt you after 2 buffers?

+ if (buf && buf->sgcount == 2) + goto out; +#endif + dev_notice(dev, "%s: unexpected end of video frame\n", + __func__); + +#ifndef CONFIG_VIDEO_OMAP1_SG + } else if (it_status & V_UP) { + unsigned long mode = CAM_READ_CACHE(pcdev, MODE); + + if (!(mode & EN_V_DOWN)) { + /* enable end of frame interrupt for current videobuf */ + CAM_WRITE(pcdev, MODE, mode | EN_V_DOWN); + } + goto out; +#endif + + } else { + dev_warn(pcdev->soc_host.v4l2_dev.dev, "%s: " + "unhandled camera interrupt, status == 0x%lx\n", + __func__, it_status); + goto out; + } + + videobuf_done(pcdev, VIDEOBUF_ERROR); +out: + spin_unlock_irqrestore(&pcdev->lock, flags); + return IRQ_HANDLED; +} + +static struct videobuf_queue_ops omap1_videobuf_ops = { + .buf_setup = omap1_videobuf_setup, + .buf_prepare = omap1_videobuf_prepare, + .buf_queue = omap1_videobuf_queue, + .buf_release = omap1_videobuf_release, +}; + + +/* + * SOC Camera host operations + */ + +static void sensor_reset(struct omap1_cam_dev *pcdev, bool reset) +{ + /* apply/release camera sensor reset if requested by platform data */ + if (pcdev->pflags & OMAP1_CAMERA_RST_HIGH) + CAM_WRITE(pcdev, GPIO, reset); + else if (pcdev->pflags & OMAP1_CAMERA_RST_LOW) + CAM_WRITE(pcdev, GPIO, !reset); +} + +/* + * The following two functions absolutely depend on the fact, that + * there can be only one camera on OMAP1 camera sensor interface + */ +static int omap1_cam_add_device(struct soc_camera_device *icd) +{ + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent); + struct omap1_cam_dev *pcdev = ici->priv; + unsigned int ctrlclock;

u32

+ int ret = 0; + + if (pcdev->icd) { + ret = -EBUSY; + goto ebusy; + } + + clk_enable(pcdev->clk); + + /* setup sensor clock */ + ctrlclock = CAM_READ(pcdev, CTRLCLOCK); + ctrlclock &= ~(CAMEXCLK_EN | MCLK_EN | DPLL_EN); + CAM_WRITE(pcdev, CTRLCLOCK, ctrlclock); + + ctrlclock &= ~FOSCMOD_MASK; + switch (pcdev->camexclk) { + case CAM_EXCLK_6MHz:

just use constants for these

+ ctrlclock |= CAMEXCLK_EN | FOSCMOD_6MHz; + break; + case CAM_EXCLK_8MHz: + ctrlclock |= CAMEXCLK_EN | FOSCMOD_8MHz | DPLL_EN; + break; + case CAM_EXCLK_9_6MHz: + ctrlclock |= CAMEXCLK_EN | FOSCMOD_9_6MHz | DPLL_EN; + break; + case CAM_EXCLK_12MHz: + ctrlclock |= CAMEXCLK_EN | FOSCMOD_12MHz; + break; + case CAM_EXCLK_24MHz: + ctrlclock |= CAMEXCLK_EN | FOSCMOD_24MHz | DPLL_EN; + default:

This is the case of "Not providing sensor clock," do you still have to enable the clock at all below (MCLK_EN)?

+ break; + } + CAM_WRITE(pcdev, CTRLCLOCK, ctrlclock & ~DPLL_EN); + + /* enable clock */ + ctrlclock |= MCLK_EN; + CAM_WRITE(pcdev, CTRLCLOCK, ctrlclock); + + sensor_reset(pcdev, 0);

use "false" for the second parameter

+ + pcdev->icd = icd; + + dev_info(icd->dev.parent, "OMAP1 Camera driver attached to camera %d\n", + icd->devnum); +ebusy: + return ret; +} + +static void omap1_cam_remove_device(struct soc_camera_device *icd) +{ + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent); + struct omap1_cam_dev *pcdev = ici->priv; + unsigned long ctrlclock; + + BUG_ON(icd != pcdev->icd); + + suspend_capture(pcdev); + disable_capture(pcdev); + + sensor_reset(pcdev, 1); + + /* disable and release system clocks */ + ctrlclock = CAM_READ(pcdev, CTRLCLOCK); + ctrlclock &= ~(MCLK_EN | DPLL_EN | CAMEXCLK_EN); + CAM_WRITE(pcdev, CTRLCLOCK, ctrlclock); + + ctrlclock = (ctrlclock & ~FOSCMOD_MASK) | FOSCMOD_12MHz; + CAM_WRITE(pcdev, CTRLCLOCK, ctrlclock); + CAM_WRITE(pcdev, CTRLCLOCK, ctrlclock | MCLK_EN); + + CAM_WRITE(pcdev, CTRLCLOCK, ctrlclock & ~MCLK_EN); + + clk_disable(pcdev->clk); + + pcdev->icd = NULL; + + dev_info(icd->dev.parent, "OMAP1 Camera driver detached from camera %d" + "\n", icd->devnum); +} + +/* Duplicate standard formats based on host capability of byte swapping */ +static const struct soc_mbus_pixelfmt omap1_cam_formats[] = { + [V4L2_MBUS_FMT_YUYV8_2X8_BE] = {

You'll have to adjust these to the new names. Basically, for all YUYV8 and YVYU8 codes, "_LE" just gets dropped, and for "_BE" you swap chrominance-luminance pairs, i.e., UYVY8_2X8 instead of YUYV8_2X8_BE and VYUY8_2X8 instead of YVYU8_2X8_BE.

+ .fourcc = V4L2_PIX_FMT_YUYV, + .name = "YUYV", + .bits_per_sample = 8, + .packing = SOC_MBUS_PACKING_2X8_PADHI, + .order = SOC_MBUS_ORDER_BE, + }, + [V4L2_MBUS_FMT_YVYU8_2X8_BE] = { + .fourcc = V4L2_PIX_FMT_YVYU, + .name = "YVYU", + .bits_per_sample = 8, + .packing = SOC_MBUS_PACKING_2X8_PADHI, + .order = SOC_MBUS_ORDER_BE, + }, + [V4L2_MBUS_FMT_YUYV8_2X8_LE] = { + .fourcc = V4L2_PIX_FMT_UYVY, + .name = "UYVY", + .bits_per_sample = 8, + .packing = SOC_MBUS_PACKING_2X8_PADHI, + .order = SOC_MBUS_ORDER_BE, + }, + [V4L2_MBUS_FMT_YVYU8_2X8_LE] = { + .fourcc = V4L2_PIX_FMT_VYUY, + .name = "VYUY", + .bits_per_sample = 8, + .packing = SOC_MBUS_PACKING_2X8_PADHI, + .order = SOC_MBUS_ORDER_BE, + }, + [V4L2_MBUS_FMT_RGB555_2X8_PADHI_BE] = { + .fourcc = V4L2_PIX_FMT_RGB555, + .name = "RGB555", + .bits_per_sample = 8, + .packing = SOC_MBUS_PACKING_2X8_PADHI, + .order = SOC_MBUS_ORDER_BE, + }, + [V4L2_MBUS_FMT_RGB555_2X8_PADHI_LE] = { + .fourcc = V4L2_PIX_FMT_RGB555X, + .name = "RGB555X", + .bits_per_sample = 8, + .packing = SOC_MBUS_PACKING_2X8_PADHI, + .order = SOC_MBUS_ORDER_BE, + }, + [V4L2_MBUS_FMT_RGB565_2X8_BE] = { + .fourcc = V4L2_PIX_FMT_RGB565, + .name = "RGB565", + .bits_per_sample = 8, + .packing = SOC_MBUS_PACKING_2X8_PADHI, + .order = SOC_MBUS_ORDER_BE, + }, + [V4L2_MBUS_FMT_RGB565_2X8_LE] = { + .fourcc = V4L2_PIX_FMT_RGB565X, + .name = "RGB565X", + .bits_per_sample = 8, + .packing = SOC_MBUS_PACKING_2X8_PADHI, + .order = SOC_MBUS_ORDER_BE, + }, +}; + +static int omap1_cam_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; + enum v4l2_mbus_pixelcode code; + 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(dev, "%s: invalid format code #%d: %d\n", __func__, + idx, code); + return 0; + } + + /* Check support for the requested bits-per-sample */ + if (fmt->bits_per_sample != 8) + return 0; + + switch (code) { + case V4L2_MBUS_FMT_YUYV8_2X8_BE: + case V4L2_MBUS_FMT_YVYU8_2X8_BE: + case V4L2_MBUS_FMT_YUYV8_2X8_LE: + case V4L2_MBUS_FMT_YVYU8_2X8_LE: + case V4L2_MBUS_FMT_RGB555_2X8_PADHI_BE: + case V4L2_MBUS_FMT_RGB555_2X8_PADHI_LE: + case V4L2_MBUS_FMT_RGB565_2X8_BE: + case V4L2_MBUS_FMT_RGB565_2X8_LE: + formats++; + if (xlate) { + xlate->host_fmt = &omap1_cam_formats[code]; + xlate->code = code; + xlate++; + dev_dbg(dev, "%s: providing format %s " + "as byte swapped code #%d\n", __func__, + omap1_cam_formats[code].name, code); + } + default: + if (xlate) + dev_dbg(dev, "%s: providing format %s " + "in pass-through mode\n", __func__, + fmt->name); + } + formats++; + if (xlate) { + xlate->host_fmt = fmt; + xlate->code = code; + xlate++; + } + + return formats; +} + +static int is_dma_aligned(s32 bytes_per_line, unsigned int height)

make it a bool

+{ + int size = bytes_per_line * height; + + return IS_ALIGNED(bytes_per_line, DMA_ELEMENT_SIZE) && + IS_ALIGNED(size, DMA_FRAME_SIZE * DMA_ELEMENT_SIZE); +} + +static int dma_align(int *width, int *height, + const struct soc_mbus_pixelfmt *fmt, bool enlarge) +{ + s32 bytes_per_line = soc_mbus_bytes_per_line(*width, fmt); + + if (bytes_per_line < 0) + return bytes_per_line; + + if (!is_dma_aligned(bytes_per_line, *height)) { + unsigned int pxalign = __fls(bytes_per_line / *width); + unsigned int salign = + DMA_FRAME_SHIFT + DMA_ELEMENT_SHIFT - pxalign; + unsigned int incr = enlarge << salign; + + v4l_bound_align_image(width, DMA_ELEMENT_SIZE >> pxalign, + *width + incr, DMA_ELEMENT_SHIFT - pxalign,

The above means, that lines must consist of an integer number of DMA elements, right? Is this really necessary?

+ height, 1, *height + incr, 0, salign); + return 0; + } + return 1; +} + +/* returns 1 on g_crop() success, 0 on cropcap() success, <0 on error */ +static int get_crop(struct soc_camera_device *icd, struct v4l2_rect *rect) +{ + struct v4l2_subdev *sd = soc_camera_to_subdev(icd); + struct device *dev = icd->dev.parent; + struct v4l2_crop crop; + int ret; + + crop.type = V4L2_BUF_TYPE_VIDEO_CAPTURE; + ret = v4l2_subdev_call(sd, video, g_crop, &crop); + if (ret == -ENOIOCTLCMD) { + struct v4l2_cropcap cc; + + dev_dbg(dev, "%s: g_crop() missing, trying cropcap() instead" + "\n", __func__);

Put "\n" back in the string

+ cc.type = V4L2_BUF_TYPE_VIDEO_CAPTURE; + ret = v4l2_subdev_call(sd, video, cropcap, &cc); + if (ret < 0) + return ret; + *rect = cc.defrect; + return 0; + } else if (ret < 0) { + return ret; + } + *rect = crop.c;

superfluous space

+ return 1; +} + +/* + * returns 1 on g_mbus_fmt() or g_crop() success, 0 on cropcap() success, + * <0 on error + */ +static int get_geometry(struct soc_camera_device *icd, struct v4l2_rect *rect, + enum v4l2_mbus_pixelcode code) +{ + struct v4l2_subdev *sd = soc_camera_to_subdev(icd); + struct device *dev = icd->dev.parent; + struct v4l2_mbus_framefmt mf; + int ret; + + ret = v4l2_subdev_call(sd, video, g_mbus_fmt, &mf); + if (ret == -ENOIOCTLCMD) { + struct v4l2_rect c; + + dev_dbg(dev, "%s: g_mbus_fmt() missing, trying g_crop() instead" + "\n", __func__);

Put "\n" back in the string

+ ret = get_crop(icd, &c); + if (ret < 0) + return ret; + /* REVISIT: + * Should cropcap() obtained defrect reflect last s_crop()? + * Can we use it here for s_crop() result verification? + */

multiline comment style

+ if (ret) { + *rect = c; /* use g_crop() result */ + } else { + dev_warn(dev, "%s: current geometry not available\n", + __func__); + return 0; + } + } else if (ret < 0) { + return ret; + } else if (mf.code != code) { + return -EINVAL; + } else { + rect->width = mf.width; + rect->height = mf.height; + } + return 1; +} + +#define subdev_call_with_sense(ret, function, args...) \ +{ \ + struct soc_camera_sense sense = { \ + .master_clock = pcdev->camexclk, \ + .pixel_clock_max = 0, \ + }; \ +\ + if (pcdev->pdata) \ + sense.pixel_clock_max = pcdev->pdata->lclk_khz_max * 1000; \ + icd->sense = &sense; \ + *(ret) = v4l2_subdev_call(sd, video, function, ##args); \ + icd->sense = NULL; \ +\ + if (sense.flags & SOCAM_SENSE_PCLK_CHANGED) { \ + if (sense.pixel_clock > sense.pixel_clock_max) { \ + dev_err(dev, "%s: " \ + "pixel clock %lu set by the camera too high!" \ + "\n", __func__, sense.pixel_clock); \

"\n" back, please

+ return -EIO; \ + } \ + } \ +}

Ok, I'd prefer to follow some good-style rules here too. (1) please align your backslashes with TABs. (2) please, don't use implicitly passed variables, in this case "sd," "pcdev," "dev," and "icd." (3) since this is a macro, you don't need to pass ret as a pointer. (4) don't "return" from a macro - in your case it's enough to "ret = -EIO." Personally, I wouldn't worry about adding 4 more parameters to the macro. I would also remove "ret" from parameters and "return" it from the macro per #define subdev_call_with_sense(pcdev, dev, icd, sd, function, args...) \ ({ \ int __ret; \ ... \ __ret = v4l2_subdev_call(sd, video, function, ##args); \ ... \ __ret = -EINVAL; \ ... \ __ret; \ })

+ +static int omap1_cam_set_crop(struct soc_camera_device *icd, + struct v4l2_crop *crop) +{ + struct v4l2_rect *rect = &crop->c; + struct v4l2_subdev *sd = soc_camera_to_subdev(icd); + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent); + struct omap1_cam_dev *pcdev = ici->priv; + struct device *dev = icd->dev.parent; + s32 bytes_per_line; + int ret; + + ret = dma_align(&rect->width, &rect->height, icd->current_fmt->host_fmt, + false); + if (ret < 0) { + dev_err(dev, "%s: failed to align %ux%u %s with DMA\n", + __func__, rect->width, rect->height, + icd->current_fmt->host_fmt->name); + return ret; + } + + subdev_call_with_sense(&ret, s_crop, crop); + if (ret < 0) { + dev_warn(dev, "%s: failed to crop to %ux%u@%u:%u\n", __func__, + rect->width, rect->height, rect->left, rect->top); + return ret; + } + + ret = get_geometry(icd, rect, icd->current_fmt->code); + if (ret < 0) { + dev_err(dev, "%s: get_geometry() failed\n", __func__); + return ret; + } + if (!ret) { + dev_warn(dev, "%s: unable to verify s_crop() results\n", + __func__); + }

Superfluous braces

+ + bytes_per_line = soc_mbus_bytes_per_line(rect->width, + icd->current_fmt->host_fmt); + if (bytes_per_line < 0) { + dev_err(dev, "%s: soc_mbus_bytes_per_line() failed\n", + __func__); + return bytes_per_line; + } + + ret = is_dma_aligned(bytes_per_line, rect->height); + if (ret < 0) {

Doesn't look like this is possible.

+ dev_err(dev, "%s: is_dma_aligned() failed\n", __func__); + return ret; + } + if (!ret) { + dev_err(dev, "%s: resulting geometry %dx%d not DMA aligned\n", + __func__, rect->width, rect->height); + return -EINVAL; + } + + icd->user_width = rect->width; + icd->user_height = rect->height;

No, this is wrong. user_width and user_height are _user_ sizes, i.e., a result sensor cropping and scaling and host cropping and scaling. Whereas set_crop sets sensor input rectangle.

+ + return ret; +} + +static int omap1_cam_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 device *dev = icd->dev.parent; + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent); + struct omap1_cam_dev *pcdev = ici->priv; + struct v4l2_pix_format *pix = &f->fmt.pix; + struct v4l2_mbus_framefmt mf; + struct v4l2_crop crop; + struct v4l2_rect *rect = &crop.c; + int bytes_per_line; + int ret; + + xlate = soc_camera_xlate_by_fourcc(icd, pix->pixelformat); + if (!xlate) { + dev_warn(dev, "%s: format %x not found\n", __func__, + pix->pixelformat); + return -EINVAL; + } + + bytes_per_line = soc_mbus_bytes_per_line(pix->width, xlate->host_fmt); + if (bytes_per_line < 0) { + dev_err(dev, "%s: soc_mbus_bytes_per_line() failed\n", + __func__); + return bytes_per_line; + } + if (pix->bytesperline && pix->bytesperline != bytes_per_line) { + dev_err(dev, "%s: bytes per line mismatch\n", __func__); + return -EINVAL; + } + ret = is_dma_aligned(bytes_per_line, pix->height); + if (ret < 0) {

ditto - that's just a bool.

+ dev_err(dev, "%s: is_dma_aligned() failed\n", __func__); + return ret; + } + if (!ret) { + dev_err(dev, "%s: image size %dx%d not DMA aligned\n", + __func__, pix->width, pix->height); + return -EINVAL; + } + + mf.width = pix->width; + mf.height = pix->height; + mf.field = pix->field; + mf.colorspace = pix->colorspace; + mf.code = xlate->code; + + subdev_call_with_sense(&ret, s_mbus_fmt, &mf); + if (ret < 0) { + dev_err(dev, "%s: failed to set format\n", __func__); + return ret; + } + + if (mf.code != xlate->code) { + dev_err(dev, "%s: unexpected pixel code change\n", __func__); + return -EINVAL; + } + icd->current_fmt = xlate; + + pix->field = mf.field; + pix->colorspace = mf.colorspace; + + if (mf.width == pix->width && mf.height == pix->height) + return 0; + + dev_notice(dev, "%s: sensor geometry differs, trying to crop to %dx%d" + "\n", __func__, pix->width, pix->height);

No, you shouldn't do this. It is normal, when S_FMT sets a format different from what the user has requested. Just leave it as is, only adjust if you cannot handle it or fail.

+ crop.type = V4L2_BUF_TYPE_VIDEO_CAPTURE; + ret = get_crop(icd, rect); + if (ret < 0) { + dev_err(dev, "%s: get_crop() failed\n", __func__); + return ret; + } + + rect->width = pix->width; + rect->height = pix->height; + + subdev_call_with_sense(&ret, s_crop, &crop); + if (ret < 0) { + dev_warn(dev, "%s: failed to crop to %ux%u@%u:%u\n", __func__, + rect->width, rect->height, rect->left, rect->top); + return ret; + } + + ret = get_geometry(icd, rect, xlate->code); + if (ret < 0) { + dev_err(dev, "%s: get_geometry() failed\n", __func__); + return ret; + } + + if (!ret) { + dev_warn(dev, "%s: s_crop() results not verified\n", __func__); + return 0; + } + + if (pix->width != rect->width || pix->height != rect->height) { + dev_err(dev, "%s: tried to set %dx%d, got %dx%d\n", __func__, + pix->width, pix->height, + rect->width, rect->height); + return -EINVAL; + }

No, that's not an error. Just use whatever you managed to configure.

+ return 0; +} + +static int omap1_cam_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 device *dev = icd->dev.parent; + struct v4l2_pix_format *pix = &f->fmt.pix; + struct v4l2_mbus_framefmt mf, testmf; + const struct soc_mbus_pixelfmt *fmt; + int ret; + + xlate = soc_camera_xlate_by_fourcc(icd, pix->pixelformat); + if (!xlate) { + dev_warn(dev, "%s: format %x not found\n", __func__, + pix->pixelformat); + return -EINVAL; + } + + fmt = xlate->host_fmt; + ret = dma_align(&pix->width, &pix->height, fmt, true); + if (ret < 0) { + dev_err(dev, "%s: failed to align %ux%u %s with DMA\n", + __func__, pix->width, pix->height, fmt->name); + return ret; + } + + mf.width = pix->width; + mf.height = pix->height; + mf.field = pix->field; + mf.colorspace = pix->colorspace; + mf.code = xlate->code; + + /* limit to sensor capabilities */ + ret = v4l2_subdev_call(sd, video, try_mbus_fmt, &mf); + if (ret < 0) { + dev_err(dev, "%s: try_mbus_fmt() failed\n", __func__); + return ret; + } + + pix->field = mf.field; + pix->colorspace = mf.colorspace; + + if (mf.width == pix->width && mf.height == pix->height && + mf.code == xlate->code) + return 0; + + dev_dbg(dev, "%s: geometry changed, recheck alignment\n", __func__); + pix->width = mf.width; + pix->height = mf.height;

This is what the sensor has returned, you put it back into the user frmat.

+ + fmt = soc_mbus_get_fmtdesc(mf.code); + ret = dma_align(&pix->width, &pix->height, fmt, false);

And you DMA-align those sizes.

+ if (ret < 0) { + dev_err(dev, "%s: failed to align %ux%u %s with DMA\n", + __func__, pix->width, pix->height, fmt->name); + return ret; + } + if (ret) + return 0; + + testmf.width = pix->width; + testmf.height = pix->height;

And use them again for try_fmt

+ testmf.field = mf.field; + testmf.colorspace = mf.colorspace; + testmf.code = mf.code; + + ret = v4l2_subdev_call(sd, video, try_mbus_fmt, &testmf); + if (ret < 0) { + dev_err(dev, "%s: try_mbus_fmt() failed\n", __func__); + return ret; + } + + if (testmf.code != mf.code || testmf.width != mf.width || + testmf.height != mf.height) { + dev_err(dev, "%s: sensor format inconsistency, giving up\n", + __func__); + return -EINVAL;

As a general rule, try_fmt shouldn't fail... Only in very weird cases, really.

+ }

And you verified, the sensor kept them.

+ dev_notice(dev, "%s: " + "sensor frame not DMA aligned, will try to crop from set_fmt()"

Hm? You did align them above? /me confused...

+ "\n", __func__); + + return 0; +} + +static void omap1_cam_init_videobuf(struct videobuf_queue *q, + struct soc_camera_device *icd) +{ + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent); + struct omap1_cam_dev *pcdev = ici->priv; + +#ifndef CONFIG_VIDEO_OMAP1_SG + videobuf_queue_dma_contig_init(q, &omap1_videobuf_ops, icd->dev.parent, + &pcdev->lock, + V4L2_BUF_TYPE_VIDEO_CAPTURE, + V4L2_FIELD_NONE, + sizeof(struct omap1_cam_buf), + icd); +#else + videobuf_queue_sg_init(q, &omap1_videobuf_ops, icd->dev.parent, + &pcdev->lock, + V4L2_BUF_TYPE_VIDEO_CAPTURE, + V4L2_FIELD_NONE, + sizeof(struct omap1_cam_buf), + icd); +#endif +} + +static int omap1_cam_reqbufs(struct soc_camera_file *icf, + struct v4l2_requestbuffers *p) +{ + int i; + + /* + * This is for locking debugging only. I removed spinlocks and now I + * check whether .prepare is ever called on a linked buffer, or whether + * a dma IRQ can occur for an in-work or unlinked buffer. Until now + * it hadn't triggered + */ + for (i = 0; i < p->count; i++) { + struct omap1_cam_buf *buf = container_of(icf->vb_vidq.bufs[i], + struct omap1_cam_buf, vb); + buf->inwork = 0; + INIT_LIST_HEAD(&buf->vb.queue); + } + + return 0; +} + +static int omap1_cam_querycap(struct soc_camera_host *ici, + struct v4l2_capability *cap) +{ + /* cap->name is set by the friendly caller:-> */ + strlcpy(cap->card, "OMAP1 Camera", sizeof(cap->card)); + cap->version = VERSION_CODE; + cap->capabilities = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING; + + return 0; +} + +static int omap1_cam_set_bus_param(struct soc_camera_device *icd, + __u32 pixfmt) +{ + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent); + struct omap1_cam_dev *pcdev = ici->priv; + struct device *dev = icd->dev.parent; + const struct soc_camera_format_xlate *xlate; + const struct soc_mbus_pixelfmt *fmt; + unsigned long camera_flags, common_flags; + unsigned int ctrlclock, mode; + int ret; + + camera_flags = icd->ops->query_bus_param(icd); + + common_flags = soc_camera_bus_param_compatible(camera_flags, + SOCAM_BUS_FLAGS); + if (!common_flags) + return -EINVAL; + + /* Make choices, possibly based on platform configuration */ + if ((common_flags & SOCAM_PCLK_SAMPLE_RISING) && + (common_flags & SOCAM_PCLK_SAMPLE_FALLING)) { + if (!pcdev->pdata || + pcdev->pdata->flags & OMAP1_CAMERA_LCLK_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; + + ctrlclock = CAM_READ_CACHE(pcdev, CTRLCLOCK); + if (ctrlclock & LCLK_EN) + CAM_WRITE(pcdev, CTRLCLOCK, ctrlclock & ~LCLK_EN); + + if (common_flags & SOCAM_PCLK_SAMPLE_RISING) { + dev_dbg(dev, "CTRLCLOCK_REG |= POLCLK\n"); + ctrlclock |= POLCLK; + } else { + dev_dbg(dev, "CTRLCLOCK_REG &= ~POLCLK\n"); + ctrlclock &= ~POLCLK; + } + CAM_WRITE(pcdev, CTRLCLOCK, ctrlclock & ~LCLK_EN); + + if (ctrlclock & LCLK_EN) + CAM_WRITE(pcdev, CTRLCLOCK, ctrlclock); + + /* select bus endianess */ + xlate = soc_camera_xlate_by_fourcc(icd, pixfmt); + fmt = xlate->host_fmt; + + if (fmt->order == SOC_MBUS_ORDER_LE) { + dev_dbg(dev, "MODE_REG &= ~ORDERCAMD\n"); + mode = CAM_READ(pcdev, MODE) & ~ORDERCAMD; + } else { + dev_dbg(dev, "MODE_REG |= ORDERCAMD\n"); + mode = CAM_READ(pcdev, MODE) | ORDERCAMD; + } + CAM_WRITE(pcdev, MODE, mode); + + return 0; +} + +static unsigned int omap1_cam_poll(struct file *file, poll_table *pt) +{ + struct soc_camera_file *icf = file->private_data; + struct omap1_cam_buf *buf; + + buf = list_entry(icf->vb_vidq.stream.next, struct omap1_cam_buf, + vb.stream); + + poll_wait(file, &buf->vb.done, pt); + + if (buf->vb.state == VIDEOBUF_DONE || + buf->vb.state == VIDEOBUF_ERROR) + return POLLIN | POLLRDNORM; + + return 0; +} + +static struct soc_camera_host_ops omap1_host_ops = { + .owner = THIS_MODULE, + .add = omap1_cam_add_device, + .remove = omap1_cam_remove_device, + .get_formats = omap1_cam_get_formats, + .set_crop = omap1_cam_set_crop, + .set_fmt = omap1_cam_set_fmt, + .try_fmt = omap1_cam_try_fmt, + .init_videobuf = omap1_cam_init_videobuf, + .reqbufs = omap1_cam_reqbufs, + .querycap = omap1_cam_querycap, + .set_bus_param = omap1_cam_set_bus_param, + .poll = omap1_cam_poll, +}; + +static int __init omap1_cam_probe(struct platform_device *pdev) +{ + struct omap1_cam_dev *pcdev; + struct resource *res; + struct clk *clk; + void __iomem *base; + unsigned int irq; + int err = 0; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + irq = platform_get_irq(pdev, 0); + if (!res || (int)irq <= 0) { + err = -ENODEV; + goto exit; + } + + clk = clk_get(&pdev->dev, "armper_ck"); + if (IS_ERR(clk)) { + err = PTR_ERR(clk); + goto exit; + } + + pcdev = kzalloc(sizeof(*pcdev), GFP_KERNEL); + if (!pcdev) { + dev_err(&pdev->dev, "Could not allocate pcdev\n"); + err = -ENOMEM; + goto exit_put_clk; + } + + pcdev->res = res; + pcdev->clk = clk; + + pcdev->pdata = pdev->dev.platform_data; + pcdev->pflags = pcdev->pdata->flags; + + if (pcdev->pdata) + pcdev->camexclk = pcdev->pdata->camexclk_khz * 1000; + + switch (pcdev->camexclk) { + case CAM_EXCLK_6MHz: + case CAM_EXCLK_8MHz: + case CAM_EXCLK_9_6MHz: + case CAM_EXCLK_12MHz: + case CAM_EXCLK_24MHz: + break; + default: + dev_warn(&pdev->dev, + "Incorrect sensor clock frequency %ld kHz, " + "should be one of 0, 6, 8, 9.6, 12 or 24 MHz, " + "please correct your platform data\n", + pcdev->pdata->camexclk_khz); + pcdev->camexclk = 0; + case 0: + dev_info(&pdev->dev, + "Not providing sensor clock\n"); + } + + INIT_LIST_HEAD(&pcdev->capture); + spin_lock_init(&pcdev->lock); + + /* + * Request the region. + */ + if (!request_mem_region(res->start, resource_size(res), DRIVER_NAME)) { + err = -EBUSY; + goto exit_kfree; + } + + base = ioremap(res->start, resource_size(res)); + if (!base) { + err = -ENOMEM; + goto exit_release; + } + pcdev->irq = irq; + pcdev->base = base; + + /* apply reset to camera sensor if requested by platform data */ + if (pcdev->pflags & OMAP1_CAMERA_RST_HIGH) + CAM_WRITE(pcdev, GPIO, 0x1); + else if (pcdev->pflags & OMAP1_CAMERA_RST_LOW) + CAM_WRITE(pcdev, GPIO, 0x0);

Why not use sensor_reset()?

+ + err = omap_request_dma(OMAP_DMA_CAMERA_IF_RX, DRIVER_NAME, + dma_isr, (void *)pcdev, &pcdev->dma_ch); + if (err < 0) { + dev_err(&pdev->dev, "Can't request DMA for OMAP1 Camera\n"); + err = -EBUSY; + goto exit_iounmap; + } + dev_dbg(&pdev->dev, "got DMA channel %d\n", pcdev->dma_ch); + + /* preconfigure DMA */ + omap_set_dma_src_params(pcdev->dma_ch, OMAP_DMA_PORT_TIPB, + OMAP_DMA_AMODE_CONSTANT, res->start + REG_CAMDATA, + 0, 0); + omap_set_dma_dest_burst_mode(pcdev->dma_ch, OMAP_DMA_DATA_BURST_4); + /* setup DMA autoinitialization */ + omap_dma_link_lch(pcdev->dma_ch, pcdev->dma_ch); + + CAM_WRITE(pcdev, MODE, THRESHOLD_LEVEL << THRESHOLD_SHIFT); + + err = request_irq(pcdev->irq, cam_isr, 0, DRIVER_NAME, pcdev); + if (err) { + dev_err(&pdev->dev, "Camera interrupt register failed\n"); + goto exit_free_dma; + } + + pcdev->soc_host.drv_name = DRIVER_NAME; + pcdev->soc_host.ops = &omap1_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_irq; + + dev_info(&pdev->dev, "OMAP1 Camera Interface driver loaded\n"); + + return 0; + +exit_free_irq: + free_irq(pcdev->irq, pcdev); +exit_free_dma: + omap_free_dma(pcdev->dma_ch); +exit_iounmap: + iounmap(base); +exit_release: + release_mem_region(res->start, resource_size(res)); +exit_kfree: + kfree(pcdev); +exit_put_clk: + clk_put(clk); +exit: + return err; +} + +static int __exit omap1_cam_remove(struct platform_device *pdev) +{ + struct soc_camera_host *soc_host = to_soc_camera_host(&pdev->dev); + struct omap1_cam_dev *pcdev = container_of(soc_host, + struct omap1_cam_dev, soc_host); + struct resource *res; + + soc_camera_host_unregister(soc_host); + + free_irq(pcdev->irq, pcdev); + + omap_free_dma(pcdev->dma_ch);

It is probably safer to first shut down the hardware, and only then unconfigure software. So, free_irq and free_dma should go first, I think.

+ + iounmap(pcdev->base); + + res = pcdev->res; + release_mem_region(res->start, resource_size(res)); + + kfree(pcdev); + + clk_put(pcdev->clk); + + dev_info(&pdev->dev, "OMAP1 Camera Interface driver unloaded\n"); + + return 0; +} + +static struct platform_driver omap1_cam_driver = { + .driver = { + .name = DRIVER_NAME, + }, + .probe = omap1_cam_probe, + .remove = __exit_p(omap1_cam_remove), +}; + +static int __init omap1_cam_init(void) +{ + return platform_driver_register(&omap1_cam_driver); +} + +static void __exit omap1_cam_exit(void) +{ + platform_driver_unregister(&omap1_cam_driver); +} + +module_init(omap1_cam_init); +module_exit(omap1_cam_exit); + +MODULE_DESCRIPTION("OMAP1 Camera Interface driver"); +MODULE_AUTHOR("Janusz Krzysztofik <jkrzyszt@tis.icnet.pl>"); +MODULE_LICENSE("GPL v2"); +MODULE_ALIAS("platform:" DRIVER_NAME);

Other patch reviews to follow... Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/

On Sun, 18 Jul 2010, Janusz Krzysztofik wrote:

This patch provides a V4L2 SoC Camera driver for OV6650 camera sensor, found on OMAP1 SoC based Amstrad Delta videophone.

Have you also had a look at drivers/media/video/gspca/sonixb.c - in also supports ov6650 among other sensors.

Since I have no experience with camera sensors, and the sensor documentation I was able to find was not very comprehensive, I left most settings at their default (reset) values, except for: - those required for proper mediabus parameters and picture format setup, - those used by controls. Resulting picture quality is far from perfect, but better than nothing.

In order to be able to get / set the sensor frame rate from userspace, I decided to provide two not yet SoC camera supported operations, g_parm and s_parm. These can be used after applying patch 4/6 from this series, "SoC Camera: add support for g_parm / s_parm operations".

Created and tested against linux-2.6.35-rc3 on Amstrad Delta.

Signed-off-by: Janusz Krzysztofik <jkrzyszt@tis.icnet.pl> --- drivers/media/video/Kconfig | 6 drivers/media/video/Makefile | 1 drivers/media/video/ov6650.c | 1336 ++++++++++++++++++++++++++++++++++++++++ include/media/v4l2-chip-ident.h | 1 4 files changed, 1344 insertions(+)

--- linux-2.6.35-rc3.orig/drivers/media/video/Kconfig 2010-06-26 15:55:29.000000000 +0200 +++ linux-2.6.35-rc3/drivers/media/video/Kconfig 2010-07-02 04:12:02.000000000 +0200 @@ -913,6 +913,12 @@ config SOC_CAMERA_PLATFORM help This is a generic SoC camera platform driver, useful for testing

+config SOC_CAMERA_OV6650 + tristate "ov6650 sensor support" + depends on SOC_CAMERA && I2C + help + This is a V4L2 SoC camera driver for the OmniVision OV6650 sensor + config SOC_CAMERA_OV772X tristate "ov772x camera support" depends on SOC_CAMERA && I2C --- linux-2.6.35-rc3.orig/drivers/media/video/Makefile 2010-06-26 15:55:29.000000000 +0200 +++ linux-2.6.35-rc3/drivers/media/video/Makefile 2010-06-26 17:28:09.000000000 +0200 @@ -79,6 +79,7 @@ obj-$(CONFIG_SOC_CAMERA_MT9M111) += mt9m obj-$(CONFIG_SOC_CAMERA_MT9T031) += mt9t031.o obj-$(CONFIG_SOC_CAMERA_MT9T112) += mt9t112.o obj-$(CONFIG_SOC_CAMERA_MT9V022) += mt9v022.o +obj-$(CONFIG_SOC_CAMERA_OV6650) += ov6650.o obj-$(CONFIG_SOC_CAMERA_OV772X) += ov772x.o obj-$(CONFIG_SOC_CAMERA_OV9640) += ov9640.o obj-$(CONFIG_SOC_CAMERA_RJ54N1) += rj54n1cb0c.o --- linux-2.6.35-rc3.orig/drivers/media/video/ov6650.c 1970-01-01 01:00:00.000000000 +0100 +++ linux-2.6.35-rc3/drivers/media/video/ov6650.c 2010-07-18 02:06:22.000000000 +0200 @@ -0,0 +1,1336 @@ +/* + * V4L2 SoC Camera driver for OmniVision OV6650 Camera Sensor + * + * Copyright (C) 2010 Janusz Krzysztofik <jkrzyszt@tis.icnet.pl> + * + * Based on OmniVision OV96xx Camera Driver + * Copyright (C) 2009 Marek Vasut <marek.vasut@gmail.com> + * + * Based on ov772x camera driver: + * Copyright (C) 2008 Renesas Solutions Corp. + * Kuninori Morimoto <morimoto.kuninori@renesas.com> + * + * Based on ov7670 and soc_camera_platform driver, + * Copyright 2006-7 Jonathan Corbet <corbet@lwn.net> + * Copyright (C) 2008 Magnus Damm + * Copyright (C) 2008, Guennadi Liakhovetski <kernel@pengutronix.de> + * + * Hardware specific bits initialy based on former work by Matt Callow + * drivers/media/video/omap/sensor_ov6650.c + * Copyright (C) 2006 Matt Callow + * + * 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/i2c.h> +#include <linux/slab.h> +#include <linux/delay.h> +#include <media/v4l2-chip-ident.h> +#include <media/soc_camera.h>

Please, sort headers alphabetically (media/ and linux/ separately, of course).

+ + +/* Register definitions */ +#define REG_GAIN 0x00 /* range 00 - 3F */ +#define REG_BLUE 0x01 +#define REG_RED 0x02 +#define REG_SAT 0x03 /* [7:4] saturation [0:3] reserved */ +#define REG_HUE 0x04 /* [7:6] rsrvd [5] hue en [4:0] hue */ + +#define REG_BRT 0x06 + +#define REG_PIDH 0x0a +#define REG_PIDL 0x0b + +#define REG_AECH 0x10 +#define REG_CLKRC 0x11 /* Data Format and Internal Clock */ + /* [7:6] Input system clock (MHz)*/ + /* 00=8, 01=12, 10=16, 11=24 */ + /* [5:0]: Internal Clock Pre-Scaler */ +#define REG_COMA 0x12 /* [7] Reset */ +#define REG_COMB 0x13 +#define REG_COMC 0x14 +#define REG_COMD 0x15 +#define REG_COML 0x16

You used TAB in most defines and a space in this one and a few ones below. Please, use the same for all (I personally would just use a space).

+#define REG_HSTRT 0x17 +#define REG_HSTOP 0x18 +#define REG_VSTRT 0x19 +#define REG_VSTOP 0x1a +#define REG_PSHFT 0x1b +#define REG_MIDH 0x1c +#define REG_MIDL 0x1d +#define REG_HSYNS 0x1e +#define REG_HSYNE 0x1f +#define REG_COME 0x20 +#define REG_YOFF 0x21 +#define REG_UOFF 0x22 +#define REG_VOFF 0x23 +#define REG_AEW 0x24 +#define REG_AEB 0x25 +#define REG_COMF 0x26 +#define REG_COMG 0x27 +#define REG_COMH 0x28 +#define REG_COMI 0x29 + +#define REG_FRARL 0x2b +#define REG_COMJ 0x2c +#define REG_COMK 0x2d +#define REG_AVGY 0x2e +#define REG_REF0 0x2f +#define REG_REF1 0x30 +#define REG_REF2 0x31 +#define REG_FRAJH 0x32 +#define REG_FRAJL 0x33 +#define REG_FACT 0x34 +#define REG_L1AEC 0x35 +#define REG_AVGU 0x36 +#define REG_AVGV 0x37 + +#define REG_SPCB 0x60 +#define REG_SPCC 0x61 +#define REG_GAM1 0x62 +#define REG_GAM2 0x63 +#define REG_GAM3 0x64 +#define REG_SPCD 0x65 + +#define REG_SPCE 0x68 +#define REG_ADCL 0x69 + +#define REG_RMCO 0x6c +#define REG_GMCO 0x6d +#define REG_BMCO 0x6e + +#define NUM_REGS (REG_BMCO + 1)

NUM_REGS is unused, don't need to define it.

+ + +/* Register bits, values, etc. */ +#define OV6650_PIDH 0x66 /* high byte of product ID number */ +#define OV6650_PIDL 0x50 /* low byte of product ID number */ +#define OV6650_MIDH 0x7F /* high byte of mfg ID */ +#define OV6650_MIDL 0xA2 /* low byte of mfg ID */ + +#define DEF_GAIN 0x00 +#define DEF_BLUE 0x80 +#define DEF_RED 0x80 + +#define SAT_SHIFT 4 +#define SAT_MASK 0xf +#define SET_SAT(x) (((x) & SAT_MASK) << SAT_SHIFT)

Nitpicking, but I would +#define SAT_SHIFT 4 +#define SAT_MASK 0xf0 +#define SET_SAT(x) (((x) << SAT_SHIFT) & SAT_MASK) Advantage: your SAT_MASK is already correctly shifted, so, you don't have to you SET_SAT(SAT_MASK) to get to the register value.

+ +#define HUE_EN BIT(5)

You have to #include <linux/bitops.h> for the BIT() macro

+#define HUE_MASK 0x1f +#define DEF_HUE 0x10 +#define SET_HUE(x) ((x) == DEF_HUE ? (x) : \ + HUE_EN | ((x) & HUE_MASK)) + +#define DEF_AECH 0x4D + +#define CLKRC_6MHz 0x00 +#define CLKRC_12MHz 0x40 +#define CLKRC_16MHz 0x80 +#define CLKRC_24MHz 0xc0 +#define CLKRC_DIV_MASK 0x3f +#define GET_CLKRC_DIV(x) (((x) & CLKRC_DIV_MASK) + 1) + +#define COMA_RESET BIT(7) +#define COMA_QCIF BIT(5) +#define COMA_RAW_RGB BIT(4) +#define COMA_RGB BIT(3) +#define COMA_BW BIT(2) +#define COMA_WORD_SWAP BIT(1) +#define COMA_BYTE_SWAP BIT(0) +#define DEF_COMA 0x00 + +#define COMB_FLIP_V BIT(7) +#define COMB_FLIP_H BIT(5) +#define COMB_AWB BIT(2) +#define COMB_AGC BIT(1) +#define COMB_AEC BIT(0) + +#define COML_ONE_CHANNEL BIT(7) + +#define DEF_HSTRT 0x24 +#define DEF_HSTOP 0xd4 +#define DEF_VSTRT 0x04 +#define DEF_VSTOP 0x94 + +#define COMF_HREF_LOW BIT(4) + +#define COMJ_PCLK_RISING BIT(4) +#define COMJ_VSYNC_HIGH BIT(0) + +/* supported resolutions */ +#define W_QCIF (DEF_HSTOP - DEF_HSTRT) +#define W_CIF (W_QCIF << 1) +#define H_QCIF (DEF_VSTOP - DEF_VSTRT) +#define H_CIF (H_QCIF << 1) + +#define FRAME_RATE_MAX 30 + + +struct ov6650_reg { + u8 reg; + u8 val; +}; + +struct ov6650 { + struct v4l2_subdev subdev; + + int gain; + int blue; + int red; + int saturation; + int hue; + int brightness; + int exposure; + int gamma; + bool vflip; + bool hflip; + bool awb; + bool agc; + int aec; + bool hue_auto; + bool qcif; + unsigned long pclk_max; /* from resolution and format */ + unsigned long pclk_limit; /* from host */ + struct v4l2_fract timeperframe; /* as requested with s_parm */ +}; + + +/* default register setup */ +static const struct ov6650_reg ov6650_regs_dflt[] = { + { REG_COMA, DEF_COMA }, /* ~COMA_RESET */ +}; + +static enum v4l2_mbus_pixelcode ov6650_codes[] = { + V4L2_MBUS_FMT_YUYV8_2X8_LE, + V4L2_MBUS_FMT_YUYV8_2X8_BE, + V4L2_MBUS_FMT_YVYU8_2X8_LE, + V4L2_MBUS_FMT_YVYU8_2X8_BE, + V4L2_MBUS_FMT_SBGGR8_1X8, + V4L2_MBUS_FMT_GREY8_1X8, +}; + +static const struct v4l2_queryctrl ov6650_controls[] = { + { + .id = V4L2_CID_AUTOGAIN, + .type = V4L2_CTRL_TYPE_BOOLEAN, + .name = "AGC", + .minimum = 0, + .maximum = 1, + .step = 1, + .default_value = 1, + }, + { + .id = V4L2_CID_GAIN, + .type = V4L2_CTRL_TYPE_INTEGER, + .name = "Gain", + .minimum = 0, + .maximum = 0x3f, + .step = 1, + .default_value = DEF_GAIN, + }, + { + .id = V4L2_CID_AUTO_WHITE_BALANCE, + .type = V4L2_CTRL_TYPE_BOOLEAN, + .name = "AWB", + .minimum = 0, + .maximum = 1, + .step = 1, + .default_value = 1, + }, + { + .id = V4L2_CID_BLUE_BALANCE, + .type = V4L2_CTRL_TYPE_INTEGER, + .name = "Blue", + .minimum = 0, + .maximum = 0xff, + .step = 1, + .default_value = DEF_BLUE, + }, + { + .id = V4L2_CID_RED_BALANCE, + .type = V4L2_CTRL_TYPE_INTEGER, + .name = "Red", + .minimum = 0, + .maximum = 0xff, + .step = 1, + .default_value = DEF_RED, + }, + { + .id = V4L2_CID_SATURATION, + .type = V4L2_CTRL_TYPE_INTEGER, + .name = "Saturation", + .minimum = 0, + .maximum = 0xf, + .step = 1, + .default_value = 0x8, + }, + { + .id = V4L2_CID_HUE_AUTO, + .type = V4L2_CTRL_TYPE_BOOLEAN, + .name = "Auto Hue", + .minimum = 0, + .maximum = 1, + .step = 1, + .default_value = 1, + }, + { + .id = V4L2_CID_HUE, + .type = V4L2_CTRL_TYPE_INTEGER, + .name = "Hue", + .minimum = 0, + .maximum = 0x1f, + .step = 1, + .default_value = DEF_HUE, + }, + { + .id = V4L2_CID_BRIGHTNESS, + .type = V4L2_CTRL_TYPE_INTEGER, + .name = "Brightness", + .minimum = 0, + .maximum = 0xff, + .step = 1, + .default_value = 0x80, + }, + { + .id = V4L2_CID_EXPOSURE_AUTO, + .type = V4L2_CTRL_TYPE_INTEGER, + .name = "AEC", + .minimum = 0, + .maximum = 3, + .step = 1, + .default_value = 0, + }, + { + .id = V4L2_CID_EXPOSURE, + .type = V4L2_CTRL_TYPE_INTEGER, + .name = "Exposure", + .minimum = 0, + .maximum = 0xff, + .step = 1, + .default_value = DEF_AECH, + }, + { + .id = V4L2_CID_GAMMA, + .type = V4L2_CTRL_TYPE_INTEGER, + .name = "Gamma", + .minimum = 0, + .maximum = 0xff, + .step = 1, + .default_value = 0x12, + }, + { + .id = V4L2_CID_VFLIP, + .type = V4L2_CTRL_TYPE_BOOLEAN, + .name = "Flip Vertically", + .minimum = 0, + .maximum = 1, + .step = 1, + .default_value = 0, + }, + { + .id = V4L2_CID_HFLIP, + .type = V4L2_CTRL_TYPE_BOOLEAN, + .name = "Flip Horizontally", + .minimum = 0, + .maximum = 1, + .step = 1, + .default_value = 0, + }, +}; + +/* read a register */ +static int ov6650_reg_read(struct i2c_client *client, u8 reg, u8 *val) +{ + int ret; + u8 data = reg; + struct i2c_msg msg = { + .addr = client->addr, + .flags = 0, + .len = 1, + .buf = &data, + }; + + ret = i2c_transfer(client->adapter, &msg, 1); + if (ret < 0) + goto err; + + msg.flags = I2C_M_RD; + ret = i2c_transfer(client->adapter, &msg, 1); + if (ret < 0) + goto err; + + *val = data; + return 0; + +err: + dev_err(&client->dev, "Failed reading register 0x%02x!\n", reg); + return ret; +} + +/* write a register */ +static int ov6650_reg_write(struct i2c_client *client, u8 reg, u8 val) +{ + int ret; + u8 _val; + unsigned char data[2] = { reg, val }; + struct i2c_msg msg = { + .addr = client->addr, + .flags = 0, + .len = 2, + .buf = data, + }; + + ret = i2c_transfer(client->adapter, &msg, 1); + if (ret < 0) { + dev_err(&client->dev, "Failed writing register 0x%02x!\n", reg); + return ret; + } + msleep(1);

Hm, interesting... Is this really needed?

+ + /* we have to read the register back ... no idea why, maybe HW bug */ + ret = ov6650_reg_read(client, reg, &_val);

You can also use "val" - it is not needed any more - and drop "_val."

+ if (ret) + dev_err(&client->dev, + "Failed reading back register 0x%02x!\n", reg); + + return 0; +} + + +/* Read a register, alter its bits, write it back */ +static int ov6650_reg_rmw(struct i2c_client *client, u8 reg, u8 set, u8 mask) +{ + u8 val; + int ret; + + ret = ov6650_reg_read(client, reg, &val); + if (ret) { + dev_err(&client->dev, + "[Read]-Modify-Write of register %02x failed!\n", reg); + return val;

You mean "return ret"

+ } + + val &= ~mask; + val |= set; + + ret = ov6650_reg_write(client, reg, val); + if (ret) + dev_err(&client->dev, + "Read-Modify-[Write] of register %02x failed!\n", reg); + + return ret; +} + +/* Soft reset the camera. This has nothing to do with the RESET pin! */ +static int ov6650_reset(struct i2c_client *client) +{ + int ret; + + dev_dbg(&client->dev, "reset\n"); + + ret = ov6650_reg_rmw(client, REG_COMA, COMA_RESET, 0); + if (ret) + dev_err(&client->dev, + "An error occured while entering soft reset!\n"); + + return ret; +} + +static struct ov6650 *to_ov6650(const struct i2c_client *client) +{ + return container_of(i2c_get_clientdata(client), struct ov6650, subdev); +} + +/* Start/Stop streaming from the device */ +static int ov6650_s_stream(struct v4l2_subdev *sd, int enable) +{ + return 0; +} + +/* Alter bus settings on camera side */ +static int ov6650_set_bus_param(struct soc_camera_device *icd, + unsigned long flags) +{ + struct soc_camera_link *icl = to_soc_camera_link(icd); + struct i2c_client *client = to_i2c_client(to_soc_camera_control(icd)); + int ret; + + flags = soc_camera_apply_sensor_flags(icl, flags); + + if (flags & SOCAM_PCLK_SAMPLE_RISING) + ret = ov6650_reg_rmw(client, REG_COMJ, COMJ_PCLK_RISING, 0); + else + ret = ov6650_reg_rmw(client, REG_COMJ, 0, COMJ_PCLK_RISING); + if (ret) + goto out; + + if (flags & SOCAM_HSYNC_ACTIVE_LOW) + ret = ov6650_reg_rmw(client, REG_COMF, COMF_HREF_LOW, 0); + else + ret = ov6650_reg_rmw(client, REG_COMF, 0, COMF_HREF_LOW); + if (ret) + goto out; + + if (flags & SOCAM_VSYNC_ACTIVE_HIGH) + ret = ov6650_reg_rmw(client, REG_COMJ, COMJ_VSYNC_HIGH, 0); + else + ret = ov6650_reg_rmw(client, REG_COMJ, 0, COMJ_VSYNC_HIGH); +out: + return ret; +} + +/* Request bus settings on camera side */ +static unsigned long ov6650_query_bus_param(struct soc_camera_device *icd) +{ + struct soc_camera_link *icl = to_soc_camera_link(icd); + + unsigned long flags = SOCAM_MASTER | \ + SOCAM_PCLK_SAMPLE_RISING | SOCAM_PCLK_SAMPLE_FALLING | \ + SOCAM_HSYNC_ACTIVE_HIGH | SOCAM_HSYNC_ACTIVE_LOW | \ + SOCAM_VSYNC_ACTIVE_HIGH | SOCAM_VSYNC_ACTIVE_LOW | \ + SOCAM_DATA_ACTIVE_HIGH | SOCAM_DATAWIDTH_8; + + return soc_camera_apply_sensor_flags(icl, flags); +} + +/* Get status of additional camera capabilities */ +static int ov6650_g_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl) +{ + struct i2c_client *client = sd->priv; + struct ov6650 *priv = to_ov6650(client); + uint8_t reg; + int ret = 0; + + switch (ctrl->id) { + case V4L2_CID_AUTOGAIN: + ctrl->value = priv->agc; + break; + case V4L2_CID_GAIN: + if (priv->agc) { + ret = ov6650_reg_read(client, REG_GAIN, ®); + ctrl->value = reg; + } else { + ctrl->value = priv->gain; + } + break; + case V4L2_CID_AUTO_WHITE_BALANCE: + ctrl->value = priv->awb; + break; + case V4L2_CID_BLUE_BALANCE: + if (priv->awb) { + ret = ov6650_reg_read(client, REG_BLUE, ®); + ctrl->value = reg; + } else { + ctrl->value = priv->blue; + } + break; + case V4L2_CID_RED_BALANCE: + if (priv->awb) { + ret = ov6650_reg_read(client, REG_RED, ®); + ctrl->value = reg; + } else { + ctrl->value = priv->red; + } + break; + case V4L2_CID_SATURATION: + ctrl->value = priv->saturation; + break; + case V4L2_CID_HUE_AUTO: + ctrl->value = priv->hue_auto; + break; + case V4L2_CID_HUE: + if (priv->hue_auto) { + ret = ov6650_reg_read(client, REG_HUE, ®); + ctrl->value = reg & 0x1f; + } else { + ctrl->value = priv->hue; + } + break; + case V4L2_CID_BRIGHTNESS: + ctrl->value = priv->brightness; + break; + case V4L2_CID_EXPOSURE_AUTO: + ctrl->value = priv->aec; + break; + case V4L2_CID_EXPOSURE: + if (priv->aec) { + ret = ov6650_reg_read(client, REG_AECH, ®); + ctrl->value = reg; + } else { + ctrl->value = priv->exposure; + } + break; + case V4L2_CID_GAMMA: + ctrl->value = priv->gamma; + break; + case V4L2_CID_VFLIP: + ctrl->value = priv->vflip; + break; + case V4L2_CID_HFLIP: + ctrl->value = priv->hflip; + break; + } + return ret; +} + +/* Set status of additional camera capabilities */ +static int ov6650_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl) +{ + struct i2c_client *client = sd->priv; + struct ov6650 *priv = to_ov6650(client); + bool automatic; + int ret = 0; + + switch (ctrl->id) { + case V4L2_CID_AUTOGAIN: + if (ctrl->value) { + ret = ov6650_reg_write(client, REG_GAIN, DEF_GAIN); + if (ret) + break; + priv->gain = DEF_GAIN; + ret = ov6650_reg_rmw(client, REG_COMB, COMB_AGC, 0); + } else { + ret = ov6650_reg_rmw(client, REG_COMB, 0, COMB_AGC); + } + if (ret) + break; + priv->agc = ctrl->value; + break; + case V4L2_CID_GAIN: + ret = ov6650_reg_write(client, REG_GAIN, ctrl->value); + if (ret) + break; + priv->gain = ctrl->value; + automatic = (priv->gain == DEF_GAIN); + if (automatic) + ret = ov6650_reg_rmw(client, REG_COMB, COMB_AGC, 0); + else + ret = ov6650_reg_rmw(client, REG_COMB, 0, COMB_AGC); + if (ret) + break; + priv->agc = automatic;

I wouldn't touch autogain here. You have V4L2_CID_AUTOGAIN for that. Setting gain to a value, that happens to be equal default, doesn't have to turn autogain on, does it?

+ break; + case V4L2_CID_AUTO_WHITE_BALANCE: + if (ctrl->value) { + ret = ov6650_reg_write(client, REG_BLUE, DEF_BLUE); + if (ret) + break; + priv->blue = DEF_BLUE; + ret = ov6650_reg_write(client, REG_RED, DEF_RED); + if (ret) + break; + priv->red = DEF_RED; + ret = ov6650_reg_rmw(client, REG_COMB, COMB_AWB, 0); + } else { + ret = ov6650_reg_rmw(client, REG_COMB, 0, COMB_AWB); + } + if (ret) + break; + priv->awb = ctrl->value; + break; + case V4L2_CID_BLUE_BALANCE: + ret = ov6650_reg_write(client, REG_BLUE, ctrl->value); + if (ret) + break; + priv->blue = ctrl->value; + automatic = (priv->blue == DEF_BLUE && + priv->red == DEF_RED); + if (automatic) + ret = ov6650_reg_rmw(client, REG_COMB, COMB_AWB, 0); + else + ret = ov6650_reg_rmw(client, REG_COMB, 0, COMB_AWB); + if (ret) + break; + priv->awb = automatic; + break; + case V4L2_CID_RED_BALANCE: + ret = ov6650_reg_write(client, REG_RED, ctrl->value); + if (ret) + break; + priv->red = ctrl->value; + automatic = (priv->blue == DEF_BLUE && + priv->red == DEF_RED); + if (automatic) + ret = ov6650_reg_rmw(client, REG_COMB, COMB_AWB, 0); + else + ret = ov6650_reg_rmw(client, REG_COMB, 0, COMB_AWB); + if (ret) + break; + priv->awb = automatic;

Same here, I wouldn't touch AWB in the above two controls. You have V4L2_CID_AUTO_WHITE_BALANCE for that.

+ break; + case V4L2_CID_SATURATION: + ret = ov6650_reg_rmw(client, REG_SAT, SET_SAT(ctrl->value), + SET_SAT(SAT_MASK));

With the proposed change it would look like + ret = ov6650_reg_rmw(client, REG_SAT, SET_SAT(ctrl->value), + SAT_MASK); which I personally find a bit more pleasant to the eyes;)

+ if (ret) + break; + priv->saturation = ctrl->value; + break; + case V4L2_CID_HUE_AUTO: + if (ctrl->value) { + ret = ov6650_reg_rmw(client, REG_HUE, + SET_HUE(DEF_HUE), SET_HUE(HUE_MASK)); + if (ret) + break; + priv->hue = DEF_HUE; + } else { + ret = ov6650_reg_rmw(client, REG_HUE, HUE_EN, 0); + } + if (ret) + break; + priv->hue_auto = ctrl->value;

Hm, sorry, don't understand. If the user sets auto-hue to ON, you set the hue enable bit and hue value to default. If the user sets auto-hue to OFF, you just set the hue enable bit on and don't change the value. Does ov6650 actually support auto-hue?

+ break; + case V4L2_CID_HUE: + ret = ov6650_reg_rmw(client, REG_HUE, SET_HUE(ctrl->value), + SET_HUE(HUE_MASK)); + if (ret) + break; + priv->hue = ctrl->value; + priv->hue_auto = (priv->hue == DEF_HUE);

Here it seems like in order to adjust hue you always have to set the enable bit. Again, I wouldn't touch hue_auto here - default and auto are different things.

+ break; + case V4L2_CID_BRIGHTNESS: + ret = ov6650_reg_write(client, REG_BRT, ctrl->value); + if (ret) + break; + priv->brightness = ctrl->value; + break; + case V4L2_CID_EXPOSURE_AUTO: + switch (ctrl->value) { + case V4L2_EXPOSURE_AUTO: + ret = ov6650_reg_write(client, REG_AECH, DEF_AECH);

Is this a requirement for auto-exposure, that you have to set the (analog manual) exposure to the default value?

+ if (ret) + break; + priv->exposure = DEF_AECH; + ret = ov6650_reg_rmw(client, REG_COMB, COMB_AEC, 0); + break; + default: + ret = ov6650_reg_rmw(client, REG_COMB, 0, COMB_AEC); + break; + } + if (ret) + break; + priv->aec = ctrl->value; + break; + case V4L2_CID_EXPOSURE: + ret = ov6650_reg_write(client, REG_AECH, ctrl->value); + if (ret) + break; + priv->exposure = ctrl->value; + automatic = (priv->exposure == DEF_AECH); + if (automatic) + ret = ov6650_reg_rmw(client, REG_COMB, COMB_AEC, 0); + else + ret = ov6650_reg_rmw(client, REG_COMB, 0, COMB_AEC); + if (ret) + break; + priv->aec = automatic ? V4L2_EXPOSURE_AUTO : + V4L2_EXPOSURE_MANUAL;

Again - don't see why you need to touch the auto setting here.

+ break; + case V4L2_CID_GAMMA: + ret = ov6650_reg_write(client, REG_GAM1, ctrl->value); + if (ret) + break; + priv->gamma = ctrl->value; + break; + case V4L2_CID_VFLIP: + if (ctrl->value) + ret = ov6650_reg_rmw(client, REG_COMB, + COMB_FLIP_V, 0); + else + ret = ov6650_reg_rmw(client, REG_COMB, + 0, COMB_FLIP_V); + if (ret) + break; + priv->vflip = ctrl->value; + break; + case V4L2_CID_HFLIP: + if (ctrl->value) + ret = ov6650_reg_rmw(client, REG_COMB, + COMB_FLIP_H, 0); + else + ret = ov6650_reg_rmw(client, REG_COMB, + 0, COMB_FLIP_H); + if (ret) + break; + priv->hflip = ctrl->value; + break; + } + + return ret; +} + +/* Get chip identification */ +static int ov6650_g_chip_ident(struct v4l2_subdev *sd, + struct v4l2_dbg_chip_ident *id) +{ + id->ident = V4L2_IDENT_OV6650; + id->revision = 0; + + return 0; +} + +#ifdef CONFIG_VIDEO_ADV_DEBUG +static int ov6650_get_register(struct v4l2_subdev *sd, + struct v4l2_dbg_register *reg) +{ + struct i2c_client *client = sd->priv; + int ret; + u8 val; + + if (reg->reg & ~0xff) + return -EINVAL; + + reg->size = 1; + + ret = ov6650_reg_read(client, reg->reg, &val); + if (ret) + return ret; + + reg->val = (__u64)val; + + return 0; +} + +static int ov6650_set_register(struct v4l2_subdev *sd, + struct v4l2_dbg_register *reg) +{ + struct i2c_client *client = sd->priv; + + if (reg->reg & ~0xff || reg->val & ~0xff) + return -EINVAL; + + return ov6650_reg_write(client, reg->reg, reg->val); +} +#endif + +/* select nearest higher resolution for capture */ +static void ov6650_res_roundup(u32 *width, u32 *height) +{ + int i; + enum { QCIF, CIF }; + int res_x[] = { 176, 352 }; + int res_y[] = { 144, 288 }; + + for (i = 0; i < ARRAY_SIZE(res_x); i++) { + if (res_x[i] >= *width && res_y[i] >= *height) { + *width = res_x[i]; + *height = res_y[i]; + return; + } + } + + *width = res_x[CIF]; + *height = res_y[CIF]; +}

This can be replaced by a version of http://www.spinics.net/lists/linux-media/msg21893.html when it is fixed and accepted;) I'll try to send an updated version of that patch tomorrow.

+ +/* program default register values */ +static int ov6650_prog_dflt(struct i2c_client *client) +{ + int i, ret; + + dev_dbg(&client->dev, "reinitializing\n"); + + for (i = 0; i < ARRAY_SIZE(ov6650_regs_dflt); i++) { + ret = ov6650_reg_write(client, ov6650_regs_dflt[i].reg, + ov6650_regs_dflt[i].val); + if (ret) + return ret; + }

Hm, please, don't. I generally don't like such register - value array magic for a number of reasons, and in your case it's just one (!) register write operation - please, remove this array and just write the register explicitly. You also don't need DEF_COMA - writing an explicit "0" and adding a comment - "clear all flags, including reset" would be perfect!

+ + return 0; +} + +/* set the format we will capture in */ +static int ov6650_s_fmt(struct v4l2_subdev *sd, + struct v4l2_mbus_framefmt *mf) +{ + struct i2c_client *client = sd->priv; + struct soc_camera_device *icd = client->dev.platform_data; + struct soc_camera_sense *sense = icd->sense; + struct ov6650 *priv = to_ov6650(client); + enum v4l2_colorspace cspace; + enum v4l2_mbus_pixelcode code = mf->code; + unsigned long pclk; + u8 coma_set = 0, coma_mask = 0, coml_set = 0, coml_mask = 0, clkrc; + int ret; + + /* select color matrix configuration for given color encoding */ + switch (code) { + case V4L2_MBUS_FMT_GREY8_1X8: + dev_dbg(&client->dev, "pixel format GREY8_1X8\n"); + coma_set |= COMA_BW; + coma_mask |= COMA_RGB | COMA_WORD_SWAP | COMA_BYTE_SWAP; + coml_mask |= COML_ONE_CHANNEL; + cspace = V4L2_COLORSPACE_JPEG; + priv->pclk_max = 4000000; + break; + case V4L2_MBUS_FMT_YUYV8_2X8_LE: + dev_dbg(&client->dev, "pixel format YUYV8_2X8_LE\n"); + coma_set |= COMA_WORD_SWAP; + coma_mask |= COMA_RGB | COMA_BW | COMA_BYTE_SWAP; + goto yuv;

Well, this doesn't look critical to me, i.e., eventually I would accept this, but, as you know, goto's in the kernel are (almost) only used for failure cases, besides, what makes this use even less pretty, this is a goto into a switch statement... I would set a "yuv = 1" variable instead of goto's, and use a "if (yuv) after the switch.

+ case V4L2_MBUS_FMT_YVYU8_2X8_LE: + dev_dbg(&client->dev, "pixel format YVYU8_2X8_LE (untested)\n"); + coma_mask |= COMA_RGB | COMA_BW | COMA_WORD_SWAP | + COMA_BYTE_SWAP; + goto yuv; + case V4L2_MBUS_FMT_YUYV8_2X8_BE: + dev_dbg(&client->dev, "pixel format YUYV8_2X8_BE\n"); + if (mf->width == W_CIF) { + coma_set |= COMA_BYTE_SWAP | COMA_WORD_SWAP; + coma_mask |= COMA_RGB | COMA_BW; + } else { + coma_set |= COMA_BYTE_SWAP; + coma_mask |= COMA_RGB | COMA_BW | COMA_WORD_SWAP; + } + goto yuv; + case V4L2_MBUS_FMT_YVYU8_2X8_BE: + dev_dbg(&client->dev, "pixel format YVYU8_2X8_BE (untested)\n"); + if (mf->width == W_CIF) { + coma_set |= COMA_BYTE_SWAP; + coma_mask |= COMA_RGB | COMA_BW | COMA_WORD_SWAP; + } else { + coma_set |= COMA_BYTE_SWAP | COMA_WORD_SWAP; + coma_mask |= COMA_RGB | COMA_BW; + } +yuv: + coml_set |= COML_ONE_CHANNEL; + cspace = V4L2_COLORSPACE_JPEG; + priv->pclk_max = 8000000; + break; + case V4L2_MBUS_FMT_SBGGR8_1X8: + dev_dbg(&client->dev, "pixel format SBGGR8_1X8 (untested)\n"); + coma_set |= COMA_RAW_RGB | COMA_RGB; + coma_mask |= COMA_BW | COMA_BYTE_SWAP | COMA_WORD_SWAP; + coml_mask |= COML_ONE_CHANNEL; + cspace = V4L2_COLORSPACE_SRGB; + priv->pclk_max = 4000000; + break; + default: + dev_err(&client->dev, "Pixel format not handled : %x\n", code); + return -EINVAL; + } + + /* select register configuration for given resolution */ + ov6650_res_roundup(&mf->width, &mf->height); + + switch (mf->width) { + case W_QCIF: + dev_dbg(&client->dev, "resolution QCIF\n"); + priv->qcif = 1; + coma_set |= COMA_QCIF; + priv->pclk_max /= 2; + break; + case W_CIF: + dev_dbg(&client->dev, "resolution CIF\n"); + priv->qcif = 0; + coma_mask |= COMA_QCIF; + break; + default: + dev_err(&client->dev, "unspported resolution!\n"); + return -EINVAL; + } + + if (priv->timeperframe.numerator && priv->timeperframe.denominator) + pclk = priv->pclk_max * priv->timeperframe.denominator / + (FRAME_RATE_MAX * priv->timeperframe.numerator); + else + pclk = priv->pclk_max; + + if (sense) { + if (sense->master_clock == 8000000) { + dev_dbg(&client->dev, "8MHz input clock\n"); + clkrc = CLKRC_6MHz; + } else if (sense->master_clock == 12000000) { + dev_dbg(&client->dev, "12MHz input clock\n"); + clkrc = CLKRC_12MHz; + } else if (sense->master_clock == 16000000) { + dev_dbg(&client->dev, "16MHz input clock\n"); + clkrc = CLKRC_16MHz; + } else if (sense->master_clock == 24000000) { + dev_dbg(&client->dev, "24MHz input clock\n"); + clkrc = CLKRC_24MHz; + } else { + dev_err(&client->dev, + "unspported input clock, check platform data" + "\n"); + return -EINVAL; + } + priv->pclk_limit = sense->pixel_clock_max; + if (priv->pclk_limit && + (priv->pclk_limit < pclk))

This can go on one line.

+ pclk = priv->pclk_limit; + } else { + priv->pclk_limit = 0; + clkrc = 0xc0; + dev_dbg(&client->dev, "using default 24MHz input clock\n"); + } + + clkrc |= (priv->pclk_max - 1) / pclk; + pclk = priv->pclk_max / GET_CLKRC_DIV(clkrc);

This would look better as clkrc_div = (priv->pclk_max - 1) / pclk; clkrc |= clkrc_div; pclk = priv->pclk_max / clkrc_div;

+ dev_dbg(&client->dev, "pixel clock divider: %ld.%ld\n", + sense->master_clock / pclk, + 10 * sense->master_clock % pclk / pclk); + + ov6650_reset(client); + + ret = ov6650_prog_dflt(client); + if (ret) + return ret; + +

Superfluous empty line;)

+ ret = ov6650_reg_rmw(client, REG_COMA, coma_set, coma_mask); + if (!ret) + ret = ov6650_reg_write(client, REG_CLKRC, clkrc); + if (!ret) + ret = ov6650_reg_rmw(client, REG_COML, coml_set, coml_mask); + + if (!ret) { + mf->code = code; + mf->colorspace = cspace; + } + + return ret; +} + +static int ov6650_try_fmt(struct v4l2_subdev *sd, + struct v4l2_mbus_framefmt *mf) +{ + ov6650_res_roundup(&mf->width, &mf->height); + + mf->field = V4L2_FIELD_NONE; + + switch (mf->code) { + case V4L2_MBUS_FMT_Y10_1X10: + mf->code = V4L2_MBUS_FMT_GREY8_1X8; + case V4L2_MBUS_FMT_GREY8_1X8: + case V4L2_MBUS_FMT_YVYU8_2X8_LE: + case V4L2_MBUS_FMT_YUYV8_2X8_LE: + case V4L2_MBUS_FMT_YVYU8_2X8_BE: + case V4L2_MBUS_FMT_YUYV8_2X8_BE: + mf->colorspace = V4L2_COLORSPACE_JPEG; + break; + default: + mf->code = V4L2_MBUS_FMT_SBGGR8_1X8; + case V4L2_MBUS_FMT_SBGGR8_1X8: + mf->colorspace = V4L2_COLORSPACE_SRGB; + break; + } + + return 0; +} + +static int ov6650_enum_fmt(struct v4l2_subdev *sd, unsigned int index, + enum v4l2_mbus_pixelcode *code) +{ + if ((unsigned int)index >= ARRAY_SIZE(ov6650_codes)) + return -EINVAL; + + *code = ov6650_codes[index]; + return 0; +} + +static int ov6650_cropcap(struct v4l2_subdev *sd, struct v4l2_cropcap *a) +{ + struct i2c_client *client = sd->priv; + struct ov6650 *priv = to_ov6650(client); + int shift = !priv->qcif; + + if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) + return -EINVAL; + + /* Crop limits depend on selected frame format (CIF/QCIF) */ + a->bounds.left = DEF_HSTRT << shift; + a->bounds.top = DEF_VSTRT << shift; + a->bounds.width = W_QCIF << shift; + a->bounds.height = H_QCIF << shift; + /* REVISIT: should defrect provide actual or default geometry? */

default

+ a->defrect = a->bounds; + a->pixelaspect.numerator = 1; + a->pixelaspect.denominator = 1; + + return 0; +} + +static int ov6650_g_crop(struct v4l2_subdev *sd, struct v4l2_crop *a) +{ + struct i2c_client *client = sd->priv; + struct ov6650 *priv = to_ov6650(client); + struct v4l2_rect *rect = &a->c; + int shift = !priv->qcif; + u8 hstrt, vstrt, hstop, vstop; + int ret; + + if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) + return -EINVAL; + + ret = ov6650_reg_read(client, REG_HSTRT, &hstrt); + if (!ret) + ret = ov6650_reg_read(client, REG_HSTOP, &hstop); + if (!ret) + ret = ov6650_reg_read(client, REG_VSTRT, &vstrt); + if (!ret) + ret = ov6650_reg_read(client, REG_VSTOP, &vstop); + + if (!ret) { + rect->left = hstrt << shift; + rect->top = vstrt << shift; + rect->width = (hstop - hstrt) << shift; + rect->height = (vstop - vstrt) << shift; + } + + return ret; +} + +static int ov6650_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *a) +{ + struct i2c_client *client = sd->priv; + struct ov6650 *priv = to_ov6650(client); + struct v4l2_rect *rect = &a->c; + int shift = !priv->qcif; + u8 hstrt, vstrt, hstop, vstop; + int ret; + + if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) + return -EINVAL; + + hstrt = rect->left >> shift; + vstrt = rect->top >> shift; + hstop = hstrt + (rect->width >> shift); + vstop = vstrt + (rect->height >> shift); + + if ((hstop > DEF_HSTOP) || (vstop > DEF_VSTOP)) { + dev_err(&client->dev, "Invalid window geometry!\n"); + return -EINVAL; + } + + ret = ov6650_reg_write(client, REG_HSTRT, hstrt); + if (!ret) + ret = ov6650_reg_write(client, REG_HSTOP, hstop); + if (!ret) + ret = ov6650_reg_write(client, REG_VSTRT, vstrt); + if (!ret) + ret = ov6650_reg_write(client, REG_VSTOP, vstop);

Are cropping and scaling on this camera absolutely independent? I.e., you can set any output format (CIF or QCIF) and it will just scale whatever rectangle has been configured? And the other way round - you set arbitrary cropping and output format stays the same?

+ + return ret; +} + +static int ov6650_g_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms) +{ + struct i2c_client *client = sd->priv; + struct v4l2_captureparm *cp = &parms->parm.capture; + u8 clkrc; + int ret; + + if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) + return -EINVAL; + + ret = ov6650_reg_read(client, REG_CLKRC, &clkrc); + if (ret < 0) + return ret; + + memset(cp, 0, sizeof(struct v4l2_captureparm)); + cp->capability = V4L2_CAP_TIMEPERFRAME; + cp->timeperframe.numerator = GET_CLKRC_DIV(clkrc); + cp->timeperframe.denominator = FRAME_RATE_MAX; + + dev_dbg(&client->dev, "Frame interval: %u/%u\n", + cp->timeperframe.numerator, cp->timeperframe.denominator); + + return 0; +} + +static int ov6650_s_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms) +{ + struct i2c_client *client = sd->priv; + struct ov6650 *priv = to_ov6650(client); + struct v4l2_captureparm *cp = &parms->parm.capture; + struct v4l2_fract *tpf = &cp->timeperframe; + int div, ret; + + if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) + return -EINVAL; + + if (cp->extendedmode != 0) + return -EINVAL; + + if (tpf->numerator == 0 || tpf->denominator == 0) + div = 1; /* Reset to full rate */ + else + div = (tpf->numerator * FRAME_RATE_MAX) / tpf->denominator; + + if (div == 0) + div = 1; + else if (div > CLKRC_DIV_MASK + 1) + div = CLKRC_DIV_MASK + 1; + + if (priv->pclk_max && priv->pclk_limit) { + ret = (priv->pclk_max - 1) / priv->pclk_limit; + if (div < ret) + div = ret; + } + + ret = ov6650_reg_rmw(client, REG_CLKRC, div - 1, CLKRC_DIV_MASK); + if (!ret) { + priv->timeperframe.numerator = tpf->numerator = FRAME_RATE_MAX; + priv->timeperframe.denominator = tpf->denominator = div; + } + + return ret; +} + +static int ov6650_video_probe(struct soc_camera_device *icd, + struct i2c_client *client) +{ + u8 pidh, pidl, midh, midl; + int ret = 0; + + /* + * check and show product ID and manufacturer ID + */ + ret = ov6650_reg_read(client, REG_PIDH, &pidh); + if (!ret) + ret = ov6650_reg_read(client, REG_PIDL, &pidl); + if (!ret) + ret = ov6650_reg_read(client, REG_MIDH, &midh); + if (!ret) + ret = ov6650_reg_read(client, REG_MIDL, &midl); + + if (ret) + goto err; + + if ((pidh != OV6650_PIDH) || (pidl != OV6650_PIDL)) { + dev_err(&client->dev, "Product ID error %x:%x\n", pidh, pidl); + ret = -ENODEV; + goto err; + } + + dev_info(&client->dev, "ov6650 Product ID %0x:%0x Manufacturer ID %x:%x" + "\n", pidh, pidl, midh, midl); + +err: + return ret; +} + +static struct soc_camera_ops ov6650_ops = { + .set_bus_param = ov6650_set_bus_param, + .query_bus_param = ov6650_query_bus_param, + .controls = ov6650_controls, + .num_controls = ARRAY_SIZE(ov6650_controls), +}; + +static struct v4l2_subdev_core_ops ov6650_core_ops = { + .g_ctrl = ov6650_g_ctrl, + .s_ctrl = ov6650_s_ctrl, + .g_chip_ident = ov6650_g_chip_ident, +#ifdef CONFIG_VIDEO_ADV_DEBUG + .g_register = ov6650_get_register, + .s_register = ov6650_set_register, +#endif +

Superfluous empty line

+}; + +static struct v4l2_subdev_video_ops ov6650_video_ops = { + .s_stream = ov6650_s_stream, + .s_mbus_fmt = ov6650_s_fmt, + .try_mbus_fmt = ov6650_try_fmt,

Please, implement.g_mbus_fmt.

+ .enum_mbus_fmt = ov6650_enum_fmt, + .cropcap = ov6650_cropcap, + .g_crop = ov6650_g_crop, + .s_crop = ov6650_s_crop, + .g_parm = ov6650_g_parm, + .s_parm = ov6650_s_parm, +

Superfluous empty line

+}; + +static struct v4l2_subdev_ops ov6650_subdev_ops = { + .core = &ov6650_core_ops, + .video = &ov6650_video_ops, +}; + +/* + * i2c_driver function + */ +static int ov6650_probe(struct i2c_client *client, + const struct i2c_device_id *did) +{ + struct ov6650 *priv; + struct soc_camera_device *icd = client->dev.platform_data;

Nothing to align here, a space before "=" would suffice;)

+ struct soc_camera_link *icl; + int ret; + + if (!icd) { + dev_err(&client->dev, "Missing soc-camera data!\n"); + return -EINVAL; + } + + icl = to_soc_camera_link(icd); + if (!icl) { + dev_err(&client->dev, "Missing platform_data for driver\n"); + return -EINVAL; + } + + priv = kzalloc(sizeof(struct ov6650), GFP_KERNEL); + if (!priv) { + dev_err(&client->dev, + "Failed to allocate memory for private data!\n"); + return -ENOMEM; + } + + v4l2_i2c_subdev_init(&priv->subdev, client, &ov6650_subdev_ops); + + icd->ops = &ov6650_ops;

A matter of taste, eventually, but I'd use a space.

+ + ret = ov6650_video_probe(icd, client); + + if (ret) { + icd->ops = NULL; + i2c_set_clientdata(client, NULL); + kfree(priv); + } + + return ret; +} + +static int ov6650_remove(struct i2c_client *client) +{ + struct ov6650 *priv = to_ov6650(client); + + i2c_set_clientdata(client, NULL); + kfree(priv); + return 0; +} + +static const struct i2c_device_id ov6650_id[] = { + { "ov6650", 0 }, + { } +}; +MODULE_DEVICE_TABLE(i2c, ov6650_id); + +static struct i2c_driver ov6650_i2c_driver = { + .driver = { + .name = "ov6650", + }, + .probe = ov6650_probe, + .remove = ov6650_remove, + .id_table = ov6650_id, +}; + +static int __init ov6650_module_init(void) +{ + return i2c_add_driver(&ov6650_i2c_driver); +} + +static void __exit ov6650_module_exit(void) +{ + i2c_del_driver(&ov6650_i2c_driver); +} + +module_init(ov6650_module_init); +module_exit(ov6650_module_exit); + +MODULE_DESCRIPTION("SoC Camera driver for OmniVision OV6650"); +MODULE_AUTHOR("Janusz Krzysztofik <jkrzyszt@tis.icnet.pl>"); +MODULE_LICENSE("GPL v2"); --- linux-2.6.35-rc3.orig/include/media/v4l2-chip-ident.h 2010-06-26 15:56:15.000000000 +0200 +++ linux-2.6.35-rc3/include/media/v4l2-chip-ident.h 2010-06-26 17:28:09.000000000 +0200 @@ -70,6 +70,7 @@ enum { V4L2_IDENT_OV9655 = 255, V4L2_IDENT_SOI968 = 256, V4L2_IDENT_OV9640 = 257, + V4L2_IDENT_OV6650 = 258,

/* module saa7146: reserved range 300-309 */ V4L2_IDENT_SAA7146 = 300,

Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/