| Message ID | 1389339867-8399-4-git-send-email-dmitry.torokhov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/09/2014 11:44 PM, Dmitry Torokhov wrote: > Instead of using 2 separate transactions when reading from the device let's > use i2c_transfer. Because we now have single point of failure I had to > change how we collect statistics. I elected to drop control data from the > stats and only track number of bytes read/written for the device data. > > Also, since we are not prepared to deal with short reads and writes change > read_block_data and write_block_data to indicate error if we detect short > transfers. We tried this change once before a couple of years ago, but the conversion was unsuccessful on some older platforms. I've tested it on some more current platforms, though, and it works there. The old platforms are running 2.6.xx series kernels, and don't look likely ever to be updated, So.... Acked-by: Christopher Heiny <cheiny@synaptics.com> > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > drivers/input/rmi4/rmi_i2c.c | 71 ++++++++++++++++++++++++-------------------- > 1 file changed, 39 insertions(+), 32 deletions(-) > > diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c > index c176218..51f5bc8 100644 > --- a/drivers/input/rmi4/rmi_i2c.c > +++ b/drivers/input/rmi4/rmi_i2c.c > @@ -57,22 +57,17 @@ struct rmi_i2c_xport { > */ > static int rmi_set_page(struct rmi_i2c_xport *rmi_i2c, u8 page) > { > - struct rmi_transport_dev *xport = &rmi_i2c->xport; > struct i2c_client *client = rmi_i2c->client; > u8 txbuf[2] = {RMI_PAGE_SELECT_REGISTER, page}; > int retval; > > - dev_dbg(&client->dev, "writes 3 bytes: %02x %02x\n", > - txbuf[0], txbuf[1]); > - xport->stats.tx_count++; > - xport->stats.tx_bytes += sizeof(txbuf); > retval = i2c_master_send(client, txbuf, sizeof(txbuf)); > if (retval != sizeof(txbuf)) { > - xport->stats.tx_errs++; > dev_err(&client->dev, > "%s: set page failed: %d.", __func__, retval); > return (retval < 0) ? retval : -EIO; > } > + > rmi_i2c->page = page; > return 0; > } > @@ -107,22 +102,27 @@ static int rmi_i2c_write_block(struct rmi_transport_dev *xport, u16 addr, > > if (RMI_I2C_PAGE(addr) != rmi_i2c->page) { > retval = rmi_set_page(rmi_i2c, RMI_I2C_PAGE(addr)); > - if (retval < 0) > + if (retval) > goto exit; > } > > + retval = i2c_master_send(client, rmi_i2c->tx_buf, tx_size); > + if (retval == tx_size) > + retval = 0; > + else if (retval >= 0) > + retval = -EIO; > + > +exit: > dev_dbg(&client->dev, > - "writes %zd bytes at %#06x: %*ph\n", len, addr, (int)len, buf); > + "write %zd bytes at %#06x: %d (%*ph)\n", > + len, addr, retval, (int)len, buf); > > xport->stats.tx_count++; > - xport->stats.tx_bytes += tx_size; > - retval = i2c_master_send(client, rmi_i2c->tx_buf, tx_size); > - if (retval < 0) > + if (retval) > xport->stats.tx_errs++; > else > - retval--; /* don't count the address byte */ > + xport->stats.tx_bytes += len; > > -exit: > mutex_unlock(&rmi_i2c->page_mutex); > return retval; > } > @@ -133,40 +133,47 @@ static int rmi_i2c_read_block(struct rmi_transport_dev *xport, u16 addr, > struct rmi_i2c_xport *rmi_i2c = > container_of(xport, struct rmi_i2c_xport, xport); > struct i2c_client *client = rmi_i2c->client; > - u8 txbuf[1] = {addr & 0xff}; > + u8 addr_offset = addr & 0xff; > int retval; > + struct i2c_msg msgs[] = { > + { > + .addr = client->addr, > + .len = sizeof(addr_offset), > + .buf = &addr_offset, > + }, > + { > + .addr = client->addr, > + .flags = I2C_M_RD, > + .len = len, > + .buf = buf, > + }, > + }; > > mutex_lock(&rmi_i2c->page_mutex); > > if (RMI_I2C_PAGE(addr) != rmi_i2c->page) { > retval = rmi_set_page(rmi_i2c, RMI_I2C_PAGE(addr)); > - if (retval < 0) > + if (retval) > goto exit; > } > > - dev_dbg(&client->dev, "writes 1 bytes: %02x\n", txbuf[0]); > + retval = i2c_transfer(client->adapter, msgs, sizeof(msgs)); > + if (retval == sizeof(msgs)) > + retval = 0; /* success */ > + else if (retval >= 0) > + retval = -EIO; > > - xport->stats.tx_count++; > - xport->stats.tx_bytes += sizeof(txbuf); > - retval = i2c_master_send(client, txbuf, sizeof(txbuf)); > - if (retval != sizeof(txbuf)) { > - xport->stats.tx_errs++; > - retval = (retval < 0) ? retval : -EIO; > - goto exit; > - } > +exit: > + dev_dbg(&client->dev, > + "read %zd bytes at %#06x: %d (%*ph)\n", > + len, addr, retval, (int)len, buf); > > xport->stats.rx_count++; > - xport->stats.rx_bytes += len; > - > - retval = i2c_master_recv(client, buf, len); > - if (retval < 0) > + if (retval) > xport->stats.rx_errs++; > else > - dev_dbg(&client->dev, > - "read %zd bytes at %#06x: %*ph\n", > - len, addr, (int)len, buf); > + xport->stats.rx_bytes += len; > > -exit: > mutex_unlock(&rmi_i2c->page_mutex); > return retval; > } >
On 01/09/2014 11:44 PM, Dmitry Torokhov wrote: > Instead of using 2 separate transactions when reading from the device let's > use i2c_transfer. Because we now have single point of failure I had to > change how we collect statistics. I elected to drop control data from the > stats and only track number of bytes read/written for the device data. > > Also, since we are not prepared to deal with short reads and writes change > read_block_data and write_block_data to indicate error if we detect short > transfers. > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > drivers/input/rmi4/rmi_i2c.c | 71 ++++++++++++++++++++++++-------------------- > 1 file changed, 39 insertions(+), 32 deletions(-) > > diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c > index c176218..51f5bc8 100644 > --- a/drivers/input/rmi4/rmi_i2c.c > +++ b/drivers/input/rmi4/rmi_i2c.c > @@ -57,22 +57,17 @@ struct rmi_i2c_xport { > */ > static int rmi_set_page(struct rmi_i2c_xport *rmi_i2c, u8 page) > { > - struct rmi_transport_dev *xport = &rmi_i2c->xport; > struct i2c_client *client = rmi_i2c->client; > u8 txbuf[2] = {RMI_PAGE_SELECT_REGISTER, page}; > int retval; > > - dev_dbg(&client->dev, "writes 3 bytes: %02x %02x\n", > - txbuf[0], txbuf[1]); > - xport->stats.tx_count++; > - xport->stats.tx_bytes += sizeof(txbuf); > retval = i2c_master_send(client, txbuf, sizeof(txbuf)); > if (retval != sizeof(txbuf)) { > - xport->stats.tx_errs++; > dev_err(&client->dev, > "%s: set page failed: %d.", __func__, retval); > return (retval < 0) ? retval : -EIO; > } > + > rmi_i2c->page = page; > return 0; > } > @@ -107,22 +102,27 @@ static int rmi_i2c_write_block(struct rmi_transport_dev *xport, u16 addr, > > if (RMI_I2C_PAGE(addr) != rmi_i2c->page) { > retval = rmi_set_page(rmi_i2c, RMI_I2C_PAGE(addr)); > - if (retval < 0) > + if (retval) > goto exit; > } > > + retval = i2c_master_send(client, rmi_i2c->tx_buf, tx_size); > + if (retval == tx_size) > + retval = 0; > + else if (retval >= 0) > + retval = -EIO; > + > +exit: > dev_dbg(&client->dev, > - "writes %zd bytes at %#06x: %*ph\n", len, addr, (int)len, buf); > + "write %zd bytes at %#06x: %d (%*ph)\n", > + len, addr, retval, (int)len, buf); > > xport->stats.tx_count++; > - xport->stats.tx_bytes += tx_size; > - retval = i2c_master_send(client, rmi_i2c->tx_buf, tx_size); > - if (retval < 0) > + if (retval) > xport->stats.tx_errs++; > else > - retval--; /* don't count the address byte */ > + xport->stats.tx_bytes += len; > > -exit: > mutex_unlock(&rmi_i2c->page_mutex); > return retval; > } > @@ -133,40 +133,47 @@ static int rmi_i2c_read_block(struct rmi_transport_dev *xport, u16 addr, > struct rmi_i2c_xport *rmi_i2c = > container_of(xport, struct rmi_i2c_xport, xport); > struct i2c_client *client = rmi_i2c->client; > - u8 txbuf[1] = {addr & 0xff}; > + u8 addr_offset = addr & 0xff; > int retval; > + struct i2c_msg msgs[] = { > + { > + .addr = client->addr, > + .len = sizeof(addr_offset), > + .buf = &addr_offset, > + }, > + { > + .addr = client->addr, > + .flags = I2C_M_RD, > + .len = len, > + .buf = buf, > + }, > + }; > > mutex_lock(&rmi_i2c->page_mutex); > > if (RMI_I2C_PAGE(addr) != rmi_i2c->page) { > retval = rmi_set_page(rmi_i2c, RMI_I2C_PAGE(addr)); > - if (retval < 0) > + if (retval) > goto exit; > } > > - dev_dbg(&client->dev, "writes 1 bytes: %02x\n", txbuf[0]); > + retval = i2c_transfer(client->adapter, msgs, sizeof(msgs)); > + if (retval == sizeof(msgs)) I think this should be: retval = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs)); if (retval == ARRAY_SIZE(msgs)) At least, that change resolved some random misbehaviors, including kernel panics. > + retval = 0; /* success */ > + else if (retval >= 0) > + retval = -EIO; > > - xport->stats.tx_count++; > - xport->stats.tx_bytes += sizeof(txbuf); > - retval = i2c_master_send(client, txbuf, sizeof(txbuf)); > - if (retval != sizeof(txbuf)) { > - xport->stats.tx_errs++; > - retval = (retval < 0) ? retval : -EIO; > - goto exit; > - } > +exit: > + dev_dbg(&client->dev, > + "read %zd bytes at %#06x: %d (%*ph)\n", > + len, addr, retval, (int)len, buf); > > xport->stats.rx_count++; > - xport->stats.rx_bytes += len; > - > - retval = i2c_master_recv(client, buf, len); > - if (retval < 0) > + if (retval) > xport->stats.rx_errs++; > else > - dev_dbg(&client->dev, > - "read %zd bytes at %#06x: %*ph\n", > - len, addr, (int)len, buf); > + xport->stats.rx_bytes += len; > > -exit: > mutex_unlock(&rmi_i2c->page_mutex); > return retval; > } >
On Tue, Jan 14, 2014 at 12:26:47AM -0800, Christopher Heiny wrote: > On 01/09/2014 11:44 PM, Dmitry Torokhov wrote: > > > >- dev_dbg(&client->dev, "writes 1 bytes: %02x\n", txbuf[0]); > >+ retval = i2c_transfer(client->adapter, msgs, sizeof(msgs)); > >+ if (retval == sizeof(msgs)) > > I think this should be: > retval = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs)); > if (retval == ARRAY_SIZE(msgs)) > At least, that change resolved some random misbehaviors, including > kernel panics. You are absolutely right, I just committed a fix for that. Thanks.
diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c index c176218..51f5bc8 100644 --- a/drivers/input/rmi4/rmi_i2c.c +++ b/drivers/input/rmi4/rmi_i2c.c @@ -57,22 +57,17 @@ struct rmi_i2c_xport { */ static int rmi_set_page(struct rmi_i2c_xport *rmi_i2c, u8 page) { - struct rmi_transport_dev *xport = &rmi_i2c->xport; struct i2c_client *client = rmi_i2c->client; u8 txbuf[2] = {RMI_PAGE_SELECT_REGISTER, page}; int retval; - dev_dbg(&client->dev, "writes 3 bytes: %02x %02x\n", - txbuf[0], txbuf[1]); - xport->stats.tx_count++; - xport->stats.tx_bytes += sizeof(txbuf); retval = i2c_master_send(client, txbuf, sizeof(txbuf)); if (retval != sizeof(txbuf)) { - xport->stats.tx_errs++; dev_err(&client->dev, "%s: set page failed: %d.", __func__, retval); return (retval < 0) ? retval : -EIO; } + rmi_i2c->page = page; return 0; } @@ -107,22 +102,27 @@ static int rmi_i2c_write_block(struct rmi_transport_dev *xport, u16 addr, if (RMI_I2C_PAGE(addr) != rmi_i2c->page) { retval = rmi_set_page(rmi_i2c, RMI_I2C_PAGE(addr)); - if (retval < 0) + if (retval) goto exit; } + retval = i2c_master_send(client, rmi_i2c->tx_buf, tx_size); + if (retval == tx_size) + retval = 0; + else if (retval >= 0) + retval = -EIO; + +exit: dev_dbg(&client->dev, - "writes %zd bytes at %#06x: %*ph\n", len, addr, (int)len, buf); + "write %zd bytes at %#06x: %d (%*ph)\n", + len, addr, retval, (int)len, buf); xport->stats.tx_count++; - xport->stats.tx_bytes += tx_size; - retval = i2c_master_send(client, rmi_i2c->tx_buf, tx_size); - if (retval < 0) + if (retval) xport->stats.tx_errs++; else - retval--; /* don't count the address byte */ + xport->stats.tx_bytes += len; -exit: mutex_unlock(&rmi_i2c->page_mutex); return retval; } @@ -133,40 +133,47 @@ static int rmi_i2c_read_block(struct rmi_transport_dev *xport, u16 addr, struct rmi_i2c_xport *rmi_i2c = container_of(xport, struct rmi_i2c_xport, xport); struct i2c_client *client = rmi_i2c->client; - u8 txbuf[1] = {addr & 0xff}; + u8 addr_offset = addr & 0xff; int retval; + struct i2c_msg msgs[] = { + { + .addr = client->addr, + .len = sizeof(addr_offset), + .buf = &addr_offset, + }, + { + .addr = client->addr, + .flags = I2C_M_RD, + .len = len, + .buf = buf, + }, + }; mutex_lock(&rmi_i2c->page_mutex); if (RMI_I2C_PAGE(addr) != rmi_i2c->page) { retval = rmi_set_page(rmi_i2c, RMI_I2C_PAGE(addr)); - if (retval < 0) + if (retval) goto exit; } - dev_dbg(&client->dev, "writes 1 bytes: %02x\n", txbuf[0]); + retval = i2c_transfer(client->adapter, msgs, sizeof(msgs)); + if (retval == sizeof(msgs)) + retval = 0; /* success */ + else if (retval >= 0) + retval = -EIO; - xport->stats.tx_count++; - xport->stats.tx_bytes += sizeof(txbuf); - retval = i2c_master_send(client, txbuf, sizeof(txbuf)); - if (retval != sizeof(txbuf)) { - xport->stats.tx_errs++; - retval = (retval < 0) ? retval : -EIO; - goto exit; - } +exit: + dev_dbg(&client->dev, + "read %zd bytes at %#06x: %d (%*ph)\n", + len, addr, retval, (int)len, buf); xport->stats.rx_count++; - xport->stats.rx_bytes += len; - - retval = i2c_master_recv(client, buf, len); - if (retval < 0) + if (retval) xport->stats.rx_errs++; else - dev_dbg(&client->dev, - "read %zd bytes at %#06x: %*ph\n", - len, addr, (int)len, buf); + xport->stats.rx_bytes += len; -exit: mutex_unlock(&rmi_i2c->page_mutex); return retval; }
Instead of using 2 separate transactions when reading from the device let's use i2c_transfer. Because we now have single point of failure I had to change how we collect statistics. I elected to drop control data from the stats and only track number of bytes read/written for the device data. Also, since we are not prepared to deal with short reads and writes change read_block_data and write_block_data to indicate error if we detect short transfers. Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/input/rmi4/rmi_i2c.c | 71 ++++++++++++++++++++++++-------------------- 1 file changed, 39 insertions(+), 32 deletions(-)