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

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2458 bytes --]

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@amd.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Thierry Reding <treding@nvidia.com>
> Reviewed-by: Ville Syrjälä <ville.syrjala@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.

> 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.

> -	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.

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

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel