[PATCH v3 0/2] power: act8945a: add charger driver for the sub-device of ACT8945A MFD

[PATCH v3 0/2] power: act8945a: add charger driver for the sub-device of ACT8945A MFD

[Wenyou Yang]

The ACT8945A is a Multi Function Device with the following subdevices:
 - Regulator
 - Charger

This patch set is to add regulator driver for ACT8945A.
It is based on the patch set:
	[PATCH v2] mfd: act8945a: add Active-semi ACT8945A PMIC MFD driver

Changes in v3:
 - update the file header with short version license and author line.
 - remove unused member of struct act8945a_charger, dev.
 - action due to removing the member of stuct act8945a_dev, dev.
 - remove the unnecessary print out.
 - remove the unnecessary act8945a_charger_remove().
 - fix align of the code-style.

Changes in v2:
 1./ Substitute of_property_read_bool() for of_get_property().
 2./ Substitute devm_power_supply_register() for power_supply_register().
 3./ Use module_platform_driver(), instead of subsys_initcall().
 4./ Substitute MODULE_LICENSE("GPL") for MODULE_LICENSE("GPL v2").

Wenyou Yang (2):
  power: act8945a: add charger driver for ACT8945A
  power: add documentation for ACT8945A's charger DT bindings

 .../devicetree/bindings/power/act8945a-charger.txt |   34 ++
 drivers/power/Kconfig                              |    7 +
 drivers/power/Makefile                             |    1 +
 drivers/power/act8945a_charger.c                   |  367 ++++++++++++++++++++
 4 files changed, 409 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/act8945a-charger.txt
 create mode 100644 drivers/power/act8945a_charger.c

-- 
1.7.9.5

[PATCH v3 1/2] power: act8945a: add charger driver for ACT8945A

[Wenyou Yang]

This patch adds new driver for Active-semi ACT8945A ActivePath
charger (part of ACT8945A MFD driver) providing power supply class
information to userspace.

The driver is configured through DTS (battery and system related
settings) and sysfs entries (timers and input over-voltage threshold).

Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
---

Changes in v3:
 - update the file header with short version license and author line.
 - remove unused member of struct act8945a_charger, dev.
 - action due to removing the member of stuct act8945a_dev, dev.
 - remove the unnecessary print out.
 - remove the unnecessary act8945a_charger_remove().
 - fix align of the code-style.

Changes in v2:
 1./ Substitute of_property_read_bool() for of_get_property().
 2./ Substitute devm_power_supply_register() for power_supply_register().
 3./ Use module_platform_driver(), instead of subsys_initcall().
 4./ Substitute MODULE_LICENSE("GPL") for MODULE_LICENSE("GPL v2").

 drivers/power/Kconfig            |    7 +
 drivers/power/Makefile           |    1 +
 drivers/power/act8945a_charger.c |  367 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 375 insertions(+)
 create mode 100644 drivers/power/act8945a_charger.c

diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index 1ddd13c..ae75211 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -75,6 +75,13 @@ config BATTERY_88PM860X
 	help
 	  Say Y here to enable battery monitor for Marvell 88PM860x chip.
 
+config BATTERY_ACT8945A
+	tristate "Active-semi ACT8945A charger driver"
+	depends on MFD_ACT8945A
+	help
+	  Say Y here to enable support for power supply provided by
+	  Active-semi ActivePath ACT8945A charger.
+
 config BATTERY_DS2760
 	tristate "DS2760 battery driver (HP iPAQ & others)"
 	depends on W1 && W1_SLAVE_DS2760
diff --git a/drivers/power/Makefile b/drivers/power/Makefile
index 0e4eab5..e46b75d 100644
--- a/drivers/power/Makefile
+++ b/drivers/power/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_WM8350_POWER)	+= wm8350_power.o
 obj-$(CONFIG_TEST_POWER)	+= test_power.o
 
 obj-$(CONFIG_BATTERY_88PM860X)	+= 88pm860x_battery.o
+obj-$(CONFIG_BATTERY_ACT8945A)	+= act8945a_charger.o
 obj-$(CONFIG_BATTERY_DS2760)	+= ds2760_battery.o
 obj-$(CONFIG_BATTERY_DS2780)	+= ds2780_battery.o
 obj-$(CONFIG_BATTERY_DS2781)	+= ds2781_battery.o
