| Message ID | 1448980166-23055-2-git-send-email-jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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;
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(-)