Re: [PATCH v2 4/4] hw/i2c: add pca954x i2c-mux switch

[Patrick Venture <venture@google.com>]

On Fri, Apr 9, 2021 at 9:51 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Hi Patrick,
>
> On 4/9/21 6:25 PM, Patrick Venture wrote:
> > The pca954x is an i2c mux, and this adds support for two variants of
> > this device: the pca9546 and pca9548.
> >
> > This device is very common on BMCs to route a different channel to each
> > PCIe i2c bus downstream from the BMC.
> >
> > Signed-off-by: Patrick Venture <venture@google.com>
> > Reviewed-by: Hao Wu <wuhaotsh@google.com>
> > Reviewed-by: Havard Skinnemoen <hskinnemoen@google.com>
> > ---
> >  MAINTAINERS                      |   6 +
> >  hw/i2c/Kconfig                   |   4 +
> >  hw/i2c/i2c_mux_pca954x.c         | 290 +++++++++++++++++++++++++++++++
> >  hw/i2c/meson.build               |   1 +
> >  hw/i2c/trace-events              |   5 +
> >  include/hw/i2c/i2c_mux_pca954x.h |  19 ++
> >  6 files changed, 325 insertions(+)
> >  create mode 100644 hw/i2c/i2c_mux_pca954x.c
> >  create mode 100644 include/hw/i2c/i2c_mux_pca954x.h
>
> > diff --git a/hw/i2c/Kconfig b/hw/i2c/Kconfig
> > index 09642a6dcb..8d120a25d5 100644
> > --- a/hw/i2c/Kconfig
> > +++ b/hw/i2c/Kconfig
> > @@ -28,3 +28,7 @@ config IMX_I2C
> >  config MPC_I2C
> >      bool
> >      select I2C
> > +
> > +config PCA954X
> > +    bool
> > +    select I2C
>
> Do you have a circular dependency when also using:
>
>        depends on I2C
>
> ?

I'm somewhat new to qemu -- I don't know what you mean, since I2C
doesn't depend on pca954x, I don't imagine there could be a circular
dependency.

>
> > +static void pca954x_realize(DeviceState *dev, Error **errp)
> > +{
> > +    Pca954xState *s = PCA954X(dev);
> > +    Pca954xClass *c = PCA954X_GET_CLASS(s);
> > +    int i;
> > +
> > +    /* SMBus modules. Cannot fail. */
> > +    for (i = 0; i < c->nchans; i++) {
> > +        Object *obj = OBJECT(&s->channel[i]);
> > +        sysbus_realize(SYS_BUS_DEVICE(obj), &error_abort);
>
> No need to cast to Object:
>
>            sysbus_realize(SYS_BUS_DEVICE(&s->channel[i]), &error_abort);
>

Ack, will fix in v3.

> > +    }
> > +}