Re: [PATCH 2/3] drm/dp/i2c: send bare addresses to properly reset i2c connections

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Apr 04, 2014 at 10:40:57AM -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.
> 
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 53 +++++++++++++++++++++++------------------
>  1 file changed, 30 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index f4babed..f0c2850 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -664,12 +664,26 @@ 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;
> +	struct drm_dp_aux_msg msg;
> +	int err = 0;
> +	u8 buf = 0;
>  
> -	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 = &buf;

Maybe just pass NULL and let buggy implementations explode?

> +		msg.size = 0;
> +		err = drm_dp_i2c_do_msg(aux, &msg);
> +		if (err < 0) {
> +			printk("error %d in bare address write\n", err);
> +			break;
> +		}
>  		/*
>  		 * Many hardware implementations support FIFOs larger than a
>  		 * single byte, but it has been empirically determined that
> @@ -677,31 +691,24 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
>  		 * decreased performance. Therefore each message is simply
>  		 * transferred byte-by-byte.
>  		 */
> -		for (j = 0; j < msgs[i].len; j++) {
> -			memset(&msg, 0, sizeof(msg));
> -			msg.address = msgs[i].addr;
> -
> -			msg.request = (msgs[i].flags & I2C_M_RD) ?
> -					DP_AUX_I2C_READ :
> -					DP_AUX_I2C_WRITE;
> -
> -			/*
> -			 * All messages except the last one are middle-of-
> -			 * transfer messages.
> -			 */
> -			if ((i < num - 1) || (j < msgs[i].len - 1))
> -				msg.request |= DP_AUX_I2C_MOT;
> -
> -			msg.buffer = msgs[i].buf + j;
> +		for (b = 0; b < msgs[m].len; b++) {
> +			msg.buffer = msgs[m].buf + b;
>  			msg.size = 1;
>  
>  			err = drm_dp_i2c_do_msg(aux, &msg);
>  			if (err < 0)
> -				return err;
> +				break;

This will abort the current message, but it'll keep going and try to
tranfer the next message. We need to abort the entire thing and proceed
to generate the i2c STOP.

Also you're now reusing the msg and expecting .transfer() to not clobber
any of the fields (apart from msg.reply). Hmm, actually the retry loop
in drm_dp_i2c_do_msg() already requires such a contract between the
caller and the callee. As we can't make the msg const due to msg.reply,
it would be nice if the documentation mentioned this somewhat important
detail.

>  		}
>  	}
> -
> -	return num;
> +	if (err >= 0)
> +		err = num;
> +	/* send a bare address packet to close out the connection */
> +	msg.request &= ~DP_AUX_I2C_MOT;
> +	msg.buffer = &buf;

Another chance to make buggy drivers explode ;)

> +	msg.size = 0;
> +	(void)drm_dp_i2c_do_msg(aux, &msg);
> +
> +	return err;
>  }
>  
>  static const struct i2c_algorithm drm_dp_i2c_algo = {
> -- 
> 1.8.3.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC