diff mbox

[4/4] dt-bindings: mfd: max8998: Add charger subnode binding

Message ID 1524844982-4714-5-git-send-email-pawel.mikolaj.chmiel@gmail.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Paweł Chmiel April 27, 2018, 4:03 p.m. UTC
This patch adds devicetree bindings documentation for
battery charging controller as the subnode of MAX8998 PMIC.
It's based on current behavior of driver.

Fixes: ee999fb3f17f ("mfd: max8998: Add support for Device Tree")
Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
---
 Documentation/devicetree/bindings/mfd/max8998.txt | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

Comments

Lee Jones April 30, 2018, 12:30 p.m. UTC | #1
On Fri, 27 Apr 2018, Paweł Chmiel wrote:

> This patch adds devicetree bindings documentation for
> battery charging controller as the subnode of MAX8998 PMIC.
> It's based on current behavior of driver.
> 
> Fixes: ee999fb3f17f ("mfd: max8998: Add support for Device Tree")

Why is this here?  This patch doesn't look like a fix to me.

> Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
> ---
>  Documentation/devicetree/bindings/mfd/max8998.txt | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/max8998.txt b/Documentation/devicetree/bindings/mfd/max8998.txt
> index 23a3650ff2a2..f95610afb57f 100644
> --- a/Documentation/devicetree/bindings/mfd/max8998.txt
> +++ b/Documentation/devicetree/bindings/mfd/max8998.txt
> @@ -50,6 +50,21 @@ Additional properties required if max8998,pmic-buck2-dvs-gpio is defined:
>  - max8998,pmic-buck2-dvs-voltage: An array of 2 voltage values in microvolts
>    for buck2 regulator that can be selected using dvs gpio.
>  
> +Charger: Configuration for battery charging controller should be added
> +inside a child node named 'charger'.
> +  Required properties:
> +  - max8998,charge-eoc: Setup "End of Charge". If value equals 0,
> +    remain value set from bootloader or default value will be used.
> +    Valid values: 0, 10 - 45
> +
> +  - max8998,charge-restart: Setup "Charge Restart Level". If value equals 0,
> +    remain value set from bootloader or default value will be used.
> +    Valid values: -1, 0, 100, 150, 200
> +
> +  - max8998,charge-timeout: Setup "Charge Full Timeout". If value equals 0,
> +    remain value set from bootloader or default value will be used.
> +    Valid values: -1, 0, 5, 6, 7
> +
>  Regulators: All the regulators of MAX8998 to be instantiated shall be
>  listed in a child node named 'regulators'. Each regulator is represented
>  by a child node of the 'regulators' node.
> @@ -99,6 +114,13 @@ Example:
>  		max8998,pmic-buck2-dvs-gpio = <&gpx0 0 3 0 0>; /* SET3 */
>  		max8998,pmic-buck2-dvs-voltage = <1350000>, <1300000>;
>  
> +		/* Charger configuration */
> +		charger {
> +			max8998,charge-eoc = <0>;
> +			max8998,charge-restart = <(-1)>;
> +			max8998,charge-timeout = <7>;
> +		};
> +
>  		/* Regulators to instantiate */
>  		regulators {
>  			ldo2_reg: LDO2 {
Paweł Chmiel April 30, 2018, 2:59 p.m. UTC | #2
On Monday, April 30, 2018 1:30:36 PM CEST Lee Jones wrote:
> On Fri, 27 Apr 2018, Paweł Chmiel wrote:
> 
> > This patch adds devicetree bindings documentation for
> > battery charging controller as the subnode of MAX8998 PMIC.
> > It's based on current behavior of driver.
> > 
> > Fixes: ee999fb3f17f ("mfd: max8998: Add support for Device Tree")
> 
> Why is this here?  This patch doesn't look like a fix to me.
Hi
I though that if previous patch, which is adding missing device tree parsing into charger driver,
has fixes tag, this patch (which is documenting it), should also contain that tag.
If it shouldn't be here, i can fix this in v2.

Thanks for feedback (this is my second patchset for Linux Kernel)
> 
> > Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
> > ---
> >  Documentation/devicetree/bindings/mfd/max8998.txt | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/mfd/max8998.txt b/Documentation/devicetree/bindings/mfd/max8998.txt
> > index 23a3650ff2a2..f95610afb57f 100644
> > --- a/Documentation/devicetree/bindings/mfd/max8998.txt
> > +++ b/Documentation/devicetree/bindings/mfd/max8998.txt
> > @@ -50,6 +50,21 @@ Additional properties required if max8998,pmic-buck2-dvs-gpio is defined:
> >  - max8998,pmic-buck2-dvs-voltage: An array of 2 voltage values in microvolts
> >    for buck2 regulator that can be selected using dvs gpio.
> >  
> > +Charger: Configuration for battery charging controller should be added
> > +inside a child node named 'charger'.
> > +  Required properties:
> > +  - max8998,charge-eoc: Setup "End of Charge". If value equals 0,
> > +    remain value set from bootloader or default value will be used.
> > +    Valid values: 0, 10 - 45
> > +
> > +  - max8998,charge-restart: Setup "Charge Restart Level". If value equals 0,
> > +    remain value set from bootloader or default value will be used.
> > +    Valid values: -1, 0, 100, 150, 200
> > +
> > +  - max8998,charge-timeout: Setup "Charge Full Timeout". If value equals 0,
> > +    remain value set from bootloader or default value will be used.
> > +    Valid values: -1, 0, 5, 6, 7
> > +
> >  Regulators: All the regulators of MAX8998 to be instantiated shall be
> >  listed in a child node named 'regulators'. Each regulator is represented
> >  by a child node of the 'regulators' node.
> > @@ -99,6 +114,13 @@ Example:
> >  		max8998,pmic-buck2-dvs-gpio = <&gpx0 0 3 0 0>; /* SET3 */
> >  		max8998,pmic-buck2-dvs-voltage = <1350000>, <1300000>;
> >  
> > +		/* Charger configuration */
> > +		charger {
> > +			max8998,charge-eoc = <0>;
> > +			max8998,charge-restart = <(-1)>;
> > +			max8998,charge-timeout = <7>;
> > +		};
> > +
> >  		/* Regulators to instantiate */
> >  		regulators {
> >  			ldo2_reg: LDO2 {
> 
>
Sebastian Reichel May 1, 2018, 12:45 p.m. UTC | #3
Hi,

On Fri, Apr 27, 2018 at 06:03:02PM +0200, Paweł Chmiel wrote:
> This patch adds devicetree bindings documentation for
> battery charging controller as the subnode of MAX8998 PMIC.
> It's based on current behavior of driver.
> 
> Fixes: ee999fb3f17f ("mfd: max8998: Add support for Device Tree")
> Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
> ---
>  Documentation/devicetree/bindings/mfd/max8998.txt | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/max8998.txt b/Documentation/devicetree/bindings/mfd/max8998.txt
> index 23a3650ff2a2..f95610afb57f 100644
> --- a/Documentation/devicetree/bindings/mfd/max8998.txt
> +++ b/Documentation/devicetree/bindings/mfd/max8998.txt
> @@ -50,6 +50,21 @@ Additional properties required if max8998,pmic-buck2-dvs-gpio is defined:
>  - max8998,pmic-buck2-dvs-voltage: An array of 2 voltage values in microvolts
>    for buck2 regulator that can be selected using dvs gpio.
>  
> +Charger: Configuration for battery charging controller should be added
> +inside a child node named 'charger'.
> +  Required properties:
> +  - max8998,charge-eoc: Setup "End of Charge". If value equals 0,
> +    remain value set from bootloader or default value will be used.
> +    Valid values: 0, 10 - 45
> +
> +  - max8998,charge-restart: Setup "Charge Restart Level". If value equals 0,
> +    remain value set from bootloader or default value will be used.
> +    Valid values: -1, 0, 100, 150, 200
> +
> +  - max8998,charge-timeout: Setup "Charge Full Timeout". If value equals 0,
> +    remain value set from bootloader or default value will be used.
> +    Valid values: -1, 0, 5, 6, 7

What are those values? seconds?

> +
>  Regulators: All the regulators of MAX8998 to be instantiated shall be
>  listed in a child node named 'regulators'. Each regulator is represented
>  by a child node of the 'regulators' node.
> @@ -99,6 +114,13 @@ Example:
>  		max8998,pmic-buck2-dvs-gpio = <&gpx0 0 3 0 0>; /* SET3 */
>  		max8998,pmic-buck2-dvs-voltage = <1350000>, <1300000>;
>  
> +		/* Charger configuration */
> +		charger {
> +			max8998,charge-eoc = <0>;
> +			max8998,charge-restart = <(-1)>;
> +			max8998,charge-timeout = <7>;
> +		};
> +
>  		/* Regulators to instantiate */
>  		regulators {
>  			ldo2_reg: LDO2 {
> -- 
> 2.7.4
>
Paweł Chmiel May 1, 2018, 2:43 p.m. UTC | #4
On Tuesday, May 1, 2018 2:45:40 PM CEST Sebastian Reichel wrote:
> Hi,
> 
> On Fri, Apr 27, 2018 at 06:03:02PM +0200, Paweł Chmiel wrote:
> > This patch adds devicetree bindings documentation for
> > battery charging controller as the subnode of MAX8998 PMIC.
> > It's based on current behavior of driver.
> > 
> > Fixes: ee999fb3f17f ("mfd: max8998: Add support for Device Tree")
> > Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
> > ---
> >  Documentation/devicetree/bindings/mfd/max8998.txt | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/mfd/max8998.txt b/Documentation/devicetree/bindings/mfd/max8998.txt
> > index 23a3650ff2a2..f95610afb57f 100644
> > --- a/Documentation/devicetree/bindings/mfd/max8998.txt
> > +++ b/Documentation/devicetree/bindings/mfd/max8998.txt
> > @@ -50,6 +50,21 @@ Additional properties required if max8998,pmic-buck2-dvs-gpio is defined:
> >  - max8998,pmic-buck2-dvs-voltage: An array of 2 voltage values in microvolts
> >    for buck2 regulator that can be selected using dvs gpio.
> >  
> > +Charger: Configuration for battery charging controller should be added
> > +inside a child node named 'charger'.
> > +  Required properties:
> > +  - max8998,charge-eoc: Setup "End of Charge". If value equals 0,
> > +    remain value set from bootloader or default value will be used.
> > +    Valid values: 0, 10 - 45
> > +
> > +  - max8998,charge-restart: Setup "Charge Restart Level". If value equals 0,
> > +    remain value set from bootloader or default value will be used.
> > +    Valid values: -1, 0, 100, 150, 200
> > +
> > +  - max8998,charge-timeout: Setup "Charge Full Timeout". If value equals 0,
> > +    remain value set from bootloader or default value will be used.
> > +    Valid values: -1, 0, 5, 6, 7
> 
> What are those values? seconds?
> 
Honestly i don't know. I've just documented values accepted currently by charger driver,
so we can use it from devicetree.
I couldn't find any max8998 datasheet with this information (units, possible values etc for those properties).

If this is not acceptable, i can drop this patch.

> > +
> >  Regulators: All the regulators of MAX8998 to be instantiated shall be
> >  listed in a child node named 'regulators'. Each regulator is represented
> >  by a child node of the 'regulators' node.
> > @@ -99,6 +114,13 @@ Example:
> >  		max8998,pmic-buck2-dvs-gpio = <&gpx0 0 3 0 0>; /* SET3 */
> >  		max8998,pmic-buck2-dvs-voltage = <1350000>, <1300000>;
> >  
> > +		/* Charger configuration */
> > +		charger {
> > +			max8998,charge-eoc = <0>;
> > +			max8998,charge-restart = <(-1)>;
> > +			max8998,charge-timeout = <7>;
> > +		};
> > +
> >  		/* Regulators to instantiate */
> >  		regulators {
> >  			ldo2_reg: LDO2 {
>
Hi,

On 5/1/18 16:43, Paweł Chmiel wrote:
> On Tuesday, May 1, 2018 2:45:40 PM CEST Sebastian Reichel wrote:
>> On Fri, Apr 27, 2018 at 06:03:02PM +0200, Paweł Chmiel wrote:
>>> This patch adds devicetree bindings documentation for
>>> battery charging controller as the subnode of MAX8998 PMIC.
>>> It's based on current behavior of driver.
>>>
>>> Fixes: ee999fb3f17f ("mfd: max8998: Add support for Device Tree")
>>> Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
>>> ---
>>>  Documentation/devicetree/bindings/mfd/max8998.txt | 22 ++++++++++++++++++++++
>>>  1 file changed, 22 insertions(+)

>>> --- a/Documentation/devicetree/bindings/mfd/max8998.txt
>>> +++ b/Documentation/devicetree/bindings/mfd/max8998.txt
>>> @@ -50,6 +50,21 @@ Additional properties required if max8998,pmic-buck2-dvs-gpio is defined:
>>>  - max8998,pmic-buck2-dvs-voltage: An array of 2 voltage values in microvolts
>>>    for buck2 regulator that can be selected using dvs gpio.
>>>  
>>> +Charger: Configuration for battery charging controller should be added
>>> +inside a child node named 'charger'.
>>> +  Required properties:
>>> +  - max8998,charge-eoc: Setup "End of Charge". If value equals 0,
>>> +    remain value set from bootloader or default value will be used.
>>> +    Valid values: 0, 10 - 45
>>> +
>>> +  - max8998,charge-restart: Setup "Charge Restart Level". If value equals 0,
>>> +    remain value set from bootloader or default value will be used.
>>> +    Valid values: -1, 0, 100, 150, 200

Perhaps change the property name to max8998,charge-restart-threshold, 
in include/linux/mfd/max8998.h we have:

 * @restart: Restart Level in mV: 100, 150, 200, and -1 for disable.
 *   If it equals 0, leave it unchanged.

Then we could make it an optional property:

 - max8998,charge-restart-threshold: Charge restart threshold in millivolts. 
   Valid values are: 0, 100, 150, 200. If the value equals 0 the charger 
   restart will be disabled.

If the property is missing the charger restart threshold configuration would
be left unchanged.

>>> +  - max8998,charge-timeout: Setup "Charge Full Timeout". If value equals 0,
>>> +    remain value set from bootloader or default value will be used.
>>> +    Valid values: -1, 0, 5, 6, 7
>>
>> What are those values? seconds?
>>
> Honestly i don't know. I've just documented values accepted currently 
> by charger driver, so we can use it from devicetree.
> I couldn't find any max8998 datasheet with this information (units, possible 
> values etc for those properties).

The charge timeout is in hours, as described in include/linux/mfd/max8998.h:


"* @timeout: Full Timeout in hours: 5, 6, 7, and -1 for disable.
 *   If it equals 0, leave it unchanged.
 *   Otherwise, leave it unchanged."

We could change description of the property to something along the lines of:

Optional properties:

  - max8998,charge-timeout: Charge timeout in hours. Valid values are:
    0, 5, 6, 7. If the value is 0 the charge timer will be disabled.

Then if the property is missing the driver will leave charge timer configuration
unchanged.
Paweł Chmiel Jan. 10, 2019, 8:11 p.m. UTC | #6
Hi

On Thursday, 10 January 2019 18:47:11 CET Sylwester Nawrocki wrote:
> Hi,
> 
> On 5/1/18 16:43, Paweł Chmiel wrote:
> > On Tuesday, May 1, 2018 2:45:40 PM CEST Sebastian Reichel wrote:
> >> On Fri, Apr 27, 2018 at 06:03:02PM +0200, Paweł Chmiel wrote:
> >>> This patch adds devicetree bindings documentation for
> >>> battery charging controller as the subnode of MAX8998 PMIC.
> >>> It's based on current behavior of driver.
> >>>
> >>> Fixes: ee999fb3f17f ("mfd: max8998: Add support for Device Tree")
> >>> Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
> >>> ---
> >>>  Documentation/devicetree/bindings/mfd/max8998.txt | 22 ++++++++++++++++++++++
> >>>  1 file changed, 22 insertions(+)
> 
> >>> --- a/Documentation/devicetree/bindings/mfd/max8998.txt
> >>> +++ b/Documentation/devicetree/bindings/mfd/max8998.txt
> >>> @@ -50,6 +50,21 @@ Additional properties required if max8998,pmic-buck2-dvs-gpio is defined:
> >>>  - max8998,pmic-buck2-dvs-voltage: An array of 2 voltage values in microvolts
> >>>    for buck2 regulator that can be selected using dvs gpio.
> >>>  
> >>> +Charger: Configuration for battery charging controller should be added
> >>> +inside a child node named 'charger'.
> >>> +  Required properties:
> >>> +  - max8998,charge-eoc: Setup "End of Charge". If value equals 0,
> >>> +    remain value set from bootloader or default value will be used.
> >>> +    Valid values: 0, 10 - 45
> >>> +
> >>> +  - max8998,charge-restart: Setup "Charge Restart Level". If value equals 0,
> >>> +    remain value set from bootloader or default value will be used.
> >>> +    Valid values: -1, 0, 100, 150, 200
> 
> Perhaps change the property name to max8998,charge-restart-threshold, 
> in include/linux/mfd/max8998.h we have:
> 
>  * @restart: Restart Level in mV: 100, 150, 200, and -1 for disable.
>  *   If it equals 0, leave it unchanged.
> 
> Then we could make it an optional property:
> 
>  - max8998,charge-restart-threshold: Charge restart threshold in millivolts. 
>    Valid values are: 0, 100, 150, 200. If the value equals 0 the charger 
>    restart will be disabled.
> 
> If the property is missing the charger restart threshold configuration would
> be left unchanged.
> 
Strange, i've looked at max8998 and didn't saw information that those values are in mV (maybe it was from vendor sources).

> >>> +  - max8998,charge-timeout: Setup "Charge Full Timeout". If value equals 0,
> >>> +    remain value set from bootloader or default value will be used.
> >>> +    Valid values: -1, 0, 5, 6, 7
> >>
> >> What are those values? seconds?
> >>
> > Honestly i don't know. I've just documented values accepted currently 
> > by charger driver, so we can use it from devicetree.
> > I couldn't find any max8998 datasheet with this information (units, possible 
> > values etc for those properties).
> 
> The charge timeout is in hours, as described in include/linux/mfd/max8998.h:
> 
> 
> "* @timeout: Full Timeout in hours: 5, 6, 7, and -1 for disable.
>  *   If it equals 0, leave it unchanged.
>  *   Otherwise, leave it unchanged."
> 
> We could change description of the property to something along the lines of:
> 
> Optional properties:
> 
>   - max8998,charge-timeout: Charge timeout in hours. Valid values are:
>     0, 5, 6, 7. If the value is 0 the charge timer will be disabled.
> 
> Then if the property is missing the driver will leave charge timer configuration
> unchanged.
> 
> 
Thanks Sylwester Nawrocki for all those hints (mostly regarding units of those magic numbers/values). I'll prepare new version of those patches with all hints included.
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/mfd/max8998.txt b/Documentation/devicetree/bindings/mfd/max8998.txt
index 23a3650ff2a2..f95610afb57f 100644
--- a/Documentation/devicetree/bindings/mfd/max8998.txt
+++ b/Documentation/devicetree/bindings/mfd/max8998.txt
@@ -50,6 +50,21 @@  Additional properties required if max8998,pmic-buck2-dvs-gpio is defined:
 - max8998,pmic-buck2-dvs-voltage: An array of 2 voltage values in microvolts
   for buck2 regulator that can be selected using dvs gpio.
 
+Charger: Configuration for battery charging controller should be added
+inside a child node named 'charger'.
+  Required properties:
+  - max8998,charge-eoc: Setup "End of Charge". If value equals 0,
+    remain value set from bootloader or default value will be used.
+    Valid values: 0, 10 - 45
+
+  - max8998,charge-restart: Setup "Charge Restart Level". If value equals 0,
+    remain value set from bootloader or default value will be used.
+    Valid values: -1, 0, 100, 150, 200
+
+  - max8998,charge-timeout: Setup "Charge Full Timeout". If value equals 0,
+    remain value set from bootloader or default value will be used.
+    Valid values: -1, 0, 5, 6, 7
+
 Regulators: All the regulators of MAX8998 to be instantiated shall be
 listed in a child node named 'regulators'. Each regulator is represented
 by a child node of the 'regulators' node.
@@ -99,6 +114,13 @@  Example:
 		max8998,pmic-buck2-dvs-gpio = <&gpx0 0 3 0 0>; /* SET3 */
 		max8998,pmic-buck2-dvs-voltage = <1350000>, <1300000>;
 
+		/* Charger configuration */
+		charger {
+			max8998,charge-eoc = <0>;
+			max8998,charge-restart = <(-1)>;
+			max8998,charge-timeout = <7>;
+		};
+
 		/* Regulators to instantiate */
 		regulators {
 			ldo2_reg: LDO2 {