From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chanwoo Choi Subject: Re: [PATCH v5 6/6] drm/bridge/sii8620: use micro-USB cable detection logic to detect MHL Date: Fri, 02 Mar 2018 09:29:43 +0900 Message-ID: <5A989AF7.50704@samsung.com> References: <20180227071134.28063-1-a.hajda@samsung.com> <20180227071134.28063-7-a.hajda@samsung.com> <5A953C21.5020007@samsung.com> <87dd3281-0471-ab2a-90c6-3f2d4bdf4750@samsung.com> <5A95DB2B.6000003@samsung.com> <54edf1d7-056f-ad75-4d94-e1efa2ffdc1c@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8BIT Return-path: In-reply-to: <54edf1d7-056f-ad75-4d94-e1efa2ffdc1c@samsung.com> Sender: linux-kernel-owner@vger.kernel.org To: Andrzej Hajda , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" Cc: Maciej Purski , Bartlomiej Zolnierkiewicz , Marek Szyprowski , dri-devel@lists.freedesktop.org, Inki Dae , Rob Herring , Mark Rutland , Krzysztof Kozlowski , Archit Taneja , Laurent Pinchart , linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-samsung-soc@vger.kernel.org, linux-usb@vger.kernel.org List-Id: linux-samsung-soc@vger.kernel.org On 2018년 02월 28일 22:44, Andrzej Hajda wrote: > On 27.02.2018 23:26, Chanwoo Choi wrote: >> Hi, >> >> On 2018년 02월 27일 21:05, Andrzej Hajda wrote: >>> On 27.02.2018 12:08, Chanwoo Choi wrote: >>>> Hi, >>>> >>>> On 2018년 02월 27일 16:11, Andrzej Hajda wrote: >>>>> From: Maciej Purski >>>>> >>>>> Currently MHL chip must be turned on permanently to detect MHL cable. It >>>>> duplicates micro-USB controller's (MUIC) functionality and consumes >>>>> unnecessary power. Lets use extcon attached to MUIC to enable MHL chip >>>>> only if it detects MHL cable. >>>>> >>>>> Signed-off-by: Maciej Purski >>>>> Signed-off-by: Andrzej Hajda >>>>> --- >>>>> v5: updated extcon API >>>>> >>>>> This is rework of the patch by Maciej with following changes: >>>>> - use micro-USB port bindings to get extcon, instead of extcon property, >>>>> - fixed remove sequence, >>>>> - fixed extcon get state logic. >>>>> >>>>> Code finding extcon node is hacky IMO, I guess ultimately it should be done >>>>> via some framework (maybe even extcon), or at least via helper, I hope it >>>>> can stay as is until the proper solution will be merged. >>>>> >>>>> Signed-off-by: Andrzej Hajda >>>>> --- >>>>> drivers/gpu/drm/bridge/sil-sii8620.c | 97 ++++++++++++++++++++++++++++++++++-- >>>>> 1 file changed, 94 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c >>>>> index 9e785b8e0ea2..62b0adabcac2 100644 >>>>> --- a/drivers/gpu/drm/bridge/sil-sii8620.c >>>>> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c >>>>> @@ -17,6 +17,7 @@ >>>>> >>>>> #include >>>>> #include >>>>> +#include >>>>> #include >>>>> #include >>>>> #include >>>>> @@ -25,6 +26,7 @@ >>>>> #include >>>>> #include >>>>> #include >>>>> +#include >>>>> #include >>>>> #include >>>>> >>>>> @@ -81,6 +83,10 @@ struct sii8620 { >>>>> struct edid *edid; >>>>> unsigned int gen2_write_burst:1; >>>>> enum sii8620_mt_state mt_state; >>>>> + struct extcon_dev *extcon; >>>>> + struct notifier_block extcon_nb; >>>>> + struct work_struct extcon_wq; >>>>> + int cable_state; >>>>> struct list_head mt_queue; >>>>> struct { >>>>> int r_size; >>>>> @@ -2175,6 +2181,77 @@ static void sii8620_init_rcp_input_dev(struct sii8620 *ctx) >>>>> ctx->rc_dev = rc_dev; >>>>> } >>>>> >>>>> +static void sii8620_cable_out(struct sii8620 *ctx) >>>>> +{ >>>>> + disable_irq(to_i2c_client(ctx->dev)->irq); >>>>> + sii8620_hw_off(ctx); >>>>> +} >>>>> + >>>>> +static void sii8620_extcon_work(struct work_struct *work) >>>>> +{ >>>>> + struct sii8620 *ctx = >>>>> + container_of(work, struct sii8620, extcon_wq); >>>>> + int state = extcon_get_state(ctx->extcon, EXTCON_DISP_MHL); >>>>> + >>>>> + if (state == ctx->cable_state) >>>>> + return; >>>>> + >>>>> + ctx->cable_state = state; >>>>> + >>>>> + if (state > 0) >>>>> + sii8620_cable_in(ctx); >>>>> + else >>>>> + sii8620_cable_out(ctx); >>>>> +} >>>>> + >>>>> +static int sii8620_extcon_notifier(struct notifier_block *self, >>>>> + unsigned long event, void *ptr) >>>>> +{ >>>>> + struct sii8620 *ctx = >>>>> + container_of(self, struct sii8620, extcon_nb); >>>>> + >>>>> + schedule_work(&ctx->extcon_wq); >>>>> + >>>>> + return NOTIFY_DONE; >>>>> +} >>>>> + >>>>> +static int sii8620_extcon_init(struct sii8620 *ctx) >>>>> +{ >>>>> + struct extcon_dev *edev; >>>>> + struct device_node *musb, *muic; >>>>> + int ret; >>>>> + >>>>> + /* get micro-USB connector node */ >>>>> + musb = of_graph_get_remote_node(ctx->dev->of_node, 1, -1); >>>>> + /* next get micro-USB Interface Controller node */ >>>>> + muic = of_get_next_parent(musb); >>>>> + >>>>> + if (!muic) { >>>>> + dev_info(ctx->dev, "no extcon found, switching to 'always on' mode\n"); >>>>> + return 0; >>>>> + } >>>>> + >>>>> + edev = extcon_find_edev_by_node(muic); >>>>> + of_node_put(muic); >>>>> + if (IS_ERR(edev)) { >>>>> + if (PTR_ERR(edev) == -EPROBE_DEFER) >>>>> + return -EPROBE_DEFER; >>>>> + dev_err(ctx->dev, "Invalid or missing extcon\n"); >>>>> + return PTR_ERR(edev); >>>>> + } >>>>> + >>>>> + ctx->extcon = edev; >>>>> + ctx->extcon_nb.notifier_call = sii8620_extcon_notifier; >>>>> + INIT_WORK(&ctx->extcon_wq, sii8620_extcon_work); >>>>> + ret = extcon_register_notifier(edev, EXTCON_DISP_MHL, &ctx->extcon_nb); >>>> You better to use devm_extcon_register_notifier(). >>> With devm version I risk that in case of device unbind notification will >>> be called after .remove callback, it seems to me quite dangerous. Or am >>> I missing something? >> If you use the cancel_work_sync() in remove() instead of flush_work(), >> you can use the 'devm_extcon_*'. > > cancel_work_sync() does not prevent works scheduled later from execution > [1] and this scenario is possible with devm_extcon_register_notifier() > and cancel_work_sync(). > So we end up with: > sii8620_remove() calls cancel_work_sync() > ... > notifier(called asynchronously) schedules sii8620_extcon_work() > ... > notifier is removed by devm framework > sii8620 context is destroyed by devm framework > ... > sii8620_extcon_work is executed on destroyed context !!! BUG !!! > > For me it seems that devm_extcon_register_notifier is not safe in this case. > > [1]: Since documentation was not clear I have performed live test > confirming my suspicions. You're right. Sorry for confusion. Reviewed-by: Chanwoo Choi [snip] -- Best Regards, Chanwoo Choi Samsung Electronics From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Subject: [v5,6/6] drm/bridge/sii8620: use micro-USB cable detection logic to detect MHL From: Chanwoo Choi Message-Id: <5A989AF7.50704@samsung.com> Date: Fri, 02 Mar 2018 09:29:43 +0900 To: Andrzej Hajda , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" Cc: Maciej Purski , Bartlomiej Zolnierkiewicz , Marek Szyprowski , dri-devel@lists.freedesktop.org, Inki Dae , Rob Herring , Mark Rutland , Krzysztof Kozlowski , Archit Taneja , Laurent Pinchart , linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-samsung-soc@vger.kernel.org, linux-usb@vger.kernel.org List-ID: T24gMjAxOOuFhCAwMuyblCAyOOydvCAyMjo0NCwgQW5kcnplaiBIYWpkYSB3cm90ZToKPiBPbiAy Ny4wMi4yMDE4IDIzOjI2LCBDaGFud29vIENob2kgd3JvdGU6Cj4+IEhpLAo+Pgo+PiBPbiAyMDE4 64WEIDAy7JuUIDI37J28IDIxOjA1LCBBbmRyemVqIEhhamRhIHdyb3RlOgo+Pj4gT24gMjcuMDIu MjAxOCAxMjowOCwgQ2hhbndvbyBDaG9pIHdyb3RlOgo+Pj4+IEhpLAo+Pj4+Cj4+Pj4gT24gMjAx OOuFhCAwMuyblCAyN+ydvCAxNjoxMSwgQW5kcnplaiBIYWpkYSB3cm90ZToKPj4+Pj4gRnJvbTog TWFjaWVqIFB1cnNraSA8bS5wdXJza2lAc2Ftc3VuZy5jb20+Cj4+Pj4+Cj4+Pj4+IEN1cnJlbnRs eSBNSEwgY2hpcCBtdXN0IGJlIHR1cm5lZCBvbiBwZXJtYW5lbnRseSB0byBkZXRlY3QgTUhMIGNh YmxlLiBJdAo+Pj4+PiBkdXBsaWNhdGVzIG1pY3JvLVVTQiBjb250cm9sbGVyJ3MgKE1VSUMpIGZ1 bmN0aW9uYWxpdHkgYW5kIGNvbnN1bWVzCj4+Pj4+IHVubmVjZXNzYXJ5IHBvd2VyLiBMZXRzIHVz ZSBleHRjb24gYXR0YWNoZWQgdG8gTVVJQyB0byBlbmFibGUgTUhMIGNoaXAKPj4+Pj4gb25seSBp ZiBpdCBkZXRlY3RzIE1ITCBjYWJsZS4KPj4+Pj4KPj4+Pj4gU2lnbmVkLW9mZi1ieTogTWFjaWVq IFB1cnNraSA8bS5wdXJza2lAc2Ftc3VuZy5jb20+Cj4+Pj4+IFNpZ25lZC1vZmYtYnk6IEFuZHJ6 ZWogSGFqZGEgPGEuaGFqZGFAc2Ftc3VuZy5jb20+Cj4+Pj4+IC0tLQo+Pj4+PiB2NTogdXBkYXRl ZCBleHRjb24gQVBJCj4+Pj4+Cj4+Pj4+IFRoaXMgaXMgcmV3b3JrIG9mIHRoZSBwYXRjaCBieSBN YWNpZWogd2l0aCBmb2xsb3dpbmcgY2hhbmdlczoKPj4+Pj4gLSB1c2UgbWljcm8tVVNCIHBvcnQg YmluZGluZ3MgdG8gZ2V0IGV4dGNvbiwgaW5zdGVhZCBvZiBleHRjb24gcHJvcGVydHksCj4+Pj4+ IC0gZml4ZWQgcmVtb3ZlIHNlcXVlbmNlLAo+Pj4+PiAtIGZpeGVkIGV4dGNvbiBnZXQgc3RhdGUg bG9naWMuCj4+Pj4+Cj4+Pj4+IENvZGUgZmluZGluZyBleHRjb24gbm9kZSBpcyBoYWNreSBJTU8s IEkgZ3Vlc3MgdWx0aW1hdGVseSBpdCBzaG91bGQgYmUgZG9uZQo+Pj4+PiB2aWEgc29tZSBmcmFt ZXdvcmsgKG1heWJlIGV2ZW4gZXh0Y29uKSwgb3IgYXQgbGVhc3QgdmlhIGhlbHBlciwgSSBob3Bl IGl0Cj4+Pj4+IGNhbiBzdGF5IGFzIGlzIHVudGlsIHRoZSBwcm9wZXIgc29sdXRpb24gd2lsbCBi ZSBtZXJnZWQuCj4+Pj4+Cj4+Pj4+IFNpZ25lZC1vZmYtYnk6IEFuZHJ6ZWogSGFqZGEgPGEuaGFq ZGFAc2Ftc3VuZy5jb20+Cj4+Pj4+IC0tLQo+Pj4+PiAgZHJpdmVycy9ncHUvZHJtL2JyaWRnZS9z aWwtc2lpODYyMC5jIHwgOTcgKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKy0tCj4+ Pj4+ICAxIGZpbGUgY2hhbmdlZCwgOTQgaW5zZXJ0aW9ucygrKSwgMyBkZWxldGlvbnMoLSkKPj4+ Pj4KPj4+Pj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvZ3B1L2RybS9icmlkZ2Uvc2lsLXNpaTg2MjAu YyBiL2RyaXZlcnMvZ3B1L2RybS9icmlkZ2Uvc2lsLXNpaTg2MjAuYwo+Pj4+PiBpbmRleCA5ZTc4 NWI4ZTBlYTIuLjYyYjBhZGFiY2FjMiAxMDA2NDQKPj4+Pj4gLS0tIGEvZHJpdmVycy9ncHUvZHJt L2JyaWRnZS9zaWwtc2lpODYyMC5jCj4+Pj4+ICsrKyBiL2RyaXZlcnMvZ3B1L2RybS9icmlkZ2Uv c2lsLXNpaTg2MjAuYwo+Pj4+PiBAQCAtMTcsNiArMTcsNyBAQAo+Pj4+PiAgCj4+Pj4+ICAjaW5j bHVkZSA8bGludXgvY2xrLmg+Cj4+Pj4+ICAjaW5jbHVkZSA8bGludXgvZGVsYXkuaD4KPj4+Pj4g KyNpbmNsdWRlIDxsaW51eC9leHRjb24uaD4KPj4+Pj4gICNpbmNsdWRlIDxsaW51eC9ncGlvL2Nv bnN1bWVyLmg+Cj4+Pj4+ICAjaW5jbHVkZSA8bGludXgvaTJjLmg+Cj4+Pj4+ICAjaW5jbHVkZSA8 bGludXgvaW50ZXJydXB0Lmg+Cj4+Pj4+IEBAIC0yNSw2ICsyNiw3IEBACj4+Pj4+ICAjaW5jbHVk ZSA8bGludXgvbGlzdC5oPgo+Pj4+PiAgI2luY2x1ZGUgPGxpbnV4L21vZHVsZS5oPgo+Pj4+PiAg I2luY2x1ZGUgPGxpbnV4L211dGV4Lmg+Cj4+Pj4+ICsjaW5jbHVkZSA8bGludXgvb2ZfZ3JhcGgu aD4KPj4+Pj4gICNpbmNsdWRlIDxsaW51eC9yZWd1bGF0b3IvY29uc3VtZXIuaD4KPj4+Pj4gICNp bmNsdWRlIDxsaW51eC9zbGFiLmg+Cj4+Pj4+ICAKPj4+Pj4gQEAgLTgxLDYgKzgzLDEwIEBAIHN0 cnVjdCBzaWk4NjIwIHsKPj4+Pj4gIAlzdHJ1Y3QgZWRpZCAqZWRpZDsKPj4+Pj4gIAl1bnNpZ25l ZCBpbnQgZ2VuMl93cml0ZV9idXJzdDoxOwo+Pj4+PiAgCWVudW0gc2lpODYyMF9tdF9zdGF0ZSBt dF9zdGF0ZTsKPj4+Pj4gKwlzdHJ1Y3QgZXh0Y29uX2RldiAqZXh0Y29uOwo+Pj4+PiArCXN0cnVj dCBub3RpZmllcl9ibG9jayBleHRjb25fbmI7Cj4+Pj4+ICsJc3RydWN0IHdvcmtfc3RydWN0IGV4 dGNvbl93cTsKPj4+Pj4gKwlpbnQgY2FibGVfc3RhdGU7Cj4+Pj4+ICAJc3RydWN0IGxpc3RfaGVh ZCBtdF9xdWV1ZTsKPj4+Pj4gIAlzdHJ1Y3Qgewo+Pj4+PiAgCQlpbnQgcl9zaXplOwo+Pj4+PiBA QCAtMjE3NSw2ICsyMTgxLDc3IEBAIHN0YXRpYyB2b2lkIHNpaTg2MjBfaW5pdF9yY3BfaW5wdXRf ZGV2KHN0cnVjdCBzaWk4NjIwICpjdHgpCj4+Pj4+ICAJY3R4LT5yY19kZXYgPSByY19kZXY7Cj4+ Pj4+ICB9Cj4+Pj4+ICAKPj4+Pj4gK3N0YXRpYyB2b2lkIHNpaTg2MjBfY2FibGVfb3V0KHN0cnVj dCBzaWk4NjIwICpjdHgpCj4+Pj4+ICt7Cj4+Pj4+ICsJZGlzYWJsZV9pcnEodG9faTJjX2NsaWVu dChjdHgtPmRldiktPmlycSk7Cj4+Pj4+ICsJc2lpODYyMF9od19vZmYoY3R4KTsKPj4+Pj4gK30K Pj4+Pj4gKwo+Pj4+PiArc3RhdGljIHZvaWQgc2lpODYyMF9leHRjb25fd29yayhzdHJ1Y3Qgd29y a19zdHJ1Y3QgKndvcmspCj4+Pj4+ICt7Cj4+Pj4+ICsJc3RydWN0IHNpaTg2MjAgKmN0eCA9Cj4+ Pj4+ICsJCWNvbnRhaW5lcl9vZih3b3JrLCBzdHJ1Y3Qgc2lpODYyMCwgZXh0Y29uX3dxKTsKPj4+ Pj4gKwlpbnQgc3RhdGUgPSBleHRjb25fZ2V0X3N0YXRlKGN0eC0+ZXh0Y29uLCBFWFRDT05fRElT UF9NSEwpOwo+Pj4+PiArCj4+Pj4+ICsJaWYgKHN0YXRlID09IGN0eC0+Y2FibGVfc3RhdGUpCj4+ Pj4+ICsJCXJldHVybjsKPj4+Pj4gKwo+Pj4+PiArCWN0eC0+Y2FibGVfc3RhdGUgPSBzdGF0ZTsK Pj4+Pj4gKwo+Pj4+PiArCWlmIChzdGF0ZSA+IDApCj4+Pj4+ICsJCXNpaTg2MjBfY2FibGVfaW4o Y3R4KTsKPj4+Pj4gKwllbHNlCj4+Pj4+ICsJCXNpaTg2MjBfY2FibGVfb3V0KGN0eCk7Cj4+Pj4+ ICt9Cj4+Pj4+ICsKPj4+Pj4gK3N0YXRpYyBpbnQgc2lpODYyMF9leHRjb25fbm90aWZpZXIoc3Ry dWN0IG5vdGlmaWVyX2Jsb2NrICpzZWxmLAo+Pj4+PiArCQkJdW5zaWduZWQgbG9uZyBldmVudCwg dm9pZCAqcHRyKQo+Pj4+PiArewo+Pj4+PiArCXN0cnVjdCBzaWk4NjIwICpjdHggPQo+Pj4+PiAr CQljb250YWluZXJfb2Yoc2VsZiwgc3RydWN0IHNpaTg2MjAsIGV4dGNvbl9uYik7Cj4+Pj4+ICsK Pj4+Pj4gKwlzY2hlZHVsZV93b3JrKCZjdHgtPmV4dGNvbl93cSk7Cj4+Pj4+ICsKPj4+Pj4gKwly ZXR1cm4gTk9USUZZX0RPTkU7Cj4+Pj4+ICt9Cj4+Pj4+ICsKPj4+Pj4gK3N0YXRpYyBpbnQgc2lp ODYyMF9leHRjb25faW5pdChzdHJ1Y3Qgc2lpODYyMCAqY3R4KQo+Pj4+PiArewo+Pj4+PiArCXN0 cnVjdCBleHRjb25fZGV2ICplZGV2Owo+Pj4+PiArCXN0cnVjdCBkZXZpY2Vfbm9kZSAqbXVzYiwg Km11aWM7Cj4+Pj4+ICsJaW50IHJldDsKPj4+Pj4gKwo+Pj4+PiArCS8qIGdldCBtaWNyby1VU0Ig Y29ubmVjdG9yIG5vZGUgKi8KPj4+Pj4gKwltdXNiID0gb2ZfZ3JhcGhfZ2V0X3JlbW90ZV9ub2Rl KGN0eC0+ZGV2LT5vZl9ub2RlLCAxLCAtMSk7Cj4+Pj4+ICsJLyogbmV4dCBnZXQgbWljcm8tVVNC IEludGVyZmFjZSBDb250cm9sbGVyIG5vZGUgKi8KPj4+Pj4gKwltdWljID0gb2ZfZ2V0X25leHRf cGFyZW50KG11c2IpOwo+Pj4+PiArCj4+Pj4+ICsJaWYgKCFtdWljKSB7Cj4+Pj4+ICsJCWRldl9p bmZvKGN0eC0+ZGV2LCAibm8gZXh0Y29uIGZvdW5kLCBzd2l0Y2hpbmcgdG8gJ2Fsd2F5cyBvbicg bW9kZVxuIik7Cj4+Pj4+ICsJCXJldHVybiAwOwo+Pj4+PiArCX0KPj4+Pj4gKwo+Pj4+PiArCWVk ZXYgPSBleHRjb25fZmluZF9lZGV2X2J5X25vZGUobXVpYyk7Cj4+Pj4+ICsJb2Zfbm9kZV9wdXQo bXVpYyk7Cj4+Pj4+ICsJaWYgKElTX0VSUihlZGV2KSkgewo+Pj4+PiArCQlpZiAoUFRSX0VSUihl ZGV2KSA9PSAtRVBST0JFX0RFRkVSKQo+Pj4+PiArCQkJcmV0dXJuIC1FUFJPQkVfREVGRVI7Cj4+ Pj4+ICsJCWRldl9lcnIoY3R4LT5kZXYsICJJbnZhbGlkIG9yIG1pc3NpbmcgZXh0Y29uXG4iKTsK Pj4+Pj4gKwkJcmV0dXJuIFBUUl9FUlIoZWRldik7Cj4+Pj4+ICsJfQo+Pj4+PiArCj4+Pj4+ICsJ Y3R4LT5leHRjb24gPSBlZGV2Owo+Pj4+PiArCWN0eC0+ZXh0Y29uX25iLm5vdGlmaWVyX2NhbGwg PSBzaWk4NjIwX2V4dGNvbl9ub3RpZmllcjsKPj4+Pj4gKwlJTklUX1dPUksoJmN0eC0+ZXh0Y29u X3dxLCBzaWk4NjIwX2V4dGNvbl93b3JrKTsKPj4+Pj4gKwlyZXQgPSBleHRjb25fcmVnaXN0ZXJf bm90aWZpZXIoZWRldiwgRVhUQ09OX0RJU1BfTUhMLCAmY3R4LT5leHRjb25fbmIpOwo+Pj4+IFlv dSBiZXR0ZXIgdG8gdXNlIGRldm1fZXh0Y29uX3JlZ2lzdGVyX25vdGlmaWVyKCkuCj4+PiBXaXRo IGRldm0gdmVyc2lvbiBJIHJpc2sgdGhhdCBpbiBjYXNlIG9mIGRldmljZSB1bmJpbmQgbm90aWZp Y2F0aW9uIHdpbGwKPj4+IGJlIGNhbGxlZCBhZnRlciAucmVtb3ZlIGNhbGxiYWNrLCBpdCBzZWVt cyB0byBtZSBxdWl0ZSBkYW5nZXJvdXMuIE9yIGFtCj4+PiBJIG1pc3Npbmcgc29tZXRoaW5nPwo+ PiBJZiB5b3UgdXNlIHRoZSBjYW5jZWxfd29ya19zeW5jKCkgaW4gcmVtb3ZlKCkgaW5zdGVhZCBv ZiBmbHVzaF93b3JrKCksCj4+IHlvdSBjYW4gdXNlIHRoZSAnZGV2bV9leHRjb25fKicuCj4gCj4g Y2FuY2VsX3dvcmtfc3luYygpIGRvZXMgbm90IHByZXZlbnQgd29ya3Mgc2NoZWR1bGVkIGxhdGVy IGZyb20gZXhlY3V0aW9uCj4gWzFdIGFuZCB0aGlzIHNjZW5hcmlvIGlzIHBvc3NpYmxlIHdpdGgg ZGV2bV9leHRjb25fcmVnaXN0ZXJfbm90aWZpZXIoKQo+IGFuZCBjYW5jZWxfd29ya19zeW5jKCku Cj4gU28gd2UgZW5kIHVwIHdpdGg6Cj4gICAgIHNpaTg2MjBfcmVtb3ZlKCkgY2FsbHMgY2FuY2Vs X3dvcmtfc3luYygpCj4gLi4uCj4gICAgIG5vdGlmaWVyKGNhbGxlZCBhc3luY2hyb25vdXNseSkg c2NoZWR1bGVzIHNpaTg2MjBfZXh0Y29uX3dvcmsoKQo+IC4uLgo+ICAgICBub3RpZmllciBpcyBy ZW1vdmVkIGJ5IGRldm0gZnJhbWV3b3JrCj4gICAgIHNpaTg2MjAgY29udGV4dCBpcyBkZXN0cm95 ZWQgYnkgZGV2bSBmcmFtZXdvcmsKPiAuLi4KPiAgICAgc2lpODYyMF9leHRjb25fd29yayBpcyBl eGVjdXRlZCBvbiBkZXN0cm95ZWQgY29udGV4dCAhISEgQlVHICEhIQo+IAo+IEZvciBtZSBpdCBz ZWVtcyB0aGF0IGRldm1fZXh0Y29uX3JlZ2lzdGVyX25vdGlmaWVyIGlzIG5vdCBzYWZlIGluIHRo aXMgY2FzZS4KPiAKPiBbMV06IFNpbmNlIGRvY3VtZW50YXRpb24gd2FzIG5vdCBjbGVhciBJIGhh dmUgcGVyZm9ybWVkIGxpdmUgdGVzdAo+IGNvbmZpcm1pbmcgbXkgc3VzcGljaW9ucy4KCllvdSdy ZSByaWdodC4gU29ycnkgZm9yIGNvbmZ1c2lvbi4KUmV2aWV3ZWQtYnk6IENoYW53b28gQ2hvaSA8 Y3cwMC5jaG9pQHNhbXN1bmcuY29tPgoKW3NuaXBdCg== From mboxrd@z Thu Jan 1 00:00:00 1970 From: cw00.choi@samsung.com (Chanwoo Choi) Date: Fri, 02 Mar 2018 09:29:43 +0900 Subject: [PATCH v5 6/6] drm/bridge/sii8620: use micro-USB cable detection logic to detect MHL In-Reply-To: <54edf1d7-056f-ad75-4d94-e1efa2ffdc1c@samsung.com> References: <20180227071134.28063-1-a.hajda@samsung.com> <20180227071134.28063-7-a.hajda@samsung.com> <5A953C21.5020007@samsung.com> <87dd3281-0471-ab2a-90c6-3f2d4bdf4750@samsung.com> <5A95DB2B.6000003@samsung.com> <54edf1d7-056f-ad75-4d94-e1efa2ffdc1c@samsung.com> Message-ID: <5A989AF7.50704@samsung.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 2018? 02? 28? 22:44, Andrzej Hajda wrote: > On 27.02.2018 23:26, Chanwoo Choi wrote: >> Hi, >> >> On 2018? 02? 27? 21:05, Andrzej Hajda wrote: >>> On 27.02.2018 12:08, Chanwoo Choi wrote: >>>> Hi, >>>> >>>> On 2018? 02? 27? 16:11, Andrzej Hajda wrote: >>>>> From: Maciej Purski >>>>> >>>>> Currently MHL chip must be turned on permanently to detect MHL cable. It >>>>> duplicates micro-USB controller's (MUIC) functionality and consumes >>>>> unnecessary power. Lets use extcon attached to MUIC to enable MHL chip >>>>> only if it detects MHL cable. >>>>> >>>>> Signed-off-by: Maciej Purski >>>>> Signed-off-by: Andrzej Hajda >>>>> --- >>>>> v5: updated extcon API >>>>> >>>>> This is rework of the patch by Maciej with following changes: >>>>> - use micro-USB port bindings to get extcon, instead of extcon property, >>>>> - fixed remove sequence, >>>>> - fixed extcon get state logic. >>>>> >>>>> Code finding extcon node is hacky IMO, I guess ultimately it should be done >>>>> via some framework (maybe even extcon), or at least via helper, I hope it >>>>> can stay as is until the proper solution will be merged. >>>>> >>>>> Signed-off-by: Andrzej Hajda >>>>> --- >>>>> drivers/gpu/drm/bridge/sil-sii8620.c | 97 ++++++++++++++++++++++++++++++++++-- >>>>> 1 file changed, 94 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c >>>>> index 9e785b8e0ea2..62b0adabcac2 100644 >>>>> --- a/drivers/gpu/drm/bridge/sil-sii8620.c >>>>> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c >>>>> @@ -17,6 +17,7 @@ >>>>> >>>>> #include >>>>> #include >>>>> +#include >>>>> #include >>>>> #include >>>>> #include >>>>> @@ -25,6 +26,7 @@ >>>>> #include >>>>> #include >>>>> #include >>>>> +#include >>>>> #include >>>>> #include >>>>> >>>>> @@ -81,6 +83,10 @@ struct sii8620 { >>>>> struct edid *edid; >>>>> unsigned int gen2_write_burst:1; >>>>> enum sii8620_mt_state mt_state; >>>>> + struct extcon_dev *extcon; >>>>> + struct notifier_block extcon_nb; >>>>> + struct work_struct extcon_wq; >>>>> + int cable_state; >>>>> struct list_head mt_queue; >>>>> struct { >>>>> int r_size; >>>>> @@ -2175,6 +2181,77 @@ static void sii8620_init_rcp_input_dev(struct sii8620 *ctx) >>>>> ctx->rc_dev = rc_dev; >>>>> } >>>>> >>>>> +static void sii8620_cable_out(struct sii8620 *ctx) >>>>> +{ >>>>> + disable_irq(to_i2c_client(ctx->dev)->irq); >>>>> + sii8620_hw_off(ctx); >>>>> +} >>>>> + >>>>> +static void sii8620_extcon_work(struct work_struct *work) >>>>> +{ >>>>> + struct sii8620 *ctx = >>>>> + container_of(work, struct sii8620, extcon_wq); >>>>> + int state = extcon_get_state(ctx->extcon, EXTCON_DISP_MHL); >>>>> + >>>>> + if (state == ctx->cable_state) >>>>> + return; >>>>> + >>>>> + ctx->cable_state = state; >>>>> + >>>>> + if (state > 0) >>>>> + sii8620_cable_in(ctx); >>>>> + else >>>>> + sii8620_cable_out(ctx); >>>>> +} >>>>> + >>>>> +static int sii8620_extcon_notifier(struct notifier_block *self, >>>>> + unsigned long event, void *ptr) >>>>> +{ >>>>> + struct sii8620 *ctx = >>>>> + container_of(self, struct sii8620, extcon_nb); >>>>> + >>>>> + schedule_work(&ctx->extcon_wq); >>>>> + >>>>> + return NOTIFY_DONE; >>>>> +} >>>>> + >>>>> +static int sii8620_extcon_init(struct sii8620 *ctx) >>>>> +{ >>>>> + struct extcon_dev *edev; >>>>> + struct device_node *musb, *muic; >>>>> + int ret; >>>>> + >>>>> + /* get micro-USB connector node */ >>>>> + musb = of_graph_get_remote_node(ctx->dev->of_node, 1, -1); >>>>> + /* next get micro-USB Interface Controller node */ >>>>> + muic = of_get_next_parent(musb); >>>>> + >>>>> + if (!muic) { >>>>> + dev_info(ctx->dev, "no extcon found, switching to 'always on' mode\n"); >>>>> + return 0; >>>>> + } >>>>> + >>>>> + edev = extcon_find_edev_by_node(muic); >>>>> + of_node_put(muic); >>>>> + if (IS_ERR(edev)) { >>>>> + if (PTR_ERR(edev) == -EPROBE_DEFER) >>>>> + return -EPROBE_DEFER; >>>>> + dev_err(ctx->dev, "Invalid or missing extcon\n"); >>>>> + return PTR_ERR(edev); >>>>> + } >>>>> + >>>>> + ctx->extcon = edev; >>>>> + ctx->extcon_nb.notifier_call = sii8620_extcon_notifier; >>>>> + INIT_WORK(&ctx->extcon_wq, sii8620_extcon_work); >>>>> + ret = extcon_register_notifier(edev, EXTCON_DISP_MHL, &ctx->extcon_nb); >>>> You better to use devm_extcon_register_notifier(). >>> With devm version I risk that in case of device unbind notification will >>> be called after .remove callback, it seems to me quite dangerous. Or am >>> I missing something? >> If you use the cancel_work_sync() in remove() instead of flush_work(), >> you can use the 'devm_extcon_*'. > > cancel_work_sync() does not prevent works scheduled later from execution > [1] and this scenario is possible with devm_extcon_register_notifier() > and cancel_work_sync(). > So we end up with: > sii8620_remove() calls cancel_work_sync() > ... > notifier(called asynchronously) schedules sii8620_extcon_work() > ... > notifier is removed by devm framework > sii8620 context is destroyed by devm framework > ... > sii8620_extcon_work is executed on destroyed context !!! BUG !!! > > For me it seems that devm_extcon_register_notifier is not safe in this case. > > [1]: Since documentation was not clear I have performed live test > confirming my suspicions. You're right. Sorry for confusion. Reviewed-by: Chanwoo Choi [snip] -- Best Regards, Chanwoo Choi Samsung Electronics