diff mbox

[v7,3/4] dt-bindings: power: supply: qcom_bms: Add bindings

Message ID 20180614151435.6471-3-ctatlor97@gmail.com (mailing list archive)
State Not Applicable, archived
Delegated to: Andy Gross
Headers show

Commit Message

Craig Tatlor June 14, 2018, 3:14 p.m. UTC
Add bindings for the Qualcomm Battery Monitoring system.

Signed-off-by: Craig Tatlor <ctatlor97@gmail.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---

* Changes from v6:
  s/celcius/celsius
  change uah to uAh.

* Changes from v5:                                                                                                                                                                                                   
  Mentions which values are 8 bit.                                                                                                                                                                                   
                                                                                                                                                                                                                     
* Changes from v4:                                                                                                                                                                                                   
  Uses proper units and expands some definitions,                                                                                                                                                                    
  along with changing vadc@ to adc@.    

 .../bindings/power/supply/qcom_bms.txt        | 92 +++++++++++++++++++
 1 file changed, 92 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/supply/qcom_bms.txt

Comments

Sebastian Reichel Sept. 16, 2018, 12:10 p.m. UTC | #1
Hi,

Sorry for my long delay in reviewing this. I like the binding,
but the "qcom," specific properties should become common properties
in

Documentation/devicetree/bindings/power/supply/battery.txt

and referenced via monitored-battery.

-- Sebastian

On Thu, Jun 14, 2018 at 04:14:16PM +0100, Craig Tatlor wrote:
> Add bindings for the Qualcomm Battery Monitoring system.
> 
> Signed-off-by: Craig Tatlor <ctatlor97@gmail.com>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---
> 
> * Changes from v6:
>   s/celcius/celsius
>   change uah to uAh.
> 
> * Changes from v5:                                                                                                                                                                                                   
>   Mentions which values are 8 bit.                                                                                                                                                                                   
>                                                                                                                                                                                                                      
> * Changes from v4:                                                                                                                                                                                                   
>   Uses proper units and expands some definitions,                                                                                                                                                                    
>   along with changing vadc@ to adc@.    
> 
>  .../bindings/power/supply/qcom_bms.txt        | 92 +++++++++++++++++++
>  1 file changed, 92 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/supply/qcom_bms.txt
> 
> diff --git a/Documentation/devicetree/bindings/power/supply/qcom_bms.txt b/Documentation/devicetree/bindings/power/supply/qcom_bms.txt
> new file mode 100644
> index 000000000000..dc0a9ab9aa64
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/qcom_bms.txt
> @@ -0,0 +1,92 @@
> +Qualcomm Battery Monitoring System
> +
> +The Qualcomm Battery Monitoring System is found inside of Qualcomm PM8941
> +PMICs. It provides open circuit voltage (OCV) and coulomb counter registers
> +that allow the OS to infer a capacity level.
> +
> +Required properties:
> +- compatible:                   Should contain "qcom,pm8941-bms".
> +- reg:                          Specifies the SPMI address and length of the
> +				controller's registers.
> +- interrupts:                   OCV threshold interrupt.
> +- io-channels:                  Should contain IIO channel specifier for the
> +				ADC channel that reports battery temperature.
> +- io-channel-names:             Should contain "temp".
> +- qcom,fcc-temp-legend-celsius: An 8 bit array containing the temperature,
> +				in degC, for each column of the full charge
> +				capacity lookup table.
> +- qcom,fcc-lut-microamp-hours:  An array of full charge capacity values in uAh,
> +				one entry for each temperature defined in in
> +				qcom,fcc-temp-legend-celsius.
> +- qcom,ocv-temp-legend-celsius: An 8 bit array containing the temperature,
> +				in degC, for each column of the OCV lookup
> +				table.
> +- qcom,ocv-capacity-legend:     An 8 bit array containing the capacity for each
> +				row of the OCV lookup table.
> +- qcom,ocv-lut-microvolt:       An array of OCV values in uV, one entry for each
> +				capacity defined in qcom,ocv-capacity-legend.
> +
> +Example:
> +		pm8941_vadc: adc@3100 {
> +			compatible = "qcom,spmi-vadc";
> +			reg = <0x3100>;
> +			interrupts = <0x0 0x31 0x0 IRQ_TYPE_EDGE_RISING>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			#io-channel-cells = <1>;
> +
> +			bat_temp {
> +				reg = <VADC_LR_MUX1_BAT_THERM>;
> +			};
> +		};
> +
> +		bms@4000 {
> +			compatible = "qcom,pm8941-bms";
> +			reg = <0x4000>;
> +			interrupts = <0x0 0x40 0x4 IRQ_TYPE_EDGE_RISING>;
> +			interrupt-names = "ocv_thr";
> +
> +			io-channels = <&pm8941_vadc VADC_LR_MUX1_BAT_THERM>;
> +			io-channel-names = "temp";
> +
> +			qcom,fcc-temp-legend-celsius = /bits/ 8 <(-10) 0 25 50 65>;
> +			qcom,fcc-lut-microamp-hours = <3230000 3260000 3380000 3410000 3360000>;
> +
> +			qcom,ocv-capacity-legend = /bits/ 8 <100 95 90 85 80 75
> +							     70 65 60 55 50 45
> +							     40 35 30 25 20 15
> +							     10 9 8 7 6 5 4 3 2
> +							     1 0>;
> +			qcom,ocv-temp-legend-celsius = /bits/ 8 <(-10) 0 25 50 65>;
> +			qcom,ocv-lut-microvolt = <43050000 43050000 43030000 42990000 42950000
> +						  42770000 42570000 42550000 42510000 42310000
> +						  42180000 41980000 41970000 41920000 41720000
> +						  41590000 41390000 41450000 41400000 41200000
> +						  41010000 40810000 40920000 40890000 40690000
> +						  40480000 40280000 40440000 40420000 40220000
> +						  40040000 39840000 40010000 39980000 39780000
> +						  39620000 39420000 39550000 39560000 39360000
> +						  39210000 39010000 39090000 39160000 38960000
> +						  38830000 38630000 38740000 38790000 38590000
> +						  38550000 38350000 38440000 38430000 38230000
> +						  38310000 38110000 38230000 38180000 37980000
> +						  38190000 37990000 38040000 38000000 37800000
> +						  38060000 37860000 37900000 37840000 37640000
> +						  37890000 37690000 37770000 37660000 37460000
> +						  37720000 37520000 37560000 37450000 37250000
> +						  37480000 37280000 37290000 37250000 37050000
> +						  37240000 37040000 37020000 36990000 36790000
> +						  37030000 36830000 36730000 36700000 36500000
> +						  36940000 36740000 36670000 36640000 36440000
> +						  36850000 36650000 36600000 36590000 36390000
> +						  36750000 36550000 36520000 36550000 36350000
> +						  36690000 36490000 36380000 36400000 36200000
> +						  36460000 36260000 36180000 36120000 35920000
> +						  36080000 35880000 35680000 35640000 35440000
> +						  35510000 35310000 35050000 35020000 34820000
> +						  34730000 34530000 34300000 34250000 34050000
> +						  33870000 33670000 33040000 32820000 32620000
> +						  30000000 30000000 30000000 30000000 30000000>;
> +		};
> +	};
> +};
> -- 
> 2.17.0
>
Craig Tatlor Sept. 20, 2018, 2:32 p.m. UTC | #2
On 16 September 2018 13:10:45 BST, Sebastian Reichel <sebastian.reichel@collabora.com> wrote:
>Hi,
>
>Sorry for my long delay in reviewing this. I like the binding,
>but the "qcom," specific properties should become common properties
>in
>
>Documentation/devicetree/bindings/power/supply/battery.txt
Thanks for the review, what bindings for ocv would you prefer? The spreadtrum ones or mine?
>and referenced via monitored-battery.
>
>-- Sebastian
>
>On Thu, Jun 14, 2018 at 04:14:16PM +0100, Craig Tatlor wrote:
>> Add bindings for the Qualcomm Battery Monitoring system.
>> 
>> Signed-off-by: Craig Tatlor <ctatlor97@gmail.com>
>> Reviewed-by: Rob Herring <robh@kernel.org>
>> ---
>> 
>> * Changes from v6:
>>   s/celcius/celsius
>>   change uah to uAh.
>> 
>> * Changes from v5:                                                   
>                                                                       
>>   Mentions which values are 8 bit.                                   
>                                                                       
>>                                                                      
>                                                                       
>> * Changes from v4:                                                   
>                                                                       
>>   Uses proper units and expands some definitions,                    
>                                                                       
>>   along with changing vadc@ to adc@.    
>> 
>>  .../bindings/power/supply/qcom_bms.txt        | 92
>+++++++++++++++++++
>>  1 file changed, 92 insertions(+)
>>  create mode 100644
>Documentation/devicetree/bindings/power/supply/qcom_bms.txt
>> 
>> diff --git
>a/Documentation/devicetree/bindings/power/supply/qcom_bms.txt
>b/Documentation/devicetree/bindings/power/supply/qcom_bms.txt
>> new file mode 100644
>> index 000000000000..dc0a9ab9aa64
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/supply/qcom_bms.txt
>> @@ -0,0 +1,92 @@
>> +Qualcomm Battery Monitoring System
>> +
>> +The Qualcomm Battery Monitoring System is found inside of Qualcomm
>PM8941
>> +PMICs. It provides open circuit voltage (OCV) and coulomb counter
>registers
>> +that allow the OS to infer a capacity level.
>> +
>> +Required properties:
>> +- compatible:                   Should contain "qcom,pm8941-bms".
>> +- reg:                          Specifies the SPMI address and
>length of the
>> +				controller's registers.
>> +- interrupts:                   OCV threshold interrupt.
>> +- io-channels:                  Should contain IIO channel specifier
>for the
>> +				ADC channel that reports battery temperature.
>> +- io-channel-names:             Should contain "temp".
>> +- qcom,fcc-temp-legend-celsius: An 8 bit array containing the
>temperature,
>> +				in degC, for each column of the full charge
>> +				capacity lookup table.
>> +- qcom,fcc-lut-microamp-hours:  An array of full charge capacity
>values in uAh,
>> +				one entry for each temperature defined in in
>> +				qcom,fcc-temp-legend-celsius.
>> +- qcom,ocv-temp-legend-celsius: An 8 bit array containing the
>temperature,
>> +				in degC, for each column of the OCV lookup
>> +				table.
>> +- qcom,ocv-capacity-legend:     An 8 bit array containing the
>capacity for each
>> +				row of the OCV lookup table.
>> +- qcom,ocv-lut-microvolt:       An array of OCV values in uV, one
>entry for each
>> +				capacity defined in qcom,ocv-capacity-legend.
>> +
>> +Example:
>> +		pm8941_vadc: adc@3100 {
>> +			compatible = "qcom,spmi-vadc";
>> +			reg = <0x3100>;
>> +			interrupts = <0x0 0x31 0x0 IRQ_TYPE_EDGE_RISING>;
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +			#io-channel-cells = <1>;
>> +
>> +			bat_temp {
>> +				reg = <VADC_LR_MUX1_BAT_THERM>;
>> +			};
>> +		};
>> +
>> +		bms@4000 {
>> +			compatible = "qcom,pm8941-bms";
>> +			reg = <0x4000>;
>> +			interrupts = <0x0 0x40 0x4 IRQ_TYPE_EDGE_RISING>;
>> +			interrupt-names = "ocv_thr";
>> +
>> +			io-channels = <&pm8941_vadc VADC_LR_MUX1_BAT_THERM>;
>> +			io-channel-names = "temp";
>> +
>> +			qcom,fcc-temp-legend-celsius = /bits/ 8 <(-10) 0 25 50 65>;
>> +			qcom,fcc-lut-microamp-hours = <3230000 3260000 3380000 3410000
>3360000>;
>> +
>> +			qcom,ocv-capacity-legend = /bits/ 8 <100 95 90 85 80 75
>> +							     70 65 60 55 50 45
>> +							     40 35 30 25 20 15
>> +							     10 9 8 7 6 5 4 3 2
>> +							     1 0>;
>> +			qcom,ocv-temp-legend-celsius = /bits/ 8 <(-10) 0 25 50 65>;
>> +			qcom,ocv-lut-microvolt = <43050000 43050000 43030000 42990000
>42950000
>> +						  42770000 42570000 42550000 42510000 42310000
>> +						  42180000 41980000 41970000 41920000 41720000
>> +						  41590000 41390000 41450000 41400000 41200000
>> +						  41010000 40810000 40920000 40890000 40690000
>> +						  40480000 40280000 40440000 40420000 40220000
>> +						  40040000 39840000 40010000 39980000 39780000
>> +						  39620000 39420000 39550000 39560000 39360000
>> +						  39210000 39010000 39090000 39160000 38960000
>> +						  38830000 38630000 38740000 38790000 38590000
>> +						  38550000 38350000 38440000 38430000 38230000
>> +						  38310000 38110000 38230000 38180000 37980000
>> +						  38190000 37990000 38040000 38000000 37800000
>> +						  38060000 37860000 37900000 37840000 37640000
>> +						  37890000 37690000 37770000 37660000 37460000
>> +						  37720000 37520000 37560000 37450000 37250000
>> +						  37480000 37280000 37290000 37250000 37050000
>> +						  37240000 37040000 37020000 36990000 36790000
>> +						  37030000 36830000 36730000 36700000 36500000
>> +						  36940000 36740000 36670000 36640000 36440000
>> +						  36850000 36650000 36600000 36590000 36390000
>> +						  36750000 36550000 36520000 36550000 36350000
>> +						  36690000 36490000 36380000 36400000 36200000
>> +						  36460000 36260000 36180000 36120000 35920000
>> +						  36080000 35880000 35680000 35640000 35440000
>> +						  35510000 35310000 35050000 35020000 34820000
>> +						  34730000 34530000 34300000 34250000 34050000
>> +						  33870000 33670000 33040000 32820000 32620000
>> +						  30000000 30000000 30000000 30000000 30000000>;
>> +		};
>> +	};
>> +};
>> -- 
>> 2.17.0
>>
Sebastian Reichel Sept. 20, 2018, 4:58 p.m. UTC | #3
[Dropped a couple of people from CC, added Baolin]

