OmniVision OV9655 camera chip via soc-camera interface

OmniVision OV9655 camera chip via soc-camera interface

[Stefan Herbrechtsmeier]

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?

Thanks
    Stefan

-- 
Dipl.-Ing. Stefan Herbrechtsmeier

Heinz Nixdorf Institute
University of Paderborn 
System and Circuit Technology 
Fürstenallee 11
D-33102 Paderborn (Germany)

office : F0.415
phone  : + 49 5251 - 60 6342
fax    : + 49 5251 - 60 6351

mailto : hbmeier@hni.upb.de

www    : http://wwwhni.upb.de/sct/mitarbeiter/hbmeier


--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: OmniVision OV9655 camera chip via soc-camera interface

[Jaime Velasco]

Hello, Stefan,

2008/4/14, Stefan Herbrechtsmeier <hbmeier@hni.uni-paderborn.de>:
> 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

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: OmniVision OV9655 camera chip via soc-camera interface

[Guennadi Liakhovetski]

On Mon, 14 Apr 2008, Stefan Herbrechtsmeier wrote:

> 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

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: OmniVision OV9655 camera chip via soc-camera interface

[Stefan Herbrechtsmeier]

Hi Jaime,

Jaime Velasco schrieb:
> Hello, Stefan,
>
> 2008/4/14, Stefan Herbrechtsmeier <hbmeier@hni.uni-paderborn.de>:
>   
>> 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.
>   
I use the soc_camera interface, because I can reused the pxa_camera 
driver. If we want to use the same driver for the stkwebcam, we have to 
implement a generic interface for the register access and split the 
OV9655 driver or a i2c bus driver for the syntek camera controller.
> 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
>   
Thanks
    Stefan

-- 
Dipl.-Ing. Stefan Herbrechtsmeier

Heinz Nixdorf Institute
University of Paderborn 
System and Circuit Technology 
Fürstenallee 11
D-33102 Paderborn (Germany)

office : F0.415
phone  : + 49 5251 - 60 6342
fax    : + 49 5251 - 60 6351

mailto : hbmeier@hni.upb.de

www    : http://wwwhni.upb.de/sct/mitarbeiter/hbmeier


--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: OmniVision OV9655 camera chip via soc-camera interface

[Stefan Herbrechtsmeier]

Guennadi Liakhovetski schrieb:
> On Mon, 14 Apr 2008, Stefan Herbrechtsmeier wrote:
>
>   
>> 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, 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.
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski
>   
Thanks
    Stefan

-- 
Dipl.-Ing. Stefan Herbrechtsmeier

Heinz Nixdorf Institute
University of Paderborn 
System and Circuit Technology 
Fürstenallee 11
D-33102 Paderborn (Germany)

office : F0.415
phone  : + 49 5251 - 60 6342
fax    : + 49 5251 - 60 6351

mailto : hbmeier@hni.upb.de

www    : http://wwwhni.upb.de/sct/mitarbeiter/hbmeier


--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: OmniVision OV9655 camera chip via soc-camera interface

[Guennadi Liakhovetski]

On Tue, 15 Apr 2008, Stefan Herbrechtsmeier wrote:

> Guennadi Liakhovetski schrieb:
> > 
> > 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, 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.

So, I think, a patch implementing 1.b could be considered.

Thanks
Guennadi
---
Guennadi Liakhovetski

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: OmniVision OV9655 camera chip via soc-camera interface

[Stefan Herbrechtsmeier]

Sorry for the late answers, but first I want to get my ov9655 driver 
work prober.

Guennadi Liakhovetski schrieb:
> On Tue, 15 Apr 2008, Stefan Herbrechtsmeier wrote:
>
>  
>> Guennadi Liakhovetski schrieb:
>>    
>>> 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, 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.
>   
This means moving the init() and reset() functions into the 
soc_camera_link. Is this right?
> 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.

Regards,
   Stefan

-- 
Dipl.-Ing. Stefan Herbrechtsmeier

Heinz Nixdorf Institute
University of Paderborn 
System and Circuit Technology 
Fürstenallee 11
D-33102 Paderborn (Germany)

office : F0.415
phone  : + 49 5251 - 60 6342
fax    : + 49 5251 - 60 6351

mailto : hbmeier@hni.upb.de

www    : http://wwwhni.upb.de/sct/mitarbeiter/hbmeier


--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: OmniVision OV9655 camera chip via soc-camera interface

[Guennadi Liakhovetski]

You forgot to cc the list, readded.

On Fri, 2 May 2008, Stefan Herbrechtsmeier wrote:

> Guennadi Liakhovetski schrieb:
> > 
> > 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.
> >   
> This means moving the init() and reset() functions into the soc_camera_link.
> Is this right?

Yes.

> > 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!)

How and why shall they change?

> 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.

Thanks
Guennadi
---
Guennadi Liakhovetski

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: OmniVision OV9655 camera chip via soc-camera interface

[Stefan Herbrechtsmeier]

Guennadi Liakhovetski schrieb:
> On Fri, 2 May 2008, Stefan Herbrechtsmeier wrote:
>
>   
>> Guennadi Liakhovetski schrieb:
>>     
>>> 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.
>>>   
>>>       
>> This means moving the init() and reset() functions into the soc_camera_link.
>> Is this right?
>>     
>
> Yes.
>
>   
>>> 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!)
>>     
>
> 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.
>   
>> 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.
>   
I mean the ->add() call in soc_camera_probe(). We first add the device 
and then probe. But I see
the problem with the external clock activation, witch it possibly need 
for probing.
> As usual, patches are welcome. When we see the code we can discuss it in 
> detail.
>   
I try to post some patches, but it's the first time for me.

Regards
    Stefan

-- 
Dipl.-Ing. Stefan Herbrechtsmeier

Heinz Nixdorf Institute
University of Paderborn 
System and Circuit Technology 
Fürstenallee 11
D-33102 Paderborn (Germany)

office : F0.415
phone  : + 49 5251 - 60 6342
fax    : + 49 5251 - 60 6351

mailto : hbmeier@hni.upb.de

www    : http://wwwhni.upb.de/sct/mitarbeiter/hbmeier


--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: OmniVision OV9655 camera chip via soc-camera interface

[Guennadi Liakhovetski]

On Fri, 2 May 2008, Stefan Herbrechtsmeier wrote:

> Guennadi Liakhovetski schrieb:
> > > 
> > > 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?
> >   
> 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?

Thanks
Guennadi
---
Guennadi Liakhovetski

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: OmniVision OV9655 camera chip via soc-camera interface

[Stefan Herbrechtsmeier]

Guennadi Liakhovetski schrieb:
> On Fri, 2 May 2008, Stefan Herbrechtsmeier wrote:
>
>   
>> Guennadi Liakhovetski schrieb:
>>     
>>>> 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?
>>>   
>>>       
>> 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?
>
>   
At the time I used VSYNC (without x_skip_left) I have different number 
of empty pixels
at the begin of each line for different resolutions. But I haven't 
really evaluate this.

Regards
    Stefan

-- 
Dipl.-Ing. Stefan Herbrechtsmeier

Heinz Nixdorf Institute
University of Paderborn 
System and Circuit Technology 
Fürstenallee 11
D-33102 Paderborn (Germany)

office : F0.415
phone  : + 49 5251 - 60 6342
fax    : + 49 5251 - 60 6351

mailto : hbmeier@hni.upb.de

www    : http://wwwhni.upb.de/sct/mitarbeiter/hbmeier


--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

[PATCH] Some suggestions for the soc_camera interface

[Stefan Herbrechtsmeier]

Hi,

some of my soc_camera suggestions as Patch:
+ Moving power and reset function into soc_camera_link
+ Rename SOCAM_HSYNC_* to SOCAM_HREF_* and introduce new
    SOCAM_HSYNC_*
+ Add x_skip_left to soc_camera_device and to pxa_camera driver
+ Change defrect.width and height to icd->width_max in soc_camera_cropcap

diff -r dd4685496fb7 linux/drivers/media/video/mt9m001.c
--- a/linux/drivers/media/video/mt9m001.c    Fri May 02 01:48:36 2008 -0300
+++ b/linux/drivers/media/video/mt9m001.c    Fri May 02 16:34:37 2008 +0200
@@ -120,7 +120,19 @@ static int reg_clear(struct soc_camera_d
 
 static int mt9m001_init(struct soc_camera_device *icd)
 {
-    int ret;
+    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);
+    }
 
     /* Disable chip, synchronous option update */
     dev_dbg(icd->vdev->dev, "%s\n", __func__);
@@ -136,8 +148,22 @@ static int mt9m001_init(struct soc_camer
 
 static int mt9m001_release(struct soc_camera_device *icd)
 {
+    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);
+    }
+
+    if (icl && icl->power) {
+        dev_dbg(icd->dev, "%s: Power off camera\n", __func__);
+        icl->power(icd->dev, 0);
+    }
+
     return 0;
 }
 
@@ -255,8 +281,8 @@ static unsigned long mt9m001_query_bus_p
 
     /* MT9M001 has all capture_format parameters fixed */
     return SOCAM_PCLK_SAMPLE_RISING |
-        SOCAM_HSYNC_ACTIVE_HIGH |
-        SOCAM_VSYNC_ACTIVE_HIGH |
+        SOCAM_HREF_ACTIVE_HIGH |
+        SOCAM_VREF_ACTIVE_HIGH |
         SOCAM_MASTER |
         width_flag;
 }
@@ -659,6 +685,7 @@ static int mt9m001_probe(struct i2c_clie
     icd->width_max    = 1280;
     icd->height_min    = 32;
     icd->height_max    = 1024;
+    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/mt9v022.c
--- a/linux/drivers/media/video/mt9v022.c    Fri May 02 01:48:36 2008 -0300
+++ b/linux/drivers/media/video/mt9v022.c    Fri May 02 16:39:49 2008 +0200
@@ -137,7 +137,19 @@ 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);
+    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,7 +176,21 @@ static int mt9v022_init(struct soc_camer
 
 static int mt9v022_release(struct soc_camera_device *icd)
 {
+    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;
 }
 
@@ -280,7 +306,7 @@ static int mt9v022_set_bus_param(struct
     if (flags & SOCAM_PCLK_SAMPLE_RISING)
         pixclk |= 0x10;
 
-    if (!(flags & SOCAM_HSYNC_ACTIVE_HIGH))
+    if (!(flags & SOCAM_HREF_ACTIVE_HIGH))
         pixclk |= 0x1;
 
     if (!(flags & SOCAM_VSYNC_ACTIVE_HIGH))
@@ -312,7 +338,7 @@ static unsigned long mt9v022_query_bus_p
         width_flag |= SOCAM_DATAWIDTH_8;
 
     return SOCAM_PCLK_SAMPLE_RISING | SOCAM_PCLK_SAMPLE_FALLING |
-        SOCAM_HSYNC_ACTIVE_HIGH | SOCAM_HSYNC_ACTIVE_LOW |
+        SOCAM_HREF_ACTIVE_HIGH | SOCAM_HREF_ACTIVE_LOW |
         SOCAM_VSYNC_ACTIVE_HIGH | SOCAM_VSYNC_ACTIVE_LOW |
         SOCAM_MASTER | SOCAM_SLAVE |
         width_flag;
@@ -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
--- a/linux/drivers/media/video/pxa_camera.c    Fri May 02 01:48:36 2008 
-0300
+++ b/linux/drivers/media/video/pxa_camera.c    Fri May 02 16:16:36 2008 
+0200
@@ -613,17 +613,6 @@ 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,20 +633,7 @@ static void pxa_camera_activate(struct p
 
 static void pxa_camera_deactivate(struct pxa_camera_dev *pcdev)
 {
-    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)
@@ -752,6 +728,8 @@ static int test_platform_param(struct px
           SOCAM_MASTER : SOCAM_SLAVE) |
         SOCAM_HSYNC_ACTIVE_HIGH |
         SOCAM_HSYNC_ACTIVE_LOW |
+        SOCAM_HREF_ACTIVE_HIGH |
+        SOCAM_HREF_ACTIVE_LOW |
         SOCAM_VSYNC_ACTIVE_HIGH |
         SOCAM_VSYNC_ACTIVE_LOW |
         SOCAM_PCLK_SAMPLE_RISING |
@@ -799,12 +777,24 @@ static int pxa_camera_set_bus_param(stru
     pcdev->channels = 1;
 
     /* Make choises, based on platform preferences */
+    if ((common_flags & SOCAM_HREF_ACTIVE_HIGH) &&
+        (common_flags & SOCAM_HREF_ACTIVE_LOW)) {
+        if (pcdev->platform_flags & PXA_CAMERA_HSP)
+            common_flags &= ~SOCAM_HREF_ACTIVE_HIGH;
+        else
+            common_flags &= ~SOCAM_HREF_ACTIVE_LOW;
+        common_flags &= ~SOCAM_HSYNC_ACTIVE_HIGH &&
+                ~SOCAM_HSYNC_ACTIVE_LOW;
+    }
+
     if ((common_flags & SOCAM_HSYNC_ACTIVE_HIGH) &&
         (common_flags & SOCAM_HSYNC_ACTIVE_LOW)) {
         if (pcdev->platform_flags & PXA_CAMERA_HSP)
             common_flags &= ~SOCAM_HSYNC_ACTIVE_HIGH;
         else
             common_flags &= ~SOCAM_HSYNC_ACTIVE_LOW;
+        common_flags &= ~SOCAM_HREF_ACTIVE_HIGH &&
+                ~SOCAM_HREF_ACTIVE_LOW;
     }
 
     if ((common_flags & SOCAM_VSYNC_ACTIVE_HIGH) &&
@@ -855,7 +845,8 @@ static int pxa_camera_set_bus_param(stru
         cicr4 |= CICR4_MCLK_EN;
     if (common_flags & SOCAM_PCLK_SAMPLE_FALLING)
         cicr4 |= CICR4_PCP;
-    if (common_flags & SOCAM_HSYNC_ACTIVE_LOW)
+    if ((common_flags & SOCAM_HSYNC_ACTIVE_LOW) ||
+        (common_flags & SOCAM_HREF_ACTIVE_LOW))
         cicr4 |= CICR4_HSP;
     if (common_flags & SOCAM_VSYNC_ACTIVE_LOW)
         cicr4 |= CICR4_VSP;
@@ -883,7 +874,7 @@ static int pxa_camera_set_bus_param(stru
     }
 
     CICR1 = cicr1;
-    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/drivers/media/video/soc_camera.c
--- a/linux/drivers/media/video/soc_camera.c    Fri May 02 01:48:36 2008 
-0300
+++ b/linux/drivers/media/video/soc_camera.c    Mon Apr 28 18:34:49 2008 
+0200
@@ -558,8 +558,8 @@ 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;
 
diff -r dd4685496fb7 linux/include/asm-arm/arch-pxa/camera.h
--- a/linux/include/asm-arm/arch-pxa/camera.h    Fri May 02 01:48:36 
2008 -0300
+++ b/linux/include/asm-arm/arch-pxa/camera.h    Fri May 02 15:53:10 
2008 +0200
@@ -36,8 +36,6 @@
 
 struct pxacamera_platform_data {
     int (*init)(struct device *);
-    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
--- a/linux/include/media/soc_camera.h    Fri May 02 01:48:36 2008 -0300
+++ b/linux/include/media/soc_camera.h    Fri May 02 15:57:06 2008 +0200
@@ -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;
@@ -79,6 +80,9 @@ struct soc_camera_host_ops {
 };
 
 struct soc_camera_link {
+    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 */
@@ -149,8 +153,8 @@ static inline struct v4l2_queryctrl cons
 
 #define SOCAM_MASTER            (1 << 0)
 #define SOCAM_SLAVE            (1 << 1)
-#define SOCAM_HSYNC_ACTIVE_HIGH        (1 << 2)
-#define SOCAM_HSYNC_ACTIVE_LOW        (1 << 3)
+#define SOCAM_HREF_ACTIVE_HIGH        (1 << 2)
+#define SOCAM_HREF_ACTIVE_LOW        (1 << 3)
 #define SOCAM_VSYNC_ACTIVE_HIGH        (1 << 4)
 #define SOCAM_VSYNC_ACTIVE_LOW        (1 << 5)
 #define SOCAM_DATAWIDTH_8        (1 << 6)
@@ -158,6 +162,8 @@ static inline struct v4l2_queryctrl cons
 #define SOCAM_DATAWIDTH_10        (1 << 8)
 #define SOCAM_PCLK_SAMPLE_RISING    (1 << 9)
 #define SOCAM_PCLK_SAMPLE_FALLING    (1 << 10)
+#define SOCAM_HSYNC_ACTIVE_HIGH        (1 << 11)
+#define SOCAM_HSYNC_ACTIVE_LOW        (1 << 12)
 
 #define SOCAM_DATAWIDTH_MASK (SOCAM_DATAWIDTH_8 | SOCAM_DATAWIDTH_9 | \
                   SOCAM_DATAWIDTH_10)
@@ -165,15 +171,16 @@ static inline unsigned long soc_camera_b
 static inline unsigned long soc_camera_bus_param_compatible(
             unsigned long camera_flags, unsigned long bus_flags)
 {
-    unsigned long common_flags, hsync, vsync, pclk;
+    unsigned long common_flags, hsync, href, vsync, pclk;
 
     common_flags = camera_flags & bus_flags;
 
     hsync = common_flags & (SOCAM_HSYNC_ACTIVE_HIGH | 
SOCAM_HSYNC_ACTIVE_LOW);
+    href = common_flags & (SOCAM_HREF_ACTIVE_HIGH | SOCAM_HREF_ACTIVE_LOW);
     vsync = common_flags & (SOCAM_VSYNC_ACTIVE_HIGH | 
SOCAM_VSYNC_ACTIVE_LOW);
     pclk = common_flags & (SOCAM_PCLK_SAMPLE_RISING | 
SOCAM_PCLK_SAMPLE_FALLING);
 
-    return (!hsync || !vsync || !pclk) ? 0 : common_flags;
+    return ((!hsync && !href) || !vsync || !pclk) ? 0 : common_flags;
 }
 
 #endif

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Some suggestions for the soc_camera interface

[Guennadi Liakhovetski]

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
****************************************************************************


On Fri, 2 May 2008, Stefan Herbrechtsmeier wrote:

> 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
--- a/linux/drivers/media/video/mt9m001.c    Fri May 02 01:48:36 2008 -0300
+++ b/linux/drivers/media/video/mt9m001.c    Fri May 02 16:34:37 2008 +0200
@@ -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...
****************************************************************************

+    struct soc_camera_link *icl = icd->client->dev.platform_data;
+    int ret;
+
+    if (icl && icl->power) {

         ^--- icl is guaranteed to be non-NULL in both mt9m001 and mt9v022
****************************************************************************

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

                ^--- Haven't compile-tested
****************************************************************************

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

    /* Disable chip, synchronous option update */
    dev_dbg(icd->vdev->dev, "%s\n", __func__);
@@ -136,0 +148,0 @@ static int mt9m001_init(struct soc_camer

static int mt9m001_release(struct soc_camera_device *icd)
{
+    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...
****************************************************************************

+    }
+
+    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
--- a/linux/drivers/media/video/mt9v022.c    Fri May 02 01:48:36 2008 -0300
+++ b/linux/drivers/media/video/mt9v022.c    Fri May 02 16:39:49 2008 +0200
@@ -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);
+    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)
{
+    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
--- a/linux/drivers/media/video/pxa_camera.c    Fri May 02 01:48:36 2008 -0300
+++ b/linux/drivers/media/video/pxa_camera.c    Fri May 02 16:16:36 2008 +0200
@@ -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)
{
-    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
--- a/linux/include/asm-arm/arch-pxa/camera.h    Fri May 02 01:48:36 2008 -0300
+++ b/linux/include/asm-arm/arch-pxa/camera.h    Fri May 02 15:53:10 2008 +0200
@@ -36,0 +36,0 @@

struct pxacamera_platform_data {
    int (*init)(struct device *);
-    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
--- a/linux/include/media/soc_camera.h    Fri May 02 01:48:36 2008 -0300
+++ b/linux/include/media/soc_camera.h    Fri May 02 15:57:06 2008 +0200
@@ -79,0 +80,0 @@ struct soc_camera_host_ops {
};

struct soc_camera_link {
+    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:

> > 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
--- a/linux/drivers/media/video/mt9m001.c    Fri May 02 01:48:36 2008 -0300
+++ b/linux/drivers/media/video/mt9m001.c    Fri May 02 16:34:37 2008 +0200
@@ -659,6 +685,7 @@ static int mt9m001_probe(struct i2c_clie
    icd->width_max    = 1280;
    icd->height_min    = 32;
    icd->height_max    = 1024;
+    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/mt9v022.c
--- a/linux/drivers/media/video/mt9v022.c    Fri May 02 01:48:36 2008 -0300
+++ b/linux/drivers/media/video/mt9v022.c    Fri May 02 16:39:49 2008 +0200
@@ -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
--- a/linux/drivers/media/video/pxa_camera.c    Fri May 02 01:48:36 2008 -0300
+++ b/linux/drivers/media/video/pxa_camera.c    Fri May 02 16:16:36 2008 +0200
@@ -883,1 +874,1 @@ static int pxa_camera_set_bus_param(stru
    }

    CICR1 = cicr1;
-    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
--- a/linux/include/media/soc_camera.h    Fri May 02 01:48:36 2008 -0300
+++ b/linux/include/media/soc_camera.h    Fri May 02 15:57:06 2008 +0200
@@ -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
--- a/linux/drivers/media/video/soc_camera.c    Fri May 02 01:48:36 2008 -0300
+++ b/linux/drivers/media/video/soc_camera.c    Mon Apr 28 18:34:49 2008 +0200
@@ -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.

Thanks
Guennadi
---
Guennadi Liakhovetski

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Some suggestions for the soc_camera interface

[Stefan Herbrechtsmeier]

Guennadi Liakhovetski schrieb:
> 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.
>   
Thanks for your help, I will keep this in mind next time.
> 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
> ****************************************************************************
>
>
> On Fri, 2 May 2008, Stefan Herbrechtsmeier wrote:
>
>   
>> 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
> --- a/linux/drivers/media/video/mt9m001.c    Fri May 02 01:48:36 2008 -0300
> +++ b/linux/drivers/media/video/mt9m001.c    Fri May 02 16:34:37 2008 +0200
> @@ -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...
> ****************************************************************************
>
> +    struct soc_camera_link *icl = icd->client->dev.platform_data;
> +    int ret;
> +
> +    if (icl && icl->power) {
>
>          ^--- icl is guaranteed to be non-NULL in both mt9m001 and mt9v022
>   
You are right, I skip all of the first tests.
> ****************************************************************************
>
> +        dev_dbg(icd->dev, "%s: Power on camera\n", __func__);
> +        icl->power(icd->dev, 1);
> +    }
> +
> +    if (icl && icd->reset) {
>
>                 ^--- Haven't compile-tested
>   
Sorry for this, I thought I have compiled all touched files.
> ****************************************************************************
>
> +        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?
>     /* Disable chip, synchronous option update */
>     dev_dbg(icd->vdev->dev, "%s\n", __func__);
> @@ -136,0 +148,0 @@ static int mt9m001_init(struct soc_camer
>
> static int mt9m001_release(struct soc_camera_device *icd)
> {
> +    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...
>   
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?
> ****************************************************************************
>
> +    }
> +
> +    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
> --- a/linux/drivers/media/video/mt9v022.c    Fri May 02 01:48:36 2008 -0300
> +++ b/linux/drivers/media/video/mt9v022.c    Fri May 02 16:39:49 2008 +0200
> @@ -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);
> +    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)
> {
> +    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
> --- a/linux/drivers/media/video/pxa_camera.c    Fri May 02 01:48:36 2008 -0300
> +++ b/linux/drivers/media/video/pxa_camera.c    Fri May 02 16:16:36 2008 +0200
> @@ -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)
> {
> -    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
> --- a/linux/include/asm-arm/arch-pxa/camera.h    Fri May 02 01:48:36 2008 -0300
> +++ b/linux/include/asm-arm/arch-pxa/camera.h    Fri May 02 15:53:10 2008 +0200
> @@ -36,0 +36,0 @@
>
> struct pxacamera_platform_data {
>     int (*init)(struct device *);
> -    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
> --- a/linux/include/media/soc_camera.h    Fri May 02 01:48:36 2008 -0300
> +++ b/linux/include/media/soc_camera.h    Fri May 02 15:57:06 2008 +0200
> @@ -79,0 +80,0 @@ struct soc_camera_host_ops {
> };
>
> struct soc_camera_link {
> +    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:
>
>   
>>> 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.
>   
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.
> ****************************************************************************
> 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
> --- a/linux/drivers/media/video/mt9m001.c    Fri May 02 01:48:36 2008 -0300
> +++ b/linux/drivers/media/video/mt9m001.c    Fri May 02 16:34:37 2008 +0200
> @@ -659,6 +685,7 @@ static int mt9m001_probe(struct i2c_clie
>     icd->width_max    = 1280;
>     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?
>     /* Default datawidth - this is the only width this camera (normally)
> diff -r dd4685496fb7 linux/drivers/media/video/mt9v022.c
> --- a/linux/drivers/media/video/mt9v022.c    Fri May 02 01:48:36 2008 -0300
> +++ b/linux/drivers/media/video/mt9v022.c    Fri May 02 16:39:49 2008 +0200
> @@ -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
> --- a/linux/drivers/media/video/pxa_camera.c    Fri May 02 01:48:36 2008 -0300
> +++ b/linux/drivers/media/video/pxa_camera.c    Fri May 02 16:16:36 2008 +0200
> @@ -883,1 +874,1 @@ static int pxa_camera_set_bus_param(stru
>     }
>
>     CICR1 = cicr1;
> -    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
> --- a/linux/include/media/soc_camera.h    Fri May 02 01:48:36 2008 -0300
> +++ b/linux/include/media/soc_camera.h    Fri May 02 15:57:06 2008 +0200
> @@ -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
> --- a/linux/drivers/media/video/soc_camera.c    Fri May 02 01:48:36 2008 -0300
> +++ b/linux/drivers/media/video/soc_camera.c    Mon Apr 28 18:34:49 2008 +0200
> @@ -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.
>   
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.

Regards
    Stefan

-- 
Dipl.-Ing. Stefan Herbrechtsmeier

Heinz Nixdorf Institute
University of Paderborn 
System and Circuit Technology 
Fürstenallee 11
D-33102 Paderborn (Germany)

office : F0.415
phone  : + 49 5251 - 60 6342
fax    : + 49 5251 - 60 6351

mailto : hbmeier@hni.upb.de

www    : http://wwwhni.upb.de/sct/mitarbeiter/hbmeier


--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Some suggestions for the soc_camera interface

[Guennadi Liakhovetski]

On Mon, 5 May 2008, Stefan Herbrechtsmeier wrote:

> > +        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?

Actually, now that I look at it again, it is neither the former nor the 
latter one. We are calling a reset method, provided by the platform here. 
Platform knows nothing about i2c or video devices associated with the 
specific camera. It only knows about its physical connection - which 
camera host it is connected to, how to locate it on that host, and how to 
reset / power on or off. So, the parameter we pass to the reset, init, and 
power functions should be a void pointer from the respective 
soc_camera_link. I.e., soc_camera_link should get a new member like

	void	*arg;	/* platform data used as argument to init, reset, power */

and then you call icl->power(icl->arg).

> > @@ -136,0 +148,0 @@ static int mt9m001_init(struct soc_camer
> > 
> > static int mt9m001_release(struct soc_camera_device *icd)
> > {
> > +    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...
> >   
> 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.

Exactly, this is implementation specidic, and in my implementation I 
didn't have any reset / power, so, I could well just throw them away. 
Let's see what you need there and adapt them to your needs.

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

Same - let's just see what your and my platforms and cameras need and let 
anyone else with different hardware either fix their hardware or provide 
patches:-)

> > ****************************************************************************
> > Patch 2
> > ****************************************************************************
> > 
> > + Rename SOCAM_HSYNC_* to SOCAM_HREF_* and introduce new
> >    SOCAM_HSYNC_*
> > 
> > This is what you've written in your earlier email:
> > 
> >   
> > > > 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.
> >   
> 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.

I understood that, but I still don't think there's anything you have to 
change in the pxa-camera driver for that. But let's see when you submit 
your patches.

> > diff -r dd4685496fb7 linux/drivers/media/video/mt9m001.c
> > --- a/linux/drivers/media/video/mt9m001.c    Fri May 02 01:48:36 2008 -0300
> > +++ b/linux/drivers/media/video/mt9m001.c    Fri May 02 16:34:37 2008 +0200
> > @@ -659,6 +685,7 @@ static int mt9m001_probe(struct i2c_clie
> >     icd->width_max    = 1280;
> >     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?

Use your esthetic feeling:-) But if it doesn't coincide with mine, I'll 
reject the patch:-))

> > 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.

Great!

Thanks
Guennadi
---
Guennadi Liakhovetski

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Some suggestions for the soc_camera interface

[Guennadi Liakhovetski]

Hi Stefan,

On Mon, 5 May 2008, Stefan Herbrechtsmeier wrote:

> Guennadi Liakhovetski schrieb:
> > 
> > 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.

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.

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

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Some suggestions for the soc_camera interface

[Stefan Herbrechtsmeier]

Guennadi Liakhovetski schrieb:
> Hi Stefan,
>
> On Mon, 5 May 2008, Stefan Herbrechtsmeier wrote:
>
>   
>> Guennadi Liakhovetski schrieb:
>>     
>>> 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.
>>     
>
> 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.

Regards
Stefan

-- 
Dipl.-Ing. Stefan Herbrechtsmeier

Heinz Nixdorf Institute
University of Paderborn 
System and Circuit Technology 
Fürstenallee 11
D-33102 Paderborn (Germany)

office : F0.415
phone  : + 49 5251 - 60 6342
fax    : + 49 5251 - 60 6351

mailto : hbmeier@hni.upb.de

www    : http://wwwhni.upb.de/sct/mitarbeiter/hbmeier


--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Some suggestions for the soc_camera interface

[Guennadi Liakhovetski]

Hi Stefan,

On Wed, 30 Jul 2008, Stefan Herbrechtsmeier wrote:

> Guennadi Liakhovetski schrieb:
> > 
> > On Mon, 5 May 2008, Stefan Herbrechtsmeier wrote:
> >   
> > > Guennadi Liakhovetski schrieb:
> > >     
> > > > 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.
> > 
> > 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...

> 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:-)

> 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:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Some suggestions for the soc_camera interface

[Paulius Zaleckas]

Guennadi Liakhovetski wrote:
> Hi Stefan,
> 
> On Wed, 30 Jul 2008, Stefan Herbrechtsmeier wrote:
> 
>> Guennadi Liakhovetski schrieb:
>>> On Mon, 5 May 2008, Stefan Herbrechtsmeier wrote:
>>>   
>>>> Guennadi Liakhovetski schrieb:
>>>>     
>>>>> 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.
>>> 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...
> 
>> 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:-)

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...

>> 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:video4linux-list-request@redhat.com?subject=unsubscribe
> https://www.redhat.com/mailman/listinfo/video4linux-list
> 

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Some suggestions for the soc_camera interface

[Stefan Herbrechtsmeier]

Paulius Zaleckas schrieb:
> Guennadi Liakhovetski wrote:
>> Hi Stefan,
>>
>> On Wed, 30 Jul 2008, Stefan Herbrechtsmeier wrote:
>>
>>> Guennadi Liakhovetski schrieb:
>>>> On Mon, 5 May 2008, Stefan Herbrechtsmeier wrote:
>>>>  
>>>>> Guennadi Liakhovetski schrieb:
>>>>>    
>>>>>> 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.
>>>> 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...
>>
>>> 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:-)
>
> 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?
>
>
>>> 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:video4linux-list-request@redhat.com?subject=unsubscribe
>> https://www.redhat.com/mailman/listinfo/video4linux-list
>>

-- 
Dipl.-Ing. Stefan Herbrechtsmeier

Heinz Nixdorf Institute
University of Paderborn 
System and Circuit Technology 
Fürstenallee 11
D-33102 Paderborn (Germany)

office : F0.415
phone  : + 49 5251 - 60 6342
fax    : + 49 5251 - 60 6351

mailto : hbmeier@hni.upb.de

www    : http://wwwhni.upb.de/sct/mitarbeiter/hbmeier


--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Some suggestions for the soc_camera interface

[Guennadi Liakhovetski]

On Wed, 30 Jul 2008, Stefan Herbrechtsmeier wrote:

> > > 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?

Yes, please. You also have to modify host camera drivers, don't you? There 
are 2 of them now: PXA and SH. Also, please, have a look if the 
platform camera driver also has to be changed.

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

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list