[v2,04/10] i2c-i801: Consolidate calls to i801_check_pre()
diff mbox

Message ID 1464570544-975-5-git-send-email-minyard@acm.org
State New
Headers show

Commit Message

Corey Minyard May 30, 2016, 1:08 a.m. UTC
From: Corey Minyard <cminyard@mvista.com>

It was always done before starting the transaction, so do it
in common code before the transaction start.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 drivers/i2c/busses/i2c-i801.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

Comments

Benjamin Tissoires June 9, 2016, 9:44 a.m. UTC | #1
On May 29 2016 or thereabouts, Corey Minyard wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> It was always done before starting the transaction, so do it
> in common code before the transaction start.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 222be9c..8794e70 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -402,10 +402,6 @@ static int i801_transaction(struct i801_priv *priv, int xact)
>  	int result;
>  	const struct i2c_adapter *adap = &priv->adapter;
>  
> -	result = i801_check_pre(priv);
> -	if (result < 0)
> -		return result;
> -
>  	if (priv->features & FEATURE_IRQ) {
>  		outb_p(xact | SMBHSTCNT_INTREN | SMBHSTCNT_START,
>  		       SMBHSTCNT(priv));
> @@ -562,10 +558,6 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>  	int result;
>  	const struct i2c_adapter *adap = &priv->adapter;
>  
> -	result = i801_check_pre(priv);
> -	if (result < 0)
> -		return result;
> -
>  	len = data->block[0];
>  
>  	if (read_write == I2C_SMBUS_WRITE) {
> @@ -789,6 +781,10 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>  		goto out;
>  	}
>  
> +	ret = i801_check_pre(priv);
> +	if (ret < 0)
> +		goto out;
> +

The calls looks identical (it gets called before each transaction), but
given that this is a status register, I wonder if this should not be
checked once the setup has been done, and not at the very early
beginning.

Cheers,
Benjamin

>  	if (block)
>  		ret = i801_block_transaction(priv, flags, data, read_write,
>  					     size);
--
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
Corey Minyard June 10, 2016, 10:52 a.m. UTC | #2
On 06/09/2016 04:44 AM, Benjamin Tissoires wrote:
> On May 29 2016 or thereabouts, Corey Minyard wrote:
>> From: Corey Minyard <cminyard@mvista.com>
>>
>> It was always done before starting the transaction, so do it
>> in common code before the transaction start.
>>
>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>> ---
>>   drivers/i2c/busses/i2c-i801.c | 12 ++++--------
>>   1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index 222be9c..8794e70 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -402,10 +402,6 @@ static int i801_transaction(struct i801_priv *priv, int xact)
>>   	int result;
>>   	const struct i2c_adapter *adap = &priv->adapter;
>>   
>> -	result = i801_check_pre(priv);
>> -	if (result < 0)
>> -		return result;
>> -
>>   	if (priv->features & FEATURE_IRQ) {
>>   		outb_p(xact | SMBHSTCNT_INTREN | SMBHSTCNT_START,
>>   		       SMBHSTCNT(priv));
>> @@ -562,10 +558,6 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>>   	int result;
>>   	const struct i2c_adapter *adap = &priv->adapter;
>>   
>> -	result = i801_check_pre(priv);
>> -	if (result < 0)
>> -		return result;
>> -
>>   	len = data->block[0];
>>   
>>   	if (read_write == I2C_SMBUS_WRITE) {
>> @@ -789,6 +781,10 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>>   		goto out;
>>   	}
>>   
>> +	ret = i801_check_pre(priv);
>> +	if (ret < 0)
>> +		goto out;
>> +
> The calls looks identical (it gets called before each transaction), but
> given that this is a status register, I wonder if this should not be
> checked once the setup has been done, and not at the very early
> beginning.

What I did is not a functional change from what was before, but I
think you may be right, it might be best to move this check this
before the switch statement above.  I'm not sure, though.

Jean, do you have any thoughts on this?

-corey

> Cheers,
> Benjamin
>
>>   	if (block)
>>   		ret = i801_block_transaction(priv, flags, data, read_write,
>>   					     size);

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

Patch
diff mbox

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 222be9c..8794e70 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -402,10 +402,6 @@  static int i801_transaction(struct i801_priv *priv, int xact)
 	int result;
 	const struct i2c_adapter *adap = &priv->adapter;
 
-	result = i801_check_pre(priv);
-	if (result < 0)
-		return result;
-
 	if (priv->features & FEATURE_IRQ) {
 		outb_p(xact | SMBHSTCNT_INTREN | SMBHSTCNT_START,
 		       SMBHSTCNT(priv));
@@ -562,10 +558,6 @@  static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 	int result;
 	const struct i2c_adapter *adap = &priv->adapter;
 
-	result = i801_check_pre(priv);
-	if (result < 0)
-		return result;
-
 	len = data->block[0];
 
 	if (read_write == I2C_SMBUS_WRITE) {
@@ -789,6 +781,10 @@  static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 		goto out;
 	}
 
+	ret = i801_check_pre(priv);
+	if (ret < 0)
+		goto out;
+
 	if (block)
 		ret = i801_block_transaction(priv, flags, data, read_write,
 					     size);