OmniVision OV9655 camera chip via soc-camera interface

Hi,

I'm writing a driver for the OmniVision OV9655 camera chip connected to a
PXA270 processor. I based my work on the soc_camera interface, but I need some additional gpios for reset and power_enable. What is the best way to pass this information to the driver?

Hello, Stefan,

2008/4/14, Stefan Herbrechtsmeier <[email protected]>:

Quoted 8 lines

Hi,

I'm writing a driver for the OmniVision OV9655 camera chip connected to a
PXA270 processor. I based my work on the soc_camera interface, but I need some additional gpios for reset and power_enable. What is the best way to pass this information to the driver?

I don't know about soc_camera, but the stkwebcam driver has code to drive the ov9650 sensors. I'd like to use some generic interface in it, instead of the current code. IIRC, Mauro suggested v4l2-int-device when the driver was submitted.
Do you think your work could be used by stkwebcam? note that I don't know much about the syntek camera controller, and stkwebcam is a reverse engineered driver.

Anyway, maybe the code in drivers/media/video/stk-sensor.c is useful for you, so feel free to use it if you want.

Regards,
Jaime

I'm writing a driver for the OmniVision OV9655 camera chip connected to a PXA270 processor. I based my work on the soc_camera interface, but I need some additional gpios for reset and power_enable. What is the best way to pass this information to the driver?

Quoted 5 lines · Stefan Herbrechtsmeier

I'm writing a driver for the OmniVision OV9655 camera chip connected to a PXA270 processor. I based my work on the soc_camera interface, but I need some additional gpios for reset and power_enable. What is the best way to pass this information to the driver?

Look in pxa_camera.c, e.g., in pxa_camera_activate. There are function calls like

pxa_camera_activate(struct pxa_camera_dev *pcdev)

{
	struct pxacamera_platform_data *pdata = pcdev->pdata;

...

	pdata->power(pcdev->dev, 1);

...

	pdata->reset(pcdev->dev, 1);

in it, which should do exactly what you need. And they are supposed to be implemented in the platform, so, you have all the required GPIO information you need there. That is exactly the reason they are defined that way - because they were thought to be platform-dependent. Let me know if there's still anything missing. It is still a work in progress, so, we are flexible and can add any (reasonable) APIs we find useful.

Thanks
Guennadi
---
Guennadi Liakhovetski

Quoted 14 lines · Guennadi Liakhovetski

Look in pxa_camera.c, e.g., in pxa_camera_activate. There are function calls like

pxa_camera_activate(struct pxa_camera_dev *pcdev)

{
	struct pxacamera_platform_data *pdata = pcdev->pdata;

...

	pdata->power(pcdev->dev, 1);

...

Quoted 9 lines

	pdata->reset(pcdev->dev, 1);

in it, which should do exactly what you need. And they are supposed to be implemented in the platform, so, you have all the required GPIO information you need there. That is exactly the reason they are defined that way - because they were thought to be platform-dependent. Let me know if there's still anything missing. It is still a work in progress, so, we are flexible and can add any (reasonable) APIs we find useful.

Thanks, that exact what I search, but maybe this functions should be in the soc_camera_link. I think this functions belong to the camera chip and not to the capture interface. This allows more than one camera chip on one capture interface with separate enable and reset.

Quoted 24 lines · Stefan Herbrechtsmeier

Quoted 13 lines · Guennadi Liakhovetski

Look in pxa_camera.c, e.g., in pxa_camera_activate. There are function calls like

pxa_camera_activate(struct pxa_camera_dev *pcdev)

{
    struct pxacamera_platform_data *pdata = pcdev->pdata;

...

    pdata->power(pcdev->dev, 1);

...

Quoted 9 lines

    pdata->reset(pcdev->dev, 1);

in it, which should do exactly what you need. And they are supposed to be implemented in the platform, so, you have all the required GPIO information you need there. That is exactly the reason they are defined that way - because they were thought to be platform-dependent. Let me know if there's still anything missing. It is still a work in progress, so, we are flexible and can add any (reasonable) APIs we find useful.

Quoted 5 lines

Thanks, that exact what I search, but maybe this functions should be in the soc_camera_link. I think this functions belong to the camera chip and not to the capture interface. This allows more than one camera chip on one capture interface with separate enable and reset.

Well, in principle, yes, I think this is a good idea. But:

1. ATM these functions are called from the camera-host (pxa-camera) driver. And until now it knew nothing about soc_camera_link. Which is also good.

a) If we want to keep calls to these functions in the camera-host
driver, we'll either have to let it also handle soc_camera_link, or introduce some parameter to these functions to tell the platform which camera shall be resetted / powered on or off.

b) Alternatively, we could call these functions from soc_camera_ops
init() and release() methods. Actually, I think, this would be the best option.

2. Do you have a real-life example with several cameras on one interface? ATM pxa_camera is explicitely limited to handle only one camera on its quick capture interface. You would have to lift that restriction too.

Quoted 9 lines · Guennadi Liakhovetski

1. ATM these functions are called from the camera-host (pxa-camera) driver. And until now it knew nothing about soc_camera_link. Which is also good.

a) If we want to keep calls to these functions in the camera-host driver,
we'll either have to let it also handle soc_camera_link, or introduce some parameter to these functions to tell the platform which camera shall be resetted / powered on or off.

Quoted 4 lines

b) Alternatively, we could call these functions from soc_camera_ops
init() and release() methods. Actually, I think, this would be the best option.

This means moving the init() and reset() functions into the soc_camera_link. Is this right?

Quoted 4 lines

2. Do you have a real-life example with several cameras on one interface? ATM pxa_camera is explicitely limited to handle only one camera on its quick capture interface. You would have to lift that restriction too.

Not at the moment.

I have some addition suggestion for the soc_camera interface:

1. Renaming SOCAM_HSYNC_* to SOCAM_HREF_*
I think the current used Signal is HREF and not HSYNC.
- HREF is active during valid pixels
- HSYNC is a impulse at the start of each line before valid pixels and need some pixel skipping.

2. Add a new SOCAM_HSYNC_* to the soc_camera interface

3. Add x_skip_left to soc_camera_device
The pxa_camera has to skip some pixel at the begin of each line if a HSYNC signal is used.
(y_skip_top and x_skip_left can change with each format adjustment!)

4. Remove camera_init() call before camera_probe()
I think the driver should first detect the hardware before it do something with it.
The first hardware initialization should be done in the probe function.

Quoted 10 lines · Stefan Herbrechtsmeier

Quoted 8 lines · Guennadi Liakhovetski

1. ATM these functions are called from the camera-host (pxa-camera) driver. And until now it knew nothing about soc_camera_link. Which is also good.

a) If we want to keep calls to these functions in the camera-host driver,
we'll either have to let it also handle soc_camera_link, or introduce some parameter to these functions to tell the platform which camera shall be resetted / powered on or off.

Quoted 8 lines

Quoted 5 lines

b) Alternatively, we could call these functions from soc_camera_ops
init() and release() methods. Actually, I think, this would be the best option.

This means moving the init() and reset() functions into the soc_camera_link. Is this right?

Yes.

Quoted 17 lines

Quoted 5 lines

2. Do you have a real-life example with several cameras on one interface? ATM pxa_camera is explicitely limited to handle only one camera on its quick capture interface. You would have to lift that restriction too.

Not at the moment.

I have some addition suggestion for the soc_camera interface:

1. Renaming SOCAM_HSYNC_* to SOCAM_HREF_*
I think the current used Signal is HREF and not HSYNC.
- HREF is active during valid pixels
- HSYNC is a impulse at the start of each line before valid pixels and need some pixel skipping.

2. Add a new SOCAM_HSYNC_* to the soc_camera interface

Quoted 5 lines

3. Add x_skip_left to soc_camera_device
The pxa_camera has to skip some pixel at the begin of each line if a HSYNC signal is used.
(y_skip_top and x_skip_left can change with each format adjustment!)

How and why shall they change?

Quoted 6 lines

4. Remove camera_init() call before camera_probe()
I think the driver should first detect the hardware before it do something with it.
The first hardware initialization should be done in the probe function.

What camera_init do you mean? If you mean the ->add() call in soc_camera_open() then it is needed to activate the interface.

As usual, patches are welcome. When we see the code we can discuss it in detail.

Quoted 7 lines · Guennadi Liakhovetski

Quoted 6 lines

3. Add x_skip_left to soc_camera_device
The pxa_camera has to skip some pixel at the begin of each line if a HSYNC signal is used.
(y_skip_top and x_skip_left can change with each format adjustment!)

Quoted 2 lines

How and why shall they change?

They shall change to support both signal types and to make the names and function clear. The OmniVision chips
support both types and you can configure the VSYNC pin. At the moment I used SOCAM_HSYNC_*
but configure the chip to use HREF to work without pixel skipping.

Quoted 11 lines · Stefan Herbrechtsmeier

Quoted 9 lines · Guennadi Liakhovetski

Quoted 6 lines

3. Add x_skip_left to soc_camera_device
The pxa_camera has to skip some pixel at the begin of each line if a HSYNC signal is used.
(y_skip_top and x_skip_left can change with each format adjustment!)

How and why shall they change?

Quoted 6 lines

They shall change to support both signal types and to make the names and function clear. The OmniVision chips
support both types and you can configure the VSYNC pin. At the moment I used SOCAM_HSYNC_*
but configure the chip to use HREF to work without pixel skipping.

Sorry, I mean why y_skip_top and x_skip_left shall change?

Hi,

some of my soc_camera suggestions as Patch:

Quoted 5 lines

1. Renaming SOCAM_HSYNC_* to SOCAM_HREF_*
I think the current used Signal is HREF and not HSYNC.
- HREF is active during valid pixels
- HSYNC is a impulse at the start of each line before valid pixels and need some pixel skipping.

Hi, and thanks for the patch.

Now, the first thing you want to do is read Documentation/SubmittingPatches in your Linux kernel tree. Having read that, you should realise, that this your patch should in fact be 4 separate patches, that you must not replace TABs with spaces, and that added code should be formattet exactly like the rest - according to Documentation/CodingStyle. I am not sure how you created those diffs, but, I think, your mailer further corrupted your patch - it removed leading spaces from unmodified, including empty lines. This too makes patches unusable. To verify your patch-mail machinery is working correctly, you can create patches, then revert them, preserving backups per

patch -b -p1 -R < my.patch

mail the patch to yourself, extract it from the mail, apply it again per

patch -p1 < my-received.patch

(this time without backup) and compare with backuped versions. After this worked you may be somewhat sure your patches will arrive to the recepients undamaged. And, of course, you should compile- and run-test your patches, which you haven't done either.

Now, I split your patch into 4 pices to make commenting easier, and this is also how you should submit them after fixing all problems, but in separate emails, and adding Signed-off-by lines:

****************************************************************************
Patch 1
****************************************************************************

Quoted 4 lines · Stefan Herbrechtsmeier

Hi,

some of my soc_camera suggestions as Patch:

+ Moving power and reset function into soc_camera_link

---

diff -r dd4685496fb7 linux/drivers/media/video/mt9m001.c

linux/drivers/media/video/mt9m001.c Fri May 02 16:34:37 2008 +0200 renamed from linux/drivers/media/video/mt9m001.c Fri May 02 01:48:36 2008 -0300 +0 -0

@@ -120,0 +120,0 @@ static int reg_clear(struct soc_camera_d

static int mt9m001_init(struct soc_camera_device *icd)

^--- Here you see how the leading space has been dropped.
****************************************************************************

{
- int ret;

^--- Even in original code you replaced TABs with spaces...
****************************************************************************

    /* Disable chip, synchronous option update */
    dev_dbg(icd->vdev->dev, "%s\n", __func__);

linux/drivers/media/video/mt9m001.c Fri May 02 16:34:37 2008 +0200 renamed from linux/drivers/media/video/mt9m001.c Fri May 02 01:48:36 2008 -0300 +0 -0

@@ -136,0 +148,0 @@ static int mt9m001_init(struct soc_camer

static int mt9m001_release(struct soc_camera_device *icd) {

linux/drivers/media/video/mt9m001.c Fri May 02 16:34:37 2008 +0200 renamed from linux/drivers/media/video/mt9m001.c Fri May 02 01:48:36 2008 -0300 +7 -0

+    struct soc_camera_link *icl = icd->client->dev.platform_data;
+
    /* Disable the chip */
    reg_write(icd, MT9M001_OUTPUT_CONTROL, 0);
+
+    if (icl && icl->reset) {
+        dev_dbg(icd->dev, "%s: Asserting camera reset\n",
+            __func__);
+        icl->reset(icd->dev, 0);

I know the original code does this too, but I don't understand why you have to reset a camera when releasing it...

****************************************************************************

linux/drivers/media/video/mt9m001.c Fri May 02 16:34:37 2008 +0200 renamed from linux/drivers/media/video/mt9m001.c Fri May 02 01:48:36 2008 -0300 +7 -0

+    }
+
+    if (icl && icl->power) {
+        dev_dbg(icd->dev, "%s: Power off camera\n", __func__);
+        icl->power(icd->dev, 0);
+    }
+
    return 0;

}

diff -r dd4685496fb7 linux/drivers/media/video/mt9v022.c

linux/drivers/media/video/mt9v022.c Fri May 02 16:39:49 2008 +0200 renamed from linux/drivers/media/video/mt9v022.c Fri May 02 01:48:36 2008 -0300 +0 -0

@@ -137,0 +137,0 @@ static int mt9v022_init(struct soc_camer

static int mt9v022_init(struct soc_camera_device *icd)

{
    struct mt9v022 *mt9v022 = container_of(icd, struct mt9v022, icd);

linux/drivers/media/video/mt9v022.c Fri May 02 16:39:49 2008 +0200 renamed from linux/drivers/media/video/mt9v022.c Fri May 02 01:48:36 2008 -0300 +12 -0

+    struct soc_camera_link *icl = icd->client->dev.platform_data;
    int ret;
+
+    if (icl && icl->power) {
+        dev_dbg(icd->dev, "%s: Power on camera\n", __func__);
+        icl->power(icd->dev, 1);
+    }
+
+    if (icl && icd->reset) {
+        dev_dbg(icd->dev, "%s: Releasing camera reset\n",
+            __func__);
+        icl->reset(icd->dev, 1);
+    }
 
    /* Almost the default mode: master, parallel, simultaneous, and an
     * undocumented bit 0x200, which is present in table 7, but not in 8,

@@ -164,0 +176,0 @@ static int mt9v022_init(struct soc_camer

static int mt9v022_release(struct soc_camera_device *icd) {

linux/drivers/media/video/mt9v022.c Fri May 02 16:39:49 2008 +0200 renamed from linux/drivers/media/video/mt9v022.c Fri May 02 01:48:36 2008 -0300 +14 -0

+    struct soc_camera_link *icl = icd->client->dev.platform_data;
+
    /* Nothing? */
+
+    if (icl && icl->reset) {
+        dev_dbg(icd->dev, "%s: Asserting camera reset\n",
+            __func__);
+        icl->reset(icd->dev, 0);
+    }
+
+    if (icl && icl->power) {
+        dev_dbg(icd->dev, "%s: Power off camera\n", __func__);
+        icl->power(icd->dev, 0);
+    }
+
    return 0;

}

diff -r dd4685496fb7 linux/drivers/media/video/pxa_camera.c

linux/drivers/media/video/pxa_camera.c Fri May 02 16:16:36 2008 +0200 renamed from linux/drivers/media/video/pxa_camera.c Fri May 02 01:48:36 2008 -0300 +0 -11

@@ -613,2 +613,2 @@ static void pxa_camera_activate(struct p
        pdata->init(pcdev->dev);
    }
 
-    if (pdata && pdata->power) {
-        dev_dbg(pcdev->dev, "%s: Power on camera\n", __func__);
-        pdata->power(pcdev->dev, 1);
-    }
-
-    if (pdata && pdata->reset) {
-        dev_dbg(pcdev->dev, "%s: Releasing camera reset\n",
-            __func__);
-        pdata->reset(pcdev->dev, 1);
-    }
-
    CICR0 = 0x3FF;   /* disable all interrupts */
 
    if (pcdev->platform_flags & PXA_CAMERA_PCLK_EN)

@@ -644,0 +633,0 @@ static void pxa_camera_activate(struct p

static void pxa_camera_deactivate(struct pxa_camera_dev *pcdev) {

linux/drivers/media/video/pxa_camera.c Fri May 02 16:16:36 2008 +0200 renamed from linux/drivers/media/video/pxa_camera.c Fri May 02 01:48:36 2008 -0300 +0 -13

-    struct pxacamera_platform_data *board = pcdev->pdata;
-
    clk_disable(pcdev->clk);
-
-    if (board && board->reset) {
-        dev_dbg(pcdev->dev, "%s: Asserting camera reset\n",
-            __func__);
-        board->reset(pcdev->dev, 0);
-    }
-
-    if (board && board->power) {
-        dev_dbg(pcdev->dev, "%s: Power off camera\n", __func__);
-        board->power(pcdev->dev, 0);
-    }

}

static irqreturn_t pxa_camera_irq(int irq, void *data) diff -r dd4685496fb7 linux/include/asm-arm/arch-pxa/camera.h

linux/include/asm-arm/arch-pxa/camera.h Fri May 02 15:53:10 2008 +0200 renamed from linux/include/asm-arm/arch-pxa/camera.h Fri May 02 01:48:36 2008 -0300 +0 -0

@@ -36,0 +36,0 @@

struct pxacamera_platform_data {
    int (*init)(struct device *);

linux/include/asm-arm/arch-pxa/camera.h Fri May 02 15:53:10 2008 +0200 renamed from linux/include/asm-arm/arch-pxa/camera.h Fri May 02 01:48:36 2008 -0300 +0 -2

-    int (*power)(struct device *, int);
-    int (*reset)(struct device *, int);
 
    unsigned long flags;
    unsigned long mclk_10khz;

diff -r dd4685496fb7 linux/include/media/soc_camera.h

linux/include/media/soc_camera.h Fri May 02 15:57:06 2008 +0200 renamed from linux/include/media/soc_camera.h Fri May 02 01:48:36 2008 -0300 +0 -0

@@ -79,0 +80,0 @@ struct soc_camera_host_ops {

};

struct soc_camera_link {

linux/include/media/soc_camera.h Fri May 02 15:57:06 2008 +0200 renamed from linux/include/media/soc_camera.h Fri May 02 01:48:36 2008 -0300 +3 -0

+    int (*power)(struct device *, int);
+    int (*reset)(struct device *, int);
+
    /* Camera bus id, used to match a camera and a bus */
    int bus_id;
    /* GPIO number to switch between 8 and 10 bit modes */

****************************************************************************
Patch 2
****************************************************************************

+ Rename SOCAM_HSYNC_* to SOCAM_HREF_* and introduce new SOCAM_HSYNC_*

This is what you've written in your earlier email:

Quoted 6 lines

Quoted 6 lines

1. Renaming SOCAM_HSYNC_* to SOCAM_HREF_*
I think the current used Signal is HREF and not HSYNC.
- HREF is active during valid pixels
- HSYNC is a impulse at the start of each line before valid pixels and need some pixel skipping.

and the signal used on PXA270 _is_ HSYNC - also according to your definition, it's only that wait counts have been set to 0, so, the signal has become equivalent to HREF. And now that you support wait count != 0, it _is_ becoming HSYNC and not HREF. Further, the macros are used to set signal polarity, regardless of whether waits are used or not. So, if we ever get a SoC, where HSYNC and HREF polarity can be controlled separately and we will want to support that - _then_ we'll need these new macros. For now, I think, you just need to support non-zero wait counts and don't need to change names / introduce new ones. So, I'm dropping this one for now.

****************************************************************************
Patch 3
****************************************************************************

+ Add x_skip_left to soc_camera_device and to pxa_camera driver

Right, this is all you need to correctly read out line data from your camera, no HSYNC / HREF renaming or adding.

---

diff -r dd4685496fb7 linux/drivers/media/video/mt9m001.c

    /* Default datawidth - this is the only width this camera (normally)

diff -r dd4685496fb7 linux/drivers/media/video/mt9v022.c

linux/drivers/media/video/mt9v022.c Fri May 02 16:39:49 2008 +0200 renamed from linux/drivers/media/video/mt9v022.c Fri May 02 01:48:36 2008 -0300 +1 -0

@@ -784,6 +810,7 @@ static int mt9v022_probe(struct i2c_clie
    icd->width_max    = 752;
    icd->height_min    = 32;
    icd->height_max    = 480;
+    icd->x_skip_left = 0;
    icd->y_skip_top    = 1;
    icd->iface    = icl->bus_id;
    /* Default datawidth - this is the only width this camera (normally)

diff -r dd4685496fb7 linux/drivers/media/video/pxa_camera.c

linux/drivers/media/video/pxa_camera.c Fri May 02 16:16:36 2008 +0200 renamed from linux/drivers/media/video/pxa_camera.c Fri May 02 01:48:36 2008 -0300 +0 -0

@@ -883,1 +874,1 @@ static int pxa_camera_set_bus_param(stru
    }

    CICR1 = cicr1;

linux/drivers/media/video/pxa_camera.c Fri May 02 16:16:36 2008 +0200 renamed from linux/drivers/media/video/pxa_camera.c Fri May 02 01:48:36 2008 -0300 +1 -1

-    CICR2 = 0;
+    CICR2 = CICR2_BLW_VAL(min((unsigned short)255, icd->x_skip_left));
    CICR3 = CICR3_LPF_VAL(icd->height - 1) |
        CICR3_BFW_VAL(min((unsigned short)255, icd->y_skip_top));
    CICR4 = mclk_get_divisor(pcdev) | cicr4;

diff -r dd4685496fb7 linux/include/media/soc_camera.h

linux/include/media/soc_camera.h Fri May 02 15:57:06 2008 +0200 renamed from linux/include/media/soc_camera.h Fri May 02 01:48:36 2008 -0300 +1 -0

@@ -29,6 +29,7 @@ struct soc_camera_device {
    unsigned short width_max;
    unsigned short height_min;
    unsigned short height_max;
+    unsigned short x_skip_left;    /* Pixel to skip at the left */
    unsigned short y_skip_top;    /* Lines to skip at the top */
    unsigned short gain;
    unsigned short exposure;

****************************************************************************
Patch 4
****************************************************************************

+ Change defrect.width and height to icd->width_max in soc_camera_cropcap

Hm, I'm not convinced. this is the _default rectangle_, max and min are set in "bounds", and I think the VGA 640x480 size is a reasonable default.

---

diff -r dd4685496fb7 linux/drivers/media/video/soc_camera.c

linux/drivers/media/video/soc_camera.c Mon Apr 28 18:34:49 2008 +0200 renamed from linux/drivers/media/video/soc_camera.c Fri May 02 01:48:36 2008 -0300 +2 -2

@@ -558,7 +558,7 @@ static int soc_camera_cropcap(struct fil
    a->bounds.height        = icd->height_max;
    a->defrect.left            = icd->x_min;
    a->defrect.top            = icd->y_min;
-    a->defrect.width        = 640;
-    a->defrect.height        = 480;
+    a->defrect.width        = icd->width_max;
+    a->defrect.height        = icd->height_max;
    a->pixelaspect.numerator    = 1;
    a->pixelaspect.denominator    = 1;

So, I would say, patches 1 and 3 look useful to me. Please fix formatting issues, add your Signed-off-by and submit in two separate emails.

Review

Quoted 5 lines

(quoted diff) +4 -0

+        dev_dbg(icd->dev, "%s: Releasing camera reset\n",
+            __func__);
+        icl->reset(icd->dev, 1);
+    }

It is right to use the icd->dev for the init/reset function or should this be icd->client->dev?

Quoted 14 lines

(quoted diff) +0 -0

@@ -136,0 +148,0 @@ static int mt9m001_init(struct soc_camer

static int mt9m001_release(struct soc_camera_device *icd) {

(quoted diff) +7 -0

+    struct soc_camera_link *icl = icd->client->dev.platform_data;
+
    /* Disable the chip */
    reg_write(icd, MT9M001_OUTPUT_CONTROL, 0);
+
+    if (icl && icl->reset) {
+        dev_dbg(icd->dev, "%s: Asserting camera reset\n",
+            __func__);
+        icl->reset(icd->dev, 0);

Quoted 3 lines

I know the original code does this too, but I don't understand why you have to reset a camera when releasing it...

I thing this is done to saving the time for a reset impulse. The mt9m001_init() only release this pin.
But this depends on the implementation of reset(). It has a value parameter so I think it changes the
state of the pin depending on the value. Or should it reset the chip irrespective of the value?
Maybe we should remove the value from the reset function to make things clear.

Some other question: Should I skip the soft reset if a hard reset is present?

Quoted 18 lines

****************************************************************************
Patch 2
****************************************************************************

+ Rename SOCAM_HSYNC_* to SOCAM_HREF_* and introduce new SOCAM_HSYNC_*

This is what you've written in your earlier email:

Quoted 7 lines

Quoted 7 lines

1. Renaming SOCAM_HSYNC_* to SOCAM_HREF_*
I think the current used Signal is HREF and not HSYNC.
- HREF is active during valid pixels
- HSYNC is a impulse at the start of each line before valid pixels and need
some pixel skipping.

Quoted 10 lines

and the signal used on PXA270 _is_ HSYNC - also according to your definition, it's only that wait counts have been set to 0, so, the signal has become equivalent to HREF. And now that you support wait count != 0, it _is_ becoming HSYNC and not HREF. Further, the macros are used to set signal polarity, regardless of whether waits are used or not. So, if we ever get a SoC, where HSYNC and HREF polarity can be controlled separately and we will want to support that - _then_ we'll need these new macros. For now, I think, you just need to support non-zero wait counts and don't need to change names / introduce new ones. So, I'm dropping this one for now.

At the OV9655 the HREF pin can configure as HREF or as HSYNC. Thats the reason why I change
this. I will resubmit it together with my OV9655 driver.

Quoted 5 lines

diff -r dd4685496fb7 linux/drivers/media/video/mt9m001.c

linux/drivers/media/video/mt9m001.c Fri May 02 16:34:37 2008 +0200 renamed from linux/drivers/media/video/mt9m001.c Fri May 02 01:48:36 2008 -0300 +0 -0

@@ -659,6 +685,7 @@ static int mt9m001_probe(struct i2c_clie
    icd->width_max    = 1280;

linux/drivers/media/video/mt9m001.c Fri May 02 16:34:37 2008 +0200 static int mt9m001_probe(struct i2c_clie

Quoted 6 lines

linux/drivers/media/video/mt9m001.c Fri May 02 16:34:37 2008 +0200 renamed from linux/drivers/media/video/mt9m001.c Fri May 02 01:48:36 2008 -0300 +1 -0

    icd->height_min    = 32;
    icd->height_max    = 1024;
+    icd->x_skip_left = 0;
    icd->y_skip_top    = 1;
    icd->iface    = icl->bus_id;
 

My new variable is longer than the others and break the tab formation. What is the best to do?

Quoted 3 lines

So, I would say, patches 1 and 3 look useful to me. Please fix formatting issues, add your Signed-off-by and submit in two separate emails.

At the moment I update my system to the 2.6.25 kernel. When everything works fine, I submit the reworked
patches the next days.

Quoted 4 lines · Guennadi Liakhovetski

So, I would say, patches 1 and 3 look useful to me. Please fix formatting issues, add your Signed-off-by and submit in two separate emails.

At the moment I update my system to the 2.6.25 kernel. When everything works fine, I submit the reworked
patches the next days.

Hi Stefan,

Quoted 6 lines · Stefan Herbrechtsmeier

Quoted 4 lines · Guennadi Liakhovetski

So, I would say, patches 1 and 3 look useful to me. Please fix formatting issues, add your Signed-off-by and submit in two separate emails.

Quoted 4 lines

At the moment I update my system to the 2.6.25 kernel. When everything works fine, I submit the reworked
patches the next days.

How is this going? Do you have your patches ready? We have a few patches brewing from sveral people, that touch the same code as yours, so, it would be good to have your "move .power, .reset to camera-link" patch applied, then we could move on with the others.

Quoted 14 lines · Guennadi Liakhovetski

Quoted 10 lines · Stefan Herbrechtsmeier

Quoted 4 lines · Guennadi Liakhovetski

So, I would say, patches 1 and 3 look useful to me. Please fix formatting
issues, add your Signed-off-by and submit in two separate emails.

At the moment I update my system to the 2.6.25 kernel. When everything works
fine, I submit the reworked
patches the next days.

Quoted 5 lines

How is this going? Do you have your patches ready? We have a few patches brewing from sveral people, that touch the same code as yours, so, it would be good to have your "move .power, .reset to camera-link" patch applied, then we could move on with the others.

Sorry for the long delay. I’m waiting for some answer form OmniVision if I’m allowed to publish my current version of the OV9655 driver or whether something violent the NDA.

Should I resend my "move .power, .reset to camera-link" patch without the driver or waiting until I can send my OV9655 driver? At the moment there is no driver which uses this.

I have also implement some simple timeperframe configuration (vidioc_s_parm) via clock PLL and divider configuration in OV9655 driver, but only with a fix mclk_10khz of 2600.

Hi Stefan,

Quoted 13 lines · Stefan Herbrechtsmeier

Quoted 12 lines · Guennadi Liakhovetski

Quoted 10 lines · Stefan Herbrechtsmeier

Quoted 4 lines · Guennadi Liakhovetski

So, I would say, patches 1 and 3 look useful to me. Please fix formatting
issues, add your Signed-off-by and submit in two separate emails.

At the moment I update my system to the 2.6.25 kernel. When everything works
fine, I submit the reworked
patches the next days.

Quoted 8 lines

Quoted 5 lines

How is this going? Do you have your patches ready? We have a few patches brewing from sveral people, that touch the same code as yours, so, it would be good to have your "move .power, .reset to camera-link" patch applied, then we could move on with the others.

Sorry for the long delay. I’m waiting for some answer form OmniVision if I’m allowed to publish my current version of the OV9655 driver or whether something violent the NDA.

Yes, this is an interesting question...

Quoted 3 lines

Should I resend my "move .power, .reset to camera-link" patch without the driver or waiting until I can send my OV9655 driver? At the moment there is no driver which uses this.

I think, yes, it would be useful to move .reset and .power now, as I said, I keep telling people "please keep in mind these fields are going to move", so, we shall really finally move them:-)

Quoted 3 lines

I have also implement some simple timeperframe configuration (vidioc_s_parm) via clock PLL and divider configuration in OV9655 driver, but only with a fix mclk_10khz of 2600.

Good! If this is ov9655-specific, let's wait until you're allowed to submit your patch:-)

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Quoted 32 lines · Guennadi Liakhovetski

Hi Stefan,

Quoted 13 lines · Stefan Herbrechtsmeier

Quoted 12 lines · Guennadi Liakhovetski

Quoted 10 lines · Stefan Herbrechtsmeier

Quoted 4 lines · Guennadi Liakhovetski

So, I would say, patches 1 and 3 look useful to me. Please fix formatting
issues, add your Signed-off-by and submit in two separate emails.

At the moment I update my system to the 2.6.25 kernel. When everything works
fine, I submit the reworked
patches the next days.

Quoted 8 lines

Quoted 5 lines

How is this going? Do you have your patches ready? We have a few patches brewing from sveral people, that touch the same code as yours, so, it would be good to have your "move .power, .reset to camera-link" patch applied, then we could move on with the others.

Sorry for the long delay. I’m waiting for some answer form OmniVision if I’m allowed to publish my current version of the OV9655 driver or whether something violent the NDA.

Yes, this is an interesting question...

Quoted 3 lines

Should I resend my "move .power, .reset to camera-link" patch without the driver or waiting until I can send my OV9655 driver? At the moment there is no driver which uses this.

Quoted 1 line

I think, yes, it would be useful to move .reset and .power now, as I said, I keep telling people "please keep in mind these fields are going to move", so, we shall really finally move them:-)

I am waiting for this also! Add me to the list :)
Currently I have mclk parameter for camera host driver... And I have to set it frequently since I am playing with couple different sensors...

Quoted 19 lines

Quoted 5 lines

I have also implement some simple timeperframe configuration (vidioc_s_parm) via clock PLL and divider configuration in OV9655 driver, but only with a fix mclk_10khz of 2600.

Good! If this is ov9655-specific, let's wait until you're allowed to submit your patch:-)

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Signature · 3 lines

video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Quoted 7 lines

Quoted 3 lines

I think, yes, it would be useful to move .reset and .power now, as I said, I keep telling people "please keep in mind these fields are going to move", so, we shall really finally move them:-)

I am waiting for this also! Add me to the list :)
Currently I have mclk parameter for camera host driver... And I have to set it frequently since I am playing with couple different sensors...

My current version only touch the soc_camera files. Should I change the mt9m001 and mt9v022 sensor to use the new functions?