[PATCH v2 0/4] PCA I2C GPIO expanders

[PATCH v2 0/4] PCA I2C GPIO expanders

[Titus Rwantare]

This patch series contains a set of i2c GPIO expanders,
with support for 4, 8, and 16 GPIO connections.

The devices are configured as GPIO *inputs by default, but can have pins
configured to be inputs with qmp commands.

For example, the following snippet in a board file for a system,
configures a 16 bit pca6416 to have pins 8-11 as inputs, then asserts
them.

    dev = DEVICE(i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 3), "pca6416", 0x72));
    object_property_set_uint(OBJECT(dev), "gpio_config", 0x0F00, &error_abort);
    object_property_set_uint(OBJECT(dev), "gpio_input", 0x0F00, &error_abort);

We currently use these to test hardware presence and LEDs in simulation.

Thanks

Since v1:
- addressed comments
- fixed typos in commit messages

Titus Rwantare (4):
  hw/gpio: add PCA6416 i2c GPIO expander
  hw/gpio: add PCA9538 8-bit GPIO expander
  hw/gpio: add PCA9536 i2c gpio expander
  hw/i2c: add canonical path to i2c event traces

 hw/arm/Kconfig                  |   1 +
 hw/gpio/Kconfig                 |   4 +
 hw/gpio/meson.build             |   1 +
 hw/gpio/pca_i2c_gpio.c          | 500 ++++++++++++++++++++++++++++++++
 hw/gpio/trace-events            |   5 +
 hw/i2c/core.c                   |   8 +-
 hw/i2c/trace-events             |   2 +-
 include/hw/gpio/pca_i2c_gpio.h  |  78 +++++
 tests/lcitool/libvirt-ci        |   2 +-
 tests/qtest/meson.build         |   1 +
 tests/qtest/pca_i2c_gpio-test.c | 169 +++++++++++
 11 files changed, 766 insertions(+), 5 deletions(-)
 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

-- 
2.39.1.581.gbfd45094c4-goog

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

[Titus Rwantare]

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
 
 config FSL_IMX25
     bool
diff --git a/hw/gpio/Kconfig b/hw/gpio/Kconfig
index d2cf3accc8..1c01d9744a 100644
--- a/hw/gpio/Kconfig
+++ b/hw/gpio/Kconfig
@@ -16,3 +16,7 @@ config GPIO_PWR
 
 config SIFIVE_GPIO
     bool
+
+config PCA_I2C_GPIO
+    bool
+    depends on I2C
diff --git a/hw/gpio/meson.build b/hw/gpio/meson.build
index b726e6d27a..1e5b602002 100644
--- a/hw/gpio/meson.build
+++ b/hw/gpio/meson.build
@@ -12,3 +12,4 @@ softmmu_ss.add(when: 'CONFIG_OMAP', if_true: files('omap_gpio.c'))
 softmmu_ss.add(when: 'CONFIG_RASPI', if_true: files('bcm2835_gpio.c'))
 softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('aspeed_gpio.c'))
 softmmu_ss.add(when: 'CONFIG_SIFIVE_GPIO', if_true: files('sifive_gpio.c'))
