On Mon, Apr 20, 2009 at 12:06:40PM +0100, Russell King - ARM Linux wrote:
> On Sat, Apr 18, 2009 at 04:10:28PM -0700, Dmitry Torokhov wrote:
> > > +config KEYBOARD_LX
> > > + tristate "LX Keyboard support"
> > > + depends on MACH_NETBOOKPRO && INPUT && INPUT_KEYBOARD
> >
> > Dependancy on INPUT_KEYBOARD is not needed, strictly speaking.
> > INPUT_KEYBOARD is only [menu]config syntactic sugar, there is no meat
> > behind the option.
>
> Gone.
>
> > > +
> > > + /*
> > > + * Some keys have additional operations after we have
> > > + * decoded them. We don't report these "special" keys
> > > + * to the input layer.
> > > + */
> > > + switch (key) {
> > > + case KEY_BRIGHTNESSUP:
> > > + case KEY_BRIGHTNESSDOWN:
> > > + input_event(kbd, EV_PWR, key, !up);
> >
> > Umm... some userspace apps might still want to get these. Strictly
> > speaking KEY_* codes belong to EV_KEY namespace.
>
> Maybe, if someone wants it I guess they can modify the driver.
>
OK.
> > > +static void lx_keyb_reset(struct input_dev *kbd)
> > > +{
> > > + unsigned char *map;
> > > + int key;
> > > +
> > > + for (key = 0; key < LX_NUMKEYCODES; key++)
> > > + if (keystate[key] & KP_DOWN) {
> > > + map = normal_map;
> > > + if (keystate[key] & KP_FN)
> > > + map = fn_map;
> > > + else if (keystate[key] & KP_NUMLCK)
> > > + map = numlck_map;
> > > + keystate[key] = KP_NONE;
> > > + input_report_key(kbd, map[key], 0);
> > > + }
> >
> > Why don't you just send 'up' event for all known keys? Input core will
> > filter out the ones that have not been 'down'ed... Better yet, don't do
> > anything here, I should finish adding forced release at suspend/resume
> > to the input core shortly.
>
> I'll leave it as is until you've decided what to do with the input core.
>
Fair enough.
> > > +static void lx_keyb_write(void *data, unsigned int addr, unsigned char val)
> > > +{
> > > + struct input_dev *kbd = data;
> >
> > Blank line between variables and the code is always appreciated ;)
>
> Added.
>
> > > + kbd->name = "LX Keyboard";
> > > + kbd->phys = "pcon/input0";
> > > + kbd->dev.parent = &dev->dev;
> > > + kbd->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_REP) |
> > > + BIT_MASK(EV_PWR);
> > > + kbd->keycode = normal_map;
> > > + kbd->keycodesize = sizeof(unsigned char);
> > > + kbd->keycodemax = ARRAY_SIZE(normal_map);
> > > +
> >
> > So we have 3 keymaps but only one can be changed form userspace... I
> > suppose it is OK given how special the other 2 keymaps are. Or we could
> > provide an alternative {get,set}keycode methods.
>
> Yes, not sure what to do about those. It is a rather specialised
> keyboard in itself, so the standard maps wouldn't work with it.
>
> The other issue here is that changing what's published may break the
> existing userspace (and since I didn't take a copy of the userspace
> build for this platform before the server was taken down, that could
> present something of a problem resulting in my use of the device being
> crippled.) Let's leave it as is and if someone does want to change it
> later...
>
I did not mean we would change anything as far as default setup is
concerned and is visible from userspace; it would be all internal
adjustment that would also allow changing numlck_map and fn_map mapping
in addition to changing normal_map. I will just put it on my TODO
list...
> > > +static int __init lx_keyb_init(void)
> > > +{
> > > + printk(KERN_INFO "LX Keyboard Driver, (c) 2004 Simtec Electronics\n");
> >
> > Is this needed? Input core will already add printk when input device is
> > added and our boot is already chatty enough...
>
> As mentioned in the other mail, now removed.
Thanks.
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
