[PATCH v2 00/13] power: bq24257: Add support for bq24250/bq24251

[PATCH v2 00/13] 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.

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 (13):
  power: bq24257: Add bit definition for temp sense enable
  power: bq24257: Add basic support for bq24250/bq24251
  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: Extend scope of mutex protection
  power: bq24257: Add charge type 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
  power: bq24257: Renaming for consistency
  dt: power: bq2425x-charger: Cover additional devices

 .../devicetree/bindings/power/bq24257.txt          |   21 -
 .../devicetree/bindings/power/bq2425x.txt          |   70 +
 drivers/power/Kconfig                              |    7 +-
 drivers/power/Makefile                             |    2 +-
 drivers/power/bq24257_charger.c                    |  858 ------------
 drivers/power/bq2425x_charger.c                    | 1404 ++++++++++++++++++++
 include/linux/power/bq2425x_charger.h              |   30 +
 7 files changed, 1509 insertions(+), 883 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/power/bq24257.txt
 create mode 100644 Documentation/devicetree/bindings/power/bq2425x.txt
 delete mode 100644 drivers/power/bq24257_charger.c
 create mode 100644 drivers/power/bq2425x_charger.c
 create mode 100644 include/linux/power/bq2425x_charger.h

-- 
1.9.1

[PATCH v2 01/13] 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 5859bc7..db81356 100644
--- a/drivers/power/bq24257_charger.c
+++ b/drivers/power/bq24257_charger.c
@@ -47,7 +47,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
@@ -135,6 +135,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

Re: [PATCH v2 01/13] power: bq24257: Add bit definition for temp sense enable

[Laurentiu Palcu]

On Tue, Sep 08, 2015 at 07:12:25PM -0500, Andreas Dannenberg wrote:
> 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>
Reviewed-by: Laurentiu Palcu <laurentiu.palcu@intel.com>

[PATCH v2 02/13] power: bq24257: Add basic support for bq24250/bq24251

[Andreas Dannenberg]

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

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

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

diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index 08beeed..fa9b1d1 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -396,11 +396,12 @@ 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 BQ2425x battery charger driver"
 	depends on I2C && GPIOLIB
 	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 db81356..670ca57 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 *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];
 
@@ -250,6 +268,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;
@@ -570,6 +592,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,
@@ -667,6 +690,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;
@@ -683,6 +707,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);
@@ -824,19 +860,27 @@ 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,bq24257", },
+	{
+		.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

Re: [PATCH v2 02/13] power: bq24257: Add basic support for bq24250/bq24251

[Laurentiu Palcu]

On Tue, Sep 08, 2015 at 07:12:26PM -0500, Andreas Dannenberg wrote:
[...]
>  
>  #include <linux/module.h>
> @@ -41,6 +45,18 @@
>  
>  #define BQ24257_ILIM_SET_DELAY		1000	/* msec */
>  
> +enum bq2425x_chip {
> +	BQ24250,
> +	BQ24251,
> +	BQ24257,
> +};
> +
> +static const char *bq2425x_chip_name[] = {
static const char *const bq2425x_chip_name[]

That's safer and it also makes checkpatch happy.

> +	"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];
>  
> @@ -250,6 +268,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;
> @@ -570,6 +592,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,
> @@ -667,6 +690,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;
> @@ -683,6 +707,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);
> @@ -824,19 +860,27 @@ 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,bq24257", },
> +	{
> +		.compatible = "ti,bq24250",
> +		.compatible = "ti,bq24251",
> +		.compatible = "ti,bq24257",
> +	},
Maybe you wanted:
	{ .compatible = "ti,bq24250", },
	{ .compatible = "ti,bq24251", },
	{ .compatible = "ti,bq24252", },

>  	{ },
>  };

BTW, ACPI enumeration works fine. I tested the series on my setup
(before starting to review the patches :D).

laurentiu

Re: [PATCH v2 02/13] power: bq24257: Add basic support for bq24250/bq24251

[Andreas Dannenberg]

On Thu, Sep 10, 2015 at 03:42:11PM +0300, Laurentiu Palcu wrote:
> On Tue, Sep 08, 2015 at 07:12:26PM -0500, Andreas Dannenberg wrote:
> [...]
> >  
> >  #include <linux/module.h>
> > @@ -41,6 +45,18 @@
> >  
> >  #define BQ24257_ILIM_SET_DELAY		1000	/* msec */
> >  
> > +enum bq2425x_chip {
> > +	BQ24250,
> > +	BQ24251,
> > +	BQ24257,
> > +};
> > +
> > +static const char *bq2425x_chip_name[] = {
> static const char *const bq2425x_chip_name[]
> 
> That's safer and it also makes checkpatch happy.

Ok changed.

> > +	"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];
> >  
> > @@ -250,6 +268,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;
> > @@ -570,6 +592,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,
> > @@ -667,6 +690,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;
> > @@ -683,6 +707,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);
> > @@ -824,19 +860,27 @@ 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,bq24257", },
> > +	{
> > +		.compatible = "ti,bq24250",
> > +		.compatible = "ti,bq24251",
> > +		.compatible = "ti,bq24257",
> > +	},
> Maybe you wanted:
> 	{ .compatible = "ti,bq24250", },
> 	{ .compatible = "ti,bq24251", },
> 	{ .compatible = "ti,bq24252", },

Yes that's what I wanted. Interestingly in my testing the device
configured in the device tree was identified correctly, but I suppose
this worked because of some other fallback mechanism (maybe using the
i2c_device_id?). Anyways it's fixed now.

> >  	{ },
> >  };
> 
> BTW, ACPI enumeration works fine. I tested the series on my setup
> (before starting to review the patches :D).

Thanks for going the extra mile and testing ACPI support, I really
appreciate it. I should get my MinnowBoard on Saturday (it'll make for a
fun weekend!) so I should be able to do such testing for this and other
TI drivers myself.

--
Andreas Dannenberg
Texas Instruments Inc

> 
> laurentiu

[PATCH v2 03/13] 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 | 100 +++++++++++++++++++++++++++++++---------
 1 file changed, 79 insertions(+), 21 deletions(-)

diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c
index 670ca57..d83898f 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,22 @@ 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 +786,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 +796,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 +860,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 +876,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

Re: [PATCH v2 03/13] power: bq24257: Allow manual setting of input current limit

[Laurentiu Palcu]

On Tue, Sep 08, 2015 at 07:12:27PM -0500, 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-l0cyMroinI0@public.gmane.org>
Reviewed-by: Laurentiu Palcu <laurentiu.palcu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
--
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 v2 04/13] 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@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 d83898f..e309ae8 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);
 	}
 
@@ -808,10 +828,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

Re: [PATCH v2 04/13] power: bq24257: Add SW-based approach for Power Good determination

[Laurentiu Palcu]

On Tue, Sep 08, 2015 at 07:12:28PM -0500, Andreas Dannenberg wrote:
> A software-based approach for determining the charger's input voltage
> "Power Good" state is introduced for devices like the bq24250 which
> don't have a dedicated hardware pin for that purpose. This SW-based
> approach is also used for other devices (with dedicated PG pin) as a
> fall back solution if that pin is not configured to be used through
> "pg-gpios".
> 
> Signed-off-by: Andreas Dannenberg <dannenberg-l0cyMroinI0@public.gmane.org>
Reviewed-by: Laurentiu Palcu <laurentiu.palcu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
--
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 v2 05/13] 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 | 66 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 63 insertions(+), 3 deletions(-)

diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c
index e309ae8..917609b 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, };
@@ -748,6 +790,14 @@ static int bq24257_fw_probe(struct bq24257_device *bq)
 				bq24257_iilimit_map, BQ24257_IILIMIT_MAP_SIZE);
 	}
 
+	ret = device_property_read_u32(bq->dev, "ti,ovp-voltage",
+				       &property);
+	if (ret < 0)
+		bq->init_data.vovp = VOVP_6500;
+	else
+		bq->init_data.vovp = bq24257_find_idx(property,
+				bq24257_vovp_map, BQ24257_VOVP_MAP_SIZE);
+
 	return 0;
 }
 
@@ -887,10 +937,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)
@@ -900,6 +958,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

Re: [PATCH v2 05/13] power: bq24257: Add over voltage protection setting support

[Laurentiu Palcu]

On Tue, Sep 08, 2015 at 07:12:29PM -0500, 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-l0cyMroinI0@public.gmane.org>
Reviewed-by: Laurentiu Palcu <laurentiu.palcu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
--
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 v2 06/13] 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 | 42 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c
index 917609b..c160fde 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,
 };
 