+softmmu_ss.add(when: 'CONFIG_PCA_I2C_GPIO', if_true: files('pca_i2c_gpio.c'))
diff --git a/hw/gpio/pca_i2c_gpio.c b/hw/gpio/pca_i2c_gpio.c
new file mode 100644
index 0000000000..434a759453
--- /dev/null
+++ b/hw/gpio/pca_i2c_gpio.c
@@ -0,0 +1,388 @@
+/*
+ * NXP PCA I2C GPIO Expanders
+ *
+ * Low-voltage translating 16-bit I2C/SMBus GPIO expander with interrupt output,
+ * reset, and configuration registers
+ *
+ * Datasheet: https://www.nxp.com/docs/en/data-sheet/PCA6416A.pdf
+ *
+ * Copyright 2023 Google LLC
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * To assert some input pins before boot, use the following in the board file of
+ * the machine:
+ *      object_property_set_uint(Object *obj, const char *name,
+ *                               uint64_t value, Error **errp);
+ * specifying name as "gpio_config" and the value as a bitfield of the inputs
+ * e.g. for the pca6416, a value of 0xFFF0, configures pins 0-3 as outputs and
+ * 4-15 as inputs.
+ * Then using name "gpio_input" with value "0x0F00" would raise GPIOs 8-11.
+ *
+ * This value can also be set at runtime through qmp externally, or by
+ * writing to the config register using i2c. The guest driver should generally
+ * control the config register, but exposing it via qmp allows external testing.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "hw/gpio/pca_i2c_gpio.h"
+#include "hw/irq.h"
+#include "hw/qdev-properties.h"
+#include "migration/vmstate.h"
+#include "qapi/visitor.h"
+#include "qemu/log.h"
+#include "qemu/module.h"
+#include "trace.h"
+
+/*
+ * 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_irq_handler(void *opaque, int n, int level)
+{
+    PCAGPIOState *ps = opaque;
+    PCAGPIOClass *pc = PCA_I2C_GPIO_GET_CLASS(opaque);
+    uint16_t mask = BIT(n);
+
+    g_assert(n < pc->num_pins);
+    g_assert(n >= 0);
+
+    ps->new_input &= ~mask;
+
+    if (level > 0) {
+        ps->new_input |= BIT(n);
+    }
+
+    pca_i2c_update_irqs(ps);
+}
+
+/* slave to master */
+static uint8_t pca6416_recv(I2CSlave *i2c)
+{
+    PCAGPIOState *ps = PCA_I2C_GPIO(i2c);
+    uint8_t data;
+
+    switch (ps->command) {
+    case PCA6416_INPUT_PORT_0:
+        data = ps->curr_input;
+        break;
+
+    case PCA6416_INPUT_PORT_1:
+        data = ps->curr_input >> 8;
+        break;
+
+    /*
+     * i2c reads to the output registers reflect the values written
+     * NOT the actual values of the gpios
+     */
+    case PCA6416_OUTPUT_PORT_0:
+        data = ps->new_output;
+        break;
+
+    case PCA6416_OUTPUT_PORT_1:
+        data = ps->new_output >> 8;
+        break;
+
+    case PCA6416_POLARITY_INVERSION_PORT_0:
+        data = ps->polarity_inv;
+        break;
+
+    case PCA6416_POLARITY_INVERSION_PORT_1:
+        data = ps->polarity_inv >> 8;
+        break;
+
+    case PCA6416_CONFIGURATION_PORT_0:
+        data = ps->config;
+        break;
+
+    case PCA6416_CONFIGURATION_PORT_1:
+        data = ps->config >> 8;
+        break;
+
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: reading from unsupported register 0x%02x\n",
+                      __func__, ps->command);
+        data = 0xFF;
+        break;
+    }
+
+    trace_pca_i2c_recv(DEVICE(ps)->canonical_path, ps->command, data);
+    return data;
+}
+
+/* master to slave */
+static int pca6416_send(I2CSlave *i2c, uint8_t data)
+{
+    PCAGPIOState *ps = PCA_I2C_GPIO(i2c);
+    if (ps->i2c_cmd) {
+        ps->command = data;
+        ps->i2c_cmd = false;
+        return 0;
+    }
+
+    trace_pca_i2c_send(DEVICE(ps)->canonical_path, ps->command, data);
+
+    switch (ps->command) {
+    case PCA6416_INPUT_PORT_0:
+    case PCA6416_INPUT_PORT_1:
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: writing to read only reg: 0x%02x\n",
+                      __func__, ps->command);
+        break;
+
+    case PCA6416_OUTPUT_PORT_0:
+        ps->new_output &= 0xFF00;
+        ps->new_output |= data;
+        break;
+
+    case PCA6416_OUTPUT_PORT_1:
+        ps->new_output &= 0xFF;
+        ps->new_output |= data << 8;
+        break;
+
+    case PCA6416_POLARITY_INVERSION_PORT_0:
+        ps->polarity_inv &= 0xFF00;
+        ps->polarity_inv |= data;
+        break;
+
+    case PCA6416_POLARITY_INVERSION_PORT_1:
+        ps->polarity_inv &= 0xFF;
+        ps->polarity_inv |= data << 8;
+        break;
+
+    case PCA6416_CONFIGURATION_PORT_0:
+        ps->config &= 0xFF00;
+        ps->config |= data;
+        break;
+
+    case PCA6416_CONFIGURATION_PORT_1:
+        ps->config &= 0xFF;
+        ps->config |= data << 8;
+        break;
+
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: writing to unsupported register\n",
+                      __func__);
+        return -1;
+    }
+
+    pca_i2c_update_irqs(ps);
+    return 0;
+}
+
+static int pca_i2c_event(I2CSlave *i2c, enum i2c_event event)
+{
+    PCAGPIOState *ps = PCA_I2C_GPIO(i2c);
+
+    switch (event) {
+    case I2C_START_RECV:
+        trace_pca_i2c_event(DEVICE(ps)->canonical_path, "START_RECV");
+        break;
+
+    case I2C_START_SEND:
+        trace_pca_i2c_event(DEVICE(ps)->canonical_path, "START_SEND");
+        ps->i2c_cmd = true;
+        break;
+
+    case I2C_FINISH:
+        trace_pca_i2c_event(DEVICE(ps)->canonical_path, "FINISH");
+        break;
+
+    case I2C_NACK:
+        trace_pca_i2c_event(DEVICE(ps)->canonical_path, "NACK");
+        break;
+
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: %s: unknown event 0x%x\n",
+                      DEVICE(ps)->canonical_path, __func__, event);
+        return -1;
+    }
+
+    return 0;
+}
+
+static void pca_i2c_config_get(Object *obj, Visitor *v, const char *name,
+                                void *opaque, Error **errp)
+{
+    PCAGPIOState *ps = PCA_I2C_GPIO(obj);
+    visit_type_uint16(v, name, &ps->config, errp);
+}
+
+static void pca_i2c_config_set(Object *obj, Visitor *v, const char *name,
+                                void *opaque, Error **errp)
+{
+    PCAGPIOState *ps = PCA_I2C_GPIO(obj);
+    if (!visit_type_uint16(v, name, &ps->config, errp)) {
+        return;
+    }
+    pca_i2c_update_irqs(ps);
+}
+
+
+static void pca_i2c_input_get(Object *obj, Visitor *v, const char *name,
+                                void *opaque, Error **errp)
+{
+    PCAGPIOState *ps = PCA_I2C_GPIO(obj);
+    visit_type_uint16(v, name, &ps->curr_input, errp);
+}
+
+static void pca_i2c_input_set(Object *obj, Visitor *v, const char *name,
+                                void *opaque, Error **errp)
+{
+    PCAGPIOState *ps = PCA_I2C_GPIO(obj);
+    if (!visit_type_uint16(v, name, &ps->new_input, errp)) {
+        return;
+    }
+    pca_i2c_update_irqs(ps);
+}
+
+static void pca_i2c_output_get(Object *obj, Visitor *v, const char *name,
+                                void *opaque, Error **errp)
+{
+    PCAGPIOState *ps = PCA_I2C_GPIO(obj);
+    visit_type_uint16(v, name, &ps->curr_output, errp);
+}
+
+static void pca_i2c_output_set(Object *obj, Visitor *v, const char *name,
+                                void *opaque, Error **errp)
+{
+    PCAGPIOState *ps = PCA_I2C_GPIO(obj);
+    if (!visit_type_uint16(v, name, &ps->new_output, errp)) {
+        return;
+    }
+    pca_i2c_update_irqs(ps);
+}
+
+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);
+}
+
+static const VMStateDescription vmstate_pca_i2c_gpio = {
+    .name = TYPE_PCA_I2C_GPIO,
+    .version_id = 0,
+    .minimum_version_id = 0,
+    .fields = (VMStateField[]) {
+        VMSTATE_I2C_SLAVE(parent, PCAGPIOState),
+        VMSTATE_UINT16(polarity_inv, PCAGPIOState),
+        VMSTATE_UINT16(config, PCAGPIOState),
+        VMSTATE_UINT16(curr_input, PCAGPIOState),
+        VMSTATE_UINT16(curr_output, PCAGPIOState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void pca6416_gpio_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
+    PCAGPIOClass *pc = PCA_I2C_GPIO_CLASS(klass);
+
+    dc->desc = "PCA6416 16-bit I/O expander";
+    pc->num_pins = PCA6416_NUM_PINS;
+
+    k->recv = pca6416_recv;
+    k->send = pca6416_send;
+}
+
+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 void pca_i2c_gpio_init(Object *obj)
+{
+    PCAGPIOState *ps = PCA_I2C_GPIO(obj);
+    PCAGPIOClass *pc = PCA_I2C_GPIO_GET_CLASS(obj);
+    DeviceState *dev = DEVICE(obj);
+
+    object_property_add(obj, "gpio_input", "uint16",
+                        pca_i2c_input_get,
+                        pca_i2c_input_set, NULL, NULL);
+    object_property_add(obj, "gpio_output", "uint16",
+                        pca_i2c_output_get,
+                        pca_i2c_output_set, NULL, NULL);
+    object_property_add(obj, "gpio_config", "uint16",
+                        pca_i2c_config_get,
+                        pca_i2c_config_set, NULL, NULL);
+    qdev_init_gpio_in(dev, pca_i2c_irq_handler, pc->num_pins);
+    qdev_init_gpio_out(dev, ps->output, pc->num_pins);
+}
+
+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,
+    },
+    {
+    .name = TYPE_PCA6416_GPIO,
+    .parent = TYPE_PCA_I2C_GPIO,
+    .class_init = pca6416_gpio_class_init,
+    },
+};
+
+DEFINE_TYPES(pca_gpio_types);
diff --git a/hw/gpio/trace-events b/hw/gpio/trace-events
index 9736b362ac..3fb0685096 100644
--- a/hw/gpio/trace-events
+++ b/hw/gpio/trace-events
@@ -31,3 +31,8 @@ sifive_gpio_update_output_irq(int64_t line, int64_t value) "line %" PRIi64 " val
 # aspeed_gpio.c
 aspeed_gpio_read(uint64_t offset, uint64_t value) "offset: 0x%" PRIx64 " value 0x%" PRIx64
 aspeed_gpio_write(uint64_t offset, uint64_t value) "offset: 0x%" PRIx64 " value 0x%" PRIx64
+
+# pca_i2c_gpio.c
+pca_i2c_event(const char *id, const char *event) "%s: %s"
+pca_i2c_recv(const char *id, uint8_t cmd, uint8_t data) "%s cmd: 0x%" PRIx8 " data 0x%" PRIx8
+pca_i2c_send(const char *id, uint8_t cmd, uint8_t data) "%s cmd: 0x%" PRIx8 " data 0x%" PRIx8
diff --git a/include/hw/gpio/pca_i2c_gpio.h b/include/hw/gpio/pca_i2c_gpio.h
new file mode 100644
index 0000000000..99322959e1
--- /dev/null
+++ b/include/hw/gpio/pca_i2c_gpio.h
@@ -0,0 +1,69 @@
+/*
+ * NXP PCA6416A
+ * Low-voltage translating 16-bit I2C/SMBus GPIO expander with interrupt output,
+ * reset, and configuration registers
+ *
+ * Datasheet: https://www.nxp.com/docs/en/data-sheet/PCA6416A.pdf
+ *
+ * Note: Polarity inversion emulation not implemented
+ *
+ * Copyright 2021 Google LLC
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#ifndef PCA_I2C_GPIO_H
+#define PCA_I2C_GPIO_H
+
+#include "hw/i2c/i2c.h"
+#include "qom/object.h"
+
+#define PCA_I2C_MAX_PINS                     16
+#define PCA6416_NUM_PINS                     16
+
+typedef struct PCAGPIOClass {
+    I2CSlaveClass parent;
+
+    uint8_t num_pins;
+} PCAGPIOClass;
+
+typedef struct PCAGPIOState {
+    I2CSlave parent;
+
+    uint16_t polarity_inv;
+    uint16_t config;
+
+    /* the values of the gpio pins are mirrored in these integers */
+    uint16_t curr_input;
+    uint16_t curr_output;
+    uint16_t new_input;
+    uint16_t new_output;
+
+    /*
+     * Note that these outputs need to be consumed by some other input
+     * to be useful, qemu ignores writes to disconnected gpio pins
+     */
+    qemu_irq output[PCA_I2C_MAX_PINS];
+
+    /* i2c transaction info */
+    uint8_t command;
+    bool i2c_cmd;
+
+} PCAGPIOState;
+
+#define TYPE_PCA_I2C_GPIO "pca_i2c_gpio"
+OBJECT_DECLARE_TYPE(PCAGPIOState, PCAGPIOClass, PCA_I2C_GPIO)
+
+#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 */
+
+#define PCA_I2C_CONFIG_DEFAULT               0
+
+#define TYPE_PCA6416_GPIO "pca6416"
+
+#endif
diff --git a/tests/lcitool/libvirt-ci b/tests/lcitool/libvirt-ci
index 319a534c22..e3eb28cf2e 160000
--- a/tests/lcitool/libvirt-ci
+++ b/tests/lcitool/libvirt-ci
@@ -1 +1 @@
-Subproject commit 319a534c220f53fc8670254cac25d6f662c82112
+Subproject commit e3eb28cf2e17fbcf7fe7e19505ee432b8ec5bbb5
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index e97616d327..49f406af6b 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -241,6 +241,7 @@ qos_test_ss.add(
   'ne2000-test.c',
   'tulip-test.c',
   'nvme-test.c',
+  'pca_i2c_gpio-test.c',
   'pca9552-test.c',
   'pci-test.c',
   'pcnet-test.c',
diff --git a/tests/qtest/pca_i2c_gpio-test.c b/tests/qtest/pca_i2c_gpio-test.c
new file mode 100644
index 0000000000..041ca34e22
--- /dev/null
+++ b/tests/qtest/pca_i2c_gpio-test.c
@@ -0,0 +1,169 @@
+/*
+ * QTest for PCA I2C GPIO expanders
+ *
+ * Copyright 2021 Google LLC
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "hw/gpio/pca_i2c_gpio.h"
+#include "libqtest-single.h"
+#include "libqos/i2c.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/qmp/qnum.h"
+#include "qemu/bitops.h"
+
+
+#define TEST_ID "pca_i2c_gpio-test"
+#define PCA_CONFIG_BYTE     0x55
+#define PCA_CONFIG_WORD     0x5555
+
+static uint16_t qmp_pca_gpio_get(const char *id, const char *property)
+{
+    QDict *response;
+    uint16_t ret;
+    response = qmp("{ 'execute': 'qom-get', 'arguments': { 'path': %s, "
+                   "'property': %s } }", id, property);
+    g_assert(qdict_haskey(response, "return"));
+    ret = qnum_get_uint(qobject_to(QNum, qdict_get(response, "return")));
+    qobject_unref(response);
+    return ret;
+}
+
+static void qmp_pca_gpio_set(const char *id, const char *property,
+                             uint16_t value)
+{
+    QDict *response;
+
+    response = qmp("{ 'execute': 'qom-set', 'arguments': { 'path': %s, "
+                   "'property': %s, 'value': %u } }",
+                   id, property, value);
+    g_assert(qdict_haskey(response, "return"));
+    qobject_unref(response);
+}
+
+static void test_set_input(void *obj, void *data, QGuestAllocator *alloc)
+{
+    QI2CDevice *i2cdev = (QI2CDevice *)obj;
+    uint8_t value;
+    uint16_t qmp_value;
+    /* configure pins to be inputs */
+    i2c_set8(i2cdev, PCA6416_CONFIGURATION_PORT_0, 0xFF);
+    i2c_set8(i2cdev, PCA6416_CONFIGURATION_PORT_1, 0xFF);
+
+    qmp_pca_gpio_set(TEST_ID, "gpio_input", 0xAAAA);
+    value = i2c_get8(i2cdev, PCA6416_INPUT_PORT_0);
+    g_assert_cmphex(value, ==, 0xAA);
+    value = i2c_get8(i2cdev, PCA6416_INPUT_PORT_1);
+    g_assert_cmphex(value, ==, 0xAA);
+
+    qmp_value = qmp_pca_gpio_get(TEST_ID, "gpio_input");
+    g_assert_cmphex(qmp_value, ==, 0xAAAA);
+}
+static void test_config(void *obj, void *data, QGuestAllocator *alloc)
+{
+
+    QI2CDevice *i2cdev = (QI2CDevice *)obj;
+    uint8_t value;
+    uint16_t qmp_value;
+    /* configure half the pins to be inputs */
+    i2c_set8(i2cdev, PCA6416_CONFIGURATION_PORT_0, PCA_CONFIG_BYTE);
+    i2c_set8(i2cdev, PCA6416_CONFIGURATION_PORT_1, PCA_CONFIG_BYTE);
+    value = i2c_get8(i2cdev, PCA6416_CONFIGURATION_PORT_0);
+    g_assert_cmphex(value, ==, PCA_CONFIG_BYTE);
+
+    value = i2c_get8(i2cdev, PCA6416_CONFIGURATION_PORT_1);
+    g_assert_cmphex(value, ==, PCA_CONFIG_BYTE);
+
+    /* the pins that match the config should be set, the rest are undef */
+    qmp_pca_gpio_set(TEST_ID, "gpio_input", 0xFFFF);
+    value = i2c_get8(i2cdev, PCA6416_INPUT_PORT_0);
+    g_assert_cmphex(value & PCA_CONFIG_BYTE, ==, 0x55);
+    value = i2c_get8(i2cdev, PCA6416_INPUT_PORT_1);
+    g_assert_cmphex(value & PCA_CONFIG_BYTE, ==, 0x55);
+    qmp_value = qmp_pca_gpio_get(TEST_ID, "gpio_input");
+    g_assert_cmphex(qmp_value & PCA_CONFIG_WORD, ==, 0x5555);
+
+    /*
+     * i2c will return the value written to the output register, not the values
+     * of the output pins, so we check only the configured pins
+     */
+    qmp_pca_gpio_set(TEST_ID, "gpio_output", 0xFFFF);
+    value = i2c_get8(i2cdev, PCA6416_OUTPUT_PORT_0);
+    g_assert_cmphex(value & ~PCA_CONFIG_BYTE, ==, 0xAA);
+    value = i2c_get8(i2cdev, PCA6416_OUTPUT_PORT_1);
+    g_assert_cmphex(value & ~PCA_CONFIG_BYTE, ==, 0xAA);
+
+    qmp_value = qmp_pca_gpio_get(TEST_ID, "gpio_output");
+    g_assert_cmphex(qmp_value & ~PCA_CONFIG_WORD, ==, 0xAAAA);
+}
+
+static void test_set_output(void *obj, void *data, QGuestAllocator *alloc)
+{
+    QI2CDevice *i2cdev = (QI2CDevice *)obj;
+    uint8_t value;
+    uint16_t qmp_value;
+    /* configure pins to be outputs */
+    i2c_set8(i2cdev, PCA6416_CONFIGURATION_PORT_0, 0);
+    i2c_set8(i2cdev, PCA6416_CONFIGURATION_PORT_1, 0);
+
+    qmp_pca_gpio_set(TEST_ID, "gpio_output", 0x5555);
+    value = i2c_get8(i2cdev, PCA6416_OUTPUT_PORT_0);
+    g_assert_cmphex(value, ==, 0x55);
+    value = i2c_get8(i2cdev, PCA6416_OUTPUT_PORT_1);
+    g_assert_cmphex(value, ==, 0x55);
+
+    qmp_value = qmp_pca_gpio_get(TEST_ID, "gpio_output");
+    g_assert_cmphex(qmp_value, ==, 0x5555);
+}
+
+static void test_tx_rx(void *obj, void *data, QGuestAllocator *alloc)
+{
+    QI2CDevice *i2cdev = (QI2CDevice *)obj;
+    uint8_t value;
+
+    i2c_set8(i2cdev, PCA6416_CONFIGURATION_PORT_0, 0xFF);
+    i2c_set8(i2cdev, PCA6416_CONFIGURATION_PORT_1, 0xFF);
+    i2c_set8(i2cdev, PCA6416_POLARITY_INVERSION_PORT_0, 0);
+    i2c_set8(i2cdev, PCA6416_POLARITY_INVERSION_PORT_1, 0);
+
+    value = i2c_get8(i2cdev, PCA6416_CONFIGURATION_PORT_0);
+    g_assert_cmphex(value, ==, 0xFF);
+
+    value = i2c_get8(i2cdev, PCA6416_CONFIGURATION_PORT_1);
+    g_assert_cmphex(value, ==, 0xFF);
+
+    value = i2c_get8(i2cdev, PCA6416_POLARITY_INVERSION_PORT_0);
+    g_assert_cmphex(value, ==, 0);
+
+    value = i2c_get8(i2cdev, PCA6416_POLARITY_INVERSION_PORT_1);
+    g_assert_cmphex(value, ==, 0);
+
+    i2c_set8(i2cdev, PCA6416_CONFIGURATION_PORT_0, 0xAB);
+    value = i2c_get8(i2cdev, PCA6416_CONFIGURATION_PORT_0);
+    g_assert_cmphex(value, ==, 0xAB);
+
+    i2c_set8(i2cdev, PCA6416_CONFIGURATION_PORT_1, 0xBC);
+    value = i2c_get8(i2cdev, PCA6416_CONFIGURATION_PORT_1);
+    g_assert_cmphex(value, ==, 0xBC);
+
+}
+
+static void pca_i2c_gpio_register_nodes(void)
+{
+    QOSGraphEdgeOptions opts = {
+        .extra_device_opts = "id=" TEST_ID ",address=0x78"
+    };
+    add_qi2c_address(&opts, &(QI2CAddress) { 0x78 });
+    g_test_set_nonfatal_assertions();
+
+    qos_node_create_driver("pca6416", i2c_device_create);
+    qos_node_consumes("pca6416", "i2c-bus", &opts);
+
+    qos_add_test("tx-rx", "pca6416", test_tx_rx, NULL);
+    qos_add_test("set_output_gpio", "pca6416", test_set_output, NULL);
+    qos_add_test("set_input_gpio", "pca6416", test_set_input, NULL);
+    qos_add_test("follow_gpio_config", "pca6416", test_config, NULL);
+}
+libqos_init(pca_i2c_gpio_register_nodes);
-- 
2.39.1.581.gbfd45094c4-goog

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.

