On Mon, Sep 25, 2017 at 06:31:58PM +0200, Noralf Trønnes wrote: > > Den 25.09.2017 16.56, skrev Noralf Trønnes: > >Hi Meghana, > > > > > >Den 22.09.2017 17.09, skrev Meghana Madhyastha: > >>Move backlight helpers from tinydrm-helpers.c to > >>tinydrm-backlight.c. This is because it is organizationally > >>simpler to understand and advantageous to group functions > >>performing a similar function to a separate file as opposed to > >>having one helper file with heteregenous helper functions. > >> > >>Signed-off-by: Meghana Madhyastha <meghana.madhyastha@xxxxxxxxx> > >>--- > > > >I don't think there is much gain in just moving the code like this. > > > >The idea is to add a drm_backlight helper that can be useful for all > >DRM drivers that use the backlight subsystem. Yes I agree. That definitely makes more sense. > > The full path to that helper would be: > drivers/gpu/drm/drm_backlight.c > > >This is what the TODO says: > >https://dri.freedesktop.org/docs/drm/gpu/todo.html#tinydrm > > > >- backlight helpers, probably best to put them into a new drm_backlight.c. > > This is because drivers/video is de-facto unmaintained. We could also > > move drivers/video/backlight to drivers/gpu/backlight and take it all > > over within drm-misc, but that’s more work. > > > >There is also this discussion to take into account: > >KMS backlight ABI proposition > >https://lists.freedesktop.org/archives/dri-devel/2017-February/133206.html > > > > > >I don't remember what came out of that discussion. > > > >Noralf. After having discussed this with Daniel on the #dri-devel irc channel, here are some of the points suggested. Daniel suggested that I first look into the usage of shared backlight helpers in drm (specifically backlight_update_status to begin with). The idea was to see whether there is any pattern in usage and/or code dupication. If there is, then the next step would be to write helper functions which can be used by other drivers (and not just tinydrm). To start with, I went through the instances of backlight_update_status in the drm code, and made the following observations(most of them are very simple/naive observations). - backlight_update_status is usually called in backlight init (and sometimes exit) functions of the drivers just after the device is registered. So backlight_update_status is called with the registered device as the parameter. Here are the following cases of properties changed/set before backlight_update_status is called. - CASE 1: Brightness is changed (either a macro BRIGHTNESS_MAX_LEVEL 100 is defined or it is manually set) This happens in the following files: gma500/cdv_device.c, gma500/mdfld_device.c, gma500/oaktrail_device.c, gma500/psb_device.c, noveau/noveau_backlight.c(here brightness is determined by fuction static int nv50_get_intensity) - CASE 2: Power property is set (to FB_BLANK_UNBLANK mostly) This happens in the following files: omapdrm/displays/panel-dpi.c, panel/panel-innolux-p079zca.c, panel/panel-jdi-lt070me05000.c, panel/panel-sharp-lq101r1sx01.c, panel/panel-sharp-ls043t1le01.c, tilcdc/tilcdc_panel.c - CASE 3: State is set This happens in the following files: tinydrm/tinydrm-helpers.c - CASE 4: Power and brightness properties are set This happens in the following files: atombios_encoders.c, radeon/radeon_legacy_encoders.c, shmobile/shmob_drm_backlight.c - CASE 5: Power and the state properties are set This happens in the following files: panel/panel-lvds.c, panel/panel-panasonic-vvx10f034n00.c, panel/panel-simple.c, panel/panel-sitronix-st7789v.c Please let me know if I am wrong / missed something. As for next steps, wouldn't it be an overkill to have a separate helper function for each of these cases ? Perhaps a generic helper function which would somehow address these cases would be more appropriate ? Thank you for your time/patience. -Meghana > >>Changes in v2: > >> -Improved commit message by explaining why the changes were made. > >> > >> drivers/gpu/drm/tinydrm/core/Makefile | 2 +- > >> drivers/gpu/drm/tinydrm/core/tinydrm-backlight.c | 103 > >>+++++++++++++++++++++++ > >> drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 94 > >>--------------------- > >> drivers/gpu/drm/tinydrm/mi0283qt.c | 1 + > >> drivers/gpu/drm/tinydrm/mipi-dbi.c | 1 + > >> include/drm/tinydrm/tinydrm-backlight.h | 18 ++++ > >> 6 files changed, 124 insertions(+), 95 deletions(-) > >> create mode 100644 drivers/gpu/drm/tinydrm/core/tinydrm-backlight.c > >> create mode 100644 include/drm/tinydrm/tinydrm-backlight.h > >> > >>diff --git a/drivers/gpu/drm/tinydrm/core/Makefile > >>b/drivers/gpu/drm/tinydrm/core/Makefile > >>index fb221e6..389ca7a 100644 > >>--- a/drivers/gpu/drm/tinydrm/core/Makefile > >>+++ b/drivers/gpu/drm/tinydrm/core/Makefile > >>@@ -1,3 +1,3 @@ > >>-tinydrm-y := tinydrm-core.o tinydrm-pipe.o tinydrm-helpers.o > >>+tinydrm-y := tinydrm-core.o tinydrm-pipe.o tinydrm-backlight.o > >>tinydrm-helpers.o > >> obj-$(CONFIG_DRM_TINYDRM) += tinydrm.o > >>diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-backlight.c > >>b/drivers/gpu/drm/tinydrm/core/tinydrm-backlight.c > >>new file mode 100644 > >>index 0000000..dc6f17d > >>--- /dev/null > >>+++ b/drivers/gpu/drm/tinydrm/core/tinydrm-backlight.c > >>@@ -0,0 +1,103 @@ > >>+#include <linux/backlight.h> > >>+#include <linux/dma-buf.h> > >>+#include <linux/pm.h> > >>+#include <linux/swab.h> > >>+ > >>+#include <drm/tinydrm/tinydrm.h> > >>+#include <drm/tinydrm/tinydrm-backlight.h> > >>+ > >>+/** > >>+ * 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 > >>+ * > >>+ * Returns: > >>+ * Zero on success, negative error code on failure. > >>+ */ > >>+int tinydrm_enable_backlight(struct backlight_device *backlight) > >>+{ > >>+ unsigned int old_state; > >>+ int ret; > >>+ > >>+ if (!backlight) > >>+ return 0; > >>+ > >>+ old_state = backlight->props.state; > >>+ backlight->props.state &= ~BL_CORE_FBBLANK; > >>+ DRM_DEBUG_KMS("Backlight state: 0x%x -> 0x%x\n", old_state, > >>+ backlight->props.state); > >>+ > >>+ ret = backlight_update_status(backlight); > >>+ if (ret) > >>+ DRM_ERROR("Failed to enable backlight %d\n", ret); > >>+ > >>+ return ret; > >>+} > >>+EXPORT_SYMBOL(tinydrm_enable_backlight); > >>+ > >>+/** > >>+ * tinydrm_disable_backlight - Disable backlight helper > >>+ * @backlight: Backlight device > >>+ * > >>+ * Returns: > >>+ * Zero on success, negative error code on failure. > >>+ */ > >>+int tinydrm_disable_backlight(struct backlight_device *backlight) > >>+{ > >>+ unsigned int old_state; > >>+ int ret; > >>+ > >>+ if (!backlight) > >>+ return 0; > >>+ > >>+ old_state = backlight->props.state; > >>+ backlight->props.state |= BL_CORE_FBBLANK; > >>+ DRM_DEBUG_KMS("Backlight state: 0x%x -> 0x%x\n", old_state, > >>+ backlight->props.state); > >>+ ret = backlight_update_status(backlight); > >>+ if (ret) > >>+ DRM_ERROR("Failed to disable backlight %d\n", ret); > >>+ > >>+ return ret; > >>+} > >>+EXPORT_SYMBOL(tinydrm_disable_backlight); > >>diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c > >>b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c > >>index bd6cce0..ee8ad8c 100644 > >>--- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c > >>+++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c > >>@@ -236,100 +236,6 @@ void tinydrm_xrgb8888_to_gray8(u8 *dst, void > >>*vaddr, struct drm_framebuffer *fb, > >> } > >> EXPORT_SYMBOL(tinydrm_xrgb8888_to_gray8); > >> -/** > >>- * tinydrm_of_find_backlight - Find backlight device in device-tree > >>- * @dev: Device > >>- * > >>- * This function looks for a DT node pointed to by a property named > >>'backlight' > >>- * and uses of_find_backlight_by_node() to get the backlight device. > >>- * Additionally if the brightness property is zero, it is set to > >>- * max_brightness. > >>- * > >>- * Returns: > >>- * NULL if there's no backlight property. > >>- * Error pointer -EPROBE_DEFER if the DT node is found, but no > >>backlight device > >>- * is found. > >>- * If the backlight device is found, a pointer to the structure is > >>returned. > >>- */ > >>-struct backlight_device *tinydrm_of_find_backlight(struct device *dev) > >>-{ > >>- struct backlight_device *backlight; > >>- struct device_node *np; > >>- > >>- np = of_parse_phandle(dev->of_node, "backlight", 0); > >>- if (!np) > >>- return NULL; > >>- > >>- backlight = of_find_backlight_by_node(np); > >>- of_node_put(np); > >>- > >>- if (!backlight) > >>- return ERR_PTR(-EPROBE_DEFER); > >>- > >>- if (!backlight->props.brightness) { > >>- backlight->props.brightness = backlight->props.max_brightness; > >>- DRM_DEBUG_KMS("Backlight brightness set to %d\n", > >>- backlight->props.brightness); > >>- } > >>- > >>- return backlight; > >>-} > >>-EXPORT_SYMBOL(tinydrm_of_find_backlight); > >>- > >>-/** > >>- * tinydrm_enable_backlight - Enable backlight helper > >>- * @backlight: Backlight device > >>- * > >>- * Returns: > >>- * Zero on success, negative error code on failure. > >>- */ > >>-int tinydrm_enable_backlight(struct backlight_device *backlight) > >>-{ > >>- unsigned int old_state; > >>- int ret; > >>- > >>- if (!backlight) > >>- return 0; > >>- > >>- old_state = backlight->props.state; > >>- backlight->props.state &= ~BL_CORE_FBBLANK; > >>- DRM_DEBUG_KMS("Backlight state: 0x%x -> 0x%x\n", old_state, > >>- backlight->props.state); > >>- > >>- ret = backlight_update_status(backlight); > >>- if (ret) > >>- DRM_ERROR("Failed to enable backlight %d\n", ret); > >>- > >>- return ret; > >>-} > >>-EXPORT_SYMBOL(tinydrm_enable_backlight); > >>- > >>-/** > >>- * tinydrm_disable_backlight - Disable backlight helper > >>- * @backlight: Backlight device > >>- * > >>- * Returns: > >>- * Zero on success, negative error code on failure. > >>- */ > >>-int tinydrm_disable_backlight(struct backlight_device *backlight) > >>-{ > >>- unsigned int old_state; > >>- int ret; > >>- > >>- if (!backlight) > >>- return 0; > >>- > >>- old_state = backlight->props.state; > >>- backlight->props.state |= BL_CORE_FBBLANK; > >>- DRM_DEBUG_KMS("Backlight state: 0x%x -> 0x%x\n", old_state, > >>- backlight->props.state); > >>- ret = backlight_update_status(backlight); > >>- if (ret) > >>- DRM_ERROR("Failed to disable backlight %d\n", ret); > >>- > >>- return ret; > >>-} > >>-EXPORT_SYMBOL(tinydrm_disable_backlight); > >> #if IS_ENABLED(CONFIG_SPI) > >> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c > >>b/drivers/gpu/drm/tinydrm/mi0283qt.c > >>index 7e5bb7d..c161d45 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/tinydrm/tinydrm-backlight.h> > >> #include <linux/delay.h> > >> #include <linux/gpio/consumer.h> > >> #include <linux/module.h> > >>diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c > >>b/drivers/gpu/drm/tinydrm/mipi-dbi.c > >>index 2caeabc..dc84f26 100644 > >>--- a/drivers/gpu/drm/tinydrm/mipi-dbi.c > >>+++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c > >>@@ -11,6 +11,7 @@ > >> #include <drm/tinydrm/mipi-dbi.h> > >> #include <drm/tinydrm/tinydrm-helpers.h> > >>+#include <drm/tinydrm/tinydrm-backlight.h> > >> #include <linux/debugfs.h> > >> #include <linux/dma-buf.h> > >> #include <linux/gpio/consumer.h> > >>diff --git a/include/drm/tinydrm/tinydrm-backlight.h > >>b/include/drm/tinydrm/tinydrm-backlight.h > >>new file mode 100644 > >>index 0000000..6a7b6d5 > >>--- /dev/null > >>+++ b/include/drm/tinydrm/tinydrm-backlight.h > >>@@ -0,0 +1,18 @@ > >>+/* > >>+ * Copyright (C) 2016 Noralf Trønnes > >>+ * > >>+ * This program is free software; you can redistribute it and/or modify > >>+ * it under the terms of the GNU General Public License as published by > >>+ * the Free Software Foundation; either version 2 of the License, or > >>+ * (at your option) any later version. > >>+ */ > >>+ > >>+#ifndef __LINUX_TINYDRM_BACKLIGHT_H > >>+#define __LINUX_TINYDRM_BACKLIGHT_H > >>+ > >>+struct backlight_device; > >>+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); > >>+ > >>+#endif /* __LINUX_TINYDRM_BACKLIGHT_H */ > > > >_______________________________________________ > >dri-devel mailing list > >dri-devel@xxxxxxxxxxxxxxxxxxxxx > >https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
