Re: [PATCH] drm/i915/i2c: Track users of GMBUS force-bit

[Jani Nikula <jani.nikula@intel.com>]

On Sat, 10 Nov 2012, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> This fixes a regression for SDVO from
>
> commit fbfcc4f3a0cf8bbde87646b74241faa8e37426bf
> Author: Jani Nikula <jani.nikula@intel.com>
> 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 <chris@chris-wilson.co.uk>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  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