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

[Peter Maydell]

On 31 May 2011 15:16, Vasily Khoruzhick <address@hidden> 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.

> +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.

> +    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.

> +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...

> +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.

> +    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 */


-- PMM