| Message ID | 20231016154632.2851957-5-Frank.Li@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i3c: master: some improvment for i3c master | expand |
Hi On 10/16/23 18:46, Frank Li wrote: > I3C allow devices early terminate data transfer. So set "actual" 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 | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c > index 3570b709cf60..444825aafa6f 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; > }; > I'm thinking would it make sense to combine this and previous patch by removing the read_len/actual_len variable from this structure and use the added one (by the patch 2/5) from "struct i3c_priv_xfer" directly?
Hi Frank, Frank.Li@nxp.com wrote on Mon, 16 Oct 2023 11:46:31 -0400: > I3C allow devices early terminate data transfer. So set "actual" 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 | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c > index 3570b709cf60..444825aafa6f 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; > }; > > @@ -1045,6 +1046,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; > } > > @@ -1062,6 +1064,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; > } > > @@ -1157,6 +1160,9 @@ 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); > + if (cmd->xfer) > + cmd->xfer->actual = cmd->actual_len; Just to be sure, wouldn't it be more natural to always fill cmd->xfer rather than checking it here? > + > if (ret) > break; > } > @@ -1344,6 +1350,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; Please follow the same pattern as below: = &xfers[i] > cmd->addr = master->addrs[data->index]; > cmd->rnw = xfers[i].rnw; > cmd->in = xfers[i].rnw ? xfers[i].data.in : NULL; Thanks, Miquèl
On Tue, Oct 17, 2023 at 11:33:34AM +0300, Jarkko Nikula wrote: > Hi > > On 10/16/23 18:46, Frank Li wrote: > > I3C allow devices early terminate data transfer. So set "actual" 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 | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c > > index 3570b709cf60..444825aafa6f 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; > > }; > I'm thinking would it make sense to combine this and previous patch by > removing the read_len/actual_len variable from this structure and use the > added one (by the patch 2/5) from "struct i3c_priv_xfer" directly? Some I2C transfer and CCC use svc_i3c_cmd, in such case xfer is NULL. Keep len/actual_len is more simple. Frank
On Tue, Oct 17, 2023 at 04:10:07PM +0200, Miquel Raynal wrote: > Hi Frank, > > Frank.Li@nxp.com wrote on Mon, 16 Oct 2023 11:46:31 -0400: > > > I3C allow devices early terminate data transfer. So set "actual" 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 | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c > > index 3570b709cf60..444825aafa6f 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; > > }; > > > > @@ -1045,6 +1046,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; > > } > > > > @@ -1062,6 +1064,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; > > } > > > > @@ -1157,6 +1160,9 @@ 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); > > + if (cmd->xfer) > > + cmd->xfer->actual = cmd->actual_len; > > Just to be sure, wouldn't it be more natural to always fill cmd->xfer > rather than checking it here? cmd->xfer is NULL for i2c and ccc transfer. So need check it. I will add comments here Frank > > > + > > if (ret) > > break; > > } > > @@ -1344,6 +1350,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; > > Please follow the same pattern as below: = &xfers[i] > > > cmd->addr = master->addrs[data->index]; > > cmd->rnw = xfers[i].rnw; > > cmd->in = xfers[i].rnw ? xfers[i].data.in : NULL; > > > Thanks, > Miquèl
diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c index 3570b709cf60..444825aafa6f 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; }; @@ -1045,6 +1046,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; } @@ -1062,6 +1064,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; } @@ -1157,6 +1160,9 @@ 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); + if (cmd->xfer) + cmd->xfer->actual = cmd->actual_len; + if (ret) break; } @@ -1344,6 +1350,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;
I3C allow devices early terminate data transfer. So set "actual" 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 | 7 +++++++ 1 file changed, 7 insertions(+)