qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 4/7] hw/gpio/nrf51_gpio: Add nRF51 GPIO peripher


From: Peter Maydell
Subject: Re: [Qemu-devel] [PATCH 4/7] hw/gpio/nrf51_gpio: Add nRF51 GPIO peripheral
Date: Thu, 16 Aug 2018 17:08:25 +0100

On 6 August 2018 at 11:01, Steffen Görtz <address@hidden> wrote:
> This adds a model of the nRF51 GPIO peripheral.
>
> Reference Manual: http://infocenter.nordicsemi.com/pdf/nRF51_RM_v3.0.pdf
>
> The nRF51 series microcontrollers support up to 32 GPIO pins in various 
> configurations.
> The pins can be used as input pins with pull-ups or pull-down.
> Furthermore, three different output driver modes per level are
> available (disconnected, standard, high-current).
>
> The GPIO-Peripheral has a mechanism for detecting level changes which is
> not featured in this model.
>
> Signed-off-by: Steffen Görtz <address@hidden>

I have a couple of style nits below but otherwise this looks good.

> +#define NRF51_GPIO_SIZE 0x1000
> +
> +#define NRF51_GPIO_REG_OUT          0x504
> +#define NRF51_GPIO_REG_OUTSET       0x508
> +#define NRF51_GPIO_REG_OUTCLR       0x50C
> +#define NRF51_GPIO_REG_IN           0x510
> +#define NRF51_GPIO_REG_DIR          0x514
> +#define NRF51_GPIO_REG_DIRSET       0x518
> +#define NRF51_GPIO_REG_DIRCLR       0x51C
> +#define NRF51_GPIO_REG_CNF_START    0x700
> +#define NRF51_GPIO_REG_CNF_END      0x77F
> +
> +#define GPIO_PULLDOWN 1
> +#define GPIO_PULLUP 3
> +
> +/**

"/**" is for doc-comment format comments. Generally we only
need those for functions that are global, not file-local ones.
If you want to do a doc-comment, then you want to do it in the
right syntax, with description of input arguments and result
(eg see include/qemu/bitops.h for some examples). If not, just
start with "/*".

> + * Check if the output driver is connected to the direction switch
> + * given the current configuration and logic level.
> + * It is not differentiated between standard and "high"(-power) drive modes.
> + */
> +static bool is_connected(uint32_t config, uint32_t level)
> +{
> +    bool state;
> +    uint32_t drive_config = extract32(config, 8, 3);
> +
> +    switch (drive_config) {
> +    case 0 ... 3:
> +        state = true;
> +        break;
> +    case 4 ... 5:
> +        state = level != 0;
> +        break;
> +    case 6 ... 7:
> +        state = level == 0;
> +        break;
> +    }
> +
> +    return state;
> +}
> +
> +static void update_output_irq(Nrf51GPIOState *s, size_t i,
> +                              bool connected, bool level)
> +{
> +    int64_t irq_level = connected ? level : -1;
> +    bool old_connected = extract32(s->old_out_connected, i, 1);
> +    bool old_level = extract32(s->old_out, i, 1);
> +
> +    if ((old_connected != connected) || (old_level != level)) {
> +        qemu_set_irq(s->output[i], irq_level);
> +        trace_nrf51_gpio_update_output_irq(i, irq_level);
> +    }
> +
> +    s->old_out = deposit32(s->old_out, i, 1, level);
> +    s->old_out_connected = deposit32(s->old_out_connected, i, 1, connected);
> +}
> +
> +static void update_state(Nrf51GPIOState *s)
> +{
> +    uint32_t pull;
> +    bool connected_out, dir, connected_in, out, input;
> +
> +    for (size_t i = 0; i < NRF51_GPIO_PINS; i++) {
> +        pull = extract32(s->cnf[i], 2, 2);
> +        dir = extract32(s->cnf[i], 0, 1);
> +        connected_in = extract32(s->in_mask, i, 1);
> +        out = extract32(s->out, i, 1);
> +        input = !extract32(s->cnf[i], 1, 1);
> +        connected_out = is_connected(s->cnf[i], out) && dir;
> +
> +        update_output_irq(s, i, connected_out, out);
> +
> +        /** Pin both driven externally and internally */

Inline comments like these should definitely not have the doc-comment
starting indicator.

> +        if (connected_out && connected_in) {
> +            qemu_log_mask(LOG_GUEST_ERROR, "GPIO pin %zu short circuited\n", 
> i);
> +        }

> diff --git a/hw/gpio/trace-events b/hw/gpio/trace-events
> new file mode 100644
> index 0000000000..11486b09b9
> --- /dev/null
> +++ b/hw/gpio/trace-events
> @@ -0,0 +1,7 @@
> +# See docs/devel/tracing.txt for syntax documentation.
> +
> +# hw/gpio/nrf51_gpio.c
> +nrf51_gpio_read(uint64_t offset, uint64_t r) "offset 0x%" PRIx64 " value 
> 0x%" PRIx64
> +nrf51_gpio_write(uint64_t offset, uint64_t value) "offset 0x%" PRIx64 " 
> value 0x%" PRIx64
> +nrf51_gpio_set(int64_t line, int64_t value) "line %" PRIi64 " value 0x%" 
> PRIi64
> +nrf51_gpio_update_output_irq(int64_t line, int64_t value) "line %" PRIi64 " 
> value 0x%" PRIi64
> \ No newline at end of file

You want to make sure there's a newline at the end of the file...


thanks
-- PMM



reply via email to

[Prev in Thread] Current Thread [Next in Thread]