Re: [Qemu-devel] [PATCH] mainstone: Fix duplicate array values for key 'space'

[Peter Maydell]

On 22 December 2013 14:11, Stefan Weil <address@hidden> 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 <address@hidden>
> ---
>
> 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