Re: [PATCH v2] soc-camera: add API documentation

[Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxx>]

On Sat, 30 Aug 2008 09:22:44 +0200
Jean-Francois Moine <moinejf@xxxxxxx> wrote:

> On Fri, 2008-08-29 at 18:34 -0300, Mauro Carvalho Chehab wrote:
> 	[split]
> > At the other side of the coin, we have lots of drivers that don't use any API
> > to split sensors from the bridge drivers, being GSPCA the newest example. I
> > believe that splitting sensors from bridge drivers will benefit to reduce the
> > amount of coding size on such drivers, and help to fix bugs that are common to
> > several webcams. Of course, such conversion would be a huge task.
> 
> I think this subject was already discussed.

Yes, but still, this is something that it is still painful: gspca has lots of
"magic" constants inside. Things like (from etoms.c):
	reg_w_val(gspca_dev, 0x80, 0x00);
	reg_w_val(gspca_dev, 0x81, 0xff);
	reg_w_val(gspca_dev, 0x80, 0x20);
	reg_w_val(gspca_dev, 0x03, 0x01);

It is something really hard to understand why reg 3 needs value 1. OK, I know that
most of this came from snooping URB's at another system driver, but, if there
were bugs at the reversed-engineered driver (and there are bugs there also),
you won't have any glue to debug.


It is much clearer if you look, for example, at ov7670:
	{ REG_CLKRC, 0x1 },	/* OV: clock scale (30 fps) */
	{ REG_TSLB,  0x04 },	/* OV */
	{ REG_COM7, 0 },	/* VGA */
	/*
	 * Set the hardware window.  These values from OV don't entirely
	 * make sense - hstop is less than hstart.  But they work...
	 */
	{ REG_HSTART, 0x13 },	{ REG_HSTOP, 0x01 },
	{ REG_HREF, 0xb6 },	{ REG_VSTART, 0x02 },
	{ REG_VSTOP, 0x7a },	{ REG_VREF, 0x0a },

Ok, it is not fair to compare with ov7670, since I suspect that OLPC got the
datasheets for that device. Yet, having a driver for each sensor makes easier
to add more detailed info at that driver, if later the device manufacturer
opens for us their datasheets, like several manufacturers already did.
 
> If you look at the gspca code, you will see that many sensors are used
> by different bridges. Indeed, some values are closed from each other,
> but what to do with the differences? (the initial values of the sensor
> registers seem to be tied to the physical wires, signal levels and
> capabilities of the bridges)

I can see two ways:

1) you can have some parameters for those values that should be bind. This is
the clearer approach, IMO. Using v4l-int-if, it is possible to create a special
handler to pass such parameters to the bridge driver, by adding a new function
prototype at:
	linux/include/media/v4l2-int-device.h

2) you can add a test inside the sensor's driver to create special cases, if
the bridge requires some special treatment. If you look, for example, on
tuner-core.c, you'll see one of such special cases:

	/* HACK: This test was added to avoid tuner to probe tda9840 and
	   tea6415c on the MXB card */
	if (client->adapter->id == I2C_HW_SAA7146 && client->addr < 0x4a) {
		kfree(t);
		return -ENODEV;
	}

Of course, this is something dirty.

> Also, the way to load the sensor registers is very different from one
> bridge to an other one (direct write, with one or many values in one
> write, paging, bridge packets with different headers/tails..).

The approach taken by v4l-int-if is generic enough to work with different
modes. A previous apporach were to use i2c for this (cafe_ccic uses this
approach), but this is not generic enough for all devices.

> Eventually, looking again at the code, you will see that the sensor
> exchanges are only a small part of the code.

The real gain is not the size reduction. Having a generic module for a sensor
have some benefits:

1) All common stuff being there means that it will be easier to maintain, since
a bug found on one module should be replied on other modules;

2) Newer drivers may be written faster, since they may use the existing code.

---

Let me add some disclaimer notice ;)

I'm not saying that we _need_ to do this change for gspca. I'm just saying that
this is a _good_ improvement to the code. I know that this change
requires lots of janitors effort with little gain to the end users.


Cheers,
Mauro

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