| Message ID | 20210616161418.2514095-11-f4bug@amsat.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/i2c: Remove confusing i2c_send_recv() API | expand |
On 6/16/21 9:14 AM, Philippe Mathieu-Daudé wrote: > @@ -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--; > } This form of updating ret is better than... > @@ -200,14 +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) { > + for (i = 0; i < len; i++) { > + if (i2c_send(i2c_bus, data[i]) < 0) { > i2c_end_transfer(i2c_bus); > break; > } > - len--; > } > - if (len == 0) { > + if (i == len) { > ret = AUX_I2C_ACK; > } ... this one. Otherwise, Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On 6/16/21 8:46 PM, Richard Henderson wrote: > On 6/16/21 9:14 AM, Philippe Mathieu-Daudé wrote: >> @@ -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--; >> } > > This form of updating ret is better than... > >> @@ -200,14 +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) { >> + for (i = 0; i < len; i++) { >> + if (i2c_send(i2c_bus, data[i]) < 0) { >> i2c_end_transfer(i2c_bus); >> break; >> } >> - len--; >> } >> - if (len == 0) { >> + if (i == len) { >> ret = AUX_I2C_ACK; >> } > > ... this one. I totally agree :) I was a bit ashamed for posting that, I thought Zoltan would prefer less changes so used this form. Will update on respin. > Otherwise, > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Thanks!
On Wed, 16 Jun 2021, Philippe Mathieu-Daudé wrote: > On 6/16/21 8:46 PM, Richard Henderson wrote: >> On 6/16/21 9:14 AM, Philippe Mathieu-Daudé wrote: >>> @@ -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--; >>> } >> >> This form of updating ret is better than... >> >>> @@ -200,14 +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) { >>> + for (i = 0; i < len; i++) { >>> + if (i2c_send(i2c_bus, data[i]) < 0) { >>> i2c_end_transfer(i2c_bus); >>> break; >>> } >>> - len--; >>> } >>> - if (len == 0) { >>> + if (i == len) { >>> ret = AUX_I2C_ACK; >>> } >> >> ... this one. > > I totally agree :) I was a bit ashamed for posting that, I thought > Zoltan would prefer less changes so used this form. > Will update on respin. It's not the number of changes that matters but if there's any change in behaviour. If you can make it clearer that there's no change in behaviour by making more changes then that's OK. Regards, BALATON Zoltan >> Otherwise, >> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > > Thanks! > >
diff --git a/hw/misc/auxbus.c b/hw/misc/auxbus.c index d96219aef88..4d270ea9ec3 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,14 +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) { + for (i = 0; i < len; i++) { + if (i2c_send(i2c_bus, data[i]) < 0) { i2c_end_transfer(i2c_bus); break; } - len--; } - if (len == 0) { + if (i == len) { ret = AUX_I2C_ACK; } break; @@ -233,16 +227,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);
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. Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- hw/misc/auxbus.c | 32 ++++++++++---------------------- 1 file changed, 10 insertions(+), 22 deletions(-)