On Mon, 6 Feb 2023 at 17:29, Dong, Eddie <eddie.d...@intel.com> wrote: > > -----Original Message----- > > From: qemu-devel-bounces+eddie.dong=intel....@nongnu.org <qemu- > > devel-bounces+eddie.dong=intel....@nongnu.org> On Behalf Of Titus > > Rwantare > > Sent: Monday, February 6, 2023 11:50 AM > > To: peter.mayd...@linaro.org > > Cc: qemu-...@nongnu.org; qemu-devel@nongnu.org; Titus Rwantare > > <tit...@google.com>; Hao Wu <wuhao...@google.com> > > Subject: [PATCH 1/3] hw/gpio: add PCA6414 i2c GPIO expander > > > > This is a simple i2c device that allows i2c capable devices to have GPIOs. > > This patch is to implement a device model of I2C to GPIO for PCA_xxx, right? > Or do you implement a general/common I2C to GPIO device? > I think it is for the former one. In this case, the commit message is not > clear.
I've redone the commit message. > > > > > Reviewed-by: Hao Wu <wuhao...@google.com> > > Signed-off-by: Titus Rwantare <tit...@google.com> > > --- > > hw/arm/Kconfig | 1 + > > hw/gpio/meson.build | 1 + > > hw/gpio/pca_i2c_gpio.c | 362 ++++++++++++++++++++++++++++++++ > > hw/gpio/trace-events | 5 + > > hw/i2c/Kconfig | 4 + > > include/hw/gpio/pca_i2c_gpio.h | 72 +++++++ > > tests/qtest/meson.build | 1 + > > tests/qtest/pca_i2c_gpio-test.c | 169 +++++++++++++++ > > 8 files changed, 615 insertions(+) > > 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/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..afae497a22 > > --- /dev/null > > +++ b/hw/gpio/pca_i2c_gpio.c > > Should this file be located under hw/i2c ? I think there's a case to be made for either location. However, there already exists an i2c device in hw/gpio. > > @@ -0,0 +1,362 @@ > > +/* > > + * 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 > > + * > > + * These devices, by default, are configured to input only. The > > +configuration is > > + * settable through qom/qmp, or i2c.To set some pins as inputs 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, sets pins 0-3 as outputs > > +and > > + * 4-15 as inputs. > > + * This value can also be set at runtime through qmp externally, or by > > + * writing to the config register using i2c. > > + * > > + */ > > + > > +#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) { > The spec says " A pin configured as an output cannot cause an interrupt". > Do we need to check out_diff here? This is confusing, as I understand it, qemu_set_irq doesn't necessarily send an interrupt to the guest. A qemu_irq can be connected to another device expecting them as inputs like GPIO devices, or connect to an interrupt controller that passes them to the guest. Someone with a better understanding of QEMU's irq structures than I have, can give a better explanation. > > + for (int i = 0; i < pc->num_pins; i++) { > At least for PCA_6416A, the state of pins are described in UINT16. We can > check > all together rather than bit scan/pin scan. > This seems possible, but I tried and broke it. I'd still need a per pin loop to call qemu_set_irq. > > + 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) { > If we use an array to define the state (or union with an array access), > we can avoid the duplicated code below. > Ack, the trade off with using the array is difficulty reasoning about what the code is doing at a glance with the different devices. Thanks for the review, -Titus