From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:55627) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QResl-0005zO-9B for qemu-devel@nongnu.org; Wed, 01 Jun 2011 02:26:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QResj-00053c-JP for qemu-devel@nongnu.org; Wed, 01 Jun 2011 02:26:19 -0400 Received: from mail-ey0-f173.google.com ([209.85.215.173]:41678) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QResj-00053O-EH for qemu-devel@nongnu.org; Wed, 01 Jun 2011 02:26:17 -0400 Received: by eyb6 with SMTP id 6so2151928eyb.4 for ; Tue, 31 May 2011 23:26:16 -0700 (PDT) From: Vasily Khoruzhick Date: Wed, 1 Jun 2011 09:25:51 +0300 References: <1306851364-22720-1-git-send-email-anarsoul@gmail.com> <1306851364-22720-2-git-send-email-anarsoul@gmail.com> In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <201106010925.51408.anarsoul@gmail.com> Subject: Re: [Qemu-devel] [PATCH 2/2] Add support for Zipit Z2 machine List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: "qemu-devel@nongnu.org" On Wednesday 01 June 2011 02:35:05 Peter Maydell wrote: > On 31 May 2011 15:16, Vasily Khoruzhick 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