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

["Michael S. Tsirkin" <mst@redhat.com>]

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?

> +    pci_config_written_t callback;
> +};
>  
>  struct PCIDevice {
>      DeviceState qdev;
>      /* PCI config space */
>      uint8_t config[PCI_CONFIG_SPACE_SIZE];
> -
> -    /* Used to implement R/W bytes */
> -    uint8_t mask[PCI_CONFIG_SPACE_SIZE];
> +    struct PCIConfigReg config_regs[PCI_CONFIG_SPACE_SIZE];

I still think separate mask/config/callback arrays
are better - they are easier for devices to use. E.g. a single memset
can make a range of register writeable, and a single function call
does everything necessary to save a range the whole config space.

Add a callback array if you like, and be done with it.

>  
>      /* the following fields are read only */
>      PCIBus *bus;
> @@ -180,6 +261,21 @@ struct PCIDevice {
>      int irq_state[4];
>  };
>  
> +typedef void(*pci_conf_init_t)(struct PCIConfigReg*);
> +
> +void pci_conf_initb(struct PCIConfigReg *config_regs, uint32_t addr,
> +                    pci_config_written_t callback, uint32_t wmask);
> +void pci_conf_initw(struct PCIConfigReg *config_regs, uint32_t addr,
> +                    pci_config_written_t callback, uint32_t wmask);
> +void pci_conf_initl(struct PCIConfigReg *config_regs, uint32_t addr,
> +                    pci_config_written_t callback, uint32_t wmask);

If we got rid of reg_offset, I think we won't need these.
We'd just do dev->callback[REGISTER] = my_callback.


-- 
MST