Re: [Qemu-devel] [PATCH v3 5/6] misc: add pca9552 LED blinker model

[Peter Maydell]

On 13 October 2017 at 15:28, Cédric Le Goater <address@hidden> wrote:
> Specs are available here :
>
>   https://www.nxp.com/docs/en/data-sheet/PCA9552.pdf
>
> This is a simple model supporting the basic registers for led and GPIO
> mode. The device also supports two blinking rates but not the model
> yet.
>
> Signed-off-by: Cédric Le Goater <address@hidden>
> ---
>
>  Changes since v2:
>
>  - removed comments on the I2C buffer size, but kept the array. I did
>    not want to rewrite the buffer handling
>
>  default-configs/arm-softmmu.mak |   1 +
>  hw/misc/Makefile.objs           |   1 +
>  hw/misc/pca9552.c               | 212 
> ++++++++++++++++++++++++++++++++++++++++
>  include/hw/misc/pca9552.h       |  32 ++++++
>  4 files changed, 246 insertions(+)
>  create mode 100644 hw/misc/pca9552.c
>  create mode 100644 include/hw/misc/pca9552.h
>
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index 5059d134c816..d868d1095a6c 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -16,6 +16,7 @@ CONFIG_TSC2005=y
>  CONFIG_LM832X=y
>  CONFIG_TMP105=y
>  CONFIG_TMP421=y
> +CONFIG_PCA9552=y
>  CONFIG_STELLARIS=y
>  CONFIG_STELLARIS_INPUT=y
>  CONFIG_STELLARIS_ENET=y
> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
> index e8f0a02f35af..e4e22880dbbc 100644
> --- a/hw/misc/Makefile.objs
> +++ b/hw/misc/Makefile.objs
> @@ -7,6 +7,7 @@ common-obj-$(CONFIG_SGA) += sga.o
>  common-obj-$(CONFIG_ISA_TESTDEV) += pc-testdev.o
>  common-obj-$(CONFIG_PCI_TESTDEV) += pci-testdev.o
>  common-obj-$(CONFIG_EDU) += edu.o
> +common-obj-$(CONFIG_PCA9552) += pca9552.o
>
>  common-obj-y += unimp.o
>
> diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
> new file mode 100644
> index 000000000000..22460f4c14fe
> --- /dev/null
> +++ b/hw/misc/pca9552.c
> @@ -0,0 +1,212 @@
> +/*
> + * PCA9552 I2C LED blinker
> + *
> + * Copyright (c) 2017, IBM Corporation.
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> + * later. See the COPYING file in the top-level directory.

Adding the url of the datasheet in the header comment
would also be useful.

> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "hw/hw.h"
> +#include "hw/misc/pca9552.h"
> +
> +#define PCA9552_INPUT0   0 /* read only input register 0 */
> +#define PCA9552_INPUT1   1 /* read only input register 1  */
> +#define PCA9552_PSC0     2 /* read/write frequency prescaler 0 */
> +#define PCA9552_PWM0     3 /* read/write PWM register 0 */
> +#define PCA9552_PSC1     4 /* read/write frequency prescaler 1 */
> +#define PCA9552_PWM1     5 /* read/write PWM register 1 */
> +#define PCA9552_LS0      6 /* read/write LED0 to LED3 selector */
> +#define PCA9552_LS1      7 /* read/write LED4 to LED7 selector */
> +#define PCA9552_LS2      8 /* read/write LED8 to LED11 selector */
> +#define PCA9552_LS3      9 /* read/write LED12 to LED15 selector */
> +
> +#define PCA9552_LED_ON   0x0
> +#define PCA9552_LED_OFF  0x1
> +#define PCA9552_LED_PWM0 0x2
> +#define PCA9552_LED_PWM1 0x3
> +
> +static uint8_t pca9552_pin_get_config(PCA9552State *s, int pin)
> +{
> +    uint8_t reg   = PCA9552_LS0 + (pin / 4);
> +    uint8_t shift = (pin % 4) << 1;
> +
> +    return (s->regs[reg] >> shift) & 0x3;

extract32() is probably more readable than handcoded
shift-and-mask.

> +}
> +
> +static void pca9552_update_pin_input(PCA9552State *s)
> +{
> +    int i;
> +
> +    for (i = 0; i < 16; i++) {
> +        uint8_t input_reg = PCA9552_INPUT0 + (i / 8);
> +        uint8_t input_shift = (i % 8);
> +        uint8_t config = pca9552_pin_get_config(s, i);
> +
> +        switch (config) {
> +        case PCA9552_LED_ON:
> +            s->regs[input_reg] |= 1 << input_shift;
> +            break;
> +        case PCA9552_LED_OFF:
> +            s->regs[input_reg] &= ~(1 << input_shift);
> +            break;
> +        case PCA9552_LED_PWM0:
> +        case PCA9552_LED_PWM1:
> +            /* ??? */
> +        default:
> +            break;
> +        }
> +    }
> +}
> +
> +static void pca9552_read(PCA9552State *s)
> +{
> +    uint8_t reg = s->pointer & 0xf;
> +
> +    s->len = 0;
> +
> +    switch (reg) {
> +    case PCA9552_INPUT0:
> +    case PCA9552_INPUT1:
> +    case PCA9552_PSC0:
> +    case PCA9552_PWM0:
> +    case PCA9552_PSC1:
> +    case PCA9552_PWM1:
> +    case PCA9552_LS0:
> +    case PCA9552_LS1:
> +    case PCA9552_LS2:
> +    case PCA9552_LS3:
> +        s->buf[s->len++] = s->regs[reg];
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: unexpected read to register 
> %d\n",
> +                      __func__, reg);
> +    }
> +}
> +
> +static void pca9552_write(PCA9552State *s)
> +{
> +    uint8_t reg = s->pointer & 0xf;
> +
> +    switch (reg) {
> +    case PCA9552_PSC0:
> +    case PCA9552_PWM0:
> +    case PCA9552_PSC1:
> +    case PCA9552_PWM1:
> +        s->regs[reg] = s->buf[0];
> +        break;
> +
> +    case PCA9552_LS0:
> +    case PCA9552_LS1:
> +    case PCA9552_LS2:
> +    case PCA9552_LS3:
> +        s->regs[reg] = s->buf[0];
> +        pca9552_update_pin_input(s);
> +        break;
> +
> +    case PCA9552_INPUT0:
> +    case PCA9552_INPUT1:
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: unexpected write to register 
> %d\n",
> +                      __func__, reg);
> +    }
> +}
> +
> +static int pca9552_recv(I2CSlave *i2c)
> +{
> +    PCA9552State *s = PCA9552(i2c);
> +
> +    if (s->len < sizeof(s->buf)) {
> +        return s->buf[s->len++];
> +    } else {
> +        return 0xff;
> +    }
> +}
> +
> +static int pca9552_send(I2CSlave *i2c, uint8_t data)
> +{
> +    PCA9552State *s = PCA9552(i2c);
> +
> +    if (s->len == 0) {
> +        s->pointer = data;
> +        s->len++;
> +    } else {
> +        if (s->len <= sizeof(s->buf)) {
> +            s->buf[s->len - 1] = data;
> +        }
> +        s->len++;
> +        pca9552_write(s);
> +    }
> +
> +    return 0;
> +}
> +
> +static int pca9552_event(I2CSlave *i2c, enum i2c_event event)
> +{
> +    PCA9552State *s = PCA9552(i2c);
> +
> +    if (event == I2C_START_RECV) {
> +        pca9552_read(s);
> +    }
> +
> +    s->len = 0;
> +    return 0;
> +}

