Re: [Qemu-devel] [PATCH v2 1/6] arm: Uniquely name imx25 I2C buses.

2016-12-02 Thread Cédric Le Goater 
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 
>>>
>>> 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 
>>> ---
>>>  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(
 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), >iomem);
 sysbus_init_irq(SYS_BUS_DEVICE(dev), >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.

Re: [Qemu-devel] [PATCH v2 1/6] arm: Uniquely name imx25 I2C buses.

2016-12-02 Thread Alexey Kardashevskiy 
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 
>>
>> 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 
>> ---
>>  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(
>>> 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), >iomem);
>>>  sysbus_init_irq(SYS_BUS_DEVICE(dev), >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. 
>>>
>>
>>
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH v2 1/6] arm: Uniquely name imx25 I2C buses.

2016-12-01 Thread Cédric Le Goater 
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 
>
> 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 
> ---
>  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(
>> 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), >iomem);
>>   sysbus_init_irq(SYS_BUS_DEVICE(dev), >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. 
>>
> 
>

Re: [Qemu-devel] [PATCH v2 1/6] arm: Uniquely name imx25 I2C buses.

2016-12-01 Thread Alexey Kardashevskiy 
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 

 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 
 ---
  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(
> 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), >iomem);
>sysbus_init_irq(SYS_BUS_DEVICE(dev), >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. 
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH v2 1/6] arm: Uniquely name imx25 I2C buses.

2016-12-01 Thread Alastair D'Silva 
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:

> > > > 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), >iomem);
>    sysbus_init_irq(SYS_BUS_DEVICE(dev), >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

Re: [Qemu-devel] [PATCH v2 1/6] arm: Uniquely name imx25 I2C buses.

2016-12-01 Thread Cédric Le Goater 
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 
>>>
>>> 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 
>>> ---
>>>  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(
 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), >iomem);
 sysbus_init_irq(SYS_BUS_DEVICE(dev), >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.

Re: [Qemu-devel] [PATCH v2 1/6] arm: Uniquely name imx25 I2C buses.

2016-11-30 Thread Alexey Kardashevskiy 
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 
>>>
>>> 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 
>>> ---
>>>  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(
 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.


-- 
Alexey

Re: [Qemu-devel] [PATCH v2 1/6] arm: Uniquely name imx25 I2C buses.

2016-11-30 Thread Alastair D'Silva 
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 
> > 
> > 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 
> > ---
> >  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(
> > >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

Re: [Qemu-devel] [PATCH v2 1/6] arm: Uniquely name imx25 I2C buses.

2016-11-30 Thread Cédric Le Goater 
On 11/30/2016 06:36 AM, Alastair D'Silva wrote:
> From: Alastair D'Silva 
> 
> 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 
> ---
>  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(>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(>iomem, OBJECT(s), _i2c_ops, s, 
> TYPE_IMX_I2C,
>IMX_I2C_MEM_SIZE);
>  sysbus_init_mmio(SYS_BUS_DEVICE(dev), >iomem);
>  sysbus_init_irq(SYS_BUS_DEVICE(dev), >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)
>