[PATCH 1/4] drm/dsi: Make mipi_dsi_dcs_write() return ssize_t

[PATCH 1/4] drm/dsi: Make mipi_dsi_dcs_write() return ssize_t

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

This function returns the value of the struct mipi_dsi_host_ops'
.transfer() so make sure the return types are consistent.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_mipi_dsi.c        | 4 ++--
 drivers/gpu/drm/panel/panel-s6e8aa0.c | 4 ++--
 include/drm/drm_mipi_dsi.h            | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index e633df2f68d8..6d2fd2077dae 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -205,8 +205,8 @@ EXPORT_SYMBOL(mipi_dsi_detach);
  * @data: pointer to the command followed by parameters
  * @len: length of @data
  */
-int mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, unsigned int channel,
-		       const void *data, size_t len)
+ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, unsigned int channel,
+			   const void *data, size_t len)
 {
 	const struct mipi_dsi_host_ops *ops = dsi->host->ops;
 	struct mipi_dsi_msg msg = {
diff --git a/drivers/gpu/drm/panel/panel-s6e8aa0.c b/drivers/gpu/drm/panel/panel-s6e8aa0.c
index 06e57a26db7a..beb43492b649 100644
--- a/drivers/gpu/drm/panel/panel-s6e8aa0.c
+++ b/drivers/gpu/drm/panel/panel-s6e8aa0.c
@@ -133,14 +133,14 @@ static int s6e8aa0_clear_error(struct s6e8aa0 *ctx)
 static void s6e8aa0_dcs_write(struct s6e8aa0 *ctx, const void *data, size_t len)
 {
 	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
-	int ret;
+	ssize_t ret;
 
 	if (ctx->error < 0)
 		return;
 
 	ret = mipi_dsi_dcs_write(dsi, dsi->channel, data, len);
 	if (ret < 0) {
-		dev_err(ctx->dev, "error %d writing dcs seq: %*ph\n", ret, len,
+		dev_err(ctx->dev, "error %zd writing dcs seq: %*ph\n", ret, len,
 			data);
 		ctx->error = ret;
 	}
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index efa1b552adc5..4b0112781910 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -127,8 +127,8 @@ struct mipi_dsi_device {
 
 int mipi_dsi_attach(struct mipi_dsi_device *dsi);
 int mipi_dsi_detach(struct mipi_dsi_device *dsi);
-int mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, unsigned int channel,
-		       const void *data, size_t len);
+ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, unsigned int channel,
+			   const void *data, size_t len);
 ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, unsigned int channel,
 			  u8 cmd, void *data, size_t len);
 
-- 
2.0.1

[PATCH 2/4] drm/dsi: Use peripheral's channel for DCS commands

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

When executing DCS commands, use the channel associated with the DSI
peripheral rather than one explicitly specified in the function call.
Devices shouldn't be able to step on each others' toes like this.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_mipi_dsi.c        | 14 ++++++--------
 drivers/gpu/drm/panel/panel-s6e8aa0.c |  4 ++--
 include/drm/drm_mipi_dsi.h            |  8 ++++----
 3 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index 6d2fd2077dae..6aa6a9e95570 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -201,16 +201,15 @@ EXPORT_SYMBOL(mipi_dsi_detach);
 /**
  * mipi_dsi_dcs_write - send DCS write command
  * @dsi: DSI device
- * @channel: virtual channel
  * @data: pointer to the command followed by parameters
  * @len: length of @data
  */
-ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, unsigned int channel,
-			   const void *data, size_t len)
+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 = channel,
+		.channel = dsi->channel,
 		.tx_buf = data,
 		.tx_len = len
 	};
@@ -239,19 +238,18 @@ EXPORT_SYMBOL(mipi_dsi_dcs_write);
 /**
  * mipi_dsi_dcs_read - send DCS read request command
  * @dsi: DSI device
- * @channel: virtual channel
  * @cmd: DCS read command
  * @data: pointer to read buffer
  * @len: length of @data
  *
  * Function returns number of read bytes or error code.
  */
-ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, unsigned int channel,
-			  u8 cmd, void *data, size_t len)
+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 = channel,
+		.channel = dsi->channel,
 		.type = MIPI_DSI_DCS_READ,
 		.tx_buf = &cmd,
 		.tx_len = 1,
diff --git a/drivers/gpu/drm/panel/panel-s6e8aa0.c b/drivers/gpu/drm/panel/panel-s6e8aa0.c
index beb43492b649..5502ef6bc074 100644
--- a/drivers/gpu/drm/panel/panel-s6e8aa0.c
+++ b/drivers/gpu/drm/panel/panel-s6e8aa0.c
@@ -138,7 +138,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, dsi->channel, data, len);
+	ret = mipi_dsi_dcs_write(dsi, data, len);
 	if (ret < 0) {
 		dev_err(ctx->dev, "error %zd writing dcs seq: %*ph\n", ret, len,
 			data);
@@ -154,7 +154,7 @@ static int s6e8aa0_dcs_read(struct s6e8aa0 *ctx, u8 cmd, void *data, size_t len)
 	if (ctx->error < 0)
 		return ctx->error;
 
-	ret = mipi_dsi_dcs_read(dsi, dsi->channel, cmd, data, len);
+	ret = mipi_dsi_dcs_read(dsi, cmd, data, len);
 	if (ret < 0) {
 		dev_err(ctx->dev, "error %d reading dcs seq(%#x)\n", ret, cmd);
 		ctx->error = ret;
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index 4b0112781910..7b5e1a9244e1 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -127,10 +127,10 @@ struct mipi_dsi_device {
 
 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, unsigned int channel,
-			   const void *data, size_t len);
-ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, unsigned int channel,
-			  u8 cmd, void *data, size_t len);
+ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, const void *data,
+			    size_t len);
+ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, u8 cmd, void *data,
+			  size_t len);
 
 /**
  * struct mipi_dsi_driver - DSI driver
-- 
2.0.1

[PATCH 3/4] drm/dsi: Make mipi_dsi_dcs_{read,write}() symmetrical

[Thierry Reding]

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

Also update the S6E8AA0 panel driver (the only user of this API) to
adapt to this new API.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_mipi_dsi.c        | 45 ++++++++++++++++++++++++-----------
 drivers/gpu/drm/panel/panel-s6e8aa0.c |  4 ++--
 include/drm/drm_mipi_dsi.h            |  4 ++--
 3 files changed, 35 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index 6aa6a9e95570..bbab06ef80c9 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -201,29 +201,41 @@ EXPORT_SYMBOL(mipi_dsi_detach);
 /**
  * mipi_dsi_dcs_write - send DCS write command
  * @dsi: DSI device
- * @data: pointer to the command followed by parameters
- * @len: length of @data
+ * @cmd: DCS command
+ * @data: pointer to the command payload
+ * @len: payload length
  */
-ssize_t mipi_dsi_dcs_write(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)
 {
 	const struct mipi_dsi_host_ops *ops = dsi->host->ops;
-	struct mipi_dsi_msg msg = {
-		.channel = dsi->channel,
-		.tx_buf = data,
-		.tx_len = len
-	};
+	struct mipi_dsi_msg msg;
+	ssize_t err;
+	u8 *tx;
 
 	if (!ops || !ops->transfer)
 		return -ENOSYS;
 
+	if (len > 0) {
+		tx = kmalloc(1 + len, GFP_KERNEL);
+		if (!tx)
+			return -ENOMEM;
+
+		tx[0] = cmd;
+		memcpy(tx + 1, data, len);
+	} else {
+		tx = &cmd;
+	}
+
+	msg.channel = dsi->channel;
+	msg.tx_len = 1 + len;
+	msg.tx_buf = tx;
+
 	switch (len) {
 	case 0:
-		return -EINVAL;
-	case 1:
 		msg.type = MIPI_DSI_DCS_SHORT_WRITE;
 		break;
-	case 2:
+	case 1:
 		msg.type = MIPI_DSI_DCS_SHORT_WRITE_PARAM;
 		break;
 	default:
@@ -231,14 +243,19 @@ ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, const void *data,
 		break;
 	}
 
-	return ops->transfer(dsi->host, &msg);
+	err = ops->transfer(dsi->host, &msg);
+
+	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
+ * @cmd: DCS command
  * @data: pointer to read buffer
  * @len: length of @data
  *
diff --git a/drivers/gpu/drm/panel/panel-s6e8aa0.c b/drivers/gpu/drm/panel/panel-s6e8aa0.c
index 5502ef6bc074..d39bee36816c 100644
--- a/drivers/gpu/drm/panel/panel-s6e8aa0.c
+++ b/drivers/gpu/drm/panel/panel-s6e8aa0.c
@@ -130,7 +130,7 @@ static int s6e8aa0_clear_error(struct s6e8aa0 *ctx)
 	return ret;
 }
 
