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

Miquel Raynal
2023-11-30T10:16:45Z

Hi Frank,

Frank.Li@nxp.com 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@nxp.com>
> ---
> 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