[v2] hw/i2c: flatten pca954x mux device

Commit Message

Patrick Leis Feb. 2, 2022, 4:45 p.m. UTC

Previously this device created N subdevices which each owned an i2c bus.
Now this device simply owns the N i2c busses directly.

Tested: Verified devices behind mux are still accessible via qmp and i2c
from within an arm32 SoC.

Reviewed-by: Hao Wu <[e-mail]>
Signed-off-by: Patrick Venture <[e-mail]>
Reviewed-by: Philippe Mathieu-Daudé <[e-mail]>
Tested-by: Philippe Mathieu-Daudé <[e-mail]>
---
v2: explicitly create an incrementing name for the i2c busses (channels).
---
 hw/i2c/i2c_mux_pca954x.c | 77 +++++++---------------------------------
 1 file changed, 13 insertions(+), 64 deletions(-)

Comments

Peter Maydell Feb. 24, 2022, 10:56 a.m. UTC | #1

On Wed, 2 Feb 2022 at 17:57, Patrick Venture <[e-mail]> wrote:
>
> Previously this device created N subdevices which each owned an i2c bus.
> Now this device simply owns the N i2c busses directly.
>
> Tested: Verified devices behind mux are still accessible via qmp and i2c
> from within an arm32 SoC.
>
> Reviewed-by: Hao Wu <[e-mail]>
> Signed-off-by: Patrick Venture <[e-mail]>
> Reviewed-by: Philippe Mathieu-Daudé <[e-mail]>
> Tested-by: Philippe Mathieu-Daudé <[e-mail]>
> ---
> v2: explicitly create an incrementing name for the i2c busses (channels).
> ---

Applied to target-arm.next, thanks.

Apologies for not picking this up earlier, the v2 got lost in the
side-conversation about spam filtering. (Blame gmail for not
doing email threading properly if you like :-))

-- PMM

Patrick Leis Feb. 24, 2022, 6:52 p.m. UTC | #2

On Thu, Feb 24, 2022 at 2:56 AM Peter Maydell <[e-mail]>
wrote:

> On Wed, 2 Feb 2022 at 17:57, Patrick Venture <[e-mail]> wrote:
> >
> > Previously this device created N subdevices which each owned an i2c bus.
> > Now this device simply owns the N i2c busses directly.
> >
> > Tested: Verified devices behind mux are still accessible via qmp and i2c
> > from within an arm32 SoC.
> >
> > Reviewed-by: Hao Wu <[e-mail]>
> > Signed-off-by: Patrick Venture <[e-mail]>
> > Reviewed-by: Philippe Mathieu-Daudé <[e-mail]>
> > Tested-by: Philippe Mathieu-Daudé <[e-mail]>
> > ---
> > v2: explicitly create an incrementing name for the i2c busses (channels).
> > ---
>
> Applied to target-arm.next, thanks.
>
> Apologies for not picking this up earlier, the v2 got lost in the
> side-conversation about spam filtering. (Blame gmail for not
> doing email threading properly if you like :-))
>

Thanks, and no problem.  This v2 is what we have downstream for this.

I'm working on a further improvement to it (separate feature change)
that'll allow setting an id on the device so that all its channels have
that the id embedded in them.  This'll handle some of the situations we're
observing where the qdev paths aren't great for command line added i2c
devices.  There's a side conversation going on about how best to accomplish
this.


>
> -- PMM
>

Patch

diff --git a/hw/i2c/i2c_mux_pca954x.c b/hw/i2c/i2c_mux_pca954x.c
index 847c59921c..a9517b612a 100644
--- a/hw/i2c/i2c_mux_pca954x.c
+++ b/hw/i2c/i2c_mux_pca954x.c
@@ -30,24 +30,6 @@ 
 #define PCA9548_CHANNEL_COUNT 8
 #define PCA9546_CHANNEL_COUNT 4
 