-static void s6e8aa0_dcs_write(struct s6e8aa0 *ctx, const void *data, size_t len)
+static void s6e8aa0_dcs_write(struct s6e8aa0 *ctx, const u8 *data, size_t len)
 {
 	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
 	ssize_t ret;
@@ -138,7 +138,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(dsi, data[0], data + 1, len - 1);
 	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 7b5e1a9244e1..bea181121e8b 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -127,8 +127,8 @@ struct mipi_dsi_device {
 
 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(struct mipi_dsi_device *dsi, u8 cmd,
+			   const void *data, size_t len);
 ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, u8 cmd, void *data,
 			  size_t len);
 
-- 
2.0.1

[PATCH 4/4] drm/panel: s6e8aa0: Use static inline for upcasting

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Use a static inline function for upcasting a struct drm_panel to the
driver-specific structure. The advantage over using a macro is that it
gives us additional type checking.

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

diff --git a/drivers/gpu/drm/panel/panel-s6e8aa0.c b/drivers/gpu/drm/panel/panel-s6e8aa0.c
index d39bee36816c..6034d300d218 100644
--- a/drivers/gpu/drm/panel/panel-s6e8aa0.c
+++ b/drivers/gpu/drm/panel/panel-s6e8aa0.c
@@ -120,7 +120,10 @@ struct s6e8aa0 {
 	int error;
 };
 
-#define panel_to_s6e8aa0(p) container_of(p, struct s6e8aa0, panel)
+static inline struct s6e8aa0 *panel_to_s6e8aa0(struct drm_panel *panel)
+{
+	return container_of(panel, struct s6e8aa0, panel);
+}
 
 static int s6e8aa0_clear_error(struct s6e8aa0 *ctx)
 {
-- 
2.0.1

Re: [PATCH 1/4] drm/dsi: Make mipi_dsi_dcs_write() return ssize_t

[Andrzej Hajda]

Hi Thierry,

Thanks for the patch.

On 07/22/2014 09:12 AM, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> This function returns the value of the struct mipi_dsi_host_ops'
> .transfer() so make sure the return types are consistent.
>
> 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        | 4 ++--
>  drivers/gpu/drm/panel/panel-s6e8aa0.c | 4 ++--
>  include/drm/drm_mipi_dsi.h            | 4 ++--
>  3 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> index e633df2f68d8..6d2fd2077dae 100644
> --- a/drivers/gpu/drm/drm_mipi_dsi.c
> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> @@ -205,8 +205,8 @@ EXPORT_SYMBOL(mipi_dsi_detach);
>   * @data: pointer to the command followed by parameters
>   * @len: length of @data
>   */
> -int mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, unsigned int channel,
> -		       const void *data, size_t len)
> +ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, unsigned int channel,
> +			   const void *data, size_t len)
>  {
>  	const struct mipi_dsi_host_ops *ops = dsi->host->ops;
>  	struct mipi_dsi_msg msg = {
> diff --git a/drivers/gpu/drm/panel/panel-s6e8aa0.c b/drivers/gpu/drm/panel/panel-s6e8aa0.c
> index 06e57a26db7a..beb43492b649 100644
> --- a/drivers/gpu/drm/panel/panel-s6e8aa0.c
> +++ b/drivers/gpu/drm/panel/panel-s6e8aa0.c
> @@ -133,14 +133,14 @@ static int s6e8aa0_clear_error(struct s6e8aa0 *ctx)
>  static void s6e8aa0_dcs_write(struct s6e8aa0 *ctx, const void *data, size_t len)
>  {
>  	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
> -	int ret;
> +	ssize_t ret;
>  
>  	if (ctx->error < 0)
>  		return;
>  
>  	ret = mipi_dsi_dcs_write(dsi, dsi->channel, data, len);
>  	if (ret < 0) {
> -		dev_err(ctx->dev, "error %d writing dcs seq: %*ph\n", ret, len,
> +		dev_err(ctx->dev, "error %zd writing dcs seq: %*ph\n", ret, len,
>  			data);
>  		ctx->error = ret;
>  	}
> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
> index efa1b552adc5..4b0112781910 100644
> --- a/include/drm/drm_mipi_dsi.h
> +++ b/include/drm/drm_mipi_dsi.h
> @@ -127,8 +127,8 @@ struct mipi_dsi_device {
>  
>  int mipi_dsi_attach(struct mipi_dsi_device *dsi);
>  int mipi_dsi_detach(struct mipi_dsi_device *dsi);
> -int mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, unsigned int channel,
> -		       const void *data, size_t len);
> +ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, unsigned int channel,
> +			   const void *data, size_t len);
>  ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, unsigned int channel,
>  			  u8 cmd, void *data, size_t len);
>  

Re: [PATCH 2/4] drm/dsi: Use peripheral's channel for DCS commands

[Andrzej Hajda]

On 07/22/2014 09:12 AM, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> When executing DCS commands, use the channel associated with the DSI
> peripheral rather than one explicitly specified in the function call.
> Devices shouldn't be able to step on each others' toes like this.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>

There could be a problem with devices associated with more than one channel,
but this is theoretical problem as for now, so:

Acked-by: Andrzej Hajda <a.hajda@samsung.com>

Regards
Andrzej


> ---
>  drivers/gpu/drm/drm_mipi_dsi.c        | 14 ++++++--------
>  drivers/gpu/drm/panel/panel-s6e8aa0.c |  4 ++--
>  include/drm/drm_mipi_dsi.h            |  8 ++++----
>  3 files changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> index 6d2fd2077dae..6aa6a9e95570 100644
> --- a/drivers/gpu/drm/drm_mipi_dsi.c
> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> @@ -201,16 +201,15 @@ EXPORT_SYMBOL(mipi_dsi_detach);
>  /**
>   * mipi_dsi_dcs_write - send DCS write command
>   * @dsi: DSI device
> - * @channel: virtual channel
>   * @data: pointer to the command followed by parameters
>   * @len: length of @data
>   */
> -ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, unsigned int channel,
> -			   const void *data, size_t len)
> +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 = channel,
> +		.channel = dsi->channel,
>  		.tx_buf = data,
>  		.tx_len = len
>  	};
> @@ -239,19 +238,18 @@ EXPORT_SYMBOL(mipi_dsi_dcs_write);
>  /**
>   * mipi_dsi_dcs_read - send DCS read request command
>   * @dsi: DSI device
> - * @channel: virtual channel
>   * @cmd: DCS read command
>   * @data: pointer to read buffer
>   * @len: length of @data
>   *
>   * Function returns number of read bytes or error code.
>   */
> -ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, unsigned int channel,
> -			  u8 cmd, void *data, size_t len)
> +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 = channel,
> +		.channel = dsi->channel,
>  		.type = MIPI_DSI_DCS_READ,
>  		.tx_buf = &cmd,
>  		.tx_len = 1,
> diff --git a/drivers/gpu/drm/panel/panel-s6e8aa0.c b/drivers/gpu/drm/panel/panel-s6e8aa0.c
> index beb43492b649..5502ef6bc074 100644
> --- a/drivers/gpu/drm/panel/panel-s6e8aa0.c
> +++ b/drivers/gpu/drm/panel/panel-s6e8aa0.c
> @@ -138,7 +138,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, dsi->channel, data, len);
> +	ret = mipi_dsi_dcs_write(dsi, data, len);
>  	if (ret < 0) {
>  		dev_err(ctx->dev, "error %zd writing dcs seq: %*ph\n", ret, len,
>  			data);
> @@ -154,7 +154,7 @@ static int s6e8aa0_dcs_read(struct s6e8aa0 *ctx, u8 cmd, void *data, size_t len)
>  	if (ctx->error < 0)
>  		return ctx->error;
>  
> -	ret = mipi_dsi_dcs_read(dsi, dsi->channel, cmd, data, len);
> +	ret = mipi_dsi_dcs_read(dsi, cmd, data, len);
>  	if (ret < 0) {
>  		dev_err(ctx->dev, "error %d reading dcs seq(%#x)\n", ret, cmd);
>  		ctx->error = ret;
> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
> index 4b0112781910..7b5e1a9244e1 100644
> --- a/include/drm/drm_mipi_dsi.h
> +++ b/include/drm/drm_mipi_dsi.h
> @@ -127,10 +127,10 @@ struct mipi_dsi_device {
>  
>  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, unsigned int channel,
> -			   const void *data, size_t len);
> -ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, unsigned int channel,
> -			  u8 cmd, void *data, size_t len);
> +ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, const void *data,
> +			    size_t len);
> +ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, u8 cmd, void *data,
> +			  size_t len);
>  
>  /**
>   * struct mipi_dsi_driver - DSI driver

Re: [PATCH 3/4] drm/dsi: Make mipi_dsi_dcs_{read, write}() symmetrical

[Andrzej Hajda]

On 07/22/2014 09:12 AM, Thierry Reding wrote:
> 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().
>
> Also update the S6E8AA0 panel driver (the only user of this API) to
> adapt to this new API.

I do not agree with that. DCS read and write are not symmetrical by design:
- write just sends data,
- read sends data then reads response.

Forcing API to be symmetrical complicates the implementation without real gain.

Regards
Andrzej



>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/gpu/drm/drm_mipi_dsi.c        | 45 ++++++++++++++++++++++++-----------
>  drivers/gpu/drm/panel/panel-s6e8aa0.c |  4 ++--
>  include/drm/drm_mipi_dsi.h            |  4 ++--
>  3 files changed, 35 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> index 6aa6a9e95570..bbab06ef80c9 100644
> --- a/drivers/gpu/drm/drm_mipi_dsi.c
> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> @@ -201,29 +201,41 @@ EXPORT_SYMBOL(mipi_dsi_detach);
>  /**
>   * mipi_dsi_dcs_write - send DCS write command
>   * @dsi: DSI device
> - * @data: pointer to the command followed by parameters
> - * @len: length of @data
> + * @cmd: DCS command
> + * @data: pointer to the command payload
> + * @len: payload length
>   */
> -ssize_t mipi_dsi_dcs_write(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)
>  {
>  	const struct mipi_dsi_host_ops *ops = dsi->host->ops;
> -	struct mipi_dsi_msg msg = {
> -		.channel = dsi->channel,
> -		.tx_buf = data,
> -		.tx_len = len
> -	};
> +	struct mipi_dsi_msg msg;
> +	ssize_t err;
> +	u8 *tx;
>  
>  	if (!ops || !ops->transfer)
>  		return -ENOSYS;
>  
> +	if (len > 0) {
> +		tx = kmalloc(1 + len, GFP_KERNEL);
> +		if (!tx)
> +			return -ENOMEM;
> +
> +		tx[0] = cmd;
> +		memcpy(tx + 1, data, len);
> +	} else {
> +		tx = &cmd;
> +	}
> +
> +	msg.channel = dsi->channel;
> +	msg.tx_len = 1 + len;
> +	msg.tx_buf = tx;
> +
>  	switch (len) {
>  	case 0:
> -		return -EINVAL;
> -	case 1:
>  		msg.type = MIPI_DSI_DCS_SHORT_WRITE;
>  		break;
> -	case 2:
> +	case 1:
>  		msg.type = MIPI_DSI_DCS_SHORT_WRITE_PARAM;
>  		break;
>  	default:
> @@ -231,14 +243,19 @@ ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, const void *data,
>  		break;
>  	}
>  
> -	return ops->transfer(dsi->host, &msg);
> +	err = ops->transfer(dsi->host, &msg);
> +
> +	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
> + * @cmd: DCS command
>   * @data: pointer to read buffer
>   * @len: length of @data
>   *
> diff --git a/drivers/gpu/drm/panel/panel-s6e8aa0.c b/drivers/gpu/drm/panel/panel-s6e8aa0.c
> index 5502ef6bc074..d39bee36816c 100644
> --- a/drivers/gpu/drm/panel/panel-s6e8aa0.c
> +++ b/drivers/gpu/drm/panel/panel-s6e8aa0.c
> @@ -130,7 +130,7 @@ static int s6e8aa0_clear_error(struct s6e8aa0 *ctx)
>  	return ret;
>  }
>  
> -static void s6e8aa0_dcs_write(struct s6e8aa0 *ctx, const void *data, size_t len)
> +static void s6e8aa0_dcs_write(struct s6e8aa0 *ctx, const u8 *data, size_t len)
>  {
>  	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
>  	ssize_t ret;
> @@ -138,7 +138,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(dsi, data[0], data + 1, len - 1);
>  	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 7b5e1a9244e1..bea181121e8b 100644
> --- a/include/drm/drm_mipi_dsi.h
> +++ b/include/drm/drm_mipi_dsi.h
> @@ -127,8 +127,8 @@ struct mipi_dsi_device {
>  
>  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(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);
>  

Re: [PATCH 4/4] drm/panel: s6e8aa0: Use static inline for upcasting

[Andrzej Hajda]

On 07/22/2014 09:12 AM, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> Use a static inline function for upcasting a struct drm_panel to the
> driver-specific structure. The advantage over using a macro is that it
> gives us additional type checking.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
Acked-by: Andrzej Hajda <a.hajda@samsung.com>

> ---
>  drivers/gpu/drm/panel/panel-s6e8aa0.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-s6e8aa0.c b/drivers/gpu/drm/panel/panel-s6e8aa0.c
> index d39bee36816c..6034d300d218 100644
> --- a/drivers/gpu/drm/panel/panel-s6e8aa0.c
> +++ b/drivers/gpu/drm/panel/panel-s6e8aa0.c
> @@ -120,7 +120,10 @@ struct s6e8aa0 {
>  	int error;
>  };
>  
> -#define panel_to_s6e8aa0(p) container_of(p, struct s6e8aa0, panel)
> +static inline struct s6e8aa0 *panel_to_s6e8aa0(struct drm_panel *panel)
> +{
> +	return container_of(panel, struct s6e8aa0, panel);
> +}
>  
>  static int s6e8aa0_clear_error(struct s6e8aa0 *ctx)
>  {

Re: [PATCH 3/4] drm/dsi: Make mipi_dsi_dcs_{read, write}() symmetrical

[Thierry Reding]

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

On Tue, Jul 22, 2014 at 09:32:58AM +0200, Andrzej Hajda wrote:
> On 07/22/2014 09:12 AM, Thierry Reding wrote:
> > 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().
> >
> > Also update the S6E8AA0 panel driver (the only user of this API) to
> > adapt to this new API.
> 
> I do not agree with that. DCS read and write are not symmetrical by design:
> - write just sends data,
> - read sends data then reads response.
> 
> Forcing API to be symmetrical complicates the implementation without real gain.

Why does it complicate anything? Granted we need to dynamically allocate
a buffer for each transfer, but it's all hidden away in a common helper
function. The big advantage in using a separate parameter for the
command is that it makes it trivial to implement the majority of DCS
commands, like this:

	const u8 format = 0x77;
	const u8 mode = 0x00;

	mipi_dsi_dcs_write(dsi, MIPI_DCS_SOFT_RESET, NULL, 0);
	mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_PIXEL_FORMAT, &format, 1);
	mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_TEAR_ON, &mode, 1);
	mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_DISPLAY_ON, NULL, 0);

Without the extra parameter, the above becomes:

	const u8 data[1] = { MIPI_DCS_SOFT_RESET };
	mipi_dsi_dcs_write(dsi, data, sizeof(data));

	const u8 data[2] = { MIPI_DCS_SET_PIXEL_FORMAT, 0x77 };
	mipi_dsi_dcs_write(dsi, data, sizeof(data));

	const u8 data[2] = { MIPI_DCS_SET_TEAR_ON, 0x00 };
	mipi_dsi_dcs_write(dsi, data, sizeof(data));

	const u8 data[1] = { MIPI_DCS_SET_DISPLAY_ON };
	mipi_dsi_dcs_write(dsi, data, sizeof(data));

In this form it looks still rather readable, but if you need to do this
within an initialization function, it become fairly cluttered:

	const u8 soft_reset[1] = { MIPI_DCS_SOFT_RESET };
	const u8 set_pixel_format[2] = { MIPI_DCS_SET_PIXEL_FORMAT, 0x77 };
	const u8 set_tear_on[2] = { MIPI_DCS_SET_TEAR_ON, 0x00 };
	const u8 set_display_on[1] = { MIPI_DCS_SET_DISPLAY_ON };

	mipi_dsi_dcs_write(dsi, soft_reset, sizeof(soft_reset));

	mipi_dsi_dcs_write(dsi, set_pixel_format, sizeof(set_pixel_format));

	mipi_dsi_dcs_write(dsi, set_tear_on, sizeof(set_tear_on));

	mipi_dsi_dcs_write(dsi, set_display_on, sizeof(set_display_on));

I find that really hard to read because it has a potentially large gap
between the place where the commands are defined and where they're used.

Some of this could possibly be alleviated by adding separate functions
for standard commands. But either way, I think having this kind of
symmetry within an API is always good (it's consistent). By the law of
least surprise people will actually expect mipi_dsi_dcs_write() to take
a command as a second parameter.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 3/4] drm/dsi: Make mipi_dsi_dcs_{read, write}() symmetrical

[Andrzej Hajda]

On 07/22/2014 10:12 AM, Thierry Reding wrote:
> On Tue, Jul 22, 2014 at 09:32:58AM +0200, Andrzej Hajda wrote:
>> On 07/22/2014 09:12 AM, Thierry Reding wrote:
>>> 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().
>>>
>>> Also update the S6E8AA0 panel driver (the only user of this API) to
>>> adapt to this new API.
>> I do not agree with that. DCS read and write are not symmetrical by design:
>> - write just sends data,
>> - read sends data then reads response.
>>
>> Forcing API to be symmetrical complicates the implementation without real gain.
> Why does it complicate anything?

Why? Lets see:

 drivers/gpu/drm/drm_mipi_dsi.c        | 45 ++++++++++++++++++++++++-----------
 drivers/gpu/drm/panel/panel-s6e8aa0.c |  4 ++--
 include/drm/drm_mipi_dsi.h            |  4 ++--
 3 files changed, 35 insertions(+), 18 deletions(-)


Original function has two vars, one 'if', one 'switch' and one operation
call, nothing more.
You proposes to add new vars, kmalloc with no memory handling, memcpy,
playing with indices, conditional kfree. Isn't it enough to call it more
complicated? :)

>  Granted we need to dynamically allocate
> a buffer for each transfer, but it's all hidden away in a common helper
> function. The big advantage in using a separate parameter for the
> command is that it makes it trivial to implement the majority of DCS
> commands, like this:
>
> 	const u8 format = 0x77;
> 	const u8 mode = 0x00;
>
> 	mipi_dsi_dcs_write(dsi, MIPI_DCS_SOFT_RESET, NULL, 0);
> 	mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_PIXEL_FORMAT, &format, 1);
> 	mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_TEAR_ON, &mode, 1);
> 	mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_DISPLAY_ON, NULL, 0);

For this I have proposed once more universal macro, sth like this:

#define mipi_dsi_dcs_write_seq(dsi, seq...) \
({\
    const u8 d[] = { seq };\
    BUILD_BUG_ON_MSG(ARRAY_SIZE(d) > 64, "DCS sequence too big for stack");\
    mipi_dsi_dcs_write(dsi, d, ARRAY_SIZE(d));\
})

It makes trivial to implement ALL DCS commands in much more readable manner.
Please compare with your examples above:
    mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_SOFT_RESET);
    mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_PIXEL_FORMAT, 0x77);
    mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_TEAR_ON, 0);
    mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_DISPLAY_ON);

It could be also accompanied by static version, for memory optimization:

#define mipi_dsi_dcs_write_seq_static(dsi, seq...) \
({\
    static const u8 d[] = { seq };\
    mipi_dsi_dcs_write(dsi, d, ARRAY_SIZE(d));\
})


>
> Without the extra parameter, the above becomes:
>
> 	const u8 data[1] = { MIPI_DCS_SOFT_RESET };
> 	mipi_dsi_dcs_write(dsi, data, sizeof(data));
>
> 	const u8 data[2] = { MIPI_DCS_SET_PIXEL_FORMAT, 0x77 };
> 	mipi_dsi_dcs_write(dsi, data, sizeof(data));
>
> 	const u8 data[2] = { MIPI_DCS_SET_TEAR_ON, 0x00 };
> 	mipi_dsi_dcs_write(dsi, data, sizeof(data));
>
> 	const u8 data[1] = { MIPI_DCS_SET_DISPLAY_ON };
> 	mipi_dsi_dcs_write(dsi, data, sizeof(data));
>
> In this form it looks still rather readable, but if you need to do this
> within an initialization function, it become fairly cluttered:
>
> 	const u8 soft_reset[1] = { MIPI_DCS_SOFT_RESET };
> 	const u8 set_pixel_format[2] = { MIPI_DCS_SET_PIXEL_FORMAT, 0x77 };
> 	const u8 set_tear_on[2] = { MIPI_DCS_SET_TEAR_ON, 0x00 };
> 	const u8 set_display_on[1] = { MIPI_DCS_SET_DISPLAY_ON };
>
> 	mipi_dsi_dcs_write(dsi, soft_reset, sizeof(soft_reset));
>
> 	mipi_dsi_dcs_write(dsi, set_pixel_format, sizeof(set_pixel_format));
>
> 	mipi_dsi_dcs_write(dsi, set_tear_on, sizeof(set_tear_on));
>
> 	mipi_dsi_dcs_write(dsi, set_display_on, sizeof(set_display_on));
>
> I find that really hard to read because it has a potentially large gap
> between the place where the commands are defined and where they're used.

You will have the same gap issue with your approach in case of DCS
commands with arguments.
With 'my' approach it is still readable.

>
> Some of this could possibly be alleviated by adding separate functions
> for standard commands. But either way, I think having this kind of
> symmetry within an API is always good (it's consistent). By the law of
> least surprise people will actually expect mipi_dsi_dcs_write() to take
> a command as a second parameter.

As I wrote earlier I do not see symmetry here: dcs-read is in fact write
and read,
dcs-write is just write. Hiding this fact in API does not seems to me
good, but I guess
it is rather matter of taste.

Regards
Andrzej

>
> Thierry

Re: [PATCH 1/4] drm/dsi: Make mipi_dsi_dcs_write() return ssize_t

[YoungJun Cho]

Hi,

On 07/22/2014 04:28 PM, Andrzej Hajda wrote:
> Hi Thierry,
>
> Thanks for the patch.
>
> On 07/22/2014 09:12 AM, Thierry Reding wrote:
>> From: Thierry Reding <treding@nvidia.com>
>>
>> This function returns the value of the struct mipi_dsi_host_ops'
>> .transfer() so make sure the return types are consistent.
>>
>> 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        | 4 ++--
>>   drivers/gpu/drm/panel/panel-s6e8aa0.c | 4 ++--
>>   include/drm/drm_mipi_dsi.h            | 4 ++--
>>   3 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
>> index e633df2f68d8..6d2fd2077dae 100644
>> --- a/drivers/gpu/drm/drm_mipi_dsi.c
>> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
>> @@ -205,8 +205,8 @@ EXPORT_SYMBOL(mipi_dsi_detach);
>>    * @data: pointer to the command followed by parameters
>>    * @len: length of @data
>>    */
>> -int mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, unsigned int channel,
>> -		       const void *data, size_t len)
>> +ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, unsigned int channel,
>> +			   const void *data, size_t len)
>>   {
>>   	const struct mipi_dsi_host_ops *ops = dsi->host->ops;
>>   	struct mipi_dsi_msg msg = {
>> diff --git a/drivers/gpu/drm/panel/panel-s6e8aa0.c b/drivers/gpu/drm/panel/panel-s6e8aa0.c
>> index 06e57a26db7a..beb43492b649 100644
>> --- a/drivers/gpu/drm/panel/panel-s6e8aa0.c
>> +++ b/drivers/gpu/drm/panel/panel-s6e8aa0.c
>> @@ -133,14 +133,14 @@ static int s6e8aa0_clear_error(struct s6e8aa0 *ctx)
>>   static void s6e8aa0_dcs_write(struct s6e8aa0 *ctx, const void *data, size_t len)
>>   {
>>   	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
>> -	int ret;
>> +	ssize_t ret;
>>
>>   	if (ctx->error < 0)
>>   		return;
>>
>>   	ret = mipi_dsi_dcs_write(dsi, dsi->channel, data, len);
>>   	if (ret < 0) {
>> -		dev_err(ctx->dev, "error %d writing dcs seq: %*ph\n", ret, len,
>> +		dev_err(ctx->dev, "error %zd writing dcs seq: %*ph\n", ret, len,
>>   			data);
>>   		ctx->error = ret;

One more thing!
This 'ctx->error' type is 'int'. So it should be changed from int to 
ssize_t in struct s6e8aa0.

Thank you.
Best regards YJ

>>   	}
>> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
>> index efa1b552adc5..4b0112781910 100644
>> --- a/include/drm/drm_mipi_dsi.h
>> +++ b/include/drm/drm_mipi_dsi.h
>> @@ -127,8 +127,8 @@ struct mipi_dsi_device {
>>
>>   int mipi_dsi_attach(struct mipi_dsi_device *dsi);
>>   int mipi_dsi_detach(struct mipi_dsi_device *dsi);
>> -int mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, unsigned int channel,
>> -		       const void *data, size_t len);
>> +ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, unsigned int channel,
>> +			   const void *data, size_t len);
>>   ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, unsigned int channel,
>>   			  u8 cmd, void *data, size_t len);
>>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

Re: [PATCH 1/4] drm/dsi: Make mipi_dsi_dcs_write() return ssize_t

[Andrzej Hajda]

On 07/22/2014 11:50 AM, YoungJun Cho wrote:
> Hi,
>
> On 07/22/2014 04:28 PM, Andrzej Hajda wrote:
>> Hi Thierry,
>>
>> Thanks for the patch.
>>
>> On 07/22/2014 09:12 AM, Thierry Reding wrote:
>>> From: Thierry Reding <treding@nvidia.com>
>>>
>>> This function returns the value of the struct mipi_dsi_host_ops'
>>> .transfer() so make sure the return types are consistent.
>>>
>>> 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        | 4 ++--
>>>   drivers/gpu/drm/panel/panel-s6e8aa0.c | 4 ++--
>>>   include/drm/drm_mipi_dsi.h            | 4 ++--
>>>   3 files changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
>>> index e633df2f68d8..6d2fd2077dae 100644
>>> --- a/drivers/gpu/drm/drm_mipi_dsi.c
>>> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
>>> @@ -205,8 +205,8 @@ EXPORT_SYMBOL(mipi_dsi_detach);
>>>    * @data: pointer to the command followed by parameters
>>>    * @len: length of @data
>>>    */
>>> -int mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, unsigned int channel,
>>> -		       const void *data, size_t len)
>>> +ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, unsigned int channel,
>>> +			   const void *data, size_t len)
>>>   {
>>>   	const struct mipi_dsi_host_ops *ops = dsi->host->ops;
>>>   	struct mipi_dsi_msg msg = {
>>> diff --git a/drivers/gpu/drm/panel/panel-s6e8aa0.c b/drivers/gpu/drm/panel/panel-s6e8aa0.c
>>> index 06e57a26db7a..beb43492b649 100644
>>> --- a/drivers/gpu/drm/panel/panel-s6e8aa0.c
>>> +++ b/drivers/gpu/drm/panel/panel-s6e8aa0.c
>>> @@ -133,14 +133,14 @@ static int s6e8aa0_clear_error(struct s6e8aa0 *ctx)
>>>   static void s6e8aa0_dcs_write(struct s6e8aa0 *ctx, const void *data, size_t len)
>>>   {
>>>   	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
>>> -	int ret;
>>> +	ssize_t ret;
>>>
>>>   	if (ctx->error < 0)
>>>   		return;
>>>
>>>   	ret = mipi_dsi_dcs_write(dsi, dsi->channel, data, len);
>>>   	if (ret < 0) {
>>> -		dev_err(ctx->dev, "error %d writing dcs seq: %*ph\n", ret, len,
>>> +		dev_err(ctx->dev, "error %zd writing dcs seq: %*ph\n", ret, len,
>>>   			data);
>>>   		ctx->error = ret;
> One more thing!
> This 'ctx->error' type is 'int'. So it should be changed from int to 
> ssize_t in struct s6e8aa0.
I do not think so. ctx->error contains always error code, and this is
guarded here
by 'if (ret < 0)'  clause.

Regards
Andrzej
>
> Thank you.
> Best regards YJ
>
>>>   	}
>>> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
>>> index efa1b552adc5..4b0112781910 100644
>>> --- a/include/drm/drm_mipi_dsi.h
>>> +++ b/include/drm/drm_mipi_dsi.h
>>> @@ -127,8 +127,8 @@ struct mipi_dsi_device {
>>>
>>>   int mipi_dsi_attach(struct mipi_dsi_device *dsi);
>>>   int mipi_dsi_detach(struct mipi_dsi_device *dsi);
>>> -int mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, unsigned int channel,
>>> -		       const void *data, size_t len);
>>> +ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, unsigned int channel,
>>> +			   const void *data, size_t len);
>>>   ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, unsigned int channel,
>>>   			  u8 cmd, void *data, size_t len);
>>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>

Re: [PATCH 1/4] drm/dsi: Make mipi_dsi_dcs_write() return ssize_t

[Andrzej Hajda]

Hi Thierry,

YoungJun's comment refreshed my memory about mipi_dsi_dcs_write return
value. It should be rather int than ssize_t. Why?
.transfer() returns the number of read bytes or error, but in case
of dcs write no bytes are read, so it in fact returns error or 0.
This is why return value was implemented originally as int.
So I do not think this patch is necessary.

Regards
Andrzej


On 07/22/2014 12:05 PM, Andrzej Hajda wrote:
> On 07/22/2014 11:50 AM, YoungJun Cho wrote:
>> Hi,
>>
>> On 07/22/2014 04:28 PM, Andrzej Hajda wrote:
>>> Hi Thierry,
>>>
>>> Thanks for the patch.
>>>
>>> On 07/22/2014 09:12 AM, Thierry Reding wrote:
>>>> From: Thierry Reding <treding@nvidia.com>
>>>>
>>>> This function returns the value of the struct mipi_dsi_host_ops'
>>>> .transfer() so make sure the return types are consistent.
>>>>
>>>> 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        | 4 ++--
>>>>   drivers/gpu/drm/panel/panel-s6e8aa0.c | 4 ++--
>>>>   include/drm/drm_mipi_dsi.h            | 4 ++--
>>>>   3 files changed, 6 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
>>>> index e633df2f68d8..6d2fd2077dae 100644
>>>> --- a/drivers/gpu/drm/drm_mipi_dsi.c
>>>> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
>>>> @@ -205,8 +205,8 @@ EXPORT_SYMBOL(mipi_dsi_detach);
>>>>    * @data: pointer to the command followed by parameters
>>>>    * @len: length of @data
>>>>    */
>>>> -int mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, unsigned int channel,
>>>> -		       const void *data, size_t len)
>>>> +ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, unsigned int channel,
>>>> +			   const void *data, size_t len)
>>>>   {
>>>>   	const struct mipi_dsi_host_ops *ops = dsi->host->ops;
>>>>   	struct mipi_dsi_msg msg = {
>>>> diff --git a/drivers/gpu/drm/panel/panel-s6e8aa0.c b/drivers/gpu/drm/panel/panel-s6e8aa0.c
>>>> index 06e57a26db7a..beb43492b649 100644
>>>> --- a/drivers/gpu/drm/panel/panel-s6e8aa0.c
>>>> +++ b/drivers/gpu/drm/panel/panel-s6e8aa0.c
>>>> @@ -133,14 +133,14 @@ static int s6e8aa0_clear_error(struct s6e8aa0 *ctx)
>>>>   static void s6e8aa0_dcs_write(struct s6e8aa0 *ctx, const void *data, size_t len)
>>>>   {
>>>>   	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
>>>> -	int ret;
>>>> +	ssize_t ret;
>>>>
>>>>   	if (ctx->error < 0)
>>>>   		return;
>>>>
>>>>   	ret = mipi_dsi_dcs_write(dsi, dsi->channel, data, len);
>>>>   	if (ret < 0) {
>>>> -		dev_err(ctx->dev, "error %d writing dcs seq: %*ph\n", ret, len,
>>>> +		dev_err(ctx->dev, "error %zd writing dcs seq: %*ph\n", ret, len,
>>>>   			data);
>>>>   		ctx->error = ret;
>> One more thing!
>> This 'ctx->error' type is 'int'. So it should be changed from int to 
>> ssize_t in struct s6e8aa0.
> I do not think so. ctx->error contains always error code, and this is
> guarded here
> by 'if (ret < 0)'  clause.
> 
> Regards
> Andrzej
>>
>> Thank you.
>> Best regards YJ
>>
>>>>   	}
>>>> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
>>>> index efa1b552adc5..4b0112781910 100644
>>>> --- a/include/drm/drm_mipi_dsi.h
>>>> +++ b/include/drm/drm_mipi_dsi.h
>>>> @@ -127,8 +127,8 @@ struct mipi_dsi_device {
>>>>
>>>>   int mipi_dsi_attach(struct mipi_dsi_device *dsi);
>>>>   int mipi_dsi_detach(struct mipi_dsi_device *dsi);
>>>> -int mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, unsigned int channel,
>>>> -		       const void *data, size_t len);
>>>> +ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, unsigned int channel,
>>>> +			   const void *data, size_t len);
>>>>   ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, unsigned int channel,
>>>>   			  u8 cmd, void *data, size_t len);
>>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>
>>

Re: [PATCH 3/4] drm/dsi: Make mipi_dsi_dcs_{read, write}() symmetrical

[Daniel Vetter]

On Tue, Jul 22, 2014 at 09:12:20AM +0200, Thierry Reding wrote:
> 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().
> 
> Also update the S6E8AA0 panel driver (the only user of this API) to
> adapt to this new API.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>

Given the patterns established by other sideband subsystesm like i2c or
the dp aux helpers we have in drm I think this is going into the right
direction. Also, this seems to match somewhat the style we have in our
hand-rolled intel dsi implementation. So I think this makes sense.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_mipi_dsi.c        | 45 ++++++++++++++++++++++++-----------
>  drivers/gpu/drm/panel/panel-s6e8aa0.c |  4 ++--
>  include/drm/drm_mipi_dsi.h            |  4 ++--
>  3 files changed, 35 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> index 6aa6a9e95570..bbab06ef80c9 100644
> --- a/drivers/gpu/drm/drm_mipi_dsi.c
> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> @@ -201,29 +201,41 @@ EXPORT_SYMBOL(mipi_dsi_detach);
>  /**
>   * mipi_dsi_dcs_write - send DCS write command
>   * @dsi: DSI device
> - * @data: pointer to the command followed by parameters
> - * @len: length of @data
> + * @cmd: DCS command
> + * @data: pointer to the command payload
> + * @len: payload length
>   */
> -ssize_t mipi_dsi_dcs_write(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)
>  {
>  	const struct mipi_dsi_host_ops *ops = dsi->host->ops;
> -	struct mipi_dsi_msg msg = {
> -		.channel = dsi->channel,
> -		.tx_buf = data,
> -		.tx_len = len
> -	};
> +	struct mipi_dsi_msg msg;
> +	ssize_t err;
> +	u8 *tx;
>  
>  	if (!ops || !ops->transfer)
>  		return -ENOSYS;
>  
> +	if (len > 0) {
> +		tx = kmalloc(1 + len, GFP_KERNEL);
> +		if (!tx)
> +			return -ENOMEM;
> +
> +		tx[0] = cmd;
> +		memcpy(tx + 1, data, len);
> +	} else {
> +		tx = &cmd;
> +	}
> +
> +	msg.channel = dsi->channel;
> +	msg.tx_len = 1 + len;
> +	msg.tx_buf = tx;
> +
>  	switch (len) {
>  	case 0:
> -		return -EINVAL;
> -	case 1:
>  		msg.type = MIPI_DSI_DCS_SHORT_WRITE;
>  		break;
> -	case 2:
> +	case 1:
>  		msg.type = MIPI_DSI_DCS_SHORT_WRITE_PARAM;
>  		break;
>  	default:
> @@ -231,14 +243,19 @@ ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, const void *data,
>  		break;
>  	}
>  
> -	return ops->transfer(dsi->host, &msg);
> +	err = ops->transfer(dsi->host, &msg);
> +
> +	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
> + * @cmd: DCS command
>   * @data: pointer to read buffer
>   * @len: length of @data
>   *
> diff --git a/drivers/gpu/drm/panel/panel-s6e8aa0.c b/drivers/gpu/drm/panel/panel-s6e8aa0.c
> index 5502ef6bc074..d39bee36816c 100644
> --- a/drivers/gpu/drm/panel/panel-s6e8aa0.c
> +++ b/drivers/gpu/drm/panel/panel-s6e8aa0.c
> @@ -130,7 +130,7 @@ static int s6e8aa0_clear_error(struct s6e8aa0 *ctx)
>  	return ret;
>  }
>  
> -static void s6e8aa0_dcs_write(struct s6e8aa0 *ctx, const void *data, size_t len)
> +static void s6e8aa0_dcs_write(struct s6e8aa0 *ctx, const u8 *data, size_t len)
>  {
>  	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
>  	ssize_t ret;
> @@ -138,7 +138,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(dsi, data[0], data + 1, len - 1);
>  	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 7b5e1a9244e1..bea181121e8b 100644
> --- a/include/drm/drm_mipi_dsi.h
> +++ b/include/drm/drm_mipi_dsi.h
> @@ -127,8 +127,8 @@ struct mipi_dsi_device {
>  
>  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(struct mipi_dsi_device *dsi, u8 cmd,
> +			   const void *data, size_t len);
>  ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, u8 cmd, void *data,
>  			  size_t len);
>  
> -- 
> 2.0.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH 3/4] drm/dsi: Make mipi_dsi_dcs_{read, write}() symmetrical

[Andrzej Hajda]

On 07/22/2014 01:20 PM, Daniel Vetter wrote:
> On Tue, Jul 22, 2014 at 09:12:20AM +0200, Thierry Reding wrote:
>> 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().
>>
>> Also update the S6E8AA0 panel driver (the only user of this API) to
>> adapt to this new API.
>>
>> Signed-off-by: Thierry Reding <treding@nvidia.com>
> Given the patterns established by other sideband subsystesm like i2c or
> the dp aux helpers we have in drm I think this is going into the right
> direction. Also, this seems to match somewhat the style we have in our
> hand-rolled intel dsi implementation. So I think this makes sense.

Could you point me out the patterns you are referring to.

Regarding intel dsi implementation I have found in intel_dsi_cmd.h:

    int dsi_vc_dcs_write(struct intel_dsi *intel_dsi, int channel,
        const u8 *data, int len);
    int dsi_vc_dcs_read(struct intel_dsi *intel_dsi, int channel,
        u8 dcs_cmd, u8 *buf, int buflen);

This is the same approach as in the current drm mipi, no forced symmetry.

There are also helpers for simple DCS commands: dsi_vc_dcs_write_[01],
but they
are using dsi_vc_dcs_write internally, which seems to me more reasonable.

Regards
Andrzej

>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> ---
>>  drivers/gpu/drm/drm_mipi_dsi.c        | 45 ++++++++++++++++++++++++-----------
>>  drivers/gpu/drm/panel/panel-s6e8aa0.c |  4 ++--
>>  include/drm/drm_mipi_dsi.h            |  4 ++--
>>  3 files changed, 35 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
>> index 6aa6a9e95570..bbab06ef80c9 100644
>> --- a/drivers/gpu/drm/drm_mipi_dsi.c
>> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
>> @@ -201,29 +201,41 @@ EXPORT_SYMBOL(mipi_dsi_detach);
>>  /**
>>   * mipi_dsi_dcs_write - send DCS write command
>>   * @dsi: DSI device
>> - * @data: pointer to the command followed by parameters
>> - * @len: length of @data
>> + * @cmd: DCS command
>> + * @data: pointer to the command payload
>> + * @len: payload length
>>   */
>> -ssize_t mipi_dsi_dcs_write(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)
>>  {
>>  	const struct mipi_dsi_host_ops *ops = dsi->host->ops;
>> -	struct mipi_dsi_msg msg = {
>> -		.channel = dsi->channel,
>> -		.tx_buf = data,
>> -		.tx_len = len
>> -	};
>> +	struct mipi_dsi_msg msg;
>> +	ssize_t err;
>> +	u8 *tx;
>>  
>>  	if (!ops || !ops->transfer)
>>  		return -ENOSYS;
>>  
>> +	if (len > 0) {
>> +		tx = kmalloc(1 + len, GFP_KERNEL);
>> +		if (!tx)
>> +			return -ENOMEM;
>> +
>> +		tx[0] = cmd;
>> +		memcpy(tx + 1, data, len);
>> +	} else {
>> +		tx = &cmd;
>> +	}
>> +
>> +	msg.channel = dsi->channel;
>> +	msg.tx_len = 1 + len;
>> +	msg.tx_buf = tx;
>> +
>>  	switch (len) {
>>  	case 0:
>> -		return -EINVAL;
>> -	case 1:
>>  		msg.type = MIPI_DSI_DCS_SHORT_WRITE;
>>  		break;
>> -	case 2:
>> +	case 1:
>>  		msg.type = MIPI_DSI_DCS_SHORT_WRITE_PARAM;
>>  		break;
>>  	default:
>> @@ -231,14 +243,19 @@ ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, const void *data,
>>  		break;
>>  	}
>>  
>> -	return ops->transfer(dsi->host, &msg);
>> +	err = ops->transfer(dsi->host, &msg);
>> +
>> +	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
>> + * @cmd: DCS command
>>   * @data: pointer to read buffer
>>   * @len: length of @data
>>   *
>> diff --git a/drivers/gpu/drm/panel/panel-s6e8aa0.c b/drivers/gpu/drm/panel/panel-s6e8aa0.c
>> index 5502ef6bc074..d39bee36816c 100644
>> --- a/drivers/gpu/drm/panel/panel-s6e8aa0.c
>> +++ b/drivers/gpu/drm/panel/panel-s6e8aa0.c
>> @@ -130,7 +130,7 @@ static int s6e8aa0_clear_error(struct s6e8aa0 *ctx)
>>  	return ret;
>>  }
>>  
>> -static void s6e8aa0_dcs_write(struct s6e8aa0 *ctx, const void *data, size_t len)
>> +static void s6e8aa0_dcs_write(struct s6e8aa0 *ctx, const u8 *data, size_t len)
>>  {
>>  	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
>>  	ssize_t ret;
>> @@ -138,7 +138,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(dsi, data[0], data + 1, len - 1);
>>  	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 7b5e1a9244e1..bea181121e8b 100644
>> --- a/include/drm/drm_mipi_dsi.h
>> +++ b/include/drm/drm_mipi_dsi.h
>> @@ -127,8 +127,8 @@ struct mipi_dsi_device {
>>  
>>  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(struct mipi_dsi_device *dsi, u8 cmd,
>> +			   const void *data, size_t len);
>>  ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, u8 cmd, void *data,
>>  			  size_t len);
>>  
>> -- 
>> 2.0.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/dsi: Make mipi_dsi_dcs_write() return ssize_t

[Thierry Reding]

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

On Tue, Jul 22, 2014 at 12:23:07PM +0200, Andrzej Hajda wrote:
> Hi Thierry,
> 
> YoungJun's comment refreshed my memory about mipi_dsi_dcs_write return
> value. It should be rather int than ssize_t. Why?
> .transfer() returns the number of read bytes or error, but in case
> of dcs write no bytes are read, so it in fact returns error or 0.
> This is why return value was implemented originally as int.
> So I do not think this patch is necessary.

I think it should return the number of bytes written or an error. That
way we give callers the maximum amount of information. They may still
choose to only handle < 0 for convenience, but at least the information
will be there should it become required at some point.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 3/4] drm/dsi: Make mipi_dsi_dcs_{read, write}() symmetrical

[Thierry Reding]

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

On Tue, Jul 22, 2014 at 11:33:11AM +0200, Andrzej Hajda wrote:
> On 07/22/2014 10:12 AM, Thierry Reding wrote:
> > On Tue, Jul 22, 2014 at 09:32:58AM +0200, Andrzej Hajda wrote:
> >> On 07/22/2014 09:12 AM, Thierry Reding wrote:
> >>> 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().
> >>>
> >>> Also update the S6E8AA0 panel driver (the only user of this API) to
> >>> adapt to this new API.
> >> I do not agree with that. DCS read and write are not symmetrical by design:
> >> - write just sends data,
> >> - read sends data then reads response.
> >>
> >> Forcing API to be symmetrical complicates the implementation without real gain.
> > Why does it complicate anything?
> 
> Why? Lets see:
> 
>  drivers/gpu/drm/drm_mipi_dsi.c        | 45 ++++++++++++++++++++++++-----------
>  drivers/gpu/drm/panel/panel-s6e8aa0.c |  4 ++--
>  include/drm/drm_mipi_dsi.h            |  4 ++--
>  3 files changed, 35 insertions(+), 18 deletions(-)
> 
> 
> Original function has two vars, one 'if', one 'switch' and one operation
> call, nothing more.
> You proposes to add new vars, kmalloc with no memory handling, memcpy,
> playing with indices, conditional kfree. Isn't it enough to call it more
> complicated? :)

It certainly makes the implementation of mipi_dsi_dcs_write() slightly
more complicated, but the point is that it makes it easier for users of
the API. So the complexity moves into one central location rather than
each call site. Ultimately that will reduce overall complexity.

> >  Granted we need to dynamically allocate
> > a buffer for each transfer, but it's all hidden away in a common helper
> > function. The big advantage in using a separate parameter for the
> > command is that it makes it trivial to implement the majority of DCS
> > commands, like this:
> >
> > 	const u8 format = 0x77;
> > 	const u8 mode = 0x00;
> >
> > 	mipi_dsi_dcs_write(dsi, MIPI_DCS_SOFT_RESET, NULL, 0);
> > 	mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_PIXEL_FORMAT, &format, 1);
> > 	mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_TEAR_ON, &mode, 1);
> > 	mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_DISPLAY_ON, NULL, 0);
> 
> For this I have proposed once more universal macro, sth like this:
> 
> #define mipi_dsi_dcs_write_seq(dsi, seq...) \
> ({\
>     const u8 d[] = { seq };\
>     BUILD_BUG_ON_MSG(ARRAY_SIZE(d) > 64, "DCS sequence too big for stack");\
>     mipi_dsi_dcs_write(dsi, d, ARRAY_SIZE(d));\
> })
> 
> It makes trivial to implement ALL DCS commands in much more readable manner.
> Please compare with your examples above:
>     mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_SOFT_RESET);
>     mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_PIXEL_FORMAT, 0x77);
>     mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_TEAR_ON, 0);
>     mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_DISPLAY_ON);
> 
> It could be also accompanied by static version, for memory optimization:
> 
> #define mipi_dsi_dcs_write_seq_static(dsi, seq...) \
> ({\
>     static const u8 d[] = { seq };\
>     mipi_dsi_dcs_write(dsi, d, ARRAY_SIZE(d));\
> })

