[PATCH 1/2] drm/i915: simplify gmbus xfer error checks

[PATCH 1/2] drm/i915: simplify gmbus xfer error checks

[Jani Nikula]

Shorter, easier to follow code with no functional changes. In all cases,
the return value ultimately comes from gmbus_wait_hw_status() anyway.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_i2c.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 1110c83953cf..ccb522c176bd 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -505,17 +505,13 @@ retry:
 			ret = gmbus_xfer_write(dev_priv, &msgs[i]);
 		}
 
+		if (!ret)
+			ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_WAIT_PHASE,
+						   GMBUS_HW_WAIT_EN);
 		if (ret == -ETIMEDOUT)
 			goto timeout;
-		if (ret == -ENXIO)
+		else if (ret)
 			goto clear_err;
-
-		ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_WAIT_PHASE,
-					   GMBUS_HW_WAIT_EN);
-		if (ret == -ENXIO)
-			goto clear_err;
-		if (ret)
-			goto timeout;
 	}
 
 	/* Generate a STOP condition on the bus. Note that gmbus can't generata
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm/i915: abstract i2c bit banging fallback in gmbus xfer

[Jani Nikula]

Choose between i2c bit banging and gmbus in a new higher level function,
and let the i2c core retry the first time we fall back to bit banging.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_i2c.c | 39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index ccb522c176bd..e26e22a72e3b 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -472,9 +472,7 @@ gmbus_xfer_index_read(struct drm_i915_private *dev_priv, struct i2c_msg *msgs)
 }
 
 static int
-gmbus_xfer(struct i2c_adapter *adapter,
-	   struct i2c_msg *msgs,
-	   int num)
+do_gmbus_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num)
 {
 	struct intel_gmbus *bus = container_of(adapter,
 					       struct intel_gmbus,
@@ -483,14 +481,6 @@ gmbus_xfer(struct i2c_adapter *adapter,
 	int i = 0, inc, try = 0;
 	int ret = 0;
 
-	intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
-	mutex_lock(&dev_priv->gmbus_mutex);
-
-	if (bus->force_bit) {
-		ret = i2c_bit_algo.master_xfer(adapter, msgs, num);
-		goto out;
-	}
-
 retry:
 	I915_WRITE(GMBUS0, bus->reg0);
 
@@ -585,13 +575,34 @@ timeout:
 		 bus->adapter.name, bus->reg0 & 0xff);
 	I915_WRITE(GMBUS0, 0);
 
-	/* Hardware may not support GMBUS over these pins? Try GPIO bitbanging instead. */
+	/*
+	 * Hardware may not support GMBUS over these pins? Try GPIO bitbanging
+	 * instead. Use EAGAIN to have i2c core retry.
+	 */
 	bus->force_bit = 1;
-	ret = i2c_bit_algo.master_xfer(adapter, msgs, num);
+	ret = -EAGAIN;
 
 out:
-	mutex_unlock(&dev_priv->gmbus_mutex);
+	return ret;
+}
+
+static int
+gmbus_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num)
+{
+	struct intel_gmbus *bus = container_of(adapter, struct intel_gmbus,
+					       adapter);
+	struct drm_i915_private *dev_priv = bus->dev_priv;
+	int ret;
+
+	intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
+	mutex_lock(&dev_priv->gmbus_mutex);
+
+	if (bus->force_bit)
+		ret = i2c_bit_algo.master_xfer(adapter, msgs, num);
+	else
+		ret = do_gmbus_xfer(adapter, msgs, num);
 
+	mutex_unlock(&dev_priv->gmbus_mutex);
 	intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS);
 
 	return ret;
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: simplify gmbus xfer error checks

[Ville Syrjälä]

On Tue, Dec 01, 2015 at 04:29:25PM +0200, Jani Nikula wrote:
> Shorter, easier to follow code with no functional changes. In all cases,
> the return value ultimately comes from gmbus_wait_hw_status() anyway.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_i2c.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> index 1110c83953cf..ccb522c176bd 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -505,17 +505,13 @@ retry:
>  			ret = gmbus_xfer_write(dev_priv, &msgs[i]);
>  		}
>  
> +		if (!ret)
> +			ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_WAIT_PHASE,
> +						   GMBUS_HW_WAIT_EN);
>  		if (ret == -ETIMEDOUT)
>  			goto timeout;
> -		if (ret == -ENXIO)
> +		else if (ret)
>  			goto clear_err;
> -
> -		ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_WAIT_PHASE,
> -					   GMBUS_HW_WAIT_EN);
> -		if (ret == -ENXIO)
> -			goto clear_err;
> -		if (ret)
> -			goto timeout;

