Re: [PATCH] spapr: Move spapr_create_nvdimm_dr_connectors() to core machine code

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3025 bytes --]

On Mon, Oct 12, 2020 at 12:15:21PM +0200, Greg Kurz wrote:
> The spapr_create_nvdimm_dr_connectors() function doesn't need to access
> any internal details of the sPAPR NVDIMM implementation. Also, pretty
> much like for the LMBs, only spapr_machine_init() is responsible for the
> creation of DR connectors for NVDIMMs.
> 
> Make this clear by making this function static in hw/ppc/spapr.c.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Hrm, I'm not really seeing the advantage to moving this.  It doesn't
have to be in spapr_nvdimm for data hiding, but it is related, and
spapr.c is kind of huge.

> ---
>  hw/ppc/spapr.c                |   10 ++++++++++
>  hw/ppc/spapr_nvdimm.c         |   11 -----------
>  include/hw/ppc/spapr_nvdimm.h |    1 -
>  3 files changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 63315f2d0fa9..ee716a12af73 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2641,6 +2641,16 @@ static hwaddr spapr_rma_size(SpaprMachineState *spapr, Error **errp)
>      return rma_size;
>  }
>  
> +static void spapr_create_nvdimm_dr_connectors(SpaprMachineState *spapr)
> +{
> +    MachineState *machine = MACHINE(spapr);
> +    int i;
> +
> +    for (i = 0; i < machine->ram_slots; i++) {
> +        spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_PMEM, i);
> +    }
> +}
> +
>  /* pSeries LPAR / sPAPR hardware init */
>  static void spapr_machine_init(MachineState *machine)
>  {
> diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
> index b3a489e9fe18..9e3d94071fe1 100644
> --- a/hw/ppc/spapr_nvdimm.c
> +++ b/hw/ppc/spapr_nvdimm.c
> @@ -106,17 +106,6 @@ void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp)
>      }
>  }
>  
> -void spapr_create_nvdimm_dr_connectors(SpaprMachineState *spapr)
> -{
> -    MachineState *machine = MACHINE(spapr);
> -    int i;
> -
> -    for (i = 0; i < machine->ram_slots; i++) {
> -        spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_PMEM, i);
> -    }
> -}
> -
> -
>  static int spapr_dt_nvdimm(SpaprMachineState *spapr, void *fdt,
>                             int parent_offset, NVDIMMDevice *nvdimm)
>  {
> diff --git a/include/hw/ppc/spapr_nvdimm.h b/include/hw/ppc/spapr_nvdimm.h
> index b834d82f5545..490b19a009f4 100644
> --- a/include/hw/ppc/spapr_nvdimm.h
> +++ b/include/hw/ppc/spapr_nvdimm.h
> @@ -31,6 +31,5 @@ void spapr_dt_persistent_memory(SpaprMachineState *spapr, void *fdt);
>  bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
>                             uint64_t size, Error **errp);
>  void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp);
> -void spapr_create_nvdimm_dr_connectors(SpaprMachineState *spapr);
>  
>  #endif
> 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]