On Mon 16 Apr 15:45 PDT 2018, abhinavk@xxxxxxxxxxxxxx wrote: > 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/ > If that's the practice in DRM land, then that's what you should do. > > > > > > Signed-off-by: Abhinav Kumar <abhinavk@xxxxxxxxxxxxxx> > > > --- > > [..] > > > +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. > Are you saying that you want to be able to create an alias for the wled entry already in /sys/class/backlight named panel0-backlight? > 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 > What seems to be going on in the s6e3ha2 driver is that the hardware has some sort of builtin backlight control, so the driver is creating a backlight driver for the purpose of exposing those controls. > Can you please comment on whether we can have our own sysfs without > the device_register()? > If the panel isn't actually a piece of backlight hardware then it should not register a backlight device. Why do you need your own sysfs? Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
