From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sam Ravnborg Subject: Re: [PATCH v1 7/7] drm: add Atmel LCDC display controller support Date: Sun, 26 Aug 2018 20:41:21 +0200 Message-ID: <20180826184121.GA24867@ravnborg.org> References: <20180812184152.GA22343@ravnborg.org> <20180812184629.3808-7-sam@ravnborg.org> <20180824143125.4e99e791@bbrezillon> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Content-Disposition: inline In-Reply-To: <20180824143125.4e99e791@bbrezillon> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Boris Brezillon Cc: Mark Rutland , devicetree@vger.kernel.org, Alexandre Belloni , linux-pwm@vger.kernel.org, Boris Brezillon , Nicolas Ferre , Nicolas Ferre , dri-devel@lists.freedesktop.org, Rob Herring , Lee Jones , linux-arm-kernel@lists.infradead.org List-Id: linux-pwm@vger.kernel.org SGkgQm9yaXMuCgpWZXJ5IHVzZWZ1bGwgZmVlZGJhY2shCkkgYW0gd29ya2luZyBvbiB2MiwgYW5k IGhhdmUgYWRkcmVzc2VkIHlvdXIgcmV2aWV3IGl0ZW1zLgpKdXN0IGEgZmV3IGNvbW1lbnRzIGJl bG93IHRvIGEgZmV3IGl0ZW1zLgpUaGUgcmVzdCBpcyBwcm9jZXNzZWQvZG9uZS4KCkkgaG9wZSB0 byBwb3N0IHYyIGluIHRoZSB3ZWVrIHRvIGNvbWUuCgo+ID4gK3N0YXRpYyBpbnQgbGNkY19kY19s b2FkKHN0cnVjdCBkcm1fZGV2aWNlICpkcm0pCj4gPiArewo+ID4gKwljb25zdCBzdHJ1Y3Qgb2Zf ZGV2aWNlX2lkICptYXRjaDsKPiA+ICsJc3RydWN0IHBsYXRmb3JtX2RldmljZSAqcGRldjsKPiA+ ICsJc3RydWN0IGxjZGNfZGMgKmxjZGNfZGM7Cj4gPiArCXN0cnVjdCBkZXZpY2UgKmRldjsKPiA+ ICsJaW50IHJldDsKPiA+ICsKPiA+ICsJZGV2ID0gZHJtLT5kZXY7Cj4gPiArCXBkZXYgPSB0b19w bGF0Zm9ybV9kZXZpY2UoZGV2KTsKPiA+ICsKPiA+ICsJbWF0Y2ggPSBvZl9tYXRjaF9ub2RlKGF0 bWVsX2xjZGNfb2ZfbWF0Y2gsIGRldi0+cGFyZW50LT5vZl9ub2RlKTsKPiA+ICsJaWYgKCFtYXRj aCkgewo+ID4gKwkJRFJNX0RFVl9FUlJPUihkZXYsICJpbnZhbGlkIGNvbXBhdGlibGUgc3RyaW5n IChub2RlPSVzKSIsCj4gPiArCQkJICAgICAgZGV2LT5wYXJlbnQtPm9mX25vZGUtPm5hbWUpOwo+ ID4gKwkJcmV0dXJuIC1FTk9ERVY7Cj4gPiArCX0KPiA+ICsKPiA+ICsJaWYgKCFtYXRjaC0+ZGF0 YSkgewo+ID4gKwkJRFJNX0RFVl9FUlJPUihkZXYsICJpbnZhbGlkIGxjZGNfZGMgZGVzY3JpcHRp b25cbiIpOwo+ID4gKwkJcmV0dXJuIC1FSU5WQUw7Cj4gPiArCX0KPiA+ICsKPiA+ICsJbGNkY19k YyA9IGRldm1fa3phbGxvYyhkZXYsIHNpemVvZigqbGNkY19kYyksIEdGUF9LRVJORUwpOwo+ID4g KwlpZiAoIWxjZGNfZGMpIHsKPiA+ICsJCURSTV9ERVZfRVJST1IoZGV2LCAiRmFpbGVkIHRvIGFs bG9jYXRlIGxjZGNfZGNcbiIpOwo+ID4gKwkJcmV0dXJuIC1FTk9NRU07Cj4gPiArCX0KPiA+ICsK PiA+ICsJLyogcmVzZXQgb2YgbGNkYyBtaWdodCBzbGVlcCBhbmQgcmVxdWlyZSBhIHByZWVtcHRp YmxlIHRhc2sgY29udGV4dCAqLwo+ID4gKwlJTklUX1dPUksoJmxjZGNfZGMtPnJlc2V0X2xjZGNf d29yaywgcmVzZXRfbGNkY193b3JrKTsKPiA+ICsKPiA+ICsJcGxhdGZvcm1fc2V0X2RydmRhdGEo cGRldiwgZHJtKTsKPiA+ICsJZGV2X3NldF9kcnZkYXRhKGRldiwgbGNkY19kYyk7Cj4gPiArCj4g PiArCWxjZGNfZGMtPm1mZF9sY2RjID0gZGV2X2dldF9kcnZkYXRhKGRldi0+cGFyZW50KTsKPiA+ ICsJZHJtLT5kZXZfcHJpdmF0ZSA9IGxjZGNfZGM7Cj4gPiArCj4gPiArCWxjZGNfZGMtPnJlZ21h cCA9IGxjZGNfZGMtPm1mZF9sY2RjLT5yZWdtYXA7Cj4gPiArCWxjZGNfZGMtPmRlc2MgPSBtYXRj aC0+ZGF0YTsKPiA+ICsJbGNkY19kYy0+ZGV2ID0gZGV2Owo+ID4gKwo+ID4gKwlsY2RjX2RjLT5s Y2Rfc3VwcGx5ID0gZGV2bV9yZWd1bGF0b3JfZ2V0KGRldiwgImxjZCIpOwo+ID4gKwlpZiAoSVNf RVJSKGxjZGNfZGMtPmxjZF9zdXBwbHkpKSB7Cj4gPiArCQlEUk1fREVWX0VSUk9SKGRldiwgIkZh aWxlZCB0byBnZXQgbGNkLXN1cHBseSAoJWxkKVxuIiwKPiA+ICsJCQkgICAgICBQVFJfRVJSKGxj ZGNfZGMtPmxjZF9zdXBwbHkpKTsKPiA+ICsJCWxjZGNfZGMtPmxjZF9zdXBwbHkgPSBOVUxMOwo+ ID4gKwl9Cj4gPiArCj4gPiArCWxjZGNfZGNfc3RhcnRfY2xvY2sobGNkY19kYyk7Cj4gCj4gSG0s IGRvIHlvdSByZWFsbHkgbmVlZCB0byBjYWxsIHRoYXQgaGVyZT8gSSdkIG1ha2UgaXQgcGFydCBv ZiB0aGUKPiBydW50aW1lIFBNIHJlc3VtZSBob29rLCBhbmQgcHV0IGEgbGNkY19kY19zdG9wX2Ns b2NrKCkgaW4gdGhlIHN1c3BlbmQKPiBob29rLgoKQXMgc3VnZ2VzdGVkIEkgaGF2ZSBpbnRyb2R1 Y2VkIHN1c3BlbmQvcmVzdW1lIGhvb2tzIGFuZCBoYXZlIGluY2x1ZGVkCnN0YXJ0L3N0b3AgY2xv Y2sgY2FsbHMuCgpCdXQgYXMgdGhlIHN1c3BlbmQvcmVzdW1lIGhvb2tzIGRlcGVuZHMgb24gQ09O RklHX1BNX1NMRUVQIEkKc3RpbGwgbmVlZCB0aGUgZXhwbGljaXQgY2FsbCBpbiB0aGlzIGZ1bmN0 aW9uLgoKPiA+ICsKPiA+ICsJcG1fcnVudGltZV9lbmFibGUoZGV2KTsKPiA+ICsKPiA+ICsJcmV0 ID0gZHJtX3ZibGFua19pbml0KGRybSwgMSk7Cj4gPiArCWlmIChyZXQpIHsKPiA+ICsJCURSTV9E RVZfRVJST1IoZGV2LCAiZmFpbGVkIHRvIGluaXRpYWxpemUgdmJsYW5rICglZClcbiIsCj4gPiAr CQkJICAgICAgcmV0KTsKPiA+ICsJCWdvdG8gZXJyX3BtX3J1bnRpbWVfZGlzYWJsZTsKPiA+ICsJ fQo+ID4gKwo+ID4gKwlyZXQgPSBsY2RjX2RjX21vZGVzZXRfaW5pdChsY2RjX2RjLCBkcm0pOwo+ ID4gKwlpZiAocmV0KSB7Cj4gPiArCQlEUk1fREVWX0VSUk9SKGRldiwgIm1vZGVzZXRfaW5pdCBm YWlsZWQgKCVkKSIsIHJldCk7Cj4gPiArCQlnb3RvIGVycl9wbV9ydW50aW1lX2Rpc2FibGU7Cj4g PiArCX0KPiA+ICsKPiA+ICsJcG1fcnVudGltZV9nZXRfc3luYyhkZXYpOwo+IAo+IFRoaXMgY2Fs bCB3aWxsIGF1dG9tYXRpY2FsbHkgY2FsbCB0aGUgcnVudGltZSBQTSByZXN1bWUgaG9vaywgc28g aWYgeW91Cj4gbmVlZCB0aGUgY2xrIHRvIGJlIGVuYWJsZWQgYmVmb3JlIHRoYXQgcG9pbnQgeW91 IHNob3VsZCBwdXQgaXQgZWFybGllci4KPiAKPiBBbHNvLCB5b3Ugc2hvdWxkIGNhbGwgcG1fcnVu dGltZV9nZXRfc3luYygpIGluIHRoZSAtPmVuYWJsZSgpIHBhdGggYW5kCj4gcG1fcnVudGltZV9w dXQoKSBpbiB0aGUgLT5kaXNhYmxlKCkgcGF0aC4KR29vZCBjYXRjaCEgQWRkZWQuCgoJU2FtCgoK X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 From: sam@ravnborg.org (Sam Ravnborg) Date: Sun, 26 Aug 2018 20:41:21 +0200 Subject: [PATCH v1 7/7] drm: add Atmel LCDC display controller support In-Reply-To: <20180824143125.4e99e791@bbrezillon> References: <20180812184152.GA22343@ravnborg.org> <20180812184629.3808-7-sam@ravnborg.org> <20180824143125.4e99e791@bbrezillon> Message-ID: <20180826184121.GA24867@ravnborg.org> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Boris. Very usefull feedback! I am working on v2, and have addressed your review items. Just a few comments below to a few items. The rest is processed/done. I hope to post v2 in the week to come. > > +static int lcdc_dc_load(struct drm_device *drm) > > +{ > > + const struct of_device_id *match; > > + struct platform_device *pdev; > > + struct lcdc_dc *lcdc_dc; > > + struct device *dev; > > + int ret; > > + > > + dev = drm->dev; > > + pdev = to_platform_device(dev); > > + > > + match = of_match_node(atmel_lcdc_of_match, dev->parent->of_node); > > + if (!match) { > > + DRM_DEV_ERROR(dev, "invalid compatible string (node=%s)", > > + dev->parent->of_node->name); > > + return -ENODEV; > > + } > > + > > + if (!match->data) { > > + DRM_DEV_ERROR(dev, "invalid lcdc_dc description\n"); > > + return -EINVAL; > > + } > > + > > + lcdc_dc = devm_kzalloc(dev, sizeof(*lcdc_dc), GFP_KERNEL); > > + if (!lcdc_dc) { > > + DRM_DEV_ERROR(dev, "Failed to allocate lcdc_dc\n"); > > + return -ENOMEM; > > + } > > + > > + /* reset of lcdc might sleep and require a preemptible task context */ > > + INIT_WORK(&lcdc_dc->reset_lcdc_work, reset_lcdc_work); > > + > > + platform_set_drvdata(pdev, drm); > > + dev_set_drvdata(dev, lcdc_dc); > > + > > + lcdc_dc->mfd_lcdc = dev_get_drvdata(dev->parent); > > + drm->dev_private = lcdc_dc; > > + > > + lcdc_dc->regmap = lcdc_dc->mfd_lcdc->regmap; > > + lcdc_dc->desc = match->data; > > + lcdc_dc->dev = dev; > > + > > + lcdc_dc->lcd_supply = devm_regulator_get(dev, "lcd"); > > + if (IS_ERR(lcdc_dc->lcd_supply)) { > > + DRM_DEV_ERROR(dev, "Failed to get lcd-supply (%ld)\n", > > + PTR_ERR(lcdc_dc->lcd_supply)); > > + lcdc_dc->lcd_supply = NULL; > > + } > > + > > + lcdc_dc_start_clock(lcdc_dc); > > Hm, do you really need to call that here? I'd make it part of the > runtime PM resume hook, and put a lcdc_dc_stop_clock() in the suspend > hook. As suggested I have introduced suspend/resume hooks and have included start/stop clock calls. But as the suspend/resume hooks depends on CONFIG_PM_SLEEP I still need the explicit call in this function. > > + > > + pm_runtime_enable(dev); > > + > > + ret = drm_vblank_init(drm, 1); > > + if (ret) { > > + DRM_DEV_ERROR(dev, "failed to initialize vblank (%d)\n", > > + ret); > > + goto err_pm_runtime_disable; > > + } > > + > > + ret = lcdc_dc_modeset_init(lcdc_dc, drm); > > + if (ret) { > > + DRM_DEV_ERROR(dev, "modeset_init failed (%d)", ret); > > + goto err_pm_runtime_disable; > > + } > > + > > + pm_runtime_get_sync(dev); > > This call will automatically call the runtime PM resume hook, so if you > need the clk to be enabled before that point you should put it earlier. > > Also, you should call pm_runtime_get_sync() in the ->enable() path and > pm_runtime_put() in the ->disable() path. Good catch! Added. Sam