* [PATCH v5 1/9] hw/i2c/core: Add i2c_try_create_slave() and i2c_realize_and_unref()
2020-06-22 18:34 [PATCH v5 0/9] hw/misc/pca9552: Trace GPIO change events Philippe Mathieu-Daudé
@ 2020-06-22 18:34 ` Philippe Mathieu-Daudé
2020-06-22 20:46 ` Corey Minyard
` (2 more replies)
2020-06-22 18:34 ` [PATCH v5 2/9] hw/misc/pca9552: Rename 'nr_leds' as 'pin_count' Philippe Mathieu-Daudé
` (7 subsequent siblings)
8 siblings, 3 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-22 18:34 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Corey Minyard, Andrew Jeffery,
Philippe Mathieu-Daudé,
Markus Armbruster, qemu-arm, Joel Stanley, Cédric Le Goater
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;
}
--
2.21.3
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v5 1/9] hw/i2c/core: Add i2c_try_create_slave() and i2c_realize_and_unref()
2020-06-22 18:34 ` [PATCH v5 1/9] hw/i2c/core: Add i2c_try_create_slave() and i2c_realize_and_unref() Philippe Mathieu-Daudé
@ 2020-06-22 20:46 ` Corey Minyard
2020-06-23 6:06 ` Cédric Le Goater
2020-06-23 8:01 ` Markus Armbruster
2 siblings, 0 replies; 22+ messages in thread
From: Corey Minyard @ 2020-06-22 20:46 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Andrew Jeffery, qemu-devel, Markus Armbruster,
qemu-arm, Joel Stanley, Cédric Le Goater
On Mon, Jun 22, 2020 at 08:34:20PM +0200, Philippe Mathieu-Daudé wrote:
> 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.
This seems consistent with outher things and looks proper.
Reviewed-by: Corey Minyard <cminyard@mvista.com>
>
> 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;
> }
>
> --
> 2.21.3
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v5 1/9] hw/i2c/core: Add i2c_try_create_slave() and i2c_realize_and_unref()
2020-06-22 18:34 ` [PATCH v5 1/9] hw/i2c/core: Add i2c_try_create_slave() and i2c_realize_and_unref() Philippe Mathieu-Daudé
2020-06-22 20:46 ` Corey Minyard
@ 2020-06-23 6:06 ` Cédric Le Goater
2020-06-23 8:01 ` Markus Armbruster
2 siblings, 0 replies; 22+ messages in thread
From: Cédric Le Goater @ 2020-06-23 6:06 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Peter Maydell, Corey Minyard, Andrew Jeffery, Markus Armbruster,
qemu-arm, Joel Stanley
On 6/22/20 8:34 PM, Philippe Mathieu-Daudé wrote:
> 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>
Reviewed-by: Cédric Le Goater <clg@kaod.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;
> }
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v5 1/9] hw/i2c/core: Add i2c_try_create_slave() and i2c_realize_and_unref()
2020-06-22 18:34 ` [PATCH v5 1/9] hw/i2c/core: Add i2c_try_create_slave() and i2c_realize_and_unref() Philippe Mathieu-Daudé
2020-06-22 20:46 ` Corey Minyard
2020-06-23 6:06 ` Cédric Le Goater
@ 2020-06-23 8:01 ` Markus Armbruster
2020-06-23 11:04 ` Philippe Mathieu-Daudé
2 siblings, 1 reply; 22+ messages in thread
From: Markus Armbruster @ 2020-06-23 8:01 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Corey Minyard, Andrew Jeffery, qemu-devel,
qemu-arm, Joel Stanley, Cédric Le Goater
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>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v5 1/9] hw/i2c/core: Add i2c_try_create_slave() and i2c_realize_and_unref()
2020-06-23 8:01 ` Markus Armbruster
@ 2020-06-23 11:04 ` Philippe Mathieu-Daudé
2020-06-23 13:28 ` Markus Armbruster
0 siblings, 1 reply; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-23 11:04 UTC (permalink / raw)
To: Markus Armbruster
Cc: Peter Maydell, Corey Minyard, Andrew Jeffery, qemu-devel,
qemu-arm, Joel Stanley, Cédric Le Goater
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.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v5 1/9] hw/i2c/core: Add i2c_try_create_slave() and i2c_realize_and_unref()
2020-06-23 11:04 ` Philippe Mathieu-Daudé
@ 2020-06-23 13:28 ` Markus Armbruster
0 siblings, 0 replies; 22+ messages in thread
From: Markus Armbruster @ 2020-06-23 13:28 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Corey Minyard, Andrew Jeffery, qemu-devel,
qemu-arm, Joel Stanley, Cédric Le Goater
Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> On 6/23/20 10:01 AM, Markus Armbruster wrote:
[...]
>> 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.
Up to you!
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v5 2/9] hw/misc/pca9552: Rename 'nr_leds' as 'pin_count'
2020-06-22 18:34 [PATCH v5 0/9] hw/misc/pca9552: Trace GPIO change events Philippe Mathieu-Daudé
2020-06-22 18:34 ` [PATCH v5 1/9] hw/i2c/core: Add i2c_try_create_slave() and i2c_realize_and_unref() Philippe Mathieu-Daudé
@ 2020-06-22 18:34 ` Philippe Mathieu-Daudé
2020-06-23 5:57 ` Cédric Le Goater
2020-06-22 18:34 ` [PATCH v5 3/9] hw/misc/pca9552: Rename generic code as pca955x Philippe Mathieu-Daudé
` (6 subsequent siblings)
8 siblings, 1 reply; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-22 18:34 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Corey Minyard, Andrew Jeffery,
Philippe Mathieu-Daudé,
qemu-arm, Joel Stanley, Cédric Le Goater
The PCA9552 device does not expose LEDs, but simple pins
to connnect LEDs to. To be clearer with the device model,
rename 'nr_leds' as 'pin_count'.
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
include/hw/misc/pca9552.h | 2 +-
hw/misc/pca9552.c | 10 +++++-----
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/include/hw/misc/pca9552.h b/include/hw/misc/pca9552.h
index ebb43c63fe..bc5ed31087 100644
--- a/include/hw/misc/pca9552.h
+++ b/include/hw/misc/pca9552.h
@@ -26,7 +26,7 @@ typedef struct PCA9552State {
uint8_t regs[PCA9552_NR_REGS];
uint8_t max_reg;
- uint8_t nr_leds;
+ uint8_t pin_count;
} PCA9552State;
#endif
diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
index cac729e35a..81da757a7e 100644
--- a/hw/misc/pca9552.c
+++ b/hw/misc/pca9552.c
@@ -37,7 +37,7 @@ static void pca9552_update_pin_input(PCA9552State *s)
{
int i;
- for (i = 0; i < s->nr_leds; i++) {
+ for (i = 0; i < s->pin_count; i++) {
uint8_t input_reg = PCA9552_INPUT0 + (i / 8);
uint8_t input_shift = (i % 8);
uint8_t config = pca9552_pin_get_config(s, i);
@@ -185,7 +185,7 @@ static void pca9552_get_led(Object *obj, Visitor *v, const char *name,
error_setg(errp, "%s: error reading %s", __func__, name);
return;
}
- if (led < 0 || led > s->nr_leds) {
+ if (led < 0 || led > s->pin_count) {
error_setg(errp, "%s invalid led %s", __func__, name);
return;
}
@@ -228,7 +228,7 @@ static void pca9552_set_led(Object *obj, Visitor *v, const char *name,
error_setg(errp, "%s: error reading %s", __func__, name);
return;
}
- if (led < 0 || led > s->nr_leds) {
+ if (led < 0 || led > s->pin_count) {
error_setg(errp, "%s invalid led %s", __func__, name);
return;
}
@@ -291,9 +291,9 @@ static void pca9552_initfn(Object *obj)
* PCA955X device
*/
s->max_reg = PCA9552_LS3;
- s->nr_leds = 16;
+ s->pin_count = 16;
- for (led = 0; led < s->nr_leds; led++) {
+ for (led = 0; led < s->pin_count; led++) {
char *name;
name = g_strdup_printf("led%d", led);
--
2.21.3
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v5 2/9] hw/misc/pca9552: Rename 'nr_leds' as 'pin_count'
2020-06-22 18:34 ` [PATCH v5 2/9] hw/misc/pca9552: Rename 'nr_leds' as 'pin_count' Philippe Mathieu-Daudé
@ 2020-06-23 5:57 ` Cédric Le Goater
0 siblings, 0 replies; 22+ messages in thread
From: Cédric Le Goater @ 2020-06-23 5:57 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Andrew Jeffery, Peter Maydell, qemu-arm, Joel Stanley, Corey Minyard
On 6/22/20 8:34 PM, Philippe Mathieu-Daudé wrote:
> The PCA9552 device does not expose LEDs, but simple pins
> to connnect LEDs to. To be clearer with the device model,
> rename 'nr_leds' as 'pin_count'.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
> ---
> include/hw/misc/pca9552.h | 2 +-
> hw/misc/pca9552.c | 10 +++++-----
> 2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/include/hw/misc/pca9552.h b/include/hw/misc/pca9552.h
> index ebb43c63fe..bc5ed31087 100644
> --- a/include/hw/misc/pca9552.h
> +++ b/include/hw/misc/pca9552.h
> @@ -26,7 +26,7 @@ typedef struct PCA9552State {
>
> uint8_t regs[PCA9552_NR_REGS];
> uint8_t max_reg;
> - uint8_t nr_leds;
> + uint8_t pin_count;
> } PCA9552State;
>
> #endif
> diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
> index cac729e35a..81da757a7e 100644
> --- a/hw/misc/pca9552.c
> +++ b/hw/misc/pca9552.c
> @@ -37,7 +37,7 @@ static void pca9552_update_pin_input(PCA9552State *s)
> {
> int i;
>
> - for (i = 0; i < s->nr_leds; i++) {
> + for (i = 0; i < s->pin_count; i++) {
> uint8_t input_reg = PCA9552_INPUT0 + (i / 8);
> uint8_t input_shift = (i % 8);
> uint8_t config = pca9552_pin_get_config(s, i);
> @@ -185,7 +185,7 @@ static void pca9552_get_led(Object *obj, Visitor *v, const char *name,
> error_setg(errp, "%s: error reading %s", __func__, name);
> return;
> }
> - if (led < 0 || led > s->nr_leds) {
> + if (led < 0 || led > s->pin_count) {
> error_setg(errp, "%s invalid led %s", __func__, name);
> return;
> }
> @@ -228,7 +228,7 @@ static void pca9552_set_led(Object *obj, Visitor *v, const char *name,
> error_setg(errp, "%s: error reading %s", __func__, name);
> return;
> }
> - if (led < 0 || led > s->nr_leds) {
> + if (led < 0 || led > s->pin_count) {
> error_setg(errp, "%s invalid led %s", __func__, name);
> return;
> }
> @@ -291,9 +291,9 @@ static void pca9552_initfn(Object *obj)
> * PCA955X device
> */
> s->max_reg = PCA9552_LS3;
> - s->nr_leds = 16;
> + s->pin_count = 16;
>
> - for (led = 0; led < s->nr_leds; led++) {
> + for (led = 0; led < s->pin_count; led++) {
> char *name;
>
> name = g_strdup_printf("led%d", led);
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v5 3/9] hw/misc/pca9552: Rename generic code as pca955x
2020-06-22 18:34 [PATCH v5 0/9] hw/misc/pca9552: Trace GPIO change events Philippe Mathieu-Daudé
2020-06-22 18:34 ` [PATCH v5 1/9] hw/i2c/core: Add i2c_try_create_slave() and i2c_realize_and_unref() Philippe Mathieu-Daudé
2020-06-22 18:34 ` [PATCH v5 2/9] hw/misc/pca9552: Rename 'nr_leds' as 'pin_count' Philippe Mathieu-Daudé
@ 2020-06-22 18:34 ` Philippe Mathieu-Daudé
2020-06-23 5:58 ` Cédric Le Goater
2020-06-22 18:34 ` [PATCH v5 4/9] hw/misc/pca9552: Add generic PCA955xClass, parent of TYPE_PCA9552 Philippe Mathieu-Daudé
` (5 subsequent siblings)
8 siblings, 1 reply; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-22 18:34 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Corey Minyard, Andrew Jeffery,
Philippe Mathieu-Daudé,
qemu-arm, Joel Stanley, Cédric Le Goater
Various code from the PCA9552 device model is generic to the
PCA955X family. We'll split the generic code in a base class
in the next commit. To ease review, first do a dumb renaming.
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
include/hw/misc/pca9552.h | 10 ++---
hw/misc/pca9552.c | 80 +++++++++++++++++++--------------------
2 files changed, 45 insertions(+), 45 deletions(-)
diff --git a/include/hw/misc/pca9552.h b/include/hw/misc/pca9552.h
index bc5ed31087..db527595a3 100644
--- a/include/hw/misc/pca9552.h
+++ b/include/hw/misc/pca9552.h
@@ -12,11 +12,11 @@
#include "hw/i2c/i2c.h"
#define TYPE_PCA9552 "pca9552"
-#define PCA9552(obj) OBJECT_CHECK(PCA9552State, (obj), TYPE_PCA9552)
+#define PCA955X(obj) OBJECT_CHECK(PCA955xState, (obj), TYPE_PCA9552)
-#define PCA9552_NR_REGS 10
+#define PCA955X_NR_REGS 10
-typedef struct PCA9552State {
+typedef struct PCA955xState {
/*< private >*/
I2CSlave i2c;
/*< public >*/
@@ -24,9 +24,9 @@ typedef struct PCA9552State {
uint8_t len;
uint8_t pointer;
- uint8_t regs[PCA9552_NR_REGS];
+ uint8_t regs[PCA955X_NR_REGS];
uint8_t max_reg;
uint8_t pin_count;
-} PCA9552State;
+} PCA955xState;
#endif
diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
index 81da757a7e..5681ff3b22 100644
--- a/hw/misc/pca9552.c
+++ b/hw/misc/pca9552.c
@@ -25,7 +25,7 @@
static const char *led_state[] = {"on", "off", "pwm0", "pwm1"};
-static uint8_t pca9552_pin_get_config(PCA9552State *s, int pin)
+static uint8_t pca955x_pin_get_config(PCA955xState *s, int pin)
{
uint8_t reg = PCA9552_LS0 + (pin / 4);
uint8_t shift = (pin % 4) << 1;
@@ -33,14 +33,14 @@ static uint8_t pca9552_pin_get_config(PCA9552State *s, int pin)
return extract32(s->regs[reg], shift, 2);
}
-static void pca9552_update_pin_input(PCA9552State *s)
+static void pca955x_update_pin_input(PCA955xState *s)
{
int i;
for (i = 0; i < s->pin_count; i++) {
uint8_t input_reg = PCA9552_INPUT0 + (i / 8);
uint8_t input_shift = (i % 8);
- uint8_t config = pca9552_pin_get_config(s, i);
+ uint8_t config = pca955x_pin_get_config(s, i);
switch (config) {
case PCA9552_LED_ON:
@@ -58,7 +58,7 @@ static void pca9552_update_pin_input(PCA9552State *s)
}
}
-static uint8_t pca9552_read(PCA9552State *s, uint8_t reg)
+static uint8_t pca955x_read(PCA955xState *s, uint8_t reg)
{
switch (reg) {
case PCA9552_INPUT0:
@@ -79,7 +79,7 @@ static uint8_t pca9552_read(PCA9552State *s, uint8_t reg)
}
}
-static void pca9552_write(PCA9552State *s, uint8_t reg, uint8_t data)
+static void pca955x_write(PCA955xState *s, uint8_t reg, uint8_t data)
{
switch (reg) {
case PCA9552_PSC0:
@@ -94,7 +94,7 @@ static void pca9552_write(PCA9552State *s, uint8_t reg, uint8_t data)
case PCA9552_LS2:
case PCA9552_LS3:
s->regs[reg] = data;
- pca9552_update_pin_input(s);
+ pca955x_update_pin_input(s);
break;
case PCA9552_INPUT0:
@@ -110,7 +110,7 @@ static void pca9552_write(PCA9552State *s, uint8_t reg, uint8_t data)
* after each byte is sent to or received by the device. The index
* rollovers to 0 when the maximum register address is reached.
*/
-static void pca9552_autoinc(PCA9552State *s)
+static void pca955x_autoinc(PCA955xState *s)
{
if (s->pointer != 0xFF && s->pointer & PCA9552_AUTOINC) {
uint8_t reg = s->pointer & 0xf;
@@ -120,12 +120,12 @@ static void pca9552_autoinc(PCA9552State *s)
}
}
-static uint8_t pca9552_recv(I2CSlave *i2c)
+static uint8_t pca955x_recv(I2CSlave *i2c)
{
- PCA9552State *s = PCA9552(i2c);
+ PCA955xState *s = PCA955X(i2c);
uint8_t ret;
- ret = pca9552_read(s, s->pointer & 0xf);
+ ret = pca955x_read(s, s->pointer & 0xf);
/*
* From the Specs:
@@ -143,40 +143,40 @@ static uint8_t pca9552_recv(I2CSlave *i2c)
__func__);
}
- pca9552_autoinc(s);
+ pca955x_autoinc(s);
return ret;
}
-static int pca9552_send(I2CSlave *i2c, uint8_t data)
+static int pca955x_send(I2CSlave *i2c, uint8_t data)
{
- PCA9552State *s = PCA9552(i2c);
+ PCA955xState *s = PCA955X(i2c);
/* First byte sent by is the register address */
if (s->len == 0) {
s->pointer = data;
s->len++;
} else {
- pca9552_write(s, s->pointer & 0xf, data);
+ pca955x_write(s, s->pointer & 0xf, data);
- pca9552_autoinc(s);
+ pca955x_autoinc(s);
}
return 0;
}
-static int pca9552_event(I2CSlave *i2c, enum i2c_event event)
+static int pca955x_event(I2CSlave *i2c, enum i2c_event event)
{
- PCA9552State *s = PCA9552(i2c);
+ PCA955xState *s = PCA955X(i2c);
s->len = 0;
return 0;
}
-static void pca9552_get_led(Object *obj, Visitor *v, const char *name,
+static void pca955x_get_led(Object *obj, Visitor *v, const char *name,
void *opaque, Error **errp)
{
- PCA9552State *s = PCA9552(obj);
+ PCA955xState *s = PCA955X(obj);
int led, rc, reg;
uint8_t state;
@@ -195,7 +195,7 @@ static void pca9552_get_led(Object *obj, Visitor *v, const char *name,
* reading the INPUTx reg
*/
reg = PCA9552_LS0 + led / 4;
- state = (pca9552_read(s, reg) >> (led % 8)) & 0x3;
+ state = (pca955x_read(s, reg) >> (led % 8)) & 0x3;
visit_type_str(v, name, (char **)&led_state[state], errp);
}
@@ -209,10 +209,10 @@ static inline uint8_t pca955x_ledsel(uint8_t oldval, int led_num, int state)
((state & 0x3) << (led_num << 1));
}
-static void pca9552_set_led(Object *obj, Visitor *v, const char *name,
+static void pca955x_set_led(Object *obj, Visitor *v, const char *name,
void *opaque, Error **errp)
{
- PCA9552State *s = PCA9552(obj);
+ PCA955xState *s = PCA955X(obj);
Error *local_err = NULL;
int led, rc, reg, val;
uint8_t state;
@@ -244,9 +244,9 @@ static void pca9552_set_led(Object *obj, Visitor *v, const char *name,
}
reg = PCA9552_LS0 + led / 4;
- val = pca9552_read(s, reg);
+ val = pca955x_read(s, reg);
val = pca955x_ledsel(val, led % 4, state);
- pca9552_write(s, reg, val);
+ pca955x_write(s, reg, val);
}
static const VMStateDescription pca9552_vmstate = {
@@ -254,17 +254,17 @@ static const VMStateDescription pca9552_vmstate = {
.version_id = 0,
.minimum_version_id = 0,
.fields = (VMStateField[]) {
- VMSTATE_UINT8(len, PCA9552State),
- VMSTATE_UINT8(pointer, PCA9552State),
- VMSTATE_UINT8_ARRAY(regs, PCA9552State, PCA9552_NR_REGS),
- VMSTATE_I2C_SLAVE(i2c, PCA9552State),
+ VMSTATE_UINT8(len, PCA955xState),
+ VMSTATE_UINT8(pointer, PCA955xState),
+ VMSTATE_UINT8_ARRAY(regs, PCA955xState, PCA955X_NR_REGS),
+ VMSTATE_I2C_SLAVE(i2c, PCA955xState),
VMSTATE_END_OF_LIST()
}
};
static void pca9552_reset(DeviceState *dev)
{
- PCA9552State *s = PCA9552(dev);
+ PCA955xState *s = PCA955X(dev);
s->regs[PCA9552_PSC0] = 0xFF;
s->regs[PCA9552_PWM0] = 0x80;
@@ -275,15 +275,15 @@ static void pca9552_reset(DeviceState *dev)
s->regs[PCA9552_LS2] = 0x55;
s->regs[PCA9552_LS3] = 0x55;
- pca9552_update_pin_input(s);
+ pca955x_update_pin_input(s);
s->pointer = 0xFF;
s->len = 0;
}
-static void pca9552_initfn(Object *obj)
+static void pca955x_initfn(Object *obj)
{
- PCA9552State *s = PCA9552(obj);
+ PCA955xState *s = PCA955X(obj);
int led;
/* If support for the other PCA955X devices are implemented, these
@@ -297,7 +297,7 @@ static void pca9552_initfn(Object *obj)
char *name;
name = g_strdup_printf("led%d", led);
- object_property_add(obj, name, "bool", pca9552_get_led, pca9552_set_led,
+ object_property_add(obj, name, "bool", pca955x_get_led, pca955x_set_led,
NULL, NULL);
g_free(name);
}
@@ -308,9 +308,9 @@ static void pca9552_class_init(ObjectClass *klass, void *data)
DeviceClass *dc = DEVICE_CLASS(klass);
I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
- k->event = pca9552_event;
- k->recv = pca9552_recv;
- k->send = pca9552_send;
+ k->event = pca955x_event;
+ k->recv = pca955x_recv;
+ k->send = pca955x_send;
dc->reset = pca9552_reset;
dc->vmsd = &pca9552_vmstate;
}
@@ -318,14 +318,14 @@ static void pca9552_class_init(ObjectClass *klass, void *data)
static const TypeInfo pca9552_info = {
.name = TYPE_PCA9552,
.parent = TYPE_I2C_SLAVE,
- .instance_init = pca9552_initfn,
- .instance_size = sizeof(PCA9552State),
+ .instance_init = pca955x_initfn,
+ .instance_size = sizeof(PCA955xState),
.class_init = pca9552_class_init,
};
-static void pca9552_register_types(void)
+static void pca955x_register_types(void)
{
type_register_static(&pca9552_info);
}
-type_init(pca9552_register_types)
+type_init(pca955x_register_types)
--
2.21.3
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v5 3/9] hw/misc/pca9552: Rename generic code as pca955x
2020-06-22 18:34 ` [PATCH v5 3/9] hw/misc/pca9552: Rename generic code as pca955x Philippe Mathieu-Daudé
@ 2020-06-23 5:58 ` Cédric Le Goater
0 siblings, 0 replies; 22+ messages in thread
From: Cédric Le Goater @ 2020-06-23 5:58 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Andrew Jeffery, Peter Maydell, qemu-arm, Joel Stanley, Corey Minyard
On 6/22/20 8:34 PM, Philippe Mathieu-Daudé wrote:
> Various code from the PCA9552 device model is generic to the
> PCA955X family. We'll split the generic code in a base class
> in the next commit. To ease review, first do a dumb renaming.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
> ---
> include/hw/misc/pca9552.h | 10 ++---
> hw/misc/pca9552.c | 80 +++++++++++++++++++--------------------
> 2 files changed, 45 insertions(+), 45 deletions(-)
>
> diff --git a/include/hw/misc/pca9552.h b/include/hw/misc/pca9552.h
> index bc5ed31087..db527595a3 100644
> --- a/include/hw/misc/pca9552.h
> +++ b/include/hw/misc/pca9552.h
> @@ -12,11 +12,11 @@
> #include "hw/i2c/i2c.h"
>
> #define TYPE_PCA9552 "pca9552"
> -#define PCA9552(obj) OBJECT_CHECK(PCA9552State, (obj), TYPE_PCA9552)
> +#define PCA955X(obj) OBJECT_CHECK(PCA955xState, (obj), TYPE_PCA9552)
>
> -#define PCA9552_NR_REGS 10
> +#define PCA955X_NR_REGS 10
>
> -typedef struct PCA9552State {
> +typedef struct PCA955xState {
> /*< private >*/
> I2CSlave i2c;
> /*< public >*/
> @@ -24,9 +24,9 @@ typedef struct PCA9552State {
> uint8_t len;
> uint8_t pointer;
>
> - uint8_t regs[PCA9552_NR_REGS];
> + uint8_t regs[PCA955X_NR_REGS];
> uint8_t max_reg;
> uint8_t pin_count;
> -} PCA9552State;
> +} PCA955xState;
>
> #endif
> diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
> index 81da757a7e..5681ff3b22 100644
> --- a/hw/misc/pca9552.c
> +++ b/hw/misc/pca9552.c
> @@ -25,7 +25,7 @@
>
> static const char *led_state[] = {"on", "off", "pwm0", "pwm1"};
>
> -static uint8_t pca9552_pin_get_config(PCA9552State *s, int pin)
> +static uint8_t pca955x_pin_get_config(PCA955xState *s, int pin)
> {
> uint8_t reg = PCA9552_LS0 + (pin / 4);
> uint8_t shift = (pin % 4) << 1;
> @@ -33,14 +33,14 @@ static uint8_t pca9552_pin_get_config(PCA9552State *s, int pin)
> return extract32(s->regs[reg], shift, 2);
> }
>
> -static void pca9552_update_pin_input(PCA9552State *s)
> +static void pca955x_update_pin_input(PCA955xState *s)
> {
> int i;
>
> for (i = 0; i < s->pin_count; i++) {
> uint8_t input_reg = PCA9552_INPUT0 + (i / 8);
> uint8_t input_shift = (i % 8);
> - uint8_t config = pca9552_pin_get_config(s, i);
> + uint8_t config = pca955x_pin_get_config(s, i);
>
> switch (config) {
> case PCA9552_LED_ON:
> @@ -58,7 +58,7 @@ static void pca9552_update_pin_input(PCA9552State *s)
> }
> }
>
> -static uint8_t pca9552_read(PCA9552State *s, uint8_t reg)
> +static uint8_t pca955x_read(PCA955xState *s, uint8_t reg)
> {
> switch (reg) {
> case PCA9552_INPUT0:
> @@ -79,7 +79,7 @@ static uint8_t pca9552_read(PCA9552State *s, uint8_t reg)
> }
> }
>
> -static void pca9552_write(PCA9552State *s, uint8_t reg, uint8_t data)
> +static void pca955x_write(PCA955xState *s, uint8_t reg, uint8_t data)
> {
> switch (reg) {
> case PCA9552_PSC0:
> @@ -94,7 +94,7 @@ static void pca9552_write(PCA9552State *s, uint8_t reg, uint8_t data)
> case PCA9552_LS2:
> case PCA9552_LS3:
> s->regs[reg] = data;
> - pca9552_update_pin_input(s);
> + pca955x_update_pin_input(s);
> break;
>
> case PCA9552_INPUT0:
> @@ -110,7 +110,7 @@ static void pca9552_write(PCA9552State *s, uint8_t reg, uint8_t data)
> * after each byte is sent to or received by the device. The index
> * rollovers to 0 when the maximum register address is reached.
> */
> -static void pca9552_autoinc(PCA9552State *s)
> +static void pca955x_autoinc(PCA955xState *s)
> {
> if (s->pointer != 0xFF && s->pointer & PCA9552_AUTOINC) {
> uint8_t reg = s->pointer & 0xf;
> @@ -120,12 +120,12 @@ static void pca9552_autoinc(PCA9552State *s)
> }
> }
>
> -static uint8_t pca9552_recv(I2CSlave *i2c)
> +static uint8_t pca955x_recv(I2CSlave *i2c)
> {
> - PCA9552State *s = PCA9552(i2c);
> + PCA955xState *s = PCA955X(i2c);
> uint8_t ret;
>
> - ret = pca9552_read(s, s->pointer & 0xf);
> + ret = pca955x_read(s, s->pointer & 0xf);
>
> /*
> * From the Specs:
> @@ -143,40 +143,40 @@ static uint8_t pca9552_recv(I2CSlave *i2c)
> __func__);
> }
>
> - pca9552_autoinc(s);
> + pca955x_autoinc(s);
>
> return ret;
> }
>
> -static int pca9552_send(I2CSlave *i2c, uint8_t data)
> +static int pca955x_send(I2CSlave *i2c, uint8_t data)
> {
> - PCA9552State *s = PCA9552(i2c);
> + PCA955xState *s = PCA955X(i2c);
>
> /* First byte sent by is the register address */
> if (s->len == 0) {
> s->pointer = data;
> s->len++;
> } else {
> - pca9552_write(s, s->pointer & 0xf, data);
> + pca955x_write(s, s->pointer & 0xf, data);
>
> - pca9552_autoinc(s);
> + pca955x_autoinc(s);
> }
>
> return 0;
> }
>
> -static int pca9552_event(I2CSlave *i2c, enum i2c_event event)
> +static int pca955x_event(I2CSlave *i2c, enum i2c_event event)
> {
> - PCA9552State *s = PCA9552(i2c);
> + PCA955xState *s = PCA955X(i2c);
>
> s->len = 0;
> return 0;
> }
>
> -static void pca9552_get_led(Object *obj, Visitor *v, const char *name,
> +static void pca955x_get_led(Object *obj, Visitor *v, const char *name,
> void *opaque, Error **errp)
> {
> - PCA9552State *s = PCA9552(obj);
> + PCA955xState *s = PCA955X(obj);
> int led, rc, reg;
> uint8_t state;
>
> @@ -195,7 +195,7 @@ static void pca9552_get_led(Object *obj, Visitor *v, const char *name,
> * reading the INPUTx reg
> */
> reg = PCA9552_LS0 + led / 4;
> - state = (pca9552_read(s, reg) >> (led % 8)) & 0x3;
> + state = (pca955x_read(s, reg) >> (led % 8)) & 0x3;
> visit_type_str(v, name, (char **)&led_state[state], errp);
> }
>
> @@ -209,10 +209,10 @@ static inline uint8_t pca955x_ledsel(uint8_t oldval, int led_num, int state)
> ((state & 0x3) << (led_num << 1));
> }
>
> -static void pca9552_set_led(Object *obj, Visitor *v, const char *name,
> +static void pca955x_set_led(Object *obj, Visitor *v, const char *name,
> void *opaque, Error **errp)
> {
> - PCA9552State *s = PCA9552(obj);
> + PCA955xState *s = PCA955X(obj);
> Error *local_err = NULL;
> int led, rc, reg, val;
> uint8_t state;
> @@ -244,9 +244,9 @@ static void pca9552_set_led(Object *obj, Visitor *v, const char *name,
> }
>
> reg = PCA9552_LS0 + led / 4;
> - val = pca9552_read(s, reg);
> + val = pca955x_read(s, reg);
> val = pca955x_ledsel(val, led % 4, state);
> - pca9552_write(s, reg, val);
> + pca955x_write(s, reg, val);
> }
>
> static const VMStateDescription pca9552_vmstate = {
> @@ -254,17 +254,17 @@ static const VMStateDescription pca9552_vmstate = {
> .version_id = 0,
> .minimum_version_id = 0,
> .fields = (VMStateField[]) {
> - VMSTATE_UINT8(len, PCA9552State),
> - VMSTATE_UINT8(pointer, PCA9552State),
> - VMSTATE_UINT8_ARRAY(regs, PCA9552State, PCA9552_NR_REGS),
> - VMSTATE_I2C_SLAVE(i2c, PCA9552State),
> + VMSTATE_UINT8(len, PCA955xState),
> + VMSTATE_UINT8(pointer, PCA955xState),
> + VMSTATE_UINT8_ARRAY(regs, PCA955xState, PCA955X_NR_REGS),
> + VMSTATE_I2C_SLAVE(i2c, PCA955xState),
> VMSTATE_END_OF_LIST()
> }
> };
>
> static void pca9552_reset(DeviceState *dev)
> {
> - PCA9552State *s = PCA9552(dev);
> + PCA955xState *s = PCA955X(dev);
>
> s->regs[PCA9552_PSC0] = 0xFF;
> s->regs[PCA9552_PWM0] = 0x80;
> @@ -275,15 +275,15 @@ static void pca9552_reset(DeviceState *dev)
> s->regs[PCA9552_LS2] = 0x55;
> s->regs[PCA9552_LS3] = 0x55;
>
> - pca9552_update_pin_input(s);
> + pca955x_update_pin_input(s);
>
> s->pointer = 0xFF;
> s->len = 0;
> }
>
> -static void pca9552_initfn(Object *obj)
> +static void pca955x_initfn(Object *obj)
> {
> - PCA9552State *s = PCA9552(obj);
> + PCA955xState *s = PCA955X(obj);
> int led;
>
> /* If support for the other PCA955X devices are implemented, these
> @@ -297,7 +297,7 @@ static void pca9552_initfn(Object *obj)
> char *name;
>
> name = g_strdup_printf("led%d", led);
> - object_property_add(obj, name, "bool", pca9552_get_led, pca9552_set_led,
> + object_property_add(obj, name, "bool", pca955x_get_led, pca955x_set_led,
> NULL, NULL);
> g_free(name);
> }
> @@ -308,9 +308,9 @@ static void pca9552_class_init(ObjectClass *klass, void *data)
> DeviceClass *dc = DEVICE_CLASS(klass);
> I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
>
> - k->event = pca9552_event;
> - k->recv = pca9552_recv;
> - k->send = pca9552_send;
> + k->event = pca955x_event;
> + k->recv = pca955x_recv;
> + k->send = pca955x_send;
> dc->reset = pca9552_reset;
> dc->vmsd = &pca9552_vmstate;
> }
> @@ -318,14 +318,14 @@ static void pca9552_class_init(ObjectClass *klass, void *data)
> static const TypeInfo pca9552_info = {
> .name = TYPE_PCA9552,
> .parent = TYPE_I2C_SLAVE,
> - .instance_init = pca9552_initfn,
> - .instance_size = sizeof(PCA9552State),
> + .instance_init = pca955x_initfn,
> + .instance_size = sizeof(PCA955xState),
> .class_init = pca9552_class_init,
> };
>
> -static void pca9552_register_types(void)
> +static void pca955x_register_types(void)
> {
> type_register_static(&pca9552_info);
> }
>
> -type_init(pca9552_register_types)
> +type_init(pca955x_register_types)
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v5 4/9] hw/misc/pca9552: Add generic PCA955xClass, parent of TYPE_PCA9552
2020-06-22 18:34 [PATCH v5 0/9] hw/misc/pca9552: Trace GPIO change events Philippe Mathieu-Daudé
` (2 preceding siblings ...)
2020-06-22 18:34 ` [PATCH v5 3/9] hw/misc/pca9552: Rename generic code as pca955x Philippe Mathieu-Daudé
@ 2020-06-22 18:34 ` Philippe Mathieu-Daudé
2020-06-23 6:02 ` Cédric Le Goater
2020-06-22 18:34 ` [PATCH v5 5/9] hw/misc/pca9552: Add a 'description' property for debugging purpose Philippe Mathieu-Daudé
` (4 subsequent siblings)
8 siblings, 1 reply; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-22 18:34 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Corey Minyard, Andrew Jeffery,
Philippe Mathieu-Daudé,
qemu-arm, Joel Stanley, Cédric Le Goater
Extract the code common to the PCA955x family in PCA955xClass,
keeping the PCA9552 specific parts into pca9552_class_init().
Remove the 'TODO' comment added in commit 5141d4158cf.
Suggested-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
include/hw/misc/pca9552.h | 6 ++--
hw/misc/pca9552.c | 72 ++++++++++++++++++++++++++++++---------
2 files changed, 58 insertions(+), 20 deletions(-)
diff --git a/include/hw/misc/pca9552.h b/include/hw/misc/pca9552.h
index db527595a3..90843b03b8 100644
--- a/include/hw/misc/pca9552.h
+++ b/include/hw/misc/pca9552.h
@@ -12,9 +12,11 @@
#include "hw/i2c/i2c.h"
#define TYPE_PCA9552 "pca9552"
-#define PCA955X(obj) OBJECT_CHECK(PCA955xState, (obj), TYPE_PCA9552)
+#define TYPE_PCA955X "pca955x"
+#define PCA955X(obj) OBJECT_CHECK(PCA955xState, (obj), TYPE_PCA955X)
#define PCA955X_NR_REGS 10
+#define PCA955X_PIN_COUNT_MAX 16
typedef struct PCA955xState {
/*< private >*/
@@ -25,8 +27,6 @@ typedef struct PCA955xState {
uint8_t pointer;
uint8_t regs[PCA955X_NR_REGS];
- uint8_t max_reg;
- uint8_t pin_count;
} PCA955xState;
#endif
diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
index 5681ff3b22..34062dbb69 100644
--- a/hw/misc/pca9552.c
+++ b/hw/misc/pca9552.c
@@ -4,6 +4,7 @@
* https://www.nxp.com/docs/en/application-note/AN264.pdf
*
* Copyright (c) 2017-2018, IBM Corporation.
+ * Copyright (c) 2020 Philippe Mathieu-Daudé
*
* This work is licensed under the terms of the GNU GPL, version 2 or
* later. See the COPYING file in the top-level directory.
@@ -18,6 +19,20 @@
#include "qapi/error.h"
#include "qapi/visitor.h"
+typedef struct PCA955xClass {
+ /*< private >*/
+ I2CSlaveClass parent_class;
+ /*< public >*/
+
+ uint8_t pin_count;
+ uint8_t max_reg;
+} PCA955xClass;
+
+#define PCA955X_CLASS(klass) \
+ OBJECT_CLASS_CHECK(PCA955xClass, (klass), TYPE_PCA955X)
+#define PCA955X_GET_CLASS(obj) \
+ OBJECT_GET_CLASS(PCA955xClass, (obj), TYPE_PCA955X)
+
#define PCA9552_LED_ON 0x0
#define PCA9552_LED_OFF 0x1
#define PCA9552_LED_PWM0 0x2
@@ -35,9 +50,10 @@ static uint8_t pca955x_pin_get_config(PCA955xState *s, int pin)
static void pca955x_update_pin_input(PCA955xState *s)
{
+ PCA955xClass *k = PCA955X_GET_CLASS(s);
int i;
- for (i = 0; i < s->pin_count; i++) {
+ for (i = 0; i < k->pin_count; i++) {
uint8_t input_reg = PCA9552_INPUT0 + (i / 8);
uint8_t input_shift = (i % 8);
uint8_t config = pca955x_pin_get_config(s, i);
@@ -112,10 +128,12 @@ static void pca955x_write(PCA955xState *s, uint8_t reg, uint8_t data)
*/
static void pca955x_autoinc(PCA955xState *s)
{
+ PCA955xClass *k = PCA955X_GET_CLASS(s);
+
if (s->pointer != 0xFF && s->pointer & PCA9552_AUTOINC) {
uint8_t reg = s->pointer & 0xf;
- reg = (reg + 1) % (s->max_reg + 1);
+ reg = (reg + 1) % (k->max_reg + 1);
s->pointer = reg | PCA9552_AUTOINC;
}
}
@@ -176,6 +194,7 @@ static int pca955x_event(I2CSlave *i2c, enum i2c_event event)
static void pca955x_get_led(Object *obj, Visitor *v, const char *name,
void *opaque, Error **errp)
{
+ PCA955xClass *k = PCA955X_GET_CLASS(obj);
PCA955xState *s = PCA955X(obj);
int led, rc, reg;
uint8_t state;
@@ -185,7 +204,7 @@ static void pca955x_get_led(Object *obj, Visitor *v, const char *name,
error_setg(errp, "%s: error reading %s", __func__, name);
return;
}
- if (led < 0 || led > s->pin_count) {
+ if (led < 0 || led > k->pin_count) {
error_setg(errp, "%s invalid led %s", __func__, name);
return;
}
@@ -212,6 +231,7 @@ static inline uint8_t pca955x_ledsel(uint8_t oldval, int led_num, int state)
static void pca955x_set_led(Object *obj, Visitor *v, const char *name,
void *opaque, Error **errp)
{
+ PCA955xClass *k = PCA955X_GET_CLASS(obj);
PCA955xState *s = PCA955X(obj);
Error *local_err = NULL;
int led, rc, reg, val;
@@ -228,7 +248,7 @@ static void pca955x_set_led(Object *obj, Visitor *v, const char *name,
error_setg(errp, "%s: error reading %s", __func__, name);
return;
}
- if (led < 0 || led > s->pin_count) {
+ if (led < 0 || led > k->pin_count) {
error_setg(errp, "%s invalid led %s", __func__, name);
return;
}
@@ -283,17 +303,10 @@ static void pca9552_reset(DeviceState *dev)
static void pca955x_initfn(Object *obj)
{
- PCA955xState *s = PCA955X(obj);
+ PCA955xClass *k = PCA955X_GET_CLASS(obj);
int led;
- /* If support for the other PCA955X devices are implemented, these
- * constant values might be part of class structure describing the
- * PCA955X device
- */
- s->max_reg = PCA9552_LS3;
- s->pin_count = 16;
-
- for (led = 0; led < s->pin_count; led++) {
+ for (led = 0; led < k->pin_count; led++) {
char *name;
name = g_strdup_printf("led%d", led);
@@ -303,7 +316,14 @@ static void pca955x_initfn(Object *obj)
}
}
-static void pca9552_class_init(ObjectClass *klass, void *data)
+static void pca955x_realize(DeviceState *dev, Error **errp)
+{
+ PCA955xClass *k = PCA955X_GET_CLASS(dev);
+
+ assert(k->pin_count <= PCA955X_PIN_COUNT_MAX);
+}
+
+static void pca955x_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
@@ -311,20 +331,38 @@ static void pca9552_class_init(ObjectClass *klass, void *data)
k->event = pca955x_event;
k->recv = pca955x_recv;
k->send = pca955x_send;
+ dc->realize = pca955x_realize;
+}
+
+static const TypeInfo pca955x_info = {
+ .name = TYPE_PCA955X,
+ .parent = TYPE_I2C_SLAVE,
+ .instance_init = pca955x_initfn,
+ .instance_size = sizeof(PCA955xState),
+ .class_init = pca955x_class_init,
+ .abstract = true,
+};
+
+static void pca9552_class_init(ObjectClass *oc, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(oc);
+ PCA955xClass *pc = PCA955X_CLASS(oc);
+
dc->reset = pca9552_reset;
dc->vmsd = &pca9552_vmstate;
+ pc->max_reg = PCA9552_LS3;
+ pc->pin_count = 16;
}
static const TypeInfo pca9552_info = {
.name = TYPE_PCA9552,
- .parent = TYPE_I2C_SLAVE,
- .instance_init = pca955x_initfn,
- .instance_size = sizeof(PCA955xState),
+ .parent = TYPE_PCA955X,
.class_init = pca9552_class_init,
};
static void pca955x_register_types(void)
{
+ type_register_static(&pca955x_info);
type_register_static(&pca9552_info);
}
--
2.21.3
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v5 4/9] hw/misc/pca9552: Add generic PCA955xClass, parent of TYPE_PCA9552
2020-06-22 18:34 ` [PATCH v5 4/9] hw/misc/pca9552: Add generic PCA955xClass, parent of TYPE_PCA9552 Philippe Mathieu-Daudé
@ 2020-06-23 6:02 ` Cédric Le Goater
0 siblings, 0 replies; 22+ messages in thread
From: Cédric Le Goater @ 2020-06-23 6:02 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Andrew Jeffery, Peter Maydell, qemu-arm, Joel Stanley, Corey Minyard
On 6/22/20 8:34 PM, Philippe Mathieu-Daudé wrote:
> Extract the code common to the PCA955x family in PCA955xClass,
> keeping the PCA9552 specific parts into pca9552_class_init().
> Remove the 'TODO' comment added in commit 5141d4158cf.
>
> Suggested-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
One comment below,
> ---
> include/hw/misc/pca9552.h | 6 ++--
> hw/misc/pca9552.c | 72 ++++++++++++++++++++++++++++++---------
> 2 files changed, 58 insertions(+), 20 deletions(-)
>
> diff --git a/include/hw/misc/pca9552.h b/include/hw/misc/pca9552.h
> index db527595a3..90843b03b8 100644
> --- a/include/hw/misc/pca9552.h
> +++ b/include/hw/misc/pca9552.h
> @@ -12,9 +12,11 @@
> #include "hw/i2c/i2c.h"
>
> #define TYPE_PCA9552 "pca9552"
> -#define PCA955X(obj) OBJECT_CHECK(PCA955xState, (obj), TYPE_PCA9552)
> +#define TYPE_PCA955X "pca955x"
> +#define PCA955X(obj) OBJECT_CHECK(PCA955xState, (obj), TYPE_PCA955X)
>
> #define PCA955X_NR_REGS 10
> +#define PCA955X_PIN_COUNT_MAX 16
>
> typedef struct PCA955xState {
> /*< private >*/
> @@ -25,8 +27,6 @@ typedef struct PCA955xState {
> uint8_t pointer;
>
> uint8_t regs[PCA955X_NR_REGS];
> - uint8_t max_reg;
> - uint8_t pin_count;
> } PCA955xState;
>
> #endif
> diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
> index 5681ff3b22..34062dbb69 100644
> --- a/hw/misc/pca9552.c
> +++ b/hw/misc/pca9552.c
> @@ -4,6 +4,7 @@
> * https://www.nxp.com/docs/en/application-note/AN264.pdf
> *
> * Copyright (c) 2017-2018, IBM Corporation.
> + * Copyright (c) 2020 Philippe Mathieu-Daudé
> *
> * This work is licensed under the terms of the GNU GPL, version 2 or
> * later. See the COPYING file in the top-level directory.
> @@ -18,6 +19,20 @@
> #include "qapi/error.h"
> #include "qapi/visitor.h"
>
> +typedef struct PCA955xClass {
> + /*< private >*/
> + I2CSlaveClass parent_class;
> + /*< public >*/
> +
> + uint8_t pin_count;
> + uint8_t max_reg;
> +} PCA955xClass;
> +
> +#define PCA955X_CLASS(klass) \
> + OBJECT_CLASS_CHECK(PCA955xClass, (klass), TYPE_PCA955X)
> +#define PCA955X_GET_CLASS(obj) \
> + OBJECT_GET_CLASS(PCA955xClass, (obj), TYPE_PCA955X)
> +
> #define PCA9552_LED_ON 0x0
> #define PCA9552_LED_OFF 0x1
> #define PCA9552_LED_PWM0 0x2
> @@ -35,9 +50,10 @@ static uint8_t pca955x_pin_get_config(PCA955xState *s, int pin)
>
> static void pca955x_update_pin_input(PCA955xState *s)
> {
> + PCA955xClass *k = PCA955X_GET_CLASS(s);
> int i;
>
> - for (i = 0; i < s->pin_count; i++) {
> + for (i = 0; i < k->pin_count; i++) {
> uint8_t input_reg = PCA9552_INPUT0 + (i / 8);
> uint8_t input_shift = (i % 8);
> uint8_t config = pca955x_pin_get_config(s, i);
> @@ -112,10 +128,12 @@ static void pca955x_write(PCA955xState *s, uint8_t reg, uint8_t data)
> */
> static void pca955x_autoinc(PCA955xState *s)
> {
> + PCA955xClass *k = PCA955X_GET_CLASS(s);
> +
> if (s->pointer != 0xFF && s->pointer & PCA9552_AUTOINC) {
> uint8_t reg = s->pointer & 0xf;
>
> - reg = (reg + 1) % (s->max_reg + 1);
> + reg = (reg + 1) % (k->max_reg + 1);
> s->pointer = reg | PCA9552_AUTOINC;
> }
> }
> @@ -176,6 +194,7 @@ static int pca955x_event(I2CSlave *i2c, enum i2c_event event)
> static void pca955x_get_led(Object *obj, Visitor *v, const char *name,
> void *opaque, Error **errp)
> {
> + PCA955xClass *k = PCA955X_GET_CLASS(obj);
> PCA955xState *s = PCA955X(obj);
> int led, rc, reg;
> uint8_t state;
> @@ -185,7 +204,7 @@ static void pca955x_get_led(Object *obj, Visitor *v, const char *name,
> error_setg(errp, "%s: error reading %s", __func__, name);
> return;
> }
> - if (led < 0 || led > s->pin_count) {
> + if (led < 0 || led > k->pin_count) {
> error_setg(errp, "%s invalid led %s", __func__, name);
> return;
> }
> @@ -212,6 +231,7 @@ static inline uint8_t pca955x_ledsel(uint8_t oldval, int led_num, int state)
> static void pca955x_set_led(Object *obj, Visitor *v, const char *name,
> void *opaque, Error **errp)
> {
> + PCA955xClass *k = PCA955X_GET_CLASS(obj);
> PCA955xState *s = PCA955X(obj);
> Error *local_err = NULL;
> int led, rc, reg, val;
> @@ -228,7 +248,7 @@ static void pca955x_set_led(Object *obj, Visitor *v, const char *name,
> error_setg(errp, "%s: error reading %s", __func__, name);
> return;
> }
> - if (led < 0 || led > s->pin_count) {
> + if (led < 0 || led > k->pin_count) {
> error_setg(errp, "%s invalid led %s", __func__, name);
> return;
> }
> @@ -283,17 +303,10 @@ static void pca9552_reset(DeviceState *dev)
>
> static void pca955x_initfn(Object *obj)
> {
> - PCA955xState *s = PCA955X(obj);
> + PCA955xClass *k = PCA955X_GET_CLASS(obj);
> int led;
>
> - /* If support for the other PCA955X devices are implemented, these
> - * constant values might be part of class structure describing the
> - * PCA955X device
> - */
> - s->max_reg = PCA9552_LS3;
> - s->pin_count = 16;
> -
> - for (led = 0; led < s->pin_count; led++) {
> + for (led = 0; led < k->pin_count; led++) {
> char *name;
>
> name = g_strdup_printf("led%d", led);
> @@ -303,7 +316,14 @@ static void pca955x_initfn(Object *obj)
> }
> }
>
> -static void pca9552_class_init(ObjectClass *klass, void *data)
> +static void pca955x_realize(DeviceState *dev, Error **errp)
> +{
> + PCA955xClass *k = PCA955X_GET_CLASS(dev);
> +
> + assert(k->pin_count <= PCA955X_PIN_COUNT_MAX);
This test could be done in the instance_init handler.
Thanks,
C.
> +}
> +
> +static void pca955x_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
> I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
> @@ -311,20 +331,38 @@ static void pca9552_class_init(ObjectClass *klass, void *data)
> k->event = pca955x_event;
> k->recv = pca955x_recv;
> k->send = pca955x_send;
> + dc->realize = pca955x_realize;
> +}
> +
> +static const TypeInfo pca955x_info = {
> + .name = TYPE_PCA955X,
> + .parent = TYPE_I2C_SLAVE,
> + .instance_init = pca955x_initfn,
> + .instance_size = sizeof(PCA955xState),
> + .class_init = pca955x_class_init,
> + .abstract = true,
> +};
> +
> +static void pca9552_class_init(ObjectClass *oc, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(oc);
> + PCA955xClass *pc = PCA955X_CLASS(oc);
> +
> dc->reset = pca9552_reset;
> dc->vmsd = &pca9552_vmstate;
> + pc->max_reg = PCA9552_LS3;
> + pc->pin_count = 16;
> }
>
> static const TypeInfo pca9552_info = {
> .name = TYPE_PCA9552,
> - .parent = TYPE_I2C_SLAVE,
> - .instance_init = pca955x_initfn,
> - .instance_size = sizeof(PCA955xState),
> + .parent = TYPE_PCA955X,
> .class_init = pca9552_class_init,
> };
>
> static void pca955x_register_types(void)
> {
> + type_register_static(&pca955x_info);
> type_register_static(&pca9552_info);
> }
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v5 5/9] hw/misc/pca9552: Add a 'description' property for debugging purpose
2020-06-22 18:34 [PATCH v5 0/9] hw/misc/pca9552: Trace GPIO change events Philippe Mathieu-Daudé
` (3 preceding siblings ...)
2020-06-22 18:34 ` [PATCH v5 4/9] hw/misc/pca9552: Add generic PCA955xClass, parent of TYPE_PCA9552 Philippe Mathieu-Daudé
@ 2020-06-22 18:34 ` Philippe Mathieu-Daudé
2020-06-22 18:34 ` [PATCH v5 6/9] hw/misc/pca9552: Trace GPIO High/Low events Philippe Mathieu-Daudé
` (3 subsequent siblings)
8 siblings, 0 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-22 18:34 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Corey Minyard, Andrew Jeffery,
Philippe Mathieu-Daudé,
qemu-arm, Joel Stanley, Cédric Le Goater
Add a description field to distinguish between multiple devices.
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
include/hw/misc/pca9552.h | 1 +
hw/misc/pca9552.c | 11 +++++++++++
2 files changed, 12 insertions(+)
diff --git a/include/hw/misc/pca9552.h b/include/hw/misc/pca9552.h
index 90843b03b8..bf1a589137 100644
--- a/include/hw/misc/pca9552.h
+++ b/include/hw/misc/pca9552.h
@@ -27,6 +27,7 @@ typedef struct PCA955xState {
uint8_t pointer;
uint8_t regs[PCA955X_NR_REGS];
+ char *description; /* For debugging purpose only */
} PCA955xState;
#endif
diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
index 34062dbb69..d6d84c6451 100644
--- a/hw/misc/pca9552.c
+++ b/hw/misc/pca9552.c
@@ -13,6 +13,7 @@
#include "qemu/osdep.h"
#include "qemu/log.h"
#include "qemu/module.h"
+#include "hw/qdev-properties.h"
#include "hw/misc/pca9552.h"
#include "hw/misc/pca9552_regs.h"
#include "migration/vmstate.h"
@@ -319,10 +320,19 @@ static void pca955x_initfn(Object *obj)
static void pca955x_realize(DeviceState *dev, Error **errp)
{
PCA955xClass *k = PCA955X_GET_CLASS(dev);
+ PCA955xState *s = PCA955X(dev);
assert(k->pin_count <= PCA955X_PIN_COUNT_MAX);
+ if (!s->description) {
+ s->description = g_strdup("pca-unspecified");
+ }
}
+static Property pca955x_properties[] = {
+ DEFINE_PROP_STRING("description", PCA955xState, description),
+ DEFINE_PROP_END_OF_LIST(),
+};
+
static void pca955x_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
@@ -332,6 +342,7 @@ static void pca955x_class_init(ObjectClass *klass, void *data)
k->recv = pca955x_recv;
k->send = pca955x_send;
dc->realize = pca955x_realize;
+ device_class_set_props(dc, pca955x_properties);
}
static const TypeInfo pca955x_info = {
--
2.21.3
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v5 6/9] hw/misc/pca9552: Trace GPIO High/Low events
2020-06-22 18:34 [PATCH v5 0/9] hw/misc/pca9552: Trace GPIO change events Philippe Mathieu-Daudé
` (4 preceding siblings ...)
2020-06-22 18:34 ` [PATCH v5 5/9] hw/misc/pca9552: Add a 'description' property for debugging purpose Philippe Mathieu-Daudé
@ 2020-06-22 18:34 ` Philippe Mathieu-Daudé
2020-06-23 6:03 ` Cédric Le Goater
2020-06-22 18:34 ` [PATCH v5 7/9] hw/arm/aspeed: Describe each PCA9552 device Philippe Mathieu-Daudé
` (2 subsequent siblings)
8 siblings, 1 reply; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-22 18:34 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Corey Minyard, Andrew Jeffery,
Philippe Mathieu-Daudé,
qemu-arm, Joel Stanley, Cédric Le Goater
Add a trivial representation of the PCA9552 GPIOs.
Example booting obmc-phosphor-image:
$ qemu-system-arm -M witherspoon-bmc -trace pca955x_gpio_status
1592689902.327837:pca955x_gpio_status pca-unspecified GPIOs 0-15 [*...............]
1592689902.329934:pca955x_gpio_status pca-unspecified GPIOs 0-15 [**..............]
1592689902.330717:pca955x_gpio_status pca-unspecified GPIOs 0-15 [***.............]
1592689902.331431:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****............]
1592689902.332163:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****.........*..]
1592689902.332888:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****.........**.]
1592689902.333629:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****.........***]
1592690032.793289:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****.........*.*]
1592690033.303163:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****.........***]
1592690033.812962:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****.........*.*]
1592690034.323234:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****.........***]
1592690034.832922:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****.........*.*]
We notice the GPIO #14 (front-power LED) starts to blink.
This LED is described in the witherspoon device-tree [*]:
front-power {
retain-state-shutdown;
default-state = "keep";
gpios = <&pca0 14 GPIO_ACTIVE_LOW>;
};
[*] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts?id=b1f9be9392f0#n140
Suggested-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
hw/misc/pca9552.c | 39 +++++++++++++++++++++++++++++++++++++++
hw/misc/trace-events | 3 +++
2 files changed, 42 insertions(+)
diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
index d6d84c6451..13f5ed0da4 100644
--- a/hw/misc/pca9552.c
+++ b/hw/misc/pca9552.c
@@ -13,12 +13,14 @@
#include "qemu/osdep.h"
#include "qemu/log.h"
#include "qemu/module.h"
+#include "qemu/bitops.h"
#include "hw/qdev-properties.h"
#include "hw/misc/pca9552.h"
#include "hw/misc/pca9552_regs.h"
#include "migration/vmstate.h"
#include "qapi/error.h"
#include "qapi/visitor.h"
+#include "trace.h"
typedef struct PCA955xClass {
/*< private >*/
@@ -49,6 +51,39 @@ static uint8_t pca955x_pin_get_config(PCA955xState *s, int pin)
return extract32(s->regs[reg], shift, 2);
}
+/* Return INPUT status (bit #N belongs to GPIO #N) */
+static uint16_t pca955x_pins_get_status(PCA955xState *s)
+{
+ return (s->regs[PCA9552_INPUT1] << 8) | s->regs[PCA9552_INPUT0];
+}
+
+static void pca955x_display_pins_status(PCA955xState *s,
+ uint16_t previous_pins_status)
+{
+ PCA955xClass *k = PCA955X_GET_CLASS(s);
+ uint16_t pins_status, pins_changed;
+ int i;
+
+ pins_status = pca955x_pins_get_status(s);
+ pins_changed = previous_pins_status ^ pins_status;
+ if (!pins_changed) {
+ return;
+ }
+ if (trace_event_get_state_backends(TRACE_PCA955X_GPIO_STATUS)) {
+ char *buf = g_newa(char, k->pin_count + 1);
+
+ for (i = 0; i < k->pin_count; i++) {
+ if (extract32(pins_status, i, 1)) {
+ buf[i] = '*';
+ } else {
+ buf[i] = '.';
+ }
+ }
+ buf[i] = '\0';
+ trace_pca955x_gpio_status(s->description, buf);
+ }
+}
+
static void pca955x_update_pin_input(PCA955xState *s)
{
PCA955xClass *k = PCA955X_GET_CLASS(s);
@@ -98,6 +133,8 @@ static uint8_t pca955x_read(PCA955xState *s, uint8_t reg)
static void pca955x_write(PCA955xState *s, uint8_t reg, uint8_t data)
{
+ uint16_t pins_status;
+
switch (reg) {
case PCA9552_PSC0:
case PCA9552_PWM0:
@@ -110,8 +147,10 @@ static void pca955x_write(PCA955xState *s, uint8_t reg, uint8_t data)
case PCA9552_LS1:
case PCA9552_LS2:
case PCA9552_LS3:
+ pins_status = pca955x_pins_get_status(s);
s->regs[reg] = data;
pca955x_update_pin_input(s);
+ pca955x_display_pins_status(s, pins_status);
break;
case PCA9552_INPUT0:
diff --git a/hw/misc/trace-events b/hw/misc/trace-events
index 5561746866..9282c60dd9 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -206,3 +206,6 @@ via1_rtc_cmd_pram_sect_write(int sector, int offset, int addr, int value) "secto
# grlib_ahb_apb_pnp.c
grlib_ahb_pnp_read(uint64_t addr, uint32_t value) "AHB PnP read addr:0x%03"PRIx64" data:0x%08x"
grlib_apb_pnp_read(uint64_t addr, uint32_t value) "APB PnP read addr:0x%03"PRIx64" data:0x%08x"
+
+# pca9552.c
+pca955x_gpio_status(const char *description, const char *buf) "%s GPIOs 0-15 [%s]"
--
2.21.3
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v5 6/9] hw/misc/pca9552: Trace GPIO High/Low events
2020-06-22 18:34 ` [PATCH v5 6/9] hw/misc/pca9552: Trace GPIO High/Low events Philippe Mathieu-Daudé
@ 2020-06-23 6:03 ` Cédric Le Goater
0 siblings, 0 replies; 22+ messages in thread
From: Cédric Le Goater @ 2020-06-23 6:03 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Andrew Jeffery, Peter Maydell, qemu-arm, Joel Stanley, Corey Minyard
On 6/22/20 8:34 PM, Philippe Mathieu-Daudé wrote:
> Add a trivial representation of the PCA9552 GPIOs.
>
> Example booting obmc-phosphor-image:
>
> $ qemu-system-arm -M witherspoon-bmc -trace pca955x_gpio_status
> 1592689902.327837:pca955x_gpio_status pca-unspecified GPIOs 0-15 [*...............]
> 1592689902.329934:pca955x_gpio_status pca-unspecified GPIOs 0-15 [**..............]
> 1592689902.330717:pca955x_gpio_status pca-unspecified GPIOs 0-15 [***.............]
> 1592689902.331431:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****............]
> 1592689902.332163:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****.........*..]
> 1592689902.332888:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****.........**.]
> 1592689902.333629:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****.........***]
> 1592690032.793289:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****.........*.*]
> 1592690033.303163:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****.........***]
> 1592690033.812962:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****.........*.*]
> 1592690034.323234:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****.........***]
> 1592690034.832922:pca955x_gpio_status pca-unspecified GPIOs 0-15 [****.........*.*]
>
> We notice the GPIO #14 (front-power LED) starts to blink.
>
> This LED is described in the witherspoon device-tree [*]:
>
> front-power {
> retain-state-shutdown;
> default-state = "keep";
> gpios = <&pca0 14 GPIO_ACTIVE_LOW>;
> };
>
> [*] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts?id=b1f9be9392f0#n140
>
> Suggested-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
> ---
> hw/misc/pca9552.c | 39 +++++++++++++++++++++++++++++++++++++++
> hw/misc/trace-events | 3 +++
> 2 files changed, 42 insertions(+)
>
> diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
> index d6d84c6451..13f5ed0da4 100644
> --- a/hw/misc/pca9552.c
> +++ b/hw/misc/pca9552.c
> @@ -13,12 +13,14 @@
> #include "qemu/osdep.h"
> #include "qemu/log.h"
> #include "qemu/module.h"
> +#include "qemu/bitops.h"
> #include "hw/qdev-properties.h"
> #include "hw/misc/pca9552.h"
> #include "hw/misc/pca9552_regs.h"
> #include "migration/vmstate.h"
> #include "qapi/error.h"
> #include "qapi/visitor.h"
> +#include "trace.h"
>
> typedef struct PCA955xClass {
> /*< private >*/
> @@ -49,6 +51,39 @@ static uint8_t pca955x_pin_get_config(PCA955xState *s, int pin)
> return extract32(s->regs[reg], shift, 2);
> }
>
> +/* Return INPUT status (bit #N belongs to GPIO #N) */
> +static uint16_t pca955x_pins_get_status(PCA955xState *s)
> +{
> + return (s->regs[PCA9552_INPUT1] << 8) | s->regs[PCA9552_INPUT0];
> +}
> +
> +static void pca955x_display_pins_status(PCA955xState *s,
> + uint16_t previous_pins_status)
> +{
> + PCA955xClass *k = PCA955X_GET_CLASS(s);
> + uint16_t pins_status, pins_changed;
> + int i;
> +
> + pins_status = pca955x_pins_get_status(s);
> + pins_changed = previous_pins_status ^ pins_status;
> + if (!pins_changed) {
> + return;
> + }
> + if (trace_event_get_state_backends(TRACE_PCA955X_GPIO_STATUS)) {
> + char *buf = g_newa(char, k->pin_count + 1);
> +
> + for (i = 0; i < k->pin_count; i++) {
> + if (extract32(pins_status, i, 1)) {
> + buf[i] = '*';
> + } else {
> + buf[i] = '.';
> + }
> + }
> + buf[i] = '\0';
> + trace_pca955x_gpio_status(s->description, buf);
> + }
> +}
> +
> static void pca955x_update_pin_input(PCA955xState *s)
> {
> PCA955xClass *k = PCA955X_GET_CLASS(s);
> @@ -98,6 +133,8 @@ static uint8_t pca955x_read(PCA955xState *s, uint8_t reg)
>
> static void pca955x_write(PCA955xState *s, uint8_t reg, uint8_t data)
> {
> + uint16_t pins_status;
> +
> switch (reg) {
> case PCA9552_PSC0:
> case PCA9552_PWM0:
> @@ -110,8 +147,10 @@ static void pca955x_write(PCA955xState *s, uint8_t reg, uint8_t data)
> case PCA9552_LS1:
> case PCA9552_LS2:
> case PCA9552_LS3:
> + pins_status = pca955x_pins_get_status(s);
> s->regs[reg] = data;
> pca955x_update_pin_input(s);
> + pca955x_display_pins_status(s, pins_status);
> break;
>
> case PCA9552_INPUT0:
> diff --git a/hw/misc/trace-events b/hw/misc/trace-events
> index 5561746866..9282c60dd9 100644
> --- a/hw/misc/trace-events
> +++ b/hw/misc/trace-events
> @@ -206,3 +206,6 @@ via1_rtc_cmd_pram_sect_write(int sector, int offset, int addr, int value) "secto
> # grlib_ahb_apb_pnp.c
> grlib_ahb_pnp_read(uint64_t addr, uint32_t value) "AHB PnP read addr:0x%03"PRIx64" data:0x%08x"
> grlib_apb_pnp_read(uint64_t addr, uint32_t value) "APB PnP read addr:0x%03"PRIx64" data:0x%08x"
> +
> +# pca9552.c
> +pca955x_gpio_status(const char *description, const char *buf) "%s GPIOs 0-15 [%s]"
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v5 7/9] hw/arm/aspeed: Describe each PCA9552 device
2020-06-22 18:34 [PATCH v5 0/9] hw/misc/pca9552: Trace GPIO change events Philippe Mathieu-Daudé
` (5 preceding siblings ...)
2020-06-22 18:34 ` [PATCH v5 6/9] hw/misc/pca9552: Trace GPIO High/Low events Philippe Mathieu-Daudé
@ 2020-06-22 18:34 ` Philippe Mathieu-Daudé
2020-06-23 6:07 ` Cédric Le Goater
2020-06-23 8:43 ` Markus Armbruster
2020-06-22 18:34 ` [PATCH v5 8/9] hw/misc/pca9552: Trace GPIO change events Philippe Mathieu-Daudé
2020-06-22 18:34 ` [PATCH v5 9/9] hw/misc/pca9552: Model qdev output GPIOs Philippe Mathieu-Daudé
8 siblings, 2 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-22 18:34 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Corey Minyard, Andrew Jeffery,
Philippe Mathieu-Daudé,
Markus Armbruster, qemu-arm, Joel Stanley, Cédric Le Goater
We have 2 distinct PCA9552 devices. Set their description
to distinguish them when looking at the trace events.
Description name taken from:
https://github.com/open-power/witherspoon-xml/blob/master/witherspoon.xml
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
Cc: Markus Armbruster <armbru@redhat.com>
---
hw/arm/aspeed.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 6b7533aeee..3d5dec4692 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -508,12 +508,15 @@ static void witherspoon_bmc_i2c_init(AspeedBoardState *bmc)
{
AspeedSoCState *soc = &bmc->soc;
uint8_t *eeprom_buf = g_malloc0(8 * 1024);
+ DeviceState *dev;
/* Bus 3: TODO bmp280@77 */
/* Bus 3: TODO max31785@52 */
/* Bus 3: TODO dps310@76 */
- i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 3), TYPE_PCA9552,
- 0x60);
+ dev = i2c_try_create_slave(TYPE_PCA9552, 0x60);
+ qdev_prop_set_string(dev, "description", "pca1");
+ i2c_realize_and_unref(dev, aspeed_i2c_get_bus(DEVICE(&soc->i2c), 3),
+ &error_fatal);
i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 4), "tmp423", 0x4c);
i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 5), "tmp423", 0x4c);
@@ -528,8 +531,10 @@ static void witherspoon_bmc_i2c_init(AspeedBoardState *bmc)
smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), 0x51,
eeprom_buf);
- i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), TYPE_PCA9552,
- 0x60);
+ dev = i2c_try_create_slave(TYPE_PCA9552, 0x60);
+ qdev_prop_set_string(dev, "description", "pca0");
+ i2c_realize_and_unref(dev, aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11),
+ &error_fatal);
/* Bus 11: TODO ucd90160@64 */
}
--
2.21.3
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v5 7/9] hw/arm/aspeed: Describe each PCA9552 device
2020-06-22 18:34 ` [PATCH v5 7/9] hw/arm/aspeed: Describe each PCA9552 device Philippe Mathieu-Daudé
@ 2020-06-23 6:07 ` Cédric Le Goater
2020-06-23 8:43 ` Markus Armbruster
1 sibling, 0 replies; 22+ messages in thread
From: Cédric Le Goater @ 2020-06-23 6:07 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Peter Maydell, Corey Minyard, Andrew Jeffery, Markus Armbruster,
qemu-arm, Joel Stanley
On 6/22/20 8:34 PM, Philippe Mathieu-Daudé wrote:
> We have 2 distinct PCA9552 devices. Set their description
> to distinguish them when looking at the trace events.
>
> Description name taken from:
> https://github.com/open-power/witherspoon-xml/blob/master/witherspoon.xml
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
> ---
> Cc: Markus Armbruster <armbru@redhat.com>
> ---
> hw/arm/aspeed.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 6b7533aeee..3d5dec4692 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -508,12 +508,15 @@ static void witherspoon_bmc_i2c_init(AspeedBoardState *bmc)
> {
> AspeedSoCState *soc = &bmc->soc;
> uint8_t *eeprom_buf = g_malloc0(8 * 1024);
> + DeviceState *dev;
>
> /* Bus 3: TODO bmp280@77 */
> /* Bus 3: TODO max31785@52 */
> /* Bus 3: TODO dps310@76 */
> - i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 3), TYPE_PCA9552,
> - 0x60);
> + dev = i2c_try_create_slave(TYPE_PCA9552, 0x60);
> + qdev_prop_set_string(dev, "description", "pca1");
> + i2c_realize_and_unref(dev, aspeed_i2c_get_bus(DEVICE(&soc->i2c), 3),
> + &error_fatal);
>
> i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 4), "tmp423", 0x4c);
> i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 5), "tmp423", 0x4c);
> @@ -528,8 +531,10 @@ static void witherspoon_bmc_i2c_init(AspeedBoardState *bmc)
>
> smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), 0x51,
> eeprom_buf);
> - i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), TYPE_PCA9552,
> - 0x60);
> + dev = i2c_try_create_slave(TYPE_PCA9552, 0x60);
> + qdev_prop_set_string(dev, "description", "pca0");
> + i2c_realize_and_unref(dev, aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11),
> + &error_fatal);
> /* Bus 11: TODO ucd90160@64 */
> }
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v5 7/9] hw/arm/aspeed: Describe each PCA9552 device
2020-06-22 18:34 ` [PATCH v5 7/9] hw/arm/aspeed: Describe each PCA9552 device Philippe Mathieu-Daudé
2020-06-23 6:07 ` Cédric Le Goater
@ 2020-06-23 8:43 ` Markus Armbruster
1 sibling, 0 replies; 22+ messages in thread
From: Markus Armbruster @ 2020-06-23 8:43 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Corey Minyard, Andrew Jeffery, qemu-devel,
qemu-arm, Joel Stanley, Cédric Le Goater
Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> We have 2 distinct PCA9552 devices. Set their description
> to distinguish them when looking at the trace events.
>
> Description name taken from:
> https://github.com/open-power/witherspoon-xml/blob/master/witherspoon.xml
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v5 8/9] hw/misc/pca9552: Trace GPIO change events
2020-06-22 18:34 [PATCH v5 0/9] hw/misc/pca9552: Trace GPIO change events Philippe Mathieu-Daudé
` (6 preceding siblings ...)
2020-06-22 18:34 ` [PATCH v5 7/9] hw/arm/aspeed: Describe each PCA9552 device Philippe Mathieu-Daudé
@ 2020-06-22 18:34 ` Philippe Mathieu-Daudé
2020-06-23 6:04 ` Cédric Le Goater
2020-06-22 18:34 ` [PATCH v5 9/9] hw/misc/pca9552: Model qdev output GPIOs Philippe Mathieu-Daudé
8 siblings, 1 reply; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-22 18:34 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Corey Minyard, Andrew Jeffery,
Philippe Mathieu-Daudé,
qemu-arm, Joel Stanley, Cédric Le Goater
Emit a trace event when a GPIO change its state.
Example booting obmc-phosphor-image:
$ qemu-system-arm -M witherspoon-bmc -trace pca955x_gpio_change
1592690552.687372:pca955x_gpio_change pca1 GPIO id:0 status: 0 -> 1
1592690552.690169:pca955x_gpio_change pca1 GPIO id:1 status: 0 -> 1
1592690552.691673:pca955x_gpio_change pca1 GPIO id:2 status: 0 -> 1
1592690552.696886:pca955x_gpio_change pca1 GPIO id:3 status: 0 -> 1
1592690552.698614:pca955x_gpio_change pca1 GPIO id:13 status: 0 -> 1
1592690552.699833:pca955x_gpio_change pca1 GPIO id:14 status: 0 -> 1
1592690552.700842:pca955x_gpio_change pca1 GPIO id:15 status: 0 -> 1
1592690683.841921:pca955x_gpio_change pca1 GPIO id:14 status: 1 -> 0
1592690683.861660:pca955x_gpio_change pca1 GPIO id:14 status: 0 -> 1
1592690684.371460:pca955x_gpio_change pca1 GPIO id:14 status: 1 -> 0
1592690684.882115:pca955x_gpio_change pca1 GPIO id:14 status: 0 -> 1
1592690685.391411:pca955x_gpio_change pca1 GPIO id:14 status: 1 -> 0
1592690685.901391:pca955x_gpio_change pca1 GPIO id:14 status: 0 -> 1
1592690686.411678:pca955x_gpio_change pca1 GPIO id:14 status: 1 -> 0
1592690686.921279:pca955x_gpio_change pca1 GPIO id:14 status: 0 -> 1
We notice the GPIO #14 (front-power LED) starts to blink.
This LED is described in the witherspoon device-tree [*]:
front-power {
retain-state-shutdown;
default-state = "keep";
gpios = <&pca0 14 GPIO_ACTIVE_LOW>;
};
[*] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts?id=b1f9be9392f0#n140
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
hw/misc/pca9552.c | 15 +++++++++++++++
hw/misc/trace-events | 1 +
2 files changed, 16 insertions(+)
diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
index 13f5ed0da4..5997eef8b2 100644
--- a/hw/misc/pca9552.c
+++ b/hw/misc/pca9552.c
@@ -82,6 +82,21 @@ static void pca955x_display_pins_status(PCA955xState *s,
buf[i] = '\0';
trace_pca955x_gpio_status(s->description, buf);
}
+ if (trace_event_get_state_backends(TRACE_PCA955X_GPIO_CHANGE)) {
+ for (i = 0; i < k->pin_count; i++) {
+ if (extract32(pins_changed, i, 1)) {
+ unsigned new_state = extract32(pins_status, i, 1);
+
+ /*
+ * We display the state using the PCA logic ("active-high").
+ * This is not the state of the LED, which signal might be
+ * wired "active-low" on the board.
+ */
+ trace_pca955x_gpio_change(s->description, i,
+ !new_state, new_state);
+ }
+ }
+ }
}
static void pca955x_update_pin_input(PCA955xState *s)
diff --git a/hw/misc/trace-events b/hw/misc/trace-events
index 9282c60dd9..7ccf683dd1 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -209,3 +209,4 @@ grlib_apb_pnp_read(uint64_t addr, uint32_t value) "APB PnP read addr:0x%03"PRIx6
# pca9552.c
pca955x_gpio_status(const char *description, const char *buf) "%s GPIOs 0-15 [%s]"
+pca955x_gpio_change(const char *description, unsigned id, unsigned prev_state, unsigned current_state) "%s GPIO id:%u status: %u -> %u"
--
2.21.3
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v5 8/9] hw/misc/pca9552: Trace GPIO change events
2020-06-22 18:34 ` [PATCH v5 8/9] hw/misc/pca9552: Trace GPIO change events Philippe Mathieu-Daudé
@ 2020-06-23 6:04 ` Cédric Le Goater
0 siblings, 0 replies; 22+ messages in thread
From: Cédric Le Goater @ 2020-06-23 6:04 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Andrew Jeffery, Peter Maydell, qemu-arm, Joel Stanley, Corey Minyard
On 6/22/20 8:34 PM, Philippe Mathieu-Daudé wrote:
> Emit a trace event when a GPIO change its state.
>
> Example booting obmc-phosphor-image:
>
> $ qemu-system-arm -M witherspoon-bmc -trace pca955x_gpio_change
> 1592690552.687372:pca955x_gpio_change pca1 GPIO id:0 status: 0 -> 1
> 1592690552.690169:pca955x_gpio_change pca1 GPIO id:1 status: 0 -> 1
> 1592690552.691673:pca955x_gpio_change pca1 GPIO id:2 status: 0 -> 1
> 1592690552.696886:pca955x_gpio_change pca1 GPIO id:3 status: 0 -> 1
> 1592690552.698614:pca955x_gpio_change pca1 GPIO id:13 status: 0 -> 1
> 1592690552.699833:pca955x_gpio_change pca1 GPIO id:14 status: 0 -> 1
> 1592690552.700842:pca955x_gpio_change pca1 GPIO id:15 status: 0 -> 1
> 1592690683.841921:pca955x_gpio_change pca1 GPIO id:14 status: 1 -> 0
> 1592690683.861660:pca955x_gpio_change pca1 GPIO id:14 status: 0 -> 1
> 1592690684.371460:pca955x_gpio_change pca1 GPIO id:14 status: 1 -> 0
> 1592690684.882115:pca955x_gpio_change pca1 GPIO id:14 status: 0 -> 1
> 1592690685.391411:pca955x_gpio_change pca1 GPIO id:14 status: 1 -> 0
> 1592690685.901391:pca955x_gpio_change pca1 GPIO id:14 status: 0 -> 1
> 1592690686.411678:pca955x_gpio_change pca1 GPIO id:14 status: 1 -> 0
> 1592690686.921279:pca955x_gpio_change pca1 GPIO id:14 status: 0 -> 1
>
> We notice the GPIO #14 (front-power LED) starts to blink.
>
> This LED is described in the witherspoon device-tree [*]:
>
> front-power {
> retain-state-shutdown;
> default-state = "keep";
> gpios = <&pca0 14 GPIO_ACTIVE_LOW>;
> };
>
> [*] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts?id=b1f9be9392f0#n140
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
> ---
> hw/misc/pca9552.c | 15 +++++++++++++++
> hw/misc/trace-events | 1 +
> 2 files changed, 16 insertions(+)
>
> diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
> index 13f5ed0da4..5997eef8b2 100644
> --- a/hw/misc/pca9552.c
> +++ b/hw/misc/pca9552.c
> @@ -82,6 +82,21 @@ static void pca955x_display_pins_status(PCA955xState *s,
> buf[i] = '\0';
> trace_pca955x_gpio_status(s->description, buf);
> }
> + if (trace_event_get_state_backends(TRACE_PCA955X_GPIO_CHANGE)) {
> + for (i = 0; i < k->pin_count; i++) {
> + if (extract32(pins_changed, i, 1)) {
> + unsigned new_state = extract32(pins_status, i, 1);
> +
> + /*
> + * We display the state using the PCA logic ("active-high").
> + * This is not the state of the LED, which signal might be
> + * wired "active-low" on the board.
> + */
> + trace_pca955x_gpio_change(s->description, i,
> + !new_state, new_state);
> + }
> + }
> + }
> }
>
> static void pca955x_update_pin_input(PCA955xState *s)
> diff --git a/hw/misc/trace-events b/hw/misc/trace-events
> index 9282c60dd9..7ccf683dd1 100644
> --- a/hw/misc/trace-events
> +++ b/hw/misc/trace-events
> @@ -209,3 +209,4 @@ grlib_apb_pnp_read(uint64_t addr, uint32_t value) "APB PnP read addr:0x%03"PRIx6
>
> # pca9552.c
> pca955x_gpio_status(const char *description, const char *buf) "%s GPIOs 0-15 [%s]"
> +pca955x_gpio_change(const char *description, unsigned id, unsigned prev_state, unsigned current_state) "%s GPIO id:%u status: %u -> %u"
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v5 9/9] hw/misc/pca9552: Model qdev output GPIOs
2020-06-22 18:34 [PATCH v5 0/9] hw/misc/pca9552: Trace GPIO change events Philippe Mathieu-Daudé
` (7 preceding siblings ...)
2020-06-22 18:34 ` [PATCH v5 8/9] hw/misc/pca9552: Trace GPIO change events Philippe Mathieu-Daudé
@ 2020-06-22 18:34 ` Philippe Mathieu-Daudé
8 siblings, 0 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-22 18:34 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Corey Minyard, Andrew Jeffery,
Philippe Mathieu-Daudé,
qemu-arm, Joel Stanley, Cédric Le Goater
The PCA9552 has 16 GPIOs which can be used as input,
output or PWM mode. QEMU models the output GPIO with
the qemu_irq type. Let the device expose the 16 GPIOs
to allow us to later connect LEDs to these outputs.
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
include/hw/misc/pca9552.h | 1 +
hw/misc/pca9552.c | 5 +++++
2 files changed, 6 insertions(+)
diff --git a/include/hw/misc/pca9552.h b/include/hw/misc/pca9552.h
index bf1a589137..600356fbf9 100644
--- a/include/hw/misc/pca9552.h
+++ b/include/hw/misc/pca9552.h
@@ -27,6 +27,7 @@ typedef struct PCA955xState {
uint8_t pointer;
uint8_t regs[PCA955X_NR_REGS];
+ qemu_irq gpio[PCA955X_PIN_COUNT_MAX];
char *description; /* For debugging purpose only */
} PCA955xState;
diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
index 5997eef8b2..38c04c54dc 100644
--- a/hw/misc/pca9552.c
+++ b/hw/misc/pca9552.c
@@ -17,6 +17,7 @@
#include "hw/qdev-properties.h"
#include "hw/misc/pca9552.h"
#include "hw/misc/pca9552_regs.h"
+#include "hw/irq.h"
#include "migration/vmstate.h"
#include "qapi/error.h"
#include "qapi/visitor.h"
@@ -111,9 +112,11 @@ static void pca955x_update_pin_input(PCA955xState *s)
switch (config) {
case PCA9552_LED_ON:
+ qemu_set_irq(s->gpio[i], 1);
s->regs[input_reg] |= 1 << input_shift;
break;
case PCA9552_LED_OFF:
+ qemu_set_irq(s->gpio[i], 0);
s->regs[input_reg] &= ~(1 << input_shift);
break;
case PCA9552_LED_PWM0:
@@ -377,6 +380,8 @@ static void pca955x_realize(DeviceState *dev, Error **errp)
PCA955xState *s = PCA955X(dev);
assert(k->pin_count <= PCA955X_PIN_COUNT_MAX);
+ qdev_init_gpio_out(dev, s->gpio, k->pin_count);
+
if (!s->description) {
s->description = g_strdup("pca-unspecified");
}
--
2.21.3
^ permalink raw reply related [flat|nested] 22+ messages in thread