From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MG9gN-0005c1-GQ for qemu-devel@nongnu.org; Mon, 15 Jun 2009 06:44:55 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MG9gI-0005ZU-HH for qemu-devel@nongnu.org; Mon, 15 Jun 2009 06:44:54 -0400 Received: from [199.232.76.173] (port=32970 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MG9gI-0005ZR-D0 for qemu-devel@nongnu.org; Mon, 15 Jun 2009 06:44:50 -0400 Received: from mx2.redhat.com ([66.187.237.31]:43635) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MG9gH-0006FS-TD for qemu-devel@nongnu.org; Mon, 15 Jun 2009 06:44:50 -0400 Date: Mon, 15 Jun 2009 13:42:38 +0300 From: "Michael S. Tsirkin" Subject: Re: [Qemu-devel] Re: [PATCH 3/7] pci: pci_default_config_write() clean up. Message-ID: <20090615104238.GC6351@redhat.com> References: <1243924970-17545-1-git-send-email-yamahata@valinux.co.jp> <1243924970-17545-4-git-send-email-yamahata@valinux.co.jp> <20090602100121.GA3397@redhat.com> <20090603023108.GJ9176%yamahata@valinux.co.jp> <20090610154806.GF28601@redhat.com> <20090615091204.GE9418%yamahata@valinux.co.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20090615091204.GE9418%yamahata@valinux.co.jp> List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Isaku Yamahata Cc: paul@codesourcery.com, mtosatti@redhat.com, avi@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com On Mon, Jun 15, 2009 at 06:12:04PM +0900, Isaku Yamahata wrote: > On Wed, Jun 10, 2009 at 06:48:06PM +0300, Michael S. Tsirkin wrote: > > On Wed, Jun 03, 2009 at 11:31:08AM +0900, Isaku Yamahata wrote: > > > On Tue, Jun 02, 2009 at 01:01:21PM +0300, Michael S. Tsirkin wrote: > > > > On Tue, Jun 02, 2009 at 03:42:46PM +0900, Isaku Yamahata wrote: > > > > > +struct PCIConfigReg { > > > > > + uint8_t wmask; > > > > > + /* offset of registers in bits for 2/4 bytes function register */ > > > > > + uint8_t reg_offset; > > > > > > > > Sorry about being dense, but the comment still doesn't help me much. > > > > Can't we simply use the index in the array as offset? > > > > > > No. I believe this is helpfull. > > > the next patch for hw/wdt_i6300esb.c is a good example. > > > With this, we can replace fragile address and len comparison > > > with one callback per one register function. > > > > > > For that, the member which represents the position in function > > > is necessary. > > > > So maybe this is going too far into a table-driven direction then. > > Tables are good for common case, exceptions are better handled > > by regular functional design. > > > > I agree addr/len comparisons are fragile, but can't we simply implement > > functions to encapsulate them? Along the lines of: > > > > static inline int offset_in_range(int offset, int address, int len) > > { > > return address <= offset && address + len > offset; > > } > > > > static inline int ranges_match(int addr1, int len1, > > int addr2, int len2) > > { > > return offset_affected(addr1, addr2, len2) || > > offset_affected(addr2, addr1, len1); > > } > > > > Switching address and len comparison to use this would be a good cleanup > > IMO. > > Introducing helper function sounds a good idea. Should be a separate patch IMO. > I suppose that reg_offset and related functions can be removed > by helper functions. > So new callback function type would be > > typedef void (*pci_config_written_t)(struct PCIDevice *d, > uint32_t addr, uint32_t val, int len); > > Since this is same as PCIConfigWriteFunc, pci_config_written_t would > be removed with the next version. -- MST