[PATCH v2 2/4] hw/gpio: add PCA9538 8-bit GPIO expander

[Titus Rwantare]

  The 8-bit expander has different register offsets than the 16-bit one,
  making them incompatible.

Reviewed-by: Hao Wu <wuhaotsh@google.com>
Signed-off-by: Titus Rwantare <titusr@google.com>
---
 hw/gpio/pca_i2c_gpio.c         | 94 ++++++++++++++++++++++++++++++++++
 include/hw/gpio/pca_i2c_gpio.h |  7 +++
 2 files changed, 101 insertions(+)

diff --git a/hw/gpio/pca_i2c_gpio.c b/hw/gpio/pca_i2c_gpio.c
index 434a759453..fa69523556 100644
--- a/hw/gpio/pca_i2c_gpio.c
+++ b/hw/gpio/pca_i2c_gpio.c
@@ -143,6 +143,41 @@ static uint8_t pca6416_recv(I2CSlave *i2c)
     return data;
 }
 
+/* slave to master */
+static uint8_t pca9538_recv(I2CSlave *i2c)
+{
+    PCAGPIOState *ps = PCA_I2C_GPIO(i2c);
+    uint8_t data;
+
+    switch (ps->command) {
+    case PCA9538_INPUT_PORT:
+        data = ps->curr_input;
+        break;
+
+    case PCA9538_OUTPUT_PORT:
+        data = ps->new_output;
+        break;
+
+    case PCA9538_POLARITY_INVERSION_PORT:
+        data = ps->polarity_inv;
+        break;
+
+    case PCA9538_CONFIGURATION_PORT:
+        data = ps->config;
+        break;
+
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: reading from unsupported register 0x%02x",
+                      __func__, ps->command);
+        data = 0xFF;
+        break;
+    }
+
+    trace_pca_i2c_recv(DEVICE(ps)->canonical_path, ps->command, data);
+    return data;
+}
+
 /* master to slave */
 static int pca6416_send(I2CSlave *i2c, uint8_t data)
 {
@@ -203,6 +238,47 @@ static int pca6416_send(I2CSlave *i2c, uint8_t data)
     return 0;
 }
 
