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

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2844 bytes --]

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.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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