[U-Boot] [PATCH 1/2] usb: kbd: allow multibyte sequences to be put into ring buffer

[Heinrich Schuchardt]

On 02/23/2018 07:04 AM, Marek Vasut wrote:
> On 02/23/2018 06:50 AM, Heinrich Schuchardt wrote:
> [...]
>>>> When starting an EFI application the xterm escape sequences are
>>>> translated to EFI scan codes. See lib/efi_loader/efi_console.c
>>>
>>> So this is only usable if you have display connected to the board ?
>>
>> Why? We are talking about input.
>>
>> Neither local nor remote display is needed to hit the F10 key on a USB
>> keyboard.
> 
> What confused me was the constant usage of xterm control sequences to
> describe terminal control sequences. Now it makes sense.
> 
> [...]
> 
>>>>>>>> +    /* Copy to buffer */
>>>>>>>> +    for (i = 0; i < count; ++i) {
>>>>>>>> +        ++data->usb_in_pointer;
>>>>>>>> +        if (data->usb_in_pointer == USB_KBD_BUFFER_LEN)
>>>>>>>> +            data->usb_in_pointer = 0;
>>>>>>>> +        data->usb_kbd_buffer[data->usb_in_pointer] = buf[i];
>>>>>>>
>>>>>>> memcpy() ?
>>>>>>
>>>>>> Typically we copy only one byte. But escape sequences have a maximum
>>>>>> length of 8 bytes (e.g. CTRL+F8).
>>>>>>
>>>>>> We have to consider the case with wrap around. This would require two
>>>>>> memcpy() calls.
>>>>>>
>>>>>> The coding would neither get faster in the average nor less complex
>>>>>> using memcpy(). So let's keep it as it is.
>>>>>
>>>>> I suspect this block of code needs cleanup .
>>>>>
>>>>
>>>> Could you, please, give some indication of what you dislike.
>>>
>>> At least the part which looks like ad-hoc implementation of memcpy() ,
>>> any other cleanups are welcome.
>>>
>>
>> Do you really want that ugly monster below for typically copying a
>> single byte?
>>
>> As said it is slower, has more lines, and gives a larger executable.
>>
>> int remaining;
>>
>> /* Copy to buffer */
>> remaining = data->usb_in_pointer + count - USB_KBD_BUFFER_LEN;
>> if (remaining < 0)
>>      remaining = 0;
>> memcpy(&usb_kbd_buffer[data->usb_in_pointer], buf, count - remaining);
>> if (remaining)
>>      memcpy(usb_kbd_buffer, buf + count - remaining, remaining);
>> data->usb_in_pointer += count;
>> if (data->usb_in_pointer >= USB_KBD_BUFFER_LEN)
>>      data->usb_in_pointer -= USB_KBD_BUFFER_LEN;
> 
> Can the code be tweaked so it doesn't use the circular buffer, but a
> linear one , thus two memcpy()s are not needed ?

This would require copying the whole buffer with memcpy() when 
withdrawing a byte.

Somehow we tend to reinvent the wheel. Let's use lib/circbuf.c here.

Regards

Heinrich

> 
> The whole usb keyboard code is not nice and could use some cleanup.
>