[PATCH 0/2] power supply class support for charger managers

[PATCH 0/2] power supply class support for charger managers

[Ramakrishna Pallala]

As we are heading towards kernel charger manager soulutions and few
are already in mainaline and few are in pipeline. It makes sense have
uniform low level driver support independent of charger manager frameworks.

This patchset add the neccessary changes to power supply class
to make charger drivers usable with charger managers.

And also enables charegr manager to use power supply drivers
for charge controls.

Ramakrishna Pallala (2):
  power_supply: Add charge control struct in power supply class
  charger_manager: Enable power supply driver support for charge
    controls

 drivers/power/Kconfig                 |    6 ++
 drivers/power/charger-manager.c       |   23 +++++----
 drivers/power/power_supply_core.c     |   18 +++++++
 include/linux/power/charger-manager.h |   84 +++++++++++++++++++++++++++++++++
 include/linux/power_supply.h          |   26 ++++++++++
 5 files changed, 146 insertions(+), 11 deletions(-)

[PATCH 1/2] power_supply: Add charge control struct in power supply class

[Ramakrishna Pallala]

This patch adds power supply charge control structure to power_supply struct
and add a Kconfig flag to use these controls by charger frameworks.

This patch also adds a helper function/API to return the charge control
struct to the charger frameworks.

Signed-off-by: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
---
 drivers/power/Kconfig             |    6 ++++++
 drivers/power/power_supply_core.c |   18 ++++++++++++++++++
 include/linux/power_supply.h      |   26 ++++++++++++++++++++++++++
 3 files changed, 50 insertions(+), 0 deletions(-)

diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index 263499f..782067b 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -346,6 +346,12 @@ config AB8500_BM
 	help
 	  Say Y to include support for AB8500 battery management.
 
+config PSY_CM_LOW_LEVEL_SUPPORT
+	bool "Low level driver support for charger managers"
+	help
+	  Say Y here to enable low level power supply class drivers
+	  support for charger manager or framework.
+
 endif # POWER_SUPPLY
 
 source "drivers/power/avs/Kconfig"
diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
index f984da1..0f393c6 100644
--- a/drivers/power/power_supply_core.c
+++ b/drivers/power/power_supply_core.c
@@ -158,6 +158,24 @@ struct power_supply *power_supply_get_by_name(char *name)
 }
 EXPORT_SYMBOL_GPL(power_supply_get_by_name);
 
+#ifdef CONFIG_PSY_CM_LOW_LEVEL_SUPPORT
+struct power_supply_charger_control
+		*power_supply_get_chrg_cntl_by_name(const char *name)
+{
+	struct device *dev = class_find_device(power_supply_class, NULL,
+			(char *)name, power_supply_match_device_by_name);
+
+	return dev ? ((struct power_supply *)dev_get_drvdata(dev))->chrg_cntl : NULL;
+}
+#else
+struct power_supply_charger_control
+		*power_supply_get_chrg_cntl_by_name(const char *name)
+{
+	return NULL;
+}
+#endif
+EXPORT_SYMBOL_GPL(power_supply_get_chrg_cntl_by_name);
+
 int power_supply_powers(struct power_supply *psy, struct device *dev)
 {
 	return sysfs_create_link(&psy->dev->kobj, &dev->kobj, "powers");
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 1f0ab90..35cdf2c 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -191,6 +191,10 @@ struct power_supply {
 	struct thermal_cooling_device *tcd;
 #endif
 
+#ifdef CONFIG_PSY_CM_LOW_LEVEL_SUPPORT
+	struct power_supply_charger_control *chrg_cntl;
+#endif
+
 #ifdef CONFIG_LEDS_TRIGGERS
 	struct led_trigger *charging_full_trig;
 	char *charging_full_trig_name;
@@ -224,7 +228,29 @@ struct power_supply_info {
 	int use_for_apm;
 };
 
+struct power_supply_charger_control {
+	const char *name;
+	/* get charging status */
+	int (*is_charging_enabled)(void);
+	int (*is_charger_enabled)(void);
+
+	/* set charging parameters */
+	int (*set_in_current_limit)(int uA);
+	int (*set_charge_current)(int uA);
+	int (*set_charge_voltage)(int uV);
+
+	/* control battery charging */
+	int (*enable_charging)(void);
+	int (*disable_charging)(void);
+
+	/* control VSYS or system supply */
+	int (*turnon_charger)(void);
+	int (*turnoff_charger)(void);
+};
+
 extern struct power_supply *power_supply_get_by_name(char *name);
+extern struct power_supply_charger_control
+		*power_supply_get_chrg_cntl_by_name(const char *name);
 extern void power_supply_changed(struct power_supply *psy);
 extern int power_supply_am_i_supplied(struct power_supply *psy);
 extern int power_supply_set_battery_charged(struct power_supply *psy);
-- 
1.7.0.4

[PATCHv2 2/2] charger_manager: Enable power supply driver support for charge controls

[Ramakrishna Pallala]

This patch enables the charger manager support for power supply drivers.

Note: this patch does NOT change the variable/function names in charger
manager as it is just an enabling patch. I will submit incremental patches
to clean some of naming conventions to appeal more reasonable/logical to the
source code reader/developer.

Signed-off-by: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
---
 drivers/power/charger-manager.c       |   23 +++++----
 include/linux/power/charger-manager.h |   84 +++++++++++++++++++++++++++++++++
 2 files changed, 96 insertions(+), 11 deletions(-)

diff --git a/drivers/power/charger-manager.c b/drivers/power/charger-manager.c
index 8a0aca6..8f6d525 100644
--- a/drivers/power/charger-manager.c
+++ b/drivers/power/charger-manager.c
@@ -336,7 +336,8 @@ static int try_charger_enable(struct charger_manager *cm, bool enable)
 			if (desc->charger_regulators[i].externally_control)
 				continue;
 
-			err = regulator_enable(desc->charger_regulators[i].consumer);
+			err = psy_cms_enable_charging(
+					desc->charger_regulators[i].consumer);
 			if (err < 0) {
 				dev_warn(cm->dev,
 					"Cannot enable %s regulator\n",
@@ -355,7 +356,8 @@ static int try_charger_enable(struct charger_manager *cm, bool enable)
 			if (desc->charger_regulators[i].externally_control)
 				continue;
 
-			err = regulator_disable(desc->charger_regulators[i].consumer);
+			err = psy_cms_disable_charging(
+				desc->charger_regulators[i].consumer);
 			if (err < 0) {
 				dev_warn(cm->dev,
 					"Cannot disable %s regulator\n",
@@ -368,9 +370,9 @@ static int try_charger_enable(struct charger_manager *cm, bool enable)
 		 * even if charger was enabled at the other places
 		 */
 		for (i = 0; i < desc->num_charger_regulators; i++) {
-			if (regulator_is_enabled(
+			if (psy_cms_is_charging_enabled(
 				    desc->charger_regulators[i].consumer)) {
-				regulator_force_disable(
+				psy_cms_force_disable_charging(
 					desc->charger_regulators[i].consumer);
 				dev_warn(cm->dev,
 					"Disable regulator(%s) forcibly.\n",
@@ -1135,8 +1137,8 @@ static void charger_extcon_work(struct work_struct *work)
 	int ret;
 
 	if (cable->attached && cable->min_uA != 0 && cable->max_uA != 0) {
-		ret = regulator_set_current_limit(cable->charger->consumer,
-					cable->min_uA, cable->max_uA);
+		ret = psy_cms_set_current_limit(cable->charger->consumer,
+						cable->min_uA, cable->max_uA);
 		if (ret < 0) {
 			pr_err("Cannot set current limit of %s (%s)\n",
 				cable->charger->regulator_name, cable->name);
@@ -1239,7 +1241,7 @@ static ssize_t charger_state_show(struct device *dev,
 	int state = 0;
 
 	if (!charger->externally_control)
-		state = regulator_is_enabled(charger->consumer);
+		state = psy_cms_is_charging_enabled(charger->consumer);
 
 	return sprintf(buf, "%s\n", state ? "enabled" : "disabled");
 }
@@ -1507,8 +1509,7 @@ static int charger_manager_probe(struct platform_device *pdev)
 					= &desc->charger_regulators[i];
 		char buf[11];
 		char *str;
-
-		charger->consumer = regulator_get(&pdev->dev,
+		charger->consumer = psy_cms_get_consumer(&pdev->dev,
 					charger->regulator_name);
 		if (charger->consumer == NULL) {
 			dev_err(&pdev->dev, "Cannot find charger(%s)n",
@@ -1639,7 +1640,7 @@ err_extcon:
 	}
 err_chg_get:
 	for (i = 0 ; i < desc->num_charger_regulators ; i++)
-		regulator_put(desc->charger_regulators[i].consumer);
+		psy_cms_put_consumer(desc->charger_regulators[i].consumer);
 
 	power_supply_unregister(&cm->charger_psy);
 err_register:
@@ -1682,7 +1683,7 @@ static int __devexit charger_manager_remove(struct platform_device *pdev)
 	}
 
 	for (i = 0 ; i < desc->num_charger_regulators ; i++)
-		regulator_put(desc->charger_regulators[i].consumer);
+		psy_cms_put_consumer(desc->charger_regulators[i].consumer);
 
 	power_supply_unregister(&cm->charger_psy);
 
diff --git a/include/linux/power/charger-manager.h b/include/linux/power/charger-manager.h
index 0e86840..f6a5b79 100644
--- a/include/linux/power/charger-manager.h
+++ b/include/linux/power/charger-manager.h
@@ -127,7 +127,11 @@ struct charger_cable {
 struct charger_regulator {
 	/* The name of regulator for charging */
 	const char *regulator_name;
+#ifdef CONFIG_PSY_CM_LOW_LEVEL_SUPPORT
+	struct power_supply_charger_control *consumer;
+#else
 	struct regulator *consumer;
+#endif
 
 	/* charger never on when system is on */
 	int externally_control;
@@ -280,4 +284,84 @@ static inline bool cm_suspend_again(void) { return false; }
 static inline void cm_notify_event(struct power_supply *psy,
 				enum cm_event_types type, char *msg) { }
 #endif
+
+#ifdef CONFIG_PSY_CM_LOW_LEVEL_SUPPORT
+static inline int psy_cms_enable_charging(
+			struct power_supply_charger_control *consumer)
+{
+	return consumer->enable_charging();
+}
+
+static inline int psy_cms_disable_charging(
+			struct power_supply_charger_control *consumer)
+{
+	return consumer->disable_charging();
+}
+
+static inline int psy_cms_force_disable_charging(
+			struct power_supply_charger_control *consumer)
+{
+	return consumer->disable_charging();
+}
+
+static inline int psy_cms_set_current_limit(struct power_supply_charger_control
+					*consumer, int min_uA, int max_uA)
+{
+	return consumer->set_in_current_limit(min_uA);
+}
+
+static inline int psy_cms_is_charging_enabled(
+			struct power_supply_charger_control *consumer)
+{
+	return consumer->is_charging_enabled();
+}
+
+static inline struct power_supply_charger_control
+		*psy_cms_get_consumer(struct device *dev, const char *name)
+{
+	return power_supply_get_chrg_cntl_by_name(name);
+}
+
+static inline void psy_cms_put_consumer(
+				struct power_supply_charger_control *consumer)
+{
+}
+#else
+static inline int psy_cms_enable_charging(struct regulator *consumer)
+{
+	return regulator_enable(consumer);
+}
+
+static inline int psy_cms_disable_charging(struct regulator *consumer)
+{
+	return regulator_disable(consumer);
+}
+
+static inline int psy_cms_force_disable_charging(struct regulator *consumer)
+{
+	return regulator_force_disable(consumer);
+}
+
+static inline int psy_cms_set_current_limit(struct regulator *consumer,
+						int min_uA, int max_uA)
+{
+	return regulator_set_current_limit(consumer, min_uA, max_uA);
+}
+
+static inline int psy_cms_is_charging_enabled(struct regulator *consumer)
+{
+	return regulator_is_enabled(consumer);
+}
+
+static inline struct regulator *psy_cms_get_consumer(struct device *dev, const char *name)
+{
+	return regulator_get(dev, name);
+}
+
+static inline void psy_cms_put_consumer(struct regulator *consumer)
+{
+	regulator_put(consumer);
+}
+#endif
+
 #endif /* _CHARGER_MANAGER_H */
-- 
1.7.0.4

Re: [PATCH 1/2] power_supply: Add charge control struct in power supply class

[Anton Vorontsov]

On Tue, Nov 27, 2012 at 01:17:02PM +0530, Ramakrishna Pallala wrote:
[...]
> +++ b/drivers/power/power_supply_core.c
> @@ -158,6 +158,24 @@ struct power_supply *power_supply_get_by_name(char *name)
>  }
>  EXPORT_SYMBOL_GPL(power_supply_get_by_name);
>  
> +#ifdef CONFIG_PSY_CM_LOW_LEVEL_SUPPORT
> +struct power_supply_charger_control
> +		*power_supply_get_chrg_cntl_by_name(const char *name)
> +{
> +	struct device *dev = class_find_device(power_supply_class, NULL,
> +			(char *)name, power_supply_match_device_by_name);
> +
> +	return dev ? ((struct power_supply *)dev_get_drvdata(dev))->chrg_cntl : NULL;
> +}
> +#else
> +struct power_supply_charger_control
> +		*power_supply_get_chrg_cntl_by_name(const char *name)
> +{
> +	return NULL;
> +}
> +#endif
> +EXPORT_SYMBOL_GPL(power_supply_get_chrg_cntl_by_name);

>  int power_supply_powers(struct power_supply *psy, struct device *dev)
>  {
>  	return sysfs_create_link(&psy->dev->kobj, &dev->kobj, "powers");
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 1f0ab90..35cdf2c 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -191,6 +191,10 @@ struct power_supply {
>  	struct thermal_cooling_device *tcd;
>  #endif
>  
> +#ifdef CONFIG_PSY_CM_LOW_LEVEL_SUPPORT
> +	struct power_supply_charger_control *chrg_cntl;
> +#endif
> +
>  #ifdef CONFIG_LEDS_TRIGGERS
>  	struct led_trigger *charging_full_trig;
>  	char *charging_full_trig_name;
> @@ -224,7 +228,29 @@ struct power_supply_info {
>  	int use_for_apm;
>  };
>  
> +struct power_supply_charger_control {
> +	const char *name;
> +	/* get charging status */
> +	int (*is_charging_enabled)(void);
> +	int (*is_charger_enabled)(void);
> +
> +	/* set charging parameters */
> +	int (*set_in_current_limit)(int uA);
> +	int (*set_charge_current)(int uA);
> +	int (*set_charge_voltage)(int uV);
> +
> +	/* control battery charging */
> +	int (*enable_charging)(void);
> +	int (*disable_charging)(void);
> +
> +	/* control VSYS or system supply */
> +	int (*turnon_charger)(void);
> +	int (*turnoff_charger)(void);
> +};
> +

I'm all for this patch, but why do you need to place it into
power_supply.h and power_supply_core.c? :) I see nothing generic here,
it's pure charger-manager stuff. So, place everything into
charger-manager.{c,h}.

You can still add this:

> +#ifdef CONFIG_PSY_CM_LOW_LEVEL_SUPPORT
> +	struct power_supply_charger_control *chrg_cntl;
> +#endif

to power_supply.h, of course. It's OK.

>  extern struct power_supply *power_supply_get_by_name(char *name);
> +extern struct power_supply_charger_control
> +		*power_supply_get_chrg_cntl_by_name(const char *name);
>  extern void power_supply_changed(struct power_supply *psy);
>  extern int power_supply_am_i_supplied(struct power_supply *psy);
>  extern int power_supply_set_battery_charged(struct power_supply *psy);
> -- 
> 1.7.0.4

RE: [PATCH 1/2] power_supply: Add charge control struct in power supply class

[Pallala, Ramakrishna]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1480 bytes --]

> > +struct power_supply_charger_control {
> > +	const char *name;
> > +	/* get charging status */
> > +	int (*is_charging_enabled)(void);
> > +	int (*is_charger_enabled)(void);
> > +
> > +	/* set charging parameters */
> > +	int (*set_in_current_limit)(int uA);
> > +	int (*set_charge_current)(int uA);
> > +	int (*set_charge_voltage)(int uV);
> > +
> > +	/* control battery charging */
> > +	int (*enable_charging)(void);
> > +	int (*disable_charging)(void);
> > +
> > +	/* control VSYS or system supply */
> > +	int (*turnon_charger)(void);
> > +	int (*turnoff_charger)(void);
> > +};
> > +
> 
> I'm all for this patch, but why do you need to place it into power_supply.h and
> power_supply_core.c? :) I see nothing generic here, it's pure charger-manager
> stuff. So, place everything into charger-manager.{c,h}.

Hi Anton,

The main reason for keeping this stuff in power_supply.h and power_supply_core.c is to make these interfaces uniform
Across multiple charger frameworks and to avoid each charger framework define it's own interfaces. If there is need for new callback
They can add to the existing struct defined above and it will available to all the frameworks. Also the work required to support a new
Framework will be reduced if the driver already support any one of the existing frameworks.
 
Thanks,
Ram
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH 1/2] power_supply: Add charge control struct in power supply class

[Tc, Jenny]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1754 bytes --]

> 
> > > +struct power_supply_charger_control {
> > > +	const char *name;
> > > +	/* get charging status */
> > > +	int (*is_charging_enabled)(void);
> > > +	int (*is_charger_enabled)(void);
> > > +
> > > +	/* set charging parameters */
> > > +	int (*set_in_current_limit)(int uA);
> > > +	int (*set_charge_current)(int uA);
> > > +	int (*set_charge_voltage)(int uV);
> > > +
> > > +	/* control battery charging */
> > > +	int (*enable_charging)(void);
> > > +	int (*disable_charging)(void);
> > > +
> > > +	/* control VSYS or system supply */
> > > +	int (*turnon_charger)(void);
> > > +	int (*turnoff_charger)(void);
> > > +};
> > > +
> >
> > I'm all for this patch, but why do you need to place it into
> > power_supply.h and power_supply_core.c? :) I see nothing generic here,
> > it's pure charger-manager stuff. So, place everything into charger-
> manager.{c,h}.
> 
> Hi Anton,
> 
> The main reason for keeping this stuff in power_supply.h and
> power_supply_core.c is to make these interfaces uniform Across multiple
> charger frameworks and to avoid each charger framework define it's own
> interfaces. If there is need for new callback They can add to the existing struct
> defined above and it will available to all the frameworks. Also the work
> required to support a new Framework will be reduced if the driver already
> support any one of the existing frameworks.
> 

Rama,

The similar functionalities are exposed by patch https://lkml.org/lkml/2012/10/18/219.
As per Anton's review comments on this patch, I'll be moving the macros to power_supply.h.
Wouldn't that be enough ?
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 1/2] power_supply: Add charge control struct in power supply class

[Anton Vorontsov]

On Mon, Jan 07, 2013 at 05:23:36AM +0000, Tc, Jenny wrote:
> > 
> > > > +struct power_supply_charger_control {
> > > > +	const char *name;
> > > > +	/* get charging status */
> > > > +	int (*is_charging_enabled)(void);
> > > > +	int (*is_charger_enabled)(void);
> > > > +
> > > > +	/* set charging parameters */
> > > > +	int (*set_in_current_limit)(int uA);
> > > > +	int (*set_charge_current)(int uA);
> > > > +	int (*set_charge_voltage)(int uV);
> > > > +
> > > > +	/* control battery charging */
> > > > +	int (*enable_charging)(void);
> > > > +	int (*disable_charging)(void);
> > > > +
> > > > +	/* control VSYS or system supply */
> > > > +	int (*turnon_charger)(void);
> > > > +	int (*turnoff_charger)(void);
> > > > +};
> > > > +
> > >
> > > I'm all for this patch, but why do you need to place it into
> > > power_supply.h and power_supply_core.c? :) I see nothing generic here,
> > > it's pure charger-manager stuff. So, place everything into charger-
> > manager.{c,h}.
> > 
> > Hi Anton,
> > 
> > The main reason for keeping this stuff in power_supply.h and
> > power_supply_core.c is to make these interfaces uniform Across multiple
> > charger frameworks and to avoid each charger framework define it's own
> > interfaces. If there is need for new callback They can add to the existing struct
> > defined above and it will available to all the frameworks. Also the work
> > required to support a new Framework will be reduced if the driver already
> > support any one of the existing frameworks.
> > 
> 
> Rama,
> 
> The similar functionalities are exposed by patch https://lkml.org/lkml/2012/10/18/219.
> As per Anton's review comments on this patch, I'll be moving the macros to power_supply.h.
> Wouldn't that be enough ?

Btw, how do these two sets relate to each other? Both seem to control
chargers in some way... But yours approach uses properties, which I like
more.

I guess you should coordinate on this?

Thanks,
Anton

RE: [PATCH 1/2] power_supply: Add charge control struct in power supply class

[Pallala, Ramakrishna]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1991 bytes --]

> > > > +struct power_supply_charger_control {
> > > > +	const char *name;
> > > > +	/* get charging status */
> > > > +	int (*is_charging_enabled)(void);
> > > > +	int (*is_charger_enabled)(void);
> > > > +
> > > > +	/* set charging parameters */
> > > > +	int (*set_in_current_limit)(int uA);
> > > > +	int (*set_charge_current)(int uA);
> > > > +	int (*set_charge_voltage)(int uV);
> > > > +
> > > > +	/* control battery charging */
> > > > +	int (*enable_charging)(void);
> > > > +	int (*disable_charging)(void);
> > > > +
> > > > +	/* control VSYS or system supply */
> > > > +	int (*turnon_charger)(void);
> > > > +	int (*turnoff_charger)(void);
> > > > +};
> > > > +
> > >
> > > I'm all for this patch, but why do you need to place it into
> > > power_supply.h and power_supply_core.c? :) I see nothing generic
> > > here, it's pure charger-manager stuff. So, place everything into
> > > charger-
> > manager.{c,h}.
> >
> > Hi Anton,
> >
> > The main reason for keeping this stuff in power_supply.h and
> > power_supply_core.c is to make these interfaces uniform Across
> > multiple charger frameworks and to avoid each charger framework define
> > it's own interfaces. If there is need for new callback They can add to
> > the existing struct defined above and it will available to all the
> > frameworks. Also the work required to support a new Framework will be
> > reduced if the driver already support any one of the existing frameworks.
> >
> 
> Rama,
> 
> The similar functionalities are exposed by patch
> https://lkml.org/lkml/2012/10/18/219.
> As per Anton's review comments on this patch, I'll be moving the macros to
> power_supply.h.
> Wouldn't that be enough ?

Though the macros seem to be fine but I would still think that call back way of interfaces would be
More flexible and straightforward.

Thanks,
Ram
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH 1/2] power_supply: Add charge control struct in power supply class

[Pallala, Ramakrishna]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2186 bytes --]

> > > > > +struct power_supply_charger_control {
> > > > > +	const char *name;
> > > > > +	/* get charging status */
> > > > > +	int (*is_charging_enabled)(void);
> > > > > +	int (*is_charger_enabled)(void);
> > > > > +
> > > > > +	/* set charging parameters */
> > > > > +	int (*set_in_current_limit)(int uA);
> > > > > +	int (*set_charge_current)(int uA);
> > > > > +	int (*set_charge_voltage)(int uV);
> > > > > +
> > > > > +	/* control battery charging */
> > > > > +	int (*enable_charging)(void);
> > > > > +	int (*disable_charging)(void);
> > > > > +
> > > > > +	/* control VSYS or system supply */
> > > > > +	int (*turnon_charger)(void);
> > > > > +	int (*turnoff_charger)(void);
> > > > > +};
> > > > > +
> > > >
> > > > I'm all for this patch, but why do you need to place it into
> > > > power_supply.h and power_supply_core.c? :) I see nothing generic
> > > > here, it's pure charger-manager stuff. So, place everything into
> > > > charger-
> > > manager.{c,h}.
> > >
> > > Hi Anton,
> > >
> > > The main reason for keeping this stuff in power_supply.h and
> > > power_supply_core.c is to make these interfaces uniform Across
> > > multiple charger frameworks and to avoid each charger framework
> > > define it's own interfaces. If there is need for new callback They
> > > can add to the existing struct defined above and it will available
> > > to all the frameworks. Also the work required to support a new
> > > Framework will be reduced if the driver already support any one of the
> existing frameworks.
> > >
> >
> > Rama,
> >
> > The similar functionalities are exposed by patch
> https://lkml.org/lkml/2012/10/18/219.
> > As per Anton's review comments on this patch, I'll be moving the macros to
> power_supply.h.
> > Wouldn't that be enough ?
> 
> Btw, how do these two sets relate to each other? Both seem to control chargers
> in some way... But yours approach uses properties, which I like more.
> 
> I guess you should coordinate on this?

Sure Anton we will sync up on this and get back to you. 
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 1/2] power_supply: Add charge control struct in power supply class

[Anton Vorontsov]

On Mon, Jan 07, 2013 at 05:43:58AM +0000, Pallala, Ramakrishna wrote:
> > > > > +struct power_supply_charger_control {
> > > > > +	const char *name;
> > > > > +	/* get charging status */
> > > > > +	int (*is_charging_enabled)(void);
> > > > > +	int (*is_charger_enabled)(void);
[...]
> > The similar functionalities are exposed by patch
> > https://lkml.org/lkml/2012/10/18/219.
> > As per Anton's review comments on this patch, I'll be moving the macros to
> > power_supply.h.
> > Wouldn't that be enough ?
> 
> Though the macros seem to be fine but I would still think that call back way of interfaces would be
> More flexible and straightforward.

We have properties mechanism deployed already, so I'd rather keep it
consistent.

p.s. We had 'properties vs. callbacks' debates before, there are pros and
cons of each approach. Unless there are some unresolvable issues, let's
stick with the original approach. :)

[PATCH 1/2] power_supply: Add charge control struct in power supply class

[Ramakrishna Pallala]

This patch adds power supply charge control structure to power_supply struct
and add a Kconfig flag to use these controls by charger frameworks.

This patch also adds a helper function/API to return the charge control
struct to the charger frameworks.

Signed-off-by: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
---
 drivers/power/Kconfig             |    6 ++++++
 drivers/power/power_supply_core.c |   18 ++++++++++++++++++
 include/linux/power_supply.h      |   26 ++++++++++++++++++++++++++
 3 files changed, 50 insertions(+), 0 deletions(-)

diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index 263499f..782067b 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -346,6 +346,12 @@ config AB8500_BM
 	help
 	  Say Y to include support for AB8500 battery management.
 
+config PSY_CM_LOW_LEVEL_SUPPORT
+	bool "Low level driver support for charger managers"
+	help
+	  Say Y here to enable low level power supply class drivers
+	  support for charger manager or framework.
+
 endif # POWER_SUPPLY
 
 source "drivers/power/avs/Kconfig"
diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
index f984da1..0f393c6 100644
--- a/drivers/power/power_supply_core.c
+++ b/drivers/power/power_supply_core.c
@@ -158,6 +158,24 @@ struct power_supply *power_supply_get_by_name(char *name)
 }
 EXPORT_SYMBOL_GPL(power_supply_get_by_name);
 
+#ifdef CONFIG_PSY_CM_LOW_LEVEL_SUPPORT
+struct power_supply_charger_control
+		*power_supply_get_chrg_cntl_by_name(const char *name)
+{
+	struct device *dev = class_find_device(power_supply_class, NULL,
+			(char *)name, power_supply_match_device_by_name);
+
+	return dev ? ((struct power_supply *)dev_get_drvdata(dev))->chrg_cntl : NULL;
+}
+#else
+struct power_supply_charger_control
+		*power_supply_get_chrg_cntl_by_name(const char *name)
+{
+	return NULL;
+}
+#endif
+EXPORT_SYMBOL_GPL(power_supply_get_chrg_cntl_by_name);
+
 int power_supply_powers(struct power_supply *psy, struct device *dev)
 {
 	return sysfs_create_link(&psy->dev->kobj, &dev->kobj, "powers");
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 1f0ab90..35cdf2c 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -191,6 +191,10 @@ struct power_supply {
 	struct thermal_cooling_device *tcd;
 #endif
 
+#ifdef CONFIG_PSY_CM_LOW_LEVEL_SUPPORT
+	struct power_supply_charger_control *chrg_cntl;
+#endif
+
 #ifdef CONFIG_LEDS_TRIGGERS
 	struct led_trigger *charging_full_trig;
 	char *charging_full_trig_name;
@@ -224,7 +228,29 @@ struct power_supply_info {
 	int use_for_apm;
 };
 
+struct power_supply_charger_control {
+	const char *name;
+	/* get charging status */
+	int (*is_charging_enabled)(void);
+	int (*is_charger_enabled)(void);
+
+	/* set charging parameters */
+	int (*set_in_current_limit)(int uA);
+	int (*set_charge_current)(int uA);
+	int (*set_charge_voltage)(int uV);
+
+	/* control battery charging */
+	int (*enable_charging)(void);
+	int (*disable_charging)(void);
+
+	/* control VSYS or system supply */
+	int (*turnon_charger)(void);
+	int (*turnoff_charger)(void);
+};
+
 extern struct power_supply *power_supply_get_by_name(char *name);
+extern struct power_supply_charger_control
+		*power_supply_get_chrg_cntl_by_name(const char *name);
 extern void power_supply_changed(struct power_supply *psy);
 extern int power_supply_am_i_supplied(struct power_supply *psy);
 extern int power_supply_set_battery_charged(struct power_supply *psy);
-- 
1.7.0.4