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