From mboxrd@z Thu Jan 1 00:00:00 1970 From: Brian Norris Subject: Re: [PATCH v2 2/3] drm/rockchip: Add ROCKCHIP DW MIPI DSI controller driver Date: Mon, 27 Nov 2017 21:24:02 -0800 Message-ID: <20171128052400.GA14640@google.com> References: <1511834125-7756-1-git-send-email-nickey.yang@rock-chips.com> <1511834125-7756-3-git-send-email-nickey.yang@rock-chips.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Content-Disposition: inline In-Reply-To: <1511834125-7756-3-git-send-email-nickey.yang@rock-chips.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Nickey Yang Cc: mark.rutland@arm.com, hl@rock-chips.com, airlied@linux.ie, hoegsberg@gmail.com, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, philippe.cornu@st.com, yannick.fertre@st.com, linux-rockchip@lists.infradead.org, robh+dt@kernel.org, zyw@rock-chips.com, xbl@rock-chips.com, mka@chromium.org, mark.yao@rock-chips.com List-Id: linux-rockchip.vger.kernel.org SGkgTmlja2V5LAoKVGhhbmtzIGZvciB0aGUgcXVpY2sgdmVyc2lvbiAyLiBZb3UndmUgZml4ZWQg bW9zdCBvZiB0aGUgdGhlIHByb2JsZW1zIEkKcG9pbnRlZCBvdXQsIGJ1dCB5b3UndmUgbWlzc2Vk IGF0IGxlYXN0IG9uZS4KCk9uIFR1ZSwgTm92IDI4LCAyMDE3IGF0IDA5OjU1OjI0QU0gKzA4MDAs IE5pY2tleSBZYW5nIHdyb3RlOgo+IEFkZCB0aGUgUk9DS0NISVAgRFNJIGNvbnRyb2xsZXIgZHJp dmVyIHRoYXQgdXNlcyB0aGUgU3lub3BzeXMgRGVzaWduV2FyZQo+IE1JUEkgRFNJIGhvc3QgY29u dHJvbGxlciBicmlkZ2UuCj4gCj4gU2lnbmVkLW9mZi1ieTogTmlja2V5IFlhbmcgPG5pY2tleS55 YW5nQHJvY2stY2hpcHMuY29tPgo+IC0tLQoKSSBzdXBwb3NlIHlvdSd2ZSBpbmNsdWRlZCBhIGNo YW5nZWxvZyBpbiB0aGUgY292ZXIgbGV0dGVyICh0aGFua3MhKSwgYnV0CnNvbWV0aW1lcyBpdCBo ZWxwcyB0byBnbyBoZXJlIHRvbywgdG8gZ2l2ZSBhbiBpbmRpdmlkdWFsIGhpc3Rvcnkgb2YgZWFj aApwYXRjaC4KCj4gIGRyaXZlcnMvZ3B1L2RybS9yb2NrY2hpcC9LY29uZmlnICAgICAgICAgICAg ICAgIHwgICAgMiArLQo+ICBkcml2ZXJzL2dwdS9kcm0vcm9ja2NoaXAvTWFrZWZpbGUgICAgICAg ICAgICAgICB8ICAgIDIgKy0KPiAgZHJpdmVycy9ncHUvZHJtL3JvY2tjaGlwL2R3LW1pcGktZHNp LmMgICAgICAgICAgfCAxMzQ5IC0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tCj4gIGRyaXZlcnMvZ3B1 L2RybS9yb2NrY2hpcC9kdy1taXBpLWRzaV9yb2NrY2hpcC5jIHwgIDc1NiArKysrKysrKysrKysr Cj4gIGRyaXZlcnMvZ3B1L2RybS9yb2NrY2hpcC9yb2NrY2hpcF9kcm1fZHJ2LmMgICAgIHwgICAg MiArLQo+ICBkcml2ZXJzL2dwdS9kcm0vcm9ja2NoaXAvcm9ja2NoaXBfZHJtX2Rydi5oICAgICB8 ICAgIDIgKy0KPiAgNiBmaWxlcyBjaGFuZ2VkLCA3NjAgaW5zZXJ0aW9ucygrKSwgMTM1MyBkZWxl dGlvbnMoLSkKPiAgZGVsZXRlIG1vZGUgMTAwNjQ0IGRyaXZlcnMvZ3B1L2RybS9yb2NrY2hpcC9k dy1taXBpLWRzaS5jCj4gIGNyZWF0ZSBtb2RlIDEwMDY0NCBkcml2ZXJzL2dwdS9kcm0vcm9ja2No aXAvZHctbWlwaS1kc2lfcm9ja2NoaXAuYwo+IAoKCj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvZ3B1 L2RybS9yb2NrY2hpcC9kdy1taXBpLWRzaV9yb2NrY2hpcC5jIGIvZHJpdmVycy9ncHUvZHJtL3Jv Y2tjaGlwL2R3LW1pcGktZHNpX3JvY2tjaGlwLmMKPiBuZXcgZmlsZSBtb2RlIDEwMDY0NAo+IGlu ZGV4IDAwMDAwMDAuLmM5MTlkNGYKPiAtLS0gL2Rldi9udWxsCj4gKysrIGIvZHJpdmVycy9ncHUv ZHJtL3JvY2tjaGlwL2R3LW1pcGktZHNpX3JvY2tjaGlwLmMKPiBAQCAtMCwwICsxLDc1NiBAQApb Li4uXQo+ICtzdGF0aWMgaW50IGR3X21pcGlfZHNpX3JvY2tjaGlwX2JpbmQoc3RydWN0IGRldmlj ZSAqZGV2LAo+ICsJCQkJICAgICBzdHJ1Y3QgZGV2aWNlICptYXN0ZXIsCj4gKwkJCQkgICAgIHZv aWQgKmRhdGEpCj4gK3sKPiArCWNvbnN0IHN0cnVjdCBvZl9kZXZpY2VfaWQgKm9mX2lkID0KPiAr CQkJb2ZfbWF0Y2hfZGV2aWNlKGR3X21pcGlfZHNpX3JvY2tjaGlwX2R0X2lkcywgZGV2KTsKPiAr CWNvbnN0IHN0cnVjdCByb2NrY2hpcF9kd19kc2lfY2hpcF9kYXRhICpjZGF0YSA9IG9mX2lkLT5k YXRhOwo+ICsJc3RydWN0IGR3X21pcGlfZHNpX3JvY2tjaGlwICpkc2kgPSBkZXZfZ2V0X2RydmRh dGEoZGV2KTsKCkhtbSwgc28gSSBzZWUgMiBpbnN0YW5jZXMgb2YgJ2Rldl9nZXRfZHJ2ZGF0YSgp JyBpbiB0aGlzIGRyaXZlciwgYW5kIG9uZQpvZiB0aGVtIGlzIGhlcmUuIEkgbmV2ZXIgc2VlIGEg J2Rldl9zZXRfZHJ2ZGF0YSgpJywgc28gdGhhdCBjYW4ndApwb3NzaWJseSBiZSB1c2VmdWwuCgpJ biB0aGlzIGNhc2UuLi55b3UndmUgYWN0dWFsbHkgbW92ZWQgYWxsIHRoZSAncHJvYmUoKScgbG9n aWMgaW50bwpiaW5kKCksIHNvIHRoZXJlJ3Mgbm8gZHJpdmVyIGRhdGEgdG8gY2Fycnkgb3ZlciBo ZXJlLiBBbmQgaW5zdGVhZCwgeW91Cmp1c3QgY2xvYmJlciB0aGlzICdkc2kgPSAuLi4nIGFzc2ln bm1lbnQgYSBmZXcgbGluZXMgYmVsb3c6Cgo+ICsJc3RydWN0IHJlc291cmNlICpyZXM7Cj4gKwlz dHJ1Y3QgcGxhdGZvcm1fZGV2aWNlICpwZGV2ID0gdG9fcGxhdGZvcm1fZGV2aWNlKGRldik7Cj4g KwlzdHJ1Y3QgZHJtX2RldmljZSAqZHJtX2RldiA9IGRhdGE7Cj4gKwlzdHJ1Y3QgZGV2aWNlX25v ZGUgKm5wID0gZGV2LT5vZl9ub2RlOwo+ICsJaW50IHJldDsKPiArCj4gKwlkc2kgPSBkZXZtX2t6 YWxsb2MoZGV2LCBzaXplb2YoKmRzaSksIEdGUF9LRVJORUwpOwoKXl5eIFNlZSBoZXJlLgoKU28g aWYgeW91IGRvbid0IGFjdHVhbGx5IG5lZWQgdG8gc3Rhc2ggYW55dGhpbmcgaW4gZHJ2ZGF0YSBi ZXR3ZWVuCnByb2JlKCkgYW5kIGJpbmQoKSwgdGhlbiB5b3UgbWlnaHQgZ2V0IGF3YXkgd2l0aG91 dCBteSBwYXRjaDoKCltQQVRDSF0gZHJtL2JyaWRnZS9zeW5vcHNpczogc3RvcCBjbG9iYmVyaW5n IGRydmRhdGEKaHR0cHM6Ly9wYXRjaHdvcmsua2VybmVsLm9yZy9wYXRjaC8xMDA3ODQ5My8KClRo ZXJlJ3Mgb25lIG90aGVyIGluc3RhbmNlIHRvIHJldmlldyB0aG91Z2g6Cgo+ICsJaWYgKCFkc2kp Cj4gKwkJcmV0dXJuIC1FTk9NRU07Cj4gKwpbLi4uXQoKPiArc3RhdGljIHZvaWQgZHdfbWlwaV9k c2lfcm9ja2NoaXBfdW5iaW5kKHN0cnVjdCBkZXZpY2UgKmRldiwKPiArCQkJCQlzdHJ1Y3QgZGV2 aWNlICptYXN0ZXIsCj4gKwkJCQkJdm9pZCAqZGF0YSkKPiArewo+ICsJc3RydWN0IGR3X21pcGlf ZHNpX3JvY2tjaGlwICpkc2kgPSBkZXZfZ2V0X2RydmRhdGEoZGV2KTsKCl5eIFRoYXQncyB3cm9u Zy4gWW91IGVpdGhlciB3YW50IHRvIGRvIHlvdXIgb3duICdkZXZfc2V0X2RydmRhdGEoKScKc29t ZXdoZXJlIChhbmQgbWFrZSBzdXJlIHRoZSBicmlkZ2UgZHJpdmVyIGRvZXNuJ3QgY2xvYmJlciBp dCwgZS5nLiwKd2l0aCBteSBwYXRjaCBhYm92ZSksIG9yIGVsc2UgeW91IG5lZWQgdG8gY29udmlu Y2UgdGhlIGJyaWRnZSBkcml2ZXIgdG8KZ2l2ZSB1cyBiYWNrIHNvbWV0aGluZyBsaWtlIGl0cyAn cHJpdl9kYXRhJyBwb2ludGVyCihkc2ktPnBsYXRfZGF0YS0+cHJpdl9kYXRhKS4KCkFsc28sIGRv bid0IHlvdSBuZWVkIHRvIGNhbGwgZHdfbWlwaV9kc2lfdW5iaW5kKCkgaGVyZT8KCj4gKwljbGtf ZGlzYWJsZV91bnByZXBhcmUoZHNpLT5wbGxyZWZfY2xrKTsKPiArfQo+ICsKPiArc3RhdGljIGNv bnN0IHN0cnVjdCBjb21wb25lbnRfb3BzIGR3X21pcGlfZHNpX3JvY2tjaGlwX29wcyA9IHsKPiAr CS5iaW5kCT0gZHdfbWlwaV9kc2lfcm9ja2NoaXBfYmluZCwKPiArCS51bmJpbmQJPSBkd19taXBp X2RzaV9yb2NrY2hpcF91bmJpbmQsCj4gK307CgouLi4KCkJyaWFuCl9fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCmRyaS1kZXZlbCBtYWlsaW5nIGxpc3QKZHJp LWRldmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpodHRwczovL2xpc3RzLmZyZWVkZXNrdG9wLm9y Zy9tYWlsbWFuL2xpc3RpbmZvL2RyaS1kZXZlbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751495AbdK1FYH (ORCPT ); Tue, 28 Nov 2017 00:24:07 -0500 Received: from mail-io0-f193.google.com ([209.85.223.193]:33616 "EHLO mail-io0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750835AbdK1FYG (ORCPT ); Tue, 28 Nov 2017 00:24:06 -0500 X-Google-Smtp-Source: AGs4zMb7ZAOMZR7MEccCo+LQlQDvFo+ThCxmINIEQ/7Eoq6YX6P7gQ2xaV8b26VgSQyZYI/5xCxiUA== Date: Mon, 27 Nov 2017 21:24:02 -0800 From: Brian Norris To: Nickey Yang Cc: mark.yao@rock-chips.com, robh+dt@kernel.org, heiko@sntech.de, mark.rutland@arm.com, airlied@linux.ie, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-rockchip@lists.infradead.org, seanpaul@chromium.org, mka@chromium.org, hoegsberg@gmail.com, architt@codeaurora.org, philippe.cornu@st.com, yannick.fertre@st.com, hl@rock-chips.com, zyw@rock-chips.com, xbl@rock-chips.com Subject: Re: [PATCH v2 2/3] drm/rockchip: Add ROCKCHIP DW MIPI DSI controller driver Message-ID: <20171128052400.GA14640@google.com> References: <1511834125-7756-1-git-send-email-nickey.yang@rock-chips.com> <1511834125-7756-3-git-send-email-nickey.yang@rock-chips.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1511834125-7756-3-git-send-email-nickey.yang@rock-chips.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 Hi Nickey, Thanks for the quick version 2. You've fixed most of the the problems I pointed out, but you've missed at least one. On Tue, Nov 28, 2017 at 09:55:24AM +0800, Nickey Yang wrote: > Add the ROCKCHIP DSI controller driver that uses the Synopsys DesignWare > MIPI DSI host controller bridge. > > Signed-off-by: Nickey Yang > --- I suppose you've included a changelog in the cover letter (thanks!), but sometimes it helps to go here too, to give an individual history of each patch. > drivers/gpu/drm/rockchip/Kconfig | 2 +- > drivers/gpu/drm/rockchip/Makefile | 2 +- > drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 1349 ----------------------- > drivers/gpu/drm/rockchip/dw-mipi-dsi_rockchip.c | 756 +++++++++++++ > drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 2 +- > drivers/gpu/drm/rockchip/rockchip_drm_drv.h | 2 +- > 6 files changed, 760 insertions(+), 1353 deletions(-) > delete mode 100644 drivers/gpu/drm/rockchip/dw-mipi-dsi.c > create mode 100644 drivers/gpu/drm/rockchip/dw-mipi-dsi_rockchip.c > > diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi_rockchip.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi_rockchip.c > new file mode 100644 > index 0000000..c919d4f > --- /dev/null > +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi_rockchip.c > @@ -0,0 +1,756 @@ [...] > +static int dw_mipi_dsi_rockchip_bind(struct device *dev, > + struct device *master, > + void *data) > +{ > + const struct of_device_id *of_id = > + of_match_device(dw_mipi_dsi_rockchip_dt_ids, dev); > + const struct rockchip_dw_dsi_chip_data *cdata = of_id->data; > + struct dw_mipi_dsi_rockchip *dsi = dev_get_drvdata(dev); Hmm, so I see 2 instances of 'dev_get_drvdata()' in this driver, and one of them is here. I never see a 'dev_set_drvdata()', so that can't possibly be useful. In this case...you've actually moved all the 'probe()' logic into bind(), so there's no driver data to carry over here. And instead, you just clobber this 'dsi = ...' assignment a few lines below: > + struct resource *res; > + struct platform_device *pdev = to_platform_device(dev); > + struct drm_device *drm_dev = data; > + struct device_node *np = dev->of_node; > + int ret; > + > + dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL); ^^^ See here. So if you don't actually need to stash anything in drvdata between probe() and bind(), then you might get away without my patch: [PATCH] drm/bridge/synopsis: stop clobbering drvdata https://patchwork.kernel.org/patch/10078493/ There's one other instance to review though: > + if (!dsi) > + return -ENOMEM; > + [...] > +static void dw_mipi_dsi_rockchip_unbind(struct device *dev, > + struct device *master, > + void *data) > +{ > + struct dw_mipi_dsi_rockchip *dsi = dev_get_drvdata(dev); ^^ That's wrong. You either want to do your own 'dev_set_drvdata()' somewhere (and make sure the bridge driver doesn't clobber it, e.g., with my patch above), or else you need to convince the bridge driver to give us back something like its 'priv_data' pointer (dsi->plat_data->priv_data). Also, don't you need to call dw_mipi_dsi_unbind() here? > + clk_disable_unprepare(dsi->pllref_clk); > +} > + > +static const struct component_ops dw_mipi_dsi_rockchip_ops = { > + .bind = dw_mipi_dsi_rockchip_bind, > + .unbind = dw_mipi_dsi_rockchip_unbind, > +}; ... Brian