Re: [Qemu-devel] [PATCH] hw/i2c: QOM'ify i2c slave

[Philippe Mathieu-Daudé]

On 01/14/2018 12:34 PM, Peter Maydell wrote:
> On 14 January 2018 at 02:45, Philippe Mathieu-Daudé <address@hidden> wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
>> ---
>>  hw/i2c/core.c | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/i2c/core.c b/hw/i2c/core.c
>> index 59068f157e..c84dbfb884 100644
>> --- a/hw/i2c/core.c
>> +++ b/hw/i2c/core.c
>> @@ -8,6 +8,7 @@
>>   */
>>
>>  #include "qemu/osdep.h"
>> +#include "qapi/error.h"
>>  #include "hw/i2c/i2c.h"
>>
>>  typedef struct I2CNode I2CNode;
>> @@ -276,16 +277,15 @@ const VMStateDescription vmstate_i2c_slave = {
>>      }
>>  };
>>
>> -static int i2c_slave_qdev_init(DeviceState *dev)
>> +static void i2c_slave_realize(DeviceState *dev, Error **errp)
>>  {
>>      I2CSlave *s = I2C_SLAVE(dev);
>>      I2CSlaveClass *sc = I2C_SLAVE_GET_CLASS(s);
>>
>> -    if (sc->init) {
>> -        return sc->init(s);
>> +    if (sc->init && sc->init(s)) {
>> +        error_setg(errp, "i2c slave initialization failed");
>> +        return;
>>      }
>> -
>> -    return 0;
>>  }
>>
>>  DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr)
>> @@ -301,7 +301,7 @@ DeviceState *i2c_create_slave(I2CBus *bus, const char 
>> *name, uint8_t addr)
>>  static void i2c_slave_class_init(ObjectClass *klass, void *data)
>>  {
>>      DeviceClass *k = DEVICE_CLASS(klass);
>> -    k->init = i2c_slave_qdev_init;
>> +    k->realize = i2c_slave_realize;
>>      set_bit(DEVICE_CATEGORY_MISC, k->categories);
>>      k->bus_type = TYPE_I2C_BUS;
>>      k->props = i2c_props;
> 
> This is changing the semantics of the I2CSlaveClass::init
> method. Is that really OK? (If nothing else, it means
> that we end up with a method named init which is called
> at realize time, which is confusing, and which doesn't
> have an API like realize which allows it to fill in
> an Error**.)

I see your point and missed it.

I'll respin this patch once I2CSlaveClass is correctly converted to
realize().

Thanks,

Phil.