From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alex Deucher Subject: Re: [PATCH 2/4] drm/dp/i2c: send bare addresses to properly reset i2c connections (v3) Date: Mon, 7 Apr 2014 09:44:06 -0400 Message-ID: References: <1396641519-18529-1-git-send-email-alexander.deucher@amd.com> <1396641519-18529-3-git-send-email-alexander.deucher@amd.com> <20140407074907.GA25718@ulmo> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail-qg0-f46.google.com (mail-qg0-f46.google.com [209.85.192.46]) by gabe.freedesktop.org (Postfix) with ESMTP id 423776E654 for ; Mon, 7 Apr 2014 06:44:08 -0700 (PDT) Received: by mail-qg0-f46.google.com with SMTP id j107so1359466qga.5 for ; Mon, 07 Apr 2014 06:44:06 -0700 (PDT) In-Reply-To: <20140407074907.GA25718@ulmo> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Thierry Reding Cc: Jani Nikula , Maling list - DRI developers , Alex Deucher , treding@nvidia.com List-Id: dri-devel@lists.freedesktop.org On Mon, Apr 7, 2014 at 3:49 AM, Thierry Reding w= rote: > 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 >> Cc: Ville Syrj=E4l=E4 >> Cc: Jani Nikula >> Cc: Thierry Reding >> Reviewed-by: Ville Syrj=E4l=E4 >> --- >> 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_he= lper.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 *ada= pter, struct i2c_msg *msgs, >> int num) >> { >> struct drm_dp_aux *aux =3D 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 =3D 0; i < num; i++) { >> - struct drm_dp_aux_msg msg; >> - int err; >> + memset(&msg, 0, sizeof(msg)); >> >> + for (m =3D 0; m < num; m++) { >> + msg.address =3D msgs[m].addr; >> + msg.request =3D (msgs[m].flags & I2C_M_RD) ? >> + DP_AUX_I2C_READ : >> + DP_AUX_I2C_WRITE; >> + msg.request |=3D DP_AUX_I2C_MOT; >> + msg.buffer =3D NULL; >> + msg.size =3D 0; >> + err =3D 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