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

[PATCH v2 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@sonymobile.com>
---

Changes in v2:
 - remove 'qcom,' from example node name
 - Added reference to parent node pm8941@0 and binding doc for it

 .../bindings/power/qcom,coincell-charger.txt       | 50 ++++++++++++++++++++++
 1 file changed, 50 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..f0316c0
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/qcom,coincell-charger.txt
@@ -0,0 +1,50 @@
+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
+
+This charger is a sub-node of one of the 8941 PMIC blocks, and is specified
+as a child node in DTS of that node.  See ../mfd/qcom,spmi-pmic.txt and
+../mfd/qcom-pm8xxx.txt
+
+Examples:
+
+	pm8941@0 {
+		coincell@2800 {
+			compatible = "qcom,pm8941-coincell";
+			reg = <0x2800>;
+
+			qcom,rset-ohms = <2100>;
+			qcom,vset-millivolts = <3000>;
+			qcom,charge-enable;
+		};
+	};
-- 
1.8.2.2

[PATCH v2 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>
---

Changes in v2:
 - (none in this file, but dts changes in 1/3 and 3/3)

 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

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

[Stephen Boyd]

On 07/14/2015 04:26 PM, Tim Bird wrote:

>   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

It looks like it would compile fine without OF, so can we drop this 
dependency? Or make it into

  depends on MFD_SPMI_PMIC || COMPILE_TEST

?

> +	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};

Nitpick: put spaces around those braces.

> +/* 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)

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

Missing braces?

> +
> +	/* 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;

Missing braces? I guess this hardware just works out of the bootloader 
after the resistor is configured?

> +
> +	/* 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");
> +

Honestly these printks seems pretty useless and could probably be left out.

> +	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;
> +	}

Does this happen?

> +
> +	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);

This should be a bool:

     enable = of_property_read_bool(node, "qcom,charge-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",

Maybe a better driver name would be qcom-spmi-coincell?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

--
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 v2 2/3] ARM: qcom: Add coincell charger driver

[Tim Bird]

On 07/14/2015 06:11 PM, Stephen Boyd wrote:
> On 07/14/2015 04:26 PM, Tim Bird wrote:
> 
>>   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
> 
> It looks like it would compile fine without OF, so can we drop this 
> dependency? Or make it into
> 
>   depends on MFD_SPMI_PMIC || COMPILE_TEST
> 
> ?
I think I had CONFIG_OF off one time, and I spent the better
part of the afternoon trying to figure out why the driver wasn't
loading.  So it compiles but doesn't actually work.
But I think a dependency on MFD_SPMI_PMIC solves this issue.
So, OK on the second suggestion.

> 
>> +	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};
> 
> Nitpick: put spaces around those braces.

OK.  I presume you mean like this:
{ 2100, 1700, 1200, 800 };

>> +/* 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)
> 
> bool enable?
>
OK
 
>> +{
>> +	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;
> 
> Missing braces?
> 
Ouch!  Yes.  Thanks.

<rant - mostly at myself>
Of all the kernel CodingStyle rules, the one I absolutely hate is not using
braces on single statement blocks.  It trips me up like this a lot.
Maybe I should add a checkpatch rule to catch this kind of thing.
</rant>

>> +
>> +	/* 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;
> 
> Missing braces? I guess this hardware just works out of the bootloader 
> after the resistor is configured?
Yes, again.  Doh!

I swear I tested the register values after loading the driver
with different rset and vset values, to make sure they were taking effect.
But that was on a dragonboard with no actual coincell present.  I was
too chicken to mess with the values on an actual phone (since I can't
actually change the capacitor used for this, and didn't want to 
damage it with the wrong values.)

I must have done the value testing before messing up my braces.  Sorry!

Will fix.  Also, I will repeat testing of the values taking effect, on
the next iteration.

>> +
>> +	/* 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");
>> +
> 
> Honestly these printks seems pretty useless and could probably be left out.

I'm torn.  This driver and hardware is simple enough that the most
likely thing these printks will catch is a mis-configured "reg" value
in the DTS, and that would apply to most of these printks.
Inability to write to a valid register should be pretty rare.

I *would* like to visibly flag that case somehow, as I'm loathe to have some
DTS author have to traipse through kernel code to figure out where the
error is coming from.  I'm not sure, but I think things are pretty quiet
on probe errors, and if I don't print something, it makes a lot of 
work for a developer to find the problem, when all they've done is mistyped
something in the DTS.

>> +	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;
>> +	}
> 
> Does this happen?
Probably not any more.  The only way this device gets initialized now
is via OF operations.  This code was forward-ported from when this driver
also operated as a platform device.  In the current situation, I don't
know of a way for the kernel to get here if of_node is missing
(but I'm not an OF expert, and I didn't want to start using
a NULL of_node.)

What does of_property_read...() do with a NULL node?

I'm a little leery of taking this check out, but if you think it's
OK I'm fine doing it.

> 
>> +
>> +	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);
> 
> This should be a bool:
> 
>      enable = of_property_read_bool(node, "qcom,charge-enable");
OK.
 
>> +	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",
> 
> Maybe a better driver name would be qcom-spmi-coincell?

OK by me.

Thanks for the review!!
 -- Tim

 

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

[Stephen Boyd]

On 07/15/2015 12:08 PM, Tim Bird wrote:
>
> On 07/14/2015 06:11 PM, Stephen Boyd wrote:
>> On 07/14/2015 04:26 PM, Tim Bird wrote:
>>
>>>    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
>> It looks like it would compile fine without OF, so can we drop this
>> dependency? Or make it into
>>
>>    depends on MFD_SPMI_PMIC || COMPILE_TEST
>>
>> ?
> I think I had CONFIG_OF off one time, and I spent the better
> part of the afternoon trying to figure out why the driver wasn't
> loading.  So it compiles but doesn't actually work.
> But I think a dependency on MFD_SPMI_PMIC solves this issue.
> So, OK on the second suggestion.
>
>>> +	select REGMAP

This config wouldn't be necessary either then because it would be 
selected implicitly by the SPMI parent driver.

>>> 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};
>> Nitpick: put spaces around those braces.
> OK.  I presume you mean like this:
> { 2100, 1700, 1200, 800 };

Yep.

>
>
>>> +	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;
>>> +	}
>> Does this happen?
> Probably not any more.  The only way this device gets initialized now
> is via OF operations.  This code was forward-ported from when this driver
> also operated as a platform device.  In the current situation, I don't
> know of a way for the kernel to get here if of_node is missing
> (but I'm not an OF expert, and I didn't want to start using
> a NULL of_node.)
>
> What does of_property_read...() do with a NULL node?

I'm pretty sure it returns success or nothing when the node is NULL.

>
> I'm a little leery of taking this check out, but if you think it's
> OK I'm fine doing it.

I'll fix any problems with the removal of the check :)

>
>>> +
>>> +	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);
>> This should be a bool:
>>
>>       enable = of_property_read_bool(node, "qcom,charge-enable");
> OK.
>   
>>> +	if (rc)
>>> +		enable = 0;
>>> +
>>> +	rc = qcom_coincell_chgr_config(chgr, rset, vset, enable);
>>> +
>>> +	return rc;

This could be simplified to a return qcom_coincell_chrg_config() too.

Also, do we even need the chgr structure allocated anywhere besides on 
the stack? It seems that it will be memory that's just lying around for 
no use after probe.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Tim Bird]

On 07/15/2015 12:44 PM, Stephen Boyd wrote:
> On 07/15/2015 12:08 PM, Tim Bird wrote:
>>
>> On 07/14/2015 06:11 PM, Stephen Boyd wrote:
>>> On 07/14/2015 04:26 PM, Tim Bird wrote:
>>>
>>>>    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
>>> It looks like it would compile fine without OF, so can we drop this
>>> dependency? Or make it into
>>>
>>>    depends on MFD_SPMI_PMIC || COMPILE_TEST
>>>
>>> ?
>> I think I had CONFIG_OF off one time, and I spent the better
>> part of the afternoon trying to figure out why the driver wasn't
>> loading.  So it compiles but doesn't actually work.
>> But I think a dependency on MFD_SPMI_PMIC solves this issue.
>> So, OK on the second suggestion.
>>
>>>> +	select REGMAP
> 
> This config wouldn't be necessary either then because it would be 
> selected implicitly by the SPMI parent driver.

OK.  I seem to recall having problems with this with an earlier kernel
version, but it looks like the parent does indeed have a "select REMAP_SPMI"
now.  So I'll get rid of this here.

[...]
>>>> +	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;
>>>> +	}
>>> Does this happen?
>> Probably not any more.  The only way this device gets initialized now
>> is via OF operations.  This code was forward-ported from when this driver
>> also operated as a platform device.  In the current situation, I don't
>> know of a way for the kernel to get here if of_node is missing
>> (but I'm not an OF expert, and I didn't want to start using
>> a NULL of_node.)
>>
>> What does of_property_read...() do with a NULL node?
> 
> I'm pretty sure it returns success or nothing when the node is NULL.
> 
>>
>> I'm a little leery of taking this check out, but if you think it's
>> OK I'm fine doing it.
> 
> I'll fix any problems with the removal of the check :)

LOL. It's a deal! :-)

[...]
>>>> +	if (rc)
>>>> +		enable = 0;
>>>> +
>>>> +	rc = qcom_coincell_chgr_config(chgr, rset, vset, enable);
>>>> +
>>>> +	return rc;
> 
> This could be simplified to a return qcom_coincell_chrg_config() too.

OK

> Also, do we even need the chgr structure allocated anywhere besides on 
> the stack? It seems that it will be memory that's just lying around for 
> no use after probe.

Nope. And Agreed.  I'll change this.

Thanks for the help!
 -- 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

[PATCH v2 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>
---

Changes in v2:
 - change coincell node name to remove 'qcom,' prefix

 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..3705f34 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 {
+		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..968f104 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>;
 		};
+
+		coincell@2800 {
+			compatible = "qcom,pm8941-coincell";
+			reg = <0x2800>;
+			status = "disabled";
+		};
 	};
 
 	usid1: pm8941@1 {
-- 
1.8.2.2