On Mon, 6 Feb 2023 at 13:38, Philippe Mathieu-Daudé <phi...@linaro.org> wrote: > > Hi Titus, > > On 6/2/23 20:49, Titus Rwantare wrote: > > This is a simple i2c device that allows i2c capable devices to have > > GPIOs. > > > > 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 > > > > +/* slave to master */ > > +static uint8_t pca6416_recv(I2CSlave *i2c) > > +{ > > + PCAGPIOState *ps = PCA_I2C_GPIO(i2c); > > + uint8_t data;
> > + default: > > + qemu_log_mask(LOG_GUEST_ERROR, > > + "%s: reading from unsupported register 0x%02x", > > Some of your qemu_log_mask() calls miss the trailing newline. Fixed. > > > > +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"); > > Maybe add the canonical_path to trace_i2c_event() so this is useful > for all I2C devices. I've added a patch that does this. > > > > +static void pca_i2c_realize(DeviceState *dev, Error **errp) > > +{ > > + PCAGPIOState *ps = PCA_I2C_GPIO(dev); > > + pca_i2c_update_irqs(ps); > > There is no reset() handler, is that on purpose? No, I've added one. > > > +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"; > > Why not set these 3 ...: > > > + dc->realize = pca_i2c_realize; > > + dc->vmsd = &vmstate_pca_i2c_gpio; > > + > > + k->event = pca_i2c_event; > > ... in a base abstract class pca_i2c_gpio_class_init()? > Right, the code evolved. This makes more sense to do now. > > > diff --git a/hw/i2c/Kconfig b/hw/i2c/Kconfig > > index 14886b35da..b59a79fddf 100644 > > --- a/hw/i2c/Kconfig > > +++ b/hw/i2c/Kconfig > > @@ -42,6 +42,10 @@ config PCA954X > > bool > > select I2C > > > > +config PCA_I2C_GPIO > > + bool > > + select I2C > > This should be 'depends on I2C'. > > Apparently various entries are incorrect in this file. > > Per docs/devel/kconfig.rst: > > Devices *depend on* the bus that they lie on, for example a PCI > device would specify ``depends on PCI``. An MMIO device will likely > have no ``depends on`` directive. Devices also *select* the buses > that the device provides, for example a SCSI adapter would specify > ``select SCSI``. I've moved the entry to hw/gpio and fixed it. > > diff --git a/include/hw/gpio/pca_i2c_gpio.h b/include/hw/gpio/pca_i2c_gpio.h > > new file mode 100644 > > index 0000000000..a10897c0e0 > > --- /dev/null > > +++ b/include/hw/gpio/pca_i2c_gpio.h > > @@ -0,0 +1,72 @@ > > +/* > > + * 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 PCA6416_NUM_PINS 16 > > ^ This is specific to TYPE_PCA6416_GPIO, and you set > PCAGPIOClass::num_pins to it in pca6416_gpio_class_init(), ... > > > + > > +typedef struct PCAGPIOClass { > > + I2CSlaveClass parent; > > + > > + uint8_t num_pins; > > +} PCAGPIOClass; > > + > > +typedef struct PCAGPIOState { > > ... but this defines the generic TYPE_PCA_I2C_GPIO ... > > > + 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[PCA6416_NUM_PINS]; > > ... which is now clamped to 16 pins. > > Maybe define/use PCA_I2C_GPIO_MAX_PINS instead here, and assert > PCAGPIOClass::num_pins <= PCA_I2C_GPIO_MAX_PINS in > pca_i2c_gpio_class_init() or a realize? > > Actually we don't need PCA6416_NUM_PINS, PCA_I2C_GPIO_MAX_PINS is > enough; and we can set 'pc->num_pins = 16' in pca6416_gpio_class_init() > or use PCA6416_NUM_PINS but restrict its definition in pca_i2c_gpio.c. I've redone class_init in v2. > > + /* 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 PCA6416_OUTPUT_DEFAULT 0xFFFF > > +#define PCA6416_CONFIG_DEFAULT 0xFFFF > > + > > +#define PCA_I2C_OUTPUT_DEFAULT 0xFFFF > > +#define PCA_I2C_CONFIG_DEFAULT 0xFFFF > > (These register definitions could be kept internal in pca_i2c_gpio.c). I put these here to use them in the qtests. > > +#define TYPE_PCA6416_GPIO "pca6416" > > + > > +#endif > > > 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', > > Should this be conditional to > config_all_devices.has_key('CONFIG_PCA_I2C_GPIO')? Is that the guidance for qos tests? All these tests would also need to be separated out. > > 'pca9552-test.c', > > 'pci-test.c', > > 'pcnet-test.c', > > Mostly nitpicking, LGTM otherwise :) > > Regards, > > Phil. Thanks for the review. -Titus