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

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

On 11/04/2014 02:58 PM, Thierry Reding wrote:
> On Tue, Nov 04, 2014 at 12:43:21PM +0100, Andrzej Hajda wrote:
>> 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 :)
> That would falsely return true for unspecified data types, too. I'll go
> with a variant that uses an explicit switch.

Sounds better.

>
>>> +		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.
> It's not. Tegra has two FIFOs that can be selected depending on the size
> of a transfer. I use this field to detect which FIFO needs to be
> selected.

But size is always equal payload_length + 4, so it does not carry any
additional information.

>
>>> +
>>> +	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;
>> };
> I'm not sure this is very useful. It's pretty trivial how you
> concatenate the individual bytes and it actually remove any ambiguity
> about the endianness.

This looks better:

val = le16_to_cpu(pkt->header32);
writel(val, REG);

than this:

val = pkt->header[2] << 16 | pkt->header[1] << 8 | pkt->header[0];
writel(val, REG);

But it is just a nitpick.

Regards
Andrzej

>
>> And of course we should document its endiannes.
> The endianness is already documented in the kerneldoc, isn't it? Data ID
> followed by Word Count (long packets) or Packet Data (short packets) and
> finally the ECC byte. That's the ordering defined in the specification,
> so I think it's fairly obvious.
>
> Thierry

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