diff mbox series

I2C: JZ4780: Fix bug for Ingenic X1000.

Message ID 1616084743-112402-2-git-send-email-zhouyanjie@wanyeetech.com (mailing list archive)
State Superseded
Headers show
Series I2C: JZ4780: Fix bug for Ingenic X1000. | expand

Commit Message

Zhou Yanjie March 18, 2021, 4:25 p.m. UTC
Only send "X1000_I2C_DC_STOP" when last byte, or it will cause
error when I2C write operation.

Fixes: 21575a7a8d4c ("I2C: JZ4780: Add support for the X1000.")

Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
---
 drivers/i2c/busses/i2c-jz4780.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Greg KH March 18, 2021, 5:01 p.m. UTC | #1
On Fri, Mar 19, 2021 at 12:25:43AM +0800, 周琰杰 (Zhou Yanjie) wrote:
> Only send "X1000_I2C_DC_STOP" when last byte, or it will cause
> error when I2C write operation.
> 
> Fixes: 21575a7a8d4c ("I2C: JZ4780: Add support for the X1000.")
> 
> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
> ---
>  drivers/i2c/busses/i2c-jz4780.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>
Wolfram Sang March 18, 2021, 5:06 p.m. UTC | #2
On Fri, Mar 19, 2021 at 12:25:43AM +0800, 周琰杰 (Zhou Yanjie) wrote:
> Only send "X1000_I2C_DC_STOP" when last byte, or it will cause
> error when I2C write operation.

Any write operation? I wonder then why nobody noticed before?

> -			while ((i2c_sta & JZ4780_I2C_STA_TFNF) &&
> -					(i2c->wt_len > 0)) {
> +			while ((i2c_sta & JZ4780_I2C_STA_TFNF) && (i2c->wt_len > 0)) {

This is a cosmetic change only IIUC. Shouldn't be in a bugfix.
Zhou Yanjie March 19, 2021, 6:58 a.m. UTC | #3
Hi Wolfram,


Sorry, please forgive my carefulness, I wrongly sent the version that 
did not clean up, resulting in missing the reporter's information and 
some errors in formats.


On 2021/3/19 上午1:06, Wolfram Sang wrote:
> On Fri, Mar 19, 2021 at 12:25:43AM +0800, 周琰杰 (Zhou Yanjie) wrote:
>> Only send "X1000_I2C_DC_STOP" when last byte, or it will cause
>> error when I2C write operation.
> Any write operation? I wonder then why nobody noticed before?


The standard I2C communication should look like this:

Read:

device_addr + w, reg_addr, device_addr + r, data;

Write:

device_addr + w, reg_addr, data;


But without this patch, it looks like this:

Read:

device_addr + w, reg_addr, device_addr + r, data;

Write:

device_addr + w, reg_addr, device_addr + w, data;

This is clearly not correct.


When I added support for X1000 to this driver, the hardware used was 
CU1000-Neo. On this hardware, there was an ADS7830 that communicated 
through I2C, but the operation of ADS7830 only involved read operations, 
so I was at that time failed to realize the problem with the write 
operation.
In addition, because X1000 did not implement relatively complete support 
in the mainline until the second half of 2020, there are still a large 
number of users who are still using the old SDK (kernel 3.10 and 
kernel4.4) provided by Ingenics, which may also be indirectly delayed 
exposure of this problem.


>> -			while ((i2c_sta & JZ4780_I2C_STA_TFNF) &&
>> -					(i2c->wt_len > 0)) {
>> +			while ((i2c_sta & JZ4780_I2C_STA_TFNF) && (i2c->wt_len > 0)) {
> This is a cosmetic change only IIUC. Shouldn't be in a bugfix.
>

My fault, I will remove it in the next version.


Thanks and best regards!
Wolfram Sang March 31, 2021, 7:18 a.m. UTC | #4
Hi, 

> > Any write operation? I wonder then why nobody noticed before?
> 
> 
> The standard I2C communication should look like this:
> 
> Read:
> 
> device_addr + w, reg_addr, device_addr + r, data;
> 
> Write:
> 
> device_addr + w, reg_addr, data;
> 
> 
> But without this patch, it looks like this:
> 
> Read:
> 
> device_addr + w, reg_addr, device_addr + r, data;
> 
> Write:
> 
> device_addr + w, reg_addr, device_addr + w, data;
> 
> This is clearly not correct.

Thanks for the additional information! I understand now. I added a bit
of this to the commit message of v2 to explain the situation.
Zhou Yanjie April 2, 2021, 3:30 p.m. UTC | #5
Hi Wolfram,

On 2021/3/31 下午3:18, Wolfram Sang wrote:
> Hi,
>
>>> Any write operation? I wonder then why nobody noticed before?
>>
>> The standard I2C communication should look like this:
>>
>> Read:
>>
>> device_addr + w, reg_addr, device_addr + r, data;
>>
>> Write:
>>
>> device_addr + w, reg_addr, data;
>>
>>
>> But without this patch, it looks like this:
>>
>> Read:
>>
>> device_addr + w, reg_addr, device_addr + r, data;
>>
>> Write:
>>
>> device_addr + w, reg_addr, device_addr + w, data;
>>
>> This is clearly not correct.
> Thanks for the additional information! I understand now. I added a bit
> of this to the commit message of v2 to explain the situation.
>

Thanks!


Best regards!
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-jz4780.c b/drivers/i2c/busses/i2c-jz4780.c
index 8509c5f..1ad093a 100644
--- a/drivers/i2c/busses/i2c-jz4780.c
+++ b/drivers/i2c/busses/i2c-jz4780.c
@@ -520,13 +520,12 @@  static irqreturn_t jz4780_i2c_irq(int irqno, void *dev_id)
 
 			i2c_sta = jz4780_i2c_readw(i2c, JZ4780_I2C_STA);
 
-			while ((i2c_sta & JZ4780_I2C_STA_TFNF) &&
-					(i2c->wt_len > 0)) {
+			while ((i2c_sta & JZ4780_I2C_STA_TFNF) && (i2c->wt_len > 0)) {
 				i2c_sta = jz4780_i2c_readw(i2c, JZ4780_I2C_STA);
 				data = *i2c->wbuf;
 				data &= ~JZ4780_I2C_DC_READ;
-				if ((!i2c->stop_hold) && (i2c->cdata->version >=
-						ID_X1000))
+				if ((i2c->wt_len == 1) && (!i2c->stop_hold) &&
+						(i2c->cdata->version >= ID_X1000))
 					data |= X1000_I2C_DC_STOP;
 				jz4780_i2c_writew(i2c, JZ4780_I2C_DC, data);
 				i2c->wbuf++;