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

[Thierry Reding]

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 at 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 at 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.

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

> 2. Copy dsi.mode_flags to msg.flags, or to dedicated field of msg.
> 3. Call transfer callback with dsi pointer instead of host, this
> way it will have access to mode_flags.

The intention was to keep .transfer() agnostic of the device. The idea
was that the DSI core would take care of these flags as necessary and
host implementations wouldn't have to worry about them.

> > +
> > +	if (size > 2) {
> > +		tx = kmalloc(2 + size, GFP_KERNEL);
> > +		if (!tx)
> > +			return -ENOMEM;
> > +
> > +		tx[0] = (size >> 0) & 0xff;
> > +		tx[1] = (size >> 8) & 0xff;
> > +
> > +		memcpy(tx + 2, payload, size);
> > +
> > +		msg.tx_len = 2 + size;
> > +		msg.tx_buf = tx;
> > +	} else {
> > +		msg.tx_buf = payload;
> > +		msg.tx_len = size;
> > +	}
> 
> Another API breakage, commented already in response to 1st patch.

Can you elaborate? How can this be API breakage if it's a new API being
introduced?

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/716151f5/attachment.sig>