From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933727AbeAXNWp (ORCPT ); Wed, 24 Jan 2018 08:22:45 -0500 Received: from mx08-00178001.pphosted.com ([91.207.212.93]:13339 "EHLO mx07-00178001.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S933589AbeAXNWo (ORCPT ); Wed, 24 Jan 2018 08:22:44 -0500 From: Philippe CORNU To: Brian Norris CC: Archit Taneja , Andrzej Hajda , Laurent Pinchart , David Airlie , Benjamin Gaignard , Bhumika Goyal , "dri-devel@lists.freedesktop.org" , "Linux Kernel" , Sandy Huang , Heiko Stubner , "linux-arm-kernel@lists.infradead.org" , "open list:ARM/Rockchip SoC..." , Yannick FERTRE , Vincent ABRIOU , Alexandre TORGUE , Maxime Coquelin , Ludovic BARRE , Mickael REULIER Subject: Re: [PATCH v1 2/2] drm/bridge/synopsys: dsi: Add a warning msg on dsi read operations Thread-Topic: [PATCH v1 2/2] drm/bridge/synopsys: dsi: Add a warning msg on dsi read operations Thread-Index: AQHTlFYmQea5Rlv+PEydWrJfc8kES6OB6L4AgAEKdQA= Date: Wed, 24 Jan 2018 13:22:04 +0000 Message-ID: <369181f6-1bb4-0559-86c9-5528be7fc459@st.com> References: <20180123142618.28384-1-philippe.cornu@st.com> <20180123142618.28384-3-philippe.cornu@st.com> In-Reply-To: Accept-Language: fr-FR, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: user-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 x-ms-exchange-messagesentrepresentingtype: 1 x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [10.75.127.45] Content-Type: text/plain; charset="utf-8" Content-ID: <00BEAF5332774E43805BEA12DB48CD41@st.com> MIME-Version: 1.0 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2018-01-24_06:,, signatures=0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id w0ODMojZ006442 Hi Brian, On 01/23/2018 10:28 PM, Brian Norris wrote: > Hi Philippe, > > I see you sent this out already today, while I only just responded > (late) to your questions about it... oh well :) > I got a short period to clean-up and adds features to this driver (1.31 ip version + maybe the read feature), sorry to have not wait a single day more. > On Tue, Jan 23, 2018 at 6:26 AM, Philippe Cornu wrote: >> The DCS/GENERIC DSI read feature is not yet implemented so it >> is important to warn the host_transfer() caller in case of >> read operation requests. >> >> Signed-off-by: Philippe Cornu >> --- >> drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c >> index 096cf5e5bb30..e46ddff8601c 100644 >> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c >> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c >> @@ -417,7 +417,14 @@ static ssize_t dw_mipi_dsi_host_transfer(struct mipi_dsi_host *host, >> if (ret) >> return ret; >> >> - nb_bytes = packet.size; >> + if (msg->rx_buf && msg->rx_len > 0) { > > It feels like you should do this check *before* you start writing > anything. It's possible to have a combination TX/RX command, and it > would be counterintuitive to only do half the operation then return > with an argument error. > Many thanks for your review. I agree with your comments. Well, my patch is not good at all because it contains a small part of the read feature I am writing... but it is not the purpose of this patch. No excuse, sorry guys for making you waste time. I will re-write a new patch 100% decorrelated from a possible future read feature. I could also wait until I have a working read feature but as it could take some times, I prefer warning users asap. >> + /* TODO dw drv improvements: implement read feature */ >> + dev_warn(dsi->dev, "read operations not yet implemented\n"); >> + return -EPERM; > > I'm not sure -EPERM is right. Feels like -EINVAL, -ENOSYS, or > -EOPNOTSUPP. I think -ENOSYS actually has been abused somewhat, so > maybe one of the other two. > not easy to pick the right one. I will use -EINVAL. >> + > > Spurious blank line? > thanks >> + } else { >> + nb_bytes = packet.size; >> + } > > You don't actually need to put this sort of thing in the 'else' case. > The other branch is an error-handling case, which definitely 'return's > early, and it's pretty standard coding style to avoid indenting the > "good" path like this. > > Brian > The else part is linked to my "read feature" too, sorry for that. I will do it simpler in next version. Thank you, Philippe :-) >> >> return nb_bytes; >> } >> -- >> 2.15.1 >>