These are dependent on bugs fixed serial: https://lore.kernel.org/imx/[email protected]/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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Frank Li <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
Hi Frank,
[email protected] 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 <[email protected]>
Thanks,
Miquèl
Hi Frank,
[email protected] 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 <[email protected]>
> ---
> 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
Hi Frank,
[email protected] 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 <[email protected]>
Reviewed-by: Miquel Raynal <[email protected]>
Thanks,
Miquèl
Hi Frank,
[email protected] 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 <[email protected]>
> ---
> 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
On Thu, Nov 30, 2023 at 11:19:59AM +0100, Miquel Raynal wrote:
> Hi Frank,
>
> [email protected] 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 <[email protected]>
> > ---
> > 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/[email protected]/T/#t
>
> ...
>
> > +int i3c_dev_getstatus_format1_locked(struct i3c_dev_desc *dev, u16 *status)
>
> ...
>
> Thanks,
> Miqu?l
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/[email protected]/T/#t
Then this patch should be part of the series you mention.
Thanks,
Miquèl
On Thu, Nov 30, 2023 at 11:19:59AM +0100, Miquel Raynal wrote:
> Hi Frank,
>
> [email protected] 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 <[email protected]>
> > ---
> > 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