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(-)

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);
-- 
2.31.1

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!
>
>