[PATCH v2 0/4] drm/dp: Introduce AUX channel infrastructure

[PATCH v2 0/4] drm/dp: Introduce AUX channel infrastructure

[Thierry Reding]

Hi,

This small series introduces some infrastructure to support AUX channels
in a generic way. Drivers make use of it by embedding and filling in a
struct drm_dp_aux. Various helpers can then be used to for example read
from or write to the DPCD.

Patch 1 adds the basic infrastructure as well as a couple of helpers to
access the DPCD.

The helper introduced in patch 2 can be used to obtain the link status
as expected by various existing DP helpers.

More convenience helpers are added in patch 3, which can come in handy
during DP initialization.

An AUX channel can also be used to implement I2C-over-AUX and patch 4
implements an I2C adapter that can be used with the DRM EDID helpers.

Changes in v2:
- reimplement I2C-over-AUX functionality to get rid of the additional
  layer
- extract retry logic from existing drivers
- add more kerneldoc comments

Thierry

Thierry Reding (4):
  drm/dp: Add AUX channel infrastructure
  drm/dp: Add drm_dp_dpcd_read_link_status()
  drm/dp: Add DisplayPort link helpers
  drm/dp: Allow registering AUX channels as I2C busses

 drivers/gpu/drm/drm_dp_helper.c | 387 ++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_dp_helper.h     | 101 +++++++++++
 2 files changed, 488 insertions(+)

-- 
1.8.4.2

[PATCH v2 1/4] drm/dp: Add AUX channel infrastructure

[Thierry Reding]

This is a superset of the current i2c_dp_aux bus functionality and can
be used to transfer native AUX in addition to I2C-over-AUX messages.

Helpers are provided to read and write the DPCD, either blockwise or
byte-wise. Many of the existing helpers for DisplayPort take a copy of a
portion of the DPCD and operate on that, without a way to write data
back to the DPCD (e.g. for configuration of the link).

Subsequent patches will build upon this infrastructure to provide common
functionality in a generic way.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 116 ++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_dp_helper.h     |  64 ++++++++++++++++++++++
 2 files changed, 180 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 9e978aae8972..bd622d462ca7 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -346,3 +346,119 @@ int drm_dp_bw_code_to_link_rate(u8 link_bw)
 	}
 }
 EXPORT_SYMBOL(drm_dp_bw_code_to_link_rate);
+
+/**
+ * drm_dp_dpcd_read() - read a series of bytes from the DPCD
+ * @aux: DisplayPort AUX channel
+ * @offset: address of the (first) register to read
+ * @buffer: buffer to store the register values
+ * @size: number of bytes in @buffer
+ *
+ * Returns the number of bytes transferred on success, or a negative error
+ * code on failure.
+ */
+ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
+			 void *buffer, size_t size)
+{
+	struct drm_dp_aux_msg msg;
+	unsigned int retry;
+	int err;
+
+	memset(&msg, 0, sizeof(msg));
+	msg.address = offset;
+	msg.request = DP_AUX_NATIVE_READ;
+	msg.buffer = buffer;
+	msg.size = size;
+
+	/*
+	 * The specification doesn't give any recommendation on how often to
+	 * retry native transactions, so retry 7 times like for I2C-over-AUX
+	 * transactions.
+	 */
+	for (retry = 0; retry < 7; retry++) {
+		err = aux->transfer(aux, &msg);
+		if (err < 0) {
+			if (err == -EBUSY)
+				continue;
+
+			return err;
+		}
+
+		if (err == 0)
+			return -EPROTO;
+
+		switch (msg.reply & DP_AUX_NATIVE_REPLY_MASK) {
+		case DP_AUX_NATIVE_REPLY_ACK:
+			return err;
+
+		case DP_AUX_NATIVE_REPLY_NACK:
+			return -EIO;
+
+		case DP_AUX_NATIVE_REPLY_DEFER:
+			usleep_range(400, 500);
+			break;
+		}
+	}
+
+	DRM_ERROR("too many retries, giving up\n");
+	return -EIO;
+}
+EXPORT_SYMBOL(drm_dp_dpcd_read);
+
+/**
+ * drm_dp_dpcd_write() - write a series of bytes to the DPCD
+ * @aux: DisplayPort AUX channel
+ * @offset: address of the (first) register to write
+ * @buffer: buffer containing the values to write
+ * @size: number of bytes in @buffer
+ *
+ * Returns the number of bytes transferred on success, or a negative error
+ * code on failure.
+ */
+ssize_t drm_dp_dpcd_write(struct drm_dp_aux *aux, unsigned int offset,
+			  void *buffer, size_t size)
+{
+	struct drm_dp_aux_msg msg;
+	unsigned int retry;
+	int err;
+
+	memset(&msg, 0, sizeof(msg));
+	msg.address = offset;
+	msg.request = DP_AUX_NATIVE_WRITE;
+	msg.buffer = buffer;
+	msg.size = size;
+
+	/*
+	 * The specification doesn't give any recommendation on how often to
+	 * retry native transactions, so retry 7 times like for I2C-over-AUX
+	 * transactions.
+	 */
+	for (retry = 0; retry < 7; retry++) {
+		err = aux->transfer(aux, &msg);
+		if (err < 0) {
+			if (err == -EBUSY)
+				continue;
+
+			return err;
+		}
+
+		if (err == 0)
+			return -EPROTO;
+
+		switch (msg.reply & DP_AUX_NATIVE_REPLY_MASK) {
+		case DP_AUX_NATIVE_REPLY_ACK:
+			return 0;
+
+		case DP_AUX_NATIVE_REPLY_NACK:
+			return -EIO;
+
+		case DP_AUX_NATIVE_REPLY_DEFER:
+			usleep_range(400, 500);
+			break;
+		}
+	}
+
+	DRM_ERROR("too many retries, giving up\n");
+	return -EIO;
+}
+EXPORT_SYMBOL(drm_dp_dpcd_write);
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index c873f9ce5871..0b14e799cd98 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -397,4 +397,68 @@ drm_dp_enhanced_frame_cap(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
 		(dpcd[DP_MAX_LANE_COUNT] & DP_ENHANCED_FRAME_CAP);
 }
 
