[PATCH RFC 0/2] simplefb: Add regulator handling support

Hi everyone,

This series adds regulator claiming and enabling support for simplefb.

Sometimes the simplefb display output path consits of external conversion
chips and/or LCD drivers and backlights. These devices normally have
GPIOs to turn them on and/or bring them out of reset, and regulators
supplying power to them.

While the kernel does not touch unclaimed GPIOs, the regulator core
happily disables unused regulators. Thus we need simplefb to claim
and enable the regulators used throughout the display pipeline.

Now the DT bindings don't support a list of regulators directly, so
I'm working around it by having a "num-supplies" property to specify
the number of supply properties to check, and name the actual supplies
as "vinN-supply".

Hans, Maxime, AXP223 support for the A23/A33 Q8 tablets will need this
to keep the LCD on. The Primo81 needs this as well.


Patch 1 adds the regulator properties to the DT binding.

Patch 2 adds code to the simplefb driver to claim and enable regulators.


Regards
ChenYu


Chen-Yu Tsai (2):
  dt-bindings: simplefb: Support a list of regulator supply properties
  simplefb: Claim and enable regulators

 .../bindings/video/simple-framebuffer.txt          |  14 ++-
 drivers/video/fbdev/simplefb.c                     | 102 ++++++++++++++++++++-
 2 files changed, 111 insertions(+), 5 deletions(-)

-- 
2.5.3

The physical display tied to the framebuffer may have regulators
providing power to it, such as power for LCDs or interface conversion
chips.

The number of regulators in use may vary, but the regulator supply
binding can not be a list. Work around this by adding a "num-supplies"
property to communicate the number of supplies, and a list of 0 ~ N
"vinN-supply" properties for the actual regulator supply.

Signed-off-by: Chen-Yu Tsai <redacted>
---
 .../devicetree/bindings/video/simple-framebuffer.txt       | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
index 4474ef6e0b95..0cc43e1be8b5 100644
--- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
+++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt

@@ -47,10 +47,14 @@ Required properties:
   - a8b8g8r8 (32-bit pixels, d[31:24]=a, d[23:16]=b, d[15:8]=g, d[7:0]=r).
 
 Optional properties:
-- clocks : List of clocks used by the framebuffer. Clocks listed here
-           are expected to already be configured correctly. The OS must
-           ensure these clocks are not modified or disabled while the
-           simple framebuffer remains active.
+- clocks : List of clocks used by the framebuffer.
+- num-supplies : The number of regulators used by the framebuffer.
+- vinN-supply : The N-th (from 0) regulator used by the framebuffer.
+
+  The above resources are expected to already be configured correctly.
+  The OS must ensure they are not modified or disabled while the simple
+  framebuffer remains active.
+
 - display : phandle pointing to the primary display hardware node
 
 Example:

@@ -68,6 +72,8 @@ chosen {
 		stride = <(1600 * 2)>;
 		format = "r5g6b5";
 		clocks = <&ahb_gates 36>, <&ahb_gates 43>, <&ahb_gates 44>;
+		num-supplies = <1>;
+		vin0-supply = <&reg_dc1sw>;
 		display = <&lcdc0>;
 	};
 	stdout-path = "display0";

-- 
2.5.3

This claims and enables regulators listed in the simple framebuffer dt
node. This is needed so that regulators powering the display pipeline
and external hardware, described in the device node and known by the
kernel code, will remain properly enabled.

Signed-off-by: Chen-Yu Tsai <redacted>
---
 drivers/video/fbdev/simplefb.c | 102 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 101 insertions(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index 52c5c7e63b52..b2e419d9be3d 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c

@@ -28,7 +28,9 @@
 #include <linux/platform_device.h>
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
+#include <linux/of.h>
 #include <linux/of_platform.h>
+#include <linux/regulator/consumer.h>
 
 static struct fb_fix_screeninfo simplefb_fix = {
 	.id		= "simple",

@@ -174,6 +176,10 @@ struct simplefb_par {
 	int clk_count;
 	struct clk **clks;
 #endif
+#if defined CONFIG_OF && defined CONFIG_REGULATOR
+	u32 regulator_count;
+	struct regulator **regulators;
+#endif
 };
 
 #if defined CONFIG_OF && defined CONFIG_COMMON_CLK

@@ -269,6 +275,93 @@ static int simplefb_clocks_init(struct simplefb_par *par,
 static void simplefb_clocks_destroy(struct simplefb_par *par) { }
 #endif
 
+#if defined CONFIG_OF && defined CONFIG_REGULATOR
+/*
+ * Regulator handling code.
+ *
+ * Here we handle the num-supplies and vin*-supply properties of our
+ * "simple-framebuffer" dt node. This is necessary so that we can make sure
+ * that any regulators needed by the display hardware that the bootloader
+ * set up for us (and for which it provided a simplefb dt node), stay up,
+ * for the life of the simplefb driver.
+ *
+ * When the driver unloads, we cleanly disable, and then release the
+ * regulators.
+ *
+ * We only complain about errors here, no action is taken as the most likely
+ * error can only happen due to a mismatch between the bootloader which set
+ * up simplefb, and the regulator definitions in the device tree. Chances are
+ * that there are no adverse effects, and if there are, a clean teardown of
+ * the fb probe will not help us much either. So just complain and carry on,
+ * and hope that the user actually gets a working fb at the end of things.
+ */
+static int simplefb_regulators_init(struct simplefb_par *par,
+	struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct regulator *regulator;
+	int i, ret;
+
+	if (dev_get_platdata(&pdev->dev) || !np)
+		return 0;
+
+	ret = of_property_read_u32(np, "num-supplies", &par->regulator_count);
+	if (ret < 0)
+		return 0;
+
+	par->regulators = devm_kcalloc(&pdev->dev, par->regulator_count,
+				       sizeof(struct regulator *), GFP_KERNEL);
+	if (!par->regulators)
+		return -ENOMEM;
+
+	for (i = 0; i < par->regulator_count; i++) {
+		char name[8];
+
+		snprintf(name, sizeof(name), "vin%d", i);
+		regulator = devm_regulator_get_optional(&pdev->dev, name);
+		if (IS_ERR(regulator)) {
+			if (PTR_ERR(regulator) = -EPROBE_DEFER)
+				return -EPROBE_DEFER;
+			dev_err(&pdev->dev, "%s: regulator %d not found: %ld\n",
+				__func__, i, PTR_ERR(regulator));
+			continue;
+		}
+		par->regulators[i] = regulator;
+	}
+
+	for (i = 0; i < par->regulator_count; i++) {
+		if (par->regulators[i]) {
+			ret = regulator_enable(par->regulators[i]);
+			if (ret) {
+				dev_err(&pdev->dev,
+					"%s: failed to enable regulator %d: %d\n",
+					__func__, i, ret);
+				devm_regulator_put(par->regulators[i]);
+				par->regulators[i] = NULL;
+			}
+		}
+	}
+
+	return 0;
+}
+
+static void simplefb_regulators_destroy(struct simplefb_par *par)
+{
+	int i;
+
+	if (!par->regulators)
+		return;
+
+	for (i = 0; i < par->regulator_count; i++)
+		if (par->regulators[i])
+			regulator_disable(par->regulators[i]);
+}
+#else
+static int simplefb_regulators_init(struct simplefb_par *par,
+	struct platform_device *pdev) { return 0; }
+static void simplefb_regulators_destroy(struct simplefb_par *par) { }
+#endif
+
 static int simplefb_probe(struct platform_device *pdev)
 {
 	int ret;

@@ -340,6 +433,10 @@ static int simplefb_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto error_unmap;
 
+	ret = simplefb_regulators_init(par, pdev);
+	if (ret < 0)
+		goto error_clocks;
+
 	dev_info(&pdev->dev, "framebuffer at 0x%lx, 0x%x bytes, mapped to 0x%p\n",
 			     info->fix.smem_start, info->fix.smem_len,
 			     info->screen_base);

@@ -351,13 +448,15 @@ static int simplefb_probe(struct platform_device *pdev)
 	ret = register_framebuffer(info);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "Unable to register simplefb: %d\n", ret);
-		goto error_clocks;
+		goto error_regulators;
 	}
 
 	dev_info(&pdev->dev, "fb%d: simplefb registered!\n", info->node);
 
 	return 0;
 
+error_regulators:
+	simplefb_regulators_destroy(par);
 error_clocks:
 	simplefb_clocks_destroy(par);
 error_unmap:

@@ -373,6 +472,7 @@ static int simplefb_remove(struct platform_device *pdev)
 	struct simplefb_par *par = info->par;
 
 	unregister_framebuffer(info);
+	simplefb_regulators_destroy(par);
 	simplefb_clocks_destroy(par);
 	framebuffer_release(info);
 

-- 
2.5.3

On Tue, Oct 13, 2015 at 01:04:17AM +0800, Chen-Yu Tsai wrote:

The physical display tied to the framebuffer may have regulators
providing power to it, such as power for LCDs or interface conversion
chips.

The number of regulators in use may vary, but the regulator supply
binding can not be a list. Work around this by adding a "num-supplies"
property to communicate the number of supplies, and a list of 0 ~ N
"vinN-supply" properties for the actual regulator supply.

This is getting more complicated by the minute...

I don't see why you need num-supplies. Why not just try probing
vin${N}-supply until such a property isn't present in the DT?

Thanks,
Mark.

On Tue, Oct 13, 2015 at 1:10 AM, Mark Rutland [off-list ref] wrote:

On Tue, Oct 13, 2015 at 01:04:17AM +0800, Chen-Yu Tsai wrote:

quoted

The physical display tied to the framebuffer may have regulators
providing power to it, such as power for LCDs or interface conversion
chips.

The number of regulators in use may vary, but the regulator supply
binding can not be a list. Work around this by adding a "num-supplies"
property to communicate the number of supplies, and a list of 0 ~ N
"vinN-supply" properties for the actual regulator supply.

This is getting more complicated by the minute...

Yeah...

I considered "backlight" and "panel" properties, but that seems to need
more effort to parse. Regulators seemed easier.

quoted

Signed-off-by: Chen-Yu Tsai <redacted>
---
 .../devicetree/bindings/video/simple-framebuffer.txt       | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
index 4474ef6e0b95..0cc43e1be8b5 100644
--- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
+++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt

@@ -47,10 +47,14 @@ Required properties:
   - a8b8g8r8 (32-bit pixels, d[31:24]=a, d[23:16]=b, d[15:8]=g, d[7:0]=r).

 Optional properties:
-- clocks : List of clocks used by the framebuffer. Clocks listed here
-           are expected to already be configured correctly. The OS must
-           ensure these clocks are not modified or disabled while the
-           simple framebuffer remains active.
+- clocks : List of clocks used by the framebuffer.
+- num-supplies : The number of regulators used by the framebuffer.
+- vinN-supply : The N-th (from 0) regulator used by the framebuffer.

I don't see why you need num-supplies. Why not just try probing
vin${N}-supply until such a property isn't present in the DT?

That's doable. Though I'd add a hard limit on it. Does 16 seem reasonable?

Thanks
ChenYu

Hi,

On 13-10-15 04:22, Chen-Yu Tsai wrote:

On Tue, Oct 13, 2015 at 1:10 AM, Mark Rutland [off-list ref] wrote:

quoted

On Tue, Oct 13, 2015 at 01:04:17AM +0800, Chen-Yu Tsai wrote:

quoted

The physical display tied to the framebuffer may have regulators
providing power to it, such as power for LCDs or interface conversion
chips.

The number of regulators in use may vary, but the regulator supply
binding can not be a list. Work around this by adding a "num-supplies"
property to communicate the number of supplies, and a list of 0 ~ N
"vinN-supply" properties for the actual regulator supply.

This is getting more complicated by the minute...

Yeah...

I considered "backlight" and "panel" properties, but that seems to need
more effort to parse. Regulators seemed easier.

quoted

quoted

Signed-off-by: Chen-Yu Tsai <redacted>
---
  .../devicetree/bindings/video/simple-framebuffer.txt       | 14 ++++++++++----
  1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
index 4474ef6e0b95..0cc43e1be8b5 100644
--- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
+++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt

@@ -47,10 +47,14 @@ Required properties:
    - a8b8g8r8 (32-bit pixels, d[31:24]=a, d[23:16]=b, d[15:8]=g, d[7:0]=r).

  Optional properties:
-- clocks : List of clocks used by the framebuffer. Clocks listed here
-           are expected to already be configured correctly. The OS must
-           ensure these clocks are not modified or disabled while the
-           simple framebuffer remains active.
+- clocks : List of clocks used by the framebuffer.
+- num-supplies : The number of regulators used by the framebuffer.
+- vinN-supply : The N-th (from 0) regulator used by the framebuffer.

I don't see why you need num-supplies. Why not just try probing
vin${N}-supply until such a property isn't present in the DT?

+1 for this.

That's doable. Though I'd add a hard limit on it. Does 16 seem reasonable?

I would not add a hard limit to the binding, you can use a fixed array in
the code to make the code simpler. I would say 8 should be sufficient, since
the limit will only be in the code we can always bump it when we need to.

Regards,

Hans

Hi,

On 12-10-15 19:04, Chen-Yu Tsai wrote:

Hi everyone,

This series adds regulator claiming and enabling support for simplefb.

Sometimes the simplefb display output path consits of external conversion
chips and/or LCD drivers and backlights. These devices normally have
GPIOs to turn them on and/or bring them out of reset, and regulators
supplying power to them.

While the kernel does not touch unclaimed GPIOs, the regulator core
happily disables unused regulators. Thus we need simplefb to claim
and enable the regulators used throughout the display pipeline.

Ack for doing something like this (adding regulator support) to simplefb,
it makes sense to have this.

Now the DT bindings don't support a list of regulators directly, so
I'm working around it by having a "num-supplies" property to specify
the number of supply properties to check, and name the actual supplies
as "vinN-supply".

Hmm, I can see the need for a "supplies" property with a list of regulators
in other use-cases (e.g. the generic mmc-pwrseq driver) too. Now as discussed
we can simply do vin0-supply - vinN-supply properties and be done with it,
but maybe we need to actually add support for a generic "supplies" property ?

And if not then maybe we need a few generic helper devm helper function which
takes a node, figures out how much vinN-supply properties there are and returns
a dynamically allocated array containing references to all the regulators, or
a PTR_ERR in case of err, at which point the caller is expected to fail the
probe so that any successfully acquired regulators are released.

Mark, what are your thoughts on this ?

Regards,

Hans

On Tue, Oct 13, 2015 at 09:08:46AM +0200, Hans de Goede wrote:

On 13-10-15 04:22, Chen-Yu Tsai wrote:

quoted

On Tue, Oct 13, 2015 at 1:10 AM, Mark Rutland [off-list ref] wrote:

quoted

On Tue, Oct 13, 2015 at 01:04:17AM +0800, Chen-Yu Tsai wrote:

quoted

quoted

quoted

+- num-supplies : The number of regulators used by the framebuffer.
+- vinN-supply : The N-th (from 0) regulator used by the framebuffer.

quoted

quoted

I don't see why you need num-supplies. Why not just try probing
vin${N}-supply until such a property isn't present in the DT?

+1 for this.

Even better would be to just enumerate all the properties on the node
and request anything with a FOO-supply name.  That way we can keep
using standard regulator bindings that name the supplies after their
actual names on the device.

quoted

That's doable. Though I'd add a hard limit on it. Does 16 seem
reasonable?

I would not add a hard limit to the binding, you can use a fixed array in
the code to make the code simpler. I would say 8 should be sufficient, since
the limit will only be in the code we can always bump it when we need
to.

Or just dynamically allocate the array and resize as needed if it starts
to get to be a problem.

On Tue, Oct 13, 2015 at 09:16:56AM +0200, Hans de Goede wrote:

On 12-10-15 19:04, Chen-Yu Tsai wrote:

quoted

Now the DT bindings don't support a list of regulators directly, so
I'm working around it by having a "num-supplies" property to specify
the number of supply properties to check, and name the actual supplies
as "vinN-supply".

Hmm, I can see the need for a "supplies" property with a list of regulators
in other use-cases (e.g. the generic mmc-pwrseq driver) too. Now as discussed
we can simply do vin0-supply - vinN-supply properties and be done with it,
but maybe we need to actually add support for a generic "supplies" property ?

I really don't like having unnamed supplies, or supplies with names that
don't correspond to the schematic names for the physical supplies.  It
makes it harder to go between the DT and the schematic and encourages
bad practice on specific chip bindings which should be done properly
since it's harder to tell if the binding is done correctly.

Adding something with the pattern of parallel arrays of phandles and
names properties that got introduced after the regulator bindings were
done also means we need to go and update every single binding using
regulators to document the new properties which is going to be tedious
and require constant policing for a while.  I'm also not a big fan of
the pattern from a legibility point of view but that's a separate thing.

And if not then maybe we need a few generic helper devm helper function which
takes a node, figures out how much vinN-supply properties there are and returns
a dynamically allocated array containing references to all the regulators, or
a PTR_ERR in case of err, at which point the caller is expected to fail the
probe so that any successfully acquired regulators are released.

I can see it for this sort of simplefb thing but I'm not sure how we'd
discourage drivers for specific hardware from also using the same helper
which then makes it easy to get sloppy board DTs which I'd expect to
lead to hassle down the road as drivers try to use their supplies and
find that actual DTs have things that don't correspond to reality in
them.  The nice thing about having drivers name the supplies they're
expecting is that it makes describing the board as it really is much
more the path of least resistance.

Hi,

On 14-10-15 12:55, Mark Brown wrote:

On Tue, Oct 13, 2015 at 09:16:56AM +0200, Hans de Goede wrote:

quoted

On 12-10-15 19:04, Chen-Yu Tsai wrote:

quoted

quoted

Now the DT bindings don't support a list of regulators directly, so
I'm working around it by having a "num-supplies" property to specify
the number of supply properties to check, and name the actual supplies
as "vinN-supply".

quoted

Hmm, I can see the need for a "supplies" property with a list of regulators
in other use-cases (e.g. the generic mmc-pwrseq driver) too. Now as discussed
we can simply do vin0-supply - vinN-supply properties and be done with it,
but maybe we need to actually add support for a generic "supplies" property ?

I really don't like having unnamed supplies, or supplies with names that
don't correspond to the schematic names for the physical supplies.  It
makes it harder to go between the DT and the schematic and encourages
bad practice on specific chip bindings which should be done properly
since it's harder to tell if the binding is done correctly.

Ok.

Adding something with the pattern of parallel arrays of phandles and
names properties that got introduced after the regulator bindings were
done also means we need to go and update every single binding using
regulators to document the new properties which is going to be tedious
and require constant policing for a while.  I'm also not a big fan of
the pattern from a legibility point of view but that's a separate thing.

Oh no, I was not suggesting to have this replace how we currently do
things, I was merely suggesting allowing to have a supplies list property
for bindings where a list of (unnamed) supplies makes sense like simplefb.

I fully agree that we do not want to see matching a supplies-names prop,
if names are needed the old name-supply schema should be used just like
it is today.

quoted

And if not then maybe we need a few generic helper devm helper function which
takes a node, figures out how much vinN-supply properties there are and returns
a dynamically allocated array containing references to all the regulators, or
a PTR_ERR in case of err, at which point the caller is expected to fail the
probe so that any successfully acquired regulators are released.

I can see it for this sort of simplefb thing but I'm not sure how we'd
discourage drivers for specific hardware from also using the same helper
which then makes it easy to get sloppy board DTs which I'd expect to
lead to hassle down the road as drivers try to use their supplies and
find that actual DTs have things that don't correspond to reality in
them.  The nice thing about having drivers name the supplies they're
expecting is that it makes describing the board as it really is much
more the path of least resistance.

Ok, so as said I see some value in this for generic drivers like
simplefb, mmc-pwrseq, but also the generic ahci-platform, ohci-platform
and ehci-platform drivers, where often it is possible to use the generic
driver (together with a soc specific phy driver) without needing to
introduce new compatibles, as all we need is to specify a phy(s),
bunch of clocks, resets, etc. It would be good IMHO if we could specify
e.g. this is a generic ehci block, which needs this list of supplies
to be enabled (note typically the supplies are tied to the phy, so
maybe not the best example).

I like your idea in your other mail where you suggest to actually
use foo-supply and bar-supply names in the simplefb node, and then have
some code simple iterate over all the properties and check for *-supply
properties, so that the proper, schematic matching names can be used.

But surely if we go this way having a helper for this so that others
can re-use that likely not entirely trivial code is a good idea ?

One user which comes to mind immediately here is the generic mmc-pwrseq
driver.

I agree that we need to be careful to not use a helper like this too
much, but I do believe it will make sense to have it in some rare cases.
We can put a big warning in both the header declaring it and above
the implementation to use it scarcely.

Regards,

Hans

On Wed, Oct 14, 2015 at 01:31:52PM +0200, Hans de Goede wrote:

I like your idea in your other mail where you suggest to actually
use foo-supply and bar-supply names in the simplefb node, and then have
some code simple iterate over all the properties and check for *-supply
properties, so that the proper, schematic matching names can be used.

But surely if we go this way having a helper for this so that others
can re-use that likely not entirely trivial code is a good idea ?

Yeah.  It's trying to come up with a way to do this that is easy to
avoid abuse that's tricky.

One user which comes to mind immediately here is the generic mmc-pwrseq
driver.

I agree that we need to be careful to not use a helper like this too
much, but I do believe it will make sense to have it in some rare cases.
We can put a big warning in both the header declaring it and above
the implementation to use it scarcely.

I'd rather have something that was visible in the code, not everyone
reads the documentation especially not subsystem maintainers reviewing
drivers that use APIs they're not familiar with.

Hi,

On 18-10-15 21:57, Mark Brown wrote:

On Wed, Oct 14, 2015 at 01:31:52PM +0200, Hans de Goede wrote:

quoted

I like your idea in your other mail where you suggest to actually
use foo-supply and bar-supply names in the simplefb node, and then have
some code simple iterate over all the properties and check for *-supply
properties, so that the proper, schematic matching names can be used.

quoted

But surely if we go this way having a helper for this so that others
can re-use that likely not entirely trivial code is a good idea ?

Yeah.  It's trying to come up with a way to do this that is easy to
avoid abuse that's tricky.

quoted

One user which comes to mind immediately here is the generic mmc-pwrseq
driver.

quoted

I agree that we need to be careful to not use a helper like this too
much, but I do believe it will make sense to have it in some rare cases.
We can put a big warning in both the header declaring it and above
the implementation to use it scarcely.

I'd rather have something that was visible in the code, not everyone
reads the documentation especially not subsystem maintainers reviewing
drivers that use APIs they're not familiar with.

I'm afraid there is not really a good way to do this though, so a big fat
warning in the header declaring the function is really the bets we can
do IMHO.

Regards,

Hans