[PATCH 1/3] ARM: dts: qcom: Add binding for the qcom coincell charger

[PATCH 1/3] ARM: dts: qcom: Add binding for the qcom coincell charger

[Tim Bird]

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)
+
+- 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
+
+- 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
+
+Examples:
+
+	qcom,coincell@2800 {
+		compatible = "qcom,pm8941-coincell";
+		reg = <0x2800>;
+
+		qcom,rset-ohms = <2100>;
+		qcom,vset-millivolts = <3000>;
+		qcom,charge-enable;
+	};
--
1.8.2.2

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

[PATCH 2/3] ARM: qcom: Add coincell charger driver

[Tim Bird]

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

The driver allows configuring the current-limiting resistor for
the charger, as well as the voltage to apply to the coincell
(or capacitor) when charging.

Signed-off-by: Tim Bird <tim.bird@sonymobile.com>
---
 drivers/misc/Kconfig         |  11 ++++
 drivers/misc/Makefile        |   1 +
 drivers/misc/qcom-coincell.c | 154 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 166 insertions(+)
 create mode 100644 drivers/misc/qcom-coincell.c

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 42c3852..0909869 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -271,6 +271,17 @@ config HP_ILO
 	  To compile this driver as a module, choose M here: the
 	  module will be called hpilo.
 
+config QCOM_COINCELL
+	tristate "Qualcomm coincell charger support"
+	depends on OF
+	select REGMAP
+	help
+	  This driver supports the coincell block found inside of
+	  Qualcomm PMICs.  The coincell charger provides a means to
+	  charge a coincell battery or backup capacitor which is used
+	  to maintain PMIC register and RTC state in the absence of
+	  external power.
+
 config SGI_GRU
 	tristate "SGI GRU driver"
 	depends on X86_UV && SMP
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index d056fb7..537d7f3 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_LKDTM)		+= lkdtm.o
 obj-$(CONFIG_TIFM_CORE)       	+= tifm_core.o
 obj-$(CONFIG_TIFM_7XX1)       	+= tifm_7xx1.o
 obj-$(CONFIG_PHANTOM)		+= phantom.o
+obj-$(CONFIG_QCOM_COINCELL)	+= qcom-coincell.o
 obj-$(CONFIG_SENSORS_BH1780)	+= bh1780gli.o
 obj-$(CONFIG_SENSORS_BH1770)	+= bh1770glc.o
 obj-$(CONFIG_SENSORS_APDS990X)	+= apds990x.o
