From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751531AbaLQKlH (ORCPT ); Wed, 17 Dec 2014 05:41:07 -0500 Received: from gw-1.arm.linux.org.uk ([78.32.30.217]:35663 "EHLO pandora.arm.linux.org.uk" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750894AbaLQKlF (ORCPT ); Wed, 17 Dec 2014 05:41:05 -0500 Date: Wed, 17 Dec 2014 10:40:49 +0000 From: Russell King - ARM Linux To: Liu Ying Cc: Thierry Reding , dri-devel@lists.freedesktop.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, p.zabel@pengutronix.de, shawn.guo@linaro.org, kernel@pengutronix.de, mturquette@linaro.org, airlied@linux.ie Subject: Re: [PATCH RFC 09/15] drm: imx: Add MIPI DSI host controller driver Message-ID: <20141217104049.GU11285@n2100.arm.linux.org.uk> References: <1418200648-32656-1-git-send-email-Ying.Liu@freescale.com> <1418200648-32656-10-git-send-email-Ying.Liu@freescale.com> <20141210131640.GD23558@ulmo.nvidia.com> <54915081.6020508@freescale.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <54915081.6020508@freescale.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Dec 17, 2014 at 05:44:33PM +0800, Liu Ying wrote: > Hi Thierry, > > Sorry for the late response. > I tried to address almost all your comments locally first. > More feedback below. > > On 12/10/2014 09:16 PM, Thierry Reding wrote: > >On Wed, Dec 10, 2014 at 04:37:22PM +0800, Liu Ying wrote: > >>+static int check_status(struct imx_mipi_dsi *dsi, u32 reg, u32 status, > >>+ int timeout, bool to_set) > >>+{ > >>+ u32 val; > >>+ bool out = false; > >>+ > >>+ val = dsi_read(dsi, reg); > >>+ for (;;) { > >>+ out = to_set ? (val & status) : !(val & status); > >>+ if (out) > >>+ break; > >>+ > >>+ if (!timeout--) > >>+ return -EFAULT; > >>+ > >>+ msleep(1); > >>+ val = dsi_read(dsi, reg); > >>+ } > >>+ return 0; > >>+} > > > >You should probably use a properly timed loop here. msleep() isn't > >guaranteed to return after exactly one millisecond, so your timeout is > >never going to be accurate. Something like the following would be better > >in my opinion: > > > > timeout = jiffies + msecs_to_jiffies(timeout); > > > > while (time_before(jiffies, timeout)) { > > ... > > } > > > >Also timeout should be unsigned long in that case. > > Accepted. Actually, that's a bad example: what we want to do is to assess success after we wait, before we decide that something has failed. In other words, we don't want to wait, and decide that we failed without first checking for success. In any case, returning -EFAULT is not sane: EFAULT doesn't mean "fault" it means "Bad address", and it is returned to userspace to mean that userspace passed the kernel a bad address. That definition does /not/ fit what's going on here. timeout = jiffies + msecs_to_jiffies(timeout); do { val = dsi_read(dsi, reg); out = to_set ? (val & status) : !(val & status); if (out) break; if (time_is_after_jiffies(timeout)) return -ETIMEDOUT; msleep(1); } while (1); return 0; would be better: we only fail immediately after we have checked whether we succeeded, and we also do the first check immediately. -- FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up according to speedtest.net. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Russell King - ARM Linux Subject: Re: [PATCH RFC 09/15] drm: imx: Add MIPI DSI host controller driver Date: Wed, 17 Dec 2014 10:40:49 +0000 Message-ID: <20141217104049.GU11285@n2100.arm.linux.org.uk> References: <1418200648-32656-1-git-send-email-Ying.Liu@freescale.com> <1418200648-32656-10-git-send-email-Ying.Liu@freescale.com> <20141210131640.GD23558@ulmo.nvidia.com> <54915081.6020508@freescale.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Content-Disposition: inline In-Reply-To: <54915081.6020508@freescale.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Liu Ying Cc: devicetree@vger.kernel.org, mturquette@linaro.org, kernel@pengutronix.de, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-arm-kernel@lists.infradead.org List-Id: devicetree@vger.kernel.org T24gV2VkLCBEZWMgMTcsIDIwMTQgYXQgMDU6NDQ6MzNQTSArMDgwMCwgTGl1IFlpbmcgd3JvdGU6 Cj4gSGkgVGhpZXJyeSwKPiAKPiBTb3JyeSBmb3IgdGhlIGxhdGUgcmVzcG9uc2UuCj4gSSB0cmll ZCB0byBhZGRyZXNzIGFsbW9zdCBhbGwgeW91ciBjb21tZW50cyBsb2NhbGx5IGZpcnN0Lgo+IE1v cmUgZmVlZGJhY2sgYmVsb3cuCj4gCj4gT24gMTIvMTAvMjAxNCAwOToxNiBQTSwgVGhpZXJyeSBS ZWRpbmcgd3JvdGU6Cj4gPk9uIFdlZCwgRGVjIDEwLCAyMDE0IGF0IDA0OjM3OjIyUE0gKzA4MDAs IExpdSBZaW5nIHdyb3RlOgo+ID4+K3N0YXRpYyBpbnQgY2hlY2tfc3RhdHVzKHN0cnVjdCBpbXhf bWlwaV9kc2kgKmRzaSwgdTMyIHJlZywgdTMyIHN0YXR1cywKPiA+PisJCQlpbnQgdGltZW91dCwg Ym9vbCB0b19zZXQpCj4gPj4rewo+ID4+Kwl1MzIgdmFsOwo+ID4+Kwlib29sIG91dCA9IGZhbHNl Owo+ID4+Kwo+ID4+Kwl2YWwgPSBkc2lfcmVhZChkc2ksIHJlZyk7Cj4gPj4rCWZvciAoOzspIHsK PiA+PisJCW91dCA9IHRvX3NldCA/ICh2YWwgJiBzdGF0dXMpIDogISh2YWwgJiBzdGF0dXMpOwo+ ID4+KwkJaWYgKG91dCkKPiA+PisJCQlicmVhazsKPiA+PisKPiA+PisJCWlmICghdGltZW91dC0t KQo+ID4+KwkJCXJldHVybiAtRUZBVUxUOwo+ID4+Kwo+ID4+KwkJbXNsZWVwKDEpOwo+ID4+KwkJ dmFsID0gZHNpX3JlYWQoZHNpLCByZWcpOwo+ID4+Kwl9Cj4gPj4rCXJldHVybiAwOwo+ID4+K30K PiA+Cj4gPllvdSBzaG91bGQgcHJvYmFibHkgdXNlIGEgcHJvcGVybHkgdGltZWQgbG9vcCBoZXJl LiBtc2xlZXAoKSBpc24ndAo+ID5ndWFyYW50ZWVkIHRvIHJldHVybiBhZnRlciBleGFjdGx5IG9u ZSBtaWxsaXNlY29uZCwgc28geW91ciB0aW1lb3V0IGlzCj4gPm5ldmVyIGdvaW5nIHRvIGJlIGFj Y3VyYXRlLiBTb21ldGhpbmcgbGlrZSB0aGUgZm9sbG93aW5nIHdvdWxkIGJlIGJldHRlcgo+ID5p biBteSBvcGluaW9uOgo+ID4KPiA+CXRpbWVvdXQgPSBqaWZmaWVzICsgbXNlY3NfdG9famlmZmll cyh0aW1lb3V0KTsKPiA+Cj4gPgl3aGlsZSAodGltZV9iZWZvcmUoamlmZmllcywgdGltZW91dCkp IHsKPiA+CQkuLi4KPiA+CX0KPiA+Cj4gPkFsc28gdGltZW91dCBzaG91bGQgYmUgdW5zaWduZWQg bG9uZyBpbiB0aGF0IGNhc2UuCj4gCj4gQWNjZXB0ZWQuCgpBY3R1YWxseSwgdGhhdCdzIGEgYmFk IGV4YW1wbGU6IHdoYXQgd2Ugd2FudCB0byBkbyBpcyB0byBhc3Nlc3Mgc3VjY2VzcwphZnRlciB3 ZSB3YWl0LCBiZWZvcmUgd2UgZGVjaWRlIHRoYXQgc29tZXRoaW5nIGhhcyBmYWlsZWQuICBJbiBv dGhlcgp3b3Jkcywgd2UgZG9uJ3Qgd2FudCB0byB3YWl0LCBhbmQgZGVjaWRlIHRoYXQgd2UgZmFp bGVkIHdpdGhvdXQgZmlyc3QKY2hlY2tpbmcgZm9yIHN1Y2Nlc3MuCgpJbiBhbnkgY2FzZSwgcmV0 dXJuaW5nIC1FRkFVTFQgaXMgbm90IHNhbmU6IEVGQVVMVCBkb2Vzbid0IG1lYW4gImZhdWx0Igpp dCBtZWFucyAiQmFkIGFkZHJlc3MiLCBhbmQgaXQgaXMgcmV0dXJuZWQgdG8gdXNlcnNwYWNlIHRv IG1lYW4gdGhhdAp1c2Vyc3BhY2UgcGFzc2VkIHRoZSBrZXJuZWwgYSBiYWQgYWRkcmVzcy4gIFRo YXQgZGVmaW5pdGlvbiBkb2VzIC9ub3QvCmZpdCB3aGF0J3MgZ29pbmcgb24gaGVyZS4KCgl0aW1l b3V0ID0gamlmZmllcyArIG1zZWNzX3RvX2ppZmZpZXModGltZW91dCk7CgoJZG8gewoJCXZhbCA9 IGRzaV9yZWFkKGRzaSwgcmVnKTsKCQlvdXQgPSB0b19zZXQgPyAodmFsICYgc3RhdHVzKSA6ICEo dmFsICYgc3RhdHVzKTsKCQlpZiAob3V0KQoJCQlicmVhazsKCgkJaWYgKHRpbWVfaXNfYWZ0ZXJf amlmZmllcyh0aW1lb3V0KSkKCQkJcmV0dXJuIC1FVElNRURPVVQ7CgoJCW1zbGVlcCgxKTsKCX0g d2hpbGUgKDEpOwoKCXJldHVybiAwOwoKd291bGQgYmUgYmV0dGVyOiB3ZSBvbmx5IGZhaWwgaW1t ZWRpYXRlbHkgYWZ0ZXIgd2UgaGF2ZSBjaGVja2VkIHdoZXRoZXIKd2Ugc3VjY2VlZGVkLCBhbmQg d2UgYWxzbyBkbyB0aGUgZmlyc3QgY2hlY2sgaW1tZWRpYXRlbHkuCgotLSAKRlRUQyBicm9hZGJh bmQgZm9yIDAuOG1pbGUgbGluZTogY3VycmVudGx5IGF0IDkuNU1icHMgZG93biA0MDBrYnBzIHVw CmFjY29yZGluZyB0byBzcGVlZHRlc3QubmV0LgpfX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fXwpkcmktZGV2ZWwgbWFpbGluZyBsaXN0CmRyaS1kZXZlbEBsaXN0 cy5mcmVlZGVza3RvcC5vcmcKaHR0cDovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xp c3RpbmZvL2RyaS1kZXZlbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 From: linux@arm.linux.org.uk (Russell King - ARM Linux) Date: Wed, 17 Dec 2014 10:40:49 +0000 Subject: [PATCH RFC 09/15] drm: imx: Add MIPI DSI host controller driver In-Reply-To: <54915081.6020508@freescale.com> References: <1418200648-32656-1-git-send-email-Ying.Liu@freescale.com> <1418200648-32656-10-git-send-email-Ying.Liu@freescale.com> <20141210131640.GD23558@ulmo.nvidia.com> <54915081.6020508@freescale.com> Message-ID: <20141217104049.GU11285@n2100.arm.linux.org.uk> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Wed, Dec 17, 2014 at 05:44:33PM +0800, Liu Ying wrote: > Hi Thierry, > > Sorry for the late response. > I tried to address almost all your comments locally first. > More feedback below. > > On 12/10/2014 09:16 PM, Thierry Reding wrote: > >On Wed, Dec 10, 2014 at 04:37:22PM +0800, Liu Ying wrote: > >>+static int check_status(struct imx_mipi_dsi *dsi, u32 reg, u32 status, > >>+ int timeout, bool to_set) > >>+{ > >>+ u32 val; > >>+ bool out = false; > >>+ > >>+ val = dsi_read(dsi, reg); > >>+ for (;;) { > >>+ out = to_set ? (val & status) : !(val & status); > >>+ if (out) > >>+ break; > >>+ > >>+ if (!timeout--) > >>+ return -EFAULT; > >>+ > >>+ msleep(1); > >>+ val = dsi_read(dsi, reg); > >>+ } > >>+ return 0; > >>+} > > > >You should probably use a properly timed loop here. msleep() isn't > >guaranteed to return after exactly one millisecond, so your timeout is > >never going to be accurate. Something like the following would be better > >in my opinion: > > > > timeout = jiffies + msecs_to_jiffies(timeout); > > > > while (time_before(jiffies, timeout)) { > > ... > > } > > > >Also timeout should be unsigned long in that case. > > Accepted. Actually, that's a bad example: what we want to do is to assess success after we wait, before we decide that something has failed. In other words, we don't want to wait, and decide that we failed without first checking for success. In any case, returning -EFAULT is not sane: EFAULT doesn't mean "fault" it means "Bad address", and it is returned to userspace to mean that userspace passed the kernel a bad address. That definition does /not/ fit what's going on here. timeout = jiffies + msecs_to_jiffies(timeout); do { val = dsi_read(dsi, reg); out = to_set ? (val & status) : !(val & status); if (out) break; if (time_is_after_jiffies(timeout)) return -ETIMEDOUT; msleep(1); } while (1); return 0; would be better: we only fail immediately after we have checked whether we succeeded, and we also do the first check immediately. -- FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up according to speedtest.net.