[PATCH v4 00/10] power: bq24257: Add support for bq24250/bq24251

[PATCH v4 00/10] 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.

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 (10):
  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: 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                    | 546 +++++++++++++++++++--
 include/linux/power/bq24257_charger.h              |  29 ++
 4 files changed, 585 insertions(+), 50 deletions(-)
 create mode 100644 include/linux/power/bq24257_charger.h

-- 
1.9.1

[PATCH v4 01/10] 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-l0cyMroinI0@public.gmane.org>
---
 .../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

--
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 v4 02/10] 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 | 46 +++++++++++++++++++++++++++++++++++++++--
 2 files changed, 47 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..1b3ddfb 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,18 @@
 
 #define BQ24257_ILIM_SET_DELAY		1000	/* msec */
 
+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 +87,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 +267,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 +591,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 +689,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 +706,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 +859,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 v4 03/10] 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 1b3ddfb..fdfe855 100644
--- a/drivers/power/bq24257_charger.c
+++ b/drivers/power/bq24257_charger.c
@@ -63,7 +63,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
@@ -153,6 +153,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 v4 04/10] 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 fdfe855..55e4ee4 100644
--- a/drivers/power/bq24257_charger.c
+++ b/drivers/power/bq24257_charger.c
@@ -74,6 +74,7 @@ struct bq24257_init_data {
 	u8 ichg;	/* charge current      */
 	u8 vbat;	/* regulation voltage  */
 	u8 iterm;	/* termination current */
+	u8 in_ilimit;	/* input current limit */
 };
 
 struct bq24257_state {
@@ -100,6 +101,8 @@ struct bq24257_device {
 	struct bq24257_state state;
 
 	struct mutex lock; /* protect state data */
+
+	bool in_ilimit_autoset_disable;
 };
 
 static bool bq24257_is_volatile_reg(struct device *dev, unsigned int reg)
@@ -188,6 +191,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)
 {
@@ -479,24 +488,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->in_ilimit_autoset_disable) {
+			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->in_ilimit_autoset_disable;
+	} else if (new_state->fault == FAULT_NO_BAT) {
+		dev_warn(bq->dev, "Battery removed\n");
+		if (!bq->in_ilimit_autoset_disable) {
+			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->in_ilimit_autoset_disable;
+	} else if (new_state->fault == FAULT_TIMER) {
 		dev_err(bq->dev, "Safety timer expired! Battery dead?\n");
 	}
 
@@ -581,7 +601,16 @@ static int bq24257_hw_init(struct bq24257_device *bq)
 	bq->state = state;
 	mutex_unlock(&bq->lock);
 
-	if (!state.power_good)
+	if (bq->in_ilimit_autoset_disable) {
+		dev_dbg(bq->dev, "manually setting iilimit = %d\n",
+			bq->init_data.in_ilimit);
+
+		/* program fixed input current limit */
+		ret = bq24257_field_write(bq, F_IILIMIT,
+					  bq->init_data.in_ilimit);
+		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)
@@ -659,6 +688,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;
@@ -682,6 +712,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)
+		/*
+		 * 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.in_ilimit = IILIMIT_500;
+	else {
+		bq->in_ilimit_autoset_disable = true;
+		bq->init_data.in_ilimit =
+				bq24257_find_idx(property,
+						 bq24257_iilimit_map,
+						 BQ24257_IILIMIT_MAP_SIZE);
+	}
+
 	return 0;
 }
 
@@ -740,8 +788,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) {
@@ -752,6 +798,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->in_ilimit_autoset_disable = true;
+
+	if (!bq->in_ilimit_autoset_disable)
+		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)
@@ -804,7 +862,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->in_ilimit_autoset_disable)
+		cancel_delayed_work_sync(&bq->iilimit_setup_work);
 
 	power_supply_unregister(bq->charger);
 
@@ -819,7 +878,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->in_ilimit_autoset_disable)
+		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 v4 05/10] 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 55e4ee4..d7488cf 100644
--- a/drivers/power/bq24257_charger.c
+++ b/drivers/power/bq24257_charger.c
@@ -103,6 +103,7 @@ struct bq24257_device {
 	struct mutex lock; /* protect state data */
 
 	bool in_ilimit_autoset_disable;
+	bool pg_gpio_disable;
 };
 
 static bool bq24257_is_volatile_reg(struct device *dev, unsigned int reg)
@@ -356,7 +357,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;
 }
@@ -676,7 +696,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);
 	}
 
@@ -810,10 +830,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 v4 06/10] 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 d7488cf..47af858 100644
--- a/drivers/power/bq24257_charger.c
+++ b/drivers/power/bq24257_charger.c
@@ -75,6 +75,7 @@ struct bq24257_init_data {
 	u8 vbat;	/* regulation voltage  */
 	u8 iterm;	/* termination current */
 	u8 in_ilimit;	/* input current limit */
+	u8 vovp;	/* over voltage protection voltage */
 };
 
 struct bq24257_state {
@@ -198,6 +199,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)
 {
@@ -413,6 +421,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 */
@@ -594,7 +613,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},
 	};
 
 	/*
@@ -664,6 +684,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, "%d\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, };
@@ -750,6 +792,15 @@ static int bq24257_fw_probe(struct bq24257_device *bq)
 						 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;
 }
 
@@ -889,10 +940,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)
@@ -902,6 +961,8 @@ static int bq24257_remove(struct i2c_client *client)
 	if (!bq->in_ilimit_autoset_disable)
 		cancel_delayed_work_sync(&bq->iilimit_setup_work);
 
+	sysfs_remove_group(&bq->charger->dev.kobj, &bq24257_attr_group);
+
 	power_supply_unregister(bq->charger);
 
 	bq24257_field_write(bq, F_RESET, 1); /* reset to defaults */
-- 
1.9.1

[PATCH v4 07/10] 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 | 44 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c
index 47af858..84a8945 100644
--- a/drivers/power/bq24257_charger.c
+++ b/drivers/power/bq24257_charger.c
@@ -76,6 +76,7 @@ struct bq24257_init_data {
 	u8 iterm;	/* termination current */
 	u8 in_ilimit;	/* input current limit */
 	u8 vovp;	/* over voltage protection voltage */
+	u8 vindpm;	/* VDMP input threshold voltage */
 };
 
 struct bq24257_state {
@@ -206,6 +207,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)
 {
@@ -432,6 +440,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 */
@@ -615,6 +634,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},
 	};
 
 	/*
@@ -648,6 +668,7 @@ static int bq24257_hw_init(struct bq24257_device *bq)
 		/* program fixed input current limit */
 		ret = bq24257_field_write(bq, F_IILIMIT,
 					  bq->init_data.in_ilimit);
+
 		if (ret < 0)
 			return ret;
 	} else if (!state.power_good)
@@ -695,10 +716,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, "%d\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,
 };
 
@@ -801,6 +835,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 v4 08/10] 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_PROP_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@ti.com>
---
 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 84a8945..517a522 100644
--- a/drivers/power/bq24257_charger.c
+++ b/drivers/power/bq24257_charger.c
@@ -265,6 +265,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->in_ilimit_autoset_disable)
+		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)
@@ -349,6 +385,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;
 	}
@@ -356,6 +395,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)
 {
@@ -691,6 +755,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[] = {
@@ -703,6 +768,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

[PATCH v4 09/10] 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 517a522..a0cb33c 100644
--- a/drivers/power/bq24257_charger.c
+++ b/drivers/power/bq24257_charger.c
@@ -794,12 +794,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, val);
+	else if (strcmp(attr->attr.name, "sysoff_enable") == 0)
+		ret = bq24257_field_write(bq, F_SYSOFF, 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 v4 10/10] 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 a0cb33c..fa18b1f 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
@@ -877,28 +880,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,
+					    GPIOD_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,
+					    GPIOD_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.in_ilimit = bq24257_find_idx(pdata->in_ilimit,
+						   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->in_ilimit_autoset_disable = pdata->in_ilimit_autoset_disable;
+
+	bq->pg_gpio_disable = pdata->pg_gpio_disable;
+}
+
 static int bq24257_fw_probe(struct bq24257_device *bq)
 {
 	int ret;
@@ -974,6 +1061,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;
@@ -1023,14 +1111,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);
 	}
 
 	/*
@@ -1087,7 +1176,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..d4f3703
--- /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 in_ilimit;				  /* input current limit (uA) */
+	u32 vovp;		      /* over voltage protection voltage (uV) */
+	u32 vindpm;			 /* VDMP input threshold voltage (uV) */
+	bool in_ilimit_autoset_disable;	/* 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
-- 
1.9.1

Re: [PATCH v4 01/10] dt: power: bq24257-charger: Cover additional devices

[Krzysztof Kozlowski]

On 16.09.2015 02:58, Andreas Dannenberg wrote:
> 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(-)
> 

Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Best regards,
Krzysztof

Re: [PATCH v4 02/10] power: bq24257: Add basic support for bq24250/bq24251

[Krzysztof Kozlowski]

On 16.09.2015 02:58, Andreas Dannenberg wrote:
> 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 | 46 +++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 47 insertions(+), 4 deletions(-)

Two comments below, but they are just suggestions/questions. The code
code now looks good:

Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

> 
> 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..1b3ddfb 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,18 @@
>  
>  #define BQ24257_ILIM_SET_DELAY		1000	/* msec */
>  
> +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 +87,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 +267,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];

You have now here strict relation between number of elements in
bq2425x_chip_name and possible types of devices (enum values). Consider
adding a comment at beginning or a BUILD_BUG_ON to prevent mismatches in
the future (like someone will add a new value in enum but will forgot to
add device name).

> +		break;
> +
>  	case POWER_SUPPLY_PROP_ONLINE:
>  		val->intval = state.power_good;
>  		break;
> @@ -569,6 +591,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 +689,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 +706,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 +859,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 },

The last "0" in each device name (in string) is on purpose?

Best regards,
Krzysztof

>  	{},
>  };
>  MODULE_DEVICE_TABLE(acpi, bq24257_acpi_match);
> 

Re: [PATCH v4 04/10] power: bq24257: Allow manual setting of input current limit

[Krzysztof Kozlowski]

On 16.09.2015 02:58, Andreas Dannenberg wrote:
> 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 fdfe855..55e4ee4 100644
> --- a/drivers/power/bq24257_charger.c
> +++ b/drivers/power/bq24257_charger.c
> @@ -74,6 +74,7 @@ struct bq24257_init_data {
>  	u8 ichg;	/* charge current      */
>  	u8 vbat;	/* regulation voltage  */
>  	u8 iterm;	/* termination current */
> +	u8 in_ilimit;	/* input current limit */
>  };
>  
>  struct bq24257_state {
> @@ -100,6 +101,8 @@ struct bq24257_device {
>  	struct bq24257_state state;
>  
>  	struct mutex lock; /* protect state data */
> +
> +	bool in_ilimit_autoset_disable;
>  };
>  
>  static bool bq24257_is_volatile_reg(struct device *dev, unsigned int reg)
> @@ -188,6 +191,12 @@ static const u32 bq24257_iterm_map[] = {
>  
>  #define BQ24257_ITERM_MAP_SIZE		ARRAY_SIZE(bq24257_iterm_map)
>  
> +static const u32 bq24257_iilimit_map[] = {

Too many ii? bq24257_ilimit_map? Or is it a shortcut for "in_ilimit"?
New fields have pattern like "in_ilimit*".
Anyway it's a confusing so maybe use everywhere the same pattern?


> +	100000, 150000, 500000, 900000, 1500000, 2000000
> +};
> +
> +#define BQ24257_IILIMIT_MAP_SIZE	ARRAY_SIZE(bq24257_iilimit_map)

ditto: ILIMIT_MAP_SIZE?

> +
>  static int bq24257_field_read(struct bq24257_device *bq,
>  			      enum bq24257_fields field_id)
>  {
> @@ -479,24 +488,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->in_ilimit_autoset_disable) {
> +			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->in_ilimit_autoset_disable;
> +	} else if (new_state->fault == FAULT_NO_BAT) {
> +		dev_warn(bq->dev, "Battery removed\n");

dev_warn? This wasn't here before... It's a bit too serious. Is removing
a battery an error condition? Maybe user just unplugged it?
dev_dbg or dev_info should be sufficient.

BTW, it is useful to quickly find boot regressions with "dmesg -l
warn,err". Removing a battery probably is not an error?

> +		if (!bq->in_ilimit_autoset_disable) {
> +			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");

Definitely not a warn. Inserting a battery is not an error condition.

> +		config_iilimit = !bq->in_ilimit_autoset_disable;
> +	} else if (new_state->fault == FAULT_TIMER) {
>  		dev_err(bq->dev, "Safety timer expired! Battery dead?\n");
>  	}

Don't you have a schedule_delayed_work() call here which now will be
executed always? Even when work was not INIT and nothing will cancel it?

>  
> @@ -581,7 +601,16 @@ static int bq24257_hw_init(struct bq24257_device *bq)
>  	bq->state = state;
>  	mutex_unlock(&bq->lock);
>  
> -	if (!state.power_good)
> +	if (bq->in_ilimit_autoset_disable) {
> +		dev_dbg(bq->dev, "manually setting iilimit = %d\n",
> +			bq->init_data.in_ilimit);

Nit, no actual difference but makes more sense to me because field is
u8: "%u".

> +
> +		/* program fixed input current limit */
> +		ret = bq24257_field_write(bq, F_IILIMIT,
> +					  bq->init_data.in_ilimit);
> +		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)
> @@ -659,6 +688,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;
> @@ -682,6 +712,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)
> +		/*
> +		 * 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.in_ilimit = IILIMIT_500;
> +	else {
> +		bq->in_ilimit_autoset_disable = true;
> +		bq->init_data.in_ilimit =
> +				bq24257_find_idx(property,
> +						 bq24257_iilimit_map,
> +						 BQ24257_IILIMIT_MAP_SIZE);
> +	}
> +
>  	return 0;
>  }
>  
> @@ -740,8 +788,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) {
> @@ -752,6 +798,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->in_ilimit_autoset_disable = true;
> +
> +	if (!bq->in_ilimit_autoset_disable)

In most places you have quite obfuscated negation of
"autoset_disable"... So maybe just "bq->in_ilimit_autoset" or
"bq->in_ilimit_autoset_enable" and the negation won't be needed? It
could be more readable.

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 16.09.2015 02:58, 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 55e4ee4..d7488cf 100644
> --- a/drivers/power/bq24257_charger.c
> +++ b/drivers/power/bq24257_charger.c
> @@ -103,6 +103,7 @@ struct bq24257_device {
>  	struct mutex lock; /* protect state data */
>  
>  	bool in_ilimit_autoset_disable;
> +	bool pg_gpio_disable;
>  };
>  
>  static bool bq24257_is_volatile_reg(struct device *dev, unsigned int reg)
> @@ -356,7 +357,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;
>  }
> @@ -676,7 +696,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");

I think if pg-gpio is provided (e.g. by DTS) but it is invalid (return
value != ENOENT) then it is an error you could print. The driver will
fallback to the software method but still user/developer may want to
notice the error (e.g. error in DTS).

Anyway it is up to you, rest looks good:

Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Best regards,
Krzysztof

Re: [PATCH v4 06/10] power: bq24257: Add over voltage protection setting support

[Krzysztof Kozlowski]

On 16.09.2015 02:58, Andreas Dannenberg wrote:
> 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 d7488cf..47af858 100644
> --- a/drivers/power/bq24257_charger.c
> +++ b/drivers/power/bq24257_charger.c
> @@ -75,6 +75,7 @@ struct bq24257_init_data {
>  	u8 vbat;	/* regulation voltage  */
>  	u8 iterm;	/* termination current */
>  	u8 in_ilimit;	/* input current limit */
> +	u8 vovp;	/* over voltage protection voltage */
>  };
>  
>  struct bq24257_state {
> @@ -198,6 +199,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)
>  {
> @@ -413,6 +421,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
> +};

What is the purpose of this enum? It is duplicating values from the map
(so you can make a mistake in its order) but actually is not used. I
don't see benefit.

> +
>  enum bq24257_port_type {
>  	PORT_TYPE_DCP,		/* Dedicated Charging Port */
>  	PORT_TYPE_CDP,		/* Charging Downstream Port */
> @@ -594,7 +613,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},
>  	};
>  
>  	/*
> @@ -664,6 +684,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, "%d\n",

"%u", you are printing u32.

> +			 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, };
> @@ -750,6 +792,15 @@ static int bq24257_fw_probe(struct bq24257_device *bq)
>  						 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;
>  }
>  
> @@ -889,10 +940,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");

power_supply_unregister(bq->charger);
or convert in separate patch to devm-like method.

Best regards,
Krzysztof

> +		return ret;
> +	}
> +
> +	return 0;
>  }
>  
>  static int bq24257_remove(struct i2c_client *client)
> @@ -902,6 +961,8 @@ static int bq24257_remove(struct i2c_client *client)
>  	if (!bq->in_ilimit_autoset_disable)
>  		cancel_delayed_work_sync(&bq->iilimit_setup_work);
>  
> +	sysfs_remove_group(&bq->charger->dev.kobj, &bq24257_attr_group);
> +
>  	power_supply_unregister(bq->charger);
>  
>  	bq24257_field_write(bq, F_RESET, 1); /* reset to defaults */
> 

Re: [PATCH v4 07/10] power: bq24257: Add input DPM voltage threshold setting support

[Krzysztof Kozlowski]

On 16.09.2015 02:58, Andreas Dannenberg wrote:
> 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 | 44 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c
> index 47af858..84a8945 100644
> --- a/drivers/power/bq24257_charger.c
> +++ b/drivers/power/bq24257_charger.c
> @@ -76,6 +76,7 @@ struct bq24257_init_data {
>  	u8 iterm;	/* termination current */
>  	u8 in_ilimit;	/* input current limit */
>  	u8 vovp;	/* over voltage protection voltage */
> +	u8 vindpm;	/* VDMP input threshold voltage */
>  };
>  
>  struct bq24257_state {
> @@ -206,6 +207,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)
>  {
> @@ -432,6 +440,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
> +};

