On Mon, Mar 18, 2013 at 08:58:47AM +0100, Hans Verkuil wrote: > Hi Jon Arne, > > Here is my review of this driver. > Great, thank you. > Regards, > > Hans > > On Thu March 14 2013 15:06:58 Jon Arne Jørgensen wrote: > > This is the core of the smi2021 module. > > It will register the module with the kernel, and register the > > usb probe function. > > > > Signed-off-by: Jon Arne Jørgensen <jonarne@xxxxxxxxxx> > > --- > > drivers/media/usb/smi2021/smi2021_main.c | 339 +++++++++++++++++++++++++++++++ > > 1 file changed, 339 insertions(+) > > create mode 100644 drivers/media/usb/smi2021/smi2021_main.c > > > > diff --git a/drivers/media/usb/smi2021/smi2021_main.c b/drivers/media/usb/smi2021/smi2021_main.c > > new file mode 100644 > > index 0000000..cc600e7 > > --- /dev/null > > +++ b/drivers/media/usb/smi2021/smi2021_main.c > > @@ -0,0 +1,339 @@ > > +/******************************************************************************* > > + * smi2021_main.c * > > + * * > > + * USB Driver for SMI2021 - EasyCAP * > > + * USB ID 1c88:003c * > > + * * > > + * ***************************************************************************** > > + * > > + * Copyright 2011-2013 Jon Arne Jørgensen > > + * <jonjon.arnearne--a.t--gmail.com> > > + * > > + * Copyright 2011, 2012 Tony Brown, Michal Demin, Jeffry Johnston > > + * > > + * This file is part of SMI2021 > > + * http://code.google.com/p/easycap-somagic-linux/ > > + * > > + * This program is free software: you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License as published by > > + * the Free Software Foundation, either version 2 of the License, or > > + * (at your option) any later version. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + * > > + * You should have received a copy of the GNU General Public License > > + * along with this program; if not, see <http://www.gnu.org/licenses/>. > > + * > > + * This driver is heavily influensed by the STK1160 driver. > > + * Copyright (C) 2012 Ezequiel Garcia > > + * <elezegarcia--a.t--gmail.com> > > + * > > + */ > > + > > +#include "smi2021.h" > > + > > +#define VENDOR_ID 0x1c88 > > + > > +static unsigned int imput; > > +module_param(imput, int, 0644); > > +MODULE_PARM_DESC(input, "Set default input"); > > + > > +MODULE_LICENSE("GPL"); > > +MODULE_AUTHOR("Jon Arne Jørgensen <jonjon.arnearne--a.t--gmail.com>"); > > +MODULE_DESCRIPTION("SMI2021 - EasyCap"); > > +MODULE_VERSION(SMI2021_DRIVER_VERSION); > > + > > + > > +struct usb_device_id smi2021_usb_device_id_table[] = { > > Should be static const > Ok. > > + { USB_DEVICE(VENDOR_ID, 0x003c) }, > > + { USB_DEVICE(VENDOR_ID, 0x003d) }, > > + { USB_DEVICE(VENDOR_ID, 0x003e) }, > > + { USB_DEVICE(VENDOR_ID, 0x003f) }, > > + { } > > +}; > > + > > +MODULE_DEVICE_TABLE(usb, smi2021_usb_device_id_table); > > + > > +static unsigned short saa7113_addrs[] = { > > + 0x4a, > > + I2C_CLIENT_END > > +}; > > Don't use this. See below for more info. > Removed. > > + > > +/******************************************************************************/ > > +/* */ > > +/* Write to saa7113 */ > > +/* */ > > +/******************************************************************************/ > > + > > +inline int transfer_usb_ctrl(struct smi2021_dev *dev, u8 *data, int len) > > +{ > > + return usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0x00), > > + 0x01, USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE, > > + 0x0b, 0x00, > > + data, len, 1000); > > +} > > + > > +int smi2021_write_reg(struct smi2021_dev *dev, u8 addr, u16 reg, u8 val) > > +{ > > + int rc; > > + u8 snd_data[8]; > > + > > + memset(snd_data, 0x00, 8); > > + > > + snd_data[SMI2021_CTRL_HEAD] = 0x0b; > > + snd_data[SMI2021_CTRL_ADDR] = addr; > > + snd_data[SMI2021_CTRL_DATA_SIZE] = 0x01; > > + > > + if (addr) { > > + /* This is I2C data for the saa7113 chip */ > > + snd_data[SMI2021_CTRL_BM_DATA_TYPE] = 0xc0; > > + snd_data[SMI2021_CTRL_BM_DATA_OFFSET] = 0x01; > > + > > + snd_data[SMI2021_CTRL_I2C_REG] = reg; > > + snd_data[SMI2021_CTRL_I2C_VAL] = val; > > + } else { > > + /* This is register settings for the smi2021 chip */ > > + snd_data[SMI2021_CTRL_BM_DATA_OFFSET] = 0x82; > > + > > + snd_data[SMI2021_CTRL_REG_HI] = __cpu_to_be16(reg) >> 8; > > + snd_data[SMI2021_CTRL_REG_LO] = __cpu_to_be16(reg); > > This looks really weird. On a little endian system REG_HI is now initialized > with the LSB, not MSB and the other way around on a big endian system. > The _cpu_to_be16 certainly doesn't belong here. > The device expects the register address as a 16bit Big Endian number. I though this code would move the HIGH byte of a big endian number into REG_HI, and the LOW byte into REG_LO. This works as expected on my little endian intel I7, but I will have take a second look at my code, I guesss I can do this without the conversion into be16. > > + > > + } > > + > > + rc = transfer_usb_ctrl(dev, snd_data, 8); > > + if (rc < 0) { > > + smi2021_warn("write failed on register 0x%x, errno: %d\n", > > + reg, rc); > > + return rc; > > + } > > + > > + return 0; > > +} > > + > > +int smi2021_read_reg(struct smi2021_dev *dev, u8 addr, u16 reg, u8 *val) > > +{ > > + int rc; > > + u8 rcv_data[13]; > > + u8 snd_data[8]; > > + memset(rcv_data, 0x00, 13); > > + memset(snd_data, 0x00, 8); > > + > > + snd_data[SMI2021_CTRL_HEAD] = 0x0b; > > + snd_data[SMI2021_CTRL_ADDR] = addr; > > + snd_data[SMI2021_CTRL_BM_DATA_TYPE] = 0x84; > > + snd_data[SMI2021_CTRL_DATA_SIZE] = 0x01; > > + snd_data[SMI2021_CTRL_I2C_REG] = reg; > > + > > + *val = 0; > > + > > + rc = transfer_usb_ctrl(dev, snd_data, 8); > > + if (rc < 0) { > > + smi2021_warn( > > + "1st pass failing to read reg 0x%x, usb-errno: %d\n", > > + reg, rc); > > + return rc; > > + } > > + > > + snd_data[SMI2021_CTRL_BM_DATA_TYPE] = 0xa0; > > + rc = transfer_usb_ctrl(dev, snd_data, 8); > > + if (rc < 0) { > > + smi2021_warn( > > + "2nd pass failing to read reg 0x%x, usb-errno: %d\n", > > + reg, rc); > > + return rc; > > + } > > + > > + rc = usb_control_msg(dev->udev, > > + usb_rcvctrlpipe(dev->udev, 0x80), 0x01, > > + USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE, > > + 0x0b, 0x00, rcv_data, 13, 1000); > > + if (rc < 0) { > > + smi2021_warn("Failed to read reg 0x%x, usb-errno: %d\n", > > + reg, rc); > > + return rc; > > + } > > + > > + *val = rcv_data[SMI2021_CTRL_I2C_RCV_VAL]; > > + return 0; > > +} > > + > > +static void smi2021_reset_device(struct smi2021_dev *dev) > > +{ > > + smi2021_write_reg(dev, 0, 0x3a, 0x80); > > + smi2021_write_reg(dev, 0, 0x3b, 0x80); > > + smi2021_write_reg(dev, 0, 0x3b, 0x00); > > +} > > + > > +static void release_v4l2_dev(struct v4l2_device *v4l2_dev) > > +{ > > + struct smi2021_dev *dev = container_of(v4l2_dev, struct smi2021_dev, > > + v4l2_dev); > > + smi2021_dbg("Releasing all resources\n"); > > + > > + smi2021_i2c_unregister(dev); > > + > > + v4l2_ctrl_handler_free(&dev->ctrl_handler); > > + v4l2_device_unregister(&dev->v4l2_dev); > > + kfree(dev); > > +} > > + > > +#define hb_mult(w_max_packet_size) (1 + (((w_max_packet_size) >> 11) & 0x03)) > > + > > +static int smi2021_scan_usb(struct usb_interface *intf, struct usb_device *udev) > > +{ > > + int i, e, ifnum, sizedescr, size; > > + const struct usb_endpoint_descriptor *desc; > > + ifnum = intf->altsetting[0].desc.bInterfaceNumber; > > + > > + for (i = 0; i < intf->num_altsetting; i++) { > > + for (e = 0; e < intf->altsetting[i].desc.bNumEndpoints; e++) { > > + desc = &intf->altsetting[i].endpoint[e].desc; > > + sizedescr = le16_to_cpu(desc->wMaxPacketSize); > > + size = sizedescr & 0x7ff; > > + > > + if (udev->speed == USB_SPEED_HIGH) > > + size = size * hb_mult(sizedescr); > > + } > > + } > > + return 0; > > +} > > + > > +/******************************************************************************/ > > +/* */ > > +/* DEVICE - PROBE & DISCONNECT */ > > +/* */ > > +/******************************************************************************/ > > +static int smi2021_usb_probe(struct usb_interface *intf, > > + const struct usb_device_id *devid) > > +{ > > + int rc = 0; > > + struct usb_device *udev = interface_to_usbdev(intf); > > + struct smi2021_dev *dev; > > + > > + if (udev == (struct usb_device *)NULL) { > > + smi2021_err("device is NULL\n"); > > + return -EFAULT; > > + } > > + > > + smi2021_scan_usb(intf, udev); > > + > > + dev = kzalloc(sizeof(struct smi2021_dev), GFP_KERNEL); > > + if (dev == NULL) > > + return -ENOMEM; > > + > > + dev->udev = udev; > > + dev->dev = &intf->dev; > > + usb_set_intfdata(intf, dev); > > + > > + /* Initialize videobuf2 stuff */ > > + rc = smi2021_vb2_setup(dev); > > + if (rc < 0) > > + goto free_err; > > + > > + spin_lock_init(&dev->buf_lock); > > + mutex_init(&dev->v4l2_lock); > > + mutex_init(&dev->vb_queue_lock); > > + > > + rc = v4l2_ctrl_handler_init(&dev->ctrl_handler, 0); > > + if (rc) { > > + smi2021_err("v4l2_ctrl_handler_init failed with: %d\n", rc); > > + goto free_err; > > + } > > + > > + dev->v4l2_dev.release = release_v4l2_dev; > > + dev->v4l2_dev.ctrl_handler = &dev->ctrl_handler; > > + rc = v4l2_device_register(dev->dev, &dev->v4l2_dev); > > + if (rc) { > > + smi2021_err("v4l2_device_register failed with %d\n", rc); > > + goto free_ctrl; > > + } > > + > > + smi2021_reset_device(dev); > > + > > + rc = smi2021_i2c_register(dev); > > + if (rc < 0) > > + goto unreg_v4l2; > > + > > + dev->sd_saa7113 = v4l2_i2c_new_subdev(&dev->v4l2_dev, &dev->i2c_adap, > > + "saa7113", 0, saa7113_addrs); > > Fill in the addr argument directly and use NULL for probe_addrs. Probing is > not something new drivers should do. > > See also my comments in the i2c.c review. > I will do this as you suggested in the ic2 review. > > + > > + smi2021_dbg("Driver version %s successfully loaded\n", > > + SMI2021_DRIVER_VERSION); > > + > > + v4l2_device_call_all(&dev->v4l2_dev, 0, core, reset, 0); > > + v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_stream, 0); > > + > > + rc = smi2021_snd_register(dev); > > + if (rc < 0) > > + goto unreg_i2c; > > + > > + rc = smi2021_video_register(dev); > > + if (rc < 0) > > + goto unreg_snd; > > + > > + return 0; > > + > > +unreg_snd: > > + smi2021_snd_unregister(dev); > > +unreg_i2c: > > + smi2021_i2c_unregister(dev); > > +unreg_v4l2: > > + v4l2_device_unregister(&dev->v4l2_dev); > > +free_ctrl: > > + v4l2_ctrl_handler_free(&dev->ctrl_handler); > > +free_err: > > + kfree(dev); > > + > > + return rc; > > +} > > + > > +static void smi2021_usb_disconnect(struct usb_interface *intf) > > +{ > > + struct smi2021_dev *dev = usb_get_intfdata(intf); > > + > > + smi2021_dbg("Going for release!\n"); > > + > > + usb_set_intfdata(intf, NULL); > > + > > + mutex_lock(&dev->vb_queue_lock); > > + mutex_lock(&dev->v4l2_lock); > > + > > + smi2021_uninit_isoc(dev); > > + smi2021_clear_queue(dev); > > + > > + video_unregister_device(&dev->vdev); > > + v4l2_device_disconnect(&dev->v4l2_dev); > > + > > + /* This way current users can detect device is gone */ > > + dev->udev = NULL; > > + > > + mutex_unlock(&dev->v4l2_lock); > > + mutex_unlock(&dev->vb_queue_lock); > > + > > + smi2021_snd_unregister(dev); > > + > > + /* > > + * This calls release_v4l2_dev if it's the last reference. > > + * Otherwise, the release is postponed until there are no users left. > > + */ > > + v4l2_device_put(&dev->v4l2_dev); > > +} > > + > > +/******************************************************************************/ > > +/* */ > > +/* MODULE - INIT & EXIT */ > > +/* */ > > +/******************************************************************************/ > > + > > +struct usb_driver smi2021_usb_driver = { > > + .name = "smi2021", > > + .id_table = smi2021_usb_device_id_table, > > + .probe = smi2021_usb_probe, > > + .disconnect = smi2021_usb_disconnect > > +}; > > + > > +module_usb_driver(smi2021_usb_driver); > > + > > > > Regards, > > Hans > -- > 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 -- 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