I've said before why I dislike these macros. One alternative for the
above would be to add something like a mipi_dsi_dcs_writev() function
that takes a variable list of arguments, somewhat like this:

	ssize_t mipi_dsi_dcs_writev(struct mipi_dsi_device *dsi, u8 command,
				    unsigned int count, ...)
	{
		...
	}

And the above would become:

	mipi_dsi_dcs_writev(dsi, MIPI_DCS_SOFT_RESET, 0);
	mipi_dsi_dcs_writev(dsi, MIPI_DCS_SET_PIXEL_FORMAT, 1, 0x77);
	...

That's still one argument more, but at least it isn't a macro.

Thierry

> 
> 
> >
> > Without the extra parameter, the above becomes:
> >
> > 	const u8 data[1] = { MIPI_DCS_SOFT_RESET };
> > 	mipi_dsi_dcs_write(dsi, data, sizeof(data));
> >
> > 	const u8 data[2] = { MIPI_DCS_SET_PIXEL_FORMAT, 0x77 };
> > 	mipi_dsi_dcs_write(dsi, data, sizeof(data));
> >
> > 	const u8 data[2] = { MIPI_DCS_SET_TEAR_ON, 0x00 };
> > 	mipi_dsi_dcs_write(dsi, data, sizeof(data));
> >
> > 	const u8 data[1] = { MIPI_DCS_SET_DISPLAY_ON };
> > 	mipi_dsi_dcs_write(dsi, data, sizeof(data));
> >
> > In this form it looks still rather readable, but if you need to do this
> > within an initialization function, it become fairly cluttered:
> >
> > 	const u8 soft_reset[1] = { MIPI_DCS_SOFT_RESET };
> > 	const u8 set_pixel_format[2] = { MIPI_DCS_SET_PIXEL_FORMAT, 0x77 };
> > 	const u8 set_tear_on[2] = { MIPI_DCS_SET_TEAR_ON, 0x00 };
> > 	const u8 set_display_on[1] = { MIPI_DCS_SET_DISPLAY_ON };
> >
> > 	mipi_dsi_dcs_write(dsi, soft_reset, sizeof(soft_reset));
> >
> > 	mipi_dsi_dcs_write(dsi, set_pixel_format, sizeof(set_pixel_format));
> >
> > 	mipi_dsi_dcs_write(dsi, set_tear_on, sizeof(set_tear_on));
> >
> > 	mipi_dsi_dcs_write(dsi, set_display_on, sizeof(set_display_on));
> >
> > I find that really hard to read because it has a potentially large gap
> > between the place where the commands are defined and where they're used.
> 
> You will have the same gap issue with your approach in case of DCS
> commands with arguments.

But it's at least reduced to only the payload. The actual command will
still be there on the same line as the function call. You don't have to
search for some array and inspect the array to find out which command is
being sent.

> With 'my' approach it is still readable.

Until you mess up the arguments and have to deal with the not-so-
readable errors from the compiler.

> > Some of this could possibly be alleviated by adding separate functions
> > for standard commands. But either way, I think having this kind of
> > symmetry within an API is always good (it's consistent). By the law of
> > least surprise people will actually expect mipi_dsi_dcs_write() to take
> > a command as a second parameter.
> 
> As I wrote earlier I do not see symmetry here: dcs-read is in fact write
> and read,
> dcs-write is just write. Hiding this fact in API does not seems to me
> good, but I guess
> it is rather matter of taste.

The symmetry isn't about the physical transfers. It's a logical
symmetry. Each DCS command is identified by a command, whether it's a
read or a write.

There's a similar dissymmetry in how the payload length is handled.
Currently peripheral drivers need to encode that within the payload
buffer, and DSI host drivers need to parse it out of that depending on
the type of write. That makes it needlessly difficult for host drivers
and I think the interface would be much easier to use if peripheral
drivers specified the payload (and its length) only and let drivers
obtain the length of writes from the .tx_len field.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 1/4] drm/dsi: Make mipi_dsi_dcs_write() return ssize_t

[Andrzej Hajda]

On 07/23/2014 09:27 AM, Thierry Reding wrote:
> On Tue, Jul 22, 2014 at 12:23:07PM +0200, Andrzej Hajda wrote:
>> Hi Thierry,
>>
>> YoungJun's comment refreshed my memory about mipi_dsi_dcs_write return
>> value. It should be rather int than ssize_t. Why?
>> .transfer() returns the number of read bytes or error, but in case
>> of dcs write no bytes are read, so it in fact returns error or 0.
>> This is why return value was implemented originally as int.
>> So I do not think this patch is necessary.
> I think it should return the number of bytes written or an error. That
> way we give callers the maximum amount of information. They may still
> choose to only handle < 0 for convenience, but at least the information
> will be there should it become required at some point.

AFAIK DSI write operation is atomic, ie either all requested bytes have
been sent
either there was an error and in such case we should assume no byte have
been sent.
Returning number of written bytes in this case is just returning length
of write buffer -
something caller already knows. Additionally it suggests that partial
write is possible
which is not true.
I know it is implemented such way in i2c (which is non atomic) but the
comment
in i2c_transfer shows its weakness [1].

[1]: http://lxr.free-electrons.com/source/drivers/i2c/i2c-core.c#L1782

Regards
Andrzej

>
> Thierry

Re: [PATCH 3/4] drm/dsi: Make mipi_dsi_dcs_{read, write}() symmetrical

[Andrzej Hajda]

On 07/23/2014 09:51 AM, Thierry Reding wrote:
> On Tue, Jul 22, 2014 at 11:33:11AM +0200, Andrzej Hajda wrote:
>> On 07/22/2014 10:12 AM, Thierry Reding wrote:
>>> On Tue, Jul 22, 2014 at 09:32:58AM +0200, Andrzej Hajda wrote:
>>>> On 07/22/2014 09:12 AM, Thierry Reding wrote:
>>>>> 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().
>>>>>
>>>>> Also update the S6E8AA0 panel driver (the only user of this API) to
>>>>> adapt to this new API.
>>>> I do not agree with that. DCS read and write are not symmetrical by design:
>>>> - write just sends data,
>>>> - read sends data then reads response.
>>>>
>>>> Forcing API to be symmetrical complicates the implementation without real gain.
>>> Why does it complicate anything?
>>
>> Why? Lets see:
>>
>>  drivers/gpu/drm/drm_mipi_dsi.c        | 45 ++++++++++++++++++++++++-----------
>>  drivers/gpu/drm/panel/panel-s6e8aa0.c |  4 ++--
>>  include/drm/drm_mipi_dsi.h            |  4 ++--
>>  3 files changed, 35 insertions(+), 18 deletions(-)
>>
>>
>> Original function has two vars, one 'if', one 'switch' and one operation
>> call, nothing more.
>> You proposes to add new vars, kmalloc with no memory handling, memcpy,
>> playing with indices, conditional kfree. Isn't it enough to call it more
>> complicated? :)
> 
> It certainly makes the implementation of mipi_dsi_dcs_write() slightly
> more complicated, but the point is that it makes it easier for users of
> the API. So the complexity moves into one central location rather than
> each call site. Ultimately that will reduce overall complexity.

I guess it will increase complexity. If you look at the s6e8aa0 or any
other driver using DCS commands you will see the most DCS commands have
constant arguments, so driver uses small static arrays without copying
them to temporary variables. With your approach every time you will have
to allocate tiny memory bufs, memcpy to them and deallocate them later.

I terms of drivers code simplicity, current version with proposed macros
do better job.

> 
>>>  Granted we need to dynamically allocate
>>> a buffer for each transfer, but it's all hidden away in a common helper
>>> function. The big advantage in using a separate parameter for the
>>> command is that it makes it trivial to implement the majority of DCS
>>> commands, like this:
>>>
>>> 	const u8 format = 0x77;
>>> 	const u8 mode = 0x00;
>>>
>>> 	mipi_dsi_dcs_write(dsi, MIPI_DCS_SOFT_RESET, NULL, 0);
>>> 	mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_PIXEL_FORMAT, &format, 1);
>>> 	mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_TEAR_ON, &mode, 1);
>>> 	mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_DISPLAY_ON, NULL, 0);
>>
>> For this I have proposed once more universal macro, sth like this:
>>
>> #define mipi_dsi_dcs_write_seq(dsi, seq...) \
>> ({\
>>     const u8 d[] = { seq };\
>>     BUILD_BUG_ON_MSG(ARRAY_SIZE(d) > 64, "DCS sequence too big for stack");\
>>     mipi_dsi_dcs_write(dsi, d, ARRAY_SIZE(d));\
>> })
>>
>> It makes trivial to implement ALL DCS commands in much more readable manner.
>> Please compare with your examples above:
>>     mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_SOFT_RESET);
>>     mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_PIXEL_FORMAT, 0x77);
>>     mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_TEAR_ON, 0);
>>     mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_DISPLAY_ON);
>>
>> It could be also accompanied by static version, for memory optimization:
>>
>> #define mipi_dsi_dcs_write_seq_static(dsi, seq...) \
>> ({\
>>     static const u8 d[] = { seq };\
>>     mipi_dsi_dcs_write(dsi, d, ARRAY_SIZE(d));\
>> })
> 
> I've said before why I dislike these macros. One alternative for the
> above would be to add something like a mipi_dsi_dcs_writev() function
> that takes a variable list of arguments,

Few greps in sources shows kernel prefers variadic macros over variadic
functions. Few examples:
#define or51132_writebytes(state, data...)
#define sn9c102_write_const_regs(sn9c102_device, data...)
#define v4l2_device_call_all(v4l2_dev, grpid, o, f, args...)
#define media_entity_call(entity, operation, args...)
#define call_memop(vb, op, args...)
#define __pcpu_size_call(stem, variable, ...)
#define rsnd_platform_call(priv, dai, func, param...)
#define send_seq(priv, data...)


> somewhat like this:
> 
> 	ssize_t mipi_dsi_dcs_writev(struct mipi_dsi_device *dsi, u8 command,
> 				    unsigned int count, ...)
> 	{
> 		...
> 	}
> 
> And the above would become:
> 
> 	mipi_dsi_dcs_writev(dsi, MIPI_DCS_SOFT_RESET, 0);
> 	mipi_dsi_dcs_writev(dsi, MIPI_DCS_SET_PIXEL_FORMAT, 1, 0x77);
> 	...
> 
> That's still one argument more, but at least it isn't a macro.
> 
> Thierry
> 
>>
>>
>>>
>>> Without the extra parameter, the above becomes:
>>>
>>> 	const u8 data[1] = { MIPI_DCS_SOFT_RESET };
>>> 	mipi_dsi_dcs_write(dsi, data, sizeof(data));
>>>
>>> 	const u8 data[2] = { MIPI_DCS_SET_PIXEL_FORMAT, 0x77 };
>>> 	mipi_dsi_dcs_write(dsi, data, sizeof(data));
>>>
>>> 	const u8 data[2] = { MIPI_DCS_SET_TEAR_ON, 0x00 };
>>> 	mipi_dsi_dcs_write(dsi, data, sizeof(data));
>>>
>>> 	const u8 data[1] = { MIPI_DCS_SET_DISPLAY_ON };
>>> 	mipi_dsi_dcs_write(dsi, data, sizeof(data));
>>>
>>> In this form it looks still rather readable, but if you need to do this
>>> within an initialization function, it become fairly cluttered:
>>>
>>> 	const u8 soft_reset[1] = { MIPI_DCS_SOFT_RESET };
>>> 	const u8 set_pixel_format[2] = { MIPI_DCS_SET_PIXEL_FORMAT, 0x77 };
>>> 	const u8 set_tear_on[2] = { MIPI_DCS_SET_TEAR_ON, 0x00 };
>>> 	const u8 set_display_on[1] = { MIPI_DCS_SET_DISPLAY_ON };
>>>
>>> 	mipi_dsi_dcs_write(dsi, soft_reset, sizeof(soft_reset));
>>>
>>> 	mipi_dsi_dcs_write(dsi, set_pixel_format, sizeof(set_pixel_format));
>>>
>>> 	mipi_dsi_dcs_write(dsi, set_tear_on, sizeof(set_tear_on));
>>>
>>> 	mipi_dsi_dcs_write(dsi, set_display_on, sizeof(set_display_on));
>>>
>>> I find that really hard to read because it has a potentially large gap
>>> between the place where the commands are defined and where they're used.
>>
>> You will have the same gap issue with your approach in case of DCS
>> commands with arguments.
> 
> But it's at least reduced to only the payload. The actual command will
> still be there on the same line as the function call. You don't have to
> search for some array and inspect the array to find out which command is
> being sent.
> 
>> With 'my' approach it is still readable.
> 
> Until you mess up the arguments and have to deal with the not-so-
> readable errors from the compiler.

This is simple macro, it is rather hard to mess with arguments,
additionally both arguments are type checked directly(seq) on
indirectly(dsi).

> 
>>> Some of this could possibly be alleviated by adding separate functions
>>> for standard commands. But either way, I think having this kind of
>>> symmetry within an API is always good (it's consistent). By the law of
>>> least surprise people will actually expect mipi_dsi_dcs_write() to take
>>> a command as a second parameter.
>>
>> As I wrote earlier I do not see symmetry here: dcs-read is in fact write
>> and read,
>> dcs-write is just write. Hiding this fact in API does not seems to me
>> good, but I guess
>> it is rather matter of taste.
> 
> The symmetry isn't about the physical transfers. It's a logical
> symmetry. Each DCS command is identified by a command, whether it's a
> read or a write.

If you insist on it maybe it will be better to leave my version of
mipi_dsi_dcs_write, maybe renamed to mipi_dsi_dcs_write_buf or sth
similar and add your version using internally my version. This way
you will have your symmetry and I will keep my simplicity :)


> 
> There's a similar dissymmetry in how the payload length is handled.
> Currently peripheral drivers need to encode that within the payload
> buffer, and DSI host drivers need to parse it out of that depending on
> the type of write. That makes it needlessly difficult for host drivers
> and I think the interface would be much easier to use if peripheral
> drivers specified the payload (and its length) only and let drivers
> obtain the length of writes from the .tx_len field.

I am not sure if I understand correctly. Where the peripherals encodes
payload length in payload buffer?

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

Re: [PATCH 3/4] drm/dsi: Make mipi_dsi_dcs_{read, write}() symmetrical

[Thierry Reding]

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

On Wed, Jul 23, 2014 at 12:59:46PM +0200, Andrzej Hajda wrote:
> On 07/23/2014 09:51 AM, Thierry Reding wrote:
> > On Tue, Jul 22, 2014 at 11:33:11AM +0200, Andrzej Hajda wrote:
> >> On 07/22/2014 10:12 AM, Thierry Reding wrote:
> >>> On Tue, Jul 22, 2014 at 09:32:58AM +0200, Andrzej Hajda wrote:
> >>>> On 07/22/2014 09:12 AM, Thierry Reding wrote:
> >>>>> 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().
> >>>>>
> >>>>> Also update the S6E8AA0 panel driver (the only user of this API) to
> >>>>> adapt to this new API.
> >>>> I do not agree with that. DCS read and write are not symmetrical by design:
> >>>> - write just sends data,
> >>>> - read sends data then reads response.
> >>>>
> >>>> Forcing API to be symmetrical complicates the implementation without real gain.
> >>> Why does it complicate anything?
> >>
> >> Why? Lets see:
> >>
> >>  drivers/gpu/drm/drm_mipi_dsi.c        | 45 ++++++++++++++++++++++++-----------
> >>  drivers/gpu/drm/panel/panel-s6e8aa0.c |  4 ++--
> >>  include/drm/drm_mipi_dsi.h            |  4 ++--
> >>  3 files changed, 35 insertions(+), 18 deletions(-)
> >>
> >>
> >> Original function has two vars, one 'if', one 'switch' and one operation
> >> call, nothing more.
> >> You proposes to add new vars, kmalloc with no memory handling, memcpy,
> >> playing with indices, conditional kfree. Isn't it enough to call it more
> >> complicated? :)
> > 
> > It certainly makes the implementation of mipi_dsi_dcs_write() slightly
> > more complicated, but the point is that it makes it easier for users of
> > the API. So the complexity moves into one central location rather than
> > each call site. Ultimately that will reduce overall complexity.
> 
> I guess it will increase complexity. If you look at the s6e8aa0 or any
> other driver using DCS commands you will see the most DCS commands have
> constant arguments, so driver uses small static arrays without copying
> them to temporary variables. With your approach every time you will have
> to allocate tiny memory bufs, memcpy to them and deallocate them later.

Yes, there are certainly additional costs. But they give us more
consistency in return. The whole point of having MIPI DCS helpers is so
that they can take away some of the work that drivers have to do. This
is core code

> I terms of drivers code simplicity, current version with proposed macros
> do better job.

Quite frankly, I think they result in horrible code. The s6e8aa0 driver
is currently not much more than a huge set of tables that are dumped to
hardware.

And of course that's "simple", but it's also completely unreadable and
makes it really difficult to factor out common pieces of code. Of course
the driver then also goes and uses these macros to execute standard DCS
commands instead of doing the right thing and writing proper functions
so that other drivers can reuse them. Yes, that's simple for the
individual drivers but it doesn't help us at all in the big picture.

> >>> Some of this could possibly be alleviated by adding separate functions
> >>> for standard commands. But either way, I think having this kind of
> >>> symmetry within an API is always good (it's consistent). By the law of
> >>> least surprise people will actually expect mipi_dsi_dcs_write() to take
> >>> a command as a second parameter.
> >>
> >> As I wrote earlier I do not see symmetry here: dcs-read is in fact write
> >> and read,
> >> dcs-write is just write. Hiding this fact in API does not seems to me
> >> good, but I guess
> >> it is rather matter of taste.
> > 
> > The symmetry isn't about the physical transfers. It's a logical
> > symmetry. Each DCS command is identified by a command, whether it's a
> > read or a write.
> 
> If you insist on it maybe it will be better to leave my version of
> mipi_dsi_dcs_write, maybe renamed to mipi_dsi_dcs_write_buf or sth
> similar and add your version using internally my version. This way
> you will have your symmetry and I will keep my simplicity :)

Maybe that would be an alternative. I'll think about it.

> > There's a similar dissymmetry in how the payload length is handled.
> > Currently peripheral drivers need to encode that within the payload
> > buffer, and DSI host drivers need to parse it out of that depending on
> > the type of write. That makes it needlessly difficult for host drivers
> > and I think the interface would be much easier to use if peripheral
> > drivers specified the payload (and its length) only and let drivers
> > obtain the length of writes from the .tx_len field.
> 
> I am not sure if I understand correctly. Where the peripherals encodes
> payload length in payload buffer?

Heh, it's not encoded in the payload buffer, which makes this even
weirder. Because now we have this binary buffer that the DSI host's
.transfer() function needs to take apart and put together differently
depending on the type of packet.

So this whole DCS business isn't really thought out very well. I'll go
play with it some more to see how we can possibly improve it. We seem to
have a similar issue for reads, where currently every host driver needs
to parse returned packets itself in order to return data to the caller.
That completely annuls the purpose of the DCS functions. They should
really be making it easier for both host and peripheral drivers by
translating between DCS and the raw DSI packet data.

So what I'm currently thinking is that we need a better definition for
exactly what data should go into a struct mipi_dsi_msg. I think it
should be raw DSI data (that is, including the header and the payload)
so that the only job of drivers is to write it into the corresponding
FIFO registers and start the transmission.

Similarly, when receiving data the .transfer() function should pass up a
complete buffer (including the header and payload) to the receive buffer
and let some upper layer handle this. That way we can layer things in
helpers rather than having to duplicate the same code in each DSI host
driver.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 3/4] drm/dsi: Make mipi_dsi_dcs_{read, write}() symmetrical

[Andrzej Hajda]

On 07/23/2014 03:37 PM, Thierry Reding wrote:
> On Wed, Jul 23, 2014 at 12:59:46PM +0200, Andrzej Hajda wrote:
>> On 07/23/2014 09:51 AM, Thierry Reding wrote:
>>> On Tue, Jul 22, 2014 at 11:33:11AM +0200, Andrzej Hajda wrote:
>>>> On 07/22/2014 10:12 AM, Thierry Reding wrote:
>>>>> On Tue, Jul 22, 2014 at 09:32:58AM +0200, Andrzej Hajda wrote:
>>>>>> On 07/22/2014 09:12 AM, Thierry Reding wrote:
>>>>>>> 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().
>>>>>>>
>>>>>>> Also update the S6E8AA0 panel driver (the only user of this API) to
>>>>>>> adapt to this new API.
>>>>>> I do not agree with that. DCS read and write are not symmetrical by design:
>>>>>> - write just sends data,
>>>>>> - read sends data then reads response.
>>>>>>
>>>>>> Forcing API to be symmetrical complicates the implementation without real gain.
>>>>> Why does it complicate anything?
>>>> Why? Lets see:
>>>>
>>>>  drivers/gpu/drm/drm_mipi_dsi.c        | 45 ++++++++++++++++++++++++-----------
>>>>  drivers/gpu/drm/panel/panel-s6e8aa0.c |  4 ++--
>>>>  include/drm/drm_mipi_dsi.h            |  4 ++--
>>>>  3 files changed, 35 insertions(+), 18 deletions(-)
>>>>
>>>>
>>>> Original function has two vars, one 'if', one 'switch' and one operation
>>>> call, nothing more.
>>>> You proposes to add new vars, kmalloc with no memory handling, memcpy,
>>>> playing with indices, conditional kfree. Isn't it enough to call it more
>>>> complicated? :)
>>> It certainly makes the implementation of mipi_dsi_dcs_write() slightly
>>> more complicated, but the point is that it makes it easier for users of
>>> the API. So the complexity moves into one central location rather than
>>> each call site. Ultimately that will reduce overall complexity.
>> I guess it will increase complexity. If you look at the s6e8aa0 or any
>> other driver using DCS commands you will see the most DCS commands have
>> constant arguments, so driver uses small static arrays without copying
>> them to temporary variables. With your approach every time you will have
>> to allocate tiny memory bufs, memcpy to them and deallocate them later.
> Yes, there are certainly additional costs. But they give us more
> consistency in return. The whole point of having MIPI DCS helpers is so
> that they can take away some of the work that drivers have to do. This
> is core code
>
>> I terms of drivers code simplicity, current version with proposed macros
>> do better job.
> Quite frankly, I think they result in horrible code. The s6e8aa0 driver
> is currently not much more than a huge set of tables that are dumped to
> hardware.
>
> And of course that's "simple", but it's also completely unreadable and
> makes it really difficult to factor out common pieces of code. Of course
> the driver then also goes and uses these macros to execute standard DCS
> commands instead of doing the right thing and writing proper functions
> so that other drivers can reuse them. Yes, that's simple for the
> individual drivers but it doesn't help us at all in the big picture.

Hmm, I think you look at the wrong driver :) panel-s6e8aa0.c uses only
four DCS commands:
        s6e8aa0_dcs_write_seq_static(ctx, MIPI_DCS_EXIT_SLEEP_MODE);
        s6e8aa0_dcs_write_seq_static(ctx, MIPI_DCS_SET_DISPLAY_ON);
        s6e8aa0_dcs_write_seq_static(ctx, MIPI_DCS_ENTER_SLEEP_MODE);
        s6e8aa0_dcs_write_seq_static(ctx, MIPI_DCS_SET_DISPLAY_OFF);
I see it rather readable :)

The rest of DSI transactions are MCS commands,  completely undocumented
- I had no access to datasheet for this
panel/IC driver during driver development. How do you imagine 'proper
functions' in such situation?

