[PATCHv7][ 1/2] backlight: gpio_backlight: Use a default state enum.

[PATCHv7][ 1/2] backlight: gpio_backlight: Use a default state enum.

[Denis Carikli]

That enum adds a "keep" state which permits to tell the
  driver trough its platform data not to touch the
  hardware during the probe.

Cc: Richard Purdie <rpurdie@rpsys.net>
Cc: Jingoo Han <jg1.han@samsung.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
Cc: devicetree at vger.kernel.org
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: linux-arm-kernel at lists.infradead.org
Cc: Lothar Wa?mann <LW@KARO-electronics.de>
Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Eric B?nard <eric@eukrea.com>
Signed-off-by: Denis Carikli <denis@eukrea.com>
---
ChangeLog v5->v6:
- New patch.
---
 drivers/video/backlight/gpio_backlight.c     |    7 +++++--
 include/linux/platform_data/gpio_backlight.h |    6 ++++++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/video/backlight/gpio_backlight.c b/drivers/video/backlight/gpio_backlight.c
index 81fb127..6ddeba9 100644
--- a/drivers/video/backlight/gpio_backlight.c
+++ b/drivers/video/backlight/gpio_backlight.c
@@ -23,6 +23,7 @@ struct gpio_backlight {
 
 	int gpio;
 	int active;
+	enum gpio_backlight_default_state def_value;
 };
 
 static int gpio_backlight_update_status(struct backlight_device *bl)
@@ -103,8 +104,10 @@ static int gpio_backlight_probe(struct platform_device *pdev)
 		return PTR_ERR(bl);
 	}
 
-	bl->props.brightness = pdata->def_value;
-	backlight_update_status(bl);
+	if (pdata->def_value != BACKLIGHT_GPIO_DEFSTATE_KEEP) {
+		bl->props.brightness = pdata->def_value;
+		backlight_update_status(bl);
+	}
 
 	platform_set_drvdata(pdev, bl);
 	return 0;
diff --git a/include/linux/platform_data/gpio_backlight.h b/include/linux/platform_data/gpio_backlight.h
index 5ae0d9c..3b437b3 100644
--- a/include/linux/platform_data/gpio_backlight.h
+++ b/include/linux/platform_data/gpio_backlight.h
@@ -10,6 +10,12 @@
 
 struct device;
 
+enum gpio_backlight_default_state {
+	BACKLIGHT_GPIO_DEFSTATE_OFF,
+	BACKLIGHT_GPIO_DEFSTATE_ON,
+	BACKLIGHT_GPIO_DEFSTATE_KEEP,
+};
+
 struct gpio_backlight_platform_data {
 	struct device *fbdev;
 	int gpio;
-- 
1.7.9.5

[PATCHv7][ 2/2] video: backlight: gpio-backlight: Add DT support.

[Denis Carikli]

Cc: Richard Purdie <rpurdie@rpsys.net>
Cc: Jingoo Han <jg1.han@samsung.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
Cc: devicetree at vger.kernel.org
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: linux-arm-kernel at lists.infradead.org
Cc: Lothar Wa?mann <LW@KARO-electronics.de>
Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Eric B?nard <eric@eukrea.com>
Signed-off-by: Denis Carikli <denis@eukrea.com>
---
ChangeLog v6->v7:
- removed a compilation warning with the removal of the useless ret declaration.

ChangeLog v5->v6:
- The default state handling was reworked:
  - it's now called default-state, and looks like the gpio-leds default-state.
  - it now has a "keep" option, like for the gpio-leds.
  - that "keep" option is the default when the default-state property is not set.
- The documentation was updated accordingly.

ChangeLog v4->v5:
- The default-brightness property now defaults to 0 in the driver.
- def_value int becomes a bool.
- The check for the gpio validity has been reworked.
---
 .../bindings/video/backlight/gpio-backlight.txt    |   23 +++++++
 drivers/video/backlight/gpio_backlight.c           |   71 +++++++++++++++++---
 2 files changed, 86 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/video/backlight/gpio-backlight.txt

diff --git a/Documentation/devicetree/bindings/video/backlight/gpio-backlight.txt b/Documentation/devicetree/bindings/video/backlight/gpio-backlight.txt
new file mode 100644
index 0000000..f36f6e3
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/backlight/gpio-backlight.txt
@@ -0,0 +1,23 @@
+gpio-backlight bindings
+
+Required properties:
+  - compatible: "gpio-backlight"
+  - gpios: describes the gpio that is used for enabling/disabling the backlight
+     (see GPIO binding[0] for more details).
+
+Optional properties:
+  - default-state: The initial state of the backlight.
+    Valid values are "on", "off", and "keep".
+    The "keep" setting will keep the backlight at whatever its current
+    state is, without producing a glitch. The default is keep if this
+    property is not present.
+
+[0]: Documentation/devicetree/bindings/gpio/gpio.txt
+
+Example:
+
+	backlight {
+		compatible = "gpio-backlight";
+		gpios = <&gpio3 4 0>;
+		default-state = "on";
+	};
diff --git a/drivers/video/backlight/gpio_backlight.c b/drivers/video/backlight/gpio_backlight.c
index 6ddeba9..2e20e32 100644
--- a/drivers/video/backlight/gpio_backlight.c
+++ b/drivers/video/backlight/gpio_backlight.c
@@ -13,6 +13,8 @@
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
 #include <linux/platform_data/gpio_backlight.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
@@ -61,6 +63,42 @@ static const struct backlight_ops gpio_backlight_ops = {
 	.check_fb	= gpio_backlight_check_fb,
 };
 
+static int gpio_backlight_probe_dt(struct platform_device *pdev,
+				   struct gpio_backlight *gbl)
+{
+	struct device_node *np = pdev->dev.of_node;
+	enum of_gpio_flags gpio_flags;
+	const char *state;
+
+	gbl->fbdev = NULL;
+	gbl->gpio = of_get_gpio_flags(np, 0, &gpio_flags);
+
+	if (!gpio_is_valid(gbl->gpio)) {
+		if (gbl->gpio != -EPROBE_DEFER) {
+			dev_err(&pdev->dev,
+				"Error: The gpios parameter is missing or invalid.\n");
+		}
+		return gbl->gpio;
+	}
+
+	gbl->active = (gpio_flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1;
+
+	state = of_get_property(np, "default-state", NULL);
+	if (state) {
+		if (!strcmp(state, "keep"))
+			gbl->def_value = BACKLIGHT_GPIO_DEFSTATE_KEEP;
+		else if (!strcmp(state, "on"))
+			gbl->def_value = BACKLIGHT_GPIO_DEFSTATE_ON;
+		else
+			gbl->def_value = BACKLIGHT_GPIO_DEFSTATE_OFF;
+	} else {
+		/* we don't want to touch the hardware if we're not told to */
+		gbl->def_value = BACKLIGHT_GPIO_DEFSTATE_KEEP;
+	}
+
+	return 0;
+}
+
 static int gpio_backlight_probe(struct platform_device *pdev)
 {
 	struct gpio_backlight_platform_data *pdata =
@@ -68,10 +106,12 @@ static int gpio_backlight_probe(struct platform_device *pdev)
 	struct backlight_properties props;
 	struct backlight_device *bl;
 	struct gpio_backlight *gbl;
+	struct device_node *np = pdev->dev.of_node;
 	int ret;
 
-	if (!pdata) {
-		dev_err(&pdev->dev, "failed to find platform data\n");
+	if (!pdata && !np) {
+		dev_err(&pdev->dev,
+			"failed to find platform data or device tree node.\n");
 		return -ENODEV;
 	}
 
@@ -80,14 +120,23 @@ static int gpio_backlight_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	gbl->dev = &pdev->dev;
-	gbl->fbdev = pdata->fbdev;
-	gbl->gpio = pdata->gpio;
-	gbl->active = pdata->active_low ? 0 : 1;
+
+	if (np) {
+		ret = gpio_backlight_probe_dt(pdev, gbl);
+		if (ret)
+			return ret;
+	} else {
+		gbl->fbdev = pdata->fbdev;
+		gbl->gpio = pdata->gpio;
+		gbl->active = pdata->active_low ? 0 : 1;
+		if (pdata->def_value != BACKLIGHT_GPIO_DEFSTATE_KEEP)
+			gbl->def_value = pdata->def_value;
+	}
 
 	ret = devm_gpio_request_one(gbl->dev, gbl->gpio, GPIOF_DIR_OUT |
 				    (gbl->active ? GPIOF_INIT_LOW
 						 : GPIOF_INIT_HIGH),
-				    pdata->name);
+				    pdata ? pdata->name : "backlight");
 	if (ret < 0) {
 		dev_err(&pdev->dev, "unable to request GPIO\n");
 		return ret;
@@ -104,8 +153,8 @@ static int gpio_backlight_probe(struct platform_device *pdev)
 		return PTR_ERR(bl);
 	}
 
-	if (pdata->def_value != BACKLIGHT_GPIO_DEFSTATE_KEEP) {
-		bl->props.brightness = pdata->def_value;
+	if (gbl->def_value != BACKLIGHT_GPIO_DEFSTATE_KEEP) {
+		bl->props.brightness = gbl->def_value;
 		backlight_update_status(bl);
 	}
 
@@ -113,10 +162,16 @@ static int gpio_backlight_probe(struct platform_device *pdev)
 	return 0;
 }
 
+static struct of_device_id gpio_backlight_of_match[] = {
+	{ .compatible = "gpio-backlight" },
+	{ /* sentinel */ }
+};
+
 static struct platform_driver gpio_backlight_driver = {
 	.driver		= {
 		.name		= "gpio-backlight",
 		.owner		= THIS_MODULE,
+		.of_match_table = of_match_ptr(gpio_backlight_of_match),
 	},
 	.probe		= gpio_backlight_probe,
 };
-- 
1.7.9.5

[PATCHv7][ 2/2] video: backlight: gpio-backlight: Add DT support.

[Thierry Reding]

On Thu, Dec 05, 2013 at 06:55:09PM +0100, Denis Carikli wrote:
[...]
> +Optional properties:
> +  - default-state: The initial state of the backlight.
> +    Valid values are "on", "off", and "keep".
> +    The "keep" setting will keep the backlight at whatever its current
> +    state is, without producing a glitch. The default is keep if this
> +    property is not present.

I'm not sure if "on", "off" and "keep" are a good choice for this
binding. Having strings for these tristate values seems suboptimal.
Other bindings have chosen a representation that, transposed to this
use-case, would read something like this:

	- default-state: The initial state of the backlight. Valid
	  values:
	  - 0: off
	  - 1: on

	If the "default-state" property is not present, the default
	will be to keep the current backlight state.

Which is in fact the exact behaviour that your binding describes, but
it's much more intuitive in my opinion.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131206/fd9e72af/attachment.sig>

Re: [PATCHv7][ 2/2] video: backlight: gpio-backlight: Add DT support.

[Alexander Shiyan]

> On Thu, Dec 05, 2013 at 06:55:09PM +0100, Denis Carikli wrote:
> [...]
> > +Optional properties:
> > +  - default-state: The initial state of the backlight.
> > +    Valid values are "on", "off", and "keep".
> > +    The "keep" setting will keep the backlight at whatever its current
> > +    state is, without producing a glitch. The default is keep if this
> > +    property is not present.
> 
> I'm not sure if "on", "off" and "keep" are a good choice for this
> binding. Having strings for these tristate values seems suboptimal.
> Other bindings have chosen a representation that, transposed to this
> use-case, would read something like this:
> 
> 	- default-state: The initial state of the backlight. Valid
> 	  values:
> 	  - 0: off
> 	  - 1: on
> 
> 	If the "default-state" property is not present, the default
> 	will be to keep the current backlight state.
> 
> Which is in fact the exact behaviour that your binding describes, but
> it's much more intuitive in my opinion.

Why we cannot use GPIO bindings for active level here?
What a reason for "keep" state? Can this be an additional property?

---

[PATCHv7][ 2/2] video: backlight: gpio-backlight: Add DT support.

[Thierry Reding]

On Fri, Dec 06, 2013 at 05:08:38PM +0400, Alexander Shiyan wrote:
> > On Thu, Dec 05, 2013 at 06:55:09PM +0100, Denis Carikli wrote:
> > [...]
> > > +Optional properties:
> > > +  - default-state: The initial state of the backlight.
> > > +    Valid values are "on", "off", and "keep".
> > > +    The "keep" setting will keep the backlight at whatever its current
> > > +    state is, without producing a glitch. The default is keep if this
> > > +    property is not present.
> > 
> > I'm not sure if "on", "off" and "keep" are a good choice for this
> > binding. Having strings for these tristate values seems suboptimal.
> > Other bindings have chosen a representation that, transposed to this
> > use-case, would read something like this:
> > 
> > 	- default-state: The initial state of the backlight. Valid
> > 	  values:
> > 	  - 0: off
> > 	  - 1: on
> > 
> > 	If the "default-state" property is not present, the default
> > 	will be to keep the current backlight state.
> > 
> > Which is in fact the exact behaviour that your binding describes, but
> > it's much more intuitive in my opinion.
> 
> Why we cannot use GPIO bindings for active level here?
> What a reason for "keep" state? Can this be an additional property?

Default state and active level are two different things.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131206/6b4b0460/attachment.sig>

Re: [PATCHv7][ 2/2] video: backlight: gpio-backlight: Add DT support.

[Alexander Shiyan]

> On Fri, Dec 06, 2013 at 05:08:38PM +0400, Alexander Shiyan wrote:
> > > On Thu, Dec 05, 2013 at 06:55:09PM +0100, Denis Carikli wrote:
> > > [...]
> > > > +Optional properties:
> > > > +  - default-state: The initial state of the backlight.
> > > > +    Valid values are "on", "off", and "keep".
> > > > +    The "keep" setting will keep the backlight at whatever its current
> > > > +    state is, without producing a glitch. The default is keep if this
> > > > +    property is not present.
> > > 
> > > I'm not sure if "on", "off" and "keep" are a good choice for this
> > > binding. Having strings for these tristate values seems suboptimal.
> > > Other bindings have chosen a representation that, transposed to this
> > > use-case, would read something like this:
> > > 
> > > 	- default-state: The initial state of the backlight. Valid
> > > 	  values:
> > > 	  - 0: off
> > > 	  - 1: on
> > > 
> > > 	If the "default-state" property is not present, the default
> > > 	will be to keep the current backlight state.
> > > 
> > > Which is in fact the exact behaviour that your binding describes, but
> > > it's much more intuitive in my opinion.
> > 
> > Why we cannot use GPIO bindings for active level here?
> > What a reason for "keep" state? Can this be an additional property?
> 
> Default state and active level are two different things.

OK. And what is "default" state? State before "unblank"?

---