+/*
+ * DisplayPort AUX channel
+ */
+
+/**
+ * struct drm_dp_aux_msg - DisplayPort AUX channel transaction
+ * @address: address of the (first) register to access
+ * @flags: contains the type of transaction as well as flags (see above)
+ * @reply: upon completion, contains the reply type of the transaction
+ * @buffer: pointer to a transmission or reception buffer
+ * @size: size of @buffer
+ */
+struct drm_dp_aux_msg {
+	unsigned int address;
+	u8 request;
+	u8 reply;
+	void *buffer;
+	size_t size;
+};
+
+/**
+ * struct drm_dp_aux - DisplayPort AUX channel
+ * @transfer: transfers a message representing a single AUX transaction
+ */
+struct drm_dp_aux {
+	ssize_t (*transfer)(struct drm_dp_aux *aux,
+			    struct drm_dp_aux_msg *msg);
+};
+
+ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
+			 void *buffer, size_t size);
+ssize_t drm_dp_dpcd_write(struct drm_dp_aux *aux, unsigned int offset,
+			  void *buffer, size_t size);
+
+/**
+ * drm_dp_dpcd_readb() - read a single byte from the DPCD
+ * @aux: DisplayPort AUX channel
+ * @offset: address of the register to read
+ * @valuep: location where the value of the register will be stored
+ *
+ * Returns the number of bytes transferred (1) on success, or a negative
+ * error code on failure.
+ */
+static inline ssize_t drm_dp_dpcd_readb(struct drm_dp_aux *aux,
+					unsigned int offset, u8 *valuep)
+{
+	return drm_dp_dpcd_read(aux, offset, valuep, 1);
+}
+
+/**
+ * drm_dp_dpcd_writeb() - write a single byte to the DPCD
+ * @aux: DisplayPort AUX channel
+ * @offset: address of the register to write
+ * @valuep: value to write to the register
+ *
+ * Returns the number of bytes transferred (1) on success, or a negative
+ * error code on failure.
+ */
+static inline ssize_t drm_dp_dpcd_writeb(struct drm_dp_aux *aux, u8 value,
+					 unsigned int offset)
+{
+	return drm_dp_dpcd_write(aux, offset, &value, 1);
+}
+
 #endif /* _DRM_DP_HELPER_H_ */
-- 
1.8.4.2

Re: [PATCH v2 1/4] drm/dp: Add AUX channel infrastructure

[Jani Nikula]

On Tue, 17 Dec 2013, Thierry Reding <thierry.reding@gmail.com> wrote:
> This is a superset of the current i2c_dp_aux bus functionality and can
> be used to transfer native AUX in addition to I2C-over-AUX messages.
>
> Helpers are provided to read and write the DPCD, either blockwise or
> byte-wise. Many of the existing helpers for DisplayPort take a copy of a
> portion of the DPCD and operate on that, without a way to write data
> back to the DPCD (e.g. for configuration of the link).
>
> Subsequent patches will build upon this infrastructure to provide common
> functionality in a generic way.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 116 ++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_dp_helper.h     |  64 ++++++++++++++++++++++
>  2 files changed, 180 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 9e978aae8972..bd622d462ca7 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -346,3 +346,119 @@ int drm_dp_bw_code_to_link_rate(u8 link_bw)
>  	}
>  }
>  EXPORT_SYMBOL(drm_dp_bw_code_to_link_rate);
> +
> +/**
> + * drm_dp_dpcd_read() - read a series of bytes from the DPCD
> + * @aux: DisplayPort AUX channel
> + * @offset: address of the (first) register to read
> + * @buffer: buffer to store the register values
> + * @size: number of bytes in @buffer
> + *
> + * Returns the number of bytes transferred on success, or a negative error
> + * code on failure.
> + */
> +ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
> +			 void *buffer, size_t size)
> +{
> +	struct drm_dp_aux_msg msg;
> +	unsigned int retry;
> +	int err;
> +
> +	memset(&msg, 0, sizeof(msg));
> +	msg.address = offset;
> +	msg.request = DP_AUX_NATIVE_READ;
> +	msg.buffer = buffer;
> +	msg.size = size;
> +
> +	/*
> +	 * The specification doesn't give any recommendation on how often to
> +	 * retry native transactions, so retry 7 times like for I2C-over-AUX
> +	 * transactions.
> +	 */
> +	for (retry = 0; retry < 7; retry++) {
> +		err = aux->transfer(aux, &msg);
> +		if (err < 0) {
> +			if (err == -EBUSY)
> +				continue;
> +
> +			return err;
> +		}
> +
> +		if (err == 0)
> +			return -EPROTO;
> +
> +		switch (msg.reply & DP_AUX_NATIVE_REPLY_MASK) {
> +		case DP_AUX_NATIVE_REPLY_ACK:
> +			return err;
> +
> +		case DP_AUX_NATIVE_REPLY_NACK:
> +			return -EIO;
> +
> +		case DP_AUX_NATIVE_REPLY_DEFER:
> +			usleep_range(400, 500);
> +			break;
> +		}
> +	}
> +
> +	DRM_ERROR("too many retries, giving up\n");
> +	return -EIO;
> +}
> +EXPORT_SYMBOL(drm_dp_dpcd_read);
> +
> +/**
> + * drm_dp_dpcd_write() - write a series of bytes to the DPCD
> + * @aux: DisplayPort AUX channel
> + * @offset: address of the (first) register to write
> + * @buffer: buffer containing the values to write
> + * @size: number of bytes in @buffer
> + *
> + * Returns the number of bytes transferred on success, or a negative error
> + * code on failure.
> + */
> +ssize_t drm_dp_dpcd_write(struct drm_dp_aux *aux, unsigned int offset,
> +			  void *buffer, size_t size)
> +{
> +	struct drm_dp_aux_msg msg;
> +	unsigned int retry;
> +	int err;
> +
> +	memset(&msg, 0, sizeof(msg));
> +	msg.address = offset;
> +	msg.request = DP_AUX_NATIVE_WRITE;
> +	msg.buffer = buffer;
> +	msg.size = size;
> +
> +	/*
> +	 * The specification doesn't give any recommendation on how often to
> +	 * retry native transactions, so retry 7 times like for I2C-over-AUX
> +	 * transactions.
> +	 */
> +	for (retry = 0; retry < 7; retry++) {
> +		err = aux->transfer(aux, &msg);
> +		if (err < 0) {
> +			if (err == -EBUSY)
> +				continue;
> +
> +			return err;
> +		}
> +
> +		if (err == 0)
> +			return -EPROTO;
> +
> +		switch (msg.reply & DP_AUX_NATIVE_REPLY_MASK) {
> +		case DP_AUX_NATIVE_REPLY_ACK:
> +			return 0;

"Returns the number of bytes transferred on success, or a negative error
code on failure." Compare drm_dp_dpcd_read.

You could add an internal helper to do both read and write, it's mostly
the same code.

BR,
Jani.

> +
> +		case DP_AUX_NATIVE_REPLY_NACK:
> +			return -EIO;
> +
> +		case DP_AUX_NATIVE_REPLY_DEFER:
> +			usleep_range(400, 500);
> +			break;
> +		}
> +	}
> +
> +	DRM_ERROR("too many retries, giving up\n");
> +	return -EIO;
> +}
> +EXPORT_SYMBOL(drm_dp_dpcd_write);
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index c873f9ce5871..0b14e799cd98 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -397,4 +397,68 @@ drm_dp_enhanced_frame_cap(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
>  		(dpcd[DP_MAX_LANE_COUNT] & DP_ENHANCED_FRAME_CAP);
>  }
>  
> +/*
> + * DisplayPort AUX channel
> + */
> +
> +/**
> + * struct drm_dp_aux_msg - DisplayPort AUX channel transaction
> + * @address: address of the (first) register to access
> + * @flags: contains the type of transaction as well as flags (see above)
> + * @reply: upon completion, contains the reply type of the transaction
> + * @buffer: pointer to a transmission or reception buffer
> + * @size: size of @buffer
> + */
> +struct drm_dp_aux_msg {
> +	unsigned int address;
> +	u8 request;
> +	u8 reply;
> +	void *buffer;
> +	size_t size;
> +};
> +
> +/**
> + * struct drm_dp_aux - DisplayPort AUX channel
> + * @transfer: transfers a message representing a single AUX transaction
> + */
> +struct drm_dp_aux {
> +	ssize_t (*transfer)(struct drm_dp_aux *aux,
> +			    struct drm_dp_aux_msg *msg);
> +};
> +
> +ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
> +			 void *buffer, size_t size);
> +ssize_t drm_dp_dpcd_write(struct drm_dp_aux *aux, unsigned int offset,
> +			  void *buffer, size_t size);
> +
> +/**
> + * drm_dp_dpcd_readb() - read a single byte from the DPCD
> + * @aux: DisplayPort AUX channel
> + * @offset: address of the register to read
> + * @valuep: location where the value of the register will be stored
> + *
> + * Returns the number of bytes transferred (1) on success, or a negative
> + * error code on failure.
> + */
> +static inline ssize_t drm_dp_dpcd_readb(struct drm_dp_aux *aux,
> +					unsigned int offset, u8 *valuep)
> +{
> +	return drm_dp_dpcd_read(aux, offset, valuep, 1);
> +}
> +
> +/**
> + * drm_dp_dpcd_writeb() - write a single byte to the DPCD
> + * @aux: DisplayPort AUX channel
> + * @offset: address of the register to write
> + * @valuep: value to write to the register
> + *
> + * Returns the number of bytes transferred (1) on success, or a negative
> + * error code on failure.
> + */
> +static inline ssize_t drm_dp_dpcd_writeb(struct drm_dp_aux *aux, u8 value,
> +					 unsigned int offset)
> +{
> +	return drm_dp_dpcd_write(aux, offset, &value, 1);
> +}
> +
>  #endif /* _DRM_DP_HELPER_H_ */
> -- 
> 1.8.4.2
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH v2 1/4] drm/dp: Add AUX channel infrastructure

[Thierry Reding]

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

On Tue, Dec 17, 2013 at 06:44:30PM +0200, Jani Nikula wrote:
> On Tue, 17 Dec 2013, Thierry Reding <thierry.reding@gmail.com> wrote:
[...]
> > +/**
> > + * drm_dp_dpcd_write() - write a series of bytes to the DPCD
> > + * @aux: DisplayPort AUX channel
> > + * @offset: address of the (first) register to write
> > + * @buffer: buffer containing the values to write
> > + * @size: number of bytes in @buffer
> > + *
> > + * Returns the number of bytes transferred on success, or a negative error
> > + * code on failure.
> > + */
> > +ssize_t drm_dp_dpcd_write(struct drm_dp_aux *aux, unsigned int offset,
> > +			  void *buffer, size_t size)
[...]
> > +		switch (msg.reply & DP_AUX_NATIVE_REPLY_MASK) {
> > +		case DP_AUX_NATIVE_REPLY_ACK:
> > +			return 0;
> 
> "Returns the number of bytes transferred on success, or a negative error
> code on failure." Compare drm_dp_dpcd_read.

Good catch!

> You could add an internal helper to do both read and write, it's mostly
> the same code.

Yes, I've factored out drm_dp_dpcd_access() which takes an additional
request parameter. That's the only difference between both functions.

Thanks,
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

Re: [PATCH v2 1/4] drm/dp: Add AUX channel infrastructure

[Jani Nikula]

On Tue, 17 Dec 2013, Thierry Reding <thierry.reding@gmail.com> wrote:
> This is a superset of the current i2c_dp_aux bus functionality and can
> be used to transfer native AUX in addition to I2C-over-AUX messages.
>
> Helpers are provided to read and write the DPCD, either blockwise or
> byte-wise. Many of the existing helpers for DisplayPort take a copy of a
> portion of the DPCD and operate on that, without a way to write data
> back to the DPCD (e.g. for configuration of the link).
>
> Subsequent patches will build upon this infrastructure to provide common
> functionality in a generic way.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 116 ++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_dp_helper.h     |  64 ++++++++++++++++++++++
>  2 files changed, 180 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 9e978aae8972..bd622d462ca7 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -346,3 +346,119 @@ int drm_dp_bw_code_to_link_rate(u8 link_bw)
>  	}
>  }
>  EXPORT_SYMBOL(drm_dp_bw_code_to_link_rate);
> +
> +/**
> + * drm_dp_dpcd_read() - read a series of bytes from the DPCD
> + * @aux: DisplayPort AUX channel
> + * @offset: address of the (first) register to read
> + * @buffer: buffer to store the register values
> + * @size: number of bytes in @buffer
> + *
> + * Returns the number of bytes transferred on success, or a negative error
> + * code on failure.
> + */
> +ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
> +			 void *buffer, size_t size)
> +{
> +	struct drm_dp_aux_msg msg;
> +	unsigned int retry;
> +	int err;
> +
> +	memset(&msg, 0, sizeof(msg));
> +	msg.address = offset;
> +	msg.request = DP_AUX_NATIVE_READ;
> +	msg.buffer = buffer;
> +	msg.size = size;
> +
> +	/*
> +	 * The specification doesn't give any recommendation on how often to
> +	 * retry native transactions, so retry 7 times like for I2C-over-AUX
> +	 * transactions.
> +	 */
> +	for (retry = 0; retry < 7; retry++) {
> +		err = aux->transfer(aux, &msg);
> +		if (err < 0) {
> +			if (err == -EBUSY)
> +				continue;
> +
> +			return err;
> +		}
> +
> +		if (err == 0)
> +			return -EPROTO;
> +
> +		switch (msg.reply & DP_AUX_NATIVE_REPLY_MASK) {
> +		case DP_AUX_NATIVE_REPLY_ACK:
> +			return err;
> +
> +		case DP_AUX_NATIVE_REPLY_NACK:
> +			return -EIO;
> +
> +		case DP_AUX_NATIVE_REPLY_DEFER:
> +			usleep_range(400, 500);
> +			break;
> +		}
> +	}
> +
> +	DRM_ERROR("too many retries, giving up\n");
> +	return -EIO;
> +}
> +EXPORT_SYMBOL(drm_dp_dpcd_read);
> +
> +/**
> + * drm_dp_dpcd_write() - write a series of bytes to the DPCD
> + * @aux: DisplayPort AUX channel
> + * @offset: address of the (first) register to write
> + * @buffer: buffer containing the values to write
> + * @size: number of bytes in @buffer
> + *
> + * Returns the number of bytes transferred on success, or a negative error
> + * code on failure.
> + */
> +ssize_t drm_dp_dpcd_write(struct drm_dp_aux *aux, unsigned int offset,
> +			  void *buffer, size_t size)
> +{
> +	struct drm_dp_aux_msg msg;
> +	unsigned int retry;
> +	int err;
> +
> +	memset(&msg, 0, sizeof(msg));
> +	msg.address = offset;
> +	msg.request = DP_AUX_NATIVE_WRITE;
> +	msg.buffer = buffer;
> +	msg.size = size;
> +
> +	/*
> +	 * The specification doesn't give any recommendation on how often to
> +	 * retry native transactions, so retry 7 times like for I2C-over-AUX
> +	 * transactions.
> +	 */
> +	for (retry = 0; retry < 7; retry++) {
> +		err = aux->transfer(aux, &msg);
> +		if (err < 0) {
> +			if (err == -EBUSY)
> +				continue;
> +
> +			return err;
> +		}
> +
> +		if (err == 0)
> +			return -EPROTO;
> +
> +		switch (msg.reply & DP_AUX_NATIVE_REPLY_MASK) {
> +		case DP_AUX_NATIVE_REPLY_ACK:
> +			return 0;
> +
> +		case DP_AUX_NATIVE_REPLY_NACK:
> +			return -EIO;
> +
> +		case DP_AUX_NATIVE_REPLY_DEFER:
> +			usleep_range(400, 500);
> +			break;
> +		}
> +	}
> +
> +	DRM_ERROR("too many retries, giving up\n");
> +	return -EIO;
> +}
> +EXPORT_SYMBOL(drm_dp_dpcd_write);
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index c873f9ce5871..0b14e799cd98 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -397,4 +397,68 @@ drm_dp_enhanced_frame_cap(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
>  		(dpcd[DP_MAX_LANE_COUNT] & DP_ENHANCED_FRAME_CAP);
>  }
>  
> +/*
> + * DisplayPort AUX channel
> + */
> +
> +/**
> + * struct drm_dp_aux_msg - DisplayPort AUX channel transaction
> + * @address: address of the (first) register to access
> + * @flags: contains the type of transaction as well as flags (see above)
> + * @reply: upon completion, contains the reply type of the transaction
> + * @buffer: pointer to a transmission or reception buffer
> + * @size: size of @buffer
> + */
> +struct drm_dp_aux_msg {
> +	unsigned int address;
> +	u8 request;
> +	u8 reply;
> +	void *buffer;
> +	size_t size;
> +};
> +
> +/**
> + * struct drm_dp_aux - DisplayPort AUX channel
> + * @transfer: transfers a message representing a single AUX transaction
> + */
> +struct drm_dp_aux {
> +	ssize_t (*transfer)(struct drm_dp_aux *aux,
> +			    struct drm_dp_aux_msg *msg);
> +};
> +
> +ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
> +			 void *buffer, size_t size);
> +ssize_t drm_dp_dpcd_write(struct drm_dp_aux *aux, unsigned int offset,
> +			  void *buffer, size_t size);
> +
> +/**
> + * drm_dp_dpcd_readb() - read a single byte from the DPCD
> + * @aux: DisplayPort AUX channel
> + * @offset: address of the register to read
> + * @valuep: location where the value of the register will be stored
> + *
> + * Returns the number of bytes transferred (1) on success, or a negative
> + * error code on failure.
> + */
> +static inline ssize_t drm_dp_dpcd_readb(struct drm_dp_aux *aux,
> +					unsigned int offset, u8 *valuep)
> +{
> +	return drm_dp_dpcd_read(aux, offset, valuep, 1);
> +}
> +
> +/**
> + * drm_dp_dpcd_writeb() - write a single byte to the DPCD
> + * @aux: DisplayPort AUX channel
> + * @offset: address of the register to write
> + * @valuep: value to write to the register
> + *
> + * Returns the number of bytes transferred (1) on success, or a negative
> + * error code on failure.
> + */
> +static inline ssize_t drm_dp_dpcd_writeb(struct drm_dp_aux *aux, u8 value,
> +					 unsigned int offset)

I'd much prefer offset before value parameter. Why would you have these
the other way around here than in all the other functions?

BR,
Jani.


> +{
> +	return drm_dp_dpcd_write(aux, offset, &value, 1);
> +}
> +
>  #endif /* _DRM_DP_HELPER_H_ */
> -- 
> 1.8.4.2
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH v2 1/4] drm/dp: Add AUX channel infrastructure

[Thierry Reding]

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

On Fri, Dec 20, 2013 at 03:08:21PM +0200, Jani Nikula wrote:
> On Tue, 17 Dec 2013, Thierry Reding <thierry.reding@gmail.com> wrote:
[...]
> > +/**
> > + * drm_dp_dpcd_writeb() - write a single byte to the DPCD
> > + * @aux: DisplayPort AUX channel
> > + * @offset: address of the register to write
> > + * @valuep: value to write to the register
> > + *
> > + * Returns the number of bytes transferred (1) on success, or a negative
> > + * error code on failure.
> > + */
> > +static inline ssize_t drm_dp_dpcd_writeb(struct drm_dp_aux *aux, u8 value,
> > +					 unsigned int offset)
> 
> I'd much prefer offset before value parameter. Why would you have these
> the other way around here than in all the other functions?

I guess I thought this would mirror the convention seen with readl() and
writel(), but I suppose since drm_dp_dpcd_readb() doesn't actually look
anything like readl() there's no consistency here anyway. I'll change it
around.

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

[PATCH v2 2/4] drm/dp: Add drm_dp_dpcd_read_link_status()

[Thierry Reding]

The function reads the link status (6 bytes starting at offset 0x202)
from the DPCD so that it can be conveniently passed to other DPCD
helpers.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 16 ++++++++++++++++
 include/drm/drm_dp_helper.h     |  3 +++
 2 files changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index bd622d462ca7..8111b8e5a8bc 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -462,3 +462,19 @@ ssize_t drm_dp_dpcd_write(struct drm_dp_aux *aux, unsigned int offset,
 	return -EIO;
 }
 EXPORT_SYMBOL(drm_dp_dpcd_write);
+
+/**
+ * drm_dp_dpcd_read_link_status() - read DPCD link status (bytes 0x202-0x207)
+ * @aux: DisplayPort AUX channel
+ * @status: buffer to store the link status in (must be at least 6 bytes)
+ *
+ * Returns the number of bytes transferred on success or a negative error
+ * code on failure.
+ */
+int drm_dp_dpcd_read_link_status(struct drm_dp_aux *aux,
+				 u8 status[DP_LINK_STATUS_SIZE])
+{
+	return drm_dp_dpcd_read(aux, DP_LANE0_1_STATUS, status,
+				DP_LINK_STATUS_SIZE);
+}
+EXPORT_SYMBOL(drm_dp_dpcd_read_link_status);
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 0b14e799cd98..38cbaef05005 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -461,4 +461,7 @@ static inline ssize_t drm_dp_dpcd_writeb(struct drm_dp_aux *aux, u8 value,
 	return drm_dp_dpcd_write(aux, offset, &value, 1);
 }
 
+int drm_dp_dpcd_read_link_status(struct drm_dp_aux *aux,
+				 u8 status[DP_LINK_STATUS_SIZE]);
+
 #endif /* _DRM_DP_HELPER_H_ */
-- 
1.8.4.2

[PATCH v2 3/4] drm/dp: Add DisplayPort link helpers

[Thierry Reding]

Add a helper to probe a DP link (reading out the maximum rate, link
count and capabilities) as well as configuring it accordingly.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 77 +++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_dp_helper.h     | 30 ++++++++++++++++
 2 files changed, 107 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 8111b8e5a8bc..01a8173c6e83 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -478,3 +478,80 @@ int drm_dp_dpcd_read_link_status(struct drm_dp_aux *aux,
 				DP_LINK_STATUS_SIZE);
 }
 EXPORT_SYMBOL(drm_dp_dpcd_read_link_status);
+
+/**
+ * drm_dp_link_probe() - probe a DisplayPort link for capabilities
+ * @aux: DisplayPort AUX channel
+ * @link: pointer to structure in which to return link capabilities
+ *
+ * The structure filled in by this function can usually be passed directly
+ * into drm_dp_link_power_up() configure the link based on the link's
+ * capabilities.
+ *
+ * Returns 0 on success or a negative error code on failure.
+ */
+int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link)
+{
+	u8 value;
+	int err;
+
+	memset(link, 0, sizeof(*link));
+
+	err = drm_dp_dpcd_readb(aux, DP_MAX_LINK_RATE, &value);
+	if (err < 0)
+		return err;
+
+	link->rate = drm_dp_bw_code_to_link_rate(value);
+
+	err = drm_dp_dpcd_readb(aux, DP_MAX_LANE_COUNT, &value);
+	if (err < 0)
+		return err;
+
+	link->num_lanes = value & DP_MAX_LANE_COUNT_MASK;
+
+	if (value & DP_ENHANCED_FRAME_CAP)
+		link->capabilities |= DP_LINK_CAP_ENHANCED_FRAMING;
+
+	return 0;
+}
+
+/**
+ * drm_dp_link_power_up() - power up and configure a DisplayPort link
+ * @aux: DisplayPort AUX channel
+ * @link: pointer to a structure containing the link configuration
+ *
+ * Returns 0 on success or a negative error code on failure.
+ */
+int drm_dp_link_power_up(struct drm_dp_aux *aux, struct drm_dp_link *link)
+{
+	u8 value;
+	int err;
+
+	err = drm_dp_dpcd_readb(aux, DP_SET_POWER, &value);
+	if (err < 0)
+		return err;
+
+	value &= ~DP_SET_POWER_MASK;
+	value |= DP_SET_POWER_D0;
+
+	err = drm_dp_dpcd_writeb(aux, value, DP_SET_POWER);
+	if (err < 0)
+		return err;
+
+	value = drm_dp_link_rate_to_bw_code(link->rate);
+
+	err = drm_dp_dpcd_writeb(aux, value, DP_LINK_BW_SET);
+	if (err < 0)
+		return err;
+
+	value = link->num_lanes;
+
+	if (link->capabilities & DP_LINK_CAP_ENHANCED_FRAMING)
+		value |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
+
+	err = drm_dp_dpcd_writeb(aux, value, DP_LANE_COUNT_SET);
+	if (err < 0)
+		return err;
+
+	return 0;
+}
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 38cbaef05005..bad9ee11a2e2 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -290,6 +290,7 @@
 #define DP_SET_POWER                        0x600
 # define DP_SET_POWER_D0                    0x1
 # define DP_SET_POWER_D3                    0x2
