| Message ID | 20220627123038.1427067-9-cristian.marussi@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SCMIv3.1 Powercap protocol and driver | expand |
On Mon, Jun 27, 2022 at 01:30:37PM +0100, Cristian Marussi wrote: > Add optional .setup and .teardown methods to the scmi_driver descriptor: > such callbacks, if provided, will be called by the SCIM core at driver > registration time, so that, an SCMI driver, registered as usual with the > module_scmi_driver() helper macro, can provide custom callbacks to be > run once for all at module load/unload time to perform specific setup > or teardown operations before/after .probe and .remove steps. > What can't the driver call this setup/teardown on its own before/after calling scmi_driver_register/unregister ? Based on the usage in 9/9, I guess it is mainly to use the module_scmi_driver ? If so, I would avoid using that or have another macro to manage this setup/teardown(once there are multiple users for that). IMO, it doesn't make sense to add callbacks to do things that are outside the scope of scmi drivers. No ?
On Fri, Jul 01, 2022 at 03:09:46PM +0100, Sudeep Holla wrote: > On Mon, Jun 27, 2022 at 01:30:37PM +0100, Cristian Marussi wrote: > > Add optional .setup and .teardown methods to the scmi_driver descriptor: > > such callbacks, if provided, will be called by the SCIM core at driver > > registration time, so that, an SCMI driver, registered as usual with the > > module_scmi_driver() helper macro, can provide custom callbacks to be > > run once for all at module load/unload time to perform specific setup > > or teardown operations before/after .probe and .remove steps. > > > > What can't the driver call this setup/teardown on its own before/after > calling scmi_driver_register/unregister ? > > Based on the usage in 9/9, I guess it is mainly to use the > module_scmi_driver ? If so, I would avoid using that or have another > macro to manage this setup/teardown(once there are multiple users for that). > IMO, it doesn't make sense to add callbacks to do things that are outside > the scope of scmi drivers. No ? > This is exactly what I was doing in fact :D at first ... defining a normal init/exit from where I called what I needed at first and then ivoke the scmi_driver_register()...so bypassing/not using the module_scmi-driver macro indeed...then I realized I needed something similar also for the SCMI Test driver, so I tried to unify; in both cases indeed the required ops to be done before the scmi_driver_register are NOT scmi related things. So I can drop this if you prefer and use bare module_init/exit that calls scmi_driver_register() after having setup what needed for the specific driver initialization (before probe)...I was not really convinced it was worth this level of unification. Thanks, Cristian
On Fri, Jul 01, 2022 at 04:09:05PM +0100, Cristian Marussi wrote: > On Fri, Jul 01, 2022 at 03:09:46PM +0100, Sudeep Holla wrote: > > On Mon, Jun 27, 2022 at 01:30:37PM +0100, Cristian Marussi wrote: > > > Add optional .setup and .teardown methods to the scmi_driver descriptor: > > > such callbacks, if provided, will be called by the SCIM core at driver > > > registration time, so that, an SCMI driver, registered as usual with the > > > module_scmi_driver() helper macro, can provide custom callbacks to be > > > run once for all at module load/unload time to perform specific setup > > > or teardown operations before/after .probe and .remove steps. > > > > > > > What can't the driver call this setup/teardown on its own before/after > > calling scmi_driver_register/unregister ? > > > > > Based on the usage in 9/9, I guess it is mainly to use the > > module_scmi_driver ? If so, I would avoid using that or have another > > macro to manage this setup/teardown(once there are multiple users for that). > > IMO, it doesn't make sense to add callbacks to do things that are outside > > the scope of scmi drivers. No ? > > > > This is exactly what I was doing in fact :D at first ... defining a normal > init/exit from where I called what I needed at first and then ivoke the > scmi_driver_register()...so bypassing/not using the module_scmi-driver macro > indeed...then I realized I needed something similar also for the SCMI Test > driver, so I tried to unify; in both cases indeed the required ops to be > done before the scmi_driver_register are NOT scmi related things. > > So I can drop this if you prefer and use bare module_init/exit that > calls scmi_driver_register() after having setup what needed for the > specific driver initialization (before probe)...I was not really > convinced it was worth this level of unification. > We can add macro for that too if there is another user for this. i.e. if and when we merge the test code using something similar, we can wrap them in a macro module_scmi_driver_setup_teardown(driver, setup, teardown) and simplify things for the users.
On Fri, Jul 01, 2022 at 04:18:29PM +0100, Sudeep Holla wrote: > On Fri, Jul 01, 2022 at 04:09:05PM +0100, Cristian Marussi wrote: > > On Fri, Jul 01, 2022 at 03:09:46PM +0100, Sudeep Holla wrote: > > > On Mon, Jun 27, 2022 at 01:30:37PM +0100, Cristian Marussi wrote: > > > > Add optional .setup and .teardown methods to the scmi_driver descriptor: > > > > such callbacks, if provided, will be called by the SCIM core at driver > > > > registration time, so that, an SCMI driver, registered as usual with the > > > > module_scmi_driver() helper macro, can provide custom callbacks to be > > > > run once for all at module load/unload time to perform specific setup > > > > or teardown operations before/after .probe and .remove steps. > > > > > > > > > > What can't the driver call this setup/teardown on its own before/after > > > calling scmi_driver_register/unregister ? > > > > > > > > Based on the usage in 9/9, I guess it is mainly to use the > > > module_scmi_driver ? If so, I would avoid using that or have another > > > macro to manage this setup/teardown(once there are multiple users for that). > > > IMO, it doesn't make sense to add callbacks to do things that are outside > > > the scope of scmi drivers. No ? > > > > > > > This is exactly what I was doing in fact :D at first ... defining a normal > > init/exit from where I called what I needed at first and then ivoke the > > scmi_driver_register()...so bypassing/not using the module_scmi-driver macro > > indeed...then I realized I needed something similar also for the SCMI Test > > driver, so I tried to unify; in both cases indeed the required ops to be > > done before the scmi_driver_register are NOT scmi related things. > > > > So I can drop this if you prefer and use bare module_init/exit that > > calls scmi_driver_register() after having setup what needed for the > > specific driver initialization (before probe)...I was not really > > convinced it was worth this level of unification. > > > > We can add macro for that too if there is another user for this. i.e. > if and when we merge the test code using something similar, we can > wrap them in a macro module_scmi_driver_setup_teardown(driver, setup, teardown) > and simplify things for the users. > Ok, thanks I'll drop this for this series. Thanks, Cristian
diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c index f6fe723ab869..e95085a66bc4 100644 --- a/drivers/firmware/arm_scmi/bus.c +++ b/drivers/firmware/arm_scmi/bus.c @@ -132,12 +132,21 @@ int scmi_driver_register(struct scmi_driver *driver, struct module *owner, { int retval; - if (!driver->probe) + if (!driver->probe || !driver->id_table) return -EINVAL; + if (driver->setup) { + retval = driver->setup(); + if (retval) + return retval; + } + retval = scmi_protocol_device_request(driver->id_table); - if (retval) + if (retval) { + if (driver->teardown) + driver->teardown(); return retval; + } driver->driver.bus = &scmi_bus_type; driver->driver.name = driver->name; @@ -156,6 +165,8 @@ void scmi_driver_unregister(struct scmi_driver *driver) { driver_unregister(&driver->driver); scmi_protocol_device_unrequest(driver->id_table); + if (driver->teardown) + driver->teardown(); } EXPORT_SYMBOL_GPL(scmi_driver_unregister); diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h index ad9641dbdd25..a922707bdfe8 100644 --- a/include/linux/scmi_protocol.h +++ b/include/linux/scmi_protocol.h @@ -805,6 +805,8 @@ struct scmi_device_id { struct scmi_driver { const char *name; + int (*setup)(void); + void (*teardown)(void); int (*probe)(struct scmi_device *sdev); void (*remove)(struct scmi_device *sdev); const struct scmi_device_id *id_table;
Add optional .setup and .teardown methods to the scmi_driver descriptor: such callbacks, if provided, will be called by the SCIM core at driver registration time, so that, an SCMI driver, registered as usual with the module_scmi_driver() helper macro, can provide custom callbacks to be run once for all at module load/unload time to perform specific setup or teardown operations before/after .probe and .remove steps. Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> --- drivers/firmware/arm_scmi/bus.c | 15 +++++++++++++-- include/linux/scmi_protocol.h | 2 ++ 2 files changed, 15 insertions(+), 2 deletions(-)