Re: [Qemu-devel] [PATCH v2 1/8] ppc4xx_i2c: Clean up and improve error logging

[David Gibson]

On Wed, Jun 06, 2018 at 12:56:32PM -0300, Philippe Mathieu-Daudé wrote:
> On 06/06/2018 10:31 AM, BALATON Zoltan wrote:
> > Make it more readable by converting register indexes to decimal
> > (avoids lot of superfluous 0x0) and distinguish errors caused by
> > accessing non-existent vs. unimplemented registers.
> > No functional change.
> > 
> > Signed-off-by: BALATON Zoltan <address@hidden>
> > ---
> >  hw/i2c/ppc4xx_i2c.c | 94 
> > +++++++++++++++++++++++++++++------------------------
> >  1 file changed, 51 insertions(+), 43 deletions(-)
> > 
> > diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
> > index ab64d19..d1936db 100644
> > --- a/hw/i2c/ppc4xx_i2c.c
> > +++ b/hw/i2c/ppc4xx_i2c.c
> > @@ -31,7 +31,7 @@
> >  #include "hw/hw.h"
> >  #include "hw/i2c/ppc4xx_i2c.h"
> >  
> > -#define PPC4xx_I2C_MEM_SIZE 0x12
> > +#define PPC4xx_I2C_MEM_SIZE 18
> 
> This looks weird, it seems all memory range sizes are in hex in other
> QEMU devices.

[snip]
> > @@ -263,32 +267,31 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr 
> > addr, uint64_t value,
> >              }
> >          }
> >          break;
> > -    case 0x07:
> > -        i2c->mdcntl = value & 0xDF;
> > +    case 7:
> > +        i2c->mdcntl = value & 0xdf;
> >          break;
> > -    case 0x08:
> > -        i2c->sts &= ~(value & 0x0A);
> > +    case 8:
> > +        i2c->sts &= ~(value & 0xa);
> 
> 'value & 0x0a' implicitly denotes than 'value' is a 8-bit register.
> Matter of taste...

I tend to prefer the forms you suggest, Philippe, but not by enough to
delay this otherwise good cleanup.  Especially since Balaton is taking
on this long neglected area of the code.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature