[PATCH v7 0/3] drm/tinydrm: drm_of_find_backlight helper

[PATCH v7 0/3] drm/tinydrm: drm_of_find_backlight helper

[Meghana Madhyastha]

Move tinydrm_of_find_backlight to drm_of.c and rename it to
drm_of_find_backlight for better organizational structure.

Changes in v7:
-Move the function definitions/declarations back to drm_of.h
 and modify the config option for of_find_backlight_by_node
 in linux/backlight.h to resolve the build errors.

Meghana Madhyastha (3):
  backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
  drm/tinydrm: Move tinydrm_of_find_backlight into drm_of.c
  drm/tinydrm: Add devres versions of drm_of_find_backlight

 drivers/gpu/drm/drm_of.c                       | 85 ++++++++++++++++++++++++++
 drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 40 ------------
 drivers/gpu/drm/tinydrm/mi0283qt.c             |  3 +-
 include/drm/drm_of.h                           | 14 +++++
 include/drm/tinydrm/tinydrm-helpers.h          |  1 -
 include/linux/backlight.h                      |  2 +-
 6 files changed, 102 insertions(+), 43 deletions(-)

-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v7 0/3] drm/tinydrm: drm_of_find_backlight helper

[Meghana Madhyastha]

Move tinydrm_of_find_backlight to drm_of.c and rename it to
drm_of_find_backlight for better organizational structure.

Changes in v7:
-Move the function definitions/declarations back to drm_of.h
 and modify the config option for of_find_backlight_by_node
 in linux/backlight.h to resolve the build errors.

Meghana Madhyastha (3):
  backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
  drm/tinydrm: Move tinydrm_of_find_backlight into drm_of.c
  drm/tinydrm: Add devres versions of drm_of_find_backlight

 drivers/gpu/drm/drm_of.c                       | 85 ++++++++++++++++++++++++++
 drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 40 ------------
 drivers/gpu/drm/tinydrm/mi0283qt.c             |  3 +-
 include/drm/drm_of.h                           | 14 +++++
 include/drm/tinydrm/tinydrm-helpers.h          |  1 -
 include/linux/backlight.h                      |  2 +-
 6 files changed, 102 insertions(+), 43 deletions(-)

-- 
2.7.4

[PATCH v7 1/3] backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)

[Meghana Madhyastha]

Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE) as part of the
if directive for the function declaration of
of_find_backlight_by_node in order to avoid module dependency
errors.

Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
---
Changes in v7:
-This patch did not exist in v6. 

 include/linux/backlight.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index 5f2fd61..a52ce82 100644
--- a/include/linux/backlight.h
+++ b/include/linux/backlight.h
@@ -162,7 +162,7 @@ struct generic_bl_info {
 	void (*kick_battery)(void);
 };
 
-#ifdef CONFIG_OF
+#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 *
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v7 1/3] backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)

[Meghana Madhyastha]

Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE) as part of the
if directive for the function declaration of
of_find_backlight_by_node in order to avoid module dependency
errors.

Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
---
Changes in v7:
-This patch did not exist in v6. 

 include/linux/backlight.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index 5f2fd61..a52ce82 100644
--- a/include/linux/backlight.h
+++ b/include/linux/backlight.h
@@ -162,7 +162,7 @@ struct generic_bl_info {
 	void (*kick_battery)(void);
 };
 
-#ifdef CONFIG_OF
+#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 *
-- 
2.7.4

[PATCH v7 2/3] drm/tinydrm: Move tinydrm_of_find_backlight into drm_of.c

[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@gmail.com>
---
Changes in v7:
-Move function declarations back to drm_of.h

 drivers/gpu/drm/drm_of.c                       | 45 ++++++++++++++++++++++++++
 drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 40 -----------------------
 drivers/gpu/drm/tinydrm/mi0283qt.c             |  3 +-
 include/drm/drm_of.h                           |  7 ++++
 include/drm/tinydrm/tinydrm-helpers.h          |  1 -
 5 files changed, 54 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
index 8dafbdf..7417725 100644
--- a/drivers/gpu/drm/drm_of.c
+++ b/drivers/gpu/drm/drm_of.c
@@ -1,7 +1,9 @@
 #include <linux/component.h>
 #include <linux/export.h>
 #include <linux/list.h>
+#include <linux/backlight.h>
 #include <linux/of_graph.h>
+#include <drm/drm_of.h>
 #include <drm/drmP.h>
 #include <drm/drm_bridge.h>
 #include <drm/drm_crtc.h>
@@ -260,3 +262,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..6e93311 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)
@@ -65,6 +66,12 @@ static inline int drm_of_find_panel_or_bridge(const struct device_node *np,
 {
 	return -EINVAL;
 }
+
+static inline struct backlight_device *
+drm_of_find_backlight(struct device *dev)
+{
+	return NULL;
+}
 #endif
 
 static inline int drm_of_encoder_active_endpoint_id(struct device_node *node,
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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v7 2/3] drm/tinydrm: Move tinydrm_of_find_backlight into drm_of.c

[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@gmail.com>
---
Changes in v7:
-Move function declarations back to drm_of.h

 drivers/gpu/drm/drm_of.c                       | 45 ++++++++++++++++++++++++++
 drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 40 -----------------------
 drivers/gpu/drm/tinydrm/mi0283qt.c             |  3 +-
 include/drm/drm_of.h                           |  7 ++++
 include/drm/tinydrm/tinydrm-helpers.h          |  1 -
 5 files changed, 54 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
index 8dafbdf..7417725 100644
--- a/drivers/gpu/drm/drm_of.c
+++ b/drivers/gpu/drm/drm_of.c
@@ -1,7 +1,9 @@
 #include <linux/component.h>
 #include <linux/export.h>
 #include <linux/list.h>
+#include <linux/backlight.h>
 #include <linux/of_graph.h>
+#include <drm/drm_of.h>
 #include <drm/drmP.h>
 #include <drm/drm_bridge.h>
 #include <drm/drm_crtc.h>
@@ -260,3 +262,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..6e93311 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)
@@ -65,6 +66,12 @@ static inline int drm_of_find_panel_or_bridge(const struct device_node *np,
 {
 	return -EINVAL;
 }
+
+static inline struct backlight_device *
+drm_of_find_backlight(struct device *dev)
+{
+	return NULL;
+}
 #endif
 
 static inline int drm_of_encoder_active_endpoint_id(struct device_node *node,
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

[PATCH v7 3/3] drm/tinydrm: Add devres versions of drm_of_find_backlight

[Meghana Madhyastha]

Add devm_drm_of_find_backlight and the corresponding release
function because some drivers use devres versions of functions
for requiring device resources.

Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
---
Changes in v7:
-None

 drivers/gpu/drm/drm_of.c           | 40 ++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/tinydrm/mi0283qt.c |  2 +-
 include/drm/drm_of.h               |  7 +++++++
 3 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
index 7417725..82b5113 100644
--- a/drivers/gpu/drm/drm_of.c
+++ b/drivers/gpu/drm/drm_of.c
@@ -305,3 +305,43 @@ struct backlight_device *drm_of_find_backlight(struct device *dev)
 	return backlight;
 }
 EXPORT_SYMBOL(drm_of_find_backlight);
+
+static void devm_drm_of_find_backlight_release(void *data)
+{
+	put_device(data);
+}
+
+/**
+ * devm_drm_of_find_backlight - Find backlight device in device-tree
+ * devres version of the function
+ * @dev: Device
+ *
+ * This is the devres version of the function drm_of_find_backlight.
+ * Some drivers use devres versions of functions for
+ * requiring device resources.
+ *
+ * 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 *devm_drm_of_find_backlight(struct device *dev)
+{
+	struct backlight_device *backlight;
+	int ret;
+
+	backlight = drm_of_find_backlight(dev);
+	if (IS_ERR_OR_NULL(backlight))
+		return backlight;
+
+	ret = devm_add_action(dev, devm_drm_of_find_backlight_release,
+			      &backlight->dev);
+	if (ret) {
+		put_device(&backlight->dev);
+		return ERR_PTR(ret);
+	}
+
+	return backlight;
+}
+EXPORT_SYMBOL(devm_drm_of_find_backlight);
diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
index 5e3d635..d37f658 100644
--- a/drivers/gpu/drm/tinydrm/mi0283qt.c
+++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
@@ -190,7 +190,7 @@ static int mi0283qt_probe(struct spi_device *spi)
 	if (IS_ERR(mipi->regulator))
 		return PTR_ERR(mipi->regulator);
 
-	mipi->backlight = drm_of_find_backlight(dev);
+	mipi->backlight = devm_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 6e93311..b95f3fb 100644
--- a/include/drm/drm_of.h
+++ b/include/drm/drm_of.h
@@ -30,6 +30,7 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
 				struct drm_panel **panel,
 				struct drm_bridge **bridge);
 struct backlight_device *drm_of_find_backlight(struct device *dev);
+struct backlight_device *devm_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)
@@ -72,6 +73,12 @@ drm_of_find_backlight(struct device *dev)
 {
 	return NULL;
 }
+
+static inline struct backlight_device *
+devm_drm_of_find_backlight(struct device *dev)
+{
+	return NULL;
+}
 #endif
 
 static inline int drm_of_encoder_active_endpoint_id(struct device_node *node,
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v7 3/3] drm/tinydrm: Add devres versions of drm_of_find_backlight

[Meghana Madhyastha]

Add devm_drm_of_find_backlight and the corresponding release
function because some drivers use devres versions of functions
for requiring device resources.

Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
---
Changes in v7:
-None

 drivers/gpu/drm/drm_of.c           | 40 ++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/tinydrm/mi0283qt.c |  2 +-
 include/drm/drm_of.h               |  7 +++++++
 3 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
index 7417725..82b5113 100644
--- a/drivers/gpu/drm/drm_of.c
+++ b/drivers/gpu/drm/drm_of.c
@@ -305,3 +305,43 @@ struct backlight_device *drm_of_find_backlight(struct device *dev)
 	return backlight;
 }
 EXPORT_SYMBOL(drm_of_find_backlight);
+
+static void devm_drm_of_find_backlight_release(void *data)
+{
+	put_device(data);
+}
+
+/**
+ * devm_drm_of_find_backlight - Find backlight device in device-tree
+ * devres version of the function
+ * @dev: Device
+ *
+ * This is the devres version of the function drm_of_find_backlight.
+ * Some drivers use devres versions of functions for
+ * requiring device resources.
+ *
+ * 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 *devm_drm_of_find_backlight(struct device *dev)
+{
+	struct backlight_device *backlight;
+	int ret;
+
+	backlight = drm_of_find_backlight(dev);
+	if (IS_ERR_OR_NULL(backlight))
+		return backlight;
+
+	ret = devm_add_action(dev, devm_drm_of_find_backlight_release,
+			      &backlight->dev);
+	if (ret) {
+		put_device(&backlight->dev);
+		return ERR_PTR(ret);
+	}
+
+	return backlight;
+}
+EXPORT_SYMBOL(devm_drm_of_find_backlight);
diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
index 5e3d635..d37f658 100644
--- a/drivers/gpu/drm/tinydrm/mi0283qt.c
+++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
@@ -190,7 +190,7 @@ static int mi0283qt_probe(struct spi_device *spi)
 	if (IS_ERR(mipi->regulator))
 		return PTR_ERR(mipi->regulator);
 
-	mipi->backlight = drm_of_find_backlight(dev);
+	mipi->backlight = devm_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 6e93311..b95f3fb 100644
--- a/include/drm/drm_of.h
+++ b/include/drm/drm_of.h
@@ -30,6 +30,7 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
 				struct drm_panel **panel,
 				struct drm_bridge **bridge);
 struct backlight_device *drm_of_find_backlight(struct device *dev);
+struct backlight_device *devm_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)
@@ -72,6 +73,12 @@ drm_of_find_backlight(struct device *dev)
 {
 	return NULL;
 }
+
+static inline struct backlight_device *
+devm_drm_of_find_backlight(struct device *dev)
+{
+	return NULL;
+}
 #endif
 
 static inline int drm_of_encoder_active_endpoint_id(struct device_node *node,
-- 
2.7.4

Re: [PATCH v7 1/3] backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)

[Daniel Thompson]

On 01/10/17 18:26, Meghana Madhyastha wrote:
> Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE) as part of the
> if directive for the function declaration of
> of_find_backlight_by_node in order to avoid module dependency
> errors.

Module dependency errors? Does you mean mean use of undefined symbols?


> Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
> ---
> Changes in v7:
> -This patch did not exist in v6.

So I'm coming to this patchset cold but can you explain *why* something 
wants to call of_find_backlight_by_node() when there is no backlight 
support enabled. Why isn't the code that called is conditional on 
BACKLIGHT_CLASS_DEVICE?

The undefined symbol issue is a pain but to be honest I'd rather solve 
the use of undefined symbols by avoiding declaring them; this making 
them into compile errors rather than link errors.


>   include/linux/backlight.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> index 5f2fd61..a52ce82 100644
> --- a/include/linux/backlight.h
> +++ b/include/linux/backlight.h
> @@ -162,7 +162,7 @@ struct generic_bl_info {
>   	void (*kick_battery)(void);
>   };
>   
> -#ifdef CONFIG_OF
> +#if defined CONFIG_OF && IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)

The above comments are more important but why does this mix defined and 
IS_ENABLED? Couldn't they both use defined (and preferably with the 
optional brackets around the CONFIG_ symbol).


Daniel.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v7 1/3] backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)

[Daniel Thompson]

On 01/10/17 18:26, Meghana Madhyastha wrote:
> Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE) as part of the
> if directive for the function declaration of
> of_find_backlight_by_node in order to avoid module dependency
> errors.

Module dependency errors? Does you mean mean use of undefined symbols?


> Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
> ---
> Changes in v7:
> -This patch did not exist in v6.

So I'm coming to this patchset cold but can you explain *why* something 
wants to call of_find_backlight_by_node() when there is no backlight 
support enabled. Why isn't the code that called is conditional on 
BACKLIGHT_CLASS_DEVICE?

The undefined symbol issue is a pain but to be honest I'd rather solve 
the use of undefined symbols by avoiding declaring them; this making 
them into compile errors rather than link errors.


>   include/linux/backlight.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> index 5f2fd61..a52ce82 100644
> --- a/include/linux/backlight.h
> +++ b/include/linux/backlight.h
> @@ -162,7 +162,7 @@ struct generic_bl_info {
>   	void (*kick_battery)(void);
>   };
>   
> -#ifdef CONFIG_OF
> +#if defined CONFIG_OF && IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)

The above comments are more important but why does this mix defined and 
IS_ENABLED? Couldn't they both use defined (and preferably with the 
optional brackets around the CONFIG_ symbol).


Daniel.

Re: [PATCH v7 1/3] backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)

[Daniel Thompson]

On 02/10/17 06:58, Daniel Thompson wrote:
> On 01/10/17 18:26, Meghana Madhyastha wrote:
>> Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE) as part of the
>> if directive for the function declaration of
>> of_find_backlight_by_node in order to avoid module dependency
>> errors.
> 
> Module dependency errors? Does you mean mean use of undefined symbols?

Sorry, drafting error! Could you pretend I wrote "Do you mean use of..." 
instead of the nonsense above!

> 
> 
>> Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
>> ---
>> Changes in v7:
>> -This patch did not exist in v6.
> 
> So I'm coming to this patchset cold but can you explain *why* something 
> wants to call of_find_backlight_by_node() when there is no backlight 
> support enabled. Why isn't the code that called is conditional on 
> BACKLIGHT_CLASS_DEVICE?
> 
> The undefined symbol issue is a pain but to be honest I'd rather solve 
> the use of undefined symbols by avoiding declaring them; this making 
> them into compile errors rather than link errors.
> 
> 
>>   include/linux/backlight.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
>> index 5f2fd61..a52ce82 100644
>> --- a/include/linux/backlight.h
>> +++ b/include/linux/backlight.h
>> @@ -162,7 +162,7 @@ struct generic_bl_info {
>>       void (*kick_battery)(void);
>>   };
>> -#ifdef CONFIG_OF
>> +#if defined CONFIG_OF && IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
> 
> The above comments are more important but why does this mix defined and 
> IS_ENABLED? Couldn't they both use defined (and preferably with the 
> optional brackets around the CONFIG_ symbol).
> 
> 
> Daniel.

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v7 1/3] backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)

[Daniel Thompson]

On 02/10/17 06:58, Daniel Thompson wrote:
> On 01/10/17 18:26, Meghana Madhyastha wrote:
>> Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE) as part of the
>> if directive for the function declaration of
>> of_find_backlight_by_node in order to avoid module dependency
>> errors.
> 
> Module dependency errors? Does you mean mean use of undefined symbols?

Sorry, drafting error! Could you pretend I wrote "Do you mean use of..." 
instead of the nonsense above!

> 
> 
>> Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
>> ---
>> Changes in v7:
>> -This patch did not exist in v6.
> 
> So I'm coming to this patchset cold but can you explain *why* something 
> wants to call of_find_backlight_by_node() when there is no backlight 
> support enabled. Why isn't the code that called is conditional on 
> BACKLIGHT_CLASS_DEVICE?
> 
> The undefined symbol issue is a pain but to be honest I'd rather solve 
> the use of undefined symbols by avoiding declaring them; this making 
> them into compile errors rather than link errors.
> 
> 
>>   include/linux/backlight.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
>> index 5f2fd61..a52ce82 100644
>> --- a/include/linux/backlight.h
>> +++ b/include/linux/backlight.h
>> @@ -162,7 +162,7 @@ struct generic_bl_info {
>>       void (*kick_battery)(void);
>>   };
>> -#ifdef CONFIG_OF
>> +#if defined CONFIG_OF && IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
> 
> The above comments are more important but why does this mix defined and 
> IS_ENABLED? Couldn't they both use defined (and preferably with the 
> optional brackets around the CONFIG_ symbol).
> 
> 
> Daniel.

Re: [PATCH v7 1/3] backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)

[Jani Nikula]

On Mon, 02 Oct 2017, Daniel Thompson <daniel.thompson@linaro.org> wrote:
> On 01/10/17 18:26, Meghana Madhyastha wrote:
>> -#ifdef CONFIG_OF
>> +#if defined CONFIG_OF && IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
>
> The above comments are more important but why does this mix defined and 
> IS_ENABLED? Couldn't they both use defined (and preferably with the 
> optional brackets around the CONFIG_ symbol).

No, always use IS_ENABLED() for tristates when you want to match 'y' or
'm'.

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v7 1/3] backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)

[Jani Nikula]

On Mon, 02 Oct 2017, Daniel Thompson <daniel.thompson@linaro.org> wrote:
> On 01/10/17 18:26, Meghana Madhyastha wrote:
>> -#ifdef CONFIG_OF
>> +#if defined CONFIG_OF && IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
>
> The above comments are more important but why does this mix defined and 
> IS_ENABLED? Couldn't they both use defined (and preferably with the 
> optional brackets around the CONFIG_ symbol).

No, always use IS_ENABLED() for tristates when you want to match 'y' or
'm'.

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH v7 1/3] backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)

[Jani Nikula]

On Mon, 02 Oct 2017, Daniel Thompson <daniel.thompson@linaro.org> wrote:
> So I'm coming to this patchset cold but can you explain *why* something 
> wants to call of_find_backlight_by_node() when there is no backlight 
> support enabled. Why isn't the code that called is conditional on 
> BACKLIGHT_CLASS_DEVICE?
>
> The undefined symbol issue is a pain but to be honest I'd rather solve 
> the use of undefined symbols by avoiding declaring them; this making 
> them into compile errors rather than link errors.

Typically the kernel header files define static inline stubs of the
functions when the actual functions aren't configured/built. The code
using the functions looks the same regardless of the config option, and
handles the -ENODEV or NULL or whatever returns from the stubs
gracefully. With the inlines, the compiler can usually throw out much of
the code anyway.

In this regard, the backlight interface is an exception, forcing the
callers to wrap the code around IS_ENABLED(BACKLIGHT_CLASS_DEVICE), not
the rule.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v7 1/3] backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)

[Jani Nikula]

On Mon, 02 Oct 2017, Daniel Thompson <daniel.thompson@linaro.org> wrote:
> So I'm coming to this patchset cold but can you explain *why* something 
> wants to call of_find_backlight_by_node() when there is no backlight 
> support enabled. Why isn't the code that called is conditional on 
> BACKLIGHT_CLASS_DEVICE?
>
> The undefined symbol issue is a pain but to be honest I'd rather solve 
> the use of undefined symbols by avoiding declaring them; this making 
> them into compile errors rather than link errors.

Typically the kernel header files define static inline stubs of the
functions when the actual functions aren't configured/built. The code
using the functions looks the same regardless of the config option, and
handles the -ENODEV or NULL or whatever returns from the stubs
gracefully. With the inlines, the compiler can usually throw out much of
the code anyway.

In this regard, the backlight interface is an exception, forcing the
callers to wrap the code around IS_ENABLED(BACKLIGHT_CLASS_DEVICE), not
the rule.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH v7 1/3] backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)

[Daniel Thompson]

On 02/10/17 09:49, Jani Nikula wrote:
> On Mon, 02 Oct 2017, Daniel Thompson <daniel.thompson@linaro.org> wrote:
>> On 01/10/17 18:26, Meghana Madhyastha wrote:
>>> -#ifdef CONFIG_OF
>>> +#if defined CONFIG_OF && IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
>>
>> The above comments are more important but why does this mix defined and
>> IS_ENABLED? Couldn't they both use defined (and preferably with the
>> optional brackets around the CONFIG_ symbol).
> 
> No, always use IS_ENABLED() for tristates when you want to match 'y' or
> 'm'.

Oops.

Actually it was the mis-match in the original code that attracted my 
attention (defined on one side, IS_ENABLED() on the other)... I'd be 
happier if both used the same approach.


Daniel.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v7 1/3] backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)

[Daniel Thompson]

On 02/10/17 09:49, Jani Nikula wrote:
> On Mon, 02 Oct 2017, Daniel Thompson <daniel.thompson@linaro.org> wrote:
>> On 01/10/17 18:26, Meghana Madhyastha wrote:
>>> -#ifdef CONFIG_OF
>>> +#if defined CONFIG_OF && IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
>>
>> The above comments are more important but why does this mix defined and
>> IS_ENABLED? Couldn't they both use defined (and preferably with the
>> optional brackets around the CONFIG_ symbol).
> 
> No, always use IS_ENABLED() for tristates when you want to match 'y' or
> 'm'.

Oops.

Actually it was the mis-match in the original code that attracted my 
attention (defined on one side, IS_ENABLED() on the other)... I'd be 
happier if both used the same approach.


Daniel.

Re: [PATCH v7 1/3] backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)

[Jani Nikula]

On Mon, 02 Oct 2017, Daniel Thompson <daniel.thompson@linaro.org> wrote:
> On 02/10/17 09:49, Jani Nikula wrote:
>> On Mon, 02 Oct 2017, Daniel Thompson <daniel.thompson@linaro.org> wrote:
>>> On 01/10/17 18:26, Meghana Madhyastha wrote:
>>>> -#ifdef CONFIG_OF
>>>> +#if defined CONFIG_OF && IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
>>>
>>> The above comments are more important but why does this mix defined and
>>> IS_ENABLED? Couldn't they both use defined (and preferably with the
>>> optional brackets around the CONFIG_ symbol).
>> 
>> No, always use IS_ENABLED() for tristates when you want to match 'y' or
>> 'm'.
>
> Oops.
>
> Actually it was the mis-match in the original code that attracted my 
> attention (defined on one side, IS_ENABLED() on the other)... I'd be 
> happier if both used the same approach.

Agreed.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v7 1/3] backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)

[Jani Nikula]

On Mon, 02 Oct 2017, Daniel Thompson <daniel.thompson@linaro.org> wrote:
> On 02/10/17 09:49, Jani Nikula wrote:
>> On Mon, 02 Oct 2017, Daniel Thompson <daniel.thompson@linaro.org> wrote:
>>> On 01/10/17 18:26, Meghana Madhyastha wrote:
>>>> -#ifdef CONFIG_OF
>>>> +#if defined CONFIG_OF && IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
>>>
>>> The above comments are more important but why does this mix defined and
>>> IS_ENABLED? Couldn't they both use defined (and preferably with the
>>> optional brackets around the CONFIG_ symbol).
>> 
>> No, always use IS_ENABLED() for tristates when you want to match 'y' or
>> 'm'.
>
> Oops.
>
> Actually it was the mis-match in the original code that attracted my 
> attention (defined on one side, IS_ENABLED() on the other)... I'd be 
> happier if both used the same approach.

Agreed.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH v7 1/3] backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)

[Daniel Vetter]

On Mon, Oct 02, 2017 at 12:00:54PM +0300, Jani Nikula wrote:
> On Mon, 02 Oct 2017, Daniel Thompson <daniel.thompson@linaro.org> wrote:
> > So I'm coming to this patchset cold but can you explain *why* something 
> > wants to call of_find_backlight_by_node() when there is no backlight 
> > support enabled. Why isn't the code that called is conditional on 
> > BACKLIGHT_CLASS_DEVICE?
> >
> > The undefined symbol issue is a pain but to be honest I'd rather solve 
> > the use of undefined symbols by avoiding declaring them; this making 
> > them into compile errors rather than link errors.
> 
> Typically the kernel header files define static inline stubs of the
> functions when the actual functions aren't configured/built. The code
> using the functions looks the same regardless of the config option, and
> handles the -ENODEV or NULL or whatever returns from the stubs
> gracefully. With the inlines, the compiler can usually throw out much of
> the code anyway.
> 
> In this regard, the backlight interface is an exception, forcing the
> callers to wrap the code around IS_ENABLED(BACKLIGHT_CLASS_DEVICE), not
> the rule.

fwiw, I think it'd be great if we can move backlight over to the common
pattern, like everyone else. It's pretty big pain as-is ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v7 1/3] backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)

[Daniel Vetter]

On Mon, Oct 02, 2017 at 12:00:54PM +0300, Jani Nikula wrote:
> On Mon, 02 Oct 2017, Daniel Thompson <daniel.thompson@linaro.org> wrote:
> > So I'm coming to this patchset cold but can you explain *why* something 
> > wants to call of_find_backlight_by_node() when there is no backlight 
> > support enabled. Why isn't the code that called is conditional on 
> > BACKLIGHT_CLASS_DEVICE?
> >
> > The undefined symbol issue is a pain but to be honest I'd rather solve 
> > the use of undefined symbols by avoiding declaring them; this making 
> > them into compile errors rather than link errors.
> 
> Typically the kernel header files define static inline stubs of the
> functions when the actual functions aren't configured/built. The code
> using the functions looks the same regardless of the config option, and
> handles the -ENODEV or NULL or whatever returns from the stubs
> gracefully. With the inlines, the compiler can usually throw out much of
> the code anyway.
> 
> In this regard, the backlight interface is an exception, forcing the
> callers to wrap the code around IS_ENABLED(BACKLIGHT_CLASS_DEVICE), not
> the rule.

fwiw, I think it'd be great if we can move backlight over to the common
pattern, like everyone else. It's pretty big pain as-is ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v7 1/3] backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)

[Daniel Thompson]

On 03/10/17 09:03, Daniel Vetter wrote:
> On Mon, Oct 02, 2017 at 12:00:54PM +0300, Jani Nikula wrote:
>> On Mon, 02 Oct 2017, Daniel Thompson <daniel.thompson@linaro.org> wrote:
>>> So I'm coming to this patchset cold but can you explain *why* something
>>> wants to call of_find_backlight_by_node() when there is no backlight
>>> support enabled. Why isn't the code that called is conditional on
>>> BACKLIGHT_CLASS_DEVICE?
>>>
>>> The undefined symbol issue is a pain but to be honest I'd rather solve
>>> the use of undefined symbols by avoiding declaring them; this making
>>> them into compile errors rather than link errors.
>>
>> Typically the kernel header files define static inline stubs of the
>> functions when the actual functions aren't configured/built. The code
>> using the functions looks the same regardless of the config option, and
>> handles the -ENODEV or NULL or whatever returns from the stubs
>> gracefully. With the inlines, the compiler can usually throw out much of
>> the code anyway.
>>
>> In this regard, the backlight interface is an exception, forcing the
>> callers to wrap the code around IS_ENABLED(BACKLIGHT_CLASS_DEVICE), not
>> the rule.
> 
> fwiw, I think it'd be great if we can move backlight over to the common
> pattern, like everyone else. It's pretty big pain as-is ...

For sure, after Jani's mail yesterday I looked at the GMA500 driver and 
concluded I didn't want code related to backlight having to look like that!

Currently the three main patterns of use are:

  1. Standalone drivers simple depend on BACKLIGHT_CLASS_DEVICE,
  2. Some compound drivers select BACKLIGHT_CLASS_DEVICE (the AMD DRM
     driver is an example of this),
  3. Other compound drivers perform heroics with the pre-processor.

The main reason I'm not just agreeing straight away is that, to adopt 
the static inline pattern for the whole API, we have to believe that #3 
above is desirable. Given the size and scope of the compound drivers in 
#3 above, I don't entirely understand why they want to avoid the select.


Daniel.


PS Whatever the outcome here, I do agree 100% that is wrong to prototype
    undefined symbols. That must be changed!
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v7 1/3] backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)

[Daniel Thompson]

On 03/10/17 09:03, Daniel Vetter wrote:
> On Mon, Oct 02, 2017 at 12:00:54PM +0300, Jani Nikula wrote:
>> On Mon, 02 Oct 2017, Daniel Thompson <daniel.thompson@linaro.org> wrote:
>>> So I'm coming to this patchset cold but can you explain *why* something
>>> wants to call of_find_backlight_by_node() when there is no backlight
>>> support enabled. Why isn't the code that called is conditional on
>>> BACKLIGHT_CLASS_DEVICE?
>>>
>>> The undefined symbol issue is a pain but to be honest I'd rather solve
>>> the use of undefined symbols by avoiding declaring them; this making
>>> them into compile errors rather than link errors.
>>
>> Typically the kernel header files define static inline stubs of the
>> functions when the actual functions aren't configured/built. The code
>> using the functions looks the same regardless of the config option, and
>> handles the -ENODEV or NULL or whatever returns from the stubs
>> gracefully. With the inlines, the compiler can usually throw out much of
>> the code anyway.
>>
>> In this regard, the backlight interface is an exception, forcing the
>> callers to wrap the code around IS_ENABLED(BACKLIGHT_CLASS_DEVICE), not
>> the rule.
> 
> fwiw, I think it'd be great if we can move backlight over to the common
> pattern, like everyone else. It's pretty big pain as-is ...

For sure, after Jani's mail yesterday I looked at the GMA500 driver and 
concluded I didn't want code related to backlight having to look like that!

Currently the three main patterns of use are:

  1. Standalone drivers simple depend on BACKLIGHT_CLASS_DEVICE,
  2. Some compound drivers select BACKLIGHT_CLASS_DEVICE (the AMD DRM
     driver is an example of this),
  3. Other compound drivers perform heroics with the pre-processor.

The main reason I'm not just agreeing straight away is that, to adopt 
the static inline pattern for the whole API, we have to believe that #3 
above is desirable. Given the size and scope of the compound drivers in 
#3 above, I don't entirely understand why they want to avoid the select.


Daniel.


PS Whatever the outcome here, I do agree 100% that is wrong to prototype
    undefined symbols. That must be changed!

Re: [PATCH v7 1/3] backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)

[Daniel Vetter]

On Tue, Oct 03, 2017 at 09:51:49AM +0100, Daniel Thompson wrote:
> On 03/10/17 09:03, Daniel Vetter wrote:
> > On Mon, Oct 02, 2017 at 12:00:54PM +0300, Jani Nikula wrote:
> > > On Mon, 02 Oct 2017, Daniel Thompson <daniel.thompson@linaro.org> wrote:
> > > > So I'm coming to this patchset cold but can you explain *why* something
> > > > wants to call of_find_backlight_by_node() when there is no backlight
> > > > support enabled. Why isn't the code that called is conditional on
> > > > BACKLIGHT_CLASS_DEVICE?
> > > > 
> > > > The undefined symbol issue is a pain but to be honest I'd rather solve
> > > > the use of undefined symbols by avoiding declaring them; this making
> > > > them into compile errors rather than link errors.
> > > 
> > > Typically the kernel header files define static inline stubs of the
> > > functions when the actual functions aren't configured/built. The code
> > > using the functions looks the same regardless of the config option, and
> > > handles the -ENODEV or NULL or whatever returns from the stubs
> > > gracefully. With the inlines, the compiler can usually throw out much of
> > > the code anyway.
> > > 
> > > In this regard, the backlight interface is an exception, forcing the
> > > callers to wrap the code around IS_ENABLED(BACKLIGHT_CLASS_DEVICE), not
> > > the rule.
> > 
> > fwiw, I think it'd be great if we can move backlight over to the common
> > pattern, like everyone else. It's pretty big pain as-is ...
> 
> For sure, after Jani's mail yesterday I looked at the GMA500 driver and
> concluded I didn't want code related to backlight having to look like that!
> 
> Currently the three main patterns of use are:
> 
>  1. Standalone drivers simple depend on BACKLIGHT_CLASS_DEVICE,
>  2. Some compound drivers select BACKLIGHT_CLASS_DEVICE (the AMD DRM
>     driver is an example of this),
>  3. Other compound drivers perform heroics with the pre-processor.
> 
> The main reason I'm not just agreeing straight away is that, to adopt the
> static inline pattern for the whole API, we have to believe that #3 above is
> desirable. Given the size and scope of the compound drivers in #3 above, I
> don't entirely understand why they want to avoid the select.

People love to over-configure their kernels and shave off a few bytes. And
big gpu drivers might have backlight support, but not always need it (e.g.
if you run without a panel as e.g. a tv set-top-box). Same with e.g. a
driver that supports both OF/DT and pci to find its devices.

On the desktop gpu driver side we don't really subscribe to this idea of
making everything optional, hence select (mostly), except select is a huge
pain. On the mobile/soc gpu side, #3 seems to be the desired outcome.
Doing static inline helpers for backlights would make both easier (since
in the end for desktop you just get a distro kernel that enables
everything anyway).

So yeah, imo if you think backlight should be a Kconfig, then it should
have static inline dummy functions to make life simpler for everyone. Only
exception are pure driver-only subsystem code which aren't ever called by
anything outside of your subsystem. But since the point of the backlight
subsystem is to provide backlight support to other drivers (there's still
the dream that we don't offload this onto userspace, that just doesn't
work well) we really should have these static inline helpers.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v7 1/3] backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)

[Daniel Vetter]

On Tue, Oct 03, 2017 at 09:51:49AM +0100, Daniel Thompson wrote:
> On 03/10/17 09:03, Daniel Vetter wrote:
> > On Mon, Oct 02, 2017 at 12:00:54PM +0300, Jani Nikula wrote:
> > > On Mon, 02 Oct 2017, Daniel Thompson <daniel.thompson@linaro.org> wrote:
> > > > So I'm coming to this patchset cold but can you explain *why* something
> > > > wants to call of_find_backlight_by_node() when there is no backlight
> > > > support enabled. Why isn't the code that called is conditional on
> > > > BACKLIGHT_CLASS_DEVICE?
> > > > 
> > > > The undefined symbol issue is a pain but to be honest I'd rather solve
> > > > the use of undefined symbols by avoiding declaring them; this making
> > > > them into compile errors rather than link errors.
> > > 
> > > Typically the kernel header files define static inline stubs of the
> > > functions when the actual functions aren't configured/built. The code
> > > using the functions looks the same regardless of the config option, and
> > > handles the -ENODEV or NULL or whatever returns from the stubs
> > > gracefully. With the inlines, the compiler can usually throw out much of
> > > the code anyway.
> > > 
> > > In this regard, the backlight interface is an exception, forcing the
> > > callers to wrap the code around IS_ENABLED(BACKLIGHT_CLASS_DEVICE), not
> > > the rule.
> > 
> > fwiw, I think it'd be great if we can move backlight over to the common
> > pattern, like everyone else. It's pretty big pain as-is ...
> 
> For sure, after Jani's mail yesterday I looked at the GMA500 driver and
> concluded I didn't want code related to backlight having to look like that!
> 
> Currently the three main patterns of use are:
> 
>  1. Standalone drivers simple depend on BACKLIGHT_CLASS_DEVICE,
>  2. Some compound drivers select BACKLIGHT_CLASS_DEVICE (the AMD DRM
>     driver is an example of this),
>  3. Other compound drivers perform heroics with the pre-processor.
> 
> The main reason I'm not just agreeing straight away is that, to adopt the
> static inline pattern for the whole API, we have to believe that #3 above is
> desirable. Given the size and scope of the compound drivers in #3 above, I
> don't entirely understand why they want to avoid the select.

People love to over-configure their kernels and shave off a few bytes. And
big gpu drivers might have backlight support, but not always need it (e.g.
if you run without a panel as e.g. a tv set-top-box). Same with e.g. a
driver that supports both OF/DT and pci to find its devices.

On the desktop gpu driver side we don't really subscribe to this idea of
making everything optional, hence select (mostly), except select is a huge
pain. On the mobile/soc gpu side, #3 seems to be the desired outcome.
Doing static inline helpers for backlights would make both easier (since
in the end for desktop you just get a distro kernel that enables
everything anyway).

So yeah, imo if you think backlight should be a Kconfig, then it should
have static inline dummy functions to make life simpler for everyone. Only
exception are pure driver-only subsystem code which aren't ever called by
anything outside of your subsystem. But since the point of the backlight
subsystem is to provide backlight support to other drivers (there's still
the dream that we don't offload this onto userspace, that just doesn't
work well) we really should have these static inline helpers.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v7 1/3] backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)

[Noralf Trønnes]

Den 03.10.2017 11.04, skrev Daniel Vetter:
> On Tue, Oct 03, 2017 at 09:51:49AM +0100, Daniel Thompson wrote:
>> On 03/10/17 09:03, Daniel Vetter wrote:
>>> On Mon, Oct 02, 2017 at 12:00:54PM +0300, Jani Nikula wrote:
>>>> On Mon, 02 Oct 2017, Daniel Thompson <daniel.thompson@linaro.org> wrote:
>>>>> So I'm coming to this patchset cold but can you explain *why* something
>>>>> wants to call of_find_backlight_by_node() when there is no backlight
>>>>> support enabled. Why isn't the code that called is conditional on
>>>>> BACKLIGHT_CLASS_DEVICE?
>>>>>
>>>>> The undefined symbol issue is a pain but to be honest I'd rather solve
>>>>> the use of undefined symbols by avoiding declaring them; this making
>>>>> them into compile errors rather than link errors.
>>>> Typically the kernel header files define static inline stubs of the
>>>> functions when the actual functions aren't configured/built. The code
>>>> using the functions looks the same regardless of the config option, and
>>>> handles the -ENODEV or NULL or whatever returns from the stubs
>>>> gracefully. With the inlines, the compiler can usually throw out much of
>>>> the code anyway.
>>>>
>>>> In this regard, the backlight interface is an exception, forcing the
>>>> callers to wrap the code around IS_ENABLED(BACKLIGHT_CLASS_DEVICE), not
>>>> the rule.
>>> fwiw, I think it'd be great if we can move backlight over to the common
>>> pattern, like everyone else. It's pretty big pain as-is ...
>> For sure, after Jani's mail yesterday I looked at the GMA500 driver and
>> concluded I didn't want code related to backlight having to look like that!
>>
>> Currently the three main patterns of use are:
>>
>>   1. Standalone drivers simple depend on BACKLIGHT_CLASS_DEVICE,
>>   2. Some compound drivers select BACKLIGHT_CLASS_DEVICE (the AMD DRM
>>      driver is an example of this),
>>   3. Other compound drivers perform heroics with the pre-processor.
>>
>> The main reason I'm not just agreeing straight away is that, to adopt the
>> static inline pattern for the whole API, we have to believe that #3 above is
>> desirable. Given the size and scope of the compound drivers in #3 above, I
>> don't entirely understand why they want to avoid the select.
> People love to over-configure their kernels and shave off a few bytes. And
> big gpu drivers might have backlight support, but not always need it (e.g.
> if you run without a panel as e.g. a tv set-top-box). Same with e.g. a
> driver that supports both OF/DT and pci to find its devices.
>
> On the desktop gpu driver side we don't really subscribe to this idea of
> making everything optional, hence select (mostly), except select is a huge
> pain. On the mobile/soc gpu side, #3 seems to be the desired outcome.
> Doing static inline helpers for backlights would make both easier (since
> in the end for desktop you just get a distro kernel that enables
> everything anyway).
>
> So yeah, imo if you think backlight should be a Kconfig, then it should
> have static inline dummy functions to make life simpler for everyone. Only
> exception are pure driver-only subsystem code which aren't ever called by
> anything outside of your subsystem. But since the point of the backlight
> subsystem is to provide backlight support to other drivers (there's still
> the dream that we don't offload this onto userspace, that just doesn't
> work well) we really should have these static inline helpers.

I suggest we put all the backlight subsystem only functionality we need,
into the backlight subsystem :-)
I've put together a suggestion below and I've deliberately dropped the
of_ infix in backlight_get() to make room for possible future additions
that can make it possible to set the connection between device and
backlight using platform table or ACPI, fashioned after gpiolib.


include/linux/backlight.h:

/**
  * backlight_enable - Enable backlight
  * @bd: the backlight device to enable
  */
static inline int backlight_enable(struct backlight_device *bd)
{
     if (!bd)
         return 0;

     bd->props.power = FB_BLANK_UNBLANK;
     bd->props.state &= ~BL_CORE_FBBLANK;

     return backlight_update_status(bd);
}

/**
  * backlight_disable - Disable backlight
  * @bd: the backlight device to disable
  */
static inline int backlight_disable(struct backlight_device *bd)
{
     if (!bd)
         return 0;

     bd->props.power = FB_BLANK_POWERDOWN;
     bd->props.state |= BL_CORE_FBBLANK;

     return backlight_update_status(bd);
}

/**
  * backlight_put - Drop backlight reference
  * @bd: the backlight device to put
  */
static inline void backlight_put(struct backlight_device *bd)
{
     if (bd)
         put_device(bd->dev);
}

#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
struct backlight_device *backlight_get(struct device *dev);
struct backlight_device *devm_backlight_get(struct device *dev);
#else
static inline struct backlight_device *backlight_get(struct device *dev)
{
     return NULL;
}

static inline struct backlight_device *devm_backlight_get(struct device 
*dev)
{
     return NULL;
}
#endif



drivers/video/backlight/backlight.c:

/**
  * backlight_get - Get backlight device
  * @dev: Device
  *
  * This function looks for a property named 'backlight' on the DT node
  * connected to @dev and looks up the backlight device.
  *
  * Call backlight_put() to drop the reference on the backlight device.
  *
  * Returns:
  * A pointer to the backlight device if found.
  * Error pointer -EPROBE_DEFER if the DT property is set, but no backlight
  * device is found.
  * NULL if there's no backlight property.
  */
struct backlight_device *backlight_get(struct device *dev)
{
     struct backlight_device *bd = NULL;
     struct device_node *np;

     if (!dev)
         return NULL;

     if (IS_ENABLED(CONFIG_OF) && dev->of_node) {
         np = of_parse_phandle(dev->of_node, 'backlight', 0);
         if (np) {
             bd = of_find_backlight_by_node(np);
             of_node_put(np);
             if (!bd)
                 return ERR_PTR(-EPROBE_DEFER);
         }
     }

     return bd;
}
EXPORT_SYMBOL(backlight_get);

static void devm_backlight_get_release(void *data)
{
     backlight_put(data);
}

/**
  * devm_backlight_get - Resource-managed backlight_get()
  * @dev: Device
  *
  * Device managed version of backlight_get(). The reference on the 
backlight
  * device is automatically dropped on driver detach.
  */
struct backlight_device *devm_backlight_get(struct device *dev)
{
     struct backlight_device *bd;
     int ret;

     bd = backlight_get(dev);
     if (!bd)
         return NULL;

     ret = devm_add_action(dev, devm_backlight_get_release, bd);
     if (ret) {
         backlight_put(bd);
         return ERR_PTR(ret);
     }

     return bd;
}
EXPORT_SYMBOL(devm_backlight_get);


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v7 1/3] backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)

[Noralf Trønnes]

Den 03.10.2017 11.04, skrev Daniel Vetter:
> On Tue, Oct 03, 2017 at 09:51:49AM +0100, Daniel Thompson wrote:
>> On 03/10/17 09:03, Daniel Vetter wrote:
>>> On Mon, Oct 02, 2017 at 12:00:54PM +0300, Jani Nikula wrote:
>>>> On Mon, 02 Oct 2017, Daniel Thompson <daniel.thompson@linaro.org> wrote:
>>>>> So I'm coming to this patchset cold but can you explain *why* something
>>>>> wants to call of_find_backlight_by_node() when there is no backlight
>>>>> support enabled. Why isn't the code that called is conditional on
>>>>> BACKLIGHT_CLASS_DEVICE?
>>>>>
>>>>> The undefined symbol issue is a pain but to be honest I'd rather solve
>>>>> the use of undefined symbols by avoiding declaring them; this making
>>>>> them into compile errors rather than link errors.
>>>> Typically the kernel header files define static inline stubs of the
>>>> functions when the actual functions aren't configured/built. The code
>>>> using the functions looks the same regardless of the config option, and
>>>> handles the -ENODEV or NULL or whatever returns from the stubs
>>>> gracefully. With the inlines, the compiler can usually throw out much of
>>>> the code anyway.
>>>>
>>>> In this regard, the backlight interface is an exception, forcing the
>>>> callers to wrap the code around IS_ENABLED(BACKLIGHT_CLASS_DEVICE), not
>>>> the rule.
>>> fwiw, I think it'd be great if we can move backlight over to the common
>>> pattern, like everyone else. It's pretty big pain as-is ...
>> For sure, after Jani's mail yesterday I looked at the GMA500 driver and
>> concluded I didn't want code related to backlight having to look like that!
>>
>> Currently the three main patterns of use are:
>>
>>   1. Standalone drivers simple depend on BACKLIGHT_CLASS_DEVICE,
>>   2. Some compound drivers select BACKLIGHT_CLASS_DEVICE (the AMD DRM
>>      driver is an example of this),
>>   3. Other compound drivers perform heroics with the pre-processor.
>>
>> The main reason I'm not just agreeing straight away is that, to adopt the
>> static inline pattern for the whole API, we have to believe that #3 above is
>> desirable. Given the size and scope of the compound drivers in #3 above, I
>> don't entirely understand why they want to avoid the select.
> People love to over-configure their kernels and shave off a few bytes. And
> big gpu drivers might have backlight support, but not always need it (e.g.
> if you run without a panel as e.g. a tv set-top-box). Same with e.g. a
> driver that supports both OF/DT and pci to find its devices.
>
> On the desktop gpu driver side we don't really subscribe to this idea of
> making everything optional, hence select (mostly), except select is a huge
> pain. On the mobile/soc gpu side, #3 seems to be the desired outcome.
> Doing static inline helpers for backlights would make both easier (since
> in the end for desktop you just get a distro kernel that enables
> everything anyway).
>
> So yeah, imo if you think backlight should be a Kconfig, then it should
> have static inline dummy functions to make life simpler for everyone. Only
> exception are pure driver-only subsystem code which aren't ever called by
> anything outside of your subsystem. But since the point of the backlight
> subsystem is to provide backlight support to other drivers (there's still
> the dream that we don't offload this onto userspace, that just doesn't
> work well) we really should have these static inline helpers.

