Re: [DPU PATCH v2 2/2] drm/panel: add backlight control support for truly panel

[Bjorn Andersson <bjorn.andersson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>]

On Mon 16 Apr 15:45 PDT 2018, abhinavk@codeaurora.org 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@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.
> 

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