Re: [PATCH v2 01/15] drm/dsi: Make mipi_dsi_dcs_{read, write}() symmetrical

[Andrzej Hajda <a.hajda@samsung.com>]

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@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@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).

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