+/* master to slave */
+static int pca9538_send(I2CSlave *i2c, uint8_t data)
+{
+    PCAGPIOState *ps = PCA_I2C_GPIO(i2c);
+    if (ps->i2c_cmd) {
+        ps->command = data;
+        ps->i2c_cmd = false;
+        return 0;
+    }
+
+    trace_pca_i2c_send(DEVICE(ps)->canonical_path, ps->command, data);
+
+    switch (ps->command) {
+    case PCA9538_INPUT_PORT:
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: writing to read only reg: 0x%02x",
+                      __func__, ps->command);
+        break;
+    case PCA9538_OUTPUT_PORT:
+        ps->new_output = data;
+        break;
+
+    case PCA9538_POLARITY_INVERSION_PORT:
+        ps->polarity_inv = data;
+        break;
+
+    case PCA9538_CONFIGURATION_PORT:
+        ps->config = data;
+        break;
+
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: writing to unsupported register\n",
+                      __func__);
+        return -1;
+    }
+
+    pca_i2c_update_irqs(ps);
+
+    return 0;
+}
+
 static int pca_i2c_event(I2CSlave *i2c, enum i2c_event event)
 {
     PCAGPIOState *ps = PCA_I2C_GPIO(i2c);
@@ -337,6 +413,19 @@ static void pca6416_gpio_class_init(ObjectClass *klass, void *data)
     k->send = pca6416_send;
 }
 
