[PATCH v2 4/4] drm/dp: Allow registering AUX channels as I2C busses

Jani Nikula jani.nikula at linux.intel.com
Tue Dec 17 09:11:21 PST 2013


On Tue, 17 Dec 2013, Thierry Reding <thierry.reding at gmail.com> wrote:
> Implements an I2C-over-AUX I2C adapter on top of the generic drm_dp_aux
> infrastructure. It extracts the retry logic from existing drivers, which
> should help in porting those drivers to this new helper.
>
> Signed-off-by: Thierry Reding <treding at nvidia.com>
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 178 ++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_dp_helper.h     |   4 +
>  2 files changed, 182 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 01a8173c6e83..8a64cf8ac8cc 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -555,3 +555,181 @@ int drm_dp_link_power_up(struct drm_dp_aux *aux, struct drm_dp_link *link)
>  
>  	return 0;
>  }
> +
> +/*
> + * I2C-over-AUX implementation
> + */
> +
> +struct drm_dp_i2c_adapter {
> +	struct i2c_adapter adapter;
> +	struct drm_dp_aux *aux;
> +};
> +
> +static u32 drm_dp_i2c_functionality(struct i2c_adapter *adapter)
> +{
> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL |
> +	       I2C_FUNC_SMBUS_READ_BLOCK_DATA |
> +	       I2C_FUNC_SMBUS_BLOCK_PROC_CALL |
> +	       I2C_FUNC_10BIT_ADDR;
> +}
> +
> +/*
> + * Transfer a single I2C-over-AUX message and handle various error conditions,
> + * retrying the transaction as appropriate.
> + */
> +static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> +{
> +	unsigned int retry;
> +	int err;
> +
> +	/*
> +	 * DP1.2 sections 2.7.7.1.5.6.1 and 2.7.7.1.6.6.1: A DP Source device
> +	 * is required to retry at least seven times upon receiving AUX_DEFER
> +	 * before giving up the AUX transaction.
> +	 */
> +	for (retry = 0; retry < 7; retry++) {
> +		err = aux->transfer(aux, msg);
> +		if (err < 0)
> +			return err;
> +
> +		switch (msg->reply & DP_AUX_NATIVE_REPLY_MASK) {
> +		case DP_AUX_NATIVE_REPLY_ACK:
> +			/*
> +			 * For I2C-over-AUX transactions this isn't enough, we
> +			 * need to check for the I2C ACK reply.
> +			 */
> +			break;
> +
> +		case DP_AUX_NATIVE_REPLY_NACK:
> +			return -EREMOTEIO;
> +
> +		case DP_AUX_NATIVE_REPLY_DEFER:
> +			/*
> +			 * We could check for I2C bitrate capabilities and if
> +			 * available adjust this interval. We could also be
> +			 * more careful with DP-to-legacy adapters where a
> +			 * long legacy cable may forc very low I2C bit rates.
                                                 ^^^^ force

> +			 * For now just defer for long enough to hopefully be
> +			 * safe for all use-cases.
> +			 */
> +			usleep_range(500, 600);

I've banged my head against the wall a bit with this (the original i915
comment similar to the above was mine) and I'd be hesitant to switch to
this as-is in i915.

First, I'd like to retain the DRM_DEBUG_KMS messages all around, as
otherwise issues here will be hard to debug. It's a pain to have to ask
to patch the kernel just get the debug info. At least we've had a fair
share of DP aux issues in our driver.

Second, I fear one size does not fit all wrt the delays. What would you
say about making all the DP_AUX_NATIVE_REPLY_DEFER and
DP_AUX_I2C_REPLY_DEFER checks do callbacks if defined, and fallback to
the default delays otherwise? I think there'll be some more churn in the
error handling, particularly for the more exotic sinks, and IMO having
them per driver would make development and bug fixing faster. I'm not
saying you need to do this now, it can be a follow-up; just asking what
you'd think of it.

I hope to be able to do more review of the series later this week.

BR,
Jani.


> +			continue;
> +
> +		default:
> +			DRM_ERROR("invalid native reply %#04x\n", msg->reply);
> +			return -EREMOTEIO;
> +		}
> +
> +		switch (msg->reply & DP_AUX_I2C_REPLY_MASK) {
> +		case DP_AUX_I2C_REPLY_ACK:
> +			/*
> +			 * Both native ACK and I2C ACK replies received. We
> +			 * can assume the transfer was successful.
> +			 */
> +			return 0;
> +
> +		case DP_AUX_I2C_REPLY_NACK:
> +			return -EREMOTEIO;
> +
> +		case DP_AUX_I2C_REPLY_DEFER:
> +			usleep_range(400, 500);
> +			continue;
> +
> +		default:
> +			DRM_ERROR("invalid I2C reply %#04x\n", msg->reply);
> +			return -EREMOTEIO;
> +		}
> +	}
> +
> +	DRM_ERROR("too many retries, giving up\n");
> +	return -EREMOTEIO;
> +}
> +
> +static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
> +			   int num)
> +{
> +	struct drm_dp_i2c_adapter *dp = adapter->algo_data;
> +	unsigned int i, j;
> +
> +	for (i = 0; i < num; i++) {
> +		struct drm_dp_aux_msg msg;
> +		int err;
> +
> +		/*
> +		 * Many hardware implementations support FIFOs larger than a
> +		 * single byte, but it has been empirically determined that
> +		 * transferring data in larger chunks can actually lead to
> +		 * 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 (j < msgs[i].len - 1)
> +				msg.request |= DP_AUX_I2C_MOT;
> +
> +			msg.buffer = msgs[i].buf + j;
> +			msg.size = 1;
> +
> +			err = drm_dp_i2c_do_msg(dp->aux, &msg);
> +			if (err < 0)
> +				return err;
> +		}
> +	}
> +
> +	return num;
> +}
> +
> +static const struct i2c_algorithm drm_dp_i2c_algo = {
> +	.functionality = drm_dp_i2c_functionality,
> +	.master_xfer = drm_dp_i2c_xfer,
> +};
> +
> +/**
> + * drm_dp_add_i2c_bus() - register an I2C adapter for I2C-over-AUX access
> + * @aux: DisplayPort AUX channel
> + *
> + * Returns a pointer to an I2C adapter on success or an ERR_PTR()-encoded
> + * error code on failure.
> + */
> +struct i2c_adapter *drm_dp_add_i2c_bus(struct drm_dp_aux *aux)
> +{
> +	struct drm_dp_i2c_adapter *adapter;
> +	int err;
> +
> +	adapter = kzalloc(sizeof(*adapter), GFP_KERNEL);
> +	if (!adapter)
> +		return ERR_PTR(-ENOMEM);
> +
> +	adapter->adapter.algo = &drm_dp_i2c_algo;
> +	adapter->adapter.algo_data = adapter;
> +	adapter->adapter.retries = 3;
> +	adapter->aux = aux;
> +
> +	adapter->adapter.class = I2C_CLASS_DDC;
> +	adapter->adapter.owner = THIS_MODULE;
> +	adapter->adapter.dev.parent = aux->dev;
> +	adapter->adapter.dev.of_node = aux->dev->of_node;
> +
> +	strncpy(adapter->adapter.name, dev_name(aux->dev),
> +		sizeof(adapter->adapter.name));
> +
> +	err = i2c_add_adapter(&adapter->adapter);
> +	if (err < 0) {
> +		kfree(adapter);
> +		return ERR_PTR(err);
> +	}
> +
> +	return &adapter->adapter;
> +}
> +EXPORT_SYMBOL(drm_dp_add_i2c_bus);
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index bad9ee11a2e2..c4acdbc2a172 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -423,6 +423,8 @@ struct drm_dp_aux_msg {
>   * @transfer: transfers a message representing a single AUX transaction
>   */
>  struct drm_dp_aux {
> +	struct device *dev;
> +
>  	ssize_t (*transfer)(struct drm_dp_aux *aux,
>  			    struct drm_dp_aux_msg *msg);
>  };
> @@ -494,4 +496,6 @@ int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link);
>   */
>  int drm_dp_link_power_up(struct drm_dp_aux *aux, struct drm_dp_link *link);
>  
> +struct i2c_adapter *drm_dp_add_i2c_bus(struct drm_dp_aux *aux);
> +
>  #endif /* _DRM_DP_HELPER_H_ */
> -- 
> 1.8.4.2
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center


More information about the dri-devel mailing list