diff --git a/drivers/misc/qcom-coincell.c b/drivers/misc/qcom-coincell.c
new file mode 100644
index 0000000..9c019e4
--- /dev/null
+++ b/drivers/misc/qcom-coincell.c
@@ -0,0 +1,154 @@
+/* Copyright (c) 2013, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2015, Sony Mobile Communications Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+
+struct qcom_coincell {
+	struct device	*dev;
+	struct regmap	*regmap;
+	u32		base_addr;
+};
+
+#define QCOM_COINCELL_REG_RSET		0x44
+#define QCOM_COINCELL_REG_VSET		0x45
+#define QCOM_COINCELL_REG_ENABLE	0x46
+
+#define QCOM_COINCELL_ENABLE		BIT(7)
+
+static const int qcom_rset_map[] = {2100, 1700, 1200, 800};
+static const int qcom_vset_map[] = {2500, 3200, 3100, 3000};
+/* NOTE: for pm8921 and others, voltage of 2500 is 16 (10000b), not 0 */
+
+static int qcom_coincell_chgr_config(struct qcom_coincell *chgr, int rset,
+				     int vset, int enable)
+{
+	int i, rc;
+	unsigned int value;
+
+	/* select current-limiting resistor */
+	for (i = 0; i < ARRAY_SIZE(qcom_rset_map); i++)
+		if (rset == qcom_rset_map[i])
+			break;
+
+	if (i >= ARRAY_SIZE(qcom_rset_map)) {
+		dev_err(chgr->dev, "invalid rset-ohms value %d\n", rset);
+		return -EINVAL;
+	}
+
+	rc = regmap_write(chgr->regmap,
+			  chgr->base_addr + QCOM_COINCELL_REG_RSET, i);
+	if (rc)
+		dev_err(chgr->dev, "could not write to RSET register\n");
+		return rc;
+
+	/* select charge voltage */
+	for (i = 0; i < ARRAY_SIZE(qcom_vset_map); i++)
+		if (vset == qcom_vset_map[i])
+			break;
+
+	if (i >= ARRAY_SIZE(qcom_vset_map)) {
+		dev_err(chgr->dev, "invalid vset-millivolts value %d\n", vset);
+		return -EINVAL;
+	}
+
+	rc = regmap_write(chgr->regmap,
+		chgr->base_addr + QCOM_COINCELL_REG_VSET, i);
+	if (rc)
+		dev_err(chgr->dev, "could not write to VSET register\n");
+		return rc;
+
+	/* set 'enable' register */
+	value = enable ? QCOM_COINCELL_ENABLE : 0;
+	rc = regmap_write(chgr->regmap,
+			  chgr->base_addr + QCOM_COINCELL_REG_ENABLE, value);
+	if (rc)
+		dev_err(chgr->dev, "could not write to ENABLE register\n");
+
+	return rc;
+}
+
+static int qcom_coincell_probe(struct platform_device *pdev)
+{
+	struct device_node *node = pdev->dev.of_node;
+	struct qcom_coincell *chgr;
+	u32 rset, vset, enable;
+	int rc;
+
+	if (!node) {
+		dev_err(&pdev->dev, "%s: device node missing\n", __func__);
+		return -ENODEV;
+	}
+
+	chgr = devm_kzalloc(&pdev->dev, sizeof(*chgr), GFP_KERNEL);
+	if (!chgr)
+		return -ENOMEM;
+
+	chgr->dev = &pdev->dev;
+
+	chgr->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	if (!chgr->regmap) {
+		dev_err(chgr->dev, "Unable to get regmap\n");
+		return -EINVAL;
+	}
+
+	rc = of_property_read_u32(node, "reg", &chgr->base_addr);
+	if (rc)
+		return rc;
+
+	rc = of_property_read_u32(node, "qcom,rset-ohms", &rset);
+	if (rc) {
+		dev_err(chgr->dev, "can't find 'qcom,rset-ohms' in DT block");
+		return rc;
+	}
+
+	rc = of_property_read_u32(node, "qcom,vset-millivolts", &vset);
+	if (rc) {
+		dev_err(chgr->dev,
+			"can't find 'qcom,vset-millivolts' in DT block");
+		return rc;
+	}
+
+	rc = of_property_read_u32(node, "qcom,charge-enable", &enable);
+	if (rc)
+		enable = 0;
+
+	rc = qcom_coincell_chgr_config(chgr, rset, vset, enable);
+
+	return rc;
+}
+
+static const struct of_device_id qcom_coincell_match_table[] = {
+	{ .compatible = "qcom,pm8941-coincell", },
+	{}
+};
+
+MODULE_DEVICE_TABLE(of, qcom_coincell_match_table);
+
+static struct platform_driver qcom_coincell_driver = {
+	.driver	= {
+		.name		= "qcom,pm8941-coincell",
+		.of_match_table	= qcom_coincell_match_table,
+	},
+	.probe		= qcom_coincell_probe,
+};
+
+module_platform_driver(qcom_coincell_driver);
+
+MODULE_DESCRIPTION("Qualcomm PMIC coincell charger driver");
+MODULE_LICENSE("GPL v2");
-- 
1.8.2.2

[PATCH 3/3] ARM: dts: qcom: Add dts changes for qcom coincell charger

[Tim Bird]

Add framework for the coincell charger DT block in pm8941 file, and
actual values for honami battery in the honami dts file.

Signed-off-by: Tim Bird <tim.bird@sonymobile.com>
---
 arch/arm/boot/dts/qcom-msm8974-sony-xperia-honami.dts | 11 +++++++++++
 arch/arm/boot/dts/qcom-pm8941.dtsi                    |  6 ++++++
 2 files changed, 17 insertions(+)

diff --git a/arch/arm/boot/dts/qcom-msm8974-sony-xperia-honami.dts b/arch/arm/boot/dts/qcom-msm8974-sony-xperia-honami.dts
index bd35b06..a7440fd 100644
--- a/arch/arm/boot/dts/qcom-msm8974-sony-xperia-honami.dts
+++ b/arch/arm/boot/dts/qcom-msm8974-sony-xperia-honami.dts
@@ -17,3 +17,14 @@
 		status = "ok";
 	};
 };
