From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrzej Hajda Subject: Re: [PATCH 05/15] drm/dsi: Implement generic read and write commands Date: Tue, 14 Oct 2014 10:59:26 +0200 Message-ID: <543CE5EE.1050206@samsung.com> References: <1413195395-3355-1-git-send-email-thierry.reding@gmail.com> <1413195395-3355-5-git-send-email-thierry.reding@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mailout4.w1.samsung.com (mailout4.w1.samsung.com [210.118.77.14]) by gabe.freedesktop.org (Postfix) with ESMTP id 9C45D6E0B8 for ; Tue, 14 Oct 2014 01:59:36 -0700 (PDT) Received: from eucpsbgm2.samsung.com (unknown [203.254.199.245]) by mailout4.w1.samsung.com (Oracle Communications Messaging Server 7u4-24.01(7.0.4.24.0) 64bit (built Nov 17 2011)) with ESMTP id <0NDF00I7GFRWR090@mailout4.w1.samsung.com> for dri-devel@lists.freedesktop.org; Tue, 14 Oct 2014 10:02:20 +0100 (BST) In-reply-to: <1413195395-3355-5-git-send-email-thierry.reding@gmail.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Thierry Reding , dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On 10/13/2014 12:16 PM, Thierry Reding wrote: > From: Thierry Reding > > 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. > > 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(+) > > 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(struct mipi_dsi_device *dsi, > EXPORT_SYMBOL(mipi_dsi_set_maximum_return_packet_size); > > /** > + * mipi_dsi_generic_write() - transmit data using a generic write packet > + * @dsi: DSI peripheral device > + * @payload: buffer containing the payload > + * @size: size of payload buffer > + * > + * This function will automatically choose the right data type depending on > + * the payload length. > + * > + * Return: The number of bytes transmitted on success or a negative error 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 = dsi->channel; Again, maybe initializer would be better. > + msg.flags = MIPI_DSI_MSG_USE_LPM | MIPI_DSI_MSG_REQ_ACK; You should not hardcode flags here, these flags have nothing to do with dsi generic write. 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. 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. > + > + if (size > 2) { > + tx = kmalloc(2 + size, GFP_KERNEL); > + if (!tx) > + return -ENOMEM; > + > + tx[0] = (size >> 0) & 0xff; > + tx[1] = (size >> 8) & 0xff; > + > + memcpy(tx + 2, payload, size); > + > + msg.tx_len = 2 + size; > + msg.tx_buf = tx; > + } else { > + msg.tx_buf = payload; > + msg.tx_len = size; > + } Another API breakage, commented already in response to 1st patch. > + > + switch (size) { > + case 0: > + msg.type = MIPI_DSI_GENERIC_SHORT_WRITE_0_PARAM; > + break; > + > + case 1: > + msg.type = MIPI_DSI_GENERIC_SHORT_WRITE_1_PARAM; > + break; > + > + case 2: > + msg.type = MIPI_DSI_GENERIC_SHORT_WRITE_2_PARAM; > + break; > + > + default: > + msg.type = MIPI_DSI_GENERIC_LONG_WRITE; > + break; > + } > + > + err = dsi->host->ops->transfer(dsi->host, &msg); > + > + if (size > 2) > + kfree(tx); > + > + return err; > +} > +EXPORT_SYMBOL(mipi_dsi_generic_write); > + > +/** > + * mipi_dsi_generic_read() - receive data using a generic read packet > + * @dsi: DSI peripheral device > + * @params: buffer containing the request parameters > + * @num_params: number of request parameters > + * @data: buffer in which to return the received data > + * @size: size of receive buffer > + * > + * This function will automatically choose the right data type depending on > + * the number of parameters passed in. > + * > + * Return: The number of bytes successfully read or a negative error code on > + * failure. > + */ > +ssize_t mipi_dsi_generic_read(struct mipi_dsi_device *dsi, const void *params, > + size_t num_params, void *data, size_t size) > +{ > + struct mipi_dsi_msg msg; > + > + memset(&msg, 0, sizeof(msg)); > + msg.channel = dsi->channel; > + msg.flags = MIPI_DSI_MSG_USE_LPM; > + msg.tx_len = num_params; > + msg.tx_buf = params; > + msg.rx_len = size; > + msg.rx_buf = data; > + > + switch (num_params) { > + case 0: > + msg.type = MIPI_DSI_GENERIC_READ_REQUEST_0_PARAM; > + break; > + > + case 1: > + msg.type = MIPI_DSI_GENERIC_READ_REQUEST_1_PARAM; > + break; > + > + case 2: > + msg.type = MIPI_DSI_GENERIC_READ_REQUEST_2_PARAM; > + break; > + > + default: > + return -EINVAL; > + } > + > + return dsi->host->ops->transfer(dsi->host, &msg); > +} > +EXPORT_SYMBOL(mipi_dsi_generic_read); > + > +/** > * mipi_dsi_dcs_write_buffer() - transmit a DCS command with payload > * @dsi: DSI peripheral device > * @data: buffer containing data to be transmitted > diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h > index ef50b5d0de57..92ca66306702 100644 > --- a/include/drm/drm_mipi_dsi.h > +++ b/include/drm/drm_mipi_dsi.h > @@ -134,6 +134,12 @@ int mipi_dsi_attach(struct mipi_dsi_device *dsi); > int mipi_dsi_detach(struct mipi_dsi_device *dsi); > int mipi_dsi_set_maximum_return_packet_size(struct mipi_dsi_device *dsi, > u16 value); > + > +ssize_t mipi_dsi_generic_write(struct mipi_dsi_device *dsi, const void *payload, > + size_t size); > +ssize_t mipi_dsi_generic_read(struct mipi_dsi_device *dsi, const void *params, > + size_t num_params, void *data, size_t size); > + > ssize_t mipi_dsi_dcs_write_buffer(struct mipi_dsi_device *dsi, > const void *data, size_t len); > ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, u8 cmd, >