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