Hi Craig, Baolin and Rob,

On Thu, Sep 20, 2018 at 03:32:29PM +0100, Craig wrote:
> On 16 September 2018 13:10:45 BST, Sebastian Reichel <sebastian.reichel@collabora.com> wrote:
> >Sorry for my long delay in reviewing this. I like the binding,
> >but the "qcom," specific properties should become common properties
> >in
> >
> >Documentation/devicetree/bindings/power/supply/battery.txt
> >and referenced via monitored-battery.

> Thanks for the review, what bindings for ocv would you prefer? The
> spreadtrum ones or mine?

Most importantly I want to see only one generic binding supporting
both use cases. As far as I can see there are two major differences:

1. Qcom uses legend properties and SC27XX embedds this into data
2. Qcom supports temperature based mapping

The second point is easy: Not having temperature information can
be a subset of the data with temperature info. The main thing to
discuss are the legend properties. I suppose we have these
proposals:

Proposal A (from Qcom BMS binding):

ocv-capacity-legend = /bits/ 8 <100 95 90 85 80 75 70 65 60 55 50 45 ...>;
ocv-temp-legend-celsius = /bits/ 8 <(-10) 0 25 50 65>;
ocv-lut-microvolt = <43050000 43050000 43030000 42990000

Proposal B (from SC27XX binding):

ocv-cap-table = <4185 100>, <4113 95>, <4066 90>, <4022 85> ...;

I prefer the second binding (with mV -> uV), but I think it becomes
messy when temperature is added. What do you think about the
following proposal (derived from pinctrl style):

Proposal C:

ocv-capacity-table-temperatures = <(-10) 0 10>;
ocv-capacity-table-0 = <4185000 100>, <4113000 95>, <4066000 90>, ...;
ocv-capacity-table-1 = <4200000 100>, <4185000 95>, <4113000 90>, ...;
ocv-capacity-table-2 = <4250000 100>, <4200000 95>, <4185000 90>, ...;

-- Sebastian
Craig Tatlor Sept. 20, 2018, 7:13 p.m. UTC | #4
On 20 September 2018 17:58:47 BST, Sebastian Reichel <sebastian.reichel@collabora.com> wrote:
>[Dropped a couple of people from CC, added Baolin]
>
>Hi Craig, Baolin and Rob,
>
>On Thu, Sep 20, 2018 at 03:32:29PM +0100, Craig wrote:
>> On 16 September 2018 13:10:45 BST, Sebastian Reichel
><sebastian.reichel@collabora.com> wrote:
>> >Sorry for my long delay in reviewing this. I like the binding,
>> >but the "qcom," specific properties should become common properties
>> >in
>> >
>> >Documentation/devicetree/bindings/power/supply/battery.txt
>> >and referenced via monitored-battery.
>
>> Thanks for the review, what bindings for ocv would you prefer? The
>> spreadtrum ones or mine?
>
>Most importantly I want to see only one generic binding supporting
>both use cases. As far as I can see there are two major differences:
>
>1. Qcom uses legend properties and SC27XX embedds this into data
>2. Qcom supports temperature based mapping
>
>The second point is easy: Not having temperature information can
>be a subset of the data with temperature info. The main thing to
>discuss are the legend properties. I suppose we have these
>proposals:
>
>Proposal A (from Qcom BMS binding):
>
>ocv-capacity-legend = /bits/ 8 <100 95 90 85 80 75 70 65 60 55 50 45
>...>;
>ocv-temp-legend-celsius = /bits/ 8 <(-10) 0 25 50 65>;
>ocv-lut-microvolt = <43050000 43050000 43030000 42990000
>
>Proposal B (from SC27XX binding):
>
>ocv-cap-table = <4185 100>, <4113 95>, <4066 90>, <4022 85> ...;
>
>I prefer the second binding (with mV -> uV), but I think it becomes
>messy when temperature is added. What do you think about the
>following proposal (derived from pinctrl style):
>
>Proposal C:
>
>ocv-capacity-table-temperatures = <(-10) 0 10>;
>ocv-capacity-table-0 = <4185000 100>, <4113000 95>, <4066000 90>, ...;
>ocv-capacity-table-1 = <4200000 100>, <4185000 95>, <4113000 90>, ...;
>ocv-capacity-table-2 = <4250000 100>, <4200000 95>, <4185000 90>, ...;
>
>-- Sebastian

C looks good to me however I do kinda think it should be millivolts as I don't think any hardware reads in microvolts and the zeroes make it look quite ugly
(Exiting) Baolin Wang Sept. 20, 2018, 8:08 p.m. UTC | #5
Hi Sebastian,

On 21 September 2018 at 00:58, Sebastian Reichel
<sebastian.reichel@collabora.com> wrote:
> [Dropped a couple of people from CC, added Baolin]
>
> Hi Craig, Baolin and Rob,
>
> On Thu, Sep 20, 2018 at 03:32:29PM +0100, Craig wrote:
>> On 16 September 2018 13:10:45 BST, Sebastian Reichel <sebastian.reichel@collabora.com> wrote:
>> >Sorry for my long delay in reviewing this. I like the binding,
>> >but the "qcom," specific properties should become common properties
>> >in
>> >
>> >Documentation/devicetree/bindings/power/supply/battery.txt
>> >and referenced via monitored-battery.
>
>> Thanks for the review, what bindings for ocv would you prefer? The
>> spreadtrum ones or mine?
>
> Most importantly I want to see only one generic binding supporting
> both use cases. As far as I can see there are two major differences:
>
> 1. Qcom uses legend properties and SC27XX embedds this into data
> 2. Qcom supports temperature based mapping
>
> The second point is easy: Not having temperature information can
> be a subset of the data with temperature info. The main thing to
> discuss are the legend properties. I suppose we have these
> proposals:
>
> Proposal A (from Qcom BMS binding):
>
> ocv-capacity-legend = /bits/ 8 <100 95 90 85 80 75 70 65 60 55 50 45 ...>;
> ocv-temp-legend-celsius = /bits/ 8 <(-10) 0 25 50 65>;
> ocv-lut-microvolt = <43050000 43050000 43030000 42990000
>
> Proposal B (from SC27XX binding):
>
> ocv-cap-table = <4185 100>, <4113 95>, <4066 90>, <4022 85> ...;
>
> I prefer the second binding (with mV -> uV), but I think it becomes
> messy when temperature is added. What do you think about the
> following proposal (derived from pinctrl style):
>
> Proposal C:
>
> ocv-capacity-table-temperatures = <(-10) 0 10>;
> ocv-capacity-table-0 = <4185000 100>, <4113000 95>, <4066000 90>, ...;
> ocv-capacity-table-1 = <4200000 100>, <4185000 95>, <4113000 90>, ...;
> ocv-capacity-table-2 = <4250000 100>, <4200000 95>, <4185000 90>, ...;

For SC27XX, we have no temperatures consideration, but I think
Proposal C can be compatible with our case.
Sebastian Reichel Sept. 20, 2018, 9:42 p.m. UTC | #6
On Thu, Sep 20, 2018 at 08:13:52PM +0100, Craig wrote:
> On 20 September 2018 17:58:47 BST, Sebastian Reichel <sebastian.reichel@collabora.com> wrote:
> >[Dropped a couple of people from CC, added Baolin]
> >
> >Hi Craig, Baolin and Rob,
> >
> >On Thu, Sep 20, 2018 at 03:32:29PM +0100, Craig wrote:
> >> On 16 September 2018 13:10:45 BST, Sebastian Reichel
> ><sebastian.reichel@collabora.com> wrote:
> >> >Sorry for my long delay in reviewing this. I like the binding,
> >> >but the "qcom," specific properties should become common properties
> >> >in
> >> >
> >> >Documentation/devicetree/bindings/power/supply/battery.txt
> >> >and referenced via monitored-battery.
> >
> >> Thanks for the review, what bindings for ocv would you prefer? The
> >> spreadtrum ones or mine?
> >
> >Most importantly I want to see only one generic binding supporting
> >both use cases. As far as I can see there are two major differences:
> >
> >1. Qcom uses legend properties and SC27XX embedds this into data
> >2. Qcom supports temperature based mapping
> >
> >The second point is easy: Not having temperature information can
> >be a subset of the data with temperature info. The main thing to
> >discuss are the legend properties. I suppose we have these
> >proposals:
> >
> >Proposal A (from Qcom BMS binding):
> >
> >ocv-capacity-legend = /bits/ 8 <100 95 90 85 80 75 70 65 60 55 50 45
> >...>;
> >ocv-temp-legend-celsius = /bits/ 8 <(-10) 0 25 50 65>;
> >ocv-lut-microvolt = <43050000 43050000 43030000 42990000
> >
> >Proposal B (from SC27XX binding):
> >
> >ocv-cap-table = <4185 100>, <4113 95>, <4066 90>, <4022 85> ...;
> >
> >I prefer the second binding (with mV -> uV), but I think it becomes
> >messy when temperature is added. What do you think about the
> >following proposal (derived from pinctrl style):
> >
> >Proposal C:
> >
> >ocv-capacity-table-temperatures = <(-10) 0 10>;
> >ocv-capacity-table-0 = <4185000 100>, <4113000 95>, <4066000 90>, ...;
> >ocv-capacity-table-1 = <4200000 100>, <4185000 95>, <4113000 90>, ...;
> >ocv-capacity-table-2 = <4250000 100>, <4200000 95>, <4185000 90>, ...;
> >
> >-- Sebastian
> 
> C looks good to me however I do kinda think it should be
> millivolts as I don't think any hardware reads in microvolts and
> the zeroes make it look quite ugly

I agree, that it looks a bit ugly in the table. Nevertheless I think we
should use microvolts, since that is being used by all other properties.

-- Sebastian
Sebastian Reichel Sept. 20, 2018, 10:13 p.m. UTC | #7
Hi,

On Fri, Sep 21, 2018 at 04:08:28AM +0800, Baolin Wang wrote:
> Hi Sebastian,
> 
> On 21 September 2018 at 00:58, Sebastian Reichel
> <sebastian.reichel@collabora.com> wrote:
> > [Dropped a couple of people from CC, added Baolin]
> >
> > Hi Craig, Baolin and Rob,
> >
> > On Thu, Sep 20, 2018 at 03:32:29PM +0100, Craig wrote:
> >> On 16 September 2018 13:10:45 BST, Sebastian Reichel <sebastian.reichel@collabora.com> wrote:
> >> >Sorry for my long delay in reviewing this. I like the binding,
> >> >but the "qcom," specific properties should become common properties
> >> >in
> >> >
> >> >Documentation/devicetree/bindings/power/supply/battery.txt
> >> >and referenced via monitored-battery.
> >
> >> Thanks for the review, what bindings for ocv would you prefer? The
> >> spreadtrum ones or mine?
> >
> > Most importantly I want to see only one generic binding supporting
> > both use cases. As far as I can see there are two major differences:
> >
> > 1. Qcom uses legend properties and SC27XX embedds this into data
> > 2. Qcom supports temperature based mapping
> >
> > The second point is easy: Not having temperature information can
> > be a subset of the data with temperature info. The main thing to
> > discuss are the legend properties. I suppose we have these
> > proposals:
> >
> > Proposal A (from Qcom BMS binding):
> >
> > ocv-capacity-legend = /bits/ 8 <100 95 90 85 80 75 70 65 60 55 50 45 ...>;
> > ocv-temp-legend-celsius = /bits/ 8 <(-10) 0 25 50 65>;
> > ocv-lut-microvolt = <43050000 43050000 43030000 42990000
> >
> > Proposal B (from SC27XX binding):
> >
> > ocv-cap-table = <4185 100>, <4113 95>, <4066 90>, <4022 85> ...;
> >
> > I prefer the second binding (with mV -> uV), but I think it becomes
> > messy when temperature is added. What do you think about the
> > following proposal (derived from pinctrl style):
> >
> > Proposal C:
> >
> > ocv-capacity-table-temperatures = <(-10) 0 10>;
> > ocv-capacity-table-0 = <4185000 100>, <4113000 95>, <4066000 90>, ...;
> > ocv-capacity-table-1 = <4200000 100>, <4185000 95>, <4113000 90>, ...;
> > ocv-capacity-table-2 = <4250000 100>, <4200000 95>, <4185000 90>, ...;
> 
> For SC27XX, we have no temperatures consideration, but I think
> Proposal C can be compatible with our case.

Yes. I think for SC27XX proposal C could be used like this:

ocv-capacity-table-temperatures = <20>; /* room temperature */
ocv-capacity-table-0 = <4185000 100>, <4113000 95>, <4066000 90>, ...;

With only one curve defined it would be used for all temperatures.

-- Sebastian
Linus Walleij Sept. 21, 2018, 3:40 p.m. UTC | #8
On Thu, Sep 20, 2018 at 9:58 AM Sebastian Reichel
<sebastian.reichel@collabora.com> wrote:

> I prefer the second binding (with mV -> uV), but I think it becomes
> messy when temperature is added. What do you think about the
> following proposal (derived from pinctrl style):
>
> Proposal C:
>
> ocv-capacity-table-temperatures = <(-10) 0 10>;
> ocv-capacity-table-0 = <4185000 100>, <4113000 95>, <4066000 90>, ...;
> ocv-capacity-table-1 = <4200000 100>, <4185000 95>, <4113000 90>, ...;
> ocv-capacity-table-2 = <4250000 100>, <4200000 95>, <4185000 90>, ...;

I think this looks very elegant. It's going to be very
easy and intuitive for people who need to maintain these
device trees and it will be easy to handle in centralized code
for all platforms. Even with the microvolt notation.

If possible could we go with this?

Yours,
Linus Walleij
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/power/supply/qcom_bms.txt b/Documentation/devicetree/bindings/power/supply/qcom_bms.txt
new file mode 100644
index 000000000000..dc0a9ab9aa64
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/supply/qcom_bms.txt
@@ -0,0 +1,92 @@ 
+Qualcomm Battery Monitoring System
+
+The Qualcomm Battery Monitoring System is found inside of Qualcomm PM8941
+PMICs. It provides open circuit voltage (OCV) and coulomb counter registers
+that allow the OS to infer a capacity level.
+
+Required properties:
+- compatible:                   Should contain "qcom,pm8941-bms".
+- reg:                          Specifies the SPMI address and length of the
+				controller's registers.
+- interrupts:                   OCV threshold interrupt.
+- io-channels:                  Should contain IIO channel specifier for the
+				ADC channel that reports battery temperature.
+- io-channel-names:             Should contain "temp".
+- qcom,fcc-temp-legend-celsius: An 8 bit array containing the temperature,
+				in degC, for each column of the full charge
+				capacity lookup table.
+- qcom,fcc-lut-microamp-hours:  An array of full charge capacity values in uAh,
+				one entry for each temperature defined in in
+				qcom,fcc-temp-legend-celsius.
+- qcom,ocv-temp-legend-celsius: An 8 bit array containing the temperature,
+				in degC, for each column of the OCV lookup
+				table.
+- qcom,ocv-capacity-legend:     An 8 bit array containing the capacity for each
+				row of the OCV lookup table.
+- qcom,ocv-lut-microvolt:       An array of OCV values in uV, one entry for each
+				capacity defined in qcom,ocv-capacity-legend.
+
+Example:
+		pm8941_vadc: adc@3100 {
+			compatible = "qcom,spmi-vadc";
+			reg = <0x3100>;
+			interrupts = <0x0 0x31 0x0 IRQ_TYPE_EDGE_RISING>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			#io-channel-cells = <1>;
+
+			bat_temp {
+				reg = <VADC_LR_MUX1_BAT_THERM>;
+			};
+		};
+
+		bms@4000 {
+			compatible = "qcom,pm8941-bms";
+			reg = <0x4000>;
+			interrupts = <0x0 0x40 0x4 IRQ_TYPE_EDGE_RISING>;
+			interrupt-names = "ocv_thr";
+
+			io-channels = <&pm8941_vadc VADC_LR_MUX1_BAT_THERM>;
+			io-channel-names = "temp";
+
+			qcom,fcc-temp-legend-celsius = /bits/ 8 <(-10) 0 25 50 65>;
+			qcom,fcc-lut-microamp-hours = <3230000 3260000 3380000 3410000 3360000>;
+
+			qcom,ocv-capacity-legend = /bits/ 8 <100 95 90 85 80 75
+							     70 65 60 55 50 45
+							     40 35 30 25 20 15
+							     10 9 8 7 6 5 4 3 2
+							     1 0>;
+			qcom,ocv-temp-legend-celsius = /bits/ 8 <(-10) 0 25 50 65>;
+			qcom,ocv-lut-microvolt = <43050000 43050000 43030000 42990000 42950000
+						  42770000 42570000 42550000 42510000 42310000
+						  42180000 41980000 41970000 41920000 41720000
+						  41590000 41390000 41450000 41400000 41200000
+						  41010000 40810000 40920000 40890000 40690000
+						  40480000 40280000 40440000 40420000 40220000
+						  40040000 39840000 40010000 39980000 39780000
+						  39620000 39420000 39550000 39560000 39360000
+						  39210000 39010000 39090000 39160000 38960000
+						  38830000 38630000 38740000 38790000 38590000
+						  38550000 38350000 38440000 38430000 38230000
+						  38310000 38110000 38230000 38180000 37980000
+						  38190000 37990000 38040000 38000000 37800000
+						  38060000 37860000 37900000 37840000 37640000
+						  37890000 37690000 37770000 37660000 37460000
+						  37720000 37520000 37560000 37450000 37250000
+						  37480000 37280000 37290000 37250000 37050000
+						  37240000 37040000 37020000 36990000 36790000
+						  37030000 36830000 36730000 36700000 36500000
+						  36940000 36740000 36670000 36640000 36440000
+						  36850000 36650000 36600000 36590000 36390000
+						  36750000 36550000 36520000 36550000 36350000
+						  36690000 36490000 36380000 36400000 36200000
+						  36460000 36260000 36180000 36120000 35920000
+						  36080000 35880000 35680000 35640000 35440000
+						  35510000 35310000 35050000 35020000 34820000
+						  34730000 34530000 34300000 34250000 34050000
+						  33870000 33670000 33040000 32820000 32620000
+						  30000000 30000000 30000000 30000000 30000000>;
+		};
+	};
+};