* [PATCH] drm/tinydrm: Move tinydrm_of_find_backlight into drm_of.c
@ 2017-09-27 13:54 ` Meghana Madhyastha
0 siblings, 0 replies; 14+ messages in thread
From: Meghana Madhyastha @ 2017-09-27 13:54 UTC (permalink / raw)
To: daniel, noralf, outreachy-kernel, dri-devel
Rename tinydrm_of_find_backlight to drm_of_find_backlight
and move it into drm_of.c from tinydrm-helpers.c. This is
because other drivers in the drm subsystem might need to call
this function. In that case and otherwise, it is better from
an organizational point of view to move it into drm_of.c along
with the other _of.c functions.
Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
---
drivers/gpu/drm/drm_of.c | 41 ++++++++++++++++++++++++++
drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 40 -------------------------
drivers/gpu/drm/tinydrm/mi0283qt.c | 3 +-
include/drm/drm_of.h | 1 +
include/drm/tinydrm/tinydrm-helpers.h | 1 -
5 files changed, 44 insertions(+), 42 deletions(-)
diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
index 8dafbdf..d8cded3 100644
--- a/drivers/gpu/drm/drm_of.c
+++ b/drivers/gpu/drm/drm_of.c
@@ -1,6 +1,7 @@
#include <linux/component.h>
#include <linux/export.h>
#include <linux/list.h>
+#include <linux/backlight.h>
#include <linux/of_graph.h>
#include <drm/drmP.h>
#include <drm/drm_bridge.h>
@@ -260,3 +261,43 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
return ret;
}
EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge);
+
+/**
+ * drm_of_find_backlight - Find backlight device in device-tree
+ * @dev: Device
+ *
+ * This function looks for a DT node pointed to by a property named 'backlight'
+ * and uses of_find_backlight_by_node() to get the backlight device.
+ * Additionally if the brightness property is zero, it is set to
+ * max_brightness.
+ *
+ * Returns:
+ * NULL if there's no backlight property.
+ * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
+ * is found.
+ * If the backlight device is found, a pointer to the structure is returned.
+ */
+struct backlight_device *drm_of_find_backlight(struct device *dev)
+{
+ struct backlight_device *backlight;
+ struct device_node *np;
+
+ np = of_parse_phandle(dev->of_node, "backlight", 0);
+ if (!np)
+ return NULL;
+
+ backlight = of_find_backlight_by_node(np);
+ of_node_put(np);
+
+ if (!backlight)
+ return ERR_PTR(-EPROBE_DEFER);
+
+ if (!backlight->props.brightness) {
+ backlight->props.brightness = backlight->props.max_brightness;
+ DRM_DEBUG_KMS("Backlight brightness set to %d\n",
+ backlight->props.brightness);
+ }
+
+ return backlight;
+}
+EXPORT_SYMBOL(drm_of_find_backlight);
diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
index bd6cce0..cd4c6a5 100644
--- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
+++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
@@ -237,46 +237,6 @@ void tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb,
EXPORT_SYMBOL(tinydrm_xrgb8888_to_gray8);
/**
- * tinydrm_of_find_backlight - Find backlight device in device-tree
- * @dev: Device
- *
- * This function looks for a DT node pointed to by a property named 'backlight'
- * and uses of_find_backlight_by_node() to get the backlight device.
- * Additionally if the brightness property is zero, it is set to
- * max_brightness.
- *
- * Returns:
- * NULL if there's no backlight property.
- * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
- * is found.
- * If the backlight device is found, a pointer to the structure is returned.
- */
-struct backlight_device *tinydrm_of_find_backlight(struct device *dev)
-{
- struct backlight_device *backlight;
- struct device_node *np;
-
- np = of_parse_phandle(dev->of_node, "backlight", 0);
- if (!np)
- return NULL;
-
- backlight = of_find_backlight_by_node(np);
- of_node_put(np);
-
- if (!backlight)
- return ERR_PTR(-EPROBE_DEFER);
-
- if (!backlight->props.brightness) {
- backlight->props.brightness = backlight->props.max_brightness;
- DRM_DEBUG_KMS("Backlight brightness set to %d\n",
- backlight->props.brightness);
- }
-
- return backlight;
-}
-EXPORT_SYMBOL(tinydrm_of_find_backlight);
-
-/**
* tinydrm_enable_backlight - Enable backlight helper
* @backlight: Backlight device
*
diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
index 7e5bb7d..5e3d635 100644
--- a/drivers/gpu/drm/tinydrm/mi0283qt.c
+++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
@@ -12,6 +12,7 @@
#include <drm/tinydrm/ili9341.h>
#include <drm/tinydrm/mipi-dbi.h>
#include <drm/tinydrm/tinydrm-helpers.h>
+#include <drm/drm_of.h>
#include <linux/delay.h>
#include <linux/gpio/consumer.h>
#include <linux/module.h>
@@ -189,7 +190,7 @@ static int mi0283qt_probe(struct spi_device *spi)
if (IS_ERR(mipi->regulator))
return PTR_ERR(mipi->regulator);
- mipi->backlight = tinydrm_of_find_backlight(dev);
+ mipi->backlight = drm_of_find_backlight(dev);
if (IS_ERR(mipi->backlight))
return PTR_ERR(mipi->backlight);
diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h
index 104dd51..e8fba5b 100644
--- a/include/drm/drm_of.h
+++ b/include/drm/drm_of.h
@@ -29,6 +29,7 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
int port, int endpoint,
struct drm_panel **panel,
struct drm_bridge **bridge);
+struct backlight_device *drm_of_find_backlight(struct device *dev);
#else
static inline uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
struct device_node *port)
diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h
index d554ded..e40ef2d 100644
--- a/include/drm/tinydrm/tinydrm-helpers.h
+++ b/include/drm/tinydrm/tinydrm-helpers.h
@@ -46,7 +46,6 @@ void tinydrm_xrgb8888_to_rgb565(u16 *dst, void *vaddr,
void tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb,
struct drm_clip_rect *clip);
-struct backlight_device *tinydrm_of_find_backlight(struct device *dev);
int tinydrm_enable_backlight(struct backlight_device *backlight);
int tinydrm_disable_backlight(struct backlight_device *backlight);
--
2.7.4
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH] drm/tinydrm: Move tinydrm_of_find_backlight into drm_of.c
@ 2017-09-27 13:54 ` Meghana Madhyastha
0 siblings, 0 replies; 14+ messages in thread
From: Meghana Madhyastha @ 2017-09-27 13:54 UTC (permalink / raw)
To: daniel, noralf, outreachy-kernel, dri-devel
Rename tinydrm_of_find_backlight to drm_of_find_backlight
and move it into drm_of.c from tinydrm-helpers.c. This is
because other drivers in the drm subsystem might need to call
this function. In that case and otherwise, it is better from
an organizational point of view to move it into drm_of.c along
with the other _of.c functions.
Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
---
drivers/gpu/drm/drm_of.c | 41 ++++++++++++++++++++++++++
drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 40 -------------------------
drivers/gpu/drm/tinydrm/mi0283qt.c | 3 +-
include/drm/drm_of.h | 1 +
include/drm/tinydrm/tinydrm-helpers.h | 1 -
5 files changed, 44 insertions(+), 42 deletions(-)
diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
index 8dafbdf..d8cded3 100644
--- a/drivers/gpu/drm/drm_of.c
+++ b/drivers/gpu/drm/drm_of.c
@@ -1,6 +1,7 @@
#include <linux/component.h>
#include <linux/export.h>
#include <linux/list.h>
+#include <linux/backlight.h>
#include <linux/of_graph.h>
#include <drm/drmP.h>
#include <drm/drm_bridge.h>
@@ -260,3 +261,43 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
return ret;
}
EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge);
+
+/**
+ * drm_of_find_backlight - Find backlight device in device-tree
+ * @dev: Device
+ *
+ * This function looks for a DT node pointed to by a property named 'backlight'
+ * and uses of_find_backlight_by_node() to get the backlight device.
+ * Additionally if the brightness property is zero, it is set to
+ * max_brightness.
+ *
+ * Returns:
+ * NULL if there's no backlight property.
+ * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
+ * is found.
+ * If the backlight device is found, a pointer to the structure is returned.
+ */
+struct backlight_device *drm_of_find_backlight(struct device *dev)
+{
+ struct backlight_device *backlight;
+ struct device_node *np;
+
+ np = of_parse_phandle(dev->of_node, "backlight", 0);
+ if (!np)
+ return NULL;
+
+ backlight = of_find_backlight_by_node(np);
+ of_node_put(np);
+
+ if (!backlight)
+ return ERR_PTR(-EPROBE_DEFER);
+
+ if (!backlight->props.brightness) {
+ backlight->props.brightness = backlight->props.max_brightness;
+ DRM_DEBUG_KMS("Backlight brightness set to %d\n",
+ backlight->props.brightness);
+ }
+
+ return backlight;
+}
+EXPORT_SYMBOL(drm_of_find_backlight);
diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
index bd6cce0..cd4c6a5 100644
--- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
+++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
@@ -237,46 +237,6 @@ void tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb,
EXPORT_SYMBOL(tinydrm_xrgb8888_to_gray8);
/**
- * tinydrm_of_find_backlight - Find backlight device in device-tree
- * @dev: Device
- *
- * This function looks for a DT node pointed to by a property named 'backlight'
- * and uses of_find_backlight_by_node() to get the backlight device.
- * Additionally if the brightness property is zero, it is set to
- * max_brightness.
- *
- * Returns:
- * NULL if there's no backlight property.
- * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
- * is found.
- * If the backlight device is found, a pointer to the structure is returned.
- */
-struct backlight_device *tinydrm_of_find_backlight(struct device *dev)
-{
- struct backlight_device *backlight;
- struct device_node *np;
-
- np = of_parse_phandle(dev->of_node, "backlight", 0);
- if (!np)
- return NULL;
-
- backlight = of_find_backlight_by_node(np);
- of_node_put(np);
-
- if (!backlight)
- return ERR_PTR(-EPROBE_DEFER);
-
- if (!backlight->props.brightness) {
- backlight->props.brightness = backlight->props.max_brightness;
- DRM_DEBUG_KMS("Backlight brightness set to %d\n",
- backlight->props.brightness);
- }
-
- return backlight;
-}
-EXPORT_SYMBOL(tinydrm_of_find_backlight);
-
-/**
* tinydrm_enable_backlight - Enable backlight helper
* @backlight: Backlight device
*
diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
index 7e5bb7d..5e3d635 100644
--- a/drivers/gpu/drm/tinydrm/mi0283qt.c
+++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
@@ -12,6 +12,7 @@
#include <drm/tinydrm/ili9341.h>
#include <drm/tinydrm/mipi-dbi.h>
#include <drm/tinydrm/tinydrm-helpers.h>
+#include <drm/drm_of.h>
#include <linux/delay.h>
#include <linux/gpio/consumer.h>
#include <linux/module.h>
@@ -189,7 +190,7 @@ static int mi0283qt_probe(struct spi_device *spi)
if (IS_ERR(mipi->regulator))
return PTR_ERR(mipi->regulator);
- mipi->backlight = tinydrm_of_find_backlight(dev);
+ mipi->backlight = drm_of_find_backlight(dev);
if (IS_ERR(mipi->backlight))
return PTR_ERR(mipi->backlight);
diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h
index 104dd51..e8fba5b 100644
--- a/include/drm/drm_of.h
+++ b/include/drm/drm_of.h
@@ -29,6 +29,7 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
int port, int endpoint,
struct drm_panel **panel,
struct drm_bridge **bridge);
+struct backlight_device *drm_of_find_backlight(struct device *dev);
#else
static inline uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
struct device_node *port)
diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h
index d554ded..e40ef2d 100644
--- a/include/drm/tinydrm/tinydrm-helpers.h
+++ b/include/drm/tinydrm/tinydrm-helpers.h
@@ -46,7 +46,6 @@ void tinydrm_xrgb8888_to_rgb565(u16 *dst, void *vaddr,
void tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb,
struct drm_clip_rect *clip);
-struct backlight_device *tinydrm_of_find_backlight(struct device *dev);
int tinydrm_enable_backlight(struct backlight_device *backlight);
int tinydrm_disable_backlight(struct backlight_device *backlight);
--
2.7.4
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/tinydrm: Move tinydrm_of_find_backlight into drm_of.c
2017-09-27 13:54 ` Meghana Madhyastha
@ 2017-09-27 15:08 ` Noralf Trønnes
-1 siblings, 0 replies; 14+ messages in thread
From: Noralf Trønnes @ 2017-09-27 15:08 UTC (permalink / raw)
To: Meghana Madhyastha, daniel, outreachy-kernel, dri-devel
Den 27.09.2017 15.54, skrev Meghana Madhyastha:
> Rename tinydrm_of_find_backlight to drm_of_find_backlight
> and move it into drm_of.c from tinydrm-helpers.c. This is
> because other drivers in the drm subsystem might need to call
> this function. In that case and otherwise, it is better from
> an organizational point of view to move it into drm_of.c along
> with the other _of.c functions.
>
> Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
> ---
I suggest you split this patch in 2, one to add the function and one to
use it in tinydrm.
> drivers/gpu/drm/drm_of.c | 41 ++++++++++++++++++++++++++
> drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 40 -------------------------
> drivers/gpu/drm/tinydrm/mi0283qt.c | 3 +-
> include/drm/drm_of.h | 1 +
> include/drm/tinydrm/tinydrm-helpers.h | 1 -
> 5 files changed, 44 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> index 8dafbdf..d8cded3 100644
> --- a/drivers/gpu/drm/drm_of.c
> +++ b/drivers/gpu/drm/drm_of.c
> @@ -1,6 +1,7 @@
> #include <linux/component.h>
> #include <linux/export.h>
> #include <linux/list.h>
> +#include <linux/backlight.h>
> #include <linux/of_graph.h>
> #include <drm/drmP.h>
> #include <drm/drm_bridge.h>
> @@ -260,3 +261,43 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> return ret;
> }
> EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge);
> +
> +/**
> + * drm_of_find_backlight - Find backlight device in device-tree
> + * @dev: Device
> + *
> + * This function looks for a DT node pointed to by a property named 'backlight'
> + * and uses of_find_backlight_by_node() to get the backlight device.
> + * Additionally if the brightness property is zero, it is set to
> + * max_brightness.
Please add a note here about the callers responsibility to call
put_device() when releasing the resource.
See the of_find_backlight_by_node() docs.
> + *
> + * Returns:
> + * NULL if there's no backlight property.
> + * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
> + * is found.
> + * If the backlight device is found, a pointer to the structure is returned.
> + */
> +struct backlight_device *drm_of_find_backlight(struct device *dev)
> +{
> + struct backlight_device *backlight;
> + struct device_node *np;
> +
> + np = of_parse_phandle(dev->of_node, "backlight", 0);
> + if (!np)
> + return NULL;
> +
> + backlight = of_find_backlight_by_node(np);
> + of_node_put(np);
> +
> + if (!backlight)
> + return ERR_PTR(-EPROBE_DEFER);
> +
> + if (!backlight->props.brightness) {
> + backlight->props.brightness = backlight->props.max_brightness;
> + DRM_DEBUG_KMS("Backlight brightness set to %d\n",
> + backlight->props.brightness);
> + }
> +
> + return backlight;
> +}
> +EXPORT_SYMBOL(drm_of_find_backlight);
Can you also please add a devm_ version of this function. tinydrm uses
devres[1] versions of functions for requiring device resources, so it
would be nice to do this for backlight as well. tinydrm is currently
broken in this respect, it doesn't put the backlight device.
I had a devm_of_find_backlight() version lying around that I've adjusted
to give you an idea of what I'm talking about:
static void devm_drm_of_find_backlight_release(void *data)
{
put_device(data);
}
struct backlight_device *devm_drm_of_find_backlight(struct device *dev)
{
struct backlight_device *backlight;
int ret;
backlight = drm_of_find_backlight(dev);
if (IS_ERR_OR_NULL(backlight))
return backlight;
ret = devm_add_action(dev, devm_drm_of_find_backlight_release,
backlight->dev);
if (ret) {
put_device(backlight->dev);
return ERR_PTR(ret);
}
return backlight;
}
[1] https://www.kernel.org/doc/Documentation/driver-model/devres.txt
Noralf.
> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> index bd6cce0..cd4c6a5 100644
> --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> @@ -237,46 +237,6 @@ void tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb,
> EXPORT_SYMBOL(tinydrm_xrgb8888_to_gray8);
>
> /**
> - * tinydrm_of_find_backlight - Find backlight device in device-tree
> - * @dev: Device
> - *
> - * This function looks for a DT node pointed to by a property named 'backlight'
> - * and uses of_find_backlight_by_node() to get the backlight device.
> - * Additionally if the brightness property is zero, it is set to
> - * max_brightness.
> - *
> - * Returns:
> - * NULL if there's no backlight property.
> - * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
> - * is found.
> - * If the backlight device is found, a pointer to the structure is returned.
> - */
> -struct backlight_device *tinydrm_of_find_backlight(struct device *dev)
> -{
> - struct backlight_device *backlight;
> - struct device_node *np;
> -
> - np = of_parse_phandle(dev->of_node, "backlight", 0);
> - if (!np)
> - return NULL;
> -
> - backlight = of_find_backlight_by_node(np);
> - of_node_put(np);
> -
> - if (!backlight)
> - return ERR_PTR(-EPROBE_DEFER);
> -
> - if (!backlight->props.brightness) {
> - backlight->props.brightness = backlight->props.max_brightness;
> - DRM_DEBUG_KMS("Backlight brightness set to %d\n",
> - backlight->props.brightness);
> - }
> -
> - return backlight;
> -}
> -EXPORT_SYMBOL(tinydrm_of_find_backlight);
> -
> -/**
> * tinydrm_enable_backlight - Enable backlight helper
> * @backlight: Backlight device
> *
> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
> index 7e5bb7d..5e3d635 100644
> --- a/drivers/gpu/drm/tinydrm/mi0283qt.c
> +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
> @@ -12,6 +12,7 @@
> #include <drm/tinydrm/ili9341.h>
> #include <drm/tinydrm/mipi-dbi.h>
> #include <drm/tinydrm/tinydrm-helpers.h>
> +#include <drm/drm_of.h>
> #include <linux/delay.h>
> #include <linux/gpio/consumer.h>
> #include <linux/module.h>
> @@ -189,7 +190,7 @@ static int mi0283qt_probe(struct spi_device *spi)
> if (IS_ERR(mipi->regulator))
> return PTR_ERR(mipi->regulator);
>
> - mipi->backlight = tinydrm_of_find_backlight(dev);
> + mipi->backlight = drm_of_find_backlight(dev);
> if (IS_ERR(mipi->backlight))
> return PTR_ERR(mipi->backlight);
>
> diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h
> index 104dd51..e8fba5b 100644
> --- a/include/drm/drm_of.h
> +++ b/include/drm/drm_of.h
> @@ -29,6 +29,7 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> int port, int endpoint,
> struct drm_panel **panel,
> struct drm_bridge **bridge);
> +struct backlight_device *drm_of_find_backlight(struct device *dev);
> #else
> static inline uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
> struct device_node *port)
> diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h
> index d554ded..e40ef2d 100644
> --- a/include/drm/tinydrm/tinydrm-helpers.h
> +++ b/include/drm/tinydrm/tinydrm-helpers.h
> @@ -46,7 +46,6 @@ void tinydrm_xrgb8888_to_rgb565(u16 *dst, void *vaddr,
> void tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb,
> struct drm_clip_rect *clip);
>
> -struct backlight_device *tinydrm_of_find_backlight(struct device *dev);
> int tinydrm_enable_backlight(struct backlight_device *backlight);
> int tinydrm_disable_backlight(struct backlight_device *backlight);
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/tinydrm: Move tinydrm_of_find_backlight into drm_of.c
@ 2017-09-27 15:08 ` Noralf Trønnes
0 siblings, 0 replies; 14+ messages in thread
From: Noralf Trønnes @ 2017-09-27 15:08 UTC (permalink / raw)
To: Meghana Madhyastha, daniel, outreachy-kernel, dri-devel
Den 27.09.2017 15.54, skrev Meghana Madhyastha:
> Rename tinydrm_of_find_backlight to drm_of_find_backlight
> and move it into drm_of.c from tinydrm-helpers.c. This is
> because other drivers in the drm subsystem might need to call
> this function. In that case and otherwise, it is better from
> an organizational point of view to move it into drm_of.c along
> with the other _of.c functions.
>
> Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
> ---
I suggest you split this patch in 2, one to add the function and one to
use it in tinydrm.
> drivers/gpu/drm/drm_of.c | 41 ++++++++++++++++++++++++++
> drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 40 -------------------------
> drivers/gpu/drm/tinydrm/mi0283qt.c | 3 +-
> include/drm/drm_of.h | 1 +
> include/drm/tinydrm/tinydrm-helpers.h | 1 -
> 5 files changed, 44 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> index 8dafbdf..d8cded3 100644
> --- a/drivers/gpu/drm/drm_of.c
> +++ b/drivers/gpu/drm/drm_of.c
> @@ -1,6 +1,7 @@
> #include <linux/component.h>
> #include <linux/export.h>
> #include <linux/list.h>
> +#include <linux/backlight.h>
> #include <linux/of_graph.h>
> #include <drm/drmP.h>
> #include <drm/drm_bridge.h>
> @@ -260,3 +261,43 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> return ret;
> }
> EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge);
> +
> +/**
> + * drm_of_find_backlight - Find backlight device in device-tree
> + * @dev: Device
> + *
> + * This function looks for a DT node pointed to by a property named 'backlight'
> + * and uses of_find_backlight_by_node() to get the backlight device.
> + * Additionally if the brightness property is zero, it is set to
> + * max_brightness.
Please add a note here about the callers responsibility to call
put_device() when releasing the resource.
See the of_find_backlight_by_node() docs.
> + *
> + * Returns:
> + * NULL if there's no backlight property.
> + * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
> + * is found.
> + * If the backlight device is found, a pointer to the structure is returned.
> + */
> +struct backlight_device *drm_of_find_backlight(struct device *dev)
> +{
> + struct backlight_device *backlight;
> + struct device_node *np;
> +
> + np = of_parse_phandle(dev->of_node, "backlight", 0);
> + if (!np)
> + return NULL;
> +
> + backlight = of_find_backlight_by_node(np);
> + of_node_put(np);
> +
> + if (!backlight)
> + return ERR_PTR(-EPROBE_DEFER);
> +
> + if (!backlight->props.brightness) {
> + backlight->props.brightness = backlight->props.max_brightness;
> + DRM_DEBUG_KMS("Backlight brightness set to %d\n",
> + backlight->props.brightness);
> + }
> +
> + return backlight;
> +}
> +EXPORT_SYMBOL(drm_of_find_backlight);
Can you also please add a devm_ version of this function. tinydrm uses
devres[1] versions of functions for requiring device resources, so it
would be nice to do this for backlight as well. tinydrm is currently
broken in this respect, it doesn't put the backlight device.
I had a devm_of_find_backlight() version lying around that I've adjusted
to give you an idea of what I'm talking about:
static void devm_drm_of_find_backlight_release(void *data)
{
put_device(data);
}
struct backlight_device *devm_drm_of_find_backlight(struct device *dev)
{
struct backlight_device *backlight;
int ret;
backlight = drm_of_find_backlight(dev);
if (IS_ERR_OR_NULL(backlight))
return backlight;
ret = devm_add_action(dev, devm_drm_of_find_backlight_release,
backlight->dev);
if (ret) {
put_device(backlight->dev);
return ERR_PTR(ret);
}
return backlight;
}
[1] https://www.kernel.org/doc/Documentation/driver-model/devres.txt
Noralf.
> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> index bd6cce0..cd4c6a5 100644
> --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> @@ -237,46 +237,6 @@ void tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb,
> EXPORT_SYMBOL(tinydrm_xrgb8888_to_gray8);
>
> /**
> - * tinydrm_of_find_backlight - Find backlight device in device-tree
> - * @dev: Device
> - *
> - * This function looks for a DT node pointed to by a property named 'backlight'
> - * and uses of_find_backlight_by_node() to get the backlight device.
> - * Additionally if the brightness property is zero, it is set to
> - * max_brightness.
> - *
> - * Returns:
> - * NULL if there's no backlight property.
> - * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
> - * is found.
> - * If the backlight device is found, a pointer to the structure is returned.
> - */
> -struct backlight_device *tinydrm_of_find_backlight(struct device *dev)
> -{
> - struct backlight_device *backlight;
> - struct device_node *np;
> -
> - np = of_parse_phandle(dev->of_node, "backlight", 0);
> - if (!np)
> - return NULL;
> -
> - backlight = of_find_backlight_by_node(np);
> - of_node_put(np);
> -
> - if (!backlight)
> - return ERR_PTR(-EPROBE_DEFER);
> -
> - if (!backlight->props.brightness) {
> - backlight->props.brightness = backlight->props.max_brightness;
> - DRM_DEBUG_KMS("Backlight brightness set to %d\n",
> - backlight->props.brightness);
> - }
> -
> - return backlight;
> -}
> -EXPORT_SYMBOL(tinydrm_of_find_backlight);
> -
> -/**
> * tinydrm_enable_backlight - Enable backlight helper
> * @backlight: Backlight device
> *
> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
> index 7e5bb7d..5e3d635 100644
> --- a/drivers/gpu/drm/tinydrm/mi0283qt.c
> +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
> @@ -12,6 +12,7 @@
> #include <drm/tinydrm/ili9341.h>
> #include <drm/tinydrm/mipi-dbi.h>
> #include <drm/tinydrm/tinydrm-helpers.h>
> +#include <drm/drm_of.h>
> #include <linux/delay.h>
> #include <linux/gpio/consumer.h>
> #include <linux/module.h>
> @@ -189,7 +190,7 @@ static int mi0283qt_probe(struct spi_device *spi)
> if (IS_ERR(mipi->regulator))
> return PTR_ERR(mipi->regulator);
>
> - mipi->backlight = tinydrm_of_find_backlight(dev);
> + mipi->backlight = drm_of_find_backlight(dev);
> if (IS_ERR(mipi->backlight))
> return PTR_ERR(mipi->backlight);
>
> diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h
> index 104dd51..e8fba5b 100644
> --- a/include/drm/drm_of.h
> +++ b/include/drm/drm_of.h
> @@ -29,6 +29,7 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> int port, int endpoint,
> struct drm_panel **panel,
> struct drm_bridge **bridge);
> +struct backlight_device *drm_of_find_backlight(struct device *dev);
> #else
> static inline uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
> struct device_node *port)
> diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h
> index d554ded..e40ef2d 100644
> --- a/include/drm/tinydrm/tinydrm-helpers.h
> +++ b/include/drm/tinydrm/tinydrm-helpers.h
> @@ -46,7 +46,6 @@ void tinydrm_xrgb8888_to_rgb565(u16 *dst, void *vaddr,
> void tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb,
> struct drm_clip_rect *clip);
>
> -struct backlight_device *tinydrm_of_find_backlight(struct device *dev);
> int tinydrm_enable_backlight(struct backlight_device *backlight);
> int tinydrm_disable_backlight(struct backlight_device *backlight);
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/tinydrm: Move tinydrm_of_find_backlight into drm_of.c
2017-09-27 15:08 ` Noralf Trønnes
@ 2017-09-28 7:15 ` Daniel Vetter
-1 siblings, 0 replies; 14+ messages in thread
From: Daniel Vetter @ 2017-09-28 7:15 UTC (permalink / raw)
To: Noralf Trønnes; +Cc: Meghana Madhyastha, outreachy-kernel, dri-devel
On Wed, Sep 27, 2017 at 05:08:07PM +0200, Noralf Trønnes wrote:
>
> Den 27.09.2017 15.54, skrev Meghana Madhyastha:
> > Rename tinydrm_of_find_backlight to drm_of_find_backlight
> > and move it into drm_of.c from tinydrm-helpers.c. This is
> > because other drivers in the drm subsystem might need to call
> > this function. In that case and otherwise, it is better from
> > an organizational point of view to move it into drm_of.c along
> > with the other _of.c functions.
> >
> > Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
> > ---
>
> I suggest you split this patch in 2, one to add the function and one to
> use it in tinydrm.
In general I'd agree to split into 3 phases: 1) add new function 2)
convert drivers 3) remove old one.
But since there's only 1 caller this seems like overkill.
> > drivers/gpu/drm/drm_of.c | 41 ++++++++++++++++++++++++++
> > drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 40 -------------------------
> > drivers/gpu/drm/tinydrm/mi0283qt.c | 3 +-
> > include/drm/drm_of.h | 1 +
> > include/drm/tinydrm/tinydrm-helpers.h | 1 -
> > 5 files changed, 44 insertions(+), 42 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> > index 8dafbdf..d8cded3 100644
> > --- a/drivers/gpu/drm/drm_of.c
> > +++ b/drivers/gpu/drm/drm_of.c
> > @@ -1,6 +1,7 @@
> > #include <linux/component.h>
> > #include <linux/export.h>
> > #include <linux/list.h>
> > +#include <linux/backlight.h>
> > #include <linux/of_graph.h>
> > #include <drm/drmP.h>
> > #include <drm/drm_bridge.h>
> > @@ -260,3 +261,43 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> > return ret;
> > }
> > EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge);
> > +
> > +/**
> > + * drm_of_find_backlight - Find backlight device in device-tree
> > + * @dev: Device
> > + *
> > + * This function looks for a DT node pointed to by a property named 'backlight'
> > + * and uses of_find_backlight_by_node() to get the backlight device.
> > + * Additionally if the brightness property is zero, it is set to
> > + * max_brightness.
>
> Please add a note here about the callers responsibility to call
> put_device() when releasing the resource.
> See the of_find_backlight_by_node() docs.
>
> > + *
> > + * Returns:
> > + * NULL if there's no backlight property.
> > + * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
> > + * is found.
> > + * If the backlight device is found, a pointer to the structure is returned.
> > + */
> > +struct backlight_device *drm_of_find_backlight(struct device *dev)
> > +{
> > + struct backlight_device *backlight;
> > + struct device_node *np;
> > +
> > + np = of_parse_phandle(dev->of_node, "backlight", 0);
> > + if (!np)
> > + return NULL;
> > +
> > + backlight = of_find_backlight_by_node(np);
> > + of_node_put(np);
> > +
> > + if (!backlight)
> > + return ERR_PTR(-EPROBE_DEFER);
> > +
> > + if (!backlight->props.brightness) {
> > + backlight->props.brightness = backlight->props.max_brightness;
> > + DRM_DEBUG_KMS("Backlight brightness set to %d\n",
> > + backlight->props.brightness);
> > + }
> > +
> > + return backlight;
> > +}
> > +EXPORT_SYMBOL(drm_of_find_backlight);
>
> Can you also please add a devm_ version of this function. tinydrm uses
> devres[1] versions of functions for requiring device resources, so it
> would be nice to do this for backlight as well. tinydrm is currently
> broken in this respect, it doesn't put the backlight device.
>
> I had a devm_of_find_backlight() version lying around that I've adjusted
> to give you an idea of what I'm talking about:
>
> static void devm_drm_of_find_backlight_release(void *data)
> {
> put_device(data);
> }
>
> struct backlight_device *devm_drm_of_find_backlight(struct device *dev)
> {
> struct backlight_device *backlight;
> int ret;
>
> backlight = drm_of_find_backlight(dev);
> if (IS_ERR_OR_NULL(backlight))
> return backlight;
>
> ret = devm_add_action(dev, devm_drm_of_find_backlight_release,
> backlight->dev);
> if (ret) {
> put_device(backlight->dev);
> return ERR_PTR(ret);
> }
>
> return backlight;
> }
Good idea for a follow up patch imo.
Another follow up patch series which would be really good is then
converting drivers over to using this (preferrably the devm_ version). A
quick git grep shows there's quite a few.
Either way, the patch that adds these to the core should also add a TODO
entry so we don't forget to complete this conversion.
Thanks, Daniel
>
> [1] https://www.kernel.org/doc/Documentation/driver-model/devres.txt
>
> Noralf.
>
> > diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> > index bd6cce0..cd4c6a5 100644
> > --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> > +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> > @@ -237,46 +237,6 @@ void tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb,
> > EXPORT_SYMBOL(tinydrm_xrgb8888_to_gray8);
> > /**
> > - * tinydrm_of_find_backlight - Find backlight device in device-tree
> > - * @dev: Device
> > - *
> > - * This function looks for a DT node pointed to by a property named 'backlight'
> > - * and uses of_find_backlight_by_node() to get the backlight device.
> > - * Additionally if the brightness property is zero, it is set to
> > - * max_brightness.
> > - *
> > - * Returns:
> > - * NULL if there's no backlight property.
> > - * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
> > - * is found.
> > - * If the backlight device is found, a pointer to the structure is returned.
> > - */
> > -struct backlight_device *tinydrm_of_find_backlight(struct device *dev)
> > -{
> > - struct backlight_device *backlight;
> > - struct device_node *np;
> > -
> > - np = of_parse_phandle(dev->of_node, "backlight", 0);
> > - if (!np)
> > - return NULL;
> > -
> > - backlight = of_find_backlight_by_node(np);
> > - of_node_put(np);
> > -
> > - if (!backlight)
> > - return ERR_PTR(-EPROBE_DEFER);
> > -
> > - if (!backlight->props.brightness) {
> > - backlight->props.brightness = backlight->props.max_brightness;
> > - DRM_DEBUG_KMS("Backlight brightness set to %d\n",
> > - backlight->props.brightness);
> > - }
> > -
> > - return backlight;
> > -}
> > -EXPORT_SYMBOL(tinydrm_of_find_backlight);
> > -
> > -/**
> > * tinydrm_enable_backlight - Enable backlight helper
> > * @backlight: Backlight device
> > *
> > diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
> > index 7e5bb7d..5e3d635 100644
> > --- a/drivers/gpu/drm/tinydrm/mi0283qt.c
> > +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
> > @@ -12,6 +12,7 @@
> > #include <drm/tinydrm/ili9341.h>
> > #include <drm/tinydrm/mipi-dbi.h>
> > #include <drm/tinydrm/tinydrm-helpers.h>
> > +#include <drm/drm_of.h>
> > #include <linux/delay.h>
> > #include <linux/gpio/consumer.h>
> > #include <linux/module.h>
> > @@ -189,7 +190,7 @@ static int mi0283qt_probe(struct spi_device *spi)
> > if (IS_ERR(mipi->regulator))
> > return PTR_ERR(mipi->regulator);
> > - mipi->backlight = tinydrm_of_find_backlight(dev);
> > + mipi->backlight = drm_of_find_backlight(dev);
> > if (IS_ERR(mipi->backlight))
> > return PTR_ERR(mipi->backlight);
> > diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h
> > index 104dd51..e8fba5b 100644
> > --- a/include/drm/drm_of.h
> > +++ b/include/drm/drm_of.h
> > @@ -29,6 +29,7 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> > int port, int endpoint,
> > struct drm_panel **panel,
> > struct drm_bridge **bridge);
> > +struct backlight_device *drm_of_find_backlight(struct device *dev);
> > #else
> > static inline uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
> > struct device_node *port)
> > diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h
> > index d554ded..e40ef2d 100644
> > --- a/include/drm/tinydrm/tinydrm-helpers.h
> > +++ b/include/drm/tinydrm/tinydrm-helpers.h
> > @@ -46,7 +46,6 @@ void tinydrm_xrgb8888_to_rgb565(u16 *dst, void *vaddr,
> > void tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb,
> > struct drm_clip_rect *clip);
> > -struct backlight_device *tinydrm_of_find_backlight(struct device *dev);
> > int tinydrm_enable_backlight(struct backlight_device *backlight);
> > int tinydrm_disable_backlight(struct backlight_device *backlight);
>
--
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/tinydrm: Move tinydrm_of_find_backlight into drm_of.c
@ 2017-09-28 7:15 ` Daniel Vetter
0 siblings, 0 replies; 14+ messages in thread
From: Daniel Vetter @ 2017-09-28 7:15 UTC (permalink / raw)
To: Noralf Trønnes
Cc: Meghana Madhyastha, daniel, outreachy-kernel, dri-devel
On Wed, Sep 27, 2017 at 05:08:07PM +0200, Noralf Tr�nnes wrote:
>
> Den 27.09.2017 15.54, skrev Meghana Madhyastha:
> > Rename tinydrm_of_find_backlight to drm_of_find_backlight
> > and move it into drm_of.c from tinydrm-helpers.c. This is
> > because other drivers in the drm subsystem might need to call
> > this function. In that case and otherwise, it is better from
> > an organizational point of view to move it into drm_of.c along
> > with the other _of.c functions.
> >
> > Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
> > ---
>
> I suggest you split this patch in 2, one to add the function and one to
> use it in tinydrm.
In general I'd agree to split into 3 phases: 1) add new function 2)
convert drivers 3) remove old one.
But since there's only 1 caller this seems like overkill.
> > drivers/gpu/drm/drm_of.c | 41 ++++++++++++++++++++++++++
> > drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 40 -------------------------
> > drivers/gpu/drm/tinydrm/mi0283qt.c | 3 +-
> > include/drm/drm_of.h | 1 +
> > include/drm/tinydrm/tinydrm-helpers.h | 1 -
> > 5 files changed, 44 insertions(+), 42 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> > index 8dafbdf..d8cded3 100644
> > --- a/drivers/gpu/drm/drm_of.c
> > +++ b/drivers/gpu/drm/drm_of.c
> > @@ -1,6 +1,7 @@
> > #include <linux/component.h>
> > #include <linux/export.h>
> > #include <linux/list.h>
> > +#include <linux/backlight.h>
> > #include <linux/of_graph.h>
> > #include <drm/drmP.h>
> > #include <drm/drm_bridge.h>
> > @@ -260,3 +261,43 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> > return ret;
> > }
> > EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge);
> > +
> > +/**
> > + * drm_of_find_backlight - Find backlight device in device-tree
> > + * @dev: Device
> > + *
> > + * This function looks for a DT node pointed to by a property named 'backlight'
> > + * and uses of_find_backlight_by_node() to get the backlight device.
> > + * Additionally if the brightness property is zero, it is set to
> > + * max_brightness.
>
> Please add a note here about the callers responsibility to call
> put_device() when releasing the resource.
> See the of_find_backlight_by_node() docs.
>
> > + *
> > + * Returns:
> > + * NULL if there's no backlight property.
> > + * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
> > + * is found.
> > + * If the backlight device is found, a pointer to the structure is returned.
> > + */
> > +struct backlight_device *drm_of_find_backlight(struct device *dev)
> > +{
> > + struct backlight_device *backlight;
> > + struct device_node *np;
> > +
> > + np = of_parse_phandle(dev->of_node, "backlight", 0);
> > + if (!np)
> > + return NULL;
> > +
> > + backlight = of_find_backlight_by_node(np);
> > + of_node_put(np);
> > +
> > + if (!backlight)
> > + return ERR_PTR(-EPROBE_DEFER);
> > +
> > + if (!backlight->props.brightness) {
> > + backlight->props.brightness = backlight->props.max_brightness;
> > + DRM_DEBUG_KMS("Backlight brightness set to %d\n",
> > + backlight->props.brightness);
> > + }
> > +
> > + return backlight;
> > +}
> > +EXPORT_SYMBOL(drm_of_find_backlight);
>
> Can you also please add a devm_ version of this function. tinydrm uses
> devres[1] versions of functions for requiring device resources, so it
> would be nice to do this for backlight as well. tinydrm is currently
> broken in this respect, it doesn't put the backlight device.
>
> I had a devm_of_find_backlight() version lying around that I've adjusted
> to give you an idea of what I'm talking about:
>
> static void devm_drm_of_find_backlight_release(void *data)
> {
> ��� put_device(data);
> }
>
> struct backlight_device *devm_drm_of_find_backlight(struct device *dev)
> {
> ��� struct backlight_device *backlight;
> ��� int ret;
>
> ��� backlight = drm_of_find_backlight(dev);
> ��� if (IS_ERR_OR_NULL(backlight))
> ��� ��� return backlight;
>
> ��� ret = devm_add_action(dev, devm_drm_of_find_backlight_release,
> ��� ��� ��� ����� backlight->dev);
> ��� if (ret) {
> ��� ��� put_device(backlight->dev);
> ��� ��� return ERR_PTR(ret);
> ��� }
>
> ��� return backlight;
> }
Good idea for a follow up patch imo.
Another follow up patch series which would be really good is then
converting drivers over to using this (preferrably the devm_ version). A
quick git grep shows there's quite a few.
Either way, the patch that adds these to the core should also add a TODO
entry so we don't forget to complete this conversion.
Thanks, Daniel
>
> [1] https://www.kernel.org/doc/Documentation/driver-model/devres.txt
>
> Noralf.
>
> > diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> > index bd6cce0..cd4c6a5 100644
> > --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> > +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> > @@ -237,46 +237,6 @@ void tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb,
> > EXPORT_SYMBOL(tinydrm_xrgb8888_to_gray8);
> > /**
> > - * tinydrm_of_find_backlight - Find backlight device in device-tree
> > - * @dev: Device
> > - *
> > - * This function looks for a DT node pointed to by a property named 'backlight'
> > - * and uses of_find_backlight_by_node() to get the backlight device.
> > - * Additionally if the brightness property is zero, it is set to
> > - * max_brightness.
> > - *
> > - * Returns:
> > - * NULL if there's no backlight property.
> > - * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
> > - * is found.
> > - * If the backlight device is found, a pointer to the structure is returned.
> > - */
> > -struct backlight_device *tinydrm_of_find_backlight(struct device *dev)
> > -{
> > - struct backlight_device *backlight;
> > - struct device_node *np;
> > -
> > - np = of_parse_phandle(dev->of_node, "backlight", 0);
> > - if (!np)
> > - return NULL;
> > -
> > - backlight = of_find_backlight_by_node(np);
> > - of_node_put(np);
> > -
> > - if (!backlight)
> > - return ERR_PTR(-EPROBE_DEFER);
> > -
> > - if (!backlight->props.brightness) {
> > - backlight->props.brightness = backlight->props.max_brightness;
> > - DRM_DEBUG_KMS("Backlight brightness set to %d\n",
> > - backlight->props.brightness);
> > - }
> > -
> > - return backlight;
> > -}
> > -EXPORT_SYMBOL(tinydrm_of_find_backlight);
> > -
> > -/**
> > * tinydrm_enable_backlight - Enable backlight helper
> > * @backlight: Backlight device
> > *
> > diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
> > index 7e5bb7d..5e3d635 100644
> > --- a/drivers/gpu/drm/tinydrm/mi0283qt.c
> > +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
> > @@ -12,6 +12,7 @@
> > #include <drm/tinydrm/ili9341.h>
> > #include <drm/tinydrm/mipi-dbi.h>
> > #include <drm/tinydrm/tinydrm-helpers.h>
> > +#include <drm/drm_of.h>
> > #include <linux/delay.h>
> > #include <linux/gpio/consumer.h>
> > #include <linux/module.h>
> > @@ -189,7 +190,7 @@ static int mi0283qt_probe(struct spi_device *spi)
> > if (IS_ERR(mipi->regulator))
> > return PTR_ERR(mipi->regulator);
> > - mipi->backlight = tinydrm_of_find_backlight(dev);
> > + mipi->backlight = drm_of_find_backlight(dev);
> > if (IS_ERR(mipi->backlight))
> > return PTR_ERR(mipi->backlight);
> > diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h
> > index 104dd51..e8fba5b 100644
> > --- a/include/drm/drm_of.h
> > +++ b/include/drm/drm_of.h
> > @@ -29,6 +29,7 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> > int port, int endpoint,
> > struct drm_panel **panel,
> > struct drm_bridge **bridge);
> > +struct backlight_device *drm_of_find_backlight(struct device *dev);
> > #else
> > static inline uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
> > struct device_node *port)
> > diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h
> > index d554ded..e40ef2d 100644
> > --- a/include/drm/tinydrm/tinydrm-helpers.h
> > +++ b/include/drm/tinydrm/tinydrm-helpers.h
> > @@ -46,7 +46,6 @@ void tinydrm_xrgb8888_to_rgb565(u16 *dst, void *vaddr,
> > void tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb,
> > struct drm_clip_rect *clip);
> > -struct backlight_device *tinydrm_of_find_backlight(struct device *dev);
> > int tinydrm_enable_backlight(struct backlight_device *backlight);
> > int tinydrm_disable_backlight(struct backlight_device *backlight);
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/tinydrm: Move tinydrm_of_find_backlight into drm_of.c
2017-09-28 7:15 ` Daniel Vetter
@ 2017-09-28 9:27 ` Meghana Madhyastha
-1 siblings, 0 replies; 14+ messages in thread
From: Meghana Madhyastha @ 2017-09-28 9:27 UTC (permalink / raw)
To: daniel, noralf, outreachy-kernel, dri-devel
On Thu, Sep 28, 2017 at 09:15:39AM +0200, Daniel Vetter wrote:
> On Wed, Sep 27, 2017 at 05:08:07PM +0200, Noralf Trønnes wrote:
> >
> > Den 27.09.2017 15.54, skrev Meghana Madhyastha:
> > > Rename tinydrm_of_find_backlight to drm_of_find_backlight
> > > and move it into drm_of.c from tinydrm-helpers.c. This is
> > > because other drivers in the drm subsystem might need to call
> > > this function. In that case and otherwise, it is better from
> > > an organizational point of view to move it into drm_of.c along
> > > with the other _of.c functions.
> > >
> > > Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
> > > ---
> >
> > I suggest you split this patch in 2, one to add the function and one to
> > use it in tinydrm.
>
> In general I'd agree to split into 3 phases: 1) add new function 2)
> convert drivers 3) remove old one.
>
> But since there's only 1 caller this seems like overkill.
>
> > > drivers/gpu/drm/drm_of.c | 41 ++++++++++++++++++++++++++
> > > drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 40 -------------------------
> > > drivers/gpu/drm/tinydrm/mi0283qt.c | 3 +-
> > > include/drm/drm_of.h | 1 +
> > > include/drm/tinydrm/tinydrm-helpers.h | 1 -
> > > 5 files changed, 44 insertions(+), 42 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> > > index 8dafbdf..d8cded3 100644
> > > --- a/drivers/gpu/drm/drm_of.c
> > > +++ b/drivers/gpu/drm/drm_of.c
> > > @@ -1,6 +1,7 @@
> > > #include <linux/component.h>
> > > #include <linux/export.h>
> > > #include <linux/list.h>
> > > +#include <linux/backlight.h>
> > > #include <linux/of_graph.h>
> > > #include <drm/drmP.h>
> > > #include <drm/drm_bridge.h>
> > > @@ -260,3 +261,43 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> > > return ret;
> > > }
> > > EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge);
> > > +
> > > +/**
> > > + * drm_of_find_backlight - Find backlight device in device-tree
> > > + * @dev: Device
> > > + *
> > > + * This function looks for a DT node pointed to by a property named 'backlight'
> > > + * and uses of_find_backlight_by_node() to get the backlight device.
> > > + * Additionally if the brightness property is zero, it is set to
> > > + * max_brightness.
> >
> > Please add a note here about the callers responsibility to call
> > put_device() when releasing the resource.
> > See the of_find_backlight_by_node() docs.
> >
> > > + *
> > > + * Returns:
> > > + * NULL if there's no backlight property.
> > > + * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
> > > + * is found.
> > > + * If the backlight device is found, a pointer to the structure is returned.
> > > + */
> > > +struct backlight_device *drm_of_find_backlight(struct device *dev)
> > > +{
> > > + struct backlight_device *backlight;
> > > + struct device_node *np;
> > > +
> > > + np = of_parse_phandle(dev->of_node, "backlight", 0);
> > > + if (!np)
> > > + return NULL;
> > > +
> > > + backlight = of_find_backlight_by_node(np);
> > > + of_node_put(np);
> > > +
> > > + if (!backlight)
> > > + return ERR_PTR(-EPROBE_DEFER);
> > > +
> > > + if (!backlight->props.brightness) {
> > > + backlight->props.brightness = backlight->props.max_brightness;
> > > + DRM_DEBUG_KMS("Backlight brightness set to %d\n",
> > > + backlight->props.brightness);
> > > + }
> > > +
> > > + return backlight;
> > > +}
> > > +EXPORT_SYMBOL(drm_of_find_backlight);
> >
> > Can you also please add a devm_ version of this function. tinydrm uses
> > devres[1] versions of functions for requiring device resources, so it
> > would be nice to do this for backlight as well. tinydrm is currently
> > broken in this respect, it doesn't put the backlight device.
> >
> > I had a devm_of_find_backlight() version lying around that I've adjusted
> > to give you an idea of what I'm talking about:
> >
> > static void devm_drm_of_find_backlight_release(void *data)
> > {
> > put_device(data);
> > }
> >
> > struct backlight_device *devm_drm_of_find_backlight(struct device *dev)
> > {
> > struct backlight_device *backlight;
> > int ret;
> >
> > backlight = drm_of_find_backlight(dev);
> > if (IS_ERR_OR_NULL(backlight))
> > return backlight;
> >
> > ret = devm_add_action(dev, devm_drm_of_find_backlight_release,
> > backlight->dev);
> > if (ret) {
> > put_device(backlight->dev);
> > return ERR_PTR(ret);
> > }
> >
> > return backlight;
> > }
>
> Good idea for a follow up patch imo.
>
> Another follow up patch series which would be really good is then
> converting drivers over to using this (preferrably the devm_ version). A
> quick git grep shows there's quite a few.
I have a quick question on this. I did a git grep and found that the
other drivers used the function of_find_backlight_by_node defined in
video/backlight. This seems to be doing a different thing (taking
struct device_node* as a parameter instead of struct device*).
Basically, this function finds the backlight device by device-tree node.
How would you suggest converting these cases ?
Thanks and regards,
Meghana
> Either way, the patch that adds these to the core should also add a TODO
> entry so we don't forget to complete this conversion.
>
> Thanks, Daniel
>
> >
> > [1] https://www.kernel.org/doc/Documentation/driver-model/devres.txt
> >
> > Noralf.
> >
> > > diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> > > index bd6cce0..cd4c6a5 100644
> > > --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> > > +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> > > @@ -237,46 +237,6 @@ void tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb,
> > > EXPORT_SYMBOL(tinydrm_xrgb8888_to_gray8);
> > > /**
> > > - * tinydrm_of_find_backlight - Find backlight device in device-tree
> > > - * @dev: Device
> > > - *
> > > - * This function looks for a DT node pointed to by a property named 'backlight'
> > > - * and uses of_find_backlight_by_node() to get the backlight device.
> > > - * Additionally if the brightness property is zero, it is set to
> > > - * max_brightness.
> > > - *
> > > - * Returns:
> > > - * NULL if there's no backlight property.
> > > - * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
> > > - * is found.
> > > - * If the backlight device is found, a pointer to the structure is returned.
> > > - */
> > > -struct backlight_device *tinydrm_of_find_backlight(struct device *dev)
> > > -{
> > > - struct backlight_device *backlight;
> > > - struct device_node *np;
> > > -
> > > - np = of_parse_phandle(dev->of_node, "backlight", 0);
> > > - if (!np)
> > > - return NULL;
> > > -
> > > - backlight = of_find_backlight_by_node(np);
> > > - of_node_put(np);
> > > -
> > > - if (!backlight)
> > > - return ERR_PTR(-EPROBE_DEFER);
> > > -
> > > - if (!backlight->props.brightness) {
> > > - backlight->props.brightness = backlight->props.max_brightness;
> > > - DRM_DEBUG_KMS("Backlight brightness set to %d\n",
> > > - backlight->props.brightness);
> > > - }
> > > -
> > > - return backlight;
> > > -}
> > > -EXPORT_SYMBOL(tinydrm_of_find_backlight);
> > > -
> > > -/**
> > > * tinydrm_enable_backlight - Enable backlight helper
> > > * @backlight: Backlight device
> > > *
> > > diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
> > > index 7e5bb7d..5e3d635 100644
> > > --- a/drivers/gpu/drm/tinydrm/mi0283qt.c
> > > +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
> > > @@ -12,6 +12,7 @@
> > > #include <drm/tinydrm/ili9341.h>
> > > #include <drm/tinydrm/mipi-dbi.h>
> > > #include <drm/tinydrm/tinydrm-helpers.h>
> > > +#include <drm/drm_of.h>
> > > #include <linux/delay.h>
> > > #include <linux/gpio/consumer.h>
> > > #include <linux/module.h>
> > > @@ -189,7 +190,7 @@ static int mi0283qt_probe(struct spi_device *spi)
> > > if (IS_ERR(mipi->regulator))
> > > return PTR_ERR(mipi->regulator);
> > > - mipi->backlight = tinydrm_of_find_backlight(dev);
> > > + mipi->backlight = drm_of_find_backlight(dev);
> > > if (IS_ERR(mipi->backlight))
> > > return PTR_ERR(mipi->backlight);
> > > diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h
> > > index 104dd51..e8fba5b 100644
> > > --- a/include/drm/drm_of.h
> > > +++ b/include/drm/drm_of.h
> > > @@ -29,6 +29,7 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> > > int port, int endpoint,
> > > struct drm_panel **panel,
> > > struct drm_bridge **bridge);
> > > +struct backlight_device *drm_of_find_backlight(struct device *dev);
> > > #else
> > > static inline uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
> > > struct device_node *port)
> > > diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h
> > > index d554ded..e40ef2d 100644
> > > --- a/include/drm/tinydrm/tinydrm-helpers.h
> > > +++ b/include/drm/tinydrm/tinydrm-helpers.h
> > > @@ -46,7 +46,6 @@ void tinydrm_xrgb8888_to_rgb565(u16 *dst, void *vaddr,
> > > void tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb,
> > > struct drm_clip_rect *clip);
> > > -struct backlight_device *tinydrm_of_find_backlight(struct device *dev);
> > > int tinydrm_enable_backlight(struct backlight_device *backlight);
> > > int tinydrm_disable_backlight(struct backlight_device *backlight);
> >
>
> --
> 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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/tinydrm: Move tinydrm_of_find_backlight into drm_of.c
@ 2017-09-28 9:27 ` Meghana Madhyastha
0 siblings, 0 replies; 14+ messages in thread
From: Meghana Madhyastha @ 2017-09-28 9:27 UTC (permalink / raw)
To: daniel, noralf, outreachy-kernel, dri-devel
On Thu, Sep 28, 2017 at 09:15:39AM +0200, Daniel Vetter wrote:
> On Wed, Sep 27, 2017 at 05:08:07PM +0200, Noralf Tr�nnes wrote:
> >
> > Den 27.09.2017 15.54, skrev Meghana Madhyastha:
> > > Rename tinydrm_of_find_backlight to drm_of_find_backlight
> > > and move it into drm_of.c from tinydrm-helpers.c. This is
> > > because other drivers in the drm subsystem might need to call
> > > this function. In that case and otherwise, it is better from
> > > an organizational point of view to move it into drm_of.c along
> > > with the other _of.c functions.
> > >
> > > Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
> > > ---
> >
> > I suggest you split this patch in 2, one to add the function and one to
> > use it in tinydrm.
>
> In general I'd agree to split into 3 phases: 1) add new function 2)
> convert drivers 3) remove old one.
>
> But since there's only 1 caller this seems like overkill.
>
> > > drivers/gpu/drm/drm_of.c | 41 ++++++++++++++++++++++++++
> > > drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 40 -------------------------
> > > drivers/gpu/drm/tinydrm/mi0283qt.c | 3 +-
> > > include/drm/drm_of.h | 1 +
> > > include/drm/tinydrm/tinydrm-helpers.h | 1 -
> > > 5 files changed, 44 insertions(+), 42 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> > > index 8dafbdf..d8cded3 100644
> > > --- a/drivers/gpu/drm/drm_of.c
> > > +++ b/drivers/gpu/drm/drm_of.c
> > > @@ -1,6 +1,7 @@
> > > #include <linux/component.h>
> > > #include <linux/export.h>
> > > #include <linux/list.h>
> > > +#include <linux/backlight.h>
> > > #include <linux/of_graph.h>
> > > #include <drm/drmP.h>
> > > #include <drm/drm_bridge.h>
> > > @@ -260,3 +261,43 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> > > return ret;
> > > }
> > > EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge);
> > > +
> > > +/**
> > > + * drm_of_find_backlight - Find backlight device in device-tree
> > > + * @dev: Device
> > > + *
> > > + * This function looks for a DT node pointed to by a property named 'backlight'
> > > + * and uses of_find_backlight_by_node() to get the backlight device.
> > > + * Additionally if the brightness property is zero, it is set to
> > > + * max_brightness.
> >
> > Please add a note here about the callers responsibility to call
> > put_device() when releasing the resource.
> > See the of_find_backlight_by_node() docs.
> >
> > > + *
> > > + * Returns:
> > > + * NULL if there's no backlight property.
> > > + * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
> > > + * is found.
> > > + * If the backlight device is found, a pointer to the structure is returned.
> > > + */
> > > +struct backlight_device *drm_of_find_backlight(struct device *dev)
> > > +{
> > > + struct backlight_device *backlight;
> > > + struct device_node *np;
> > > +
> > > + np = of_parse_phandle(dev->of_node, "backlight", 0);
> > > + if (!np)
> > > + return NULL;
> > > +
> > > + backlight = of_find_backlight_by_node(np);
> > > + of_node_put(np);
> > > +
> > > + if (!backlight)
> > > + return ERR_PTR(-EPROBE_DEFER);
> > > +
> > > + if (!backlight->props.brightness) {
> > > + backlight->props.brightness = backlight->props.max_brightness;
> > > + DRM_DEBUG_KMS("Backlight brightness set to %d\n",
> > > + backlight->props.brightness);
> > > + }
> > > +
> > > + return backlight;
> > > +}
> > > +EXPORT_SYMBOL(drm_of_find_backlight);
> >
> > Can you also please add a devm_ version of this function. tinydrm uses
> > devres[1] versions of functions for requiring device resources, so it
> > would be nice to do this for backlight as well. tinydrm is currently
> > broken in this respect, it doesn't put the backlight device.
> >
> > I had a devm_of_find_backlight() version lying around that I've adjusted
> > to give you an idea of what I'm talking about:
> >
> > static void devm_drm_of_find_backlight_release(void *data)
> > {
> > ��� put_device(data);
> > }
> >
> > struct backlight_device *devm_drm_of_find_backlight(struct device *dev)
> > {
> > ��� struct backlight_device *backlight;
> > ��� int ret;
> >
> > ��� backlight = drm_of_find_backlight(dev);
> > ��� if (IS_ERR_OR_NULL(backlight))
> > ��� ��� return backlight;
> >
> > ��� ret = devm_add_action(dev, devm_drm_of_find_backlight_release,
> > ��� ��� ��� ����� backlight->dev);
> > ��� if (ret) {
> > ��� ��� put_device(backlight->dev);
> > ��� ��� return ERR_PTR(ret);
> > ��� }
> >
> > ��� return backlight;
> > }
>
> Good idea for a follow up patch imo.
>
> Another follow up patch series which would be really good is then
> converting drivers over to using this (preferrably the devm_ version). A
> quick git grep shows there's quite a few.
I have a quick question on this. I did a git grep and found that the
other drivers used the function of_find_backlight_by_node defined in
video/backlight. This seems to be doing a different thing (taking
struct device_node* as a parameter instead of struct device*).
Basically, this function finds the backlight device by device-tree node.
How would you suggest converting these cases ?
Thanks and regards,
Meghana
> Either way, the patch that adds these to the core should also add a TODO
> entry so we don't forget to complete this conversion.
>
> Thanks, Daniel
>
> >
> > [1] https://www.kernel.org/doc/Documentation/driver-model/devres.txt
> >
> > Noralf.
> >
> > > diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> > > index bd6cce0..cd4c6a5 100644
> > > --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> > > +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> > > @@ -237,46 +237,6 @@ void tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb,
> > > EXPORT_SYMBOL(tinydrm_xrgb8888_to_gray8);
> > > /**
> > > - * tinydrm_of_find_backlight - Find backlight device in device-tree
> > > - * @dev: Device
> > > - *
> > > - * This function looks for a DT node pointed to by a property named 'backlight'
> > > - * and uses of_find_backlight_by_node() to get the backlight device.
> > > - * Additionally if the brightness property is zero, it is set to
> > > - * max_brightness.
> > > - *
> > > - * Returns:
> > > - * NULL if there's no backlight property.
> > > - * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
> > > - * is found.
> > > - * If the backlight device is found, a pointer to the structure is returned.
> > > - */
> > > -struct backlight_device *tinydrm_of_find_backlight(struct device *dev)
> > > -{
> > > - struct backlight_device *backlight;
> > > - struct device_node *np;
> > > -
> > > - np = of_parse_phandle(dev->of_node, "backlight", 0);
> > > - if (!np)
> > > - return NULL;
> > > -
> > > - backlight = of_find_backlight_by_node(np);
> > > - of_node_put(np);
> > > -
> > > - if (!backlight)
> > > - return ERR_PTR(-EPROBE_DEFER);
> > > -
> > > - if (!backlight->props.brightness) {
> > > - backlight->props.brightness = backlight->props.max_brightness;
> > > - DRM_DEBUG_KMS("Backlight brightness set to %d\n",
> > > - backlight->props.brightness);
> > > - }
> > > -
> > > - return backlight;
> > > -}
> > > -EXPORT_SYMBOL(tinydrm_of_find_backlight);
> > > -
> > > -/**
> > > * tinydrm_enable_backlight - Enable backlight helper
> > > * @backlight: Backlight device
> > > *
> > > diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
> > > index 7e5bb7d..5e3d635 100644
> > > --- a/drivers/gpu/drm/tinydrm/mi0283qt.c
> > > +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
> > > @@ -12,6 +12,7 @@
> > > #include <drm/tinydrm/ili9341.h>
> > > #include <drm/tinydrm/mipi-dbi.h>
> > > #include <drm/tinydrm/tinydrm-helpers.h>
> > > +#include <drm/drm_of.h>
> > > #include <linux/delay.h>
> > > #include <linux/gpio/consumer.h>
> > > #include <linux/module.h>
> > > @@ -189,7 +190,7 @@ static int mi0283qt_probe(struct spi_device *spi)
> > > if (IS_ERR(mipi->regulator))
> > > return PTR_ERR(mipi->regulator);
> > > - mipi->backlight = tinydrm_of_find_backlight(dev);
> > > + mipi->backlight = drm_of_find_backlight(dev);
> > > if (IS_ERR(mipi->backlight))
> > > return PTR_ERR(mipi->backlight);
> > > diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h
> > > index 104dd51..e8fba5b 100644
> > > --- a/include/drm/drm_of.h
> > > +++ b/include/drm/drm_of.h
> > > @@ -29,6 +29,7 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> > > int port, int endpoint,
> > > struct drm_panel **panel,
> > > struct drm_bridge **bridge);
> > > +struct backlight_device *drm_of_find_backlight(struct device *dev);
> > > #else
> > > static inline uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
> > > struct device_node *port)
> > > diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h
> > > index d554ded..e40ef2d 100644
> > > --- a/include/drm/tinydrm/tinydrm-helpers.h
> > > +++ b/include/drm/tinydrm/tinydrm-helpers.h
> > > @@ -46,7 +46,6 @@ void tinydrm_xrgb8888_to_rgb565(u16 *dst, void *vaddr,
> > > void tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb,
> > > struct drm_clip_rect *clip);
> > > -struct backlight_device *tinydrm_of_find_backlight(struct device *dev);
> > > int tinydrm_enable_backlight(struct backlight_device *backlight);
> > > int tinydrm_disable_backlight(struct backlight_device *backlight);
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/tinydrm: Move tinydrm_of_find_backlight into drm_of.c
2017-09-28 9:27 ` Meghana Madhyastha
@ 2017-09-28 9:38 ` Daniel Vetter
-1 siblings, 0 replies; 14+ messages in thread
From: Daniel Vetter @ 2017-09-28 9:38 UTC (permalink / raw)
To: Meghana Madhyastha; +Cc: outreachy-kernel, dri-devel
On Thu, Sep 28, 2017 at 11:27 AM, Meghana Madhyastha
<meghana.madhyastha@gmail.com> wrote:
> On Thu, Sep 28, 2017 at 09:15:39AM +0200, Daniel Vetter wrote:
>> On Wed, Sep 27, 2017 at 05:08:07PM +0200, Noralf Trønnes wrote:
>> >
>> > Den 27.09.2017 15.54, skrev Meghana Madhyastha:
>> > > Rename tinydrm_of_find_backlight to drm_of_find_backlight
>> > > and move it into drm_of.c from tinydrm-helpers.c. This is
>> > > because other drivers in the drm subsystem might need to call
>> > > this function. In that case and otherwise, it is better from
>> > > an organizational point of view to move it into drm_of.c along
>> > > with the other _of.c functions.
>> > >
>> > > Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
>> > > ---
>> >
>> > I suggest you split this patch in 2, one to add the function and one to
>> > use it in tinydrm.
>>
>> In general I'd agree to split into 3 phases: 1) add new function 2)
>> convert drivers 3) remove old one.
>>
>> But since there's only 1 caller this seems like overkill.
>>
>> > > drivers/gpu/drm/drm_of.c | 41 ++++++++++++++++++++++++++
>> > > drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 40 -------------------------
>> > > drivers/gpu/drm/tinydrm/mi0283qt.c | 3 +-
>> > > include/drm/drm_of.h | 1 +
>> > > include/drm/tinydrm/tinydrm-helpers.h | 1 -
>> > > 5 files changed, 44 insertions(+), 42 deletions(-)
>> > >
>> > > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
>> > > index 8dafbdf..d8cded3 100644
>> > > --- a/drivers/gpu/drm/drm_of.c
>> > > +++ b/drivers/gpu/drm/drm_of.c
>> > > @@ -1,6 +1,7 @@
>> > > #include <linux/component.h>
>> > > #include <linux/export.h>
>> > > #include <linux/list.h>
>> > > +#include <linux/backlight.h>
>> > > #include <linux/of_graph.h>
>> > > #include <drm/drmP.h>
>> > > #include <drm/drm_bridge.h>
>> > > @@ -260,3 +261,43 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
>> > > return ret;
>> > > }
>> > > EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge);
>> > > +
>> > > +/**
>> > > + * drm_of_find_backlight - Find backlight device in device-tree
>> > > + * @dev: Device
>> > > + *
>> > > + * This function looks for a DT node pointed to by a property named 'backlight'
>> > > + * and uses of_find_backlight_by_node() to get the backlight device.
>> > > + * Additionally if the brightness property is zero, it is set to
>> > > + * max_brightness.
>> >
>> > Please add a note here about the callers responsibility to call
>> > put_device() when releasing the resource.
>> > See the of_find_backlight_by_node() docs.
>> >
>> > > + *
>> > > + * Returns:
>> > > + * NULL if there's no backlight property.
>> > > + * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
>> > > + * is found.
>> > > + * If the backlight device is found, a pointer to the structure is returned.
>> > > + */
>> > > +struct backlight_device *drm_of_find_backlight(struct device *dev)
>> > > +{
>> > > + struct backlight_device *backlight;
>> > > + struct device_node *np;
>> > > +
>> > > + np = of_parse_phandle(dev->of_node, "backlight", 0);
>> > > + if (!np)
>> > > + return NULL;
>> > > +
>> > > + backlight = of_find_backlight_by_node(np);
>> > > + of_node_put(np);
>> > > +
>> > > + if (!backlight)
>> > > + return ERR_PTR(-EPROBE_DEFER);
>> > > +
>> > > + if (!backlight->props.brightness) {
>> > > + backlight->props.brightness = backlight->props.max_brightness;
>> > > + DRM_DEBUG_KMS("Backlight brightness set to %d\n",
>> > > + backlight->props.brightness);
>> > > + }
>> > > +
>> > > + return backlight;
>> > > +}
>> > > +EXPORT_SYMBOL(drm_of_find_backlight);
>> >
>> > Can you also please add a devm_ version of this function. tinydrm uses
>> > devres[1] versions of functions for requiring device resources, so it
>> > would be nice to do this for backlight as well. tinydrm is currently
>> > broken in this respect, it doesn't put the backlight device.
>> >
>> > I had a devm_of_find_backlight() version lying around that I've adjusted
>> > to give you an idea of what I'm talking about:
>> >
>> > static void devm_drm_of_find_backlight_release(void *data)
>> > {
>> > put_device(data);
>> > }
>> >
>> > struct backlight_device *devm_drm_of_find_backlight(struct device *dev)
>> > {
>> > struct backlight_device *backlight;
>> > int ret;
>> >
>> > backlight = drm_of_find_backlight(dev);
>> > if (IS_ERR_OR_NULL(backlight))
>> > return backlight;
>> >
>> > ret = devm_add_action(dev, devm_drm_of_find_backlight_release,
>> > backlight->dev);
>> > if (ret) {
>> > put_device(backlight->dev);
>> > return ERR_PTR(ret);
>> > }
>> >
>> > return backlight;
>> > }
>>
>> Good idea for a follow up patch imo.
>>
>> Another follow up patch series which would be really good is then
>> converting drivers over to using this (preferrably the devm_ version). A
>> quick git grep shows there's quite a few.
>
> I have a quick question on this. I did a git grep and found that the
> other drivers used the function of_find_backlight_by_node defined in
> video/backlight. This seems to be doing a different thing (taking
> struct device_node* as a parameter instead of struct device*).
> Basically, this function finds the backlight device by device-tree node.
> How would you suggest converting these cases ?
The function you're moving also uses of_find_backglight_by_node, but
wraps it up to be more convenient for drm drivers. I didn't carefully
audit all of them, but I'd assume a bunch of these other callers might
be able to use the same convenience helper. But I might be wrong, and
we need a drm_panel specific helper. Just something I've noticed and
figured might be worth to check.
-Daniel
>
> Thanks and regards,
> Meghana
>
>> Either way, the patch that adds these to the core should also add a TODO
>> entry so we don't forget to complete this conversion.
>>
>> Thanks, Daniel
>>
>> >
>> > [1] https://www.kernel.org/doc/Documentation/driver-model/devres.txt
>> >
>> > Noralf.
>> >
>> > > diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>> > > index bd6cce0..cd4c6a5 100644
>> > > --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>> > > +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>> > > @@ -237,46 +237,6 @@ void tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb,
>> > > EXPORT_SYMBOL(tinydrm_xrgb8888_to_gray8);
>> > > /**
>> > > - * tinydrm_of_find_backlight - Find backlight device in device-tree
>> > > - * @dev: Device
>> > > - *
>> > > - * This function looks for a DT node pointed to by a property named 'backlight'
>> > > - * and uses of_find_backlight_by_node() to get the backlight device.
>> > > - * Additionally if the brightness property is zero, it is set to
>> > > - * max_brightness.
>> > > - *
>> > > - * Returns:
>> > > - * NULL if there's no backlight property.
>> > > - * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
>> > > - * is found.
>> > > - * If the backlight device is found, a pointer to the structure is returned.
>> > > - */
>> > > -struct backlight_device *tinydrm_of_find_backlight(struct device *dev)
>> > > -{
>> > > - struct backlight_device *backlight;
>> > > - struct device_node *np;
>> > > -
>> > > - np = of_parse_phandle(dev->of_node, "backlight", 0);
>> > > - if (!np)
>> > > - return NULL;
>> > > -
>> > > - backlight = of_find_backlight_by_node(np);
>> > > - of_node_put(np);
>> > > -
>> > > - if (!backlight)
>> > > - return ERR_PTR(-EPROBE_DEFER);
>> > > -
>> > > - if (!backlight->props.brightness) {
>> > > - backlight->props.brightness = backlight->props.max_brightness;
>> > > - DRM_DEBUG_KMS("Backlight brightness set to %d\n",
>> > > - backlight->props.brightness);
>> > > - }
>> > > -
>> > > - return backlight;
>> > > -}
>> > > -EXPORT_SYMBOL(tinydrm_of_find_backlight);
>> > > -
>> > > -/**
>> > > * tinydrm_enable_backlight - Enable backlight helper
>> > > * @backlight: Backlight device
>> > > *
>> > > diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
>> > > index 7e5bb7d..5e3d635 100644
>> > > --- a/drivers/gpu/drm/tinydrm/mi0283qt.c
>> > > +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
>> > > @@ -12,6 +12,7 @@
>> > > #include <drm/tinydrm/ili9341.h>
>> > > #include <drm/tinydrm/mipi-dbi.h>
>> > > #include <drm/tinydrm/tinydrm-helpers.h>
>> > > +#include <drm/drm_of.h>
>> > > #include <linux/delay.h>
>> > > #include <linux/gpio/consumer.h>
>> > > #include <linux/module.h>
>> > > @@ -189,7 +190,7 @@ static int mi0283qt_probe(struct spi_device *spi)
>> > > if (IS_ERR(mipi->regulator))
>> > > return PTR_ERR(mipi->regulator);
>> > > - mipi->backlight = tinydrm_of_find_backlight(dev);
>> > > + mipi->backlight = drm_of_find_backlight(dev);
>> > > if (IS_ERR(mipi->backlight))
>> > > return PTR_ERR(mipi->backlight);
>> > > diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h
>> > > index 104dd51..e8fba5b 100644
>> > > --- a/include/drm/drm_of.h
>> > > +++ b/include/drm/drm_of.h
>> > > @@ -29,6 +29,7 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
>> > > int port, int endpoint,
>> > > struct drm_panel **panel,
>> > > struct drm_bridge **bridge);
>> > > +struct backlight_device *drm_of_find_backlight(struct device *dev);
>> > > #else
>> > > static inline uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
>> > > struct device_node *port)
>> > > diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h
>> > > index d554ded..e40ef2d 100644
>> > > --- a/include/drm/tinydrm/tinydrm-helpers.h
>> > > +++ b/include/drm/tinydrm/tinydrm-helpers.h
>> > > @@ -46,7 +46,6 @@ void tinydrm_xrgb8888_to_rgb565(u16 *dst, void *vaddr,
>> > > void tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb,
>> > > struct drm_clip_rect *clip);
>> > > -struct backlight_device *tinydrm_of_find_backlight(struct device *dev);
>> > > int tinydrm_enable_backlight(struct backlight_device *backlight);
>> > > int tinydrm_disable_backlight(struct backlight_device *backlight);
>> >
>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/tinydrm: Move tinydrm_of_find_backlight into drm_of.c
@ 2017-09-28 9:38 ` Daniel Vetter
0 siblings, 0 replies; 14+ messages in thread
From: Daniel Vetter @ 2017-09-28 9:38 UTC (permalink / raw)
To: Meghana Madhyastha; +Cc: Noralf Trønnes, outreachy-kernel, dri-devel
On Thu, Sep 28, 2017 at 11:27 AM, Meghana Madhyastha
<meghana.madhyastha@gmail.com> wrote:
> On Thu, Sep 28, 2017 at 09:15:39AM +0200, Daniel Vetter wrote:
>> On Wed, Sep 27, 2017 at 05:08:07PM +0200, Noralf Trønnes wrote:
>> >
>> > Den 27.09.2017 15.54, skrev Meghana Madhyastha:
>> > > Rename tinydrm_of_find_backlight to drm_of_find_backlight
>> > > and move it into drm_of.c from tinydrm-helpers.c. This is
>> > > because other drivers in the drm subsystem might need to call
>> > > this function. In that case and otherwise, it is better from
>> > > an organizational point of view to move it into drm_of.c along
>> > > with the other _of.c functions.
>> > >
>> > > Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
>> > > ---
>> >
>> > I suggest you split this patch in 2, one to add the function and one to
>> > use it in tinydrm.
>>
>> In general I'd agree to split into 3 phases: 1) add new function 2)
>> convert drivers 3) remove old one.
>>
>> But since there's only 1 caller this seems like overkill.
>>
>> > > drivers/gpu/drm/drm_of.c | 41 ++++++++++++++++++++++++++
>> > > drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 40 -------------------------
>> > > drivers/gpu/drm/tinydrm/mi0283qt.c | 3 +-
>> > > include/drm/drm_of.h | 1 +
>> > > include/drm/tinydrm/tinydrm-helpers.h | 1 -
>> > > 5 files changed, 44 insertions(+), 42 deletions(-)
>> > >
>> > > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
>> > > index 8dafbdf..d8cded3 100644
>> > > --- a/drivers/gpu/drm/drm_of.c
>> > > +++ b/drivers/gpu/drm/drm_of.c
>> > > @@ -1,6 +1,7 @@
>> > > #include <linux/component.h>
>> > > #include <linux/export.h>
>> > > #include <linux/list.h>
>> > > +#include <linux/backlight.h>
>> > > #include <linux/of_graph.h>
>> > > #include <drm/drmP.h>
>> > > #include <drm/drm_bridge.h>
>> > > @@ -260,3 +261,43 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
>> > > return ret;
>> > > }
>> > > EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge);
>> > > +
>> > > +/**
>> > > + * drm_of_find_backlight - Find backlight device in device-tree
>> > > + * @dev: Device
>> > > + *
>> > > + * This function looks for a DT node pointed to by a property named 'backlight'
>> > > + * and uses of_find_backlight_by_node() to get the backlight device.
>> > > + * Additionally if the brightness property is zero, it is set to
>> > > + * max_brightness.
>> >
>> > Please add a note here about the callers responsibility to call
>> > put_device() when releasing the resource.
>> > See the of_find_backlight_by_node() docs.
>> >
>> > > + *
>> > > + * Returns:
>> > > + * NULL if there's no backlight property.
>> > > + * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
>> > > + * is found.
>> > > + * If the backlight device is found, a pointer to the structure is returned.
>> > > + */
>> > > +struct backlight_device *drm_of_find_backlight(struct device *dev)
>> > > +{
>> > > + struct backlight_device *backlight;
>> > > + struct device_node *np;
>> > > +
>> > > + np = of_parse_phandle(dev->of_node, "backlight", 0);
>> > > + if (!np)
>> > > + return NULL;
>> > > +
>> > > + backlight = of_find_backlight_by_node(np);
>> > > + of_node_put(np);
>> > > +
>> > > + if (!backlight)
>> > > + return ERR_PTR(-EPROBE_DEFER);
>> > > +
>> > > + if (!backlight->props.brightness) {
>> > > + backlight->props.brightness = backlight->props.max_brightness;
>> > > + DRM_DEBUG_KMS("Backlight brightness set to %d\n",
>> > > + backlight->props.brightness);
>> > > + }
>> > > +
>> > > + return backlight;
>> > > +}
>> > > +EXPORT_SYMBOL(drm_of_find_backlight);
>> >
>> > Can you also please add a devm_ version of this function. tinydrm uses
>> > devres[1] versions of functions for requiring device resources, so it
>> > would be nice to do this for backlight as well. tinydrm is currently
>> > broken in this respect, it doesn't put the backlight device.
>> >
>> > I had a devm_of_find_backlight() version lying around that I've adjusted
>> > to give you an idea of what I'm talking about:
>> >
>> > static void devm_drm_of_find_backlight_release(void *data)
>> > {
>> > put_device(data);
>> > }
>> >
>> > struct backlight_device *devm_drm_of_find_backlight(struct device *dev)
>> > {
>> > struct backlight_device *backlight;
>> > int ret;
>> >
>> > backlight = drm_of_find_backlight(dev);
>> > if (IS_ERR_OR_NULL(backlight))
>> > return backlight;
>> >
>> > ret = devm_add_action(dev, devm_drm_of_find_backlight_release,
>> > backlight->dev);
>> > if (ret) {
>> > put_device(backlight->dev);
>> > return ERR_PTR(ret);
>> > }
>> >
>> > return backlight;
>> > }
>>
>> Good idea for a follow up patch imo.
>>
>> Another follow up patch series which would be really good is then
>> converting drivers over to using this (preferrably the devm_ version). A
>> quick git grep shows there's quite a few.
>
> I have a quick question on this. I did a git grep and found that the
> other drivers used the function of_find_backlight_by_node defined in
> video/backlight. This seems to be doing a different thing (taking
> struct device_node* as a parameter instead of struct device*).
> Basically, this function finds the backlight device by device-tree node.
> How would you suggest converting these cases ?
The function you're moving also uses of_find_backglight_by_node, but
wraps it up to be more convenient for drm drivers. I didn't carefully
audit all of them, but I'd assume a bunch of these other callers might
be able to use the same convenience helper. But I might be wrong, and
we need a drm_panel specific helper. Just something I've noticed and
figured might be worth to check.
-Daniel
>
> Thanks and regards,
> Meghana
>
>> Either way, the patch that adds these to the core should also add a TODO
>> entry so we don't forget to complete this conversion.
>>
>> Thanks, Daniel
>>
>> >
>> > [1] https://www.kernel.org/doc/Documentation/driver-model/devres.txt
>> >
>> > Noralf.
>> >
>> > > diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>> > > index bd6cce0..cd4c6a5 100644
>> > > --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>> > > +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>> > > @@ -237,46 +237,6 @@ void tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb,
>> > > EXPORT_SYMBOL(tinydrm_xrgb8888_to_gray8);
>> > > /**
>> > > - * tinydrm_of_find_backlight - Find backlight device in device-tree
>> > > - * @dev: Device
>> > > - *
>> > > - * This function looks for a DT node pointed to by a property named 'backlight'
>> > > - * and uses of_find_backlight_by_node() to get the backlight device.
>> > > - * Additionally if the brightness property is zero, it is set to
>> > > - * max_brightness.
>> > > - *
>> > > - * Returns:
>> > > - * NULL if there's no backlight property.
>> > > - * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
>> > > - * is found.
>> > > - * If the backlight device is found, a pointer to the structure is returned.
>> > > - */
>> > > -struct backlight_device *tinydrm_of_find_backlight(struct device *dev)
>> > > -{
>> > > - struct backlight_device *backlight;
>> > > - struct device_node *np;
>> > > -
>> > > - np = of_parse_phandle(dev->of_node, "backlight", 0);
>> > > - if (!np)
>> > > - return NULL;
>> > > -
>> > > - backlight = of_find_backlight_by_node(np);
>> > > - of_node_put(np);
>> > > -
>> > > - if (!backlight)
>> > > - return ERR_PTR(-EPROBE_DEFER);
>> > > -
>> > > - if (!backlight->props.brightness) {
>> > > - backlight->props.brightness = backlight->props.max_brightness;
>> > > - DRM_DEBUG_KMS("Backlight brightness set to %d\n",
>> > > - backlight->props.brightness);
>> > > - }
>> > > -
>> > > - return backlight;
>> > > -}
>> > > -EXPORT_SYMBOL(tinydrm_of_find_backlight);
>> > > -
>> > > -/**
>> > > * tinydrm_enable_backlight - Enable backlight helper
>> > > * @backlight: Backlight device
>> > > *
>> > > diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
>> > > index 7e5bb7d..5e3d635 100644
>> > > --- a/drivers/gpu/drm/tinydrm/mi0283qt.c
>> > > +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
>> > > @@ -12,6 +12,7 @@
>> > > #include <drm/tinydrm/ili9341.h>
>> > > #include <drm/tinydrm/mipi-dbi.h>
>> > > #include <drm/tinydrm/tinydrm-helpers.h>
>> > > +#include <drm/drm_of.h>
>> > > #include <linux/delay.h>
>> > > #include <linux/gpio/consumer.h>
>> > > #include <linux/module.h>
>> > > @@ -189,7 +190,7 @@ static int mi0283qt_probe(struct spi_device *spi)
>> > > if (IS_ERR(mipi->regulator))
>> > > return PTR_ERR(mipi->regulator);
>> > > - mipi->backlight = tinydrm_of_find_backlight(dev);
>> > > + mipi->backlight = drm_of_find_backlight(dev);
>> > > if (IS_ERR(mipi->backlight))
>> > > return PTR_ERR(mipi->backlight);
>> > > diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h
>> > > index 104dd51..e8fba5b 100644
>> > > --- a/include/drm/drm_of.h
>> > > +++ b/include/drm/drm_of.h
>> > > @@ -29,6 +29,7 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
>> > > int port, int endpoint,
>> > > struct drm_panel **panel,
>> > > struct drm_bridge **bridge);
>> > > +struct backlight_device *drm_of_find_backlight(struct device *dev);
>> > > #else
>> > > static inline uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
>> > > struct device_node *port)
>> > > diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h
>> > > index d554ded..e40ef2d 100644
>> > > --- a/include/drm/tinydrm/tinydrm-helpers.h
>> > > +++ b/include/drm/tinydrm/tinydrm-helpers.h
>> > > @@ -46,7 +46,6 @@ void tinydrm_xrgb8888_to_rgb565(u16 *dst, void *vaddr,
>> > > void tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb,
>> > > struct drm_clip_rect *clip);
>> > > -struct backlight_device *tinydrm_of_find_backlight(struct device *dev);
>> > > int tinydrm_enable_backlight(struct backlight_device *backlight);
>> > > int tinydrm_disable_backlight(struct backlight_device *backlight);
>> >
>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/tinydrm: Move tinydrm_of_find_backlight into drm_of.c
2017-09-28 9:38 ` Daniel Vetter
@ 2017-09-29 3:47 ` Meghana Madhyastha
-1 siblings, 0 replies; 14+ messages in thread
From: Meghana Madhyastha @ 2017-09-29 3:47 UTC (permalink / raw)
To: daniel, noralf, outreachy-kernel, dri-devel
On Thu, Sep 28, 2017 at 11:38:36AM +0200, Daniel Vetter wrote:
> On Thu, Sep 28, 2017 at 11:27 AM, Meghana Madhyastha
> <meghana.madhyastha@gmail.com> wrote:
> > On Thu, Sep 28, 2017 at 09:15:39AM +0200, Daniel Vetter wrote:
> >> On Wed, Sep 27, 2017 at 05:08:07PM +0200, Noralf Trønnes wrote:
> >> >
> >> > Den 27.09.2017 15.54, skrev Meghana Madhyastha:
> >> > > Rename tinydrm_of_find_backlight to drm_of_find_backlight
> >> > > and move it into drm_of.c from tinydrm-helpers.c. This is
> >> > > because other drivers in the drm subsystem might need to call
> >> > > this function. In that case and otherwise, it is better from
> >> > > an organizational point of view to move it into drm_of.c along
> >> > > with the other _of.c functions.
> >> > >
> >> > > Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
> >> > > ---
> >> >
> >> > I suggest you split this patch in 2, one to add the function and one to
> >> > use it in tinydrm.
> >>
> >> In general I'd agree to split into 3 phases: 1) add new function 2)
> >> convert drivers 3) remove old one.
> >>
> >> But since there's only 1 caller this seems like overkill.
> >>
> >> > > drivers/gpu/drm/drm_of.c | 41 ++++++++++++++++++++++++++
> >> > > drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 40 -------------------------
> >> > > drivers/gpu/drm/tinydrm/mi0283qt.c | 3 +-
> >> > > include/drm/drm_of.h | 1 +
> >> > > include/drm/tinydrm/tinydrm-helpers.h | 1 -
> >> > > 5 files changed, 44 insertions(+), 42 deletions(-)
> >> > >
> >> > > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> >> > > index 8dafbdf..d8cded3 100644
> >> > > --- a/drivers/gpu/drm/drm_of.c
> >> > > +++ b/drivers/gpu/drm/drm_of.c
> >> > > @@ -1,6 +1,7 @@
> >> > > #include <linux/component.h>
> >> > > #include <linux/export.h>
> >> > > #include <linux/list.h>
> >> > > +#include <linux/backlight.h>
> >> > > #include <linux/of_graph.h>
> >> > > #include <drm/drmP.h>
> >> > > #include <drm/drm_bridge.h>
> >> > > @@ -260,3 +261,43 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> >> > > return ret;
> >> > > }
> >> > > EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge);
> >> > > +
> >> > > +/**
> >> > > + * drm_of_find_backlight - Find backlight device in device-tree
> >> > > + * @dev: Device
> >> > > + *
> >> > > + * This function looks for a DT node pointed to by a property named 'backlight'
> >> > > + * and uses of_find_backlight_by_node() to get the backlight device.
> >> > > + * Additionally if the brightness property is zero, it is set to
> >> > > + * max_brightness.
> >> >
> >> > Please add a note here about the callers responsibility to call
> >> > put_device() when releasing the resource.
> >> > See the of_find_backlight_by_node() docs.
> >> >
> >> > > + *
> >> > > + * Returns:
> >> > > + * NULL if there's no backlight property.
> >> > > + * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
> >> > > + * is found.
> >> > > + * If the backlight device is found, a pointer to the structure is returned.
> >> > > + */
> >> > > +struct backlight_device *drm_of_find_backlight(struct device *dev)
> >> > > +{
> >> > > + struct backlight_device *backlight;
> >> > > + struct device_node *np;
> >> > > +
> >> > > + np = of_parse_phandle(dev->of_node, "backlight", 0);
> >> > > + if (!np)
> >> > > + return NULL;
> >> > > +
> >> > > + backlight = of_find_backlight_by_node(np);
> >> > > + of_node_put(np);
> >> > > +
> >> > > + if (!backlight)
> >> > > + return ERR_PTR(-EPROBE_DEFER);
> >> > > +
> >> > > + if (!backlight->props.brightness) {
> >> > > + backlight->props.brightness = backlight->props.max_brightness;
> >> > > + DRM_DEBUG_KMS("Backlight brightness set to %d\n",
> >> > > + backlight->props.brightness);
> >> > > + }
> >> > > +
> >> > > + return backlight;
> >> > > +}
> >> > > +EXPORT_SYMBOL(drm_of_find_backlight);
> >> >
> >> > Can you also please add a devm_ version of this function. tinydrm uses
> >> > devres[1] versions of functions for requiring device resources, so it
> >> > would be nice to do this for backlight as well. tinydrm is currently
> >> > broken in this respect, it doesn't put the backlight device.
> >> >
> >> > I had a devm_of_find_backlight() version lying around that I've adjusted
> >> > to give you an idea of what I'm talking about:
> >> >
> >> > static void devm_drm_of_find_backlight_release(void *data)
> >> > {
> >> > put_device(data);
> >> > }
> >> >
> >> > struct backlight_device *devm_drm_of_find_backlight(struct device *dev)
> >> > {
> >> > struct backlight_device *backlight;
> >> > int ret;
> >> >
> >> > backlight = drm_of_find_backlight(dev);
> >> > if (IS_ERR_OR_NULL(backlight))
> >> > return backlight;
> >> >
> >> > ret = devm_add_action(dev, devm_drm_of_find_backlight_release,
> >> > backlight->dev);
> >> > if (ret) {
> >> > put_device(backlight->dev);
> >> > return ERR_PTR(ret);
> >> > }
> >> >
> >> > return backlight;
> >> > }
> >>
> >> Good idea for a follow up patch imo.
> >>
> >> Another follow up patch series which would be really good is then
> >> converting drivers over to using this (preferrably the devm_ version). A
> >> quick git grep shows there's quite a few.
> >
> > I have a quick question on this. I did a git grep and found that the
> > other drivers used the function of_find_backlight_by_node defined in
> > video/backlight. This seems to be doing a different thing (taking
> > struct device_node* as a parameter instead of struct device*).
> > Basically, this function finds the backlight device by device-tree node.
> > How would you suggest converting these cases ?
>
> The function you're moving also uses of_find_backglight_by_node, but
> wraps it up to be more convenient for drm drivers. I didn't carefully
> audit all of them, but I'd assume a bunch of these other callers might
> be able to use the same convenience helper. But I might be wrong, and
> we need a drm_panel specific helper. Just something I've noticed and
> figured might be worth to check.
> -Daniel
Yes you are right. Sorry, I hadn't carefully looked into all of them,
but now having checked the calls, it looks like they can all be replaced by
of_find_backlight. The only thing extra of_find_backlight is doing is
setting the brightness to be the maximum brightness which makes sense here.
I can send in a patch to replace these.
> > Thanks and regards,
> > Meghana
> >
> >> Either way, the patch that adds these to the core should also add a TODO
> >> entry so we don't forget to complete this conversion.
> >>
> >> Thanks, Daniel
> >>
> >> >
> >> > [1] https://www.kernel.org/doc/Documentation/driver-model/devres.txt
> >> >
> >> > Noralf.
> >> >
> >> > > diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> >> > > index bd6cce0..cd4c6a5 100644
> >> > > --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> >> > > +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> >> > > @@ -237,46 +237,6 @@ void tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb,
> >> > > EXPORT_SYMBOL(tinydrm_xrgb8888_to_gray8);
> >> > > /**
> >> > > - * tinydrm_of_find_backlight - Find backlight device in device-tree
> >> > > - * @dev: Device
> >> > > - *
> >> > > - * This function looks for a DT node pointed to by a property named 'backlight'
> >> > > - * and uses of_find_backlight_by_node() to get the backlight device.
> >> > > - * Additionally if the brightness property is zero, it is set to
> >> > > - * max_brightness.
> >> > > - *
> >> > > - * Returns:
> >> > > - * NULL if there's no backlight property.
> >> > > - * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
> >> > > - * is found.
> >> > > - * If the backlight device is found, a pointer to the structure is returned.
> >> > > - */
> >> > > -struct backlight_device *tinydrm_of_find_backlight(struct device *dev)
> >> > > -{
> >> > > - struct backlight_device *backlight;
> >> > > - struct device_node *np;
> >> > > -
> >> > > - np = of_parse_phandle(dev->of_node, "backlight", 0);
> >> > > - if (!np)
> >> > > - return NULL;
> >> > > -
> >> > > - backlight = of_find_backlight_by_node(np);
> >> > > - of_node_put(np);
> >> > > -
> >> > > - if (!backlight)
> >> > > - return ERR_PTR(-EPROBE_DEFER);
> >> > > -
> >> > > - if (!backlight->props.brightness) {
> >> > > - backlight->props.brightness = backlight->props.max_brightness;
> >> > > - DRM_DEBUG_KMS("Backlight brightness set to %d\n",
> >> > > - backlight->props.brightness);
> >> > > - }
> >> > > -
> >> > > - return backlight;
> >> > > -}
> >> > > -EXPORT_SYMBOL(tinydrm_of_find_backlight);
> >> > > -
> >> > > -/**
> >> > > * tinydrm_enable_backlight - Enable backlight helper
> >> > > * @backlight: Backlight device
> >> > > *
> >> > > diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
> >> > > index 7e5bb7d..5e3d635 100644
> >> > > --- a/drivers/gpu/drm/tinydrm/mi0283qt.c
> >> > > +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
> >> > > @@ -12,6 +12,7 @@
> >> > > #include <drm/tinydrm/ili9341.h>
> >> > > #include <drm/tinydrm/mipi-dbi.h>
> >> > > #include <drm/tinydrm/tinydrm-helpers.h>
> >> > > +#include <drm/drm_of.h>
> >> > > #include <linux/delay.h>
> >> > > #include <linux/gpio/consumer.h>
> >> > > #include <linux/module.h>
> >> > > @@ -189,7 +190,7 @@ static int mi0283qt_probe(struct spi_device *spi)
> >> > > if (IS_ERR(mipi->regulator))
> >> > > return PTR_ERR(mipi->regulator);
> >> > > - mipi->backlight = tinydrm_of_find_backlight(dev);
> >> > > + mipi->backlight = drm_of_find_backlight(dev);
> >> > > if (IS_ERR(mipi->backlight))
> >> > > return PTR_ERR(mipi->backlight);
> >> > > diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h
> >> > > index 104dd51..e8fba5b 100644
> >> > > --- a/include/drm/drm_of.h
> >> > > +++ b/include/drm/drm_of.h
> >> > > @@ -29,6 +29,7 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> >> > > int port, int endpoint,
> >> > > struct drm_panel **panel,
> >> > > struct drm_bridge **bridge);
> >> > > +struct backlight_device *drm_of_find_backlight(struct device *dev);
> >> > > #else
> >> > > static inline uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
> >> > > struct device_node *port)
> >> > > diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h
> >> > > index d554ded..e40ef2d 100644
> >> > > --- a/include/drm/tinydrm/tinydrm-helpers.h
> >> > > +++ b/include/drm/tinydrm/tinydrm-helpers.h
> >> > > @@ -46,7 +46,6 @@ void tinydrm_xrgb8888_to_rgb565(u16 *dst, void *vaddr,
> >> > > void tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb,
> >> > > struct drm_clip_rect *clip);
> >> > > -struct backlight_device *tinydrm_of_find_backlight(struct device *dev);
> >> > > int tinydrm_enable_backlight(struct backlight_device *backlight);
> >> > > int tinydrm_disable_backlight(struct backlight_device *backlight);
> >> >
> >>
> >> --
> >> Daniel Vetter
> >> Software Engineer, Intel Corporation
> >> http://blog.ffwll.ch
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/tinydrm: Move tinydrm_of_find_backlight into drm_of.c
@ 2017-09-29 3:47 ` Meghana Madhyastha
0 siblings, 0 replies; 14+ messages in thread
From: Meghana Madhyastha @ 2017-09-29 3:47 UTC (permalink / raw)
To: daniel, noralf, outreachy-kernel, dri-devel
On Thu, Sep 28, 2017 at 11:38:36AM +0200, Daniel Vetter wrote:
> On Thu, Sep 28, 2017 at 11:27 AM, Meghana Madhyastha
> <meghana.madhyastha@gmail.com> wrote:
> > On Thu, Sep 28, 2017 at 09:15:39AM +0200, Daniel Vetter wrote:
> >> On Wed, Sep 27, 2017 at 05:08:07PM +0200, Noralf Tr�nnes wrote:
> >> >
> >> > Den 27.09.2017 15.54, skrev Meghana Madhyastha:
> >> > > Rename tinydrm_of_find_backlight to drm_of_find_backlight
> >> > > and move it into drm_of.c from tinydrm-helpers.c. This is
> >> > > because other drivers in the drm subsystem might need to call
> >> > > this function. In that case and otherwise, it is better from
> >> > > an organizational point of view to move it into drm_of.c along
> >> > > with the other _of.c functions.
> >> > >
> >> > > Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
> >> > > ---
> >> >
> >> > I suggest you split this patch in 2, one to add the function and one to
> >> > use it in tinydrm.
> >>
> >> In general I'd agree to split into 3 phases: 1) add new function 2)
> >> convert drivers 3) remove old one.
> >>
> >> But since there's only 1 caller this seems like overkill.
> >>
> >> > > drivers/gpu/drm/drm_of.c | 41 ++++++++++++++++++++++++++
> >> > > drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 40 -------------------------
> >> > > drivers/gpu/drm/tinydrm/mi0283qt.c | 3 +-
> >> > > include/drm/drm_of.h | 1 +
> >> > > include/drm/tinydrm/tinydrm-helpers.h | 1 -
> >> > > 5 files changed, 44 insertions(+), 42 deletions(-)
> >> > >
> >> > > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> >> > > index 8dafbdf..d8cded3 100644
> >> > > --- a/drivers/gpu/drm/drm_of.c
> >> > > +++ b/drivers/gpu/drm/drm_of.c
> >> > > @@ -1,6 +1,7 @@
> >> > > #include <linux/component.h>
> >> > > #include <linux/export.h>
> >> > > #include <linux/list.h>
> >> > > +#include <linux/backlight.h>
> >> > > #include <linux/of_graph.h>
> >> > > #include <drm/drmP.h>
> >> > > #include <drm/drm_bridge.h>
> >> > > @@ -260,3 +261,43 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> >> > > return ret;
> >> > > }
> >> > > EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge);
> >> > > +
> >> > > +/**
> >> > > + * drm_of_find_backlight - Find backlight device in device-tree
> >> > > + * @dev: Device
> >> > > + *
> >> > > + * This function looks for a DT node pointed to by a property named 'backlight'
> >> > > + * and uses of_find_backlight_by_node() to get the backlight device.
> >> > > + * Additionally if the brightness property is zero, it is set to
> >> > > + * max_brightness.
> >> >
> >> > Please add a note here about the callers responsibility to call
> >> > put_device() when releasing the resource.
> >> > See the of_find_backlight_by_node() docs.
> >> >
> >> > > + *
> >> > > + * Returns:
> >> > > + * NULL if there's no backlight property.
> >> > > + * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
> >> > > + * is found.
> >> > > + * If the backlight device is found, a pointer to the structure is returned.
> >> > > + */
> >> > > +struct backlight_device *drm_of_find_backlight(struct device *dev)
> >> > > +{
> >> > > + struct backlight_device *backlight;
> >> > > + struct device_node *np;
> >> > > +
> >> > > + np = of_parse_phandle(dev->of_node, "backlight", 0);
> >> > > + if (!np)
> >> > > + return NULL;
> >> > > +
> >> > > + backlight = of_find_backlight_by_node(np);
> >> > > + of_node_put(np);
> >> > > +
> >> > > + if (!backlight)
> >> > > + return ERR_PTR(-EPROBE_DEFER);
> >> > > +
> >> > > + if (!backlight->props.brightness) {
> >> > > + backlight->props.brightness = backlight->props.max_brightness;
> >> > > + DRM_DEBUG_KMS("Backlight brightness set to %d\n",
> >> > > + backlight->props.brightness);
> >> > > + }
> >> > > +
> >> > > + return backlight;
> >> > > +}
> >> > > +EXPORT_SYMBOL(drm_of_find_backlight);
> >> >
> >> > Can you also please add a devm_ version of this function. tinydrm uses
> >> > devres[1] versions of functions for requiring device resources, so it
> >> > would be nice to do this for backlight as well. tinydrm is currently
> >> > broken in this respect, it doesn't put the backlight device.
> >> >
> >> > I had a devm_of_find_backlight() version lying around that I've adjusted
> >> > to give you an idea of what I'm talking about:
> >> >
> >> > static void devm_drm_of_find_backlight_release(void *data)
> >> > {
> >> > put_device(data);
> >> > }
> >> >
> >> > struct backlight_device *devm_drm_of_find_backlight(struct device *dev)
> >> > {
> >> > struct backlight_device *backlight;
> >> > int ret;
> >> >
> >> > backlight = drm_of_find_backlight(dev);
> >> > if (IS_ERR_OR_NULL(backlight))
> >> > return backlight;
> >> >
> >> > ret = devm_add_action(dev, devm_drm_of_find_backlight_release,
> >> > backlight->dev);
> >> > if (ret) {
> >> > put_device(backlight->dev);
> >> > return ERR_PTR(ret);
> >> > }
> >> >
> >> > return backlight;
> >> > }
> >>
> >> Good idea for a follow up patch imo.
> >>
> >> Another follow up patch series which would be really good is then
> >> converting drivers over to using this (preferrably the devm_ version). A
> >> quick git grep shows there's quite a few.
> >
> > I have a quick question on this. I did a git grep and found that the
> > other drivers used the function of_find_backlight_by_node defined in
> > video/backlight. This seems to be doing a different thing (taking
> > struct device_node* as a parameter instead of struct device*).
> > Basically, this function finds the backlight device by device-tree node.
> > How would you suggest converting these cases ?
>
> The function you're moving also uses of_find_backglight_by_node, but
> wraps it up to be more convenient for drm drivers. I didn't carefully
> audit all of them, but I'd assume a bunch of these other callers might
> be able to use the same convenience helper. But I might be wrong, and
> we need a drm_panel specific helper. Just something I've noticed and
> figured might be worth to check.
> -Daniel
Yes you are right. Sorry, I hadn't carefully looked into all of them,
but now having checked the calls, it looks like they can all be replaced by
of_find_backlight. The only thing extra of_find_backlight is doing is
setting the brightness to be the maximum brightness which makes sense here.
I can send in a patch to replace these.
> > Thanks and regards,
> > Meghana
> >
> >> Either way, the patch that adds these to the core should also add a TODO
> >> entry so we don't forget to complete this conversion.
> >>
> >> Thanks, Daniel
> >>
> >> >
> >> > [1] https://www.kernel.org/doc/Documentation/driver-model/devres.txt
> >> >
> >> > Noralf.
> >> >
> >> > > diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> >> > > index bd6cce0..cd4c6a5 100644
> >> > > --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> >> > > +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> >> > > @@ -237,46 +237,6 @@ void tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb,
> >> > > EXPORT_SYMBOL(tinydrm_xrgb8888_to_gray8);
> >> > > /**
> >> > > - * tinydrm_of_find_backlight - Find backlight device in device-tree
> >> > > - * @dev: Device
> >> > > - *
> >> > > - * This function looks for a DT node pointed to by a property named 'backlight'
> >> > > - * and uses of_find_backlight_by_node() to get the backlight device.
> >> > > - * Additionally if the brightness property is zero, it is set to
> >> > > - * max_brightness.
> >> > > - *
> >> > > - * Returns:
> >> > > - * NULL if there's no backlight property.
> >> > > - * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
> >> > > - * is found.
> >> > > - * If the backlight device is found, a pointer to the structure is returned.
> >> > > - */
> >> > > -struct backlight_device *tinydrm_of_find_backlight(struct device *dev)
> >> > > -{
> >> > > - struct backlight_device *backlight;
> >> > > - struct device_node *np;
> >> > > -
> >> > > - np = of_parse_phandle(dev->of_node, "backlight", 0);
> >> > > - if (!np)
> >> > > - return NULL;
> >> > > -
> >> > > - backlight = of_find_backlight_by_node(np);
> >> > > - of_node_put(np);
> >> > > -
> >> > > - if (!backlight)
> >> > > - return ERR_PTR(-EPROBE_DEFER);
> >> > > -
> >> > > - if (!backlight->props.brightness) {
> >> > > - backlight->props.brightness = backlight->props.max_brightness;
> >> > > - DRM_DEBUG_KMS("Backlight brightness set to %d\n",
> >> > > - backlight->props.brightness);
> >> > > - }
> >> > > -
> >> > > - return backlight;
> >> > > -}
> >> > > -EXPORT_SYMBOL(tinydrm_of_find_backlight);
> >> > > -
> >> > > -/**
> >> > > * tinydrm_enable_backlight - Enable backlight helper
> >> > > * @backlight: Backlight device
> >> > > *
> >> > > diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
> >> > > index 7e5bb7d..5e3d635 100644
> >> > > --- a/drivers/gpu/drm/tinydrm/mi0283qt.c
> >> > > +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
> >> > > @@ -12,6 +12,7 @@
> >> > > #include <drm/tinydrm/ili9341.h>
> >> > > #include <drm/tinydrm/mipi-dbi.h>
> >> > > #include <drm/tinydrm/tinydrm-helpers.h>
> >> > > +#include <drm/drm_of.h>
> >> > > #include <linux/delay.h>
> >> > > #include <linux/gpio/consumer.h>
> >> > > #include <linux/module.h>
> >> > > @@ -189,7 +190,7 @@ static int mi0283qt_probe(struct spi_device *spi)
> >> > > if (IS_ERR(mipi->regulator))
> >> > > return PTR_ERR(mipi->regulator);
> >> > > - mipi->backlight = tinydrm_of_find_backlight(dev);
> >> > > + mipi->backlight = drm_of_find_backlight(dev);
> >> > > if (IS_ERR(mipi->backlight))
> >> > > return PTR_ERR(mipi->backlight);
> >> > > diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h
> >> > > index 104dd51..e8fba5b 100644
> >> > > --- a/include/drm/drm_of.h
> >> > > +++ b/include/drm/drm_of.h
> >> > > @@ -29,6 +29,7 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> >> > > int port, int endpoint,
> >> > > struct drm_panel **panel,
> >> > > struct drm_bridge **bridge);
> >> > > +struct backlight_device *drm_of_find_backlight(struct device *dev);
> >> > > #else
> >> > > static inline uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
> >> > > struct device_node *port)
> >> > > diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h
> >> > > index d554ded..e40ef2d 100644
> >> > > --- a/include/drm/tinydrm/tinydrm-helpers.h
> >> > > +++ b/include/drm/tinydrm/tinydrm-helpers.h
> >> > > @@ -46,7 +46,6 @@ void tinydrm_xrgb8888_to_rgb565(u16 *dst, void *vaddr,
> >> > > void tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb,
> >> > > struct drm_clip_rect *clip);
> >> > > -struct backlight_device *tinydrm_of_find_backlight(struct device *dev);
> >> > > int tinydrm_enable_backlight(struct backlight_device *backlight);
> >> > > int tinydrm_disable_backlight(struct backlight_device *backlight);
> >> >
> >>
> >> --
> >> Daniel Vetter
> >> Software Engineer, Intel Corporation
> >> http://blog.ffwll.ch
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/tinydrm: Move tinydrm_of_find_backlight into drm_of.c
2017-09-27 13:54 ` Meghana Madhyastha
@ 2017-09-29 21:58 ` kbuild test robot
-1 siblings, 0 replies; 14+ messages in thread
From: kbuild test robot @ 2017-09-29 21:58 UTC (permalink / raw)
To: Meghana Madhyastha; +Cc: outreachy-kernel, dri-devel, kbuild-all
[-- Attachment #1: Type: text/plain, Size: 3250 bytes --]
Hi Meghana,
[auto build test ERROR on drm/drm-next]
[also build test ERROR on v4.14-rc2 next-20170929]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Meghana-Madhyastha/drm-tinydrm-Move-tinydrm_of_find_backlight-into-drm_of-c/20170930-052310
base: git://people.freedesktop.org/~airlied/linux.git drm-next
config: x86_64-randconfig-x017-201739 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All error/warnings (new ones prefixed by >>):
drivers/gpu//drm/tinydrm/mi0283qt.c: In function 'mi0283qt_probe':
>> drivers/gpu//drm/tinydrm/mi0283qt.c:192:20: error: implicit declaration of function 'drm_of_find_backlight' [-Werror=implicit-function-declaration]
mipi->backlight = drm_of_find_backlight(dev);
^~~~~~~~~~~~~~~~~~~~~
>> drivers/gpu//drm/tinydrm/mi0283qt.c:192:18: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
mipi->backlight = drm_of_find_backlight(dev);
^
cc1: some warnings being treated as errors
vim +/drm_of_find_backlight +192 drivers/gpu//drm/tinydrm/mi0283qt.c
163
164 static int mi0283qt_probe(struct spi_device *spi)
165 {
166 struct device *dev = &spi->dev;
167 struct mipi_dbi *mipi;
168 struct gpio_desc *dc;
169 u32 rotation = 0;
170 int ret;
171
172 mipi = devm_kzalloc(dev, sizeof(*mipi), GFP_KERNEL);
173 if (!mipi)
174 return -ENOMEM;
175
176 mipi->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
177 if (IS_ERR(mipi->reset)) {
178 dev_err(dev, "Failed to get gpio 'reset'\n");
179 return PTR_ERR(mipi->reset);
180 }
181
182 dc = devm_gpiod_get_optional(dev, "dc", GPIOD_OUT_LOW);
183 if (IS_ERR(dc)) {
184 dev_err(dev, "Failed to get gpio 'dc'\n");
185 return PTR_ERR(dc);
186 }
187
188 mipi->regulator = devm_regulator_get(dev, "power");
189 if (IS_ERR(mipi->regulator))
190 return PTR_ERR(mipi->regulator);
191
> 192 mipi->backlight = drm_of_find_backlight(dev);
193 if (IS_ERR(mipi->backlight))
194 return PTR_ERR(mipi->backlight);
195
196 device_property_read_u32(dev, "rotation", &rotation);
197
198 ret = mipi_dbi_spi_init(spi, mipi, dc);
199 if (ret)
200 return ret;
201
202 ret = mipi_dbi_init(&spi->dev, mipi, &mi0283qt_pipe_funcs,
203 &mi0283qt_driver, &mi0283qt_mode, rotation);
204 if (ret)
205 return ret;
206
207 ret = mi0283qt_init(mipi);
208 if (ret)
209 return ret;
210
211 /* use devres to fini after drm unregister (drv->remove is before) */
212 ret = devm_add_action(dev, mi0283qt_fini, mipi);
213 if (ret) {
214 mi0283qt_fini(mipi);
215 return ret;
216 }
217
218 spi_set_drvdata(spi, mipi);
219
220 return devm_tinydrm_register(&mipi->tinydrm);
221 }
222
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29255 bytes --]
[-- Attachment #3: Type: text/plain, Size: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/tinydrm: Move tinydrm_of_find_backlight into drm_of.c
@ 2017-09-29 21:58 ` kbuild test robot
0 siblings, 0 replies; 14+ messages in thread
From: kbuild test robot @ 2017-09-29 21:58 UTC (permalink / raw)
To: Meghana Madhyastha
Cc: kbuild-all, daniel, noralf, outreachy-kernel, dri-devel
[-- Attachment #1: Type: text/plain, Size: 3250 bytes --]
Hi Meghana,
[auto build test ERROR on drm/drm-next]
[also build test ERROR on v4.14-rc2 next-20170929]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Meghana-Madhyastha/drm-tinydrm-Move-tinydrm_of_find_backlight-into-drm_of-c/20170930-052310
base: git://people.freedesktop.org/~airlied/linux.git drm-next
config: x86_64-randconfig-x017-201739 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All error/warnings (new ones prefixed by >>):
drivers/gpu//drm/tinydrm/mi0283qt.c: In function 'mi0283qt_probe':
>> drivers/gpu//drm/tinydrm/mi0283qt.c:192:20: error: implicit declaration of function 'drm_of_find_backlight' [-Werror=implicit-function-declaration]
mipi->backlight = drm_of_find_backlight(dev);
^~~~~~~~~~~~~~~~~~~~~
>> drivers/gpu//drm/tinydrm/mi0283qt.c:192:18: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
mipi->backlight = drm_of_find_backlight(dev);
^
cc1: some warnings being treated as errors
vim +/drm_of_find_backlight +192 drivers/gpu//drm/tinydrm/mi0283qt.c
163
164 static int mi0283qt_probe(struct spi_device *spi)
165 {
166 struct device *dev = &spi->dev;
167 struct mipi_dbi *mipi;
168 struct gpio_desc *dc;
169 u32 rotation = 0;
170 int ret;
171
172 mipi = devm_kzalloc(dev, sizeof(*mipi), GFP_KERNEL);
173 if (!mipi)
174 return -ENOMEM;
175
176 mipi->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
177 if (IS_ERR(mipi->reset)) {
178 dev_err(dev, "Failed to get gpio 'reset'\n");
179 return PTR_ERR(mipi->reset);
180 }
181
182 dc = devm_gpiod_get_optional(dev, "dc", GPIOD_OUT_LOW);
183 if (IS_ERR(dc)) {
184 dev_err(dev, "Failed to get gpio 'dc'\n");
185 return PTR_ERR(dc);
186 }
187
188 mipi->regulator = devm_regulator_get(dev, "power");
189 if (IS_ERR(mipi->regulator))
190 return PTR_ERR(mipi->regulator);
191
> 192 mipi->backlight = drm_of_find_backlight(dev);
193 if (IS_ERR(mipi->backlight))
194 return PTR_ERR(mipi->backlight);
195
196 device_property_read_u32(dev, "rotation", &rotation);
197
198 ret = mipi_dbi_spi_init(spi, mipi, dc);
199 if (ret)
200 return ret;
201
202 ret = mipi_dbi_init(&spi->dev, mipi, &mi0283qt_pipe_funcs,
203 &mi0283qt_driver, &mi0283qt_mode, rotation);
204 if (ret)
205 return ret;
206
207 ret = mi0283qt_init(mipi);
208 if (ret)
209 return ret;
210
211 /* use devres to fini after drm unregister (drv->remove is before) */
212 ret = devm_add_action(dev, mi0283qt_fini, mipi);
213 if (ret) {
214 mi0283qt_fini(mipi);
215 return ret;
216 }
217
218 spi_set_drvdata(spi, mipi);
219
220 return devm_tinydrm_register(&mipi->tinydrm);
221 }
222
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29255 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2017-09-29 21:58 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-27 13:54 [PATCH] drm/tinydrm: Move tinydrm_of_find_backlight into drm_of.c Meghana Madhyastha
2017-09-27 13:54 ` Meghana Madhyastha
2017-09-27 15:08 ` Noralf Trønnes
2017-09-27 15:08 ` Noralf Trønnes
2017-09-28 7:15 ` Daniel Vetter
2017-09-28 7:15 ` Daniel Vetter
2017-09-28 9:27 ` Meghana Madhyastha
2017-09-28 9:27 ` Meghana Madhyastha
2017-09-28 9:38 ` Daniel Vetter
2017-09-28 9:38 ` Daniel Vetter
2017-09-29 3:47 ` Meghana Madhyastha
2017-09-29 3:47 ` Meghana Madhyastha
2017-09-29 21:58 ` kbuild test robot
2017-09-29 21:58 ` kbuild test robot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.