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

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2391 bytes --]

On Tue, Oct 14, 2014 at 01:13:38PM +0200, Thierry Reding wrote:
> On Tue, Oct 14, 2014 at 12:57:31PM +0200, Thierry Reding wrote:
> > On Tue, Oct 14, 2014 at 12:38:15PM +0200, Andrzej Hajda wrote:
> [...]
> > > It has nothing to do with helpers symmetry, this is serious API change.
> > 
> > No, it's not. It replaces an existing API, mipi_dsi_dcs_write() with a
> > different one, mipi_dsi_dcs_write_buffer() and converts the one call
> > site where the function is used.
> > 
> > Then it introduces a new function that behaves the same but uses a
> > different signature that takes the DCS command byte as an explicit
> > parameter instead of embedding the DCS command byte into the "payload"
> > buffer.
> > 
> > I don't understand why you think we're changing anything fundamental
> > here.
> 
> I think I understand now why you think this is a problem. It seems like
> the Exynos driver was implemented in a way that it injects the length of
> the payload manually, whereas mipi_dsi_dcs_write() is written in a way
> to insert the length into the payload already.
> 
> Letting host implementations take care of this is just going to lead to
> duplicating the same code everywhere. I think we should let the DSI core
> figure out what type of packet it is supposed to generate and then build
> the complete message using that data. That way what the DSI host gets is
> a raw buffer that it just needs to write into the right registers.
> 
> Specifically, if we don't do this in the DSI core, then *every* DSI host
> driver needs to do something similar to the
> 
> 	if (exynos_dsi_is_short_dsi_type(msg->type)) {
> 		...
> 	}
> 
> block in their implementation of the .transfer() function. And none of
> that is really driver specific. It's defined by the DSI specification
> and therefore has no place in the individual drivers.

It should be pretty trivial to adapt the Exynos implementation to deal
with the DSI core providing the payload this way. I'm thinking something
like:

	xfer.data[0] = msg->tx_buf[0];

	if (msg->tx_len > 1)
		xfer.data[1] = msg->tx_buf[1];
	else
		xfer.data[1] = 0;

	xfer.tx_len = 0;

	if (msg->tx_len > 2) {
		xfer.tx_payload = &msg->tx_buf[2];
		xfer.tx_len = msg->tx_len - 2;
	}

And it can probably be even more reduced if the now unneeded .data or
.tx_payload fields are removed.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel