On Mon, Mar 26, 2012 at 10:49 PM, Daniel Vetter <daniel at ffwll.ch> wrote:
> On Mon, Mar 26, 2012 at 10:26:42PM +0800, Daniel Kurtz wrote:
>> Instead of rolling our own custom quirk_xfer function, use the bit_algo
>> pre_xfer and post_xfer functions to setup and teardown bit-banged
>> i2c transactions.
>>
>> gmbus_xfer uses .force_bit to determine which i2c_algorithm to use,
>> either i2c_bit_algo.master_xfer or its own. ?So, Similarly, let gmbus_func
>> use .force_bit to determine which i2c functionalities are available,
>> either i2c_bit_algo.functionality, or its own.
>
> Please split this part of the patch into a separate patch. Furthermore I'm
> not sure what this should buy us, given that we might magically changes
> our i2c feature set once with gone to fallback mode. Can you please
> elaborate why we need this?
An i2c adapter's functionality is provided by its algorithm.
Since these gmbus adapters can [for now] change their algorithm at
runtime, I thought the functionality returned should match the
currently selected algorithm at any given moment.
Arguably, the adapter actually sort of provides the union of the two
functionalities since if a particular transfer fails using gmbus, it
gets retried using bit-banged. But then again, this is a one-shot
permanent switch, so perhaps we should return the union of the
functionalities if force_bit == 0, and then only the bit-algo
functionality after the switch?
>
> The pre_xfer/post_xfer stuff looks good to me, that part along is
> Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
>
> Yours, Daniel
>
>>
>> Signed-off-by: Daniel Kurtz <djkurtz at chromium.org>
>> ---
>> ?drivers/gpu/drm/i915/intel_i2c.c | ? 72
>> +++++++++++++++++++++++--------------
>> ?1 files changed, 45 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_i2c.c
>> b/drivers/gpu/drm/i915/intel_i2c.c
>> index 54f85a1..ae08a08 100644
>> --- a/drivers/gpu/drm/i915/intel_i2c.c
>> +++ b/drivers/gpu/drm/i915/intel_i2c.c
>> @@ -137,6 +137,35 @@ static void set_data(void *data, int state_high)
>> ? ? ? POSTING_READ(bus->gpio_reg);
>> ?}
>>
>> +static int
>> +intel_gpio_pre_xfer(struct i2c_adapter *adapter)
>> +{
>> + ? ? struct intel_gmbus *bus = container_of(adapter,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct intel_gmbus,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?adapter);
>> + ? ? struct drm_i915_private *dev_priv = bus->dev_priv;
>> +
>> + ? ? intel_i2c_reset(dev_priv->dev);
>> + ? ? intel_i2c_quirk_set(dev_priv, true);
>> + ? ? set_data(bus, 1);
>> + ? ? set_clock(bus, 1);
>> + ? ? udelay(I2C_RISEFALL_TIME);
>> + ? ? return 0;
>> +}
>> +
>> +static void
>> +intel_gpio_post_xfer(struct i2c_adapter *adapter)
>> +{
>> + ? ? struct intel_gmbus *bus = container_of(adapter,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct intel_gmbus,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?adapter);
>> + ? ? struct drm_i915_private *dev_priv = bus->dev_priv;
>> +
>> + ? ? set_data(bus, 1);
>> + ? ? set_clock(bus, 1);
>> + ? ? intel_i2c_quirk_set(dev_priv, false);
>> +}
>> +
>> ?static bool
>> ?intel_gpio_setup(struct intel_gmbus *bus, u32 pin)
>> ?{
>> @@ -166,6 +195,8 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 pin)
>> ? ? ? algo->setscl = set_clock;
>> ? ? ? algo->getsda = get_data;
>> ? ? ? algo->getscl = get_clock;
>> + ? ? algo->pre_xfer = intel_gpio_pre_xfer;
>> + ? ? algo->post_xfer = intel_gpio_post_xfer;
>> ? ? ? algo->udelay = I2C_RISEFALL_TIME;
>> ? ? ? algo->timeout = usecs_to_jiffies(2200);
>> ? ? ? algo->data = bus;
>> @@ -174,30 +205,6 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 pin)
>> ?}
>>
>> ?static int
>> -intel_i2c_quirk_xfer(struct intel_gmbus *bus,
>> - ? ? ? ? ? ? ? ? ?struct i2c_msg *msgs,
>> - ? ? ? ? ? ? ? ? ?int num)
>> -{
>> - ? ? struct drm_i915_private *dev_priv = bus->dev_priv;
>> - ? ? int ret;
>> -
>> - ? ? intel_i2c_reset(dev_priv->dev);
>> -
>> - ? ? intel_i2c_quirk_set(dev_priv, true);
>> - ? ? set_data(bus, 1);
>> - ? ? set_clock(bus, 1);
>> - ? ? udelay(I2C_RISEFALL_TIME);
>> -
>> - ? ? ret = i2c_bit_algo.master_xfer(&bus->adapter, msgs, num);
>> -
>> - ? ? set_data(bus, 1);
>> - ? ? set_clock(bus, 1);
>> - ? ? intel_i2c_quirk_set(dev_priv, false);
>> -
>> - ? ? return ret;
>> -}
>> -
>> -static int
>> ?gmbus_xfer(struct i2c_adapter *adapter,
>> ? ? ? ? ?struct i2c_msg *msgs,
>> ? ? ? ? ?int num)
>> @@ -211,7 +218,7 @@ gmbus_xfer(struct i2c_adapter *adapter,
>> ? ? ? mutex_lock(&dev_priv->gmbus_mutex);
>>
>> ? ? ? if (bus->force_bit) {
>> - ? ? ? ? ? ? ret = intel_i2c_quirk_xfer(bus, msgs, num);
>> + ? ? ? ? ? ? ret = i2c_bit_algo.master_xfer(adapter, msgs, num);
>> ? ? ? ? ? ? ? goto out;
>> ? ? ? }
>>
>> @@ -325,8 +332,9 @@ timeout:
>> ? ? ? ? ? ? ? ret = -EIO;
>> ? ? ? } else {
>> ? ? ? ? ? ? ? bus->force_bit = true;
>> - ? ? ? ? ? ? ret = intel_i2c_quirk_xfer(bus, msgs, num);
>> + ? ? ? ? ? ? ret = i2c_bit_algo.master_xfer(adapter, msgs, num);
>> ? ? ? }
>> +
>> ?out:
>> ? ? ? mutex_unlock(&dev_priv->gmbus_mutex);
>> ? ? ? return ret;
>> @@ -334,11 +342,21 @@ out:
>>
>> ?static u32 gmbus_func(struct i2c_adapter *adapter)
>> ?{
>> - ? ? return i2c_bit_algo.functionality(adapter) &
>> + ? ? struct intel_gmbus *bus = container_of(adapter,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct intel_gmbus,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?adapter);
>> + ? ? struct drm_i915_private *dev_priv = bus->dev_priv;
>> + ? ? u32 func;
>> +
>> + ? ? mutex_lock(&dev_priv->gmbus_mutex);
>> + ? ? func = bus->force_bit ? i2c_bit_algo.functionality(adapter) :
>> ? ? ? ? ? ? ? (I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL |
>> ? ? ? ? ? ? ? /* I2C_FUNC_10BIT_ADDR | */
>> ? ? ? ? ? ? ? I2C_FUNC_SMBUS_READ_BLOCK_DATA |
>> ? ? ? ? ? ? ? I2C_FUNC_SMBUS_BLOCK_PROC_CALL);
>> + ? ? mutex_unlock(&dev_priv->gmbus_mutex);
>> +
>> + ? ? return func;
>> ?}
>>
>> ?static const struct i2c_algorithm gmbus_algorithm = {
>> --
>> 1.7.7.3
>>
>
> --
> Daniel Vetter
> Mail: daniel at ffwll.ch
> Mobile: +41 (0)79 365 57 48
