Re: [PATCH v5 1/9] hw/i2c/core: Add i2c_try_create_slave() and i2c_realize_and_unref()

[Philippe Mathieu-Daudé]

On 6/23/20 10:01 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> 
>> Extract i2c_try_create_slave() and i2c_realize_and_unref()
>> from i2c_create_slave().
>> We can now set properties on a I2CSlave before it is realized.
>>
>> This is in line with the recent qdev/QOM changes merged
>> in commit 6675a653d2e.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> Cc: Markus Armbruster <armbru@redhat.com>
>> ---
>>  include/hw/i2c/i2c.h |  2 ++
>>  hw/i2c/core.c        | 18 ++++++++++++++++--
>>  2 files changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
>> index 4117211565..d6e3d85faf 100644
>> --- a/include/hw/i2c/i2c.h
>> +++ b/include/hw/i2c/i2c.h
>> @@ -80,6 +80,8 @@ int i2c_send(I2CBus *bus, uint8_t data);
>>  uint8_t i2c_recv(I2CBus *bus);
>>  
>>  DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr);
>> +DeviceState *i2c_try_create_slave(const char *name, uint8_t addr);
>> +bool i2c_realize_and_unref(DeviceState *dev, I2CBus *bus, Error **errp);
>>  
>>  /* lm832x.c */
>>  void lm832x_key_event(DeviceState *dev, int key, int state);
>> diff --git a/hw/i2c/core.c b/hw/i2c/core.c
>> index 1aac457a2a..acf34a12d6 100644
>> --- a/hw/i2c/core.c
>> +++ b/hw/i2c/core.c
>> @@ -267,13 +267,27 @@ const VMStateDescription vmstate_i2c_slave = {
>>      }
>>  };
>>  
>> -DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr)
>> +DeviceState *i2c_try_create_slave(const char *name, uint8_t addr)
>>  {
>>      DeviceState *dev;
>>  
>>      dev = qdev_new(name);
>>      qdev_prop_set_uint8(dev, "address", addr);
>> -    qdev_realize_and_unref(dev, &bus->qbus, &error_fatal);
>> +    return dev;
>> +}
>> +
>> +bool i2c_realize_and_unref(DeviceState *dev, I2CBus *bus, Error **errp)
>> +{
>> +    return qdev_realize_and_unref(dev, &bus->qbus, errp);
>> +}
>> +
>> +DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr)
>> +{
>> +    DeviceState *dev;
>> +
>> +    dev = i2c_try_create_slave(name, addr);
>> +    i2c_realize_and_unref(dev, bus, &error_fatal);
>> +
>>      return dev;
>>  }
> 
> We use "create_simple" names for functions that allocate, initialize,
> configure and realize device objects: pci_create_simple(),
> isa_create_simple(), usb_create_simple().  Calling this one
> i2c_create_slave() is okay with me.  I'd prefer
> i2c_slave_create_simple(), though.
> 
> We use "new" names for functions that allocate and initialize device
> objects: pci_new(), isa_new(), usb_new().  Let's call this one
> i2c_slave_new().
> 
> Your use of "realize_and_unref" matches existing names elsewhere:
> pci_realize_and_unref(), isa_realize_and_unref(),
> usb_realize_and_unref().  However, the other two i2c functions are
> called i2c_slave_FOO(), not i2c_FOO().  You could name this one
> i2c_slave_realize_and_unref().  Another path to consistency: drop the
> slave_ from all three names.
> 
> Ideally with my naming suggestions considered:
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

If you don't mind, as this series is already fully reviewed,
I'll correct/improve on top of it.

Thanks!

Phil.