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

[Sujeev Dias <sdias-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>]

Testing my responsde


On 04/17/2018 11:21 AM, abhinavk@codeaurora.org wrote:
> 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
> -- 
> To unsubscribe from this list: send the line "unsubscribe 
> linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

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