Re: [PATCH 05/15] drm/dsi: Implement generic read and write commands

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

On 10/14/2014 12:03 PM, Thierry Reding wrote:
> On Tue, Oct 14, 2014 at 10:59:26AM +0200, Andrzej Hajda wrote:
>> On 10/13/2014 12:16 PM, Thierry Reding wrote:
>>> From: Thierry Reding <treding@nvidia.com>
>>>
>>> Implement generic read and write commands. Selection of the proper data
>>> type for packets is done automatically based on the number of parameters
>>> or payload length.
>>>
>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>> ---
>>>  drivers/gpu/drm/drm_mipi_dsi.c | 115 +++++++++++++++++++++++++++++++++++++++++
>>>  include/drm/drm_mipi_dsi.h     |   6 +++
>>>  2 files changed, 121 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
>>> index 27fc6dac5e4a..7cd69273dbad 100644
>>> --- a/drivers/gpu/drm/drm_mipi_dsi.c
>>> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
>>> @@ -229,6 +229,121 @@ int mipi_dsi_set_maximum_return_packet_size(struct mipi_dsi_device *dsi,
>>>  EXPORT_SYMBOL(mipi_dsi_set_maximum_return_packet_size);
>>>  
>>>  /**
>>> + * mipi_dsi_generic_write() - transmit data using a generic write packet
>>> + * @dsi: DSI peripheral device
>>> + * @payload: buffer containing the payload
>>> + * @size: size of payload buffer
>>> + *
>>> + * This function will automatically choose the right data type depending on
>>> + * the payload length.
>>> + *
>>> + * Return: The number of bytes transmitted on success or a negative error code
>>> + * on failure.
>>> + */
>>> +ssize_t mipi_dsi_generic_write(struct mipi_dsi_device *dsi, const void *payload,
>>> +			       size_t size)
>>> +{
>>> +	struct mipi_dsi_msg msg;
>>> +	ssize_t err;
>>> +	u8 *tx;
>>> +
>>> +	memset(&msg, 0, sizeof(msg));
>>> +	msg.channel = dsi->channel;
>> Again, maybe initializer would be better.
> Why?
>
>>> +	msg.flags = MIPI_DSI_MSG_USE_LPM | MIPI_DSI_MSG_REQ_ACK;
>>
>> You should not hardcode flags here, these flags have nothing to do with
>> dsi generic write.
> Agreed, these seem to be left-over from debugging that I overlooked when
> squashing together various patches.
>
>> But there is general problem of encoding flags in
>> helpers. Possible solutions I see:
>> 1. Translate DSI flags to msg flags as in case of MIPI_DSI_MODE_LPM ->
>> MIPI_DSI_MSG_USE_LPM.
> That's how it was originally intended. The DSI device's flags would be
> used to derive message flags, yet wouldn't be an exact copy because not
> all flags are relevant to message transfers.

OK

>
> There's a bit of an inconsistency here, though. MIPI_DSI_MODE_LPM isn't
> very clearly defined. It says:
>
> 	/* transmit data in low power */
> 	#define MIPI_DSI_MODE_LPM	BIT(11)
>
> Whereas:
>
> 	/* use Low Power Mode to transmit message */
> 	#define MIPI_DSI_MSG_USE_LPM	BIT(1)
>
> Currently we assume that data == message in the above. However
> MIPI_DSI_MODE_LPM could also mean that video data is supposed to be
> transmitted in low-power mode.
>
> I vaguely remember discussing this with you and Inki (?) before in the
> context of continuous vs. non-continuous clocks, but there didn't seem
> to be any final outcome on that discussion.

According to specs "Video
information should only be transmitted using High Speed Mode"[1],
but there is still command mode. Anyway it would be good to clarify
that it is only for messages.

[1]: DSI spec 1.2a chapter 4.2.2 Video Mode Operation

>
> According to the specification, version 1.02.00, section 5.2, "The
> peripheral shall be capable of receiving any transmission in Low Power
> or High Speed Mode." That would indicate that MIPI_DSI_MSG_USE_LPM is
> completely unnecessary, unless a device isn't compliant with the spec.

Toshiba bridge TC358764 can be controlled only using LPM commands.
IIRC the panel Inki was working on also requires initialization using
only LPM.
Also your patch "Always use LPM" suggests that you also have
non-compliant devices.

Regards
Andrzej