Hmm. So gmbus_waut_hw_status() only ever returns -ENXIO,-ETIMEDOUT,0. And
since the xfer functions return value also comes from gmbus_wait_hw_status()
the same holds for them.

For -ENXIO we want to go to clear_err, for -ETIMEDOUT we want to go to
timeout. Other error values can't happen so the old 'if (ret)' case
actually checks for timeouts. So even if you sort of reversed the checks
there, it actually still checks for the same thing.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  	}
>  
>  	/* Generate a STOP condition on the bus. Note that gmbus can't generata
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: abstract i2c bit banging fallback in gmbus xfer

[Ville Syrjälä]

On Tue, Dec 01, 2015 at 04:29:26PM +0200, Jani Nikula wrote:
> Choose between i2c bit banging and gmbus in a new higher level function,
> and let the i2c core retry the first time we fall back to bit banging.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_i2c.c | 39 +++++++++++++++++++++++++--------------
>  1 file changed, 25 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> index ccb522c176bd..e26e22a72e3b 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -472,9 +472,7 @@ gmbus_xfer_index_read(struct drm_i915_private *dev_priv, struct i2c_msg *msgs)
>  }
>  
>  static int
> -gmbus_xfer(struct i2c_adapter *adapter,
> -	   struct i2c_msg *msgs,
> -	   int num)
> +do_gmbus_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num)
>  {
>  	struct intel_gmbus *bus = container_of(adapter,
>  					       struct intel_gmbus,
> @@ -483,14 +481,6 @@ gmbus_xfer(struct i2c_adapter *adapter,
>  	int i = 0, inc, try = 0;
>  	int ret = 0;
>  
> -	intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
> -	mutex_lock(&dev_priv->gmbus_mutex);
> -
> -	if (bus->force_bit) {
> -		ret = i2c_bit_algo.master_xfer(adapter, msgs, num);
> -		goto out;
> -	}
> -
>  retry:
>  	I915_WRITE(GMBUS0, bus->reg0);
>  
> @@ -585,13 +575,34 @@ timeout:
>  		 bus->adapter.name, bus->reg0 & 0xff);
>  	I915_WRITE(GMBUS0, 0);
>  
> -	/* Hardware may not support GMBUS over these pins? Try GPIO bitbanging instead. */
> +	/*
> +	 * Hardware may not support GMBUS over these pins? Try GPIO bitbanging
> +	 * instead. Use EAGAIN to have i2c core retry.
> +	 */
>  	bus->force_bit = 1;
> -	ret = i2c_bit_algo.master_xfer(adapter, msgs, num);
> +	ret = -EAGAIN;

This does mean we're left to the mercy of the timeout handling in the
i2c core. That is if our gmbus attempt takes too long, we may never
get a chance to retry with bit banging. Not sure if that's a real
concern or not.

Otherwise lgtm.

>  
>  out:
> -	mutex_unlock(&dev_priv->gmbus_mutex);
> +	return ret;
> +}
> +
> +static int
> +gmbus_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num)
> +{
> +	struct intel_gmbus *bus = container_of(adapter, struct intel_gmbus,
> +					       adapter);
> +	struct drm_i915_private *dev_priv = bus->dev_priv;
> +	int ret;
> +
> +	intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
> +	mutex_lock(&dev_priv->gmbus_mutex);
> +
> +	if (bus->force_bit)
> +		ret = i2c_bit_algo.master_xfer(adapter, msgs, num);
> +	else
> +		ret = do_gmbus_xfer(adapter, msgs, num);
>  
> +	mutex_unlock(&dev_priv->gmbus_mutex);
>  	intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS);
>  
>  	return ret;
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: abstract i2c bit banging fallback in gmbus xfer

[Jani Nikula]

On Tue, 01 Dec 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Tue, Dec 01, 2015 at 04:29:26PM +0200, Jani Nikula wrote:
>> Choose between i2c bit banging and gmbus in a new higher level function,
>> and let the i2c core retry the first time we fall back to bit banging.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_i2c.c | 39 +++++++++++++++++++++++++--------------
>>  1 file changed, 25 insertions(+), 14 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
>> index ccb522c176bd..e26e22a72e3b 100644
>> --- a/drivers/gpu/drm/i915/intel_i2c.c
>> +++ b/drivers/gpu/drm/i915/intel_i2c.c
>> @@ -472,9 +472,7 @@ gmbus_xfer_index_read(struct drm_i915_private *dev_priv, struct i2c_msg *msgs)
>>  }
>>  
>>  static int
>> -gmbus_xfer(struct i2c_adapter *adapter,
>> -	   struct i2c_msg *msgs,
>> -	   int num)
>> +do_gmbus_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num)
>>  {
>>  	struct intel_gmbus *bus = container_of(adapter,
>>  					       struct intel_gmbus,
>> @@ -483,14 +481,6 @@ gmbus_xfer(struct i2c_adapter *adapter,
>>  	int i = 0, inc, try = 0;
>>  	int ret = 0;
>>  
>> -	intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
>> -	mutex_lock(&dev_priv->gmbus_mutex);
>> -
>> -	if (bus->force_bit) {
>> -		ret = i2c_bit_algo.master_xfer(adapter, msgs, num);
>> -		goto out;
>> -	}
>> -
>>  retry:
>>  	I915_WRITE(GMBUS0, bus->reg0);
>>  
>> @@ -585,13 +575,34 @@ timeout:
>>  		 bus->adapter.name, bus->reg0 & 0xff);
>>  	I915_WRITE(GMBUS0, 0);
>>  
>> -	/* Hardware may not support GMBUS over these pins? Try GPIO bitbanging instead. */
>> +	/*
>> +	 * Hardware may not support GMBUS over these pins? Try GPIO bitbanging
>> +	 * instead. Use EAGAIN to have i2c core retry.
>> +	 */
>>  	bus->force_bit = 1;
>> -	ret = i2c_bit_algo.master_xfer(adapter, msgs, num);
>> +	ret = -EAGAIN;
>
> This does mean we're left to the mercy of the timeout handling in the
> i2c core. That is if our gmbus attempt takes too long, we may never
> get a chance to retry with bit banging. Not sure if that's a real
> concern or not.

IIUC the longest it can take to transfer 4 bytes without timing out is
just under 50 ms. The i2c core use a default timeout of 1000 ms. So if
we first succesfully, but slowly manage to transfer quite a bit (well,
at least 80 bytes, almost but not quite timing out), and then time out,
i2c core won't try again.

I do not think this is a real concern, but if it is, we can increase the
adapter->timeout in our gmbus setup code.

BR,
Jani.



>
> Otherwise lgtm.
>
>>  
>>  out:
>> -	mutex_unlock(&dev_priv->gmbus_mutex);
>> +	return ret;
>> +}
>> +
>> +static int
>> +gmbus_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num)
>> +{
>> +	struct intel_gmbus *bus = container_of(adapter, struct intel_gmbus,
>> +					       adapter);
>> +	struct drm_i915_private *dev_priv = bus->dev_priv;
>> +	int ret;
>> +
>> +	intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
>> +	mutex_lock(&dev_priv->gmbus_mutex);
>> +
>> +	if (bus->force_bit)
>> +		ret = i2c_bit_algo.master_xfer(adapter, msgs, num);
>> +	else
>> +		ret = do_gmbus_xfer(adapter, msgs, num);
>>  
>> +	mutex_unlock(&dev_priv->gmbus_mutex);
>>  	intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS);
>>  
>>  	return ret;
>> -- 
>> 2.1.4
>> 
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: abstract i2c bit banging fallback in gmbus xfer

[Ville Syrjälä]

On Tue, Dec 01, 2015 at 05:38:30PM +0200, Jani Nikula wrote:
> On Tue, 01 Dec 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Tue, Dec 01, 2015 at 04:29:26PM +0200, Jani Nikula wrote:
> >> Choose between i2c bit banging and gmbus in a new higher level function,
> >> and let the i2c core retry the first time we fall back to bit banging.
> >> 
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_i2c.c | 39 +++++++++++++++++++++++++--------------
> >>  1 file changed, 25 insertions(+), 14 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> >> index ccb522c176bd..e26e22a72e3b 100644
> >> --- a/drivers/gpu/drm/i915/intel_i2c.c
> >> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> >> @@ -472,9 +472,7 @@ gmbus_xfer_index_read(struct drm_i915_private *dev_priv, struct i2c_msg *msgs)
> >>  }
> >>  
> >>  static int
> >> -gmbus_xfer(struct i2c_adapter *adapter,
> >> -	   struct i2c_msg *msgs,
> >> -	   int num)
> >> +do_gmbus_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num)
> >>  {
> >>  	struct intel_gmbus *bus = container_of(adapter,
> >>  					       struct intel_gmbus,
> >> @@ -483,14 +481,6 @@ gmbus_xfer(struct i2c_adapter *adapter,
> >>  	int i = 0, inc, try = 0;
> >>  	int ret = 0;
> >>  
> >> -	intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
> >> -	mutex_lock(&dev_priv->gmbus_mutex);
> >> -
> >> -	if (bus->force_bit) {
> >> -		ret = i2c_bit_algo.master_xfer(adapter, msgs, num);
> >> -		goto out;
> >> -	}
> >> -
> >>  retry:
> >>  	I915_WRITE(GMBUS0, bus->reg0);
> >>  
> >> @@ -585,13 +575,34 @@ timeout:
> >>  		 bus->adapter.name, bus->reg0 & 0xff);
> >>  	I915_WRITE(GMBUS0, 0);
> >>  
> >> -	/* Hardware may not support GMBUS over these pins? Try GPIO bitbanging instead. */
> >> +	/*
> >> +	 * Hardware may not support GMBUS over these pins? Try GPIO bitbanging
> >> +	 * instead. Use EAGAIN to have i2c core retry.
> >> +	 */
> >>  	bus->force_bit = 1;
> >> -	ret = i2c_bit_algo.master_xfer(adapter, msgs, num);
> >> +	ret = -EAGAIN;
> >
> > This does mean we're left to the mercy of the timeout handling in the
> > i2c core. That is if our gmbus attempt takes too long, we may never
> > get a chance to retry with bit banging. Not sure if that's a real
> > concern or not.
> 
> IIUC the longest it can take to transfer 4 bytes without timing out is
> just under 50 ms. The i2c core use a default timeout of 1000 ms. So if
> we first succesfully, but slowly manage to transfer quite a bit (well,
> at least 80 bytes, almost but not quite timing out), and then time out,
> i2c core won't try again.
> 
> I do not think this is a real concern, but if it is, we can increase the
> adapter->timeout in our gmbus setup code.

Hmm. I was thinking it's shorter than that, but I guess I was mixing it
up with the i2c-bit-algo timeout which is 2.2ms (per some DDC spec
IIRC).

1 second seems plenty enough to me.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> BR,
> Jani.
> 
> 
> 
> >
> > Otherwise lgtm.
> >
> >>  
> >>  out:
> >> -	mutex_unlock(&dev_priv->gmbus_mutex);
> >> +	return ret;
> >> +}
> >> +
> >> +static int
> >> +gmbus_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num)
> >> +{
> >> +	struct intel_gmbus *bus = container_of(adapter, struct intel_gmbus,
> >> +					       adapter);
> >> +	struct drm_i915_private *dev_priv = bus->dev_priv;
> >> +	int ret;
> >> +
> >> +	intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
> >> +	mutex_lock(&dev_priv->gmbus_mutex);
> >> +
> >> +	if (bus->force_bit)
> >> +		ret = i2c_bit_algo.master_xfer(adapter, msgs, num);
> >> +	else
> >> +		ret = do_gmbus_xfer(adapter, msgs, num);
> >>  
> >> +	mutex_unlock(&dev_priv->gmbus_mutex);
> >>  	intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS);
> >>  
> >>  	return ret;
> >> -- 
> >> 2.1.4
> >> 
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: abstract i2c bit banging fallback in gmbus xfer

[Jani Nikula]

