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: Wed, 23 Jul 2014 09:51:02 +0200 Message-ID: <20140723075100.GE15759@ulmo.nvidia.com> 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> <20140722081238.GE18258@ulmo> <53CE2FD7.6030107@samsung.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0586896005==" Return-path: Received: from mail-pd0-f176.google.com (mail-pd0-f176.google.com [209.85.192.176]) by gabe.freedesktop.org (Postfix) with ESMTP id DD5086E5C5 for ; Wed, 23 Jul 2014 00:51:06 -0700 (PDT) Received: by mail-pd0-f176.google.com with SMTP id y10so1145174pdj.7 for ; Wed, 23 Jul 2014 00:51:06 -0700 (PDT) In-Reply-To: <53CE2FD7.6030107@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 --===============0586896005== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="IU5/I01NYhRvwH70" Content-Disposition: inline --IU5/I01NYhRvwH70 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Jul 22, 2014 at 11:33:11AM +0200, Andrzej Hajda wrote: > On 07/22/2014 10:12 AM, Thierry Reding wrote: > > 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 ext= ra > >>> 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. > >> I do not agree with that. DCS read and write are not symmetrical by de= sign: > >> - write just sends data, > >> - read sends data then reads response. > >> > >> Forcing API to be symmetrical complicates the implementation without r= eal gain. > > Why does it complicate anything? >=20 > Why? Lets see: >=20 > drivers/gpu/drm/drm_mipi_dsi.c | 45 ++++++++++++++++++++++++-----= ------ > drivers/gpu/drm/panel/panel-s6e8aa0.c | 4 ++-- > include/drm/drm_mipi_dsi.h | 4 ++-- > 3 files changed, 35 insertions(+), 18 deletions(-) >=20 >=20 > Original function has two vars, one 'if', one 'switch' and one operation > call, nothing more. > You proposes to add new vars, kmalloc with no memory handling, memcpy, > playing with indices, conditional kfree. Isn't it enough to call it more > complicated? :) It certainly makes the implementation of mipi_dsi_dcs_write() slightly more complicated, but the point is that it makes it easier for users of the API. So the complexity moves into one central location rather than each call site. Ultimately that will reduce overall complexity. > > 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); >=20 > For this I have proposed once more universal macro, sth like this: >=20 > #define mipi_dsi_dcs_write_seq(dsi, seq...) \ > ({\ > const u8 d[] =3D { seq };\ > BUILD_BUG_ON_MSG(ARRAY_SIZE(d) > 64, "DCS sequence too big for stack"= );\ > mipi_dsi_dcs_write(dsi, d, ARRAY_SIZE(d));\ > }) >=20 > It makes trivial to implement ALL DCS commands in much more readable mann= er. > Please compare with your examples above: > mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_SOFT_RESET); > mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_PIXEL_FORMAT, 0x77); > mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_TEAR_ON, 0); > mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_DISPLAY_ON); >=20 > It could be also accompanied by static version, for memory optimization: >=20 > #define mipi_dsi_dcs_write_seq_static(dsi, seq...) \ > ({\ > static const u8 d[] =3D { seq };\ > mipi_dsi_dcs_write(dsi, d, ARRAY_SIZE(d));\ > }) I've said before why I dislike these macros. One alternative for the above would be to add something like a mipi_dsi_dcs_writev() function that takes a variable list of arguments, somewhat like this: ssize_t mipi_dsi_dcs_writev(struct mipi_dsi_device *dsi, u8 command, unsigned int count, ...) { ... } And the above would become: mipi_dsi_dcs_writev(dsi, MIPI_DCS_SOFT_RESET, 0); mipi_dsi_dcs_writev(dsi, MIPI_DCS_SET_PIXEL_FORMAT, 1, 0x77); ... That's still one argument more, but at least it isn't a macro. Thierry >=20 >=20 > > > > 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. >=20 > You will have the same gap issue with your approach in case of DCS > commands with arguments. But it's at least reduced to only the payload. The actual command will still be there on the same line as the function call. You don't have to search for some array and inspect the array to find out which command is being sent. > With 'my' approach it is still readable. Until you mess up the arguments and have to deal with the not-so- readable errors from the compiler. > > 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. >=20 > As I wrote earlier I do not see symmetry here: dcs-read is in fact write > and read, > dcs-write is just write. Hiding this fact in API does not seems to me > good, but I guess > it is rather matter of taste. The symmetry isn't about the physical transfers. It's a logical symmetry. Each DCS command is identified by a command, whether it's a read or a write. There's a similar dissymmetry in how the payload length is handled. Currently peripheral drivers need to encode that within the payload buffer, and DSI host drivers need to parse it out of that depending on the type of write. That makes it needlessly difficult for host drivers and I think the interface would be much easier to use if peripheral drivers specified the payload (and its length) only and let drivers obtain the length of writes from the .tx_len field. Thierry --IU5/I01NYhRvwH70 Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJTz2ljAAoJEN0jrNd/PrOhHiwQAK1nusdwpepNORYwRv/UaKHn BThY9E90LX5KIHrfCMMAlXEg/AEs0ONzYRe3vqNpkDBbv7WEOEOd3ZJaFnTYpqOC lO16kjnIk4r6ys4ZoV5uvcccsAQmxahVvDkHjGZE7uycuS5f8uQf6wSmaD0NTSdk sDqbidXpz88IN7uZR8q2pCuhx1jrGA/ksaLQwuczeNezo7eNdN4zxhEOHGbKt/M1 yYW+ryrb0S2zEEZnP9iYVzaWEfRL0sSJHm1q6Ab46LGQg4Y5NkF+tU4wn0At2mpM sVDEpuKZ4Li1jPfReuVQ/6bDEn0bxKrfl/WnWpBB5OgJ5FvZQVlii7HFaYv7Wwz5 ToXbj4fBqmy9Eo1AAg8A4zbA3mX07+LJkXvQEDUjjmpwT/6oAvfTG4GeA22bjpmm TMV8LyJ6ZaKdZrSuavtm7hQKcMzf+GLVAE7Q4ZI9A4DA4/iWXioVjh3Bm8203w9K 1D8AmEO36lYhuImm/51ZDzvEvsd3/UIG494E2Z3yvtWac0QeHZidTaLsM3sZQE5h Yi3Erzt/cc7OBMoO2A83gWpeI1D+vutSK1H+kDPHGspMq4kmV2voEM92v0m4eCbA Pniok1u4RM8lJ7RU8bk60iHQJ3uOUt+73W23qWj536HQ+gBh5ch5Vl+PjGUvQONG PHvqCqwJ9ov7ItEpQRz/ =Jp2L -----END PGP SIGNATURE----- --IU5/I01NYhRvwH70-- --===============0586896005== 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 --===============0586896005==--