To be able to handle the HWEN pin of the lm3630a, add an enable gpio to the driver and a property.
Tested on Kobo Clara HD.
Changes in v2: simplification and reordering
Andreas Kemnade (2): dt-bindings: backlight: lm3630a: add enable_gpios backlight: lm3630a: add an enable gpio for the HWEN pin
.../bindings/leds/backlight/lm3630a-backlight.yaml | 5 +++++ drivers/video/backlight/lm3630a_bl.c | 10 ++++++++++ 2 files changed, 15 insertions(+)
add enable-gpios to describe HWEN pin
Signed-off-by: Andreas Kemnade andreas@kemnade.info --- changes in v2: add example .../bindings/leds/backlight/lm3630a-backlight.yaml | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.yaml b/Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.yaml index dc129d9a329e..1fa83feffe16 100644 --- a/Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.yaml +++ b/Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.yaml @@ -29,6 +29,10 @@ properties: '#size-cells': const: 0
+ enable-gpios: + description: GPIO to use to enable/disable the backlight (HWEN pin). + maxItems: 1 + required: - compatible - reg @@ -92,6 +96,7 @@ examples: i2c { #address-cells = <1>; #size-cells = <0>; + enable-gpios = <&gpio2 5 GPIO_ACTIVE_HIGH>;
led-controller@38 { compatible = "ti,lm3630a";
On Tue, Sep 10, 2019 at 11:29:08PM +0200, Andreas Kemnade wrote:
add enable-gpios to describe HWEN pin
Signed-off-by: Andreas Kemnade andreas@kemnade.info
Acked-by: Daniel Thompson daniel.thompson@linaro.org
changes in v2: add example .../bindings/leds/backlight/lm3630a-backlight.yaml | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.yaml b/Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.yaml index dc129d9a329e..1fa83feffe16 100644 --- a/Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.yaml +++ b/Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.yaml @@ -29,6 +29,10 @@ properties: '#size-cells': const: 0
- enable-gpios:
- description: GPIO to use to enable/disable the backlight (HWEN pin).
- maxItems: 1
required:
- compatible
- reg
@@ -92,6 +96,7 @@ examples: i2c { #address-cells = <1>; #size-cells = <0>;
enable-gpios = <&gpio2 5 GPIO_ACTIVE_HIGH>; led-controller@38 { compatible = "ti,lm3630a";-- 2.20.1
Andreas
On 9/11/19 5:08 AM, Daniel Thompson wrote:
On Tue, Sep 10, 2019 at 11:29:08PM +0200, Andreas Kemnade wrote:
add enable-gpios to describe HWEN pin
Signed-off-by: Andreas Kemnade andreas@kemnade.info
Acked-by: Daniel Thompson daniel.thompson@linaro.org
changes in v2: add example .../bindings/leds/backlight/lm3630a-backlight.yaml | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.yaml b/Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.yaml index dc129d9a329e..1fa83feffe16 100644 --- a/Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.yaml +++ b/Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.yaml @@ -29,6 +29,10 @@ properties: '#size-cells': const: 0
- enable-gpios:
- description: GPIO to use to enable/disable the backlight (HWEN pin).
- maxItems: 1
- required:
- compatible
- reg
@@ -92,6 +96,7 @@ examples: i2c { #address-cells = <1>; #size-cells = <0>;
enable-gpios = <&gpio2 5 GPIO_ACTIVE_HIGH>;
This is in the wrong place. This is implying that the gpio is for the i2c parent
This needs to go under the led-controller node below
Dan
led-controller@38 { compatible = "ti,lm3630a";-- 2.20.1
For now just enable it in the probe function to allow i2c access. Disabling also means resetting the register values to default and according to the datasheet does not give power savings
Tested on Kobo Clara HD.
Signed-off-by: Andreas Kemnade andreas@kemnade.info --- changes in v2: - simplification - correct gpio direction initialisation
drivers/video/backlight/lm3630a_bl.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/drivers/video/backlight/lm3630a_bl.c b/drivers/video/backlight/lm3630a_bl.c index 8f84f3684f04..9d0639d4202d 100644 --- a/drivers/video/backlight/lm3630a_bl.c +++ b/drivers/video/backlight/lm3630a_bl.c @@ -12,6 +12,8 @@ #include <linux/uaccess.h> #include <linux/interrupt.h> #include <linux/regmap.h> +#include <linux/gpio/consumer.h> +#include <linux/gpio.h> #include <linux/pwm.h> #include <linux/platform_data/lm3630a_bl.h>
@@ -48,6 +50,7 @@ struct lm3630a_chip { struct lm3630a_platform_data *pdata; struct backlight_device *bleda; struct backlight_device *bledb; + struct gpio_desc *enable_gpio; struct regmap *regmap; struct pwm_device *pwmd; }; @@ -535,6 +538,13 @@ static int lm3630a_probe(struct i2c_client *client, } pchip->pdata = pdata;
+ pchip->enable_gpio = devm_gpiod_get_optional(&client->dev, "enable", + GPIOD_OUT_HIGH); + if (IS_ERR(pchip->enable_gpio)) { + rval = PTR_ERR(pchip->enable_gpio); + return rval; + } + /* chip initialize */ rval = lm3630a_chip_init(pchip); if (rval < 0) {
On Tue, Sep 10, 2019 at 11:29:09PM +0200, Andreas Kemnade wrote:
For now just enable it in the probe function to allow i2c access. Disabling also means resetting the register values to default and according to the datasheet does not give power savings
Tested on Kobo Clara HD.
Signed-off-by: Andreas Kemnade andreas@kemnade.info
changes in v2:
- simplification
- correct gpio direction initialisation
drivers/video/backlight/lm3630a_bl.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/drivers/video/backlight/lm3630a_bl.c b/drivers/video/backlight/lm3630a_bl.c index 8f84f3684f04..9d0639d4202d 100644 --- a/drivers/video/backlight/lm3630a_bl.c +++ b/drivers/video/backlight/lm3630a_bl.c @@ -12,6 +12,8 @@ #include <linux/uaccess.h> #include <linux/interrupt.h> #include <linux/regmap.h> +#include <linux/gpio/consumer.h> +#include <linux/gpio.h>
Nitpicking... but I don't think linux/gpio.h is used anymore.
#include <linux/pwm.h> #include <linux/platform_data/lm3630a_bl.h>
@@ -48,6 +50,7 @@ struct lm3630a_chip { struct lm3630a_platform_data *pdata; struct backlight_device *bleda; struct backlight_device *bledb;
- struct gpio_desc *enable_gpio; struct regmap *regmap; struct pwm_device *pwmd;
}; @@ -535,6 +538,13 @@ static int lm3630a_probe(struct i2c_client *client, } pchip->pdata = pdata;
- pchip->enable_gpio = devm_gpiod_get_optional(&client->dev, "enable",
GPIOD_OUT_HIGH);- if (IS_ERR(pchip->enable_gpio)) {
rval = PTR_ERR(pchip->enable_gpio);return rval;- }
- /* chip initialize */ rval = lm3630a_chip_init(pchip); if (rval < 0) {
-- 2.20.1
On 9/11/19 5:25 AM, Daniel Thompson wrote:
On Tue, Sep 10, 2019 at 11:29:09PM +0200, Andreas Kemnade wrote:
For now just enable it in the probe function to allow i2c access. Disabling also means resetting the register values to default and according to the datasheet does not give power savings
Tested on Kobo Clara HD.
Signed-off-by: Andreas Kemnade andreas@kemnade.info
changes in v2:
simplification
correct gpio direction initialisation
drivers/video/backlight/lm3630a_bl.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/drivers/video/backlight/lm3630a_bl.c b/drivers/video/backlight/lm3630a_bl.c index 8f84f3684f04..9d0639d4202d 100644 --- a/drivers/video/backlight/lm3630a_bl.c +++ b/drivers/video/backlight/lm3630a_bl.c @@ -12,6 +12,8 @@ #include <linux/uaccess.h> #include <linux/interrupt.h> #include <linux/regmap.h> +#include <linux/gpio/consumer.h> +#include <linux/gpio.h>
Nitpicking... but I don't think linux/gpio.h is used anymore.
#include <linux/pwm.h> #include <linux/platform_data/lm3630a_bl.h>
@@ -48,6 +50,7 @@ struct lm3630a_chip { struct lm3630a_platform_data *pdata; struct backlight_device *bleda; struct backlight_device *bledb;
- struct gpio_desc *enable_gpio; struct regmap *regmap; struct pwm_device *pwmd; };
@@ -535,6 +538,13 @@ static int lm3630a_probe(struct i2c_client *client, } pchip->pdata = pdata;
- pchip->enable_gpio = devm_gpiod_get_optional(&client->dev, "enable",
GPIOD_OUT_HIGH);- if (IS_ERR(pchip->enable_gpio)) {
rval = PTR_ERR(pchip->enable_gpio);return rval;
the enable gpio is optional so if it fails you log the error and move on
Also on driver removal did you want to set the GPIO to low to disable the device to save power?
Dan
- }
- /* chip initialize */ rval = lm3630a_chip_init(pchip); if (rval < 0) {
-- 2.20.1
On Wed, 11 Sep 2019 13:48:36 -0500 Dan Murphy dmurphy@ti.com wrote:
@@ -535,6 +538,13 @@ static int lm3630a_probe(struct i2c_client *client, } pchip->pdata = pdata;
- pchip->enable_gpio = devm_gpiod_get_optional(&client->dev, "enable",
GPIOD_OUT_HIGH);- if (IS_ERR(pchip->enable_gpio)) {
rval = PTR_ERR(pchip->enable_gpio);return rval;the enable gpio is optional so if it fails you log the error and move on
well, if the gpio is not there, then it returns NULL. It might return e.g. -EDEFER. So I need to check for errors here.
Also on driver removal did you want to set the GPIO to low to disable the device to save power?
page 5 of the datasheet says:
Ishdn = Typ. 1µA max. 4µA. For HWEN=Vin, I2c shutdown (I guess this means outputs powered off) ond for HWEN=GND.
So are we really saving something here?
Regards, Andreas
On Wed, Sep 11, 2019 at 01:48:36PM -0500, Dan Murphy wrote:
On 9/11/19 5:25 AM, Daniel Thompson wrote:
On Tue, Sep 10, 2019 at 11:29:09PM +0200, Andreas Kemnade wrote:
For now just enable it in the probe function to allow i2c access. Disabling also means resetting the register values to default and according to the datasheet does not give power savings
Tested on Kobo Clara HD.
Signed-off-by: Andreas Kemnade andreas@kemnade.info
changes in v2:
simplification
correct gpio direction initialisation
drivers/video/backlight/lm3630a_bl.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/drivers/video/backlight/lm3630a_bl.c b/drivers/video/backlight/lm3630a_bl.c index 8f84f3684f04..9d0639d4202d 100644 --- a/drivers/video/backlight/lm3630a_bl.c +++ b/drivers/video/backlight/lm3630a_bl.c @@ -12,6 +12,8 @@ #include <linux/uaccess.h> #include <linux/interrupt.h> #include <linux/regmap.h> +#include <linux/gpio/consumer.h> +#include <linux/gpio.h>
Nitpicking... but I don't think linux/gpio.h is used anymore.
#include <linux/pwm.h> #include <linux/platform_data/lm3630a_bl.h> @@ -48,6 +50,7 @@ struct lm3630a_chip { struct lm3630a_platform_data *pdata; struct backlight_device *bleda; struct backlight_device *bledb;
- struct gpio_desc *enable_gpio; struct regmap *regmap; struct pwm_device *pwmd; };
@@ -535,6 +538,13 @@ static int lm3630a_probe(struct i2c_client *client, } pchip->pdata = pdata;
- pchip->enable_gpio = devm_gpiod_get_optional(&client->dev, "enable",
GPIOD_OUT_HIGH);- if (IS_ERR(pchip->enable_gpio)) {
rval = PTR_ERR(pchip->enable_gpio);return rval;the enable gpio is optional so if it fails you log the error and move on
Isn't the effect of this to cope gracefully if enable-gpios is absent but to fail with an error if enable-gpios exists and is broken. I thought this code pattern is fairly common.
Also on driver removal did you want to set the GPIO to low to disable the device to save power?
As it happens I offered to opposite feedback for v1: https://lists.freedesktop.org/archives/dri-devel/2019-September/234918.html
Basically if the power matters then we should take care of things in the PM code path (which for this driver means reacting properly to suspended flag when updating the brightness). If the power doesn't matter then, given unallocated GPIO pins are in an unknown state anyway, there is no point in tidying up because we don't know what value to restore.
Daniel.
dri-devel@lists.freedesktop.org
-
Andreas Kemnade -
Dan Murphy -
Daniel Thompson