Re: [Qemu-devel] Re: [PATCH 3/7] pci: pci_default_config_write() clean up.

[Isaku Yamahata]

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.
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.

thanks,
-- 
yamahata