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

[abhinavk@codeaurora.org]

Hi Bjorn

Thanks for the comments.

Reply inline.

On 2018-04-16 23:13, Bjorn Andersson wrote:
> 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
[Abhinav] This particular panel isnt a piece of backlight hardware.
But, we want to have our own sysfs to give flexibility to our userspace
to write and read stuff for its proprietary uses.
Thats how our downstream has been working so far and hence to maintain
the compatibility would like to have it.
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel