Re: [PATCH] add Sensoray 2255 v4l driver

[Mauro Carvalho Chehab]

Hi Greg and Dean,

On Thu, 26 Jun 2008 16:15:51 -0700
Greg KH <greg@xxxxxxxxx> wrote:

> All of the previous review comments have been addressed in this version.
> Mauro, can you apply this to your tree if there are no other objections?

Oliver pointed some issues that seems to fix some bugs at the driver, including
one race condition (freeing with timers active). I would add a few other ones. 

Even with those issues, I think we should commit the driver at my tree.
Having the code there will allow more people to test and eventually discover
more issues.

So, if it is ok for you, I'm committing the current patch. I'll be waiting for
patches fixing the pointed issues.

Cheers,
Mauro

> +config USB_S2255
> +	tristate "USB Sensoray 2255 video capture device"
> +	depends on VIDEO_V4L2
> +	select VIDEOBUF_VMALLOC

I think you need also to add:
	select VIDEOBUF_GEN

since select doesn't do recursion, afaik.

> +#define SYS_FRAMES_MAXSIZE	(720*288*2*2 + 4096)

CodingStyle: it should be:

#define SYS_FRAMES_MAXSIZE	(720 * 288 * 2 * 2 + 4096)

> +module_param(debug, int, 0);
> +module_param(vid_limit, int, 0);
> +module_param(video_nr, int, 0);

Why permissions are 0? IMO, it would be better to use 0x644.

> +/* converts 2255 planar format to yuyv or uyvy */
> +static void planar422p_to_yuv_packed(const unsigned char *in,
> +				     unsigned char *out,
> +				     int width, int height,
> +				     int fmt)
> +{
> +	unsigned char *pY;
> +	unsigned char *pCb;
> +	unsigned char *pCr;
> +	unsigned long size = height * width;
> +	unsigned int i;
> +	pY = (unsigned char *)in;
> +	pCr = (unsigned char *)in + height * width;
> +	pCb = (unsigned char *)in + height * width + (height * width / 2);
> +	for (i = 0; i < size * 2; i += 4) {
> +		out[i] = (fmt == V4L2_PIX_FMT_YUYV) ? *pY++ : *pCr++;
> +		out[i + 1] = (fmt == V4L2_PIX_FMT_YUYV) ? *pCr++ : *pY++;
> +		out[i + 2] = (fmt == V4L2_PIX_FMT_YUYV) ? *pY++ : *pCb++;
> +		out[i + 3] = (fmt == V4L2_PIX_FMT_YUYV) ? *pCb++ : *pY++;
> +	}
> +	return;
> +}

Hmm... still keeping format conversion here? Ok for a first version, provided
that you move this to firmware on a near version. Btw, we have an userspace
library to handle such conversions right now. It allows its dynamic usage even
with applications that don't explicitly call the library (even binary-only can
use this). It is available at:
	http://hansdegoede.livejournal.com/3636.html

IMO, the proper way is to remove this conversion (and add at the library if this
conversion is not there yet).

> +static int buffer_setup(struct videobuf_queue *vq, unsigned int *count,
> +			unsigned int *size)
> +{
> +	struct s2255_fh *fh = vq->priv_data;
> +
> +	*size = fh->width * fh->height * (fh->fmt->depth >> 3);
> +
> +	if (0 == *count)
> +		*count = S2255_DEF_BUFS;
> +
> +	while (*size * *count > vid_limit * 1024 * 1024)

CodingStyle: Better to use *count inside parenthesis.

> +static int s2255_open(struct inode *inode, struct file *file)
> +{
	...
> +	dev->users[cur_channel]++;
> +	if (dev->users[cur_channel] > S2255_MAX_USERS) {
> +		dev->users[cur_channel]--;
> +		mutex_unlock(&dev->open_lock);
> +		printk(KERN_INFO "s2255drv: too many open handles!\n");
> +		return -EBUSY;
> +	}

Hmm... V4L2 API allows multiple opens by device. This is, in fact, used
sometimes to allow a better control of the video device, like, for example
using a different  app to change the device controls (for example, qv4l2).

It is preferred to not limit the max users but, instead, to limit the driver to
server more than one stream at a time.

---

I'm not seen any code for suspend/resume. I doubt that the state of the video
will return back from S1/S3 with the current code. IMO, you'll need to
implement explicit handlers for saving the current state and recovering
video/audio streams after returning back from sleep.

A last word is about the usage of locks at the driver. I suspect you would need
to lock on almost all ioctl handlers. However, as we're currently protected by
Kernel big lock, the code is ok. There are some proposed changes to move away
from KBL. When this happen, we may need to review the locking schema.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/