From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [PATCH 2/4] drm/dp/i2c: send bare addresses to properly reset i2c connections (v3) Date: Mon, 7 Apr 2014 09:49:09 +0200 Message-ID: <20140407074907.GA25718@ulmo> References: <1396641519-18529-1-git-send-email-alexander.deucher@amd.com> <1396641519-18529-3-git-send-email-alexander.deucher@amd.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1648714129==" Return-path: Received: from mail-bk0-f47.google.com (mail-bk0-f47.google.com [209.85.214.47]) by gabe.freedesktop.org (Postfix) with ESMTP id 40F686E532 for ; Mon, 7 Apr 2014 00:50:00 -0700 (PDT) Received: by mail-bk0-f47.google.com with SMTP id w10so581522bkz.6 for ; Mon, 07 Apr 2014 00:49:59 -0700 (PDT) In-Reply-To: <1396641519-18529-3-git-send-email-alexander.deucher@amd.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Alex Deucher Cc: Jani Nikula , dri-devel@lists.freedesktop.org, Alex Deucher , treding@nvidia.com List-Id: dri-devel@lists.freedesktop.org --===============1648714129== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="a8Wt8u1KmwUX3Y2C" Content-Disposition: inline --a8Wt8u1KmwUX3Y2C Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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. >=20 > This fixes EDID fetches on certain monitors especially with > dp bridges. >=20 > 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 >=20 > Signed-off-by: Alex Deucher > Cc: Ville Syrj=C3=A4l=C3=A4 > Cc: Jani Nikula > Cc: Thierry Reding > Reviewed-by: Ville Syrj=C3=A4l=C3=A4 > --- > 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. > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_hel= per.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 *adap= ter, 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. > - for (i =3D 0; i < num; i++) { > - struct drm_dp_aux_msg msg; > - int err; > + memset(&msg, 0, sizeof(msg)); > =20 > + 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. I'd prefer if we simply provided the complete message rather than rely on drivers not to touch anything but the reply field. Thierry --a8Wt8u1KmwUX3Y2C Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJTQlhzAAoJEN0jrNd/PrOhPRgP/jsPLc77cift2hYOXd0AeFTp XJ6suRvSvY/xNa3dbIwsmOA9XV8j2SgfK8G1AOu4LYt0rEG5jmGvkHKQS74LsPKm AAp5GUh3N//gFbdR2um8sfC8HnfWPtQbvSbHtjF8PqyuX9zN01IbNizPwsD0zrOC VV5v5kAS3G10WgEWphtSWVhHjvf3Z6FelLXEgvMGuszEqHvouNZJNbnm6bIUyKRE t/5c62cflbFtAoWbVcmVguf/i+jos8+i09S0RxlwNEOhDA1vzXruzgkWkXIty9J2 l8mXA28qajnIFeC/2botccz4c+6vka28i49g+aznMqUdQbZ8gvjW26a1mZMCrVul 26X7v9yZ7nKOm6WUxYARdprpBrneIwHQeBPvq+DgDmTZ3V6scdQUgM/oPM7mEGkC TCR9D7bj63APzCbcoS8OOuEEKuoyPY7U4qgjGiQgTCk7DNtal7cCmTm4iarLxo1u 3bKs7O1FEibPZ8MvZMq7KXb9hNYoEuwjVkY8HBRhAmUhHNCSLz9+x46wfLwJehSx +ABRr0pNFfUeBCamjz335i9G5BDhgXpgGOt4eZUvJrLVrtFl0Rpl2Bhn4i0rcQ3b VkoTaGvdoTf4ZFPvxCPk0tEb7ithpY55KKmgJurplkxsYxKGuYb3dVoVYWtUR7B9 sbAPwsRr7NEWovWe+SOM =JUS5 -----END PGP SIGNATURE----- --a8Wt8u1KmwUX3Y2C-- --===============1648714129== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel --===============1648714129==--