2016-11-22 13:42:04

by Peter Ujfalusi

[permalink] [raw]
Subject: [PATCH v4 0/2] backlight: pwm_bl: Fix the initial power state selection

Hi,

Changes since v3:
- Change 'pwm' to 'PWM' in comments and commit message
- Extended the comment before we change the GPIO direction and state in case it
was set as input initially.
- Acked/Review-by added from Philipp and Thierry

Changes since v2:
- Do not change the way how the GPIO initially configured as input is handled.
Configure it as output and set it's state as active.

Changes since v1:
- Handling of the enable GPIO is reworked:
- Only change direction to output when the pin was input and in this case set
the GPIO line physical low
- With this change we can ensure that the enable GPIO is output so we do not
need to check the direction of it later on.

Cover letter:

3698d7e7d221 backlight: pwm_bl: Avoid backlight flicker when probed from DT

added support for avoiding backlight flickering, which in essence was designed
to not enable the baclkight when the driver loads, but let the user of the
backlight to enable it later on.

There are boards (like am437x-gp-evm) where we do not have valid GPIO to enable
the backlight (TPS61081DRC's EN pin is connected to V3_3D) and the regulator
is always on (VBAT in case of the gp-evm). In this board the logic to check the
GPIO state and the regulator is failing and the backlight will be enabled as
soon as the pwm_bl driver is loaded.

By extending the check to look at the PWM state this issue can be avoided and
the backlight will be enabled only when it's user is asking it to be enabled.

Regards,
Peter
---
Peter Ujfalusi (2):
backlight: pwm_bl: Move the checks for initial power state to a
separate function
backlight: pwm_bl: Check the PWM state for initial backlight power
state

drivers/video/backlight/pwm_bl.c | 60 +++++++++++++++++++++++++++-------------
1 file changed, 41 insertions(+), 19 deletions(-)

--
2.11.0.rc2


2016-11-22 13:42:17

by Peter Ujfalusi

[permalink] [raw]
Subject: [PATCH v4 2/2] backlight: pwm_bl: Check the PWM state for initial backlight power state

If the PWM is not enabled the backlight initially should not be enabled
either if we have booted with DT and there is a phandle pointing to the
backlight node.

The patch extends the checks to decide if we should keep the backlight off
initially.

Signed-off-by: Peter Ujfalusi <[email protected]>
Acked-by: Philipp Zabel <[email protected]>
Reviewed-by: Thierry Reding <[email protected]>
---
drivers/video/backlight/pwm_bl.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 5712ddd053dd..d7efcb632f7d 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -215,6 +215,10 @@ static int pwm_backlight_initial_power_state(const struct pwm_bl_data *pb)
if (!regulator_is_enabled(pb->power_supply))
return FB_BLANK_POWERDOWN;

+ /* The PWM is disabled, keep it like this */
+ if (!pwm_is_enabled(pb->pwm))
+ return FB_BLANK_POWERDOWN;
+
return FB_BLANK_UNBLANK;
}

--
2.11.0.rc2

2016-11-22 13:42:10

by Peter Ujfalusi

[permalink] [raw]
Subject: [PATCH v4 1/2] backlight: pwm_bl: Move the checks for initial power state to a separate function

Move the checks to select the initial state for the backlight to a new
function and document the checks we are doing.

With the separate function it is going to be easier to fix or improve the
initial power state configuration later and it is easier to read the code.

Signed-off-by: Peter Ujfalusi <[email protected]>
Reviewed-by: Philipp Zabel <[email protected]>
Reviewed-by: Thierry Reding <[email protected]>
---
drivers/video/backlight/pwm_bl.c | 56 ++++++++++++++++++++++++++--------------
1 file changed, 37 insertions(+), 19 deletions(-)

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 12614006211e..5712ddd053dd 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -192,6 +192,32 @@ static int pwm_backlight_parse_dt(struct device *dev,
}
#endif