+
+&spmi_bus {
+	pm8941@0 {
+		qcom,coincell@2800 {
+			status = "ok";
+			qcom,rset-ohms = <2100>;
+			qcom,vset-millivolts = <3000>;
+			qcom,charge-enable;
+		};
+	};
+};
diff --git a/arch/arm/boot/dts/qcom-pm8941.dtsi b/arch/arm/boot/dts/qcom-pm8941.dtsi
index aa774e6..a7d9c4b 100644
--- a/arch/arm/boot/dts/qcom-pm8941.dtsi
+++ b/arch/arm/boot/dts/qcom-pm8941.dtsi
@@ -125,6 +125,12 @@
 			interrupts = <0x0 0x36 0x0 IRQ_TYPE_EDGE_RISING>;
 			qcom,external-resistor-micro-ohms = <10000>;
 		};
+
+		qcom,coincell@2800 {
+			compatible = "qcom,pm8941-coincell";
+			reg = <0x2800>;
+			status = "disabled";
+		};
 	};
 
 	usid1: pm8941@1 {
-- 
1.8.2.2

Re: [PATCH 1/3] ARM: dts: qcom: Add binding for the qcom coincell charger

[Rob Herring]

On Tue, Jul 14, 2015 at 4:41 PM, Tim Bird <tim.bird-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org> 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-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org> wrote:
>>> 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.

>>
>>> +- 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@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.
>
>>
>> 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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] ARM: dts: qcom: Add binding for the qcom coincell charger

[Tim Bird]

On 07/14/2015 06:07 PM, Rob Herring wrote:
> On Tue, Jul 14, 2015 at 4:41 PM, Tim Bird <tim.bird@sonymobile.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@sonymobile.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@sonymobile.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 :) ).

Sorry, I have no idea how the sentence would end, so I think I'm missing
where you are headed.

> If we do keep this, I think it 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).

Are you suggesting something like this, then?

  - qcom,charger-disable:
       Usage: optional
       Value type: <none>
       Definition: defining this property disables charging

The logic would be as follows:
 - if the developer wants to just use the firmware settings, then
  the kernel would just not define this dts node at all, and nothing
  would change on kernel boot
 - if the developers want to change the settings, either turning off
  the charger, or specifying desired settings, then they define
  the appropriate attributes.

I'm OK with that.

It would make no sense to define rset and vset values when this
is defined.  Should I note that somewhere in the binding doc?

Thanks.
 -- Tim

Re: [PATCH 1/3] ARM: dts: qcom: Add binding for the qcom coincell charger

[Rob Herring]

On Wed, Jul 15, 2015 at 1:24 PM, Tim Bird <tim.bird-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org> wrote:
> On 07/14/2015 06:07 PM, Rob Herring wrote:
>> On Tue, Jul 14, 2015 at 4:41 PM, Tim Bird <tim.bird-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org> wrote:
>>> On 07/13/2015 08:59 PM, Rob Herring wrote:
>>>> On Mon, Jul 13, 2015 at 6:39 PM, Tim Bird <tim.bird-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org> wrote:
>>>>> This binding is used to configure the driver for the coincell charger
>>>>> found in Qualcomm PMICs.

[...]

>>>>> +- 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 :) ).
>
> Sorry, I have no idea how the sentence would end, so I think I'm missing
> where you are headed.

dtbs should be separate from the kernel and part of the firmware. I'm
certain you recall those discussions or have sucessfully blocked them
from memory. If the dtb is part of the firmware, then changing the dtb
to change the kernel's handling of this would not make a lot of sense.

I was going to say if you want to change what firmware did, then you
could just do it from userspace. A delay from kernel boot to userspace
init would not matter here. However, if you have no other reason for
having a userspace interface, that probably isn't worth it and it is
fine as is.

>
>> If we do keep this, I think it 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).
>
> Are you suggesting something like this, then?
>
>   - qcom,charger-disable:
>        Usage: optional
>        Value type: <none>

s/<none>/boolean/

But otherwise, yes this looks fine.

>        Definition: defining this property disables charging
>
> The logic would be as follows:
>  - if the developer wants to just use the firmware settings, then
>   the kernel would just not define this dts node at all, and nothing
>   would change on kernel boot

Well, the kernel doesn't decide dts settings, but yes I agree that
removing or disabling the node would disable any kernel control.

>  - if the developers want to change the settings, either turning off
>   the charger, or specifying desired settings, then they define
>   the appropriate attributes.
>
> I'm OK with that.

I am too.

> It would make no sense to define rset and vset values when this
> is defined.  Should I note that somewhere in the binding doc?

They are somewhat don't care unless changing them has some side
effect. I'll leave it up to you.

Rob
--
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

Re: [PATCH 1/3] ARM: dts: qcom: Add binding for the qcom coincell charger

[Tim Bird]