diff --git a/drivers/power/act8945a_charger.c b/drivers/power/act8945a_charger.c
new file mode 100644
index 0000000..643a2ed
--- /dev/null
+++ b/drivers/power/act8945a_charger.c
@@ -0,0 +1,367 @@
+/*
+ * Power supply driver for the Active-semi ACT8945A PMIC
+ *
+ * Copyright (C) 2015 Atmel Corporation
+ *
+ * Author: Wenyou Yang <wenyou.yang@atmel.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+#include <linux/module.h>
+#include <linux/mfd/act8945a.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+#include <linux/power_supply.h>
+#include <linux/regmap.h>
+
+static const char *act8945a_charger_model = "ACT8945A";
+static const char *act8945a_charger_manufacturer = "Active-semi";
+
+/**
+ * ACT8945A Charger Register Map
+ */
+
+/* 0x70: Reserved */
+#define ACT8945A_APCH_CFG		0x71
+#define ACT8945A_APCH_STATUS		0x78
+#define ACT8945A_APCH_CTRL		0x79
+#define ACT8945A_APCH_STATE		0x7A
+
+/* ACT8945A_APCH_CFG */
+#define APCH_CFG_OVPSET		(0x03 << 0)
+#define		APCH_CFG_OVPSET_6V6	(0x0 << 0)
+#define		APCH_CFG_OVPSET_7V	(0x1 << 0)
+#define		APCH_CFG_OVPSET_7V5	(0x2 << 0)
+#define		APCH_CFG_OVPSET_8V	(0x3 << 0)
+#define APCH_CFG_PRETIMO	(0x03 << 2)
+#define		APCH_CFG_PRETIMO_40_MIN		(0x0 << 2)
+#define		APCH_CFG_PRETIMO_60_MIN		(0x1 << 2)
+#define		APCH_CFG_PRETIMO_80_MIN		(0x2 << 2)
+#define		APCH_CFG_PRETIMO_DISABLED	(0x3 << 2)
+#define APCH_CFG_TOTTIMO	(0x03 << 4)
+#define		APCH_CFG_TOTTIMO_3_HOUR		(0x0 << 4)
+#define		APCH_CFG_TOTTIMO_4_HOUR		(0x1 << 4)
+#define		APCH_CFG_TOTTIMO_5_HOUR		(0x2 << 4)
+#define		APCH_CFG_TOTTIMO_DISABLED	(0x3 << 4)
+#define APCH_CFG_SUSCHG		(0x01 << 7)
+
+#define APCH_STATUS_CHGDAT	(0x01 << 0)
+#define APCH_STATUS_INDAT	(0x01 << 1)
+#define APCH_STATUS_TEMPDAT	(0x01 << 2)
+#define APCH_STATUS_TIMRDAT	(0x01 << 3)
+#define APCH_STATUS_CHGSTAT	(0x01 << 4)
+#define APCH_STATUS_INSTAT	(0x01 << 5)
+#define APCH_STATUS_TEMPSTAT	(0x01 << 6)
+#define APCH_STATUS_TIMRSTAT	(0x01 << 7)
+
+#define APCH_CTRL_CHGEOCOUT	(0x01 << 0)
+#define APCH_CTRL_INDIS		(0x01 << 1)
+#define APCH_CTRL_TEMPOUT	(0x01 << 2)
+#define APCH_CTRL_TIMRPRE	(0x01 << 3)
+#define APCH_CTRL_CHGEOCIN	(0x01 << 4)
+#define APCH_CTRL_INCON		(0x01 << 5)
+#define APCH_CTRL_TEMPIN	(0x01 << 6)
+#define APCH_CTRL_TIMRTOT	(0x01 << 7)
+
+#define APCH_STATE_ACINSTAT	(0x01 << 1)
+#define APCH_STATE_CSTATE	(0x03 << 4)
+#define APCH_STATE_CSTATE_SHIFT		4
+#define		APCH_STATE_CSTATE_DISABLED	0x00
+#define		APCH_STATE_CSTATE_EOC		0x01
+#define		APCH_STATE_CSTATE_FAST		0x02
+#define		APCH_STATE_CSTATE_PRE		0x03
+
+struct act8945a_charger {
+	struct act8945a_dev *act8945a_dev;
+	struct power_supply *psy;
+
+	u32 tolal_time_out;
+	u32 pre_time_out;
+	u32 input_voltage_threshold;
+	bool battery_temperature;
+	int chglev_pin;
+	int chglev_value;
+};
+
+static int act8945a_get_charger_state(struct regmap *regmap, int *val)
+{
+	int ret;
+	unsigned int status, state;
+
+	ret = regmap_read(regmap, ACT8945A_APCH_STATUS, &status);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_read(regmap, ACT8945A_APCH_STATE, &state);
+	if (ret < 0)
+		return ret;
+
+	state &= APCH_STATE_CSTATE;
+	state >>= APCH_STATE_CSTATE_SHIFT;
+
+	if (state == APCH_STATE_CSTATE_EOC) {
+		if (status & APCH_STATUS_CHGDAT)
+			*val = POWER_SUPPLY_STATUS_FULL;
+		else
+			*val = POWER_SUPPLY_STATUS_NOT_CHARGING;
+	} else if ((state == APCH_STATE_CSTATE_FAST) ||
+		   (state == APCH_STATE_CSTATE_PRE)) {
+		*val = POWER_SUPPLY_STATUS_CHARGING;
+	} else {
+		*val = POWER_SUPPLY_STATUS_NOT_CHARGING;
+	}
+
+	return 0;
+}
+
+static int act8945a_get_charge_type(struct regmap *regmap, int *val)
+{
+	int ret;
+	unsigned int state;
+
+	ret = regmap_read(regmap, ACT8945A_APCH_STATE, &state);
+	if (ret < 0)
+		return ret;
+
+	state &= APCH_STATE_CSTATE;
+	state >>= APCH_STATE_CSTATE_SHIFT;
+
+	switch (state) {
+	case APCH_STATE_CSTATE_PRE:
+		*val = POWER_SUPPLY_CHARGE_TYPE_TRICKLE;
+		break;
+	case APCH_STATE_CSTATE_FAST:
+		*val = POWER_SUPPLY_CHARGE_TYPE_FAST;
+		break;
+	case APCH_STATE_CSTATE_EOC:
+	case APCH_STATE_CSTATE_DISABLED:
+	default:
+		*val = POWER_SUPPLY_CHARGE_TYPE_NONE;
+	}
+
+	return 0;
+}
+
+static int act8945a_get_battery_health(struct act8945a_charger *charger,
+				       struct regmap *regmap, int *val)
+{
+	int ret;
+	unsigned int status;
+
+	ret = regmap_read(regmap, ACT8945A_APCH_STATUS, &status);
+	if (ret < 0)
+		return ret;
+
+	if (charger->battery_temperature && !(status & APCH_STATUS_TEMPDAT))
+		*val = POWER_SUPPLY_HEALTH_OVERHEAT;
+	else if (!(status & APCH_STATUS_INDAT))
+		*val = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
+	else if (status & APCH_STATUS_TIMRDAT)
+		*val = POWER_SUPPLY_HEALTH_SAFETY_TIMER_EXPIRE;
+	else
+		*val = POWER_SUPPLY_HEALTH_GOOD;
+
+	return 0;
+}
+
+static enum power_supply_property act8945a_charger_props[] = {
+	POWER_SUPPLY_PROP_STATUS,
+	POWER_SUPPLY_PROP_CHARGE_TYPE,
+	POWER_SUPPLY_PROP_TECHNOLOGY,
+	POWER_SUPPLY_PROP_HEALTH,
+	POWER_SUPPLY_PROP_MODEL_NAME,
+	POWER_SUPPLY_PROP_MANUFACTURER
+};
+
+static int act8945a_charger_get_property(struct power_supply *psy,
+					 enum power_supply_property prop,
+					 union power_supply_propval *val)
+{
+	struct act8945a_charger *charger = power_supply_get_drvdata(psy);
+	struct regmap *regmap = charger->act8945a_dev->regmap;
+	int ret = 0;
+
+	switch (prop) {
+	case POWER_SUPPLY_PROP_STATUS:
+		ret = act8945a_get_charger_state(regmap, &val->intval);
+		break;
+	case POWER_SUPPLY_PROP_CHARGE_TYPE:
+		ret = act8945a_get_charge_type(regmap, &val->intval);
+		break;
+	case POWER_SUPPLY_PROP_TECHNOLOGY:
+		val->intval = POWER_SUPPLY_TECHNOLOGY_LION;
+		break;
+	case POWER_SUPPLY_PROP_HEALTH:
+		ret = act8945a_get_battery_health(charger,
+						  regmap, &val->intval);
+		break;
+	case POWER_SUPPLY_PROP_MODEL_NAME:
+		val->strval = act8945a_charger_model;
+		break;
+	case POWER_SUPPLY_PROP_MANUFACTURER:
+		val->strval = act8945a_charger_manufacturer;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
+static const struct power_supply_desc act8945a_charger_desc = {
+	.name		= "act8945a-charger",
+	.type		= POWER_SUPPLY_TYPE_BATTERY,
+	.get_property	= act8945a_charger_get_property,
+	.properties	= act8945a_charger_props,
+	.num_properties	= ARRAY_SIZE(act8945a_charger_props),
+};
+
+#define DEFAULT_TOTAL_TIME_OUT		3
+#define DEFAULT_PRE_TIME_OUT		40
+#define DEFAULT_INPUT_OVP_THRESHOLD	6600
+
+static int act8945a_charger_parse_dt(struct device *dev,
+				     struct act8945a_charger *charger)
+{
+	struct device_node *np = dev->of_node;
+	enum of_gpio_flags flags;
+
+	if (!np) {
+		dev_err(dev, "no charger of node\n");
+		return -EINVAL;
+	}
+
+	charger->chglev_pin = of_get_named_gpio_flags(np,
+				"active-semi,chglev-gpio", 0, &flags);
+
+	charger->chglev_value = (flags == OF_GPIO_ACTIVE_LOW) ? 0 : 1;
+
+	charger->battery_temperature = of_property_read_bool(np,
+					"active-semi,battery_temperature");
+
+	if (of_property_read_u32(np, "active-semi,input_voltage_threshold",
+				 &charger->input_voltage_threshold))
+		charger->input_voltage_threshold = DEFAULT_PRE_TIME_OUT;
+
+	if (of_property_read_u32(np, "active-semi,precondition_timeout",
+				 &charger->pre_time_out))
+		charger->pre_time_out = DEFAULT_PRE_TIME_OUT;
+
+	if (of_property_read_u32(np, "active-semi,total_timeout",
+				 &charger->tolal_time_out))
+		charger->tolal_time_out = DEFAULT_TOTAL_TIME_OUT;
+
+	return 0;
+}
+
+static int act8945a_charger_config(struct act8945a_charger *charger)
+{
+	struct regmap *regmap = charger->act8945a_dev->regmap;
+	u8 value = 0;
+
+	if (gpio_is_valid(charger->chglev_pin))
+		gpio_set_value(charger->chglev_pin, charger->chglev_value);
+
+	switch (charger->input_voltage_threshold) {
+	case 8000:
+		value |= APCH_CFG_OVPSET_8V;
+		break;
+	case 7500:
+		value |= APCH_CFG_OVPSET_7V5;
+		break;
+	case 7000:
+		value |= APCH_CFG_OVPSET_7V;
+		break;
+	case 6600:
+	default:
+		value |= APCH_CFG_OVPSET_6V6;
+		break;
+	}
+
+	switch (charger->pre_time_out) {
+	case 60:
+		value |= APCH_CFG_PRETIMO_60_MIN;
+		break;
+	case 80:
+		value |= APCH_CFG_PRETIMO_80_MIN;
+		break;
+	case 0:
+		value |= APCH_CFG_PRETIMO_DISABLED;
+		break;
+	case 40:
+	default:
+		value |= APCH_CFG_PRETIMO_40_MIN;
+		break;
+	}
+
+	switch (charger->tolal_time_out) {
+	case 4:
+		value |= APCH_CFG_TOTTIMO_4_HOUR;
+		break;
+	case 5:
+		value |= APCH_CFG_TOTTIMO_5_HOUR;
+		break;
+	case 0:
+		value |= APCH_CFG_TOTTIMO_DISABLED;
+		break;
+	case 3:
+	default:
+		value |= APCH_CFG_TOTTIMO_3_HOUR;
+		break;
+	}
+
+	return regmap_write(regmap, ACT8945A_APCH_CFG, value);
+}
+
+static int act8945a_charger_probe(struct platform_device *pdev)
+{
+	struct act8945a_dev *act8945a_dev = dev_get_drvdata(pdev->dev.parent);
+	struct act8945a_charger *charger;
+	struct power_supply_config psy_cfg = {};
+	int ret;
+
+	charger = devm_kzalloc(&pdev->dev, sizeof(*charger), GFP_KERNEL);
+	if (!charger)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, charger);
+
+	charger->act8945a_dev = act8945a_dev;
+
+	ret = act8945a_charger_parse_dt(&pdev->dev, charger);
+	if (ret)
+		return ret;
+
+	ret = act8945a_charger_config(charger);
+	if (ret)
+		return ret;
+
+	psy_cfg.of_node	= pdev->dev.of_node;
+	psy_cfg.drv_data = charger;
+
+	charger->psy = devm_power_supply_register(&pdev->dev,
+						  &act8945a_charger_desc,
+						  &psy_cfg);
+	if (IS_ERR(charger->psy)) {
+		dev_err(&pdev->dev, "failed to register power supply\n");
+		return PTR_ERR(charger->psy);
+	}
+
+	return 0;
+}
+
+static struct platform_driver act8945a_charger_driver = {
+	.driver	= {
+		.name = "act8945a-charger",
+	},
+	.probe	= act8945a_charger_probe,
+};
+module_platform_driver(act8945a_charger_driver);
+
+MODULE_DESCRIPTION("Active-semi ACT8945A ActivePath charger driver");
+MODULE_AUTHOR("Wenyou Yang <wenyou.yang@atmel.com>");
+MODULE_LICENSE("GPL");
-- 
1.7.9.5

Re: [PATCH v3 1/2] power: act8945a: add charger driver for ACT8945A

[Krzysztof Kozlowski]

On 12.01.2016 17:09, Wenyou Yang wrote:
> This patch adds new driver for Active-semi ACT8945A ActivePath
> charger (part of ACT8945A MFD driver) providing power supply class
> information to userspace.
> 
> The driver is configured through DTS (battery and system related
> settings) and sysfs entries (timers and input over-voltage threshold).
> 
> Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> ---
> 
> Changes in v3:
>  - update the file header with short version license and author line.
>  - remove unused member of struct act8945a_charger, dev.
>  - action due to removing the member of stuct act8945a_dev, dev.
>  - remove the unnecessary print out.
>  - remove the unnecessary act8945a_charger_remove().
>  - fix align of the code-style.
> 
> Changes in v2:
>  1./ Substitute of_property_read_bool() for of_get_property().
>  2./ Substitute devm_power_supply_register() for power_supply_register().
>  3./ Use module_platform_driver(), instead of subsys_initcall().
>  4./ Substitute MODULE_LICENSE("GPL") for MODULE_LICENSE("GPL v2").
> 
>  drivers/power/Kconfig            |    7 +
>  drivers/power/Makefile           |    1 +
>  drivers/power/act8945a_charger.c |  367 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 375 insertions(+)
>  create mode 100644 drivers/power/act8945a_charger.c
> 
> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
> index 1ddd13c..ae75211 100644
> --- a/drivers/power/Kconfig
> +++ b/drivers/power/Kconfig
> @@ -75,6 +75,13 @@ config BATTERY_88PM860X
>  	help
>  	  Say Y here to enable battery monitor for Marvell 88PM860x chip.
>  
> +config BATTERY_ACT8945A
> +	tristate "Active-semi ACT8945A charger driver"
> +	depends on MFD_ACT8945A
> +	help
> +	  Say Y here to enable support for power supply provided by
> +	  Active-semi ActivePath ACT8945A charger.
> +
>  config BATTERY_DS2760
>  	tristate "DS2760 battery driver (HP iPAQ & others)"
>  	depends on W1 && W1_SLAVE_DS2760
> diff --git a/drivers/power/Makefile b/drivers/power/Makefile
> index 0e4eab5..e46b75d 100644
> --- a/drivers/power/Makefile
> +++ b/drivers/power/Makefile
> @@ -17,6 +17,7 @@ obj-$(CONFIG_WM8350_POWER)	+= wm8350_power.o
>  obj-$(CONFIG_TEST_POWER)	+= test_power.o
>  
>  obj-$(CONFIG_BATTERY_88PM860X)	+= 88pm860x_battery.o
> +obj-$(CONFIG_BATTERY_ACT8945A)	+= act8945a_charger.o
>  obj-$(CONFIG_BATTERY_DS2760)	+= ds2760_battery.o
>  obj-$(CONFIG_BATTERY_DS2780)	+= ds2780_battery.o
>  obj-$(CONFIG_BATTERY_DS2781)	+= ds2781_battery.o
> diff --git a/drivers/power/act8945a_charger.c b/drivers/power/act8945a_charger.c
> new file mode 100644
> index 0000000..643a2ed
> --- /dev/null
> +++ b/drivers/power/act8945a_charger.c
> @@ -0,0 +1,367 @@
> +/*
> + * Power supply driver for the Active-semi ACT8945A PMIC
> + *
> + * Copyright (C) 2015 Atmel Corporation
> + *
> + * Author: Wenyou Yang <wenyou.yang@atmel.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +#include <linux/module.h>
> +#include <linux/mfd/act8945a.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
> +#include <linux/platform_device.h>
> +#include <linux/power_supply.h>
> +#include <linux/regmap.h>
> +
> +static const char *act8945a_charger_model = "ACT8945A";
> +static const char *act8945a_charger_manufacturer = "Active-semi";
> +
> +/**
> + * ACT8945A Charger Register Map
> + */
> +
> +/* 0x70: Reserved */
> +#define ACT8945A_APCH_CFG		0x71
> +#define ACT8945A_APCH_STATUS		0x78
> +#define ACT8945A_APCH_CTRL		0x79
> +#define ACT8945A_APCH_STATE		0x7A
> +
> +/* ACT8945A_APCH_CFG */
> +#define APCH_CFG_OVPSET		(0x03 << 0)
> +#define		APCH_CFG_OVPSET_6V6	(0x0 << 0)
> +#define		APCH_CFG_OVPSET_7V	(0x1 << 0)
> +#define		APCH_CFG_OVPSET_7V5	(0x2 << 0)
> +#define		APCH_CFG_OVPSET_8V	(0x3 << 0)

Why the tabs after #define? Nooooo...

> +#define APCH_CFG_PRETIMO	(0x03 << 2)
> +#define		APCH_CFG_PRETIMO_40_MIN		(0x0 << 2)
> +#define		APCH_CFG_PRETIMO_60_MIN		(0x1 << 2)
> +#define		APCH_CFG_PRETIMO_80_MIN		(0x2 << 2)
> +#define		APCH_CFG_PRETIMO_DISABLED	(0x3 << 2)
> +#define APCH_CFG_TOTTIMO	(0x03 << 4)
> +#define		APCH_CFG_TOTTIMO_3_HOUR		(0x0 << 4)
> +#define		APCH_CFG_TOTTIMO_4_HOUR		(0x1 << 4)
> +#define		APCH_CFG_TOTTIMO_5_HOUR		(0x2 << 4)
> +#define		APCH_CFG_TOTTIMO_DISABLED	(0x3 << 4)
> +#define APCH_CFG_SUSCHG		(0x01 << 7)
> +
> +#define APCH_STATUS_CHGDAT	(0x01 << 0)
> +#define APCH_STATUS_INDAT	(0x01 << 1)
> +#define APCH_STATUS_TEMPDAT	(0x01 << 2)
> +#define APCH_STATUS_TIMRDAT	(0x01 << 3)
> +#define APCH_STATUS_CHGSTAT	(0x01 << 4)
> +#define APCH_STATUS_INSTAT	(0x01 << 5)
> +#define APCH_STATUS_TEMPSTAT	(0x01 << 6)
> +#define APCH_STATUS_TIMRSTAT	(0x01 << 7)
> +
> +#define APCH_CTRL_CHGEOCOUT	(0x01 << 0)
> +#define APCH_CTRL_INDIS		(0x01 << 1)
> +#define APCH_CTRL_TEMPOUT	(0x01 << 2)
> +#define APCH_CTRL_TIMRPRE	(0x01 << 3)
> +#define APCH_CTRL_CHGEOCIN	(0x01 << 4)
> +#define APCH_CTRL_INCON		(0x01 << 5)
> +#define APCH_CTRL_TEMPIN	(0x01 << 6)
> +#define APCH_CTRL_TIMRTOT	(0x01 << 7)

For the STATUS and CTRL use BIT(n) macro.


> +
> +#define APCH_STATE_ACINSTAT	(0x01 << 1)
> +#define APCH_STATE_CSTATE	(0x03 << 4)
> +#define APCH_STATE_CSTATE_SHIFT		4
> +#define		APCH_STATE_CSTATE_DISABLED	0x00
> +#define		APCH_STATE_CSTATE_EOC		0x01
> +#define		APCH_STATE_CSTATE_FAST		0x02
> +#define		APCH_STATE_CSTATE_PRE		0x03
> +
> +struct act8945a_charger {
> +	struct act8945a_dev *act8945a_dev;
> +	struct power_supply *psy;
> +
> +	u32 tolal_time_out;
> +	u32 pre_time_out;
> +	u32 input_voltage_threshold;
> +	bool battery_temperature;
> +	int chglev_pin;
> +	int chglev_value;
> +};
> +
> +static int act8945a_get_charger_state(struct regmap *regmap, int *val)
> +{
> +	int ret;
> +	unsigned int status, state;
> +
> +	ret = regmap_read(regmap, ACT8945A_APCH_STATUS, &status);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_read(regmap, ACT8945A_APCH_STATE, &state);
> +	if (ret < 0)
> +		return ret;
> +
> +	state &= APCH_STATE_CSTATE;
> +	state >>= APCH_STATE_CSTATE_SHIFT;
> +
> +	if (state == APCH_STATE_CSTATE_EOC) {
> +		if (status & APCH_STATUS_CHGDAT)
> +			*val = POWER_SUPPLY_STATUS_FULL;
> +		else
> +			*val = POWER_SUPPLY_STATUS_NOT_CHARGING;
> +	} else if ((state == APCH_STATE_CSTATE_FAST) ||
> +		   (state == APCH_STATE_CSTATE_PRE)) {
> +		*val = POWER_SUPPLY_STATUS_CHARGING;
> +	} else {
> +		*val = POWER_SUPPLY_STATUS_NOT_CHARGING;
> +	}
> +
> +	return 0;
> +}
> +
> +static int act8945a_get_charge_type(struct regmap *regmap, int *val)
> +{
> +	int ret;
> +	unsigned int state;
> +
> +	ret = regmap_read(regmap, ACT8945A_APCH_STATE, &state);
> +	if (ret < 0)
> +		return ret;
> +
> +	state &= APCH_STATE_CSTATE;
> +	state >>= APCH_STATE_CSTATE_SHIFT;
> +
> +	switch (state) {
> +	case APCH_STATE_CSTATE_PRE:
> +		*val = POWER_SUPPLY_CHARGE_TYPE_TRICKLE;
> +		break;
> +	case APCH_STATE_CSTATE_FAST:
> +		*val = POWER_SUPPLY_CHARGE_TYPE_FAST;
> +		break;
> +	case APCH_STATE_CSTATE_EOC:
> +	case APCH_STATE_CSTATE_DISABLED:
> +	default:
> +		*val = POWER_SUPPLY_CHARGE_TYPE_NONE;
> +	}
> +
> +	return 0;
> +}
> +
> +static int act8945a_get_battery_health(struct act8945a_charger *charger,
> +				       struct regmap *regmap, int *val)
> +{
> +	int ret;
> +	unsigned int status;
> +
> +	ret = regmap_read(regmap, ACT8945A_APCH_STATUS, &status);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (charger->battery_temperature && !(status & APCH_STATUS_TEMPDAT))
> +		*val = POWER_SUPPLY_HEALTH_OVERHEAT;
> +	else if (!(status & APCH_STATUS_INDAT))
> +		*val = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
> +	else if (status & APCH_STATUS_TIMRDAT)
> +		*val = POWER_SUPPLY_HEALTH_SAFETY_TIMER_EXPIRE;
> +	else
> +		*val = POWER_SUPPLY_HEALTH_GOOD;
> +
> +	return 0;
> +}
> +
> +static enum power_supply_property act8945a_charger_props[] = {
> +	POWER_SUPPLY_PROP_STATUS,
> +	POWER_SUPPLY_PROP_CHARGE_TYPE,
> +	POWER_SUPPLY_PROP_TECHNOLOGY,
> +	POWER_SUPPLY_PROP_HEALTH,
> +	POWER_SUPPLY_PROP_MODEL_NAME,
> +	POWER_SUPPLY_PROP_MANUFACTURER
> +};
> +
> +static int act8945a_charger_get_property(struct power_supply *psy,
> +					 enum power_supply_property prop,
> +					 union power_supply_propval *val)
> +{
> +	struct act8945a_charger *charger = power_supply_get_drvdata(psy);
> +	struct regmap *regmap = charger->act8945a_dev->regmap;
> +	int ret = 0;
> +
> +	switch (prop) {
> +	case POWER_SUPPLY_PROP_STATUS:
> +		ret = act8945a_get_charger_state(regmap, &val->intval);
> +		break;
> +	case POWER_SUPPLY_PROP_CHARGE_TYPE:
> +		ret = act8945a_get_charge_type(regmap, &val->intval);
> +		break;
> +	case POWER_SUPPLY_PROP_TECHNOLOGY:
> +		val->intval = POWER_SUPPLY_TECHNOLOGY_LION;
> +		break;
> +	case POWER_SUPPLY_PROP_HEALTH:
> +		ret = act8945a_get_battery_health(charger,
> +						  regmap, &val->intval);
> +		break;
> +	case POWER_SUPPLY_PROP_MODEL_NAME:
> +		val->strval = act8945a_charger_model;
> +		break;
> +	case POWER_SUPPLY_PROP_MANUFACTURER:
> +		val->strval = act8945a_charger_manufacturer;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
> +static const struct power_supply_desc act8945a_charger_desc = {
> +	.name		= "act8945a-charger",
> +	.type		= POWER_SUPPLY_TYPE_BATTERY,
> +	.get_property	= act8945a_charger_get_property,
> +	.properties	= act8945a_charger_props,
> +	.num_properties	= ARRAY_SIZE(act8945a_charger_props),
> +};
> +
> +#define DEFAULT_TOTAL_TIME_OUT		3
> +#define DEFAULT_PRE_TIME_OUT		40
> +#define DEFAULT_INPUT_OVP_THRESHOLD	6600
> +
> +static int act8945a_charger_parse_dt(struct device *dev,
> +				     struct act8945a_charger *charger)
> +{
> +	struct device_node *np = dev->of_node;
> +	enum of_gpio_flags flags;
> +
> +	if (!np) {
> +		dev_err(dev, "no charger of node\n");
> +		return -EINVAL;
> +	}
> +
> +	charger->chglev_pin = of_get_named_gpio_flags(np,
> +				"active-semi,chglev-gpio", 0, &flags);
> +
> +	charger->chglev_value = (flags == OF_GPIO_ACTIVE_LOW) ? 0 : 1;
> +
> +	charger->battery_temperature = of_property_read_bool(np,
> +					"active-semi,battery_temperature");
> +
> +	if (of_property_read_u32(np, "active-semi,input_voltage_threshold",
> +				 &charger->input_voltage_threshold))
> +		charger->input_voltage_threshold = DEFAULT_PRE_TIME_OUT;
> +
> +	if (of_property_read_u32(np, "active-semi,precondition_timeout",
> +				 &charger->pre_time_out))
> +		charger->pre_time_out = DEFAULT_PRE_TIME_OUT;
> +
> +	if (of_property_read_u32(np, "active-semi,total_timeout",
> +				 &charger->tolal_time_out))
> +		charger->tolal_time_out = DEFAULT_TOTAL_TIME_OUT;
> +
> +	return 0;
> +}
> +
> +static int act8945a_charger_config(struct act8945a_charger *charger)
> +{
> +	struct regmap *regmap = charger->act8945a_dev->regmap;
> +	u8 value = 0;

Regmap uses "unsigned int". I think it is better not to mix it so use in
the driver "unsigned int" for interactions with regmap.

[...]

Javier pointed the auto-loading issue before (because you are not
providing any device table). Probably that is not what you want.

Best regards,
Krzysztof

RE: [PATCH v3 1/2] power: act8945a: add charger driver for ACT8945A

[Yang, Wenyou]

Hi Krzysztof,

Thank you!

> -----Original Message-----
> From: Krzysztof Kozlowski [mailto:k.kozlowski@samsung.com]
> Sent: 2016年1月13日 10:12
> To: Yang, Wenyou <Wenyou.Yang@atmel.com>; Sebastian Reichel
> <sre@kernel.org>; Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>; David
> Woodhouse <dwmw2@infradead.org>; Rob Herring <robh+dt@kernel.org>; Pawel
> Moll <pawel.moll@arm.com>; Mark Rutland <mark.rutland@arm.com>; Ian
> Campbell <ijc+devicetree@hellion.org.uk>; Kumar Gala <galak@codeaurora.org>
> Cc: Javier Martinez Canillas <javier@dowhile0.org>; Lee Jones
> <lee.jones@linaro.org>; Ferre, Nicolas <Nicolas.FERRE@atmel.com>; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
> pm@vger.kernel.org
> Subject: Re: [PATCH v3 1/2] power: act8945a: add charger driver for ACT8945A
> 
> On 12.01.2016 17:09, Wenyou Yang wrote:
> > This patch adds new driver for Active-semi ACT8945A ActivePath charger
> > (part of ACT8945A MFD driver) providing power supply class information
> > to userspace.
> >
> > The driver is configured through DTS (battery and system related
> > settings) and sysfs entries (timers and input over-voltage threshold).
> >
> > Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> > ---
> >
> > Changes in v3:
> >  - update the file header with short version license and author line.
> >  - remove unused member of struct act8945a_charger, dev.
> >  - action due to removing the member of stuct act8945a_dev, dev.
> >  - remove the unnecessary print out.
> >  - remove the unnecessary act8945a_charger_remove().
> >  - fix align of the code-style.
> >
> > Changes in v2:
> >  1./ Substitute of_property_read_bool() for of_get_property().
> >  2./ Substitute devm_power_supply_register() for power_supply_register().
> >  3./ Use module_platform_driver(), instead of subsys_initcall().
> >  4./ Substitute MODULE_LICENSE("GPL") for MODULE_LICENSE("GPL v2").
> >
> >  drivers/power/Kconfig            |    7 +
> >  drivers/power/Makefile           |    1 +
> >  drivers/power/act8945a_charger.c |  367
> > ++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 375 insertions(+)
> >  create mode 100644 drivers/power/act8945a_charger.c
> >
> > diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig index
> > 1ddd13c..ae75211 100644
> > --- a/drivers/power/Kconfig
> > +++ b/drivers/power/Kconfig
> > @@ -75,6 +75,13 @@ config BATTERY_88PM860X
> >  	help
> >  	  Say Y here to enable battery monitor for Marvell 88PM860x chip.
> >
> > +config BATTERY_ACT8945A
> > +	tristate "Active-semi ACT8945A charger driver"
> > +	depends on MFD_ACT8945A
> > +	help
> > +	  Say Y here to enable support for power supply provided by
> > +	  Active-semi ActivePath ACT8945A charger.
> > +
> >  config BATTERY_DS2760
> >  	tristate "DS2760 battery driver (HP iPAQ & others)"
> >  	depends on W1 && W1_SLAVE_DS2760
> > diff --git a/drivers/power/Makefile b/drivers/power/Makefile index
> > 0e4eab5..e46b75d 100644
> > --- a/drivers/power/Makefile
> > +++ b/drivers/power/Makefile
> > @@ -17,6 +17,7 @@ obj-$(CONFIG_WM8350_POWER)	+=
> wm8350_power.o
> >  obj-$(CONFIG_TEST_POWER)	+= test_power.o
> >
> >  obj-$(CONFIG_BATTERY_88PM860X)	+= 88pm860x_battery.o
> > +obj-$(CONFIG_BATTERY_ACT8945A)	+= act8945a_charger.o
> >  obj-$(CONFIG_BATTERY_DS2760)	+= ds2760_battery.o
> >  obj-$(CONFIG_BATTERY_DS2780)	+= ds2780_battery.o
> >  obj-$(CONFIG_BATTERY_DS2781)	+= ds2781_battery.o
> > diff --git a/drivers/power/act8945a_charger.c
> > b/drivers/power/act8945a_charger.c
> > new file mode 100644
> > index 0000000..643a2ed
> > --- /dev/null
> > +++ b/drivers/power/act8945a_charger.c
> > @@ -0,0 +1,367 @@
> > +/*
> > + * Power supply driver for the Active-semi ACT8945A PMIC
> > + *
> > + * Copyright (C) 2015 Atmel Corporation
> > + *
> > + * Author: Wenyou Yang <wenyou.yang@atmel.com>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > +modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + */
> > +#include <linux/module.h>
> > +#include <linux/mfd/act8945a.h>
> > +#include <linux/of.h>
> > +#include <linux/of_gpio.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/power_supply.h>
> > +#include <linux/regmap.h>
> > +
> > +static const char *act8945a_charger_model = "ACT8945A"; static const
> > +char *act8945a_charger_manufacturer = "Active-semi";
> > +
> > +/**
> > + * ACT8945A Charger Register Map
> > + */
> > +
> > +/* 0x70: Reserved */
> > +#define ACT8945A_APCH_CFG		0x71
> > +#define ACT8945A_APCH_STATUS		0x78
> > +#define ACT8945A_APCH_CTRL		0x79
> > +#define ACT8945A_APCH_STATE		0x7A
> > +
> > +/* ACT8945A_APCH_CFG */
> > +#define APCH_CFG_OVPSET		(0x03 << 0)
> > +#define		APCH_CFG_OVPSET_6V6	(0x0 << 0)
> > +#define		APCH_CFG_OVPSET_7V	(0x1 << 0)
> > +#define		APCH_CFG_OVPSET_7V5	(0x2 << 0)
> > +#define		APCH_CFG_OVPSET_8V	(0x3 << 0)
> 
> Why the tabs after #define? Nooooo...
> 
> > +#define APCH_CFG_PRETIMO	(0x03 << 2)
> > +#define		APCH_CFG_PRETIMO_40_MIN		(0x0 << 2)
> > +#define		APCH_CFG_PRETIMO_60_MIN		(0x1 << 2)
> > +#define		APCH_CFG_PRETIMO_80_MIN		(0x2 << 2)
> > +#define		APCH_CFG_PRETIMO_DISABLED	(0x3 << 2)
> > +#define APCH_CFG_TOTTIMO	(0x03 << 4)
> > +#define		APCH_CFG_TOTTIMO_3_HOUR		(0x0 << 4)
> > +#define		APCH_CFG_TOTTIMO_4_HOUR		(0x1 << 4)
> > +#define		APCH_CFG_TOTTIMO_5_HOUR		(0x2 << 4)
> > +#define		APCH_CFG_TOTTIMO_DISABLED	(0x3 << 4)
> > +#define APCH_CFG_SUSCHG		(0x01 << 7)
> > +
> > +#define APCH_STATUS_CHGDAT	(0x01 << 0)
> > +#define APCH_STATUS_INDAT	(0x01 << 1)
> > +#define APCH_STATUS_TEMPDAT	(0x01 << 2)
> > +#define APCH_STATUS_TIMRDAT	(0x01 << 3)
> > +#define APCH_STATUS_CHGSTAT	(0x01 << 4)
> > +#define APCH_STATUS_INSTAT	(0x01 << 5)
> > +#define APCH_STATUS_TEMPSTAT	(0x01 << 6)
> > +#define APCH_STATUS_TIMRSTAT	(0x01 << 7)
> > +
> > +#define APCH_CTRL_CHGEOCOUT	(0x01 << 0)
> > +#define APCH_CTRL_INDIS		(0x01 << 1)
> > +#define APCH_CTRL_TEMPOUT	(0x01 << 2)
> > +#define APCH_CTRL_TIMRPRE	(0x01 << 3)
> > +#define APCH_CTRL_CHGEOCIN	(0x01 << 4)
> > +#define APCH_CTRL_INCON		(0x01 << 5)
> > +#define APCH_CTRL_TEMPIN	(0x01 << 6)
> > +#define APCH_CTRL_TIMRTOT	(0x01 << 7)
> 
> For the STATUS and CTRL use BIT(n) macro.
> 
> 
> > +
> > +#define APCH_STATE_ACINSTAT	(0x01 << 1)
> > +#define APCH_STATE_CSTATE	(0x03 << 4)
> > +#define APCH_STATE_CSTATE_SHIFT		4
> > +#define		APCH_STATE_CSTATE_DISABLED	0x00
> > +#define		APCH_STATE_CSTATE_EOC		0x01
> > +#define		APCH_STATE_CSTATE_FAST		0x02
> > +#define		APCH_STATE_CSTATE_PRE		0x03
> > +
> > +struct act8945a_charger {
> > +	struct act8945a_dev *act8945a_dev;
> > +	struct power_supply *psy;
> > +
> > +	u32 tolal_time_out;
> > +	u32 pre_time_out;
> > +	u32 input_voltage_threshold;
> > +	bool battery_temperature;
> > +	int chglev_pin;
> > +	int chglev_value;
> > +};
> > +
> > +static int act8945a_get_charger_state(struct regmap *regmap, int
> > +*val) {
> > +	int ret;
> > +	unsigned int status, state;
> > +
> > +	ret = regmap_read(regmap, ACT8945A_APCH_STATUS, &status);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = regmap_read(regmap, ACT8945A_APCH_STATE, &state);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	state &= APCH_STATE_CSTATE;
> > +	state >>= APCH_STATE_CSTATE_SHIFT;
> > +
> > +	if (state == APCH_STATE_CSTATE_EOC) {
> > +		if (status & APCH_STATUS_CHGDAT)
> > +			*val = POWER_SUPPLY_STATUS_FULL;
> > +		else
> > +			*val = POWER_SUPPLY_STATUS_NOT_CHARGING;
> > +	} else if ((state == APCH_STATE_CSTATE_FAST) ||
> > +		   (state == APCH_STATE_CSTATE_PRE)) {
> > +		*val = POWER_SUPPLY_STATUS_CHARGING;
> > +	} else {
> > +		*val = POWER_SUPPLY_STATUS_NOT_CHARGING;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int act8945a_get_charge_type(struct regmap *regmap, int *val)
> > +{
> > +	int ret;
> > +	unsigned int state;
> > +
> > +	ret = regmap_read(regmap, ACT8945A_APCH_STATE, &state);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	state &= APCH_STATE_CSTATE;
> > +	state >>= APCH_STATE_CSTATE_SHIFT;
> > +
> > +	switch (state) {
> > +	case APCH_STATE_CSTATE_PRE:
> > +		*val = POWER_SUPPLY_CHARGE_TYPE_TRICKLE;
> > +		break;
> > +	case APCH_STATE_CSTATE_FAST:
> > +		*val = POWER_SUPPLY_CHARGE_TYPE_FAST;
> > +		break;
> > +	case APCH_STATE_CSTATE_EOC:
> > +	case APCH_STATE_CSTATE_DISABLED:
> > +	default:
> > +		*val = POWER_SUPPLY_CHARGE_TYPE_NONE;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int act8945a_get_battery_health(struct act8945a_charger *charger,
> > +				       struct regmap *regmap, int *val) {
> > +	int ret;
> > +	unsigned int status;
> > +
> > +	ret = regmap_read(regmap, ACT8945A_APCH_STATUS, &status);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if (charger->battery_temperature && !(status &
> APCH_STATUS_TEMPDAT))
> > +		*val = POWER_SUPPLY_HEALTH_OVERHEAT;
> > +	else if (!(status & APCH_STATUS_INDAT))
> > +		*val = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
> > +	else if (status & APCH_STATUS_TIMRDAT)
> > +		*val = POWER_SUPPLY_HEALTH_SAFETY_TIMER_EXPIRE;
> > +	else
> > +		*val = POWER_SUPPLY_HEALTH_GOOD;
> > +
> > +	return 0;
> > +}
> > +
> > +static enum power_supply_property act8945a_charger_props[] = {
> > +	POWER_SUPPLY_PROP_STATUS,
> > +	POWER_SUPPLY_PROP_CHARGE_TYPE,
> > +	POWER_SUPPLY_PROP_TECHNOLOGY,
> > +	POWER_SUPPLY_PROP_HEALTH,
> > +	POWER_SUPPLY_PROP_MODEL_NAME,
> > +	POWER_SUPPLY_PROP_MANUFACTURER
> > +};
> > +
> > +static int act8945a_charger_get_property(struct power_supply *psy,
> > +					 enum power_supply_property prop,
> > +					 union power_supply_propval *val) {
> > +	struct act8945a_charger *charger = power_supply_get_drvdata(psy);
> > +	struct regmap *regmap = charger->act8945a_dev->regmap;
> > +	int ret = 0;
> > +
> > +	switch (prop) {
> > +	case POWER_SUPPLY_PROP_STATUS:
> > +		ret = act8945a_get_charger_state(regmap, &val->intval);
> > +		break;
> > +	case POWER_SUPPLY_PROP_CHARGE_TYPE:
> > +		ret = act8945a_get_charge_type(regmap, &val->intval);
> > +		break;
> > +	case POWER_SUPPLY_PROP_TECHNOLOGY:
> > +		val->intval = POWER_SUPPLY_TECHNOLOGY_LION;
> > +		break;
> > +	case POWER_SUPPLY_PROP_HEALTH:
> > +		ret = act8945a_get_battery_health(charger,
> > +						  regmap, &val->intval);
> > +		break;
> > +	case POWER_SUPPLY_PROP_MODEL_NAME:
> > +		val->strval = act8945a_charger_model;
> > +		break;
> > +	case POWER_SUPPLY_PROP_MANUFACTURER:
> > +		val->strval = act8945a_charger_manufacturer;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static const struct power_supply_desc act8945a_charger_desc = {
> > +	.name		= "act8945a-charger",
> > +	.type		= POWER_SUPPLY_TYPE_BATTERY,
> > +	.get_property	= act8945a_charger_get_property,
> > +	.properties	= act8945a_charger_props,
> > +	.num_properties	= ARRAY_SIZE(act8945a_charger_props),
> > +};
> > +
> > +#define DEFAULT_TOTAL_TIME_OUT		3
> > +#define DEFAULT_PRE_TIME_OUT		40
> > +#define DEFAULT_INPUT_OVP_THRESHOLD	6600
> > +
> > +static int act8945a_charger_parse_dt(struct device *dev,
> > +				     struct act8945a_charger *charger) {
> > +	struct device_node *np = dev->of_node;
> > +	enum of_gpio_flags flags;
> > +
> > +	if (!np) {
> > +		dev_err(dev, "no charger of node\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	charger->chglev_pin = of_get_named_gpio_flags(np,
> > +				"active-semi,chglev-gpio", 0, &flags);
> > +
> > +	charger->chglev_value = (flags == OF_GPIO_ACTIVE_LOW) ? 0 : 1;
> > +
> > +	charger->battery_temperature = of_property_read_bool(np,
> > +					"active-semi,battery_temperature");
> > +
> > +	if (of_property_read_u32(np, "active-semi,input_voltage_threshold",
> > +				 &charger->input_voltage_threshold))
> > +		charger->input_voltage_threshold = DEFAULT_PRE_TIME_OUT;
> > +
> > +	if (of_property_read_u32(np, "active-semi,precondition_timeout",
> > +				 &charger->pre_time_out))
> > +		charger->pre_time_out = DEFAULT_PRE_TIME_OUT;
> > +
> > +	if (of_property_read_u32(np, "active-semi,total_timeout",
> > +				 &charger->tolal_time_out))
> > +		charger->tolal_time_out = DEFAULT_TOTAL_TIME_OUT;
> > +
> > +	return 0;
> > +}
> > +
> > +static int act8945a_charger_config(struct act8945a_charger *charger)
> > +{
> > +	struct regmap *regmap = charger->act8945a_dev->regmap;
> > +	u8 value = 0;
> 
> Regmap uses "unsigned int". I think it is better not to mix it so use in the driver
> "unsigned int" for interactions with regmap.

I checked, it seems they are not mixed in the patch. Or I am wrong?

> 
> [...]
> 
> Javier pointed the auto-loading issue before (because you are not providing any
> device table). Probably that is not what you want.

Yes, but I  still don't understand for now ...

> 
> Best regards,
> Krzysztof


Best Regards,
Wenyou Yang

Re: [PATCH v3 1/2] power: act8945a: add charger driver for ACT8945A

[Krzysztof Kozlowski]

On 13.01.2016 12:53, Yang, Wenyou wrote:
> Hi Krzysztof,
> 
> Thank you!
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski [mailto:k.kozlowski@samsung.com]
>> Sent: 2016年1月13日 10:12
>> To: Yang, Wenyou <Wenyou.Yang@atmel.com>; Sebastian Reichel
>> <sre@kernel.org>; Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>; David
>> Woodhouse <dwmw2@infradead.org>; Rob Herring <robh+dt@kernel.org>; Pawel
>> Moll <pawel.moll@arm.com>; Mark Rutland <mark.rutland@arm.com>; Ian
>> Campbell <ijc+devicetree@hellion.org.uk>; Kumar Gala <galak@codeaurora.org>
>> Cc: Javier Martinez Canillas <javier@dowhile0.org>; Lee Jones
>> <lee.jones@linaro.org>; Ferre, Nicolas <Nicolas.FERRE@atmel.com>; linux-arm-
>> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
>> pm@vger.kernel.org
>> Subject: Re: [PATCH v3 1/2] power: act8945a: add charger driver for ACT8945A
>>
>> On 12.01.2016 17:09, Wenyou Yang wrote:
>>> This patch adds new driver for Active-semi ACT8945A ActivePath charger
>>> (part of ACT8945A MFD driver) providing power supply class information
>>> to userspace.
>>>
>>> The driver is configured through DTS (battery and system related
>>> settings) and sysfs entries (timers and input over-voltage threshold).
>>>
>>> Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
>>> ---
>>>
>>> Changes in v3:
>>>  - update the file header with short version license and author line.
>>>  - remove unused member of struct act8945a_charger, dev.
>>>  - action due to removing the member of stuct act8945a_dev, dev.
>>>  - remove the unnecessary print out.
>>>  - remove the unnecessary act8945a_charger_remove().
>>>  - fix align of the code-style.
>>>
>>> Changes in v2:
>>>  1./ Substitute of_property_read_bool() for of_get_property().
>>>  2./ Substitute devm_power_supply_register() for power_supply_register().
>>>  3./ Use module_platform_driver(), instead of subsys_initcall().
>>>  4./ Substitute MODULE_LICENSE("GPL") for MODULE_LICENSE("GPL v2").
>>>
>>>  drivers/power/Kconfig            |    7 +
>>>  drivers/power/Makefile           |    1 +
>>>  drivers/power/act8945a_charger.c |  367
>>> ++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 375 insertions(+)
>>>  create mode 100644 drivers/power/act8945a_charger.c
>>>
>>> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig index
>>> 1ddd13c..ae75211 100644
>>> --- a/drivers/power/Kconfig
>>> +++ b/drivers/power/Kconfig
>>> @@ -75,6 +75,13 @@ config BATTERY_88PM860X
>>>  	help
>>>  	  Say Y here to enable battery monitor for Marvell 88PM860x chip.
>>>
>>> +config BATTERY_ACT8945A
>>> +	tristate "Active-semi ACT8945A charger driver"
>>> +	depends on MFD_ACT8945A
>>> +	help
>>> +	  Say Y here to enable support for power supply provided by
>>> +	  Active-semi ActivePath ACT8945A charger.
>>> +
>>>  config BATTERY_DS2760
>>>  	tristate "DS2760 battery driver (HP iPAQ & others)"
>>>  	depends on W1 && W1_SLAVE_DS2760
>>> diff --git a/drivers/power/Makefile b/drivers/power/Makefile index
>>> 0e4eab5..e46b75d 100644
>>> --- a/drivers/power/Makefile
>>> +++ b/drivers/power/Makefile
>>> @@ -17,6 +17,7 @@ obj-$(CONFIG_WM8350_POWER)	+=
>> wm8350_power.o
>>>  obj-$(CONFIG_TEST_POWER)	+= test_power.o
>>>
>>>  obj-$(CONFIG_BATTERY_88PM860X)	+= 88pm860x_battery.o
>>> +obj-$(CONFIG_BATTERY_ACT8945A)	+= act8945a_charger.o
>>>  obj-$(CONFIG_BATTERY_DS2760)	+= ds2760_battery.o
>>>  obj-$(CONFIG_BATTERY_DS2780)	+= ds2780_battery.o
>>>  obj-$(CONFIG_BATTERY_DS2781)	+= ds2781_battery.o
>>> diff --git a/drivers/power/act8945a_charger.c
>>> b/drivers/power/act8945a_charger.c
>>> new file mode 100644
>>> index 0000000..643a2ed
>>> --- /dev/null
>>> +++ b/drivers/power/act8945a_charger.c
>>> @@ -0,0 +1,367 @@
>>> +/*
>>> + * Power supply driver for the Active-semi ACT8945A PMIC
>>> + *
>>> + * Copyright (C) 2015 Atmel Corporation
>>> + *
>>> + * Author: Wenyou Yang <wenyou.yang@atmel.com>
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>>> +modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + *
>>> + */
>>> +#include <linux/module.h>
>>> +#include <linux/mfd/act8945a.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_gpio.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/power_supply.h>
>>> +#include <linux/regmap.h>
>>> +
>>> +static const char *act8945a_charger_model = "ACT8945A"; static const
>>> +char *act8945a_charger_manufacturer = "Active-semi";
>>> +
>>> +/**
>>> + * ACT8945A Charger Register Map
>>> + */
>>> +
>>> +/* 0x70: Reserved */
>>> +#define ACT8945A_APCH_CFG		0x71
>>> +#define ACT8945A_APCH_STATUS		0x78
>>> +#define ACT8945A_APCH_CTRL		0x79
>>> +#define ACT8945A_APCH_STATE		0x7A
>>> +
>>> +/* ACT8945A_APCH_CFG */
>>> +#define APCH_CFG_OVPSET		(0x03 << 0)
>>> +#define		APCH_CFG_OVPSET_6V6	(0x0 << 0)
>>> +#define		APCH_CFG_OVPSET_7V	(0x1 << 0)
>>> +#define		APCH_CFG_OVPSET_7V5	(0x2 << 0)
>>> +#define		APCH_CFG_OVPSET_8V	(0x3 << 0)
>>
>> Why the tabs after #define? Nooooo...
>>
>>> +#define APCH_CFG_PRETIMO	(0x03 << 2)
>>> +#define		APCH_CFG_PRETIMO_40_MIN		(0x0 << 2)
>>> +#define		APCH_CFG_PRETIMO_60_MIN		(0x1 << 2)
>>> +#define		APCH_CFG_PRETIMO_80_MIN		(0x2 << 2)
>>> +#define		APCH_CFG_PRETIMO_DISABLED	(0x3 << 2)
>>> +#define APCH_CFG_TOTTIMO	(0x03 << 4)
>>> +#define		APCH_CFG_TOTTIMO_3_HOUR		(0x0 << 4)
>>> +#define		APCH_CFG_TOTTIMO_4_HOUR		(0x1 << 4)
>>> +#define		APCH_CFG_TOTTIMO_5_HOUR		(0x2 << 4)
>>> +#define		APCH_CFG_TOTTIMO_DISABLED	(0x3 << 4)
>>> +#define APCH_CFG_SUSCHG		(0x01 << 7)
>>> +
>>> +#define APCH_STATUS_CHGDAT	(0x01 << 0)
>>> +#define APCH_STATUS_INDAT	(0x01 << 1)
>>> +#define APCH_STATUS_TEMPDAT	(0x01 << 2)
>>> +#define APCH_STATUS_TIMRDAT	(0x01 << 3)
>>> +#define APCH_STATUS_CHGSTAT	(0x01 << 4)
>>> +#define APCH_STATUS_INSTAT	(0x01 << 5)
>>> +#define APCH_STATUS_TEMPSTAT	(0x01 << 6)
>>> +#define APCH_STATUS_TIMRSTAT	(0x01 << 7)
>>> +
>>> +#define APCH_CTRL_CHGEOCOUT	(0x01 << 0)
>>> +#define APCH_CTRL_INDIS		(0x01 << 1)
>>> +#define APCH_CTRL_TEMPOUT	(0x01 << 2)
>>> +#define APCH_CTRL_TIMRPRE	(0x01 << 3)
>>> +#define APCH_CTRL_CHGEOCIN	(0x01 << 4)
>>> +#define APCH_CTRL_INCON		(0x01 << 5)
>>> +#define APCH_CTRL_TEMPIN	(0x01 << 6)
>>> +#define APCH_CTRL_TIMRTOT	(0x01 << 7)
>>
>> For the STATUS and CTRL use BIT(n) macro.
>>
>>
>>> +
>>> +#define APCH_STATE_ACINSTAT	(0x01 << 1)
>>> +#define APCH_STATE_CSTATE	(0x03 << 4)
>>> +#define APCH_STATE_CSTATE_SHIFT		4
>>> +#define		APCH_STATE_CSTATE_DISABLED	0x00
>>> +#define		APCH_STATE_CSTATE_EOC		0x01
>>> +#define		APCH_STATE_CSTATE_FAST		0x02
>>> +#define		APCH_STATE_CSTATE_PRE		0x03
>>> +
>>> +struct act8945a_charger {
>>> +	struct act8945a_dev *act8945a_dev;
>>> +	struct power_supply *psy;
>>> +
>>> +	u32 tolal_time_out;
>>> +	u32 pre_time_out;
>>> +	u32 input_voltage_threshold;
>>> +	bool battery_temperature;
>>> +	int chglev_pin;
>>> +	int chglev_value;
>>> +};
>>> +
>>> +static int act8945a_get_charger_state(struct regmap *regmap, int
>>> +*val) {
>>> +	int ret;
>>> +	unsigned int status, state;
>>> +
>>> +	ret = regmap_read(regmap, ACT8945A_APCH_STATUS, &status);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +
>>> +	ret = regmap_read(regmap, ACT8945A_APCH_STATE, &state);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +
>>> +	state &= APCH_STATE_CSTATE;
>>> +	state >>= APCH_STATE_CSTATE_SHIFT;
>>> +
>>> +	if (state == APCH_STATE_CSTATE_EOC) {
>>> +		if (status & APCH_STATUS_CHGDAT)
>>> +			*val = POWER_SUPPLY_STATUS_FULL;
>>> +		else
>>> +			*val = POWER_SUPPLY_STATUS_NOT_CHARGING;
>>> +	} else if ((state == APCH_STATE_CSTATE_FAST) ||
>>> +		   (state == APCH_STATE_CSTATE_PRE)) {
>>> +		*val = POWER_SUPPLY_STATUS_CHARGING;
>>> +	} else {
>>> +		*val = POWER_SUPPLY_STATUS_NOT_CHARGING;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int act8945a_get_charge_type(struct regmap *regmap, int *val)
>>> +{
>>> +	int ret;
>>> +	unsigned int state;
>>> +
>>> +	ret = regmap_read(regmap, ACT8945A_APCH_STATE, &state);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +
>>> +	state &= APCH_STATE_CSTATE;
>>> +	state >>= APCH_STATE_CSTATE_SHIFT;
>>> +
>>> +	switch (state) {
>>> +	case APCH_STATE_CSTATE_PRE:
>>> +		*val = POWER_SUPPLY_CHARGE_TYPE_TRICKLE;
>>> +		break;
>>> +	case APCH_STATE_CSTATE_FAST:
>>> +		*val = POWER_SUPPLY_CHARGE_TYPE_FAST;
>>> +		break;
>>> +	case APCH_STATE_CSTATE_EOC:
>>> +	case APCH_STATE_CSTATE_DISABLED:
>>> +	default:
>>> +		*val = POWER_SUPPLY_CHARGE_TYPE_NONE;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int act8945a_get_battery_health(struct act8945a_charger *charger,
>>> +				       struct regmap *regmap, int *val) {
>>> +	int ret;
>>> +	unsigned int status;
>>> +
>>> +	ret = regmap_read(regmap, ACT8945A_APCH_STATUS, &status);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +
>>> +	if (charger->battery_temperature && !(status &
>> APCH_STATUS_TEMPDAT))
>>> +		*val = POWER_SUPPLY_HEALTH_OVERHEAT;
>>> +	else if (!(status & APCH_STATUS_INDAT))
>>> +		*val = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
>>> +	else if (status & APCH_STATUS_TIMRDAT)
>>> +		*val = POWER_SUPPLY_HEALTH_SAFETY_TIMER_EXPIRE;
>>> +	else
>>> +		*val = POWER_SUPPLY_HEALTH_GOOD;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static enum power_supply_property act8945a_charger_props[] = {
>>> +	POWER_SUPPLY_PROP_STATUS,
>>> +	POWER_SUPPLY_PROP_CHARGE_TYPE,
>>> +	POWER_SUPPLY_PROP_TECHNOLOGY,
>>> +	POWER_SUPPLY_PROP_HEALTH,
>>> +	POWER_SUPPLY_PROP_MODEL_NAME,
>>> +	POWER_SUPPLY_PROP_MANUFACTURER
>>> +};
>>> +
>>> +static int act8945a_charger_get_property(struct power_supply *psy,
>>> +					 enum power_supply_property prop,
>>> +					 union power_supply_propval *val) {
>>> +	struct act8945a_charger *charger = power_supply_get_drvdata(psy);
>>> +	struct regmap *regmap = charger->act8945a_dev->regmap;
>>> +	int ret = 0;
>>> +
>>> +	switch (prop) {
>>> +	case POWER_SUPPLY_PROP_STATUS:
>>> +		ret = act8945a_get_charger_state(regmap, &val->intval);
>>> +		break;
>>> +	case POWER_SUPPLY_PROP_CHARGE_TYPE:
>>> +		ret = act8945a_get_charge_type(regmap, &val->intval);
>>> +		break;
>>> +	case POWER_SUPPLY_PROP_TECHNOLOGY:
>>> +		val->intval = POWER_SUPPLY_TECHNOLOGY_LION;
>>> +		break;
>>> +	case POWER_SUPPLY_PROP_HEALTH:
>>> +		ret = act8945a_get_battery_health(charger,
>>> +						  regmap, &val->intval);
>>> +		break;
>>> +	case POWER_SUPPLY_PROP_MODEL_NAME:
>>> +		val->strval = act8945a_charger_model;
>>> +		break;
>>> +	case POWER_SUPPLY_PROP_MANUFACTURER:
>>> +		val->strval = act8945a_charger_manufacturer;
>>> +		break;
>>> +	default:
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static const struct power_supply_desc act8945a_charger_desc = {
>>> +	.name		= "act8945a-charger",
>>> +	.type		= POWER_SUPPLY_TYPE_BATTERY,
>>> +	.get_property	= act8945a_charger_get_property,
>>> +	.properties	= act8945a_charger_props,
>>> +	.num_properties	= ARRAY_SIZE(act8945a_charger_props),
>>> +};
>>> +
>>> +#define DEFAULT_TOTAL_TIME_OUT		3
>>> +#define DEFAULT_PRE_TIME_OUT		40
>>> +#define DEFAULT_INPUT_OVP_THRESHOLD	6600
>>> +
>>> +static int act8945a_charger_parse_dt(struct device *dev,
>>> +				     struct act8945a_charger *charger) {
>>> +	struct device_node *np = dev->of_node;
>>> +	enum of_gpio_flags flags;
>>> +
>>> +	if (!np) {
>>> +		dev_err(dev, "no charger of node\n");
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	charger->chglev_pin = of_get_named_gpio_flags(np,
>>> +				"active-semi,chglev-gpio", 0, &flags);
>>> +
>>> +	charger->chglev_value = (flags == OF_GPIO_ACTIVE_LOW) ? 0 : 1;
>>> +
>>> +	charger->battery_temperature = of_property_read_bool(np,
>>> +					"active-semi,battery_temperature");
>>> +
>>> +	if (of_property_read_u32(np, "active-semi,input_voltage_threshold",
>>> +				 &charger->input_voltage_threshold))
>>> +		charger->input_voltage_threshold = DEFAULT_PRE_TIME_OUT;
>>> +
>>> +	if (of_property_read_u32(np, "active-semi,precondition_timeout",
>>> +				 &charger->pre_time_out))
>>> +		charger->pre_time_out = DEFAULT_PRE_TIME_OUT;
>>> +
>>> +	if (of_property_read_u32(np, "active-semi,total_timeout",
>>> +				 &charger->tolal_time_out))
>>> +		charger->tolal_time_out = DEFAULT_TOTAL_TIME_OUT;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int act8945a_charger_config(struct act8945a_charger *charger)
>>> +{
>>> +	struct regmap *regmap = charger->act8945a_dev->regmap;
>>> +	u8 value = 0;
>>
>> Regmap uses "unsigned int". I think it is better not to mix it so use in the driver
>> "unsigned int" for interactions with regmap.
> 
> I checked, it seems they are not mixed in the patch. Or I am wrong?

I mean that use "unsigned int value = 0;" here. You're going cast it
anyway to unsigned int when calling regmap functions.

> 
>>
>> [...]
>>
>> Javier pointed the auto-loading issue before (because you are not providing any
>> device table). Probably that is not what you want.
> 
> Yes, but I  still don't understand for now ...

Try building this as module and check whether it got automatically
loaded during boot. :)

Best regards,
Krzysztof

RE: [PATCH v3 1/2] power: act8945a: add charger driver for ACT8945A

[Yang, Wenyou]

Hi Krzysztof,

> -----Original Message-----
> From: Krzysztof Kozlowski [mailto:k.kozlowski@samsung.com]
> Sent: 2016年1月13日 12:10
> To: Yang, Wenyou <Wenyou.Yang@atmel.com>; Sebastian Reichel
> <sre@kernel.org>; Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>; David
> Woodhouse <dwmw2@infradead.org>; Rob Herring <robh+dt@kernel.org>; Pawel
> Moll <pawel.moll@arm.com>; Mark Rutland <mark.rutland@arm.com>; Ian
> Campbell <ijc+devicetree@hellion.org.uk>; Kumar Gala <galak@codeaurora.org>
> Cc: Javier Martinez Canillas <javier@dowhile0.org>; Lee Jones
> <lee.jones@linaro.org>; Ferre, Nicolas <Nicolas.FERRE@atmel.com>; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
> pm@vger.kernel.org
> Subject: Re: [PATCH v3 1/2] power: act8945a: add charger driver for ACT8945A
> 
> On 13.01.2016 12:53, Yang, Wenyou wrote:
> > Hi Krzysztof,
> >
> > Thank you!
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski [mailto:k.kozlowski@samsung.com]
> >> Sent: 2016年1月13日 10:12
> >> To: Yang, Wenyou <Wenyou.Yang@atmel.com>; Sebastian Reichel
> >> <sre@kernel.org>; Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>;
> >> David Woodhouse <dwmw2@infradead.org>; Rob Herring
> >> <robh+dt@kernel.org>; Pawel Moll <pawel.moll@arm.com>; Mark Rutland
> >> <mark.rutland@arm.com>; Ian Campbell <ijc+devicetree@hellion.org.uk>;
> >> Kumar Gala <galak@codeaurora.org>
> >> Cc: Javier Martinez Canillas <javier@dowhile0.org>; Lee Jones
> >> <lee.jones@linaro.org>; Ferre, Nicolas <Nicolas.FERRE@atmel.com>;
> >> linux-arm- kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> >> linux- pm@vger.kernel.org
> >> Subject: Re: [PATCH v3 1/2] power: act8945a: add charger driver for
> >> ACT8945A
> >>
> >> On 12.01.2016 17:09, Wenyou Yang wrote:
> >>> This patch adds new driver for Active-semi ACT8945A ActivePath
> >>> charger (part of ACT8945A MFD driver) providing power supply class
> >>> information to userspace.
> >>>
> >>> The driver is configured through DTS (battery and system related
> >>> settings) and sysfs entries (timers and input over-voltage threshold).
> >>>
> >>> Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> >>> ---
> >>>
> >>> Changes in v3:
> >>>  - update the file header with short version license and author line.
> >>>  - remove unused member of struct act8945a_charger, dev.
> >>>  - action due to removing the member of stuct act8945a_dev, dev.
> >>>  - remove the unnecessary print out.
> >>>  - remove the unnecessary act8945a_charger_remove().
> >>>  - fix align of the code-style.
> >>>
> >>> Changes in v2:
> >>>  1./ Substitute of_property_read_bool() for of_get_property().
> >>>  2./ Substitute devm_power_supply_register() for power_supply_register().
> >>>  3./ Use module_platform_driver(), instead of subsys_initcall().
> >>>  4./ Substitute MODULE_LICENSE("GPL") for MODULE_LICENSE("GPL v2").
> >>>
> >>>  drivers/power/Kconfig            |    7 +
> >>>  drivers/power/Makefile           |    1 +
> >>>  drivers/power/act8945a_charger.c |  367
> >>> ++++++++++++++++++++++++++++++++++++++
> >>>  3 files changed, 375 insertions(+)
> >>>  create mode 100644 drivers/power/act8945a_charger.c
> >>>
> >>> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig index
> >>> 1ddd13c..ae75211 100644
> >>> --- a/drivers/power/Kconfig
> >>> +++ b/drivers/power/Kconfig
> >>> @@ -75,6 +75,13 @@ config BATTERY_88PM860X
> >>>  	help
> >>>  	  Say Y here to enable battery monitor for Marvell 88PM860x chip.
> >>>
> >>> +config BATTERY_ACT8945A
> >>> +	tristate "Active-semi ACT8945A charger driver"
> >>> +	depends on MFD_ACT8945A
> >>> +	help
> >>> +	  Say Y here to enable support for power supply provided by
> >>> +	  Active-semi ActivePath ACT8945A charger.
> >>> +
> >>>  config BATTERY_DS2760
> >>>  	tristate "DS2760 battery driver (HP iPAQ & others)"
> >>>  	depends on W1 && W1_SLAVE_DS2760
> >>> diff --git a/drivers/power/Makefile b/drivers/power/Makefile index
> >>> 0e4eab5..e46b75d 100644
> >>> --- a/drivers/power/Makefile
> >>> +++ b/drivers/power/Makefile
> >>> @@ -17,6 +17,7 @@ obj-$(CONFIG_WM8350_POWER)	+=
> >> wm8350_power.o
> >>>  obj-$(CONFIG_TEST_POWER)	+= test_power.o
> >>>
> >>>  obj-$(CONFIG_BATTERY_88PM860X)	+= 88pm860x_battery.o
> >>> +obj-$(CONFIG_BATTERY_ACT8945A)	+= act8945a_charger.o
> >>>  obj-$(CONFIG_BATTERY_DS2760)	+= ds2760_battery.o
> >>>  obj-$(CONFIG_BATTERY_DS2780)	+= ds2780_battery.o
> >>>  obj-$(CONFIG_BATTERY_DS2781)	+= ds2781_battery.o
> >>> diff --git a/drivers/power/act8945a_charger.c
> >>> b/drivers/power/act8945a_charger.c
> >>> new file mode 100644
> >>> index 0000000..643a2ed
> >>> --- /dev/null
> >>> +++ b/drivers/power/act8945a_charger.c
> >>> @@ -0,0 +1,367 @@
> >>> +/*
> >>> + * Power supply driver for the Active-semi ACT8945A PMIC
> >>> + *
> >>> + * Copyright (C) 2015 Atmel Corporation
> >>> + *
> >>> + * Author: Wenyou Yang <wenyou.yang@atmel.com>
> >>> + *
> >>> + * This program is free software; you can redistribute it and/or
> >>> +modify
> >>> + * it under the terms of the GNU General Public License version 2
> >>> +as
> >>> + * published by the Free Software Foundation.
> >>> + *
> >>> + */
> >>> +#include <linux/module.h>
> >>> +#include <linux/mfd/act8945a.h>
> >>> +#include <linux/of.h>
> >>> +#include <linux/of_gpio.h>
> >>> +#include <linux/platform_device.h>
> >>> +#include <linux/power_supply.h>
> >>> +#include <linux/regmap.h>
> >>> +
> >>> +static const char *act8945a_charger_model = "ACT8945A"; static
> >>> +const char *act8945a_charger_manufacturer = "Active-semi";
> >>> +
> >>> +/**
> >>> + * ACT8945A Charger Register Map
> >>> + */
> >>> +
> >>> +/* 0x70: Reserved */
> >>> +#define ACT8945A_APCH_CFG		0x71
> >>> +#define ACT8945A_APCH_STATUS		0x78
> >>> +#define ACT8945A_APCH_CTRL		0x79
> >>> +#define ACT8945A_APCH_STATE		0x7A
> >>> +
> >>> +/* ACT8945A_APCH_CFG */
> >>> +#define APCH_CFG_OVPSET		(0x03 << 0)
> >>> +#define		APCH_CFG_OVPSET_6V6	(0x0 << 0)
> >>> +#define		APCH_CFG_OVPSET_7V	(0x1 << 0)
> >>> +#define		APCH_CFG_OVPSET_7V5	(0x2 << 0)
> >>> +#define		APCH_CFG_OVPSET_8V	(0x3 << 0)
> >>
> >> Why the tabs after #define? Nooooo...
> >>
> >>> +#define APCH_CFG_PRETIMO	(0x03 << 2)
> >>> +#define		APCH_CFG_PRETIMO_40_MIN		(0x0 << 2)
> >>> +#define		APCH_CFG_PRETIMO_60_MIN		(0x1 << 2)
> >>> +#define		APCH_CFG_PRETIMO_80_MIN		(0x2 << 2)
> >>> +#define		APCH_CFG_PRETIMO_DISABLED	(0x3 << 2)
> >>> +#define APCH_CFG_TOTTIMO	(0x03 << 4)
> >>> +#define		APCH_CFG_TOTTIMO_3_HOUR		(0x0 << 4)
> >>> +#define		APCH_CFG_TOTTIMO_4_HOUR		(0x1 << 4)
> >>> +#define		APCH_CFG_TOTTIMO_5_HOUR		(0x2 << 4)
> >>> +#define		APCH_CFG_TOTTIMO_DISABLED	(0x3 << 4)
> >>> +#define APCH_CFG_SUSCHG		(0x01 << 7)
> >>> +
> >>> +#define APCH_STATUS_CHGDAT	(0x01 << 0)
> >>> +#define APCH_STATUS_INDAT	(0x01 << 1)
> >>> +#define APCH_STATUS_TEMPDAT	(0x01 << 2)
> >>> +#define APCH_STATUS_TIMRDAT	(0x01 << 3)
> >>> +#define APCH_STATUS_CHGSTAT	(0x01 << 4)
> >>> +#define APCH_STATUS_INSTAT	(0x01 << 5)
> >>> +#define APCH_STATUS_TEMPSTAT	(0x01 << 6)
> >>> +#define APCH_STATUS_TIMRSTAT	(0x01 << 7)
> >>> +
> >>> +#define APCH_CTRL_CHGEOCOUT	(0x01 << 0)
> >>> +#define APCH_CTRL_INDIS		(0x01 << 1)
> >>> +#define APCH_CTRL_TEMPOUT	(0x01 << 2)
> >>> +#define APCH_CTRL_TIMRPRE	(0x01 << 3)
> >>> +#define APCH_CTRL_CHGEOCIN	(0x01 << 4)
> >>> +#define APCH_CTRL_INCON		(0x01 << 5)
> >>> +#define APCH_CTRL_TEMPIN	(0x01 << 6)
> >>> +#define APCH_CTRL_TIMRTOT	(0x01 << 7)
> >>
> >> For the STATUS and CTRL use BIT(n) macro.
> >>
> >>
> >>> +
> >>> +#define APCH_STATE_ACINSTAT	(0x01 << 1)
> >>> +#define APCH_STATE_CSTATE	(0x03 << 4)
> >>> +#define APCH_STATE_CSTATE_SHIFT		4
> >>> +#define		APCH_STATE_CSTATE_DISABLED	0x00
> >>> +#define		APCH_STATE_CSTATE_EOC		0x01
> >>> +#define		APCH_STATE_CSTATE_FAST		0x02
> >>> +#define		APCH_STATE_CSTATE_PRE		0x03
> >>> +
> >>> +struct act8945a_charger {
> >>> +	struct act8945a_dev *act8945a_dev;
> >>> +	struct power_supply *psy;
> >>> +
> >>> +	u32 tolal_time_out;
> >>> +	u32 pre_time_out;
> >>> +	u32 input_voltage_threshold;
> >>> +	bool battery_temperature;
> >>> +	int chglev_pin;
> >>> +	int chglev_value;
> >>> +};
> >>> +
> >>> +static int act8945a_get_charger_state(struct regmap *regmap, int
> >>> +*val) {
> >>> +	int ret;
> >>> +	unsigned int status, state;
> >>> +
> >>> +	ret = regmap_read(regmap, ACT8945A_APCH_STATUS, &status);
> >>> +	if (ret < 0)
> >>> +		return ret;
> >>> +
> >>> +	ret = regmap_read(regmap, ACT8945A_APCH_STATE, &state);
> >>> +	if (ret < 0)
> >>> +		return ret;
> >>> +
> >>> +	state &= APCH_STATE_CSTATE;
> >>> +	state >>= APCH_STATE_CSTATE_SHIFT;
> >>> +
> >>> +	if (state == APCH_STATE_CSTATE_EOC) {
> >>> +		if (status & APCH_STATUS_CHGDAT)
> >>> +			*val = POWER_SUPPLY_STATUS_FULL;
> >>> +		else
> >>> +			*val = POWER_SUPPLY_STATUS_NOT_CHARGING;
> >>> +	} else if ((state == APCH_STATE_CSTATE_FAST) ||
> >>> +		   (state == APCH_STATE_CSTATE_PRE)) {
> >>> +		*val = POWER_SUPPLY_STATUS_CHARGING;
> >>> +	} else {
> >>> +		*val = POWER_SUPPLY_STATUS_NOT_CHARGING;
> >>> +	}
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static int act8945a_get_charge_type(struct regmap *regmap, int
> >>> +*val) {
> >>> +	int ret;
> >>> +	unsigned int state;
> >>> +
> >>> +	ret = regmap_read(regmap, ACT8945A_APCH_STATE, &state);
> >>> +	if (ret < 0)
> >>> +		return ret;
> >>> +
> >>> +	state &= APCH_STATE_CSTATE;
> >>> +	state >>= APCH_STATE_CSTATE_SHIFT;
> >>> +
> >>> +	switch (state) {
> >>> +	case APCH_STATE_CSTATE_PRE:
> >>> +		*val = POWER_SUPPLY_CHARGE_TYPE_TRICKLE;
> >>> +		break;
> >>> +	case APCH_STATE_CSTATE_FAST:
> >>> +		*val = POWER_SUPPLY_CHARGE_TYPE_FAST;
> >>> +		break;
> >>> +	case APCH_STATE_CSTATE_EOC:
> >>> +	case APCH_STATE_CSTATE_DISABLED:
> >>> +	default:
> >>> +		*val = POWER_SUPPLY_CHARGE_TYPE_NONE;
> >>> +	}
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static int act8945a_get_battery_health(struct act8945a_charger *charger,
> >>> +				       struct regmap *regmap, int *val) {
> >>> +	int ret;
> >>> +	unsigned int status;
> >>> +
> >>> +	ret = regmap_read(regmap, ACT8945A_APCH_STATUS, &status);
> >>> +	if (ret < 0)
> >>> +		return ret;
> >>> +
> >>> +	if (charger->battery_temperature && !(status &
> >> APCH_STATUS_TEMPDAT))
> >>> +		*val = POWER_SUPPLY_HEALTH_OVERHEAT;
> >>> +	else if (!(status & APCH_STATUS_INDAT))
> >>> +		*val = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
> >>> +	else if (status & APCH_STATUS_TIMRDAT)
> >>> +		*val = POWER_SUPPLY_HEALTH_SAFETY_TIMER_EXPIRE;
> >>> +	else
> >>> +		*val = POWER_SUPPLY_HEALTH_GOOD;
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static enum power_supply_property act8945a_charger_props[] = {
> >>> +	POWER_SUPPLY_PROP_STATUS,
> >>> +	POWER_SUPPLY_PROP_CHARGE_TYPE,
> >>> +	POWER_SUPPLY_PROP_TECHNOLOGY,
> >>> +	POWER_SUPPLY_PROP_HEALTH,
> >>> +	POWER_SUPPLY_PROP_MODEL_NAME,
> >>> +	POWER_SUPPLY_PROP_MANUFACTURER
> >>> +};
> >>> +
> >>> +static int act8945a_charger_get_property(struct power_supply *psy,
> >>> +					 enum power_supply_property prop,
> >>> +					 union power_supply_propval *val) {
> >>> +	struct act8945a_charger *charger = power_supply_get_drvdata(psy);
> >>> +	struct regmap *regmap = charger->act8945a_dev->regmap;
> >>> +	int ret = 0;
> >>> +
> >>> +	switch (prop) {
> >>> +	case POWER_SUPPLY_PROP_STATUS:
> >>> +		ret = act8945a_get_charger_state(regmap, &val->intval);
> >>> +		break;
> >>> +	case POWER_SUPPLY_PROP_CHARGE_TYPE:
> >>> +		ret = act8945a_get_charge_type(regmap, &val->intval);
> >>> +		break;
> >>> +	case POWER_SUPPLY_PROP_TECHNOLOGY:
> >>> +		val->intval = POWER_SUPPLY_TECHNOLOGY_LION;
> >>> +		break;
> >>> +	case POWER_SUPPLY_PROP_HEALTH:
> >>> +		ret = act8945a_get_battery_health(charger,
> >>> +						  regmap, &val->intval);
> >>> +		break;
> >>> +	case POWER_SUPPLY_PROP_MODEL_NAME:
> >>> +		val->strval = act8945a_charger_model;
> >>> +		break;
> >>> +	case POWER_SUPPLY_PROP_MANUFACTURER:
> >>> +		val->strval = act8945a_charger_manufacturer;
> >>> +		break;
> >>> +	default:
> >>> +		return -EINVAL;
> >>> +	}
> >>> +
> >>> +	return ret;
> >>> +}
> >>> +
> >>> +static const struct power_supply_desc act8945a_charger_desc = {
> >>> +	.name		= "act8945a-charger",
> >>> +	.type		= POWER_SUPPLY_TYPE_BATTERY,
> >>> +	.get_property	= act8945a_charger_get_property,
> >>> +	.properties	= act8945a_charger_props,
> >>> +	.num_properties	= ARRAY_SIZE(act8945a_charger_props),
> >>> +};
> >>> +
> >>> +#define DEFAULT_TOTAL_TIME_OUT		3
> >>> +#define DEFAULT_PRE_TIME_OUT		40
> >>> +#define DEFAULT_INPUT_OVP_THRESHOLD	6600
> >>> +
> >>> +static int act8945a_charger_parse_dt(struct device *dev,
> >>> +				     struct act8945a_charger *charger) {
> >>> +	struct device_node *np = dev->of_node;
> >>> +	enum of_gpio_flags flags;
> >>> +
> >>> +	if (!np) {
> >>> +		dev_err(dev, "no charger of node\n");
> >>> +		return -EINVAL;
> >>> +	}
> >>> +
> >>> +	charger->chglev_pin = of_get_named_gpio_flags(np,
> >>> +				"active-semi,chglev-gpio", 0, &flags);
> >>> +
> >>> +	charger->chglev_value = (flags == OF_GPIO_ACTIVE_LOW) ? 0 : 1;
> >>> +
> >>> +	charger->battery_temperature = of_property_read_bool(np,
> >>> +					"active-semi,battery_temperature");
> >>> +
> >>> +	if (of_property_read_u32(np, "active-semi,input_voltage_threshold",
> >>> +				 &charger->input_voltage_threshold))
> >>> +		charger->input_voltage_threshold = DEFAULT_PRE_TIME_OUT;
> >>> +
> >>> +	if (of_property_read_u32(np, "active-semi,precondition_timeout",
> >>> +				 &charger->pre_time_out))
> >>> +		charger->pre_time_out = DEFAULT_PRE_TIME_OUT;
> >>> +
> >>> +	if (of_property_read_u32(np, "active-semi,total_timeout",
> >>> +				 &charger->tolal_time_out))
> >>> +		charger->tolal_time_out = DEFAULT_TOTAL_TIME_OUT;
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static int act8945a_charger_config(struct act8945a_charger
> >>> +*charger) {
> >>> +	struct regmap *regmap = charger->act8945a_dev->regmap;
> >>> +	u8 value = 0;
> >>
> >> Regmap uses "unsigned int". I think it is better not to mix it so use
> >> in the driver "unsigned int" for interactions with regmap.
> >
> > I checked, it seems they are not mixed in the patch. Or I am wrong?
> 
> I mean that use "unsigned int value = 0;" here. You're going cast it anyway to
> unsigned int when calling regmap functions.
> 
> >
> >>
> >> [...]
> >>
> >> Javier pointed the auto-loading issue before (because you are not
> >> providing any device table). Probably that is not what you want.
> >
> > Yes, but I  still don't understand for now ...
> 
> Try building this as module and check whether it got automatically loaded during
> boot. :)

Thank you for your help!

I will try.


Best Regards,
Wenyou Yang

[PATCH v3 2/2] power: add documentation for ACT8945A's charger DT bindings

[Wenyou Yang]

This patch adds documentation for the DT bindings of the charger
subdevice of ACT8945A MFD.

Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
---

Changes in v3: None
Changes in v2: None

 .../devicetree/bindings/power/act8945a-charger.txt |   34 ++++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/act8945a-charger.txt

diff --git a/Documentation/devicetree/bindings/power/act8945a-charger.txt b/Documentation/devicetree/bindings/power/act8945a-charger.txt
new file mode 100644
index 0000000..868e0164
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/act8945a-charger.txt
@@ -0,0 +1,34 @@
+Device-Tree bindings for charger of Active-semi ACT8945A Multi-Function Device
+
+Required properties:
+ - compatible: "active-semi,act8945a-charger"
+ - active-semi,chglev-gpio = Specifies a gpio that uses to select
+   the charge current level.
+
+Optional properties:
+ - active-semi,battery_temperature: Indicates if it is allowed to check
+   the battery temperature. If it is missing, assume the battery temperature
+   is not allowed to check.
+ - active-semi,input_voltage_threshold: unit: mV;
+   Specifies the charger's input over-voltage threshold value;
+   The value can be: 6600, 7000, 7500, 8000; default: 6600
+ - active-semi,precondition_timeout: unit: minutes;
+   Specifies the charger's PRECONDITION safety timer setting value;
+   The value can be: 40, 60, 80, 0; If 0, it means to disable this timer;
+   default: 40.
+ - active-semi,total_timeout: unit: hours;
+   Specifies the charger's total safety timer setting value;
+   The value can be: 3, 4, 5, 0; If 0, it means to disable this timer;
+   default: 3.
+
+Example:
+
+	charger {
+		compatible = "active-semi,act8945a-charger";
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_charger_chglev>;
+		active-semi,chglev-gpio = <&pioA 12 GPIO_ACTIVE_HIGH>;
+		active-semi,input_voltage_threshold = <6600>;
+		active-semi,precondition_timeout = <40>;
+		active-semi,total_timeout = <3>;
+	};
-- 
1.7.9.5

Re: [PATCH v3 2/2] power: add documentation for ACT8945A's charger DT bindings

[Krzysztof Kozlowski]

On 12.01.2016 17:09, Wenyou Yang wrote:
> This patch adds documentation for the DT bindings of the charger
> subdevice of ACT8945A MFD.
> 
> Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> ---
> 
> Changes in v3: None
> Changes in v2: None
> 
>  .../devicetree/bindings/power/act8945a-charger.txt |   34 ++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/act8945a-charger.txt
> 
> diff --git a/Documentation/devicetree/bindings/power/act8945a-charger.txt b/Documentation/devicetree/bindings/power/act8945a-charger.txt
> new file mode 100644
> index 0000000..868e0164
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/act8945a-charger.txt
> @@ -0,0 +1,34 @@
> +Device-Tree bindings for charger of Active-semi ACT8945A Multi-Function Device
> +
> +Required properties:
> + - compatible: "active-semi,act8945a-charger"
> + - active-semi,chglev-gpio = Specifies a gpio that uses to select
> +   the charge current level.

active-semi,chglev-gpios
See: Documentation/devicetree/bindings/gpio/gpio.txt

> +
> +Optional properties:
> + - active-semi,battery_temperature: Indicates if it is allowed to check
> +   the battery temperature. If it is missing, assume the battery temperature
> +   is not allowed to check.

Here and in rest of properties - no underscores but hyphens. The name
suggests that this is a value... but this is a boolean?

> + - active-semi,input_voltage_threshold: unit: mV;
> +   Specifies the charger's input over-voltage threshold value;
> +   The value can be: 6600, 7000, 7500, 8000; default: 6600

I think adding units suffix in binding is preferred. so "-microvolt"?

Best regards,
Krzysztof


> + - active-semi,precondition_timeout: unit: minutes;
> +   Specifies the charger's PRECONDITION safety timer setting value;
> +   The value can be: 40, 60, 80, 0; If 0, it means to disable this timer;
> +   default: 40.
> + - active-semi,total_timeout: unit: hours;
> +   Specifies the charger's total safety timer setting value;
> +   The value can be: 3, 4, 5, 0; If 0, it means to disable this timer;
> +   default: 3.
> +
> +Example:
> +
> +	charger {
> +		compatible = "active-semi,act8945a-charger";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_charger_chglev>;
> +		active-semi,chglev-gpio = <&pioA 12 GPIO_ACTIVE_HIGH>;
> +		active-semi,input_voltage_threshold = <6600>;
> +		active-semi,precondition_timeout = <40>;
> +		active-semi,total_timeout = <3>;
> +	};
> 

RE: [PATCH v3 2/2] power: add documentation for ACT8945A's charger DT bindings

[Yang, Wenyou]

Hi Krzysztof,

Thank you for your so much feedback.

I will change it in next version.

> -----Original Message-----
> From: Krzysztof Kozlowski [mailto:k.kozlowski@samsung.com]
> Sent: 2016年1月13日 10:00
> To: Yang, Wenyou <Wenyou.Yang@atmel.com>; Sebastian Reichel
> <sre@kernel.org>; Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>; David
> Woodhouse <dwmw2@infradead.org>; Rob Herring <robh+dt@kernel.org>; Pawel
> Moll <pawel.moll@arm.com>; Mark Rutland <mark.rutland@arm.com>; Ian
> Campbell <ijc+devicetree@hellion.org.uk>; Kumar Gala <galak@codeaurora.org>
> Cc: Javier Martinez Canillas <javier@dowhile0.org>; Lee Jones
> <lee.jones@linaro.org>; Ferre, Nicolas <Nicolas.FERRE@atmel.com>; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
> pm@vger.kernel.org
> Subject: Re: [PATCH v3 2/2] power: add documentation for ACT8945A's charger
> DT bindings
> 
> On 12.01.2016 17:09, Wenyou Yang wrote:
> > This patch adds documentation for the DT bindings of the charger
> > subdevice of ACT8945A MFD.
> >
> > Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> > ---
> >
> > Changes in v3: None
> > Changes in v2: None
> >
> >  .../devicetree/bindings/power/act8945a-charger.txt |   34
> ++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/power/act8945a-charger.txt
> >
> > diff --git
> > a/Documentation/devicetree/bindings/power/act8945a-charger.txt
> > b/Documentation/devicetree/bindings/power/act8945a-charger.txt
> > new file mode 100644
> > index 0000000..868e0164
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/power/act8945a-charger.txt
> > @@ -0,0 +1,34 @@
> > +Device-Tree bindings for charger of Active-semi ACT8945A
> > +Multi-Function Device
> > +
> > +Required properties:
> > + - compatible: "active-semi,act8945a-charger"
> > + - active-semi,chglev-gpio = Specifies a gpio that uses to select
> > +   the charge current level.
> 
> active-semi,chglev-gpios
> See: Documentation/devicetree/bindings/gpio/gpio.txt
> 
> > +
> > +Optional properties:
> > + - active-semi,battery_temperature: Indicates if it is allowed to check
> > +   the battery temperature. If it is missing, assume the battery temperature
> > +   is not allowed to check.
> 
> Here and in rest of properties - no underscores but hyphens. The name suggests
> that this is a value... but this is a boolean?
> 
> > + - active-semi,input_voltage_threshold: unit: mV;
> > +   Specifies the charger's input over-voltage threshold value;
> > +   The value can be: 6600, 7000, 7500, 8000; default: 6600
> 
> I think adding units suffix in binding is preferred. so "-microvolt"?
> 
> Best regards,
> Krzysztof
> 
> 
> > + - active-semi,precondition_timeout: unit: minutes;
> > +   Specifies the charger's PRECONDITION safety timer setting value;
> > +   The value can be: 40, 60, 80, 0; If 0, it means to disable this timer;
> > +   default: 40.
> > + - active-semi,total_timeout: unit: hours;
> > +   Specifies the charger's total safety timer setting value;
> > +   The value can be: 3, 4, 5, 0; If 0, it means to disable this timer;
> > +   default: 3.
> > +
> > +Example:
> > +
> > +	charger {
> > +		compatible = "active-semi,act8945a-charger";
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&pinctrl_charger_chglev>;
> > +		active-semi,chglev-gpio = <&pioA 12 GPIO_ACTIVE_HIGH>;
> > +		active-semi,input_voltage_threshold = <6600>;
> > +		active-semi,precondition_timeout = <40>;
> > +		active-semi,total_timeout = <3>;
> > +	};
> >


Best Regards,
Wenyou Yang

Re: [PATCH v3 0/2] power: act8945a: add charger driver for the sub-device of ACT8945A MFD

[Krzysztof Kozlowski]

On 12.01.2016 17:09, Wenyou Yang wrote:
> The ACT8945A is a Multi Function Device with the following subdevices:
>  - Regulator
>  - Charger
> 
> This patch set is to add regulator driver for ACT8945A.
> It is based on the patch set:
> 	[PATCH v2] mfd: act8945a: add Active-semi ACT8945A PMIC MFD driver
> 

The useful information would be can be applied independently to other
tree than MFD. From the sentence above I would guess that not (in that
case don't send it as separate patchset)... but from the code I think it
could (because it depends on MFD_ACT8945A).

Anyway please mark it explicitly - it helps the maintainer.

Best regards,
Krzysztof

Re: [PATCH v3 0/2] power: act8945a: add charger driver for the sub-device of ACT8945A MFD

[Krzysztof Kozlowski]

On 13.01.2016 10:48, Krzysztof Kozlowski wrote:
> On 12.01.2016 17:09, Wenyou Yang wrote:
>> The ACT8945A is a Multi Function Device with the following subdevices:
>>  - Regulator
>>  - Charger
>>
>> This patch set is to add regulator driver for ACT8945A.
>> It is based on the patch set:
>> 	[PATCH v2] mfd: act8945a: add Active-semi ACT8945A PMIC MFD driver
>>
> 
> The useful information would be can be applied independently to other
> tree than MFD.

Oh damn, that was far from English. I meant:

The useful information would be whether it can be applied independently
from that patchset.

>From the sentence above I would guess not (in that
case don't send it as separate patchset)... but from the code I think it
could (because it depends on MFD_ACT8945A so no build errors).

Anyway please mark it explicitly - it helps the maintainer to make the
decision.

Best regards,
Krzysztof