+# define DP_SET_POWER_MASK                  0x3
 
 #define DP_PSR_ERROR_STATUS                 0x2006  /* XXX 1.2? */
 # define DP_PSR_LINK_CRC_ERROR              (1 << 0)
@@ -464,4 +465,33 @@ static inline ssize_t drm_dp_dpcd_writeb(struct drm_dp_aux *aux, u8 value,
 int drm_dp_dpcd_read_link_status(struct drm_dp_aux *aux,
 				 u8 status[DP_LINK_STATUS_SIZE]);
 
+/*
+ * DisplayPort link
+ */
+#define DP_LINK_CAP_ENHANCED_FRAMING (1 << 0)
+
+struct drm_dp_link {
+	unsigned int rate;
+	unsigned int num_lanes;
+	unsigned long capabilities;
+};
+
+/**
+ * drm_dp_link_probe() - probe link properties and capabilities
+ * @aux: DisplayPort AUX channel
+ * @link: DisplayPort link
+ *
+ * Returns 0 on success or a negative error code on failure.
+ */
+int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link);
+
+/**
+ * drm_dp_link_power_up() - power up a link
+ * @aux: DisplayPort AUX channel
+ * @link: DisplayPort link
+ *
+ * Returns 0 on success or a negative error code on failure.
+ */
+int drm_dp_link_power_up(struct drm_dp_aux *aux, struct drm_dp_link *link);
+
 #endif /* _DRM_DP_HELPER_H_ */
-- 
1.8.4.2

Re: [PATCH v2 3/4] drm/dp: Add DisplayPort link helpers

[Jani Nikula]

On Tue, 17 Dec 2013, Thierry Reding <thierry.reding@gmail.com> wrote:
> Add a helper to probe a DP link (reading out the maximum rate, link
> count and capabilities) as well as configuring it accordingly.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 77 +++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_dp_helper.h     | 30 ++++++++++++++++
>  2 files changed, 107 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 8111b8e5a8bc..01a8173c6e83 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -478,3 +478,80 @@ int drm_dp_dpcd_read_link_status(struct drm_dp_aux *aux,
>  				DP_LINK_STATUS_SIZE);
>  }
>  EXPORT_SYMBOL(drm_dp_dpcd_read_link_status);
> +
> +/**
> + * drm_dp_link_probe() - probe a DisplayPort link for capabilities
> + * @aux: DisplayPort AUX channel
> + * @link: pointer to structure in which to return link capabilities
> + *
> + * The structure filled in by this function can usually be passed directly
> + * into drm_dp_link_power_up() configure the link based on the link's
> + * capabilities.
> + *
> + * Returns 0 on success or a negative error code on failure.
> + */
> +int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link)
> +{
> +	u8 value;
> +	int err;
> +
> +	memset(link, 0, sizeof(*link));
> +
> +	err = drm_dp_dpcd_readb(aux, DP_MAX_LINK_RATE, &value);
> +	if (err < 0)
> +		return err;
> +
> +	link->rate = drm_dp_bw_code_to_link_rate(value);
> +
> +	err = drm_dp_dpcd_readb(aux, DP_MAX_LANE_COUNT, &value);
> +	if (err < 0)
> +		return err;
> +
> +	link->num_lanes = value & DP_MAX_LANE_COUNT_MASK;
> +
> +	if (value & DP_ENHANCED_FRAME_CAP)
> +		link->capabilities |= DP_LINK_CAP_ENHANCED_FRAMING;
> +
> +	return 0;
> +}

Okay, I'm biased due to the i915 implementation, but I think it's better
to read the first 16 bytes of DPCD in one block, cache that, and provide
accessors to the information. Keep the DPCD reads and writes to a
minimum.

