Hi Ezequiel,
a few minor comments below...
On 06/23/2012 08:36 PM, Ezequiel Garcia wrote:
> This driver adds support for stk1160 usb bridge as used in some
> video/audio usb capture devices.
> It is a complete rewrite of staging/media/easycap driver and
> it's expected as a future replacement.
>
> Signed-off-by: Ezequiel Garcia<elezegar...@gmail.com>
> ---
[...]
> +/*
> + * Read/Write stk registers
> + */
> +int stk1160_read_reg(struct stk1160 *dev, u16 reg, u8 *value)
> +{
> + int ret;
> +
> + *value = 0;
> + ret = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
> + 0x00,
> + USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> + 0x00,
> + reg,
> + value,
> + sizeof(u8),
> + HZ);
A bit strange indentation, but it's up to you to keep it or not.
> + if (ret< 0) {
> + stk1160_err("read failed on reg 0x%x (%d)\n",
> + reg, ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +int stk1160_write_reg(struct stk1160 *dev, u16 reg, u16 value)
> +{
> + int ret;
> +
> + ret = usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0),
> + 0x01,
> + USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> + value,
> + reg,
> + NULL,
> + 0,
> + HZ);
> + if (ret< 0) {
> + stk1160_err("write failed on reg 0x%x (%d)\n",
> + reg, ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +/* TODO: We should break this into pieces */
> +static void stk1160_reg_reset(struct stk1160 *dev)
> +{
> + int i;
> +
> + static struct regval ctl[] = {
static const struct regval... ?
> + {STK1160_GCTRL+2, 0x0078},
> +
> + {STK1160_RMCTL+1, 0x0000},
> + {STK1160_RMCTL+3, 0x0002},
> +
> + {STK1160_PLLSO, 0x0010},
> + {STK1160_PLLSO+1, 0x0000},
> + {STK1160_PLLSO+2, 0x0014},
> + {STK1160_PLLSO+3, 0x000E},
> +
> + {STK1160_PLLFD, 0x0046},
> +
> + /* Timing generator setup */
> + {STK1160_TIGEN, 0x0012},
> + {STK1160_TICTL, 0x002D},
> + {STK1160_TICTL+1, 0x0001},
> + {STK1160_TICTL+2, 0x0000},
> + {STK1160_TICTL+3, 0x0000},
> + {STK1160_TIGEN, 0x0080},
> +
> + {0xffff, 0xffff}
> + };
> +
> + for (i = 0; ctl[i].reg != 0xffff; i++)
> + stk1160_write_reg(dev, ctl[i].reg, ctl[i].val);
> +
> + /* Set selected input from module parameter */
> + switch (dev->ctl_input) {
> + case 0:
> + stk1160_write_reg(dev, STK1160_GCTRL, 0x98);
> + break;
> + case 1:
> + stk1160_write_reg(dev, STK1160_GCTRL, 0x90);
> + break;
> + case 2:
> + stk1160_write_reg(dev, STK1160_GCTRL, 0x88);
> + break;
> + case 3:
> + stk1160_write_reg(dev, STK1160_GCTRL, 0x80);
> + break;
How about doing something like:
static const u8 gctrl[] = {
0x98, 0x90, 0x88, 0x80
};
if (dev->ctl_input < ARRAY_SIZE(gctrl))
stk1160_write_reg(dev, STK1160_GCTRL, gctrl[dev->ctl_input]);
?
> + }
> +}
> +
...
> +static int stk1160_i2c_write_reg(struct stk1160 *dev, u8 addr,
> + u8 reg, u8 value)
> +{
> + int rc, timeout;
> + u8 flag;
> +
> + /* Set serial device address */
> + rc = stk1160_write_reg(dev, STK1160_SICTL_SDA, addr);
> + if (rc< 0)
> + return rc;
> +
> + /* Set i2c device register sub-address */
> + rc = stk1160_write_reg(dev, STK1160_SBUSW_WA, reg);
> + if (rc< 0)
> + return rc;
> +
> + /* Set i2c device register value */
> + rc = stk1160_write_reg(dev, STK1160_SBUSW_WD, value);
> + if (rc< 0)
> + return rc;
> +
> + /* Start write now */
> + rc = stk1160_write_reg(dev, STK1160_SICTL, 0x01);
> + if (rc< 0)
> + return rc;
> +
> + /* Wait until Write Finish bit is set */
> + for (timeout = 100; timeout> 0;
> + timeout -= 20) {
> + stk1160_read_reg(dev, STK1160_SICTL+1,&flag);
> + /* write done? */
> + if (flag& 0x04)
> + break;
> +
> + msleep(20);
> + }
A bit strange loop, perhaps it's worth to rework it as following:
unsigned long end = jiffies + msecs_to_jiffies(timeout);
while (time_is_after_jiffies(end)) {
...
usleep_range(...);
}
to better control the actual time spent on busy waiting.
You may also want to create a separate function i.e.
stk1160_i2c_busy_wait(struct stk1160 *dev, u8 wait_bit_mask, unsigned int
timeout)
and use to avoid repeating similar pattern.
> +
> + if (timeout == 0)
> + /* return EBUSY, or EFAULT, or what ? */
-ETIMEDOUT ?
> + return -EIO;
> +
> + return 0;
> +}
> +
> +static int stk1160_i2c_read_reg(struct stk1160 *dev, u8 addr,
> + u8 reg, u8 *value)
> +{
> + int rc, timeout;
> + u8 flag;
> +
> + /* Set serial device address */
> + rc = stk1160_write_reg(dev, STK1160_SICTL_SDA, addr);
> + if (rc< 0)
> + return rc;
> +
> + /* Set i2c device register sub-address */
> + rc = stk1160_write_reg(dev, STK1160_SBUSR_RA, reg);
> + if (rc< 0)
> + return rc;
> +
> + /* Start read now */
> + rc = stk1160_write_reg(dev, STK1160_SICTL, 0x20);
> + if (rc< 0)
> + return rc;
> +
> + /* Wait until Read Finish bit is set */
> + for (timeout = 100; timeout> 0;
> + timeout -= 20) {
> + stk1160_read_reg(dev, STK1160_SICTL+1,&flag);
> + /* read done? */
> + if (flag& 0x01)
> + break;
> +
> + msleep(20);
> + }
> +
> + if (timeout == 0)
> + /* return EBUSY, or EFAULT, or what ? */
> + return -EIO;
> +
> + stk1160_read_reg(dev, STK1160_SBUSR_RD, value);
> + if (rc< 0)
> + return rc;
> +
> + return 0;
> +}
> +
> +/*
> + * stk1160_i2c_check_for_device()
> + * check if there is a i2c_device at the supplied address
> + */
> +static int stk1160_i2c_check_for_device(struct stk1160 *dev,
> + unsigned char addr)
> +{
> + int rc, write_timeout;
> + u8 flag;
> +
> + /* Set serial device address */
> + rc = stk1160_write_reg(dev, STK1160_SICTL_SDA, addr);
> + if (rc< 0)
> + return rc;
> +
> + /* Set device sub-address, we'll chip version reg */
> + rc = stk1160_write_reg(dev, STK1160_SBUSR_RA, 0x00);
> + if (rc< 0)
> + return rc;
> +
> + /* Start read now */
> + rc = stk1160_write_reg(dev, STK1160_SICTL, 0x20);
> + if (rc< 0)
> + return rc;
> +
> + /* Wait until Read Finish bit is set */
> + for (write_timeout = 40; write_timeout> 0;
> + write_timeout -= 20) {
> + stk1160_read_reg(dev, STK1160_SICTL+1,&flag);
> + if (flag& 0x01)
> + return 0;
> +
> + msleep(20);
> + }
> +
> + return -ENODEV;
> +}
> +
...
> +
> +/* GPIO Control */
> +#define STK1160_GCTRL 0x000
> +
> +/* Remote Wakup Control */
> +#define STK1160_RMCTL 0x00C
Care to use lower case for all hex numbers ?
> +
> +/*
> + * Decoder Control Register:
> + * This byte controls capture start/stop
> + * with bit #7 (0x?? OR 0x80 to activate).
> + */
> +#define STK1160_DCTRL 0x100
> +
> +/* Capture Frame Start Position */
> +#define STK116_CFSPO 0x110
> +#define STK116_CFSPO_STX_L 0x110
> +#define STK116_CFSPO_STX_H 0x111
> +#define STK116_CFSPO_STY_L 0x112
> +#define STK116_CFSPO_STY_H 0x113
> +
> +/* Capture Frame End Position */
> +#define STK116_CFEPO 0x114
> +#define STK116_CFEPO_ENX_L 0x114
> +#define STK116_CFEPO_ENX_H 0x115
> +#define STK116_CFEPO_ENY_L 0x116
> +#define STK116_CFEPO_ENY_H 0x117
> +
> +/* Serial Interface Control */
> +#define STK1160_SICTL 0x200
> +#define STK1160_SICTL_CD 0x202
> +#define STK1160_SICTL_SDA 0x203
> +
> +/* Serial Bus Write */
> +#define STK1160_SBUSW 0x204
> +#define STK1160_SBUSW_WA 0x204
> +#define STK1160_SBUSW_WD 0x205
> +
> +/* Serial Bus Read */
> +#define STK1160_SBUSR 0x208
> +#define STK1160_SBUSR_RA 0x208
> +#define STK1160_SBUSR_RD 0x209
> +
> +/* Alternate Serial Inteface Control */
> +#define STK1160_ASIC 0x2FC
> +
> +/* PLL Select Options */
> +#define STK1160_PLLSO 0x018
> +
> +/* PLL Frequency Divider */
> +#define STK1160_PLLFD 0x01C
> +
> +/* Timing Generator */
> +#define STK1160_TIGEN 0x300
> +
> +/* Timing Control Parameter */
> +#define STK1160_TICTL 0x350
> +
> +/* AC97 Audio Control */
> +#define STK1160_AC97CTL_0 0x500
> +#define STK1160_AC97CTL_1 0x504
> +
> +/* Use [0:6] bits of register 0x504 to set codec command address */
> +#define STK1160_AC97_ADDR 0x504
> +/* Use [16:31] bits of register 0x500 to set codec command data */
> +#define STK1160_AC97_CMD 0x502
> +
> +/* Audio I2S Interface */
> +#define STK1160_I2SCTL 0x50c
> +
> +/* EEPROM Interface */
> +#define STK1160_EEPROM_SZ 0x5f0
...
> +static int vidioc_querybuf(struct file *file, void *priv, struct v4l2_buffer
> *p)
> +{
> + struct stk1160 *dev = video_drvdata(file);
> + int rc;
> +
> + if (!stk1160_is_owner(dev, file))
> + return -EBUSY;
> +
> + rc = vb2_querybuf(&dev->vb_vidq, p);
> +
> + return rc;
What about dropping the local "rc" variable here ?
> +}
> +
> +static int vidioc_qbuf(struct file *file, void *priv, struct v4l2_buffer *p)
> +{
> + struct stk1160 *dev = video_drvdata(file);
> + int rc;
> +
> + if (!stk1160_is_owner(dev, file))
> + return -EBUSY;
> +
> + rc = vb2_qbuf(&dev->vb_vidq, p);
> +
> + return rc;
Ditto.
> +}
> +
> +static int vidioc_dqbuf(struct file *file, void *priv, struct v4l2_buffer *p)
> +{
> + struct stk1160 *dev = video_drvdata(file);
> + int rc;
> +
> + if (!stk1160_is_owner(dev, file))
> + return -EBUSY;
> +
> + rc = vb2_dqbuf(&dev->vb_vidq, p, file->f_flags& O_NONBLOCK);
> +
> + return rc;
Ditto.
> +}
> +
> +static int vidioc_streamon(struct file *file, void *priv, enum v4l2_buf_type
> i)
> +{
> + struct stk1160 *dev = video_drvdata(file);
> + int rc;
> +
> + if (!stk1160_is_owner(dev, file))
> + return -EBUSY;
> +
> + rc = vb2_streamon(&dev->vb_vidq, i);
> +
> + return rc;
Ditto.
> +}
> +
> +static int vidioc_streamoff(struct file *file, void *priv, enum
> v4l2_buf_type i)
> +{
> + struct stk1160 *dev = video_drvdata(file);
> + int rc;
> +
> + if (!stk1160_is_owner(dev, file))
> + return -EBUSY;
> +
> + rc = vb2_streamoff(&dev->vb_vidq, i);
> +
> + return rc;
Ditto.
> +}
> +
> +/*
> + * vidioc ioctls
> + */
...
> +static int vidioc_s_input(struct file *file, void *priv, unsigned int i)
> +{
> + struct stk1160 *dev = video_drvdata(file);
> +
> + if (!stk1160_acquire_owner(dev, file))
> + return -EBUSY;
> +
> + if (i> STK1160_MAX_INPUT)
> + return -EINVAL;
> +
> + dev->ctl_input = i;
> +
> + switch (dev->ctl_input) {
> + case 0:
> + stk1160_write_reg(dev, STK1160_GCTRL, 0x98);
> + break;
> + case 1:
> + stk1160_write_reg(dev, STK1160_GCTRL, 0x90);
> + break;
> + case 2:
> + stk1160_write_reg(dev, STK1160_GCTRL, 0x88);
> + break;
> + case 3:
> + stk1160_write_reg(dev, STK1160_GCTRL, 0x80);
> + break;
> + }
Same block of code as in stk1160_reg_reset(). Probably it's worth
to move it into separate function.
> +
> + return 0;
> +}
> +
...
> +/*
> + * Videobuf2 operations
> + */
> +static int queue_setup(struct vb2_queue *vq, const struct v4l2_format
> *v4l_fmt,
> + unsigned int *nbuffers, unsigned int *nplanes,
> + unsigned int sizes[], void *alloc_ctxs[])
> +{
> + struct stk1160 *dev = vb2_get_drv_priv(vq);
> + unsigned long size;
> +
> + size = dev->width * dev->height * 2;
> +
> + /*
> + * Here we can change the number of buffers being requested.
> + * So, we set a minimum and a maximum like this:
> + */
> + *nbuffers = clamp_t(unsigned int, *nbuffers,
> + STK1160_MIN_VIDEO_BUFFERS, STK1160_MAX_VIDEO_BUFFERS);
> +
> + /* This means a packed colorformat */
> + *nplanes = 1;
> +
> + sizes[0] = size;
> +
> + stk1160_info("%s: buffer count %d, each %ld bytes\n",
> + __func__, *nbuffers, size);
> +
> + return 0;
> +}
> +
> +static void buffer_queue(struct vb2_buffer *vb)
> +{
> + unsigned long flags = 0;
No need to initialize here.
> + struct stk1160 *dev = vb2_get_drv_priv(vb->vb2_queue);
> + struct stk1160_buffer *buf =
> + container_of(vb, struct stk1160_buffer, vb);
> +
> + spin_lock_irqsave(&dev->buf_lock, flags);
> + if (!dev->udev) {
> + /*
> + * If the device is disconnected return the buffer to userspace
> + * directly. The next QBUF call will fail with -ENODEV.
> + */
> + vb2_buffer_done(&buf->vb, VB2_BUF_STATE_ERROR);
> + } else {
> +
> + buf->mem = vb2_plane_vaddr(vb, 0);
> + buf->length = vb2_plane_size(vb, 0);
> + buf->bytesused = 0;
> + buf->pos = 0;
> +
> + /*
> + * If buffer length is different from expected then we return
> + * the buffer to userspace directly.
> + */
> + if (buf->length != dev->width * dev->height * 2)
It should be OK when the buffer size is larger than the amount of data
the device will be writing to it. So it's better to change the
condition statement to:
if (buf->length < dev->width * dev->height * 2)
> + vb2_buffer_done(&buf->vb, VB2_BUF_STATE_ERROR);
> + else
> + list_add_tail(&buf->list,&dev->avail_bufs);
> +
> + }
> + spin_unlock_irqrestore(&dev->buf_lock, flags);
> +}
> +
> +static int start_streaming(struct vb2_queue *vq, unsigned int count)
> +{
> + struct stk1160 *dev = vb2_get_drv_priv(vq);
> + int rc;
Not needed ?
> +
> + rc = stk1160_start_streaming(dev);
> +
> + return rc;
> +}
> +
> +/* abort streaming and wait for last buffer */
> +static int stop_streaming(struct vb2_queue *vq)
> +{
> + struct stk1160 *dev = vb2_get_drv_priv(vq);
> + int rc;
?
> +
> + rc = stk1160_stop_streaming(dev, true);
> +
> + return rc;
> +}
--
Regards,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
