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

[Thierry Reding]

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 at 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 at samsung.com>
> > Signed-off-by: Thierry Reding <treding at 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.

> > +		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.

> > +
> > +	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.

> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20141104/6158d54c/attachment-0001.sig>