Re: [Qemu-devel] [PATCH 15/20] acpi: switch smbus to memory api

["Andreas Färber" <afaerber@suse.de>]

Am 23.11.2012 16:48, schrieb Gerd Hoffmann:
> diff --git a/hw/pm_smbus.c b/hw/pm_smbus.c
> index 5d6046d..ea1380c 100644
> --- a/hw/pm_smbus.c
> +++ b/hw/pm_smbus.c
[...]
> @@ -170,7 +170,16 @@ uint32_t smb_ioport_readb(void *opaque, uint32_t addr)
>      return val;
>  }
>  
> +static const MemoryRegionOps pm_smbus_ops = {
> +    .read = smb_ioport_readb,
> +    .write = smb_ioport_writeb,
> +    .valid.min_access_size = 1,
> +    .valid.max_access_size = 1,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};

I notice that in comparison to Julien's patch, you are setting .valid
here where he used .impl.

Also a generic C question: When using C99-style struct initializers as
for the MemoryRegionOps, I understand that the fields not explicitly
assigned are zero-initialized. Does that also apply to .foo.bar = baz
notation or would it be advisable to use nested .foo = { .bar = baz }?

> +
>  void pm_smbus_init(DeviceState *parent, PMSMBus *smb)
>  {
>      smb->smbus = i2c_init_bus(parent, "i2c");
> +    memory_region_init_io(&smb->io, &pm_smbus_ops, smb, "pm-smbus", 64);
>  }
> diff --git a/hw/pm_smbus.h b/hw/pm_smbus.h
> index 4750a40..e3069bf 100644
> --- a/hw/pm_smbus.h
> +++ b/hw/pm_smbus.h
> @@ -3,6 +3,7 @@
>  
>  typedef struct PMSMBus {
>      i2c_bus *smbus;
> +    MemoryRegion io;
>  
>      uint8_t smb_stat;
>      uint8_t smb_ctl;

With a view to further QOM'ifying these devices, please keep the parent
field separated, i.e.

     i2c_bus *smbus;

+    MemoryRegion io;
+
     uint8_t ...

(Once macros and types are correctly used - as you do for ICH9 below -
smbus would also be renamed to parent_obj for clarity, cf. QOM PHB/IDE
series, and optionally excluded from gtk-doc documentation.)

Regards,
Andreas

> diff --git a/hw/smbus_ich9.c b/hw/smbus_ich9.c
> index 6940583..54e7e12 100644
> --- a/hw/smbus_ich9.c
> +++ b/hw/smbus_ich9.c
[...]
> @@ -54,42 +53,23 @@ static const VMStateDescription vmstate_ich9_smbus = {
>      }
>  };
>  
> -static void ich9_smb_ioport_writeb(void *opaque, hwaddr addr,
> -                                   uint64_t val, unsigned size)
> +static void ich9_smbus_write_config(PCIDevice *d, uint32_t address,
> +                                    uint32_t val, int len)
>  {
> -    ICH9SMBState *s = opaque;
> -    uint8_t hostc = s->dev.config[ICH9_SMB_HOSTC];
> +    ICH9SMBState *s = ICH9_SMB_DEVICE(d);
[snip]

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg