Message ID | 1387721496-28197-1-git-send-email-sw@weilnetz.de |
---|---|
State | Superseded |
Headers | show |
On 22 December 2013 14:11, Stefan Weil <sw@weilnetz.de> wrote: > cgcc reported a duplicate initialisation. Mainstone includes a matrix > keyboard where two different positions map to 'space'. > > QEMU uses the reversed mapping and cannot map 'space' to two different > matrix positions. > > Signed-off-by: Stefan Weil <sw@weilnetz.de> > --- > > Of course we could also randomly select one of the two matrix positions, > but I assume that this is not necessary. That would be kind of silly :-) We can either map 'space' to just one of the emulated mainstone keys, or we pick another key to map to the second 'space'. I don't know the hardware either, so don't know which would be preferable. > I don't know any mainstone internals, so I had to look into the Linux > code where I noticed some discrepancies which should be clarified: > > Extract from Linux: > > KEY(1, 5, KEY_LEFTSHIFT), > KEY(2, 5, KEY_SPACE), > KEY(3, 5, KEY_SPACE), > KEY(4, 5, KEY_ENTER), > KEY(5, 5, KEY_BACKSPACE), > > Extract from QEMU: > > [0x2a] = {5,1}, /* shift */ > [0x39] = {5,2}, /* space */ > [0x39] = {5,3}, /* space */ > [0x1c] = {5,5}, /* enter */ > > It looks like the 'enter' key is not mapped correctly, > and 'backspace' is missing. There are some other missing keys if you believe the Linux mapping. See also the kernel commit message 55c26e40 which suggests there are still further hardware keys which aren't handled by a single simple row/column. In all I would classify this under "don't bother fixing unless somebody actually complains, preferably with a confirmation that the kernel they're using does work correctly on real hardware". > hw/arm/mainstone.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/hw/arm/mainstone.c b/hw/arm/mainstone.c > index 9402c84..223a4b1 100644 > --- a/hw/arm/mainstone.c > +++ b/hw/arm/mainstone.c > @@ -76,7 +76,9 @@ static struct keymap map[0xE0] = { > [0xc7] = {5,0}, /* Home */ > [0x2a] = {5,1}, /* shift */ > [0x39] = {5,2}, /* space */ > +#if 0 /* always map to first column, row pair */ > [0x39] = {5,3}, /* space */ > +#endif A few remarks here. Firstly, this is a change of behaviour, because with the previous duplicate the compiler picks the last of the dup entries, so {5, 3}. (checked vs gcc and clang.) In the absence of any definite reason to switch we should definitely prefer to make no change in behaviour. Secondly, no #if 0, please. Add a descriptive comment about the lack of a key to map to the other matrix position. Thirdly, is this really stable material? It's been this way for six years, and the only actual effect is that you get a warning from a picky static analysis tool... thanks -- PMM
diff --git a/hw/arm/mainstone.c b/hw/arm/mainstone.c index 9402c84..223a4b1 100644 --- a/hw/arm/mainstone.c +++ b/hw/arm/mainstone.c @@ -76,7 +76,9 @@ static struct keymap map[0xE0] = { [0xc7] = {5,0}, /* Home */ [0x2a] = {5,1}, /* shift */ [0x39] = {5,2}, /* space */ +#if 0 /* always map to first column, row pair */ [0x39] = {5,3}, /* space */ +#endif [0x1c] = {5,5}, /* enter */ [0xc8] = {6,0}, /* up */ [0xd0] = {6,1}, /* down */
cgcc reported a duplicate initialisation. Mainstone includes a matrix keyboard where two different positions map to 'space'. QEMU uses the reversed mapping and cannot map 'space' to two different matrix positions. Signed-off-by: Stefan Weil <sw@weilnetz.de> --- Of course we could also randomly select one of the two matrix positions, but I assume that this is not necessary. I don't know any mainstone internals, so I had to look into the Linux code where I noticed some discrepancies which should be clarified: Extract from Linux: KEY(1, 5, KEY_LEFTSHIFT), KEY(2, 5, KEY_SPACE), KEY(3, 5, KEY_SPACE), KEY(4, 5, KEY_ENTER), KEY(5, 5, KEY_BACKSPACE), Extract from QEMU: [0x2a] = {5,1}, /* shift */ [0x39] = {5,2}, /* space */ [0x39] = {5,3}, /* space */ [0x1c] = {5,5}, /* enter */ It looks like the 'enter' key is not mapped correctly, and 'backspace' is missing. Regards, Stefan hw/arm/mainstone.c | 2 ++ 1 file changed, 2 insertions(+)