Re: [PATCH 1/1] media: dvb-usb/af9015, fix disconnection crashes

Re: [PATCH 1/1] media: dvb-usb/af9015, fix disconnection crashes

[Jiri Slaby]

[-- Attachment #1: Type: text/plain, Size: 1320 bytes --]

On 01/26/2010 02:08 PM, Jiri Kosina wrote:
>> In my understanding the cause of the remote problem is chipset bug which sets
>> USB2.0 polling interval to 4096ms. Therefore HID remote does not work at all
>> or starts repeating. It is possible to implement remote as polling from the
>> driver which works very well. But HID problem still remains. I have some hacks
>> in my mind to test to kill HID. One is to configure HID wrongly to see if it
>> stops outputting characters. Other way is try to read remote codes directly
>> from the chip memory.
> 
> Yes, Pekka Sarnila has added this workaround to the HID driver, as the 
> device is apparently broken.
> 
> I want to better understand why others are not hitting this with the 
> DVB remote driver before removing the quirk from HID code completely.

I think, we should go for a better way. Thanks Pekka for hints, I ended
up with the patch in the attachment. Could you try it whether it works
for you?

I have 2 dvb-t receivers and both of them need fullspeed quirk. Further
disable_rc_polling (a dvb_usb module parameter) must be set to not get
doubled characters now. And then, it works like a charm.

Note that, it's just some kind of proof of concept. A migration of
af9015 devices from dvb-usb-remote needs to be done first.

Ideas, comments?

regards,
-- 
js

[-- Attachment #2: dvb-remote-fix.patch --]
[-- Type: text/x-patch, Size: 6004 bytes --]

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 139668d..0daf90a 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -122,6 +122,13 @@ config DRAGONRISE_FF
 	Say Y here if you want to enable force feedback support for DragonRise Inc.
 	game controllers.
 
+config HID_DVB
+	tristate "DVB remotes support" if EMBEDDED
+	depends on USB_HID
+	default !EMBEDDED
+	---help---
+	Say Y here if you have DVB remote controllers.
+
 config HID_EZKEY
 	tristate "Ezkey" if EMBEDDED
 	depends on USB_HID
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index b62d4b3..a336b2a 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -30,6 +30,7 @@ obj-$(CONFIG_HID_CHERRY)	+= hid-cherry.o
 obj-$(CONFIG_HID_CHICONY)	+= hid-chicony.o
 obj-$(CONFIG_HID_CYPRESS)	+= hid-cypress.o
 obj-$(CONFIG_HID_DRAGONRISE)	+= hid-drff.o
+obj-$(CONFIG_HID_DVB)		+= hid-dvb.o
 obj-$(CONFIG_HID_EZKEY)		+= hid-ezkey.o
 obj-$(CONFIG_HID_GYRATION)	+= hid-gyration.o
 obj-$(CONFIG_HID_KENSINGTON)	+= hid-kensington.o
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 2dd9b28..678c553 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1252,6 +1252,7 @@ static const struct hid_device_id hid_blacklist[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_3M, USB_DEVICE_ID_3M1968) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_A4TECH, USB_DEVICE_ID_A4TECH_WCP32PU) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_A4TECH, USB_DEVICE_ID_A4TECH_X5_005D) },
+	{ HID_USB_DEVICE(USB_VENDOR_ID_AFATECH, USB_DEVICE_ID_AFATECH_AF9016) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_ATV_IRCONTROL) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IRCONTROL4) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MIGHTYMOUSE) },
@@ -1310,6 +1311,7 @@ static const struct hid_device_id hid_blacklist[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_KENSINGTON, USB_DEVICE_ID_KS_SLIMBLADE) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_KYE, USB_DEVICE_ID_KYE_ERGO_525V) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_LABTEC, USB_DEVICE_ID_LABTEC_WIRELESS_KEYBOARD) },
+	{ HID_USB_DEVICE(USB_VENDOR_ID_LEADTEK, USB_DEVICE_ID_DTV_GOLD) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_MX3000_RECEIVER) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_S510_RECEIVER) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_S510_RECEIVER_2) },
diff --git a/drivers/hid/hid-dvb.c b/drivers/hid/hid-dvb.c
new file mode 100644
index 0000000..ee94c07
--- /dev/null
+++ b/drivers/hid/hid-dvb.c
@@ -0,0 +1,78 @@
+/*
+ *  HID driver for dvb devices
+ *
+ *  Copyright (c) 2010 Jiri Slaby
+ *
+ *  Licensed under the GPLv2.
+ */
+
+#include <linux/device.h>
+#include <linux/hid.h>
+#include <linux/module.h>
+
+#include "hid-ids.h"
+
+#define FULLSPEED_INTERVAL	0x1
+
+static int dvb_event(struct hid_device *hdev, struct hid_field *field,
+		struct hid_usage *usage, __s32 value)
+{
+	/* we won't get a "key up" event */
+	if (value) {
+		input_event(field->hidinput->input, usage->type, usage->code, 1);
+		input_event(field->hidinput->input, usage->type, usage->code, 0);
+	}
+	return 1;
+}
+
+static int dvb_probe(struct hid_device *hdev, const struct hid_device_id *id)
+{
+	unsigned long quirks = id->driver_data;
+	int ret;
+
+	if (quirks & FULLSPEED_INTERVAL)
+		hdev->quirks |= HID_QUIRK_FULLSPEED_INTERVAL;
+
+	ret = hid_parse(hdev);
+	if (ret) {
+		dev_err(&hdev->dev, "parse failed\n");
+		goto end;
+	}
+
+	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
+	if (ret)
+		dev_err(&hdev->dev, "hw start failed\n");
+end:
+	return ret;
+}
+
+static const struct hid_device_id dvb_devices[] = {
+	{ HID_USB_DEVICE(USB_VENDOR_ID_AFATECH, USB_DEVICE_ID_AFATECH_AF9016),
+		.driver_data = FULLSPEED_INTERVAL },
+	{ HID_USB_DEVICE(USB_VENDOR_ID_LEADTEK, USB_DEVICE_ID_DTV_GOLD),
+		.driver_data = FULLSPEED_INTERVAL },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(hid, dvb_devices);
+
+static struct hid_driver dvb_driver = {
+	.name = "dvb",
+	.id_table = dvb_devices,
+	.probe = dvb_probe,
+	.event = dvb_event,
+};
+
+static int __init dvb_init(void)
+{
+	return hid_register_driver(&dvb_driver);
+}
+
+static void __exit dvb_exit(void)
+{
+	hid_unregister_driver(&dvb_driver);
+}
+
+module_init(dvb_init);
+module_exit(dvb_exit);
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 39ff98a..7a6495f 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -18,6 +18,9 @@
 #ifndef HID_IDS_H_FILE
 #define HID_IDS_H_FILE
 
+#define USB_VENDOR_ID_LEADTEK		0x0413
+#define USB_DEVICE_ID_DTV_GOLD		0x6029
+
 #define USB_VENDOR_ID_3M		0x0596
 #define USB_DEVICE_ID_3M1968		0x0500
 
diff --git a/drivers/hid/usbhid/hid-quirks.c b/drivers/hid/usbhid/hid-quirks.c
index 88a1c69..74aac20 100644
--- a/drivers/hid/usbhid/hid-quirks.c
+++ b/drivers/hid/usbhid/hid-quirks.c
@@ -41,8 +41,6 @@ static const struct hid_blacklist {
 	{ USB_VENDOR_ID_SAITEK, USB_DEVICE_ID_SAITEK_RUMBLEPAD, HID_QUIRK_BADPAD },
 	{ USB_VENDOR_ID_TOPMAX, USB_DEVICE_ID_TOPMAX_COBRAPAD, HID_QUIRK_BADPAD },
 
-	{ USB_VENDOR_ID_AFATECH, USB_DEVICE_ID_AFATECH_AF9016, HID_QUIRK_FULLSPEED_INTERVAL },
-
 	{ USB_VENDOR_ID_ETURBOTOUCH, USB_DEVICE_ID_ETURBOTOUCH, HID_QUIRK_MULTI_INPUT },
 	{ USB_VENDOR_ID_PANTHERLORD, USB_DEVICE_ID_PANTHERLORD_TWIN_USB_JOYSTICK, HID_QUIRK_MULTI_INPUT | HID_QUIRK_SKIP_OUTPUT_REPORTS },
 	{ USB_VENDOR_ID_PLAYDOTCOM, USB_DEVICE_ID_PLAYDOTCOM_EMS_USBII, HID_QUIRK_MULTI_INPUT },
diff --git a/drivers/media/dvb/dvb-usb/af9015.c b/drivers/media/dvb/dvb-usb/af9015.c
index 650c913..239c2d0 100644
--- a/drivers/media/dvb/dvb-usb/af9015.c
+++ b/drivers/media/dvb/dvb-usb/af9015.c
@@ -1613,7 +1613,10 @@ static int af9015_usb_probe(struct usb_interface *intf,
 
 	/* interface 0 is used by DVB-T receiver and
 	   interface 1 is for remote controller (HID) */
-	if (intf->cur_altsetting->desc.bInterfaceNumber == 0) {
+	if (intf->cur_altsetting->desc.bInterfaceNumber != 0)
+		return -ENODEV;
+
+	{
 		ret = af9015_read_config(udev);
 		if (ret)
 			return ret;

Re: [PATCH 1/1] media: dvb-usb/af9015, fix disconnection crashes

[Mauro Carvalho Chehab]

Jiri Slaby wrote:
> On 01/26/2010 02:08 PM, Jiri Kosina wrote:
>>> In my understanding the cause of the remote problem is chipset bug which sets
>>> USB2.0 polling interval to 4096ms. Therefore HID remote does not work at all
>>> or starts repeating. It is possible to implement remote as polling from the
>>> driver which works very well. But HID problem still remains. I have some hacks
>>> in my mind to test to kill HID. One is to configure HID wrongly to see if it
>>> stops outputting characters. Other way is try to read remote codes directly
>>> from the chip memory.
>> Yes, Pekka Sarnila has added this workaround to the HID driver, as the 
>> device is apparently broken.
>>
>> I want to better understand why others are not hitting this with the 
>> DVB remote driver before removing the quirk from HID code completely.
> 
> I think, we should go for a better way. Thanks Pekka for hints, I ended
> up with the patch in the attachment. Could you try it whether it works
> for you?
> 
> I have 2 dvb-t receivers and both of them need fullspeed quirk. Further
> disable_rc_polling (a dvb_usb module parameter) must be set to not get
> doubled characters now. And then, it works like a charm.

Module parameters always bothers me. They should be used as last resort alternatives
when there's no other possible way to make it work properly.

If we know for sure that the RC polling should be disabled by an specific device, 
just add this logic at the driver.

> Note that, it's just some kind of proof of concept. A migration of
> af9015 devices from dvb-usb-remote needs to be done first.
> 
> Ideas, comments?

Please next time, send the patch inlined. As you're using Thunderbird, you'll likely need
Asalted-patches[1] to avoid thunderbird to destroy your patches.

[1]https://hg.mozilla.org/users/clarkbw_gnome.org/asalted-patches/


 +config HID_DVB
+	tristate "DVB remotes support" if EMBEDDED
+	depends on USB_HID
+	default !EMBEDDED
+	---help---
+	Say Y here if you have DVB remote controllers.
+

I think the better would be to use a more generic name, like HID_RC (for Remote Controller).
I suspect we may need in the future other hacks for other similar devices.

+static int dvb_event(struct hid_device *hdev, struct hid_field *field,
+		struct hid_usage *usage, __s32 value)
+{
+	/* we won't get a "key up" event */
+	if (value) {
+		input_event(field->hidinput->input, usage->type, usage->code, 1);
+		input_event(field->hidinput->input, usage->type, usage->code, 0);
+	}
+	return 1;
+}

Several V4L/DVB IR's have keyup/keydown events. So I think the name here is also wrong:
it is better to name the function as dvb_nokeyup_event() and eventually add an specific
quirk to indicate devices that only have key up events.

-- 

Cheers,
Mauro

Re: [PATCH 1/1] media: dvb-usb/af9015, fix disconnection crashes

[Jiri Slaby]

On 02/04/2010 01:04 PM, Mauro Carvalho Chehab wrote:
>> I have 2 dvb-t receivers and both of them need fullspeed quirk. Further
>> disable_rc_polling (a dvb_usb module parameter) must be set to not get
>> doubled characters now. And then, it works like a charm.
> 
> Module parameters always bothers me. They should be used as last resort alternatives
> when there's no other possible way to make it work properly.
> 
> If we know for sure that the RC polling should be disabled by an specific device, 
> just add this logic at the driver.

Yes, this is planned and written below:

>> Note that, it's just some kind of proof of concept. A migration of
>> af9015 devices from dvb-usb-remote needs to be done first.
>>
>> Ideas, comments?
> 
> Please next time, send the patch inlined. As you're using Thunderbird, you'll likely need
> Asalted-patches[1] to avoid thunderbird to destroy your patches.

I must disagree for two reasons: (a) it was not patch intended for merge
and (b) it was a plain-text attachment which is fine even for
submission. However I don't like patches as attachments so if I decide
to submit it for a merge later, you will not see it as an attachment
then :).

> +config HID_DVB
> +	tristate "DVB remotes support" if EMBEDDED
> +	depends on USB_HID
> +	default !EMBEDDED
> +	---help---
> +	Say Y here if you have DVB remote controllers.
> +
> 
> I think the better would be to use a more generic name, like HID_RC (for Remote Controller).
> I suspect we may need in the future other hacks for other similar devices.

Seconded. I would only go for some other abbreviation other than RC or
not abbreviate that at all.

> +static int dvb_event(struct hid_device *hdev, struct hid_field *field,
> +		struct hid_usage *usage, __s32 value)
> +{
> +	/* we won't get a "key up" event */
> +	if (value) {
> +		input_event(field->hidinput->input, usage->type, usage->code, 1);
> +		input_event(field->hidinput->input, usage->type, usage->code, 0);
> +	}
> +	return 1;
> +}
> 
> Several V4L/DVB IR's have keyup/keydown events. So I think the name here is also wrong:
> it is better to name the function as dvb_nokeyup_event() and eventually add an specific
> quirk to indicate devices that only have key up events.

If such appear later, it can be rewritten. I don't plan to add such
functionality now until somebody comes with device IDs which should be
handled that way and tests it, because I guess I will definitely do it
wrong otherwise. Do you know/have such a device?

There are many of quirks needed for various devices. I already wrote
about af9005 which sends key repeat aside from key down etc. But the
same as above, I can't test it (and don't want to introduce
regressions). So again, if somebody can test it, I'll be happy to code it.

thanks for the input,
-- 
js

Re: [PATCH 1/1] media: dvb-usb/af9015, fix disconnection crashes

[Pekka Sarnila]

Well the thing is that this device (afatech) offers two different ways 
to read the remote: vendor class bulk endpoint (0/81, used afatech 
driver) and HID class interrupt endpoint (1/83, seen as an ordinary usb 
keyboard by the usb/hid system). When I used unmodified kernel (with new 
firmware) both of these were used simultaneously. When added afatech to 
hid blacklist hid of course did not work any more, so only the bulk 
endpoint was used, and things worked trough afatech driver, nor was the 
HID FULLSPEED QUIRK needed.

The problem using vendor class is that there is no standard. Each vendor 
can define its own way using endpoints (and has done so e.g Logitech 
joysticks). Thus each usb ir receiver must have its own specific driver. 
  However then you get the raw ir codes. When using HID class, generic 
HID driver can do the job. But then you get HID key codes directly not 
ir codes.

Also this should not be seen at all as dvb question. First, not all the 
world uses dvb standard (including USA) but uses very similar tv-sticks 
with identical ir receivers and remotes. Second there are many other 
type of usb devices with ir receiver. So dvb layer should not be 
involved at all. There maybe would be need for hid-ir-remote layer (your 
code suggestion moved there). However even there IMHO better would be 
just to improve HID <-> input layer interface so that input layer could 
divert the remotes input to generic remotes layer instead of keyboard 
layer. IMHO this would be the real linux way.

The FULLSPEED thing is really not ir receiver specific problem. It can 
happen with any device with interrupt endpoint. That's the reason why I 
placed the quirk to HID driver.

However even the HID driver is logically a wrong place. Really it should 
belong to the usb/endpoint layer because this really is not HID specific 
problem but usb layer problem (as also Jiri Kosina then pointed out). 
However linux usb layer has been build so that it was technically 
impossible to put it there without completely redesigning usb <-> higher 
layer (including HID) interface. But I'm of the opinion that that 
redesign should be done anyway. Even when no quirk is needed interrupt 
endpoint handling is a usb level task not a hid level (or any other 
higher level) task. The usb layer should do the interrupt endpoint 
polling and just put up interrupt events to higher layers. Partly this 
confusion is due the poor usb/hid specifications. It really should be a 
device/endpoint-quirk.

Now the question is, how much all usb based ir receivers have in common, 
and how much they differ. This should determine on what level and in 
which way they should be handled. How much and on what level there 
should be common code and where device specific driver code would be 
needed and where and how the ir-to-code translate should take place.

IMHO all that have HID endpoint that works (either as such or with some 
generic quirk) should be handled by HID first and then conveyed to 
generic remotes layer that handles all remote controllers what ever the 
lower layers (and does translate per remote not per ir receiver). Vendor 
class should be avoided unless that's the only way to make it work 
right. But using HID is not without problems either.

Now with the two receivers that need the quirk. If they don't have 
vendor class bulk endpoint for reading ir codes, then specific driver is 
out anyway. However then changes to HID driver would be needed to make 
the translate work. The quirk just makes them work as generic usb 
keyboard with no remote specific translate. With afatech, driver loads 
the translate table to the device, different for different remotes. I 
don't know if one table could handle them all. Maybe this table should 
be loaded from a user space file. Nor do I know how it is with other 
receivers: can the table be loaded or is it fixed. In any case a generic 
secondary per remote user configurable translate would be highly 
desirable. And I don't mean lircd. This job is IMHO kernel level job and 
lircd won't work here anyway. It does ir code to key code or function 
translate not key code to key code translate. How nice it would be if 
there would be a generic usb ir receiver class that all vendors used. 
There seems to be no way to  make this well and clean.

Well it got a bit long, but the problem is not simple either, and it 
cuts trough many parts (and maintainers) of the kernel.

So what to do?

Pekka

Jiri Slaby wrote:
> On 01/26/2010 02:08 PM, Jiri Kosina wrote:
> 
>>>In my understanding the cause of the remote problem is chipset bug which sets
>>>USB2.0 polling interval to 4096ms. Therefore HID remote does not work at all
>>>or starts repeating. It is possible to implement remote as polling from the
>>>driver which works very well. But HID problem still remains. I have some hacks
>>>in my mind to test to kill HID. One is to configure HID wrongly to see if it
>>>stops outputting characters. Other way is try to read remote codes directly
>>>from the chip memory.
>>
>>Yes, Pekka Sarnila has added this workaround to the HID driver, as the 
>>device is apparently broken.
>>
>>I want to better understand why others are not hitting this with the 
>>DVB remote driver before removing the quirk from HID code completely.
> 
> 
> I think, we should go for a better way. Thanks Pekka for hints, I ended
> up with the patch in the attachment. Could you try it whether it works
> for you?
> 
> I have 2 dvb-t receivers and both of them need fullspeed quirk. Further
> disable_rc_polling (a dvb_usb module parameter) must be set to not get
> doubled characters now. And then, it works like a charm.
> 
> Note that, it's just some kind of proof of concept. A migration of
> af9015 devices from dvb-usb-remote needs to be done first.
> 
> Ideas, comments?
> 
> regards,
> 
> 
> ------------------------------------------------------------------------
> 
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 139668d..0daf90a 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -122,6 +122,13 @@ config DRAGONRISE_FF
>  	Say Y here if you want to enable force feedback support for DragonRise Inc.
>  	game controllers.
>  
> +config HID_DVB
> +	tristate "DVB remotes support" if EMBEDDED
> +	depends on USB_HID
> +	default !EMBEDDED
> +	---help---
> +	Say Y here if you have DVB remote controllers.
> +
>  config HID_EZKEY
>  	tristate "Ezkey" if EMBEDDED
>  	depends on USB_HID
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index b62d4b3..a336b2a 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -30,6 +30,7 @@ obj-$(CONFIG_HID_CHERRY)	+= hid-cherry.o
>  obj-$(CONFIG_HID_CHICONY)	+= hid-chicony.o
>  obj-$(CONFIG_HID_CYPRESS)	+= hid-cypress.o
>  obj-$(CONFIG_HID_DRAGONRISE)	+= hid-drff.o
> +obj-$(CONFIG_HID_DVB)		+= hid-dvb.o
>  obj-$(CONFIG_HID_EZKEY)		+= hid-ezkey.o
>  obj-$(CONFIG_HID_GYRATION)	+= hid-gyration.o
>  obj-$(CONFIG_HID_KENSINGTON)	+= hid-kensington.o
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 2dd9b28..678c553 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1252,6 +1252,7 @@ static const struct hid_device_id hid_blacklist[] = {
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_3M, USB_DEVICE_ID_3M1968) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_A4TECH, USB_DEVICE_ID_A4TECH_WCP32PU) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_A4TECH, USB_DEVICE_ID_A4TECH_X5_005D) },
> +	{ HID_USB_DEVICE(USB_VENDOR_ID_AFATECH, USB_DEVICE_ID_AFATECH_AF9016) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_ATV_IRCONTROL) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IRCONTROL4) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MIGHTYMOUSE) },
> @@ -1310,6 +1311,7 @@ static const struct hid_device_id hid_blacklist[] = {
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_KENSINGTON, USB_DEVICE_ID_KS_SLIMBLADE) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_KYE, USB_DEVICE_ID_KYE_ERGO_525V) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_LABTEC, USB_DEVICE_ID_LABTEC_WIRELESS_KEYBOARD) },
> +	{ HID_USB_DEVICE(USB_VENDOR_ID_LEADTEK, USB_DEVICE_ID_DTV_GOLD) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_MX3000_RECEIVER) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_S510_RECEIVER) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_S510_RECEIVER_2) },
> diff --git a/drivers/hid/hid-dvb.c b/drivers/hid/hid-dvb.c
> new file mode 100644
> index 0000000..ee94c07
> --- /dev/null
> +++ b/drivers/hid/hid-dvb.c
> @@ -0,0 +1,78 @@
> +/*
> + *  HID driver for dvb devices
> + *
> + *  Copyright (c) 2010 Jiri Slaby
> + *
> + *  Licensed under the GPLv2.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/hid.h>
> +#include <linux/module.h>
> +
> +#include "hid-ids.h"
> +
> +#define FULLSPEED_INTERVAL	0x1
> +
> +static int dvb_event(struct hid_device *hdev, struct hid_field *field,
> +		struct hid_usage *usage, __s32 value)
> +{
> +	/* we won't get a "key up" event */
> +	if (value) {
> +		input_event(field->hidinput->input, usage->type, usage->code, 1);
> +		input_event(field->hidinput->input, usage->type, usage->code, 0);
> +	}
> +	return 1;
> +}
> +
> +static int dvb_probe(struct hid_device *hdev, const struct hid_device_id *id)
> +{
> +	unsigned long quirks = id->driver_data;
> +	int ret;
> +
> +	if (quirks & FULLSPEED_INTERVAL)
> +		hdev->quirks |= HID_QUIRK_FULLSPEED_INTERVAL;
> +
> +	ret = hid_parse(hdev);
> +	if (ret) {
> +		dev_err(&hdev->dev, "parse failed\n");
> +		goto end;
> +	}
> +
> +	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> +	if (ret)
> +		dev_err(&hdev->dev, "hw start failed\n");
> +end:
> +	return ret;
> +}
> +
> +static const struct hid_device_id dvb_devices[] = {
> +	{ HID_USB_DEVICE(USB_VENDOR_ID_AFATECH, USB_DEVICE_ID_AFATECH_AF9016),
> +		.driver_data = FULLSPEED_INTERVAL },
> +	{ HID_USB_DEVICE(USB_VENDOR_ID_LEADTEK, USB_DEVICE_ID_DTV_GOLD),
> +		.driver_data = FULLSPEED_INTERVAL },
> +	{ }
> +};
> +
> +MODULE_DEVICE_TABLE(hid, dvb_devices);
> +
> +static struct hid_driver dvb_driver = {
> +	.name = "dvb",
> +	.id_table = dvb_devices,
> +	.probe = dvb_probe,
> +	.event = dvb_event,
> +};
> +
> +static int __init dvb_init(void)
> +{
> +	return hid_register_driver(&dvb_driver);
> +}
> +
> +static void __exit dvb_exit(void)
> +{
> +	hid_unregister_driver(&dvb_driver);
> +}
> +
> +module_init(dvb_init);
> +module_exit(dvb_exit);
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 39ff98a..7a6495f 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -18,6 +18,9 @@
>  #ifndef HID_IDS_H_FILE
>  #define HID_IDS_H_FILE
>  
> +#define USB_VENDOR_ID_LEADTEK		0x0413
> +#define USB_DEVICE_ID_DTV_GOLD		0x6029
> +
>  #define USB_VENDOR_ID_3M		0x0596
>  #define USB_DEVICE_ID_3M1968		0x0500
>  
> diff --git a/drivers/hid/usbhid/hid-quirks.c b/drivers/hid/usbhid/hid-quirks.c
> index 88a1c69..74aac20 100644
> --- a/drivers/hid/usbhid/hid-quirks.c
> +++ b/drivers/hid/usbhid/hid-quirks.c
> @@ -41,8 +41,6 @@ static const struct hid_blacklist {
>  	{ USB_VENDOR_ID_SAITEK, USB_DEVICE_ID_SAITEK_RUMBLEPAD, HID_QUIRK_BADPAD },
>  	{ USB_VENDOR_ID_TOPMAX, USB_DEVICE_ID_TOPMAX_COBRAPAD, HID_QUIRK_BADPAD },
>  
> -	{ USB_VENDOR_ID_AFATECH, USB_DEVICE_ID_AFATECH_AF9016, HID_QUIRK_FULLSPEED_INTERVAL },
> -
>  	{ USB_VENDOR_ID_ETURBOTOUCH, USB_DEVICE_ID_ETURBOTOUCH, HID_QUIRK_MULTI_INPUT },
>  	{ USB_VENDOR_ID_PANTHERLORD, USB_DEVICE_ID_PANTHERLORD_TWIN_USB_JOYSTICK, HID_QUIRK_MULTI_INPUT | HID_QUIRK_SKIP_OUTPUT_REPORTS },
>  	{ USB_VENDOR_ID_PLAYDOTCOM, USB_DEVICE_ID_PLAYDOTCOM_EMS_USBII, HID_QUIRK_MULTI_INPUT },
> diff --git a/drivers/media/dvb/dvb-usb/af9015.c b/drivers/media/dvb/dvb-usb/af9015.c
> index 650c913..239c2d0 100644
> --- a/drivers/media/dvb/dvb-usb/af9015.c
> +++ b/drivers/media/dvb/dvb-usb/af9015.c
> @@ -1613,7 +1613,10 @@ static int af9015_usb_probe(struct usb_interface *intf,
>  
>  	/* interface 0 is used by DVB-T receiver and
>  	   interface 1 is for remote controller (HID) */
> -	if (intf->cur_altsetting->desc.bInterfaceNumber == 0) {
> +	if (intf->cur_altsetting->desc.bInterfaceNumber != 0)
> +		return -ENODEV;
> +
> +	{
>  		ret = af9015_read_config(udev);
>  		if (ret)
>  			return ret;

Re: [PATCH 1/1] media: dvb-usb/af9015, fix disconnection crashes

[Jiri Kosina]

On Thu, 4 Feb 2010, Pekka Sarnila wrote:

> The FULLSPEED thing is really not ir receiver specific problem. It can 
> happen with any device with interrupt endpoint. That's the reason why I 
> placed the quirk to HID driver.
> 
> However even the HID driver is logically a wrong place. Really it should 
> belong to the usb/endpoint layer because this really is not HID specific 
> problem but usb layer problem (as also Jiri Kosina then pointed out). 
> However linux usb layer has been build so that it was technically 
> impossible to put it there without completely redesigning usb <-> higher 
> layer (including HID) interface. But I'm of the opinion that that 
> redesign should be done anyway. Even when no quirk is needed interrupt 
> endpoint handling is a usb level task not a hid level (or any other 
> higher level) task. The usb layer should do the interrupt endpoint 
> polling and just put up interrupt events to higher layers. Partly this 
> confusion is due the poor usb/hid specifications. It really should be a 
> device/endpoint-quirk.

Yes, I still think what I have stated before, that this should be properly 
handled in the USB stack.

On the other hand, in usbhid driver we do a lot of USB-specific 
lower-level things anyway, so it's technically more-or-less OK to apply 
the quirk there as well (and that's why I have accepted it back then).

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: [PATCH 1/1] media: dvb-usb/af9015, fix disconnection crashes

[Mauro Carvalho Chehab]

Jiri Slaby wrote:
> On 02/04/2010 01:04 PM, Mauro Carvalho Chehab wrote:
>>> I have 2 dvb-t receivers and both of them need fullspeed quirk. Further
>>> disable_rc_polling (a dvb_usb module parameter) must be set to not get
>>> doubled characters now. And then, it works like a charm.
>> Module parameters always bothers me. They should be used as last resort alternatives
>> when there's no other possible way to make it work properly.
>>
>> If we know for sure that the RC polling should be disabled by an specific device, 
>> just add this logic at the driver.
> 
> Yes, this is planned and written below:

Ok.
> 
>>> Note that, it's just some kind of proof of concept. A migration of
>>> af9015 devices from dvb-usb-remote needs to be done first.
>>>
>>> Ideas, comments?
>> Please next time, send the patch inlined. As you're using Thunderbird, you'll likely need
>> Asalted-patches[1] to avoid thunderbird to destroy your patches.
> 
> I must disagree for two reasons: (a) it was not patch intended for merge
> and (b) it was a plain-text attachment which is fine even for
> submission. However I don't like patches as attachments so if I decide
> to submit it for a merge later, you will not see it as an attachment
> then :).

Attachments aren't good for reply, as they appear as a file. So, people need to
open the attachment on a separate application to see and to cut-and-paste
if they want to comment, like what I did.

>> +config HID_DVB
>> +	tristate "DVB remotes support" if EMBEDDED
>> +	depends on USB_HID
>> +	default !EMBEDDED
>> +	---help---
>> +	Say Y here if you have DVB remote controllers.
>> +
>>
>> I think the better would be to use a more generic name, like HID_RC (for Remote Controller).
>> I suspect we may need in the future other hacks for other similar devices.
> 
> Seconded. I would only go for some other abbreviation other than RC or
> not abbreviate that at all.

we're using irrcv at the IR remote class. So, this can be another name for it.

>> +static int dvb_event(struct hid_device *hdev, struct hid_field *field,
>> +		struct hid_usage *usage, __s32 value)
>> +{
>> +	/* we won't get a "key up" event */
>> +	if (value) {
>> +		input_event(field->hidinput->input, usage->type, usage->code, 1);
>> +		input_event(field->hidinput->input, usage->type, usage->code, 0);
>> +	}
>> +	return 1;
>> +}
>>
>> Several V4L/DVB IR's have keyup/keydown events. So I think the name here is also wrong:
>> it is better to name the function as dvb_nokeyup_event() and eventually add an specific
>> quirk to indicate devices that only have key up events.
> 
> If such appear later, it can be rewritten. I don't plan to add such
> functionality now until somebody comes with device IDs which should be
> handled that way and tests it, because I guess I will definitely do it
> wrong otherwise. 
> Do you know/have such a device?

I have several devices here with different ways to generate keyup/keydown or
keydown/repeat, but they don't export a standard usb HID interface (in a matter
of fact, I have just one PCI device that came with a separate USB HID interface,
but this device always worked fine - also - as it has physically a separate device -
it doesn't generate any conflict with the DVB hardware).

The point is that it is better to name the function right since the beginning.
 
> There are many of quirks needed for various devices. I already wrote
> about af9005 which sends key repeat aside from key down etc. But the
> same as above, I can't test it (and don't want to introduce
> regressions). So again, if somebody can test it, I'll be happy to code it.
> 
> thanks for the input,


-- 

Cheers,
Mauro

Re: [PATCH 1/1] media: dvb-usb/af9015, fix disconnection crashes

[Jiri Slaby]

On 02/04/2010 02:41 PM, Mauro Carvalho Chehab wrote:
> The point is that it is better to name the function right since the beginning.

Sorry, I misunderstood you for the first time. It's .event member of
hid_driver. Hence I named it dvb_event (or now rc_event or whatever).

The function may contain decisions on what to do with the event based
for example on quirks set up in .probe. And if the function grows later,
it may be factored out to rc_nokeyup_event. But rc_event is a root for
the decision tree and it should be there forever. Does it make sense now?

-- 
js

Re: [PATCH 1/1] media: dvb-usb/af9015, fix disconnection crashes

[Mauro Carvalho Chehab]

Pekka Sarnila wrote:

> The problem using vendor class is that there is no standard. Each vendor
> can define its own way using endpoints (and has done so e.g Logitech
> joysticks). Thus each usb ir receiver must have its own specific driver.
>  However then you get the raw ir codes. When using HID class, generic
> HID driver can do the job. But then you get HID key codes directly not
> ir codes.

We started writing an abstraction layer for IR, using the input. The idea
is to allow the IR receivers to work with different IR's, as several users
prefer to use universal IR's to control their devices, instead of the original
one. This is already used by all V4L drivers and I intend to port most of
the DVB drivers to use it as well soon.

> Also this should not be seen at all as dvb question. First, not all the
> world uses dvb standard (including USA) but uses very similar tv-sticks
> with identical ir receivers and remotes. 

Despiste its name, the DVB subsystem is not specific for DVB standards, but
it is meant to be used by all DTV standards (and almost all DTV standards
are already supported).

> Second there are many other
> type of usb devices with ir receiver. So dvb layer should not be
> involved at all. There maybe would be need for hid-ir-remote layer (your
> code suggestion moved there). However even there IMHO better would be
> just to improve HID <-> input layer interface so that input layer could
> divert the remotes input to generic remotes layer instead of keyboard
> layer. IMHO this would be the real linux way.

This is already happening.

See drivers/media/IR on linux-next for the IR common code. The ir-core is
provided by ir-keytable.c and ir-sysfs.c, and it is not DVB or V4L specific.

The ir-common module were developed for V4L drivers and will probably be
changed into a more generic way, with the integration with lirc.

> However linux usb layer has been build so that it was technically
> impossible to put it there without completely redesigning usb <-> higher
> layer (including HID) interface. But I'm of the opinion that that
> redesign should be done anyway. 

Feel free to submit patches. My plan is to integrate the DVB devices soon
into the new ir-core. I should start with dvb-usb-remote, where most of the
DVB USB devices use to attach their IR's. Unfortunately, af9015 doesn't 
rely on the dvb-usb-remote, so, it will require some specific changes.
As I don't have any af9015 device, I'll likely postpone it or wait for
someone to do the job.

> Now the question is, how much all usb based ir receivers have in common,
> and how much they differ. This should determine on what level and in
> which way they should be handled. How much and on what level there
> should be common code and where device specific driver code would be
> needed and where and how the ir-to-code translate should take place.

There are several different ways for IR receivers, and, at least for 
vendor class, no standard way to handle. They can use GPIO polling,
they can use i2c layer, they can use IRQ (on PCI devices) and the data
may be provided in parallel or on a serial interface.

The idea of the ir-core is to provide support for all those options.

> IMHO all that have HID endpoint that works (either as such or with some
> generic quirk) should be handled by HID first and then conveyed to
> generic remotes layer that handles all remote controllers what ever the
> lower layers (and does translate per remote not per ir receiver). Vendor
> class should be avoided unless that's the only way to make it work
> right. But using HID is not without problems either.

Almost all chipsets only provide IR via vendor class. I agree that using 
standard HID class is the better way for doing it.

> Now with the two receivers that need the quirk. If they don't have
> vendor class bulk endpoint for reading ir codes, then specific driver is
> out anyway. However then changes to HID driver would be needed to make
> the translate work. The quirk just makes them work as generic usb
> keyboard with no remote specific translate. With afatech, driver loads
> the translate table to the device, different for different remotes. I
> don't know if one table could handle them all. Maybe this table should
> be loaded from a user space file. Nor do I know how it is with other
> receivers: can the table be loaded or is it fixed. In any case a generic
> secondary per remote user configurable translate would be highly
> desirable. And I don't mean lircd. This job is IMHO kernel level job and
> lircd won't work here anyway. It does ir code to key code or function
> translate not key code to key code translate. How nice it would be if
> there would be a generic usb ir receiver class that all vendors used.
> There seems to be no way to  make this well and clean.

The ir-core provides standard ways to replace the IR keycode and IR protocols.
The IR protocol change is already working with vendor class with em28xx driver.

Cheers,
Mauro

Re: [PATCH 1/1] media: dvb-usb/af9015, fix disconnection crashes

[Mauro Carvalho Chehab]

Jiri Slaby wrote:
> On 02/04/2010 02:41 PM, Mauro Carvalho Chehab wrote:
>> The point is that it is better to name the function right since the beginning.
> 
> Sorry, I misunderstood you for the first time. It's .event member of
> hid_driver. Hence I named it dvb_event (or now rc_event or whatever).
> 
> The function may contain decisions on what to do with the event based
> for example on quirks set up in .probe. And if the function grows later,
> it may be factored out to rc_nokeyup_event. But rc_event is a root for
> the decision tree and it should be there forever. Does it make sense now?

Ah, ok. Due to the comments at the function, I misunderstood that you were
planning to have separate functions for separate quirks.


-- 

Cheers,
Mauro

Re: [PATCH 1/1] media: dvb-usb/af9015, fix disconnection crashes

[Pekka Sarnila]

Yes, my comment maybe criticizes more the basic architectural structure 
of usb putting it's own work up to higher layer. The only practical 
thing is that, if there is a non-HID device suffering from that 
FULLSPEED problem, the quirk won't help it. Anyway in current kernel 
structure usb layer doesn't handle endpoint setup at all, thus it simply 
can not do the job.

Pekka

Jiri Kosina wrote:
> 
> Yes, I still think what I have stated before, that this should be properly 
> handled in the USB stack.
> 
> On the other hand, in usbhid driver we do a lot of USB-specific 
> lower-level things anyway, so it's technically more-or-less OK to apply 
> the quirk there as well (and that's why I have accepted it back then).
> 

Re: [PATCH 1/1] media: dvb-usb/af9015, fix disconnection crashes

[Mauro Carvalho Chehab]

Jarod Wilson wrote:
> On 02/04/2010 08:41 AM, Mauro Carvalho Chehab wrote:
>> Jiri Slaby wrote:
>>> On 02/04/2010 01:04 PM, Mauro Carvalho Chehab wrote:
>>>>> I have 2 dvb-t receivers and both of them need fullspeed quirk.
>>>>> Further
>>>>> disable_rc_polling (a dvb_usb module parameter) must be set to not get
>>>>> doubled characters now. And then, it works like a charm.
>>>> Module parameters always bothers me. They should be used as last
>>>> resort alternatives
>>>> when there's no other possible way to make it work properly.
>>>>
>>>> If we know for sure that the RC polling should be disabled by an
>>>> specific device,
>>>> just add this logic at the driver.
>>>
>>> Yes, this is planned and written below:
>>
>> Ok.
>>>
>>>>> Note that, it's just some kind of proof of concept. A migration of
>>>>> af9015 devices from dvb-usb-remote needs to be done first.
>>>>>
>>>>> Ideas, comments?
>>>> Please next time, send the patch inlined. As you're using
>>>> Thunderbird, you'll likely need
>>>> Asalted-patches[1] to avoid thunderbird to destroy your patches.
>>>
>>> I must disagree for two reasons: (a) it was not patch intended for merge
>>> and (b) it was a plain-text attachment which is fine even for
>>> submission. However I don't like patches as attachments so if I decide
>>> to submit it for a merge later, you will not see it as an attachment
>>> then :).
>>
>> Attachments aren't good for reply, as they appear as a file. So,
>> people need to
>> open the attachment on a separate application to see and to cut-and-paste
>> if they want to comment, like what I did.
> 
> Just as an FYI... If you use mutt appropriately configured, it'll DTRT
> with attached patches and let you reply with them quoted inline, and
> actually, thunderbird 3 will more or less work with attached patches if
> you do a select-all, then hit reply (tbird finally has 'quote selected
> text' support).

RHEL5 has Thunderbird 2, so, quote selected text doesn't work. I don't 
like very much to use text mailers, but i prefer the alpine interface.
I never saw this feature in alpine. Maybe I just never managed to properly
configure it there.

Claws-mail has this feature, its monothread/monotask structure is very
bad, since it stops answering to the edit window, if it starts to fetch new
emails while you're editing an email. So, I stopped using it.

-- 

Cheers,
Mauro

Re: [PATCH 1/1] media: dvb-usb/af9015, fix disconnection crashes

[Pekka Sarnila]

Mauro Carvalho Chehab wrote:
> Pekka Sarnila wrote:
> 
>>The problem using vendor class is that there is no standard. Each vendor
>>can define its own way using endpoints (and has done so e.g Logitech
>>joysticks). Thus each usb ir receiver must have its own specific driver.
>> However then you get the raw ir codes. When using HID class, generic
>>HID driver can do the job. But then you get HID key codes directly not
>>ir codes.
> 
> We started writing an abstraction layer for IR, using the input. The idea
> is to allow the IR receivers to work with different IR's, as several users
> prefer to use universal IR's to control their devices, instead of the original
> one. This is already used by all V4L drivers and I intend to port most of
> the DVB drivers to use it as well soon.

This is very good.

The problem here is that at least afatech ir receiver has the ir code to 
key code build in, so trough HID you can use only the remote whose ir to 
key translate table has been loaded to the aftech device. Unless that 
can be easily controlled by the user HID is no good for this.

> 
>>Also this should not be seen at all as dvb question. First, not all the
>>world uses dvb standard (including USA) but uses very similar tv-sticks
>>with identical ir receivers and remotes. 
> 
> Despiste its name, the DVB subsystem is not specific for DVB standards, but
> it is meant to be used by all DTV standards (and almost all DTV standards
> are already supported).

But most of the world still uses analog (they can not afford yet the 
transfer). Those tv-sticks have identical receivers. And there are usb 
ir receivers not embedded to tv-sticks. Conceptually and technically it 
has nothing to do with any tv or any other audiovisual system system 
(e.g. a remote controlled weather station).

So dvb is both as a place and a name misleading.

>>Second there are many other
>>type of usb devices with ir receiver. So dvb layer should not be
>>involved at all. There maybe would be need for hid-ir-remote layer (your
>>code suggestion moved there). However even there IMHO better would be
>>just to improve HID <-> input layer interface so that input layer could
>>divert the remotes input to generic remotes layer instead of keyboard
>>layer. IMHO this would be the real linux way.
>  
> This is already happening.
> 
> See drivers/media/IR on linux-next for the IR common code. The ir-core is
> provided by ir-keytable.c and ir-sysfs.c, and it is not DVB or V4L specific.

Well I was talking about HID remotes that don't give ir codes but key 
codes. What to do with them?

> The ir-common module were developed for V4L drivers and will probably be
> changed into a more generic way, with the integration with lirc.
> 
> 
>>However linux usb layer has been build so that it was technically
>>impossible to put it there without completely redesigning usb <-> higher
>>layer (including HID) interface. But I'm of the opinion that that
>>redesign should be done anyway. 
> 
> Feel free to submit patches. My plan is to integrate the DVB devices soon

Yes, I have thought of it. But it is a big job, I'm quite busy, and 
after all mostly the benefit is more esthetical than practical. A lot of 
other usb strandard and vendor class interrupt endpoint drivers beside 
HID should be rewritten. Writing new ones would be easier though.

> into the new ir-core. I should start with dvb-usb-remote, where most of the
> DVB USB devices use to attach their IR's. Unfortunately, af9015 doesn't 
> rely on the dvb-usb-remote, so, it will require some specific changes.
> As I don't have any af9015 device, I'll likely postpone it or wait for
> someone to do the job.

Well that was the original point of my involvement. It can support both 
the dvb-usb-remote and HID. The problem with af9015/dvb-usb-remote is 
that it uses usb vendor class endpoint (all trough I have used 'vendor 
class' to specifically mean usb vendor class). Usb vendor classes have 
no standard. But af9015 can also use USB HID class (remote as keyboard) 
in a standard manner. That would avoid using special device  based driver.

>>Now the question is, how much all usb based ir receivers have in common,
>>and how much they differ. This should determine on what level and in
>>which way they should be handled. How much and on what level there
>>should be common code and where device specific driver code would be
>>needed and where and how the ir-to-code translate should take place.
> 
> 
> There are several different ways for IR receivers, and, at least for 
> vendor class, no standard way to handle. They can use GPIO polling,
> they can use i2c layer, they can use IRQ (on PCI devices) and the data
> may be provided in parallel or on a serial interface.

Well the thing is that even with usb each device can have non standard 
user vendor endpoint. In case of af9015 it can provide the ir code.

> The idea of the ir-core is to provide support for all those options.

Even for those that do provide key codes trough standard HID layer 
instead of ir codes trough specific device drivers?

>>IMHO all that have HID endpoint that works (either as such or with some
>>generic quirk) should be handled by HID first and then conveyed to
>>generic remotes layer that handles all remote controllers what ever the
>>lower layers (and does translate per remote not per ir receiver). Vendor
>>class should be avoided unless that's the only way to make it work
>>right. But using HID is not without problems either.
> 
> Almost all chipsets only provide IR via vendor class. I agree that using 
> standard HID class is the better way for doing it.

Yes, but look below.

>>Now with the two receivers that need the quirk. If they don't have
>>vendor class bulk endpoint for reading ir codes, then specific driver is
>>out anyway. However then changes to HID driver would be needed to make
>>the translate work. The quirk just makes them work as generic usb
>>keyboard with no remote specific translate. With afatech, driver loads
>>the translate table to the device, different for different remotes. I
>>don't know if one table could handle them all. Maybe this table should
>>be loaded from a user space file. Nor do I know how it is with other
>>receivers: can the table be loaded or is it fixed. In any case a generic
>>secondary per remote user configurable translate would be highly
>>desirable. And I don't mean lircd. This job is IMHO kernel level job and
>>lircd won't work here anyway. It does ir code to key code or function
>>translate not key code to key code translate. How nice it would be if
>>there would be a generic usb ir receiver class that all vendors used.
>>There seems to be no way to  make this well and clean.
> 
> 
> The ir-core provides standard ways to replace the IR keycode and IR protocols.
> The IR protocol change is already working with vendor class with em28xx driver.

The thing is that remote controller trough HID layer does not provide IR 
keycode but standard keyboard key code. And HID layer does not know that 
it is a remote controller but sees it as an ordinary usb keyboard. The 
question is that how can it tell the upper layer that it really is a 
remote controller so that the event would end up to generic ir-core.

Pekka

> Cheers,
> Mauro

Re: [PATCH 1/1] media: dvb-usb/af9015, fix disconnection crashes

[Mauro Carvalho Chehab]

Pekka Sarnila wrote:

> The problem here is that at least afatech ir receiver has the ir code to
> key code build in, so trough HID you can use only the remote whose ir to
> key translate table has been loaded to the aftech device. Unless that
> can be easily controlled by the user HID is no good for this.

The ir-core allows a driver to start with a built in table, that can be
dynamically replaced in userspace by a different table, and even with a
different protocol, if supported by the driver/device.

> But most of the world still uses analog (they can not afford yet the
> transfer). Those tv-sticks have identical receivers. And there are usb
> ir receivers not embedded to tv-sticks. Conceptually and technically it
> has nothing to do with any tv or any other audiovisual system system
> (e.g. a remote controlled weather station).
> 
> So dvb is both as a place and a name misleading.

It happens that almost all tv products (analog or digital) come with some 
IR support. But you can find also some products that are just IR.

That's why we're moving it to be outside the V4L and the DVB directory.
For now, it is at /drivers/media/IR (since it helps us to move the code, while
there are still some dependencies at ir-common). But the better is to move it
later to /drivers/IR or /drivers/input/IR.
 
>> See drivers/media/IR on linux-next for the IR common code. The ir-core is
>> provided by ir-keytable.c and ir-sysfs.c, and it is not DVB or V4L
>> specific.
> 
> Well I was talking about HID remotes that don't give ir codes but key
> codes. What to do with them?

What happens if you use it to receive keycodes from a different IR using
the same protocol?

Maybe it can still be valid to keep allowing keycode translation.

>> As I don't have any af9015 device, I'll likely postpone it or wait for
>> someone to do the job.
> 
> Well that was the original point of my involvement. It can support both
> the dvb-usb-remote and HID. The problem with af9015/dvb-usb-remote is
> that it uses usb vendor class endpoint (all trough I have used 'vendor
> class' to specifically mean usb vendor class). Usb vendor classes have
> no standard. But af9015 can also use USB HID class (remote as keyboard)
> in a standard manner. That would avoid using special device  based driver.

Well, as HID, you may loose the capability of using a different IR than the
one shipped with the af9015. It may make sense to just disable HID and use
USB Vendor Class.
 
> Well the thing is that even with usb each device can have non standard
> user vendor endpoint. In case of af9015 it can provide the ir code.

It doesn't matter how the IR code is get, kernel needs to translate it into
a linux key. That's where the ir-core enters: instead of registering the device
directly at input event, the driver registers via ir_input_register():

int ir_input_register(struct input_dev *input_dev,
                      const struct ir_scancode_table *rc_tab,
                      const struct ir_dev_props *props)

The arguments are the input device, the keycode->scancode table and an optional
argument that specifies the IR supported protocols, and a callback function
to be called when a different protocol is requested.

The IR subsystem will do a dynamic allocation for the keytable and will take 
care of EVIOCGKEY/EVIOCSKEY events. It will increase/decrease the keytable size
if needed.

This way, userspace may replace the scancode/keycode table and even the IR protocol,
without needing to add any specific code at the driver.

It will also create some sysfs nodes that will help udev to identify when a new IR
device driver is loaded, allowing the keycode replacement during device hotplug.

The only needed change, at the driver, is to replace input_register_device/input_unregister_device
by ir_input_register/ir_input_unregister.

> 
>> The idea of the ir-core is to provide support for all those options.
> 
> Even for those that do provide key codes trough standard HID layer
> instead of ir codes trough specific device drivers?

It basically depends on what the HID layer receives from the IR. Yet, it is possible to
use the ir-core layer in order to use an IR keycode to produce a different code. It is
not always clear what certain remote keys are supposed to do.

For example, should the <POWER> key turn off the PC, or just quit the application?

>> The ir-core provides standard ways to replace the IR keycode and IR
>> protocols.
>> The IR protocol change is already working with vendor class with
>> em28xx driver.
> 
> The thing is that remote controller trough HID layer does not provide IR
> keycode but standard keyboard key code. And HID layer does not know that
> it is a remote controller but sees it as an ordinary usb keyboard. The
> question is that how can it tell the upper layer that it really is a
> remote controller so that the event would end up to generic ir-core.

For HID remote controllers, the only way I see is to have an USB ID list of devices
that are known to be remote controllers and register them via ir_input_register,
instead of input_register_device.

Cheers,
Mauro

Re: [PATCH 1/1] media: dvb-usb/af9015, fix disconnection crashes

[Dmitry Torokhov]

On Thu, Feb 04, 2010 at 11:31:45AM +0100, Jiri Slaby wrote:
 +
> +static int dvb_event(struct hid_device *hdev, struct hid_field *field,
> +		struct hid_usage *usage, __s32 value)
> +{
> +	/* we won't get a "key up" event */
> +	if (value) {
> +		input_event(field->hidinput->input, usage->type, usage->code, 1);
> +		input_event(field->hidinput->input, usage->type, usage->code, 0);

Do not ever forget input_sync(), you need 2 of them here.

With the latest changes to evdev, if you are using SIGIO you won't get
wioken up until EV_SYN/SYN_REPORT.

-- 
Dmitry

Re: [PATCH 1/1] media: dvb-usb/af9015, fix disconnection crashes

[Jiri Slaby]

On 02/04/2010 07:14 PM, Dmitry Torokhov wrote:
> On Thu, Feb 04, 2010 at 11:31:45AM +0100, Jiri Slaby wrote:
>  +
>> +static int dvb_event(struct hid_device *hdev, struct hid_field *field,
>> +		struct hid_usage *usage, __s32 value)
>> +{
>> +	/* we won't get a "key up" event */
>> +	if (value) {
>> +		input_event(field->hidinput->input, usage->type, usage->code, 1);
>> +		input_event(field->hidinput->input, usage->type, usage->code, 0);
> 
> Do not ever forget input_sync(), you need 2 of them here.
> 
> With the latest changes to evdev, if you are using SIGIO you won't get
> wioken up until EV_SYN/SYN_REPORT.

HID layer syncs on its own. So the second is not needed. Why is needed
the first?

I.e. should there be one also in dvb_usb_read_remote_control?

Re: [PATCH 1/1] media: dvb-usb/af9015, fix disconnection crashes

[Dmitry Torokhov]

On Thu, Feb 04, 2010 at 07:33:22PM +0100, Jiri Slaby wrote:
> On 02/04/2010 07:14 PM, Dmitry Torokhov wrote:
> > On Thu, Feb 04, 2010 at 11:31:45AM +0100, Jiri Slaby wrote:
> >  +
> >> +static int dvb_event(struct hid_device *hdev, struct hid_field *field,
> >> +		struct hid_usage *usage, __s32 value)
> >> +{
> >> +	/* we won't get a "key up" event */
> >> +	if (value) {
> >> +		input_event(field->hidinput->input, usage->type, usage->code, 1);
> >> +		input_event(field->hidinput->input, usage->type, usage->code, 0);
> > 
> > Do not ever forget input_sync(), you need 2 of them here.
> > 
> > With the latest changes to evdev, if you are using SIGIO you won't get
> > wioken up until EV_SYN/SYN_REPORT.
> 
> HID layer syncs on its own. So the second is not needed. Why is needed
> the first?
> 

Userpsace has a right to accumulate events and only act on them when
receiving EV_SYN. Press + release in the same event block may be treated
as no change. The same as REL_X +2, REL_X -2 - no need to move pointer at
all. And so on.

> I.e. should there be one also in dvb_usb_read_remote_control?

Probably, I have not looked.

-- 
Dmitry

Re: [PATCH 1/1] media: dvb-usb/af9015, fix disconnection crashes

[Jarod Wilson]

On 02/04/2010 08:41 AM, Mauro Carvalho Chehab wrote:
> Jiri Slaby wrote:
>> On 02/04/2010 01:04 PM, Mauro Carvalho Chehab wrote:
>>>> I have 2 dvb-t receivers and both of them need fullspeed quirk. Further
>>>> disable_rc_polling (a dvb_usb module parameter) must be set to not get
>>>> doubled characters now. And then, it works like a charm.
>>> Module parameters always bothers me. They should be used as last resort alternatives
>>> when there's no other possible way to make it work properly.
>>>
>>> If we know for sure that the RC polling should be disabled by an specific device,
>>> just add this logic at the driver.
>>
>> Yes, this is planned and written below:
>
> Ok.
>>
>>>> Note that, it's just some kind of proof of concept. A migration of
>>>> af9015 devices from dvb-usb-remote needs to be done first.
>>>>
>>>> Ideas, comments?
>>> Please next time, send the patch inlined. As you're using Thunderbird, you'll likely need
>>> Asalted-patches[1] to avoid thunderbird to destroy your patches.
>>
>> I must disagree for two reasons: (a) it was not patch intended for merge
>> and (b) it was a plain-text attachment which is fine even for
>> submission. However I don't like patches as attachments so if I decide
>> to submit it for a merge later, you will not see it as an attachment
>> then :).
>
> Attachments aren't good for reply, as they appear as a file. So, people need to
> open the attachment on a separate application to see and to cut-and-paste
> if they want to comment, like what I did.

Just as an FYI... If you use mutt appropriately configured, it'll DTRT 
with attached patches and let you reply with them quoted inline, and 
actually, thunderbird 3 will more or less work with attached patches if 
you do a select-all, then hit reply (tbird finally has 'quote selected 
text' support).

Not that I'm advocating patches as attachments.

-- 
Jarod Wilson
jarod@redhat.com

Re: [PATCH 1/1] media: dvb-usb/af9015, fix disconnection crashes

[Pekka Sarnila]

Mauro Carvalho Chehab wrote:
> Pekka Sarnila wrote:

>>So dvb is both as a place and a name misleading.
> 
> It happens that almost all tv products (analog or digital) come with some 
> IR support. But you can find also some products that are just IR.
> 
> That's why we're moving it to be outside the V4L and the DVB directory.
> For now, it is at /drivers/media/IR (since it helps us to move the code, while
> there are still some dependencies at ir-common). But the better is to move it
> later to /drivers/IR or /drivers/input/IR.

I was referring to the dvb-usb-remote. I think it's right place should 
not be under dvb but under usb/remotes. If you have an usb remote device 
that has nothing to do with audio-video, it would be strange to have its 
driver under dvb. Dvb is a protocol name and should be used us such in 
linux kernel as well. Otherwise it is misleading.

>>Well I was talking about HID remotes that don't give ir codes but key
>>codes. What to do with them?
>  
> What happens if you use it to receive keycodes from a different IR using
> the same protocol?

Nothing. It responds only to those codes that are at the moment loaded 
into its internal table.

> Maybe it can still be valid to keep allowing keycode translation.

Yes, provided that it can translate also keyboard keycodes to some other 
keycodes.

> Well, as HID, you may loose the capability of using a different IR than the
> one shipped with the af9015. It may make sense to just disable HID and use
> USB Vendor Class.

Yes, that is the question. Both ways have good arguments for.

>>The thing is that remote controller trough HID layer does not provide IR
>>keycode but standard keyboard key code. And HID layer does not know that
>>it is a remote controller but sees it as an ordinary usb keyboard. The
>>question is that how can it tell the upper layer that it really is a
>>remote controller so that the event would end up to generic ir-core.
> 
> For HID remote controllers, the only way I see is to have an USB ID list of devices
> that are known to be remote controllers and register them via ir_input_register,
> instead of input_register_device.

Well, the current kernel architecture for input devices is so, that a 
higher layer generic driver registers a handler with the input layer 
telling by the way of capabilities what kind deices it is willing to 
handle. Lower layer devices drivers register with the input layer 
telling it what kind of capabilities they have. Based on that the input 
layer finds a match and relays events accordingly. Although arguments 
against this mechanism can be found, one of its good sides is that it 
completely separates generic device layer from device driver layer. E.g. 
the coder of mouse layer does not need to know anything about various 
mouse device drivers and vv. This is very good for the highly 
distributed way linux is being developed.

IMHO ir remote controller receivers should be no exception to that. So 
rather than use separate ir_input_register, a generic remote controller 
layer (not only IR code based) should register a handler with the input 
layer with suitable capabilities, and all remote controller device 
drivers should register with the input layer telling by capabilities 
that they are remote controllers. They would send events in standard way 
to the input layer. Input layer would do the rest as normally.

Also in naming, due consideration should be given to the fact that IR 
can be used for other type of communication as well.

> Cheers,
> Mauro

Re: [PATCH 1/1] media: dvb-usb/af9015, fix disconnection crashes

[Greg KH]

On Thu, Feb 04, 2010 at 04:07:36PM +0200, Pekka Sarnila wrote:
> Yes, my comment maybe criticizes more the basic architectural structure of 
> usb putting it's own work up to higher layer. The only practical thing is 
> that, if there is a non-HID device suffering from that FULLSPEED problem, 
> the quirk won't help it. Anyway in current kernel structure usb layer 
> doesn't handle endpoint setup at all, thus it simply can not do the job.

Patches to the USB core to resolve this issue are always gladly
appreciated :)

thanks,

greg k-h