Re: [PATCH v14 03/32] firmware: arm_scmi: Refactor __scmi_event_handler_get_ops helper

[Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>]

On Fri, 16 Oct 2026 19:55:24 +0100
Cristian Marussi <cristian.marussi@arm.com> wrote:

> Refactor the __scmi_event_handler_get_ops notification helper to simplify
> its logic and make use of guards for locking.
> 
> Mostly no functional change, in the sense that while the functionality
> remained the same, a richer set of errors is returned where before was
> always a -EINVAL.

Maybe mention why scmi_allocate_event_handler() is now wrapped in
an scmi_allocate_pending_event_handler()

I like the change but beyond logical division of activities it
isn't immediately obvious why that split exists.

One really minor thing inline that I don't care that much about.

Anyhow, both trivial so
Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
>  drivers/firmware/arm_scmi/notify.c | 73 +++++++++++++++++++-----------
>  1 file changed, 46 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/notify.c b/drivers/firmware/arm_scmi/notify.c
> index 7d1bb424a723..2bc2e03df639 100644
> --- a/drivers/firmware/arm_scmi/notify.c
> +++ b/drivers/firmware/arm_scmi/notify.c
> @@ -890,7 +890,6 @@ void scmi_deregister_protocol_events(const struct scmi_handle *handle,
>  
>  /**
>   * scmi_allocate_event_handler()  - Allocate Event handler
> - * @ni: A reference to the notification instance to use
>   * @evt_key: 32bit key uniquely bind to the event identified by the tuple
>   *	     (proto_id, evt_id, src_id)
>   *
> @@ -903,8 +902,7 @@ void scmi_deregister_protocol_events(const struct scmi_handle *handle,
>   * Context: Assumes to be called with @pending_mtx already acquired.
>   * Return: the freshly allocated structure on Success
>   */
> -static struct scmi_event_handler *
> -scmi_allocate_event_handler(struct scmi_notify_instance *ni, u32 evt_key)
> +static struct scmi_event_handler *scmi_allocate_event_handler(u32 evt_key)
>  {
>  	struct scmi_event_handler *hndl;
>  
> @@ -914,8 +912,20 @@ scmi_allocate_event_handler(struct scmi_notify_instance *ni, u32 evt_key)
>  	hndl->key = evt_key;
>  	BLOCKING_INIT_NOTIFIER_HEAD(&hndl->chain);
>  	refcount_set(&hndl->users, 1);
> -	/* New handlers are created pending */
> -	hash_add(ni->pending_events_handlers, &hndl->hash, hndl->key);
> +
> +	return hndl;
> +}
> +
> +static struct scmi_event_handler *
> +scmi_allocate_pending_event_handler(struct scmi_notify_instance *ni,
> +				    u32 evt_key)
> +{
> +	struct scmi_event_handler *hndl;
> +
> +	lockdep_assert_held(&ni->pending_mtx);
> +	hndl =  scmi_allocate_event_handler(evt_key);
> +	if (hndl)
> +		hash_add(ni->pending_events_handlers, &hndl->hash, hndl->key);
Trivial but I'd keep to a strict errors out of line
	if (!hndl)
		return NULL

	hash_add(ni->...
	
	return hndl;

It burns a few lines be saves a few brain cells.

>  
>  	return hndl;
>  }