Re: [PATCH v6 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 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




[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