From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MnHAH-00045o-CM for qemu-devel@nongnu.org; Mon, 14 Sep 2009 15:24:41 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MnHAC-00045c-Sy for qemu-devel@nongnu.org; Mon, 14 Sep 2009 15:24:40 -0400 Received: from [199.232.76.173] (port=37116 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MnHAC-00045Z-Ob for qemu-devel@nongnu.org; Mon, 14 Sep 2009 15:24:36 -0400 Received: from mail-ew0-f221.google.com ([209.85.219.221]:37209) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MnHAC-00053S-Av for qemu-devel@nongnu.org; Mon, 14 Sep 2009 15:24:36 -0400 Received: by ewy21 with SMTP id 21so3057184ewy.8 for ; Mon, 14 Sep 2009 12:24:35 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <87ljkhxsi1.fsf@pike.pond.sub.org> References: <37d5a3ce941bc89f6d73b8e7bf746952a0074da2.1252699478.git.armbru@redhat.com> <20090912101009.GC16110@laped.iglesias.mooo.com> <4AAE0575.8060607@redhat.com> <20090914091545.GA21160@edde.se.axis.com> <87ljkhxsi1.fsf@pike.pond.sub.org> From: Blue Swirl Date: Mon, 14 Sep 2009 22:24:15 +0300 Message-ID: Subject: Re: [Qemu-devel] [PATCH 1/6] Move function definitions out of xilinx.h Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel@nongnu.org, "Edgar E. Iglesias" , Paul Brook , Gerd Hoffmann On Mon, Sep 14, 2009 at 9:59 PM, Markus Armbruster wrot= e: > Blue Swirl writes: > >> On Mon, Sep 14, 2009 at 12:15 PM, Edgar E. Iglesias >> wrote: >>> On Mon, Sep 14, 2009 at 10:57:25AM +0200, Gerd Hoffmann wrote: >>>> On 09/12/09 12:10, Edgar E. Iglesias wrote: >>>>> On Sat, Sep 12, 2009 at 09:04:17AM +0300, Blue Swirl wrote: >>>>>> On Fri, Sep 11, 2009 at 11:19 PM, Markus Armbruster >>>>>> wrote: >>>>>>> xilinx.h defines a couple of static inline functions for creating >>>>>>> devices. =C2=A0While that's a fair technique for hot functions, dev= ice >>>>>>> initialization is about as cold as it gets. =C2=A0Define them in th= e device >>>>>>> source files instead, and keep only declarations in the header. >>>>>> >>>>>> If I understood the qdev plan correctly, this is going to wrong >>>>>> direction. These functions should reside near the instantiation, not >>>>>> in the device code. The current approach looks OK if there are going >>>>>> to be more users of the devices. >>>> >>>> The functions should go away ;) >>>> >>>> Some day the information carried by those code snippeds should come fr= om a >>>> machine description file, then we'll don't need them any more. >>> >>> I agree. >>> >>>> >>>>> I agree that they shouldn't be in the device source. >>>>> The reason they ended up in a header and not with the petalogix board >>>>> was that in my tree there are multiple boards using these functions >>>>> to easy instantiate devices. >>>> >>>> They have to be somewhere. =C2=A0Having them in a header file is uncle= an. Having >>>> them in the board-specific code isn't practical when multiple boards s= hare >>>> the code. =C2=A0I'd stick them to the device source code as well. =C2= =A0Also note >>>> that this is common practice elsewhere in the tree. >>> >>> I disagree. >>> >>> But if ppl feel very strongly about this, I can remove them and deal wi= th >>> the code duplication in my tree. Afterall, it's unlikely that upsteam >>> qemu gets more xilinx boards before some kind of device tree driven >>> board support is there. >> >> I also disagree. Why would the functions in a header file be unclean? >> The common practice is wrong. > > The common practice is to put code in .c files, and stick to > declarations needed by several .c files in .h files. =C2=A0It's okay to > deviate from that common practice if there's a good reason --- we do > that in places. =C2=A0However, in this case, I couldn't see a reason, so = I > moved the code from the header to the place where such code lives for > pretty much every other qdevified device: the device code. Understandable, since we don't see the source code of the other xilinx boar= ds. > Doing it the same way everywhere means that maintainers have an easier > time to predict where stuff is, how it's named, and so forth. =C2=A0Don't > disappoint reasonable maintainer expectations without need. It's still wrong, however disappointing that may be. But with enough cleanups and guidance, the expectations will be directed to right path. > I don't want to make a big deal out of this. =C2=A0If the code must remai= n in > the header, expectations be damned, I'll respin the series and move on. Pushing the device instantiation code higher makes the device code a bit simpler. Also the internal state can be kept private, provided that the interface is designed properly. For example, slavio_timer.c does not export internal state and all functions are static. The former init function is now in sun4m.c. Actually, while I'm quite happy with the device level design (and I think other targets should also be worked to this direction, not the opposite), the next level design is not so clear since we don't have the machine config or something.