[PATCH v13 0/3] Move backlight helper functions from tinydrm-helpers to linux/backlight

[PATCH v13 0/3] Move backlight helper functions from tinydrm-helpers to linux/backlight

[Meghana Madhyastha]

Move drm helper functions from tinydrm-helpers to linux/backlight for
ease of use by callers in other drivers.

Changes in v13:
-Rebase against drm-misc
-Put devm_backlight_put back

Meghana Madhyastha (3):
  drm/tinydrm: Move helper functions from tinydrm-helpers to backlight.h
  drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c
  drm/tinydrm: Add devres versions of backlight_get

 drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 95 --------------------------
 drivers/gpu/drm/tinydrm/mi0283qt.c             |  3 +-
 drivers/gpu/drm/tinydrm/mipi-dbi.c             |  5 +-
 drivers/video/backlight/backlight.c            | 68 ++++++++++++++++++
 include/drm/tinydrm/tinydrm-helpers.h          |  4 --
 include/linux/backlight.h                      | 55 +++++++++++++++
 6 files changed, 128 insertions(+), 102 deletions(-)

-- 
2.7.4

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

[PATCH v13 0/3] Move backlight helper functions from tinydrm-helpers to linux/backlight

[Meghana Madhyastha]

Move drm helper functions from tinydrm-helpers to linux/backlight for
ease of use by callers in other drivers.

Changes in v13:
-Rebase against drm-misc
-Put devm_backlight_put back

Meghana Madhyastha (3):
  drm/tinydrm: Move helper functions from tinydrm-helpers to backlight.h
  drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c
  drm/tinydrm: Add devres versions of backlight_get

 drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 95 --------------------------
 drivers/gpu/drm/tinydrm/mi0283qt.c             |  3 +-
 drivers/gpu/drm/tinydrm/mipi-dbi.c             |  5 +-
 drivers/video/backlight/backlight.c            | 68 ++++++++++++++++++
 include/drm/tinydrm/tinydrm-helpers.h          |  4 --
 include/linux/backlight.h                      | 55 +++++++++++++++
 6 files changed, 128 insertions(+), 102 deletions(-)

-- 
2.7.4

[PATCH v13 1/3] drm/tinydrm: Move helper functions from tinydrm-helpers to backlight.h

[Meghana Madhyastha]

Move the helper functions enable_backlight and disable_backlight
from tinydrm-helpers.c to backlight.h as static inline functions so
that they can be used by other drivers.

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

 drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 55 --------------------------
 drivers/gpu/drm/tinydrm/mipi-dbi.c             |  5 ++-
 include/drm/tinydrm/tinydrm-helpers.h          |  2 -
 include/linux/backlight.h                      | 30 ++++++++++++++
 4 files changed, 33 insertions(+), 59 deletions(-)

diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
index bd6cce0..a42dee6 100644
--- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
+++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
@@ -276,61 +276,6 @@ struct backlight_device *tinydrm_of_find_backlight(struct device *dev)
 }
 EXPORT_SYMBOL(tinydrm_of_find_backlight);
 
-/**
- * tinydrm_enable_backlight - Enable backlight helper
- * @backlight: Backlight device
- *
- * Returns:
- * Zero on success, negative error code on failure.
- */
-int tinydrm_enable_backlight(struct backlight_device *backlight)
-{
-	unsigned int old_state;
-	int ret;
-
-	if (!backlight)
-		return 0;
-
-	old_state = backlight->props.state;
-	backlight->props.state &= ~BL_CORE_FBBLANK;
-	DRM_DEBUG_KMS("Backlight state: 0x%x -> 0x%x\n", old_state,
-		      backlight->props.state);
-
-	ret = backlight_update_status(backlight);
-	if (ret)
-		DRM_ERROR("Failed to enable backlight %d\n", ret);
-
-	return ret;
-}
-EXPORT_SYMBOL(tinydrm_enable_backlight);
-
-/**
- * tinydrm_disable_backlight - Disable backlight helper
- * @backlight: Backlight device
- *
- * Returns:
- * Zero on success, negative error code on failure.
- */
-int tinydrm_disable_backlight(struct backlight_device *backlight)
-{
-	unsigned int old_state;
-	int ret;
-
-	if (!backlight)
-		return 0;
-
-	old_state = backlight->props.state;
-	backlight->props.state |= BL_CORE_FBBLANK;
-	DRM_DEBUG_KMS("Backlight state: 0x%x -> 0x%x\n", old_state,
-		      backlight->props.state);
-	ret = backlight_update_status(backlight);
-	if (ret)
-		DRM_ERROR("Failed to disable backlight %d\n", ret);
-
-	return ret;
-}
-EXPORT_SYMBOL(tinydrm_disable_backlight);
-
 #if IS_ENABLED(CONFIG_SPI)
 
 /**
diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c
index d43e992..2561549 100644
--- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
+++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
@@ -12,6 +12,7 @@
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/tinydrm/mipi-dbi.h>
 #include <drm/tinydrm/tinydrm-helpers.h>
+#include <linux/backlight.h>
 #include <linux/debugfs.h>
 #include <linux/dma-buf.h>
 #include <linux/gpio/consumer.h>
@@ -280,7 +281,7 @@ void mipi_dbi_pipe_enable(struct drm_simple_display_pipe *pipe,
 	if (fb)
 		fb->funcs->dirty(fb, NULL, 0, 0, NULL, 0);
 
-	tinydrm_enable_backlight(mipi->backlight);
+	backlight_enable(mipi->backlight);
 }
 EXPORT_SYMBOL(mipi_dbi_pipe_enable);
 
@@ -319,7 +320,7 @@ void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe)
 	mipi->enabled = false;
 
 	if (mipi->backlight)
-		tinydrm_disable_backlight(mipi->backlight);
+		backlight_disable(mipi->backlight);
 	else
 		mipi_dbi_blank(mipi);
 }
diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h
index d554ded..f54fae0 100644
--- a/include/drm/tinydrm/tinydrm-helpers.h
+++ b/include/drm/tinydrm/tinydrm-helpers.h
@@ -47,8 +47,6 @@ 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);
 
 size_t tinydrm_spi_max_transfer_size(struct spi_device *spi, size_t max_len);
 bool tinydrm_spi_bpw_supported(struct spi_device *spi, u8 bpw);
diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index 5f2fd61..b88fabb 100644
--- a/include/linux/backlight.h
+++ b/include/linux/backlight.h
@@ -129,6 +129,36 @@ static inline int backlight_update_status(struct backlight_device *bd)
 	return ret;
 }
 
+/**
+ * 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);
+}
+
 extern struct backlight_device *backlight_device_register(const char *name,
 	struct device *dev, void *devdata, const struct backlight_ops *ops,
 	const struct backlight_properties *props);
-- 
2.7.4

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

[PATCH v13 1/3] drm/tinydrm: Move helper functions from tinydrm-helpers to backlight.h

[Meghana Madhyastha]

Move the helper functions enable_backlight and disable_backlight
from tinydrm-helpers.c to backlight.h as static inline functions so
that they can be used by other drivers.

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

 drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 55 --------------------------
 drivers/gpu/drm/tinydrm/mipi-dbi.c             |  5 ++-
 include/drm/tinydrm/tinydrm-helpers.h          |  2 -
 include/linux/backlight.h                      | 30 ++++++++++++++
 4 files changed, 33 insertions(+), 59 deletions(-)

diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
index bd6cce0..a42dee6 100644
--- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
+++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
@@ -276,61 +276,6 @@ struct backlight_device *tinydrm_of_find_backlight(struct device *dev)
 }
 EXPORT_SYMBOL(tinydrm_of_find_backlight);
 
-/**
- * tinydrm_enable_backlight - Enable backlight helper
- * @backlight: Backlight device
- *
- * Returns:
- * Zero on success, negative error code on failure.
- */
-int tinydrm_enable_backlight(struct backlight_device *backlight)
-{
-	unsigned int old_state;
-	int ret;
-
-	if (!backlight)
-		return 0;
-
-	old_state = backlight->props.state;
-	backlight->props.state &= ~BL_CORE_FBBLANK;
-	DRM_DEBUG_KMS("Backlight state: 0x%x -> 0x%x\n", old_state,
-		      backlight->props.state);
-
-	ret = backlight_update_status(backlight);
-	if (ret)
-		DRM_ERROR("Failed to enable backlight %d\n", ret);
-
-	return ret;
-}
-EXPORT_SYMBOL(tinydrm_enable_backlight);
-
-/**
- * tinydrm_disable_backlight - Disable backlight helper
- * @backlight: Backlight device
- *
- * Returns:
- * Zero on success, negative error code on failure.
- */
-int tinydrm_disable_backlight(struct backlight_device *backlight)
-{
-	unsigned int old_state;
-	int ret;
-
-	if (!backlight)
-		return 0;
-
-	old_state = backlight->props.state;
-	backlight->props.state |= BL_CORE_FBBLANK;
-	DRM_DEBUG_KMS("Backlight state: 0x%x -> 0x%x\n", old_state,
-		      backlight->props.state);
-	ret = backlight_update_status(backlight);
-	if (ret)
-		DRM_ERROR("Failed to disable backlight %d\n", ret);
-
-	return ret;
-}
-EXPORT_SYMBOL(tinydrm_disable_backlight);
-
 #if IS_ENABLED(CONFIG_SPI)
 
 /**
diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c
index d43e992..2561549 100644
--- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
+++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
@@ -12,6 +12,7 @@
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/tinydrm/mipi-dbi.h>
 #include <drm/tinydrm/tinydrm-helpers.h>
+#include <linux/backlight.h>
 #include <linux/debugfs.h>
 #include <linux/dma-buf.h>
 #include <linux/gpio/consumer.h>
@@ -280,7 +281,7 @@ void mipi_dbi_pipe_enable(struct drm_simple_display_pipe *pipe,
 	if (fb)
 		fb->funcs->dirty(fb, NULL, 0, 0, NULL, 0);
 
-	tinydrm_enable_backlight(mipi->backlight);
+	backlight_enable(mipi->backlight);
 }
 EXPORT_SYMBOL(mipi_dbi_pipe_enable);
 
@@ -319,7 +320,7 @@ void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe)
 	mipi->enabled = false;
 
 	if (mipi->backlight)
-		tinydrm_disable_backlight(mipi->backlight);
+		backlight_disable(mipi->backlight);
 	else
 		mipi_dbi_blank(mipi);
 }
diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h
index d554ded..f54fae0 100644
--- a/include/drm/tinydrm/tinydrm-helpers.h
+++ b/include/drm/tinydrm/tinydrm-helpers.h
@@ -47,8 +47,6 @@ 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);
 
 size_t tinydrm_spi_max_transfer_size(struct spi_device *spi, size_t max_len);
 bool tinydrm_spi_bpw_supported(struct spi_device *spi, u8 bpw);
diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index 5f2fd61..b88fabb 100644
--- a/include/linux/backlight.h
+++ b/include/linux/backlight.h
@@ -129,6 +129,36 @@ static inline int backlight_update_status(struct backlight_device *bd)
 	return ret;
 }
 
+/**
+ * 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);
+}
+
 extern struct backlight_device *backlight_device_register(const char *name,
 	struct device *dev, void *devdata, const struct backlight_ops *ops,
 	const struct backlight_properties *props);
-- 
2.7.4

[PATCH v13 2/3] drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c

[Meghana Madhyastha]

Rename tinydrm_of_find_backlight to backlight_get and move it
to linux/backlight.c so that it can be used by other drivers.

Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
---
Changes in v13:
 -Add backlight_put to backlight.h in this patch

 drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 40 --------------------------
 drivers/gpu/drm/tinydrm/mi0283qt.c             |  3 +-
 drivers/video/backlight/backlight.c            | 37 ++++++++++++++++++++++++
 include/drm/tinydrm/tinydrm-helpers.h          |  2 --
 include/linux/backlight.h                      | 19 ++++++++++++
 5 files changed, 58 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
index a42dee6..cb1a01a 100644
--- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
+++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
@@ -236,46 +236,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);
-
 #if IS_ENABLED(CONFIG_SPI)
 
 /**
diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
index 7fd2691..a932185 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 <linux/backlight.h>
 #include <linux/delay.h>
 #include <linux/gpio/consumer.h>
 #include <linux/module.h>
@@ -188,7 +189,7 @@ static int mi0283qt_probe(struct spi_device *spi)
 	if (IS_ERR(mipi->regulator))
 		return PTR_ERR(mipi->regulator);
 
-	mipi->backlight = tinydrm_of_find_backlight(dev);
+	mipi->backlight = backlight_get(dev);
 	if (IS_ERR(mipi->backlight))
 		return PTR_ERR(mipi->backlight);
 
diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index 8049e76..c4e94d0 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -580,6 +580,43 @@ struct backlight_device *of_find_backlight_by_node(struct device_node *node)
 EXPORT_SYMBOL(of_find_backlight_by_node);
 #endif
 
+/**
+ * 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 __exit backlight_class_exit(void)
 {
 	class_destroy(backlight_class);
diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h
index f54fae0..0a4ddbc 100644
--- a/include/drm/tinydrm/tinydrm-helpers.h
+++ b/include/drm/tinydrm/tinydrm-helpers.h
@@ -46,8 +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);
-
 size_t tinydrm_spi_max_transfer_size(struct spi_device *spi, size_t max_len);
 bool tinydrm_spi_bpw_supported(struct spi_device *spi, u8 bpw);
 int tinydrm_spi_transfer(struct spi_device *spi, u32 speed_hz,
diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index b88fabb..987a6d7 100644
--- a/include/linux/backlight.h
+++ b/include/linux/backlight.h
@@ -159,6 +159,16 @@ static inline int backlight_disable(struct backlight_device *bd)
 	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);
+}
+
 extern struct backlight_device *backlight_device_register(const char *name,
 	struct device *dev, void *devdata, const struct backlight_ops *ops,
 	const struct backlight_properties *props);
@@ -202,4 +212,13 @@ of_find_backlight_by_node(struct device_node *node)
 }
 #endif
 
+#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
+struct backlight_device *backlight_get(struct device *dev);
+#else
+static inline struct backlight_device *backlight_get(struct device *dev)
+{
+	return NULL;
+}
+#endif
+
 #endif
-- 
2.7.4

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

[PATCH v13 2/3] drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c

[Meghana Madhyastha]

Rename tinydrm_of_find_backlight to backlight_get and move it
to linux/backlight.c so that it can be used by other drivers.

Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
---
Changes in v13:
 -Add backlight_put to backlight.h in this patch

 drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 40 --------------------------
 drivers/gpu/drm/tinydrm/mi0283qt.c             |  3 +-
 drivers/video/backlight/backlight.c            | 37 ++++++++++++++++++++++++
 include/drm/tinydrm/tinydrm-helpers.h          |  2 --
 include/linux/backlight.h                      | 19 ++++++++++++
 5 files changed, 58 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
index a42dee6..cb1a01a 100644
--- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
+++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
@@ -236,46 +236,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);
-
 #if IS_ENABLED(CONFIG_SPI)
 
 /**
diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
index 7fd2691..a932185 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 <linux/backlight.h>
 #include <linux/delay.h>
 #include <linux/gpio/consumer.h>
 #include <linux/module.h>
@@ -188,7 +189,7 @@ static int mi0283qt_probe(struct spi_device *spi)
 	if (IS_ERR(mipi->regulator))
 		return PTR_ERR(mipi->regulator);
 
-	mipi->backlight = tinydrm_of_find_backlight(dev);
+	mipi->backlight = backlight_get(dev);
 	if (IS_ERR(mipi->backlight))
 		return PTR_ERR(mipi->backlight);
 
diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index 8049e76..c4e94d0 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -580,6 +580,43 @@ struct backlight_device *of_find_backlight_by_node(struct device_node *node)
 EXPORT_SYMBOL(of_find_backlight_by_node);
 #endif
 
+/**
+ * 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 __exit backlight_class_exit(void)
 {
 	class_destroy(backlight_class);
diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h
index f54fae0..0a4ddbc 100644
--- a/include/drm/tinydrm/tinydrm-helpers.h
+++ b/include/drm/tinydrm/tinydrm-helpers.h
@@ -46,8 +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);
-
 size_t tinydrm_spi_max_transfer_size(struct spi_device *spi, size_t max_len);
 bool tinydrm_spi_bpw_supported(struct spi_device *spi, u8 bpw);
 int tinydrm_spi_transfer(struct spi_device *spi, u32 speed_hz,
diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index b88fabb..987a6d7 100644
--- a/include/linux/backlight.h
+++ b/include/linux/backlight.h
@@ -159,6 +159,16 @@ static inline int backlight_disable(struct backlight_device *bd)
 	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);
+}
+
 extern struct backlight_device *backlight_device_register(const char *name,
 	struct device *dev, void *devdata, const struct backlight_ops *ops,
 	const struct backlight_properties *props);
@@ -202,4 +212,13 @@ of_find_backlight_by_node(struct device_node *node)
 }
 #endif
 
+#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
+struct backlight_device *backlight_get(struct device *dev);
+#else
+static inline struct backlight_device *backlight_get(struct device *dev)
+{
+	return NULL;
+}
+#endif
+
 #endif
-- 
2.7.4

[PATCH v13 3/3] drm/tinydrm: Add devres versions of backlight_get

[Meghana Madhyastha]

Add devm_backlight_get 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 v13:
 -Add devm_backlight_put to backlight.c

 drivers/gpu/drm/tinydrm/mi0283qt.c  |  2 +-
 drivers/video/backlight/backlight.c | 31 +++++++++++++++++++++++++++++++
 include/linux/backlight.h           |  6 ++++++
 3 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
index a932185..e3e7583 100644
--- a/drivers/gpu/drm/tinydrm/mi0283qt.c
+++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
@@ -189,7 +189,7 @@ static int mi0283qt_probe(struct spi_device *spi)
 	if (IS_ERR(mipi->regulator))
 		return PTR_ERR(mipi->regulator);
 
-	mipi->backlight = backlight_get(dev);
+	mipi->backlight = devm_backlight_get(dev);
 	if (IS_ERR(mipi->backlight))
 		return PTR_ERR(mipi->backlight);
 
diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index c4e94d0..b6c505a 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -617,6 +617,37 @@ struct backlight_device *backlight_get(struct device *dev)
 }
 EXPORT_SYMBOL(backlight_get);
 
+static void devm_backlight_put(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_put, bd);
+	if (ret) {
+		backlight_put(bd);
+		return ERR_PTR(ret);
+	}
+
+	return bd;
+}
+EXPORT_SYMBOL(devm_backlight_get);
+
 static void __exit backlight_class_exit(void)
 {
 	class_destroy(backlight_class);
diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index 987a6d7..8db9ba9 100644
--- a/include/linux/backlight.h
+++ b/include/linux/backlight.h
@@ -214,11 +214,17 @@ of_find_backlight_by_node(struct device_node *node)
 
 #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
 
 #endif
-- 
2.7.4

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

[PATCH v13 3/3] drm/tinydrm: Add devres versions of backlight_get

[Meghana Madhyastha]

Add devm_backlight_get 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 v13:
 -Add devm_backlight_put to backlight.c

 drivers/gpu/drm/tinydrm/mi0283qt.c  |  2 +-
 drivers/video/backlight/backlight.c | 31 +++++++++++++++++++++++++++++++
 include/linux/backlight.h           |  6 ++++++
 3 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
index a932185..e3e7583 100644
--- a/drivers/gpu/drm/tinydrm/mi0283qt.c
+++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
@@ -189,7 +189,7 @@ static int mi0283qt_probe(struct spi_device *spi)
 	if (IS_ERR(mipi->regulator))
 		return PTR_ERR(mipi->regulator);
 
-	mipi->backlight = backlight_get(dev);
+	mipi->backlight = devm_backlight_get(dev);
 	if (IS_ERR(mipi->backlight))
 		return PTR_ERR(mipi->backlight);
 
diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index c4e94d0..b6c505a 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -617,6 +617,37 @@ struct backlight_device *backlight_get(struct device *dev)
 }
 EXPORT_SYMBOL(backlight_get);
 
+static void devm_backlight_put(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_put, bd);
+	if (ret) {
+		backlight_put(bd);
+		return ERR_PTR(ret);
+	}
+
+	return bd;
+}
+EXPORT_SYMBOL(devm_backlight_get);
+
 static void __exit backlight_class_exit(void)
 {
 	class_destroy(backlight_class);
diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index 987a6d7..8db9ba9 100644
--- a/include/linux/backlight.h
+++ b/include/linux/backlight.h
@@ -214,11 +214,17 @@ of_find_backlight_by_node(struct device_node *node)
 
 #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
 
 #endif
-- 
2.7.4

Re: [PATCH v13 1/3] drm/tinydrm: Move helper functions from tinydrm-helpers to backlight.h

[Daniel Thompson]

On 13/10/17 11:40, Meghana Madhyastha wrote:
> Move the helper functions enable_backlight and disable_backlight
> from tinydrm-helpers.c to backlight.h as static inline functions so
> that they can be used by other drivers.
> 
> Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
> ---
> <snip>
> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> index 5f2fd61..b88fabb 100644
> --- a/include/linux/backlight.h
> +++ b/include/linux/backlight.h
> @@ -129,6 +129,36 @@ static inline int backlight_update_status(struct backlight_device *bd)
>   	return ret;
>   }
>   
> +/**
> + * 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;

I wonder if we should be updating bd->probs.fb_blank here?

The semantics around bd->probs.fb_blank is a bit nasty (alright, a lot 
nasty). Essentially it's a duplicated copy of the BL_CORE_FBBLANK bit 
with a different bit pattern. Its part of a refactoring that git tells 
me has been 8 years in the making (and counting)...

However, so long as we've got it, I would prefer to keep it consistent 
with the BL_CORE_FBBLANK bit, both here...


> +
> +	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;
... and here.


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

Re: [PATCH v13 1/3] drm/tinydrm: Move helper functions from tinydrm-helpers to backlight.h

[Daniel Thompson]

On 13/10/17 11:40, Meghana Madhyastha wrote:
> Move the helper functions enable_backlight and disable_backlight
> from tinydrm-helpers.c to backlight.h as static inline functions so
> that they can be used by other drivers.
> 
> Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
> ---
> <snip>
> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> index 5f2fd61..b88fabb 100644
> --- a/include/linux/backlight.h
> +++ b/include/linux/backlight.h
> @@ -129,6 +129,36 @@ static inline int backlight_update_status(struct backlight_device *bd)
>   	return ret;
>   }
>   
> +/**
> + * 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;

I wonder if we should be updating bd->probs.fb_blank here?

The semantics around bd->probs.fb_blank is a bit nasty (alright, a lot 
nasty). Essentially it's a duplicated copy of the BL_CORE_FBBLANK bit 
with a different bit pattern. Its part of a refactoring that git tells 
me has been 8 years in the making (and counting)...

However, so long as we've got it, I would prefer to keep it consistent 
with the BL_CORE_FBBLANK bit, both here...


> +
> +	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;
... and here.


Daniel.

Re: [PATCH v13 2/3] drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c

[Daniel Thompson]

On 13/10/17 11:41, Meghana Madhyastha wrote:
> Rename tinydrm_of_find_backlight to backlight_get and move it
> to linux/backlight.c so that it can be used by other drivers.
> 
> Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>

Acked-by: Daniel Thompson <daniel.thompson@linaro.org>


> ---
> Changes in v13:
>   -Add backlight_put to backlight.h in this patch
> 
>   drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 40 --------------------------
>   drivers/gpu/drm/tinydrm/mi0283qt.c             |  3 +-
>   drivers/video/backlight/backlight.c            | 37 ++++++++++++++++++++++++
>   include/drm/tinydrm/tinydrm-helpers.h          |  2 --
>   include/linux/backlight.h                      | 19 ++++++++++++
>   5 files changed, 58 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> index a42dee6..cb1a01a 100644
> --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> @@ -236,46 +236,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);
> -
>   #if IS_ENABLED(CONFIG_SPI)
>   
>   /**
> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
> index 7fd2691..a932185 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 <linux/backlight.h>
>   #include <linux/delay.h>
>   #include <linux/gpio/consumer.h>
>   #include <linux/module.h>
> @@ -188,7 +189,7 @@ static int mi0283qt_probe(struct spi_device *spi)
>   	if (IS_ERR(mipi->regulator))
>   		return PTR_ERR(mipi->regulator);
>   
> -	mipi->backlight = tinydrm_of_find_backlight(dev);
> +	mipi->backlight = backlight_get(dev);
>   	if (IS_ERR(mipi->backlight))
>   		return PTR_ERR(mipi->backlight);
>   
> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
> index 8049e76..c4e94d0 100644
> --- a/drivers/video/backlight/backlight.c
> +++ b/drivers/video/backlight/backlight.c
> @@ -580,6 +580,43 @@ struct backlight_device *of_find_backlight_by_node(struct device_node *node)
>   EXPORT_SYMBOL(of_find_backlight_by_node);
>   #endif
>   
> +/**
> + * 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 __exit backlight_class_exit(void)
>   {
>   	class_destroy(backlight_class);
> diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h
> index f54fae0..0a4ddbc 100644
> --- a/include/drm/tinydrm/tinydrm-helpers.h
> +++ b/include/drm/tinydrm/tinydrm-helpers.h
> @@ -46,8 +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);
> -
>   size_t tinydrm_spi_max_transfer_size(struct spi_device *spi, size_t max_len);
>   bool tinydrm_spi_bpw_supported(struct spi_device *spi, u8 bpw);
>   int tinydrm_spi_transfer(struct spi_device *spi, u32 speed_hz,
> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> index b88fabb..987a6d7 100644
> --- a/include/linux/backlight.h
> +++ b/include/linux/backlight.h
> @@ -159,6 +159,16 @@ static inline int backlight_disable(struct backlight_device *bd)
>   	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);
> +}
> +
>   extern struct backlight_device *backlight_device_register(const char *name,
>   	struct device *dev, void *devdata, const struct backlight_ops *ops,
>   	const struct backlight_properties *props);
> @@ -202,4 +212,13 @@ of_find_backlight_by_node(struct device_node *node)
>   }
>   #endif
>   
> +#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
> +struct backlight_device *backlight_get(struct device *dev);
> +#else
> +static inline struct backlight_device *backlight_get(struct device *dev)
> +{
> +	return NULL;
> +}
> +#endif
> +
>   #endif
> 

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

Re: [PATCH v13 2/3] drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c

[Daniel Thompson]

On 13/10/17 11:41, Meghana Madhyastha wrote:
> Rename tinydrm_of_find_backlight to backlight_get and move it
> to linux/backlight.c so that it can be used by other drivers.
> 
> Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>

Acked-by: Daniel Thompson <daniel.thompson@linaro.org>


> ---
> Changes in v13:
>   -Add backlight_put to backlight.h in this patch
> 
>   drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 40 --------------------------
>   drivers/gpu/drm/tinydrm/mi0283qt.c             |  3 +-
>   drivers/video/backlight/backlight.c            | 37 ++++++++++++++++++++++++
>   include/drm/tinydrm/tinydrm-helpers.h          |  2 --
>   include/linux/backlight.h                      | 19 ++++++++++++
>   5 files changed, 58 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> index a42dee6..cb1a01a 100644
> --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> @@ -236,46 +236,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);
> -
>   #if IS_ENABLED(CONFIG_SPI)
>   
>   /**
> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
> index 7fd2691..a932185 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 <linux/backlight.h>
>   #include <linux/delay.h>
>   #include <linux/gpio/consumer.h>
>   #include <linux/module.h>
> @@ -188,7 +189,7 @@ static int mi0283qt_probe(struct spi_device *spi)
>   	if (IS_ERR(mipi->regulator))
>   		return PTR_ERR(mipi->regulator);
>   
> -	mipi->backlight = tinydrm_of_find_backlight(dev);
> +	mipi->backlight = backlight_get(dev);
>   	if (IS_ERR(mipi->backlight))
>   		return PTR_ERR(mipi->backlight);
>   
> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
> index 8049e76..c4e94d0 100644
> --- a/drivers/video/backlight/backlight.c
> +++ b/drivers/video/backlight/backlight.c
> @@ -580,6 +580,43 @@ struct backlight_device *of_find_backlight_by_node(struct device_node *node)
>   EXPORT_SYMBOL(of_find_backlight_by_node);
>   #endif
>   
> +/**
> + * 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 __exit backlight_class_exit(void)
>   {
>   	class_destroy(backlight_class);
> diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h
> index f54fae0..0a4ddbc 100644
> --- a/include/drm/tinydrm/tinydrm-helpers.h
> +++ b/include/drm/tinydrm/tinydrm-helpers.h
> @@ -46,8 +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);
> -
>   size_t tinydrm_spi_max_transfer_size(struct spi_device *spi, size_t max_len);
>   bool tinydrm_spi_bpw_supported(struct spi_device *spi, u8 bpw);
>   int tinydrm_spi_transfer(struct spi_device *spi, u32 speed_hz,
> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> index b88fabb..987a6d7 100644
> --- a/include/linux/backlight.h
> +++ b/include/linux/backlight.h
> @@ -159,6 +159,16 @@ static inline int backlight_disable(struct backlight_device *bd)
>   	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);
> +}
> +
>   extern struct backlight_device *backlight_device_register(const char *name,
>   	struct device *dev, void *devdata, const struct backlight_ops *ops,
>   	const struct backlight_properties *props);
> @@ -202,4 +212,13 @@ of_find_backlight_by_node(struct device_node *node)
>   }
>   #endif
>   
> +#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
> +struct backlight_device *backlight_get(struct device *dev);
> +#else
> +static inline struct backlight_device *backlight_get(struct device *dev)
> +{
> +	return NULL;
> +}
> +#endif
> +
>   #endif
> 

Re: [PATCH v13 3/3] drm/tinydrm: Add devres versions of backlight_get

[Daniel Thompson]

On 13/10/17 11:42, Meghana Madhyastha wrote:
> Add devm_backlight_get and the corresponding release
> function because some drivers use devres versions of functions
> for requiring device resources.

s/requiring/acquiring/


> Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>

With the above fixed:
Acked-by: Daniel Thompson <daniel.thompson@linaro.org>


> ---
> Changes in v13:
>   -Add devm_backlight_put to backlight.c
> 
>   drivers/gpu/drm/tinydrm/mi0283qt.c  |  2 +-
>   drivers/video/backlight/backlight.c | 31 +++++++++++++++++++++++++++++++
>   include/linux/backlight.h           |  6 ++++++
>   3 files changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
> index a932185..e3e7583 100644
> --- a/drivers/gpu/drm/tinydrm/mi0283qt.c
> +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
> @@ -189,7 +189,7 @@ static int mi0283qt_probe(struct spi_device *spi)
>   	if (IS_ERR(mipi->regulator))
>   		return PTR_ERR(mipi->regulator);
>   
> -	mipi->backlight = backlight_get(dev);
> +	mipi->backlight = devm_backlight_get(dev);
>   	if (IS_ERR(mipi->backlight))
>   		return PTR_ERR(mipi->backlight);
>   
> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
> index c4e94d0..b6c505a 100644
> --- a/drivers/video/backlight/backlight.c
> +++ b/drivers/video/backlight/backlight.c
> @@ -617,6 +617,37 @@ struct backlight_device *backlight_get(struct device *dev)
>   }
>   EXPORT_SYMBOL(backlight_get);
>   
> +static void devm_backlight_put(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_put, bd);
> +	if (ret) {
> +		backlight_put(bd);
> +		return ERR_PTR(ret);
> +	}
> +
> +	return bd;
> +}
> +EXPORT_SYMBOL(devm_backlight_get);
> +
>   static void __exit backlight_class_exit(void)
>   {
>   	class_destroy(backlight_class);
> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> index 987a6d7..8db9ba9 100644
> --- a/include/linux/backlight.h
> +++ b/include/linux/backlight.h
> @@ -214,11 +214,17 @@ of_find_backlight_by_node(struct device_node *node)
>   
>   #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
>   
>   #endif
> 

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

Re: [PATCH v13 3/3] drm/tinydrm: Add devres versions of backlight_get

[Daniel Thompson]

On 13/10/17 11:42, Meghana Madhyastha wrote:
> Add devm_backlight_get and the corresponding release
> function because some drivers use devres versions of functions
> for requiring device resources.

s/requiring/acquiring/


> Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>

With the above fixed:
Acked-by: Daniel Thompson <daniel.thompson@linaro.org>


> ---
> Changes in v13:
>   -Add devm_backlight_put to backlight.c
> 
>   drivers/gpu/drm/tinydrm/mi0283qt.c  |  2 +-
>   drivers/video/backlight/backlight.c | 31 +++++++++++++++++++++++++++++++
>   include/linux/backlight.h           |  6 ++++++
>   3 files changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
> index a932185..e3e7583 100644
> --- a/drivers/gpu/drm/tinydrm/mi0283qt.c
> +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
> @@ -189,7 +189,7 @@ static int mi0283qt_probe(struct spi_device *spi)
>   	if (IS_ERR(mipi->regulator))
>   		return PTR_ERR(mipi->regulator);
>   
> -	mipi->backlight = backlight_get(dev);
> +	mipi->backlight = devm_backlight_get(dev);
>   	if (IS_ERR(mipi->backlight))
>   		return PTR_ERR(mipi->backlight);
>   
> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
> index c4e94d0..b6c505a 100644
> --- a/drivers/video/backlight/backlight.c
> +++ b/drivers/video/backlight/backlight.c
> @@ -617,6 +617,37 @@ struct backlight_device *backlight_get(struct device *dev)
>   }
>   EXPORT_SYMBOL(backlight_get);
>   
> +static void devm_backlight_put(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_put, bd);
> +	if (ret) {
> +		backlight_put(bd);
> +		return ERR_PTR(ret);
> +	}
> +
> +	return bd;
> +}
> +EXPORT_SYMBOL(devm_backlight_get);
> +
>   static void __exit backlight_class_exit(void)
>   {
>   	class_destroy(backlight_class);
> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> index 987a6d7..8db9ba9 100644
> --- a/include/linux/backlight.h
> +++ b/include/linux/backlight.h
> @@ -214,11 +214,17 @@ of_find_backlight_by_node(struct device_node *node)
>   
>   #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
>   
>   #endif
> 

Re: [PATCH v13 0/3] Move backlight helper functions from tinydrm-helpers to linux/backlight

[Daniel Vetter]

Hi Meghana,

On Fri, Oct 13, 2017 at 04:09:27PM +0530, Meghana Madhyastha wrote:
> Move drm helper functions from tinydrm-helpers to linux/backlight for
> ease of use by callers in other drivers.

I've been a bit overloaded past few days, so didn't get around to looking
at any of your patches yet, but just a quick suggestion on process: Please
don't resend complicated patches this quickly. It takes various reviewers
a while to grasp the latest updates and complications, often they first
need to find a bit of time to really think things through. So if you
resend multiple times per day it gets overwhelming and confusing for
reviewers. E.g. on your work here I think about 4 different people
comment, but all in different threads, so harder to get towards some
consensus.

Your enthusiasm and drive is great here, but please keep in mind that the
people reviewing your patches do this more or less in their spare time
(Sean&me blocked some time for outreachy, but can't go through it all
every day).

I think Sean volunteered to take a detailed look at your series, pls ping
him on irc to coordinate patch reworks (if that's still needed).

Thanks, Daniel

> Changes in v13:
> -Rebase against drm-misc
> -Put devm_backlight_put back
> 
> Meghana Madhyastha (3):
>   drm/tinydrm: Move helper functions from tinydrm-helpers to backlight.h
>   drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c
>   drm/tinydrm: Add devres versions of backlight_get
> 
>  drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 95 --------------------------
>  drivers/gpu/drm/tinydrm/mi0283qt.c             |  3 +-
>  drivers/gpu/drm/tinydrm/mipi-dbi.c             |  5 +-
>  drivers/video/backlight/backlight.c            | 68 ++++++++++++++++++
>  include/drm/tinydrm/tinydrm-helpers.h          |  4 --
>  include/linux/backlight.h                      | 55 +++++++++++++++
>  6 files changed, 128 insertions(+), 102 deletions(-)
> 
> -- 
> 2.7.4
> 

-- 
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 v13 0/3] Move backlight helper functions from tinydrm-helpers to linux/backlight

[Daniel Vetter]

Hi Meghana,

On Fri, Oct 13, 2017 at 04:09:27PM +0530, Meghana Madhyastha wrote:
> Move drm helper functions from tinydrm-helpers to linux/backlight for
> ease of use by callers in other drivers.

I've been a bit overloaded past few days, so didn't get around to looking
at any of your patches yet, but just a quick suggestion on process: Please
don't resend complicated patches this quickly. It takes various reviewers
a while to grasp the latest updates and complications, often they first
need to find a bit of time to really think things through. So if you
resend multiple times per day it gets overwhelming and confusing for
reviewers. E.g. on your work here I think about 4 different people
comment, but all in different threads, so harder to get towards some
consensus.

Your enthusiasm and drive is great here, but please keep in mind that the
people reviewing your patches do this more or less in their spare time
(Sean&me blocked some time for outreachy, but can't go through it all
every day).

I think Sean volunteered to take a detailed look at your series, pls ping
him on irc to coordinate patch reworks (if that's still needed).

Thanks, Daniel

> Changes in v13:
> -Rebase against drm-misc
> -Put devm_backlight_put back
> 
> Meghana Madhyastha (3):
>   drm/tinydrm: Move helper functions from tinydrm-helpers to backlight.h
>   drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c
>   drm/tinydrm: Add devres versions of backlight_get
> 
>  drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 95 --------------------------
>  drivers/gpu/drm/tinydrm/mi0283qt.c             |  3 +-
>  drivers/gpu/drm/tinydrm/mipi-dbi.c             |  5 +-
>  drivers/video/backlight/backlight.c            | 68 ++++++++++++++++++
>  include/drm/tinydrm/tinydrm-helpers.h          |  4 --
>  include/linux/backlight.h                      | 55 +++++++++++++++
>  6 files changed, 128 insertions(+), 102 deletions(-)
> 
> -- 
> 2.7.4
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Outreachy kernel] [PATCH v13 2/3] drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c

[Sean Paul]

On Fri, Oct 13, 2017 at 04:11:43PM +0530, Meghana Madhyastha wrote:
> Rename tinydrm_of_find_backlight to backlight_get and move it
> to linux/backlight.c so that it can be used by other drivers.

[apologies if this has been brought up in previous versions, I haven't been
following closely]

I don't think "backlight_get" is a good name for this function. How about
of_find_backlight_by_name (since there's already of_find_backlight_by_node)?

> 
> Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
> ---
> Changes in v13:
>  -Add backlight_put to backlight.h in this patch
> 
>  drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 40 --------------------------
>  drivers/gpu/drm/tinydrm/mi0283qt.c             |  3 +-
>  drivers/video/backlight/backlight.c            | 37 ++++++++++++++++++++++++
>  include/drm/tinydrm/tinydrm-helpers.h          |  2 --
>  include/linux/backlight.h                      | 19 ++++++++++++
>  5 files changed, 58 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> index a42dee6..cb1a01a 100644
> --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> @@ -236,46 +236,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);
> -
>  #if IS_ENABLED(CONFIG_SPI)
>  
>  /**
> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
> index 7fd2691..a932185 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 <linux/backlight.h>
>  #include <linux/delay.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/module.h>
> @@ -188,7 +189,7 @@ static int mi0283qt_probe(struct spi_device *spi)
>  	if (IS_ERR(mipi->regulator))
>  		return PTR_ERR(mipi->regulator);
>  
> -	mipi->backlight = tinydrm_of_find_backlight(dev);
> +	mipi->backlight = backlight_get(dev);
>  	if (IS_ERR(mipi->backlight))
>  		return PTR_ERR(mipi->backlight);
>  
> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
> index 8049e76..c4e94d0 100644
> --- a/drivers/video/backlight/backlight.c
> +++ b/drivers/video/backlight/backlight.c
> @@ -580,6 +580,43 @@ struct backlight_device *of_find_backlight_by_node(struct device_node *node)
>  EXPORT_SYMBOL(of_find_backlight_by_node);
>  #endif
>  
> +/**
> + * 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) {

I'm not really used to seeing IS_ENABLED(CONFIG_BLAH) inline. The common
patterns seems to be wrapping the actual implementation in #if
IS_ENABLED(CONFIG_BLAH) and then sticking a stub implementation in the #else.

I see below that you already have a stub if backlight is not enabled, so expand
that #if to include CONFIG_OF as well.

> +		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 __exit backlight_class_exit(void)
>  {
>  	class_destroy(backlight_class);
> diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h
> index f54fae0..0a4ddbc 100644
> --- a/include/drm/tinydrm/tinydrm-helpers.h
> +++ b/include/drm/tinydrm/tinydrm-helpers.h
> @@ -46,8 +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);
> -
>  size_t tinydrm_spi_max_transfer_size(struct spi_device *spi, size_t max_len);
>  bool tinydrm_spi_bpw_supported(struct spi_device *spi, u8 bpw);
>  int tinydrm_spi_transfer(struct spi_device *spi, u32 speed_hz,
> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> index b88fabb..987a6d7 100644
> --- a/include/linux/backlight.h
> +++ b/include/linux/backlight.h
> @@ -159,6 +159,16 @@ static inline int backlight_disable(struct backlight_device *bd)
>  	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);
> +}

I'm not convinced this function needs to exist.

> +
>  extern struct backlight_device *backlight_device_register(const char *name,
>  	struct device *dev, void *devdata, const struct backlight_ops *ops,
>  	const struct backlight_properties *props);
> @@ -202,4 +212,13 @@ of_find_backlight_by_node(struct device_node *node)
>  }
>  #endif
>  
> +#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
> +struct backlight_device *backlight_get(struct device *dev);
> +#else
> +static inline struct backlight_device *backlight_get(struct device *dev)
> +{
> +	return NULL;
> +}
> +#endif
> +
>  #endif
> -- 
> 2.7.4
> 
> -- 
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/e2ef05bf46438427d08c6a61fe5058156800db95.1507890285.git.meghana.madhyastha%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Outreachy kernel] [PATCH v13 2/3] drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c

[Sean Paul]

On Fri, Oct 13, 2017 at 04:11:43PM +0530, Meghana Madhyastha wrote:
> Rename tinydrm_of_find_backlight to backlight_get and move it
> to linux/backlight.c so that it can be used by other drivers.

[apologies if this has been brought up in previous versions, I haven't been
following closely]

I don't think "backlight_get" is a good name for this function. How about
of_find_backlight_by_name (since there's already of_find_backlight_by_node)?

> 
> Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
> ---
> Changes in v13:
>  -Add backlight_put to backlight.h in this patch
> 
>  drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 40 --------------------------
>  drivers/gpu/drm/tinydrm/mi0283qt.c             |  3 +-
>  drivers/video/backlight/backlight.c            | 37 ++++++++++++++++++++++++
>  include/drm/tinydrm/tinydrm-helpers.h          |  2 --
>  include/linux/backlight.h                      | 19 ++++++++++++
>  5 files changed, 58 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> index a42dee6..cb1a01a 100644
> --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> @@ -236,46 +236,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);
> -
>  #if IS_ENABLED(CONFIG_SPI)
>  
>  /**
> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
> index 7fd2691..a932185 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 <linux/backlight.h>
>  #include <linux/delay.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/module.h>
> @@ -188,7 +189,7 @@ static int mi0283qt_probe(struct spi_device *spi)
>  	if (IS_ERR(mipi->regulator))
>  		return PTR_ERR(mipi->regulator);
>  
> -	mipi->backlight = tinydrm_of_find_backlight(dev);
> +	mipi->backlight = backlight_get(dev);
>  	if (IS_ERR(mipi->backlight))
>  		return PTR_ERR(mipi->backlight);
>  
> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
> index 8049e76..c4e94d0 100644
> --- a/drivers/video/backlight/backlight.c
> +++ b/drivers/video/backlight/backlight.c
> @@ -580,6 +580,43 @@ struct backlight_device *of_find_backlight_by_node(struct device_node *node)
>  EXPORT_SYMBOL(of_find_backlight_by_node);
>  #endif
>  
> +/**
> + * 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) {

I'm not really used to seeing IS_ENABLED(CONFIG_BLAH) inline. The common
patterns seems to be wrapping the actual implementation in #if
IS_ENABLED(CONFIG_BLAH) and then sticking a stub implementation in the #else.

I see below that you already have a stub if backlight is not enabled, so expand
that #if to include CONFIG_OF as well.

> +		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 __exit backlight_class_exit(void)
>  {
>  	class_destroy(backlight_class);
> diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h
> index f54fae0..0a4ddbc 100644
> --- a/include/drm/tinydrm/tinydrm-helpers.h
> +++ b/include/drm/tinydrm/tinydrm-helpers.h
> @@ -46,8 +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);
> -
>  size_t tinydrm_spi_max_transfer_size(struct spi_device *spi, size_t max_len);
>  bool tinydrm_spi_bpw_supported(struct spi_device *spi, u8 bpw);
>  int tinydrm_spi_transfer(struct spi_device *spi, u32 speed_hz,
> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> index b88fabb..987a6d7 100644
> --- a/include/linux/backlight.h
> +++ b/include/linux/backlight.h
> @@ -159,6 +159,16 @@ static inline int backlight_disable(struct backlight_device *bd)
>  	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);
> +}

I'm not convinced this function needs to exist.

> +
>  extern struct backlight_device *backlight_device_register(const char *name,
>  	struct device *dev, void *devdata, const struct backlight_ops *ops,
>  	const struct backlight_properties *props);
> @@ -202,4 +212,13 @@ of_find_backlight_by_node(struct device_node *node)
>  }
>  #endif
>  
> +#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
> +struct backlight_device *backlight_get(struct device *dev);
> +#else
> +static inline struct backlight_device *backlight_get(struct device *dev)
> +{
> +	return NULL;
> +}
> +#endif
> +
>  #endif
> -- 
> 2.7.4
> 
> -- 
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/e2ef05bf46438427d08c6a61fe5058156800db95.1507890285.git.meghana.madhyastha%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [Outreachy kernel] [PATCH v13 3/3] drm/tinydrm: Add devres versions of backlight_get

[Sean Paul]

On Fri, Oct 13, 2017 at 04:12:55PM +0530, Meghana Madhyastha wrote:
> Add devm_backlight_get and the corresponding release
> function because some drivers use devres versions of functions
> for requiring device resources.
> 

This patch looks fine, just update the names to be consistent with the previous
version and account for backlight_put not being present.

Sean

> Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
> ---
> Changes in v13:
>  -Add devm_backlight_put to backlight.c
> 
>  drivers/gpu/drm/tinydrm/mi0283qt.c  |  2 +-
>  drivers/video/backlight/backlight.c | 31 +++++++++++++++++++++++++++++++
>  include/linux/backlight.h           |  6 ++++++
>  3 files changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
> index a932185..e3e7583 100644
> --- a/drivers/gpu/drm/tinydrm/mi0283qt.c
> +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
> @@ -189,7 +189,7 @@ static int mi0283qt_probe(struct spi_device *spi)
>  	if (IS_ERR(mipi->regulator))
>  		return PTR_ERR(mipi->regulator);
>  
> -	mipi->backlight = backlight_get(dev);
> +	mipi->backlight = devm_backlight_get(dev);
>  	if (IS_ERR(mipi->backlight))
>  		return PTR_ERR(mipi->backlight);
>  
> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
> index c4e94d0..b6c505a 100644
> --- a/drivers/video/backlight/backlight.c
> +++ b/drivers/video/backlight/backlight.c
> @@ -617,6 +617,37 @@ struct backlight_device *backlight_get(struct device *dev)
>  }
>  EXPORT_SYMBOL(backlight_get);
>  
> +static void devm_backlight_put(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_put, bd);
> +	if (ret) {
> +		backlight_put(bd);
> +		return ERR_PTR(ret);
> +	}
> +
> +	return bd;
> +}
> +EXPORT_SYMBOL(devm_backlight_get);
> +
>  static void __exit backlight_class_exit(void)
>  {
>  	class_destroy(backlight_class);
> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> index 987a6d7..8db9ba9 100644
> --- a/include/linux/backlight.h
> +++ b/include/linux/backlight.h
> @@ -214,11 +214,17 @@ of_find_backlight_by_node(struct device_node *node)
>  
>  #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
>  
>  #endif
> -- 
> 2.7.4
> 
> -- 
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/4b1acbfbddd2a123001e035a62801456891276d5.1507890285.git.meghana.madhyastha%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Outreachy kernel] [PATCH v13 3/3] drm/tinydrm: Add devres versions of backlight_get

[Sean Paul]

On Fri, Oct 13, 2017 at 04:12:55PM +0530, Meghana Madhyastha wrote:
> Add devm_backlight_get and the corresponding release
> function because some drivers use devres versions of functions
> for requiring device resources.
> 

This patch looks fine, just update the names to be consistent with the previous
version and account for backlight_put not being present.

Sean

> Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
> ---
> Changes in v13:
>  -Add devm_backlight_put to backlight.c
> 
>  drivers/gpu/drm/tinydrm/mi0283qt.c  |  2 +-
>  drivers/video/backlight/backlight.c | 31 +++++++++++++++++++++++++++++++
>  include/linux/backlight.h           |  6 ++++++
>  3 files changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
> index a932185..e3e7583 100644
> --- a/drivers/gpu/drm/tinydrm/mi0283qt.c
> +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
> @@ -189,7 +189,7 @@ static int mi0283qt_probe(struct spi_device *spi)
>  	if (IS_ERR(mipi->regulator))
>  		return PTR_ERR(mipi->regulator);
>  
> -	mipi->backlight = backlight_get(dev);
> +	mipi->backlight = devm_backlight_get(dev);
>  	if (IS_ERR(mipi->backlight))
>  		return PTR_ERR(mipi->backlight);
>  
> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
> index c4e94d0..b6c505a 100644
> --- a/drivers/video/backlight/backlight.c
> +++ b/drivers/video/backlight/backlight.c
> @@ -617,6 +617,37 @@ struct backlight_device *backlight_get(struct device *dev)
>  }
>  EXPORT_SYMBOL(backlight_get);
>  
> +static void devm_backlight_put(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_put, bd);
> +	if (ret) {
> +		backlight_put(bd);
> +		return ERR_PTR(ret);
> +	}
> +
> +	return bd;
> +}
> +EXPORT_SYMBOL(devm_backlight_get);
> +
>  static void __exit backlight_class_exit(void)
>  {
>  	class_destroy(backlight_class);
> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> index 987a6d7..8db9ba9 100644
> --- a/include/linux/backlight.h
> +++ b/include/linux/backlight.h
> @@ -214,11 +214,17 @@ of_find_backlight_by_node(struct device_node *node)
>  
>  #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
>  
>  #endif
> -- 
> 2.7.4
> 
> -- 
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/4b1acbfbddd2a123001e035a62801456891276d5.1507890285.git.meghana.madhyastha%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [Outreachy kernel] [PATCH v13 2/3] drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c

[Noralf Trønnes]

Den 13.10.2017 22.25, skrev Sean Paul:
> On Fri, Oct 13, 2017 at 04:11:43PM +0530, Meghana Madhyastha wrote:
>> Rename tinydrm_of_find_backlight to backlight_get and move it
>> to linux/backlight.c so that it can be used by other drivers.
> [apologies if this has been brought up in previous versions, I haven't been
> following closely]
>
> I don't think "backlight_get" is a good name for this function. How about
> of_find_backlight_by_name (since there's already of_find_backlight_by_node)?

I came up with that name modelled after gpiod_get() and gpiod_put() and I
deliberately kept the of_ part out of the name like the gpio functions.
gpiod_get() checks OF, ACPI and platform for gpios and calling it
backlight_get() would keep the door open for other ways of connecting
backlight devices in the future, other than Device Tree.

I think of_find_backlight() would be better than *_by_name(), since
'backlight' is the common DT property name, so it wouldn't make much sense
to require every caller to pass in the same name.

Either way is fine with me.

Noralf.


>> Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
>> ---
>> Changes in v13:
>>   -Add backlight_put to backlight.h in this patch
>>
>>   drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 40 --------------------------
>>   drivers/gpu/drm/tinydrm/mi0283qt.c             |  3 +-
>>   drivers/video/backlight/backlight.c            | 37 ++++++++++++++++++++++++
>>   include/drm/tinydrm/tinydrm-helpers.h          |  2 --
>>   include/linux/backlight.h                      | 19 ++++++++++++
>>   5 files changed, 58 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>> index a42dee6..cb1a01a 100644
>> --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>> @@ -236,46 +236,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);
>> -
>>   #if IS_ENABLED(CONFIG_SPI)
>>   
>>   /**
>> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
>> index 7fd2691..a932185 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 <linux/backlight.h>
>>   #include <linux/delay.h>
>>   #include <linux/gpio/consumer.h>
>>   #include <linux/module.h>
>> @@ -188,7 +189,7 @@ static int mi0283qt_probe(struct spi_device *spi)
>>   	if (IS_ERR(mipi->regulator))
>>   		return PTR_ERR(mipi->regulator);
>>   
>> -	mipi->backlight = tinydrm_of_find_backlight(dev);
>> +	mipi->backlight = backlight_get(dev);
>>   	if (IS_ERR(mipi->backlight))
>>   		return PTR_ERR(mipi->backlight);
>>   
>> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
>> index 8049e76..c4e94d0 100644
>> --- a/drivers/video/backlight/backlight.c
>> +++ b/drivers/video/backlight/backlight.c
>> @@ -580,6 +580,43 @@ struct backlight_device *of_find_backlight_by_node(struct device_node *node)
>>   EXPORT_SYMBOL(of_find_backlight_by_node);
>>   #endif
>>   
>> +/**
>> + * 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) {
> I'm not really used to seeing IS_ENABLED(CONFIG_BLAH) inline. The common
> patterns seems to be wrapping the actual implementation in #if
> IS_ENABLED(CONFIG_BLAH) and then sticking a stub implementation in the #else.
>
> I see below that you already have a stub if backlight is not enabled, so expand
> that #if to include CONFIG_OF as well.
>
>> +		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 __exit backlight_class_exit(void)
>>   {
>>   	class_destroy(backlight_class);
>> diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h
>> index f54fae0..0a4ddbc 100644
>> --- a/include/drm/tinydrm/tinydrm-helpers.h
>> +++ b/include/drm/tinydrm/tinydrm-helpers.h
>> @@ -46,8 +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);
>> -
>>   size_t tinydrm_spi_max_transfer_size(struct spi_device *spi, size_t max_len);
>>   bool tinydrm_spi_bpw_supported(struct spi_device *spi, u8 bpw);
>>   int tinydrm_spi_transfer(struct spi_device *spi, u32 speed_hz,
>> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
>> index b88fabb..987a6d7 100644
>> --- a/include/linux/backlight.h
>> +++ b/include/linux/backlight.h
>> @@ -159,6 +159,16 @@ static inline int backlight_disable(struct backlight_device *bd)
>>   	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);
>> +}
> I'm not convinced this function needs to exist.
>
>> +
>>   extern struct backlight_device *backlight_device_register(const char *name,
>>   	struct device *dev, void *devdata, const struct backlight_ops *ops,
>>   	const struct backlight_properties *props);
>> @@ -202,4 +212,13 @@ of_find_backlight_by_node(struct device_node *node)
>>   }
>>   #endif
>>   
>> +#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
>> +struct backlight_device *backlight_get(struct device *dev);
>> +#else
>> +static inline struct backlight_device *backlight_get(struct device *dev)
>> +{
>> +	return NULL;
>> +}
>> +#endif
>> +
>>   #endif
>> -- 
>> 2.7.4
>>
>> -- 
>> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
>> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
>> To post to this group, send email to outreachy-kernel@googlegroups.com.
>> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/e2ef05bf46438427d08c6a61fe5058156800db95.1507890285.git.meghana.madhyastha%40gmail.com.
>> For more options, visit https://groups.google.com/d/optout.

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

Re: [Outreachy kernel] [PATCH v13 2/3] drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c

[Noralf Trønnes]

Den 13.10.2017 22.25, skrev Sean Paul:
> On Fri, Oct 13, 2017 at 04:11:43PM +0530, Meghana Madhyastha wrote:
>> Rename tinydrm_of_find_backlight to backlight_get and move it
>> to linux/backlight.c so that it can be used by other drivers.
> [apologies if this has been brought up in previous versions, I haven't been
> following closely]
>
> I don't think "backlight_get" is a good name for this function. How about
> of_find_backlight_by_name (since there's already of_find_backlight_by_node)?

I came up with that name modelled after gpiod_get() and gpiod_put() and I
deliberately kept the of_ part out of the name like the gpio functions.
gpiod_get() checks OF, ACPI and platform for gpios and calling it
backlight_get() would keep the door open for other ways of connecting
backlight devices in the future, other than Device Tree.

I think of_find_backlight() would be better than *_by_name(), since
'backlight' is the common DT property name, so it wouldn't make much sense
to require every caller to pass in the same name.

Either way is fine with me.

Noralf.


>> Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
>> ---
>> Changes in v13:
>>   -Add backlight_put to backlight.h in this patch
>>
>>   drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 40 --------------------------
>>   drivers/gpu/drm/tinydrm/mi0283qt.c             |  3 +-
>>   drivers/video/backlight/backlight.c            | 37 ++++++++++++++++++++++++
>>   include/drm/tinydrm/tinydrm-helpers.h          |  2 --
>>   include/linux/backlight.h                      | 19 ++++++++++++
>>   5 files changed, 58 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>> index a42dee6..cb1a01a 100644
>> --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>> @@ -236,46 +236,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);
>> -
>>   #if IS_ENABLED(CONFIG_SPI)
>>   
>>   /**
>> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
>> index 7fd2691..a932185 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 <linux/backlight.h>
>>   #include <linux/delay.h>
>>   #include <linux/gpio/consumer.h>
>>   #include <linux/module.h>
>> @@ -188,7 +189,7 @@ static int mi0283qt_probe(struct spi_device *spi)
>>   	if (IS_ERR(mipi->regulator))
>>   		return PTR_ERR(mipi->regulator);
>>   
>> -	mipi->backlight = tinydrm_of_find_backlight(dev);
>> +	mipi->backlight = backlight_get(dev);
>>   	if (IS_ERR(mipi->backlight))
>>   		return PTR_ERR(mipi->backlight);
>>   
>> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
>> index 8049e76..c4e94d0 100644
>> --- a/drivers/video/backlight/backlight.c
>> +++ b/drivers/video/backlight/backlight.c
>> @@ -580,6 +580,43 @@ struct backlight_device *of_find_backlight_by_node(struct device_node *node)
>>   EXPORT_SYMBOL(of_find_backlight_by_node);
>>   #endif
>>   
>> +/**
>> + * 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) {
> I'm not really used to seeing IS_ENABLED(CONFIG_BLAH) inline. The common
> patterns seems to be wrapping the actual implementation in #if
> IS_ENABLED(CONFIG_BLAH) and then sticking a stub implementation in the #else.
>
> I see below that you already have a stub if backlight is not enabled, so expand
> that #if to include CONFIG_OF as well.
>
>> +		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 __exit backlight_class_exit(void)
>>   {
>>   	class_destroy(backlight_class);
>> diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h
>> index f54fae0..0a4ddbc 100644
>> --- a/include/drm/tinydrm/tinydrm-helpers.h
>> +++ b/include/drm/tinydrm/tinydrm-helpers.h
>> @@ -46,8 +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);
>> -
>>   size_t tinydrm_spi_max_transfer_size(struct spi_device *spi, size_t max_len);
>>   bool tinydrm_spi_bpw_supported(struct spi_device *spi, u8 bpw);
>>   int tinydrm_spi_transfer(struct spi_device *spi, u32 speed_hz,
>> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
>> index b88fabb..987a6d7 100644
>> --- a/include/linux/backlight.h
>> +++ b/include/linux/backlight.h
>> @@ -159,6 +159,16 @@ static inline int backlight_disable(struct backlight_device *bd)
>>   	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);
>> +}
> I'm not convinced this function needs to exist.
>
>> +
>>   extern struct backlight_device *backlight_device_register(const char *name,
>>   	struct device *dev, void *devdata, const struct backlight_ops *ops,
>>   	const struct backlight_properties *props);
>> @@ -202,4 +212,13 @@ of_find_backlight_by_node(struct device_node *node)
>>   }
>>   #endif
>>   
>> +#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
>> +struct backlight_device *backlight_get(struct device *dev);
>> +#else
>> +static inline struct backlight_device *backlight_get(struct device *dev)
>> +{
>> +	return NULL;
>> +}
>> +#endif
>> +
>>   #endif
>> -- 
>> 2.7.4
>>
>> -- 
>> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
>> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
>> To post to this group, send email to outreachy-kernel@googlegroups.com.
>> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/e2ef05bf46438427d08c6a61fe5058156800db95.1507890285.git.meghana.madhyastha%40gmail.com.
>> For more options, visit https://groups.google.com/d/optout.

Re: [Outreachy kernel] [PATCH v13 2/3] drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c

[Rob Clark]

On Fri, Oct 13, 2017 at 4:25 PM, Sean Paul <seanpaul@chromium.org> wrote:
> On Fri, Oct 13, 2017 at 04:11:43PM +0530, Meghana Madhyastha wrote:
>> Rename tinydrm_of_find_backlight to backlight_get and move it
>> to linux/backlight.c so that it can be used by other drivers.
>
> [apologies if this has been brought up in previous versions, I haven't been
> following closely]
>
> I don't think "backlight_get" is a good name for this function. How about
> of_find_backlight_by_name (since there's already of_find_backlight_by_node)?
>
>>
>> Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
>> ---
>> Changes in v13:
>>  -Add backlight_put to backlight.h in this patch
>>
>>  drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 40 --------------------------
>>  drivers/gpu/drm/tinydrm/mi0283qt.c             |  3 +-
>>  drivers/video/backlight/backlight.c            | 37 ++++++++++++++++++++++++
>>  include/drm/tinydrm/tinydrm-helpers.h          |  2 --
>>  include/linux/backlight.h                      | 19 ++++++++++++
>>  5 files changed, 58 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>> index a42dee6..cb1a01a 100644
>> --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>> @@ -236,46 +236,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);
>> -
>>  #if IS_ENABLED(CONFIG_SPI)
>>
>>  /**
>> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
>> index 7fd2691..a932185 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 <linux/backlight.h>
>>  #include <linux/delay.h>
>>  #include <linux/gpio/consumer.h>
>>  #include <linux/module.h>
>> @@ -188,7 +189,7 @@ static int mi0283qt_probe(struct spi_device *spi)
>>       if (IS_ERR(mipi->regulator))
>>               return PTR_ERR(mipi->regulator);
>>
>> -     mipi->backlight = tinydrm_of_find_backlight(dev);
>> +     mipi->backlight = backlight_get(dev);
>>       if (IS_ERR(mipi->backlight))
>>               return PTR_ERR(mipi->backlight);
>>
>> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
>> index 8049e76..c4e94d0 100644
>> --- a/drivers/video/backlight/backlight.c
>> +++ b/drivers/video/backlight/backlight.c
>> @@ -580,6 +580,43 @@ struct backlight_device *of_find_backlight_by_node(struct device_node *node)
>>  EXPORT_SYMBOL(of_find_backlight_by_node);
>>  #endif
>>
>> +/**
>> + * 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) {
>
> I'm not really used to seeing IS_ENABLED(CONFIG_BLAH) inline. The common
> patterns seems to be wrapping the actual implementation in #if
> IS_ENABLED(CONFIG_BLAH) and then sticking a stub implementation in the #else.


drive-by comment, but I actually prefer 'if (IS_ENABLED(CONFIG_FOO))
..' when possible.. it keeps the code compiled by the front-end of the
compiler in more configs so less likely to bitrot, and the compiler is
plenty good at DCE to eliminate the code when disabled in later stages
of the compiler..

possibly exceptions are things that are called frequently where you
want to inline the CONFIG_FOO=n stub so it can be eliminated without
calling into another object file (at least until LTO gets good), but I
guess that is not the case here..

BR,
-R


> I see below that you already have a stub if backlight is not enabled, so expand
> that #if to include CONFIG_OF as well.
>
>> +             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 __exit backlight_class_exit(void)
>>  {
>>       class_destroy(backlight_class);
>> diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h
>> index f54fae0..0a4ddbc 100644
>> --- a/include/drm/tinydrm/tinydrm-helpers.h
>> +++ b/include/drm/tinydrm/tinydrm-helpers.h
>> @@ -46,8 +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);
>> -
>>  size_t tinydrm_spi_max_transfer_size(struct spi_device *spi, size_t max_len);
>>  bool tinydrm_spi_bpw_supported(struct spi_device *spi, u8 bpw);
>>  int tinydrm_spi_transfer(struct spi_device *spi, u32 speed_hz,
>> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
>> index b88fabb..987a6d7 100644
>> --- a/include/linux/backlight.h
>> +++ b/include/linux/backlight.h
>> @@ -159,6 +159,16 @@ static inline int backlight_disable(struct backlight_device *bd)
>>       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);
>> +}
>
> I'm not convinced this function needs to exist.
>
>> +
>>  extern struct backlight_device *backlight_device_register(const char *name,
>>       struct device *dev, void *devdata, const struct backlight_ops *ops,
>>       const struct backlight_properties *props);
>> @@ -202,4 +212,13 @@ of_find_backlight_by_node(struct device_node *node)
>>  }
>>  #endif
>>
>> +#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
>> +struct backlight_device *backlight_get(struct device *dev);
>> +#else
>> +static inline struct backlight_device *backlight_get(struct device *dev)
>> +{
>> +     return NULL;
>> +}
>> +#endif
>> +
>>  #endif
>> --
>> 2.7.4
>>
>> --
>> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
>> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
>> To post to this group, send email to outreachy-kernel@googlegroups.com.
>> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/e2ef05bf46438427d08c6a61fe5058156800db95.1507890285.git.meghana.madhyastha%40gmail.com.
>> For more options, visit https://groups.google.com/d/optout.
>
> --
> Sean Paul, Software Engineer, Google / Chromium OS
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Outreachy kernel] [PATCH v13 2/3] drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c

[Rob Clark]

On Fri, Oct 13, 2017 at 4:25 PM, Sean Paul <seanpaul@chromium.org> wrote:
> On Fri, Oct 13, 2017 at 04:11:43PM +0530, Meghana Madhyastha wrote:
>> Rename tinydrm_of_find_backlight to backlight_get and move it
>> to linux/backlight.c so that it can be used by other drivers.
>
> [apologies if this has been brought up in previous versions, I haven't been
> following closely]
>
> I don't think "backlight_get" is a good name for this function. How about
> of_find_backlight_by_name (since there's already of_find_backlight_by_node)?
>
>>
>> Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
>> ---
>> Changes in v13:
>>  -Add backlight_put to backlight.h in this patch
>>
>>  drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 40 --------------------------
>>  drivers/gpu/drm/tinydrm/mi0283qt.c             |  3 +-
>>  drivers/video/backlight/backlight.c            | 37 ++++++++++++++++++++++++
>>  include/drm/tinydrm/tinydrm-helpers.h          |  2 --
>>  include/linux/backlight.h                      | 19 ++++++++++++
>>  5 files changed, 58 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>> index a42dee6..cb1a01a 100644
>> --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>> @@ -236,46 +236,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);
>> -
>>  #if IS_ENABLED(CONFIG_SPI)
>>
>>  /**
>> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
>> index 7fd2691..a932185 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 <linux/backlight.h>
>>  #include <linux/delay.h>
>>  #include <linux/gpio/consumer.h>
>>  #include <linux/module.h>
>> @@ -188,7 +189,7 @@ static int mi0283qt_probe(struct spi_device *spi)
>>       if (IS_ERR(mipi->regulator))
>>               return PTR_ERR(mipi->regulator);
>>
>> -     mipi->backlight = tinydrm_of_find_backlight(dev);
>> +     mipi->backlight = backlight_get(dev);
>>       if (IS_ERR(mipi->backlight))
>>               return PTR_ERR(mipi->backlight);
>>
>> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
>> index 8049e76..c4e94d0 100644
>> --- a/drivers/video/backlight/backlight.c
>> +++ b/drivers/video/backlight/backlight.c
>> @@ -580,6 +580,43 @@ struct backlight_device *of_find_backlight_by_node(struct device_node *node)
>>  EXPORT_SYMBOL(of_find_backlight_by_node);
>>  #endif
>>
>> +/**
>> + * 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) {
>
> I'm not really used to seeing IS_ENABLED(CONFIG_BLAH) inline. The common
> patterns seems to be wrapping the actual implementation in #if
> IS_ENABLED(CONFIG_BLAH) and then sticking a stub implementation in the #else.


drive-by comment, but I actually prefer 'if (IS_ENABLED(CONFIG_FOO))
..' when possible.. it keeps the code compiled by the front-end of the
compiler in more configs so less likely to bitrot, and the compiler is
plenty good at DCE to eliminate the code when disabled in later stages
of the compiler..

possibly exceptions are things that are called frequently where you
want to inline the CONFIG_FOO=n stub so it can be eliminated without
calling into another object file (at least until LTO gets good), but I
guess that is not the case here..

BR,
-R


> I see below that you already have a stub if backlight is not enabled, so expand
> that #if to include CONFIG_OF as well.
>
>> +             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 __exit backlight_class_exit(void)
>>  {
>>       class_destroy(backlight_class);
>> diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h
>> index f54fae0..0a4ddbc 100644
>> --- a/include/drm/tinydrm/tinydrm-helpers.h
>> +++ b/include/drm/tinydrm/tinydrm-helpers.h
>> @@ -46,8 +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);
>> -
>>  size_t tinydrm_spi_max_transfer_size(struct spi_device *spi, size_t max_len);
>>  bool tinydrm_spi_bpw_supported(struct spi_device *spi, u8 bpw);
>>  int tinydrm_spi_transfer(struct spi_device *spi, u32 speed_hz,
>> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
>> index b88fabb..987a6d7 100644
>> --- a/include/linux/backlight.h
>> +++ b/include/linux/backlight.h
>> @@ -159,6 +159,16 @@ static inline int backlight_disable(struct backlight_device *bd)
>>       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);
>> +}
>
> I'm not convinced this function needs to exist.
>
>> +
>>  extern struct backlight_device *backlight_device_register(const char *name,
>>       struct device *dev, void *devdata, const struct backlight_ops *ops,
>>       const struct backlight_properties *props);
>> @@ -202,4 +212,13 @@ of_find_backlight_by_node(struct device_node *node)
>>  }
>>  #endif
>>
>> +#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
>> +struct backlight_device *backlight_get(struct device *dev);
>> +#else
>> +static inline struct backlight_device *backlight_get(struct device *dev)
>> +{
>> +     return NULL;
>> +}
>> +#endif
>> +
>>  #endif
>> --
>> 2.7.4
>>
>> --
>> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
>> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
>> To post to this group, send email to outreachy-kernel@googlegroups.com.
>> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/e2ef05bf46438427d08c6a61fe5058156800db95.1507890285.git.meghana.madhyastha%40gmail.com.
>> For more options, visit https://groups.google.com/d/optout.
>
> --
> Sean Paul, Software Engineer, Google / Chromium OS
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Outreachy kernel] [PATCH v13 2/3] drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c

[Sean Paul]

On Fri, Oct 13, 2017 at 6:42 PM, Noralf Trønnes <noralf@tronnes.org> wrote:
>
> Den 13.10.2017 22.25, skrev Sean Paul:
>>
>> On Fri, Oct 13, 2017 at 04:11:43PM +0530, Meghana Madhyastha wrote:
>>>
>>> Rename tinydrm_of_find_backlight to backlight_get and move it
>>> to linux/backlight.c so that it can be used by other drivers.
>>
>> [apologies if this has been brought up in previous versions, I haven't
>> been
>> following closely]
>>
>> I don't think "backlight_get" is a good name for this function. How about
>> of_find_backlight_by_name (since there's already
>> of_find_backlight_by_node)?
>
>
> I came up with that name modelled after gpiod_get() and gpiod_put() and I
> deliberately kept the of_ part out of the name like the gpio functions.
> gpiod_get() checks OF, ACPI and platform for gpios and calling it
> backlight_get() would keep the door open for other ways of connecting
> backlight devices in the future, other than Device Tree.
>

Thanks for the background, Noralf! Apologies for stepping on top of
your previous reviews.

> I think of_find_backlight() would be better than *_by_name(), since
> 'backlight' is the common DT property name, so it wouldn't make much sense
> to require every caller to pass in the same name.
>

of_find_backlight() sounds like a good compromise.

Sean


> Either way is fine with me.
>
> Noralf.
>
>
>
>>> Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
>>> ---
>>> Changes in v13:
>>>   -Add backlight_put to backlight.h in this patch
>>>
>>>   drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 40
>>> --------------------------
>>>   drivers/gpu/drm/tinydrm/mi0283qt.c             |  3 +-
>>>   drivers/video/backlight/backlight.c            | 37
>>> ++++++++++++++++++++++++
>>>   include/drm/tinydrm/tinydrm-helpers.h          |  2 --
>>>   include/linux/backlight.h                      | 19 ++++++++++++
>>>   5 files changed, 58 insertions(+), 43 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>>> b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>>> index a42dee6..cb1a01a 100644
>>> --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>>> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>>> @@ -236,46 +236,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);
>>> -
>>>   #if IS_ENABLED(CONFIG_SPI)
>>>     /**
>>> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c
>>> b/drivers/gpu/drm/tinydrm/mi0283qt.c
>>> index 7fd2691..a932185 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 <linux/backlight.h>
>>>   #include <linux/delay.h>
>>>   #include <linux/gpio/consumer.h>
>>>   #include <linux/module.h>
>>> @@ -188,7 +189,7 @@ static int mi0283qt_probe(struct spi_device *spi)
>>>         if (IS_ERR(mipi->regulator))
>>>                 return PTR_ERR(mipi->regulator);
>>>   -     mipi->backlight = tinydrm_of_find_backlight(dev);
>>> +       mipi->backlight = backlight_get(dev);
>>>         if (IS_ERR(mipi->backlight))
>>>                 return PTR_ERR(mipi->backlight);
>>>   diff --git a/drivers/video/backlight/backlight.c
>>> b/drivers/video/backlight/backlight.c
>>> index 8049e76..c4e94d0 100644
>>> --- a/drivers/video/backlight/backlight.c
>>> +++ b/drivers/video/backlight/backlight.c
>>> @@ -580,6 +580,43 @@ struct backlight_device
>>> *of_find_backlight_by_node(struct device_node *node)
>>>   EXPORT_SYMBOL(of_find_backlight_by_node);
>>>   #endif
>>>   +/**
>>> + * 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) {
>>
>> I'm not really used to seeing IS_ENABLED(CONFIG_BLAH) inline. The common
>> patterns seems to be wrapping the actual implementation in #if
>> IS_ENABLED(CONFIG_BLAH) and then sticking a stub implementation in the
>> #else.
>>
>> I see below that you already have a stub if backlight is not enabled, so
>> expand
>> that #if to include CONFIG_OF as well.
>>
>>> +               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 __exit backlight_class_exit(void)
>>>   {
>>>         class_destroy(backlight_class);
>>> diff --git a/include/drm/tinydrm/tinydrm-helpers.h
>>> b/include/drm/tinydrm/tinydrm-helpers.h
>>> index f54fae0..0a4ddbc 100644
>>> --- a/include/drm/tinydrm/tinydrm-helpers.h
>>> +++ b/include/drm/tinydrm/tinydrm-helpers.h
>>> @@ -46,8 +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);
>>> -
>>>   size_t tinydrm_spi_max_transfer_size(struct spi_device *spi, size_t
>>> max_len);
>>>   bool tinydrm_spi_bpw_supported(struct spi_device *spi, u8 bpw);
>>>   int tinydrm_spi_transfer(struct spi_device *spi, u32 speed_hz,
>>> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
>>> index b88fabb..987a6d7 100644
>>> --- a/include/linux/backlight.h
>>> +++ b/include/linux/backlight.h
>>> @@ -159,6 +159,16 @@ static inline int backlight_disable(struct
>>> backlight_device *bd)
>>>         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);
>>> +}
>>
>> I'm not convinced this function needs to exist.
>>
>>> +
>>>   extern struct backlight_device *backlight_device_register(const char
>>> *name,
>>>         struct device *dev, void *devdata, const struct backlight_ops
>>> *ops,
>>>         const struct backlight_properties *props);
>>> @@ -202,4 +212,13 @@ of_find_backlight_by_node(struct device_node *node)
>>>   }
>>>   #endif
>>>   +#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
>>> +struct backlight_device *backlight_get(struct device *dev);
>>> +#else
>>> +static inline struct backlight_device *backlight_get(struct device *dev)
>>> +{
>>> +       return NULL;
>>> +}
>>> +#endif
>>> +
>>>   #endif
>>> --
>>> 2.7.4
>>>
>>> --
>>> You received this message because you are subscribed to the Google Groups
>>> "outreachy-kernel" group.
>>> To unsubscribe from this group and stop receiving emails from it, send an
>>> email to outreachy-kernel+unsubscribe@googlegroups.com.
>>> To post to this group, send email to outreachy-kernel@googlegroups.com.
>>> To view this discussion on the web visit
>>> https://groups.google.com/d/msgid/outreachy-kernel/e2ef05bf46438427d08c6a61fe5058156800db95.1507890285.git.meghana.madhyastha%40gmail.com.
>>> For more options, visit https://groups.google.com/d/optout.
>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Outreachy kernel] [PATCH v13 2/3] drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c

[Sean Paul]

On Fri, Oct 13, 2017 at 6:42 PM, Noralf Trønnes <noralf@tronnes.org> wrote:
>
> Den 13.10.2017 22.25, skrev Sean Paul:
>>
>> On Fri, Oct 13, 2017 at 04:11:43PM +0530, Meghana Madhyastha wrote:
>>>
>>> Rename tinydrm_of_find_backlight to backlight_get and move it
>>> to linux/backlight.c so that it can be used by other drivers.
>>
>> [apologies if this has been brought up in previous versions, I haven't
>> been
>> following closely]
>>
>> I don't think "backlight_get" is a good name for this function. How about
>> of_find_backlight_by_name (since there's already
>> of_find_backlight_by_node)?
>
>
> I came up with that name modelled after gpiod_get() and gpiod_put() and I
> deliberately kept the of_ part out of the name like the gpio functions.
> gpiod_get() checks OF, ACPI and platform for gpios and calling it
> backlight_get() would keep the door open for other ways of connecting
> backlight devices in the future, other than Device Tree.
>

Thanks for the background, Noralf! Apologies for stepping on top of
your previous reviews.

> I think of_find_backlight() would be better than *_by_name(), since
> 'backlight' is the common DT property name, so it wouldn't make much sense
> to require every caller to pass in the same name.
>

of_find_backlight() sounds like a good compromise.

Sean


> Either way is fine with me.
>
> Noralf.
>
>
>
>>> Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
>>> ---
>>> Changes in v13:
>>>   -Add backlight_put to backlight.h in this patch
>>>
>>>   drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 40
>>> --------------------------
>>>   drivers/gpu/drm/tinydrm/mi0283qt.c             |  3 +-
>>>   drivers/video/backlight/backlight.c            | 37
>>> ++++++++++++++++++++++++
>>>   include/drm/tinydrm/tinydrm-helpers.h          |  2 --
>>>   include/linux/backlight.h                      | 19 ++++++++++++
>>>   5 files changed, 58 insertions(+), 43 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>>> b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>>> index a42dee6..cb1a01a 100644
>>> --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>>> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>>> @@ -236,46 +236,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);
>>> -
>>>   #if IS_ENABLED(CONFIG_SPI)
>>>     /**
>>> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c
>>> b/drivers/gpu/drm/tinydrm/mi0283qt.c
>>> index 7fd2691..a932185 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 <linux/backlight.h>
>>>   #include <linux/delay.h>
>>>   #include <linux/gpio/consumer.h>
>>>   #include <linux/module.h>
>>> @@ -188,7 +189,7 @@ static int mi0283qt_probe(struct spi_device *spi)
>>>         if (IS_ERR(mipi->regulator))
>>>                 return PTR_ERR(mipi->regulator);
>>>   -     mipi->backlight = tinydrm_of_find_backlight(dev);
>>> +       mipi->backlight = backlight_get(dev);
>>>         if (IS_ERR(mipi->backlight))
>>>                 return PTR_ERR(mipi->backlight);
>>>   diff --git a/drivers/video/backlight/backlight.c
>>> b/drivers/video/backlight/backlight.c
>>> index 8049e76..c4e94d0 100644
>>> --- a/drivers/video/backlight/backlight.c
>>> +++ b/drivers/video/backlight/backlight.c
>>> @@ -580,6 +580,43 @@ struct backlight_device
>>> *of_find_backlight_by_node(struct device_node *node)
>>>   EXPORT_SYMBOL(of_find_backlight_by_node);
>>>   #endif
>>>   +/**
>>> + * 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) {
>>
>> I'm not really used to seeing IS_ENABLED(CONFIG_BLAH) inline. The common
>> patterns seems to be wrapping the actual implementation in #if
>> IS_ENABLED(CONFIG_BLAH) and then sticking a stub implementation in the
>> #else.
>>
>> I see below that you already have a stub if backlight is not enabled, so
>> expand
>> that #if to include CONFIG_OF as well.
>>
>>> +               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 __exit backlight_class_exit(void)
>>>   {
>>>         class_destroy(backlight_class);
>>> diff --git a/include/drm/tinydrm/tinydrm-helpers.h
>>> b/include/drm/tinydrm/tinydrm-helpers.h
>>> index f54fae0..0a4ddbc 100644
>>> --- a/include/drm/tinydrm/tinydrm-helpers.h
>>> +++ b/include/drm/tinydrm/tinydrm-helpers.h
>>> @@ -46,8 +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);
>>> -
>>>   size_t tinydrm_spi_max_transfer_size(struct spi_device *spi, size_t
>>> max_len);
>>>   bool tinydrm_spi_bpw_supported(struct spi_device *spi, u8 bpw);
>>>   int tinydrm_spi_transfer(struct spi_device *spi, u32 speed_hz,
>>> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
>>> index b88fabb..987a6d7 100644
>>> --- a/include/linux/backlight.h
>>> +++ b/include/linux/backlight.h
>>> @@ -159,6 +159,16 @@ static inline int backlight_disable(struct
>>> backlight_device *bd)
>>>         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);
>>> +}
>>
>> I'm not convinced this function needs to exist.
>>
>>> +
>>>   extern struct backlight_device *backlight_device_register(const char
>>> *name,
>>>         struct device *dev, void *devdata, const struct backlight_ops
>>> *ops,
>>>         const struct backlight_properties *props);
>>> @@ -202,4 +212,13 @@ of_find_backlight_by_node(struct device_node *node)
>>>   }
>>>   #endif
>>>   +#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
>>> +struct backlight_device *backlight_get(struct device *dev);
>>> +#else
>>> +static inline struct backlight_device *backlight_get(struct device *dev)
>>> +{
>>> +       return NULL;
>>> +}
>>> +#endif
>>> +
>>>   #endif
>>> --
>>> 2.7.4
>>>
>>> --
>>> You received this message because you are subscribed to the Google Groups
>>> "outreachy-kernel" group.
>>> To unsubscribe from this group and stop receiving emails from it, send an
>>> email to outreachy-kernel+unsubscribe@googlegroups.com.
>>> To post to this group, send email to outreachy-kernel@googlegroups.com.
>>> To view this discussion on the web visit
>>> https://groups.google.com/d/msgid/outreachy-kernel/e2ef05bf46438427d08c6a61fe5058156800db95.1507890285.git.meghana.madhyastha%40gmail.com.
>>> For more options, visit https://groups.google.com/d/optout.
>
>

Re: [Outreachy kernel] [PATCH v13 2/3] drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c

[Sean Paul]

On Fri, Oct 13, 2017 at 7:25 PM, Rob Clark <robdclark@gmail.com> wrote:
> On Fri, Oct 13, 2017 at 4:25 PM, Sean Paul <seanpaul@chromium.org> wrote:
>> On Fri, Oct 13, 2017 at 04:11:43PM +0530, Meghana Madhyastha wrote:
>>> Rename tinydrm_of_find_backlight to backlight_get and move it
>>> to linux/backlight.c so that it can be used by other drivers.
>>
>> [apologies if this has been brought up in previous versions, I haven't been
>> following closely]
>>
>> I don't think "backlight_get" is a good name for this function. How about
>> of_find_backlight_by_name (since there's already of_find_backlight_by_node)?
>>
>>>
>>> Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
>>> ---
>>> Changes in v13:
>>>  -Add backlight_put to backlight.h in this patch
>>>
>>>  drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 40 --------------------------
>>>  drivers/gpu/drm/tinydrm/mi0283qt.c             |  3 +-
>>>  drivers/video/backlight/backlight.c            | 37 ++++++++++++++++++++++++
>>>  include/drm/tinydrm/tinydrm-helpers.h          |  2 --
>>>  include/linux/backlight.h                      | 19 ++++++++++++
>>>  5 files changed, 58 insertions(+), 43 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>>> index a42dee6..cb1a01a 100644
>>> --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>>> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>>> @@ -236,46 +236,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);
>>> -
>>>  #if IS_ENABLED(CONFIG_SPI)
>>>
>>>  /**
>>> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
>>> index 7fd2691..a932185 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 <linux/backlight.h>
>>>  #include <linux/delay.h>
>>>  #include <linux/gpio/consumer.h>
>>>  #include <linux/module.h>
>>> @@ -188,7 +189,7 @@ static int mi0283qt_probe(struct spi_device *spi)
>>>       if (IS_ERR(mipi->regulator))
>>>               return PTR_ERR(mipi->regulator);
>>>
>>> -     mipi->backlight = tinydrm_of_find_backlight(dev);
>>> +     mipi->backlight = backlight_get(dev);
>>>       if (IS_ERR(mipi->backlight))
>>>               return PTR_ERR(mipi->backlight);
>>>
>>> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
>>> index 8049e76..c4e94d0 100644
>>> --- a/drivers/video/backlight/backlight.c
>>> +++ b/drivers/video/backlight/backlight.c
>>> @@ -580,6 +580,43 @@ struct backlight_device *of_find_backlight_by_node(struct device_node *node)
>>>  EXPORT_SYMBOL(of_find_backlight_by_node);
>>>  #endif
>>>
>>> +/**
>>> + * 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) {
>>
>> I'm not really used to seeing IS_ENABLED(CONFIG_BLAH) inline. The common
>> patterns seems to be wrapping the actual implementation in #if
>> IS_ENABLED(CONFIG_BLAH) and then sticking a stub implementation in the #else.
>
>
> drive-by comment, but I actually prefer 'if (IS_ENABLED(CONFIG_FOO))
> ..' when possible.. it keeps the code compiled by the front-end of the
> compiler in more configs so less likely to bitrot, and the compiler is
> plenty good at DCE to eliminate the code when disabled in later stages
> of the compiler..
>

Sounds reasonable. I think there is already a stub implementation for
this function, so it's a little awkward gating on one CONFIG and not
another. Given Noralf's idea that backlight_get() be a general purpose
accessor, it makes a bit more sense to gate the entire function on
CONFIG_BACKLIGHT_CLASS_DRIVER and only a portion on CONFIG_OF.

So, I guess I'm fine with leaving this here.

Sean

> possibly exceptions are things that are called frequently where you
> want to inline the CONFIG_FOO=n stub so it can be eliminated without
> calling into another object file (at least until LTO gets good), but I
> guess that is not the case here..
>
> BR,
> -R
>
>
>> I see below that you already have a stub if backlight is not enabled, so expand
>> that #if to include CONFIG_OF as well.
>>
>>> +             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 __exit backlight_class_exit(void)
>>>  {
>>>       class_destroy(backlight_class);
>>> diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h
>>> index f54fae0..0a4ddbc 100644
>>> --- a/include/drm/tinydrm/tinydrm-helpers.h
>>> +++ b/include/drm/tinydrm/tinydrm-helpers.h
>>> @@ -46,8 +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);
>>> -
>>>  size_t tinydrm_spi_max_transfer_size(struct spi_device *spi, size_t max_len);
>>>  bool tinydrm_spi_bpw_supported(struct spi_device *spi, u8 bpw);
>>>  int tinydrm_spi_transfer(struct spi_device *spi, u32 speed_hz,
>>> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
>>> index b88fabb..987a6d7 100644
>>> --- a/include/linux/backlight.h
>>> +++ b/include/linux/backlight.h
>>> @@ -159,6 +159,16 @@ static inline int backlight_disable(struct backlight_device *bd)
>>>       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);
>>> +}
>>
>> I'm not convinced this function needs to exist.
>>
>>> +
>>>  extern struct backlight_device *backlight_device_register(const char *name,
>>>       struct device *dev, void *devdata, const struct backlight_ops *ops,
>>>       const struct backlight_properties *props);
>>> @@ -202,4 +212,13 @@ of_find_backlight_by_node(struct device_node *node)
>>>  }
>>>  #endif
>>>
>>> +#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
>>> +struct backlight_device *backlight_get(struct device *dev);
>>> +#else
>>> +static inline struct backlight_device *backlight_get(struct device *dev)
>>> +{
>>> +     return NULL;
>>> +}
>>> +#endif
>>> +
>>>  #endif
>>> --
>>> 2.7.4
>>>
>>> --
>>> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
>>> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
>>> To post to this group, send email to outreachy-kernel@googlegroups.com.
>>> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/e2ef05bf46438427d08c6a61fe5058156800db95.1507890285.git.meghana.madhyastha%40gmail.com.
>>> For more options, visit https://groups.google.com/d/optout.
>>
>> --
>> Sean Paul, Software Engineer, Google / Chromium OS
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Outreachy kernel] [PATCH v13 2/3] drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c

[Sean Paul]

On Fri, Oct 13, 2017 at 7:25 PM, Rob Clark <robdclark@gmail.com> wrote:
> On Fri, Oct 13, 2017 at 4:25 PM, Sean Paul <seanpaul@chromium.org> wrote:
>> On Fri, Oct 13, 2017 at 04:11:43PM +0530, Meghana Madhyastha wrote:
>>> Rename tinydrm_of_find_backlight to backlight_get and move it
>>> to linux/backlight.c so that it can be used by other drivers.
>>
>> [apologies if this has been brought up in previous versions, I haven't been
>> following closely]
>>
>> I don't think "backlight_get" is a good name for this function. How about
>> of_find_backlight_by_name (since there's already of_find_backlight_by_node)?
>>
>>>
>>> Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
>>> ---
>>> Changes in v13:
>>>  -Add backlight_put to backlight.h in this patch
>>>
>>>  drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 40 --------------------------
>>>  drivers/gpu/drm/tinydrm/mi0283qt.c             |  3 +-
>>>  drivers/video/backlight/backlight.c            | 37 ++++++++++++++++++++++++
>>>  include/drm/tinydrm/tinydrm-helpers.h          |  2 --
>>>  include/linux/backlight.h                      | 19 ++++++++++++
>>>  5 files changed, 58 insertions(+), 43 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>>> index a42dee6..cb1a01a 100644
>>> --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>>> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>>> @@ -236,46 +236,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);
>>> -
>>>  #if IS_ENABLED(CONFIG_SPI)
>>>
>>>  /**
>>> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
>>> index 7fd2691..a932185 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 <linux/backlight.h>
>>>  #include <linux/delay.h>
>>>  #include <linux/gpio/consumer.h>
>>>  #include <linux/module.h>
>>> @@ -188,7 +189,7 @@ static int mi0283qt_probe(struct spi_device *spi)
>>>       if (IS_ERR(mipi->regulator))
>>>               return PTR_ERR(mipi->regulator);
>>>
>>> -     mipi->backlight = tinydrm_of_find_backlight(dev);
>>> +     mipi->backlight = backlight_get(dev);
>>>       if (IS_ERR(mipi->backlight))
>>>               return PTR_ERR(mipi->backlight);
>>>
>>> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
>>> index 8049e76..c4e94d0 100644
>>> --- a/drivers/video/backlight/backlight.c
>>> +++ b/drivers/video/backlight/backlight.c
>>> @@ -580,6 +580,43 @@ struct backlight_device *of_find_backlight_by_node(struct device_node *node)
>>>  EXPORT_SYMBOL(of_find_backlight_by_node);
>>>  #endif
>>>
>>> +/**
>>> + * 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) {
>>
>> I'm not really used to seeing IS_ENABLED(CONFIG_BLAH) inline. The common
>> patterns seems to be wrapping the actual implementation in #if
>> IS_ENABLED(CONFIG_BLAH) and then sticking a stub implementation in the #else.
>
>
> drive-by comment, but I actually prefer 'if (IS_ENABLED(CONFIG_FOO))
> ..' when possible.. it keeps the code compiled by the front-end of the
> compiler in more configs so less likely to bitrot, and the compiler is
> plenty good at DCE to eliminate the code when disabled in later stages
> of the compiler..
>

Sounds reasonable. I think there is already a stub implementation for
this function, so it's a little awkward gating on one CONFIG and not
another. Given Noralf's idea that backlight_get() be a general purpose
accessor, it makes a bit more sense to gate the entire function on
CONFIG_BACKLIGHT_CLASS_DRIVER and only a portion on CONFIG_OF.

So, I guess I'm fine with leaving this here.

Sean

> possibly exceptions are things that are called frequently where you
> want to inline the CONFIG_FOO=n stub so it can be eliminated without
> calling into another object file (at least until LTO gets good), but I
> guess that is not the case here..
>
> BR,
> -R
>
>
>> I see below that you already have a stub if backlight is not enabled, so expand
>> that #if to include CONFIG_OF as well.
>>
>>> +             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 __exit backlight_class_exit(void)
>>>  {
>>>       class_destroy(backlight_class);
>>> diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h
>>> index f54fae0..0a4ddbc 100644
>>> --- a/include/drm/tinydrm/tinydrm-helpers.h
>>> +++ b/include/drm/tinydrm/tinydrm-helpers.h
>>> @@ -46,8 +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);
>>> -
>>>  size_t tinydrm_spi_max_transfer_size(struct spi_device *spi, size_t max_len);
>>>  bool tinydrm_spi_bpw_supported(struct spi_device *spi, u8 bpw);
>>>  int tinydrm_spi_transfer(struct spi_device *spi, u32 speed_hz,
>>> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
>>> index b88fabb..987a6d7 100644
>>> --- a/include/linux/backlight.h
>>> +++ b/include/linux/backlight.h
>>> @@ -159,6 +159,16 @@ static inline int backlight_disable(struct backlight_device *bd)
>>>       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);
>>> +}
>>
>> I'm not convinced this function needs to exist.
>>
>>> +
>>>  extern struct backlight_device *backlight_device_register(const char *name,
>>>       struct device *dev, void *devdata, const struct backlight_ops *ops,
>>>       const struct backlight_properties *props);
>>> @@ -202,4 +212,13 @@ of_find_backlight_by_node(struct device_node *node)
>>>  }
>>>  #endif
>>>
>>> +#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
>>> +struct backlight_device *backlight_get(struct device *dev);
>>> +#else
>>> +static inline struct backlight_device *backlight_get(struct device *dev)
>>> +{
>>> +     return NULL;
>>> +}
>>> +#endif
>>> +
>>>  #endif
>>> --
>>> 2.7.4
>>>
>>> --
>>> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
>>> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
>>> To post to this group, send email to outreachy-kernel@googlegroups.com.
>>> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/e2ef05bf46438427d08c6a61fe5058156800db95.1507890285.git.meghana.madhyastha%40gmail.com.
>>> For more options, visit https://groups.google.com/d/optout.
>>
>> --
>> Sean Paul, Software Engineer, Google / Chromium OS
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Outreachy kernel] [PATCH v13 2/3] drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c

[Meghana Madhyastha]

On Mon, Oct 16, 2017 at 02:26:00PM -0400, Sean Paul wrote:
> On Fri, Oct 13, 2017 at 6:42 PM, Noralf Trønnes <noralf@tronnes.org> wrote:
> >
> > Den 13.10.2017 22.25, skrev Sean Paul:
> >>
> >> On Fri, Oct 13, 2017 at 04:11:43PM +0530, Meghana Madhyastha wrote:
> >>>
> >>> Rename tinydrm_of_find_backlight to backlight_get and move it
> >>> to linux/backlight.c so that it can be used by other drivers.
> >>
> >> [apologies if this has been brought up in previous versions, I haven't
> >> been
> >> following closely]
> >>
> >> I don't think "backlight_get" is a good name for this function. How about
> >> of_find_backlight_by_name (since there's already
> >> of_find_backlight_by_node)?
> >
> >
> > I came up with that name modelled after gpiod_get() and gpiod_put() and I
> > deliberately kept the of_ part out of the name like the gpio functions.
> > gpiod_get() checks OF, ACPI and platform for gpios and calling it
> > backlight_get() would keep the door open for other ways of connecting
> > backlight devices in the future, other than Device Tree.
> >
> 
> Thanks for the background, Noralf! Apologies for stepping on top of
> your previous reviews.
> 
> > I think of_find_backlight() would be better than *_by_name(), since
> > 'backlight' is the common DT property name, so it wouldn't make much sense
> > to require every caller to pass in the same name.
> >
> 
> of_find_backlight() sounds like a good compromise.
> 
> Sean

Are there any changes that need to be made to this patchset now ?

Regards,
Meghana

> 
> > Either way is fine with me.
> >
> > Noralf.
> >
> >
> >
> >>> Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
> >>> ---
> >>> Changes in v13:
> >>>   -Add backlight_put to backlight.h in this patch
> >>>
> >>>   drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 40
> >>> --------------------------
> >>>   drivers/gpu/drm/tinydrm/mi0283qt.c             |  3 +-
> >>>   drivers/video/backlight/backlight.c            | 37
> >>> ++++++++++++++++++++++++
> >>>   include/drm/tinydrm/tinydrm-helpers.h          |  2 --
> >>>   include/linux/backlight.h                      | 19 ++++++++++++
> >>>   5 files changed, 58 insertions(+), 43 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> >>> b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> >>> index a42dee6..cb1a01a 100644
> >>> --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> >>> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> >>> @@ -236,46 +236,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);
> >>> -
> >>>   #if IS_ENABLED(CONFIG_SPI)
> >>>     /**
> >>> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c
> >>> b/drivers/gpu/drm/tinydrm/mi0283qt.c
> >>> index 7fd2691..a932185 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 <linux/backlight.h>
> >>>   #include <linux/delay.h>
> >>>   #include <linux/gpio/consumer.h>
> >>>   #include <linux/module.h>
> >>> @@ -188,7 +189,7 @@ static int mi0283qt_probe(struct spi_device *spi)
> >>>         if (IS_ERR(mipi->regulator))
> >>>                 return PTR_ERR(mipi->regulator);
> >>>   -     mipi->backlight = tinydrm_of_find_backlight(dev);
> >>> +       mipi->backlight = backlight_get(dev);
> >>>         if (IS_ERR(mipi->backlight))
> >>>                 return PTR_ERR(mipi->backlight);
> >>>   diff --git a/drivers/video/backlight/backlight.c
> >>> b/drivers/video/backlight/backlight.c
> >>> index 8049e76..c4e94d0 100644
> >>> --- a/drivers/video/backlight/backlight.c
> >>> +++ b/drivers/video/backlight/backlight.c
> >>> @@ -580,6 +580,43 @@ struct backlight_device
> >>> *of_find_backlight_by_node(struct device_node *node)
> >>>   EXPORT_SYMBOL(of_find_backlight_by_node);
> >>>   #endif
> >>>   +/**
> >>> + * 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) {
> >>
> >> I'm not really used to seeing IS_ENABLED(CONFIG_BLAH) inline. The common
> >> patterns seems to be wrapping the actual implementation in #if
> >> IS_ENABLED(CONFIG_BLAH) and then sticking a stub implementation in the
> >> #else.
> >>
> >> I see below that you already have a stub if backlight is not enabled, so
> >> expand
> >> that #if to include CONFIG_OF as well.
> >>
> >>> +               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 __exit backlight_class_exit(void)
> >>>   {
> >>>         class_destroy(backlight_class);
> >>> diff --git a/include/drm/tinydrm/tinydrm-helpers.h
> >>> b/include/drm/tinydrm/tinydrm-helpers.h
> >>> index f54fae0..0a4ddbc 100644
> >>> --- a/include/drm/tinydrm/tinydrm-helpers.h
> >>> +++ b/include/drm/tinydrm/tinydrm-helpers.h
> >>> @@ -46,8 +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);
> >>> -
> >>>   size_t tinydrm_spi_max_transfer_size(struct spi_device *spi, size_t
> >>> max_len);
> >>>   bool tinydrm_spi_bpw_supported(struct spi_device *spi, u8 bpw);
> >>>   int tinydrm_spi_transfer(struct spi_device *spi, u32 speed_hz,
> >>> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> >>> index b88fabb..987a6d7 100644
> >>> --- a/include/linux/backlight.h
> >>> +++ b/include/linux/backlight.h
> >>> @@ -159,6 +159,16 @@ static inline int backlight_disable(struct
> >>> backlight_device *bd)
> >>>         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);
> >>> +}
> >>
> >> I'm not convinced this function needs to exist.
> >>
> >>> +
> >>>   extern struct backlight_device *backlight_device_register(const char
> >>> *name,
> >>>         struct device *dev, void *devdata, const struct backlight_ops
> >>> *ops,
> >>>         const struct backlight_properties *props);
> >>> @@ -202,4 +212,13 @@ of_find_backlight_by_node(struct device_node *node)
> >>>   }
> >>>   #endif
> >>>   +#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
> >>> +struct backlight_device *backlight_get(struct device *dev);
> >>> +#else
> >>> +static inline struct backlight_device *backlight_get(struct device *dev)
> >>> +{
> >>> +       return NULL;
> >>> +}
> >>> +#endif
> >>> +
> >>>   #endif
> >>> --
> >>> 2.7.4
> >>>
> >>> --
> >>> You received this message because you are subscribed to the Google Groups
> >>> "outreachy-kernel" group.
> >>> To unsubscribe from this group and stop receiving emails from it, send an
> >>> email to outreachy-kernel+unsubscribe@googlegroups.com.
> >>> To post to this group, send email to outreachy-kernel@googlegroups.com.
> >>> To view this discussion on the web visit
> >>> https://groups.google.com/d/msgid/outreachy-kernel/e2ef05bf46438427d08c6a61fe5058156800db95.1507890285.git.meghana.madhyastha%40gmail.com.
> >>> For more options, visit https://groups.google.com/d/optout.
> >
> >
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Outreachy kernel] [PATCH v13 2/3] drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c

[Meghana Madhyastha]

On Mon, Oct 16, 2017 at 02:26:00PM -0400, Sean Paul wrote:
> On Fri, Oct 13, 2017 at 6:42 PM, Noralf Tr�nnes <noralf@tronnes.org> wrote:
> >
> > Den 13.10.2017 22.25, skrev Sean Paul:
> >>
> >> On Fri, Oct 13, 2017 at 04:11:43PM +0530, Meghana Madhyastha wrote:
> >>>
> >>> Rename tinydrm_of_find_backlight to backlight_get and move it
> >>> to linux/backlight.c so that it can be used by other drivers.
> >>
> >> [apologies if this has been brought up in previous versions, I haven't
> >> been
> >> following closely]
> >>
> >> I don't think "backlight_get" is a good name for this function. How about
> >> of_find_backlight_by_name (since there's already
> >> of_find_backlight_by_node)?
> >
> >
> > I came up with that name modelled after gpiod_get() and gpiod_put() and I
> > deliberately kept the of_ part out of the name like the gpio functions.
> > gpiod_get() checks OF, ACPI and platform for gpios and calling it
> > backlight_get() would keep the door open for other ways of connecting
> > backlight devices in the future, other than Device Tree.
> >
> 
> Thanks for the background, Noralf! Apologies for stepping on top of
> your previous reviews.
> 
> > I think of_find_backlight() would be better than *_by_name(), since
> > 'backlight' is the common DT property name, so it wouldn't make much sense
> > to require every caller to pass in the same name.
> >
> 
> of_find_backlight() sounds like a good compromise.
> 
> Sean

Are there any changes that need to be made to this patchset now ?

Regards,
Meghana

> 
> > Either way is fine with me.
> >
> > Noralf.
> >
> >
> >
> >>> Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
> >>> ---
> >>> Changes in v13:
> >>>   -Add backlight_put to backlight.h in this patch
> >>>
> >>>   drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 40
> >>> --------------------------
> >>>   drivers/gpu/drm/tinydrm/mi0283qt.c             |  3 +-
> >>>   drivers/video/backlight/backlight.c            | 37
> >>> ++++++++++++++++++++++++
> >>>   include/drm/tinydrm/tinydrm-helpers.h          |  2 --
> >>>   include/linux/backlight.h                      | 19 ++++++++++++
> >>>   5 files changed, 58 insertions(+), 43 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> >>> b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> >>> index a42dee6..cb1a01a 100644
> >>> --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> >>> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> >>> @@ -236,46 +236,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);
> >>> -
> >>>   #if IS_ENABLED(CONFIG_SPI)
> >>>     /**
> >>> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c
> >>> b/drivers/gpu/drm/tinydrm/mi0283qt.c
> >>> index 7fd2691..a932185 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 <linux/backlight.h>
> >>>   #include <linux/delay.h>
> >>>   #include <linux/gpio/consumer.h>
> >>>   #include <linux/module.h>
> >>> @@ -188,7 +189,7 @@ static int mi0283qt_probe(struct spi_device *spi)
> >>>         if (IS_ERR(mipi->regulator))
> >>>                 return PTR_ERR(mipi->regulator);
> >>>   -     mipi->backlight = tinydrm_of_find_backlight(dev);
> >>> +       mipi->backlight = backlight_get(dev);
> >>>         if (IS_ERR(mipi->backlight))
> >>>                 return PTR_ERR(mipi->backlight);
> >>>   diff --git a/drivers/video/backlight/backlight.c
> >>> b/drivers/video/backlight/backlight.c
> >>> index 8049e76..c4e94d0 100644
> >>> --- a/drivers/video/backlight/backlight.c
> >>> +++ b/drivers/video/backlight/backlight.c
> >>> @@ -580,6 +580,43 @@ struct backlight_device
> >>> *of_find_backlight_by_node(struct device_node *node)
> >>>   EXPORT_SYMBOL(of_find_backlight_by_node);
> >>>   #endif
> >>>   +/**
> >>> + * 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) {
> >>
> >> I'm not really used to seeing IS_ENABLED(CONFIG_BLAH) inline. The common
> >> patterns seems to be wrapping the actual implementation in #if
> >> IS_ENABLED(CONFIG_BLAH) and then sticking a stub implementation in the
> >> #else.
> >>
> >> I see below that you already have a stub if backlight is not enabled, so
> >> expand
> >> that #if to include CONFIG_OF as well.
> >>
> >>> +               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 __exit backlight_class_exit(void)
> >>>   {
> >>>         class_destroy(backlight_class);
> >>> diff --git a/include/drm/tinydrm/tinydrm-helpers.h
> >>> b/include/drm/tinydrm/tinydrm-helpers.h
> >>> index f54fae0..0a4ddbc 100644
> >>> --- a/include/drm/tinydrm/tinydrm-helpers.h
> >>> +++ b/include/drm/tinydrm/tinydrm-helpers.h
> >>> @@ -46,8 +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);
> >>> -
> >>>   size_t tinydrm_spi_max_transfer_size(struct spi_device *spi, size_t
> >>> max_len);
> >>>   bool tinydrm_spi_bpw_supported(struct spi_device *spi, u8 bpw);
> >>>   int tinydrm_spi_transfer(struct spi_device *spi, u32 speed_hz,
> >>> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> >>> index b88fabb..987a6d7 100644
> >>> --- a/include/linux/backlight.h
> >>> +++ b/include/linux/backlight.h
> >>> @@ -159,6 +159,16 @@ static inline int backlight_disable(struct
> >>> backlight_device *bd)
> >>>         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);
> >>> +}
> >>
> >> I'm not convinced this function needs to exist.
> >>
> >>> +
> >>>   extern struct backlight_device *backlight_device_register(const char
> >>> *name,
> >>>         struct device *dev, void *devdata, const struct backlight_ops
> >>> *ops,
> >>>         const struct backlight_properties *props);
> >>> @@ -202,4 +212,13 @@ of_find_backlight_by_node(struct device_node *node)
> >>>   }
> >>>   #endif
> >>>   +#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
> >>> +struct backlight_device *backlight_get(struct device *dev);
> >>> +#else
> >>> +static inline struct backlight_device *backlight_get(struct device *dev)
> >>> +{
> >>> +       return NULL;
> >>> +}
> >>> +#endif
> >>> +
> >>>   #endif
> >>> --
> >>> 2.7.4
> >>>
> >>> --
> >>> You received this message because you are subscribed to the Google Groups
> >>> "outreachy-kernel" group.
> >>> To unsubscribe from this group and stop receiving emails from it, send an
> >>> email to outreachy-kernel+unsubscribe@googlegroups.com.
> >>> To post to this group, send email to outreachy-kernel@googlegroups.com.
> >>> To view this discussion on the web visit
> >>> https://groups.google.com/d/msgid/outreachy-kernel/e2ef05bf46438427d08c6a61fe5058156800db95.1507890285.git.meghana.madhyastha%40gmail.com.
> >>> For more options, visit https://groups.google.com/d/optout.
> >
> >

Re: [Outreachy kernel] [PATCH v13 2/3] drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c

[Sean Paul]

On Wed, Oct 18, 2017 at 1:11 PM, Meghana Madhyastha
<meghana.madhyastha@gmail.com> wrote:
> On Mon, Oct 16, 2017 at 02:26:00PM -0400, Sean Paul wrote:
>> On Fri, Oct 13, 2017 at 6:42 PM, Noralf Trønnes <noralf@tronnes.org> wrote:
>> >
>> > Den 13.10.2017 22.25, skrev Sean Paul:
>> >>
>> >> On Fri, Oct 13, 2017 at 04:11:43PM +0530, Meghana Madhyastha wrote:
>> >>>
>> >>> Rename tinydrm_of_find_backlight to backlight_get and move it
>> >>> to linux/backlight.c so that it can be used by other drivers.
>> >>
>> >> [apologies if this has been brought up in previous versions, I haven't
>> >> been
>> >> following closely]
>> >>
>> >> I don't think "backlight_get" is a good name for this function. How about
>> >> of_find_backlight_by_name (since there's already
>> >> of_find_backlight_by_node)?
>> >
>> >
>> > I came up with that name modelled after gpiod_get() and gpiod_put() and I
>> > deliberately kept the of_ part out of the name like the gpio functions.
>> > gpiod_get() checks OF, ACPI and platform for gpios and calling it
>> > backlight_get() would keep the door open for other ways of connecting
>> > backlight devices in the future, other than Device Tree.
>> >
>>
>> Thanks for the background, Noralf! Apologies for stepping on top of
>> your previous reviews.
>>
>> > I think of_find_backlight() would be better than *_by_name(), since
>> > 'backlight' is the common DT property name, so it wouldn't make much sense
>> > to require every caller to pass in the same name.
>> >
>>
>> of_find_backlight() sounds like a good compromise.
>>
>> Sean
>
> Are there any changes that need to be made to this patchset now ?
>

I still prefer s/backlight_get/of_find_backlight/ and it seems Noralf
is Ok with that too. I think there were also review comments
surrounding the _put function?

Sean

> Regards,
> Meghana
>
>>
>> > Either way is fine with me.
>> >
>> > Noralf.
>> >
>> >
>> >
>> >>> Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
>> >>> ---
>> >>> Changes in v13:
>> >>>   -Add backlight_put to backlight.h in this patch
>> >>>
>> >>>   drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 40
>> >>> --------------------------
>> >>>   drivers/gpu/drm/tinydrm/mi0283qt.c             |  3 +-
>> >>>   drivers/video/backlight/backlight.c            | 37
>> >>> ++++++++++++++++++++++++
>> >>>   include/drm/tinydrm/tinydrm-helpers.h          |  2 --
>> >>>   include/linux/backlight.h                      | 19 ++++++++++++
>> >>>   5 files changed, 58 insertions(+), 43 deletions(-)
>> >>>
>> >>> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>> >>> b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>> >>> index a42dee6..cb1a01a 100644
>> >>> --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>> >>> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>> >>> @@ -236,46 +236,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);
>> >>> -
>> >>>   #if IS_ENABLED(CONFIG_SPI)
>> >>>     /**
>> >>> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c
>> >>> b/drivers/gpu/drm/tinydrm/mi0283qt.c
>> >>> index 7fd2691..a932185 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 <linux/backlight.h>
>> >>>   #include <linux/delay.h>
>> >>>   #include <linux/gpio/consumer.h>
>> >>>   #include <linux/module.h>
>> >>> @@ -188,7 +189,7 @@ static int mi0283qt_probe(struct spi_device *spi)
>> >>>         if (IS_ERR(mipi->regulator))
>> >>>                 return PTR_ERR(mipi->regulator);
>> >>>   -     mipi->backlight = tinydrm_of_find_backlight(dev);
>> >>> +       mipi->backlight = backlight_get(dev);
>> >>>         if (IS_ERR(mipi->backlight))
>> >>>                 return PTR_ERR(mipi->backlight);
>> >>>   diff --git a/drivers/video/backlight/backlight.c
>> >>> b/drivers/video/backlight/backlight.c
>> >>> index 8049e76..c4e94d0 100644
>> >>> --- a/drivers/video/backlight/backlight.c
>> >>> +++ b/drivers/video/backlight/backlight.c
>> >>> @@ -580,6 +580,43 @@ struct backlight_device
>> >>> *of_find_backlight_by_node(struct device_node *node)
>> >>>   EXPORT_SYMBOL(of_find_backlight_by_node);
>> >>>   #endif
>> >>>   +/**
>> >>> + * 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) {
>> >>
>> >> I'm not really used to seeing IS_ENABLED(CONFIG_BLAH) inline. The common
>> >> patterns seems to be wrapping the actual implementation in #if
>> >> IS_ENABLED(CONFIG_BLAH) and then sticking a stub implementation in the
>> >> #else.
>> >>
>> >> I see below that you already have a stub if backlight is not enabled, so
>> >> expand
>> >> that #if to include CONFIG_OF as well.
>> >>
>> >>> +               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 __exit backlight_class_exit(void)
>> >>>   {
>> >>>         class_destroy(backlight_class);
>> >>> diff --git a/include/drm/tinydrm/tinydrm-helpers.h
>> >>> b/include/drm/tinydrm/tinydrm-helpers.h
>> >>> index f54fae0..0a4ddbc 100644
>> >>> --- a/include/drm/tinydrm/tinydrm-helpers.h
>> >>> +++ b/include/drm/tinydrm/tinydrm-helpers.h
>> >>> @@ -46,8 +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);
>> >>> -
>> >>>   size_t tinydrm_spi_max_transfer_size(struct spi_device *spi, size_t
>> >>> max_len);
>> >>>   bool tinydrm_spi_bpw_supported(struct spi_device *spi, u8 bpw);
>> >>>   int tinydrm_spi_transfer(struct spi_device *spi, u32 speed_hz,
>> >>> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
>> >>> index b88fabb..987a6d7 100644
>> >>> --- a/include/linux/backlight.h
>> >>> +++ b/include/linux/backlight.h
>> >>> @@ -159,6 +159,16 @@ static inline int backlight_disable(struct
>> >>> backlight_device *bd)
>> >>>         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);
>> >>> +}
>> >>
>> >> I'm not convinced this function needs to exist.
>> >>
>> >>> +
>> >>>   extern struct backlight_device *backlight_device_register(const char
>> >>> *name,
>> >>>         struct device *dev, void *devdata, const struct backlight_ops
>> >>> *ops,
>> >>>         const struct backlight_properties *props);
>> >>> @@ -202,4 +212,13 @@ of_find_backlight_by_node(struct device_node *node)
>> >>>   }
>> >>>   #endif
>> >>>   +#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
>> >>> +struct backlight_device *backlight_get(struct device *dev);
>> >>> +#else
>> >>> +static inline struct backlight_device *backlight_get(struct device *dev)
>> >>> +{
>> >>> +       return NULL;
>> >>> +}
>> >>> +#endif
>> >>> +
>> >>>   #endif
>> >>> --
>> >>> 2.7.4
>> >>>
>> >>> --
>> >>> You received this message because you are subscribed to the Google Groups
>> >>> "outreachy-kernel" group.
>> >>> To unsubscribe from this group and stop receiving emails from it, send an
>> >>> email to outreachy-kernel+unsubscribe@googlegroups.com.
>> >>> To post to this group, send email to outreachy-kernel@googlegroups.com.
>> >>> To view this discussion on the web visit
>> >>> https://groups.google.com/d/msgid/outreachy-kernel/e2ef05bf46438427d08c6a61fe5058156800db95.1507890285.git.meghana.madhyastha%40gmail.com.
>> >>> For more options, visit https://groups.google.com/d/optout.
>> >
>> >
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Outreachy kernel] [PATCH v13 2/3] drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c

[Sean Paul]

On Wed, Oct 18, 2017 at 1:11 PM, Meghana Madhyastha
<meghana.madhyastha@gmail.com> wrote:
> On Mon, Oct 16, 2017 at 02:26:00PM -0400, Sean Paul wrote:
>> On Fri, Oct 13, 2017 at 6:42 PM, Noralf Trønnes <noralf@tronnes.org> wrote:
>> >
>> > Den 13.10.2017 22.25, skrev Sean Paul:
>> >>
>> >> On Fri, Oct 13, 2017 at 04:11:43PM +0530, Meghana Madhyastha wrote:
>> >>>
>> >>> Rename tinydrm_of_find_backlight to backlight_get and move it
>> >>> to linux/backlight.c so that it can be used by other drivers.
>> >>
>> >> [apologies if this has been brought up in previous versions, I haven't
>> >> been
>> >> following closely]
>> >>
>> >> I don't think "backlight_get" is a good name for this function. How about
>> >> of_find_backlight_by_name (since there's already
>> >> of_find_backlight_by_node)?
>> >
>> >
>> > I came up with that name modelled after gpiod_get() and gpiod_put() and I
>> > deliberately kept the of_ part out of the name like the gpio functions.
>> > gpiod_get() checks OF, ACPI and platform for gpios and calling it
>> > backlight_get() would keep the door open for other ways of connecting
>> > backlight devices in the future, other than Device Tree.
>> >
>>
>> Thanks for the background, Noralf! Apologies for stepping on top of
>> your previous reviews.
>>
>> > I think of_find_backlight() would be better than *_by_name(), since
>> > 'backlight' is the common DT property name, so it wouldn't make much sense
>> > to require every caller to pass in the same name.
>> >
>>
>> of_find_backlight() sounds like a good compromise.
>>
>> Sean
>
> Are there any changes that need to be made to this patchset now ?
>

I still prefer s/backlight_get/of_find_backlight/ and it seems Noralf
is Ok with that too. I think there were also review comments
surrounding the _put function?

Sean

> Regards,
> Meghana
>
>>
>> > Either way is fine with me.
>> >
>> > Noralf.
>> >
>> >
>> >
>> >>> Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
>> >>> ---
>> >>> Changes in v13:
>> >>>   -Add backlight_put to backlight.h in this patch
>> >>>
>> >>>   drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 40
>> >>> --------------------------
>> >>>   drivers/gpu/drm/tinydrm/mi0283qt.c             |  3 +-
>> >>>   drivers/video/backlight/backlight.c            | 37
>> >>> ++++++++++++++++++++++++
>> >>>   include/drm/tinydrm/tinydrm-helpers.h          |  2 --
>> >>>   include/linux/backlight.h                      | 19 ++++++++++++
>> >>>   5 files changed, 58 insertions(+), 43 deletions(-)
>> >>>
>> >>> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>> >>> b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>> >>> index a42dee6..cb1a01a 100644
>> >>> --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>> >>> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>> >>> @@ -236,46 +236,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);
>> >>> -
>> >>>   #if IS_ENABLED(CONFIG_SPI)
>> >>>     /**
>> >>> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c
>> >>> b/drivers/gpu/drm/tinydrm/mi0283qt.c
>> >>> index 7fd2691..a932185 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 <linux/backlight.h>
>> >>>   #include <linux/delay.h>
>> >>>   #include <linux/gpio/consumer.h>
>> >>>   #include <linux/module.h>
>> >>> @@ -188,7 +189,7 @@ static int mi0283qt_probe(struct spi_device *spi)
>> >>>         if (IS_ERR(mipi->regulator))
>> >>>                 return PTR_ERR(mipi->regulator);
>> >>>   -     mipi->backlight = tinydrm_of_find_backlight(dev);
>> >>> +       mipi->backlight = backlight_get(dev);
>> >>>         if (IS_ERR(mipi->backlight))
>> >>>                 return PTR_ERR(mipi->backlight);
>> >>>   diff --git a/drivers/video/backlight/backlight.c
>> >>> b/drivers/video/backlight/backlight.c
>> >>> index 8049e76..c4e94d0 100644
>> >>> --- a/drivers/video/backlight/backlight.c
>> >>> +++ b/drivers/video/backlight/backlight.c
>> >>> @@ -580,6 +580,43 @@ struct backlight_device
>> >>> *of_find_backlight_by_node(struct device_node *node)
>> >>>   EXPORT_SYMBOL(of_find_backlight_by_node);
>> >>>   #endif
>> >>>   +/**
>> >>> + * 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) {
>> >>
>> >> I'm not really used to seeing IS_ENABLED(CONFIG_BLAH) inline. The common
>> >> patterns seems to be wrapping the actual implementation in #if
>> >> IS_ENABLED(CONFIG_BLAH) and then sticking a stub implementation in the
>> >> #else.
>> >>
>> >> I see below that you already have a stub if backlight is not enabled, so
>> >> expand
>> >> that #if to include CONFIG_OF as well.
>> >>
>> >>> +               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 __exit backlight_class_exit(void)
>> >>>   {
>> >>>         class_destroy(backlight_class);
>> >>> diff --git a/include/drm/tinydrm/tinydrm-helpers.h
>> >>> b/include/drm/tinydrm/tinydrm-helpers.h
>> >>> index f54fae0..0a4ddbc 100644
>> >>> --- a/include/drm/tinydrm/tinydrm-helpers.h
>> >>> +++ b/include/drm/tinydrm/tinydrm-helpers.h
>> >>> @@ -46,8 +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);
>> >>> -
>> >>>   size_t tinydrm_spi_max_transfer_size(struct spi_device *spi, size_t
>> >>> max_len);
>> >>>   bool tinydrm_spi_bpw_supported(struct spi_device *spi, u8 bpw);
>> >>>   int tinydrm_spi_transfer(struct spi_device *spi, u32 speed_hz,
>> >>> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
>> >>> index b88fabb..987a6d7 100644
>> >>> --- a/include/linux/backlight.h
>> >>> +++ b/include/linux/backlight.h
>> >>> @@ -159,6 +159,16 @@ static inline int backlight_disable(struct
>> >>> backlight_device *bd)
>> >>>         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);
>> >>> +}
>> >>
>> >> I'm not convinced this function needs to exist.
>> >>
>> >>> +
>> >>>   extern struct backlight_device *backlight_device_register(const char
>> >>> *name,
>> >>>         struct device *dev, void *devdata, const struct backlight_ops
>> >>> *ops,
>> >>>         const struct backlight_properties *props);
>> >>> @@ -202,4 +212,13 @@ of_find_backlight_by_node(struct device_node *node)
>> >>>   }
>> >>>   #endif
>> >>>   +#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
>> >>> +struct backlight_device *backlight_get(struct device *dev);
>> >>> +#else
>> >>> +static inline struct backlight_device *backlight_get(struct device *dev)
>> >>> +{
>> >>> +       return NULL;
>> >>> +}
>> >>> +#endif
>> >>> +
>> >>>   #endif
>> >>> --
>> >>> 2.7.4
>> >>>
>> >>> --
>> >>> You received this message because you are subscribed to the Google Groups
>> >>> "outreachy-kernel" group.
>> >>> To unsubscribe from this group and stop receiving emails from it, send an
>> >>> email to outreachy-kernel+unsubscribe@googlegroups.com.
>> >>> To post to this group, send email to outreachy-kernel@googlegroups.com.
>> >>> To view this discussion on the web visit
>> >>> https://groups.google.com/d/msgid/outreachy-kernel/e2ef05bf46438427d08c6a61fe5058156800db95.1507890285.git.meghana.madhyastha%40gmail.com.
>> >>> For more options, visit https://groups.google.com/d/optout.
>> >
>> >

Re: [Outreachy kernel] [PATCH v13 2/3] drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c

[Daniel Thompson]

On 20/10/17 18:20, Sean Paul wrote:
> On Wed, Oct 18, 2017 at 1:11 PM, Meghana Madhyastha
> <meghana.madhyastha@gmail.com> wrote:
>> On Mon, Oct 16, 2017 at 02:26:00PM -0400, Sean Paul wrote:
>>> On Fri, Oct 13, 2017 at 6:42 PM, Noralf Trønnes <noralf@tronnes.org> wrote:
>>>>
>>>> Den 13.10.2017 22.25, skrev Sean Paul:
>>>>>
>>>>> On Fri, Oct 13, 2017 at 04:11:43PM +0530, Meghana Madhyastha wrote:
>>>>>>
>>>>>> Rename tinydrm_of_find_backlight to backlight_get and move it
>>>>>> to linux/backlight.c so that it can be used by other drivers.
>>>>>
>>>>> [apologies if this has been brought up in previous versions, I haven't
>>>>> been
>>>>> following closely]
>>>>>
>>>>> I don't think "backlight_get" is a good name for this function. How about
>>>>> of_find_backlight_by_name (since there's already
>>>>> of_find_backlight_by_node)?
>>>>
>>>>
>>>> I came up with that name modelled after gpiod_get() and gpiod_put() and I
>>>> deliberately kept the of_ part out of the name like the gpio functions.
>>>> gpiod_get() checks OF, ACPI and platform for gpios and calling it
>>>> backlight_get() would keep the door open for other ways of connecting
>>>> backlight devices in the future, other than Device Tree.
>>>>
>>>
>>> Thanks for the background, Noralf! Apologies for stepping on top of
>>> your previous reviews.
>>>
>>>> I think of_find_backlight() would be better than *_by_name(), since
>>>> 'backlight' is the common DT property name, so it wouldn't make much sense
>>>> to require every caller to pass in the same name.
>>>>
>>>
>>> of_find_backlight() sounds like a good compromise.
>>>
>>> Sean
>>
>> Are there any changes that need to be made to this patchset now ?
>>
> 
> I still prefer s/backlight_get/of_find_backlight/ and it seems Noralf
> is Ok with that too. I think there were also review comments
> surrounding the _put function?

How did this conversation result in a new patchset with the name changed?

Did I miss something.


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

Re: [Outreachy kernel] [PATCH v13 2/3] drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c

[Daniel Thompson]

On 20/10/17 18:20, Sean Paul wrote:
> On Wed, Oct 18, 2017 at 1:11 PM, Meghana Madhyastha
> <meghana.madhyastha@gmail.com> wrote:
>> On Mon, Oct 16, 2017 at 02:26:00PM -0400, Sean Paul wrote:
>>> On Fri, Oct 13, 2017 at 6:42 PM, Noralf Trønnes <noralf@tronnes.org> wrote:
>>>>
>>>> Den 13.10.2017 22.25, skrev Sean Paul:
>>>>>
>>>>> On Fri, Oct 13, 2017 at 04:11:43PM +0530, Meghana Madhyastha wrote:
>>>>>>
>>>>>> Rename tinydrm_of_find_backlight to backlight_get and move it
>>>>>> to linux/backlight.c so that it can be used by other drivers.
>>>>>
>>>>> [apologies if this has been brought up in previous versions, I haven't
>>>>> been
>>>>> following closely]
>>>>>
>>>>> I don't think "backlight_get" is a good name for this function. How about
>>>>> of_find_backlight_by_name (since there's already
>>>>> of_find_backlight_by_node)?
>>>>
>>>>
>>>> I came up with that name modelled after gpiod_get() and gpiod_put() and I
>>>> deliberately kept the of_ part out of the name like the gpio functions.
>>>> gpiod_get() checks OF, ACPI and platform for gpios and calling it
>>>> backlight_get() would keep the door open for other ways of connecting
>>>> backlight devices in the future, other than Device Tree.
>>>>
>>>
>>> Thanks for the background, Noralf! Apologies for stepping on top of
>>> your previous reviews.
>>>
>>>> I think of_find_backlight() would be better than *_by_name(), since
>>>> 'backlight' is the common DT property name, so it wouldn't make much sense
>>>> to require every caller to pass in the same name.
>>>>
>>>
>>> of_find_backlight() sounds like a good compromise.
>>>
>>> Sean
>>
>> Are there any changes that need to be made to this patchset now ?
>>
> 
> I still prefer s/backlight_get/of_find_backlight/ and it seems Noralf
> is Ok with that too. I think there were also review comments
> surrounding the _put function?

How did this conversation result in a new patchset with the name changed?

Did I miss something.


Daniel.

Re: [Outreachy kernel] [PATCH v13 2/3] drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c

[Sean Paul]

On Mon, Oct 23, 2017 at 11:35:42AM +0100, Daniel Thompson wrote:
> On 20/10/17 18:20, Sean Paul wrote:
> > On Wed, Oct 18, 2017 at 1:11 PM, Meghana Madhyastha
> > <meghana.madhyastha@gmail.com> wrote:
> > > On Mon, Oct 16, 2017 at 02:26:00PM -0400, Sean Paul wrote:
> > > > On Fri, Oct 13, 2017 at 6:42 PM, Noralf Trønnes <noralf@tronnes.org> wrote:
> > > > > 
> > > > > Den 13.10.2017 22.25, skrev Sean Paul:
> > > > > > 
> > > > > > On Fri, Oct 13, 2017 at 04:11:43PM +0530, Meghana Madhyastha wrote:
> > > > > > > 
> > > > > > > Rename tinydrm_of_find_backlight to backlight_get and move it
> > > > > > > to linux/backlight.c so that it can be used by other drivers.
> > > > > > 
> > > > > > [apologies if this has been brought up in previous versions, I haven't
> > > > > > been
> > > > > > following closely]
> > > > > > 
> > > > > > I don't think "backlight_get" is a good name for this function. How about
> > > > > > of_find_backlight_by_name (since there's already
> > > > > > of_find_backlight_by_node)?
> > > > > 
> > > > > 
> > > > > I came up with that name modelled after gpiod_get() and gpiod_put() and I
> > > > > deliberately kept the of_ part out of the name like the gpio functions.
> > > > > gpiod_get() checks OF, ACPI and platform for gpios and calling it
> > > > > backlight_get() would keep the door open for other ways of connecting
> > > > > backlight devices in the future, other than Device Tree.
> > > > > 
> > > > 
> > > > Thanks for the background, Noralf! Apologies for stepping on top of
> > > > your previous reviews.
> > > > 
> > > > > I think of_find_backlight() would be better than *_by_name(), since
> > > > > 'backlight' is the common DT property name, so it wouldn't make much sense
> > > > > to require every caller to pass in the same name.
> > > > > 
> > > > 
> > > > of_find_backlight() sounds like a good compromise.
> > > > 
> > > > Sean
> > > 
> > > Are there any changes that need to be made to this patchset now ?
> > > 
> > 
> > I still prefer s/backlight_get/of_find_backlight/ and it seems Noralf
> > is Ok with that too. I think there were also review comments
> > surrounding the _put function?
> 
> How did this conversation result in a new patchset with the name changed?
> 
> Did I miss something.
> 

I asked the name be changed to of_find_backlight and that backlight_put be
removed.

Sean

> 
> Daniel.

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Outreachy kernel] [PATCH v13 2/3] drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c

[Sean Paul]

On Mon, Oct 23, 2017 at 11:35:42AM +0100, Daniel Thompson wrote:
> On 20/10/17 18:20, Sean Paul wrote:
> > On Wed, Oct 18, 2017 at 1:11 PM, Meghana Madhyastha
> > <meghana.madhyastha@gmail.com> wrote:
> > > On Mon, Oct 16, 2017 at 02:26:00PM -0400, Sean Paul wrote:
> > > > On Fri, Oct 13, 2017 at 6:42 PM, Noralf Tr�nnes <noralf@tronnes.org> wrote:
> > > > > 
> > > > > Den 13.10.2017 22.25, skrev Sean Paul:
> > > > > > 
> > > > > > On Fri, Oct 13, 2017 at 04:11:43PM +0530, Meghana Madhyastha wrote:
> > > > > > > 
> > > > > > > Rename tinydrm_of_find_backlight to backlight_get and move it
> > > > > > > to linux/backlight.c so that it can be used by other drivers.
> > > > > > 
> > > > > > [apologies if this has been brought up in previous versions, I haven't
> > > > > > been
> > > > > > following closely]
> > > > > > 
> > > > > > I don't think "backlight_get" is a good name for this function. How about
> > > > > > of_find_backlight_by_name (since there's already
> > > > > > of_find_backlight_by_node)?
> > > > > 
> > > > > 
> > > > > I came up with that name modelled after gpiod_get() and gpiod_put() and I
> > > > > deliberately kept the of_ part out of the name like the gpio functions.
> > > > > gpiod_get() checks OF, ACPI and platform for gpios and calling it
> > > > > backlight_get() would keep the door open for other ways of connecting
> > > > > backlight devices in the future, other than Device Tree.
> > > > > 
> > > > 
> > > > Thanks for the background, Noralf! Apologies for stepping on top of
> > > > your previous reviews.
> > > > 
> > > > > I think of_find_backlight() would be better than *_by_name(), since
> > > > > 'backlight' is the common DT property name, so it wouldn't make much sense
> > > > > to require every caller to pass in the same name.
> > > > > 
> > > > 
> > > > of_find_backlight() sounds like a good compromise.
> > > > 
> > > > Sean
> > > 
> > > Are there any changes that need to be made to this patchset now ?
> > > 
> > 
> > I still prefer s/backlight_get/of_find_backlight/ and it seems Noralf
> > is Ok with that too. I think there were also review comments
> > surrounding the _put function?
> 
> How did this conversation result in a new patchset with the name changed?
> 
> Did I miss something.
> 

I asked the name be changed to of_find_backlight and that backlight_put be
removed.

Sean

> 
> Daniel.

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [Outreachy kernel] [PATCH v13 2/3] drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c

[Daniel Thompson]

On 24/10/17 13:16, Sean Paul wrote:
> On Mon, Oct 23, 2017 at 11:35:42AM +0100, Daniel Thompson wrote:
>> On 20/10/17 18:20, Sean Paul wrote:
>>> On Wed, Oct 18, 2017 at 1:11 PM, Meghana Madhyastha
>>> <meghana.madhyastha@gmail.com> wrote:
>>>> On Mon, Oct 16, 2017 at 02:26:00PM -0400, Sean Paul wrote:
>>>>> On Fri, Oct 13, 2017 at 6:42 PM, Noralf Trønnes <noralf@tronnes.org> wrote:
>>>>>>
>>>>>> Den 13.10.2017 22.25, skrev Sean Paul:
>>>>>>>
>>>>>>> On Fri, Oct 13, 2017 at 04:11:43PM +0530, Meghana Madhyastha wrote:
>>>>>>>>
>>>>>>>> Rename tinydrm_of_find_backlight to backlight_get and move it
>>>>>>>> to linux/backlight.c so that it can be used by other drivers.
>>>>>>>
>>>>>>> [apologies if this has been brought up in previous versions, I haven't
>>>>>>> been
>>>>>>> following closely]
>>>>>>>
>>>>>>> I don't think "backlight_get" is a good name for this function. How about
>>>>>>> of_find_backlight_by_name (since there's already
>>>>>>> of_find_backlight_by_node)?
>>>>>>
>>>>>>
>>>>>> I came up with that name modelled after gpiod_get() and gpiod_put() and I
>>>>>> deliberately kept the of_ part out of the name like the gpio functions.
>>>>>> gpiod_get() checks OF, ACPI and platform for gpios and calling it
>>>>>> backlight_get() would keep the door open for other ways of connecting
>>>>>> backlight devices in the future, other than Device Tree.
>>>>>>
>>>>>
>>>>> Thanks for the background, Noralf! Apologies for stepping on top of
>>>>> your previous reviews.
>>>>>
>>>>>> I think of_find_backlight() would be better than *_by_name(), since
>>>>>> 'backlight' is the common DT property name, so it wouldn't make much sense
>>>>>> to require every caller to pass in the same name.
>>>>>>
>>>>>
>>>>> of_find_backlight() sounds like a good compromise.
>>>>>
>>>>> Sean
>>>>
>>>> Are there any changes that need to be made to this patchset now ?
>>>>
>>>
>>> I still prefer s/backlight_get/of_find_backlight/ and it seems Noralf
>>> is Ok with that too. I think there were also review comments
>>> surrounding the _put function?
>>
>> How did this conversation result in a new patchset with the name changed?
>>
>> Did I miss something.
>>
> 
> I asked the name be changed to of_find_backlight and that backlight_put be
> removed.

Quite so.

Sorry, I was filtering my mail way too fast and misread your comment as 
"I still prefer backlight_get" :-O
That was my problem with reading, not your problem with writing.


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

Re: [Outreachy kernel] [PATCH v13 2/3] drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c

[Daniel Thompson]

On 24/10/17 13:16, Sean Paul wrote:
> On Mon, Oct 23, 2017 at 11:35:42AM +0100, Daniel Thompson wrote:
>> On 20/10/17 18:20, Sean Paul wrote:
>>> On Wed, Oct 18, 2017 at 1:11 PM, Meghana Madhyastha
>>> <meghana.madhyastha@gmail.com> wrote:
>>>> On Mon, Oct 16, 2017 at 02:26:00PM -0400, Sean Paul wrote:
>>>>> On Fri, Oct 13, 2017 at 6:42 PM, Noralf Trønnes <noralf@tronnes.org> wrote:
>>>>>>
>>>>>> Den 13.10.2017 22.25, skrev Sean Paul:
>>>>>>>
>>>>>>> On Fri, Oct 13, 2017 at 04:11:43PM +0530, Meghana Madhyastha wrote:
>>>>>>>>
>>>>>>>> Rename tinydrm_of_find_backlight to backlight_get and move it
>>>>>>>> to linux/backlight.c so that it can be used by other drivers.
>>>>>>>
>>>>>>> [apologies if this has been brought up in previous versions, I haven't
>>>>>>> been
>>>>>>> following closely]
>>>>>>>
>>>>>>> I don't think "backlight_get" is a good name for this function. How about
>>>>>>> of_find_backlight_by_name (since there's already
>>>>>>> of_find_backlight_by_node)?
>>>>>>
>>>>>>
>>>>>> I came up with that name modelled after gpiod_get() and gpiod_put() and I
>>>>>> deliberately kept the of_ part out of the name like the gpio functions.
>>>>>> gpiod_get() checks OF, ACPI and platform for gpios and calling it
>>>>>> backlight_get() would keep the door open for other ways of connecting
>>>>>> backlight devices in the future, other than Device Tree.
>>>>>>
>>>>>
>>>>> Thanks for the background, Noralf! Apologies for stepping on top of
>>>>> your previous reviews.
>>>>>
>>>>>> I think of_find_backlight() would be better than *_by_name(), since
>>>>>> 'backlight' is the common DT property name, so it wouldn't make much sense
>>>>>> to require every caller to pass in the same name.
>>>>>>
>>>>>
>>>>> of_find_backlight() sounds like a good compromise.
>>>>>
>>>>> Sean
>>>>
>>>> Are there any changes that need to be made to this patchset now ?
>>>>
>>>
>>> I still prefer s/backlight_get/of_find_backlight/ and it seems Noralf
>>> is Ok with that too. I think there were also review comments
>>> surrounding the _put function?
>>
>> How did this conversation result in a new patchset with the name changed?
>>
>> Did I miss something.
>>
> 
> I asked the name be changed to of_find_backlight and that backlight_put be
> removed.

Quite so.

Sorry, I was filtering my mail way too fast and misread your comment as 
"I still prefer backlight_get" :-O
That was my problem with reading, not your problem with writing.


Daniel.

Re: [Outreachy kernel] [PATCH v13 2/3] drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c

[Sean Paul]

On Tue, Oct 24, 2017 at 01:51:45PM +0100, Daniel Thompson wrote:
> On 24/10/17 13:16, Sean Paul wrote:
> > On Mon, Oct 23, 2017 at 11:35:42AM +0100, Daniel Thompson wrote:
> > > On 20/10/17 18:20, Sean Paul wrote:
> > > > On Wed, Oct 18, 2017 at 1:11 PM, Meghana Madhyastha
> > > > <meghana.madhyastha@gmail.com> wrote:
> > > > > On Mon, Oct 16, 2017 at 02:26:00PM -0400, Sean Paul wrote:
> > > > > > On Fri, Oct 13, 2017 at 6:42 PM, Noralf Trønnes <noralf@tronnes.org> wrote:
> > > > > > > 
> > > > > > > Den 13.10.2017 22.25, skrev Sean Paul:
> > > > > > > > 
> > > > > > > > On Fri, Oct 13, 2017 at 04:11:43PM +0530, Meghana Madhyastha wrote:
> > > > > > > > > 
> > > > > > > > > Rename tinydrm_of_find_backlight to backlight_get and move it
> > > > > > > > > to linux/backlight.c so that it can be used by other drivers.
> > > > > > > > 
> > > > > > > > [apologies if this has been brought up in previous versions, I haven't
> > > > > > > > been
> > > > > > > > following closely]
> > > > > > > > 
> > > > > > > > I don't think "backlight_get" is a good name for this function. How about
> > > > > > > > of_find_backlight_by_name (since there's already
> > > > > > > > of_find_backlight_by_node)?
> > > > > > > 
> > > > > > > 
> > > > > > > I came up with that name modelled after gpiod_get() and gpiod_put() and I
> > > > > > > deliberately kept the of_ part out of the name like the gpio functions.
> > > > > > > gpiod_get() checks OF, ACPI and platform for gpios and calling it
> > > > > > > backlight_get() would keep the door open for other ways of connecting
> > > > > > > backlight devices in the future, other than Device Tree.
> > > > > > > 
> > > > > > 
> > > > > > Thanks for the background, Noralf! Apologies for stepping on top of
> > > > > > your previous reviews.
> > > > > > 
> > > > > > > I think of_find_backlight() would be better than *_by_name(), since
> > > > > > > 'backlight' is the common DT property name, so it wouldn't make much sense
> > > > > > > to require every caller to pass in the same name.
> > > > > > > 
> > > > > > 
> > > > > > of_find_backlight() sounds like a good compromise.
> > > > > > 
> > > > > > Sean
> > > > > 
> > > > > Are there any changes that need to be made to this patchset now ?
> > > > > 
> > > > 
> > > > I still prefer s/backlight_get/of_find_backlight/ and it seems Noralf
> > > > is Ok with that too. I think there were also review comments
> > > > surrounding the _put function?
> > > 
> > > How did this conversation result in a new patchset with the name changed?
> > > 
> > > Did I miss something.
> > > 
> > 
> > I asked the name be changed to of_find_backlight and that backlight_put be
> > removed.
> 
> Quite so.
> 
> Sorry, I was filtering my mail way too fast and misread your comment as "I
> still prefer backlight_get" :-O

I can certainly relate to that problem! I'm happy we got this sorted out :-)

Sean

> That was my problem with reading, not your problem with writing.
> 
> 
> Daniel.

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Outreachy kernel] [PATCH v13 2/3] drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c

[Sean Paul]

On Tue, Oct 24, 2017 at 01:51:45PM +0100, Daniel Thompson wrote:
> On 24/10/17 13:16, Sean Paul wrote:
> > On Mon, Oct 23, 2017 at 11:35:42AM +0100, Daniel Thompson wrote:
> > > On 20/10/17 18:20, Sean Paul wrote:
> > > > On Wed, Oct 18, 2017 at 1:11 PM, Meghana Madhyastha
> > > > <meghana.madhyastha@gmail.com> wrote:
> > > > > On Mon, Oct 16, 2017 at 02:26:00PM -0400, Sean Paul wrote:
> > > > > > On Fri, Oct 13, 2017 at 6:42 PM, Noralf Tr�nnes <noralf@tronnes.org> wrote:
> > > > > > > 
> > > > > > > Den 13.10.2017 22.25, skrev Sean Paul:
> > > > > > > > 
> > > > > > > > On Fri, Oct 13, 2017 at 04:11:43PM +0530, Meghana Madhyastha wrote:
> > > > > > > > > 
> > > > > > > > > Rename tinydrm_of_find_backlight to backlight_get and move it
> > > > > > > > > to linux/backlight.c so that it can be used by other drivers.
> > > > > > > > 
> > > > > > > > [apologies if this has been brought up in previous versions, I haven't
> > > > > > > > been
> > > > > > > > following closely]
> > > > > > > > 
> > > > > > > > I don't think "backlight_get" is a good name for this function. How about
> > > > > > > > of_find_backlight_by_name (since there's already
> > > > > > > > of_find_backlight_by_node)?
> > > > > > > 
> > > > > > > 
> > > > > > > I came up with that name modelled after gpiod_get() and gpiod_put() and I
> > > > > > > deliberately kept the of_ part out of the name like the gpio functions.
> > > > > > > gpiod_get() checks OF, ACPI and platform for gpios and calling it
> > > > > > > backlight_get() would keep the door open for other ways of connecting
> > > > > > > backlight devices in the future, other than Device Tree.
> > > > > > > 
> > > > > > 
> > > > > > Thanks for the background, Noralf! Apologies for stepping on top of
> > > > > > your previous reviews.
> > > > > > 
> > > > > > > I think of_find_backlight() would be better than *_by_name(), since
> > > > > > > 'backlight' is the common DT property name, so it wouldn't make much sense
> > > > > > > to require every caller to pass in the same name.
> > > > > > > 
> > > > > > 
> > > > > > of_find_backlight() sounds like a good compromise.
> > > > > > 
> > > > > > Sean
> > > > > 
> > > > > Are there any changes that need to be made to this patchset now ?
> > > > > 
> > > > 
> > > > I still prefer s/backlight_get/of_find_backlight/ and it seems Noralf
> > > > is Ok with that too. I think there were also review comments
> > > > surrounding the _put function?
> > > 
> > > How did this conversation result in a new patchset with the name changed?
> > > 
> > > Did I miss something.
> > > 
> > 
> > I asked the name be changed to of_find_backlight and that backlight_put be
> > removed.
> 
> Quite so.
> 
> Sorry, I was filtering my mail way too fast and misread your comment as "I
> still prefer backlight_get" :-O

I can certainly relate to that problem! I'm happy we got this sorted out :-)

Sean

> That was my problem with reading, not your problem with writing.
> 
> 
> Daniel.

-- 
Sean Paul, Software Engineer, Google / Chromium OS