Date:	Thu, 13 Aug 2009 08:56:37 -0700
From:	Dmitry Torokhov <dmitry.torokhov@...il.com>
To:	David Härdeman <david@...deman.nu>
Cc:	linux-kernel@...r.kernel.org, linux-input@...r.kernel.org,
	jbarnes@...tuousgeek.org, akpm@...ux-foundation.org,
	bjorn.helgaas@...com, randy.dunlap@...cle.com
Subject: Re: [patch 2/2] Add a driver for the Winbond WPCD376I Consumer IR
	hardware

On Thu, Aug 13, 2009 at 11:34:44AM +0200, David Härdeman wrote:
> On Thu, August 13, 2009 08:58, Dmitry Torokhov wrote:
> > Hi David,
> 
> Hi,
> 
> > Please keep Makefile sorted alphabetically.
> 
> Ok
> 
> >> +static struct wbcir_key rc6_def_keymap[] = {
> >> +	{ 0x800F0400, KEY_0			},
> >> +	{ 0x800F0401, KEY_1			},
> >> +	{ 0x800F0402, KEY_2			},
> >> +	{ 0x800F0403, KEY_3			},
> >> +	{ 0x800F0404, KEY_4			},
> >> +	{ 0x800F0405, KEY_5			},
> >> +	{ 0x800F0406, KEY_6			},
> >> +	{ 0x800F0407, KEY_7			},
> >> +	{ 0x800F0408, KEY_8			},
> >> +	{ 0x800F0409, KEY_9			},
> >
> > Make these ones KEY_NUMERIC_* as well, this should help users whose
> > keymaps have numbers in upper register normally.
> 
> Ok
> 
> >> +	{ 0x800F041D, KEY_NUMERIC_STAR		},
> >> +	{ 0x800F041C, KEY_NUMERIC_POUND		},
> >> +	{ 0x800F0410, KEY_VOLUMEUP		},
> >> +	{ 0x800F0411, KEY_VOLUMEDOWN		},
> >> +	{ 0x800F0412, KEY_CHANNELUP		},
> >> +	{ 0x800F0413, KEY_CHANNELDOWN		},
> >> +	{ 0x800F040E, KEY_MUTE			},
> >> +	{ 0x800F040D, KEY_VENDOR		}, /* Vista Logo Key */
> >> +	{ 0x800F041E, KEY_UP			},
> >> +	{ 0x800F041F, KEY_DOWN			},
> >> +	{ 0x800F0420, KEY_LEFT			},
> >> +	{ 0x800F0421, KEY_RIGHT			},
> >> +	{ 0x800F0422, KEY_OK			},
> >> +	{ 0x800F0423, KEY_ESC			},
> >> +	{ 0x800F040F, KEY_INFO			},
> >> +	{ 0x800F040A, KEY_CLEAR			},
> >> +	{ 0x800F040B, KEY_ENTER			},
> >> +	{ 0x800F045B, KEY_RED			},
> >> +	{ 0x800F045C, KEY_GREEN			},
> >> +	{ 0x800F045D, KEY_YELLOW		},
> >> +	{ 0x800F045E, KEY_BLUE			},
> >> +	{ 0x800F045A, KEY_TEXT			},
> >> +	{ 0x800F0427, KEY_SWITCHVIDEOMODE	},
> >> +	{ 0x800F040C, KEY_POWER			},
> >> +	{ 0x800F0450, KEY_RADIO			},
> >> +	{ 0x800F0448, KEY_PVR			},
> >> +	{ 0x800F0447, KEY_AUDIO			},
> >> +	{ 0x800F0426, KEY_EPG			},
> >> +	{ 0x800F0449, KEY_CAMERA		},
> >> +	{ 0x800F0425, KEY_TV			},
> >> +	{ 0x800F044A, KEY_VIDEO			},
> >> +	{ 0x800F0424, KEY_DVD			},
> >> +	{ 0x800F0416, KEY_PLAY			},
> >> +	{ 0x800F0418, KEY_PAUSE			},
> >> +	{ 0x800F0419, KEY_STOP			},
> >> +	{ 0x800F0414, KEY_FASTFORWARD		},
> >> +	{ 0x800F041A, KEY_NEXT			},
> >> +	{ 0x800F041B, KEY_PREVIOUS		},
> >> +	{ 0x800F0415, KEY_REWIND		},
> >> +	{ 0x800F0417, KEY_RECORD		},
> >
> > Umm, it looks like if you do (code & 0x800F0400) you can switch to
> > standard array-based keymap and won't even need list manipulation.
> 
> I didn't want to do that since it would potentially tie the driver to one
> particular remote (the one I used for testing while writing the driver).
> The hardware isn't shipped with any specific remote so that wouldn't be
> very user friendly.
> 
> This was the best solution I could come up with without adding some real
> limitations to the functionality of the driver.

I see.

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

> 
> Anyway, I hope that I can make this a moot point in the future by adding
> more IR-specific functionality to the input system. I've been thinking
> along the lines of IR-specific get/set-keycode ioctl's which would take a
> struct which defines the IR command to map to a key.
> 
> >> +};
> >> +
> >> +/* Registers and other state is protected by wbcir_lock */
> >> +struct wbcir_data {
> >> +	unsigned long wbase;        /* Wake-Up Baseaddr		*/
> >> +	unsigned long ebase;        /* Enhanced Func. Baseaddr	*/
> >> +	unsigned long sbase;        /* Serial Port Baseaddr	*/
> >> +	unsigned int  irq;          /* Serial Port IRQ		*/
> >> +
> >> +	struct input_dev *input_dev;
> >> +	struct timer_list timer_keyup;
> >> +	struct led_trigger *rxtrigger;
> >> +	struct led_trigger *txtrigger;
> >> +	struct led_classdev led;
> >> +
> >> +	u32 last_scancode;
> >> +	unsigned int last_keycode;
> >> +	u8 last_toggle;
> >> +	u8 keypressed;
> >> +	unsigned long keyup_jiffies;
> >> +	unsigned int idle_count;
> >> +
> >> +	/* RX irdata and parsing state */
> >> +	unsigned long irdata[30];
> >> +	unsigned int irdata_count;
> >> +	unsigned int irdata_idle;
> >> +	unsigned int irdata_off;
> >> +	unsigned int irdata_error;
> >> +
> >> +	/* Protected by keytable_lock */
> >> +	struct list_head keytable;
> >
> > I think this has a potential to deadlock... Set and get keycodes are
> > called with event lock taken, and then your implementations acquire
> > keytable lock. When you emit input events the opposite happens - you
> > take the keytable lock and then input core takes event lock.
> 
> Good catch, I'll have to look into that...
> 
> >> +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.

> >> +static int
> >> +wbcir_probe(struct pnp_dev *device, const struct pnp_device_id *dev_id)
> >
> > __devinit
> >
> ...
> >> +	dev_info(&device->dev, "Found device "
> >> +		 "(w: 0x%lX, e: 0x%lX, s: 0x%lX, i: %u)\n",
> >> +		 data->wbase, data->ebase, data->sbase, data->irq);
> >> +
> >
> > dev_dbg() I think.
> 
> Ok
> 
> >> +static void
> >> +wbcir_remove(struct pnp_dev *device)
> >
> > __devexit
> 
> Ok
> 
> >> +static struct pnp_driver wbcir_driver = {
> >> +	.name     = WBCIR_ACPI_NAME,
> >> +	.id_table = wbcir_ids,
> >> +	.probe    = wbcir_probe,
> >> +	.remove   = wbcir_remove,
> >
> > __devexit_p()
> 
> Ok
> 
> >> +	.suspend  = wbcir_suspend,
> >> +	.resume   = wbcir_resume,
> >
> > Switch to dev_pm_ops?
> 
> Don't want to do that just yet. dev_pm_ops wasn't even on my radar until
> yesterday when I stumbled upon the documentation (in a header file rather
> than in Documentation/...eh?). I'll certainly look into it when the
> suspend/resume functionality has seen more testing and bug fixing.
> 
> 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.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists