| Message ID | 20200628142429.17111-3-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | spitz: fix hacks, fix CID 1421913, various cleanups | expand |
On 6/28/20 4:24 PM, Peter Maydell wrote: > For the four Spitz-family machines (akita, borzoi, spitz, terrier) > create a proper abstract class SpitzMachineClass which encapsulates > the common behaviour, rather than having them all derive directly > from TYPE_MACHINE: > * instead of each machine class setting mc->init to a wrapper > function which calls spitz_common_init() with parameters, > put that data in the SpitzMachineClass and make spitz_common_init > the SpitzMachineClass machine-init function > * move the settings of mc->block_default_type and > mc->ignore_memory_transaction_failures into the SpitzMachineClass > class init rather than repeating them in each machine's class init > > (The motivation is that we're going to want to keep some state in > the SpitzMachineState so we can connect GPIOs between devices created > in one sub-function of the machine init to devices created in a > different sub-function.) > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > hw/arm/spitz.c | 91 ++++++++++++++++++++++++++++++-------------------- > 1 file changed, 55 insertions(+), 36 deletions(-) > > diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c > index 9eaedab79b5..c70e912a33d 100644 > --- a/hw/arm/spitz.c > +++ b/hw/arm/spitz.c > @@ -33,6 +33,26 @@ > #include "exec/address-spaces.h" > #include "cpu.h" > > +enum spitz_model_e { spitz, akita, borzoi, terrier }; > + > +typedef struct { > + MachineClass parent; > + enum spitz_model_e model; Nitpick, I'd drop the not very useful typedef and use directly ...: enum { spitz, akita, borzoi, terrier } model > + int arm_id; > +} SpitzMachineClass; > + > +typedef struct { > + MachineState parent; > +} SpitzMachineState; > + > +#define TYPE_SPITZ_MACHINE "spitz-common" > +#define SPITZ_MACHINE(obj) \ > + OBJECT_CHECK(SpitzMachineState, obj, TYPE_SPITZ_MACHINE) > +#define SPITZ_MACHINE_GET_CLASS(obj) \ > + OBJECT_GET_CLASS(SpitzMachineClass, obj, TYPE_SPITZ_MACHINE) > +#define SPITZ_MACHINE_CLASS(klass) \ > + OBJECT_CLASS_CHECK(SpitzMachineClass, klass, TYPE_SPITZ_MACHINE) > + > #undef REG_FMT > #define REG_FMT "0x%02lx" > > @@ -905,8 +925,6 @@ static void spitz_gpio_setup(PXA2xxState *cpu, int slots) > } > > /* Board init. */ > -enum spitz_model_e { spitz, akita, borzoi, terrier }; > - > #define SPITZ_RAM 0x04000000 > #define SPITZ_ROM 0x00800000 > > @@ -915,9 +933,10 @@ static struct arm_boot_info spitz_binfo = { > .ram_size = 0x04000000, > }; > > -static void spitz_common_init(MachineState *machine, > - enum spitz_model_e model, int arm_id) > +static void spitz_common_init(MachineState *machine) > { > + SpitzMachineClass *smc = SPITZ_MACHINE_GET_CLASS(machine); > + enum spitz_model_e model = smc->model; ... and use 'smc->model' in place. Patch easier to review with 'git-diff -W' [*]. Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> [*] Content of my .gitattributes: $ cat .gitattributes *.c diff=c *.cpp diff=cpp *.m text diff=objc *.h diff=c *.py diff=python *.json text *.pl text diff=perl *.sh text eol=lf *.xml text *.yml text *.bz2 binary
On Mon, 29 Jun 2020 at 09:55, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > On 6/28/20 4:24 PM, Peter Maydell wrote: > > For the four Spitz-family machines (akita, borzoi, spitz, terrier) > > create a proper abstract class SpitzMachineClass which encapsulates > > the common behaviour, rather than having them all derive directly > > from TYPE_MACHINE: > > * instead of each machine class setting mc->init to a wrapper > > function which calls spitz_common_init() with parameters, > > put that data in the SpitzMachineClass and make spitz_common_init > > the SpitzMachineClass machine-init function > > * move the settings of mc->block_default_type and > > mc->ignore_memory_transaction_failures into the SpitzMachineClass > > class init rather than repeating them in each machine's class init > > > > (The motivation is that we're going to want to keep some state in > > the SpitzMachineState so we can connect GPIOs between devices created > > in one sub-function of the machine init to devices created in a > > different sub-function.) > > > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > --- > > hw/arm/spitz.c | 91 ++++++++++++++++++++++++++++++-------------------- > > 1 file changed, 55 insertions(+), 36 deletions(-) > > > > diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c > > index 9eaedab79b5..c70e912a33d 100644 > > --- a/hw/arm/spitz.c > > +++ b/hw/arm/spitz.c > > @@ -33,6 +33,26 @@ > > #include "exec/address-spaces.h" > > #include "cpu.h" > > > > +enum spitz_model_e { spitz, akita, borzoi, terrier }; > > + > > +typedef struct { > > + MachineClass parent; > > + enum spitz_model_e model; > > Nitpick, I'd drop the not very useful typedef and use > directly ...: > > enum { spitz, akita, borzoi, terrier } model This is just code motion, I didn't want to mess with the existing way this enum was defined. > > -static void spitz_common_init(MachineState *machine, > > - enum spitz_model_e model, int arm_id) > > +static void spitz_common_init(MachineState *machine) > > { > > + SpitzMachineClass *smc = SPITZ_MACHINE_GET_CLASS(machine); > > + enum spitz_model_e model = smc->model; > > ... and use 'smc->model' in place. Similarly here I used smc->arm_id in-place because there was only one user, but went for model = smc->model because there were multiple users and it would have put extra changed lines into the patch that I didn't think were necessary. thanks -- PMM
diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c index 9eaedab79b5..c70e912a33d 100644 --- a/hw/arm/spitz.c +++ b/hw/arm/spitz.c @@ -33,6 +33,26 @@ #include "exec/address-spaces.h" #include "cpu.h" +enum spitz_model_e { spitz, akita, borzoi, terrier }; + +typedef struct { + MachineClass parent; + enum spitz_model_e model; + int arm_id; +} SpitzMachineClass; + +typedef struct { + MachineState parent; +} SpitzMachineState; + +#define TYPE_SPITZ_MACHINE "spitz-common" +#define SPITZ_MACHINE(obj) \ + OBJECT_CHECK(SpitzMachineState, obj, TYPE_SPITZ_MACHINE) +#define SPITZ_MACHINE_GET_CLASS(obj) \ + OBJECT_GET_CLASS(SpitzMachineClass, obj, TYPE_SPITZ_MACHINE) +#define SPITZ_MACHINE_CLASS(klass) \ + OBJECT_CLASS_CHECK(SpitzMachineClass, klass, TYPE_SPITZ_MACHINE) + #undef REG_FMT #define REG_FMT "0x%02lx" @@ -905,8 +925,6 @@ static void spitz_gpio_setup(PXA2xxState *cpu, int slots) } /* Board init. */ -enum spitz_model_e { spitz, akita, borzoi, terrier }; - #define SPITZ_RAM 0x04000000 #define SPITZ_ROM 0x00800000 @@ -915,9 +933,10 @@ static struct arm_boot_info spitz_binfo = { .ram_size = 0x04000000, }; -static void spitz_common_init(MachineState *machine, - enum spitz_model_e model, int arm_id) +static void spitz_common_init(MachineState *machine) { + SpitzMachineClass *smc = SPITZ_MACHINE_GET_CLASS(machine); + enum spitz_model_e model = smc->model; PXA2xxState *mpu; DeviceState *scp0, *scp1 = NULL; MemoryRegion *address_space_mem = get_system_memory(); @@ -958,100 +977,100 @@ static void spitz_common_init(MachineState *machine, /* A 4.0 GB microdrive is permanently sitting in CF slot 0. */ spitz_microdrive_attach(mpu, 0); - spitz_binfo.board_id = arm_id; + spitz_binfo.board_id = smc->arm_id; arm_load_kernel(mpu->cpu, machine, &spitz_binfo); sl_bootparam_write(SL_PXA_PARAM_BASE); } -static void spitz_init(MachineState *machine) +static void spitz_common_class_init(ObjectClass *oc, void *data) { - spitz_common_init(machine, spitz, 0x2c9); + MachineClass *mc = MACHINE_CLASS(oc); + + mc->block_default_type = IF_IDE; + mc->ignore_memory_transaction_failures = true; + mc->init = spitz_common_init; } -static void borzoi_init(MachineState *machine) -{ - spitz_common_init(machine, borzoi, 0x33f); -} - -static void akita_init(MachineState *machine) -{ - spitz_common_init(machine, akita, 0x2e8); -} - -static void terrier_init(MachineState *machine) -{ - spitz_common_init(machine, terrier, 0x33f); -} +static const TypeInfo spitz_common_info = { + .name = TYPE_SPITZ_MACHINE, + .parent = TYPE_MACHINE, + .abstract = true, + .instance_size = sizeof(SpitzMachineState), + .class_size = sizeof(SpitzMachineClass), + .class_init = spitz_common_class_init, +}; static void akitapda_class_init(ObjectClass *oc, void *data) { MachineClass *mc = MACHINE_CLASS(oc); + SpitzMachineClass *smc = SPITZ_MACHINE_CLASS(oc); mc->desc = "Sharp SL-C1000 (Akita) PDA (PXA270)"; - mc->init = akita_init; - mc->ignore_memory_transaction_failures = true; mc->default_cpu_type = ARM_CPU_TYPE_NAME("pxa270-c0"); + smc->model = akita; + smc->arm_id = 0x2e8; } static const TypeInfo akitapda_type = { .name = MACHINE_TYPE_NAME("akita"), - .parent = TYPE_MACHINE, + .parent = TYPE_SPITZ_MACHINE, .class_init = akitapda_class_init, }; static void spitzpda_class_init(ObjectClass *oc, void *data) { MachineClass *mc = MACHINE_CLASS(oc); + SpitzMachineClass *smc = SPITZ_MACHINE_CLASS(oc); mc->desc = "Sharp SL-C3000 (Spitz) PDA (PXA270)"; - mc->init = spitz_init; - mc->block_default_type = IF_IDE; - mc->ignore_memory_transaction_failures = true; mc->default_cpu_type = ARM_CPU_TYPE_NAME("pxa270-c0"); + smc->model = spitz; + smc->arm_id = 0x2c9; } static const TypeInfo spitzpda_type = { .name = MACHINE_TYPE_NAME("spitz"), - .parent = TYPE_MACHINE, + .parent = TYPE_SPITZ_MACHINE, .class_init = spitzpda_class_init, }; static void borzoipda_class_init(ObjectClass *oc, void *data) { MachineClass *mc = MACHINE_CLASS(oc); + SpitzMachineClass *smc = SPITZ_MACHINE_CLASS(oc); mc->desc = "Sharp SL-C3100 (Borzoi) PDA (PXA270)"; - mc->init = borzoi_init; - mc->block_default_type = IF_IDE; - mc->ignore_memory_transaction_failures = true; mc->default_cpu_type = ARM_CPU_TYPE_NAME("pxa270-c0"); + smc->model = borzoi; + smc->arm_id = 0x33f; } static const TypeInfo borzoipda_type = { .name = MACHINE_TYPE_NAME("borzoi"), - .parent = TYPE_MACHINE, + .parent = TYPE_SPITZ_MACHINE, .class_init = borzoipda_class_init, }; static void terrierpda_class_init(ObjectClass *oc, void *data) { MachineClass *mc = MACHINE_CLASS(oc); + SpitzMachineClass *smc = SPITZ_MACHINE_CLASS(oc); mc->desc = "Sharp SL-C3200 (Terrier) PDA (PXA270)"; - mc->init = terrier_init; - mc->block_default_type = IF_IDE; - mc->ignore_memory_transaction_failures = true; mc->default_cpu_type = ARM_CPU_TYPE_NAME("pxa270-c5"); + smc->model = terrier; + smc->arm_id = 0x33f; } static const TypeInfo terrierpda_type = { .name = MACHINE_TYPE_NAME("terrier"), - .parent = TYPE_MACHINE, + .parent = TYPE_SPITZ_MACHINE, .class_init = terrierpda_class_init, }; static void spitz_machine_init(void) { + type_register_static(&spitz_common_info); type_register_static(&akitapda_type); type_register_static(&spitzpda_type); type_register_static(&borzoipda_type);
For the four Spitz-family machines (akita, borzoi, spitz, terrier) create a proper abstract class SpitzMachineClass which encapsulates the common behaviour, rather than having them all derive directly from TYPE_MACHINE: * instead of each machine class setting mc->init to a wrapper function which calls spitz_common_init() with parameters, put that data in the SpitzMachineClass and make spitz_common_init the SpitzMachineClass machine-init function * move the settings of mc->block_default_type and mc->ignore_memory_transaction_failures into the SpitzMachineClass class init rather than repeating them in each machine's class init (The motivation is that we're going to want to keep some state in the SpitzMachineState so we can connect GPIOs between devices created in one sub-function of the machine init to devices created in a different sub-function.) Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/arm/spitz.c | 91 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 55 insertions(+), 36 deletions(-) -- 2.20.1