On Thu, 26 Mar 2015, Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote:
> 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.
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@xxxxxxxxx
>
> 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
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
