All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 01/16] drm/dsi: Add message to packet translator
@ 2014-11-03  9:13 Thierry Reding
  2014-11-03  9:13 ` [PATCH v4 02/16] drm/dsi: Add DSI transfer helper Thierry Reding
                   ` (15 more replies)
  0 siblings, 16 replies; 28+ messages in thread
From: Thierry Reding @ 2014-11-03  9:13 UTC (permalink / raw)
  To: Thierry Reding; +Cc: Andrzej Hajda, dri-devel

From: Thierry Reding <treding@nvidia.com>

This commit introduces a new function, mipi_dsi_create_packet(), which
converts from a MIPI DSI message to a MIPI DSI packet. The MIPI DSI
packet is as close to the protocol described in the DSI specification as
possible and useful in drivers that need to write a DSI packet into a
FIFO to send a message off to the peripheral.

Suggested-by: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_mipi_dsi.c | 45 ++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_mipi_dsi.h     | 18 +++++++++++++++++
 2 files changed, 63 insertions(+)

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index eb6dfe52cab2..76e81aba8220 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -199,6 +199,51 @@ int mipi_dsi_detach(struct mipi_dsi_device *dsi)
 EXPORT_SYMBOL(mipi_dsi_detach);
 
 /**
+ * mipi_dsi_create_packet - create a packet from a message according to the
+ *     DSI protocol
+ * @packet: pointer to a DSI packet structure
+ * @msg: message to translate into a packet
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
+int mipi_dsi_create_packet(struct mipi_dsi_packet *packet,
+			   const struct mipi_dsi_msg *msg)
+{
+	const u8 *tx = msg->tx_buf;
+
+	if (!packet || !msg)
+		return -EINVAL;
+
+	memset(packet, 0, sizeof(*packet));
+	packet->header[0] = ((msg->channel & 0x3) << 6) | (msg->type & 0x3f);
+
+	/* TODO: compute ECC if hardware support is not available */
+
+	/*
+	 * Long write packets contain the word count in header bytes 1 and 2.
+	 * The payload follows the header and is word count bytes long.
+	 *
+	 * Short write packets encode up to two parameters in header bytes 1
+	 * and 2.
+	 */
+	if (msg->tx_len > 2) {
+		packet->header[1] = (msg->tx_len >> 0) & 0xff;
+		packet->header[2] = (msg->tx_len >> 8) & 0xff;
+
+		packet->payload_length = msg->tx_len;
+		packet->payload = tx;
+	} else {
+		packet->header[1] = (msg->tx_len > 0) ? tx[0] : 0;
+		packet->header[2] = (msg->tx_len > 1) ? tx[1] : 0;
+	}
+
+	packet->size = sizeof(packet->header) + packet->payload_length;
+
+	return 0;
+}
+EXPORT_SYMBOL(mipi_dsi_create_packet);
+
+/**
  * mipi_dsi_dcs_write - send DCS write command
  * @dsi: DSI device
  * @data: pointer to the command followed by parameters
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index 8569dc5a1026..663aa68826f4 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -44,6 +44,24 @@ struct mipi_dsi_msg {
 };
 
 /**
+ * struct mipi_dsi_packet - represents a MIPI DSI packet in protocol format
+ * @size: size (in bytes) of the packet
+ * @header: the four bytes that make up the header (Data ID, Word Count or
+ *     Packet Data, and ECC)
+ * @payload_length: number of bytes in the payload
+ * @payload: a pointer to a buffer containing the payload, if any
+ */
+struct mipi_dsi_packet {
+	size_t size;
+	u8 header[4];
+	size_t payload_length;
+	const u8 *payload;
+};
+
+int mipi_dsi_create_packet(struct mipi_dsi_packet *packet,
+			   const struct mipi_dsi_msg *msg);
+
+/**
  * struct mipi_dsi_host_ops - DSI bus operations
  * @attach: attach DSI device to DSI host
  * @detach: detach DSI device from DSI host
-- 
2.1.2

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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v4 02/16] drm/dsi: Add DSI transfer helper
  2014-11-03  9:13 [PATCH v4 01/16] drm/dsi: Add message to packet translator Thierry Reding
@ 2014-11-03  9:13 ` Thierry Reding
  2014-11-04 11:47   ` Andrzej Hajda
  2014-11-03  9:13 ` [PATCH v4 03/16] drm/dsi: Make mipi_dsi_dcs_{read, write}() symmetrical Thierry Reding
                   ` (14 subsequent siblings)
  15 siblings, 1 reply; 28+ messages in thread
From: Thierry Reding @ 2014-11-03  9:13 UTC (permalink / raw)
  To: Thierry Reding; +Cc: Andrzej Hajda, dri-devel

From: Thierry Reding <treding@nvidia.com>

A common pattern is starting to emerge for higher level transfer
helpers. Create a new helper that encapsulates this pattern and avoids
code duplication.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_mipi_dsi.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index 76e81aba8220..89a228b4eacc 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -198,6 +198,20 @@ int mipi_dsi_detach(struct mipi_dsi_device *dsi)
 }
 EXPORT_SYMBOL(mipi_dsi_detach);
 
+static ssize_t mipi_dsi_device_transfer(struct mipi_dsi_device *dsi,
+					struct mipi_dsi_msg *msg)
+{
+	const struct mipi_dsi_host_ops *ops = dsi->host->ops;
+
+	if (!ops || !ops->transfer)
+		return -ENOSYS;
+
+	if (dsi->mode_flags & MIPI_DSI_MODE_LPM)
+		msg->flags |= MIPI_DSI_MSG_USE_LPM;
+
+	return ops->transfer(dsi->host, msg);
+}
+
 /**
  * mipi_dsi_create_packet - create a packet from a message according to the
  *     DSI protocol
@@ -252,16 +266,12 @@ EXPORT_SYMBOL(mipi_dsi_create_packet);
 ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, const void *data,
 			    size_t len)
 {
-	const struct mipi_dsi_host_ops *ops = dsi->host->ops;
 	struct mipi_dsi_msg msg = {
 		.channel = dsi->channel,
 		.tx_buf = data,
 		.tx_len = len
 	};
 
-	if (!ops || !ops->transfer)
-		return -ENOSYS;
-
 	switch (len) {
 	case 0:
 		return -EINVAL;
@@ -276,10 +286,7 @@ ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, const void *data,
 		break;
 	}
 
-	if (dsi->mode_flags & MIPI_DSI_MODE_LPM)
-		msg.flags = MIPI_DSI_MSG_USE_LPM;
-
-	return ops->transfer(dsi->host, &msg);
+	return mipi_dsi_device_transfer(dsi, &msg);
 }
 EXPORT_SYMBOL(mipi_dsi_dcs_write);
 
@@ -295,7 +302,6 @@ EXPORT_SYMBOL(mipi_dsi_dcs_write);
 ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, u8 cmd, void *data,
 			  size_t len)
 {
-	const struct mipi_dsi_host_ops *ops = dsi->host->ops;
 	struct mipi_dsi_msg msg = {
 		.channel = dsi->channel,
 		.type = MIPI_DSI_DCS_READ,
@@ -305,13 +311,7 @@ ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, u8 cmd, void *data,
 		.rx_len = len
 	};
 
-	if (!ops || !ops->transfer)
-		return -ENOSYS;
-
-	if (dsi->mode_flags & MIPI_DSI_MODE_LPM)
-		msg.flags = MIPI_DSI_MSG_USE_LPM;
-
-	return ops->transfer(dsi->host, &msg);
+	return mipi_dsi_device_transfer(dsi, &msg);
 }
 EXPORT_SYMBOL(mipi_dsi_dcs_read);
 
-- 
2.1.2

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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v4 03/16] drm/dsi: Make mipi_dsi_dcs_{read, write}() symmetrical
  2014-11-03  9:13 [PATCH v4 01/16] drm/dsi: Add message to packet translator Thierry Reding
  2014-11-03  9:13 ` [PATCH v4 02/16] drm/dsi: Add DSI transfer helper Thierry Reding
@ 2014-11-03  9:13 ` Thierry Reding
  2014-11-03  9:13 ` [PATCH v4 04/16] drm/dsi: Constify mipi_dsi_msg Thierry Reding
                   ` (13 subsequent siblings)
  15 siblings, 0 replies; 28+ messages in thread
From: Thierry Reding @ 2014-11-03  9:13 UTC (permalink / raw)
  To: Thierry Reding; +Cc: Andrzej Hajda, dri-devel

From: Thierry Reding <treding@nvidia.com>

Currently the mipi_dsi_dcs_write() function requires the DCS command
byte to be embedded within the write buffer whereas mipi_dsi_dcs_read()
has a separate parameter. Make them more symmetrical by adding an extra
command parameter to mipi_dsi_dcs_write().

The S6E8AA0 driver relies on the old asymmetric API and there's concern
that moving to the new API may be less efficient. Provide a new function
with the old semantics for those cases and make the S6E8AA0 driver use
it instead.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v4:
- do not handle protocol-level details in mipi_dsi_dcs_write()

Changes in v3:
- reuse mipi_dsi_dcs_write_buffer() in mipi_dsi_dcs_write()
- keep local ops variable for consistency
- use common helper to simplify code
- fix typo in comment

Changes in v2:
- provide mipi_dsi_dcs_write_buffer() for backwards compatibility

 drivers/gpu/drm/drm_mipi_dsi.c        | 77 +++++++++++++++++++++++++++++------
 drivers/gpu/drm/panel/panel-s6e8aa0.c |  2 +-
 include/drm/drm_mipi_dsi.h            |  6 ++-
 3 files changed, 70 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index 89a228b4eacc..aa1aab24181c 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -258,13 +258,19 @@ int mipi_dsi_create_packet(struct mipi_dsi_packet *packet,
 EXPORT_SYMBOL(mipi_dsi_create_packet);
 
 /**
- * mipi_dsi_dcs_write - send DCS write command
- * @dsi: DSI device
- * @data: pointer to the command followed by parameters
- * @len: length of @data
+ * mipi_dsi_dcs_write_buffer() - transmit a DCS command with payload
+ * @dsi: DSI peripheral device
+ * @data: buffer containing data to be transmitted
+ * @len: size of transmission buffer
+ *
+ * This function will automatically choose the right data type depending on
+ * the command payload length.
+ *
+ * Return: The number of bytes successfully transmitted or a negative error
+ * code on failure.
  */
-ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, const void *data,
-			    size_t len)
+ssize_t mipi_dsi_dcs_write_buffer(struct mipi_dsi_device *dsi,
+				  const void *data, size_t len)
 {
 	struct mipi_dsi_msg msg = {
 		.channel = dsi->channel,
@@ -275,12 +281,15 @@ ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, const void *data,
 	switch (len) {
 	case 0:
 		return -EINVAL;
+
 	case 1:
 		msg.type = MIPI_DSI_DCS_SHORT_WRITE;
 		break;
+
 	case 2:
 		msg.type = MIPI_DSI_DCS_SHORT_WRITE_PARAM;
 		break;
+
 	default:
 		msg.type = MIPI_DSI_DCS_LONG_WRITE;
 		break;
@@ -288,16 +297,60 @@ ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, const void *data,
 
 	return mipi_dsi_device_transfer(dsi, &msg);
 }
+EXPORT_SYMBOL(mipi_dsi_dcs_write_buffer);
+
+/**
+ * mipi_dsi_dcs_write() - send DCS write command
+ * @dsi: DSI peripheral device
+ * @cmd: DCS command
+ * @data: buffer containing the command payload
+ * @len: command payload length
+ *
+ * This function will automatically choose the right data type depending on
+ * the command payload length.
+ *
+ * Return: The number of bytes successfully transmitted or a negative error
+ * code on failure.
+ */
+ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, u8 cmd,
+			   const void *data, size_t len)
+{
+	ssize_t err;
+	size_t size;
+	u8 *tx;
+
+	if (len > 0) {
+		size = 1 + len;
+
+		tx = kmalloc(size, GFP_KERNEL);
+		if (!tx)
+			return -ENOMEM;
+
+		/* concatenate the DCS command byte and the payload */
+		tx[0] = cmd;
+		memcpy(&tx[1], data, len);
+	} else {
+		tx = &cmd;
+		size = 1;
+	}
+
+	err = mipi_dsi_dcs_write_buffer(dsi, tx, size);
+
+	if (len > 0)
+		kfree(tx);
+
+	return err;
+}
 EXPORT_SYMBOL(mipi_dsi_dcs_write);
 
 /**
- * mipi_dsi_dcs_read - send DCS read request command
- * @dsi: DSI device
- * @cmd: DCS read command
- * @data: pointer to read buffer
- * @len: length of @data
+ * mipi_dsi_dcs_read() - send DCS read request command
+ * @dsi: DSI peripheral device
+ * @cmd: DCS command
+ * @data: buffer in which to receive data
+ * @len: size of receive buffer
  *
- * Function returns number of read bytes or error code.
+ * Return: The number of bytes read or a negative error code on failure.
  */
 ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, u8 cmd, void *data,
 			  size_t len)
diff --git a/drivers/gpu/drm/panel/panel-s6e8aa0.c b/drivers/gpu/drm/panel/panel-s6e8aa0.c
index b5217fe37f02..0f85a7c37687 100644
--- a/drivers/gpu/drm/panel/panel-s6e8aa0.c
+++ b/drivers/gpu/drm/panel/panel-s6e8aa0.c
@@ -141,7 +141,7 @@ static void s6e8aa0_dcs_write(struct s6e8aa0 *ctx, const void *data, size_t len)
 	if (ctx->error < 0)
 		return;
 
-	ret = mipi_dsi_dcs_write(dsi, data, len);
+	ret = mipi_dsi_dcs_write_buffer(dsi, data, len);
 	if (ret < 0) {
 		dev_err(ctx->dev, "error %zd writing dcs seq: %*ph\n", ret, len,
 			data);
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index 663aa68826f4..e37b1962ab7e 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -150,8 +150,10 @@ static inline struct mipi_dsi_device *to_mipi_dsi_device(struct device *dev)
 
 int mipi_dsi_attach(struct mipi_dsi_device *dsi);
 int mipi_dsi_detach(struct mipi_dsi_device *dsi);
-ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, const void *data,
-			    size_t len);
+ssize_t mipi_dsi_dcs_write_buffer(struct mipi_dsi_device *dsi,
+				  const void *data, size_t len);
+ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, u8 cmd,
+			   const void *data, size_t len);
 ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, u8 cmd, void *data,
 			  size_t len);
 
-- 
2.1.2

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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v4 04/16] drm/dsi: Constify mipi_dsi_msg
  2014-11-03  9:13 [PATCH v4 01/16] drm/dsi: Add message to packet translator Thierry Reding
  2014-11-03  9:13 ` [PATCH v4 02/16] drm/dsi: Add DSI transfer helper Thierry Reding
  2014-11-03  9:13 ` [PATCH v4 03/16] drm/dsi: Make mipi_dsi_dcs_{read, write}() symmetrical Thierry Reding
@ 2014-11-03  9:13 ` Thierry Reding
  2014-11-03  9:13 ` [PATCH v4 05/16] drm/dsi: Add mipi_dsi_set_maximum_return_packet_size() helper Thierry Reding
                   ` (12 subsequent siblings)
  15 siblings, 0 replies; 28+ messages in thread
From: Thierry Reding @ 2014-11-03  9:13 UTC (permalink / raw)
  To: Thierry Reding; +Cc: Andrzej Hajda, dri-devel

From: Thierry Reding <treding@nvidia.com>

struct mipi_dsi_msg is a read-only structure, drivers should never need
to modify it. Make this explicit by making all references to the struct
const.

Acked-by: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/exynos/exynos_drm_dsi.c | 2 +-
 include/drm/drm_mipi_dsi.h              | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index acf7e9e39dcd..f43d25896f3b 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -1236,7 +1236,7 @@ static bool exynos_dsi_is_short_dsi_type(u8 type)
 }
 
 static ssize_t exynos_dsi_host_transfer(struct mipi_dsi_host *host,
-				       struct mipi_dsi_msg *msg)
+				        const struct mipi_dsi_msg *msg)
 {
 	struct exynos_dsi *dsi = host_to_dsi(host);
 	struct exynos_dsi_transfer xfer;
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index e37b1962ab7e..6600cf630585 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -74,7 +74,7 @@ struct mipi_dsi_host_ops {
 	int (*detach)(struct mipi_dsi_host *host,
 		      struct mipi_dsi_device *dsi);
 	ssize_t (*transfer)(struct mipi_dsi_host *host,
-			    struct mipi_dsi_msg *msg);
+			    const struct mipi_dsi_msg *msg);
 };
 
 /**
-- 
2.1.2

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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v4 05/16] drm/dsi: Add mipi_dsi_set_maximum_return_packet_size() helper
  2014-11-03  9:13 [PATCH v4 01/16] drm/dsi: Add message to packet translator Thierry Reding
                   ` (2 preceding siblings ...)
  2014-11-03  9:13 ` [PATCH v4 04/16] drm/dsi: Constify mipi_dsi_msg Thierry Reding
@ 2014-11-03  9:13 ` Thierry Reding
  2014-11-03  9:13 ` [PATCH v4 06/16] drm/panel: s6e8aa0: Use standard MIPI DSI function Thierry Reding
                   ` (11 subsequent siblings)
  15 siblings, 0 replies; 28+ messages in thread
From: Thierry Reding @ 2014-11-03  9:13 UTC (permalink / raw)
  To: Thierry Reding; +Cc: Andrzej Hajda, dri-devel

From: YoungJun Cho <yj44.cho@samsung.com>

This function can be used to set the maximum return packet size for a
MIPI DSI peripheral.

Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
[treding: endianess, kerneldoc, return value]
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v4:
- use C99 initializer

Changes in v3:
- use common helper to simplify code

 drivers/gpu/drm/drm_mipi_dsi.c | 24 ++++++++++++++++++++++++
 include/drm/drm_mipi_dsi.h     |  2 ++
 2 files changed, 26 insertions(+)

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index aa1aab24181c..640cabcd6c1f 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -257,6 +257,30 @@ int mipi_dsi_create_packet(struct mipi_dsi_packet *packet,
 }
 EXPORT_SYMBOL(mipi_dsi_create_packet);
 
+/*
+ * mipi_dsi_set_maximum_return_packet_size() - specify the maximum size of the
+ *    the payload in a long packet transmitted from the peripheral back to the
+ *    host processor
+ * @dsi: DSI peripheral device
+ * @value: the maximum size of the payload
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
+int mipi_dsi_set_maximum_return_packet_size(struct mipi_dsi_device *dsi,
+					    u16 value)
+{
+	u8 tx[2] = { value & 0xff, value >> 8 };
+	struct mipi_dsi_msg msg = {
+		.channel = dsi->channel,
+		.type = MIPI_DSI_SET_MAXIMUM_RETURN_PACKET_SIZE,
+		.tx_len = sizeof(tx),
+		.tx_buf = tx,
+	};
+
+	return mipi_dsi_device_transfer(dsi, &msg);
+}
+EXPORT_SYMBOL(mipi_dsi_set_maximum_return_packet_size);
+
 /**
  * mipi_dsi_dcs_write_buffer() - transmit a DCS command with payload
  * @dsi: DSI peripheral device
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index 6600cf630585..bab67099872b 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -150,6 +150,8 @@ static inline struct mipi_dsi_device *to_mipi_dsi_device(struct device *dev)
 
 int mipi_dsi_attach(struct mipi_dsi_device *dsi);
 int mipi_dsi_detach(struct mipi_dsi_device *dsi);
+int mipi_dsi_set_maximum_return_packet_size(struct mipi_dsi_device *dsi,
+					    u16 value);
 ssize_t mipi_dsi_dcs_write_buffer(struct mipi_dsi_device *dsi,
 				  const void *data, size_t len);
 ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, u8 cmd,
-- 
2.1.2

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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v4 06/16] drm/panel: s6e8aa0: Use standard MIPI DSI function
  2014-11-03  9:13 [PATCH v4 01/16] drm/dsi: Add message to packet translator Thierry Reding
                   ` (3 preceding siblings ...)
  2014-11-03  9:13 ` [PATCH v4 05/16] drm/dsi: Add mipi_dsi_set_maximum_return_packet_size() helper Thierry Reding
@ 2014-11-03  9:13 ` Thierry Reding
  2014-11-03  9:13 ` [PATCH v4 07/16] drm/dsi: Implement generic read and write commands Thierry Reding
                   ` (10 subsequent siblings)
  15 siblings, 0 replies; 28+ messages in thread
From: Thierry Reding @ 2014-11-03  9:13 UTC (permalink / raw)
  To: Thierry Reding; +Cc: Andrzej Hajda, dri-devel

From: Thierry Reding <treding@nvidia.com>

Use the newly introduced mipi_dsi_set_maximum_return_packet_size()
function to replace an open-coded version.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/panel/panel-s6e8aa0.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-s6e8aa0.c b/drivers/gpu/drm/panel/panel-s6e8aa0.c
index 0f85a7c37687..c31e2953f290 100644
--- a/drivers/gpu/drm/panel/panel-s6e8aa0.c
+++ b/drivers/gpu/drm/panel/panel-s6e8aa0.c
@@ -800,27 +800,15 @@ static void s6e8aa0_panel_init(struct s6e8aa0 *ctx)
 }
 
 static void s6e8aa0_set_maximum_return_packet_size(struct s6e8aa0 *ctx,
-						   int size)
+						   u16 size)
 {
 	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
-	const struct mipi_dsi_host_ops *ops = dsi->host->ops;
-	u8 buf[] = {size, 0};
-	struct mipi_dsi_msg msg = {
-		.channel = dsi->channel,
-		.type = MIPI_DSI_SET_MAXIMUM_RETURN_PACKET_SIZE,
-		.tx_len = sizeof(buf),
-		.tx_buf = buf
-	};
 	int ret;
 
 	if (ctx->error < 0)
 		return;
 
-	if (!ops || !ops->transfer)
-		ret = -EIO;
-	else
-		ret = ops->transfer(dsi->host, &msg);
-
+	ret = mipi_dsi_set_maximum_return_packet_size(dsi, size);
 	if (ret < 0) {
 		dev_err(ctx->dev,
 			"error %d setting maximum return packet size to %d\n",
-- 
2.1.2

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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v4 07/16] drm/dsi: Implement generic read and write commands
  2014-11-03  9:13 [PATCH v4 01/16] drm/dsi: Add message to packet translator Thierry Reding
                   ` (4 preceding siblings ...)
  2014-11-03  9:13 ` [PATCH v4 06/16] drm/panel: s6e8aa0: Use standard MIPI DSI function Thierry Reding
@ 2014-11-03  9:13 ` Thierry Reding
  2014-11-03  9:13 ` [PATCH v4 08/16] drm/dsi: Implement some standard DCS commands Thierry Reding
                   ` (9 subsequent siblings)
  15 siblings, 0 replies; 28+ messages in thread
From: Thierry Reding @ 2014-11-03  9:13 UTC (permalink / raw)
  To: Thierry Reding; +Cc: Andrzej Hajda, dri-devel

From: Thierry Reding <treding@nvidia.com>

Implement generic read and write commands. Selection of the proper data
type for packets is done automatically based on the number of parameters
or payload length.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v4:
- do not handle protocol-level details in generic read/write

Changes in v3:
- use common helper to simplify code

 drivers/gpu/drm/drm_mipi_dsi.c | 89 ++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_mipi_dsi.h     |  6 +++
 2 files changed, 95 insertions(+)

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index 640cabcd6c1f..aee7962401fd 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -282,6 +282,95 @@ int mipi_dsi_set_maximum_return_packet_size(struct mipi_dsi_device *dsi,
 EXPORT_SYMBOL(mipi_dsi_set_maximum_return_packet_size);
 
 /**
+ * mipi_dsi_generic_write() - transmit data using a generic write packet
+ * @dsi: DSI peripheral device
+ * @payload: buffer containing the payload
+ * @size: size of payload buffer
+ *
+ * This function will automatically choose the right data type depending on
+ * the payload length.
+ *
+ * Return: The number of bytes transmitted on success or a negative error code
+ * on failure.
+ */
+ssize_t mipi_dsi_generic_write(struct mipi_dsi_device *dsi, const void *payload,
+			       size_t size)
+{
+	struct mipi_dsi_msg msg = {
+		.channel = dsi->channel,
+		.tx_buf = payload,
+		.tx_len = size
+	};
+
+	switch (size) {
+	case 0:
+		msg.type = MIPI_DSI_GENERIC_SHORT_WRITE_0_PARAM;
+		break;
+
+	case 1:
+		msg.type = MIPI_DSI_GENERIC_SHORT_WRITE_1_PARAM;
+		break;
+
+	case 2:
+		msg.type = MIPI_DSI_GENERIC_SHORT_WRITE_2_PARAM;
+		break;
+
+	default:
+		msg.type = MIPI_DSI_GENERIC_LONG_WRITE;
+		break;
+	}
+
+	return mipi_dsi_device_transfer(dsi, &msg);
+}
+EXPORT_SYMBOL(mipi_dsi_generic_write);
+
+/**
+ * mipi_dsi_generic_read() - receive data using a generic read packet
+ * @dsi: DSI peripheral device
+ * @params: buffer containing the request parameters
+ * @num_params: number of request parameters
+ * @data: buffer in which to return the received data
+ * @size: size of receive buffer
+ *
+ * This function will automatically choose the right data type depending on
+ * the number of parameters passed in.
+ *
+ * Return: The number of bytes successfully read or a negative error code on
+ * failure.
+ */
+ssize_t mipi_dsi_generic_read(struct mipi_dsi_device *dsi, const void *params,
+			      size_t num_params, void *data, size_t size)
+{
+	struct mipi_dsi_msg msg = {
+		.channel = dsi->channel,
+		.tx_len = num_params,
+		.tx_buf = params,
+		.rx_len = size,
+		.rx_buf = data
+	};
+
+	switch (num_params) {
+	case 0:
+		msg.type = MIPI_DSI_GENERIC_READ_REQUEST_0_PARAM;
+		break;
+
+	case 1:
+		msg.type = MIPI_DSI_GENERIC_READ_REQUEST_1_PARAM;
+		break;
+
+	case 2:
+		msg.type = MIPI_DSI_GENERIC_READ_REQUEST_2_PARAM;
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	return mipi_dsi_device_transfer(dsi, &msg);
+}
+EXPORT_SYMBOL(mipi_dsi_generic_read);
+
+/**
  * mipi_dsi_dcs_write_buffer() - transmit a DCS command with payload
  * @dsi: DSI peripheral device
  * @data: buffer containing data to be transmitted
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index bab67099872b..7b7bf895e1b2 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -152,6 +152,12 @@ int mipi_dsi_attach(struct mipi_dsi_device *dsi);
 int mipi_dsi_detach(struct mipi_dsi_device *dsi);
 int mipi_dsi_set_maximum_return_packet_size(struct mipi_dsi_device *dsi,
 					    u16 value);
+
+ssize_t mipi_dsi_generic_write(struct mipi_dsi_device *dsi, const void *payload,
+			       size_t size);
+ssize_t mipi_dsi_generic_read(struct mipi_dsi_device *dsi, const void *params,
+			      size_t num_params, void *data, size_t size);
+
 ssize_t mipi_dsi_dcs_write_buffer(struct mipi_dsi_device *dsi,
 				  const void *data, size_t len);
 ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, u8 cmd,
-- 
2.1.2

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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v4 08/16] drm/dsi: Implement some standard DCS commands
  2014-11-03  9:13 [PATCH v4 01/16] drm/dsi: Add message to packet translator Thierry Reding
                   ` (5 preceding siblings ...)
  2014-11-03  9:13 ` [PATCH v4 07/16] drm/dsi: Implement generic read and write commands Thierry Reding
@ 2014-11-03  9:13 ` Thierry Reding
  2014-11-03  9:13 ` [PATCH v4 09/16] drm/dsi: Add to DocBook documentation Thierry Reding
                   ` (8 subsequent siblings)
  15 siblings, 0 replies; 28+ messages in thread
From: Thierry Reding @ 2014-11-03  9:13 UTC (permalink / raw)
  To: Thierry Reding; +Cc: Andrzej Hajda, dri-devel

From: YoungJun Cho <yj44.cho@samsung.com>

Add helpers for the {enter,exit}_sleep_mode, set_display_{on,off} and
set_tear_{on,off} DCS commands.

Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
[treding: kerneldoc and other minor cleanup]
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_mipi_dsi.c | 118 +++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_mipi_dsi.h     |  19 +++++++
 2 files changed, 137 insertions(+)

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index aee7962401fd..7a7217ee03af 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -481,6 +481,124 @@ ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, u8 cmd, void *data,
 }
 EXPORT_SYMBOL(mipi_dsi_dcs_read);
 
+/**
+ * mipi_dsi_dcs_enter_sleep_mode() - disable all unnecessary blocks inside the
+ *    display module except interface communication
+ * @dsi: DSI peripheral device
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
+int mipi_dsi_dcs_enter_sleep_mode(struct mipi_dsi_device *dsi)
+{
+	ssize_t err;
+
+	err = mipi_dsi_dcs_write(dsi, MIPI_DCS_ENTER_SLEEP_MODE, NULL, 0);
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+EXPORT_SYMBOL(mipi_dsi_dcs_enter_sleep_mode);
+
+/**
+ * mipi_dsi_dcs_exit_sleep_mode() - enable all blocks inside the display
+ *    module
+ * @dsi: DSI peripheral device
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
+int mipi_dsi_dcs_exit_sleep_mode(struct mipi_dsi_device *dsi)
+{
+	ssize_t err;
+
+	err = mipi_dsi_dcs_write(dsi, MIPI_DCS_EXIT_SLEEP_MODE, NULL, 0);
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+EXPORT_SYMBOL(mipi_dsi_dcs_exit_sleep_mode);
+
+/**
+ * mipi_dsi_dcs_set_display_off() - stop displaying the image data on the
+ *    display device
+ * @dsi: DSI peripheral device
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
+int mipi_dsi_dcs_set_display_off(struct mipi_dsi_device *dsi)
+{
+	ssize_t err;
+
+	err = mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_DISPLAY_OFF, NULL, 0);
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+EXPORT_SYMBOL(mipi_dsi_dcs_set_display_off);
+
+/**
+ * mipi_dsi_dcs_set_display_on() - start displaying the image data on the
+ *    display device
+ * @dsi: DSI peripheral device
+ *
+ * Return: 0 on success or a negative error code on failure
+ */
+int mipi_dsi_dcs_set_display_on(struct mipi_dsi_device *dsi)
+{
+	ssize_t err;
+
+	err = mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_DISPLAY_ON, NULL, 0);
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+EXPORT_SYMBOL(mipi_dsi_dcs_set_display_on);
+
+/**
+ * mipi_dsi_dcs_set_tear_off() - turn off the display module's Tearing Effect
+ *    output signal on the TE signal line
+ * @dsi: DSI peripheral device
+ *
+ * Return: 0 on success or a negative error code on failure
+ */
+int mipi_dsi_dcs_set_tear_off(struct mipi_dsi_device *dsi)
+{
+	ssize_t err;
+
+	err = mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_TEAR_OFF, NULL, 0);
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+EXPORT_SYMBOL(mipi_dsi_dcs_set_tear_off);
+
+/**
+ * mipi_dsi_dcs_set_tear_on() - turn on the display module's Tearing Effect
+ *    output signal on the TE signal line.
+ * @dsi: DSI peripheral device
+ * @mode: the Tearing Effect Output Line mode
+ *
+ * Return: 0 on success or a negative error code on failure
+ */
+int mipi_dsi_dcs_set_tear_on(struct mipi_dsi_device *dsi,
+			     enum mipi_dsi_dcs_tear_mode mode)
+{
+	u8 value = mode;
+	ssize_t err;
+
+	err = mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_TEAR_ON, &value,
+				 sizeof(value));
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+EXPORT_SYMBOL(mipi_dsi_dcs_set_tear_on);
+
 static int mipi_dsi_drv_probe(struct device *dev)
 {
 	struct mipi_dsi_driver *drv = to_mipi_dsi_driver(dev->driver);
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index 7b7bf895e1b2..d84429111ac4 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -158,12 +158,31 @@ ssize_t mipi_dsi_generic_write(struct mipi_dsi_device *dsi, const void *payload,
 ssize_t mipi_dsi_generic_read(struct mipi_dsi_device *dsi, const void *params,
 			      size_t num_params, void *data, size_t size);
 
+/**
+ * enum mipi_dsi_dcs_tear_mode - Tearing Effect Output Line mode
+ * @MIPI_DSI_DCS_TEAR_MODE_VBLANK: the TE output line consists of V-Blanking
+ *    information only
+ * @MIPI_DSI_DCS_TEAR_MODE_VHBLANK : the TE output line consists of both
+ *    V-Blanking and H-Blanking information
+ */
+enum mipi_dsi_dcs_tear_mode {
+	MIPI_DSI_DCS_TEAR_MODE_VBLANK,
+	MIPI_DSI_DCS_TEAR_MODE_VHBLANK,
+};
+
 ssize_t mipi_dsi_dcs_write_buffer(struct mipi_dsi_device *dsi,
 				  const void *data, size_t len);
 ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, u8 cmd,
 			   const void *data, size_t len);
 ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, u8 cmd, void *data,
 			  size_t len);
+int mipi_dsi_dcs_enter_sleep_mode(struct mipi_dsi_device *dsi);
+int mipi_dsi_dcs_exit_sleep_mode(struct mipi_dsi_device *dsi);
+int mipi_dsi_dcs_set_display_off(struct mipi_dsi_device *dsi);
+int mipi_dsi_dcs_set_display_on(struct mipi_dsi_device *dsi);
+int mipi_dsi_dcs_set_tear_off(struct mipi_dsi_device *dsi);
+int mipi_dsi_dcs_set_tear_on(struct mipi_dsi_device *dsi,
+			     enum mipi_dsi_dcs_tear_mode mode);
 
 /**
  * struct mipi_dsi_driver - DSI driver
-- 
2.1.2

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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v4 09/16] drm/dsi: Add to DocBook documentation
  2014-11-03  9:13 [PATCH v4 01/16] drm/dsi: Add message to packet translator Thierry Reding
                   ` (6 preceding siblings ...)
  2014-11-03  9:13 ` [PATCH v4 08/16] drm/dsi: Implement some standard DCS commands Thierry Reding
@ 2014-11-03  9:13 ` Thierry Reding
  2014-11-03  9:13 ` [PATCH v4 10/16] drm/dsi: Implement DCS nop command Thierry Reding
                   ` (7 subsequent siblings)
  15 siblings, 0 replies; 28+ messages in thread
From: Thierry Reding @ 2014-11-03  9:13 UTC (permalink / raw)
  To: Thierry Reding; +Cc: Andrzej Hajda, dri-devel

From: Thierry Reding <treding@nvidia.com>

Integrate the MIPI DSI helpers into DocBook and clean up various
kerneldoc warnings. Also add a brief DOC section and clarify some
aspects of the mipi_dsi_host struct's .transfer() operation.

Acked-by: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 Documentation/DocBook/drm.tmpl |  6 ++++++
 drivers/gpu/drm/drm_mipi_dsi.c | 18 ++++++++++++++++--
 include/drm/drm_mipi_dsi.h     | 16 ++++++++++++++--
 3 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index f6a9d7b21380..d89ca5830697 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -2343,6 +2343,12 @@ void intel_crt_init(struct drm_device *dev)
 !Edrivers/gpu/drm/drm_dp_mst_topology.c
     </sect2>
     <sect2>
+      <title>MIPI DSI Helper Functions Reference</title>
+!Pdrivers/gpu/drm/drm_mipi_dsi.c dsi helpers
+!Iinclude/drm/drm_mipi_dsi.h
+!Edrivers/gpu/drm/drm_mipi_dsi.c
+    </sect2>
+    <sect2>
       <title>EDID Helper Functions Reference</title>
 !Edrivers/gpu/drm/drm_edid.c
     </sect2>
diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index 7a7217ee03af..b4e10d3c81ce 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -35,6 +35,16 @@
 
 #include <video/mipi_display.h>
 
+/**
+ * DOC: dsi helpers
+ *
+ * These functions contain some common logic and helpers to deal with MIPI DSI
+ * peripherals.
+ *
+ * Helpers are provided for a number of standard MIPI DSI command as well as a
+ * subset of the MIPI DCS command set.
+ */
+
 static int mipi_dsi_device_match(struct device *dev, struct device_driver *drv)
 {
 	return of_driver_match_device(dev, drv);
@@ -624,8 +634,10 @@ static void mipi_dsi_drv_shutdown(struct device *dev)
 }
 
 /**
- * mipi_dsi_driver_register - register a driver for DSI devices
+ * mipi_dsi_driver_register() - register a driver for DSI devices
  * @drv: DSI driver structure
+ *
+ * Return: 0 on success or a negative error code on failure.
  */
 int mipi_dsi_driver_register(struct mipi_dsi_driver *drv)
 {
@@ -642,8 +654,10 @@ int mipi_dsi_driver_register(struct mipi_dsi_driver *drv)
 EXPORT_SYMBOL(mipi_dsi_driver_register);
 
 /**
- * mipi_dsi_driver_unregister - unregister a driver for DSI devices
+ * mipi_dsi_driver_unregister() - unregister a driver for DSI devices
  * @drv: DSI driver structure
+ *
+ * Return: 0 on success or a negative error code on failure.
  */
 void mipi_dsi_driver_unregister(struct mipi_dsi_driver *drv)
 {
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index d84429111ac4..359287d4ca92 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -26,6 +26,7 @@ struct mipi_dsi_device;
  * struct mipi_dsi_msg - read/write DSI buffer
  * @channel: virtual channel id
  * @type: payload data type
+ * @flags: flags controlling this message transmission
  * @tx_len: length of @tx_buf
  * @tx_buf: data to be written
  * @rx_len: length of @rx_buf
@@ -65,8 +66,19 @@ int mipi_dsi_create_packet(struct mipi_dsi_packet *packet,
  * struct mipi_dsi_host_ops - DSI bus operations
  * @attach: attach DSI device to DSI host
  * @detach: detach DSI device from DSI host
- * @transfer: send and/or receive DSI packet, return number of received bytes,
- * 	      or error
+ * @transfer: transmit a DSI packet
+ *
+ * DSI packets transmitted by .transfer() are passed in as mipi_dsi_msg
+ * structures. This structure contains information about the type of packet
+ * being transmitted as well as the transmit and receive buffers. When an
+ * error is encountered during transmission, this function will return a
+ * negative error code. On success it shall return the number of bytes
+ * transmitted for write packets or the number of bytes received for read
+ * packets.
+ *
+ * Note that typically DSI packet transmission is atomic, so the .transfer()
+ * function will seldomly return anything other than the number of bytes
+ * contained in the transmit buffer on success.
  */
 struct mipi_dsi_host_ops {
 	int (*attach)(struct mipi_dsi_host *host,
-- 
2.1.2

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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v4 10/16] drm/dsi: Implement DCS nop command
  2014-11-03  9:13 [PATCH v4 01/16] drm/dsi: Add message to packet translator Thierry Reding
                   ` (7 preceding siblings ...)
  2014-11-03  9:13 ` [PATCH v4 09/16] drm/dsi: Add to DocBook documentation Thierry Reding
@ 2014-11-03  9:13 ` Thierry Reding
  2014-11-03  9:13 ` [PATCH v4 11/16] drm/dsi: Implement DCS soft_reset command Thierry Reding
                   ` (6 subsequent siblings)
  15 siblings, 0 replies; 28+ messages in thread
From: Thierry Reding @ 2014-11-03  9:13 UTC (permalink / raw)
  To: Thierry Reding; +Cc: Andrzej Hajda, dri-devel

From: Thierry Reding <treding@nvidia.com>

Provide a small convenience wrapper that transmits a DCS nop command.

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

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index b4e10d3c81ce..f1617fe5469e 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -492,6 +492,24 @@ ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, u8 cmd, void *data,
 EXPORT_SYMBOL(mipi_dsi_dcs_read);
 
 /**
+ * mipi_dsi_dcs_nop() - send DCS nop packet
+ * @dsi: DSI peripheral device
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
+int mipi_dsi_dcs_nop(struct mipi_dsi_device *dsi)
+{
+	ssize_t err;
+
+	err = mipi_dsi_dcs_write(dsi, MIPI_DCS_NOP, NULL, 0);
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+EXPORT_SYMBOL(mipi_dsi_dcs_nop);
+
+/**
  * mipi_dsi_dcs_enter_sleep_mode() - disable all unnecessary blocks inside the
  *    display module except interface communication
  * @dsi: DSI peripheral device
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index 359287d4ca92..23b4ac575aa5 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -188,6 +188,7 @@ ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, u8 cmd,
 			   const void *data, size_t len);
 ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, u8 cmd, void *data,
 			  size_t len);
+int mipi_dsi_dcs_nop(struct mipi_dsi_device *dsi);
 int mipi_dsi_dcs_enter_sleep_mode(struct mipi_dsi_device *dsi);
 int mipi_dsi_dcs_exit_sleep_mode(struct mipi_dsi_device *dsi);
 int mipi_dsi_dcs_set_display_off(struct mipi_dsi_device *dsi);
-- 
2.1.2

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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v4 11/16] drm/dsi: Implement DCS soft_reset command
  2014-11-03  9:13 [PATCH v4 01/16] drm/dsi: Add message to packet translator Thierry Reding
                   ` (8 preceding siblings ...)
  2014-11-03  9:13 ` [PATCH v4 10/16] drm/dsi: Implement DCS nop command Thierry Reding
@ 2014-11-03  9:13 ` Thierry Reding
  2014-11-03  9:13 ` [PATCH v4 12/16] drm/dsi: Implement DCS get_power_mode command Thierry Reding
                   ` (5 subsequent siblings)
  15 siblings, 0 replies; 28+ messages in thread
From: Thierry Reding @ 2014-11-03  9:13 UTC (permalink / raw)
  To: Thierry Reding; +Cc: Andrzej Hajda, dri-devel

From: Thierry Reding <treding@nvidia.com>

Provide a small convenience wrapper that transmits a DCS soft_reset
command.

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

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index f1617fe5469e..acfaaa37b905 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -510,6 +510,24 @@ int mipi_dsi_dcs_nop(struct mipi_dsi_device *dsi)
 EXPORT_SYMBOL(mipi_dsi_dcs_nop);
 
 /**
+ * mipi_dsi_dcs_soft_reset() - perform a software reset of the display module
+ * @dsi: DSI peripheral device
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
+int mipi_dsi_dcs_soft_reset(struct mipi_dsi_device *dsi)
+{
+	ssize_t err;
+
+	err = mipi_dsi_dcs_write(dsi, MIPI_DCS_SOFT_RESET, NULL, 0);
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+EXPORT_SYMBOL(mipi_dsi_dcs_soft_reset);
+
+/**
  * mipi_dsi_dcs_enter_sleep_mode() - disable all unnecessary blocks inside the
  *    display module except interface communication
  * @dsi: DSI peripheral device
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index 23b4ac575aa5..c72838679136 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -189,6 +189,7 @@ ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, u8 cmd,
 ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, u8 cmd, void *data,
 			  size_t len);
 int mipi_dsi_dcs_nop(struct mipi_dsi_device *dsi);
+int mipi_dsi_dcs_soft_reset(struct mipi_dsi_device *dsi);
 int mipi_dsi_dcs_enter_sleep_mode(struct mipi_dsi_device *dsi);
 int mipi_dsi_dcs_exit_sleep_mode(struct mipi_dsi_device *dsi);
 int mipi_dsi_dcs_set_display_off(struct mipi_dsi_device *dsi);
-- 
2.1.2

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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v4 12/16] drm/dsi: Implement DCS get_power_mode command
  2014-11-03  9:13 [PATCH v4 01/16] drm/dsi: Add message to packet translator Thierry Reding
                   ` (9 preceding siblings ...)
  2014-11-03  9:13 ` [PATCH v4 11/16] drm/dsi: Implement DCS soft_reset command Thierry Reding
@ 2014-11-03  9:13 ` Thierry Reding
  2014-11-03  9:13 ` [PATCH v4 13/16] drm/dsi: Implement DCS {get, set}_pixel_format commands Thierry Reding
                   ` (4 subsequent siblings)
  15 siblings, 0 replies; 28+ messages in thread
From: Thierry Reding @ 2014-11-03  9:13 UTC (permalink / raw)
  To: Thierry Reding; +Cc: Andrzej Hajda, dri-devel

From: Thierry Reding <treding@nvidia.com>

Provide a small convenience wrapper that transmits a DCS get_power_mode
command. A set of bitmasks for the mode bits is also provided.

Acked-by: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v4:
- return -ENODATA when no data could be read

 drivers/gpu/drm/drm_mipi_dsi.c | 25 +++++++++++++++++++++++++
 include/drm/drm_mipi_dsi.h     |  7 +++++++
 2 files changed, 32 insertions(+)

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index acfaaa37b905..da34469b9b98 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -528,6 +528,31 @@ int mipi_dsi_dcs_soft_reset(struct mipi_dsi_device *dsi)
 EXPORT_SYMBOL(mipi_dsi_dcs_soft_reset);
 
 /**
+ * mipi_dsi_dcs_get_power_mode() - query the display module's current power
+ *    mode
+ * @dsi: DSI peripheral device
+ * @mode: return location for the current power mode
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
+int mipi_dsi_dcs_get_power_mode(struct mipi_dsi_device *dsi, u8 *mode)
+{
+	ssize_t err;
+
+	err = mipi_dsi_dcs_read(dsi, MIPI_DCS_GET_POWER_MODE, mode,
+				sizeof(*mode));
+	if (err <= 0) {
+		if (err == 0)
+			err = -ENODATA;
+
+		return err;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(mipi_dsi_dcs_get_power_mode);
+
+/**
  * mipi_dsi_dcs_enter_sleep_mode() - disable all unnecessary blocks inside the
  *    display module except interface communication
  * @dsi: DSI peripheral device
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index c72838679136..4cbf8e658a3a 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -182,6 +182,12 @@ enum mipi_dsi_dcs_tear_mode {
 	MIPI_DSI_DCS_TEAR_MODE_VHBLANK,
 };
 
+#define MIPI_DSI_DCS_POWER_MODE_DISPLAY (1 << 2)
+#define MIPI_DSI_DCS_POWER_MODE_NORMAL  (1 << 3)
+#define MIPI_DSI_DCS_POWER_MODE_SLEEP   (1 << 4)
+#define MIPI_DSI_DCS_POWER_MODE_PARTIAL (1 << 5)
+#define MIPI_DSI_DCS_POWER_MODE_IDLE    (1 << 6)
+
 ssize_t mipi_dsi_dcs_write_buffer(struct mipi_dsi_device *dsi,
 				  const void *data, size_t len);
 ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, u8 cmd,
@@ -190,6 +196,7 @@ ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, u8 cmd, void *data,
 			  size_t len);
 int mipi_dsi_dcs_nop(struct mipi_dsi_device *dsi);
 int mipi_dsi_dcs_soft_reset(struct mipi_dsi_device *dsi);
+int mipi_dsi_dcs_get_power_mode(struct mipi_dsi_device *dsi, u8 *mode);
 int mipi_dsi_dcs_enter_sleep_mode(struct mipi_dsi_device *dsi);
 int mipi_dsi_dcs_exit_sleep_mode(struct mipi_dsi_device *dsi);
 int mipi_dsi_dcs_set_display_off(struct mipi_dsi_device *dsi);
-- 
2.1.2

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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v4 13/16] drm/dsi: Implement DCS {get, set}_pixel_format commands
  2014-11-03  9:13 [PATCH v4 01/16] drm/dsi: Add message to packet translator Thierry Reding
                   ` (10 preceding siblings ...)
  2014-11-03  9:13 ` [PATCH v4 12/16] drm/dsi: Implement DCS get_power_mode command Thierry Reding
@ 2014-11-03  9:13 ` Thierry Reding
  2014-11-03 17:10   ` Sean Paul
  2014-11-03  9:13 ` [PATCH v4 14/16] drm/dsi: Implement DCS set_{column, page}_address commands Thierry Reding
                   ` (3 subsequent siblings)
  15 siblings, 1 reply; 28+ messages in thread
From: Thierry Reding @ 2014-11-03  9:13 UTC (permalink / raw)
  To: Thierry Reding; +Cc: Andrzej Hajda, dri-devel

From: Thierry Reding <treding@nvidia.com>

Provide small convenience wrappers to query or set the pixel format used
by the interface.

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

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index da34469b9b98..226822a44457 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -553,6 +553,27 @@ int mipi_dsi_dcs_get_power_mode(struct mipi_dsi_device *dsi, u8 *mode)
 EXPORT_SYMBOL(mipi_dsi_dcs_get_power_mode);
 
 /**
+ * mipi_dsi_dcs_get_pixel_format() - gets the pixel format for the RGB image
+ *    data used by the interface
+ * @dsi: DSI peripheral device
+ * @format: return location for the pixel format
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
+int mipi_dsi_dcs_get_pixel_format(struct mipi_dsi_device *dsi, u8 *format)
+{
+	ssize_t err;
+
+	err = mipi_dsi_dcs_read(dsi, MIPI_DCS_GET_PIXEL_FORMAT, format,
+				sizeof(*format));
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+EXPORT_SYMBOL(mipi_dsi_dcs_get_pixel_format);
+
+/**
  * mipi_dsi_dcs_enter_sleep_mode() - disable all unnecessary blocks inside the
  *    display module except interface communication
  * @dsi: DSI peripheral device
@@ -670,6 +691,27 @@ int mipi_dsi_dcs_set_tear_on(struct mipi_dsi_device *dsi,
 }
 EXPORT_SYMBOL(mipi_dsi_dcs_set_tear_on);
 
+/**
+ * mipi_dsi_dcs_set_pixel_format() - sets the pixel format for the RGB image
+ *    data used by the interface
+ * @dsi: DSI peripheral device
+ * @format: pixel format
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
+int mipi_dsi_dcs_set_pixel_format(struct mipi_dsi_device *dsi, u8 format)
+{
+	ssize_t err;
+
+	err = mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_PIXEL_FORMAT, &format,
+				 sizeof(format));
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+EXPORT_SYMBOL(mipi_dsi_dcs_set_pixel_format);
+
 static int mipi_dsi_drv_probe(struct device *dev)
 {
 	struct mipi_dsi_driver *drv = to_mipi_dsi_driver(dev->driver);
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index 4cbf8e658a3a..415d01f90086 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -197,6 +197,7 @@ ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, u8 cmd, void *data,
 int mipi_dsi_dcs_nop(struct mipi_dsi_device *dsi);
 int mipi_dsi_dcs_soft_reset(struct mipi_dsi_device *dsi);
 int mipi_dsi_dcs_get_power_mode(struct mipi_dsi_device *dsi, u8 *mode);
+int mipi_dsi_dcs_get_pixel_format(struct mipi_dsi_device *dsi, u8 *format);
 int mipi_dsi_dcs_enter_sleep_mode(struct mipi_dsi_device *dsi);
 int mipi_dsi_dcs_exit_sleep_mode(struct mipi_dsi_device *dsi);
 int mipi_dsi_dcs_set_display_off(struct mipi_dsi_device *dsi);
@@ -204,6 +205,7 @@ int mipi_dsi_dcs_set_display_on(struct mipi_dsi_device *dsi);
 int mipi_dsi_dcs_set_tear_off(struct mipi_dsi_device *dsi);
 int mipi_dsi_dcs_set_tear_on(struct mipi_dsi_device *dsi,
 			     enum mipi_dsi_dcs_tear_mode mode);
+int mipi_dsi_dcs_set_pixel_format(struct mipi_dsi_device *dsi, u8 format);
 
 /**
  * struct mipi_dsi_driver - DSI driver
-- 
2.1.2

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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v4 14/16] drm/dsi: Implement DCS set_{column, page}_address commands
  2014-11-03  9:13 [PATCH v4 01/16] drm/dsi: Add message to packet translator Thierry Reding
                   ` (11 preceding siblings ...)
  2014-11-03  9:13 ` [PATCH v4 13/16] drm/dsi: Implement DCS {get, set}_pixel_format commands Thierry Reding
@ 2014-11-03  9:13 ` Thierry Reding
  2014-11-03  9:13 ` [PATCH v4 15/16] drm/dsi: Resolve MIPI DSI device from phandle Thierry Reding
                   ` (2 subsequent siblings)
  15 siblings, 0 replies; 28+ messages in thread
From: Thierry Reding @ 2014-11-03  9:13 UTC (permalink / raw)
  To: Thierry Reding; +Cc: Andrzej Hajda, dri-devel

From: Thierry Reding <treding@nvidia.com>

Provide small convenience wrappers to set the column and page extents of
the frame memory accessed by the host processors.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_mipi_dsi.c | 46 ++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_mipi_dsi.h     |  4 ++++
 2 files changed, 50 insertions(+)

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index 226822a44457..97dcbaa6a301 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -650,6 +650,52 @@ int mipi_dsi_dcs_set_display_on(struct mipi_dsi_device *dsi)
 EXPORT_SYMBOL(mipi_dsi_dcs_set_display_on);
 
 /**
+ * mipi_dsi_dcs_set_column_address() - define the column extent of the frame
+ *    memory accessed by the host processor
+ * @dsi: DSI peripheral device
+ * @start: first column of frame memory
+ * @end: last column of frame memory
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
+int mipi_dsi_dcs_set_column_address(struct mipi_dsi_device *dsi, u16 start,
+				    u16 end)
+{
+	u8 payload[4] = { start >> 8, start & 0xff, end >> 8, end & 0xff };
+	ssize_t err;
+
+	err = mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_COLUMN_ADDRESS, payload,
+				 sizeof(payload));
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+
+/**
+ * mipi_dsi_dcs_set_page_address() - define the page extent of the frame
+ *    memory accessed by the host processor
+ * @dsi: DSI peripheral device
+ * @start: first page of frame memory
+ * @end: last page of frame memory
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
+int mipi_dsi_dcs_set_page_address(struct mipi_dsi_device *dsi, u16 start,
+				  u16 end)
+{
+	u8 payload[4] = { start >> 8, start & 0xff, end >> 8, end & 0xff };
+	ssize_t err;
+
+	err = mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_PAGE_ADDRESS, payload,
+				 sizeof(payload));
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+
+/**
  * mipi_dsi_dcs_set_tear_off() - turn off the display module's Tearing Effect
  *    output signal on the TE signal line
  * @dsi: DSI peripheral device
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index 415d01f90086..6031593ee231 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -202,6 +202,10 @@ int mipi_dsi_dcs_enter_sleep_mode(struct mipi_dsi_device *dsi);
 int mipi_dsi_dcs_exit_sleep_mode(struct mipi_dsi_device *dsi);
 int mipi_dsi_dcs_set_display_off(struct mipi_dsi_device *dsi);
 int mipi_dsi_dcs_set_display_on(struct mipi_dsi_device *dsi);
+int mipi_dsi_dcs_set_column_address(struct mipi_dsi_device *dsi, u16 start,
+				    u16 end);
+int mipi_dsi_dcs_set_page_address(struct mipi_dsi_device *dsi, u16 start,
+				  u16 end);
 int mipi_dsi_dcs_set_tear_off(struct mipi_dsi_device *dsi);
 int mipi_dsi_dcs_set_tear_on(struct mipi_dsi_device *dsi,
 			     enum mipi_dsi_dcs_tear_mode mode);
-- 
2.1.2

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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v4 15/16] drm/dsi: Resolve MIPI DSI device from phandle
  2014-11-03  9:13 [PATCH v4 01/16] drm/dsi: Add message to packet translator Thierry Reding
                   ` (12 preceding siblings ...)
  2014-11-03  9:13 ` [PATCH v4 14/16] drm/dsi: Implement DCS set_{column, page}_address commands Thierry Reding
@ 2014-11-03  9:13 ` Thierry Reding
  2014-11-03  9:13 ` [PATCH v4 16/16] drm/panel: Add Sharp LQ101R1SX01 support Thierry Reding
  2014-11-04 11:43 ` [PATCH v4 01/16] drm/dsi: Add message to packet translator Andrzej Hajda
  15 siblings, 0 replies; 28+ messages in thread
From: Thierry Reding @ 2014-11-03  9:13 UTC (permalink / raw)
  To: Thierry Reding; +Cc: Andrzej Hajda, dri-devel

From: Thierry Reding <treding@nvidia.com>

Add a function, of_find_mipi_dsi_device_by_node(), that can be used to
resolve a phandle to a MIPI DSI device.

Acked-by: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_mipi_dsi.c | 23 +++++++++++++++++++++++
 include/drm/drm_mipi_dsi.h     |  1 +
 2 files changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index 97dcbaa6a301..afaa36c37fcb 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -67,6 +67,29 @@ static struct bus_type mipi_dsi_bus_type = {
 	.pm = &mipi_dsi_device_pm_ops,
 };
 
+static int of_device_match(struct device *dev, void *data)
+{
+	return dev->of_node == data;
+}
+
+/**
+ * of_find_mipi_dsi_device_by_node() - find the MIPI DSI device matching a
+ *    device tree node
+ * @np: device tree node
+ *
+ * Return: A pointer to the MIPI DSI device corresponding to @np or NULL if no
+ *    such device exists (or has not been registered yet).
+ */
+struct mipi_dsi_device *of_find_mipi_dsi_device_by_node(struct device_node *np)
+{
+	struct device *dev;
+
+	dev = bus_find_device(&mipi_dsi_bus_type, NULL, np, of_device_match);
+
+	return dev ? to_mipi_dsi_device(dev) : NULL;
+}
+EXPORT_SYMBOL(of_find_mipi_dsi_device_by_node);
+
 static void mipi_dsi_dev_release(struct device *dev)
 {
 	struct mipi_dsi_device *dsi = to_mipi_dsi_device(dev);
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index 6031593ee231..16c2e9890631 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -160,6 +160,7 @@ static inline struct mipi_dsi_device *to_mipi_dsi_device(struct device *dev)
 	return container_of(dev, struct mipi_dsi_device, dev);
 }
 
+struct mipi_dsi_device *of_find_mipi_dsi_device_by_node(struct device_node *np);
 int mipi_dsi_attach(struct mipi_dsi_device *dsi);
 int mipi_dsi_detach(struct mipi_dsi_device *dsi);
 int mipi_dsi_set_maximum_return_packet_size(struct mipi_dsi_device *dsi,
-- 
2.1.2

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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v4 16/16] drm/panel: Add Sharp LQ101R1SX01 support
  2014-11-03  9:13 [PATCH v4 01/16] drm/dsi: Add message to packet translator Thierry Reding
                   ` (13 preceding siblings ...)
  2014-11-03  9:13 ` [PATCH v4 15/16] drm/dsi: Resolve MIPI DSI device from phandle Thierry Reding
@ 2014-11-03  9:13 ` Thierry Reding
  2014-11-03 17:28   ` Sean Paul
  2014-11-04 10:41   ` Andrzej Hajda
  2014-11-04 11:43 ` [PATCH v4 01/16] drm/dsi: Add message to packet translator Andrzej Hajda
  15 siblings, 2 replies; 28+ messages in thread
From: Thierry Reding @ 2014-11-03  9:13 UTC (permalink / raw)
  To: Thierry Reding; +Cc: Andrzej Hajda, dri-devel

From: Thierry Reding <treding@nvidia.com>

This panel requires dual-channel mode. The device accepts command-mode
data on 8 lanes and will therefore need a dual-channel DSI controller.
The two interfaces that make up this device need to be instantiated in
the controllers that gang up to provide the dual-channel DSI host.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v4:
- use low power mode since highspeed message transfers don't work
- clarify required and optional properties for both DSI links
- power off panel when .prepare() fails
- properly drop reference to DSI-LINK2
- don't allocate memory for DSI-LINK2
- propagate errors on failure

 .../bindings/panel/sharp,lq101r1sx01.txt           |  49 +++
 drivers/gpu/drm/panel/Kconfig                      |  13 +
 drivers/gpu/drm/panel/Makefile                     |   1 +
 drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c    | 464 +++++++++++++++++++++
 4 files changed, 527 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt
 create mode 100644 drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c

diff --git a/Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt b/Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt
new file mode 100644
index 000000000000..f522bb8e47e1
--- /dev/null
+++ b/Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt
@@ -0,0 +1,49 @@
+Sharp Microelectronics 10.1" WQXGA TFT LCD panel
+
+This panel requires a dual-channel DSI host to operate. It supports two modes:
+- left-right: each channel drives the left or right half of the screen
+- even-odd: each channel drives the even or odd lines of the screen
+
+Each of the DSI channels controls a separate DSI peripheral. The peripheral
+driven by the first link (DSI-LINK1), left or even, is considered the primary
+peripheral and controls the device. The 'link2' property contains a phandle
+to the peripheral driven by the second link (DSI-LINK2, right or odd).
+
+Note that in video mode the DSI-LINK1 interface always provides the left/even
+pixels and DSI-LINK2 always provides the right/odd pixels. In command mode it
+is possible to program either link to drive the left/even or right/odd pixels
+but for the sake of consistency this binding assumes that the same assignment
+is chosen as for video mode.
+
+Required properties:
+- compatible: should be "sharp,lq101r1sx01"
+- reg: DSI virtual channel of the peripheral
+
+Required properties (for DSI-LINK1 only):
+- link2: phandle to the DSI peripheral on the secondary link. Note that the
+  presence of this property marks the containing node as DSI-LINK1.
+- power-supply: phandle of the regulator that provides the supply voltage
+
+Optional properties (for DSI-LINK1 only):
+- backlight: phandle of the backlight device attached to the panel
+
+Example:
+
+	dsi@54300000 {
+		panel: panel@0 {
+			compatible = "sharp,lq101r1sx01";
+			reg = <0>;
+
+			link2 = <&secondary>;
+
+			power-supply = <...>;
+			backlight = <...>;
+		};
+	};
+
+	dsi@54400000 {
+		secondary: panel@0 {
+			compatible = "sharp,lq101r1sx01";
+			reg = <0>;
+		};
+	};
diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index bee9f72b3a93..024e98ef8e4d 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -27,4 +27,17 @@ config DRM_PANEL_S6E8AA0
 	select DRM_MIPI_DSI
 	select VIDEOMODE_HELPERS
 
+config DRM_PANEL_SHARP_LQ101R1SX01
+	tristate "Sharp LQ101R1SX01 panel"
+	depends on OF
+	depends on DRM_MIPI_DSI
+	help
+	  Say Y here if you want to enable support for Sharp LQ101R1SX01
+	  TFT-LCD modules. The panel has a 2560x1600 resolution and uses
+	  24 bit RGB per pixel. It provides a dual MIPI DSI interface to
+	  the host and has a built-in LED backlight.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called panel-sharp-lq101r1sx01.
+
 endmenu
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index 8b929212fad7..4b2a0430804b 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -1,3 +1,4 @@
 obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.o
 obj-$(CONFIG_DRM_PANEL_LD9040) += panel-ld9040.o
 obj-$(CONFIG_DRM_PANEL_S6E8AA0) += panel-s6e8aa0.o
+obj-$(CONFIG_DRM_PANEL_SHARP_LQ101R1SX01) += panel-sharp-lq101r1sx01.o
diff --git a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
new file mode 100644
index 000000000000..ee0e7f57e4a1
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
@@ -0,0 +1,464 @@
+/*
+ * Copyright (C) 2014 NVIDIA Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/backlight.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regulator/consumer.h>
+
+#include <drm/drmP.h>
+#include <drm/drm_crtc.h>
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_panel.h>
+
+#include <video/mipi_display.h>
+
+#include <linux/host1x.h>
+
+struct sharp_panel {
+	struct drm_panel base;
+	/* the datasheet refers to them as DSI-LINK1 and DSI-LINK2 */
+	struct mipi_dsi_device *link1;
+	struct mipi_dsi_device *link2;
+
+	struct backlight_device *backlight;
+	struct regulator *supply;
+
+	bool prepared;
+	bool enabled;
+
+	const struct drm_display_mode *mode;
+};
+
+static inline struct sharp_panel *to_sharp_panel(struct drm_panel *panel)
+{
+	return container_of(panel, struct sharp_panel, base);
+}
+
+static int sharp_panel_write(struct sharp_panel *sharp, u16 offset, u8 value)
+{
+	u8 payload[3] = { offset >> 8, offset & 0xff, value };
+	struct mipi_dsi_device *dsi = sharp->link1;
+	ssize_t err;
+
+	err = mipi_dsi_generic_write(dsi, payload, sizeof(payload));
+	if (err < 0) {
+		dev_err(&dsi->dev, "failed to write %02x to %04x: %d\n",
+			value, offset, err);
+		return err;
+	}
+
+	err = mipi_dsi_dcs_nop(dsi);
+	if (err < 0) {
+		dev_err(&dsi->dev, "failed to send DCS nop: %d\n", err);
+		return err;
+	}
+
+	usleep_range(10, 20);
+
+	return 0;
+}
+
+static __maybe_unused int sharp_panel_read(struct sharp_panel *sharp,
+					   u16 offset, u8 *value)
+{
+	ssize_t err;
+
+	cpu_to_be16s(&offset);
+
+	err = mipi_dsi_generic_read(sharp->link1, &offset, sizeof(offset),
+				    value, sizeof(*value));
+	if (err < 0)
+		dev_err(&sharp->link1->dev, "failed to read from %04x: %d\n",
+			offset, err);
+
+	return err;
+}
+
+static int sharp_panel_disable(struct drm_panel *panel)
+{
+	struct sharp_panel *sharp = to_sharp_panel(panel);
+
+	if (!sharp->enabled)
+		return 0;
+
+	if (sharp->backlight) {
+		sharp->backlight->props.power = FB_BLANK_POWERDOWN;
+		backlight_update_status(sharp->backlight);
+	}
+
+	sharp->enabled = false;
+
+	return 0;
+}
+
+static int sharp_panel_unprepare(struct drm_panel *panel)
+{
+	struct sharp_panel *sharp = to_sharp_panel(panel);
+	int err;
+
+	if (!sharp->prepared)
+		return 0;
+
+	err = mipi_dsi_dcs_set_display_off(sharp->link1);
+	if (err < 0)
+		dev_err(panel->dev, "failed to set display off: %d\n", err);
+
+	err = mipi_dsi_dcs_enter_sleep_mode(sharp->link1);
+	if (err < 0)
+		dev_err(panel->dev, "failed to enter sleep mode: %d\n", err);
+
+	msleep(120);
+
+	regulator_disable(sharp->supply);
+
+	sharp->prepared = false;
+
+	return 0;
+}
+
+static int sharp_setup_symmetrical_split(struct mipi_dsi_device *left,
+					 struct mipi_dsi_device *right,
+					 const struct drm_display_mode *mode)
+{
+	int err;
+
+	err = mipi_dsi_dcs_set_column_address(left, 0, mode->hdisplay / 2 - 1);
+	if (err < 0) {
+		dev_err(&left->dev, "failed to set column address: %d\n", err);
+		return err;
+	}
+
+	err = mipi_dsi_dcs_set_page_address(left, 0, mode->vdisplay - 1);
+	if (err < 0) {
+		dev_err(&left->dev, "failed to set page address: %d\n", err);
+		return err;
+	}
+
+	err = mipi_dsi_dcs_set_column_address(right, mode->hdisplay / 2,
+					      mode->hdisplay - 1);
+	if (err < 0) {
+		dev_err(&right->dev, "failed to set column address: %d\n", err);
+		return err;
+	}
+
+	err = mipi_dsi_dcs_set_page_address(right, 0, mode->vdisplay - 1);
+	if (err < 0) {
+		dev_err(&right->dev, "failed to set page address: %d\n", err);
+		return err;
+	}
+
+	return 0;
+}
+
+static int sharp_panel_prepare(struct drm_panel *panel)
+{
+	struct sharp_panel *sharp = to_sharp_panel(panel);
+	u8 format = MIPI_DCS_PIXEL_FMT_24BIT;
+	int err;
+
+	if (sharp->prepared)
+		return 0;
+
+	err = regulator_enable(sharp->supply);
+	if (err < 0)
+		return err;
+
+	usleep_range(10000, 20000);
+
+	err = mipi_dsi_dcs_soft_reset(sharp->link1);
+	if (err < 0) {
+		dev_err(panel->dev, "soft reset failed: %d\n", err);
+		goto poweroff;
+	}
+
+	msleep(120);
+
+	err = mipi_dsi_dcs_exit_sleep_mode(sharp->link1);
+	if (err < 0) {
+		dev_err(panel->dev, "failed to exit sleep mode: %d\n", err);
+		goto poweroff;
+	}
+
+	/*
+	 * The MIPI DCS specification mandates this delay only between the
+	 * exit_sleep_mode and enter_sleep_mode commands, so it isn't strictly
+	 * necessary here.
+	 */
+	/*
+	msleep(120);
+	*/
+
+	/* set left-right mode */
+	err = sharp_panel_write(sharp, 0x1000, 0x2a);
+	if (err < 0) {
+		dev_err(panel->dev, "failed to set left-right mode: %d\n", err);
+		goto poweroff;
+	}
+
+	/* enable command mode */
+	err = sharp_panel_write(sharp, 0x1001, 0x01);
+	if (err < 0) {
+		dev_err(panel->dev, "failed to enable command mode: %d\n", err);
+		goto poweroff;
+	}
+
+	err = mipi_dsi_dcs_set_pixel_format(sharp->link1, format);
+	if (err < 0) {
+		dev_err(panel->dev, "failed to set pixel format: %d\n", err);
+		goto poweroff;
+	}
+
+	/*
+	 * TODO: The device supports both left-right and even-odd split
+	 * configurations, but this driver currently supports only the left-
+	 * right split. To support a different mode a mechanism needs to be
+	 * put in place to communicate the configuration back to the DSI host
+	 * controller.
+	 */
+	err = sharp_setup_symmetrical_split(sharp->link1, sharp->link2,
+					    sharp->mode);
+	if (err < 0) {
+		dev_err(panel->dev, "failed to set up symmetrical split: %d\n",
+			err);
+		goto poweroff;
+	}
+
+	err = mipi_dsi_dcs_set_display_on(sharp->link1);
+	if (err < 0) {
+		dev_err(panel->dev, "failed to set display on: %d\n", err);
+		goto poweroff;
+	}
+
+	sharp->prepared = true;
+
+	return 0;
+
+poweroff:
+	regulator_disable(sharp->supply);
+	return err;
+}
+
+static int sharp_panel_enable(struct drm_panel *panel)
+{
+	struct sharp_panel *sharp = to_sharp_panel(panel);
+
+	if (sharp->enabled)
+		return 0;
+
+	if (sharp->backlight) {
+		sharp->backlight->props.power = FB_BLANK_UNBLANK;
+		backlight_update_status(sharp->backlight);
+	}
+
+	sharp->enabled = true;
+
+	return 0;
+}
+
+static const struct drm_display_mode default_mode = {
+	.clock = 278000,
+	.hdisplay = 2560,
+	.hsync_start = 2560 + 128,
+	.hsync_end = 2560 + 128 + 64,
+	.htotal = 2560 + 128 + 64 + 64,
+	.vdisplay = 1600,
+	.vsync_start = 1600 + 4,
+	.vsync_end = 1600 + 4 + 8,
+	.vtotal = 1600 + 4 + 8 + 32,
+	.vrefresh = 60,
+};
+
+static int sharp_panel_get_modes(struct drm_panel *panel)
+{
+	struct drm_display_mode *mode;
+
+	mode = drm_mode_duplicate(panel->drm, &default_mode);
+	if (!mode) {
+		dev_err(panel->drm->dev, "failed to add mode %ux%ux@%u\n",
+			default_mode.hdisplay, default_mode.vdisplay,
+			default_mode.vrefresh);
+		return -ENOMEM;
+	}
+
+	drm_mode_set_name(mode);
+
+	drm_mode_probed_add(panel->connector, mode);
+
+	panel->connector->display_info.width_mm = 217;
+	panel->connector->display_info.height_mm = 136;
+
+	return 1;
+}
+
+static const struct drm_panel_funcs sharp_panel_funcs = {
+	.disable = sharp_panel_disable,
+	.unprepare = sharp_panel_unprepare,
+	.prepare = sharp_panel_prepare,
+	.enable = sharp_panel_enable,
+	.get_modes = sharp_panel_get_modes,
+};
+
+static const struct of_device_id sharp_of_match[] = {
+	{ .compatible = "sharp,lq101r1sx01", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, sharp_of_match);
+
+static int sharp_panel_add(struct sharp_panel *sharp)
+{
+	struct device_node *np;
+	int err;
+
+	sharp->mode = &default_mode;
+
+	sharp->supply = devm_regulator_get(&sharp->link1->dev, "power");
+	if (IS_ERR(sharp->supply))
+		return PTR_ERR(sharp->supply);
+
+	np = of_parse_phandle(sharp->link1->dev.of_node, "backlight", 0);
+	if (np) {
+		sharp->backlight = of_find_backlight_by_node(np);
+		of_node_put(np);
+
+		if (!sharp->backlight)
+			return -EPROBE_DEFER;
+	}
+
+	drm_panel_init(&sharp->base);
+	sharp->base.funcs = &sharp_panel_funcs;
+	sharp->base.dev = &sharp->link1->dev;
+
+	err = drm_panel_add(&sharp->base);
+	if (err < 0)
+		goto put_backlight;
+
+	return 0;
+
+put_backlight:
+	if (sharp->backlight)
+		put_device(&sharp->backlight->dev);
+
+	return err;
+}
+
+static void sharp_panel_del(struct sharp_panel *sharp)
+{
+	if (sharp->base.dev)
+		drm_panel_remove(&sharp->base);
+
+	if (sharp->backlight)
+		put_device(&sharp->backlight->dev);
+
+	if (sharp->link2)
+		put_device(&sharp->link2->dev);
+}
+
+static int sharp_panel_probe(struct mipi_dsi_device *dsi)
+{
+	struct mipi_dsi_device *secondary = NULL;
+	struct sharp_panel *sharp;
+	struct device_node *np;
+	int err;
+
+	dsi->lanes = 4;
+	dsi->format = MIPI_DSI_FMT_RGB888;
+	dsi->mode_flags = MIPI_DSI_MODE_LPM;
+
+	/* Find DSI-LINK1 */
+	np = of_parse_phandle(dsi->dev.of_node, "link2", 0);
+	if (np) {
+		secondary = of_find_mipi_dsi_device_by_node(np);
+		of_node_put(np);
+
+		if (!secondary)
+			return -EPROBE_DEFER;
+	}
+
+	/* register a panel for only the DSI-LINK1 interface */
+	if (secondary) {
+		sharp = devm_kzalloc(&dsi->dev, sizeof(*sharp), GFP_KERNEL);
+		if (!sharp) {
+			put_device(&secondary->dev);
+			return -ENOMEM;
+		}
+
+		mipi_dsi_set_drvdata(dsi, sharp);
+
+		sharp->link2 = secondary;
+		sharp->link1 = dsi;
+
+		err = sharp_panel_add(sharp);
+		if (err < 0) {
+			put_device(&secondary->dev);
+			return err;
+		}
+	}
+
+	err = mipi_dsi_attach(dsi);
+	if (err < 0) {
+		if (secondary)
+			sharp_panel_del(sharp);
+
+		return err;
+	}
+
+	return 0;
+}
+
+static int sharp_panel_remove(struct mipi_dsi_device *dsi)
+{
+	struct sharp_panel *sharp = mipi_dsi_get_drvdata(dsi);
+	int err;
+
+	/* only detach from host for the DSI-LINK2 interface */
+	if (!sharp) {
+		mipi_dsi_detach(dsi);
+		return 0;
+	}
+
+	err = sharp_panel_disable(&sharp->base);
+	if (err < 0)
+		dev_err(&dsi->dev, "failed to disable panel: %d\n", err);
+
+	err = mipi_dsi_detach(dsi);
+	if (err < 0)
+		dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", err);
+
+	drm_panel_detach(&sharp->base);
+	sharp_panel_del(sharp);
+
+	return 0;
+}
+
+static void sharp_panel_shutdown(struct mipi_dsi_device *dsi)
+{
+	struct sharp_panel *sharp = mipi_dsi_get_drvdata(dsi);
+
+	/* nothing to do for DSI-LINK2 */
+	if (!sharp)
+		return;
+
+	sharp_panel_disable(&sharp->base);
+}
+
+static struct mipi_dsi_driver sharp_panel_driver = {
+	.driver = {
+		.name = "panel-sharp-lq101r1sx01",
+		.of_match_table = sharp_of_match,
+	},
+	.probe = sharp_panel_probe,
+	.remove = sharp_panel_remove,
+	.shutdown = sharp_panel_shutdown,
+};
+module_mipi_dsi_driver(sharp_panel_driver);
+
+MODULE_AUTHOR("Thierry Reding <treding@nvidia.com>");
+MODULE_DESCRIPTION("Sharp LQ101R1SX01 panel driver");
+MODULE_LICENSE("GPL v2");
-- 
2.1.2

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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [PATCH v4 13/16] drm/dsi: Implement DCS {get, set}_pixel_format commands
  2014-11-03  9:13 ` [PATCH v4 13/16] drm/dsi: Implement DCS {get, set}_pixel_format commands Thierry Reding
@ 2014-11-03 17:10   ` Sean Paul
  2014-11-04 13:34     ` [PATCH v4 13/16] drm/dsi: Implement DCS {get,set}_pixel_format commands Thierry Reding
  0 siblings, 1 reply; 28+ messages in thread
From: Sean Paul @ 2014-11-03 17:10 UTC (permalink / raw)
  To: Thierry Reding; +Cc: Andrzej Hajda, dri-devel

On Mon, Nov 3, 2014 at 4:13 AM, Thierry Reding <thierry.reding@gmail.com> wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> Provide small convenience wrappers to query or set the pixel format used
> by the interface.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/gpu/drm/drm_mipi_dsi.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_mipi_dsi.h     |  2 ++
>  2 files changed, 44 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> index da34469b9b98..226822a44457 100644
> --- a/drivers/gpu/drm/drm_mipi_dsi.c
> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> @@ -553,6 +553,27 @@ int mipi_dsi_dcs_get_power_mode(struct mipi_dsi_device *dsi, u8 *mode)
>  EXPORT_SYMBOL(mipi_dsi_dcs_get_power_mode);
>
>  /**
> + * mipi_dsi_dcs_get_pixel_format() - gets the pixel format for the RGB image
> + *    data used by the interface
> + * @dsi: DSI peripheral device
> + * @format: return location for the pixel format
> + *
> + * Return: 0 on success or a negative error code on failure.
> + */
> +int mipi_dsi_dcs_get_pixel_format(struct mipi_dsi_device *dsi, u8 *format)
> +{
> +       ssize_t err;
> +
> +       err = mipi_dsi_dcs_read(dsi, MIPI_DCS_GET_PIXEL_FORMAT, format,
> +                               sizeof(*format));
> +       if (err < 0)
> +               return err;

This should probably return -ENODATA when err != sizeof(*format), like
the previous patch did.

It might be worth moving that logic into mipi_dsi_dcs_read, assuming
we're not interested in partial data.

Sean



> +
> +       return 0;
> +}
> +EXPORT_SYMBOL(mipi_dsi_dcs_get_pixel_format);
> +
> +/**
>   * mipi_dsi_dcs_enter_sleep_mode() - disable all unnecessary blocks inside the
>   *    display module except interface communication
>   * @dsi: DSI peripheral device
> @@ -670,6 +691,27 @@ int mipi_dsi_dcs_set_tear_on(struct mipi_dsi_device *dsi,
>  }
>  EXPORT_SYMBOL(mipi_dsi_dcs_set_tear_on);
>
> +/**
> + * mipi_dsi_dcs_set_pixel_format() - sets the pixel format for the RGB image
> + *    data used by the interface
> + * @dsi: DSI peripheral device
> + * @format: pixel format
> + *
> + * Return: 0 on success or a negative error code on failure.
> + */
> +int mipi_dsi_dcs_set_pixel_format(struct mipi_dsi_device *dsi, u8 format)
> +{
> +       ssize_t err;
> +
> +       err = mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_PIXEL_FORMAT, &format,
> +                                sizeof(format));
> +       if (err < 0)
> +               return err;
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL(mipi_dsi_dcs_set_pixel_format);
> +
>  static int mipi_dsi_drv_probe(struct device *dev)
>  {
>         struct mipi_dsi_driver *drv = to_mipi_dsi_driver(dev->driver);
> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
> index 4cbf8e658a3a..415d01f90086 100644
> --- a/include/drm/drm_mipi_dsi.h
> +++ b/include/drm/drm_mipi_dsi.h
> @@ -197,6 +197,7 @@ ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, u8 cmd, void *data,
>  int mipi_dsi_dcs_nop(struct mipi_dsi_device *dsi);
>  int mipi_dsi_dcs_soft_reset(struct mipi_dsi_device *dsi);
>  int mipi_dsi_dcs_get_power_mode(struct mipi_dsi_device *dsi, u8 *mode);
> +int mipi_dsi_dcs_get_pixel_format(struct mipi_dsi_device *dsi, u8 *format);
>  int mipi_dsi_dcs_enter_sleep_mode(struct mipi_dsi_device *dsi);
>  int mipi_dsi_dcs_exit_sleep_mode(struct mipi_dsi_device *dsi);
>  int mipi_dsi_dcs_set_display_off(struct mipi_dsi_device *dsi);
> @@ -204,6 +205,7 @@ int mipi_dsi_dcs_set_display_on(struct mipi_dsi_device *dsi);
>  int mipi_dsi_dcs_set_tear_off(struct mipi_dsi_device *dsi);
>  int mipi_dsi_dcs_set_tear_on(struct mipi_dsi_device *dsi,
>                              enum mipi_dsi_dcs_tear_mode mode);
> +int mipi_dsi_dcs_set_pixel_format(struct mipi_dsi_device *dsi, u8 format);
>
>  /**
>   * struct mipi_dsi_driver - DSI driver
> --
> 2.1.2
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v4 16/16] drm/panel: Add Sharp LQ101R1SX01 support
  2014-11-03  9:13 ` [PATCH v4 16/16] drm/panel: Add Sharp LQ101R1SX01 support Thierry Reding
@ 2014-11-03 17:28   ` Sean Paul
  2014-11-04 10:41   ` Andrzej Hajda
  1 sibling, 0 replies; 28+ messages in thread
From: Sean Paul @ 2014-11-03 17:28 UTC (permalink / raw)
  To: Thierry Reding; +Cc: Andrzej Hajda, dri-devel

On Mon, Nov 3, 2014 at 4:13 AM, Thierry Reding <thierry.reding@gmail.com> wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> This panel requires dual-channel mode. The device accepts command-mode
> data on 8 lanes and will therefore need a dual-channel DSI controller.
> The two interfaces that make up this device need to be instantiated in
> the controllers that gang up to provide the dual-channel DSI host.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>


Aside from the ENODATA nit I picked in patch 13, feel free to add my
R-b to all of the patches in this set.

Reviewed-by: Sean Paul <seanpaul@chromium.org>



> ---
> Changes in v4:
> - use low power mode since highspeed message transfers don't work
> - clarify required and optional properties for both DSI links
> - power off panel when .prepare() fails
> - properly drop reference to DSI-LINK2
> - don't allocate memory for DSI-LINK2
> - propagate errors on failure
>
>  .../bindings/panel/sharp,lq101r1sx01.txt           |  49 +++
>  drivers/gpu/drm/panel/Kconfig                      |  13 +
>  drivers/gpu/drm/panel/Makefile                     |   1 +
>  drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c    | 464 +++++++++++++++++++++
>  4 files changed, 527 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt
>  create mode 100644 drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
>
> diff --git a/Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt b/Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt
> new file mode 100644
> index 000000000000..f522bb8e47e1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt
> @@ -0,0 +1,49 @@
> +Sharp Microelectronics 10.1" WQXGA TFT LCD panel
> +
> +This panel requires a dual-channel DSI host to operate. It supports two modes:
> +- left-right: each channel drives the left or right half of the screen
> +- even-odd: each channel drives the even or odd lines of the screen
> +
> +Each of the DSI channels controls a separate DSI peripheral. The peripheral
> +driven by the first link (DSI-LINK1), left or even, is considered the primary
> +peripheral and controls the device. The 'link2' property contains a phandle
> +to the peripheral driven by the second link (DSI-LINK2, right or odd).
> +
> +Note that in video mode the DSI-LINK1 interface always provides the left/even
> +pixels and DSI-LINK2 always provides the right/odd pixels. In command mode it
> +is possible to program either link to drive the left/even or right/odd pixels
> +but for the sake of consistency this binding assumes that the same assignment
> +is chosen as for video mode.
> +
> +Required properties:
> +- compatible: should be "sharp,lq101r1sx01"
> +- reg: DSI virtual channel of the peripheral
> +
> +Required properties (for DSI-LINK1 only):
> +- link2: phandle to the DSI peripheral on the secondary link. Note that the
> +  presence of this property marks the containing node as DSI-LINK1.
> +- power-supply: phandle of the regulator that provides the supply voltage
> +
> +Optional properties (for DSI-LINK1 only):
> +- backlight: phandle of the backlight device attached to the panel
> +
> +Example:
> +
> +       dsi@54300000 {
> +               panel: panel@0 {
> +                       compatible = "sharp,lq101r1sx01";
> +                       reg = <0>;
> +
> +                       link2 = <&secondary>;
> +
> +                       power-supply = <...>;
> +                       backlight = <...>;
> +               };
> +       };
> +
> +       dsi@54400000 {
> +               secondary: panel@0 {
> +                       compatible = "sharp,lq101r1sx01";
> +                       reg = <0>;
> +               };
> +       };
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index bee9f72b3a93..024e98ef8e4d 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -27,4 +27,17 @@ config DRM_PANEL_S6E8AA0
>         select DRM_MIPI_DSI
>         select VIDEOMODE_HELPERS
>
> +config DRM_PANEL_SHARP_LQ101R1SX01
> +       tristate "Sharp LQ101R1SX01 panel"
> +       depends on OF
> +       depends on DRM_MIPI_DSI
> +       help
> +         Say Y here if you want to enable support for Sharp LQ101R1SX01
> +         TFT-LCD modules. The panel has a 2560x1600 resolution and uses
> +         24 bit RGB per pixel. It provides a dual MIPI DSI interface to
> +         the host and has a built-in LED backlight.
> +
> +         To compile this driver as a module, choose M here: the module
> +         will be called panel-sharp-lq101r1sx01.
> +
>  endmenu
> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> index 8b929212fad7..4b2a0430804b 100644
> --- a/drivers/gpu/drm/panel/Makefile
> +++ b/drivers/gpu/drm/panel/Makefile
> @@ -1,3 +1,4 @@
>  obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.o
>  obj-$(CONFIG_DRM_PANEL_LD9040) += panel-ld9040.o
>  obj-$(CONFIG_DRM_PANEL_S6E8AA0) += panel-s6e8aa0.o
> +obj-$(CONFIG_DRM_PANEL_SHARP_LQ101R1SX01) += panel-sharp-lq101r1sx01.o
> diff --git a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
> new file mode 100644
> index 000000000000..ee0e7f57e4a1
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
> @@ -0,0 +1,464 @@
> +/*
> + * Copyright (C) 2014 NVIDIA Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/backlight.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_panel.h>
> +
> +#include <video/mipi_display.h>
> +
> +#include <linux/host1x.h>
> +
> +struct sharp_panel {
> +       struct drm_panel base;
> +       /* the datasheet refers to them as DSI-LINK1 and DSI-LINK2 */
> +       struct mipi_dsi_device *link1;
> +       struct mipi_dsi_device *link2;
> +
> +       struct backlight_device *backlight;
> +       struct regulator *supply;
> +
> +       bool prepared;
> +       bool enabled;
> +
> +       const struct drm_display_mode *mode;
> +};
> +
> +static inline struct sharp_panel *to_sharp_panel(struct drm_panel *panel)
> +{
> +       return container_of(panel, struct sharp_panel, base);
> +}
> +
> +static int sharp_panel_write(struct sharp_panel *sharp, u16 offset, u8 value)
> +{
> +       u8 payload[3] = { offset >> 8, offset & 0xff, value };
> +       struct mipi_dsi_device *dsi = sharp->link1;
> +       ssize_t err;
> +
> +       err = mipi_dsi_generic_write(dsi, payload, sizeof(payload));
> +       if (err < 0) {
> +               dev_err(&dsi->dev, "failed to write %02x to %04x: %d\n",
> +                       value, offset, err);
> +               return err;
> +       }
> +
> +       err = mipi_dsi_dcs_nop(dsi);
> +       if (err < 0) {
> +               dev_err(&dsi->dev, "failed to send DCS nop: %d\n", err);
> +               return err;
> +       }
> +
> +       usleep_range(10, 20);
> +
> +       return 0;
> +}
> +
> +static __maybe_unused int sharp_panel_read(struct sharp_panel *sharp,
> +                                          u16 offset, u8 *value)
> +{
> +       ssize_t err;
> +
> +       cpu_to_be16s(&offset);
> +
> +       err = mipi_dsi_generic_read(sharp->link1, &offset, sizeof(offset),
> +                                   value, sizeof(*value));
> +       if (err < 0)
> +               dev_err(&sharp->link1->dev, "failed to read from %04x: %d\n",
> +                       offset, err);
> +
> +       return err;
> +}
> +
> +static int sharp_panel_disable(struct drm_panel *panel)
> +{
> +       struct sharp_panel *sharp = to_sharp_panel(panel);
> +
> +       if (!sharp->enabled)
> +               return 0;
> +
> +       if (sharp->backlight) {
> +               sharp->backlight->props.power = FB_BLANK_POWERDOWN;
> +               backlight_update_status(sharp->backlight);
> +       }
> +
> +       sharp->enabled = false;
> +
> +       return 0;
> +}
> +
> +static int sharp_panel_unprepare(struct drm_panel *panel)
> +{
> +       struct sharp_panel *sharp = to_sharp_panel(panel);
> +       int err;
> +
> +       if (!sharp->prepared)
> +               return 0;
> +
> +       err = mipi_dsi_dcs_set_display_off(sharp->link1);
> +       if (err < 0)
> +               dev_err(panel->dev, "failed to set display off: %d\n", err);
> +
> +       err = mipi_dsi_dcs_enter_sleep_mode(sharp->link1);
> +       if (err < 0)
> +               dev_err(panel->dev, "failed to enter sleep mode: %d\n", err);
> +
> +       msleep(120);
> +
> +       regulator_disable(sharp->supply);
> +
> +       sharp->prepared = false;
> +
> +       return 0;
> +}
> +
> +static int sharp_setup_symmetrical_split(struct mipi_dsi_device *left,
> +                                        struct mipi_dsi_device *right,
> +                                        const struct drm_display_mode *mode)
> +{
> +       int err;
> +
> +       err = mipi_dsi_dcs_set_column_address(left, 0, mode->hdisplay / 2 - 1);
> +       if (err < 0) {
> +               dev_err(&left->dev, "failed to set column address: %d\n", err);
> +               return err;
> +       }
> +
> +       err = mipi_dsi_dcs_set_page_address(left, 0, mode->vdisplay - 1);
> +       if (err < 0) {
> +               dev_err(&left->dev, "failed to set page address: %d\n", err);
> +               return err;
> +       }
> +
> +       err = mipi_dsi_dcs_set_column_address(right, mode->hdisplay / 2,
> +                                             mode->hdisplay - 1);
> +       if (err < 0) {
> +               dev_err(&right->dev, "failed to set column address: %d\n", err);
> +               return err;
> +       }
> +
> +       err = mipi_dsi_dcs_set_page_address(right, 0, mode->vdisplay - 1);
> +       if (err < 0) {
> +               dev_err(&right->dev, "failed to set page address: %d\n", err);
> +               return err;
> +       }
> +
> +       return 0;
> +}
> +
> +static int sharp_panel_prepare(struct drm_panel *panel)
> +{
> +       struct sharp_panel *sharp = to_sharp_panel(panel);
> +       u8 format = MIPI_DCS_PIXEL_FMT_24BIT;
> +       int err;
> +
> +       if (sharp->prepared)
> +               return 0;
> +
> +       err = regulator_enable(sharp->supply);
> +       if (err < 0)
> +               return err;
> +
> +       usleep_range(10000, 20000);
> +
> +       err = mipi_dsi_dcs_soft_reset(sharp->link1);
> +       if (err < 0) {
> +               dev_err(panel->dev, "soft reset failed: %d\n", err);
> +               goto poweroff;
> +       }
> +
> +       msleep(120);
> +
> +       err = mipi_dsi_dcs_exit_sleep_mode(sharp->link1);
> +       if (err < 0) {
> +               dev_err(panel->dev, "failed to exit sleep mode: %d\n", err);
> +               goto poweroff;
> +       }
> +
> +       /*
> +        * The MIPI DCS specification mandates this delay only between the
> +        * exit_sleep_mode and enter_sleep_mode commands, so it isn't strictly
> +        * necessary here.
> +        */
> +       /*
> +       msleep(120);
> +       */
> +
> +       /* set left-right mode */
> +       err = sharp_panel_write(sharp, 0x1000, 0x2a);
> +       if (err < 0) {
> +               dev_err(panel->dev, "failed to set left-right mode: %d\n", err);
> +               goto poweroff;
> +       }
> +
> +       /* enable command mode */
> +       err = sharp_panel_write(sharp, 0x1001, 0x01);
> +       if (err < 0) {
> +               dev_err(panel->dev, "failed to enable command mode: %d\n", err);
> +               goto poweroff;
> +       }
> +
> +       err = mipi_dsi_dcs_set_pixel_format(sharp->link1, format);
> +       if (err < 0) {
> +               dev_err(panel->dev, "failed to set pixel format: %d\n", err);
> +               goto poweroff;
> +       }
> +
> +       /*
> +        * TODO: The device supports both left-right and even-odd split
> +        * configurations, but this driver currently supports only the left-
> +        * right split. To support a different mode a mechanism needs to be
> +        * put in place to communicate the configuration back to the DSI host
> +        * controller.
> +        */
> +       err = sharp_setup_symmetrical_split(sharp->link1, sharp->link2,
> +                                           sharp->mode);
> +       if (err < 0) {
> +               dev_err(panel->dev, "failed to set up symmetrical split: %d\n",
> +                       err);
> +               goto poweroff;
> +       }
> +
> +       err = mipi_dsi_dcs_set_display_on(sharp->link1);
> +       if (err < 0) {
> +               dev_err(panel->dev, "failed to set display on: %d\n", err);
> +               goto poweroff;
> +       }
> +
> +       sharp->prepared = true;
> +
> +       return 0;
> +
> +poweroff:
> +       regulator_disable(sharp->supply);
> +       return err;
> +}
> +
> +static int sharp_panel_enable(struct drm_panel *panel)
> +{
> +       struct sharp_panel *sharp = to_sharp_panel(panel);
> +
> +       if (sharp->enabled)
> +               return 0;
> +
> +       if (sharp->backlight) {
> +               sharp->backlight->props.power = FB_BLANK_UNBLANK;
> +               backlight_update_status(sharp->backlight);
> +       }
> +
> +       sharp->enabled = true;
> +
> +       return 0;
> +}
> +
> +static const struct drm_display_mode default_mode = {
> +       .clock = 278000,
> +       .hdisplay = 2560,
> +       .hsync_start = 2560 + 128,
> +       .hsync_end = 2560 + 128 + 64,
> +       .htotal = 2560 + 128 + 64 + 64,
> +       .vdisplay = 1600,
> +       .vsync_start = 1600 + 4,
> +       .vsync_end = 1600 + 4 + 8,
> +       .vtotal = 1600 + 4 + 8 + 32,
> +       .vrefresh = 60,
> +};
> +
> +static int sharp_panel_get_modes(struct drm_panel *panel)
> +{
> +       struct drm_display_mode *mode;
> +
> +       mode = drm_mode_duplicate(panel->drm, &default_mode);
> +       if (!mode) {
> +               dev_err(panel->drm->dev, "failed to add mode %ux%ux@%u\n",
> +                       default_mode.hdisplay, default_mode.vdisplay,
> +                       default_mode.vrefresh);
> +               return -ENOMEM;
> +       }
> +
> +       drm_mode_set_name(mode);
> +
> +       drm_mode_probed_add(panel->connector, mode);
> +
> +       panel->connector->display_info.width_mm = 217;
> +       panel->connector->display_info.height_mm = 136;
> +
> +       return 1;
> +}
> +
> +static const struct drm_panel_funcs sharp_panel_funcs = {
> +       .disable = sharp_panel_disable,
> +       .unprepare = sharp_panel_unprepare,
> +       .prepare = sharp_panel_prepare,
> +       .enable = sharp_panel_enable,
> +       .get_modes = sharp_panel_get_modes,
> +};
> +
> +static const struct of_device_id sharp_of_match[] = {
> +       { .compatible = "sharp,lq101r1sx01", },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(of, sharp_of_match);
> +
> +static int sharp_panel_add(struct sharp_panel *sharp)
> +{
> +       struct device_node *np;
> +       int err;
> +
> +       sharp->mode = &default_mode;
> +
> +       sharp->supply = devm_regulator_get(&sharp->link1->dev, "power");
> +       if (IS_ERR(sharp->supply))
> +               return PTR_ERR(sharp->supply);
> +
> +       np = of_parse_phandle(sharp->link1->dev.of_node, "backlight", 0);
> +       if (np) {
> +               sharp->backlight = of_find_backlight_by_node(np);
> +               of_node_put(np);
> +
> +               if (!sharp->backlight)
> +                       return -EPROBE_DEFER;
> +       }
> +
> +       drm_panel_init(&sharp->base);
> +       sharp->base.funcs = &sharp_panel_funcs;
> +       sharp->base.dev = &sharp->link1->dev;
> +
> +       err = drm_panel_add(&sharp->base);
> +       if (err < 0)
> +               goto put_backlight;
> +
> +       return 0;
> +
> +put_backlight:
> +       if (sharp->backlight)
> +               put_device(&sharp->backlight->dev);
> +
> +       return err;
> +}
> +
> +static void sharp_panel_del(struct sharp_panel *sharp)
> +{
> +       if (sharp->base.dev)
> +               drm_panel_remove(&sharp->base);
> +
> +       if (sharp->backlight)
> +               put_device(&sharp->backlight->dev);
> +
> +       if (sharp->link2)
> +               put_device(&sharp->link2->dev);
> +}
> +
> +static int sharp_panel_probe(struct mipi_dsi_device *dsi)
> +{
> +       struct mipi_dsi_device *secondary = NULL;
> +       struct sharp_panel *sharp;
> +       struct device_node *np;
> +       int err;
> +
> +       dsi->lanes = 4;
> +       dsi->format = MIPI_DSI_FMT_RGB888;
> +       dsi->mode_flags = MIPI_DSI_MODE_LPM;
> +
> +       /* Find DSI-LINK1 */
> +       np = of_parse_phandle(dsi->dev.of_node, "link2", 0);
> +       if (np) {
> +               secondary = of_find_mipi_dsi_device_by_node(np);
> +               of_node_put(np);
> +
> +               if (!secondary)
> +                       return -EPROBE_DEFER;
> +       }
> +
> +       /* register a panel for only the DSI-LINK1 interface */
> +       if (secondary) {
> +               sharp = devm_kzalloc(&dsi->dev, sizeof(*sharp), GFP_KERNEL);
> +               if (!sharp) {
> +                       put_device(&secondary->dev);
> +                       return -ENOMEM;
> +               }
> +
> +               mipi_dsi_set_drvdata(dsi, sharp);
> +
> +               sharp->link2 = secondary;
> +               sharp->link1 = dsi;
> +
> +               err = sharp_panel_add(sharp);
> +               if (err < 0) {
> +                       put_device(&secondary->dev);
> +                       return err;
> +               }
> +       }
> +
> +       err = mipi_dsi_attach(dsi);
> +       if (err < 0) {
> +               if (secondary)
> +                       sharp_panel_del(sharp);
> +
> +               return err;
> +       }
> +
> +       return 0;
> +}
> +
> +static int sharp_panel_remove(struct mipi_dsi_device *dsi)
> +{
> +       struct sharp_panel *sharp = mipi_dsi_get_drvdata(dsi);
> +       int err;
> +
> +       /* only detach from host for the DSI-LINK2 interface */
> +       if (!sharp) {
> +               mipi_dsi_detach(dsi);
> +               return 0;
> +       }
> +
> +       err = sharp_panel_disable(&sharp->base);
> +       if (err < 0)
> +               dev_err(&dsi->dev, "failed to disable panel: %d\n", err);
> +
> +       err = mipi_dsi_detach(dsi);
> +       if (err < 0)
> +               dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", err);
> +
> +       drm_panel_detach(&sharp->base);
> +       sharp_panel_del(sharp);
> +
> +       return 0;
> +}
> +
> +static void sharp_panel_shutdown(struct mipi_dsi_device *dsi)
> +{
> +       struct sharp_panel *sharp = mipi_dsi_get_drvdata(dsi);
> +
> +       /* nothing to do for DSI-LINK2 */
> +       if (!sharp)
> +               return;
> +
> +       sharp_panel_disable(&sharp->base);
> +}
> +
> +static struct mipi_dsi_driver sharp_panel_driver = {
> +       .driver = {
> +               .name = "panel-sharp-lq101r1sx01",
> +               .of_match_table = sharp_of_match,
> +       },
> +       .probe = sharp_panel_probe,
> +       .remove = sharp_panel_remove,
> +       .shutdown = sharp_panel_shutdown,
> +};
> +module_mipi_dsi_driver(sharp_panel_driver);
> +
> +MODULE_AUTHOR("Thierry Reding <treding@nvidia.com>");
> +MODULE_DESCRIPTION("Sharp LQ101R1SX01 panel driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.1.2
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v4 16/16] drm/panel: Add Sharp LQ101R1SX01 support
  2014-11-03  9:13 ` [PATCH v4 16/16] drm/panel: Add Sharp LQ101R1SX01 support Thierry Reding
  2014-11-03 17:28   ` Sean Paul
@ 2014-11-04 10:41   ` Andrzej Hajda
  2014-11-04 13:29     ` Thierry Reding
  1 sibling, 1 reply; 28+ messages in thread
From: Andrzej Hajda @ 2014-11-04 10:41 UTC (permalink / raw)
  To: Thierry Reding; +Cc: dri-devel

On 11/03/2014 10:13 AM, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> This panel requires dual-channel mode. The device accepts command-mode
> data on 8 lanes and will therefore need a dual-channel DSI controller.
> The two interfaces that make up this device need to be instantiated in
> the controllers that gang up to provide the dual-channel DSI host.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Changes in v4:
> - use low power mode since highspeed message transfers don't work
> - clarify required and optional properties for both DSI links
> - power off panel when .prepare() fails
> - properly drop reference to DSI-LINK2
> - don't allocate memory for DSI-LINK2
> - propagate errors on failure
> 
>  .../bindings/panel/sharp,lq101r1sx01.txt           |  49 +++
>  drivers/gpu/drm/panel/Kconfig                      |  13 +
>  drivers/gpu/drm/panel/Makefile                     |   1 +
>  drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c    | 464 +++++++++++++++++++++
>  4 files changed, 527 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt
>  create mode 100644 drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
> 
> diff --git a/Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt b/Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt
> new file mode 100644
> index 000000000000..f522bb8e47e1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt
> @@ -0,0 +1,49 @@
> +Sharp Microelectronics 10.1" WQXGA TFT LCD panel
> +
> +This panel requires a dual-channel DSI host to operate. It supports two modes:
> +- left-right: each channel drives the left or right half of the screen
> +- even-odd: each channel drives the even or odd lines of the screen
> +
> +Each of the DSI channels controls a separate DSI peripheral. The peripheral
> +driven by the first link (DSI-LINK1), left or even, is considered the primary
> +peripheral and controls the device. The 'link2' property contains a phandle
> +to the peripheral driven by the second link (DSI-LINK2, right or odd).
> +
> +Note that in video mode the DSI-LINK1 interface always provides the left/even
> +pixels and DSI-LINK2 always provides the right/odd pixels. In command mode it
> +is possible to program either link to drive the left/even or right/odd pixels
> +but for the sake of consistency this binding assumes that the same assignment
> +is chosen as for video mode.
> +
> +Required properties:
> +- compatible: should be "sharp,lq101r1sx01"
> +- reg: DSI virtual channel of the peripheral
> +
> +Required properties (for DSI-LINK1 only):
> +- link2: phandle to the DSI peripheral on the secondary link. Note that the
> +  presence of this property marks the containing node as DSI-LINK1.
> +- power-supply: phandle of the regulator that provides the supply voltage
> +
> +Optional properties (for DSI-LINK1 only):
> +- backlight: phandle of the backlight device attached to the panel
> +
> +Example:
> +
> +	dsi@54300000 {
> +		panel: panel@0 {
> +			compatible = "sharp,lq101r1sx01";
> +			reg = <0>;
> +
> +			link2 = <&secondary>;
> +
> +			power-supply = <...>;
> +			backlight = <...>;
> +		};
> +	};
> +
> +	dsi@54400000 {
> +		secondary: panel@0 {
> +			compatible = "sharp,lq101r1sx01";
> +			reg = <0>;
> +		};
> +	};

The bindings above and the implementation below clearly shows
that the secondary node is just a dummy dsi device.
Hiding this behind conditionals in both docs and code does not look good
to me. On the other side having common dummy dsi driver
would allow to reuse it in other dual-dsi devices.
So instead of 2nd node you would have:
	dsi@54400000 {
		secondary: panel@0 {
			compatible = "dsi-dummy-whatever";
			reg = <0>;
		};
	};

Or just:
	dsi2: dsi@54400000 {
	}

with phandle to dsi2 in 1st node, in such case 2nd dsi dev would be
created dynamically like with i2c_new_dummy.

But this is of course just my opinion.

> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index bee9f72b3a93..024e98ef8e4d 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -27,4 +27,17 @@ config DRM_PANEL_S6E8AA0
>  	select DRM_MIPI_DSI
>  	select VIDEOMODE_HELPERS
>  
> +config DRM_PANEL_SHARP_LQ101R1SX01
> +	tristate "Sharp LQ101R1SX01 panel"
> +	depends on OF
> +	depends on DRM_MIPI_DSI
> +	help
> +	  Say Y here if you want to enable support for Sharp LQ101R1SX01
> +	  TFT-LCD modules. The panel has a 2560x1600 resolution and uses
> +	  24 bit RGB per pixel. It provides a dual MIPI DSI interface to
> +	  the host and has a built-in LED backlight.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called panel-sharp-lq101r1sx01.
> +
>  endmenu
> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> index 8b929212fad7..4b2a0430804b 100644
> --- a/drivers/gpu/drm/panel/Makefile
> +++ b/drivers/gpu/drm/panel/Makefile
> @@ -1,3 +1,4 @@
>  obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.o
>  obj-$(CONFIG_DRM_PANEL_LD9040) += panel-ld9040.o
>  obj-$(CONFIG_DRM_PANEL_S6E8AA0) += panel-s6e8aa0.o
> +obj-$(CONFIG_DRM_PANEL_SHARP_LQ101R1SX01) += panel-sharp-lq101r1sx01.o
> diff --git a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
> new file mode 100644
> index 000000000000..ee0e7f57e4a1
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
> @@ -0,0 +1,464 @@
> +/*
> + * Copyright (C) 2014 NVIDIA Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/backlight.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_panel.h>
> +
> +#include <video/mipi_display.h>
> +
> +#include <linux/host1x.h>
> +
> +struct sharp_panel {
> +	struct drm_panel base;
> +	/* the datasheet refers to them as DSI-LINK1 and DSI-LINK2 */
> +	struct mipi_dsi_device *link1;
> +	struct mipi_dsi_device *link2;
> +
> +	struct backlight_device *backlight;
> +	struct regulator *supply;
> +
> +	bool prepared;
> +	bool enabled;

Nitpick.
As I wrote in review to previous version it is up to panel client
(usually connector) to call panel callbacks properly, we should not add
additional checks to panels. If call order is incorrect then the client
should be fixed.

> +
> +	const struct drm_display_mode *mode;
> +};
> +
> +static inline struct sharp_panel *to_sharp_panel(struct drm_panel *panel)
> +{
> +	return container_of(panel, struct sharp_panel, base);
> +}
> +
> +static int sharp_panel_write(struct sharp_panel *sharp, u16 offset, u8 value)
> +{
> +	u8 payload[3] = { offset >> 8, offset & 0xff, value };
> +	struct mipi_dsi_device *dsi = sharp->link1;
> +	ssize_t err;
> +
> +	err = mipi_dsi_generic_write(dsi, payload, sizeof(payload));
> +	if (err < 0) {
> +		dev_err(&dsi->dev, "failed to write %02x to %04x: %d\n",
> +			value, offset, err);
> +		return err;
> +	}
> +
> +	err = mipi_dsi_dcs_nop(dsi);
> +	if (err < 0) {
> +		dev_err(&dsi->dev, "failed to send DCS nop: %d\n", err);
> +		return err;
> +	}
> +
> +	usleep_range(10, 20);
> +
> +	return 0;
> +}
> +
> +static __maybe_unused int sharp_panel_read(struct sharp_panel *sharp,
> +					   u16 offset, u8 *value)
> +{
> +	ssize_t err;
> +
> +	cpu_to_be16s(&offset);
> +
> +	err = mipi_dsi_generic_read(sharp->link1, &offset, sizeof(offset),
> +				    value, sizeof(*value));
> +	if (err < 0)
> +		dev_err(&sharp->link1->dev, "failed to read from %04x: %d\n",
> +			offset, err);
> +
> +	return err;
> +}
> +
> +static int sharp_panel_disable(struct drm_panel *panel)
> +{
> +	struct sharp_panel *sharp = to_sharp_panel(panel);
> +
> +	if (!sharp->enabled)
> +		return 0;
> +
> +	if (sharp->backlight) {
> +		sharp->backlight->props.power = FB_BLANK_POWERDOWN;
> +		backlight_update_status(sharp->backlight);
> +	}
> +
> +	sharp->enabled = false;
> +
> +	return 0;
> +}
> +
> +static int sharp_panel_unprepare(struct drm_panel *panel)
> +{
> +	struct sharp_panel *sharp = to_sharp_panel(panel);
> +	int err;
> +
> +	if (!sharp->prepared)
> +		return 0;
> +
> +	err = mipi_dsi_dcs_set_display_off(sharp->link1);
> +	if (err < 0)
> +		dev_err(panel->dev, "failed to set display off: %d\n", err);
> +
> +	err = mipi_dsi_dcs_enter_sleep_mode(sharp->link1);
> +	if (err < 0)
> +		dev_err(panel->dev, "failed to enter sleep mode: %d\n", err);
> +
> +	msleep(120);
> +
> +	regulator_disable(sharp->supply);
> +
> +	sharp->prepared = false;
> +
> +	return 0;
> +}
> +
> +static int sharp_setup_symmetrical_split(struct mipi_dsi_device *left,
> +					 struct mipi_dsi_device *right,
> +					 const struct drm_display_mode *mode)
> +{
> +	int err;
> +
> +	err = mipi_dsi_dcs_set_column_address(left, 0, mode->hdisplay / 2 - 1);
> +	if (err < 0) {
> +		dev_err(&left->dev, "failed to set column address: %d\n", err);
> +		return err;
> +	}
> +
> +	err = mipi_dsi_dcs_set_page_address(left, 0, mode->vdisplay - 1);
> +	if (err < 0) {
> +		dev_err(&left->dev, "failed to set page address: %d\n", err);
> +		return err;
> +	}
> +
> +	err = mipi_dsi_dcs_set_column_address(right, mode->hdisplay / 2,
> +					      mode->hdisplay - 1);
> +	if (err < 0) {
> +		dev_err(&right->dev, "failed to set column address: %d\n", err);
> +		return err;
> +	}
> +
> +	err = mipi_dsi_dcs_set_page_address(right, 0, mode->vdisplay - 1);
> +	if (err < 0) {
> +		dev_err(&right->dev, "failed to set page address: %d\n", err);
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
> +static int sharp_panel_prepare(struct drm_panel *panel)
> +{
> +	struct sharp_panel *sharp = to_sharp_panel(panel);
> +	u8 format = MIPI_DCS_PIXEL_FMT_24BIT;
> +	int err;
> +
> +	if (sharp->prepared)
> +		return 0;
> +
> +	err = regulator_enable(sharp->supply);
> +	if (err < 0)
> +		return err;
> +
> +	usleep_range(10000, 20000);
> +
> +	err = mipi_dsi_dcs_soft_reset(sharp->link1);
> +	if (err < 0) {
> +		dev_err(panel->dev, "soft reset failed: %d\n", err);
> +		goto poweroff;
> +	}
> +
> +	msleep(120);
> +
> +	err = mipi_dsi_dcs_exit_sleep_mode(sharp->link1);
> +	if (err < 0) {
> +		dev_err(panel->dev, "failed to exit sleep mode: %d\n", err);
> +		goto poweroff;
> +	}
> +
> +	/*
> +	 * The MIPI DCS specification mandates this delay only between the
> +	 * exit_sleep_mode and enter_sleep_mode commands, so it isn't strictly
> +	 * necessary here.
> +	 */
> +	/*
> +	msleep(120);
> +	*/
> +
> +	/* set left-right mode */
> +	err = sharp_panel_write(sharp, 0x1000, 0x2a);
> +	if (err < 0) {
> +		dev_err(panel->dev, "failed to set left-right mode: %d\n", err);
> +		goto poweroff;
> +	}
> +
> +	/* enable command mode */
> +	err = sharp_panel_write(sharp, 0x1001, 0x01);
> +	if (err < 0) {
> +		dev_err(panel->dev, "failed to enable command mode: %d\n", err);
> +		goto poweroff;
> +	}
> +
> +	err = mipi_dsi_dcs_set_pixel_format(sharp->link1, format);
> +	if (err < 0) {
> +		dev_err(panel->dev, "failed to set pixel format: %d\n", err);
> +		goto poweroff;
> +	}
> +
> +	/*
> +	 * TODO: The device supports both left-right and even-odd split
> +	 * configurations, but this driver currently supports only the left-
> +	 * right split. To support a different mode a mechanism needs to be
> +	 * put in place to communicate the configuration back to the DSI host
> +	 * controller.
> +	 */
> +	err = sharp_setup_symmetrical_split(sharp->link1, sharp->link2,
> +					    sharp->mode);
> +	if (err < 0) {
> +		dev_err(panel->dev, "failed to set up symmetrical split: %d\n",
> +			err);
> +		goto poweroff;
> +	}
> +
> +	err = mipi_dsi_dcs_set_display_on(sharp->link1);
> +	if (err < 0) {
> +		dev_err(panel->dev, "failed to set display on: %d\n", err);
> +		goto poweroff;
> +	}
> +
> +	sharp->prepared = true;
> +
> +	return 0;
> +
> +poweroff:
> +	regulator_disable(sharp->supply);
> +	return err;
> +}
> +
> +static int sharp_panel_enable(struct drm_panel *panel)
> +{
> +	struct sharp_panel *sharp = to_sharp_panel(panel);
> +
> +	if (sharp->enabled)
> +		return 0;
> +
> +	if (sharp->backlight) {
> +		sharp->backlight->props.power = FB_BLANK_UNBLANK;
> +		backlight_update_status(sharp->backlight);
> +	}
> +
> +	sharp->enabled = true;
> +
> +	return 0;
> +}
> +
> +static const struct drm_display_mode default_mode = {
> +	.clock = 278000,
> +	.hdisplay = 2560,
> +	.hsync_start = 2560 + 128,
> +	.hsync_end = 2560 + 128 + 64,
> +	.htotal = 2560 + 128 + 64 + 64,
> +	.vdisplay = 1600,
> +	.vsync_start = 1600 + 4,
> +	.vsync_end = 1600 + 4 + 8,
> +	.vtotal = 1600 + 4 + 8 + 32,
> +	.vrefresh = 60,
> +};
> +
> +static int sharp_panel_get_modes(struct drm_panel *panel)
> +{
> +	struct drm_display_mode *mode;
> +
> +	mode = drm_mode_duplicate(panel->drm, &default_mode);
> +	if (!mode) {
> +		dev_err(panel->drm->dev, "failed to add mode %ux%ux@%u\n",
> +			default_mode.hdisplay, default_mode.vdisplay,
> +			default_mode.vrefresh);
> +		return -ENOMEM;
> +	}
> +
> +	drm_mode_set_name(mode);
> +
> +	drm_mode_probed_add(panel->connector, mode);
> +
> +	panel->connector->display_info.width_mm = 217;
> +	panel->connector->display_info.height_mm = 136;
> +
> +	return 1;
> +}
> +
> +static const struct drm_panel_funcs sharp_panel_funcs = {
> +	.disable = sharp_panel_disable,
> +	.unprepare = sharp_panel_unprepare,
> +	.prepare = sharp_panel_prepare,
> +	.enable = sharp_panel_enable,
> +	.get_modes = sharp_panel_get_modes,
> +};
> +
> +static const struct of_device_id sharp_of_match[] = {
> +	{ .compatible = "sharp,lq101r1sx01", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, sharp_of_match);
> +
> +static int sharp_panel_add(struct sharp_panel *sharp)
> +{
> +	struct device_node *np;
> +	int err;
> +
> +	sharp->mode = &default_mode;
> +
> +	sharp->supply = devm_regulator_get(&sharp->link1->dev, "power");
> +	if (IS_ERR(sharp->supply))
> +		return PTR_ERR(sharp->supply);
> +
> +	np = of_parse_phandle(sharp->link1->dev.of_node, "backlight", 0);
> +	if (np) {
> +		sharp->backlight = of_find_backlight_by_node(np);
> +		of_node_put(np);
> +
> +		if (!sharp->backlight)
> +			return -EPROBE_DEFER;
> +	}
> +
> +	drm_panel_init(&sharp->base);
> +	sharp->base.funcs = &sharp_panel_funcs;
> +	sharp->base.dev = &sharp->link1->dev;
> +
> +	err = drm_panel_add(&sharp->base);
> +	if (err < 0)
> +		goto put_backlight;
> +
> +	return 0;
> +
> +put_backlight:
> +	if (sharp->backlight)
> +		put_device(&sharp->backlight->dev);
> +
> +	return err;
> +}
> +
> +static void sharp_panel_del(struct sharp_panel *sharp)
> +{
> +	if (sharp->base.dev)
> +		drm_panel_remove(&sharp->base);
> +
> +	if (sharp->backlight)
> +		put_device(&sharp->backlight->dev);
> +
> +	if (sharp->link2)
> +		put_device(&sharp->link2->dev);
> +}
> +
> +static int sharp_panel_probe(struct mipi_dsi_device *dsi)
> +{
> +	struct mipi_dsi_device *secondary = NULL;
> +	struct sharp_panel *sharp;
> +	struct device_node *np;
> +	int err;
> +
> +	dsi->lanes = 4;
> +	dsi->format = MIPI_DSI_FMT_RGB888;
> +	dsi->mode_flags = MIPI_DSI_MODE_LPM;
> +
> +	/* Find DSI-LINK1 */
> +	np = of_parse_phandle(dsi->dev.of_node, "link2", 0);
> +	if (np) {
> +		secondary = of_find_mipi_dsi_device_by_node(np);
> +		of_node_put(np);
> +
> +		if (!secondary)
> +			return -EPROBE_DEFER;
> +	}
> +
> +	/* register a panel for only the DSI-LINK1 interface */
> +	if (secondary) {
> +		sharp = devm_kzalloc(&dsi->dev, sizeof(*sharp), GFP_KERNEL);
> +		if (!sharp) {
> +			put_device(&secondary->dev);
> +			return -ENOMEM;
> +		}
> +
> +		mipi_dsi_set_drvdata(dsi, sharp);
> +
> +		sharp->link2 = secondary;
> +		sharp->link1 = dsi;
> +
> +		err = sharp_panel_add(sharp);
> +		if (err < 0) {
> +			put_device(&secondary->dev);
> +			return err;
> +		}
> +	}
> +
> +	err = mipi_dsi_attach(dsi);
> +	if (err < 0) {
> +		if (secondary)
> +			sharp_panel_del(sharp);
> +
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
> +static int sharp_panel_remove(struct mipi_dsi_device *dsi)
> +{
> +	struct sharp_panel *sharp = mipi_dsi_get_drvdata(dsi);
> +	int err;
> +
> +	/* only detach from host for the DSI-LINK2 interface */
> +	if (!sharp) {
> +		mipi_dsi_detach(dsi);

Quotation from previous review:

>> There is no locking mechanism here, so it is possible that
>> everything can crash if someone unbind driver from LINK2 and then try to
>> enable the panel.
> 
> I don't think so. Since we're not doing anything with the DSI-LINK2
> device anymore it's irrelevant whether it is bound to the driver or
> not.
> 

Please consider following scenario:
1. panel link2 is probed
2. panel link1 is probed
3. panel link2 is unbound (for example by: echo link2
>/sys/bus/dsi/drivers/*lq101*/unbind)
4. drm is bound:
   - during sharp_setup_symmetrical_split you will call transfer
     but there is no device attached to dsi2.

Maybe it will not cause any troubles but it seems incorrect.

I guess simple workaround is to do device_lock and check if device is
bound every time you want to access link2 device.


> +		return 0;
> +	}
> +
> +	err = sharp_panel_disable(&sharp->base);
> +	if (err < 0)
> +		dev_err(&dsi->dev, "failed to disable panel: %d\n", err);
> +
> +	err = mipi_dsi_detach(dsi);
> +	if (err < 0)
> +		dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", err);
> +
> +	drm_panel_detach(&sharp->base);
> +	sharp_panel_del(sharp);

You have following flow:

sharp_probe:
	drm_panel_add
	dsi_attach

sharp_remove:
	drm_panel_disable
	dsi_detach
	drm_panel_detach
	drm_panel_del

So panel initialization is done by connector
but panel de-initialization is done by the panel itself,
on the other side connector can also disable/detach the panel.
So it seems that drm_panel resource ownership is split between
the panel and the connector. It seems racy, unless you provide
additional synchronization mechanisms.
And indeed there are races here, for example there are two processes:
- (1) connector calls sharp_prepare, in the middle of
sharp_setup_symmetrical_split process goes to sleep,
- (2) sharp_panel_remove is called due to unbind, or module removal,
sharp_disable is called,
- (1) process wakes up, it tries to continue
sharp_setup_symmetrical_split, results are unpredictable.

If you leave ownership of drm_panel to connector you can reuse
synchronization mechanisms of connector/drm inside connector. Otherwise
you will need to add another locks in drm_panel.

You can look at exynos_drm_dsi.c code, especially:
- exynos_dsi_detect,
- exynos_dsi_host_attach,
- exynos_dsi_host_detach,

and usage of dsi->panel_node and dsi->panel with drm synchronization via
drm_helper_hpd_irq_event.

Regards
Andrzej




> +
> +	return 0;
> +}
> +
> +static void sharp_panel_shutdown(struct mipi_dsi_device *dsi)
> +{
> +	struct sharp_panel *sharp = mipi_dsi_get_drvdata(dsi);
> +
> +	/* nothing to do for DSI-LINK2 */
> +	if (!sharp)
> +		return;
> +
> +	sharp_panel_disable(&sharp->base);
> +}
> +
> +static struct mipi_dsi_driver sharp_panel_driver = {
> +	.driver = {
> +		.name = "panel-sharp-lq101r1sx01",
> +		.of_match_table = sharp_of_match,
> +	},
> +	.probe = sharp_panel_probe,
> +	.remove = sharp_panel_remove,
> +	.shutdown = sharp_panel_shutdown,
> +};
> +module_mipi_dsi_driver(sharp_panel_driver);
> +
> +MODULE_AUTHOR("Thierry Reding <treding@nvidia.com>");
> +MODULE_DESCRIPTION("Sharp LQ101R1SX01 panel driver");
> +MODULE_LICENSE("GPL v2");
> 

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v4 01/16] drm/dsi: Add message to packet translator
  2014-11-03  9:13 [PATCH v4 01/16] drm/dsi: Add message to packet translator Thierry Reding
                   ` (14 preceding siblings ...)
  2014-11-03  9:13 ` [PATCH v4 16/16] drm/panel: Add Sharp LQ101R1SX01 support Thierry Reding
@ 2014-11-04 11:43 ` Andrzej Hajda
  2014-11-04 13:58   ` Thierry Reding
  15 siblings, 1 reply; 28+ messages in thread
From: Andrzej Hajda @ 2014-11-04 11:43 UTC (permalink / raw)
  To: Thierry Reding; +Cc: dri-devel

On 11/03/2014 10:13 AM, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> This commit introduces a new function, mipi_dsi_create_packet(), which
> converts from a MIPI DSI message to a MIPI DSI packet. The MIPI DSI
> packet is as close to the protocol described in the DSI specification as
> possible and useful in drivers that need to write a DSI packet into a
> FIFO to send a message off to the peripheral.
>
> Suggested-by: Andrzej Hajda <a.hajda@samsung.com>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/gpu/drm/drm_mipi_dsi.c | 45 ++++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_mipi_dsi.h     | 18 +++++++++++++++++
>  2 files changed, 63 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> index eb6dfe52cab2..76e81aba8220 100644
> --- a/drivers/gpu/drm/drm_mipi_dsi.c
> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> @@ -199,6 +199,51 @@ int mipi_dsi_detach(struct mipi_dsi_device *dsi)
>  EXPORT_SYMBOL(mipi_dsi_detach);
>  
>  /**
> + * mipi_dsi_create_packet - create a packet from a message according to the
> + *     DSI protocol
> + * @packet: pointer to a DSI packet structure
> + * @msg: message to translate into a packet
> + *
> + * Return: 0 on success or a negative error code on failure.
> + */
> +int mipi_dsi_create_packet(struct mipi_dsi_packet *packet,
> +			   const struct mipi_dsi_msg *msg)
> +{
> +	const u8 *tx = msg->tx_buf;
> +
> +	if (!packet || !msg)
> +		return -EINVAL;
> +
> +	memset(packet, 0, sizeof(*packet));
> +	packet->header[0] = ((msg->channel & 0x3) << 6) | (msg->type & 0x3f);
> +
> +	/* TODO: compute ECC if hardware support is not available */
> +
> +	/*
> +	 * Long write packets contain the word count in header bytes 1 and 2.
> +	 * The payload follows the header and is word count bytes long.
> +	 *
> +	 * Short write packets encode up to two parameters in header bytes 1
> +	 * and 2.
> +	 */
> +	if (msg->tx_len > 2) {

This is incorrect, you can have long packet of payload length 0, look for
"zero-byte Data Payload" phrase. I think you should check msg->type here.

I have used:

static bool exynos_dsi_is_short_dsi_type(u8 type)
{
    return (type & 0x0f) <= 8;
}

quite ugly, but works :)

> +		packet->header[1] = (msg->tx_len >> 0) & 0xff;
> +		packet->header[2] = (msg->tx_len >> 8) & 0xff;
> +
> +		packet->payload_length = msg->tx_len;
> +		packet->payload = tx;
> +	} else {
> +		packet->header[1] = (msg->tx_len > 0) ? tx[0] : 0;
> +		packet->header[2] = (msg->tx_len > 1) ? tx[1] : 0;
> +	}
> +
> +	packet->size = sizeof(packet->header) + packet->payload_length;

size seems to be completely useless.

> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(mipi_dsi_create_packet);
> +
> +/**
>   * mipi_dsi_dcs_write - send DCS write command
>   * @dsi: DSI device
>   * @data: pointer to the command followed by parameters
> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
> index 8569dc5a1026..663aa68826f4 100644
> --- a/include/drm/drm_mipi_dsi.h
> +++ b/include/drm/drm_mipi_dsi.h
> @@ -44,6 +44,24 @@ struct mipi_dsi_msg {
>  };
>  
>  /**
> + * struct mipi_dsi_packet - represents a MIPI DSI packet in protocol format
> + * @size: size (in bytes) of the packet
> + * @header: the four bytes that make up the header (Data ID, Word Count or
> + *     Packet Data, and ECC)
> + * @payload_length: number of bytes in the payload
> + * @payload: a pointer to a buffer containing the payload, if any
> + */
> +struct mipi_dsi_packet {
> +	size_t size;
> +	u8 header[4];

I wonder if it wouldn't be good to make it u32 or at least anonymous union:
union {
    u8 header[4];
    u32 header32;
};

And of course we should document its endiannes.
This way we can use le16_to_cpu(pkt->header32) instead of constructing
u32 value from array.

Regards
Andrzej

> +	size_t payload_length;
> +	const u8 *payload;
> +};
> +
> +int mipi_dsi_create_packet(struct mipi_dsi_packet *packet,
> +			   const struct mipi_dsi_msg *msg);
> +
> +/**
>   * struct mipi_dsi_host_ops - DSI bus operations
>   * @attach: attach DSI device to DSI host
>   * @detach: detach DSI device from DSI host

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v4 02/16] drm/dsi: Add DSI transfer helper
  2014-11-03  9:13 ` [PATCH v4 02/16] drm/dsi: Add DSI transfer helper Thierry Reding
@ 2014-11-04 11:47   ` Andrzej Hajda
  0 siblings, 0 replies; 28+ messages in thread
From: Andrzej Hajda @ 2014-11-04 11:47 UTC (permalink / raw)
  To: Thierry Reding; +Cc: dri-devel

On 11/03/2014 10:13 AM, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> A common pattern is starting to emerge for higher level transfer
> helpers. Create a new helper that encapsulates this pattern and avoids
> code duplication.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
Acked-by: Andrzej Hajda <a.hajda@samsung.com>
--
Regards
Andrzej

> ---
>  drivers/gpu/drm/drm_mipi_dsi.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> index 76e81aba8220..89a228b4eacc 100644
> --- a/drivers/gpu/drm/drm_mipi_dsi.c
> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> @@ -198,6 +198,20 @@ int mipi_dsi_detach(struct mipi_dsi_device *dsi)
>  }
>  EXPORT_SYMBOL(mipi_dsi_detach);
>  
> +static ssize_t mipi_dsi_device_transfer(struct mipi_dsi_device *dsi,
> +					struct mipi_dsi_msg *msg)
> +{
> +	const struct mipi_dsi_host_ops *ops = dsi->host->ops;
> +
> +	if (!ops || !ops->transfer)
> +		return -ENOSYS;
> +
> +	if (dsi->mode_flags & MIPI_DSI_MODE_LPM)
> +		msg->flags |= MIPI_DSI_MSG_USE_LPM;
> +
> +	return ops->transfer(dsi->host, msg);
> +}
> +
>  /**
>   * mipi_dsi_create_packet - create a packet from a message according to the
>   *     DSI protocol
> @@ -252,16 +266,12 @@ EXPORT_SYMBOL(mipi_dsi_create_packet);
>  ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, const void *data,
>  			    size_t len)
>  {
> -	const struct mipi_dsi_host_ops *ops = dsi->host->ops;
>  	struct mipi_dsi_msg msg = {
>  		.channel = dsi->channel,
>  		.tx_buf = data,
>  		.tx_len = len
>  	};
>  
> -	if (!ops || !ops->transfer)
> -		return -ENOSYS;
> -
>  	switch (len) {
>  	case 0:
>  		return -EINVAL;
> @@ -276,10 +286,7 @@ ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, const void *data,
>  		break;
>  	}
>  
> -	if (dsi->mode_flags & MIPI_DSI_MODE_LPM)
> -		msg.flags = MIPI_DSI_MSG_USE_LPM;
> -
> -	return ops->transfer(dsi->host, &msg);
> +	return mipi_dsi_device_transfer(dsi, &msg);
>  }
>  EXPORT_SYMBOL(mipi_dsi_dcs_write);
>  
> @@ -295,7 +302,6 @@ EXPORT_SYMBOL(mipi_dsi_dcs_write);
>  ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, u8 cmd, void *data,
>  			  size_t len)
>  {
> -	const struct mipi_dsi_host_ops *ops = dsi->host->ops;
>  	struct mipi_dsi_msg msg = {
>  		.channel = dsi->channel,
>  		.type = MIPI_DSI_DCS_READ,
> @@ -305,13 +311,7 @@ ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, u8 cmd, void *data,
>  		.rx_len = len
>  	};
>  
> -	if (!ops || !ops->transfer)
> -		return -ENOSYS;
> -
> -	if (dsi->mode_flags & MIPI_DSI_MODE_LPM)
> -		msg.flags = MIPI_DSI_MSG_USE_LPM;
> -
> -	return ops->transfer(dsi->host, &msg);
> +	return mipi_dsi_device_transfer(dsi, &msg);
>  }
>  EXPORT_SYMBOL(mipi_dsi_dcs_read);
>  

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v4 16/16] drm/panel: Add Sharp LQ101R1SX01 support
  2014-11-04 10:41   ` Andrzej Hajda
@ 2014-11-04 13:29     ` Thierry Reding
  2014-11-05 14:39       ` Andrzej Hajda
  0 siblings, 1 reply; 28+ messages in thread
From: Thierry Reding @ 2014-11-04 13:29 UTC (permalink / raw)
  To: Andrzej Hajda; +Cc: dri-devel


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

On Tue, Nov 04, 2014 at 11:41:15AM +0100, Andrzej Hajda wrote:
> On 11/03/2014 10:13 AM, Thierry Reding wrote:
[...]
> > diff --git a/Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt b/Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt
[...]
> > +Example:
> > +
> > +	dsi@54300000 {
> > +		panel: panel@0 {
> > +			compatible = "sharp,lq101r1sx01";
> > +			reg = <0>;
> > +
> > +			link2 = <&secondary>;
> > +
> > +			power-supply = <...>;
> > +			backlight = <...>;
> > +		};
> > +	};
> > +
> > +	dsi@54400000 {
> > +		secondary: panel@0 {
> > +			compatible = "sharp,lq101r1sx01";
> > +			reg = <0>;
> > +		};
> > +	};
> 
> The bindings above and the implementation below clearly shows
> that the secondary node is just a dummy dsi device.

It's not only a dummy device. Binding it to the device's compatible
string allows the driver to properly set up the DSI device (flags,
number of lanes, ...).

> Hiding this behind conditionals in both docs and code does not look good
> to me. On the other side having common dummy dsi driver
> would allow to reuse it in other dual-dsi devices.
> So instead of 2nd node you would have:
> 	dsi@54400000 {
> 		secondary: panel@0 {
> 			compatible = "dsi-dummy-whatever";
> 			reg = <0>;
> 		};
> 	};
> 
> Or just:
> 	dsi2: dsi@54400000 {
> 	}
> 
> with phandle to dsi2 in 1st node, in such case 2nd dsi dev would be
> created dynamically like with i2c_new_dummy.

I don't think that's technically valid DT. Every device must have a
compatible property. And the compatible property should have an entry
for the specific model of the device. "dummy" doesn't qualify for that

Hopefully when more dual-channel DSI devices get supported some kind of
pattern will emerge. For now I'll go with what I have in this patch. It
is as accurate as I can think of.

> > diff --git a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
[...]
> > +struct sharp_panel {
> > +	struct drm_panel base;
> > +	/* the datasheet refers to them as DSI-LINK1 and DSI-LINK2 */
> > +	struct mipi_dsi_device *link1;
> > +	struct mipi_dsi_device *link2;
> > +
> > +	struct backlight_device *backlight;
> > +	struct regulator *supply;
> > +
> > +	bool prepared;
> > +	bool enabled;
> 
> Nitpick.
> As I wrote in review to previous version it is up to panel client
> (usually connector) to call panel callbacks properly, we should not add
> additional checks to panels. If call order is incorrect then the client
> should be fixed.

There are no such guarantees today. But hopefully this will change with
atomic modesetting.

> > +static int sharp_panel_remove(struct mipi_dsi_device *dsi)
> > +{
> > +	struct sharp_panel *sharp = mipi_dsi_get_drvdata(dsi);
> > +	int err;
> > +
> > +	/* only detach from host for the DSI-LINK2 interface */
> > +	if (!sharp) {
> > +		mipi_dsi_detach(dsi);
> 
> Quotation from previous review:
> 
> >> There is no locking mechanism here, so it is possible that
> >> everything can crash if someone unbind driver from LINK2 and then try to
> >> enable the panel.
> > 
> > I don't think so. Since we're not doing anything with the DSI-LINK2
> > device anymore it's irrelevant whether it is bound to the driver or
> > not.
> > 
> 
> Please consider following scenario:
> 1. panel link2 is probed
> 2. panel link1 is probed
> 3. panel link2 is unbound (for example by: echo link2
> >/sys/bus/dsi/drivers/*lq101*/unbind)
> 4. drm is bound:
>    - during sharp_setup_symmetrical_split you will call transfer
>      but there is no device attached to dsi2.
> 
> Maybe it will not cause any troubles but it seems incorrect.

The device is still there. The driver may not be bound to it, but we
don't need that for the transfers anyway.

> I guess simple workaround is to do device_lock and check if device is
> bound every time you want to access link2 device.

I don't see where the problem is. We do keep a reference to the LINK2
device from the first, so it can't just disappear. The only way it could
disappear is if the host controller that provides its bus goes away, but
there's code at the DSI host controller level to deal with that.

> > +		return 0;
> > +	}
> > +
> > +	err = sharp_panel_disable(&sharp->base);
> > +	if (err < 0)
> > +		dev_err(&dsi->dev, "failed to disable panel: %d\n", err);
> > +
> > +	err = mipi_dsi_detach(dsi);
> > +	if (err < 0)
> > +		dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", err);
> > +
> > +	drm_panel_detach(&sharp->base);
> > +	sharp_panel_del(sharp);
> 
> You have following flow:
> 
> sharp_probe:
> 	drm_panel_add
> 	dsi_attach
> 
> sharp_remove:
> 	drm_panel_disable
> 	dsi_detach
> 	drm_panel_detach
> 	drm_panel_del
> 
> So panel initialization is done by connector
> but panel de-initialization is done by the panel itself,

No. The panel registers with the panel registry so that the connector
can find it. But when the panel registers it doesn't know anything about
the DRM device it is going to be attached to, so we have to attach it to
the connector only when that becomes available.

When the driver is unloaded, the panel unregisters from the registry.
Also it needs to detach from a connector if it's still connected so that
the connector can be marked unconnected. Similarly when the connector
goes away it must let the panel know that it's being detached.

But it looks like this infrastructure is generally somewhat immature.
For example this only works relatively well for DSI panels because we
have the DSI peripheral attach/detach callbacks. For RGB or LVDS panels
we have no such thing, so there's no way currently for these panels to
hotplug.

> on the other side connector can also disable/detach the panel.
> So it seems that drm_panel resource ownership is split between
> the panel and the connector. It seems racy, unless you provide
> additional synchronization mechanisms.
> And indeed there are races here, for example there are two processes:
> - (1) connector calls sharp_prepare, in the middle of
> sharp_setup_symmetrical_split process goes to sleep,
> - (2) sharp_panel_remove is called due to unbind, or module removal,
> sharp_disable is called,
> - (1) process wakes up, it tries to continue
> sharp_setup_symmetrical_split, results are unpredictable.

That's nothing you can solve by simply changing the calling order here.
DRM panel is completely missing any infrastructure to allow you to
safely deal with that kind of situation.

But it's something that I've been trying to fix for the past couple of
days.

> If you leave ownership of drm_panel to connector you can reuse
> synchronization mechanisms of connector/drm inside connector. Otherwise
> you will need to add another locks in drm_panel.
> 
> You can look at exynos_drm_dsi.c code, especially:
> - exynos_dsi_detect,
> - exynos_dsi_host_attach,
> - exynos_dsi_host_detach,
> 
> and usage of dsi->panel_node and dsi->panel with drm synchronization via
> drm_helper_hpd_irq_event.

There are a number of ways that one could possibly implement this. I do
not think any of the existing ones is as good as it could be and I think
we really ought to standardize on how to do this to make it easier for
other drivers to adapt DRM panels (and bridges for that matter).

Unfortunately before we have a good plan on how to handle panels more
uniformly I don't see how we can possibly make everybody happy. It seems
like currently whatever panel is written for Exynos isn't going to work
on Tegra and vice-versa. So how about we merge this series and I'll look
into how we can unify things?

I'd appreciate any ideas on how such a unification could look.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v4 13/16] drm/dsi: Implement DCS {get,set}_pixel_format commands
  2014-11-03 17:10   ` Sean Paul
@ 2014-11-04 13:34     ` Thierry Reding
  0 siblings, 0 replies; 28+ messages in thread
From: Thierry Reding @ 2014-11-04 13:34 UTC (permalink / raw)
  To: Sean Paul; +Cc: Andrzej Hajda, dri-devel


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

On Mon, Nov 03, 2014 at 12:10:29PM -0500, Sean Paul wrote:
> On Mon, Nov 3, 2014 at 4:13 AM, Thierry Reding <thierry.reding@gmail.com> wrote:
> > From: Thierry Reding <treding@nvidia.com>
> >
> > Provide small convenience wrappers to query or set the pixel format used
> > by the interface.
> >
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >  drivers/gpu/drm/drm_mipi_dsi.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_mipi_dsi.h     |  2 ++
> >  2 files changed, 44 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> > index da34469b9b98..226822a44457 100644
> > --- a/drivers/gpu/drm/drm_mipi_dsi.c
> > +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> > @@ -553,6 +553,27 @@ int mipi_dsi_dcs_get_power_mode(struct mipi_dsi_device *dsi, u8 *mode)
> >  EXPORT_SYMBOL(mipi_dsi_dcs_get_power_mode);
> >
> >  /**
> > + * mipi_dsi_dcs_get_pixel_format() - gets the pixel format for the RGB image
> > + *    data used by the interface
> > + * @dsi: DSI peripheral device
> > + * @format: return location for the pixel format
> > + *
> > + * Return: 0 on success or a negative error code on failure.
> > + */
> > +int mipi_dsi_dcs_get_pixel_format(struct mipi_dsi_device *dsi, u8 *format)
> > +{
> > +       ssize_t err;
> > +
> > +       err = mipi_dsi_dcs_read(dsi, MIPI_DCS_GET_PIXEL_FORMAT, format,
> > +                               sizeof(*format));
> > +       if (err < 0)
> > +               return err;
> 
> This should probably return -ENODATA when err != sizeof(*format), like
> the previous patch did.

Done.

> It might be worth moving that logic into mipi_dsi_dcs_read, assuming
> we're not interested in partial data.

Agreed. I think we can leave that for a subsequent refactoring once
these function have matured a bit more and patterns appear.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v4 01/16] drm/dsi: Add message to packet translator
  2014-11-04 11:43 ` [PATCH v4 01/16] drm/dsi: Add message to packet translator Andrzej Hajda
@ 2014-11-04 13:58   ` Thierry Reding
  2014-11-04 14:21     ` Andrzej Hajda
  0 siblings, 1 reply; 28+ messages in thread
From: Thierry Reding @ 2014-11-04 13:58 UTC (permalink / raw)
  To: Andrzej Hajda; +Cc: dri-devel


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

On Tue, Nov 04, 2014 at 12:43:21PM +0100, Andrzej Hajda wrote:
> On 11/03/2014 10:13 AM, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> >
> > This commit introduces a new function, mipi_dsi_create_packet(), which
> > converts from a MIPI DSI message to a MIPI DSI packet. The MIPI DSI
> > packet is as close to the protocol described in the DSI specification as
> > possible and useful in drivers that need to write a DSI packet into a
> > FIFO to send a message off to the peripheral.
> >
> > Suggested-by: Andrzej Hajda <a.hajda@samsung.com>
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >  drivers/gpu/drm/drm_mipi_dsi.c | 45 ++++++++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_mipi_dsi.h     | 18 +++++++++++++++++
> >  2 files changed, 63 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> > index eb6dfe52cab2..76e81aba8220 100644
> > --- a/drivers/gpu/drm/drm_mipi_dsi.c
> > +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> > @@ -199,6 +199,51 @@ int mipi_dsi_detach(struct mipi_dsi_device *dsi)
> >  EXPORT_SYMBOL(mipi_dsi_detach);
> >  
> >  /**
> > + * mipi_dsi_create_packet - create a packet from a message according to the
> > + *     DSI protocol
> > + * @packet: pointer to a DSI packet structure
> > + * @msg: message to translate into a packet
> > + *
> > + * Return: 0 on success or a negative error code on failure.
> > + */
> > +int mipi_dsi_create_packet(struct mipi_dsi_packet *packet,
> > +			   const struct mipi_dsi_msg *msg)
> > +{
> > +	const u8 *tx = msg->tx_buf;
> > +
> > +	if (!packet || !msg)
> > +		return -EINVAL;
> > +
> > +	memset(packet, 0, sizeof(*packet));
> > +	packet->header[0] = ((msg->channel & 0x3) << 6) | (msg->type & 0x3f);
> > +
> > +	/* TODO: compute ECC if hardware support is not available */
> > +
> > +	/*
> > +	 * Long write packets contain the word count in header bytes 1 and 2.
> > +	 * The payload follows the header and is word count bytes long.
> > +	 *
> > +	 * Short write packets encode up to two parameters in header bytes 1
> > +	 * and 2.
> > +	 */
> > +	if (msg->tx_len > 2) {
> 
> This is incorrect, you can have long packet of payload length 0, look for
> "zero-byte Data Payload" phrase. I think you should check msg->type here.
> 
> I have used:
> 
> static bool exynos_dsi_is_short_dsi_type(u8 type)
> {
>     return (type & 0x0f) <= 8;
> }
> 
> quite ugly, but works :)

That would falsely return true for unspecified data types, too. I'll go
with a variant that uses an explicit switch.

> > +		packet->header[1] = (msg->tx_len >> 0) & 0xff;
> > +		packet->header[2] = (msg->tx_len >> 8) & 0xff;
> > +
> > +		packet->payload_length = msg->tx_len;
> > +		packet->payload = tx;
> > +	} else {
> > +		packet->header[1] = (msg->tx_len > 0) ? tx[0] : 0;
> > +		packet->header[2] = (msg->tx_len > 1) ? tx[1] : 0;
> > +	}
> > +
> > +	packet->size = sizeof(packet->header) + packet->payload_length;
> 
> size seems to be completely useless.

It's not. Tegra has two FIFOs that can be selected depending on the size
of a transfer. I use this field to detect which FIFO needs to be
selected.

> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(mipi_dsi_create_packet);
> > +
> > +/**
> >   * mipi_dsi_dcs_write - send DCS write command
> >   * @dsi: DSI device
> >   * @data: pointer to the command followed by parameters
> > diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
> > index 8569dc5a1026..663aa68826f4 100644
> > --- a/include/drm/drm_mipi_dsi.h
> > +++ b/include/drm/drm_mipi_dsi.h
> > @@ -44,6 +44,24 @@ struct mipi_dsi_msg {
> >  };
> >  
> >  /**
> > + * struct mipi_dsi_packet - represents a MIPI DSI packet in protocol format
> > + * @size: size (in bytes) of the packet
> > + * @header: the four bytes that make up the header (Data ID, Word Count or
> > + *     Packet Data, and ECC)
> > + * @payload_length: number of bytes in the payload
> > + * @payload: a pointer to a buffer containing the payload, if any
> > + */
> > +struct mipi_dsi_packet {
> > +	size_t size;
> > +	u8 header[4];
> 
> I wonder if it wouldn't be good to make it u32 or at least anonymous union:
> union {
>     u8 header[4];
>     u32 header32;
> };

I'm not sure this is very useful. It's pretty trivial how you
concatenate the individual bytes and it actually remove any ambiguity
about the endianness.

> And of course we should document its endiannes.

The endianness is already documented in the kerneldoc, isn't it? Data ID
followed by Word Count (long packets) or Packet Data (short packets) and
finally the ECC byte. That's the ordering defined in the specification,
so I think it's fairly obvious.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v4 01/16] drm/dsi: Add message to packet translator
  2014-11-04 13:58   ` Thierry Reding
@ 2014-11-04 14:21     ` Andrzej Hajda
  2014-11-04 14:39       ` Thierry Reding
  0 siblings, 1 reply; 28+ messages in thread
From: Andrzej Hajda @ 2014-11-04 14:21 UTC (permalink / raw)
  To: Thierry Reding; +Cc: dri-devel

On 11/04/2014 02:58 PM, Thierry Reding wrote:
> On Tue, Nov 04, 2014 at 12:43:21PM +0100, Andrzej Hajda wrote:
>> On 11/03/2014 10:13 AM, Thierry Reding wrote:
>>> From: Thierry Reding <treding@nvidia.com>
>>>
>>> This commit introduces a new function, mipi_dsi_create_packet(), which
>>> converts from a MIPI DSI message to a MIPI DSI packet. The MIPI DSI
>>> packet is as close to the protocol described in the DSI specification as
>>> possible and useful in drivers that need to write a DSI packet into a
>>> FIFO to send a message off to the peripheral.
>>>
>>> Suggested-by: Andrzej Hajda <a.hajda@samsung.com>
>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>> ---
>>>  drivers/gpu/drm/drm_mipi_dsi.c | 45 ++++++++++++++++++++++++++++++++++++++++++
>>>  include/drm/drm_mipi_dsi.h     | 18 +++++++++++++++++
>>>  2 files changed, 63 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
>>> index eb6dfe52cab2..76e81aba8220 100644
>>> --- a/drivers/gpu/drm/drm_mipi_dsi.c
>>> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
>>> @@ -199,6 +199,51 @@ int mipi_dsi_detach(struct mipi_dsi_device *dsi)
>>>  EXPORT_SYMBOL(mipi_dsi_detach);
>>>  
>>>  /**
>>> + * mipi_dsi_create_packet - create a packet from a message according to the
>>> + *     DSI protocol
>>> + * @packet: pointer to a DSI packet structure
>>> + * @msg: message to translate into a packet
>>> + *
>>> + * Return: 0 on success or a negative error code on failure.
>>> + */
>>> +int mipi_dsi_create_packet(struct mipi_dsi_packet *packet,
>>> +			   const struct mipi_dsi_msg *msg)
>>> +{
>>> +	const u8 *tx = msg->tx_buf;
>>> +
>>> +	if (!packet || !msg)
>>> +		return -EINVAL;
>>> +
>>> +	memset(packet, 0, sizeof(*packet));
>>> +	packet->header[0] = ((msg->channel & 0x3) << 6) | (msg->type & 0x3f);
>>> +
>>> +	/* TODO: compute ECC if hardware support is not available */
>>> +
>>> +	/*
>>> +	 * Long write packets contain the word count in header bytes 1 and 2.
>>> +	 * The payload follows the header and is word count bytes long.
>>> +	 *
>>> +	 * Short write packets encode up to two parameters in header bytes 1
>>> +	 * and 2.
>>> +	 */
>>> +	if (msg->tx_len > 2) {
>> This is incorrect, you can have long packet of payload length 0, look for
>> "zero-byte Data Payload" phrase. I think you should check msg->type here.
>>
>> I have used:
>>
>> static bool exynos_dsi_is_short_dsi_type(u8 type)
>> {
>>     return (type & 0x0f) <= 8;
>> }
>>
>> quite ugly, but works :)
> That would falsely return true for unspecified data types, too. I'll go
> with a variant that uses an explicit switch.

Sounds better.

>
>>> +		packet->header[1] = (msg->tx_len >> 0) & 0xff;
>>> +		packet->header[2] = (msg->tx_len >> 8) & 0xff;
>>> +
>>> +		packet->payload_length = msg->tx_len;
>>> +		packet->payload = tx;
>>> +	} else {
>>> +		packet->header[1] = (msg->tx_len > 0) ? tx[0] : 0;
>>> +		packet->header[2] = (msg->tx_len > 1) ? tx[1] : 0;
>>> +	}
>>> +
>>> +	packet->size = sizeof(packet->header) + packet->payload_length;
>> size seems to be completely useless.
> It's not. Tegra has two FIFOs that can be selected depending on the size
> of a transfer. I use this field to detect which FIFO needs to be
> selected.

But size is always equal payload_length + 4, so it does not carry any
additional information.

>
>>> +
>>> +	return 0;
>>> +}
>>> +EXPORT_SYMBOL(mipi_dsi_create_packet);
>>> +
>>> +/**
>>>   * mipi_dsi_dcs_write - send DCS write command
>>>   * @dsi: DSI device
>>>   * @data: pointer to the command followed by parameters
>>> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
>>> index 8569dc5a1026..663aa68826f4 100644
>>> --- a/include/drm/drm_mipi_dsi.h
>>> +++ b/include/drm/drm_mipi_dsi.h
>>> @@ -44,6 +44,24 @@ struct mipi_dsi_msg {
>>>  };
>>>  
>>>  /**
>>> + * struct mipi_dsi_packet - represents a MIPI DSI packet in protocol format
>>> + * @size: size (in bytes) of the packet
>>> + * @header: the four bytes that make up the header (Data ID, Word Count or
>>> + *     Packet Data, and ECC)
>>> + * @payload_length: number of bytes in the payload
>>> + * @payload: a pointer to a buffer containing the payload, if any
>>> + */
>>> +struct mipi_dsi_packet {
>>> +	size_t size;
>>> +	u8 header[4];
>> I wonder if it wouldn't be good to make it u32 or at least anonymous union:
>> union {
>>     u8 header[4];
>>     u32 header32;
>> };
> I'm not sure this is very useful. It's pretty trivial how you
> concatenate the individual bytes and it actually remove any ambiguity
> about the endianness.

This looks better:

val = le16_to_cpu(pkt->header32);
writel(val, REG);

than this:

val = pkt->header[2] << 16 | pkt->header[1] << 8 | pkt->header[0];
writel(val, REG);

But it is just a nitpick.

Regards
Andrzej

>
>> And of course we should document its endiannes.
> The endianness is already documented in the kerneldoc, isn't it? Data ID
> followed by Word Count (long packets) or Packet Data (short packets) and
> finally the ECC byte. That's the ordering defined in the specification,
> so I think it's fairly obvious.
>
> Thierry

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v4 01/16] drm/dsi: Add message to packet translator
  2014-11-04 14:21     ` Andrzej Hajda
@ 2014-11-04 14:39       ` Thierry Reding
  2014-11-05 13:35         ` Andrzej Hajda
  0 siblings, 1 reply; 28+ messages in thread
From: Thierry Reding @ 2014-11-04 14:39 UTC (permalink / raw)
  To: Andrzej Hajda; +Cc: dri-devel


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

On Tue, Nov 04, 2014 at 03:21:14PM +0100, Andrzej Hajda wrote:
> On 11/04/2014 02:58 PM, Thierry Reding wrote:
> > On Tue, Nov 04, 2014 at 12:43:21PM +0100, Andrzej Hajda wrote:
> >> On 11/03/2014 10:13 AM, Thierry Reding wrote:
[...]
> >>> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
[...]
> >>> +		packet->header[1] = (msg->tx_len >> 0) & 0xff;
> >>> +		packet->header[2] = (msg->tx_len >> 8) & 0xff;
> >>> +
> >>> +		packet->payload_length = msg->tx_len;
> >>> +		packet->payload = tx;
> >>> +	} else {
> >>> +		packet->header[1] = (msg->tx_len > 0) ? tx[0] : 0;
> >>> +		packet->header[2] = (msg->tx_len > 1) ? tx[1] : 0;
> >>> +	}
> >>> +
> >>> +	packet->size = sizeof(packet->header) + packet->payload_length;
> >> size seems to be completely useless.
> > It's not. Tegra has two FIFOs that can be selected depending on the size
> > of a transfer. I use this field to detect which FIFO needs to be
> > selected.
> 
> But size is always equal payload_length + 4, so it does not carry any
> additional information.

Right, but it's out of convenience to prevent every driver from doing
this again. It's part of the help that the helper provides. =)

> >>> +
> >>> +	return 0;
> >>> +}
> >>> +EXPORT_SYMBOL(mipi_dsi_create_packet);
> >>> +
> >>> +/**
> >>>   * mipi_dsi_dcs_write - send DCS write command
> >>>   * @dsi: DSI device
> >>>   * @data: pointer to the command followed by parameters
> >>> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
> >>> index 8569dc5a1026..663aa68826f4 100644
> >>> --- a/include/drm/drm_mipi_dsi.h
> >>> +++ b/include/drm/drm_mipi_dsi.h
> >>> @@ -44,6 +44,24 @@ struct mipi_dsi_msg {
> >>>  };
> >>>  
> >>>  /**
> >>> + * struct mipi_dsi_packet - represents a MIPI DSI packet in protocol format
> >>> + * @size: size (in bytes) of the packet
> >>> + * @header: the four bytes that make up the header (Data ID, Word Count or
> >>> + *     Packet Data, and ECC)
> >>> + * @payload_length: number of bytes in the payload
> >>> + * @payload: a pointer to a buffer containing the payload, if any
> >>> + */
> >>> +struct mipi_dsi_packet {
> >>> +	size_t size;
> >>> +	u8 header[4];
> >> I wonder if it wouldn't be good to make it u32 or at least anonymous union:
> >> union {
> >>     u8 header[4];
> >>     u32 header32;
> >> };
> > I'm not sure this is very useful. It's pretty trivial how you
> > concatenate the individual bytes and it actually remove any ambiguity
> > about the endianness.
> 
> This looks better:
> 
> val = le16_to_cpu(pkt->header32);
> writel(val, REG);
> 
> than this:
> 
> val = pkt->header[2] << 16 | pkt->header[1] << 8 | pkt->header[0];
> writel(val, REG);

I disagree. =) Having the individual bytes makes their ordering very
explicit.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v4 01/16] drm/dsi: Add message to packet translator
  2014-11-04 14:39       ` Thierry Reding
@ 2014-11-05 13:35         ` Andrzej Hajda
  0 siblings, 0 replies; 28+ messages in thread
From: Andrzej Hajda @ 2014-11-05 13:35 UTC (permalink / raw)
  To: Thierry Reding; +Cc: dri-devel

On 11/04/2014 03:39 PM, Thierry Reding wrote:
> On Tue, Nov 04, 2014 at 03:21:14PM +0100, Andrzej Hajda wrote:
>> On 11/04/2014 02:58 PM, Thierry Reding wrote:
>>> On Tue, Nov 04, 2014 at 12:43:21PM +0100, Andrzej Hajda wrote:
>>>> On 11/03/2014 10:13 AM, Thierry Reding wrote:
> [...]
>>>>> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> [...]
>>>>> +		packet->header[1] = (msg->tx_len >> 0) & 0xff;
>>>>> +		packet->header[2] = (msg->tx_len >> 8) & 0xff;
>>>>> +
>>>>> +		packet->payload_length = msg->tx_len;
>>>>> +		packet->payload = tx;
>>>>> +	} else {
>>>>> +		packet->header[1] = (msg->tx_len > 0) ? tx[0] : 0;
>>>>> +		packet->header[2] = (msg->tx_len > 1) ? tx[1] : 0;
>>>>> +	}
>>>>> +
>>>>> +	packet->size = sizeof(packet->header) + packet->payload_length;
>>>> size seems to be completely useless.
>>> It's not. Tegra has two FIFOs that can be selected depending on the size
>>> of a transfer. I use this field to detect which FIFO needs to be
>>> selected.
>> But size is always equal payload_length + 4, so it does not carry any
>> additional information.
> Right, but it's out of convenience to prevent every driver from doing
> this again. It's part of the help that the helper provides. =)
>
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +EXPORT_SYMBOL(mipi_dsi_create_packet);
>>>>> +
>>>>> +/**
>>>>>   * mipi_dsi_dcs_write - send DCS write command
>>>>>   * @dsi: DSI device
>>>>>   * @data: pointer to the command followed by parameters
>>>>> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
>>>>> index 8569dc5a1026..663aa68826f4 100644
>>>>> --- a/include/drm/drm_mipi_dsi.h
>>>>> +++ b/include/drm/drm_mipi_dsi.h
>>>>> @@ -44,6 +44,24 @@ struct mipi_dsi_msg {
>>>>>  };
>>>>>  
>>>>>  /**
>>>>> + * struct mipi_dsi_packet - represents a MIPI DSI packet in protocol format
>>>>> + * @size: size (in bytes) of the packet
>>>>> + * @header: the four bytes that make up the header (Data ID, Word Count or
>>>>> + *     Packet Data, and ECC)
>>>>> + * @payload_length: number of bytes in the payload
>>>>> + * @payload: a pointer to a buffer containing the payload, if any
>>>>> + */
>>>>> +struct mipi_dsi_packet {
>>>>> +	size_t size;
>>>>> +	u8 header[4];
>>>> I wonder if it wouldn't be good to make it u32 or at least anonymous union:
>>>> union {
>>>>     u8 header[4];
>>>>     u32 header32;
>>>> };
>>> I'm not sure this is very useful. It's pretty trivial how you
>>> concatenate the individual bytes and it actually remove any ambiguity
>>> about the endianness.
>> This looks better:
>>
>> val = le16_to_cpu(pkt->header32);
>> writel(val, REG);
>>
>> than this:
>>
>> val = pkt->header[2] << 16 | pkt->header[1] << 8 | pkt->header[0];
>> writel(val, REG);
> I disagree. =) Having the individual bytes makes their ordering very
> explicit.
>

Wow, you want to keep size field to prevent drivers from adding
sometimes 4 to payload,
but you do not want to simplify header calculation that is much more
complicated :)

Regards
Andrzej




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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v4 16/16] drm/panel: Add Sharp LQ101R1SX01 support
  2014-11-04 13:29     ` Thierry Reding
@ 2014-11-05 14:39       ` Andrzej Hajda
  0 siblings, 0 replies; 28+ messages in thread
From: Andrzej Hajda @ 2014-11-05 14:39 UTC (permalink / raw)
  To: Thierry Reding; +Cc: dri-devel

On 11/04/2014 02:29 PM, Thierry Reding wrote:
> On Tue, Nov 04, 2014 at 11:41:15AM +0100, Andrzej Hajda wrote:
>> On 11/03/2014 10:13 AM, Thierry Reding wrote:
> [...]
>>> diff --git a/Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt b/Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt
> [...]
>>> +Example:
>>> +
>>> +	dsi@54300000 {
>>> +		panel: panel@0 {
>>> +			compatible = "sharp,lq101r1sx01";
>>> +			reg = <0>;
>>> +
>>> +			link2 = <&secondary>;
>>> +
>>> +			power-supply = <...>;
>>> +			backlight = <...>;
>>> +		};
>>> +	};
>>> +
>>> +	dsi@54400000 {
>>> +		secondary: panel@0 {
>>> +			compatible = "sharp,lq101r1sx01";
>>> +			reg = <0>;
>>> +		};
>>> +	};
>>
>> The bindings above and the implementation below clearly shows
>> that the secondary node is just a dummy dsi device.
> 
> It's not only a dummy device. Binding it to the device's compatible
> string allows the driver to properly set up the DSI device (flags,
> number of lanes, ...).

This is why I called it dummy DSI device :)

> 
>> Hiding this behind conditionals in both docs and code does not look good
>> to me. On the other side having common dummy dsi driver
>> would allow to reuse it in other dual-dsi devices.
>> So instead of 2nd node you would have:
>> 	dsi@54400000 {
>> 		secondary: panel@0 {
>> 			compatible = "dsi-dummy-whatever";
>> 			reg = <0>;
>> 		};
>> 	};
>>
>> Or just:
>> 	dsi2: dsi@54400000 {
>> 	}
>>
>> with phandle to dsi2 in 1st node, in such case 2nd dsi dev would be
>> created dynamically like with i2c_new_dummy.
> 
> I don't think that's technically valid DT. Every device must have a
> compatible property. And the compatible property should have an entry
> for the specific model of the device. "dummy" doesn't qualify for that

One could ask if it is technically valid to represent one hw device via
two separate nodes. Each choice scarifies something.

> 
> Hopefully when more dual-channel DSI devices get supported some kind of
> pattern will emerge. For now I'll go with what I have in this patch. It
> is as accurate as I can think of.

Agreed


> 
>>> diff --git a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
> [...]
>>> +struct sharp_panel {
>>> +	struct drm_panel base;
>>> +	/* the datasheet refers to them as DSI-LINK1 and DSI-LINK2 */
>>> +	struct mipi_dsi_device *link1;
>>> +	struct mipi_dsi_device *link2;
>>> +
>>> +	struct backlight_device *backlight;
>>> +	struct regulator *supply;
>>> +
>>> +	bool prepared;
>>> +	bool enabled;
>>
>> Nitpick.
>> As I wrote in review to previous version it is up to panel client
>> (usually connector) to call panel callbacks properly, we should not add
>> additional checks to panels. If call order is incorrect then the client
>> should be fixed.
> 
> There are no such guarantees today. But hopefully this will change with
> atomic modesetting.

I do not understand that. It is the connector code who calls these
callbacks. Why cannot you force it to call them in proper order?

> 
>>> +static int sharp_panel_remove(struct mipi_dsi_device *dsi)
>>> +{
>>> +	struct sharp_panel *sharp = mipi_dsi_get_drvdata(dsi);
>>> +	int err;
>>> +
>>> +	/* only detach from host for the DSI-LINK2 interface */
>>> +	if (!sharp) {
>>> +		mipi_dsi_detach(dsi);
>>
>> Quotation from previous review:
>>
>>>> There is no locking mechanism here, so it is possible that
>>>> everything can crash if someone unbind driver from LINK2 and then try to
>>>> enable the panel.
>>>
>>> I don't think so. Since we're not doing anything with the DSI-LINK2
>>> device anymore it's irrelevant whether it is bound to the driver or
>>> not.
>>>
>>
>> Please consider following scenario:
>> 1. panel link2 is probed
>> 2. panel link1 is probed
>> 3. panel link2 is unbound (for example by: echo link2
>>> /sys/bus/dsi/drivers/*lq101*/unbind)
>> 4. drm is bound:
>>    - during sharp_setup_symmetrical_split you will call transfer
>>      but there is no device attached to dsi2.
>>
>> Maybe it will not cause any troubles but it seems incorrect.
> 
> The device is still there. The driver may not be bound to it, but we
> don't need that for the transfers anyway.

And you do not want to call it dummy device :)

> 
>> I guess simple workaround is to do device_lock and check if device is
>> bound every time you want to access link2 device.
> 
> I don't see where the problem is. We do keep a reference to the LINK2
> device from the first, so it can't just disappear. The only way it could
> disappear is if the host controller that provides its bus goes away, but
> there's code at the DSI host controller level to deal with that.
> 

As I understand it is important to you that host is set-up according to
settings passed during LINK2 device attachment.
IMHO assumption that the host will keep these settings after device is
detached is incorrect. The host can return to its initial state or any
other state. I think that it is valid if the host refuses transfers on
'non-attached' channels.


>>> +		return 0;
>>> +	}
>>> +
>>> +	err = sharp_panel_disable(&sharp->base);
>>> +	if (err < 0)
>>> +		dev_err(&dsi->dev, "failed to disable panel: %d\n", err);
>>> +
>>> +	err = mipi_dsi_detach(dsi);
>>> +	if (err < 0)
>>> +		dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", err);
>>> +
>>> +	drm_panel_detach(&sharp->base);
>>> +	sharp_panel_del(sharp);
>>
>> You have following flow:
>>
>> sharp_probe:
>> 	drm_panel_add
>> 	dsi_attach
>>
>> sharp_remove:
>> 	drm_panel_disable
>> 	dsi_detach
>> 	drm_panel_detach
>> 	drm_panel_del
>>
>> So panel initialization is done by connector
>> but panel de-initialization is done by the panel itself,
> 
> No. The panel registers with the panel registry so that the connector
> can find it. But when the panel registers it doesn't know anything about
> the DRM device it is going to be attached to, so we have to attach it to
> the connector only when that becomes available.
> 
> When the driver is unloaded, the panel unregisters from the registry.
> Also it needs to detach from a connector if it's still connected so that
> the connector can be marked unconnected. Similarly when the connector
> goes away it must let the panel know that it's being detached.
> 
> But it looks like this infrastructure is generally somewhat immature.
> For example this only works relatively well for DSI panels because we
> have the DSI peripheral attach/detach callbacks. For RGB or LVDS panels
> we have no such thing, so there's no way currently for these panels to
> hotplug.

This is why I have proposed interface_tracker framework, we would have
hotplug/hotunplug callbacks already :)

> 
>> on the other side connector can also disable/detach the panel.
>> So it seems that drm_panel resource ownership is split between
>> the panel and the connector. It seems racy, unless you provide
>> additional synchronization mechanisms.
>> And indeed there are races here, for example there are two processes:
>> - (1) connector calls sharp_prepare, in the middle of
>> sharp_setup_symmetrical_split process goes to sleep,
>> - (2) sharp_panel_remove is called due to unbind, or module removal,
>> sharp_disable is called,
>> - (1) process wakes up, it tries to continue
>> sharp_setup_symmetrical_split, results are unpredictable.
> 
> That's nothing you can solve by simply changing the calling order here.
> DRM panel is completely missing any infrastructure to allow you to
> safely deal with that kind of situation.

This is not true for DSI panels - attach/detach callbacks allows to do
it correctly.

> 
> But it's something that I've been trying to fix for the past couple of
> days.
> 
>> If you leave ownership of drm_panel to connector you can reuse
>> synchronization mechanisms of connector/drm inside connector. Otherwise
>> you will need to add another locks in drm_panel.
>>
>> You can look at exynos_drm_dsi.c code, especially:
>> - exynos_dsi_detect,
>> - exynos_dsi_host_attach,
>> - exynos_dsi_host_detach,
>>
>> and usage of dsi->panel_node and dsi->panel with drm synchronization via
>> drm_helper_hpd_irq_event.
> 
> There are a number of ways that one could possibly implement this.

I do not think so. There are simple rules of synchronization, if you
follow them you will usually end up with similar design.


> I do
> not think any of the existing ones is as good as it could be and I think
> we really ought to standardize on how to do this to make it easier for
> other drivers to adapt DRM panels (and bridges for that matter).
> 
> Unfortunately before we have a good plan on how to handle panels more
> uniformly I don't see how we can possibly make everybody happy. It seems
> like currently whatever panel is written for Exynos isn't going to work
> on Tegra and vice-versa. So how about we merge this series and I'll look
> into how we can unify things?

This is not about compatibility between different hosts and panels, this
is just about making sharp panel driver work correctly with tegra dsi host.
I think it is quite easy to fix and it would be good to do it before
merging.

Unification is another issue which can be solved later.

Regards
Andrzej

> 
> I'd appreciate any ideas on how such a unification could look.
> 
> Thierry
> 
> 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

end of thread, other threads:[~2014-11-05 14:39 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-11-03  9:13 [PATCH v4 01/16] drm/dsi: Add message to packet translator Thierry Reding
2014-11-03  9:13 ` [PATCH v4 02/16] drm/dsi: Add DSI transfer helper Thierry Reding
2014-11-04 11:47   ` Andrzej Hajda
2014-11-03  9:13 ` [PATCH v4 03/16] drm/dsi: Make mipi_dsi_dcs_{read, write}() symmetrical Thierry Reding
2014-11-03  9:13 ` [PATCH v4 04/16] drm/dsi: Constify mipi_dsi_msg Thierry Reding
2014-11-03  9:13 ` [PATCH v4 05/16] drm/dsi: Add mipi_dsi_set_maximum_return_packet_size() helper Thierry Reding
2014-11-03  9:13 ` [PATCH v4 06/16] drm/panel: s6e8aa0: Use standard MIPI DSI function Thierry Reding
2014-11-03  9:13 ` [PATCH v4 07/16] drm/dsi: Implement generic read and write commands Thierry Reding
2014-11-03  9:13 ` [PATCH v4 08/16] drm/dsi: Implement some standard DCS commands Thierry Reding
2014-11-03  9:13 ` [PATCH v4 09/16] drm/dsi: Add to DocBook documentation Thierry Reding
2014-11-03  9:13 ` [PATCH v4 10/16] drm/dsi: Implement DCS nop command Thierry Reding
2014-11-03  9:13 ` [PATCH v4 11/16] drm/dsi: Implement DCS soft_reset command Thierry Reding
2014-11-03  9:13 ` [PATCH v4 12/16] drm/dsi: Implement DCS get_power_mode command Thierry Reding
2014-11-03  9:13 ` [PATCH v4 13/16] drm/dsi: Implement DCS {get, set}_pixel_format commands Thierry Reding
2014-11-03 17:10   ` Sean Paul
2014-11-04 13:34     ` [PATCH v4 13/16] drm/dsi: Implement DCS {get,set}_pixel_format commands Thierry Reding
2014-11-03  9:13 ` [PATCH v4 14/16] drm/dsi: Implement DCS set_{column, page}_address commands Thierry Reding
2014-11-03  9:13 ` [PATCH v4 15/16] drm/dsi: Resolve MIPI DSI device from phandle Thierry Reding
2014-11-03  9:13 ` [PATCH v4 16/16] drm/panel: Add Sharp LQ101R1SX01 support Thierry Reding
2014-11-03 17:28   ` Sean Paul
2014-11-04 10:41   ` Andrzej Hajda
2014-11-04 13:29     ` Thierry Reding
2014-11-05 14:39       ` Andrzej Hajda
2014-11-04 11:43 ` [PATCH v4 01/16] drm/dsi: Add message to packet translator Andrzej Hajda
2014-11-04 13:58   ` Thierry Reding
2014-11-04 14:21     ` Andrzej Hajda
2014-11-04 14:39       ` Thierry Reding
2014-11-05 13:35         ` Andrzej Hajda

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.