[PATCH 2/4] drm/dp/i2c: send bare addresses to properly reset i2c connections (v3)

[Alex Deucher]

On Mon, Apr 7, 2014 at 3:49 AM, Thierry Reding <thierry.reding at gmail.com> wrote:
> On Fri, Apr 04, 2014 at 03:58:37PM -0400, Alex Deucher wrote:
>> We need bare address packets at the start and end of
>> each i2c over aux transaction to properly reset the connection
>> between transactions.  This mirrors what the existing dp i2c
>> over aux algo currently does.
>>
>> This fixes EDID fetches on certain monitors especially with
>> dp bridges.
>>
>> v2: update as per Ville's comments
>>     - Set buffer to NULL for zero sized packets
>>     - abort the entre transaction if one of the messages fails
>> v3: drop leftover debugging code
>>
>> Signed-off-by: Alex Deucher <alexander.deucher at amd.com>
>> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>> Cc: Jani Nikula <jani.nikula at intel.com>
>> Cc: Thierry Reding <treding at nvidia.com>
>> Reviewed-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
>> ---
>>  drivers/gpu/drm/drm_dp_helper.c | 52 +++++++++++++++++++++++------------------
>>  1 file changed, 29 insertions(+), 23 deletions(-)
>
> Can we please document that zero-sized messages specify address-only
> transactions? Perhaps it would also be useful to mention that these can
> only happen for I2C-over-AUX messages.

Can do.  I don't know of any uses for bare address packets with
regular aux off hand, but there may be cases I'm not familiar with.
Does anyone know of any use for a bare address packet with regular
aux?

>
>> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
>> index 74724aa..dfe4cf4 100644
>> --- a/drivers/gpu/drm/drm_dp_helper.c
>> +++ b/drivers/gpu/drm/drm_dp_helper.c
>> @@ -664,12 +664,23 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
>>                          int num)
>>  {
>>       struct drm_dp_aux *aux = adapter->algo_data;
>> -     unsigned int i, j;
>> +     unsigned int m, b;
>
> I don't see why these would need to be changed. i and j are perfectly
> fine loop variable names.

It was easier for me to follow the code since the variables matched
the objects they were iterating, but I can change them back if you'd
prefer.

>
>> -     for (i = 0; i < num; i++) {
>> -             struct drm_dp_aux_msg msg;
>> -             int err;
>> +     memset(&msg, 0, sizeof(msg));
>>
>> +     for (m = 0; m < num; m++) {
>> +             msg.address = msgs[m].addr;
>> +             msg.request = (msgs[m].flags & I2C_M_RD) ?
>> +                     DP_AUX_I2C_READ :
>> +                     DP_AUX_I2C_WRITE;
>> +             msg.request |= DP_AUX_I2C_MOT;
>> +             msg.buffer = NULL;
>> +             msg.size = 0;
>> +             err = drm_dp_i2c_do_msg(aux, &msg);
>> +             if (err < 0)
>> +                     break;
>
> This seems somewhat brittle to me. Even though I notice that patch 3/4
> updates a comment that documents these assumptions, I don't see a reason
> for these assumptions in the first place.

We already assume that in drm_dp_i2c_do_msg() for the retry loop.

>
> I'd prefer if we simply provided the complete message rather than rely
> on drivers not to touch anything but the reply field.

Are you suggesting we re-write drm_dp_i2c_do_msg() or move the retry
logic into drm_dp_i2c_xfer()?  Do you mind if we do that in a follow
up patch so we can keep it separate from the addition of the bare
address packets?


Alex