Re: [PATCH 5/5] hw/i2c: Document the I2C qdev helpers

[Philippe Mathieu-Daudé]

On 6/30/20 12:15 PM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> 
>> In commit d88c42ff2c we added new prototype but neglected to
>> add their documentation. Fix that.
>>
>> Reported-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  include/hw/i2c/i2c.h | 48 ++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 48 insertions(+)
>>
>> diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
>> index c533058998..fcc61e509b 100644
>> --- a/include/hw/i2c/i2c.h
>> +++ b/include/hw/i2c/i2c.h
>> @@ -79,8 +79,56 @@ int i2c_send_recv(I2CBus *bus, uint8_t *data, bool send);
>>  int i2c_send(I2CBus *bus, uint8_t data);
>>  uint8_t i2c_recv(I2CBus *bus);
>>  
>> +/**
>> + * Create an I2C slave device on the heap.
>> + * @name: a device type name
>> + * @addr: I2C address of the slave when put on a bus
>> + *
>> + * This only initializes the device state structure and allows
>> + * properties to be set. Type @name must exist. The device still
>> + * needs to be realized. See qdev-core.h.
>> + */
>>  I2CSlave *i2c_slave_new(const char *name, uint8_t addr);
>> +
>> +/**
>> + * Create an I2C slave device on the heap.
> 
> Suggest "Create and realize ..."
> 
>> + * @bus: I2C bus to put it on
>> + * @name: I2C slave device type name
>> + * @addr: I2C address of the slave when put on a bus
>> + *
>> + * Create the device state structure, initialize it, put it on the
>> + * specified @bus, and drop the reference to it (the device is realized).
>> + * Any error aborts the process.
> 
> Stick to imperative mood: Abort on error.
> 
> Do we need the sentence?  Doc comments of object_new(), qdev_new() and
> i2c_slave_new() don't have it, they document *preconditions* instead,
> using "must", and rely on the tacit understanding that a function may
> abort or crash when its documented preconditions aren't met.  Matter of
> taste, I guess.
> 
>> + */
>>  I2CSlave *i2c_slave_create_simple(I2CBus *bus, const char *name, uint8_t 
>> addr);
>> +
>> +/**
>> + * i2c_slave_realize_and_unref: realize and unref an I2C slave device
> 
> Either consistently waste space for repeating the function name at the
> beginning of its doc comment, or consistently don't :)
> 
> qdev_realize_and_unref()'s doc comment says "and drop a reference"
> instead of "unref", because "unref" is not a word.
> 
>> + * @dev: I2C slave device to realize
>> + * @bus: I2C bus to put it on
>> + * @addr: I2C address of the slave on the bus
>> + * @errp: error pointer
> 
> $ git-grep -h "@errp:" | sort -u
>  *  @errp: pointer to Error*, to store an error if it happens
>  * @errp:   error object
>  * @errp: Error object
>  * @errp: Error object which may be set by job_complete(); this is not
>  * @errp: Error object.
>  * @errp: If an error occurs, a pointer to an area to store the error
>  * @errp: Output pointer for error information. Can be NULL.
>  * @errp: Pointer for reporting an #Error.
>  * @errp: Populated with an error in failure cases
>  * @errp: a pointer to an Error that is filled if getting/setting fails.
>  * @errp: a pointer to return the #Error object if an error occurs.
>  * @errp: an error indicator
>  * @errp: error
>  * @errp: error object
>  * @errp: error object handle
>  * @errp: handle to an error object
>  * @errp: if an error occurs, a pointer to an area to store the error
>  * @errp: indirect pointer to Error to be set
>  * @errp: location to store error
>  * @errp: location to store error information
>  * @errp: location to store error information.
>  * @errp: location to store error, will be set only for exception
>  * @errp: pointer to Error*, to store an error if it happens.
>  * @errp: pointer to NULL initialized error object
>  * @errp: pointer to a NULL initialized error object
>  * @errp: pointer to a NULL-initialized error object
>  * @errp: pointer to an error
>  * @errp: pointer to error object
>  * @errp: pointer to initialized error object
>  * @errp: pointer to uninitialized error object
> 
> Aside: gotta love these two.
> 
>  * @errp: returns an error if this function fails
>  * @errp: set *errp if the check failed, with reason
>  * @errp: set in case of an error
>  * @errp: set on error
>  * @errp: unused
>  * @errp: where to put errors
> 
> Plenty of choice, recommend not to invent another one :)
> 
>> + *
>> + * Call 'realize' on @dev, put it on the specified @bus, and drop the
>> + * reference to it. Errors are reported via @errp and by returning
>> + * false.
> 
> Recommend to use a separate paragraph for the return value.  Since your
> comment style resembles GTK-Doc style[*], you may just as well use it
> for the return value, like this:
> 
>       Returns: %true on success, %false on failure.
> 
> By convention, it goes after the function description.

OK, I'll use whatever you prefer. Maybe the shorter the easier.

I will see if I can find to spend more time on this during the
week-end, but I can't promise anything. Anyway since it is
documentation it can be integrated during soft freeze.

Thanks for your detailed review.

> 
>> + *
>> + * This function is useful if you have created @dev via qdev_new(),
>> + * i2c_slave_new() or i2c_slave_try_new() (which take a reference to
>> + * the device it returns to you), so that you can set properties on it
>> + * before realizing it. If you don't need to set properties then
>> + * i2c_slave_create_simple() is probably better (as it does the create,
>> + * init and realize in one step).
>> + *
>> + * If you are embedding the I2C slave into another QOM device and
>> + * initialized it via some variant on object_initialize_child() then
>> + * do not use this function, because that family of functions arrange
>> + * for the only reference to the child device to be held by the parent
>> + * via the child<> property, and so the reference-count-drop done here
>> + * would be incorrect.  (Instead you would want i2c_slave_realize(),
>> + * which doesn't currently exist but would be trivial to create if we
>> + * had any code that wanted it.)
>> + */
> 
> The advice on use is more elaborate qdev_realize_and_unref()'s.  That
> one simply shows intended use.  I doubt we need more.  But as the person
> who wrote qdev_realize_and_unref(), I'm singularly unqualified judging
> the need ;)
> 
>>  bool i2c_slave_realize_and_unref(I2CSlave *dev, I2CBus *bus, Error **errp);
>>  
>>  /* lm832x.c */
> 
> 
> [*] A style I dislike, but it's common in QEMU, so you're certainly
> entitled to use it.
> 
>