This patchset introduces some changes such as move tinydrm_of_find_backlight to drm_of.c and rename it to drm_of_find_backlight for better organizational structure.
Changes in v3: -Changed it back to a single patch for the caller in mi0283qt.c and the removal and renaming of the helper function.
Meghana Madhyastha (2): drm/tinydrm: Move tinydrm_of_find_backlight into drm_of.c drm/tinydrm: Add devres versions of drm_of_find_backlight
drivers/gpu/drm/drm_of.c | 92 ++++++++++++++++++++++++++ drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 40 ----------- drivers/gpu/drm/tinydrm/mi0283qt.c | 3 +- include/drm/drm_of.h | 3 + include/drm/tinydrm/tinydrm-helpers.h | 1 - 5 files changed, 97 insertions(+), 42 deletions(-)
Rename tinydrm_of_find_backlight to drm_of_find_backlight and move it into drm_of.c from tinydrm-helpers.c. This is because other drivers in the drm subsystem might need to call this function. In that case and otherwise, it is better from an organizational point of view to move it into drm_of.c along with the other _of.c functions.
Signed-off-by: Meghana Madhyastha meghana.madhyastha@gmail.com --- Changes in v3: -Change it back to a single patch from two patches in v2
drivers/gpu/drm/drm_of.c | 44 ++++++++++++++++++++++++++ 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, 47 insertions(+), 42 deletions(-)
diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c index 8dafbdf..d878d3a 100644 --- a/drivers/gpu/drm/drm_of.c +++ b/drivers/gpu/drm/drm_of.c @@ -1,6 +1,7 @@ #include <linux/component.h> #include <linux/export.h> #include <linux/list.h> +#include <linux/backlight.h> #include <linux/of_graph.h> #include <drm/drmP.h> #include <drm/drm_bridge.h> @@ -260,3 +261,46 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, return ret; } EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge); + +/** + * drm_of_find_backlight - Find backlight device in device-tree + * @dev: Device + * + * This function looks for a DT node pointed to by a property named 'backlight' + * and uses of_find_backlight_by_node() to get the backlight device. + * Additionally if the brightness property is zero, it is set to + * max_brightness. + * + * Note: It is the responsibility of the caller to call put_device() when + * releasing the resource. + * + * Returns: + * NULL if there's no backlight property. + * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device + * is found. + * If the backlight device is found, a pointer to the structure is returned. + */ +struct backlight_device *drm_of_find_backlight(struct device *dev) +{ + struct backlight_device *backlight; + struct device_node *np; + + np = of_parse_phandle(dev->of_node, "backlight", 0); + if (!np) + return NULL; + + backlight = of_find_backlight_by_node(np); + of_node_put(np); + + if (!backlight) + return ERR_PTR(-EPROBE_DEFER); + + if (!backlight->props.brightness) { + backlight->props.brightness = backlight->props.max_brightness; + DRM_DEBUG_KMS("Backlight brightness set to %d\n", + backlight->props.brightness); + } + + return backlight; +} +EXPORT_SYMBOL(drm_of_find_backlight); diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c index bd6cce0..cd4c6a5 100644 --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c @@ -237,46 +237,6 @@ void tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb, EXPORT_SYMBOL(tinydrm_xrgb8888_to_gray8);
/** - * tinydrm_of_find_backlight - Find backlight device in device-tree - * @dev: Device - * - * This function looks for a DT node pointed to by a property named 'backlight' - * and uses of_find_backlight_by_node() to get the backlight device. - * Additionally if the brightness property is zero, it is set to - * max_brightness. - * - * Returns: - * NULL if there's no backlight property. - * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device - * is found. - * If the backlight device is found, a pointer to the structure is returned. - */ -struct backlight_device *tinydrm_of_find_backlight(struct device *dev) -{ - struct backlight_device *backlight; - struct device_node *np; - - np = of_parse_phandle(dev->of_node, "backlight", 0); - if (!np) - return NULL; - - backlight = of_find_backlight_by_node(np); - of_node_put(np); - - if (!backlight) - return ERR_PTR(-EPROBE_DEFER); - - if (!backlight->props.brightness) { - backlight->props.brightness = backlight->props.max_brightness; - DRM_DEBUG_KMS("Backlight brightness set to %d\n", - backlight->props.brightness); - } - - return backlight; -} -EXPORT_SYMBOL(tinydrm_of_find_backlight); - -/** * tinydrm_enable_backlight - Enable backlight helper * @backlight: Backlight device * diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c index 7e5bb7d..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);
On Thu, Sep 28, 2017 at 02:44:34PM +0530, Meghana Madhyastha wrote:
Rename tinydrm_of_find_backlight to drm_of_find_backlight and move it into drm_of.c from tinydrm-helpers.c. This is because other drivers in the drm subsystem might need to call this function. In that case and otherwise, it is better from an organizational point of view to move it into drm_of.c along with the other _of.c functions.
Signed-off-by: Meghana Madhyastha meghana.madhyastha@gmail.com
Changes in v3: -Change it back to a single patch from two patches in v2
drivers/gpu/drm/drm_of.c | 44 ++++++++++++++++++++++++++ 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, 47 insertions(+), 42 deletions(-)
diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c index 8dafbdf..d878d3a 100644 --- a/drivers/gpu/drm/drm_of.c +++ b/drivers/gpu/drm/drm_of.c @@ -1,6 +1,7 @@ #include <linux/component.h> #include <linux/export.h> #include <linux/list.h> +#include <linux/backlight.h> #include <linux/of_graph.h> #include <drm/drmP.h> #include <drm/drm_bridge.h> @@ -260,3 +261,46 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, return ret; } EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge);
+/**
- drm_of_find_backlight - Find backlight device in device-tree
- @dev: Device
- This function looks for a DT node pointed to by a property named 'backlight'
- and uses of_find_backlight_by_node() to get the backlight device.
- Additionally if the brightness property is zero, it is set to
- max_brightness.
- Note: It is the responsibility of the caller to call put_device() when
- releasing the resource.
- Returns:
- NULL if there's no backlight property.
- Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
- is found.
- If the backlight device is found, a pointer to the structure is returned.
- */
+struct backlight_device *drm_of_find_backlight(struct device *dev) +{
- struct backlight_device *backlight;
- struct device_node *np;
- np = of_parse_phandle(dev->of_node, "backlight", 0);
- if (!np)
return NULL;- backlight = of_find_backlight_by_node(np);
- of_node_put(np);
- if (!backlight)
return ERR_PTR(-EPROBE_DEFER);- if (!backlight->props.brightness) {
backlight->props.brightness = backlight->props.max_brightness;DRM_DEBUG_KMS("Backlight brightness set to %d\n",backlight->props.brightness);- }
- return backlight;
+} +EXPORT_SYMBOL(drm_of_find_backlight); diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c index bd6cce0..cd4c6a5 100644 --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c @@ -237,46 +237,6 @@ void tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb, EXPORT_SYMBOL(tinydrm_xrgb8888_to_gray8);
/**
- tinydrm_of_find_backlight - Find backlight device in device-tree
- @dev: Device
- This function looks for a DT node pointed to by a property named 'backlight'
- and uses of_find_backlight_by_node() to get the backlight device.
- Additionally if the brightness property is zero, it is set to
- max_brightness.
- Returns:
- NULL if there's no backlight property.
- Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
- is found.
- If the backlight device is found, a pointer to the structure is returned.
- */
-struct backlight_device *tinydrm_of_find_backlight(struct device *dev) -{
- struct backlight_device *backlight;
- struct device_node *np;
- np = of_parse_phandle(dev->of_node, "backlight", 0);
- if (!np)
return NULL;- backlight = of_find_backlight_by_node(np);
- of_node_put(np);
- if (!backlight)
return ERR_PTR(-EPROBE_DEFER);- if (!backlight->props.brightness) {
backlight->props.brightness = backlight->props.max_brightness;DRM_DEBUG_KMS("Backlight brightness set to %d\n",backlight->props.brightness);- }
- return backlight;
-} -EXPORT_SYMBOL(tinydrm_of_find_backlight);
-/**
- tinydrm_enable_backlight - Enable backlight helper
- @backlight: Backlight device
diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c index 7e5bb7d..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)
Minor detail I missed: The dummy version for the #else here is missing. Not a problem for tinydrm, but might be an issue for a driver where CONFIG_OF is optional.
Otherwise I think this patch looks good. -Daniel
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
Den 28.09.2017 16.08, skrev Daniel Vetter:
On Thu, Sep 28, 2017 at 02:44:34PM +0530, Meghana Madhyastha wrote:
Rename tinydrm_of_find_backlight to drm_of_find_backlight and move it into drm_of.c from tinydrm-helpers.c. This is because other drivers in the drm subsystem might need to call this function. In that case and otherwise, it is better from an organizational point of view to move it into drm_of.c along with the other _of.c functions.
Signed-off-by: Meghana Madhyastha meghana.madhyastha@gmail.com
Changes in v3: -Change it back to a single patch from two patches in v2
drivers/gpu/drm/drm_of.c | 44 ++++++++++++++++++++++++++ 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, 47 insertions(+), 42 deletions(-)
diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c index 8dafbdf..d878d3a 100644 --- a/drivers/gpu/drm/drm_of.c +++ b/drivers/gpu/drm/drm_of.c @@ -1,6 +1,7 @@ #include <linux/component.h> #include <linux/export.h> #include <linux/list.h> +#include <linux/backlight.h> #include <linux/of_graph.h> #include <drm/drmP.h> #include <drm/drm_bridge.h> @@ -260,3 +261,46 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, return ret; } EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge);
+/**
- drm_of_find_backlight - Find backlight device in device-tree
- @dev: Device
- This function looks for a DT node pointed to by a property named 'backlight'
- and uses of_find_backlight_by_node() to get the backlight device.
- Additionally if the brightness property is zero, it is set to
- max_brightness.
- Note: It is the responsibility of the caller to call put_device() when
- releasing the resource.
- Returns:
- NULL if there's no backlight property.
- Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
- is found.
- If the backlight device is found, a pointer to the structure is returned.
- */
+struct backlight_device *drm_of_find_backlight(struct device *dev) +{
- struct backlight_device *backlight;
- struct device_node *np;
- np = of_parse_phandle(dev->of_node, "backlight", 0);
- if (!np)
return NULL;- backlight = of_find_backlight_by_node(np);
- of_node_put(np);
- if (!backlight)
return ERR_PTR(-EPROBE_DEFER);- if (!backlight->props.brightness) {
backlight->props.brightness = backlight->props.max_brightness;DRM_DEBUG_KMS("Backlight brightness set to %d\n",backlight->props.brightness);- }
- return backlight;
+} +EXPORT_SYMBOL(drm_of_find_backlight); diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c index bd6cce0..cd4c6a5 100644 --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c @@ -237,46 +237,6 @@ void tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb, EXPORT_SYMBOL(tinydrm_xrgb8888_to_gray8);
/**
- tinydrm_of_find_backlight - Find backlight device in device-tree
- @dev: Device
- This function looks for a DT node pointed to by a property named 'backlight'
- and uses of_find_backlight_by_node() to get the backlight device.
- Additionally if the brightness property is zero, it is set to
- max_brightness.
- Returns:
- NULL if there's no backlight property.
- Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
- is found.
- If the backlight device is found, a pointer to the structure is returned.
- */
-struct backlight_device *tinydrm_of_find_backlight(struct device *dev) -{
- struct backlight_device *backlight;
- struct device_node *np;
- np = of_parse_phandle(dev->of_node, "backlight", 0);
- if (!np)
return NULL;- backlight = of_find_backlight_by_node(np);
- of_node_put(np);
- if (!backlight)
return ERR_PTR(-EPROBE_DEFER);- if (!backlight->props.brightness) {
backlight->props.brightness = backlight->props.max_brightness;DRM_DEBUG_KMS("Backlight brightness set to %d\n",backlight->props.brightness);- }
- return backlight;
-} -EXPORT_SYMBOL(tinydrm_of_find_backlight);
-/**
- tinydrm_enable_backlight - Enable backlight helper
- @backlight: Backlight device
diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c index 7e5bb7d..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)
Minor detail I missed: The dummy version for the #else here is missing. Not a problem for tinydrm, but might be an issue for a driver where CONFIG_OF is optional.
Yeah, we need a dummy version. tinydrm can in theory be used on non-DT platforms. The only resource that isn't available there is backlight since there is no platform or ACPI way of describing that dependency.
Another problem I discovered when trying to compile this, is that if DRM is builtin and LCD_CLASS_DEVICE is a module:
drivers/gpu/drm/drm_of.c:292: undefined reference to `of_find_backlight_by_node'
I see that I solved this in tinydrm by just selecting BACKLIGHT_LCD_SUPPORT and LCD_CLASS_DEVICE. I'm not aware of a way to force LCD_CLASS_DEVICE to be builtin if DRM is builtin.
Arnd fixed a similar type of dependecy in the repaper driver by forcing repaper to be a module if necessary. To be honest, I don't know why his fix works: https://github.com/torvalds/linux/commit/8312a3fe84b96e30a010fa20f933606d976...
Another way of solving this is to put the function in drivers/gpu/drm/drm_backlight.c and add a DRM_BACKLIGHT konfig option that does the selection.
Noralf.
Otherwise I think this patch looks good. -Daniel
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
On Thu, Sep 28, 2017 at 06:02:27PM +0200, Noralf Trønnes wrote:
Den 28.09.2017 16.08, skrev Daniel Vetter:
On Thu, Sep 28, 2017 at 02:44:34PM +0530, Meghana Madhyastha wrote:
Rename tinydrm_of_find_backlight to drm_of_find_backlight and move it into drm_of.c from tinydrm-helpers.c. This is because other drivers in the drm subsystem might need to call this function. In that case and otherwise, it is better from an organizational point of view to move it into drm_of.c along with the other _of.c functions.
Signed-off-by: Meghana Madhyastha meghana.madhyastha@gmail.com
Changes in v3: -Change it back to a single patch from two patches in v2
drivers/gpu/drm/drm_of.c | 44 ++++++++++++++++++++++++++ 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, 47 insertions(+), 42 deletions(-)
diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c index 8dafbdf..d878d3a 100644 --- a/drivers/gpu/drm/drm_of.c +++ b/drivers/gpu/drm/drm_of.c @@ -1,6 +1,7 @@ #include <linux/component.h> #include <linux/export.h> #include <linux/list.h> +#include <linux/backlight.h> #include <linux/of_graph.h> #include <drm/drmP.h> #include <drm/drm_bridge.h> @@ -260,3 +261,46 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, return ret; } EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge);
+/**
- drm_of_find_backlight - Find backlight device in device-tree
- @dev: Device
- This function looks for a DT node pointed to by a property named 'backlight'
- and uses of_find_backlight_by_node() to get the backlight device.
- Additionally if the brightness property is zero, it is set to
- max_brightness.
- Note: It is the responsibility of the caller to call put_device() when
- releasing the resource.
- Returns:
- NULL if there's no backlight property.
- Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
- is found.
- If the backlight device is found, a pointer to the structure is returned.
- */
+struct backlight_device *drm_of_find_backlight(struct device *dev) +{
- struct backlight_device *backlight;
- struct device_node *np;
- np = of_parse_phandle(dev->of_node, "backlight", 0);
- if (!np)
return NULL;- backlight = of_find_backlight_by_node(np);
- of_node_put(np);
- if (!backlight)
return ERR_PTR(-EPROBE_DEFER);- if (!backlight->props.brightness) {
backlight->props.brightness = backlight->props.max_brightness;DRM_DEBUG_KMS("Backlight brightness set to %d\n",backlight->props.brightness);- }
- return backlight;
+} +EXPORT_SYMBOL(drm_of_find_backlight); diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c index bd6cce0..cd4c6a5 100644 --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c @@ -237,46 +237,6 @@ void tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb, EXPORT_SYMBOL(tinydrm_xrgb8888_to_gray8); /**
- tinydrm_of_find_backlight - Find backlight device in device-tree
- @dev: Device
- This function looks for a DT node pointed to by a property named 'backlight'
- and uses of_find_backlight_by_node() to get the backlight device.
- Additionally if the brightness property is zero, it is set to
- max_brightness.
- Returns:
- NULL if there's no backlight property.
- Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
- is found.
- If the backlight device is found, a pointer to the structure is returned.
- */
-struct backlight_device *tinydrm_of_find_backlight(struct device *dev) -{
- struct backlight_device *backlight;
- struct device_node *np;
- np = of_parse_phandle(dev->of_node, "backlight", 0);
- if (!np)
return NULL;- backlight = of_find_backlight_by_node(np);
- of_node_put(np);
- if (!backlight)
return ERR_PTR(-EPROBE_DEFER);- if (!backlight->props.brightness) {
backlight->props.brightness = backlight->props.max_brightness;DRM_DEBUG_KMS("Backlight brightness set to %d\n",backlight->props.brightness);- }
- return backlight;
-} -EXPORT_SYMBOL(tinydrm_of_find_backlight);
-/**
- tinydrm_enable_backlight - Enable backlight helper
- @backlight: Backlight device
diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c index 7e5bb7d..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)
Minor detail I missed: The dummy version for the #else here is missing. Not a problem for tinydrm, but might be an issue for a driver where CONFIG_OF is optional.
Yeah, we need a dummy version. tinydrm can in theory be used on non-DT platforms. The only resource that isn't available there is backlight since there is no platform or ACPI way of describing that dependency.
Another problem I discovered when trying to compile this, is that if DRM is builtin and LCD_CLASS_DEVICE is a module:
drivers/gpu/drm/drm_of.c:292: undefined reference to `of_find_backlight_by_node'
I see that I solved this in tinydrm by just selecting BACKLIGHT_LCD_SUPPORT and LCD_CLASS_DEVICE. I'm not aware of a way to force LCD_CLASS_DEVICE to be builtin if DRM is builtin.
Arnd fixed a similar type of dependecy in the repaper driver by forcing repaper to be a module if necessary. To be honest, I don't know why his fix works: https://github.com/torvalds/linux/commit/8312a3fe84b96e30a010fa20f933606d976...
I didn't quite understand how we could use this fix for the current case. Isn't this specific to the repaper driver dependency ?
Another way of solving this is to put the function in drivers/gpu/drm/drm_backlight.c and add a DRM_BACKLIGHT konfig option that does the selection.
Noralf.
Otherwise I think this patch looks good. -Daniel
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
Den 29.09.2017 05.22, skrev Meghana Madhyastha:
On Thu, Sep 28, 2017 at 06:02:27PM +0200, Noralf Trønnes wrote:
Den 28.09.2017 16.08, skrev Daniel Vetter:
On Thu, Sep 28, 2017 at 02:44:34PM +0530, Meghana Madhyastha wrote:
Rename tinydrm_of_find_backlight to drm_of_find_backlight and move it into drm_of.c from tinydrm-helpers.c. This is because other drivers in the drm subsystem might need to call this function. In that case and otherwise, it is better from an organizational point of view to move it into drm_of.c along with the other _of.c functions.
Signed-off-by: Meghana Madhyastha meghana.madhyastha@gmail.com
Changes in v3: -Change it back to a single patch from two patches in v2
drivers/gpu/drm/drm_of.c | 44 ++++++++++++++++++++++++++ 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, 47 insertions(+), 42 deletions(-)
diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c index 8dafbdf..d878d3a 100644 --- a/drivers/gpu/drm/drm_of.c +++ b/drivers/gpu/drm/drm_of.c @@ -1,6 +1,7 @@ #include <linux/component.h> #include <linux/export.h> #include <linux/list.h> +#include <linux/backlight.h> #include <linux/of_graph.h> #include <drm/drmP.h> #include <drm/drm_bridge.h> @@ -260,3 +261,46 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, return ret; } EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge);
+/**
- drm_of_find_backlight - Find backlight device in device-tree
- @dev: Device
- This function looks for a DT node pointed to by a property named 'backlight'
- and uses of_find_backlight_by_node() to get the backlight device.
- Additionally if the brightness property is zero, it is set to
- max_brightness.
- Note: It is the responsibility of the caller to call put_device() when
- releasing the resource.
- Returns:
- NULL if there's no backlight property.
- Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
- is found.
- If the backlight device is found, a pointer to the structure is returned.
- */
+struct backlight_device *drm_of_find_backlight(struct device *dev) +{
- struct backlight_device *backlight;
- struct device_node *np;
- np = of_parse_phandle(dev->of_node, "backlight", 0);
- if (!np)
return NULL;- backlight = of_find_backlight_by_node(np);
- of_node_put(np);
- if (!backlight)
return ERR_PTR(-EPROBE_DEFER);- if (!backlight->props.brightness) {
backlight->props.brightness = backlight->props.max_brightness;DRM_DEBUG_KMS("Backlight brightness set to %d\n",backlight->props.brightness);- }
- return backlight;
+} +EXPORT_SYMBOL(drm_of_find_backlight); diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c index bd6cce0..cd4c6a5 100644 --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c @@ -237,46 +237,6 @@ void tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb, EXPORT_SYMBOL(tinydrm_xrgb8888_to_gray8); /**
- tinydrm_of_find_backlight - Find backlight device in device-tree
- @dev: Device
- This function looks for a DT node pointed to by a property named 'backlight'
- and uses of_find_backlight_by_node() to get the backlight device.
- Additionally if the brightness property is zero, it is set to
- max_brightness.
- Returns:
- NULL if there's no backlight property.
- Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
- is found.
- If the backlight device is found, a pointer to the structure is returned.
- */
-struct backlight_device *tinydrm_of_find_backlight(struct device *dev) -{
- struct backlight_device *backlight;
- struct device_node *np;
- np = of_parse_phandle(dev->of_node, "backlight", 0);
- if (!np)
return NULL;- backlight = of_find_backlight_by_node(np);
- of_node_put(np);
- if (!backlight)
return ERR_PTR(-EPROBE_DEFER);- if (!backlight->props.brightness) {
backlight->props.brightness = backlight->props.max_brightness;DRM_DEBUG_KMS("Backlight brightness set to %d\n",backlight->props.brightness);- }
- return backlight;
-} -EXPORT_SYMBOL(tinydrm_of_find_backlight);
-/**
- tinydrm_enable_backlight - Enable backlight helper
- @backlight: Backlight device
diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c index 7e5bb7d..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)
Minor detail I missed: The dummy version for the #else here is missing. Not a problem for tinydrm, but might be an issue for a driver where CONFIG_OF is optional.
Yeah, we need a dummy version. tinydrm can in theory be used on non-DT platforms. The only resource that isn't available there is backlight since there is no platform or ACPI way of describing that dependency.
Another problem I discovered when trying to compile this, is that if DRM is builtin and LCD_CLASS_DEVICE is a module:
drivers/gpu/drm/drm_of.c:292: undefined reference to `of_find_backlight_by_node'
I see that I solved this in tinydrm by just selecting BACKLIGHT_LCD_SUPPORT and LCD_CLASS_DEVICE. I'm not aware of a way to force LCD_CLASS_DEVICE to be builtin if DRM is builtin.
Arnd fixed a similar type of dependecy in the repaper driver by forcing repaper to be a module if necessary. To be honest, I don't know why his fix works: https://github.com/torvalds/linux/commit/8312a3fe84b96e30a010fa20f933606d976...
I didn't quite understand how we could use this fix for the current case. Isn't this specific to the repaper driver dependency ?
This is the solution Daniel mentions:
menuconfig DRM tristate "Direct Rendering Manager (XFree86 4.1.0 and higher DRI support)" depends on (AGP || AGP=n) && !EMULATED_CMPXCHG && HAS_DMA + depends on (LCD_CLASS_DEVICE || LCD_CLASS_DEVICE=n)
We also have another problem: CONFIG_OF=y && CONFIG_LCD_CLASS_DEVICE=n That will result in a missing symbol error.
You can try this change to include/linux/backlight.h:
-#ifdef CONFIG_OF +#if defined(CONFIG_OF) && IS_ENABLED(CONFIG_LCD_CLASS_DEVICE) struct backlight_device *of_find_backlight_by_node(struct device_node *node); #else static inline struct backlight_device * of_find_backlight_by_node(struct device_node *node) { return NULL; } #endif
This will need to be a patch of its own since it's another subsystem.
You have to try the various kconfig variations to be sure, I'm no kconfig expert. CONFIG_OF=y/n, CONFIG_DRM=y/m, CONFIG_LCD_CLASS_DEVICE=y/m/n
Noralf.
Another way of solving this is to put the function in drivers/gpu/drm/drm_backlight.c and add a DRM_BACKLIGHT konfig option that does the selection.
Noralf.
Otherwise I think this patch looks good. -Daniel
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
On Fri, Sep 29, 2017 at 02:10:31PM +0200, Noralf Trønnes wrote:
Den 29.09.2017 05.22, skrev Meghana Madhyastha:
On Thu, Sep 28, 2017 at 06:02:27PM +0200, Noralf Trønnes wrote:
Den 28.09.2017 16.08, skrev Daniel Vetter:
On Thu, Sep 28, 2017 at 02:44:34PM +0530, Meghana Madhyastha wrote:
Rename tinydrm_of_find_backlight to drm_of_find_backlight and move it into drm_of.c from tinydrm-helpers.c. This is because other drivers in the drm subsystem might need to call this function. In that case and otherwise, it is better from an organizational point of view to move it into drm_of.c along with the other _of.c functions.
Signed-off-by: Meghana Madhyastha meghana.madhyastha@gmail.com
Changes in v3: -Change it back to a single patch from two patches in v2
drivers/gpu/drm/drm_of.c | 44 ++++++++++++++++++++++++++ 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, 47 insertions(+), 42 deletions(-)
diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c index 8dafbdf..d878d3a 100644 --- a/drivers/gpu/drm/drm_of.c +++ b/drivers/gpu/drm/drm_of.c @@ -1,6 +1,7 @@ #include <linux/component.h> #include <linux/export.h> #include <linux/list.h> +#include <linux/backlight.h> #include <linux/of_graph.h> #include <drm/drmP.h> #include <drm/drm_bridge.h> @@ -260,3 +261,46 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, return ret; } EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge);
+/**
- drm_of_find_backlight - Find backlight device in device-tree
- @dev: Device
- This function looks for a DT node pointed to by a property named 'backlight'
- and uses of_find_backlight_by_node() to get the backlight device.
- Additionally if the brightness property is zero, it is set to
- max_brightness.
- Note: It is the responsibility of the caller to call put_device() when
- releasing the resource.
- Returns:
- NULL if there's no backlight property.
- Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
- is found.
- If the backlight device is found, a pointer to the structure is returned.
- */
+struct backlight_device *drm_of_find_backlight(struct device *dev) +{
- struct backlight_device *backlight;
- struct device_node *np;
- np = of_parse_phandle(dev->of_node, "backlight", 0);
- if (!np)
return NULL;- backlight = of_find_backlight_by_node(np);
- of_node_put(np);
- if (!backlight)
return ERR_PTR(-EPROBE_DEFER);- if (!backlight->props.brightness) {
backlight->props.brightness = backlight->props.max_brightness;DRM_DEBUG_KMS("Backlight brightness set to %d\n",backlight->props.brightness);- }
- return backlight;
+} +EXPORT_SYMBOL(drm_of_find_backlight); diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c index bd6cce0..cd4c6a5 100644 --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c @@ -237,46 +237,6 @@ void tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb, EXPORT_SYMBOL(tinydrm_xrgb8888_to_gray8); /**
- tinydrm_of_find_backlight - Find backlight device in device-tree
- @dev: Device
- This function looks for a DT node pointed to by a property named 'backlight'
- and uses of_find_backlight_by_node() to get the backlight device.
- Additionally if the brightness property is zero, it is set to
- max_brightness.
- Returns:
- NULL if there's no backlight property.
- Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
- is found.
- If the backlight device is found, a pointer to the structure is returned.
- */
-struct backlight_device *tinydrm_of_find_backlight(struct device *dev) -{
- struct backlight_device *backlight;
- struct device_node *np;
- np = of_parse_phandle(dev->of_node, "backlight", 0);
- if (!np)
return NULL;- backlight = of_find_backlight_by_node(np);
- of_node_put(np);
- if (!backlight)
return ERR_PTR(-EPROBE_DEFER);- if (!backlight->props.brightness) {
backlight->props.brightness = backlight->props.max_brightness;DRM_DEBUG_KMS("Backlight brightness set to %d\n",backlight->props.brightness);- }
- return backlight;
-} -EXPORT_SYMBOL(tinydrm_of_find_backlight);
-/**
- tinydrm_enable_backlight - Enable backlight helper
- @backlight: Backlight device
diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c index 7e5bb7d..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)
Minor detail I missed: The dummy version for the #else here is missing. Not a problem for tinydrm, but might be an issue for a driver where CONFIG_OF is optional.
Yeah, we need a dummy version. tinydrm can in theory be used on non-DT platforms. The only resource that isn't available there is backlight since there is no platform or ACPI way of describing that dependency.
Another problem I discovered when trying to compile this, is that if DRM is builtin and LCD_CLASS_DEVICE is a module:
drivers/gpu/drm/drm_of.c:292: undefined reference to `of_find_backlight_by_node'
I see that I solved this in tinydrm by just selecting BACKLIGHT_LCD_SUPPORT and LCD_CLASS_DEVICE. I'm not aware of a way to force LCD_CLASS_DEVICE to be builtin if DRM is builtin.
Arnd fixed a similar type of dependecy in the repaper driver by forcing repaper to be a module if necessary. To be honest, I don't know why his fix works: https://github.com/torvalds/linux/commit/8312a3fe84b96e30a010fa20f933606d976...
I didn't quite understand how we could use this fix for the current case. Isn't this specific to the repaper driver dependency ?
This is the solution Daniel mentions:
menuconfig DRM tristate "Direct Rendering Manager (XFree86 4.1.0 and higher DRI support)" depends on (AGP || AGP=n) && !EMULATED_CMPXCHG && HAS_DMA + depends on (LCD_CLASS_DEVICE || LCD_CLASS_DEVICE=n)
I had tried this but it resulted in a recursive dependencies error.
Regards, Meghana
We also have another problem: CONFIG_OF=y && CONFIG_LCD_CLASS_DEVICE=n That will result in a missing symbol error.
You can try this change to include/linux/backlight.h:
-#ifdef CONFIG_OF +#if defined(CONFIG_OF) && IS_ENABLED(CONFIG_LCD_CLASS_DEVICE) struct backlight_device *of_find_backlight_by_node(struct device_node *node); #else static inline struct backlight_device * of_find_backlight_by_node(struct device_node *node) { return NULL; } #endif
This will need to be a patch of its own since it's another subsystem.
You have to try the various kconfig variations to be sure, I'm no kconfig expert. CONFIG_OF=y/n, CONFIG_DRM=y/m, CONFIG_LCD_CLASS_DEVICE=y/m/n
Noralf.
Sure will try this.
Another way of solving this is to put the function in drivers/gpu/drm/drm_backlight.c and add a DRM_BACKLIGHT konfig option that does the selection.
Noralf.
Otherwise I think this patch looks good. -Daniel
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
Den 29.09.2017 14.20, skrev Meghana Madhyastha:
On Fri, Sep 29, 2017 at 02:10:31PM +0200, Noralf Trønnes wrote:
Den 29.09.2017 05.22, skrev Meghana Madhyastha:
On Thu, Sep 28, 2017 at 06:02:27PM +0200, Noralf Trønnes wrote:
Den 28.09.2017 16.08, skrev Daniel Vetter:
On Thu, Sep 28, 2017 at 02:44:34PM +0530, Meghana Madhyastha wrote:
Rename tinydrm_of_find_backlight to drm_of_find_backlight and move it into drm_of.c from tinydrm-helpers.c. This is because other drivers in the drm subsystem might need to call this function. In that case and otherwise, it is better from an organizational point of view to move it into drm_of.c along with the other _of.c functions.
Signed-off-by: Meghana Madhyastha meghana.madhyastha@gmail.com
Changes in v3: -Change it back to a single patch from two patches in v2
drivers/gpu/drm/drm_of.c | 44 ++++++++++++++++++++++++++ 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, 47 insertions(+), 42 deletions(-)
diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c index 8dafbdf..d878d3a 100644 --- a/drivers/gpu/drm/drm_of.c +++ b/drivers/gpu/drm/drm_of.c @@ -1,6 +1,7 @@ #include <linux/component.h> #include <linux/export.h> #include <linux/list.h> +#include <linux/backlight.h> #include <linux/of_graph.h> #include <drm/drmP.h> #include <drm/drm_bridge.h> @@ -260,3 +261,46 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, return ret; } EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge);
+/**
- drm_of_find_backlight - Find backlight device in device-tree
- @dev: Device
- This function looks for a DT node pointed to by a property named 'backlight'
- and uses of_find_backlight_by_node() to get the backlight device.
- Additionally if the brightness property is zero, it is set to
- max_brightness.
- Note: It is the responsibility of the caller to call put_device() when
- releasing the resource.
- Returns:
- NULL if there's no backlight property.
- Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
- is found.
- If the backlight device is found, a pointer to the structure is returned.
- */
+struct backlight_device *drm_of_find_backlight(struct device *dev) +{
- struct backlight_device *backlight;
- struct device_node *np;
- np = of_parse_phandle(dev->of_node, "backlight", 0);
- if (!np)
return NULL;- backlight = of_find_backlight_by_node(np);
- of_node_put(np);
- if (!backlight)
return ERR_PTR(-EPROBE_DEFER);- if (!backlight->props.brightness) {
backlight->props.brightness = backlight->props.max_brightness;DRM_DEBUG_KMS("Backlight brightness set to %d\n",backlight->props.brightness);- }
- return backlight;
+} +EXPORT_SYMBOL(drm_of_find_backlight); diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c index bd6cce0..cd4c6a5 100644 --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c @@ -237,46 +237,6 @@ void tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb, EXPORT_SYMBOL(tinydrm_xrgb8888_to_gray8); /**
- tinydrm_of_find_backlight - Find backlight device in device-tree
- @dev: Device
- This function looks for a DT node pointed to by a property named 'backlight'
- and uses of_find_backlight_by_node() to get the backlight device.
- Additionally if the brightness property is zero, it is set to
- max_brightness.
- Returns:
- NULL if there's no backlight property.
- Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
- is found.
- If the backlight device is found, a pointer to the structure is returned.
- */
-struct backlight_device *tinydrm_of_find_backlight(struct device *dev) -{
- struct backlight_device *backlight;
- struct device_node *np;
- np = of_parse_phandle(dev->of_node, "backlight", 0);
- if (!np)
return NULL;- backlight = of_find_backlight_by_node(np);
- of_node_put(np);
- if (!backlight)
return ERR_PTR(-EPROBE_DEFER);- if (!backlight->props.brightness) {
backlight->props.brightness = backlight->props.max_brightness;DRM_DEBUG_KMS("Backlight brightness set to %d\n",backlight->props.brightness);- }
- return backlight;
-} -EXPORT_SYMBOL(tinydrm_of_find_backlight);
-/**
- tinydrm_enable_backlight - Enable backlight helper
- @backlight: Backlight device
diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c index 7e5bb7d..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)
Minor detail I missed: The dummy version for the #else here is missing. Not a problem for tinydrm, but might be an issue for a driver where CONFIG_OF is optional.
Yeah, we need a dummy version. tinydrm can in theory be used on non-DT platforms. The only resource that isn't available there is backlight since there is no platform or ACPI way of describing that dependency.
Another problem I discovered when trying to compile this, is that if DRM is builtin and LCD_CLASS_DEVICE is a module:
drivers/gpu/drm/drm_of.c:292: undefined reference to `of_find_backlight_by_node'
I see that I solved this in tinydrm by just selecting BACKLIGHT_LCD_SUPPORT and LCD_CLASS_DEVICE. I'm not aware of a way to force LCD_CLASS_DEVICE to be builtin if DRM is builtin.
Arnd fixed a similar type of dependecy in the repaper driver by forcing repaper to be a module if necessary. To be honest, I don't know why his fix works: https://github.com/torvalds/linux/commit/8312a3fe84b96e30a010fa20f933606d976...
I didn't quite understand how we could use this fix for the current case. Isn't this specific to the repaper driver dependency ?
This is the solution Daniel mentions:
menuconfig DRM tristate "Direct Rendering Manager (XFree86 4.1.0 and higher DRI support)" depends on (AGP || AGP=n) && !EMULATED_CMPXCHG && HAS_DMA + depends on (LCD_CLASS_DEVICE || LCD_CLASS_DEVICE=n)
I had tried this but it resulted in a recursive dependencies error.
What's the error? Could this have something to do with tinydrm selecting backlight and depending on DRM?
Noralf.
Regards, Meghana
We also have another problem: CONFIG_OF=y && CONFIG_LCD_CLASS_DEVICE=n That will result in a missing symbol error.
You can try this change to include/linux/backlight.h:
-#ifdef CONFIG_OF +#if defined(CONFIG_OF) && IS_ENABLED(CONFIG_LCD_CLASS_DEVICE) struct backlight_device *of_find_backlight_by_node(struct device_node *node); #else static inline struct backlight_device * of_find_backlight_by_node(struct device_node *node) { return NULL; } #endif
This will need to be a patch of its own since it's another subsystem.
You have to try the various kconfig variations to be sure, I'm no kconfig expert. CONFIG_OF=y/n, CONFIG_DRM=y/m, CONFIG_LCD_CLASS_DEVICE=y/m/n
Noralf.
Sure will try this.
Another way of solving this is to put the function in drivers/gpu/drm/drm_backlight.c and add a DRM_BACKLIGHT konfig option that does the selection.
Noralf.
Otherwise I think this patch looks good. -Daniel
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
On Fri, Sep 29, 2017 at 02:33:12PM +0200, Noralf Trønnes wrote:
Den 29.09.2017 14.20, skrev Meghana Madhyastha:
On Fri, Sep 29, 2017 at 02:10:31PM +0200, Noralf Trønnes wrote:
Den 29.09.2017 05.22, skrev Meghana Madhyastha:
On Thu, Sep 28, 2017 at 06:02:27PM +0200, Noralf Trønnes wrote:
Den 28.09.2017 16.08, skrev Daniel Vetter:
On Thu, Sep 28, 2017 at 02:44:34PM +0530, Meghana Madhyastha wrote: >Rename tinydrm_of_find_backlight to drm_of_find_backlight >and move it into drm_of.c from tinydrm-helpers.c. This is >because other drivers in the drm subsystem might need to call >this function. In that case and otherwise, it is better from >an organizational point of view to move it into drm_of.c along >with the other _of.c functions. > >Signed-off-by: Meghana Madhyastha meghana.madhyastha@gmail.com >--- >Changes in v3: >-Change it back to a single patch from two patches in v2 > > drivers/gpu/drm/drm_of.c | 44 ++++++++++++++++++++++++++ > 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, 47 insertions(+), 42 deletions(-) > >diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c >index 8dafbdf..d878d3a 100644 >--- a/drivers/gpu/drm/drm_of.c >+++ b/drivers/gpu/drm/drm_of.c >@@ -1,6 +1,7 @@ > #include <linux/component.h> > #include <linux/export.h> > #include <linux/list.h> >+#include <linux/backlight.h> > #include <linux/of_graph.h> > #include <drm/drmP.h> > #include <drm/drm_bridge.h> >@@ -260,3 +261,46 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, > return ret; > } > EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge); >+ >+/** >+ * drm_of_find_backlight - Find backlight device in device-tree >+ * @dev: Device >+ * >+ * This function looks for a DT node pointed to by a property named 'backlight' >+ * and uses of_find_backlight_by_node() to get the backlight device. >+ * Additionally if the brightness property is zero, it is set to >+ * max_brightness. >+ * >+ * Note: It is the responsibility of the caller to call put_device() when >+ * releasing the resource. >+ * >+ * Returns: >+ * NULL if there's no backlight property. >+ * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device >+ * is found. >+ * If the backlight device is found, a pointer to the structure is returned. >+ */ >+struct backlight_device *drm_of_find_backlight(struct device *dev) >+{ >+ struct backlight_device *backlight; >+ struct device_node *np; >+ >+ np = of_parse_phandle(dev->of_node, "backlight", 0); >+ if (!np) >+ return NULL; >+ >+ backlight = of_find_backlight_by_node(np); >+ of_node_put(np); >+ >+ if (!backlight) >+ return ERR_PTR(-EPROBE_DEFER); >+ >+ if (!backlight->props.brightness) { >+ backlight->props.brightness = backlight->props.max_brightness; >+ DRM_DEBUG_KMS("Backlight brightness set to %d\n", >+ backlight->props.brightness); >+ } >+ >+ return backlight; >+} >+EXPORT_SYMBOL(drm_of_find_backlight); >diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c >index bd6cce0..cd4c6a5 100644 >--- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c >+++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c >@@ -237,46 +237,6 @@ void tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb, > EXPORT_SYMBOL(tinydrm_xrgb8888_to_gray8); > /** >- * tinydrm_of_find_backlight - Find backlight device in device-tree >- * @dev: Device >- * >- * This function looks for a DT node pointed to by a property named 'backlight' >- * and uses of_find_backlight_by_node() to get the backlight device. >- * Additionally if the brightness property is zero, it is set to >- * max_brightness. >- * >- * Returns: >- * NULL if there's no backlight property. >- * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device >- * is found. >- * If the backlight device is found, a pointer to the structure is returned. >- */ >-struct backlight_device *tinydrm_of_find_backlight(struct device *dev) >-{ >- struct backlight_device *backlight; >- struct device_node *np; >- >- np = of_parse_phandle(dev->of_node, "backlight", 0); >- if (!np) >- return NULL; >- >- backlight = of_find_backlight_by_node(np); >- of_node_put(np); >- >- if (!backlight) >- return ERR_PTR(-EPROBE_DEFER); >- >- if (!backlight->props.brightness) { >- backlight->props.brightness = backlight->props.max_brightness; >- DRM_DEBUG_KMS("Backlight brightness set to %d\n", >- backlight->props.brightness); >- } >- >- return backlight; >-} >-EXPORT_SYMBOL(tinydrm_of_find_backlight); >- >-/** > * tinydrm_enable_backlight - Enable backlight helper > * @backlight: Backlight device > * >diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c >index 7e5bb7d..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) Minor detail I missed: The dummy version for the #else here is missing. Not a problem for tinydrm, but might be an issue for a driver where CONFIG_OF is optional.
Yeah, we need a dummy version. tinydrm can in theory be used on non-DT platforms. The only resource that isn't available there is backlight since there is no platform or ACPI way of describing that dependency.
Another problem I discovered when trying to compile this, is that if DRM is builtin and LCD_CLASS_DEVICE is a module:
drivers/gpu/drm/drm_of.c:292: undefined reference to `of_find_backlight_by_node'
I see that I solved this in tinydrm by just selecting BACKLIGHT_LCD_SUPPORT and LCD_CLASS_DEVICE. I'm not aware of a way to force LCD_CLASS_DEVICE to be builtin if DRM is builtin.
Arnd fixed a similar type of dependecy in the repaper driver by forcing repaper to be a module if necessary. To be honest, I don't know why his fix works: https://github.com/torvalds/linux/commit/8312a3fe84b96e30a010fa20f933606d976...
I didn't quite understand how we could use this fix for the current case. Isn't this specific to the repaper driver dependency ?
This is the solution Daniel mentions:
menuconfig DRM tristate "Direct Rendering Manager (XFree86 4.1.0 and higher DRI support)" depends on (AGP || AGP=n) && !EMULATED_CMPXCHG && HAS_DMA + depends on (LCD_CLASS_DEVICE || LCD_CLASS_DEVICE=n)
I had tried this but it resulted in a recursive dependencies error.
What's the error? Could this have something to do with tinydrm selecting backlight and depending on DRM?
Noralf.
Looks like a case of circular dependencies. The exact error is this:
scripts/kconfig/mconf Kconfig drivers/gpu/drm/Kconfig:7:error: recursive dependency detected! For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/gpu/drm/Kconfig:7: symbol DRM depends on LCD_CLASS_DEVICE For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/video/backlight/Kconfig:16: symbol LCD_CLASS_DEVICE is selected by FB_CLPS711X For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/video/fbdev/Kconfig:338: symbol FB_CLPS711X depends on FB For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/video/fbdev/Kconfig:5: symbol FB is selected by DRM_KMS_FB_HELPER For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/gpu/drm/Kconfig:73: symbol DRM_KMS_FB_HELPER is selected by DRM_FBDEV_EMULATION For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/gpu/drm/Kconfig:90: symbol DRM_FBDEV_EMULATION depends on DRM
Long story short, the dependency loop is as follows:
DRM -> LCD_CLASS_DEVICE -> FB_CLPS711X -> FB -> DRM_KMS_FB_HELPER -> DRM_FBDEV_EMULATION -> DRM
Regards, Meghana
Regards, Meghana
We also have another problem: CONFIG_OF=y && CONFIG_LCD_CLASS_DEVICE=n That will result in a missing symbol error.
You can try this change to include/linux/backlight.h:
-#ifdef CONFIG_OF +#if defined(CONFIG_OF) && IS_ENABLED(CONFIG_LCD_CLASS_DEVICE) struct backlight_device *of_find_backlight_by_node(struct device_node *node); #else static inline struct backlight_device * of_find_backlight_by_node(struct device_node *node) { return NULL; } #endif
This will need to be a patch of its own since it's another subsystem.
You have to try the various kconfig variations to be sure, I'm no kconfig expert. CONFIG_OF=y/n, CONFIG_DRM=y/m, CONFIG_LCD_CLASS_DEVICE=y/m/n
Noralf.
Sure will try this.
Another way of solving this is to put the function in drivers/gpu/drm/drm_backlight.c and add a DRM_BACKLIGHT konfig option that does the selection.
Noralf.
Otherwise I think this patch looks good. -Daniel
>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 >
Den 29.09.2017 16.13, skrev Meghana Madhyastha:
On Fri, Sep 29, 2017 at 02:33:12PM +0200, Noralf Trønnes wrote:
Den 29.09.2017 14.20, skrev Meghana Madhyastha:
On Fri, Sep 29, 2017 at 02:10:31PM +0200, Noralf Trønnes wrote:
Den 29.09.2017 05.22, skrev Meghana Madhyastha:
On Thu, Sep 28, 2017 at 06:02:27PM +0200, Noralf Trønnes wrote:
Den 28.09.2017 16.08, skrev Daniel Vetter: > On Thu, Sep 28, 2017 at 02:44:34PM +0530, Meghana Madhyastha wrote: >> Rename tinydrm_of_find_backlight to drm_of_find_backlight >> and move it into drm_of.c from tinydrm-helpers.c. This is >> because other drivers in the drm subsystem might need to call >> this function. In that case and otherwise, it is better from >> an organizational point of view to move it into drm_of.c along >> with the other _of.c functions. >> >> Signed-off-by: Meghana Madhyastha meghana.madhyastha@gmail.com >> --- >> Changes in v3: >> -Change it back to a single patch from two patches in v2 >> >> drivers/gpu/drm/drm_of.c | 44 ++++++++++++++++++++++++++ >> 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, 47 insertions(+), 42 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c >> index 8dafbdf..d878d3a 100644 >> --- a/drivers/gpu/drm/drm_of.c >> +++ b/drivers/gpu/drm/drm_of.c >> @@ -1,6 +1,7 @@ >> #include <linux/component.h> >> #include <linux/export.h> >> #include <linux/list.h> >> +#include <linux/backlight.h> >> #include <linux/of_graph.h> >> #include <drm/drmP.h> >> #include <drm/drm_bridge.h> >> @@ -260,3 +261,46 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, >> return ret; >> } >> EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge); >> + >> +/** >> + * drm_of_find_backlight - Find backlight device in device-tree >> + * @dev: Device >> + * >> + * This function looks for a DT node pointed to by a property named 'backlight' >> + * and uses of_find_backlight_by_node() to get the backlight device. >> + * Additionally if the brightness property is zero, it is set to >> + * max_brightness. >> + * >> + * Note: It is the responsibility of the caller to call put_device() when >> + * releasing the resource. >> + * >> + * Returns: >> + * NULL if there's no backlight property. >> + * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device >> + * is found. >> + * If the backlight device is found, a pointer to the structure is returned. >> + */ >> +struct backlight_device *drm_of_find_backlight(struct device *dev) >> +{ >> + struct backlight_device *backlight; >> + struct device_node *np; >> + >> + np = of_parse_phandle(dev->of_node, "backlight", 0); >> + if (!np) >> + return NULL; >> + >> + backlight = of_find_backlight_by_node(np); >> + of_node_put(np); >> + >> + if (!backlight) >> + return ERR_PTR(-EPROBE_DEFER); >> + >> + if (!backlight->props.brightness) { >> + backlight->props.brightness = backlight->props.max_brightness; >> + DRM_DEBUG_KMS("Backlight brightness set to %d\n", >> + backlight->props.brightness); >> + } >> + >> + return backlight; >> +} >> +EXPORT_SYMBOL(drm_of_find_backlight);
<snip>
Another problem I discovered when trying to compile this, is that if DRM is builtin and LCD_CLASS_DEVICE is a module:
drivers/gpu/drm/drm_of.c:292: undefined reference to `of_find_backlight_by_node'
I see that I solved this in tinydrm by just selecting BACKLIGHT_LCD_SUPPORT and LCD_CLASS_DEVICE. I'm not aware of a way to force LCD_CLASS_DEVICE to be builtin if DRM is builtin.
Arnd fixed a similar type of dependecy in the repaper driver by forcing repaper to be a module if necessary. To be honest, I don't know why his fix works: https://github.com/torvalds/linux/commit/8312a3fe84b96e30a010fa20f933606d976...
I didn't quite understand how we could use this fix for the current case. Isn't this specific to the repaper driver dependency ?
This is the solution Daniel mentions:
menuconfig DRM tristate "Direct Rendering Manager (XFree86 4.1.0 and higher DRI support)" depends on (AGP || AGP=n) && !EMULATED_CMPXCHG && HAS_DMA + depends on (LCD_CLASS_DEVICE || LCD_CLASS_DEVICE=n)
I had tried this but it resulted in a recursive dependencies error.
What's the error? Could this have something to do with tinydrm selecting backlight and depending on DRM?
Noralf.
Looks like a case of circular dependencies. The exact error is this:
scripts/kconfig/mconf Kconfig drivers/gpu/drm/Kconfig:7:error: recursive dependency detected! For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/gpu/drm/Kconfig:7: symbol DRM depends on LCD_CLASS_DEVICE For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/video/backlight/Kconfig:16: symbol LCD_CLASS_DEVICE is selected by FB_CLPS711X For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/video/fbdev/Kconfig:338: symbol FB_CLPS711X depends on FB For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/video/fbdev/Kconfig:5: symbol FB is selected by DRM_KMS_FB_HELPER For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/gpu/drm/Kconfig:73: symbol DRM_KMS_FB_HELPER is selected by DRM_FBDEV_EMULATION For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/gpu/drm/Kconfig:90: symbol DRM_FBDEV_EMULATION depends on DRM
Long story short, the dependency loop is as follows:
DRM -> LCD_CLASS_DEVICE -> FB_CLPS711X -> FB -> DRM_KMS_FB_HELPER -> DRM_FBDEV_EMULATION -> DRM
I have messed up the options here, it should be BACKLIGHT_CLASS_DEVICE:
menuconfig DRM depends on (BACKLIGHT_CLASS_DEVICE || BACKLIGHT_CLASS_DEVICE=n)
It doesn't help with the recusrsive problem, but at least it makes sense:
drivers/gpu/drm/Kconfig:7: symbol DRM depends on BACKLIGHT_CLASS_DEVICE drivers/video/backlight/Kconfig:158: symbol BACKLIGHT_CLASS_DEVICE is selected by DRM_RADEON drivers/gpu/drm/Kconfig:164: symbol DRM_RADEON depends on DRM
I don't know how to solve this...
Noralf.
On Fri, Sep 29, 2017 at 05:41:04PM +0200, Noralf Trønnes wrote:
Den 29.09.2017 16.13, skrev Meghana Madhyastha:
On Fri, Sep 29, 2017 at 02:33:12PM +0200, Noralf Trønnes wrote:
Den 29.09.2017 14.20, skrev Meghana Madhyastha:
On Fri, Sep 29, 2017 at 02:10:31PM +0200, Noralf Trønnes wrote:
Den 29.09.2017 05.22, skrev Meghana Madhyastha:
On Thu, Sep 28, 2017 at 06:02:27PM +0200, Noralf Trønnes wrote: >Den 28.09.2017 16.08, skrev Daniel Vetter: >>On Thu, Sep 28, 2017 at 02:44:34PM +0530, Meghana Madhyastha wrote: >>>Rename tinydrm_of_find_backlight to drm_of_find_backlight >>>and move it into drm_of.c from tinydrm-helpers.c. This is >>>because other drivers in the drm subsystem might need to call >>>this function. In that case and otherwise, it is better from >>>an organizational point of view to move it into drm_of.c along >>>with the other _of.c functions. >>> >>>Signed-off-by: Meghana Madhyastha meghana.madhyastha@gmail.com >>>--- >>>Changes in v3: >>>-Change it back to a single patch from two patches in v2 >>> >>> drivers/gpu/drm/drm_of.c | 44 ++++++++++++++++++++++++++ >>> 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, 47 insertions(+), 42 deletions(-) >>> >>>diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c >>>index 8dafbdf..d878d3a 100644 >>>--- a/drivers/gpu/drm/drm_of.c >>>+++ b/drivers/gpu/drm/drm_of.c >>>@@ -1,6 +1,7 @@ >>> #include <linux/component.h> >>> #include <linux/export.h> >>> #include <linux/list.h> >>>+#include <linux/backlight.h> >>> #include <linux/of_graph.h> >>> #include <drm/drmP.h> >>> #include <drm/drm_bridge.h> >>>@@ -260,3 +261,46 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, >>> return ret; >>> } >>> EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge); >>>+ >>>+/** >>>+ * drm_of_find_backlight - Find backlight device in device-tree >>>+ * @dev: Device >>>+ * >>>+ * This function looks for a DT node pointed to by a property named 'backlight' >>>+ * and uses of_find_backlight_by_node() to get the backlight device. >>>+ * Additionally if the brightness property is zero, it is set to >>>+ * max_brightness. >>>+ * >>>+ * Note: It is the responsibility of the caller to call put_device() when >>>+ * releasing the resource. >>>+ * >>>+ * Returns: >>>+ * NULL if there's no backlight property. >>>+ * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device >>>+ * is found. >>>+ * If the backlight device is found, a pointer to the structure is returned. >>>+ */ >>>+struct backlight_device *drm_of_find_backlight(struct device *dev) >>>+{ >>>+ struct backlight_device *backlight; >>>+ struct device_node *np; >>>+ >>>+ np = of_parse_phandle(dev->of_node, "backlight", 0); >>>+ if (!np) >>>+ return NULL; >>>+ >>>+ backlight = of_find_backlight_by_node(np); >>>+ of_node_put(np); >>>+ >>>+ if (!backlight) >>>+ return ERR_PTR(-EPROBE_DEFER); >>>+ >>>+ if (!backlight->props.brightness) { >>>+ backlight->props.brightness = backlight->props.max_brightness; >>>+ DRM_DEBUG_KMS("Backlight brightness set to %d\n", >>>+ backlight->props.brightness); >>>+ } >>>+ >>>+ return backlight; >>>+} >>>+EXPORT_SYMBOL(drm_of_find_backlight);
<snip>
>Another problem I discovered when trying to compile this, is that if >DRM is builtin and LCD_CLASS_DEVICE is a module: > >drivers/gpu/drm/drm_of.c:292: undefined reference to >`of_find_backlight_by_node' > >I see that I solved this in tinydrm by just selecting >BACKLIGHT_LCD_SUPPORT and LCD_CLASS_DEVICE. I'm not aware of a way to >force LCD_CLASS_DEVICE to be builtin if DRM is builtin. > >Arnd fixed a similar type of dependecy in the repaper driver by forcing >repaper to be a module if necessary. To be honest, I don't know why his >fix works: >https://github.com/torvalds/linux/commit/8312a3fe84b96e30a010fa20f933606d976... I didn't quite understand how we could use this fix for the current case. Isn't this specific to the repaper driver dependency ?
This is the solution Daniel mentions:
menuconfig DRM tristate "Direct Rendering Manager (XFree86 4.1.0 and higher DRI support)" depends on (AGP || AGP=n) && !EMULATED_CMPXCHG && HAS_DMA + depends on (LCD_CLASS_DEVICE || LCD_CLASS_DEVICE=n)
I had tried this but it resulted in a recursive dependencies error.
What's the error? Could this have something to do with tinydrm selecting backlight and depending on DRM?
Noralf.
Looks like a case of circular dependencies. The exact error is this:
scripts/kconfig/mconf Kconfig drivers/gpu/drm/Kconfig:7:error: recursive dependency detected! For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/gpu/drm/Kconfig:7: symbol DRM depends on LCD_CLASS_DEVICE For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/video/backlight/Kconfig:16: symbol LCD_CLASS_DEVICE is selected by FB_CLPS711X For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/video/fbdev/Kconfig:338: symbol FB_CLPS711X depends on FB For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/video/fbdev/Kconfig:5: symbol FB is selected by DRM_KMS_FB_HELPER For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/gpu/drm/Kconfig:73: symbol DRM_KMS_FB_HELPER is selected by DRM_FBDEV_EMULATION For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/gpu/drm/Kconfig:90: symbol DRM_FBDEV_EMULATION depends on DRM
Long story short, the dependency loop is as follows:
DRM -> LCD_CLASS_DEVICE -> FB_CLPS711X -> FB -> DRM_KMS_FB_HELPER -> DRM_FBDEV_EMULATION -> DRM
I have messed up the options here, it should be BACKLIGHT_CLASS_DEVICE:
menuconfig DRM depends on (BACKLIGHT_CLASS_DEVICE || BACKLIGHT_CLASS_DEVICE=n)
It doesn't help with the recusrsive problem, but at least it makes sense:
drivers/gpu/drm/Kconfig:7: symbol DRM depends on BACKLIGHT_CLASS_DEVICE drivers/video/backlight/Kconfig:158: symbol BACKLIGHT_CLASS_DEVICE is selected by DRM_RADEON drivers/gpu/drm/Kconfig:164: symbol DRM_RADEON depends on DRM
I don't know how to solve this...
Noralf.
Looks like select BACKLIGHT_CLASS_DEVICE and BACKLIGHT_LCD_SUPPORT seemed to work (atleast when I replicated the configurations from the kbuild support). Maybe that is because we are not forcing the dependencies when we do a select ?
Thanks and regards, Meghana
On Fri, Sep 29, 2017 at 05:41:04PM +0200, Noralf Trønnes wrote:
Den 29.09.2017 16.13, skrev Meghana Madhyastha:
On Fri, Sep 29, 2017 at 02:33:12PM +0200, Noralf Trønnes wrote:
Den 29.09.2017 14.20, skrev Meghana Madhyastha:
On Fri, Sep 29, 2017 at 02:10:31PM +0200, Noralf Trønnes wrote:
Den 29.09.2017 05.22, skrev Meghana Madhyastha:
On Thu, Sep 28, 2017 at 06:02:27PM +0200, Noralf Trønnes wrote: > Den 28.09.2017 16.08, skrev Daniel Vetter: > > On Thu, Sep 28, 2017 at 02:44:34PM +0530, Meghana Madhyastha wrote: > > > Rename tinydrm_of_find_backlight to drm_of_find_backlight > > > and move it into drm_of.c from tinydrm-helpers.c. This is > > > because other drivers in the drm subsystem might need to call > > > this function. In that case and otherwise, it is better from > > > an organizational point of view to move it into drm_of.c along > > > with the other _of.c functions. > > > > > > Signed-off-by: Meghana Madhyastha meghana.madhyastha@gmail.com > > > --- > > > Changes in v3: > > > -Change it back to a single patch from two patches in v2 > > > > > > drivers/gpu/drm/drm_of.c | 44 ++++++++++++++++++++++++++ > > > 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, 47 insertions(+), 42 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c > > > index 8dafbdf..d878d3a 100644 > > > --- a/drivers/gpu/drm/drm_of.c > > > +++ b/drivers/gpu/drm/drm_of.c > > > @@ -1,6 +1,7 @@ > > > #include <linux/component.h> > > > #include <linux/export.h> > > > #include <linux/list.h> > > > +#include <linux/backlight.h> > > > #include <linux/of_graph.h> > > > #include <drm/drmP.h> > > > #include <drm/drm_bridge.h> > > > @@ -260,3 +261,46 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, > > > return ret; > > > } > > > EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge); > > > + > > > +/** > > > + * drm_of_find_backlight - Find backlight device in device-tree > > > + * @dev: Device > > > + * > > > + * This function looks for a DT node pointed to by a property named 'backlight' > > > + * and uses of_find_backlight_by_node() to get the backlight device. > > > + * Additionally if the brightness property is zero, it is set to > > > + * max_brightness. > > > + * > > > + * Note: It is the responsibility of the caller to call put_device() when > > > + * releasing the resource. > > > + * > > > + * Returns: > > > + * NULL if there's no backlight property. > > > + * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device > > > + * is found. > > > + * If the backlight device is found, a pointer to the structure is returned. > > > + */ > > > +struct backlight_device *drm_of_find_backlight(struct device *dev) > > > +{ > > > + struct backlight_device *backlight; > > > + struct device_node *np; > > > + > > > + np = of_parse_phandle(dev->of_node, "backlight", 0); > > > + if (!np) > > > + return NULL; > > > + > > > + backlight = of_find_backlight_by_node(np); > > > + of_node_put(np); > > > + > > > + if (!backlight) > > > + return ERR_PTR(-EPROBE_DEFER); > > > + > > > + if (!backlight->props.brightness) { > > > + backlight->props.brightness = backlight->props.max_brightness; > > > + DRM_DEBUG_KMS("Backlight brightness set to %d\n", > > > + backlight->props.brightness); > > > + } > > > + > > > + return backlight; > > > +} > > > +EXPORT_SYMBOL(drm_of_find_backlight);
<snip>
> Another problem I discovered when trying to compile this, is that if > DRM is builtin and LCD_CLASS_DEVICE is a module: > > drivers/gpu/drm/drm_of.c:292: undefined reference to > `of_find_backlight_by_node' > > I see that I solved this in tinydrm by just selecting > BACKLIGHT_LCD_SUPPORT and LCD_CLASS_DEVICE. I'm not aware of a way to > force LCD_CLASS_DEVICE to be builtin if DRM is builtin. > > Arnd fixed a similar type of dependecy in the repaper driver by forcing > repaper to be a module if necessary. To be honest, I don't know why his > fix works: > https://github.com/torvalds/linux/commit/8312a3fe84b96e30a010fa20f933606d976... I didn't quite understand how we could use this fix for the current case. Isn't this specific to the repaper driver dependency ?
This is the solution Daniel mentions:
menuconfig DRM tristate "Direct Rendering Manager (XFree86 4.1.0 and higher DRI support)" depends on (AGP || AGP=n) && !EMULATED_CMPXCHG && HAS_DMA + depends on (LCD_CLASS_DEVICE || LCD_CLASS_DEVICE=n)
I had tried this but it resulted in a recursive dependencies error.
What's the error? Could this have something to do with tinydrm selecting backlight and depending on DRM?
Noralf.
Looks like a case of circular dependencies. The exact error is this:
scripts/kconfig/mconf Kconfig drivers/gpu/drm/Kconfig:7:error: recursive dependency detected! For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/gpu/drm/Kconfig:7: symbol DRM depends on LCD_CLASS_DEVICE For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/video/backlight/Kconfig:16: symbol LCD_CLASS_DEVICE is selected by FB_CLPS711X For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/video/fbdev/Kconfig:338: symbol FB_CLPS711X depends on FB For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/video/fbdev/Kconfig:5: symbol FB is selected by DRM_KMS_FB_HELPER For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/gpu/drm/Kconfig:73: symbol DRM_KMS_FB_HELPER is selected by DRM_FBDEV_EMULATION For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/gpu/drm/Kconfig:90: symbol DRM_FBDEV_EMULATION depends on DRM
Long story short, the dependency loop is as follows:
DRM -> LCD_CLASS_DEVICE -> FB_CLPS711X -> FB -> DRM_KMS_FB_HELPER -> DRM_FBDEV_EMULATION -> DRM
I have messed up the options here, it should be BACKLIGHT_CLASS_DEVICE:
menuconfig DRM depends on (BACKLIGHT_CLASS_DEVICE || BACKLIGHT_CLASS_DEVICE=n)
It doesn't help with the recusrsive problem, but at least it makes sense:
drivers/gpu/drm/Kconfig:7: symbol DRM depends on BACKLIGHT_CLASS_DEVICE drivers/video/backlight/Kconfig:158: symbol BACKLIGHT_CLASS_DEVICE is selected by DRM_RADEON drivers/gpu/drm/Kconfig:164: symbol DRM_RADEON depends on DRM
I don't know how to solve this...
You can't mix depends and select on the same symbol. More or less. So either all depends or all selects, but select only works well if the option is a top-level one (otherwise you also need to select _all_ its dependencies).
And yes backlight is known to be an especially bad horror show in Kconfig, unfortunately no one yet managed to clean up the mess :-/
So correct fix is to check out what all the other drivers do and then do the same (and hope there's a clear majority).
Also adding Jani, who looked at the backlight Kconfig mess in the past. -Daniel
On Mon, 02 Oct 2017, Daniel Vetter daniel@ffwll.ch wrote:
Also adding Jani, who looked at the backlight Kconfig mess in the past.
I've even sent patches to fix some of the dependency mess, but the problem is social not technical. The problem is that people think "select" is more convenient than "depends" because they can just enable a config that way, while "depends" would require finding and enabling all the dependencies before the menu option even shows up.
I don't deny, that's annoying. But it's also abuse of select, see Documentation/kbuild/kconfig-language.txt:
Note: select should be used with care. select will force a symbol to a value without visiting the dependencies. By abusing select you are able to select a symbol FOO even if FOO depends on BAR that is not set. In general use select only for non-visible symbols (no prompts anywhere) and for symbols with no dependencies. That will limit the usefulness but on the other hand avoid the illegal configurations all over.
The real fix would be making finding and enabling dependencies recursively more convenient, but I don't see that happening anytime soon.
BR, Jani.
Den 02.10.2017 10.13, skrev Jani Nikula:
On Mon, 02 Oct 2017, Daniel Vetter daniel@ffwll.ch wrote:
Also adding Jani, who looked at the backlight Kconfig mess in the past.
I've even sent patches to fix some of the dependency mess, but the problem is social not technical. The problem is that people think "select" is more convenient than "depends" because they can just enable a config that way, while "depends" would require finding and enabling all the dependencies before the menu option even shows up.
I don't deny, that's annoying. But it's also abuse of select, see Documentation/kbuild/kconfig-language.txt:
Note: select should be used with care. select will force a symbol to a value without visiting the dependencies. By abusing select you are able to select a symbol FOO even if FOO depends on BAR that is not set. In general use select only for non-visible symbols (no prompts anywhere) and for symbols with no dependencies. That will limit the usefulness but on the other hand avoid the illegal configurations all over.
The real fix would be making finding and enabling dependencies recursively more convenient, but I don't see that happening anytime soon.
If we don't select BACKLIGHT in drivers, we can end up with CONFIG_DRM=y and CONFIG_BACKLIGHT_CLASS_DEVICE=m.
So we can either do:
menuconfig DRM depends on (BACKLIGHT_CLASS_DEVICE || BACKLIGHT_CLASS_DEVICE=n)
Or we can:
-#ifdef CONFIG_OF +#if IS_ENABLED(CONFIG_OF) && IS_REACHABLE(CONFIG_BACKLIGHT_CLASS_DEVICE) struct backlight_device *of_find_backlight_by_node(struct device_node *node); #else static inline struct backlight_device *
Using the second one it won't be obvious to users why backlight doesn't work, they have after all enabled backlight (but as module and DRM builtin).
So I suppose the first one is preferred. What effect will such a change have on an existing configuration where DRM=y and BACKLIGHT_CLASS_DEVICE=m? Will DRM become a module or will it be disabled?
Noralf.
Den 03.10.2017 16.58, skrev Noralf Trønnes:
Den 02.10.2017 10.13, skrev Jani Nikula:
On Mon, 02 Oct 2017, Daniel Vetter daniel@ffwll.ch wrote:
Also adding Jani, who looked at the backlight Kconfig mess in the past.
I've even sent patches to fix some of the dependency mess, but the problem is social not technical. The problem is that people think "select" is more convenient than "depends" because they can just enable a config that way, while "depends" would require finding and enabling all the dependencies before the menu option even shows up.
I don't deny, that's annoying. But it's also abuse of select, see Documentation/kbuild/kconfig-language.txt:
Note: select should be used with care. select will force a symbol to a value without visiting the dependencies. By abusing select you are able to select a symbol FOO even if FOO depends on BAR that is not set. In general use select only for non-visible symbols (no prompts anywhere) and for symbols with no dependencies. That will limit the usefulness but on the other hand avoid the illegal configurations all over.
The real fix would be making finding and enabling dependencies recursively more convenient, but I don't see that happening anytime soon.
If we don't select BACKLIGHT in drivers, we can end up with CONFIG_DRM=y and CONFIG_BACKLIGHT_CLASS_DEVICE=m.
So we can either do:
menuconfig DRM depends on (BACKLIGHT_CLASS_DEVICE || BACKLIGHT_CLASS_DEVICE=n)
No, this was the thing that didn't work:
drivers/gpu/drm/Kconfig:7: symbol DRM depends on BACKLIGHT_CLASS_DEVICE drivers/video/backlight/Kconfig:158: symbol BACKLIGHT_CLASS_DEVICE is selected by DRM_RADEON drivers/gpu/drm/Kconfig:164: symbol DRM_RADEON depends on DRM
Or we can:
-#ifdef CONFIG_OF +#if IS_ENABLED(CONFIG_OF) && IS_REACHABLE(CONFIG_BACKLIGHT_CLASS_DEVICE) struct backlight_device *of_find_backlight_by_node(struct device_node *node); #else static inline struct backlight_device *
Using the second one it won't be obvious to users why backlight doesn't work, they have after all enabled backlight (but as module and DRM builtin).
So I suppose the first one is preferred. What effect will such a change have on an existing configuration where DRM=y and BACKLIGHT_CLASS_DEVICE=m? Will DRM become a module or will it be disabled?
Noralf.
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Den 03.10.2017 18.02, skrev Noralf Trønnes:
Den 03.10.2017 16.58, skrev Noralf Trønnes:
Den 02.10.2017 10.13, skrev Jani Nikula:
On Mon, 02 Oct 2017, Daniel Vetter daniel@ffwll.ch wrote:
Also adding Jani, who looked at the backlight Kconfig mess in the past.
I've even sent patches to fix some of the dependency mess, but the problem is social not technical. The problem is that people think "select" is more convenient than "depends" because they can just enable a config that way, while "depends" would require finding and enabling all the dependencies before the menu option even shows up.
I don't deny, that's annoying. But it's also abuse of select, see Documentation/kbuild/kconfig-language.txt:
Note: select should be used with care. select will force a symbol to a value without visiting the dependencies. By abusing select you are able to select a symbol FOO even if FOO depends on BAR that is not set. In general use select only for non-visible symbols (no prompts anywhere) and for symbols with no dependencies. That will limit the usefulness but on the other hand avoid the illegal configurations all over.
The real fix would be making finding and enabling dependencies recursively more convenient, but I don't see that happening anytime soon.
If we don't select BACKLIGHT in drivers, we can end up with CONFIG_DRM=y and CONFIG_BACKLIGHT_CLASS_DEVICE=m.
So we can either do:
menuconfig DRM depends on (BACKLIGHT_CLASS_DEVICE || BACKLIGHT_CLASS_DEVICE=n)
No, this was the thing that didn't work:
drivers/gpu/drm/Kconfig:7: symbol DRM depends on BACKLIGHT_CLASS_DEVICE drivers/video/backlight/Kconfig:158: symbol BACKLIGHT_CLASS_DEVICE is selected by DRM_RADEON drivers/gpu/drm/Kconfig:164: symbol DRM_RADEON depends on DRM
How about putting the dependency in the driver combined with IS_REACHABLE, but not in backlight.h as I first proposed, that won't work:
menuconfig DRM_TINYDRM - select BACKLIGHT_LCD_SUPPORT - select BACKLIGHT_CLASS_DEVICE + depends on (BACKLIGHT_CLASS_DEVICE || BACKLIGHT_CLASS_DEVICE=n)
struct backlight_device *drm_of_find_backlight(struct device *dev) { struct backlight_device *backlight; struct device_node *np;
if (!IS_REACHABLE(CONFIG_BACKLIGHT_CLASS_DEVICE)) return NULL; [...] }
But the IS_REACHABLE doesn't feel right.
Maybe the problem is putting a function in DRM that really doesn't belong there. How about calling it of_find_backlight() instead and put it in backlight.[hc]?
Noralf.
Or we can:
-#ifdef CONFIG_OF +#if IS_ENABLED(CONFIG_OF) && IS_REACHABLE(CONFIG_BACKLIGHT_CLASS_DEVICE) struct backlight_device *of_find_backlight_by_node(struct device_node *node); #else static inline struct backlight_device *
Using the second one it won't be obvious to users why backlight doesn't work, they have after all enabled backlight (but as module and DRM builtin).
So I suppose the first one is preferred. What effect will such a change have on an existing configuration where DRM=y and BACKLIGHT_CLASS_DEVICE=m? Will DRM become a module or will it be disabled?
Noralf.
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Oct 03, 2017 at 07:38:29PM +0200, Noralf Trønnes wrote:
Den 03.10.2017 18.02, skrev Noralf Trønnes:
Den 03.10.2017 16.58, skrev Noralf Trønnes:
Den 02.10.2017 10.13, skrev Jani Nikula:
On Mon, 02 Oct 2017, Daniel Vetter daniel@ffwll.ch wrote:
Also adding Jani, who looked at the backlight Kconfig mess in the past.
I've even sent patches to fix some of the dependency mess, but the problem is social not technical. The problem is that people think "select" is more convenient than "depends" because they can just enable a config that way, while "depends" would require finding and enabling all the dependencies before the menu option even shows up.
I don't deny, that's annoying. But it's also abuse of select, see Documentation/kbuild/kconfig-language.txt:
Note: select should be used with care. select will force a symbol to a value without visiting the dependencies. By abusing select you are able to select a symbol FOO even if FOO depends on BAR that is not set. In general use select only for non-visible symbols (no prompts anywhere) and for symbols with no dependencies. That will limit the usefulness but on the other hand avoid the illegal configurations all over.
The real fix would be making finding and enabling dependencies recursively more convenient, but I don't see that happening anytime soon.
If we don't select BACKLIGHT in drivers, we can end up with CONFIG_DRM=y and CONFIG_BACKLIGHT_CLASS_DEVICE=m.
So we can either do:
menuconfig DRM depends on (BACKLIGHT_CLASS_DEVICE || BACKLIGHT_CLASS_DEVICE=n)
No, this was the thing that didn't work:
drivers/gpu/drm/Kconfig:7: symbol DRM depends on BACKLIGHT_CLASS_DEVICE drivers/video/backlight/Kconfig:158: symbol BACKLIGHT_CLASS_DEVICE is selected by DRM_RADEON drivers/gpu/drm/Kconfig:164: symbol DRM_RADEON depends on DRM
How about putting the dependency in the driver combined with IS_REACHABLE, but not in backlight.h as I first proposed, that won't work:
menuconfig DRM_TINYDRM - select BACKLIGHT_LCD_SUPPORT - select BACKLIGHT_CLASS_DEVICE + depends on (BACKLIGHT_CLASS_DEVICE || BACKLIGHT_CLASS_DEVICE=n)
struct backlight_device *drm_of_find_backlight(struct device *dev) { struct backlight_device *backlight; struct device_node *np;
if (!IS_REACHABLE(CONFIG_BACKLIGHT_CLASS_DEVICE)) return NULL; [...] }
But the IS_REACHABLE doesn't feel right.
Maybe the problem is putting a function in DRM that really doesn't belong there. How about calling it of_find_backlight() instead and put it in backlight.[hc]?
Wouldn't it make more sense to put the function in DRM ? Because all of the callers are located in DRM and so I'd think it makes more sense to put it in DRM along with the other _of functions.
Noralf.
Or we can:
-#ifdef CONFIG_OF +#if IS_ENABLED(CONFIG_OF) && IS_REACHABLE(CONFIG_BACKLIGHT_CLASS_DEVICE) struct backlight_device *of_find_backlight_by_node(struct device_node *node); #else static inline struct backlight_device *
I have one question. Why isn't your previous solution a good solution? i.e IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE) which was part of v7
Using the second one it won't be obvious to users why backlight doesn't work, they have after all enabled backlight (but as module and DRM builtin).
So I suppose the first one is preferred. What effect will such a change have on an existing configuration where DRM=y and BACKLIGHT_CLASS_DEVICE=m? Will DRM become a module or will it be disabled?
Why would it be disabled in this case ?
Thanks and regards, Meghana
Noralf.
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Den 05.10.2017 05.53, skrev Meghana Madhyastha:
On Tue, Oct 03, 2017 at 07:38:29PM +0200, Noralf Trønnes wrote:
Den 03.10.2017 18.02, skrev Noralf Trønnes:
Den 03.10.2017 16.58, skrev Noralf Trønnes:
Den 02.10.2017 10.13, skrev Jani Nikula:
On Mon, 02 Oct 2017, Daniel Vetter daniel@ffwll.ch wrote:
Also adding Jani, who looked at the backlight Kconfig mess in the past.
I've even sent patches to fix some of the dependency mess, but the problem is social not technical. The problem is that people think "select" is more convenient than "depends" because they can just enable a config that way, while "depends" would require finding and enabling all the dependencies before the menu option even shows up.
I don't deny, that's annoying. But it's also abuse of select, see Documentation/kbuild/kconfig-language.txt:
Note: select should be used with care. select will force a symbol to a value without visiting the dependencies. By abusing select you are able to select a symbol FOO even if FOO depends on BAR that is not set. In general use select only for non-visible symbols (no prompts anywhere) and for symbols with no dependencies. That will limit the usefulness but on the other hand avoid the illegal configurations all over.
The real fix would be making finding and enabling dependencies recursively more convenient, but I don't see that happening anytime soon.
If we don't select BACKLIGHT in drivers, we can end up with CONFIG_DRM=y and CONFIG_BACKLIGHT_CLASS_DEVICE=m.
So we can either do:
menuconfig DRM depends on (BACKLIGHT_CLASS_DEVICE || BACKLIGHT_CLASS_DEVICE=n)
No, this was the thing that didn't work:
drivers/gpu/drm/Kconfig:7: symbol DRM depends on BACKLIGHT_CLASS_DEVICE drivers/video/backlight/Kconfig:158: symbol BACKLIGHT_CLASS_DEVICE is selected by DRM_RADEON drivers/gpu/drm/Kconfig:164: symbol DRM_RADEON depends on DRM
How about putting the dependency in the driver combined with IS_REACHABLE, but not in backlight.h as I first proposed, that won't work:
menuconfig DRM_TINYDRM - select BACKLIGHT_LCD_SUPPORT - select BACKLIGHT_CLASS_DEVICE + depends on (BACKLIGHT_CLASS_DEVICE || BACKLIGHT_CLASS_DEVICE=n)
struct backlight_device *drm_of_find_backlight(struct device *dev) { struct backlight_device *backlight; struct device_node *np;
if (!IS_REACHABLE(CONFIG_BACKLIGHT_CLASS_DEVICE)) return NULL; [...] }
But the IS_REACHABLE doesn't feel right.
Maybe the problem is putting a function in DRM that really doesn't belong there. How about calling it of_find_backlight() instead and put it in backlight.[hc]?
Wouldn't it make more sense to put the function in DRM ? Because all of the callers are located in DRM and so I'd think it makes more sense to put it in DRM along with the other _of functions.
I don't think it matters where the callers are, but the fact that the functions are all about backlight and nothing about DRM, except the error messages.
I've made a suggestion in the last version where we have backlight maintainer attention.
Noralf.
Noralf.
Or we can:
-#ifdef CONFIG_OF +#if IS_ENABLED(CONFIG_OF) && IS_REACHABLE(CONFIG_BACKLIGHT_CLASS_DEVICE) struct backlight_device *of_find_backlight_by_node(struct device_node *node); #else static inline struct backlight_device *
I have one question. Why isn't your previous solution a good solution? i.e IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE) which was part of v7
Using the second one it won't be obvious to users why backlight doesn't work, they have after all enabled backlight (but as module and DRM builtin).
So I suppose the first one is preferred. What effect will such a change have on an existing configuration where DRM=y and BACKLIGHT_CLASS_DEVICE=m? Will DRM become a module or will it be disabled?
Why would it be disabled in this case ?
Thanks and regards, Meghana
Noralf.
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Sep 28, 2017 at 06:02:27PM +0200, Noralf Trønnes wrote:
Den 28.09.2017 16.08, skrev Daniel Vetter:
On Thu, Sep 28, 2017 at 02:44:34PM +0530, Meghana Madhyastha wrote:
Rename tinydrm_of_find_backlight to drm_of_find_backlight and move it into drm_of.c from tinydrm-helpers.c. This is because other drivers in the drm subsystem might need to call this function. In that case and otherwise, it is better from an organizational point of view to move it into drm_of.c along with the other _of.c functions.
Signed-off-by: Meghana Madhyastha meghana.madhyastha@gmail.com
Changes in v3: -Change it back to a single patch from two patches in v2
drivers/gpu/drm/drm_of.c | 44 ++++++++++++++++++++++++++ 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, 47 insertions(+), 42 deletions(-)
diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c index 8dafbdf..d878d3a 100644 --- a/drivers/gpu/drm/drm_of.c +++ b/drivers/gpu/drm/drm_of.c @@ -1,6 +1,7 @@ #include <linux/component.h> #include <linux/export.h> #include <linux/list.h> +#include <linux/backlight.h> #include <linux/of_graph.h> #include <drm/drmP.h> #include <drm/drm_bridge.h> @@ -260,3 +261,46 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, return ret; } EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge);
+/**
- drm_of_find_backlight - Find backlight device in device-tree
- @dev: Device
- This function looks for a DT node pointed to by a property named 'backlight'
- and uses of_find_backlight_by_node() to get the backlight device.
- Additionally if the brightness property is zero, it is set to
- max_brightness.
- Note: It is the responsibility of the caller to call put_device() when
- releasing the resource.
- Returns:
- NULL if there's no backlight property.
- Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
- is found.
- If the backlight device is found, a pointer to the structure is returned.
- */
+struct backlight_device *drm_of_find_backlight(struct device *dev) +{
- struct backlight_device *backlight;
- struct device_node *np;
- np = of_parse_phandle(dev->of_node, "backlight", 0);
- if (!np)
return NULL;- backlight = of_find_backlight_by_node(np);
- of_node_put(np);
- if (!backlight)
return ERR_PTR(-EPROBE_DEFER);- if (!backlight->props.brightness) {
backlight->props.brightness = backlight->props.max_brightness;DRM_DEBUG_KMS("Backlight brightness set to %d\n",backlight->props.brightness);- }
- return backlight;
+} +EXPORT_SYMBOL(drm_of_find_backlight); diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c index bd6cce0..cd4c6a5 100644 --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c @@ -237,46 +237,6 @@ void tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb, EXPORT_SYMBOL(tinydrm_xrgb8888_to_gray8); /**
- tinydrm_of_find_backlight - Find backlight device in device-tree
- @dev: Device
- This function looks for a DT node pointed to by a property named 'backlight'
- and uses of_find_backlight_by_node() to get the backlight device.
- Additionally if the brightness property is zero, it is set to
- max_brightness.
- Returns:
- NULL if there's no backlight property.
- Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
- is found.
- If the backlight device is found, a pointer to the structure is returned.
- */
-struct backlight_device *tinydrm_of_find_backlight(struct device *dev) -{
- struct backlight_device *backlight;
- struct device_node *np;
- np = of_parse_phandle(dev->of_node, "backlight", 0);
- if (!np)
return NULL;- backlight = of_find_backlight_by_node(np);
- of_node_put(np);
- if (!backlight)
return ERR_PTR(-EPROBE_DEFER);- if (!backlight->props.brightness) {
backlight->props.brightness = backlight->props.max_brightness;DRM_DEBUG_KMS("Backlight brightness set to %d\n",backlight->props.brightness);- }
- return backlight;
-} -EXPORT_SYMBOL(tinydrm_of_find_backlight);
-/**
- tinydrm_enable_backlight - Enable backlight helper
- @backlight: Backlight device
diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c index 7e5bb7d..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)
Minor detail I missed: The dummy version for the #else here is missing. Not a problem for tinydrm, but might be an issue for a driver where CONFIG_OF is optional.
Yeah, we need a dummy version. tinydrm can in theory be used on non-DT platforms. The only resource that isn't available there is backlight since there is no platform or ACPI way of describing that dependency.
Another problem I discovered when trying to compile this, is that if DRM is builtin and LCD_CLASS_DEVICE is a module:
drivers/gpu/drm/drm_of.c:292: undefined reference to `of_find_backlight_by_node'
I see that I solved this in tinydrm by just selecting BACKLIGHT_LCD_SUPPORT and LCD_CLASS_DEVICE. I'm not aware of a way to force LCD_CLASS_DEVICE to be builtin if DRM is builtin.
The usual way to force such an optional depency is
depends on (FOO || FOO=n)
That way it's still optional, but if FOO is enabled as a module, you can't enable the current symbol as built-in. See e.g. how DRM vs. AGP is handled. -Daniel
Arnd fixed a similar type of dependecy in the repaper driver by forcing repaper to be a module if necessary. To be honest, I don't know why his fix works: https://github.com/torvalds/linux/commit/8312a3fe84b96e30a010fa20f933606d976...
Another way of solving this is to put the function in drivers/gpu/drm/drm_backlight.c and add a DRM_BACKLIGHT konfig option that does the selection.
Noralf.
Otherwise I think this patch looks good. -Daniel
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
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-drm_... base: git://people.freedesktop.org/~airlied/linux.git drm-next config: arm-sunxi_defconfig (attached as .config) compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=arm
All errors (new ones prefixed by >>):
drivers/gpu/drm/drm_of.o: In function `drm_of_find_backlight':
drm_of.c:(.text+0x3bc): undefined reference to `of_find_backlight_by_node'
--- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
Add devm_drm_of_find_backlight and the corresponding release function because some drivers such as tinydrm use devres versions of functions for requiring device resources.
Signed-off-by: Meghana Madhyastha meghana.madhyastha@gmail.com --- Changes in v3: -None
drivers/gpu/drm/drm_of.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ include/drm/drm_of.h | 2 ++ 2 files changed, 50 insertions(+)
diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c index d878d3a..238e8e5 100644 --- a/drivers/gpu/drm/drm_of.c +++ b/drivers/gpu/drm/drm_of.c @@ -304,3 +304,51 @@ struct backlight_device *drm_of_find_backlight(struct device *dev) return backlight; } EXPORT_SYMBOL(drm_of_find_backlight); + +/** + * devm_drm_of_find_backlight_release - Release backlight device + * @dev: Device + * + * This is the release function corresponding to the devm_drm_of_find_backlight. + * Each devres entry is associated with a release function. + */ +static void devm_drm_of_find_backlight_release(void *data) +{ + put_device(data); +} +EXPORT_SYMBOL(devm_drm_of_find_backlight_release); + +/** + * devm_drm_of_find_backlight - Find backlight device in device-tree + * devres version of the function + * @dev: Device + * + * This is the devres version of the function drm_of_find_backlight. + * Some drivers such as tinydrm use devres versions of functions for + * requiring device resources. + * + * Returns: + * NULL if there's no backlight property. + * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device + * is found. + * If the backlight device is found, a pointer to the structure is returned. + */ +struct backlight_device *devm_drm_of_find_backlight(struct device *dev) +{ + struct backlight_device *backlight; + int ret; + + backlight = drm_of_find_backlight(dev); + if (IS_ERR_OR_NULL(backlight)) + return backlight; + + ret = devm_add_action(dev, devm_drm_of_find_backlight_release, + &backlight->dev); + if (ret) { + put_device(&backlight->dev); + return ERR_PTR(ret); + } + + return backlight; +} +EXPORT_SYMBOL(devm_drm_of_find_backlight); diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h index e8fba5b..071fb3b 100644 --- a/include/drm/drm_of.h +++ b/include/drm/drm_of.h @@ -30,7 +30,9 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, struct drm_panel **panel, struct drm_bridge **bridge); struct backlight_device *drm_of_find_backlight(struct device *dev); +struct backlight_device *devm_drm_of_find_backlight(struct device *dev); #else +static void devm_drm_of_find_backlight_release(void *data); static inline uint32_t drm_of_find_possible_crtcs(struct drm_device *dev, struct device_node *port) {
Den 28.09.2017 11.15, skrev Meghana Madhyastha:
Add devm_drm_of_find_backlight and the corresponding release function because some drivers such as tinydrm use devres versions of functions for requiring device resources.
Signed-off-by: Meghana Madhyastha meghana.madhyastha@gmail.com
Changes in v3: -None
drivers/gpu/drm/drm_of.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ include/drm/drm_of.h | 2 ++ 2 files changed, 50 insertions(+)
diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c index d878d3a..238e8e5 100644 --- a/drivers/gpu/drm/drm_of.c +++ b/drivers/gpu/drm/drm_of.c @@ -304,3 +304,51 @@ struct backlight_device *drm_of_find_backlight(struct device *dev) return backlight; } EXPORT_SYMBOL(drm_of_find_backlight);
+/**
- devm_drm_of_find_backlight_release - Release backlight device
- @dev: Device
- This is the release function corresponding to the devm_drm_of_find_backlight.
- Each devres entry is associated with a release function.
- */
This is an internal function so no need for docs or exporting. I know that some devm_ functions have explicit release functions, but I don't think this is necessary since those users can just use drm_of_find_backlight() directly instead.
+static void devm_drm_of_find_backlight_release(void *data) +{
- put_device(data);
+} +EXPORT_SYMBOL(devm_drm_of_find_backlight_release);
+/**
- devm_drm_of_find_backlight - Find backlight device in device-tree
- devres version of the function
- @dev: Device
- This is the devres version of the function drm_of_find_backlight.
- Some drivers such as tinydrm use devres versions of functions for
No need to mention tinydrm here.
- requiring device resources.
- Returns:
- NULL if there's no backlight property.
- Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
- is found.
- If the backlight device is found, a pointer to the structure is returned.
- */
+struct backlight_device *devm_drm_of_find_backlight(struct device *dev) +{
- struct backlight_device *backlight;
- int ret;
- backlight = drm_of_find_backlight(dev);
- if (IS_ERR_OR_NULL(backlight))
return backlight;- ret = devm_add_action(dev, devm_drm_of_find_backlight_release,
&backlight->dev);- if (ret) {
put_device(&backlight->dev);return ERR_PTR(ret);- }
- return backlight;
+} +EXPORT_SYMBOL(devm_drm_of_find_backlight); diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h index e8fba5b..071fb3b 100644 --- a/include/drm/drm_of.h +++ b/include/drm/drm_of.h @@ -30,7 +30,9 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, struct drm_panel **panel, struct drm_bridge **bridge); struct backlight_device *drm_of_find_backlight(struct device *dev); +struct backlight_device *devm_drm_of_find_backlight(struct device *dev); #else
We need a dummy version of devm_drm_of_find_backlight() here that returns NULL as in the previous patch.
+static void devm_drm_of_find_backlight_release(void *data);
And this isn't needed as explained above.
static inline uint32_t drm_of_find_possible_crtcs(struct drm_device *dev, struct device_node *port) {
I'd appreciate if you could also switch mi0283qt over to this helper :-)
Noralf.
On Thu, Sep 28, 2017 at 06:19:35PM +0200, Noralf Trønnes wrote:
Den 28.09.2017 11.15, skrev Meghana Madhyastha:
Add devm_drm_of_find_backlight and the corresponding release function because some drivers such as tinydrm use devres versions of functions for requiring device resources.
Signed-off-by: Meghana Madhyastha meghana.madhyastha@gmail.com
Changes in v3: -None
drivers/gpu/drm/drm_of.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ include/drm/drm_of.h | 2 ++ 2 files changed, 50 insertions(+)
diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c index d878d3a..238e8e5 100644 --- a/drivers/gpu/drm/drm_of.c +++ b/drivers/gpu/drm/drm_of.c @@ -304,3 +304,51 @@ struct backlight_device *drm_of_find_backlight(struct device *dev) return backlight; } EXPORT_SYMBOL(drm_of_find_backlight);
+/**
- devm_drm_of_find_backlight_release - Release backlight device
- @dev: Device
- This is the release function corresponding to the devm_drm_of_find_backlight.
- Each devres entry is associated with a release function.
- */
This is an internal function so no need for docs or exporting. I know that some devm_ functions have explicit release functions, but I don't think this is necessary since those users can just use drm_of_find_backlight() directly instead.
I have a question here. devm_drm_of_find_backlight_release is a wrapper around put_device which is passed as a parameter to devm_add_action in devm_drm_of_find_backlight. So isn't the function useful here ?
+static void devm_drm_of_find_backlight_release(void *data) +{
- put_device(data);
+} +EXPORT_SYMBOL(devm_drm_of_find_backlight_release);
+/**
- devm_drm_of_find_backlight - Find backlight device in device-tree
- devres version of the function
- @dev: Device
- This is the devres version of the function drm_of_find_backlight.
- Some drivers such as tinydrm use devres versions of functions for
No need to mention tinydrm here.
- requiring device resources.
- Returns:
- NULL if there's no backlight property.
- Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
- is found.
- If the backlight device is found, a pointer to the structure is returned.
- */
+struct backlight_device *devm_drm_of_find_backlight(struct device *dev) +{
- struct backlight_device *backlight;
- int ret;
- backlight = drm_of_find_backlight(dev);
- if (IS_ERR_OR_NULL(backlight))
return backlight;- ret = devm_add_action(dev, devm_drm_of_find_backlight_release,
&backlight->dev);- if (ret) {
put_device(&backlight->dev);return ERR_PTR(ret);- }
- return backlight;
+} +EXPORT_SYMBOL(devm_drm_of_find_backlight); diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h index e8fba5b..071fb3b 100644 --- a/include/drm/drm_of.h +++ b/include/drm/drm_of.h @@ -30,7 +30,9 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, struct drm_panel **panel, struct drm_bridge **bridge); struct backlight_device *drm_of_find_backlight(struct device *dev); +struct backlight_device *devm_drm_of_find_backlight(struct device *dev); #else
We need a dummy version of devm_drm_of_find_backlight() here that returns NULL as in the previous patch.
+static void devm_drm_of_find_backlight_release(void *data);
And this isn't needed as explained above.
static inline uint32_t drm_of_find_possible_crtcs(struct drm_device *dev, struct device_node *port) {
I'd appreciate if you could also switch mi0283qt over to this helper :-)
Noralf.
Den 29.09.2017 05.17, skrev Meghana Madhyastha:
On Thu, Sep 28, 2017 at 06:19:35PM +0200, Noralf Trønnes wrote:
Den 28.09.2017 11.15, skrev Meghana Madhyastha:
Add devm_drm_of_find_backlight and the corresponding release function because some drivers such as tinydrm use devres versions of functions for requiring device resources.
Signed-off-by: Meghana Madhyastha meghana.madhyastha@gmail.com
Changes in v3: -None
drivers/gpu/drm/drm_of.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ include/drm/drm_of.h | 2 ++ 2 files changed, 50 insertions(+)
diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c index d878d3a..238e8e5 100644 --- a/drivers/gpu/drm/drm_of.c +++ b/drivers/gpu/drm/drm_of.c @@ -304,3 +304,51 @@ struct backlight_device *drm_of_find_backlight(struct device *dev) return backlight; } EXPORT_SYMBOL(drm_of_find_backlight);
+/**
- devm_drm_of_find_backlight_release - Release backlight device
- @dev: Device
- This is the release function corresponding to the devm_drm_of_find_backlight.
- Each devres entry is associated with a release function.
- */
This is an internal function so no need for docs or exporting. I know that some devm_ functions have explicit release functions, but I don't think this is necessary since those users can just use drm_of_find_backlight() directly instead.
I have a question here. devm_drm_of_find_backlight_release is a wrapper around put_device which is passed as a parameter to devm_add_action in devm_drm_of_find_backlight. So isn't the function useful here ?
We need the function for devm_add_action(), but no one outside of this file needs it. Hence the static definition.
We can't use put_device directly: ret = devm_add_action(dev, put_device, &backlight->dev); because devm_add_action expects an argument like this: void (*action)(void *) and put_device is: void put_device(struct device *dev) They differ in their argument type: void * vs. struct device * The compiler would complain.
So we need a wrapper.
+static void devm_drm_of_find_backlight_release(void *data) +{
- put_device(data);
+} +EXPORT_SYMBOL(devm_drm_of_find_backlight_release);
+/**
- devm_drm_of_find_backlight - Find backlight device in device-tree
- devres version of the function
- @dev: Device
- This is the devres version of the function drm_of_find_backlight.
- Some drivers such as tinydrm use devres versions of functions for
No need to mention tinydrm here.
- requiring device resources.
- Returns:
- NULL if there's no backlight property.
- Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
- is found.
- If the backlight device is found, a pointer to the structure is returned.
- */
+struct backlight_device *devm_drm_of_find_backlight(struct device *dev) +{
- struct backlight_device *backlight;
- int ret;
- backlight = drm_of_find_backlight(dev);
- if (IS_ERR_OR_NULL(backlight))
return backlight;- ret = devm_add_action(dev, devm_drm_of_find_backlight_release,
&backlight->dev);- if (ret) {
put_device(&backlight->dev);return ERR_PTR(ret);- }
- return backlight;
+} +EXPORT_SYMBOL(devm_drm_of_find_backlight); diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h index e8fba5b..071fb3b 100644 --- a/include/drm/drm_of.h +++ b/include/drm/drm_of.h @@ -30,7 +30,9 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, struct drm_panel **panel, struct drm_bridge **bridge); struct backlight_device *drm_of_find_backlight(struct device *dev); +struct backlight_device *devm_drm_of_find_backlight(struct device *dev); #else
We need a dummy version of devm_drm_of_find_backlight() here that returns NULL as in the previous patch.
+static void devm_drm_of_find_backlight_release(void *data);
And this isn't needed as explained above.
static inline uint32_t drm_of_find_possible_crtcs(struct drm_device *dev, struct device_node *port) {
I'd appreciate if you could also switch mi0283qt over to this helper :-)
Noralf.
On Fri, Sep 29, 2017 at 02:20:16PM +0200, Noralf Trønnes wrote:
Den 29.09.2017 05.17, skrev Meghana Madhyastha:
On Thu, Sep 28, 2017 at 06:19:35PM +0200, Noralf Trønnes wrote:
Den 28.09.2017 11.15, skrev Meghana Madhyastha:
Add devm_drm_of_find_backlight and the corresponding release function because some drivers such as tinydrm use devres versions of functions for requiring device resources.
Signed-off-by: Meghana Madhyastha meghana.madhyastha@gmail.com
Changes in v3: -None
drivers/gpu/drm/drm_of.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ include/drm/drm_of.h | 2 ++ 2 files changed, 50 insertions(+)
diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c index d878d3a..238e8e5 100644 --- a/drivers/gpu/drm/drm_of.c +++ b/drivers/gpu/drm/drm_of.c @@ -304,3 +304,51 @@ struct backlight_device *drm_of_find_backlight(struct device *dev) return backlight; } EXPORT_SYMBOL(drm_of_find_backlight);
+/**
- devm_drm_of_find_backlight_release - Release backlight device
- @dev: Device
- This is the release function corresponding to the devm_drm_of_find_backlight.
- Each devres entry is associated with a release function.
- */
This is an internal function so no need for docs or exporting. I know that some devm_ functions have explicit release functions, but I don't think this is necessary since those users can just use drm_of_find_backlight() directly instead.
I have a question here. devm_drm_of_find_backlight_release is a wrapper around put_device which is passed as a parameter to devm_add_action in devm_drm_of_find_backlight. So isn't the function useful here ?
We need the function for devm_add_action(), but no one outside of this file needs it. Hence the static definition.
We can't use put_device directly: ret = devm_add_action(dev, put_device, &backlight->dev); because devm_add_action expects an argument like this: void (*action)(void *) and put_device is: void put_device(struct device *dev) They differ in their argument type: void * vs. struct device * The compiler would complain.
So we need a wrapper.
Yes this is what I thought. Okay, I misunderstood your previous suggestion as removing the release function entirely which is why I asked my question. Now it makes sense and I understood.
Thanks for the clarifications.
Regards, Meghana
+static void devm_drm_of_find_backlight_release(void *data) +{
- put_device(data);
+} +EXPORT_SYMBOL(devm_drm_of_find_backlight_release);
+/**
- devm_drm_of_find_backlight - Find backlight device in device-tree
- devres version of the function
- @dev: Device
- This is the devres version of the function drm_of_find_backlight.
- Some drivers such as tinydrm use devres versions of functions for
No need to mention tinydrm here.
- requiring device resources.
- Returns:
- NULL if there's no backlight property.
- Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
- is found.
- If the backlight device is found, a pointer to the structure is returned.
- */
+struct backlight_device *devm_drm_of_find_backlight(struct device *dev) +{
- struct backlight_device *backlight;
- int ret;
- backlight = drm_of_find_backlight(dev);
- if (IS_ERR_OR_NULL(backlight))
return backlight;- ret = devm_add_action(dev, devm_drm_of_find_backlight_release,
&backlight->dev);- if (ret) {
put_device(&backlight->dev);return ERR_PTR(ret);- }
- return backlight;
+} +EXPORT_SYMBOL(devm_drm_of_find_backlight); diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h index e8fba5b..071fb3b 100644 --- a/include/drm/drm_of.h +++ b/include/drm/drm_of.h @@ -30,7 +30,9 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, struct drm_panel **panel, struct drm_bridge **bridge); struct backlight_device *drm_of_find_backlight(struct device *dev); +struct backlight_device *devm_drm_of_find_backlight(struct device *dev); #else
We need a dummy version of devm_drm_of_find_backlight() here that returns NULL as in the previous patch.
+static void devm_drm_of_find_backlight_release(void *data);
And this isn't needed as explained above.
static inline uint32_t drm_of_find_possible_crtcs(struct drm_device *dev, struct device_node *port) {
I'd appreciate if you could also switch mi0283qt over to this helper :-)
Noralf.
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Jani Nikula -
kbuild test robot -
Meghana Madhyastha -
Noralf Trønnes