From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751032AbdAaOrt (ORCPT ); Tue, 31 Jan 2017 09:47:49 -0500 Received: from mail-qk0-f181.google.com ([209.85.220.181]:34534 "EHLO mail-qk0-f181.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750911AbdAaOrl (ORCPT ); Tue, 31 Jan 2017 09:47:41 -0500 Date: Tue, 31 Jan 2017 09:47:05 -0500 From: Sean Paul To: John Keeping Cc: Chris Zhong , linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v3 24/24] drm/rockchip: dw-mipi-dsi: support read commands Message-ID: <20170131144705.GW20076@art_vandelay> References: <20170129132444.25251-1-john@metanate.com> <20170129132444.25251-25-john@metanate.com> <20170130152611.GA20076@art_vandelay> <20170130181427.1940024f.john@metanate.com> <20170130201609.GM20076@art_vandelay> <20170131124147.170d8588.john@metanate.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170131124147.170d8588.john@metanate.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Jan 31, 2017 at 12:41:47PM +0000, John Keeping wrote: > On Mon, 30 Jan 2017 15:16:09 -0500, Sean Paul wrote: > > > On Mon, Jan 30, 2017 at 06:14:27PM +0000, John Keeping wrote: > > > On Mon, 30 Jan 2017 10:26:11 -0500, Sean Paul wrote: > > > > > > > On Sun, Jan 29, 2017 at 01:24:44PM +0000, John Keeping wrote: > > > > > I haven't found any method for getting the length of a response, so this > > > > > just uses the requested rx_len > > > > > > > > > > Signed-off-by: John Keeping > > > > > --- > > > > > v3: > > > > > - Fix checkpatch warnings > > > > > Unchanged in v2 > > > > > > > > > > drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 56 ++++++++++++++++++++++++++++++++++ > > > > > 1 file changed, 56 insertions(+) > > > > > > > > > > diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c > > > > > index cf3ca6b0cbdb..cc58ada75425 100644 > > > > > --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c > > > > > +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c > > > > > @@ -678,6 +678,56 @@ static int dw_mipi_dsi_dcs_long_write(struct dw_mipi_dsi *dsi, > > > > > return dw_mipi_dsi_gen_pkt_hdr_write(dsi, hdr_val); > > > > > } > > > > > > > > > > +static int dw_mipi_dsi_dcs_read(struct dw_mipi_dsi *dsi, > > > > > + const struct mipi_dsi_msg *msg) > > > > > +{ > > > > > + const u8 *tx_buf = msg->tx_buf; > > > > > + u8 *rx_buf = msg->rx_buf; > > > > > + size_t i; > > > > > + int ret, val; > > > > > + > > > > > + dsi_write(dsi, DSI_PCKHDL_CFG, EN_CRC_RX | EN_ECC_RX | EN_BTA); > > > > > + dsi_write(dsi, DSI_GEN_HDR, > > > > > + GEN_HDATA(tx_buf[0]) | GEN_HTYPE(msg->type)); > > > > > + > > > > > + ret = readl_poll_timeout(dsi->base + DSI_CMD_PKT_STATUS, > > > > > + val, !(val & GEN_RD_CMD_BUSY), 1000, > > > > > + CMD_PKT_STATUS_TIMEOUT_US); > > > > > + if (ret < 0) { > > > > > + dev_err(dsi->dev, "failed to read command response\n"); > > > > > + return ret; > > > > > + } > > > > > + > > > > > + for (i = 0; i < msg->rx_len;) { > > > > > + u32 pld = dsi_read(dsi, DSI_GEN_PLD_DATA); > > > > > + > > > > > + while (i < msg->rx_len) { > > > > > + rx_buf[i] = pld & 0xff; > > > > > + pld >>= 8; > > > > > + i++; > > > > > + } > > > > > + } > > > > > > > > AFAICT, the outer for loop just initializes i and ensures msg->rx_len is > > > > non-zero? > > > > > > > > I think the following would be easier to read (and safe against the case where > > > > msg->rx_len > sizeof(pld) (even though this shouldn't happen according to DCS > > > > spec)). > > > > > > > > if (msg->rx_len > 0) { > > > > u32 pld = dsi_read(dsi, DSI_GEN_PLD_DATA); > > > > memcpy(rx_buf, &pld, MIN(msg->rx_len, sizeof(pld)); > > > > } > > > > > > I think the intent was to handle rx_len > 4, but the patch is obvously > > > completely broken regarding that. As far as I can tell, rx_len is > > > limited by the maximum return packet size which can be any value up to > > > the maximum size of a long packet, so we may need to read from the FIFO > > > multiple times. > > > > > > The loop should be something like this: > > > > > > for (i = 0; i < msg->rx_len;) { > > > u32 pld = dsi_read(dsi, DSI_GEN_PLD_DATA); > > > int j; > > > > > > for (j = 0; j < 4 && i < msg->rx_len; i++, j++) { > > > rx_buf[i] = pld & 0xff; > > > pld >>= 8; > > > } > > > } > > > > Short packets should never exceed 32 bits, so I don't think you need to add the > > nested loop. > > The read response is not restricted to a short packet. I have a panel > that documents a read request that returns up to 64KiB, admittedly with > a continuation command and the panels I have seem to only be programmed > to return 5 bytes of meaningful data, but they do return all of those > bytes in a single read response. Ah, apologies for the misunderstanding. In that case, I think you can get away with replacing the inner loop in your snippet with a memcpy and call it a day. Sean > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Sean Paul, Software Engineer, Google / Chromium OS From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sean Paul Subject: Re: [PATCH v3 24/24] drm/rockchip: dw-mipi-dsi: support read commands Date: Tue, 31 Jan 2017 09:47:05 -0500 Message-ID: <20170131144705.GW20076@art_vandelay> References: <20170129132444.25251-1-john@metanate.com> <20170129132444.25251-25-john@metanate.com> <20170130152611.GA20076@art_vandelay> <20170130181427.1940024f.john@metanate.com> <20170130201609.GM20076@art_vandelay> <20170131124147.170d8588.john@metanate.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Content-Disposition: inline In-Reply-To: <20170131124147.170d8588.john@metanate.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: John Keeping Cc: Chris Zhong , linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-arm-kernel@lists.infradead.org List-Id: linux-rockchip.vger.kernel.org T24gVHVlLCBKYW4gMzEsIDIwMTcgYXQgMTI6NDE6NDdQTSArMDAwMCwgSm9obiBLZWVwaW5nIHdy b3RlOgo+IE9uIE1vbiwgMzAgSmFuIDIwMTcgMTU6MTY6MDkgLTA1MDAsIFNlYW4gUGF1bCB3cm90 ZToKPiAKPiA+IE9uIE1vbiwgSmFuIDMwLCAyMDE3IGF0IDA2OjE0OjI3UE0gKzAwMDAsIEpvaG4g S2VlcGluZyB3cm90ZToKPiA+ID4gT24gTW9uLCAzMCBKYW4gMjAxNyAxMDoyNjoxMSAtMDUwMCwg U2VhbiBQYXVsIHdyb3RlOgo+ID4gPiAgIAo+ID4gPiA+IE9uIFN1biwgSmFuIDI5LCAyMDE3IGF0 IDAxOjI0OjQ0UE0gKzAwMDAsIEpvaG4gS2VlcGluZyB3cm90ZTogIAo+ID4gPiA+ID4gSSBoYXZl bid0IGZvdW5kIGFueSBtZXRob2QgZm9yIGdldHRpbmcgdGhlIGxlbmd0aCBvZiBhIHJlc3BvbnNl LCBzbyB0aGlzCj4gPiA+ID4gPiBqdXN0IHVzZXMgdGhlIHJlcXVlc3RlZCByeF9sZW4KPiA+ID4g PiA+IAo+ID4gPiA+ID4gU2lnbmVkLW9mZi1ieTogSm9obiBLZWVwaW5nIDxqb2huQG1ldGFuYXRl LmNvbT4KPiA+ID4gPiA+IC0tLQo+ID4gPiA+ID4gdjM6Cj4gPiA+ID4gPiAtIEZpeCBjaGVja3Bh dGNoIHdhcm5pbmdzCj4gPiA+ID4gPiBVbmNoYW5nZWQgaW4gdjIKPiA+ID4gPiA+IAo+ID4gPiA+ ID4gIGRyaXZlcnMvZ3B1L2RybS9yb2NrY2hpcC9kdy1taXBpLWRzaS5jIHwgNTYgKysrKysrKysr KysrKysrKysrKysrKysrKysrKysrKysrKwo+ID4gPiA+ID4gIDEgZmlsZSBjaGFuZ2VkLCA1NiBp bnNlcnRpb25zKCspCj4gPiA+ID4gPiAKPiA+ID4gPiA+IGRpZmYgLS1naXQgYS9kcml2ZXJzL2dw dS9kcm0vcm9ja2NoaXAvZHctbWlwaS1kc2kuYyBiL2RyaXZlcnMvZ3B1L2RybS9yb2NrY2hpcC9k dy1taXBpLWRzaS5jCj4gPiA+ID4gPiBpbmRleCBjZjNjYTZiMGNiZGIuLmNjNThhZGE3NTQyNSAx MDA2NDQKPiA+ID4gPiA+IC0tLSBhL2RyaXZlcnMvZ3B1L2RybS9yb2NrY2hpcC9kdy1taXBpLWRz aS5jCj4gPiA+ID4gPiArKysgYi9kcml2ZXJzL2dwdS9kcm0vcm9ja2NoaXAvZHctbWlwaS1kc2ku Ywo+ID4gPiA+ID4gQEAgLTY3OCw2ICs2NzgsNTYgQEAgc3RhdGljIGludCBkd19taXBpX2RzaV9k Y3NfbG9uZ193cml0ZShzdHJ1Y3QgZHdfbWlwaV9kc2kgKmRzaSwKPiA+ID4gPiA+ICAJcmV0dXJu IGR3X21pcGlfZHNpX2dlbl9wa3RfaGRyX3dyaXRlKGRzaSwgaGRyX3ZhbCk7Cj4gPiA+ID4gPiAg fQo+ID4gPiA+ID4gIAo+ID4gPiA+ID4gK3N0YXRpYyBpbnQgZHdfbWlwaV9kc2lfZGNzX3JlYWQo c3RydWN0IGR3X21pcGlfZHNpICpkc2ksCj4gPiA+ID4gPiArCQkJCWNvbnN0IHN0cnVjdCBtaXBp X2RzaV9tc2cgKm1zZykKPiA+ID4gPiA+ICt7Cj4gPiA+ID4gPiArCWNvbnN0IHU4ICp0eF9idWYg PSBtc2ctPnR4X2J1ZjsKPiA+ID4gPiA+ICsJdTggKnJ4X2J1ZiA9IG1zZy0+cnhfYnVmOwo+ID4g PiA+ID4gKwlzaXplX3QgaTsKPiA+ID4gPiA+ICsJaW50IHJldCwgdmFsOwo+ID4gPiA+ID4gKwo+ ID4gPiA+ID4gKwlkc2lfd3JpdGUoZHNpLCBEU0lfUENLSERMX0NGRywgRU5fQ1JDX1JYIHwgRU5f RUNDX1JYIHwgRU5fQlRBKTsKPiA+ID4gPiA+ICsJZHNpX3dyaXRlKGRzaSwgRFNJX0dFTl9IRFIs Cj4gPiA+ID4gPiArCQkgIEdFTl9IREFUQSh0eF9idWZbMF0pIHwgR0VOX0hUWVBFKG1zZy0+dHlw ZSkpOwo+ID4gPiA+ID4gKwo+ID4gPiA+ID4gKwlyZXQgPSByZWFkbF9wb2xsX3RpbWVvdXQoZHNp LT5iYXNlICsgRFNJX0NNRF9QS1RfU1RBVFVTLAo+ID4gPiA+ID4gKwkJCQkgdmFsLCAhKHZhbCAm IEdFTl9SRF9DTURfQlVTWSksIDEwMDAsCj4gPiA+ID4gPiArCQkJCSBDTURfUEtUX1NUQVRVU19U SU1FT1VUX1VTKTsKPiA+ID4gPiA+ICsJaWYgKHJldCA8IDApIHsKPiA+ID4gPiA+ICsJCWRldl9l cnIoZHNpLT5kZXYsICJmYWlsZWQgdG8gcmVhZCBjb21tYW5kIHJlc3BvbnNlXG4iKTsKPiA+ID4g PiA+ICsJCXJldHVybiByZXQ7Cj4gPiA+ID4gPiArCX0KPiA+ID4gPiA+ICsKPiA+ID4gPiA+ICsJ Zm9yIChpID0gMDsgaSA8IG1zZy0+cnhfbGVuOykgewo+ID4gPiA+ID4gKwkJdTMyIHBsZCA9IGRz aV9yZWFkKGRzaSwgRFNJX0dFTl9QTERfREFUQSk7Cj4gPiA+ID4gPiArCj4gPiA+ID4gPiArCQl3 aGlsZSAoaSA8IG1zZy0+cnhfbGVuKSB7Cj4gPiA+ID4gPiArCQkJcnhfYnVmW2ldID0gcGxkICYg MHhmZjsKPiA+ID4gPiA+ICsJCQlwbGQgPj49IDg7Cj4gPiA+ID4gPiArCQkJaSsrOwo+ID4gPiA+ ID4gKwkJfQo+ID4gPiA+ID4gKwl9ICAgIAo+ID4gPiA+IAo+ID4gPiA+IEFGQUlDVCwgdGhlIG91 dGVyIGZvciBsb29wIGp1c3QgaW5pdGlhbGl6ZXMgaSBhbmQgZW5zdXJlcyBtc2ctPnJ4X2xlbiBp cwo+ID4gPiA+IG5vbi16ZXJvPyAKPiA+ID4gPiAKPiA+ID4gPiBJIHRoaW5rIHRoZSBmb2xsb3dp bmcgd291bGQgYmUgZWFzaWVyIHRvIHJlYWQgKGFuZCBzYWZlIGFnYWluc3QgdGhlIGNhc2Ugd2hl cmUKPiA+ID4gPiBtc2ctPnJ4X2xlbiA+IHNpemVvZihwbGQpIChldmVuIHRob3VnaCB0aGlzIHNo b3VsZG4ndCBoYXBwZW4gYWNjb3JkaW5nIHRvIERDUwo+ID4gPiA+IHNwZWMpKS4KPiA+ID4gPiAK PiA+ID4gPiBpZiAobXNnLT5yeF9sZW4gPiAwKSB7Cj4gPiA+ID4gICAgICAgICB1MzIgcGxkID0g ZHNpX3JlYWQoZHNpLCBEU0lfR0VOX1BMRF9EQVRBKTsKPiA+ID4gPiAgICAgICAgIG1lbWNweShy eF9idWYsICZwbGQsIE1JTihtc2ctPnJ4X2xlbiwgc2l6ZW9mKHBsZCkpOwo+ID4gPiA+IH0gIAo+ ID4gPiAKPiA+ID4gSSB0aGluayB0aGUgaW50ZW50IHdhcyB0byBoYW5kbGUgcnhfbGVuID4gNCwg YnV0IHRoZSBwYXRjaCBpcyBvYnZvdXNseQo+ID4gPiBjb21wbGV0ZWx5IGJyb2tlbiByZWdhcmRp bmcgdGhhdC4gIEFzIGZhciBhcyBJIGNhbiB0ZWxsLCByeF9sZW4gaXMKPiA+ID4gbGltaXRlZCBi eSB0aGUgbWF4aW11bSByZXR1cm4gcGFja2V0IHNpemUgd2hpY2ggY2FuIGJlIGFueSB2YWx1ZSB1 cCB0bwo+ID4gPiB0aGUgbWF4aW11bSBzaXplIG9mIGEgbG9uZyBwYWNrZXQsIHNvIHdlIG1heSBu ZWVkIHRvIHJlYWQgZnJvbSB0aGUgRklGTwo+ID4gPiBtdWx0aXBsZSB0aW1lcy4KPiA+ID4gCj4g PiA+IFRoZSBsb29wIHNob3VsZCBiZSBzb21ldGhpbmcgbGlrZSB0aGlzOgo+ID4gPiAKPiA+ID4g CWZvciAoaSA9IDA7IGkgPCBtc2ctPnJ4X2xlbjspIHsKPiA+ID4gCQl1MzIgcGxkID0gZHNpX3Jl YWQoZHNpLCBEU0lfR0VOX1BMRF9EQVRBKTsKPiA+ID4gCQlpbnQgajsKPiA+ID4gCj4gPiA+IAkJ Zm9yIChqID0gMDsgaiA8IDQgJiYgaSA8IG1zZy0+cnhfbGVuOyBpKyssIGorKykgewo+ID4gPiAJ CQlyeF9idWZbaV0gPSBwbGQgJiAweGZmOwo+ID4gPiAJCQlwbGQgPj49IDg7Cj4gPiA+IAkJfQo+ ID4gPiAJfSAgCj4gPiAKPiA+IFNob3J0IHBhY2tldHMgc2hvdWxkIG5ldmVyIGV4Y2VlZCAzMiBi aXRzLCBzbyBJIGRvbid0IHRoaW5rIHlvdSBuZWVkIHRvIGFkZCB0aGUKPiA+IG5lc3RlZCBsb29w Lgo+IAo+IFRoZSByZWFkIHJlc3BvbnNlIGlzIG5vdCByZXN0cmljdGVkIHRvIGEgc2hvcnQgcGFj a2V0LiAgSSBoYXZlIGEgcGFuZWwKPiB0aGF0IGRvY3VtZW50cyBhIHJlYWQgcmVxdWVzdCB0aGF0 IHJldHVybnMgdXAgdG8gNjRLaUIsIGFkbWl0dGVkbHkgd2l0aAo+IGEgY29udGludWF0aW9uIGNv bW1hbmQgYW5kIHRoZSBwYW5lbHMgSSBoYXZlIHNlZW0gdG8gb25seSBiZSBwcm9ncmFtbWVkCj4g dG8gcmV0dXJuIDUgYnl0ZXMgb2YgbWVhbmluZ2Z1bCBkYXRhLCBidXQgdGhleSBkbyByZXR1cm4g YWxsIG9mIHRob3NlCj4gYnl0ZXMgaW4gYSBzaW5nbGUgcmVhZCByZXNwb25zZS4KCkFoLCBhcG9s b2dpZXMgZm9yIHRoZSBtaXN1bmRlcnN0YW5kaW5nLiBJbiB0aGF0IGNhc2UsIEkgdGhpbmsgeW91 IGNhbiBnZXQgYXdheQp3aXRoIHJlcGxhY2luZyB0aGUgaW5uZXIgbG9vcCBpbiB5b3VyIHNuaXBw ZXQgd2l0aCBhIG1lbWNweSBhbmQgY2FsbCBpdCBhIGRheS4KClNlYW4KCj4gX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KPiBkcmktZGV2ZWwgbWFpbGluZyBs aXN0Cj4gZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9yZwo+IGh0dHBzOi8vbGlzdHMuZnJl ZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCgotLSAKU2VhbiBQYXVsLCBT b2Z0d2FyZSBFbmdpbmVlciwgR29vZ2xlIC8gQ2hyb21pdW0gT1MKX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVsIG1haWxpbmcgbGlzdApkcmkt ZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlzdHMuZnJlZWRlc2t0b3Aub3Jn L21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 From: seanpaul@chromium.org (Sean Paul) Date: Tue, 31 Jan 2017 09:47:05 -0500 Subject: [PATCH v3 24/24] drm/rockchip: dw-mipi-dsi: support read commands In-Reply-To: <20170131124147.170d8588.john@metanate.com> References: <20170129132444.25251-1-john@metanate.com> <20170129132444.25251-25-john@metanate.com> <20170130152611.GA20076@art_vandelay> <20170130181427.1940024f.john@metanate.com> <20170130201609.GM20076@art_vandelay> <20170131124147.170d8588.john@metanate.com> Message-ID: <20170131144705.GW20076@art_vandelay> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Tue, Jan 31, 2017 at 12:41:47PM +0000, John Keeping wrote: > On Mon, 30 Jan 2017 15:16:09 -0500, Sean Paul wrote: > > > On Mon, Jan 30, 2017 at 06:14:27PM +0000, John Keeping wrote: > > > On Mon, 30 Jan 2017 10:26:11 -0500, Sean Paul wrote: > > > > > > > On Sun, Jan 29, 2017 at 01:24:44PM +0000, John Keeping wrote: > > > > > I haven't found any method for getting the length of a response, so this > > > > > just uses the requested rx_len > > > > > > > > > > Signed-off-by: John Keeping > > > > > --- > > > > > v3: > > > > > - Fix checkpatch warnings > > > > > Unchanged in v2 > > > > > > > > > > drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 56 ++++++++++++++++++++++++++++++++++ > > > > > 1 file changed, 56 insertions(+) > > > > > > > > > > diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c > > > > > index cf3ca6b0cbdb..cc58ada75425 100644 > > > > > --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c > > > > > +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c > > > > > @@ -678,6 +678,56 @@ static int dw_mipi_dsi_dcs_long_write(struct dw_mipi_dsi *dsi, > > > > > return dw_mipi_dsi_gen_pkt_hdr_write(dsi, hdr_val); > > > > > } > > > > > > > > > > +static int dw_mipi_dsi_dcs_read(struct dw_mipi_dsi *dsi, > > > > > + const struct mipi_dsi_msg *msg) > > > > > +{ > > > > > + const u8 *tx_buf = msg->tx_buf; > > > > > + u8 *rx_buf = msg->rx_buf; > > > > > + size_t i; > > > > > + int ret, val; > > > > > + > > > > > + dsi_write(dsi, DSI_PCKHDL_CFG, EN_CRC_RX | EN_ECC_RX | EN_BTA); > > > > > + dsi_write(dsi, DSI_GEN_HDR, > > > > > + GEN_HDATA(tx_buf[0]) | GEN_HTYPE(msg->type)); > > > > > + > > > > > + ret = readl_poll_timeout(dsi->base + DSI_CMD_PKT_STATUS, > > > > > + val, !(val & GEN_RD_CMD_BUSY), 1000, > > > > > + CMD_PKT_STATUS_TIMEOUT_US); > > > > > + if (ret < 0) { > > > > > + dev_err(dsi->dev, "failed to read command response\n"); > > > > > + return ret; > > > > > + } > > > > > + > > > > > + for (i = 0; i < msg->rx_len;) { > > > > > + u32 pld = dsi_read(dsi, DSI_GEN_PLD_DATA); > > > > > + > > > > > + while (i < msg->rx_len) { > > > > > + rx_buf[i] = pld & 0xff; > > > > > + pld >>= 8; > > > > > + i++; > > > > > + } > > > > > + } > > > > > > > > AFAICT, the outer for loop just initializes i and ensures msg->rx_len is > > > > non-zero? > > > > > > > > I think the following would be easier to read (and safe against the case where > > > > msg->rx_len > sizeof(pld) (even though this shouldn't happen according to DCS > > > > spec)). > > > > > > > > if (msg->rx_len > 0) { > > > > u32 pld = dsi_read(dsi, DSI_GEN_PLD_DATA); > > > > memcpy(rx_buf, &pld, MIN(msg->rx_len, sizeof(pld)); > > > > } > > > > > > I think the intent was to handle rx_len > 4, but the patch is obvously > > > completely broken regarding that. As far as I can tell, rx_len is > > > limited by the maximum return packet size which can be any value up to > > > the maximum size of a long packet, so we may need to read from the FIFO > > > multiple times. > > > > > > The loop should be something like this: > > > > > > for (i = 0; i < msg->rx_len;) { > > > u32 pld = dsi_read(dsi, DSI_GEN_PLD_DATA); > > > int j; > > > > > > for (j = 0; j < 4 && i < msg->rx_len; i++, j++) { > > > rx_buf[i] = pld & 0xff; > > > pld >>= 8; > > > } > > > } > > > > Short packets should never exceed 32 bits, so I don't think you need to add the > > nested loop. > > The read response is not restricted to a short packet. I have a panel > that documents a read request that returns up to 64KiB, admittedly with > a continuation command and the panels I have seem to only be programmed > to return 5 bytes of meaningful data, but they do return all of those > bytes in a single read response. Ah, apologies for the misunderstanding. In that case, I think you can get away with replacing the inner loop in your snippet with a memcpy and call it a day. Sean > _______________________________________________ > dri-devel mailing list > dri-devel at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Sean Paul, Software Engineer, Google / Chromium OS