[PATCH v2 01/15] drm/dsi: Make mipi_dsi_dcs_{read,write}() symmetrical
Andrzej Hajda
a.hajda at samsung.com
Mon Oct 13 05:29:15 PDT 2014
Hi Thierry,
On 10/13/2014 12:16 PM, Thierry Reding wrote:
> From: Thierry Reding <treding at 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 new function simplifies calls in case write has no arguments,
and it complicates it in other cases.
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);
}
This way we would have simplified calls for commands without arguments
but without complication in other cases.
Anyway as I was not able to not convince you before, I guess I will not
convince you now.
I just recalled my arguments just to show my concerns to other developers.
On the other side as you left the old API I can live with it :)
Below some additional comments.
>
> 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 at 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.
> +}
> +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)
> +
> + 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?
Regards
Andrzej
> }
> EXPORT_SYMBOL(mipi_dsi_dcs_read);
>
> diff --git a/drivers/gpu/drm/panel/panel-s6e8aa0.c b/drivers/gpu/drm/panel/panel-s6e8aa0.c
> index b5217fe37f02..0f85a7c37687 100644
> --- a/drivers/gpu/drm/panel/panel-s6e8aa0.c
> +++ b/drivers/gpu/drm/panel/panel-s6e8aa0.c
> @@ -141,7 +141,7 @@ static void s6e8aa0_dcs_write(struct s6e8aa0 *ctx, const void *data, size_t len)
> if (ctx->error < 0)
> return;
>
> - ret = mipi_dsi_dcs_write(dsi, data, len);
> + ret = mipi_dsi_dcs_write_buffer(dsi, data, len);
> if (ret < 0) {
> dev_err(ctx->dev, "error %zd writing dcs seq: %*ph\n", ret, len,
> data);
> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
> index 8569dc5a1026..ccc3869e137b 100644
> --- a/include/drm/drm_mipi_dsi.h
> +++ b/include/drm/drm_mipi_dsi.h
> @@ -132,8 +132,10 @@ static inline struct mipi_dsi_device *to_mipi_dsi_device(struct device *dev)
>
> int mipi_dsi_attach(struct mipi_dsi_device *dsi);
> int mipi_dsi_detach(struct mipi_dsi_device *dsi);
> -ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, const void *data,
> - size_t len);
> +ssize_t mipi_dsi_dcs_write_buffer(struct mipi_dsi_device *dsi,
> + const void *data, size_t len);
> +ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, u8 cmd,
> + const void *data, size_t len);
> ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, u8 cmd, void *data,
> size_t len);
>
More information about the dri-devel
mailing list