+static void pca9538_gpio_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
+    PCAGPIOClass *pc = PCA_I2C_GPIO_CLASS(klass);
+
+    dc->desc = "PCA9538 8-bit I/O expander";
+    pc->num_pins = PCA9538_NUM_PINS;
+
+    k->recv = pca9538_recv;
+    k->send = pca9538_send;
+}
+
 static void pca_i2c_gpio_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -383,6 +472,11 @@ static const TypeInfo pca_gpio_types[] = {
     .parent = TYPE_PCA_I2C_GPIO,
     .class_init = pca6416_gpio_class_init,
     },
+    {
+    .name = TYPE_PCA9538_GPIO,
+    .parent = TYPE_PCA_I2C_GPIO,
+    .class_init = pca9538_gpio_class_init,
+    },
 };
 
 DEFINE_TYPES(pca_gpio_types);
diff --git a/include/hw/gpio/pca_i2c_gpio.h b/include/hw/gpio/pca_i2c_gpio.h
index 99322959e1..3ab7d19a97 100644
--- a/include/hw/gpio/pca_i2c_gpio.h
+++ b/include/hw/gpio/pca_i2c_gpio.h
@@ -19,6 +19,7 @@
 
 #define PCA_I2C_MAX_PINS                     16
 #define PCA6416_NUM_PINS                     16
