From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1030971AbdDTJBX (ORCPT ); Thu, 20 Apr 2017 05:01:23 -0400 Received: from mga02.intel.com ([134.134.136.20]:40880 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S970125AbdDTJBV (ORCPT ); Thu, 20 Apr 2017 05:01:21 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.37,225,1488873600"; d="scan'208";a="847937064" From: Jani Nikula To: Arnd Bergmann , Laurent Pinchart Cc: Tomi Valkeinen , Linux Kernel Mailing List , dri-devel Subject: Re: [PATCH 1/3] drm/omap: displays: panel-dpi: add backlight dependency In-Reply-To: Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo References: <20170419175939.189098-1-arnd@arndb.de> <2979804.i6EKSW6ohY@avalon> Date: Thu, 20 Apr 2017 12:01:17 +0300 Message-ID: <87o9vrsbaa.fsf@intel.com> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 19 Apr 2017, Arnd Bergmann wrote: > On Wed, Apr 19, 2017 at 10:21 PM, Laurent Pinchart > wrote: >>> >>> This adds a dependency like we have for the other panel drivers. >> >> I believe the dependency should be made optional. DPI panels that don't need >> backlight control should be supported by a kernel that has backlight support >> compiled out. > > That would be nice in principle, but I fear this would cause additional > problems. > >> --- a/include/linux/backlight.h >> +++ b/include/linux/backlight.h >> @@ -162,7 +162,7 @@ struct generic_bl_info { >> void (*kick_battery)(void); >> }; >> >> -#ifdef CONFIG_OF >> +#if IS_ENABLED(CONFIG_OF) && IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE) >> struct backlight_device *of_find_backlight_by_node(struct device_node *node); >> #else >> static inline struct backlight_device * >> >> >> We might need to create stubs for backlight_force_update() and >> backlight_device_set_brightness() too. >> > > With BACKLIGHT_CLASS_DEVICE=m, you still get a link error when the user is > in a built-in driver. Using 'depends on' usually solves this (except for drivers > that cannot be modules). > > There are three possible workarounds for this that I can think of: > > - Use 'depends on BACKLIGHT_CLASS_DEVICE || BACKLIGHT_CLASS_DEVICE=n' > in each driver that implements optional backlight support. We do > this elsewhere, but > it's confusing and easy to get wrong. FWIW I think this is the fix, and not a workaround. "depends on FOO || FOO=n" is an expression used throughout the kernel, and it accurately describes the dependency here. Of course, all drivers implementing this must still wrap backlight class usage around IS_ENABLED(). BR, Jani. > > - use IS_REACHABLE() instead of IS_ENABLED() when testing for > backlight support. This will always result in a kernel that builds cleanly, > but can be surprising for users when backlight support is a module that > gets loaded at boot, but it is still not used. > > - Make BACKLIGHT_CLASS_DEVICE a 'bool' symbol instead, and force the > core API code to always be built-in or completely disabled. This makes > it really easy to use, at the expense of a larger kernel image for those that > currently use a loadable module. > > Arnd > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Jani Nikula, Intel Open Source Technology Center