[PATCH v5 00/11] power: bq24257: Add support for bq24250/bq24251

[PATCH v5 00/11] power: bq24257: Add support for bq24250/bq24251

[Andreas Dannenberg]

This patch series extends the driver to also support bq24250/bq24251.

The bq24250/251/257 devices have a very similar feature set and are
virtually identical from a control register point of view so it made
sense to extend the existing driver rather than submitting a new driver.
In addition to the new device support the driver is also extended to
allow access to some device features previously hidden. Basic and
potentially dangerous charger config parameters affecting the actual
charging of the Li-Ion battery are only configurable through firmware
rather than sysfs properties. However some newly introduced properties
are exposed through sysfs properties as access to them may be desired
from userspace. For example, it is now possible to manually configure
the maximum current drawn from the input source to accommodate different
chargers (0.5A, 1.5A, 2.0A and so on) based on system knowledge a
userspace application may have rather than rely on the auto-detection
mechanism that may not work in all possible scenarios.

Note that most patches have dependencies on other patches in the series.

v5:
- Added patch to use the managed version of power supply register which
  also fixes a code flow issue that was introduced
- Minor fixes / consistency cleanup (Krzysztof's feedback)

v4:
- Removed configurability of the safety timer multiplier through DT

v3:
- Dropped the driver/symbol rename patch from v2 due to anticipated
  issues with upcoming bq2425x family additions
- Reverted additional mutex coverage for I2C access due to regmap
  built-in mutex protection being sufficient
- Removed support for trickle charging due to being a rare/uncommon
  use case
- Fixed most checkpatch.pl --strict alignment issues (except where
  the line length would exceed 80 chars)
- Fixed an issue with how the return value of gpio_to_desc() was
  handled
- Fixed an issue with the definition of bq24257_of_match[]
- Reordered the patch series to put the DT doc changes to the
  beginning

v2:
- Aligned DT bindings better with existing "ti,*" charger bindings
- Dropped patch that improperly reported a missing battery as a dead
  battery
- Fixed (hopefully, that is -- still waiting for my test platform)
  issue with how the private ACPI driver_data used to identify which
  bq2425x device to use
- Removed boolean DT/ACPI properties mostly by replacing them with more
  intelligent handling in the driver
- Rework/clarification of DT bindings doc
- Renamed/refactored filenames/symbols from bq24257 to bq2425x to
  better reflect that multiple devices are covered. Despite initial
  hesitation I feel this is a good opportunity for some clean-up as
  the driver is still very new in the Kernel so the change should be
  low risk. This also addresses one of Andrew Davis' feedback items.
  Plus, it makes for a nice alignment with the existing bq2415x_charger
  driver.

v1:
- Initial submission

Andreas Dannenberg (11):
  dt: power: bq24257-charger: Cover additional devices
  power: bq24257: Add basic support for bq24250/bq24251
  power: bq24257: Add bit definition for temp sense enable
  power: bq24257: Allow manual setting of input current limit
  power: bq24257: Add SW-based approach for Power Good determination
  power: bq24257: Use managed power supply register
  power: bq24257: Add over voltage protection setting support
  power: bq24257: Add input DPM voltage threshold setting support
  power: bq24257: Allow input current limit sysfs access
  power: bq24257: Add various device-specific sysfs properties
  power: bq24257: Add platform data based initialization

 .../devicetree/bindings/power/bq24257.txt          |  55 +-
 drivers/power/Kconfig                              |   5 +-
 drivers/power/bq24257_charger.c                    | 555 +++++++++++++++++++--
 include/linux/power/bq24257_charger.h              |  29 ++
 4 files changed, 591 insertions(+), 53 deletions(-)
 create mode 100644 include/linux/power/bq24257_charger.h

-- 
1.9.1

[PATCH v5 01/11] dt: power: bq24257-charger: Cover additional devices

[Andreas Dannenberg]

Extend the bq24257 charger's device tree documentation to cover the
bq24250 and bq24251 devices as well feature additions.

Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
---
 .../devicetree/bindings/power/bq24257.txt          | 55 ++++++++++++++++++++--
 1 file changed, 50 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/power/bq24257.txt b/Documentation/devicetree/bindings/power/bq24257.txt
index 5c9d394..3815897 100644
--- a/Documentation/devicetree/bindings/power/bq24257.txt
+++ b/Documentation/devicetree/bindings/power/bq24257.txt
@@ -1,21 +1,66 @@
-Binding for TI bq24257 Li-Ion Charger
+Binding for TI bq24250/bq24251/bq24257 Li-Ion Charger
 
 Required properties:
 - compatible: Should contain one of the following:
+ * "ti,bq24250"
+ * "ti,bq24251"
  * "ti,bq24257"
-- reg:			   integer, i2c address of the device.
+- reg: integer, i2c address of the device.
+- stat-gpios: GPIO used for the devices STAT_IN pin. Alternatively the pin can
+    also be defined through the standard interrupt definition properties (see
+    optional properties section below). Only use one method.
 - ti,battery-regulation-voltage: integer, maximum charging voltage in uV.
-- ti,charge-current:	   integer, maximum charging current in uA.
-- ti,termination-current:  integer, charge will be terminated when current in
-			   constant-voltage phase drops below this value (in uA).
+- ti,charge-current: integer, maximum charging current in uA.
+- ti,termination-current: integer, charge will be terminated when current in
+    constant-voltage phase drops below this value (in uA).
+
+Optional properties:
+- pg-gpios: GPIO used for connecting the bq2425x device PG (Power Good) pin.
+    This pin is not available on all devices however it should be used if
+    possible as this is the recommended way to obtain the charger's input PG
+    state. If this pin is not specified a software-based approach for PG
+    detection is used.
+- ti,current-limit: The maximum current to be drawn from the charger's input
+    (in uA). If this property is not specified a USB D+/D- signal based charger
+    type detection is used (if available) and the input limit current is set
+    accordingly. If the D+/D- based detection is not available on a given device
+    a default of 500,000 is used (=500mA).
+- ti,ovp-voltage: Configures the over voltage protection voltage (in uV). If
+    not specified a default of 6,5000,000 (=6.5V) is used.
+- ti,in-dpm-voltage: Configures the threshold input voltage for the dynamic
+    power path management (in uV). If not specified a default of 4,360,000
+    (=4.36V) is used.
+- interrupt-parent: Should be the phandle for the interrupt controller. Use in
+    conjunction with "interrupts" and only in case "stat-gpios" is not used.
+- interrupts: Interrupt mapping for GPIO IRQ (configure for both edges). Use in
+    conjunction with "interrupt-parent" and only in case "stat-gpios" is not
+    used.
 
 Example:
 
 bq24257 {
 	compatible = "ti,bq24257";
 	reg = <0x6a>;
+	stat-gpios = <&gpio1 16 GPIO_ACTIVE_HIGH>;
+	pg-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>;
 
 	ti,battery-regulation-voltage = <4200000>;
 	ti,charge-current = <1000000>;
 	ti,termination-current = <50000>;
 };
+
+Example:
+
+bq24250 {
+	compatible = "ti,bq24250";
+	reg = <0x6a>;
+	interrupt-parent = <&gpio1>;
+	interrupts = <16 IRQ_TYPE_EDGE_BOTH>;
+
+	ti,battery-regulation-voltage = <4200000>;
+	ti,charge-current = <500000>;
+	ti,termination-current = <50000>;
+	ti,current-limit = <900000>;
+	ti,ovp-voltage = <9500000>;
+	ti,in-dpm-voltage = <4440000>;
+};
-- 
1.9.1

[PATCH v5 02/11] power: bq24257: Add basic support for bq24250/bq24251

[Andreas Dannenberg]

This patch adds basic support for bq24250 and bq24251 which are very
similar to the bq24257 the driver was originally written for. Basic
support means the ability to select a device through Kconfig, DT and
ACPI, an instance variable allowing to check which chip is active, and
the reporting back of the selected device through the
POWER_SUPPLY_PROP_MODEL_NAME power supply sysfs property.

This patch by itself is not sufficient to actually use those two added
devices in a real-world setting due to some feature differences which
are addressed by other patches in this series.

Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
---
 drivers/power/Kconfig           |  5 +++--
 drivers/power/bq24257_charger.c | 50 +++++++++++++++++++++++++++++++++++++++--
 2 files changed, 51 insertions(+), 4 deletions(-)

diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index f8758d6..7ecd9b6 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -397,12 +397,13 @@ config CHARGER_BQ24190
 	  Say Y to enable support for the TI BQ24190 battery charger.
 
 config CHARGER_BQ24257
-	tristate "TI BQ24257 battery charger driver"
+	tristate "TI BQ24250/24251/24257 battery charger driver"
 	depends on I2C
 	depends on GPIOLIB || COMPILE_TEST
 	depends on REGMAP_I2C
 	help
-	  Say Y to enable support for the TI BQ24257 battery charger.
+	  Say Y to enable support for the TI BQ24250, BQ24251, and BQ24257 battery
+	  chargers.
 
 config CHARGER_BQ24735
 	tristate "TI BQ24735 battery charger support"
diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c
index 5859bc7..6e50239 100644
--- a/drivers/power/bq24257_charger.c
+++ b/drivers/power/bq24257_charger.c
@@ -13,6 +13,10 @@
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
+ * Datasheets:
+ * http://www.ti.com/product/bq24250
+ * http://www.ti.com/product/bq24251
+ * http://www.ti.com/product/bq24257
  */
 
 #include <linux/module.h>
@@ -41,6 +45,22 @@
 
 #define BQ24257_ILIM_SET_DELAY		1000	/* msec */
 
+/*
+ * When adding support for new devices make sure that enum bq2425x_chip and
+ * bq2425x_chip_name[] always stay in sync!
+ */
+enum bq2425x_chip {
+	BQ24250,
+	BQ24251,
+	BQ24257,
+};
+
+static const char *const bq2425x_chip_name[] = {
+	"bq24250",
+	"bq24251",
+	"bq24257",
+};
+
 enum bq24257_fields {
 	F_WD_FAULT, F_WD_EN, F_STAT, F_FAULT,			    /* REG 1 */
 	F_RESET, F_IILIMIT, F_EN_STAT, F_EN_TERM, F_CE, F_HZ_MODE,  /* REG 2 */
@@ -71,6 +91,8 @@ struct bq24257_device {
 	struct device *dev;
 	struct power_supply *charger;
 
+	enum bq2425x_chip chip;
+
 	struct regmap *rmap;
 	struct regmap_field *rmap_fields[F_MAX_FIELDS];
 
@@ -249,6 +271,10 @@ static int bq24257_power_supply_get_property(struct power_supply *psy,
 		val->strval = BQ24257_MANUFACTURER;
 		break;
 
+	case POWER_SUPPLY_PROP_MODEL_NAME:
+		val->strval = bq2425x_chip_name[bq->chip];
+		break;
+
 	case POWER_SUPPLY_PROP_ONLINE:
 		val->intval = state.power_good;
 		break;
@@ -569,6 +595,7 @@ static int bq24257_hw_init(struct bq24257_device *bq)
 
 static enum power_supply_property bq24257_power_supply_props[] = {
 	POWER_SUPPLY_PROP_MANUFACTURER,
+	POWER_SUPPLY_PROP_MODEL_NAME,
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_ONLINE,
 	POWER_SUPPLY_PROP_HEALTH,
@@ -666,6 +693,7 @@ static int bq24257_probe(struct i2c_client *client,
 {
 	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
 	struct device *dev = &client->dev;
+	const struct acpi_device_id *acpi_id;
 	struct bq24257_device *bq;
 	int ret;
 	int i;
@@ -682,6 +710,18 @@ static int bq24257_probe(struct i2c_client *client,
 	bq->client = client;
 	bq->dev = dev;
 
+	if (ACPI_HANDLE(dev)) {
+		acpi_id = acpi_match_device(dev->driver->acpi_match_table,
+					    &client->dev);
+		if (!acpi_id) {
+			dev_err(dev, "Failed to match ACPI device\n");
+			return -ENODEV;
+		}
+		bq->chip = (enum bq2425x_chip)acpi_id->driver_data;
+	} else {
+		bq->chip = (enum bq2425x_chip)id->driver_data;
+	}
+
 	mutex_init(&bq->lock);
 
 	bq->rmap = devm_regmap_init_i2c(client, &bq24257_regmap_config);
@@ -823,19 +863,25 @@ static const struct dev_pm_ops bq24257_pm = {
 };
 
 static const struct i2c_device_id bq24257_i2c_ids[] = {
-	{ "bq24257", 0 },
+	{ "bq24250", BQ24250 },
+	{ "bq24251", BQ24251 },
+	{ "bq24257", BQ24257 },
 	{},
 };
 MODULE_DEVICE_TABLE(i2c, bq24257_i2c_ids);
 
 static const struct of_device_id bq24257_of_match[] = {
+	{ .compatible = "ti,bq24250", },
+	{ .compatible = "ti,bq24251", },
 	{ .compatible = "ti,bq24257", },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, bq24257_of_match);
 
 static const struct acpi_device_id bq24257_acpi_match[] = {
-	{"BQ242570", 0},
+	{ "BQ242500", BQ24250 },
+	{ "BQ242510", BQ24251 },
+	{ "BQ242570", BQ24257 },
 	{},
 };
 MODULE_DEVICE_TABLE(acpi, bq24257_acpi_match);
-- 
1.9.1

[PATCH v5 03/11] power: bq24257: Add bit definition for temp sense enable

[Andreas Dannenberg]

Adding a missing bit definition for the sake of consistency device model
vs. bit field representation. No change in functionality.

Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
---
 drivers/power/bq24257_charger.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c
index 6e50239..bd496e5 100644
--- a/drivers/power/bq24257_charger.c
+++ b/drivers/power/bq24257_charger.c
@@ -67,7 +67,7 @@ enum bq24257_fields {
 	F_VBAT, F_USB_DET,					    /* REG 3 */
 	F_ICHG, F_ITERM,					    /* REG 4 */
 	F_LOOP_STATUS, F_LOW_CHG, F_DPDM_EN, F_CE_STATUS, F_VINDPM, /* REG 5 */
-	F_X2_TMR_EN, F_TMR, F_SYSOFF, F_TS_STAT,		    /* REG 6 */
+	F_X2_TMR_EN, F_TMR, F_SYSOFF, F_TS_EN, F_TS_STAT,	    /* REG 6 */
 	F_VOVP, F_CLR_VDP, F_FORCE_BATDET, F_FORCE_PTM,		    /* REG 7 */
 
 	F_MAX_FIELDS
@@ -157,6 +157,7 @@ static const struct reg_field bq24257_reg_fields[] = {
 	[F_X2_TMR_EN]		= REG_FIELD(BQ24257_REG_6, 7, 7),
 	[F_TMR]			= REG_FIELD(BQ24257_REG_6, 5, 6),
 	[F_SYSOFF]		= REG_FIELD(BQ24257_REG_6, 4, 4),
+	[F_TS_EN]		= REG_FIELD(BQ24257_REG_6, 3, 3),
 	[F_TS_STAT]		= REG_FIELD(BQ24257_REG_6, 0, 2),
 	/* REG 7 */
 	[F_VOVP]		= REG_FIELD(BQ24257_REG_7, 5, 7),
-- 
1.9.1

[PATCH v5 04/11] power: bq24257: Allow manual setting of input current limit

[Andreas Dannenberg]

A new optional device property called "ti,current-limit" is introduced
to allow disabling the D+/D- USB signal-based charger type auto-
detection algorithm used to set the input current limit and instead to
use a fixed input current limit.

Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
---
 drivers/power/bq24257_charger.c | 102 +++++++++++++++++++++++++++++++---------
 1 file changed, 81 insertions(+), 21 deletions(-)

diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c
index bd496e5..502dd8a5 100644
--- a/drivers/power/bq24257_charger.c
+++ b/drivers/power/bq24257_charger.c
@@ -78,6 +78,7 @@ struct bq24257_init_data {
 	u8 ichg;	/* charge current      */
 	u8 vbat;	/* regulation voltage  */
 	u8 iterm;	/* termination current */
+	u8 iilimit;	/* input current limit */
 };
 
 struct bq24257_state {
@@ -104,6 +105,8 @@ struct bq24257_device {
 	struct bq24257_state state;
 
 	struct mutex lock; /* protect state data */
+
+	bool iilimit_autoset_enable;
 };
 
 static bool bq24257_is_volatile_reg(struct device *dev, unsigned int reg)
@@ -192,6 +195,12 @@ static const u32 bq24257_iterm_map[] = {
 
 #define BQ24257_ITERM_MAP_SIZE		ARRAY_SIZE(bq24257_iterm_map)
 
+static const u32 bq24257_iilimit_map[] = {
+	100000, 150000, 500000, 900000, 1500000, 2000000
+};
+
+#define BQ24257_IILIMIT_MAP_SIZE	ARRAY_SIZE(bq24257_iilimit_map)
+
 static int bq24257_field_read(struct bq24257_device *bq,
 			      enum bq24257_fields field_id)
 {
@@ -483,24 +492,35 @@ static void bq24257_handle_state_change(struct bq24257_device *bq,
 	old_state = bq->state;
 	mutex_unlock(&bq->lock);
 
-	if (!new_state->power_good) {			     /* power removed */
-		cancel_delayed_work_sync(&bq->iilimit_setup_work);
-
-		/* activate D+/D- port detection algorithm */
-		ret = bq24257_field_write(bq, F_DPDM_EN, 1);
-		if (ret < 0)
-			goto error;
+	/*
+	 * Handle BQ2425x state changes observing whether the D+/D- based input
+	 * current limit autoset functionality is enabled.
+	 */
+	if (!new_state->power_good) {
+		dev_dbg(bq->dev, "Power removed\n");
+		if (bq->iilimit_autoset_enable) {
+			cancel_delayed_work_sync(&bq->iilimit_setup_work);
 
-		reset_iilimit = true;
-	} else if (!old_state.power_good) {		    /* power inserted */
-		config_iilimit = true;
-	} else if (new_state->fault == FAULT_NO_BAT) {	   /* battery removed */
-		cancel_delayed_work_sync(&bq->iilimit_setup_work);
+			/* activate D+/D- port detection algorithm */
+			ret = bq24257_field_write(bq, F_DPDM_EN, 1);
+			if (ret < 0)
+				goto error;
 
-		reset_iilimit = true;
-	} else if (old_state.fault == FAULT_NO_BAT) {    /* battery connected */
-		config_iilimit = true;
-	} else if (new_state->fault == FAULT_TIMER) { /* safety timer expired */
+			reset_iilimit = true;
+		}
+	} else if (!old_state.power_good) {
+		dev_dbg(bq->dev, "Power inserted\n");
+		config_iilimit = bq->iilimit_autoset_enable;
+	} else if (new_state->fault == FAULT_NO_BAT) {
+		dev_warn(bq->dev, "Battery removed\n");
+		if (bq->iilimit_autoset_enable) {
+			cancel_delayed_work_sync(&bq->iilimit_setup_work);
+			reset_iilimit = true;
+		}
+	} else if (old_state.fault == FAULT_NO_BAT) {
+		dev_warn(bq->dev, "Battery connected\n");
+		config_iilimit = bq->iilimit_autoset_enable;
+	} else if (new_state->fault == FAULT_TIMER) {
 		dev_err(bq->dev, "Safety timer expired! Battery dead?\n");
 	}
 
@@ -585,7 +605,16 @@ static int bq24257_hw_init(struct bq24257_device *bq)
 	bq->state = state;
 	mutex_unlock(&bq->lock);
 
-	if (!state.power_good)
+	if (!bq->iilimit_autoset_enable) {
+		dev_dbg(bq->dev, "manually setting iilimit = %u\n",
+			bq->init_data.iilimit);
+
+		/* program fixed input current limit */
+		ret = bq24257_field_write(bq, F_IILIMIT,
+					  bq->init_data.iilimit);
+		if (ret < 0)
+			return ret;
+	} else if (!state.power_good)
 		/* activate D+/D- detection algorithm */
 		ret = bq24257_field_write(bq, F_DPDM_EN, 1);
 	else if (state.fault != FAULT_NO_BAT)
@@ -663,6 +692,7 @@ static int bq24257_fw_probe(struct bq24257_device *bq)
 	int ret;
 	u32 property;
 
+	/* Required properties */
 	ret = device_property_read_u32(bq->dev, "ti,charge-current", &property);
 	if (ret < 0)
 		return ret;
@@ -686,6 +716,24 @@ static int bq24257_fw_probe(struct bq24257_device *bq)
 	bq->init_data.iterm = bq24257_find_idx(property, bq24257_iterm_map,
 					       BQ24257_ITERM_MAP_SIZE);
 
+	/* Optional properties. If not provided use reasonable default. */
+	ret = device_property_read_u32(bq->dev, "ti,current-limit",
+				       &property);
+	if (ret < 0) {
+		bq->iilimit_autoset_enable = true;
+
+		/*
+		 * Explicitly set a default value which will be needed for
+		 * devices that don't support the automatic setting of the input
+		 * current limit through the charger type detection mechanism.
+		 */
+		bq->init_data.iilimit = IILIMIT_500;
+	} else
+		bq->init_data.iilimit =
+				bq24257_find_idx(property,
+						 bq24257_iilimit_map,
+						 BQ24257_IILIMIT_MAP_SIZE);
+
 	return 0;
 }
 
@@ -744,8 +792,6 @@ static int bq24257_probe(struct i2c_client *client,
 
 	i2c_set_clientdata(client, bq);
 
-	INIT_DELAYED_WORK(&bq->iilimit_setup_work, bq24257_iilimit_setup_work);
-
 	if (!dev->platform_data) {
 		ret = bq24257_fw_probe(bq);
 		if (ret < 0) {
@@ -756,6 +802,18 @@ static int bq24257_probe(struct i2c_client *client,
 		return -ENODEV;
 	}
 
+	/*
+	 * The BQ24250 doesn't support the D+/D- based charger type detection
+	 * used for the automatic setting of the input current limit setting so
+	 * explicitly disable that feature.
+	 */
+	if (bq->chip == BQ24250)
+		bq->iilimit_autoset_enable = false;
+
+	if (bq->iilimit_autoset_enable)
+		INIT_DELAYED_WORK(&bq->iilimit_setup_work,
+				  bq24257_iilimit_setup_work);
+
 	/* we can only check Power Good status by probing the PG pin */
 	ret = bq24257_pg_gpio_probe(bq);
 	if (ret < 0)
@@ -808,7 +866,8 @@ static int bq24257_remove(struct i2c_client *client)
 {
 	struct bq24257_device *bq = i2c_get_clientdata(client);
 
-	cancel_delayed_work_sync(&bq->iilimit_setup_work);
+	if (bq->iilimit_autoset_enable)
+		cancel_delayed_work_sync(&bq->iilimit_setup_work);
 
 	power_supply_unregister(bq->charger);
 
@@ -823,7 +882,8 @@ static int bq24257_suspend(struct device *dev)
 	struct bq24257_device *bq = dev_get_drvdata(dev);
 	int ret = 0;
 
-	cancel_delayed_work_sync(&bq->iilimit_setup_work);
+	if (bq->iilimit_autoset_enable)
+		cancel_delayed_work_sync(&bq->iilimit_setup_work);
 
 	/* reset all registers to default (and activate standalone mode) */
 	ret = bq24257_field_write(bq, F_RESET, 1);
-- 
1.9.1

[PATCH v5 05/11] power: bq24257: Add SW-based approach for Power Good determination

[Andreas Dannenberg]

A software-based approach for determining the charger's input voltage
"Power Good" state is introduced for devices like the bq24250 which
don't have a dedicated hardware pin for that purpose. This SW-based
approach is also used for other devices (with dedicated PG pin) as a
fall back solution if that pin is not configured to be used through
"pg-gpios".

Signed-off-by: Andreas Dannenberg <dannenberg-l0cyMroinI0@public.gmane.org>
---
 drivers/power/bq24257_charger.c | 49 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 43 insertions(+), 6 deletions(-)

diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c
index 502dd8a5..135cfd4 100644
--- a/drivers/power/bq24257_charger.c
+++ b/drivers/power/bq24257_charger.c
@@ -107,6 +107,7 @@ struct bq24257_device {
 	struct mutex lock; /* protect state data */
 
 	bool iilimit_autoset_enable;
+	bool pg_gpio_disable;
 };
 
 static bool bq24257_is_volatile_reg(struct device *dev, unsigned int reg)
@@ -360,7 +361,26 @@ static int bq24257_get_chip_state(struct bq24257_device *bq,
 
 	state->fault = ret;
 
-	state->power_good = !gpiod_get_value_cansleep(bq->pg);
+	if (bq->pg_gpio_disable)
+		/*
+		 * If we have a chip without a dedicated power-good GPIO or
+		 * some other explicit bit that would provide this information
+		 * assume the power is good if there is no supply related
+		 * fault - and not good otherwise. There is a possibility for
+		 * other errors to mask that power in fact is not good but this
+		 * is probably the best we can do here.
+		 */
+		switch (state->fault) {
+		case FAULT_INPUT_OVP:
+		case FAULT_INPUT_UVLO:
+		case FAULT_INPUT_LDO_LOW:
+			state->power_good = false;
+			break;
+		default:
+			state->power_good = true;
+		}
+	else
+		state->power_good = !gpiod_get_value_cansleep(bq->pg);
 
 	return 0;
 }
@@ -680,7 +700,7 @@ static int bq24257_pg_gpio_probe(struct bq24257_device *bq)
 {
 	bq->pg = devm_gpiod_get_index(bq->dev, BQ24257_PG_GPIO, 0, GPIOD_IN);
 	if (IS_ERR(bq->pg)) {
-		dev_err(bq->dev, "could not probe PG pin\n");
+		dev_info(bq->dev, "could not probe PG pin\n");
 		return PTR_ERR(bq->pg);
 	}
 
@@ -814,10 +834,27 @@ static int bq24257_probe(struct i2c_client *client,
 		INIT_DELAYED_WORK(&bq->iilimit_setup_work,
 				  bq24257_iilimit_setup_work);
 
-	/* we can only check Power Good status by probing the PG pin */
-	ret = bq24257_pg_gpio_probe(bq);
-	if (ret < 0)
-		return ret;
+	/*
+	 * The BQ24250 doesn't have a dedicated Power Good (PG) pin so we
+	 * explicitly disable this feature for this device and instead use
+	 * a SW-based approach to determine the PG state.
+	 */
+	if (bq->chip == BQ24250)
+		bq->pg_gpio_disable = true;
+
+	/*
+	 * For devices that do have a dedicated PG pin go ahead and probe it,
+	 * using the SW-based approach as a fall back solution. Note that the
+	 * use of the dedicated pin is preferred.
+	 */
+	if (!bq->pg_gpio_disable) {
+		ret = bq24257_pg_gpio_probe(bq);
+		if (ret < 0) {
+			dev_info(bq->dev,
+				 "using SW-based power-good detection\n");
+			bq->pg_gpio_disable = true;
+		}
+	}
 
 	/* reset all registers to defaults */
 	ret = bq24257_field_write(bq, F_RESET, 1);
-- 
1.9.1

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

[PATCH v5 06/11] power: bq24257: Use managed power supply register

[Andreas Dannenberg]

Use the devm_* managed version of the function to register the power
supply and remove the associated unregister function.

Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
---
 drivers/power/bq24257_charger.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c
index 135cfd4..6ef61ae 100644
--- a/drivers/power/bq24257_charger.c
+++ b/drivers/power/bq24257_charger.c
@@ -675,8 +675,10 @@ static int bq24257_power_supply_init(struct bq24257_device *bq)
 	psy_cfg.supplied_to = bq24257_charger_supplied_to;
 	psy_cfg.num_supplicants = ARRAY_SIZE(bq24257_charger_supplied_to);
 
-	bq->charger = power_supply_register(bq->dev, &bq24257_power_supply_desc,
-					    &psy_cfg);
+	bq->charger = devm_power_supply_register(bq->dev,
+						 &bq24257_power_supply_desc,
+						 &psy_cfg);
+
 	if (IS_ERR(bq->charger))
 		return PTR_ERR(bq->charger);
 
@@ -906,8 +908,6 @@ static int bq24257_remove(struct i2c_client *client)
 	if (bq->iilimit_autoset_enable)
 		cancel_delayed_work_sync(&bq->iilimit_setup_work);
 
-	power_supply_unregister(bq->charger);
-
 	bq24257_field_write(bq, F_RESET, 1); /* reset to defaults */
 
 	return 0;
-- 
1.9.1

[PATCH v5 07/11] power: bq24257: Add over voltage protection setting support

[Andreas Dannenberg]

A new optional device property called "ti,ovp-voltage" is introduced to
allow configuring the input over voltage protection setting.

This commit also adds the basic sysfs support for custom properties
which is being used to allow userspace to read the current ovp-voltage
setting.

Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
---
 drivers/power/bq24257_charger.c | 67 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 64 insertions(+), 3 deletions(-)

diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c
index 6ef61ae..8d7077d 100644
--- a/drivers/power/bq24257_charger.c
+++ b/drivers/power/bq24257_charger.c
@@ -79,6 +79,7 @@ struct bq24257_init_data {
 	u8 vbat;	/* regulation voltage  */
 	u8 iterm;	/* termination current */
 	u8 iilimit;	/* input current limit */
+	u8 vovp;	/* over voltage protection voltage */
 };
 
 struct bq24257_state {
@@ -202,6 +203,13 @@ static const u32 bq24257_iilimit_map[] = {
 
 #define BQ24257_IILIMIT_MAP_SIZE	ARRAY_SIZE(bq24257_iilimit_map)
 
+static const u32 bq24257_vovp_map[] = {
+	6000000, 6500000, 7000000, 8000000, 9000000, 9500000, 10000000,
+	10500000
+};
+
+#define BQ24257_VOVP_MAP_SIZE		ARRAY_SIZE(bq24257_vovp_map)
+
 static int bq24257_field_read(struct bq24257_device *bq,
 			      enum bq24257_fields field_id)
 {
@@ -417,6 +425,17 @@ enum bq24257_in_ilimit {
 	IILIMIT_NONE,
 };
 
+enum bq24257_vovp {
+	VOVP_6000,
+	VOVP_6500,
+	VOVP_7000,
+	VOVP_8000,
+	VOVP_9000,
+	VOVP_9500,
+	VOVP_10000,
+	VOVP_10500
+};
+
 enum bq24257_port_type {
 	PORT_TYPE_DCP,		/* Dedicated Charging Port */
 	PORT_TYPE_CDP,		/* Charging Downstream Port */
@@ -598,7 +617,8 @@ static int bq24257_hw_init(struct bq24257_device *bq)
 	} init_data[] = {
 		{F_ICHG, bq->init_data.ichg},
 		{F_VBAT, bq->init_data.vbat},
-		{F_ITERM, bq->init_data.iterm}
+		{F_ITERM, bq->init_data.iterm},
+		{F_VOVP, bq->init_data.vovp},
 	};
 
 	/*
@@ -668,6 +688,28 @@ static const struct power_supply_desc bq24257_power_supply_desc = {
 	.get_property = bq24257_power_supply_get_property,
 };
 
+static ssize_t bq24257_show_ovp_voltage(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	struct power_supply *psy = dev_get_drvdata(dev);
+	struct bq24257_device *bq = power_supply_get_drvdata(psy);
+
+	return scnprintf(buf, PAGE_SIZE, "%u\n",
+			 bq24257_vovp_map[bq->init_data.vovp]);
+}
+
+static DEVICE_ATTR(ovp_voltage, S_IRUGO, bq24257_show_ovp_voltage, NULL);
+
+static struct attribute *bq24257_charger_attr[] = {
+	&dev_attr_ovp_voltage.attr,
+	NULL,
+};
+
+static const struct attribute_group bq24257_attr_group = {
+	.attrs = bq24257_charger_attr,
+};
+
 static int bq24257_power_supply_init(struct bq24257_device *bq)
 {
 	struct power_supply_config psy_cfg = { .drv_data = bq, };
@@ -756,6 +798,15 @@ static int bq24257_fw_probe(struct bq24257_device *bq)
 						 bq24257_iilimit_map,
 						 BQ24257_IILIMIT_MAP_SIZE);
 
+	ret = device_property_read_u32(bq->dev, "ti,ovp-voltage",
+				       &property);
+	if (ret < 0)
+		bq->init_data.vovp = VOVP_6500;
+	else
+		bq->init_data.vovp = bq24257_find_idx(property,
+						      bq24257_vovp_map,
+						      BQ24257_VOVP_MAP_SIZE);
+
 	return 0;
 }
 
@@ -895,10 +946,18 @@ static int bq24257_probe(struct i2c_client *client,
 		return ret;
 
 	ret = bq24257_power_supply_init(bq);
-	if (ret < 0)
+	if (ret < 0) {
 		dev_err(dev, "Failed to register power supply\n");
+		return ret;
+	}
 
-	return ret;
+	ret = sysfs_create_group(&bq->charger->dev.kobj, &bq24257_attr_group);
+	if (ret < 0) {
+		dev_err(dev, "Can't create sysfs entries\n");
+		return ret;
+	}
+
+	return 0;
 }
 
 static int bq24257_remove(struct i2c_client *client)
@@ -908,6 +967,8 @@ static int bq24257_remove(struct i2c_client *client)
 	if (bq->iilimit_autoset_enable)
 		cancel_delayed_work_sync(&bq->iilimit_setup_work);
 
+	sysfs_remove_group(&bq->charger->dev.kobj, &bq24257_attr_group);
+
 	bq24257_field_write(bq, F_RESET, 1); /* reset to defaults */
 
 	return 0;
-- 
1.9.1

[PATCH v5 08/11] power: bq24257: Add input DPM voltage threshold setting support

[Andreas Dannenberg]

A new optional device property called "ti,in-dpm-voltage" is introduced
to allow configuring the input voltage threshold for the devices'
dynamic power path management (DPM) feature. In short, it can be used to
prevent the input voltage from dropping below a certain value as current
is drawn to charge the battery or supply the system.

Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
---
 drivers/power/bq24257_charger.c | 43 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c
index 8d7077d..615624c 100644
--- a/drivers/power/bq24257_charger.c
+++ b/drivers/power/bq24257_charger.c
@@ -80,6 +80,7 @@ struct bq24257_init_data {
 	u8 iterm;	/* termination current */
 	u8 iilimit;	/* input current limit */
 	u8 vovp;	/* over voltage protection voltage */
+	u8 vindpm;	/* VDMP input threshold voltage */
 };
 
 struct bq24257_state {
@@ -210,6 +211,13 @@ static const u32 bq24257_vovp_map[] = {
 
 #define BQ24257_VOVP_MAP_SIZE		ARRAY_SIZE(bq24257_vovp_map)
 
+static const u32 bq24257_vindpm_map[] = {
+	4200000, 4280000, 4360000, 4440000, 4520000, 4600000, 4680000,
+	4760000
+};
+
+#define BQ24257_VINDPM_MAP_SIZE		ARRAY_SIZE(bq24257_vindpm_map)
+
 static int bq24257_field_read(struct bq24257_device *bq,
 			      enum bq24257_fields field_id)
 {
@@ -436,6 +444,17 @@ enum bq24257_vovp {
 	VOVP_10500
 };
 
+enum bq24257_vindpm {
+	VINDPM_4200,
+	VINDPM_4280,
+	VINDPM_4360,
+	VINDPM_4440,
+	VINDPM_4520,
+	VINDPM_4600,
+	VINDPM_4680,
+	VINDPM_4760
+};
+
 enum bq24257_port_type {
 	PORT_TYPE_DCP,		/* Dedicated Charging Port */
 	PORT_TYPE_CDP,		/* Charging Downstream Port */
@@ -619,6 +638,7 @@ static int bq24257_hw_init(struct bq24257_device *bq)
 		{F_VBAT, bq->init_data.vbat},
 		{F_ITERM, bq->init_data.iterm},
 		{F_VOVP, bq->init_data.vovp},
+		{F_VINDPM, bq->init_data.vindpm},
 	};
 
 	/*
@@ -699,10 +719,23 @@ static ssize_t bq24257_show_ovp_voltage(struct device *dev,
 			 bq24257_vovp_map[bq->init_data.vovp]);
 }
 
+static ssize_t bq24257_show_in_dpm_voltage(struct device *dev,
+					   struct device_attribute *attr,
+					   char *buf)
+{
+	struct power_supply *psy = dev_get_drvdata(dev);
+	struct bq24257_device *bq = power_supply_get_drvdata(psy);
+
+	return scnprintf(buf, PAGE_SIZE, "%u\n",
+			 bq24257_vindpm_map[bq->init_data.vindpm]);
+}
+
 static DEVICE_ATTR(ovp_voltage, S_IRUGO, bq24257_show_ovp_voltage, NULL);
+static DEVICE_ATTR(in_dpm_voltage, S_IRUGO, bq24257_show_in_dpm_voltage, NULL);
 
 static struct attribute *bq24257_charger_attr[] = {
 	&dev_attr_ovp_voltage.attr,
+	&dev_attr_in_dpm_voltage.attr,
 	NULL,
 };
 
@@ -807,6 +840,16 @@ static int bq24257_fw_probe(struct bq24257_device *bq)
 						      bq24257_vovp_map,
 						      BQ24257_VOVP_MAP_SIZE);
 
+	ret = device_property_read_u32(bq->dev, "ti,in-dpm-voltage",
+				       &property);
+	if (ret < 0)
+		bq->init_data.vindpm = VINDPM_4360;
+	else
+		bq->init_data.vindpm =
+				bq24257_find_idx(property,
+						 bq24257_vindpm_map,
+						 BQ24257_VINDPM_MAP_SIZE);
+
 	return 0;
 }
 
-- 
1.9.1

[PATCH v5 09/11] power: bq24257: Allow input current limit sysfs access

[Andreas Dannenberg]

This patch allows reading (and writing, if the D+/D- USB signal-based
charger type detection is disabled) of the input current limit through
the power supply's input_current_limit sysfs property. This allows
userspace to see what charger was detected and to re-configure the
maximum current drawn from the external supply at runtime based on
system-level knowledge or user input.

Signed-off-by: Andreas Dannenberg <dannenberg-l0cyMroinI0@public.gmane.org>
---
 drivers/power/bq24257_charger.c | 67 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c
index 615624c..66867d5 100644
--- a/drivers/power/bq24257_charger.c
+++ b/drivers/power/bq24257_charger.c
@@ -269,6 +269,42 @@ enum bq24257_fault {
 	FAULT_INPUT_LDO_LOW,
 };
 
+static int bq24257_get_input_current_limit(struct bq24257_device *bq,
+					   union power_supply_propval *val)
+{
+	int ret;
+
+	ret = bq24257_field_read(bq, F_IILIMIT);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * The "External ILIM" and "Production & Test" modes are not exposed
+	 * through this driver and not being covered by the lookup table.
+	 * Should such a mode have become active let's return an error rather
+	 * than exceeding the bounds of the lookup table and returning
+	 * garbage.
+	 */
+	if (ret >= BQ24257_IILIMIT_MAP_SIZE)
+		return -ENODATA;
+
+	val->intval = bq24257_iilimit_map[ret];
+
+	return 0;
+}
+
+static int bq24257_set_input_current_limit(struct bq24257_device *bq,
+					const union power_supply_propval *val)
+{
+	if (bq->iilimit_autoset_enable)
+		return -EPERM;
+
+	return bq24257_field_write(bq, F_IILIMIT,
+				   bq24257_find_idx(val->intval,
+						    bq24257_iilimit_map,
+						    BQ24257_IILIMIT_MAP_SIZE));
+}
+
 static int bq24257_power_supply_get_property(struct power_supply *psy,
 					     enum power_supply_property psp,
 					     union power_supply_propval *val)
@@ -353,6 +389,9 @@ static int bq24257_power_supply_get_property(struct power_supply *psy,
 		val->intval = bq24257_iterm_map[bq->init_data.iterm];
 		break;
 
+	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
+		return bq24257_get_input_current_limit(bq, val);
+
 	default:
 		return -EINVAL;
 	}
@@ -360,6 +399,31 @@ static int bq24257_power_supply_get_property(struct power_supply *psy,
 	return 0;
 }
 
+static int bq24257_power_supply_set_property(struct power_supply *psy,
+					enum power_supply_property prop,
+					const union power_supply_propval *val)
+{
+	struct bq24257_device *bq = power_supply_get_drvdata(psy);
+
+	switch (prop) {
+	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
+		return bq24257_set_input_current_limit(bq, val);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int bq24257_power_supply_property_is_writeable(struct power_supply *psy,
+					enum power_supply_property psp)
+{
+	switch (psp) {
+	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
+		return true;
+	default:
+		return false;
+	}
+}
+
 static int bq24257_get_chip_state(struct bq24257_device *bq,
 				  struct bq24257_state *state)
 {
@@ -694,6 +758,7 @@ static enum power_supply_property bq24257_power_supply_props[] = {
 	POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE,
 	POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX,
 	POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT,
+	POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
 };
 
 static char *bq24257_charger_supplied_to[] = {
@@ -706,6 +771,8 @@ static const struct power_supply_desc bq24257_power_supply_desc = {
 	.properties = bq24257_power_supply_props,
 	.num_properties = ARRAY_SIZE(bq24257_power_supply_props),
 	.get_property = bq24257_power_supply_get_property,
+	.set_property = bq24257_power_supply_set_property,
+	.property_is_writeable = bq24257_power_supply_property_is_writeable,
 };
 
 static ssize_t bq24257_show_ovp_voltage(struct device *dev,
-- 
1.9.1

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

[PATCH v5 10/11] power: bq24257: Add various device-specific sysfs properties

[Andreas Dannenberg]

This patch adds support for enabling/disabling optional device specific
features through sysfs properties at runtime.

* High-impedance mode enable/disable
* Sysoff enable/disable

Refer to the respective device datasheets for more information:

http://www.ti.com/product/bq24250
http://www.ti.com/product/bq24251
http://www.ti.com/product/bq24257

Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
---
 drivers/power/bq24257_charger.c | 53 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c
index 66867d5..a1068291 100644
--- a/drivers/power/bq24257_charger.c
+++ b/drivers/power/bq24257_charger.c
@@ -797,12 +797,65 @@ static ssize_t bq24257_show_in_dpm_voltage(struct device *dev,
 			 bq24257_vindpm_map[bq->init_data.vindpm]);
 }
 
+static ssize_t bq24257_sysfs_show_enable(struct device *dev,
+					 struct device_attribute *attr,
+					 char *buf)
+{
+	struct power_supply *psy = dev_get_drvdata(dev);
+	struct bq24257_device *bq = power_supply_get_drvdata(psy);
+	int ret;
+
+	if (strcmp(attr->attr.name, "high_impedance_enable") == 0)
+		ret = bq24257_field_read(bq, F_HZ_MODE);
+	else if (strcmp(attr->attr.name, "sysoff_enable") == 0)
+		ret = bq24257_field_read(bq, F_SYSOFF);
+	else
+		return -EINVAL;
+
+	if (ret < 0)
+		return ret;
+
+	return scnprintf(buf, PAGE_SIZE, "%d\n", ret);
+}
+
+static ssize_t bq24257_sysfs_set_enable(struct device *dev,
+					struct device_attribute *attr,
+					const char *buf,
+					size_t count)
+{
+	struct power_supply *psy = dev_get_drvdata(dev);
+	struct bq24257_device *bq = power_supply_get_drvdata(psy);
+	long val;
+	int ret;
+
+	if (kstrtol(buf, 10, &val) < 0)
+		return -EINVAL;
+
+	if (strcmp(attr->attr.name, "high_impedance_enable") == 0)
+		ret = bq24257_field_write(bq, F_HZ_MODE, (bool)val);
+	else if (strcmp(attr->attr.name, "sysoff_enable") == 0)
+		ret = bq24257_field_write(bq, F_SYSOFF, (bool)val);
+	else
+		return -EINVAL;
+
+	if (ret < 0)
+		return ret;
+
+	return count;
+}
+
 static DEVICE_ATTR(ovp_voltage, S_IRUGO, bq24257_show_ovp_voltage, NULL);
 static DEVICE_ATTR(in_dpm_voltage, S_IRUGO, bq24257_show_in_dpm_voltage, NULL);
+static DEVICE_ATTR(high_impedance_enable, S_IWUSR | S_IRUGO,
+		   bq24257_sysfs_show_enable, bq24257_sysfs_set_enable);
+static DEVICE_ATTR(sysoff_enable, S_IWUSR | S_IRUGO,
+		   bq24257_sysfs_show_enable, bq24257_sysfs_set_enable);
 
 static struct attribute *bq24257_charger_attr[] = {
 	&dev_attr_ovp_voltage.attr,
 	&dev_attr_in_dpm_voltage.attr,
+	&dev_attr_high_impedance_enable.attr,
+	&dev_attr_sysoff_enable.attr,
 	NULL,
 };
 
-- 
1.9.1

[PATCH v5 11/11] power: bq24257: Add platform data based initialization

[Andreas Dannenberg]

The patch adds a way to setup and initialize the device through the use
of platform data with configuration options equivalent to when using
device firmware (DT or ACPI) for systems where this is not available.

Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
---
 drivers/power/bq24257_charger.c       | 117 ++++++++++++++++++++++++++++++----
 include/linux/power/bq24257_charger.h |  29 +++++++++
 2 files changed, 135 insertions(+), 11 deletions(-)
 create mode 100644 include/linux/power/bq24257_charger.h

diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c
index a1068291..6cc288c 100644
--- a/drivers/power/bq24257_charger.c
+++ b/drivers/power/bq24257_charger.c
@@ -27,10 +27,13 @@
 #include <linux/gpio/consumer.h>
 #include <linux/interrupt.h>
 #include <linux/delay.h>
+#include <linux/gpio.h>
 
 #include <linux/acpi.h>
 #include <linux/of.h>
 
+#include <linux/power/bq24257_charger.h>
+
 #define BQ24257_REG_1			0x00
 #define BQ24257_REG_2			0x01
 #define BQ24257_REG_3			0x02
@@ -882,28 +885,112 @@ static int bq24257_power_supply_init(struct bq24257_device *bq)
 
 static int bq24257_irq_probe(struct bq24257_device *bq)
 {
+	struct bq24257_platform_data *pdata = bq->client->dev.platform_data;
 	struct gpio_desc *stat_irq;
+	int ret;
+
+	if (!pdata) {
+		stat_irq = devm_gpiod_get_index(bq->dev, BQ24257_STAT_IRQ, 0,
+						GPIOD_IN);
+		if (IS_ERR(stat_irq)) {
+			dev_err(bq->dev, "could not probe stat_irq pin\n");
+			return PTR_ERR(stat_irq);
+		}
+	} else {
+		if (!gpio_is_valid(pdata->stat_gpio)) {
+			dev_err(bq->dev, "invalid stat_irq pin\n");
+			return -EINVAL;
+		}
 
-	stat_irq = devm_gpiod_get_index(bq->dev, BQ24257_STAT_IRQ, 0, GPIOD_IN);
-	if (IS_ERR(stat_irq)) {
-		dev_err(bq->dev, "could not probe stat_irq pin\n");
-		return PTR_ERR(stat_irq);
+		ret = devm_gpio_request_one(bq->dev, pdata->stat_gpio,
+					    GPIOF_IN, BQ24257_STAT_IRQ);
+		if (ret) {
+			dev_err(bq->dev, "stat_irq pin request failed\n");
+			return ret;
+		}
+
+		stat_irq = gpio_to_desc(pdata->stat_gpio);
+		if (!stat_irq) {
+			dev_err(bq->dev,
+				"could not get stat_irq pin descriptor\n");
+			return -EINVAL;
+		}
 	}
 
+	dev_dbg(bq->dev, "probed stat_irq pin = %d", desc_to_gpio(stat_irq));
+
 	return gpiod_to_irq(stat_irq);
 }
 
 static int bq24257_pg_gpio_probe(struct bq24257_device *bq)
 {
-	bq->pg = devm_gpiod_get_index(bq->dev, BQ24257_PG_GPIO, 0, GPIOD_IN);
-	if (IS_ERR(bq->pg)) {
-		dev_info(bq->dev, "could not probe PG pin\n");
-		return PTR_ERR(bq->pg);
+	struct bq24257_platform_data *pdata = bq->client->dev.platform_data;
+	int ret;
+
+	if (!pdata) {
+		bq->pg = devm_gpiod_get_index(bq->dev, BQ24257_PG_GPIO, 0,
+					      GPIOD_IN);
+		if (IS_ERR(bq->pg)) {
+			dev_info(bq->dev, "could not probe PG pin\n");
+			return PTR_ERR(bq->pg);
+		}
+	} else {
+		if (!gpio_is_valid(pdata->pg_gpio)) {
+			dev_info(bq->dev, "invalid PG pin\n");
+			return -EINVAL;
+		}
+
+		ret = devm_gpio_request_one(bq->dev, pdata->pg_gpio,
+					    GPIOF_IN, BQ24257_PG_GPIO);
+		if (ret) {
+			dev_info(bq->dev, "PG pin request failed\n");
+			return ret;
+		}
+
+		bq->pg = gpio_to_desc(pdata->pg_gpio);
+		if (!bq->pg) {
+			dev_info(bq->dev, "could not get PG pin descriptor\n");
+			return -EINVAL;
+		}
 	}
 
+	dev_dbg(bq->dev, "probed PG pin = %d", desc_to_gpio(bq->pg));
+
 	return 0;
 }
 
+static void bq24257_pdata_probe(struct bq24257_device *bq,
+				struct bq24257_platform_data *pdata)
+{
+	bq->init_data.ichg = bq24257_find_idx(pdata->ichg,
+					      bq24257_ichg_map,
+					      BQ24257_ICHG_MAP_SIZE);
+
+	bq->init_data.vbat = bq24257_find_idx(pdata->vbat,
+					      bq24257_vbat_map,
+					      BQ24257_VBAT_MAP_SIZE);
+
+	bq->init_data.iterm = bq24257_find_idx(pdata->iterm,
+					       bq24257_iterm_map,
+					       BQ24257_ITERM_MAP_SIZE);
+
+	bq->init_data.iilimit = bq24257_find_idx(pdata->iilimit,
+						 bq24257_iilimit_map,
+						 BQ24257_IILIMIT_MAP_SIZE);
+
+	bq->init_data.vovp = bq24257_find_idx(pdata->vovp,
+					      bq24257_vovp_map,
+					      BQ24257_VOVP_MAP_SIZE);
+
+	bq->init_data.vindpm = bq24257_find_idx(pdata->vindpm,
+						bq24257_vindpm_map,
+						BQ24257_VINDPM_MAP_SIZE);
+
+	bq->iilimit_autoset_enable = pdata->iilimit_autoset_enable;
+
+	bq->pg_gpio_disable = pdata->pg_gpio_disable;
+}
+
 static int bq24257_fw_probe(struct bq24257_device *bq)
 {
 	int ret;
@@ -979,6 +1066,7 @@ static int bq24257_probe(struct i2c_client *client,
 	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
 	struct device *dev = &client->dev;
 	const struct acpi_device_id *acpi_id;
+	struct bq24257_platform_data *pdata = client->dev.platform_data;
 	struct bq24257_device *bq;
 	int ret;
 	int i;
@@ -1028,14 +1116,15 @@ static int bq24257_probe(struct i2c_client *client,
 
 	i2c_set_clientdata(client, bq);
 
-	if (!dev->platform_data) {
+	if (!pdata) {
 		ret = bq24257_fw_probe(bq);
 		if (ret < 0) {
 			dev_err(dev, "Cannot read device properties.\n");
 			return ret;
 		}
 	} else {
-		return -ENODEV;
+		dev_dbg(dev, "init using platform data\n");
+		bq24257_pdata_probe(bq, pdata);
 	}
 
 	/*
@@ -1092,7 +1181,13 @@ static int bq24257_probe(struct i2c_client *client,
 		return ret;
 	}
 
-	if (client->irq <= 0)
+	/*
+	 * When using device firmware we either take the IRQ specified directly
+	 * or probe for a pin named BQ24257_STAT_IRQ. In case of using platform
+	 * data always run the IRQ pin probe function to finish IRQ and GPIO
+	 * setup based on the stat pin specified in the platform data.
+	 */
+	if (client->irq <= 0 || pdata)
 		client->irq = bq24257_irq_probe(bq);
 
 	if (client->irq < 0) {
diff --git a/include/linux/power/bq24257_charger.h b/include/linux/power/bq24257_charger.h
new file mode 100644
index 0000000..9b4c73e
--- /dev/null
+++ b/include/linux/power/bq24257_charger.h
@@ -0,0 +1,29 @@
+/*
+ * Platform data for the TI bq24257 battery charger driver.
+ *
+ * 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.
+ */
+
+#ifndef _BQ24257_CHARGER_H_
+#define _BQ24257_CHARGER_H_
+
+#include <asm/types.h>
+#include <linux/types.h>
+
+struct bq24257_platform_data {
+	u32 ichg;				       /* charge current (uA) */
+	u32 vbat;				   /* regulation voltage (uV) */
+	u32 iterm;				  /* termination current (uA) */
+	u32 iilimit;				  /* input current limit (uA) */
+	u32 vovp;		      /* over voltage protection voltage (uV) */
+	u32 vindpm;			 /* VDMP input threshold voltage (uV) */
+	bool iilimit_autoset_enable;	/* auto-detect of input current limit */
+	bool pg_gpio_disable;	     /* use of a dedicated pin for Power Good */
+
+	int stat_gpio;				  /* stat_int (interrupt) pin */
+	int pg_gpio;					    /* power good pin */
+};
+
+#endif /* _BQ24257_CHARGER_H_ */
-- 
1.9.1

Re: [PATCH v5 01/11] dt: power: bq24257-charger: Cover additional devices

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 519 bytes --]

Hi,

On Fri, Sep 18, 2015 at 04:39:49PM -0500, Andreas Dannenberg wrote:
> Extend the bq24257 charger's device tree documentation to cover the
> bq24250 and bq24251 devices as well feature additions.

The binding looks fine to except for:

> +- stat-gpios: GPIO used for the devices STAT_IN pin. Alternatively the pin can
> +    also be defined through the standard interrupt definition properties (see
> +    optional properties section below). Only use one method.

Why do you expose two ways for this?

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v5 09/11] power: bq24257: Allow input current limit sysfs access

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 615 bytes --]

Hi,

On Fri, Sep 18, 2015 at 04:39:57PM -0500, Andreas Dannenberg wrote:
> This patch allows reading (and writing, if the D+/D- USB signal-based
> charger type detection is disabled) of the input current limit through
> the power supply's input_current_limit sysfs property. This allows
> userspace to see what charger was detected and to re-configure the
> maximum current drawn from the external supply at runtime based on
> system-level knowledge or user input.

Maybe also support writing into input_current_limit in auto mode.
Just disable auto detection until "auto" is written into sysfs node.

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v5 11/11] power: bq24257: Add platform data based initialization

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1102 bytes --]

Hi,

On Fri, Sep 18, 2015 at 04:39:59PM -0500, Andreas Dannenberg wrote:
> The patch adds a way to setup and initialize the device through the use
> of platform data with configuration options equivalent to when using
> device firmware (DT or ACPI) for systems where this is not available.
> ---
>  drivers/power/bq24257_charger.c       | 117 ++++++++++++++++++++++++++++++----
>  include/linux/power/bq24257_charger.h |  29 +++++++++
>  2 files changed, 135 insertions(+), 11 deletions(-)
>  create mode 100644 include/linux/power/bq24257_charger.h
> 
> diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c
> index a1068291..6cc288c 100644
> --- a/drivers/power/bq24257_charger.c
> +++ b/drivers/power/bq24257_charger.c
> @@ -27,10 +27,13 @@
>  #include <linux/gpio/consumer.h>
>  #include <linux/interrupt.h>
>  #include <linux/delay.h>
> +#include <linux/gpio.h>

gpiod interface is also supported for platform devices. Just request
it the same way as for DT. Read section "Platform Data" in this
file: Documentation/gpio/board.txt

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v5 00/11] power: bq24257: Add support for bq24250/bq24251

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1315 bytes --]

Hi,

On Fri, Sep 18, 2015 at 04:39:48PM -0500, Andreas Dannenberg wrote:
> This patch series extends the driver to also support bq24250/bq24251.
> 
> The bq24250/251/257 devices have a very similar feature set and are
> virtually identical from a control register point of view so it made
> sense to extend the existing driver rather than submitting a new driver.
> In addition to the new device support the driver is also extended to
> allow access to some device features previously hidden. Basic and
> potentially dangerous charger config parameters affecting the actual
> charging of the Li-Ion battery are only configurable through firmware
> rather than sysfs properties. However some newly introduced properties
> are exposed through sysfs properties as access to them may be desired
> from userspace. For example, it is now possible to manually configure
> the maximum current drawn from the input source to accommodate different
> chargers (0.5A, 1.5A, 2.0A and so on) based on system knowledge a
> userspace application may have rather than rely on the auto-detection
> mechanism that may not work in all possible scenarios.
> 
> Note that most patches have dependencies on other patches in the series.

Please document the custom sysfs files in Documentation/ABI/.

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v5 05/11] power: bq24257: Add SW-based approach for Power Good determination

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 3705 bytes --]

Hi,

On Fri, Sep 18, 2015 at 04:39:53PM -0500, Andreas Dannenberg wrote:
> A software-based approach for determining the charger's input voltage
> "Power Good" state is introduced for devices like the bq24250 which
> don't have a dedicated hardware pin for that purpose. This SW-based
> approach is also used for other devices (with dedicated PG pin) as a
> fall back solution if that pin is not configured to be used through
> "pg-gpios".
> 
> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> ---
>  drivers/power/bq24257_charger.c | 49 ++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 43 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c
> index 502dd8a5..135cfd4 100644
> --- a/drivers/power/bq24257_charger.c
> +++ b/drivers/power/bq24257_charger.c
> @@ -107,6 +107,7 @@ struct bq24257_device {
>  	struct mutex lock; /* protect state data */
>  
>  	bool iilimit_autoset_enable;
> +	bool pg_gpio_disable;

I don't think this is required.

>  };
>  
>  static bool bq24257_is_volatile_reg(struct device *dev, unsigned int reg)
> @@ -360,7 +361,26 @@ static int bq24257_get_chip_state(struct bq24257_device *bq,
>  
>  	state->fault = ret;
>  
> -	state->power_good = !gpiod_get_value_cansleep(bq->pg);
> +	if (bq->pg_gpio_disable)
> +		/*
> +		 * If we have a chip without a dedicated power-good GPIO or
> +		 * some other explicit bit that would provide this information
> +		 * assume the power is good if there is no supply related
> +		 * fault - and not good otherwise. There is a possibility for
> +		 * other errors to mask that power in fact is not good but this
> +		 * is probably the best we can do here.
> +		 */
> +		switch (state->fault) {
> +		case FAULT_INPUT_OVP:
> +		case FAULT_INPUT_UVLO:
> +		case FAULT_INPUT_LDO_LOW:
> +			state->power_good = false;
> +			break;
> +		default:
> +			state->power_good = true;
> +		}
> +	else
> +		state->power_good = !gpiod_get_value_cansleep(bq->pg);

I guess you can just handle this like an optional gpio

if(bq->pg)
    state->power_good = !gpiod_get_value_cansleep(bq->pg);
else
    ...

>  
>  	return 0;
>  }
> @@ -680,7 +700,7 @@ static int bq24257_pg_gpio_probe(struct bq24257_device *bq)
>  {
>  	bq->pg = devm_gpiod_get_index(bq->dev, BQ24257_PG_GPIO, 0, GPIOD_IN);
>  	if (IS_ERR(bq->pg)) {
> -		dev_err(bq->dev, "could not probe PG pin\n");
> +		dev_info(bq->dev, "could not probe PG pin\n");
>  		return PTR_ERR(bq->pg);
>  	}
>  
> @@ -814,10 +834,27 @@ static int bq24257_probe(struct i2c_client *client,
>  		INIT_DELAYED_WORK(&bq->iilimit_setup_work,
>  				  bq24257_iilimit_setup_work);
>  
> -	/* we can only check Power Good status by probing the PG pin */
> -	ret = bq24257_pg_gpio_probe(bq);
> -	if (ret < 0)
> -		return ret;
> +	/*
> +	 * The BQ24250 doesn't have a dedicated Power Good (PG) pin so we
> +	 * explicitly disable this feature for this device and instead use
> +	 * a SW-based approach to determine the PG state.
> +	 */
> +	if (bq->chip == BQ24250)
> +		bq->pg_gpio_disable = true;
> +
> +	/*
> +	 * For devices that do have a dedicated PG pin go ahead and probe it,
> +	 * using the SW-based approach as a fall back solution. Note that the
> +	 * use of the dedicated pin is preferred.
> +	 */
> +	if (!bq->pg_gpio_disable) {
> +		ret = bq24257_pg_gpio_probe(bq);
> +		if (ret < 0) {
> +			dev_info(bq->dev,
> +				 "using SW-based power-good detection\n");
> +			bq->pg_gpio_disable = true;
> +		}
> +	}
>  
>  	/* reset all registers to defaults */
>  	ret = bq24257_field_write(bq, F_RESET, 1);

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v5 01/11] dt: power: bq24257-charger: Cover additional devices

[Andreas Dannenberg]

On Tue, Sep 22, 2015 at 06:24:48PM +0200, Sebastian Reichel wrote:
> Hi,
> 
> On Fri, Sep 18, 2015 at 04:39:49PM -0500, Andreas Dannenberg wrote:
> > Extend the bq24257 charger's device tree documentation to cover the
> > bq24250 and bq24251 devices as well feature additions.
> 
> The binding looks fine to except for:
> 
> > +- stat-gpios: GPIO used for the devices STAT_IN pin. Alternatively the pin can
> > +    also be defined through the standard interrupt definition properties (see
> > +    optional properties section below). Only use one method.
> 
> Why do you expose two ways for this?

Hi Sebastian. The original driver exposed this - it just didn't
document it in the DT binding doc. I'm not sure why this was introduced
in the first place (Laurentiu?). I know we can't change the API but
since it was never documented maybe we can remove it?

Regards,

--
Andreas Dannenberg
Texas Instruments Inc


> 
> -- Sebastian

Re: [PATCH v5 09/11] power: bq24257: Allow input current limit sysfs access

[Andreas Dannenberg]

On Tue, Sep 22, 2015 at 09:16:49PM +0200, Sebastian Reichel wrote:
> Hi,
> 
> On Fri, Sep 18, 2015 at 04:39:57PM -0500, Andreas Dannenberg wrote:
> > This patch allows reading (and writing, if the D+/D- USB signal-based
> > charger type detection is disabled) of the input current limit through
> > the power supply's input_current_limit sysfs property. This allows
> > userspace to see what charger was detected and to re-configure the
> > maximum current drawn from the external supply at runtime based on
> > system-level knowledge or user input.
> 
> Maybe also support writing into input_current_limit in auto mode.
> Just disable auto detection until "auto" is written into sysfs node.

Auto-detection was enabled by default in the original driver so I think
that should be left intact. I added the ability to manually override
this via DT with a fixed value, and then configure said fixed value
through sysfs at runtime.

I'm not 100% clear on the usecase of runtime enabling/disabling auto so
I'd rather leave the implementation as-is. Either auto mode is enabled
or not -- and this is directly tied to the DT setting. But if someone
has a strong usecase for this I can certainly add it.

Regards,

--
Andreas Dannenberg
Texas Instruments Inc

> 
> -- Sebastian

Re: [PATCH v5 09/11] power: bq24257: Allow input current limit sysfs access

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1539 bytes --]

On Tue, Sep 22, 2015 at 05:10:45PM -0500, Andreas Dannenberg wrote:
> On Tue, Sep 22, 2015 at 09:16:49PM +0200, Sebastian Reichel wrote:
> > On Fri, Sep 18, 2015 at 04:39:57PM -0500, Andreas Dannenberg wrote:
> > > This patch allows reading (and writing, if the D+/D- USB signal-based
> > > charger type detection is disabled) of the input current limit through
> > > the power supply's input_current_limit sysfs property. This allows
> > > userspace to see what charger was detected and to re-configure the
> > > maximum current drawn from the external supply at runtime based on
> > > system-level knowledge or user input.
> > 
> > Maybe also support writing into input_current_limit in auto mode.
> > Just disable auto detection until "auto" is written into sysfs node.
> 
> Auto-detection was enabled by default in the original driver so I think
> that should be left intact. I added the ability to manually override
> this via DT with a fixed value, and then configure said fixed value
> through sysfs at runtime.
> 
> I'm not 100% clear on the usecase of runtime enabling/disabling auto so
> I'd rather leave the implementation as-is. Either auto mode is enabled
> or not -- and this is directly tied to the DT setting. But if someone
> has a strong usecase for this I can certainly add it.

For some usb power supplies auto-detection doesn't work very well,
resulting in a 100mA default fallback. Users knowing their hardware
could force charging with the correct input current limitation.

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v5 01/11] dt: power: bq24257-charger: Cover additional devices

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1120 bytes --]

Hi,

On Tue, Sep 22, 2015 at 04:58:24PM -0500, Andreas Dannenberg wrote:
> On Tue, Sep 22, 2015 at 06:24:48PM +0200, Sebastian Reichel wrote:
> > On Fri, Sep 18, 2015 at 04:39:49PM -0500, Andreas Dannenberg wrote:
> > > Extend the bq24257 charger's device tree documentation to cover the
> > > bq24250 and bq24251 devices as well feature additions.
> > 
> > The binding looks fine to except for:
> > 
> > > +- stat-gpios: GPIO used for the devices STAT_IN pin. Alternatively the pin can
> > > +    also be defined through the standard interrupt definition properties (see
> > > +    optional properties section below). Only use one method.
> > 
> > Why do you expose two ways for this?
> 
> Hi Sebastian. The original driver exposed this - it just didn't
> document it in the DT binding doc. I'm not sure why this was introduced
> in the first place (Laurentiu?). I know we can't change the API but
> since it was never documented maybe we can remove it?

It seems this is neither documented, nor used. So I guess it can be
removed. Let's wait for feedback from Laurentiu, though.

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v5 01/11] dt: power: bq24257-charger: Cover additional devices

[Laurentiu Palcu]

Hi,

On Wed, Sep 23, 2015 at 02:34:15AM +0200, Sebastian Reichel wrote:
> Hi,
> 
> On Tue, Sep 22, 2015 at 04:58:24PM -0500, Andreas Dannenberg wrote:
> > On Tue, Sep 22, 2015 at 06:24:48PM +0200, Sebastian Reichel wrote:
> > > On Fri, Sep 18, 2015 at 04:39:49PM -0500, Andreas Dannenberg wrote:
> > > > Extend the bq24257 charger's device tree documentation to cover the
> > > > bq24250 and bq24251 devices as well feature additions.
> > > 
> > > The binding looks fine to except for:
> > > 
> > > > +- stat-gpios: GPIO used for the devices STAT_IN pin. Alternatively the pin can
> > > > +    also be defined through the standard interrupt definition properties (see
> > > > +    optional properties section below). Only use one method.
> > > 
> > > Why do you expose two ways for this?
> > 
> > Hi Sebastian. The original driver exposed this - it just didn't
> > document it in the DT binding doc. I'm not sure why this was introduced
> > in the first place (Laurentiu?). I know we can't change the API but
> > since it was never documented maybe we can remove it?
> 
> It seems this is neither documented, nor used. So I guess it can be
> removed. Let's wait for feedback from Laurentiu, though.
This was needed to have both ACPI and DT enumeration work. At the time I
wrote the driver, GpioInt resources in ACPI were not passed to the
driver in client->irq, as opposed to DT enumeration.

However, conveniently enough, the patch below landed in master by the
same time bq24257 driver went in. So, I believe it should be safe now to
remove the stat-gpio probing.

commit 845c877009cf014b971aab7f54613f9185a824b0
Author: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Date:   Wed May 6 13:29:08 2015 +0300

    i2c / ACPI: Assign IRQ for devices that have GpioInt automatically

laurentiu
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 09/11] power: bq24257: Allow input current limit sysfs access

[Andreas Dannenberg]

On Wed, Sep 23, 2015 at 02:29:06AM +0200, Sebastian Reichel wrote:
> On Tue, Sep 22, 2015 at 05:10:45PM -0500, Andreas Dannenberg wrote:
> > On Tue, Sep 22, 2015 at 09:16:49PM +0200, Sebastian Reichel wrote:
> > > On Fri, Sep 18, 2015 at 04:39:57PM -0500, Andreas Dannenberg wrote:
> > > > This patch allows reading (and writing, if the D+/D- USB signal-based
> > > > charger type detection is disabled) of the input current limit through
> > > > the power supply's input_current_limit sysfs property. This allows
> > > > userspace to see what charger was detected and to re-configure the
> > > > maximum current drawn from the external supply at runtime based on
> > > > system-level knowledge or user input.
> > > 
> > > Maybe also support writing into input_current_limit in auto mode.
> > > Just disable auto detection until "auto" is written into sysfs node.
> > 
> > Auto-detection was enabled by default in the original driver so I think
> > that should be left intact. I added the ability to manually override
> > this via DT with a fixed value, and then configure said fixed value
> > through sysfs at runtime.
> > 
> > I'm not 100% clear on the usecase of runtime enabling/disabling auto so
> > I'd rather leave the implementation as-is. Either auto mode is enabled
> > or not -- and this is directly tied to the DT setting. But if someone
> > has a strong usecase for this I can certainly add it.
> 
> For some usb power supplies auto-detection doesn't work very well,
> resulting in a 100mA default fallback. Users knowing their hardware
> could force charging with the correct input current limitation.

Ok. So how should we best go about extending the usage of the
'input_current_limit' sysfs node for this charger? You mentioned
writing 'auto' into it should enable the auto-detection mode. I suppose
writing a fixed current value will disable it. But how to indicate to
the user when reading 'input_current_limit' whether auto mode is enabled
or not (I think this is something we should do). Can we return a mixed
number/string like this?

# Example: charger auto detection mode is disabled, and input current
# limit is configured as 500mA.

$ cat input_current_limit
500000

# Example: charger auto detection mode is enabled, and a charger
# supporting 1A was detected (note the mixed number/string thats
# returned)

$ cat input_current_limit
1000000 (auto)

Would that work? Or should we introduce a new sysfs property?

--
Andreas Dannenberg
Texas Instruments Inc

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

Re: [PATCH v5 01/11] dt: power: bq24257-charger: Cover additional devices

[Andreas Dannenberg]

On Wed, Sep 23, 2015 at 11:14:56AM +0300, Laurentiu Palcu wrote:
> Hi,
> 
> On Wed, Sep 23, 2015 at 02:34:15AM +0200, Sebastian Reichel wrote:
> > Hi,
> > 
> > On Tue, Sep 22, 2015 at 04:58:24PM -0500, Andreas Dannenberg wrote:
> > > On Tue, Sep 22, 2015 at 06:24:48PM +0200, Sebastian Reichel wrote:
> > > > On Fri, Sep 18, 2015 at 04:39:49PM -0500, Andreas Dannenberg wrote:
> > > > > Extend the bq24257 charger's device tree documentation to cover the
> > > > > bq24250 and bq24251 devices as well feature additions.
> > > > 
> > > > The binding looks fine to except for:
> > > > 
> > > > > +- stat-gpios: GPIO used for the devices STAT_IN pin. Alternatively the pin can
> > > > > +    also be defined through the standard interrupt definition properties (see
> > > > > +    optional properties section below). Only use one method.
> > > > 
> > > > Why do you expose two ways for this?
> > > 
> > > Hi Sebastian. The original driver exposed this - it just didn't
> > > document it in the DT binding doc. I'm not sure why this was introduced
> > > in the first place (Laurentiu?). I know we can't change the API but
> > > since it was never documented maybe we can remove it?
> > 
> > It seems this is neither documented, nor used. So I guess it can be
> > removed. Let's wait for feedback from Laurentiu, though.
> This was needed to have both ACPI and DT enumeration work. At the time I
> wrote the driver, GpioInt resources in ACPI were not passed to the
> driver in client->irq, as opposed to DT enumeration.
> 
> However, conveniently enough, the patch below landed in master by the
> same time bq24257 driver went in. So, I believe it should be safe now to
> remove the stat-gpio probing.
> 
> commit 845c877009cf014b971aab7f54613f9185a824b0
> Author: Mika Westerberg <mika.westerberg@linux.intel.com>
> Date:   Wed May 6 13:29:08 2015 +0300
> 
>     i2c / ACPI: Assign IRQ for devices that have GpioInt automatically
> 

Thanks for the background! I'll be happy to simplify this as part of my
patch series.

--
Andreas Dannenberg
Texas Instruments Inc

Re: [PATCH v5 09/11] power: bq24257: Allow input current limit sysfs access

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 3050 bytes --]

Hi,

On Wed, Sep 23, 2015 at 09:11:46AM -0500, Andreas Dannenberg wrote:
> On Wed, Sep 23, 2015 at 02:29:06AM +0200, Sebastian Reichel wrote:
> > On Tue, Sep 22, 2015 at 05:10:45PM -0500, Andreas Dannenberg wrote:
> > > On Tue, Sep 22, 2015 at 09:16:49PM +0200, Sebastian Reichel wrote:
> > > > On Fri, Sep 18, 2015 at 04:39:57PM -0500, Andreas Dannenberg wrote:
> > > > > This patch allows reading (and writing, if the D+/D- USB signal-based
> > > > > charger type detection is disabled) of the input current limit through
> > > > > the power supply's input_current_limit sysfs property. This allows
> > > > > userspace to see what charger was detected and to re-configure the
> > > > > maximum current drawn from the external supply at runtime based on
> > > > > system-level knowledge or user input.
> > > > 
> > > > Maybe also support writing into input_current_limit in auto mode.
> > > > Just disable auto detection until "auto" is written into sysfs node.
> > > 
> > > Auto-detection was enabled by default in the original driver so I think
> > > that should be left intact. I added the ability to manually override
> > > this via DT with a fixed value, and then configure said fixed value
> > > through sysfs at runtime.
> > > 
> > > I'm not 100% clear on the usecase of runtime enabling/disabling auto so
> > > I'd rather leave the implementation as-is. Either auto mode is enabled
> > > or not -- and this is directly tied to the DT setting. But if someone
> > > has a strong usecase for this I can certainly add it.
> > 
> > For some usb power supplies auto-detection doesn't work very well,
> > resulting in a 100mA default fallback. Users knowing their hardware
> > could force charging with the correct input current limitation.
> 
> Ok. So how should we best go about extending the usage of the
> 'input_current_limit' sysfs node for this charger? You mentioned
> writing 'auto' into it should enable the auto-detection mode. I suppose
> writing a fixed current value will disable it. But how to indicate to
> the user when reading 'input_current_limit' whether auto mode is enabled
> or not (I think this is something we should do).

Right, I haven't thought of this.

> Can we return a mixed number/string like this?
> 
> # Example: charger auto detection mode is disabled, and input current
> # limit is configured as 500mA.
> 
> $ cat input_current_limit
> 500000
> 
> # Example: charger auto detection mode is enabled, and a charger
> # supporting 1A was detected (note the mixed number/string thats
> # returned)
> 
> $ cat input_current_limit
> 1000000 (auto)
> 
> Would that work?

No. sysfs nodes should only contain one value per file.

> Or should we introduce a new sysfs property?

So maybe keep this patch as is (disallow setting the limit in auto
mode) and create another file for setting (and getting) the mode.

Also it might be a good idea to return to safe defaults when the
charger is disconnected (-> reset to DT specified values).

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v5 09/11] power: bq24257: Allow input current limit sysfs access

[Andreas Dannenberg]

On Wed, Sep 23, 2015 at 05:02:28PM +0200, Sebastian Reichel wrote:
> Hi,
> 
> On Wed, Sep 23, 2015 at 09:11:46AM -0500, Andreas Dannenberg wrote:
> > Ok. So how should we best go about extending the usage of the
> > 'input_current_limit' sysfs node for this charger? You mentioned
> > writing 'auto' into it should enable the auto-detection mode. I suppose
> > writing a fixed current value will disable it. But how to indicate to
> > the user when reading 'input_current_limit' whether auto mode is enabled
> > or not (I think this is something we should do).
> 
> Right, I haven't thought of this.
> 
> > Can we return a mixed number/string like this?
> > 
> > # Example: charger auto detection mode is disabled, and input current
> > # limit is configured as 500mA.
> > 
> > $ cat input_current_limit
> > 500000
> > 
> > # Example: charger auto detection mode is enabled, and a charger
> > # supporting 1A was detected (note the mixed number/string thats
> > # returned)
> > 
> > $ cat input_current_limit
> > 1000000 (auto)
> > 
> > Would that work?
> 
> No. sysfs nodes should only contain one value per file.
> 
> > Or should we introduce a new sysfs property?
> 
> So maybe keep this patch as is (disallow setting the limit in auto
> mode) and create another file for setting (and getting) the mode.

Thanks for the continued feedback. Will look into this and add a new
property.
 
> Also it might be a good idea to return to safe defaults when the
> charger is disconnected (-> reset to DT specified values).

It already does this when the charger type auto-detection is enabled.
When configured for manually setting the input current limit the use
case is a bit more complex and I do not recommend resetting the limit to
500mA. Let me explain why:

1) Using USB chargers is only one of the ways the bq2525x devices can be
used in a system. Imagine a shipping product that comes with its own
proprietary wall power (or built-in) supply that let's say cranks out
2A.  As a vendor you want to configure your system (meaning, the bq2425x
via DT) to a fixed value of 2A. The user un-plugging and re-plugging the
power supply should not arbitrarily change that pre-configured limit

2) Even in case of USB chargers, userspace could detect the power
un-plug event, and re-configure the limit to something else.  So I think
it's really policy that should not be implemented in the Kernel driver.

Regards,

--
Andreas Dannenberg
Texas Instruments Inc

Re: [PATCH v5 09/11] power: bq24257: Allow input current limit sysfs access

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 3318 bytes --]

Hi,

On Wed, Sep 23, 2015 at 01:32:58PM -0500, Andreas Dannenberg wrote:
> On Wed, Sep 23, 2015 at 05:02:28PM +0200, Sebastian Reichel wrote:
> > On Wed, Sep 23, 2015 at 09:11:46AM -0500, Andreas Dannenberg wrote:
> > > Ok. So how should we best go about extending the usage of the
> > > 'input_current_limit' sysfs node for this charger? You mentioned
> > > writing 'auto' into it should enable the auto-detection mode. I suppose
> > > writing a fixed current value will disable it. But how to indicate to
> > > the user when reading 'input_current_limit' whether auto mode is enabled
> > > or not (I think this is something we should do).
> > 
> > Right, I haven't thought of this.
> > 
> > > Can we return a mixed number/string like this?
> > > 
> > > # Example: charger auto detection mode is disabled, and input current
> > > # limit is configured as 500mA.
> > > 
> > > $ cat input_current_limit
> > > 500000
> > > 
> > > # Example: charger auto detection mode is enabled, and a charger
> > > # supporting 1A was detected (note the mixed number/string thats
> > > # returned)
> > > 
> > > $ cat input_current_limit
> > > 1000000 (auto)
> > > 
> > > Would that work?
> > 
> > No. sysfs nodes should only contain one value per file.
> > 
> > > Or should we introduce a new sysfs property?
> > 
> > So maybe keep this patch as is (disallow setting the limit in auto
> > mode) and create another file for setting (and getting) the mode.
> 
> Thanks for the continued feedback. Will look into this and add a new
> property.
>  
> > Also it might be a good idea to return to safe defaults when the
> > charger is disconnected (-> reset to DT specified values).
> 
> It already does this when the charger type auto-detection is enabled.
> When configured for manually setting the input current limit the use
> case is a bit more complex and I do not recommend resetting the limit to
> 500mA. Let me explain why:
>
> 1) Using USB chargers is only one of the ways the bq2525x devices can be
> used in a system. Imagine a shipping product that comes with its own
> proprietary wall power (or built-in) supply that let's say cranks out
> 2A.  As a vendor you want to configure your system (meaning, the bq2425x
> via DT) to a fixed value of 2A. The user un-plugging and re-plugging the
> power supply should not arbitrarily change that pre-configured limit

I did not write, that it should reset to 500mA, but that it should
reset to DT specified values. So e.g. if the device is confiured to
be in auto mode in DT, then configured to use fixed 1000mA, then it
should return to auto mode on unplug. OTOH if DT specifies 500mA
fixed and user sets 1000mA, then it should return to 500mA fixes.
So re-plugging returns to pre-configured limits.

> 2) Even in case of USB chargers, userspace could detect the power
> un-plug event, and re-configure the limit to something else.  So I think
> it's really policy that should not be implemented in the Kernel driver.

I think it should be done exactly the opposite way. Userspace should
set the custom value again. My main motivation is, that DT values
should be safe for all attachable power supplies, while the user
supplied value may only be safe with the currently connected power
supply.

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v5 05/11] power: bq24257: Add SW-based approach for Power Good determination

[Andreas Dannenberg]

On Tue, Sep 22, 2015 at 09:37:20PM +0200, Sebastian Reichel wrote:
> Hi,
> 
> On Fri, Sep 18, 2015 at 04:39:53PM -0500, Andreas Dannenberg wrote:
> > -	state->power_good = !gpiod_get_value_cansleep(bq->pg);
> > +	if (bq->pg_gpio_disable)
> > +		/*
> > +		 * If we have a chip without a dedicated power-good GPIO or
> > +		 * some other explicit bit that would provide this information
> > +		 * assume the power is good if there is no supply related
> > +		 * fault - and not good otherwise. There is a possibility for
> > +		 * other errors to mask that power in fact is not good but this
> > +		 * is probably the best we can do here.
> > +		 */
> > +		switch (state->fault) {
> > +		case FAULT_INPUT_OVP:
> > +		case FAULT_INPUT_UVLO:
> > +		case FAULT_INPUT_LDO_LOW:
> > +			state->power_good = false;
> > +			break;
> > +		default:
> > +			state->power_good = true;
> > +		}
> > +	else
> > +		state->power_good = !gpiod_get_value_cansleep(bq->pg);
> 
> I guess you can just handle this like an optional gpio
> 
> if(bq->pg)
>     state->power_good = !gpiod_get_value_cansleep(bq->pg);
> else
>     ...

What happens when somebody wants to use GPIO number 0? According to
gpio_is_valid() this is a valid GPIO so technically I should not use a
check against zero to see whether the user has configured a GPIO for
this purpose and wants to use it, no?

Regards,

--
Andreas Dannenberg
Texas Instruments Inc

Re: [PATCH v5 09/11] power: bq24257: Allow input current limit sysfs access

[Andreas Dannenberg]

On Wed, Sep 23, 2015 at 08:53:59PM +0200, Sebastian Reichel wrote:
> Hi,
> 
> On Wed, Sep 23, 2015 at 01:32:58PM -0500, Andreas Dannenberg wrote:
> > On Wed, Sep 23, 2015 at 05:02:28PM +0200, Sebastian Reichel wrote:
> > > On Wed, Sep 23, 2015 at 09:11:46AM -0500, Andreas Dannenberg wrote:
> > > > Ok. So how should we best go about extending the usage of the
> > > > 'input_current_limit' sysfs node for this charger? You mentioned
> > > > writing 'auto' into it should enable the auto-detection mode. I suppose
> > > > writing a fixed current value will disable it. But how to indicate to
> > > > the user when reading 'input_current_limit' whether auto mode is enabled
> > > > or not (I think this is something we should do).
> > > 
> > > Right, I haven't thought of this.
> > > 
> > > > Can we return a mixed number/string like this?
> > > > 
> > > > # Example: charger auto detection mode is disabled, and input current
> > > > # limit is configured as 500mA.
> > > > 
> > > > $ cat input_current_limit
> > > > 500000
> > > > 
> > > > # Example: charger auto detection mode is enabled, and a charger
> > > > # supporting 1A was detected (note the mixed number/string thats
> > > > # returned)
> > > > 
> > > > $ cat input_current_limit
> > > > 1000000 (auto)
> > > > 
> > > > Would that work?
> > > 
> > > No. sysfs nodes should only contain one value per file.
> > > 
> > > > Or should we introduce a new sysfs property?
> > > 
> > > So maybe keep this patch as is (disallow setting the limit in auto
> > > mode) and create another file for setting (and getting) the mode.
> > 
> > Thanks for the continued feedback. Will look into this and add a new
> > property.
> >  
> > > Also it might be a good idea to return to safe defaults when the
> > > charger is disconnected (-> reset to DT specified values).
> > 
> > It already does this when the charger type auto-detection is enabled.
> > When configured for manually setting the input current limit the use
> > case is a bit more complex and I do not recommend resetting the limit to
> > 500mA. Let me explain why:
> >
> > 1) Using USB chargers is only one of the ways the bq2525x devices can be
> > used in a system. Imagine a shipping product that comes with its own
> > proprietary wall power (or built-in) supply that let's say cranks out
> > 2A.  As a vendor you want to configure your system (meaning, the bq2425x
> > via DT) to a fixed value of 2A. The user un-plugging and re-plugging the
> > power supply should not arbitrarily change that pre-configured limit
> 
> I did not write, that it should reset to 500mA, but that it should
> reset to DT specified values. So e.g. if the device is confiured to
> be in auto mode in DT, then configured to use fixed 1000mA, then it
> should return to auto mode on unplug. OTOH if DT specifies 500mA
> fixed and user sets 1000mA, then it should return to 500mA fixes.
> So re-plugging returns to pre-configured limits.

Ok yes considering the default being the DT value what you originally
wrote that makes more sense now. My mind was buried in the code :)

> > 2) Even in case of USB chargers, userspace could detect the power
> > un-plug event, and re-configure the limit to something else.  So I think
> > it's really policy that should not be implemented in the Kernel driver.
> 
> I think it should be done exactly the opposite way. Userspace should
> set the custom value again. My main motivation is, that DT values
> should be safe for all attachable power supplies, while the user
> supplied value may only be safe with the currently connected power
> supply.

I don't think there is really a right or wrong here but I do see your
point. It's a workable and safe solution. Will implement accordingly.

Regards,

--
Andreas Dannenberg
Texas Instruments Inc

Re: [PATCH v5 05/11] power: bq24257: Add SW-based approach for Power Good determination

[Andreas Dannenberg]

On Wed, Sep 23, 2015 at 02:34:27PM -0500, Andreas Dannenberg wrote:
> On Tue, Sep 22, 2015 at 09:37:20PM +0200, Sebastian Reichel wrote:
> > 
> > I guess you can just handle this like an optional gpio
> > 
> > if(bq->pg)
> >     state->power_good = !gpiod_get_value_cansleep(bq->pg);
> > else
> >     ...
> 
> What happens when somebody wants to use GPIO number 0? According to
> gpio_is_valid() this is a valid GPIO so technically I should not use a
> check against zero to see whether the user has configured a GPIO for
> this purpose and wants to use it, no?

Ok never mind I figured it out. bq->pg is of type gpio_desc and not the
actual GPIO number. Together with your suggestion of how to use gpiod_*
in combination with platform data this will be a nice simplification!

Regards,

--
Andreas Dannenberg
Texas Instruments Inc