On Sun, Oct 01, 2017 at 04:47:26PM +0200, Noralf Trønnes wrote: > > Den 01.10.2017 15.34, skrev Meghana Madhyastha: > >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) > > It will work, the compiler doesn't complain as long as you include > backlight.h and make sure drm_of.c is built. But that would result > in a complexity that humans would struggle to get right. And in this > case it's even 2 different subsystems with different maintainers... > > So in order to succeed with complex projects, humans need some more > rules than what the compiler requires. And one of those are: > A header file is usually connected to a source file. > > backlight.h contains prototypes and definitions for backlight.c > drm_of.h contains prototypes and definitions for drm_of.c Okay that makes sense. Initially, I thought that we could add the declarations/definitions along with of_find_backlight_by_node in linux/backlight.h as I hadn't realized that each .c file should strictly have a one to one correspondence with the .h file of the same name but as you mentioned, it is necessary in a large/complex project like this. Thank you for the clarifications ! Regards, Meghana > > >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