On Tue, 01 Dec 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Tue, Dec 01, 2015 at 05:38:30PM +0200, Jani Nikula wrote:
>> On Tue, 01 Dec 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> > On Tue, Dec 01, 2015 at 04:29:26PM +0200, Jani Nikula wrote:
>> >> Choose between i2c bit banging and gmbus in a new higher level function,
>> >> and let the i2c core retry the first time we fall back to bit banging.
>> >> 
>> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> >> ---
>> >>  drivers/gpu/drm/i915/intel_i2c.c | 39 +++++++++++++++++++++++++--------------
>> >>  1 file changed, 25 insertions(+), 14 deletions(-)
>> >> 
>> >> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
>> >> index ccb522c176bd..e26e22a72e3b 100644
>> >> --- a/drivers/gpu/drm/i915/intel_i2c.c
>> >> +++ b/drivers/gpu/drm/i915/intel_i2c.c
>> >> @@ -472,9 +472,7 @@ gmbus_xfer_index_read(struct drm_i915_private *dev_priv, struct i2c_msg *msgs)
>> >>  }
>> >>  
>> >>  static int
>> >> -gmbus_xfer(struct i2c_adapter *adapter,
>> >> -	   struct i2c_msg *msgs,
>> >> -	   int num)
>> >> +do_gmbus_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num)
>> >>  {
>> >>  	struct intel_gmbus *bus = container_of(adapter,
>> >>  					       struct intel_gmbus,
>> >> @@ -483,14 +481,6 @@ gmbus_xfer(struct i2c_adapter *adapter,
>> >>  	int i = 0, inc, try = 0;
>> >>  	int ret = 0;
>> >>  
>> >> -	intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
>> >> -	mutex_lock(&dev_priv->gmbus_mutex);
>> >> -
>> >> -	if (bus->force_bit) {
>> >> -		ret = i2c_bit_algo.master_xfer(adapter, msgs, num);
>> >> -		goto out;
>> >> -	}
>> >> -
>> >>  retry:
>> >>  	I915_WRITE(GMBUS0, bus->reg0);
>> >>  
>> >> @@ -585,13 +575,34 @@ timeout:
>> >>  		 bus->adapter.name, bus->reg0 & 0xff);
>> >>  	I915_WRITE(GMBUS0, 0);
>> >>  
>> >> -	/* Hardware may not support GMBUS over these pins? Try GPIO bitbanging instead. */
>> >> +	/*
>> >> +	 * Hardware may not support GMBUS over these pins? Try GPIO bitbanging
>> >> +	 * instead. Use EAGAIN to have i2c core retry.
>> >> +	 */
>> >>  	bus->force_bit = 1;
>> >> -	ret = i2c_bit_algo.master_xfer(adapter, msgs, num);
>> >> +	ret = -EAGAIN;
>> >
>> > This does mean we're left to the mercy of the timeout handling in the
>> > i2c core. That is if our gmbus attempt takes too long, we may never
>> > get a chance to retry with bit banging. Not sure if that's a real
>> > concern or not.
>> 
>> IIUC the longest it can take to transfer 4 bytes without timing out is
>> just under 50 ms. The i2c core use a default timeout of 1000 ms. So if
>> we first succesfully, but slowly manage to transfer quite a bit (well,
>> at least 80 bytes, almost but not quite timing out), and then time out,
>> i2c core won't try again.
>> 
>> I do not think this is a real concern, but if it is, we can increase the
>> adapter->timeout in our gmbus setup code.
>
> Hmm. I was thinking it's shorter than that, but I guess I was mixing it
> up with the i2c-bit-algo timeout which is 2.2ms (per some DDC spec
> IIRC).
>
> 1 second seems plenty enough to me.
>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Both pushed to drm-intel-next-queued, thanks for the review.

BR,
Jani.

>
>> 
>> BR,
>> Jani.
>> 
>> 
>> 
>> >
>> > Otherwise lgtm.
>> >
>> >>  
>> >>  out:
>> >> -	mutex_unlock(&dev_priv->gmbus_mutex);
>> >> +	return ret;
>> >> +}
>> >> +
>> >> +static int
>> >> +gmbus_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num)
>> >> +{
>> >> +	struct intel_gmbus *bus = container_of(adapter, struct intel_gmbus,
>> >> +					       adapter);
>> >> +	struct drm_i915_private *dev_priv = bus->dev_priv;
>> >> +	int ret;
>> >> +
>> >> +	intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
>> >> +	mutex_lock(&dev_priv->gmbus_mutex);
>> >> +
>> >> +	if (bus->force_bit)
>> >> +		ret = i2c_bit_algo.master_xfer(adapter, msgs, num);
>> >> +	else
>> >> +		ret = do_gmbus_xfer(adapter, msgs, num);
>> >>  
>> >> +	mutex_unlock(&dev_priv->gmbus_mutex);
>> >>  	intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS);
>> >>  
>> >>  	return ret;
>> >> -- 
>> >> 2.1.4
>> >> 
>> >> _______________________________________________
>> >> Intel-gfx mailing list
>> >> Intel-gfx@lists.freedesktop.org
>> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> 
>> -- 
>> Jani Nikula, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx