Hi Sean

Thanks for the comments.

Some replies inline.

On 2018-04-13 13:46, Sean Paul wrote:
On Sat, Apr 07, 2018 at 12:06:53AM -0700, Abhinav Kumar wrote:
Register truly panel as a backlight led device and
provide methods to control its backlight operation.

Signed-off-by: Abhinav Kumar <abhin...@codeaurora.org>
---
drivers/gpu/drm/panel/panel-truly-dual-dsi.c | 96 +++++++++++++++++++++++++++-
 1 file changed, 94 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-truly-dual-dsi.c b/drivers/gpu/drm/panel/panel-truly-dual-dsi.c
index 47891ee..5d0ef90 100644
--- a/drivers/gpu/drm/panel/panel-truly-dual-dsi.c
+++ b/drivers/gpu/drm/panel/panel-truly-dual-dsi.c
@@ -14,6 +14,7 @@
 #include <linux/gpio/consumer.h>
 #include <linux/regulator/consumer.h>
 #include <linux/pinctrl/consumer.h>
+#include <linux/leds.h>

Includes should be alphabetical.
[Abhinav] Ok, will take care of this in v2.


 #include <video/mipi_display.h>
 #include <video/of_videomode.h>
@@ -23,6 +24,9 @@
 #include <drm/drm_panel.h>
 #include <drm/drm_mipi_dsi.h>

+#define BL_NODE_NAME_SIZE 32
+#define PRIM_DISPLAY_NODE 0
+
 struct truly_wqxga {
        struct device *dev;
        struct drm_panel panel;
@@ -33,6 +37,8 @@ struct truly_wqxga {
        struct gpio_desc *mode_gpio;

        struct backlight_device *backlight;
+       /* WLED params */
+       struct led_trigger *wled;
        struct videomode vm;

        struct mipi_dsi_device *dsi[2];
@@ -447,6 +453,83 @@ static void truly_wqxga_panel_del(struct truly_wqxga *ctx)
                put_device(&ctx->dsi[1]->dev);
 }

+static int truly_backlight_device_update_status(struct backlight_device *bd)
+{
+       int brightness;
+       int max_brightness;
+       int rc = 0;
+
extra line

[Abhinav] Ok, will remove it in v2.

+       struct truly_wqxga *ctx = dev_get_drvdata(&bd->dev);
+
+       brightness = bd->props.brightness;
+       max_brightness = bd->props.max_brightness;
+
+       if ((bd->props.power != FB_BLANK_UNBLANK) ||
+               (bd->props.state & BL_CORE_FBBLANK) ||
+                 (bd->props.state & BL_CORE_SUSPENDED))
+               brightness = 0;
+
+       if (brightness > max_brightness)
+               brightness = max_brightness;
+
+       /* Need to check WLED driver capability upstream */
+       if (ctx && ctx->wled)

ctx can't be NULL, so no need to check for that. And if ctx->wled is null, it
doesn't seem like this function will do anything. So how about just not
registering the backlight if wled == NULL (if that's possible).
[Abhinav] Yes, will remove the NULL check.
We are registering the backlight in the backlight_setup(), this was more of an additional check, to make sure ctx->wled is present before we trigger.
Not sure if we should register again here.

+               led_trigger_event(ctx->wled, brightness);
+
+       return rc;
+}
+
+static int truly_backlight_device_get_brightness(struct backlight_device *bd)
+{
+       return bd->props.brightness;
+}
+
+static const struct backlight_ops truly_backlight_device_ops = {
+       .update_status = truly_backlight_device_update_status,
+       .get_brightness = truly_backlight_device_get_brightness,
+};
+
+static int truly_backlight_setup(struct truly_wqxga *ctx)
+{
+       struct backlight_properties props;
+       char bl_node_name[BL_NODE_NAME_SIZE];
+
+       if (!ctx) {
+               dev_err(ctx->dev, "invalid context\n");
+               return -EINVAL;
+       }

This can't happen.
[Abhinav] Will remove it.

+
+       if (!ctx->backlight) {
+               memset(&props, 0, sizeof(props));
+               props.type = BACKLIGHT_RAW;
+               props.power = FB_BLANK_UNBLANK;
+               props.max_brightness = 4096;
+
+               snprintf(bl_node_name, BL_NODE_NAME_SIZE, "panel%u-backlight",
+                                PRIM_DISPLAY_NODE);

Given that PRIM_DISPLAY_NODE is always 0, this seems like overkill for a pretty generic name "panel0-backlight". So let's just call it "truly_backlight" in the
register call.

[Abhinav] The reason for keeping it "panel0-backlight" is because userspace is using this node name to write the backlight. Changing the name will need more changes in our
userspace.

+
+               ctx->backlight =  backlight_device_register(bl_node_name,
+                               ctx->dev, ctx,
+                               &truly_backlight_device_ops, &props);
+
+               if (IS_ERR_OR_NULL(ctx->backlight)) {
+                       pr_err("Failed to register backlight\n");
+                       ctx->backlight = NULL;
+                       return -ENODEV;
+               }
+
+               /* Register with the LED driver interface */
+               led_trigger_register_simple("bkl-trigger", &ctx->wled);
+
+               if (!ctx->wled) {
+                       pr_err("backlight led registration failed\n");
+                       return -ENODEV;
+               }
+       }
+
+       return 0;
+}
+
 static int truly_wqxga_probe(struct mipi_dsi_device *dsi)
 {
        struct device *dev = &dsi->dev;
@@ -466,10 +549,11 @@ static int truly_wqxga_probe(struct mipi_dsi_device *dsi)
                secondary = of_find_mipi_dsi_device_by_node(np);
                of_node_put(np);

+               ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
+

Why move this?
[Abhinav] Thanks for catching this, yes not required.
Will move it back.

                if (!secondary)
                        return -EPROBE_DEFER;

-               ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
                if (!ctx) {
                        put_device(&secondary->dev);
                        return -ENOMEM;
@@ -485,6 +569,12 @@ static int truly_wqxga_probe(struct mipi_dsi_device *dsi)
                        put_device(&secondary->dev);
                        return ret;
                }
+
+               ret = truly_backlight_setup(ctx);
+               if (ret) {
+                       put_device(&secondary->dev);
+                       return ret;
+               }
        }

        ret = mipi_dsi_attach(dsi);
@@ -504,8 +594,10 @@ static int truly_wqxga_remove(struct mipi_dsi_device *dsi)
        mipi_dsi_detach(dsi);

        /* delete panel only for the DSI1 interface */
-       if (ctx)
+       if (ctx) {
                truly_wqxga_panel_del(ctx);
+               kfree(ctx);
+       }

        return 0;
 }
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Reply via email to