From: Dean Anderson <dean@sensoray.com>

This driver adds support for the Sensoray 2255 devices.

It was primarily developed by Dean Anderson with only a little bit of
guidance and cleanup by Greg.

From: Dean Anderson <dean@sensoray.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
 drivers/media/video/Kconfig    |    9 
 drivers/media/video/Makefile   |    1 
 drivers/media/video/s2255drv.c | 2719 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 2729 insertions(+)

--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -893,6 +893,15 @@ config USB_STKWEBCAM
 	  To compile this driver as a module, choose M here: the
 	  module will be called stkwebcam.
 
+config USB_S2255
+	tristate "USB Sensoray 2255 video capture device"
+	depends on VIDEO_V4L2
+	select VIDEOBUF_VMALLOC
+	default n
+	help
+	  Say Y here if you want support for the Sensoray 2255 USB device.
+	  This driver can be compiled as a module, called s2255drv.
+
 endif # V4L_USB_DRIVERS
 
 config SOC_CAMERA
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -122,6 +122,7 @@ obj-$(CONFIG_USB_IBMCAM)        += usbvi
 obj-$(CONFIG_USB_KONICAWC)      += usbvideo/
 obj-$(CONFIG_USB_VICAM)         += usbvideo/
 obj-$(CONFIG_USB_QUICKCAM_MESSENGER)	+= usbvideo/
+obj-$(CONFIG_USB_S2255)		+= s2255drv.o
 
 obj-$(CONFIG_VIDEO_IVTV) += ivtv/
 obj-$(CONFIG_VIDEO_CX18) += cx18/
--- /dev/null
+++ b/drivers/media/video/s2255drv.c
@@ -0,0 +1,2719 @@
+/*
+ * Sensoray 2255 USB Video for Linux driver
+ *
+ * Copyright (C) 2007-2008 by Sensoray Company Inc.
+ *                            Dean Anderson
+ *
+ *      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, version 2.
+ *
+ * Some video buffer code based on vivi driver:
+ *
+ * TODO: Incorporate videodev2 frame rate(FR) enumeration
+ *       (currently ...
[ message continues ]

Hi Dean, Greg,



Why do you do those conversions in kernelspace?
ffmpeg/libswscale has optimized code for colourspace conversions.
I know a few drivers do that in kernelspace but it's way more flexible
in userspace and depending on the optimization requires less CPU
power.

Markus
--

[ message continues ]

I thought they were there as needed by some V4L1 applications, but that
code was recently removed by Dean I think.  If they don't need to be
there, and userspace apps can properly handle the different colorspace,
then I'll be glad to remove them.

thanks,

greg k-h
--

[ message continues ]

Virtually all apps (V4L1 & 2) can handle YUV and RGB colorspaces.
Certainly all the major ones do and all the major libraries as well.

The problem is when the device only supports some vendor specific or
otherwise very uncommon format.  In that case not doing the conversion in
the kernel means the device won't work with any existing software without
patches.  In this case, while it's not "the right way", drivers often end
up including an in kernel conversion for pragmatic reasons.

This was a problem with the bayer format, but now userspace support for
that format is more common.
--

[ message continues ]

I agree the conversions don't belong in a driver. For the record, the 
following are done in the 2255 hardware: V4L2_PIX_FMT_GREY and 
V4L2_PIX_FMT_YUV422P.

Since planar YUV formats such as V4L2_PIX_FMT_YUV422P are still not that 
well supported, is it possible to keep at least one packed YUV 
format(V4L2_PIX_FMT_YUYV) in the driver?  If not, let me know.  I will 
strongly suggest that the hardware Engineers add YUY2 or YUYV on board 
in the DSP firmware.  Thanks, Dean










--

[ message continues ]

Maybe it's better to fix up the corresponding application? If someone
wants to get those devices work he already either has to upgrade his
system or compile it manually at the moment.
With libswscale it's just a few lines of code to convert the formats
with a decent performance.
Seems like the demand of conversions is also growing for the future.

Markus
--

[ message continues ]

The better is to have YUY2 and YUYV formats at the firmware and/or hardware.
Those are the more common formats, and userspace apps generally try one of
those first. Also, as it is common on several devices, the userspace handlers
are more tested.

Cheers,
Mauro
--

[ message continues ]

Hi,

1. how about a *.h file?

2. You can inline these.

+static int norm_maxw(struct video_device *vdev)
+{
+       return (vdev->current_norm != V4L2_STD_PAL_B) ?
+           LINE_SZ_4CIFS_NTSC : LINE_SZ_4CIFS_PAL;
+}

3. The firmware stuff. That's an interesting solution. However:

a - if you don't need that delay, use a work queue

b - that's mean, use interruptible sleep

+               /* give 1 second for firmware to load in case
+                  driver loaded and then device immediately opened */
+               msleep(1000);

