From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH] drm/bridge/synopsis: stop clobbering drvdata Date: Tue, 09 Jan 2018 15:36:53 +0200 Message-ID: <15815822.Bd0fLxLFzJ@avalon> References: <20171128010538.119114-1-briannorris@chromium.org> <1743921.AofnJoQ8Rz@avalon> <20171128182121.GA116479@google.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20171128182121.GA116479@google.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: dri-devel@lists.freedesktop.org Cc: Brian Norris , hl@rock-chips.com, David Airlie , linux-kernel@vger.kernel.org, Jeffy Chen , Doug Anderson , Philippe Cornu , Yannick Fertre , linux-rockchip@lists.infradead.org, Nickey Yang , mka@chromium.org, Vincent Abriou List-Id: linux-rockchip.vger.kernel.org SGkgQnJpYW4sCgpPbiBUdWVzZGF5LCAyOCBOb3ZlbWJlciAyMDE3IDIwOjIxOjIzIEVFVCBCcmlh biBOb3JyaXMgd3JvdGU6Cj4gT24gVHVlLCBOb3YgMjgsIDIwMTcgYXQgMDI6NTE6NDZQTSArMDIw MCwgTGF1cmVudCBQaW5jaGFydCB3cm90ZToKPiA+IEhpIEJyaWFuLAo+ID4gCj4gPiBUaGFuayB5 b3UgZm9yIHRoZSBwYXRjaC4KPiA+IAo+ID4gSSdkIG1lbnRpb24gZHctbWlwaS1kc2kgaW4gdGhl IHN1YmplY3QgbGluZSBhcyB0aGUgZGlyZWN0b3J5IGNvbnRhaW5zIHRoZQo+ID4gZHctaGRtaSBk cml2ZXIgYXMgd2VsbCB0aGF0IHRoaXMgcGF0Y2ggZG9lc24ndCB0b3VjaC4KPiAKPiBZZXAuIERv ZXMgaXQgbmVlZCBhbm90aGVyIHRhZyBpbiB0aGUgc3ViamVjdD8gZS5nLiwgJy4uLi9kdy1taXBp LWRzaTonPwo+IAo+ID4gT24gVHVlc2RheSwgMjggTm92ZW1iZXIgMjAxNyAwMzowNTozOCBFRVQg QnJpYW4gTm9ycmlzIHdyb3RlOgo+ID4+IEJyaWRnZSBkcml2ZXJzL2hlbHBlcnMgc2hvdWxkbid0 IGJlIGNsb2JiZXJpbmcgdGhlIGRydmRhdGEsIHNpbmNlIGEKPiA+PiBwYXJlbnQgZHJpdmVyIG1p Z2h0IG5lZWQgdG8gb3duIHRoaXMuCj4gPiAKPiA+IEJ5IHBhcmVudCBkcml2ZXIgSSBhc3N1bWUg eW91IG1lYW4gYSBnbHVlIGRyaXZlciB0aGF0IGJpbmRzIHRvIHRoZSBTb0MtCj4gPiBzcGVjaWZp YyBjb21wYXRpYmxlIHN0cmluZyBmb3IgdGhlIERTSSB0cmFuc21pdHRlci4KPiAKPiBJbmRlZWQu IE5pY2tleSBwaWNrZWQgdGhpcyB1cCBmb3IgaGlzIFJvY2tjaGlwIGRyaXZlciBzdWJtaXNzaW9u LCBidXQKPiBtYXliZSB3ZSBzaG91bGQgcmV3b3JkIHRoZSBjb21taXQgbWVzc2FnZSBhIGJpdC4K CkhvdyBhYm91dCAiZHJtOiBkdy1taXBpLWRzaTogU3RvcCBjbG9iYmVyaW5nIGRydmRhdGEiID8K Cj4gPj4gSW5zdGVhZCwgbGV0J3MgcmV0dXJuIG91cgo+ID4+ICdkd19taXBpX2RzaScgb2JqZWN0 IGFuZCBoYXZlIGNhbGxlcnMgcGFzcyB0aGF0IGJhY2sgdG8gdXMgZm9yIHJlbW92YWwuCj4gPj4g Cj4gPj4gU2lnbmVkLW9mZi1ieTogQnJpYW4gTm9ycmlzIDxicmlhbm5vcnJpc0BjaHJvbWl1bS5v cmc+Cj4gPiAKPiA+IFdvdWxkbid0IGl0IGJlIGNsZWFuZXIgdG8gZW1iZWQgdGhlIGR3X21pcGlf ZHNpIHN0cnVjdHVyZSBpbiB0aGUgcGFyZW50LQo+ID4gc3BlY2lmaWMgZGF0YSBzdHJ1Y3R1cmUg KHN0cnVjdCBkd19taXBpX2RzaV9zdG0gYW5kIHN0cnVjdAo+ID4gZHdfbWlwaV9kc2lfcm9ja2No aXAgd2hlbiB0aGUgIltQQVRDSCB2MyAwLzVdIFVwZGF0ZSBST0NLQ0hJUCBEU0kgZHJpdmVyCj4g PiB0aGF0IHVzZXMgZHctbWlwaS1kc2kgYnJpZGdlIiBwYXRjaCBzZXJpZXMgd2lsbCBsYW5kKSBp bnN0ZWFkIG9mCj4gPiBhbGxvY2F0aW5nIGl0IGR5bmFtaWNhbGx5ID8gV2Ugd291bGQgdGhlbiBo YXZlIGEgc2luZ2xlIG9iamVjdCB0byB0cmFjay4KPiAKPiBJIHN1cHBvc2Ugd2UgY291bGQgZG8g dGhhdCB0b28uIEJ1dCB0aGF0IHdvdWxkIHJlcXVpcmUgZXhwb3NpbmcgdGhlCj4gd2hvbGUgbGF5 b3V0IG9mICdzdHJ1Y3QgZHdfbWlwaV9kc2knIHRvIHVzZXJzLiBEbyB3ZSB3YW50IHRvIHNhY3Jp ZmljZQo+IHRoZSBlbmZvcmNlZCBzZXBhcmF0aW9uIGZvciBhIGxpdHRsZSBiaXQgb2YgbmljZXIg b2JqZWN0IGhhbmRsaW5nPwoKSSBjZXJ0YWlubHkgZG9uJ3QgdGhpbmsgd2Ugc2hvdWxkIGdvIGZv ciBzcGFnaGV0dGkgY29kZSB3aXRoIGFsbCBvYmplY3RzIAphY2Nlc3NpbmcgZWFjaCBvdGhlciA6 KSBPbiB0aGUgb3RoZXIgaGFuZCwgd2UncmUgdGFsa2luZyBhYm91dCBDIGNvZGUsIGFuZCB3ZSAK dGh1cyBoYXZlIG5vIHdheSB0byBlbmZvcmNlIGFjY2VzcyByZXN0cmljdGlvbnMgaW4gdGhlIGNv bXBpbGVyLCBzbyBpdCdzIGEgCmxvc3QgYmF0dGxlIGFueXdheS4gSSBkb24ndCBzZWUgYW4gaXNz dWUgd2l0aCBleHBvc2luZyB0aGUgb2JqZWN0IGluIHRoZSBzZW5zZSAKb2YgbW92aW5nIGl0cyBk ZWZpbml0aW9uIHRvIGEgaGVhZGVyIGZpbGUgaWYgaXQgcmVzdWx0cyBpbiBjbGVhbmVyIGNvZGUu IEkgCnRoaW5rIHdlIG5lZWQgdG8gdHJ1c3QgZGV2ZWxvcGVycyBub3QgdG8gYWJ1c2UgaW50ZXJu YWwgQVBJcywgYW5kIGlmIHRoZXkgZG8sIApjYXRjaCBpdCBkdXJpbmcgcmV2aWV3LgoKPiBBbHNv LCB0aGlzIHdhcyBtb2RlbGVkIGEgYml0IGFmdGVyIHRoZSBzaW1pbGFyIHJld29yayBuZWVkZWQg dG8gdW50YW5nbGUKPiB0aGUgZHJ2ZGF0YSBoYW5kbGluZyBpbiB0aGUgUm9ja2NoaXAgYW5hbG9n aXggRFAgZHJpdmVyIHZzLiB0aGUgYW5hbG9naXgKPiBicmlkZ2UgRFAgY29kZToKPiAKPiBbUEFU Q0ggdjYgMDMvMTBdIGRybS9icmlkZ2U6IGFuYWxvZ2l4OiBEbyBub3QgdXNlIGRldmljZSdzIGRy dmRhdGEKPiBodHRwczovL3BhdGNod29yay5rZXJuZWwub3JnL3BhdGNoLzEwMDE1ODc1LwoKLS0g ClJlZ2FyZHMsCgpMYXVyZW50IFBpbmNoYXJ0CgpfX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fXwpkcmktZGV2ZWwgbWFpbGluZyBsaXN0CmRyaS1kZXZlbEBsaXN0 cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9s aXN0aW5mby9kcmktZGV2ZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757504AbeAINgY (ORCPT + 1 other); Tue, 9 Jan 2018 08:36:24 -0500 Received: from galahad.ideasonboard.com ([185.26.127.97]:56481 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753913AbeAINgX (ORCPT ); Tue, 9 Jan 2018 08:36:23 -0500 From: Laurent Pinchart To: dri-devel@lists.freedesktop.org Cc: Brian Norris , Doug Anderson , hl@rock-chips.com, linux-rockchip@lists.infradead.org, David Airlie , Jeffy Chen , Philippe Cornu , linux-kernel@vger.kernel.org, Yannick Fertre , Nickey Yang , mka@chromium.org, Vincent Abriou Subject: Re: [PATCH] drm/bridge/synopsis: stop clobbering drvdata Date: Tue, 09 Jan 2018 15:36:53 +0200 Message-ID: <15815822.Bd0fLxLFzJ@avalon> Organization: Ideas on Board Oy In-Reply-To: <20171128182121.GA116479@google.com> References: <20171128010538.119114-1-briannorris@chromium.org> <1743921.AofnJoQ8Rz@avalon> <20171128182121.GA116479@google.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: Hi Brian, On Tuesday, 28 November 2017 20:21:23 EET Brian Norris wrote: > On Tue, Nov 28, 2017 at 02:51:46PM +0200, Laurent Pinchart wrote: > > Hi Brian, > > > > Thank you for the patch. > > > > I'd mention dw-mipi-dsi in the subject line as the directory contains the > > dw-hdmi driver as well that this patch doesn't touch. > > Yep. Does it need another tag in the subject? e.g., '.../dw-mipi-dsi:'? > > > On Tuesday, 28 November 2017 03:05:38 EET Brian Norris wrote: > >> Bridge drivers/helpers shouldn't be clobbering the drvdata, since a > >> parent driver might need to own this. > > > > By parent driver I assume you mean a glue driver that binds to the SoC- > > specific compatible string for the DSI transmitter. > > Indeed. Nickey picked this up for his Rockchip driver submission, but > maybe we should reword the commit message a bit. How about "drm: dw-mipi-dsi: Stop clobbering drvdata" ? > >> Instead, let's return our > >> 'dw_mipi_dsi' object and have callers pass that back to us for removal. > >> > >> Signed-off-by: Brian Norris > > > > Wouldn't it be cleaner to embed the dw_mipi_dsi structure in the parent- > > specific data structure (struct dw_mipi_dsi_stm and struct > > dw_mipi_dsi_rockchip when the "[PATCH v3 0/5] Update ROCKCHIP DSI driver > > that uses dw-mipi-dsi bridge" patch series will land) instead of > > allocating it dynamically ? We would then have a single object to track. > > I suppose we could do that too. But that would require exposing the > whole layout of 'struct dw_mipi_dsi' to users. Do we want to sacrifice > the enforced separation for a little bit of nicer object handling? I certainly don't think we should go for spaghetti code with all objects accessing each other :) On the other hand, we're talking about C code, and we thus have no way to enforce access restrictions in the compiler, so it's a lost battle anyway. I don't see an issue with exposing the object in the sense of moving its definition to a header file if it results in cleaner code. I think we need to trust developers not to abuse internal APIs, and if they do, catch it during review. > Also, this was modeled a bit after the similar rework needed to untangle > the drvdata handling in the Rockchip analogix DP driver vs. the analogix > bridge DP code: > > [PATCH v6 03/10] drm/bridge: analogix: Do not use device's drvdata > https://patchwork.kernel.org/patch/10015875/ -- Regards, Laurent Pinchart