On 6/30/20 12:15 PM, Markus Armbruster wrote: > Philippe Mathieu-Daudé <f4...@amsat.org> writes: > >> In commit d88c42ff2c we added new prototype but neglected to >> add their documentation. Fix that. >> >> Reported-by: Peter Maydell <peter.mayd...@linaro.org> >> Signed-off-by: Philippe Mathieu-Daudé <f4...@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. > >
