On 11/04/2014 03:39 PM, Thierry Reding wrote:
> On Tue, Nov 04, 2014 at 03:21:14PM +0100, Andrzej Hajda wrote:
>> 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:
> [...]
>>>>> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> [...]
>>>>> + 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.
> Right, but it's out of convenience to prevent every driver from doing
> this again. It's part of the help that the helper provides. =)
>
>>>>> +
>>>>> + 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);
> I disagree. =) Having the individual bytes makes their ordering very
> explicit.
>
Wow, you want to keep size field to prevent drivers from adding
sometimes 4 to payload,
but you do not want to simplify header calculation that is much more
complicated :)
Regards
Andrzej
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
