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

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

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

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

Hi,

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

> ---
>  .../devicetree/bindings/power/bq24257.txt          | 21 -------
>  .../devicetree/bindings/power/bq2425x.txt          | 70 ++++++++++++++++++++++
>  2 files changed, 70 insertions(+), 21 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/power/bq24257.txt
>  create mode 100644 Documentation/devicetree/bindings/power/bq2425x.txt
> 
> diff --git a/Documentation/devicetree/bindings/power/bq24257.txt b/Documentation/devicetree/bindings/power/bq24257.txt
> deleted file mode 100644
> index 5c9d394..0000000
> --- a/Documentation/devicetree/bindings/power/bq24257.txt
> +++ /dev/null
> @@ -1,21 +0,0 @@
> -Binding for TI bq24257 Li-Ion Charger
> -
> -Required properties:
> -- compatible: Should contain one of the following:
> - * "ti,bq24257"
> -- reg:			   integer, i2c address of the device.
> -- ti,battery-regulation-voltage: integer, maximum charging voltage in uV.
> -- ti,charge-current:	   integer, maximum charging current in uA.
> -- ti,termination-current:  integer, charge will be terminated when current in
> -			   constant-voltage phase drops below this value (in uA).
> -
> -Example:
> -
> -bq24257 {
> -	compatible = "ti,bq24257";
> -	reg = <0x6a>;
> -
> -	ti,battery-regulation-voltage = <4200000>;
> -	ti,charge-current = <1000000>;
> -	ti,termination-current = <50000>;
> -};
> diff --git a/Documentation/devicetree/bindings/power/bq2425x.txt b/Documentation/devicetree/bindings/power/bq2425x.txt
> new file mode 100644
> index 0000000..3e171c3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/bq2425x.txt
> @@ -0,0 +1,70 @@
> +Binding for TI bq2425x Li-Ion Charger
> +
> +Required properties:
> +- compatible: Should contain one of the following:
> + * "ti,bq24250"
> + * "ti,bq24251"
> + * "ti,bq24257"
> +- reg: integer, i2c address of the device.
> +- stat-gpios: GPIO used for the devices STAT_IN pin. Alternatively the pin can
> +    also be defined through the standard interrupt definition properties (see
> +    optional properties section below). Only use one method.
> +- ti,battery-regulation-voltage: integer, maximum charging voltage in uV.
> +- ti,charge-current: integer, maximum charging current in uA.
> +- ti,termination-current: integer, charge will be terminated when current in
> +    constant-voltage phase drops below this value (in uA).
> +
> +Optional properties:
> +- pg-gpios: GPIO used for connecting the bq2425x device PG (Power Good) pin.
> +    This pin is not available on all devices however it should be used if
> +    possible as this is the recommended way to obtain the charger's input PG
> +    state. If this pin is not specified a software-based approach for PG
> +    detection is used.
> +- ti,current-limit: The maximum current to be drawn from the charger's input
> +    (in uA). If this property is not specified a USB D+/D- signal based charger
> +    type detection is used (if available) and the input limit current is set
> +    accordingly. If the D+/D- based detection is not available on a given device
> +    a default of 500,000 is used (=500mA).

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

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

Best regards,
Krzysztof

> +- ti,ovp-voltage: Configures the over voltage protection voltage (in uV). If
> +    not specified a default of 6,5000,000 (=6.5V) is used.
> +- ti,in-dpm-voltage: Configures the threshold input voltage for the dynamic
> +    power path management (in uV). If not specified a default of 4,360,000
> +    (=4.36V) is used.
> +- ti,safety-timer-2x-enable: If this property is set to 1 the device's safety
> +    timer is extended (slowed down) by a factor of two. Setting this property
> +    to 0 or not providing it will leave the safety timer at its default
> +    setting.
> +- interrupt-parent: Should be the phandle for the interrupt controller. Use in
> +    conjunction with "interrupts" and only in case "stat-gpios" is not used.
> +- interrupts: Interrupt mapping for GPIO IRQ (configure for both edges). Use in
> +    conjunction with "interrupt-parent" and only in case "stat-gpios" is not
> +    used.
> +
> +Example:
> +
> +bq24257 {
> +	compatible = "ti,bq24257";
> +	reg = <0x6a>;
> +	stat-gpios = <&gpio1 16 GPIO_ACTIVE_HIGH>;
> +	pg-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>;
> +
> +	ti,battery-regulation-voltage = <4200000>;
> +	ti,charge-current = <1000000>;
> +	ti,termination-current = <50000>;
> +};
> +
> +Example:
> +
> +bq24250 {
> +	compatible = "ti,bq24250";
> +	reg = <0x6a>;
> +	interrupt-parent = <&gpio1>;
> +	interrupts = <16 IRQ_TYPE_EDGE_BOTH>;
> +
> +	ti,battery-regulation-voltage = <4200000>;
> +	ti,charge-current = <500000>;
> +	ti,termination-current = <50000>;
> +	ti,current-limit = <900000>;
> +	ti,ovp-voltage = <9500000>;
> +	ti,in-dpm-voltage = <4440000>;
> +};
> 

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

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

Thanks for all your efforts and time checking those patches!

> 
> > ---
> >  .../devicetree/bindings/power/bq24257.txt          | 21 -------
> >  .../devicetree/bindings/power/bq2425x.txt          | 70 ++++++++++++++++++++++
> >  2 files changed, 70 insertions(+), 21 deletions(-)
> >  delete mode 100644 Documentation/devicetree/bindings/power/bq24257.txt
> >  create mode 100644 Documentation/devicetree/bindings/power/bq2425x.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/power/bq24257.txt b/Documentation/devicetree/bindings/power/bq24257.txt
> > deleted file mode 100644
> > index 5c9d394..0000000
> > --- a/Documentation/devicetree/bindings/power/bq24257.txt
> > +++ /dev/null
> > @@ -1,21 +0,0 @@
> > -Binding for TI bq24257 Li-Ion Charger
> > -
> > -Required properties:
> > -- compatible: Should contain one of the following:
> > - * "ti,bq24257"
> > -- reg:			   integer, i2c address of the device.
> > -- ti,battery-regulation-voltage: integer, maximum charging voltage in uV.
> > -- ti,charge-current:	   integer, maximum charging current in uA.
> > -- ti,termination-current:  integer, charge will be terminated when current in
> > -			   constant-voltage phase drops below this value (in uA).
> > -
> > -Example:
> > -
> > -bq24257 {
> > -	compatible = "ti,bq24257";
> > -	reg = <0x6a>;
> > -
> > -	ti,battery-regulation-voltage = <4200000>;
> > -	ti,charge-current = <1000000>;
> > -	ti,termination-current = <50000>;
> > -};
> > diff --git a/Documentation/devicetree/bindings/power/bq2425x.txt b/Documentation/devicetree/bindings/power/bq2425x.txt
> > new file mode 100644
> > index 0000000..3e171c3
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/power/bq2425x.txt
> > @@ -0,0 +1,70 @@
> > +Binding for TI bq2425x Li-Ion Charger
> > +
> > +Required properties:
> > +- compatible: Should contain one of the following:
> > + * "ti,bq24250"
> > + * "ti,bq24251"
> > + * "ti,bq24257"
> > +- reg: integer, i2c address of the device.
> > +- stat-gpios: GPIO used for the devices STAT_IN pin. Alternatively the pin can
> > +    also be defined through the standard interrupt definition properties (see
> > +    optional properties section below). Only use one method.
> > +- ti,battery-regulation-voltage: integer, maximum charging voltage in uV.
> > +- ti,charge-current: integer, maximum charging current in uA.
> > +- ti,termination-current: integer, charge will be terminated when current in
> > +    constant-voltage phase drops below this value (in uA).
> > +
> > +Optional properties:
> > +- pg-gpios: GPIO used for connecting the bq2425x device PG (Power Good) pin.
> > +    This pin is not available on all devices however it should be used if
> > +    possible as this is the recommended way to obtain the charger's input PG
> > +    state. If this pin is not specified a software-based approach for PG
> > +    detection is used.
> > +- ti,current-limit: The maximum current to be drawn from the charger's input
> > +    (in uA). If this property is not specified a USB D+/D- signal based charger
> > +    type detection is used (if available) and the input limit current is set
> > +    accordingly. If the D+/D- based detection is not available on a given device
> > +    a default of 500,000 is used (=500mA).
> 
> I understand this is different property than "ti,charge-current:
> integer, default charging current (in mA)" from bq24261_charger patch?

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

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

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

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

Regards,

--
Andreas Dannenberg
Texas Instruments Inc


> 
> Best regards,
> Krzysztof
> 
> > +- ti,ovp-voltage: Configures the over voltage protection voltage (in uV). If
> > +    not specified a default of 6,5000,000 (=6.5V) is used.
> > +- ti,in-dpm-voltage: Configures the threshold input voltage for the dynamic
> > +    power path management (in uV). If not specified a default of 4,360,000
> > +    (=4.36V) is used.
> > +- ti,safety-timer-2x-enable: If this property is set to 1 the device's safety
> > +    timer is extended (slowed down) by a factor of two. Setting this property
> > +    to 0 or not providing it will leave the safety timer at its default
> > +    setting.
> > +- interrupt-parent: Should be the phandle for the interrupt controller. Use in
> > +    conjunction with "interrupts" and only in case "stat-gpios" is not used.
> > +- interrupts: Interrupt mapping for GPIO IRQ (configure for both edges). Use in
> > +    conjunction with "interrupt-parent" and only in case "stat-gpios" is not
> > +    used.
> > +
> > +Example:
> > +
> > +bq24257 {
> > +	compatible = "ti,bq24257";
> > +	reg = <0x6a>;
> > +	stat-gpios = <&gpio1 16 GPIO_ACTIVE_HIGH>;
> > +	pg-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>;
> > +
> > +	ti,battery-regulation-voltage = <4200000>;
> > +	ti,charge-current = <1000000>;
> > +	ti,termination-current = <50000>;
> > +};
> > +
> > +Example:
> > +
> > +bq24250 {
> > +	compatible = "ti,bq24250";
> > +	reg = <0x6a>;
> > +	interrupt-parent = <&gpio1>;
> > +	interrupts = <16 IRQ_TYPE_EDGE_BOTH>;
> > +
> > +	ti,battery-regulation-voltage = <4200000>;
> > +	ti,charge-current = <500000>;
> > +	ti,termination-current = <50000>;
> > +	ti,current-limit = <900000>;
> > +	ti,ovp-voltage = <9500000>;
> > +	ti,in-dpm-voltage = <4440000>;
> > +};
> > 
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 09.09.2015 11:48, Andreas Dannenberg wrote:
> On Wed, Sep 09, 2015 at 09:27:06AM +0900, Krzysztof Kozlowski wrote:
>> On 09.09.2015 09:12, Andreas Dannenberg wrote:
>>> Extend the bq2425x charger's device tree documentation to cover the
>>> bq24250 and bq24257 devices as well as recent feature additions.
>>>
>>> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
>>
>> Hi,
>>
>> Thanks for updates. Everything pointed previous looks good. After
>> looking at Ramakrishna Pallala's (Cc-ed) patch ("power: bq24261_charger:
>> Add support for TI BQ24261 charger") I have only one comment below.
> 
> Thanks for all your efforts and time checking those patches!
> 
>>
>>> ---
>>>  .../devicetree/bindings/power/bq24257.txt          | 21 -------
>>>  .../devicetree/bindings/power/bq2425x.txt          | 70 ++++++++++++++++++++++
>>>  2 files changed, 70 insertions(+), 21 deletions(-)
>>>  delete mode 100644 Documentation/devicetree/bindings/power/bq24257.txt
>>>  create mode 100644 Documentation/devicetree/bindings/power/bq2425x.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/power/bq24257.txt b/Documentation/devicetree/bindings/power/bq24257.txt
>>> deleted file mode 100644
>>> index 5c9d394..0000000
>>> --- a/Documentation/devicetree/bindings/power/bq24257.txt
>>> +++ /dev/null
>>> @@ -1,21 +0,0 @@
>>> -Binding for TI bq24257 Li-Ion Charger
>>> -
>>> -Required properties:
>>> -- compatible: Should contain one of the following:
>>> - * "ti,bq24257"
>>> -- reg:			   integer, i2c address of the device.
>>> -- ti,battery-regulation-voltage: integer, maximum charging voltage in uV.
>>> -- ti,charge-current:	   integer, maximum charging current in uA.
>>> -- ti,termination-current:  integer, charge will be terminated when current in
>>> -			   constant-voltage phase drops below this value (in uA).
>>> -
>>> -Example:
>>> -
>>> -bq24257 {
>>> -	compatible = "ti,bq24257";
>>> -	reg = <0x6a>;
>>> -
>>> -	ti,battery-regulation-voltage = <4200000>;
>>> -	ti,charge-current = <1000000>;
>>> -	ti,termination-current = <50000>;
>>> -};
>>> diff --git a/Documentation/devicetree/bindings/power/bq2425x.txt b/Documentation/devicetree/bindings/power/bq2425x.txt
>>> new file mode 100644
>>> index 0000000..3e171c3
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/power/bq2425x.txt
>>> @@ -0,0 +1,70 @@
>>> +Binding for TI bq2425x Li-Ion Charger
>>> +
>>> +Required properties:
>>> +- compatible: Should contain one of the following:
>>> + * "ti,bq24250"
>>> + * "ti,bq24251"
>>> + * "ti,bq24257"
>>> +- reg: integer, i2c address of the device.
>>> +- stat-gpios: GPIO used for the devices STAT_IN pin. Alternatively the pin can
>>> +    also be defined through the standard interrupt definition properties (see
>>> +    optional properties section below). Only use one method.
>>> +- ti,battery-regulation-voltage: integer, maximum charging voltage in uV.
>>> +- ti,charge-current: integer, maximum charging current in uA.
>>> +- ti,termination-current: integer, charge will be terminated when current in
>>> +    constant-voltage phase drops below this value (in uA).
>>> +
>>> +Optional properties:
>>> +- pg-gpios: GPIO used for connecting the bq2425x device PG (Power Good) pin.
>>> +    This pin is not available on all devices however it should be used if
>>> +    possible as this is the recommended way to obtain the charger's input PG
>>> +    state. If this pin is not specified a software-based approach for PG
>>> +    detection is used.
>>> +- ti,current-limit: The maximum current to be drawn from the charger's input
>>> +    (in uA). If this property is not specified a USB D+/D- signal based charger
>>> +    type detection is used (if available) and the input limit current is set
>>> +    accordingly. If the D+/D- based detection is not available on a given device
>>> +    a default of 500,000 is used (=500mA).
>>
>> I understand this is different property than "ti,charge-current:
>> integer, default charging current (in mA)" from bq24261_charger patch?
> 
> Correct this is what "ti,current-limit" is for. And I borrowed that
> definition from bq2415x_charger.c where it's used in the same context.
> The reason for this property to exist is for systems where the external
> power supply does more than just charging the battery, such as supplying
> the system while it's charging or after it has finished charging. All
> this only makes sense of course if the "ti,current-limit" is larger than
> "ti,charge-current".
> 
> And if you see a few lines above the bq24257 driver also has a
> "ti,charge-current" property. And yes this is what actually controls how
> much current is delivered to the battery.

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

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

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


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

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

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

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


Best regards,
Krzysztof

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

On Wed, Sep 09, 2015 at 01:58:09PM +0900, Krzysztof Kozlowski wrote:
> On 09.09.2015 11:48, Andreas Dannenberg wrote:
> > On Wed, Sep 09, 2015 at 09:27:06AM +0900, Krzysztof Kozlowski wrote:
> >> On 09.09.2015 09:12, Andreas Dannenberg wrote:
> >>> Extend the bq2425x charger's device tree documentation to cover the
> >>> bq24250 and bq24257 devices as well as recent feature additions.
> >>>
> >>> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> >>
> >> Hi,
> >>
> >> Thanks for updates. Everything pointed previous looks good. After
> >> looking at Ramakrishna Pallala's (Cc-ed) patch ("power: bq24261_charger:
> >> Add support for TI BQ24261 charger") I have only one comment below.
> > 
> > Thanks for all your efforts and time checking those patches!
> > 
> >>
> >>> ---
> >>>  .../devicetree/bindings/power/bq24257.txt          | 21 -------
> >>>  .../devicetree/bindings/power/bq2425x.txt          | 70 ++++++++++++++++++++++
> >>>  2 files changed, 70 insertions(+), 21 deletions(-)
> >>>  delete mode 100644 Documentation/devicetree/bindings/power/bq24257.txt
> >>>  create mode 100644 Documentation/devicetree/bindings/power/bq2425x.txt
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/power/bq24257.txt b/Documentation/devicetree/bindings/power/bq24257.txt
> >>> deleted file mode 100644
> >>> index 5c9d394..0000000
> >>> --- a/Documentation/devicetree/bindings/power/bq24257.txt
> >>> +++ /dev/null
> >>> @@ -1,21 +0,0 @@
> >>> -Binding for TI bq24257 Li-Ion Charger
> >>> -
> >>> -Required properties:
> >>> -- compatible: Should contain one of the following:
> >>> - * "ti,bq24257"
> >>> -- reg:			   integer, i2c address of the device.
> >>> -- ti,battery-regulation-voltage: integer, maximum charging voltage in uV.
> >>> -- ti,charge-current:	   integer, maximum charging current in uA.
> >>> -- ti,termination-current:  integer, charge will be terminated when current in
> >>> -			   constant-voltage phase drops below this value (in uA).
> >>> -
> >>> -Example:
> >>> -
> >>> -bq24257 {
> >>> -	compatible = "ti,bq24257";
> >>> -	reg = <0x6a>;
> >>> -
> >>> -	ti,battery-regulation-voltage = <4200000>;
> >>> -	ti,charge-current = <1000000>;
> >>> -	ti,termination-current = <50000>;
> >>> -};
> >>> diff --git a/Documentation/devicetree/bindings/power/bq2425x.txt b/Documentation/devicetree/bindings/power/bq2425x.txt
> >>> new file mode 100644
> >>> index 0000000..3e171c3
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/power/bq2425x.txt
> >>> @@ -0,0 +1,70 @@
> >>> +Binding for TI bq2425x Li-Ion Charger
> >>> +
> >>> +Required properties:
> >>> +- compatible: Should contain one of the following:
> >>> + * "ti,bq24250"
> >>> + * "ti,bq24251"
> >>> + * "ti,bq24257"
> >>> +- reg: integer, i2c address of the device.
> >>> +- stat-gpios: GPIO used for the devices STAT_IN pin. Alternatively the pin can
> >>> +    also be defined through the standard interrupt definition properties (see
> >>> +    optional properties section below). Only use one method.
> >>> +- ti,battery-regulation-voltage: integer, maximum charging voltage in uV.
> >>> +- ti,charge-current: integer, maximum charging current in uA.
> >>> +- ti,termination-current: integer, charge will be terminated when current in
> >>> +    constant-voltage phase drops below this value (in uA).
> >>> +
> >>> +Optional properties:
> >>> +- pg-gpios: GPIO used for connecting the bq2425x device PG (Power Good) pin.
> >>> +    This pin is not available on all devices however it should be used if
> >>> +    possible as this is the recommended way to obtain the charger's input PG
> >>> +    state. If this pin is not specified a software-based approach for PG
> >>> +    detection is used.
> >>> +- ti,current-limit: The maximum current to be drawn from the charger's input
> >>> +    (in uA). If this property is not specified a USB D+/D- signal based charger
> >>> +    type detection is used (if available) and the input limit current is set
> >>> +    accordingly. If the D+/D- based detection is not available on a given device
> >>> +    a default of 500,000 is used (=500mA).
> >>
> >> I understand this is different property than "ti,charge-current:
> >> integer, default charging current (in mA)" from bq24261_charger patch?
> > 
> > Correct this is what "ti,current-limit" is for. And I borrowed that
> > definition from bq2415x_charger.c where it's used in the same context.
> > The reason for this property to exist is for systems where the external
> > power supply does more than just charging the battery, such as supplying
> > the system while it's charging or after it has finished charging. All
> > this only makes sense of course if the "ti,current-limit" is larger than
> > "ti,charge-current".
> > 
> > And if you see a few lines above the bq24257 driver also has a
> > "ti,charge-current" property. And yes this is what actually controls how
> > much current is delivered to the battery.
> 
> Right, I missed that one. Everything is clear now.
> 
> So we have different bindings. Existing ones:
> bq2415x.txt - ti,charge-current - maximum charging current in mA
> bq24257.txt - ti,charge-current - maximum charging current in uA
> bq25890.txt - ti,charge-current - maximum charging current in uA
> bq24735.txt - ti,charge-current - charge current (?) in mA

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Regards,

--
Andreas Dannenberg
Texas Instruments Inc
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

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

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

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

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

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

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


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

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

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

I agree.

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

I agree but wait for some more data from Ramakrishna.

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

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

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

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

Okay.

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

Okay.

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

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

Best regards,
Krzysztof

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

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

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

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

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

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

Understood.

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

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

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

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

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

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

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

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

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

Regards,

--
Andreas Dannenberg
Texas Instruments Inc
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[...]

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

Hi Laurentiu,

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

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

Regards,

--
Andreas Dannenberg
Texas Instruments Inc
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

(...)

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

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

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

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

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

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

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

Best regards,
Krzysztof

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

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

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

Regards,

--
Andreas Dannenberg
Texas Instruments Inc

 
> It's up to you people. Just make a first step and we'll see how other
> people react (and what Sebastian thinks about it :) ).
> 
> Best regards,
> Krzysztof
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html