Re: [PATCH 3/4] drm/dsi: Make mipi_dsi_dcs_{read, write}() symmetrical

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

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

On Tue, Jul 22, 2014 at 09:32:58AM +0200, Andrzej Hajda wrote:
> On 07/22/2014 09:12 AM, 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().
> >
> > Also update the S6E8AA0 panel driver (the only user of this API) to
> > adapt to this new API.
> 
> I do not agree with that. DCS read and write are not symmetrical by design:
> - write just sends data,
> - read sends data then reads response.
> 
> Forcing API to be symmetrical complicates the implementation without real gain.

Why does it complicate anything? Granted we need to dynamically allocate
a buffer for each transfer, but it's all hidden away in a common helper
function. The big advantage in using a separate parameter for the
command is that it makes it trivial to implement the majority of DCS
commands, like this:

	const u8 format = 0x77;
	const u8 mode = 0x00;

	mipi_dsi_dcs_write(dsi, MIPI_DCS_SOFT_RESET, NULL, 0);
	mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_PIXEL_FORMAT, &format, 1);
	mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_TEAR_ON, &mode, 1);
	mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_DISPLAY_ON, NULL, 0);

Without the extra parameter, the above becomes:

	const u8 data[1] = { MIPI_DCS_SOFT_RESET };
	mipi_dsi_dcs_write(dsi, data, sizeof(data));

	const u8 data[2] = { MIPI_DCS_SET_PIXEL_FORMAT, 0x77 };
	mipi_dsi_dcs_write(dsi, data, sizeof(data));

	const u8 data[2] = { MIPI_DCS_SET_TEAR_ON, 0x00 };
	mipi_dsi_dcs_write(dsi, data, sizeof(data));

	const u8 data[1] = { MIPI_DCS_SET_DISPLAY_ON };
	mipi_dsi_dcs_write(dsi, data, sizeof(data));

In this form it looks still rather readable, but if you need to do this
within an initialization function, it become fairly cluttered:

	const u8 soft_reset[1] = { MIPI_DCS_SOFT_RESET };
	const u8 set_pixel_format[2] = { MIPI_DCS_SET_PIXEL_FORMAT, 0x77 };
	const u8 set_tear_on[2] = { MIPI_DCS_SET_TEAR_ON, 0x00 };
	const u8 set_display_on[1] = { MIPI_DCS_SET_DISPLAY_ON };

	mipi_dsi_dcs_write(dsi, soft_reset, sizeof(soft_reset));

	mipi_dsi_dcs_write(dsi, set_pixel_format, sizeof(set_pixel_format));

	mipi_dsi_dcs_write(dsi, set_tear_on, sizeof(set_tear_on));

	mipi_dsi_dcs_write(dsi, set_display_on, sizeof(set_display_on));

I find that really hard to read because it has a potentially large gap
between the place where the commands are defined and where they're used.

Some of this could possibly be alleviated by adding separate functions
for standard commands. But either way, I think having this kind of
symmetry within an API is always good (it's consistent). By the law of
least surprise people will actually expect mipi_dsi_dcs_write() to take
a command as a second parameter.

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