Re: [Qemu-devel] [RFC 2/3] hw/char/nrf51_uart: Implement nRF51 SoC UART

[Peter Maydell]

On 1 June 2018 at 16:21, Julia Suvorova <address@hidden> wrote:
> On 31.05.2018 12:42, Stefan Hajnoczi wrote:
>> Please add a comment here:
>>
>>    /* The hardware has no transmit error reporting, so silently drop the
>> byte */
>>
>
> The QEMU code is inconsistent with comments. Some files are full of
> comments, some have only code. Is there any policy how to comment
> the files?

The only policy we have written down is that you should use /* */
even for one-line comments.
    /* one line comments like this */

We're inconsistent about how to format multiline comments, but
my preference for the arm parts of the codebase is:
    /* multi line comments look
     * like this
     */

For the actual content of comments:
 * comments generally should describe the 'why' rather than the 'what'
   (but not "why did we change this?", which goes in the commit message)
 * comments are helpful where the code looks wrong or strange
   (as an indication of "this is deliberate and this is why";
   Stefan's suggested comment above is in this category). If
   there's a way to write the code that doesn't look wrong then
   that's obviously preferable :-)
 * you don't need to comment things that are obvious from just
   reading the code
 * I prefer to err on the side of more comments rather than fewer
 * you don't need to replicate lots of info that's in the data sheet,
   but don't assume your readers know all the weird corners of the
   hardware behaviour inside-out, either
 * known missing or broken things can be commented with TODO or
   FIXME comments (but if you have too many of these for easy things
   we may ask you to just fix some of them :-))

Code reviewers will let you know if you've got the balance wrong.

As a special case:
 * any new function with global scope should have a doc-comment
   (starts with "/**") describing its purpose and API

Optionally:
 * for a few recent devices I thought it was useful to document
   in their header what the "QEMU interface" (what the various
   QOM properties, named GPIO outputs, inputs, etc are). You can
   see examples with 'grep "QEMU interface" include/'. If that seems
   helpful, feel free to have a comment like that; if not, skip it.

Overall advice: QEMU is a large code base, and it has both new
and well maintained parts and also very old parts that are
unloved and often use no-longer-recommended style or APIs.
When you're looking around for examples to copy the style of,
it's better to try to find something that's been added or
overhauled recently.

>>> +static int uart_can_receive(void *opaque)
>>> +{
>>> +    Nrf51UART *s = NRF51_UART(opaque);
>>> +
>>> +    return (s->rx_fifo_len < sizeof(s->rx_fifo));
>>
>> This is very subtle: this function returns the number of bytes that can
>> be read.  This expression looks like a boolean return value but actually
>> relies on the implicit int cast (false -> 0 bytes, true -> 1 byte).
>>
>> Please write it so it doesn't look like a boolean return value:
>>
>>    if (s->rx_fifo_len >= sizeof(s->rx_fifo)) {
>>        return 0;
>>    }
>>
>>    return 1 /* byte */;
>>
>> Alternatively, you could handle more than 1 byte:
>>
>>    return sizeof(s->rx_fifo) - s->rx_fifo_len;
>>
>> but now uart_receive() needs to handle up to sizeof(s->rx_fifo) bytes of
>> data in a single call.
>
> I will rewrite uart_receive function to accept many bytes at once.

Stefan, I was wondering about this when I read this patch.
What is the API for the can_receive and receive functions?
There's no documentation of the semantics either with the
IOReadHandler or IOCanReadHandler typedefs in main-loop.h,
or in the doc comment for qemu_chr_fe_set_handlers.

I'm guessing that the answer is "your can_read handler should
return the number of bytes you can accept, and a subsequent call
to the read handler then must accept that many bytes, it can't
change its mind and only accept a smaller number" (with some sort
of guarantee by the caller that it won't let guest execution or
other simulation-changes things between the call to can_receive
and receive) ?

(Similarly we don't document the semantics for the NetClientInfo
can_receive and receive functions.)

>>> +}
>>> +
>>> +static void nrf51_uart_realize(DeviceState *dev, Error **errp)
>>> +{
>>> +    Nrf51UART *s = NRF51_UART(dev);
>>> +
>>> +    qemu_chr_fe_set_handlers(&s->chr, uart_can_receive, uart_receive,
>>> +                             NULL, NULL, s, NULL, true);
>>> +}
>>> +
>>> +static void nrf51_uart_init(Object *obj)
>>> +{
>>> +    Nrf51UART *s = NRF51_UART(obj);
>>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>>> +
>>> +    memory_region_init_io(&s->mmio, obj, &uart_ops, s,
>>> +                          "nrf51_soc.uart", 0x1000);
>>
>> s/0x1000/UART_SIZE/
>
> Should I just add a new define or move the existing one from nrf51_soc.c to
> some header (or any other way to pass the UART_SIZE)?

Usual thing here is that the uart's own header file defines this sort
of constant, and then the SoC's .c file includes that header file
and uses the constant.

thanks
-- PMM