Re: [Qemu-devel] [PATCH 2/2] Add support for Zipit Z2 machine

[Vasily Khoruzhick <anarsoul@gmail.com>]

On Wednesday 01 June 2011 02:35:05 Peter Maydell wrote:
> On 31 May 2011 15:16, Vasily Khoruzhick <anarsoul@gmail.com> wrote:
> > +static uint32_t zipit_lcd_transfer(SSISlave *dev, uint32_t value)
> > +{
> > +    ZipitLCD *z = FROM_SSI_SLAVE(ZipitLCD, dev);
> > +    if (z->enabled) {
> > +        z->buf[z->pos] = value & 0xff;
> > +        z->pos++;
> > +    }
> > +    if (z->pos == 3) {
> > +        switch (z->buf[0]) {
> > +            case 0x74:
> > +                printf("%s: reg: 0x%.2x\n", __func__, z->buf[2]);
> > +                break;
> > +            case 0x76:
> > +                printf("%s: value: 0x%.4x\n", __func__, z->buf[1] << 8 |
> > z->buf[2]); +                break;
> > +            default:
> > +                printf("%s: unknown command!\n", __func__);
> > +                break;
> > +        }
> > +        z->pos = 0;
> > +    }
> > +    return 0;
> > +}
> 
> Presumably this is a stub for later functionality?
> I don't think we should be printing the debug tracing by default.

Ok, will fix it

> > +static SSISlaveInfo zipit_lcd_info = {
> > +    .qdev.name = "zipit-lcd",
> > +    .qdev.size = sizeof(ZipitLCD),
> > +    .init = zipit_lcd_init,
> > +    .transfer = zipit_lcd_transfer
> > +};
> 
> Not that the device does much, but it ought to have a
> VMStateDescription structure to save/load its state.

Ok

> > +    case I2C_START_RECV:
> > +        if (s->len != 1)
> > +            printf("%s: short message!?\n", __func__);
> 
> QEMU coding style demands braces here. Running through
> scripts/checkpatch.pl should catch this kind of thing.

Ok

> > +static int aer915_recv(i2c_slave *slave)
> > +{
> > +    int retval = 0x00;
> > +    AER915State *s = FROM_I2C_SLAVE(AER915State, slave);
> > +
> > +    /* Hardcoded value */
> 
> This comment isn't telling us anything we can't see from
> the code. More interesting would be what the hardcoded
> value actually means...

Ok

> > +static I2CSlaveInfo aer915_info = {
> > +    .qdev.name = "aer915",
> > +    .qdev.size = sizeof(AER915State),
> > +    .init = aer915_init,
> > +    .event = aer915_event,
> > +    .recv = aer915_recv,
> > +    .send = aer915_send
> > +};
> 
> Missing save/load support again.

This device is stateless, it only reports battery voltage (which is 
hardcoded), so nothing to save/load here.

> > +    bus = pxa2xx_i2c_bus(cpu->i2c[0]);
> > +    i2c_create_slave(bus, "aer915", 0x55);
> > +    wm = i2c_create_slave(bus, "wm8750", 0x1b);
> > +    /* .. and to the sound interface.  */
> 
> This comment has come from spitz.c, and it doesn't make
> much sense here since you've not copied the preceding
> comment which it is a continuation of:
>     /* Attach a WM8750 to the bus */

Ok, will remove it.

Thanks for review :)

Regards
Vasily