On Mon, Oct 13, 2014 at 02:29:15PM +0200, Andrzej Hajda wrote:
> Hi Thierry,
>
> On 10/13/2014 12:16 PM, Thierry Reding wrote:
> > From: Thierry Reding <treding@xxxxxxxxxx>
> >
> > 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().
>
> As we discussed before I do not see much symmetry here - dcs read is
> a kind of write command with non empty response.
The symmetry is in the API. DCS is a command specification, therefore
the command is the primary means of identification. Hiding the command
inside the payload buffer obfuscates in my opinion. Using an explicit
command parameter makes it immediately obvious from the function call
what command is being sent without having to look up the static buffer
where the command byte is embedded.
This is really the same as for I2C for example. You can use the very
low-level i2c_transfer() and it will allow you to perform any type of
transfer. That's the equivalent of the DSI host's .transfer() function.
On top of that you get helpers like i2c_smbus_write_byte_data() and
i2c_smbus_read_byte_data(), which both take a command byte. For writes
that command byte will likely be sent in the same transfer as the data,
but the important aspect here is that you get a high-level API because
it makes it easier or more intuitive to perform the task at hand.
> The new function simplifies calls in case write has no arguments,
> and it complicates it in other cases.
I don't see what is complicated here. The code itself may be slightly
more complicated, but I think the advantage of having a consistent API
outweighs that.
> But the same simplification could be done by just providing inline function,
> for example:
> static inline ... mipi_dsi_dcs_write_cmd0(dsi, cmd)
> {
> return mipi_dsi_dcs_write(dsi, &cmd, 1);
> }
You get exactly the same using mipi_dsi_dcs_write(dsi, cmd, NULL, 0)
with the implementation proposed here. Again I think that's a pretty
natural representation of what's happening: "send command <cmd> with
no payload".
The above simplification works only for no parameters, whereas what I
proposed works with parameters, too:
mipi_dsi_dcs_write(dsi, cmd, value, sizeof(value));
I essentially allows you to construct DCS commands straight from the
specification by filling an array with the parameters as given and
passing it as payload for the command.
> This way we would have simplified calls for commands without arguments
> but without complication in other cases.
Really, I don't see what's complicated about it. Yes, the implementation
is somewhat more complex and requires an allocation. But there are fast
paths in the kernel to deal with those, so I wouldn't expect any of the
calls to take significantly longer than if this was written into a
statically allocated buffer.
> > 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@xxxxxxxxxx>
> > ---
> > 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);
>
> It is just nitpick, but why did you replaced ops variable
> by multiple dsi->host->ops expressions? I though it is a good
> practice to avoid evaluating the same expression multiple times.
I thought I was making this consistent with other functions, but
checking again I did exactly the opposite =). As for the evaluation I
don't see the point. Any compiler worth its salt will notice that we're
using the same object and optimize this, whether we store it in a local
variable or not.
Also I find it slightly confusing that we have a local variable for the
ops, but then need to do dsi->host to get a reference to the host.
Anyway, I'll remove that change to restore consistency with other
functions.
> > +}
> > +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;
> > +
> > + 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;
> > + }
> > +
> > + memset(&msg, 0, sizeof(msg));
> > + msg.channel = dsi->channel;
> > + msg.tx_len = size;
> > + msg.tx_buf = tx;
> > +
> > + switch (len) {
> > + case 0:
> > + msg.type = MIPI_DSI_DCS_SHORT_WRITE;
> > + break;
> > + case 1:
> > + msg.type = MIPI_DSI_DCS_SHORT_WRITE_PARAM;
> > + break;
> > default:
> > msg.type = MIPI_DSI_DCS_LONG_WRITE;
> > break;
> > @@ -234,23 +321,27 @@ ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, const void *data,
> > if (dsi->mode_flags & MIPI_DSI_MODE_LPM)
> > msg.flags = MIPI_DSI_MSG_USE_LPM;
> >
> > - return ops->transfer(dsi->host, &msg);
> > + err = dsi->host->ops->transfer(dsi->host, &msg);
>
> I think it would be better to replace the code above (starting from
> memset) by:
> err = mipi_dsi_dcs_write_buffer(dsi, tx, size)
Good point, will do.
>
> > +
> > + 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
> > - * @data: pointer to read buffer
> > - * @len: length of @data
> > + * mipi_dsi_dcs_read() - send DCS read request command
> > + * @dsi: DSI peripheral device
> > + * @cmd: DCS command
> > + * @data: buffer in which to receive data
> > + * @len: size of receive buffer
> > *
> > - * Function returns number of read bytes or error code.
> > + * Return: The number of bytes read or a negative error code on failure.
> > */
> > ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, u8 cmd, void *data,
> > size_t len)
> > {
> > - const struct mipi_dsi_host_ops *ops = dsi->host->ops;
> > struct mipi_dsi_msg msg = {
> > .channel = dsi->channel,
> > .type = MIPI_DSI_DCS_READ,
> > @@ -260,13 +351,13 @@ ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, u8 cmd, void *data,
> > .rx_len = len
> > };
> >
> > - if (!ops || !ops->transfer)
> > + if (!dsi->host->ops || !dsi->host->ops->transfer)
> > return -ENOSYS;
> >
> > if (dsi->mode_flags & MIPI_DSI_MODE_LPM)
> > msg.flags = MIPI_DSI_MSG_USE_LPM;
> >
> > - return ops->transfer(dsi->host, &msg);
> > + return dsi->host->ops->transfer(dsi->host, &msg);
>
> Again, why multiple evaluations of dsi->host->ops vs single one?
I've undone that change for consistency.
Thanks,
Thierry
Attachment:
pgp6J42t3PuwW.pgp
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
