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

[abhinavk-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org]

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 <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.

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