>>>>> Some of this could possibly be alleviated by adding separate functions
>>>>> for standard commands. But either way, I think having this kind of
>>>>> symmetry within an API is always good (it's consistent). By the law of
>>>>> least surprise people will actually expect mipi_dsi_dcs_write() to take
>>>>> a command as a second parameter.
>>>> As I wrote earlier I do not see symmetry here: dcs-read is in fact write
>>>> and read,
>>>> dcs-write is just write. Hiding this fact in API does not seems to me
>>>> good, but I guess
>>>> it is rather matter of taste.
>>> The symmetry isn't about the physical transfers. It's a logical
>>> symmetry. Each DCS command is identified by a command, whether it's a
>>> read or a write.
>> If you insist on it maybe it will be better to leave my version of
>> mipi_dsi_dcs_write, maybe renamed to mipi_dsi_dcs_write_buf or sth
>> similar and add your version using internally my version. This way
>> you will have your symmetry and I will keep my simplicity :)
> Maybe that would be an alternative. I'll think about it.
>
>>> There's a similar dissymmetry in how the payload length is handled.
>>> Currently peripheral drivers need to encode that within the payload
>>> buffer, and DSI host drivers need to parse it out of that depending on
>>> the type of write. That makes it needlessly difficult for host drivers
>>> and I think the interface would be much easier to use if peripheral
>>> drivers specified the payload (and its length) only and let drivers
>>> obtain the length of writes from the .tx_len field.
>> I am not sure if I understand correctly. Where the peripherals encodes
>> payload length in payload buffer?
> Heh, it's not encoded in the payload buffer, which makes this even
> weirder. Because now we have this binary buffer that the DSI host's
> .transfer() function needs to take apart and put together differently
> depending on the type of packet.
>
> So this whole DCS business isn't really thought out very well. I'll go
> play with it some more to see how we can possibly improve it. We seem to
> have a similar issue for reads, where currently every host driver needs
> to parse returned packets itself in order to return data to the caller.
> That completely annuls the purpose of the DCS functions. They should
> really be making it easier for both host and peripheral drivers by
> translating between DCS and the raw DSI packet data.

I guess this could be an area of improvement, but it would be good
to have more than one implementation of dsi transfer in mainline
to have possibility of looking for common patterns and for differences.

>
> So what I'm currently thinking is that we need a better definition for
> exactly what data should go into a struct mipi_dsi_msg. I think it
> should be raw DSI data (that is, including the header and the payload)
> so that the only job of drivers is to write it into the corresponding
> FIFO registers and start the transmission.

This or just helpers in DSI host to calculate raw DSI data.
What about ECC? Should it be present also in tx buffer?

Andrzej

>
> Similarly, when receiving data the .transfer() function should pass up a
> complete buffer (including the header and payload) to the receive buffer
> and let some upper layer handle this. That way we can layer things in
> helpers rather than having to duplicate the same code in each DSI host
> driver.
>
> Thierry

Re: [PATCH 3/4] drm/dsi: Make mipi_dsi_dcs_{read, write}() symmetrical

[Thierry Reding]

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

On Thu, Jul 24, 2014 at 09:57:04AM +0200, Andrzej Hajda wrote:
> On 07/23/2014 03:37 PM, Thierry Reding wrote:
> > On Wed, Jul 23, 2014 at 12:59:46PM +0200, Andrzej Hajda wrote:
> >> On 07/23/2014 09:51 AM, Thierry Reding wrote:
> >>> On Tue, Jul 22, 2014 at 11:33:11AM +0200, Andrzej Hajda wrote:
> >>>> On 07/22/2014 10:12 AM, Thierry Reding wrote:
> >>>>> On Tue, Jul 22, 2014 at 09:32:58AM +0200, Andrzej Hajda wrote:
> >>>>>> On 07/22/2014 09:12 AM, Thierry Reding wrote:
> >>>>>>> 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().
> >>>>>>>
> >>>>>>> Also update the S6E8AA0 panel driver (the only user of this API) to
> >>>>>>> adapt to this new API.
> >>>>>> I do not agree with that. DCS read and write are not symmetrical by design:
> >>>>>> - write just sends data,
> >>>>>> - read sends data then reads response.
> >>>>>>
> >>>>>> Forcing API to be symmetrical complicates the implementation without real gain.
> >>>>> Why does it complicate anything?
> >>>> Why? Lets see:
> >>>>
> >>>>  drivers/gpu/drm/drm_mipi_dsi.c        | 45 ++++++++++++++++++++++++-----------
> >>>>  drivers/gpu/drm/panel/panel-s6e8aa0.c |  4 ++--
> >>>>  include/drm/drm_mipi_dsi.h            |  4 ++--
> >>>>  3 files changed, 35 insertions(+), 18 deletions(-)
> >>>>
> >>>>
> >>>> Original function has two vars, one 'if', one 'switch' and one operation
> >>>> call, nothing more.
> >>>> You proposes to add new vars, kmalloc with no memory handling, memcpy,
> >>>> playing with indices, conditional kfree. Isn't it enough to call it more
> >>>> complicated? :)
> >>> It certainly makes the implementation of mipi_dsi_dcs_write() slightly
> >>> more complicated, but the point is that it makes it easier for users of
> >>> the API. So the complexity moves into one central location rather than
> >>> each call site. Ultimately that will reduce overall complexity.
> >> I guess it will increase complexity. If you look at the s6e8aa0 or any
> >> other driver using DCS commands you will see the most DCS commands have
> >> constant arguments, so driver uses small static arrays without copying
> >> them to temporary variables. With your approach every time you will have
> >> to allocate tiny memory bufs, memcpy to them and deallocate them later.
> > Yes, there are certainly additional costs. But they give us more
> > consistency in return. The whole point of having MIPI DCS helpers is so
> > that they can take away some of the work that drivers have to do. This
> > is core code
> >
> >> I terms of drivers code simplicity, current version with proposed macros
> >> do better job.
> > Quite frankly, I think they result in horrible code. The s6e8aa0 driver
> > is currently not much more than a huge set of tables that are dumped to
> > hardware.
> >
> > And of course that's "simple", but it's also completely unreadable and
> > makes it really difficult to factor out common pieces of code. Of course
> > the driver then also goes and uses these macros to execute standard DCS
> > commands instead of doing the right thing and writing proper functions
> > so that other drivers can reuse them. Yes, that's simple for the
> > individual drivers but it doesn't help us at all in the big picture.
> 
> Hmm, I think you look at the wrong driver :) panel-s6e8aa0.c uses only
> four DCS commands:
>         s6e8aa0_dcs_write_seq_static(ctx, MIPI_DCS_EXIT_SLEEP_MODE);
>         s6e8aa0_dcs_write_seq_static(ctx, MIPI_DCS_SET_DISPLAY_ON);
>         s6e8aa0_dcs_write_seq_static(ctx, MIPI_DCS_ENTER_SLEEP_MODE);
>         s6e8aa0_dcs_write_seq_static(ctx, MIPI_DCS_SET_DISPLAY_OFF);
> I see it rather readable :)

When I was talking about readable I wasn't talking about these. These
are what I said should be generic functions. They aren't s6e8aa0
specific at all, therefore shouldn't be using s6e8aa0 specific
functions.

> The rest of DSI transactions are MCS commands,  completely undocumented
> - I had no access to datasheet for this
> panel/IC driver during driver development.

So did you just guess the right sequences? Somebody must have had access
to the documentation at some point.

> How do you imagine 'proper functions' in such situation?

I agree that that's a problem. However I think we should try really hard
to get access to proper documentation. We require other drivers to avoid
these kinds of magic values, why should panels be different?

While there may be circumstances where it's simply not possible to get
access to the documentation, providing an API that allows to pass in any
arbitrary binary buffer actively encourages this kind of behaviour. I
know that we can't really prevent it by extracting the command into a
separate function argument, but at least there's a slight chance that
it'll make people think for a second before just dumping some sequence
into a binary buffer and passing it to some function.

> >>>>> Some of this could possibly be alleviated by adding separate functions
> >>>>> for standard commands. But either way, I think having this kind of
> >>>>> symmetry within an API is always good (it's consistent). By the law of
> >>>>> least surprise people will actually expect mipi_dsi_dcs_write() to take
> >>>>> a command as a second parameter.
> >>>> As I wrote earlier I do not see symmetry here: dcs-read is in fact write
> >>>> and read,
> >>>> dcs-write is just write. Hiding this fact in API does not seems to me
> >>>> good, but I guess
> >>>> it is rather matter of taste.
> >>> The symmetry isn't about the physical transfers. It's a logical
> >>> symmetry. Each DCS command is identified by a command, whether it's a
> >>> read or a write.
> >> If you insist on it maybe it will be better to leave my version of
> >> mipi_dsi_dcs_write, maybe renamed to mipi_dsi_dcs_write_buf or sth
> >> similar and add your version using internally my version. This way
> >> you will have your symmetry and I will keep my simplicity :)
> > Maybe that would be an alternative. I'll think about it.
> >
> >>> There's a similar dissymmetry in how the payload length is handled.
> >>> Currently peripheral drivers need to encode that within the payload
> >>> buffer, and DSI host drivers need to parse it out of that depending on
> >>> the type of write. That makes it needlessly difficult for host drivers
> >>> and I think the interface would be much easier to use if peripheral
> >>> drivers specified the payload (and its length) only and let drivers
> >>> obtain the length of writes from the .tx_len field.
> >> I am not sure if I understand correctly. Where the peripherals encodes
> >> payload length in payload buffer?
> > Heh, it's not encoded in the payload buffer, which makes this even
> > weirder. Because now we have this binary buffer that the DSI host's
> > .transfer() function needs to take apart and put together differently
> > depending on the type of packet.
> >
> > So this whole DCS business isn't really thought out very well. I'll go
> > play with it some more to see how we can possibly improve it. We seem to
> > have a similar issue for reads, where currently every host driver needs
> > to parse returned packets itself in order to return data to the caller.
> > That completely annuls the purpose of the DCS functions. They should
> > really be making it easier for both host and peripheral drivers by
> > translating between DCS and the raw DSI packet data.
> 
> I guess this could be an area of improvement, but it would be good
> to have more than one implementation of dsi transfer in mainline
> to have possibility of looking for common patterns and for differences.

I'm currently working on exactly that, which is why I'm finding all
these inconsistencies. While it's always good to have more than one
implementation to look for common patterns, it's usually pretty bad
when there's only one, because then it's fairly likely that the code
won't work very well on a second implementation.

So what we need to be doing now is find ways to make this work equally
well for Exynos and Tegra (as well as across panel implementations) and
then hope it'll be sufficiently good so that we don't have to redesign
once again when the third implementation comes around.

> > So what I'm currently thinking is that we need a better definition for
> > exactly what data should go into a struct mipi_dsi_msg. I think it
> > should be raw DSI data (that is, including the header and the payload)
> > so that the only job of drivers is to write it into the corresponding
> > FIFO registers and start the transmission.
> 
> This or just helpers in DSI host to calculate raw DSI data.

I think either way would work. I slightly prefer the direct layering
because we're implementing a standard protocol on top of another
standard protocol. So it's fairly similar to networking protocols or
even filesystems on top of the block layer.

Therefore I think it makes sense to have an API that takes DCS inputs,
such as command and payload, translates them to DSI and passes raw DSI
to the driver, since presumably that's what the driver needs. That way
we can move the protocol bits into common functions and let drivers do
what they do best: interface with the hardware.

> What about ECC? Should it be present also in tx buffer?

ECC and CS are somewhat special. Hardware will probably provide a way to
offload ECC and CS computations in the majority of cases, so it doesn't
make much sense to compute them by default. I see two ways to achieve
that: a) provide flags in struct mipi_dsi_host for supported features so
that the protocol code can compute it if necessary or b) provide helpers
that take a raw DSI packet and compute its CS or ECC.

While I couldn't find an explicit reference to a CS or ECC enable bit,
there also doesn't seem to be any code to compute it, so I assume it's
always enabled (or at least by default). Tegra can also compute them in
hardware, so I don't think we need to concern ourselves with that at
this point.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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