| Message ID | 681500CE65202E47A192754B01DAB4671B1674818F@SDE12.beckipc.net |
|---|---|
| State | Accepted |
| Headers | show |
Cc += Andy Duan On Tue, Jun 20, 2017 at 10:20:42AM +0200, Michail Georgios Etairidis wrote: > > The i2c-imx driver incorrectly uses readb()/writeb() to read and > write to the appropriate registers when performing a repeated start. > The appropriate imx_i2c_read_reg()/imx_i2c_write_reg() functions > should be used instead. Performing a repeated start results in > a kernel panic. The platform is imx. I really wonder why this didn't pop up earlier, maybe repeated start just isn't that usual. > Signed-off-by: Michail G Etairidis <m.etairidis@beck-ipc.com> Fixes: ce1a78840ff7 ("i2c: imx: add DMA support for freescale i2c driver") Fixes: 054b62d9f25c ("i2c: imx: fix the i2c bus hang issue when do repeat restart") Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Best regards Uwe
On Tue, Jun 20, 2017 at 10:28:35AM +0200, Uwe Kleine-König wrote: > Cc += Andy Duan > > On Tue, Jun 20, 2017 at 10:20:42AM +0200, Michail Georgios Etairidis wrote: > > > > The i2c-imx driver incorrectly uses readb()/writeb() to read and > > write to the appropriate registers when performing a repeated start. > > The appropriate imx_i2c_read_reg()/imx_i2c_write_reg() functions > > should be used instead. Performing a repeated start results in > > a kernel panic. The platform is imx. > > I really wonder why this didn't pop up earlier, maybe repeated start > just isn't that usual. Quite the contrary, read a register or something from an EEPROM and you should have the write (for the memory pointer) and the read (actual data) connected with a repeated start. The readb() calls are only accessed when a read message is not the last message. Since most transfers look like what I described above, this is not so super often. Add to that that not everyone reports such patches back upstream, this might go unnoticed for a while. So, thanks Michail for the patch and keeping at it! Note: I found there is some code duplication in the driver. Both 'if (is_lastmsg)' blocks look like copied to me...
On Tue, Jun 20, 2017 at 10:20:42AM +0200, Michail Georgios Etairidis wrote: > > The i2c-imx driver incorrectly uses readb()/writeb() to read and > write to the appropriate registers when performing a repeated start. > The appropriate imx_i2c_read_reg()/imx_i2c_write_reg() functions > should be used instead. Performing a repeated start results in > a kernel panic. The platform is imx. > > Signed-off-by: Michail G Etairidis <m.etairidis@beck-ipc.com> > Applied to for-current with the acks and fixes tags added, thanks!
diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c index 95ed171..54a47b4 100644 --- a/drivers/i2c/busses/i2c-imx.c +++ b/drivers/i2c/busses/i2c-imx.c @@ -734,9 +734,9 @@ static int i2c_imx_dma_read(struct imx_i2c_struct *i2c_imx, * the first read operation, otherwise the first read cost * one extra clock cycle. */ - temp = readb(i2c_imx->base + IMX_I2C_I2CR); + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR); temp |= I2CR_MTX; - writeb(temp, i2c_imx->base + IMX_I2C_I2CR); + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR); } msgs->buf[msgs->len-1] = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR); @@ -857,9 +857,9 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs, bo * the first read operation, otherwise the first read cost * one extra clock cycle. */ - temp = readb(i2c_imx->base + IMX_I2C_I2CR); + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR); temp |= I2CR_MTX; - writeb(temp, i2c_imx->base + IMX_I2C_I2CR); + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR); } } else if (i == (msgs->len - 2)) { dev_dbg(&i2c_imx->adapter.dev,
The i2c-imx driver incorrectly uses readb()/writeb() to read and write to the appropriate registers when performing a repeated start. The appropriate imx_i2c_read_reg()/imx_i2c_write_reg() functions should be used instead. Performing a repeated start results in a kernel panic. The platform is imx. Signed-off-by: Michail G Etairidis <m.etairidis@beck-ipc.com> --- I am resubmitting this patch to add the required Signed-off-by information. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html