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

[Jani Nikula]

On Thu, 26 Mar 2015, Jani Nikula <jani.nikula at linux.intel.com> wrote:
> 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.

Okay, so I ended up fixing this myself [1], replacing patches 20, 21,
and 22 of this series.

BR,
Jani.

[1] http://mid.gmane.org/cover.1427407523.git.jani.nikula@intel.com



>
> 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

-- 
Jani Nikula, Intel Open Source Technology Center