Re: [PATCH v4 01/16] drm/dsi: Add message to packet translator

[Andrzej Hajda <a.hajda@samsung.com>]

On 11/03/2014 10:13 AM, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> This commit introduces a new function, mipi_dsi_create_packet(), which
> converts from a MIPI DSI message to a MIPI DSI packet. The MIPI DSI
> packet is as close to the protocol described in the DSI specification as
> possible and useful in drivers that need to write a DSI packet into a
> FIFO to send a message off to the peripheral.
>
> Suggested-by: Andrzej Hajda <a.hajda@samsung.com>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/gpu/drm/drm_mipi_dsi.c | 45 ++++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_mipi_dsi.h     | 18 +++++++++++++++++
>  2 files changed, 63 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> index eb6dfe52cab2..76e81aba8220 100644
> --- a/drivers/gpu/drm/drm_mipi_dsi.c
> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> @@ -199,6 +199,51 @@ int mipi_dsi_detach(struct mipi_dsi_device *dsi)
>  EXPORT_SYMBOL(mipi_dsi_detach);
>  
>  /**
> + * mipi_dsi_create_packet - create a packet from a message according to the
> + *     DSI protocol
> + * @packet: pointer to a DSI packet structure
> + * @msg: message to translate into a packet
> + *
> + * Return: 0 on success or a negative error code on failure.
> + */
> +int mipi_dsi_create_packet(struct mipi_dsi_packet *packet,
> +			   const struct mipi_dsi_msg *msg)
> +{
> +	const u8 *tx = msg->tx_buf;
> +
> +	if (!packet || !msg)
> +		return -EINVAL;
> +
> +	memset(packet, 0, sizeof(*packet));
> +	packet->header[0] = ((msg->channel & 0x3) << 6) | (msg->type & 0x3f);
> +
> +	/* TODO: compute ECC if hardware support is not available */
> +
> +	/*
> +	 * Long write packets contain the word count in header bytes 1 and 2.
> +	 * The payload follows the header and is word count bytes long.
> +	 *
> +	 * Short write packets encode up to two parameters in header bytes 1
> +	 * and 2.
> +	 */
> +	if (msg->tx_len > 2) {

This is incorrect, you can have long packet of payload length 0, look for
"zero-byte Data Payload" phrase. I think you should check msg->type here.

I have used:

static bool exynos_dsi_is_short_dsi_type(u8 type)
{
    return (type & 0x0f) <= 8;
}

quite ugly, but works :)

> +		packet->header[1] = (msg->tx_len >> 0) & 0xff;
> +		packet->header[2] = (msg->tx_len >> 8) & 0xff;
> +
> +		packet->payload_length = msg->tx_len;
> +		packet->payload = tx;
> +	} else {
> +		packet->header[1] = (msg->tx_len > 0) ? tx[0] : 0;
> +		packet->header[2] = (msg->tx_len > 1) ? tx[1] : 0;
> +	}
> +
> +	packet->size = sizeof(packet->header) + packet->payload_length;

size seems to be completely useless.

> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(mipi_dsi_create_packet);
> +
> +/**
>   * mipi_dsi_dcs_write - send DCS write command
>   * @dsi: DSI device
>   * @data: pointer to the command followed by parameters
> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
> index 8569dc5a1026..663aa68826f4 100644
> --- a/include/drm/drm_mipi_dsi.h
> +++ b/include/drm/drm_mipi_dsi.h
> @@ -44,6 +44,24 @@ struct mipi_dsi_msg {
>  };
>  
>  /**
> + * struct mipi_dsi_packet - represents a MIPI DSI packet in protocol format
> + * @size: size (in bytes) of the packet
> + * @header: the four bytes that make up the header (Data ID, Word Count or
> + *     Packet Data, and ECC)
> + * @payload_length: number of bytes in the payload
> + * @payload: a pointer to a buffer containing the payload, if any
> + */
> +struct mipi_dsi_packet {
> +	size_t size;
> +	u8 header[4];

I wonder if it wouldn't be good to make it u32 or at least anonymous union:
union {
    u8 header[4];
    u32 header32;
};

And of course we should document its endiannes.
This way we can use le16_to_cpu(pkt->header32) instead of constructing
u32 value from array.

Regards
Andrzej

> +	size_t payload_length;
> +	const u8 *payload;
> +};
> +
> +int mipi_dsi_create_packet(struct mipi_dsi_packet *packet,
> +			   const struct mipi_dsi_msg *msg);
> +
> +/**
>   * struct mipi_dsi_host_ops - DSI bus operations
>   * @attach: attach DSI device to DSI host
>   * @detach: detach DSI device from DSI host

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