Re: [PATCH 2/3] firmware: arm_scmi: Move scmi protocols initialisation into the driver

[Sudeep Holla <sudeep.holla@arm.com>]

On Mon, Sep 07, 2020 at 07:06:01PM +0100, Cristian Marussi wrote:
> On Mon, Sep 07, 2020 at 12:29:19PM +0100, Sudeep Holla wrote:
> > In preparation to enable building SCMI as a single module, let us move
> > the SCMI protocol initialisation call into the driver. This enables us
> > to also add de-initialisation of the SCMI protocols.
> > 
> > The main reason for this is to keep it simple instead of maintaining
> > it as separate modules and dealing with all possible initcall races
> > and deferred probe handling. We can move it as separate modules if
> > needed in future.
> > 
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> >  drivers/firmware/arm_scmi/clock.c   |  7 +------
> >  drivers/firmware/arm_scmi/common.h  | 21 +++++++++++++++++++++
> >  drivers/firmware/arm_scmi/driver.c  | 10 ++++++++++
> >  drivers/firmware/arm_scmi/perf.c    |  7 +------
> >  drivers/firmware/arm_scmi/power.c   |  7 +------
> >  drivers/firmware/arm_scmi/reset.c   |  7 +------
> >  drivers/firmware/arm_scmi/sensors.c |  7 +------
> >  7 files changed, 36 insertions(+), 30 deletions(-)
> > 
> > diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
> > index 75e39882746e..606396f748f0 100644
> > --- a/drivers/firmware/arm_scmi/clock.c
> > +++ b/drivers/firmware/arm_scmi/clock.c
> > @@ -364,9 +364,4 @@ static int scmi_clock_protocol_init(struct scmi_handle *handle)
> >  	return 0;
> >  }
> >  
> > -static int __init scmi_clock_init(void)
> > -{
> > -	return scmi_protocol_register(SCMI_PROTOCOL_CLOCK,
> > -				      &scmi_clock_protocol_init);
> > -}
> > -subsys_initcall(scmi_clock_init);
> > +DEFINE_SCMI_PROTOCOL_INIT_EXIT(SCMI_PROTOCOL_CLOCK, clock)
> > diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
> > index 5fa42eba6de7..6d98a6c47005 100644
> > --- a/drivers/firmware/arm_scmi/common.h
> > +++ b/drivers/firmware/arm_scmi/common.h
> > @@ -159,6 +159,27 @@ int scmi_base_protocol_init(struct scmi_handle *h);
> >  int __init scmi_bus_init(void);
> >  void __exit scmi_bus_exit(void);
> >  
> > +#define DECLARE_SCMI_INIT_EXIT(func)		\
> > +	int __init scmi_##func##_init(void);	\
> > +	void __exit scmi_##func##_exit(void)
> > +DECLARE_SCMI_INIT_EXIT(clock);
> > +DECLARE_SCMI_INIT_EXIT(perf);
> > +DECLARE_SCMI_INIT_EXIT(power);
> > +DECLARE_SCMI_INIT_EXIT(reset);
> > +DECLARE_SCMI_INIT_EXIT(sensors);
> > +DECLARE_SCMI_INIT_EXIT(bus);
> > +
> 
> Can we call these protocols' functions (and related macros) something like:
> 
> 	scmi_##PROTO##_load/_unload or _register/_unregister 
> 
> given that in SCMI stack we usually intend something else with protocol
> initialization and in fact each protocol has its own dedicated protocol_init
> function which is called at a different time.
>

Agreed, will fix it.

> 
> > +#define DEFINE_SCMI_PROTOCOL_INIT_EXIT(id, name)	\
> > +int __init scmi_##name##_init(void)			\
> > +{ \
> > +	return scmi_protocol_register((id), &scmi_##name##_protocol_init); \
> > +} \
> > +\
> > +void __exit scmi_##name##_exit(void) \
> > +{ \
> > +	scmi_protocol_unregister((id)); \
> > +}
> > +
> >  /* SCMI Transport */
> >  /**
> >   * struct scmi_chan_info - Structure representing a SCMI channel information
> > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> > index f4d9601c053f..2a1396b74fa5 100644
> > --- a/drivers/firmware/arm_scmi/driver.c
> > +++ b/drivers/firmware/arm_scmi/driver.c
> > @@ -931,6 +931,11 @@ static struct platform_driver scmi_driver = {
> >  static int __init scmi_driver_init(void)
> >  {
> >  	scmi_bus_init();
> > +	scmi_clock_init();
> > +	scmi_perf_init();
> > +	scmi_power_init();
> > +	scmi_reset_init();
> > +	scmi_sensors_init();
> >  
> >  	return platform_driver_register(&scmi_driver);
> >  }
> > @@ -939,6 +944,11 @@ module_init(scmi_driver_init);
> >  static void __exit scmi_driver_exit(void)
> >  {
> >  	scmi_bus_exit();
> 
> Shouldn't this bus_exit() be issued in reverse oerder at the end
> after protocols have being _exited() ?
> 

Ah right, will fix this too.

-- 
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel