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@gmail.com >--- >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); >diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c >index bd6cce0..cd4c6a5 100644 >--- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c >+++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c >@@ -237,46 +237,6 @@ void tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb, > EXPORT_SYMBOL(tinydrm_xrgb8888_to_gray8); > /** >- * tinydrm_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. >- * >- * 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 *tinydrm_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(tinydrm_of_find_backlight); >- >-/** > * tinydrm_enable_backlight - Enable backlight helper > * @backlight: Backlight device > * >diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c >index 7e5bb7d..5e3d635 100644 >--- a/drivers/gpu/drm/tinydrm/mi0283qt.c >+++ b/drivers/gpu/drm/tinydrm/mi0283qt.c >@@ -12,6 +12,7 @@ > #include <drm/tinydrm/ili9341.h> > #include <drm/tinydrm/mipi-dbi.h> > #include <drm/tinydrm/tinydrm-helpers.h> >+#include <drm/drm_of.h> > #include <linux/delay.h> > #include <linux/gpio/consumer.h> > #include <linux/module.h> >@@ -189,7 +190,7 @@ static int mi0283qt_probe(struct spi_device *spi) > if (IS_ERR(mipi->regulator)) > return PTR_ERR(mipi->regulator); >- mipi->backlight = tinydrm_of_find_backlight(dev); >+ mipi->backlight = drm_of_find_backlight(dev); > if (IS_ERR(mipi->backlight)) > return PTR_ERR(mipi->backlight); >diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h >index 104dd51..e8fba5b 100644 >--- a/include/drm/drm_of.h >+++ b/include/drm/drm_of.h >@@ -29,6 +29,7 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, > int port, int endpoint, > struct drm_panel **panel, > struct drm_bridge **bridge); >+struct backlight_device *drm_of_find_backlight(struct device *dev); > #else > static inline uint32_t drm_of_find_possible_crtcs(struct drm_device *dev, > struct device_node *port) Minor detail I missed: The dummy version for the #else here is missing. Not a problem for tinydrm, but might be an issue for a driver where CONFIG_OF is optional.
Yeah, we need a dummy version. tinydrm can in theory be used on non-DT platforms. The only resource that isn't available there is backlight since there is no platform or ACPI way of describing that dependency.
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/8312a3fe84b96e30a010fa20f933606d976...
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
Regards, Meghana
Regards, Meghana
We also have another problem: CONFIG_OF=y && CONFIG_LCD_CLASS_DEVICE=n That will result in a missing symbol error.
You can try this change to include/linux/backlight.h:
-#ifdef CONFIG_OF +#if defined(CONFIG_OF) && IS_ENABLED(CONFIG_LCD_CLASS_DEVICE) struct backlight_device *of_find_backlight_by_node(struct device_node *node); #else static inline struct backlight_device * of_find_backlight_by_node(struct device_node *node) { return NULL; } #endif
This will need to be a patch of its own since it's another subsystem.
You have to try the various kconfig variations to be sure, I'm no kconfig expert. CONFIG_OF=y/n, CONFIG_DRM=y/m, CONFIG_LCD_CLASS_DEVICE=y/m/n
Noralf.
Sure will try this.
Another way of solving this is to put the function in drivers/gpu/drm/drm_backlight.c and add a DRM_BACKLIGHT konfig option that does the selection.
Noralf.
Otherwise I think this patch looks good. -Daniel
>diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h >index d554ded..e40ef2d 100644 >--- a/include/drm/tinydrm/tinydrm-helpers.h >+++ b/include/drm/tinydrm/tinydrm-helpers.h >@@ -46,7 +46,6 @@ void tinydrm_xrgb8888_to_rgb565(u16 *dst, void *vaddr, > void tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb, > struct drm_clip_rect *clip); >-struct backlight_device *tinydrm_of_find_backlight(struct device *dev); > int tinydrm_enable_backlight(struct backlight_device *backlight); > int tinydrm_disable_backlight(struct backlight_device *backlight); >-- >2.7.4 >