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