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:
- 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.
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 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.
- Copy dsi.mode_flags to msg.flags, or to dedicated field of msg.
- Call transfer callback with dsi pointer instead of host, this
way it will have access to mode_flags.
The intention was to keep .transfer() agnostic of the device. The idea was that the DSI core would take care of these flags as necessary and host implementations wouldn't have to worry about them.
- 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.
Can you elaborate? How can this be API breakage if it's a new API being introduced?
Thierry