From patchwork Thu May 2 23:58:32 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Peter Crosthwaite X-Patchwork-Id: 241119 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 1E4592C00CA for ; Fri, 3 May 2013 09:58:56 +1000 (EST) Received: from localhost ([::1]:53160 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UY3Oo-0007eU-2L for incoming@patchwork.ozlabs.org; Thu, 02 May 2013 19:58:54 -0400 Received: from eggs.gnu.org ([208.118.235.92]:44391) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UY3OV-0007eO-R5 for qemu-devel@nongnu.org; Thu, 02 May 2013 19:58:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UY3OU-0006bT-Dw for qemu-devel@nongnu.org; Thu, 02 May 2013 19:58:35 -0400 Received: from mail-bk0-x235.google.com ([2a00:1450:4008:c01::235]:35349) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UY3OU-0006bJ-3O for qemu-devel@nongnu.org; Thu, 02 May 2013 19:58:34 -0400 Received: by mail-bk0-f53.google.com with SMTP id i18so505825bkv.12 for ; Thu, 02 May 2013 16:58:32 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=mime-version:x-received:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type :x-gm-message-state; bh=2R0bgNYHKWgGJQ3E2MGjGMzlkFNCQ5vbD9L2wwoqU3w=; b=Mo2V1g9uU18Kum4PPxuGkq4TobmLf87vBJRetvMHyZHI5nY4VM0x82tisFyRKPiaS7 IuoWqlhrbgbJHaHNLJbH8f69WGEPxZOHVIDP23j3nYS79jFU9DDlX9xEtV+TGnw09F/J U41nHOkzmBz6eZxbHpTljJkOrv+hz3iOosB03UPKMnCg/5dssn0rre5rW/4PAg9lSuMz nPttwp/bWv7dzhhYvmtJkM0ZiXYOMf5OTNlurryS9qflem3ULe3O0UhVn5kN3/pqRcEL 3+CDQas/etXk1dUA/bTl0yOQca6CFJK9m0yWnEzd3Z4SZtbzgaF1iBKKbdDi5bvBFUnV zCMA== MIME-Version: 1.0 X-Received: by 10.204.71.77 with SMTP id g13mr2747421bkj.50.1367539112604; Thu, 02 May 2013 16:58:32 -0700 (PDT) Received: by 10.205.132.196 with HTTP; Thu, 2 May 2013 16:58:32 -0700 (PDT) In-Reply-To: <5182B165.2010204@tribudubois.net> References: <1367438026-27573-1-git-send-email-jcd@tribudubois.net> <5182B165.2010204@tribudubois.net> Date: Fri, 3 May 2013 09:58:32 +1000 X-Google-Sender-Auth: 6QwLUWkXWMvxS3H8va67tNfLYJQ Message-ID: From: Peter Crosthwaite To: Jean-Christophe DUBOIS X-Gm-Message-State: ALoCoQlm7hPzosPhKnPB28kvVEvtR4Ywn22kHCVtlLn6XApZPx2rSD7znsmW6s/oiq+9GOP7oyZw X-detected-operating-system: by eggs.gnu.org: Error: Malformed IPv6 address (bad octet value). X-Received-From: 2a00:1450:4008:c01::235 Cc: qemu-devel@nongnu.org Subject: Re: [Qemu-devel] [PATCH 1/2] Add i.MX I2C device emulator. X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Hi JC, On Fri, May 3, 2013 at 4:33 AM, Jean-Christophe DUBOIS wrote: > Peter, > > Thanks for you review. > > I have a few questions below. > > JC > > > On 05/02/2013 02:16 PM, Peter Crosthwaite wrote: >> >> Hi Jean-Christophe, >> >> Thanks for your contribution. Please run the patch through >> scripts/checkpatch.pl to check for formatting errors. >> >> On Thu, May 2, 2013 at 5:53 AM, Jean-Christophe DUBOIS >> wrote: >> ERROR: return is not a function, parentheses are not required >> #148: FILE: hw/i2c/imx_i2c.c:114: >> + return (s->i2cr & I2CR_IEN); >> >> >From checkpatch, here and below. > > > Will do. > > >>> +} >>> + >>> +static inline bool imx_i2c_interrupt_is_enabled(imx_i2c_state *s) >>> +{ >>> + return (s->i2cr & I2CR_IIEN); >>> +} >>> + >>> +static inline bool imx_i2c_is_master(imx_i2c_state *s) >>> +{ >>> + return (s->i2cr & I2CR_MSTA); >>> +} >>> + >>> +static inline bool imx_i2c_direction_is_tx(imx_i2c_state *s) >>> +{ >>> + return (s->i2cr & I2CR_MTX); >>> +} >>> + >>> +static void imx_i2c_reset(DeviceState *d) >>> +{ >>> + imx_i2c_state *s = FROM_SYSBUS(imx_i2c_state, SYS_BUS_DEVICE(d)); >>> + >> >> Please don't use FROM_SYSBUS in new code. Use QOM cast macros. >> >> http://wiki.qemu.org/QOMConventions >> >> Has useful guidelines for the rules around this for new devices. > > > It is not very clear what you do expect. Could you point me to a driver that > is up to date. > Its actually quite hard to find them but hw/dma/pl330.c was fairly recently reviewed and accepted sysbus device. There is one QOM violation in there just with a quick scan for the more common errors, that is best fixed in new device models. fix below: Type cast macro: #define TYPE_PL330 "pl330" #define PL330(obj) OBJECT_CHECK(PL330State, (obj), TYPE_PL330) Realize fn: static void pl330_realize(DeviceState *dev, Error **errp) { int i; PL330State *s = PL330(dev); (Theres an example usage of QOM cast macro). and its wiring up: static void pl330_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); dc->realize = pl330_realize; dc->reset = pl330_reset; dc->props = pl330_properties; dc->vmsd = &vmstate_pl330; } > >>> + if (s->address != ADDR_RESET) { >>> + i2c_end_transfer(s->bus); >>> + } >>> + >>> + s->address = ADDR_RESET; >>> + s->iadr = IADR_RESET; >>> + s->ifdr = IFDR_RESET; >>> + s->i2cr = I2CR_RESET; >>> + s->i2sr = I2SR_RESET; >>> + s->i2dr_read = I2DR_RESET; >>> + s->i2dr_write = I2DR_RESET; >>> +} >>> + >>> +static inline void imx_i2c_raise_interrupt(imx_i2c_state *s) >>> +{ >>> + /* >>> + * raise an interrupt if the device is enabled and it is configured >>> + * to generate some interrupts. >>> + */ >>> + if (imx_i2c_is_enabled(s) && imx_i2c_interrupt_is_enabled(s)) { >>> + s->i2sr |= I2SR_IIF; >>> + qemu_irq_raise(s->irq); >>> + } >>> +} >>> + >>> +static uint64_t imx_i2c_read(void *opaque, hwaddr offset, >>> + unsigned size) >>> +{ >>> + imx_i2c_state *s = (imx_i2c_state *)opaque; >> >> QOM cast macro. > > > Could you point me to an up to date driver using QOM cast macro? > Covered above > >> >> + >> +static const MemoryRegionOps imx_i2c_ops = { >> + .read = imx_i2c_read, >> + .write = imx_i2c_write, >> + .endianness = DEVICE_NATIVE_ENDIAN, >> I think you may need a .valid definition here as it looks like you >> have restrictions on access size and alignment? (looks like 4bytes >> accesses only). > > > I'll look into this. > More example: static const MemoryRegionOps pl330_ops = { .read = pl330_iomem_read, .write = pl330_iomem_write, .endianness = DEVICE_NATIVE_ENDIAN, .impl = { .min_access_size = 4, .max_access_size = 4, } }; > >>> +}; >>> + >>> +static const VMStateDescription imx_i2c_vmstate = { >>> + .name = TYPE_IMX_I2C, >>> + .version_id = 1, >>> + .minimum_version_id = 1, >>> + .fields = (VMStateField[]) { >>> + VMSTATE_UINT16(address, imx_i2c_state), >>> + VMSTATE_UINT16(iadr, imx_i2c_state), >>> + VMSTATE_UINT16(ifdr, imx_i2c_state), >>> + VMSTATE_UINT16(i2cr, imx_i2c_state), >>> + VMSTATE_UINT16(i2sr, imx_i2c_state), >>> + VMSTATE_UINT16(i2dr_read, imx_i2c_state), >>> + VMSTATE_UINT16(i2dr_write, imx_i2c_state), >>> + VMSTATE_END_OF_LIST() >>> + } >>> +}; >>> + >>> +static int imx_i2c_init(SysBusDevice *dev) >>> +{ >> >> Use of the SysBusDeviceClass::init function is deprecated. Please use >> DeviceClass::realise or Object::init. With no reliance on properties I >> would suggest this one can be done as just an Object::init fn. > > Could you point me to the documentation or an up to date example? > Not sure if there's any formal documentation around. But its covered by the pl330 examples. There are some relevant code comments around the place (include/hw/qdev-core.h). * # Realization # * Devices are constructed in two stages, * 1) object instantiation via object_initialize() and * 2) device realization via #DeviceState:realized property. * The former may not fail (it might assert or exit), the latter may return * error information to the caller and must be re-entrant. * Trivial field initializations should go into #TypeInfo.instance_init. * Operations depending on @props static properties should go into @realize. * After successful realization, setting static properties will fail. Regards, Peter --- a/hw/dma/pl330.c +++ b/hw/dma/pl330.c @@ -227,7 +227,7 @@ static const VMStateDescription vmstate_pl330_queue = { }; struct PL330State { - SysBusDevice busdev; + SysBusDevice parent_obj; Code of interest: