On Thursday 15 January 2009 06:13:29 Jean-Francois Moine wrote:
> Hi again,
>
> Here are some remarks about your patch.
>
> > +/* 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.
>
Noted. I used the mars subdriver as a basis for this driver, and apparently
an old version of it at that. Will be fixed in v2.
> > +/* 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.
>
Will be removed in v2.
> > +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.
>
Also borrowed from the mars subdriver. Will be removed in v2.
> > + sd->sof_read = 0;
> > +
> > + intpipe = usb_sndintpipe(gspca_dev->dev, 0);
> > + err_code = usb_clear_halt(gspca_dev->dev, intpipe);
>
> Is this really needed?
>
Also borrowed from the mars subdriver, and doesn't appear to be necessary for
the camera to work. Will be removed in v2.
Regards,
-Kyle
--
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
