Hi Titus,
On 8/2/23 23:43, Titus Rwantare wrote:
The PCA6416 is an i2c device with 16 GPIOs.
Reviewed-by: Hao Wu <wuhao...@google.com>
Signed-off-by: Titus Rwantare <tit...@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.