Patchwork [RFC,v2,2/3] power: power_supply: Add core support for supplied_nodes

login
register
mail settings
Submitter Rhyland Klein
Date Feb. 21, 2013, 11:11 p.m.
Message ID <1361488272-21010-3-git-send-email-rklein@nvidia.com>
Download mbox | patch
Permalink /patch/222439/
State Not Applicable, archived
Headers show

Comments

Rhyland Klein - Feb. 21, 2013, 11:11 p.m.
With the growing support for dt, it make sense to try to make use of
dt features to make the general code cleaner. This patch is an
attempt to commonize how chargers and their supplies are linked.

Following common dt convention, the "supplied-to" char** list is
replaced with phandle lists defined in the supplies which contain
phandles of their suppliers.

This has the effect however of introducing an inversion in the internal
mechanics of how this information is stored. In the case of non-dt,
the char** list of supplies is stored in the charger. In the dt case,
a device_node * list is stored in the supplies of their chargers,
however this seems to be the only way to support this.

Signed-off-by: Rhyland Klein <rklein@nvidia.com>
---
v2:
  - Changed from struct device_node* contained in suppliers to a list stored
    in the supplies.
  - changed logic for the is_supplied_by check to handle the entire loop
    as the array structure is different between the 2 paths.

 drivers/power/power_supply_core.c |   46 +++++++++++++++++++++++++++----------
 include/linux/power_supply.h      |    9 ++++++++
 2 files changed, 43 insertions(+), 12 deletions(-)
Stephen Warren - Feb. 22, 2013, 7:49 p.m.
On 02/21/2013 04:11 PM, Rhyland Klein wrote:
> With the growing support for dt, it make sense to try to make use of
> dt features to make the general code cleaner. This patch is an
> attempt to commonize how chargers and their supplies are linked.
> 
> Following common dt convention, the "supplied-to" char** list is
> replaced with phandle lists defined in the supplies which contain
> phandles of their suppliers.
> 
> This has the effect however of introducing an inversion in the internal
> mechanics of how this information is stored. In the case of non-dt,
> the char** list of supplies is stored in the charger. In the dt case,
> a device_node * list is stored in the supplies of their chargers,
> however this seems to be the only way to support this.

When parsing the DT, you can convert from phandle (or struct device_node
*) to the name of the referenced supply by simple lookup. So, you could
store supply names rather than device_node *. Can't you then also fill
in the referenced supply's existing char** list of supplies?

Of course, making this interact-with/use -EPROBE_DEFERRED might be
challenging, since this would be operating in the inverse order to other
producer/consumer relationships, which might cause loops.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren - Feb. 22, 2013, 8:09 p.m.
On 02/21/2013 04:11 PM, Rhyland Klein wrote:
> With the growing support for dt, it make sense to try to make use of
> dt features to make the general code cleaner. This patch is an
> attempt to commonize how chargers and their supplies are linked.
> 
> Following common dt convention, the "supplied-to" char** list is
> replaced with phandle lists defined in the supplies which contain
> phandles of their suppliers.
> 
> This has the effect however of introducing an inversion in the internal
> mechanics of how this information is stored. In the case of non-dt,
> the char** list of supplies is stored in the charger. In the dt case,
> a device_node * list is stored in the supplies of their chargers,
> however this seems to be the only way to support this.

grep over the whole kernel tree for supplied_to doesn't yield /too/ many
hits, although I didn't look at the complexity of most of them. Would it
be possible to invert all the current in-kernel uses to represent a
supplied_from/by model instead? That would mean the proposed DT binding
would then represent the same relationship ordering as the kernel code,
which would be easier to handle.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rhyland Klein - Feb. 22, 2013, 9:55 p.m.
On 2/22/2013 2:49 PM, Stephen Warren wrote:
> On 02/21/2013 04:11 PM, Rhyland Klein wrote:
>> With the growing support for dt, it make sense to try to make use of
>> dt features to make the general code cleaner. This patch is an
>> attempt to commonize how chargers and their supplies are linked.
>>
>> Following common dt convention, the "supplied-to" char** list is
>> replaced with phandle lists defined in the supplies which contain
>> phandles of their suppliers.
>>
>> This has the effect however of introducing an inversion in the internal
>> mechanics of how this information is stored. In the case of non-dt,
>> the char** list of supplies is stored in the charger. In the dt case,
>> a device_node * list is stored in the supplies of their chargers,
>> however this seems to be the only way to support this.
> When parsing the DT, you can convert from phandle (or struct device_node
> *) to the name of the referenced supply by simple lookup. So, you could
> store supply names rather than device_node *. Can't you then also fill
> in the referenced supply's existing char** list of supplies?
>
> Of course, making this interact-with/use -EPROBE_DEFERRED might be
> challenging, since this would be operating in the inverse order to other
> producer/consumer relationships, which might cause loops.
The main problem I ran into when I was essentially trying to do this, 
was that the list of names that
are used to match the power_supplies are the strings set as "name" in 
the power_supply structs. This
doesn't get set automatically based on their nodes, and it is currently 
up to each driver to define their
own name.

For example, the sbs-battery driver uses the name "sbs-XXX" where XX is 
its dev_name. Other drivers
use "%s-$%d" as i2c_device_id->name, + instance number. Then the only 
solution I see is to require a new
property that defines the power-supply's name in the devicetree.

This solution with device_nodes, while not ideal, seems the be the best 
bet from what I see. Maybe
someone else has a better idea.

-rhyland
Rhyland Klein - Feb. 22, 2013, 9:58 p.m.
On 2/22/2013 3:09 PM, Stephen Warren wrote:
> On 02/21/2013 04:11 PM, Rhyland Klein wrote:
>> With the growing support for dt, it make sense to try to make use of
>> dt features to make the general code cleaner. This patch is an
>> attempt to commonize how chargers and their supplies are linked.
>>
>> Following common dt convention, the "supplied-to" char** list is
>> replaced with phandle lists defined in the supplies which contain
>> phandles of their suppliers.
>>
>> This has the effect however of introducing an inversion in the internal
>> mechanics of how this information is stored. In the case of non-dt,
>> the char** list of supplies is stored in the charger. In the dt case,
>> a device_node * list is stored in the supplies of their chargers,
>> however this seems to be the only way to support this.
> grep over the whole kernel tree for supplied_to doesn't yield /too/ many
> hits, although I didn't look at the complexity of most of them. Would it
> be possible to invert all the current in-kernel uses to represent a
> supplied_from/by model instead? That would mean the proposed DT binding
> would then represent the same relationship ordering as the kernel code,
> which would be easier to handle.
I think it is surely possible to change all the existing drivers to the 
inverse logic as
you suggested. That might make a good follow patchset.

-rhyland
Stephen Warren - Feb. 22, 2013, 11:01 p.m.
On 02/22/2013 02:55 PM, Rhyland Klein wrote:
> On 2/22/2013 2:49 PM, Stephen Warren wrote:
>> On 02/21/2013 04:11 PM, Rhyland Klein wrote:
>>> With the growing support for dt, it make sense to try to make use of
>>> dt features to make the general code cleaner. This patch is an
>>> attempt to commonize how chargers and their supplies are linked.
>>>
>>> Following common dt convention, the "supplied-to" char** list is
>>> replaced with phandle lists defined in the supplies which contain
>>> phandles of their suppliers.
>>>
>>> This has the effect however of introducing an inversion in the internal
>>> mechanics of how this information is stored. In the case of non-dt,
>>> the char** list of supplies is stored in the charger. In the dt case,
>>> a device_node * list is stored in the supplies of their chargers,
>>> however this seems to be the only way to support this.
>> When parsing the DT, you can convert from phandle (or struct device_node
>> *) to the name of the referenced supply by simple lookup. So, you could
>> store supply names rather than device_node *. Can't you then also fill
>> in the referenced supply's existing char** list of supplies?
>>
>> Of course, making this interact-with/use -EPROBE_DEFERRED might be
>> challenging, since this would be operating in the inverse order to other
>> producer/consumer relationships, which might cause loops.
> The main problem I ran into when I was essentially trying to do this,
> was that the list of names that
> are used to match the power_supplies are the strings set as "name" in
> the power_supply structs. This
> doesn't get set automatically based on their nodes, and it is currently
> up to each driver to define their
> own name.
> 
> For example, the sbs-battery driver uses the name "sbs-XXX" where XX is
> its dev_name. Other drivers
> use "%s-$%d" as i2c_device_id->name, + instance number. Then the only
> solution I see is to require a new
> property that defines the power-supply's name in the devicetree.
> 
> This solution with device_nodes, while not ideal, seems the be the best
> bet from what I see. Maybe
> someone else has a better idea.

For other resource types, this is handled by the (phandle -> whatever)
conversion process actually being a function call on the referenced
object, so that the driver code for it can look up the data in the
actual device/... object etc. See the various .of_xlate functions that
exist in the kernel.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rhyland Klein - Feb. 28, 2013, 7:48 p.m.
On 2/22/2013 4:58 PM, Rhyland Klein wrote:
> On 2/22/2013 3:09 PM, Stephen Warren wrote:
>> On 02/21/2013 04:11 PM, Rhyland Klein wrote:
>>> With the growing support for dt, it make sense to try to make use of
>>> dt features to make the general code cleaner. This patch is an
>>> attempt to commonize how chargers and their supplies are linked.
>>>
>>> Following common dt convention, the "supplied-to" char** list is
>>> replaced with phandle lists defined in the supplies which contain
>>> phandles of their suppliers.
>>>
>>> This has the effect however of introducing an inversion in the internal
>>> mechanics of how this information is stored. In the case of non-dt,
>>> the char** list of supplies is stored in the charger. In the dt case,
>>> a device_node * list is stored in the supplies of their chargers,
>>> however this seems to be the only way to support this.
>> grep over the whole kernel tree for supplied_to doesn't yield /too/ many
>> hits, although I didn't look at the complexity of most of them. Would it
>> be possible to invert all the current in-kernel uses to represent a
>> supplied_from/by model instead? That would mean the proposed DT binding
>> would then represent the same relationship ordering as the kernel code,
>> which would be easier to handle.
> I think it is surely possible to change all the existing drivers to the
> inverse logic as
> you suggested. That might make a good follow patchset.
>
> -rhyland
>

Anton, David, would you be adverse to the changing of supplied_to from 
being a
list of batteries stored in a charger to being a list of chargers stored 
in batteries?

(I only use terms charger and batteries as it is much clearer for me to 
read in place
of terms like supplier and supply which are more accurate but much more 
confusing
when used together).

-rhyland
Rhyland Klein - Feb. 28, 2013, 7:54 p.m.
On 2/22/2013 6:01 PM, Stephen Warren wrote:
> On 02/22/2013 02:55 PM, Rhyland Klein wrote:
>> On 2/22/2013 2:49 PM, Stephen Warren wrote:
>>> On 02/21/2013 04:11 PM, Rhyland Klein wrote:
>>>> With the growing support for dt, it make sense to try to make use of
>>>> dt features to make the general code cleaner. This patch is an
>>>> attempt to commonize how chargers and their supplies are linked.
>>>>
>>>> Following common dt convention, the "supplied-to" char** list is
>>>> replaced with phandle lists defined in the supplies which contain
>>>> phandles of their suppliers.
>>>>
>>>> This has the effect however of introducing an inversion in the internal
>>>> mechanics of how this information is stored. In the case of non-dt,
>>>> the char** list of supplies is stored in the charger. In the dt case,
>>>> a device_node * list is stored in the supplies of their chargers,
>>>> however this seems to be the only way to support this.
>>> When parsing the DT, you can convert from phandle (or struct device_node
>>> *) to the name of the referenced supply by simple lookup. So, you could
>>> store supply names rather than device_node *. Can't you then also fill
>>> in the referenced supply's existing char** list of supplies?
>>>
>>> Of course, making this interact-with/use -EPROBE_DEFERRED might be
>>> challenging, since this would be operating in the inverse order to other
>>> producer/consumer relationships, which might cause loops.
>> The main problem I ran into when I was essentially trying to do this,
>> was that the list of names that
>> are used to match the power_supplies are the strings set as "name" in
>> the power_supply structs. This
>> doesn't get set automatically based on their nodes, and it is currently
>> up to each driver to define their
>> own name.
>>
>> For example, the sbs-battery driver uses the name "sbs-XXX" where XX is
>> its dev_name. Other drivers
>> use "%s-$%d" as i2c_device_id->name, + instance number. Then the only
>> solution I see is to require a new
>> property that defines the power-supply's name in the devicetree.
>>
>> This solution with device_nodes, while not ideal, seems the be the best
>> bet from what I see. Maybe
>> someone else has a better idea.
> For other resource types, this is handled by the (phandle -> whatever)
> conversion process actually being a function call on the referenced
> object, so that the driver code for it can look up the data in the
> actual device/... object etc. See the various .of_xlate functions that
> exist in the kernel.

I think this makes sense assuming we can change the existing supplies_to 
property
to match this style so that there isn't an inversion in the 2 paths.

Then I think the idea of an of_xlate function would work fine given that 
there are no
circular dependencies (causes issues with -EPROBE_DEFER). If that is the 
case, then
we could add a step to power_supply registration where it would generate the
char * list of supplies and store it in the power_supply being 
registered. In that way,
from then on, it wouldn't matter how the power_supply was registered, 
and the list
of supplies would be the same in either case.

Anton, David, have you seen it, or can you potentially see a case where 
circular
dependencies could arise? I can't but maybe you know of a situation I don't.

I will start working on patches to support this, including changing the way
supplied_to currently works, while I await answers whether or not it 
would be
acceptable.

-rhyland
Anton Vorontsov - March 2, 2013, 10:48 p.m.
On Thu, Feb 28, 2013 at 02:48:03PM -0500, Rhyland Klein wrote:
[...]
> Anton, David, would you be adverse to the changing of supplied_to
> from being a
> list of batteries stored in a charger to being a list of chargers
> stored in batteries?

I wonder if we can support both ways?..

Thanks,
Anton
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rhyland Klein - March 4, 2013, 5:32 p.m.
On 3/2/2013 5:48 PM, Anton Vorontsov wrote:
> On Thu, Feb 28, 2013 at 02:48:03PM -0500, Rhyland Klein wrote:
> [...]
>> Anton, David, would you be adverse to the changing of supplied_to
>> from being a
>> list of batteries stored in a charger to being a list of chargers
>> stored in batteries?
> I wonder if we can support both ways?..
>
> Thanks,
> Anton
Well, the interesting factor becomes either we end up with 2 arrays
(supplied_to, supplied_from) or we make 1 array (horray, save space!)
but need to then either have a flag or use the power_supply type to
know how to interpret the single array (as supplied_to or supplied_from).

Adding in the second array and adds a char ** and an int, which doesn't seem
like as much overhead as trying to figure out how to interpret the 
single array
so I am inclined to stick with 2 arrays.

-rhyland
Anton Vorontsov - March 4, 2013, 5:47 p.m.
On Mon, Mar 04, 2013 at 12:32:35PM -0500, Rhyland Klein wrote:
[...]
> >>Anton, David, would you be adverse to the changing of supplied_to
> >>from being a
> >>list of batteries stored in a charger to being a list of chargers
> >>stored in batteries?
> >>
> >I wonder if we can support both ways?..
>
> Well, the interesting factor becomes either we end up with 2 arrays
> (supplied_to, supplied_from) or we make 1 array (horray, save space!)
> but need to then either have a flag or use the power_supply type to
> know how to interpret the single array (as supplied_to or supplied_from).
> 
> Adding in the second array and adds a char ** and an int, which doesn't seem
> like as much overhead as trying to figure out how to interpret the
> single array
> so I am inclined to stick with 2 arrays.

Yup, two arrays seems very reasonable. Plus that way we don't need to
convert existing drivers, so we avoid churn.

Thanks!

Anton
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
index 8a7cfb3..a87c5b8 100644
--- a/drivers/power/power_supply_core.c
+++ b/drivers/power/power_supply_core.c
@@ -26,17 +26,40 @@  EXPORT_SYMBOL_GPL(power_supply_class);
 
 static struct device_type power_supply_dev_type;
 
+static int __power_supply_is_supplied_by(struct power_supply *supplier,
+					 struct power_supply *supply)
+{
+	int i;
+#ifdef CONFIG_OF
+	struct power_supply_supplies *pss;
+#endif
+
+#ifdef CONFIG_OF
+	list_for_each_entry(pss, &supply->supplies.list, list) {
+		if (supplier->supplies.node == pss->node)
+			return 0;
+	}
+#endif
+
+	for (i = 0; i < supplier->num_supplicants; i++) {
+		if (!supply->name || !supplier->supplied_to)
+			continue;
+		if (!strcmp(supplier->supplied_to[i], supply->name))
+			return 0;
+	}
+
+	return -EINVAL;
+}
+
 static int __power_supply_changed_work(struct device *dev, void *data)
 {
 	struct power_supply *psy = (struct power_supply *)data;
 	struct power_supply *pst = dev_get_drvdata(dev);
-	int i;
 
-	for (i = 0; i < psy->num_supplicants; i++)
-		if (!strcmp(psy->supplied_to[i], pst->name)) {
-			if (pst->external_power_changed)
-				pst->external_power_changed(pst);
-		}
+	if (__power_supply_is_supplied_by(psy, pst)) {
+		if (pst->external_power_changed)
+			pst->external_power_changed(pst);
+	}
 	return 0;
 }
 
@@ -68,13 +91,9 @@  static int __power_supply_am_i_supplied(struct device *dev, void *data)
 	union power_supply_propval ret = {0,};
 	struct power_supply *psy = (struct power_supply *)data;
 	struct power_supply *epsy = dev_get_drvdata(dev);
-	int i;
 
-	for (i = 0; i < epsy->num_supplicants; i++) {
-		if (!strcmp(epsy->supplied_to[i], psy->name)) {
-			if (epsy->get_property(epsy,
-				  POWER_SUPPLY_PROP_ONLINE, &ret))
-				continue;
+	if (__power_supply_is_supplied_by(epsy, psy)) {
+		if (!epsy->get_property(epsy, POWER_SUPPLY_PROP_ONLINE, &ret)) {
 			if (ret.intval)
 				return ret.intval;
 		}
@@ -334,6 +353,9 @@  int power_supply_register(struct device *parent, struct power_supply *psy)
 	dev_set_drvdata(dev, psy);
 	psy->dev = dev;
 
+#ifdef CONFIG_OF
+	INIT_LIST_HEAD(&psy->supplies.list);
+#endif
 	INIT_WORK(&psy->changed_work, power_supply_changed_work);
 
 	rc = kobject_set_name(&dev->kobj, "%s", psy->name);
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 1f0ab90..d16f6ab 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -15,6 +15,7 @@ 
 
 #include <linux/workqueue.h>
 #include <linux/leds.h>
+#include <linux/list.h>
 
 struct device;
 
@@ -160,12 +161,20 @@  union power_supply_propval {
 	const char *strval;
 };
 
+struct power_supply_supplies {
+	struct device_node *node;
+	struct list_head list;
+};
+
 struct power_supply {
 	const char *name;
 	enum power_supply_type type;
 	enum power_supply_property *properties;
 	size_t num_properties;
 
+#ifdef CONFIG_OF
+	struct power_supply_supplies supplies;
+#endif
 	char **supplied_to;
 	size_t num_supplicants;