From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrzej Hajda Subject: Re: [PATCH v2 01/15] drm/dsi: Make mipi_dsi_dcs_{read,write}() symmetrical Date: Tue, 21 Oct 2014 13:19:26 +0200 Message-ID: <5446413E.1000908@samsung.com> References: <1413195395-3355-1-git-send-email-thierry.reding@gmail.com> <543BC59B.3090408@samsung.com> <20141014090009.GA18993@ulmo> <54463F42.60502@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mailout3.w1.samsung.com (mailout3.w1.samsung.com [210.118.77.13]) by gabe.freedesktop.org (Postfix) with ESMTP id 5DCC06E1D7 for ; Tue, 21 Oct 2014 04:19:29 -0700 (PDT) Received: from eucpsbgm2.samsung.com (unknown [203.254.199.245]) by mailout3.w1.samsung.com (Oracle Communications Messaging Server 7u4-24.01(7.0.4.24.0) 64bit (built Nov 17 2011)) with ESMTP id <0NDS003PVKX7ND00@mailout3.w1.samsung.com> for dri-devel@lists.freedesktop.org; Tue, 21 Oct 2014 12:22:19 +0100 (BST) In-reply-to: <54463F42.60502@samsung.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Thierry Reding Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org 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 >>>> >>>> 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