From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:54084) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QRYSs-00082p-BU for qemu-devel@nongnu.org; Tue, 31 May 2011 19:35:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QRYSq-0003cR-TE for qemu-devel@nongnu.org; Tue, 31 May 2011 19:35:10 -0400 Received: from mail-pw0-f45.google.com ([209.85.160.45]:57325) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QRYSq-0003az-8j for qemu-devel@nongnu.org; Tue, 31 May 2011 19:35:08 -0400 Received: by pwi6 with SMTP id 6so2478666pwi.4 for ; Tue, 31 May 2011 16:35:06 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <1306851364-22720-2-git-send-email-anarsoul@gmail.com> References: <1306851364-22720-1-git-send-email-anarsoul@gmail.com> <1306851364-22720-2-git-send-email-anarsoul@gmail.com> Date: Wed, 1 Jun 2011 00:35:05 +0100 Message-ID: From: Peter Maydell Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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: Vasily Khoruzhick Cc: "qemu-devel@nongnu.org" On 31 May 2011 15:16, Vasily Khoruzhick wrote: > +static uint32_t zipit_lcd_transfer(SSISlave *dev, uint32_t value) > +{ > + =C2=A0 =C2=A0ZipitLCD *z =3D FROM_SSI_SLAVE(ZipitLCD, dev); > + =C2=A0 =C2=A0if (z->enabled) { > + =C2=A0 =C2=A0 =C2=A0 =C2=A0z->buf[z->pos] =3D value & 0xff; > + =C2=A0 =C2=A0 =C2=A0 =C2=A0z->pos++; > + =C2=A0 =C2=A0} > + =C2=A0 =C2=A0if (z->pos =3D=3D 3) { > + =C2=A0 =C2=A0 =C2=A0 =C2=A0switch (z->buf[0]) { > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0case 0x74: > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0printf("%s: reg:= 0x%.2x\n", __func__, z->buf[2]); > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0break; > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0case 0x76: > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0printf("%s: valu= e: 0x%.4x\n", __func__, z->buf[1] << 8 | z->buf[2]); > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0break; > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0default: > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0printf("%s: unkn= own command!\n", __func__); > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0break; > + =C2=A0 =C2=A0 =C2=A0 =C2=A0} > + =C2=A0 =C2=A0 =C2=A0 =C2=A0z->pos =3D 0; > + =C2=A0 =C2=A0} > + =C2=A0 =C2=A0return 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 =3D { > + =C2=A0 =C2=A0.qdev.name =3D "zipit-lcd", > + =C2=A0 =C2=A0.qdev.size =3D sizeof(ZipitLCD), > + =C2=A0 =C2=A0.init =3D zipit_lcd_init, > + =C2=A0 =C2=A0.transfer =3D zipit_lcd_transfer > +}; Not that the device does much, but it ought to have a VMStateDescription structure to save/load its state. > + =C2=A0 =C2=A0case I2C_START_RECV: > + =C2=A0 =C2=A0 =C2=A0 =C2=A0if (s->len !=3D 1) > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0printf("%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) > +{ > + =C2=A0 =C2=A0int retval =3D 0x00; > + =C2=A0 =C2=A0AER915State *s =3D FROM_I2C_SLAVE(AER915State, slave); > + > + =C2=A0 =C2=A0/* 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 =3D { > + =C2=A0 =C2=A0.qdev.name =3D "aer915", > + =C2=A0 =C2=A0.qdev.size =3D sizeof(AER915State), > + =C2=A0 =C2=A0.init =3D aer915_init, > + =C2=A0 =C2=A0.event =3D aer915_event, > + =C2=A0 =C2=A0.recv =3D aer915_recv, > + =C2=A0 =C2=A0.send =3D aer915_send > +}; Missing save/load support again. > + =C2=A0 =C2=A0bus =3D pxa2xx_i2c_bus(cpu->i2c[0]); > + =C2=A0 =C2=A0i2c_create_slave(bus, "aer915", 0x55); > + =C2=A0 =C2=A0wm =3D i2c_create_slave(bus, "wm8750", 0x1b); > + =C2=A0 =C2=A0/* .. and to the sound interface. =C2=A0*/ 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