[PATCH v14 0/3] Move backlight helper functions from tinydrm-helpers to linux/backlight

[Noralf Trønnes]

Den 11.12.2017 18.45, skrev Noralf Trønnes:
>
> Den 11.12.2017 15.58, skrev Meghana Madhyastha:
>> On Mon, Dec 11, 2017 at 03:12:06PM +0100, Noralf Trønnes wrote:
>>> Den 11.12.2017 14.17, skrev Meghana Madhyastha:
>>>> On Sat, Dec 09, 2017 at 03:09:28PM +0100, Noralf Trønnes wrote:
>>>>> Den 21.10.2017 13.55, skrev Meghana Madhyastha:
>>>>>> Changes in v14:
>>>>>> - s/backlight_get/of_find_backlight/ in patch 2/3
>>>>>> - Change commit message in patch 3/3 from requiring to acquiring
>>>>>>
>>>>>> Meghana Madhyastha (3):
>>>>>>    drm/tinydrm: Move helper functions from tinydrm-helpers to 
>>>>>> backlight.h
>>>>>>    drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c
>>>>>>    drm/tinydrm: Add devres versions of of_find_backlight
>>>>> I tried the patchset and this is what I got:
>>>>>
>>>>> [    8.057792] Unable to handle kernel paging request at virtual 
>>>>> address
>>>>> fffffe6b
<snip>
>>>>> [    9.144181] ---[ end trace 149c05934b6a6dcc ]---
>>>> Is the reason possibly because we have omitted error checking on the
>>>> return value of backlight_enable function ? 
>>>> tinydrm_enable_backlight and
>>>> tinydrm_disable_baclight do this.
>>>> Eg.
>>>> ret = backlight_update_status(backlight);
>>>> if (ret)
>>>>     DRM_ERROR("Failed to enable backlight %d\n", ret);
>>>>
>>>> I'm not sure, just asking whether this could be a possible reason
>>>> for the above trace.
>>> The crash happens during probe.
>>> I guess you'll figure this out when you get some hw to test on.
>> I have set up the raspberry pi and have built and boot into the 
>> custom kernel
>> but I am waiting for the panel to arrive. Meanwhile, any thoughts on
>> error message ? Sorry for the trivial question, but I did not quite
>> understand the crash message and how to replicate it.
>
> of_find_backlight() can return an error pointer (-EPROBE_DEFER):
>
> diff --git a/drivers/video/backlight/backlight.c 
> b/drivers/video/backlight/backlight.c
> index 4bb7bf3ee443..57370c5d51f0 100644
> --- a/drivers/video/backlight/backlight.c
> +++ b/drivers/video/backlight/backlight.c
> @@ -635,8 +635,8 @@ struct backlight_device 
> *devm_of_find_backlight(struct device *dev)
>         int ret;
>
>         bd = of_find_backlight(dev);
> -       if (!bd)
> -               return NULL;
> +       if (IS_ERR_OR_NULL(bd))
> +               return bd;
>
>         ret = devm_add_action(dev, devm_backlight_put, bd);
>         if (ret) {
>
> That solved the crash, but the backlight didn't turn on.
> I had to do this as well:
>
> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> index 5c441d4c049c..6f9925f10a7c 100644
> --- a/include/linux/backlight.h
> +++ b/include/linux/backlight.h
> @@ -139,6 +139,8 @@ static inline int backlight_enable(struct 
> backlight_device *bd)
>         if (!bd)
>                 return 0;
>
> +       if (!bd->props.brightness)
> +               bd->props.brightness = bd->props.max_brightness;

No, this is wrong, it should happen on probe, not every time it's enabled.
So maybe it should be done in of_find_backlight() or something.
I see that I'm currently doing it in tinydrm_of_find_backlight().

> bd->props.power = FB_BLANK_UNBLANK;
>         bd->props.state &= ~BL_CORE_FBBLANK;
>
> This is my backlight node[1]:
>
> backlight: backlight {
>     compatible = "gpio-backlight";
>     gpios = <&gpio 18 0>; // GPIO_ACTIVE_HIGH
> };
>
> Not certain that this is the "correct" solution, but I can't use the
> gpio-backlight property default-on, because that would turn on backlight
> before the pipeline is enabled.
>
> You can dry-run the driver without a panel connected, it can work
> without being able to read from the controller.
>
> Noralf.
>
> [1] 
> https://github.com/notro/tinydrm/blob/master/rpi-overlays/rpi-display-overlay.dts
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>