On Tue, Jul 14, 2015 at 4:41 PM, Tim Bird [off-list ref] wrote:

Rob,

Thanks for the quick feedback.  You responded off-list.  I don't know if
you meant to do this or not.  My response is off-list as well, but let me
know if I should have responded on-list, or set something differently in
my original e-mail.

Didn't mean to do that. Added back now.

Also adding Mark B in case he has any comments with respect to regulators.

On 07/13/2015 08:59 PM, Rob Herring wrote:

quoted

On Mon, Jul 13, 2015 at 6:39 PM, Tim Bird [off-list ref] wrote:

quoted

This binding is used to configure the driver for the coincell charger
found in Qualcomm PMICs.

Signed-off-by: Tim Bird <tim.bird-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
---
 .../bindings/power/qcom,coincell-charger.txt       | 44 ++++++++++++++++++++++
 1 file changed, 44 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/qcom,coincell-charger.txt

diff --git a/Documentation/devicetree/bindings/power/qcom,coincell-charger.txt b/Documentation/devicetree/bindings/power/qcom,coincell-charger.txt
new file mode 100644
index 0000000..bf39e2a
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/qcom,coincell-charger.txt

@@ -0,0 +1,44 @@
+Qualcomm Coincell Charger:
+
+The hardware block controls charging for a coincell or capacitor that is
+used to provide power backup for certain features of the power management
+IC (PMIC)
+

Would the regulator binding work for this? I ask mainly because that
is what FSL mc13892 coincell charger already uses.

I could use the regulator binding, but I think it would imply
more functionality than the driver supports.  The regulator
binding docs list lots of (admittedly optional) attributes that
don't really apply.

I looked at the mc13892 binding, and I didn't like it too much,
because it doesn't really indicate what the allowed values are
for the regulator -- but it's possible on that hardware that it's
acting like a normal regulator with a fine-grained range of
voltage outputs and other configurable attributes.

This is really just 2 values and an enable bit and I think
the mapping from the regulator attributes to this is not a
good fit.

Okay.

quoted

quoted

+- compatible:
+       Usage: required
+       Value type: <string>
+       Definition: must be: "qcom,pm8941-coincell"
+
+- reg:
+       Usage: required
+       Value type: <u32>
+       Definition: base address of the coincell charger registers
+
+- qcom,rset-ohms:
+       Usage: required
+       Value type: <u32>
+       Definition: resistance (in ohms) for current-limiting resistor
+               must be one of: 800, 1200, 1700, 2100

Can you define the current limit and then calculate the resistor value
to use in the driver? Current is ultimately what the battery is
spec'ed to.

It's possible, but not very useful.  The resistor is one of 4 values,
and specifies exactly what the hardware is doing.  Specifying the current
limit is imprecise, and would have to be mapped onto the correct
resistor value (depending on the voltage) by the driver.

Besides being imprecise, however, it's not how these things are usually
specified.  The SoC specification, system configuration and schematics
list the resistor value, and the data sheet for the coincell itself
usually specifies a recommended voltage and resistor value.

Okay, if that is how things are spec'ed with batteries then I'm okay with it.

So if we specify the max current then developers setting this
would need to translate the numbers to what the dt expects, so
that the driver can reverse that math and fit it to one of the
supported values.

quoted

quoted

+- qcom,vset-millivolts:
+       Usage: required
+       Value type: <u32>
+       Definition: voltage (in millivolts) to apply for charging
+               must be one of: 2500, 3000, 3100, 3200
+
+- qcom,charge-enable:
+       Usage: optional
+       Value type: <u32> or <none>
+       Definition: definining this property, with an optional non-zero
+               value, enables charging

I'm not sure that this belongs in DT. Don't you want to enable
charging when plugged in perhaps or at some voltage threshold?

In practice this is never changed at runtime. It's only set at kernel boot.
The main use of this is to override (either on or off) whatever the firmware
did.

If your firmware and dtb are separate from your kernel, then ... (well
you know where I'm headed :) ).

If we do keep this, I think is should be a disable property with not
present being the default and enabling charging. Also, it only needs
to be bool (i.e. no value).

Rob

quoted

quoted

+
+Examples:
+
+       qcom,coincell@2800 {

This should be a sub node of the PMIC, right. Please show in the
example and refer to the relevant PMIC binding doc.

OK.

quoted

Also, drop the "qcom," from the node name.

OK.
Will do these in the next patch rev.

quoted

quoted

+               compatible = "qcom,pm8941-coincell";
+               reg = <0x2800>;
+
+               qcom,rset-ohms = <2100>;
+               qcom,vset-millivolts = <3000>;
+               qcom,charge-enable;
+       };
--
1.8.2.2

Thanks!
 -- Tim

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