From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.3 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id C8AB8C432C0 for ; Tue, 3 Dec 2019 06:32:51 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 99439206DF for ; Tue, 3 Dec 2019 06:32:51 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="shWRtoXq"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="CzbNxfKY" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 99439206DF Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=ideasonboard.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=QFukoE0eCTTyknC9m0HzlUWS8js342t9DnIiu/6WwG4=; b=shWRtoXqNCneel /PKEAb6P3qHD6TI8ZgsC0Q/dL00IrAX65O+yLFKP511+MkX0x55a9rqGoTy0Rwsd7isanGBokS4f0 EMrBRxYhkz8et6jZJSbWGPJNZoW1+W/ljWzvL8SvfHQiRuWLWzz90jI/0D8c2tNl30rbS/LmKNYHy oHTosr6iE0uYHe53t6XRUqePyofI+P3FfoVyXkZqFEbHq3dhtyVvfVExzoOMMuZjn3ioHE5HwFuZa hKDUoLxd+2KVQjCZAkRMkKtYSQm5nv3fRvwtxx1QRu7DfEWajMYPpoA3e+2BVX9tomMWQV2svb0ds okx9XIKvGaotUy1IkqTQ==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1ic1ju-0005ZP-Tx; Tue, 03 Dec 2019 06:32:50 +0000 Received: from perceval.ideasonboard.com ([2001:4b98:dc2:55:216:3eff:fef7:d647]) by bombadil.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1ic1jm-0005RF-U9; Tue, 03 Dec 2019 06:32:44 +0000 Received: from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi [81.175.216.236]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id A0EE7309; Tue, 3 Dec 2019 07:32:38 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1575354758; bh=Rt79t6HLiiyCOgEHHbw0L8UEFS/OIX7umWr/gd4dZCY=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=CzbNxfKYd5bUsGEcS2rhEu+uoCUjokLQ4WsuQbWKgzc+U8thHTq+0k9fQcWtl5tBO 4A036+g0JTCEDoJzFyTqN2qYLNgzFQkFiQ6DEO4AiENXoPs8NrzYjq9FVBILJTVxEU Z5mhFidnih+AEdUFFomcz9JMUgNvEQGQpSDkIyzM= Date: Tue, 3 Dec 2019 08:32:32 +0200 From: Laurent Pinchart To: Sam Ravnborg Subject: Re: [PATCH v1 02/26] drm/panel: add backlight support Message-ID: <20191203063232.GB4730@pendragon.ideasonboard.com> References: <20191202193230.21310-1-sam@ravnborg.org> <20191202193230.21310-3-sam@ravnborg.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20191202193230.21310-3-sam@ravnborg.org> User-Agent: Mutt/1.10.1 (2018-07-13) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20191202_223243_268051_06D26E33 X-CRM114-Status: GOOD ( 28.47 ) X-BeenThere: linux-mediatek@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Neil Armstrong , David Airlie , Linus Walleij , dri-devel@lists.freedesktop.org, Andrzej Hajda , Thierry Reding , Benjamin Gaignard , Stefan Agner , linux-samsung-soc@vger.kernel.org, linux-rockchip@lists.infradead.org, Tomi Valkeinen , NXP Linux Team , Jagan Teki , Jitao Shi , Pengutronix Kernel Team , Maarten Lankhorst , Maxime Ripard , linux-mediatek@lists.infradead.org, Abhinav Kumar , linux-tegra@vger.kernel.org, Maxime Ripard , Sean Paul , linux-arm-kernel@lists.infradead.org, Purism Kernel Team , linux-renesas-soc@vger.kernel.org, Boris Brezillon , Daniel Vetter Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "Linux-mediatek" Errors-To: linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.org Hi Sam, Thank you for the patch. On Mon, Dec 02, 2019 at 08:32:06PM +0100, Sam Ravnborg wrote: > Panels often supports backlight as specified in a device tree. > Update the drm_panel infrastructure to support this to > simplify the drivers. > > With this the panel driver just needs to add the following to the > probe() function: > > err = drm_panel_of_backlight(panel); > if (err) > return err; > > Then drm_panel will handle all the rest. > > v2: > - Drop test of CONFIG_DRM_PANEL in header-file (Laurent) > - do not enable backlight if ->enable() returns an error > > Signed-off-by: Sam Ravnborg > Cc: Maarten Lankhorst > Cc: Maxime Ripard > Cc: Sean Paul > Cc: Thierry Reding > Cc: Sam Ravnborg > Cc: David Airlie > Cc: Daniel Vetter > --- > drivers/gpu/drm/drm_panel.c | 49 +++++++++++++++++++++++++++++++++++-- > include/drm/drm_panel.h | 23 +++++++++++++++++ > 2 files changed, 70 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c > index 2d59cdd05e50..35609c90e467 100644 > --- a/drivers/gpu/drm/drm_panel.c > +++ b/drivers/gpu/drm/drm_panel.c > @@ -21,6 +21,7 @@ > * DEALINGS IN THE SOFTWARE. > */ > > +#include > #include > #include > > @@ -196,13 +197,18 @@ EXPORT_SYMBOL(drm_panel_unprepare); > */ > int drm_panel_enable(struct drm_panel *panel) > { > + int ret = 0; > + > if (!panel) > return -EINVAL; > > if (panel->funcs && panel->funcs->enable) > - return panel->funcs->enable(panel); > + ret = panel->funcs->enable(panel); > > - return 0; > + if (ret >= 0) I'd write if (panel->funcs && panel->funcs->enable) { ret = panel->funcs->enable(panel); if (ret < 0) return ret; } and then handle the backlight with one less indentation level. > + backlight_enable(panel->backlight); What is backlight_enable() returns an error ? Should we at least log that ? > + > + return ret; > } > EXPORT_SYMBOL(drm_panel_enable); > > @@ -221,6 +227,8 @@ int drm_panel_disable(struct drm_panel *panel) > if (!panel) > return -EINVAL; > > + backlight_disable(panel->backlight); > + > if (panel->funcs && panel->funcs->disable) > return panel->funcs->disable(panel); > > @@ -289,6 +297,43 @@ struct drm_panel *of_drm_find_panel(const struct device_node *np) > EXPORT_SYMBOL(of_drm_find_panel); > #endif > > +#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE > +/** > + * drm_panel_of_backlight - use backlight device node for backlight > + * @panel: DRM panel > + * > + * Use this function to enable backlight handling if your panel > + * uses device tree and has a backlight handle. s/handle/phandle/ > + * > + * When panel is enabled backlight will be enabled after a s/panel/the panel/ > + * successfull call to &drm_panel_funcs.enable() > + * > + * When panel is disabled backlight will be disabled before the Same here. > + * call to &drm_panel_funcs.disable(). > + * > + * A typical implementation for a panel driver supporting device tree > + * will call this function and then backlight just works. How about "will call this function at probe time. Backlight will then be handled transparently without requiring any intervention from the driver at runtime." > + * > + * Return: 0 on success or a negative error code on failure. > + */ > +int drm_panel_of_backlight(struct drm_panel *panel) > +{ > + struct backlight_device *backlight; > + > + if (!panel || !panel->dev) > + return -EINVAL; > + > + backlight = devm_of_find_backlight(panel->dev); > + > + if (IS_ERR(backlight)) > + return PTR_ERR(backlight); > + > + panel->backlight = backlight; > + return 0; > +} > +EXPORT_SYMBOL(drm_panel_of_backlight); > +#endif > + > MODULE_AUTHOR("Thierry Reding "); > MODULE_DESCRIPTION("DRM panel infrastructure"); > MODULE_LICENSE("GPL and additional rights"); > diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h > index ce8da64022b4..d30c98567384 100644 > --- a/include/drm/drm_panel.h > +++ b/include/drm/drm_panel.h > @@ -28,6 +28,7 @@ > #include > #include > > +struct backlight_device; > struct device_node; > struct drm_connector; > struct drm_device; > @@ -59,6 +60,10 @@ struct display_timing; > * > * To save power when no video data is transmitted, a driver can power down > * the panel. This is the job of the .unprepare() function. > + * > + * Backlight can be handled automatically if configured using > + * drm_panel_of_backlight(). Then the driver do not need to implement the s/do not/does not/ > + * functionality to enable/disable backlight. > */ > struct drm_panel_funcs { > /** > @@ -132,6 +137,15 @@ struct drm_panel { > */ > struct device *dev; > > + /** > + * @backlight: > + * > + * Backlight device, used to turn on backlight after > + * the call to enable(), and to turn off > + * backlight before call to disable(). s/before call/before the call/ or maybe simpler s/before call to disable()/before disable()/ (and 'after enable()' above in that case). Should you mention that this field shall not be set directly by drivers, but is set by drm_panel_of_backlight() instead ? You can also reflow the text to reach the 80 columns limit :-) > + */ > + struct backlight_device *backlight; > + > /** > * @funcs: > * > @@ -183,4 +197,13 @@ static inline struct drm_panel *of_drm_find_panel(const struct device_node *np) > } > #endif > > +#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE) > +int drm_panel_of_backlight(struct drm_panel *panel); > +#else > +static inline int drm_panel_of_backlight(struct drm_panel *panel) > +{ > + return -EINVAL; Shouldn't you return 0 instead ? Otherwise panel driver that can support backlight will all fail to probe if CONFIG_BACKLIGHT_CLASS_DEVICE is disabled. > +} > +#endif > + > #endif -- Regards, Laurent Pinchart _______________________________________________ Linux-mediatek mailing list Linux-mediatek@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-mediatek From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH v1 02/26] drm/panel: add backlight support Date: Tue, 3 Dec 2019 08:32:32 +0200 Message-ID: <20191203063232.GB4730@pendragon.ideasonboard.com> References: <20191202193230.21310-1-sam@ravnborg.org> <20191202193230.21310-3-sam@ravnborg.org> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Content-Disposition: inline In-Reply-To: <20191202193230.21310-3-sam@ravnborg.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Sam Ravnborg Cc: Neil Armstrong , David Airlie , dri-devel@lists.freedesktop.org, Thierry Reding , linux-samsung-soc@vger.kernel.org, linux-rockchip@lists.infradead.org, Tomi Valkeinen , NXP Linux Team , Jagan Teki , Jitao Shi , Pengutronix Kernel Team , linux-mediatek@lists.infradead.org, Abhinav Kumar , linux-tegra@vger.kernel.org, Maxime Ripard , Sean Paul , linux-arm-kernel@lists.infradead.org, Purism Kernel Team , linux-renesas-soc@vger.kernel.org, Boris Brezillon List-Id: linux-rockchip.vger.kernel.org SGkgU2FtLAoKVGhhbmsgeW91IGZvciB0aGUgcGF0Y2guCgpPbiBNb24sIERlYyAwMiwgMjAxOSBh dCAwODozMjowNlBNICswMTAwLCBTYW0gUmF2bmJvcmcgd3JvdGU6Cj4gUGFuZWxzIG9mdGVuIHN1 cHBvcnRzIGJhY2tsaWdodCBhcyBzcGVjaWZpZWQgaW4gYSBkZXZpY2UgdHJlZS4KPiBVcGRhdGUg dGhlIGRybV9wYW5lbCBpbmZyYXN0cnVjdHVyZSB0byBzdXBwb3J0IHRoaXMgdG8KPiBzaW1wbGlm eSB0aGUgZHJpdmVycy4KPiAKPiBXaXRoIHRoaXMgdGhlIHBhbmVsIGRyaXZlciBqdXN0IG5lZWRz IHRvIGFkZCB0aGUgZm9sbG93aW5nIHRvIHRoZQo+IHByb2JlKCkgZnVuY3Rpb246Cj4gCj4gICAg IGVyciA9IGRybV9wYW5lbF9vZl9iYWNrbGlnaHQocGFuZWwpOwo+ICAgICBpZiAoZXJyKQo+ICAg ICAgICAgICAgIHJldHVybiBlcnI7Cj4gCj4gVGhlbiBkcm1fcGFuZWwgd2lsbCBoYW5kbGUgYWxs IHRoZSByZXN0Lgo+IAo+IHYyOgo+IC0gRHJvcCB0ZXN0IG9mIENPTkZJR19EUk1fUEFORUwgaW4g aGVhZGVyLWZpbGUgKExhdXJlbnQpCj4gLSBkbyBub3QgZW5hYmxlIGJhY2tsaWdodCBpZiAtPmVu YWJsZSgpIHJldHVybnMgYW4gZXJyb3IKPiAKPiBTaWduZWQtb2ZmLWJ5OiBTYW0gUmF2bmJvcmcg PHNhbUByYXZuYm9yZy5vcmc+Cj4gQ2M6IE1hYXJ0ZW4gTGFua2hvcnN0IDxtYWFydGVuLmxhbmto b3JzdEBsaW51eC5pbnRlbC5jb20+Cj4gQ2M6IE1heGltZSBSaXBhcmQgPG1heGltZS5yaXBhcmRA Ym9vdGxpbi5jb20+Cj4gQ2M6IFNlYW4gUGF1bCA8c2VhbkBwb29ybHkucnVuPgo+IENjOiBUaGll cnJ5IFJlZGluZyA8dGhpZXJyeS5yZWRpbmdAZ21haWwuY29tPgo+IENjOiBTYW0gUmF2bmJvcmcg PHNhbUByYXZuYm9yZy5vcmc+Cj4gQ2M6IERhdmlkIEFpcmxpZSA8YWlybGllZEBsaW51eC5pZT4K PiBDYzogRGFuaWVsIFZldHRlciA8ZGFuaWVsQGZmd2xsLmNoPgo+IC0tLQo+ICBkcml2ZXJzL2dw dS9kcm0vZHJtX3BhbmVsLmMgfCA0OSArKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysr Ky0tCj4gIGluY2x1ZGUvZHJtL2RybV9wYW5lbC5oICAgICB8IDIzICsrKysrKysrKysrKysrKysr Cj4gIDIgZmlsZXMgY2hhbmdlZCwgNzAgaW5zZXJ0aW9ucygrKSwgMiBkZWxldGlvbnMoLSkKPiAK PiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9ncHUvZHJtL2RybV9wYW5lbC5jIGIvZHJpdmVycy9ncHUv ZHJtL2RybV9wYW5lbC5jCj4gaW5kZXggMmQ1OWNkZDA1ZTUwLi4zNTYwOWM5MGU0NjcgMTAwNjQ0 Cj4gLS0tIGEvZHJpdmVycy9ncHUvZHJtL2RybV9wYW5lbC5jCj4gKysrIGIvZHJpdmVycy9ncHUv ZHJtL2RybV9wYW5lbC5jCj4gQEAgLTIxLDYgKzIxLDcgQEAKPiAgICogREVBTElOR1MgSU4gVEhF IFNPRlRXQVJFLgo+ICAgKi8KPiAgCj4gKyNpbmNsdWRlIDxsaW51eC9iYWNrbGlnaHQuaD4KPiAg I2luY2x1ZGUgPGxpbnV4L2Vyci5oPgo+ICAjaW5jbHVkZSA8bGludXgvbW9kdWxlLmg+Cj4gIAo+ IEBAIC0xOTYsMTMgKzE5NywxOCBAQCBFWFBPUlRfU1lNQk9MKGRybV9wYW5lbF91bnByZXBhcmUp Owo+ICAgKi8KPiAgaW50IGRybV9wYW5lbF9lbmFibGUoc3RydWN0IGRybV9wYW5lbCAqcGFuZWwp Cj4gIHsKPiArCWludCByZXQgPSAwOwo+ICsKPiAgCWlmICghcGFuZWwpCj4gIAkJcmV0dXJuIC1F SU5WQUw7Cj4gIAo+ICAJaWYgKHBhbmVsLT5mdW5jcyAmJiBwYW5lbC0+ZnVuY3MtPmVuYWJsZSkK PiAtCQlyZXR1cm4gcGFuZWwtPmZ1bmNzLT5lbmFibGUocGFuZWwpOwo+ICsJCXJldCA9IHBhbmVs LT5mdW5jcy0+ZW5hYmxlKHBhbmVsKTsKPiAgCj4gLQlyZXR1cm4gMDsKPiArCWlmIChyZXQgPj0g MCkKCgpJJ2Qgd3JpdGUKCglpZiAocGFuZWwtPmZ1bmNzICYmIHBhbmVsLT5mdW5jcy0+ZW5hYmxl KSB7CgkJcmV0ID0gcGFuZWwtPmZ1bmNzLT5lbmFibGUocGFuZWwpOwoJCWlmIChyZXQgPCAwKQoJ CQlyZXR1cm4gcmV0OwoJfQoKYW5kIHRoZW4gaGFuZGxlIHRoZSBiYWNrbGlnaHQgd2l0aCBvbmUg bGVzcyBpbmRlbnRhdGlvbiBsZXZlbC4KCgo+ICsJCWJhY2tsaWdodF9lbmFibGUocGFuZWwtPmJh Y2tsaWdodCk7CgpXaGF0IGlzIGJhY2tsaWdodF9lbmFibGUoKSByZXR1cm5zIGFuIGVycm9yID8g U2hvdWxkIHdlIGF0IGxlYXN0IGxvZwp0aGF0ID8KCj4gKwo+ICsJcmV0dXJuIHJldDsKPiAgfQo+ ICBFWFBPUlRfU1lNQk9MKGRybV9wYW5lbF9lbmFibGUpOwo+ICAKPiBAQCAtMjIxLDYgKzIyNyw4 IEBAIGludCBkcm1fcGFuZWxfZGlzYWJsZShzdHJ1Y3QgZHJtX3BhbmVsICpwYW5lbCkKPiAgCWlm ICghcGFuZWwpCj4gIAkJcmV0dXJuIC1FSU5WQUw7Cj4gIAo+ICsJYmFja2xpZ2h0X2Rpc2FibGUo cGFuZWwtPmJhY2tsaWdodCk7Cj4gKwo+ICAJaWYgKHBhbmVsLT5mdW5jcyAmJiBwYW5lbC0+ZnVu Y3MtPmRpc2FibGUpCj4gIAkJcmV0dXJuIHBhbmVsLT5mdW5jcy0+ZGlzYWJsZShwYW5lbCk7Cj4g IAo+IEBAIC0yODksNiArMjk3LDQzIEBAIHN0cnVjdCBkcm1fcGFuZWwgKm9mX2RybV9maW5kX3Bh bmVsKGNvbnN0IHN0cnVjdCBkZXZpY2Vfbm9kZSAqbnApCj4gIEVYUE9SVF9TWU1CT0wob2ZfZHJt X2ZpbmRfcGFuZWwpOwo+ICAjZW5kaWYKPiAgCj4gKyNpZmRlZiBDT05GSUdfQkFDS0xJR0hUX0NM QVNTX0RFVklDRQo+ICsvKioKPiArICogZHJtX3BhbmVsX29mX2JhY2tsaWdodCAtIHVzZSBiYWNr bGlnaHQgZGV2aWNlIG5vZGUgZm9yIGJhY2tsaWdodAo+ICsgKiBAcGFuZWw6IERSTSBwYW5lbAo+ ICsgKgo+ICsgKiBVc2UgdGhpcyBmdW5jdGlvbiB0byBlbmFibGUgYmFja2xpZ2h0IGhhbmRsaW5n IGlmIHlvdXIgcGFuZWwKPiArICogdXNlcyBkZXZpY2UgdHJlZSBhbmQgaGFzIGEgYmFja2xpZ2h0 IGhhbmRsZS4KCnMvaGFuZGxlL3BoYW5kbGUvCgo+ICsgKgo+ICsgKiBXaGVuIHBhbmVsIGlzIGVu YWJsZWQgYmFja2xpZ2h0IHdpbGwgYmUgZW5hYmxlZCBhZnRlciBhCgpzL3BhbmVsL3RoZSBwYW5l bC8KCj4gKyAqIHN1Y2Nlc3NmdWxsIGNhbGwgdG8gJmRybV9wYW5lbF9mdW5jcy5lbmFibGUoKQo+ ICsgKgo+ICsgKiBXaGVuIHBhbmVsIGlzIGRpc2FibGVkIGJhY2tsaWdodCB3aWxsIGJlIGRpc2Fi bGVkIGJlZm9yZSB0aGUKClNhbWUgaGVyZS4KCj4gKyAqIGNhbGwgdG8gJmRybV9wYW5lbF9mdW5j cy5kaXNhYmxlKCkuCj4gKyAqCj4gKyAqIEEgdHlwaWNhbCBpbXBsZW1lbnRhdGlvbiBmb3IgYSBw YW5lbCBkcml2ZXIgc3VwcG9ydGluZyBkZXZpY2UgdHJlZQo+ICsgKiB3aWxsIGNhbGwgdGhpcyBm dW5jdGlvbiBhbmQgdGhlbiBiYWNrbGlnaHQganVzdCB3b3Jrcy4KCkhvdyBhYm91dAoKIndpbGwg Y2FsbCB0aGlzIGZ1bmN0aW9uIGF0IHByb2JlIHRpbWUuIEJhY2tsaWdodCB3aWxsIHRoZW4gYmUg aGFuZGxlZAp0cmFuc3BhcmVudGx5IHdpdGhvdXQgcmVxdWlyaW5nIGFueSBpbnRlcnZlbnRpb24g ZnJvbSB0aGUgZHJpdmVyIGF0CnJ1bnRpbWUuIgoKPiArICoKPiArICogUmV0dXJuOiAwIG9uIHN1 Y2Nlc3Mgb3IgYSBuZWdhdGl2ZSBlcnJvciBjb2RlIG9uIGZhaWx1cmUuCj4gKyAqLwo+ICtpbnQg ZHJtX3BhbmVsX29mX2JhY2tsaWdodChzdHJ1Y3QgZHJtX3BhbmVsICpwYW5lbCkKPiArewo+ICsJ c3RydWN0IGJhY2tsaWdodF9kZXZpY2UgKmJhY2tsaWdodDsKPiArCj4gKwlpZiAoIXBhbmVsIHx8 ICFwYW5lbC0+ZGV2KQo+ICsJCXJldHVybiAtRUlOVkFMOwo+ICsKPiArCWJhY2tsaWdodCA9IGRl dm1fb2ZfZmluZF9iYWNrbGlnaHQocGFuZWwtPmRldik7Cj4gKwo+ICsJaWYgKElTX0VSUihiYWNr bGlnaHQpKQo+ICsgICAgICAgICAgICAgICAgcmV0dXJuIFBUUl9FUlIoYmFja2xpZ2h0KTsKPiAr Cj4gKwlwYW5lbC0+YmFja2xpZ2h0ID0gYmFja2xpZ2h0Owo+ICsJcmV0dXJuIDA7Cj4gK30KPiAr RVhQT1JUX1NZTUJPTChkcm1fcGFuZWxfb2ZfYmFja2xpZ2h0KTsKPiArI2VuZGlmCj4gKwo+ICBN T0RVTEVfQVVUSE9SKCJUaGllcnJ5IFJlZGluZyA8dHJlZGluZ0BudmlkaWEuY29tPiIpOwo+ICBN T0RVTEVfREVTQ1JJUFRJT04oIkRSTSBwYW5lbCBpbmZyYXN0cnVjdHVyZSIpOwo+ICBNT0RVTEVf TElDRU5TRSgiR1BMIGFuZCBhZGRpdGlvbmFsIHJpZ2h0cyIpOwo+IGRpZmYgLS1naXQgYS9pbmNs dWRlL2RybS9kcm1fcGFuZWwuaCBiL2luY2x1ZGUvZHJtL2RybV9wYW5lbC5oCj4gaW5kZXggY2U4 ZGE2NDAyMmI0Li5kMzBjOTg1NjczODQgMTAwNjQ0Cj4gLS0tIGEvaW5jbHVkZS9kcm0vZHJtX3Bh bmVsLmgKPiArKysgYi9pbmNsdWRlL2RybS9kcm1fcGFuZWwuaAo+IEBAIC0yOCw2ICsyOCw3IEBA Cj4gICNpbmNsdWRlIDxsaW51eC9lcnJuby5oPgo+ICAjaW5jbHVkZSA8bGludXgvbGlzdC5oPgo+ ICAKPiArc3RydWN0IGJhY2tsaWdodF9kZXZpY2U7Cj4gIHN0cnVjdCBkZXZpY2Vfbm9kZTsKPiAg c3RydWN0IGRybV9jb25uZWN0b3I7Cj4gIHN0cnVjdCBkcm1fZGV2aWNlOwo+IEBAIC01OSw2ICs2 MCwxMCBAQCBzdHJ1Y3QgZGlzcGxheV90aW1pbmc7Cj4gICAqCj4gICAqIFRvIHNhdmUgcG93ZXIg d2hlbiBubyB2aWRlbyBkYXRhIGlzIHRyYW5zbWl0dGVkLCBhIGRyaXZlciBjYW4gcG93ZXIgZG93 bgo+ICAgKiB0aGUgcGFuZWwuIFRoaXMgaXMgdGhlIGpvYiBvZiB0aGUgLnVucHJlcGFyZSgpIGZ1 bmN0aW9uLgo+ICsgKgo+ICsgKiBCYWNrbGlnaHQgY2FuIGJlIGhhbmRsZWQgYXV0b21hdGljYWxs eSBpZiBjb25maWd1cmVkIHVzaW5nCj4gKyAqIGRybV9wYW5lbF9vZl9iYWNrbGlnaHQoKS4gVGhl biB0aGUgZHJpdmVyIGRvIG5vdCBuZWVkIHRvIGltcGxlbWVudCB0aGUKCnMvZG8gbm90L2RvZXMg bm90LwoKPiArICogZnVuY3Rpb25hbGl0eSB0byBlbmFibGUvZGlzYWJsZSBiYWNrbGlnaHQuCj4g ICAqLwo+ICBzdHJ1Y3QgZHJtX3BhbmVsX2Z1bmNzIHsKPiAgCS8qKgo+IEBAIC0xMzIsNiArMTM3 LDE1IEBAIHN0cnVjdCBkcm1fcGFuZWwgewo+ICAJICovCj4gIAlzdHJ1Y3QgZGV2aWNlICpkZXY7 Cj4gIAo+ICsJLyoqCj4gKwkgKiBAYmFja2xpZ2h0Ogo+ICsJICoKPiArCSAqIEJhY2tsaWdodCBk ZXZpY2UsIHVzZWQgdG8gdHVybiBvbiBiYWNrbGlnaHQgYWZ0ZXIKPiArCSAqIHRoZSBjYWxsIHRv IGVuYWJsZSgpLCBhbmQgdG8gdHVybiBvZmYKPiArCSAqIGJhY2tsaWdodCBiZWZvcmUgY2FsbCB0 byBkaXNhYmxlKCkuCgpzL2JlZm9yZSBjYWxsL2JlZm9yZSB0aGUgY2FsbC8KCm9yIG1heWJlIHNp bXBsZXIKCgpzL2JlZm9yZSBjYWxsIHRvIGRpc2FibGUoKS9iZWZvcmUgZGlzYWJsZSgpLwoKKGFu ZCAnYWZ0ZXIgZW5hYmxlKCknIGFib3ZlIGluIHRoYXQgY2FzZSkuCgpTaG91bGQgeW91IG1lbnRp b24gdGhhdCB0aGlzIGZpZWxkIHNoYWxsIG5vdCBiZSBzZXQgZGlyZWN0bHkgYnkgZHJpdmVycywK YnV0IGlzIHNldCBieSBkcm1fcGFuZWxfb2ZfYmFja2xpZ2h0KCkgaW5zdGVhZCA/CgpZb3UgY2Fu IGFsc28gcmVmbG93IHRoZSB0ZXh0IHRvIHJlYWNoIHRoZSA4MCBjb2x1bW5zIGxpbWl0IDotKQoK PiArCSAqLwo+ICsJc3RydWN0IGJhY2tsaWdodF9kZXZpY2UgKmJhY2tsaWdodDsKPiArCj4gIAkv KioKPiAgCSAqIEBmdW5jczoKPiAgCSAqCj4gQEAgLTE4Myw0ICsxOTcsMTMgQEAgc3RhdGljIGlu bGluZSBzdHJ1Y3QgZHJtX3BhbmVsICpvZl9kcm1fZmluZF9wYW5lbChjb25zdCBzdHJ1Y3QgZGV2 aWNlX25vZGUgKm5wKQo+ICB9Cj4gICNlbmRpZgo+ICAKPiArI2lmIElTX0VOQUJMRUQoQ09ORklH X0JBQ0tMSUdIVF9DTEFTU19ERVZJQ0UpCj4gK2ludCBkcm1fcGFuZWxfb2ZfYmFja2xpZ2h0KHN0 cnVjdCBkcm1fcGFuZWwgKnBhbmVsKTsKPiArI2Vsc2UKPiArc3RhdGljIGlubGluZSBpbnQgZHJt X3BhbmVsX29mX2JhY2tsaWdodChzdHJ1Y3QgZHJtX3BhbmVsICpwYW5lbCkKPiArewo+ICsJcmV0 dXJuIC1FSU5WQUw7CgpTaG91bGRuJ3QgeW91IHJldHVybiAwIGluc3RlYWQgPyBPdGhlcndpc2Ug cGFuZWwgZHJpdmVyIHRoYXQgY2FuIHN1cHBvcnQKYmFja2xpZ2h0IHdpbGwgYWxsIGZhaWwgdG8g cHJvYmUgaWYgQ09ORklHX0JBQ0tMSUdIVF9DTEFTU19ERVZJQ0UgaXMKZGlzYWJsZWQuCgo+ICt9 Cj4gKyNlbmRpZgo+ICsKPiAgI2VuZGlmCgotLSAKUmVnYXJkcywKCkxhdXJlbnQgUGluY2hhcnQK X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVs From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.3 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 59FBEC43215 for ; Tue, 3 Dec 2019 06:32:43 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 314FA2073C for ; Tue, 3 Dec 2019 06:32:43 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="CzbNxfKY" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726991AbfLCGcm (ORCPT ); Tue, 3 Dec 2019 01:32:42 -0500 Received: from perceval.ideasonboard.com ([213.167.242.64]:56370 "EHLO perceval.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726521AbfLCGcm (ORCPT ); Tue, 3 Dec 2019 01:32:42 -0500 Received: from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi [81.175.216.236]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id A0EE7309; Tue, 3 Dec 2019 07:32:38 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1575354758; bh=Rt79t6HLiiyCOgEHHbw0L8UEFS/OIX7umWr/gd4dZCY=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=CzbNxfKYd5bUsGEcS2rhEu+uoCUjokLQ4WsuQbWKgzc+U8thHTq+0k9fQcWtl5tBO 4A036+g0JTCEDoJzFyTqN2qYLNgzFQkFiQ6DEO4AiENXoPs8NrzYjq9FVBILJTVxEU Z5mhFidnih+AEdUFFomcz9JMUgNvEQGQpSDkIyzM= Date: Tue, 3 Dec 2019 08:32:32 +0200 From: Laurent Pinchart To: Sam Ravnborg Cc: dri-devel@lists.freedesktop.org, Thierry Reding , Abhinav Kumar , Andrzej Hajda , Benjamin Gaignard , Boris Brezillon , Daniel Vetter , David Airlie , Jagan Teki , Jitao Shi , Linus Walleij , linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, linux-renesas-soc@vger.kernel.org, linux-rockchip@lists.infradead.org, linux-samsung-soc@vger.kernel.org, linux-tegra@vger.kernel.org, Maarten Lankhorst , Maxime Ripard , Neil Armstrong , NXP Linux Team , Pengutronix Kernel Team , Purism Kernel Team , Sean Paul , Stefan Agner , Tomi Valkeinen , Maxime Ripard Subject: Re: [PATCH v1 02/26] drm/panel: add backlight support Message-ID: <20191203063232.GB4730@pendragon.ideasonboard.com> References: <20191202193230.21310-1-sam@ravnborg.org> <20191202193230.21310-3-sam@ravnborg.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20191202193230.21310-3-sam@ravnborg.org> User-Agent: Mutt/1.10.1 (2018-07-13) Sender: linux-samsung-soc-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-samsung-soc@vger.kernel.org Hi Sam, Thank you for the patch. On Mon, Dec 02, 2019 at 08:32:06PM +0100, Sam Ravnborg wrote: > Panels often supports backlight as specified in a device tree. > Update the drm_panel infrastructure to support this to > simplify the drivers. > > With this the panel driver just needs to add the following to the > probe() function: > > err = drm_panel_of_backlight(panel); > if (err) > return err; > > Then drm_panel will handle all the rest. > > v2: > - Drop test of CONFIG_DRM_PANEL in header-file (Laurent) > - do not enable backlight if ->enable() returns an error > > Signed-off-by: Sam Ravnborg > Cc: Maarten Lankhorst > Cc: Maxime Ripard > Cc: Sean Paul > Cc: Thierry Reding > Cc: Sam Ravnborg > Cc: David Airlie > Cc: Daniel Vetter > --- > drivers/gpu/drm/drm_panel.c | 49 +++++++++++++++++++++++++++++++++++-- > include/drm/drm_panel.h | 23 +++++++++++++++++ > 2 files changed, 70 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c > index 2d59cdd05e50..35609c90e467 100644 > --- a/drivers/gpu/drm/drm_panel.c > +++ b/drivers/gpu/drm/drm_panel.c > @@ -21,6 +21,7 @@ > * DEALINGS IN THE SOFTWARE. > */ > > +#include > #include > #include > > @@ -196,13 +197,18 @@ EXPORT_SYMBOL(drm_panel_unprepare); > */ > int drm_panel_enable(struct drm_panel *panel) > { > + int ret = 0; > + > if (!panel) > return -EINVAL; > > if (panel->funcs && panel->funcs->enable) > - return panel->funcs->enable(panel); > + ret = panel->funcs->enable(panel); > > - return 0; > + if (ret >= 0) I'd write if (panel->funcs && panel->funcs->enable) { ret = panel->funcs->enable(panel); if (ret < 0) return ret; } and then handle the backlight with one less indentation level. > + backlight_enable(panel->backlight); What is backlight_enable() returns an error ? Should we at least log that ? > + > + return ret; > } > EXPORT_SYMBOL(drm_panel_enable); > > @@ -221,6 +227,8 @@ int drm_panel_disable(struct drm_panel *panel) > if (!panel) > return -EINVAL; > > + backlight_disable(panel->backlight); > + > if (panel->funcs && panel->funcs->disable) > return panel->funcs->disable(panel); > > @@ -289,6 +297,43 @@ struct drm_panel *of_drm_find_panel(const struct device_node *np) > EXPORT_SYMBOL(of_drm_find_panel); > #endif > > +#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE > +/** > + * drm_panel_of_backlight - use backlight device node for backlight > + * @panel: DRM panel > + * > + * Use this function to enable backlight handling if your panel > + * uses device tree and has a backlight handle. s/handle/phandle/ > + * > + * When panel is enabled backlight will be enabled after a s/panel/the panel/ > + * successfull call to &drm_panel_funcs.enable() > + * > + * When panel is disabled backlight will be disabled before the Same here. > + * call to &drm_panel_funcs.disable(). > + * > + * A typical implementation for a panel driver supporting device tree > + * will call this function and then backlight just works. How about "will call this function at probe time. Backlight will then be handled transparently without requiring any intervention from the driver at runtime." > + * > + * Return: 0 on success or a negative error code on failure. > + */ > +int drm_panel_of_backlight(struct drm_panel *panel) > +{ > + struct backlight_device *backlight; > + > + if (!panel || !panel->dev) > + return -EINVAL; > + > + backlight = devm_of_find_backlight(panel->dev); > + > + if (IS_ERR(backlight)) > + return PTR_ERR(backlight); > + > + panel->backlight = backlight; > + return 0; > +} > +EXPORT_SYMBOL(drm_panel_of_backlight); > +#endif > + > MODULE_AUTHOR("Thierry Reding "); > MODULE_DESCRIPTION("DRM panel infrastructure"); > MODULE_LICENSE("GPL and additional rights"); > diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h > index ce8da64022b4..d30c98567384 100644 > --- a/include/drm/drm_panel.h > +++ b/include/drm/drm_panel.h > @@ -28,6 +28,7 @@ > #include > #include > > +struct backlight_device; > struct device_node; > struct drm_connector; > struct drm_device; > @@ -59,6 +60,10 @@ struct display_timing; > * > * To save power when no video data is transmitted, a driver can power down > * the panel. This is the job of the .unprepare() function. > + * > + * Backlight can be handled automatically if configured using > + * drm_panel_of_backlight(). Then the driver do not need to implement the s/do not/does not/ > + * functionality to enable/disable backlight. > */ > struct drm_panel_funcs { > /** > @@ -132,6 +137,15 @@ struct drm_panel { > */ > struct device *dev; > > + /** > + * @backlight: > + * > + * Backlight device, used to turn on backlight after > + * the call to enable(), and to turn off > + * backlight before call to disable(). s/before call/before the call/ or maybe simpler s/before call to disable()/before disable()/ (and 'after enable()' above in that case). Should you mention that this field shall not be set directly by drivers, but is set by drm_panel_of_backlight() instead ? You can also reflow the text to reach the 80 columns limit :-) > + */ > + struct backlight_device *backlight; > + > /** > * @funcs: > * > @@ -183,4 +197,13 @@ static inline struct drm_panel *of_drm_find_panel(const struct device_node *np) > } > #endif > > +#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE) > +int drm_panel_of_backlight(struct drm_panel *panel); > +#else > +static inline int drm_panel_of_backlight(struct drm_panel *panel) > +{ > + return -EINVAL; Shouldn't you return 0 instead ? Otherwise panel driver that can support backlight will all fail to probe if CONFIG_BACKLIGHT_CLASS_DEVICE is disabled. > +} > +#endif > + > #endif -- Regards, Laurent Pinchart From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.3 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E438CC432C0 for ; Tue, 3 Dec 2019 06:32:46 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id A43E2206DF for ; Tue, 3 Dec 2019 06:32:46 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="nRiNMOAi"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="CzbNxfKY" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org A43E2206DF Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=ideasonboard.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=uBbFWMbc+fdjkehk/8R2pjqzncac/Nvc5gEwS3rhD9Y=; b=nRiNMOAi2pO9Oz 8dN28PsGMbh57kyBSubu0Jv9oRBkypiPsLLD2/whv3cEtsIcRdUBr8FyQqiaSXeqHo1yh426MzOFF MG8OLLLRDixYfMilpK7/ZfY47OgHkfeQpXQ0oDSo9WEn4ZjHzppDwwBFnhKbvWInWHhT8P53FFAoP XiwXTTHS5aMLdrdd1Lx/lZzSgYiEHWvavSQKpAulhYXLQOIGLNsNTcJkZu2GK40m/oM/1mRa1gI2j yzVkK0m+mETaFsyVAdY4/lrx8HzQ7veO/71FxiSjOnFZJYprKbYErbnvIqDu3FLxMqxpr2JMSDSrW kRyeRT5dvSqkJisNCVkg==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1ic1jp-0005S7-Lq; Tue, 03 Dec 2019 06:32:45 +0000 Received: from perceval.ideasonboard.com ([2001:4b98:dc2:55:216:3eff:fef7:d647]) by bombadil.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1ic1jm-0005RF-U9; Tue, 03 Dec 2019 06:32:44 +0000 Received: from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi [81.175.216.236]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id A0EE7309; Tue, 3 Dec 2019 07:32:38 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1575354758; bh=Rt79t6HLiiyCOgEHHbw0L8UEFS/OIX7umWr/gd4dZCY=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=CzbNxfKYd5bUsGEcS2rhEu+uoCUjokLQ4WsuQbWKgzc+U8thHTq+0k9fQcWtl5tBO 4A036+g0JTCEDoJzFyTqN2qYLNgzFQkFiQ6DEO4AiENXoPs8NrzYjq9FVBILJTVxEU Z5mhFidnih+AEdUFFomcz9JMUgNvEQGQpSDkIyzM= Date: Tue, 3 Dec 2019 08:32:32 +0200 From: Laurent Pinchart To: Sam Ravnborg Subject: Re: [PATCH v1 02/26] drm/panel: add backlight support Message-ID: <20191203063232.GB4730@pendragon.ideasonboard.com> References: <20191202193230.21310-1-sam@ravnborg.org> <20191202193230.21310-3-sam@ravnborg.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20191202193230.21310-3-sam@ravnborg.org> User-Agent: Mutt/1.10.1 (2018-07-13) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20191202_223243_268051_06D26E33 X-CRM114-Status: GOOD ( 28.47 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Neil Armstrong , David Airlie , Linus Walleij , dri-devel@lists.freedesktop.org, Andrzej Hajda , Thierry Reding , Benjamin Gaignard , Stefan Agner , linux-samsung-soc@vger.kernel.org, linux-rockchip@lists.infradead.org, Tomi Valkeinen , NXP Linux Team , Jagan Teki , Jitao Shi , Pengutronix Kernel Team , Maarten Lankhorst , Maxime Ripard , linux-mediatek@lists.infradead.org, Abhinav Kumar , linux-tegra@vger.kernel.org, Maxime Ripard , Sean Paul , linux-arm-kernel@lists.infradead.org, Purism Kernel Team , linux-renesas-soc@vger.kernel.org, Boris Brezillon , Daniel Vetter Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Sam, Thank you for the patch. On Mon, Dec 02, 2019 at 08:32:06PM +0100, Sam Ravnborg wrote: > Panels often supports backlight as specified in a device tree. > Update the drm_panel infrastructure to support this to > simplify the drivers. > > With this the panel driver just needs to add the following to the > probe() function: > > err = drm_panel_of_backlight(panel); > if (err) > return err; > > Then drm_panel will handle all the rest. > > v2: > - Drop test of CONFIG_DRM_PANEL in header-file (Laurent) > - do not enable backlight if ->enable() returns an error > > Signed-off-by: Sam Ravnborg > Cc: Maarten Lankhorst > Cc: Maxime Ripard > Cc: Sean Paul > Cc: Thierry Reding > Cc: Sam Ravnborg > Cc: David Airlie > Cc: Daniel Vetter > --- > drivers/gpu/drm/drm_panel.c | 49 +++++++++++++++++++++++++++++++++++-- > include/drm/drm_panel.h | 23 +++++++++++++++++ > 2 files changed, 70 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c > index 2d59cdd05e50..35609c90e467 100644 > --- a/drivers/gpu/drm/drm_panel.c > +++ b/drivers/gpu/drm/drm_panel.c > @@ -21,6 +21,7 @@ > * DEALINGS IN THE SOFTWARE. > */ > > +#include > #include > #include > > @@ -196,13 +197,18 @@ EXPORT_SYMBOL(drm_panel_unprepare); > */ > int drm_panel_enable(struct drm_panel *panel) > { > + int ret = 0; > + > if (!panel) > return -EINVAL; > > if (panel->funcs && panel->funcs->enable) > - return panel->funcs->enable(panel); > + ret = panel->funcs->enable(panel); > > - return 0; > + if (ret >= 0) I'd write if (panel->funcs && panel->funcs->enable) { ret = panel->funcs->enable(panel); if (ret < 0) return ret; } and then handle the backlight with one less indentation level. > + backlight_enable(panel->backlight); What is backlight_enable() returns an error ? Should we at least log that ? > + > + return ret; > } > EXPORT_SYMBOL(drm_panel_enable); > > @@ -221,6 +227,8 @@ int drm_panel_disable(struct drm_panel *panel) > if (!panel) > return -EINVAL; > > + backlight_disable(panel->backlight); > + > if (panel->funcs && panel->funcs->disable) > return panel->funcs->disable(panel); > > @@ -289,6 +297,43 @@ struct drm_panel *of_drm_find_panel(const struct device_node *np) > EXPORT_SYMBOL(of_drm_find_panel); > #endif > > +#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE > +/** > + * drm_panel_of_backlight - use backlight device node for backlight > + * @panel: DRM panel > + * > + * Use this function to enable backlight handling if your panel > + * uses device tree and has a backlight handle. s/handle/phandle/ > + * > + * When panel is enabled backlight will be enabled after a s/panel/the panel/ > + * successfull call to &drm_panel_funcs.enable() > + * > + * When panel is disabled backlight will be disabled before the Same here. > + * call to &drm_panel_funcs.disable(). > + * > + * A typical implementation for a panel driver supporting device tree > + * will call this function and then backlight just works. How about "will call this function at probe time. Backlight will then be handled transparently without requiring any intervention from the driver at runtime." > + * > + * Return: 0 on success or a negative error code on failure. > + */ > +int drm_panel_of_backlight(struct drm_panel *panel) > +{ > + struct backlight_device *backlight; > + > + if (!panel || !panel->dev) > + return -EINVAL; > + > + backlight = devm_of_find_backlight(panel->dev); > + > + if (IS_ERR(backlight)) > + return PTR_ERR(backlight); > + > + panel->backlight = backlight; > + return 0; > +} > +EXPORT_SYMBOL(drm_panel_of_backlight); > +#endif > + > MODULE_AUTHOR("Thierry Reding "); > MODULE_DESCRIPTION("DRM panel infrastructure"); > MODULE_LICENSE("GPL and additional rights"); > diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h > index ce8da64022b4..d30c98567384 100644 > --- a/include/drm/drm_panel.h > +++ b/include/drm/drm_panel.h > @@ -28,6 +28,7 @@ > #include > #include > > +struct backlight_device; > struct device_node; > struct drm_connector; > struct drm_device; > @@ -59,6 +60,10 @@ struct display_timing; > * > * To save power when no video data is transmitted, a driver can power down > * the panel. This is the job of the .unprepare() function. > + * > + * Backlight can be handled automatically if configured using > + * drm_panel_of_backlight(). Then the driver do not need to implement the s/do not/does not/ > + * functionality to enable/disable backlight. > */ > struct drm_panel_funcs { > /** > @@ -132,6 +137,15 @@ struct drm_panel { > */ > struct device *dev; > > + /** > + * @backlight: > + * > + * Backlight device, used to turn on backlight after > + * the call to enable(), and to turn off > + * backlight before call to disable(). s/before call/before the call/ or maybe simpler s/before call to disable()/before disable()/ (and 'after enable()' above in that case). Should you mention that this field shall not be set directly by drivers, but is set by drm_panel_of_backlight() instead ? You can also reflow the text to reach the 80 columns limit :-) > + */ > + struct backlight_device *backlight; > + > /** > * @funcs: > * > @@ -183,4 +197,13 @@ static inline struct drm_panel *of_drm_find_panel(const struct device_node *np) > } > #endif > > +#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE) > +int drm_panel_of_backlight(struct drm_panel *panel); > +#else > +static inline int drm_panel_of_backlight(struct drm_panel *panel) > +{ > + return -EINVAL; Shouldn't you return 0 instead ? Otherwise panel driver that can support backlight will all fail to probe if CONFIG_BACKLIGHT_CLASS_DEVICE is disabled. > +} > +#endif > + > #endif -- Regards, Laurent Pinchart _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel