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
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
