LinuxLists.cc - [PATCH 1/1] media: dvb-usb/af9015, fix disconnection crashes

[permalink] [raw]

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

On 01/25/2010 12:44 AM, Antti Palosaari wrote:
> When I now test this patch with debugs enabled I don't see
> .probe and .disconnect be called for this HID interface (interface 1) at
> all and thus checks not needed.

What happens if you disable the HID layer? Or at least if you add an
ignore quirk for the device in usbhid?

I forbid usbhid to attach to the device, as the remote kills X with HID
driver. With dvb-usb-remote it works just fine (with remote=2 for af9015
or the 4 patches I've sent).

--
js

2010-01-25 18:08:09

by Antti Palosaari

[permalink] [raw]

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

On 01/25/2010 11:12 AM, Jiri Slaby wrote:
> On 01/25/2010 12:44 AM, Antti Palosaari wrote:
>> When I now test this patch with debugs enabled I don't see
>> .probe and .disconnect be called for this HID interface (interface 1) at
>> all and thus checks not needed.
>
> What happens if you disable the HID layer? Or at least if you add an
> ignore quirk for the device in usbhid?

Looks like Fedora doesn't have usbhid compiled as module. I looked
hid-quirks.c file and there was only one af9015 device blacklisted
15a4:9016. I have 15a4:9015, 15a4:9016 and 13d3:3237 devices and no
difference.

How can I disable HID layer?

> I forbid usbhid to attach to the device, as the remote kills X with HID
> driver. With dvb-usb-remote it works just fine (with remote=2 for af9015
> or the 4 patches I've sent).

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.

But all in all, your patch does not break anything, it is safe to add.
It could be still nice to know if there is better alternatives. And
there is surely few other devices having HID remote - are those also
affected.

regards
Antti
--
http://palosaari.fi/

2010-01-26 13:08:49

by Jiri Kosina

[permalink] [raw]

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

On Mon, 25 Jan 2010, Antti Palosaari wrote:

> > What happens if you disable the HID layer? Or at least if you add an
> > ignore quirk for the device in usbhid?
>
> Looks like Fedora doesn't have usbhid compiled as module. I looked
> hid-quirks.c file and there was only one af9015 device blacklisted 15a4:9016.
> I have 15a4:9015, 15a4:9016 and 13d3:3237 devices and no difference.
>
> How can I disable HID layer?

In case usbhid is compiled in, you should still be able to force the
ignore quirk by passing

usbhid.quirks=0x15a4:0x9015:0x04

to kernel boot commandline.

> > I forbid usbhid to attach to the device, as the remote kills X with HID
> > driver. With dvb-usb-remote it works just fine (with remote=2 for af9015
> > or the 4 patches I've sent).
>
> 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.

> But all in all, your patch does not break anything, it is safe to add. It
> could be still nice to know if there is better alternatives. And there is
> surely few other devices having HID remote - are those also affected.

--
Jiri Kosina
SUSE Labs, Novell Inc.

2010-02-04 10:31:53

[permalink] [raw]

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

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

Attachments:

dvb-remote-fix.patch (5.86 kB)

2010-02-04 12:05:32

[permalink] [raw]

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

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

2010-02-04 12:53:15

[permalink] [raw]

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

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

2010-02-04 13:21:52

[permalink] [raw]

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

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;

2010-02-04 13:27:46

by Jiri Kosina

[permalink] [raw]

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

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.

2010-02-04 13:42:26

[permalink] [raw]

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

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

2010-02-04 13:51:53

[permalink] [raw]

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

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

2010-02-04 14:04:46

[permalink] [raw]

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

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

2010-02-04 14:05:52

[permalink] [raw]

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

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).
>

2010-02-04 14:06:30

[permalink] [raw]

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

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

2010-02-04 14:07:58

by Jarod Wilson

[permalink] [raw]

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

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
[email protected]

2010-02-04 14:27:29

[permalink] [raw]

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

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

2010-02-04 15:03:58

[permalink] [raw]

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

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

2010-02-04 17:14:15

[permalink] [raw]

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

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

2010-02-04 18:14:18

by Dmitry Torokhov

[permalink] [raw]

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

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

2010-02-04 18:33:33

[permalink] [raw]

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

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?

2010-02-04 18:41:49

by Dmitry Torokhov

[permalink] [raw]

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

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

2010-02-05 14:30:24