Re: [patch 2/2] Add a driver for the Winbond WPCD376I Consumer IR hardware

From: "David Härdeman" <david@hardeman.nu>
To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: linux-kernel@vger.kernel.org, linux-input@vger.kernel.org,
	jbarnes@virtuousgeek.org, akpm@linux-foundation.org
Subject: Re: [patch 2/2] Add a driver for the Winbond WPCD376I Consumer IR hardware
Date: Thu, 13 Aug 2009 19:58:43 +0200	[thread overview]
Message-ID: <20090813175843.GA11922@hardeman.nu> (raw)
In-Reply-To: <20090813162726.DBD9A526EC9@mailhub.coreip.homeip.net>

On Thu, Aug 13, 2009 at 08:56:37AM -0700, Dmitry Torokhov wrote:
>On Thu, Aug 13, 2009 at 11:34:44AM +0200, David Härdeman wrote:
>> The main problem right now is that getkeycode is practically useless since
>> you can't blindly guess at a full range of 2^32 different scancodes to get
>> the complete keymap. Perhaps a index-based getkeycode would make sense...
>
>The drivers that have such sparce keymaps are expected to issue
>EV_MSC/MSC_SCAN events to aid userspace in identifying the "scancodes"
>that are emitted by the device.

Ok, I've added EV_MSC/MSC_SCAN support.

>>>> +static struct device_attribute dev_attr_last_scancode = {
>>>> +	.attr = {
>>>> +		.name = "last_scancode",
>>>> +		.mode = 0444,
>>>> +	},
>>>> +	.show = wbcir_show_last_scancode,
>>>> +	.store = NULL,
>>>> +
>>>> +};
>>>
>>> Why is this needed? And if this is needed we have a nice macro
>>> for that.
>>
>> I hope I've explained it wrt. EV_IR in my other mail. It's for building
>> keymaps of unknown remotes. And it'll go away once EV_IR is supported so I
>> don't think there's much point in fiddling with it now?
>>
>
>Because once the driver is in mainline it becomes part of userspace ABI
>and has to stay for a looong time.

I've removed the sysfs attribute as EV_MSC/MSC_SCAN provides the same 
functionality.

>> Thanks for the review. Are you willing to push the driver upstream through
>> the input tree once I've implemented your suggested fixes?
>>
>
>I'd need to take a look at your EV_IR patyches and see how they will
>affect this driver. I do nt want to merge something that will stay one
>way for half a release and then will switch to completely new interface.

The EV_IR functionality is intended to be used in addition to regular 
key up/down/repeat events. Much like EV_MSC/MSC_SCAN but more 
descriptive and specific to IR protocols. Then advanced user-space apps 
can choose whether to use the in-kernel keymap or map remote codes 
directly to suitable events (and it allows remote keymaps to be built 
easily in user-space).

As the future EV_IR functionality is additional to the current 
functionality, I hope the driver still can be merged before I have EV_IR 
patches ready for review. I'll post an updated patch shortly.

-- 
David Härdeman