Re: [PATCH] hw/i2c: flatten pca954x mux device

[Patrick Venture <venture@google.com>]

[-- Attachment #1: Type: text/plain, Size: 3595 bytes --]

On Tue, Feb 1, 2022 at 12:54 PM Patrick Venture <venture@google.com> wrote:

>
>
> On Tue, Feb 1, 2022 at 11:02 AM Philippe Mathieu-Daudé <f4bug@amsat.org>
> wrote:
>
>> On 1/2/22 17:30, Patrick Venture 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 <wuhaotsh@google.com>
>> > Signed-off-by: Patrick Venture <venture@google.com>
>> > ---
>> >   hw/i2c/i2c_mux_pca954x.c | 75 ++++++----------------------------------
>> >   1 file changed, 11 insertions(+), 64 deletions(-)
>>
>> >   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);
>> > +        /* start all channels as disabled. */
>> > +        s->enabled[i] = false;
>> > +        s->bus[i] = i2c_init_bus(DEVICE(s), "channel[*]");
>>
>> This is not a QOM property, so you need to initialize manually:
>>
>
> that was my suspicion but this is the output I'm seeing:
>
> {'execute': 'qom-list', 'arguments': { 'path':
> '/machine/soc/smbus[0]/i2c-bus/child[0]' }}
>
> {"return": [
> {"name": "type", "type": "string"},
> {"name": "parent_bus", "type": "link<bus>"},
> {"name": "realized", "type": "bool"},
> {"name": "hotplugged", "type": "bool"},
> {"name": "hotpluggable", "type": "bool"},
> {"name": "address", "type": "uint8"},
> {"name": "channel[3]", "type": "child<i2c-bus>"},
> {"name": "channel[0]", "type": "child<i2c-bus>"},
> {"name": "channel[1]", "type": "child<i2c-bus>"},
> {"name": "channel[2]", "type": "child<i2c-bus>"}
> ]}
>
> It seems to be naming them via the order they're created.
>
> Is this not behaving how you expect?
>

Philippe,

I0202 08:29:45.380384  6641 stream.go:31] qemu: child buses at "pca9546":
"channel[*]", "channel[*]", "channel[*]", "channel[*]"

Ok, so that's interesting.  In one system (using qom-list) it's correct,
but then when using it to do path assignment (qdev-monitor), it fails...

I'm not as fond of the name i2c-bus.%d, since they're referred to as
channels in the datasheet.  If I do the manual name creation, can I keep
the name channel or should I pivot over?

Thanks


>
>>
>> -- >8 --
>> diff --git a/hw/i2c/i2c_mux_pca954x.c b/hw/i2c/i2c_mux_pca954x.c
>> index f9ce633b3a..a9517b612a 100644
>> --- a/hw/i2c/i2c_mux_pca954x.c
>> +++ b/hw/i2c/i2c_mux_pca954x.c
>> @@ -189,9 +189,11 @@ static void pca954x_init(Object *obj)
>>
>>       /* SMBus modules. Cannot fail. */
>>       for (i = 0; i < c->nchans; i++) {
>> +        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), "channel[*]");
>> +        s->bus[i] = i2c_init_bus(DEVICE(s), bus_name);
>>       }
>>   }
>>
>> ---
>>
>> (look at HMP 'info qtree' output).
>>
>> >       }
>> >   }
>>
>> With the change:
>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>
>

[-- Attachment #2: Type: text/html, Size: 5811 bytes --]