Re: [PATCH v7 5/7] drivers/i2c: Add transfer implementation for FSI algorithm
On 05/29/2018 07:08 PM, Andy Shevchenko wrote:
On Wed, May 30, 2018 at 1:24 AM, Eddie James wrote:
From: "Edward A. James"
Execute I2C transfers from the FSI-attached I2C master. Use polling
instead of interrupts as we have no hardware IRQ over FSI.
+ if (msg->flags & I2C_M_RD)
+ cmd |= I2C_CMD_READ;
I think we have a helper for this, though not sure.
Didn't see any other I2C drivers using any helper for msg->flags.
+static int fsi_i2c_write_fifo(struct fsi_i2c_port *port, struct i2c_msg *msg,
+ u8 fifo_count)
+{
+ int write;
+ int rc = 0;
Redundant assignment.
+ struct fsi_i2c_master *i2c = port->master;
+ int bytes_to_write = i2c->fifo_size - fifo_count;
+ int bytes_remaining = msg->len - port->xfrd;
+ if (bytes_to_write > bytes_remaining)
+ bytes_to_write = bytes_remaining;
_write = min(_write, _remaining);
+ while (bytes_to_write > 0) {
+ write = bytes_to_write;
+ /* fsi limited to max 4 byte aligned ops */
+ if (bytes_to_write > 4)
+ write = 4;
+ else if (write == 3)
+ write = 2;
write = min_t(int, 4, rounddown_pow_of_two(bytes_to_write));
Also check it carefully, it might be optimized even more, though I
didn't think much.
I think it is more readable this way, and I'm not convinced the
min(rounddown()) is faster. I did however add a common function to do
this check since it's performed in both the read and write fifo
functions. Let me know what you think on v8.
Thanks,
Eddie
Re: [PATCH v7 5/7] drivers/i2c: Add transfer implementation for FSI algorithm
On 05/29/2018 07:08 PM, Andy Shevchenko wrote:
On Wed, May 30, 2018 at 1:24 AM, Eddie James wrote:
From: "Edward A. James"
Execute I2C transfers from the FSI-attached I2C master. Use polling
instead of interrupts as we have no hardware IRQ over FSI.
+ if (msg->flags & I2C_M_RD)
+ cmd |= I2C_CMD_READ;
I think we have a helper for this, though not sure.
Didn't see any other I2C drivers using any helper for msg->flags.
+static int fsi_i2c_write_fifo(struct fsi_i2c_port *port, struct i2c_msg *msg,
+ u8 fifo_count)
+{
+ int write;
+ int rc = 0;
Redundant assignment.
+ struct fsi_i2c_master *i2c = port->master;
+ int bytes_to_write = i2c->fifo_size - fifo_count;
+ int bytes_remaining = msg->len - port->xfrd;
+ if (bytes_to_write > bytes_remaining)
+ bytes_to_write = bytes_remaining;
_write = min(_write, _remaining);
+ while (bytes_to_write > 0) {
+ write = bytes_to_write;
+ /* fsi limited to max 4 byte aligned ops */
+ if (bytes_to_write > 4)
+ write = 4;
+ else if (write == 3)
+ write = 2;
write = min_t(int, 4, rounddown_pow_of_two(bytes_to_write));
Also check it carefully, it might be optimized even more, though I
didn't think much.
I think it is more readable this way, and I'm not convinced the
min(rounddown()) is faster. I did however add a common function to do
this check since it's performed in both the read and write fifo
functions. Let me know what you think on v8.
Thanks,
Eddie
Re: [PATCH v7 5/7] drivers/i2c: Add transfer implementation for FSI algorithm
On Wed, May 30, 2018 at 1:24 AM, Eddie James wrote:
> From: "Edward A. James"
>
> Execute I2C transfers from the FSI-attached I2C master. Use polling
> instead of interrupts as we have no hardware IRQ over FSI.
> + if (msg->flags & I2C_M_RD)
> + cmd |= I2C_CMD_READ;
I think we have a helper for this, though not sure.
> +static int fsi_i2c_write_fifo(struct fsi_i2c_port *port, struct i2c_msg *msg,
> + u8 fifo_count)
> +{
> + int write;
> + int rc = 0;
Redundant assignment.
> + struct fsi_i2c_master *i2c = port->master;
> + int bytes_to_write = i2c->fifo_size - fifo_count;
> + int bytes_remaining = msg->len - port->xfrd;
> + if (bytes_to_write > bytes_remaining)
> + bytes_to_write = bytes_remaining;
_write = min(_write, _remaining);
> + while (bytes_to_write > 0) {
> + write = bytes_to_write;
> + /* fsi limited to max 4 byte aligned ops */
> + if (bytes_to_write > 4)
> + write = 4;
> + else if (write == 3)
> + write = 2;
write = min_t(int, 4, rounddown_pow_of_two(bytes_to_write));
Also check it carefully, it might be optimized even more, though I
didn't think much.
> + }
> + return rc;
How it can be non-zero?
> +}
> +
> +static int fsi_i2c_read_fifo(struct fsi_i2c_port *port, struct i2c_msg *msg,
> +u8 fifo_count)
> +{
> + int read;
> + int rc = 0;
Redundant assignment.
> + struct fsi_i2c_master *i2c = port->master;
> + int xfr_remaining = msg->len - port->xfrd;
> + u32 dummy;
> +
> + while (fifo_count) {
> + read = fifo_count;
> + /* fsi limited to max 4 byte aligned ops */
> + if (fifo_count > 4)
> + read = 4;
> + else if (read == 3)
> + read = 2;
See above for write case and do in similar way.
> +
> + if (xfr_remaining) {
> + if (xfr_remaining < read)
> + read = xfr_remaining;
read = min(read, _remaining);
> +
> + rc = fsi_device_read(i2c->fsi, I2C_FSI_FIFO,
> +>buf[port->xfrd], read);
> + if (rc)
> + return rc;
> +
> + port->xfrd += read;
> + xfr_remaining -= read;
> + } else {
> + /* no more buffer but data in fifo, need to clear it
> */
> + rc = fsi_device_read(i2c->fsi, I2C_FSI_FIFO, ,
> +read);
> + if (rc)
> + return rc;
> + }
> +
> + fifo_count -= read;
> + }
> +
> + return rc;
How non-zero possible here?
> +}
> + if (port->xfrd < msg->len)
> + rc = -ENODATA;
> + else
> + rc = msg->len;
> +
> + return rc;
if (...)
return -ENODATA;
return msg->len;
?
> + u32 status = 0;
Redundant assignment.
> + return -ETIME;
ETIMEDOUT ?
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v7 5/7] drivers/i2c: Add transfer implementation for FSI algorithm
On Wed, May 30, 2018 at 1:24 AM, Eddie James wrote:
> From: "Edward A. James"
>
> Execute I2C transfers from the FSI-attached I2C master. Use polling
> instead of interrupts as we have no hardware IRQ over FSI.
> + if (msg->flags & I2C_M_RD)
> + cmd |= I2C_CMD_READ;
I think we have a helper for this, though not sure.
> +static int fsi_i2c_write_fifo(struct fsi_i2c_port *port, struct i2c_msg *msg,
> + u8 fifo_count)
> +{
> + int write;
> + int rc = 0;
Redundant assignment.
> + struct fsi_i2c_master *i2c = port->master;
> + int bytes_to_write = i2c->fifo_size - fifo_count;
> + int bytes_remaining = msg->len - port->xfrd;
> + if (bytes_to_write > bytes_remaining)
> + bytes_to_write = bytes_remaining;
_write = min(_write, _remaining);
> + while (bytes_to_write > 0) {
> + write = bytes_to_write;
> + /* fsi limited to max 4 byte aligned ops */
> + if (bytes_to_write > 4)
> + write = 4;
> + else if (write == 3)
> + write = 2;
write = min_t(int, 4, rounddown_pow_of_two(bytes_to_write));
Also check it carefully, it might be optimized even more, though I
didn't think much.
> + }
> + return rc;
How it can be non-zero?
> +}
> +
> +static int fsi_i2c_read_fifo(struct fsi_i2c_port *port, struct i2c_msg *msg,
> +u8 fifo_count)
> +{
> + int read;
> + int rc = 0;
Redundant assignment.
> + struct fsi_i2c_master *i2c = port->master;
> + int xfr_remaining = msg->len - port->xfrd;
> + u32 dummy;
> +
> + while (fifo_count) {
> + read = fifo_count;
> + /* fsi limited to max 4 byte aligned ops */
> + if (fifo_count > 4)
> + read = 4;
> + else if (read == 3)
> + read = 2;
See above for write case and do in similar way.
> +
> + if (xfr_remaining) {
> + if (xfr_remaining < read)
> + read = xfr_remaining;
read = min(read, _remaining);
> +
> + rc = fsi_device_read(i2c->fsi, I2C_FSI_FIFO,
> +>buf[port->xfrd], read);
> + if (rc)
> + return rc;
> +
> + port->xfrd += read;
> + xfr_remaining -= read;
> + } else {
> + /* no more buffer but data in fifo, need to clear it
> */
> + rc = fsi_device_read(i2c->fsi, I2C_FSI_FIFO, ,
> +read);
> + if (rc)
> + return rc;
> + }
> +
> + fifo_count -= read;
> + }
> +
> + return rc;
How non-zero possible here?
> +}
> + if (port->xfrd < msg->len)
> + rc = -ENODATA;
> + else
> + rc = msg->len;
> +
> + return rc;
if (...)
return -ENODATA;
return msg->len;
?
> + u32 status = 0;
Redundant assignment.
> + return -ETIME;
ETIMEDOUT ?
--
With Best Regards,
Andy Shevchenko
[PATCH v7 5/7] drivers/i2c: Add transfer implementation for FSI algorithm
From: "Edward A. James"
Execute I2C transfers from the FSI-attached I2C master. Use polling
instead of interrupts as we have no hardware IRQ over FSI.
Signed-off-by: Edward A. James
---
drivers/i2c/busses/i2c-fsi.c | 203 ++-
1 file changed, 201 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/busses/i2c-fsi.c b/drivers/i2c/busses/i2c-fsi.c
index c0b17df..78e3586 100644
--- a/drivers/i2c/busses/i2c-fsi.c
+++ b/drivers/i2c/busses/i2c-fsi.c
@@ -150,6 +150,7 @@ struct fsi_i2c_port {
struct i2c_adapter adapter;
struct fsi_i2c_master *master;
u16 port;
+ u16 xfrd;
};
static int fsi_i2c_read_reg(struct fsi_device *fsi, unsigned int reg,
@@ -234,6 +235,103 @@ static int fsi_i2c_set_port(struct fsi_i2c_port *port)
return rc;
}
+static int fsi_i2c_start(struct fsi_i2c_port *port, struct i2c_msg *msg,
+bool stop)
+{
+ int rc;
+ struct fsi_i2c_master *i2c = port->master;
+ u32 cmd = I2C_CMD_WITH_START | I2C_CMD_WITH_ADDR;
+
+ port->xfrd = 0;
+
+ if (msg->flags & I2C_M_RD)
+ cmd |= I2C_CMD_READ;
+
+ if (stop || msg->flags & I2C_M_STOP)
+ cmd |= I2C_CMD_WITH_STOP;
+
+ cmd = SETFIELD(I2C_CMD_ADDR, cmd, msg->addr >> 1);
+ cmd = SETFIELD(I2C_CMD_LEN, cmd, msg->len);
+
+ rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_CMD, );
+
+ return rc;
+}
+
+static int fsi_i2c_write_fifo(struct fsi_i2c_port *port, struct i2c_msg *msg,
+ u8 fifo_count)
+{
+ int write;
+ int rc = 0;
+ struct fsi_i2c_master *i2c = port->master;
+ int bytes_to_write = i2c->fifo_size - fifo_count;
+ int bytes_remaining = msg->len - port->xfrd;
+
+ if (bytes_to_write > bytes_remaining)
+ bytes_to_write = bytes_remaining;
+
+ while (bytes_to_write > 0) {
+ write = bytes_to_write;
+ /* fsi limited to max 4 byte aligned ops */
+ if (bytes_to_write > 4)
+ write = 4;
+ else if (write == 3)
+ write = 2;
+
+ rc = fsi_device_write(i2c->fsi, I2C_FSI_FIFO,
+ >buf[port->xfrd], write);
+ if (rc)
+ return rc;
+
+ port->xfrd += write;
+ bytes_to_write -= write;
+ }
+
+ return rc;
+}
+
+static int fsi_i2c_read_fifo(struct fsi_i2c_port *port, struct i2c_msg *msg,
+u8 fifo_count)
+{
+ int read;
+ int rc = 0;
+ struct fsi_i2c_master *i2c = port->master;
+ int xfr_remaining = msg->len - port->xfrd;
+ u32 dummy;
+
+ while (fifo_count) {
+ read = fifo_count;
+ /* fsi limited to max 4 byte aligned ops */
+ if (fifo_count > 4)
+ read = 4;
+ else if (read == 3)
+ read = 2;
+
+ if (xfr_remaining) {
+ if (xfr_remaining < read)
+ read = xfr_remaining;
+
+ rc = fsi_device_read(i2c->fsi, I2C_FSI_FIFO,
+>buf[port->xfrd], read);
+ if (rc)
+ return rc;
+
+ port->xfrd += read;
+ xfr_remaining -= read;
+ } else {
+ /* no more buffer but data in fifo, need to clear it */
+ rc = fsi_device_read(i2c->fsi, I2C_FSI_FIFO, ,
+read);
+ if (rc)
+ return rc;
+ }
+
+ fifo_count -= read;
+ }
+
+ return rc;
+}
+
static int fsi_i2c_reset_bus(struct fsi_i2c_master *i2c)
{
int i, rc;
@@ -396,17 +494,118 @@ static int fsi_i2c_abort(struct fsi_i2c_port *port, u32
status)
return -ETIME;
}
+static int fsi_i2c_handle_status(struct fsi_i2c_port *port,
+struct i2c_msg *msg, u32 status)
+{
+ int rc;
+ u8 fifo_count;
+
+ if (status & I2C_STAT_ERR) {
+ rc = fsi_i2c_abort(port, status);
+ if (rc)
+ return rc;
+
+ if (status & I2C_STAT_INV_CMD)
+ return -EINVAL;
+
+ if (status & (I2C_STAT_PARITY | I2C_STAT_BE_OVERRUN |
+ I2C_STAT_BE_ACCESS))
+ return -EPROTO;
+
+ if (status & I2C_STAT_NACK)
+ return -ENXIO;
+
+ if (status & I2C_STAT_LOST_ARB)
+ return -EAGAIN;
+
+ if (status & I2C_STAT_STOP_ERR)
+ return -EBADMSG;
+
+ return -EIO;
+ }
+
+ if (status &
[PATCH v7 5/7] drivers/i2c: Add transfer implementation for FSI algorithm
From: "Edward A. James"
Execute I2C transfers from the FSI-attached I2C master. Use polling
instead of interrupts as we have no hardware IRQ over FSI.
Signed-off-by: Edward A. James
---
drivers/i2c/busses/i2c-fsi.c | 203 ++-
1 file changed, 201 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/busses/i2c-fsi.c b/drivers/i2c/busses/i2c-fsi.c
index c0b17df..78e3586 100644
--- a/drivers/i2c/busses/i2c-fsi.c
+++ b/drivers/i2c/busses/i2c-fsi.c
@@ -150,6 +150,7 @@ struct fsi_i2c_port {
struct i2c_adapter adapter;
struct fsi_i2c_master *master;
u16 port;
+ u16 xfrd;
};
static int fsi_i2c_read_reg(struct fsi_device *fsi, unsigned int reg,
@@ -234,6 +235,103 @@ static int fsi_i2c_set_port(struct fsi_i2c_port *port)
return rc;
}
+static int fsi_i2c_start(struct fsi_i2c_port *port, struct i2c_msg *msg,
+bool stop)
+{
+ int rc;
+ struct fsi_i2c_master *i2c = port->master;
+ u32 cmd = I2C_CMD_WITH_START | I2C_CMD_WITH_ADDR;
+
+ port->xfrd = 0;
+
+ if (msg->flags & I2C_M_RD)
+ cmd |= I2C_CMD_READ;
+
+ if (stop || msg->flags & I2C_M_STOP)
+ cmd |= I2C_CMD_WITH_STOP;
+
+ cmd = SETFIELD(I2C_CMD_ADDR, cmd, msg->addr >> 1);
+ cmd = SETFIELD(I2C_CMD_LEN, cmd, msg->len);
+
+ rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_CMD, );
+
+ return rc;
+}
+
+static int fsi_i2c_write_fifo(struct fsi_i2c_port *port, struct i2c_msg *msg,
+ u8 fifo_count)
+{
+ int write;
+ int rc = 0;
+ struct fsi_i2c_master *i2c = port->master;
+ int bytes_to_write = i2c->fifo_size - fifo_count;
+ int bytes_remaining = msg->len - port->xfrd;
+
+ if (bytes_to_write > bytes_remaining)
+ bytes_to_write = bytes_remaining;
+
+ while (bytes_to_write > 0) {
+ write = bytes_to_write;
+ /* fsi limited to max 4 byte aligned ops */
+ if (bytes_to_write > 4)
+ write = 4;
+ else if (write == 3)
+ write = 2;
+
+ rc = fsi_device_write(i2c->fsi, I2C_FSI_FIFO,
+ >buf[port->xfrd], write);
+ if (rc)
+ return rc;
+
+ port->xfrd += write;
+ bytes_to_write -= write;
+ }
+
+ return rc;
+}
+
+static int fsi_i2c_read_fifo(struct fsi_i2c_port *port, struct i2c_msg *msg,
+u8 fifo_count)
+{
+ int read;
+ int rc = 0;
+ struct fsi_i2c_master *i2c = port->master;
+ int xfr_remaining = msg->len - port->xfrd;
+ u32 dummy;
+
+ while (fifo_count) {
+ read = fifo_count;
+ /* fsi limited to max 4 byte aligned ops */
+ if (fifo_count > 4)
+ read = 4;
+ else if (read == 3)
+ read = 2;
+
+ if (xfr_remaining) {
+ if (xfr_remaining < read)
+ read = xfr_remaining;
+
+ rc = fsi_device_read(i2c->fsi, I2C_FSI_FIFO,
+>buf[port->xfrd], read);
+ if (rc)
+ return rc;
+
+ port->xfrd += read;
+ xfr_remaining -= read;
+ } else {
+ /* no more buffer but data in fifo, need to clear it */
+ rc = fsi_device_read(i2c->fsi, I2C_FSI_FIFO, ,
+read);
+ if (rc)
+ return rc;
+ }
+
+ fifo_count -= read;
+ }
+
+ return rc;
+}
+
static int fsi_i2c_reset_bus(struct fsi_i2c_master *i2c)
{
int i, rc;
@@ -396,17 +494,118 @@ static int fsi_i2c_abort(struct fsi_i2c_port *port, u32
status)
return -ETIME;
}
+static int fsi_i2c_handle_status(struct fsi_i2c_port *port,
+struct i2c_msg *msg, u32 status)
+{
+ int rc;
+ u8 fifo_count;
+
+ if (status & I2C_STAT_ERR) {
+ rc = fsi_i2c_abort(port, status);
+ if (rc)
+ return rc;
+
+ if (status & I2C_STAT_INV_CMD)
+ return -EINVAL;
+
+ if (status & (I2C_STAT_PARITY | I2C_STAT_BE_OVERRUN |
+ I2C_STAT_BE_ACCESS))
+ return -EPROTO;
+
+ if (status & I2C_STAT_NACK)
+ return -ENXIO;
+
+ if (status & I2C_STAT_LOST_ARB)
+ return -EAGAIN;
+
+ if (status & I2C_STAT_STOP_ERR)
+ return -EBADMSG;
+
+ return -EIO;
+ }
+
+ if (status &
