From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [PATCH 05/15] drm/dsi: Implement generic read and write commands Date: Tue, 14 Oct 2014 12:03:32 +0200 Message-ID: <20141014100331.GF18993@ulmo> References: <1413195395-3355-1-git-send-email-thierry.reding@gmail.com> <1413195395-3355-5-git-send-email-thierry.reding@gmail.com> <543CE5EE.1050206@samsung.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0974755205==" Return-path: Received: from mail-wi0-f176.google.com (mail-wi0-f176.google.com [209.85.212.176]) by gabe.freedesktop.org (Postfix) with ESMTP id BC5BC6E0D4 for ; Tue, 14 Oct 2014 03:03:35 -0700 (PDT) Received: by mail-wi0-f176.google.com with SMTP id hi2so9623898wib.3 for ; Tue, 14 Oct 2014 03:03:34 -0700 (PDT) In-Reply-To: <543CE5EE.1050206@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 --===============0974755205== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="A9z/3b/E4MkkD+7G" Content-Disposition: inline --A9z/3b/E4MkkD+7G Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Oct 14, 2014 at 10:59:26AM +0200, Andrzej Hajda wrote: > On 10/13/2014 12:16 PM, Thierry Reding wrote: > > From: Thierry Reding > >=20 > > Implement generic read and write commands. Selection of the proper data > > type for packets is done automatically based on the number of parameters > > or payload length. > >=20 > > Signed-off-by: Thierry Reding > > --- > > drivers/gpu/drm/drm_mipi_dsi.c | 115 +++++++++++++++++++++++++++++++++= ++++++++ > > include/drm/drm_mipi_dsi.h | 6 +++ > > 2 files changed, 121 insertions(+) > >=20 > > diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_= dsi.c > > index 27fc6dac5e4a..7cd69273dbad 100644 > > --- a/drivers/gpu/drm/drm_mipi_dsi.c > > +++ b/drivers/gpu/drm/drm_mipi_dsi.c > > @@ -229,6 +229,121 @@ int mipi_dsi_set_maximum_return_packet_size(struc= t mipi_dsi_device *dsi, > > EXPORT_SYMBOL(mipi_dsi_set_maximum_return_packet_size); > > =20 > > /** > > + * mipi_dsi_generic_write() - transmit data using a generic write pack= et > > + * @dsi: DSI peripheral device > > + * @payload: buffer containing the payload > > + * @size: size of payload buffer > > + * > > + * This function will automatically choose the right data type dependi= ng on > > + * the payload length. > > + * > > + * Return: The number of bytes transmitted on success or a negative er= ror code > > + * on failure. > > + */ > > +ssize_t mipi_dsi_generic_write(struct mipi_dsi_device *dsi, const void= *payload, > > + size_t size) > > +{ > > + struct mipi_dsi_msg msg; > > + ssize_t err; > > + u8 *tx; > > + > > + memset(&msg, 0, sizeof(msg)); > > + msg.channel =3D dsi->channel; >=20 > Again, maybe initializer would be better. Why? > > + msg.flags =3D MIPI_DSI_MSG_USE_LPM | MIPI_DSI_MSG_REQ_ACK; >=20 >=20 > You should not hardcode flags here, these flags have nothing to do with > dsi generic write. Agreed, these seem to be left-over from debugging that I overlooked when squashing together various patches. > But there is general problem of encoding flags in > helpers. Possible solutions I see: > 1. Translate DSI flags to msg flags as in case of MIPI_DSI_MODE_LPM -> > MIPI_DSI_MSG_USE_LPM. That's how it was originally intended. The DSI device's flags would be used to derive message flags, yet wouldn't be an exact copy because not all flags are relevant to message transfers. There's a bit of an inconsistency here, though. MIPI_DSI_MODE_LPM isn't very clearly defined. It says: /* transmit data in low power */ #define MIPI_DSI_MODE_LPM BIT(11) Whereas: /* use Low Power Mode to transmit message */ #define MIPI_DSI_MSG_USE_LPM BIT(1) Currently we assume that data =3D=3D message in the above. However MIPI_DSI_MODE_LPM could also mean that video data is supposed to be transmitted in low-power mode. I vaguely remember discussing this with you and Inki (?) before in the context of continuous vs. non-continuous clocks, but there didn't seem to be any final outcome on that discussion. According to the specification, version 1.02.00, section 5.2, "The peripheral shall be capable of receiving any transmission in Low Power or High Speed Mode." That would indicate that MIPI_DSI_MSG_USE_LPM is completely unnecessary, unless a device isn't compliant with the spec. > 2. Copy dsi.mode_flags to msg.flags, or to dedicated field of msg. > 3. Call transfer callback with dsi pointer instead of host, this > way it will have access to mode_flags. The intention was to keep .transfer() agnostic of the device. The idea was that the DSI core would take care of these flags as necessary and host implementations wouldn't have to worry about them. > > + > > + if (size > 2) { > > + tx =3D kmalloc(2 + size, GFP_KERNEL); > > + if (!tx) > > + return -ENOMEM; > > + > > + tx[0] =3D (size >> 0) & 0xff; > > + tx[1] =3D (size >> 8) & 0xff; > > + > > + memcpy(tx + 2, payload, size); > > + > > + msg.tx_len =3D 2 + size; > > + msg.tx_buf =3D tx; > > + } else { > > + msg.tx_buf =3D payload; > > + msg.tx_len =3D size; > > + } >=20 > Another API breakage, commented already in response to 1st patch. Can you elaborate? How can this be API breakage if it's a new API being introduced? Thierry --A9z/3b/E4MkkD+7G Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJUPPTzAAoJEN0jrNd/PrOhN7AP/i61ETQLKCp7zHnXx0/J3N3w /IZpJ4LVDP3EJmNT9sGKST4HIJ39om1h+b9+9XqFk5FrBif3LhoF455t+CF/foa4 /FxKekyHgBjeo/Oork3JK4nTijE+neGUdUqcZ8O0kcivvUy0WrFIbIymzbaF/xTw lZp56ftOZ9102qbODamvV7jRae7oH6ICrpnN7dWt9Rrutsl2vuwqEXTWswYKULRP dCdNzCpcaswfUrJedU20DwPDiM58lDpiCu4NMRrG28rWjtQHQOSqAe9Yp7otl/XP tcCRvU1SzcWKflbpgJIcpcnjzjqdnyHVnWmSAh7nNSvRYTg94KDbmwqmtP1aKsUY 1CosCDvwMZzfFNDvwsj1dzZZ2F0nHHL58+ImAXKz7yupdvdKnwe4LDa1NaHDX70Y smZPTPJDo2NHfwe/AANe9Utmx8MDjCCQckfi4VTjTshNw2Llh98Bj+KxZWZ8jVpu bhHLLoT7O7X3ZHQOGNCUVia5+1TvGngoDijm8hCVwgV9yH07jEITwyVqTkQDBBjl A95BbHxMlRpMNmQhDsf9f0k/sOa+kTj17cFAsNyuBLk3jbfcDcoSbR6YvZYbGcRF 8Pxu3fsZP5dGOKlvMtUVu8SbgfWgwOcvL8/8AC2Kz5OLjQBfbD2eaoC4GWCXO5WG uDvyhx0CZAAtyTK5Xfh4 =XJi/ -----END PGP SIGNATURE----- --A9z/3b/E4MkkD+7G-- --===============0974755205== 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 --===============0974755205==--