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:00 AM, Thierry Reding wrote:
> On Mon, Oct 13, 2014 at 02:29:15PM +0200, Andrzej Hajda wrote:
>> Hi Thierry,
>>
>> 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().
>> As we discussed before I do not see much symmetry here - dcs read is
>> a kind of write command with non empty response.
> The symmetry is in the API. DCS is a command specification, therefore
> the command is the primary means of identification. Hiding the command
> inside the payload buffer obfuscates in my opinion. Using an explicit
> command parameter makes it immediately obvious from the function call
> what command is being sent without having to look up the static buffer
> where the command byte is embedded.

1. According to specs command is an integral part of the payload,
separating it from the payload is unnatural.
2. It is rather difficult to obfuscate command using static buffers, you can
  use variadic macros as in panel_s6e8aa0.c, for example:
    mipi_dsi_dcs(dsi, MIPI_DCS_SET_PIXEL_FORMAT, 13);
  it is the nicest way IMHO - you do not need to pass to the call extra
arguments,
  neither number of parameters, neither pointers, you just pass command
  and its parameters.

  But if you do not like variadic macros you can still use static buffers:
    u8 dcs_set_pixel_format[] = {MIPI_DCS_SET_PIXEL_FORMAT, 13};
    ...
    mipi_dsi_dcs_write(dsi, dcs_set_pixel_format,
ARRAY_SIZE(dcs_set_pixel_format));

  As you see you should pass buffer name, which should clearly indicate
  what is its content, again no obfuscation.

Again, if it does not convince you I am fine with this patch. It leaves
the possibility
to use 'old' API, so it is OK to me.

Regards
Andrzej