| Message ID | 20230213173033.98762-6-shentey@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ICH9 cleanup | expand |
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 > >
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? >> }
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 > > > > } >
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>
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 --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); }
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(-)