From mboxrd@z Thu Jan 1 00:00:00 1970 From: linux@armlinux.org.uk (Russell King - ARM Linux) Date: Mon, 27 Aug 2018 18:59:47 +0100 Subject: [PATCH v2 4/7] drm/i2c: tda998x: convert to bridge driver In-Reply-To: <20180827161601eucas1p2951c1ce0eeeabbcf9d190d7dca28c91c~OyfFOhdDv1363313633eucas1p2b@eucas1p2.samsung.com> References: <20180730164137.GD17271@n2100.armlinux.org.uk> <20180827161601eucas1p2951c1ce0eeeabbcf9d190d7dca28c91c~OyfFOhdDv1363313633eucas1p2b@eucas1p2.samsung.com> Message-ID: <20180827175947.GE30658@n2100.armlinux.org.uk> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Andrzej, On Mon, Aug 27, 2018 at 06:15:59PM +0200, Andrzej Hajda wrote: > On 30.07.2018 18:42, Russell King wrote: > > static void tda998x_destroy(struct tda998x_priv *priv) > > { > > + drm_bridge_remove(&priv->bridge); > > + > > /* disable all IRQs and free the IRQ handler */ > > cec_write(priv, REG_CEC_RXSHPDINTENA, 0); > > reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD); > > @@ -1650,6 +1663,7 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv) > > mutex_init(&priv->mutex); /* protect the page access */ > > mutex_init(&priv->audio_mutex); /* protect access from audio thread */ > > mutex_init(&priv->edid_mutex); > > + INIT_LIST_HEAD(&priv->bridge.list); > > This line can be probably removed, unless there is a reason I am not > aware of. The addition above of drm_bridge_remove() to tda998x_destroy() means that we end up calling this function in the error cleanup path. This avoids unnecessary complexity with lots of different gotos - tda998x has had a long history of not cleaning up stuff properly. devm interfaces for bridge do not help avoid that - devm stuff only works if everything that is registered previously is cleaned up via devm mechanisms to ensure that a device's interface becomes unavailable before stuff (eg, edid timers, detect work) is started to be cleaned up. Otherwise, there's a chance of this stuff being triggered during tear-down. > > +static int tda998x_bind(struct device *dev, struct device *master, void *data) > > +{ > > + struct i2c_client *client = to_i2c_client(dev); > > + struct drm_device *drm = data; > > + struct tda998x_priv *priv; > > + int ret; > > + > > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > > + if (!priv) > > + return -ENOMEM; > > + > > + dev_set_drvdata(dev, priv); > > + > > + ret = tda998x_create(client, priv); > > + if (ret) > > + return ret; > > + > > + ret = tda998x_encoder_init(dev, drm); > > + if (ret) { > > + tda998x_destroy(priv); > > + return ret; > > + } > > + return 0; > > It could be replaced by: > ??? ret = tda998x_encoder_init(dev, drm); > ??? if (ret) > ??? ??? tda998x_destroy(priv); > ??? return ret; > > but this is probably matter of taste. It's not clear to me what "It" is - I think you're suggesting combining tda998x_create() and tda998x_encoder_init() ? The code is structured this way to make the following patches easier - there is no point of combining things only to have to then break them apart again in a later patch. Please see patch 7, where tda998x_create() moves out of this function, where exactly this happens. > Moreover I guess priv->is_on could be removed if enable/disable > callbacks are called only by drm_core, but this is for another patch. Is it guaranteed that a bridge ->enable or ->disable callback won't be called twice, even for legacy drivers? I think atomic guarantees this but I don't think it's guaranteed for legacy drivers. I'm guessing Rob had a reason why he added the check when he originally created the driver (encoder ->dpms can be called for the same dpms state multiple times?) -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up According to speedtest.net: 13Mbps down 490kbps up From mboxrd@z Thu Jan 1 00:00:00 1970 From: Russell King - ARM Linux Subject: Re: [PATCH v2 4/7] drm/i2c: tda998x: convert to bridge driver Date: Mon, 27 Aug 2018 18:59:47 +0100 Message-ID: <20180827175947.GE30658@n2100.armlinux.org.uk> References: <20180730164137.GD17271@n2100.armlinux.org.uk> <20180827161601eucas1p2951c1ce0eeeabbcf9d190d7dca28c91c~OyfFOhdDv1363313633eucas1p2b@eucas1p2.samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from pandora.armlinux.org.uk (pandora.armlinux.org.uk [IPv6:2001:4d48:ad52:3201:214:fdff:fe10:1be6]) by gabe.freedesktop.org (Postfix) with ESMTPS id F3CFB6E27E for ; Mon, 27 Aug 2018 18:00:03 +0000 (UTC) Content-Disposition: inline In-Reply-To: <20180827161601eucas1p2951c1ce0eeeabbcf9d190d7dca28c91c~OyfFOhdDv1363313633eucas1p2b@eucas1p2.samsung.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Andrzej Hajda Cc: David Airlie , Liviu Dudau , dri-devel@lists.freedesktop.org, Tomi Valkeinen , Jyri Sarha , Peter Rosin , linux-arm-kernel@lists.infradead.org List-Id: dri-devel@lists.freedesktop.org SGkgQW5kcnplaiwKCk9uIE1vbiwgQXVnIDI3LCAyMDE4IGF0IDA2OjE1OjU5UE0gKzAyMDAsIEFu ZHJ6ZWogSGFqZGEgd3JvdGU6Cj4gT24gMzAuMDcuMjAxOCAxODo0MiwgUnVzc2VsbCBLaW5nIHdy b3RlOgo+ID4gIHN0YXRpYyB2b2lkIHRkYTk5OHhfZGVzdHJveShzdHJ1Y3QgdGRhOTk4eF9wcml2 ICpwcml2KQo+ID4gIHsKPiA+ICsJZHJtX2JyaWRnZV9yZW1vdmUoJnByaXYtPmJyaWRnZSk7Cj4g PiArCj4gPiAgCS8qIGRpc2FibGUgYWxsIElSUXMgYW5kIGZyZWUgdGhlIElSUSBoYW5kbGVyICov Cj4gPiAgCWNlY193cml0ZShwcml2LCBSRUdfQ0VDX1JYU0hQRElOVEVOQSwgMCk7Cj4gPiAgCXJl Z19jbGVhcihwcml2LCBSRUdfSU5UX0ZMQUdTXzIsIElOVF9GTEFHU18yX0VESURfQkxLX1JEKTsK PiA+IEBAIC0xNjUwLDYgKzE2NjMsNyBAQCBzdGF0aWMgaW50IHRkYTk5OHhfY3JlYXRlKHN0cnVj dCBpMmNfY2xpZW50ICpjbGllbnQsIHN0cnVjdCB0ZGE5OTh4X3ByaXYgKnByaXYpCj4gPiAgCW11 dGV4X2luaXQoJnByaXYtPm11dGV4KTsJLyogcHJvdGVjdCB0aGUgcGFnZSBhY2Nlc3MgKi8KPiA+ ICAJbXV0ZXhfaW5pdCgmcHJpdi0+YXVkaW9fbXV0ZXgpOyAvKiBwcm90ZWN0IGFjY2VzcyBmcm9t IGF1ZGlvIHRocmVhZCAqLwo+ID4gIAltdXRleF9pbml0KCZwcml2LT5lZGlkX211dGV4KTsKPiA+ ICsJSU5JVF9MSVNUX0hFQUQoJnByaXYtPmJyaWRnZS5saXN0KTsKPiAKPiBUaGlzIGxpbmUgY2Fu IGJlIHByb2JhYmx5IHJlbW92ZWQsIHVubGVzcyB0aGVyZSBpcyBhIHJlYXNvbiBJIGFtIG5vdAo+ IGF3YXJlIG9mLgoKVGhlIGFkZGl0aW9uIGFib3ZlIG9mIGRybV9icmlkZ2VfcmVtb3ZlKCkgdG8g dGRhOTk4eF9kZXN0cm95KCkgbWVhbnMKdGhhdCB3ZSBlbmQgdXAgY2FsbGluZyB0aGlzIGZ1bmN0 aW9uIGluIHRoZSBlcnJvciBjbGVhbnVwIHBhdGguICBUaGlzCmF2b2lkcyB1bm5lY2Vzc2FyeSBj b21wbGV4aXR5IHdpdGggbG90cyBvZiBkaWZmZXJlbnQgZ290b3MgLSB0ZGE5OTh4CmhhcyBoYWQg YSBsb25nIGhpc3Rvcnkgb2Ygbm90IGNsZWFuaW5nIHVwIHN0dWZmIHByb3Blcmx5LgoKZGV2bSBp bnRlcmZhY2VzIGZvciBicmlkZ2UgZG8gbm90IGhlbHAgYXZvaWQgdGhhdCAtIGRldm0gc3R1ZmYg b25seQp3b3JrcyBpZiBldmVyeXRoaW5nIHRoYXQgaXMgcmVnaXN0ZXJlZCBwcmV2aW91c2x5IGlz IGNsZWFuZWQgdXAgdmlhCmRldm0gbWVjaGFuaXNtcyB0byBlbnN1cmUgdGhhdCBhIGRldmljZSdz IGludGVyZmFjZSBiZWNvbWVzIHVuYXZhaWxhYmxlCmJlZm9yZSBzdHVmZiAoZWcsIGVkaWQgdGlt ZXJzLCBkZXRlY3Qgd29yaykgaXMgc3RhcnRlZCB0byBiZSBjbGVhbmVkIHVwLgpPdGhlcndpc2Us IHRoZXJlJ3MgYSBjaGFuY2Ugb2YgdGhpcyBzdHVmZiBiZWluZyB0cmlnZ2VyZWQgZHVyaW5nCnRl YXItZG93bi4KCj4gPiArc3RhdGljIGludCB0ZGE5OTh4X2JpbmQoc3RydWN0IGRldmljZSAqZGV2 LCBzdHJ1Y3QgZGV2aWNlICptYXN0ZXIsIHZvaWQgKmRhdGEpCj4gPiArewo+ID4gKwlzdHJ1Y3Qg aTJjX2NsaWVudCAqY2xpZW50ID0gdG9faTJjX2NsaWVudChkZXYpOwo+ID4gKwlzdHJ1Y3QgZHJt X2RldmljZSAqZHJtID0gZGF0YTsKPiA+ICsJc3RydWN0IHRkYTk5OHhfcHJpdiAqcHJpdjsKPiA+ ICsJaW50IHJldDsKPiA+ICsKPiA+ICsJcHJpdiA9IGRldm1fa3phbGxvYyhkZXYsIHNpemVvZigq cHJpdiksIEdGUF9LRVJORUwpOwo+ID4gKwlpZiAoIXByaXYpCj4gPiArCQlyZXR1cm4gLUVOT01F TTsKPiA+ICsKPiA+ICsJZGV2X3NldF9kcnZkYXRhKGRldiwgcHJpdik7Cj4gPiArCj4gPiArCXJl dCA9IHRkYTk5OHhfY3JlYXRlKGNsaWVudCwgcHJpdik7Cj4gPiArCWlmIChyZXQpCj4gPiArCQly ZXR1cm4gcmV0Owo+ID4gKwo+ID4gKwlyZXQgPSB0ZGE5OTh4X2VuY29kZXJfaW5pdChkZXYsIGRy bSk7Cj4gPiArCWlmIChyZXQpIHsKPiA+ICsJCXRkYTk5OHhfZGVzdHJveShwcml2KTsKPiA+ICsJ CXJldHVybiByZXQ7Cj4gPiArCX0KPiA+ICsJcmV0dXJuIDA7Cj4gCj4gSXQgY291bGQgYmUgcmVw bGFjZWQgYnk6Cj4gwqDCoMKgIHJldCA9IHRkYTk5OHhfZW5jb2Rlcl9pbml0KGRldiwgZHJtKTsK PiDCoMKgwqAgaWYgKHJldCkKPiDCoMKgwqAgwqDCoMKgIHRkYTk5OHhfZGVzdHJveShwcml2KTsK PiDCoMKgwqAgcmV0dXJuIHJldDsKPiAKPiBidXQgdGhpcyBpcyBwcm9iYWJseSBtYXR0ZXIgb2Yg dGFzdGUuCgpJdCdzIG5vdCBjbGVhciB0byBtZSB3aGF0ICJJdCIgaXMgLSBJIHRoaW5rIHlvdSdy ZSBzdWdnZXN0aW5nIGNvbWJpbmluZwp0ZGE5OTh4X2NyZWF0ZSgpIGFuZCB0ZGE5OTh4X2VuY29k ZXJfaW5pdCgpID8KClRoZSBjb2RlIGlzIHN0cnVjdHVyZWQgdGhpcyB3YXkgdG8gbWFrZSB0aGUg Zm9sbG93aW5nIHBhdGNoZXMgZWFzaWVyIC0KdGhlcmUgaXMgbm8gcG9pbnQgb2YgY29tYmluaW5n IHRoaW5ncyBvbmx5IHRvIGhhdmUgdG8gdGhlbiBicmVhayB0aGVtCmFwYXJ0IGFnYWluIGluIGEg bGF0ZXIgcGF0Y2guICBQbGVhc2Ugc2VlIHBhdGNoIDcsIHdoZXJlIHRkYTk5OHhfY3JlYXRlKCkK bW92ZXMgb3V0IG9mIHRoaXMgZnVuY3Rpb24sIHdoZXJlIGV4YWN0bHkgdGhpcyBoYXBwZW5zLgoK PiBNb3Jlb3ZlciBJIGd1ZXNzIHByaXYtPmlzX29uIGNvdWxkIGJlIHJlbW92ZWQgaWYgZW5hYmxl L2Rpc2FibGUKPiBjYWxsYmFja3MgYXJlIGNhbGxlZCBvbmx5IGJ5IGRybV9jb3JlLCBidXQgdGhp cyBpcyBmb3IgYW5vdGhlciBwYXRjaC4KCklzIGl0IGd1YXJhbnRlZWQgdGhhdCBhIGJyaWRnZSAt PmVuYWJsZSBvciAtPmRpc2FibGUgY2FsbGJhY2sgd29uJ3QgYmUKY2FsbGVkIHR3aWNlLCBldmVu IGZvciBsZWdhY3kgZHJpdmVycz8gIEkgdGhpbmsgYXRvbWljIGd1YXJhbnRlZXMgdGhpcwpidXQg SSBkb24ndCB0aGluayBpdCdzIGd1YXJhbnRlZWQgZm9yIGxlZ2FjeSBkcml2ZXJzLgoKSSdtIGd1 ZXNzaW5nIFJvYiBoYWQgYSByZWFzb24gd2h5IGhlIGFkZGVkIHRoZSBjaGVjayB3aGVuIGhlIG9y aWdpbmFsbHkKY3JlYXRlZCB0aGUgZHJpdmVyIChlbmNvZGVyIC0+ZHBtcyBjYW4gYmUgY2FsbGVk IGZvciB0aGUgc2FtZSBkcG1zCnN0YXRlIG11bHRpcGxlIHRpbWVzPykKCi0tIApSTUsncyBQYXRj aCBzeXN0ZW06IGh0dHA6Ly93d3cuYXJtbGludXgub3JnLnVrL2RldmVsb3Blci9wYXRjaGVzLwpG VFRDIGJyb2FkYmFuZCBmb3IgMC44bWlsZSBsaW5lIGluIHN1YnVyYmlhOiBzeW5jIGF0IDEzLjhN YnBzIGRvd24gNjMwa2JwcyB1cApBY2NvcmRpbmcgdG8gc3BlZWR0ZXN0Lm5ldDogMTNNYnBzIGRv d24gNDkwa2JwcyB1cApfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fXwpkcmktZGV2ZWwgbWFpbGluZyBsaXN0CmRyaS1kZXZlbEBsaXN0cy5mcmVlZGVza3RvcC5v cmcKaHR0cHM6Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2 ZWwK