On 10/13/2014 12:16 PM, Thierry Reding wrote:

From: Thierry Reding treding@nvidia.com

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

Signed-off-by: Thierry Reding treding@nvidia.com

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

-- Regards Andrzej

---

drivers/gpu/drm/exynos/exynos_drm_dsi.c | 2 +- include/drm/drm_mipi_dsi.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index 24741d8758e8..c5f3c76bfac3 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -1236,7 +1236,7 @@ static bool exynos_dsi_is_short_dsi_type(u8 type) }

static ssize_t exynos_dsi_host_transfer(struct mipi_dsi_host *host,

• 		       struct mipi_dsi_msg *msg)
  

• 		        const struct mipi_dsi_msg *msg)
  

{ struct exynos_dsi *dsi = host_to_dsi(host); struct exynos_dsi_transfer xfer; diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h index ccc3869e137b..836cc2b677d0 100644 --- a/include/drm/drm_mipi_dsi.h +++ b/include/drm/drm_mipi_dsi.h @@ -56,7 +56,7 @@ struct mipi_dsi_host_ops { int (*detach)(struct mipi_dsi_host *host, struct mipi_dsi_device *dsi); ssize_t (*transfer)(struct mipi_dsi_host *host,

• 	    struct mipi_dsi_msg *msg);
  

• 	    const struct mipi_dsi_msg *msg);
  

};

/**

On 10/13/2014 12:16 PM, Thierry Reding wrote:

From: YoungJun Cho yj44.cho@samsung.com

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

Signed-off-by: YoungJun Cho yj44.cho@samsung.com [treding: endianess, kerneldoc, return value] Signed-off-by: Thierry Reding treding@nvidia.com

---

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

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c index 1702ffd07986..27fc6dac5e4a 100644 --- a/drivers/gpu/drm/drm_mipi_dsi.c +++ b/drivers/gpu/drm/drm_mipi_dsi.c @@ -198,6 +198,36 @@ int mipi_dsi_detach(struct mipi_dsi_device *dsi) } EXPORT_SYMBOL(mipi_dsi_detach);

+/*

• • mipi_dsi_set_maximum_return_packet_size() - specify the maximum size of the
• • the payload in a long packet transmitted from the peripheral back to the
• • host processor
• • @dsi: DSI peripheral device
• • @value: the maximum size of the payload
• • Return: 0 on success or a negative error code on failure.
• */

+int mipi_dsi_set_maximum_return_packet_size(struct mipi_dsi_device *dsi,

• 			    u16 value)
  

+{

• u8 tx[2] = { value & 0xff, value >> 8 };
• struct mipi_dsi_msg msg;
• ssize_t err;
• memset(&msg, 0, sizeof(msg));
• msg.channel = dsi->channel;
• msg.type = MIPI_DSI_SET_MAXIMUM_RETURN_PACKET_SIZE;
• msg.tx_len = sizeof(tx);
• msg.tx_buf = tx;

An alternative (without tx variable) could be:

__cpu_to_le16s(&value); msg.tx_len = sizeof(value); msg.tx_buf = &value;

But it is just pico-optimization.

• err = dsi->host->ops->transfer(dsi->host, &msg);

I guess the sequence: if (dsi->mode_flags & MIPI_DSI_MODE_LPM) msg.flags = MIPI_DSI_MSG_USE_LPM;

should be before this callback.

The same should be in all other helpers calling ops->transfer, so I guess it may be good to move it all to separate function, for example sth like: ... dsi_transfer(dsi, msg) { if (!ops || !ops->transfer) return -NOSYS;

if (dsi->mode_flags & MIPI_DSI_MODE_LPM) msg.flags = MIPI_DSI_MSG_USE_LPM;

return ops->transfer(dsi->host, msg); }

Regards Andrzej

• if (err < 0)

• return err;
  

• return 0;

+} +EXPORT_SYMBOL(mipi_dsi_set_maximum_return_packet_size);

/**

• mipi_dsi_dcs_write_buffer() - transmit a DCS command with payload
• @dsi: DSI peripheral device

diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h index 836cc2b677d0..ef50b5d0de57 100644 --- a/include/drm/drm_mipi_dsi.h +++ b/include/drm/drm_mipi_dsi.h @@ -132,6 +132,8 @@ static inline struct mipi_dsi_device *to_mipi_dsi_device(struct device *dev)

int mipi_dsi_attach(struct mipi_dsi_device *dsi); int mipi_dsi_detach(struct mipi_dsi_device *dsi); +int mipi_dsi_set_maximum_return_packet_size(struct mipi_dsi_device *dsi,

• 			    u16 value);
  

ssize_t mipi_dsi_dcs_write_buffer(struct mipi_dsi_device *dsi, const void *data, size_t len); ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, u8 cmd,

On Mon, Oct 13, 2014 at 04:13:16PM +0200, Andrzej Hajda wrote:

On 10/13/2014 12:16 PM, Thierry Reding wrote:

...

From: Thierry Reding treding@nvidia.com

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

Signed-off-by: Thierry Reding treding@nvidia.com

drivers/gpu/drm/panel/panel-s6e8aa0.c | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-s6e8aa0.c b/drivers/gpu/drm/panel/panel-s6e8aa0.c index 0f85a7c37687..c31e2953f290 100644 --- a/drivers/gpu/drm/panel/panel-s6e8aa0.c +++ b/drivers/gpu/drm/panel/panel-s6e8aa0.c @@ -800,27 +800,15 @@ static void s6e8aa0_panel_init(struct s6e8aa0 *ctx) }

static void s6e8aa0_set_maximum_return_packet_size(struct s6e8aa0 *ctx,

• 				   int size)
  

• 				   u16 size)
  

I guess this whole function should be removed and direct call to mipi_dsi_set_maximum_return_packet_size should be used.

There's additional error handling logic in this function which would have needed extra untangling. Since I don't have the hardware to test this change I kept it to a minimum possible to reduce the risk of accidentally breaking anything.

Feel free to send a follow-on patch cleaning this up as you see fit.

Thierry

On 10/13/2014 12:16 PM, Thierry Reding wrote:

From: Thierry Reding treding@nvidia.com

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

Signed-off-by: Thierry Reding treding@nvidia.com

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

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c index 27fc6dac5e4a..7cd69273dbad 100644 --- a/drivers/gpu/drm/drm_mipi_dsi.c +++ b/drivers/gpu/drm/drm_mipi_dsi.c @@ -229,6 +229,121 @@ int mipi_dsi_set_maximum_return_packet_size(struct mipi_dsi_device *dsi, EXPORT_SYMBOL(mipi_dsi_set_maximum_return_packet_size);

/**

• • mipi_dsi_generic_write() - transmit data using a generic write packet
• • @dsi: DSI peripheral device
• • @payload: buffer containing the payload
• • @size: size of payload buffer
• • This function will automatically choose the right data type depending on
• • the payload length.
• • Return: The number of bytes transmitted on success or a negative error code
• • on failure.
• */

+ssize_t mipi_dsi_generic_write(struct mipi_dsi_device *dsi, const void *payload,

• 	       size_t size)
  

+{

• struct mipi_dsi_msg msg;
• ssize_t err;
• u8 *tx;
• memset(&msg, 0, sizeof(msg));
• msg.channel = dsi->channel;

Again, maybe initializer would be better.

• msg.flags = MIPI_DSI_MSG_USE_LPM | MIPI_DSI_MSG_REQ_ACK;

You should not hardcode flags here, these flags have nothing to do with dsi generic write. But there is general problem of encoding flags in helpers. Possible solutions I see: 1. Translate DSI flags to msg flags as in case of MIPI_DSI_MODE_LPM -> MIPI_DSI_MSG_USE_LPM. 2. Copy dsi.mode_flags to msg.flags, or to dedicated field of msg. 3. Call transfer callback with dsi pointer instead of host, this way it will have access to mode_flags.

• if (size > 2) {

• tx = kmalloc(2 + size, GFP_KERNEL);
  

• if (!tx)
  

• 	return -ENOMEM;
  

• tx[0] = (size >> 0) & 0xff;
  

• tx[1] = (size >> 8) & 0xff;
  

• memcpy(tx + 2, payload, size);
  

• msg.tx_len = 2 + size;
  

• msg.tx_buf = tx;
  

• } else {

• msg.tx_buf = payload;
  

• msg.tx_len = size;
  

• }

Another API breakage, commented already in response to 1st patch.

• switch (size) {
• case 0:

• msg.type = MIPI_DSI_GENERIC_SHORT_WRITE_0_PARAM;
  

• break;
  

• case 1:

• msg.type = MIPI_DSI_GENERIC_SHORT_WRITE_1_PARAM;
  

• break;
  

• case 2:

• msg.type = MIPI_DSI_GENERIC_SHORT_WRITE_2_PARAM;
  

• break;
  

• default:

• msg.type = MIPI_DSI_GENERIC_LONG_WRITE;
  

• break;
  

• }
• err = dsi->host->ops->transfer(dsi->host, &msg);
• if (size > 2)

• kfree(tx);
  

• return err;

+} +EXPORT_SYMBOL(mipi_dsi_generic_write);

+/**

• • mipi_dsi_generic_read() - receive data using a generic read packet
• • @dsi: DSI peripheral device
• • @params: buffer containing the request parameters
• • @num_params: number of request parameters
• • @data: buffer in which to return the received data
• • @size: size of receive buffer
• • This function will automatically choose the right data type depending on
• • the number of parameters passed in.
• • Return: The number of bytes successfully read or a negative error code on
• • failure.
• */

+ssize_t mipi_dsi_generic_read(struct mipi_dsi_device *dsi, const void *params,

• 	      size_t num_params, void *data, size_t size)
  

+{

• struct mipi_dsi_msg msg;
• memset(&msg, 0, sizeof(msg));
• msg.channel = dsi->channel;
• msg.flags = MIPI_DSI_MSG_USE_LPM;
• msg.tx_len = num_params;
• msg.tx_buf = params;
• msg.rx_len = size;
• msg.rx_buf = data;
• switch (num_params) {
• case 0:

• msg.type = MIPI_DSI_GENERIC_READ_REQUEST_0_PARAM;
  

• break;
  

• case 1:

• msg.type = MIPI_DSI_GENERIC_READ_REQUEST_1_PARAM;
  

• break;
  

• case 2:

• msg.type = MIPI_DSI_GENERIC_READ_REQUEST_2_PARAM;
  

• break;
  

• default:

• return -EINVAL;
  

• }
• return dsi->host->ops->transfer(dsi->host, &msg);

+} +EXPORT_SYMBOL(mipi_dsi_generic_read);

+/**

• mipi_dsi_dcs_write_buffer() - transmit a DCS command with payload
• @dsi: DSI peripheral device
• @data: buffer containing data to be transmitted

diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h index ef50b5d0de57..92ca66306702 100644 --- a/include/drm/drm_mipi_dsi.h +++ b/include/drm/drm_mipi_dsi.h @@ -134,6 +134,12 @@ int mipi_dsi_attach(struct mipi_dsi_device *dsi); int mipi_dsi_detach(struct mipi_dsi_device *dsi); int mipi_dsi_set_maximum_return_packet_size(struct mipi_dsi_device *dsi, u16 value);

+ssize_t mipi_dsi_generic_write(struct mipi_dsi_device *dsi, const void *payload,

• 	       size_t size);
  

+ssize_t mipi_dsi_generic_read(struct mipi_dsi_device *dsi, const void *params,

• 	      size_t num_params, void *data, size_t size);
  

ssize_t mipi_dsi_dcs_write_buffer(struct mipi_dsi_device *dsi, const void *data, size_t len); ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, u8 cmd,

On 10/14/2014 12:03 PM, Thierry Reding wrote:

On Tue, Oct 14, 2014 at 10:59:26AM +0200, Andrzej Hajda wrote:

...

On 10/13/2014 12:16 PM, Thierry Reding wrote:

...

From: Thierry Reding treding@nvidia.com

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

Signed-off-by: Thierry Reding treding@nvidia.com

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

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c index 27fc6dac5e4a..7cd69273dbad 100644 --- a/drivers/gpu/drm/drm_mipi_dsi.c +++ b/drivers/gpu/drm/drm_mipi_dsi.c @@ -229,6 +229,121 @@ int mipi_dsi_set_maximum_return_packet_size(struct mipi_dsi_device *dsi, EXPORT_SYMBOL(mipi_dsi_set_maximum_return_packet_size);

/**

• • mipi_dsi_generic_write() - transmit data using a generic write packet
• • @dsi: DSI peripheral device
• • @payload: buffer containing the payload
• • @size: size of payload buffer
• • This function will automatically choose the right data type depending on
• • the payload length.
• • Return: The number of bytes transmitted on success or a negative error code
• • on failure.
• */

+ssize_t mipi_dsi_generic_write(struct mipi_dsi_device *dsi, const void *payload,

• 	       size_t size)
  

+{

• struct mipi_dsi_msg msg;
• ssize_t err;
• u8 *tx;
• memset(&msg, 0, sizeof(msg));
• msg.channel = dsi->channel;

Again, maybe initializer would be better.

Why?

...

...

• msg.flags = MIPI_DSI_MSG_USE_LPM | MIPI_DSI_MSG_REQ_ACK;

You should not hardcode flags here, these flags have nothing to do with dsi generic write.

Agreed, these seem to be left-over from debugging that I overlooked when squashing together various patches.

...

But there is general problem of encoding flags in helpers. Possible solutions I see:

1. Translate DSI flags to msg flags as in case of MIPI_DSI_MODE_LPM ->

MIPI_DSI_MSG_USE_LPM.

That's how it was originally intended. The DSI device's flags would be used to derive message flags, yet wouldn't be an exact copy because not all flags are relevant to message transfers.

OK

There's a bit of an inconsistency here, though. MIPI_DSI_MODE_LPM isn't very clearly defined. It says:

/* transmit data in low power */ #define MIPI_DSI_MODE_LPM BIT(11)

Whereas:

/* use Low Power Mode to transmit message */ #define MIPI_DSI_MSG_USE_LPM BIT(1)

Currently we assume that data == message in the above. However MIPI_DSI_MODE_LPM could also mean that video data is supposed to be transmitted in low-power mode.

I vaguely remember discussing this with you and Inki (?) before in the context of continuous vs. non-continuous clocks, but there didn't seem to be any final outcome on that discussion.

According to specs "Video information should only be transmitted using High Speed Mode"[1], but there is still command mode. Anyway it would be good to clarify that it is only for messages.

[1]: DSI spec 1.2a chapter 4.2.2 Video Mode Operation

According to the specification, version 1.02.00, section 5.2, "The peripheral shall be capable of receiving any transmission in Low Power or High Speed Mode." That would indicate that MIPI_DSI_MSG_USE_LPM is completely unnecessary, unless a device isn't compliant with the spec.

Toshiba bridge TC358764 can be controlled only using LPM commands. IIRC the panel Inki was working on also requires initialization using only LPM. Also your patch "Always use LPM" suggests that you also have non-compliant devices.

Regards Andrzej

On 10/13/2014 12:16 PM, Thierry Reding wrote:

From: YoungJun Cho yj44.cho@samsung.com

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

Signed-off-by: YoungJun Cho yj44.cho@samsung.com [treding: kerneldoc and other minor cleanup] Signed-off-by: Thierry Reding treding@nvidia.com

The code below clearly shows that returning number of written bytes by mipi_dsi_dcs_write is useless and causes only code bloat. If mipi_dsi_dcs_write would return only error these function could be written as static inlines in header file, for example:

static inline int mipi_dsi_dcs_enter_sleep_mode(struct mipi_dsi_device *dsi) { return mipi_dsi_dcs_write(dsi, MIPI_DCS_ENTER_SLEEP_MODE, NULL, 0); }

Regards Andrzej

---

drivers/gpu/drm/drm_mipi_dsi.c | 118 +++++++++++++++++++++++++++++++++++++++++ include/drm/drm_mipi_dsi.h | 19 +++++++ 2 files changed, 137 insertions(+)

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c index 7cd69273dbad..8bb38db861ec 100644 --- a/drivers/gpu/drm/drm_mipi_dsi.c +++ b/drivers/gpu/drm/drm_mipi_dsi.c @@ -506,6 +506,124 @@ ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, u8 cmd, void *data, } EXPORT_SYMBOL(mipi_dsi_dcs_read);

+/**

• • mipi_dsi_dcs_enter_sleep_mode() - disable all unnecessary blocks inside the
• • display module except interface communication
• • @dsi: DSI peripheral device
• • Return: 0 on success or a negative error code on failure.
• */

+int mipi_dsi_dcs_enter_sleep_mode(struct mipi_dsi_device *dsi) +{

• ssize_t err;
• err = mipi_dsi_dcs_write(dsi, MIPI_DCS_ENTER_SLEEP_MODE, NULL, 0);
• if (err < 0)

• return err;
  

• return 0;

+} +EXPORT_SYMBOL(mipi_dsi_dcs_enter_sleep_mode);

+/**

• • mipi_dsi_dcs_exit_sleep_mode() - enable all blocks inside the display
• • module
• • @dsi: DSI peripheral device
• • Return: 0 on success or a negative error code on failure.
• */

+int mipi_dsi_dcs_exit_sleep_mode(struct mipi_dsi_device *dsi) +{

• ssize_t err;
• err = mipi_dsi_dcs_write(dsi, MIPI_DCS_EXIT_SLEEP_MODE, NULL, 0);
• if (err < 0)

• return err;
  

• return 0;

+} +EXPORT_SYMBOL(mipi_dsi_dcs_exit_sleep_mode);

+/**

• • mipi_dsi_dcs_set_display_off() - stop displaying the image data on the
• • display device
• • @dsi: DSI peripheral device
• • Return: 0 on success or a negative error code on failure.
• */

+int mipi_dsi_dcs_set_display_off(struct mipi_dsi_device *dsi) +{

• ssize_t err;
• err = mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_DISPLAY_OFF, NULL, 0);
• if (err < 0)

• return err;
  

• return 0;

+} +EXPORT_SYMBOL(mipi_dsi_dcs_set_display_off);

+/**

• • mipi_dsi_dcs_set_display_on() - start displaying the image data on the
• • display device
• • @dsi: DSI peripheral device
• • Return: 0 on success or a negative error code on failure
• */

+int mipi_dsi_dcs_set_display_on(struct mipi_dsi_device *dsi) +{

• ssize_t err;
• err = mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_DISPLAY_ON, NULL, 0);
• if (err < 0)

• return err;
  

• return 0;

+} +EXPORT_SYMBOL(mipi_dsi_dcs_set_display_on);

+/**

• • mipi_dsi_dcs_set_tear_off() - turn off the display module's Tearing Effect
• • output signal on the TE signal line
• • @dsi: DSI peripheral device
• • Return: 0 on success or a negative error code on failure
• */

+int mipi_dsi_dcs_set_tear_off(struct mipi_dsi_device *dsi) +{

• ssize_t err;
• err = mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_TEAR_OFF, NULL, 0);
• if (err < 0)

• return err;
  

• return 0;

+} +EXPORT_SYMBOL(mipi_dsi_dcs_set_tear_off);

+/**

• • mipi_dsi_dcs_set_tear_on() - turn on the display module's Tearing Effect
• • output signal on the TE signal line.
• • @dsi: DSI peripheral device
• • @mode: the Tearing Effect Output Line mode
• • Return: 0 on success or a negative error code on failure
• */

+int mipi_dsi_dcs_set_tear_on(struct mipi_dsi_device *dsi,

• 	     enum mipi_dsi_dcs_tear_mode mode)
  

+{

• u8 value = mode;
• ssize_t err;
• err = mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_TEAR_ON, &value,

• 		 sizeof(value));
  

• if (err < 0)

• return err;
  

• return 0;

+} +EXPORT_SYMBOL(mipi_dsi_dcs_set_tear_on);

static int mipi_dsi_drv_probe(struct device *dev) { struct mipi_dsi_driver *drv = to_mipi_dsi_driver(dev->driver); diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h index 92ca66306702..bde663725a58 100644 --- a/include/drm/drm_mipi_dsi.h +++ b/include/drm/drm_mipi_dsi.h @@ -140,12 +140,31 @@ ssize_t mipi_dsi_generic_write(struct mipi_dsi_device *dsi, const void *payload, ssize_t mipi_dsi_generic_read(struct mipi_dsi_device *dsi, const void *params, size_t num_params, void *data, size_t size);

+/**

• • enum mipi_dsi_dcs_tear_mode - Tearing Effect Output Line mode
• • @MIPI_DSI_DCS_TEAR_MODE_VBLANK: the TE output line consists of V-Blanking
• • information only
• • @MIPI_DSI_DCS_TEAR_MODE_VHBLANK : the TE output line consists of both
• • V-Blanking and H-Blanking information
• */

+enum mipi_dsi_dcs_tear_mode {

• MIPI_DSI_DCS_TEAR_MODE_VBLANK,
• MIPI_DSI_DCS_TEAR_MODE_VHBLANK,

+};

ssize_t mipi_dsi_dcs_write_buffer(struct mipi_dsi_device *dsi, const void *data, size_t len); ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, u8 cmd, const void *data, size_t len); ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, u8 cmd, void *data, size_t len); +int mipi_dsi_dcs_enter_sleep_mode(struct mipi_dsi_device *dsi); +int mipi_dsi_dcs_exit_sleep_mode(struct mipi_dsi_device *dsi); +int mipi_dsi_dcs_set_display_off(struct mipi_dsi_device *dsi); +int mipi_dsi_dcs_set_display_on(struct mipi_dsi_device *dsi); +int mipi_dsi_dcs_set_tear_off(struct mipi_dsi_device *dsi); +int mipi_dsi_dcs_set_tear_on(struct mipi_dsi_device *dsi,

• 	     enum mipi_dsi_dcs_tear_mode mode);
  
  

/**

• struct mipi_dsi_driver - DSI driver

On 10/13/2014 12:16 PM, Thierry Reding wrote:

From: Thierry Reding treding@nvidia.com

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

Signed-off-by: Thierry Reding treding@nvidia.com

Thanks

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

---

Documentation/DocBook/drm.tmpl | 6 ++++++ drivers/gpu/drm/drm_mipi_dsi.c | 18 ++++++++++++++++-- include/drm/drm_mipi_dsi.h | 16 ++++++++++++++-- 3 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl index be35bc328b77..da733c28c92f 100644 --- a/Documentation/DocBook/drm.tmpl +++ b/Documentation/DocBook/drm.tmpl @@ -2343,6 +2343,12 @@ void intel_crt_init(struct drm_device *dev) !Edrivers/gpu/drm/drm_dp_mst_topology.c </sect2> <sect2>

•  <title>MIPI DSI Helper Functions Reference</title>
  

+!Pdrivers/gpu/drm/drm_mipi_dsi.c dsi helpers +!Iinclude/drm/drm_mipi_dsi.h +!Edrivers/gpu/drm/drm_mipi_dsi.c

• </sect2>

• <sect2> <title>EDID Helper Functions Reference</title>

!Edrivers/gpu/drm/drm_edid.c </sect2> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c index 8bb38db861ec..6749b88a6c72 100644 --- a/drivers/gpu/drm/drm_mipi_dsi.c +++ b/drivers/gpu/drm/drm_mipi_dsi.c @@ -35,6 +35,16 @@

#include <video/mipi_display.h>

+/**

• • DOC: dsi helpers
• • These functions contain some common logic and helpers to deal with MIPI DSI
• • peripherals.
• • Helpers are provided for a number of standard MIPI DSI command as well as a
• • subset of the MIPI DCS command set.
• */

static int mipi_dsi_device_match(struct device *dev, struct device_driver *drv) { return of_driver_match_device(dev, drv); @@ -649,8 +659,10 @@ static void mipi_dsi_drv_shutdown(struct device *dev) }

/**

• • mipi_dsi_driver_register - register a driver for DSI devices

• • mipi_dsi_driver_register() - register a driver for DSI devices
  • @drv: DSI driver structure
• • Return: 0 on success or a negative error code on failure.
  */

int mipi_dsi_driver_register(struct mipi_dsi_driver *drv) { @@ -667,8 +679,10 @@ int mipi_dsi_driver_register(struct mipi_dsi_driver *drv) EXPORT_SYMBOL(mipi_dsi_driver_register);

/**

• • mipi_dsi_driver_unregister - unregister a driver for DSI devices

• • mipi_dsi_driver_unregister() - unregister a driver for DSI devices
  • @drv: DSI driver structure
• • Return: 0 on success or a negative error code on failure.
  */

void mipi_dsi_driver_unregister(struct mipi_dsi_driver *drv) { diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h index bde663725a58..8693711eb623 100644 --- a/include/drm/drm_mipi_dsi.h +++ b/include/drm/drm_mipi_dsi.h @@ -26,6 +26,7 @@ struct mipi_dsi_device;

• struct mipi_dsi_msg - read/write DSI buffer
• @channel: virtual channel id
• @type: payload data type

• • @flags: flags controlling this message transmission
  • @tx_len: length of @tx_buf
  • @tx_buf: data to be written
  • @rx_len: length of @rx_buf

@@ -47,8 +48,19 @@ struct mipi_dsi_msg {

• struct mipi_dsi_host_ops - DSI bus operations
• @attach: attach DSI device to DSI host
• @detach: detach DSI device from DSI host

• • @transfer: send and/or receive DSI packet, return number of received bytes,

• •      or error
    

• • @transfer: transmit a DSI packet
• • DSI packets transmitted by .transfer() are passed in as mipi_dsi_msg
• • structures. This structure contains information about the type of packet
• • being transmitted as well as the transmit and receive buffers. When an
• • error is encountered during transmission, this function will return a
• • negative error code. On success it shall return the number of bytes
• • transmitted for write packets or the number of bytes received for read
• • packets.
• • Note that typically DSI packet transmission is atomic, so the .transfer()
• • function will seldomly return anything other than the number of bytes
• • contained in the transmit buffer on success.
  */

struct mipi_dsi_host_ops { int (*attach)(struct mipi_dsi_host *host,

On 10/13/2014 12:16 PM, Thierry Reding wrote:

From: Thierry Reding treding@nvidia.com

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

Signed-off-by: Thierry Reding treding@nvidia.com

For this and patches 09, 11, 12 the same comments apply as for 06/15. Beside this they look OK to me.

Regards Andrzej

---

drivers/gpu/drm/drm_mipi_dsi.c | 18 ++++++++++++++++++ include/drm/drm_mipi_dsi.h | 1 + 2 files changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c index 6749b88a6c72..c88dcda2f4ac 100644 --- a/drivers/gpu/drm/drm_mipi_dsi.c +++ b/drivers/gpu/drm/drm_mipi_dsi.c @@ -517,6 +517,24 @@ ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, u8 cmd, void *data, EXPORT_SYMBOL(mipi_dsi_dcs_read);

/**

• • mipi_dsi_dcs_nop() - send DCS nop packet
• • @dsi: DSI peripheral device
• • Return: 0 on success or a negative error code on failure.
• */

+int mipi_dsi_dcs_nop(struct mipi_dsi_device *dsi) +{

• ssize_t err;
• err = mipi_dsi_dcs_write(dsi, MIPI_DCS_NOP, NULL, 0);
• if (err < 0)

• return err;
  

• return 0;

+} +EXPORT_SYMBOL(mipi_dsi_dcs_nop);

+/**

• mipi_dsi_dcs_enter_sleep_mode() - disable all unnecessary blocks inside the
• display module except interface communication
• @dsi: DSI peripheral device

diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h index 8693711eb623..be16c8e769bd 100644 --- a/include/drm/drm_mipi_dsi.h +++ b/include/drm/drm_mipi_dsi.h @@ -170,6 +170,7 @@ ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, u8 cmd, const void *data, size_t len); ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, u8 cmd, void *data, size_t len); +int mipi_dsi_dcs_nop(struct mipi_dsi_device *dsi); int mipi_dsi_dcs_enter_sleep_mode(struct mipi_dsi_device *dsi); int mipi_dsi_dcs_exit_sleep_mode(struct mipi_dsi_device *dsi); int mipi_dsi_dcs_set_display_off(struct mipi_dsi_device *dsi);

On Mon, Oct 20, 2014 at 05:56:57PM +0200, Andrzej Hajda wrote:

On 10/13/2014 12:16 PM, Thierry Reding wrote:

...

From: Thierry Reding treding@nvidia.com

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

Signed-off-by: Thierry Reding treding@nvidia.com

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

diff --git a/Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt b/Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt new file mode 100644 index 000000000000..4ab4380ddac8 --- /dev/null +++ b/Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt @@ -0,0 +1,46 @@ +Sharp Microelectronics 10.1" WQXGA TFT LCD panel

+This panel requires a dual-channel DSI host to operate. It supports two modes: +- left-right: each channel drives the left or right half of the screen +- even-odd: each channel drives the even or odd lines of the screen

+Each of the DSI channels controls a separate DSI peripheral. The peripheral +driven by the first link (DSI-LINK1), left or even, is considered the primary +peripheral and controls the device. The 'link2' property contains a phandle +to the peripheral driven by the second link (DSI-LINK2, right or odd).

+Note that in video mode the DSI-LINK1 interface always provides the left/even +pixels and DSI-LINK2 always provides the right/odd pixels. In command mode it +is possible to program either link to drive the left/even or right/odd pixels +but for the sake of consistency this binding assumes that the same assignment +is chosen as for video mode.

+Required properties: +- compatible: should be "sharp,lq101r1sx01" +- link2: phandle to the DSI peripheral on the secondary link. Note that the

• presence of this property marks the containing node as DSI-LINK1.

+- power-supply: phandle of the regulator that provides the supply voltage

+Optional properties: +- backlight: phandle of the backlight device attached to the panel

+Example:

• dsi@54300000 {

• panel: panel@0 {
  

• 	compatible = "sharp,lq101r1sx01";
  

• 	reg = <0>;
  

• 	link2 = <&secondary>;
  

• 	power-supply = <...>;
  

• 	backlight = <...>;
  

• };
  

• };
• dsi@54400000 {

• secondary: panel@0 {
  

• 	compatible = "sharp,lq101r1sx01";
  

• 	reg = <0>;
  

• };
  

• };

The example does not follow rules above, 2nd node does not contain required properties. Maybe it should be clearly stated that power-supply, link2 and backlight properties can be present only in LINK1 node; LINK2 node should contain nothing more than compatible and reg.

I've updated the binding documentation to clarify this.

I guess if it wouldn't be better if different compatibles could be used to distinguish LINK1 and LINK2 nodes, this way you can provide different sets of required/optional properties for both nodes. As a bonus you can have different probe/remove/shutdown callback per link.

I think having separate compatibles isn't entirely accurate. They're both really the same device. There's already the link2 property that distinguishes both halves.

Having separate probe/remove/shutdown isn't something I consider a bonus. It would mean that we need to register a second driver. That is, the device will no longer be driven by a single driver, but rather two drivers that then need to talk to one another again. And it's not like there's a lot to do for DSI-LINK2 in the first place, as you point out yourself.

I'd rather stick with just a single driver and handle the distinction via the link2 property, which we need to get access to the second peripheral anyway.

...

diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile

[...]

...

+static int sharp_panel_write(struct sharp_panel *sharp, u16 offset, u8 value) +{

• u8 payload[3] = { offset >> 8, offset & 0xff, value };
• struct mipi_dsi_device *dsi = sharp->link1;
• ssize_t err;
• err = mipi_dsi_generic_write(dsi, payload, sizeof(payload));
• if (err < 0)

• dev_err(&dsi->dev, "failed to write %02x to %04x: %d\n",
  

• 	value, offset, err);
  

• err = mipi_dsi_dcs_nop(dsi);
• if (err < 0)

• dev_err(&dsi->dev, "failed to send DCS nop: %d\n", err);
  

If mipi_dsi_generic_write fails and mipi_dsi_dcs_nop succeeds function return success, is it OK?

No, this should always return an error if it fails. If these writes don't succeed the panel won't be usable anyway. Also returning an error will actually allow us to decide what to do about the failure in the caller, where more context exists to make a proper decision.

I've fixed this to propagate the error from mipi_dsi_generic_write() and mipi_dsi_dcs_nop().

...

• usleep_range(10, 20);
• return err;

+}

+static __maybe_unused int sharp_panel_read(struct sharp_panel *sharp,

• 			   u16 offset, u8 *value)
  

+{

• ssize_t err;
• cpu_to_be16s(&offset);
• err = mipi_dsi_generic_read(sharp->link1, &offset, sizeof(offset),

• 		    value, sizeof(*value));
  

• if (err < 0)

• dev_err(&sharp->link1->dev, "failed to read from %04x: %d\n",
  

• 	offset, err);
  

• return err;

+}

+static int sharp_panel_disable(struct drm_panel *panel) +{

• struct sharp_panel *sharp = to_sharp_panel(panel);
• if (!sharp->enabled)

• return 0;
  

Shouldn't we assume disable callback can be called only on enabled panel?

That's not something the core does for us currently. We might want to change that at some point, but until then we can simply do what all other panels do.

...

• if (sharp->backlight) {

• sharp->backlight->props.power = FB_BLANK_POWERDOWN;
  

• backlight_update_status(sharp->backlight);
  

• }
• sharp->enabled = false;
• return 0;

+}

+static int sharp_panel_unprepare(struct drm_panel *panel) +{

• struct sharp_panel *sharp = to_sharp_panel(panel);
• int err;
• if (!sharp->prepared)

• return 0;
  

Shouldn't we assume unprepare callback can be called only on enabled panel?

See above.

...

+static int sharp_setup_symmetrical_split(struct mipi_dsi_device *left,

• 			 struct mipi_dsi_device *right,
  

• 			 const struct drm_display_mode *mode)
  

+{

• int err;
• err = mipi_dsi_dcs_set_column_address(left, 0, mode->hdisplay / 2 - 1);
• if (err < 0)

• dev_err(&left->dev, "failed to set column address: %d\n", err);
  

• err = mipi_dsi_dcs_set_page_address(left, 0, mode->vdisplay - 1);
• if (err < 0)

• dev_err(&left->dev, "failed to set page address: %d\n", err);
  

• err = mipi_dsi_dcs_set_column_address(right, mode->hdisplay / 2,

• 			      mode->hdisplay - 1);
  

• if (err < 0)

• dev_err(&right->dev, "failed to set column address: %d\n", err);
  

• err = mipi_dsi_dcs_set_page_address(right, 0, mode->vdisplay - 1);
• if (err < 0)

• dev_err(&right->dev, "failed to set page address: %d\n", err);
  

• return 0;

It always returns 0, maybe it could be void?

I think this should also propagate all errors.

...

+static int sharp_panel_prepare(struct drm_panel *panel) +{

[...]

...

• sharp->prepared = true;
• return err;

You should return 0 here, or if you want to signal error to caller you should unwind changes, I guess regulator_disable should be enough.

Done.

...

+static int sharp_panel_probe(struct mipi_dsi_device *dsi) +{

• struct mipi_dsi_device *secondary = NULL;
• struct sharp_panel *sharp;
• struct device_node *np;
• int err;
• dsi->lanes = 4;
• dsi->format = MIPI_DSI_FMT_RGB888;
• dsi->mode_flags = 0;
• /* Find DSI-LINK1 */
• np = of_parse_phandle(dsi->dev.of_node, "link2", 0);
• if (np) {

• secondary = of_find_mipi_dsi_device_by_node(np);
  

• of_node_put(np);
  

• if (!secondary)
  

• 	return -EPROBE_DEFER;
  

• }
• sharp = devm_kzalloc(&dsi->dev, sizeof(*sharp), GFP_KERNEL);
• if (!sharp)

• return -ENOMEM;
  

Do we really need this for LINK2 device? You can just check if mipi_dsi_get_drvdata is not null to distinguish them, I suppose.

Done.

...

+static int sharp_panel_remove(struct mipi_dsi_device *dsi) +{

• struct sharp_panel *sharp = mipi_dsi_get_drvdata(dsi);
• int err;
• /* only detach from host for the DSI-LINK2 interface */
• if (!sharp->link2) {

• mipi_dsi_detach(dsi);
  

There is no locking mechanism here, so it is possible that everything can crash if someone unbind driver from LINK2 and then try to enable the panel.

I don't think so. Since we're not doing anything with the DSI-LINK2 device anymore it's irrelevant whether it is bound to the driver or not.

...

• return 0;
  

• }
• err = sharp_panel_disable(&sharp->base);
• if (err < 0)

• dev_err(&dsi->dev, "failed to disable panel: %d\n", err);
  

IMHO calling mipi_dsi_detach below should cause connector to call panel disable and unprepare so the call above seems to me unnecessary.

I don't think the connector has any business doing anything with the panel on mipi_dsi_detach(). I suppose we could implement something like that as part of drm_panel_detach(), but that's not the case today, so this simply follows what every other panel has done so far.

...

• err = mipi_dsi_detach(dsi);
• if (err < 0)

• dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", err);
  

• drm_panel_detach(&sharp->base);

drm_panel_attach is called from tegra_dsi_host_attach, wouldn't be more 'symmetrical' to call drm_panel_detach from tegra_dsi_host_detach :)

No, it's not called from tegra_dsi_host_attach(), it's called as part of the DSI output initialization at DRM load time.

drm_panel_detach() really needs to be called from two places: when the panel driver is unloaded and when the connector is unloaded. It seems like this is another area where we may have to put more thought into how to handle it more uniformly across drivers.

Thierry

On Mon, Oct 13, 2014 at 02:29:15PM +0200, Andrzej Hajda wrote:

Hi Thierry,

On 10/13/2014 12:16 PM, 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().

As we discussed before I do not see much symmetry here - dcs read is a kind of write command with non empty response.

The symmetry is in the API. DCS is a command specification, therefore the command is the primary means of identification. Hiding the command inside the payload buffer obfuscates in my opinion. Using an explicit command parameter makes it immediately obvious from the function call what command is being sent without having to look up the static buffer where the command byte is embedded.

This is really the same as for I2C for example. You can use the very low-level i2c_transfer() and it will allow you to perform any type of transfer. That's the equivalent of the DSI host's .transfer() function. On top of that you get helpers like i2c_smbus_write_byte_data() and i2c_smbus_read_byte_data(), which both take a command byte. For writes that command byte will likely be sent in the same transfer as the data, but the important aspect here is that you get a high-level API because it makes it easier or more intuitive to perform the task at hand.

The new function simplifies calls in case write has no arguments, and it complicates it in other cases.

I don't see what is complicated here. The code itself may be slightly more complicated, but I think the advantage of having a consistent API outweighs that.

But the same simplification could be done by just providing inline function, for example: static inline ... mipi_dsi_dcs_write_cmd0(dsi, cmd) { return mipi_dsi_dcs_write(dsi, &cmd, 1); }

You get exactly the same using mipi_dsi_dcs_write(dsi, cmd, NULL, 0) with the implementation proposed here. Again I think that's a pretty natural representation of what's happening: "send command <cmd> with no payload".

The above simplification works only for no parameters, whereas what I proposed works with parameters, too:

mipi_dsi_dcs_write(dsi, cmd, value, sizeof(value));

I essentially allows you to construct DCS commands straight from the specification by filling an array with the parameters as given and passing it as payload for the command.

This way we would have simplified calls for commands without arguments but without complication in other cases.

Really, I don't see what's complicated about it. Yes, the implementation is somewhat more complex and requires an allocation. But there are fast paths in the kernel to deal with those, so I wouldn't expect any of the calls to take significantly longer than if this was written into a statically allocated buffer.

...

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

Signed-off-by: Thierry Reding treding@nvidia.com

Changes in v2:

• provide mipi_dsi_dcs_write_buffer() for backwards compatibility

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

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c index eb6dfe52cab2..1702ffd07986 100644 --- a/drivers/gpu/drm/drm_mipi_dsi.c +++ b/drivers/gpu/drm/drm_mipi_dsi.c @@ -199,33 +199,120 @@ int mipi_dsi_detach(struct mipi_dsi_device *dsi) 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

• • mipi_dsi_dcs_write_buffer() - transmit a DCS command with payload
• • @dsi: DSI peripheral device
• • @data: buffer containing data to be transmitted
• • @len: size of transmission buffer
• • This function will automatically choose the right data type depending on
• • the command payload length.
• • Return: The number of bytes successfully transmitted or a negative error
• • code on failure.
  */

-ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, const void *data,

• 	    size_t len)
  

+ssize_t mipi_dsi_dcs_write_buffer(struct mipi_dsi_device *dsi,

• 		  const void *data, size_t len)
  

{

• const struct mipi_dsi_host_ops *ops = dsi->host->ops; struct mipi_dsi_msg msg = { .channel = dsi->channel, .tx_buf = data, .tx_len = len };

• if (!ops || !ops->transfer)

• if (!dsi->host->ops || !dsi->host->ops->transfer) return -ENOSYS;

  switch (len) { case 0: return -EINVAL;

• case 1: msg.type = MIPI_DSI_DCS_SHORT_WRITE; break;

• case 2: msg.type = MIPI_DSI_DCS_SHORT_WRITE_PARAM; break;

• default:

• msg.type = MIPI_DSI_DCS_LONG_WRITE;
  

• break;
  

• }

• return dsi->host->ops->transfer(dsi->host, &msg);

It is just nitpick, but why did you replaced ops variable by multiple dsi->host->ops expressions? I though it is a good practice to avoid evaluating the same expression multiple times.

I thought I was making this consistent with other functions, but checking again I did exactly the opposite =). As for the evaluation I don't see the point. Any compiler worth its salt will notice that we're using the same object and optimize this, whether we store it in a local variable or not.

Also I find it slightly confusing that we have a local variable for the ops, but then need to do dsi->host to get a reference to the host.

Anyway, I'll remove that change to restore consistency with other functions.

...

+} +EXPORT_SYMBOL(mipi_dsi_dcs_write_buffer);

+/**

• • mipi_dsi_dcs_write() - send DCS write command
• • @dsi: DSI peripheral device
• • @cmd: DCS command
• • @data: buffer containing the command payload
• • @len: command payload length
• • This function will automatically choose the right data type depending on
• • the command payload length.
• • Return: The number of bytes successfully transmitted or a negative error
• • code on failure.
• */

+ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, u8 cmd,

• 	   const void *data, size_t len)
  

+{

• struct mipi_dsi_msg msg;
• ssize_t err;
• size_t size;
• u8 *tx;
• if (!dsi->host->ops || !dsi->host->ops->transfer)

• return -ENOSYS;
  

• if (len > 0) {

• unsigned int offset = 0;
  

• /*
  

•  * DCS long write packets contain the word count in the header
  

•  * bytes 1 and 2 and have a payload containing the DCS command
  

•  * byte folowed by word count minus one bytes.
  

•  *
  

•  * DCS short write packets encode the DCS command and up to
  

•  * one parameter in header bytes 1 and 2.
  

•  */
  

• if (len > 1)
  

• 	size = 3 + len;
  

• else
  

• 	size = 1 + len;
  

• tx = kmalloc(size, GFP_KERNEL);
  

• if (!tx)
  

• 	return -ENOMEM;
  

• /* write word count to header for DCS long write packets */
  

• if (len > 1) {
  

• 	tx[offset++] = ((1 + len) >> 0) & 0xff;
  

• 	tx[offset++] = ((1 + len) >> 8) & 0xff;
  

• }
  

• /* write the DCS command byte followed by the payload */
  

• tx[offset++] = cmd;
  

• memcpy(tx + offset, data, len);
  

• } else {

• tx = &cmd;
  

• size = 1;
  

• }
• memset(&msg, 0, sizeof(msg));
• msg.channel = dsi->channel;
• msg.tx_len = size;
• msg.tx_buf = tx;
• switch (len) {
• case 0:

• msg.type = MIPI_DSI_DCS_SHORT_WRITE;
  

• break;
  

• case 1:

• msg.type = MIPI_DSI_DCS_SHORT_WRITE_PARAM;
  

• break;
  

  default: msg.type = MIPI_DSI_DCS_LONG_WRITE; break;

@@ -234,23 +321,27 @@ ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, const void *data, if (dsi->mode_flags & MIPI_DSI_MODE_LPM) msg.flags = MIPI_DSI_MSG_USE_LPM;

• return ops->transfer(dsi->host, &msg);

• err = dsi->host->ops->transfer(dsi->host, &msg);

I think it would be better to replace the code above (starting from memset) by: err = mipi_dsi_dcs_write_buffer(dsi, tx, size)

Good point, will do.

...

• if (len > 0)

• kfree(tx);
  

• return err;

} EXPORT_SYMBOL(mipi_dsi_dcs_write);

/**

• • mipi_dsi_dcs_read - send DCS read request command
• • @dsi: DSI device
• • @cmd: DCS read command
• • @data: pointer to read buffer
• • @len: length of @data

• • mipi_dsi_dcs_read() - send DCS read request command
• • @dsi: DSI peripheral device
• • @cmd: DCS command
• • @data: buffer in which to receive data
• • @len: size of receive buffer

• • Function returns number of read bytes or error code.

• • Return: The number of bytes read or a negative error code on failure.
  */

ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, u8 cmd, void *data, size_t len) {

• const struct mipi_dsi_host_ops *ops = dsi->host->ops; struct mipi_dsi_msg msg = { .channel = dsi->channel, .type = MIPI_DSI_DCS_READ,

@@ -260,13 +351,13 @@ ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, u8 cmd, void *data, .rx_len = len };

• if (!ops || !ops->transfer)

• if (!dsi->host->ops || !dsi->host->ops->transfer) return -ENOSYS;

  if (dsi->mode_flags & MIPI_DSI_MODE_LPM) msg.flags = MIPI_DSI_MSG_USE_LPM;

• return ops->transfer(dsi->host, &msg);

• return dsi->host->ops->transfer(dsi->host, &msg);

Again, why multiple evaluations of dsi->host->ops vs single one?

I've undone that change for consistency.

Thanks, Thierry

On Tue, Oct 14, 2014 at 10:00:32AM +0200, Andrzej Hajda wrote:

On 10/13/2014 12:16 PM, 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().

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

Signed-off-by: Thierry Reding treding@nvidia.com

Changes in v2:

• provide mipi_dsi_dcs_write_buffer() for backwards compatibility

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

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c index eb6dfe52cab2..1702ffd07986 100644 --- a/drivers/gpu/drm/drm_mipi_dsi.c +++ b/drivers/gpu/drm/drm_mipi_dsi.c @@ -199,33 +199,120 @@ int mipi_dsi_detach(struct mipi_dsi_device *dsi) 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

• • mipi_dsi_dcs_write_buffer() - transmit a DCS command with payload
• • @dsi: DSI peripheral device
• • @data: buffer containing data to be transmitted
• • @len: size of transmission buffer
• • This function will automatically choose the right data type depending on
• • the command payload length.
• • Return: The number of bytes successfully transmitted or a negative error
• • code on failure.
  */

-ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, const void *data,

• 	    size_t len)
  

+ssize_t mipi_dsi_dcs_write_buffer(struct mipi_dsi_device *dsi,

• 		  const void *data, size_t len)
  

{

• const struct mipi_dsi_host_ops *ops = dsi->host->ops; struct mipi_dsi_msg msg = { .channel = dsi->channel, .tx_buf = data, .tx_len = len };

• if (!ops || !ops->transfer)

• if (!dsi->host->ops || !dsi->host->ops->transfer) return -ENOSYS;

  switch (len) { case 0: return -EINVAL;

• case 1: msg.type = MIPI_DSI_DCS_SHORT_WRITE; break;

• case 2: msg.type = MIPI_DSI_DCS_SHORT_WRITE_PARAM; break;

• default:

• msg.type = MIPI_DSI_DCS_LONG_WRITE;
  

• break;
  

• }

• return dsi->host->ops->transfer(dsi->host, &msg);

+} +EXPORT_SYMBOL(mipi_dsi_dcs_write_buffer);

+/**

• • mipi_dsi_dcs_write() - send DCS write command
• • @dsi: DSI peripheral device
• • @cmd: DCS command
• • @data: buffer containing the command payload
• • @len: command payload length
• • This function will automatically choose the right data type depending on
• • the command payload length.
• • Return: The number of bytes successfully transmitted or a negative error
• • code on failure.
• */

+ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, u8 cmd,

• 	   const void *data, size_t len)
  

+{

• struct mipi_dsi_msg msg;
• ssize_t err;
• size_t size;
• u8 *tx;
• if (!dsi->host->ops || !dsi->host->ops->transfer)

• return -ENOSYS;
  

• if (len > 0) {

• unsigned int offset = 0;
  

• /*
  

•  * DCS long write packets contain the word count in the header
  

•  * bytes 1 and 2 and have a payload containing the DCS command
  

•  * byte folowed by word count minus one bytes.
  

•  *
  

•  * DCS short write packets encode the DCS command and up to
  

•  * one parameter in header bytes 1 and 2.
  

•  */
  

• if (len > 1)
  

• 	size = 3 + len;
  

• else
  

• 	size = 1 + len;
  

I guess "size = 2" would be better here.

This is on purpose because it documents the format. If len > 1, then the packet is a long write, so we need three bytes (command & word count) in addition to the payload. For short writes, len <= 1 and we only need one extra byte (command).

...

• tx = kmalloc(size, GFP_KERNEL);
  

• if (!tx)
  

• 	return -ENOMEM;
  

• /* write word count to header for DCS long write packets */
  

• if (len > 1) {
  

• 	tx[offset++] = ((1 + len) >> 0) & 0xff;
  

• 	tx[offset++] = ((1 + len) >> 8) & 0xff;
  

• }
  

• /* write the DCS command byte followed by the payload */
  

• tx[offset++] = cmd;
  

• memcpy(tx + offset, data, len);
  

• } else {

• tx = &cmd;
  

• size = 1;
  

• }

Contents of this conditional is incompatible with the current API. mipi_dsi_msg.tx_buf contains only data and mipi_dsi_msg.tx_len contains lenght of this data. Now you try to embed length of data into tx_buf and this breaks the API.

Huh? Of course the new API has different semantics, but that's the whole point of it. The else branch here is to optimize for the case where a command has no payload. In that case there is no need for allocating an extra buffer, since the command byte is the only data transferred.

And of course changing API would require also changing current users of the API.

There's a single user of this function and this patch switches it over to the compatibility function mipi_dsi_dcs_write_buffer().

But in the first place it would be good to know why do you want to change the API? What are benefits of this solution?

I've already explained this elsewhere.

Thierry

On Tue, Oct 14, 2014 at 12:38:15PM +0200, Andrzej Hajda wrote:

On 10/14/2014 11:44 AM, Thierry Reding wrote:

...

On Tue, Oct 14, 2014 at 10:00:32AM +0200, Andrzej Hajda wrote:

...

On 10/13/2014 12:16 PM, 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().

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

Signed-off-by: Thierry Reding treding@nvidia.com

Changes in v2:

• provide mipi_dsi_dcs_write_buffer() for backwards compatibility

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

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c index eb6dfe52cab2..1702ffd07986 100644 --- a/drivers/gpu/drm/drm_mipi_dsi.c +++ b/drivers/gpu/drm/drm_mipi_dsi.c @@ -199,33 +199,120 @@ int mipi_dsi_detach(struct mipi_dsi_device *dsi) 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

• • mipi_dsi_dcs_write_buffer() - transmit a DCS command with payload
• • @dsi: DSI peripheral device
• • @data: buffer containing data to be transmitted
• • @len: size of transmission buffer
• • This function will automatically choose the right data type depending on
• • the command payload length.
• • Return: The number of bytes successfully transmitted or a negative error
• • code on failure.
  */

-ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, const void *data,

• 	    size_t len)
  

+ssize_t mipi_dsi_dcs_write_buffer(struct mipi_dsi_device *dsi,

• 		  const void *data, size_t len)
  

{

• const struct mipi_dsi_host_ops *ops = dsi->host->ops; struct mipi_dsi_msg msg = { .channel = dsi->channel, .tx_buf = data, .tx_len = len };

• if (!ops || !ops->transfer)

• if (!dsi->host->ops || !dsi->host->ops->transfer) return -ENOSYS;

  switch (len) { case 0: return -EINVAL;

• case 1: msg.type = MIPI_DSI_DCS_SHORT_WRITE; break;

• case 2: msg.type = MIPI_DSI_DCS_SHORT_WRITE_PARAM; break;

• default:

• msg.type = MIPI_DSI_DCS_LONG_WRITE;
  

• break;
  

• }

• return dsi->host->ops->transfer(dsi->host, &msg);

+} +EXPORT_SYMBOL(mipi_dsi_dcs_write_buffer);

+/**

• • mipi_dsi_dcs_write() - send DCS write command
• • @dsi: DSI peripheral device
• • @cmd: DCS command
• • @data: buffer containing the command payload
• • @len: command payload length
• • This function will automatically choose the right data type depending on
• • the command payload length.
• • Return: The number of bytes successfully transmitted or a negative error
• • code on failure.
• */

+ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, u8 cmd,

• 	   const void *data, size_t len)
  

+{

• struct mipi_dsi_msg msg;
• ssize_t err;
• size_t size;
• u8 *tx;
• if (!dsi->host->ops || !dsi->host->ops->transfer)

• return -ENOSYS;
  

• if (len > 0) {

• unsigned int offset = 0;
  

• /*
  

•  * DCS long write packets contain the word count in the header
  

•  * bytes 1 and 2 and have a payload containing the DCS command
  

•  * byte folowed by word count minus one bytes.
  

•  *
  

•  * DCS short write packets encode the DCS command and up to
  

•  * one parameter in header bytes 1 and 2.
  

•  */
  

• if (len > 1)
  

• 	size = 3 + len;
  

• else
  

• 	size = 1 + len;
  

I guess "size = 2" would be better here.

This is on purpose because it documents the format. If len > 1, then the packet is a long write, so we need three bytes (command & word count) in addition to the payload. For short writes, len <= 1 and we only need one extra byte (command).

...

...

• tx = kmalloc(size, GFP_KERNEL);
  

• if (!tx)
  

• 	return -ENOMEM;
  

• /* write word count to header for DCS long write packets */
  

• if (len > 1) {
  

• 	tx[offset++] = ((1 + len) >> 0) & 0xff;
  

• 	tx[offset++] = ((1 + len) >> 8) & 0xff;
  

• }
  

• /* write the DCS command byte followed by the payload */
  

• tx[offset++] = cmd;
  

• memcpy(tx + offset, data, len);
  

• } else {

• tx = &cmd;
  

• size = 1;
  

• }

Contents of this conditional is incompatible with the current API. mipi_dsi_msg.tx_buf contains only data and mipi_dsi_msg.tx_len contains lenght of this data. Now you try to embed length of data into tx_buf and this breaks the API.

Huh? Of course the new API has different semantics, but that's the whole point of it. The else branch here is to optimize for the case where a command has no payload. In that case there is no need for allocating an extra buffer, since the command byte is the only data transferred.

If this is the whole point of it why patch description says nothing about it?

I thought the patch description said this. What do you think needs to be added?

It has nothing to do with helpers symmetry, this is serious API change.

No, it's not. It replaces an existing API, mipi_dsi_dcs_write() with a different one, mipi_dsi_dcs_write_buffer() and converts the one call site where the function is used.

Then it introduces a new function that behaves the same but uses a different signature that takes the DCS command byte as an explicit parameter instead of embedding the DCS command byte into the "payload" buffer.

I don't understand why you think we're changing anything fundamental here.

...

...

And of course changing API would require also changing current users of the API.

There's a single user of this function and this patch switches it over to the compatibility function mipi_dsi_dcs_write_buffer().

Mostly panels are users of these functions and these functions uses transfer callback internally. If we allow different semantic for transfer msg we will end up with panels cooperating only with specific dsi hosts. I do not think it is good direction.

That's not at all the intention. Both functions are supposed to keep working the same way. mipi_dsi_dcs_write_buffer() becomes what was previously called mipi_dsi_dcs_write() and mipi_dsi_dcs_write() is a new helper that concatenates a command byte with payload and passes it into mipi_dsi_dcs_write_buffer().

So if you find that mipi_dsi_dcs_write_buffer() now does the wrong thing and breaks the s6e8aa0 panel, please point out what exactly is going wrong so that I can fix it.

...

...

But in the first place it would be good to know why do you want to change the API? What are benefits of this solution?

I've already explained this elsewhere.

Where? I remember only one discussion where you claimed this is better solution for you [1], but without explanation.

I explained this in an earlier reply to you in this thread.

Do you have patches/repo for tegra with transfer callback implemented with this semantic? Maybe looking at the code will be helpful.

I posted these patches for review to dri-devel yesterday, so they should be available there. If you prefer patchwork, see:

https://patchwork.kernel.org/patch/5075161/

And look for tegra_dsi_host_transfer().

Thierry

On Tue, Oct 14, 2014 at 01:25:44PM +0200, Andrzej Hajda wrote:

On 10/14/2014 12:57 PM, Thierry Reding wrote:

...

On Tue, Oct 14, 2014 at 12:38:15PM +0200, Andrzej Hajda wrote:

...

On 10/14/2014 11:44 AM, Thierry Reding wrote:

...

On Tue, Oct 14, 2014 at 10:00:32AM +0200, Andrzej Hajda wrote:

...

On 10/13/2014 12:16 PM, 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().

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

Signed-off-by: Thierry Reding treding@nvidia.com

Changes in v2:

• provide mipi_dsi_dcs_write_buffer() for backwards compatibility

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

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c index eb6dfe52cab2..1702ffd07986 100644 --- a/drivers/gpu/drm/drm_mipi_dsi.c +++ b/drivers/gpu/drm/drm_mipi_dsi.c @@ -199,33 +199,120 @@ int mipi_dsi_detach(struct mipi_dsi_device *dsi) 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

• • mipi_dsi_dcs_write_buffer() - transmit a DCS command with payload
• • @dsi: DSI peripheral device
• • @data: buffer containing data to be transmitted
• • @len: size of transmission buffer
• • This function will automatically choose the right data type depending on
• • the command payload length.
• • Return: The number of bytes successfully transmitted or a negative error
• • code on failure.
  */

-ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, const void *data,

• 	    size_t len)
  

+ssize_t mipi_dsi_dcs_write_buffer(struct mipi_dsi_device *dsi,

• 		  const void *data, size_t len)
  

{

• const struct mipi_dsi_host_ops *ops = dsi->host->ops; struct mipi_dsi_msg msg = { .channel = dsi->channel, .tx_buf = data, .tx_len = len };

• if (!ops || !ops->transfer)

• if (!dsi->host->ops || !dsi->host->ops->transfer) return -ENOSYS;

  switch (len) { case 0: return -EINVAL;

• case 1: msg.type = MIPI_DSI_DCS_SHORT_WRITE; break;

• case 2: msg.type = MIPI_DSI_DCS_SHORT_WRITE_PARAM; break;

• default:

• msg.type = MIPI_DSI_DCS_LONG_WRITE;
  

• break;
  

• }

• return dsi->host->ops->transfer(dsi->host, &msg);

+} +EXPORT_SYMBOL(mipi_dsi_dcs_write_buffer);

+/**

• • mipi_dsi_dcs_write() - send DCS write command
• • @dsi: DSI peripheral device
• • @cmd: DCS command
• • @data: buffer containing the command payload
• • @len: command payload length
• • This function will automatically choose the right data type depending on
• • the command payload length.
• • Return: The number of bytes successfully transmitted or a negative error
• • code on failure.
• */

+ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, u8 cmd,

• 	   const void *data, size_t len)
  

+{

• struct mipi_dsi_msg msg;
• ssize_t err;
• size_t size;
• u8 *tx;
• if (!dsi->host->ops || !dsi->host->ops->transfer)

• return -ENOSYS;
  

• if (len > 0) {

• unsigned int offset = 0;
  

• /*
  

•  * DCS long write packets contain the word count in the header
  

•  * bytes 1 and 2 and have a payload containing the DCS command
  

•  * byte folowed by word count minus one bytes.
  

•  *
  

•  * DCS short write packets encode the DCS command and up to
  

•  * one parameter in header bytes 1 and 2.
  

•  */
  

• if (len > 1)
  

• 	size = 3 + len;
  

• else
  

• 	size = 1 + len;
  

I guess "size = 2" would be better here.

This is on purpose because it documents the format. If len > 1, then the packet is a long write, so we need three bytes (command & word count) in addition to the payload. For short writes, len <= 1 and we only need one extra byte (command).

...

...

• tx = kmalloc(size, GFP_KERNEL);
  

• if (!tx)
  

• 	return -ENOMEM;
  

• /* write word count to header for DCS long write packets */
  

• if (len > 1) {
  

• 	tx[offset++] = ((1 + len) >> 0) & 0xff;
  

• 	tx[offset++] = ((1 + len) >> 8) & 0xff;
  

• }
  

• /* write the DCS command byte followed by the payload */
  

• tx[offset++] = cmd;
  

• memcpy(tx + offset, data, len);
  

• } else {

• tx = &cmd;
  

• size = 1;
  

• }

Contents of this conditional is incompatible with the current API. mipi_dsi_msg.tx_buf contains only data and mipi_dsi_msg.tx_len contains lenght of this data. Now you try to embed length of data into tx_buf and this breaks the API.

Huh? Of course the new API has different semantics, but that's the whole point of it. The else branch here is to optimize for the case where a command has no payload. In that case there is no need for allocating an extra buffer, since the command byte is the only data transferred.

If this is the whole point of it why patch description says nothing about it?

I thought the patch description said this. What do you think needs to be added?

In short, that new API assumes that transfer callback must interpret write buffer differently than before :) Ie that sometimes at the beginning of the buffer there will be additional bytes.

Right, we never defined exactly which part of code would take which data and into what data it would be transformed. That obviously breaks as soon as two implementations make different assumptions. =)

I'm glad we noticed the disconnect this early, where it's still pretty easy to fix.

...

...

It has nothing to do with helpers symmetry, this is serious API change.

No, it's not. It replaces an existing API, mipi_dsi_dcs_write() with a different one, mipi_dsi_dcs_write_buffer() and converts the one call site where the function is used.

Then it introduces a new function that behaves the same but uses a different signature that takes the DCS command byte as an explicit parameter instead of embedding the DCS command byte into the "payload" buffer.

I don't understand why you think we're changing anything fundamental here.

...

...

...

And of course changing API would require also changing current users of the API.

There's a single user of this function and this patch switches it over to the compatibility function mipi_dsi_dcs_write_buffer().

Mostly panels are users of these functions and these functions uses transfer callback internally. If we allow different semantic for transfer msg we will end up with panels cooperating only with specific dsi hosts. I do not think it is good direction.

That's not at all the intention. Both functions are supposed to keep working the same way. mipi_dsi_dcs_write_buffer() becomes what was previously called mipi_dsi_dcs_write() and mipi_dsi_dcs_write() is a new helper that concatenates a command byte with payload and passes it into mipi_dsi_dcs_write_buffer().

So if you find that mipi_dsi_dcs_write_buffer() now does the wrong thing and breaks the s6e8aa0 panel, please point out what exactly is going wrong so that I can fix it.

The only wrong thing is that s6e8aa0 cannot work with tegra host and lq101r1sx01 cannot work with Exynos. Not big deal at the moment but clearly bad design. I guess there is non-zero chance that we will have a panel which should cooperate with both dsi hosts.

Agreed. The set of bytes transferred to the DSI host should be rigorously defined to make sure they all end up sending the same commands to the panels.

Thierry

On Tue, Oct 14, 2014 at 03:53:26PM +0200, Andrzej Hajda wrote:

On 10/14/2014 01:29 PM, Thierry Reding wrote:

...

On Tue, Oct 14, 2014 at 01:25:44PM +0200, Andrzej Hajda wrote:

...

On 10/14/2014 12:57 PM, Thierry Reding wrote:

...

On Tue, Oct 14, 2014 at 12:38:15PM +0200, Andrzej Hajda wrote:

...

On 10/14/2014 11:44 AM, Thierry Reding wrote:

...

On Tue, Oct 14, 2014 at 10:00:32AM +0200, Andrzej Hajda wrote: > On 10/13/2014 12:16 PM, 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(). >> >> The S6E8AA0 driver relies on the old asymmetric API and there's concern >> that moving to the new API may be less efficient. Provide a new function >> with the old semantics for those cases and make the S6E8AA0 driver use >> it instead. >> >> Signed-off-by: Thierry Reding treding@nvidia.com >> --- >> Changes in v2: >> - provide mipi_dsi_dcs_write_buffer() for backwards compatibility >> >> drivers/gpu/drm/drm_mipi_dsi.c | 127 +++++++++++++++++++++++++++++----- >> drivers/gpu/drm/panel/panel-s6e8aa0.c | 2 +- >> include/drm/drm_mipi_dsi.h | 6 +- >> 3 files changed, 114 insertions(+), 21 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c >> index eb6dfe52cab2..1702ffd07986 100644 >> --- a/drivers/gpu/drm/drm_mipi_dsi.c >> +++ b/drivers/gpu/drm/drm_mipi_dsi.c >> @@ -199,33 +199,120 @@ int mipi_dsi_detach(struct mipi_dsi_device *dsi) >> 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 >> + * mipi_dsi_dcs_write_buffer() - transmit a DCS command with payload >> + * @dsi: DSI peripheral device >> + * @data: buffer containing data to be transmitted >> + * @len: size of transmission buffer >> + * >> + * This function will automatically choose the right data type depending on >> + * the command payload length. >> + * >> + * Return: The number of bytes successfully transmitted or a negative error >> + * code on failure. >> */ >> -ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, const void *data, >> - size_t len) >> +ssize_t mipi_dsi_dcs_write_buffer(struct mipi_dsi_device *dsi, >> + const void *data, size_t len) >> { >> - const struct mipi_dsi_host_ops *ops = dsi->host->ops; >> struct mipi_dsi_msg msg = { >> .channel = dsi->channel, >> .tx_buf = data, >> .tx_len = len >> }; >> >> - if (!ops || !ops->transfer) >> + if (!dsi->host->ops || !dsi->host->ops->transfer) >> return -ENOSYS; >> >> switch (len) { >> case 0: >> return -EINVAL; >> + >> case 1: >> msg.type = MIPI_DSI_DCS_SHORT_WRITE; >> break; >> + >> case 2: >> msg.type = MIPI_DSI_DCS_SHORT_WRITE_PARAM; >> break; >> + >> + default: >> + msg.type = MIPI_DSI_DCS_LONG_WRITE; >> + break; >> + } >> + >> + return dsi->host->ops->transfer(dsi->host, &msg); >> +} >> +EXPORT_SYMBOL(mipi_dsi_dcs_write_buffer); >> + >> +/** >> + * mipi_dsi_dcs_write() - send DCS write command >> + * @dsi: DSI peripheral device >> + * @cmd: DCS command >> + * @data: buffer containing the command payload >> + * @len: command payload length >> + * >> + * This function will automatically choose the right data type depending on >> + * the command payload length. >> + * >> + * Return: The number of bytes successfully transmitted or a negative error >> + * code on failure. >> + */ >> +ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, u8 cmd, >> + const void *data, size_t len) >> +{ >> + struct mipi_dsi_msg msg; >> + ssize_t err; >> + size_t size; >> + u8 *tx; >> + >> + if (!dsi->host->ops || !dsi->host->ops->transfer) >> + return -ENOSYS; >> + >> + if (len > 0) { >> + unsigned int offset = 0; >> + >> + /* >> + * DCS long write packets contain the word count in the header >> + * bytes 1 and 2 and have a payload containing the DCS command >> + * byte folowed by word count minus one bytes. >> + * >> + * DCS short write packets encode the DCS command and up to >> + * one parameter in header bytes 1 and 2. >> + */ >> + if (len > 1) >> + size = 3 + len; >> + else >> + size = 1 + len; > I guess "size = 2" would be better here. This is on purpose because it documents the format. If len > 1, then the packet is a long write, so we need three bytes (command & word count) in addition to the payload. For short writes, len <= 1 and we only need one extra byte (command).

>> + >> + tx = kmalloc(size, GFP_KERNEL); >> + if (!tx) >> + return -ENOMEM; >> + >> + /* write word count to header for DCS long write packets */ >> + if (len > 1) { >> + tx[offset++] = ((1 + len) >> 0) & 0xff; >> + tx[offset++] = ((1 + len) >> 8) & 0xff; >> + } >> + >> + /* write the DCS command byte followed by the payload */ >> + tx[offset++] = cmd; >> + memcpy(tx + offset, data, len); >> + } else { >> + tx = &cmd; >> + size = 1; >> + } > Contents of this conditional is incompatible with the current API. > mipi_dsi_msg.tx_buf contains only data and mipi_dsi_msg.tx_len contains > lenght of this data. Now you try to embed length of data into tx_buf and > this breaks the API. Huh? Of course the new API has different semantics, but that's the whole point of it. The else branch here is to optimize for the case where a command has no payload. In that case there is no need for allocating an extra buffer, since the command byte is the only data transferred.

If this is the whole point of it why patch description says nothing about it?

I thought the patch description said this. What do you think needs to be added?

In short, that new API assumes that transfer callback must interpret write buffer differently than before :) Ie that sometimes at the beginning of the buffer there will be additional bytes.

Right, we never defined exactly which part of code would take which data and into what data it would be transformed. That obviously breaks as soon as two implementations make different assumptions. =)

In previous version of this patch [1] you have made different assumption, and in the discussion you were clearly aware of the current implementation, so your reaction here surprises me little bit. Maybe some misunderstanding. Anyway I am glad we are now both aware of the problem.

It's possible I didn't realize the full complexity of the problem at the time. To summarize, I think the helpers in the core should do as much work as they possibly can, so that drivers don't have to duplicate the same over and over again. That is, the DCS helpers that are under discussion here should create a buffer that reflects the packet that is to be sent to the DSI peripheral, including the specific layout of the header. So a struct mipi_dsi_msg contains the following information:

- .channel + .type make up the DI (Data ID) field in the packet header

for short packets: - .tx_buf[0] and .tx_buf[1] correspond to Data 0 and Data 1 fields in the packet header (both of these are only present if .tx_len is larger than 0 and 1, and default to 0 otherwise)

for long packets: - .tx_buf[0] and .tx_buf[1] correspond to the word count - .tx_buf[2..] represent the payload (word count - 2 bytes)

That's pretty much what section 8.4 General Packet Structure of the DSI specification describes. This intentionally leaves out the header ECC and 16-bit packet footer (checksum). These are typically implemented in hardware, and if they aren't we can provide helpers that compute them based on the header, and the payload in .tx_buf. That way all the packet composition defined in the specification is handled by common code and a driver only needs to have the hardware-specific knowledge, namely where the various pieces need to be written in order to transmit them as desired.

Does that make sense?

Thierry

On Wed, Oct 15, 2014 at 01:01:16PM +0200, Andrzej Hajda wrote:

On 10/14/2014 04:16 PM, Thierry Reding wrote:

...

On Tue, Oct 14, 2014 at 03:53:26PM +0200, Andrzej Hajda wrote:

...

On 10/14/2014 01:29 PM, Thierry Reding wrote:

...

On Tue, Oct 14, 2014 at 01:25:44PM +0200, Andrzej Hajda wrote:

...

On 10/14/2014 12:57 PM, Thierry Reding wrote:

...

On Tue, Oct 14, 2014 at 12:38:15PM +0200, Andrzej Hajda wrote: > On 10/14/2014 11:44 AM, Thierry Reding wrote: >> On Tue, Oct 14, 2014 at 10:00:32AM +0200, Andrzej Hajda wrote: >>> On 10/13/2014 12:16 PM, 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(). >>>> >>>> The S6E8AA0 driver relies on the old asymmetric API and there's concern >>>> that moving to the new API may be less efficient. Provide a new function >>>> with the old semantics for those cases and make the S6E8AA0 driver use >>>> it instead. >>>> >>>> Signed-off-by: Thierry Reding treding@nvidia.com >>>> --- >>>> Changes in v2: >>>> - provide mipi_dsi_dcs_write_buffer() for backwards compatibility >>>> >>>> drivers/gpu/drm/drm_mipi_dsi.c | 127 +++++++++++++++++++++++++++++----- >>>> drivers/gpu/drm/panel/panel-s6e8aa0.c | 2 +- >>>> include/drm/drm_mipi_dsi.h | 6 +- >>>> 3 files changed, 114 insertions(+), 21 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c >>>> index eb6dfe52cab2..1702ffd07986 100644 >>>> --- a/drivers/gpu/drm/drm_mipi_dsi.c >>>> +++ b/drivers/gpu/drm/drm_mipi_dsi.c >>>> @@ -199,33 +199,120 @@ int mipi_dsi_detach(struct mipi_dsi_device *dsi) >>>> 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 >>>> + * mipi_dsi_dcs_write_buffer() - transmit a DCS command with payload >>>> + * @dsi: DSI peripheral device >>>> + * @data: buffer containing data to be transmitted >>>> + * @len: size of transmission buffer >>>> + * >>>> + * This function will automatically choose the right data type depending on >>>> + * the command payload length. >>>> + * >>>> + * Return: The number of bytes successfully transmitted or a negative error >>>> + * code on failure. >>>> */ >>>> -ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, const void *data, >>>> - size_t len) >>>> +ssize_t mipi_dsi_dcs_write_buffer(struct mipi_dsi_device *dsi, >>>> + const void *data, size_t len) >>>> { >>>> - const struct mipi_dsi_host_ops *ops = dsi->host->ops; >>>> struct mipi_dsi_msg msg = { >>>> .channel = dsi->channel, >>>> .tx_buf = data, >>>> .tx_len = len >>>> }; >>>> >>>> - if (!ops || !ops->transfer) >>>> + if (!dsi->host->ops || !dsi->host->ops->transfer) >>>> return -ENOSYS; >>>> >>>> switch (len) { >>>> case 0: >>>> return -EINVAL; >>>> + >>>> case 1: >>>> msg.type = MIPI_DSI_DCS_SHORT_WRITE; >>>> break; >>>> + >>>> case 2: >>>> msg.type = MIPI_DSI_DCS_SHORT_WRITE_PARAM; >>>> break; >>>> + >>>> + default: >>>> + msg.type = MIPI_DSI_DCS_LONG_WRITE; >>>> + break; >>>> + } >>>> + >>>> + return dsi->host->ops->transfer(dsi->host, &msg); >>>> +} >>>> +EXPORT_SYMBOL(mipi_dsi_dcs_write_buffer); >>>> + >>>> +/** >>>> + * mipi_dsi_dcs_write() - send DCS write command >>>> + * @dsi: DSI peripheral device >>>> + * @cmd: DCS command >>>> + * @data: buffer containing the command payload >>>> + * @len: command payload length >>>> + * >>>> + * This function will automatically choose the right data type depending on >>>> + * the command payload length. >>>> + * >>>> + * Return: The number of bytes successfully transmitted or a negative error >>>> + * code on failure. >>>> + */ >>>> +ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, u8 cmd, >>>> + const void *data, size_t len) >>>> +{ >>>> + struct mipi_dsi_msg msg; >>>> + ssize_t err; >>>> + size_t size; >>>> + u8 *tx; >>>> + >>>> + if (!dsi->host->ops || !dsi->host->ops->transfer) >>>> + return -ENOSYS; >>>> + >>>> + if (len > 0) { >>>> + unsigned int offset = 0; >>>> + >>>> + /* >>>> + * DCS long write packets contain the word count in the header >>>> + * bytes 1 and 2 and have a payload containing the DCS command >>>> + * byte folowed by word count minus one bytes. >>>> + * >>>> + * DCS short write packets encode the DCS command and up to >>>> + * one parameter in header bytes 1 and 2. >>>> + */ >>>> + if (len > 1) >>>> + size = 3 + len; >>>> + else >>>> + size = 1 + len; >>> I guess "size = 2" would be better here. >> This is on purpose because it documents the format. If len > 1, then the >> packet is a long write, so we need three bytes (command & word count) in >> addition to the payload. For short writes, len <= 1 and we only need one >> extra byte (command). >> >>>> + >>>> + tx = kmalloc(size, GFP_KERNEL); >>>> + if (!tx) >>>> + return -ENOMEM; >>>> + >>>> + /* write word count to header for DCS long write packets */ >>>> + if (len > 1) { >>>> + tx[offset++] = ((1 + len) >> 0) & 0xff; >>>> + tx[offset++] = ((1 + len) >> 8) & 0xff; >>>> + } >>>> + >>>> + /* write the DCS command byte followed by the payload */ >>>> + tx[offset++] = cmd; >>>> + memcpy(tx + offset, data, len); >>>> + } else { >>>> + tx = &cmd; >>>> + size = 1; >>>> + } >>> Contents of this conditional is incompatible with the current API. >>> mipi_dsi_msg.tx_buf contains only data and mipi_dsi_msg.tx_len contains >>> lenght of this data. Now you try to embed length of data into tx_buf and >>> this breaks the API. >> Huh? Of course the new API has different semantics, but that's the whole >> point of it. The else branch here is to optimize for the case where a >> command has no payload. In that case there is no need for allocating an >> extra buffer, since the command byte is the only data transferred. > If this is the whole point of it why patch description says nothing > about it? I thought the patch description said this. What do you think needs to be added?

In short, that new API assumes that transfer callback must interpret write buffer differently than before :) Ie that sometimes at the beginning of the buffer there will be additional bytes.

Right, we never defined exactly which part of code would take which data and into what data it would be transformed. That obviously breaks as soon as two implementations make different assumptions. =)

In previous version of this patch [1] you have made different assumption, and in the discussion you were clearly aware of the current implementation, so your reaction here surprises me little bit. Maybe some misunderstanding. Anyway I am glad we are now both aware of the problem.

It's possible I didn't realize the full complexity of the problem at the time. To summarize, I think the helpers in the core should do as much work as they possibly can, so that drivers don't have to duplicate the same over and over again. That is, the DCS helpers that are under discussion here should create a buffer that reflects the packet that is to be sent to the DSI peripheral, including the specific layout of the header. So a struct mipi_dsi_msg contains the following information:

• .channel + .type make up the DI (Data ID) field in the packet header

for short packets:

• .tx_buf[0] and .tx_buf[1] correspond to Data 0 and Data 1 fields in the packet header (both of these are only present if .tx_len is larger than 0 and 1, and default to 0 otherwise)

for long packets:

• .tx_buf[0] and .tx_buf[1] correspond to the word count
• .tx_buf[2..] represent the payload (word count - 2 bytes)

That's pretty much what section 8.4 General Packet Structure of the DSI specification describes. This intentionally leaves out the header ECC and 16-bit packet footer (checksum). These are typically implemented in hardware, and if they aren't we can provide helpers that compute them based on the header, and the payload in .tx_buf. That way all the packet composition defined in the specification is handled by common code and a driver only needs to have the hardware-specific knowledge, namely where the various pieces need to be written in order to transmit them as desired.

Does that make sense?

According to DSI specification we can describe DSI message/command/transaction on two levels:

1. Application layer - message is described by a triplet (data_type,

channel_id, data). 2. Protocol layer - message is described as a four byte packet header, optionally followed by packet data (payload) and checksum (which we can skip it here as it should be generated by HW).

In the current API the 1st approach is used. There is no defined standard how to program DSI host to generate specific message, so this approach seems to be the most natural in general case.

On the other side all DSI hosts I looked at use approach based on protocol layer, ie. packet header is written to one FIFO register and payload to another (exynos, intel, omap) or the same (tegra).

Your proposition is something close to 2nd approach, maybe it would be better to convert to completely to 2nd approach:

struct mipi_dsi_msg { u8 header[4]; /* u32 header; ??? */ void *payload; /* NULL in case of short packets */ size_t payload_len; ... };

Anyway, I think conversion to protocol layer should be done by helper but this helper should be called rather from dsi host, otherwise we can encounter some day dsi host which we need to feed with data differently and we will need to perform back-conversion from protocol layer to app layer, it will not be difficult it will be just ugly :)

What about creating helpers to make dsi packets from current dsi message. Sth like:

... drm_mipi_create_packet(struct mipi_dsi_packet *pkt, struct mipi_dsi_msg *msg) { if (long packet) { pkt->header = ...; pkt->payload = msg->tx_buf; pkt->len = msg->tx_len; } else { pkt->header = ...; pkt->payload = NULL; pkt->len = 0; } }

This way in dsi host we will have sth like:

host_transfer(...msg) { struct mipi_dsi_packet pkt;

drm_mipi_create_packet(&pkt, msg);

writel(msg.header, REG_HDR);

for (i = 0; i < pkt.len; i += 4) writel(pkt.payload[i..i+3], REG_PAYLOAD); }

Please note that this way we can avoid dynamic memory allocation/copy/deallocation, I know it is cheap, but it does not seems to be necessary.

Yes, that sounds pretty reasonable on a quick glance. I guess the mipi_dsi_create_packet() function could take an additional parameter at some point to generate the header ECC and checksum for hardware that can't do it on the fly.

I'll give this a shot to see how it's going to work in practice. Given how Exynos currently uses the application layer interface it should be possible to use the helper as a means to transition more easily. Do you plan on converting to the helper once it's available? It seems like it would fit your hardware better than the current approach.

Thierry

On 10/14/2014 11:44 AM, Thierry Reding wrote:

On Tue, Oct 14, 2014 at 10:00:32AM +0200, Andrzej Hajda wrote:

...

On 10/13/2014 12:16 PM, 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().

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

Signed-off-by: Thierry Reding treding@nvidia.com

Changes in v2:

• provide mipi_dsi_dcs_write_buffer() for backwards compatibility

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

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c index eb6dfe52cab2..1702ffd07986 100644 --- a/drivers/gpu/drm/drm_mipi_dsi.c +++ b/drivers/gpu/drm/drm_mipi_dsi.c @@ -199,33 +199,120 @@ int mipi_dsi_detach(struct mipi_dsi_device *dsi) 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

• • mipi_dsi_dcs_write_buffer() - transmit a DCS command with payload
• • @dsi: DSI peripheral device
• • @data: buffer containing data to be transmitted
• • @len: size of transmission buffer
• • This function will automatically choose the right data type depending on
• • the command payload length.
• • Return: The number of bytes successfully transmitted or a negative error
• • code on failure.
  */

-ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, const void *data,

• 	    size_t len)
  

+ssize_t mipi_dsi_dcs_write_buffer(struct mipi_dsi_device *dsi,

• 		  const void *data, size_t len)
  

{

• const struct mipi_dsi_host_ops *ops = dsi->host->ops; struct mipi_dsi_msg msg = { .channel = dsi->channel, .tx_buf = data, .tx_len = len };

• if (!ops || !ops->transfer)

• if (!dsi->host->ops || !dsi->host->ops->transfer) return -ENOSYS;

  switch (len) { case 0: return -EINVAL;

• case 1: msg.type = MIPI_DSI_DCS_SHORT_WRITE; break;

• case 2: msg.type = MIPI_DSI_DCS_SHORT_WRITE_PARAM; break;

• default:

• msg.type = MIPI_DSI_DCS_LONG_WRITE;
  

• break;
  

• }

• return dsi->host->ops->transfer(dsi->host, &msg);

+} +EXPORT_SYMBOL(mipi_dsi_dcs_write_buffer);

+/**

• • mipi_dsi_dcs_write() - send DCS write command
• • @dsi: DSI peripheral device
• • @cmd: DCS command
• • @data: buffer containing the command payload
• • @len: command payload length
• • This function will automatically choose the right data type depending on
• • the command payload length.
• • Return: The number of bytes successfully transmitted or a negative error
• • code on failure.
• */

+ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, u8 cmd,

• 	   const void *data, size_t len)
  

+{

• struct mipi_dsi_msg msg;
• ssize_t err;
• size_t size;
• u8 *tx;
• if (!dsi->host->ops || !dsi->host->ops->transfer)

• return -ENOSYS;
  

• if (len > 0) {

• unsigned int offset = 0;
  

• /*
  

•  * DCS long write packets contain the word count in the header
  

•  * bytes 1 and 2 and have a payload containing the DCS command
  

•  * byte folowed by word count minus one bytes.
  

•  *
  

•  * DCS short write packets encode the DCS command and up to
  

•  * one parameter in header bytes 1 and 2.
  

•  */
  

• if (len > 1)
  

• 	size = 3 + len;
  

• else
  

• 	size = 1 + len;
  

I guess "size = 2" would be better here.

This is on purpose because it documents the format. If len > 1, then the packet is a long write, so we need three bytes (command & word count) in addition to the payload. For short writes, len <= 1 and we only need one extra byte (command).

In such case you can put size calculation outside of the main if. Maybe even you can get rid of size variable and calculate msg.tx_len directly.

Regards Andrzej

...

...

• tx = kmalloc(size, GFP_KERNEL);
  

• if (!tx)
  

• 	return -ENOMEM;
  

• /* write word count to header for DCS long write packets */
  

• if (len > 1) {
  

• 	tx[offset++] = ((1 + len) >> 0) & 0xff;
  

• 	tx[offset++] = ((1 + len) >> 8) & 0xff;
  

• }
  

• /* write the DCS command byte followed by the payload */
  

• tx[offset++] = cmd;
  

• memcpy(tx + offset, data, len);
  

• } else {

• tx = &cmd;
  

• size = 1;
  

• }

Contents of this conditional is incompatible with the current API. mipi_dsi_msg.tx_buf contains only data and mipi_dsi_msg.tx_len contains lenght of this data. Now you try to embed length of data into tx_buf and this breaks the API.

Huh? Of course the new API has different semantics, but that's the whole point of it. The else branch here is to optimize for the case where a command has no payload. In that case there is no need for allocating an extra buffer, since the command byte is the only data transferred.

...

And of course changing API would require also changing current users of the API.

There's a single user of this function and this patch switches it over to the compatibility function mipi_dsi_dcs_write_buffer().

...

But in the first place it would be good to know why do you want to change the API? What are benefits of this solution?

I've already explained this elsewhere.

Thierry