Re: [PATCH v4 2/6] i3c: master: svc: add hot join support

[Miquel Raynal]

Hi Frank,

Frank.Li@xxxxxxx wrote on Wed, 29 Nov 2023 17:12:21 -0500:

> Add hot join support for svc master controller. Enable hot join defaultly.

							by default

> User can use sys entry to disable hot join.

Should we do the opposite? Disable hot-join by default and allow
enabling it?

> 
> Signed-off-by: Frank Li <Frank.Li@xxxxxxx>
> ---
>  drivers/i3c/master/svc-i3c-master.c | 58 +++++++++++++++++++++++++++--
>  1 file changed, 54 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> index 6b6bdd163af4f..880c6ae76c013 100644
> --- a/drivers/i3c/master/svc-i3c-master.c
> +++ b/drivers/i3c/master/svc-i3c-master.c
> @@ -128,6 +128,9 @@
>  /* This parameter depends on the implementation and may be tuned */
>  #define SVC_I3C_FIFO_SIZE 16
>  
> +#define SVC_I3C_EVENT_IBI	BIT(0)
> +#define SVC_I3C_EVENT_HOTJOIN	BIT(1)
> +
>  struct svc_i3c_cmd {
>  	u8 addr;
>  	bool rnw;
> @@ -176,6 +179,7 @@ struct svc_i3c_regs_save {
>   * @ibi.tbq_slot: To be queued IBI slot
>   * @ibi.lock: IBI lock
>   * @lock: Transfer lock, protect between IBI work thread and callbacks from master
> + * @enabled_events: Bit masks for enable events (IBI, HotJoin).
>   */
>  struct svc_i3c_master {
>  	struct i3c_master_controller base;
> @@ -205,6 +209,7 @@ struct svc_i3c_master {
>  		spinlock_t lock;
>  	} ibi;
>  	struct mutex lock;
> +	int enabled_events;
>  };
>  
>  /**
> @@ -428,13 +433,16 @@ static void svc_i3c_master_ibi_work(struct work_struct *work)
>  	switch (ibitype) {
>  	case SVC_I3C_MSTATUS_IBITYPE_IBI:
>  		dev = svc_i3c_master_dev_from_addr(master, ibiaddr);
> -		if (!dev)
> +		if (!dev || !(master->enabled_events & SVC_I3C_EVENT_IBI))

If we are going to add new events like that maybe we should have a
helper. So we can then extend the helper with the list of enabled
events?

>  			svc_i3c_master_nack_ibi(master);
>  		else
>  			svc_i3c_master_handle_ibi(master, dev);
>  		break;
>  	case SVC_I3C_MSTATUS_IBITYPE_HOT_JOIN:
> -		svc_i3c_master_ack_ibi(master, false);
> +		if (master->enabled_events & SVC_I3C_EVENT_HOTJOIN)

And you could reuse the helper here

> +			svc_i3c_master_ack_ibi(master, false);
> +		else
> +			svc_i3c_master_nack_ibi(master);
>  		break;
>  	case SVC_I3C_MSTATUS_IBITYPE_MASTER_REQUEST:
>  		svc_i3c_master_nack_ibi(master);
> @@ -471,7 +479,9 @@ static void svc_i3c_master_ibi_work(struct work_struct *work)
>  		svc_i3c_master_emit_stop(master);
>  		break;
>  	case SVC_I3C_MSTATUS_IBITYPE_HOT_JOIN:
> -		queue_work(master->base.wq, &master->hj_work);
> +		svc_i3c_master_emit_stop(master);
> +		if (master->enabled_events & SVC_I3C_EVENT_HOTJOIN)

and here

> +			queue_work(master->base.wq, &master->hj_work);
>  		break;

Thanks,
Miquèl