PATCH: soc-camera: use flag for colour / bw camera instead of module parameter

PATCH: soc-camera: use flag for colour / bw camera instead of module parameter

[Sascha Hauer]

Hi,

Use a flag in struct soc_camera_link for differentiation between
a black/white and a colour camera rather than a module parameter.
This allows for having colour and black/white cameras in the same
system.
Note that this one breaks the phytec pcm027 pxa board as it makes it
impossible to switch between cameras on the command line. I will send
an updated version of this patch once I know this patch is acceptable
this way.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

---
 drivers/media/video/mt9v022.c |    7 +------
 include/media/soc_camera.h    |    8 ++++++++
 2 files changed, 9 insertions(+), 6 deletions(-)

Index: include/media/soc_camera.h
===================================================================
--- include/media/soc_camera.h.orig
+++ include/media/soc_camera.h
@@ -80,6 +80,12 @@ struct soc_camera_host_ops {
 	void (*spinlock_free)(spinlock_t *);
 };
 
+/* There are several cameras out there which come in a black/white
+ * and a colour variant. They are logically the same cameras and thus
+ * can't be detected in software
+ */
+#define SOCAM_LINK_COLOUR	(1 << 0)
+
 struct soc_camera_link {
 	/* Camera bus id, used to match a camera and a bus */
 	int bus_id;
@@ -88,6 +94,8 @@ struct soc_camera_link {
 	/* (de-)activate this camera. Can be left empty if only one camera is
 	 * connected to this bus. */
 	void (*activate)(struct soc_camera_link *, int);
+
+	unsigned long flags;
 };
 
 static inline struct soc_camera_device *to_soc_camera_dev(struct device *dev)
Index: drivers/media/video/mt9v022.c
===================================================================
--- drivers/media/video/mt9v022.c.orig
+++ drivers/media/video/mt9v022.c
@@ -23,10 +23,6 @@
  * The platform has to define i2c_board_info
  * and call i2c_register_board_info() */
 
-static char *sensor_type;
-module_param(sensor_type, charp, S_IRUGO);
-MODULE_PARM_DESC(sensor_type, "Sensor type: \"colour\" or \"monochrome\"\n");
-
 /* mt9v022 selected register addresses */
 #define MT9V022_CHIP_VERSION		0x00
 #define MT9V022_COLUMN_START		0x01
@@ -698,8 +694,7 @@ static int mt9v022_video_probe(struct so
 	}
 
 	/* Set monochrome or colour sensor type */
-	if (sensor_type && (!strcmp("colour", sensor_type) ||
-			    !strcmp("color", sensor_type))) {
+	if (icd->link->flags & SOCAM_LINK_COLOUR) {
 		ret = reg_write(icd, MT9V022_PIXEL_OPERATION_MODE, 4 | 0x11);
 		mt9v022->model = V4L2_IDENT_MT9V022IX7ATC;
 		icd->formats = mt9v022_colour_formats;

-- 
-- 
 Pengutronix - Linux Solutions for Science and Industry
   Handelsregister:  Amtsgericht Hildesheim, HRA 2686
     Hannoversche Str. 2, 31134 Hildesheim, Germany
   Phone: +49-5121-206917-0 |  Fax: +49-5121-206917-9

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

Re: PATCH: soc-camera: use flag for colour / bw camera instead of module parameter

[Sascha Hauer]

Also in -p1 fashion:


Use a flag in struct soc_camera_link for differentiation between
a black/white and a colour camera rather than a module parameter.
This allows for having colour and black/white cameras in the same
system.
Note that this one breaks the phytec pcm027 pxa board as it makes it
impossible to switch between cameras on the command line. I will send
an updated version of this patch once I know this patch is acceptable
this way.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

---
 drivers/media/video/mt9v022.c |    7 +------
 include/media/soc_camera.h    |    8 ++++++++
 2 files changed, 9 insertions(+), 6 deletions(-)

Index: linux-2.6/include/media/soc_camera.h
===================================================================
--- linux-2.6.orig/include/media/soc_camera.h
+++ linux-2.6/include/media/soc_camera.h
@@ -80,6 +80,12 @@ struct soc_camera_host_ops {
 	void (*spinlock_free)(spinlock_t *);
 };
 
+/* There are several cameras out there which come in a black/white
+ * and a colour variant. They are logically the same cameras and thus
+ * can't be detected in software
+ */
+#define SOCAM_LINK_COLOUR	(1 << 0)
+
 struct soc_camera_link {
 	/* Camera bus id, used to match a camera and a bus */
 	int bus_id;
@@ -88,6 +94,8 @@ struct soc_camera_link {
 	/* (de-)activate this camera. Can be left empty if only one camera is
 	 * connected to this bus. */
 	void (*activate)(struct soc_camera_link *, int);
+
+	unsigned long flags;
 };
 
 static inline struct soc_camera_device *to_soc_camera_dev(struct device *dev)
Index: linux-2.6/drivers/media/video/mt9v022.c
===================================================================
--- linux-2.6.orig/drivers/media/video/mt9v022.c
+++ linux-2.6/drivers/media/video/mt9v022.c
@@ -23,10 +23,6 @@
  * The platform has to define i2c_board_info
  * and call i2c_register_board_info() */
 
-static char *sensor_type;
-module_param(sensor_type, charp, S_IRUGO);
-MODULE_PARM_DESC(sensor_type, "Sensor type: \"colour\" or \"monochrome\"\n");
-
 /* mt9v022 selected register addresses */
 #define MT9V022_CHIP_VERSION		0x00
 #define MT9V022_COLUMN_START		0x01
@@ -698,8 +694,7 @@ static int mt9v022_video_probe(struct so
 	}
 
 	/* Set monochrome or colour sensor type */
-	if (sensor_type && (!strcmp("colour", sensor_type) ||
-			    !strcmp("color", sensor_type))) {
+	if (icd->link->flags & SOCAM_LINK_COLOUR) {
 		ret = reg_write(icd, MT9V022_PIXEL_OPERATION_MODE, 4 | 0x11);
 		mt9v022->model = V4L2_IDENT_MT9V022IX7ATC;
 		icd->formats = mt9v022_colour_formats;

-- 
-- 
 Pengutronix - Linux Solutions for Science and Industry
   Handelsregister:  Amtsgericht Hildesheim, HRA 2686
     Hannoversche Str. 2, 31134 Hildesheim, Germany
   Phone: +49-5121-206917-0 |  Fax: +49-5121-206917-9

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

Re: PATCH: soc-camera: use flag for colour / bw camera instead of module parameter

[Paulius Zaleckas]

Sascha Hauer wrote:
> Also in -p1 fashion:
> 
> 
> Use a flag in struct soc_camera_link for differentiation between
> a black/white and a colour camera rather than a module parameter.
> This allows for having colour and black/white cameras in the same
> system.

IMO you should pass void *sensor_data in there. Just to be as generic as
possible, because you don't know what flags, functions and etc. you will
need for all other sensors in the world...

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

Re: PATCH: soc-camera: use flag for colour / bw camera instead of module parameter

[Guennadi Liakhovetski]

On Tue, 15 Jul 2008, Sascha Hauer wrote:

> Use a flag in struct soc_camera_link for differentiation between
> a black/white and a colour camera rather than a module parameter.
> This allows for having colour and black/white cameras in the same
> system.
> Note that this one breaks the phytec pcm027 pxa board as it makes it
> impossible to switch between cameras on the command line. I will send
> an updated version of this patch once I know this patch is acceptable
> this way.

Yes, we did discuss this on IRC and I did agree to use a platform-provided 
parameter to specify camera properties like colour / monochrome, but now 
as I see it, I think, it might not be a very good idea. Having it as a 
parameter you can just reload the driver with a different parameter to 
test your colour camera in b/w mode. With this change you would need a new 
kernel. What about an array of module parameters? Specifying flags on the 
command line is too weird, so, maybe colo(u)r=0,2 where numbers are 
camera-IDs? Or even 0:0 with bus-IDs. Yes, you would have to add optional 
camera-ID to the link struct.

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: soc-camera: use flag for colour / bw camera instead of module parameter

[Sascha Hauer]

On Tue, Jul 15, 2008 at 10:43:53PM +0200, Guennadi Liakhovetski wrote:
> On Tue, 15 Jul 2008, Sascha Hauer wrote:
> 
> > Use a flag in struct soc_camera_link for differentiation between
> > a black/white and a colour camera rather than a module parameter.
> > This allows for having colour and black/white cameras in the same
> > system.
> > Note that this one breaks the phytec pcm027 pxa board as it makes it
> > impossible to switch between cameras on the command line. I will send
> > an updated version of this patch once I know this patch is acceptable
> > this way.
> 
> Yes, we did discuss this on IRC and I did agree to use a platform-provided 
> parameter to specify camera properties like colour / monochrome, but now 
> as I see it, I think, it might not be a very good idea. Having it as a 
> parameter you can just reload the driver with a different parameter to 
> test your colour camera in b/w mode. With this change you would need a new 
> kernel.

I think it's a more common case to specify the correct camera on a
per-board basis than to test a colour camera in b/w mode. Only
developers want to do this and they know how to start a new kernel,
right? ;)
Another thing that came to my mind is that this particular camera has an
internal PLL for pixel clock generation. It can use the pxa pixel clock
directly or the one from the PLL. At the moment there is no way to
specify which clock to use, so we might even want to add a pointer to a
camera specific struct to soc_camera_link. This would be the right place
to put colour/bw flags aswell.

> What about an array of module parameters? Specifying flags on the 
> command line is too weird, so, maybe colo(u)r=0,2 where numbers are 
> camera-IDs? Or even 0:0 with bus-IDs. Yes, you would have to add optional 
> camera-ID to the link struct.

I don't like module parameters for specifying my hardware. It
reminds me of the ISA days where you had to specify iobase/irq this way.

Sascha

-- 
 Pengutronix - Linux Solutions for Science and Industry
   Handelsregister:  Amtsgericht Hildesheim, HRA 2686
     Hannoversche Str. 2, 31134 Hildesheim, Germany
   Phone: +49-5121-206917-0 |  Fax: +49-5121-206917-9

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

Re: PATCH: soc-camera: use flag for colour / bw camera instead of module parameter

[Sascha Hauer]

On Wed, Jul 16, 2008 at 07:49:22AM +0200, Sascha Hauer wrote:
> On Tue, Jul 15, 2008 at 10:43:53PM +0200, Guennadi Liakhovetski wrote:
> > On Tue, 15 Jul 2008, Sascha Hauer wrote:
> > 
> > > Use a flag in struct soc_camera_link for differentiation between
> > > a black/white and a colour camera rather than a module parameter.
> > > This allows for having colour and black/white cameras in the same
> > > system.
> > > Note that this one breaks the phytec pcm027 pxa board as it makes it
> > > impossible to switch between cameras on the command line. I will send
> > > an updated version of this patch once I know this patch is acceptable
> > > this way.
> > 
> > Yes, we did discuss this on IRC and I did agree to use a platform-provided 
> > parameter to specify camera properties like colour / monochrome, but now 
> > as I see it, I think, it might not be a very good idea. Having it as a 
> > parameter you can just reload the driver with a different parameter to 
> > test your colour camera in b/w mode. With this change you would need a new 
> > kernel.
> 
> I think it's a more common case to specify the correct camera on a
> per-board basis than to test a colour camera in b/w mode. Only
> developers want to do this and they know how to start a new kernel,
> right? ;)
> Another thing that came to my mind is that this particular camera has an
> internal PLL for pixel clock generation. It can use the pxa pixel clock
> directly or the one from the PLL. At the moment there is no way to
> specify which clock to use, so we might even want to add a pointer to a
> camera specific struct to soc_camera_link. This would be the right place
> to put colour/bw flags aswell.

Speaking of which, what's currently in pxa_camera_platform_data is
camera specific and not board specific (think of two cameras connected
to the pxa requiring two different clocks). So soc_camera_link should
look something like:

struct soc_camera_link {
	/* Camera bus id, used to match a camera and a bus */
	int bus_id;
	/* host specific data, i.e. struct pxa_camera_platform_data */
	void *host_info;
	/* camera specific info, i.e. struct mt9v022_data */
	void *camera_info;
	/* (de-)activate this camera. Can be left empty if only one camera is
	 * connected to this bus. */
	void (*activate)(struct soc_camera_link *, int);
};

I'm lucky, at the moment I have two identical cameras connected to my board
(besides the colour/bw thing)

Sascha

-- 
 Pengutronix - Linux Solutions for Science and Industry
   Handelsregister:  Amtsgericht Hildesheim, HRA 2686
     Hannoversche Str. 2, 31134 Hildesheim, Germany
   Phone: +49-5121-206917-0 |  Fax: +49-5121-206917-9

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

Re: PATCH: soc-camera: use flag for colour / bw camera instead of module parameter

[Guennadi Liakhovetski]

On Wed, 16 Jul 2008, Sascha Hauer wrote:

> On Tue, Jul 15, 2008 at 10:43:53PM +0200, Guennadi Liakhovetski wrote:
> > On Tue, 15 Jul 2008, Sascha Hauer wrote:
> > 
> > > Use a flag in struct soc_camera_link for differentiation between
> > > a black/white and a colour camera rather than a module parameter.
> > > This allows for having colour and black/white cameras in the same
> > > system.
> > > Note that this one breaks the phytec pcm027 pxa board as it makes it
> > > impossible to switch between cameras on the command line. I will send
> > > an updated version of this patch once I know this patch is acceptable
> > > this way.
> > 
> > Yes, we did discuss this on IRC and I did agree to use a platform-provided 
> > parameter to specify camera properties like colour / monochrome, but now 
> > as I see it, I think, it might not be a very good idea. Having it as a 
> > parameter you can just reload the driver with a different parameter to 
> > test your colour camera in b/w mode. With this change you would need a new 
> > kernel.
> 
> I think it's a more common case to specify the correct camera on a
> per-board basis than to test a colour camera in b/w mode. Only
> developers want to do this and they know how to start a new kernel,
> right? ;)

Let me think: ker-nel... Yeah, I think, I've heard something about it 
already...

But I can also imagine cases when end-users would benefit from this module 
parameter: think about a company producing two cameras - one with colour 
and one with bw sensor. With the module parameter they only have to 
load drivers / the kernel differently, with platform data they have to 
maintain two kernel versions. Unless they are smart enough and put those 
cameras on different i2c addresses. But, ok, the current trend seems 
indeed to be to specify such information in the platform data, even though 
not only ISA drivers use module-parameters for this. Anyway, I'm flexible 
about this:-) Let's do it.

> Another thing that came to my mind is that this particular camera has an
> internal PLL for pixel clock generation. It can use the pxa pixel clock
> directly or the one from the PLL.

Many CMOS cameras have this.

> At the moment there is no way to
> specify which clock to use, so we might even want to add a pointer to a
> camera specific struct to soc_camera_link. This would be the right place
> to put colour/bw flags aswell.

There is one, and it is used during parameter negotiation. See 
SOCAM_MASTER and its use in mt9v022.c and pxa_camera.c, mt9m001 can only 
be a master (use internal clock), so, it is not including SOCAM_SLAVE in 
its supported mode flags. Isn't this enough? Do you really have to enforce 
the use of one or another clock, or is it enough to let the two drivers 
choose a supported configuration?

> Speaking of which, what's currently in pxa_camera_platform_data is
> camera specific and not board specific (think of two cameras connected
> to the pxa requiring two different clocks).

Yes, someone already suggested to make .power and .reset per camera: 
http://marc.info/?t=120974473900009&r=1&w=2 and 
http://marc.info/?t=121007886200003&r=1&w=2, but this work is not finished 
yet - he wanted to resubmit his patches when his camera driver is ready.

> So soc_camera_link should
> look something like:
> 
> struct soc_camera_link {
> 	/* Camera bus id, used to match a camera and a bus */
> 	int bus_id;
> 	/* host specific data, i.e. struct pxa_camera_platform_data */
> 	void *host_info;
> 	/* camera specific info, i.e. struct mt9v022_data */
> 	void *camera_info;
> 	/* (de-)activate this camera. Can be left empty if only one camera is
> 	 * connected to this bus. */
> 	void (*activate)(struct soc_camera_link *, int);
> };
> 
> I'm lucky, at the moment I have two identical cameras connected to my board
> (besides the colour/bw thing)

Well, soc_camera_link is indeed per-sensor and we can and shall use it to 
specify camera-specific platform parameters. So, I'm fine with moving 
.power and .reset into it. Then you won't need your .activate any more, 
right? As for the rest - I don't like too many void pointers... This 
struct is for the platform to configure camera drivers. So, it should 
contain _data_, not pointers to camera- and host-specific structs. If we 
need to specify a colour / monochrome sensor, let's do this directly. But 
without void pointers, please.

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: soc-camera: use flag for colour / bw camera instead of module parameter

[Robert Schwebel]

On Wed, Jul 16, 2008 at 09:19:44AM +0200, Guennadi Liakhovetski wrote:
> But I can also imagine cases when end-users would benefit from this
> module parameter: think about a company producing two cameras - one
> with colour and one with bw sensor. With the module parameter they
> only have to load drivers / the kernel differently, with platform data
> they have to maintain two kernel versions.

They can also put the definitions in a separate module and load them
according to their needs.

rsc
-- 
 Dipl.-Ing. Robert Schwebel | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry
   Handelsregister:  Amtsgericht Hildesheim, HRA 2686
     Hannoversche Str. 2, 31134 Hildesheim, Germany
   Phone: +49-5121-206917-0 |  Fax: +49-5121-206917-9

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

Re: PATCH: soc-camera: use flag for colour / bw camera instead of module parameter

[Sascha Hauer]

On Wed, Jul 16, 2008 at 09:19:44AM +0200, Guennadi Liakhovetski wrote:
> On Wed, 16 Jul 2008, Sascha Hauer wrote:
> 
> > On Tue, Jul 15, 2008 at 10:43:53PM +0200, Guennadi Liakhovetski wrote:
> > > On Tue, 15 Jul 2008, Sascha Hauer wrote:
> > > 
> > > > Use a flag in struct soc_camera_link for differentiation between
> > > > a black/white and a colour camera rather than a module parameter.
> > > > This allows for having colour and black/white cameras in the same
> > > > system.
> > > > Note that this one breaks the phytec pcm027 pxa board as it makes it
> > > > impossible to switch between cameras on the command line. I will send
> > > > an updated version of this patch once I know this patch is acceptable
> > > > this way.
> > > 
> > > Yes, we did discuss this on IRC and I did agree to use a platform-provided 
> > > parameter to specify camera properties like colour / monochrome, but now 
> > > as I see it, I think, it might not be a very good idea. Having it as a 
> > > parameter you can just reload the driver with a different parameter to 
> > > test your colour camera in b/w mode. With this change you would need a new 
> > > kernel.
> > 
> > I think it's a more common case to specify the correct camera on a
> > per-board basis than to test a colour camera in b/w mode. Only
> > developers want to do this and they know how to start a new kernel,
> > right? ;)
> 
> Let me think: ker-nel... Yeah, I think, I've heard something about it 
> already...
> 
> But I can also imagine cases when end-users would benefit from this module 
> parameter: think about a company producing two cameras - one with colour 
> and one with bw sensor. With the module parameter they only have to 
> load drivers / the kernel differently, with platform data they have to 
> maintain two kernel versions. Unless they are smart enough and put those 
> cameras on different i2c addresses. But, ok, the current trend seems 
> indeed to be to specify such information in the platform data, even though 
> not only ISA drivers use module-parameters for this. Anyway, I'm flexible 
> about this:-) Let's do it.
> 
> > Another thing that came to my mind is that this particular camera has an
> > internal PLL for pixel clock generation. It can use the pxa pixel clock
> > directly or the one from the PLL.
> 
> Many CMOS cameras have this.
> 
> > At the moment there is no way to
> > specify which clock to use, so we might even want to add a pointer to a
> > camera specific struct to soc_camera_link. This would be the right place
> > to put colour/bw flags aswell.
> 
> There is one, and it is used during parameter negotiation. See 
> SOCAM_MASTER and its use in mt9v022.c and pxa_camera.c, mt9m001 can only 
> be a master (use internal clock), so, it is not including SOCAM_SLAVE in 
> its supported mode flags. Isn't this enough? Do you really have to enforce 
> the use of one or another clock, or is it enough to let the two drivers 
> choose a supported configuration?

My camera supports a maximum clock input of 96MHz without PLL and a
range of 16-27MHz with PLL. Say you want to use the PLL so you choose a
clock input of 25MHz (in struct pxa_camera_platform_data). To what value
do you adjust the PLL? The highest possible value of 96MHz is too fast
for my long data lines.

> 
> > Speaking of which, what's currently in pxa_camera_platform_data is
> > camera specific and not board specific (think of two cameras connected
> > to the pxa requiring two different clocks).
> 
> Yes, someone already suggested to make .power and .reset per camera: 
> http://marc.info/?t=120974473900009&r=1&w=2 and 
> http://marc.info/?t=121007886200003&r=1&w=2, but this work is not finished 
> yet - he wanted to resubmit his patches when his camera driver is ready.
> 
> > So soc_camera_link should
> > look something like:
> > 
> > struct soc_camera_link {
> > 	/* Camera bus id, used to match a camera and a bus */
> > 	int bus_id;
> > 	/* host specific data, i.e. struct pxa_camera_platform_data */
> > 	void *host_info;
> > 	/* camera specific info, i.e. struct mt9v022_data */
> > 	void *camera_info;
> > 	/* (de-)activate this camera. Can be left empty if only one camera is
> > 	 * connected to this bus. */
> > 	void (*activate)(struct soc_camera_link *, int);
> > };
> > 
> > I'm lucky, at the moment I have two identical cameras connected to my board
> > (besides the colour/bw thing)
> 
> Well, soc_camera_link is indeed per-sensor and we can and shall use it to 
> specify camera-specific platform parameters. So, I'm fine with moving 
> .power and .reset into it. Then you won't need your .activate any more,

