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

[BALATON Zoltan]

On Fri, 8 Jun 2018, David Gibson wrote:
> 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.

I prefer code that does not have unneeded chars so it's easier to read, 
that's why I've removed all 0x0 from this file which made it more 
comprehensible. But I'll add the 0 back to this place in next respin as 
you both seem to prefer that and since other bit masks are two digit too 
it's also consistent that way.