Re: [PATCH v3 10/13] hw/misc/auxbus: Replace i2c_send_recv() by i2c_recv() & i2c_send()

[BALATON Zoltan]

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!