On Mon, Apr 11, 2016 at 12:50:44PM +0300, Jani Nikula wrote:
> On Mon, 07 Mar 2016, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > When the GMBUS based i2c transfer times out, we try to fall back to
> > bit-banging and retry the operation that way. However if the bit-banging
> > attempt also fails, we should probably go back to the GMBUS method for
> > the next attempt. Maybe there simply wasn't anyone one the bus at this
> > time.
> >
> > There's also a bit of a mess going on with the force_bit handling.
> > It's supposed to be a ref count actually, and it is as far as
> > intel_gmbus_force_bit() is concerned. But it's treated as just a
> > flag by the timeout based bit-banging fallback. I suppose that's
> > fine since we should never end up in the timeout fallback case
> > if force_bit was already non-zero. However now that we want to restore
> > things back to where they were after the bit-banging attempt failed,
> > we're going to have to do things a bit differently to avoid clobbering
> > the force_bit count as set up by intel_gmbus_force_bit(). So let's
> > dedicate the high bit as a flag for the low level timeout based fallback
> > and treat the rest of the bits as a ref count just as before.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 1 +
> > drivers/gpu/drm/i915/intel_i2c.c | 10 +++++++---
> > 2 files changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index f37ac120a29d..2348fea59592 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1043,6 +1043,7 @@ struct intel_fbc_work;
> >
> > struct intel_gmbus {
> > struct i2c_adapter adapter;
> > +#define GMBUS_FORCE_BIT_RETRY (1U << 31)
> > u32 force_bit;
> > u32 reg0;
> > i915_reg_t gpio_reg;
> > diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> > index 7bf8a485e18f..5d4b3604afd2 100644
> > --- a/drivers/gpu/drm/i915/intel_i2c.c
> > +++ b/drivers/gpu/drm/i915/intel_i2c.c
> > @@ -579,7 +579,6 @@ timeout:
> > * Hardware may not support GMBUS over these pins? Try GPIO bitbanging
> > * instead. Use EAGAIN to have i2c core retry.
> > */
> > - bus->force_bit = 1;
> > ret = -EAGAIN;
> >
> > out:
> > @@ -597,10 +596,15 @@ gmbus_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num)
> > intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
> > mutex_lock(&dev_priv->gmbus_mutex);
> >
> > - if (bus->force_bit)
> > + if (bus->force_bit) {
> > ret = i2c_bit_algo.master_xfer(adapter, msgs, num);
> > - else
> > + if (ret < 0)
> > + bus->force_bit &= ~GMBUS_FORCE_BIT_RETRY;
> > + } else {
> > ret = do_gmbus_xfer(adapter, msgs, num);
> > + if (ret == -EAGAIN)
> > + bus->force_bit |= GMBUS_FORCE_BIT_RETRY;
>
> Hmm, would this all be simpler if we did the first bit-banging retry
> here ourselves after all, and set ->force_bit only if bit-banging
> succeeds after gmbus -EAGAIN? I think moving the retry out of
> do_gmbus_xfer() was the right thing to do to, but maybe I went too far
> by pushing it all the way to i2c core?
>
> Anyway, this patch looks good, but it's just a bit subtle with the
> -EAGAIN and one retry and all.
>
> Up to you.
>
> Reviewed-by: Jani Nikula <jani.nikula@xxxxxxxxx>
I left it alone for now, if for no other reason than to avoid
having to redo any testing right now. We can always revisit it later.
Rest of the series pushed to dinq. Thanks for the reviews.
>
> > + }
> >
> > mutex_unlock(&dev_priv->gmbus_mutex);
> > intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS);
>
> --
> Jani Nikula, Intel Open Source Technology Center
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