+#define PCA9538_NUM_PINS                     8
 
 typedef struct PCAGPIOClass {
     I2CSlaveClass parent;
@@ -62,8 +63,14 @@ OBJECT_DECLARE_TYPE(PCAGPIOState, PCAGPIOClass, PCA_I2C_GPIO)
 #define PCA6416_CONFIGURATION_PORT_0         0x06 /* read/write */
 #define PCA6416_CONFIGURATION_PORT_1         0x07 /* read/write */
 
+#define PCA9538_INPUT_PORT                   0x00 /* read */
+#define PCA9538_OUTPUT_PORT                  0x01 /* read/write */
+#define PCA9538_POLARITY_INVERSION_PORT      0x02 /* read/write */
+#define PCA9538_CONFIGURATION_PORT           0x03 /* read/write */
+
 #define PCA_I2C_CONFIG_DEFAULT               0
 
 #define TYPE_PCA6416_GPIO "pca6416"
+#define TYPE_PCA9538_GPIO "pca9538"
 
 #endif
-- 
2.39.1.581.gbfd45094c4-goog

Re: [PATCH v2 2/4] hw/gpio: add PCA9538 8-bit GPIO expander

[Philippe Mathieu-Daudé]

On 8/2/23 23:43, Titus Rwantare wrote:
>    The 8-bit expander has different register offsets than the 16-bit one,
>    making them incompatible.

Following extract/deposit API suggestion on previous patch, an
alternatively is to use PCAGPIOClass::input_port/output_port/...
offset fields.

> Reviewed-by: Hao Wu <wuhaotsh@google.com>
> Signed-off-by: Titus Rwantare <titusr@google.com>
> ---
>   hw/gpio/pca_i2c_gpio.c         | 94 ++++++++++++++++++++++++++++++++++
>   include/hw/gpio/pca_i2c_gpio.h |  7 +++
>   2 files changed, 101 insertions(+)


> diff --git a/include/hw/gpio/pca_i2c_gpio.h b/include/hw/gpio/pca_i2c_gpio.h
> index 99322959e1..3ab7d19a97 100644
> --- a/include/hw/gpio/pca_i2c_gpio.h
> +++ b/include/hw/gpio/pca_i2c_gpio.h
> @@ -19,6 +19,7 @@
>   
>   #define PCA_I2C_MAX_PINS                     16
>   #define PCA6416_NUM_PINS                     16
> +#define PCA9538_NUM_PINS                     8
>   
>   typedef struct PCAGPIOClass {
>       I2CSlaveClass parent;
> @@ -62,8 +63,14 @@ OBJECT_DECLARE_TYPE(PCAGPIOState, PCAGPIOClass, PCA_I2C_GPIO)
>   #define PCA6416_CONFIGURATION_PORT_0         0x06 /* read/write */
>   #define PCA6416_CONFIGURATION_PORT_1         0x07 /* read/write */
>   
> +#define PCA9538_INPUT_PORT                   0x00 /* read */
> +#define PCA9538_OUTPUT_PORT                  0x01 /* read/write */
> +#define PCA9538_POLARITY_INVERSION_PORT      0x02 /* read/write */
> +#define PCA9538_CONFIGURATION_PORT           0x03 /* read/write */

Something like this maybe:

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

         if (ps->command == pc->input_port) {
             data = extract16(ps->curr_input, shift, 8);
         } else if (ps->command == pc->output_port) {
             data = extract16(ps->curr_input, shift, 8);
         } else if (...) {

Maybe I'm over-engineering :)

[PATCH v2 3/4] hw/gpio: add PCA9536 i2c gpio expander

[Titus Rwantare]

This device has the same register layout as the pca9538, but 4 fewer
gpio pins. This commit lowers the number of pins initialised, and reuses
the pca9538 logic.

Reviewed-by: Hao Wu <wuhaotsh@google.com>
Signed-off-by: Titus Rwantare <titusr@google.com>
---
 hw/gpio/pca_i2c_gpio.c         | 18 ++++++++++++++++++
 include/hw/gpio/pca_i2c_gpio.h |  2 ++
 2 files changed, 20 insertions(+)

diff --git a/hw/gpio/pca_i2c_gpio.c b/hw/gpio/pca_i2c_gpio.c
index fa69523556..8e30064990 100644
--- a/hw/gpio/pca_i2c_gpio.c
+++ b/hw/gpio/pca_i2c_gpio.c
@@ -426,6 +426,19 @@ static void pca9538_gpio_class_init(ObjectClass *klass, void *data)
     k->send = pca9538_send;
 }
 
+static void pca9536_gpio_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
+    PCAGPIOClass *pc = PCA_I2C_GPIO_CLASS(klass);
+
+    dc->desc = "PCA9536 4-bit I/O expander";
+    pc->num_pins = PCA9536_NUM_PINS;
+
+    k->recv = pca9538_recv;
+    k->send = pca9538_send;
+}
+
 static void pca_i2c_gpio_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -477,6 +490,11 @@ static const TypeInfo pca_gpio_types[] = {
     .parent = TYPE_PCA_I2C_GPIO,
     .class_init = pca9538_gpio_class_init,
     },
+    {
+    .name = TYPE_PCA9536_GPIO,
+    .parent = TYPE_PCA_I2C_GPIO,
+    .class_init = pca9536_gpio_class_init,
+    },
 };
 
 DEFINE_TYPES(pca_gpio_types);
