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