Re: [PATCH 02/17] hw/arm/spitz: Create SpitzMachineClass abstract base class

[Peter Maydell]

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