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@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().
>>>
>>> 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);
>>> +}
>>> +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).
In such case you can put size calculation outside of the main if. Maybe
even you can
get rid of size variable and calculate msg.tx_len directly.
Regards
Andrzej
>
>>> +
>>> + 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.
>
>> 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().
>
>> 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.
>
> Thierry
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
