[PATCH v4 0/6] i3c: master: some improvment for i3c master

[PATCH v4 0/6] i3c: master: some improvment for i3c master

[Frank Li]

These are dependent on bugs fixed serial: https://lore.kernel.org/imx/20231016153232.2851095-1-Frank.Li@nxp.com/T/#t
There are three major improvement

1. Add actual size in i3c_transfer because i3c allow target early termiate
transfer.
2. Add API for i3c_dev_gettstatus_format1 for i3c comand GET_STATUS.
3. svc master enable hotjoin default

Change from v2 to v3
-fix build warning

All warnings (new ones prefixed by >>):

   drivers/i3c/master/svc-i3c-master.c:212: warning: Function parameter or member 'lock' not described in 'svc_i3c_master'
>> drivers/i3c/master/svc-i3c-master.c:212: warning: Function parameter or member 'enabled_events' not described in 'svc_i3c_master'
   2 warnings as Errors

Change from v1 to v2.
- Hotjoin control by sys entry, default enable hotjoin, which standard i3c
feature, user can disable by echo 0 > /sys/bus/i3c/i3c-0/hotjoin
- use actual_len, add document
- using &xfers[i]
- still keep check cmd->xfer because i2c and ccc transfer, xfer is NULL.
- fixed issue found by checkpatch --strict

Frank Li (6):
  i3c: master: add enable(disable) hot join in sys entry
  i3c: master: svc: add hot join support
  i3c: add actual_len in i3c_priv_xfer
  i3c: svc: rename read_len as actual_len
  i3c: master: svc return actual transfer data len
  i3c: add API i3c_dev_gettstatus_format1() to get target device status

 drivers/i3c/device.c                |  24 ++++++
 drivers/i3c/internals.h             |   1 +
 drivers/i3c/master.c                | 110 ++++++++++++++++++++++++++++
 drivers/i3c/master/svc-i3c-master.c |  92 ++++++++++++++++++-----
 include/linux/i3c/device.h          |   3 +
 include/linux/i3c/master.h          |   5 ++
 6 files changed, 218 insertions(+), 17 deletions(-)

-- 
2.34.1

[PATCH v4 1/6] i3c: master: add enable(disable) hot join in sys entry

[Frank Li]

Add hotjoin entry in sys file system allow user enable/disable hotjoin
feature.

Add (*enable(disable)_hotjoin)() to i3c_master_controller_ops.
Add api i3c_master_enable(disable)_hotjoin();

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/i3c/master.c       | 84 ++++++++++++++++++++++++++++++++++++++
 include/linux/i3c/master.h |  5 +++
 2 files changed, 89 insertions(+)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 08aeb69a78003..ed5e27cd20811 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -526,6 +526,89 @@ static ssize_t i2c_scl_frequency_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(i2c_scl_frequency);
 
