| Message ID | 1465465471-28740-2-git-send-email-k.kozlowski@samsung.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
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.
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-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 --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 */
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(+)