[PATCH v3 0/5] BQ24190 devicetree config

[PATCH v3 0/5] BQ24190 devicetree config

[Liam Breck]

Hi Sebastian, here is this series that was waiting on the power_supply_battery_info
patchset. Thanks!

Overview:
BQ24190 uses power_supply_battery_info fields for precharge & charge-termination 
current and a DT property for system min voltage. A new DT binding documents these.

Changes in v3:
* clarify gpio-hog use in DT binding doc
* drop unrelated change from DT table patch

Changes in v2:
* new patch adds bq24192i to DT table
* dropped patches for power_supply precharge-current; they moved to bq27xxx series
* dropped patches for recently-applied fixes
* rebased to -next

Issues in v1:
* power_supply_prop_precharge & endcharge are added at end of enum to avoid sysfs breakage
* "Set bq24190-battery device .type=unknown" is a temporary workaround to be fixed in v2

Liam Breck (5):
  power: bq24190_charger: Add ti,bq24192i to devicetree table
  devicetree: power: Add docs for TI BQ24190 battery charger
  power: bq24190_charger: Enable devicetree config
  power: bq24190_charger: Add property system-minimum-microvolt
  power: bq24190_charger: Add power_supply_battery_info support

 .../devicetree/bindings/power/supply/bq24190.txt   |  51 ++++++
 drivers/power/supply/bq24190_charger.c             | 173 ++++++++++++++++++---
 2 files changed, 204 insertions(+), 20 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/power/supply/bq24190.txt

-- 
2.13.2

[PATCH v3 1/5] power: bq24190_charger: Add ti,bq24192i to devicetree table

[Liam Breck]

From: Liam Breck <kernel@networkimprov.net>

bq24192i was previously only in ID table, so add it to DT table.

Cc: Tony Lindgren <tony@atomide.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Liam Breck <kernel@networkimprov.net>
---
 drivers/power/supply/bq24190_charger.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index 14199561..40b4bba7 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -1732,6 +1732,7 @@ MODULE_DEVICE_TABLE(i2c, bq24190_i2c_ids);
 #ifdef CONFIG_OF
 static const struct of_device_id bq24190_of_match[] = {
 	{ .compatible = "ti,bq24190", },
+	{ .compatible = "ti,bq24192i", },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, bq24190_of_match);
-- 
2.13.2

Re: [PATCH v3 1/5] power: bq24190_charger: Add ti,bq24192i to devicetree table

[Tony Lindgren]

* Liam Breck <liam@networkimprov.net> [170825 22:15]:
> From: Liam Breck <kernel@networkimprov.net>
> 
> bq24192i was previously only in ID table, so add it to DT table.
> 
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Liam Breck <kernel@networkimprov.net>

Acked-by: Tony Lindgren <tony@atomide.com>

> ---
>  drivers/power/supply/bq24190_charger.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index 14199561..40b4bba7 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -1732,6 +1732,7 @@ MODULE_DEVICE_TABLE(i2c, bq24190_i2c_ids);
>  #ifdef CONFIG_OF
>  static const struct of_device_id bq24190_of_match[] = {
>  	{ .compatible = "ti,bq24190", },
> +	{ .compatible = "ti,bq24192i", },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(of, bq24190_of_match);
> -- 
> 2.13.2
> 

[PATCH v3 2/5] devicetree: power: Add docs for TI BQ24190 battery charger

[Liam Breck]

From: Liam Breck <kernel-RYWXG+zxWwBdeoIcmNTgJF6hYfS7NtTn@public.gmane.org>

Document monitored-battery and ti,system-minimum-microvolt properties.

Cc: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Liam Breck <kernel-RYWXG+zxWwBdeoIcmNTgJF6hYfS7NtTn@public.gmane.org>
---
 .../devicetree/bindings/power/supply/bq24190.txt   | 51 ++++++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/supply/bq24190.txt

diff --git a/Documentation/devicetree/bindings/power/supply/bq24190.txt b/Documentation/devicetree/bindings/power/supply/bq24190.txt
new file mode 100644
index 00000000..9e517d30
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/supply/bq24190.txt
@@ -0,0 +1,51 @@
+TI BQ24190 Li-Ion Battery Charger
+
+Required properties:
+- compatible: contains one of the following:
+    * "ti,bq24190"
+    * "ti,bq24192i"
+- reg: integer, I2C address of the charger.
+- interrupts[-extended]: configuration for charger INT pin.
+
+Optional properties:
+- monitored-battery: phandle of battery characteristics devicetree node
+  The charger uses the following battery properties:
+    + precharge-current-microamp: maximum charge current during precharge
+      phase (typically 20% of battery capacity).
+    + charge-term-current-microamp: a charge cycle terminates when the
+      battery voltage is above recharge threshold, and the current is below
+      this setting (typically 10% of battery capacity).
+  See also Documentation/devicetree/bindings/power/supply/battery.txt
+- ti,system-minimum-microvolt: when power is connected and the battery is below
+  minimum system voltage, the system will be regulated above this setting.
+
+Notes:
+- Some circuit boards wire the chip's "OTG" pin high (enabling 500mA default
+  charge current on USB SDP ports, among other features). To simulate this on
+  boards that wire the pin to a GPIO, set a gpio-hog.
+
+Example:
+
+	bat: battery {
+		compatible = "simple-battery";
+		precharge-current-microamp = <256000>;
+		charge-term-current-microamp = <128000>;
+		// etc.
+	};
+
+	bq24190: charger@6a {
+		compatible = "ti,bq24190";
+		reg = <0x6a>;
+		interrupts-extended = <&gpiochip 10 IRQ_TYPE_EDGE_FALLING>;
+		monitored-battery = <&bat>;
+		ti,system-minimum-microvolt = <3200000>;
+	};
+
+	&twl_gpio {
+		otg {
+			gpio-hog;
+			gpios = <6 0>;
+			output-high;
+			line-name = "otg-gpio";
+		};
+	};
-- 
2.13.2

--
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 v3 2/5] devicetree: power: Add docs for TI BQ24190 battery charger

[Tony Lindgren]

* Liam Breck <liam@networkimprov.net> [170825 22:15]:
> From: Liam Breck <kernel@networkimprov.net>
> 
> Document monitored-battery and ti,system-minimum-microvolt properties.
> 
> Cc: Rob Herring <robh@kernel.org>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Liam Breck <kernel@networkimprov.net>

Seems OK to me:

Acked-by: Tony Lindgren <tony@atomide.com>

[PATCH v3 3/5] power: bq24190_charger: Enable devicetree config

[Liam Breck]

From: Liam Breck <kernel@networkimprov.net>

Add get_config(). Rename set_mode_host() to set_config().
Call get_config() and hw_init() after power_supply_register().
No functional changes.

Cc: Tony Lindgren <tony@atomide.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Liam Breck <kernel@networkimprov.net>
---
 drivers/power/supply/bq24190_charger.c | 63 +++++++++++++++++++++++-----------
 1 file changed, 43 insertions(+), 20 deletions(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index 40b4bba7..c6be00d7 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -504,15 +504,7 @@ static int bq24190_sysfs_create_group(struct bq24190_dev_info *bdi)
 static inline void bq24190_sysfs_remove_group(struct bq24190_dev_info *bdi) {}
 #endif
 
-/*
- * According to the "Host Mode and default Mode" section of the
- * manual, a write to any register causes the bq24190 to switch
- * from default mode to host mode.  It will switch back to default
- * mode after a WDT timeout unless the WDT is turned off as well.
- * So, by simply turning off the WDT, we accomplish both with the
- * same write.
- */
-static int bq24190_set_mode_host(struct bq24190_dev_info *bdi)
+static int bq24190_set_config(struct bq24190_dev_info *bdi)
 {
 	int ret;
 	u8 v;
@@ -523,9 +515,22 @@ static int bq24190_set_mode_host(struct bq24190_dev_info *bdi)
 
 	bdi->watchdog = ((v & BQ24190_REG_CTTC_WATCHDOG_MASK) >>
 					BQ24190_REG_CTTC_WATCHDOG_SHIFT);
+
+	/*
+	 * According to the "Host Mode and default Mode" section of the
+	 * manual, a write to any register causes the bq24190 to switch
+	 * from default mode to host mode.  It will switch back to default
+	 * mode after a WDT timeout unless the WDT is turned off as well.
+	 * So, by simply turning off the WDT, we accomplish both with the
+	 * same write.
+	 */
 	v &= ~BQ24190_REG_CTTC_WATCHDOG_MASK;
 
-	return bq24190_write(bdi, BQ24190_REG_CTTC, v);
+	ret = bq24190_write(bdi, BQ24190_REG_CTTC, v);
+	if (ret < 0)
+		return ret;
+
+	return 0;
 }
 
 static int bq24190_register_reset(struct bq24190_dev_info *bdi)
@@ -1456,13 +1461,25 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
 	if (ret < 0)
 		return ret;
 
-	ret = bq24190_set_mode_host(bdi);
+	ret = bq24190_set_config(bdi);
 	if (ret < 0)
 		return ret;
 
 	return bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
 }
 
+static int bq24190_get_config(struct bq24190_dev_info *bdi)
+{
+#ifdef CONFIG_OF
+	int v;
+
+	if (!bdi->dev->of_node)
+		return -EINVAL;
+
+#endif
+	return 0;
+}
+
 static int bq24190_probe(struct i2c_client *client,
 		const struct i2c_device_id *id)
 {
@@ -1493,7 +1510,7 @@ static int bq24190_probe(struct i2c_client *client,
 
 	i2c_set_clientdata(client, bdi);
 
-	if (!client->irq) {
+	if (client->irq <= 0) {
 		dev_err(dev, "Can't get irq info\n");
 		return -EINVAL;
 	}
@@ -1526,12 +1543,6 @@ static int bq24190_probe(struct i2c_client *client,
 		goto out_pmrt;
 	}
 
-	ret = bq24190_hw_init(bdi);
-	if (ret < 0) {
-		dev_err(dev, "Hardware init failed\n");
-		goto out_pmrt;
-	}
-
 	charger_cfg.drv_data = bdi;
 	charger_cfg.supplied_to = bq24190_charger_supplied_to;
 	charger_cfg.num_supplicants = ARRAY_SIZE(bq24190_charger_supplied_to),
@@ -1556,8 +1567,20 @@ static int bq24190_probe(struct i2c_client *client,
 		}
 	}
 
+	ret = bq24190_get_config(bdi);
+	if (ret < 0) {
+		dev_err(dev, "Can't get devicetree config\n");
+		goto out_charger;
+	}
+
+	ret = bq24190_hw_init(bdi);
+	if (ret < 0) {
+		dev_err(dev, "Hardware init failed\n");
+		goto out_charger;
+	}
+
 	ret = bq24190_sysfs_create_group(bdi);
-	if (ret) {
+	if (ret < 0) {
 		dev_err(dev, "Can't create sysfs entries\n");
 		goto out_charger;
 	}
@@ -1700,7 +1723,7 @@ static __maybe_unused int bq24190_pm_resume(struct device *dev)
 	}
 
 	bq24190_register_reset(bdi);
-	bq24190_set_mode_host(bdi);
+	bq24190_set_config(bdi);
 	bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
 
 	if (error >= 0) {
-- 
2.13.2

Re: [PATCH v3 3/5] power: bq24190_charger: Enable devicetree config

[Hans de Goede]

Hi,

On 26-08-17 07:14, Liam Breck wrote:
> From: Liam Breck <kernel@networkimprov.net>
> 
> Add get_config(). Rename set_mode_host() to set_config().
> Call get_config() and hw_init() after power_supply_register().
> No functional changes.

Doing hw_init after power_supply_register() means that userspace
can get/set power_supply properties before hw_init has run, this
generally is not a good idea.

> 
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Liam Breck <kernel@networkimprov.net>
> ---
>   drivers/power/supply/bq24190_charger.c | 63 +++++++++++++++++++++++-----------
>   1 file changed, 43 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index 40b4bba7..c6be00d7 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -504,15 +504,7 @@ static int bq24190_sysfs_create_group(struct bq24190_dev_info *bdi)
>   static inline void bq24190_sysfs_remove_group(struct bq24190_dev_info *bdi) {}
>   #endif
>   
> -/*
> - * According to the "Host Mode and default Mode" section of the
> - * manual, a write to any register causes the bq24190 to switch
> - * from default mode to host mode.  It will switch back to default
> - * mode after a WDT timeout unless the WDT is turned off as well.
> - * So, by simply turning off the WDT, we accomplish both with the
> - * same write.
> - */
> -static int bq24190_set_mode_host(struct bq24190_dev_info *bdi)
> +static int bq24190_set_config(struct bq24190_dev_info *bdi)
>   {
>   	int ret;
>   	u8 v;
> @@ -523,9 +515,22 @@ static int bq24190_set_mode_host(struct bq24190_dev_info *bdi)
>   
>   	bdi->watchdog = ((v & BQ24190_REG_CTTC_WATCHDOG_MASK) >>
>   					BQ24190_REG_CTTC_WATCHDOG_SHIFT);
> +
> +	/*
> +	 * According to the "Host Mode and default Mode" section of the
> +	 * manual, a write to any register causes the bq24190 to switch
> +	 * from default mode to host mode.  It will switch back to default
> +	 * mode after a WDT timeout unless the WDT is turned off as well.
> +	 * So, by simply turning off the WDT, we accomplish both with the
> +	 * same write.
> +	 */
>   	v &= ~BQ24190_REG_CTTC_WATCHDOG_MASK;
>   
> -	return bq24190_write(bdi, BQ24190_REG_CTTC, v);
> +	ret = bq24190_write(bdi, BQ24190_REG_CTTC, v);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
>   }
>   
>   static int bq24190_register_reset(struct bq24190_dev_info *bdi)
> @@ -1456,13 +1461,25 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
>   	if (ret < 0)
>   		return ret;
>   
> -	ret = bq24190_set_mode_host(bdi);
> +	ret = bq24190_set_config(bdi);
>   	if (ret < 0)
>   		return ret;
>   
>   	return bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
>   }
>   
> +static int bq24190_get_config(struct bq24190_dev_info *bdi)
> +{
> +#ifdef CONFIG_OF
> +	int v;

This variable is unused (in this patch)

> +
> +	if (!bdi->dev->of_node)
> +		return -EINVAL;

I don't think that this is a good idea, at least on ARM
(AFAIK) a kernel can be configured to support both DT and
ACPI, just having CONFIG_OF enabled as such is not a good
reason to fail to probe when there is no of_node.

IMHO it would be better to instead put the reading of the
"ti,system-minimum-microvolt" property introduced in the next
patch inside a: if (bdi->dev->of_node) { ... } block.

> +
> +#endif
> +	return 0;
> +}
> +
>   static int bq24190_probe(struct i2c_client *client,
>   		const struct i2c_device_id *id)
>   {
> @@ -1493,7 +1510,7 @@ static int bq24190_probe(struct i2c_client *client,
>   
>   	i2c_set_clientdata(client, bdi);
>   
> -	if (!client->irq) {
> +	if (client->irq <= 0) {
>   		dev_err(dev, "Can't get irq info\n");
>   		return -EINVAL;
>   	}
> @@ -1526,12 +1543,6 @@ static int bq24190_probe(struct i2c_client *client,
>   		goto out_pmrt;
>   	}
>   
> -	ret = bq24190_hw_init(bdi);
> -	if (ret < 0) {
> -		dev_err(dev, "Hardware init failed\n");
> -		goto out_pmrt;
> -	}
> -
>   	charger_cfg.drv_data = bdi;
>   	charger_cfg.supplied_to = bq24190_charger_supplied_to;
>   	charger_cfg.num_supplicants = ARRAY_SIZE(bq24190_charger_supplied_to),
> @@ -1556,8 +1567,20 @@ static int bq24190_probe(struct i2c_client *client,
>   		}
>   	}
>   
> +	ret = bq24190_get_config(bdi);
> +	if (ret < 0) {
> +		dev_err(dev, "Can't get devicetree config\n");
> +		goto out_charger;
> +	}
> +
> +	ret = bq24190_hw_init(bdi);
> +	if (ret < 0) {
> +		dev_err(dev, "Hardware init failed\n");
> +		goto out_charger;
> +	}
> +
>   	ret = bq24190_sysfs_create_group(bdi);
> -	if (ret) {
> +	if (ret < 0) {
>   		dev_err(dev, "Can't create sysfs entries\n");
>   		goto out_charger;
>   	}
> @@ -1700,7 +1723,7 @@ static __maybe_unused int bq24190_pm_resume(struct device *dev)
>   	}
>   
>   	bq24190_register_reset(bdi);
> -	bq24190_set_mode_host(bdi);
> +	bq24190_set_config(bdi);
>   	bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
>   
>   	if (error >= 0) {
> 

Regards,

Hans

Re: [PATCH v3 3/5] power: bq24190_charger: Enable devicetree config

[Liam Breck]

Hi Hans, thanks for the input...

On Mon, Aug 28, 2017 at 5:13 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 26-08-17 07:14, Liam Breck wrote:
>>
>> From: Liam Breck <kernel@networkimprov.net>
>>
>> Add get_config(). Rename set_mode_host() to set_config().
>> Call get_config() and hw_init() after power_supply_register().
>> No functional changes.
>
>
> Doing hw_init after power_supply_register() means that userspace
> can get/set power_supply properties before hw_init has run, this
> generally is not a good idea.

hw_init() needs data from DT, and power_supply_get_battery_info()
(called by get_config() in later patch) gets the DT data. It takes a
power_supply argument, hence the new ordering.

Is there a way to defer availability of a registered power_supply to
userspace during probe()? I recall looking into this when I wrote that
and concluding that it was safe, but I don't recall why :-/

>
>>
>> Cc: Tony Lindgren <tony@atomide.com>
>> Cc: Hans de Goede <hdegoede@redhat.com>
>> Signed-off-by: Liam Breck <kernel@networkimprov.net>
>> ---
>>   drivers/power/supply/bq24190_charger.c | 63
>> +++++++++++++++++++++++-----------
>>   1 file changed, 43 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/power/supply/bq24190_charger.c
>> b/drivers/power/supply/bq24190_charger.c
>> index 40b4bba7..c6be00d7 100644
>> --- a/drivers/power/supply/bq24190_charger.c
>> +++ b/drivers/power/supply/bq24190_charger.c
>> @@ -504,15 +504,7 @@ static int bq24190_sysfs_create_group(struct
>> bq24190_dev_info *bdi)
>>   static inline void bq24190_sysfs_remove_group(struct bq24190_dev_info
>> *bdi) {}
>>   #endif
>>   -/*
>> - * According to the "Host Mode and default Mode" section of the
>> - * manual, a write to any register causes the bq24190 to switch
>> - * from default mode to host mode.  It will switch back to default
>> - * mode after a WDT timeout unless the WDT is turned off as well.
>> - * So, by simply turning off the WDT, we accomplish both with the
>> - * same write.
>> - */
>> -static int bq24190_set_mode_host(struct bq24190_dev_info *bdi)
>> +static int bq24190_set_config(struct bq24190_dev_info *bdi)
>>   {
>>         int ret;
>>         u8 v;
>> @@ -523,9 +515,22 @@ static int bq24190_set_mode_host(struct
>> bq24190_dev_info *bdi)
>>         bdi->watchdog = ((v & BQ24190_REG_CTTC_WATCHDOG_MASK) >>
>>                                         BQ24190_REG_CTTC_WATCHDOG_SHIFT);
>> +
>> +       /*
>> +        * According to the "Host Mode and default Mode" section of the
>> +        * manual, a write to any register causes the bq24190 to switch
>> +        * from default mode to host mode.  It will switch back to default
>> +        * mode after a WDT timeout unless the WDT is turned off as well.
>> +        * So, by simply turning off the WDT, we accomplish both with the
>> +        * same write.
>> +        */
>>         v &= ~BQ24190_REG_CTTC_WATCHDOG_MASK;
>>   -     return bq24190_write(bdi, BQ24190_REG_CTTC, v);
>> +       ret = bq24190_write(bdi, BQ24190_REG_CTTC, v);
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       return 0;
>>   }
>>     static int bq24190_register_reset(struct bq24190_dev_info *bdi)
>> @@ -1456,13 +1461,25 @@ static int bq24190_hw_init(struct bq24190_dev_info
>> *bdi)
>>         if (ret < 0)
>>                 return ret;
>>   -     ret = bq24190_set_mode_host(bdi);
>> +       ret = bq24190_set_config(bdi);
>>         if (ret < 0)
>>                 return ret;
>>         return bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
>>   }
>>   +static int bq24190_get_config(struct bq24190_dev_info *bdi)
>> +{
>> +#ifdef CONFIG_OF
>> +       int v;
>
>
> This variable is unused (in this patch)
>
>> +
>> +       if (!bdi->dev->of_node)
>> +               return -EINVAL;
>
>
> I don't think that this is a good idea, at least on ARM
> (AFAIK) a kernel can be configured to support both DT and
> ACPI, just having CONFIG_OF enabled as such is not a good
> reason to fail to probe when there is no of_node.
>
> IMHO it would be better to instead put the reading of the
> "ti,system-minimum-microvolt" property introduced in the next
> patch inside a: if (bdi->dev->of_node) { ... } block.

Well that one is read with device_property_read_ so mebbe I should
drop ifdef config_of and only check dev->of_node before
_get_battery_info() which requires DT.

>> +
>> +#endif
>> +       return 0;
>> +}
>> +
>>   static int bq24190_probe(struct i2c_client *client,
>>                 const struct i2c_device_id *id)
>>   {
>> @@ -1493,7 +1510,7 @@ static int bq24190_probe(struct i2c_client *client,
>>         i2c_set_clientdata(client, bdi);
>>   -     if (!client->irq) {
>> +       if (client->irq <= 0) {
>>                 dev_err(dev, "Can't get irq info\n");
>>                 return -EINVAL;
>>         }
>> @@ -1526,12 +1543,6 @@ static int bq24190_probe(struct i2c_client *client,
>>                 goto out_pmrt;
>>         }
>>   -     ret = bq24190_hw_init(bdi);
>> -       if (ret < 0) {
>> -               dev_err(dev, "Hardware init failed\n");
>> -               goto out_pmrt;
>> -       }
>> -
>>         charger_cfg.drv_data = bdi;
>>         charger_cfg.supplied_to = bq24190_charger_supplied_to;
>>         charger_cfg.num_supplicants =
>> ARRAY_SIZE(bq24190_charger_supplied_to),
>> @@ -1556,8 +1567,20 @@ static int bq24190_probe(struct i2c_client *client,
>>                 }
>>         }
>>   +     ret = bq24190_get_config(bdi);
>> +       if (ret < 0) {
>> +               dev_err(dev, "Can't get devicetree config\n");
>> +               goto out_charger;
>> +       }
>> +
>> +       ret = bq24190_hw_init(bdi);
>> +       if (ret < 0) {
>> +               dev_err(dev, "Hardware init failed\n");
>> +               goto out_charger;
>> +       }
>> +
>>         ret = bq24190_sysfs_create_group(bdi);
>> -       if (ret) {
>> +       if (ret < 0) {
>>                 dev_err(dev, "Can't create sysfs entries\n");
>>                 goto out_charger;
>>         }
>> @@ -1700,7 +1723,7 @@ static __maybe_unused int bq24190_pm_resume(struct
>> device *dev)
>>         }
>>         bq24190_register_reset(bdi);
>> -       bq24190_set_mode_host(bdi);
>> +       bq24190_set_config(bdi);
>>         bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
>>         if (error >= 0) {
>>
>
> Regards,
>
> Hans

Re: [PATCH v3 3/5] power: bq24190_charger: Enable devicetree config

[Hans de Goede]

Hi,

On 28-08-17 19:59, Liam Breck wrote:
> Hi Hans, thanks for the input...
> 
> On Mon, Aug 28, 2017 at 5:13 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 26-08-17 07:14, Liam Breck wrote:
>>>
>>> From: Liam Breck <kernel@networkimprov.net>
>>>
>>> Add get_config(). Rename set_mode_host() to set_config().
>>> Call get_config() and hw_init() after power_supply_register().
>>> No functional changes.
>>
>>
>> Doing hw_init after power_supply_register() means that userspace
>> can get/set power_supply properties before hw_init has run, this
>> generally is not a good idea.
> 
> hw_init() needs data from DT, and power_supply_get_battery_info()
> (called by get_config() in later patch) gets the DT data. It takes a
> power_supply argument, hence the new ordering.
> 
> Is there a way to defer availability of a registered power_supply to
> userspace during probe()?

No I don't think so.

> I recall looking into this when I wrote that
> and concluding that it was safe, but I don't recall why :-/

I guess you came to that conclusion because non of the properties
we export really depend on any of the settings which are setup by
hw_init, so doing that after registering, although unusual should
be fine I guess. So lets just keep this as is.

> 
>>
>>>
>>> Cc: Tony Lindgren <tony@atomide.com>
>>> Cc: Hans de Goede <hdegoede@redhat.com>
>>> Signed-off-by: Liam Breck <kernel@networkimprov.net>
>>> ---
>>>    drivers/power/supply/bq24190_charger.c | 63
>>> +++++++++++++++++++++++-----------
>>>    1 file changed, 43 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/power/supply/bq24190_charger.c
>>> b/drivers/power/supply/bq24190_charger.c
>>> index 40b4bba7..c6be00d7 100644
>>> --- a/drivers/power/supply/bq24190_charger.c
>>> +++ b/drivers/power/supply/bq24190_charger.c
>>> @@ -504,15 +504,7 @@ static int bq24190_sysfs_create_group(struct
>>> bq24190_dev_info *bdi)
>>>    static inline void bq24190_sysfs_remove_group(struct bq24190_dev_info
>>> *bdi) {}
>>>    #endif
>>>    -/*
>>> - * According to the "Host Mode and default Mode" section of the
>>> - * manual, a write to any register causes the bq24190 to switch
>>> - * from default mode to host mode.  It will switch back to default
>>> - * mode after a WDT timeout unless the WDT is turned off as well.
>>> - * So, by simply turning off the WDT, we accomplish both with the
>>> - * same write.
>>> - */
>>> -static int bq24190_set_mode_host(struct bq24190_dev_info *bdi)
>>> +static int bq24190_set_config(struct bq24190_dev_info *bdi)
>>>    {
>>>          int ret;
>>>          u8 v;
>>> @@ -523,9 +515,22 @@ static int bq24190_set_mode_host(struct
>>> bq24190_dev_info *bdi)
>>>          bdi->watchdog = ((v & BQ24190_REG_CTTC_WATCHDOG_MASK) >>
>>>                                          BQ24190_REG_CTTC_WATCHDOG_SHIFT);
>>> +
>>> +       /*
>>> +        * According to the "Host Mode and default Mode" section of the
>>> +        * manual, a write to any register causes the bq24190 to switch
>>> +        * from default mode to host mode.  It will switch back to default
>>> +        * mode after a WDT timeout unless the WDT is turned off as well.
>>> +        * So, by simply turning off the WDT, we accomplish both with the
>>> +        * same write.
>>> +        */
>>>          v &= ~BQ24190_REG_CTTC_WATCHDOG_MASK;
>>>    -     return bq24190_write(bdi, BQ24190_REG_CTTC, v);
>>> +       ret = bq24190_write(bdi, BQ24190_REG_CTTC, v);
>>> +       if (ret < 0)
>>> +               return ret;
>>> +
>>> +       return 0;
>>>    }
>>>      static int bq24190_register_reset(struct bq24190_dev_info *bdi)
>>> @@ -1456,13 +1461,25 @@ static int bq24190_hw_init(struct bq24190_dev_info
>>> *bdi)
>>>          if (ret < 0)
>>>                  return ret;
>>>    -     ret = bq24190_set_mode_host(bdi);
>>> +       ret = bq24190_set_config(bdi);
>>>          if (ret < 0)
>>>                  return ret;
>>>          return bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
>>>    }
>>>    +static int bq24190_get_config(struct bq24190_dev_info *bdi)
>>> +{
>>> +#ifdef CONFIG_OF
>>> +       int v;
>>
>>
>> This variable is unused (in this patch)
>>
>>> +
>>> +       if (!bdi->dev->of_node)
>>> +               return -EINVAL;
>>
>>
>> I don't think that this is a good idea, at least on ARM
>> (AFAIK) a kernel can be configured to support both DT and
>> ACPI, just having CONFIG_OF enabled as such is not a good
>> reason to fail to probe when there is no of_node.
>>
>> IMHO it would be better to instead put the reading of the
>> "ti,system-minimum-microvolt" property introduced in the next
>> patch inside a: if (bdi->dev->of_node) { ... } block.
> 
> Well that one is read with device_property_read_ so mebbe I should
> drop ifdef config_of and only check dev->of_node before
> _get_battery_info() which requires DT.

Sounds good to me.

Regards,

Hans


> 
>>> +
>>> +#endif
>>> +       return 0;
>>> +}
>>> +
>>>    static int bq24190_probe(struct i2c_client *client,
>>>                  const struct i2c_device_id *id)
>>>    {
>>> @@ -1493,7 +1510,7 @@ static int bq24190_probe(struct i2c_client *client,
>>>          i2c_set_clientdata(client, bdi);
>>>    -     if (!client->irq) {
>>> +       if (client->irq <= 0) {
>>>                  dev_err(dev, "Can't get irq info\n");
>>>                  return -EINVAL;
>>>          }
>>> @@ -1526,12 +1543,6 @@ static int bq24190_probe(struct i2c_client *client,
>>>                  goto out_pmrt;
>>>          }
>>>    -     ret = bq24190_hw_init(bdi);
>>> -       if (ret < 0) {
>>> -               dev_err(dev, "Hardware init failed\n");
>>> -               goto out_pmrt;
>>> -       }
>>> -
>>>          charger_cfg.drv_data = bdi;
>>>          charger_cfg.supplied_to = bq24190_charger_supplied_to;
>>>          charger_cfg.num_supplicants =
>>> ARRAY_SIZE(bq24190_charger_supplied_to),
>>> @@ -1556,8 +1567,20 @@ static int bq24190_probe(struct i2c_client *client,
>>>                  }
>>>          }
>>>    +     ret = bq24190_get_config(bdi);
>>> +       if (ret < 0) {
>>> +               dev_err(dev, "Can't get devicetree config\n");
>>> +               goto out_charger;
>>> +       }
>>> +
>>> +       ret = bq24190_hw_init(bdi);
>>> +       if (ret < 0) {
>>> +               dev_err(dev, "Hardware init failed\n");
>>> +               goto out_charger;
>>> +       }
>>> +
>>>          ret = bq24190_sysfs_create_group(bdi);
>>> -       if (ret) {
>>> +       if (ret < 0) {
>>>                  dev_err(dev, "Can't create sysfs entries\n");
>>>                  goto out_charger;
>>>          }
>>> @@ -1700,7 +1723,7 @@ static __maybe_unused int bq24190_pm_resume(struct
>>> device *dev)
>>>          }
>>>          bq24190_register_reset(bdi);
>>> -       bq24190_set_mode_host(bdi);
>>> +       bq24190_set_config(bdi);
>>>          bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
>>>          if (error >= 0) {
>>>
>>
>> Regards,
>>
>> Hans

[PATCH v3 4/5] power: bq24190_charger: Add property system-minimum-microvolt

[Liam Breck]

From: Liam Breck <kernel@networkimprov.net>

Set minimum system voltage limit obtained from device property.

Cc: Tony Lindgren <tony@atomide.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Liam Breck <kernel@networkimprov.net>
---
 drivers/power/supply/bq24190_charger.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index c6be00d7..450f3ab3 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -43,6 +43,8 @@
 #define BQ24190_REG_POC_CHG_CONFIG_OTG			0x2
 #define BQ24190_REG_POC_SYS_MIN_MASK		(BIT(3) | BIT(2) | BIT(1))
 #define BQ24190_REG_POC_SYS_MIN_SHIFT		1
+#define BQ24190_REG_POC_SYS_MIN_MIN			3000
+#define BQ24190_REG_POC_SYS_MIN_MAX			3700
 #define BQ24190_REG_POC_BOOST_LIM_MASK		BIT(0)
 #define BQ24190_REG_POC_BOOST_LIM_SHIFT		0
 
@@ -159,6 +161,7 @@ struct bq24190_dev_info {
 	char				model_name[I2C_NAME_SIZE];
 	bool				initialized;
 	bool				irq_event;
+	u16				sys_min;
 	struct mutex			f_reg_lock;
 	u8				f_reg;
 	u8				ss_reg;
@@ -530,6 +533,16 @@ static int bq24190_set_config(struct bq24190_dev_info *bdi)
 	if (ret < 0)
 		return ret;
 
+	if (bdi->sys_min) {
+		v = bdi->sys_min / 100 - 30; // manual section 9.5.1.2, table 9
+		ret = bq24190_write_mask(bdi, BQ24190_REG_POC,
+					 BQ24190_REG_POC_SYS_MIN_MASK,
+					 BQ24190_REG_POC_SYS_MIN_SHIFT,
+					 v);
+		if (ret < 0)
+			return ret;
+	}
+
 	return 0;
 }
 
@@ -1471,11 +1484,21 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
 static int bq24190_get_config(struct bq24190_dev_info *bdi)
 {
 #ifdef CONFIG_OF
+	const char * const s = "ti,system-minimum-microvolt";
 	int v;
 
 	if (!bdi->dev->of_node)
 		return -EINVAL;
 
+	if (device_property_read_u32(bdi->dev, s, &v) == 0) {
+		v /= 1000;
+		if (v >= BQ24190_REG_POC_SYS_MIN_MIN
+		 && v <= BQ24190_REG_POC_SYS_MIN_MAX)
+			bdi->sys_min = v;
+		else
+			dev_warn(bdi->dev, "invalid value for %s: %u\n", s, v);
+	}
+
 #endif
 	return 0;
 }
-- 
2.13.2

Re: [PATCH v3 4/5] power: bq24190_charger: Add property system-minimum-microvolt

[Tony Lindgren]

* Liam Breck <liam@networkimprov.net> [170825 22:15]:
> From: Liam Breck <kernel@networkimprov.net>
> 
> Set minimum system voltage limit obtained from device property.
> 
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Liam Breck <kernel@networkimprov.net>

Acked-by: Tony Lindgren <tony@atomide.com>

[PATCH v3 5/5] power: bq24190_charger: Add power_supply_battery_info support

[Liam Breck]

From: Liam Breck <kernel@networkimprov.net>

Set pre-charge and charge-term current, obtained from power_supply_battery_info.
Add sysfs attributes precharge_current & charge_term_current.

Cc: Tony Lindgren <tony@atomide.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Liam Breck <kernel@networkimprov.net>
---
 drivers/power/supply/bq24190_charger.c | 86 ++++++++++++++++++++++++++++++++++
 1 file changed, 86 insertions(+)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index 450f3ab3..7681a67a 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -59,9 +59,13 @@
 #define BQ24190_REG_PCTCC_IPRECHG_MASK		(BIT(7) | BIT(6) | BIT(5) | \
 						 BIT(4))
 #define BQ24190_REG_PCTCC_IPRECHG_SHIFT		4
+#define BQ24190_REG_PCTCC_IPRECHG_MIN			128
+#define BQ24190_REG_PCTCC_IPRECHG_MAX			2048
 #define BQ24190_REG_PCTCC_ITERM_MASK		(BIT(3) | BIT(2) | BIT(1) | \
 						 BIT(0))
 #define BQ24190_REG_PCTCC_ITERM_SHIFT		0
+#define BQ24190_REG_PCTCC_ITERM_MIN			128
+#define BQ24190_REG_PCTCC_ITERM_MAX			2048
 
 #define BQ24190_REG_CVC		0x04 /* Charge Voltage Control */
 #define BQ24190_REG_CVC_VREG_MASK		(BIT(7) | BIT(6) | BIT(5) | \
@@ -162,6 +166,8 @@ struct bq24190_dev_info {
 	bool				initialized;
 	bool				irq_event;
 	u16				sys_min;
+	u16				iprechg;
+	u16				iterm;
 	struct mutex			f_reg_lock;
 	u8				f_reg;
 	u8				ss_reg;
@@ -543,6 +549,26 @@ static int bq24190_set_config(struct bq24190_dev_info *bdi)
 			return ret;
 	}
 
+	if (bdi->iprechg) {
+		v = bdi->iprechg / 128 - 1; // manual section 9.5.1.4, table 11
+		ret = bq24190_write_mask(bdi, BQ24190_REG_PCTCC,
+					 BQ24190_REG_PCTCC_IPRECHG_MASK,
+					 BQ24190_REG_PCTCC_IPRECHG_SHIFT,
+					 v);
+		if (ret < 0)
+			return ret;
+	}
+
+	if (bdi->iterm) {
+		v = bdi->iterm / 128 - 1; // manual section 9.5.1.4, table 11
+		ret = bq24190_write_mask(bdi, BQ24190_REG_PCTCC,
+					 BQ24190_REG_PCTCC_ITERM_MASK,
+					 BQ24190_REG_PCTCC_ITERM_SHIFT,
+					 v);
+		if (ret < 0)
+			return ret;
+	}
+
 	return 0;
 }
 
@@ -787,6 +813,38 @@ static int bq24190_charger_set_temp_alert_max(struct bq24190_dev_info *bdi,
 	return bq24190_battery_set_temp_alert_max(bdi, val);
 }
 
+static int bq24190_charger_get_precharge(struct bq24190_dev_info *bdi,
+		union power_supply_propval *val)
+{
+	u8 v;
+	int ret;
+
+	ret = bq24190_read_mask(bdi, BQ24190_REG_PCTCC,
+			BQ24190_REG_PCTCC_IPRECHG_MASK,
+			BQ24190_REG_PCTCC_IPRECHG_SHIFT, &v);
+	if (ret < 0)
+		return ret;
+
+	val->intval = ++v * 128 * 1000;
+	return 0;
+}
+
+static int bq24190_charger_get_charge_term(struct bq24190_dev_info *bdi,
+		union power_supply_propval *val)
+{
+	u8 v;
+	int ret;
+
+	ret = bq24190_read_mask(bdi, BQ24190_REG_PCTCC,
+			BQ24190_REG_PCTCC_ITERM_MASK,
+			BQ24190_REG_PCTCC_ITERM_SHIFT, &v);
+	if (ret < 0)
+		return ret;
+
+	val->intval = ++v * 128 * 1000;
+	return 0;
+}
+
 static int bq24190_charger_get_current(struct bq24190_dev_info *bdi,
 		union power_supply_propval *val)
 {
@@ -907,6 +965,12 @@ static int bq24190_charger_get_property(struct power_supply *psy,
 	case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
 		ret =  bq24190_charger_get_temp_alert_max(bdi, val);
 		break;
+	case POWER_SUPPLY_PROP_PRECHARGE_CURRENT:
+		ret = bq24190_charger_get_precharge(bdi, val);
+		break;
+	case POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT:
+		ret = bq24190_charger_get_charge_term(bdi, val);
+		break;
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
 		ret = bq24190_charger_get_current(bdi, val);
 		break;
@@ -1006,6 +1070,8 @@ static enum power_supply_property bq24190_charger_properties[] = {
 	POWER_SUPPLY_PROP_ONLINE,
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_TEMP_ALERT_MAX,
+	POWER_SUPPLY_PROP_PRECHARGE_CURRENT,
+	POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT,
 	POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT,
 	POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
 	POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE,
@@ -1485,6 +1551,7 @@ static int bq24190_get_config(struct bq24190_dev_info *bdi)
 {
 #ifdef CONFIG_OF
 	const char * const s = "ti,system-minimum-microvolt";
+	struct power_supply_battery_info info = {};
 	int v;
 
 	if (!bdi->dev->of_node)
@@ -1499,6 +1566,24 @@ static int bq24190_get_config(struct bq24190_dev_info *bdi)
 			dev_warn(bdi->dev, "invalid value for %s: %u\n", s, v);
 	}
 
+	if (!power_supply_get_battery_info(bdi->charger, &info)) {
+		v = info.precharge_current_ua / 1000;
+		if (v >= BQ24190_REG_PCTCC_IPRECHG_MIN
+		 && v <= BQ24190_REG_PCTCC_IPRECHG_MAX)
+			bdi->iprechg = v;
+		else
+			dev_warn(bdi->dev, "invalid value for battery:precharge-current-microamp: %d\n",
+				 v);
+
+		v = info.charge_term_current_ua / 1000;
+		if (v >= BQ24190_REG_PCTCC_ITERM_MIN
+		 && v <= BQ24190_REG_PCTCC_ITERM_MAX)
+			bdi->iterm = v;
+		else
+			dev_warn(bdi->dev, "invalid value for battery:charge-term-current-microamp: %d\n",
+				 v);
+	}
+
 #endif
 	return 0;
 }
@@ -1567,6 +1652,7 @@ static int bq24190_probe(struct i2c_client *client,
 	}
 
 	charger_cfg.drv_data = bdi;
+	charger_cfg.of_node = dev->of_node;
 	charger_cfg.supplied_to = bq24190_charger_supplied_to;
 	charger_cfg.num_supplicants = ARRAY_SIZE(bq24190_charger_supplied_to),
 	bdi->charger = power_supply_register(dev, &bq24190_charger_desc,
-- 
2.13.2

Re: [PATCH v3 5/5] power: bq24190_charger: Add power_supply_battery_info support

[Tony Lindgren]

* Liam Breck <liam@networkimprov.net> [170825 22:15]:
> From: Liam Breck <kernel@networkimprov.net>
> 
> Set pre-charge and charge-term current, obtained from power_supply_battery_info.
> Add sysfs attributes precharge_current & charge_term_current.
> 
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Liam Breck <kernel@networkimprov.net>

Acked-by: Tony Lindgren <tony@atomide.com>