@@ -798,6 +832,14 @@ static int bq24257_fw_probe(struct bq24257_device *bq)
 		bq->init_data.vovp = bq24257_find_idx(property,
 				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

Re: [PATCH v2 06/13] power: bq24257: Add input DPM voltage threshold setting support

[Laurentiu Palcu]

On Tue, Sep 08, 2015 at 07:12:30PM -0500, 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>
Reviewed-by: Laurentiu Palcu <laurentiu.palcu@intel.com>

[PATCH v2 07/13] power: bq24257: Extend scope of mutex protection

[Andreas Dannenberg]

This commit extends the use of the existing mutex to pave the way for
using direct device access inside sysfs getter/setter routines in a
way that the access during interrupts and timer routines does not
interfere with device access by the CPU or between multiple cores.

This also addresses a potential race condition that could be caused
by bq24257_irq_handler_thread() and bq24257_iilimit_autoset() accessing
the device simultaneously.

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

diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c
index c160fde..f0d4307 100644
--- a/drivers/power/bq24257_charger.c
+++ b/drivers/power/bq24257_charger.c
@@ -102,7 +102,7 @@ struct bq24257_device {
 	struct bq24257_init_data init_data;
 	struct bq24257_state state;
 
-	struct mutex lock; /* protect state data */
+	struct mutex lock; /* protect device access and state data */
 
 	bool in_ilimit_autoset_disable;
 	bool pg_gpio_disable;
@@ -271,10 +271,10 @@ static int bq24257_power_supply_get_property(struct power_supply *psy,
 {
 	struct bq24257_device *bq = power_supply_get_drvdata(psy);
 	struct bq24257_state state;
+	int ret = 0;
 
 	mutex_lock(&bq->lock);
 	state = bq->state;
-	mutex_unlock(&bq->lock);
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_STATUS:
@@ -350,10 +350,12 @@ static int bq24257_power_supply_get_property(struct power_supply *psy,
 		break;
 
 	default:
-		return -EINVAL;
+		ret = -EINVAL;
 	}
 
-	return 0;
+	mutex_unlock(&bq->lock);
+
+	return ret;
 }
 
 static int bq24257_get_chip_state(struct bq24257_device *bq,
@@ -400,15 +402,9 @@ static int bq24257_get_chip_state(struct bq24257_device *bq,
 static bool bq24257_state_changed(struct bq24257_device *bq,
 				  struct bq24257_state *new_state)
 {
-	int ret;
-
-	mutex_lock(&bq->lock);
-	ret = (bq->state.status != new_state->status ||
-	       bq->state.fault != new_state->fault ||
-	       bq->state.power_good != new_state->power_good);
-	mutex_unlock(&bq->lock);
-
-	return ret;
+	return bq->state.status != new_state->status ||
+			bq->state.fault != new_state->fault ||
+			bq->state.power_good != new_state->power_good;
 }
 
 enum bq24257_loop_status {
@@ -531,7 +527,9 @@ static void bq24257_iilimit_setup_work(struct work_struct *work)
 	struct bq24257_device *bq = container_of(work, struct bq24257_device,
 						 iilimit_setup_work.work);
 
+	mutex_lock(&bq->lock);
 	bq24257_iilimit_autoset(bq);
+	mutex_unlock(&bq->lock);
 }
 
 static void bq24257_handle_state_change(struct bq24257_device *bq,
@@ -542,9 +540,7 @@ static void bq24257_handle_state_change(struct bq24257_device *bq,
 	bool reset_iilimit = false;
 	bool config_iilimit = false;
 
-	mutex_lock(&bq->lock);
 	old_state = bq->state;
-	mutex_unlock(&bq->lock);
 
 	/*
 	 * Handle BQ2425x state changes observing whether the D+/D- based input
@@ -599,25 +595,30 @@ static irqreturn_t bq24257_irq_handler_thread(int irq, void *private)
 	struct bq24257_device *bq = private;
 	struct bq24257_state state;
 
+	mutex_lock(&bq->lock);
+
 	ret = bq24257_get_chip_state(bq, &state);
 	if (ret < 0)
-		return IRQ_HANDLED;
+		goto out;
 
 	if (!bq24257_state_changed(bq, &state))
-		return IRQ_HANDLED;
+		goto out;
 
 	dev_dbg(bq->dev, "irq(state changed): status/fault/pg = %d/%d/%d\n",
 		state.status, state.fault, state.power_good);
 
 	bq24257_handle_state_change(bq, &state);
-
-	mutex_lock(&bq->lock);
 	bq->state = state;
+
 	mutex_unlock(&bq->lock);
 
 	power_supply_changed(bq->charger);
 
 	return IRQ_HANDLED;
+
+out:
+	mutex_unlock(&bq->lock);
+	return IRQ_HANDLED;
 }
 
 static int bq24257_hw_init(struct bq24257_device *bq)
@@ -657,9 +658,7 @@ static int bq24257_hw_init(struct bq24257_device *bq)
 	if (ret < 0)
 		return ret;
 
-	mutex_lock(&bq->lock);
 	bq->state = state;
-	mutex_unlock(&bq->lock);
 
 	if (bq->in_ilimit_autoset_disable) {
 		dev_dbg(bq->dev, "manually setting iilimit = %d\n",
@@ -1018,11 +1017,15 @@ static int bq24257_suspend(struct device *dev)
 	if (!bq->in_ilimit_autoset_disable)
 		cancel_delayed_work_sync(&bq->iilimit_setup_work);
 
+	mutex_lock(&bq->lock);
+
 	/* reset all registers to default (and activate standalone mode) */
 	ret = bq24257_field_write(bq, F_RESET, 1);
 	if (ret < 0)
 		dev_err(bq->dev, "Cannot reset chip to standalone mode.\n");
 
+	mutex_unlock(&bq->lock);
+
 	return ret;
 }
 
@@ -1031,24 +1034,31 @@ static int bq24257_resume(struct device *dev)
 	int ret;
 	struct bq24257_device *bq = dev_get_drvdata(dev);
 
+	mutex_lock(&bq->lock);
+
 	ret = regcache_drop_region(bq->rmap, BQ24257_REG_1, BQ24257_REG_7);
 	if (ret < 0)
-		return ret;
+		goto err;
 
 	ret = bq24257_field_write(bq, F_RESET, 0);
 	if (ret < 0)
-		return ret;
+		goto err;
 
 	ret = bq24257_hw_init(bq);
 	if (ret < 0) {
 		dev_err(bq->dev, "Cannot init chip after resume.\n");
-		return ret;
+		goto err;
 	}
 
+	mutex_unlock(&bq->lock);
+
 	/* signal userspace, maybe state changed while suspended */
 	power_supply_changed(bq->charger);
-
 	return 0;
+
+err:
+	mutex_unlock(&bq->lock);
+	return ret;
 }
 #endif
 
-- 
1.9.1

Re: [PATCH v2 07/13] power: bq24257: Extend scope of mutex protection

[Laurentiu Palcu]

On Tue, Sep 08, 2015 at 07:12:31PM -0500, Andreas Dannenberg wrote:
> This commit extends the use of the existing mutex to pave the way for
> using direct device access inside sysfs getter/setter routines in a
> way that the access during interrupts and timer routines does not
> interfere with device access by the CPU or between multiple cores.
> 
> This also addresses a potential race condition that could be caused
> by bq24257_irq_handler_thread() and bq24257_iilimit_autoset() accessing
> the device simultaneously.
What potential race? AFAIK, we're doing all the operations through
regmap, and regmap operations are serialized. It has its own mutex for
this. Unless I got it all wrong...

So, IMHO, you don't need to protect against simultaneous device access.
It's taken care of by regmap. Chip state data protection should be enough.

laurentiu

Re: [PATCH v2 07/13] power: bq24257: Extend scope of mutex protection

[Andreas Dannenberg]

On Thu, Sep 10, 2015 at 04:43:10PM +0300, Laurentiu Palcu wrote:
> On Tue, Sep 08, 2015 at 07:12:31PM -0500, Andreas Dannenberg wrote:
> > This commit extends the use of the existing mutex to pave the way for
> > using direct device access inside sysfs getter/setter routines in a
> > way that the access during interrupts and timer routines does not
> > interfere with device access by the CPU or between multiple cores.
> > 
> > This also addresses a potential race condition that could be caused
> > by bq24257_irq_handler_thread() and bq24257_iilimit_autoset() accessing
> > the device simultaneously.
> What potential race? AFAIK, we're doing all the operations through
> regmap, and regmap operations are serialized. It has its own mutex for
> this. Unless I got it all wrong...
> 
> So, IMHO, you don't need to protect against simultaneous device access.
> It's taken care of by regmap. Chip state data protection should be enough.

You are right, closely inspecting regmap.c shows that this driver indeed
uses mutexes (and instantiates them during init if needed) which is a
fantastic feature. The reasons for my proposed mutex changes were based
on the assumption that there is no built-in protection. Since this
assumption is wrong the potential race condition I pointed out in the
original code doesn't exist and I also don't see a need despite the
additions I make in this patch series to add/change the existing mutex
scheme. Will drop that patch.

Thanks,

--
Andreas Dannenberg
Texas Instruments Inc

> 
> laurentiu
> 
> 

[PATCH v2 08/13] power: bq24257: Add charge type setting support

[Andreas Dannenberg]

The bq2425x devices support charging with a configurable charge current
i_chg or a specific fixed low-current in what's called Low Charge mode.
This patch exposes access to the Low Charge mode through a POWER_
SUPPLY_PROP_CHARGE_TYPE sysfs property value of POWER_SUPPLY_CHARGE_
TYPE_TRICKLE. It also allows to completely disable charging if desired
by setting the property to POWER_SUPPLY_CHARGE_TYPE_NONE. Normal
charging with the configured i_chg current is activated through
POWER_SUPPLY_CHARGE_TYPE_FAST (default).

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

diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c
index f0d4307..886040c 100644
--- a/drivers/power/bq24257_charger.c
+++ b/drivers/power/bq24257_charger.c
@@ -265,6 +265,56 @@ enum bq24257_fault {
 	FAULT_INPUT_LDO_LOW,
 };
 
+static int bq24257_get_charge_type(struct bq24257_device *bq,
+		union power_supply_propval *val)
+{
+	int ret;
+
+	ret = bq24257_field_read(bq, F_CE);
+	if (ret < 0)
+		return ret;
+
+	/* Charge Enable is active-low */
+	if (ret) {
+		val->intval = POWER_SUPPLY_CHARGE_TYPE_NONE;
+		return 0;
+	}
+
+	/* Low Charge means a fixed low-current is used instead of i_chg */
+	ret = bq24257_field_read(bq, F_LOW_CHG);
+	if (ret < 0)
+		return ret;
+
+	val->intval = ret ? POWER_SUPPLY_CHARGE_TYPE_TRICKLE :
+			POWER_SUPPLY_CHARGE_TYPE_FAST;
+
+	return 0;
+}
+
+static int bq24257_set_charge_type(struct bq24257_device *bq,
+		const union power_supply_propval *val)
+{
+	int ret;
+
+	switch (val->intval) {
+	case POWER_SUPPLY_CHARGE_TYPE_NONE:
+		return bq24257_field_write(bq, F_CE, 1);
+	case POWER_SUPPLY_CHARGE_TYPE_TRICKLE:
+		ret = bq24257_field_write(bq, F_LOW_CHG, 1);
+		break;
+	case POWER_SUPPLY_CHARGE_TYPE_FAST:
+		ret = bq24257_field_write(bq, F_LOW_CHG, 0);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (ret < 0)
+		return ret;
+
+	return bq24257_field_write(bq, F_CE, 0);
+}
+
 static int bq24257_power_supply_get_property(struct power_supply *psy,
 					     enum power_supply_property psp,
 					     union power_supply_propval *val)
@@ -290,6 +340,10 @@ static int bq24257_power_supply_get_property(struct power_supply *psy,
 			val->intval = POWER_SUPPLY_STATUS_UNKNOWN;
 		break;
 
+	case POWER_SUPPLY_PROP_CHARGE_TYPE:
+		ret = bq24257_get_charge_type(bq, val);
+		break;
+
 	case POWER_SUPPLY_PROP_MANUFACTURER:
 		val->strval = BQ24257_MANUFACTURER;
 		break;
@@ -358,6 +412,39 @@ static int bq24257_power_supply_get_property(struct power_supply *psy,
 	return ret;
 }
 
+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);
+	int ret;
+
+	mutex_lock(&bq->lock);
+
+	switch (prop) {
+	case POWER_SUPPLY_PROP_CHARGE_TYPE:
+		ret = bq24257_set_charge_type(bq, val);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
+	mutex_unlock(&bq->lock);
+
+	return ret;
+}
+
+static int bq24257_power_supply_property_is_writeable(struct power_supply *psy,
+		enum power_supply_property psp)
+{
+	switch (psp) {
+	case POWER_SUPPLY_PROP_CHARGE_TYPE:
+		return true;
+	default:
+		return false;
+	}
+}
+
 static int bq24257_get_chip_state(struct bq24257_device *bq,
 				  struct bq24257_state *state)
 {
@@ -683,6 +770,7 @@ 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_CHARGE_TYPE,
 	POWER_SUPPLY_PROP_ONLINE,
 	POWER_SUPPLY_PROP_HEALTH,
 	POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT,
@@ -702,6 +790,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

Re: [PATCH v2 08/13] power: bq24257: Add charge type setting support

[Laurentiu Palcu]

On Tue, Sep 08, 2015 at 07:12:32PM -0500, Andreas Dannenberg wrote:
> The bq2425x devices support charging with a configurable charge current
> i_chg or a specific fixed low-current in what's called Low Charge mode.
> This patch exposes access to the Low Charge mode through a POWER_
> SUPPLY_PROP_CHARGE_TYPE sysfs property value of POWER_SUPPLY_CHARGE_
> TYPE_TRICKLE. It also allows to completely disable charging if desired
> by setting the property to POWER_SUPPLY_CHARGE_TYPE_NONE. Normal
> charging with the configured i_chg current is activated through
> POWER_SUPPLY_CHARGE_TYPE_FAST (default).
> 
> Signed-off-by: Andreas Dannenberg <dannenberg-l0cyMroinI0@public.gmane.org>

I have some questions here. Are there any cases in the wild, when
userspace selects the charging type (apart from testing purposes)? And
if yes, how is userspace supposed to detect/decide when to switch from
trickle to fast?

I have the strange feeling that we're adding properties that will never
be used... Other people's opinions are welcomed too.

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

Re: [PATCH v2 08/13] power: bq24257: Add charge type setting support

[Andreas Dannenberg]

On Thu, Sep 10, 2015 at 04:56:22PM +0300, Laurentiu Palcu wrote:
> On Tue, Sep 08, 2015 at 07:12:32PM -0500, Andreas Dannenberg wrote:
> > The bq2425x devices support charging with a configurable charge current
> > i_chg or a specific fixed low-current in what's called Low Charge mode.
> > This patch exposes access to the Low Charge mode through a POWER_
> > SUPPLY_PROP_CHARGE_TYPE sysfs property value of POWER_SUPPLY_CHARGE_
> > TYPE_TRICKLE. It also allows to completely disable charging if desired
> > by setting the property to POWER_SUPPLY_CHARGE_TYPE_NONE. Normal
> > charging with the configured i_chg current is activated through
> > POWER_SUPPLY_CHARGE_TYPE_FAST (default).
> > 
> > Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> 
> I have some questions here. Are there any cases in the wild, when
> userspace selects the charging type (apart from testing purposes)? And
> if yes, how is userspace supposed to detect/decide when to switch from
> trickle to fast?
> 
> I have the strange feeling that we're adding properties that will never
> be used... Other people's opinions are welcomed too.

Hi Laurentiu. I just talked to our product people to see as to how the
underlying LOW_CHG feature would be used and they essentially said its a
legacy feature used by only one customer in the past and that it doesn't
need to get exposed in our driver. I had initially implemented it in
analogy with the bq24190_charger.c driver that exposes a somewhat
similar feature but if nobody needs it from a TI point of view I'm ok
dropping it. Unless somebody screams they want it :)

Regards,

--
Andreas Dannenberg
Texas Instruments Inc


> 
> laurentiu

[PATCH v2 09/13] 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 | 44 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c
index 886040c..f0602b3 100644
--- a/drivers/power/bq24257_charger.c
+++ b/drivers/power/bq24257_charger.c
@@ -315,6 +315,41 @@ static int bq24257_set_charge_type(struct bq24257_device *bq,
 	return bq24257_field_write(bq, F_CE, 0);
 }
 
+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)
@@ -403,6 +438,10 @@ 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:
+		ret = bq24257_get_input_current_limit(bq, val);
+		break;
+
 	default:
 		ret = -EINVAL;
 	}
@@ -425,6 +464,9 @@ static int bq24257_power_supply_set_property(struct power_supply *psy,
 	case POWER_SUPPLY_PROP_CHARGE_TYPE:
 		ret = bq24257_set_charge_type(bq, val);
 		break;
+	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
+		ret = bq24257_set_input_current_limit(bq, val);
+		break;
 	default:
 		ret = -EINVAL;
 	}
@@ -439,6 +481,7 @@ static int bq24257_power_supply_property_is_writeable(struct power_supply *psy,
 {
 	switch (psp) {
 	case POWER_SUPPLY_PROP_CHARGE_TYPE:
+	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
 		return true;
 	default:
 		return false;
@@ -778,6 +821,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[] = {
-- 
1.9.1

Re: [PATCH v2 09/13] power: bq24257: Allow input current limit sysfs access

[Laurentiu Palcu]

On Tue, Sep 08, 2015 at 07:12:33PM -0500, Andreas Dannenberg wrote:
> This patch allows reading (and writing, if the D+/D- USB signal-based
> charger type detection is disabled) of the input current limit through
> the POWER_SUPPLY_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>
Reviewed-by: Laurentiu Palcu <laurentiu.palcu@intel.com>

[PATCH v2 12/13] power: bq24257: Renaming for consistency

[Andreas Dannenberg]

Rename files and refactor definitions, function names, etc as well as
the driver name itself to reflect that this driver supports multiple
devices and not just the bq24257.

Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
---
 drivers/power/Kconfig                              |   2 +-
 drivers/power/Makefile                             |   2 +-
 .../power/{bq24257_charger.c => bq2425x_charger.c} | 532 ++++++++++-----------
 .../power/{bq24257_charger.h => bq2425x_charger.h} |   8 +-
 4 files changed, 272 insertions(+), 272 deletions(-)
 rename drivers/power/{bq24257_charger.c => bq2425x_charger.c} (64%)
 rename include/linux/power/{bq24257_charger.h => bq2425x_charger.h} (85%)

diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index fa9b1d1..07112b1 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -395,7 +395,7 @@ config CHARGER_BQ24190
 	help
 	  Say Y to enable support for the TI BQ24190 battery charger.
 
-config CHARGER_BQ24257
+config CHARGER_BQ2425X
 	tristate "TI BQ2425x battery charger driver"
 	depends on I2C && GPIOLIB
 	depends on REGMAP_I2C
diff --git a/drivers/power/Makefile b/drivers/power/Makefile
index 5752ce8..4aa9048 100644
--- a/drivers/power/Makefile
+++ b/drivers/power/Makefile
@@ -59,7 +59,7 @@ obj-$(CONFIG_CHARGER_MAX8997)	+= max8997_charger.o
 obj-$(CONFIG_CHARGER_MAX8998)	+= max8998_charger.o
 obj-$(CONFIG_CHARGER_BQ2415X)	+= bq2415x_charger.o
 obj-$(CONFIG_CHARGER_BQ24190)	+= bq24190_charger.o
-obj-$(CONFIG_CHARGER_BQ24257)	+= bq24257_charger.o
+obj-$(CONFIG_CHARGER_BQ2425X)	+= bq2425x_charger.o
 obj-$(CONFIG_CHARGER_BQ24735)	+= bq24735-charger.o
 obj-$(CONFIG_CHARGER_BQ25890)	+= bq25890_charger.o
 obj-$(CONFIG_POWER_AVS)		+= avs/
diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq2425x_charger.c
similarity index 64%
rename from drivers/power/bq24257_charger.c
rename to drivers/power/bq2425x_charger.c
index b5e82ed..3bf30d4 100644
--- a/drivers/power/bq24257_charger.c
+++ b/drivers/power/bq2425x_charger.c
@@ -1,5 +1,5 @@
 /*
- * TI BQ24257 charger driver
+ * TI BQ2425x charger driver
  *
  * Copyright (C) 2015 Intel Corporation
  *
@@ -32,21 +32,21 @@
 #include <linux/acpi.h>
 #include <linux/of.h>
 
-#include <linux/power/bq24257_charger.h>
+#include <linux/power/bq2425x_charger.h>
 
-#define BQ24257_REG_1			0x00
-#define BQ24257_REG_2			0x01
-#define BQ24257_REG_3			0x02
-#define BQ24257_REG_4			0x03
-#define BQ24257_REG_5			0x04
-#define BQ24257_REG_6			0x05
-#define BQ24257_REG_7			0x06
+#define BQ2425X_REG_1			0x00
+#define BQ2425X_REG_2			0x01
+#define BQ2425X_REG_3			0x02
+#define BQ2425X_REG_4			0x03
+#define BQ2425X_REG_5			0x04
+#define BQ2425X_REG_6			0x05
+#define BQ2425X_REG_7			0x06
 
-#define BQ24257_MANUFACTURER		"Texas Instruments"
-#define BQ24257_STAT_IRQ		"stat"
-#define BQ24257_PG_GPIO			"pg"
+#define BQ2425X_MANUFACTURER		"Texas Instruments"
+#define BQ2425X_STAT_IRQ		"stat"
+#define BQ2425X_PG_GPIO			"pg"
 
-#define BQ24257_ILIM_SET_DELAY		1000	/* msec */
+#define BQ2425X_ILIM_SET_DELAY		1000	/* msec */
 
 enum bq2425x_chip {
 	BQ24250,
@@ -60,7 +60,7 @@ static const char *bq2425x_chip_name[] = {
 	"bq24257",
 };
 
-enum bq24257_fields {
+enum bq2425x_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 */
 	F_VBAT, F_USB_DET,					    /* REG 3 */
@@ -73,7 +73,7 @@ enum bq24257_fields {
 };
 
 /* initial field values, converted from uV/uA */
-struct bq24257_init_data {
+struct bq2425x_init_data {
 	u8 ichg;	/* charge current      */
 	u8 vbat;	/* regulation voltage  */
 	u8 iterm;	/* termination current */
@@ -83,13 +83,13 @@ struct bq24257_init_data {
 	bool timer2x_enable;	/* slow down safety timer by 2x */
 };
 
-struct bq24257_state {
+struct bq2425x_state {
 	u8 status;
 	u8 fault;
 	bool power_good;
 };
 
-struct bq24257_device {
+struct bq2425x_device {
 	struct i2c_client *client;
 	struct device *dev;
 	struct power_supply *charger;
@@ -103,8 +103,8 @@ struct bq24257_device {
 
 	struct delayed_work iilimit_setup_work;
 
-	struct bq24257_init_data init_data;
-	struct bq24257_state state;
+	struct bq2425x_init_data init_data;
+	struct bq2425x_state state;
 
 	struct mutex lock; /* protect device access and state data */
 
@@ -112,11 +112,11 @@ struct bq24257_device {
 	bool pg_gpio_disable;
 };
 
-static bool bq24257_is_volatile_reg(struct device *dev, unsigned int reg)
+static bool bq2425x_is_volatile_reg(struct device *dev, unsigned int reg)
 {
 	switch (reg) {
-	case BQ24257_REG_2:
-	case BQ24257_REG_4:
+	case BQ2425X_REG_2:
+	case BQ2425X_REG_4:
 		return false;
 
 	default:
@@ -124,55 +124,55 @@ static bool bq24257_is_volatile_reg(struct device *dev, unsigned int reg)
 	}
 }
 
-static const struct regmap_config bq24257_regmap_config = {
+static const struct regmap_config bq2425x_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
 
-	.max_register = BQ24257_REG_7,
+	.max_register = BQ2425X_REG_7,
 	.cache_type = REGCACHE_RBTREE,
 
-	.volatile_reg = bq24257_is_volatile_reg,
+	.volatile_reg = bq2425x_is_volatile_reg,
 };
 
-static const struct reg_field bq24257_reg_fields[] = {
+static const struct reg_field bq2425x_reg_fields[] = {
 	/* REG 1 */
-	[F_WD_FAULT]		= REG_FIELD(BQ24257_REG_1, 7, 7),
-	[F_WD_EN]		= REG_FIELD(BQ24257_REG_1, 6, 6),
-	[F_STAT]		= REG_FIELD(BQ24257_REG_1, 4, 5),
-	[F_FAULT]		= REG_FIELD(BQ24257_REG_1, 0, 3),
+	[F_WD_FAULT]		= REG_FIELD(BQ2425X_REG_1, 7, 7),
+	[F_WD_EN]		= REG_FIELD(BQ2425X_REG_1, 6, 6),
+	[F_STAT]		= REG_FIELD(BQ2425X_REG_1, 4, 5),
+	[F_FAULT]		= REG_FIELD(BQ2425X_REG_1, 0, 3),
 	/* REG 2 */
-	[F_RESET]		= REG_FIELD(BQ24257_REG_2, 7, 7),
-	[F_IILIMIT]		= REG_FIELD(BQ24257_REG_2, 4, 6),
-	[F_EN_STAT]		= REG_FIELD(BQ24257_REG_2, 3, 3),
-	[F_EN_TERM]		= REG_FIELD(BQ24257_REG_2, 2, 2),
-	[F_CE]			= REG_FIELD(BQ24257_REG_2, 1, 1),
-	[F_HZ_MODE]		= REG_FIELD(BQ24257_REG_2, 0, 0),
+	[F_RESET]		= REG_FIELD(BQ2425X_REG_2, 7, 7),
+	[F_IILIMIT]		= REG_FIELD(BQ2425X_REG_2, 4, 6),
+	[F_EN_STAT]		= REG_FIELD(BQ2425X_REG_2, 3, 3),
+	[F_EN_TERM]		= REG_FIELD(BQ2425X_REG_2, 2, 2),
+	[F_CE]			= REG_FIELD(BQ2425X_REG_2, 1, 1),
+	[F_HZ_MODE]		= REG_FIELD(BQ2425X_REG_2, 0, 0),
 	/* REG 3 */
-	[F_VBAT]		= REG_FIELD(BQ24257_REG_3, 2, 7),
-	[F_USB_DET]		= REG_FIELD(BQ24257_REG_3, 0, 1),
+	[F_VBAT]		= REG_FIELD(BQ2425X_REG_3, 2, 7),
+	[F_USB_DET]		= REG_FIELD(BQ2425X_REG_3, 0, 1),
 	/* REG 4 */
-	[F_ICHG]		= REG_FIELD(BQ24257_REG_4, 3, 7),
-	[F_ITERM]		= REG_FIELD(BQ24257_REG_4, 0, 2),
+	[F_ICHG]		= REG_FIELD(BQ2425X_REG_4, 3, 7),
+	[F_ITERM]		= REG_FIELD(BQ2425X_REG_4, 0, 2),
 	/* REG 5 */
-	[F_LOOP_STATUS]		= REG_FIELD(BQ24257_REG_5, 6, 7),
-	[F_LOW_CHG]		= REG_FIELD(BQ24257_REG_5, 5, 5),
-	[F_DPDM_EN]		= REG_FIELD(BQ24257_REG_5, 4, 4),
-	[F_CE_STATUS]		= REG_FIELD(BQ24257_REG_5, 3, 3),
-	[F_VINDPM]		= REG_FIELD(BQ24257_REG_5, 0, 2),
+	[F_LOOP_STATUS]		= REG_FIELD(BQ2425X_REG_5, 6, 7),
+	[F_LOW_CHG]		= REG_FIELD(BQ2425X_REG_5, 5, 5),
+	[F_DPDM_EN]		= REG_FIELD(BQ2425X_REG_5, 4, 4),
+	[F_CE_STATUS]		= REG_FIELD(BQ2425X_REG_5, 3, 3),
+	[F_VINDPM]		= REG_FIELD(BQ2425X_REG_5, 0, 2),
 	/* REG 6 */
-	[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),
+	[F_X2_TMR_EN]		= REG_FIELD(BQ2425X_REG_6, 7, 7),
+	[F_TMR]			= REG_FIELD(BQ2425X_REG_6, 5, 6),
+	[F_SYSOFF]		= REG_FIELD(BQ2425X_REG_6, 4, 4),
+	[F_TS_EN]		= REG_FIELD(BQ2425X_REG_6, 3, 3),
+	[F_TS_STAT]		= REG_FIELD(BQ2425X_REG_6, 0, 2),
 	/* REG 7 */
-	[F_VOVP]		= REG_FIELD(BQ24257_REG_7, 5, 7),
-	[F_CLR_VDP]		= REG_FIELD(BQ24257_REG_7, 4, 4),
-	[F_FORCE_BATDET]	= REG_FIELD(BQ24257_REG_7, 3, 3),
-	[F_FORCE_PTM]		= REG_FIELD(BQ24257_REG_7, 2, 2)
+	[F_VOVP]		= REG_FIELD(BQ2425X_REG_7, 5, 7),
+	[F_CLR_VDP]		= REG_FIELD(BQ2425X_REG_7, 4, 4),
+	[F_FORCE_BATDET]	= REG_FIELD(BQ2425X_REG_7, 3, 3),
+	[F_FORCE_PTM]		= REG_FIELD(BQ2425X_REG_7, 2, 2)
 };
 
-static const u32 bq24257_vbat_map[] = {
+static const u32 bq2425x_vbat_map[] = {
 	3500000, 3520000, 3540000, 3560000, 3580000, 3600000, 3620000, 3640000,
 	3660000, 3680000, 3700000, 3720000, 3740000, 3760000, 3780000, 3800000,
 	3820000, 3840000, 3860000, 3880000, 3900000, 3920000, 3940000, 3960000,
@@ -181,45 +181,45 @@ static const u32 bq24257_vbat_map[] = {
 	4300000, 4320000, 4340000, 4360000, 4380000, 4400000, 4420000, 4440000
 };
 
-#define BQ24257_VBAT_MAP_SIZE		ARRAY_SIZE(bq24257_vbat_map)
+#define BQ2425X_VBAT_MAP_SIZE		ARRAY_SIZE(bq2425x_vbat_map)
 
-static const u32 bq24257_ichg_map[] = {
+static const u32 bq2425x_ichg_map[] = {
 	500000, 550000, 600000, 650000, 700000, 750000, 800000, 850000, 900000,
 	950000, 1000000, 1050000, 1100000, 1150000, 1200000, 1250000, 1300000,
 	1350000, 1400000, 1450000, 1500000, 1550000, 1600000, 1650000, 1700000,
 	1750000, 1800000, 1850000, 1900000, 1950000, 2000000
 };
 
-#define BQ24257_ICHG_MAP_SIZE		ARRAY_SIZE(bq24257_ichg_map)
+#define BQ2425X_ICHG_MAP_SIZE		ARRAY_SIZE(bq2425x_ichg_map)
 
-static const u32 bq24257_iterm_map[] = {
+static const u32 bq2425x_iterm_map[] = {
 	50000, 75000, 100000, 125000, 150000, 175000, 200000, 225000
 };
 
-#define BQ24257_ITERM_MAP_SIZE		ARRAY_SIZE(bq24257_iterm_map)
+#define BQ2425X_ITERM_MAP_SIZE		ARRAY_SIZE(bq2425x_iterm_map)
 
-static const u32 bq24257_iilimit_map[] = {
+static const u32 bq2425x_iilimit_map[] = {
 	100000, 150000, 500000, 900000, 1500000, 2000000
 };
 
-#define BQ24257_IILIMIT_MAP_SIZE	ARRAY_SIZE(bq24257_iilimit_map)
+#define BQ2425X_IILIMIT_MAP_SIZE	ARRAY_SIZE(bq2425x_iilimit_map)
 
-static const u32 bq24257_vovp_map[] = {
+static const u32 bq2425x_vovp_map[] = {
 	6000000, 6500000, 7000000, 8000000, 9000000, 9500000, 10000000,
 	10500000
 };
 
-#define BQ24257_VOVP_MAP_SIZE		ARRAY_SIZE(bq24257_vovp_map)
+#define BQ2425X_VOVP_MAP_SIZE		ARRAY_SIZE(bq2425x_vovp_map)
 
-static const u32 bq24257_vindpm_map[] = {
+static const u32 bq2425x_vindpm_map[] = {
 	4200000, 4280000, 4360000, 4440000, 4520000, 4600000, 4680000,
 	4760000
 };
 
-#define BQ24257_VINDPM_MAP_SIZE		ARRAY_SIZE(bq24257_vindpm_map)
+#define BQ2425X_VINDPM_MAP_SIZE		ARRAY_SIZE(bq2425x_vindpm_map)
 
-static int bq24257_field_read(struct bq24257_device *bq,
-			      enum bq24257_fields field_id)
+static int bq2425x_field_read(struct bq2425x_device *bq,
+			      enum bq2425x_fields field_id)
 {
 	int ret;
 	int val;
@@ -231,13 +231,13 @@ static int bq24257_field_read(struct bq24257_device *bq,
 	return val;
 }
 
-static int bq24257_field_write(struct bq24257_device *bq,
-			       enum bq24257_fields field_id, u8 val)
+static int bq2425x_field_write(struct bq2425x_device *bq,
+			       enum bq2425x_fields field_id, u8 val)
 {
 	return regmap_field_write(bq->rmap_fields[field_id], val);
 }
 
-static u8 bq24257_find_idx(u32 value, const u32 *map, u8 map_size)
+static u8 bq2425x_find_idx(u32 value, const u32 *map, u8 map_size)
 {
 	u8 idx;
 
@@ -248,14 +248,14 @@ static u8 bq24257_find_idx(u32 value, const u32 *map, u8 map_size)
 	return idx - 1;
 }
 
-enum bq24257_status {
+enum bq2425x_status {
 	STATUS_READY,
 	STATUS_CHARGE_IN_PROGRESS,
 	STATUS_CHARGE_DONE,
 	STATUS_FAULT,
 };
 
-enum bq24257_fault {
+enum bq2425x_fault {
 	FAULT_NORMAL,
 	FAULT_INPUT_OVP,
 	FAULT_INPUT_UVLO,
@@ -269,12 +269,12 @@ enum bq24257_fault {
 	FAULT_INPUT_LDO_LOW,
 };
 
-static int bq24257_get_charge_type(struct bq24257_device *bq,
+static int bq2425x_get_charge_type(struct bq2425x_device *bq,
 		union power_supply_propval *val)
 {
 	int ret;
 
-	ret = bq24257_field_read(bq, F_CE);
+	ret = bq2425x_field_read(bq, F_CE);
 	if (ret < 0)
 		return ret;
 
@@ -285,7 +285,7 @@ static int bq24257_get_charge_type(struct bq24257_device *bq,
 	}
 
 	/* Low Charge means a fixed low-current is used instead of i_chg */
-	ret = bq24257_field_read(bq, F_LOW_CHG);
+	ret = bq2425x_field_read(bq, F_LOW_CHG);
 	if (ret < 0)
 		return ret;
 
@@ -295,19 +295,19 @@ static int bq24257_get_charge_type(struct bq24257_device *bq,
 	return 0;
 }
 
-static int bq24257_set_charge_type(struct bq24257_device *bq,
+static int bq2425x_set_charge_type(struct bq2425x_device *bq,
 		const union power_supply_propval *val)
 {
 	int ret;
 
 	switch (val->intval) {
 	case POWER_SUPPLY_CHARGE_TYPE_NONE:
-		return bq24257_field_write(bq, F_CE, 1);
+		return bq2425x_field_write(bq, F_CE, 1);
 	case POWER_SUPPLY_CHARGE_TYPE_TRICKLE:
-		ret = bq24257_field_write(bq, F_LOW_CHG, 1);
+		ret = bq2425x_field_write(bq, F_LOW_CHG, 1);
 		break;
 	case POWER_SUPPLY_CHARGE_TYPE_FAST:
-		ret = bq24257_field_write(bq, F_LOW_CHG, 0);
+		ret = bq2425x_field_write(bq, F_LOW_CHG, 0);
 		break;
 	default:
 		return -EINVAL;
@@ -316,15 +316,15 @@ static int bq24257_set_charge_type(struct bq24257_device *bq,
 	if (ret < 0)
 		return ret;
 
-	return bq24257_field_write(bq, F_CE, 0);
+	return bq2425x_field_write(bq, F_CE, 0);
 }
 
-static int bq24257_get_input_current_limit(struct bq24257_device *bq,
+static int bq2425x_get_input_current_limit(struct bq2425x_device *bq,
 		union power_supply_propval *val)
 {
 	int ret;
 
-	ret = bq24257_field_read(bq, F_IILIMIT);
+	ret = bq2425x_field_read(bq, F_IILIMIT);
 	if (ret < 0)
 		return ret;
 
@@ -335,31 +335,31 @@ static int bq24257_get_input_current_limit(struct bq24257_device *bq,
 	 * than exceeding the bounds of the lookup table and returning
 	 * garbage.
 	 */
-	if (ret >= BQ24257_IILIMIT_MAP_SIZE)
+	if (ret >= BQ2425X_IILIMIT_MAP_SIZE)
 		return -ENODATA;
 
-	val->intval = bq24257_iilimit_map[ret];
+	val->intval = bq2425x_iilimit_map[ret];
 
 	return 0;
 }
 
-static int bq24257_set_input_current_limit(struct bq24257_device *bq,
+static int bq2425x_set_input_current_limit(struct bq2425x_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));
+	return bq2425x_field_write(bq, F_IILIMIT, bq2425x_find_idx(
+			val->intval, bq2425x_iilimit_map,
+			BQ2425X_IILIMIT_MAP_SIZE));
 }
 
-static int bq24257_power_supply_get_property(struct power_supply *psy,
+static int bq2425x_power_supply_get_property(struct power_supply *psy,
 					     enum power_supply_property psp,
 					     union power_supply_propval *val)
 {
-	struct bq24257_device *bq = power_supply_get_drvdata(psy);
-	struct bq24257_state state;
+	struct bq2425x_device *bq = power_supply_get_drvdata(psy);
+	struct bq2425x_state state;
 	int ret = 0;
 
 	mutex_lock(&bq->lock);
@@ -380,11 +380,11 @@ static int bq24257_power_supply_get_property(struct power_supply *psy,
 		break;
 
 	case POWER_SUPPLY_PROP_CHARGE_TYPE:
-		ret = bq24257_get_charge_type(bq, val);
+		ret = bq2425x_get_charge_type(bq, val);
 		break;
 
 	case POWER_SUPPLY_PROP_MANUFACTURER:
-		val->strval = BQ24257_MANUFACTURER;
+		val->strval = BQ2425X_MANUFACTURER;
 		break;
 
 	case POWER_SUPPLY_PROP_MODEL_NAME:
@@ -423,27 +423,27 @@ static int bq24257_power_supply_get_property(struct power_supply *psy,
 		break;
 
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
-		val->intval = bq24257_ichg_map[bq->init_data.ichg];
+		val->intval = bq2425x_ichg_map[bq->init_data.ichg];
 		break;
 
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
-		val->intval = bq24257_ichg_map[BQ24257_ICHG_MAP_SIZE - 1];
+		val->intval = bq2425x_ichg_map[BQ2425X_ICHG_MAP_SIZE - 1];
 		break;
 
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
-		val->intval = bq24257_vbat_map[bq->init_data.vbat];
+		val->intval = bq2425x_vbat_map[bq->init_data.vbat];
 		break;
 
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX:
-		val->intval = bq24257_vbat_map[BQ24257_VBAT_MAP_SIZE - 1];
+		val->intval = bq2425x_vbat_map[BQ2425X_VBAT_MAP_SIZE - 1];
 		break;
 
 	case POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT:
-		val->intval = bq24257_iterm_map[bq->init_data.iterm];
+		val->intval = bq2425x_iterm_map[bq->init_data.iterm];
 		break;
 
 	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
-		ret = bq24257_get_input_current_limit(bq, val);
+		ret = bq2425x_get_input_current_limit(bq, val);
 		break;
 
 	default:
@@ -455,21 +455,21 @@ static int bq24257_power_supply_get_property(struct power_supply *psy,
 	return ret;
 }
 
-static int bq24257_power_supply_set_property(struct power_supply *psy,
+static int bq2425x_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);
+	struct bq2425x_device *bq = power_supply_get_drvdata(psy);
 	int ret;
 
 	mutex_lock(&bq->lock);
 
 	switch (prop) {
 	case POWER_SUPPLY_PROP_CHARGE_TYPE:
-		ret = bq24257_set_charge_type(bq, val);
+		ret = bq2425x_set_charge_type(bq, val);
 		break;
 	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
-		ret = bq24257_set_input_current_limit(bq, val);
+		ret = bq2425x_set_input_current_limit(bq, val);
 		break;
 	default:
 		ret = -EINVAL;
@@ -480,7 +480,7 @@ static int bq24257_power_supply_set_property(struct power_supply *psy,
 	return ret;
 }
 
-static int bq24257_power_supply_property_is_writeable(struct power_supply *psy,
+static int bq2425x_power_supply_property_is_writeable(struct power_supply *psy,
 		enum power_supply_property psp)
 {
 	switch (psp) {
@@ -492,18 +492,18 @@ static int bq24257_power_supply_property_is_writeable(struct power_supply *psy,
 	}
 }
 
-static int bq24257_get_chip_state(struct bq24257_device *bq,
-				  struct bq24257_state *state)
+static int bq2425x_get_chip_state(struct bq2425x_device *bq,
+				  struct bq2425x_state *state)
 {
 	int ret;
 
-	ret = bq24257_field_read(bq, F_STAT);
+	ret = bq2425x_field_read(bq, F_STAT);
 	if (ret < 0)
 		return ret;
 
 	state->status = ret;
 
-	ret = bq24257_field_read(bq, F_FAULT);
+	ret = bq2425x_field_read(bq, F_FAULT);
 	if (ret < 0)
 		return ret;
 
@@ -533,22 +533,22 @@ static int bq24257_get_chip_state(struct bq24257_device *bq,
 	return 0;
 }
 
-static bool bq24257_state_changed(struct bq24257_device *bq,
-				  struct bq24257_state *new_state)
+static bool bq2425x_state_changed(struct bq2425x_device *bq,
+				  struct bq2425x_state *new_state)
 {
 	return bq->state.status != new_state->status ||
 			bq->state.fault != new_state->fault ||
 			bq->state.power_good != new_state->power_good;
 }
 
-enum bq24257_loop_status {
+enum bq2425x_loop_status {
 	LOOP_STATUS_NONE,
 	LOOP_STATUS_IN_DPM,
 	LOOP_STATUS_IN_CURRENT_LIMIT,
 	LOOP_STATUS_THERMAL,
 };
 
-enum bq24257_in_ilimit {
+enum bq2425x_in_ilimit {
 	IILIMIT_100,
 	IILIMIT_150,
 	IILIMIT_500,
@@ -559,7 +559,7 @@ enum bq24257_in_ilimit {
 	IILIMIT_NONE,
 };
 
-enum bq24257_vovp {
+enum bq2425x_vovp {
 	VOVP_6000,
 	VOVP_6500,
 	VOVP_7000,
@@ -570,7 +570,7 @@ enum bq24257_vovp {
 	VOVP_10500
 };
 
-enum bq24257_vindpm {
+enum bq2425x_vindpm {
 	VINDPM_4200,
 	VINDPM_4280,
 	VINDPM_4360,
@@ -581,21 +581,21 @@ enum bq24257_vindpm {
 	VINDPM_4760
 };
 
-enum bq24257_port_type {
+enum bq2425x_port_type {
 	PORT_TYPE_DCP,		/* Dedicated Charging Port */
 	PORT_TYPE_CDP,		/* Charging Downstream Port */
 	PORT_TYPE_SDP,		/* Standard Downstream Port */
 	PORT_TYPE_NON_STANDARD,
 };
 
-enum bq24257_safety_timer {
+enum bq2425x_safety_timer {
 	SAFETY_TIMER_45,
 	SAFETY_TIMER_360,
 	SAFETY_TIMER_540,
 	SAFETY_TIMER_NONE,
 };
 
-static int bq24257_iilimit_autoset(struct bq24257_device *bq)
+static int bq2425x_iilimit_autoset(struct bq2425x_device *bq)
 {
 	int loop_status;
 	int iilimit;
@@ -608,13 +608,13 @@ static int bq24257_iilimit_autoset(struct bq24257_device *bq)
 		[PORT_TYPE_NON_STANDARD] = IILIMIT_500
 	};
 
-	ret = bq24257_field_read(bq, F_LOOP_STATUS);
+	ret = bq2425x_field_read(bq, F_LOOP_STATUS);
 	if (ret < 0)
 		goto error;
 
 	loop_status = ret;
 
-	ret = bq24257_field_read(bq, F_IILIMIT);
+	ret = bq2425x_field_read(bq, F_IILIMIT);
 	if (ret < 0)
 		goto error;
 
@@ -628,21 +628,21 @@ static int bq24257_iilimit_autoset(struct bq24257_device *bq)
 	if (loop_status == LOOP_STATUS_IN_DPM && iilimit == IILIMIT_500)
 		return 0;
 
-	ret = bq24257_field_read(bq, F_USB_DET);
+	ret = bq2425x_field_read(bq, F_USB_DET);
 	if (ret < 0)
 		goto error;
 
 	port_type = ret;
 
-	ret = bq24257_field_write(bq, F_IILIMIT, new_iilimit[port_type]);
+	ret = bq2425x_field_write(bq, F_IILIMIT, new_iilimit[port_type]);
 	if (ret < 0)
 		goto error;
 
-	ret = bq24257_field_write(bq, F_TMR, SAFETY_TIMER_360);
+	ret = bq2425x_field_write(bq, F_TMR, SAFETY_TIMER_360);
 	if (ret < 0)
 		goto error;
 
-	ret = bq24257_field_write(bq, F_CLR_VDP, 1);
+	ret = bq2425x_field_write(bq, F_CLR_VDP, 1);
 	if (ret < 0)
 		goto error;
 
@@ -656,21 +656,21 @@ error:
 	return ret;
 }
 
-static void bq24257_iilimit_setup_work(struct work_struct *work)
+static void bq2425x_iilimit_setup_work(struct work_struct *work)
 {
-	struct bq24257_device *bq = container_of(work, struct bq24257_device,
+	struct bq2425x_device *bq = container_of(work, struct bq2425x_device,
 						 iilimit_setup_work.work);
 
 	mutex_lock(&bq->lock);
-	bq24257_iilimit_autoset(bq);
+	bq2425x_iilimit_autoset(bq);
 	mutex_unlock(&bq->lock);
 }
 
-static void bq24257_handle_state_change(struct bq24257_device *bq,
-					struct bq24257_state *new_state)
+static void bq2425x_handle_state_change(struct bq2425x_device *bq,
+					struct bq2425x_state *new_state)
 {
 	int ret;
-	struct bq24257_state old_state;
+	struct bq2425x_state old_state;
 	bool reset_iilimit = false;
 	bool config_iilimit = false;
 
@@ -686,7 +686,7 @@ static void bq24257_handle_state_change(struct bq24257_device *bq,
 			cancel_delayed_work_sync(&bq->iilimit_setup_work);
 
 			/* activate D+/D- port detection algorithm */
-			ret = bq24257_field_write(bq, F_DPDM_EN, 1);
+			ret = bq2425x_field_write(bq, F_DPDM_EN, 1);
 			if (ret < 0)
 				goto error;
 
@@ -709,12 +709,12 @@ static void bq24257_handle_state_change(struct bq24257_device *bq,
 	}
 
 	if (reset_iilimit) {
-		ret = bq24257_field_write(bq, F_IILIMIT, IILIMIT_500);
+		ret = bq2425x_field_write(bq, F_IILIMIT, IILIMIT_500);
 		if (ret < 0)
 			goto error;
 	} else if (config_iilimit) {
 		schedule_delayed_work(&bq->iilimit_setup_work,
-				      msecs_to_jiffies(BQ24257_ILIM_SET_DELAY));
+				      msecs_to_jiffies(BQ2425X_ILIM_SET_DELAY));
 	}
 
 	return;
@@ -723,25 +723,25 @@ error:
 	dev_err(bq->dev, "%s: Error communicating with the chip.\n", __func__);
 }
 
-static irqreturn_t bq24257_irq_handler_thread(int irq, void *private)
+static irqreturn_t bq2425x_irq_handler_thread(int irq, void *private)
 {
 	int ret;
-	struct bq24257_device *bq = private;
-	struct bq24257_state state;
+	struct bq2425x_device *bq = private;
+	struct bq2425x_state state;
 
 	mutex_lock(&bq->lock);
 
-	ret = bq24257_get_chip_state(bq, &state);
+	ret = bq2425x_get_chip_state(bq, &state);
 	if (ret < 0)
 		goto out;
 
-	if (!bq24257_state_changed(bq, &state))
+	if (!bq2425x_state_changed(bq, &state))
 		goto out;
 
 	dev_dbg(bq->dev, "irq(state changed): status/fault/pg = %d/%d/%d\n",
 		state.status, state.fault, state.power_good);
 
-	bq24257_handle_state_change(bq, &state);
+	bq2425x_handle_state_change(bq, &state);
 	bq->state = state;
 
 	mutex_unlock(&bq->lock);
@@ -755,11 +755,11 @@ out:
 	return IRQ_HANDLED;
 }
 
-static int bq24257_hw_init(struct bq24257_device *bq)
+static int bq2425x_hw_init(struct bq2425x_device *bq)
 {
 	int ret;
 	int i;
-	struct bq24257_state state;
+	struct bq2425x_state state;
 
 	const struct {
 		int field;
@@ -777,19 +777,19 @@ static int bq24257_hw_init(struct bq24257_device *bq)
 	 * Disable the watchdog timer to prevent the IC from going back to
 	 * default settings after 50 seconds of I2C inactivity.
 	 */
-	ret = bq24257_field_write(bq, F_WD_EN, 0);
+	ret = bq2425x_field_write(bq, F_WD_EN, 0);
 	if (ret < 0)
 		return ret;
 
 	/* configure the charge currents and voltages */
 	for (i = 0; i < ARRAY_SIZE(init_data); i++) {
-		ret = bq24257_field_write(bq, init_data[i].field,
+		ret = bq2425x_field_write(bq, init_data[i].field,
 					  init_data[i].value);
 		if (ret < 0)
 			return ret;
 	}
 
-	ret = bq24257_get_chip_state(bq, &state);
+	ret = bq2425x_get_chip_state(bq, &state);
 	if (ret < 0)
 		return ret;
 
@@ -800,21 +800,21 @@ static int bq24257_hw_init(struct bq24257_device *bq)
 				bq->init_data.in_ilimit);
 
 		/* program fixed input current limit */
-		ret = bq24257_field_write(bq, F_IILIMIT,
+		ret = bq2425x_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);
+		ret = bq2425x_field_write(bq, F_DPDM_EN, 1);
 	else if (state.fault != FAULT_NO_BAT)
-		ret = bq24257_iilimit_autoset(bq);
+		ret = bq2425x_iilimit_autoset(bq);
 
 	return ret;
 }
 
-static enum power_supply_property bq24257_power_supply_props[] = {
+static enum power_supply_property bq2425x_power_supply_props[] = {
 	POWER_SUPPLY_PROP_MANUFACTURER,
 	POWER_SUPPLY_PROP_MODEL_NAME,
 	POWER_SUPPLY_PROP_STATUS,
@@ -829,56 +829,56 @@ static enum power_supply_property bq24257_power_supply_props[] = {
 	POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
 };
 
-static char *bq24257_charger_supplied_to[] = {
+static char *bq2425x_charger_supplied_to[] = {
 	"main-battery",
 };
 
-static const struct power_supply_desc bq24257_power_supply_desc = {
-	.name = "bq24257-charger",
+static const struct power_supply_desc bq2425x_power_supply_desc = {
+	.name = "bq2425x-charger",
 	.type = POWER_SUPPLY_TYPE_USB,
-	.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
+	.properties = bq2425x_power_supply_props,
+	.num_properties = ARRAY_SIZE(bq2425x_power_supply_props),
+	.get_property = bq2425x_power_supply_get_property,
+	.set_property = bq2425x_power_supply_set_property,
+	.property_is_writeable	= bq2425x_power_supply_property_is_writeable
 };
 
-static ssize_t bq24257_show_ovp_voltage(struct device *dev,
+static ssize_t bq2425x_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);
+	struct bq2425x_device *bq = power_supply_get_drvdata(psy);
 
 	return scnprintf(buf, PAGE_SIZE, "%d\n",
-			bq24257_vovp_map[bq->init_data.vovp]);
+			bq2425x_vovp_map[bq->init_data.vovp]);
 }
 
-static ssize_t bq24257_show_in_dpm_voltage(struct device *dev,
+static ssize_t bq2425x_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);
+	struct bq2425x_device *bq = power_supply_get_drvdata(psy);
 
 	return scnprintf(buf, PAGE_SIZE, "%d\n",
-			bq24257_vindpm_map[bq->init_data.vindpm]);
+			bq2425x_vindpm_map[bq->init_data.vindpm]);
 }
 
-static ssize_t bq24257_sysfs_show_enable(struct device *dev,
+static ssize_t bq2425x_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);
+	struct bq2425x_device *bq = power_supply_get_drvdata(psy);
 	int ret;
 
 	mutex_lock(&bq->lock);
 
 	if (strcmp(attr->attr.name, "high_impedance_enable") == 0)
-		ret = bq24257_field_read(bq, F_HZ_MODE);
+		ret = bq2425x_field_read(bq, F_HZ_MODE);
 	else if (strcmp(attr->attr.name, "sysoff_enable") == 0)
-		ret = bq24257_field_read(bq, F_SYSOFF);
+		ret = bq2425x_field_read(bq, F_SYSOFF);
 	else if (strcmp(attr->attr.name, "timer2x_enable") == 0)
 		ret = bq->init_data.timer2x_enable;
 	else
@@ -892,13 +892,13 @@ static ssize_t bq24257_sysfs_show_enable(struct device *dev,
 	return scnprintf(buf, PAGE_SIZE, "%d\n", ret);
 }
 
-static ssize_t bq24257_sysfs_set_enable(struct device *dev,
+static ssize_t bq2425x_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);
+	struct bq2425x_device *bq = power_supply_get_drvdata(psy);
 	long val;
 	int ret;
 
@@ -908,9 +908,9 @@ static ssize_t bq24257_sysfs_set_enable(struct device *dev,
 	mutex_lock(&bq->lock);
 
 	if (strcmp(attr->attr.name, "high_impedance_enable") == 0)
-		ret = bq24257_field_write(bq, F_HZ_MODE, val);
+		ret = bq2425x_field_write(bq, F_HZ_MODE, val);
 	else if (strcmp(attr->attr.name, "sysoff_enable") == 0)
-		ret = bq24257_field_write(bq, F_SYSOFF, val);
+		ret = bq2425x_field_write(bq, F_SYSOFF, val);
 	else
 		ret = -EINVAL;
 
@@ -922,16 +922,16 @@ static ssize_t bq24257_sysfs_set_enable(struct device *dev,
 	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(ovp_voltage, S_IRUGO, bq2425x_show_ovp_voltage, NULL);
+static DEVICE_ATTR(in_dpm_voltage, S_IRUGO, bq2425x_show_in_dpm_voltage, NULL);
 static DEVICE_ATTR(high_impedance_enable, S_IWUSR | S_IRUGO,
-		bq24257_sysfs_show_enable, bq24257_sysfs_set_enable);
+		bq2425x_sysfs_show_enable, bq2425x_sysfs_set_enable);
 static DEVICE_ATTR(sysoff_enable, S_IWUSR | S_IRUGO,
-		bq24257_sysfs_show_enable, bq24257_sysfs_set_enable);
+		bq2425x_sysfs_show_enable, bq2425x_sysfs_set_enable);
 static DEVICE_ATTR(timer2x_enable, S_IRUGO,
-		bq24257_sysfs_show_enable, NULL);
+		bq2425x_sysfs_show_enable, NULL);
 
-static struct attribute *bq24257_charger_attr[] = {
+static struct attribute *bq2425x_charger_attr[] = {
 	&dev_attr_ovp_voltage.attr,
 	&dev_attr_in_dpm_voltage.attr,
 	&dev_attr_high_impedance_enable.attr,
@@ -940,18 +940,18 @@ static struct attribute *bq24257_charger_attr[] = {
 	NULL,
 };
 
-static const struct attribute_group bq24257_attr_group = {
-	.attrs = bq24257_charger_attr,
+static const struct attribute_group bq2425x_attr_group = {
+	.attrs = bq2425x_charger_attr,
 };
 
-static int bq24257_power_supply_init(struct bq24257_device *bq)
+static int bq2425x_power_supply_init(struct bq2425x_device *bq)
 {
 	struct power_supply_config psy_cfg = { .drv_data = bq, };
 
-	psy_cfg.supplied_to = bq24257_charger_supplied_to;
-	psy_cfg.num_supplicants = ARRAY_SIZE(bq24257_charger_supplied_to);
+	psy_cfg.supplied_to = bq2425x_charger_supplied_to;
+	psy_cfg.num_supplicants = ARRAY_SIZE(bq2425x_charger_supplied_to);
 
-	bq->charger = power_supply_register(bq->dev, &bq24257_power_supply_desc,
+	bq->charger = power_supply_register(bq->dev, &bq2425x_power_supply_desc,
 					    &psy_cfg);
 	if (IS_ERR(bq->charger))
 		return PTR_ERR(bq->charger);
@@ -959,14 +959,14 @@ static int bq24257_power_supply_init(struct bq24257_device *bq)
 	return 0;
 }
 
-static int bq24257_irq_probe(struct bq24257_device *bq)
+static int bq2425x_irq_probe(struct bq2425x_device *bq)
 {
-	struct bq24257_platform_data *pdata = bq->client->dev.platform_data;
+	struct bq2425x_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,
+		stat_irq = devm_gpiod_get_index(bq->dev, BQ2425X_STAT_IRQ, 0,
 				GPIOD_IN);
 	else {
 		if (!gpio_is_valid(pdata->stat_gpio)) {
@@ -975,7 +975,7 @@ static int bq24257_irq_probe(struct bq24257_device *bq)
 		}
 
 		ret = devm_gpio_request_one(bq->dev, pdata->stat_gpio,
-				GPIOD_IN, BQ24257_STAT_IRQ);
+				GPIOD_IN, BQ2425X_STAT_IRQ);
 		if (ret) {
 			dev_err(bq->dev, "stat_irq pin request failed\n");
 			return ret;
@@ -994,13 +994,13 @@ static int bq24257_irq_probe(struct bq24257_device *bq)
 	return gpiod_to_irq(stat_irq);
 }
 
-static int bq24257_pg_gpio_probe(struct bq24257_device *bq)
+static int bq2425x_pg_gpio_probe(struct bq2425x_device *bq)
 {
-	struct bq24257_platform_data *pdata = bq->client->dev.platform_data;
+	struct bq2425x_platform_data *pdata = bq->client->dev.platform_data;
 	int ret;
 
 	if (!pdata)
-		bq->pg = devm_gpiod_get_index(bq->dev, BQ24257_PG_GPIO, 0,
+		bq->pg = devm_gpiod_get_index(bq->dev, BQ2425X_PG_GPIO, 0,
 				GPIOD_IN);
 	else {
 		if (!gpio_is_valid(pdata->pg_gpio)) {
@@ -1009,7 +1009,7 @@ static int bq24257_pg_gpio_probe(struct bq24257_device *bq)
 		}
 
 		ret = devm_gpio_request_one(bq->dev, pdata->pg_gpio,
-				GPIOD_IN, BQ24257_PG_GPIO);
+				GPIOD_IN, BQ2425X_PG_GPIO);
 
 		if (ret) {
 			dev_err(bq->dev, "PG pin request failed\n");
@@ -1029,26 +1029,26 @@ static int bq24257_pg_gpio_probe(struct bq24257_device *bq)
 	return 0;
 }
 
-static void bq24257_pdata_probe(struct bq24257_device *bq,
-		struct bq24257_platform_data *pdata)
+static void bq2425x_pdata_probe(struct bq2425x_device *bq,
+		struct bq2425x_platform_data *pdata)
 {
-	bq->init_data.ichg = bq24257_find_idx(pdata->ichg,
-			bq24257_ichg_map, BQ24257_ICHG_MAP_SIZE);
+	bq->init_data.ichg = bq2425x_find_idx(pdata->ichg,
+			bq2425x_ichg_map, BQ2425X_ICHG_MAP_SIZE);
 
-	bq->init_data.vbat = bq24257_find_idx(pdata->vbat,
-			bq24257_vbat_map, BQ24257_VBAT_MAP_SIZE);
+	bq->init_data.vbat = bq2425x_find_idx(pdata->vbat,
+			bq2425x_vbat_map, BQ2425X_VBAT_MAP_SIZE);
 
-	bq->init_data.iterm = bq24257_find_idx(pdata->iterm,
-			bq24257_iterm_map, BQ24257_ITERM_MAP_SIZE);
+	bq->init_data.iterm = bq2425x_find_idx(pdata->iterm,
+			bq2425x_iterm_map, BQ2425X_ITERM_MAP_SIZE);
 
-	bq->init_data.in_ilimit = bq24257_find_idx(pdata->in_ilimit,
-			bq24257_iilimit_map, BQ24257_IILIMIT_MAP_SIZE);
+	bq->init_data.in_ilimit = bq2425x_find_idx(pdata->in_ilimit,
+			bq2425x_iilimit_map, BQ2425X_IILIMIT_MAP_SIZE);
 
-	bq->init_data.vovp = bq24257_find_idx(pdata->vovp, bq24257_vovp_map,
-			BQ24257_VOVP_MAP_SIZE);
+	bq->init_data.vovp = bq2425x_find_idx(pdata->vovp, bq2425x_vovp_map,
+			BQ2425X_VOVP_MAP_SIZE);
 
-	bq->init_data.vindpm = bq24257_find_idx(pdata->vindpm,
-			bq24257_vindpm_map, BQ24257_VINDPM_MAP_SIZE);
+	bq->init_data.vindpm = bq2425x_find_idx(pdata->vindpm,
+			bq2425x_vindpm_map, BQ2425X_VINDPM_MAP_SIZE);
 
 	bq->init_data.timer2x_enable = pdata->timer2x_enable;
 
@@ -1057,7 +1057,7 @@ static void bq24257_pdata_probe(struct bq24257_device *bq,
 	bq->pg_gpio_disable = pdata->pg_gpio_disable;
 }
 
-static int bq24257_fw_probe(struct bq24257_device *bq)
+static int bq2425x_fw_probe(struct bq2425x_device *bq)
 {
 	int ret;
 	u32 property;
@@ -1067,24 +1067,24 @@ static int bq24257_fw_probe(struct bq24257_device *bq)
 	if (ret < 0)
 		return ret;
 
-	bq->init_data.ichg = bq24257_find_idx(property, bq24257_ichg_map,
-					      BQ24257_ICHG_MAP_SIZE);
+	bq->init_data.ichg = bq2425x_find_idx(property, bq2425x_ichg_map,
+					      BQ2425X_ICHG_MAP_SIZE);
 
 	ret = device_property_read_u32(bq->dev, "ti,battery-regulation-voltage",
 				       &property);
 	if (ret < 0)
 		return ret;
 
-	bq->init_data.vbat = bq24257_find_idx(property, bq24257_vbat_map,
-					      BQ24257_VBAT_MAP_SIZE);
+	bq->init_data.vbat = bq2425x_find_idx(property, bq2425x_vbat_map,
+					      BQ2425X_VBAT_MAP_SIZE);
 
 	ret = device_property_read_u32(bq->dev, "ti,termination-current",
 				       &property);
 	if (ret < 0)
 		return ret;
 
-	bq->init_data.iterm = bq24257_find_idx(property, bq24257_iterm_map,
-					       BQ24257_ITERM_MAP_SIZE);
+	bq->init_data.iterm = bq2425x_find_idx(property, bq2425x_iterm_map,
+					       BQ2425X_ITERM_MAP_SIZE);
 
 	/* Optional properties. If not provided use reasonable default. */
 	ret = device_property_read_u32(bq->dev, "ti,current-limit",
@@ -1098,8 +1098,8 @@ static int bq24257_fw_probe(struct bq24257_device *bq)
 		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);
+		bq->init_data.in_ilimit = bq2425x_find_idx(property,
+				bq2425x_iilimit_map, BQ2425X_IILIMIT_MAP_SIZE);
 	}
 
 	ret = device_property_read_u32(bq->dev, "ti,ovp-voltage",
@@ -1107,16 +1107,16 @@ static int bq24257_fw_probe(struct bq24257_device *bq)
 	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);
+		bq->init_data.vovp = bq2425x_find_idx(property,
+				bq2425x_vovp_map, BQ2425X_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);
+		bq->init_data.vindpm = bq2425x_find_idx(property,
+				bq2425x_vindpm_map, BQ2425X_VINDPM_MAP_SIZE);
 
 	ret = device_property_read_u32(bq->dev, "ti,safety-timer-2x-enable",
 				       &property);
@@ -1128,14 +1128,14 @@ static int bq24257_fw_probe(struct bq24257_device *bq)
 	return 0;
 }
 
-static int bq24257_probe(struct i2c_client *client,
+static int bq2425x_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
 	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;
+	struct bq2425x_platform_data *pdata = client->dev.platform_data;
+	struct bq2425x_device *bq;
 	int ret;
 	int i;
 
@@ -1165,14 +1165,14 @@ static int bq24257_probe(struct i2c_client *client,
 
 	mutex_init(&bq->lock);
 
-	bq->rmap = devm_regmap_init_i2c(client, &bq24257_regmap_config);
+	bq->rmap = devm_regmap_init_i2c(client, &bq2425x_regmap_config);
 	if (IS_ERR(bq->rmap)) {
 		dev_err(dev, "failed to allocate register map\n");
 		return PTR_ERR(bq->rmap);
 	}
 
-	for (i = 0; i < ARRAY_SIZE(bq24257_reg_fields); i++) {
-		const struct reg_field *reg_fields = bq24257_reg_fields;
+	for (i = 0; i < ARRAY_SIZE(bq2425x_reg_fields); i++) {
+		const struct reg_field *reg_fields = bq2425x_reg_fields;
 
 		bq->rmap_fields[i] = devm_regmap_field_alloc(dev, bq->rmap,
 							     reg_fields[i]);
@@ -1185,14 +1185,14 @@ static int bq24257_probe(struct i2c_client *client,
 	i2c_set_clientdata(client, bq);
 
 	if (!pdata) {
-		ret = bq24257_fw_probe(bq);
+		ret = bq2425x_fw_probe(bq);
 		if (ret < 0) {
 			dev_err(dev, "Cannot read device properties.\n");
 			return ret;
 		}
 	} else {
 		dev_dbg(dev, "init using platform data\n");
-		bq24257_pdata_probe(bq, pdata);
+		bq2425x_pdata_probe(bq, pdata);
 	}
 
 	/*
@@ -1205,7 +1205,7 @@ static int bq24257_probe(struct i2c_client *client,
 
 	if (!bq->in_ilimit_autoset_disable)
 		INIT_DELAYED_WORK(&bq->iilimit_setup_work,
-				bq24257_iilimit_setup_work);
+				bq2425x_iilimit_setup_work);
 
 	/*
 	 * The BQ24250 doesn't have a dedicated Power Good (PG) pin so we
@@ -1221,7 +1221,7 @@ static int bq24257_probe(struct i2c_client *client,
 	 * use of the dedicated pin is preferred.
 	 */
 	if (!bq->pg_gpio_disable) {
-		ret = bq24257_pg_gpio_probe(bq);
+		ret = bq2425x_pg_gpio_probe(bq);
 		if (ret < 0) {
 			dev_info(bq->dev,
 				"using SW-based power-good detection\n");
@@ -1230,7 +1230,7 @@ static int bq24257_probe(struct i2c_client *client,
 	}
 
 	/* reset all registers to defaults */
-	ret = bq24257_field_write(bq, F_RESET, 1);
+	ret = bq2425x_field_write(bq, F_RESET, 1);
 	if (ret < 0)
 		return ret;
 
@@ -1239,11 +1239,11 @@ static int bq24257_probe(struct i2c_client *client,
 	 * returns 1 on this bit, so this is the only way to avoid resetting the
 	 * chip every time we update another field in this register.
 	 */
-	ret = bq24257_field_write(bq, F_RESET, 0);
+	ret = bq2425x_field_write(bq, F_RESET, 0);
 	if (ret < 0)
 		return ret;
 
-	ret = bq24257_hw_init(bq);
+	ret = bq2425x_hw_init(bq);
 	if (ret < 0) {
 		dev_err(dev, "Cannot initialize the chip.\n");
 		return ret;
@@ -1251,12 +1251,12 @@ static int bq24257_probe(struct i2c_client *client,
 
 	/*
 	 * 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
+	 * or probe for a pin named BQ2425X_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);
+		client->irq = bq2425x_irq_probe(bq);
 
 	if (client->irq < 0) {
 		dev_err(dev, "no irq resource found\n");
@@ -1264,20 +1264,20 @@ static int bq24257_probe(struct i2c_client *client,
 	}
 
 	ret = devm_request_threaded_irq(dev, client->irq, NULL,
-					bq24257_irq_handler_thread,
+					bq2425x_irq_handler_thread,
 					IRQF_TRIGGER_FALLING |
 					IRQF_TRIGGER_RISING | IRQF_ONESHOT,
-					BQ24257_STAT_IRQ, bq);
+					BQ2425X_STAT_IRQ, bq);
 	if (ret)
 		return ret;
 
-	ret = bq24257_power_supply_init(bq);
+	ret = bq2425x_power_supply_init(bq);
 	if (ret < 0) {
 		dev_err(dev, "Failed to register power supply\n");
 		return ret;
 	}
 
-	ret = sysfs_create_group(&bq->charger->dev.kobj, &bq24257_attr_group);
+	ret = sysfs_create_group(&bq->charger->dev.kobj, &bq2425x_attr_group);
 	if (ret < 0) {
 		dev_err(dev, "Can't create sysfs entries\n");
 		return ret;
@@ -1286,26 +1286,26 @@ static int bq24257_probe(struct i2c_client *client,
 	return 0;
 }
 
-static int bq24257_remove(struct i2c_client *client)
+static int bq2425x_remove(struct i2c_client *client)
 {
-	struct bq24257_device *bq = i2c_get_clientdata(client);
+	struct bq2425x_device *bq = i2c_get_clientdata(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);
+	sysfs_remove_group(&bq->charger->dev.kobj, &bq2425x_attr_group);
 
 	power_supply_unregister(bq->charger);
 
-	bq24257_field_write(bq, F_RESET, 1); /* reset to defaults */
+	bq2425x_field_write(bq, F_RESET, 1); /* reset to defaults */
 
 	return 0;
 }
 
 #ifdef CONFIG_PM_SLEEP
-static int bq24257_suspend(struct device *dev)
+static int bq2425x_suspend(struct device *dev)
 {
-	struct bq24257_device *bq = dev_get_drvdata(dev);
+	struct bq2425x_device *bq = dev_get_drvdata(dev);
 	int ret = 0;
 
 	if (!bq->in_ilimit_autoset_disable)
@@ -1314,7 +1314,7 @@ static int bq24257_suspend(struct device *dev)
 	mutex_lock(&bq->lock);
 
 	/* reset all registers to default (and activate standalone mode) */
-	ret = bq24257_field_write(bq, F_RESET, 1);
+	ret = bq2425x_field_write(bq, F_RESET, 1);
 	if (ret < 0)
 		dev_err(bq->dev, "Cannot reset chip to standalone mode.\n");
 
@@ -1323,22 +1323,22 @@ static int bq24257_suspend(struct device *dev)
 	return ret;
 }
 
-static int bq24257_resume(struct device *dev)
+static int bq2425x_resume(struct device *dev)
 {
 	int ret;
-	struct bq24257_device *bq = dev_get_drvdata(dev);
+	struct bq2425x_device *bq = dev_get_drvdata(dev);
 
 	mutex_lock(&bq->lock);
 
-	ret = regcache_drop_region(bq->rmap, BQ24257_REG_1, BQ24257_REG_7);
+	ret = regcache_drop_region(bq->rmap, BQ2425X_REG_1, BQ2425X_REG_7);
 	if (ret < 0)
 		goto err;
 
-	ret = bq24257_field_write(bq, F_RESET, 0);
+	ret = bq2425x_field_write(bq, F_RESET, 0);
 	if (ret < 0)
 		goto err;
 
-	ret = bq24257_hw_init(bq);
+	ret = bq2425x_hw_init(bq);
 	if (ret < 0) {
 		dev_err(bq->dev, "Cannot init chip after resume.\n");
 		goto err;
@@ -1356,19 +1356,19 @@ err:
 }
 #endif
 
-static const struct dev_pm_ops bq24257_pm = {
-	SET_SYSTEM_SLEEP_PM_OPS(bq24257_suspend, bq24257_resume)
+static const struct dev_pm_ops bq2425x_pm = {
+	SET_SYSTEM_SLEEP_PM_OPS(bq2425x_suspend, bq2425x_resume)
 };
 
-static const struct i2c_device_id bq24257_i2c_ids[] = {
+static const struct i2c_device_id bq2425x_i2c_ids[] = {
 	{ "bq24250", BQ24250 },
 	{ "bq24251", BQ24251 },
 	{ "bq24257", BQ24257 },
 	{},
 };
-MODULE_DEVICE_TABLE(i2c, bq24257_i2c_ids);
+MODULE_DEVICE_TABLE(i2c, bq2425x_i2c_ids);
 
-static const struct of_device_id bq24257_of_match[] = {
+static const struct of_device_id bq2425x_of_match[] = {
 	{
 		.compatible = "ti,bq24250",
 		.compatible = "ti,bq24251",
@@ -1376,29 +1376,29 @@ static const struct of_device_id bq24257_of_match[] = {
 	},
 	{ },
 };
-MODULE_DEVICE_TABLE(of, bq24257_of_match);
+MODULE_DEVICE_TABLE(of, bq2425x_of_match);
 
-static const struct acpi_device_id bq24257_acpi_match[] = {
+static const struct acpi_device_id bq2425x_acpi_match[] = {
 	{ "BQ242500", BQ24250 },
 	{ "BQ242510", BQ24251 },
 	{ "BQ242570", BQ24257 },
 	{},
 };
-MODULE_DEVICE_TABLE(acpi, bq24257_acpi_match);
+MODULE_DEVICE_TABLE(acpi, bq2425x_acpi_match);
 
-static struct i2c_driver bq24257_driver = {
+static struct i2c_driver bq2425x_driver = {
 	.driver = {
-		.name = "bq24257-charger",
-		.of_match_table = of_match_ptr(bq24257_of_match),
-		.acpi_match_table = ACPI_PTR(bq24257_acpi_match),
-		.pm = &bq24257_pm,
+		.name = "bq2425x-charger",
+		.of_match_table = of_match_ptr(bq2425x_of_match),
+		.acpi_match_table = ACPI_PTR(bq2425x_acpi_match),
+		.pm = &bq2425x_pm,
 	},
-	.probe = bq24257_probe,
-	.remove = bq24257_remove,
-	.id_table = bq24257_i2c_ids,
+	.probe = bq2425x_probe,
+	.remove = bq2425x_remove,
+	.id_table = bq2425x_i2c_ids,
 };
-module_i2c_driver(bq24257_driver);
+module_i2c_driver(bq2425x_driver);
 
 MODULE_AUTHOR("Laurentiu Palcu <laurentiu.palcu@intel.com>");
-MODULE_DESCRIPTION("bq24257 charger driver");
+MODULE_DESCRIPTION("bq2425x charger driver");
 MODULE_LICENSE("GPL");
diff --git a/include/linux/power/bq24257_charger.h b/include/linux/power/bq2425x_charger.h
similarity index 85%
rename from include/linux/power/bq24257_charger.h
rename to include/linux/power/bq2425x_charger.h
index 7fc505b..782667e 100644
--- a/include/linux/power/bq24257_charger.h
+++ b/include/linux/power/bq2425x_charger.h
@@ -1,18 +1,18 @@
 /*
- * Platform data for the TI bq24257 battery charger driver.
+ * Platform data for the TI bq2425x 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_
+#ifndef _BQ2425X_CHARGER_H_
+#define _BQ2425X_CHARGER_H_
 
 #include <asm/types.h>
 #include <linux/types.h>
 
-struct bq24257_platform_data {
+struct bq2425x_platform_data {
 	u32 ichg;				       /* charge current (uA) */
 	u32 vbat;				   /* regulation voltage (uV) */
 	u32 iterm;				  /* termination current (uA) */
-- 
1.9.1

Re: [PATCH v2 12/13] power: bq24257: Renaming for consistency

[Laurentiu Palcu]

On Tue, Sep 08, 2015 at 07:12:36PM -0500, Andreas Dannenberg wrote:
> Rename files and refactor definitions, function names, etc as well as
> the driver name itself to reflect that this driver supports multiple
> devices and not just the bq24257.
> 
> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
Reviewed-by: Laurentiu Palcu <laurentiu.palcu@intel.com>

[PATCH v2 10/13] power: bq24257: Add various device-specific sysfs properties

[Andreas Dannenberg]

This patch adds support to enable/disable some device specific features
through device properties at boot time or sysfs properties at runtime
depending on whether they should be accessible during runtime or not.
The use of those features is be optional however they might be helpful
in certain scenarios:

* High-impedance mode enable/disable (through sysfs r/w)
* Sysoff enable/disable (through sysfs r/w)
* 2X Timer enable/disable (through "ti,safety-timer-2x-enable" dev
  property)

If not explicitly modified those settings are left at their device-
specific power-on defaults. Refer to the respectice 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-l0cyMroinI0@public.gmane.org>
---
 drivers/power/bq24257_charger.c | 75 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c
index f0602b3..9a4a7a0 100644
--- a/drivers/power/bq24257_charger.c
+++ b/drivers/power/bq24257_charger.c
@@ -77,6 +77,7 @@ struct bq24257_init_data {
 	u8 in_ilimit;	/* input current limit */
 	u8 vovp;	/* over voltage protection voltage */
 	u8 vindpm;	/* VDMP input threshold voltage */
+	bool timer2x_enable;	/* slow down safety timer by 2x */
 };
 
 struct bq24257_state {
@@ -766,6 +767,7 @@ static int bq24257_hw_init(struct bq24257_device *bq)
 		{F_ITERM, bq->init_data.iterm},
 		{F_VOVP, bq->init_data.vovp},
 		{F_VINDPM, bq->init_data.vindpm},
+		{F_X2_TMR_EN, bq->init_data.timer2x_enable},
 	};
 
 	/*
@@ -860,12 +862,78 @@ 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;
+
+	mutex_lock(&bq->lock);
+
+	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 if (strcmp(attr->attr.name, "timer2x_enable") == 0)
+		ret = bq->init_data.timer2x_enable;
+	else
+		ret = -EINVAL;
+
+	mutex_unlock(&bq->lock);
+
+	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;
+
+	mutex_lock(&bq->lock);
+
+	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
+		ret = -EINVAL;
+
+	mutex_unlock(&bq->lock);
+
+	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 DEVICE_ATTR(timer2x_enable, S_IRUGO,
+		bq24257_sysfs_show_enable, NULL);
 
 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,
+	&dev_attr_timer2x_enable.attr,
 	NULL,
 };
 
@@ -973,6 +1041,13 @@ static int bq24257_fw_probe(struct bq24257_device *bq)
 		bq->init_data.vindpm = bq24257_find_idx(property,
 				bq24257_vindpm_map, BQ24257_VINDPM_MAP_SIZE);
 
+	ret = device_property_read_u32(bq->dev, "ti,safety-timer-2x-enable",
+				       &property);
+	if (ret < 0)
+		bq->init_data.timer2x_enable = false;
+	else
+		bq->init_data.timer2x_enable = property;
+
 	return 0;
 }
 
-- 
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

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

[Laurentiu Palcu]

On Tue, Sep 08, 2015 at 07:12:34PM -0500, Andreas Dannenberg wrote:
> This patch adds support to enable/disable some device specific features
> through device properties at boot time or sysfs properties at runtime
> depending on whether they should be accessible during runtime or not.
> The use of those features is be optional however they might be helpful
> in certain scenarios:
> 
> * High-impedance mode enable/disable (through sysfs r/w)
> * Sysoff enable/disable (through sysfs r/w)
> * 2X Timer enable/disable (through "ti,safety-timer-2x-enable" dev
>   property)
> 
> If not explicitly modified those settings are left at their device-
> specific power-on defaults. Refer to the respectice 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-l0cyMroinI0@public.gmane.org>

I'm OK with adding these properties, but I'll wait for your explanation
on the mutex patch since I really don't think you need it, except for
protecting chip state data.

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

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

[Andreas Dannenberg]

On Thu, Sep 10, 2015 at 05:13:32PM +0300, Laurentiu Palcu wrote:
> On Tue, Sep 08, 2015 at 07:12:34PM -0500, Andreas Dannenberg wrote:
> > This patch adds support to enable/disable some device specific features
> > through device properties at boot time or sysfs properties at runtime
> > depending on whether they should be accessible during runtime or not.
> > The use of those features is be optional however they might be helpful
> > in certain scenarios:
> > 
> > * High-impedance mode enable/disable (through sysfs r/w)
> > * Sysoff enable/disable (through sysfs r/w)
> > * 2X Timer enable/disable (through "ti,safety-timer-2x-enable" dev
> >   property)
> > 
> > If not explicitly modified those settings are left at their device-
> > specific power-on defaults. Refer to the respectice 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>
> 
> I'm OK with adding these properties, but I'll wait for your explanation
> on the mutex patch since I really don't think you need it, except for
> protecting chip state data.

Yes as per the other thread we don't need additional mutex protection
after all.

Regards,

--
Andreas Dannenberg
Texas Instruments Inc


> 
> laurentiu

[PATCH v2 11/13] 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-l0cyMroinI0@public.gmane.org>
---
 drivers/power/bq24257_charger.c       | 95 +++++++++++++++++++++++++++++++++--
 include/linux/power/bq24257_charger.h | 30 +++++++++++
 2 files changed, 120 insertions(+), 5 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 9a4a7a0..b5e82ed 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
@@ -958,28 +961,102 @@ 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);
+	else {
+		if (!gpio_is_valid(pdata->stat_gpio)) {
+			dev_err(bq->dev, "invalid stat_irq pin\n");
+			return -EINVAL;
+		}
+
+		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);
+	}
 
-	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);
 	}
 
+	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);
+	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);
+	else {
+		if (!gpio_is_valid(pdata->pg_gpio)) {
+			dev_err(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_err(bq->dev, "PG pin request failed\n");
+			return ret;
+		}
+
+		bq->pg = gpio_to_desc(pdata->pg_gpio);
+	}
+
 	if (IS_ERR(bq->pg)) {
 		dev_info(bq->dev, "could not probe PG pin\n");
 		return PTR_ERR(bq->pg);
 	}
 
+	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->init_data.timer2x_enable = pdata->timer2x_enable;
+
+	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;
@@ -1057,6 +1134,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;
@@ -1106,14 +1184,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);
 	}
 
 	/*
@@ -1170,7 +1249,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..7fc505b
--- /dev/null
+++ b/include/linux/power/bq24257_charger.h
@@ -0,0 +1,30 @@
+/*
+ * 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 */
+	bool timer2x_enable;		      /* slow down safety timer by 2x */
+
+	int stat_gpio;				  /* stat_int (interrupt) pin */
+	int pg_gpio;					    /* power good pin */
+};
+
+#endif
-- 
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

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

[Laurentiu Palcu]

On Tue, Sep 08, 2015 at 07:12:35PM -0500, Andreas Dannenberg wrote:
> The patch adds a way to setup and initialize the device through the use
> of platform data with configuration options equivalent to when using
> device firmware (DT or ACPI) for systems where this is not available.
> 
> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> ---
>  drivers/power/bq24257_charger.c       | 95 +++++++++++++++++++++++++++++++++--
>  include/linux/power/bq24257_charger.h | 30 +++++++++++
>  2 files changed, 120 insertions(+), 5 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 9a4a7a0..b5e82ed 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
> @@ -958,28 +961,102 @@ 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);
> +	else {
> +		if (!gpio_is_valid(pdata->stat_gpio)) {
> +			dev_err(bq->dev, "invalid stat_irq pin\n");
> +			return -EINVAL;
> +		}
> +
> +		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);
> +	}
>  
> -	stat_irq = devm_gpiod_get_index(bq->dev, BQ24257_STAT_IRQ, 0, GPIOD_IN);
>  	if (IS_ERR(stat_irq)) {
gpio_to_desc() can return NULL. In this case, IS_ERR() will be false. I
think this is not what you want...

The rest seems fine.

laurentiu

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

[Andreas Dannenberg]

On Thu, Sep 10, 2015 at 05:40:53PM +0300, Laurentiu Palcu wrote:
> On Tue, Sep 08, 2015 at 07:12:35PM -0500, Andreas Dannenberg wrote:
> > The patch adds a way to setup and initialize the device through the use
> > of platform data with configuration options equivalent to when using
> > device firmware (DT or ACPI) for systems where this is not available.
> > 
> > Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> > ---
> >  drivers/power/bq24257_charger.c       | 95 +++++++++++++++++++++++++++++++++--
> >  include/linux/power/bq24257_charger.h | 30 +++++++++++
> >  2 files changed, 120 insertions(+), 5 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 9a4a7a0..b5e82ed 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
> > @@ -958,28 +961,102 @@ 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);
> > +	else {
> > +		if (!gpio_is_valid(pdata->stat_gpio)) {
> > +			dev_err(bq->dev, "invalid stat_irq pin\n");
> > +			return -EINVAL;
> > +		}
> > +
> > +		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);
> > +	}
> >  
> > -	stat_irq = devm_gpiod_get_index(bq->dev, BQ24257_STAT_IRQ, 0, GPIOD_IN);
> >  	if (IS_ERR(stat_irq)) {
> gpio_to_desc() can return NULL. In this case, IS_ERR() will be false. I
> think this is not what you want...

Good catch! Fixed.

Thanks,

--
Andreas Dannenberg
Texas Instruments Inc

> 
> The rest seems fine.
> 
> laurentiu

[PATCH v2 13/13] dt: power: bq2425x-charger: Cover additional devices

[Andreas Dannenberg]

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

Signed-off-by: Andreas Dannenberg <dannenberg-l0cyMroinI0@public.gmane.org>
---
 .../devicetree/bindings/power/bq24257.txt          | 21 -------
 .../devicetree/bindings/power/bq2425x.txt          | 70 ++++++++++++++++++++++
 2 files changed, 70 insertions(+), 21 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/power/bq24257.txt
 create mode 100644 Documentation/devicetree/bindings/power/bq2425x.txt

diff --git a/Documentation/devicetree/bindings/power/bq24257.txt b/Documentation/devicetree/bindings/power/bq24257.txt
deleted file mode 100644
index 5c9d394..0000000
--- a/Documentation/devicetree/bindings/power/bq24257.txt
+++ /dev/null
@@ -1,21 +0,0 @@
-Binding for TI bq24257 Li-Ion Charger
-
-Required properties:
-- compatible: Should contain one of the following:
- * "ti,bq24257"
-- reg:			   integer, i2c address of the device.
-- 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).
-
-Example:
-
-bq24257 {
-	compatible = "ti,bq24257";
-	reg = <0x6a>;
-
-	ti,battery-regulation-voltage = <4200000>;
-	ti,charge-current = <1000000>;
-	ti,termination-current = <50000>;
-};
diff --git a/Documentation/devicetree/bindings/power/bq2425x.txt b/Documentation/devicetree/bindings/power/bq2425x.txt
new file mode 100644
index 0000000..3e171c3
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/bq2425x.txt
@@ -0,0 +1,70 @@
+Binding for TI bq2425x 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.
+- 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).
+
+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.
+- ti,safety-timer-2x-enable: If this property is set to 1 the device's safety
+    timer is extended (slowed down) by a factor of two. Setting this property
+    to 0 or not providing it will leave the safety timer at its default
+    setting.
+- 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

Re: [PATCH v2 13/13] dt: power: bq2425x-charger: Cover additional devices

[Krzysztof Kozlowski]

On 09.09.2015 09:12, Andreas Dannenberg wrote:
> Extend the bq2425x charger's device tree documentation to cover the
> bq24250 and bq24257 devices as well as recent feature additions.
> 
> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>

Hi,

Thanks for updates. Everything pointed previous looks good. After
looking at Ramakrishna Pallala's (Cc-ed) patch ("power: bq24261_charger:
Add support for TI BQ24261 charger") I have only one comment below.

> ---
>  .../devicetree/bindings/power/bq24257.txt          | 21 -------
>  .../devicetree/bindings/power/bq2425x.txt          | 70 ++++++++++++++++++++++
>  2 files changed, 70 insertions(+), 21 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/power/bq24257.txt
>  create mode 100644 Documentation/devicetree/bindings/power/bq2425x.txt
> 
> diff --git a/Documentation/devicetree/bindings/power/bq24257.txt b/Documentation/devicetree/bindings/power/bq24257.txt
> deleted file mode 100644
> index 5c9d394..0000000
> --- a/Documentation/devicetree/bindings/power/bq24257.txt
> +++ /dev/null
> @@ -1,21 +0,0 @@
> -Binding for TI bq24257 Li-Ion Charger
> -
> -Required properties:
> -- compatible: Should contain one of the following:
> - * "ti,bq24257"
> -- reg:			   integer, i2c address of the device.
> -- 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).
> -
> -Example:
> -
> -bq24257 {
> -	compatible = "ti,bq24257";
> -	reg = <0x6a>;
> -
> -	ti,battery-regulation-voltage = <4200000>;
> -	ti,charge-current = <1000000>;
> -	ti,termination-current = <50000>;
> -};
> diff --git a/Documentation/devicetree/bindings/power/bq2425x.txt b/Documentation/devicetree/bindings/power/bq2425x.txt
> new file mode 100644
> index 0000000..3e171c3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/bq2425x.txt
> @@ -0,0 +1,70 @@
> +Binding for TI bq2425x 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.
> +- 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).
> +
> +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).

I understand this is different property than "ti,charge-current:
integer, default charging current (in mA)" from bq24261_charger patch?

This one is for setting limit on current drawn from the charger and the
bq24261's is to limit current delivered to battery?

Best regards,
Krzysztof

> +- 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.
> +- ti,safety-timer-2x-enable: If this property is set to 1 the device's safety
> +    timer is extended (slowed down) by a factor of two. Setting this property
> +    to 0 or not providing it will leave the safety timer at its default
> +    setting.
> +- 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>;
> +};
> 

Re: [PATCH v2 13/13] dt: power: bq2425x-charger: Cover additional devices

[Andreas Dannenberg]

On Wed, Sep 09, 2015 at 09:27:06AM +0900, Krzysztof Kozlowski wrote:
> On 09.09.2015 09:12, Andreas Dannenberg wrote:
> > Extend the bq2425x charger's device tree documentation to cover the
> > bq24250 and bq24257 devices as well as recent feature additions.
> > 
> > Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> 
> Hi,
> 
> Thanks for updates. Everything pointed previous looks good. After
> looking at Ramakrishna Pallala's (Cc-ed) patch ("power: bq24261_charger:
> Add support for TI BQ24261 charger") I have only one comment below.

Thanks for all your efforts and time checking those patches!

> 
> > ---
> >  .../devicetree/bindings/power/bq24257.txt          | 21 -------
> >  .../devicetree/bindings/power/bq2425x.txt          | 70 ++++++++++++++++++++++
> >  2 files changed, 70 insertions(+), 21 deletions(-)
> >  delete mode 100644 Documentation/devicetree/bindings/power/bq24257.txt
> >  create mode 100644 Documentation/devicetree/bindings/power/bq2425x.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/power/bq24257.txt b/Documentation/devicetree/bindings/power/bq24257.txt
> > deleted file mode 100644
> > index 5c9d394..0000000
> > --- a/Documentation/devicetree/bindings/power/bq24257.txt
> > +++ /dev/null
> > @@ -1,21 +0,0 @@
> > -Binding for TI bq24257 Li-Ion Charger
> > -
> > -Required properties:
> > -- compatible: Should contain one of the following:
> > - * "ti,bq24257"
> > -- reg:			   integer, i2c address of the device.
> > -- 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).
> > -
> > -Example:
> > -
> > -bq24257 {
> > -	compatible = "ti,bq24257";
> > -	reg = <0x6a>;
> > -
> > -	ti,battery-regulation-voltage = <4200000>;
> > -	ti,charge-current = <1000000>;
> > -	ti,termination-current = <50000>;
> > -};
> > diff --git a/Documentation/devicetree/bindings/power/bq2425x.txt b/Documentation/devicetree/bindings/power/bq2425x.txt
> > new file mode 100644
> > index 0000000..3e171c3
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/power/bq2425x.txt
> > @@ -0,0 +1,70 @@
> > +Binding for TI bq2425x 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.
> > +- 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).
> > +
> > +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).
> 
> I understand this is different property than "ti,charge-current:
> integer, default charging current (in mA)" from bq24261_charger patch?

Correct this is what "ti,current-limit" is for. And I borrowed that
definition from bq2415x_charger.c where it's used in the same context.
The reason for this property to exist is for systems where the external
power supply does more than just charging the battery, such as supplying
the system while it's charging or after it has finished charging. All
this only makes sense of course if the "ti,current-limit" is larger than
"ti,charge-current".

And if you see a few lines above the bq24257 driver also has a
"ti,charge-current" property. And yes this is what actually controls how
much current is delivered to the battery.

> This one is for setting limit on current drawn from the charger and the
> bq24261's is to limit current delivered to battery?

The bq24261 driver only exposes a "ti,charge-current" property which is
used in accordance with how other drivers use it. And it also supports
setting the input current limit but only automatically and not via DT.

Regards,

--
Andreas Dannenberg
Texas Instruments Inc


> 
> Best regards,
> Krzysztof
> 
> > +- 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.
> > +- ti,safety-timer-2x-enable: If this property is set to 1 the device's safety
> > +    timer is extended (slowed down) by a factor of two. Setting this property
> > +    to 0 or not providing it will leave the safety timer at its default
> > +    setting.
> > +- 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>;
> > +};
> > 
> 

Re: [PATCH v2 13/13] dt: power: bq2425x-charger: Cover additional devices

[Krzysztof Kozlowski]

On 09.09.2015 11:48, Andreas Dannenberg wrote:
> On Wed, Sep 09, 2015 at 09:27:06AM +0900, Krzysztof Kozlowski wrote:
>> On 09.09.2015 09:12, Andreas Dannenberg wrote:
>>> Extend the bq2425x charger's device tree documentation to cover the
>>> bq24250 and bq24257 devices as well as recent feature additions.
>>>
>>> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
>>
>> Hi,
>>
>> Thanks for updates. Everything pointed previous looks good. After
>> looking at Ramakrishna Pallala's (Cc-ed) patch ("power: bq24261_charger:
>> Add support for TI BQ24261 charger") I have only one comment below.
> 
> Thanks for all your efforts and time checking those patches!
> 
>>
>>> ---
>>>  .../devicetree/bindings/power/bq24257.txt          | 21 -------
>>>  .../devicetree/bindings/power/bq2425x.txt          | 70 ++++++++++++++++++++++
>>>  2 files changed, 70 insertions(+), 21 deletions(-)
>>>  delete mode 100644 Documentation/devicetree/bindings/power/bq24257.txt
>>>  create mode 100644 Documentation/devicetree/bindings/power/bq2425x.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/power/bq24257.txt b/Documentation/devicetree/bindings/power/bq24257.txt
>>> deleted file mode 100644
>>> index 5c9d394..0000000
>>> --- a/Documentation/devicetree/bindings/power/bq24257.txt
>>> +++ /dev/null
>>> @@ -1,21 +0,0 @@
>>> -Binding for TI bq24257 Li-Ion Charger
>>> -
>>> -Required properties:
>>> -- compatible: Should contain one of the following:
>>> - * "ti,bq24257"
>>> -- reg:			   integer, i2c address of the device.
>>> -- 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).
>>> -
>>> -Example:
>>> -
>>> -bq24257 {
>>> -	compatible = "ti,bq24257";
>>> -	reg = <0x6a>;
>>> -
>>> -	ti,battery-regulation-voltage = <4200000>;
>>> -	ti,charge-current = <1000000>;
>>> -	ti,termination-current = <50000>;
>>> -};
>>> diff --git a/Documentation/devicetree/bindings/power/bq2425x.txt b/Documentation/devicetree/bindings/power/bq2425x.txt
>>> new file mode 100644
>>> index 0000000..3e171c3
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/power/bq2425x.txt
>>> @@ -0,0 +1,70 @@
>>> +Binding for TI bq2425x 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.
>>> +- 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).
>>> +
>>> +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).
>>
>> I understand this is different property than "ti,charge-current:
>> integer, default charging current (in mA)" from bq24261_charger patch?
> 
> Correct this is what "ti,current-limit" is for. And I borrowed that
> definition from bq2415x_charger.c where it's used in the same context.
> The reason for this property to exist is for systems where the external
> power supply does more than just charging the battery, such as supplying
> the system while it's charging or after it has finished charging. All
> this only makes sense of course if the "ti,current-limit" is larger than
> "ti,charge-current".
> 
> And if you see a few lines above the bq24257 driver also has a
> "ti,charge-current" property. And yes this is what actually controls how
> much current is delivered to the battery.

Right, I missed that one. Everything is clear now.

So we have different bindings. Existing ones:
bq2415x.txt - ti,charge-current - maximum charging current in mA
bq24257.txt - ti,charge-current - maximum charging current in uA
bq25890.txt - ti,charge-current - maximum charging current in uA
bq24735.txt - ti,charge-current - charge current (?) in mA
bq2415x.txt - ti,current-limit  - maximum current to be drawn in mA

New bindings:
ti,charge-current (bq24261) - default charge current in mA
ti,max-charge-current (bq24261) - maximum charging current in mA
ti,current-limit (bq2425x) - maximum current to be drawn in uA


Damn it... It's a mess. And there is no device prefixes...

The bq24261's bindings look most sensible (max prefix for max charge
current) but they are not compatible with existing bindings for
different devices.

There is no way to unify or make consistent all of these bindings.
However one could try to add new stuff in a more sensible way. For
example how about (for bq2425x-charger and bq24261_charger... BTW notice
even the difference in using underscore and hyphen!):

ti,charge-current - maximum charging current in uA
(that one must be supported, it's for existing bq24257 devices)
ti,default-charge-current (bq24261) - default charge current in *uA*
ti,current-limit (bq2425x) - maximum current to be drawn in *uA*


Best regards,
Krzysztof

Re: [PATCH v2 13/13] dt: power: bq2425x-charger: Cover additional devices

[Andreas Dannenberg]

On Wed, Sep 09, 2015 at 01:58:09PM +0900, Krzysztof Kozlowski wrote:
> On 09.09.2015 11:48, Andreas Dannenberg wrote:
> > On Wed, Sep 09, 2015 at 09:27:06AM +0900, Krzysztof Kozlowski wrote:
> >> On 09.09.2015 09:12, Andreas Dannenberg wrote:
> >>> Extend the bq2425x charger's device tree documentation to cover the
> >>> bq24250 and bq24257 devices as well as recent feature additions.
> >>>
> >>> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> >>
> >> Hi,
> >>
> >> Thanks for updates. Everything pointed previous looks good. After
> >> looking at Ramakrishna Pallala's (Cc-ed) patch ("power: bq24261_charger:
> >> Add support for TI BQ24261 charger") I have only one comment below.
> > 
> > Thanks for all your efforts and time checking those patches!
> > 
> >>
> >>> ---
> >>>  .../devicetree/bindings/power/bq24257.txt          | 21 -------
> >>>  .../devicetree/bindings/power/bq2425x.txt          | 70 ++++++++++++++++++++++
> >>>  2 files changed, 70 insertions(+), 21 deletions(-)
> >>>  delete mode 100644 Documentation/devicetree/bindings/power/bq24257.txt
> >>>  create mode 100644 Documentation/devicetree/bindings/power/bq2425x.txt
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/power/bq24257.txt b/Documentation/devicetree/bindings/power/bq24257.txt
> >>> deleted file mode 100644
> >>> index 5c9d394..0000000
> >>> --- a/Documentation/devicetree/bindings/power/bq24257.txt
> >>> +++ /dev/null
> >>> @@ -1,21 +0,0 @@
> >>> -Binding for TI bq24257 Li-Ion Charger
> >>> -
> >>> -Required properties:
> >>> -- compatible: Should contain one of the following:
> >>> - * "ti,bq24257"
> >>> -- reg:			   integer, i2c address of the device.
> >>> -- 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).
> >>> -
> >>> -Example:
> >>> -
> >>> -bq24257 {
> >>> -	compatible = "ti,bq24257";
> >>> -	reg = <0x6a>;
> >>> -
> >>> -	ti,battery-regulation-voltage = <4200000>;
> >>> -	ti,charge-current = <1000000>;
> >>> -	ti,termination-current = <50000>;
> >>> -};
> >>> diff --git a/Documentation/devicetree/bindings/power/bq2425x.txt b/Documentation/devicetree/bindings/power/bq2425x.txt
> >>> new file mode 100644
> >>> index 0000000..3e171c3
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/power/bq2425x.txt
> >>> @@ -0,0 +1,70 @@
> >>> +Binding for TI bq2425x 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.
> >>> +- 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).
> >>> +
> >>> +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).
> >>
> >> I understand this is different property than "ti,charge-current:
> >> integer, default charging current (in mA)" from bq24261_charger patch?
> > 
> > Correct this is what "ti,current-limit" is for. And I borrowed that
> > definition from bq2415x_charger.c where it's used in the same context.
> > The reason for this property to exist is for systems where the external
> > power supply does more than just charging the battery, such as supplying
> > the system while it's charging or after it has finished charging. All
> > this only makes sense of course if the "ti,current-limit" is larger than
> > "ti,charge-current".
> > 
> > And if you see a few lines above the bq24257 driver also has a
> > "ti,charge-current" property. And yes this is what actually controls how
> > much current is delivered to the battery.
> 
> Right, I missed that one. Everything is clear now.
> 
> So we have different bindings. Existing ones:
> bq2415x.txt - ti,charge-current - maximum charging current in mA
> bq24257.txt - ti,charge-current - maximum charging current in uA
> bq25890.txt - ti,charge-current - maximum charging current in uA
> bq24735.txt - ti,charge-current - charge current (?) in mA

I just spent some time with the bq24735 datasheet and the way that
device appears to work is putting the user in control of the charging
process rather than implementing a fully automatic control loop, but
either way I still think it's valid to call the property
ti,charge-current and use a description of "maximum charging current"
for that device (there is no DT bindings doc however). And yes the unit
is mA as one can see from reverse-engineering the register settings.

On a related note the datasheet for that device says you have to
periodically re-send the charge current setting every 44...175s to keep
charging with the configured current or disable the device's watchdog
timer. Neither of which the driver seems to do. I can probably go back
and get some HW and test if that driver actually works as advertised.
(also see http://www.ti.com/lit/ds/symlink/bq24735.pdf, Page 21)

> bq2415x.txt - ti,current-limit  - maximum current to be drawn in mA
> 
> New bindings:
> ti,charge-current (bq24261) - default charge current in mA
> ti,max-charge-current (bq24261) - maximum charging current in mA

This ti,max-charge-current is actually an interesting one. It's not a
device setting as it does not impact any of the device registers at all.
Instead, it's an artificial limit that can be set through DT that
prevents somebody from going into sysfs and configuring a charge current
higher than ti,max-charge-current. In other drivers I have seen that the
sysfs property reflecting that max charge current is just read-only and
gives you the maximum the HW is capable of. From a device point of view
there is nothing configurable about this property.

> ti,current-limit (bq2425x) - maximum current to be drawn in uA
> 
> 
> Damn it... It's a mess. And there is no device prefixes...

ACK :)  Let's see how we can bring a little sense into it.
 
> The bq24261's bindings look most sensible (max prefix for max charge
> current) but they are not compatible with existing bindings for
> different devices.

Hmm I think they are compatible, it's just a question making the DT
bindings description for the bq24261 fit better into what's already
there. For example, like this:

(1) ti,charge-current: integer, maximum charging current in mA. For this
device as for others this setting controls the max current the device
uses to charge the battery so the established description is good
however the general use of this property name itself is not 100%
accurate (too late for that).
(2) ti,max-charge-current: Unless there is a good reason to keep it,
REMOVE this property (alongside ti,max-charge-voltage). Instead, have
the charger directly report back the maximum device charge current
(BQ24261_MAX_CC) via sysfs like most other charger drivers do (bq24190,
bq24257, bq25890, rt9455).

> There is no way to unify or make consistent all of these bindings.
> However one could try to add new stuff in a more sensible way. For
> example how about (for bq2425x-charger and bq24261_charger... BTW notice
> even the difference in using underscore and hyphen!):

:(  You are talking about the driver .name, right? I saw that too. If
the bq24261 charger was to change it's device name to use a hyphen at
least it would be consistent with bq2415x and bq2425x.

> ti,charge-current - maximum charging current in uA
> (that one must be supported, it's for existing bq24257 devices)

Agreed. As discussed earlier this one is pretty established -- but in
mA (not uA).

> ti,default-charge-current (bq24261) - default charge current in *uA*

We don't need that. If you look where it goes (registers) this should be
called ti,charge-current for the bq24261 (like it already is). Exactly
the same name/usage as the other bqxxxx chargers. We just need to update
the description.

> ti,current-limit (bq2425x) - maximum current to be drawn in *uA*

Ok. Again here my preference would be mA. Like already on the bq2415x.
If we can change the bq2425x driver to mA (see separate thread) we'd be
closer to a more unified set of properties. Otherwise we would have
properties with the same name but different units (is this even
possible?).

Regards,

--
Andreas Dannenberg
Texas Instruments Inc

Re: [PATCH v2 13/13] dt: power: bq2425x-charger: Cover additional devices

[Krzysztof Kozlowski]

On 10.09.2015 05:15, Andreas Dannenberg wrote:
> On Wed, Sep 09, 2015 at 01:58:09PM +0900, Krzysztof Kozlowski wrote:
>>
>> So we have different bindings. Existing ones:
>> bq2415x.txt - ti,charge-current - maximum charging current in mA
>> bq24257.txt - ti,charge-current - maximum charging current in uA
>> bq25890.txt - ti,charge-current - maximum charging current in uA
>> bq24735.txt - ti,charge-current - charge current (?) in mA
> 
> I just spent some time with the bq24735 datasheet and the way that
> device appears to work is putting the user in control of the charging
> process rather than implementing a fully automatic control loop, but
> either way I still think it's valid to call the property
> ti,charge-current and use a description of "maximum charging current"
> for that device (there is no DT bindings doc however). And yes the unit
> is mA as one can see from reverse-engineering the register settings.

Just for the record: the units used by device registers are not related
to units used in DT binding. You can use whatever you want. The driver
should just convert them.

> 
> On a related note the datasheet for that device says you have to
> periodically re-send the charge current setting every 44...175s to keep
> charging with the configured current or disable the device's watchdog
> timer. Neither of which the driver seems to do. I can probably go back
> and get some HW and test if that driver actually works as advertised.
> (also see http://www.ti.com/lit/ds/symlink/bq24735.pdf, Page 21)

I mentioned existing bindings for reference. To show that it's a mess.
However you cannot change them now (at least easily). You could add new
bindings and mark old as deprecated (still they would have to be
supported by the driver)...

> 
>> bq2415x.txt - ti,current-limit  - maximum current to be drawn in mA
>>
>> New bindings:
>> ti,charge-current (bq24261) - default charge current in mA
>> ti,max-charge-current (bq24261) - maximum charging current in mA
> 
> This ti,max-charge-current is actually an interesting one. It's not a
> device setting as it does not impact any of the device registers at all.
> Instead, it's an artificial limit that can be set through DT that
> prevents somebody from going into sysfs and configuring a charge current
> higher than ti,max-charge-current. In other drivers I have seen that the
> sysfs property reflecting that max charge current is just read-only and
> gives you the maximum the HW is capable of. From a device point of view
> there is nothing configurable about this property.

Hmmm, so now I wonder whether this should be a DT binding. The purpose
of DT isn't the control of the driver like enable some stuff, set some
value used by the driver. The DT provides information about hardware so
the driver could properly configure the device and work with it.

Reason to put this into DT would be:
For example one device configuration (device, board, connected battery)
could handle one maximum value and the other configuration would require
lower one. The device and its capabilities (bq24257 for example) are the
same but configuration changes.


If all configurations of bq24261 device have the same maximum value then
it should not be a DT property but it should be hard-coded in the driver
instead.

Ramakrishna,
Could you describe the reason behind "ti,max-charge-current" and
"pdata.max_cc" in bq24261 driver?

> 
>> ti,current-limit (bq2425x) - maximum current to be drawn in uA
>>
>>
>> Damn it... It's a mess. And there is no device prefixes...
> 
> ACK :)  Let's see how we can bring a little sense into it.
>  
>> The bq24261's bindings look most sensible (max prefix for max charge
>> current) but they are not compatible with existing bindings for
>> different devices.
> 
> Hmm I think they are compatible, it's just a question making the DT
> bindings description for the bq24261 fit better into what's already
> there. For example, like this:
> 
> (1) ti,charge-current: integer, maximum charging current in mA. For this
> device as for others this setting controls the max current the device
> uses to charge the battery so the established description is good
> however the general use of this property name itself is not 100%
> accurate (too late for that).

I agree.

> (2) ti,max-charge-current: Unless there is a good reason to keep it,
> REMOVE this property (alongside ti,max-charge-voltage). Instead, have
> the charger directly report back the maximum device charge current
> (BQ24261_MAX_CC) via sysfs like most other charger drivers do (bq24190,
> bq24257, bq25890, rt9455).

I agree but wait for some more data from Ramakrishna.

> 
>> There is no way to unify or make consistent all of these bindings.
>> However one could try to add new stuff in a more sensible way. For
>> example how about (for bq2425x-charger and bq24261_charger... BTW notice
>> even the difference in using underscore and hyphen!):
> 
> :(  You are talking about the driver .name, right? I saw that too. If
> the bq24261 charger was to change it's device name to use a hyphen at
> least it would be consistent with bq2415x and bq2425x.

The name of file and name of driver. Most of existing bq* drivers have
"_" in file name and "-" in driver name. So maybe use it in
bq2425x-charger (file name: bq2425x_charger)?

To prevent future issues in naming and bindings consistency maybe there
should be a dedicated maintainer for bq/TI charger devices? Pali Rohar
recently took care for devices related to Nokia N900 but maybe someone
from TI should also take care of rest or all of them (if TI is
interested in this)?

> 
>> ti,charge-current - maximum charging current in uA
>> (that one must be supported, it's for existing bq24257 devices)
> 
> Agreed. As discussed earlier this one is pretty established -- but in
> mA (not uA).

Okay.

> 
>> ti,default-charge-current (bq24261) - default charge current in *uA*
> 
> We don't need that. If you look where it goes (registers) this should be
> called ti,charge-current for the bq24261 (like it already is). Exactly
> the same name/usage as the other bqxxxx chargers. We just need to update
> the description.

Okay.

> 
>> ti,current-limit (bq2425x) - maximum current to be drawn in *uA*
> 
> Ok. Again here my preference would be mA. Like already on the bq2415x.
> If we can change the bq2425x driver to mA (see separate thread) we'd be
> closer to a more unified set of properties. Otherwise we would have
> properties with the same name but different units (is this even
> possible?).

mA would be nice... but bq2425x driver must support existing device
which means it must support existing bindings. Unfortunately the
existing binding for bq24257 is in uA.

Best regards,
Krzysztof

Re: [PATCH v2 13/13] dt: power: bq2425x-charger: Cover additional devices

[Laurentiu Palcu]

On Thu, Sep 10, 2015 at 09:15:09AM +0900, Krzysztof Kozlowski wrote:
> On 10.09.2015 05:15, Andreas Dannenberg wrote:
> > On Wed, Sep 09, 2015 at 01:58:09PM +0900, Krzysztof Kozlowski wrote:
> >>
> >> So we have different bindings. Existing ones:
> >> bq2415x.txt - ti,charge-current - maximum charging current in mA
> >> bq24257.txt - ti,charge-current - maximum charging current in uA
> >> bq25890.txt - ti,charge-current - maximum charging current in uA
> >> bq24735.txt - ti,charge-current - charge current (?) in mA
> > 
> > I just spent some time with the bq24735 datasheet and the way that
> > device appears to work is putting the user in control of the charging
> > process rather than implementing a fully automatic control loop, but
> > either way I still think it's valid to call the property
> > ti,charge-current and use a description of "maximum charging current"
> > for that device (there is no DT bindings doc however). And yes the unit
> > is mA as one can see from reverse-engineering the register settings.
> 
> Just for the record: the units used by device registers are not related
> to units used in DT binding. You can use whatever you want. The driver
> should just convert them.
> 
> > 
> > On a related note the datasheet for that device says you have to
> > periodically re-send the charge current setting every 44...175s to keep
> > charging with the configured current or disable the device's watchdog
> > timer. Neither of which the driver seems to do. I can probably go back
> > and get some HW and test if that driver actually works as advertised.
> > (also see http://www.ti.com/lit/ds/symlink/bq24735.pdf, Page 21)
> 
> I mentioned existing bindings for reference. To show that it's a mess.
> However you cannot change them now (at least easily). You could add new
> bindings and mark old as deprecated (still they would have to be
> supported by the driver)...
> 
> > 
> >> bq2415x.txt - ti,current-limit  - maximum current to be drawn in mA
> >>
> >> New bindings:
> >> ti,charge-current (bq24261) - default charge current in mA
> >> ti,max-charge-current (bq24261) - maximum charging current in mA
> > 
> > This ti,max-charge-current is actually an interesting one. It's not a
> > device setting as it does not impact any of the device registers at all.
> > Instead, it's an artificial limit that can be set through DT that
> > prevents somebody from going into sysfs and configuring a charge current
> > higher than ti,max-charge-current. In other drivers I have seen that the
> > sysfs property reflecting that max charge current is just read-only and
> > gives you the maximum the HW is capable of. From a device point of view
> > there is nothing configurable about this property.
> 
> Hmmm, so now I wonder whether this should be a DT binding. The purpose
> of DT isn't the control of the driver like enable some stuff, set some
> value used by the driver. The DT provides information about hardware so
> the driver could properly configure the device and work with it.
> 
> Reason to put this into DT would be:
> For example one device configuration (device, board, connected battery)
> could handle one maximum value and the other configuration would require
> lower one. The device and its capabilities (bq24257 for example) are the
> same but configuration changes.
> 
> 
> If all configurations of bq24261 device have the same maximum value then
> it should not be a DT property but it should be hard-coded in the driver
> instead.
> 
> Ramakrishna,
> Could you describe the reason behind "ti,max-charge-current" and
> "pdata.max_cc" in bq24261 driver?
> 
> > 
> >> ti,current-limit (bq2425x) - maximum current to be drawn in uA
> >>
> >>
> >> Damn it... It's a mess. And there is no device prefixes...
> > 
> > ACK :)  Let's see how we can bring a little sense into it.
> >  
> >> The bq24261's bindings look most sensible (max prefix for max charge
> >> current) but they are not compatible with existing bindings for
> >> different devices.
> > 
> > Hmm I think they are compatible, it's just a question making the DT
> > bindings description for the bq24261 fit better into what's already
> > there. For example, like this:
> > 
> > (1) ti,charge-current: integer, maximum charging current in mA. For this
> > device as for others this setting controls the max current the device
> > uses to charge the battery so the established description is good
> > however the general use of this property name itself is not 100%
> > accurate (too late for that).
> 
> I agree.
> 
> > (2) ti,max-charge-current: Unless there is a good reason to keep it,
> > REMOVE this property (alongside ti,max-charge-voltage). Instead, have
> > the charger directly report back the maximum device charge current
> > (BQ24261_MAX_CC) via sysfs like most other charger drivers do (bq24190,
> > bq24257, bq25890, rt9455).
> 
> I agree but wait for some more data from Ramakrishna.
> 
> > 
> >> There is no way to unify or make consistent all of these bindings.
> >> However one could try to add new stuff in a more sensible way. For
> >> example how about (for bq2425x-charger and bq24261_charger... BTW notice
> >> even the difference in using underscore and hyphen!):
> > 
> > :(  You are talking about the driver .name, right? I saw that too. If
> > the bq24261 charger was to change it's device name to use a hyphen at
> > least it would be consistent with bq2415x and bq2425x.
> 
> The name of file and name of driver. Most of existing bq* drivers have
> "_" in file name and "-" in driver name. So maybe use it in
> bq2425x-charger (file name: bq2425x_charger)?
> 
> To prevent future issues in naming and bindings consistency maybe there
> should be a dedicated maintainer for bq/TI charger devices? Pali Rohar
> recently took care for devices related to Nokia N900 but maybe someone
> from TI should also take care of rest or all of them (if TI is
> interested in this)?
> 
> > 
> >> ti,charge-current - maximum charging current in uA
> >> (that one must be supported, it's for existing bq24257 devices)
> > 
> > Agreed. As discussed earlier this one is pretty established -- but in
> > mA (not uA).
> 
> Okay.
> 
> > 
> >> ti,default-charge-current (bq24261) - default charge current in *uA*
> > 
> > We don't need that. If you look where it goes (registers) this should be
> > called ti,charge-current for the bq24261 (like it already is). Exactly
> > the same name/usage as the other bqxxxx chargers. We just need to update
> > the description.
> 
> Okay.
> 
> > 
> >> ti,current-limit (bq2425x) - maximum current to be drawn in *uA*
> > 
> > Ok. Again here my preference would be mA. Like already on the bq2415x.
> > If we can change the bq2425x driver to mA (see separate thread) we'd be
> > closer to a more unified set of properties. Otherwise we would have
> > properties with the same name but different units (is this even
> > possible?).
> 
> mA would be nice... but bq2425x driver must support existing device
> which means it must support existing bindings. Unfortunately the
> existing binding for bq24257 is in uA.

Oh boy... apparently this unit discrepancy mess for TI chargers was my
doing. :/

I replied on the other thread already on my bindings choice... As I said
there, all we can do now is agree on the binding names to be consistent.
I'm afraid the units (as Krzysztof pointed) are pretty much settled...
:|

laurentiu

Re: [PATCH v2 13/13] dt: power: bq2425x-charger: Cover additional devices

[Andreas Dannenberg]

On Thu, Sep 10, 2015 at 06:00:32PM +0300, Laurentiu Palcu wrote:
> On Thu, Sep 10, 2015 at 09:15:09AM +0900, Krzysztof Kozlowski wrote:
> > On 10.09.2015 05:15, Andreas Dannenberg wrote:

[...]

> > >> ti,current-limit (bq2425x) - maximum current to be drawn in *uA*
> > > 
> > > Ok. Again here my preference would be mA. Like already on the bq2415x.
> > > If we can change the bq2425x driver to mA (see separate thread) we'd be
> > > closer to a more unified set of properties. Otherwise we would have
> > > properties with the same name but different units (is this even
> > > possible?).
> > 
> > mA would be nice... but bq2425x driver must support existing device
> > which means it must support existing bindings. Unfortunately the
> > existing binding for bq24257 is in uA.
> 
> Oh boy... apparently this unit discrepancy mess for TI chargers was my
> doing. :/
> 
> I replied on the other thread already on my bindings choice... As I said
> there, all we can do now is agree on the binding names to be consistent.
> I'm afraid the units (as Krzysztof pointed) are pretty much settled...
> :|

Hi Laurentiu,

Yes let's keep the binding names consistent. And as for the units -
given the new state of the world (recent driver additions, inability to
change units on existing drivers) the way this aspects looks to me now
we should probably go wih uA/uV for all new bqXXX chargers. It's not a
big deal, looks like micro units are widely used in other parts of the
Kernel already (see IIO framework).

As indicated in the other thread I'll be happy to step in as the TI/bqXXX
DT bindings maintainer moving forward.

Regards,

--
Andreas Dannenberg
Texas Instruments Inc

Re: [PATCH v2 13/13] dt: power: bq2425x-charger: Cover additional devices

[Andreas Dannenberg]

On Thu, Sep 10, 2015 at 09:15:09AM +0900, Krzysztof Kozlowski wrote:
> On 10.09.2015 05:15, Andreas Dannenberg wrote:
> > On Wed, Sep 09, 2015 at 01:58:09PM +0900, Krzysztof Kozlowski wrote:
> >>
> >> So we have different bindings. Existing ones:
> >> bq2415x.txt - ti,charge-current - maximum charging current in mA
> >> bq24257.txt - ti,charge-current - maximum charging current in uA
> >> bq25890.txt - ti,charge-current - maximum charging current in uA
> >> bq24735.txt - ti,charge-current - charge current (?) in mA
> > 
> > I just spent some time with the bq24735 datasheet and the way that
> > device appears to work is putting the user in control of the charging
> > process rather than implementing a fully automatic control loop, but
> > either way I still think it's valid to call the property
> > ti,charge-current and use a description of "maximum charging current"
> > for that device (there is no DT bindings doc however). And yes the unit
> > is mA as one can see from reverse-engineering the register settings.
> 
> Just for the record: the units used by device registers are not related
> to units used in DT binding. You can use whatever you want. The driver
> should just convert them.
> 
> > 
> > On a related note the datasheet for that device says you have to
> > periodically re-send the charge current setting every 44...175s to keep
> > charging with the configured current or disable the device's watchdog
> > timer. Neither of which the driver seems to do. I can probably go back
> > and get some HW and test if that driver actually works as advertised.
> > (also see http://www.ti.com/lit/ds/symlink/bq24735.pdf, Page 21)
> 
> I mentioned existing bindings for reference. To show that it's a mess.
> However you cannot change them now (at least easily). You could add new
> bindings and mark old as deprecated (still they would have to be
> supported by the driver)...

Understood.

> >> bq2415x.txt - ti,current-limit  - maximum current to be drawn in mA
> >>
> >> New bindings:
> >> ti,charge-current (bq24261) - default charge current in mA
> >> ti,max-charge-current (bq24261) - maximum charging current in mA
> > 
> > This ti,max-charge-current is actually an interesting one. It's not a
> > device setting as it does not impact any of the device registers at all.
> > Instead, it's an artificial limit that can be set through DT that
> > prevents somebody from going into sysfs and configuring a charge current
> > higher than ti,max-charge-current. In other drivers I have seen that the
> > sysfs property reflecting that max charge current is just read-only and
> > gives you the maximum the HW is capable of. From a device point of view
> > there is nothing configurable about this property.
> 
> Hmmm, so now I wonder whether this should be a DT binding. The purpose
> of DT isn't the control of the driver like enable some stuff, set some
> value used by the driver. The DT provides information about hardware so
> the driver could properly configure the device and work with it.
> 
> Reason to put this into DT would be:
> For example one device configuration (device, board, connected battery)
> could handle one maximum value and the other configuration would require
> lower one. The device and its capabilities (bq24257 for example) are the
> same but configuration changes.

Yes this could be useful in this case but this property only makes sense
if userspace is allowed to change the actual charge current through
sysfs as some drivers do including Ram's proposed bq24261 driver. But if we
get rid of the general sysfs configurability of the charge current
(which I say we should) we don't need that ti,max-charge-current property.
Also see...

http://marc.info/?l=linux-pm&m=143080413218161&w=2
 
> If all configurations of bq24261 device have the same maximum value then
> it should not be a DT property but it should be hard-coded in the driver
> instead.
> 
> Ramakrishna,
> Could you describe the reason behind "ti,max-charge-current" and
> "pdata.max_cc" in bq24261 driver?

[...]
 
> >> There is no way to unify or make consistent all of these bindings.
> >> However one could try to add new stuff in a more sensible way. For
> >> example how about (for bq2425x-charger and bq24261_charger... BTW notice
> >> even the difference in using underscore and hyphen!):
> > 
> > :(  You are talking about the driver .name, right? I saw that too. If
> > the bq24261 charger was to change it's device name to use a hyphen at
> > least it would be consistent with bq2415x and bq2425x.
> 
> The name of file and name of driver. Most of existing bq* drivers have
> "_" in file name and "-" in driver name. So maybe use it in
> bq2425x-charger (file name: bq2425x_charger)?

Yes the bq2425x_charger already follows the same trend - underscore in
the filename and dash in the driver name. The bq26261_charger.c driver
is the one that should be adopted (already provided feedback on a
separate thread).

> To prevent future issues in naming and bindings consistency maybe there
> should be a dedicated maintainer for bq/TI charger devices? Pali Rohar
> recently took care for devices related to Nokia N900 but maybe someone
> from TI should also take care of rest or all of them (if TI is
> interested in this)?

I'd be happy to step in here. Let me know how I can help and contribute.
 
[...]

> >> ti,current-limit (bq2425x) - maximum current to be drawn in *uA*
> > 
> > Ok. Again here my preference would be mA. Like already on the bq2415x.
> > If we can change the bq2425x driver to mA (see separate thread) we'd be
> > closer to a more unified set of properties. Otherwise we would have
> > properties with the same name but different units (is this even
> > possible?).
> 
> mA would be nice... but bq2425x driver must support existing device
> which means it must support existing bindings. Unfortunately the
> existing binding for bq24257 is in uA.

Ok understood. I guess I interpreted your previous statement with you
being ok with either milli or micro too broadly. Will stick to what we
have then.

Regards,

--
Andreas Dannenberg
Texas Instruments Inc

Re: [PATCH v2 13/13] dt: power: bq2425x-charger: Cover additional devices

[Krzysztof Kozlowski]

On 11.09.2015 05:57, Andreas Dannenberg wrote:
> On Thu, Sep 10, 2015 at 09:15:09AM +0900, Krzysztof Kozlowski wrote:

(...)

> 
>>>> bq2415x.txt - ti,current-limit  - maximum current to be drawn in mA
>>>>
>>>> New bindings:
>>>> ti,charge-current (bq24261) - default charge current in mA
>>>> ti,max-charge-current (bq24261) - maximum charging current in mA
>>>
>>> This ti,max-charge-current is actually an interesting one. It's not a
>>> device setting as it does not impact any of the device registers at all.
>>> Instead, it's an artificial limit that can be set through DT that
>>> prevents somebody from going into sysfs and configuring a charge current
>>> higher than ti,max-charge-current. In other drivers I have seen that the
>>> sysfs property reflecting that max charge current is just read-only and
>>> gives you the maximum the HW is capable of. From a device point of view
>>> there is nothing configurable about this property.
>>
>> Hmmm, so now I wonder whether this should be a DT binding. The purpose
>> of DT isn't the control of the driver like enable some stuff, set some
>> value used by the driver. The DT provides information about hardware so
>> the driver could properly configure the device and work with it.
>>
>> Reason to put this into DT would be:
>> For example one device configuration (device, board, connected battery)
>> could handle one maximum value and the other configuration would require
>> lower one. The device and its capabilities (bq24257 for example) are the
>> same but configuration changes.
> 
> Yes this could be useful in this case but this property only makes sense
> if userspace is allowed to change the actual charge current through
> sysfs as some drivers do including Ram's proposed bq24261 driver. But if we
> get rid of the general sysfs configurability of the charge current
> (which I say we should) we don't need that ti,max-charge-current property.
> Also see...
> 
> http://marc.info/?l=linux-pm&m=143080413218161&w=2

Right, that's my previous reply. :) I must admit that here and for
bq24261 I looked mostly at bindings so I did not initially about the
sysfs interface.

Anyway I am not convinced to having such sysfs interfaces and definitely
they should not mess with DT. I would rather expect these to be totally
orthogonal.

Summarizing all these bq24261 properties:
 - ti,enable-user-write
 - ti,max-charge-current
 - ti,max-charge-voltage
are related to that sysfs interface, not to hardware configuration. If
that's true, then all should be gone.

> 
>> To prevent future issues in naming and bindings consistency maybe there
>> should be a dedicated maintainer for bq/TI charger devices? Pali Rohar
>> recently took care for devices related to Nokia N900 but maybe someone
>> from TI should also take care of rest or all of them (if TI is
>> interested in this)?
> 
> I'd be happy to step in here. Let me know how I can help and contribute.

I don't know Sebastian's opinion on that idea but I think usually a new
maintainer and reviewer of particular drivers is welcomed by community.
I see also Intel's interest and his contributions in these chargers.
Laurentiu seems to do a thorough review as well. There can be both
official reviewers.

It's up to you people. Just make a first step and we'll see how other
people react (and what Sebastian thinks about it :) ).

Best regards,
Krzysztof

Re: [PATCH v2 13/13] dt: power: bq2425x-charger: Cover additional devices

[Andreas Dannenberg]

On Fri, Sep 11, 2015 at 09:34:10AM +0900, Krzysztof Kozlowski wrote:
> On 11.09.2015 05:57, Andreas Dannenberg wrote:
> > On Thu, Sep 10, 2015 at 09:15:09AM +0900, Krzysztof Kozlowski wrote:
> 
> (...)
> 
> > 
> >>>> bq2415x.txt - ti,current-limit  - maximum current to be drawn in mA
> >>>>
> >>>> New bindings:
> >>>> ti,charge-current (bq24261) - default charge current in mA
> >>>> ti,max-charge-current (bq24261) - maximum charging current in mA
> >>>
> >>> This ti,max-charge-current is actually an interesting one. It's not a
> >>> device setting as it does not impact any of the device registers at all.
> >>> Instead, it's an artificial limit that can be set through DT that
> >>> prevents somebody from going into sysfs and configuring a charge current
> >>> higher than ti,max-charge-current. In other drivers I have seen that the
> >>> sysfs property reflecting that max charge current is just read-only and
> >>> gives you the maximum the HW is capable of. From a device point of view
> >>> there is nothing configurable about this property.
> >>
> >> Hmmm, so now I wonder whether this should be a DT binding. The purpose
> >> of DT isn't the control of the driver like enable some stuff, set some
> >> value used by the driver. The DT provides information about hardware so
> >> the driver could properly configure the device and work with it.
> >>
> >> Reason to put this into DT would be:
> >> For example one device configuration (device, board, connected battery)
> >> could handle one maximum value and the other configuration would require
> >> lower one. The device and its capabilities (bq24257 for example) are the
> >> same but configuration changes.
> > 
> > Yes this could be useful in this case but this property only makes sense
> > if userspace is allowed to change the actual charge current through
> > sysfs as some drivers do including Ram's proposed bq24261 driver. But if we
> > get rid of the general sysfs configurability of the charge current
> > (which I say we should) we don't need that ti,max-charge-current property.
> > Also see...
> > 
> > http://marc.info/?l=linux-pm&m=143080413218161&w=2
> 
> Right, that's my previous reply. :) I must admit that here and for
> bq24261 I looked mostly at bindings so I did not initially about the
> sysfs interface.
> 
> Anyway I am not convinced to having such sysfs interfaces and definitely
> they should not mess with DT. I would rather expect these to be totally
> orthogonal.
> 
> Summarizing all these bq24261 properties:
>  - ti,enable-user-write
>  - ti,max-charge-current
>  - ti,max-charge-voltage
> are related to that sysfs interface, not to hardware configuration. If
> that's true, then all should be gone.
> 
> > 
> >> To prevent future issues in naming and bindings consistency maybe there
> >> should be a dedicated maintainer for bq/TI charger devices? Pali Rohar
> >> recently took care for devices related to Nokia N900 but maybe someone
> >> from TI should also take care of rest or all of them (if TI is
> >> interested in this)?
> > 
> > I'd be happy to step in here. Let me know how I can help and contribute.
> 
> I don't know Sebastian's opinion on that idea but I think usually a new
> maintainer and reviewer of particular drivers is welcomed by community.
> I see also Intel's interest and his contributions in these chargers.
> Laurentiu seems to do a thorough review as well. There can be both
> official reviewers.

Hi Krzysztof,
If we already have good coverage then that's great. The value I can add
is that I literally sit in the same building with the "battery charger
people" :) so it's easy to go talk to them to clarify technical details
amongst other things that might come up as drivers are getting
developed/reviewed. So at a minimum folks like Laurentiu should feel
free to reach out to me as needed.

Regards,

--
Andreas Dannenberg
Texas Instruments Inc

 
> It's up to you people. Just make a first step and we'll see how other
> people react (and what Sebastian thinks about it :) ).
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v2 00/13] power: bq24257: Add support for bq24250/bq24251

[Laurentiu Palcu]

On Tue, Sep 08, 2015 at 07:12:24PM -0500, Andreas Dannenberg wrote:
> This patch series extends the driver to also support bq24250/bq24251.
> 
> The bq24250/251/257 devices have a very similar feature set and are
> virtually identical from a control register point of view so it made
> sense to extend the existing driver rather than submitting a new driver.
> In addition to the new device support the driver is also extended to
> allow access to some device features previously hidden. Basic and
> potentially dangerous charger config parameters affecting the actual
> charging of the Li-Ion battery are only configurable through firmware
> rather than sysfs properties. However some newly introduced properties
> are exposed through sysfs properties as access to them may be desired
> from userspace. For example, it is now possible to manually configure
> the maximum current drawn from the input source to accommodate different
> chargers (0.5A, 1.5A, 2.0A and so on) based on system knowledge a
> userspace application may have rather than rely on the auto-detection
> mechanism that may not work in all possible scenarios.
> 
> Note that most patches have dependencies on other patches in the series.
> 
> 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.
I can't say I fully agree with this rename but, on the other hand, since you
work for the chip manufacturer, you probably know better what other chips
(if any), with the same naming scheme, are due to be released and make
sure they are registry compatible. Otherwise, it'll be fun.

For the entire patchset, please run it through 'checkpatch --strict'.
There are some minor style issues that are worth fixing.

Regarding the DT patch (on which I'll comment separately, after I go
through the entire patchset), make sure you put it before the code
implementing the binding [1].

[1] Documentation/devicetree/bindings/submitting-patches.txt

laurentiu

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

Re: [PATCH v2 00/13] power: bq24257: Add support for bq24250/bq24251

[Andreas Dannenberg]

On Thu, Sep 10, 2015 at 03:26:16PM +0300, Laurentiu Palcu wrote:
> On Tue, Sep 08, 2015 at 07:12:24PM -0500, Andreas Dannenberg wrote:
> > This patch series extends the driver to also support bq24250/bq24251.
> > 
> > The bq24250/251/257 devices have a very similar feature set and are
> > virtually identical from a control register point of view so it made
> > sense to extend the existing driver rather than submitting a new driver.
> > In addition to the new device support the driver is also extended to
> > allow access to some device features previously hidden. Basic and
> > potentially dangerous charger config parameters affecting the actual
> > charging of the Li-Ion battery are only configurable through firmware
> > rather than sysfs properties. However some newly introduced properties
> > are exposed through sysfs properties as access to them may be desired
> > from userspace. For example, it is now possible to manually configure
> > the maximum current drawn from the input source to accommodate different
> > chargers (0.5A, 1.5A, 2.0A and so on) based on system knowledge a
> > userspace application may have rather than rely on the auto-detection
> > mechanism that may not work in all possible scenarios.
> > 
> > Note that most patches have dependencies on other patches in the series.
> > 
> > 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.
> I can't say I fully agree with this rename but, on the other hand, since you
> work for the chip manufacturer, you probably know better what other chips
> (if any), with the same naming scheme, are due to be released and make
> sure they are registry compatible. Otherwise, it'll be fun.

Yes the expectation is that potential future devices will fit with minor
changes to the driver. I was given no indication that this wouldn't be
the case but I'm double-checking with the product/planning people for
that family.

> For the entire patchset, please run it through 'checkpatch --strict'.
> There are some minor style issues that are worth fixing.

Ok will do. But I'm not a big fan of --strict, especially the alignment
of function arguments that spill into the next line. If it is required
by the Kernel community (is it?) why not making --strict a default-on
option of checkpatch.pl or mandating it in one of the docs?
 
> Regarding the DT patch (on which I'll comment separately, after I go
> through the entire patchset), make sure you put it before the code
> implementing the binding [1].
> 
> [1] Documentation/devicetree/bindings/submitting-patches.txt

I had actually read this but my DT doc binding patch also renames the
binding file itself. So technically it isn't valid until after the
rename patch was applied... which is against the patch ordering... Any
creative idea on how to solve this?

Thanks,

--
Andreas Dannenberg
Texas Instruments Inc

Re: [PATCH v2 00/13] power: bq24257: Add support for bq24250/bq24251

[Laurentiu Palcu]

On Thu, Sep 10, 2015 at 04:26:26PM -0500, Andreas Dannenberg wrote:
> On Thu, Sep 10, 2015 at 03:26:16PM +0300, Laurentiu Palcu wrote:
> > On Tue, Sep 08, 2015 at 07:12:24PM -0500, Andreas Dannenberg wrote:
[...] 
> > For the entire patchset, please run it through 'checkpatch --strict'.
> > There are some minor style issues that are worth fixing.
> 
> Ok will do. But I'm not a big fan of --strict, especially the alignment
> of function arguments that spill into the next line. If it is required
> by the Kernel community (is it?) why not making --strict a default-on
> option of checkpatch.pl or mandating it in one of the docs?
I don't think this is required by the kernel community. However, I
believe that fixing those style issues will make the code tidier. The
reason this is not the default option for checkpatch is because you
cannot always align those function arguments without exceeding the 80
chars line limit, etc. Which is reasonable. So, wherever we can do it,
let's, at least, try. :)

>  
> > Regarding the DT patch (on which I'll comment separately, after I go
> > through the entire patchset), make sure you put it before the code
> > implementing the binding [1].
> > 
> > [1] Documentation/devicetree/bindings/submitting-patches.txt
> 
> I had actually read this but my DT doc binding patch also renames the
> binding file itself. So technically it isn't valid until after the
> rename patch was applied... which is against the patch ordering... Any
> creative idea on how to solve this?
One idea would be to start the patchset upfront with the renaming. This
will not introduce new bindings nor change any driver functionality, so
it should be ok. Then introduce the new bindings and continue with the
rest of implementation. I think this will be compliant with the DT doc
requirements and it'll probably silence checkpatch warnings like the one
below:

WARNING: DT compatible string "ti,bq24250" appears un-documented --
check ./Documentation/devicetree/bindings/

The only downside to this is that you'll have to rework the rest of the
patches to use the new prefixes... :/

laurentiu

Re: [PATCH v2 00/13] power: bq24257: Add support for bq24250/bq24251

[Andreas Dannenberg]

On Thu, Sep 10, 2015 at 04:26:26PM -0500, Andreas Dannenberg wrote:
> On Thu, Sep 10, 2015 at 03:26:16PM +0300, Laurentiu Palcu wrote:
> > On Tue, Sep 08, 2015 at 07:12:24PM -0500, Andreas Dannenberg wrote:
> > > v2:
[...]
> > > - 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.
> > I can't say I fully agree with this rename but, on the other hand, since you
> > work for the chip manufacturer, you probably know better what other chips
> > (if any), with the same naming scheme, are due to be released and make
> > sure they are registry compatible. Otherwise, it'll be fun.
> 
> Yes the expectation is that potential future devices will fit with minor
> changes to the driver. I was given no indication that this wouldn't be
> the case but I'm double-checking with the product/planning people for
> that family.

Hi Laurentiu, full stop on the renaming. After double-checking with the
responsible bq242xx engineers it turns out it will be difficult to
maintain a family concept for the bq2425x driver moving forward (unless
you like nested switch/case statements sprinkled throughout the driver).

I feel sometimes even the most well-thought out and "future-proof"
family naming schemes will eventually turn into nothing more than a set
of GUIDs :) From a silicon vendor perspective there are several reasons
for that including that the namespace for part numbers (including their
length) is actually quite limited - a fact that's not immediately
obvious from the outside.

This being said I'll drop the renaming patch, and let's rely on folks
using Kconfig to find and use the correct driver.

--
Andreas Dannenberg
Texas Instruments Inc