[PATCH v1 0/7] BQ24190 support for power_supply_battery_info and DT binding

[PATCH v1 0/7] BQ24190 support for power_supply_battery_info and DT binding

[Liam Breck]

Overview:
BQ24190 needs settings for pre-charge & end-charge current and system min voltage to function 
correctly. Pre- and end-charge current belong in power_supply_battery_info as they are based on 
battery capacity. We add _PRECHARGE & _ENDCHARGE elements to enum power_supply_property.

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 (7):
  devicetree: power: battery: Add properties for pre-charge and end-charge current
  devicetree: power: Add docs for TI BQ24190 battery charger
  power: power_supply_core: Add *_battery_info fields for pre- and end-charge current
  power: bq24190_charger: Uniform pm_runtime_get() failure handling
  power: bq24190_charger: Reorder init sequence in probe()
  power: bq24190_charger: Add power_supply_battery_info and devicetree support
  power: bq24190_charger: Set bq24190-battery device .type=unknown

 .../devicetree/bindings/power/supply/battery.txt   |   4 +
 .../devicetree/bindings/power/supply/bq24190.txt   |  47 +++++++
 drivers/power/supply/bq24190_charger.c             | 152 ++++++++++++++++-----
 drivers/power/supply/power_supply_core.c           |   6 +
 include/linux/power_supply.h                       |   5 +
 5 files changed, 179 insertions(+), 35 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/power/supply/bq24190.txt

-- 
2.9.3

[PATCH v1 1/7] devicetree: power: battery: Add properties for pre-charge and end-charge current

[Liam Breck]

From: Liam Breck <kernel@networkimprov.net>

precharge-current-microamp and endcharge-current-microamp are used
by battery chargers at the beginning and end of a charging cycle.

Depends-on: https://patchwork.kernel.org/patch/9633605/
Cc: Rob Herring <robh@kernel.org>
Cc: devicetree@vger.kernel.org
Signed-off-by: Liam Breck <kernel@networkimprov.net>
---
 Documentation/devicetree/bindings/power/supply/battery.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/supply/battery.txt b/Documentation/devicetree/bindings/power/supply/battery.txt
index 53a68c0..494374a 100644
--- a/Documentation/devicetree/bindings/power/supply/battery.txt
+++ b/Documentation/devicetree/bindings/power/supply/battery.txt
@@ -12,6 +12,8 @@ Optional Properties:
  - voltage-min-design-microvolt: drained battery voltage
  - energy-full-design-microwatt-hours: battery design energy
  - charge-full-design-microamp-hours: battery design capacity
+ - precharge-current-microamp: current for pre-charge phase
+ - endcharge-current-microamp: current for charge termination phase
 
 Battery properties are named, where possible, for the corresponding 
 elements in enum power_supply_property, defined in
@@ -28,6 +30,8 @@ Example:
 		voltage-min-design-microvolt = <3200000>;
 		energy-full-design-microwatt-hours = <5290000>;
 		charge-full-design-microamp-hours = <1430000>;
+		precharge-current-microamp = <256000>;
+		endcharge-current-microamp = <128000>;
 	};
 
 	charger: charger@11 {
-- 
2.9.3

Re: [PATCH v1 1/7] devicetree: power: battery: Add properties for pre-charge and end-charge current

[Sebastian Reichel]

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

Hi,

On Tue, Mar 21, 2017 at 03:09:15PM -0700, Liam Breck wrote:
> From: Liam Breck <kernel@networkimprov.net>
> 
> precharge-current-microamp and endcharge-current-microamp are used
> by battery chargers at the beginning and end of a charging cycle.
> 
> Depends-on: https://patchwork.kernel.org/patch/9633605/
> Cc: Rob Herring <robh@kernel.org>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Liam Breck <kernel@networkimprov.net>

Acked-by: Sebastian Reichel <sre@kernel.org>

I think it makes sense to merge this into the patch adding
simple-battery.

>  Documentation/devicetree/bindings/power/supply/battery.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/power/supply/battery.txt b/Documentation/devicetree/bindings/power/supply/battery.txt
> index 53a68c0..494374a 100644
> --- a/Documentation/devicetree/bindings/power/supply/battery.txt
> +++ b/Documentation/devicetree/bindings/power/supply/battery.txt
> @@ -12,6 +12,8 @@ Optional Properties:
>   - voltage-min-design-microvolt: drained battery voltage
>   - energy-full-design-microwatt-hours: battery design energy
>   - charge-full-design-microamp-hours: battery design capacity
> + - precharge-current-microamp: current for pre-charge phase
> + - endcharge-current-microamp: current for charge termination phase
>  
>  Battery properties are named, where possible, for the corresponding 
>  elements in enum power_supply_property, defined in
> @@ -28,6 +30,8 @@ Example:
>  		voltage-min-design-microvolt = <3200000>;
>  		energy-full-design-microwatt-hours = <5290000>;
>  		charge-full-design-microamp-hours = <1430000>;
> +		precharge-current-microamp = <256000>;
> +		endcharge-current-microamp = <128000>;
>  	};
>  
>  	charger: charger@11 {
> -- 
> 2.9.3
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 1/7] devicetree: power: battery: Add properties for pre-charge and end-charge current

[Liam Breck]

On Fri, Mar 24, 2017 at 2:01 AM, Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> Hi,
>
> On Tue, Mar 21, 2017 at 03:09:15PM -0700, Liam Breck wrote:
>> From: Liam Breck <kernel-RYWXG+zxWwBdeoIcmNTgJF6hYfS7NtTn@public.gmane.org>
>>
>> precharge-current-microamp and endcharge-current-microamp are used
>> by battery chargers at the beginning and end of a charging cycle.
>>
>> Depends-on: https://patchwork.kernel.org/patch/9633605/
>> 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>
>
> Acked-by: Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>
> I think it makes sense to merge this into the patch adding
> simple-battery.

It would make sense, but it means a new _prop_precharge/endcharge
patch in that patchset, and I am trying to constrain it at this stage.

So if it's OK, I'd like to keep all that in this patchset.

>>  Documentation/devicetree/bindings/power/supply/battery.txt | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/power/supply/battery.txt b/Documentation/devicetree/bindings/power/supply/battery.txt
>> index 53a68c0..494374a 100644
>> --- a/Documentation/devicetree/bindings/power/supply/battery.txt
>> +++ b/Documentation/devicetree/bindings/power/supply/battery.txt
>> @@ -12,6 +12,8 @@ Optional Properties:
>>   - voltage-min-design-microvolt: drained battery voltage
>>   - energy-full-design-microwatt-hours: battery design energy
>>   - charge-full-design-microamp-hours: battery design capacity
>> + - precharge-current-microamp: current for pre-charge phase
>> + - endcharge-current-microamp: current for charge termination phase
>>
>>  Battery properties are named, where possible, for the corresponding
>>  elements in enum power_supply_property, defined in
>> @@ -28,6 +30,8 @@ Example:
>>               voltage-min-design-microvolt = <3200000>;
>>               energy-full-design-microwatt-hours = <5290000>;
>>               charge-full-design-microamp-hours = <1430000>;
>> +             precharge-current-microamp = <256000>;
>> +             endcharge-current-microamp = <128000>;
>>       };
>>
>>       charger: charger@11 {
>> --
>> 2.9.3
>>
--
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 v1 1/7] devicetree: power: battery: Add properties for pre-charge and end-charge current

[Rob Herring]

On Fri, Mar 24, 2017 at 05:34:26PM -0700, Liam Breck wrote:
> On Fri, Mar 24, 2017 at 2:01 AM, Sebastian Reichel <sre@kernel.org> wrote:
> > Hi,
> >
> > On Tue, Mar 21, 2017 at 03:09:15PM -0700, Liam Breck wrote:
> >> From: Liam Breck <kernel@networkimprov.net>
> >>
> >> precharge-current-microamp and endcharge-current-microamp are used
> >> by battery chargers at the beginning and end of a charging cycle.
> >>
> >> Depends-on: https://patchwork.kernel.org/patch/9633605/
> >> Cc: Rob Herring <robh@kernel.org>
> >> Cc: devicetree@vger.kernel.org
> >> Signed-off-by: Liam Breck <kernel@networkimprov.net>
> >
> > Acked-by: Sebastian Reichel <sre@kernel.org>
> >
> > I think it makes sense to merge this into the patch adding
> > simple-battery.

Agreed.

> 
> It would make sense, but it means a new _prop_precharge/endcharge
> patch in that patchset, and I am trying to constrain it at this stage.

Please make bindings complete as possible. You don't have to have the 
driver side.

Rob

Re: [PATCH v1 1/7] devicetree: power: battery: Add properties for pre-charge and end-charge current

[Hans de Goede]

Hi,

On 21-03-17 23:09, Liam Breck wrote:
> From: Liam Breck <kernel-RYWXG+zxWwBdeoIcmNTgJF6hYfS7NtTn@public.gmane.org>
>
> precharge-current-microamp and endcharge-current-microamp are used
> by battery chargers at the beginning and end of a charging cycle.
>
> Depends-on: https://patchwork.kernel.org/patch/9633605/
> 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>

Looks good to me:

Reviewed-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Regards,

Hans


> ---
>  Documentation/devicetree/bindings/power/supply/battery.txt | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/power/supply/battery.txt b/Documentation/devicetree/bindings/power/supply/battery.txt
> index 53a68c0..494374a 100644
> --- a/Documentation/devicetree/bindings/power/supply/battery.txt
> +++ b/Documentation/devicetree/bindings/power/supply/battery.txt
> @@ -12,6 +12,8 @@ Optional Properties:
>   - voltage-min-design-microvolt: drained battery voltage
>   - energy-full-design-microwatt-hours: battery design energy
>   - charge-full-design-microamp-hours: battery design capacity
> + - precharge-current-microamp: current for pre-charge phase
> + - endcharge-current-microamp: current for charge termination phase
>
>  Battery properties are named, where possible, for the corresponding
>  elements in enum power_supply_property, defined in
> @@ -28,6 +30,8 @@ Example:
>  		voltage-min-design-microvolt = <3200000>;
>  		energy-full-design-microwatt-hours = <5290000>;
>  		charge-full-design-microamp-hours = <1430000>;
> +		precharge-current-microamp = <256000>;
> +		endcharge-current-microamp = <128000>;
>  	};
>
>  	charger: charger@11 {
>
--
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 v1 1/7] devicetree: power: battery: Add properties for pre-charge and end-charge current

[Rob Herring]

On Tue, Mar 21, 2017 at 03:09:15PM -0700, Liam Breck wrote:
> From: Liam Breck <kernel-RYWXG+zxWwBdeoIcmNTgJF6hYfS7NtTn@public.gmane.org>
> 
> precharge-current-microamp and endcharge-current-microamp are used
> by battery chargers at the beginning and end of a charging cycle.
> 
> Depends-on: https://patchwork.kernel.org/patch/9633605/
> 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>
> ---
>  Documentation/devicetree/bindings/power/supply/battery.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/power/supply/battery.txt b/Documentation/devicetree/bindings/power/supply/battery.txt
> index 53a68c0..494374a 100644
> --- a/Documentation/devicetree/bindings/power/supply/battery.txt
> +++ b/Documentation/devicetree/bindings/power/supply/battery.txt
> @@ -12,6 +12,8 @@ Optional Properties:
>   - voltage-min-design-microvolt: drained battery voltage
>   - energy-full-design-microwatt-hours: battery design energy
>   - charge-full-design-microamp-hours: battery design capacity
> + - precharge-current-microamp: current for pre-charge phase
> + - endcharge-current-microamp: current for charge termination phase

current is implied by microamp, so perhaps just pre-charge-microamp and 
end-charge-microamp.

I know little about batteries, but don't you also need to know when each 
phase starts/ends? 

I mainly ask because we just added the previous properties and now we're 
adding 2 more. While fine to add features to a driver one by one, we 
really shouldn't for bindings. The h/w is not evolving (in a month).

Rob
--
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 v1 1/7] devicetree: power: battery: Add properties for pre-charge and end-charge current

[Liam Breck]

Hi Rob,

On Tue, Mar 28, 2017 at 5:39 PM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Tue, Mar 21, 2017 at 03:09:15PM -0700, Liam Breck wrote:
>> From: Liam Breck <kernel-RYWXG+zxWwBdeoIcmNTgJF6hYfS7NtTn@public.gmane.org>
>>
>> precharge-current-microamp and endcharge-current-microamp are used
>> by battery chargers at the beginning and end of a charging cycle.
>>
>> Depends-on: https://patchwork.kernel.org/patch/9633605/
>> 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>
>> ---
>>  Documentation/devicetree/bindings/power/supply/battery.txt | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/power/supply/battery.txt b/Documentation/devicetree/bindings/power/supply/battery.txt
>> index 53a68c0..494374a 100644
>> --- a/Documentation/devicetree/bindings/power/supply/battery.txt
>> +++ b/Documentation/devicetree/bindings/power/supply/battery.txt
>> @@ -12,6 +12,8 @@ Optional Properties:
>>   - voltage-min-design-microvolt: drained battery voltage
>>   - energy-full-design-microwatt-hours: battery design energy
>>   - charge-full-design-microamp-hours: battery design capacity
>> + - precharge-current-microamp: current for pre-charge phase
>> + - endcharge-current-microamp: current for charge termination phase
>
> current is implied by microamp, so perhaps just pre-charge-microamp and
> end-charge-microamp.

Ah, this is why I want to document the naming scheme for battery node
properties in battery.txt, by referring to a linux header file :-)

The names mirror enum power_supply_property elements wherever
possible. Shortening them as you suggest makes dts code a little more
terse, but obscures their relationship with power_supply sysfs
attributes. I prefer brevity myself, but there is no strong case to
reword the names for DT use.

> I know little about batteries, but don't you also need to know when each
> phase starts/ends?

Meaning at what voltage levels? We don't need it for this
battery/charger pair; no idea about others...

> I mainly ask because we just added the previous properties and now we're
> adding 2 more. While fine to add features to a driver one by one, we
> really shouldn't for bindings. The h/w is not evolving (in a month).

And a third patchset from Quentin Schulz adds another :-)

I think you may need to accept piecemeal assembly in this case... No
one has made a study of all properties that should be in the battery
node. (precharge_current wasn't even in power_supply_property until
this patchset.) Sebastian requested we create this binding in the
process of adding DT support to a fuel gauge, so we coded what that
called for.

I hope that helps...
--
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 v1 2/7] devicetree: power: Add docs for TI BQ24190 battery charger

[Liam Breck]

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>
---
 .../devicetree/bindings/power/supply/bq24190.txt   | 47 ++++++++++++++++++++++
 1 file changed, 47 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 0000000..d252d10
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/supply/bq24190.txt
@@ -0,0 +1,47 @@
+Binding for TI BQ24190 Li-Ion Battery Charger
+
+Required properties:
+- compatible: Should contain one of the following:
+    * "ti,bq24190"
+- reg: integer, I2C address of the device.
+
+Optional properties:
+- monitored-battery: phandle of battery information devicetree node
+    These battery properties are relevant:
+    + precharge-current-microamp: maximum charge current during precharge
+      phase (typically 20% of battery capacity).
+    + endcharge-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 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.
+
+Other features:
+- Use gpio-hog to set the OTG pin high to enable 500mA charge current on USB SDP port.
+
+Example:
+
+bat: battery {
+	compatible = "simple-battery";
+	precharge-current-microamp = <256000>;
+	endcharge-current-microamp = <128000>;
+};
+
+bq24190 charger@6a {
+	compatible = "ti,bq24190";
+	reg = <0x6a>;
+	// interrupt configuration here
+	monitored-battery = <&bat>;
+	ti,system-minimum-microvolt = <3200000>;
+};
+
+&twl_gpio {
+	otg {
+		gpio-hog;
+		gpios = <6 0>;
+		output-high;
+		line-name = "otg-gpio";
+	};
+};
-- 
2.9.3

Re: [PATCH v1 2/7] devicetree: power: Add docs for TI BQ24190 battery charger

[Sebastian Reichel]

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

Hi,

The interrupt must be documented, since its mandatory.

On Tue, Mar 21, 2017 at 03:09:16PM -0700, Liam Breck wrote:
> 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>
> ---
>  .../devicetree/bindings/power/supply/bq24190.txt   | 47 ++++++++++++++++++++++
>  1 file changed, 47 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 0000000..d252d10
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/bq24190.txt
> @@ -0,0 +1,47 @@
> +Binding for TI BQ24190 Li-Ion Battery Charger
> +
> +Required properties:
> +- compatible: Should contain one of the following:
> +    * "ti,bq24190"
> +- reg: integer, I2C address of the device.

interrupts: interrupt specifier for bq24190's INT

> +
> +Optional properties:
> +- monitored-battery: phandle of battery information devicetree node
> +    These battery properties are relevant:
> +    + precharge-current-microamp: maximum charge current during precharge
> +      phase (typically 20% of battery capacity).
> +    + endcharge-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 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.
> +
> +Other features:
> +- Use gpio-hog to set the OTG pin high to enable 500mA charge current on USB SDP port.
> +
> +Example:
> +
> +bat: battery {
> +	compatible = "simple-battery";
> +	precharge-current-microamp = <256000>;
> +	endcharge-current-microamp = <128000>;
> +};
> +
> +bq24190 charger@6a {
> +	compatible = "ti,bq24190";
> +	reg = <0x6a>;
> +	// interrupt configuration here

interrupt-parent = <&gpiochip23>;
interrupts = <42 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.9.3
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 2/7] devicetree: power: Add docs for TI BQ24190 battery charger

[Hans de Goede]

Hi,

On 21-03-17 23:09, Liam Breck wrote:
> 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>

Looks good to me:

Reviewed-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Regards,

Hans



> ---
>  .../devicetree/bindings/power/supply/bq24190.txt   | 47 ++++++++++++++++++++++
>  1 file changed, 47 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 0000000..d252d10
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/bq24190.txt
> @@ -0,0 +1,47 @@
> +Binding for TI BQ24190 Li-Ion Battery Charger
> +
> +Required properties:
> +- compatible: Should contain one of the following:
> +    * "ti,bq24190"
> +- reg: integer, I2C address of the device.
> +
> +Optional properties:
> +- monitored-battery: phandle of battery information devicetree node
> +    These battery properties are relevant:
> +    + precharge-current-microamp: maximum charge current during precharge
> +      phase (typically 20% of battery capacity).
> +    + endcharge-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 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.
> +
> +Other features:
> +- Use gpio-hog to set the OTG pin high to enable 500mA charge current on USB SDP port.
> +
> +Example:
> +
> +bat: battery {
> +	compatible = "simple-battery";
> +	precharge-current-microamp = <256000>;
> +	endcharge-current-microamp = <128000>;
> +};
> +
> +bq24190 charger@6a {
> +	compatible = "ti,bq24190";
> +	reg = <0x6a>;
> +	// interrupt configuration here
> +	monitored-battery = <&bat>;
> +	ti,system-minimum-microvolt = <3200000>;
> +};
> +
> +&twl_gpio {
> +	otg {
> +		gpio-hog;
> +		gpios = <6 0>;
> +		output-high;
> +		line-name = "otg-gpio";
> +	};
> +};
>
--
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 v1 2/7] devicetree: power: Add docs for TI BQ24190 battery charger

[Rob Herring]

On Tue, Mar 21, 2017 at 03:09:16PM -0700, Liam Breck wrote:
> 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   | 47 ++++++++++++++++++++++
>  1 file changed, 47 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 0000000..d252d10
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/bq24190.txt
> @@ -0,0 +1,47 @@
> +Binding for TI BQ24190 Li-Ion Battery Charger
> +
> +Required properties:
> +- compatible: Should contain one of the following:
> +    * "ti,bq24190"
> +- reg: integer, I2C address of the device.
> +
> +Optional properties:
> +- monitored-battery: phandle of battery information devicetree node
> +    These battery properties are relevant:
> +    + precharge-current-microamp: maximum charge current during precharge
> +      phase (typically 20% of battery capacity).
> +    + endcharge-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 Documentation/devicetree/bindings/power/supply/battery.txt

This isn't really relevant to the binding. The battery properties 
shouldn't vary with the charger.

> +- ti,system-minimum-microvolt: when power is connected and the battery
> +    is below minimum system voltage, the system will be regulated above this
> +    setting.
> +
> +Other features:
> +- Use gpio-hog to set the OTG pin high to enable 500mA charge current on USB SDP port.
> +
> +Example:
> +
> +bat: battery {
> +	compatible = "simple-battery";
> +	precharge-current-microamp = <256000>;
> +	endcharge-current-microamp = <128000>;
> +};
> +
> +bq24190 charger@6a {
> +	compatible = "ti,bq24190";
> +	reg = <0x6a>;
> +	// interrupt configuration here
> +	monitored-battery = <&bat>;
> +	ti,system-minimum-microvolt = <3200000>;
> +};
> +
> +&twl_gpio {
> +	otg {
> +		gpio-hog;
> +		gpios = <6 0>;
> +		output-high;
> +		line-name = "otg-gpio";
> +	};
> +};
> -- 
> 2.9.3
> 
--
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 v1 2/7] devicetree: power: Add docs for TI BQ24190 battery charger

[Liam Breck]

Hi Rob,

On Tue, Mar 28, 2017 at 5:47 PM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Tue, Mar 21, 2017 at 03:09:16PM -0700, Liam Breck wrote:
>> 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   | 47 ++++++++++++++++++++++
>>  1 file changed, 47 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 0000000..d252d10
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/supply/bq24190.txt
>> @@ -0,0 +1,47 @@
>> +Binding for TI BQ24190 Li-Ion Battery Charger
>> +
>> +Required properties:
>> +- compatible: Should contain one of the following:
>> +    * "ti,bq24190"
>> +- reg: integer, I2C address of the device.
>> +
>> +Optional properties:
>> +- monitored-battery: phandle of battery information devicetree node
>> +    These battery properties are relevant:
>> +    + precharge-current-microamp: maximum charge current during precharge
>> +      phase (typically 20% of battery capacity).
>> +    + endcharge-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 Documentation/devicetree/bindings/power/supply/battery.txt
>
> This isn't really relevant to the binding. The battery properties
> shouldn't vary with the charger.

Different components need different properties from the battery node.
This charger needs the above two, so we should document that, no?

>> +- ti,system-minimum-microvolt: when power is connected and the battery
>> +    is below minimum system voltage, the system will be regulated above this
>> +    setting.
>> +
>> +Other features:
>> +- Use gpio-hog to set the OTG pin high to enable 500mA charge current on USB SDP port.
>> +
>> +Example:
>> +
>> +bat: battery {
>> +     compatible = "simple-battery";
>> +     precharge-current-microamp = <256000>;
>> +     endcharge-current-microamp = <128000>;
>> +};
>> +
>> +bq24190 charger@6a {
>> +     compatible = "ti,bq24190";
>> +     reg = <0x6a>;
>> +     // interrupt configuration here
>> +     monitored-battery = <&bat>;
>> +     ti,system-minimum-microvolt = <3200000>;
>> +};
>> +
>> +&twl_gpio {
>> +     otg {
>> +             gpio-hog;
>> +             gpios = <6 0>;
>> +             output-high;
>> +             line-name = "otg-gpio";
>> +     };
>> +};
>> --
>> 2.9.3
>>
--
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 v1 3/7] power: power_supply_core: Add *_battery_info fields for pre- and end-charge current

[Liam Breck]

From: Liam Breck <kernel@networkimprov.net>

Battery chargers use precharge_current_ua and endcharge_current_ua
at the beginning and end of a charging cycle. These values are set
according to battery capacity.

Depends-on: https://patchwork.kernel.org/patch/9633613/
Signed-off-by: Liam Breck <kernel@networkimprov.net>
---
 drivers/power/supply/power_supply_core.c | 6 ++++++
 include/linux/power_supply.h             | 5 +++++
 2 files changed, 11 insertions(+)

diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index 61e20b1..17088c5 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -498,6 +498,8 @@ int power_supply_get_battery_info(struct power_supply *psy,
 	info->energy_full_design_uwh = -EINVAL;
 	info->charge_full_design_uah = -EINVAL;
 	info->voltage_min_design_uv  = -EINVAL;
+	info->precharge_current_ua   = -EINVAL;
+	info->endcharge_current_ua   = -EINVAL;
 
 	if (!psy->of_node) {
 		dev_warn(&psy->dev, "%s currently only supports devicetree\n",
@@ -527,6 +529,10 @@ int power_supply_get_battery_info(struct power_supply *psy,
 			     &info->charge_full_design_uah);
 	of_property_read_u32(battery_np, "voltage-min-design-microvolt",
 			     &info->voltage_min_design_uv);
+	of_property_read_u32(battery_np, "precharge-current-microamp",
+			     &info->precharge_current_ua);
+	of_property_read_u32(battery_np, "endcharge-current-microamp",
+			     &info->endcharge_current_ua);
 
 	return 0;
 }
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index e84f1d3..864214c 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -152,6 +152,9 @@ enum power_supply_property {
 	POWER_SUPPLY_PROP_MODEL_NAME,
 	POWER_SUPPLY_PROP_MANUFACTURER,
 	POWER_SUPPLY_PROP_SERIAL_NUMBER,
+/* where to place these? inserting above causes sysfs errors */
+	POWER_SUPPLY_PROP_PRECHARGE_CURRENT,
+	POWER_SUPPLY_PROP_ENDCHARGE_CURRENT,
 };
 
 enum power_supply_type {
@@ -301,6 +304,8 @@ struct power_supply_battery_info {
 	int energy_full_design_uwh;	/* microWatt-hours */
 	int charge_full_design_uah;	/* microAmp-hours */
 	int voltage_min_design_uv;	/* microVolts */
+	int precharge_current_ua;	/* microAmps */
+	int endcharge_current_ua;       /* microAmps */
 };
 
 extern struct atomic_notifier_head power_supply_notifier;
-- 
2.9.3

Re: [PATCH v1 3/7] power: power_supply_core: Add *_battery_info fields for pre- and end-charge current

[Hans de Goede]

Hi,

On 21-03-17 23:09, Liam Breck wrote:
> From: Liam Breck <kernel@networkimprov.net>
>
> Battery chargers use precharge_current_ua and endcharge_current_ua
> at the beginning and end of a charging cycle. These values are set
> according to battery capacity.
>
> Depends-on: https://patchwork.kernel.org/patch/9633613/
> Signed-off-by: Liam Breck <kernel@networkimprov.net>
> ---
>  drivers/power/supply/power_supply_core.c | 6 ++++++
>  include/linux/power_supply.h             | 5 +++++
>  2 files changed, 11 insertions(+)
>
> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
> index 61e20b1..17088c5 100644
> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -498,6 +498,8 @@ int power_supply_get_battery_info(struct power_supply *psy,
>  	info->energy_full_design_uwh = -EINVAL;
>  	info->charge_full_design_uah = -EINVAL;
>  	info->voltage_min_design_uv  = -EINVAL;
> +	info->precharge_current_ua   = -EINVAL;
> +	info->endcharge_current_ua   = -EINVAL;
>
>  	if (!psy->of_node) {
>  		dev_warn(&psy->dev, "%s currently only supports devicetree\n",
> @@ -527,6 +529,10 @@ int power_supply_get_battery_info(struct power_supply *psy,
>  			     &info->charge_full_design_uah);
>  	of_property_read_u32(battery_np, "voltage-min-design-microvolt",
>  			     &info->voltage_min_design_uv);
> +	of_property_read_u32(battery_np, "precharge-current-microamp",
> +			     &info->precharge_current_ua);
> +	of_property_read_u32(battery_np, "endcharge-current-microamp",
> +			     &info->endcharge_current_ua);
>
>  	return 0;
>  }
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index e84f1d3..864214c 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -152,6 +152,9 @@ enum power_supply_property {
>  	POWER_SUPPLY_PROP_MODEL_NAME,
>  	POWER_SUPPLY_PROP_MANUFACTURER,
>  	POWER_SUPPLY_PROP_SERIAL_NUMBER,
> +/* where to place these? inserting above causes sysfs errors */
> +	POWER_SUPPLY_PROP_PRECHARGE_CURRENT,
> +	POWER_SUPPLY_PROP_ENDCHARGE_CURRENT,
>  };
>
>  enum power_supply_type {

Ok, I just checked this and power_supply_sysfs.c contains the following:

static ssize_t power_supply_show_property(struct device *dev,
                                           struct device_attribute *attr,
                                           char *buf) {
	<snip special handling of some options>

         else if (off >= POWER_SUPPLY_PROP_MODEL_NAME)
                 return sprintf(buf, "%s\n", value.strval);

	return sprintf(buf, "%d\n", value.intval);
}

So since the options you are adding are integer options they
should be above POWER_SUPPLY_PROP_MODEL_NAME, note there
are comments in the enum stating this:

enum power_supply_property {
         /* Properties of type `int' */
         POWER_SUPPLY_PROP_STATUS = 0,
<snip>
         /* Properties of type `const char *' */
         POWER_SUPPLY_PROP_MODEL_NAME,
         POWER_SUPPLY_PROP_MANUFACTURER,
         POWER_SUPPLY_PROP_SERIAL_NUMBER,
};

And you also need to update the power_supply_attrs[]
array in drivers/power/supply/power_supply_sysfs.c

I guess missing the latter is what was causing you to see
issues with the sorting order.

Regards,

Hans


> @@ -301,6 +304,8 @@ struct power_supply_battery_info {
>  	int energy_full_design_uwh;	/* microWatt-hours */
>  	int charge_full_design_uah;	/* microAmp-hours */
>  	int voltage_min_design_uv;	/* microVolts */
> +	int precharge_current_ua;	/* microAmps */
> +	int endcharge_current_ua;       /* microAmps */
>  };
>
>  extern struct atomic_notifier_head power_supply_notifier;
>

Re: [PATCH v1 3/7] power: power_supply_core: Add *_battery_info fields for pre- and end-charge current

[Sebastian Reichel]

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

Hi,

On Tue, Mar 21, 2017 at 03:09:17PM -0700, Liam Breck wrote:
> From: Liam Breck <kernel@networkimprov.net>
> 
> Battery chargers use precharge_current_ua and endcharge_current_ua
> at the beginning and end of a charging cycle. These values are set
> according to battery capacity.
> 
> Depends-on: https://patchwork.kernel.org/patch/9633613/
> Signed-off-by: Liam Breck <kernel@networkimprov.net>
> ---
>  drivers/power/supply/power_supply_core.c | 6 ++++++
>  include/linux/power_supply.h             | 5 +++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
> index 61e20b1..17088c5 100644
> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -498,6 +498,8 @@ int power_supply_get_battery_info(struct power_supply *psy,
>  	info->energy_full_design_uwh = -EINVAL;
>  	info->charge_full_design_uah = -EINVAL;
>  	info->voltage_min_design_uv  = -EINVAL;
> +	info->precharge_current_ua   = -EINVAL;
> +	info->endcharge_current_ua   = -EINVAL;
>  
>  	if (!psy->of_node) {
>  		dev_warn(&psy->dev, "%s currently only supports devicetree\n",
> @@ -527,6 +529,10 @@ int power_supply_get_battery_info(struct power_supply *psy,
>  			     &info->charge_full_design_uah);
>  	of_property_read_u32(battery_np, "voltage-min-design-microvolt",
>  			     &info->voltage_min_design_uv);
> +	of_property_read_u32(battery_np, "precharge-current-microamp",
> +			     &info->precharge_current_ua);
> +	of_property_read_u32(battery_np, "endcharge-current-microamp",
> +			     &info->endcharge_current_ua);
>  
>  	return 0;
>  }
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index e84f1d3..864214c 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -152,6 +152,9 @@ enum power_supply_property {
>  	POWER_SUPPLY_PROP_MODEL_NAME,
>  	POWER_SUPPLY_PROP_MANUFACTURER,
>  	POWER_SUPPLY_PROP_SERIAL_NUMBER,
> +/* where to place these? inserting above causes sysfs errors */
> +	POWER_SUPPLY_PROP_PRECHARGE_CURRENT,
> +	POWER_SUPPLY_PROP_ENDCHARGE_CURRENT,
>  };

Ack on adding standard property for charge current of pre & end
phase. Please do this in its own patch. This requires modification
of a few places, see for example the following commit adding
CHARGE_CURRENT_MAX: 2815b786c3bb86fff97f1f6e2f0874903ff2339b

>  enum power_supply_type {
> @@ -301,6 +304,8 @@ struct power_supply_battery_info {
>  	int energy_full_design_uwh;	/* microWatt-hours */
>  	int charge_full_design_uah;	/* microAmp-hours */
>  	int voltage_min_design_uv;	/* microVolts */
> +	int precharge_current_ua;	/* microAmps */
> +	int endcharge_current_ua;       /* microAmps */
>  };
>  
>  extern struct atomic_notifier_head power_supply_notifier;
> -- 
> 2.9.3
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v1 4/7] power: bq24190_charger: Uniform pm_runtime_get() failure handling

[Liam Breck]

From: Liam Breck <kernel@networkimprov.net>

On pm_runtime_get() failure, always emit error message,
and call pm_runtime_put_noidle() in irq handler.

Fixes: 13d6fa8447fa power: bq24190_charger: Use PM runtime autosuspend
Cc: Mark Greer <mgreer@animalcreek.com>
Signed-off-by: Liam Breck <kernel@networkimprov.net>
---
 drivers/power/supply/bq24190_charger.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index c443988..ad09e48 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -1267,12 +1267,13 @@ static void bq24190_check_status(struct bq24190_dev_info *bdi)
 static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
 {
 	struct bq24190_dev_info *bdi = data;
-	int ret;
+	int error;
 
 	bdi->irq_event = true;
-	ret = pm_runtime_get_sync(bdi->dev);
-	if (ret < 0) {
-		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", ret);
+	error = pm_runtime_get_sync(bdi->dev);
+	if (error < 0) {
+		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
+		pm_runtime_put_noidle(bdi->dev);
 		return IRQ_NONE;
 	}
 	bq24190_check_status(bdi);
@@ -1399,8 +1400,10 @@ static int bq24190_probe(struct i2c_client *client,
 	pm_runtime_use_autosuspend(dev);
 	pm_runtime_set_autosuspend_delay(dev, 600);
 	ret = pm_runtime_get_sync(dev);
-	if (ret < 0)
+	if (ret < 0) {
+		dev_err(dev, "pm_runtime_get failed: %i\n", ret);
 		goto out1;
+	}
 
 	ret = bq24190_hw_init(bdi);
 	if (ret < 0) {
-- 
2.9.3

Re: [PATCH v1 4/7] power: bq24190_charger: Uniform pm_runtime_get() failure handling

[Hans de Goede]

Hi,

On 21-03-17 23:09, Liam Breck wrote:
> From: Liam Breck <kernel@networkimprov.net>
>
> On pm_runtime_get() failure, always emit error message,
> and call pm_runtime_put_noidle() in irq handler.
>
> Fixes: 13d6fa8447fa power: bq24190_charger: Use PM runtime autosuspend
> Cc: Mark Greer <mgreer@animalcreek.com>
> Signed-off-by: Liam Breck <kernel@networkimprov.net>

Great I also noticed this needed fixing, but did not get
around to it:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

> ---
>  drivers/power/supply/bq24190_charger.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index c443988..ad09e48 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -1267,12 +1267,13 @@ static void bq24190_check_status(struct bq24190_dev_info *bdi)
>  static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
>  {
>  	struct bq24190_dev_info *bdi = data;
> -	int ret;
> +	int error;
>
>  	bdi->irq_event = true;
> -	ret = pm_runtime_get_sync(bdi->dev);
> -	if (ret < 0) {
> -		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", ret);
> +	error = pm_runtime_get_sync(bdi->dev);
> +	if (error < 0) {
> +		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
> +		pm_runtime_put_noidle(bdi->dev);
>  		return IRQ_NONE;
>  	}
>  	bq24190_check_status(bdi);
> @@ -1399,8 +1400,10 @@ static int bq24190_probe(struct i2c_client *client,
>  	pm_runtime_use_autosuspend(dev);
>  	pm_runtime_set_autosuspend_delay(dev, 600);
>  	ret = pm_runtime_get_sync(dev);
> -	if (ret < 0)
> +	if (ret < 0) {
> +		dev_err(dev, "pm_runtime_get failed: %i\n", ret);
>  		goto out1;
> +	}
>
>  	ret = bq24190_hw_init(bdi);
>  	if (ret < 0) {
>

Re: [PATCH v1 4/7] power: bq24190_charger: Uniform pm_runtime_get() failure handling

[Tony Lindgren]

* Liam Breck <liam@networkimprov.net> [170321 15:11]:
> From: Liam Breck <kernel@networkimprov.net>
> 
> On pm_runtime_get() failure, always emit error message,
> and call pm_runtime_put_noidle() in irq handler.
> 
> Fixes: 13d6fa8447fa power: bq24190_charger: Use PM runtime autosuspend

Not sure if this fixes anything? If it does the description
should say what it fixes.

Other than that, looks OK:

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

> Cc: Mark Greer <mgreer@animalcreek.com>
> Signed-off-by: Liam Breck <kernel@networkimprov.net>
> ---
>  drivers/power/supply/bq24190_charger.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index c443988..ad09e48 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -1267,12 +1267,13 @@ static void bq24190_check_status(struct bq24190_dev_info *bdi)
>  static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
>  {
>  	struct bq24190_dev_info *bdi = data;
> -	int ret;
> +	int error;
>  
>  	bdi->irq_event = true;
> -	ret = pm_runtime_get_sync(bdi->dev);
> -	if (ret < 0) {
> -		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", ret);
> +	error = pm_runtime_get_sync(bdi->dev);
> +	if (error < 0) {
> +		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
> +		pm_runtime_put_noidle(bdi->dev);
>  		return IRQ_NONE;
>  	}
>  	bq24190_check_status(bdi);
> @@ -1399,8 +1400,10 @@ static int bq24190_probe(struct i2c_client *client,
>  	pm_runtime_use_autosuspend(dev);
>  	pm_runtime_set_autosuspend_delay(dev, 600);
>  	ret = pm_runtime_get_sync(dev);
> -	if (ret < 0)
> +	if (ret < 0) {
> +		dev_err(dev, "pm_runtime_get failed: %i\n", ret);
>  		goto out1;
> +	}
>  
>  	ret = bq24190_hw_init(bdi);
>  	if (ret < 0) {
> -- 
> 2.9.3
> 

Re: [PATCH v1 4/7] power: bq24190_charger: Uniform pm_runtime_get() failure handling

[Sebastian Reichel]

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

Hi,

On Tue, Mar 21, 2017 at 03:09:18PM -0700, Liam Breck wrote:
> From: Liam Breck <kernel@networkimprov.net>
> 
> On pm_runtime_get() failure, always emit error message,
> and call pm_runtime_put_noidle() in irq handler.
> 
> Fixes: 13d6fa8447fa power: bq24190_charger: Use PM runtime autosuspend
> Cc: Mark Greer <mgreer@animalcreek.com>
> Signed-off-by: Liam Breck <kernel@networkimprov.net>
> ---
>  drivers/power/supply/bq24190_charger.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index c443988..ad09e48 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -1267,12 +1267,13 @@ static void bq24190_check_status(struct bq24190_dev_info *bdi)
>  static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
>  {
>  	struct bq24190_dev_info *bdi = data;
> -	int ret;
> +	int error;
>  
>  	bdi->irq_event = true;
> -	ret = pm_runtime_get_sync(bdi->dev);
> -	if (ret < 0) {
> -		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", ret);
> +	error = pm_runtime_get_sync(bdi->dev);
> +	if (error < 0) {
> +		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
> +		pm_runtime_put_noidle(bdi->dev);
>  		return IRQ_NONE;
>  	}
>  	bq24190_check_status(bdi);
> @@ -1399,8 +1400,10 @@ static int bq24190_probe(struct i2c_client *client,
>  	pm_runtime_use_autosuspend(dev);
>  	pm_runtime_set_autosuspend_delay(dev, 600);
>  	ret = pm_runtime_get_sync(dev);
> -	if (ret < 0)
> +	if (ret < 0) {
> +		dev_err(dev, "pm_runtime_get failed: %i\n", ret);
>  		goto out1;
> +	}

This should goto out2, which will make out1 not used anymore.
Would be nice, if you move this patch to the beginning of the
series in the next version, so that I can pick it already while
waiting for Acks on the DT binding.

-- Sebastian

>  
>  	ret = bq24190_hw_init(bdi);
>  	if (ret < 0) {
> -- 
> 2.9.3
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v1 5/7] power: bq24190_charger: Reorder init sequence in probe()

[Liam Breck]

From: Liam Breck <kernel@networkimprov.net>

We need a struct power_supply object from register() before
calling setup_dt()

Cc: Mark Greer <mgreer@animalcreek.com>
Signed-off-by: Liam Breck <kernel@networkimprov.net>
---
 drivers/power/supply/bq24190_charger.c | 37 +++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index ad09e48..2fbc7f3 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -1386,16 +1386,6 @@ static int bq24190_probe(struct i2c_client *client,
 
 	i2c_set_clientdata(client, bdi);
 
-	if (dev->of_node)
-		ret = bq24190_setup_dt(bdi);
-	else
-		ret = bq24190_setup_pdata(bdi, pdata);
-
-	if (ret) {
-		dev_err(dev, "Can't get irq info\n");
-		return -EINVAL;
-	}
-
 	pm_runtime_enable(dev);
 	pm_runtime_use_autosuspend(dev);
 	pm_runtime_set_autosuspend_delay(dev, 600);
@@ -1405,12 +1395,6 @@ static int bq24190_probe(struct i2c_client *client,
 		goto out1;
 	}
 
-	ret = bq24190_hw_init(bdi);
-	if (ret < 0) {
-		dev_err(dev, "Hardware init failed\n");
-		goto out2;
-	}
-
 	charger_cfg.drv_data = bdi;
 	charger_cfg.supplied_to = bq24190_charger_supplied_to;
 	charger_cfg.num_supplicants = ARRAY_SIZE(bq24190_charger_supplied_to),
@@ -1431,6 +1415,23 @@ static int bq24190_probe(struct i2c_client *client,
 		goto out3;
 	}
 
+	if (dev->of_node)
+		ret = bq24190_setup_dt(bdi);
+	else
+		ret = bq24190_setup_pdata(bdi, pdata);
+
+	if (ret) {
+		dev_err(dev, "Can't get irq info\n");
+		ret = -EINVAL;
+		goto out4;
+	}
+
+	ret = bq24190_hw_init(bdi);
+	if (ret < 0) {
+		dev_err(dev, "Hardware init failed\n");
+		goto out4;
+	}
+
 	ret = bq24190_sysfs_create_group(bdi);
 	if (ret) {
 		dev_err(dev, "Can't create sysfs entries\n");
@@ -1459,6 +1460,8 @@ static int bq24190_probe(struct i2c_client *client,
 	bq24190_sysfs_remove_group(bdi);
 
 out4:
+	if (bdi->gpio_int)
+		gpio_free(bdi->gpio_int);
 	power_supply_unregister(bdi->battery);
 
 out3:
@@ -1470,8 +1473,6 @@ static int bq24190_probe(struct i2c_client *client,
 out1:
 	pm_runtime_dont_use_autosuspend(dev);
 	pm_runtime_disable(dev);
-	if (bdi->gpio_int)
-		gpio_free(bdi->gpio_int);
 	return ret;
 }
 
-- 
2.9.3

Re: [PATCH v1 5/7] power: bq24190_charger: Reorder init sequence in probe()

[Hans de Goede]

Hi,

On 21-03-17 23:09, Liam Breck wrote:
> From: Liam Breck <kernel@networkimprov.net>
>
> We need a struct power_supply object from register() before
> calling setup_dt()
>
> Cc: Mark Greer <mgreer@animalcreek.com>
> Signed-off-by: Liam Breck <kernel@networkimprov.net>

Looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> ---
>  drivers/power/supply/bq24190_charger.c | 37 +++++++++++++++++-----------------
>  1 file changed, 19 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index ad09e48..2fbc7f3 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -1386,16 +1386,6 @@ static int bq24190_probe(struct i2c_client *client,
>
>  	i2c_set_clientdata(client, bdi);
>
> -	if (dev->of_node)
> -		ret = bq24190_setup_dt(bdi);
> -	else
> -		ret = bq24190_setup_pdata(bdi, pdata);
> -
> -	if (ret) {
> -		dev_err(dev, "Can't get irq info\n");
> -		return -EINVAL;
> -	}
> -
>  	pm_runtime_enable(dev);
>  	pm_runtime_use_autosuspend(dev);
>  	pm_runtime_set_autosuspend_delay(dev, 600);
> @@ -1405,12 +1395,6 @@ static int bq24190_probe(struct i2c_client *client,
>  		goto out1;
>  	}
>
> -	ret = bq24190_hw_init(bdi);
> -	if (ret < 0) {
> -		dev_err(dev, "Hardware init failed\n");
> -		goto out2;
> -	}
> -
>  	charger_cfg.drv_data = bdi;
>  	charger_cfg.supplied_to = bq24190_charger_supplied_to;
>  	charger_cfg.num_supplicants = ARRAY_SIZE(bq24190_charger_supplied_to),
> @@ -1431,6 +1415,23 @@ static int bq24190_probe(struct i2c_client *client,
>  		goto out3;
>  	}
>
> +	if (dev->of_node)
> +		ret = bq24190_setup_dt(bdi);
> +	else
> +		ret = bq24190_setup_pdata(bdi, pdata);
> +
> +	if (ret) {
> +		dev_err(dev, "Can't get irq info\n");
> +		ret = -EINVAL;
> +		goto out4;
> +	}
> +
> +	ret = bq24190_hw_init(bdi);
> +	if (ret < 0) {
> +		dev_err(dev, "Hardware init failed\n");
> +		goto out4;
> +	}
> +
>  	ret = bq24190_sysfs_create_group(bdi);
>  	if (ret) {
>  		dev_err(dev, "Can't create sysfs entries\n");
> @@ -1459,6 +1460,8 @@ static int bq24190_probe(struct i2c_client *client,
>  	bq24190_sysfs_remove_group(bdi);
>
>  out4:
> +	if (bdi->gpio_int)
> +		gpio_free(bdi->gpio_int);
>  	power_supply_unregister(bdi->battery);
>
>  out3:
> @@ -1470,8 +1473,6 @@ static int bq24190_probe(struct i2c_client *client,
>  out1:
>  	pm_runtime_dont_use_autosuspend(dev);
>  	pm_runtime_disable(dev);
> -	if (bdi->gpio_int)
> -		gpio_free(bdi->gpio_int);
>  	return ret;
>  }
>
>

Re: [PATCH v1 5/7] power: bq24190_charger: Reorder init sequence in probe()

[Tony Lindgren]

* Hans de Goede <hdegoede@redhat.com> [170322 05:16]:
> Hi,
> 
> On 21-03-17 23:09, Liam Breck wrote:
> > From: Liam Breck <kernel@networkimprov.net>
> > 
> > We need a struct power_supply object from register() before
> > calling setup_dt()
> > 
> > Cc: Mark Greer <mgreer@animalcreek.com>
> > Signed-off-by: Liam Breck <kernel@networkimprov.net>
> 
> Looks good to me:
> 
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Yeah me too:

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

[PATCH v1 6/7] power: bq24190_charger: Add power_supply_battery_info and devicetree support

[Liam Breck]

From: Liam Breck <kernel@networkimprov.net>

Obtain pre-charge and end-charge current values from power_supply_battery_info.
Obtain minimum system voltage level from devicetree.

Cc: Mark Greer <mgreer@animalcreek.com>
Signed-off-by: Liam Breck <kernel@networkimprov.net>
---
 drivers/power/supply/bq24190_charger.c | 102 +++++++++++++++++++++++++++++----
 1 file changed, 90 insertions(+), 12 deletions(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index 2fbc7f3..a52816b 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -43,6 +43,8 @@
 #define BQ24190_REG_POC_SYS_MIN_SHIFT		1
 #define BQ24190_REG_POC_BOOST_LIM_MASK		BIT(0)
 #define BQ24190_REG_POC_BOOST_LIM_SHIFT		0
+#define BQ24190_REG_POC_SYS_MIN_MIN		3000
+#define BQ24190_REG_POC_SYS_MIN_MAX		3700
 
 #define BQ24190_REG_CCC		0x02 /* Charge Current Control */
 #define BQ24190_REG_CCC_ICHG_MASK		(BIT(7) | BIT(6) | BIT(5) | \
@@ -58,6 +60,10 @@
 #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_IPRECHG_MIN		128
+#define BQ24190_REG_PCTCC_IPRECHG_MAX		2048
+#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) | \
@@ -157,6 +163,9 @@ struct bq24190_dev_info {
 	unsigned int			irq;
 	bool				initialized;
 	bool				irq_event;
+	u16				sys_min;
+	u16				iprechg;
+	u16				iterm;
 	struct mutex			f_reg_lock;
 	u8				f_reg;
 	u8				ss_reg;
@@ -496,19 +505,20 @@ 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_operating_params(struct bq24190_dev_info *bdi)
 {
 	int ret;
 	u8 v;
 
+	/*
+	 * 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.
+	 */
+
 	ret = bq24190_read(bdi, BQ24190_REG_CTTC, &v);
 	if (ret < 0)
 		return ret;
@@ -517,7 +527,41 @@ static int bq24190_set_mode_host(struct bq24190_dev_info *bdi)
 					BQ24190_REG_CTTC_WATCHDOG_SHIFT);
 	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;
+
+	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;
+	}
+
+	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;
 }
 
 static int bq24190_register_reset(struct bq24190_dev_info *bdi)
@@ -1304,7 +1348,7 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
 	if (ret < 0)
 		return ret;
 
-	ret = bq24190_set_mode_host(bdi);
+	ret = bq24190_set_operating_params(bdi);
 	if (ret < 0)
 		return ret;
 
@@ -1314,6 +1358,39 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
 #ifdef CONFIG_OF
 static int bq24190_setup_dt(struct bq24190_dev_info *bdi)
 {
+	struct power_supply_battery_info info = {};
+	const char const* s = "ti,system-minimum-microvolt";
+	int v;
+
+	if (!of_property_read_u32(bdi->dev->of_node, s, &v)) {
+		v /= 1000;
+		if (v >= BQ24190_REG_POC_SYS_MIN_MIN
+		 && v <= BQ24190_REG_POC_SYS_MIN_MAX)
+			bdi->sys_min = v;
+		else
+			dev_err(bdi->dev, "invalid value for %s: %u\n", s, v);
+	}
+
+	if (!power_supply_get_battery_info(bdi->battery, &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_err(bdi->dev,
+				"invalid value for battery:precharge-current-microamp: %d\n",
+				v);
+
+		v = info.endcharge_current_ua / 1000;
+		if (v >= BQ24190_REG_PCTCC_ITERM_MIN
+		 && v <= BQ24190_REG_PCTCC_ITERM_MAX)
+			bdi->iterm = v;
+		else
+			dev_err(bdi->dev,
+				"invalid value for battery:endcharge-current-microamp: %d\n",
+				v);
+	}
+
 	bdi->irq = irq_of_parse_and_map(bdi->dev->of_node, 0);
 	if (bdi->irq <= 0)
 		return -1;
@@ -1407,6 +1484,7 @@ static int bq24190_probe(struct i2c_client *client,
 	}
 
 	battery_cfg.drv_data = bdi;
+	battery_cfg.of_node = dev->of_node;
 	bdi->battery = power_supply_register(dev, &bq24190_battery_desc,
 						&battery_cfg);
 	if (IS_ERR(bdi->battery)) {
@@ -1570,7 +1648,7 @@ static int bq24190_pm_resume(struct device *dev)
 	}
 
 	bq24190_register_reset(bdi);
-	bq24190_set_mode_host(bdi);
+	bq24190_set_operating_params(bdi);
 	bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
 
 	if (error >= 0) {
-- 
2.9.3

Re: [PATCH v1 6/7] power: bq24190_charger: Add power_supply_battery_info and devicetree support

[Hans de Goede]

Hi,

On 21-03-17 23:09, Liam Breck wrote:
> From: Liam Breck <kernel@networkimprov.net>
>
> Obtain pre-charge and end-charge current values from power_supply_battery_info.
> Obtain minimum system voltage level from devicetree.
>
> Cc: Mark Greer <mgreer@animalcreek.com>
> Signed-off-by: Liam Breck <kernel@networkimprov.net>

Looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans



> ---
>  drivers/power/supply/bq24190_charger.c | 102 +++++++++++++++++++++++++++++----
>  1 file changed, 90 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index 2fbc7f3..a52816b 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -43,6 +43,8 @@
>  #define BQ24190_REG_POC_SYS_MIN_SHIFT		1
>  #define BQ24190_REG_POC_BOOST_LIM_MASK		BIT(0)
>  #define BQ24190_REG_POC_BOOST_LIM_SHIFT		0
> +#define BQ24190_REG_POC_SYS_MIN_MIN		3000
> +#define BQ24190_REG_POC_SYS_MIN_MAX		3700
>  Hi,
>  #define BQ24190_REG_CCC		0x02 /* Charge Current Control */
>  #define BQ24190_REG_CCC_ICHG_MASK		(BIT(7) | BIT(6) | BIT(5) | \
> @@ -58,6 +60,10 @@
>  #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_IPRECHG_MIN		128
> +#define BQ24190_REG_PCTCC_IPRECHG_MAX		2048
> +#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) | \
> @@ -157,6 +163,9 @@ struct bq24190_dev_info {
>  	unsigned int			irq;
>  	bool				initialized;
>  	bool				irq_event;
> +	u16				sys_min;
> +	u16				iprechg;
> +	u16				iterm;
>  	struct mutex			f_reg_lock;
>  	u8				f_reg;
>  	u8				ss_reg;
> @@ -496,19 +505,20 @@ 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_operating_params(struct bq24190_dev_info *bdi)
>  {
>  	int ret;
>  	u8 v;
>
> +	/*
> +	 * 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.
> +	 */
> +
>  	ret = bq24190_read(bdi, BQ24190_REG_CTTC, &v);
>  	if (ret < 0)
>  		return ret;
> @@ -517,7 +527,41 @@ static int bq24190_set_mode_host(struct bq24190_dev_info *bdi)
>  					BQ24190_REG_CTTC_WATCHDOG_SHIFT);
>  	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;
> +
> +	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;
> +	}
> +
> +	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;
>  }
>
>  static int bq24190_register_reset(struct bq24190_dev_info *bdi)
> @@ -1304,7 +1348,7 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
>  	if (ret < 0)
>  		return ret;
>
> -	ret = bq24190_set_mode_host(bdi);
> +	ret = bq24190_set_operating_params(bdi);
>  	if (ret < 0)
>  		return ret;
>
> @@ -1314,6 +1358,39 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
>  #ifdef CONFIG_OF
>  static int bq24190_setup_dt(struct bq24190_dev_info *bdi)
>  {
> +	struct power_supply_battery_info info = {};
> +	const char const* s = "ti,system-minimum-microvolt";
> +	int v;
> +
> +	if (!of_property_read_u32(bdi->dev->of_node, s, &v)) {
> +		v /= 1000;
> +		if (v >= BQ24190_REG_POC_SYS_MIN_MIN
> +		 && v <= BQ24190_REG_POC_SYS_MIN_MAX)
> +			bdi->sys_min = v;
> +		else
> +			dev_err(bdi->dev, "invalid value for %s: %u\n", s, v);
> +	}
> +
> +	if (!power_supply_get_battery_info(bdi->battery, &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_err(bdi->dev,
> +				"invalid value for battery:precharge-current-microamp: %d\n",
> +				v);
> +
> +		v = info.endcharge_current_ua / 1000;
> +		if (v >= BQ24190_REG_PCTCC_ITERM_MIN
> +		 && v <= BQ24190_REG_PCTCC_ITERM_MAX)
> +			bdi->iterm = v;
> +		else
> +			dev_err(bdi->dev,
> +				"invalid value for battery:endcharge-current-microamp: %d\n",
> +				v);
> +	}
> +
>  	bdi->irq = irq_of_parse_and_map(bdi->dev->of_node, 0);
>  	if (bdi->irq <= 0)
>  		return -1;
> @@ -1407,6 +1484,7 @@ static int bq24190_probe(struct i2c_client *client,
>  	}
>
>  	battery_cfg.drv_data = bdi;
> +	battery_cfg.of_node = dev->of_node;
>  	bdi->battery = power_supply_register(dev, &bq24190_battery_desc,
>  						&battery_cfg);
>  	if (IS_ERR(bdi->battery)) {
> @@ -1570,7 +1648,7 @@ static int bq24190_pm_resume(struct device *dev)
>  	}
>
>  	bq24190_register_reset(bdi);
> -	bq24190_set_mode_host(bdi);
> +	bq24190_set_operating_params(bdi);
>  	bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
>
>  	if (error >= 0) {
>

Re: [PATCH v1 6/7] power: bq24190_charger: Add power_supply_battery_info and devicetree support

[Sebastian Reichel]

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

Hi,

On Tue, Mar 21, 2017 at 03:09:20PM -0700, Liam Breck wrote:
> From: Liam Breck <kernel@networkimprov.net>
> 
> Obtain pre-charge and end-charge current values from power_supply_battery_info.
> Obtain minimum system voltage level from devicetree.
> 
> Cc: Mark Greer <mgreer@animalcreek.com>
> Signed-off-by: Liam Breck <kernel@networkimprov.net>
>
> [...]
>
> @@ -1314,6 +1358,39 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
>  #ifdef CONFIG_OF
>  static int bq24190_setup_dt(struct bq24190_dev_info *bdi)
>  {
> +	struct power_supply_battery_info info = {};
> +	const char const* s = "ti,system-minimum-microvolt";
> +	int v;
> +
> +	if (!of_property_read_u32(bdi->dev->of_node, s, &v)) {
> +		v /= 1000;
> +		if (v >= BQ24190_REG_POC_SYS_MIN_MIN
> +		 && v <= BQ24190_REG_POC_SYS_MIN_MAX)
> +			bdi->sys_min = v;
> +		else
> +			dev_err(bdi->dev, "invalid value for %s: %u\n", s, v);
> +	}

Let's use device_property_read_u32 here. With that change
there is nothing DT specific, so the function can be named
bq24190_setup (or everything is done directly in probe, that's
also fine with me).

> +	if (!power_supply_get_battery_info(bdi->battery, &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_err(bdi->dev,
> +				"invalid value for battery:precharge-current-microamp: %d\n",
> +				v);
> +
> +		v = info.endcharge_current_ua / 1000;
> +		if (v >= BQ24190_REG_PCTCC_ITERM_MIN
> +		 && v <= BQ24190_REG_PCTCC_ITERM_MAX)
> +			bdi->iterm = v;
> +		else
> +			dev_err(bdi->dev,
> +				"invalid value for battery:endcharge-current-microamp: %d\n",
> +				v);
> +	}
> +
>  	bdi->irq = irq_of_parse_and_map(bdi->dev->of_node, 0);
>  	if (bdi->irq <= 0)
>  		return -1;

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v1 7/7] power: bq24190_charger: Set bq24190-battery device .type=unknown

[Liam Breck]

From: Liam Breck <kernel@networkimprov.net>

Not for upstream. Temporary workaround to prevent bq24190-battery device
from interfering with a fuel gauge that has .type=power_supply_type_battery

I'll move properties from -battery device to -charger in a subsequent version.

Cc: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/bq24190_charger.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index a52816b..12f3780 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -1228,7 +1228,7 @@ static enum power_supply_property bq24190_battery_properties[] = {
 
 static const struct power_supply_desc bq24190_battery_desc = {
 	.name			= "bq24190-battery",
-	.type			= POWER_SUPPLY_TYPE_BATTERY,
+	.type			= POWER_SUPPLY_TYPE_UNKNOWN,
 	.properties		= bq24190_battery_properties,
 	.num_properties		= ARRAY_SIZE(bq24190_battery_properties),
 	.get_property		= bq24190_battery_get_property,
-- 
2.9.3

Re: [PATCH v1 7/7] power: bq24190_charger: Set bq24190-battery device .type=unknown

[Hans de Goede]

Hi,

On 21-03-17 23:09, Liam Breck wrote:
> From: Liam Breck <kernel@networkimprov.net>
>
> Not for upstream. Temporary workaround to prevent bq24190-battery device
> from interfering with a fuel gauge that has .type=power_supply_type_battery
>
> I'll move properties from -battery device to -charger in a subsequent version.
>
> Cc: Hans de Goede <hdegoede@redhat.com>

Hmm, I don't like this. I was about to post v2 of my patches (which I will
rebase on top of this series), which contains a patch to completely remove
the battery power_supply. I understand if you think it is too early for
that, but I believe that if we (temporarily) want to keep it around,
it would be better to do something like this:

     /*
      * Note the disable-battery-power-supply property is purely an in
      * kernel interface to avoid having 2 battery type power_supplies for
      * a single physical battery when there also is a fuel-gauge driver.
      * The plan is to remove the battery power_supply from the bq24190
      * driver completely, but that still needs some work.
      * Do NOT use this property in DT files.
      */
     if (!device_property_read_bool(dev, "disable-battery-power-supply")) {
         register(battery)...
     }

Sebastian would something like the above work for you as an interim
solution ? Since this uses device-properties I can easily add the
property to my i2c_board_info, and when the battery power_supply gets
removed all together the board_info can be updated independently of
the power_supply driver, so we can easily clean this up afterwards.

Regards,

Hans


> ---
>  drivers/power/supply/bq24190_charger.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index a52816b..12f3780 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -1228,7 +1228,7 @@ static enum power_supply_property bq24190_battery_properties[] = {
>
>  static const struct power_supply_desc bq24190_battery_desc = {
>  	.name			= "bq24190-battery",
> -	.type			= POWER_SUPPLY_TYPE_BATTERY,
> +	.type			= POWER_SUPPLY_TYPE_UNKNOWN,
>  	.properties		= bq24190_battery_properties,
>  	.num_properties		= ARRAY_SIZE(bq24190_battery_properties),
>  	.get_property		= bq24190_battery_get_property,
>

Re: [PATCH v1 7/7] power: bq24190_charger: Set bq24190-battery device .type=unknown

[Hans de Goede]

Hi,

On 22-03-17 13:25, Hans de Goede wrote:
> Hi,
>
> On 21-03-17 23:09, Liam Breck wrote:
>> From: Liam Breck <kernel@networkimprov.net>
>>
>> Not for upstream. Temporary workaround to prevent bq24190-battery device
>> from interfering with a fuel gauge that has .type=power_supply_type_battery
>>
>> I'll move properties from -battery device to -charger in a subsequent version.
>>
>> Cc: Hans de Goede <hdegoede@redhat.com>
>
> Hmm, I don't like this. I was about to post v2 of my patches (which I will
> rebase on top of this series)

It seems this series does not apply to:

https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git/log/?h=for-next

It looks like there is another series which sits in between ?

So I'm going to send the current v2 set of my bq24190_charger
patches as is for review purposes.

I'm fine with rebasing it on top of your series if that is considered
ready for merging soon. Do you have a git-repo / branch somewhere
with your complete series ?

Otherwise if Sebastian is happy with my patches we can do it the
other way around and merge my series first and then rebase yours on top.
Eitherway we will figure it out.

Regards,

Hans

Re: [PATCH v1 7/7] power: bq24190_charger: Set bq24190-battery device .type=unknown

[Liam Breck]

On Wed, Mar 22, 2017 at 6:15 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 22-03-17 13:25, Hans de Goede wrote:
>>
>> Hi,
>>
>> On 21-03-17 23:09, Liam Breck wrote:
>>>
>>> From: Liam Breck <kernel@networkimprov.net>
>>>
>>> Not for upstream. Temporary workaround to prevent bq24190-battery device
>>> from interfering with a fuel gauge that has
>>> .type=power_supply_type_battery
>>>
>>> I'll move properties from -battery device to -charger in a subsequent
>>> version.
>>>
>>> Cc: Hans de Goede <hdegoede@redhat.com>
>>
>>
>> Hmm, I don't like this. I was about to post v2 of my patches (which I will
>> rebase on top of this series)
>
>
> It seems this series does not apply to:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git/log/?h=for-next
>
> It looks like there is another series which sits in between ?

Yes, see two patches linked in Depends-on lines. Could you add those and rebase?

> So I'm going to send the current v2 set of my bq24190_charger
> patches as is for review purposes.
>
> I'm fine with rebasing it on top of your series if that is considered
> ready for merging soon. Do you have a git-repo / branch somewhere
> with your complete series ?

https://github.com/networkimprov/linux/commits/anvl-v4.9-bq24190-dt-1.1/

However this includes much unrelated stuff, and two patches from Tony
which are already in -next. Prob easier to git am the patches linked
by Depends-on.

> Otherwise if Sebastian is happy with my patches we can do it the
> other way around and merge my series first and then rebase yours on top.
> Eitherway we will figure it out.

Re: [PATCH v1 7/7] power: bq24190_charger: Set bq24190-battery device .type=unknown

[Hans de Goede]

Hi,

On 22-03-17 14:37, Liam Breck wrote:
> On Wed, Mar 22, 2017 at 6:15 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 22-03-17 13:25, Hans de Goede wrote:
>>>
>>> Hi,
>>>
>>> On 21-03-17 23:09, Liam Breck wrote:
>>>>
>>>> From: Liam Breck <kernel@networkimprov.net>
>>>>
>>>> Not for upstream. Temporary workaround to prevent bq24190-battery device
>>>> from interfering with a fuel gauge that has
>>>> .type=power_supply_type_battery
>>>>
>>>> I'll move properties from -battery device to -charger in a subsequent
>>>> version.
>>>>
>>>> Cc: Hans de Goede <hdegoede@redhat.com>
>>>
>>>
>>> Hmm, I don't like this. I was about to post v2 of my patches (which I will
>>> rebase on top of this series)
>>
>>
>> It seems this series does not apply to:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git/log/?h=for-next
>>
>> It looks like there is another series which sits in between ?
>
> Yes, see two patches linked in Depends-on lines. Could you add those and rebase?

I can confirm that with those 2 patches thrown in things apply
cleanly on linux-power-supply/for-next.

I see that these 2 patches add a new devicetree binding for describing
a battery. That is really cool stuff and long overdue IMHO I'm glad
to see someone doing this.

But (sorry) in my experience getting new devicetree bindings upstream
often takes a while, while people are discussing the best way to
solve things. So I'm not quite sure this will go upstream real soon
now and I would like to get my patches into -next in time for 4.12
if possible.

So for now I'm going to hold of on rebasing and just send v2 as is
(based on linux-power-supply/for-next). If Sebastian thinks your
patches should go first I will happily do a rebased v3.

Regards,

Hans

Re: [PATCH v1 7/7] power: bq24190_charger: Set bq24190-battery device .type=unknown

[Liam Breck]

On Wed, Mar 22, 2017 at 7:41 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 22-03-17 14:37, Liam Breck wrote:
>>
>> On Wed, Mar 22, 2017 at 6:15 AM, Hans de Goede <hdegoede@redhat.com>
>> wrote:
>>>
>>> Hi,
>>>
>>> On 22-03-17 13:25, Hans de Goede wrote:
>>>>
>>>>
>>>> Hi,
>>>>
>>>> On 21-03-17 23:09, Liam Breck wrote:
>>>>>
>>>>>
>>>>> From: Liam Breck <kernel@networkimprov.net>
>>>>>
>>>>> Not for upstream. Temporary workaround to prevent bq24190-battery
>>>>> device
>>>>> from interfering with a fuel gauge that has
>>>>> .type=power_supply_type_battery
>>>>>
>>>>> I'll move properties from -battery device to -charger in a subsequent
>>>>> version.
>>>>>
>>>>> Cc: Hans de Goede <hdegoede@redhat.com>
>>>>
>>>>
>>>>
>>>> Hmm, I don't like this. I was about to post v2 of my patches (which I
>>>> will
>>>> rebase on top of this series)
>>>
>>>
>>>
>>> It seems this series does not apply to:
>>>
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git/log/?h=for-next
>>>
>>> It looks like there is another series which sits in between ?
>>
>>
>> Yes, see two patches linked in Depends-on lines. Could you add those and
>> rebase?
>
>
> I can confirm that with those 2 patches thrown in things apply
> cleanly on linux-power-supply/for-next.
>
> I see that these 2 patches add a new devicetree binding for describing
> a battery. That is really cool stuff and long overdue IMHO I'm glad
> to see someone doing this.
>
> But (sorry) in my experience getting new devicetree bindings upstream
> often takes a while, while people are discussing the best way to
> solve things. So I'm not quite sure this will go upstream real soon
> now and I would like to get my patches into -next in time for 4.12
> if possible.

Sebastian asked us to write those two patches, and has acked one, I
expect ack on the docs for this version (the DT api is settled). The
only thing holding up that patchset is a test pass by the TI
maintainer of bq27xxx_battery. But we can separate the
power_supply_core changes from that if nec.

I would like to run your patches in my environment for some time
before acking. For that, I need the -battery back! And also this
patchset.

How's progress on the pmic-gauge battery driver? I'd be curious to see
it since I've been adding features to the bq27xxx driver lately.

> So for now I'm going to hold of on rebasing and just send v2 as is
> (based on linux-power-supply/for-next). If Sebastian thinks your
> patches should go first I will happily do a rebased v3.
>
> Regards,
>
> Hans

Re: [PATCH v1 7/7] power: bq24190_charger: Set bq24190-battery device .type=unknown

[Hans de Goede]

Hi,

On 22-03-17 19:33, Liam Breck wrote:
> On Wed, Mar 22, 2017 at 7:41 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 22-03-17 14:37, Liam Breck wrote:
>>>
>>> On Wed, Mar 22, 2017 at 6:15 AM, Hans de Goede <hdegoede@redhat.com>
>>> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 22-03-17 13:25, Hans de Goede wrote:
>>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>> On 21-03-17 23:09, Liam Breck wrote:
>>>>>>
>>>>>>
>>>>>> From: Liam Breck <kernel@networkimprov.net>
>>>>>>
>>>>>> Not for upstream. Temporary workaround to prevent bq24190-battery
>>>>>> device
>>>>>> from interfering with a fuel gauge that has
>>>>>> .type=power_supply_type_battery
>>>>>>
>>>>>> I'll move properties from -battery device to -charger in a subsequent
>>>>>> version.
>>>>>>
>>>>>> Cc: Hans de Goede <hdegoede@redhat.com>
>>>>>
>>>>>
>>>>>
>>>>> Hmm, I don't like this. I was about to post v2 of my patches (which I
>>>>> will
>>>>> rebase on top of this series)
>>>>
>>>>
>>>>
>>>> It seems this series does not apply to:
>>>>
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git/log/?h=for-next
>>>>
>>>> It looks like there is another series which sits in between ?
>>>
>>>
>>> Yes, see two patches linked in Depends-on lines. Could you add those and
>>> rebase?
>>
>>
>> I can confirm that with those 2 patches thrown in things apply
>> cleanly on linux-power-supply/for-next.
>>
>> I see that these 2 patches add a new devicetree binding for describing
>> a battery. That is really cool stuff and long overdue IMHO I'm glad
>> to see someone doing this.
>>
>> But (sorry) in my experience getting new devicetree bindings upstream
>> often takes a while, while people are discussing the best way to
>> solve things. So I'm not quite sure this will go upstream real soon
>> now and I would like to get my patches into -next in time for 4.12
>> if possible.
>
> Sebastian asked us to write those two patches, and has acked one, I
> expect ack on the docs for this version (the DT api is settled). The
> only thing holding up that patchset is a test pass by the TI
> maintainer of bq27xxx_battery. But we can separate the
> power_supply_core changes from that if nec.

Ok, lets let Sebastian decide on the order in which to merge these.

> I would like to run your patches in my environment for some time
> before acking. For that, I need the -battery back! And also this
> patchset.

If Sebastian decides he wants to wait with merging these till your
patches are merged then I'll rebase, otherwise you need to rebase
anyways and can do so for your testing.

> How's progress on the pmic-gauge battery driver? I'd be curious to see
> it since I've been adding features to the bq27xxx driver lately.

Progress is good I hope to post a v2 today, in the mean time
you can find it here:

https://github.com/jwrdegoede/linux-sunxi/commit/7b667e5068393afc29c7173c9f4eae46b026a849

Regards,

Hans


>
>> So for now I'm going to hold of on rebasing and just send v2 as is
>> (based on linux-power-supply/for-next). If Sebastian thinks your
>> patches should go first I will happily do a rebased v3.
>>
>> Regards,
>>
>> Hans

Re: [PATCH v1 7/7] power: bq24190_charger: Set bq24190-battery device .type=unknown

[Liam Breck]

On Thu, Mar 23, 2017 at 1:18 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
>
> On 22-03-17 19:33, Liam Breck wrote:
>>
>> On Wed, Mar 22, 2017 at 7:41 AM, Hans de Goede <hdegoede@redhat.com>
>> wrote:
>>>
>>> Hi,
>>>
>>> On 22-03-17 14:37, Liam Breck wrote:
>>>>
>>>>
>>>> On Wed, Mar 22, 2017 at 6:15 AM, Hans de Goede <hdegoede@redhat.com>
>>>> wrote:
>>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>> On 22-03-17 13:25, Hans de Goede wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> On 21-03-17 23:09, Liam Breck wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> From: Liam Breck <kernel@networkimprov.net>
>>>>>>>
>>>>>>> Not for upstream. Temporary workaround to prevent bq24190-battery
>>>>>>> device
>>>>>>> from interfering with a fuel gauge that has
>>>>>>> .type=power_supply_type_battery
>>>>>>>
>>>>>>> I'll move properties from -battery device to -charger in a subsequent
>>>>>>> version.
>>>>>>>
>>>>>>> Cc: Hans de Goede <hdegoede@redhat.com>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Hmm, I don't like this. I was about to post v2 of my patches (which I
>>>>>> will
>>>>>> rebase on top of this series)
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> It seems this series does not apply to:
>>>>>
>>>>>
>>>>>
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git/log/?h=for-next
>>>>>
>>>>> It looks like there is another series which sits in between ?
>>>>
>>>>
>>>>
>>>> Yes, see two patches linked in Depends-on lines. Could you add those and
>>>> rebase?
>>>
>>>
>>>
>>> I can confirm that with those 2 patches thrown in things apply
>>> cleanly on linux-power-supply/for-next.
>>>
>>> I see that these 2 patches add a new devicetree binding for describing
>>> a battery. That is really cool stuff and long overdue IMHO I'm glad
>>> to see someone doing this.
>>>
>>> But (sorry) in my experience getting new devicetree bindings upstream
>>> often takes a while, while people are discussing the best way to
>>> solve things. So I'm not quite sure this will go upstream real soon
>>> now and I would like to get my patches into -next in time for 4.12
>>> if possible.
>>
>>
>> Sebastian asked us to write those two patches, and has acked one, I
>> expect ack on the docs for this version (the DT api is settled). The
>> only thing holding up that patchset is a test pass by the TI
>> maintainer of bq27xxx_battery. But we can separate the
>> power_supply_core changes from that if nec.
>
>
> Ok, lets let Sebastian decide on the order in which to merge these.
>
>> I would like to run your patches in my environment for some time
>> before acking. For that, I need the -battery back! And also this
>> patchset.
>
>
> If Sebastian decides he wants to wait with merging these till your
> patches are merged then I'll rebase, otherwise you need to rebase
> anyways and can do so for your testing.

I'm grateful for your patches, and I do need to test your code before
it's merged. Pls do what you can to make that straightforward. If
there's a reason to expedite your patchset, do let me know.

To clarify my role, I hired Mark to write this driver from scratch
four years ago, and since then have hired Tony periodically to
test/fix it. I've also invested a significant cash and man-years on
the hw design that has this chip. After looking at this code and the
docs over all this time, I realized I could be improving it myself,
hence the recent commits.

If I had a spare board to send to facilitate your efforts, I would,
but the yield on last prototype run was abysmal. Later this year I can
get you one.

>> How's progress on the pmic-gauge battery driver? I'd be curious to see
>> it since I've been adding features to the bq27xxx driver lately.
>
>
> Progress is good I hope to post a v2 today, in the mean time
> you can find it here:
>
> https://github.com/jwrdegoede/linux-sunxi/commit/7b667e5068393afc29c7173c9f4eae46b026a849

Nice!

>
>
>
>>
>>> So for now I'm going to hold of on rebasing and just send v2 as is
>>> (based on linux-power-supply/for-next). If Sebastian thinks your
>>> patches should go first I will happily do a rebased v3.
>>>
>>> Regards,
>>>
>>> Hans

Re: [PATCH v1 7/7] power: bq24190_charger: Set bq24190-battery device .type=unknown

[Liam Breck]

Hi, thanks for reviews and comments on sysfs issue.

On Wed, Mar 22, 2017 at 5:25 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 21-03-17 23:09, Liam Breck wrote:
>>
>> From: Liam Breck <kernel@networkimprov.net>
>>
>> Not for upstream. Temporary workaround to prevent bq24190-battery device
>> from interfering with a fuel gauge that has
>> .type=power_supply_type_battery
>>
>> I'll move properties from -battery device to -charger in a subsequent
>> version.
>>
>> Cc: Hans de Goede <hdegoede@redhat.com>
>
>
> Hmm, I don't like this. I was about to post v2 of my patches (which I will
> rebase on top of this series), which contains a patch to completely remove
> the battery power_supply. I understand if you think it is too early for
> that, but I believe that if we (temporarily) want to keep it around,
> it would be better to do something like this:

I need most of the properties in -battery; I am working on moving them
into -charger. It's not a trivial cut/paste but should be done by v2
next week.

This patch should clear the way for you to implement your battery
device, without touching the -battery code. That way I can run your
patches in my environment. Don't worry, it's not going upstream. I
didn't even include signed-off-by :-)

BTW I think we can just remove bdi->model; it's not used. However
let's not claim to support any chip we haven't actually tested, so in
hw_init()...

if (v != BQ24190_REG_VPRS_PN_24190 &&
    v != BQ24190_REG_VPRS_PN_yours)
        return -ENODEV;

>     /*
>      * Note the disable-battery-power-supply property is purely an in
>      * kernel interface to avoid having 2 battery type power_supplies for
>      * a single physical battery when there also is a fuel-gauge driver.
>      * The plan is to remove the battery power_supply from the bq24190
>      * driver completely, but that still needs some work.
>      * Do NOT use this property in DT files.
>      */
>     if (!device_property_read_bool(dev, "disable-battery-power-supply")) {
>         register(battery)...
>     }
>
> Sebastian would something like the above work for you as an interim
> solution ? Since this uses device-properties I can easily add the
> property to my i2c_board_info, and when the battery power_supply gets
> removed all together the board_info can be updated independently of
> the power_supply driver, so we can easily clean this up afterwards.

Would make sense if req'd upstream, but I don't think it will be.

> Regards,
>
> Hans
>
>
>
>> ---
>>  drivers/power/supply/bq24190_charger.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/power/supply/bq24190_charger.c
>> b/drivers/power/supply/bq24190_charger.c
>> index a52816b..12f3780 100644
>> --- a/drivers/power/supply/bq24190_charger.c
>> +++ b/drivers/power/supply/bq24190_charger.c
>> @@ -1228,7 +1228,7 @@ static enum power_supply_property
>> bq24190_battery_properties[] = {
>>
>>  static const struct power_supply_desc bq24190_battery_desc = {
>>         .name                   = "bq24190-battery",
>> -       .type                   = POWER_SUPPLY_TYPE_BATTERY,
>> +       .type                   = POWER_SUPPLY_TYPE_UNKNOWN,
>>         .properties             = bq24190_battery_properties,
>>         .num_properties         = ARRAY_SIZE(bq24190_battery_properties),
>>         .get_property           = bq24190_battery_get_property,
>>
>

Re: [PATCH v1 7/7] power: bq24190_charger: Set bq24190-battery device .type=unknown

[Sebastian Reichel]

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

Hi,

On Tue, Mar 21, 2017 at 03:09:21PM -0700, Liam Breck wrote:
> From: Liam Breck <kernel@networkimprov.net>
> 
> Not for upstream.

Obviously.

> Temporary workaround to prevent bq24190-battery device
> from interfering with a fuel gauge that has .type=power_supply_type_battery
> 
> I'll move properties from -battery device to -charger in a subsequent version.

I thought about the transistor, which can take the battery
offline and IMHO it should be exposed as regulator.

That still leaves the health status reading, though. From
userspace's point-of-view this should be merged into the
battery device, if fuel-gauge can't provide that information.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 7/7] power: bq24190_charger: Set bq24190-battery device .type=unknown

[Liam Breck]

On Thu, Mar 23, 2017 at 3:52 AM, Sebastian Reichel <sre@kernel.org> wrote:
> Hi,
>
> On Tue, Mar 21, 2017 at 03:09:21PM -0700, Liam Breck wrote:
>> From: Liam Breck <kernel@networkimprov.net>
>>
>> Not for upstream.
>
> Obviously.
>
>> Temporary workaround to prevent bq24190-battery device
>> from interfering with a fuel gauge that has .type=power_supply_type_battery
>>
>> I'll move properties from -battery device to -charger in a subsequent version.
>
> I thought about the transistor, which can take the battery
> offline and IMHO it should be exposed as regulator.

OK

> That still leaves the health status reading, though. From
> userspace's point-of-view this should be merged into the
> battery device, if fuel-gauge can't provide that information.

I think the bigger issue is that multiple sysfs devices for what is
conceptually one power supply is awkward. With the addition of static
battery object, we have yet another set of properties which belong to
that supply.

A solution to this would be a core registry which allows various
drivers to submit properties to a named supply. Properties with the
same id (e.g. health) are OK given a tag (input-V, NTC-1, battery-V)
to differentiate them. Duplicates would be dropped (e.g. both drivers
submit monitored-battery stats). The result would be represented in
sysfs as a single device, where each property would appear as a file
or directory as necessary. And perhaps also as a file "health_summary"
which indicates whether there's trouble below decks.

Re: [PATCH v1 7/7] power: bq24190_charger: Set bq24190-battery device .type=unknown

[Sebastian Reichel]

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

Hi,

On Thu, Mar 23, 2017 at 04:31:50AM -0700, Liam Breck wrote:
>> That still leaves the health status reading, though. From
>> userspace's point-of-view this should be merged into the
>> battery device, if fuel-gauge can't provide that information.
> 
> I think the bigger issue is that multiple sysfs devices for what is
> conceptually one power supply is awkward. With the addition of static
> battery object, we have yet another set of properties which belong to
> that supply.

The simple-battery is consumed by the fuel-gauge, so not a problem.

> A solution to this would be a core registry which allows various
> drivers to submit properties to a named supply. Properties with the
> same id (e.g. health) are OK given a tag (input-V, NTC-1, battery-V)
> to differentiate them. Duplicates would be dropped (e.g. both drivers
> submit monitored-battery stats). The result would be represented in
> sysfs as a single device, where each property would appear as a file
> or directory as necessary. And perhaps also as a file "health_summary"
> which indicates whether there's trouble below decks.

Yes, that would be the proper solution. Might become important, if
more platforms with multiple chips providing battery information
appear. Since I don't think you want to implement your suggestion,
I suggest sticking to the custom "bat_fault" sysfs property. That
should already be exposed for the charger device as far as I can
see.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 7/7] power: bq24190_charger: Set bq24190-battery device .type=unknown

[Liam Breck]

On Thu, Mar 23, 2017 at 6:37 AM, Sebastian Reichel <sre@kernel.org> wrote:
> Hi,
>
> On Thu, Mar 23, 2017 at 04:31:50AM -0700, Liam Breck wrote:
>>> That still leaves the health status reading, though. From
>>> userspace's point-of-view this should be merged into the
>>> battery device, if fuel-gauge can't provide that information.
>>
>> I think the bigger issue is that multiple sysfs devices for what is
>> conceptually one power supply is awkward. With the addition of static
>> battery object, we have yet another set of properties which belong to
>> that supply.
>
> The simple-battery is consumed by the fuel-gauge, so not a problem.
>
>> A solution to this would be a core registry which allows various
>> drivers to submit properties to a named supply. Properties with the
>> same id (e.g. health) are OK given a tag (input-V, NTC-1, battery-V)
>> to differentiate them. Duplicates would be dropped (e.g. both drivers
>> submit monitored-battery stats). The result would be represented in
>> sysfs as a single device, where each property would appear as a file
>> or directory as necessary. And perhaps also as a file "health_summary"
>> which indicates whether there's trouble below decks.
>
> Yes, that would be the proper solution. Might become important, if
> more platforms with multiple chips providing battery information
> appear. Since I don't think you want to implement your suggestion,

If you're genuinely interested, I could probably ask Matt to work on this...

> I suggest sticking to the custom "bat_fault" sysfs property. That
> should already be exposed for the charger device as far as I can
> see.

Re: [PATCH v1 7/7] power: bq24190_charger: Set bq24190-battery device .type=unknown

[Liam Breck]

On Thu, Mar 23, 2017 at 3:52 AM, Sebastian Reichel <sre@kernel.org> wrote:
> Hi,
>
> On Tue, Mar 21, 2017 at 03:09:21PM -0700, Liam Breck wrote:
>> From: Liam Breck <kernel@networkimprov.net>
>>
>> Not for upstream.
>
> Obviously.
>
>> Temporary workaround to prevent bq24190-battery device
>> from interfering with a fuel gauge that has .type=power_supply_type_battery
>>
>> I'll move properties from -battery device to -charger in a subsequent version.
>
> I thought about the transistor, which can take the battery
> offline and IMHO it should be exposed as regulator.
>
> That still leaves the health status reading, though. From
> userspace's point-of-view this should be merged into the
> battery device, if fuel-gauge can't provide that information.

You mean merged into the -charger device? -battery is the one we're dropping.

If you did mean -battery, could you elaborate?

Re: [PATCH v1 7/7] power: bq24190_charger: Set bq24190-battery device .type=unknown

[Sebastian Reichel]

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

Hi,

On Thu, Mar 23, 2017 at 01:47:23PM -0700, Liam Breck wrote:
> On Thu, Mar 23, 2017 at 3:52 AM, Sebastian Reichel <sre@kernel.org> wrote:
> > That still leaves the health status reading, though. From
> > userspace's point-of-view this should be merged into the
> > battery device, if fuel-gauge can't provide that information.
> 
> You mean merged into the -charger device? -battery is the one
> we're dropping.
> 
> If you did mean -battery, could you elaborate?

I was not talking about bq24190-battery. I meant the battery
device provided by the fuel-gauge. From userspace's point of
view a battery device, which is usually derived from the
fuel-gauge should contain battery related information.

Since the battery health information is battery related it
should somehow be merged into the data from the fuel-gauge.
That's what Hans implemented with the function-pointer
construct and what you suggested to implement in this mail:

http://marc.info/?l=linux-pm&m=149026871313238&w=2

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 7/7] power: bq24190_charger: Set bq24190-battery device .type=unknown

[Liam Breck]

On Fri, Mar 24, 2017 at 2:39 AM, Sebastian Reichel <sre@kernel.org> wrote:
>
> Hi,
>
> On Thu, Mar 23, 2017 at 01:47:23PM -0700, Liam Breck wrote:
> > On Thu, Mar 23, 2017 at 3:52 AM, Sebastian Reichel <sre@kernel.org> wrote:
> > > That still leaves the health status reading, though. From
> > > userspace's point-of-view this should be merged into the
> > > battery device, if fuel-gauge can't provide that information.
> >
> > You mean merged into the -charger device? -battery is the one
> > we're dropping.
> >
> > If you did mean -battery, could you elaborate?
>
> I was not talking about bq24190-battery. I meant the battery
> device provided by the fuel-gauge. From userspace's point of
> view a battery device, which is usually derived from the
> fuel-gauge should contain battery related information.

It doesn't sound right to ask every gauge to absorb features from a
specific charger. Userspace apps are already watching both USB and
battery power supply types for a given battery. They'll pick up a
health issue if we merge -battery and -charger health. Isn't that
enough?

> Since the battery health information is battery related it
> should somehow be merged into the data from the fuel-gauge.
> That's what Hans implemented with the function-pointer
> construct and what you suggested to implement in this mail:
>
> http://marc.info/?l=linux-pm&m=149026871313238&w=2

The merge I suggested was of properties from all power drivers into a
single sysfs device. Which I would work on if you are eager to have
it. I realize it would take a while to be merged.

Re: [PATCH v1 7/7] power: bq24190_charger: Set bq24190-battery device .type=unknown

[Sebastian Reichel]

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

Hi,

On Fri, Mar 24, 2017 at 03:44:51AM -0700, Liam Breck wrote:
> On Fri, Mar 24, 2017 at 2:39 AM, Sebastian Reichel <sre@kernel.org> wrote:
> > On Thu, Mar 23, 2017 at 01:47:23PM -0700, Liam Breck wrote:
> > > On Thu, Mar 23, 2017 at 3:52 AM, Sebastian Reichel <sre@kernel.org> wrote:
> > > > That still leaves the health status reading, though. From
> > > > userspace's point-of-view this should be merged into the
> > > > battery device, if fuel-gauge can't provide that information.
> > >
> > > You mean merged into the -charger device? -battery is the one
> > > we're dropping.
> > >
> > > If you did mean -battery, could you elaborate?
> >
> > I was not talking about bq24190-battery. I meant the battery
> > device provided by the fuel-gauge. From userspace's point of
> > view a battery device, which is usually derived from the
> > fuel-gauge should contain battery related information.
> 
> It doesn't sound right to ask every gauge to absorb features from a
> specific charger. Userspace apps are already watching both USB and
> battery power supply types for a given battery. They'll pick up a
> health issue if we merge -battery and -charger health. Isn't that
> enough?

Technically it's not correct, since high battery temperature is a
much bigger safety risk than high charger chip temperature. I guess
it's good enough, though. I guess the battery temperature health is
related to charging anyways, which is different from the ones making
the battery itself dangerous. So I'm fine with that solution.

> > Since the battery health information is battery related it
> > should somehow be merged into the data from the fuel-gauge.
> > That's what Hans implemented with the function-pointer
> > construct and what you suggested to implement in this mail:
> >
> > http://marc.info/?l=linux-pm&m=149026871313238&w=2
> 
> The merge I suggested was of properties from all power drivers into a
> single sysfs device. Which I would work on if you are eager to have
> it. I realize it would take a while to be merged.

Only the devices belonging to the same battery should be
exposed as one device. Also charger is extra device, since
it may be used to charge multiple batteries. So far its
pretty uncommon, that charger chip provides information
about the battery, so you would spent lots of time for
a single corner case. So while I would like to have it,
I don't think its worth the time to implement it.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 7/7] power: bq24190_charger: Set bq24190-battery device .type=unknown

[Liam Breck]

On Fri, Mar 24, 2017 at 4:22 AM, Sebastian Reichel <sre@kernel.org> wrote:
> Hi,
>
> On Fri, Mar 24, 2017 at 03:44:51AM -0700, Liam Breck wrote:
>> On Fri, Mar 24, 2017 at 2:39 AM, Sebastian Reichel <sre@kernel.org> wrote:
>> > On Thu, Mar 23, 2017 at 01:47:23PM -0700, Liam Breck wrote:
>> > > On Thu, Mar 23, 2017 at 3:52 AM, Sebastian Reichel <sre@kernel.org> wrote:
>> > > > That still leaves the health status reading, though. From
>> > > > userspace's point-of-view this should be merged into the
>> > > > battery device, if fuel-gauge can't provide that information.
>> > >
>> > > You mean merged into the -charger device? -battery is the one
>> > > we're dropping.
>> > >
>> > > If you did mean -battery, could you elaborate?
>> >
>> > I was not talking about bq24190-battery. I meant the battery
>> > device provided by the fuel-gauge. From userspace's point of
>> > view a battery device, which is usually derived from the
>> > fuel-gauge should contain battery related information.
>>
>> It doesn't sound right to ask every gauge to absorb features from a
>> specific charger. Userspace apps are already watching both USB and
>> battery power supply types for a given battery. They'll pick up a
>> health issue if we merge -battery and -charger health. Isn't that
>> enough?
>
> Technically it's not correct, since high battery temperature is a
> much bigger safety risk than high charger chip temperature. I guess
> it's good enough, though. I guess the battery temperature health is
> related to charging anyways, which is different from the ones making
> the battery itself dangerous. So I'm fine with that solution.
>
>> > Since the battery health information is battery related it
>> > should somehow be merged into the data from the fuel-gauge.
>> > That's what Hans implemented with the function-pointer
>> > construct and what you suggested to implement in this mail:
>> >
>> > http://marc.info/?l=linux-pm&m=149026871313238&w=2
>>
>> The merge I suggested was of properties from all power drivers into a
>> single sysfs device. Which I would work on if you are eager to have
>> it. I realize it would take a while to be merged.
>
> Only the devices belonging to the same battery should be
> exposed as one device. Also charger is extra device, since
> it may be used to charge multiple batteries. So far its
> pretty uncommon, that charger chip provides information
> about the battery, so you would spent lots of time for
> a single corner case. So while I would like to have it,
> I don't think its worth the time to implement it.

Don't most battery-powered boxes register separate TYPE_BATTERY and
TYPE_USB/MAINS power supplies? This would unify properties from both
under a single sysfs node, so it's probably lots of cases.

A charger aware of multiple batteries would register properties at a
named supply (sysfs node) for each battery. There would be no charger
sysfs node. Maybe there would be a "DC in" node for its common
regulator functions.

There would be no more chip names appearing as sysfs nodes. System
config (DT, etc) would name the power sysfs nodes.

Hm, the first to-do is mock-up sysfs trees for various use cases...

Re: [PATCH v1 7/7] power: bq24190_charger: Set bq24190-battery device .type=unknown

[Sebastian Reichel]

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

On Fri, Mar 24, 2017 at 04:56:34AM -0700, Liam Breck wrote:
> On Fri, Mar 24, 2017 at 4:22 AM, Sebastian Reichel <sre@kernel.org> wrote:
> > Hi,
> >
> > On Fri, Mar 24, 2017 at 03:44:51AM -0700, Liam Breck wrote:
> >> On Fri, Mar 24, 2017 at 2:39 AM, Sebastian Reichel <sre@kernel.org> wrote:
> >> > On Thu, Mar 23, 2017 at 01:47:23PM -0700, Liam Breck wrote:
> >> > > On Thu, Mar 23, 2017 at 3:52 AM, Sebastian Reichel <sre@kernel.org> wrote:
> >> > > > That still leaves the health status reading, though. From
> >> > > > userspace's point-of-view this should be merged into the
> >> > > > battery device, if fuel-gauge can't provide that information.
> >> > >
> >> > > You mean merged into the -charger device? -battery is the one
> >> > > we're dropping.
> >> > >
> >> > > If you did mean -battery, could you elaborate?
> >> >
> >> > I was not talking about bq24190-battery. I meant the battery
> >> > device provided by the fuel-gauge. From userspace's point of
> >> > view a battery device, which is usually derived from the
> >> > fuel-gauge should contain battery related information.
> >>
> >> It doesn't sound right to ask every gauge to absorb features from a
> >> specific charger. Userspace apps are already watching both USB and
> >> battery power supply types for a given battery. They'll pick up a
> >> health issue if we merge -battery and -charger health. Isn't that
> >> enough?
> >
> > Technically it's not correct, since high battery temperature is a
> > much bigger safety risk than high charger chip temperature. I guess
> > it's good enough, though. I guess the battery temperature health is
> > related to charging anyways, which is different from the ones making
> > the battery itself dangerous. So I'm fine with that solution.
> >
> >> > Since the battery health information is battery related it
> >> > should somehow be merged into the data from the fuel-gauge.
> >> > That's what Hans implemented with the function-pointer
> >> > construct and what you suggested to implement in this mail:
> >> >
> >> > http://marc.info/?l=linux-pm&m=149026871313238&w=2
> >>
> >> The merge I suggested was of properties from all power drivers into a
> >> single sysfs device. Which I would work on if you are eager to have
> >> it. I realize it would take a while to be merged.
> >
> > Only the devices belonging to the same battery should be
> > exposed as one device. Also charger is extra device, since
> > it may be used to charge multiple batteries. So far its
> > pretty uncommon, that charger chip provides information
> > about the battery, so you would spent lots of time for
> > a single corner case. So while I would like to have it,
> > I don't think its worth the time to implement it.
> 
> Don't most battery-powered boxes register separate TYPE_BATTERY and
> TYPE_USB/MAINS power supplies? This would unify properties from both
> under a single sysfs node, so it's probably lots of cases.

Well that will be problematic for systems providing USB and MAINS
and can use both at the same time. The only thing, that should be
unified are properties for the same battery with charger != battery.
Instead ".supplied_to" should be set correctly for the charger, so
that the system knows, that a charger supplies its energy to a
specific battery.

> A charger aware of multiple batteries would register properties at a
> named supply (sysfs node) for each battery. There would be no charger
> sysfs node. Maybe there would be a "DC in" node for its common
> regulator functions.
> 
> There would be no more chip names appearing as sysfs nodes. System
> config (DT, etc) would name the power sysfs nodes.
> 
> Hm, the first to-do is mock-up sysfs trees for various use cases...

I don't see any advantage in that. It also breaks userspace ABI by
removing the charger node and results in work being done by the kernel,
that can easily be done in userspace.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 7/7] power: bq24190_charger: Set bq24190-battery device .type=unknown

[Liam Breck]

Hi Sebastian,

Re surfacing the batfet state as a regulator...

On Thu, Mar 23, 2017 at 3:52 AM, Sebastian Reichel <sre@kernel.org> wrote:
> Hi,
>
> On Tue, Mar 21, 2017 at 03:09:21PM -0700, Liam Breck wrote:
>> From: Liam Breck <kernel@networkimprov.net>
>>
>> Not for upstream.
>
> Obviously.
>
>> Temporary workaround to prevent bq24190-battery device
>> from interfering with a fuel gauge that has .type=power_supply_type_battery
>>
>> I'll move properties from -battery device to -charger in a subsequent version.
>
> I thought about the transistor, which can take the battery
> offline and IMHO it should be exposed as regulator.

Note we cannot get the batfet voltage level from a register, just
batfet_disable == {0,1}. Should we alternatively consider doing this?
/sys/class...bq24190-charger/online = vbus_up && !batfet_disable

Otherwise, what are your thoughts on values for these attributes?
from https://kernel.org/doc/Documentation/ABI/testing/sysfs-class-regulator

/sys/class/regulator/.../state
/sys/class/regulator/.../status
/sys/class/regulator/.../type
/sys/class/regulator/.../name
/sys/class/regulator/.../num_users
/sys/class/regulator/.../parent

I assume the volts, amps, and suspend attributes aren't relevant.

I don't think we can broadcast batfet_disable changes via
regulator_notifier_call_chain(), since we don't see an interrupt on
batfet changes. From
https://kernel.org/doc/Documentation/power/regulator/regulator.txt

Any pointers to example code appreciated...




> That still leaves the health status reading, though. From
> userspace's point-of-view this should be merged into the
> battery device, if fuel-gauge can't provide that information.
>
> -- Sebastian