I suggest we put all the backlight subsystem only functionality we need,
into the backlight subsystem :-)
I've put together a suggestion below and I've deliberately dropped the
of_ infix in backlight_get() to make room for possible future additions
that can make it possible to set the connection between device and
backlight using platform table or ACPI, fashioned after gpiolib.


include/linux/backlight.h:

/**
  * backlight_enable - Enable backlight
  * @bd: the backlight device to enable
  */
static inline int backlight_enable(struct backlight_device *bd)
{
     if (!bd)
         return 0;

     bd->props.power = FB_BLANK_UNBLANK;
     bd->props.state &= ~BL_CORE_FBBLANK;

     return backlight_update_status(bd);
}

/**
  * backlight_disable - Disable backlight
  * @bd: the backlight device to disable
  */
static inline int backlight_disable(struct backlight_device *bd)
{
     if (!bd)
         return 0;

     bd->props.power = FB_BLANK_POWERDOWN;
     bd->props.state |= BL_CORE_FBBLANK;

     return backlight_update_status(bd);
}

/**
  * backlight_put - Drop backlight reference
  * @bd: the backlight device to put
  */
static inline void backlight_put(struct backlight_device *bd)
{
     if (bd)
         put_device(bd->dev);
}

#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
struct backlight_device *backlight_get(struct device *dev);
struct backlight_device *devm_backlight_get(struct device *dev);
#else
static inline struct backlight_device *backlight_get(struct device *dev)
{
     return NULL;
}