diff --git a/include/hw/gpio/pca_i2c_gpio.h b/include/hw/gpio/pca_i2c_gpio.h
index 3ab7d19a97..6f8260a8c6 100644
--- a/include/hw/gpio/pca_i2c_gpio.h
+++ b/include/hw/gpio/pca_i2c_gpio.h
@@ -20,6 +20,7 @@
 #define PCA_I2C_MAX_PINS                     16
 #define PCA6416_NUM_PINS                     16
 #define PCA9538_NUM_PINS                     8
+#define PCA9536_NUM_PINS                     4
 
 typedef struct PCAGPIOClass {
     I2CSlaveClass parent;
@@ -72,5 +73,6 @@ OBJECT_DECLARE_TYPE(PCAGPIOState, PCAGPIOClass, PCA_I2C_GPIO)
 
 #define TYPE_PCA6416_GPIO "pca6416"
 #define TYPE_PCA9538_GPIO "pca9538"
+#define TYPE_PCA9536_GPIO "pca9536"
 
 #endif
-- 
2.39.1.581.gbfd45094c4-goog

Re: [PATCH v2 3/4] hw/gpio: add PCA9536 i2c gpio expander

[Philippe Mathieu-Daudé]

On 8/2/23 23:43, Titus Rwantare wrote:
> This device has the same register layout as the pca9538, but 4 fewer
> gpio pins. This commit lowers the number of pins initialised, and reuses
> the pca9538 logic.
> 
> Reviewed-by: Hao Wu <wuhaotsh@google.com>
> Signed-off-by: Titus Rwantare <titusr@google.com>
> ---
>   hw/gpio/pca_i2c_gpio.c         | 18 ++++++++++++++++++
>   include/hw/gpio/pca_i2c_gpio.h |  2 ++
>   2 files changed, 20 insertions(+)


> +static void pca9536_gpio_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
> +    PCAGPIOClass *pc = PCA_I2C_GPIO_CLASS(klass);
> +
> +    dc->desc = "PCA9536 4-bit I/O expander";
> +    pc->num_pins = PCA9536_NUM_PINS;
> +
> +    k->recv = pca9538_recv;
> +    k->send = pca9538_send;

Can we rename as pca953x_recv() / pca953x_send() in previous patch?

Otherwise,
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH v2 4/4] hw/i2c: add canonical path to i2c event traces

[Titus Rwantare]

Signed-off-by: Titus Rwantare <titusr@google.com>
---
 hw/i2c/core.c       | 8 +++++---
 hw/i2c/trace-events | 2 +-
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index d4ba8146bf..c583c1497a 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -161,7 +161,8 @@ static int i2c_do_start_transfer(I2CBus *bus, uint8_t address,
            start condition.  */
 
         if (sc->event) {
-            trace_i2c_event(event == I2C_START_SEND ? "start" : "start_async",
+            trace_i2c_event(DEVICE(s)->canonical_path,
+                            event == I2C_START_SEND ? "start" : "start_async",
                             s->address);
             rv = sc->event(s, event);
             if (rv && !bus->broadcast) {
@@ -227,7 +228,7 @@ void i2c_end_transfer(I2CBus *bus)
         I2CSlave *s = node->elt;
         sc = I2C_SLAVE_GET_CLASS(s);
         if (sc->event) {
-            trace_i2c_event("finish", s->address);
+            trace_i2c_event(DEVICE(s)->canonical_path, "finish", s->address);
             sc->event(s, I2C_FINISH);
         }
         QLIST_REMOVE(node, next);
@@ -314,7 +315,8 @@ void i2c_nack(I2CBus *bus)
     QLIST_FOREACH(node, &bus->current_devs, next) {
         sc = I2C_SLAVE_GET_CLASS(node->elt);
         if (sc->event) {
-            trace_i2c_event("nack", node->elt->address);
+            trace_i2c_event(DEVICE(node->elt)->canonical_path,
+                            "nack", node->elt->address);
             sc->event(node->elt, I2C_NACK);
         }
     }
diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events
index 8e88aa24c1..f42a1ff539 100644
--- a/hw/i2c/trace-events
+++ b/hw/i2c/trace-events
@@ -9,7 +9,7 @@ bitbang_i2c_data(unsigned dat, unsigned clk, unsigned old_out, unsigned new_out)
 
 # core.c
 
-i2c_event(const char *event, uint8_t address) "%s(addr:0x%02x)"
+i2c_event(const char *id, const char *event, uint8_t address) "%s: %s(addr:0x%02x)"
 i2c_send(uint8_t address, uint8_t data) "send(addr:0x%02x) data:0x%02x"
 i2c_send_async(uint8_t address, uint8_t data) "send_async(addr:0x%02x) data:0x%02x"
 i2c_recv(uint8_t address, uint8_t data) "recv(addr:0x%02x) data:0x%02x"
-- 
2.39.1.581.gbfd45094c4-goog

Re: [PATCH v2 4/4] hw/i2c: add canonical path to i2c event traces

[Philippe Mathieu-Daudé]

On 8/2/23 23:43, Titus Rwantare wrote:
> Signed-off-by: Titus Rwantare <titusr@google.com>
> ---
>   hw/i2c/core.c       | 8 +++++---
>   hw/i2c/trace-events | 2 +-
>   2 files changed, 6 insertions(+), 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>