Hi Bjorn

Thanks for the review.

Reply inline.

On 2018-04-16 09:41, Bjorn Andersson wrote:
On Sat 14 Apr 00:25 PDT 2018, Abhinav Kumar wrote:

Register truly panel as a backlight led device and
provide methods to control its backlight operation.

Changes in v2:
- Removed redundant NULL checks
- Arranged headers alphabetically
- Formatting fixes

The change log goes below the "---" line.

[Abhinav] As sean mentioned, its quite common to list it to view it in the log.
This practice has been followed by quite a few in DRM
Another reference here https://patchwork.freedesktop.org/patch/211361/


Signed-off-by: Abhinav Kumar <abhin...@codeaurora.org>
---
[..]
+static int truly_backlight_setup(struct truly_wqxga *ctx)
+{
+       struct backlight_properties props;
+       char bl_node_name[BL_NODE_NAME_SIZE];
+
+       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);
+
+               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;
+               }

It seems like you're registering a backlight driver for the sake of
invoking the LED backlight trigger to control the WLED.

The WLED is a backlight driver, so all you should have to do is add the
following line to your probe:

        ctx->backlight = devm_of_find_backlight(dev);

and then add "backlight = <&wled>" to your dt node.

Regards,
Bjorn
[Abhinav] Thats not the only purpose of backlight_device_register().
We want to hook up our panel with the parent backlight driver in
drivers/video/backlight/backlight.c and also route all the update_backlight_status()
calls through the sysfs of the newly registered node.

The of_find_backlight() method doesnt seem to allow us to register our own
sysfs method.

BTW, this isnt something which we are doing uniquely.
There are other panels which seem to be doing this :

drivers/gpu/drm/panel/panel-samsung-s6e3ha2.c

Can you please comment on whether we can have our own sysfs without
the device_register()?

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Reply via email to