On Wed, 14 Jan 2009 20:59:41 -0600
Kyle Guinn <[email protected]> wrote:

> gspca: Add MR97310A driver
> 
> From: Kyle Guinn <[email protected]>
> 
> This patch adds support for USB webcams based on the MR97310A chip.
> It was tested with an Aiptek PenCam VGA+ webcam.

Hi again,

Here are some remarks about your patch.

        [snip]
> +/* the bytes to write are in gspca_dev->usb_buf */
> +static int reg_w(struct gspca_dev *gspca_dev,
> +              __u16 index, int len)
> +{
> +     int rc;
> +
> +     rc = usb_bulk_msg(gspca_dev->dev,
> +                       usb_sndbulkpipe(gspca_dev->dev, 4),
> +                       gspca_dev->usb_buf, len, 0, 500);
> +     if (rc < 0)
> +             PDEBUG(D_ERR, "reg write [%02x] error %d", index,
> rc);
> +     return rc;
> +}

The 'index' parameter is not useful: the register is always in the first
byte of the buffer.

        [snip]
> +/* this function is called at probe time */
> +static int sd_config(struct gspca_dev *gspca_dev,
> +                  const struct usb_device_id *id)
> +{
> +     struct cam *cam;
> +
> +     cam = &gspca_dev->cam;
> +     cam->epaddr = 0x01;

This variable has been removed in the last versions of gspca.

> +     cam->cam_mode = vga_mode;
> +     cam->nmodes = ARRAY_SIZE(vga_mode);
> +     return 0;
> +}
        [snip]
> +static int sd_start(struct gspca_dev *gspca_dev)
> +{
> +     struct sd *sd = (struct sd *) gspca_dev;
> +     __u8 *data = gspca_dev->usb_buf;
> +     int err_code;
> +     int intpipe;
> +
> +     PDEBUG(D_STREAM, "camera start, iface %d, alt 8",
> gspca_dev->iface);
> +     err_code = usb_set_interface(gspca_dev->dev,
> gspca_dev->iface, 8);
> +     if (err_code < 0) {
> +             PDEBUG(D_ERR|D_STREAM, "Set packet size: set
> interface error");
> +             return err_code;
> +     }

The usb_set_interface() is already done in the gspca main.

        [snip]
> +     sd->sof_read = 0;
> +
> +     intpipe = usb_sndintpipe(gspca_dev->dev, 0);
> +     err_code = usb_clear_halt(gspca_dev->dev, intpipe);

Is this really needed?

> +     data[0] = 0x00;
> +     data[1] = 0x4d;  /* ISOC transfering enable... */
> +     reg_w(gspca_dev, data[0], 2);
> +     return err_code;
> +}
        [snip]

-- 
Ken ar c'hentan |             ** Breizh ha Linux atav! **
Jef             |               http://moinejf.free.fr/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to