* [PATCH 0/5] hw/i2c: Rename method names for consistency and add documentation
@ 2020-06-29 17:38 Philippe Mathieu-Daudé
2020-06-29 17:38 ` [PATCH 1/5] hw/i2c/aspeed_i2c: Simplify aspeed_i2c_get_bus() Philippe Mathieu-Daudé
` (5 more replies)
0 siblings, 6 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-29 17:38 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Philippe Mathieu-Daudé,
Corey Minyard, Andrew Jeffery, Markus Armbruster,
Cédric Le Goater, qemu-arm, qemu-ppc, Joel Stanley,
Jan Kiszka, David Gibson
In commit d88c42ff2c we added 2 methods: i2c_try_create_slave()
and i2c_realize_and_unref().
Markus noted their name could be improved for consistency [1],
and Peter reported the lack of documentation [2]. Fix that now.
[1] https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg07060.html
[2] https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg08997.html
Philippe Mathieu-Daudé (5):
hw/i2c/aspeed_i2c: Simplify aspeed_i2c_get_bus()
hw/i2c: Rename i2c_try_create_slave() as i2c_slave_new()
hw/i2c: Rename i2c_realize_and_unref() as
i2c_slave_realize_and_unref()
hw/i2c: Rename i2c_create_slave() as i2c_slave_create_simple()
hw/i2c: Document the I2C qdev helpers
include/hw/i2c/aspeed_i2c.h | 2 +-
include/hw/i2c/i2c.h | 54 ++++++++++++++++++++++--
hw/arm/aspeed.c | 82 +++++++++++++++++++------------------
hw/arm/musicpal.c | 4 +-
hw/arm/nseries.c | 8 ++--
hw/arm/pxa2xx.c | 5 ++-
hw/arm/realview.c | 2 +-
hw/arm/spitz.c | 4 +-
hw/arm/stellaris.c | 2 +-
hw/arm/tosa.c | 2 +-
hw/arm/versatilepb.c | 2 +-
hw/arm/vexpress.c | 2 +-
hw/arm/z2.c | 4 +-
hw/display/sii9022.c | 2 +-
hw/i2c/aspeed_i2c.c | 3 +-
hw/i2c/core.c | 15 ++++---
hw/ppc/e500.c | 2 +-
hw/ppc/sam460ex.c | 2 +-
18 files changed, 123 insertions(+), 74 deletions(-)
--
2.21.3
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/5] hw/i2c/aspeed_i2c: Simplify aspeed_i2c_get_bus()
2020-06-29 17:38 [PATCH 0/5] hw/i2c: Rename method names for consistency and add documentation Philippe Mathieu-Daudé
@ 2020-06-29 17:38 ` Philippe Mathieu-Daudé
2020-06-30 9:28 ` Markus Armbruster
2020-07-13 12:23 ` Cédric Le Goater
2020-06-29 17:38 ` [PATCH 2/5] hw/i2c: Rename i2c_try_create_slave() as i2c_slave_new() Philippe Mathieu-Daudé
` (4 subsequent siblings)
5 siblings, 2 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-29 17:38 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Philippe Mathieu-Daudé,
Corey Minyard, Andrew Jeffery, Markus Armbruster,
Cédric Le Goater, qemu-arm, qemu-ppc, Joel Stanley,
Jan Kiszka, David Gibson
Simplify aspeed_i2c_get_bus() by using a AspeedI2CState argument.
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
include/hw/i2c/aspeed_i2c.h | 2 +-
hw/arm/aspeed.c | 70 ++++++++++++++++++-------------------
hw/i2c/aspeed_i2c.c | 3 +-
3 files changed, 37 insertions(+), 38 deletions(-)
diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
index f1b9e5bf91..243789ae5d 100644
--- a/include/hw/i2c/aspeed_i2c.h
+++ b/include/hw/i2c/aspeed_i2c.h
@@ -93,6 +93,6 @@ typedef struct AspeedI2CClass {
} AspeedI2CClass;
-I2CBus *aspeed_i2c_get_bus(DeviceState *dev, int busnr);
+I2CBus *aspeed_i2c_get_bus(AspeedI2CState *s, int busnr);
#endif /* ASPEED_I2C_H */
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 379f9672a5..1285bf82c0 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -385,13 +385,13 @@ static void palmetto_bmc_i2c_init(AspeedMachineState *bmc)
/* The palmetto platform expects a ds3231 RTC but a ds1338 is
* enough to provide basic RTC features. Alarms will be missing */
- i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 0), "ds1338", 0x68);
+ i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 0), "ds1338", 0x68);
- smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 0), 0x50,
+ smbus_eeprom_init_one(aspeed_i2c_get_bus(&soc->i2c, 0), 0x50,
eeprom_buf);
/* add a TMP423 temperature sensor */
- dev = i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 2),
+ dev = i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 2),
"tmp423", 0x4c);
object_property_set_int(OBJECT(dev), 31000, "temperature0", &error_abort);
object_property_set_int(OBJECT(dev), 28000, "temperature1", &error_abort);
@@ -404,16 +404,16 @@ static void ast2500_evb_i2c_init(AspeedMachineState *bmc)
AspeedSoCState *soc = &bmc->soc;
uint8_t *eeprom_buf = g_malloc0(8 * 1024);
- smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 3), 0x50,
+ smbus_eeprom_init_one(aspeed_i2c_get_bus(&soc->i2c, 3), 0x50,
eeprom_buf);
/* The AST2500 EVB expects a LM75 but a TMP105 is compatible */
- i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 7),
+ i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 7),
TYPE_TMP105, 0x4d);
/* The AST2500 EVB does not have an RTC. Let's pretend that one is
* plugged on the I2C bus header */
- i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), "ds1338", 0x32);
+ i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 11), "ds1338", 0x32);
}
static void ast2600_evb_i2c_init(AspeedMachineState *bmc)
@@ -428,36 +428,36 @@ static void romulus_bmc_i2c_init(AspeedMachineState *bmc)
/* The romulus board expects Epson RX8900 I2C RTC but a ds1338 is
* good enough */
- i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), "ds1338", 0x32);
+ i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 11), "ds1338", 0x32);
}
static void swift_bmc_i2c_init(AspeedMachineState *bmc)
{
AspeedSoCState *soc = &bmc->soc;
- i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 3), "pca9552", 0x60);
+ i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 3), "pca9552", 0x60);
/* The swift board expects a TMP275 but a TMP105 is compatible */
- i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 7), "tmp105", 0x48);
+ i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 7), "tmp105", 0x48);
/* The swift board expects a pca9551 but a pca9552 is compatible */
- i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 7), "pca9552", 0x60);
+ i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 7), "pca9552", 0x60);
/* The swift board expects an Epson RX8900 RTC but a ds1338 is compatible */
- i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 8), "ds1338", 0x32);
- i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 8), "pca9552", 0x60);
+ i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 8), "ds1338", 0x32);
+ i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 8), "pca9552", 0x60);
- i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 9), "tmp423", 0x4c);
+ i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 9), "tmp423", 0x4c);
/* The swift board expects a pca9539 but a pca9552 is compatible */
- i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 9), "pca9552", 0x74);
+ i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 9), "pca9552", 0x74);
- i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 10), "tmp423", 0x4c);
+ i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 10), "tmp423", 0x4c);
/* The swift board expects a pca9539 but a pca9552 is compatible */
- i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 10), "pca9552",
+ i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 10), "pca9552",
0x74);
/* The swift board expects a TMP275 but a TMP105 is compatible */
- i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 12), "tmp105", 0x48);
- i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 12), "tmp105", 0x4a);
+ i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 12), "tmp105", 0x48);
+ i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 12), "tmp105", 0x4a);
}
static void sonorapass_bmc_i2c_init(AspeedMachineState *bmc)
@@ -465,32 +465,32 @@ static void sonorapass_bmc_i2c_init(AspeedMachineState *bmc)
AspeedSoCState *soc = &bmc->soc;
/* bus 2 : */
- i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 2), "tmp105", 0x48);
- i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 2), "tmp105", 0x49);
+ i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 2), "tmp105", 0x48);
+ i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 2), "tmp105", 0x49);
/* bus 2 : pca9546 @ 0x73 */
/* bus 3 : pca9548 @ 0x70 */
/* bus 4 : */
uint8_t *eeprom4_54 = g_malloc0(8 * 1024);
- smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 4), 0x54,
+ smbus_eeprom_init_one(aspeed_i2c_get_bus(&soc->i2c, 4), 0x54,
eeprom4_54);
/* PCA9539 @ 0x76, but PCA9552 is compatible */
- i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 4), "pca9552", 0x76);
+ i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 4), "pca9552", 0x76);
/* PCA9539 @ 0x77, but PCA9552 is compatible */
- i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 4), "pca9552", 0x77);
+ i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 4), "pca9552", 0x77);
/* bus 6 : */
- i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 6), "tmp105", 0x48);
- i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 6), "tmp105", 0x49);
+ i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 6), "tmp105", 0x48);
+ i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 6), "tmp105", 0x49);
/* bus 6 : pca9546 @ 0x73 */
/* bus 8 : */
uint8_t *eeprom8_56 = g_malloc0(8 * 1024);
- smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 8), 0x56,
+ smbus_eeprom_init_one(aspeed_i2c_get_bus(&soc->i2c, 8), 0x56,
eeprom8_56);
- i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 8), "pca9552", 0x60);
- i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 8), "pca9552", 0x61);
+ i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 8), "pca9552", 0x60);
+ i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 8), "pca9552", 0x61);
/* bus 8 : adc128d818 @ 0x1d */
/* bus 8 : adc128d818 @ 0x1f */
@@ -515,25 +515,25 @@ static void witherspoon_bmc_i2c_init(AspeedMachineState *bmc)
/* Bus 3: TODO dps310@76 */
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),
+ i2c_realize_and_unref(dev, aspeed_i2c_get_bus(&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);
+ i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 4), "tmp423", 0x4c);
+ i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 5), "tmp423", 0x4c);
/* The Witherspoon expects a TMP275 but a TMP105 is compatible */
- i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 9), TYPE_TMP105,
+ i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 9), TYPE_TMP105,
0x4a);
/* The witherspoon board expects Epson RX8900 I2C RTC but a ds1338 is
* good enough */
- i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), "ds1338", 0x32);
+ i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 11), "ds1338", 0x32);
- smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), 0x51,
+ smbus_eeprom_init_one(aspeed_i2c_get_bus(&soc->i2c, 11), 0x51,
eeprom_buf);
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),
+ i2c_realize_and_unref(dev, aspeed_i2c_get_bus(&soc->i2c, 11),
&error_fatal);
/* Bus 11: TODO ucd90160@64 */
}
diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index fb973a983d..518a3f5c6f 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -959,9 +959,8 @@ static void aspeed_i2c_register_types(void)
type_init(aspeed_i2c_register_types)
-I2CBus *aspeed_i2c_get_bus(DeviceState *dev, int busnr)
+I2CBus *aspeed_i2c_get_bus(AspeedI2CState *s, int busnr)
{
- AspeedI2CState *s = ASPEED_I2C(dev);
AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(s);
I2CBus *bus = NULL;
--
2.21.3
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 2/5] hw/i2c: Rename i2c_try_create_slave() as i2c_slave_new()
2020-06-29 17:38 [PATCH 0/5] hw/i2c: Rename method names for consistency and add documentation Philippe Mathieu-Daudé
2020-06-29 17:38 ` [PATCH 1/5] hw/i2c/aspeed_i2c: Simplify aspeed_i2c_get_bus() Philippe Mathieu-Daudé
@ 2020-06-29 17:38 ` Philippe Mathieu-Daudé
2020-06-29 21:29 ` Corey Minyard
2020-06-29 21:37 ` BALATON Zoltan
2020-06-29 17:38 ` [PATCH 3/5] hw/i2c: Rename i2c_realize_and_unref() as i2c_slave_realize_and_unref() Philippe Mathieu-Daudé
` (3 subsequent siblings)
5 siblings, 2 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-29 17:38 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Philippe Mathieu-Daudé,
Corey Minyard, Andrew Jeffery, Markus Armbruster,
Cédric Le Goater, qemu-arm, qemu-ppc, Joel Stanley,
Jan Kiszka, David Gibson
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(). Since we have to update
all the callers, also let it return a I2CSlave object.
Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
include/hw/i2c/i2c.h | 2 +-
hw/arm/aspeed.c | 4 ++--
hw/i2c/core.c | 9 ++++-----
3 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index d6e3d85faf..18efc668f1 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -79,8 +79,8 @@ 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);
+I2CSlave *i2c_slave_new(const char *name, uint8_t addr);
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 */
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 1285bf82c0..54ca36e0b6 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -513,7 +513,7 @@ static void witherspoon_bmc_i2c_init(AspeedMachineState *bmc)
/* Bus 3: TODO bmp280@77 */
/* Bus 3: TODO max31785@52 */
/* Bus 3: TODO dps310@76 */
- dev = i2c_try_create_slave(TYPE_PCA9552, 0x60);
+ dev = DEVICE(i2c_slave_new(TYPE_PCA9552, 0x60));
qdev_prop_set_string(dev, "description", "pca1");
i2c_realize_and_unref(dev, aspeed_i2c_get_bus(&soc->i2c, 3),
&error_fatal);
@@ -531,7 +531,7 @@ static void witherspoon_bmc_i2c_init(AspeedMachineState *bmc)
smbus_eeprom_init_one(aspeed_i2c_get_bus(&soc->i2c, 11), 0x51,
eeprom_buf);
- dev = i2c_try_create_slave(TYPE_PCA9552, 0x60);
+ dev = DEVICE(i2c_slave_new(TYPE_PCA9552, 0x60));
qdev_prop_set_string(dev, "description", "pca0");
i2c_realize_and_unref(dev, aspeed_i2c_get_bus(&soc->i2c, 11),
&error_fatal);
diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index acf34a12d6..6eacb4a463 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -267,13 +267,13 @@ const VMStateDescription vmstate_i2c_slave = {
}
};
-DeviceState *i2c_try_create_slave(const char *name, uint8_t addr)
+I2CSlave *i2c_slave_new(const char *name, uint8_t addr)
{
DeviceState *dev;
dev = qdev_new(name);
qdev_prop_set_uint8(dev, "address", addr);
- return dev;
+ return I2C_SLAVE(dev);
}
bool i2c_realize_and_unref(DeviceState *dev, I2CBus *bus, Error **errp)
@@ -283,10 +283,9 @@ bool i2c_realize_and_unref(DeviceState *dev, I2CBus *bus, Error **errp)
DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr)
{
- DeviceState *dev;
+ DeviceState *dev = DEVICE(i2c_slave_new(name, addr));
- dev = i2c_try_create_slave(name, addr);
- i2c_realize_and_unref(dev, bus, &error_fatal);
+ i2c_realize_and_unref(I2C_SLAVE(dev), bus, &error_fatal);
return dev;
}
--
2.21.3
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 3/5] hw/i2c: Rename i2c_realize_and_unref() as i2c_slave_realize_and_unref()
2020-06-29 17:38 [PATCH 0/5] hw/i2c: Rename method names for consistency and add documentation Philippe Mathieu-Daudé
2020-06-29 17:38 ` [PATCH 1/5] hw/i2c/aspeed_i2c: Simplify aspeed_i2c_get_bus() Philippe Mathieu-Daudé
2020-06-29 17:38 ` [PATCH 2/5] hw/i2c: Rename i2c_try_create_slave() as i2c_slave_new() Philippe Mathieu-Daudé
@ 2020-06-29 17:38 ` Philippe Mathieu-Daudé
2020-06-29 21:29 ` Corey Minyard
2020-06-29 17:38 ` [PATCH 4/5] hw/i2c: Rename i2c_create_slave() as i2c_slave_create_simple() Philippe Mathieu-Daudé
` (2 subsequent siblings)
5 siblings, 1 reply; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-29 17:38 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Philippe Mathieu-Daudé,
Corey Minyard, Andrew Jeffery, Markus Armbruster,
Cédric Le Goater, qemu-arm, qemu-ppc, Joel Stanley,
Jan Kiszka, David Gibson
The other i2c functions are called i2c_slave_FOO(). Rename as
i2c_slave_realize_and_unref() to be consistent.
Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
include/hw/i2c/i2c.h | 2 +-
hw/arm/aspeed.c | 10 ++++++----
hw/i2c/core.c | 6 +++---
3 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index 18efc668f1..cb7211f027 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -81,7 +81,7 @@ uint8_t i2c_recv(I2CBus *bus);
I2CSlave *i2c_slave_new(const char *name, uint8_t addr);
DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr);
-bool i2c_realize_and_unref(DeviceState *dev, I2CBus *bus, Error **errp);
+bool i2c_slave_realize_and_unref(I2CSlave *dev, I2CBus *bus, Error **errp);
/* lm832x.c */
void lm832x_key_event(DeviceState *dev, int key, int state);
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 54ca36e0b6..ed14e79f57 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -515,8 +515,9 @@ static void witherspoon_bmc_i2c_init(AspeedMachineState *bmc)
/* Bus 3: TODO dps310@76 */
dev = DEVICE(i2c_slave_new(TYPE_PCA9552, 0x60));
qdev_prop_set_string(dev, "description", "pca1");
- i2c_realize_and_unref(dev, aspeed_i2c_get_bus(&soc->i2c, 3),
- &error_fatal);
+ i2c_slave_realize_and_unref(I2C_SLAVE(dev),
+ aspeed_i2c_get_bus(&soc->i2c, 3),
+ &error_fatal);
i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 4), "tmp423", 0x4c);
i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 5), "tmp423", 0x4c);
@@ -533,8 +534,9 @@ static void witherspoon_bmc_i2c_init(AspeedMachineState *bmc)
eeprom_buf);
dev = DEVICE(i2c_slave_new(TYPE_PCA9552, 0x60));
qdev_prop_set_string(dev, "description", "pca0");
- i2c_realize_and_unref(dev, aspeed_i2c_get_bus(&soc->i2c, 11),
- &error_fatal);
+ i2c_slave_realize_and_unref(I2C_SLAVE(dev),
+ aspeed_i2c_get_bus(&soc->i2c, 11),
+ &error_fatal);
/* Bus 11: TODO ucd90160@64 */
}
diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index 6eacb4a463..135ea56036 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -276,16 +276,16 @@ I2CSlave *i2c_slave_new(const char *name, uint8_t addr)
return I2C_SLAVE(dev);
}
-bool i2c_realize_and_unref(DeviceState *dev, I2CBus *bus, Error **errp)
+bool i2c_slave_realize_and_unref(I2CSlave *dev, I2CBus *bus, Error **errp)
{
- return qdev_realize_and_unref(dev, &bus->qbus, errp);
+ return qdev_realize_and_unref(&dev->qdev, &bus->qbus, errp);
}
DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr)
{
DeviceState *dev = DEVICE(i2c_slave_new(name, addr));
- i2c_realize_and_unref(I2C_SLAVE(dev), bus, &error_fatal);
+ i2c_slave_realize_and_unref(I2C_SLAVE(dev), bus, &error_fatal);
return dev;
}
--
2.21.3
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 4/5] hw/i2c: Rename i2c_create_slave() as i2c_slave_create_simple()
2020-06-29 17:38 [PATCH 0/5] hw/i2c: Rename method names for consistency and add documentation Philippe Mathieu-Daudé
` (2 preceding siblings ...)
2020-06-29 17:38 ` [PATCH 3/5] hw/i2c: Rename i2c_realize_and_unref() as i2c_slave_realize_and_unref() Philippe Mathieu-Daudé
@ 2020-06-29 17:38 ` Philippe Mathieu-Daudé
2020-06-29 21:29 ` Corey Minyard
2020-06-30 9:48 ` Markus Armbruster
2020-06-29 17:38 ` [PATCH 5/5] hw/i2c: Document the I2C qdev helpers Philippe Mathieu-Daudé
2020-06-29 21:28 ` [PATCH 0/5] hw/i2c: Rename method names for consistency and add documentation Corey Minyard
5 siblings, 2 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-29 17:38 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Philippe Mathieu-Daudé,
Corey Minyard, Andrew Jeffery, Markus Armbruster,
Cédric Le Goater, qemu-arm, qemu-ppc, Joel Stanley,
Jan Kiszka, David Gibson
We use "create_simple" names for functions that allocate, initialize,
configure and realize device objects: pci_create_simple(),
isa_create_simple(), usb_create_simple(). For consistency, rename
i2c_create_slave() as i2c_slave_create_simple(). Since we have
to update all the callers, also let it return a I2CSlave object.
Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
include/hw/i2c/i2c.h | 2 +-
hw/arm/aspeed.c | 58 ++++++++++++++++++++++----------------------
hw/arm/musicpal.c | 4 +--
hw/arm/nseries.c | 8 +++---
hw/arm/pxa2xx.c | 5 ++--
hw/arm/realview.c | 2 +-
hw/arm/spitz.c | 4 +--
hw/arm/stellaris.c | 2 +-
hw/arm/tosa.c | 2 +-
hw/arm/versatilepb.c | 2 +-
hw/arm/vexpress.c | 2 +-
hw/arm/z2.c | 4 +--
hw/display/sii9022.c | 2 +-
hw/i2c/core.c | 6 ++---
hw/ppc/e500.c | 2 +-
hw/ppc/sam460ex.c | 2 +-
16 files changed, 54 insertions(+), 53 deletions(-)
diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index cb7211f027..c533058998 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -80,7 +80,7 @@ int i2c_send(I2CBus *bus, uint8_t data);
uint8_t i2c_recv(I2CBus *bus);
I2CSlave *i2c_slave_new(const char *name, uint8_t addr);
-DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr);
+I2CSlave *i2c_slave_create_simple(I2CBus *bus, const char *name, uint8_t addr);
bool i2c_slave_realize_and_unref(I2CSlave *dev, I2CBus *bus, Error **errp);
/* lm832x.c */
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index ed14e79f57..5fa95f0f02 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -385,14 +385,14 @@ static void palmetto_bmc_i2c_init(AspeedMachineState *bmc)
/* The palmetto platform expects a ds3231 RTC but a ds1338 is
* enough to provide basic RTC features. Alarms will be missing */
- i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 0), "ds1338", 0x68);
+ i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 0), "ds1338", 0x68);
smbus_eeprom_init_one(aspeed_i2c_get_bus(&soc->i2c, 0), 0x50,
eeprom_buf);
/* add a TMP423 temperature sensor */
- dev = i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 2),
- "tmp423", 0x4c);
+ dev = DEVICE(i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 2),
+ "tmp423", 0x4c));
object_property_set_int(OBJECT(dev), 31000, "temperature0", &error_abort);
object_property_set_int(OBJECT(dev), 28000, "temperature1", &error_abort);
object_property_set_int(OBJECT(dev), 20000, "temperature2", &error_abort);
@@ -408,12 +408,12 @@ static void ast2500_evb_i2c_init(AspeedMachineState *bmc)
eeprom_buf);
/* The AST2500 EVB expects a LM75 but a TMP105 is compatible */
- i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 7),
+ i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 7),
TYPE_TMP105, 0x4d);
/* The AST2500 EVB does not have an RTC. Let's pretend that one is
* plugged on the I2C bus header */
- i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 11), "ds1338", 0x32);
+ i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 11), "ds1338", 0x32);
}
static void ast2600_evb_i2c_init(AspeedMachineState *bmc)
@@ -428,36 +428,36 @@ static void romulus_bmc_i2c_init(AspeedMachineState *bmc)
/* The romulus board expects Epson RX8900 I2C RTC but a ds1338 is
* good enough */
- i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 11), "ds1338", 0x32);
+ i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 11), "ds1338", 0x32);
}
static void swift_bmc_i2c_init(AspeedMachineState *bmc)
{
AspeedSoCState *soc = &bmc->soc;
- i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 3), "pca9552", 0x60);
+ i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 3), "pca9552", 0x60);
/* The swift board expects a TMP275 but a TMP105 is compatible */
- i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 7), "tmp105", 0x48);
+ i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 7), "tmp105", 0x48);
/* The swift board expects a pca9551 but a pca9552 is compatible */
- i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 7), "pca9552", 0x60);
+ i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 7), "pca9552", 0x60);
/* The swift board expects an Epson RX8900 RTC but a ds1338 is compatible */
- i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 8), "ds1338", 0x32);
- i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 8), "pca9552", 0x60);
+ i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 8), "ds1338", 0x32);
+ i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 8), "pca9552", 0x60);
- i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 9), "tmp423", 0x4c);
+ i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 9), "tmp423", 0x4c);
/* The swift board expects a pca9539 but a pca9552 is compatible */
- i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 9), "pca9552", 0x74);
+ i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 9), "pca9552", 0x74);
- i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 10), "tmp423", 0x4c);
+ i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 10), "tmp423", 0x4c);
/* The swift board expects a pca9539 but a pca9552 is compatible */
- i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 10), "pca9552",
+ i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 10), "pca9552",
0x74);
/* The swift board expects a TMP275 but a TMP105 is compatible */
- i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 12), "tmp105", 0x48);
- i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 12), "tmp105", 0x4a);
+ i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 12), "tmp105", 0x48);
+ i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 12), "tmp105", 0x4a);
}
static void sonorapass_bmc_i2c_init(AspeedMachineState *bmc)
@@ -465,8 +465,8 @@ static void sonorapass_bmc_i2c_init(AspeedMachineState *bmc)
AspeedSoCState *soc = &bmc->soc;
/* bus 2 : */
- i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 2), "tmp105", 0x48);
- i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 2), "tmp105", 0x49);
+ i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 2), "tmp105", 0x48);
+ i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 2), "tmp105", 0x49);
/* bus 2 : pca9546 @ 0x73 */
/* bus 3 : pca9548 @ 0x70 */
@@ -476,21 +476,21 @@ static void sonorapass_bmc_i2c_init(AspeedMachineState *bmc)
smbus_eeprom_init_one(aspeed_i2c_get_bus(&soc->i2c, 4), 0x54,
eeprom4_54);
/* PCA9539 @ 0x76, but PCA9552 is compatible */
- i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 4), "pca9552", 0x76);
+ i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 4), "pca9552", 0x76);
/* PCA9539 @ 0x77, but PCA9552 is compatible */
- i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 4), "pca9552", 0x77);
+ i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 4), "pca9552", 0x77);
/* bus 6 : */
- i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 6), "tmp105", 0x48);
- i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 6), "tmp105", 0x49);
+ i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 6), "tmp105", 0x48);
+ i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 6), "tmp105", 0x49);
/* bus 6 : pca9546 @ 0x73 */
/* bus 8 : */
uint8_t *eeprom8_56 = g_malloc0(8 * 1024);
smbus_eeprom_init_one(aspeed_i2c_get_bus(&soc->i2c, 8), 0x56,
eeprom8_56);
- i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 8), "pca9552", 0x60);
- i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 8), "pca9552", 0x61);
+ i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 8), "pca9552", 0x60);
+ i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 8), "pca9552", 0x61);
/* bus 8 : adc128d818 @ 0x1d */
/* bus 8 : adc128d818 @ 0x1f */
@@ -519,16 +519,16 @@ static void witherspoon_bmc_i2c_init(AspeedMachineState *bmc)
aspeed_i2c_get_bus(&soc->i2c, 3),
&error_fatal);
- i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 4), "tmp423", 0x4c);
- i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 5), "tmp423", 0x4c);
+ i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 4), "tmp423", 0x4c);
+ i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 5), "tmp423", 0x4c);
/* The Witherspoon expects a TMP275 but a TMP105 is compatible */
- i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 9), TYPE_TMP105,
+ i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 9), TYPE_TMP105,
0x4a);
/* The witherspoon board expects Epson RX8900 I2C RTC but a ds1338 is
* good enough */
- i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 11), "ds1338", 0x32);
+ i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 11), "ds1338", 0x32);
smbus_eeprom_init_one(aspeed_i2c_get_bus(&soc->i2c, 11), 0x51,
eeprom_buf);
diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
index 394a3345bd..bf7bd28b94 100644
--- a/hw/arm/musicpal.c
+++ b/hw/arm/musicpal.c
@@ -1584,7 +1584,7 @@ static void musicpal_init(MachineState *machine)
DeviceState *i2c_dev;
DeviceState *lcd_dev;
DeviceState *key_dev;
- DeviceState *wm8750_dev;
+ I2CSlave *wm8750_dev;
SysBusDevice *s;
I2CBus *i2c;
int i;
@@ -1687,7 +1687,7 @@ static void musicpal_init(MachineState *machine)
qdev_connect_gpio_out(key_dev, i, qdev_get_gpio_in(dev, i + 15));
}
- wm8750_dev = i2c_create_slave(i2c, TYPE_WM8750, MP_WM_ADDR);
+ wm8750_dev = i2c_slave_create_simple(i2c, TYPE_WM8750, MP_WM_ADDR);
dev = qdev_new(TYPE_MV88W8618_AUDIO);
s = SYS_BUS_DEVICE(dev);
object_property_set_link(OBJECT(dev), OBJECT(wm8750_dev),
diff --git a/hw/arm/nseries.c b/hw/arm/nseries.c
index 428a2a2c5a..e48092ca04 100644
--- a/hw/arm/nseries.c
+++ b/hw/arm/nseries.c
@@ -215,7 +215,7 @@ static void n8x0_i2c_setup(struct n800_s *s)
I2CBus *i2c = omap_i2c_bus(s->mpu->i2c[0]);
/* Attach a menelaus PM chip */
- dev = i2c_create_slave(i2c, "twl92230", N8X0_MENELAUS_ADDR);
+ dev = DEVICE(i2c_slave_create_simple(i2c, "twl92230", N8X0_MENELAUS_ADDR));
qdev_connect_gpio_out(dev, 3,
qdev_get_gpio_in(s->mpu->ih[0],
OMAP_INT_24XX_SYS_NIRQ));
@@ -224,7 +224,7 @@ static void n8x0_i2c_setup(struct n800_s *s)
qemu_register_powerdown_notifier(&n8x0_system_powerdown_notifier);
/* Attach a TMP105 PM chip (A0 wired to ground) */
- dev = i2c_create_slave(i2c, TYPE_TMP105, N8X0_TMP105_ADDR);
+ dev = DEVICE(i2c_slave_create_simple(i2c, TYPE_TMP105, N8X0_TMP105_ADDR));
qdev_connect_gpio_out(dev, 0, tmp_irq);
}
@@ -416,8 +416,8 @@ static void n810_kbd_setup(struct n800_s *s)
/* Attach the LM8322 keyboard to the I2C bus,
* should happen in n8x0_i2c_setup and s->kbd be initialised here. */
- s->kbd = i2c_create_slave(omap_i2c_bus(s->mpu->i2c[0]),
- "lm8323", N810_LM8323_ADDR);
+ s->kbd = DEVICE(i2c_slave_create_simple(omap_i2c_bus(s->mpu->i2c[0]),
+ "lm8323", N810_LM8323_ADDR));
qdev_connect_gpio_out(s->kbd, 0, kbd_irq);
}
diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
index f104a33463..6203c4cfe0 100644
--- a/hw/arm/pxa2xx.c
+++ b/hw/arm/pxa2xx.c
@@ -1522,8 +1522,9 @@ PXA2xxI2CState *pxa2xx_i2c_init(hwaddr base,
s = PXA2XX_I2C(i2c_dev);
/* FIXME: Should the slave device really be on a separate bus? */
i2cbus = i2c_init_bus(dev, "dummy");
- dev = i2c_create_slave(i2cbus, TYPE_PXA2XX_I2C_SLAVE, 0);
- s->slave = PXA2XX_I2C_SLAVE(dev);
+ s->slave = PXA2XX_I2C_SLAVE(i2c_slave_create_simple(i2cbus,
+ TYPE_PXA2XX_I2C_SLAVE,
+ 0));
s->slave->host = s;
return s;
diff --git a/hw/arm/realview.c b/hw/arm/realview.c
index b6c0a1adb9..e105b6509f 100644
--- a/hw/arm/realview.c
+++ b/hw/arm/realview.c
@@ -285,7 +285,7 @@ static void realview_init(MachineState *machine,
dev = sysbus_create_simple(TYPE_VERSATILE_I2C, 0x10002000, NULL);
i2c = (I2CBus *)qdev_get_child_bus(dev, "i2c");
- i2c_create_slave(i2c, "ds1338", 0x68);
+ i2c_slave_create_simple(i2c, "ds1338", 0x68);
/* Memory map for RealView Emulation Baseboard: */
/* 0x10000000 System registers. */
diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
index fc18212e68..716ca3c7b6 100644
--- a/hw/arm/spitz.c
+++ b/hw/arm/spitz.c
@@ -758,7 +758,7 @@ static void spitz_i2c_setup(PXA2xxState *cpu)
DeviceState *wm;
/* Attach a WM8750 to the bus */
- wm = i2c_create_slave(bus, TYPE_WM8750, 0);
+ wm = DEVICE(i2c_slave_create_simple(bus, TYPE_WM8750, 0));
spitz_wm8750_addr(wm, 0, 0);
qdev_connect_gpio_out(cpu->gpio, SPITZ_GPIO_WM,
@@ -773,7 +773,7 @@ static void spitz_i2c_setup(PXA2xxState *cpu)
static void spitz_akita_i2c_setup(PXA2xxState *cpu)
{
/* Attach a Max7310 to Akita I2C bus. */
- i2c_create_slave(pxa2xx_i2c_bus(cpu->i2c[0]), "max7310",
+ i2c_slave_create_simple(pxa2xx_i2c_bus(cpu->i2c[0]), "max7310",
AKITA_MAX_ADDR);
}
diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
index 97ef566c12..3f45550cf6 100644
--- a/hw/arm/stellaris.c
+++ b/hw/arm/stellaris.c
@@ -1380,7 +1380,7 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
qdev_get_gpio_in(nvic, 8));
i2c = (I2CBus *)qdev_get_child_bus(dev, "i2c");
if (board->peripherals & BP_OLED_I2C) {
- i2c_create_slave(i2c, "ssd0303", 0x3d);
+ i2c_slave_create_simple(i2c, "ssd0303", 0x3d);
}
}
diff --git a/hw/arm/tosa.c b/hw/arm/tosa.c
index 5dee2d76c6..8c1ee0cdd1 100644
--- a/hw/arm/tosa.c
+++ b/hw/arm/tosa.c
@@ -206,7 +206,7 @@ static uint8_t tosa_dac_recv(I2CSlave *s)
static void tosa_tg_init(PXA2xxState *cpu)
{
I2CBus *bus = pxa2xx_i2c_bus(cpu->i2c[0]);
- i2c_create_slave(bus, TYPE_TOSA_DAC, DAC_BASE);
+ i2c_slave_create_simple(bus, TYPE_TOSA_DAC, DAC_BASE);
ssi_create_slave(cpu->ssp[1], "tosa-ssp");
}
diff --git a/hw/arm/versatilepb.c b/hw/arm/versatilepb.c
index e596b8170f..34709afb32 100644
--- a/hw/arm/versatilepb.c
+++ b/hw/arm/versatilepb.c
@@ -317,7 +317,7 @@ static void versatile_init(MachineState *machine, int board_id)
dev = sysbus_create_simple(TYPE_VERSATILE_I2C, 0x10002000, NULL);
i2c = (I2CBus *)qdev_get_child_bus(dev, "i2c");
- i2c_create_slave(i2c, "ds1338", 0x68);
+ i2c_slave_create_simple(i2c, "ds1338", 0x68);
/* Add PL041 AACI Interface to the LM4549 codec */
pl041 = qdev_new("pl041");
diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index 5bf9cff8a8..4f6a2b6ddd 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -642,7 +642,7 @@ static void vexpress_common_init(MachineState *machine)
dev = sysbus_create_simple(TYPE_VERSATILE_I2C, map[VE_SERIALDVI], NULL);
i2c = (I2CBus *)qdev_get_child_bus(dev, "i2c");
- i2c_create_slave(i2c, "sii9022", 0x39);
+ i2c_slave_create_simple(i2c, "sii9022", 0x39);
sysbus_create_simple("pl031", map[VE_RTC], pic[4]); /* RTC */
diff --git a/hw/arm/z2.c b/hw/arm/z2.c
index a0f4095990..8cf8189f6f 100644
--- a/hw/arm/z2.c
+++ b/hw/arm/z2.c
@@ -327,8 +327,8 @@ static void z2_init(MachineState *machine)
type_register_static(&aer915_info);
z2_lcd = ssi_create_slave(mpu->ssp[1], "zipit-lcd");
bus = pxa2xx_i2c_bus(mpu->i2c[0]);
- i2c_create_slave(bus, TYPE_AER915, 0x55);
- wm = i2c_create_slave(bus, TYPE_WM8750, 0x1b);
+ i2c_slave_create_simple(bus, TYPE_AER915, 0x55);
+ wm = DEVICE(i2c_slave_create_simple(bus, TYPE_WM8750, 0x1b));
mpu->i2s->opaque = wm;
mpu->i2s->codec_out = wm8750_dac_dat;
mpu->i2s->codec_in = wm8750_adc_dat;
diff --git a/hw/display/sii9022.c b/hw/display/sii9022.c
index 0710ce9de5..3b82a8567f 100644
--- a/hw/display/sii9022.c
+++ b/hw/display/sii9022.c
@@ -161,7 +161,7 @@ static void sii9022_realize(DeviceState *dev, Error **errp)
I2CBus *bus;
bus = I2C_BUS(qdev_get_parent_bus(dev));
- i2c_create_slave(bus, TYPE_I2CDDC, 0x50);
+ i2c_slave_create_simple(bus, TYPE_I2CDDC, 0x50);
}
static void sii9022_class_init(ObjectClass *klass, void *data)
diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index 135ea56036..21ec52ac5a 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -281,11 +281,11 @@ bool i2c_slave_realize_and_unref(I2CSlave *dev, I2CBus *bus, Error **errp)
return qdev_realize_and_unref(&dev->qdev, &bus->qbus, errp);
}
-DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr)
+I2CSlave *i2c_slave_create_simple(I2CBus *bus, const char *name, uint8_t addr)
{
- DeviceState *dev = DEVICE(i2c_slave_new(name, addr));
+ I2CSlave *dev = i2c_slave_new(name, addr);
- i2c_slave_realize_and_unref(I2C_SLAVE(dev), bus, &error_fatal);
+ i2c_slave_realize_and_unref(dev, bus, &error_abort);
return dev;
}
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 51bf95b303..67924915ae 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -944,7 +944,7 @@ void ppce500_init(MachineState *machine)
memory_region_add_subregion(ccsr_addr_space, MPC8544_I2C_REGS_OFFSET,
sysbus_mmio_get_region(s, 0));
i2c = (I2CBus *)qdev_get_child_bus(dev, "i2c");
- i2c_create_slave(i2c, "ds1338", RTC_REGS_OFFSET);
+ i2c_slave_create_simple(i2c, "ds1338", RTC_REGS_OFFSET);
/* General Utility device */
diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index 1a106a68de..1702344c46 100644
--- a/hw/ppc/sam460ex.c
+++ b/hw/ppc/sam460ex.c
@@ -339,7 +339,7 @@ static void sam460ex_init(MachineState *machine)
spd_data[20] = 4; /* SO-DIMM module */
smbus_eeprom_init_one(i2c, 0x50, spd_data);
/* RTC */
- i2c_create_slave(i2c, "m41t80", 0x68);
+ i2c_slave_create_simple(i2c, "m41t80", 0x68);
dev = sysbus_create_simple(TYPE_PPC4xx_I2C, 0x4ef600800, uic[0][3]);
--
2.21.3
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 5/5] hw/i2c: Document the I2C qdev helpers
2020-06-29 17:38 [PATCH 0/5] hw/i2c: Rename method names for consistency and add documentation Philippe Mathieu-Daudé
` (3 preceding siblings ...)
2020-06-29 17:38 ` [PATCH 4/5] hw/i2c: Rename i2c_create_slave() as i2c_slave_create_simple() Philippe Mathieu-Daudé
@ 2020-06-29 17:38 ` Philippe Mathieu-Daudé
2020-06-29 21:30 ` Corey Minyard
2020-06-30 10:15 ` Markus Armbruster
2020-06-29 21:28 ` [PATCH 0/5] hw/i2c: Rename method names for consistency and add documentation Corey Minyard
5 siblings, 2 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-29 17:38 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Philippe Mathieu-Daudé,
Corey Minyard, Andrew Jeffery, Markus Armbruster,
Cédric Le Goater, qemu-arm, qemu-ppc, Joel Stanley,
Jan Kiszka, David Gibson
In commit d88c42ff2c we added new prototype but neglected to
add their documentation. Fix that.
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
include/hw/i2c/i2c.h | 48 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 48 insertions(+)
diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index c533058998..fcc61e509b 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -79,8 +79,56 @@ int i2c_send_recv(I2CBus *bus, uint8_t *data, bool send);
int i2c_send(I2CBus *bus, uint8_t data);
uint8_t i2c_recv(I2CBus *bus);
+/**
+ * Create an I2C slave device on the heap.
+ * @name: a device type name
+ * @addr: I2C address of the slave when put on a bus
+ *
+ * This only initializes the device state structure and allows
+ * properties to be set. Type @name must exist. The device still
+ * needs to be realized. See qdev-core.h.
+ */
I2CSlave *i2c_slave_new(const char *name, uint8_t addr);
+
+/**
+ * Create an I2C slave device on the heap.
+ * @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.
+ */
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
+ * @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
+ *
+ * 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.
+ *
+ * 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.)
+ */
bool i2c_slave_realize_and_unref(I2CSlave *dev, I2CBus *bus, Error **errp);
/* lm832x.c */
--
2.21.3
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 0/5] hw/i2c: Rename method names for consistency and add documentation
2020-06-29 17:38 [PATCH 0/5] hw/i2c: Rename method names for consistency and add documentation Philippe Mathieu-Daudé
` (4 preceding siblings ...)
2020-06-29 17:38 ` [PATCH 5/5] hw/i2c: Document the I2C qdev helpers Philippe Mathieu-Daudé
@ 2020-06-29 21:28 ` Corey Minyard
5 siblings, 0 replies; 22+ messages in thread
From: Corey Minyard @ 2020-06-29 21:28 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Corey Minyard, Andrew Jeffery, qemu-devel,
Markus Armbruster, qemu-arm, qemu-ppc, Cédric Le Goater,
Jan Kiszka, David Gibson, Joel Stanley
On Mon, Jun 29, 2020 at 07:38:16PM +0200, Philippe Mathieu-Daudé wrote:
> In commit d88c42ff2c we added 2 methods: i2c_try_create_slave()
> and i2c_realize_and_unref().
> Markus noted their name could be improved for consistency [1],
> and Peter reported the lack of documentation [2]. Fix that now.
Looking over these, I don't see an issue. I didn't review the aspeed
device changes (patch 1); that's probably better for the aspeed
maintainer to review.
But I do like the improvement in consistency.
-corey
>
> [1] https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg07060.html
> [2] https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg08997.html
>
> Philippe Mathieu-Daudé (5):
> hw/i2c/aspeed_i2c: Simplify aspeed_i2c_get_bus()
> hw/i2c: Rename i2c_try_create_slave() as i2c_slave_new()
> hw/i2c: Rename i2c_realize_and_unref() as
> i2c_slave_realize_and_unref()
> hw/i2c: Rename i2c_create_slave() as i2c_slave_create_simple()
> hw/i2c: Document the I2C qdev helpers
>
> include/hw/i2c/aspeed_i2c.h | 2 +-
> include/hw/i2c/i2c.h | 54 ++++++++++++++++++++++--
> hw/arm/aspeed.c | 82 +++++++++++++++++++------------------
> hw/arm/musicpal.c | 4 +-
> hw/arm/nseries.c | 8 ++--
> hw/arm/pxa2xx.c | 5 ++-
> hw/arm/realview.c | 2 +-
> hw/arm/spitz.c | 4 +-
> hw/arm/stellaris.c | 2 +-
> hw/arm/tosa.c | 2 +-
> hw/arm/versatilepb.c | 2 +-
> hw/arm/vexpress.c | 2 +-
> hw/arm/z2.c | 4 +-
> hw/display/sii9022.c | 2 +-
> hw/i2c/aspeed_i2c.c | 3 +-
> hw/i2c/core.c | 15 ++++---
> hw/ppc/e500.c | 2 +-
> hw/ppc/sam460ex.c | 2 +-
> 18 files changed, 123 insertions(+), 74 deletions(-)
>
> --
> 2.21.3
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/5] hw/i2c: Rename i2c_try_create_slave() as i2c_slave_new()
2020-06-29 17:38 ` [PATCH 2/5] hw/i2c: Rename i2c_try_create_slave() as i2c_slave_new() Philippe Mathieu-Daudé
@ 2020-06-29 21:29 ` Corey Minyard
2020-06-29 21:37 ` BALATON Zoltan
1 sibling, 0 replies; 22+ messages in thread
From: Corey Minyard @ 2020-06-29 21:29 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Corey Minyard, Andrew Jeffery, qemu-devel,
Markus Armbruster, qemu-arm, qemu-ppc, Cédric Le Goater,
Jan Kiszka, David Gibson, Joel Stanley
On Mon, Jun 29, 2020 at 07:38:18PM +0200, Philippe Mathieu-Daudé wrote:
> 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(). Since we have to update
> all the callers, also let it return a I2CSlave object.
Reviewed-by: Corey Minyard <cminyard@mvista.com>
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> include/hw/i2c/i2c.h | 2 +-
> hw/arm/aspeed.c | 4 ++--
> hw/i2c/core.c | 9 ++++-----
> 3 files changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
> index d6e3d85faf..18efc668f1 100644
> --- a/include/hw/i2c/i2c.h
> +++ b/include/hw/i2c/i2c.h
> @@ -79,8 +79,8 @@ 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);
>
> +I2CSlave *i2c_slave_new(const char *name, uint8_t addr);
> 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 */
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 1285bf82c0..54ca36e0b6 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -513,7 +513,7 @@ static void witherspoon_bmc_i2c_init(AspeedMachineState *bmc)
> /* Bus 3: TODO bmp280@77 */
> /* Bus 3: TODO max31785@52 */
> /* Bus 3: TODO dps310@76 */
> - dev = i2c_try_create_slave(TYPE_PCA9552, 0x60);
> + dev = DEVICE(i2c_slave_new(TYPE_PCA9552, 0x60));
> qdev_prop_set_string(dev, "description", "pca1");
> i2c_realize_and_unref(dev, aspeed_i2c_get_bus(&soc->i2c, 3),
> &error_fatal);
> @@ -531,7 +531,7 @@ static void witherspoon_bmc_i2c_init(AspeedMachineState *bmc)
>
> smbus_eeprom_init_one(aspeed_i2c_get_bus(&soc->i2c, 11), 0x51,
> eeprom_buf);
> - dev = i2c_try_create_slave(TYPE_PCA9552, 0x60);
> + dev = DEVICE(i2c_slave_new(TYPE_PCA9552, 0x60));
> qdev_prop_set_string(dev, "description", "pca0");
> i2c_realize_and_unref(dev, aspeed_i2c_get_bus(&soc->i2c, 11),
> &error_fatal);
> diff --git a/hw/i2c/core.c b/hw/i2c/core.c
> index acf34a12d6..6eacb4a463 100644
> --- a/hw/i2c/core.c
> +++ b/hw/i2c/core.c
> @@ -267,13 +267,13 @@ const VMStateDescription vmstate_i2c_slave = {
> }
> };
>
> -DeviceState *i2c_try_create_slave(const char *name, uint8_t addr)
> +I2CSlave *i2c_slave_new(const char *name, uint8_t addr)
> {
> DeviceState *dev;
>
> dev = qdev_new(name);
> qdev_prop_set_uint8(dev, "address", addr);
> - return dev;
> + return I2C_SLAVE(dev);
> }
>
> bool i2c_realize_and_unref(DeviceState *dev, I2CBus *bus, Error **errp)
> @@ -283,10 +283,9 @@ bool i2c_realize_and_unref(DeviceState *dev, I2CBus *bus, Error **errp)
>
> DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr)
> {
> - DeviceState *dev;
> + DeviceState *dev = DEVICE(i2c_slave_new(name, addr));
>
> - dev = i2c_try_create_slave(name, addr);
> - i2c_realize_and_unref(dev, bus, &error_fatal);
> + i2c_realize_and_unref(I2C_SLAVE(dev), bus, &error_fatal);
>
> return dev;
> }
> --
> 2.21.3
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/5] hw/i2c: Rename i2c_realize_and_unref() as i2c_slave_realize_and_unref()
2020-06-29 17:38 ` [PATCH 3/5] hw/i2c: Rename i2c_realize_and_unref() as i2c_slave_realize_and_unref() Philippe Mathieu-Daudé
@ 2020-06-29 21:29 ` Corey Minyard
0 siblings, 0 replies; 22+ messages in thread
From: Corey Minyard @ 2020-06-29 21:29 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Corey Minyard, Andrew Jeffery, qemu-devel,
Markus Armbruster, qemu-arm, qemu-ppc, Cédric Le Goater,
Jan Kiszka, David Gibson, Joel Stanley
On Mon, Jun 29, 2020 at 07:38:19PM +0200, Philippe Mathieu-Daudé wrote:
> The other i2c functions are called i2c_slave_FOO(). Rename as
> i2c_slave_realize_and_unref() to be consistent.
Reviewed-by: Corey Minyard <cminyard@mvista.com>
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> include/hw/i2c/i2c.h | 2 +-
> hw/arm/aspeed.c | 10 ++++++----
> hw/i2c/core.c | 6 +++---
> 3 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
> index 18efc668f1..cb7211f027 100644
> --- a/include/hw/i2c/i2c.h
> +++ b/include/hw/i2c/i2c.h
> @@ -81,7 +81,7 @@ uint8_t i2c_recv(I2CBus *bus);
>
> I2CSlave *i2c_slave_new(const char *name, uint8_t addr);
> DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr);
> -bool i2c_realize_and_unref(DeviceState *dev, I2CBus *bus, Error **errp);
> +bool i2c_slave_realize_and_unref(I2CSlave *dev, I2CBus *bus, Error **errp);
>
> /* lm832x.c */
> void lm832x_key_event(DeviceState *dev, int key, int state);
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 54ca36e0b6..ed14e79f57 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -515,8 +515,9 @@ static void witherspoon_bmc_i2c_init(AspeedMachineState *bmc)
> /* Bus 3: TODO dps310@76 */
> dev = DEVICE(i2c_slave_new(TYPE_PCA9552, 0x60));
> qdev_prop_set_string(dev, "description", "pca1");
> - i2c_realize_and_unref(dev, aspeed_i2c_get_bus(&soc->i2c, 3),
> - &error_fatal);
> + i2c_slave_realize_and_unref(I2C_SLAVE(dev),
> + aspeed_i2c_get_bus(&soc->i2c, 3),
> + &error_fatal);
>
> i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 4), "tmp423", 0x4c);
> i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 5), "tmp423", 0x4c);
> @@ -533,8 +534,9 @@ static void witherspoon_bmc_i2c_init(AspeedMachineState *bmc)
> eeprom_buf);
> dev = DEVICE(i2c_slave_new(TYPE_PCA9552, 0x60));
> qdev_prop_set_string(dev, "description", "pca0");
> - i2c_realize_and_unref(dev, aspeed_i2c_get_bus(&soc->i2c, 11),
> - &error_fatal);
> + i2c_slave_realize_and_unref(I2C_SLAVE(dev),
> + aspeed_i2c_get_bus(&soc->i2c, 11),
> + &error_fatal);
> /* Bus 11: TODO ucd90160@64 */
> }
>
> diff --git a/hw/i2c/core.c b/hw/i2c/core.c
> index 6eacb4a463..135ea56036 100644
> --- a/hw/i2c/core.c
> +++ b/hw/i2c/core.c
> @@ -276,16 +276,16 @@ I2CSlave *i2c_slave_new(const char *name, uint8_t addr)
> return I2C_SLAVE(dev);
> }
>
> -bool i2c_realize_and_unref(DeviceState *dev, I2CBus *bus, Error **errp)
> +bool i2c_slave_realize_and_unref(I2CSlave *dev, I2CBus *bus, Error **errp)
> {
> - return qdev_realize_and_unref(dev, &bus->qbus, errp);
> + return qdev_realize_and_unref(&dev->qdev, &bus->qbus, errp);
> }
>
> DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr)
> {
> DeviceState *dev = DEVICE(i2c_slave_new(name, addr));
>
> - i2c_realize_and_unref(I2C_SLAVE(dev), bus, &error_fatal);
> + i2c_slave_realize_and_unref(I2C_SLAVE(dev), bus, &error_fatal);
>
> return dev;
> }
> --
> 2.21.3
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/5] hw/i2c: Rename i2c_create_slave() as i2c_slave_create_simple()
2020-06-29 17:38 ` [PATCH 4/5] hw/i2c: Rename i2c_create_slave() as i2c_slave_create_simple() Philippe Mathieu-Daudé
@ 2020-06-29 21:29 ` Corey Minyard
2020-06-30 9:48 ` Markus Armbruster
1 sibling, 0 replies; 22+ messages in thread
From: Corey Minyard @ 2020-06-29 21:29 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Corey Minyard, Andrew Jeffery, qemu-devel,
Markus Armbruster, qemu-arm, qemu-ppc, Cédric Le Goater,
Jan Kiszka, David Gibson, Joel Stanley
On Mon, Jun 29, 2020 at 07:38:20PM +0200, Philippe Mathieu-Daudé wrote:
> We use "create_simple" names for functions that allocate, initialize,
> configure and realize device objects: pci_create_simple(),
> isa_create_simple(), usb_create_simple(). For consistency, rename
> i2c_create_slave() as i2c_slave_create_simple(). Since we have
> to update all the callers, also let it return a I2CSlave object.
Reviewed-by: Corey Minyard <cminyard@mvista.com>
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> include/hw/i2c/i2c.h | 2 +-
> hw/arm/aspeed.c | 58 ++++++++++++++++++++++----------------------
> hw/arm/musicpal.c | 4 +--
> hw/arm/nseries.c | 8 +++---
> hw/arm/pxa2xx.c | 5 ++--
> hw/arm/realview.c | 2 +-
> hw/arm/spitz.c | 4 +--
> hw/arm/stellaris.c | 2 +-
> hw/arm/tosa.c | 2 +-
> hw/arm/versatilepb.c | 2 +-
> hw/arm/vexpress.c | 2 +-
> hw/arm/z2.c | 4 +--
> hw/display/sii9022.c | 2 +-
> hw/i2c/core.c | 6 ++---
> hw/ppc/e500.c | 2 +-
> hw/ppc/sam460ex.c | 2 +-
> 16 files changed, 54 insertions(+), 53 deletions(-)
>
> diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
> index cb7211f027..c533058998 100644
> --- a/include/hw/i2c/i2c.h
> +++ b/include/hw/i2c/i2c.h
> @@ -80,7 +80,7 @@ int i2c_send(I2CBus *bus, uint8_t data);
> uint8_t i2c_recv(I2CBus *bus);
>
> I2CSlave *i2c_slave_new(const char *name, uint8_t addr);
> -DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr);
> +I2CSlave *i2c_slave_create_simple(I2CBus *bus, const char *name, uint8_t addr);
> bool i2c_slave_realize_and_unref(I2CSlave *dev, I2CBus *bus, Error **errp);
>
> /* lm832x.c */
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index ed14e79f57..5fa95f0f02 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -385,14 +385,14 @@ static void palmetto_bmc_i2c_init(AspeedMachineState *bmc)
>
> /* The palmetto platform expects a ds3231 RTC but a ds1338 is
> * enough to provide basic RTC features. Alarms will be missing */
> - i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 0), "ds1338", 0x68);
> + i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 0), "ds1338", 0x68);
>
> smbus_eeprom_init_one(aspeed_i2c_get_bus(&soc->i2c, 0), 0x50,
> eeprom_buf);
>
> /* add a TMP423 temperature sensor */
> - dev = i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 2),
> - "tmp423", 0x4c);
> + dev = DEVICE(i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 2),
> + "tmp423", 0x4c));
> object_property_set_int(OBJECT(dev), 31000, "temperature0", &error_abort);
> object_property_set_int(OBJECT(dev), 28000, "temperature1", &error_abort);
> object_property_set_int(OBJECT(dev), 20000, "temperature2", &error_abort);
> @@ -408,12 +408,12 @@ static void ast2500_evb_i2c_init(AspeedMachineState *bmc)
> eeprom_buf);
>
> /* The AST2500 EVB expects a LM75 but a TMP105 is compatible */
> - i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 7),
> + i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 7),
> TYPE_TMP105, 0x4d);
>
> /* The AST2500 EVB does not have an RTC. Let's pretend that one is
> * plugged on the I2C bus header */
> - i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 11), "ds1338", 0x32);
> + i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 11), "ds1338", 0x32);
> }
>
> static void ast2600_evb_i2c_init(AspeedMachineState *bmc)
> @@ -428,36 +428,36 @@ static void romulus_bmc_i2c_init(AspeedMachineState *bmc)
>
> /* The romulus board expects Epson RX8900 I2C RTC but a ds1338 is
> * good enough */
> - i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 11), "ds1338", 0x32);
> + i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 11), "ds1338", 0x32);
> }
>
> static void swift_bmc_i2c_init(AspeedMachineState *bmc)
> {
> AspeedSoCState *soc = &bmc->soc;
>
> - i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 3), "pca9552", 0x60);
> + i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 3), "pca9552", 0x60);
>
> /* The swift board expects a TMP275 but a TMP105 is compatible */
> - i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 7), "tmp105", 0x48);
> + i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 7), "tmp105", 0x48);
> /* The swift board expects a pca9551 but a pca9552 is compatible */
> - i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 7), "pca9552", 0x60);
> + i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 7), "pca9552", 0x60);
>
> /* The swift board expects an Epson RX8900 RTC but a ds1338 is compatible */
> - i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 8), "ds1338", 0x32);
> - i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 8), "pca9552", 0x60);
> + i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 8), "ds1338", 0x32);
> + i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 8), "pca9552", 0x60);
>
> - i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 9), "tmp423", 0x4c);
> + i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 9), "tmp423", 0x4c);
> /* The swift board expects a pca9539 but a pca9552 is compatible */
> - i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 9), "pca9552", 0x74);
> + i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 9), "pca9552", 0x74);
>
> - i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 10), "tmp423", 0x4c);
> + i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 10), "tmp423", 0x4c);
> /* The swift board expects a pca9539 but a pca9552 is compatible */
> - i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 10), "pca9552",
> + i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 10), "pca9552",
> 0x74);
>
> /* The swift board expects a TMP275 but a TMP105 is compatible */
> - i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 12), "tmp105", 0x48);
> - i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 12), "tmp105", 0x4a);
> + i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 12), "tmp105", 0x48);
> + i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 12), "tmp105", 0x4a);
> }
>
> static void sonorapass_bmc_i2c_init(AspeedMachineState *bmc)
> @@ -465,8 +465,8 @@ static void sonorapass_bmc_i2c_init(AspeedMachineState *bmc)
> AspeedSoCState *soc = &bmc->soc;
>
> /* bus 2 : */
> - i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 2), "tmp105", 0x48);
> - i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 2), "tmp105", 0x49);
> + i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 2), "tmp105", 0x48);
> + i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 2), "tmp105", 0x49);
> /* bus 2 : pca9546 @ 0x73 */
>
> /* bus 3 : pca9548 @ 0x70 */
> @@ -476,21 +476,21 @@ static void sonorapass_bmc_i2c_init(AspeedMachineState *bmc)
> smbus_eeprom_init_one(aspeed_i2c_get_bus(&soc->i2c, 4), 0x54,
> eeprom4_54);
> /* PCA9539 @ 0x76, but PCA9552 is compatible */
> - i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 4), "pca9552", 0x76);
> + i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 4), "pca9552", 0x76);
> /* PCA9539 @ 0x77, but PCA9552 is compatible */
> - i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 4), "pca9552", 0x77);
> + i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 4), "pca9552", 0x77);
>
> /* bus 6 : */
> - i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 6), "tmp105", 0x48);
> - i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 6), "tmp105", 0x49);
> + i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 6), "tmp105", 0x48);
> + i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 6), "tmp105", 0x49);
> /* bus 6 : pca9546 @ 0x73 */
>
> /* bus 8 : */
> uint8_t *eeprom8_56 = g_malloc0(8 * 1024);
> smbus_eeprom_init_one(aspeed_i2c_get_bus(&soc->i2c, 8), 0x56,
> eeprom8_56);
> - i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 8), "pca9552", 0x60);
> - i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 8), "pca9552", 0x61);
> + i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 8), "pca9552", 0x60);
> + i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 8), "pca9552", 0x61);
> /* bus 8 : adc128d818 @ 0x1d */
> /* bus 8 : adc128d818 @ 0x1f */
>
> @@ -519,16 +519,16 @@ static void witherspoon_bmc_i2c_init(AspeedMachineState *bmc)
> aspeed_i2c_get_bus(&soc->i2c, 3),
> &error_fatal);
>
> - i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 4), "tmp423", 0x4c);
> - i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 5), "tmp423", 0x4c);
> + i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 4), "tmp423", 0x4c);
> + i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 5), "tmp423", 0x4c);
>
> /* The Witherspoon expects a TMP275 but a TMP105 is compatible */
> - i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 9), TYPE_TMP105,
> + i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 9), TYPE_TMP105,
> 0x4a);
>
> /* The witherspoon board expects Epson RX8900 I2C RTC but a ds1338 is
> * good enough */
> - i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 11), "ds1338", 0x32);
> + i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 11), "ds1338", 0x32);
>
> smbus_eeprom_init_one(aspeed_i2c_get_bus(&soc->i2c, 11), 0x51,
> eeprom_buf);
> diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
> index 394a3345bd..bf7bd28b94 100644
> --- a/hw/arm/musicpal.c
> +++ b/hw/arm/musicpal.c
> @@ -1584,7 +1584,7 @@ static void musicpal_init(MachineState *machine)
> DeviceState *i2c_dev;
> DeviceState *lcd_dev;
> DeviceState *key_dev;
> - DeviceState *wm8750_dev;
> + I2CSlave *wm8750_dev;
> SysBusDevice *s;
> I2CBus *i2c;
> int i;
> @@ -1687,7 +1687,7 @@ static void musicpal_init(MachineState *machine)
> qdev_connect_gpio_out(key_dev, i, qdev_get_gpio_in(dev, i + 15));
> }
>
> - wm8750_dev = i2c_create_slave(i2c, TYPE_WM8750, MP_WM_ADDR);
> + wm8750_dev = i2c_slave_create_simple(i2c, TYPE_WM8750, MP_WM_ADDR);
> dev = qdev_new(TYPE_MV88W8618_AUDIO);
> s = SYS_BUS_DEVICE(dev);
> object_property_set_link(OBJECT(dev), OBJECT(wm8750_dev),
> diff --git a/hw/arm/nseries.c b/hw/arm/nseries.c
> index 428a2a2c5a..e48092ca04 100644
> --- a/hw/arm/nseries.c
> +++ b/hw/arm/nseries.c
> @@ -215,7 +215,7 @@ static void n8x0_i2c_setup(struct n800_s *s)
> I2CBus *i2c = omap_i2c_bus(s->mpu->i2c[0]);
>
> /* Attach a menelaus PM chip */
> - dev = i2c_create_slave(i2c, "twl92230", N8X0_MENELAUS_ADDR);
> + dev = DEVICE(i2c_slave_create_simple(i2c, "twl92230", N8X0_MENELAUS_ADDR));
> qdev_connect_gpio_out(dev, 3,
> qdev_get_gpio_in(s->mpu->ih[0],
> OMAP_INT_24XX_SYS_NIRQ));
> @@ -224,7 +224,7 @@ static void n8x0_i2c_setup(struct n800_s *s)
> qemu_register_powerdown_notifier(&n8x0_system_powerdown_notifier);
>
> /* Attach a TMP105 PM chip (A0 wired to ground) */
> - dev = i2c_create_slave(i2c, TYPE_TMP105, N8X0_TMP105_ADDR);
> + dev = DEVICE(i2c_slave_create_simple(i2c, TYPE_TMP105, N8X0_TMP105_ADDR));
> qdev_connect_gpio_out(dev, 0, tmp_irq);
> }
>
> @@ -416,8 +416,8 @@ static void n810_kbd_setup(struct n800_s *s)
>
> /* Attach the LM8322 keyboard to the I2C bus,
> * should happen in n8x0_i2c_setup and s->kbd be initialised here. */
> - s->kbd = i2c_create_slave(omap_i2c_bus(s->mpu->i2c[0]),
> - "lm8323", N810_LM8323_ADDR);
> + s->kbd = DEVICE(i2c_slave_create_simple(omap_i2c_bus(s->mpu->i2c[0]),
> + "lm8323", N810_LM8323_ADDR));
> qdev_connect_gpio_out(s->kbd, 0, kbd_irq);
> }
>
> diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
> index f104a33463..6203c4cfe0 100644
> --- a/hw/arm/pxa2xx.c
> +++ b/hw/arm/pxa2xx.c
> @@ -1522,8 +1522,9 @@ PXA2xxI2CState *pxa2xx_i2c_init(hwaddr base,
> s = PXA2XX_I2C(i2c_dev);
> /* FIXME: Should the slave device really be on a separate bus? */
> i2cbus = i2c_init_bus(dev, "dummy");
> - dev = i2c_create_slave(i2cbus, TYPE_PXA2XX_I2C_SLAVE, 0);
> - s->slave = PXA2XX_I2C_SLAVE(dev);
> + s->slave = PXA2XX_I2C_SLAVE(i2c_slave_create_simple(i2cbus,
> + TYPE_PXA2XX_I2C_SLAVE,
> + 0));
> s->slave->host = s;
>
> return s;
> diff --git a/hw/arm/realview.c b/hw/arm/realview.c
> index b6c0a1adb9..e105b6509f 100644
> --- a/hw/arm/realview.c
> +++ b/hw/arm/realview.c
> @@ -285,7 +285,7 @@ static void realview_init(MachineState *machine,
>
> dev = sysbus_create_simple(TYPE_VERSATILE_I2C, 0x10002000, NULL);
> i2c = (I2CBus *)qdev_get_child_bus(dev, "i2c");
> - i2c_create_slave(i2c, "ds1338", 0x68);
> + i2c_slave_create_simple(i2c, "ds1338", 0x68);
>
> /* Memory map for RealView Emulation Baseboard: */
> /* 0x10000000 System registers. */
> diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
> index fc18212e68..716ca3c7b6 100644
> --- a/hw/arm/spitz.c
> +++ b/hw/arm/spitz.c
> @@ -758,7 +758,7 @@ static void spitz_i2c_setup(PXA2xxState *cpu)
> DeviceState *wm;
>
> /* Attach a WM8750 to the bus */
> - wm = i2c_create_slave(bus, TYPE_WM8750, 0);
> + wm = DEVICE(i2c_slave_create_simple(bus, TYPE_WM8750, 0));
>
> spitz_wm8750_addr(wm, 0, 0);
> qdev_connect_gpio_out(cpu->gpio, SPITZ_GPIO_WM,
> @@ -773,7 +773,7 @@ static void spitz_i2c_setup(PXA2xxState *cpu)
> static void spitz_akita_i2c_setup(PXA2xxState *cpu)
> {
> /* Attach a Max7310 to Akita I2C bus. */
> - i2c_create_slave(pxa2xx_i2c_bus(cpu->i2c[0]), "max7310",
> + i2c_slave_create_simple(pxa2xx_i2c_bus(cpu->i2c[0]), "max7310",
> AKITA_MAX_ADDR);
> }
>
> diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
> index 97ef566c12..3f45550cf6 100644
> --- a/hw/arm/stellaris.c
> +++ b/hw/arm/stellaris.c
> @@ -1380,7 +1380,7 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
> qdev_get_gpio_in(nvic, 8));
> i2c = (I2CBus *)qdev_get_child_bus(dev, "i2c");
> if (board->peripherals & BP_OLED_I2C) {
> - i2c_create_slave(i2c, "ssd0303", 0x3d);
> + i2c_slave_create_simple(i2c, "ssd0303", 0x3d);
> }
> }
>
> diff --git a/hw/arm/tosa.c b/hw/arm/tosa.c
> index 5dee2d76c6..8c1ee0cdd1 100644
> --- a/hw/arm/tosa.c
> +++ b/hw/arm/tosa.c
> @@ -206,7 +206,7 @@ static uint8_t tosa_dac_recv(I2CSlave *s)
> static void tosa_tg_init(PXA2xxState *cpu)
> {
> I2CBus *bus = pxa2xx_i2c_bus(cpu->i2c[0]);
> - i2c_create_slave(bus, TYPE_TOSA_DAC, DAC_BASE);
> + i2c_slave_create_simple(bus, TYPE_TOSA_DAC, DAC_BASE);
> ssi_create_slave(cpu->ssp[1], "tosa-ssp");
> }
>
> diff --git a/hw/arm/versatilepb.c b/hw/arm/versatilepb.c
> index e596b8170f..34709afb32 100644
> --- a/hw/arm/versatilepb.c
> +++ b/hw/arm/versatilepb.c
> @@ -317,7 +317,7 @@ static void versatile_init(MachineState *machine, int board_id)
>
> dev = sysbus_create_simple(TYPE_VERSATILE_I2C, 0x10002000, NULL);
> i2c = (I2CBus *)qdev_get_child_bus(dev, "i2c");
> - i2c_create_slave(i2c, "ds1338", 0x68);
> + i2c_slave_create_simple(i2c, "ds1338", 0x68);
>
> /* Add PL041 AACI Interface to the LM4549 codec */
> pl041 = qdev_new("pl041");
> diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
> index 5bf9cff8a8..4f6a2b6ddd 100644
> --- a/hw/arm/vexpress.c
> +++ b/hw/arm/vexpress.c
> @@ -642,7 +642,7 @@ static void vexpress_common_init(MachineState *machine)
>
> dev = sysbus_create_simple(TYPE_VERSATILE_I2C, map[VE_SERIALDVI], NULL);
> i2c = (I2CBus *)qdev_get_child_bus(dev, "i2c");
> - i2c_create_slave(i2c, "sii9022", 0x39);
> + i2c_slave_create_simple(i2c, "sii9022", 0x39);
>
> sysbus_create_simple("pl031", map[VE_RTC], pic[4]); /* RTC */
>
> diff --git a/hw/arm/z2.c b/hw/arm/z2.c
> index a0f4095990..8cf8189f6f 100644
> --- a/hw/arm/z2.c
> +++ b/hw/arm/z2.c
> @@ -327,8 +327,8 @@ static void z2_init(MachineState *machine)
> type_register_static(&aer915_info);
> z2_lcd = ssi_create_slave(mpu->ssp[1], "zipit-lcd");
> bus = pxa2xx_i2c_bus(mpu->i2c[0]);
> - i2c_create_slave(bus, TYPE_AER915, 0x55);
> - wm = i2c_create_slave(bus, TYPE_WM8750, 0x1b);
> + i2c_slave_create_simple(bus, TYPE_AER915, 0x55);
> + wm = DEVICE(i2c_slave_create_simple(bus, TYPE_WM8750, 0x1b));
> mpu->i2s->opaque = wm;
> mpu->i2s->codec_out = wm8750_dac_dat;
> mpu->i2s->codec_in = wm8750_adc_dat;
> diff --git a/hw/display/sii9022.c b/hw/display/sii9022.c
> index 0710ce9de5..3b82a8567f 100644
> --- a/hw/display/sii9022.c
> +++ b/hw/display/sii9022.c
> @@ -161,7 +161,7 @@ static void sii9022_realize(DeviceState *dev, Error **errp)
> I2CBus *bus;
>
> bus = I2C_BUS(qdev_get_parent_bus(dev));
> - i2c_create_slave(bus, TYPE_I2CDDC, 0x50);
> + i2c_slave_create_simple(bus, TYPE_I2CDDC, 0x50);
> }
>
> static void sii9022_class_init(ObjectClass *klass, void *data)
> diff --git a/hw/i2c/core.c b/hw/i2c/core.c
> index 135ea56036..21ec52ac5a 100644
> --- a/hw/i2c/core.c
> +++ b/hw/i2c/core.c
> @@ -281,11 +281,11 @@ bool i2c_slave_realize_and_unref(I2CSlave *dev, I2CBus *bus, Error **errp)
> return qdev_realize_and_unref(&dev->qdev, &bus->qbus, errp);
> }
>
> -DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr)
> +I2CSlave *i2c_slave_create_simple(I2CBus *bus, const char *name, uint8_t addr)
> {
> - DeviceState *dev = DEVICE(i2c_slave_new(name, addr));
> + I2CSlave *dev = i2c_slave_new(name, addr);
>
> - i2c_slave_realize_and_unref(I2C_SLAVE(dev), bus, &error_fatal);
> + i2c_slave_realize_and_unref(dev, bus, &error_abort);
>
> return dev;
> }
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 51bf95b303..67924915ae 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -944,7 +944,7 @@ void ppce500_init(MachineState *machine)
> memory_region_add_subregion(ccsr_addr_space, MPC8544_I2C_REGS_OFFSET,
> sysbus_mmio_get_region(s, 0));
> i2c = (I2CBus *)qdev_get_child_bus(dev, "i2c");
> - i2c_create_slave(i2c, "ds1338", RTC_REGS_OFFSET);
> + i2c_slave_create_simple(i2c, "ds1338", RTC_REGS_OFFSET);
>
>
> /* General Utility device */
> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
> index 1a106a68de..1702344c46 100644
> --- a/hw/ppc/sam460ex.c
> +++ b/hw/ppc/sam460ex.c
> @@ -339,7 +339,7 @@ static void sam460ex_init(MachineState *machine)
> spd_data[20] = 4; /* SO-DIMM module */
> smbus_eeprom_init_one(i2c, 0x50, spd_data);
> /* RTC */
> - i2c_create_slave(i2c, "m41t80", 0x68);
> + i2c_slave_create_simple(i2c, "m41t80", 0x68);
>
> dev = sysbus_create_simple(TYPE_PPC4xx_I2C, 0x4ef600800, uic[0][3]);
>
> --
> 2.21.3
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 5/5] hw/i2c: Document the I2C qdev helpers
2020-06-29 17:38 ` [PATCH 5/5] hw/i2c: Document the I2C qdev helpers Philippe Mathieu-Daudé
@ 2020-06-29 21:30 ` Corey Minyard
2020-06-30 10:15 ` Markus Armbruster
1 sibling, 0 replies; 22+ messages in thread
From: Corey Minyard @ 2020-06-29 21:30 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Corey Minyard, Andrew Jeffery, qemu-devel,
Markus Armbruster, qemu-arm, qemu-ppc, Cédric Le Goater,
Jan Kiszka, David Gibson, Joel Stanley
On Mon, Jun 29, 2020 at 07:38:21PM +0200, Philippe Mathieu-Daudé wrote:
> In commit d88c42ff2c we added new prototype but neglected to
> add their documentation. Fix that.
Reviewed-by: Corey Minyard <cminyard@mvista.com>
>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> include/hw/i2c/i2c.h | 48 ++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 48 insertions(+)
>
> diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
> index c533058998..fcc61e509b 100644
> --- a/include/hw/i2c/i2c.h
> +++ b/include/hw/i2c/i2c.h
> @@ -79,8 +79,56 @@ int i2c_send_recv(I2CBus *bus, uint8_t *data, bool send);
> int i2c_send(I2CBus *bus, uint8_t data);
> uint8_t i2c_recv(I2CBus *bus);
>
> +/**
> + * Create an I2C slave device on the heap.
> + * @name: a device type name
> + * @addr: I2C address of the slave when put on a bus
> + *
> + * This only initializes the device state structure and allows
> + * properties to be set. Type @name must exist. The device still
> + * needs to be realized. See qdev-core.h.
> + */
> I2CSlave *i2c_slave_new(const char *name, uint8_t addr);
> +
> +/**
> + * Create an I2C slave device on the heap.
> + * @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.
> + */
> 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
> + * @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
> + *
> + * 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.
> + *
> + * 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.)
> + */
> bool i2c_slave_realize_and_unref(I2CSlave *dev, I2CBus *bus, Error **errp);
>
> /* lm832x.c */
> --
> 2.21.3
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/5] hw/i2c: Rename i2c_try_create_slave() as i2c_slave_new()
2020-06-29 17:38 ` [PATCH 2/5] hw/i2c: Rename i2c_try_create_slave() as i2c_slave_new() Philippe Mathieu-Daudé
2020-06-29 21:29 ` Corey Minyard
@ 2020-06-29 21:37 ` BALATON Zoltan
2020-06-30 8:29 ` Philippe Mathieu-Daudé
2020-06-30 9:37 ` Markus Armbruster
1 sibling, 2 replies; 22+ messages in thread
From: BALATON Zoltan @ 2020-06-29 21:37 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Corey Minyard, Andrew Jeffery, qemu-devel,
Markus Armbruster, qemu-arm, qemu-ppc, Cédric Le Goater,
Jan Kiszka, David Gibson, Joel Stanley
[-- Attachment #1: Type: text/plain, Size: 3681 bytes --]
On Mon, 29 Jun 2020, Philippe Mathieu-Daudé wrote:
> 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(). Since we have to update
> all the callers, also let it return a I2CSlave object.
All the callers now need a cast due to change to I2CSlave * instead of
what they expect. Does that really worth it? Also this introduces
inconsistency between i2c_create_slave and i2c_new so not sure about that
part but I don't really mind either way. Maybe return what most callers
expect so the calls are simple and don't need an additional cast in most
of the cases?
Regards,
BALATON Zoltan
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> include/hw/i2c/i2c.h | 2 +-
> hw/arm/aspeed.c | 4 ++--
> hw/i2c/core.c | 9 ++++-----
> 3 files changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
> index d6e3d85faf..18efc668f1 100644
> --- a/include/hw/i2c/i2c.h
> +++ b/include/hw/i2c/i2c.h
> @@ -79,8 +79,8 @@ 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);
>
> +I2CSlave *i2c_slave_new(const char *name, uint8_t addr);
> 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 */
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 1285bf82c0..54ca36e0b6 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -513,7 +513,7 @@ static void witherspoon_bmc_i2c_init(AspeedMachineState *bmc)
> /* Bus 3: TODO bmp280@77 */
> /* Bus 3: TODO max31785@52 */
> /* Bus 3: TODO dps310@76 */
> - dev = i2c_try_create_slave(TYPE_PCA9552, 0x60);
> + dev = DEVICE(i2c_slave_new(TYPE_PCA9552, 0x60));
> qdev_prop_set_string(dev, "description", "pca1");
> i2c_realize_and_unref(dev, aspeed_i2c_get_bus(&soc->i2c, 3),
> &error_fatal);
> @@ -531,7 +531,7 @@ static void witherspoon_bmc_i2c_init(AspeedMachineState *bmc)
>
> smbus_eeprom_init_one(aspeed_i2c_get_bus(&soc->i2c, 11), 0x51,
> eeprom_buf);
> - dev = i2c_try_create_slave(TYPE_PCA9552, 0x60);
> + dev = DEVICE(i2c_slave_new(TYPE_PCA9552, 0x60));
> qdev_prop_set_string(dev, "description", "pca0");
> i2c_realize_and_unref(dev, aspeed_i2c_get_bus(&soc->i2c, 11),
> &error_fatal);
> diff --git a/hw/i2c/core.c b/hw/i2c/core.c
> index acf34a12d6..6eacb4a463 100644
> --- a/hw/i2c/core.c
> +++ b/hw/i2c/core.c
> @@ -267,13 +267,13 @@ const VMStateDescription vmstate_i2c_slave = {
> }
> };
>
> -DeviceState *i2c_try_create_slave(const char *name, uint8_t addr)
> +I2CSlave *i2c_slave_new(const char *name, uint8_t addr)
> {
> DeviceState *dev;
>
> dev = qdev_new(name);
> qdev_prop_set_uint8(dev, "address", addr);
> - return dev;
> + return I2C_SLAVE(dev);
> }
>
> bool i2c_realize_and_unref(DeviceState *dev, I2CBus *bus, Error **errp)
> @@ -283,10 +283,9 @@ bool i2c_realize_and_unref(DeviceState *dev, I2CBus *bus, Error **errp)
>
> DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr)
> {
> - DeviceState *dev;
> + DeviceState *dev = DEVICE(i2c_slave_new(name, addr));
>
> - dev = i2c_try_create_slave(name, addr);
> - i2c_realize_and_unref(dev, bus, &error_fatal);
> + i2c_realize_and_unref(I2C_SLAVE(dev), bus, &error_fatal);
>
> return dev;
> }
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/5] hw/i2c: Rename i2c_try_create_slave() as i2c_slave_new()
2020-06-29 21:37 ` BALATON Zoltan
@ 2020-06-30 8:29 ` Philippe Mathieu-Daudé
2020-06-30 9:37 ` Markus Armbruster
1 sibling, 0 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-30 8:29 UTC (permalink / raw)
To: BALATON Zoltan
Cc: Peter Maydell, Corey Minyard, Andrew Jeffery, Joel Stanley,
qemu-devel, Markus Armbruster, qemu-arm, qemu-ppc,
Cédric Le Goater, Jan Kiszka, David Gibson
On 6/29/20 11:37 PM, BALATON Zoltan wrote:
> On Mon, 29 Jun 2020, Philippe Mathieu-Daudé wrote:
>> 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(). Since we have to update
>> all the callers, also let it return a I2CSlave object.
>
> All the callers now need a cast due to change to I2CSlave * instead of
> what they expect. Does that really worth it? Also this introduces
> inconsistency between i2c_create_slave and i2c_new so not sure about
> that part but I don't really mind either way. Maybe return what most
> callers expect so the calls are simple and don't need an additional cast
> in most of the cases?
You are right that the code guidance is not clear regarding what
qdev_foo_new() should return.
Working on another object (LEDs) Richard suggested me to return
the full type, so I understood it was the recommended default:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg714729.html
>
> Regards,
> BALATON Zoltan
>
>> Suggested-by: Markus Armbruster <armbru@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> include/hw/i2c/i2c.h | 2 +-
>> hw/arm/aspeed.c | 4 ++--
>> hw/i2c/core.c | 9 ++++-----
>> 3 files changed, 7 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
>> index d6e3d85faf..18efc668f1 100644
>> --- a/include/hw/i2c/i2c.h
>> +++ b/include/hw/i2c/i2c.h
>> @@ -79,8 +79,8 @@ 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);
>>
>> +I2CSlave *i2c_slave_new(const char *name, uint8_t addr);
>> 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 */
>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>> index 1285bf82c0..54ca36e0b6 100644
>> --- a/hw/arm/aspeed.c
>> +++ b/hw/arm/aspeed.c
>> @@ -513,7 +513,7 @@ static void
>> witherspoon_bmc_i2c_init(AspeedMachineState *bmc)
>> /* Bus 3: TODO bmp280@77 */
>> /* Bus 3: TODO max31785@52 */
>> /* Bus 3: TODO dps310@76 */
>> - dev = i2c_try_create_slave(TYPE_PCA9552, 0x60);
>> + dev = DEVICE(i2c_slave_new(TYPE_PCA9552, 0x60));
>> qdev_prop_set_string(dev, "description", "pca1");
>> i2c_realize_and_unref(dev, aspeed_i2c_get_bus(&soc->i2c, 3),
>> &error_fatal);
>> @@ -531,7 +531,7 @@ static void
>> witherspoon_bmc_i2c_init(AspeedMachineState *bmc)
>>
>> smbus_eeprom_init_one(aspeed_i2c_get_bus(&soc->i2c, 11), 0x51,
>> eeprom_buf);
>> - dev = i2c_try_create_slave(TYPE_PCA9552, 0x60);
>> + dev = DEVICE(i2c_slave_new(TYPE_PCA9552, 0x60));
>> qdev_prop_set_string(dev, "description", "pca0");
>> i2c_realize_and_unref(dev, aspeed_i2c_get_bus(&soc->i2c, 11),
>> &error_fatal);
>> diff --git a/hw/i2c/core.c b/hw/i2c/core.c
>> index acf34a12d6..6eacb4a463 100644
>> --- a/hw/i2c/core.c
>> +++ b/hw/i2c/core.c
>> @@ -267,13 +267,13 @@ const VMStateDescription vmstate_i2c_slave = {
>> }
>> };
>>
>> -DeviceState *i2c_try_create_slave(const char *name, uint8_t addr)
>> +I2CSlave *i2c_slave_new(const char *name, uint8_t addr)
>> {
>> DeviceState *dev;
>>
>> dev = qdev_new(name);
>> qdev_prop_set_uint8(dev, "address", addr);
>> - return dev;
>> + return I2C_SLAVE(dev);
>> }
>>
>> bool i2c_realize_and_unref(DeviceState *dev, I2CBus *bus, Error **errp)
>> @@ -283,10 +283,9 @@ bool i2c_realize_and_unref(DeviceState *dev,
>> I2CBus *bus, Error **errp)
>>
>> DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t
>> addr)
>> {
>> - DeviceState *dev;
>> + DeviceState *dev = DEVICE(i2c_slave_new(name, addr));
>>
>> - dev = i2c_try_create_slave(name, addr);
>> - i2c_realize_and_unref(dev, bus, &error_fatal);
>> + i2c_realize_and_unref(I2C_SLAVE(dev), bus, &error_fatal);
>>
>> return dev;
>> }
>>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/5] hw/i2c/aspeed_i2c: Simplify aspeed_i2c_get_bus()
2020-06-29 17:38 ` [PATCH 1/5] hw/i2c/aspeed_i2c: Simplify aspeed_i2c_get_bus() Philippe Mathieu-Daudé
@ 2020-06-30 9:28 ` Markus Armbruster
2020-07-13 12:23 ` Cédric Le Goater
1 sibling, 0 replies; 22+ messages in thread
From: Markus Armbruster @ 2020-06-30 9:28 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Corey Minyard, Andrew Jeffery, qemu-devel,
qemu-arm, qemu-ppc, Cédric Le Goater, Jan Kiszka,
David Gibson, Joel Stanley
Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> Simplify aspeed_i2c_get_bus() by using a AspeedI2CState argument.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
The real motivation seems to be simplifying the callers: every single
one of them casts the argument from AspeedI2CState * to DeviceState *.
Pointing that out in the commit message wouldn't hurt.
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/5] hw/i2c: Rename i2c_try_create_slave() as i2c_slave_new()
2020-06-29 21:37 ` BALATON Zoltan
2020-06-30 8:29 ` Philippe Mathieu-Daudé
@ 2020-06-30 9:37 ` Markus Armbruster
1 sibling, 0 replies; 22+ messages in thread
From: Markus Armbruster @ 2020-06-30 9:37 UTC (permalink / raw)
To: BALATON Zoltan
Cc: Peter Maydell, Corey Minyard, Andrew Jeffery, Joel Stanley,
qemu-devel, Philippe Mathieu-Daudé,
qemu-arm, qemu-ppc, Cédric Le Goater, Jan Kiszka,
David Gibson
BALATON Zoltan <balaton@eik.bme.hu> writes:
> On Mon, 29 Jun 2020, Philippe Mathieu-Daudé wrote:
>> 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(). Since we have to update
>> all the callers, also let it return a I2CSlave object.
>
> All the callers now need a cast due to change to I2CSlave * instead of
Actually, all but one; I'll mark it inline.
> what they expect. Does that really worth it? Also this introduces
> inconsistency between i2c_create_slave and i2c_new so not sure about
For what it's worth, this inconsistency is healed in PATCH 4.
> that part but I don't really mind either way. Maybe return what most
> callers expect so the calls are simple and don't need an additional
> cast in most of the cases?
I'd prefer consistency with similar FOO_new() functions for abstract
devices plugging into a FOOBus: pci_new(), isa_new(), usb_new().
> Regards,
> BALATON Zoltan
>
>> Suggested-by: Markus Armbruster <armbru@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> include/hw/i2c/i2c.h | 2 +-
>> hw/arm/aspeed.c | 4 ++--
>> hw/i2c/core.c | 9 ++++-----
>> 3 files changed, 7 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
>> index d6e3d85faf..18efc668f1 100644
>> --- a/include/hw/i2c/i2c.h
>> +++ b/include/hw/i2c/i2c.h
>> @@ -79,8 +79,8 @@ 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);
>>
>> +I2CSlave *i2c_slave_new(const char *name, uint8_t addr);
>> 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 */
>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>> index 1285bf82c0..54ca36e0b6 100644
>> --- a/hw/arm/aspeed.c
>> +++ b/hw/arm/aspeed.c
>> @@ -513,7 +513,7 @@ static void witherspoon_bmc_i2c_init(AspeedMachineState *bmc)
>> /* Bus 3: TODO bmp280@77 */
>> /* Bus 3: TODO max31785@52 */
>> /* Bus 3: TODO dps310@76 */
>> - dev = i2c_try_create_slave(TYPE_PCA9552, 0x60);
>> + dev = DEVICE(i2c_slave_new(TYPE_PCA9552, 0x60));
>> qdev_prop_set_string(dev, "description", "pca1");
>> i2c_realize_and_unref(dev, aspeed_i2c_get_bus(&soc->i2c, 3),
>> &error_fatal);
>> @@ -531,7 +531,7 @@ static void witherspoon_bmc_i2c_init(AspeedMachineState *bmc)
>>
>> smbus_eeprom_init_one(aspeed_i2c_get_bus(&soc->i2c, 11), 0x51,
>> eeprom_buf);
>> - dev = i2c_try_create_slave(TYPE_PCA9552, 0x60);
>> + dev = DEVICE(i2c_slave_new(TYPE_PCA9552, 0x60));
>> qdev_prop_set_string(dev, "description", "pca0");
>> i2c_realize_and_unref(dev, aspeed_i2c_get_bus(&soc->i2c, 11),
>> &error_fatal);
>> diff --git a/hw/i2c/core.c b/hw/i2c/core.c
>> index acf34a12d6..6eacb4a463 100644
>> --- a/hw/i2c/core.c
>> +++ b/hw/i2c/core.c
>> @@ -267,13 +267,13 @@ const VMStateDescription vmstate_i2c_slave = {
>> }
>> };
>>
>> -DeviceState *i2c_try_create_slave(const char *name, uint8_t addr)
>> +I2CSlave *i2c_slave_new(const char *name, uint8_t addr)
>> {
>> DeviceState *dev;
>>
>> dev = qdev_new(name);
>> qdev_prop_set_uint8(dev, "address", addr);
>> - return dev;
>> + return I2C_SLAVE(dev);
>> }
>>
>> bool i2c_realize_and_unref(DeviceState *dev, I2CBus *bus, Error **errp)
>> @@ -283,10 +283,9 @@ bool i2c_realize_and_unref(DeviceState *dev, I2CBus *bus, Error **errp)
>>
>> DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr)
>> {
>> - DeviceState *dev;
>> + DeviceState *dev = DEVICE(i2c_slave_new(name, addr));
>>
>> - dev = i2c_try_create_slave(name, addr);
>> - i2c_realize_and_unref(dev, bus, &error_fatal);
>> + i2c_realize_and_unref(I2C_SLAVE(dev), bus, &error_fatal);
>>
>> return dev;
>> }
Fewer casts:
DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr)
{
I2CSlave *dev = i2c_slave_new(name, addr );
i2c_realize_and_unref(dev, bus, &error_fatal);
return DEVICE(dev);
}
It's all the same after PATCH 4. You decide whether to polish the
intermediate state.
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/5] hw/i2c: Rename i2c_create_slave() as i2c_slave_create_simple()
2020-06-29 17:38 ` [PATCH 4/5] hw/i2c: Rename i2c_create_slave() as i2c_slave_create_simple() Philippe Mathieu-Daudé
2020-06-29 21:29 ` Corey Minyard
@ 2020-06-30 9:48 ` Markus Armbruster
1 sibling, 0 replies; 22+ messages in thread
From: Markus Armbruster @ 2020-06-30 9:48 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Corey Minyard, Andrew Jeffery, qemu-devel,
qemu-arm, qemu-ppc, Cédric Le Goater, Jan Kiszka,
David Gibson, Joel Stanley
Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> We use "create_simple" names for functions that allocate, initialize,
> configure and realize device objects: pci_create_simple(),
> isa_create_simple(), usb_create_simple(). For consistency, rename
> i2c_create_slave() as i2c_slave_create_simple(). Since we have
> to update all the callers, also let it return a I2CSlave object.
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> 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
* Re: [PATCH 5/5] hw/i2c: Document the I2C qdev helpers
2020-06-29 17:38 ` [PATCH 5/5] hw/i2c: Document the I2C qdev helpers Philippe Mathieu-Daudé
2020-06-29 21:30 ` Corey Minyard
@ 2020-06-30 10:15 ` Markus Armbruster
2020-06-30 10:45 ` Philippe Mathieu-Daudé
2020-06-30 13:16 ` Peter Maydell
1 sibling, 2 replies; 22+ messages in thread
From: Markus Armbruster @ 2020-06-30 10:15 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Corey Minyard, Andrew Jeffery, qemu-devel,
qemu-arm, qemu-ppc, Cédric Le Goater, Jan Kiszka,
David Gibson, Joel Stanley
Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> In commit d88c42ff2c we added new prototype but neglected to
> add their documentation. Fix that.
>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> include/hw/i2c/i2c.h | 48 ++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 48 insertions(+)
>
> diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
> index c533058998..fcc61e509b 100644
> --- a/include/hw/i2c/i2c.h
> +++ b/include/hw/i2c/i2c.h
> @@ -79,8 +79,56 @@ int i2c_send_recv(I2CBus *bus, uint8_t *data, bool send);
> int i2c_send(I2CBus *bus, uint8_t data);
> uint8_t i2c_recv(I2CBus *bus);
>
> +/**
> + * Create an I2C slave device on the heap.
> + * @name: a device type name
> + * @addr: I2C address of the slave when put on a bus
> + *
> + * This only initializes the device state structure and allows
> + * properties to be set. Type @name must exist. The device still
> + * needs to be realized. See qdev-core.h.
> + */
> I2CSlave *i2c_slave_new(const char *name, uint8_t addr);
> +
> +/**
> + * Create an I2C slave device on the heap.
Suggest "Create and realize ..."
> + * @bus: I2C bus to put it on
> + * @name: I2C slave device type name
> + * @addr: I2C address of the slave when put on a bus
> + *
> + * Create the device state structure, initialize it, put it on the
> + * specified @bus, and drop the reference to it (the device is realized).
> + * Any error aborts the process.
Stick to imperative mood: Abort on error.
Do we need the sentence? Doc comments of object_new(), qdev_new() and
i2c_slave_new() don't have it, they document *preconditions* instead,
using "must", and rely on the tacit understanding that a function may
abort or crash when its documented preconditions aren't met. Matter of
taste, I guess.
> + */
> I2CSlave *i2c_slave_create_simple(I2CBus *bus, const char *name, uint8_t addr);
> +
> +/**
> + * i2c_slave_realize_and_unref: realize and unref an I2C slave device
Either consistently waste space for repeating the function name at the
beginning of its doc comment, or consistently don't :)
qdev_realize_and_unref()'s doc comment says "and drop a reference"
instead of "unref", because "unref" is not a word.
> + * @dev: I2C slave device to realize
> + * @bus: I2C bus to put it on
> + * @addr: I2C address of the slave on the bus
> + * @errp: error pointer
$ git-grep -h "@errp:" | sort -u
* @errp: pointer to Error*, to store an error if it happens
* @errp: error object
* @errp: Error object
* @errp: Error object which may be set by job_complete(); this is not
* @errp: Error object.
* @errp: If an error occurs, a pointer to an area to store the error
* @errp: Output pointer for error information. Can be NULL.
* @errp: Pointer for reporting an #Error.
* @errp: Populated with an error in failure cases
* @errp: a pointer to an Error that is filled if getting/setting fails.
* @errp: a pointer to return the #Error object if an error occurs.
* @errp: an error indicator
* @errp: error
* @errp: error object
* @errp: error object handle
* @errp: handle to an error object
* @errp: if an error occurs, a pointer to an area to store the error
* @errp: indirect pointer to Error to be set
* @errp: location to store error
* @errp: location to store error information
* @errp: location to store error information.
* @errp: location to store error, will be set only for exception
* @errp: pointer to Error*, to store an error if it happens.
* @errp: pointer to NULL initialized error object
* @errp: pointer to a NULL initialized error object
* @errp: pointer to a NULL-initialized error object
* @errp: pointer to an error
* @errp: pointer to error object
* @errp: pointer to initialized error object
* @errp: pointer to uninitialized error object
Aside: gotta love these two.
* @errp: returns an error if this function fails
* @errp: set *errp if the check failed, with reason
* @errp: set in case of an error
* @errp: set on error
* @errp: unused
* @errp: where to put errors
Plenty of choice, recommend not to invent another one :)
> + *
> + * Call 'realize' on @dev, put it on the specified @bus, and drop the
> + * reference to it. Errors are reported via @errp and by returning
> + * false.
Recommend to use a separate paragraph for the return value. Since your
comment style resembles GTK-Doc style[*], you may just as well use it
for the return value, like this:
Returns: %true on success, %false on failure.
By convention, it goes after the function description.
> + *
> + * 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.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 5/5] hw/i2c: Document the I2C qdev helpers
2020-06-30 10:15 ` Markus Armbruster
@ 2020-06-30 10:45 ` Philippe Mathieu-Daudé
2020-06-30 13:16 ` Peter Maydell
1 sibling, 0 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-30 10:45 UTC (permalink / raw)
To: Markus Armbruster
Cc: Peter Maydell, Corey Minyard, Andrew Jeffery, qemu-devel,
Joel Stanley, qemu-arm, qemu-ppc, Cédric Le Goater,
Jan Kiszka, David Gibson
On 6/30/20 12:15 PM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
>
>> In commit d88c42ff2c we added new prototype but neglected to
>> add their documentation. Fix that.
>>
>> Reported-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> include/hw/i2c/i2c.h | 48 ++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 48 insertions(+)
>>
>> diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
>> index c533058998..fcc61e509b 100644
>> --- a/include/hw/i2c/i2c.h
>> +++ b/include/hw/i2c/i2c.h
>> @@ -79,8 +79,56 @@ int i2c_send_recv(I2CBus *bus, uint8_t *data, bool send);
>> int i2c_send(I2CBus *bus, uint8_t data);
>> uint8_t i2c_recv(I2CBus *bus);
>>
>> +/**
>> + * Create an I2C slave device on the heap.
>> + * @name: a device type name
>> + * @addr: I2C address of the slave when put on a bus
>> + *
>> + * This only initializes the device state structure and allows
>> + * properties to be set. Type @name must exist. The device still
>> + * needs to be realized. See qdev-core.h.
>> + */
>> I2CSlave *i2c_slave_new(const char *name, uint8_t addr);
>> +
>> +/**
>> + * Create an I2C slave device on the heap.
>
> Suggest "Create and realize ..."
>
>> + * @bus: I2C bus to put it on
>> + * @name: I2C slave device type name
>> + * @addr: I2C address of the slave when put on a bus
>> + *
>> + * Create the device state structure, initialize it, put it on the
>> + * specified @bus, and drop the reference to it (the device is realized).
>> + * Any error aborts the process.
>
> Stick to imperative mood: Abort on error.
>
> Do we need the sentence? Doc comments of object_new(), qdev_new() and
> i2c_slave_new() don't have it, they document *preconditions* instead,
> using "must", and rely on the tacit understanding that a function may
> abort or crash when its documented preconditions aren't met. Matter of
> taste, I guess.
>
>> + */
>> I2CSlave *i2c_slave_create_simple(I2CBus *bus, const char *name, uint8_t addr);
>> +
>> +/**
>> + * i2c_slave_realize_and_unref: realize and unref an I2C slave device
>
> Either consistently waste space for repeating the function name at the
> beginning of its doc comment, or consistently don't :)
>
> qdev_realize_and_unref()'s doc comment says "and drop a reference"
> instead of "unref", because "unref" is not a word.
>
>> + * @dev: I2C slave device to realize
>> + * @bus: I2C bus to put it on
>> + * @addr: I2C address of the slave on the bus
>> + * @errp: error pointer
>
> $ git-grep -h "@errp:" | sort -u
> * @errp: pointer to Error*, to store an error if it happens
> * @errp: error object
> * @errp: Error object
> * @errp: Error object which may be set by job_complete(); this is not
> * @errp: Error object.
> * @errp: If an error occurs, a pointer to an area to store the error
> * @errp: Output pointer for error information. Can be NULL.
> * @errp: Pointer for reporting an #Error.
> * @errp: Populated with an error in failure cases
> * @errp: a pointer to an Error that is filled if getting/setting fails.
> * @errp: a pointer to return the #Error object if an error occurs.
> * @errp: an error indicator
> * @errp: error
> * @errp: error object
> * @errp: error object handle
> * @errp: handle to an error object
> * @errp: if an error occurs, a pointer to an area to store the error
> * @errp: indirect pointer to Error to be set
> * @errp: location to store error
> * @errp: location to store error information
> * @errp: location to store error information.
> * @errp: location to store error, will be set only for exception
> * @errp: pointer to Error*, to store an error if it happens.
> * @errp: pointer to NULL initialized error object
> * @errp: pointer to a NULL initialized error object
> * @errp: pointer to a NULL-initialized error object
> * @errp: pointer to an error
> * @errp: pointer to error object
> * @errp: pointer to initialized error object
> * @errp: pointer to uninitialized error object
>
> Aside: gotta love these two.
>
> * @errp: returns an error if this function fails
> * @errp: set *errp if the check failed, with reason
> * @errp: set in case of an error
> * @errp: set on error
> * @errp: unused
> * @errp: where to put errors
>
> Plenty of choice, recommend not to invent another one :)
>
>> + *
>> + * Call 'realize' on @dev, put it on the specified @bus, and drop the
>> + * reference to it. Errors are reported via @errp and by returning
>> + * false.
>
> Recommend to use a separate paragraph for the return value. Since your
> comment style resembles GTK-Doc style[*], you may just as well use it
> for the return value, like this:
>
> Returns: %true on success, %false on failure.
>
> By convention, it goes after the function description.
OK, I'll use whatever you prefer. Maybe the shorter the easier.
I will see if I can find to spend more time on this during the
week-end, but I can't promise anything. Anyway since it is
documentation it can be integrated during soft freeze.
Thanks for your detailed review.
>
>> + *
>> + * This function is useful if you have created @dev via qdev_new(),
>> + * i2c_slave_new() or i2c_slave_try_new() (which take a reference to
>> + * the device it returns to you), so that you can set properties on it
>> + * before realizing it. If you don't need to set properties then
>> + * i2c_slave_create_simple() is probably better (as it does the create,
>> + * init and realize in one step).
>> + *
>> + * If you are embedding the I2C slave into another QOM device and
>> + * initialized it via some variant on object_initialize_child() then
>> + * do not use this function, because that family of functions arrange
>> + * for the only reference to the child device to be held by the parent
>> + * via the child<> property, and so the reference-count-drop done here
>> + * would be incorrect. (Instead you would want i2c_slave_realize(),
>> + * which doesn't currently exist but would be trivial to create if we
>> + * had any code that wanted it.)
>> + */
>
> The advice on use is more elaborate qdev_realize_and_unref()'s. That
> one simply shows intended use. I doubt we need more. But as the person
> who wrote qdev_realize_and_unref(), I'm singularly unqualified judging
> the need ;)
>
>> bool i2c_slave_realize_and_unref(I2CSlave *dev, I2CBus *bus, Error **errp);
>>
>> /* lm832x.c */
>
>
> [*] A style I dislike, but it's common in QEMU, so you're certainly
> entitled to use it.
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 5/5] hw/i2c: Document the I2C qdev helpers
2020-06-30 10:15 ` Markus Armbruster
2020-06-30 10:45 ` Philippe Mathieu-Daudé
@ 2020-06-30 13:16 ` Peter Maydell
2020-07-14 7:05 ` Markus Armbruster
1 sibling, 1 reply; 22+ messages in thread
From: Peter Maydell @ 2020-06-30 13:16 UTC (permalink / raw)
To: Markus Armbruster
Cc: Corey Minyard, Andrew Jeffery, Philippe Mathieu-Daudé,
QEMU Developers, qemu-arm, qemu-ppc, Cédric Le Goater,
Jan Kiszka, David Gibson, Joel Stanley
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.
thanks
-- PMM
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/5] hw/i2c/aspeed_i2c: Simplify aspeed_i2c_get_bus()
2020-06-29 17:38 ` [PATCH 1/5] hw/i2c/aspeed_i2c: Simplify aspeed_i2c_get_bus() Philippe Mathieu-Daudé
2020-06-30 9:28 ` Markus Armbruster
@ 2020-07-13 12:23 ` Cédric Le Goater
1 sibling, 0 replies; 22+ messages in thread
From: Cédric Le Goater @ 2020-07-13 12:23 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Peter Maydell, Corey Minyard, Andrew Jeffery, Markus Armbruster,
qemu-arm, qemu-ppc, Joel Stanley, Jan Kiszka, David Gibson
On 6/29/20 7:38 PM, Philippe Mathieu-Daudé wrote:
> Simplify aspeed_i2c_get_bus() by using a AspeedI2CState argument.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
> ---
> include/hw/i2c/aspeed_i2c.h | 2 +-
> hw/arm/aspeed.c | 70 ++++++++++++++++++-------------------
> hw/i2c/aspeed_i2c.c | 3 +-
> 3 files changed, 37 insertions(+), 38 deletions(-)
>
> diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
> index f1b9e5bf91..243789ae5d 100644
> --- a/include/hw/i2c/aspeed_i2c.h
> +++ b/include/hw/i2c/aspeed_i2c.h
> @@ -93,6 +93,6 @@ typedef struct AspeedI2CClass {
>
> } AspeedI2CClass;
>
> -I2CBus *aspeed_i2c_get_bus(DeviceState *dev, int busnr);
> +I2CBus *aspeed_i2c_get_bus(AspeedI2CState *s, int busnr);
>
> #endif /* ASPEED_I2C_H */
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 379f9672a5..1285bf82c0 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -385,13 +385,13 @@ static void palmetto_bmc_i2c_init(AspeedMachineState *bmc)
>
> /* The palmetto platform expects a ds3231 RTC but a ds1338 is
> * enough to provide basic RTC features. Alarms will be missing */
> - i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 0), "ds1338", 0x68);
> + i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 0), "ds1338", 0x68);
>
> - smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 0), 0x50,
> + smbus_eeprom_init_one(aspeed_i2c_get_bus(&soc->i2c, 0), 0x50,
> eeprom_buf);
>
> /* add a TMP423 temperature sensor */
> - dev = i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 2),
> + dev = i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 2),
> "tmp423", 0x4c);
> object_property_set_int(OBJECT(dev), 31000, "temperature0", &error_abort);
> object_property_set_int(OBJECT(dev), 28000, "temperature1", &error_abort);
> @@ -404,16 +404,16 @@ static void ast2500_evb_i2c_init(AspeedMachineState *bmc)
> AspeedSoCState *soc = &bmc->soc;
> uint8_t *eeprom_buf = g_malloc0(8 * 1024);
>
> - smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 3), 0x50,
> + smbus_eeprom_init_one(aspeed_i2c_get_bus(&soc->i2c, 3), 0x50,
> eeprom_buf);
>
> /* The AST2500 EVB expects a LM75 but a TMP105 is compatible */
> - i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 7),
> + i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 7),
> TYPE_TMP105, 0x4d);
>
> /* The AST2500 EVB does not have an RTC. Let's pretend that one is
> * plugged on the I2C bus header */
> - i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), "ds1338", 0x32);
> + i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 11), "ds1338", 0x32);
> }
>
> static void ast2600_evb_i2c_init(AspeedMachineState *bmc)
> @@ -428,36 +428,36 @@ static void romulus_bmc_i2c_init(AspeedMachineState *bmc)
>
> /* The romulus board expects Epson RX8900 I2C RTC but a ds1338 is
> * good enough */
> - i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), "ds1338", 0x32);
> + i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 11), "ds1338", 0x32);
> }
>
> static void swift_bmc_i2c_init(AspeedMachineState *bmc)
> {
> AspeedSoCState *soc = &bmc->soc;
>
> - i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 3), "pca9552", 0x60);
> + i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 3), "pca9552", 0x60);
>
> /* The swift board expects a TMP275 but a TMP105 is compatible */
> - i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 7), "tmp105", 0x48);
> + i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 7), "tmp105", 0x48);
> /* The swift board expects a pca9551 but a pca9552 is compatible */
> - i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 7), "pca9552", 0x60);
> + i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 7), "pca9552", 0x60);
>
> /* The swift board expects an Epson RX8900 RTC but a ds1338 is compatible */
> - i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 8), "ds1338", 0x32);
> - i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 8), "pca9552", 0x60);
> + i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 8), "ds1338", 0x32);
> + i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 8), "pca9552", 0x60);
>
> - i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 9), "tmp423", 0x4c);
> + i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 9), "tmp423", 0x4c);
> /* The swift board expects a pca9539 but a pca9552 is compatible */
> - i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 9), "pca9552", 0x74);
> + i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 9), "pca9552", 0x74);
>
> - i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 10), "tmp423", 0x4c);
> + i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 10), "tmp423", 0x4c);
> /* The swift board expects a pca9539 but a pca9552 is compatible */
> - i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 10), "pca9552",
> + i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 10), "pca9552",
> 0x74);
>
> /* The swift board expects a TMP275 but a TMP105 is compatible */
> - i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 12), "tmp105", 0x48);
> - i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 12), "tmp105", 0x4a);
> + i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 12), "tmp105", 0x48);
> + i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 12), "tmp105", 0x4a);
> }
>
> static void sonorapass_bmc_i2c_init(AspeedMachineState *bmc)
> @@ -465,32 +465,32 @@ static void sonorapass_bmc_i2c_init(AspeedMachineState *bmc)
> AspeedSoCState *soc = &bmc->soc;
>
> /* bus 2 : */
> - i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 2), "tmp105", 0x48);
> - i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 2), "tmp105", 0x49);
> + i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 2), "tmp105", 0x48);
> + i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 2), "tmp105", 0x49);
> /* bus 2 : pca9546 @ 0x73 */
>
> /* bus 3 : pca9548 @ 0x70 */
>
> /* bus 4 : */
> uint8_t *eeprom4_54 = g_malloc0(8 * 1024);
> - smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 4), 0x54,
> + smbus_eeprom_init_one(aspeed_i2c_get_bus(&soc->i2c, 4), 0x54,
> eeprom4_54);
> /* PCA9539 @ 0x76, but PCA9552 is compatible */
> - i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 4), "pca9552", 0x76);
> + i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 4), "pca9552", 0x76);
> /* PCA9539 @ 0x77, but PCA9552 is compatible */
> - i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 4), "pca9552", 0x77);
> + i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 4), "pca9552", 0x77);
>
> /* bus 6 : */
> - i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 6), "tmp105", 0x48);
> - i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 6), "tmp105", 0x49);
> + i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 6), "tmp105", 0x48);
> + i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 6), "tmp105", 0x49);
> /* bus 6 : pca9546 @ 0x73 */
>
> /* bus 8 : */
> uint8_t *eeprom8_56 = g_malloc0(8 * 1024);
> - smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 8), 0x56,
> + smbus_eeprom_init_one(aspeed_i2c_get_bus(&soc->i2c, 8), 0x56,
> eeprom8_56);
> - i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 8), "pca9552", 0x60);
> - i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 8), "pca9552", 0x61);
> + i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 8), "pca9552", 0x60);
> + i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 8), "pca9552", 0x61);
> /* bus 8 : adc128d818 @ 0x1d */
> /* bus 8 : adc128d818 @ 0x1f */
>
> @@ -515,25 +515,25 @@ static void witherspoon_bmc_i2c_init(AspeedMachineState *bmc)
> /* Bus 3: TODO dps310@76 */
> 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),
> + i2c_realize_and_unref(dev, aspeed_i2c_get_bus(&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);
> + i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 4), "tmp423", 0x4c);
> + i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 5), "tmp423", 0x4c);
>
> /* The Witherspoon expects a TMP275 but a TMP105 is compatible */
> - i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 9), TYPE_TMP105,
> + i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 9), TYPE_TMP105,
> 0x4a);
>
> /* The witherspoon board expects Epson RX8900 I2C RTC but a ds1338 is
> * good enough */
> - i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), "ds1338", 0x32);
> + i2c_create_slave(aspeed_i2c_get_bus(&soc->i2c, 11), "ds1338", 0x32);
>
> - smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), 0x51,
> + smbus_eeprom_init_one(aspeed_i2c_get_bus(&soc->i2c, 11), 0x51,
> eeprom_buf);
> 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),
> + i2c_realize_and_unref(dev, aspeed_i2c_get_bus(&soc->i2c, 11),
> &error_fatal);
> /* Bus 11: TODO ucd90160@64 */
> }
> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> index fb973a983d..518a3f5c6f 100644
> --- a/hw/i2c/aspeed_i2c.c
> +++ b/hw/i2c/aspeed_i2c.c
> @@ -959,9 +959,8 @@ static void aspeed_i2c_register_types(void)
> type_init(aspeed_i2c_register_types)
>
>
> -I2CBus *aspeed_i2c_get_bus(DeviceState *dev, int busnr)
> +I2CBus *aspeed_i2c_get_bus(AspeedI2CState *s, int busnr)
> {
> - AspeedI2CState *s = ASPEED_I2C(dev);
> AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(s);
> I2CBus *bus = NULL;
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 5/5] hw/i2c: Document the I2C qdev helpers
2020-06-30 13:16 ` Peter Maydell
@ 2020-07-14 7:05 ` Markus Armbruster
2020-07-14 9:32 ` Peter Maydell
0 siblings, 1 reply; 22+ messages in thread
From: Markus Armbruster @ 2020-07-14 7:05 UTC (permalink / raw)
To: Peter Maydell
Cc: Corey Minyard, Andrew Jeffery, Joel Stanley,
Philippe Mathieu-Daudé,
QEMU Developers, qemu-arm, qemu-ppc, Cédric Le Goater,
Jan Kiszka, David Gibson
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 :)
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 5/5] hw/i2c: Document the I2C qdev helpers
2020-07-14 7:05 ` Markus Armbruster
@ 2020-07-14 9:32 ` Peter Maydell
0 siblings, 0 replies; 22+ messages in thread
From: Peter Maydell @ 2020-07-14 9:32 UTC (permalink / raw)
To: Markus Armbruster
Cc: Corey Minyard, Andrew Jeffery, Joel Stanley,
Philippe Mathieu-Daudé,
QEMU Developers, qemu-arm, qemu-ppc, Cédric Le Goater,
Jan Kiszka, David Gibson
On Tue, 14 Jul 2020 at 08:06, Markus Armbruster <armbru@redhat.com> wrote:
> 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 :)
Yes, that's part of
https://patchew.org/QEMU/20200711142425.16283-1-peter.maydell@linaro.org/20200711142425.16283-2-peter.maydell@linaro.org/
thanks
-- PMM
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2020-07-14 9:33 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-29 17:38 [PATCH 0/5] hw/i2c: Rename method names for consistency and add documentation Philippe Mathieu-Daudé
2020-06-29 17:38 ` [PATCH 1/5] hw/i2c/aspeed_i2c: Simplify aspeed_i2c_get_bus() Philippe Mathieu-Daudé
2020-06-30 9:28 ` Markus Armbruster
2020-07-13 12:23 ` Cédric Le Goater
2020-06-29 17:38 ` [PATCH 2/5] hw/i2c: Rename i2c_try_create_slave() as i2c_slave_new() Philippe Mathieu-Daudé
2020-06-29 21:29 ` Corey Minyard
2020-06-29 21:37 ` BALATON Zoltan
2020-06-30 8:29 ` Philippe Mathieu-Daudé
2020-06-30 9:37 ` Markus Armbruster
2020-06-29 17:38 ` [PATCH 3/5] hw/i2c: Rename i2c_realize_and_unref() as i2c_slave_realize_and_unref() Philippe Mathieu-Daudé
2020-06-29 21:29 ` Corey Minyard
2020-06-29 17:38 ` [PATCH 4/5] hw/i2c: Rename i2c_create_slave() as i2c_slave_create_simple() Philippe Mathieu-Daudé
2020-06-29 21:29 ` Corey Minyard
2020-06-30 9:48 ` Markus Armbruster
2020-06-29 17:38 ` [PATCH 5/5] hw/i2c: Document the I2C qdev helpers Philippe Mathieu-Daudé
2020-06-29 21:30 ` Corey Minyard
2020-06-30 10:15 ` Markus Armbruster
2020-06-30 10:45 ` Philippe Mathieu-Daudé
2020-06-30 13:16 ` Peter Maydell
2020-07-14 7:05 ` Markus Armbruster
2020-07-14 9:32 ` Peter Maydell
2020-06-29 21:28 ` [PATCH 0/5] hw/i2c: Rename method names for consistency and add documentation Corey Minyard
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.