diff mbox

i2c: i2c-imx: Use correct function to write to register

Message ID 681500CE65202E47A192754B01DAB4671B16747E36@SDE12.beckipc.net
State Superseded
Headers show

Commit Message

Michail Georgios Etairidis June 9, 2017, 1:28 p.m. UTC 
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.

I am resubmitting this patch as the previous one was erroneous. This patch is based on the current upstream kernel code.

--
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

Comments

Wolfram Sang June 19, 2017, 2:46 p.m. UTC | #1 
On Fri, Jun 09, 2017 at 03:28:12PM +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.

On what platform? It has a different regshift, I assume. Anyone from PTX
or Freescale can confirm?

> I am resubmitting this patch as the previous one was erroneous. This
> patch is based on the current upstream kernel code.

There is no Signed-off-by. I need that. Read
Documentation/process/submit-checklist.rst if you don't know about it
yet.
Uwe Kleine-König June 19, 2017, 6:54 p.m. UTC | #2 
On Fri, Jun 09, 2017 at 03:28:12PM +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.

please limit lines in the commit to 76 chars.

> I am resubmitting this patch as the previous one was erroneous. This patch is based on the current upstream kernel code.
> 
> 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);

These were introduced by commit ce1a78840ff7a authored by Yao Yuan.

>  	}
>  	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);

These were introduced by commit 054b62d9f25c9 authored by Fugang Duan.

I added these two people to Cc:, maybe they are still at Freescale/NXP
and can comment.

	imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);

results in

	readb(i2c_imx->base + (reg << i2c_imx->hwdata->regshift));

where .regshift is 2 for imx and 0 for vf610. So the patch makes a
difference on the former platforms only.

Best regards
Uwe
Andy Duan June 20, 2017, 1:50 a.m. UTC | #3 
From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Sent: Tuesday, June 20, 2017 2:54 AM
>On Fri, Jun 09, 2017 at 03:28:12PM +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.
>
>please limit lines in the commit to 76 chars.
>
>> I am resubmitting this patch as the previous one was erroneous. This patch
>is based on the current upstream kernel code.
>>
>> 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);
>
>These were introduced by commit ce1a78840ff7a authored by Yao Yuan.
>
>>  	}
>>  	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);
>
>These were introduced by commit 054b62d9f25c9 authored by Fugang Duan.
>
>I added these two people to Cc:, maybe they are still at Freescale/NXP and
>can comment.
>
>	imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
>
>results in
>
>	readb(i2c_imx->base + (reg << i2c_imx->hwdata->regshift));
>
>where .regshift is 2 for imx and 0 for vf610. So the patch makes a difference
>on the former platforms only.
>
>Best regards
>Uwe
>
The patch seems fine. It should use .imx_i2c_read_reg() instead of readb().

Acked-by: Fugang Duan <fugang.duan@nxp.com>

>--
>Pengutronix e.K.                           | Uwe Kleine-König            |
>Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
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
diff mbox

Patch

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,