Same as patch 6/10... but now see the use case - to choose default value
not by index of array but from this enum.

I see the idea behind so skip that comment from 6/10.

> +
>  enum bq24257_port_type {
>  	PORT_TYPE_DCP,		/* Dedicated Charging Port */
>  	PORT_TYPE_CDP,		/* Charging Downstream Port */
> @@ -615,6 +634,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},
>  	};
>  
>  	/*
> @@ -648,6 +668,7 @@ static int bq24257_hw_init(struct bq24257_device *bq)
>  		/* program fixed input current limit */
>  		ret = bq24257_field_write(bq, F_IILIMIT,
>  					  bq->init_data.in_ilimit);
> +

Not part of this patch. It confuses the reviewer of patch. If you need
cleanups, make them separate.

>  		if (ret < 0)
>  			return ret;
>  	} else if (!state.power_good)
> @@ -695,10 +716,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, "%d\n",
> +			 bq24257_vindpm_map[bq->init_data.vindpm]);

Same as patch 6/10.

Best regards,
Krzysztof

> +}
> +
>  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,
>  };
>  
> @@ -801,6 +835,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;
>  }
>  
> 

Re: [PATCH v4 08/10] power: bq24257: Allow input current limit sysfs access

[Krzysztof Kozlowski]

On 16.09.2015 02:58, 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_PROP_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@ti.com>
> ---
>  drivers/power/bq24257_charger.c | 67 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
> 

Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

BTW you are exposing more and more sysfs attributes which are userspace
interfaces. They should be documented:
Documentation/SubmitChecklist - point 19
Documentation/ABI/README

Although not all new drivers (and perhaps not all reviewers) follow this
rule...

Best regards,
Krzysztof

Re: [PATCH v4 09/10] power: bq24257: Add various device-specific sysfs properties

[Krzysztof Kozlowski]

On 16.09.2015 02:58, Andreas Dannenberg wrote:
> 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 517a522..a0cb33c 100644
> --- a/drivers/power/bq24257_charger.c
> +++ b/drivers/power/bq24257_charger.c
> @@ -794,12 +794,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;
> +

There is no validation for input number. Although this is harmless but
one may expect that writing value of 1, 2 or 3 would give the same
result. But it would not. Value of 2 would be equal to 0, right?

> +	if (strcmp(attr->attr.name, "high_impedance_enable") == 0)
> +		ret = bq24257_field_write(bq, F_HZ_MODE, val);
> +	else if (strcmp(attr->attr.name, "sysoff_enable") == 0)
> +		ret = bq24257_field_write(bq, F_SYSOFF, 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);

This applies to previous patches actually: DEVICE_ATTR_RO?

> +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);

DEVICE_ATTR_RW?

Best regards,
Krzysztof

Re: [PATCH v4 10/10] power: bq24257: Add platform data based initialization

[Krzysztof Kozlowski]

On 16.09.2015 02:58, 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.
> 
> 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 a0cb33c..fa18b1f 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
> @@ -877,28 +880,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,
> +					    GPIOD_IN, BQ24257_STAT_IRQ);

That should be GPIOF_* flag, not GPIOD.

> +		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,
> +					    GPIOD_IN, BQ24257_PG_GPIO);

ditto

> +		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.in_ilimit = bq24257_find_idx(pdata->in_ilimit,
> +						   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->in_ilimit_autoset_disable = pdata->in_ilimit_autoset_disable;
> +
> +	bq->pg_gpio_disable = pdata->pg_gpio_disable;
> +}
> +
>  static int bq24257_fw_probe(struct bq24257_device *bq)
>  {
>  	int ret;
> @@ -974,6 +1061,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;
> @@ -1023,14 +1111,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);
>  	}
>  
>  	/*
> @@ -1087,7 +1176,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.
> +	 */

Why?

> +	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..d4f3703
> --- /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 in_ilimit;				  /* input current limit (uA) */
> +	u32 vovp;		      /* over voltage protection voltage (uV) */
> +	u32 vindpm;			 /* VDMP input threshold voltage (uV) */
> +	bool in_ilimit_autoset_disable;	/* 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

#endif /* _BQ24257_CHARGER_H_ */

Best regards,
Krzysztof

Re: [PATCH v4 02/10] power: bq24257: Add basic support for bq24250/bq24251

[Andreas Dannenberg]

On Wed, Sep 16, 2015 at 09:19:11AM +0900, Krzysztof Kozlowski wrote:
> On 16.09.2015 02:58, Andreas Dannenberg wrote:
> > 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 | 46 +++++++++++++++++++++++++++++++++++++++--
> >  2 files changed, 47 insertions(+), 4 deletions(-)
> 
> Two comments below, but they are just suggestions/questions. The code
> code now looks good:
> 
> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> 
> > 
> > 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..1b3ddfb 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,18 @@
> >  
> >  #define BQ24257_ILIM_SET_DELAY		1000	/* msec */
> >  
> > +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 +87,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 +267,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];
> 
> You have now here strict relation between number of elements in
> bq2425x_chip_name and possible types of devices (enum values). Consider
> adding a comment at beginning or a BUILD_BUG_ON to prevent mismatches in
> the future (like someone will add a new value in enum but will forgot to
> add device name).

Good suggestion, will look into this.
 
> > +		break;
> > +
> >  	case POWER_SUPPLY_PROP_ONLINE:
> >  		val->intval = state.power_good;
> >  		break;
> > @@ -569,6 +591,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 +689,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 +706,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 +859,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 },
> 
> The last "0" in each device name (in string) is on purpose?

I had the same question when I first picked up the work on this driver
and asked Laurentiu about this aspect offline and apparently it's
related to an ACPI requirement. Here is his reply in its entirety:

---Quote Laurentiu Begin---
If the driver had been developed starting from an actual end product, we
would have used the actual ACPI ID, provided by the product FW. However,
since our development "device" was a combination of EVM, USB-to-I2C
bridge and QEMU, we had the liberty to choose whatever ACPI ID we
wanted, provided that if an actual product using this chip appeared, we
could always add the new ACPI ID to bq24257_acpi_match[] and have the
driver working.

The ACPI specification states: "A valid ACPI ID must be of the form
"NNNN####" where N is an uppercase letter or a digit ('0'-'9') and # is
a hex digit." But it does not strictly forbids one to use another
combination as long as it's 8 characters long.

The only specification compliant combination that came to mind was
"TIBQ2425" but it missed the last digit... Hence, I decided to use
"BQ242570" for the time being. At least it contains the entire chip
name
---Quote Laurentiu End---

This all made sense so I simply extended the scheme that was used.

Regards,
Andreas

> 
> Best regards,
> Krzysztof
> 
> >  	{},
> >  };
> >  MODULE_DEVICE_TABLE(acpi, bq24257_acpi_match);
> > 
> 

Re: [PATCH v4 04/10] power: bq24257: Allow manual setting of input current limit

[Andreas Dannenberg]

On Wed, Sep 16, 2015 at 09:41:49AM +0900, Krzysztof Kozlowski wrote:
> On 16.09.2015 02:58, Andreas Dannenberg wrote:
> > 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 fdfe855..55e4ee4 100644
> > --- a/drivers/power/bq24257_charger.c
> > +++ b/drivers/power/bq24257_charger.c
> > @@ -74,6 +74,7 @@ struct bq24257_init_data {
> >  	u8 ichg;	/* charge current      */
> >  	u8 vbat;	/* regulation voltage  */
> >  	u8 iterm;	/* termination current */
> > +	u8 in_ilimit;	/* input current limit */
> >  };
> >  
> >  struct bq24257_state {
> > @@ -100,6 +101,8 @@ struct bq24257_device {
> >  	struct bq24257_state state;
> >  
> >  	struct mutex lock; /* protect state data */
> > +
> > +	bool in_ilimit_autoset_disable;
> >  };
> >  
> >  static bool bq24257_is_volatile_reg(struct device *dev, unsigned int reg)
> > @@ -188,6 +191,12 @@ static const u32 bq24257_iterm_map[] = {
> >  
> >  #define BQ24257_ITERM_MAP_SIZE		ARRAY_SIZE(bq24257_iterm_map)
> >  
> > +static const u32 bq24257_iilimit_map[] = {
> 
> Too many ii? bq24257_ilimit_map? Or is it a shortcut for "in_ilimit"?
> New fields have pattern like "in_ilimit*".
> Anyway it's a confusing so maybe use everywhere the same pattern?

Yes that's supposed to be a shortcut for this internal variable. It's a
low-risk refactor so let me tweak this.

> > +	100000, 150000, 500000, 900000, 1500000, 2000000
> > +};
> > +
> > +#define BQ24257_IILIMIT_MAP_SIZE	ARRAY_SIZE(bq24257_iilimit_map)
> 
> ditto: ILIMIT_MAP_SIZE?

Ditto :)

> > +
> >  static int bq24257_field_read(struct bq24257_device *bq,
> >  			      enum bq24257_fields field_id)
> >  {
> > @@ -479,24 +488,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->in_ilimit_autoset_disable) {
> > +			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->in_ilimit_autoset_disable;
> > +	} else if (new_state->fault == FAULT_NO_BAT) {
> > +		dev_warn(bq->dev, "Battery removed\n");
> 
> dev_warn? This wasn't here before... It's a bit too serious. Is removing
> a battery an error condition? Maybe user just unplugged it?
> dev_dbg or dev_info should be sufficient.
> 
> BTW, it is useful to quickly find boot regressions with "dmesg -l
> warn,err". Removing a battery probably is not an error?

I would argue that most devices that use a Li-Ion battery have the
battery built-in and it's not user removable. Therefore, if the battery
would ever go disconnected I figured it'll most likely be something
serious such as a contact breaking loose or something else dramatic,
warranting at least a warning. Plus, many devices with built in Li-Ion
batteries are actually designed in a way that they don't really function
properly with the battery taken out as the HW is often designed to draw
supplemental current from the battery during times of load in addition
to the A/C supply (key feature of many charger chips).

> 
> > +		if (!bq->in_ilimit_autoset_disable) {
> > +			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");
> 
> Definitely not a warn. Inserting a battery is not an error condition.

Same as above.

> > +		config_iilimit = !bq->in_ilimit_autoset_disable;
> > +	} else if (new_state->fault == FAULT_TIMER) {
> >  		dev_err(bq->dev, "Safety timer expired! Battery dead?\n");
> >  	}
> 
> Don't you have a schedule_delayed_work() call here which now will be
> executed always? Even when work was not INIT and nothing will cancel it?

It'll be more obvious when looking at the merged code but the schedule_
delayed_work() call only happens if config_iilimit==true which only
happens when the input limit current autoset functionality is not
disabled. If that's the case (autoset functionality is enabled) the INIT
for that work is executed during probe.
 
> >  
> > @@ -581,7 +601,16 @@ static int bq24257_hw_init(struct bq24257_device *bq)
> >  	bq->state = state;
> >  	mutex_unlock(&bq->lock);
> >  
> > -	if (!state.power_good)
> > +	if (bq->in_ilimit_autoset_disable) {
> > +		dev_dbg(bq->dev, "manually setting iilimit = %d\n",
> > +			bq->init_data.in_ilimit);
> 
> Nit, no actual difference but makes more sense to me because field is
> u8: "%u".

Yes that should be changed.

> > +
> > +		/* program fixed input current limit */
> > +		ret = bq24257_field_write(bq, F_IILIMIT,
> > +					  bq->init_data.in_ilimit);
> > +		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)
> > @@ -659,6 +688,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;
> > @@ -682,6 +712,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)
> > +		/*
> > +		 * 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.in_ilimit = IILIMIT_500;
> > +	else {
> > +		bq->in_ilimit_autoset_disable = true;
> > +		bq->init_data.in_ilimit =
> > +				bq24257_find_idx(property,
> > +						 bq24257_iilimit_map,
> > +						 BQ24257_IILIMIT_MAP_SIZE);
> > +	}
> > +
> >  	return 0;
> >  }
> >  
> > @@ -740,8 +788,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) {
> > @@ -752,6 +798,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->in_ilimit_autoset_disable = true;
> > +
> > +	if (!bq->in_ilimit_autoset_disable)
> 
> In most places you have quite obfuscated negation of
> "autoset_disable"... So maybe just "bq->in_ilimit_autoset" or
> "bq->in_ilimit_autoset_enable" and the negation won't be needed? It
> could be more readable.

This stems from the fact that this was initially tied to a boolean DT
property with the same name which is no longer there. The other reason
was setting this property to non-zero changes the default behavior of
the driver (that used to have autoset always enabled) so I wanted to
reflect this behavior in the logic. The driver was tested pretty well so
unless you feel strongly about this I would rather leave it as-is.

Thanks for the feedback btw. I know it's a lot of work.

Regards,

--
Andreas Dannenberg
Texas Instruments Inc

Re: [PATCH v4 06/10] power: bq24257: Add over voltage protection setting support

[Andreas Dannenberg]

On Wed, Sep 16, 2015 at 02:55:12PM +0900, Krzysztof Kozlowski wrote:
> On 16.09.2015 02:58, Andreas Dannenberg wrote:
> > 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 d7488cf..47af858 100644
> > --- a/drivers/power/bq24257_charger.c
> > +++ b/drivers/power/bq24257_charger.c
> > @@ -75,6 +75,7 @@ struct bq24257_init_data {
> >  	u8 vbat;	/* regulation voltage  */
> >  	u8 iterm;	/* termination current */
> >  	u8 in_ilimit;	/* input current limit */
> > +	u8 vovp;	/* over voltage protection voltage */
> >  };
> >  
> >  struct bq24257_state {
> > @@ -198,6 +199,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)
> >  {
> > @@ -413,6 +421,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
> > +};
> 
> What is the purpose of this enum? It is duplicating values from the map
> (so you can make a mistake in its order) but actually is not used. I
> don't see benefit.

It's used in bq24257_power_supply_init() below (with "it" meaning one of
the values is used). I wouldn't really expect any mistakes to be made
since those tables are not something somebody can insert values into
(only append if ever).
 
> > +
> >  enum bq24257_port_type {
> >  	PORT_TYPE_DCP,		/* Dedicated Charging Port */
> >  	PORT_TYPE_CDP,		/* Charging Downstream Port */
> > @@ -594,7 +613,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},
> >  	};
> >  
> >  	/*
> > @@ -664,6 +684,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, "%d\n",
> 
> "%u", you are printing u32.

Good catch.

> > +}
> > +
> > +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, };
> > @@ -750,6 +792,15 @@ static int bq24257_fw_probe(struct bq24257_device *bq)
> >  						 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;
> >  }
> >  
> > @@ -889,10 +940,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");
> 
> power_supply_unregister(bq->charger);
> or convert in separate patch to devm-like method.

Nice find! I prefer using devm* methods any day if they exist. Will add
an extra patch to fold that in.

Regards,

--
Andreas Dannenberg
Texas Instruments Inc


> 
> Best regards,
> Krzysztof
> 
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> >  }
> >  
> >  static int bq24257_remove(struct i2c_client *client)
> > @@ -902,6 +961,8 @@ static int bq24257_remove(struct i2c_client *client)
> >  	if (!bq->in_ilimit_autoset_disable)
> >  		cancel_delayed_work_sync(&bq->iilimit_setup_work);
> >  
> > +	sysfs_remove_group(&bq->charger->dev.kobj, &bq24257_attr_group);
> > +
> >  	power_supply_unregister(bq->charger);
> >  
> >  	bq24257_field_write(bq, F_RESET, 1); /* reset to defaults */
> > 
> 

Re: [PATCH v4 07/10] power: bq24257: Add input DPM voltage threshold setting support

[Andreas Dannenberg]

On Wed, Sep 16, 2015 at 03:06:04PM +0900, Krzysztof Kozlowski wrote:
> On 16.09.2015 02:58, Andreas Dannenberg wrote:
> > 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 | 44 +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 44 insertions(+)
> > 
> > diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c
> > index 47af858..84a8945 100644
> > --- a/drivers/power/bq24257_charger.c
> > +++ b/drivers/power/bq24257_charger.c
> > @@ -76,6 +76,7 @@ struct bq24257_init_data {
> >  	u8 iterm;	/* termination current */
> >  	u8 in_ilimit;	/* input current limit */
> >  	u8 vovp;	/* over voltage protection voltage */
> > +	u8 vindpm;	/* VDMP input threshold voltage */
> >  };
> >  
> >  struct bq24257_state {
> > @@ -206,6 +207,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)
> >  {
> > @@ -432,6 +440,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
> > +};
> 
> Same as patch 6/10... but now see the use case - to choose default value
> not by index of array but from this enum.
> 
> I see the idea behind so skip that comment from 6/10.

OK.

> > +
> >  enum bq24257_port_type {
> >  	PORT_TYPE_DCP,		/* Dedicated Charging Port */
> >  	PORT_TYPE_CDP,		/* Charging Downstream Port */
> > @@ -615,6 +634,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},
> >  	};
> >  
> >  	/*
> > @@ -648,6 +668,7 @@ static int bq24257_hw_init(struct bq24257_device *bq)
> >  		/* program fixed input current limit */
> >  		ret = bq24257_field_write(bq, F_IILIMIT,
> >  					  bq->init_data.in_ilimit);
> > +
> 
> Not part of this patch. It confuses the reviewer of patch. If you need
> cleanups, make them separate.

Artifact. This should not be there.

> >  		if (ret < 0)
> >  			return ret;
> >  	} else if (!state.power_good)
> > @@ -695,10 +716,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, "%d\n",
> > +			 bq24257_vindpm_map[bq->init_data.vindpm]);
> 
> Same as patch 6/10.

OK.

Regards,

--
Andreas Dannenberg
Texas Instruments Inc

> 
> Best regards,
> Krzysztof
> 
> > +}
> > +
> >  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,
> >  };
> >  
> > @@ -801,6 +835,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;
> >  }
> >  
> > 
> 

Re: [PATCH v4 08/10] power: bq24257: Allow input current limit sysfs access

[Andreas Dannenberg]

On Wed, Sep 16, 2015 at 03:31:06PM +0900, Krzysztof Kozlowski wrote:
> On 16.09.2015 02:58, 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_PROP_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@ti.com>
> > ---
> >  drivers/power/bq24257_charger.c | 67 +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 67 insertions(+)
> > 
> 
> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> 
> BTW you are exposing more and more sysfs attributes which are userspace
> interfaces. They should be documented:
> Documentation/SubmitChecklist - point 19
> Documentation/ABI/README

The patch comment is not 100% accurate. I'm not declaring/exposing a new
sysfs property but rather use the power supply framework to expose the
sysfs property that's associated with POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT
which is actually called "input_current_limit". Will reword the description
to make this clearer.

Regards,

--
Andreas Dannenberg
Texas Instruments Inc

> 
> Although not all new drivers (and perhaps not all reviewers) follow this
> rule...
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v4 09/10] power: bq24257: Add various device-specific sysfs properties

[Andreas Dannenberg]

On Wed, Sep 16, 2015 at 05:10:06PM +0900, Krzysztof Kozlowski wrote:
> On 16.09.2015 02:58, Andreas Dannenberg wrote:
> > 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 517a522..a0cb33c 100644
> > --- a/drivers/power/bq24257_charger.c
> > +++ b/drivers/power/bq24257_charger.c
> > @@ -794,12 +794,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;
> > +
> 
> There is no validation for input number. Although this is harmless but
> one may expect that writing value of 1, 2 or 3 would give the same
> result. But it would not. Value of 2 would be equal to 0, right?

You are right 2 would result in 0. Changing where the value gets passed
into bq24257_field_write() from "val" to "(bool)val" will make this more
forgiving.

> > +	if (strcmp(attr->attr.name, "high_impedance_enable") == 0)
> > +		ret = bq24257_field_write(bq, F_HZ_MODE, val);
> > +	else if (strcmp(attr->attr.name, "sysoff_enable") == 0)
> > +		ret = bq24257_field_write(bq, F_SYSOFF, 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);
> 
> This applies to previous patches actually: DEVICE_ATTR_RO?

Ok. Will simplify.

> 
> > +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);
> 
> DEVICE_ATTR_RW?

Ditto.

Regards,

--
Andreas Dannenberg
Texas Instruments Inc

Re: [PATCH v4 10/10] power: bq24257: Add platform data based initialization

[Andreas Dannenberg]

On Wed, Sep 16, 2015 at 05:31:58PM +0900, Krzysztof Kozlowski wrote:
> On 16.09.2015 02:58, 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.
> > 
> > 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 a0cb33c..fa18b1f 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
> > @@ -877,28 +880,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,
> > +					    GPIOD_IN, BQ24257_STAT_IRQ);
> 
> That should be GPIOF_* flag, not GPIOD.

Good catch. Looks like it worked because the definitions happen to result
in the same value.

> > +		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,
> > +					    GPIOD_IN, BQ24257_PG_GPIO);
> 
> ditto

OK.
 
> > +		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.in_ilimit = bq24257_find_idx(pdata->in_ilimit,
> > +						   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->in_ilimit_autoset_disable = pdata->in_ilimit_autoset_disable;
> > +
> > +	bq->pg_gpio_disable = pdata->pg_gpio_disable;
> > +}
> > +
> >  static int bq24257_fw_probe(struct bq24257_device *bq)
> >  {
> >  	int ret;
> > @@ -974,6 +1061,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;
> > @@ -1023,14 +1111,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);
> >  	}
> >  
> >  	/*
> > @@ -1087,7 +1176,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.
> > +	 */
> 
> Why?

The original driver would allow configuring the IRQ either through
client->irq or through a separete DT entry called "stat_gpios" (which
seems redundant... might it be required for ACPI? Or...?). I modeled the
configuration via pdata the same way, this way aligning the fields in
the pdata structure with the DT entries. Let me know if you have a
preference to simplify this.

> 
> > +	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..d4f3703
> > --- /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 in_ilimit;				  /* input current limit (uA) */
> > +	u32 vovp;		      /* over voltage protection voltage (uV) */
> > +	u32 vindpm;			 /* VDMP input threshold voltage (uV) */
> > +	bool in_ilimit_autoset_disable;	/* 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
> 
> #endif /* _BQ24257_CHARGER_H_ */

Ok.

Regards,

--
Andreas Dannenberg
Texas Instruments Inc

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v4 04/10] power: bq24257: Allow manual setting of input current limit

[Krzysztof Kozlowski]

On 17.09.2015 04:23, Andreas Dannenberg wrote:
> On Wed, Sep 16, 2015 at 09:41:49AM +0900, Krzysztof Kozlowski wrote:
>>> -		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->in_ilimit_autoset_disable;
>>> +	} else if (new_state->fault == FAULT_NO_BAT) {
>>> +		dev_warn(bq->dev, "Battery removed\n");
>>
>> dev_warn? This wasn't here before... It's a bit too serious. Is removing
>> a battery an error condition? Maybe user just unplugged it?
>> dev_dbg or dev_info should be sufficient.
>>
>> BTW, it is useful to quickly find boot regressions with "dmesg -l
>> warn,err". Removing a battery probably is not an error?
> 
> I would argue that most devices that use a Li-Ion battery have the
> battery built-in and it's not user removable. Therefore, if the battery
> would ever go disconnected I figured it'll most likely be something
> serious such as a contact breaking loose or something else dramatic,
> warranting at least a warning. Plus, many devices with built in Li-Ion
> batteries are actually designed in a way that they don't really function
> properly with the battery taken out as the HW is often designed to draw
> supplemental current from the battery during times of load in addition
> to the A/C supply (key feature of many charger chips).

Okay, I guess if there ever will be an user annoyed by dmesg's after
removing the battery we can always revisit this. :)

> 
>>
>>> +		if (!bq->in_ilimit_autoset_disable) {
>>> +			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");
>>
>> Definitely not a warn. Inserting a battery is not an error condition.
> 
> Same as above.

OK

> 
>>> +		config_iilimit = !bq->in_ilimit_autoset_disable;
>>> +	} else if (new_state->fault == FAULT_TIMER) {
>>>  		dev_err(bq->dev, "Safety timer expired! Battery dead?\n");
>>>  	}
>>
>> Don't you have a schedule_delayed_work() call here which now will be
>> executed always? Even when work was not INIT and nothing will cancel it?
> 
> It'll be more obvious when looking at the merged code but the schedule_
> delayed_work() call only happens if config_iilimit==true which only
> happens when the input limit current autoset functionality is not
> disabled. If that's the case (autoset functionality is enabled) the INIT
> for that work is executed during probe.
>  

OK

>>>  
>>> @@ -581,7 +601,16 @@ static int bq24257_hw_init(struct bq24257_device *bq)
>>>  	bq->state = state;
>>>  	mutex_unlock(&bq->lock);
>>>  
>>> -	if (!state.power_good)
>>> +	if (bq->in_ilimit_autoset_disable) {
>>> +		dev_dbg(bq->dev, "manually setting iilimit = %d\n",
>>> +			bq->init_data.in_ilimit);
>>
>> Nit, no actual difference but makes more sense to me because field is
>> u8: "%u".
> 
> Yes that should be changed.
> 
>>> +
>>> +		/* program fixed input current limit */
>>> +		ret = bq24257_field_write(bq, F_IILIMIT,
>>> +					  bq->init_data.in_ilimit);
>>> +		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)
>>> @@ -659,6 +688,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;
>>> @@ -682,6 +712,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)
>>> +		/*
>>> +		 * 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.in_ilimit = IILIMIT_500;
>>> +	else {
>>> +		bq->in_ilimit_autoset_disable = true;
>>> +		bq->init_data.in_ilimit =
>>> +				bq24257_find_idx(property,
>>> +						 bq24257_iilimit_map,
>>> +						 BQ24257_IILIMIT_MAP_SIZE);
>>> +	}
>>> +
>>>  	return 0;
>>>  }
>>>  
>>> @@ -740,8 +788,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) {
>>> @@ -752,6 +798,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->in_ilimit_autoset_disable = true;
>>> +
>>> +	if (!bq->in_ilimit_autoset_disable)
>>
>> In most places you have quite obfuscated negation of
>> "autoset_disable"... So maybe just "bq->in_ilimit_autoset" or
>> "bq->in_ilimit_autoset_enable" and the negation won't be needed? It
>> could be more readable.
> 
> This stems from the fact that this was initially tied to a boolean DT
> property with the same name which is no longer there. The other reason
> was setting this property to non-zero changes the default behavior of
> the driver (that used to have autoset always enabled) so I wanted to
> reflect this behavior in the logic. The driver was tested pretty well so
> unless you feel strongly about this I would rather leave it as-is.

IMHO the code would be more readable but I don't insist.

Best regards,
Krzysztof

Re: [PATCH v4 10/10] power: bq24257: Add platform data based initialization

[Krzysztof Kozlowski]

On 17.09.2015 05:11, Andreas Dannenberg wrote:
> On Wed, Sep 16, 2015 at 05:31:58PM +0900, Krzysztof Kozlowski wrote:
>>> @@ -1087,7 +1176,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.
>>> +	 */
>>
>> Why?
> 
> The original driver would allow configuring the IRQ either through
> client->irq or through a separete DT entry called "stat_gpios" (which
> seems redundant... might it be required for ACPI? Or...?). I modeled the
> configuration via pdata the same way, this way aligning the fields in
> the pdata structure with the DT entries. Let me know if you have a
> preference to simplify this.
> 

Right, it makes sense now.

Best regards,
Krzysztof

Re: [PATCH v4 09/10] power: bq24257: Add various device-specific sysfs properties

[Andreas Dannenberg]

On Wed, Sep 16, 2015 at 02:54:53PM -0500, Andreas Dannenberg wrote:
> On Wed, Sep 16, 2015 at 05:10:06PM +0900, Krzysztof Kozlowski wrote:
> > On 16.09.2015 02:58, Andreas Dannenberg wrote:
> > >  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);
> > 
> > This applies to previous patches actually: DEVICE_ATTR_RO?
> 
> Ok. Will simplify.

Actually I looked into this more and realized that the use of the
pre-configured DEVICE_ATTR_* definitions imposes certain function names,
which would mean renaming the functions including dropping the bq24257_
prefix those function names have, making the code inconsistent and
decreasing the unique-ness of those function names in the global Kernel
namespace. So I rather leave the more general DEVICE_ATTR() definitions
in place.

Regards,

--
Andreas Dannenberg
Texas Instruments Inc

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

[Andreas Dannenberg]

On Wed, Sep 16, 2015 at 02:41:13PM +0900, Krzysztof Kozlowski wrote:
> On 16.09.2015 02:58, Andreas Dannenberg wrote:
> > @@ -676,7 +696,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");
> 
> I think if pg-gpio is provided (e.g. by DTS) but it is invalid (return
> value != ENOENT) then it is an error you could print. The driver will
> fallback to the software method but still user/developer may want to
> notice the error (e.g. error in DTS).
> 
> Anyway it is up to you, rest looks good:

Krzysztof,
I looked at this closer and all the error scenarios I could come up with
to test regarding the pin definition in DT (wrong pin numbers, missing
definition altogether) all generated an -ENOENT coming out of
devm_gpiod_get_index() with no differentiation between the cases. I'm
sure there is some way to accomplish that but for the time being I left
the refinement between dev_err() and dev_info() alone.

Regards,

--
Andreas Dannenberg
Texas Instruments Inc