Sorry, I accidentally sent the previous reply while I wasn't done with the review yet. On Tue, 19 Jan 2016 11:09:33 -0600, minyard@xxxxxxx wrote: > @@ -691,13 +682,15 @@ static int i801_block_transaction(struct i801_priv *priv, > doesn't mention this limitation. */ > if ((priv->features & FEATURE_BLOCK_BUFFER) > && command != I2C_SMBUS_I2C_BLOCK_DATA > - && i801_set_block_buffer_mode(priv) == 0) > + && i801_set_block_buffer_mode(priv) == 0) { > + if (hwpec) > + priv->xact_extra |= SMBHSTCNT_PEC_EN; > result = i801_block_transaction_by_block(priv, data, > - read_write, hwpec); > - else > + read_write); > + } else > result = i801_block_transaction_byte_by_byte(priv, data, > read_write, > - command, hwpec); > + command); > > (...) > @@ -776,11 +770,17 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr, > return -EOPNOTSUPP; > } > > - if (hwpec) /* enable/disable hardware PEC */ > + result = i801_check_pre(priv); > + if (result < 0) > + return result; > + > + if (hwpec) { /* enable/disable hardware PEC */ > outb_p(inb_p(SMBAUXCTL(priv)) | SMBAUXCTL_CRC, SMBAUXCTL(priv)); > - else > + } else { > outb_p(inb_p(SMBAUXCTL(priv)) & (~SMBAUXCTL_CRC), > SMBAUXCTL(priv)); > + priv->xact_extra &= ~SMBHSTCNT_PEC_EN; > + } I'm confused by this asymmetry. You clear the PEC flag here, but you set it in i801_block_transaction() above. Originally the flag was set in i801_block_transaction_by_block() (and didn't have to be cleared, as it was temporary.) With your implementation, PEC may or may not be enabled if a driver asks for a non-block transaction with PEC. If this is the first transaction then it will not be enabled (bug already existed before your patch.) But if the previous transaction was a block transaction with PEC then the flag will still be present, so PEC will still be enabled. The previous implementation was wrong but at least it was consistently so. This makes me believe we should rather fix the bugs first, and then look into cleaning up this part of the code. If you start storing transaction-dependent information in struct i801_priv, you must make sure that nothing will leak from one transaction to the next. I still have to review the rest of your patch series, but I don't think it makes sense to carry the PEC flag that way if the rest of the transaction information is still passed as function parameters. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html