Re: [PATCH v4 4/5] drm/dp: Allow registering AUX channels as I2C busses

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 21 Jan 2014, Thierry Reding <thierry.reding@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.
>
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>

I know I reviewed this already, but here are some more comments based on
converting i915 on top of this...

> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Changes in v4:
> - fix typo "bitrate" -> "bit rate"
>
> Changes in v3:
> - add back DRM_DEBUG_KMS and DRM_ERROR messages
> - embed i2c_adapter within struct drm_dp_aux
> - fix typo in comment
>
>  drivers/gpu/drm/drm_dp_helper.c | 183 ++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_dp_helper.h     |   5 ++
>  2 files changed, 188 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index eea15b1414ed..dba4205de274 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -364,6 +364,13 @@ EXPORT_SYMBOL(drm_dp_bw_code_to_link_rate);
>   *
>   * The .dev field should be set to a pointer to the device that implements
>   * the AUX channel.
> + *
> + * An AUX channel can also be used to transport I2C messages to a sink. A
> + * typical application of that is to access an EDID that's present in the
> + * sink device. The .transfer() function can also be used to execute such
> + * transactions. The drm_dp_aux_register_i2c_bus() function registers an
> + * I2C adapter that can be passed to drm_probe_ddc(). Upon removal, drivers
> + * should call drm_dp_aux_unregister_i2c_bus() to remove the I2C adapter.
>   */
>  
>  static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
> @@ -566,3 +573,179 @@ int drm_dp_link_configure(struct drm_dp_aux *aux, struct drm_dp_link *link)
>  
>  	return 0;
>  }
> +
> +/*
> + * I2C-over-AUX implementation
> + */
> +
> +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) {
> +			if (err == -EBUSY)
> +				continue;
> +
> +			DRM_DEBUG_KMS("transaction failed: %d\n", err);
> +			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:
> +			DRM_DEBUG_KMS("native nack\n");
> +			return -EREMOTEIO;
> +
> +		case DP_AUX_NATIVE_REPLY_DEFER:
> +			DRM_DEBUG_KMS("native defer");
> +			/*
> +			 * We could check for I2C bit rate capabilities and if
> +			 * available adjust this interval. We could also be
> +			 * more careful with DP-to-legacy adapters where a
> +			 * long legacy cable may force very low I2C bit rates.
> +			 *
> +			 * For now just defer for long enough to hopefully be
> +			 * safe for all use-cases.
> +			 */
> +			usleep_range(500, 600);
> +			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:
> +			DRM_DEBUG_KMS("I2C nack\n");
> +			return -EREMOTEIO;
> +
> +		case DP_AUX_I2C_REPLY_DEFER:
> +			DRM_DEBUG_KMS("I2C defer\n");
> +			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_aux *aux = adapter->algo_data;
> +	unsigned int i, j;
> +
> +	for (i = 0; i < num; i++) {
> +		struct drm_dp_aux_msg msg;
> +		int err;
> +

Should there be a start message similar to the i2c_algo_dp_aux_address()
call in i2c_algo_dp_aux_xfer() here? How would that map out to the
->transfer() function? msg.size = 0?

> +		/*
> +		 * 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.
> +		 */

Ville suggested this should be up to the drivers to decide. Maybe we
could have an aux->max_i2c_message_size (or similar), set by the
drivers, defaulting to 1, and this would chop up the message as
necessary? This could be a follow-up patch.

> +		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(aux, &msg);
> +			if (err < 0)
> +				return err;
> +		}
> +	}

Should there be a stop message similar to the i2c_algo_dp_aux_stop()
call in i2c_algo_dp_aux_xfer() here? Does not seem as crucial as the
start message because DP_AUX_I2C_MOT is handled above.

> +
> +	return num;
> +}
> +
> +static const struct i2c_algorithm drm_dp_i2c_algo = {
> +	.functionality = drm_dp_i2c_functionality,
> +	.master_xfer = drm_dp_i2c_xfer,
> +};
> +
> +/**
> + * drm_dp_aux_register_i2c_bus() - register an I2C adapter for I2C-over-AUX
> + * @aux: DisplayPort AUX channel
> + *
> + * Returns 0 on success or a negative error code on failure.
> + */
> +int drm_dp_aux_register_i2c_bus(struct drm_dp_aux *aux)
> +{
> +	aux->ddc.algo = &drm_dp_i2c_algo;
> +	aux->ddc.algo_data = aux;
> +	aux->ddc.retries = 3;
> +
> +	aux->ddc.class = I2C_CLASS_DDC;
> +	aux->ddc.owner = THIS_MODULE;
> +	aux->ddc.dev.parent = aux->dev;
> +	aux->ddc.dev.of_node = aux->dev->of_node;
> +
> +	strncpy(aux->ddc.name, dev_name(aux->dev), sizeof(aux->ddc.name));
> +

Should there be something similar to i2c_dp_aux_reset_bus() here like in
i2c_dp_aux_add_bus()?

> +	return i2c_add_adapter(&aux->ddc);
> +}
> +EXPORT_SYMBOL(drm_dp_aux_register_i2c_bus);
> +
> +/**
> + * drm_dp_aux_unregister_i2c_bus() - unregister an I2C-over-AUX adapter
> + * @aux: DisplayPort AUX channel
> + */
> +void drm_dp_aux_unregister_i2c_bus(struct drm_dp_aux *aux)
> +{
> +	i2c_del_adapter(&aux->ddc);
> +}
> +EXPORT_SYMBOL(drm_dp_aux_unregister_i2c_bus);
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index c7b3c736c40a..4d67bd12089b 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -421,10 +421,12 @@ struct drm_dp_aux_msg {
>  
>  /**
>   * struct drm_dp_aux - DisplayPort AUX channel
> + * @ddc: I2C adapter that can be used for I2C-over-AUX communication
>   * @dev: pointer to struct device that is the parent for this AUX channel
>   * @transfer: transfers a message representing a single AUX transaction
>   */
>  struct drm_dp_aux {
> +	struct i2c_adapter ddc;
>  	struct device *dev;
>  
>  	ssize_t (*transfer)(struct drm_dp_aux *aux,
> @@ -485,4 +487,7 @@ 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);
>  int drm_dp_link_configure(struct drm_dp_aux *aux, struct drm_dp_link *link);
>  
> +int drm_dp_aux_register_i2c_bus(struct drm_dp_aux *aux);
> +void drm_dp_aux_unregister_i2c_bus(struct drm_dp_aux *aux);
> +
>  #endif /* _DRM_DP_HELPER_H_ */
> -- 
> 1.8.4.2
>

-- 
Jani Nikula, Intel Open Source Technology Center