Re: [PATCH v4 4/8] hw/misc/pca9552: Add a 'description' property for debugging purpose

[Philippe Mathieu-Daudé]

On 6/25/20 8:37 AM, Markus Armbruster wrote:
> Cédric Le Goater <clg@kaod.org> writes:
> 
>> On 6/22/20 10:31 AM, Philippe Mathieu-Daudé wrote:
>>> On 6/22/20 8:27 AM, Cédric Le Goater wrote:
>>>> On 6/21/20 12:58 AM, Philippe Mathieu-Daudé wrote:
>>>>> Add a description field to distinguish between multiple devices.
> 
> Pardon my lack of imagination: how does this help you with debugging?

Ah, the patch subject is indeed incorrect, this should be:
"... for *tracing* purpose" (I use tracing when debugging).

In the next patch, we use the 'description' property:

+# pca9552.c
+pca9552_gpio_status(const char *description, const char *buf) "%s GPIOs
0-15 [%s]"

So when the machine has multiple PCA9552 and guest accesses both,
we can distinct which one is used. For me having "pca1" / "pca0"
is easier to follow than the address of the QOM object.

> 
>>>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>>>>
>>>> Could it be a QOM attribute ? 
>>>
>>> What do you call a 'QOM attribute'?
>>> Is it what qdev properties implement?
>>> (in this case via DEFINE_PROP_STRING).
>>
>> I meant a default Object property, which would apply to all Objects. 
> 
> Good point.  Many devices have multiple component objects of the same
> type.
> 
>> What you did is fine, so :
>>
>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>>
>> but, may be, a well defined child name is enough for the purpose.
> 
> object_get_canonical_path() returns a distinct path for each (component)
> object.  The path components are the child property names.
> 
> Properties can have descriptions: object_property_set_description().

TIL object_property_set_description :>

Ah, there is no equivalent object_property_get_description(),
we have to use object_get_canonical_path(). Hmm, not obvious.

> 
> Sufficient?

I don't know... This seems a complex way to do something simple...
This is already a QDEV. Having to use QOM API seems going
backward, since we have the DEFINE_PROP_STRING() macros available
in "hw/qdev-properties.h".

Maybe I'm not seeing the advantages clearly. I'll try later.

Thanks for your review,

Phil.