[PATCH v2 01/15] drm/dsi: Make mipi_dsi_dcs_{read, write}() symmetrical
Thierry Reding
thierry.reding at gmail.com
Tue Oct 14 03:57:31 PDT 2014
On Tue, Oct 14, 2014 at 12:38:15PM +0200, Andrzej Hajda wrote:
> On 10/14/2014 11:44 AM, Thierry Reding wrote:
> > On Tue, Oct 14, 2014 at 10:00:32AM +0200, Andrzej Hajda wrote:
> >> On 10/13/2014 12:16 PM, Thierry Reding wrote:
> >>> From: Thierry Reding <treding at nvidia.com>
> >>>
> >>> 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().
> >>>
> >>> The S6E8AA0 driver relies on the old asymmetric API and there's concern
> >>> that moving to the new API may be less efficient. Provide a new function
> >>> with the old semantics for those cases and make the S6E8AA0 driver use
> >>> it instead.
> >>>
> >>> Signed-off-by: Thierry Reding <treding at nvidia.com>
> >>> ---
> >>> Changes in v2:
> >>> - provide mipi_dsi_dcs_write_buffer() for backwards compatibility
> >>>
> >>> drivers/gpu/drm/drm_mipi_dsi.c | 127 +++++++++++++++++++++++++++++-----
> >>> drivers/gpu/drm/panel/panel-s6e8aa0.c | 2 +-
> >>> include/drm/drm_mipi_dsi.h | 6 +-
> >>> 3 files changed, 114 insertions(+), 21 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> >>> index eb6dfe52cab2..1702ffd07986 100644
> >>> --- a/drivers/gpu/drm/drm_mipi_dsi.c
> >>> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> >>> @@ -199,33 +199,120 @@ int mipi_dsi_detach(struct mipi_dsi_device *dsi)
> >>> 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
> >>> + * mipi_dsi_dcs_write_buffer() - transmit a DCS command with payload
> >>> + * @dsi: DSI peripheral device
> >>> + * @data: buffer containing data to be transmitted
> >>> + * @len: size of transmission buffer
> >>> + *
> >>> + * This function will automatically choose the right data type depending on
> >>> + * the command payload length.
> >>> + *
> >>> + * Return: The number of bytes successfully transmitted or a negative error
> >>> + * code on failure.
> >>> */
> >>> -ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, const void *data,
> >>> - size_t len)
> >>> +ssize_t mipi_dsi_dcs_write_buffer(struct mipi_dsi_device *dsi,
> >>> + 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
> >>> };
> >>>
> >>> - if (!ops || !ops->transfer)
> >>> + if (!dsi->host->ops || !dsi->host->ops->transfer)
> >>> return -ENOSYS;
> >>>
> >>> switch (len) {
> >>> case 0:
> >>> return -EINVAL;
> >>> +
> >>> case 1:
> >>> msg.type = MIPI_DSI_DCS_SHORT_WRITE;
> >>> break;
> >>> +
> >>> case 2:
> >>> msg.type = MIPI_DSI_DCS_SHORT_WRITE_PARAM;
> >>> break;
> >>> +
> >>> + default:
> >>> + msg.type = MIPI_DSI_DCS_LONG_WRITE;
> >>> + break;
> >>> + }
> >>> +
> >>> + return dsi->host->ops->transfer(dsi->host, &msg);
> >>> +}
> >>> +EXPORT_SYMBOL(mipi_dsi_dcs_write_buffer);
> >>> +
> >>> +/**
> >>> + * mipi_dsi_dcs_write() - send DCS write command
> >>> + * @dsi: DSI peripheral device
> >>> + * @cmd: DCS command
> >>> + * @data: buffer containing the command payload
> >>> + * @len: command payload length
> >>> + *
> >>> + * This function will automatically choose the right data type depending on
> >>> + * the command payload length.
> >>> + *
> >>> + * Return: The number of bytes successfully transmitted or a negative error
> >>> + * code on failure.
> >>> + */
> >>> +ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, u8 cmd,
> >>> + const void *data, size_t len)
> >>> +{
> >>> + struct mipi_dsi_msg msg;
> >>> + ssize_t err;
> >>> + size_t size;
> >>> + u8 *tx;
> >>> +
> >>> + if (!dsi->host->ops || !dsi->host->ops->transfer)
> >>> + return -ENOSYS;
> >>> +
> >>> + if (len > 0) {
> >>> + unsigned int offset = 0;
> >>> +
> >>> + /*
> >>> + * DCS long write packets contain the word count in the header
> >>> + * bytes 1 and 2 and have a payload containing the DCS command
> >>> + * byte folowed by word count minus one bytes.
> >>> + *
> >>> + * DCS short write packets encode the DCS command and up to
> >>> + * one parameter in header bytes 1 and 2.
> >>> + */
> >>> + if (len > 1)
> >>> + size = 3 + len;
> >>> + else
> >>> + size = 1 + len;
> >> I guess "size = 2" would be better here.
> > This is on purpose because it documents the format. If len > 1, then the
> > packet is a long write, so we need three bytes (command & word count) in
> > addition to the payload. For short writes, len <= 1 and we only need one
> > extra byte (command).
> >
> >>> +
> >>> + tx = kmalloc(size, GFP_KERNEL);
> >>> + if (!tx)
> >>> + return -ENOMEM;
> >>> +
> >>> + /* write word count to header for DCS long write packets */
> >>> + if (len > 1) {
> >>> + tx[offset++] = ((1 + len) >> 0) & 0xff;
> >>> + tx[offset++] = ((1 + len) >> 8) & 0xff;
> >>> + }
> >>> +
> >>> + /* write the DCS command byte followed by the payload */
> >>> + tx[offset++] = cmd;
> >>> + memcpy(tx + offset, data, len);
> >>> + } else {
> >>> + tx = &cmd;
> >>> + size = 1;
> >>> + }
> >> Contents of this conditional is incompatible with the current API.
> >> mipi_dsi_msg.tx_buf contains only data and mipi_dsi_msg.tx_len contains
> >> lenght of this data. Now you try to embed length of data into tx_buf and
> >> this breaks the API.
> > Huh? Of course the new API has different semantics, but that's the whole
> > point of it. The else branch here is to optimize for the case where a
> > command has no payload. In that case there is no need for allocating an
> > extra buffer, since the command byte is the only data transferred.
>
> If this is the whole point of it why patch description says nothing
> about it?
I thought the patch description said this. What do you think needs to be
added?
> It has nothing to do with helpers symmetry, this is serious API change.
No, it's not. It replaces an existing API, mipi_dsi_dcs_write() with a
different one, mipi_dsi_dcs_write_buffer() and converts the one call
site where the function is used.
Then it introduces a new function that behaves the same but uses a
different signature that takes the DCS command byte as an explicit
parameter instead of embedding the DCS command byte into the "payload"
buffer.
I don't understand why you think we're changing anything fundamental
here.
> >> And of course changing API would require also changing current users of
> >> the API.
> > There's a single user of this function and this patch switches it over
> > to the compatibility function mipi_dsi_dcs_write_buffer().
>
> Mostly panels are users of these functions and these functions uses
> transfer callback internally. If we allow different semantic for
> transfer msg
> we will end up with panels cooperating only with specific dsi hosts.
> I do not think it is good direction.
That's not at all the intention. Both functions are supposed to keep
working the same way. mipi_dsi_dcs_write_buffer() becomes what was
previously called mipi_dsi_dcs_write() and mipi_dsi_dcs_write() is a
new helper that concatenates a command byte with payload and passes
it into mipi_dsi_dcs_write_buffer().
So if you find that mipi_dsi_dcs_write_buffer() now does the wrong thing
and breaks the s6e8aa0 panel, please point out what exactly is going
wrong so that I can fix it.
> >> But in the first place it would be good to know why do you want to
> >> change the API? What are benefits of this solution?
> > I've already explained this elsewhere.
>
> Where? I remember only one discussion where you claimed this is better
> solution
> for you [1], but without explanation.
I explained this in an earlier reply to you in this thread.
> Do you have patches/repo for tegra with transfer callback implemented
> with this semantic?
> Maybe looking at the code will be helpful.
I posted these patches for review to dri-devel yesterday, so they should
be available there. If you prefer patchwork, see:
https://patchwork.kernel.org/patch/5075161/
And look for tegra_dsi_host_transfer().
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20141014/7b862ef3/attachment.sig>
More information about the dri-devel
mailing list