| Message ID | 1361488272-21010-3-git-send-email-rklein@nvidia.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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
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
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
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
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
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
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
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
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
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
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;
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(-)