From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932154AbeAWVPP (ORCPT ); Tue, 23 Jan 2018 16:15:15 -0500 Received: from mail-pf0-f196.google.com ([209.85.192.196]:42709 "EHLO mail-pf0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752234AbeAWVPN (ORCPT ); Tue, 23 Jan 2018 16:15:13 -0500 X-Google-Smtp-Source: AH8x224ORT5ytx+44w/Cx2Yv9Odzpypz5yZ/EkmoGM/3ecnkPHfU19pi+XWet2e118VO08+T/xVfHQ== Date: Tue, 23 Jan 2018 13:15:10 -0800 From: Brian Norris To: Philippe CORNU Cc: Archit Taneja , Andrzej Hajda , Laurent Pinchart , David Airlie , Yannick FERTRE , Vincent ABRIOU , "dri-devel@lists.freedesktop.org" , "linux-kernel@vger.kernel.org" , Sean Paul , Nickey Yang , "hl@rock-chips.com" , "linux-rockchip@lists.infradead.org" , "mka@chromium.org" , "hoegsberg@gmail.com" , "zyw@rock-chips.com" , "xbl@rock-chips.com" Subject: Re: [PATCH] drm/bridge/synopsys: dsi: use common mipi_dsi_create_packet() Message-ID: <20180123211505.xvpu3putjjbqnl2b@ban.mtv.corp.google.com> References: <20180106003813.4816-1-briannorris@chromium.org> <715bb58c-efa6-7944-f186-c186d7fae569@st.com> <20180109185512.GA73309@google.com> <023c159e-0a7f-7d1b-a2d8-8ef19033a1b3@st.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <023c159e-0a7f-7d1b-a2d8-8ef19033a1b3@st.com> User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Philippe, On Thu, Jan 18, 2018 at 11:40:48AM +0000, Philippe CORNU wrote: > On 01/11/2018 12:16 PM, Philippe CORNU wrote: > > To be honest, I do not really like the memcpy here too and I agree with > > you regarding the BE issue. > > > > My first "stm" driver (ie. before using this "freescale/rockchip" > > dw-mipi-dsi driver with the memcpy) used the "exact" same code as the > > Tegra dsi tegra_dsi_writesl() function with the 2 loops. > > > > https://elixir.free-electrons.com/linux/v4.14/source/drivers/gpu/drm/tegra/dsi.c#L1248 > > > > > > IMHO, it is better than memcpy... > > I added these 3 "documentation" lines, maybe we may reuse them or > > something similar... > > > > /* > >  * Write 8-bit payload data into the 32-bit payload data register. > >  * ex: payload data "0x01, 0x02, 0x03, 0x04, 0x05, 0x06" will become > >  * "0x04030201 0x00000605" 32-bit writes > >  */ > > > > Not sure it helps to fix the BE issue but we may add a TODO stating that > > "this loop has not been tested on BE"... > > > > What is your opinion? I'm sorry, I don't think I noticed your reply here. I'm trying to unbury some email, but that's sometimes a losing battle... That code actually does look correct, and it's perhaps marginally better-looking in my opinion. It's up to you if you want to propose another patch :) At this point, it's only a matter of nice code, not correctness I believe. > As your patch has been merged, I have few short questions and for each > related new patch, I would like to know if you prefer that I implement > it or if you prefer to do it by yourself, it's really like you want, on > my side, no problem to make them all, some or none, I don't want us to > implement these in parallel :-) > > * Do you have any opinion regarding Tegra-like loops vs the memcpy? (see > my comment above) If you think the Tegra-like loops is a better approach > than memcpy, there is a small patch to write. My opinion is above. > * Returned value with number of bytes received/transferred: there is a > small patch to write I don't think I followed that one very well. I'm not sure my opinion really matters, as long as you get someone else to agree. I do not plan to write any such patch in the near term. > * Regarding read operations: I propose to add a TODO + DRM_WARN in case > someone want to use the API for read operations. Note that I plan to > implement the read feature but I do not know yet when and maybe Rockchip > people already have something ~ready? The warning would be nice to do now, regardless. Rockchip folks wrote up something for read support here [1], but it's based on a semi-forked version of the driver (we're trying to clean up the divergence, but it's not there yet). Perhaps it would provide useful fodder for your work. I don't think Rockchip is immediately working on upstreaming this particular patch, so it's totally fair to handle it yourself. It's got the GPL sign-off ;) Brian [1] https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/863347