From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [PATCH v4 01/16] drm/dsi: Add message to packet translator Date: Tue, 4 Nov 2014 14:58:49 +0100 Message-ID: <20141104135847.GB31200@ulmo> References: <1415006021-29313-1-git-send-email-thierry.reding@gmail.com> <5458BBD9.8000803@samsung.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1417585685==" Return-path: Received: from mail-wg0-f42.google.com (mail-wg0-f42.google.com [74.125.82.42]) by gabe.freedesktop.org (Postfix) with ESMTP id DA3366E22A for ; Tue, 4 Nov 2014 05:58:51 -0800 (PST) Received: by mail-wg0-f42.google.com with SMTP id k14so14958116wgh.1 for ; Tue, 04 Nov 2014 05:58:50 -0800 (PST) In-Reply-To: <5458BBD9.8000803@samsung.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Andrzej Hajda Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============1417585685== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="i0/AhcQY5QxfSsSZ" Content-Disposition: inline --i0/AhcQY5QxfSsSZ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 > > > > 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 > > Signed-off-by: Thierry Reding > > --- > > 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); > > =20 > > /** > > + * mipi_dsi_create_packet - create a packet from a message according t= o 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 =3D msg->tx_buf; > > + > > + if (!packet || !msg) > > + return -EINVAL; > > + > > + memset(packet, 0, sizeof(*packet)); > > + packet->header[0] =3D ((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) { >=20 > 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. >=20 > I have used: >=20 > static bool exynos_dsi_is_short_dsi_type(u8 type) > { > return (type & 0x0f) <=3D 8; > } >=20 > 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] =3D (msg->tx_len >> 0) & 0xff; > > + packet->header[2] =3D (msg->tx_len >> 8) & 0xff; > > + > > + packet->payload_length =3D msg->tx_len; > > + packet->payload =3D tx; > > + } else { > > + packet->header[1] =3D (msg->tx_len > 0) ? tx[0] : 0; > > + packet->header[2] =3D (msg->tx_len > 1) ? tx[1] : 0; > > + } > > + > > + packet->size =3D sizeof(packet->header) + packet->payload_length; >=20 > 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 { > > }; > > =20 > > /** > > + * struct mipi_dsi_packet - represents a MIPI DSI packet in protocol f= ormat > > + * @size: size (in bytes) of the packet > > + * @header: the four bytes that make up the header (Data ID, Word Coun= t 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]; >=20 > I wonder if it wouldn't be good to make it u32 or at least anonymous unio= n: > 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 --i0/AhcQY5QxfSsSZ Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJUWNuXAAoJEN0jrNd/PrOhFzIP/jovqqb3iJRJ+RqjPZtX0q/P luGYrhGPMJHxuvj1133ZrvdyXd/2b9jMjcBMQ+XDcI+B44aLbn/mUJYSCWfKafES F9d6uJpEf5S7S8x81SgxLs2QrFtsaLUtHtVOcxuKeaRTQsy+C8uofLKCVRlspn7J jcPpzUYcTeEZpflWEF2iFxCQ7Bm8gIUbfS420HFZVeZVngRG8EECgwgUnd2/E9vA lbnkgxy5fnoRuq5Rapdud6fM7l/GVY+4YlkUGtjrMzFXd9S4qVjtCdaQxg3dQOoo ctqrVPT/tCkICewwRAOF6iDy52DJO8pde8+zTGQPVodYFXR4rfAUsqVwir6jaEHA yk/U0OTdvI08Hn0/bsrlcgO7n0qkTtxoAwoSFMmdbIg7fE8sfOY2NHGgFR3lu6P1 VoFbK4CArL4QIXTTMFotfkmvCNGwEgo0LIlwKHmZZjJ0CcCq1IKgfaRynB4wrjyz gfai6wSFB1hUQLrDkaGNBAmHvgkm2Mdab7rIETRhWSpPbqxcm32md5CDTC7sWLfK /aBrPHSDuxn1Vmk6hPmIo8cSONwPKVQJ1bkCysZKCo64m+gc407+m18G6wejBbTF 4Pg3JR9Pv8y9GwbmPCSWvRxC527t4VUCAVIKWCp7G6r1GcTyCuEbwobV+YLGTB3f dDOLj9o9hQJ8VI0dCuiB =jQlb -----END PGP SIGNATURE----- --i0/AhcQY5QxfSsSZ-- --===============1417585685== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHA6Ly9saXN0 cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK --===============1417585685==--