[i2c] [patch 2.6.23-rc5 2/2] i2c-algo-bit read block data bugfix
Jean Delvare
khali at linux-fr.org
Sun Sep 9 16:36:04 CEST 2007
Hi David,
On Fri, 07 Sep 2007 14:59:15 -0700, David Brownell wrote:
> This fixes a bug in the way i2c-algo-bit handles I2C_M_RECV_LEN,
> used to implement i2c_smbus_read_block_data(). Previously, in the
> absence of PEC (rarely used!) it would NAK the "length" byte:
>
> S addr Rd [A] [length] NA
>
> That prevents the subsequent data bytes from being read:
>
> S addr Rd [A] [length] { A [data] }* NA
>
> The primary fix just reorders two code blocks, so the length used
> in the "should I NAK now?" check incorporates the data which it
> just read from the slave device.
>
> However, that move also highlighted other fault handling glitches.
> This fixes those by abstracting the RX path ack/nak logic, so it
> can be used in more than one location. Also, a few CodingStyle
> issues were also resolved.
Most of which I'll have to back out as I want this patch to be as small
as possible: I intend to get it in 2.6.22-stable.
> Signed-off-by: David Brownell <dbrownell at users.sourceforge.net>
>
> ---
> drivers/i2c/algos/i2c-algo-bit.c | 62 +++++++++++++++++++++++----------------
> 1 files changed, 38 insertions(+), 24 deletions(-)
>
> --- at91.orig/drivers/i2c/algos/i2c-algo-bit.c 2007-09-05 16:10:35.000000000 -0700
> +++ at91/drivers/i2c/algos/i2c-algo-bit.c 2007-09-07 10:12:31.000000000 -0700
> @@ -357,13 +357,29 @@ static int sendbytes(struct i2c_adapter
> return wrcount;
> }
>
> +static int acknak(struct i2c_adapter *i2c_adap, u8 byte, int is_ack)
The "byte" parameter is not used.
> +{
> + struct i2c_algo_bit_data *adap = i2c_adap->algo_data;
> +
> + /* assert: sda is high */
> + if (is_ack) /* send ack */
> + setsda(adap, 0);
> + udelay((adap->udelay + 1) / 2);
> + if (sclhi(adap) < 0) { /* timeout */
> + dev_err(&i2c_adap->dev, "readbytes: ack/nak timeout\n");
> + return -ETIMEDOUT;
> + }
> + scllo(adap);
> + return 0;
> +}
> +
> static int readbytes(struct i2c_adapter *i2c_adap, struct i2c_msg *msg)
> {
> int inval;
> - int rdcount=0; /* counts bytes read */
> - struct i2c_algo_bit_data *adap = i2c_adap->algo_data;
> + int rdcount = 0; /* counts bytes read */
> unsigned char *temp = msg->buf;
> int count = msg->len;
> + const unsigned flags = msg->flags;
>
> while (count > 0) {
> inval = i2c_inb(i2c_adap);
> @@ -377,38 +393,36 @@ static int readbytes(struct i2c_adapter
> temp++;
> count--;
>
> - if (msg->flags & I2C_M_NO_RD_ACK) {
> - bit_dbg(2, &i2c_adap->dev, "i2c_inb: 0x%02x\n",
> - inval);
> - continue;
> - }
> -
> - /* assert: sda is high */
> - if (count) /* send ack */
> - setsda(adap, 0);
> - udelay((adap->udelay + 1) / 2);
> - bit_dbg(2, &i2c_adap->dev, "i2c_inb: 0x%02x %s\n", inval,
> - count ? "A" : "NA");
> - if (sclhi(adap)<0) { /* timeout */
> - dev_err(&i2c_adap->dev, "readbytes: timeout at ack\n");
> - return -ETIMEDOUT;
> - };
> - scllo(adap);
> -
> /* Some SMBus transactions require that we receive the
> - transaction length as the first read byte. */
> - if (rdcount == 1 && (msg->flags & I2C_M_RECV_LEN)) {
> + * transaction length as the first read byte.
> + */
> + if (rdcount == 1 && (flags & I2C_M_RECV_LEN)) {
> if (inval <= 0 || inval > I2C_SMBUS_BLOCK_MAX) {
> + if (!(flags & I2C_M_NO_RD_ACK))
> + acknak(i2c_adap, inval, 0);
> dev_err(&i2c_adap->dev, "readbytes: invalid "
> "block length (%d)\n", inval);
> return -EREMOTEIO;
> }
> /* The original count value accounts for the extra
> - bytes, that is, either 1 for a regular transaction,
> - or 2 for a PEC transaction. */
> + * bytes, that is, either 1 for a regular transaction,
> + * or 2 for a PEC transaction.
> + */
> count += inval;
> msg->len += inval;
> }
> +
> + bit_dbg(2, &i2c_adap->dev, "readbytes: 0x%02x %s\n",
> + inval,
> + (flags & I2C_M_NO_RD_ACK)
> + ? "(no ack/nak)"
> + : (count ? "A" : "NA"));
> +
> + if (!(flags & I2C_M_NO_RD_ACK)) {
> + inval = acknak(i2c_adap, inval, count);
> + if (inval < 0)
> + return inval;
> + }
> }
> return rdcount;
> }
I've cleanups up both problems myself, and here is for reference the
patch I'll be pushing to Linus:
--- linux-2.6.23-rc5.orig/drivers/i2c/algos/i2c-algo-bit.c 2007-09-09 15:48:55.000000000 +0200
+++ linux-2.6.23-rc5/drivers/i2c/algos/i2c-algo-bit.c 2007-09-09 16:01:06.000000000 +0200
@@ -357,13 +357,29 @@ static int sendbytes(struct i2c_adapter
return wrcount;
}
+static int acknak(struct i2c_adapter *i2c_adap, int is_ack)
+{
+ struct i2c_algo_bit_data *adap = i2c_adap->algo_data;
+
+ /* assert: sda is high */
+ if (is_ack) /* send ack */
+ setsda(adap, 0);
+ udelay((adap->udelay + 1) / 2);
+ if (sclhi(adap) < 0) { /* timeout */
+ dev_err(&i2c_adap->dev, "readbytes: ack/nak timeout\n");
+ return -ETIMEDOUT;
+ }
+ scllo(adap);
+ return 0;
+}
+
static int readbytes(struct i2c_adapter *i2c_adap, struct i2c_msg *msg)
{
int inval;
int rdcount=0; /* counts bytes read */
- struct i2c_algo_bit_data *adap = i2c_adap->algo_data;
unsigned char *temp = msg->buf;
int count = msg->len;
+ const unsigned flags = msg->flags;
while (count > 0) {
inval = i2c_inb(i2c_adap);
@@ -377,28 +393,12 @@ static int readbytes(struct i2c_adapter
temp++;
count--;
- if (msg->flags & I2C_M_NO_RD_ACK) {
- bit_dbg(2, &i2c_adap->dev, "i2c_inb: 0x%02x\n",
- inval);
- continue;
- }
-
- /* assert: sda is high */
- if (count) /* send ack */
- setsda(adap, 0);
- udelay((adap->udelay + 1) / 2);
- bit_dbg(2, &i2c_adap->dev, "i2c_inb: 0x%02x %s\n", inval,
- count ? "A" : "NA");
- if (sclhi(adap)<0) { /* timeout */
- dev_err(&i2c_adap->dev, "readbytes: timeout at ack\n");
- return -ETIMEDOUT;
- };
- scllo(adap);
-
/* Some SMBus transactions require that we receive the
transaction length as the first read byte. */
- if (rdcount == 1 && (msg->flags & I2C_M_RECV_LEN)) {
+ if (rdcount == 1 && (flags & I2C_M_RECV_LEN)) {
if (inval <= 0 || inval > I2C_SMBUS_BLOCK_MAX) {
+ if (!(flags & I2C_M_NO_RD_ACK))
+ acknak(i2c_adap, 0);
dev_err(&i2c_adap->dev, "readbytes: invalid "
"block length (%d)\n", inval);
return -EREMOTEIO;
@@ -409,6 +409,18 @@ static int readbytes(struct i2c_adapter
count += inval;
msg->len += inval;
}
+
+ bit_dbg(2, &i2c_adap->dev, "readbytes: 0x%02x %s\n",
+ inval,
+ (flags & I2C_M_NO_RD_ACK)
+ ? "(no ack/nak)"
+ : (count ? "A" : "NA"));
+
+ if (!(flags & I2C_M_NO_RD_ACK)) {
+ inval = acknak(i2c_adap, count);
+ if (inval < 0)
+ return inval;
+ }
}
return rdcount;
}
I've tested this patch with standard I2C block reads (no
I2C_M_NO_RD_ACK, no I2C_M_RECV_LEN, no PEC) and it worked fine.
Thanks,
--
Jean Delvare
More information about the i2c
mailing list