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

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

[Andreas Dannenberg]

This patch series extends the driver to also support bq24250/bq24251. Most
patches have dependencies on other patches in the series but I left them spread
out so that one can better understand the change history. However I can
certainly fold them together and resubmit if needed.

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. As per offline discussion with Laurentiu the basic
and potentially dangerous charger config parameters affecting the actual
charging of the Li-Ion battery are still 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.

Laurentiu-- I've spent quite some time testing the driver however the one piece
I could not test was the ACPI integration due to lack of suitable HW. For this
do you think you could give this a quick try?  I'm particularly interested if
the private driver data from the bq24257_acpi_match[] structure gets properly
passed down into bq24257_probe(). Also if you could recommend an embedded HW
platform that I could generally use to test ACPI support I'd appreciate that
(MinnowBoard MAX?).

--
Andreas Dannenberg
Texas Instruments Inc


Andreas Dannenberg (13):
  power: bq24257: Add bit definition for temp sense enable
  power: bq24257: Add dead battery reporting
  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 VINDPM voltage threshold setting support
  power: bq24257: Extend scope of mutex protection
  power: bq24257: Add charge type setting support
  power: bq24257: Add in_ilimit setting support
  power: bq24257: Add various device-specific sysfs properties
  power: bq24257: Add platform data based initialization
  dt: power: bq24257-charger: Cover additional devices

 .../devicetree/bindings/power/bq24257.txt          |  59 +-
 drivers/power/Kconfig                              |   5 +-
 drivers/power/bq24257_charger.c                    | 652 +++++++++++++++++++--
 include/linux/power/bq24257_charger.h              |  30 +
 4 files changed, 677 insertions(+), 69 deletions(-)
 create mode 100644 include/linux/power/bq24257_charger.h

-- 
1.9.1

[PATCH 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 01/13] power: bq24257: Add bit definition for temp sense enable

[Andrew F. Davis]

On 08/31/2015 09:10 PM, 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>

Acked-by: Andrew F. Davis <afd@ti.com>

-- 
Andrew F. Davis

[PATCH 02/13] power: bq24257: Add dead battery reporting

[Andreas Dannenberg]

A missing/disconnected battery is now reported as dead rather than an
unspecified failure via the charger's sysfs health property.

$ cat health
Dead

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

diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c
index db81356..0b34528 100644
--- a/drivers/power/bq24257_charger.c
+++ b/drivers/power/bq24257_charger.c
@@ -274,6 +274,10 @@ static int bq24257_power_supply_get_property(struct power_supply *psy,
 			val->intval = POWER_SUPPLY_HEALTH_SAFETY_TIMER_EXPIRE;
 			break;
 
+		case FAULT_NO_BAT:
+			val->intval = POWER_SUPPLY_HEALTH_DEAD;
+			break;
+
 		default:
 			val->intval = POWER_SUPPLY_HEALTH_UNSPEC_FAILURE;
 			break;
-- 
1.9.1

Re: [PATCH 02/13] power: bq24257: Add dead battery reporting

[Andrew F. Davis]

On 08/31/2015 09:10 PM, Andreas Dannenberg wrote:
> A missing/disconnected battery is now reported as dead rather than an
> unspecified failure via the charger's sysfs health property.
>
> $ cat health
> Dead

Poor cat :(

>
> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> ---
>   drivers/power/bq24257_charger.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c
> index db81356..0b34528 100644
> --- a/drivers/power/bq24257_charger.c
> +++ b/drivers/power/bq24257_charger.c
> @@ -274,6 +274,10 @@ static int bq24257_power_supply_get_property(struct power_supply *psy,
>   			val->intval = POWER_SUPPLY_HEALTH_SAFETY_TIMER_EXPIRE;
>   			break;
>
> +		case FAULT_NO_BAT:
> +			val->intval = POWER_SUPPLY_HEALTH_DEAD;
> +			break;
> +

I think the best thing to do would be to return -ENODEV as suggested by
power_supply_sysfs.c:305. Also you should probably add the POWER_SUPPLY_PROP_PRESENT
property check and set intval to 0 when there is no battery.

Regards,
Andrew

>   		default:
>   			val->intval = POWER_SUPPLY_HEALTH_UNSPEC_FAILURE;
>   			break;
>

Re: [PATCH 02/13] power: bq24257: Add dead battery reporting

[Andreas Dannenberg]

Andrew- thanks for your feeback. Answers below...

On Tue, Sep 01, 2015 at 02:33:11PM -0500, Andrew F. Davis wrote:
> On 08/31/2015 09:10 PM, Andreas Dannenberg wrote:
> >A missing/disconnected battery is now reported as dead rather than an
> >unspecified failure via the charger's sysfs health property.
> >
> >$ cat health
> >Dead
> 
> Poor cat :(
> 

I had to laugh pretty hard when I saw that getting printed onto the
console for the first time so I had to include it here.

> >
> >Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> >---
> >  drivers/power/bq24257_charger.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> >diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c
> >index db81356..0b34528 100644
> >--- a/drivers/power/bq24257_charger.c
> >+++ b/drivers/power/bq24257_charger.c
> >@@ -274,6 +274,10 @@ static int bq24257_power_supply_get_property(struct power_supply *psy,
> >  			val->intval = POWER_SUPPLY_HEALTH_SAFETY_TIMER_EXPIRE;
> >  			break;
> >
> >+		case FAULT_NO_BAT:
> >+			val->intval = POWER_SUPPLY_HEALTH_DEAD;
> >+			break;
> >+
> 
> I think the best thing to do would be to return -ENODEV as suggested by
> power_supply_sysfs.c:305. Also you should probably add the POWER_SUPPLY_PROP_PRESENT
> property check and set intval to 0 when there is no battery.

Good find with -ENODEV. However in this case here the power supply is
really a combination of a charger and a battery (which could be split
out accordingly but that's a different story). If I were to report
-ENODEV I would basically say the entire power supply is gone which is
not correct IMHO. Even with a missing battery a system is typically
functional as it gets powered through USB and the charger IC is still
there and functioning. So I think I would either leave the reporting
as *_DEAD or skip the patch. I'm curious if there are additional
opinions.

--
Andreas Dannenberg
Texas Instruments Inc

> 
> Regards,
> Andrew
> 
> >  		default:
> >  			val->intval = POWER_SUPPLY_HEALTH_UNSPEC_FAILURE;
> >  			break;
> >

Re: [PATCH 02/13] power: bq24257: Add dead battery reporting

[Andrew F. Davis]

On 09/01/2015 04:04 PM, Andreas Dannenberg wrote:
> Andrew- thanks for your feeback. Answers below...
>
> On Tue, Sep 01, 2015 at 02:33:11PM -0500, Andrew F. Davis wrote:
>> On 08/31/2015 09:10 PM, Andreas Dannenberg wrote:
>>> A missing/disconnected battery is now reported as dead rather than an
>>> unspecified failure via the charger's sysfs health property.
>>>
>>> $ cat health
>>> Dead
>>
>> Poor cat :(
>>
>
> I had to laugh pretty hard when I saw that getting printed onto the
> console for the first time so I had to include it here.
>
>>>
>>> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
>>> ---
>>>   drivers/power/bq24257_charger.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c
>>> index db81356..0b34528 100644
>>> --- a/drivers/power/bq24257_charger.c
>>> +++ b/drivers/power/bq24257_charger.c
>>> @@ -274,6 +274,10 @@ static int bq24257_power_supply_get_property(struct power_supply *psy,
>>>   			val->intval = POWER_SUPPLY_HEALTH_SAFETY_TIMER_EXPIRE;
>>>   			break;
>>>
>>> +		case FAULT_NO_BAT:
>>> +			val->intval = POWER_SUPPLY_HEALTH_DEAD;
>>> +			break;
>>> +
>>
>> I think the best thing to do would be to return -ENODEV as suggested by
>> power_supply_sysfs.c:305. Also you should probably add the POWER_SUPPLY_PROP_PRESENT
>> property check and set intval to 0 when there is no battery.
>
> Good find with -ENODEV. However in this case here the power supply is
> really a combination of a charger and a battery (which could be split
> out accordingly but that's a different story). If I were to report
> -ENODEV I would basically say the entire power supply is gone which is
> not correct IMHO. Even with a missing battery a system is typically
> functional as it gets powered through USB and the charger IC is still
> there and functioning. So I think I would either leave the reporting
> as *_DEAD or skip the patch. I'm curious if there are additional
> opinions.
>

It looks like returning -ENODEV for one property would not mark the whole
device gone, just POWER_SUPPLY_PROP_HEALTH, but I guess that depends on
what user-space does with the info. I would think that this is what
POWER_SUPPLY_PROP_PRESENT is for but different drivers seem to use it for
different things. Anyway, reporting DEAD for a missing battery is probably
not the most correct option, maybe drop the patch ( for the cat's sake :) ).

-- 
Andrew F. Davis

> --
> Andreas Dannenberg
> Texas Instruments Inc
>
>>
>> Regards,
>> Andrew
>>
>>>   		default:
>>>   			val->intval = POWER_SUPPLY_HEALTH_UNSPEC_FAILURE;
>>>   			break;
>>>

Re: [PATCH 02/13] power: bq24257: Add dead battery reporting

[Laurentiu Palcu]

On Tue, Sep 01, 2015 at 04:16:26PM -0500, Andrew F. Davis wrote:
> 
> 
> On 09/01/2015 04:04 PM, Andreas Dannenberg wrote:
> >Andrew- thanks for your feeback. Answers below...
> >
> >On Tue, Sep 01, 2015 at 02:33:11PM -0500, Andrew F. Davis wrote:
> >>On 08/31/2015 09:10 PM, Andreas Dannenberg wrote:
> >>>A missing/disconnected battery is now reported as dead rather than an
> >>>unspecified failure via the charger's sysfs health property.
> >>>
> >>>$ cat health
> >>>Dead
> >>
> >>Poor cat :(
> >>
> >
> >I had to laugh pretty hard when I saw that getting printed onto the
> >console for the first time so I had to include it here.
> >
> >>>
> >>>Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> >>>---
> >>>  drivers/power/bq24257_charger.c | 4 ++++
> >>>  1 file changed, 4 insertions(+)
> >>>
> >>>diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c
> >>>index db81356..0b34528 100644
> >>>--- a/drivers/power/bq24257_charger.c
> >>>+++ b/drivers/power/bq24257_charger.c
> >>>@@ -274,6 +274,10 @@ static int bq24257_power_supply_get_property(struct power_supply *psy,
> >>>  			val->intval = POWER_SUPPLY_HEALTH_SAFETY_TIMER_EXPIRE;
> >>>  			break;
> >>>
> >>>+		case FAULT_NO_BAT:
> >>>+			val->intval = POWER_SUPPLY_HEALTH_DEAD;
> >>>+			break;
> >>>+
> >>
> >>I think the best thing to do would be to return -ENODEV as suggested by
> >>power_supply_sysfs.c:305. Also you should probably add the POWER_SUPPLY_PROP_PRESENT
> >>property check and set intval to 0 when there is no battery.
> >
> >Good find with -ENODEV. However in this case here the power supply is
> >really a combination of a charger and a battery (which could be split
> >out accordingly but that's a different story). If I were to report
> >-ENODEV I would basically say the entire power supply is gone which is
> >not correct IMHO. Even with a missing battery a system is typically
> >functional as it gets powered through USB and the charger IC is still
> >there and functioning. So I think I would either leave the reporting
> >as *_DEAD or skip the patch. I'm curious if there are additional
> >opinions.
> >
> 
> It looks like returning -ENODEV for one property would not mark the whole
> device gone, just POWER_SUPPLY_PROP_HEALTH, but I guess that depends on
> what user-space does with the info. I would think that this is what
> POWER_SUPPLY_PROP_PRESENT is for but different drivers seem to use it for
> different things. Anyway, reporting DEAD for a missing battery is probably
> not the most correct option, maybe drop the patch ( for the cat's sake :) ).
I'm also in favor of dropping this patch for the same reasons. Also,
since the latest phones/tablets do not allow battery removal, it's
highly unlikely we'll even hit this case nowadays. Perhaps, we could use
DEAD if the battery did not charge and the safety timer expired?
But, if one alters iilimit and sets a limit that's too low for the
battery to charge in time, that doesn't mean the battery is dead... :/

That's the main reason I'm so reluctant on having properties like charge
current, charge voltage, or even input current limit, writable in
sysfs.

laurentiu

Re: [PATCH 02/13] power: bq24257: Add dead battery reporting

[Andreas Dannenberg]

On Fri, Sep 04, 2015 at 04:28:05PM +0300, Laurentiu Palcu wrote:
> On Tue, Sep 01, 2015 at 04:16:26PM -0500, Andrew F. Davis wrote:
> > On 09/01/2015 04:04 PM, Andreas Dannenberg wrote:
> > >>>  drivers/power/bq24257_charger.c | 4 ++++
> > >>>  1 file changed, 4 insertions(+)
> > >>>
> > >>>diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c
> > >>>index db81356..0b34528 100644
> > >>>--- a/drivers/power/bq24257_charger.c
> > >>>+++ b/drivers/power/bq24257_charger.c
> > >>>@@ -274,6 +274,10 @@ static int bq24257_power_supply_get_property(struct power_supply *psy,
> > >>>  			val->intval = POWER_SUPPLY_HEALTH_SAFETY_TIMER_EXPIRE;
> > >>>  			break;
> > >>>
> > >>>+		case FAULT_NO_BAT:
> > >>>+			val->intval = POWER_SUPPLY_HEALTH_DEAD;
> > >>>+			break;
> > >>>+
> > >>
> > >>I think the best thing to do would be to return -ENODEV as suggested by
> > >>power_supply_sysfs.c:305. Also you should probably add the POWER_SUPPLY_PROP_PRESENT
> > >>property check and set intval to 0 when there is no battery.
> > >
> > >Good find with -ENODEV. However in this case here the power supply is
> > >really a combination of a charger and a battery (which could be split
> > >out accordingly but that's a different story). If I were to report
> > >-ENODEV I would basically say the entire power supply is gone which is
> > >not correct IMHO. Even with a missing battery a system is typically
> > >functional as it gets powered through USB and the charger IC is still
> > >there and functioning. So I think I would either leave the reporting
> > >as *_DEAD or skip the patch. I'm curious if there are additional
> > >opinions.
> > >
> > 
> > It looks like returning -ENODEV for one property would not mark the whole
> > device gone, just POWER_SUPPLY_PROP_HEALTH, but I guess that depends on
> > what user-space does with the info. I would think that this is what
> > POWER_SUPPLY_PROP_PRESENT is for but different drivers seem to use it for
> > different things. Anyway, reporting DEAD for a missing battery is probably
> > not the most correct option, maybe drop the patch ( for the cat's sake :) ).
> I'm also in favor of dropping this patch for the same reasons. Also,
> since the latest phones/tablets do not allow battery removal, it's
> highly unlikely we'll even hit this case nowadays. Perhaps, we could use

Hi Laurentiu, ok I'll drop it. Understood about the unlikeliness of a
battery removal for most devices using LiIon during runtime but a more
granular reporting would help in case there is an issue with the HW
itself (loose contact, manufacturing defect in the battery, etc.)

> DEAD if the battery did not charge and the safety timer expired?
> But, if one alters iilimit and sets a limit that's too low for the
> battery to charge in time, that doesn't mean the battery is dead... :/

Yeah unless there is a specific register bit or some very specific
guidance in a device datasheet as how to detect a truly dead battery I
would be a bit hesitant trying to interpret device register values to
derive to such a conclusion. Sometimes algorithms which seem simple are
only simple because one doesn't consider all the corner cases initially
:)

> That's the main reason I'm so reluctant on having properties like charge
> current, charge voltage, or even input current limit, writable in
> sysfs.

Agreed from a safety perspective. And if someone really wants to play
with fire (pun intended) they can always change the driver in their own
copy of the Kernel.

--
Andreas Dannenberg
Texas Instruments Inc

[PATCH 03/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-l0cyMroinI0@public.gmane.org>
---
 drivers/power/Kconfig           |  5 +++--
 drivers/power/bq24257_charger.c | 34 +++++++++++++++++++++++++++++++---
 2 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index 08beeed..0a2b033 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 BQ24250/251/257 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 0b34528..45232bd 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,12 @@
 
 #define BQ24257_ILIM_SET_DELAY		1000	/* msec */
 
+enum bq2425x_chip {
+	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 +81,9 @@ struct bq24257_device {
 	struct device *dev;
 	struct power_supply *charger;
 
+	enum bq2425x_chip chip;
+	char chip_name[I2C_NAME_SIZE];
+
 	struct regmap *rmap;
 	struct regmap_field *rmap_fields[F_MAX_FIELDS];
 
@@ -250,6 +263,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 = bq->chip_name;
+		break;
+
 	case POWER_SUPPLY_PROP_ONLINE:
 		val->intval = state.power_good;
 		break;
@@ -574,6 +591,7 @@ static int bq24257_hw_init(struct bq24257_device *bq)
 
 static enum power_supply_property bq24257_power_supply_props[] = {
 	POWER_SUPPLY_PROP_MANUFACTURER,
+	POWER_SUPPLY_PROP_MODEL_NAME,
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_ONLINE,
 	POWER_SUPPLY_PROP_HEALTH,
@@ -686,6 +704,8 @@ static int bq24257_probe(struct i2c_client *client,
 
 	bq->client = client;
 	bq->dev = dev;
+	bq->chip = (enum bq2425x_chip)id->driver_data;
+	strncpy(bq->chip_name, id->name, I2C_NAME_SIZE);
 
 	mutex_init(&bq->lock);
 
@@ -828,19 +848,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

--
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 03/13] power: bq24257: Add basic support for bq24250/bq24251

[Andrew F. Davis]

On 08/31/2015 09:10 PM, Andreas Dannenberg wrote:
> This patch adds basic support for bq24250 and bq24251 which are very
> similar to the bq24257 the driver was originally written for. Basic
> support means the ability to select a device through Kconfig, DT and
> ACPI, an instance variable allowing to check which chip is active, and
> the reporting back of the selected device through the
> POWER_SUPPLY_PROP_MODEL_NAME power supply sysfs property.
>
> This patch by itself is not sufficient to actually use those two added
> devices in a real-world setting due to some feature differences which
> are addressed by other patches in this series.
>
> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> ---
>   drivers/power/Kconfig           |  5 +++--
>   drivers/power/bq24257_charger.c | 34 +++++++++++++++++++++++++++++++---
>   2 files changed, 34 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
> index 08beeed..0a2b033 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 BQ24250/251/257 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.

I don't see this done very often, perhaps the additional devices make this
driver a good candidate for a rename? BQ2425X?

Regards,
Andrew

>
>   config CHARGER_BQ24735
>   	tristate "TI BQ24735 battery charger support"
> diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c
> index 0b34528..45232bd 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,12 @@
>
>   #define BQ24257_ILIM_SET_DELAY		1000	/* msec */
>
> +enum bq2425x_chip {
> +	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 +81,9 @@ struct bq24257_device {
>   	struct device *dev;
>   	struct power_supply *charger;
>
> +	enum bq2425x_chip chip;
> +	char chip_name[I2C_NAME_SIZE];
> +
>   	struct regmap *rmap;
>   	struct regmap_field *rmap_fields[F_MAX_FIELDS];
>
> @@ -250,6 +263,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 = bq->chip_name;
> +		break;
> +
>   	case POWER_SUPPLY_PROP_ONLINE:
>   		val->intval = state.power_good;
>   		break;
> @@ -574,6 +591,7 @@ static int bq24257_hw_init(struct bq24257_device *bq)
>
>   static enum power_supply_property bq24257_power_supply_props[] = {
>   	POWER_SUPPLY_PROP_MANUFACTURER,
> +	POWER_SUPPLY_PROP_MODEL_NAME,
>   	POWER_SUPPLY_PROP_STATUS,
>   	POWER_SUPPLY_PROP_ONLINE,
>   	POWER_SUPPLY_PROP_HEALTH,
> @@ -686,6 +704,8 @@ static int bq24257_probe(struct i2c_client *client,
>
>   	bq->client = client;
>   	bq->dev = dev;
> +	bq->chip = (enum bq2425x_chip)id->driver_data;
> +	strncpy(bq->chip_name, id->name, I2C_NAME_SIZE);
>
>   	mutex_init(&bq->lock);
>
> @@ -828,19 +848,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);
>

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

[Andreas Dannenberg]

On Tue, Sep 01, 2015 at 02:48:57PM -0500, Andrew F. Davis wrote:
> On 08/31/2015 09:10 PM, Andreas Dannenberg wrote:
> >This patch adds basic support for bq24250 and bq24251 which are very
> >similar to the bq24257 the driver was originally written for. Basic
> >support means the ability to select a device through Kconfig, DT and
> >ACPI, an instance variable allowing to check which chip is active, and
> >the reporting back of the selected device through the
> >POWER_SUPPLY_PROP_MODEL_NAME power supply sysfs property.
> >
> >This patch by itself is not sufficient to actually use those two added
> >devices in a real-world setting due to some feature differences which
> >are addressed by other patches in this series.
> >
> >Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> >---
> >  drivers/power/Kconfig           |  5 +++--
> >  drivers/power/bq24257_charger.c | 34 +++++++++++++++++++++++++++++++---
> >  2 files changed, 34 insertions(+), 5 deletions(-)
> >
> >diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
> >index 08beeed..0a2b033 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 BQ24250/251/257 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.
> 
> I don't see this done very often, perhaps the additional devices make this
> driver a good candidate for a rename? BQ2425X?

The patch series is made in a way to ensure 100% compatibility of the
patched driver vs. the original driver so I chose to not touch
definitions and filenames (in case you implied this) which seems to
follow the general trend. I've seen earlier discussion and somebody
explained that no matter how hard you try you will never be able to have
a naming convention that's completely future-proof.

Personally in my own code I tend to refactor filenames and such as I
refactor the actual code to keep things neat... but then, I know the
scope of where that code is used very well so I can avoid breakage.

Thanks,

--
Andreas Dannenberg
Texas Instruments Inc

> 
> Regards,
> Andrew
> 
> >
> >  config CHARGER_BQ24735
> >  	tristate "TI BQ24735 battery charger support"
> >diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c
> >index 0b34528..45232bd 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,12 @@
> >
> >  #define BQ24257_ILIM_SET_DELAY		1000	/* msec */
> >
> >+enum bq2425x_chip {
> >+	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 +81,9 @@ struct bq24257_device {
> >  	struct device *dev;
> >  	struct power_supply *charger;
> >
> >+	enum bq2425x_chip chip;
> >+	char chip_name[I2C_NAME_SIZE];
> >+
> >  	struct regmap *rmap;
> >  	struct regmap_field *rmap_fields[F_MAX_FIELDS];
> >
> >@@ -250,6 +263,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 = bq->chip_name;
> >+		break;
> >+
> >  	case POWER_SUPPLY_PROP_ONLINE:
> >  		val->intval = state.power_good;
> >  		break;
> >@@ -574,6 +591,7 @@ static int bq24257_hw_init(struct bq24257_device *bq)
> >
> >  static enum power_supply_property bq24257_power_supply_props[] = {
> >  	POWER_SUPPLY_PROP_MANUFACTURER,
> >+	POWER_SUPPLY_PROP_MODEL_NAME,
> >  	POWER_SUPPLY_PROP_STATUS,
> >  	POWER_SUPPLY_PROP_ONLINE,
> >  	POWER_SUPPLY_PROP_HEALTH,
> >@@ -686,6 +704,8 @@ static int bq24257_probe(struct i2c_client *client,
> >
> >  	bq->client = client;
> >  	bq->dev = dev;
> >+	bq->chip = (enum bq2425x_chip)id->driver_data;
> >+	strncpy(bq->chip_name, id->name, I2C_NAME_SIZE);
> >
> >  	mutex_init(&bq->lock);
> >
> >@@ -828,19 +848,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);
> >

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

[Laurentiu Palcu]

On Mon, Aug 31, 2015 at 09:10:25PM -0500, Andreas Dannenberg wrote:
[...]
> @@ -686,6 +704,8 @@ static int bq24257_probe(struct i2c_client *client,
>  
>  	bq->client = client;
>  	bq->dev = dev;
> +	bq->chip = (enum bq2425x_chip)id->driver_data;
> +	strncpy(bq->chip_name, id->name, I2C_NAME_SIZE);
id is NULL when ACPI enumerated... In order to check the device was
enumerated by ACPI, ACPI_HANDLE(dev) should be non-NULL. Then you can
use acpi_match_device() to fetch the acpi_id.

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 03/13] power: bq24257: Add basic support for bq24250/bq24251

[Andreas Dannenberg]

On Wed, Sep 02, 2015 at 11:19:13AM +0300, Laurentiu Palcu wrote:
> On Mon, Aug 31, 2015 at 09:10:25PM -0500, Andreas Dannenberg wrote:
> [...]
> > @@ -686,6 +704,8 @@ static int bq24257_probe(struct i2c_client *client,
> >  
> >  	bq->client = client;
> >  	bq->dev = dev;
> > +	bq->chip = (enum bq2425x_chip)id->driver_data;
> > +	strncpy(bq->chip_name, id->name, I2C_NAME_SIZE);
> id is NULL when ACPI enumerated... In order to check the device was
> enumerated by ACPI, ACPI_HANDLE(dev) should be non-NULL. Then you can
> use acpi_match_device() to fetch the acpi_id.

Thanks for the advice. I had seen similar code in bq2415x_charger.c.
Will take some glues from there and fix it.

Regards,

--
Andreas Dannenberg
Texas Instruments Inc


> 
> laurentiu

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

[Laurentiu Palcu]

On Mon, Aug 31, 2015 at 09:10:22PM -0500, Andreas Dannenberg wrote:
> This patch series extends the driver to also support bq24250/bq24251. Most
> patches have dependencies on other patches in the series but I left them spread
> out so that one can better understand the change history. However I can
> certainly fold them together and resubmit if needed.
> 
> 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. As per offline discussion with Laurentiu the basic
> and potentially dangerous charger config parameters affecting the actual
> charging of the Li-Ion battery are still 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.
> 
> Laurentiu-- I've spent quite some time testing the driver however the one piece
> I could not test was the ACPI integration due to lack of suitable HW. For this
> do you think you could give this a quick try?  I'm particularly interested if
> the private driver data from the bq24257_acpi_match[] structure gets properly
> passed down into bq24257_probe(). 
I'll comment on this on the patch itself.

> Also if you could recommend an embedded HW platform that I could
> generally use to test ACPI support I'd appreciate that (MinnowBoard
> MAX?).
I suppose you could use a Minnow Max quite well, if you wish. It has I2C
pins available on the Low Speed Expansion connector and you could easily
connect an EVB to it. You'll need to override the ACPI tables though, in
order to add your own device description.

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 00/13] power: bq24257: Add support for bq24250/bq24251

[Andreas Dannenberg]

On Wed, Sep 02, 2015 at 11:07:54AM +0300, Laurentiu Palcu wrote:
> On Mon, Aug 31, 2015 at 09:10:22PM -0500, Andreas Dannenberg wrote:
> > Also if you could recommend an embedded HW platform that I could
> > generally use to test ACPI support I'd appreciate that (MinnowBoard
> > MAX?).
> I suppose you could use a Minnow Max quite well, if you wish. It has I2C
> pins available on the Low Speed Expansion connector and you could easily
> connect an EVB to it. You'll need to override the ACPI tables though, in
> order to add your own device description.

Thanks, will get one. I don't like to modify/change code without being
able to really test all aspects of it (like in this case, ACPI) so this
will fill an important gap.

Regards,

--
Andreas Dannenberg
Texas Instruments Inc


> 
> laurentiu
>  

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

[Andreas Dannenberg]

A new device property called "ti,in-ilimit-autoset-disable" is
introduced to allow disabling the D+/D- based auto-detection algorithm
used to determine the input current limit. This feature is also
explicitly disabled on bq25250 devices which don't support D+/D-
charger type detection.

If "ti,in-ilimit-autoset-disable" is set the actual input current limit
can be configured through the "ti,in-limit-current" property.

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

diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c
index 45232bd..229fbce 100644
--- a/drivers/power/bq24257_charger.c
+++ b/drivers/power/bq24257_charger.c
@@ -68,6 +68,8 @@ struct bq24257_init_data {
 	u8 ichg;	/* charge current      */
 	u8 vbat;	/* regulation voltage  */
 	u8 iterm;	/* termination current */
+	u8 in_ilimit;	/* input current limit */
+	bool in_ilimit_autoset_disable;	/* auto-detect of input current limit */
 };
 
 struct bq24257_state {
@@ -95,6 +97,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)
@@ -183,6 +187,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)
 {
@@ -478,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");
 	}
 
@@ -580,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)
@@ -658,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;
@@ -681,6 +712,19 @@ 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 */
+	ret = device_property_read_u32(bq->dev, "ti,in-limit-current",
+				       &property);
+
+	if (ret < 0)
+		bq->init_data.in_ilimit = IILIMIT_500;
+	else
+		bq->init_data.in_ilimit = bq24257_find_idx(property,
+				bq24257_iilimit_map, BQ24257_IILIMIT_MAP_SIZE);
+
+	bq->init_data.in_ilimit_autoset_disable = device_property_read_bool(
+			bq->dev, "ti,in-ilimit-autoset-disable");
+
 	return 0;
 }
 
@@ -728,8 +772,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) {
@@ -740,6 +782,18 @@ static int bq24257_probe(struct i2c_client *client,
 		return -ENODEV;
 	}
 
+	/*
+	 * The BQ24250 doesn't support the D+/D- based detection for the
+	 * input current limit setting so explicitly disable that feature
+	 * otherwise respect the user configuration.
+	 */
+	bq->in_ilimit_autoset_disable = bq->chip == BQ24250 ?
+			true : bq->init_data.in_ilimit_autoset_disable;
+
+	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)
@@ -792,7 +846,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);
 
@@ -807,7 +862,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 04/13] power: bq24257: Allow manual setting of input current limit

[Andrew F. Davis]

On 08/31/2015 09:10 PM, Andreas Dannenberg wrote:
> A new device property called "ti,in-ilimit-autoset-disable" is
> introduced to allow disabling the D+/D- based auto-detection algorithm
> used to determine the input current limit. This feature is also
> explicitly disabled on bq25250 devices which don't support D+/D-
> charger type detection.
>
> If "ti,in-ilimit-autoset-disable" is set the actual input current limit
> can be configured through the "ti,in-limit-current" property.
>
> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>

Acked-by: Andrew F. Davis <afd@ti.com>

-- 
Andrew F. Davis

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

[Laurentiu Palcu]

On Mon, Aug 31, 2015 at 09:10:26PM -0500, Andreas Dannenberg wrote:
> A new device property called "ti,in-ilimit-autoset-disable" is
> introduced to allow disabling the D+/D- based auto-detection algorithm
> used to determine the input current limit. This feature is also
> explicitly disabled on bq25250 devices which don't support D+/D-
s/bq25250/bq24250/

The rest looks good.
laurentiu

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

[Andreas Dannenberg]

A new device property called "ti,pg-gpio-disable" is introduced to
allow disabling the GPIO-based Power Good implementation, and
replacing it with a SW-only alternative. This is helpful for devices
which don't have a dedicated PG pin such as bq25250.

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

diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c
index 229fbce..2d396c3 100644
--- a/drivers/power/bq24257_charger.c
+++ b/drivers/power/bq24257_charger.c
@@ -70,6 +70,7 @@ struct bq24257_init_data {
 	u8 iterm;	/* termination current */
 	u8 in_ilimit;	/* input current limit */
 	bool in_ilimit_autoset_disable;	/* auto-detect of input current limit */
+	bool pg_gpio_disable; /* use of a dedicated pin for Power Good */
 };
 
 struct bq24257_state {
@@ -99,6 +100,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 +358,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;
 }
@@ -725,6 +746,9 @@ static int bq24257_fw_probe(struct bq24257_device *bq)
 	bq->init_data.in_ilimit_autoset_disable = device_property_read_bool(
 			bq->dev, "ti,in-ilimit-autoset-disable");
 
+	bq->init_data.pg_gpio_disable = device_property_read_bool(
+			bq->dev, "ti,pg-gpio-disable");
+
 	return 0;
 }
 
@@ -794,10 +818,22 @@ 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. Note that the most
+	 * reliable way to determine the PG state is through the use of the
+	 * dedicated PG pin so on devices where it is available using that pin
+	 * should be the method of choice.
+	 */
+	bq->pg_gpio_disable = bq->chip == BQ24250 ?
+			true : bq->init_data.pg_gpio_disable;
+
+	if (!bq->pg_gpio_disable) {
+		ret = bq24257_pg_gpio_probe(bq);
+		if (ret < 0)
+			return ret;
+	}
 
 	/* reset all registers to defaults */
 	ret = bq24257_field_write(bq, F_RESET, 1);
-- 
1.9.1

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

[Andrew F. Davis]

On 08/31/2015 09:10 PM, Andreas Dannenberg wrote:
> A new device property called "ti,pg-gpio-disable" is introduced to
> allow disabling the GPIO-based Power Good implementation, and
> replacing it with a SW-only alternative. This is helpful for devices
> which don't have a dedicated PG pin such as bq25250.
>
> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>

Acked-by: Andrew F. Davis <afd@ti.com>

-- 
Andrew F. Davis

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

[Laurentiu Palcu]

On Mon, Aug 31, 2015 at 09:10:27PM -0500, Andreas Dannenberg wrote:
> A new device property called "ti,pg-gpio-disable" is introduced to
> allow disabling the GPIO-based Power Good implementation, and
> replacing it with a SW-only alternative. This is helpful for devices
> which don't have a dedicated PG pin such as bq25250.
s/bq25250/bq24250

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

[PATCH 06/13] power: bq24257: Add over voltage protection setting support

[Andreas Dannenberg]

A new optional device property called "ti,vovp-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 vovp-voltage
setting.

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

diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c
index 2d396c3..c6bb2b5 100644
--- a/drivers/power/bq24257_charger.c
+++ b/drivers/power/bq24257_charger.c
@@ -69,6 +69,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 */
 	bool in_ilimit_autoset_disable;	/* auto-detect of input current limit */
 	bool pg_gpio_disable; /* use of a dedicated pin for Power Good */
 };
@@ -195,6 +196,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)
 {
@@ -414,6 +422,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 */
@@ -595,7 +614,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}
 	};
 
 	/*
@@ -665,6 +685,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, };
@@ -733,16 +775,23 @@ 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 */
+	/* Optional properties. If not provided use reasonable default. */
 	ret = device_property_read_u32(bq->dev, "ti,in-limit-current",
 				       &property);
-
 	if (ret < 0)
 		bq->init_data.in_ilimit = IILIMIT_500;
 	else
 		bq->init_data.in_ilimit = bq24257_find_idx(property,
 				bq24257_iilimit_map, BQ24257_IILIMIT_MAP_SIZE);
 
+	ret = device_property_read_u32(bq->dev, "ti,vovp-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);
+
 	bq->init_data.in_ilimit_autoset_disable = device_property_read_bool(
 			bq->dev, "ti,in-ilimit-autoset-disable");
 
@@ -872,10 +921,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)
@@ -885,6 +942,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 06/13] power: bq24257: Add over voltage protection setting support

[Andrew F. Davis]

On 08/31/2015 09:10 PM, Andreas Dannenberg wrote:
> A new optional device property called "ti,vovp-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 vovp-voltage
> setting.
>
> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>

Acked-by: Andrew F. Davis <afd@ti.com>

-- 
Andrew F. Davis

[PATCH 07/13] power: bq24257: Add VINDPM voltage threshold setting support

[Andreas Dannenberg]

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

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

diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c
index c6bb2b5..2271a88 100644
--- a/drivers/power/bq24257_charger.c
+++ b/drivers/power/bq24257_charger.c
@@ -70,6 +70,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 */
 	bool in_ilimit_autoset_disable;	/* auto-detect of input current limit */
 	bool pg_gpio_disable; /* use of a dedicated pin for Power Good */
 };
@@ -203,6 +204,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)
 {
@@ -433,6 +441,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,7 +634,8 @@ static int bq24257_hw_init(struct bq24257_device *bq)
 		{F_ICHG, bq->init_data.ichg},
 		{F_VBAT, bq->init_data.vbat},
 		{F_ITERM, bq->init_data.iterm},
-		{F_VOVP, bq->init_data.vovp}
+		{F_VOVP, bq->init_data.vovp},
+		{F_VINDPM, bq->init_data.vindpm}
 	};
 
 	/*
@@ -649,6 +669,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)
@@ -696,10 +717,23 @@ static ssize_t bq24257_show_ovp_voltage(struct device *dev,
 			bq24257_vovp_map[bq->init_data.vovp]);
 }
 
+static ssize_t bq24257_show_vindpm_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(vindpm_voltage, S_IRUGO, bq24257_show_vindpm_voltage, NULL);
 
 static struct attribute *bq24257_charger_attr[] = {
 	&dev_attr_ovp_voltage.attr,
+	&dev_attr_vindpm_voltage.attr,
 	NULL,
 };
 
@@ -792,6 +826,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,vindpm-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.in_ilimit_autoset_disable = device_property_read_bool(
 			bq->dev, "ti,in-ilimit-autoset-disable");
 
-- 
1.9.1

Re: [PATCH 07/13] power: bq24257: Add VINDPM voltage threshold setting support

[Andrew F. Davis]

On 08/31/2015 09:10 PM, Andreas Dannenberg wrote:
> A new optional device property called "ti,vindpm-voltage" is introduced
> to allow configuring the input voltage threshold for the devices'
> dynamic power path management feature. In short, it can be used to
> prevent the input voltage from dropping below a certain value as current
> is drawn to charge the battery or supply the system.
>
> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> ---
>   drivers/power/bq24257_charger.c | 44 ++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c
> index c6bb2b5..2271a88 100644
> --- a/drivers/power/bq24257_charger.c
> +++ b/drivers/power/bq24257_charger.c
> @@ -70,6 +70,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 */
>   	bool in_ilimit_autoset_disable;	/* auto-detect of input current limit */
>   	bool pg_gpio_disable; /* use of a dedicated pin for Power Good */
>   };
> @@ -203,6 +204,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)
>   {
> @@ -433,6 +441,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,7 +634,8 @@ static int bq24257_hw_init(struct bq24257_device *bq)
>   		{F_ICHG, bq->init_data.ichg},
>   		{F_VBAT, bq->init_data.vbat},
>   		{F_ITERM, bq->init_data.iterm},
> -		{F_VOVP, bq->init_data.vovp}
> +		{F_VOVP, bq->init_data.vovp},
> +		{F_VINDPM, bq->init_data.vindpm}

Small comment, even though the last comma is not needed it helps so you
don't have a patch that removes and re-adds a line to add the comma
every time you rearrange or add to a list like above.

Regards,
Andrew

>   	};
>
>   	/*
> @@ -649,6 +669,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)
> @@ -696,10 +717,23 @@ static ssize_t bq24257_show_ovp_voltage(struct device *dev,
>   			bq24257_vovp_map[bq->init_data.vovp]);
>   }
>
> +static ssize_t bq24257_show_vindpm_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(vindpm_voltage, S_IRUGO, bq24257_show_vindpm_voltage, NULL);
>
>   static struct attribute *bq24257_charger_attr[] = {
>   	&dev_attr_ovp_voltage.attr,
> +	&dev_attr_vindpm_voltage.attr,
>   	NULL,
>   };
>
> @@ -792,6 +826,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,vindpm-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.in_ilimit_autoset_disable = device_property_read_bool(
>   			bq->dev, "ti,in-ilimit-autoset-disable");
>
>

[PATCH 08/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 2271a88..ad3fd2d 100644
--- a/drivers/power/bq24257_charger.c
+++ b/drivers/power/bq24257_charger.c
@@ -99,7 +99,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;
@@ -268,10 +268,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:
@@ -351,10 +351,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,
@@ -401,15 +403,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 {
@@ -532,7 +528,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,
@@ -543,9 +541,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
@@ -600,25 +596,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)
@@ -658,9 +659,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",
@@ -1002,11 +1001,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;
 }
 
@@ -1015,24 +1018,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 08/13] power: bq24257: Extend scope of mutex protection

[Andrew F. Davis]

On 08/31/2015 09:10 PM, 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.
>
> 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 2271a88..ad3fd2d 100644
> --- a/drivers/power/bq24257_charger.c
> +++ b/drivers/power/bq24257_charger.c
> @@ -99,7 +99,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 */
>

I think it would make more sense to make a different lock for the device access. Protecting
unrelated things with the same lock can lead to unnecessary blocks.

>   	bool in_ilimit_autoset_disable;
>   	bool pg_gpio_disable;
> @@ -268,10 +268,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);
>

Like here, this makes it very obvious what data the lock is protecting, wrapping this whole
block doesn't make much sense to me as all the subsequent reads are from the thread-local data.
I see in another patch in this series you do add a device read, but that read could be locked
by itself (with the device access lock) so we don't block this whole function when execution
may not even go down the path with the read.

Regards,
Andrew

>   	switch (psp) {
>   	case POWER_SUPPLY_PROP_STATUS:
> @@ -351,10 +351,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,
> @@ -401,15 +403,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 {
> @@ -532,7 +528,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,
> @@ -543,9 +541,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
> @@ -600,25 +596,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)
> @@ -658,9 +659,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",
> @@ -1002,11 +1001,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;
>   }
>
> @@ -1015,24 +1018,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
>
>

-- 
Andrew F. Davis

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

[Andreas Dannenberg]

On Tue, Sep 01, 2015 at 03:34:45PM -0500, Andrew F. Davis wrote:
> On 08/31/2015 09:10 PM, 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.
> >
> >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 2271a88..ad3fd2d 100644
> >--- a/drivers/power/bq24257_charger.c
> >+++ b/drivers/power/bq24257_charger.c
> >@@ -99,7 +99,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 */
> >
> 
> I think it would make more sense to make a different lock for the device access. Protecting
> unrelated things with the same lock can lead to unnecessary blocks.

Andrew- thanks for spending time analyzing the patch series.

Actually the extension of the scope of the mutex seemed to fit almost
naturally into the existing code as the device status (register
contents) I'm opening more access to is just as important "state" as the
driver-maintained "struct bq24257_state state" data that I need to
maintain together. I can either do that by extending the existing mutex
scope, or by creating a new mutex like you suggested which I think would
essentially need to be a superset of the existing mutex for the purposes
of the patched driver.

Also it was rather easy to extend because almost all of the use of the
mutex were in functions directly called from the IRQ routine (and only
from there) such as bq24257_state_changed() and
bq24257_handle_state_change() so I just had to wrap them at that level,
also protecting the device access functions contained there, effectively
killing two birds with one stone.

The bq24257_hw_init() on the other hand only needed mutex protection
when called from bq24257_resume() and I moved that up to the caller.

> 
> >  	bool in_ilimit_autoset_disable;
> >  	bool pg_gpio_disable;
> >@@ -268,10 +268,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);
> >
> 
> Like here, this makes it very obvious what data the lock is protecting, wrapping this whole
> block doesn't make much sense to me as all the subsequent reads are from the thread-local data.
> I see in another patch in this series you do add a device read, but that read could be locked
> by itself (with the device access lock) so we don't block this whole function when execution
> may not even go down the path with the read.

Yes you are right it's not needed here but rather was added in
preparation for the later inclusion of functions such as for supporting
POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT do direct device access so that's
just how I split up my overall edits into patches. Also I figured
having the entire statement wrapped doesn't really hurt and enables
easier additional extensions involving device access. In fact, it was
also done out of consistency with all the other sysfs getter/setter
routines in the final combined patched driver..

Thanks,

--
Andreas Dannenberg
Texas Instruments Inc

> 
> Regards,
> Andrew
> 
> >  	switch (psp) {
> >  	case POWER_SUPPLY_PROP_STATUS:
> >@@ -351,10 +351,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,
> >@@ -401,15 +403,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 {
> >@@ -532,7 +528,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,
> >@@ -543,9 +541,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
> >@@ -600,25 +596,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)
> >@@ -658,9 +659,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",
> >@@ -1002,11 +1001,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;
> >  }
> >
> >@@ -1015,24 +1018,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
> >
> >
> 
> -- 
> Andrew F. Davis

[PATCH 09/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 ad3fd2d..7b66030 100644
--- a/drivers/power/bq24257_charger.c
+++ b/drivers/power/bq24257_charger.c
@@ -262,6 +262,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)
@@ -287,6 +337,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;
@@ -359,6 +413,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)
 {
@@ -684,6 +771,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,
@@ -703,6 +791,8 @@ static const struct power_supply_desc bq24257_power_supply_desc = {
 	.properties = bq24257_power_supply_props,
 	.num_properties = ARRAY_SIZE(bq24257_power_supply_props),
 	.get_property = bq24257_power_supply_get_property,
+	.set_property = bq24257_power_supply_set_property,
+	.property_is_writeable	= bq24257_power_supply_property_is_writeable
 };
 
 static ssize_t bq24257_show_ovp_voltage(struct device *dev,
-- 
1.9.1

[PATCH 10/13] power: bq24257: Add in_ilimit setting support

[Andreas Dannenberg]

This patch allows reading (and writing, if the the automatic setting
of in_ilimit is disabled) the input current limit through the
POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT sysfs property. This property is
being exposed as writable in order to allow userspace to re-configure
the charge current 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 7b66030..42cc6fe 100644
--- a/drivers/power/bq24257_charger.c
+++ b/drivers/power/bq24257_charger.c
@@ -312,6 +312,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)
@@ -404,6 +439,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;
 	}
@@ -426,6 +465,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;
 	}
@@ -440,6 +482,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;
@@ -779,6 +822,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

[PATCH 11/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,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>
---
 drivers/power/bq24257_charger.c | 75 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 73 insertions(+), 2 deletions(-)

diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c
index 42cc6fe..d95ca46 100644
--- a/drivers/power/bq24257_charger.c
+++ b/drivers/power/bq24257_charger.c
@@ -72,7 +72,8 @@ struct bq24257_init_data {
 	u8 vovp;	/* over voltage protection voltage */
 	u8 vindpm;	/* VDMP input threshold voltage */
 	bool in_ilimit_autoset_disable;	/* auto-detect of input current limit */
-	bool pg_gpio_disable; /* use of a dedicated pin for Power Good */
+	bool pg_gpio_disable;	     /* use of a dedicated pin for Power Good */
+	bool timer2x_enable;		       /* slow down safety time by 2x */
 };
 
 struct bq24257_state {
@@ -766,7 +767,8 @@ 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}
+		{F_VINDPM, bq->init_data.vindpm},
+		{F_X2_TMR_EN, bq->init_data.timer2x_enable}
 	};
 
 	/*
@@ -861,12 +863,78 @@ static ssize_t bq24257_show_vindpm_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(vindpm_voltage, S_IRUGO, bq24257_show_vindpm_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_vindpm_voltage.attr,
+	&dev_attr_high_impedance_enable.attr,
+	&dev_attr_sysoff_enable.attr,
+	&dev_attr_timer2x_enable.attr,
 	NULL,
 };
 
@@ -973,6 +1041,9 @@ static int bq24257_fw_probe(struct bq24257_device *bq)
 	bq->init_data.pg_gpio_disable = device_property_read_bool(
 			bq->dev, "ti,pg-gpio-disable");
 
+	bq->init_data.timer2x_enable = device_property_read_bool(
+			bq->dev, "ti,timer-2x-enable");
+
 	return 0;
 }
 
-- 
1.9.1

[PATCH 12/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@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 d95ca46..72b33d2 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
@@ -959,28 +962,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_err(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.in_ilimit_autoset_disable =
+			pdata->in_ilimit_autoset_disable;
+
+	bq->init_data.pg_gpio_disable = pdata->pg_gpio_disable;
+	bq->init_data.timer2x_enable = pdata->timer2x_enable;
+}
+
 static int bq24257_fw_probe(struct bq24257_device *bq)
 {
 	int ret;
@@ -1052,6 +1129,7 @@ static int bq24257_probe(struct i2c_client *client,
 {
 	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
 	struct device *dev = &client->dev;
+	struct bq24257_platform_data *pdata = client->dev.platform_data;
 	struct bq24257_device *bq;
 	int ret;
 	int i;
@@ -1091,14 +1169,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);
 	}
 
 	/*
@@ -1150,7 +1229,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

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

[Andreas Dannenberg]

Extend the bq24257 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>
---
 .../devicetree/bindings/power/bq24257.txt          | 59 ++++++++++++++++++++--
 1 file changed, 55 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/power/bq24257.txt b/Documentation/devicetree/bindings/power/bq24257.txt
index 5c9d394..36b115c 100644
--- a/Documentation/devicetree/bindings/power/bq24257.txt
+++ b/Documentation/devicetree/bindings/power/bq24257.txt
@@ -2,20 +2,71 @@ Binding for TI bq24257 Li-Ion Charger
 
 Required properties:
 - compatible: Should contain one of the following:
+ * "ti,bq24250"
+ * "ti,bq24251"
  * "ti,bq24257"
-- reg:			   integer, i2c address of the device.
+- reg: integer, i2c address of the device.
+- stat-gpio: 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.
+- pg-gpio: GPIO used for the device PG (Power Good) pin. This pin is not
+    available on all devices and therefore only required on applicable devices.
+    However it is also possible to explicitly disable the use of this pin
+    through the optional property "ti,pg-gpio-disable" (see below) if desired.
+    In this case the "pg-gpio" property is no longer required.
 - ti,battery-regulation-voltage: integer, maximum charging voltage in uV.
-- ti,charge-current:	   integer, maximum charging current in uA.
-- ti,termination-current:  integer, charge will be terminated when current in
-			   constant-voltage phase drops below this value (in uA).
+- ti,charge-current: integer, maximum charging current in uA.
+- ti,termination-current: integer, charge will be terminated when current in
+    constant-voltage phase drops below this value (in uA).
+
+Optional properties:
+- ti,in-ilimit-autoset-disable: If this boolean property is present it disables
+    the automatic setting of the input current limit. This property is also set
+    implicitly on devices without charger type detection. If this property is
+    provided the input current limit should be set manually through
+    "ti,in-limit-current".
+- ti,in-limit-current: The maximum current to be drawn from the charger's input
+    (in uA). Use this for devices that don't have charger type detection or if
+    you have have set "ti,in-ilimit-autoset-disable".
+- ti,vovp-voltage: Configures the over voltage protection voltage (in uV).
+- ti,vindpm-voltage:  Configures the threshold input voltage for the dynamic
+    power path management (in uV).
+- ti,pg-gpio-disable: If this boolean property is provided a software-based
+    approach for power good determination is used. Note that the PG-pin based
+    approach is generally preferable.
+- ti,timer-2x-enable: If this boolean property is provided the device's safety
+    timer is extended by a factor of two.
+- interrupt-parent: Should be the phandle for the interrupt controller. Use in
+    conjunction with "interrupts" and only in case "stat-gpio" is not used.
+- interrupts: Interrupt mapping for GPIO IRQ (configure for both edges). Use in
+    conjunction with "interrupt-parent" and only in case "stat-gpio" is not
+    used.
 
 Example:
 
 bq24257 {
 	compatible = "ti,bq24257";
 	reg = <0x6a>;
+	stat-gpio = <&gpio1 16 GPIO_ACTIVE_HIGH>;
+	pg-gpio = <&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,in-limit-current = <900000>;
+	ti,vovp-voltage = <9500000>;
+	ti,vindpm-voltage = <4440000>;
+};
-- 
1.9.1

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

[Krzysztof Kozlowski]

2015-09-01 11:10 GMT+09:00 Andreas Dannenberg <dannenberg-l0cyMroinI0@public.gmane.org>:
> Extend the bq24257 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          | 59 ++++++++++++++++++++--
>  1 file changed, 55 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/power/bq24257.txt b/Documentation/devicetree/bindings/power/bq24257.txt
> index 5c9d394..36b115c 100644
> --- a/Documentation/devicetree/bindings/power/bq24257.txt
> +++ b/Documentation/devicetree/bindings/power/bq24257.txt
> @@ -2,20 +2,71 @@ Binding for TI bq24257 Li-Ion Charger
>
>  Required properties:
>  - compatible: Should contain one of the following:
> + * "ti,bq24250"
> + * "ti,bq24251"
>   * "ti,bq24257"
> -- reg:                    integer, i2c address of the device.
> +- reg: integer, i2c address of the device.
> +- stat-gpio: 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.

Preferred suffix should be "gpios":
Documentation/devicetree/bindings/gpio/gpio.txt

> +- pg-gpio: GPIO used for the device PG (Power Good) pin. This pin is not
> +    available on all devices and therefore only required on applicable devices.
> +    However it is also possible to explicitly disable the use of this pin
> +    through the optional property "ti,pg-gpio-disable" (see below) if desired.
> +    In this case the "pg-gpio" property is no longer required.
>  - ti,battery-regulation-voltage: integer, maximum charging voltage in uV.
> -- ti,charge-current:      integer, maximum charging current in uA.
> -- ti,termination-current:  integer, charge will be terminated when current in
> -                          constant-voltage phase drops below this value (in uA).
> +- ti,charge-current: integer, maximum charging current in uA.
> +- ti,termination-current: integer, charge will be terminated when current in
> +    constant-voltage phase drops below this value (in uA).
> +
> +Optional properties:
> +- ti,in-ilimit-autoset-disable: If this boolean property is present it disables

I thought "ilimit" is a typo but apparently not :) . The names of this
property and "ti,in-limit-current" below are quite obfuscated. I am
thinking about different names but first I would like to understand
the feature: what do you mean by "automatic setting of the input
current limit"? Like adjusting the limit of possible current to given
charger type?

> +    the automatic setting of the input current limit. This property is also set
> +    implicitly on devices without charger type detection. If this property is
> +    provided the input current limit should be set manually through
> +    "ti,in-limit-current".
> +- ti,in-limit-current: The maximum current to be drawn from the charger's input
> +    (in uA). Use this for devices that don't have charger type detection or if
> +    you have have set "ti,in-ilimit-autoset-disable".
> +- ti,vovp-voltage: Configures the over voltage protection voltage (in uV).
> +- ti,vindpm-voltage:  Configures the threshold input voltage for the dynamic
> +    power path management (in uV).

I think the leading 'v' could be removed, so "ti,ovp-voltage" and
"ti,in-dpm-voltage"?

> +- ti,pg-gpio-disable: If this boolean property is provided a software-based
> +    approach for power good determination is used. Note that the PG-pin based
> +    approach is generally preferable.

That's confusing. If someone does not want to use pg-gpio then he
could just skip the "pg-gpio" property?

> +- ti,timer-2x-enable: If this boolean property is provided the device's safety
> +    timer is extended by a factor of two.

Boolean properties like this have disadvantage - they cannot be
overridden. Also extending them is difficult (e.g. for some next
device the timer could have value of 4). Usually it is better to have
a value-based property with "0" meaning default or disabled.

Also please look at existing bindings:
$ git grep "ti," Documentation/devicetree/bindings/power
Maybe you could reuse some? Each of these existing and new bindings
are made generic (not device specific) so they should be reused.

Best regards,
Krzysztof

> +- interrupt-parent: Should be the phandle for the interrupt controller. Use in
> +    conjunction with "interrupts" and only in case "stat-gpio" is not used.
> +- interrupts: Interrupt mapping for GPIO IRQ (configure for both edges). Use in
> +    conjunction with "interrupt-parent" and only in case "stat-gpio" is not
> +    used.
>
>  Example:
>
>  bq24257 {
>         compatible = "ti,bq24257";
>         reg = <0x6a>;
> +       stat-gpio = <&gpio1 16 GPIO_ACTIVE_HIGH>;
> +       pg-gpio = <&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,in-limit-current = <900000>;
> +       ti,vovp-voltage = <9500000>;
> +       ti,vindpm-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 13/13] dt: power: bq24257-charger: Cover additional devices

[Andreas Dannenberg]

Hi Krzysztof, thanks for your feedback... comments below.

On Wed, Sep 02, 2015 at 02:24:05PM +0900, Krzysztof Kozlowski wrote:
> 2015-09-01 11:10 GMT+09:00 Andreas Dannenberg <dannenberg@ti.com>:
> > --- a/Documentation/devicetree/bindings/power/bq24257.txt
> > +++ b/Documentation/devicetree/bindings/power/bq24257.txt
> > @@ -2,20 +2,71 @@ Binding for TI bq24257 Li-Ion Charger
> >
> >  Required properties:
> >  - compatible: Should contain one of the following:
> > + * "ti,bq24250"
> > + * "ti,bq24251"
> >   * "ti,bq24257"
> > -- reg:                    integer, i2c address of the device.
> > +- reg: integer, i2c address of the device.
> > +- stat-gpio: 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.
> 
> Preferred suffix should be "gpios":
> Documentation/devicetree/bindings/gpio/gpio.txt

Did not know this. Thanks for pointing to the src document. Will fix.

> > +Optional properties:
> > +- ti,in-ilimit-autoset-disable: If this boolean property is present it disables
> 
> I thought "ilimit" is a typo but apparently not :) . The names of this
> property and "ti,in-limit-current" below are quite obfuscated. I am
> thinking about different names but first I would like to understand
> the feature: what do you mean by "automatic setting of the input
> current limit"? Like adjusting the limit of possible current to given
> charger type?

Yes that's right. Some of the chargers can check the USB D+/D- lines to
determine what kind of charger is attached (DCP, CDP, SDP, other) - see
bq24257 device datasheet for more info:
http://www.ti.com/product/bq24257

And this feature is being used in the original driver Laurentiu
developed. However this detection mechanism (or any detection mechanism
in general) may not always be appropriate hence the new property to
disable it and manually provide an input current limit of how much can
be drawn from the charger. Plus it's needed for devices that don't have
the D+/D- detection feature.

> 
> > +    the automatic setting of the input current limit. This property is also set
> > +    implicitly on devices without charger type detection. If this property is
> > +    provided the input current limit should be set manually through
> > +    "ti,in-limit-current".
> > +- ti,in-limit-current: The maximum current to be drawn from the charger's input
> > +    (in uA). Use this for devices that don't have charger type detection or if
> > +    you have have set "ti,in-ilimit-autoset-disable".
> > +- ti,vovp-voltage: Configures the over voltage protection voltage (in uV).
> > +- ti,vindpm-voltage:  Configures the threshold input voltage for the dynamic
> > +    power path management (in uV).
> 
> I think the leading 'v' could be removed, so "ti,ovp-voltage" and
> "ti,in-dpm-voltage"?

Yes I was thinking about this but then went with something that closely
reflects the device datasheets to maximize correlation. Removing the 'v'
would make it look cleaner and more universal and I suppose it's not to
far of a stretch for someone to associate "ti,ovp-voltag" with the VOVP
bit field in the devices register map. So let's change it.

> 
> > +- ti,pg-gpio-disable: If this boolean property is provided a software-based
> > +    approach for power good determination is used. Note that the PG-pin based
> > +    approach is generally preferable.
> 
> That's confusing. If someone does not want to use pg-gpio then he
> could just skip the "pg-gpio" property?

Yes that's right. Somebody might chose to do so because they don't want
to sacrifice the extra pin. And I need to accommodate for devices that
don't have a PG pin so the logic behind this had to be implemented
anyways. I just made it accessible. What about I work on improving that
explanation hopefully to make it less confusing.
> 
> > +- ti,timer-2x-enable: If this boolean property is provided the device's safety
> > +    timer is extended by a factor of two.
> 
> Boolean properties like this have disadvantage - they cannot be
> overridden. Also extending them is difficult (e.g. for some next
> device the timer could have value of 4). Usually it is better to have
> a value-based property with "0" meaning default or disabled.

When I looked through the Kernel headers I found the boolean property
parse function and figured that's probably what I should be using. But
as I was implementing it I had similar concerns to what you mentioned.
If it's general practice to prefer the value based properties I'll be
happy to change that.

> 
> Also please look at existing bindings:
> $ git grep "ti," Documentation/devicetree/bindings/power
> Maybe you could reuse some? Each of these existing and new bindings
> are made generic (not device specific) so they should be reused.

Already spent time with existing bqXXXXX drivers to see if I can recycle
properties. Will give it another run-through to make sure there is
maximum re-use.

Thanks,

--
Andreas Dannenberg
Texas Instruments Inc

> 
> Best regards,
> Krzysztof
> 
> > +- interrupt-parent: Should be the phandle for the interrupt controller. Use in
> > +    conjunction with "interrupts" and only in case "stat-gpio" is not used.
> > +- interrupts: Interrupt mapping for GPIO IRQ (configure for both edges). Use in
> > +    conjunction with "interrupt-parent" and only in case "stat-gpio" is not
> > +    used.
> >
> >  Example:
> >
> >  bq24257 {
> >         compatible = "ti,bq24257";
> >         reg = <0x6a>;
> > +       stat-gpio = <&gpio1 16 GPIO_ACTIVE_HIGH>;
> > +       pg-gpio = <&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,in-limit-current = <900000>;
> > +       ti,vovp-voltage = <9500000>;
> > +       ti,vindpm-voltage = <4440000>;
> > +};
> > --
> > 1.9.1
> >

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

[Krzysztof Kozlowski]

On 02.09.2015 23:03, Andreas Dannenberg wrote:
> Hi Krzysztof, thanks for your feedback... comments below.
> 
> On Wed, Sep 02, 2015 at 02:24:05PM +0900, Krzysztof Kozlowski wrote:
>> 2015-09-01 11:10 GMT+09:00 Andreas Dannenberg <dannenberg@ti.com>:
>>> --- a/Documentation/devicetree/bindings/power/bq24257.txt
>>> +++ b/Documentation/devicetree/bindings/power/bq24257.txt
>>> @@ -2,20 +2,71 @@ Binding for TI bq24257 Li-Ion Charger
>>>
>>>  Required properties:
>>>  - compatible: Should contain one of the following:
>>> + * "ti,bq24250"
>>> + * "ti,bq24251"
>>>   * "ti,bq24257"
>>> -- reg:                    integer, i2c address of the device.
>>> +- reg: integer, i2c address of the device.
>>> +- stat-gpio: 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.
>>
>> Preferred suffix should be "gpios":
>> Documentation/devicetree/bindings/gpio/gpio.txt
> 
> Did not know this. Thanks for pointing to the src document. Will fix.
> 
>>> +Optional properties:
>>> +- ti,in-ilimit-autoset-disable: If this boolean property is present it disables
>>
>> I thought "ilimit" is a typo but apparently not :) . The names of this
>> property and "ti,in-limit-current" below are quite obfuscated. I am
>> thinking about different names but first I would like to understand
>> the feature: what do you mean by "automatic setting of the input
>> current limit"? Like adjusting the limit of possible current to given
>> charger type?
> 
> Yes that's right. Some of the chargers can check the USB D+/D- lines to
> determine what kind of charger is attached (DCP, CDP, SDP, other) - see
> bq24257 device datasheet for more info:
> http://www.ti.com/product/bq24257
> 
> And this feature is being used in the original driver Laurentiu
> developed. However this detection mechanism (or any detection mechanism
> in general) may not always be appropriate hence the new property to
> disable it and manually provide an input current limit of how much can
> be drawn from the charger. Plus it's needed for devices that don't have
> the D+/D- detection feature.

Thanks for explanation. How about leaving only "ti,in-limit-current"
(get rid of "ti,in-ilimit-autoset-disable"). If it is present then the
autodetection/autoset would be disabled.

> 
>>
>>> +    the automatic setting of the input current limit. This property is also set
>>> +    implicitly on devices without charger type detection. If this property is
>>> +    provided the input current limit should be set manually through
>>> +    "ti,in-limit-current".
>>> +- ti,in-limit-current: The maximum current to be drawn from the charger's input
>>> +    (in uA). Use this for devices that don't have charger type detection or if
>>> +    you have have set "ti,in-ilimit-autoset-disable".
>>> +- ti,vovp-voltage: Configures the over voltage protection voltage (in uV).
>>> +- ti,vindpm-voltage:  Configures the threshold input voltage for the dynamic
>>> +    power path management (in uV).
>>
>> I think the leading 'v' could be removed, so "ti,ovp-voltage" and
>> "ti,in-dpm-voltage"?
> 
> Yes I was thinking about this but then went with something that closely
> reflects the device datasheets to maximize correlation. Removing the 'v'
> would make it look cleaner and more universal and I suppose it's not to
> far of a stretch for someone to associate "ti,ovp-voltag" with the VOVP
> bit field in the devices register map. So let's change it.

Thanks. Bindings don't have to follow naming convention of company's
datasheet.

> 
>>
>>> +- ti,pg-gpio-disable: If this boolean property is provided a software-based
>>> +    approach for power good determination is used. Note that the PG-pin based
>>> +    approach is generally preferable.
>>
>> That's confusing. If someone does not want to use pg-gpio then he
>> could just skip the "pg-gpio" property?
> 
> Yes that's right. Somebody might chose to do so because they don't want
> to sacrifice the extra pin. And I need to accommodate for devices that
> don't have a PG pin so the logic behind this had to be implemented
> anyways. I just made it accessible. What about I work on improving that
> explanation hopefully to make it less confusing.

I still don't get why you need extra "ti,pg-gpio-disable" property. It
could work like this:
1. Get rid of "ti,pg-gpio-disable" and make "pg-gpio" optional.
2. If it is present, use it.
3. If it is not present, use software based approach (the same as
setting "ti,pg-gpio-disable" previously).

Would that work?

>>
>>> +- ti,timer-2x-enable: If this boolean property is provided the device's safety
>>> +    timer is extended by a factor of two.
>>
>> Boolean properties like this have disadvantage - they cannot be
>> overridden. Also extending them is difficult (e.g. for some next
>> device the timer could have value of 4). Usually it is better to have
>> a value-based property with "0" meaning default or disabled.
> 
> When I looked through the Kernel headers I found the boolean property
> parse function and figured that's probably what I should be using. But
> as I was implementing it I had similar concerns to what you mentioned.
> If it's general practice to prefer the value based properties I'll be
> happy to change that.

Usually the boolean is a good choice but here you used it for a
numerical property. You are preparing a generic TI bindings so it is
wise to think about other future drivers. Would they have other values
for timer factor? Would you want to set it in common board DTSI and
override it in some one specific child DTS?

Best regards,
Krzysztof

> 
>>
>> Also please look at existing bindings:
>> $ git grep "ti," Documentation/devicetree/bindings/power
>> Maybe you could reuse some? Each of these existing and new bindings
>> are made generic (not device specific) so they should be reused.
> 
> Already spent time with existing bqXXXXX drivers to see if I can recycle
> properties. Will give it another run-through to make sure there is
> maximum re-use.
> 
> Thanks,
> 
> --
> Andreas Dannenberg
> Texas Instruments Inc
> 
>>
>> Best regards,
>> Krzysztof
>>
>>> +- interrupt-parent: Should be the phandle for the interrupt controller. Use in
>>> +    conjunction with "interrupts" and only in case "stat-gpio" is not used.
>>> +- interrupts: Interrupt mapping for GPIO IRQ (configure for both edges). Use in
>>> +    conjunction with "interrupt-parent" and only in case "stat-gpio" is not
>>> +    used.
>>>
>>>  Example:
>>>
>>>  bq24257 {
>>>         compatible = "ti,bq24257";
>>>         reg = <0x6a>;
>>> +       stat-gpio = <&gpio1 16 GPIO_ACTIVE_HIGH>;
>>> +       pg-gpio = <&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,in-limit-current = <900000>;
>>> +       ti,vovp-voltage = <9500000>;
>>> +       ti,vindpm-voltage = <4440000>;
>>> +};
>>> --
>>> 1.9.1
>>>
> 

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

[Andreas Dannenberg]

On Thu, Sep 03, 2015 at 10:31:20AM +0900, Krzysztof Kozlowski wrote:
> > Yes that's right. Some of the chargers can check the USB D+/D- lines to
> > determine what kind of charger is attached (DCP, CDP, SDP, other) - see
> > bq24257 device datasheet for more info:
> > http://www.ti.com/product/bq24257
> > 
> > And this feature is being used in the original driver Laurentiu
> > developed. However this detection mechanism (or any detection mechanism
> > in general) may not always be appropriate hence the new property to
> > disable it and manually provide an input current limit of how much can
> > be drawn from the charger. Plus it's needed for devices that don't have
> > the D+/D- detection feature.
> 
> Thanks for explanation. How about leaving only "ti,in-limit-current"
> (get rid of "ti,in-ilimit-autoset-disable"). If it is present then the
> autodetection/autoset would be disabled.

I had it separated out to make the DT declarations more explicit (so one
would immediately "see" that autoset is disabled) but other than that
there is no reason why this couldn't get folded together. Will change.

> > Yes I was thinking about this but then went with something that closely
> > reflects the device datasheets to maximize correlation. Removing the 'v'
> > would make it look cleaner and more universal and I suppose it's not to
> > far of a stretch for someone to associate "ti,ovp-voltag" with the VOVP
> > bit field in the devices register map. So let's change it.
> 
> Thanks. Bindings don't have to follow naming convention of company's
> datasheet.

Ok.

> > Yes that's right. Somebody might chose to do so because they don't want
> > to sacrifice the extra pin. And I need to accommodate for devices that
> > don't have a PG pin so the logic behind this had to be implemented
> > anyways. I just made it accessible. What about I work on improving that
> > explanation hopefully to make it less confusing.
> 
> I still don't get why you need extra "ti,pg-gpio-disable" property. It
> could work like this:
> 1. Get rid of "ti,pg-gpio-disable" and make "pg-gpio" optional.
> 2. If it is present, use it.
> 3. If it is not present, use software based approach (the same as
> setting "ti,pg-gpio-disable" previously).
> 
> Would that work?

Similar to the earlier comment - the idea was to have it more explicit.
Plus, the original driver would hard-fail when the "pg-gpio" pin is not
provided (since it was essential for that driver's ability to function).
If I change to automatically fallback to the SW solution such an error
case would in theory not be 100% backward compatible. I could however
more clearly indicate/log which method is being used (dev_info) so
somebody rummaging through the logs would still see it. Will simplify.

> > When I looked through the Kernel headers I found the boolean property
> > parse function and figured that's probably what I should be using. But
> > as I was implementing it I had similar concerns to what you mentioned.
> > If it's general practice to prefer the value based properties I'll be
> > happy to change that.
> 
> Usually the boolean is a good choice but here you used it for a
> numerical property. You are preparing a generic TI bindings so it is
> wise to think about other future drivers. Would they have other values
> for timer factor? Would you want to set it in common board DTSI and
> override it in some one specific child DTS?

Thanks again for the additional comments and the pointers.

Regards,

--
Andreas Dannenberg
Texas Instruments Inc


> 
> Best regards,
> Krzysztof
> 
> > 
> >>
> >> Also please look at existing bindings:
> >> $ git grep "ti," Documentation/devicetree/bindings/power
> >> Maybe you could reuse some? Each of these existing and new bindings
> >> are made generic (not device specific) so they should be reused.
> > 
> > Already spent time with existing bqXXXXX drivers to see if I can recycle
> > properties. Will give it another run-through to make sure there is
> > maximum re-use.
> > 
> > Thanks,
> > 
> > --
> > Andreas Dannenberg
> > Texas Instruments Inc
> > 
> >>
> >> Best regards,
> >> Krzysztof
> >>
> >>> +- interrupt-parent: Should be the phandle for the interrupt controller. Use in
> >>> +    conjunction with "interrupts" and only in case "stat-gpio" is not used.
> >>> +- interrupts: Interrupt mapping for GPIO IRQ (configure for both edges). Use in
> >>> +    conjunction with "interrupt-parent" and only in case "stat-gpio" is not
> >>> +    used.
> >>>
> >>>  Example:
> >>>
> >>>  bq24257 {
> >>>         compatible = "ti,bq24257";
> >>>         reg = <0x6a>;
> >>> +       stat-gpio = <&gpio1 16 GPIO_ACTIVE_HIGH>;
> >>> +       pg-gpio = <&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,in-limit-current = <900000>;
> >>> +       ti,vovp-voltage = <9500000>;
> >>> +       ti,vindpm-voltage = <4440000>;
> >>> +};
> >>> --
> >>> 1.9.1
> >>>
> > 
> 

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

[Krzysztof Kozlowski]

On 03.09.2015 10:47, Andreas Dannenberg wrote:
> On Thu, Sep 03, 2015 at 10:31:20AM +0900, Krzysztof Kozlowski wrote:
>>> Yes that's right. Some of the chargers can check the USB D+/D- lines to
>>> determine what kind of charger is attached (DCP, CDP, SDP, other) - see
>>> bq24257 device datasheet for more info:
>>> http://www.ti.com/product/bq24257
>>>
>>> And this feature is being used in the original driver Laurentiu
>>> developed. However this detection mechanism (or any detection mechanism
>>> in general) may not always be appropriate hence the new property to
>>> disable it and manually provide an input current limit of how much can
>>> be drawn from the charger. Plus it's needed for devices that don't have
>>> the D+/D- detection feature.
>>
>> Thanks for explanation. How about leaving only "ti,in-limit-current"
>> (get rid of "ti,in-ilimit-autoset-disable"). If it is present then the
>> autodetection/autoset would be disabled.
> 
> I had it separated out to make the DT declarations more explicit (so one
> would immediately "see" that autoset is disabled) but other than that
> there is no reason why this couldn't get folded together. Will change.
> 
>>> Yes I was thinking about this but then went with something that closely
>>> reflects the device datasheets to maximize correlation. Removing the 'v'
>>> would make it look cleaner and more universal and I suppose it's not to
>>> far of a stretch for someone to associate "ti,ovp-voltag" with the VOVP
>>> bit field in the devices register map. So let's change it.
>>
>> Thanks. Bindings don't have to follow naming convention of company's
>> datasheet.
> 
> Ok.
> 
>>> Yes that's right. Somebody might chose to do so because they don't want
>>> to sacrifice the extra pin. And I need to accommodate for devices that
>>> don't have a PG pin so the logic behind this had to be implemented
>>> anyways. I just made it accessible. What about I work on improving that
>>> explanation hopefully to make it less confusing.
>>
>> I still don't get why you need extra "ti,pg-gpio-disable" property. It
>> could work like this:
>> 1. Get rid of "ti,pg-gpio-disable" and make "pg-gpio" optional.
>> 2. If it is present, use it.
>> 3. If it is not present, use software based approach (the same as
>> setting "ti,pg-gpio-disable" previously).
>>
>> Would that work?
> 
> Similar to the earlier comment - the idea was to have it more explicit.
> Plus, the original driver would hard-fail when the "pg-gpio" pin is not
> provided (since it was essential for that driver's ability to function).
> If I change to automatically fallback to the SW solution such an error
> case would in theory not be 100% backward compatible. I could however
> more clearly indicate/log which method is being used (dev_info) so
> somebody rummaging through the logs would still see it. Will simplify.

Right, the kernel's backward compatibility has to be preserved. However
for bindings I am not sure.

Let's assume booting on device with bq24257.
1. Old kernel booted with a DTB which doesn't have "pg-gpio" would print
error (probe would fail).
2. New kernel booted in the same situation (1) would assume GPIOs have
to be disabled.

3. Old kernel booted with your previous solution (DTB containing both
"pg-gpio" and "ti,pg-gpio-disable") would work fine ignoring the
"disable" property.
4. New kernel in the same situation (3) would disable GPIOs.

There is a difference between (1) and (3). New DTB is not backward
compatible with old kernels for existing bq24257 devices.

Am I understanding this correctly?

Best regards,
Krzysztof



> 
>>> When I looked through the Kernel headers I found the boolean property
>>> parse function and figured that's probably what I should be using. But
>>> as I was implementing it I had similar concerns to what you mentioned.
>>> If it's general practice to prefer the value based properties I'll be
>>> happy to change that.
>>
>> Usually the boolean is a good choice but here you used it for a
>> numerical property. You are preparing a generic TI bindings so it is
>> wise to think about other future drivers. Would they have other values
>> for timer factor? Would you want to set it in common board DTSI and
>> override it in some one specific child DTS?
> 
> Thanks again for the additional comments and the pointers.
> 
> Regards,
> 
> --
> Andreas Dannenberg
> Texas Instruments Inc
> 
> 
>>
>> Best regards,
>> Krzysztof
>>
>>>
>>>>
>>>> Also please look at existing bindings:
>>>> $ git grep "ti," Documentation/devicetree/bindings/power
>>>> Maybe you could reuse some? Each of these existing and new bindings
>>>> are made generic (not device specific) so they should be reused.
>>>
>>> Already spent time with existing bqXXXXX drivers to see if I can recycle
>>> properties. Will give it another run-through to make sure there is
>>> maximum re-use.
>>>
>>> Thanks,
>>>
>>> --
>>> Andreas Dannenberg
>>> Texas Instruments Inc
>>>
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>>
>>>>> +- interrupt-parent: Should be the phandle for the interrupt controller. Use in
>>>>> +    conjunction with "interrupts" and only in case "stat-gpio" is not used.
>>>>> +- interrupts: Interrupt mapping for GPIO IRQ (configure for both edges). Use in
>>>>> +    conjunction with "interrupt-parent" and only in case "stat-gpio" is not
>>>>> +    used.
>>>>>
>>>>>  Example:
>>>>>
>>>>>  bq24257 {
>>>>>         compatible = "ti,bq24257";
>>>>>         reg = <0x6a>;
>>>>> +       stat-gpio = <&gpio1 16 GPIO_ACTIVE_HIGH>;
>>>>> +       pg-gpio = <&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,in-limit-current = <900000>;
>>>>> +       ti,vovp-voltage = <9500000>;
>>>>> +       ti,vindpm-voltage = <4440000>;
>>>>> +};
>>>>> --
>>>>> 1.9.1
>>>>>
>>>
>>
> 

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

[Andreas Dannenberg]

On Thu, Sep 03, 2015 at 10:57:12AM +0900, Krzysztof Kozlowski wrote:
> On 03.09.2015 10:47, Andreas Dannenberg wrote:
> > On Thu, Sep 03, 2015 at 10:31:20AM +0900, Krzysztof Kozlowski wrote:
> >> I still don't get why you need extra "ti,pg-gpio-disable" property. It
> >> could work like this:
> >> 1. Get rid of "ti,pg-gpio-disable" and make "pg-gpio" optional.
> >> 2. If it is present, use it.
> >> 3. If it is not present, use software based approach (the same as
> >> setting "ti,pg-gpio-disable" previously).
> >>
> >> Would that work?
> > 
> > Similar to the earlier comment - the idea was to have it more explicit.
> > Plus, the original driver would hard-fail when the "pg-gpio" pin is not
> > provided (since it was essential for that driver's ability to function).
> > If I change to automatically fallback to the SW solution such an error
> > case would in theory not be 100% backward compatible. I could however
> > more clearly indicate/log which method is being used (dev_info) so
> > somebody rummaging through the logs would still see it. Will simplify.
> 
> Right, the kernel's backward compatibility has to be preserved. However
> for bindings I am not sure.

Hi Krzysztof. Please see pages 3-5 in the below PDF, that's what I had
in mind. Maybe I mis-interpreted it or there is newer information.
http://events.linuxfoundation.org/sites/events/files/slides/petazzoni-dt-as-stable-abi-fairy-tale.pdf

With the proposed changes, while the existing bindings (strings) would
stay the same, the failure behavior in one specific use case would be
slightly different. The question is, where should we draw the line for
compatibility? I would argue in this case the general usage is not
affected so it should be OK making the change/simplification you propose
(getting rid of "ti,pg-gpio-disable"). 
 
> Let's assume booting on device with bq24257.
> 1. Old kernel booted with a DTB which doesn't have "pg-gpio" would print
> error (probe would fail).
> 2. New kernel booted in the same situation (1) would assume GPIOs have
> to be disabled.
> 
> 3. Old kernel booted with your previous solution (DTB containing both
> "pg-gpio" and "ti,pg-gpio-disable") would work fine ignoring the
> "disable" property.
> 4. New kernel in the same situation (3) would disable GPIOs.
> 
> There is a difference between (1) and (3). New DTB is not backward
> compatible with old kernels for existing bq24257 devices.
> 
> Am I understanding this correctly?

Well almost. I think the difference between case 1 and 3 isn't really an
issue from a use case point of view. The case I was trying to describe
is as follows (let's call it case 5):

5. An old DTB not containing "pg-gpio" (or with that property being
misconfigured) is booted with the new Kernel. The boot will succeed as
the SW approach for PG detection gets invoked automatically.

The difference now is between (1) and (5). If in (5) somebody specifies
"pg-gpio" they would want to explicitly use this pin and be notified if
the respective setup fails. My proposal earlier for this was to output
through dev_info() during driver probe whether an actual GPIO pin is
being used for power-good detection or whether the fall-back SW
algorithm has been invoked. This way, at least there is a notification.



Thanks and Regards,

--
Andreas Dannenberg
Texas Instruments Inc

> 
> Best regards,
> Krzysztof

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

[Krzysztof Kozlowski]

On 04.09.2015 01:09, Andreas Dannenberg wrote:
> On Thu, Sep 03, 2015 at 10:57:12AM +0900, Krzysztof Kozlowski wrote:
>> On 03.09.2015 10:47, Andreas Dannenberg wrote:
>>> On Thu, Sep 03, 2015 at 10:31:20AM +0900, Krzysztof Kozlowski wrote:
>>>> I still don't get why you need extra "ti,pg-gpio-disable" property. It
>>>> could work like this:
>>>> 1. Get rid of "ti,pg-gpio-disable" and make "pg-gpio" optional.
>>>> 2. If it is present, use it.
>>>> 3. If it is not present, use software based approach (the same as
>>>> setting "ti,pg-gpio-disable" previously).
>>>>
>>>> Would that work?
>>>
>>> Similar to the earlier comment - the idea was to have it more explicit.
>>> Plus, the original driver would hard-fail when the "pg-gpio" pin is not
>>> provided (since it was essential for that driver's ability to function).
>>> If I change to automatically fallback to the SW solution such an error
>>> case would in theory not be 100% backward compatible. I could however
>>> more clearly indicate/log which method is being used (dev_info) so
>>> somebody rummaging through the logs would still see it. Will simplify.
>>
>> Right, the kernel's backward compatibility has to be preserved. However
>> for bindings I am not sure.
> 
> Hi Krzysztof. Please see pages 3-5 in the below PDF, that's what I had
> in mind. Maybe I mis-interpreted it or there is newer information.
> http://events.linuxfoundation.org/sites/events/files/slides/petazzoni-dt-as-stable-abi-fairy-tale.pdf

I think our case is not covered there. I am talking about backward
compatibility of DTB with older kernels, not about backward
compatibility of kernel.

AFAIR this is not a requirement so you can change the behaviour of DTB
(backward compatibility of kernel is fully preserved).

> 
> With the proposed changes, while the existing bindings (strings) would
> stay the same, the failure behavior in one specific use case would be
> slightly different. The question is, where should we draw the line for
> compatibility? I would argue in this case the general usage is not
> affected so it should be OK making the change/simplification you propose
> (getting rid of "ti,pg-gpio-disable"). 

If we don't care about using newer DTB with older kernel then there is
no compatibility line to draw. You can safely change the behaviour.

>  
>> Let's assume booting on device with bq24257.
>> 1. Old kernel booted with a DTB which doesn't have "pg-gpio" would print
>> error (probe would fail).
>> 2. New kernel booted in the same situation (1) would assume GPIOs have
>> to be disabled.
>>
>> 3. Old kernel booted with your previous solution (DTB containing both
>> "pg-gpio" and "ti,pg-gpio-disable") would work fine ignoring the
>> "disable" property.
>> 4. New kernel in the same situation (3) would disable GPIOs.
>>
>> There is a difference between (1) and (3). New DTB is not backward
>> compatible with old kernels for existing bq24257 devices.
>>
>> Am I understanding this correctly?
> 
> Well almost. I think the difference between case 1 and 3 isn't really an
> issue from a use case point of view. The case I was trying to describe
> is as follows (let's call it case 5):
> 
> 5. An old DTB not containing "pg-gpio" (or with that property being
> misconfigured) is booted with the new Kernel. The boot will succeed as
> the SW approach for PG detection gets invoked automatically.
> 
> The difference now is between (1) and (5). If in (5) somebody specifies
> "pg-gpio" they would want to explicitly use this pin and be notified if
> the respective setup fails. My proposal earlier for this was to output
> through dev_info() during driver probe whether an actual GPIO pin is
> being used for power-good detection or whether the fall-back SW
> algorithm has been invoked. This way, at least there is a notification.

I think I understand. IMHO if pg-gpio is specified and it fails (like
GPIO is invalid) the kernel should not switch to SW mode. This should be
all-or-nothing and described in a specific way: iff pg-gpio is missing
then SW mode would be used because that was the intention of developer.

Best regards,
Krzysztof