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

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

On 10/21/2014 01:10 PM, Andrzej Hajda wrote:
> 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.

To be more precise, I am fine with introduction of 'symmetrical' API,
there other issues regarding this patch which were discussed elsewhere.

Regards
Andrzej