-/*
- * struct Pca954xChannel - The i2c mux device will have N of these states
- * that own the i2c channel bus.
- * @bus: The owned channel bus.
- * @enabled: Is this channel active?
- */
-typedef struct Pca954xChannel {
-    SysBusDevice parent;
-
-    I2CBus       *bus;
-
-    bool         enabled;
-} Pca954xChannel;
-
-#define TYPE_PCA954X_CHANNEL "pca954x-channel"
-#define PCA954X_CHANNEL(obj) \
-    OBJECT_CHECK(Pca954xChannel, (obj), TYPE_PCA954X_CHANNEL)
-
 /*
  * struct Pca954xState - The pca954x state object.
  * @control: The value written to the mux control.
@@ -59,8 +41,8 @@  typedef struct Pca954xState {
 
     uint8_t control;
 
-    /* The channel i2c buses. */
-    Pca954xChannel channel[PCA9548_CHANNEL_COUNT];
+    bool enabled[PCA9548_CHANNEL_COUNT];
+    I2CBus *bus[PCA9548_CHANNEL_COUNT];
 } Pca954xState;
 
 /*
@@ -98,11 +80,11 @@  static bool pca954x_match(I2CSlave *candidate, uint8_t address,
     }
 
     for (i = 0; i < mc->nchans; i++) {
-        if (!mux->channel[i].enabled) {
+        if (!mux->enabled[i]) {
             continue;
         }
 
-        if (i2c_scan_bus(mux->channel[i].bus, address, broadcast,
+        if (i2c_scan_bus(mux->bus[i], address, broadcast,
                          current_devs)) {
             if (!broadcast) {
                 return true;
@@ -125,9 +107,9 @@  static void pca954x_enable_channel(Pca954xState *s, uint8_t enable_mask)
      */
     for (i = 0; i < mc->nchans; i++) {
         if (enable_mask & (1 << i)) {
-            s->channel[i].enabled = true;
+            s->enabled[i] = true;
         } else {
-            s->channel[i].enabled = false;
+            s->enabled[i] = false;
         }
     }
 }
@@ -184,23 +166,7 @@  I2CBus *pca954x_i2c_get_bus(I2CSlave *mux, uint8_t channel)
     Pca954xState *pca954x = PCA954X(mux);
 
     g_assert(channel < pc->nchans);
-    return I2C_BUS(qdev_get_child_bus(DEVICE(&pca954x->channel[channel]),
-                                      "i2c-bus"));
-}
-
-static void pca954x_channel_init(Object *obj)
-{
-    Pca954xChannel *s = PCA954X_CHANNEL(obj);
-    s->bus = i2c_init_bus(DEVICE(s), "i2c-bus");
-
-    /* Start all channels as disabled. */
-    s->enabled = false;
-}
-
-static void pca954x_channel_class_init(ObjectClass *klass, void *data)
-{
-    DeviceClass *dc = DEVICE_CLASS(klass);
-    dc->desc = "Pca954x Channel";
+    return pca954x->bus[channel];
 }
 
 static void pca9546_class_init(ObjectClass *klass, void *data)
@@ -215,28 +181,19 @@  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);
-    Pca954xClass *c = PCA954X_GET_CLASS(s);
-    int i;
-
-    /* SMBus modules. Cannot fail. */
-    for (i = 0; i < c->nchans; i++) {
-        sysbus_realize(SYS_BUS_DEVICE(&s->channel[i]), &error_abort);
-    }
-}
-
 static void pca954x_init(Object *obj)
 {
     Pca954xState *s = PCA954X(obj);
     Pca954xClass *c = PCA954X_GET_CLASS(obj);
     int i;
 
-    /* Only initialize the children we expect. */
+    /* SMBus modules. Cannot fail. */
     for (i = 0; i < c->nchans; i++) {
-        object_initialize_child(obj, "channel[*]", &s->channel[i],
-                                TYPE_PCA954X_CHANNEL);
+        g_autofree gchar *bus_name = g_strdup_printf("i2c.%d", i);
+
+        /* start all channels as disabled. */
+        s->enabled[i] = false;
+        s->bus[i] = i2c_init_bus(DEVICE(s), bus_name);
     }
 }
 
@@ -252,7 +209,6 @@  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;
@@ -278,13 +234,6 @@  static const TypeInfo pca954x_info[] = {
         .parent        = TYPE_PCA954X,
         .class_init    = pca9548_class_init,
     },
-    {
-        .name = TYPE_PCA954X_CHANNEL,
-        .parent = TYPE_SYS_BUS_DEVICE,
-        .class_init = pca954x_channel_class_init,
-        .instance_size = sizeof(Pca954xChannel),
-        .instance_init = pca954x_channel_init,
-    }
 };
 
 DEFINE_TYPES(pca954x_info)