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

[Dmitry Torokhov]

Hi Janusz,

On Sat, Dec 12, 2009 at 09:34:07PM +0100, Janusz Krzysztofik wrote:
> 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,

I guess I meant "proper keyboard driver" in the sense of a driver that
creates and registeres its own input_dev and perform direct translation
of data coming form hardware into linux input events.

>  and what kind of devices can be considered as generic 
> serio-style data sources and what can't, and more? 

If a same device can be used on different platforms and several devices
may use the same port then I can see the need of creating a serio
abstraction. But if tere is 1:1 relationship then a driver that attaches
directly to the hardware might make sense.

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

Of course atkbd would work fine if it is fed the data it expects. The
same can be said for any driver in the tree, isn't it? It is possible
to write HID-to-atkbd or "gpio matrix to atkbd" "serios" but that does
not mean it is the right thing to do.

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

Can they be used simultaneously? Would not the translation that you
perform right now cause issues with the gamepad? Are you thinking about
reusing one of the existing gamepad modules in the same fashion as you
do for atkbd but translating the data into its native format?

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

atkbd is supposed to support PS/2 devices speaking AT keyboard protocol.
We also do support such devices behind dumb controllers that do not
allow querying the device but we expect those to at least produce the
standard scancodes.

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

Yes, if there can be several devices connected to it then it makes sense
to create a port abstraction for different drivers to plug into.

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

Yes, exactly.

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

I would need to know a bit more about the gamepad, but so far I still
think that current attempot of creating intermediate serio module which
allows to use atkbd driver is not the proper solution.

-- 
Dmitry