+static int i3c_set_hotjoin(struct i3c_master_controller *master, bool enable)
+{
+	int ret;
+
+	if (!master ||
+	    !master->ops ||
+	    !master->ops->enable_hotjoin ||
+	    !master->ops->disable_hotjoin
+	   )
+		return -EINVAL;
+
+	i3c_bus_normaluse_lock(&master->bus);
+
+	if (enable)
+		ret = master->ops->enable_hotjoin(master);
+	else
+		ret = master->ops->disable_hotjoin(master);
+
+	master->hotjoin = enable;
+
+	i3c_bus_normaluse_unlock(&master->bus);
+
+	return ret;
+}
+
+static ssize_t hotjoin_store(struct device *dev, struct device_attribute *attr,
+			     const char *buf, size_t count)
+{
+	struct i3c_bus *i3cbus = dev_to_i3cbus(dev);
+	int ret;
+	long res;
+
+	if (!i3cbus->cur_master)
+		return -EINVAL;
+
+	if (kstrtol(buf, 10, &res))
+		return -EINVAL;
+
+	ret = i3c_set_hotjoin(i3cbus->cur_master->common.master, !!res);
+	if (ret)
+		return ret;
+
+	return count;
+}
+
+/*
+ * i3c_master_enable_hotjoin - Enable hotjoin
+ * @master: I3C master object
+ *
+ * Return: a 0 in case of success, an negative error code otherwise.
+ */
+int i3c_master_enable_hotjoin(struct i3c_master_controller *master)
+{
+	return i3c_set_hotjoin(master, true);
+}
+EXPORT_SYMBOL_GPL(i3c_master_enable_hotjoin);
+
+/*
+ * i3c_master_disable_hotjoin - Disable hotjoin
+ * @master: I3C master object
+ *
+ * Return: a 0 in case of success, an negative error code otherwise.
+ */
+int i3c_master_disable_hotjoin(struct i3c_master_controller *master)
+{
+	return i3c_set_hotjoin(master, false);
+}
+EXPORT_SYMBOL_GPL(i3c_master_disable_hotjoin);
+
+static ssize_t hotjoin_show(struct device *dev, struct device_attribute *da, char *buf)
+{
+	struct i3c_bus *i3cbus = dev_to_i3cbus(dev);
+	ssize_t ret;
+
+	i3c_bus_normaluse_lock(i3cbus);
+	ret = sysfs_emit(buf, "%d\n", i3cbus->cur_master->common.master->hotjoin);
+	i3c_bus_normaluse_unlock(i3cbus);
+
+	return ret;
+}
+
+static DEVICE_ATTR_RW(hotjoin);
+
 static struct attribute *i3c_masterdev_attrs[] = {
 	&dev_attr_mode.attr,
 	&dev_attr_current_master.attr,
@@ -536,6 +619,7 @@ static struct attribute *i3c_masterdev_attrs[] = {
 	&dev_attr_pid.attr,
 	&dev_attr_dynamic_address.attr,
 	&dev_attr_hdrcap.attr,
+	&dev_attr_hotjoin.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(i3c_masterdev);
diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
index 0b52da4f23467..65b8965968af2 100644
--- a/include/linux/i3c/master.h
+++ b/include/linux/i3c/master.h
@@ -452,6 +452,8 @@ struct i3c_master_controller_ops {
 	int (*disable_ibi)(struct i3c_dev_desc *dev);
 	void (*recycle_ibi_slot)(struct i3c_dev_desc *dev,
 				 struct i3c_ibi_slot *slot);
+	int (*enable_hotjoin)(struct i3c_master_controller *master);
+	int (*disable_hotjoin)(struct i3c_master_controller *master);
 };
 
 /**
@@ -487,6 +489,7 @@ struct i3c_master_controller {
 	const struct i3c_master_controller_ops *ops;
 	unsigned int secondary : 1;
 	unsigned int init_done : 1;
+	unsigned int hotjoin: 1;
 	struct {
 		struct list_head i3c;
 		struct list_head i2c;
@@ -543,6 +546,8 @@ int i3c_master_register(struct i3c_master_controller *master,
 			const struct i3c_master_controller_ops *ops,
 			bool secondary);
 void i3c_master_unregister(struct i3c_master_controller *master);
+int i3c_master_enable_hotjoin(struct i3c_master_controller *master);
+int i3c_master_disable_hotjoin(struct i3c_master_controller *master);
 
 /**
  * i3c_dev_get_master_data() - get master private data attached to an I3C
-- 
2.34.1

Re: [PATCH v4 1/6] i3c: master: add enable(disable) hot join in sys entry

[Miquel Raynal]

Hi Frank,

Frank.Li@nxp.com wrote on Wed, 29 Nov 2023 17:12:20 -0500:

> Add hotjoin entry in sys file system allow user enable/disable hotjoin
> feature.
> 
> Add (*enable(disable)_hotjoin)() to i3c_master_controller_ops.
> Add api i3c_master_enable(disable)_hotjoin();
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/i3c/master.c       | 84 ++++++++++++++++++++++++++++++++++++++
>  include/linux/i3c/master.h |  5 +++
>  2 files changed, 89 insertions(+)
> 
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 08aeb69a78003..ed5e27cd20811 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -526,6 +526,89 @@ static ssize_t i2c_scl_frequency_show(struct device *dev,
>  }
>  static DEVICE_ATTR_RO(i2c_scl_frequency);
>  
> +static int i3c_set_hotjoin(struct i3c_master_controller *master, bool enable)
> +{
> +	int ret;
> +
> +	if (!master ||
> +	    !master->ops ||
> +	    !master->ops->enable_hotjoin ||
> +	    !master->ops->disable_hotjoin
> +	   )

Style is wrong here (max limit is 100 chars and the ) should not be on
a new line)

> +		return -EINVAL;
> +
> +	i3c_bus_normaluse_lock(&master->bus);
> +
> +	if (enable)
> +		ret = master->ops->enable_hotjoin(master);
> +	else
> +		ret = master->ops->disable_hotjoin(master);
> +
> +	master->hotjoin = enable;
> +
> +	i3c_bus_normaluse_unlock(&master->bus);
> +
> +	return ret;
> +}
> +
> +static ssize_t hotjoin_store(struct device *dev, struct device_attribute *attr,
> +			     const char *buf, size_t count)
> +{
> +	struct i3c_bus *i3cbus = dev_to_i3cbus(dev);
> +	int ret;
> +	long res;
> +
> +	if (!i3cbus->cur_master)
> +		return -EINVAL;
> +
> +	if (kstrtol(buf, 10, &res))
> +		return -EINVAL;

Isn't there a better approach to get a y/n answer in sysfs?

> +
> +	ret = i3c_set_hotjoin(i3cbus->cur_master->common.master, !!res);
> +	if (ret)
> +		return ret;
> +
> +	return count;
> +}
> +

Thanks,
Miquèl

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

[Frank Li]

Add hot join support for svc master controller. Enable hot join defaultly.
User can use sys entry to disable hot join.

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))
 			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)
+			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)
+			queue_work(master->base.wq, &master->hj_work);
 		break;
 	case SVC_I3C_MSTATUS_IBITYPE_MASTER_REQUEST:
 	default:
@@ -1471,6 +1481,7 @@ static int svc_i3c_master_enable_ibi(struct i3c_dev_desc *dev)
 		return ret;
 	}
 
+	master->enabled_events |= SVC_I3C_EVENT_IBI;
 	svc_i3c_master_enable_interrupts(master, SVC_I3C_MINT_SLVSTART);
 
 	return i3c_master_enec_locked(m, dev->info.dyn_addr, I3C_CCC_EVENT_SIR);
@@ -1482,7 +1493,9 @@ static int svc_i3c_master_disable_ibi(struct i3c_dev_desc *dev)
 	struct svc_i3c_master *master = to_svc_i3c_master(m);
 	int ret;
 
-	svc_i3c_master_disable_interrupts(master);
+	master->enabled_events &= ~SVC_I3C_EVENT_IBI;
+	if (!master->enabled_events)
+		svc_i3c_master_disable_interrupts(master);
 
 	ret = i3c_master_disec_locked(m, dev->info.dyn_addr, I3C_CCC_EVENT_SIR);
 
@@ -1492,6 +1505,39 @@ static int svc_i3c_master_disable_ibi(struct i3c_dev_desc *dev)
 	return ret;
 }
 
+static int svc_i3c_master_enable_hotjoin(struct i3c_master_controller *m)
+{
+	struct svc_i3c_master *master = to_svc_i3c_master(m);
+	int ret;
+
+	ret = pm_runtime_resume_and_get(master->dev);
+	if (ret < 0) {
+		dev_err(master->dev, "<%s> Cannot get runtime PM.\n", __func__);
+		return ret;
+	}
+
+	master->enabled_events |= SVC_I3C_EVENT_HOTJOIN;
+
+	svc_i3c_master_enable_interrupts(master, SVC_I3C_MINT_SLVSTART);
+
+	return 0;
+}
+
+static int svc_i3c_master_disable_hotjoin(struct i3c_master_controller *m)
+{
+	struct svc_i3c_master *master = to_svc_i3c_master(m);
+
+	master->enabled_events &= ~SVC_I3C_EVENT_HOTJOIN;
+
+	if (!master->enabled_events)
+		svc_i3c_master_disable_interrupts(master);
+
+	pm_runtime_mark_last_busy(master->dev);
+	pm_runtime_put_autosuspend(master->dev);
+
+	return 0;
+}
+
 static void svc_i3c_master_recycle_ibi_slot(struct i3c_dev_desc *dev,
 					    struct i3c_ibi_slot *slot)
 {
@@ -1518,6 +1564,8 @@ static const struct i3c_master_controller_ops svc_i3c_master_ops = {
 	.recycle_ibi_slot = svc_i3c_master_recycle_ibi_slot,
 	.enable_ibi = svc_i3c_master_enable_ibi,
 	.disable_ibi = svc_i3c_master_disable_ibi,
+	.enable_hotjoin = svc_i3c_master_enable_hotjoin,
+	.disable_hotjoin = svc_i3c_master_disable_hotjoin,
 };
 
 static int svc_i3c_master_prepare_clks(struct svc_i3c_master *master)
@@ -1630,6 +1678,8 @@ static int svc_i3c_master_probe(struct platform_device *pdev)
 	pm_runtime_mark_last_busy(&pdev->dev);
 	pm_runtime_put_autosuspend(&pdev->dev);
 
+	i3c_master_enable_hotjoin(&master->base);
+
 	return 0;
 
 rpm_disable:
-- 
2.34.1

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

[Miquel Raynal]

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

[PATCH v4 3/6] i3c: add actual_len in i3c_priv_xfer

[Frank Li]

In MIPI I3C Specification:

"Ninth Bit of SDR Target Returned (Read) Data as End-of-Data: In I2C, the
ninth Data bit from Target to Controller is an ACK by the Controller. By
contrast, in I3C this bit allows the Target to end a Read, and allows the
Controller to Abort a Read. In SDR terms, the ninth bit of Read data is
referred to as the T-Bit (for ‘Transition’)"

I3C allow devices early terminate data transfer. So need "actual_len" field
to indicate how much get by i3c_priv_xfer.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 include/linux/i3c/device.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/i3c/device.h b/include/linux/i3c/device.h
index 90fa83464f003..ef6217da8253b 100644
--- a/include/linux/i3c/device.h
+++ b/include/linux/i3c/device.h
@@ -54,6 +54,7 @@ enum i3c_hdr_mode {
  * struct i3c_priv_xfer - I3C SDR private transfer
  * @rnw: encodes the transfer direction. true for a read, false for a write
  * @len: transfer length in bytes of the transfer
+ * @actual_len: actual length in bytes are transferred by the controller
  * @data: input/output buffer
  * @data.in: input buffer. Must point to a DMA-able buffer
  * @data.out: output buffer. Must point to a DMA-able buffer
@@ -62,6 +63,7 @@ enum i3c_hdr_mode {
 struct i3c_priv_xfer {
 	u8 rnw;
 	u16 len;
+	u16 actual_len;
 	union {
 		void *in;
 		const void *out;
-- 
2.34.1

Re: [PATCH v4 3/6] i3c: add actual_len in i3c_priv_xfer

[Miquel Raynal]

Hi Frank,

Frank.Li@nxp.com wrote on Wed, 29 Nov 2023 17:12:22 -0500:

> In MIPI I3C Specification:
> 
> "Ninth Bit of SDR Target Returned (Read) Data as End-of-Data: In I2C, the
> ninth Data bit from Target to Controller is an ACK by the Controller. By
> contrast, in I3C this bit allows the Target to end a Read, and allows the
> Controller to Abort a Read. In SDR terms, the ninth bit of Read data is
> referred to as the T-Bit (for ‘Transition’)"
> 
> I3C allow devices early terminate data transfer. So need "actual_len" field
> to indicate how much get by i3c_priv_xfer.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>

Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>

Thanks,
Miquèl

[PATCH v4 4/6] i3c: svc: rename read_len as actual_len

[Frank Li]

I3C transfer (SDR), target can early terminate read transfer.
I3C transfer (HDR), target can end write transfer.
I2C transfer, target can NACK write transfer.

'actual_len' is better name than 'read_len'.

Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/i3c/master/svc-i3c-master.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index 880c6ae76c013..be742e6734e39 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -137,7 +137,7 @@ struct svc_i3c_cmd {
 	u8 *in;
 	const void *out;
 	unsigned int len;
-	unsigned int read_len;
+	unsigned int actual_len;
 	bool continued;
 };
 
@@ -1033,7 +1033,7 @@ static int svc_i3c_master_write(struct svc_i3c_master *master,
 static int svc_i3c_master_xfer(struct svc_i3c_master *master,
 			       bool rnw, unsigned int xfer_type, u8 addr,
 			       u8 *in, const u8 *out, unsigned int xfer_len,
-			       unsigned int *read_len, bool continued)
+			       unsigned int *actual_len, bool continued)
 {
 	u32 reg;
 	int ret;
@@ -1046,7 +1046,7 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
 	       SVC_I3C_MCTRL_IBIRESP_NACK |
 	       SVC_I3C_MCTRL_DIR(rnw) |
 	       SVC_I3C_MCTRL_ADDR(addr) |
-	       SVC_I3C_MCTRL_RDTERM(*read_len),
+	       SVC_I3C_MCTRL_RDTERM(*actual_len),
 	       master->regs + SVC_I3C_MCTRL);
 
 	ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
@@ -1084,7 +1084,7 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
 		goto emit_stop;
 
 	if (rnw)
-		*read_len = ret;
+		*actual_len = ret;
 
 	ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
 				 SVC_I3C_MSTATUS_COMPLETE(reg), 0, 1000);
@@ -1166,7 +1166,7 @@ static void svc_i3c_master_start_xfer_locked(struct svc_i3c_master *master)
 
 		ret = svc_i3c_master_xfer(master, cmd->rnw, xfer->type,
 					  cmd->addr, cmd->in, cmd->out,
-					  cmd->len, &cmd->read_len,
+					  cmd->len, &cmd->actual_len,
 					  cmd->continued);
 		if (ret)
 			break;
@@ -1252,7 +1252,7 @@ static int svc_i3c_master_send_bdcast_ccc_cmd(struct svc_i3c_master *master,
 	cmd->in = NULL;
 	cmd->out = buf;
 	cmd->len = xfer_len;
-	cmd->read_len = 0;
+	cmd->actual_len = 0;
 	cmd->continued = false;
 
 	mutex_lock(&master->lock);
@@ -1272,7 +1272,7 @@ static int svc_i3c_master_send_direct_ccc_cmd(struct svc_i3c_master *master,
 					      struct i3c_ccc_cmd *ccc)
 {
 	unsigned int xfer_len = ccc->dests[0].payload.len;
-	unsigned int read_len = ccc->rnw ? xfer_len : 0;
+	unsigned int actual_len = ccc->rnw ? xfer_len : 0;
 	struct svc_i3c_xfer *xfer;
 	struct svc_i3c_cmd *cmd;
 	int ret;
@@ -1290,7 +1290,7 @@ static int svc_i3c_master_send_direct_ccc_cmd(struct svc_i3c_master *master,
 	cmd->in = NULL;
 	cmd->out = &ccc->id;
 	cmd->len = 1;
-	cmd->read_len = 0;
+	cmd->actual_len = 0;
 	cmd->continued = true;
 
 	/* Directed message */
@@ -1300,7 +1300,7 @@ static int svc_i3c_master_send_direct_ccc_cmd(struct svc_i3c_master *master,
 	cmd->in = ccc->rnw ? ccc->dests[0].payload.data : NULL;
 	cmd->out = ccc->rnw ? NULL : ccc->dests[0].payload.data,
 	cmd->len = xfer_len;
-	cmd->read_len = read_len;
+	cmd->actual_len = actual_len;
 	cmd->continued = false;
 
 	mutex_lock(&master->lock);
@@ -1309,8 +1309,8 @@ static int svc_i3c_master_send_direct_ccc_cmd(struct svc_i3c_master *master,
 		svc_i3c_master_dequeue_xfer(master, xfer);
 	mutex_unlock(&master->lock);
 
-	if (cmd->read_len != xfer_len)
-		ccc->dests[0].payload.len = cmd->read_len;
+	if (cmd->actual_len != xfer_len)
+		ccc->dests[0].payload.len = cmd->actual_len;
 
 	ret = xfer->ret;
 	svc_i3c_master_free_xfer(xfer);
@@ -1360,7 +1360,7 @@ static int svc_i3c_master_priv_xfers(struct i3c_dev_desc *dev,
 		cmd->in = xfers[i].rnw ? xfers[i].data.in : NULL;
 		cmd->out = xfers[i].rnw ? NULL : xfers[i].data.out;
 		cmd->len = xfers[i].len;
-		cmd->read_len = xfers[i].rnw ? xfers[i].len : 0;
+		cmd->actual_len = xfers[i].rnw ? xfers[i].len : 0;
 		cmd->continued = (i + 1) < nxfers;
 	}
 
@@ -1400,7 +1400,7 @@ static int svc_i3c_master_i2c_xfers(struct i2c_dev_desc *dev,
 		cmd->in = cmd->rnw ? xfers[i].buf : NULL;
 		cmd->out = cmd->rnw ? NULL : xfers[i].buf;
 		cmd->len = xfers[i].len;
-		cmd->read_len = cmd->rnw ? xfers[i].len : 0;
+		cmd->actual_len = cmd->rnw ? xfers[i].len : 0;
 		cmd->continued = (i + 1 < nxfers);
 	}
 
-- 
2.34.1

[PATCH v4 5/6] i3c: master: svc return actual transfer data len

[Frank Li]

I3C allow devices early terminate data transfer. So set "actual_len" to
indicate how much data get by i3c_priv_xfer.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/i3c/master/svc-i3c-master.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index be742e6734e39..9dc12f9936a27 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -138,6 +138,7 @@ struct svc_i3c_cmd {
 	const void *out;
 	unsigned int len;
 	unsigned int actual_len;
+	struct i3c_priv_xfer *xfer;
 	bool continued;
 };
 
@@ -1056,6 +1057,7 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
 
 	if (readl(master->regs + SVC_I3C_MERRWARN) & SVC_I3C_MERRWARN_NACK) {
 		ret = -ENXIO;
+		*actual_len = 0;
 		goto emit_stop;
 	}
 
@@ -1073,6 +1075,7 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
 	 */
 	if (SVC_I3C_MSTATUS_IBIWON(reg)) {
 		ret = -ENXIO;
+		*actual_len = 0;
 		goto emit_stop;
 	}
 
@@ -1168,6 +1171,10 @@ static void svc_i3c_master_start_xfer_locked(struct svc_i3c_master *master)
 					  cmd->addr, cmd->in, cmd->out,
 					  cmd->len, &cmd->actual_len,
 					  cmd->continued);
+		/* cmd->xfer is NULL if I2C or CCC transfer */
+		if (cmd->xfer)
+			cmd->xfer->actual_len = cmd->actual_len;
+
 		if (ret)
 			break;
 	}
@@ -1355,6 +1362,7 @@ static int svc_i3c_master_priv_xfers(struct i3c_dev_desc *dev,
 	for (i = 0; i < nxfers; i++) {
 		struct svc_i3c_cmd *cmd = &xfer->cmds[i];
 
+		cmd->xfer = &xfers[i];
 		cmd->addr = master->addrs[data->index];
 		cmd->rnw = xfers[i].rnw;
 		cmd->in = xfers[i].rnw ? xfers[i].data.in : NULL;
-- 
2.34.1

Re: [PATCH v4 5/6] i3c: master: svc return actual transfer data len

[Miquel Raynal]

Hi Frank,

Frank.Li@nxp.com wrote on Wed, 29 Nov 2023 17:12:24 -0500:

> I3C allow devices early terminate data transfer. So set "actual_len" to
> indicate how much data get by i3c_priv_xfer.

The title prefix should be the same in the two patches addressing the
SVC driver.

With this fixed:

Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
Thanks,
Miquèl

[PATCH v4 6/6] i3c: add API i3c_dev_gettstatus_format1() to get target device status

[Frank Li]

I3C standard 5.1.9.3.15 Get Device Status (GETSTATUS):
Get request for one I3C Target Device to return its current status.

Add API to fetch it with format1.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/i3c/device.c       | 24 ++++++++++++++++++++++++
 drivers/i3c/internals.h    |  1 +
 drivers/i3c/master.c       | 26 ++++++++++++++++++++++++++
 include/linux/i3c/device.h |  1 +
 4 files changed, 52 insertions(+)

diff --git a/drivers/i3c/device.c b/drivers/i3c/device.c
index 1a6a8703dbc3a..aa26cf50ab9c6 100644
--- a/drivers/i3c/device.c
+++ b/drivers/i3c/device.c
@@ -196,6 +196,30 @@ void i3c_device_free_ibi(struct i3c_device *dev)
 }
 EXPORT_SYMBOL_GPL(i3c_device_free_ibi);
 
+/**
+ * i3c_device_getstatus_format1() - Get device status with format 1.
+ * @dev: device for which you want to get status.
+ * @status: I3C status format 1
+ *
+ * Return: 0 in case of success, a negative error core otherwise.
+ */
+int i3c_device_getstatus_format1(struct i3c_device *dev, u16 *status)
+{
+	int ret = -EINVAL;
+
+	if (!status)
+		return -EINVAL;
+
+	i3c_bus_normaluse_lock(dev->bus);
+	if (dev->desc)
+		ret = i3c_dev_getstatus_format1_locked(dev->desc, status);
+
+	i3c_bus_normaluse_unlock(dev->bus);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(i3c_device_getstatus_format1);
+
 /**
  * i3cdev_to_dev() - Returns the device embedded in @i3cdev
  * @i3cdev: I3C device
diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h
index 908a807badaf9..976ad26ca79c2 100644
--- a/drivers/i3c/internals.h
+++ b/drivers/i3c/internals.h
@@ -24,4 +24,5 @@ int i3c_dev_enable_ibi_locked(struct i3c_dev_desc *dev);
 int i3c_dev_request_ibi_locked(struct i3c_dev_desc *dev,
 			       const struct i3c_ibi_setup *req);
 void i3c_dev_free_ibi_locked(struct i3c_dev_desc *dev);
+int i3c_dev_getstatus_format1_locked(struct i3c_dev_desc *dev, u16 *status);
 #endif /* I3C_INTERNAL_H */
diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index ed5e27cd20811..6a16ebdd180b5 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -2924,6 +2924,32 @@ void i3c_dev_free_ibi_locked(struct i3c_dev_desc *dev)
 	dev->ibi = NULL;
 }
 
+int i3c_dev_getstatus_format1_locked(struct i3c_dev_desc *dev, u16 *status)
+{
+	struct i3c_master_controller *master = i3c_dev_get_master(dev);
+	struct i3c_ccc_getstatus *format1;
+	struct i3c_ccc_cmd_dest dest;
+	struct i3c_ccc_cmd cmd;
+	int ret;
+
+	format1 = i3c_ccc_cmd_dest_init(&dest, dev->info.dyn_addr, sizeof(*format1));
+	if (!format1)
+		return -ENOMEM;
+
+	i3c_ccc_cmd_init(&cmd, true, I3C_CCC_GETSTATUS, &dest, 1);
+
+	ret = i3c_master_send_ccc_cmd_locked(master, &cmd);
+	if (ret)
+		goto out;
+
+	*status = be16_to_cpu(format1->status);
+
+out:
+	i3c_ccc_cmd_dest_cleanup(&dest);
+
+	return ret;
+}
+
 static int __init i3c_init(void)
 {
 	int res;
diff --git a/include/linux/i3c/device.h b/include/linux/i3c/device.h
index ef6217da8253b..5f511bd400f11 100644
--- a/include/linux/i3c/device.h
+++ b/include/linux/i3c/device.h
@@ -345,4 +345,5 @@ void i3c_device_free_ibi(struct i3c_device *dev);
 int i3c_device_enable_ibi(struct i3c_device *dev);
 int i3c_device_disable_ibi(struct i3c_device *dev);
 
+int i3c_device_getstatus_format1(struct i3c_device *dev, u16 *status);
 #endif /* I3C_DEV_H */
-- 
2.34.1

Re: [PATCH v4 6/6] i3c: add API i3c_dev_gettstatus_format1() to get target device status

[Miquel Raynal]

Hi Frank,

Frank.Li@nxp.com wrote on Wed, 29 Nov 2023 17:12:25 -0500:

> I3C standard 5.1.9.3.15 Get Device Status (GETSTATUS):
> Get request for one I3C Target Device to return its current status.
> 
> Add API to fetch it with format1.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/i3c/device.c       | 24 ++++++++++++++++++++++++
>  drivers/i3c/internals.h    |  1 +
>  drivers/i3c/master.c       | 26 ++++++++++++++++++++++++++
>  include/linux/i3c/device.h |  1 +
>  4 files changed, 52 insertions(+)
> 
> diff --git a/drivers/i3c/device.c b/drivers/i3c/device.c
> index 1a6a8703dbc3a..aa26cf50ab9c6 100644
> --- a/drivers/i3c/device.c
> +++ b/drivers/i3c/device.c
> @@ -196,6 +196,30 @@ void i3c_device_free_ibi(struct i3c_device *dev)
>  }
>  EXPORT_SYMBOL_GPL(i3c_device_free_ibi);
>  
> +/**
> + * i3c_device_getstatus_format1() - Get device status with format 1.
> + * @dev: device for which you want to get status.
> + * @status: I3C status format 1
> + *
> + * Return: 0 in case of success, a negative error core otherwise.
> + */
> +int i3c_device_getstatus_format1(struct i3c_device *dev, u16 *status)

I'm not a big fan of the opposition between "i3c_device" and "i3c_dev".
Could we clarify the prefixes?

> +{
> +	int ret = -EINVAL;

Init not useful

> +
> +	if (!status)
> +		return -EINVAL;
> +
> +	i3c_bus_normaluse_lock(dev->bus);
> +	if (dev->desc)
> +		ret = i3c_dev_getstatus_format1_locked(dev->desc, status);
> +
> +	i3c_bus_normaluse_unlock(dev->bus);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(i3c_device_getstatus_format1);

There is no user yet. Maybe this needs to be done in another series?
Same below.

...

> +int i3c_dev_getstatus_format1_locked(struct i3c_dev_desc *dev, u16 *status)

...

Thanks,
Miquèl

Re: [PATCH v4 6/6] i3c: add API i3c_dev_gettstatus_format1() to get target device status

[Frank Li]

On Thu, Nov 30, 2023 at 11:19:59AM +0100, Miquel Raynal wrote:
> Hi Frank,
> 
> Frank.Li@nxp.com wrote on Wed, 29 Nov 2023 17:12:25 -0500:
> 
> > I3C standard 5.1.9.3.15 Get Device Status (GETSTATUS):
> > Get request for one I3C Target Device to return its current status.
> > 
> > Add API to fetch it with format1.
> > 
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  drivers/i3c/device.c       | 24 ++++++++++++++++++++++++
> >  drivers/i3c/internals.h    |  1 +
> >  drivers/i3c/master.c       | 26 ++++++++++++++++++++++++++
> >  include/linux/i3c/device.h |  1 +
> >  4 files changed, 52 insertions(+)
> > 
> > diff --git a/drivers/i3c/device.c b/drivers/i3c/device.c
> > index 1a6a8703dbc3a..aa26cf50ab9c6 100644
> > --- a/drivers/i3c/device.c
> > +++ b/drivers/i3c/device.c
> > @@ -196,6 +196,30 @@ void i3c_device_free_ibi(struct i3c_device *dev)
> >  }
> >  EXPORT_SYMBOL_GPL(i3c_device_free_ibi);
> >  
> > +/**
> > + * i3c_device_getstatus_format1() - Get device status with format 1.
> > + * @dev: device for which you want to get status.
> > + * @status: I3C status format 1
> > + *
> > + * Return: 0 in case of success, a negative error core otherwise.
> > + */
> > +int i3c_device_getstatus_format1(struct i3c_device *dev, u16 *status)
> 
> I'm not a big fan of the opposition between "i3c_device" and "i3c_dev".
> Could we clarify the prefixes?

look likes:
	Internal API between device/master/ use i3c_dev
	External API for I3C target device use i3c_device
> 
> > +{
> > +	int ret = -EINVAL;
> 
> Init not useful
> 
> > +
> > +	if (!status)
> > +		return -EINVAL;
> > +
> > +	i3c_bus_normaluse_lock(dev->bus);
> > +	if (dev->desc)
> > +		ret = i3c_dev_getstatus_format1_locked(dev->desc, status);
> > +
> > +	i3c_bus_normaluse_unlock(dev->bus);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(i3c_device_getstatus_format1);
> 
> There is no user yet. Maybe this needs to be done in another series?
> Same below.

See
https://lore.kernel.org/imx/202311070330.5mylauLR-lkp@intel.com/T/#t

> 
> ...
> 
> > +int i3c_dev_getstatus_format1_locked(struct i3c_dev_desc *dev, u16 *status)
> 
> ...
> 
> Thanks,
> Miquèl

Re: [PATCH v4 6/6] i3c: add API i3c_dev_gettstatus_format1() to get target device status

[Miquel Raynal]

Hi Frank,

> > > +
> > > +	if (!status)
> > > +		return -EINVAL;
> > > +
> > > +	i3c_bus_normaluse_lock(dev->bus);
> > > +	if (dev->desc)
> > > +		ret = i3c_dev_getstatus_format1_locked(dev->desc, status);
> > > +
> > > +	i3c_bus_normaluse_unlock(dev->bus);
> > > +
> > > +	return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(i3c_device_getstatus_format1);  
> > 
> > There is no user yet. Maybe this needs to be done in another series?
> > Same below.  
> 
> See
> https://lore.kernel.org/imx/202311070330.5mylauLR-lkp@intel.com/T/#t

Then this patch should be part of the series you mention.

Thanks,
Miquèl

Re: [PATCH v4 6/6] i3c: add API i3c_dev_gettstatus_format1() to get target device statusy

[Frank Li]

On Thu, Nov 30, 2023 at 11:19:59AM +0100, Miquel Raynal wrote:
> Hi Frank,
> 
> Frank.Li@nxp.com wrote on Wed, 29 Nov 2023 17:12:25 -0500:
> 
> > I3C standard 5.1.9.3.15 Get Device Status (GETSTATUS):
> > Get request for one I3C Target Device to return its current status.
> > 
> > Add API to fetch it with format1.
> > 
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  drivers/i3c/device.c       | 24 ++++++++++++++++++++++++
> >  drivers/i3c/internals.h    |  1 +
> >  drivers/i3c/master.c       | 26 ++++++++++++++++++++++++++
> >  include/linux/i3c/device.h |  1 +
> >  4 files changed, 52 insertions(+)
> > 
> > diff --git a/drivers/i3c/device.c b/drivers/i3c/device.c
> > index 1a6a8703dbc3a..aa26cf50ab9c6 100644
> > --- a/drivers/i3c/device.c
> > +++ b/drivers/i3c/device.c
> > @@ -196,6 +196,30 @@ void i3c_device_free_ibi(struct i3c_device *dev)
> >  }
> >  EXPORT_SYMBOL_GPL(i3c_device_free_ibi);
> >  
> > +/**
> > + * i3c_device_getstatus_format1() - Get device status with format 1.
> > + * @dev: device for which you want to get status.
> > + * @status: I3C status format 1
> > + *
> > + * Return: 0 in case of success, a negative error core otherwise.
> > + */
> > +int i3c_device_getstatus_format1(struct i3c_device *dev, u16 *status)
> 
> I'm not a big fan of the opposition between "i3c_device" and "i3c_dev".
> Could we clarify the prefixes?
> 
> > +{
> > +	int ret = -EINVAL;
> 
> Init not useful

No, if dev->desc is NULL, ret will be random value without init.

Frank

> 
> > +
> > +	if (!status)
> > +		return -EINVAL;
> > +
> > +	i3c_bus_normaluse_lock(dev->bus);
> > +	if (dev->desc)
> > +		ret = i3c_dev_getstatus_format1_locked(dev->desc, status);
> > +
> > +	i3c_bus_normaluse_unlock(dev->bus);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(i3c_device_getstatus_format1);
> 
> There is no user yet. Maybe this needs to be done in another series?
> Same below.
> 
> ...
> 
> > +int i3c_dev_getstatus_format1_locked(struct i3c_dev_desc *dev, u16 *status)
> 
> ...
> 
> Thanks,
> Miquèl