From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [PATCH 3/4] drm/dsi: Make mipi_dsi_dcs_{read, write}() symmetrical Date: Tue, 22 Jul 2014 10:12:39 +0200 Message-ID: <20140722081238.GE18258@ulmo> References: <1406013141-18552-1-git-send-email-thierry.reding@gmail.com> <1406013141-18552-3-git-send-email-thierry.reding@gmail.com> <53CE13AA.5040408@samsung.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1020661141==" Return-path: Received: from mail-wi0-f169.google.com (mail-wi0-f169.google.com [209.85.212.169]) by gabe.freedesktop.org (Postfix) with ESMTP id 232A96E226 for ; Tue, 22 Jul 2014 01:12:42 -0700 (PDT) Received: by mail-wi0-f169.google.com with SMTP id n3so5437638wiv.2 for ; Tue, 22 Jul 2014 01:12:42 -0700 (PDT) In-Reply-To: <53CE13AA.5040408@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 --===============1020661141== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="N1GIdlSm9i+YlY4t" Content-Disposition: inline --N1GIdlSm9i+YlY4t Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Jul 22, 2014 at 09:32:58AM +0200, Andrzej Hajda wrote: > On 07/22/2014 09:12 AM, 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 extra > > command parameter to mipi_dsi_dcs_write(). > > > > Also update the S6E8AA0 panel driver (the only user of this API) to > > adapt to this new API. >=20 > I do not agree with that. DCS read and write are not symmetrical by desig= n: > - write just sends data, > - read sends data then reads response. >=20 > Forcing API to be symmetrical complicates the implementation without real= gain. Why does it complicate anything? Granted we need to dynamically allocate a buffer for each transfer, but it's all hidden away in a common helper function. The big advantage in using a separate parameter for the command is that it makes it trivial to implement the majority of DCS commands, like this: const u8 format =3D 0x77; const u8 mode =3D 0x00; mipi_dsi_dcs_write(dsi, MIPI_DCS_SOFT_RESET, NULL, 0); mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_PIXEL_FORMAT, &format, 1); mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_TEAR_ON, &mode, 1); mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_DISPLAY_ON, NULL, 0); Without the extra parameter, the above becomes: const u8 data[1] =3D { MIPI_DCS_SOFT_RESET }; mipi_dsi_dcs_write(dsi, data, sizeof(data)); const u8 data[2] =3D { MIPI_DCS_SET_PIXEL_FORMAT, 0x77 }; mipi_dsi_dcs_write(dsi, data, sizeof(data)); const u8 data[2] =3D { MIPI_DCS_SET_TEAR_ON, 0x00 }; mipi_dsi_dcs_write(dsi, data, sizeof(data)); const u8 data[1] =3D { MIPI_DCS_SET_DISPLAY_ON }; mipi_dsi_dcs_write(dsi, data, sizeof(data)); In this form it looks still rather readable, but if you need to do this within an initialization function, it become fairly cluttered: const u8 soft_reset[1] =3D { MIPI_DCS_SOFT_RESET }; const u8 set_pixel_format[2] =3D { MIPI_DCS_SET_PIXEL_FORMAT, 0x77 }; const u8 set_tear_on[2] =3D { MIPI_DCS_SET_TEAR_ON, 0x00 }; const u8 set_display_on[1] =3D { MIPI_DCS_SET_DISPLAY_ON }; mipi_dsi_dcs_write(dsi, soft_reset, sizeof(soft_reset)); mipi_dsi_dcs_write(dsi, set_pixel_format, sizeof(set_pixel_format)); mipi_dsi_dcs_write(dsi, set_tear_on, sizeof(set_tear_on)); mipi_dsi_dcs_write(dsi, set_display_on, sizeof(set_display_on)); I find that really hard to read because it has a potentially large gap between the place where the commands are defined and where they're used. Some of this could possibly be alleviated by adding separate functions for standard commands. But either way, I think having this kind of symmetry within an API is always good (it's consistent). By the law of least surprise people will actually expect mipi_dsi_dcs_write() to take a command as a second parameter. Thierry --N1GIdlSm9i+YlY4t Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJTzhz2AAoJEN0jrNd/PrOhDIcP/RYqxTxin56kmXa/Z5+npNdq 7OfxIkTPA1f1mKxTDbGG+kj3Qfah/j00Gy8Q8B12OIa/ZpUJIPc+gLbQFPCCgQ2F I1pCWULzhGyW9AjSZMloltZ0hDYnzPZ8vXyTiDk3gz78oRda8+4tGpjkeYzee61q YaJu9X6cfq4MyXytwJD2hLesVhF//Z1a3jKookfGE4v0epUhp5GT9PrGBh2ZokHI BKcQwJCD72KIHHekI1+U4bRB6DdCr4RjN22e5/WLbKH+wtnY3sj8QXBuvZnuQ+gR giALQ+7zCb4ivzwM3yZIdu6efwbh4QsLu7FcNMw2FC5l2eexyo7lT+aszgYXd0OE u7Sojn41EAgv9VITXmiX0DxIp9e7wKzM00yH4ORPS4t9cF+j9YrnKHlE3DfpgJU+ UBZfTKshYhrvTK7dqD3bC5ze7PRavSIoBFvsmPhzCtu0KdaAQkVdRQD/V3BDrnM4 wy2/RKJl06xV/bUU1GzbyXWacWMOjgiao624z8zZ6InhndGjDKmrq22a1AJQAFg1 MOWDGYBjt+sZYMHD/8dYjyk05PgXdEmI3tyl27TyUwtLoBGErwm0tQXbMKn0Hn80 3ClePuUT8913QkGreTetLHOVbas7clKYEc9dlMfE06Mw5UKCC7nOyblVC3Y2l5I+ ajqjhmf+mXQnes/Vi9Qo =+/wk -----END PGP SIGNATURE----- --N1GIdlSm9i+YlY4t-- --===============1020661141== 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 --===============1020661141==--