static inline struct backlight_device *devm_backlight_get(struct device 
*dev)
{
     return NULL;
}
#endif



drivers/video/backlight/backlight.c:

/**
  * backlight_get - Get backlight device
  * @dev: Device
  *
  * This function looks for a property named 'backlight' on the DT node
  * connected to @dev and looks up the backlight device.
  *
  * Call backlight_put() to drop the reference on the backlight device.
  *
  * Returns:
  * A pointer to the backlight device if found.
  * Error pointer -EPROBE_DEFER if the DT property is set, but no backlight
  * device is found.
  * NULL if there's no backlight property.
  */
struct backlight_device *backlight_get(struct device *dev)
{
     struct backlight_device *bd = NULL;
     struct device_node *np;

     if (!dev)
         return NULL;

     if (IS_ENABLED(CONFIG_OF) && dev->of_node) {
         np = of_parse_phandle(dev->of_node, 'backlight', 0);
         if (np) {
             bd = of_find_backlight_by_node(np);
             of_node_put(np);
             if (!bd)
                 return ERR_PTR(-EPROBE_DEFER);
         }
     }

     return bd;
}
EXPORT_SYMBOL(backlight_get);

static void devm_backlight_get_release(void *data)
{
     backlight_put(data);
}

/**
  * devm_backlight_get - Resource-managed backlight_get()
  * @dev: Device
  *
  * Device managed version of backlight_get(). The reference on the 
backlight
  * device is automatically dropped on driver detach.
  */
struct backlight_device *devm_backlight_get(struct device *dev)
{
     struct backlight_device *bd;
     int ret;

     bd = backlight_get(dev);
     if (!bd)
         return NULL;

     ret = devm_add_action(dev, devm_backlight_get_release, bd);
     if (ret) {
         backlight_put(bd);
         return ERR_PTR(ret);
     }

     return bd;
}
EXPORT_SYMBOL(devm_backlight_get);

Re: [PATCH v7 1/3] backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)

[Daniel Thompson]

On 06/10/17 19:01, Noralf Trønnes wrote:
> 
> Den 03.10.2017 11.04, skrev Daniel Vetter:
>> On Tue, Oct 03, 2017 at 09:51:49AM +0100, Daniel Thompson wrote:
>>> On 03/10/17 09:03, Daniel Vetter wrote:
>>>> On Mon, Oct 02, 2017 at 12:00:54PM +0300, Jani Nikula wrote:
>>>>> On Mon, 02 Oct 2017, Daniel Thompson <daniel.thompson@linaro.org> 
>>>>> wrote:
>>>>>> So I'm coming to this patchset cold but can you explain *why* 
>>>>>> something
>>>>>> wants to call of_find_backlight_by_node() when there is no backlight
>>>>>> support enabled. Why isn't the code that called is conditional on
>>>>>> BACKLIGHT_CLASS_DEVICE?
>>>>>>
>>>>>> The undefined symbol issue is a pain but to be honest I'd rather 
>>>>>> solve
>>>>>> the use of undefined symbols by avoiding declaring them; this making
>>>>>> them into compile errors rather than link errors.
>>>>> Typically the kernel header files define static inline stubs of the
>>>>> functions when the actual functions aren't configured/built. The code
>>>>> using the functions looks the same regardless of the config option, 
>>>>> and
>>>>> handles the -ENODEV or NULL or whatever returns from the stubs
>>>>> gracefully. With the inlines, the compiler can usually throw out 
>>>>> much of
>>>>> the code anyway.
>>>>>
>>>>> In this regard, the backlight interface is an exception, forcing the
>>>>> callers to wrap the code around IS_ENABLED(BACKLIGHT_CLASS_DEVICE), 
>>>>> not
>>>>> the rule.
>>>> fwiw, I think it'd be great if we can move backlight over to the common
>>>> pattern, like everyone else. It's pretty big pain as-is ...
>>> For sure, after Jani's mail yesterday I looked at the GMA500 driver and
>>> concluded I didn't want code related to backlight having to look like 
>>> that!
>>>
>>> Currently the three main patterns of use are:
>>>
>>>   1. Standalone drivers simple depend on BACKLIGHT_CLASS_DEVICE,
>>>   2. Some compound drivers select BACKLIGHT_CLASS_DEVICE (the AMD DRM
>>>      driver is an example of this),
>>>   3. Other compound drivers perform heroics with the pre-processor.
>>>
>>> The main reason I'm not just agreeing straight away is that, to adopt 
>>> the
>>> static inline pattern for the whole API, we have to believe that #3 
>>> above is
>>> desirable. Given the size and scope of the compound drivers in #3 
>>> above, I
>>> don't entirely understand why they want to avoid the select.
>> People love to over-configure their kernels and shave off a few bytes. 
>> And
>> big gpu drivers might have backlight support, but not always need it 
>> (e.g.
>> if you run without a panel as e.g. a tv set-top-box). Same with e.g. a
>> driver that supports both OF/DT and pci to find its devices.
>>
>> On the desktop gpu driver side we don't really subscribe to this idea of
>> making everything optional, hence select (mostly), except select is a 
>> huge
>> pain. On the mobile/soc gpu side, #3 seems to be the desired outcome.
>> Doing static inline helpers for backlights would make both easier (since
>> in the end for desktop you just get a distro kernel that enables
>> everything anyway).
>>
>> So yeah, imo if you think backlight should be a Kconfig, then it should
>> have static inline dummy functions to make life simpler for everyone. 
>> Only
>> exception are pure driver-only subsystem code which aren't ever called by
>> anything outside of your subsystem. But since the point of the backlight
>> subsystem is to provide backlight support to other drivers (there's still
>> the dream that we don't offload this onto userspace, that just doesn't
>> work well) we really should have these static inline helpers.
> 
> I suggest we put all the backlight subsystem only functionality we need,
> into the backlight subsystem :-)
> I've put together a suggestion below and I've deliberately dropped the
> of_ infix in backlight_get() to make room for possible future additions
> that can make it possible to set the connection between device and
> backlight using platform table or ACPI, fashioned after gpiolib.

Looks good to me.

If I've read this right, other sub-systems can use these symbols with or 
without BACKLIGHT_CLASS_DEVICE and still compile OK?


Daniel.


> 
> 
> include/linux/backlight.h:
> 
> /**
>   * backlight_enable - Enable backlight
>   * @bd: the backlight device to enable
>   */
> static inline int backlight_enable(struct backlight_device *bd)
> {
>      if (!bd)
>          return 0;
> 
>      bd->props.power = FB_BLANK_UNBLANK;
>      bd->props.state &= ~BL_CORE_FBBLANK;
> 
>      return backlight_update_status(bd);
> }
> 
> /**
>   * backlight_disable - Disable backlight
>   * @bd: the backlight device to disable
>   */
> static inline int backlight_disable(struct backlight_device *bd)
> {
>      if (!bd)
>          return 0;
> 
>      bd->props.power = FB_BLANK_POWERDOWN;
>      bd->props.state |= BL_CORE_FBBLANK;
> 
>      return backlight_update_status(bd);
> }
> 
> /**
>   * backlight_put - Drop backlight reference
>   * @bd: the backlight device to put
>   */
> static inline void backlight_put(struct backlight_device *bd)
> {
>      if (bd)
>          put_device(bd->dev);
> }
> 
> #if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
> struct backlight_device *backlight_get(struct device *dev);
> struct backlight_device *devm_backlight_get(struct device *dev);
> #else
> static inline struct backlight_device *backlight_get(struct device *dev)
> {
>      return NULL;
> }
> 
> static inline struct backlight_device *devm_backlight_get(struct device 
> *dev)
> {
>      return NULL;
> }
> #endif
> 
> 
> 
> drivers/video/backlight/backlight.c:
> 
> /**
>   * backlight_get - Get backlight device
>   * @dev: Device
>   *
>   * This function looks for a property named 'backlight' on the DT node
>   * connected to @dev and looks up the backlight device.
>   *
>   * Call backlight_put() to drop the reference on the backlight device.
>   *
>   * Returns:
>   * A pointer to the backlight device if found.
>   * Error pointer -EPROBE_DEFER if the DT property is set, but no backlight
>   * device is found.
>   * NULL if there's no backlight property.
>   */
> struct backlight_device *backlight_get(struct device *dev)
> {
>      struct backlight_device *bd = NULL;
>      struct device_node *np;
> 
>      if (!dev)
>          return NULL;
> 
>      if (IS_ENABLED(CONFIG_OF) && dev->of_node) {
>          np = of_parse_phandle(dev->of_node, 'backlight', 0);
>          if (np) {
>              bd = of_find_backlight_by_node(np);
>              of_node_put(np);
>              if (!bd)
>                  return ERR_PTR(-EPROBE_DEFER);
>          }
>      }
> 
>      return bd;
> }
> EXPORT_SYMBOL(backlight_get);
> 
> static void devm_backlight_get_release(void *data)
> {
>      backlight_put(data);
> }
> 
> /**
>   * devm_backlight_get - Resource-managed backlight_get()
>   * @dev: Device
>   *
>   * Device managed version of backlight_get(). The reference on the 
> backlight
>   * device is automatically dropped on driver detach.
>   */
> struct backlight_device *devm_backlight_get(struct device *dev)
> {
>      struct backlight_device *bd;
>      int ret;
> 
>      bd = backlight_get(dev);
>      if (!bd)
>          return NULL;
> 
>      ret = devm_add_action(dev, devm_backlight_get_release, bd);
>      if (ret) {
>          backlight_put(bd);
>          return ERR_PTR(ret);
>      }
> 
>      return bd;
> }
> EXPORT_SYMBOL(devm_backlight_get);
> 
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v7 1/3] backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)

[Daniel Thompson]

On 06/10/17 19:01, Noralf Trønnes wrote:
> 
> Den 03.10.2017 11.04, skrev Daniel Vetter:
>> On Tue, Oct 03, 2017 at 09:51:49AM +0100, Daniel Thompson wrote:
>>> On 03/10/17 09:03, Daniel Vetter wrote:
>>>> On Mon, Oct 02, 2017 at 12:00:54PM +0300, Jani Nikula wrote:
>>>>> On Mon, 02 Oct 2017, Daniel Thompson <daniel.thompson@linaro.org> 
>>>>> wrote:
>>>>>> So I'm coming to this patchset cold but can you explain *why* 
>>>>>> something
>>>>>> wants to call of_find_backlight_by_node() when there is no backlight
>>>>>> support enabled. Why isn't the code that called is conditional on
>>>>>> BACKLIGHT_CLASS_DEVICE?
>>>>>>
>>>>>> The undefined symbol issue is a pain but to be honest I'd rather 
>>>>>> solve
>>>>>> the use of undefined symbols by avoiding declaring them; this making
>>>>>> them into compile errors rather than link errors.
>>>>> Typically the kernel header files define static inline stubs of the
>>>>> functions when the actual functions aren't configured/built. The code
>>>>> using the functions looks the same regardless of the config option, 
>>>>> and
>>>>> handles the -ENODEV or NULL or whatever returns from the stubs
>>>>> gracefully. With the inlines, the compiler can usually throw out 
>>>>> much of
>>>>> the code anyway.
>>>>>
>>>>> In this regard, the backlight interface is an exception, forcing the
>>>>> callers to wrap the code around IS_ENABLED(BACKLIGHT_CLASS_DEVICE), 
>>>>> not
>>>>> the rule.
>>>> fwiw, I think it'd be great if we can move backlight over to the common
>>>> pattern, like everyone else. It's pretty big pain as-is ...
>>> For sure, after Jani's mail yesterday I looked at the GMA500 driver and
>>> concluded I didn't want code related to backlight having to look like 
>>> that!
>>>
>>> Currently the three main patterns of use are:
>>>
>>>   1. Standalone drivers simple depend on BACKLIGHT_CLASS_DEVICE,
>>>   2. Some compound drivers select BACKLIGHT_CLASS_DEVICE (the AMD DRM
>>>      driver is an example of this),
>>>   3. Other compound drivers perform heroics with the pre-processor.
>>>
>>> The main reason I'm not just agreeing straight away is that, to adopt 
>>> the
>>> static inline pattern for the whole API, we have to believe that #3 
>>> above is
>>> desirable. Given the size and scope of the compound drivers in #3 
>>> above, I
>>> don't entirely understand why they want to avoid the select.
>> People love to over-configure their kernels and shave off a few bytes. 
>> And
>> big gpu drivers might have backlight support, but not always need it 
>> (e.g.
>> if you run without a panel as e.g. a tv set-top-box). Same with e.g. a
>> driver that supports both OF/DT and pci to find its devices.
>>
>> On the desktop gpu driver side we don't really subscribe to this idea of
>> making everything optional, hence select (mostly), except select is a 
>> huge
>> pain. On the mobile/soc gpu side, #3 seems to be the desired outcome.
>> Doing static inline helpers for backlights would make both easier (since
>> in the end for desktop you just get a distro kernel that enables
>> everything anyway).
>>
>> So yeah, imo if you think backlight should be a Kconfig, then it should
>> have static inline dummy functions to make life simpler for everyone. 
>> Only
>> exception are pure driver-only subsystem code which aren't ever called by
>> anything outside of your subsystem. But since the point of the backlight
>> subsystem is to provide backlight support to other drivers (there's still
>> the dream that we don't offload this onto userspace, that just doesn't
>> work well) we really should have these static inline helpers.
> 
> I suggest we put all the backlight subsystem only functionality we need,
> into the backlight subsystem :-)
> I've put together a suggestion below and I've deliberately dropped the
> of_ infix in backlight_get() to make room for possible future additions
> that can make it possible to set the connection between device and
> backlight using platform table or ACPI, fashioned after gpiolib.

