On Tue, 17 Mar 2015, Imre Deak <imre.deak@xxxxxxxxx> wrote:
> From: "A.Sunil Kamath" <sunil.kamath@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> Signed-off-by: Damien Lespiau <damien.lespiau@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
