| Message ID | 20161130053629.23340-2-alastair@au1.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 11/30/2016 06:36 AM, Alastair D'Silva wrote: > From: Alastair D'Silva <alastair@d-silva.org> > > The imx25 chip provides 3 i2c buses, but they have all been named > "i2c", which makes it difficult to predict which bus a device will > be connected to when specified on the command line. > > This patch addresses the issue by naming the buses uniquely: > i2c.0 i2c.1 i2c.2 > > Signed-off-by: Alastair D'Silva <alastair@d-silva.org> > --- > hw/arm/imx25_pdk.c | 4 +--- > hw/i2c/imx_i2c.c | 6 +++++- > 2 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/hw/arm/imx25_pdk.c b/hw/arm/imx25_pdk.c > index 025b608..c6f04d3 100644 > --- a/hw/arm/imx25_pdk.c > +++ b/hw/arm/imx25_pdk.c > @@ -138,9 +138,7 @@ static void imx25_pdk_init(MachineState *machine) > * We add it here (only on qtest usage) to be able to do a bit > * of simple qtest. See "make check" for details. > */ > - i2c_create_slave((I2CBus *)qdev_get_child_bus(DEVICE(&s->soc.i2c[0]), > - "i2c"), > - "ds1338", 0x68); > + i2c_create_slave(s->soc.i2c[0].bus, "ds1338", 0x68); > } > } > > diff --git a/hw/i2c/imx_i2c.c b/hw/i2c/imx_i2c.c > index 37e5a62..7be10fb 100644 > --- a/hw/i2c/imx_i2c.c > +++ b/hw/i2c/imx_i2c.c > @@ -305,12 +305,16 @@ static const VMStateDescription imx_i2c_vmstate = { > static void imx_i2c_realize(DeviceState *dev, Error **errp) > { > IMXI2CState *s = IMX_I2C(dev); > + static int bus_count; hmm, the static is ugly :/ Isn't there other ways to achieve this naming ? Thanks, C. > + char name[16]; > + > + snprintf(name, sizeof(name), "i2c.%d", bus_count++); > > memory_region_init_io(&s->iomem, OBJECT(s), &imx_i2c_ops, s, TYPE_IMX_I2C, > IMX_I2C_MEM_SIZE); > sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->iomem); > sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq); > - s->bus = i2c_init_bus(DEVICE(dev), "i2c"); > + s->bus = i2c_init_bus(DEVICE(dev), name); > } > > static void imx_i2c_class_init(ObjectClass *klass, void *data) >
On Wed, 2016-11-30 at 09:18 +0100, Cédric Le Goater wrote: > On 11/30/2016 06:36 AM, Alastair D'Silva wrote: > > From: Alastair D'Silva <alastair@d-silva.org> > > > > The imx25 chip provides 3 i2c buses, but they have all been named > > "i2c", which makes it difficult to predict which bus a device will > > be connected to when specified on the command line. > > > > This patch addresses the issue by naming the buses uniquely: > > i2c.0 i2c.1 i2c.2 > > > > Signed-off-by: Alastair D'Silva <alastair@d-silva.org> > > --- > > hw/arm/imx25_pdk.c | 4 +--- > > hw/i2c/imx_i2c.c | 6 +++++- > > 2 files changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/hw/arm/imx25_pdk.c b/hw/arm/imx25_pdk.c > > index 025b608..c6f04d3 100644 > > --- a/hw/arm/imx25_pdk.c > > +++ b/hw/arm/imx25_pdk.c > > @@ -138,9 +138,7 @@ static void imx25_pdk_init(MachineState > > *machine) > > * We add it here (only on qtest usage) to be able to do a > > bit > > * of simple qtest. See "make check" for details. > > */ > > - i2c_create_slave((I2CBus *)qdev_get_child_bus(DEVICE(&s- > > >soc.i2c[0]), > > - "i2c"), > > - "ds1338", 0x68); > > + i2c_create_slave(s->soc.i2c[0].bus, "ds1338", 0x68); > > } > > } > > > > diff --git a/hw/i2c/imx_i2c.c b/hw/i2c/imx_i2c.c > > index 37e5a62..7be10fb 100644 > > --- a/hw/i2c/imx_i2c.c > > +++ b/hw/i2c/imx_i2c.c > > @@ -305,12 +305,16 @@ static const VMStateDescription > > imx_i2c_vmstate = { > > static void imx_i2c_realize(DeviceState *dev, Error **errp) > > { > > IMXI2CState *s = IMX_I2C(dev); > > + static int bus_count; > > hmm, the static is ugly :/ > > Isn't there other ways to achieve this naming ? > > Thanks, > > C. > I'm not seeing an obvious way around it. The busses are realized independently (so I can't implement what we do with the aspeed i2c busses), and it is named before fsl-imx25:fsl_imx25_realize() can apply specific properties to the bus. If you have any suggestions, I'm all ears. -- Alastair D'Silva Open Source Developer Linux Technology Centre, IBM Australia mob: 0423 762 819
On 01/12/16 11:42, Alastair D'Silva wrote: > On Wed, 2016-11-30 at 09:18 +0100, Cédric Le Goater wrote: >> On 11/30/2016 06:36 AM, Alastair D'Silva wrote: >>> From: Alastair D'Silva <alastair@d-silva.org> >>> >>> The imx25 chip provides 3 i2c buses, but they have all been named >>> "i2c", which makes it difficult to predict which bus a device will >>> be connected to when specified on the command line. >>> >>> This patch addresses the issue by naming the buses uniquely: >>> i2c.0 i2c.1 i2c.2 It is still not guaranteed that first in the command will get "i2c.0" name, second - "i2c.1", etc. Just pass id=i2cX via the command line explicitly when creating a i2c bus device and use it as a bus id, and I am pretty sure QEMU will add a period and a number itself. >>> >>> Signed-off-by: Alastair D'Silva <alastair@d-silva.org> >>> --- >>> hw/arm/imx25_pdk.c | 4 +--- >>> hw/i2c/imx_i2c.c | 6 +++++- >>> 2 files changed, 6 insertions(+), 4 deletions(-) >>> >>> diff --git a/hw/arm/imx25_pdk.c b/hw/arm/imx25_pdk.c >>> index 025b608..c6f04d3 100644 >>> --- a/hw/arm/imx25_pdk.c >>> +++ b/hw/arm/imx25_pdk.c >>> @@ -138,9 +138,7 @@ static void imx25_pdk_init(MachineState >>> *machine) >>> * We add it here (only on qtest usage) to be able to do a >>> bit >>> * of simple qtest. See "make check" for details. >>> */ >>> - i2c_create_slave((I2CBus *)qdev_get_child_bus(DEVICE(&s- >>>> soc.i2c[0]), >>> - "i2c"), >>> - "ds1338", 0x68); >>> + i2c_create_slave(s->soc.i2c[0].bus, "ds1338", 0x68); >>> } >>> } >>> >>> diff --git a/hw/i2c/imx_i2c.c b/hw/i2c/imx_i2c.c >>> index 37e5a62..7be10fb 100644 >>> --- a/hw/i2c/imx_i2c.c >>> +++ b/hw/i2c/imx_i2c.c >>> @@ -305,12 +305,16 @@ static const VMStateDescription >>> imx_i2c_vmstate = { >>> static void imx_i2c_realize(DeviceState *dev, Error **errp) >>> { >>> IMXI2CState *s = IMX_I2C(dev); >>> + static int bus_count; >> >> hmm, the static is ugly :/ >> >> Isn't there other ways to achieve this naming ? >> >> Thanks, >> >> C. >> > > I'm not seeing an obvious way around it. The busses are realized > independently (so I can't implement what we do with the aspeed i2c > busses), and it is named before fsl-imx25:fsl_imx25_realize() can apply > specific properties to the bus. > > If you have any suggestions, I'm all ears.
On 12/01/2016 01:42 AM, Alastair D'Silva wrote: > On Wed, 2016-11-30 at 09:18 +0100, Cédric Le Goater wrote: >> On 11/30/2016 06:36 AM, Alastair D'Silva wrote: >>> From: Alastair D'Silva <alastair@d-silva.org> >>> >>> The imx25 chip provides 3 i2c buses, but they have all been named >>> "i2c", which makes it difficult to predict which bus a device will >>> be connected to when specified on the command line. >>> >>> This patch addresses the issue by naming the buses uniquely: >>> i2c.0 i2c.1 i2c.2 >>> >>> Signed-off-by: Alastair D'Silva <alastair@d-silva.org> >>> --- >>> hw/arm/imx25_pdk.c | 4 +--- >>> hw/i2c/imx_i2c.c | 6 +++++- >>> 2 files changed, 6 insertions(+), 4 deletions(-) >>> >>> diff --git a/hw/arm/imx25_pdk.c b/hw/arm/imx25_pdk.c >>> index 025b608..c6f04d3 100644 >>> --- a/hw/arm/imx25_pdk.c >>> +++ b/hw/arm/imx25_pdk.c >>> @@ -138,9 +138,7 @@ static void imx25_pdk_init(MachineState >>> *machine) >>> * We add it here (only on qtest usage) to be able to do a >>> bit >>> * of simple qtest. See "make check" for details. >>> */ >>> - i2c_create_slave((I2CBus *)qdev_get_child_bus(DEVICE(&s- >>>> soc.i2c[0]), >>> - "i2c"), >>> - "ds1338", 0x68); >>> + i2c_create_slave(s->soc.i2c[0].bus, "ds1338", 0x68); >>> } >>> } >>> >>> diff --git a/hw/i2c/imx_i2c.c b/hw/i2c/imx_i2c.c >>> index 37e5a62..7be10fb 100644 >>> --- a/hw/i2c/imx_i2c.c >>> +++ b/hw/i2c/imx_i2c.c >>> @@ -305,12 +305,16 @@ static const VMStateDescription >>> imx_i2c_vmstate = { >>> static void imx_i2c_realize(DeviceState *dev, Error **errp) >>> { >>> IMXI2CState *s = IMX_I2C(dev); >>> + static int bus_count; >> >> hmm, the static is ugly :/ >> >> Isn't there other ways to achieve this naming ? >> >> Thanks, >> >> C. >> > > I'm not seeing an obvious way around it. The busses are realized > independently (so I can't implement what we do with the aspeed i2c > busses), and it is named before fsl-imx25:fsl_imx25_realize() can apply > specific properties to the bus. > > If you have any suggestions, I'm all ears. What about that ? @@ -310,7 +310,7 @@ static void imx_i2c_realize(DeviceState IMX_I2C_MEM_SIZE); sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->iomem); sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq); - s->bus = i2c_init_bus(DEVICE(dev), "i2c"); + s->bus = i2c_init_bus(DEVICE(dev), NULL); } static void imx_i2c_class_init(ObjectClass *klass, void *data) Which should name automatically the I2C objects : (qemu) info qom-tree /machine (imx25-pdk-machine) /peripheral (container) /soc (fsl,imx25) /peripheral-anon (container) /unattached (container) /device[0] (arm926-arm-cpu) /unnamed-gpio-in[1] (irq) /unnamed-gpio-in[3] (irq) /unnamed-gpio-in[2] (irq) /unnamed-gpio-in[0] (irq) /device[15] (imx.i2c) /imx.i2c[0] (qemu:memory-region) /i2c-bus.0 (i2c-bus) /device[17] (imx.i2c) /imx.i2c[0] (qemu:memory-region) /i2c-bus.2 (i2c-bus) /device[16] (imx.i2c) /imx.i2c[0] (qemu:memory-region) /i2c-bus.1 (i2c-bus) .... Cheers, C.
On Thu, 2016-12-01 at 13:31 +0100, Cédric Le Goater wrote: > On 12/01/2016 01:42 AM, Alastair D'Silva wrote: > > On Wed, 2016-11-30 at 09:18 +0100, Cédric Le Goater wrote: > > > On 11/30/2016 06:36 AM, Alastair D'Silva wrote: <snip> > > > > diff --git a/hw/i2c/imx_i2c.c b/hw/i2c/imx_i2c.c > > > > index 37e5a62..7be10fb 100644 > > > > --- a/hw/i2c/imx_i2c.c > > > > +++ b/hw/i2c/imx_i2c.c > > > > @@ -305,12 +305,16 @@ static const VMStateDescription > > > > imx_i2c_vmstate = { > > > > static void imx_i2c_realize(DeviceState *dev, Error **errp) > > > > { > > > > IMXI2CState *s = IMX_I2C(dev); > > > > + static int bus_count; > > > > > > hmm, the static is ugly :/ > > > > > > Isn't there other ways to achieve this naming ? > > > > > > Thanks, > > > > > > C. > > > > > > > I'm not seeing an obvious way around it. The busses are realized > > independently (so I can't implement what we do with the aspeed i2c > > busses), and it is named before fsl-imx25:fsl_imx25_realize() can > > apply > > specific properties to the bus. > > > > If you have any suggestions, I'm all ears. > > What about that ? > > @@ -310,7 +310,7 @@ static void imx_i2c_realize(DeviceState > IMX_I2C_MEM_SIZE); > sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->iomem); > sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq); > - s->bus = i2c_init_bus(DEVICE(dev), "i2c"); > + s->bus = i2c_init_bus(DEVICE(dev), NULL); > } > > static void imx_i2c_class_init(ObjectClass *klass, void *data) > > Which should name automatically the I2C objects : > > (qemu) info qom-tree > /machine (imx25-pdk-machine) > /peripheral (container) > /soc (fsl,imx25) > /peripheral-anon (container) > /unattached (container) > /device[0] (arm926-arm-cpu) > /unnamed-gpio-in[1] (irq) > /unnamed-gpio-in[3] (irq) > /unnamed-gpio-in[2] (irq) > /unnamed-gpio-in[0] (irq) > > /device[15] (imx.i2c) > /imx.i2c[0] (qemu:memory-region) > /i2c-bus.0 (i2c-bus) > /device[17] (imx.i2c) > /imx.i2c[0] (qemu:memory-region) > /i2c-bus.2 (i2c-bus) > /device[16] (imx.i2c) > /imx.i2c[0] (qemu:memory-region) > /i2c-bus.1 (i2c-bus) > .... > > > Cheers, > > C. Oh, great, that looks like a much better solution, thanks :) -- Alastair D'Silva mob: 0423 762 819 skype: alastair_dsilva Twitter: @EvilDeece blog: http://alastair.d-silva.org
On 01/12/16 23:31, Cédric Le Goater wrote: > On 12/01/2016 01:42 AM, Alastair D'Silva wrote: >> On Wed, 2016-11-30 at 09:18 +0100, Cédric Le Goater wrote: >>> On 11/30/2016 06:36 AM, Alastair D'Silva wrote: >>>> From: Alastair D'Silva <alastair@d-silva.org> >>>> >>>> The imx25 chip provides 3 i2c buses, but they have all been named >>>> "i2c", which makes it difficult to predict which bus a device will >>>> be connected to when specified on the command line. >>>> >>>> This patch addresses the issue by naming the buses uniquely: >>>> i2c.0 i2c.1 i2c.2 >>>> >>>> Signed-off-by: Alastair D'Silva <alastair@d-silva.org> >>>> --- >>>> hw/arm/imx25_pdk.c | 4 +--- >>>> hw/i2c/imx_i2c.c | 6 +++++- >>>> 2 files changed, 6 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/hw/arm/imx25_pdk.c b/hw/arm/imx25_pdk.c >>>> index 025b608..c6f04d3 100644 >>>> --- a/hw/arm/imx25_pdk.c >>>> +++ b/hw/arm/imx25_pdk.c >>>> @@ -138,9 +138,7 @@ static void imx25_pdk_init(MachineState >>>> *machine) >>>> * We add it here (only on qtest usage) to be able to do a >>>> bit >>>> * of simple qtest. See "make check" for details. >>>> */ >>>> - i2c_create_slave((I2CBus *)qdev_get_child_bus(DEVICE(&s- >>>>> soc.i2c[0]), >>>> - "i2c"), >>>> - "ds1338", 0x68); >>>> + i2c_create_slave(s->soc.i2c[0].bus, "ds1338", 0x68); >>>> } >>>> } >>>> >>>> diff --git a/hw/i2c/imx_i2c.c b/hw/i2c/imx_i2c.c >>>> index 37e5a62..7be10fb 100644 >>>> --- a/hw/i2c/imx_i2c.c >>>> +++ b/hw/i2c/imx_i2c.c >>>> @@ -305,12 +305,16 @@ static const VMStateDescription >>>> imx_i2c_vmstate = { >>>> static void imx_i2c_realize(DeviceState *dev, Error **errp) >>>> { >>>> IMXI2CState *s = IMX_I2C(dev); >>>> + static int bus_count; >>> >>> hmm, the static is ugly :/ >>> >>> Isn't there other ways to achieve this naming ? >>> >>> Thanks, >>> >>> C. >>> >> >> I'm not seeing an obvious way around it. The busses are realized >> independently (so I can't implement what we do with the aspeed i2c >> busses), and it is named before fsl-imx25:fsl_imx25_realize() can apply >> specific properties to the bus. >> >> If you have any suggestions, I'm all ears. > > What about that ? > > @@ -310,7 +310,7 @@ static void imx_i2c_realize(DeviceState > IMX_I2C_MEM_SIZE); > sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->iomem); > sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq); > - s->bus = i2c_init_bus(DEVICE(dev), "i2c"); > + s->bus = i2c_init_bus(DEVICE(dev), NULL); > } > > static void imx_i2c_class_init(ObjectClass *klass, void *data) > > Which should name automatically the I2C objects : If you ever do migration, you'll have to specify "id" in the command line anyway. Even in the example below the buses are numbered in messed order, is that desired effect (may be it is, just asking :) )? > > (qemu) info qom-tree > /machine (imx25-pdk-machine) > /peripheral (container) > /soc (fsl,imx25) > /peripheral-anon (container) > /unattached (container) > /device[0] (arm926-arm-cpu) > /unnamed-gpio-in[1] (irq) > /unnamed-gpio-in[3] (irq) > /unnamed-gpio-in[2] (irq) > /unnamed-gpio-in[0] (irq) > > /device[15] (imx.i2c) > /imx.i2c[0] (qemu:memory-region) > /i2c-bus.0 (i2c-bus) > /device[17] (imx.i2c) > /imx.i2c[0] (qemu:memory-region) > /i2c-bus.2 (i2c-bus) > /device[16] (imx.i2c) > /imx.i2c[0] (qemu:memory-region) > /i2c-bus.1 (i2c-bus) > .... > > > Cheers, > > C. >
On 12/02/2016 12:34 AM, Alexey Kardashevskiy wrote: > On 01/12/16 23:31, Cédric Le Goater wrote: >> On 12/01/2016 01:42 AM, Alastair D'Silva wrote: >>> On Wed, 2016-11-30 at 09:18 +0100, Cédric Le Goater wrote: >>>> On 11/30/2016 06:36 AM, Alastair D'Silva wrote: >>>>> From: Alastair D'Silva <alastair@d-silva.org> >>>>> >>>>> The imx25 chip provides 3 i2c buses, but they have all been named >>>>> "i2c", which makes it difficult to predict which bus a device will >>>>> be connected to when specified on the command line. >>>>> >>>>> This patch addresses the issue by naming the buses uniquely: >>>>> i2c.0 i2c.1 i2c.2 >>>>> >>>>> Signed-off-by: Alastair D'Silva <alastair@d-silva.org> >>>>> --- >>>>> hw/arm/imx25_pdk.c | 4 +--- >>>>> hw/i2c/imx_i2c.c | 6 +++++- >>>>> 2 files changed, 6 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/hw/arm/imx25_pdk.c b/hw/arm/imx25_pdk.c >>>>> index 025b608..c6f04d3 100644 >>>>> --- a/hw/arm/imx25_pdk.c >>>>> +++ b/hw/arm/imx25_pdk.c >>>>> @@ -138,9 +138,7 @@ static void imx25_pdk_init(MachineState >>>>> *machine) >>>>> * We add it here (only on qtest usage) to be able to do a >>>>> bit >>>>> * of simple qtest. See "make check" for details. >>>>> */ >>>>> - i2c_create_slave((I2CBus *)qdev_get_child_bus(DEVICE(&s- >>>>>> soc.i2c[0]), >>>>> - "i2c"), >>>>> - "ds1338", 0x68); >>>>> + i2c_create_slave(s->soc.i2c[0].bus, "ds1338", 0x68); >>>>> } >>>>> } >>>>> >>>>> diff --git a/hw/i2c/imx_i2c.c b/hw/i2c/imx_i2c.c >>>>> index 37e5a62..7be10fb 100644 >>>>> --- a/hw/i2c/imx_i2c.c >>>>> +++ b/hw/i2c/imx_i2c.c >>>>> @@ -305,12 +305,16 @@ static const VMStateDescription >>>>> imx_i2c_vmstate = { >>>>> static void imx_i2c_realize(DeviceState *dev, Error **errp) >>>>> { >>>>> IMXI2CState *s = IMX_I2C(dev); >>>>> + static int bus_count; >>>> >>>> hmm, the static is ugly :/ >>>> >>>> Isn't there other ways to achieve this naming ? >>>> >>>> Thanks, >>>> >>>> C. >>>> >>> >>> I'm not seeing an obvious way around it. The busses are realized >>> independently (so I can't implement what we do with the aspeed i2c >>> busses), and it is named before fsl-imx25:fsl_imx25_realize() can apply >>> specific properties to the bus. >>> >>> If you have any suggestions, I'm all ears. >> >> What about that ? >> >> @@ -310,7 +310,7 @@ static void imx_i2c_realize(DeviceState >> IMX_I2C_MEM_SIZE); >> sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->iomem); >> sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq); >> - s->bus = i2c_init_bus(DEVICE(dev), "i2c"); >> + s->bus = i2c_init_bus(DEVICE(dev), NULL); >> } >> >> static void imx_i2c_class_init(ObjectClass *klass, void *data) >> >> Which should name automatically the I2C objects : > > > If you ever do migration, you'll have to specify "id" in the command line > anyway. Even in the example below the buses are numbered in messed order, > is that desired effect (may be it is, just asking :) )? That's how it comes out with qom-tree. I haven't dug deeper to see how this was implemented. C. >> >> (qemu) info qom-tree >> /machine (imx25-pdk-machine) >> /peripheral (container) >> /soc (fsl,imx25) >> /peripheral-anon (container) >> /unattached (container) >> /device[0] (arm926-arm-cpu) >> /unnamed-gpio-in[1] (irq) >> /unnamed-gpio-in[3] (irq) >> /unnamed-gpio-in[2] (irq) >> /unnamed-gpio-in[0] (irq) >> >> /device[15] (imx.i2c) >> /imx.i2c[0] (qemu:memory-region) >> /i2c-bus.0 (i2c-bus) >> /device[17] (imx.i2c) >> /imx.i2c[0] (qemu:memory-region) >> /i2c-bus.2 (i2c-bus) >> /device[16] (imx.i2c) >> /imx.i2c[0] (qemu:memory-region) >> /i2c-bus.1 (i2c-bus) >> .... >> >> >> Cheers, >> >> C. >> > >
On 02/12/16 18:55, Cédric Le Goater wrote: > On 12/02/2016 12:34 AM, Alexey Kardashevskiy wrote: >> On 01/12/16 23:31, Cédric Le Goater wrote: >>> On 12/01/2016 01:42 AM, Alastair D'Silva wrote: >>>> On Wed, 2016-11-30 at 09:18 +0100, Cédric Le Goater wrote: >>>>> On 11/30/2016 06:36 AM, Alastair D'Silva wrote: >>>>>> From: Alastair D'Silva <alastair@d-silva.org> >>>>>> >>>>>> The imx25 chip provides 3 i2c buses, but they have all been named >>>>>> "i2c", which makes it difficult to predict which bus a device will >>>>>> be connected to when specified on the command line. >>>>>> >>>>>> This patch addresses the issue by naming the buses uniquely: >>>>>> i2c.0 i2c.1 i2c.2 >>>>>> >>>>>> Signed-off-by: Alastair D'Silva <alastair@d-silva.org> >>>>>> --- >>>>>> hw/arm/imx25_pdk.c | 4 +--- >>>>>> hw/i2c/imx_i2c.c | 6 +++++- >>>>>> 2 files changed, 6 insertions(+), 4 deletions(-) >>>>>> >>>>>> diff --git a/hw/arm/imx25_pdk.c b/hw/arm/imx25_pdk.c >>>>>> index 025b608..c6f04d3 100644 >>>>>> --- a/hw/arm/imx25_pdk.c >>>>>> +++ b/hw/arm/imx25_pdk.c >>>>>> @@ -138,9 +138,7 @@ static void imx25_pdk_init(MachineState >>>>>> *machine) >>>>>> * We add it here (only on qtest usage) to be able to do a >>>>>> bit >>>>>> * of simple qtest. See "make check" for details. >>>>>> */ >>>>>> - i2c_create_slave((I2CBus *)qdev_get_child_bus(DEVICE(&s- >>>>>>> soc.i2c[0]), >>>>>> - "i2c"), >>>>>> - "ds1338", 0x68); >>>>>> + i2c_create_slave(s->soc.i2c[0].bus, "ds1338", 0x68); >>>>>> } >>>>>> } >>>>>> >>>>>> diff --git a/hw/i2c/imx_i2c.c b/hw/i2c/imx_i2c.c >>>>>> index 37e5a62..7be10fb 100644 >>>>>> --- a/hw/i2c/imx_i2c.c >>>>>> +++ b/hw/i2c/imx_i2c.c >>>>>> @@ -305,12 +305,16 @@ static const VMStateDescription >>>>>> imx_i2c_vmstate = { >>>>>> static void imx_i2c_realize(DeviceState *dev, Error **errp) >>>>>> { >>>>>> IMXI2CState *s = IMX_I2C(dev); >>>>>> + static int bus_count; >>>>> >>>>> hmm, the static is ugly :/ >>>>> >>>>> Isn't there other ways to achieve this naming ? >>>>> >>>>> Thanks, >>>>> >>>>> C. >>>>> >>>> >>>> I'm not seeing an obvious way around it. The busses are realized >>>> independently (so I can't implement what we do with the aspeed i2c >>>> busses), and it is named before fsl-imx25:fsl_imx25_realize() can apply >>>> specific properties to the bus. >>>> >>>> If you have any suggestions, I'm all ears. >>> >>> What about that ? >>> >>> @@ -310,7 +310,7 @@ static void imx_i2c_realize(DeviceState >>> IMX_I2C_MEM_SIZE); >>> sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->iomem); >>> sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq); >>> - s->bus = i2c_init_bus(DEVICE(dev), "i2c"); >>> + s->bus = i2c_init_bus(DEVICE(dev), NULL); >>> } >>> >>> static void imx_i2c_class_init(ObjectClass *klass, void *data) >>> >>> Which should name automatically the I2C objects : >> >> >> If you ever do migration, you'll have to specify "id" in the command line >> anyway. Even in the example below the buses are numbered in messed order, >> is that desired effect (may be it is, just asking :) )? > > That's how it comes out with qom-tree. I haven't dug deeper to see > how this was implemented. That's fine, your approach will give unique names, it is just hard to predict what i2c device will end up connected to which i2c bus, and I usually want to control this instead of guessing (which will work till some nonobvious change in QOM :) ). > > C. > >>> >>> (qemu) info qom-tree >>> /machine (imx25-pdk-machine) >>> /peripheral (container) >>> /soc (fsl,imx25) >>> /peripheral-anon (container) >>> /unattached (container) >>> /device[0] (arm926-arm-cpu) >>> /unnamed-gpio-in[1] (irq) >>> /unnamed-gpio-in[3] (irq) >>> /unnamed-gpio-in[2] (irq) >>> /unnamed-gpio-in[0] (irq) >>> >>> /device[15] (imx.i2c) >>> /imx.i2c[0] (qemu:memory-region) >>> /i2c-bus.0 (i2c-bus) >>> /device[17] (imx.i2c) >>> /imx.i2c[0] (qemu:memory-region) >>> /i2c-bus.2 (i2c-bus) >>> /device[16] (imx.i2c) >>> /imx.i2c[0] (qemu:memory-region) >>> /i2c-bus.1 (i2c-bus) >>> .... >>> >>> >>> Cheers, >>> >>> C. >>> >> >> >
On 12/03/2016 08:16 AM, Alexey Kardashevskiy wrote: > On 02/12/16 18:55, Cédric Le Goater wrote: >> On 12/02/2016 12:34 AM, Alexey Kardashevskiy wrote: >>> On 01/12/16 23:31, Cédric Le Goater wrote: >>>> On 12/01/2016 01:42 AM, Alastair D'Silva wrote: >>>>> On Wed, 2016-11-30 at 09:18 +0100, Cédric Le Goater wrote: >>>>>> On 11/30/2016 06:36 AM, Alastair D'Silva wrote: >>>>>>> From: Alastair D'Silva <alastair@d-silva.org> >>>>>>> >>>>>>> The imx25 chip provides 3 i2c buses, but they have all been named >>>>>>> "i2c", which makes it difficult to predict which bus a device will >>>>>>> be connected to when specified on the command line. >>>>>>> >>>>>>> This patch addresses the issue by naming the buses uniquely: >>>>>>> i2c.0 i2c.1 i2c.2 >>>>>>> >>>>>>> Signed-off-by: Alastair D'Silva <alastair@d-silva.org> >>>>>>> --- >>>>>>> hw/arm/imx25_pdk.c | 4 +--- >>>>>>> hw/i2c/imx_i2c.c | 6 +++++- >>>>>>> 2 files changed, 6 insertions(+), 4 deletions(-) >>>>>>> >>>>>>> diff --git a/hw/arm/imx25_pdk.c b/hw/arm/imx25_pdk.c >>>>>>> index 025b608..c6f04d3 100644 >>>>>>> --- a/hw/arm/imx25_pdk.c >>>>>>> +++ b/hw/arm/imx25_pdk.c >>>>>>> @@ -138,9 +138,7 @@ static void imx25_pdk_init(MachineState >>>>>>> *machine) >>>>>>> * We add it here (only on qtest usage) to be able to do a >>>>>>> bit >>>>>>> * of simple qtest. See "make check" for details. >>>>>>> */ >>>>>>> - i2c_create_slave((I2CBus *)qdev_get_child_bus(DEVICE(&s- >>>>>>>> soc.i2c[0]), >>>>>>> - "i2c"), >>>>>>> - "ds1338", 0x68); >>>>>>> + i2c_create_slave(s->soc.i2c[0].bus, "ds1338", 0x68); >>>>>>> } >>>>>>> } >>>>>>> >>>>>>> diff --git a/hw/i2c/imx_i2c.c b/hw/i2c/imx_i2c.c >>>>>>> index 37e5a62..7be10fb 100644 >>>>>>> --- a/hw/i2c/imx_i2c.c >>>>>>> +++ b/hw/i2c/imx_i2c.c >>>>>>> @@ -305,12 +305,16 @@ static const VMStateDescription >>>>>>> imx_i2c_vmstate = { >>>>>>> static void imx_i2c_realize(DeviceState *dev, Error **errp) >>>>>>> { >>>>>>> IMXI2CState *s = IMX_I2C(dev); >>>>>>> + static int bus_count; >>>>>> >>>>>> hmm, the static is ugly :/ >>>>>> >>>>>> Isn't there other ways to achieve this naming ? >>>>>> >>>>>> Thanks, >>>>>> >>>>>> C. >>>>>> >>>>> >>>>> I'm not seeing an obvious way around it. The busses are realized >>>>> independently (so I can't implement what we do with the aspeed i2c >>>>> busses), and it is named before fsl-imx25:fsl_imx25_realize() can apply >>>>> specific properties to the bus. >>>>> >>>>> If you have any suggestions, I'm all ears. >>>> >>>> What about that ? >>>> >>>> @@ -310,7 +310,7 @@ static void imx_i2c_realize(DeviceState >>>> IMX_I2C_MEM_SIZE); >>>> sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->iomem); >>>> sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq); >>>> - s->bus = i2c_init_bus(DEVICE(dev), "i2c"); >>>> + s->bus = i2c_init_bus(DEVICE(dev), NULL); >>>> } >>>> >>>> static void imx_i2c_class_init(ObjectClass *klass, void *data) >>>> >>>> Which should name automatically the I2C objects : >>> >>> >>> If you ever do migration, you'll have to specify "id" in the command line >>> anyway. Even in the example below the buses are numbered in messed order, >>> is that desired effect (may be it is, just asking :) )? >> >> That's how it comes out with qom-tree. I haven't dug deeper to see >> how this was implemented. > > That's fine, your approach will give unique names, it is just hard to > predict what i2c device will end up connected to which i2c bus, and I > usually want to control this instead of guessing (which will work till some > nonobvious change in QOM :) ). yes so we could also set an id property before doing realize I agree. but for the purpose of this test, I don't think this is really needed. Thanks, C.
diff --git a/hw/arm/imx25_pdk.c b/hw/arm/imx25_pdk.c index 025b608..c6f04d3 100644 --- a/hw/arm/imx25_pdk.c +++ b/hw/arm/imx25_pdk.c @@ -138,9 +138,7 @@ static void imx25_pdk_init(MachineState *machine) * We add it here (only on qtest usage) to be able to do a bit * of simple qtest. See "make check" for details. */ - i2c_create_slave((I2CBus *)qdev_get_child_bus(DEVICE(&s->soc.i2c[0]), - "i2c"), - "ds1338", 0x68); + i2c_create_slave(s->soc.i2c[0].bus, "ds1338", 0x68); } } diff --git a/hw/i2c/imx_i2c.c b/hw/i2c/imx_i2c.c index 37e5a62..7be10fb 100644 --- a/hw/i2c/imx_i2c.c +++ b/hw/i2c/imx_i2c.c @@ -305,12 +305,16 @@ static const VMStateDescription imx_i2c_vmstate = { static void imx_i2c_realize(DeviceState *dev, Error **errp) { IMXI2CState *s = IMX_I2C(dev); + static int bus_count; + char name[16]; + + snprintf(name, sizeof(name), "i2c.%d", bus_count++); memory_region_init_io(&s->iomem, OBJECT(s), &imx_i2c_ops, s, TYPE_IMX_I2C, IMX_I2C_MEM_SIZE); sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->iomem); sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq); - s->bus = i2c_init_bus(DEVICE(dev), "i2c"); + s->bus = i2c_init_bus(DEVICE(dev), name); } static void imx_i2c_class_init(ObjectClass *klass, void *data)