Looks good to me.

If I've read this right, other sub-systems can use these symbols with or 
without BACKLIGHT_CLASS_DEVICE and still compile OK?


Daniel.


> 
> 
> include/linux/backlight.h:
> 
> /**
>   * backlight_enable - Enable backlight
>   * @bd: the backlight device to enable
>   */
> static inline int backlight_enable(struct backlight_device *bd)
> {
>      if (!bd)
>          return 0;
> 
>      bd->props.power = FB_BLANK_UNBLANK;
>      bd->props.state &= ~BL_CORE_FBBLANK;
> 
>      return backlight_update_status(bd);
> }
> 
> /**
>   * backlight_disable - Disable backlight
>   * @bd: the backlight device to disable
>   */
> static inline int backlight_disable(struct backlight_device *bd)
> {
>      if (!bd)
>          return 0;
> 
>      bd->props.power = FB_BLANK_POWERDOWN;
>      bd->props.state |= BL_CORE_FBBLANK;
> 
>      return backlight_update_status(bd);
> }
> 
> /**
>   * backlight_put - Drop backlight reference
>   * @bd: the backlight device to put
>   */
> static inline void backlight_put(struct backlight_device *bd)
> {
>      if (bd)
>          put_device(bd->dev);
> }
> 
> #if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
> struct backlight_device *backlight_get(struct device *dev);
> struct backlight_device *devm_backlight_get(struct device *dev);
> #else
> static inline struct backlight_device *backlight_get(struct device *dev)
> {
>      return NULL;
> }
> 
> static inline struct backlight_device *devm_backlight_get(struct device 
> *dev)
> {
>      return NULL;
> }
> #endif
> 
> 
> 
> drivers/video/backlight/backlight.c:
> 
> /**
>   * backlight_get - Get backlight device
>   * @dev: Device
>   *
>   * This function looks for a property named 'backlight' on the DT node
>   * connected to @dev and looks up the backlight device.
>   *
>   * Call backlight_put() to drop the reference on the backlight device.
>   *
>   * Returns:
>   * A pointer to the backlight device if found.
>   * Error pointer -EPROBE_DEFER if the DT property is set, but no backlight
>   * device is found.
>   * NULL if there's no backlight property.
>   */
> struct backlight_device *backlight_get(struct device *dev)
> {
>      struct backlight_device *bd = NULL;
>      struct device_node *np;
> 
>      if (!dev)
>          return NULL;
> 
>      if (IS_ENABLED(CONFIG_OF) && dev->of_node) {
>          np = of_parse_phandle(dev->of_node, 'backlight', 0);
>          if (np) {
>              bd = of_find_backlight_by_node(np);
>              of_node_put(np);
>              if (!bd)
>                  return ERR_PTR(-EPROBE_DEFER);
>          }
>      }
> 
>      return bd;
> }
> EXPORT_SYMBOL(backlight_get);
> 
> static void devm_backlight_get_release(void *data)
> {
>      backlight_put(data);
> }
> 
> /**
>   * devm_backlight_get - Resource-managed backlight_get()
>   * @dev: Device
>   *
>   * Device managed version of backlight_get(). The reference on the 
> backlight
>   * device is automatically dropped on driver detach.
>   */
> struct backlight_device *devm_backlight_get(struct device *dev)
> {
>      struct backlight_device *bd;
>      int ret;
> 
>      bd = backlight_get(dev);
>      if (!bd)
>          return NULL;
> 
>      ret = devm_add_action(dev, devm_backlight_get_release, bd);
>      if (ret) {
>          backlight_put(bd);
>          return ERR_PTR(ret);
>      }
> 
>      return bd;
> }
> EXPORT_SYMBOL(devm_backlight_get);
> 
> 

Re: [PATCH v7 1/3] backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)

[Noralf Trønnes]

Den 09.10.2017 11.06, skrev Daniel Thompson:
> On 06/10/17 19:01, Noralf Trønnes wrote:
>>
>> Den 03.10.2017 11.04, skrev Daniel Vetter:
>>> On Tue, Oct 03, 2017 at 09:51:49AM +0100, Daniel Thompson wrote:
>>>> On 03/10/17 09:03, Daniel Vetter wrote:
>>>>> On Mon, Oct 02, 2017 at 12:00:54PM +0300, Jani Nikula wrote:
>>>>>> On Mon, 02 Oct 2017, Daniel Thompson <daniel.thompson@linaro.org> 
>>>>>> wrote:
>>>>>>> So I'm coming to this patchset cold but can you explain *why* 
>>>>>>> something
>>>>>>> wants to call of_find_backlight_by_node() when there is no 
>>>>>>> backlight
>>>>>>> support enabled. Why isn't the code that called is conditional on
>>>>>>> BACKLIGHT_CLASS_DEVICE?
>>>>>>>
>>>>>>> The undefined symbol issue is a pain but to be honest I'd rather 
>>>>>>> solve
>>>>>>> the use of undefined symbols by avoiding declaring them; this 
>>>>>>> making
>>>>>>> them into compile errors rather than link errors.
>>>>>> Typically the kernel header files define static inline stubs of the
>>>>>> functions when the actual functions aren't configured/built. The 
>>>>>> code
>>>>>> using the functions looks the same regardless of the config 
>>>>>> option, and
>>>>>> handles the -ENODEV or NULL or whatever returns from the stubs
>>>>>> gracefully. With the inlines, the compiler can usually throw out 
>>>>>> much of
>>>>>> the code anyway.
>>>>>>
>>>>>> In this regard, the backlight interface is an exception, forcing the
>>>>>> callers to wrap the code around 
>>>>>> IS_ENABLED(BACKLIGHT_CLASS_DEVICE), not
>>>>>> the rule.
>>>>> fwiw, I think it'd be great if we can move backlight over to the 
>>>>> common
>>>>> pattern, like everyone else. It's pretty big pain as-is ...
>>>> For sure, after Jani's mail yesterday I looked at the GMA500 driver 
>>>> and
>>>> concluded I didn't want code related to backlight having to look 
>>>> like that!
>>>>
>>>> Currently the three main patterns of use are:
>>>>
>>>>   1. Standalone drivers simple depend on BACKLIGHT_CLASS_DEVICE,
>>>>   2. Some compound drivers select BACKLIGHT_CLASS_DEVICE (the AMD DRM
>>>>      driver is an example of this),
>>>>   3. Other compound drivers perform heroics with the pre-processor.
>>>>
>>>> The main reason I'm not just agreeing straight away is that, to 
>>>> adopt the
>>>> static inline pattern for the whole API, we have to believe that #3 
>>>> above is
>>>> desirable. Given the size and scope of the compound drivers in #3 
>>>> above, I
>>>> don't entirely understand why they want to avoid the select.
>>> People love to over-configure their kernels and shave off a few 
>>> bytes. And
>>> big gpu drivers might have backlight support, but not always need it 
>>> (e.g.
>>> if you run without a panel as e.g. a tv set-top-box). Same with e.g. a
>>> driver that supports both OF/DT and pci to find its devices.
>>>
>>> On the desktop gpu driver side we don't really subscribe to this 
>>> idea of
>>> making everything optional, hence select (mostly), except select is 
>>> a huge
>>> pain. On the mobile/soc gpu side, #3 seems to be the desired outcome.
>>> Doing static inline helpers for backlights would make both easier 
>>> (since
>>> in the end for desktop you just get a distro kernel that enables
>>> everything anyway).
>>>
>>> So yeah, imo if you think backlight should be a Kconfig, then it should
>>> have static inline dummy functions to make life simpler for 
>>> everyone. Only
>>> exception are pure driver-only subsystem code which aren't ever 
>>> called by
>>> anything outside of your subsystem. But since the point of the 
>>> backlight
>>> subsystem is to provide backlight support to other drivers (there's 
>>> still
>>> the dream that we don't offload this onto userspace, that just doesn't
>>> work well) we really should have these static inline helpers.
>>
>> I suggest we put all the backlight subsystem only functionality we need,
>> into the backlight subsystem :-)
>> I've put together a suggestion below and I've deliberately dropped the
>> of_ infix in backlight_get() to make room for possible future additions
>> that can make it possible to set the connection between device and
>> backlight using platform table or ACPI, fashioned after gpiolib.
>
> Looks good to me.
>
> If I've read this right, other sub-systems can use these symbols with 
> or without BACKLIGHT_CLASS_DEVICE and still compile OK?
>

The code haven't seen a compiler, but that's the idea.

Noralf.

>
> Daniel.
>
>
>>
>>
>> include/linux/backlight.h:
>>
>> /**
>>   * backlight_enable - Enable backlight
>>   * @bd: the backlight device to enable
>>   */
>> static inline int backlight_enable(struct backlight_device *bd)
>> {
>>      if (!bd)
>>          return 0;
>>
>>      bd->props.power = FB_BLANK_UNBLANK;
>>      bd->props.state &= ~BL_CORE_FBBLANK;
>>
>>      return backlight_update_status(bd);
>> }
>>
>> /**
>>   * backlight_disable - Disable backlight
>>   * @bd: the backlight device to disable
>>   */
>> static inline int backlight_disable(struct backlight_device *bd)
>> {
>>      if (!bd)
>>          return 0;
>>
>>      bd->props.power = FB_BLANK_POWERDOWN;
>>      bd->props.state |= BL_CORE_FBBLANK;
>>
>>      return backlight_update_status(bd);
>> }
>>
>> /**
>>   * backlight_put - Drop backlight reference
>>   * @bd: the backlight device to put
>>   */
>> static inline void backlight_put(struct backlight_device *bd)
>> {
>>      if (bd)
>>          put_device(bd->dev);
>> }
>>
>> #if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
>> struct backlight_device *backlight_get(struct device *dev);
>> struct backlight_device *devm_backlight_get(struct device *dev);
>> #else
>> static inline struct backlight_device *backlight_get(struct device *dev)
>> {
>>      return NULL;
>> }
>>
>> static inline struct backlight_device *devm_backlight_get(struct 
>> device *dev)
>> {
>>      return NULL;
>> }
>> #endif
>>
>>
>>
>> drivers/video/backlight/backlight.c:
>>
>> /**
>>   * backlight_get - Get backlight device
>>   * @dev: Device
>>   *
>>   * This function looks for a property named 'backlight' on the DT node
>>   * connected to @dev and looks up the backlight device.
>>   *
>>   * Call backlight_put() to drop the reference on the backlight device.
>>   *
>>   * Returns:
>>   * A pointer to the backlight device if found.
>>   * Error pointer -EPROBE_DEFER if the DT property is set, but no 
>> backlight
>>   * device is found.
>>   * NULL if there's no backlight property.
>>   */
>> struct backlight_device *backlight_get(struct device *dev)
>> {
>>      struct backlight_device *bd = NULL;
>>      struct device_node *np;
>>
>>      if (!dev)
>>          return NULL;
>>
>>      if (IS_ENABLED(CONFIG_OF) && dev->of_node) {
>>          np = of_parse_phandle(dev->of_node, 'backlight', 0);
>>          if (np) {
>>              bd = of_find_backlight_by_node(np);
>>              of_node_put(np);
>>              if (!bd)
>>                  return ERR_PTR(-EPROBE_DEFER);
>>          }
>>      }
>>
>>      return bd;
>> }
>> EXPORT_SYMBOL(backlight_get);
>>
>> static void devm_backlight_get_release(void *data)
>> {
>>      backlight_put(data);
>> }
>>
>> /**
>>   * devm_backlight_get - Resource-managed backlight_get()
>>   * @dev: Device
>>   *
>>   * Device managed version of backlight_get(). The reference on the 
>> backlight
>>   * device is automatically dropped on driver detach.
>>   */
>> struct backlight_device *devm_backlight_get(struct device *dev)
>> {
>>      struct backlight_device *bd;
>>      int ret;
>>
>>      bd = backlight_get(dev);
>>      if (!bd)
>>          return NULL;
>>
>>      ret = devm_add_action(dev, devm_backlight_get_release, bd);
>>      if (ret) {
>>          backlight_put(bd);
>>          return ERR_PTR(ret);
>>      }
>>
>>      return bd;
>> }
>> EXPORT_SYMBOL(devm_backlight_get);
>>
>>
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v7 1/3] backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)

[Noralf Trønnes]

Den 09.10.2017 11.06, skrev Daniel Thompson:
> On 06/10/17 19:01, Noralf Trønnes wrote:
>>
>> Den 03.10.2017 11.04, skrev Daniel Vetter:
>>> On Tue, Oct 03, 2017 at 09:51:49AM +0100, Daniel Thompson wrote:
>>>> On 03/10/17 09:03, Daniel Vetter wrote:
>>>>> On Mon, Oct 02, 2017 at 12:00:54PM +0300, Jani Nikula wrote:
>>>>>> On Mon, 02 Oct 2017, Daniel Thompson <daniel.thompson@linaro.org> 
>>>>>> wrote:
>>>>>>> So I'm coming to this patchset cold but can you explain *why* 
>>>>>>> something
>>>>>>> wants to call of_find_backlight_by_node() when there is no 
>>>>>>> backlight
>>>>>>> support enabled. Why isn't the code that called is conditional on
>>>>>>> BACKLIGHT_CLASS_DEVICE?
>>>>>>>
>>>>>>> The undefined symbol issue is a pain but to be honest I'd rather 
>>>>>>> solve
>>>>>>> the use of undefined symbols by avoiding declaring them; this 
>>>>>>> making
>>>>>>> them into compile errors rather than link errors.
>>>>>> Typically the kernel header files define static inline stubs of the
>>>>>> functions when the actual functions aren't configured/built. The 
>>>>>> code
>>>>>> using the functions looks the same regardless of the config 
>>>>>> option, and
>>>>>> handles the -ENODEV or NULL or whatever returns from the stubs
>>>>>> gracefully. With the inlines, the compiler can usually throw out 
>>>>>> much of
>>>>>> the code anyway.
>>>>>>
>>>>>> In this regard, the backlight interface is an exception, forcing the
>>>>>> callers to wrap the code around 
>>>>>> IS_ENABLED(BACKLIGHT_CLASS_DEVICE), not
>>>>>> the rule.
>>>>> fwiw, I think it'd be great if we can move backlight over to the 
>>>>> common
>>>>> pattern, like everyone else. It's pretty big pain as-is ...
>>>> For sure, after Jani's mail yesterday I looked at the GMA500 driver 
>>>> and
>>>> concluded I didn't want code related to backlight having to look 
>>>> like that!
>>>>
>>>> Currently the three main patterns of use are:
>>>>
>>>>   1. Standalone drivers simple depend on BACKLIGHT_CLASS_DEVICE,
>>>>   2. Some compound drivers select BACKLIGHT_CLASS_DEVICE (the AMD DRM
>>>>      driver is an example of this),
>>>>   3. Other compound drivers perform heroics with the pre-processor.
>>>>
>>>> The main reason I'm not just agreeing straight away is that, to 
>>>> adopt the
>>>> static inline pattern for the whole API, we have to believe that #3 
>>>> above is
>>>> desirable. Given the size and scope of the compound drivers in #3 
>>>> above, I
>>>> don't entirely understand why they want to avoid the select.
>>> People love to over-configure their kernels and shave off a few 
>>> bytes. And
>>> big gpu drivers might have backlight support, but not always need it 
>>> (e.g.
>>> if you run without a panel as e.g. a tv set-top-box). Same with e.g. a
>>> driver that supports both OF/DT and pci to find its devices.
>>>
>>> On the desktop gpu driver side we don't really subscribe to this 
>>> idea of
>>> making everything optional, hence select (mostly), except select is 
>>> a huge
>>> pain. On the mobile/soc gpu side, #3 seems to be the desired outcome.
>>> Doing static inline helpers for backlights would make both easier 
>>> (since
>>> in the end for desktop you just get a distro kernel that enables
>>> everything anyway).
>>>
>>> So yeah, imo if you think backlight should be a Kconfig, then it should
>>> have static inline dummy functions to make life simpler for 
>>> everyone. Only
>>> exception are pure driver-only subsystem code which aren't ever 
>>> called by
>>> anything outside of your subsystem. But since the point of the 
>>> backlight
>>> subsystem is to provide backlight support to other drivers (there's 
>>> still
>>> the dream that we don't offload this onto userspace, that just doesn't
>>> work well) we really should have these static inline helpers.
>>
>> I suggest we put all the backlight subsystem only functionality we need,
>> into the backlight subsystem :-)
>> I've put together a suggestion below and I've deliberately dropped the
>> of_ infix in backlight_get() to make room for possible future additions
>> that can make it possible to set the connection between device and
>> backlight using platform table or ACPI, fashioned after gpiolib.
>
> Looks good to me.
>
> If I've read this right, other sub-systems can use these symbols with 
> or without BACKLIGHT_CLASS_DEVICE and still compile OK?
>

The code haven't seen a compiler, but that's the idea.

Noralf.

>
> Daniel.
>
>
>>
>>
>> include/linux/backlight.h:
>>
>> /**
>>   * backlight_enable - Enable backlight
>>   * @bd: the backlight device to enable
>>   */
>> static inline int backlight_enable(struct backlight_device *bd)
>> {
>>      if (!bd)
>>          return 0;
>>
>>      bd->props.power = FB_BLANK_UNBLANK;
>>      bd->props.state &= ~BL_CORE_FBBLANK;
>>
>>      return backlight_update_status(bd);
>> }
>>
>> /**
>>   * backlight_disable - Disable backlight
>>   * @bd: the backlight device to disable
>>   */
>> static inline int backlight_disable(struct backlight_device *bd)
>> {
>>      if (!bd)
>>          return 0;
>>
>>      bd->props.power = FB_BLANK_POWERDOWN;
>>      bd->props.state |= BL_CORE_FBBLANK;
>>
>>      return backlight_update_status(bd);
>> }
>>
>> /**
>>   * backlight_put - Drop backlight reference
>>   * @bd: the backlight device to put
>>   */
>> static inline void backlight_put(struct backlight_device *bd)
>> {
>>      if (bd)
>>          put_device(bd->dev);
>> }
>>
>> #if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
>> struct backlight_device *backlight_get(struct device *dev);
>> struct backlight_device *devm_backlight_get(struct device *dev);
>> #else
>> static inline struct backlight_device *backlight_get(struct device *dev)
>> {
>>      return NULL;
>> }
>>
>> static inline struct backlight_device *devm_backlight_get(struct 
>> device *dev)
>> {
>>      return NULL;
>> }
>> #endif
>>
>>
>>
>> drivers/video/backlight/backlight.c:
>>
>> /**
>>   * backlight_get - Get backlight device
>>   * @dev: Device
>>   *
>>   * This function looks for a property named 'backlight' on the DT node
>>   * connected to @dev and looks up the backlight device.
>>   *
>>   * Call backlight_put() to drop the reference on the backlight device.
>>   *
>>   * Returns:
>>   * A pointer to the backlight device if found.
>>   * Error pointer -EPROBE_DEFER if the DT property is set, but no 
>> backlight
>>   * device is found.
>>   * NULL if there's no backlight property.
>>   */
>> struct backlight_device *backlight_get(struct device *dev)
>> {
>>      struct backlight_device *bd = NULL;
>>      struct device_node *np;
>>
>>      if (!dev)
>>          return NULL;
>>
>>      if (IS_ENABLED(CONFIG_OF) && dev->of_node) {
>>          np = of_parse_phandle(dev->of_node, 'backlight', 0);
>>          if (np) {
>>              bd = of_find_backlight_by_node(np);
>>              of_node_put(np);
>>              if (!bd)
>>                  return ERR_PTR(-EPROBE_DEFER);
>>          }
>>      }
>>
>>      return bd;
>> }
>> EXPORT_SYMBOL(backlight_get);
>>
>> static void devm_backlight_get_release(void *data)
>> {
>>      backlight_put(data);
>> }
>>
>> /**
>>   * devm_backlight_get - Resource-managed backlight_get()
>>   * @dev: Device
>>   *
>>   * Device managed version of backlight_get(). The reference on the 
>> backlight
>>   * device is automatically dropped on driver detach.
>>   */
>> struct backlight_device *devm_backlight_get(struct device *dev)
>> {
>>      struct backlight_device *bd;
>>      int ret;
>>
>>      bd = backlight_get(dev);
>>      if (!bd)
>>          return NULL;
>>
>>      ret = devm_add_action(dev, devm_backlight_get_release, bd);
>>      if (ret) {
>>          backlight_put(bd);
>>          return ERR_PTR(ret);
>>      }
>>
>>      return bd;
>> }
>> EXPORT_SYMBOL(devm_backlight_get);
>>
>>
>