> +
> +/**
> + * drm_dp_link_power_up() - power up and configure a DisplayPort link
> + * @aux: DisplayPort AUX channel
> + * @link: pointer to a structure containing the link configuration
> + *
> + * Returns 0 on success or a negative error code on failure.
> + */
> +int drm_dp_link_power_up(struct drm_dp_aux *aux, struct drm_dp_link *link)
> +{
> +	u8 value;
> +	int err;
> +
> +	err = drm_dp_dpcd_readb(aux, DP_SET_POWER, &value);

If this is turning on the sink, we'll need to give the sink some time to
wake up, and retry, per the DP spec.

Maybe I'd leave setting DP_SET_POWER up to the drivers altogether, at
least for now. (Again, I'm biased, and this would make it easier for
i915 to adopt this stuff. ;)

BR,
Jani.

> +	if (err < 0)
> +		return err;
> +
> +	value &= ~DP_SET_POWER_MASK;
> +	value |= DP_SET_POWER_D0;
> +
> +	err = drm_dp_dpcd_writeb(aux, value, DP_SET_POWER);
> +	if (err < 0)
> +		return err;
> +
> +	value = drm_dp_link_rate_to_bw_code(link->rate);
> +
> +	err = drm_dp_dpcd_writeb(aux, value, DP_LINK_BW_SET);
> +	if (err < 0)
> +		return err;
> +
> +	value = link->num_lanes;
> +
> +	if (link->capabilities & DP_LINK_CAP_ENHANCED_FRAMING)
> +		value |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
> +
> +	err = drm_dp_dpcd_writeb(aux, value, DP_LANE_COUNT_SET);
> +	if (err < 0)
> +		return err;

IMO you should always combine DPCD reads and writes as much as
possible. DP_LINK_BW_SET and DP_LANE_COUNT_SET are back to back.


> +
> +	return 0;
> +}
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 38cbaef05005..bad9ee11a2e2 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -290,6 +290,7 @@
>  #define DP_SET_POWER                        0x600
>  # define DP_SET_POWER_D0                    0x1
>  # define DP_SET_POWER_D3                    0x2
> +# define DP_SET_POWER_MASK                  0x3
>  
>  #define DP_PSR_ERROR_STATUS                 0x2006  /* XXX 1.2? */
>  # define DP_PSR_LINK_CRC_ERROR              (1 << 0)
> @@ -464,4 +465,33 @@ static inline ssize_t drm_dp_dpcd_writeb(struct drm_dp_aux *aux, u8 value,
>  int drm_dp_dpcd_read_link_status(struct drm_dp_aux *aux,
>  				 u8 status[DP_LINK_STATUS_SIZE]);
>  
> +/*
> + * DisplayPort link
> + */
> +#define DP_LINK_CAP_ENHANCED_FRAMING (1 << 0)
> +
> +struct drm_dp_link {
> +	unsigned int rate;
> +	unsigned int num_lanes;
> +	unsigned long capabilities;
> +};
> +
> +/**
> + * drm_dp_link_probe() - probe link properties and capabilities
> + * @aux: DisplayPort AUX channel
> + * @link: DisplayPort link
> + *
> + * Returns 0 on success or a negative error code on failure.
> + */
> +int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link);
> +
> +/**
> + * drm_dp_link_power_up() - power up a link
> + * @aux: DisplayPort AUX channel
> + * @link: DisplayPort link
> + *
> + * Returns 0 on success or a negative error code on failure.
> + */
> +int drm_dp_link_power_up(struct drm_dp_aux *aux, struct drm_dp_link *link);
> +
>  #endif /* _DRM_DP_HELPER_H_ */
> -- 
> 1.8.4.2
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center

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

[Thierry Reding]

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@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.
+			 *
+			 * 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:
+			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

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

[Jani Nikula]

On Tue, 17 Dec 2013, 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.
>
> Signed-off-by: Thierry Reding <treding@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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center

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

[Thierry Reding]

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

On Tue, Dec 17, 2013 at 07:11:21PM +0200, Jani Nikula wrote:
> On Tue, 17 Dec 2013, Thierry Reding <thierry.reding@gmail.com> wrote:
[...]
> > +		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

Fixed.

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

Okay, I'll add those back in. I'll assume the same goes for DRM_ERROR
messages.

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

Sure, we should be able to easily do that. One possible alternative to
this would be to handle in in the drivers by handling DEFER explicitly
and then returning -EBUSY. -EBUSY will cause native AUX transactions to
be retried and I just noticed that radeon does the same for I2C-over-AUX
transactions, so I'll add that special case there as well.

The benefit of doing that would be that the helper stays relatively
simple. On the downside it may not be as explicit as a driver-specific
callback.

Thanks,
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

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

[Daniel Vetter]

On Tue, Dec 17, 2013 at 05:20:07PM +0100, Thierry Reding 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@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;
> +};

I'd just embedded an i2c adapter into the drm_dp_aux structure - I think
drivers always want such a thing. Then maybe rename the setup function
from add to register_i2c_bus or so. For smoother transition drivers can
always store a pointer to this i2c_adapter somewhere more convenient for
them.
-Daniel

> +
> +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.
> +			 *
> +			 * 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:
> +			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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Thierry Reding]

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

On Wed, Dec 18, 2013 at 09:52:47AM +0100, Daniel Vetter wrote:
> On Tue, Dec 17, 2013 at 05:20:07PM +0100, Thierry Reding 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@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;
> > +};
> 
> I'd just embedded an i2c adapter into the drm_dp_aux structure - I think
> drivers always want such a thing. Then maybe rename the setup function
> from add to register_i2c_bus or so. For smoother transition drivers can
> always store a pointer to this i2c_adapter somewhere more convenient for
> them.

Okay, I've made that change. I was slightly worried that it might cause
lifetime issues, but looking somewhat more closely perhaps that's not an
issue since i2c_adapters seem to be carefully designed to easily allow
being embedded.

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