On Sun, Oct 01, 2017 at 03:26:36PM +0200, Noralf Trønnes wrote: > > Den 01.10.2017 06.14, skrev Meghana Madhyastha: > >On Sat, Sep 30, 2017 at 09:04:57PM +0200, Noralf Trønnes wrote: > >>Den 30.09.2017 19.12, skrev Meghana Madhyastha: > >>>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 v6: > >>>-Move function declarations to linux/backlight.h to resolve > >>> module dependency issues. > >>> > >>> drivers/gpu/drm/drm_of.c | 44 ++++++++++++++++++++++++++ > >>> drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 40 ----------------------- > >>> drivers/gpu/drm/tinydrm/mi0283qt.c | 2 +- > >>> include/drm/tinydrm/tinydrm-helpers.h | 1 - > >>> include/linux/backlight.h | 11 +++++++ > >>> 5 files changed, 56 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..682b4db 100644 > >>>--- a/drivers/gpu/drm/tinydrm/mi0283qt.c > >>>+++ b/drivers/gpu/drm/tinydrm/mi0283qt.c > >>>@@ -189,7 +189,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/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); > >>>diff --git a/include/linux/backlight.h b/include/linux/backlight.h > >>>index 5f2fd61..47a095e 100644 > >>>--- a/include/linux/backlight.h > >>>+++ b/include/linux/backlight.h > >>>@@ -172,4 +172,15 @@ of_find_backlight_by_node(struct device_node *node) > >>> } > >>> #endif > >>>+#if defined(CONFIG_OF) && IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE) > >>>+struct backlight_device * > >>>+drm_of_find_backlight(struct device *dev); > >>>+#else > >>>+static inline struct backlight_device * > >>>+drm_of_find_backlight(struct device *dev) > >>>+{ > >>>+ return NULL; > >>>+} > >>>+#endif > >>>+ > >>> #endif > >>This isn't what I meant, you only change the of_find_backlight_by_node() > >>declaration/definition. It should be like this: > >> > >>#if defined(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 * > >>of_find_backlight_by_node(struct device_node *node) > >>{ > >> return NULL; > >>} > >>#endif > >I see. So basically add the function declaration of drm_of_find_backlight > >and devm_drm_of_find_backlight in drm_of.h and this function declaration > >need not be inside the #ifdef CONFIG / #else part? > >So there need not be a dummy version of drm_of_find_backlight here > >because we are anyway taking care of that in of_find_backliht by node? > >Sorry for the many versions and thank you for the clarifications. > > You need the dummy part in drm_of.h because you need to handle the > CONFIG_OF option here as well. > > If drm_of_find_backlight() was only 2-3 lines of code, you could have > made a static inline function in drm_of.h outside the #if/#else/#endif, > but since it's longer than that it should be in drm_of.c. > > CONFIG_OF decides if drm_of.c is built (as part of drm.o) or not: > > drivers/gpu/drm/Makefile: > drm-$(CONFIG_OF) += drm_of.o > > This means we need a function prototype declaration for the CONFIG_OF=y > case, and an inline function definition for the CONFIG_OF=n case. > > Noralf. I have one more question. In that case, why wouldn't my previous patch work ? (i.e definition and declaration of drm_of_find_backlight in linux/backlight.h with if/else directives) Regards, Meghana > >Regards, > >Meghana > >>And this change needs to be a patch on it's own. The backlight subsystem > >>has it's own maintainer that will review this change. > >>It's best to send him/them the whole patchset so they see the context. > >> > >>Noralf. > >> > _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel