Re: [PATCH 1/3] hw/gpio: add PCA6414 i2c GPIO expander

[Titus Rwantare]

On Mon, 6 Feb 2023 at 13:38, Philippe Mathieu-Daudé <philmd@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 <wuhaotsh@google.com>
> > Signed-off-by: Titus Rwantare <titusr@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