On Mon, Apr 7, 2014 at 3:49 AM, Thierry Reding <thierry.reding@xxxxxxxxx> 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@xxxxxxx>
>> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>> Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
>> Cc: Thierry Reding <treding@xxxxxxxxxx>
>> Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>> ---
>> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
