diff mbox series

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

Message ID 20230321182744.2814034-1-venture@google.com (mailing list archive)
State New, archived
Headers show
Series [RESEND] hw/i2c: Enable an id for the pca954x devices | expand

Commit Message

Patrick Leis March 21, 2023, 6:27 p.m. UTC 
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.

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(+)

Comments

Corey Minyard March 22, 2023, 1:41 a.m. UTC | #1 
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.

-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
>
Patrick Leis March 22, 2023, 5:03 p.m. UTC | #2 
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.


>
> -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
> >
>
Corey Minyard March 22, 2023, 6:35 p.m. UTC | #3 
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
> > >
> >
diff mbox series

Patch

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()
+};
+
 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[] = {