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

["Cédric Le Goater" <clg@kaod.org>]

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.