On 07/15/2015 02:22 PM, Rob Herring wrote:
> On Wed, Jul 15, 2015 at 1:24 PM, Tim Bird <tim.bird-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org> wrote:
>> On 07/14/2015 06:07 PM, Rob Herring wrote:
>>> On Tue, Jul 14, 2015 at 4:41 PM, Tim Bird <tim.bird-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org> wrote:
>>>> On 07/13/2015 08:59 PM, Rob Herring wrote:
>>>>> On Mon, Jul 13, 2015 at 6:39 PM, Tim Bird <tim.bird-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org> wrote:
>>>>>> This binding is used to configure the driver for the coincell charger
>>>>>> found in Qualcomm PMICs.
> 
> [...]
> 
>>>>>> +- 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 :) ).
>>
>> Sorry, I have no idea how the sentence would end, so I think I'm missing
>> where you are headed.
> 
> dtbs should be separate from the kernel and part of the firmware. I'm
> certain you recall those discussions or have sucessfully blocked them
> from memory.

Ah yes, those discussions. :-)

Having dtbs come from firmware is not on the horizon yet
for projects I'm working on, so I haven't really considered
the ramifications.

> If the dtb is part of the firmware, then changing the dtb
> to change the kernel's handling of this would not make a lot of sense.

Indeed.

> I was going to say if you want to change what firmware did, then you
> could just do it from userspace. A delay from kernel boot to userspace
> init would not matter here. However, if you have no other reason for
> having a userspace interface, that probably isn't worth it and it is
> fine as is.
> 
>>
>>> If we do keep this, I think it 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).
>>
>> Are you suggesting something like this, then?
>>
>>   - qcom,charger-disable:
>>        Usage: optional
>>        Value type: <none>
> 
> s/<none>/boolean/
> 
> But otherwise, yes this looks fine.
> 
>>        Definition: defining this property disables charging
>>
>> The logic would be as follows:
>>  - if the developer wants to just use the firmware settings, then
>>   the kernel would just not define this dts node at all, and nothing
>>   would change on kernel boot
> 
> Well, the kernel doesn't decide dts settings, but yes I agree that
> removing or disabling the node would disable any kernel control.
> 
>>  - if the developers want to change the settings, either turning off
>>   the charger, or specifying desired settings, then they define
>>   the appropriate attributes.
>>
>> I'm OK with that.
> 
> I am too.
> 
>> It would make no sense to define rset and vset values when this
>> is defined.  Should I note that somewhere in the binding doc?
> 
> They are somewhat don't care unless changing them has some side
> effect. I'll leave it up to you.

OK - these are indeed "don't care" in that case.
I probably don't have to explain in the binding doc that
adjusting settings for disabled hardware doesn't make sense.

Thanks again for the quick feedback.
 -- 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

Re: [PATCH 1/3] ARM: dts: qcom: Add binding for the qcom coincell charger

[Bjorn Andersson]

On Wed 15 Jul 15:27 PDT 2015, Tim Bird wrote:

> 
> 
> On 07/15/2015 02:22 PM, Rob Herring wrote:
> > On Wed, Jul 15, 2015 at 1:24 PM, Tim Bird <tim.bird@sonymobile.com> wrote:
> >> On 07/14/2015 06:07 PM, Rob Herring wrote:
> >>> On Tue, Jul 14, 2015 at 4:41 PM, Tim Bird <tim.bird@sonymobile.com> wrote:
> >>>> On 07/13/2015 08:59 PM, Rob Herring wrote:
> >>>>> On Mon, Jul 13, 2015 at 6:39 PM, Tim Bird <tim.bird@sonymobile.com> wrote:
> >>>>>> This binding is used to configure the driver for the coincell charger
> >>>>>> found in Qualcomm PMICs.
> > 
> > [...]
> > 
> >>>>>> +- 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 :) ).
> >>
> >> Sorry, I have no idea how the sentence would end, so I think I'm missing
> >> where you are headed.
> > 
> > dtbs should be separate from the kernel and part of the firmware. I'm
> > certain you recall those discussions or have sucessfully blocked them
> > from memory.
> 
> Ah yes, those discussions. :-)
> 
> Having dtbs come from firmware is not on the horizon yet
> for projects I'm working on, so I haven't really considered
> the ramifications.
> 

This has nothing to do about how the dtb, kernel and boot is stored on
the device; we already store them as 3 separate entities and they can be
upgraded independently. Neither one of them is read only and they will
never be!

We've already passed the point where we've gotten the pieces into
mainline that would make it possible to run all devices on e.g. the 8974
platform from a single zImage. The fact that we store the dtb in
adjacent blocks is simply a convenience thing.

Regards,
Bjorn