[Intel-gfx] [PATCH 22/49] drm/i915/bxt: Avoid registering unused gmbus ports as i2c adapter

[Jani Nikula]

On Tue, 17 Mar 2015, Imre Deak <imre.deak at intel.com> wrote:
> From: "A.Sunil Kamath" <sunil.kamath at intel.com>
>
> Though we populate all gmbus ports during setup_gmbus, only
> valid ones should be registered to i2c adapters. This is
> important as userspace can directly interact with the i2c bus.
>
> While populating gmbus register we ensure that unused ports
> will have gpio_reg value set as 0. This patch ensures that
> only those with non zero gpio reg will get registered as i2c
> adapter and this is applicable for all platforms.
>
> del_adapter will check if the adaptor was really added
> before, still its better to avoid unnecessary calls to the same.
> This patch also adds a check to deregister only added i2c adapters.
>
> Tested using i2c-tools to confirm that only valid gmbus ports
> are registered as i2c adapter.
>
> BXT will have only valid i2c adapters as below:
> i2c-x      i2c     i915 gmbus dpc      I2C adapter
> i2c-x+1    i2c     i915 gmbus dpb      I2C adapter
> i2c-x+2    i2c     i915 gmbus misc     I2C adapter
>
> v1: This patch is added as per review comments from
> Daniel Vetter on gmbus BXT patch
>
> Issue: VIZ-3574
> Signed-off-by: A.Sunil Kamath <sunil.kamath at intel.com>
> Signed-off-by: Damien Lespiau <damien.lespiau at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_i2c.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> index 06892b5..d5ca310 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -599,9 +599,12 @@ int intel_setup_gmbus(struct drm_device *dev)
>  
>  		intel_gpio_setup(bus, port);
>  
> -		ret = i2c_add_adapter(&bus->adapter);
> -		if (ret)
> -			goto err;
> +		/* Do not register unused gmbus ports as i2c adapter */
> +		if (bus->gpio_reg) {

This works by accident, as bus->gpio_reg will contain
dev_priv->gpio_mmio_base even if the reg is 0. Luckily for BXT this is
the case.

I think the question is, why do we initialize anything for
dev_priv->gmbus[i] where i is an unused pin pair? I think we should have
the validity check at the top, and if (invalid) continue;.

I care because we appear to be registering unused adapters also on other
recent platforms, and we should fix that too. That doesn't have to be
part of bxt enabling, but I don't like adding broken stuff that makes
future work harder.

BR,
Jani.



> + ret = i2c_add_adapter(&bus->adapter); + if (ret) + goto err; + } }
>  
>  	intel_i2c_reset(dev_priv->dev);
> @@ -611,7 +614,8 @@ int intel_setup_gmbus(struct drm_device *dev)
>  err:
>  	while (--i) {
>  		struct intel_gmbus *bus = &dev_priv->gmbus[i];
> -		i2c_del_adapter(&bus->adapter);
> +		if (bus->gpio_reg)
> +			i2c_del_adapter(&bus->adapter);
>  	}
>  	return ret;
>  }
> @@ -652,6 +656,7 @@ void intel_teardown_gmbus(struct drm_device *dev)
>  
>  	for (i = 0; i < GMBUS_NUM_PORTS; i++) {
>  		struct intel_gmbus *bus = &dev_priv->gmbus[i];
> -		i2c_del_adapter(&bus->adapter);
> +		if (bus->gpio_reg)
> +			i2c_del_adapter(&bus->adapter);
>  	}
>  }
> -- 
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center