Reading the data sheet, it doesn't look like this is
correctly implementing reads and writes of more than one byte.
If you look at figures 11, 12 and 13 the guest can:
 * read or write multiple registers at once with a
   single transaction with multiple bytes, using the
   autoincrement (AI) bit in the command byte
 * read or writes multiple bytes of data from a port
   configured for GPIO with a single transaction
   (in this case AI would be clear)

There's a clearer description in the application note:
https://www.nxp.com/docs/en/application-note/AN264.pdf
(on page 12).

I think to implement this you don't need buf[] at all
(and len is only there to distinguish "first byte" from
"not first byte").

Rather than calling pca9552_read() from pca9552_event()
you should call it from pca9552_recv(), which means that
it gets called for each byte requested and you don't
need to stuff the value into buf[] and then fish it
back out again.

Similarly, rather than pca9552_send() writing the data
into s->buf[] and then pca9552_write() fishing it out,
you can just pass it directly to pca9552_write().

In both cases you want to implement the "increment
pointer as required if AI bit is set" so multibyte
transfers step through the register set, rolling over
from 9 to 0.

I don't think the Linux driver bothers to use this, but it's
worth getting it right from the start because it affects how
we represent the device state and thus migration compat.

> +static const VMStateDescription pca9552_vmstate = {
> +    .name = "PCA9552",
> +    .version_id = 0,
> +    .minimum_version_id = 0,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8(len, PCA9552State),
> +        VMSTATE_UINT8(pointer, PCA9552State),
> +        VMSTATE_UINT8_ARRAY(buf, PCA9552State, 1),
> +        VMSTATE_UINT8_ARRAY(regs, PCA9552State, PCA9552_NR_REGS),
> +        VMSTATE_I2C_SLAVE(i2c, PCA9552State),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void pca9552_reset(DeviceState *dev)
> +{
> +    PCA9552State *s = PCA9552(dev);
> +
> +    s->regs[PCA9552_PSC0] = 0xFF;
> +    s->regs[PCA9552_PWM0] = 0x80;
> +    s->regs[PCA9552_PSC1] = 0xFF;
> +    s->regs[PCA9552_PWM1] = 0x80;
> +    s->regs[PCA9552_LS0] = 0x55; /* all OFF */
> +    s->regs[PCA9552_LS1] = 0x55;
> +    s->regs[PCA9552_LS2] = 0x55;
> +    s->regs[PCA9552_LS3] = 0x55;
> +
> +    pca9552_update_pin_input(s);

Don't we also need to reset the pointer and len fields?


thanks
-- PMM