Re: [PATCH v3 06/13] hw/i2c/ppc4xx_i2c: Replace i2c_send_recv() by i2c_recv() & i2c_send()

[BALATON Zoltan]

On Wed, 16 Jun 2021, Philippe Mathieu-Daudé wrote:
> On 6/16/21 10:01 PM, BALATON Zoltan wrote:
> > On Wed, 16 Jun 2021, Philippe Mathieu-Daudé wrote:
> > > On 6/16/21 9:16 PM, Corey Minyard wrote:
> > > > On Wed, Jun 16, 2021 at 06:14:11PM +0200, Philippe Mathieu-Daudé wrote:
> > > > > Instead of using the confuse i2c_send_recv(), rewrite to directly
> > > > > call i2c_recv() & i2c_send(), resulting in code easire to review.
> > > > >
> > > > > 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..b3d3da56e38 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])) {
> > > >
> > > > In the previous patch you checked < 0, it would be nice to be
> > > > consistent.
> > >
> > > I did that first but thought Zoltan wouldn't be happy, then went back :)
> > >
> > > I'll fix for the next iteration, thanks.
> >
> > I generally had no problem with i2c_send_recv only that its argument
> > that decides which operation to do was inverted compared to other
> > similar i2c functions so my original patch just corrected that for
> > consistency and I was happy with that.
>
> But we have to make the maintainer happy too to get patch merged ;)
>
> > Having a send_recv in one func
> > allowed to avoid if-else in some places like these but if you think it's
> > better without this function at all I can work with that too. I'll have
> > to check if these changes could break anything. At first sight I'm not
> > sure errors are handled as before if recv fails but it was years ago I
> > did the sm501 and ati parts and I forgot how they work so I need to
> > check again. I'll wait for the final version of the series then and test
> > that.
>
> Thanks, that would be great!

I've tried AmigaOS and MorphOS on sam460ex and mac99 with sm501 and 
ati-vga and these still work so I think the patches are OK but I did not 
test other changes to other machines so I did not give a tested-by for the 
series just some reviewed-by to patches I've verified. (Found a regression 
with AROS but that's not related to these changes.)

Regards,
BALATON Zoltan