diff mbox series

[05/12] hw/i2c/smbus_ich9: Inline ich9_smb_init() and remove it

Message ID 20230213173033.98762-6-shentey@gmail.com
State New
Headers show
Series ICH9 cleanup | expand

Commit Message

Bernhard Beschow Feb. 13, 2023, 5:30 p.m. UTC 
ich9_smb_init() is a legacy init function, so modernize the code.

Note that the smb_io_base parameter was unused.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/i386/ich9.h |  1 -
 hw/i2c/smbus_ich9.c    | 13 +++----------
 hw/i386/pc_q35.c       | 11 ++++++++---
 3 files changed, 11 insertions(+), 14 deletions(-)

Comments

Corey Minyard Feb. 18, 2023, 8:25 p.m. UTC | #1 
On Mon, Feb 13, 2023 at 06:30:26PM +0100, Bernhard Beschow wrote:
> ich9_smb_init() is a legacy init function, so modernize the code.
> 
> Note that the smb_io_base parameter was unused.

Acked-by: Corey Minyard <cminyard@mvista.com>

> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>  include/hw/i386/ich9.h |  1 -
>  hw/i2c/smbus_ich9.c    | 13 +++----------
>  hw/i386/pc_q35.c       | 11 ++++++++---
>  3 files changed, 11 insertions(+), 14 deletions(-)
> 
> diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h
> index 05464f6965..52ea116f44 100644
> --- a/include/hw/i386/ich9.h
> +++ b/include/hw/i386/ich9.h
> @@ -9,7 +9,6 @@
>  #include "qom/object.h"
>  
>  void ich9_lpc_pm_init(PCIDevice *pci_lpc, bool smm_enabled);
> -I2CBus *ich9_smb_init(PCIBus *bus, int devfn, uint32_t smb_io_base);
>  
>  void ich9_generate_smi(void);
>  
> diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
> index d29c0f6ffa..f0dd3cb147 100644
> --- a/hw/i2c/smbus_ich9.c
> +++ b/hw/i2c/smbus_ich9.c
> @@ -105,6 +105,9 @@ static void ich9_smbus_realize(PCIDevice *d, Error **errp)
>      pm_smbus_init(&d->qdev, &s->smb, false);
>      pci_register_bar(d, ICH9_SMB_SMB_BASE_BAR, PCI_BASE_ADDRESS_SPACE_IO,
>                       &s->smb.io);
> +
> +    s->smb.set_irq = ich9_smb_set_irq;
> +    s->smb.opaque = s;
>  }
>  
>  static void build_ich9_smb_aml(AcpiDevAmlIf *adev, Aml *scope)
> @@ -137,16 +140,6 @@ static void ich9_smb_class_init(ObjectClass *klass, void *data)
>      adevc->build_dev_aml = build_ich9_smb_aml;
>  }
>  
> -I2CBus *ich9_smb_init(PCIBus *bus, int devfn, uint32_t smb_io_base)
> -{
> -    PCIDevice *d =
> -        pci_create_simple_multifunction(bus, devfn, true, TYPE_ICH9_SMB_DEVICE);
> -    ICH9SMBState *s = ICH9_SMB_DEVICE(d);
> -    s->smb.set_irq = ich9_smb_set_irq;
> -    s->smb.opaque = s;
> -    return s->smb.smbus;
> -}
> -
>  static const TypeInfo ich9_smb_info = {
>      .name   = TYPE_ICH9_SMB_DEVICE,
>      .parent = TYPE_PCI_DEVICE,
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 4af8474f31..85ba8ed951 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -316,10 +316,15 @@ static void pc_q35_init(MachineState *machine)
>      }
>  
>      if (pcms->smbus_enabled) {
> +        PCIDevice *smb;
> +
>          /* TODO: Populate SPD eeprom data.  */
> -        pcms->smbus = ich9_smb_init(host_bus,
> -                                    PCI_DEVFN(ICH9_SMB_DEV, ICH9_SMB_FUNC),
> -                                    0xb100);
> +        smb = pci_create_simple_multifunction(host_bus,
> +                                              PCI_DEVFN(ICH9_SMB_DEV,
> +                                                        ICH9_SMB_FUNC),
> +                                              true, TYPE_ICH9_SMB_DEVICE);
> +        pcms->smbus = I2C_BUS(qdev_get_child_bus(DEVICE(smb), "i2c"));
> +
>          smbus_eeprom_init(pcms->smbus, 8, NULL, 0);
>      }
>  
> -- 
> 2.39.1
> 
>
Philippe Mathieu-Daudé Feb. 19, 2023, 1:45 p.m. UTC | #2 
On 18/2/23 21:25, Corey Minyard wrote:
> On Mon, Feb 13, 2023 at 06:30:26PM +0100, Bernhard Beschow wrote:
>> ich9_smb_init() is a legacy init function, so modernize the code.
>>
>> Note that the smb_io_base parameter was unused.
> 
> Acked-by: Corey Minyard <cminyard@mvista.com>
> 
>>
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   include/hw/i386/ich9.h |  1 -
>>   hw/i2c/smbus_ich9.c    | 13 +++----------
>>   hw/i386/pc_q35.c       | 11 ++++++++---
>>   3 files changed, 11 insertions(+), 14 deletions(-)
>>
>> diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h
>> index 05464f6965..52ea116f44 100644
>> --- a/include/hw/i386/ich9.h
>> +++ b/include/hw/i386/ich9.h
>> @@ -9,7 +9,6 @@
>>   #include "qom/object.h"
>>   
>>   void ich9_lpc_pm_init(PCIDevice *pci_lpc, bool smm_enabled);
>> -I2CBus *ich9_smb_init(PCIBus *bus, int devfn, uint32_t smb_io_base);
>>   
>>   void ich9_generate_smi(void);
>>   
>> diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
>> index d29c0f6ffa..f0dd3cb147 100644
>> --- a/hw/i2c/smbus_ich9.c
>> +++ b/hw/i2c/smbus_ich9.c
>> @@ -105,6 +105,9 @@ static void ich9_smbus_realize(PCIDevice *d, Error **errp)
>>       pm_smbus_init(&d->qdev, &s->smb, false);
>>       pci_register_bar(d, ICH9_SMB_SMB_BASE_BAR, PCI_BASE_ADDRESS_SPACE_IO,
>>                        &s->smb.io);
>> +
>> +    s->smb.set_irq = ich9_smb_set_irq;
>> +    s->smb.opaque = s;

Corey, shouldn't we expand pm_smbus_init() to take set_irq / opaque
arguments?

>>   }
Corey Minyard Feb. 19, 2023, 2:21 p.m. UTC | #3 
On Sun, Feb 19, 2023 at 02:45:44PM +0100, Philippe Mathieu-Daudé wrote:
> On 18/2/23 21:25, Corey Minyard wrote:
> > On Mon, Feb 13, 2023 at 06:30:26PM +0100, Bernhard Beschow wrote:
> > > ich9_smb_init() is a legacy init function, so modernize the code.
> > > 
> > > Note that the smb_io_base parameter was unused.
> > 
> > Acked-by: Corey Minyard <cminyard@mvista.com>
> > 
> > > 
> > > Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> > > ---
> > >   include/hw/i386/ich9.h |  1 -
> > >   hw/i2c/smbus_ich9.c    | 13 +++----------
> > >   hw/i386/pc_q35.c       | 11 ++++++++---
> > >   3 files changed, 11 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h
> > > index 05464f6965..52ea116f44 100644
> > > --- a/include/hw/i386/ich9.h
> > > +++ b/include/hw/i386/ich9.h
> > > @@ -9,7 +9,6 @@
> > >   #include "qom/object.h"
> > >   void ich9_lpc_pm_init(PCIDevice *pci_lpc, bool smm_enabled);
> > > -I2CBus *ich9_smb_init(PCIBus *bus, int devfn, uint32_t smb_io_base);
> > >   void ich9_generate_smi(void);
> > > diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
> > > index d29c0f6ffa..f0dd3cb147 100644
> > > --- a/hw/i2c/smbus_ich9.c
> > > +++ b/hw/i2c/smbus_ich9.c
> > > @@ -105,6 +105,9 @@ static void ich9_smbus_realize(PCIDevice *d, Error **errp)
> > >       pm_smbus_init(&d->qdev, &s->smb, false);
> > >       pci_register_bar(d, ICH9_SMB_SMB_BASE_BAR, PCI_BASE_ADDRESS_SPACE_IO,
> > >                        &s->smb.io);
> > > +
> > > +    s->smb.set_irq = ich9_smb_set_irq;
> > > +    s->smb.opaque = s;
> 
> Corey, shouldn't we expand pm_smbus_init() to take set_irq / opaque
> arguments?

That would be nice, but the other two user of this function,
hw/isa/vt82c686.c and hw/acpi/piix4.c, don't use these fields.
I doubt we are getting any new users.  I'm fine either way, but the
value is not large.

-corey

> 
> > >   }
>
Philippe Mathieu-Daudé Feb. 27, 2023, 11:53 a.m. UTC | #4 
On 19/2/23 15:21, Corey Minyard wrote:
> On Sun, Feb 19, 2023 at 02:45:44PM +0100, Philippe Mathieu-Daudé wrote:
>> On 18/2/23 21:25, Corey Minyard wrote:
>>> On Mon, Feb 13, 2023 at 06:30:26PM +0100, Bernhard Beschow wrote:
>>>> ich9_smb_init() is a legacy init function, so modernize the code.
>>>>
>>>> Note that the smb_io_base parameter was unused.
>>>
>>> Acked-by: Corey Minyard <cminyard@mvista.com>
>>>
>>>>
>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>> ---
>>>>    include/hw/i386/ich9.h |  1 -
>>>>    hw/i2c/smbus_ich9.c    | 13 +++----------
>>>>    hw/i386/pc_q35.c       | 11 ++++++++---
>>>>    3 files changed, 11 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h
>>>> index 05464f6965..52ea116f44 100644
>>>> --- a/include/hw/i386/ich9.h
>>>> +++ b/include/hw/i386/ich9.h
>>>> @@ -9,7 +9,6 @@
>>>>    #include "qom/object.h"
>>>>    void ich9_lpc_pm_init(PCIDevice *pci_lpc, bool smm_enabled);
>>>> -I2CBus *ich9_smb_init(PCIBus *bus, int devfn, uint32_t smb_io_base);
>>>>    void ich9_generate_smi(void);
>>>> diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
>>>> index d29c0f6ffa..f0dd3cb147 100644
>>>> --- a/hw/i2c/smbus_ich9.c
>>>> +++ b/hw/i2c/smbus_ich9.c
>>>> @@ -105,6 +105,9 @@ static void ich9_smbus_realize(PCIDevice *d, Error **errp)
>>>>        pm_smbus_init(&d->qdev, &s->smb, false);
>>>>        pci_register_bar(d, ICH9_SMB_SMB_BASE_BAR, PCI_BASE_ADDRESS_SPACE_IO,
>>>>                         &s->smb.io);
>>>> +
>>>> +    s->smb.set_irq = ich9_smb_set_irq;
>>>> +    s->smb.opaque = s;
>>
>> Corey, shouldn't we expand pm_smbus_init() to take set_irq / opaque
>> arguments?
> 
> That would be nice, but the other two user of this function,
> hw/isa/vt82c686.c and hw/acpi/piix4.c, don't use these fields.
> I doubt we are getting any new users.  I'm fine either way, but the
> value is not large.

The value is in enforcing stricter APIs (providing good examples).

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Corey Minyard Feb. 27, 2023, 7:34 p.m. UTC | #5 
On Mon, Feb 27, 2023 at 12:53:23PM +0100, Philippe Mathieu-Daudé wrote:
> On 19/2/23 15:21, Corey Minyard wrote:
> > On Sun, Feb 19, 2023 at 02:45:44PM +0100, Philippe Mathieu-Daudé wrote:
> > > On 18/2/23 21:25, Corey Minyard wrote:
> > > > On Mon, Feb 13, 2023 at 06:30:26PM +0100, Bernhard Beschow wrote:
> > > > > ich9_smb_init() is a legacy init function, so modernize the code.
> > > > > 
> > > > > Note that the smb_io_base parameter was unused.
> > > > 
> > > > Acked-by: Corey Minyard <cminyard@mvista.com>
> > > > 
> > > > > 
> > > > > Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> > > > > ---
> > > > >    include/hw/i386/ich9.h |  1 -
> > > > >    hw/i2c/smbus_ich9.c    | 13 +++----------
> > > > >    hw/i386/pc_q35.c       | 11 ++++++++---
> > > > >    3 files changed, 11 insertions(+), 14 deletions(-)
> > > > > 
> > > > > diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h
> > > > > index 05464f6965..52ea116f44 100644
> > > > > --- a/include/hw/i386/ich9.h
> > > > > +++ b/include/hw/i386/ich9.h
> > > > > @@ -9,7 +9,6 @@
> > > > >    #include "qom/object.h"
> > > > >    void ich9_lpc_pm_init(PCIDevice *pci_lpc, bool smm_enabled);
> > > > > -I2CBus *ich9_smb_init(PCIBus *bus, int devfn, uint32_t smb_io_base);
> > > > >    void ich9_generate_smi(void);
> > > > > diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
> > > > > index d29c0f6ffa..f0dd3cb147 100644
> > > > > --- a/hw/i2c/smbus_ich9.c
> > > > > +++ b/hw/i2c/smbus_ich9.c
> > > > > @@ -105,6 +105,9 @@ static void ich9_smbus_realize(PCIDevice *d, Error **errp)
> > > > >        pm_smbus_init(&d->qdev, &s->smb, false);
> > > > >        pci_register_bar(d, ICH9_SMB_SMB_BASE_BAR, PCI_BASE_ADDRESS_SPACE_IO,
> > > > >                         &s->smb.io);
> > > > > +
> > > > > +    s->smb.set_irq = ich9_smb_set_irq;
> > > > > +    s->smb.opaque = s;
> > > 
> > > Corey, shouldn't we expand pm_smbus_init() to take set_irq / opaque
> > > arguments?
> > 
> > That would be nice, but the other two user of this function,
> > hw/isa/vt82c686.c and hw/acpi/piix4.c, don't use these fields.
> > I doubt we are getting any new users.  I'm fine either way, but the
> > value is not large.
> 
> The value is in enforcing stricter APIs (providing good examples).

I agree, and the change would be good, but IMHO it's beyond the scope of
this change and would slow things down.

I'll prepare a patch that does this.

Thanks,

-corey

> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
>
diff mbox series

Patch

diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h
index 05464f6965..52ea116f44 100644
--- a/include/hw/i386/ich9.h
+++ b/include/hw/i386/ich9.h
@@ -9,7 +9,6 @@ 
 #include "qom/object.h"
 
 void ich9_lpc_pm_init(PCIDevice *pci_lpc, bool smm_enabled);
-I2CBus *ich9_smb_init(PCIBus *bus, int devfn, uint32_t smb_io_base);
 
 void ich9_generate_smi(void);
 
diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
index d29c0f6ffa..f0dd3cb147 100644
--- a/hw/i2c/smbus_ich9.c
+++ b/hw/i2c/smbus_ich9.c
@@ -105,6 +105,9 @@  static void ich9_smbus_realize(PCIDevice *d, Error **errp)
     pm_smbus_init(&d->qdev, &s->smb, false);
     pci_register_bar(d, ICH9_SMB_SMB_BASE_BAR, PCI_BASE_ADDRESS_SPACE_IO,
                      &s->smb.io);
+
+    s->smb.set_irq = ich9_smb_set_irq;
+    s->smb.opaque = s;
 }
 
 static void build_ich9_smb_aml(AcpiDevAmlIf *adev, Aml *scope)
@@ -137,16 +140,6 @@  static void ich9_smb_class_init(ObjectClass *klass, void *data)
     adevc->build_dev_aml = build_ich9_smb_aml;
 }
 
-I2CBus *ich9_smb_init(PCIBus *bus, int devfn, uint32_t smb_io_base)
-{
-    PCIDevice *d =
-        pci_create_simple_multifunction(bus, devfn, true, TYPE_ICH9_SMB_DEVICE);
-    ICH9SMBState *s = ICH9_SMB_DEVICE(d);
-    s->smb.set_irq = ich9_smb_set_irq;
-    s->smb.opaque = s;
-    return s->smb.smbus;
-}
-
 static const TypeInfo ich9_smb_info = {
     .name   = TYPE_ICH9_SMB_DEVICE,
     .parent = TYPE_PCI_DEVICE,
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 4af8474f31..85ba8ed951 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -316,10 +316,15 @@  static void pc_q35_init(MachineState *machine)
     }
 
     if (pcms->smbus_enabled) {
+        PCIDevice *smb;
+
         /* TODO: Populate SPD eeprom data.  */
-        pcms->smbus = ich9_smb_init(host_bus,
-                                    PCI_DEVFN(ICH9_SMB_DEV, ICH9_SMB_FUNC),
-                                    0xb100);
+        smb = pci_create_simple_multifunction(host_bus,
+                                              PCI_DEVFN(ICH9_SMB_DEV,
+                                                        ICH9_SMB_FUNC),
+                                              true, TYPE_ICH9_SMB_DEVICE);
+        pcms->smbus = I2C_BUS(qdev_get_child_bus(DEVICE(smb), "i2c"));
+
         smbus_eeprom_init(pcms->smbus, 8, NULL, 0);
     }