Hi Scott
Thanks for the patch. I am not doing a full review, just a couple of notes:
On Tue, 13 Sep 2011, Scott Jiang wrote:
> this is a v4l2 sensor-level driver for the ST VS6624 camera
>
> Signed-off-by: Scott Jiang <scott.jiang.linux@xxxxxxxxx>
> ---
[snip]
> diff --git a/drivers/media/video/vs6624.c b/drivers/media/video/vs6624.c
> new file mode 100644
> index 0000000..b0030ab
> --- /dev/null
> +++ b/drivers/media/video/vs6624.c
> @@ -0,0 +1,941 @@
[snip]
> +#define VGA_WIDTH 640
> +#define VGA_HEIGHT 480
> +#define QVGA_WIDTH 320
> +#define QVGA_HEIGHT 240
> +#define QQVGA_WIDTH 160
> +#define QQVGA_HEIGHT 120
> +#define CIF_WIDTH 352
> +#define CIF_HEIGHT 288
> +#define QCIF_WIDTH 176
> +#define QCIF_HEIGHT 144
> +#define QQCIF_WIDTH 88
> +#define QQCIF_HEIGHT 72
...Can anyone put these in a central header, please? really, please?;-)
[snip]
> +static const u16 vs6624_p1[] = {
> + 0x8104, 0x03,
> + 0x8105, 0x01,
> + 0xc900, 0x03,
> + 0xc904, 0x47,
> + 0xc905, 0x10,
> + 0xc906, 0x80,
> + 0xc907, 0x3a,
> + 0x903a, 0x02,
> + 0x903b, 0x47,
> + 0x903c, 0x15,
> + 0xc908, 0x31,
> + 0xc909, 0xdc,
> + 0xc90a, 0x80,
> + 0xc90b, 0x44,
> + 0x9044, 0x02,
> + 0x9045, 0x31,
> + 0x9046, 0xe2,
> + 0xc90c, 0x07,
> + 0xc90d, 0xe0,
> + 0xc90e, 0x80,
> + 0xc90f, 0x47,
> + 0x9047, 0x90,
> + 0x9048, 0x83,
> + 0x9049, 0x81,
> + 0x904a, 0xe0,
> + 0x904b, 0x60,
> + 0x904c, 0x08,
> + 0x904d, 0x90,
> + 0x904e, 0xc0,
> + 0x904f, 0x43,
> + 0x9050, 0x74,
> + 0x9051, 0x01,
> + 0x9052, 0xf0,
> + 0x9053, 0x80,
> + 0x9054, 0x05,
> + 0x9055, 0xE4,
> + 0x9056, 0x90,
> + 0x9057, 0xc0,
> + 0x9058, 0x43,
> + 0x9059, 0xf0,
> + 0x905a, 0x02,
> + 0x905b, 0x07,
> + 0x905c, 0xec,
> + 0xc910, 0x5d,
> + 0xc911, 0xca,
> + 0xc912, 0x80,
> + 0xc913, 0x5d,
> + 0x905d, 0xa3,
> + 0x905e, 0x04,
> + 0x905f, 0xf0,
> + 0x9060, 0xa3,
> + 0x9061, 0x04,
> + 0x9062, 0xf0,
> + 0x9063, 0x22,
> + 0xc914, 0x72,
> + 0xc915, 0x92,
> + 0xc916, 0x80,
> + 0xc917, 0x64,
> + 0x9064, 0x74,
> + 0x9065, 0x01,
> + 0x9066, 0x02,
> + 0x9067, 0x72,
> + 0x9068, 0x95,
> + 0xc918, 0x47,
> + 0xc919, 0xf2,
> + 0xc91a, 0x81,
> + 0xc91b, 0x69,
> + 0x9169, 0x74,
> + 0x916a, 0x02,
> + 0x916b, 0xf0,
> + 0x916c, 0xec,
> + 0x916d, 0xb4,
> + 0x916e, 0x10,
> + 0x916f, 0x0a,
> + 0x9170, 0x90,
> + 0x9171, 0x80,
> + 0x9172, 0x16,
> + 0x9173, 0xe0,
> + 0x9174, 0x70,
> + 0x9175, 0x04,
> + 0x9176, 0x90,
> + 0x9177, 0xd3,
> + 0x9178, 0xc4,
> + 0x9179, 0xf0,
> + 0x917a, 0x22,
> + 0xc91c, 0x0a,
> + 0xc91d, 0xbe,
> + 0xc91e, 0x80,
> + 0xc91f, 0x73,
> + 0x9073, 0xfc,
> + 0x9074, 0xa3,
> + 0x9075, 0xe0,
> + 0x9076, 0xf5,
> + 0x9077, 0x82,
> + 0x9078, 0x8c,
> + 0x9079, 0x83,
> + 0x907a, 0xa3,
> + 0x907b, 0xa3,
> + 0x907c, 0xe0,
> + 0x907d, 0xfc,
> + 0x907e, 0xa3,
> + 0x907f, 0xe0,
> + 0x9080, 0xc3,
> + 0x9081, 0x9f,
> + 0x9082, 0xff,
> + 0x9083, 0xec,
> + 0x9084, 0x9e,
> + 0x9085, 0xfe,
> + 0x9086, 0x02,
> + 0x9087, 0x0a,
> + 0x9088, 0xea,
> + 0xc920, 0x47,
> + 0xc921, 0x38,
> + 0xc922, 0x80,
> + 0xc923, 0x89,
> + 0x9089, 0xec,
> + 0x908a, 0xd3,
> + 0x908b, 0x94,
> + 0x908c, 0x20,
> + 0x908d, 0x40,
> + 0x908e, 0x01,
> + 0x908f, 0x1c,
> + 0x9090, 0x90,
> + 0x9091, 0xd3,
> + 0x9092, 0xd4,
> + 0x9093, 0xec,
> + 0x9094, 0xf0,
> + 0x9095, 0x02,
> + 0x9096, 0x47,
> + 0x9097, 0x3d,
> + 0xc924, 0x45,
> + 0xc925, 0xca,
> + 0xc926, 0x80,
> + 0xc927, 0x98,
> + 0x9098, 0x12,
> + 0x9099, 0x77,
> + 0x909a, 0xd6,
> + 0x909b, 0x02,
> + 0x909c, 0x45,
> + 0x909d, 0xcd,
> + 0xc928, 0x20,
> + 0xc929, 0xd5,
> + 0xc92a, 0x80,
> + 0xc92b, 0x9e,
> + 0x909e, 0x90,
> + 0x909f, 0x82,
> + 0x90a0, 0x18,
> + 0x90a1, 0xe0,
> + 0x90a2, 0xb4,
> + 0x90a3, 0x03,
> + 0x90a4, 0x0e,
> + 0x90a5, 0x90,
> + 0x90a6, 0x83,
> + 0x90a7, 0xbf,
> + 0x90a8, 0xe0,
> + 0x90a9, 0x60,
> + 0x90aa, 0x08,
> + 0x90ab, 0x90,
> + 0x90ac, 0x81,
> + 0x90ad, 0xfc,
> + 0x90ae, 0xe0,
> + 0x90af, 0xff,
> + 0x90b0, 0xc3,
> + 0x90b1, 0x13,
> + 0x90b2, 0xf0,
> + 0x90b3, 0x90,
> + 0x90b4, 0x81,
> + 0x90b5, 0xfc,
> + 0x90b6, 0xe0,
> + 0x90b7, 0xff,
> + 0x90b8, 0x02,
> + 0x90b9, 0x20,
> + 0x90ba, 0xda,
> + 0xc92c, 0x70,
> + 0xc92d, 0xbc,
> + 0xc92e, 0x80,
> + 0xc92f, 0xbb,
> + 0x90bb, 0x90,
> + 0x90bc, 0x82,
> + 0x90bd, 0x18,
> + 0x90be, 0xe0,
> + 0x90bf, 0xb4,
> + 0x90c0, 0x03,
> + 0x90c1, 0x06,
> + 0x90c2, 0x90,
> + 0x90c3, 0xc1,
> + 0x90c4, 0x06,
> + 0x90c5, 0x74,
> + 0x90c6, 0x05,
> + 0x90c7, 0xf0,
> + 0x90c8, 0x90,
> + 0x90c9, 0xd3,
> + 0x90ca, 0xa0,
> + 0x90cb, 0x02,
> + 0x90cc, 0x70,
> + 0x90cd, 0xbf,
> + 0xc930, 0x72,
> + 0xc931, 0x21,
> + 0xc932, 0x81,
> + 0xc933, 0x3b,
> + 0x913b, 0x7d,
> + 0x913c, 0x02,
> + 0x913d, 0x7f,
> + 0x913e, 0x7b,
> + 0x913f, 0x02,
> + 0x9140, 0x72,
> + 0x9141, 0x25,
> + 0xc934, 0x28,
> + 0xc935, 0xae,
> + 0xc936, 0x80,
> + 0xc937, 0xd2,
> + 0x90d2, 0xf0,
> + 0x90d3, 0x90,
> + 0x90d4, 0xd2,
> + 0x90d5, 0x0a,
> + 0x90d6, 0x02,
> + 0x90d7, 0x28,
> + 0x90d8, 0xb4,
> + 0xc938, 0x28,
> + 0xc939, 0xb1,
> + 0xc93a, 0x80,
> + 0xc93b, 0xd9,
> + 0x90d9, 0x90,
> + 0x90da, 0x83,
> + 0x90db, 0xba,
> + 0x90dc, 0xe0,
> + 0x90dd, 0xff,
> + 0x90de, 0x90,
> + 0x90df, 0xd2,
> + 0x90e0, 0x08,
> + 0x90e1, 0xe0,
> + 0x90e2, 0xe4,
> + 0x90e3, 0xef,
> + 0x90e4, 0xf0,
> + 0x90e5, 0xa3,
> + 0x90e6, 0xe0,
> + 0x90e7, 0x74,
> + 0x90e8, 0xff,
> + 0x90e9, 0xf0,
> + 0x90ea, 0x90,
> + 0x90eb, 0xd2,
> + 0x90ec, 0x0a,
> + 0x90ed, 0x02,
> + 0x90ee, 0x28,
> + 0x90ef, 0xb4,
> + 0xc93c, 0x29,
> + 0xc93d, 0x79,
> + 0xc93e, 0x80,
> + 0xc93f, 0xf0,
> + 0x90f0, 0xf0,
> + 0x90f1, 0x90,
> + 0x90f2, 0xd2,
> + 0x90f3, 0x0e,
> + 0x90f4, 0x02,
> + 0x90f5, 0x29,
> + 0x90f6, 0x7f,
> + 0xc940, 0x29,
> + 0xc941, 0x7c,
> + 0xc942, 0x80,
> + 0xc943, 0xf7,
> + 0x90f7, 0x90,
> + 0x90f8, 0x83,
> + 0x90f9, 0xba,
> + 0x90fa, 0xe0,
> + 0x90fb, 0xff,
> + 0x90fc, 0x90,
> + 0x90fd, 0xd2,
> + 0x90fe, 0x0c,
> + 0x90ff, 0xe0,
> + 0x9100, 0xe4,
> + 0x9101, 0xef,
> + 0x9102, 0xf0,
> + 0x9103, 0xa3,
> + 0x9104, 0xe0,
> + 0x9105, 0x74,
> + 0x9106, 0xff,
> + 0x9107, 0xf0,
> + 0x9108, 0x90,
> + 0x9109, 0xd2,
> + 0x910a, 0x0e,
> + 0x910b, 0x02,
> + 0x910c, 0x29,
> + 0x910d, 0x7f,
> + 0xc944, 0x2a,
> + 0xc945, 0x42,
> + 0xc946, 0x81,
> + 0xc947, 0x0e,
> + 0x910e, 0xf0,
> + 0x910f, 0x90,
> + 0x9110, 0xd2,
> + 0x9111, 0x12,
> + 0x9112, 0x02,
> + 0x9113, 0x2a,
> + 0x9114, 0x48,
> + 0xc948, 0x2a,
> + 0xc949, 0x45,
> + 0xc94a, 0x81,
> + 0xc94b, 0x15,
> + 0x9115, 0x90,
> + 0x9116, 0x83,
> + 0x9117, 0xba,
> + 0x9118, 0xe0,
> + 0x9119, 0xff,
> + 0x911a, 0x90,
> + 0x911b, 0xd2,
> + 0x911c, 0x10,
> + 0x911d, 0xe0,
> + 0x911e, 0xe4,
> + 0x911f, 0xef,
> + 0x9120, 0xf0,
> + 0x9121, 0xa3,
> + 0x9122, 0xe0,
> + 0x9123, 0x74,
> + 0x9124, 0xff,
> + 0x9125, 0xf0,
> + 0x9126, 0x90,
> + 0x9127, 0xd2,
> + 0x9128, 0x12,
> + 0x9129, 0x02,
> + 0x912a, 0x2a,
> + 0x912b, 0x48,
> + 0xc900, 0x01,
> + 0x0000, 0x00,
> +};
> +
> +static const u16 vs6624_p2[] = {
> + 0x806f, 0x01,
> + 0x058c, 0x01,
> + 0x0000, 0x00,
> +};
> +
> +static const u16 vs6624_run_setup[] = {
> + 0x1d18, 0x00, /* Enableconstrainedwhitebalance */
I'm sure many other reviewers will also ask you to replace numerical
register addresses with symbolic names, since it looks like a sufficiently
detailed documentation is available to you.
> + 0x200d, 0x3c, /* Damper PeakGain Output MSB */
Actually, some of these registers are already defined in your header:
+#define VS6624_PEAK_MIN_OUT_G_MSB 0x200D /* minimum damper output for gain MSB */
so, please, just use those names here and add defines for missing registers
> + 0x200e, 0x66, /* Damper PeakGain Output LSB */
> + 0x1f03, 0x65, /* Damper Low MSB */
> + 0x1f04, 0xd1, /* Damper Low LSB */
> + 0x1f07, 0x66, /* Damper High MSB */
> + 0x1f08, 0x62, /* Damper High LSB */
> + 0x1f0b, 0x00, /* Damper Min output MSB */
> + 0x1f0c, 0x00, /* Damper Min output LSB */
> + 0x2600, 0x00, /* Nora fDisable */
> + 0x2602, 0x04, /* Nora usage */
> + 0x260d, 0x63, /* Damper Low MSB Changed 0x63 to 0x65 */
> + 0x260e, 0xd1, /* Damper Low LSB */
> + 0x2611, 0x68, /* Damper High MSB */
> + 0x2612, 0xdd, /* Damper High LSB */
> + 0x2615, 0x3a, /* Damper Min output MSB */
> + 0x2616, 0x00, /* Damper Min output LSB */
> + 0x2480, 0x00, /* Disable */
> + 0x1d8a, 0x30, /* MAXWeightHigh */
> + 0x1d91, 0x62, /* fpDamperLowThresholdHigh MSB */
> + 0x1d92, 0x4a, /* fpDamperLowThresholdHigh LSB */
> + 0x1d95, 0x65, /* fpDamperHighThresholdHigh MSB */
> + 0x1d96, 0x0e, /* fpDamperHighThresholdHigh LSB */
> + 0x1da1, 0x3a, /* fpMinimumDamperOutputLow MSB */
> + 0x1da2, 0xb8, /* fpMinimumDamperOutputLow LSB */
> + 0x1e08, 0x06, /* MAXWeightLow */
> + 0x1e0a, 0x0a, /* MAXWeightHigh */
> + 0x1601, 0x3a, /* Red A MSB */
> + 0x1602, 0x14, /* Red A LSB */
> + 0x1605, 0x3b, /* Blue A MSB */
> + 0x1606, 0x85, /* BLue A LSB */
> + 0x1609, 0x3b, /* RED B MSB */
> + 0x160a, 0x85, /* RED B LSB */
> + 0x160d, 0x3a, /* Blue B MSB */
> + 0x160e, 0x14, /* Blue B LSB */
> + 0x1611, 0x30, /* Max Distance from Locus MSB */
> + 0x1612, 0x8f, /* Max Distance from Locus MSB */
> + 0x1614, 0x01, /* Enable constrainer */
> + 0x0000, 0x00,
> +};
> +
> +static const u16 vs6624_default[] = {
> + VS6624_CONTRAST0, 0x84,
> + VS6624_SATURATION0, 0x75,
> + VS6624_GAMMA0, 0x11,
> + VS6624_CONTRAST1, 0x84,
> + VS6624_SATURATION1, 0x75,
> + VS6624_GAMMA1, 0x11,
> + VS6624_MAN_RG, 0x80,
> + VS6624_MAN_GG, 0x80,
> + VS6624_MAN_BG, 0x80,
> + VS6624_WB_MODE, 0x1,
> + VS6624_EXPO_COMPENSATION, 0xfe,
> + VS6624_EXPO_METER, 0x0,
> + VS6624_LIGHT_FREQ, 0x64,
> + VS6624_PEAK_GAIN, 0xe,
> + VS6624_PEAK_LOW_THR, 0x28,
> + VS6624_HMIRROR0, 0x0,
> + VS6624_VFLIP0, 0x0,
> + VS6624_ZOOM_HSTEP0_MSB, 0x0,
> + VS6624_ZOOM_HSTEP0_LSB, 0x1,
> + VS6624_ZOOM_VSTEP0_MSB, 0x0,
> + VS6624_ZOOM_VSTEP0_LSB, 0x1,
> + VS6624_PAN_HSTEP0_MSB, 0x0,
> + VS6624_PAN_HSTEP0_LSB, 0xf,
> + VS6624_PAN_VSTEP0_MSB, 0x0,
> + VS6624_PAN_VSTEP0_LSB, 0xf,
> + VS6624_SENSOR_MODE, 0x1,
> + VS6624_SYNC_CODE_SETUP, 0x21,
> + VS6624_DISABLE_FR_DAMPER, 0x0,
> + VS6624_FR_DEN, 0x1,
> + VS6624_FR_NUM_LSB, 0xf,
> + VS6624_INIT_PIPE_SETUP, 0x0,
> + VS6624_IMG_FMT0, 0x0,
> + VS6624_YUV_SETUP, 0x1,
> + VS6624_IMAGE_SIZE0, 0x2,
> + 0x0000, 0x00,
> +};
[snip]
> +static int __devinit vs6624_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct vs6624 *sensor;
> + struct v4l2_subdev *sd;
> + struct v4l2_ctrl_handler *hdl;
> + u16 device_id;
> + const unsigned *ce;
> + int ret;
> +
> + /* Check if the adapter supports the needed features */
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> + return -EIO;
> +
> + ce = client->dev.platform_data;
> + if (ce == NULL)
> + return -EINVAL;
> +
> + ret = gpio_request(*ce, "VS6624 Chip Enable");
> + if (ret) {
> + v4l_err(client, "failed to request GPIO %d\n", *ce);
> + return ret;
> + }
> + gpio_direction_output(*ce, 1);
> + /* wait 100ms before any further i2c writes are performed */
> + mdelay(100);
Logically, it could be a good idea to toggle chip-enable in your
v4l2_subdev_core_ops::s_power() method, but if you really have to wait for
100ms before accessing the chip...
> +
> + sensor = kzalloc(sizeof(*sensor), GFP_KERNEL);
> + if (sensor == NULL) {
> + gpio_free(*ce);
> + return -ENOMEM;
> + }
> +
> + sd = &sensor->sd;
> + v4l2_i2c_subdev_init(sd, client, &vs6624_ops);
> +
> + vs6624_writeregs(sd, vs6624_p1);
> + vs6624_write(sd, VS6624_MICRO_EN, 0x2);
> + vs6624_write(sd, VS6624_DIO_EN, 0x1);
> + mdelay(10);
> + vs6624_writeregs(sd, vs6624_p2);
> +
> + /* make sure the sensor is vs6624 */
> + device_id = vs6624_read(sd, VS6624_DEV_ID_MSB) << 8
> + | vs6624_read(sd, VS6624_DEV_ID_LSB);
Wow... this is like saying - sorry, guys, the chip, we just killed by
writing random rubbish to it wasn't a vs6624;-) I mean, are ID registers
really unreadable before writing defaults to registers?
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
