[E3-hacking] [RFC][PATCH 4/5] input: serio: add support for Amstrad Delta serial keyboard port

Sat Dec 12 20:34:07 GMT 2009

Friday 11 December 2009 09:01:28 Dmitry Torokhov napisał(a):
> On Fri, Dec 11, 2009 at 04:09:58AM +0100, Janusz Krzysztofik wrote:
> > Friday 11 December 2009 03:36:58 Dmitry Torokhov napisał(a):
> > > On Thu, Dec 10, 2009 at 09:07:50PM +0100, Janusz Krzysztofik wrote:
> > > > +
> > > > +/*
> > > > + * This table converts the amstrad mailboard scancodes to normal PC-
> > > > AT scancodes
> > > > + * The diagram below shows the amstrad keyboard, with the raw
> > > > scancodes
> > > > + *
> > > > + *   (70) (7A) (46) (7C) (77)   Amstrad     (72) (69) (1A) (2A)
> > > > (1C) (15)
> > > > + * [  71][1:74][2:73][3:6B][4:22][5:1B][6:1D][7:1E][8:79][9:7D]
> > > > [0:75][  6C]
> > > > + *  [Q:21][W:23][E:24][R:26][T:52][Y:5D][U:0D][I:0E][O:32][P:34]  |
> > > > return|
> > > > + *   [A:31][S:33][D:35][F:36][G:29][H:5B][J:03][K:76][L:3A][@:3B]
> > > >
> > > > |   2C|
> > > >
> > > > + *  [  3C][Z:3D][X:4E][C:54][V:0B][B:05][N:41][M:42][.:43][  3E]
> > > > [  55]
> > > > + *  [  83][  06][  49][           4B         ][,:44][  16][  2E]
> > > > [  09]
> > > > + *
> > > > + * These scancodes are then translated to AT scancodes using the
> > > > following table
> > > > + * The amstrad keyboard does not produce any extended scancodes,
> > > > but we need to
> > > > + * translate some amstrad scancodes to a AT extended scancode,
> > > > hence the 16bit
> > > > + * value for the translated scancode
> > > > + *
> > >
> > > No, please write a proper keyboard driver instead of creating this
> > > Frankenstain monster. It is not a generic serio-style data source so it
> > > should not use serio, should reside in drivers/input/keyboard and
> > > create input device by itself.
> >
> > I'll see if I'm able to create a proper driver myself when I find some
> > spare time.
>
> Yes, that would be great, thank you. In fact it should end up about the
> same as this driver, except instead of creating and registering serio
> port you will need to register input_dev structure and instead of
> translating into atkbd scancodes you need to translate directly into
> Linux keycodes (KEY_A, KEY_1 and so on).

Dmitry,

Thanks for your directions. However, before I get to work, please let me still 
better understand what I am really supposed to create here, and why you think 
it should be done one way and not the other. I'd rather avoid submitting an 
input related code that you might find not adequate in the future.

First, I have never submitted a single line of code that would be accepted for
inclusion into drivers/input before. Could you please recommend a
documentation for me to read, from where I could learn what a proper keyboard 
driver should look like, and what kind of devices can be considered as generic 
serio-style data sources and what can't, and more? Or should I better give up 
being that short of required knowledge?

Moreover, please have a look at these two messages posted by Cliff Lawson, who 
worked for Amstrad while the E3 (codename Delta) was being developed, being 
involved in that development:

http://www.earth.li/pipermail/e3-hacking/2006-April/000445.html
http://www.earth.li/pipermail/e3-hacking/2006-April/000449.html

As one can read, the Amstrad Delta keyboard was described by Cliff as a PS/2 
device, even if not connected to a regular PS/2 port but some MPU GPIO lines 
directly instead.

Furthermore, it has been proven by Matt Callow, who created the serio driver 
you didn't like, that the atkbd keyboard driver already works fine for this 
keyboard if fed with scancodes matching what it is told to expect.

Please also note that the E3 keyboard port is supposed to be used not only for 
getting input from the keyboard, but from a bundled gamepad as well. Not yet 
supported, but who knows if forever.

That said, do you still think there is a need to create a new, separate 
keyboard driver for the device in question? Isn't reusing an existing, proven 
to be working correctly, keyboard driver module, isn't it a good solution 
here? If not, could you please elaborate on why it's not? I always thought 
that being PS/2 compatible is enough for a device to be considered as 
supported by atkbd, but maybe I just don't understand what the atkbd driver is 
designed for and what a supported device should look like.

Moreover, isn't a port, that is supposed to interact with at least two 
distinct input devices, isn't it worth of creating a separate driver for it, 
be it serio module or anything else that would better match the input subsystem 
design?

In your initial answer, you quoted a part of the patch, namely that containing 
a description of the driver's scancode translation functionality. I guess you 
tried to say that traslating scancodes inside a port driver is wrong. I would 
agree with that, without any reservations.

But for your other conclusions, could you please reconsider if still valid, 
and elaborate on them if you think they are?

Thanks,
Janusz


PS. To be honest, I have to inform every E3 hacker still interrested in 
testing the driver, even if already NAKed upstream, that I happened to fail 
with testing the final, submitted version of this particular patch from the 
series. I tested everything, patch by patch, after splitting the initial one 
into several distinct and then after cleaning them up one by one, but 
unfortunatelly missed testing this one after being cleaned up finally. As a 
result, it doesn't build. Please use the extra patch below, applied over the 
series, if interrested in testing the driver in it's current form. I'm sorry 
for that.

Signed-off-by: Janusz Krzysztofik <jkrzyszt at tis.icnet.pl>

---

diff -upr git.orig/arch/arm/mach-omap1/include/mach/ams-delta-fiq.h git/arch/arm/mach-omap1/include/mach/ams-delta-fiq.h

--- git.orig/arch/arm/mach-omap1/include/mach/ams-delta-fiq.h	2009-12-11 18:19:19.000000000 +0100
+++ git/arch/arm/mach-omap1/include/mach/ams-delta-fiq.h	2009-12-11 19:19:04.000000000 +0100
@@ -51,6 +51,7 @@
 
 #define FIQ_CIRC_BUFF		30      /*Start of circular buffer */
 
+extern unsigned int fiq_buffer[];
 extern unsigned char qwerty_fiqin_start, qwerty_fiqin_end;
 
 extern void __init ams_delta_init_fiq(void);
diff -upr git.orig/drivers/input/serio/ams_delta_keyboard.c git/drivers/input/serio/ams_delta_keyboard.c
--- git.orig/drivers/input/serio/ams_delta_keyboard.c	2009-12-11 18:19:19.000000000 +0100
+++ git/drivers/input/serio/ams_delta_keyboard.c	2009-12-11 19:23:48.000000000 +0100
@@ -223,7 +223,7 @@ gpio_data:
 serio:
 	kfree(ams_delta_kbd_port);
 err:
-	return retval;
+	return err;
 }
 module_init(ams_delta_kbd_init);
 

