Date:	Tue, 14 Jul 2015 20:07:04 -0500
From:	Rob Herring <robherring2@...il.com>
To:	Tim Bird <tim.bird@...ymobile.com>
Cc:	Arnd Bergmann <arnd@...db.de>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	linux-arm-msm <linux-arm-msm@...r.kernel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Bjorn Andersson <bjorn.andersson@...ymobile.com>,
	Mark Brown <broonie@...nel.org>
Subject: Re: [PATCH 1/3] ARM: dts: qcom: Add binding for the qcom coincell charger

On Tue, Jul 14, 2015 at 4:41 PM, Tim Bird <tim.bird@...ymobile.com> 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:
>> On Mon, Jul 13, 2015 at 6:39 PM, Tim Bird <tim.bird@...ymobile.com> wrote:
>>> This binding is used to configure the driver for the coincell charger
>>> found in Qualcomm PMICs.
>>>
>>> Signed-off-by: Tim Bird <tim.bird@...ymobile.com>
>>> ---
>>>  .../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.

>>
>>> +- 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.
>
>>
>>> +- 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

>
>>> +
>>> +Examples:
>>> +
>>> +       qcom,coincell@...0 {
>>
>> This should be a sub node of the PMIC, right. Please show in the
>> example and refer to the relevant PMIC binding doc.
> OK.
>
>>
>> Also, drop the "qcom," from the node name.
> OK.
> Will do these in the next patch rev.
>
>>> +               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 linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists