Re: [PATCH v6 01/37] firmware: arm_scmi: review protocol registration interface
From: Sudeep Holla <hidden>
Date: 2021-03-08 04:39:34
Also in:
lkml
On Tue, Feb 02, 2021 at 10:15:19PM +0000, Cristian Marussi wrote:
quoted hunk ↗ jump to hunk
Extend common protocol registration routines and provide some new generic protocols get/put helpers that can track protocols usage and automatically perform the proper initialization and de-initialization on demand when required. Convert all standard protocols to use this new registration scheme while keeping them all still using the usual initialization logic bound to SCMI devices probing. Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> --- v2 --> v3 - removed new Base protocol initialization, it will be re-introduced later with all other protocols --- drivers/firmware/arm_scmi/base.c | 8 ++ drivers/firmware/arm_scmi/bus.c | 61 ++++++++--- drivers/firmware/arm_scmi/clock.c | 10 +- drivers/firmware/arm_scmi/common.h | 30 +++++- drivers/firmware/arm_scmi/driver.c | 159 +++++++++++++++++++++++++++- drivers/firmware/arm_scmi/perf.c | 10 +- drivers/firmware/arm_scmi/power.c | 10 +- drivers/firmware/arm_scmi/reset.c | 10 +- drivers/firmware/arm_scmi/sensors.c | 8 +- drivers/firmware/arm_scmi/system.c | 8 +- drivers/firmware/arm_scmi/voltage.c | 8 +- include/linux/scmi_protocol.h | 6 +- 12 files changed, 296 insertions(+), 32 deletions(-)diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c index 1377ec76a45d..044aa9e3ebb0 100644 --- a/drivers/firmware/arm_scmi/bus.c +++ b/drivers/firmware/arm_scmi/bus.c@@ -16,7 +16,7 @@ #include "common.h" static DEFINE_IDA(scmi_bus_id); -static DEFINE_IDR(scmi_protocols); +static DEFINE_IDR(scmi_available_protocols);
[nit] Any particular reason for this name change ? Does it hold refer to something different from before ? IIRC, this is list of registered protocols ? Available might refer to the ones advertised to be present by the platform firmware ?
quoted hunk ↗ jump to hunk
static DEFINE_SPINLOCK(protocol_lock); static const struct scmi_device_id *@@ -51,13 +51,29 @@ static int scmi_dev_match(struct device *dev, struct device_driver *drv) return 0; } +const struct scmi_protocol *scmi_get_protocol(int protocol_id) +{ + const struct scmi_protocol *proto; + + proto = idr_find(&scmi_available_protocols, protocol_id); + if (!proto) { + pr_warn("SCMI Protocol 0x%x not found!\n", protocol_id); + return NULL; + } + + pr_debug("GOT SCMI Protocol 0x%x\n", protocol_id); +
[nit] For sake of consistency, s/GOT/Found/ [..]
quoted hunk ↗ jump to hunk
@@ -194,26 +210,45 @@ void scmi_set_handle(struct scmi_device *scmi_dev) scmi_dev->handle = scmi_handle_get(&scmi_dev->dev); } -int scmi_protocol_register(int protocol_id, scmi_prot_init_fn_t fn) +int scmi_protocol_register(const struct scmi_protocol *proto) { int ret; + if (!proto) { + pr_err("invalid protocol\n"); + return -EINVAL; + } + + if (!proto->init && !proto->init_instance) { + pr_err("missing .init() for protocol 0x%x\n", proto->id);
s/.init()/init as it can be init_instance too ?
quoted hunk ↗ jump to hunk
diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c index 4645677d86f1..e8c84cff9922 100644 --- a/drivers/firmware/arm_scmi/clock.c +++ b/drivers/firmware/arm_scmi/clock.c@@ -2,7 +2,7 @@ /* * System Control and Management Interface (SCMI) Clock Protocol * - * Copyright (C) 2018 ARM Ltd. + * Copyright (C) 2018-2020 ARM Ltd.
2021 perhaps ? Few instances are not updated, I prefer to be consistent across all modified scmi firmware driver files. Other than these minor comments, the other changes looks good to me. -- Regards, Sudeep _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel