From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrzej Hajda Subject: Re: [PATCH 3/4] drm/dsi: Make mipi_dsi_dcs_{read, write}() symmetrical Date: Wed, 23 Jul 2014 08:32:20 +0200 Message-ID: <53CF56F4.30408@samsung.com> References: <1406013141-18552-1-git-send-email-thierry.reding@gmail.com> <1406013141-18552-3-git-send-email-thierry.reding@gmail.com> <20140722112009.GK15237@phenom.ffwll.local> 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 F2A456E5AC for ; Tue, 22 Jul 2014 23:32:50 -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 <0N9500CQZJIEEX80@mailout4.w1.samsung.com> for dri-devel@lists.freedesktop.org; Wed, 23 Jul 2014 07:32:38 +0100 (BST) In-reply-to: <20140722112009.GK15237@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Daniel Vetter , Thierry Reding Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On 07/22/2014 01:20 PM, Daniel Vetter wrote: > On Tue, Jul 22, 2014 at 09:12:20AM +0200, 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. >> >> Signed-off-by: Thierry Reding > Given the patterns established by other sideband subsystesm like i2c or > the dp aux helpers we have in drm I think this is going into the right > direction. Also, this seems to match somewhat the style we have in our > hand-rolled intel dsi implementation. So I think this makes sense. Could you point me out the patterns you are referring to. Regarding intel dsi implementation I have found in intel_dsi_cmd.h: int dsi_vc_dcs_write(struct intel_dsi *intel_dsi, int channel, const u8 *data, int len); int dsi_vc_dcs_read(struct intel_dsi *intel_dsi, int channel, u8 dcs_cmd, u8 *buf, int buflen); This is the same approach as in the current drm mipi, no forced symmetry. There are also helpers for simple DCS commands: dsi_vc_dcs_write_[01], but they are using dsi_vc_dcs_write internally, which seems to me more reasonable. Regards Andrzej > > Reviewed-by: Daniel Vetter >> --- >> 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(-) >> >> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c >> index 6aa6a9e95570..bbab06ef80c9 100644 >> --- a/drivers/gpu/drm/drm_mipi_dsi.c >> +++ b/drivers/gpu/drm/drm_mipi_dsi.c >> @@ -201,29 +201,41 @@ EXPORT_SYMBOL(mipi_dsi_detach); >> /** >> * mipi_dsi_dcs_write - send DCS write command >> * @dsi: DSI device >> - * @data: pointer to the command followed by parameters >> - * @len: length of @data >> + * @cmd: DCS command >> + * @data: pointer to the command payload >> + * @len: payload length >> */ >> -ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, const void *data, >> - size_t len) >> +ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, u8 cmd, >> + const void *data, size_t len) >> { >> const struct mipi_dsi_host_ops *ops = dsi->host->ops; >> - struct mipi_dsi_msg msg = { >> - .channel = dsi->channel, >> - .tx_buf = data, >> - .tx_len = len >> - }; >> + struct mipi_dsi_msg msg; >> + ssize_t err; >> + u8 *tx; >> >> if (!ops || !ops->transfer) >> return -ENOSYS; >> >> + if (len > 0) { >> + tx = kmalloc(1 + len, GFP_KERNEL); >> + if (!tx) >> + return -ENOMEM; >> + >> + tx[0] = cmd; >> + memcpy(tx + 1, data, len); >> + } else { >> + tx = &cmd; >> + } >> + >> + msg.channel = dsi->channel; >> + msg.tx_len = 1 + len; >> + msg.tx_buf = tx; >> + >> switch (len) { >> case 0: >> - return -EINVAL; >> - case 1: >> msg.type = MIPI_DSI_DCS_SHORT_WRITE; >> break; >> - case 2: >> + case 1: >> msg.type = MIPI_DSI_DCS_SHORT_WRITE_PARAM; >> break; >> default: >> @@ -231,14 +243,19 @@ ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, const void *data, >> break; >> } >> >> - return ops->transfer(dsi->host, &msg); >> + err = ops->transfer(dsi->host, &msg); >> + >> + if (len > 0) >> + kfree(tx); >> + >> + return err; >> } >> EXPORT_SYMBOL(mipi_dsi_dcs_write); >> >> /** >> * mipi_dsi_dcs_read - send DCS read request command >> * @dsi: DSI device >> - * @cmd: DCS read command >> + * @cmd: DCS command >> * @data: pointer to read buffer >> * @len: length of @data >> * >> diff --git a/drivers/gpu/drm/panel/panel-s6e8aa0.c b/drivers/gpu/drm/panel/panel-s6e8aa0.c >> index 5502ef6bc074..d39bee36816c 100644 >> --- a/drivers/gpu/drm/panel/panel-s6e8aa0.c >> +++ b/drivers/gpu/drm/panel/panel-s6e8aa0.c >> @@ -130,7 +130,7 @@ static int s6e8aa0_clear_error(struct s6e8aa0 *ctx) >> return ret; >> } >> >> -static void s6e8aa0_dcs_write(struct s6e8aa0 *ctx, const void *data, size_t len) >> +static void s6e8aa0_dcs_write(struct s6e8aa0 *ctx, const u8 *data, size_t len) >> { >> struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev); >> ssize_t ret; >> @@ -138,7 +138,7 @@ static void s6e8aa0_dcs_write(struct s6e8aa0 *ctx, const void *data, size_t len) >> if (ctx->error < 0) >> return; >> >> - ret = mipi_dsi_dcs_write(dsi, data, len); >> + ret = mipi_dsi_dcs_write(dsi, data[0], data + 1, len - 1); >> if (ret < 0) { >> dev_err(ctx->dev, "error %zd writing dcs seq: %*ph\n", ret, len, >> data); >> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h >> index 7b5e1a9244e1..bea181121e8b 100644 >> --- a/include/drm/drm_mipi_dsi.h >> +++ b/include/drm/drm_mipi_dsi.h >> @@ -127,8 +127,8 @@ struct mipi_dsi_device { >> >> int mipi_dsi_attach(struct mipi_dsi_device *dsi); >> int mipi_dsi_detach(struct mipi_dsi_device *dsi); >> -ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, const void *data, >> - size_t len); >> +ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, u8 cmd, >> + const void *data, size_t len); >> ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, u8 cmd, void *data, >> size_t len); >> >> -- >> 2.0.1 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/dri-devel