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

[Jiri Slaby <jirislaby@gmail.com>]

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