particularly you'd stall khubd processing an early unplug

c - obviously loading the firmware might have failed after waiting

+               if (dev->fw_data->fw_state == FWSTATE_NOTLOADED) {
+                       err("2255 firmware loading stalled\n");
+                       mutex_unlock(&usb_s2255_open_mutex);
+                       return -EAGAIN;
+               }
+       }

you need a check for failure in an else branch

d - so you'll never release firmware in the error case unless you unplug

+       /* if first open after firmware loaded, release the firmware */
+       if (dev->fw_data->fw) {
+               release_firmware(dev->fw_data->fw);
+               dev->fw_data->fw = NULL;
+       }

e - you need to report errors

+static void s2255_timer(unsigned long user_data)
+{
+       struct complete_data *data = (struct complete_data *)user_data;
+       dprintk(100, "s2255 timer\n");
+       if (usb_submit_urb(data->fw_urb, GFP_ATOMIC) < 0) {
+               printk(KERN_ERR "s2255: can't submit urb\n");
+               if (data->fw) {
+                       release_firmware(data->fw);
+                       data->fw = NULL;
+               }
+               return;
+       }
+}

f - also here

+static void s2255_fwchunk_complete(struct urb *urb)
+{
+       struct complete_data *data = urb->context;
+       struct usb_device *udev = urb->dev;
+       int len;
+       dprintk(100, "udev %p urb ...
[ message continues ]

Actually, on second thought, I take that back. It's a bad solution.
If you don't want to do it in probe(), the only other sensible place
is in open(). That way you can avoid the whole trouble if nobody
opens the device. And you need to handle the case of unloaded
firmware anyway, so you can trigger firmware load there.

	Regards
		Oliver

--

[ message continues ]

No, we want to do firmware load on probe, I'll change it to be async so
that probe can continue on.  Need to see if I can disconnect the driver
from device after probe succeeds in case the firmware fails.

thanks for the review, I appreciate it and will fix them up later today
or tomorrow.

greg k-h
--

[ message continues ]

Could you state your reasons for that preference?

	Regards
		Oliver

--

[ message continues ]

I don't want to create the device nodes, and have userspace think the
device really is working, only to have everything fall down and die if
open() is called and the firmware isn't present.  I'd rather not create
the video devices if we know this isn't going to work at all.

Just trying to be nicer to users who would get very confused, "but wait,
the device nodes are there, and the application finds the device, why is
it not working properly?"

thanks,

greg k-h
--

[ message continues ]

Yes, that makes sense. However, then you might want to look into
spawning a thread per bus rather than a delayed failure mechanism.

	Regards
		Oliver


--

[ message continues ]

Hi Greg and Dean,

In general, the driver seems sane and almost ready for their kernel inclusion.
However, there were several videobuf changes affecting mostly the users of
videobuf-vmalloc during the last kernel development cycle, to fix lock issues.
The code should be reviewed at the light of the latest version, and tested. The
good news is that the videobuf handler will be simpler, but a lock is now required.

I have other comments to help improving the driver quality.

Btw, I noticed the lack of Dean's SOB. Is this intentional?

Cheers,
Mauro.

On Wed, 14 May 2008 13:59:27 -0700

It seems a little dangerous to use just DIR_IN/DIR_OUT, since a future change
on a kernel header could cause namespace conflict. IMO, the better is to rename
those macros to something more specific, like S2255_DIR_IN.

The same comment is also true to struct and other namespace definitions, like

Why to reserve 32 integers here? While this may have some sense at userspace

Just those two norms? Are you sure it doesn't support the other european PAL


This seems to be violating CodingStyle. It should be something like:
	{
		.id = foo,
<snip/>
	}, {

The same applies also to other structs. The better is to check the patch with

As already discussed, the driver should support only the video standards

I saw another discussion about this.

I like the idea of loading firmware asynchronously, provided that you block at
open() or at ioctl(), if the user requested to open before having the firmware

This is probably obsolete. We've cleaned up those code. The better is to take a
look at videobuf implementation at em28xx-video, where some lock bugs were fixed and the
code were simplified. I don't have your hardware to test, but I suspect that

Better to use "strlcpy" instead of just "strcpy", to avoid the risk of buffer
overflow. Ok, currently, there's no risk, but, if tomorrow someone changes
something here, the bug will emerge.

Also, please fill bus info, with something ...
[ message continues ]

Concurrent calls to open() will be delayed. Not a problem, if the
firmware issue is fixed as it needs to be.

	Regards
		Oliver

--

[ message continues ]

Hi Oliver,

Just to be clear.  You mean fixing the loading issues(ugly msleep, 
etc...) that Mauro mentioned, not the YUV formats(which seem unrelated)? 

Would it be ok to have just 422P and greyscale in the driver for now if 
we can't get a firmware update in a reasonable timeframe? It's certainly 
not ideal, but we could direct our customers to the libraries, at least 
until we get a proper HW fix.

Best regards,

Dean



--

[ message continues ]

I am incompetent to answer that.

	Regards
		Oliver
--

[ message continues ]

Dean,

On Fri, 16 May 2008 08:57:15 -0700

Feature regression is something that shouldn't happen. If we add the driver
with a color conversion inside, we should somehow keep this feature available
on newer versions. That's said, I don't see any issue if the later versions do
this implementation inside hardware/firmware, since the end result is that the
driver will keep supporting those standards.

From my side, I'm ok on having this color conversion for a short timeframe (one
or two kernel releases), if you are sure that those features will be present
on the next firmwares.

The better is to mark the driver as EXPERIMENTAL, at Kconfig.

Cheers,
Mauro
--

[ message continues ]

>Btw, I noticed the lack of Dean's SOB. Is this intentional?


It's not intentional, I can sign off on it.  The suggestions look good.

I have a few other questions.  First, is Video for Linux version 1 going 
to be obsoleted soon?  Do the V4L1 compatibility routines still work in 
the latest driver?  I had problems running the VIVI (virtual video 
driver) driver with VideoLan/VLC 0.8.6a-f, but it worked with VLC 9.0 
with the new V4L2 interface.

"videodev: "s2255v" has no release callback. Please fix your driver for 
proper sysfs support, see http://lwn.net/Articles/36850/"

Should we get rid of the warning message above? It's also been present 
in VIVI for quite a few kernel releases.



--

[ message continues ]

On Fri, 16 May 2008 07:53:46 -0700




VLC V4L1 implementation were broken. It first starts DMA and streaming, then,
it calls some ioctls that changes the buffer size. The compat handler doesn't
accept this behaviour, since it would cause buffer overflow. AFAIK, only bttv
driver used to support this behaviour. On V4L1 mode, bttv were allocating
enough memory for the maximum resolution. So, subsequent buffer changes works
properly. 

It would be valuable if you could work on a safe way to implement backward
compat for this broken behaviour. In this case, you would need to change the
compat implementation at videobuf, and let v4l1-compat module to be aware that
it is safe to allow buffer size changes.

Yet, this seems to much work for something that should be already removed from

The message doesn't cause any harm, but the better is to fix this also. This
were already corrected at the latest vivi versions.

Cheers,
Mauro
--

[ message continues ]