Re: [PATCH v3 1/2] drm/tinydrm: Move tinydrm_of_find_backlight into drm_of.c

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Sep 29, 2017 at 05:41:04PM +0200, Noralf Trønnes wrote:
> 
> Den 29.09.2017 16.13, skrev Meghana Madhyastha:
> >On Fri, Sep 29, 2017 at 02:33:12PM +0200, Noralf Trønnes wrote:
> >>Den 29.09.2017 14.20, skrev Meghana Madhyastha:
> >>>On Fri, Sep 29, 2017 at 02:10:31PM +0200, Noralf Trønnes wrote:
> >>>>Den 29.09.2017 05.22, skrev Meghana Madhyastha:
> >>>>>On Thu, Sep 28, 2017 at 06:02:27PM +0200, Noralf Trønnes wrote:
> >>>>>>Den 28.09.2017 16.08, skrev Daniel Vetter:
> >>>>>>>On Thu, Sep 28, 2017 at 02:44:34PM +0530, Meghana Madhyastha wrote:
> >>>>>>>>Rename tinydrm_of_find_backlight to drm_of_find_backlight
> >>>>>>>>and move it into drm_of.c from tinydrm-helpers.c. This is
> >>>>>>>>because other drivers in the drm subsystem might need to call
> >>>>>>>>this function. In that case and otherwise, it is better from
> >>>>>>>>an organizational point of view to move it into drm_of.c along
> >>>>>>>>with the other _of.c functions.
> >>>>>>>>
> >>>>>>>>Signed-off-by: Meghana Madhyastha <meghana.madhyastha@xxxxxxxxx>
> >>>>>>>>---
> >>>>>>>>Changes in v3:
> >>>>>>>>-Change it back to a single patch from two patches in v2
> >>>>>>>>
> >>>>>>>>  drivers/gpu/drm/drm_of.c                       | 44 ++++++++++++++++++++++++++
> >>>>>>>>  drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 40 -----------------------
> >>>>>>>>  drivers/gpu/drm/tinydrm/mi0283qt.c             |  3 +-
> >>>>>>>>  include/drm/drm_of.h                           |  1 +
> >>>>>>>>  include/drm/tinydrm/tinydrm-helpers.h          |  1 -
> >>>>>>>>  5 files changed, 47 insertions(+), 42 deletions(-)
> >>>>>>>>
> >>>>>>>>diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> >>>>>>>>index 8dafbdf..d878d3a 100644
> >>>>>>>>--- a/drivers/gpu/drm/drm_of.c
> >>>>>>>>+++ b/drivers/gpu/drm/drm_of.c
> >>>>>>>>@@ -1,6 +1,7 @@
> >>>>>>>>  #include <linux/component.h>
> >>>>>>>>  #include <linux/export.h>
> >>>>>>>>  #include <linux/list.h>
> >>>>>>>>+#include <linux/backlight.h>
> >>>>>>>>  #include <linux/of_graph.h>
> >>>>>>>>  #include <drm/drmP.h>
> >>>>>>>>  #include <drm/drm_bridge.h>
> >>>>>>>>@@ -260,3 +261,46 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> >>>>>>>>  	return ret;
> >>>>>>>>  }
> >>>>>>>>  EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge);
> >>>>>>>>+
> >>>>>>>>+/**
> >>>>>>>>+ * drm_of_find_backlight - Find backlight device in device-tree
> >>>>>>>>+ * @dev: Device
> >>>>>>>>+ *
> >>>>>>>>+ * This function looks for a DT node pointed to by a property named 'backlight'
> >>>>>>>>+ * and uses of_find_backlight_by_node() to get the backlight device.
> >>>>>>>>+ * Additionally if the brightness property is zero, it is set to
> >>>>>>>>+ * max_brightness.
> >>>>>>>>+ *
> >>>>>>>>+ * Note: It is the responsibility of the caller to call put_device() when
> >>>>>>>>+ * releasing the resource.
> >>>>>>>>+ *
> >>>>>>>>+ * Returns:
> >>>>>>>>+ * NULL if there's no backlight property.
> >>>>>>>>+ * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
> >>>>>>>>+ * is found.
> >>>>>>>>+ * If the backlight device is found, a pointer to the structure is returned.
> >>>>>>>>+ */
> >>>>>>>>+struct backlight_device *drm_of_find_backlight(struct device *dev)
> >>>>>>>>+{
> >>>>>>>>+	struct backlight_device *backlight;
> >>>>>>>>+	struct device_node *np;
> >>>>>>>>+
> >>>>>>>>+	np = of_parse_phandle(dev->of_node, "backlight", 0);
> >>>>>>>>+	if (!np)
> >>>>>>>>+		return NULL;
> >>>>>>>>+
> >>>>>>>>+	backlight = of_find_backlight_by_node(np);
> >>>>>>>>+	of_node_put(np);
> >>>>>>>>+
> >>>>>>>>+	if (!backlight)
> >>>>>>>>+		return ERR_PTR(-EPROBE_DEFER);
> >>>>>>>>+
> >>>>>>>>+	if (!backlight->props.brightness) {
> >>>>>>>>+		backlight->props.brightness = backlight->props.max_brightness;
> >>>>>>>>+		DRM_DEBUG_KMS("Backlight brightness set to %d\n",
> >>>>>>>>+			      backlight->props.brightness);
> >>>>>>>>+	}
> >>>>>>>>+
> >>>>>>>>+	return backlight;
> >>>>>>>>+}
> >>>>>>>>+EXPORT_SYMBOL(drm_of_find_backlight);
> 
> <snip>
> 
> >>>>>>Another problem I discovered when trying to compile this, is that if
> >>>>>>DRM is builtin and LCD_CLASS_DEVICE is a module:
> >>>>>>
> >>>>>>drivers/gpu/drm/drm_of.c:292: undefined reference to
> >>>>>>`of_find_backlight_by_node'
> >>>>>>
> >>>>>>I see that I solved this in tinydrm by just selecting
> >>>>>>BACKLIGHT_LCD_SUPPORT and LCD_CLASS_DEVICE. I'm not aware of a way to
> >>>>>>force LCD_CLASS_DEVICE to be builtin if DRM is builtin.
> >>>>>>
> >>>>>>Arnd fixed a similar type of dependecy in the repaper driver by forcing
> >>>>>>repaper to be a module if necessary. To be honest, I don't know why his
> >>>>>>fix works:
> >>>>>>https://github.com/torvalds/linux/commit/8312a3fe84b96e30a010fa20f933606d976ac002
> >>>>>I didn't quite understand how we could use this fix for the current
> >>>>>case. Isn't this specific to the repaper driver dependency ?
> >>>>This is the solution Daniel mentions:
> >>>>
> >>>>  menuconfig DRM
> >>>>      tristate "Direct Rendering Manager (XFree86 4.1.0 and higher DRI
> >>>>support)"
> >>>>      depends on (AGP || AGP=n) && !EMULATED_CMPXCHG && HAS_DMA
> >>>>+   depends on (LCD_CLASS_DEVICE || LCD_CLASS_DEVICE=n)
> >>>I had tried this but it resulted in a recursive dependencies error.
> >>What's the error?
> >>Could this have something to do with tinydrm selecting backlight and
> >>depending on DRM?
> >>
> >>Noralf.
> >Looks like a case of circular dependencies. The exact error is this:
> >
> >scripts/kconfig/mconf  Kconfig
> >drivers/gpu/drm/Kconfig:7:error: recursive dependency detected!
> >For a resolution refer to Documentation/kbuild/kconfig-language.txt
> >subsection "Kconfig recursive dependency limitations"
> >drivers/gpu/drm/Kconfig:7:	symbol DRM depends on LCD_CLASS_DEVICE
> >For a resolution refer to Documentation/kbuild/kconfig-language.txt
> >subsection "Kconfig recursive dependency limitations"
> >drivers/video/backlight/Kconfig:16:	symbol LCD_CLASS_DEVICE is
> >selected by FB_CLPS711X
> >For a resolution refer to Documentation/kbuild/kconfig-language.txt
> >subsection "Kconfig recursive dependency limitations"
> >drivers/video/fbdev/Kconfig:338:	symbol FB_CLPS711X depends on FB
> >For a resolution refer to Documentation/kbuild/kconfig-language.txt
> >subsection "Kconfig recursive dependency limitations"
> >drivers/video/fbdev/Kconfig:5:	symbol FB is selected by
> >DRM_KMS_FB_HELPER
> >For a resolution refer to Documentation/kbuild/kconfig-language.txt
> >subsection "Kconfig recursive dependency limitations"
> >drivers/gpu/drm/Kconfig:73:	symbol DRM_KMS_FB_HELPER is selected by
> >DRM_FBDEV_EMULATION
> >For a resolution refer to Documentation/kbuild/kconfig-language.txt
> >subsection "Kconfig recursive dependency limitations"
> >drivers/gpu/drm/Kconfig:90:	symbol DRM_FBDEV_EMULATION depends on
> >DRM
> >
> >Long story short, the dependency loop is as follows:
> >
> >DRM -> LCD_CLASS_DEVICE ->  FB_CLPS711X -> FB -> DRM_KMS_FB_HELPER ->
> >DRM_FBDEV_EMULATION -> DRM
> 
> I have messed up the options here, it should be BACKLIGHT_CLASS_DEVICE:
> 
> menuconfig DRM
>     depends on (BACKLIGHT_CLASS_DEVICE || BACKLIGHT_CLASS_DEVICE=n)
> 
> It doesn't help with the recusrsive problem, but at least it makes sense:
> 
> drivers/gpu/drm/Kconfig:7:      symbol DRM depends on BACKLIGHT_CLASS_DEVICE
> drivers/video/backlight/Kconfig:158:    symbol BACKLIGHT_CLASS_DEVICE is
> selected by DRM_RADEON
> drivers/gpu/drm/Kconfig:164:    symbol DRM_RADEON depends on DRM
> 
> I don't know how to solve this...
> 
> Noralf.

Looks like select BACKLIGHT_CLASS_DEVICE and BACKLIGHT_LCD_SUPPORT
seemed to work (atleast when I replicated the configurations from the 
kbuild support). Maybe that is because we are not forcing the
dependencies when we do a select ?

Thanks and regards,
Meghana
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux