[PATCH v2] drm/tinydrm: Move backlight helpers to a separate file
Daniel Vetter
daniel at ffwll.ch
Tue Sep 26 11:32:15 UTC 2017
On Tue, Sep 26, 2017 at 04:46:53PM +0530, Meghana Madhyastha wrote:
> 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 at gmail.com>
> > >>---
> > >
> > >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.
I suspect that a lot of these combinations are just plain wrong, but
happen to kinda work with the combination of gpu driver and backlight
driver they're used on. tbh I have no idea which one is the correct
version for enabling a backlight correctly ...
So definitely a good task to refactor this into a proper helper, but looks
a lot more involved than at first sight.
Do you have any of the hardware supported by any of these drivers? lsmod
and then comparing with the modules you're building in your own tree
should help you figure this out.
-Daniel
>
> -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 at lists.freedesktop.org
> > >https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > >
> >
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the dri-devel
mailing list