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
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);
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);
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);
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); };
/**
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,
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",
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,
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
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,
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);
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);
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);
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
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
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
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);
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,
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");
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
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");
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:
- panel link2 is probed
- panel link1 is probed
- panel link2 is unbound (for example by: echo link2
/sys/bus/dsi/drivers/*lq101*/unbind)
- 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
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:
- panel link2 is probed
- panel link1 is probed
- panel link2 is unbound (for example by: echo link2
/sys/bus/dsi/drivers/*lq101*/unbind)
- 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
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
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
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
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
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@lists.freedesktop.org