[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/2] Add support for Zipit Z2 machine
From: |
Vasily Khoruzhick |
Subject: |
Re: [Qemu-devel] [PATCH 2/2] Add support for Zipit Z2 machine |
Date: |
Wed, 1 Jun 2011 09:25:51 +0300 |
User-agent: |
KMail/1.13.7 (Linux/2.6.39-gentoo-anarsoul; KDE/4.6.3; i686; ; ) |
On Wednesday 01 June 2011 02:35:05 Peter Maydell wrote:
> 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.
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
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [Qemu-devel] [PATCH 2/2] Add support for Zipit Z2 machine,
Vasily Khoruzhick <=