* [PATCH 1/2] drm/i915: simplify gmbus xfer error checks @ 2015-12-01 14:29 Jani Nikula 2015-12-01 14:29 ` [PATCH 2/2] drm/i915: abstract i2c bit banging fallback in gmbus xfer Jani Nikula 2015-12-01 14:45 ` [PATCH 1/2] drm/i915: simplify gmbus xfer error checks Ville Syrjälä 0 siblings, 2 replies; 7+ messages in thread From: Jani Nikula @ 2015-12-01 14:29 UTC (permalink / raw) To: intel-gfx; +Cc: 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] drm/i915: abstract i2c bit banging fallback in gmbus xfer 2015-12-01 14:29 [PATCH 1/2] drm/i915: simplify gmbus xfer error checks Jani Nikula @ 2015-12-01 14:29 ` Jani Nikula 2015-12-01 14:57 ` Ville Syrjälä 2015-12-01 14:45 ` [PATCH 1/2] drm/i915: simplify gmbus xfer error checks Ville Syrjälä 1 sibling, 1 reply; 7+ messages in thread From: Jani Nikula @ 2015-12-01 14:29 UTC (permalink / raw) To: intel-gfx; +Cc: 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] drm/i915: abstract i2c bit banging fallback in gmbus xfer 2015-12-01 14:29 ` [PATCH 2/2] drm/i915: abstract i2c bit banging fallback in gmbus xfer Jani Nikula @ 2015-12-01 14:57 ` Ville Syrjälä 2015-12-01 15:38 ` Jani Nikula 0 siblings, 1 reply; 7+ messages in thread From: Ville Syrjälä @ 2015-12-01 14:57 UTC (permalink / raw) To: Jani Nikula; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] drm/i915: abstract i2c bit banging fallback in gmbus xfer 2015-12-01 14:57 ` Ville Syrjälä @ 2015-12-01 15:38 ` Jani Nikula 2015-12-01 15:52 ` Ville Syrjälä 0 siblings, 1 reply; 7+ messages in thread From: Jani Nikula @ 2015-12-01 15:38 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] drm/i915: abstract i2c bit banging fallback in gmbus xfer 2015-12-01 15:38 ` Jani Nikula @ 2015-12-01 15:52 ` Ville Syrjälä 2015-12-02 11:35 ` Jani Nikula 0 siblings, 1 reply; 7+ messages in thread From: Ville Syrjälä @ 2015-12-01 15:52 UTC (permalink / raw) To: Jani Nikula; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] drm/i915: abstract i2c bit banging fallback in gmbus xfer 2015-12-01 15:52 ` Ville Syrjälä @ 2015-12-02 11:35 ` Jani Nikula 0 siblings, 0 replies; 7+ messages in thread From: Jani Nikula @ 2015-12-02 11:35 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] drm/i915: simplify gmbus xfer error checks 2015-12-01 14:29 [PATCH 1/2] drm/i915: simplify gmbus xfer error checks Jani Nikula 2015-12-01 14:29 ` [PATCH 2/2] drm/i915: abstract i2c bit banging fallback in gmbus xfer Jani Nikula @ 2015-12-01 14:45 ` Ville Syrjälä 1 sibling, 0 replies; 7+ messages in thread From: Ville Syrjälä @ 2015-12-01 14:45 UTC (permalink / raw) To: Jani Nikula; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-12-02 11:31 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-12-01 14:29 [PATCH 1/2] drm/i915: simplify gmbus xfer error checks Jani Nikula 2015-12-01 14:29 ` [PATCH 2/2] drm/i915: abstract i2c bit banging fallback in gmbus xfer Jani Nikula 2015-12-01 14:57 ` Ville Syrjälä 2015-12-01 15:38 ` Jani Nikula 2015-12-01 15:52 ` Ville Syrjälä 2015-12-02 11:35 ` Jani Nikula 2015-12-01 14:45 ` [PATCH 1/2] drm/i915: simplify gmbus xfer error checks Ville Syrjälä
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox