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