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 15:39:31 +0100 Message-ID: <20141104143930.GD31200@ulmo> References: <1415006021-29313-1-git-send-email-thierry.reding@gmail.com> <5458BBD9.8000803@samsung.com> <20141104135847.GB31200@ulmo> <5458E0DA.2050905@samsung.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0561362196==" Return-path: Received: from mail-wg0-f49.google.com (mail-wg0-f49.google.com [74.125.82.49]) by gabe.freedesktop.org (Postfix) with ESMTP id 20FCF6E1ED for ; Tue, 4 Nov 2014 06:39:33 -0800 (PST) Received: by mail-wg0-f49.google.com with SMTP id x13so14915768wgg.8 for ; Tue, 04 Nov 2014 06:39:33 -0800 (PST) In-Reply-To: <5458E0DA.2050905@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 --===============0561362196== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="6WlEvdN9Dv0WHSBl" Content-Disposition: inline --6WlEvdN9Dv0WHSBl Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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_mip= i_dsi.c [...] > >>> + 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; > >> 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. >=20 > 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. =3D) > >>> + > >>> + 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= format > >>> + * @size: size (in bytes) of the packet > >>> + * @header: the four bytes that make up the header (Data ID, Word Co= unt 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 u= nion: > >> 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. >=20 > This looks better: >=20 > val =3D le16_to_cpu(pkt->header32); > writel(val, REG); >=20 > than this: >=20 > val =3D pkt->header[2] << 16 | pkt->header[1] << 8 | pkt->header[0]; > writel(val, REG); I disagree. =3D) Having the individual bytes makes their ordering very explicit. Thierry --6WlEvdN9Dv0WHSBl Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJUWOUiAAoJEN0jrNd/PrOhfBAQAJZMKcVE2XbcL+c3ZVifIeJZ ZyALL+dZsLvJBFB6m7/i9hk4V6mk2eV81AfIZZ7/6wh/n2Ls3ertSZbmaePrgI26 3Xv2GNcd9k6jd48fOF3B/b32/4Kqt+tPcgeO3SeIj8KwOyISDGf7dY1NHFOUQEI2 qg6kci8O+CCveYEOe8CWnOtrjWVVGRWgFpchcjGxZZrqgVSMMG6xIYwrhfk7WMjr 9algYFOvVc2j4CyJbmlm6mDlZhgwCqw1RZBnIr16UXukYToQLYU5JfkruJPe38sE laxxdEJwSazRRFnDB2fx0pLpP5/WcU3nUWsI1BAA9mvYpwMiRYshfCG9OnYWgXea jb8Bml9svYbrNfVAdKMBN3tA7ytEYBy8xZeIoIeaeOueObiPB2ZzFF3QMUJtUvD6 Tx/gvuM1IgNXsZZQC/YY6KPsmnXPWCz79mDQanwuUXla52RLTnz1Em8py0yrUWgR 5RmbMJ6jr/RAZPb76wG/M5Bhm0+2bvRWIPtIO3RSaEbDgET1P4tS1knmut3EhJyR FU5UeQFlWmRyMEJXt0O7cz83tWDXmkRsmPXW7rSYKpF3YjBbn1aDliJZVa/aNBOr Db3vSz1yX/tQO/0wTkJytI4CO9/kIW7Ci3xvU0zGC7T3e3z1eE5bqV/tG/2f9lxi KEnNWPekZ0OKfMGKMRrR =l/7K -----END PGP SIGNATURE----- --6WlEvdN9Dv0WHSBl-- --===============0561362196== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHA6Ly9saXN0 cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK --===============0561362196==--