Ok, I like 'power' better than 'activate', so I'll change it.

> right? As for the rest - I don't like too many void pointers... This 
> struct is for the platform to configure camera drivers. So, it should 
> contain _data_, not pointers to camera- and host-specific structs. If we 
> need to specify a colour / monochrome sensor, let's do this directly. But 
> without void pointers, please.

Well, I don't like the use of void pointers, too, but Specifying colour/bw
directly does not solve the problem with the input clocks though. The
gpio field in soc_camera_link is camera specific aswell, so I have the
feeling that we end up adding more and more fields to soc_camera_link
which are useful only for a few cameras.


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

-- 
-- 
 Pengutronix - Linux Solutions for Science and Industry
   Handelsregister:  Amtsgericht Hildesheim, HRA 2686
     Hannoversche Str. 2, 31134 Hildesheim, Germany
   Phone: +49-5121-206917-0 |  Fax: +49-5121-206917-9

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

Re: PATCH: soc-camera: use flag for colour / bw camera instead of module parameter

[Guennadi Liakhovetski]

(list re-added)

On Wed, 16 Jul 2008, Sascha Hauer wrote:

> On Wed, Jul 16, 2008 at 11:24:17AM +0200, Guennadi Liakhovetski wrote:
> > On Wed, 16 Jul 2008, Sascha Hauer wrote:
> > 
> > > On Wed, Jul 16, 2008 at 09:19:44AM +0200, Guennadi Liakhovetski wrote:
> > > > 
> > > > There is one, and it is used during parameter negotiation. See 
> > > > SOCAM_MASTER and its use in mt9v022.c and pxa_camera.c, mt9m001 can only 
> > > > be a master (use internal clock), so, it is not including SOCAM_SLAVE in 
> > > > its supported mode flags. Isn't this enough? Do you really have to enforce 
> > > > the use of one or another clock, or is it enough to let the two drivers 
> > > > choose a supported configuration?
> > > 
> > > My camera supports a maximum clock input of 96MHz without PLL and a
> > > range of 16-27MHz with PLL. Say you want to use the PLL so you choose a
> > > clock input of 25MHz (in struct pxa_camera_platform_data). To what value
> > > do you adjust the PLL? The highest possible value of 96MHz is too fast
> > > for my long data lines.
> > 
> > Sorry, I do not quite understand the question. Are you asking what 
> > frequency I would select, or you're asking how to let the driver(s) 
> > configure the selected frequency?
> 
> My problem is that I do not have a way to specify the pixelclock for the
> sensor. I can specify the clock output of the pxa, but not the one from
> the sensor.
> 
> > 
> > > Well, I don't like the use of void pointers, too, but Specifying colour/bw
> > > directly does not solve the problem with the input clocks though. The
> > > gpio field in soc_camera_link is camera specific aswell, so I have the
> > > feeling that we end up adding more and more fields to soc_camera_link
> > > which are useful only for a few cameras.
> > 
> > So far we haven't decided, that we need to specify the frequency in camera 
> > configuration. Maybe we will need to add supported frequencies to 
> > parameter negotiation. In your case it would suffice, if the host-driver 
> > specified, that it wants to run at 25MHz, as configured by the platform 
> > data, then the camera driver can decide which modes (master / slave) it 
> > supports at _this_ frequency. Say, if you request 15 or 30MHz the driver 
> > will only report slave-mode.
> 
> I'm not talking about master/slave mode. My camera always works in
> master mode (meaning that it generates synchronization signals).
> 
> To make my point a bit more clear, my clock flow is as follows:
> 
> CIF_MCLK -> sensor PLL -> CIF_PCLK
> 
> I can specify CIF_MCLK via mclk_10khz, but the sensor driver has no idea
> to what frequency it has to adjust the PLL:
> 
> - The sensor can generate a maximum clock of 96MHz out of whatever input
>   clock thanks to its PLL
> - The pxa accepts a maximum clock of 26MHz
> - The user may want to limit the pixel clock to a lower value because of
>   long data lines.

Ok, _now_ I see your problem - you really have 2 clock frequencies, that 
can be configured independently... That's bad:-( And you think it's not 
worth it adding sensor type (colour vs. monochrome) _and_ clock frequency 
to the link struct because not many cameras will need them and other 
comeras might have other similarly "unique" properties, which, if all 
added to the link struct, would needlessly blow it up. Well, still, I 
would prefer putting these two directly into the link struct. But if you 
strongly disagree, I will accept a single void pointer to camera private 
config data in it too:-(

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: soc-camera: use flag for colour / bw camera instead of module parameter

[Sascha Hauer]

On Wed, Jul 16, 2008 at 01:16:54PM +0200, Guennadi Liakhovetski wrote:
> (list re-added)
> 
> On Wed, 16 Jul 2008, Sascha Hauer wrote:
> 
> > On Wed, Jul 16, 2008 at 11:24:17AM +0200, Guennadi Liakhovetski wrote:
> > > On Wed, 16 Jul 2008, Sascha Hauer wrote:
> > > 
> > > > On Wed, Jul 16, 2008 at 09:19:44AM +0200, Guennadi Liakhovetski wrote:
> > > > > 
> > > > > There is one, and it is used during parameter negotiation. See 
> > > > > SOCAM_MASTER and its use in mt9v022.c and pxa_camera.c, mt9m001 can only 
> > > > > be a master (use internal clock), so, it is not including SOCAM_SLAVE in 
> > > > > its supported mode flags. Isn't this enough? Do you really have to enforce 
> > > > > the use of one or another clock, or is it enough to let the two drivers 
> > > > > choose a supported configuration?
> > > > 
> > > > My camera supports a maximum clock input of 96MHz without PLL and a
> > > > range of 16-27MHz with PLL. Say you want to use the PLL so you choose a
> > > > clock input of 25MHz (in struct pxa_camera_platform_data). To what value
> > > > do you adjust the PLL? The highest possible value of 96MHz is too fast
> > > > for my long data lines.
> > > 
> > > Sorry, I do not quite understand the question. Are you asking what 
> > > frequency I would select, or you're asking how to let the driver(s) 
> > > configure the selected frequency?
> > 
> > My problem is that I do not have a way to specify the pixelclock for the
> > sensor. I can specify the clock output of the pxa, but not the one from
> > the sensor.
> > 
> > > 
> > > > Well, I don't like the use of void pointers, too, but Specifying colour/bw
> > > > directly does not solve the problem with the input clocks though. The
> > > > gpio field in soc_camera_link is camera specific aswell, so I have the
> > > > feeling that we end up adding more and more fields to soc_camera_link
> > > > which are useful only for a few cameras.
> > > 
> > > So far we haven't decided, that we need to specify the frequency in camera 
> > > configuration. Maybe we will need to add supported frequencies to 
> > > parameter negotiation. In your case it would suffice, if the host-driver 
> > > specified, that it wants to run at 25MHz, as configured by the platform 
> > > data, then the camera driver can decide which modes (master / slave) it 
> > > supports at _this_ frequency. Say, if you request 15 or 30MHz the driver 
> > > will only report slave-mode.
> > 
> > I'm not talking about master/slave mode. My camera always works in
> > master mode (meaning that it generates synchronization signals).
> > 
> > To make my point a bit more clear, my clock flow is as follows:
> > 
> > CIF_MCLK -> sensor PLL -> CIF_PCLK
> > 
> > I can specify CIF_MCLK via mclk_10khz, but the sensor driver has no idea
> > to what frequency it has to adjust the PLL:
> > 
> > - The sensor can generate a maximum clock of 96MHz out of whatever input
> >   clock thanks to its PLL
> > - The pxa accepts a maximum clock of 26MHz
> > - The user may want to limit the pixel clock to a lower value because of
> >   long data lines.
> 
> Ok, _now_ I see your problem - you really have 2 clock frequencies, that 
> can be configured independently... That's bad:-( And you think it's not 
> worth it adding sensor type (colour vs. monochrome) _and_ clock frequency 
> to the link struct because not many cameras will need them and other 
> comeras might have other similarly "unique" properties, which, if all 
> added to the link struct, would needlessly blow it up.

The bw/colour and frequency thing might be quite common, it's the other
"unique" properties that I'm afraid of.

> Well, still, I 
> would prefer putting these two directly into the link struct. But if you 
> strongly disagree, I will accept a single void pointer to camera private 
> config data in it too:-(

Ok, lets put them directly into the link struct for the time being. If
we run into problems we can change it later on, maybe I'm just too
paranoid.

Sascha

-- 
 Pengutronix - Linux Solutions for Science and Industry
   Handelsregister:  Amtsgericht Hildesheim, HRA 2686
     Hannoversche Str. 2, 31134 Hildesheim, Germany
   Phone: +49-5121-206917-0 |  Fax: +49-5121-206917-9

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