Re: [PATCH v2 1/4] hw/gpio: add PCA6416 i2c GPIO expander

[Philippe Mathieu-Daudé]

Hi Titus,

On 8/2/23 23:43, Titus Rwantare wrote:
> The PCA6416 is an i2c device with 16 GPIOs.
>
> Reviewed-by: Hao Wu <wuhaotsh@google.com>
> Signed-off-by: Titus Rwantare <titusr@google.com>
> ---
>   hw/arm/Kconfig                  |   1 +
>   hw/gpio/Kconfig                 |   4 +
>   hw/gpio/meson.build             |   1 +
>   hw/gpio/pca_i2c_gpio.c          | 388 ++++++++++++++++++++++++++++++++
>   hw/gpio/trace-events            |   5 +
>   include/hw/gpio/pca_i2c_gpio.h  |  69 ++++++
>   tests/lcitool/libvirt-ci        |   2 +-
>   tests/qtest/meson.build         |   1 +
>   tests/qtest/pca_i2c_gpio-test.c | 169 ++++++++++++++
>   9 files changed, 639 insertions(+), 1 deletion(-)
>   create mode 100644 hw/gpio/pca_i2c_gpio.c
>   create mode 100644 include/hw/gpio/pca_i2c_gpio.h
>   create mode 100644 tests/qtest/pca_i2c_gpio-test.c
>
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index 2d157de9b8..1b533ddd76 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -418,6 +418,7 @@ config NPCM7XX
>       select SSI
>       select UNIMP
>       select PCA954X
> +    select PCA_I2C_GPIO

Shouldn't this be s/select/imply/? See docs/devel/kconfig.rst:

  Boards specify their constituent devices using ``imply`` and
  ``select``  directives.  A device should be listed under ``select``
  if the board cannot be started at all without it.  It should be
  listed under ``imply`` if (depending on the QEMU command line) the
  board may or may not be started without it.  Boards also default to
  false; they are  enabled by the ``default-configs/*.mak`` for the
  target they apply to.

Better to split this as another "hw/arm: Allow NPCM7xx machines to use
the PCA6416 i2c GPIO expander" patch.

> +/*
> + * compare new_output to curr_output and update irq to match new_output
> + *
> + * The Input port registers (registers 0 and 1) reflect the incoming logic
> + * levels of the pins, regardless of whether the pin is defined as an input or
> + * an output by the Configuration register.
> + */
> +static void pca_i2c_update_irqs(PCAGPIOState *ps)
> +{
> +    PCAGPIOClass *pc = PCA_I2C_GPIO_GET_CLASS(ps);
> +    uint16_t out_diff = ps->new_output ^ ps->curr_output;
> +    uint16_t in_diff = ps->new_input ^ ps->curr_input;
> +    uint16_t mask, pin_i;
> +
> +    if (in_diff || out_diff) {
> +        for (int i = 0; i < pc->num_pins; i++) {
> +            mask = BIT(i);
> +            /* pin must be configured as an output to be set here */
> +            if (out_diff & ~ps->config & mask) {
> +                pin_i = mask & ps->new_output;
> +                qemu_set_irq(ps->output[i], pin_i > 0);
> +                ps->curr_output &= ~mask;
> +                ps->curr_output |= pin_i;
> +            }
> +
> +            if (in_diff & mask) {
> +                ps->curr_input &= ~mask;
> +                ps->curr_input |= mask & ps->new_input;
> +            }
> +        }
> +        /* make diff = 0 */
> +        ps->new_input = ps->curr_input;
> +    }
> +}

> +static void pca_i2c_enter_reset(Object *obj, ResetType type)
> +{
> +    PCAGPIOState *ps = PCA_I2C_GPIO(obj);
> +    PCAGPIOClass *pc = PCA_I2C_GPIO_GET_CLASS(obj);
> +
> +    for (int i = 0; i < pc->num_pins; i++) {
> +        qemu_irq_lower(ps->output[i]);
> +    }
> +
> +    ps->polarity_inv = 0;
> +    ps->config = 0;
> +    ps->curr_input = 0;
> +    ps->curr_output = 0;
> +    ps->new_input = 0;
> +    ps->new_output = 0;
> +    ps->command = 0;
> +}
> +
> +static void pca_i2c_realize(DeviceState *dev, Error **errp)
> +{
> +    PCAGPIOState *ps = PCA_I2C_GPIO(dev);
> +    pca_i2c_update_irqs(ps);
> +}

pca_i2c_realize() occurs once before the reset() handler, so it
seems redundant. We can probably remove it.

> +static void pca_i2c_gpio_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
> +    ResettableClass *rc = RESETTABLE_CLASS(klass);
> +
> +    dc->realize = pca_i2c_realize;
> +    dc->vmsd = &vmstate_pca_i2c_gpio;
> +    rc->phases.enter = pca_i2c_enter_reset;
> +    k->event = pca_i2c_event;
> +}

> +static const TypeInfo pca_gpio_types[] = {
> +    {
> +    .name = TYPE_PCA_I2C_GPIO,
> +    .parent = TYPE_I2C_SLAVE,
> +    .instance_size = sizeof(PCAGPIOState),
> +    .instance_init = pca_i2c_gpio_init,
> +    .class_size = sizeof(PCAGPIOClass),
> +    .class_init = pca_i2c_gpio_class_init,
> +    .abstract = true,

Per CODING_STYLE this should be indented +4 spaces.

> +    },
> +    {
> +    .name = TYPE_PCA6416_GPIO,
> +    .parent = TYPE_PCA_I2C_GPIO,
> +    .class_init = pca6416_gpio_class_init,
> +    },
> +};


> +#define PCA6416_INPUT_PORT_0                 0x00 /* read */
> +#define PCA6416_INPUT_PORT_1                 0x01 /* read */
> +#define PCA6416_OUTPUT_PORT_0                0x02 /* read/write */
> +#define PCA6416_OUTPUT_PORT_1                0x03 /* read/write */
> +#define PCA6416_POLARITY_INVERSION_PORT_0    0x04 /* read/write */
> +#define PCA6416_POLARITY_INVERSION_PORT_1    0x05 /* read/write */
> +#define PCA6416_CONFIGURATION_PORT_0         0x06 /* read/write */
> +#define PCA6416_CONFIGURATION_PORT_1         0x07 /* read/write */

IIUC the registers are 16-bit but the I2CSlaveClass API forces you
to process 8-bit at a time. Possible simpler to use the extract /
deposit API:

    #define PCA6416_INPUT_PORT                 0x00 /* read */
    #define PCA6416_OUTPUT_PORT                0x02 /* read/write */

    static uint8_t pca6416_recv(I2CSlave *i2c)
    {
        PCAGPIOState *ps = PCA_I2C_GPIO(i2c);
        unsigned shift = (ps->command) & 1 ? 8 : 0;
        uint8_t data;

        switch (ps->command) {
        case PCA6416_INPUT_PORT:
            data = extract16(ps->curr_input, shift, 8);
            break;

    static int pca6416_send(I2CSlave *i2c, uint8_t data)
    {
        PCAGPIOState *ps = PCA_I2C_GPIO(i2c);
        unsigned shift = (ps->command) & 1 ? 0 : 8;
        ...
        switch (ps->command) {
        ...
        case PCA6416_OUTPUT_PORT:
            ps->new_output = deposit32(ps->new_output, shift 8,
                                       extract16(data, shift, 8));
            break;

Regards,

Phil.