Re: [PATCH v2] drm/tinydrm: Move backlight helpers to a separate file

[Meghana Madhyastha <meghana.madhyastha@gmail.com>]

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@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.

-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@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