linux-kernel - Re: [PATCH] backlight: led_bl: Add support for an "enable" GPIO

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20211102111942.xd7eqz2zrtb7zotc@maple.lan>
Date:   Tue, 2 Nov 2021 11:19:42 +0000
From:   Daniel Thompson <daniel.thompson@...aro.org>
To:     Corentin LABBE <clabbe@...libre.com>
Cc:     lee.jones@...aro.org, jingoohan1@...il.com,
        dri-devel@...ts.freedesktop.org, linux-fbdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, khilman@...libre.com,
        Jean-Jacques Hiblot <jjhiblot@...com>,
        Pavel Machek <pavel@....cz>
Subject: Re: [PATCH] backlight: led_bl: Add support for an "enable" GPIO

On Tue, Nov 02, 2021 at 10:04:55AM +0000, Corentin LABBE wrote:
> From: Jean-Jacques Hiblot <jjhiblot@...com>
> 
> This patch adds support for an "enable GPIO".

Before taking this kind of change is there a good reason why backlight
should manage the GPIO? I thought that the LED subsystem was a sub
system for LEDs (not LED controllers). In other words if you direct
that the LED be lit up then isn't it the LED driver's job to manage
the GPIO and ensure that it lights up?


Daniel.


> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@...com>
> Signed-off-by: Corentin LABBE <clabbe@...libre.com>
> ---
>  drivers/video/backlight/led_bl.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/video/backlight/led_bl.c b/drivers/video/backlight/led_bl.c
> index f54d256e2d54..ebd7acc32980 100644
> --- a/drivers/video/backlight/led_bl.c
> +++ b/drivers/video/backlight/led_bl.c
> @@ -8,6 +8,7 @@
>  
>  #include <linux/backlight.h>
>  #include <linux/leds.h>
> +#include <linux/gpio.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  
> @@ -15,6 +16,7 @@ struct led_bl_data {
>  	struct device		*dev;
>  	struct backlight_device	*bl_dev;
>  	struct led_classdev	**leds;
> +	struct gpio_desc        *enable_gpio;
>  	bool			enabled;
>  	int			nb_leds;
>  	unsigned int		*levels;
> @@ -35,6 +37,9 @@ static void led_bl_set_brightness(struct led_bl_data *priv, int level)
>  	for (i = 0; i < priv->nb_leds; i++)
>  		led_set_brightness(priv->leds[i], bkl_brightness);
>  
> +	if (!priv->enabled)
> +		gpiod_set_value_cansleep(priv->enable_gpio, 1);
> +
>  	priv->enabled = true;
>  }
>  
> @@ -48,6 +53,9 @@ static void led_bl_power_off(struct led_bl_data *priv)
>  	for (i = 0; i < priv->nb_leds; i++)
>  		led_set_brightness(priv->leds[i], LED_OFF);
>  
> +	if (priv->enabled)
> +		gpiod_set_value_cansleep(priv->enable_gpio, 0);
> +
>  	priv->enabled = false;
>  }
>  
> @@ -209,6 +217,11 @@ static int led_bl_probe(struct platform_device *pdev)
>  		return PTR_ERR(priv->bl_dev);
>  	}
>  
> +	priv->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
> +						    GPIOD_OUT_LOW);
> +	if (IS_ERR(priv->enable_gpio))
> +		return PTR_ERR(priv->enable_gpio);
> +
>  	for (i = 0; i < priv->nb_leds; i++)
>  		led_sysfs_disable(priv->leds[i]);
>  
> -- 
> 2.25.1
>