| Message ID | 20210907065822.1152443-6-clg@kaod.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | aspeed/smc: Cleanups and QOMification | expand |
On 9/7/21 8:58 AM, Cédric Le Goater wrote: > There is no use for it. Hmmm this is not the correct justification. This devices sits on a bus, so its state will be released when the bus is released. There is no need to release it manually, so we can remove the reference. > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > include/hw/ssi/aspeed_smc.h | 1 - > hw/arm/aspeed.c | 11 +++++------ > 2 files changed, 5 insertions(+), 7 deletions(-) > > diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h > index 0ea536a44c3a..f32f66f9a838 100644 > --- a/include/hw/ssi/aspeed_smc.h > +++ b/include/hw/ssi/aspeed_smc.h > @@ -37,7 +37,6 @@ typedef struct AspeedSMCFlash { > uint32_t size; > > MemoryRegion mmio; > - DeviceState *flash; > } AspeedSMCFlash; > > #define TYPE_ASPEED_SMC "aspeed.smc" > diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c > index 95ce4b1670ac..64c3a7fb66db 100644 > --- a/hw/arm/aspeed.c > +++ b/hw/arm/aspeed.c > @@ -274,18 +274,17 @@ static void aspeed_board_init_flashes(AspeedSMCState *s, > int i ; > > for (i = 0; i < s->num_cs; ++i) { > - AspeedSMCFlash *fl = &s->flashes[i]; > DriveInfo *dinfo = drive_get_next(IF_MTD); > qemu_irq cs_line; > + DeviceState *dev; > > - fl->flash = qdev_new(flashtype); > + dev = qdev_new(flashtype); > if (dinfo) { > - qdev_prop_set_drive(fl->flash, "drive", > - blk_by_legacy_dinfo(dinfo)); > + qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo)); > } > - qdev_realize_and_unref(fl->flash, BUS(s->spi), &error_fatal); > + qdev_realize_and_unref(dev, BUS(s->spi), &error_fatal); > > - cs_line = qdev_get_gpio_in_named(fl->flash, SSI_GPIO_CS, 0); > + cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0); > sysbus_connect_irq(SYS_BUS_DEVICE(s), i + 1, cs_line); > } > } >
On 9/7/21 10:36 AM, Philippe Mathieu-Daudé wrote: > On 9/7/21 8:58 AM, Cédric Le Goater wrote: >> There is no use for it. > > Hmmm this is not the correct justification. > > This devices sits on a bus, so its state will be released when > the bus is released. There is no need to release it manually, > so we can remove the reference. That's what the code is doing AFAIUI. This is just removing the AspeedSMCFlash attribute because it is not used anywhere else than under aspeed_board_init_flashes(). Is there anything else ? I am bit lost by your comment. Thanks, C. > >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> --- >> include/hw/ssi/aspeed_smc.h | 1 - >> hw/arm/aspeed.c | 11 +++++------ >> 2 files changed, 5 insertions(+), 7 deletions(-) >> >> diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h >> index 0ea536a44c3a..f32f66f9a838 100644 >> --- a/include/hw/ssi/aspeed_smc.h >> +++ b/include/hw/ssi/aspeed_smc.h >> @@ -37,7 +37,6 @@ typedef struct AspeedSMCFlash { >> uint32_t size; >> >> MemoryRegion mmio; >> - DeviceState *flash; >> } AspeedSMCFlash; >> >> #define TYPE_ASPEED_SMC "aspeed.smc" >> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c >> index 95ce4b1670ac..64c3a7fb66db 100644 >> --- a/hw/arm/aspeed.c >> +++ b/hw/arm/aspeed.c >> @@ -274,18 +274,17 @@ static void aspeed_board_init_flashes(AspeedSMCState *s, >> int i ; >> >> for (i = 0; i < s->num_cs; ++i) { >> - AspeedSMCFlash *fl = &s->flashes[i]; >> DriveInfo *dinfo = drive_get_next(IF_MTD); >> qemu_irq cs_line; >> + DeviceState *dev; >> >> - fl->flash = qdev_new(flashtype); >> + dev = qdev_new(flashtype); >> if (dinfo) { >> - qdev_prop_set_drive(fl->flash, "drive", >> - blk_by_legacy_dinfo(dinfo)); >> + qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo)); >> } >> - qdev_realize_and_unref(fl->flash, BUS(s->spi), &error_fatal); >> + qdev_realize_and_unref(dev, BUS(s->spi), &error_fatal); >> >> - cs_line = qdev_get_gpio_in_named(fl->flash, SSI_GPIO_CS, 0); >> + cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0); >> sysbus_connect_irq(SYS_BUS_DEVICE(s), i + 1, cs_line); >> } >> } >> >
On 9/7/21 11:40 AM, Cédric Le Goater wrote: > On 9/7/21 10:36 AM, Philippe Mathieu-Daudé wrote: >> On 9/7/21 8:58 AM, Cédric Le Goater wrote: >>> There is no use for it. >> >> Hmmm this is not the correct justification. >> >> This devices sits on a bus, so its state will be released when >> the bus is released. There is no need to release it manually, >> so we can remove the reference. > > That's what the code is doing AFAIUI. > > This is just removing the AspeedSMCFlash attribute because it is > not used anywhere else than under aspeed_board_init_flashes(). > > Is there anything else ? I am bit lost by your comment. I was thinking of d4e1d8f57eb ("hw/arm/tosa: Encapsulate misc GPIO handling in a device"), if the device were not created on a bus, the we'd need to keep this reference, otherwise we'd leak it. Anyhow this is board code where we are not releasing anything. Maybe "There is no need to keep a reference of the flash qdev in the AspeedSMCFlash state: the SPI bus takes ownership and will release its resources. Remove AspeedSMCFlash::flash."? Anyway no big deal with the comment, Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h index 0ea536a44c3a..f32f66f9a838 100644 --- a/include/hw/ssi/aspeed_smc.h +++ b/include/hw/ssi/aspeed_smc.h @@ -37,7 +37,6 @@ typedef struct AspeedSMCFlash { uint32_t size; MemoryRegion mmio; - DeviceState *flash; } AspeedSMCFlash; #define TYPE_ASPEED_SMC "aspeed.smc" diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c index 95ce4b1670ac..64c3a7fb66db 100644 --- a/hw/arm/aspeed.c +++ b/hw/arm/aspeed.c @@ -274,18 +274,17 @@ static void aspeed_board_init_flashes(AspeedSMCState *s, int i ; for (i = 0; i < s->num_cs; ++i) { - AspeedSMCFlash *fl = &s->flashes[i]; DriveInfo *dinfo = drive_get_next(IF_MTD); qemu_irq cs_line; + DeviceState *dev; - fl->flash = qdev_new(flashtype); + dev = qdev_new(flashtype); if (dinfo) { - qdev_prop_set_drive(fl->flash, "drive", - blk_by_legacy_dinfo(dinfo)); + qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo)); } - qdev_realize_and_unref(fl->flash, BUS(s->spi), &error_fatal); + qdev_realize_and_unref(dev, BUS(s->spi), &error_fatal); - cs_line = qdev_get_gpio_in_named(fl->flash, SSI_GPIO_CS, 0); + cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0); sysbus_connect_irq(SYS_BUS_DEVICE(s), i + 1, cs_line); } }
There is no use for it. Signed-off-by: Cédric Le Goater <clg@kaod.org> --- include/hw/ssi/aspeed_smc.h | 1 - hw/arm/aspeed.c | 11 +++++------ 2 files changed, 5 insertions(+), 7 deletions(-)