RE: [PATCH 3/5] OV3640: Add driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Alexey,

> -----Original Message-----
> From: Alexey Klimov [mailto:klimov.linux@xxxxxxxxx]

<snip>

> > --- /dev/null
> > +++ b/drivers/media/video/ov3640.c
> > @@ -0,0 +1,2202 @@
> > +/*
> > + * drivers/media/video/ov3640.c
> > + *
> > + * ov3640 sensor driver
> > + *
> > + *
> > + * Copyright (C) 2008 Texas Instruments.
> 
> 2009 ?

Fixed.

<snip>

> > +static int ov3640_read_reg(struct i2c_client *client, u16 data_length,
> u16 reg,
> > +                                                               u32
> *val)
> > +{
> > +       int err = 0;
> > +       struct i2c_msg msg[1];
> > +       unsigned char data[4];
> > +
> > +       if (!client->adapter)
> > +               return -ENODEV;
> > +
> > +       msg->addr = client->addr;
> > +       msg->flags = I2C_M_WR;
> > +       msg->len = 2;
> > +       msg->buf = data;
> > +
> > +       /* High byte goes out first */
> > +       data[0] = (u8) (reg >> 8);
> > +       data[1] = (u8) (reg & 0xff);
> > +
> > +       err = i2c_transfer(client->adapter, msg, 1);
> 
> Please, let me understand.. You call i2c_transfer() and ask it to
> transfer one message(third parameter), right ?
> So, the returned value is negative errno or the number of messages
> executed. Logic says that you should check somethin like this:
> 	if (err = 1) {
> 		good;
> 	} else {
> 		i2c_transfer failed;
> 		we should deal with it(printk, try again, etc)
> 	}
> 
> Or even:
> 	if (unlikely(err != 1)) {
> 		i2c_transfer failed;
> 	}
> 	Good code continue;
> 
> Right or wrong ?

The idea of this piece of code is to:

- First do a write transfer with nothing else than the register address to read.
- Then do a I2C read transfer to receive the value from register selected in previous write with the size of data_length variable.

I do agree that perhaps a cleanup for this piece of code needs to be done... I'll clean it up, test, and repost the resulting patch for your review. :)

> 
> > +       if (err >= 0) {
> > +               mdelay(3);
> > +               msg->flags = I2C_M_RD;
> > +               msg->len = data_length;
> > +               err = i2c_transfer(client->adapter, msg, 1);
> > +       }
> > +       if (err >= 0) {
> > +               *val = 0;
> > +               /* High byte comes first */
> > +               if (data_length == 1)
> > +                       *val = data[0];
> > +               else if (data_length == 2)
> > +                       *val = data[1] + (data[0] << 8);
> > +               else
> > +                       *val = data[3] + (data[2] << 8) +
> > +                               (data[1] << 16) + (data[0] << 24);
> > +               return 0;
> > +       }
> > +       dev_err(&client->dev, "read from offset 0x%x error %d", reg,
> err);
> 
> "\n" should be in dev_err.

Done

> 
> > +       return err;
> > +}
> > +
> > +/* Write a value to a register in ov3640 sensor device.
> > + * @client: i2c driver client structure.
> > + * @reg: Address of the register to read value from.
> > + * @val: Value to be written to a specific register.
> > + * Returns zero if successful, or non-zero otherwise.
> > + */
> > +static int ov3640_write_reg(struct i2c_client *client, u16 reg, u8 val)
> > +{
> > +       int err = 0;
> > +       struct i2c_msg msg[1];
> > +       unsigned char data[3];
> > +       int retries = 0;
> > +
> > +       if (!client->adapter)
> > +               return -ENODEV;
> > +retry:
> > +       msg->addr = client->addr;
> > +       msg->flags = I2C_M_WR;
> > +       msg->len = 3;
> > +       msg->buf = data;
> > +
> > +       /* high byte goes out first */
> > +       data[0] = (u8) (reg >> 8);
> > +       data[1] = (u8) (reg & 0xff);
> > +       data[2] = val;
> > +
> > +       err = i2c_transfer(client->adapter, msg, 1);
> > +       udelay(50);
> > +
> > +       if (err >= 0)
> 
> Well, probably all checks of returned values after i2c_transfer should
> be reformatted in right way.

I'll redesign this...

> 
> 
> > +               return 0;
> > +
> > +       if (retries <= 5) {
> > +               dev_dbg(&client->dev, "Retrying I2C... %d", retries);
> 
> "\n"

Done.

<snip>

> > +       /* FIXME: QXGA framerate setting forced to 15 FPS */
> > +       if (isize == QXGA) {
> > +               err = ov3640_write_reg(client, OV3640_PLL_1, 0x32);
> > +               err = ov3640_write_reg(client, OV3640_PLL_2, 0x21);
> > +               err = ov3640_write_reg(client, OV3640_PLL_3, 0x21);
> > +               err = ov3640_write_reg(client, OV3640_CLK, 0x01);
> > +               err = ov3640_write_reg(client, 0x304c, 0x81);
> 
> I see no checking of returned values. For example, if first function
> failed, rest functions will keep going.

Agreed.. I think I'll definitely redesign this to avoid this kind of trouble.

<snip>

> > +
> > +       /* Common registers */
> > +       err = ov3640_write_regs(client, ov3640_common[isize]);
> > +
> > +       /* Configure image size and pixel format */
> > +       err = ov3640_write_regs(client, ov3640_reg_init[pfmt][isize]);
> > +
> > +       /* Setting of frame rate (OV suggested way) */
> > +       err = ov3640_set_framerate(client, &sensor->timeperframe,
> isize);
> > +#ifdef CONFIG_VIDEO_OV3640_CSI2
> > +       /* Set CSI2 common register settings */
> > +       err = ov3640_write_regs(client, ov3640_common_csi2);
> > +#endif
> 
> Again, no checking of err.

Agree, will fix..

<snip>

> > +
> > +               err = ov3640_write_reg(client, OV3640_SIZE_IN_MISC,
> > +                                                       (vsize_h |
> hsize_h));
> > +               err = ov3640_write_reg(client, OV3640_HSIZE_IN_L,
> hsize_l);
> > +               err = ov3640_write_reg(client, OV3640_VSIZE_IN_L,
> vsize_l);
> > +               err = ov3640_write_reg(client, OV3640_SIZE_OUT_MISC,
> > +                                                       (height_h |
> width_h));
> > +               err = ov3640_write_reg(client, OV3640_HSIZE_OUT_L,
> width_l);
> > +               err = ov3640_write_reg(client, OV3640_VSIZE_OUT_L,
> height_l);
> > +               err = ov3640_write_reg(client, OV3640_ISP_PAD_CTR2,
> 0x42);
> > +               err = ov3640_write_reg(client, OV3640_ISP_XOUT_H,
> width_h);
> > +               err = ov3640_write_reg(client, OV3640_ISP_XOUT_L,
> > +                                                       (width_l  -
> 0x08));
> > +               err = ov3640_write_reg(client, OV3640_ISP_YOUT_H,
> > +                                                       (height_h >>
> 4));
> > +               err = ov3640_write_reg(client, OV3640_ISP_YOUT_L,
> > +                                                       (height_l -
> 0x04));
> 
> The same thing here.

Agree, will fix..

<snip> 

> > +static int ioctl_s_ctrl(struct v4l2_int_device *s,
> > +                            struct v4l2_control *vc)
> > +{
> > +       int retval = -EINVAL;
> > +       int i;
> > +       struct ov3640_sensor *sensor = s->priv;
> > +       struct i2c_client *client = sensor->i2c_client;
> > +       struct vcontrol *lvc;
> > +
> > +       i = find_vctrl(vc->id);
> > +       if (i < 0)
> > +               return -EINVAL;
> > +
> > +       lvc = &video_control[i];
> > +
> > +       switch (vc->id) {
> > +       case V4L2_CID_BRIGHTNESS:
> > +               if (vc->value >= 0 && vc->value <= 6) {
> > +                       retval = ov3640_write_regs(client,
> > +                                                       brightness[vc-
> >value]);
> > +               } else {
> > +                       dev_err(&client->dev, "BRIGHTNESS LEVEL NOT
> SUPPORTED");
> > +                       return -EINVAL;
> > +               }
> > +               break;
> > +       case V4L2_CID_CONTRAST:
> > +               if (vc->value >= 0 && vc->value <= 6)
> > +                       retval = ov3640_write_regs(client, contrast[vc-
> >value]);
> > +               else {
> > +                       dev_err(&client->dev, "CONTRAST LEVEL NOT
> SUPPORTED");
> > +                       return -EINVAL;
> > +               }
> > +               break;
> > +       case V4L2_CID_PRIVATE_BASE:
> > +               if (vc->value >= 0 && vc->value <= 2)
> > +                       retval = ov3640_write_regs(client, colors[vc-
> >value]);
> > +               else {
> > +                       dev_err(&client->dev, "COLOR LEVEL NOT
> SUPPORTED");
> > +                       return -EINVAL;
> 
> I don't sure - may be you like big letters in logs..
> Anyway, "\n" is absent in this 3 dev_err.

Ok, removed all caps, and added "\n".

Thanks,
Sergio
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux