The Linux Kernel Mailing List
 help / color / mirror / Atom feed
* [PATCH v2] charger-manager: Add support sysfs entry for charger
@ 2012-09-21  9:49 Chanwoo Choi
  2012-09-21 23:43 ` Anton Vorontsov
  0 siblings, 1 reply; 3+ messages in thread
From: Chanwoo Choi @ 2012-09-21  9:49 UTC (permalink / raw)
  To: anton.vorontsov
  Cc: cbouatmailru, jenny.tc, ramakrishna.pallala, myungjoo.ham,
	kyungmin.park, linux-kernel, Chanwoo Choi

This patch add support sysfs entry for each charger(regulator).
Charger-manager use one or more chargers for charging battery but
some charger isn't necessary on specific scenario. So, if some charger
isn't needed, can disable specific charger through 'externally_control'
entry while system is on state and confirm the information(name, state)
of charger.

patch-v2:
- Fix build break and fix codingstype which remove space in the for loop statement

  CHECK   drivers/power/charger-manager.c
drivers/power/charger-manager.c:1559:17: error: undefined identifier 'cable'
drivers/power/charger-manager.c:1564:17: error: undefined identifier 'cable'
drivers/power/charger-manager.c:1569:17: error: undefined identifier 'cable'
  CC      drivers/power/charger-manager.o
drivers/power/charger-manager.c: In function 'charger_manager_probe':
drivers/power/charger-manager.c:1559:3: error: 'cable' undeclared (first use in this function)
drivers/power/charger-manager.c:1559:3: note: each undeclared identifier is reported only once for each function it appears in
make[1]: *** [drivers/power/charger-manager.o] Error 1

patch-v1:
the list of added sysfs entry
- /sys/class/power_supply/battery/chargers/charger.[index]/name
: show name of charger(regulator)
- /sys/class/power_supply/battery/chargers/charger.[index]/state
: show either enabled or disabled state of charger
- /sys/class/power_supply/battery/chargers/charger.[index]/externally_control

If 'externally_control' of specific charger is 1, Charger-manager cannot enable
regulator for charging when charger cable is attached and charger must be
maintained with disabled state. If 'externally_control' is zero, Charger-manager
usually can control to enable/disable regulator.

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Signed-off-by: Myungjoo Ham <myungjoo.ham@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/power/charger-manager.c       |  174 ++++++++++++++++++++++++++++++++-
 include/linux/power/charger-manager.h |   19 ++++
 2 files changed, 192 insertions(+), 1 deletions(-)

diff --git a/drivers/power/charger-manager.c b/drivers/power/charger-manager.c
index e92ec55..a1ab825 100644
--- a/drivers/power/charger-manager.c
+++ b/drivers/power/charger-manager.c
@@ -22,6 +22,7 @@
 #include <linux/platform_device.h>
 #include <linux/power/charger-manager.h>
 #include <linux/regulator/consumer.h>
+#include <linux/sysfs.h>
 
 static const char * const default_event_names[] = {
 	[CM_EVENT_UNKNOWN] = "Unknown",
@@ -332,6 +333,9 @@ static int try_charger_enable(struct charger_manager *cm, bool enable)
 		cm->charging_end_time = 0;
 
 		for (i = 0 ; i < desc->num_charger_regulators ; i++) {
+			if (desc->charger_regulators[i].externally_control)
+				continue;
+
 			err = regulator_enable(desc->charger_regulators[i].consumer);
 			if (err < 0) {
 				dev_warn(cm->dev,
@@ -348,6 +352,9 @@ static int try_charger_enable(struct charger_manager *cm, bool enable)
 		cm->charging_end_time = ktime_to_ms(ktime_get());
 
 		for (i = 0 ; i < desc->num_charger_regulators ; i++) {
+			if (desc->charger_regulators[i].externally_control)
+				continue;
+
 			err = regulator_disable(desc->charger_regulators[i].consumer);
 			if (err < 0) {
 				dev_warn(cm->dev,
@@ -1217,12 +1224,101 @@ static int charger_extcon_init(struct charger_manager *cm,
 	return ret;
 }
 
+/* help function of sysfs node to control charger(regulator) */
+static ssize_t charger_name_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct charger_regulator *charger
+		= container_of(attr, struct charger_regulator, attr_name);
+	return sprintf(buf, "%s\n", charger->regulator_name);
+}
+
+static ssize_t charger_state_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct charger_regulator *charger
+		= container_of(attr, struct charger_regulator, attr_state);
+	int state = 1;
+
+	if (charger->externally_control)
+		state = 0;
+	else
+		state = regulator_is_enabled(charger->consumer);
+
+	return sprintf(buf, "%s\n", state ? "enabled" : "disabled");
+}
+
+static ssize_t charger_externally_control_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct charger_regulator *charger = container_of(attr,
+			struct charger_regulator, attr_externally_control);
+	return sprintf(buf, "%d\n", charger->externally_control);
+}
+
+static ssize_t charger_externally_control_store(struct device *dev,
+				struct device_attribute *attr, const char *buf,
+				size_t count)
+{
+	struct charger_regulator *charger
+		= container_of(attr, struct charger_regulator,
+					attr_externally_control);
+	struct charger_manager *cm = charger->cm;
+	struct charger_desc *desc = cm->desc;
+	int i, ret;
+	int externally_control;
+
+	ret = sscanf(buf, "%d", &externally_control);
+	if (ret == 0) {
+		ret = -EINVAL;
+		return ret;
+	}
+
+	if (externally_control) {
+		int chargers_externally_control = 1;
+
+		for (i = 0 ; i < desc->num_charger_regulators ; i++) {
+			if ((&desc->charger_regulators[i] != charger)
+			&& !desc->charger_regulators[i].externally_control) {
+				/*
+				 * At least, one charger is controlled by
+				 * charger-manager
+				 */
+				chargers_externally_control = 0;
+				break;
+			}
+		}
+
+		if (!chargers_externally_control) {
+			if (cm->charger_enabled) {
+				try_charger_enable(charger->cm, false);
+				charger->externally_control
+					= externally_control;
+				try_charger_enable(charger->cm, true);
+			} else
+				charger->externally_control
+					= externally_control;
+
+		} else {
+			dev_warn(cm->dev,
+				"\'%s\' regulator should be controlled "
+				"in charger-manager because charger-manager "
+				"must need at least one charger for charging\n",
+				charger->regulator_name);
+		}
+	} else
+		charger->externally_control = externally_control;
+
+	return count;
+}
+
 static int charger_manager_probe(struct platform_device *pdev)
 {
 	struct charger_desc *desc = dev_get_platdata(&pdev->dev);
 	struct charger_manager *cm;
 	int ret = 0, i = 0;
 	int j = 0;
+	int chargers_externally_control = 1;
 	union power_supply_propval val;
 
 	if (g_desc && !rtc_dev && g_desc->rtc_name) {
@@ -1412,6 +1508,8 @@ static int charger_manager_probe(struct platform_device *pdev)
 	for (i = 0 ; i < desc->num_charger_regulators ; i++) {
 		struct charger_regulator *charger
 					= &desc->charger_regulators[i];
+		char buf[11];
+		char *str;
 
 		charger->consumer = regulator_get(&pdev->dev,
 					charger->regulator_name);
@@ -1421,8 +1519,9 @@ static int charger_manager_probe(struct platform_device *pdev)
 			ret = -EINVAL;
 			goto err_chg_get;
 		}
+		charger->cm = cm;
 
-		for (j = 0 ; j < charger->num_cables ; j++) {
+		for (j = 0; j < charger->num_cables; j++) {
 			struct charger_cable *cable = &charger->cables[j];
 
 			ret = charger_extcon_init(cm, cable);
@@ -1434,6 +1533,71 @@ static int charger_manager_probe(struct platform_device *pdev)
 			cable->charger = charger;
 			cable->cm = cm;
 		}
+
+		/* Create sysfs entry to control charger(regulator) */
+		snprintf(buf, 10, "charger.%d", i);
+		str = kzalloc(sizeof(char) * (strlen(buf) + 1), GFP_KERNEL);
+		if (!str) {
+			for (i--; i >= 0; i--) {
+				charger = &desc->charger_regulators[i];
+				kfree(charger->attr_g.name);
+			}
+			ret = -ENOMEM;
+
+			goto err_extcon;
+		}
+		strcpy(str, buf);
+
+		charger->attrs[0] = &charger->attr_name.attr;
+		charger->attrs[1] = &charger->attr_state.attr;
+		charger->attrs[2] = &charger->attr_externally_control.attr;
+		charger->attrs[3] = NULL;
+		charger->attr_g.name = str;
+		charger->attr_g.attrs = charger->attrs;
+
+		sysfs_attr_init(&charger->attr_name.attr);
+		charger->attr_name.attr.name = "name";
+		charger->attr_name.attr.mode = 0444;
+		charger->attr_name.show = charger_name_show;
+
+		sysfs_attr_init(&charger->attr_state.attr);
+		charger->attr_state.attr.name = "state";
+		charger->attr_state.attr.mode = 0444;
+		charger->attr_state.show = charger_state_show;
+
+		sysfs_attr_init(&charger->attr_externally_control.attr);
+		charger->attr_externally_control.attr.name
+				= "externally_control";
+		charger->attr_externally_control.attr.mode = 0644;
+		charger->attr_externally_control.show
+				= charger_externally_control_show;
+		charger->attr_externally_control.store
+				= charger_externally_control_store;
+
+		if (!desc->charger_regulators[i].externally_control
+				|| !chargers_externally_control) {
+			chargers_externally_control = 0;
+		}
+		dev_info(&pdev->dev, "\'%s\' regulator's externally_control"
+				"is %d\n", charger->regulator_name,
+				charger->externally_control);
+
+		ret = sysfs_create_group(&cm->charger_psy.dev->kobj,
+				&charger->attr_g);
+		if (ret < 0) {
+			dev_info(&pdev->dev, "Cannot create sysfs entry"
+					"of %s regulator\n",
+					charger->regulator_name);
+		}
+	}
+
+	if (chargers_externally_control) {
+		dev_err(&pdev->dev, "Cannot register regulator because "
+				"charger-manager must need at least "
+				"one charger for charging battery\n");
+
+		ret = -EINVAL;
+		goto err_chg_enable;
 	}
 
 	ret = try_charger_enable(cm, true);
@@ -1459,6 +1623,14 @@ static int charger_manager_probe(struct platform_device *pdev)
 	return 0;
 
 err_chg_enable:
+	for (i = 0 ; i < desc->num_charger_regulators ; i++) {
+		struct charger_regulator *charger
+					= &desc->charger_regulators[i];
+		charger = &desc->charger_regulators[i];
+		sysfs_remove_group(&cm->charger_psy.dev->kobj,
+				&charger->attr_g);
+		kfree(charger->attr_g.name);
+	}
 err_extcon:
 	for (i = 0 ; i < desc->num_charger_regulators ; i++) {
 		struct charger_regulator *charger
diff --git a/include/linux/power/charger-manager.h b/include/linux/power/charger-manager.h
index a7b388e..0e86840 100644
--- a/include/linux/power/charger-manager.h
+++ b/include/linux/power/charger-manager.h
@@ -109,24 +109,43 @@ struct charger_cable {
  * struct charger_regulator
  * @regulator_name: the name of regulator for using charger.
  * @consumer: the regulator consumer for the charger.
+ * @externally_control:
+ *	Set if the charger-manager cannot control charger,
+ *	the charger will be maintained with disabled state.
  * @cables:
  *	the array of charger cables to enable/disable charger
  *	and set current limit according to constratint data of
  *	struct charger_cable if only charger cable included
  *	in the array of charger cables is attached/detached.
  * @num_cables: the number of charger cables.
+ * @attr_g: Attribute group for the charger(regulator)
+ * @attr_name: "name" sysfs entry
+ * @attr_state: "state" sysfs entry
+ * @attr_externally_control: "externally_control" sysfs entry
+ * @attrs: Arrays pointing to attr_name/state/externally_control for attr_g
  */
 struct charger_regulator {
 	/* The name of regulator for charging */
 	const char *regulator_name;
 	struct regulator *consumer;
 
+	/* charger never on when system is on */
+	int externally_control;
+
 	/*
 	 * Store constraint information related to current limit,
 	 * each cable have different condition for charging.
 	 */
 	struct charger_cable *cables;
 	int num_cables;
+
+	struct attribute_group attr_g;
+	struct device_attribute attr_name;
+	struct device_attribute attr_state;
+	struct device_attribute attr_externally_control;
+	struct attribute *attrs[4];
+
+	struct charger_manager *cm;
 };
 
 /**
-- 
1.7.0.4


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] charger-manager: Add support sysfs entry for charger
  2012-09-21  9:49 [PATCH v2] charger-manager: Add support sysfs entry for charger Chanwoo Choi
@ 2012-09-21 23:43 ` Anton Vorontsov
  2012-09-22  5:23   ` Kyungmin Park
  0 siblings, 1 reply; 3+ messages in thread
From: Anton Vorontsov @ 2012-09-21 23:43 UTC (permalink / raw)
  To: Chanwoo Choi
  Cc: jenny.tc, ramakrishna.pallala, myungjoo.ham, kyungmin.park,
	linux-kernel

On Fri, Sep 21, 2012 at 06:49:37PM +0900, Chanwoo Choi wrote:
> This patch add support sysfs entry for each charger(regulator).
> Charger-manager use one or more chargers for charging battery but
> some charger isn't necessary on specific scenario. So, if some charger
> isn't needed, can disable specific charger through 'externally_control'
> entry while system is on state and confirm the information(name, state)
> of charger.
[...]
> the list of added sysfs entry
> - /sys/class/power_supply/battery/chargers/charger.[index]/name
> : show name of charger(regulator)
> - /sys/class/power_supply/battery/chargers/charger.[index]/state
> : show either enabled or disabled state of charger
> - /sys/class/power_supply/battery/chargers/charger.[index]/externally_control
> 
> If 'externally_control' of specific charger is 1, Charger-manager cannot enable
> regulator for charging when charger cable is attached and charger must be
> maintained with disabled state. If 'externally_control' is zero, Charger-manager
> usually can control to enable/disable regulator.
> 
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> Signed-off-by: Myungjoo Ham <myungjoo.ham@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---

Thanks for your work, folks!

There are some serious issues wrt. the coding standards, though.

(I must confess that because of the issues I didn't look much into the
logic itself, I hope it works.)

>  drivers/power/charger-manager.c       |  174 ++++++++++++++++++++++++++++++++-
>  include/linux/power/charger-manager.h |   19 ++++
>  2 files changed, 192 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/power/charger-manager.c b/drivers/power/charger-manager.c
> index e92ec55..a1ab825 100644
> --- a/drivers/power/charger-manager.c
> +++ b/drivers/power/charger-manager.c
> @@ -22,6 +22,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/power/charger-manager.h>
>  #include <linux/regulator/consumer.h>
> +#include <linux/sysfs.h>
>  
>  static const char * const default_event_names[] = {
>  	[CM_EVENT_UNKNOWN] = "Unknown",
> @@ -332,6 +333,9 @@ static int try_charger_enable(struct charger_manager *cm, bool enable)
>  		cm->charging_end_time = 0;
>  
>  		for (i = 0 ; i < desc->num_charger_regulators ; i++) {
> +			if (desc->charger_regulators[i].externally_control)
> +				continue;
> +
>  			err = regulator_enable(desc->charger_regulators[i].consumer);
>  			if (err < 0) {
>  				dev_warn(cm->dev,
> @@ -348,6 +352,9 @@ static int try_charger_enable(struct charger_manager *cm, bool enable)
>  		cm->charging_end_time = ktime_to_ms(ktime_get());
>  
>  		for (i = 0 ; i < desc->num_charger_regulators ; i++) {
> +			if (desc->charger_regulators[i].externally_control)
> +				continue;
> +
>  			err = regulator_disable(desc->charger_regulators[i].consumer);
>  			if (err < 0) {
>  				dev_warn(cm->dev,
> @@ -1217,12 +1224,101 @@ static int charger_extcon_init(struct charger_manager *cm,
>  	return ret;
>  }
>  
> +/* help function of sysfs node to control charger(regulator) */
> +static ssize_t charger_name_show(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct charger_regulator *charger
> +		= container_of(attr, struct charger_regulator, attr_name);

Here must be an empty line.

> +	return sprintf(buf, "%s\n", charger->regulator_name);
> +}
> +
> +static ssize_t charger_state_show(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct charger_regulator *charger
> +		= container_of(attr, struct charger_regulator, attr_state);
> +	int state = 1;

= 1 is not needed. Instead, you could rewrite it as

int state = 0;

if (!charger->externally_control)
	state = regulator_is_enabled(charger->consumer);

> +	if (charger->externally_control)
> +		state = 0;
> +	else
> +		state = regulator_is_enabled(charger->consumer);
> +
> +	return sprintf(buf, "%s\n", state ? "enabled" : "disabled");
> +}
> +
> +static ssize_t charger_externally_control_show(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct charger_regulator *charger = container_of(attr,
> +			struct charger_regulator, attr_externally_control);

Empty line is required.

> +	return sprintf(buf, "%d\n", charger->externally_control);
> +}
> +
> +static ssize_t charger_externally_control_store(struct device *dev,
> +				struct device_attribute *attr, const char *buf,
> +				size_t count)
> +{
> +	struct charger_regulator *charger
> +		= container_of(attr, struct charger_regulator,
> +					attr_externally_control);
> +	struct charger_manager *cm = charger->cm;
> +	struct charger_desc *desc = cm->desc;
> +	int i, ret;

One variable declaration per line.

> +	int externally_control;
> +
> +	ret = sscanf(buf, "%d", &externally_control);
> +	if (ret == 0) {
> +		ret = -EINVAL;
> +		return ret;
> +	}
> +
> +	if (externally_control) {

By writing

if (!externally_control) {
	charger->externally_control = externally_control;
	return count;
}

You could greatly reduce indentation level (and readability).

> +		int chargers_externally_control = 1;
> +
> +		for (i = 0 ; i < desc->num_charger_regulators ; i++) {
> +			if ((&desc->charger_regulators[i] != charger)

Parethesis are not needed for the != condition.

> +			&& !desc->charger_regulators[i].externally_control) {

Wrong indentation for 'if' continuation line, and && should have
been on the previous line.

> +				/*
> +				 * At least, one charger is controlled by
> +				 * charger-manager
> +				 */
> +				chargers_externally_control = 0;
> +				break;
> +			}
> +		}
> +
> +		if (!chargers_externally_control) {
> +			if (cm->charger_enabled) {
> +				try_charger_enable(charger->cm, false);
> +				charger->externally_control
> +					= externally_control;
> +				try_charger_enable(charger->cm, true);
> +			} else
> +				charger->externally_control
> +					= externally_control;

Since you use multiline 'if', else must have opening braced as
well, i.e.

} else {
	....
}

> +
> +		} else {
> +			dev_warn(cm->dev,
> +				"\'%s\' regulator should be controlled "

\ is not needed.

> +				"in charger-manager because charger-manager "
> +				"must need at least one charger for charging\n",
> +				charger->regulator_name);
> +		}
> +	} else
> +		charger->externally_control = externally_control;
> +
> +	return count;
> +}
> +
>  static int charger_manager_probe(struct platform_device *pdev)
>  {
>  	struct charger_desc *desc = dev_get_platdata(&pdev->dev);
>  	struct charger_manager *cm;
>  	int ret = 0, i = 0;
>  	int j = 0;
> +	int chargers_externally_control = 1;
>  	union power_supply_propval val;
>  
>  	if (g_desc && !rtc_dev && g_desc->rtc_name) {
> @@ -1412,6 +1508,8 @@ static int charger_manager_probe(struct platform_device *pdev)
>  	for (i = 0 ; i < desc->num_charger_regulators ; i++) {
>  		struct charger_regulator *charger
>  					= &desc->charger_regulators[i];
> +		char buf[11];
> +		char *str;
>  
>  		charger->consumer = regulator_get(&pdev->dev,
>  					charger->regulator_name);
> @@ -1421,8 +1519,9 @@ static int charger_manager_probe(struct platform_device *pdev)
>  			ret = -EINVAL;
>  			goto err_chg_get;
>  		}
> +		charger->cm = cm;
>  
> -		for (j = 0 ; j < charger->num_cables ; j++) {
> +		for (j = 0; j < charger->num_cables; j++) {

While this exact change is technically correct, it does not belong
to this patch.

>  			struct charger_cable *cable = &charger->cables[j];
>  
>  			ret = charger_extcon_init(cm, cable);
> @@ -1434,6 +1533,71 @@ static int charger_manager_probe(struct platform_device *pdev)
>  			cable->charger = charger;
>  			cable->cm = cm;
>  		}
> +
> +		/* Create sysfs entry to control charger(regulator) */
> +		snprintf(buf, 10, "charger.%d", i);
> +		str = kzalloc(sizeof(char) * (strlen(buf) + 1), GFP_KERNEL);
> +		if (!str) {
> +			for (i--; i >= 0; i--) {
> +				charger = &desc->charger_regulators[i];
> +				kfree(charger->attr_g.name);
> +			}
> +			ret = -ENOMEM;
> +
> +			goto err_extcon;
> +		}
> +		strcpy(str, buf);
> +
> +		charger->attrs[0] = &charger->attr_name.attr;
> +		charger->attrs[1] = &charger->attr_state.attr;
> +		charger->attrs[2] = &charger->attr_externally_control.attr;
> +		charger->attrs[3] = NULL;
> +		charger->attr_g.name = str;
> +		charger->attr_g.attrs = charger->attrs;
> +
> +		sysfs_attr_init(&charger->attr_name.attr);
> +		charger->attr_name.attr.name = "name";
> +		charger->attr_name.attr.mode = 0444;
> +		charger->attr_name.show = charger_name_show;
> +
> +		sysfs_attr_init(&charger->attr_state.attr);
> +		charger->attr_state.attr.name = "state";
> +		charger->attr_state.attr.mode = 0444;
> +		charger->attr_state.show = charger_state_show;
> +
> +		sysfs_attr_init(&charger->attr_externally_control.attr);
> +		charger->attr_externally_control.attr.name
> +				= "externally_control";
> +		charger->attr_externally_control.attr.mode = 0644;
> +		charger->attr_externally_control.show
> +				= charger_externally_control_show;
> +		charger->attr_externally_control.store
> +				= charger_externally_control_store;
> +
> +		if (!desc->charger_regulators[i].externally_control
> +				|| !chargers_externally_control) {

wrong || placement (should be on the previous line)

> +			chargers_externally_control = 0;
> +		}
> +		dev_info(&pdev->dev, "\'%s\' regulator's externally_control"

\ is not needed.

> +				"is %d\n", charger->regulator_name,
> +				charger->externally_control);
> +
> +		ret = sysfs_create_group(&cm->charger_psy.dev->kobj,
> +				&charger->attr_g);
> +		if (ret < 0) {
> +			dev_info(&pdev->dev, "Cannot create sysfs entry"
> +					"of %s regulator\n",
> +					charger->regulator_name);
> +		}
> +	}
> +
> +	if (chargers_externally_control) {
> +		dev_err(&pdev->dev, "Cannot register regulator because "
> +				"charger-manager must need at least "
> +				"one charger for charging battery\n");
> +
> +		ret = -EINVAL;
> +		goto err_chg_enable;
>  	}
>  
>  	ret = try_charger_enable(cm, true);
> @@ -1459,6 +1623,14 @@ static int charger_manager_probe(struct platform_device *pdev)
>  	return 0;
>  
>  err_chg_enable:
> +	for (i = 0 ; i < desc->num_charger_regulators ; i++) {

No spaces before ";".

> +		struct charger_regulator *charger
> +					= &desc->charger_regulators[i];

Empty line here.

> +		struct charger_regulator *charger
> +					= &desc->charger_regulators[i];
> +		charger = &desc->charger_regulators[i];

One of the assignments is not needed.

So pretty much every line of this patch breaks coding style rules. I fixed
it all myself and applied the patch (since it's pretty late, and I don't
want you to miss the merge window. But double check the final result, it
is inlined down below)

Also, please carefully (re)read Documentation/CodingStyle, and fully
adhere to these standards for any further patch submissions.

Thanks,
Anton.


- - - -
From: Chanwoo Choi <cw00.choi@samsung.com>
Subject: charger-manager: Add support sysfs entry for charger

This patch add support sysfs entry for each charger(regulator).
Charger-manager use one or more chargers for charging battery but some
charger isn't necessary on specific scenario. So, if some charger isn't
needed, can disable specific charger through 'externally_control' entry
while system is on state and confirm the information(name, state) of
charger.

The list of added sysfs entry
- /sys/class/power_supply/battery/chargers/charger.[index]/name
  show name of charger(regulator)
- /sys/class/power_supply/battery/chargers/charger.[index]/state
  show either enabled or disabled state of charger
- /sys/class/power_supply/battery/chargers/charger.[index]/externally_control

If 'externally_control' of specific charger is 1, Charger-manager cannot
enable regulator for charging when charger cable is attached and charger
must be maintained with disabled state. If 'externally_control' is zero,
Charger-manager usually can control to enable/disable regulator.

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Signed-off-by: Myungjoo Ham <myungjoo.ham@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>
---
 drivers/power/charger-manager.c       | 172 ++++++++++++++++++++++++++++++++++
 include/linux/power/charger-manager.h |  19 ++++
 2 files changed, 191 insertions(+)

diff --git a/drivers/power/charger-manager.c b/drivers/power/charger-manager.c
index e92ec55..92dfa5c 100644
--- a/drivers/power/charger-manager.c
+++ b/drivers/power/charger-manager.c
@@ -22,6 +22,7 @@
 #include <linux/platform_device.h>
 #include <linux/power/charger-manager.h>
 #include <linux/regulator/consumer.h>
+#include <linux/sysfs.h>
 
 static const char * const default_event_names[] = {
 	[CM_EVENT_UNKNOWN] = "Unknown",
@@ -332,6 +333,9 @@ static int try_charger_enable(struct charger_manager *cm, bool enable)
 		cm->charging_end_time = 0;
 
 		for (i = 0 ; i < desc->num_charger_regulators ; i++) {
+			if (desc->charger_regulators[i].externally_control)
+				continue;
+
 			err = regulator_enable(desc->charger_regulators[i].consumer);
 			if (err < 0) {
 				dev_warn(cm->dev,
@@ -348,6 +352,9 @@ static int try_charger_enable(struct charger_manager *cm, bool enable)
 		cm->charging_end_time = ktime_to_ms(ktime_get());
 
 		for (i = 0 ; i < desc->num_charger_regulators ; i++) {
+			if (desc->charger_regulators[i].externally_control)
+				continue;
+
 			err = regulator_disable(desc->charger_regulators[i].consumer);
 			if (err < 0) {
 				dev_warn(cm->dev,
@@ -1217,12 +1224,101 @@ static int charger_extcon_init(struct charger_manager *cm,
 	return ret;
 }
 
+/* help function of sysfs node to control charger(regulator) */
+static ssize_t charger_name_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct charger_regulator *charger
+		= container_of(attr, struct charger_regulator, attr_name);
+
+	return sprintf(buf, "%s\n", charger->regulator_name);
+}
+
+static ssize_t charger_state_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct charger_regulator *charger
+		= container_of(attr, struct charger_regulator, attr_state);
+	int state = 0;
+
+	if (!charger->externally_control)
+		state = regulator_is_enabled(charger->consumer);
+
+	return sprintf(buf, "%s\n", state ? "enabled" : "disabled");
+}
+
+static ssize_t charger_externally_control_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct charger_regulator *charger = container_of(attr,
+			struct charger_regulator, attr_externally_control);
+
+	return sprintf(buf, "%d\n", charger->externally_control);
+}
+
+static ssize_t charger_externally_control_store(struct device *dev,
+				struct device_attribute *attr, const char *buf,
+				size_t count)
+{
+	struct charger_regulator *charger
+		= container_of(attr, struct charger_regulator,
+					attr_externally_control);
+	struct charger_manager *cm = charger->cm;
+	struct charger_desc *desc = cm->desc;
+	int i;
+	int ret;
+	int externally_control;
+	int chargers_externally_control = 1;
+
+	ret = sscanf(buf, "%d", &externally_control);
+	if (ret == 0) {
+		ret = -EINVAL;
+		return ret;
+	}
+
+	if (!externally_control) {
+		charger->externally_control = 0;
+		return count;
+	}
+
+	for (i = 0; i < desc->num_charger_regulators; i++) {
+		if (&desc->charger_regulators[i] != charger &&
+			      !desc->charger_regulators[i].externally_control) {
+			/*
+			 * At least, one charger is controlled by
+			 * charger-manager
+			 */
+			chargers_externally_control = 0;
+			break;
+		}
+	}
+
+	if (!chargers_externally_control) {
+		if (cm->charger_enabled) {
+			try_charger_enable(charger->cm, false);
+			charger->externally_control = externally_control;
+			try_charger_enable(charger->cm, true);
+		} else {
+			charger->externally_control = externally_control;
+		}
+	} else {
+		dev_warn(cm->dev,
+			"'%s' regulator should be controlled "
+			"in charger-manager because charger-manager "
+			"must need at least one charger for charging\n",
+			charger->regulator_name);
+	}
+
+	return count;
+}
+
 static int charger_manager_probe(struct platform_device *pdev)
 {
 	struct charger_desc *desc = dev_get_platdata(&pdev->dev);
 	struct charger_manager *cm;
 	int ret = 0, i = 0;
 	int j = 0;
+	int chargers_externally_control = 1;
 	union power_supply_propval val;
 
 	if (g_desc && !rtc_dev && g_desc->rtc_name) {
@@ -1412,6 +1508,8 @@ static int charger_manager_probe(struct platform_device *pdev)
 	for (i = 0 ; i < desc->num_charger_regulators ; i++) {
 		struct charger_regulator *charger
 					= &desc->charger_regulators[i];
+		char buf[11];
+		char *str;
 
 		charger->consumer = regulator_get(&pdev->dev,
 					charger->regulator_name);
@@ -1421,6 +1519,7 @@ static int charger_manager_probe(struct platform_device *pdev)
 			ret = -EINVAL;
 			goto err_chg_get;
 		}
+		charger->cm = cm;
 
 		for (j = 0 ; j < charger->num_cables ; j++) {
 			struct charger_cable *cable = &charger->cables[j];
@@ -1434,6 +1533,71 @@ static int charger_manager_probe(struct platform_device *pdev)
 			cable->charger = charger;
 			cable->cm = cm;
 		}
+
+		/* Create sysfs entry to control charger(regulator) */
+		snprintf(buf, 10, "charger.%d", i);
+		str = kzalloc(sizeof(char) * (strlen(buf) + 1), GFP_KERNEL);
+		if (!str) {
+			for (i--; i >= 0; i--) {
+				charger = &desc->charger_regulators[i];
+				kfree(charger->attr_g.name);
+			}
+			ret = -ENOMEM;
+
+			goto err_extcon;
+		}
+		strcpy(str, buf);
+
+		charger->attrs[0] = &charger->attr_name.attr;
+		charger->attrs[1] = &charger->attr_state.attr;
+		charger->attrs[2] = &charger->attr_externally_control.attr;
+		charger->attrs[3] = NULL;
+		charger->attr_g.name = str;
+		charger->attr_g.attrs = charger->attrs;
+
+		sysfs_attr_init(&charger->attr_name.attr);
+		charger->attr_name.attr.name = "name";
+		charger->attr_name.attr.mode = 0444;
+		charger->attr_name.show = charger_name_show;
+
+		sysfs_attr_init(&charger->attr_state.attr);
+		charger->attr_state.attr.name = "state";
+		charger->attr_state.attr.mode = 0444;
+		charger->attr_state.show = charger_state_show;
+
+		sysfs_attr_init(&charger->attr_externally_control.attr);
+		charger->attr_externally_control.attr.name
+				= "externally_control";
+		charger->attr_externally_control.attr.mode = 0644;
+		charger->attr_externally_control.show
+				= charger_externally_control_show;
+		charger->attr_externally_control.store
+				= charger_externally_control_store;
+
+		if (!desc->charger_regulators[i].externally_control ||
+				!chargers_externally_control) {
+			chargers_externally_control = 0;
+		}
+		dev_info(&pdev->dev, "'%s' regulator's externally_control"
+				"is %d\n", charger->regulator_name,
+				charger->externally_control);
+
+		ret = sysfs_create_group(&cm->charger_psy.dev->kobj,
+				&charger->attr_g);
+		if (ret < 0) {
+			dev_info(&pdev->dev, "Cannot create sysfs entry"
+					"of %s regulator\n",
+					charger->regulator_name);
+		}
+	}
+
+	if (chargers_externally_control) {
+		dev_err(&pdev->dev, "Cannot register regulator because "
+				"charger-manager must need at least "
+				"one charger for charging battery\n");
+
+		ret = -EINVAL;
+		goto err_chg_enable;
 	}
 
 	ret = try_charger_enable(cm, true);
@@ -1459,6 +1623,14 @@ static int charger_manager_probe(struct platform_device *pdev)
 	return 0;
 
 err_chg_enable:
+	for (i = 0; i < desc->num_charger_regulators; i++) {
+		struct charger_regulator *charger;
+
+		charger = &desc->charger_regulators[i];
+		sysfs_remove_group(&cm->charger_psy.dev->kobj,
+				&charger->attr_g);
+		kfree(charger->attr_g.name);
+	}
 err_extcon:
 	for (i = 0 ; i < desc->num_charger_regulators ; i++) {
 		struct charger_regulator *charger
diff --git a/include/linux/power/charger-manager.h b/include/linux/power/charger-manager.h
index a7b388e..0e86840 100644
--- a/include/linux/power/charger-manager.h
+++ b/include/linux/power/charger-manager.h
@@ -109,24 +109,43 @@ struct charger_cable {
  * struct charger_regulator
  * @regulator_name: the name of regulator for using charger.
  * @consumer: the regulator consumer for the charger.
+ * @externally_control:
+ *	Set if the charger-manager cannot control charger,
+ *	the charger will be maintained with disabled state.
  * @cables:
  *	the array of charger cables to enable/disable charger
  *	and set current limit according to constratint data of
  *	struct charger_cable if only charger cable included
  *	in the array of charger cables is attached/detached.
  * @num_cables: the number of charger cables.
+ * @attr_g: Attribute group for the charger(regulator)
+ * @attr_name: "name" sysfs entry
+ * @attr_state: "state" sysfs entry
+ * @attr_externally_control: "externally_control" sysfs entry
+ * @attrs: Arrays pointing to attr_name/state/externally_control for attr_g
  */
 struct charger_regulator {
 	/* The name of regulator for charging */
 	const char *regulator_name;
 	struct regulator *consumer;
 
+	/* charger never on when system is on */
+	int externally_control;
+
 	/*
 	 * Store constraint information related to current limit,
 	 * each cable have different condition for charging.
 	 */
 	struct charger_cable *cables;
 	int num_cables;
+
+	struct attribute_group attr_g;
+	struct device_attribute attr_name;
+	struct device_attribute attr_state;
+	struct device_attribute attr_externally_control;
+	struct attribute *attrs[4];
+
+	struct charger_manager *cm;
 };
 
 /**
-- 
1.7.11.5


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] charger-manager: Add support sysfs entry for charger
  2012-09-21 23:43 ` Anton Vorontsov
@ 2012-09-22  5:23   ` Kyungmin Park
  0 siblings, 0 replies; 3+ messages in thread
From: Kyungmin Park @ 2012-09-22  5:23 UTC (permalink / raw)
  To: Anton Vorontsov
  Cc: Chanwoo Choi, jenny.tc, ramakrishna.pallala, myungjoo.ham,
	linux-kernel

On 9/22/12, Anton Vorontsov <anton.vorontsov@linaro.org> wrote:
> On Fri, Sep 21, 2012 at 06:49:37PM +0900, Chanwoo Choi wrote:
>> This patch add support sysfs entry for each charger(regulator).
>> Charger-manager use one or more chargers for charging battery but
>> some charger isn't necessary on specific scenario. So, if some charger
>> isn't needed, can disable specific charger through 'externally_control'
>> entry while system is on state and confirm the information(name, state)
>> of charger.
> [...]
>> the list of added sysfs entry
>> - /sys/class/power_supply/battery/chargers/charger.[index]/name
>> : show name of charger(regulator)
>> - /sys/class/power_supply/battery/chargers/charger.[index]/state
>> : show either enabled or disabled state of charger
>> -
>> /sys/class/power_supply/battery/chargers/charger.[index]/externally_control
>>
>> If 'externally_control' of specific charger is 1, Charger-manager cannot
>> enable
>> regulator for charging when charger cable is attached and charger must be
>> maintained with disabled state. If 'externally_control' is zero,
>> Charger-manager
>> usually can control to enable/disable regulator.
>>
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>> Signed-off-by: Myungjoo Ham <myungjoo.ham@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>
> Thanks for your work, folks!
>
> There are some serious issues wrt. the coding standards, though.
>
> (I must confess that because of the issues I didn't look much into the
> logic itself, I hope it works.)
>
>>  drivers/power/charger-manager.c       |  174
>> ++++++++++++++++++++++++++++++++-
>>  include/linux/power/charger-manager.h |   19 ++++
>>  2 files changed, 192 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/power/charger-manager.c
>> b/drivers/power/charger-manager.c
>> index e92ec55..a1ab825 100644
>> --- a/drivers/power/charger-manager.c
>> +++ b/drivers/power/charger-manager.c
>> @@ -22,6 +22,7 @@
>>  #include <linux/platform_device.h>
>>  #include <linux/power/charger-manager.h>
>>  #include <linux/regulator/consumer.h>
>> +#include <linux/sysfs.h>
>>
>>  static const char * const default_event_names[] = {
>>  	[CM_EVENT_UNKNOWN] = "Unknown",
>> @@ -332,6 +333,9 @@ static int try_charger_enable(struct charger_manager
>> *cm, bool enable)
>>  		cm->charging_end_time = 0;
>>
>>  		for (i = 0 ; i < desc->num_charger_regulators ; i++) {
>> +			if (desc->charger_regulators[i].externally_control)
>> +				continue;
>> +
>>  			err = regulator_enable(desc->charger_regulators[i].consumer);
>>  			if (err < 0) {
>>  				dev_warn(cm->dev,
>> @@ -348,6 +352,9 @@ static int try_charger_enable(struct charger_manager
>> *cm, bool enable)
>>  		cm->charging_end_time = ktime_to_ms(ktime_get());
>>
>>  		for (i = 0 ; i < desc->num_charger_regulators ; i++) {
>> +			if (desc->charger_regulators[i].externally_control)
>> +				continue;
>> +
>>  			err = regulator_disable(desc->charger_regulators[i].consumer);
>>  			if (err < 0) {
>>  				dev_warn(cm->dev,
>> @@ -1217,12 +1224,101 @@ static int charger_extcon_init(struct
>> charger_manager *cm,
>>  	return ret;
>>  }
>>
>> +/* help function of sysfs node to control charger(regulator) */
>> +static ssize_t charger_name_show(struct device *dev,
>> +				struct device_attribute *attr, char *buf)
>> +{
>> +	struct charger_regulator *charger
>> +		= container_of(attr, struct charger_regulator, attr_name);
>
> Here must be an empty line.
>
>> +	return sprintf(buf, "%s\n", charger->regulator_name);
>> +}
>> +
>> +static ssize_t charger_state_show(struct device *dev,
>> +				struct device_attribute *attr, char *buf)
>> +{
>> +	struct charger_regulator *charger
>> +		= container_of(attr, struct charger_regulator, attr_state);
>> +	int state = 1;
>
> = 1 is not needed. Instead, you could rewrite it as
>
> int state = 0;
>
> if (!charger->externally_control)
> 	state = regulator_is_enabled(charger->consumer);
>
>> +	if (charger->externally_control)
>> +		state = 0;
>> +	else
>> +		state = regulator_is_enabled(charger->consumer);
>> +
>> +	return sprintf(buf, "%s\n", state ? "enabled" : "disabled");
>> +}
>> +
>> +static ssize_t charger_externally_control_show(struct device *dev,
>> +				struct device_attribute *attr, char *buf)
>> +{
>> +	struct charger_regulator *charger = container_of(attr,
>> +			struct charger_regulator, attr_externally_control);
>
> Empty line is required.
>
>> +	return sprintf(buf, "%d\n", charger->externally_control);
>> +}
>> +
>> +static ssize_t charger_externally_control_store(struct device *dev,
>> +				struct device_attribute *attr, const char *buf,
>> +				size_t count)
>> +{
>> +	struct charger_regulator *charger
>> +		= container_of(attr, struct charger_regulator,
>> +					attr_externally_control);
>> +	struct charger_manager *cm = charger->cm;
>> +	struct charger_desc *desc = cm->desc;
>> +	int i, ret;
>
> One variable declaration per line.
>
>> +	int externally_control;
>> +
>> +	ret = sscanf(buf, "%d", &externally_control);
>> +	if (ret == 0) {
>> +		ret = -EINVAL;
>> +		return ret;
>> +	}
>> +
>> +	if (externally_control) {
>
> By writing
>
> if (!externally_control) {
> 	charger->externally_control = externally_control;
> 	return count;
> }
>
> You could greatly reduce indentation level (and readability).
>
>> +		int chargers_externally_control = 1;
>> +
>> +		for (i = 0 ; i < desc->num_charger_regulators ; i++) {
>> +			if ((&desc->charger_regulators[i] != charger)
>
> Parethesis are not needed for the != condition.
>
>> +			&& !desc->charger_regulators[i].externally_control) {
>
> Wrong indentation for 'if' continuation line, and && should have
> been on the previous line.
>
>> +				/*
>> +				 * At least, one charger is controlled by
>> +				 * charger-manager
>> +				 */
>> +				chargers_externally_control = 0;
>> +				break;
>> +			}
>> +		}
>> +
>> +		if (!chargers_externally_control) {
>> +			if (cm->charger_enabled) {
>> +				try_charger_enable(charger->cm, false);
>> +				charger->externally_control
>> +					= externally_control;
>> +				try_charger_enable(charger->cm, true);
>> +			} else
>> +				charger->externally_control
>> +					= externally_control;
>
> Since you use multiline 'if', else must have opening braced as
> well, i.e.
>
> } else {
> 	....
> }
>
>> +
>> +		} else {
>> +			dev_warn(cm->dev,
>> +				"\'%s\' regulator should be controlled "
>
> \ is not needed.
>
>> +				"in charger-manager because charger-manager "
>> +				"must need at least one charger for charging\n",
>> +				charger->regulator_name);
>> +		}
>> +	} else
>> +		charger->externally_control = externally_control;
>> +
>> +	return count;
>> +}
>> +
>>  static int charger_manager_probe(struct platform_device *pdev)
>>  {
>>  	struct charger_desc *desc = dev_get_platdata(&pdev->dev);
>>  	struct charger_manager *cm;
>>  	int ret = 0, i = 0;
>>  	int j = 0;
>> +	int chargers_externally_control = 1;
>>  	union power_supply_propval val;
>>
>>  	if (g_desc && !rtc_dev && g_desc->rtc_name) {
>> @@ -1412,6 +1508,8 @@ static int charger_manager_probe(struct
>> platform_device *pdev)
>>  	for (i = 0 ; i < desc->num_charger_regulators ; i++) {
>>  		struct charger_regulator *charger
>>  					= &desc->charger_regulators[i];
>> +		char buf[11];
>> +		char *str;
>>
>>  		charger->consumer = regulator_get(&pdev->dev,
>>  					charger->regulator_name);
>> @@ -1421,8 +1519,9 @@ static int charger_manager_probe(struct
>> platform_device *pdev)
>>  			ret = -EINVAL;
>>  			goto err_chg_get;
>>  		}
>> +		charger->cm = cm;
>>
>> -		for (j = 0 ; j < charger->num_cables ; j++) {
>> +		for (j = 0; j < charger->num_cables; j++) {
>
> While this exact change is technically correct, it does not belong
> to this patch.
>
>>  			struct charger_cable *cable = &charger->cables[j];
>>
>>  			ret = charger_extcon_init(cm, cable);
>> @@ -1434,6 +1533,71 @@ static int charger_manager_probe(struct
>> platform_device *pdev)
>>  			cable->charger = charger;
>>  			cable->cm = cm;
>>  		}
>> +
>> +		/* Create sysfs entry to control charger(regulator) */
>> +		snprintf(buf, 10, "charger.%d", i);
>> +		str = kzalloc(sizeof(char) * (strlen(buf) + 1), GFP_KERNEL);
>> +		if (!str) {
>> +			for (i--; i >= 0; i--) {
>> +				charger = &desc->charger_regulators[i];
>> +				kfree(charger->attr_g.name);
>> +			}
>> +			ret = -ENOMEM;
>> +
>> +			goto err_extcon;
>> +		}
>> +		strcpy(str, buf);
>> +
>> +		charger->attrs[0] = &charger->attr_name.attr;
>> +		charger->attrs[1] = &charger->attr_state.attr;
>> +		charger->attrs[2] = &charger->attr_externally_control.attr;
>> +		charger->attrs[3] = NULL;
>> +		charger->attr_g.name = str;
>> +		charger->attr_g.attrs = charger->attrs;
>> +
>> +		sysfs_attr_init(&charger->attr_name.attr);
>> +		charger->attr_name.attr.name = "name";
>> +		charger->attr_name.attr.mode = 0444;
>> +		charger->attr_name.show = charger_name_show;
>> +
>> +		sysfs_attr_init(&charger->attr_state.attr);
>> +		charger->attr_state.attr.name = "state";
>> +		charger->attr_state.attr.mode = 0444;
>> +		charger->attr_state.show = charger_state_show;
>> +
>> +		sysfs_attr_init(&charger->attr_externally_control.attr);
>> +		charger->attr_externally_control.attr.name
>> +				= "externally_control";
>> +		charger->attr_externally_control.attr.mode = 0644;
>> +		charger->attr_externally_control.show
>> +				= charger_externally_control_show;
>> +		charger->attr_externally_control.store
>> +				= charger_externally_control_store;
>> +
>> +		if (!desc->charger_regulators[i].externally_control
>> +				|| !chargers_externally_control) {
>
> wrong || placement (should be on the previous line)
>
>> +			chargers_externally_control = 0;
>> +		}
>> +		dev_info(&pdev->dev, "\'%s\' regulator's externally_control"
>
> \ is not needed.
>
>> +				"is %d\n", charger->regulator_name,
>> +				charger->externally_control);
>> +
>> +		ret = sysfs_create_group(&cm->charger_psy.dev->kobj,
>> +				&charger->attr_g);
>> +		if (ret < 0) {
>> +			dev_info(&pdev->dev, "Cannot create sysfs entry"
>> +					"of %s regulator\n",
>> +					charger->regulator_name);
>> +		}
>> +	}
>> +
>> +	if (chargers_externally_control) {
>> +		dev_err(&pdev->dev, "Cannot register regulator because "
>> +				"charger-manager must need at least "
>> +				"one charger for charging battery\n");
>> +
>> +		ret = -EINVAL;
>> +		goto err_chg_enable;
>>  	}
>>
>>  	ret = try_charger_enable(cm, true);
>> @@ -1459,6 +1623,14 @@ static int charger_manager_probe(struct
>> platform_device *pdev)
>>  	return 0;
>>
>>  err_chg_enable:
>> +	for (i = 0 ; i < desc->num_charger_regulators ; i++) {
>
> No spaces before ";".
>
>> +		struct charger_regulator *charger
>> +					= &desc->charger_regulators[i];
>
> Empty line here.
>
>> +		struct charger_regulator *charger
>> +					= &desc->charger_regulators[i];
>> +		charger = &desc->charger_regulators[i];
>
> One of the assignments is not needed.
>
> So pretty much every line of this patch breaks coding style rules. I fixed
> it all myself and applied the patch (since it's pretty late, and I don't
> want you to miss the merge window. But double check the final result, it
> is inlined down below)
>
> Also, please carefully (re)read Documentation/CodingStyle, and fully
> adhere to these standards for any further patch submissions.
>
> Thanks,
> Anton.
>
>
> - - - -
> From: Chanwoo Choi <cw00.choi@samsung.com>
> Subject: charger-manager: Add support sysfs entry for charger
>
> This patch add support sysfs entry for each charger(regulator).
> Charger-manager use one or more chargers for charging battery but some
> charger isn't necessary on specific scenario. So, if some charger isn't
> needed, can disable specific charger through 'externally_control' entry
> while system is on state and confirm the information(name, state) of
> charger.
>
> The list of added sysfs entry
> - /sys/class/power_supply/battery/chargers/charger.[index]/name
>   show name of charger(regulator)
> - /sys/class/power_supply/battery/chargers/charger.[index]/state
>   show either enabled or disabled state of charger
> -
> /sys/class/power_supply/battery/chargers/charger.[index]/externally_control
>
> If 'externally_control' of specific charger is 1, Charger-manager cannot
> enable regulator for charging when charger cable is attached and charger
> must be maintained with disabled state. If 'externally_control' is zero,
> Charger-manager usually can control to enable/disable regulator.
>
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> Signed-off-by: Myungjoo Ham <myungjoo.ham@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>
Thanks Antron.
> ---
>  drivers/power/charger-manager.c       | 172
> ++++++++++++++++++++++++++++++++++
>  include/linux/power/charger-manager.h |  19 ++++
>  2 files changed, 191 insertions(+)
>
> diff --git a/drivers/power/charger-manager.c
> b/drivers/power/charger-manager.c
> index e92ec55..92dfa5c 100644
> --- a/drivers/power/charger-manager.c
> +++ b/drivers/power/charger-manager.c
> @@ -22,6 +22,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/power/charger-manager.h>
>  #include <linux/regulator/consumer.h>
> +#include <linux/sysfs.h>
>
>  static const char * const default_event_names[] = {
>  	[CM_EVENT_UNKNOWN] = "Unknown",
> @@ -332,6 +333,9 @@ static int try_charger_enable(struct charger_manager
> *cm, bool enable)
>  		cm->charging_end_time = 0;
>
>  		for (i = 0 ; i < desc->num_charger_regulators ; i++) {
> +			if (desc->charger_regulators[i].externally_control)
> +				continue;
> +
>  			err = regulator_enable(desc->charger_regulators[i].consumer);
>  			if (err < 0) {
>  				dev_warn(cm->dev,
> @@ -348,6 +352,9 @@ static int try_charger_enable(struct charger_manager
> *cm, bool enable)
>  		cm->charging_end_time = ktime_to_ms(ktime_get());
>
>  		for (i = 0 ; i < desc->num_charger_regulators ; i++) {
> +			if (desc->charger_regulators[i].externally_control)
> +				continue;
> +
>  			err = regulator_disable(desc->charger_regulators[i].consumer);
>  			if (err < 0) {
>  				dev_warn(cm->dev,
> @@ -1217,12 +1224,101 @@ static int charger_extcon_init(struct
> charger_manager *cm,
>  	return ret;
>  }
>
> +/* help function of sysfs node to control charger(regulator) */
> +static ssize_t charger_name_show(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct charger_regulator *charger
> +		= container_of(attr, struct charger_regulator, attr_name);
> +
> +	return sprintf(buf, "%s\n", charger->regulator_name);
> +}
> +
> +static ssize_t charger_state_show(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct charger_regulator *charger
> +		= container_of(attr, struct charger_regulator, attr_state);
> +	int state = 0;
> +
> +	if (!charger->externally_control)
> +		state = regulator_is_enabled(charger->consumer);
> +
> +	return sprintf(buf, "%s\n", state ? "enabled" : "disabled");
> +}
> +
> +static ssize_t charger_externally_control_show(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct charger_regulator *charger = container_of(attr,
> +			struct charger_regulator, attr_externally_control);
> +
> +	return sprintf(buf, "%d\n", charger->externally_control);
> +}
> +
> +static ssize_t charger_externally_control_store(struct device *dev,
> +				struct device_attribute *attr, const char *buf,
> +				size_t count)
> +{
> +	struct charger_regulator *charger
> +		= container_of(attr, struct charger_regulator,
> +					attr_externally_control);
> +	struct charger_manager *cm = charger->cm;
> +	struct charger_desc *desc = cm->desc;
> +	int i;
> +	int ret;
> +	int externally_control;
> +	int chargers_externally_control = 1;
> +
> +	ret = sscanf(buf, "%d", &externally_control);
> +	if (ret == 0) {
> +		ret = -EINVAL;
> +		return ret;
> +	}
> +
> +	if (!externally_control) {
> +		charger->externally_control = 0;
> +		return count;
> +	}
> +
> +	for (i = 0; i < desc->num_charger_regulators; i++) {
> +		if (&desc->charger_regulators[i] != charger &&
> +			      !desc->charger_regulators[i].externally_control) {
> +			/*
> +			 * At least, one charger is controlled by
> +			 * charger-manager
> +			 */
> +			chargers_externally_control = 0;
> +			break;
> +		}
> +	}
> +
> +	if (!chargers_externally_control) {
> +		if (cm->charger_enabled) {
> +			try_charger_enable(charger->cm, false);
> +			charger->externally_control = externally_control;
> +			try_charger_enable(charger->cm, true);
> +		} else {
> +			charger->externally_control = externally_control;
> +		}
> +	} else {
> +		dev_warn(cm->dev,
> +			"'%s' regulator should be controlled "
> +			"in charger-manager because charger-manager "
> +			"must need at least one charger for charging\n",
> +			charger->regulator_name);
> +	}
> +
> +	return count;
> +}
> +
>  static int charger_manager_probe(struct platform_device *pdev)
>  {
>  	struct charger_desc *desc = dev_get_platdata(&pdev->dev);
>  	struct charger_manager *cm;
>  	int ret = 0, i = 0;
>  	int j = 0;
> +	int chargers_externally_control = 1;
>  	union power_supply_propval val;
>
>  	if (g_desc && !rtc_dev && g_desc->rtc_name) {
> @@ -1412,6 +1508,8 @@ static int charger_manager_probe(struct
> platform_device *pdev)
>  	for (i = 0 ; i < desc->num_charger_regulators ; i++) {
>  		struct charger_regulator *charger
>  					= &desc->charger_regulators[i];
> +		char buf[11];
> +		char *str;
>
>  		charger->consumer = regulator_get(&pdev->dev,
>  					charger->regulator_name);
> @@ -1421,6 +1519,7 @@ static int charger_manager_probe(struct
> platform_device *pdev)
>  			ret = -EINVAL;
>  			goto err_chg_get;
>  		}
> +		charger->cm = cm;
>
>  		for (j = 0 ; j < charger->num_cables ; j++) {
>  			struct charger_cable *cable = &charger->cables[j];
> @@ -1434,6 +1533,71 @@ static int charger_manager_probe(struct
> platform_device *pdev)
>  			cable->charger = charger;
>  			cable->cm = cm;
>  		}
> +
> +		/* Create sysfs entry to control charger(regulator) */
> +		snprintf(buf, 10, "charger.%d", i);
> +		str = kzalloc(sizeof(char) * (strlen(buf) + 1), GFP_KERNEL);
> +		if (!str) {
> +			for (i--; i >= 0; i--) {
> +				charger = &desc->charger_regulators[i];
> +				kfree(charger->attr_g.name);
> +			}
> +			ret = -ENOMEM;
> +
> +			goto err_extcon;
> +		}
> +		strcpy(str, buf);
> +
> +		charger->attrs[0] = &charger->attr_name.attr;
> +		charger->attrs[1] = &charger->attr_state.attr;
> +		charger->attrs[2] = &charger->attr_externally_control.attr;
> +		charger->attrs[3] = NULL;
> +		charger->attr_g.name = str;
> +		charger->attr_g.attrs = charger->attrs;
> +
> +		sysfs_attr_init(&charger->attr_name.attr);
> +		charger->attr_name.attr.name = "name";
> +		charger->attr_name.attr.mode = 0444;
> +		charger->attr_name.show = charger_name_show;
> +
> +		sysfs_attr_init(&charger->attr_state.attr);
> +		charger->attr_state.attr.name = "state";
> +		charger->attr_state.attr.mode = 0444;
> +		charger->attr_state.show = charger_state_show;
> +
> +		sysfs_attr_init(&charger->attr_externally_control.attr);
> +		charger->attr_externally_control.attr.name
> +				= "externally_control";
> +		charger->attr_externally_control.attr.mode = 0644;
> +		charger->attr_externally_control.show
> +				= charger_externally_control_show;
> +		charger->attr_externally_control.store
> +				= charger_externally_control_store;
> +
> +		if (!desc->charger_regulators[i].externally_control ||
> +				!chargers_externally_control) {
> +			chargers_externally_control = 0;
> +		}
> +		dev_info(&pdev->dev, "'%s' regulator's externally_control"
> +				"is %d\n", charger->regulator_name,
> +				charger->externally_control);
> +
> +		ret = sysfs_create_group(&cm->charger_psy.dev->kobj,
> +				&charger->attr_g);
> +		if (ret < 0) {
> +			dev_info(&pdev->dev, "Cannot create sysfs entry"
> +					"of %s regulator\n",
> +					charger->regulator_name);
> +		}
> +	}
> +
> +	if (chargers_externally_control) {
> +		dev_err(&pdev->dev, "Cannot register regulator because "
> +				"charger-manager must need at least "
> +				"one charger for charging battery\n");
> +
> +		ret = -EINVAL;
> +		goto err_chg_enable;
>  	}
>
>  	ret = try_charger_enable(cm, true);
> @@ -1459,6 +1623,14 @@ static int charger_manager_probe(struct
> platform_device *pdev)
>  	return 0;
>
>  err_chg_enable:
> +	for (i = 0; i < desc->num_charger_regulators; i++) {
> +		struct charger_regulator *charger;
> +
> +		charger = &desc->charger_regulators[i];
> +		sysfs_remove_group(&cm->charger_psy.dev->kobj,
> +				&charger->attr_g);
> +		kfree(charger->attr_g.name);
> +	}
>  err_extcon:
>  	for (i = 0 ; i < desc->num_charger_regulators ; i++) {
>  		struct charger_regulator *charger
> diff --git a/include/linux/power/charger-manager.h
> b/include/linux/power/charger-manager.h
> index a7b388e..0e86840 100644
> --- a/include/linux/power/charger-manager.h
> +++ b/include/linux/power/charger-manager.h
> @@ -109,24 +109,43 @@ struct charger_cable {
>   * struct charger_regulator
>   * @regulator_name: the name of regulator for using charger.
>   * @consumer: the regulator consumer for the charger.
> + * @externally_control:
> + *	Set if the charger-manager cannot control charger,
> + *	the charger will be maintained with disabled state.
>   * @cables:
>   *	the array of charger cables to enable/disable charger
>   *	and set current limit according to constratint data of
>   *	struct charger_cable if only charger cable included
>   *	in the array of charger cables is attached/detached.
>   * @num_cables: the number of charger cables.
> + * @attr_g: Attribute group for the charger(regulator)
> + * @attr_name: "name" sysfs entry
> + * @attr_state: "state" sysfs entry
> + * @attr_externally_control: "externally_control" sysfs entry
> + * @attrs: Arrays pointing to attr_name/state/externally_control for
> attr_g
>   */
>  struct charger_regulator {
>  	/* The name of regulator for charging */
>  	const char *regulator_name;
>  	struct regulator *consumer;
>
> +	/* charger never on when system is on */
> +	int externally_control;
> +
>  	/*
>  	 * Store constraint information related to current limit,
>  	 * each cable have different condition for charging.
>  	 */
>  	struct charger_cable *cables;
>  	int num_cables;
> +
> +	struct attribute_group attr_g;
> +	struct device_attribute attr_name;
> +	struct device_attribute attr_state;
> +	struct device_attribute attr_externally_control;
> +	struct attribute *attrs[4];
> +
> +	struct charger_manager *cm;
>  };
>
>  /**
> --
> 1.7.11.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2012-09-22  5:23 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-09-21  9:49 [PATCH v2] charger-manager: Add support sysfs entry for charger Chanwoo Choi
2012-09-21 23:43 ` Anton Vorontsov
2012-09-22  5:23   ` Kyungmin Park

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox