From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [PATCH v2 01/15] drm/dsi: Make mipi_dsi_dcs_{read, write}() symmetrical Date: Tue, 14 Oct 2014 12:57:31 +0200 Message-ID: <20141014105730.GH18993@ulmo> References: <1413195395-3355-1-git-send-email-thierry.reding@gmail.com> <543CD820.8080606@samsung.com> <20141014094400.GD18993@ulmo> <543CFD17.10806@samsung.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1621575353==" Return-path: Received: from mail-wg0-f41.google.com (mail-wg0-f41.google.com [74.125.82.41]) by gabe.freedesktop.org (Postfix) with ESMTP id 747FF883C5 for ; Tue, 14 Oct 2014 03:57:34 -0700 (PDT) Received: by mail-wg0-f41.google.com with SMTP id b13so10576723wgh.0 for ; Tue, 14 Oct 2014 03:57:33 -0700 (PDT) In-Reply-To: <543CFD17.10806@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 --===============1621575353== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="CEUtFxTsmBsHRLs3" Content-Disposition: inline --CEUtFxTsmBsHRLs3 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Oct 14, 2014 at 12:38:15PM +0200, Andrzej Hajda wrote: > On 10/14/2014 11:44 AM, Thierry Reding wrote: > > On Tue, Oct 14, 2014 at 10:00:32AM +0200, Andrzej Hajda wrote: > >> On 10/13/2014 12:16 PM, Thierry Reding wrote: > >>> From: Thierry Reding > >>> > >>> Currently the mipi_dsi_dcs_write() function requires the DCS command > >>> byte to be embedded within the write buffer whereas mipi_dsi_dcs_read= () > >>> has a separate parameter. Make them more symmetrical by adding an ext= ra > >>> command parameter to mipi_dsi_dcs_write(). > >>> > >>> The S6E8AA0 driver relies on the old asymmetric API and there's conce= rn > >>> that moving to the new API may be less efficient. Provide a new funct= ion > >>> with the old semantics for those cases and make the S6E8AA0 driver use > >>> it instead. > >>> > >>> Signed-off-by: Thierry Reding > >>> --- > >>> Changes in v2: > >>> - provide mipi_dsi_dcs_write_buffer() for backwards compatibility > >>> > >>> drivers/gpu/drm/drm_mipi_dsi.c | 127 ++++++++++++++++++++++++= +++++----- > >>> drivers/gpu/drm/panel/panel-s6e8aa0.c | 2 +- > >>> include/drm/drm_mipi_dsi.h | 6 +- > >>> 3 files changed, 114 insertions(+), 21 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mip= i_dsi.c > >>> index eb6dfe52cab2..1702ffd07986 100644 > >>> --- a/drivers/gpu/drm/drm_mipi_dsi.c > >>> +++ b/drivers/gpu/drm/drm_mipi_dsi.c > >>> @@ -199,33 +199,120 @@ int mipi_dsi_detach(struct mipi_dsi_device *ds= i) > >>> EXPORT_SYMBOL(mipi_dsi_detach); > >>> =20 > >>> /** > >>> - * mipi_dsi_dcs_write - send DCS write command > >>> - * @dsi: DSI device > >>> - * @data: pointer to the command followed by parameters > >>> - * @len: length of @data > >>> + * mipi_dsi_dcs_write_buffer() - transmit a DCS command with payload > >>> + * @dsi: DSI peripheral device > >>> + * @data: buffer containing data to be transmitted > >>> + * @len: size of transmission buffer > >>> + * > >>> + * This function will automatically choose the right data type depen= ding on > >>> + * the command payload length. > >>> + * > >>> + * Return: The number of bytes successfully transmitted or a negativ= e error > >>> + * code on failure. > >>> */ > >>> -ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, const void *= data, > >>> - size_t len) > >>> +ssize_t mipi_dsi_dcs_write_buffer(struct mipi_dsi_device *dsi, > >>> + const void *data, size_t len) > >>> { > >>> - const struct mipi_dsi_host_ops *ops =3D dsi->host->ops; > >>> struct mipi_dsi_msg msg =3D { > >>> .channel =3D dsi->channel, > >>> .tx_buf =3D data, > >>> .tx_len =3D len > >>> }; > >>> =20 > >>> - if (!ops || !ops->transfer) > >>> + if (!dsi->host->ops || !dsi->host->ops->transfer) > >>> return -ENOSYS; > >>> =20 > >>> switch (len) { > >>> case 0: > >>> return -EINVAL; > >>> + > >>> case 1: > >>> msg.type =3D MIPI_DSI_DCS_SHORT_WRITE; > >>> break; > >>> + > >>> case 2: > >>> msg.type =3D MIPI_DSI_DCS_SHORT_WRITE_PARAM; > >>> break; > >>> + > >>> + default: > >>> + msg.type =3D MIPI_DSI_DCS_LONG_WRITE; > >>> + break; > >>> + } > >>> + > >>> + return dsi->host->ops->transfer(dsi->host, &msg); > >>> +} > >>> +EXPORT_SYMBOL(mipi_dsi_dcs_write_buffer); > >>> + > >>> +/** > >>> + * mipi_dsi_dcs_write() - send DCS write command > >>> + * @dsi: DSI peripheral device > >>> + * @cmd: DCS command > >>> + * @data: buffer containing the command payload > >>> + * @len: command payload length > >>> + * > >>> + * This function will automatically choose the right data type depen= ding on > >>> + * the command payload length. > >>> + * > >>> + * Return: The number of bytes successfully transmitted or a negativ= e error > >>> + * code on failure. > >>> + */ > >>> +ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, u8 cmd, > >>> + const void *data, size_t len) > >>> +{ > >>> + struct mipi_dsi_msg msg; > >>> + ssize_t err; > >>> + size_t size; > >>> + u8 *tx; > >>> + > >>> + if (!dsi->host->ops || !dsi->host->ops->transfer) > >>> + return -ENOSYS; > >>> + > >>> + if (len > 0) { > >>> + unsigned int offset =3D 0; > >>> + > >>> + /* > >>> + * DCS long write packets contain the word count in the header > >>> + * bytes 1 and 2 and have a payload containing the DCS command > >>> + * byte folowed by word count minus one bytes. > >>> + * > >>> + * DCS short write packets encode the DCS command and up to > >>> + * one parameter in header bytes 1 and 2. > >>> + */ > >>> + if (len > 1) > >>> + size =3D 3 + len; > >>> + else > >>> + size =3D 1 + len; > >> I guess "size =3D 2" would be better here. > > This is on purpose because it documents the format. If len > 1, then the > > packet is a long write, so we need three bytes (command & word count) in > > addition to the payload. For short writes, len <=3D 1 and we only need = one > > extra byte (command). > > > >>> + > >>> + tx =3D kmalloc(size, GFP_KERNEL); > >>> + if (!tx) > >>> + return -ENOMEM; > >>> + > >>> + /* write word count to header for DCS long write packets */ > >>> + if (len > 1) { > >>> + tx[offset++] =3D ((1 + len) >> 0) & 0xff; > >>> + tx[offset++] =3D ((1 + len) >> 8) & 0xff; > >>> + } > >>> + > >>> + /* write the DCS command byte followed by the payload */ > >>> + tx[offset++] =3D cmd; > >>> + memcpy(tx + offset, data, len); > >>> + } else { > >>> + tx =3D &cmd; > >>> + size =3D 1; > >>> + } > >> Contents of this conditional is incompatible with the current API. > >> mipi_dsi_msg.tx_buf contains only data and mipi_dsi_msg.tx_len contains > >> lenght of this data. Now you try to embed length of data into tx_buf a= nd > >> this breaks the API. > > Huh? Of course the new API has different semantics, but that's the whole > > point of it. The else branch here is to optimize for the case where a > > command has no payload. In that case there is no need for allocating an > > extra buffer, since the command byte is the only data transferred. >=20 > If this is the whole point of it why patch description says nothing > about it? I thought the patch description said this. What do you think needs to be added? > It has nothing to do with helpers symmetry, this is serious API change. No, it's not. It replaces an existing API, mipi_dsi_dcs_write() with a different one, mipi_dsi_dcs_write_buffer() and converts the one call site where the function is used. Then it introduces a new function that behaves the same but uses a different signature that takes the DCS command byte as an explicit parameter instead of embedding the DCS command byte into the "payload" buffer. I don't understand why you think we're changing anything fundamental here. > >> And of course changing API would require also changing current users of > >> the API. > > There's a single user of this function and this patch switches it over > > to the compatibility function mipi_dsi_dcs_write_buffer(). >=20 > Mostly panels are users of these functions and these functions uses > transfer callback internally. If we allow different semantic for > transfer msg > we will end up with panels cooperating only with specific dsi hosts. > I do not think it is good direction. That's not at all the intention. Both functions are supposed to keep working the same way. mipi_dsi_dcs_write_buffer() becomes what was previously called mipi_dsi_dcs_write() and mipi_dsi_dcs_write() is a new helper that concatenates a command byte with payload and passes it into mipi_dsi_dcs_write_buffer(). So if you find that mipi_dsi_dcs_write_buffer() now does the wrong thing and breaks the s6e8aa0 panel, please point out what exactly is going wrong so that I can fix it. > >> But in the first place it would be good to know why do you want to > >> change the API? What are benefits of this solution? > > I've already explained this elsewhere. >=20 > Where? I remember only one discussion where you claimed this is better > solution > for you [1], but without explanation. I explained this in an earlier reply to you in this thread. > Do you have patches/repo for tegra with transfer callback implemented > with this semantic? > Maybe looking at the code will be helpful. I posted these patches for review to dri-devel yesterday, so they should be available there. If you prefer patchwork, see: https://patchwork.kernel.org/patch/5075161/ And look for tegra_dsi_host_transfer(). Thierry --CEUtFxTsmBsHRLs3 Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJUPQGaAAoJEN0jrNd/PrOhg/QQAJ08bFnEBopN4tMvv/DZZqjy 6bPFtz4MteoRsW/06l8flEHAA7eO/R2bcP280wkZpnimEdDui3vIrwptsQ7zgnvR lSLkd3ENkF9mocXRtIpsgfitk8NkPtXA2Ce+Dj3WuIysXGxMA8iP+OQ/VkFpwlGt NdTgGjLr5GEJZl0cxFiTtoD67QIqPLupPKRucy2/M7uXAbP3Ij4TLrL38rv/m8je jCAI5uzqPcdBPq7v9XAQZFuuP2GY7tLcjJtGSN+dKTWrtm73GJfUztOT4yuERD78 0V6whV904LRVWn8lplYg1UuerELpUIgFrj8plMdZ6E4Gl1TP8Et1GQU1UpmtrsDs /mQio3XiZHSc5I136bVtBt2Kn053bhnFG4mV+QKt30MiLb5SnqUrk9ZQ1E1u3VQu DLnx7S4auDcMIas1J1/rVXIirQsnzWC3i+Zb75kicTRaeJYc8Senxi5nspc24Y6i mrCSgqyBKdDcP0hu86rwwqbfkfAiWse/0VWnoBLatd9QjQuK1Q909rhbMInS9+33 m0yxklWouCus47p5EQZEWfML2bXHAuji4nuD9Nf3AmKKhbLnuNea7izWzr6xJmZF PPKUHBPxpQQDUo0fVPm64Ap5fTBT25prMW2tnzMix6mrufeWiFGoLhwKSabj7f/9 j/vFw1yVbZ9kJmd0V8kY =Uiig -----END PGP SIGNATURE----- --CEUtFxTsmBsHRLs3-- --===============1621575353== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel --===============1621575353==--