+static int pwm_backlight_initial_power_state(const struct pwm_bl_data *pb)
+{
+ struct device_node *node = pb->dev->of_node;
+
+ /* Not booted with device tree or no phandle link to the node */
+ if (!node || !node->phandle)
+ return FB_BLANK_UNBLANK;
+
+ /*
+ * If the driver is probed from the device tree and there is a
+ * phandle link pointing to the backlight node, it is safe to
+ * assume that another driver will enable the backlight at the
+ * appropriate time. Therefore, if it is disabled, keep it so.
+ */
+
+ /* if the enable GPIO is disabled, do not enable the backlight */
+ if (pb->enable_gpio && gpiod_get_value(pb->enable_gpio) == 0)
+ return FB_BLANK_POWERDOWN;
+
+ /* The regulator is disabled, do not enable the backlight */
+ if (!regulator_is_enabled(pb->power_supply))
+ return FB_BLANK_POWERDOWN;
+
+ return FB_BLANK_UNBLANK;
+}
+
static int pwm_backlight_probe(struct platform_device *pdev)
{
struct platform_pwm_backlight_data *data = dev_get_platdata(&pdev->dev);
@@ -200,7 +226,6 @@ static int pwm_backlight_probe(struct platform_device *pdev)
struct backlight_device *bl;
struct device_node *node = pdev->dev.of_node;
struct pwm_bl_data *pb;
- int initial_blank = FB_BLANK_UNBLANK;
struct pwm_args pargs;
int ret;

@@ -267,20 +292,16 @@ static int pwm_backlight_probe(struct platform_device *pdev)
pb->enable_gpio = gpio_to_desc(data->enable_gpio);
}

- if (pb->enable_gpio) {
- /*
- * If the driver is probed from the device tree and there is a
- * phandle link pointing to the backlight node, it is safe to
- * assume that another driver will enable the backlight at the
- * appropriate time. Therefore, if it is disabled, keep it so.
- */
- if (node && node->phandle &&
- gpiod_get_direction(pb->enable_gpio) == GPIOF_DIR_OUT &&
- gpiod_get_value(pb->enable_gpio) == 0)
- initial_blank = FB_BLANK_POWERDOWN;
- else
- gpiod_direction_output(pb->enable_gpio, 1);
- }
+ /*
+ * If the GPIO is configured as input, change the direction to output
+ * and set the GPIO as active.
+ * Do not force the GPIO to active when it was already output as it
+ * could cause backlight flickering or we would enable the backlight too
+ * early. Leave the decision of the initial backlight state for later.
+ */
+ if (pb->enable_gpio &&
+ gpiod_get_direction(pb->enable_gpio) == GPIOF_DIR_IN)
+ gpiod_direction_output(pb->enable_gpio, 1);

pb->power_supply = devm_regulator_get(&pdev->dev, "power");
if (IS_ERR(pb->power_supply)) {
@@ -288,9 +309,6 @@ static int pwm_backlight_probe(struct platform_device *pdev)
goto err_alloc;
}

- if (node && node->phandle && !regulator_is_enabled(pb->power_supply))
- initial_blank = FB_BLANK_POWERDOWN;
-
pb->pwm = devm_pwm_get(&pdev->dev, NULL);
if (IS_ERR(pb->pwm) && PTR_ERR(pb->pwm) != -EPROBE_DEFER && !node) {
dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n");
@@ -347,7 +365,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
}

bl->props.brightness = data->dft_brightness;
- bl->props.power = initial_blank;
+ bl->props.power = pwm_backlight_initial_power_state(pb);
backlight_update_status(bl);

platform_set_drvdata(pdev, bl);
--
2.11.0.rc2

2016-12-09 09:10:40

by Lee Jones

[permalink] [raw]
Subject: Re: [PATCH v4 1/2] backlight: pwm_bl: Move the checks for initial power state to a separate function

On Tue, 22 Nov 2016, Peter Ujfalusi wrote:

> Move the checks to select the initial state for the backlight to a new
> function and document the checks we are doing.
>
> With the separate function it is going to be easier to fix or improve the
> initial power state configuration later and it is easier to read the code.
>
> Signed-off-by: Peter Ujfalusi <[email protected]>
> Reviewed-by: Philipp Zabel <[email protected]>
> Reviewed-by: Thierry Reding <[email protected]>
> ---
> drivers/video/backlight/pwm_bl.c | 56 ++++++++++++++++++++++++++--------------
> 1 file changed, 37 insertions(+), 19 deletions(-)

Applied for v4.11, thanks.

> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index 12614006211e..5712ddd053dd 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -192,6 +192,32 @@ static int pwm_backlight_parse_dt(struct device *dev,
> }
> #endif
>
> +static int pwm_backlight_initial_power_state(const struct pwm_bl_data *pb)
> +{
> + struct device_node *node = pb->dev->of_node;
> +
> + /* Not booted with device tree or no phandle link to the node */
> + if (!node || !node->phandle)
> + return FB_BLANK_UNBLANK;
> +
> + /*
> + * If the driver is probed from the device tree and there is a
> + * phandle link pointing to the backlight node, it is safe to
> + * assume that another driver will enable the backlight at the
> + * appropriate time. Therefore, if it is disabled, keep it so.
> + */
> +
> + /* if the enable GPIO is disabled, do not enable the backlight */
> + if (pb->enable_gpio && gpiod_get_value(pb->enable_gpio) == 0)
> + return FB_BLANK_POWERDOWN;
> +
> + /* The regulator is disabled, do not enable the backlight */
> + if (!regulator_is_enabled(pb->power_supply))
> + return FB_BLANK_POWERDOWN;
> +
> + return FB_BLANK_UNBLANK;
> +}
> +
> static int pwm_backlight_probe(struct platform_device *pdev)
> {
> struct platform_pwm_backlight_data *data = dev_get_platdata(&pdev->dev);
> @@ -200,7 +226,6 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> struct backlight_device *bl;
> struct device_node *node = pdev->dev.of_node;
> struct pwm_bl_data *pb;
> - int initial_blank = FB_BLANK_UNBLANK;
> struct pwm_args pargs;
> int ret;
>
> @@ -267,20 +292,16 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> pb->enable_gpio = gpio_to_desc(data->enable_gpio);
> }
>
> - if (pb->enable_gpio) {
> - /*
> - * If the driver is probed from the device tree and there is a
> - * phandle link pointing to the backlight node, it is safe to
> - * assume that another driver will enable the backlight at the
> - * appropriate time. Therefore, if it is disabled, keep it so.
> - */
> - if (node && node->phandle &&
> - gpiod_get_direction(pb->enable_gpio) == GPIOF_DIR_OUT &&
> - gpiod_get_value(pb->enable_gpio) == 0)
> - initial_blank = FB_BLANK_POWERDOWN;
> - else
> - gpiod_direction_output(pb->enable_gpio, 1);
> - }
> + /*
> + * If the GPIO is configured as input, change the direction to output
> + * and set the GPIO as active.
> + * Do not force the GPIO to active when it was already output as it
> + * could cause backlight flickering or we would enable the backlight too
> + * early. Leave the decision of the initial backlight state for later.
> + */
> + if (pb->enable_gpio &&
> + gpiod_get_direction(pb->enable_gpio) == GPIOF_DIR_IN)
> + gpiod_direction_output(pb->enable_gpio, 1);
>
> pb->power_supply = devm_regulator_get(&pdev->dev, "power");
> if (IS_ERR(pb->power_supply)) {
> @@ -288,9 +309,6 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> goto err_alloc;
> }
>
> - if (node && node->phandle && !regulator_is_enabled(pb->power_supply))
> - initial_blank = FB_BLANK_POWERDOWN;
> -
> pb->pwm = devm_pwm_get(&pdev->dev, NULL);
> if (IS_ERR(pb->pwm) && PTR_ERR(pb->pwm) != -EPROBE_DEFER && !node) {
> dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n");
> @@ -347,7 +365,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> }
>
> bl->props.brightness = data->dft_brightness;
> - bl->props.power = initial_blank;
> + bl->props.power = pwm_backlight_initial_power_state(pb);
> backlight_update_status(bl);
>
> platform_set_drvdata(pdev, bl);

--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

2016-12-09 09:10:53

by Lee Jones

[permalink] [raw]
Subject: Re: [PATCH v4 2/2] backlight: pwm_bl: Check the PWM state for initial backlight power state

On Tue, 22 Nov 2016, Peter Ujfalusi wrote:

> If the PWM is not enabled the backlight initially should not be enabled
> either if we have booted with DT and there is a phandle pointing to the
> backlight node.
>
> The patch extends the checks to decide if we should keep the backlight off
> initially.
>
> Signed-off-by: Peter Ujfalusi <[email protected]>
> Acked-by: Philipp Zabel <[email protected]>
> Reviewed-by: Thierry Reding <[email protected]>
> ---
> drivers/video/backlight/pwm_bl.c | 4 ++++
> 1 file changed, 4 insertions(+)

Applied for v4.11, thanks.

> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index 5712ddd053dd..d7efcb632f7d 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -215,6 +215,10 @@ static int pwm_backlight_initial_power_state(const struct pwm_bl_data *pb)
> if (!regulator_is_enabled(pb->power_supply))
> return FB_BLANK_POWERDOWN;
>
> + /* The PWM is disabled, keep it like this */
> + if (!pwm_is_enabled(pb->pwm))
> + return FB_BLANK_POWERDOWN;
> +
> return FB_BLANK_UNBLANK;
> }
>

--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

2017-03-21 18:58:22

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH v4 1/2] backlight: pwm_bl: Move the checks for initial power state to a separate function

Hi Peter,

On Tue, Nov 22, 2016 at 2:41 PM, Peter Ujfalusi <[email protected]> wrote:
> Move the checks to select the initial state for the backlight to a new
> function and document the checks we are doing.

This is far from a simple "move"...

> With the separate function it is going to be easier to fix or improve the
> initial power state configuration later and it is easier to read the code.
>
> Signed-off-by: Peter Ujfalusi <[email protected]>
> Reviewed-by: Philipp Zabel <[email protected]>
> Reviewed-by: Thierry Reding <[email protected]>

This patch (commit 7613c922315e308a in v4.11-rc1) broke the display on
r8a7740/armadillo.

> @@ -267,20 +292,16 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> pb->enable_gpio = gpio_to_desc(data->enable_gpio);
> }
>
> - if (pb->enable_gpio) {
> - /*
> - * If the driver is probed from the device tree and there is a
> - * phandle link pointing to the backlight node, it is safe to
> - * assume that another driver will enable the backlight at the
> - * appropriate time. Therefore, if it is disabled, keep it so.
> - */
> - if (node && node->phandle &&
> - gpiod_get_direction(pb->enable_gpio) == GPIOF_DIR_OUT &&
> - gpiod_get_value(pb->enable_gpio) == 0)
> - initial_blank = FB_BLANK_POWERDOWN;
> - else
> - gpiod_direction_output(pb->enable_gpio, 1);

In my case, "node" points to the "/backlight" node, but phandle is NULL.
Hence before, gpiod_direction_output() was called to enable the GPIO...

> - }
> + /*
> + * If the GPIO is configured as input, change the direction to output
> + * and set the GPIO as active.
> + * Do not force the GPIO to active when it was already output as it
> + * could cause backlight flickering or we would enable the backlight too
> + * early. Leave the decision of the initial backlight state for later.
> + */
> + if (pb->enable_gpio &&
> + gpiod_get_direction(pb->enable_gpio) == GPIOF_DIR_IN)
> + gpiod_direction_output(pb->enable_gpio, 1);

... while now it's no longer called, as gpiod_get_direction() returns
-EINVAL.

Indeed, r8a7740_pfc does not implement the .get_direction() callback,
so gpiod_get_direction() always returns -EINVAL, which is never equal
to GPIOF_DIR_IN.

Restoring the old behavior by changing the above test to

if (pb->enable_gpio &&
(!node || !node->phandle ||
gpiod_get_direction(pb->enable_gpio) == GPIOF_DIR_IN))

fixes the display for me, but leads to a more complex expression.

However, changing the test to

if (pb->enable_gpio &&
gpiod_get_direction(pb->enable_gpio) != GPIOF_DIR_OUT)

also fixes the display, as an error is always different from GPIOF_DIR_OUT.

Anyone with comments or suggestions to fix this for real?

Thanks!

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2017-03-22 11:06:56

by Philipp Zabel

[permalink] [raw]
Subject: Re: [PATCH v4 1/2] backlight: pwm_bl: Move the checks for initial power state to a separate function

On Tue, 2017-03-21 at 19:48 +0100, Geert Uytterhoeven wrote:
> Hi Peter,
>
> On Tue, Nov 22, 2016 at 2:41 PM, Peter Ujfalusi <[email protected]> wrote:
> > Move the checks to select the initial state for the backlight to a new
> > function and document the checks we are doing.
>
> This is far from a simple "move"...
>
> > With the separate function it is going to be easier to fix or improve the
> > initial power state configuration later and it is easier to read the code.
> >
> > Signed-off-by: Peter Ujfalusi <[email protected]>
> > Reviewed-by: Philipp Zabel <[email protected]>
> > Reviewed-by: Thierry Reding <[email protected]>
>
> This patch (commit 7613c922315e308a in v4.11-rc1) broke the display on
> r8a7740/armadillo.
>
> > @@ -267,20 +292,16 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> > pb->enable_gpio = gpio_to_desc(data->enable_gpio);
> > }
> >
> > - if (pb->enable_gpio) {
> > - /*
> > - * If the driver is probed from the device tree and there is a
> > - * phandle link pointing to the backlight node, it is safe to
> > - * assume that another driver will enable the backlight at the
> > - * appropriate time. Therefore, if it is disabled, keep it so.
> > - */
> > - if (node && node->phandle &&
> > - gpiod_get_direction(pb->enable_gpio) == GPIOF_DIR_OUT &&
> > - gpiod_get_value(pb->enable_gpio) == 0)
> > - initial_blank = FB_BLANK_POWERDOWN;
> > - else
> > - gpiod_direction_output(pb->enable_gpio, 1);
>
> In my case, "node" points to the "/backlight" node, but phandle is NULL.
> Hence before, gpiod_direction_output() was called to enable the GPIO...
>
> > - }
> > + /*
> > + * If the GPIO is configured as input, change the direction to output
> > + * and set the GPIO as active.
> > + * Do not force the GPIO to active when it was already output as it
> > + * could cause backlight flickering or we would enable the backlight too
> > + * early. Leave the decision of the initial backlight state for later.
> > + */
> > + if (pb->enable_gpio &&
> > + gpiod_get_direction(pb->enable_gpio) == GPIOF_DIR_IN)
> > + gpiod_direction_output(pb->enable_gpio, 1);
>
> ... while now it's no longer called, as gpiod_get_direction() returns
> -EINVAL.
>
> Indeed, r8a7740_pfc does not implement the .get_direction() callback,
> so gpiod_get_direction() always returns -EINVAL, which is never equal
> to GPIOF_DIR_IN.

Oh, I didn't think about this at all, anymore. Though I believe to
remember that this was the reason that I checked for
(gpiod_get_direction(pb->enable_gpio) == GPIOF_DIR_OUT) before, so ...

> Restoring the old behavior by changing the above test to
>
> if (pb->enable_gpio &&
> (!node || !node->phandle ||
> gpiod_get_direction(pb->enable_gpio) == GPIOF_DIR_IN))
>
> fixes the display for me, but leads to a more complex expression.
>
> However, changing the test to
>
> if (pb->enable_gpio &&
> gpiod_get_direction(pb->enable_gpio) != GPIOF_DIR_OUT)
>
> also fixes the display, as an error is always different from GPIOF_DIR_OUT.
>
> Anyone with comments or suggestions to fix this for real?

... I'm in favor of the latter, as this is closer to the initial
intention. I'd also mention this in the comment:

/*
* If the GPIO is not known to be already configured as output, that is,
* if gpiod_get_direction returns either GPIOF_DIR_IN or -EINVAL, change
* the direction to output and set the GPIO as active.
* Do not force the GPIO to active when it was already output as it
* could cause backlight flickering or we would enable the backlight too
* early. Leave the decision of the initial backlight state for later.
*/

regards
Philipp

2017-03-22 12:34:50

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH v4 1/2] backlight: pwm_bl: Move the checks for initial power state to a separate function

Hi Philip,

On Wed, Mar 22, 2017 at 11:46 AM, Philipp Zabel <[email protected]> wrote:
> On Tue, 2017-03-21 at 19:48 +0100, Geert Uytterhoeven wrote:
>> On Tue, Nov 22, 2016 at 2:41 PM, Peter Ujfalusi <[email protected]> wrote:
>> > Move the checks to select the initial state for the backlight to a new
>> > function and document the checks we are doing.
>>
>> This is far from a simple "move"...
>>
>> > With the separate function it is going to be easier to fix or improve the
>> > initial power state configuration later and it is easier to read the code.
>> >
>> > Signed-off-by: Peter Ujfalusi <[email protected]>
>> > Reviewed-by: Philipp Zabel <[email protected]>
>> > Reviewed-by: Thierry Reding <[email protected]>
>>
>> This patch (commit 7613c922315e308a in v4.11-rc1) broke the display on
>> r8a7740/armadillo.
>>
>> > @@ -267,20 +292,16 @@ static int pwm_backlight_probe(struct platform_device *pdev)
>> > pb->enable_gpio = gpio_to_desc(data->enable_gpio);
>> > }
>> >
>> > - if (pb->enable_gpio) {
>> > - /*
>> > - * If the driver is probed from the device tree and there is a
>> > - * phandle link pointing to the backlight node, it is safe to
>> > - * assume that another driver will enable the backlight at the
>> > - * appropriate time. Therefore, if it is disabled, keep it so.
>> > - */
>> > - if (node && node->phandle &&
>> > - gpiod_get_direction(pb->enable_gpio) == GPIOF_DIR_OUT &&
>> > - gpiod_get_value(pb->enable_gpio) == 0)
>> > - initial_blank = FB_BLANK_POWERDOWN;
>> > - else
>> > - gpiod_direction_output(pb->enable_gpio, 1);
>>
>> In my case, "node" points to the "/backlight" node, but phandle is NULL.
>> Hence before, gpiod_direction_output() was called to enable the GPIO...
>>
>> > - }
>> > + /*
>> > + * If the GPIO is configured as input, change the direction to output
>> > + * and set the GPIO as active.
>> > + * Do not force the GPIO to active when it was already output as it
>> > + * could cause backlight flickering or we would enable the backlight too
>> > + * early. Leave the decision of the initial backlight state for later.
>> > + */
>> > + if (pb->enable_gpio &&
>> > + gpiod_get_direction(pb->enable_gpio) == GPIOF_DIR_IN)
>> > + gpiod_direction_output(pb->enable_gpio, 1);
>>
>> ... while now it's no longer called, as gpiod_get_direction() returns
>> -EINVAL.
>>
>> Indeed, r8a7740_pfc does not implement the .get_direction() callback,
>> so gpiod_get_direction() always returns -EINVAL, which is never equal
>> to GPIOF_DIR_IN.
>
> Oh, I didn't think about this at all, anymore. Though I believe to
> remember that this was the reason that I checked for
> (gpiod_get_direction(pb->enable_gpio) == GPIOF_DIR_OUT) before, so ...
>
>> Restoring the old behavior by changing the above test to
>>
>> if (pb->enable_gpio &&
>> (!node || !node->phandle ||
>> gpiod_get_direction(pb->enable_gpio) == GPIOF_DIR_IN))
>>
>> fixes the display for me, but leads to a more complex expression.
>>
>> However, changing the test to
>>
>> if (pb->enable_gpio &&
>> gpiod_get_direction(pb->enable_gpio) != GPIOF_DIR_OUT)
>>
>> also fixes the display, as an error is always different from GPIOF_DIR_OUT.
>>
>> Anyone with comments or suggestions to fix this for real?
>
> ... I'm in favor of the latter, as this is closer to the initial
> intention. I'd also mention this in the comment:
>
> /*
> * If the GPIO is not known to be already configured as output, that is,
> * if gpiod_get_direction returns either GPIOF_DIR_IN or -EINVAL, change
> * the direction to output and set the GPIO as active.
> * Do not force the GPIO to active when it was already output as it
> * could cause backlight flickering or we would enable the backlight too
> * early. Leave the decision of the initial backlight state for later.
> */

Thanks, I'll cook up a patch.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds