diff mbox

[RFC,v4,01/14] regulator: of: Add helper for getting all supplies

Message ID 1465465471-28740-2-git-send-email-k.kozlowski@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Krzysztof Kozlowski June 9, 2016, 9:44 a.m. UTC
Few drivers have a need of getting regulator supplies without knowing
their names:
1. The Simple Framebuffer driver works on setup provided by bootloader
   (outside of scope of kernel);
2. Generic power sequence driver may be attached to any device node.

Add a Device Tree helper for parsing "-supply" properties and returning
allocated bulk regulator consumers.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 drivers/regulator/of_regulator.c       | 86 ++++++++++++++++++++++++++++++++++
 include/linux/regulator/of_regulator.h | 13 +++++
 2 files changed, 99 insertions(+)

Comments

Mark Brown June 9, 2016, 10:29 a.m. UTC | #1
On Thu, Jun 09, 2016 at 11:44:18AM +0200, Krzysztof Kozlowski wrote:
> Few drivers have a need of getting regulator supplies without knowing
> their names:
> 1. The Simple Framebuffer driver works on setup provided by bootloader
>    (outside of scope of kernel);
> 2. Generic power sequence driver may be attached to any device node.
> 
> Add a Device Tree helper for parsing "-supply" properties and returning
> allocated bulk regulator consumers.

I'm still very concerned that this is just an invitation to people to
write half baked regulator consumers and half baked DTs to go along with
it, making it a standard API that doesn't have big red flags on it that
will flag up when "normal" drivers use it is not good.  Right now this
just looks like a standard API and people are going to just start using
it.  If we are going to do this perhaps we need a separate header or
something to help flag this up.

In the case of power sequences I'd expect the sequences to perform
operations on named supplies - the core shouldn't know what the supplies
are but the thing specifying the sequence should.

>  drivers/regulator/of_regulator.c       | 86 ++++++++++++++++++++++++++++++++++
>  include/linux/regulator/of_regulator.h | 13 +++++
>  2 files changed, 99 insertions(+)

The external interface shouldn't be DT specific, the Intel people are
busy importing all of DT into ACPI so they'll doubtless want an ACPI
version.
Krzysztof Kozlowski June 9, 2016, 11:42 a.m. UTC | #2
On 06/09/2016 12:29 PM, Mark Brown wrote:
> On Thu, Jun 09, 2016 at 11:44:18AM +0200, Krzysztof Kozlowski wrote:
>> Few drivers have a need of getting regulator supplies without knowing
>> their names:
>> 1. The Simple Framebuffer driver works on setup provided by bootloader
>>    (outside of scope of kernel);
>> 2. Generic power sequence driver may be attached to any device node.
>>
>> Add a Device Tree helper for parsing "-supply" properties and returning
>> allocated bulk regulator consumers.
> 
> I'm still very concerned that this is just an invitation to people to
> write half baked regulator consumers and half baked DTs to go along with
> it, making it a standard API that doesn't have big red flags on it that
> will flag up when "normal" drivers use it is not good.  Right now this
> just looks like a standard API and people are going to just start using
> it.  If we are going to do this perhaps we need a separate header or
> something to help flag this up.

No problem, I can move it to a special header.  Actually, if you dislike
this as an API, it does not have to be in header at all.  I can just
duplicate the simplefb code.

> In the case of power sequences I'd expect the sequences to perform
> operations on named supplies - the core shouldn't know what the supplies
> are but the thing specifying the sequence should.

Hm, so maybe passing names like:

usb3503@08 {
	reset-gpios = <&gpx3 5 GPIO_ACTIVE_HIGH>;
	initial-mode = <1>;
	vdd-supply = <&buck8_reg>;
	foo-supply = <&buck9_reg>;

        power-sequence;
	power-sequence-supplies = "vdd", "foo";
};

but this is getting against initial idea of not adding any
power-sequence properties.

> 
>>  drivers/regulator/of_regulator.c       | 86 ++++++++++++++++++++++++++++++++++
>>  include/linux/regulator/of_regulator.h | 13 +++++
>>  2 files changed, 99 insertions(+)
> 
> The external interface shouldn't be DT specific, the Intel people are
> busy importing all of DT into ACPI so they'll doubtless want an ACPI
> version.

Sure, I'll add it if this approach is acceptable.  At this moment this
is not necessary to show my idea so I prefer to avoid doing work which
might be discarded very fast by review.

Thanks for feedback!

Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki June 9, 2016, 12:50 p.m. UTC | #3
On Thu, Jun 9, 2016 at 12:29 PM, Mark Brown <broonie@kernel.org> wrote:
> On Thu, Jun 09, 2016 at 11:44:18AM +0200, Krzysztof Kozlowski wrote:
>> Few drivers have a need of getting regulator supplies without knowing
>> their names:
>> 1. The Simple Framebuffer driver works on setup provided by bootloader
>>    (outside of scope of kernel);
>> 2. Generic power sequence driver may be attached to any device node.
>>
>> Add a Device Tree helper for parsing "-supply" properties and returning
>> allocated bulk regulator consumers.
>
> I'm still very concerned that this is just an invitation to people to
> write half baked regulator consumers and half baked DTs to go along with
> it, making it a standard API that doesn't have big red flags on it that
> will flag up when "normal" drivers use it is not good.  Right now this
> just looks like a standard API and people are going to just start using
> it.  If we are going to do this perhaps we need a separate header or
> something to help flag this up.
>
> In the case of power sequences I'd expect the sequences to perform
> operations on named supplies - the core shouldn't know what the supplies
> are but the thing specifying the sequence should.
>
>>  drivers/regulator/of_regulator.c       | 86 ++++++++++++++++++++++++++++++++++
>>  include/linux/regulator/of_regulator.h | 13 +++++
>>  2 files changed, 99 insertions(+)
>
> The external interface shouldn't be DT specific, the Intel people are
> busy importing all of DT into ACPI

Well, not really.

If you are referring to the pinctrl proposal discussed recently, that
was a proposal from one group at Intel and AFAICS it has been
abandoned.

> so they'll doubtless want an ACPI version.

That is possible, though, so I agree that the external interface
should not be DT-specific.
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown June 9, 2016, 3:57 p.m. UTC | #4
On Thu, Jun 09, 2016 at 02:50:26PM +0200, Rafael J. Wysocki wrote:
> On Thu, Jun 9, 2016 at 12:29 PM, Mark Brown <broonie@kernel.org> wrote:

> > The external interface shouldn't be DT specific, the Intel people are
> > busy importing all of DT into ACPI

> Well, not really.

> If you are referring to the pinctrl proposal discussed recently, that
> was a proposal from one group at Intel and AFAICS it has been
> abandoned.

Well, it was that, it was the device properties stuff in general and the
overlays stuff - plus just the fact that with the Minnowboard style use
case people have and the desire to use ACPI it seems like that's going
to be something you need.
Rob Herring (Arm) June 10, 2016, 5:30 p.m. UTC | #5
On Thu, Jun 09, 2016 at 01:42:02PM +0200, Krzysztof Kozlowski wrote:
> On 06/09/2016 12:29 PM, Mark Brown wrote:
> > On Thu, Jun 09, 2016 at 11:44:18AM +0200, Krzysztof Kozlowski wrote:
> >> Few drivers have a need of getting regulator supplies without knowing
> >> their names:
> >> 1. The Simple Framebuffer driver works on setup provided by bootloader
> >>    (outside of scope of kernel);
> >> 2. Generic power sequence driver may be attached to any device node.
> >>
> >> Add a Device Tree helper for parsing "-supply" properties and returning
> >> allocated bulk regulator consumers.
> > 
> > I'm still very concerned that this is just an invitation to people to
> > write half baked regulator consumers and half baked DTs to go along with
> > it, making it a standard API that doesn't have big red flags on it that
> > will flag up when "normal" drivers use it is not good.  Right now this
> > just looks like a standard API and people are going to just start using
> > it.  If we are going to do this perhaps we need a separate header or
> > something to help flag this up.
> 
> No problem, I can move it to a special header.  Actually, if you dislike
> this as an API, it does not have to be in header at all.  I can just
> duplicate the simplefb code.
> 
> > In the case of power sequences I'd expect the sequences to perform
> > operations on named supplies - the core shouldn't know what the supplies
> > are but the thing specifying the sequence should.
> 
> Hm, so maybe passing names like:
> 
> usb3503@08 {
> 	reset-gpios = <&gpx3 5 GPIO_ACTIVE_HIGH>;
> 	initial-mode = <1>;
> 	vdd-supply = <&buck8_reg>;
> 	foo-supply = <&buck9_reg>;
> 
>         power-sequence;
> 	power-sequence-supplies = "vdd", "foo";

This alone would be fine as it is just one property, but then what's 
next? power-sequence-delay, power-sequence-clocks, etc. What if you 
need to express ordering relationship of supplies, clocks, gpios? We end 
up with a scripting language in DT and we don't want to have that.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Heiko Stuebner June 10, 2016, 6:49 p.m. UTC | #6
Am Freitag, 10. Juni 2016, 12:30:56 schrieb Rob Herring:
> On Thu, Jun 09, 2016 at 01:42:02PM +0200, Krzysztof Kozlowski wrote:
> > On 06/09/2016 12:29 PM, Mark Brown wrote:
> > > On Thu, Jun 09, 2016 at 11:44:18AM +0200, Krzysztof Kozlowski wrote:
> > >> Few drivers have a need of getting regulator supplies without knowing
> > >> their names:
> > >> 1. The Simple Framebuffer driver works on setup provided by bootloader
> > >> 
> > >>    (outside of scope of kernel);
> > >> 
> > >> 2. Generic power sequence driver may be attached to any device node.
> > >> 
> > >> Add a Device Tree helper for parsing "-supply" properties and returning
> > >> allocated bulk regulator consumers.
> > > 
> > > I'm still very concerned that this is just an invitation to people to
> > > write half baked regulator consumers and half baked DTs to go along with
> > > it, making it a standard API that doesn't have big red flags on it that
> > > will flag up when "normal" drivers use it is not good.  Right now this
> > > just looks like a standard API and people are going to just start using
> > > it.  If we are going to do this perhaps we need a separate header or
> > > something to help flag this up.
> > 
> > No problem, I can move it to a special header.  Actually, if you dislike
> > this as an API, it does not have to be in header at all.  I can just
> > duplicate the simplefb code.
> > 
> > > In the case of power sequences I'd expect the sequences to perform
> > > operations on named supplies - the core shouldn't know what the supplies
> > > are but the thing specifying the sequence should.
> > 
> > Hm, so maybe passing names like:
> > 
> > usb3503@08 {
> > 
> > 	reset-gpios = <&gpx3 5 GPIO_ACTIVE_HIGH>;
> > 	initial-mode = <1>;
> > 	vdd-supply = <&buck8_reg>;
> > 	foo-supply = <&buck9_reg>;
> > 	
> >         power-sequence;
> > 	
> > 	power-sequence-supplies = "vdd", "foo";
> 
> This alone would be fine as it is just one property, but then what's
> next? power-sequence-delay, power-sequence-clocks, etc. What if you
> need to express ordering relationship of supplies, clocks, gpios? We end
> up with a scripting language in DT and we don't want to have that.

Also, at least from the simple blocks I've seen so far (usb-hub chips, usb-
sata bridges), a power-supply feels more like it should be a regular phy-
supply of the usbphy connected the the host-controller.

It's similar for mmc-controllers where vmmc-supply on the controller handles 
the power-supply of the connected card and thus the current mmc power-
sequences do not handle regulators.

Reset-gpios and clock inputs are clearly properties of the actual device, but 
the supply control should probably lay with the host controller, especially as 
it is the one deciding when to power on/off things.


--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Chen June 12, 2016, 7:29 a.m. UTC | #7
On Fri, Jun 10, 2016 at 12:30:56PM -0500, Rob Herring wrote:
> On Thu, Jun 09, 2016 at 01:42:02PM +0200, Krzysztof Kozlowski wrote:
> > On 06/09/2016 12:29 PM, Mark Brown wrote:
> > > On Thu, Jun 09, 2016 at 11:44:18AM +0200, Krzysztof Kozlowski wrote:
> > >> Few drivers have a need of getting regulator supplies without knowing
> > >> their names:
> > >> 1. The Simple Framebuffer driver works on setup provided by bootloader
> > >>    (outside of scope of kernel);
> > >> 2. Generic power sequence driver may be attached to any device node.
> > >>
> > >> Add a Device Tree helper for parsing "-supply" properties and returning
> > >> allocated bulk regulator consumers.
> > > 
> > > I'm still very concerned that this is just an invitation to people to
> > > write half baked regulator consumers and half baked DTs to go along with
> > > it, making it a standard API that doesn't have big red flags on it that
> > > will flag up when "normal" drivers use it is not good.  Right now this
> > > just looks like a standard API and people are going to just start using
> > > it.  If we are going to do this perhaps we need a separate header or
> > > something to help flag this up.
> > 
> > No problem, I can move it to a special header.  Actually, if you dislike
> > this as an API, it does not have to be in header at all.  I can just
> > duplicate the simplefb code.
> > 
> > > In the case of power sequences I'd expect the sequences to perform
> > > operations on named supplies - the core shouldn't know what the supplies
> > > are but the thing specifying the sequence should.
> > 
> > Hm, so maybe passing names like:
> > 
> > usb3503@08 {
> > 	reset-gpios = <&gpx3 5 GPIO_ACTIVE_HIGH>;
> > 	initial-mode = <1>;
> > 	vdd-supply = <&buck8_reg>;
> > 	foo-supply = <&buck9_reg>;
> > 
> >         power-sequence;
> > 	power-sequence-supplies = "vdd", "foo";
> 
> This alone would be fine as it is just one property, but then what's 
> next? power-sequence-delay, power-sequence-clocks, etc. What if you 
> need to express ordering relationship of supplies, clocks, gpios? We end 
> up with a scripting language in DT and we don't want to have that.
> 

Can we do things like below:

- DT describes hardware elements (clock, gpios, etc) for power sequence, and we
need a node for power sequence.
- Power sequence framework handles getting hardware elements.
- Power sequence platform driver handles special sequence for devices,
and we can create some generic drivers for generic devices.
Peter Chen June 13, 2016, 3:44 a.m. UTC | #8
On Sun, Jun 12, 2016 at 03:29:01PM +0800, Peter Chen wrote:
> On Fri, Jun 10, 2016 at 12:30:56PM -0500, Rob Herring wrote:
> > On Thu, Jun 09, 2016 at 01:42:02PM +0200, Krzysztof Kozlowski wrote:
> > > On 06/09/2016 12:29 PM, Mark Brown wrote:
> > > > On Thu, Jun 09, 2016 at 11:44:18AM +0200, Krzysztof Kozlowski wrote:
> > > >> Few drivers have a need of getting regulator supplies without knowing
> > > >> their names:
> > > >> 1. The Simple Framebuffer driver works on setup provided by bootloader
> > > >>    (outside of scope of kernel);
> > > >> 2. Generic power sequence driver may be attached to any device node.
> > > >>
> > > >> Add a Device Tree helper for parsing "-supply" properties and returning
> > > >> allocated bulk regulator consumers.
> > > > 
> > > > I'm still very concerned that this is just an invitation to people to
> > > > write half baked regulator consumers and half baked DTs to go along with
> > > > it, making it a standard API that doesn't have big red flags on it that
> > > > will flag up when "normal" drivers use it is not good.  Right now this
> > > > just looks like a standard API and people are going to just start using
> > > > it.  If we are going to do this perhaps we need a separate header or
> > > > something to help flag this up.
> > > 
> > > No problem, I can move it to a special header.  Actually, if you dislike
> > > this as an API, it does not have to be in header at all.  I can just
> > > duplicate the simplefb code.
> > > 
> > > > In the case of power sequences I'd expect the sequences to perform
> > > > operations on named supplies - the core shouldn't know what the supplies
> > > > are but the thing specifying the sequence should.
> > > 
> > > Hm, so maybe passing names like:
> > > 
> > > usb3503@08 {
> > > 	reset-gpios = <&gpx3 5 GPIO_ACTIVE_HIGH>;
> > > 	initial-mode = <1>;
> > > 	vdd-supply = <&buck8_reg>;
> > > 	foo-supply = <&buck9_reg>;
> > > 
> > >         power-sequence;
> > > 	power-sequence-supplies = "vdd", "foo";
> > 
> > This alone would be fine as it is just one property, but then what's 
> > next? power-sequence-delay, power-sequence-clocks, etc. What if you 
> > need to express ordering relationship of supplies, clocks, gpios? We end 
> > up with a scripting language in DT and we don't want to have that.
> > 
> 
> Can we do things like below:
> 
> - DT describes hardware elements (clock, gpios, etc) for power sequence, and we
> need a node for power sequence.
> - Power sequence framework handles getting hardware elements.

Framework may do few things, since hardware elements are also different
for devices.

> - Power sequence platform driver handles special sequence for devices,
> and we can create some generic drivers for generic devices.
> 

So, my suggestion is do like mmc does (like this patch set does). The
reasons like belows:

- This piece of power sequence code needs to work like device driver, not
library, it is easy to manage resources using device driver.
- The device on the bus has still not been found, so this piece of code
can't be in device driver on each subsystem.
- We need to have a place for these power sequences drivers

Ideally, I hope it can work like regulator class, but it seems hard to
compatible with current mmc-pwrseq DT node.
diff mbox

Patch

diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index cd828dbf9d52..0d2c8dd0ebc0 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -350,3 +350,89 @@  struct regulator_init_data *regulator_of_get_init_data(struct device *dev,
 
 	return init_data;
 }
+
+/**
+ * devm_of_regulator_all_get - get all regulator consumers
+ *
+ * @dev:           Device to supply
+ * @num_consumers  Number of consumers registered (only on success)
+ * @consumers:     Configuration of consumers; names of supplies and clients
+ *                 are stored here; allocated only on success (NULL otherwise);
+ *
+ * @return 0 on success, an errno on failure.
+ *
+ * This helper function allows drivers to get all regulator consumers
+ * for given device in one operation.  The names of regulator supplies
+ * do not have to be provided.  If any of the regulators cannot be
+ * acquired then any regulators that were allocated will be freed
+ * before returning to the caller.
+ */
+int devm_of_regulator_all_get(struct device *dev, unsigned int *num_consumers,
+			      struct regulator_bulk_data **consumers)
+{
+	struct device_node *np = dev->of_node;
+	struct regulator_bulk_data *bulk;
+	unsigned int count = 0, i = 0;
+	struct property *prop;
+	const char *p;
+	int ret;
+
+	if (!np) {
+		ret = 0;
+		goto err; /* Not really an error */
+	}
+
+	/* Count the number of regulator supplies */
+	for_each_property_of_node(np, prop) {
+		p = strstr(prop->name, "-supply");
+		if (p && p != prop->name)
+			count++;
+	}
+
+	if (!count) {
+		ret = 0;
+		goto err; /* Not really an error */
+	}
+
+	bulk = devm_kcalloc(dev, count, sizeof(**consumers), GFP_KERNEL);
+	if (!bulk) {
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	/* Get all the names for supplies */
+	for_each_property_of_node(np, prop) {
+		char *name;
+		int len;
+
+		p = strstr(prop->name, "-supply");
+		if (!p || p == prop->name)
+			continue;
+
+		len = strlen(prop->name) - strlen("-supply") + 1;
+		name = devm_kzalloc(dev, len, GFP_KERNEL);
+		if (!name) {
+			ret = -ENOMEM;
+			goto err;
+		}
+
+		strlcpy(name, prop->name, len);
+		bulk[i++].supply = name;
+	}
+
+	ret = devm_regulator_bulk_get(dev, i, bulk);
+	if (ret)
+		goto err;
+
+	*consumers = bulk;
+	*num_consumers = i;
+
+	return 0;
+
+err:
+	*num_consumers = 0;
+	*consumers = NULL;
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(devm_of_regulator_all_get);
diff --git a/include/linux/regulator/of_regulator.h b/include/linux/regulator/of_regulator.h
index 763953f7e3b8..93a3b7fe92e8 100644
--- a/include/linux/regulator/of_regulator.h
+++ b/include/linux/regulator/of_regulator.h
@@ -7,6 +7,7 @@ 
 #define __LINUX_OF_REG_H
 
 struct regulator_desc;
+struct regulator_bulk_data;
 
 struct of_regulator_match {
 	const char *name;
@@ -24,6 +25,9 @@  extern struct regulator_init_data
 extern int of_regulator_match(struct device *dev, struct device_node *node,
 			      struct of_regulator_match *matches,
 			      unsigned int num_matches);
+extern int devm_of_regulator_all_get(struct device *dev,
+				     unsigned int *num_consumers,
+				     struct regulator_bulk_data **consumers);
 #else
 static inline struct regulator_init_data
 	*of_get_regulator_init_data(struct device *dev,
@@ -40,6 +44,15 @@  static inline int of_regulator_match(struct device *dev,
 {
 	return 0;
 }
+static inline int devm_of_regulator_all_get(struct device *dev,
+					    unsigned int *num_consumers,
+					    struct regulator_bulk_data **consumers)
+{
+	*num_consumers = 0;
+	*consumers = NULL;
+
+	return 0;
+}
 #endif /* CONFIG_OF */
 
 #endif /* __LINUX_OF_REG_H */