* [PATCH v5 01/15] hw/input/lm832x: Move lm832x_key_event() declaration to "lm832x.h"
2021-06-17 11:53 [PATCH v5 00/15] hw/i2c: Remove confusing i2c_send_recv() API Philippe Mathieu-Daudé
@ 2021-06-17 11:53 ` Philippe Mathieu-Daudé
2021-06-17 11:53 ` [PATCH v5 02/15] hw/input/lm832x: Define TYPE_LM8323 in public header Philippe Mathieu-Daudé
` (14 subsequent siblings)
15 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-06-17 11:53 UTC (permalink / raw)
To: qemu-devel; +Cc: Corey Minyard, Richard Henderson, Philippe Mathieu-Daudé
lm832x_key_event() is specific go LM832x devices, not to the
I2C bus API. Move it out of "i2c.h" to a new header.
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Acked-by: Corey Minyard <cminyard@mvista.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
include/hw/i2c/i2c.h | 3 ---
include/hw/input/lm832x.h | 26 ++++++++++++++++++++++++++
hw/arm/nseries.c | 1 +
hw/input/lm832x.c | 1 +
MAINTAINERS | 1 +
5 files changed, 29 insertions(+), 3 deletions(-)
create mode 100644 include/hw/input/lm832x.h
diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index ff4129ea704..850815e707c 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -142,9 +142,6 @@ 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 */
-void lm832x_key_event(DeviceState *dev, int key, int state);
-
extern const VMStateDescription vmstate_i2c_slave;
#define VMSTATE_I2C_SLAVE(_field, _state) { \
diff --git a/include/hw/input/lm832x.h b/include/hw/input/lm832x.h
new file mode 100644
index 00000000000..f47e579ff90
--- /dev/null
+++ b/include/hw/input/lm832x.h
@@ -0,0 +1,26 @@
+/*
+ * National Semiconductor LM8322/8323 GPIO keyboard & PWM chips.
+ *
+ * Copyright (C) 2008 Nokia Corporation
+ * Written by Andrzej Zaborowski <andrew@openedhand.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 or
+ * (at your option) version 3 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef HW_INPUT_LM832X
+#define HW_INPUT_LM832X
+
+void lm832x_key_event(DeviceState *dev, int key, int state);
+
+#endif
diff --git a/hw/arm/nseries.c b/hw/arm/nseries.c
index 0aefa5d0f3e..7b82b8682e8 100644
--- a/hw/arm/nseries.c
+++ b/hw/arm/nseries.c
@@ -34,6 +34,7 @@
#include "hw/boards.h"
#include "hw/i2c/i2c.h"
#include "hw/display/blizzard.h"
+#include "hw/input/lm832x.h"
#include "hw/input/tsc2xxx.h"
#include "hw/misc/cbus.h"
#include "hw/misc/tmp105.h"
diff --git a/hw/input/lm832x.c b/hw/input/lm832x.c
index 4cb1e9de01f..d2b92b457e3 100644
--- a/hw/input/lm832x.c
+++ b/hw/input/lm832x.c
@@ -19,6 +19,7 @@
*/
#include "qemu/osdep.h"
+#include "hw/input/lm832x.h"
#include "hw/i2c/i2c.h"
#include "hw/irq.h"
#include "migration/vmstate.h"
diff --git a/MAINTAINERS b/MAINTAINERS
index 636bf2f5365..c56bc112ccf 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -792,6 +792,7 @@ F: hw/input/tsc2005.c
F: hw/misc/cbus.c
F: hw/rtc/twl92230.c
F: include/hw/display/blizzard.h
+F: include/hw/input/lm832x.h
F: include/hw/input/tsc2xxx.h
F: include/hw/misc/cbus.h
F: tests/acceptance/machine_arm_n8x0.py
--
2.31.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v5 02/15] hw/input/lm832x: Define TYPE_LM8323 in public header
2021-06-17 11:53 [PATCH v5 00/15] hw/i2c: Remove confusing i2c_send_recv() API Philippe Mathieu-Daudé
2021-06-17 11:53 ` [PATCH v5 01/15] hw/input/lm832x: Move lm832x_key_event() declaration to "lm832x.h" Philippe Mathieu-Daudé
@ 2021-06-17 11:53 ` Philippe Mathieu-Daudé
2021-06-17 11:53 ` [PATCH v5 03/15] hw/display/sm501: Simplify sm501_i2c_write() logic Philippe Mathieu-Daudé
` (13 subsequent siblings)
15 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-06-17 11:53 UTC (permalink / raw)
To: qemu-devel; +Cc: Corey Minyard, Richard Henderson, Philippe Mathieu-Daudé
Define TYPE_LM8323 in the public "hw/input/lm832x.h"
header and use it in hw/arm/nseries.c.
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Acked-by: Corey Minyard <cminyard@mvista.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
include/hw/input/lm832x.h | 2 ++
hw/arm/nseries.c | 2 +-
hw/input/lm832x.c | 1 -
3 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/include/hw/input/lm832x.h b/include/hw/input/lm832x.h
index f47e579ff90..2a58ccf8916 100644
--- a/include/hw/input/lm832x.h
+++ b/include/hw/input/lm832x.h
@@ -21,6 +21,8 @@
#ifndef HW_INPUT_LM832X
#define HW_INPUT_LM832X
+#define TYPE_LM8323 "lm8323"
+
void lm832x_key_event(DeviceState *dev, int key, int state);
#endif
diff --git a/hw/arm/nseries.c b/hw/arm/nseries.c
index 7b82b8682e8..3a51264e3cf 100644
--- a/hw/arm/nseries.c
+++ b/hw/arm/nseries.c
@@ -417,7 +417,7 @@ 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 = DEVICE(i2c_slave_create_simple(omap_i2c_bus(s->mpu->i2c[0]),
- "lm8323", N810_LM8323_ADDR));
+ TYPE_LM8323, N810_LM8323_ADDR));
qdev_connect_gpio_out(s->kbd, 0, kbd_irq);
}
diff --git a/hw/input/lm832x.c b/hw/input/lm832x.c
index d2b92b457e3..19a646d9bb4 100644
--- a/hw/input/lm832x.c
+++ b/hw/input/lm832x.c
@@ -28,7 +28,6 @@
#include "ui/console.h"
#include "qom/object.h"
-#define TYPE_LM8323 "lm8323"
OBJECT_DECLARE_SIMPLE_TYPE(LM823KbdState, LM8323)
struct LM823KbdState {
--
2.31.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v5 03/15] hw/display/sm501: Simplify sm501_i2c_write() logic
2021-06-17 11:53 [PATCH v5 00/15] hw/i2c: Remove confusing i2c_send_recv() API Philippe Mathieu-Daudé
2021-06-17 11:53 ` [PATCH v5 01/15] hw/input/lm832x: Move lm832x_key_event() declaration to "lm832x.h" Philippe Mathieu-Daudé
2021-06-17 11:53 ` [PATCH v5 02/15] hw/input/lm832x: Define TYPE_LM8323 in public header Philippe Mathieu-Daudé
@ 2021-06-17 11:53 ` Philippe Mathieu-Daudé
2021-06-17 11:53 ` [PATCH v5 04/15] hw/display/sm501: Replace i2c_send_recv() by i2c_recv() & i2c_send() Philippe Mathieu-Daudé
` (12 subsequent siblings)
15 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-06-17 11:53 UTC (permalink / raw)
To: qemu-devel; +Cc: Corey Minyard, Richard Henderson, Philippe Mathieu-Daudé
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Acked-by: Corey Minyard <cminyard@mvista.com>
Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
hw/display/sm501.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 8789722ef27..f276276f7f1 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -1036,8 +1036,9 @@ static void sm501_i2c_write(void *opaque, hwaddr addr, uint64_t value,
int res = i2c_start_transfer(s->i2c_bus,
s->i2c_addr >> 1,
s->i2c_addr & 1);
- s->i2c_status |= (res ? SM501_I2C_STATUS_ERROR : 0);
- if (!res) {
+ if (res) {
+ s->i2c_status |= SM501_I2C_STATUS_ERROR;
+ } else {
int i;
for (i = 0; i <= s->i2c_byte_count; i++) {
res = i2c_send_recv(s->i2c_bus, &s->i2c_data[i],
--
2.31.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v5 04/15] hw/display/sm501: Replace i2c_send_recv() by i2c_recv() & i2c_send()
2021-06-17 11:53 [PATCH v5 00/15] hw/i2c: Remove confusing i2c_send_recv() API Philippe Mathieu-Daudé
` (2 preceding siblings ...)
2021-06-17 11:53 ` [PATCH v5 03/15] hw/display/sm501: Simplify sm501_i2c_write() logic Philippe Mathieu-Daudé
@ 2021-06-17 11:53 ` Philippe Mathieu-Daudé
2021-06-17 23:43 ` BALATON Zoltan
2021-06-17 11:53 ` [PATCH v5 05/15] hw/i2c/ppc4xx_i2c: Add reference to datasheet Philippe Mathieu-Daudé
` (11 subsequent siblings)
15 siblings, 1 reply; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-06-17 11:53 UTC (permalink / raw)
To: qemu-devel; +Cc: Corey Minyard, Richard Henderson, Philippe Mathieu-Daudé
Instead of using the confuse i2c_send_recv(), rewrite to directly
call i2c_recv() & i2c_send(), resulting in code easier to review.
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Acked-by: Corey Minyard <cminyard@mvista.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
hw/display/sm501.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index f276276f7f1..569661a0746 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -1033,17 +1033,18 @@ static void sm501_i2c_write(void *opaque, hwaddr addr, uint64_t value,
case SM501_I2C_CONTROL:
if (value & SM501_I2C_CONTROL_ENABLE) {
if (value & SM501_I2C_CONTROL_START) {
+ bool is_recv = s->i2c_addr & 1;
int res = i2c_start_transfer(s->i2c_bus,
s->i2c_addr >> 1,
- s->i2c_addr & 1);
+ is_recv);
if (res) {
s->i2c_status |= SM501_I2C_STATUS_ERROR;
} else {
int i;
for (i = 0; i <= s->i2c_byte_count; i++) {
- res = i2c_send_recv(s->i2c_bus, &s->i2c_data[i],
- !(s->i2c_addr & 1));
- if (res) {
+ if (is_recv) {
+ s->i2c_data[i] = i2c_recv(s->i2c_bus);
+ } else if (i2c_send(s->i2c_bus, s->i2c_data[i]) < 0) {
s->i2c_status |= SM501_I2C_STATUS_ERROR;
return;
}
--
2.31.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v5 04/15] hw/display/sm501: Replace i2c_send_recv() by i2c_recv() & i2c_send()
2021-06-17 11:53 ` [PATCH v5 04/15] hw/display/sm501: Replace i2c_send_recv() by i2c_recv() & i2c_send() Philippe Mathieu-Daudé
@ 2021-06-17 23:43 ` BALATON Zoltan
0 siblings, 0 replies; 21+ messages in thread
From: BALATON Zoltan @ 2021-06-17 23:43 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: Corey Minyard, Richard Henderson, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1910 bytes --]
On Thu, 17 Jun 2021, Philippe Mathieu-Daudé wrote:
> Instead of using the confuse i2c_send_recv(), rewrite to directly
> call i2c_recv() & i2c_send(), resulting in code easier to review.
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Acked-by: Corey Minyard <cminyard@mvista.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> hw/display/sm501.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index f276276f7f1..569661a0746 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -1033,17 +1033,18 @@ static void sm501_i2c_write(void *opaque, hwaddr addr, uint64_t value,
> case SM501_I2C_CONTROL:
> if (value & SM501_I2C_CONTROL_ENABLE) {
> if (value & SM501_I2C_CONTROL_START) {
> + bool is_recv = s->i2c_addr & 1;
> int res = i2c_start_transfer(s->i2c_bus,
> s->i2c_addr >> 1,
> - s->i2c_addr & 1);
> + is_recv);
> if (res) {
> s->i2c_status |= SM501_I2C_STATUS_ERROR;
> } else {
> int i;
> for (i = 0; i <= s->i2c_byte_count; i++) {
> - res = i2c_send_recv(s->i2c_bus, &s->i2c_data[i],
> - !(s->i2c_addr & 1));
> - if (res) {
> + if (is_recv) {
> + s->i2c_data[i] = i2c_recv(s->i2c_bus);
> + } else if (i2c_send(s->i2c_bus, s->i2c_data[i]) < 0) {
> s->i2c_status |= SM501_I2C_STATUS_ERROR;
> return;
> }
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v5 05/15] hw/i2c/ppc4xx_i2c: Add reference to datasheet
2021-06-17 11:53 [PATCH v5 00/15] hw/i2c: Remove confusing i2c_send_recv() API Philippe Mathieu-Daudé
` (3 preceding siblings ...)
2021-06-17 11:53 ` [PATCH v5 04/15] hw/display/sm501: Replace i2c_send_recv() by i2c_recv() & i2c_send() Philippe Mathieu-Daudé
@ 2021-06-17 11:53 ` Philippe Mathieu-Daudé
2021-06-17 11:53 ` [PATCH v5 06/15] hw/i2c/ppc4xx_i2c: Replace i2c_send_recv() by i2c_recv() & i2c_send() Philippe Mathieu-Daudé
` (10 subsequent siblings)
15 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-06-17 11:53 UTC (permalink / raw)
To: qemu-devel; +Cc: Corey Minyard, Richard Henderson, Philippe Mathieu-Daudé
It took me a while to find this model datasheet, since it is
an OCR scan. Add a reference to save other developers time.
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Acked-by: Corey Minyard <cminyard@mvista.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
hw/i2c/ppc4xx_i2c.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
index c0a8e045670..f4c5bc12d36 100644
--- a/hw/i2c/ppc4xx_i2c.c
+++ b/hw/i2c/ppc4xx_i2c.c
@@ -1,6 +1,8 @@
/*
* PPC4xx I2C controller emulation
*
+ * Documentation: PPC405GP User's Manual, Chapter 22. IIC Bus Interface
+ *
* Copyright (c) 2007 Jocelyn Mayer
* Copyright (c) 2012 François Revol
* Copyright (c) 2016-2018 BALATON Zoltan
--
2.31.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v5 06/15] hw/i2c/ppc4xx_i2c: Replace i2c_send_recv() by i2c_recv() & i2c_send()
2021-06-17 11:53 [PATCH v5 00/15] hw/i2c: Remove confusing i2c_send_recv() API Philippe Mathieu-Daudé
` (4 preceding siblings ...)
2021-06-17 11:53 ` [PATCH v5 05/15] hw/i2c/ppc4xx_i2c: Add reference to datasheet Philippe Mathieu-Daudé
@ 2021-06-17 11:53 ` Philippe Mathieu-Daudé
2021-06-17 23:45 ` BALATON Zoltan
2021-06-17 11:53 ` [PATCH v5 07/15] hw/misc/auxbus: Fix MOT/classic I2C mode Philippe Mathieu-Daudé
` (9 subsequent siblings)
15 siblings, 1 reply; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-06-17 11:53 UTC (permalink / raw)
To: qemu-devel; +Cc: Corey Minyard, Richard Henderson, Philippe Mathieu-Daudé
Instead of using the confuse i2c_send_recv(), rewrite to directly
call i2c_recv() & i2c_send(), resulting in code easier to review.
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Acked-by: Corey Minyard <cminyard@mvista.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
hw/i2c/ppc4xx_i2c.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
index f4c5bc12d36..75d50f15158 100644
--- a/hw/i2c/ppc4xx_i2c.c
+++ b/hw/i2c/ppc4xx_i2c.c
@@ -240,11 +240,14 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
i2c->sts &= ~IIC_STS_ERR;
}
}
- if (!(i2c->sts & IIC_STS_ERR) &&
- i2c_send_recv(i2c->bus, &i2c->mdata[i], !recv)) {
- i2c->sts |= IIC_STS_ERR;
- i2c->extsts |= IIC_EXTSTS_XFRA;
- break;
+ if (!(i2c->sts & IIC_STS_ERR)) {
+ if (recv) {
+ i2c->mdata[i] = i2c_recv(i2c->bus);
+ } else if (i2c_send(i2c->bus, i2c->mdata[i]) < 0) {
+ i2c->sts |= IIC_STS_ERR;
+ i2c->extsts |= IIC_EXTSTS_XFRA;
+ break;
+ }
}
if (value & IIC_CNTL_RPST || !(value & IIC_CNTL_CHT)) {
i2c_end_transfer(i2c->bus);
--
2.31.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v5 06/15] hw/i2c/ppc4xx_i2c: Replace i2c_send_recv() by i2c_recv() & i2c_send()
2021-06-17 11:53 ` [PATCH v5 06/15] hw/i2c/ppc4xx_i2c: Replace i2c_send_recv() by i2c_recv() & i2c_send() Philippe Mathieu-Daudé
@ 2021-06-17 23:45 ` BALATON Zoltan
0 siblings, 0 replies; 21+ messages in thread
From: BALATON Zoltan @ 2021-06-17 23:45 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: Corey Minyard, Richard Henderson, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1655 bytes --]
On Thu, 17 Jun 2021, Philippe Mathieu-Daudé wrote:
> Instead of using the confuse i2c_send_recv(), rewrite to directly
> call i2c_recv() & i2c_send(), resulting in code easier to review.
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Acked-by: Corey Minyard <cminyard@mvista.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> hw/i2c/ppc4xx_i2c.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
> index f4c5bc12d36..75d50f15158 100644
> --- a/hw/i2c/ppc4xx_i2c.c
> +++ b/hw/i2c/ppc4xx_i2c.c
> @@ -240,11 +240,14 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
> i2c->sts &= ~IIC_STS_ERR;
> }
> }
> - if (!(i2c->sts & IIC_STS_ERR) &&
> - i2c_send_recv(i2c->bus, &i2c->mdata[i], !recv)) {
> - i2c->sts |= IIC_STS_ERR;
> - i2c->extsts |= IIC_EXTSTS_XFRA;
> - break;
> + if (!(i2c->sts & IIC_STS_ERR)) {
> + if (recv) {
> + i2c->mdata[i] = i2c_recv(i2c->bus);
> + } else if (i2c_send(i2c->bus, i2c->mdata[i]) < 0) {
> + i2c->sts |= IIC_STS_ERR;
> + i2c->extsts |= IIC_EXTSTS_XFRA;
> + break;
> + }
> }
> if (value & IIC_CNTL_RPST || !(value & IIC_CNTL_CHT)) {
> i2c_end_transfer(i2c->bus);
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v5 07/15] hw/misc/auxbus: Fix MOT/classic I2C mode
2021-06-17 11:53 [PATCH v5 00/15] hw/i2c: Remove confusing i2c_send_recv() API Philippe Mathieu-Daudé
` (5 preceding siblings ...)
2021-06-17 11:53 ` [PATCH v5 06/15] hw/i2c/ppc4xx_i2c: Replace i2c_send_recv() by i2c_recv() & i2c_send() Philippe Mathieu-Daudé
@ 2021-06-17 11:53 ` Philippe Mathieu-Daudé
2021-06-17 11:53 ` [PATCH v5 08/15] hw/misc/auxbus: Explode READ_I2C / WRITE_I2C_MOT cases Philippe Mathieu-Daudé
` (8 subsequent siblings)
15 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-06-17 11:53 UTC (permalink / raw)
To: qemu-devel; +Cc: Corey Minyard, Richard Henderson, Philippe Mathieu-Daudé
Since its introduction in commit 6fc7f77fd2 i2c_start_transfer()
uses incorrectly the direction of the transfer (the last argument
is called 'is_recv'). Fix by inverting the argument, we now have
is_recv = !is_write.
Fixes: 6fc7f77fd2 ("introduce aux-bus")
Reported-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Acked-by: Corey Minyard <cminyard@mvista.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
hw/misc/auxbus.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/misc/auxbus.c b/hw/misc/auxbus.c
index 6c099ae2a2d..148b070ce4a 100644
--- a/hw/misc/auxbus.c
+++ b/hw/misc/auxbus.c
@@ -139,7 +139,7 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
i2c_end_transfer(i2c_bus);
}
- if (i2c_start_transfer(i2c_bus, address, is_write)) {
+ if (i2c_start_transfer(i2c_bus, address, !is_write)) {
ret = AUX_I2C_NACK;
break;
}
@@ -170,7 +170,7 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
/*
* No transactions started..
*/
- if (i2c_start_transfer(i2c_bus, address, is_write)) {
+ if (i2c_start_transfer(i2c_bus, address, !is_write)) {
break;
}
} else if ((address != bus->last_i2c_address) ||
@@ -179,7 +179,7 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
* Transaction started but we need to restart..
*/
i2c_end_transfer(i2c_bus);
- if (i2c_start_transfer(i2c_bus, address, is_write)) {
+ if (i2c_start_transfer(i2c_bus, address, !is_write)) {
break;
}
}
--
2.31.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v5 08/15] hw/misc/auxbus: Explode READ_I2C / WRITE_I2C_MOT cases
2021-06-17 11:53 [PATCH v5 00/15] hw/i2c: Remove confusing i2c_send_recv() API Philippe Mathieu-Daudé
` (6 preceding siblings ...)
2021-06-17 11:53 ` [PATCH v5 07/15] hw/misc/auxbus: Fix MOT/classic I2C mode Philippe Mathieu-Daudé
@ 2021-06-17 11:53 ` Philippe Mathieu-Daudé
2021-06-17 11:53 ` [PATCH v5 09/15] hw/misc/auxbus: Replace 'is_write' boolean by its value Philippe Mathieu-Daudé
` (7 subsequent siblings)
15 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-06-17 11:53 UTC (permalink / raw)
To: qemu-devel; +Cc: Corey Minyard, Richard Henderson, Philippe Mathieu-Daudé
To allow further simplifications in the following commits,
start copying WRITE_I2C code to the READ_I2C, and READ_I2C_MOT
to WRITE_I2C_MOT. No logical change.
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Acked-by: Corey Minyard <cminyard@mvista.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
hw/misc/auxbus.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 53 insertions(+)
diff --git a/hw/misc/auxbus.c b/hw/misc/auxbus.c
index 148b070ce4a..9cc9cf3be32 100644
--- a/hw/misc/auxbus.c
+++ b/hw/misc/auxbus.c
@@ -133,6 +133,26 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
* Classic I2C transactions..
*/
case READ_I2C:
+ is_write = cmd == READ_I2C ? false : true;
+ if (i2c_bus_busy(i2c_bus)) {
+ i2c_end_transfer(i2c_bus);
+ }
+
+ if (i2c_start_transfer(i2c_bus, address, !is_write)) {
+ ret = AUX_I2C_NACK;
+ break;
+ }
+
+ ret = AUX_I2C_ACK;
+ while (len > 0) {
+ if (i2c_send_recv(i2c_bus, data++, is_write) < 0) {
+ ret = AUX_I2C_NACK;
+ break;
+ }
+ len--;
+ }
+ i2c_end_transfer(i2c_bus);
+ break;
case WRITE_I2C:
is_write = cmd == READ_I2C ? false : true;
if (i2c_bus_busy(i2c_bus)) {
@@ -163,6 +183,39 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
* - We changed the address.
*/
case WRITE_I2C_MOT:
+ is_write = cmd == READ_I2C_MOT ? false : true;
+ ret = AUX_I2C_NACK;
+ if (!i2c_bus_busy(i2c_bus)) {
+ /*
+ * No transactions started..
+ */
+ if (i2c_start_transfer(i2c_bus, address, !is_write)) {
+ break;
+ }
+ } else if ((address != bus->last_i2c_address) ||
+ (bus->last_transaction != cmd)) {
+ /*
+ * Transaction started but we need to restart..
+ */
+ i2c_end_transfer(i2c_bus);
+ if (i2c_start_transfer(i2c_bus, address, !is_write)) {
+ break;
+ }
+ }
+
+ bus->last_transaction = cmd;
+ bus->last_i2c_address = address;
+ while (len > 0) {
+ if (i2c_send_recv(i2c_bus, data++, is_write) < 0) {
+ i2c_end_transfer(i2c_bus);
+ break;
+ }
+ len--;
+ }
+ if (len == 0) {
+ ret = AUX_I2C_ACK;
+ }
+ break;
case READ_I2C_MOT:
is_write = cmd == READ_I2C_MOT ? false : true;
ret = AUX_I2C_NACK;
--
2.31.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v5 09/15] hw/misc/auxbus: Replace 'is_write' boolean by its value
2021-06-17 11:53 [PATCH v5 00/15] hw/i2c: Remove confusing i2c_send_recv() API Philippe Mathieu-Daudé
` (7 preceding siblings ...)
2021-06-17 11:53 ` [PATCH v5 08/15] hw/misc/auxbus: Explode READ_I2C / WRITE_I2C_MOT cases Philippe Mathieu-Daudé
@ 2021-06-17 11:53 ` Philippe Mathieu-Daudé
2021-06-17 11:53 ` [PATCH v5 10/15] hw/misc/auxbus: Replace i2c_send_recv() by i2c_recv() & i2c_send() Philippe Mathieu-Daudé
` (6 subsequent siblings)
15 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-06-17 11:53 UTC (permalink / raw)
To: qemu-devel; +Cc: Corey Minyard, Richard Henderson, Philippe Mathieu-Daudé
Remove the 'is_write' boolean by directly using its value in place.
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Acked-by: Corey Minyard <cminyard@mvista.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
hw/misc/auxbus.c | 28 +++++++++++-----------------
1 file changed, 11 insertions(+), 17 deletions(-)
diff --git a/hw/misc/auxbus.c b/hw/misc/auxbus.c
index 9cc9cf3be32..d96219aef88 100644
--- a/hw/misc/auxbus.c
+++ b/hw/misc/auxbus.c
@@ -106,7 +106,6 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
AUXReply ret = AUX_NACK;
I2CBus *i2c_bus = aux_get_i2c_bus(bus);
size_t i;
- bool is_write = false;
DPRINTF("request at address 0x%" PRIX32 ", command %u, len %u\n", address,
cmd, len);
@@ -117,11 +116,10 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
*/
case WRITE_AUX:
case READ_AUX:
- is_write = cmd == READ_AUX ? false : true;
for (i = 0; i < len; i++) {
if (!address_space_rw(&bus->aux_addr_space, address++,
MEMTXATTRS_UNSPECIFIED, data++, 1,
- is_write)) {
+ cmd == WRITE_AUX)) {
ret = AUX_I2C_ACK;
} else {
ret = AUX_NACK;
@@ -133,19 +131,18 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
* Classic I2C transactions..
*/
case READ_I2C:
- is_write = cmd == READ_I2C ? false : true;
if (i2c_bus_busy(i2c_bus)) {
i2c_end_transfer(i2c_bus);
}
- if (i2c_start_transfer(i2c_bus, address, !is_write)) {
+ if (i2c_start_transfer(i2c_bus, address, true)) {
ret = AUX_I2C_NACK;
break;
}
ret = AUX_I2C_ACK;
while (len > 0) {
- if (i2c_send_recv(i2c_bus, data++, is_write) < 0) {
+ if (i2c_send_recv(i2c_bus, data++, false) < 0) {
ret = AUX_I2C_NACK;
break;
}
@@ -154,19 +151,18 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
i2c_end_transfer(i2c_bus);
break;
case WRITE_I2C:
- is_write = cmd == READ_I2C ? false : true;
if (i2c_bus_busy(i2c_bus)) {
i2c_end_transfer(i2c_bus);
}
- if (i2c_start_transfer(i2c_bus, address, !is_write)) {
+ if (i2c_start_transfer(i2c_bus, address, false)) {
ret = AUX_I2C_NACK;
break;
}
ret = AUX_I2C_ACK;
while (len > 0) {
- if (i2c_send_recv(i2c_bus, data++, is_write) < 0) {
+ if (i2c_send_recv(i2c_bus, data++, true) < 0) {
ret = AUX_I2C_NACK;
break;
}
@@ -183,13 +179,12 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
* - We changed the address.
*/
case WRITE_I2C_MOT:
- is_write = cmd == READ_I2C_MOT ? false : true;
ret = AUX_I2C_NACK;
if (!i2c_bus_busy(i2c_bus)) {
/*
* No transactions started..
*/
- if (i2c_start_transfer(i2c_bus, address, !is_write)) {
+ if (i2c_start_transfer(i2c_bus, address, false)) {
break;
}
} else if ((address != bus->last_i2c_address) ||
@@ -198,7 +193,7 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
* Transaction started but we need to restart..
*/
i2c_end_transfer(i2c_bus);
- if (i2c_start_transfer(i2c_bus, address, !is_write)) {
+ if (i2c_start_transfer(i2c_bus, address, false)) {
break;
}
}
@@ -206,7 +201,7 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
bus->last_transaction = cmd;
bus->last_i2c_address = address;
while (len > 0) {
- if (i2c_send_recv(i2c_bus, data++, is_write) < 0) {
+ if (i2c_send_recv(i2c_bus, data++, true) < 0) {
i2c_end_transfer(i2c_bus);
break;
}
@@ -217,13 +212,12 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
}
break;
case READ_I2C_MOT:
- is_write = cmd == READ_I2C_MOT ? false : true;
ret = AUX_I2C_NACK;
if (!i2c_bus_busy(i2c_bus)) {
/*
* No transactions started..
*/
- if (i2c_start_transfer(i2c_bus, address, !is_write)) {
+ if (i2c_start_transfer(i2c_bus, address, true)) {
break;
}
} else if ((address != bus->last_i2c_address) ||
@@ -232,7 +226,7 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
* Transaction started but we need to restart..
*/
i2c_end_transfer(i2c_bus);
- if (i2c_start_transfer(i2c_bus, address, !is_write)) {
+ if (i2c_start_transfer(i2c_bus, address, true)) {
break;
}
}
@@ -240,7 +234,7 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
bus->last_transaction = cmd;
bus->last_i2c_address = address;
while (len > 0) {
- if (i2c_send_recv(i2c_bus, data++, is_write) < 0) {
+ if (i2c_send_recv(i2c_bus, data++, false) < 0) {
i2c_end_transfer(i2c_bus);
break;
}
--
2.31.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v5 10/15] hw/misc/auxbus: Replace i2c_send_recv() by i2c_recv() & i2c_send()
2021-06-17 11:53 [PATCH v5 00/15] hw/i2c: Remove confusing i2c_send_recv() API Philippe Mathieu-Daudé
` (8 preceding siblings ...)
2021-06-17 11:53 ` [PATCH v5 09/15] hw/misc/auxbus: Replace 'is_write' boolean by its value Philippe Mathieu-Daudé
@ 2021-06-17 11:53 ` Philippe Mathieu-Daudé
2021-06-17 11:53 ` [PATCH v5 11/15] hw/i2c: Remove confusing i2c_send_recv() Philippe Mathieu-Daudé
` (5 subsequent siblings)
15 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-06-17 11:53 UTC (permalink / raw)
To: qemu-devel; +Cc: Corey Minyard, Richard Henderson, Philippe Mathieu-Daudé
Instead of using the confuse i2c_send_recv(), replace
i2c_send_recv(send = true) by i2c_send() and
i2c_send_recv(send = false) by i2c_recv().
During the replacement we also change a while() statement by for().
The resulting code is easier to review.
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Acked-by: Corey Minyard <cminyard@mvista.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
hw/misc/auxbus.c | 35 +++++++++++------------------------
1 file changed, 11 insertions(+), 24 deletions(-)
diff --git a/hw/misc/auxbus.c b/hw/misc/auxbus.c
index d96219aef88..44aa9730bc9 100644
--- a/hw/misc/auxbus.c
+++ b/hw/misc/auxbus.c
@@ -141,12 +141,8 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
}
ret = AUX_I2C_ACK;
- while (len > 0) {
- if (i2c_send_recv(i2c_bus, data++, false) < 0) {
- ret = AUX_I2C_NACK;
- break;
- }
- len--;
+ for (i = 0; i < len; i++) {
+ data[i] = i2c_recv(i2c_bus);
}
i2c_end_transfer(i2c_bus);
break;
@@ -161,12 +157,11 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
}
ret = AUX_I2C_ACK;
- while (len > 0) {
- if (i2c_send_recv(i2c_bus, data++, true) < 0) {
+ for (i = 0; i < len; i++) {
+ if (i2c_send(i2c_bus, data[i]) < 0) {
ret = AUX_I2C_NACK;
break;
}
- len--;
}
i2c_end_transfer(i2c_bus);
break;
@@ -200,15 +195,13 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
bus->last_transaction = cmd;
bus->last_i2c_address = address;
- while (len > 0) {
- if (i2c_send_recv(i2c_bus, data++, true) < 0) {
+ ret = AUX_I2C_ACK;
+ for (i = 0; i < len; i++) {
+ if (i2c_send(i2c_bus, data[i]) < 0) {
i2c_end_transfer(i2c_bus);
+ ret = AUX_I2C_NACK;
break;
}
- len--;
- }
- if (len == 0) {
- ret = AUX_I2C_ACK;
}
break;
case READ_I2C_MOT:
@@ -233,16 +226,10 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
bus->last_transaction = cmd;
bus->last_i2c_address = address;
- while (len > 0) {
- if (i2c_send_recv(i2c_bus, data++, false) < 0) {
- i2c_end_transfer(i2c_bus);
- break;
- }
- len--;
- }
- if (len == 0) {
- ret = AUX_I2C_ACK;
+ for (i = 0; i < len; i++) {
+ data[i] = i2c_recv(i2c_bus);
}
+ ret = AUX_I2C_ACK;
break;
default:
qemu_log_mask(LOG_UNIMP, "AUX cmd=%u not implemented\n", cmd);
--
2.31.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v5 11/15] hw/i2c: Remove confusing i2c_send_recv()
2021-06-17 11:53 [PATCH v5 00/15] hw/i2c: Remove confusing i2c_send_recv() API Philippe Mathieu-Daudé
` (9 preceding siblings ...)
2021-06-17 11:53 ` [PATCH v5 10/15] hw/misc/auxbus: Replace i2c_send_recv() by i2c_recv() & i2c_send() Philippe Mathieu-Daudé
@ 2021-06-17 11:53 ` Philippe Mathieu-Daudé
2021-06-17 11:53 ` [PATCH v5 12/15] hw/i2c: Rename i2c_set_slave_address() -> i2c_slave_set_address() Philippe Mathieu-Daudé
` (4 subsequent siblings)
15 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-06-17 11:53 UTC (permalink / raw)
To: qemu-devel; +Cc: Corey Minyard, Richard Henderson, Philippe Mathieu-Daudé
We replaced all the i2c_send_recv() calls by the clearer i2c_recv()
and i2c_send(), so we can remove this confusing API.
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Acked-by: Corey Minyard <cminyard@mvista.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
include/hw/i2c/i2c.h | 1 -
hw/i2c/core.c | 50 +++++++++++++++++++-------------------------
2 files changed, 21 insertions(+), 30 deletions(-)
diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index 850815e707c..99635b837a5 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -84,7 +84,6 @@ int i2c_bus_busy(I2CBus *bus);
int i2c_start_transfer(I2CBus *bus, uint8_t address, int recv);
void i2c_end_transfer(I2CBus *bus);
void i2c_nack(I2CBus *bus);
-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);
bool i2c_scan_bus(I2CBus *bus, uint8_t address, bool broadcast,
diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index 3a7bae311df..27a66df7f34 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -188,50 +188,42 @@ void i2c_end_transfer(I2CBus *bus)
bus->broadcast = false;
}
-int i2c_send_recv(I2CBus *bus, uint8_t *data, bool send)
+int i2c_send(I2CBus *bus, uint8_t data)
{
I2CSlaveClass *sc;
I2CSlave *s;
I2CNode *node;
int ret = 0;
- if (send) {
- QLIST_FOREACH(node, &bus->current_devs, next) {
- s = node->elt;
- sc = I2C_SLAVE_GET_CLASS(s);
- if (sc->send) {
- trace_i2c_send(s->address, *data);
- ret = ret || sc->send(s, *data);
- } else {
- ret = -1;
- }
+ QLIST_FOREACH(node, &bus->current_devs, next) {
+ s = node->elt;
+ sc = I2C_SLAVE_GET_CLASS(s);
+ if (sc->send) {
+ trace_i2c_send(s->address, data);
+ ret = ret || sc->send(s, data);
+ } else {
+ ret = -1;
}
- return ret ? -1 : 0;
- } else {
- ret = 0xff;
- if (!QLIST_EMPTY(&bus->current_devs) && !bus->broadcast) {
- sc = I2C_SLAVE_GET_CLASS(QLIST_FIRST(&bus->current_devs)->elt);
- if (sc->recv) {
- s = QLIST_FIRST(&bus->current_devs)->elt;
- ret = sc->recv(s);
- trace_i2c_recv(s->address, ret);
- }
- }
- *data = ret;
- return 0;
}
-}
-int i2c_send(I2CBus *bus, uint8_t data)
-{
- return i2c_send_recv(bus, &data, true);
+ return ret ? -1 : 0;
}
uint8_t i2c_recv(I2CBus *bus)
{
uint8_t data = 0xff;
+ I2CSlaveClass *sc;
+ I2CSlave *s;
+
+ if (!QLIST_EMPTY(&bus->current_devs) && !bus->broadcast) {
+ sc = I2C_SLAVE_GET_CLASS(QLIST_FIRST(&bus->current_devs)->elt);
+ if (sc->recv) {
+ s = QLIST_FIRST(&bus->current_devs)->elt;
+ data = sc->recv(s);
+ trace_i2c_recv(s->address, data);
+ }
+ }
- i2c_send_recv(bus, &data, false);
return data;
}
--
2.31.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v5 12/15] hw/i2c: Rename i2c_set_slave_address() -> i2c_slave_set_address()
2021-06-17 11:53 [PATCH v5 00/15] hw/i2c: Remove confusing i2c_send_recv() API Philippe Mathieu-Daudé
` (10 preceding siblings ...)
2021-06-17 11:53 ` [PATCH v5 11/15] hw/i2c: Remove confusing i2c_send_recv() Philippe Mathieu-Daudé
@ 2021-06-17 11:53 ` Philippe Mathieu-Daudé
2021-06-17 11:53 ` [PATCH v5 13/15] hw/i2c: Make i2c_start_transfer() direction argument a boolean Philippe Mathieu-Daudé
` (3 subsequent siblings)
15 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-06-17 11:53 UTC (permalink / raw)
To: qemu-devel; +Cc: Corey Minyard, Richard Henderson, Philippe Mathieu-Daudé
Other functions from I2C slave API are named "i2c_slave_XXX()".
Follow that pattern with set_address(). Add docstring along.
No logical change.
Patch created mechanically using:
$ sed -i s/i2c_set_slave_address/i2c_slave_set_address/ \
$(git grep -l i2c_set_slave_address)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Acked-by: Corey Minyard <cminyard@mvista.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
include/hw/i2c/i2c.h | 8 +++++++-
hw/arm/pxa2xx.c | 2 +-
hw/arm/spitz.c | 4 ++--
hw/display/ati.c | 2 +-
hw/display/sm501.c | 2 +-
hw/display/xlnx_dp.c | 2 +-
hw/i2c/core.c | 2 +-
hw/i2c/imx_i2c.c | 2 +-
8 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index 99635b837a5..2adf521b271 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -79,7 +79,6 @@ struct I2CBus {
};
I2CBus *i2c_init_bus(DeviceState *parent, const char *name);
-void i2c_set_slave_address(I2CSlave *dev, uint8_t address);
int i2c_bus_busy(I2CBus *bus);
int i2c_start_transfer(I2CBus *bus, uint8_t address, int recv);
void i2c_end_transfer(I2CBus *bus);
@@ -141,6 +140,13 @@ 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);
+/**
+ * Set the I2C bus address of a slave device
+ * @dev: I2C slave device
+ * @address: I2C address of the slave when put on a bus
+ */
+void i2c_slave_set_address(I2CSlave *dev, uint8_t address);
+
extern const VMStateDescription vmstate_i2c_slave;
#define VMSTATE_I2C_SLAVE(_field, _state) { \
diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
index fdc4955e95b..15a247efae2 100644
--- a/hw/arm/pxa2xx.c
+++ b/hw/arm/pxa2xx.c
@@ -1437,7 +1437,7 @@ static void pxa2xx_i2c_write(void *opaque, hwaddr addr,
break;
case ISAR:
- i2c_set_slave_address(I2C_SLAVE(s->slave), value & 0x7f);
+ i2c_slave_set_address(I2C_SLAVE(s->slave), value & 0x7f);
break;
case IDBR:
diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
index b45a929cbd9..c0f0f8193f4 100644
--- a/hw/arm/spitz.c
+++ b/hw/arm/spitz.c
@@ -769,9 +769,9 @@ static void spitz_wm8750_addr(void *opaque, int line, int level)
{
I2CSlave *wm = (I2CSlave *) opaque;
if (level)
- i2c_set_slave_address(wm, SPITZ_WM_ADDRH);
+ i2c_slave_set_address(wm, SPITZ_WM_ADDRH);
else
- i2c_set_slave_address(wm, SPITZ_WM_ADDRL);
+ i2c_slave_set_address(wm, SPITZ_WM_ADDRL);
}
static void spitz_i2c_setup(PXA2xxState *cpu)
diff --git a/hw/display/ati.c b/hw/display/ati.c
index 4c3ad8f47b0..31f22754dce 100644
--- a/hw/display/ati.c
+++ b/hw/display/ati.c
@@ -968,7 +968,7 @@ static void ati_vga_realize(PCIDevice *dev, Error **errp)
I2CBus *i2cbus = i2c_init_bus(DEVICE(s), "ati-vga.ddc");
bitbang_i2c_init(&s->bbi2c, i2cbus);
I2CSlave *i2cddc = I2C_SLAVE(qdev_new(TYPE_I2CDDC));
- i2c_set_slave_address(i2cddc, 0x50);
+ i2c_slave_set_address(i2cddc, 0x50);
qdev_realize_and_unref(DEVICE(i2cddc), BUS(i2cbus), &error_abort);
/* mmio register space */
diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 569661a0746..663c37e7f28 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -1828,7 +1828,7 @@ static void sm501_init(SM501State *s, DeviceState *dev,
s->i2c_bus = i2c_init_bus(dev, "sm501.i2c");
/* ddc */
I2CDDCState *ddc = I2CDDC(qdev_new(TYPE_I2CDDC));
- i2c_set_slave_address(I2C_SLAVE(ddc), 0x50);
+ i2c_slave_set_address(I2C_SLAVE(ddc), 0x50);
qdev_realize_and_unref(DEVICE(ddc), BUS(s->i2c_bus), &error_abort);
/* mmio */
diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c
index 4fd6aeb18b5..2bb7a5441ad 100644
--- a/hw/display/xlnx_dp.c
+++ b/hw/display/xlnx_dp.c
@@ -1253,7 +1253,7 @@ static void xlnx_dp_init(Object *obj)
object_property_add_child(OBJECT(s), "dpcd", OBJECT(s->dpcd));
s->edid = I2CDDC(qdev_new("i2c-ddc"));
- i2c_set_slave_address(I2C_SLAVE(s->edid), 0x50);
+ i2c_slave_set_address(I2C_SLAVE(s->edid), 0x50);
object_property_add_child(OBJECT(s), "edid", OBJECT(s->edid));
fifo8_create(&s->rx_fifo, 16);
diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index 27a66df7f34..6af24c9e797 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -66,7 +66,7 @@ I2CBus *i2c_init_bus(DeviceState *parent, const char *name)
return bus;
}
-void i2c_set_slave_address(I2CSlave *dev, uint8_t address)
+void i2c_slave_set_address(I2CSlave *dev, uint8_t address)
{
dev->address = address;
}
diff --git a/hw/i2c/imx_i2c.c b/hw/i2c/imx_i2c.c
index 2e02e1c4faa..9792583fea7 100644
--- a/hw/i2c/imx_i2c.c
+++ b/hw/i2c/imx_i2c.c
@@ -171,7 +171,7 @@ static void imx_i2c_write(void *opaque, hwaddr offset,
switch (offset) {
case IADR_ADDR:
s->iadr = value & IADR_MASK;
- /* i2c_set_slave_address(s->bus, (uint8_t)s->iadr); */
+ /* i2c_slave_set_address(s->bus, (uint8_t)s->iadr); */
break;
case IFDR_ADDR:
s->ifdr = value & IFDR_MASK;
--
2.31.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v5 13/15] hw/i2c: Make i2c_start_transfer() direction argument a boolean
2021-06-17 11:53 [PATCH v5 00/15] hw/i2c: Remove confusing i2c_send_recv() API Philippe Mathieu-Daudé
` (11 preceding siblings ...)
2021-06-17 11:53 ` [PATCH v5 12/15] hw/i2c: Rename i2c_set_slave_address() -> i2c_slave_set_address() Philippe Mathieu-Daudé
@ 2021-06-17 11:53 ` Philippe Mathieu-Daudé
2021-06-17 11:53 ` [PATCH v5 14/15] hw/i2c: Extract i2c_do_start_transfer() from i2c_start_transfer() Philippe Mathieu-Daudé
` (2 subsequent siblings)
15 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-06-17 11:53 UTC (permalink / raw)
To: qemu-devel; +Cc: Corey Minyard, Richard Henderson, Philippe Mathieu-Daudé
From: BALATON Zoltan <balaton@eik.bme.hu>
Make the argument representing the direction of the transfer a
boolean type.
Rename the boolean argument as 'is_recv' to match i2c_recv_send().
Document the function prototype.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Message-Id: <20200621145235.9E241745712@zero.eik.bme.hu>
[PMD: Split patch, added docstring]
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Acked-by: Corey Minyard <cminyard@mvista.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
include/hw/i2c/i2c.h | 12 +++++++++++-
hw/i2c/core.c | 4 ++--
2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index 2adf521b271..21f2dba1bf7 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -80,7 +80,17 @@ struct I2CBus {
I2CBus *i2c_init_bus(DeviceState *parent, const char *name);
int i2c_bus_busy(I2CBus *bus);
-int i2c_start_transfer(I2CBus *bus, uint8_t address, int recv);
+
+/**
+ * i2c_start_transfer: start a transfer on an I2C bus.
+ *
+ * @bus: #I2CBus to be used
+ * @address: address of the slave
+ * @is_recv: indicates the transfer direction
+ *
+ * Returns: 0 on success, -1 on error
+ */
+int i2c_start_transfer(I2CBus *bus, uint8_t address, bool is_recv);
void i2c_end_transfer(I2CBus *bus);
void i2c_nack(I2CBus *bus);
int i2c_send(I2CBus *bus, uint8_t data);
diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index 6af24c9e797..6639ca8c2e0 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -115,7 +115,7 @@ bool i2c_scan_bus(I2CBus *bus, uint8_t address, bool broadcast,
* without releasing the bus. If that fails, the bus is still
* in a transaction.
*/
-int i2c_start_transfer(I2CBus *bus, uint8_t address, int recv)
+int i2c_start_transfer(I2CBus *bus, uint8_t address, bool is_recv)
{
I2CSlaveClass *sc;
I2CNode *node;
@@ -157,7 +157,7 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address, int recv)
if (sc->event) {
trace_i2c_event("start", s->address);
- rv = sc->event(s, recv ? I2C_START_RECV : I2C_START_SEND);
+ rv = sc->event(s, is_recv ? I2C_START_RECV : I2C_START_SEND);
if (rv && !bus->broadcast) {
if (bus_scanned) {
/* First call, terminate the transfer. */
--
2.31.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v5 14/15] hw/i2c: Extract i2c_do_start_transfer() from i2c_start_transfer()
2021-06-17 11:53 [PATCH v5 00/15] hw/i2c: Remove confusing i2c_send_recv() API Philippe Mathieu-Daudé
` (12 preceding siblings ...)
2021-06-17 11:53 ` [PATCH v5 13/15] hw/i2c: Make i2c_start_transfer() direction argument a boolean Philippe Mathieu-Daudé
@ 2021-06-17 11:53 ` Philippe Mathieu-Daudé
2021-06-17 23:57 ` BALATON Zoltan
2021-06-17 11:53 ` [PATCH v5 15/15] hw/i2c: Introduce i2c_start_recv() and i2c_start_send() Philippe Mathieu-Daudé
2021-06-17 13:16 ` [PATCH v5 00/15] hw/i2c: Remove confusing i2c_send_recv() API Corey Minyard
15 siblings, 1 reply; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-06-17 11:53 UTC (permalink / raw)
To: qemu-devel; +Cc: Corey Minyard, Richard Henderson, Philippe Mathieu-Daudé
To allow further simplications, extract i2c_do_start_transfer()
from i2c_start_transfer(). This is mostly the same function,
but the former is static and takes an enum argument.
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
hw/i2c/core.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index 6639ca8c2e0..5483bf95a3e 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -114,8 +114,11 @@ bool i2c_scan_bus(I2CBus *bus, uint8_t address, bool broadcast,
* protocol uses a start transfer to switch from write to read mode
* without releasing the bus. If that fails, the bus is still
* in a transaction.
+ *
+ * @event must be I2C_START_RECV or I2C_START_SEND.
*/
-int i2c_start_transfer(I2CBus *bus, uint8_t address, bool is_recv)
+static int i2c_do_start_transfer(I2CBus *bus, uint8_t address,
+ enum i2c_event event)
{
I2CSlaveClass *sc;
I2CNode *node;
@@ -157,7 +160,7 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address, bool is_recv)
if (sc->event) {
trace_i2c_event("start", s->address);
- rv = sc->event(s, is_recv ? I2C_START_RECV : I2C_START_SEND);
+ rv = sc->event(s, event);
if (rv && !bus->broadcast) {
if (bus_scanned) {
/* First call, terminate the transfer. */
@@ -170,6 +173,13 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address, bool is_recv)
return 0;
}
+int i2c_start_transfer(I2CBus *bus, uint8_t address, bool is_recv)
+{
+ return i2c_do_start_transfer(bus, address, is_recv
+ ? I2C_START_RECV
+ : I2C_START_SEND);
+}
+
void i2c_end_transfer(I2CBus *bus)
{
I2CSlaveClass *sc;
--
2.31.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v5 14/15] hw/i2c: Extract i2c_do_start_transfer() from i2c_start_transfer()
2021-06-17 11:53 ` [PATCH v5 14/15] hw/i2c: Extract i2c_do_start_transfer() from i2c_start_transfer() Philippe Mathieu-Daudé
@ 2021-06-17 23:57 ` BALATON Zoltan
0 siblings, 0 replies; 21+ messages in thread
From: BALATON Zoltan @ 2021-06-17 23:57 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: Corey Minyard, Richard Henderson, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2677 bytes --]
On Thu, 17 Jun 2021, Philippe Mathieu-Daudé wrote:
> To allow further simplications, extract i2c_do_start_transfer()
> from i2c_start_transfer(). This is mostly the same function,
> but the former is static and takes an enum argument.
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> hw/i2c/core.c | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/hw/i2c/core.c b/hw/i2c/core.c
> index 6639ca8c2e0..5483bf95a3e 100644
> --- a/hw/i2c/core.c
> +++ b/hw/i2c/core.c
> @@ -114,8 +114,11 @@ bool i2c_scan_bus(I2CBus *bus, uint8_t address, bool broadcast,
> * protocol uses a start transfer to switch from write to read mode
> * without releasing the bus. If that fails, the bus is still
> * in a transaction.
> + *
> + * @event must be I2C_START_RECV or I2C_START_SEND.
> */
> -int i2c_start_transfer(I2CBus *bus, uint8_t address, bool is_recv)
> +static int i2c_do_start_transfer(I2CBus *bus, uint8_t address,
> + enum i2c_event event)
> {
> I2CSlaveClass *sc;
> I2CNode *node;
> @@ -157,7 +160,7 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address, bool is_recv)
>
> if (sc->event) {
> trace_i2c_event("start", s->address);
> - rv = sc->event(s, is_recv ? I2C_START_RECV : I2C_START_SEND);
> + rv = sc->event(s, event);
> if (rv && !bus->broadcast) {
> if (bus_scanned) {
> /* First call, terminate the transfer. */
> @@ -170,6 +173,13 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address, bool is_recv)
> return 0;
> }
>
> +int i2c_start_transfer(I2CBus *bus, uint8_t address, bool is_recv)
> +{
> + return i2c_do_start_transfer(bus, address, is_recv
> + ? I2C_START_RECV
> + : I2C_START_SEND);
> +}
> +
Why is this better than keeping the original function and just add
wrappers calling that for i2c_start_send/start_recv? So you could just
drop this patch and change the next one to call i2c_start_transfer(bus,
addr, true) for i2c_start_recv and with false for start_send. This seems
to add another variant for no good reason. If the enum would only have
these two values then that could be an improvement (although the function
is static so nothing else could use it) but it's actually reusing an enum
with other values so this does not seem to make it much better. Then you
could just keep the existing function.
Regards,
BALATON Zoltan
> void i2c_end_transfer(I2CBus *bus)
> {
> I2CSlaveClass *sc;
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v5 15/15] hw/i2c: Introduce i2c_start_recv() and i2c_start_send()
2021-06-17 11:53 [PATCH v5 00/15] hw/i2c: Remove confusing i2c_send_recv() API Philippe Mathieu-Daudé
` (13 preceding siblings ...)
2021-06-17 11:53 ` [PATCH v5 14/15] hw/i2c: Extract i2c_do_start_transfer() from i2c_start_transfer() Philippe Mathieu-Daudé
@ 2021-06-17 11:53 ` Philippe Mathieu-Daudé
2021-06-18 0:27 ` BALATON Zoltan
2021-06-17 13:16 ` [PATCH v5 00/15] hw/i2c: Remove confusing i2c_send_recv() API Corey Minyard
15 siblings, 1 reply; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-06-17 11:53 UTC (permalink / raw)
To: qemu-devel; +Cc: Corey Minyard, Richard Henderson, Philippe Mathieu-Daudé
To ease reviewing code using the I2C bus API, introduce the
i2c_start_recv() and i2c_start_send() helpers which don't
take the confusing 'is_recv' boolean argument.
Use these new helpers in the SMBus / AUX bus models.
Suggested-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
include/hw/i2c/i2c.h | 24 ++++++++++++++++++++++++
hw/i2c/core.c | 10 ++++++++++
hw/i2c/pm_smbus.c | 4 ++--
hw/i2c/smbus_master.c | 22 +++++++++++-----------
hw/misc/auxbus.c | 12 ++++++------
5 files changed, 53 insertions(+), 19 deletions(-)
diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index 21f2dba1bf7..5ca3b708c0b 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -88,9 +88,33 @@ int i2c_bus_busy(I2CBus *bus);
* @address: address of the slave
* @is_recv: indicates the transfer direction
*
+ * When @is_recv is a known boolean constant, use the
+ * i2c_start_recv() or i2c_start_send() helper instead.
+ *
* Returns: 0 on success, -1 on error
*/
int i2c_start_transfer(I2CBus *bus, uint8_t address, bool is_recv);
+
+/**
+ * i2c_start_recv: start a 'receive' transfer on an I2C bus.
+ *
+ * @bus: #I2CBus to be used
+ * @address: address of the slave
+ *
+ * Returns: 0 on success, -1 on error
+ */
+int i2c_start_recv(I2CBus *bus, uint8_t address);
+
+/**
+ * i2c_start_send: start a 'send' transfer on an I2C bus.
+ *
+ * @bus: #I2CBus to be used
+ * @address: address of the slave
+ *
+ * Returns: 0 on success, -1 on error
+ */
+int i2c_start_send(I2CBus *bus, uint8_t address);
+
void i2c_end_transfer(I2CBus *bus);
void i2c_nack(I2CBus *bus);
int i2c_send(I2CBus *bus, uint8_t data);
diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index 5483bf95a3e..416372ad00c 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -180,6 +180,16 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address, bool is_recv)
: I2C_START_SEND);
}
+int i2c_start_recv(I2CBus *bus, uint8_t address)
+{
+ return i2c_do_start_transfer(bus, address, I2C_START_RECV);
+}
+
+int i2c_start_send(I2CBus *bus, uint8_t address)
+{
+ return i2c_do_start_transfer(bus, address, I2C_START_SEND);
+}
+
void i2c_end_transfer(I2CBus *bus)
{
I2CSlaveClass *sc;
diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
index 06e1e5321b9..d7eae548cbc 100644
--- a/hw/i2c/pm_smbus.c
+++ b/hw/i2c/pm_smbus.c
@@ -128,14 +128,14 @@ static void smb_transaction(PMSMBus *s)
* So at least Linux may or may not set the read bit here.
* So just ignore the read bit for this command.
*/
- if (i2c_start_transfer(bus, addr, 0)) {
+ if (i2c_start_send(bus, addr)) {
goto error;
}
ret = i2c_send(bus, s->smb_data1);
if (ret) {
goto error;
}
- if (i2c_start_transfer(bus, addr, 1)) {
+ if (i2c_start_recv(bus, addr)) {
goto error;
}
s->in_i2c_block_read = true;
diff --git a/hw/i2c/smbus_master.c b/hw/i2c/smbus_master.c
index dc43b8637d1..6a53c34e70b 100644
--- a/hw/i2c/smbus_master.c
+++ b/hw/i2c/smbus_master.c
@@ -29,7 +29,7 @@ int smbus_receive_byte(I2CBus *bus, uint8_t addr)
{
uint8_t data;
- if (i2c_start_transfer(bus, addr, 1)) {
+ if (i2c_start_recv(bus, addr)) {
return -1;
}
data = i2c_recv(bus);
@@ -40,7 +40,7 @@ int smbus_receive_byte(I2CBus *bus, uint8_t addr)
int smbus_send_byte(I2CBus *bus, uint8_t addr, uint8_t data)
{
- if (i2c_start_transfer(bus, addr, 0)) {
+ if (i2c_start_send(bus, addr)) {
return -1;
}
i2c_send(bus, data);
@@ -51,11 +51,11 @@ int smbus_send_byte(I2CBus *bus, uint8_t addr, uint8_t data)
int smbus_read_byte(I2CBus *bus, uint8_t addr, uint8_t command)
{
uint8_t data;
- if (i2c_start_transfer(bus, addr, 0)) {
+ if (i2c_start_send(bus, addr)) {
return -1;
}
i2c_send(bus, command);
- if (i2c_start_transfer(bus, addr, 1)) {
+ if (i2c_start_recv(bus, addr)) {
i2c_end_transfer(bus);
return -1;
}
@@ -67,7 +67,7 @@ int smbus_read_byte(I2CBus *bus, uint8_t addr, uint8_t command)
int smbus_write_byte(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t data)
{
- if (i2c_start_transfer(bus, addr, 0)) {
+ if (i2c_start_send(bus, addr)) {
return -1;
}
i2c_send(bus, command);
@@ -79,11 +79,11 @@ int smbus_write_byte(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t data)
int smbus_read_word(I2CBus *bus, uint8_t addr, uint8_t command)
{
uint16_t data;
- if (i2c_start_transfer(bus, addr, 0)) {
+ if (i2c_start_send(bus, addr)) {
return -1;
}
i2c_send(bus, command);
- if (i2c_start_transfer(bus, addr, 1)) {
+ if (i2c_start_recv(bus, addr)) {
i2c_end_transfer(bus);
return -1;
}
@@ -96,7 +96,7 @@ int smbus_read_word(I2CBus *bus, uint8_t addr, uint8_t command)
int smbus_write_word(I2CBus *bus, uint8_t addr, uint8_t command, uint16_t data)
{
- if (i2c_start_transfer(bus, addr, 0)) {
+ if (i2c_start_send(bus, addr)) {
return -1;
}
i2c_send(bus, command);
@@ -113,12 +113,12 @@ int smbus_read_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
int i;
if (send_cmd) {
- if (i2c_start_transfer(bus, addr, 0)) {
+ if (i2c_start_send(bus, addr)) {
return -1;
}
i2c_send(bus, command);
}
- if (i2c_start_transfer(bus, addr, 1)) {
+ if (i2c_start_recv(bus, addr)) {
if (send_cmd) {
i2c_end_transfer(bus);
}
@@ -149,7 +149,7 @@ int smbus_write_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
len = 32;
}
- if (i2c_start_transfer(bus, addr, 0)) {
+ if (i2c_start_send(bus, addr)) {
return -1;
}
i2c_send(bus, command);
diff --git a/hw/misc/auxbus.c b/hw/misc/auxbus.c
index 44aa9730bc9..434ff8d910d 100644
--- a/hw/misc/auxbus.c
+++ b/hw/misc/auxbus.c
@@ -135,7 +135,7 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
i2c_end_transfer(i2c_bus);
}
- if (i2c_start_transfer(i2c_bus, address, true)) {
+ if (i2c_start_recv(i2c_bus, address)) {
ret = AUX_I2C_NACK;
break;
}
@@ -151,7 +151,7 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
i2c_end_transfer(i2c_bus);
}
- if (i2c_start_transfer(i2c_bus, address, false)) {
+ if (i2c_start_send(i2c_bus, address)) {
ret = AUX_I2C_NACK;
break;
}
@@ -179,7 +179,7 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
/*
* No transactions started..
*/
- if (i2c_start_transfer(i2c_bus, address, false)) {
+ if (i2c_start_send(i2c_bus, address)) {
break;
}
} else if ((address != bus->last_i2c_address) ||
@@ -188,7 +188,7 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
* Transaction started but we need to restart..
*/
i2c_end_transfer(i2c_bus);
- if (i2c_start_transfer(i2c_bus, address, false)) {
+ if (i2c_start_send(i2c_bus, address)) {
break;
}
}
@@ -210,7 +210,7 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
/*
* No transactions started..
*/
- if (i2c_start_transfer(i2c_bus, address, true)) {
+ if (i2c_start_recv(i2c_bus, address)) {
break;
}
} else if ((address != bus->last_i2c_address) ||
@@ -219,7 +219,7 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
* Transaction started but we need to restart..
*/
i2c_end_transfer(i2c_bus);
- if (i2c_start_transfer(i2c_bus, address, true)) {
+ if (i2c_start_recv(i2c_bus, address)) {
break;
}
}
--
2.31.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v5 15/15] hw/i2c: Introduce i2c_start_recv() and i2c_start_send()
2021-06-17 11:53 ` [PATCH v5 15/15] hw/i2c: Introduce i2c_start_recv() and i2c_start_send() Philippe Mathieu-Daudé
@ 2021-06-18 0:27 ` BALATON Zoltan
0 siblings, 0 replies; 21+ messages in thread
From: BALATON Zoltan @ 2021-06-18 0:27 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: Corey Minyard, Richard Henderson, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 9179 bytes --]
On Thu, 17 Jun 2021, Philippe Mathieu-Daudé wrote:
> To ease reviewing code using the I2C bus API, introduce the
> i2c_start_recv() and i2c_start_send() helpers which don't
> take the confusing 'is_recv' boolean argument.
In defence of this is_recv I'd like to mention that I2C has such a bit in
device addresses that decides if it's a receive or send so this is
actually a good match for that. The problem was that the send_recv
function had it reversed. With that fixed it was actually a good match for
i2c operations and in some cases allowed to write less code. Like the
auxbus shows that became twice as large using these separate functions
instead of being able to handle send or receive the same way.
Regards,
BALATON Zoltan
> Use these new helpers in the SMBus / AUX bus models.
>
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> include/hw/i2c/i2c.h | 24 ++++++++++++++++++++++++
> hw/i2c/core.c | 10 ++++++++++
> hw/i2c/pm_smbus.c | 4 ++--
> hw/i2c/smbus_master.c | 22 +++++++++++-----------
> hw/misc/auxbus.c | 12 ++++++------
> 5 files changed, 53 insertions(+), 19 deletions(-)
>
> diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
> index 21f2dba1bf7..5ca3b708c0b 100644
> --- a/include/hw/i2c/i2c.h
> +++ b/include/hw/i2c/i2c.h
> @@ -88,9 +88,33 @@ int i2c_bus_busy(I2CBus *bus);
> * @address: address of the slave
> * @is_recv: indicates the transfer direction
> *
> + * When @is_recv is a known boolean constant, use the
> + * i2c_start_recv() or i2c_start_send() helper instead.
> + *
> * Returns: 0 on success, -1 on error
> */
> int i2c_start_transfer(I2CBus *bus, uint8_t address, bool is_recv);
> +
> +/**
> + * i2c_start_recv: start a 'receive' transfer on an I2C bus.
> + *
> + * @bus: #I2CBus to be used
> + * @address: address of the slave
> + *
> + * Returns: 0 on success, -1 on error
> + */
> +int i2c_start_recv(I2CBus *bus, uint8_t address);
> +
> +/**
> + * i2c_start_send: start a 'send' transfer on an I2C bus.
> + *
> + * @bus: #I2CBus to be used
> + * @address: address of the slave
> + *
> + * Returns: 0 on success, -1 on error
> + */
> +int i2c_start_send(I2CBus *bus, uint8_t address);
> +
> void i2c_end_transfer(I2CBus *bus);
> void i2c_nack(I2CBus *bus);
> int i2c_send(I2CBus *bus, uint8_t data);
> diff --git a/hw/i2c/core.c b/hw/i2c/core.c
> index 5483bf95a3e..416372ad00c 100644
> --- a/hw/i2c/core.c
> +++ b/hw/i2c/core.c
> @@ -180,6 +180,16 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address, bool is_recv)
> : I2C_START_SEND);
> }
>
> +int i2c_start_recv(I2CBus *bus, uint8_t address)
> +{
> + return i2c_do_start_transfer(bus, address, I2C_START_RECV);
> +}
> +
> +int i2c_start_send(I2CBus *bus, uint8_t address)
> +{
> + return i2c_do_start_transfer(bus, address, I2C_START_SEND);
> +}
> +
> void i2c_end_transfer(I2CBus *bus)
> {
> I2CSlaveClass *sc;
> diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
> index 06e1e5321b9..d7eae548cbc 100644
> --- a/hw/i2c/pm_smbus.c
> +++ b/hw/i2c/pm_smbus.c
> @@ -128,14 +128,14 @@ static void smb_transaction(PMSMBus *s)
> * So at least Linux may or may not set the read bit here.
> * So just ignore the read bit for this command.
> */
> - if (i2c_start_transfer(bus, addr, 0)) {
> + if (i2c_start_send(bus, addr)) {
> goto error;
> }
> ret = i2c_send(bus, s->smb_data1);
> if (ret) {
> goto error;
> }
> - if (i2c_start_transfer(bus, addr, 1)) {
> + if (i2c_start_recv(bus, addr)) {
> goto error;
> }
> s->in_i2c_block_read = true;
> diff --git a/hw/i2c/smbus_master.c b/hw/i2c/smbus_master.c
> index dc43b8637d1..6a53c34e70b 100644
> --- a/hw/i2c/smbus_master.c
> +++ b/hw/i2c/smbus_master.c
> @@ -29,7 +29,7 @@ int smbus_receive_byte(I2CBus *bus, uint8_t addr)
> {
> uint8_t data;
>
> - if (i2c_start_transfer(bus, addr, 1)) {
> + if (i2c_start_recv(bus, addr)) {
> return -1;
> }
> data = i2c_recv(bus);
> @@ -40,7 +40,7 @@ int smbus_receive_byte(I2CBus *bus, uint8_t addr)
>
> int smbus_send_byte(I2CBus *bus, uint8_t addr, uint8_t data)
> {
> - if (i2c_start_transfer(bus, addr, 0)) {
> + if (i2c_start_send(bus, addr)) {
> return -1;
> }
> i2c_send(bus, data);
> @@ -51,11 +51,11 @@ int smbus_send_byte(I2CBus *bus, uint8_t addr, uint8_t data)
> int smbus_read_byte(I2CBus *bus, uint8_t addr, uint8_t command)
> {
> uint8_t data;
> - if (i2c_start_transfer(bus, addr, 0)) {
> + if (i2c_start_send(bus, addr)) {
> return -1;
> }
> i2c_send(bus, command);
> - if (i2c_start_transfer(bus, addr, 1)) {
> + if (i2c_start_recv(bus, addr)) {
> i2c_end_transfer(bus);
> return -1;
> }
> @@ -67,7 +67,7 @@ int smbus_read_byte(I2CBus *bus, uint8_t addr, uint8_t command)
>
> int smbus_write_byte(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t data)
> {
> - if (i2c_start_transfer(bus, addr, 0)) {
> + if (i2c_start_send(bus, addr)) {
> return -1;
> }
> i2c_send(bus, command);
> @@ -79,11 +79,11 @@ int smbus_write_byte(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t data)
> int smbus_read_word(I2CBus *bus, uint8_t addr, uint8_t command)
> {
> uint16_t data;
> - if (i2c_start_transfer(bus, addr, 0)) {
> + if (i2c_start_send(bus, addr)) {
> return -1;
> }
> i2c_send(bus, command);
> - if (i2c_start_transfer(bus, addr, 1)) {
> + if (i2c_start_recv(bus, addr)) {
> i2c_end_transfer(bus);
> return -1;
> }
> @@ -96,7 +96,7 @@ int smbus_read_word(I2CBus *bus, uint8_t addr, uint8_t command)
>
> int smbus_write_word(I2CBus *bus, uint8_t addr, uint8_t command, uint16_t data)
> {
> - if (i2c_start_transfer(bus, addr, 0)) {
> + if (i2c_start_send(bus, addr)) {
> return -1;
> }
> i2c_send(bus, command);
> @@ -113,12 +113,12 @@ int smbus_read_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
> int i;
>
> if (send_cmd) {
> - if (i2c_start_transfer(bus, addr, 0)) {
> + if (i2c_start_send(bus, addr)) {
> return -1;
> }
> i2c_send(bus, command);
> }
> - if (i2c_start_transfer(bus, addr, 1)) {
> + if (i2c_start_recv(bus, addr)) {
> if (send_cmd) {
> i2c_end_transfer(bus);
> }
> @@ -149,7 +149,7 @@ int smbus_write_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
> len = 32;
> }
>
> - if (i2c_start_transfer(bus, addr, 0)) {
> + if (i2c_start_send(bus, addr)) {
> return -1;
> }
> i2c_send(bus, command);
> diff --git a/hw/misc/auxbus.c b/hw/misc/auxbus.c
> index 44aa9730bc9..434ff8d910d 100644
> --- a/hw/misc/auxbus.c
> +++ b/hw/misc/auxbus.c
> @@ -135,7 +135,7 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
> i2c_end_transfer(i2c_bus);
> }
>
> - if (i2c_start_transfer(i2c_bus, address, true)) {
> + if (i2c_start_recv(i2c_bus, address)) {
> ret = AUX_I2C_NACK;
> break;
> }
> @@ -151,7 +151,7 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
> i2c_end_transfer(i2c_bus);
> }
>
> - if (i2c_start_transfer(i2c_bus, address, false)) {
> + if (i2c_start_send(i2c_bus, address)) {
> ret = AUX_I2C_NACK;
> break;
> }
> @@ -179,7 +179,7 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
> /*
> * No transactions started..
> */
> - if (i2c_start_transfer(i2c_bus, address, false)) {
> + if (i2c_start_send(i2c_bus, address)) {
> break;
> }
> } else if ((address != bus->last_i2c_address) ||
> @@ -188,7 +188,7 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
> * Transaction started but we need to restart..
> */
> i2c_end_transfer(i2c_bus);
> - if (i2c_start_transfer(i2c_bus, address, false)) {
> + if (i2c_start_send(i2c_bus, address)) {
> break;
> }
> }
> @@ -210,7 +210,7 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
> /*
> * No transactions started..
> */
> - if (i2c_start_transfer(i2c_bus, address, true)) {
> + if (i2c_start_recv(i2c_bus, address)) {
> break;
> }
> } else if ((address != bus->last_i2c_address) ||
> @@ -219,7 +219,7 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
> * Transaction started but we need to restart..
> */
> i2c_end_transfer(i2c_bus);
> - if (i2c_start_transfer(i2c_bus, address, true)) {
> + if (i2c_start_recv(i2c_bus, address)) {
> break;
> }
> }
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v5 00/15] hw/i2c: Remove confusing i2c_send_recv() API
2021-06-17 11:53 [PATCH v5 00/15] hw/i2c: Remove confusing i2c_send_recv() API Philippe Mathieu-Daudé
` (14 preceding siblings ...)
2021-06-17 11:53 ` [PATCH v5 15/15] hw/i2c: Introduce i2c_start_recv() and i2c_start_send() Philippe Mathieu-Daudé
@ 2021-06-17 13:16 ` Corey Minyard
15 siblings, 0 replies; 21+ messages in thread
From: Corey Minyard @ 2021-06-17 13:16 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: Corey Minyard, qemu-devel
On Thu, Jun 17, 2021 at 01:53:19PM +0200, Philippe Mathieu-Daudé wrote:
> Full series reviewed, all comments addressed.
>
> Corey, could you take this via your tree?
Ok. I'll do some testing then request a pull.
-corey
>
> Regards,
>
> Phil.
>
> This is a respin of Zoltan's patch:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg714711.html
>
> Since v4:
> - removed assertion in i2c_do_start_transfer (Richard)
> - added Richard R-b tags
>
> Since v3:
> - addressed minor review comments from Richard/Corey
> - added R-b/A-b tags
> - implemented Richard suggestion (last 2 patches, 14 & 15)
>
> Since v2, tried to address Corey's review comments resulting
> in a i2c_send_recv() removal and code easier to review (to my
> taste at least).
>
> BALATON Zoltan (1):
> hw/i2c: Make i2c_start_transfer() direction argument a boolean
>
> Philippe Mathieu-Daudé (14):
> hw/input/lm832x: Move lm832x_key_event() declaration to "lm832x.h"
> hw/input/lm832x: Define TYPE_LM8323 in public header
> hw/display/sm501: Simplify sm501_i2c_write() logic
> hw/display/sm501: Replace i2c_send_recv() by i2c_recv() & i2c_send()
> hw/i2c/ppc4xx_i2c: Add reference to datasheet
> hw/i2c/ppc4xx_i2c: Replace i2c_send_recv() by i2c_recv() & i2c_send()
> hw/misc/auxbus: Fix MOT/classic I2C mode
> hw/misc/auxbus: Explode READ_I2C / WRITE_I2C_MOT cases
> hw/misc/auxbus: Replace 'is_write' boolean by its value
> hw/misc/auxbus: Replace i2c_send_recv() by i2c_recv() & i2c_send()
> hw/i2c: Remove confusing i2c_send_recv()
> hw/i2c: Rename i2c_set_slave_address() -> i2c_slave_set_address()
> hw/i2c: Extract i2c_do_start_transfer() from i2c_start_transfer()
> hw/i2c: Introduce i2c_start_recv() and i2c_start_send()
>
> include/hw/i2c/i2c.h | 46 +++++++++++++++++++++---
> include/hw/input/lm832x.h | 28 +++++++++++++++
> hw/arm/nseries.c | 3 +-
> hw/arm/pxa2xx.c | 2 +-
> hw/arm/spitz.c | 4 +--
> hw/display/ati.c | 2 +-
> hw/display/sm501.c | 16 +++++----
> hw/display/xlnx_dp.c | 2 +-
> hw/i2c/core.c | 76 ++++++++++++++++++++++-----------------
> hw/i2c/imx_i2c.c | 2 +-
> hw/i2c/pm_smbus.c | 4 +--
> hw/i2c/ppc4xx_i2c.c | 15 +++++---
> hw/i2c/smbus_master.c | 22 ++++++------
> hw/input/lm832x.c | 2 +-
> hw/misc/auxbus.c | 70 ++++++++++++++++++++++++++----------
> MAINTAINERS | 1 +
> 16 files changed, 207 insertions(+), 88 deletions(-)
> create mode 100644 include/hw/input/lm832x.h
>
> --
> 2.31.1
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread