From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jani Nikula Subject: Re: [PATCH] drm/i915/i2c: Track users of GMBUS force-bit Date: Mon, 12 Nov 2012 09:37:06 +0200 Message-ID: <87mwyn5l25.fsf@intel.com> References: <1352563101-1701-1-git-send-email-chris@chris-wilson.co.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTP id 7395F9E771 for ; Sun, 11 Nov 2012 23:36:45 -0800 (PST) In-Reply-To: <1352563101-1701-1-git-send-email-chris@chris-wilson.co.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Chris Wilson , intel-gfx@lists.freedesktop.org Cc: Mika Kuoppala List-Id: intel-gfx@lists.freedesktop.org On Sat, 10 Nov 2012, Chris Wilson wrote: > This fixes a regression for SDVO from > > commit fbfcc4f3a0cf8bbde87646b74241faa8e37426bf > Author: Jani Nikula > Date: Mon Oct 22 16:12:18 2012 +0300 > > drm/i915/sdvo: restore i2c adapter config on intel_sdvo_init() failures > > As SDVOB and SDVOC are multiplexed on the same pin, if a chipset does > not have the second SDVO encoder, it will then remove the force-bit > setting on the common i2c adapter during teardown. All subsequent > attempts of trying to use GMBUS with SDVOB then fail. > > Signed-off-by: Chris Wilson > Cc: Jani Nikula > Cc: Mika Kuoppala > --- > drivers/gpu/drm/i915/i915_drv.h | 2 +- > drivers/gpu/drm/i915/intel_i2c.c | 9 ++++++--- > 2 files changed, 7 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index c2daad1..cf407b1 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -394,7 +394,7 @@ struct intel_fbc_work; > > struct intel_gmbus { > struct i2c_adapter adapter; > - bool force_bit; > + u32 force_bit; > u32 reg0; > u32 gpio_reg; > struct i2c_algo_bit_data bit_algo; > diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c > index 84b68a6..f8ca0be 100644 > --- a/drivers/gpu/drm/i915/intel_i2c.c > +++ b/drivers/gpu/drm/i915/intel_i2c.c > @@ -482,7 +482,7 @@ timeout: > I915_WRITE(GMBUS0 + reg_offset, 0); > > /* Hardware may not support GMBUS over these pins? Try GPIO bitbanging instead. */ > - bus->force_bit = true; > + bus->force_bit = 1; > ret = i2c_bit_algo.master_xfer(adapter, msgs, num); > > out: > @@ -542,7 +542,7 @@ int intel_setup_gmbus(struct drm_device *dev) > > /* gmbus seems to be broken on i830 */ > if (IS_I830(dev)) > - bus->force_bit = true; > + bus->force_bit = 1; > > intel_gpio_setup(bus, port); > > @@ -583,7 +583,10 @@ void intel_gmbus_force_bit(struct i2c_adapter *adapter, bool force_bit) > { > struct intel_gmbus *bus = to_intel_gmbus(adapter); > > - bus->force_bit = force_bit; > + bus->force_bit += force_bit ? 1 : -1; > + DRM_DEBUG_KMS("%sabling bit-banging on %s. force bit now %d\n", > + force_bit ? "dis" : "en", adapter->name, > + bus->force_bit); To be honest, I think this creates a mine for someone to step on. bus->force_bit is checked using if (bus->force_bit). So in the default state, doing either intel_gmbus_force_bit(true) *or* intel_gmbus_force_bit(false) will enable force_bit. bus->force_bit is implicitly set to 1 on timeout and IS_I830(), so you also can't assume a neat pair of intel_gmbus_force_bit()s will always work. So, good piece of debugging there (and sorry about causing that in the first place!) but I think we should be more careful with the implementation. Maybe changing the ifs to (bus->force_bit > 0) and always using intel_gmbus_force_bit() also internally (instead of setting to true) are enough, but perhaps you have even better ideas. BR, Jani. > } > > void intel_teardown_gmbus(struct drm_device *dev) > -- > 1.7.10.4