Re: [PATCH RESEND] hw/i2c: Enable an id for the pca954x devices

[Corey Minyard <minyard@acm.org>]

On Wed, Mar 22, 2023 at 10:03:27AM -0700, Patrick Venture wrote:
> On Tue, Mar 21, 2023 at 6:41 PM Corey Minyard <cminyard@mvista.com> wrote:
> 
> > On Tue, Mar 21, 2023 at 11:27:44AM -0700, Patrick Venture wrote:
> > > This allows the devices to be more readily found and specified.
> > > Without setting the id field, they can only be found by device type
> > > name, which doesn't let you specify the second of the same device type
> > > behind a bus.
> >
> > So basically, this helps you find a specific device if you have more
> > than one of them.  Yeah, probably a good idea in this case.
> >
> > >
> > > Tested: Verified that by default the device was findable with the id
> > > 'pca954x[77]', for an instance attached at that address.
> > >
> > > Signed-off-by: Patrick Venture <venture@google.com>
> > > Reviewed-by: Hao Wu <wuhaotsh@google.com>
> > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > ---
> > >  hw/i2c/i2c_mux_pca954x.c | 22 ++++++++++++++++++++++
> > >  1 file changed, 22 insertions(+)
> > >
> > > diff --git a/hw/i2c/i2c_mux_pca954x.c b/hw/i2c/i2c_mux_pca954x.c
> > > index a9517b612a..4f8c2d6ae1 100644
> > > --- a/hw/i2c/i2c_mux_pca954x.c
> > > +++ b/hw/i2c/i2c_mux_pca954x.c
> > > @@ -20,6 +20,7 @@
> > >  #include "hw/i2c/i2c_mux_pca954x.h"
> > >  #include "hw/i2c/smbus_slave.h"
> > >  #include "hw/qdev-core.h"
> > > +#include "hw/qdev-properties.h"
> > >  #include "hw/sysbus.h"
> > >  #include "qemu/log.h"
> > >  #include "qemu/module.h"
> > > @@ -43,6 +44,8 @@ typedef struct Pca954xState {
> > >
> > >      bool enabled[PCA9548_CHANNEL_COUNT];
> > >      I2CBus *bus[PCA9548_CHANNEL_COUNT];
> > > +
> > > +    char *id;
> > >  } Pca954xState;
> > >
> > >  /*
> > > @@ -181,6 +184,17 @@ static void pca9548_class_init(ObjectClass *klass,
> > void *data)
> > >      s->nchans = PCA9548_CHANNEL_COUNT;
> > >  }
> > >
> > > +static void pca954x_realize(DeviceState *dev, Error **errp)
> > > +{
> > > +    Pca954xState *s = PCA954X(dev);
> > > +    DeviceState *d = DEVICE(s);
> > > +    if (s->id) {
> > > +        d->id = g_strdup(s->id);
> > > +    } else {
> > > +        d->id = g_strdup_printf("pca954x[%x]", s->parent.i2c.address);
> > > +    }
> > > +}
> > > +
> > >  static void pca954x_init(Object *obj)
> > >  {
> > >      Pca954xState *s = PCA954X(obj);
> > > @@ -197,6 +211,11 @@ static void pca954x_init(Object *obj)
> > >      }
> > >  }
> > >
> > > +static Property pca954x_props[] = {
> > > +    DEFINE_PROP_STRING("id", Pca954xState, id),
> > > +    DEFINE_PROP_END_OF_LIST()
> > > +};
> >
> > There is already an "id=" thing in qemu for doing links between front
> > ends and back ends.  That's probably not the best name to use.  Several
> > devices, like network devices, use "name" for identification in the
> > monitor.
> >
> 
> So I should change it to name? I'm ok with that. I think bus would be
> slightly confusing.  "id" was short and easy.  Will send a v2.

Yes, "name" would be more consistent with other usage.

-corey

> 
> 
> >
> > -corey
> >
> > > +
> > >  static void pca954x_class_init(ObjectClass *klass, void *data)
> > >  {
> > >      I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
> > > @@ -209,9 +228,12 @@ static void pca954x_class_init(ObjectClass *klass,
> > void *data)
> > >      rc->phases.enter = pca954x_enter_reset;
> > >
> > >      dc->desc = "Pca954x i2c-mux";
> > > +    dc->realize = pca954x_realize;
> > >
> > >      k->write_data = pca954x_write_data;
> > >      k->receive_byte = pca954x_read_byte;
> > > +
> > > +    device_class_set_props(dc, pca954x_props);
> > >  }
> > >
> > >  static const TypeInfo pca954x_info[] = {
> > > --
> > > 2.35.1.894.gb6a874cedc-goog
> > >
> >