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

[Markus Armbruster <armbru@redhat.com>]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Tue, 30 Jun 2020 at 11:15, Markus Armbruster <armbru@redhat.com> 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>
>
>> > + * 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 ;)
>
> If qdev_realize_and_unref() has documentation which gives
> the use-cases similar to the text above, then we could make
> this text say "This function follows the patterns and
> intended usecases for qdev_realize_and_unref(); see the
> documentation for that function for whether you would be
> better off using i2c_realize() or (the not-yet-existing)
> i2c_slave_realize()" or similar. I originally wrote the
> version of the above text for ssi_realize_and_unref()
> as essentially the documentation I would have liked
> qdev_realize_and_unref() to have, ie including the nuances
> which I had to figure out for myself.

To document wrappers as simple as ssi_realize_and_unref(), pointing to
the wrapped function should suffice.  When more elaborate documentation
is wanted, it's probably wanted for the wrapped function.

Since you felt a need for a more elaborate ssi_realize_and_unref() doc
comment, you should probably propose a patch for
qdev_realize_and_unref()'s doc comment :)