[RFC PATCH v2 0/5] Support for Qualcomm QPNP PMIC's

[RFC PATCH v2 0/5] Support for Qualcomm QPNP PMIC's

[Stanimir Varbanov]

Hello to all,

Here is the second version of the RFC for Qualcomm QPNP PMIC's support.
The difference since first version [1] is that now we use platform
devices for PMIC peripherals instead of new qpnp-bus. The first patch
in the set implements an *of* based parsing of PMIC peripheral
resources and adding mfd cell for it. The devicetree node should
contain a compatible and reg proparties. The intention was to keep
things simple and similar to the other mfd drivers.

The second patch describes the devicetree binding of the slave devices
attached to the SPMI interface. The other few patches are example of
a qpnp-rtc driver and binding documentation.

Comments are welcome.

regards,
Stan

[1] http://www.spinics.net/lists/linux-arm-msm/msg09745.html

Ivan T. Ivanov (1):
  dt: qcom: msm8974: add qpnp-spmi device nodes

Josh Cartwright (1):
  mfd: qpnp: add support for Qualcomm QPNP PMICs

Stanimir Varbanov (3):
  rtc: add qpnp rtc driver
  dt: msm8974: add qpnp rtc device node
  dt: rtc: add binding document for qpnp rtc

 .../devicetree/bindings/rtc/qcom,rtc-qpnp.txt      |   24 +
 arch/arm/boot/dts/qcom-msm8974.dtsi                |   53 +++
 drivers/mfd/Kconfig                                |   15 +
 drivers/mfd/Makefile                               |    1 +
 drivers/mfd/qpnp-spmi.c                            |  129 +++++
 drivers/rtc/Kconfig                                |    8 +
 drivers/rtc/Makefile                               |    1 +
 drivers/rtc/rtc-qpnp.c                             |  489 ++++++++++++++++++++
 8 files changed, 720 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/rtc/qcom,rtc-qpnp.txt
 create mode 100644 drivers/mfd/qpnp-spmi.c
 create mode 100644 drivers/rtc/rtc-qpnp.c

[RFC PATCH v2 1/5] mfd: qpnp: add support for Qualcomm QPNP PMICs

[Stanimir Varbanov]

From: Josh Cartwright <joshc@codeaurora.org>

The Qualcomm QPNP PMIC chips are components used with the
Snapdragon 800 series SoC family.  This driver exists
largely as a glue mfd component, it exists to be an owner
of an SPMI regmap for children devices described in
device tree.

Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
---
 drivers/mfd/Kconfig     |   15 ++++++
 drivers/mfd/Makefile    |    1 +
 drivers/mfd/qpnp-spmi.c |  129 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 145 insertions(+), 0 deletions(-)
 create mode 100644 drivers/mfd/qpnp-spmi.c

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index ee8204c..258b733 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -524,6 +524,21 @@ config MFD_PM8921_CORE
 	  Say M here if you want to include support for PM8921 chip as a module.
 	  This will build a module called "pm8921-core".
 
+config MFD_QPNP_SPMI
+	tristate "Qualcomm QPNP SPMI PMIC"
+	depends on ARCH_QCOM || COMPILE_TEST
+	depends on OF
+	select MFD_CORE
+	select REGMAP_SPMI
+	help
+	  This enables support for the Qualcomm QPNP SPMI PMICs.
+	  These PMICs are currently used with the Snapdragon 800 series of
+	  SoCs.  Note, that this will only be useful paired with descriptions
+	  of the independent functions as children nodes in the device tree.
+
+	  Say M here if you want to include support for the QPNP SPMI PMIC
+	  series as a module.  The module will be called "qpnp-spmi".
+
 config MFD_RDC321X
 	tristate "RDC R-321x southbridge"
 	select MFD_CORE
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 8afedba..31833d7 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -153,6 +153,7 @@ obj-$(CONFIG_MFD_SI476X_CORE)	+= si476x-core.o
 obj-$(CONFIG_MFD_CS5535)	+= cs5535-mfd.o
 obj-$(CONFIG_MFD_OMAP_USB_HOST)	+= omap-usb-host.o omap-usb-tll.o
 obj-$(CONFIG_MFD_PM8921_CORE) 	+= pm8921-core.o ssbi.o
+obj-$(CONFIG_MFD_QPNP_SPMI)	+= qpnp-spmi.o
 obj-$(CONFIG_TPS65911_COMPARATOR)	+= tps65911-comparator.o
 obj-$(CONFIG_MFD_TPS65090)	+= tps65090.o
 obj-$(CONFIG_MFD_AAT2870_CORE)	+= aat2870-core.o
diff --git a/drivers/mfd/qpnp-spmi.c b/drivers/mfd/qpnp-spmi.c
new file mode 100644
index 0000000..efd7d3e
--- /dev/null
+++ b/drivers/mfd/qpnp-spmi.c
@@ -0,0 +1,129 @@
+/* Copyright (c) 2014, The Linux Foundation. All rights reserved.
+ *
+ * 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/spmi.h>
+#include <linux/regmap.h>
+#include <linux/of_address.h>
+#include <linux/slab.h>
+#include <linux/mfd/core.h>
+
+#define QPNP_RESOURCE_SIZE	256
+
+static const struct regmap_config qpnp_regmap_config = {
+	.reg_bits	= 16,
+	.val_bits	= 8,
+	.max_register	= 0xffff,
+};
+
+static int qpnp_index_to_resource(struct device_node *np, int index,
+				  struct resource *res)
+{
+	const char *name = NULL;
+	const __be32 *addrp;
+	u64 addr;
+
+	addrp = of_get_address(np, index, NULL, NULL);
+	if (!addrp)
+		return -EINVAL;
+
+	addr = of_read_number(addrp, 1);
+	if (addr == OF_BAD_ADDR)
+		return -EINVAL;
+
+	of_property_read_string_index(np, "reg-names", index, &name);
+
+	res->start = addr;
+	res->end = addr + QPNP_RESOURCE_SIZE - 1;
+	res->flags = IORESOURCE_REG;
+	res->name = name ? name : np->name;
+
+	return 0;
+}
+
+static int qpnp_add_device(struct spmi_device *root, struct device_node *child)
+{
+	struct mfd_cell cell = {};
+	struct resource *res, *r;
+	int num_resources = 0;
+	const char *compat;
+	int ret, i;
+
+	compat = of_get_property(child, "compatible", NULL);
+	if (!compat)
+		return -ENODEV;
+
+	while (of_get_address(child, num_resources, NULL, NULL))
+		num_resources++;
+
+	if (!num_resources)
+		return -ENODEV;
+
+	res = kcalloc(num_resources, sizeof(*res), GFP_KERNEL);
+	if (!res)
+		return -ENOMEM;
+
+	r = res;
+	for (i = 0; i < num_resources; i++, r++)
+		qpnp_index_to_resource(child, i, r);
+
+	cell.name = kasprintf(GFP_KERNEL, "%x.%04x.%s", root->usid,
+			      (u16)res[0].start, child->name);
+	cell.of_compatible = compat;
+	cell.num_resources = num_resources;
+	cell.resources = res;
+
+	ret = mfd_add_devices(&root->dev, PLATFORM_DEVID_NONE, &cell, 1,
+			      NULL, 0, NULL);
+
+	kfree(res);
+	kfree(cell.name);
+
+	return ret;
+}
+
+static int qpnp_probe(struct spmi_device *sdev)
+{
+	struct device_node *root = sdev->dev.of_node;
+	struct device_node *child;
+	struct regmap *regmap;
+
+	regmap = devm_regmap_init_spmi_ext(sdev, &qpnp_regmap_config);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	for_each_available_child_of_node(root, child)
+		qpnp_add_device(sdev, child);
+
+	return 0;
+}
+
+static void qpnp_remove(struct spmi_device *sdev)
+{
+	mfd_remove_devices(&sdev->dev);
+}
+
+static const struct of_device_id qpnp_id_table[] = {
+	{ .compatible = "qcom,qpnp-spmi", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, qpnp_id_table);
+
+static struct spmi_driver qpnp_driver = {
+	.probe = qpnp_probe,
+	.remove = qpnp_remove,
+	.driver = {
+		.name = "qpnp-spmi",
+		.of_match_table = qpnp_id_table,
+	},
+};
+module_spmi_driver(qpnp_driver);
-- 
1.7.0.4

Re: [RFC PATCH v2 1/5] mfd: qpnp: add support for Qualcomm QPNP PMICs

[Lee Jones]

On Thu, 03 Jul 2014, Stanimir Varbanov wrote:
> From: Josh Cartwright <joshc-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> 
> The Qualcomm QPNP PMIC chips are components used with the
> Snapdragon 800 series SoC family.  This driver exists
> largely as a glue mfd component, it exists to be an owner
> of an SPMI regmap for children devices described in
> device tree.
> 
> Signed-off-by: Josh Cartwright <joshc-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> Signed-off-by: Stanimir Varbanov <svarbanov-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/mfd/Kconfig     |   15 ++++++
>  drivers/mfd/Makefile    |    1 +
>  drivers/mfd/qpnp-spmi.c |  129 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 145 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/mfd/qpnp-spmi.c
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index ee8204c..258b733 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -524,6 +524,21 @@ config MFD_PM8921_CORE
>  	  Say M here if you want to include support for PM8921 chip as a module.
>  	  This will build a module called "pm8921-core".
>  
> +config MFD_QPNP_SPMI
> +	tristate "Qualcomm QPNP SPMI PMIC"
> +	depends on ARCH_QCOM || COMPILE_TEST
> +	depends on OF
> +	select MFD_CORE
> +	select REGMAP_SPMI
> +	help
> +	  This enables support for the Qualcomm QPNP SPMI PMICs.
> +	  These PMICs are currently used with the Snapdragon 800 series of
> +	  SoCs.  Note, that this will only be useful paired with descriptions
> +	  of the independent functions as children nodes in the device tree.

It would be helpful if the expected types of child devices were
mentioned here in the help section.

> +	  Say M here if you want to include support for the QPNP SPMI PMIC
> +	  series as a module.  The module will be called "qpnp-spmi".
> +
>  config MFD_RDC321X
>  	tristate "RDC R-321x southbridge"
>  	select MFD_CORE
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 8afedba..31833d7 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -153,6 +153,7 @@ obj-$(CONFIG_MFD_SI476X_CORE)	+= si476x-core.o
>  obj-$(CONFIG_MFD_CS5535)	+= cs5535-mfd.o
>  obj-$(CONFIG_MFD_OMAP_USB_HOST)	+= omap-usb-host.o omap-usb-tll.o
>  obj-$(CONFIG_MFD_PM8921_CORE) 	+= pm8921-core.o ssbi.o
> +obj-$(CONFIG_MFD_QPNP_SPMI)	+= qpnp-spmi.o
>  obj-$(CONFIG_TPS65911_COMPARATOR)	+= tps65911-comparator.o
>  obj-$(CONFIG_MFD_TPS65090)	+= tps65090.o
>  obj-$(CONFIG_MFD_AAT2870_CORE)	+= aat2870-core.o
> diff --git a/drivers/mfd/qpnp-spmi.c b/drivers/mfd/qpnp-spmi.c
> new file mode 100644
> index 0000000..efd7d3e
> --- /dev/null
> +++ b/drivers/mfd/qpnp-spmi.c
> @@ -0,0 +1,129 @@
> +/* Copyright (c) 2014, The Linux Foundation. All rights reserved.
> + *

Non-standard multi-line comment.  First line should be blank.

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

No author?

> + * 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/spmi.h>
> +#include <linux/regmap.h>
> +#include <linux/of_address.h>
> +#include <linux/slab.h>
> +#include <linux/mfd/core.h>
> +
> +#define QPNP_RESOURCE_SIZE	256
> +
> +static const struct regmap_config qpnp_regmap_config = {
> +	.reg_bits	= 16,
> +	.val_bits	= 8,
> +	.max_register	= 0xffff,
> +};
> +
> +static int qpnp_index_to_resource(struct device_node *np, int index,
> +				  struct resource *res)
> +{
> +	const char *name = NULL;
> +	const __be32 *addrp;
> +	u64 addr;
> +
> +	addrp = of_get_address(np, index, NULL, NULL);
> +	if (!addrp)
> +		return -EINVAL;
> +
> +	addr = of_read_number(addrp, 1);
> +	if (addr == OF_BAD_ADDR)
> +		return -EINVAL;
> +
> +	of_property_read_string_index(np, "reg-names", index, &name);
> +
> +	res->start = addr;
> +	res->end = addr + QPNP_RESOURCE_SIZE - 1;
> +	res->flags = IORESOURCE_REG;
> +	res->name = name ? name : np->name;
> +
> +	return 0;
> +}
> +
> +static int qpnp_add_device(struct spmi_device *root, struct device_node *child)
> +{
> +	struct mfd_cell cell = {};
> +	struct resource *res, *r;
> +	int num_resources = 0;
> +	const char *compat;
> +	int ret, i;
> +
> +	compat = of_get_property(child, "compatible", NULL);
> +	if (!compat)
> +		return -ENODEV;
> +
> +	while (of_get_address(child, num_resources, NULL, NULL))
> +		num_resources++;
> +
> +	if (!num_resources)
> +		return -ENODEV;
> +
> +	res = kcalloc(num_resources, sizeof(*res), GFP_KERNEL);
> +	if (!res)
> +		return -ENOMEM;
> +
> +	r = res;
> +	for (i = 0; i < num_resources; i++, r++)
> +		qpnp_index_to_resource(child, i, r);
> +
> +	cell.name = kasprintf(GFP_KERNEL, "%x.%04x.%s", root->usid,
> +			      (u16)res[0].start, child->name);
> +	cell.of_compatible = compat;
> +	cell.num_resources = num_resources;
> +	cell.resources = res;
> +
> +	ret = mfd_add_devices(&root->dev, PLATFORM_DEVID_NONE, &cell, 1,
> +			      NULL, 0, NULL);
> +
> +	kfree(res);
> +	kfree(cell.name);
> +
> +	return ret;
> +}
> +
> +static int qpnp_probe(struct spmi_device *sdev)
> +{
> +	struct device_node *root = sdev->dev.of_node;
> +	struct device_node *child;
> +	struct regmap *regmap;
> +
> +	regmap = devm_regmap_init_spmi_ext(sdev, &qpnp_regmap_config);
> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +
> +	for_each_available_child_of_node(root, child)
> +		qpnp_add_device(sdev, child);

This entire driver looks like a re-write of of_platform_populate().

Why?

> +	return 0;
> +}
> +
> +static void qpnp_remove(struct spmi_device *sdev)
> +{
> +	mfd_remove_devices(&sdev->dev);
> +}
> +
> +static const struct of_device_id qpnp_id_table[] = {
> +	{ .compatible = "qcom,qpnp-spmi", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, qpnp_id_table);
> +
> +static struct spmi_driver qpnp_driver = {
> +	.probe = qpnp_probe,
> +	.remove = qpnp_remove,
> +	.driver = {
> +		.name = "qpnp-spmi",
> +		.of_match_table = qpnp_id_table,
> +	},
> +};
> +module_spmi_driver(qpnp_driver);

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
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: [RFC PATCH v2 1/5] mfd: qpnp: add support for Qualcomm QPNP PMICs

[Stanimir Varbanov]

Hi Lee,

Thanks for the comments.

On 07/09/2014 05:34 PM, Lee Jones wrote:
> On Thu, 03 Jul 2014, Stanimir Varbanov wrote:
>> From: Josh Cartwright <joshc@codeaurora.org>
>>
>> The Qualcomm QPNP PMIC chips are components used with the
>> Snapdragon 800 series SoC family.  This driver exists
>> largely as a glue mfd component, it exists to be an owner
>> of an SPMI regmap for children devices described in
>> device tree.
>>
>> Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
>> Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
>> ---
>>  drivers/mfd/Kconfig     |   15 ++++++
>>  drivers/mfd/Makefile    |    1 +
>>  drivers/mfd/qpnp-spmi.c |  129 +++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 145 insertions(+), 0 deletions(-)
>>  create mode 100644 drivers/mfd/qpnp-spmi.c
>>
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index ee8204c..258b733 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -524,6 +524,21 @@ config MFD_PM8921_CORE
>>  	  Say M here if you want to include support for PM8921 chip as a module.
>>  	  This will build a module called "pm8921-core".
>>  
>> +config MFD_QPNP_SPMI
>> +	tristate "Qualcomm QPNP SPMI PMIC"
>> +	depends on ARCH_QCOM || COMPILE_TEST
>> +	depends on OF
>> +	select MFD_CORE
>> +	select REGMAP_SPMI
>> +	help
>> +	  This enables support for the Qualcomm QPNP SPMI PMICs.
>> +	  These PMICs are currently used with the Snapdragon 800 series of
>> +	  SoCs.  Note, that this will only be useful paired with descriptions
>> +	  of the independent functions as children nodes in the device tree.
> 
> It would be helpful if the expected types of child devices were
> mentioned here in the help section.

They are many, but I could list the major ones.

> 
>> +	  Say M here if you want to include support for the QPNP SPMI PMIC
>> +	  series as a module.  The module will be called "qpnp-spmi".
>> +
>>  config MFD_RDC321X
>>  	tristate "RDC R-321x southbridge"
>>  	select MFD_CORE
>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>> index 8afedba..31833d7 100644
>> --- a/drivers/mfd/Makefile
>> +++ b/drivers/mfd/Makefile
>> @@ -153,6 +153,7 @@ obj-$(CONFIG_MFD_SI476X_CORE)	+= si476x-core.o
>>  obj-$(CONFIG_MFD_CS5535)	+= cs5535-mfd.o
>>  obj-$(CONFIG_MFD_OMAP_USB_HOST)	+= omap-usb-host.o omap-usb-tll.o
>>  obj-$(CONFIG_MFD_PM8921_CORE) 	+= pm8921-core.o ssbi.o
>> +obj-$(CONFIG_MFD_QPNP_SPMI)	+= qpnp-spmi.o
>>  obj-$(CONFIG_TPS65911_COMPARATOR)	+= tps65911-comparator.o
>>  obj-$(CONFIG_MFD_TPS65090)	+= tps65090.o
>>  obj-$(CONFIG_MFD_AAT2870_CORE)	+= aat2870-core.o
>> diff --git a/drivers/mfd/qpnp-spmi.c b/drivers/mfd/qpnp-spmi.c
>> new file mode 100644
>> index 0000000..efd7d3e
>> --- /dev/null
>> +++ b/drivers/mfd/qpnp-spmi.c
>> @@ -0,0 +1,129 @@
>> +/* Copyright (c) 2014, The Linux Foundation. All rights reserved.
>> + *
> 
> Non-standard multi-line comment.  First line should be blank.

sure, will fix.

> 
>> + * 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.
> 
> No author?
> 
>> + * 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/spmi.h>
>> +#include <linux/regmap.h>
>> +#include <linux/of_address.h>
>> +#include <linux/slab.h>
>> +#include <linux/mfd/core.h>
>> +
>> +#define QPNP_RESOURCE_SIZE	256
>> +
>> +static const struct regmap_config qpnp_regmap_config = {
>> +	.reg_bits	= 16,
>> +	.val_bits	= 8,
>> +	.max_register	= 0xffff,
>> +};
>> +
>> +static int qpnp_index_to_resource(struct device_node *np, int index,
>> +				  struct resource *res)
>> +{
>> +	const char *name = NULL;
>> +	const __be32 *addrp;
>> +	u64 addr;
>> +
>> +	addrp = of_get_address(np, index, NULL, NULL);
>> +	if (!addrp)
>> +		return -EINVAL;
>> +
>> +	addr = of_read_number(addrp, 1);
>> +	if (addr == OF_BAD_ADDR)
>> +		return -EINVAL;
>> +
>> +	of_property_read_string_index(np, "reg-names", index, &name);
>> +
>> +	res->start = addr;
>> +	res->end = addr + QPNP_RESOURCE_SIZE - 1;
>> +	res->flags = IORESOURCE_REG;
>> +	res->name = name ? name : np->name;
>> +
>> +	return 0;
>> +}
>> +
>> +static int qpnp_add_device(struct spmi_device *root, struct device_node *child)
>> +{
>> +	struct mfd_cell cell = {};
>> +	struct resource *res, *r;
>> +	int num_resources = 0;
>> +	const char *compat;
>> +	int ret, i;
>> +
>> +	compat = of_get_property(child, "compatible", NULL);
>> +	if (!compat)
>> +		return -ENODEV;
>> +
>> +	while (of_get_address(child, num_resources, NULL, NULL))
>> +		num_resources++;
>> +
>> +	if (!num_resources)
>> +		return -ENODEV;
>> +
>> +	res = kcalloc(num_resources, sizeof(*res), GFP_KERNEL);
>> +	if (!res)
>> +		return -ENOMEM;
>> +
>> +	r = res;
>> +	for (i = 0; i < num_resources; i++, r++)
>> +		qpnp_index_to_resource(child, i, r);
>> +
>> +	cell.name = kasprintf(GFP_KERNEL, "%x.%04x.%s", root->usid,
>> +			      (u16)res[0].start, child->name);
>> +	cell.of_compatible = compat;
>> +	cell.num_resources = num_resources;
>> +	cell.resources = res;
>> +
>> +	ret = mfd_add_devices(&root->dev, PLATFORM_DEVID_NONE, &cell, 1,
>> +			      NULL, 0, NULL);
>> +
>> +	kfree(res);
>> +	kfree(cell.name);
>> +
>> +	return ret;
>> +}
>> +
>> +static int qpnp_probe(struct spmi_device *sdev)
>> +{
>> +	struct device_node *root = sdev->dev.of_node;
>> +	struct device_node *child;
>> +	struct regmap *regmap;
>> +
>> +	regmap = devm_regmap_init_spmi_ext(sdev, &qpnp_regmap_config);
>> +	if (IS_ERR(regmap))
>> +		return PTR_ERR(regmap);
>> +
>> +	for_each_available_child_of_node(root, child)
>> +		qpnp_add_device(sdev, child);
> 
> This entire driver looks like a re-write of of_platform_populate().
> 
> Why?

of_platform_populate is not used because the PMIC function resources are
non-translatable. You can see that the resources are of type
IORESOURCE_REG (qpnp_index_to_resource()) not IORESOURCE_MEM or _IO.

The whole point of this mfd driver is to parse devicetree to prepare
resources for every child and create platform device for it through
mfd_add_devices(). Then the PMIC function driver got its resources and
use them as register addresses passed to regmap. These register accesses
hits the SPMI controller which is the physical interface between PMIC's
and SoC.

-- 
regards,
Stan

Re: [RFC PATCH v2 1/5] mfd: qpnp: add support for Qualcomm QPNP PMICs

[Lee Jones]

On Wed, 09 Jul 2014, Stanimir Varbanov wrote:

> Hi Lee,
> 
> Thanks for the comments.
> 
> On 07/09/2014 05:34 PM, Lee Jones wrote:
> > On Thu, 03 Jul 2014, Stanimir Varbanov wrote:
> >> From: Josh Cartwright <joshc@codeaurora.org>
> >>
> >> The Qualcomm QPNP PMIC chips are components used with the
> >> Snapdragon 800 series SoC family.  This driver exists
> >> largely as a glue mfd component, it exists to be an owner
> >> of an SPMI regmap for children devices described in
> >> device tree.
> >>
> >> Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
> >> Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
> >> ---
> >>  drivers/mfd/Kconfig     |   15 ++++++
> >>  drivers/mfd/Makefile    |    1 +
> >>  drivers/mfd/qpnp-spmi.c |  129 +++++++++++++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 145 insertions(+), 0 deletions(-)
> >>  create mode 100644 drivers/mfd/qpnp-spmi.c
> >>
> >> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> >> index ee8204c..258b733 100644
> >> --- a/drivers/mfd/Kconfig
> >> +++ b/drivers/mfd/Kconfig
> >> @@ -524,6 +524,21 @@ config MFD_PM8921_CORE
> >>  	  Say M here if you want to include support for PM8921 chip as a module.
> >>  	  This will build a module called "pm8921-core".
> >>  
> >> +config MFD_QPNP_SPMI
> >> +	tristate "Qualcomm QPNP SPMI PMIC"
> >> +	depends on ARCH_QCOM || COMPILE_TEST
> >> +	depends on OF
> >> +	select MFD_CORE
> >> +	select REGMAP_SPMI
> >> +	help
> >> +	  This enables support for the Qualcomm QPNP SPMI PMICs.
> >> +	  These PMICs are currently used with the Snapdragon 800 series of
> >> +	  SoCs.  Note, that this will only be useful paired with descriptions
> >> +	  of the independent functions as children nodes in the device tree.
> > 
> > It would be helpful if the expected types of child devices were
> > mentioned here in the help section.
> 
> They are many, but I could list the major ones.
> 
> > 
> >> +	  Say M here if you want to include support for the QPNP SPMI PMIC
> >> +	  series as a module.  The module will be called "qpnp-spmi".
> >> +
> >>  config MFD_RDC321X
> >>  	tristate "RDC R-321x southbridge"
> >>  	select MFD_CORE
> >> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> >> index 8afedba..31833d7 100644
> >> --- a/drivers/mfd/Makefile
> >> +++ b/drivers/mfd/Makefile
> >> @@ -153,6 +153,7 @@ obj-$(CONFIG_MFD_SI476X_CORE)	+= si476x-core.o
> >>  obj-$(CONFIG_MFD_CS5535)	+= cs5535-mfd.o
> >>  obj-$(CONFIG_MFD_OMAP_USB_HOST)	+= omap-usb-host.o omap-usb-tll.o
> >>  obj-$(CONFIG_MFD_PM8921_CORE) 	+= pm8921-core.o ssbi.o
> >> +obj-$(CONFIG_MFD_QPNP_SPMI)	+= qpnp-spmi.o
> >>  obj-$(CONFIG_TPS65911_COMPARATOR)	+= tps65911-comparator.o
> >>  obj-$(CONFIG_MFD_TPS65090)	+= tps65090.o
> >>  obj-$(CONFIG_MFD_AAT2870_CORE)	+= aat2870-core.o
> >> diff --git a/drivers/mfd/qpnp-spmi.c b/drivers/mfd/qpnp-spmi.c
> >> new file mode 100644
> >> index 0000000..efd7d3e
> >> --- /dev/null
> >> +++ b/drivers/mfd/qpnp-spmi.c
> >> @@ -0,0 +1,129 @@
> >> +/* Copyright (c) 2014, The Linux Foundation. All rights reserved.
> >> + *
> > 
> > Non-standard multi-line comment.  First line should be blank.
> 
> sure, will fix.
> 
> > 
> >> + * 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.
> > 
> > No author?
> > 
> >> + * 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/spmi.h>
> >> +#include <linux/regmap.h>
> >> +#include <linux/of_address.h>
> >> +#include <linux/slab.h>
> >> +#include <linux/mfd/core.h>
> >> +
> >> +#define QPNP_RESOURCE_SIZE	256
> >> +
> >> +static const struct regmap_config qpnp_regmap_config = {
> >> +	.reg_bits	= 16,
> >> +	.val_bits	= 8,
> >> +	.max_register	= 0xffff,
> >> +};
> >> +
> >> +static int qpnp_index_to_resource(struct device_node *np, int index,
> >> +				  struct resource *res)
> >> +{
> >> +	const char *name = NULL;
> >> +	const __be32 *addrp;
> >> +	u64 addr;
> >> +
> >> +	addrp = of_get_address(np, index, NULL, NULL);
> >> +	if (!addrp)
> >> +		return -EINVAL;
> >> +
> >> +	addr = of_read_number(addrp, 1);
> >> +	if (addr == OF_BAD_ADDR)
> >> +		return -EINVAL;
> >> +
> >> +	of_property_read_string_index(np, "reg-names", index, &name);
> >> +
> >> +	res->start = addr;
> >> +	res->end = addr + QPNP_RESOURCE_SIZE - 1;
> >> +	res->flags = IORESOURCE_REG;
> >> +	res->name = name ? name : np->name;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int qpnp_add_device(struct spmi_device *root, struct device_node *child)
> >> +{
> >> +	struct mfd_cell cell = {};
> >> +	struct resource *res, *r;
> >> +	int num_resources = 0;
> >> +	const char *compat;
> >> +	int ret, i;
> >> +
> >> +	compat = of_get_property(child, "compatible", NULL);
> >> +	if (!compat)
> >> +		return -ENODEV;
> >> +
> >> +	while (of_get_address(child, num_resources, NULL, NULL))
> >> +		num_resources++;
> >> +
> >> +	if (!num_resources)
> >> +		return -ENODEV;
> >> +
> >> +	res = kcalloc(num_resources, sizeof(*res), GFP_KERNEL);
> >> +	if (!res)
> >> +		return -ENOMEM;
> >> +
> >> +	r = res;
> >> +	for (i = 0; i < num_resources; i++, r++)
> >> +		qpnp_index_to_resource(child, i, r);
> >> +
> >> +	cell.name = kasprintf(GFP_KERNEL, "%x.%04x.%s", root->usid,
> >> +			      (u16)res[0].start, child->name);
> >> +	cell.of_compatible = compat;
> >> +	cell.num_resources = num_resources;
> >> +	cell.resources = res;
> >> +
> >> +	ret = mfd_add_devices(&root->dev, PLATFORM_DEVID_NONE, &cell, 1,
> >> +			      NULL, 0, NULL);
> >> +
> >> +	kfree(res);
> >> +	kfree(cell.name);
> >> +
> >> +	return ret;
> >> +}
> >> +
> >> +static int qpnp_probe(struct spmi_device *sdev)
> >> +{
> >> +	struct device_node *root = sdev->dev.of_node;
> >> +	struct device_node *child;
> >> +	struct regmap *regmap;
> >> +
> >> +	regmap = devm_regmap_init_spmi_ext(sdev, &qpnp_regmap_config);
> >> +	if (IS_ERR(regmap))
> >> +		return PTR_ERR(regmap);
> >> +
> >> +	for_each_available_child_of_node(root, child)
> >> +		qpnp_add_device(sdev, child);
> > 
> > This entire driver looks like a re-write of of_platform_populate().
> > 
> > Why?
> 
> of_platform_populate is not used because the PMIC function resources are
> non-translatable. You can see that the resources are of type
> IORESOURCE_REG (qpnp_index_to_resource()) not IORESOURCE_MEM or _IO.
> 
> The whole point of this mfd driver is to parse devicetree to prepare
> resources for every child and create platform device for it through
> mfd_add_devices(). Then the PMIC function driver got its resources and
> use them as register addresses passed to regmap. These register accesses
> hits the SPMI controller which is the physical interface between PMIC's
> and SoC.

I can't help but think that if this is required, it should be part of
the core OF code, rather than doing your own thing which looks
frighteningly like existing framework functionality.

Either way, I'm going to need a DT Ack before I accept this.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [RFC PATCH v2 1/5] mfd: qpnp: add support for Qualcomm QPNP PMICs

[Stanimir Varbanov]

Hi,

+Rob

Rob I used your old email, sorry about that.

<snip>

>>>> The Qualcomm QPNP PMIC chips are components used with the
>>>> Snapdragon 800 series SoC family.  This driver exists
>>>> largely as a glue mfd component, it exists to be an owner
>>>> of an SPMI regmap for children devices described in
>>>> device tree.
>>>>
>>>> Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
>>>> Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
>>>> ---
>>>>  drivers/mfd/Kconfig     |   15 ++++++
>>>>  drivers/mfd/Makefile    |    1 +
>>>>  drivers/mfd/qpnp-spmi.c |  129 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>  3 files changed, 145 insertions(+), 0 deletions(-)
>>>>  create mode 100644 drivers/mfd/qpnp-spmi.c
>>>>
>>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>>>> index ee8204c..258b733 100644
>>>> --- a/drivers/mfd/Kconfig
>>>> +++ b/drivers/mfd/Kconfig
>>>> @@ -524,6 +524,21 @@ config MFD_PM8921_CORE
>>>>  	  Say M here if you want to include support for PM8921 chip as a module.
>>>>  	  This will build a module called "pm8921-core".
>>>>  
>>>> +config MFD_QPNP_SPMI
>>>> +	tristate "Qualcomm QPNP SPMI PMIC"
>>>> +	depends on ARCH_QCOM || COMPILE_TEST
>>>> +	depends on OF
>>>> +	select MFD_CORE
>>>> +	select REGMAP_SPMI
>>>> +	help
>>>> +	  This enables support for the Qualcomm QPNP SPMI PMICs.
>>>> +	  These PMICs are currently used with the Snapdragon 800 series of
>>>> +	  SoCs.  Note, that this will only be useful paired with descriptions
>>>> +	  of the independent functions as children nodes in the device tree.
>>>

<snip>

>>>
>>>> + * 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/spmi.h>
>>>> +#include <linux/regmap.h>
>>>> +#include <linux/of_address.h>
>>>> +#include <linux/slab.h>
>>>> +#include <linux/mfd/core.h>
>>>> +
>>>> +#define QPNP_RESOURCE_SIZE	256
>>>> +
>>>> +static const struct regmap_config qpnp_regmap_config = {
>>>> +	.reg_bits	= 16,
>>>> +	.val_bits	= 8,
>>>> +	.max_register	= 0xffff,
>>>> +};
>>>> +
>>>> +static int qpnp_index_to_resource(struct device_node *np, int index,
>>>> +				  struct resource *res)
>>>> +{
>>>> +	const char *name = NULL;
>>>> +	const __be32 *addrp;
>>>> +	u64 addr;
>>>> +
>>>> +	addrp = of_get_address(np, index, NULL, NULL);
>>>> +	if (!addrp)
>>>> +		return -EINVAL;
>>>> +
>>>> +	addr = of_read_number(addrp, 1);
>>>> +	if (addr == OF_BAD_ADDR)
>>>> +		return -EINVAL;
>>>> +
>>>> +	of_property_read_string_index(np, "reg-names", index, &name);
>>>> +
>>>> +	res->start = addr;
>>>> +	res->end = addr + QPNP_RESOURCE_SIZE - 1;
>>>> +	res->flags = IORESOURCE_REG;
>>>> +	res->name = name ? name : np->name;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int qpnp_add_device(struct spmi_device *root, struct device_node *child)
>>>> +{
>>>> +	struct mfd_cell cell = {};
>>>> +	struct resource *res, *r;
>>>> +	int num_resources = 0;
>>>> +	const char *compat;
>>>> +	int ret, i;
>>>> +
>>>> +	compat = of_get_property(child, "compatible", NULL);
>>>> +	if (!compat)
>>>> +		return -ENODEV;
>>>> +
>>>> +	while (of_get_address(child, num_resources, NULL, NULL))
>>>> +		num_resources++;
>>>> +
>>>> +	if (!num_resources)
>>>> +		return -ENODEV;
>>>> +
>>>> +	res = kcalloc(num_resources, sizeof(*res), GFP_KERNEL);
>>>> +	if (!res)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	r = res;
>>>> +	for (i = 0; i < num_resources; i++, r++)
>>>> +		qpnp_index_to_resource(child, i, r);
>>>> +
>>>> +	cell.name = kasprintf(GFP_KERNEL, "%x.%04x.%s", root->usid,
>>>> +			      (u16)res[0].start, child->name);
>>>> +	cell.of_compatible = compat;
>>>> +	cell.num_resources = num_resources;
>>>> +	cell.resources = res;
>>>> +
>>>> +	ret = mfd_add_devices(&root->dev, PLATFORM_DEVID_NONE, &cell, 1,
>>>> +			      NULL, 0, NULL);
>>>> +
>>>> +	kfree(res);
>>>> +	kfree(cell.name);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static int qpnp_probe(struct spmi_device *sdev)
>>>> +{
>>>> +	struct device_node *root = sdev->dev.of_node;
>>>> +	struct device_node *child;
>>>> +	struct regmap *regmap;
>>>> +
>>>> +	regmap = devm_regmap_init_spmi_ext(sdev, &qpnp_regmap_config);
>>>> +	if (IS_ERR(regmap))
>>>> +		return PTR_ERR(regmap);
>>>> +
>>>> +	for_each_available_child_of_node(root, child)
>>>> +		qpnp_add_device(sdev, child);
>>>
>>> This entire driver looks like a re-write of of_platform_populate().
>>>
>>> Why?
>>
>> of_platform_populate is not used because the PMIC function resources are
>> non-translatable. You can see that the resources are of type
>> IORESOURCE_REG (qpnp_index_to_resource()) not IORESOURCE_MEM or _IO.
>>
>> The whole point of this mfd driver is to parse devicetree to prepare
>> resources for every child and create platform device for it through
>> mfd_add_devices(). Then the PMIC function driver got its resources and
>> use them as register addresses passed to regmap. These register accesses
>> hits the SPMI controller which is the physical interface between PMIC's
>> and SoC.
> 
> I can't help but think that if this is required, it should be part of
> the core OF code, rather than doing your own thing which looks
> frighteningly like existing framework functionality.

Lee,

does it make sense to have a common of_mfd code for devicetree parsing?
Is that discussed already? I mean presently the mfd_cell resources and
number of resources are passed by the mfd_add_devices users. Is it
possible to have common code which parses devicetree sub-nodes of the
parent device_node and fill resources/create platform devices for them.


Rob, Grant I hope you guys have something to say here?


> 
> Either way, I'm going to need a DT Ack before I accept this.
> 

Sure.

-- 
regards,
Stan

Re: [RFC PATCH v2 1/5] mfd: qpnp: add support for Qualcomm QPNP PMICs

[Lee Jones]

On Thu, 10 Jul 2014, Stanimir Varbanov wrote:
> >>>> The Qualcomm QPNP PMIC chips are components used with the
> >>>> Snapdragon 800 series SoC family.  This driver exists
> >>>> largely as a glue mfd component, it exists to be an owner
> >>>> of an SPMI regmap for children devices described in
> >>>> device tree.
> >>>>
> >>>> Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
> >>>> Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
> >>>> ---
> >>>>  drivers/mfd/Kconfig     |   15 ++++++
> >>>>  drivers/mfd/Makefile    |    1 +
> >>>>  drivers/mfd/qpnp-spmi.c |  129 +++++++++++++++++++++++++++++++++++++++++++++++
> >>>>  3 files changed, 145 insertions(+), 0 deletions(-)
> >>>>  create mode 100644 drivers/mfd/qpnp-spmi.c
> >>>>
> >>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> >>>> index ee8204c..258b733 100644
> >>>> --- a/drivers/mfd/Kconfig
> >>>> +++ b/drivers/mfd/Kconfig
> >>>> @@ -524,6 +524,21 @@ config MFD_PM8921_CORE
> >>>>  	  Say M here if you want to include support for PM8921 chip as a module.
> >>>>  	  This will build a module called "pm8921-core".
> >>>>  
> >>>> +config MFD_QPNP_SPMI
> >>>> +	tristate "Qualcomm QPNP SPMI PMIC"
> >>>> +	depends on ARCH_QCOM || COMPILE_TEST
> >>>> +	depends on OF
> >>>> +	select MFD_CORE
> >>>> +	select REGMAP_SPMI
> >>>> +	help
> >>>> +	  This enables support for the Qualcomm QPNP SPMI PMICs.
> >>>> +	  These PMICs are currently used with the Snapdragon 800 series of
> >>>> +	  SoCs.  Note, that this will only be useful paired with descriptions
> >>>> +	  of the independent functions as children nodes in the device tree.
> >>>
> 
> <snip>
> 
> >>>
> >>>> + * 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/spmi.h>
> >>>> +#include <linux/regmap.h>
> >>>> +#include <linux/of_address.h>
> >>>> +#include <linux/slab.h>
> >>>> +#include <linux/mfd/core.h>
> >>>> +
> >>>> +#define QPNP_RESOURCE_SIZE	256
> >>>> +
> >>>> +static const struct regmap_config qpnp_regmap_config = {
> >>>> +	.reg_bits	= 16,
> >>>> +	.val_bits	= 8,
> >>>> +	.max_register	= 0xffff,
> >>>> +};
> >>>> +
> >>>> +static int qpnp_index_to_resource(struct device_node *np, int index,
> >>>> +				  struct resource *res)
> >>>> +{
> >>>> +	const char *name = NULL;
> >>>> +	const __be32 *addrp;
> >>>> +	u64 addr;
> >>>> +
> >>>> +	addrp = of_get_address(np, index, NULL, NULL);
> >>>> +	if (!addrp)
> >>>> +		return -EINVAL;
> >>>> +
> >>>> +	addr = of_read_number(addrp, 1);
> >>>> +	if (addr == OF_BAD_ADDR)
> >>>> +		return -EINVAL;
> >>>> +
> >>>> +	of_property_read_string_index(np, "reg-names", index, &name);
> >>>> +
> >>>> +	res->start = addr;
> >>>> +	res->end = addr + QPNP_RESOURCE_SIZE - 1;
> >>>> +	res->flags = IORESOURCE_REG;
> >>>> +	res->name = name ? name : np->name;
> >>>> +
> >>>> +	return 0;
> >>>> +}
> >>>> +
> >>>> +static int qpnp_add_device(struct spmi_device *root, struct device_node *child)
> >>>> +{
> >>>> +	struct mfd_cell cell = {};
> >>>> +	struct resource *res, *r;
> >>>> +	int num_resources = 0;
> >>>> +	const char *compat;
> >>>> +	int ret, i;
> >>>> +
> >>>> +	compat = of_get_property(child, "compatible", NULL);
> >>>> +	if (!compat)
> >>>> +		return -ENODEV;
> >>>> +
> >>>> +	while (of_get_address(child, num_resources, NULL, NULL))
> >>>> +		num_resources++;
> >>>> +
> >>>> +	if (!num_resources)
> >>>> +		return -ENODEV;
> >>>> +
> >>>> +	res = kcalloc(num_resources, sizeof(*res), GFP_KERNEL);
> >>>> +	if (!res)
> >>>> +		return -ENOMEM;
> >>>> +
> >>>> +	r = res;
> >>>> +	for (i = 0; i < num_resources; i++, r++)
> >>>> +		qpnp_index_to_resource(child, i, r);
> >>>> +
> >>>> +	cell.name = kasprintf(GFP_KERNEL, "%x.%04x.%s", root->usid,
> >>>> +			      (u16)res[0].start, child->name);
> >>>> +	cell.of_compatible = compat;
> >>>> +	cell.num_resources = num_resources;
> >>>> +	cell.resources = res;
> >>>> +
> >>>> +	ret = mfd_add_devices(&root->dev, PLATFORM_DEVID_NONE, &cell, 1,
> >>>> +			      NULL, 0, NULL);
> >>>> +
> >>>> +	kfree(res);
> >>>> +	kfree(cell.name);
> >>>> +
> >>>> +	return ret;
> >>>> +}
> >>>> +
> >>>> +static int qpnp_probe(struct spmi_device *sdev)
> >>>> +{
> >>>> +	struct device_node *root = sdev->dev.of_node;
> >>>> +	struct device_node *child;
> >>>> +	struct regmap *regmap;
> >>>> +
> >>>> +	regmap = devm_regmap_init_spmi_ext(sdev, &qpnp_regmap_config);
> >>>> +	if (IS_ERR(regmap))
> >>>> +		return PTR_ERR(regmap);
> >>>> +
> >>>> +	for_each_available_child_of_node(root, child)
> >>>> +		qpnp_add_device(sdev, child);
> >>>
> >>> This entire driver looks like a re-write of of_platform_populate().
> >>>
> >>> Why?
> >>
> >> of_platform_populate is not used because the PMIC function resources are
> >> non-translatable. You can see that the resources are of type
> >> IORESOURCE_REG (qpnp_index_to_resource()) not IORESOURCE_MEM or _IO.
> >>
> >> The whole point of this mfd driver is to parse devicetree to prepare
> >> resources for every child and create platform device for it through
> >> mfd_add_devices(). Then the PMIC function driver got its resources and
> >> use them as register addresses passed to regmap. These register accesses
> >> hits the SPMI controller which is the physical interface between PMIC's
> >> and SoC.
> > 
> > I can't help but think that if this is required, it should be part of
> > the core OF code, rather than doing your own thing which looks
> > frighteningly like existing framework functionality.
> 
> does it make sense to have a common of_mfd code for devicetree parsing?
> Is that discussed already? I mean presently the mfd_cell resources and
> number of resources are passed by the mfd_add_devices users. Is it
> possible to have common code which parses devicetree sub-nodes of the
> parent device_node and fill resources/create platform devices for them.

I'm not sure it does.  Normally users _either_ represent devices in
MFD cells from within the driver _or_ populate using existing DT
interfaces i.e. of_platform_populate().  This is the first time I've
seen someone attempt to parse the entire MFD node structure from
within a driver.

After searching for some documentation to try and figure this out, I
noticed that you've also missed a patch from your set:

  mfd: qpnp-spmi: document DT bindings for Qualcomm QPNP PMICs

... which might help enlighten the DT guys.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [RFC PATCH v2 1/5] mfd: qpnp: add support for Qualcomm QPNP PMICs

[Stanimir Varbanov]

Hi Lee,

On 07/11/2014 12:07 PM, Lee Jones wrote:
> On Thu, 10 Jul 2014, Stanimir Varbanov wrote:
>>>>>> The Qualcomm QPNP PMIC chips are components used with the
>>>>>> Snapdragon 800 series SoC family.  This driver exists
>>>>>> largely as a glue mfd component, it exists to be an owner
>>>>>> of an SPMI regmap for children devices described in
>>>>>> device tree.
>>>>>>
>>>>>> Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
>>>>>> Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
>>>>>> ---
>>>>>>  drivers/mfd/Kconfig     |   15 ++++++
>>>>>>  drivers/mfd/Makefile    |    1 +
>>>>>>  drivers/mfd/qpnp-spmi.c |  129 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>  3 files changed, 145 insertions(+), 0 deletions(-)
>>>>>>  create mode 100644 drivers/mfd/qpnp-spmi.c
>>>>>>
>>>>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>>>>>> index ee8204c..258b733 100644
>>>>>> --- a/drivers/mfd/Kconfig
>>>>>> +++ b/drivers/mfd/Kconfig
>>>>>> @@ -524,6 +524,21 @@ config MFD_PM8921_CORE
>>>>>>  	  Say M here if you want to include support for PM8921 chip as a module.
>>>>>>  	  This will build a module called "pm8921-core".
>>>>>>  
>>>>>> +config MFD_QPNP_SPMI
>>>>>> +	tristate "Qualcomm QPNP SPMI PMIC"
>>>>>> +	depends on ARCH_QCOM || COMPILE_TEST
>>>>>> +	depends on OF
>>>>>> +	select MFD_CORE
>>>>>> +	select REGMAP_SPMI
>>>>>> +	help
>>>>>> +	  This enables support for the Qualcomm QPNP SPMI PMICs.
>>>>>> +	  These PMICs are currently used with the Snapdragon 800 series of
>>>>>> +	  SoCs.  Note, that this will only be useful paired with descriptions
>>>>>> +	  of the independent functions as children nodes in the device tree.
>>>>>
>>
>> <snip>
>>
>>>>>
>>>>>> + * 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/spmi.h>
>>>>>> +#include <linux/regmap.h>
>>>>>> +#include <linux/of_address.h>
>>>>>> +#include <linux/slab.h>
>>>>>> +#include <linux/mfd/core.h>
>>>>>> +
>>>>>> +#define QPNP_RESOURCE_SIZE	256
>>>>>> +
>>>>>> +static const struct regmap_config qpnp_regmap_config = {
>>>>>> +	.reg_bits	= 16,
>>>>>> +	.val_bits	= 8,
>>>>>> +	.max_register	= 0xffff,
>>>>>> +};
>>>>>> +
>>>>>> +static int qpnp_index_to_resource(struct device_node *np, int index,
>>>>>> +				  struct resource *res)
>>>>>> +{
>>>>>> +	const char *name = NULL;
>>>>>> +	const __be32 *addrp;
>>>>>> +	u64 addr;
>>>>>> +
>>>>>> +	addrp = of_get_address(np, index, NULL, NULL);
>>>>>> +	if (!addrp)
>>>>>> +		return -EINVAL;
>>>>>> +
>>>>>> +	addr = of_read_number(addrp, 1);
>>>>>> +	if (addr == OF_BAD_ADDR)
>>>>>> +		return -EINVAL;
>>>>>> +
>>>>>> +	of_property_read_string_index(np, "reg-names", index, &name);
>>>>>> +
>>>>>> +	res->start = addr;
>>>>>> +	res->end = addr + QPNP_RESOURCE_SIZE - 1;
>>>>>> +	res->flags = IORESOURCE_REG;
>>>>>> +	res->name = name ? name : np->name;
>>>>>> +
>>>>>> +	return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int qpnp_add_device(struct spmi_device *root, struct device_node *child)
>>>>>> +{
>>>>>> +	struct mfd_cell cell = {};
>>>>>> +	struct resource *res, *r;
>>>>>> +	int num_resources = 0;
>>>>>> +	const char *compat;
>>>>>> +	int ret, i;
>>>>>> +
>>>>>> +	compat = of_get_property(child, "compatible", NULL);
>>>>>> +	if (!compat)
>>>>>> +		return -ENODEV;
>>>>>> +
>>>>>> +	while (of_get_address(child, num_resources, NULL, NULL))
>>>>>> +		num_resources++;
>>>>>> +
>>>>>> +	if (!num_resources)
>>>>>> +		return -ENODEV;
>>>>>> +
>>>>>> +	res = kcalloc(num_resources, sizeof(*res), GFP_KERNEL);
>>>>>> +	if (!res)
>>>>>> +		return -ENOMEM;
>>>>>> +
>>>>>> +	r = res;
>>>>>> +	for (i = 0; i < num_resources; i++, r++)
>>>>>> +		qpnp_index_to_resource(child, i, r);
>>>>>> +
>>>>>> +	cell.name = kasprintf(GFP_KERNEL, "%x.%04x.%s", root->usid,
>>>>>> +			      (u16)res[0].start, child->name);
>>>>>> +	cell.of_compatible = compat;
>>>>>> +	cell.num_resources = num_resources;
>>>>>> +	cell.resources = res;
>>>>>> +
>>>>>> +	ret = mfd_add_devices(&root->dev, PLATFORM_DEVID_NONE, &cell, 1,
>>>>>> +			      NULL, 0, NULL);
>>>>>> +
>>>>>> +	kfree(res);
>>>>>> +	kfree(cell.name);
>>>>>> +
>>>>>> +	return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static int qpnp_probe(struct spmi_device *sdev)
>>>>>> +{
>>>>>> +	struct device_node *root = sdev->dev.of_node;
>>>>>> +	struct device_node *child;
>>>>>> +	struct regmap *regmap;
>>>>>> +
>>>>>> +	regmap = devm_regmap_init_spmi_ext(sdev, &qpnp_regmap_config);
>>>>>> +	if (IS_ERR(regmap))
>>>>>> +		return PTR_ERR(regmap);
>>>>>> +
>>>>>> +	for_each_available_child_of_node(root, child)
>>>>>> +		qpnp_add_device(sdev, child);
>>>>>
>>>>> This entire driver looks like a re-write of of_platform_populate().
>>>>>
>>>>> Why?
>>>>
>>>> of_platform_populate is not used because the PMIC function resources are
>>>> non-translatable. You can see that the resources are of type
>>>> IORESOURCE_REG (qpnp_index_to_resource()) not IORESOURCE_MEM or _IO.
>>>>
>>>> The whole point of this mfd driver is to parse devicetree to prepare
>>>> resources for every child and create platform device for it through
>>>> mfd_add_devices(). Then the PMIC function driver got its resources and
>>>> use them as register addresses passed to regmap. These register accesses
>>>> hits the SPMI controller which is the physical interface between PMIC's
>>>> and SoC.
>>>
>>> I can't help but think that if this is required, it should be part of
>>> the core OF code, rather than doing your own thing which looks
>>> frighteningly like existing framework functionality.
>>
>> does it make sense to have a common of_mfd code for devicetree parsing?
>> Is that discussed already? I mean presently the mfd_cell resources and
>> number of resources are passed by the mfd_add_devices users. Is it
>> possible to have common code which parses devicetree sub-nodes of the
>> parent device_node and fill resources/create platform devices for them.
> 
> I'm not sure it does.  Normally users _either_ represent devices in
> MFD cells from within the driver _or_ populate using existing DT
> interfaces i.e. of_platform_populate().  This is the first time I've
> seen someone attempt to parse the entire MFD node structure from
> within a driver.

Our goal is to use common mfd driver for various Qualcomm PMIC from the
same PMIC generation (codenamed QPNP). Presently we have good
abstraction of the register manipulation functions through regmap-spmi
layer. Using the devicetree to represent different peripherals base
addresses (reg property) gives us a benefit by avoiding huge header
files in linux/mfd and make flexible transition to new PMIC's which just
change the base addresses but keep the IP block the same. This way we
should change the devicetree binding without touch the drivers.

> 
> After searching for some documentation to try and figure this out, I
> noticed that you've also missed a patch from your set:
> 
>   mfd: qpnp-spmi: document DT bindings for Qualcomm QPNP PMICs

I sent a new version without RFC tag which includes this binding
document [1].

> 
> ... which might help enlighten the DT guys.
> 

-- 
regards,
Stan

[1] https://lkml.org/lkml/2014/7/8/428

Re: [RFC PATCH v2 1/5] mfd: qpnp: add support for Qualcomm QPNP PMICs

[Lee Jones]

On Mon, 14 Jul 2014, Stanimir Varbanov wrote:
> On 07/11/2014 12:07 PM, Lee Jones wrote:
> > On Thu, 10 Jul 2014, Stanimir Varbanov wrote:
> >>>>>> The Qualcomm QPNP PMIC chips are components used with the
> >>>>>> Snapdragon 800 series SoC family.  This driver exists
> >>>>>> largely as a glue mfd component, it exists to be an owner
> >>>>>> of an SPMI regmap for children devices described in
> >>>>>> device tree.
> >>>>>>
> >>>>>> Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
> >>>>>> Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
> >>>>>> ---
> >>>>>>  drivers/mfd/Kconfig     |   15 ++++++
> >>>>>>  drivers/mfd/Makefile    |    1 +
> >>>>>>  drivers/mfd/qpnp-spmi.c |  129 +++++++++++++++++++++++++++++++++++++++++++++++
> >>>>>>  3 files changed, 145 insertions(+), 0 deletions(-)
> >>>>>>  create mode 100644 drivers/mfd/qpnp-spmi.c
> >>>>>>
> >>>>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> >>>>>> index ee8204c..258b733 100644
> >>>>>> --- a/drivers/mfd/Kconfig
> >>>>>> +++ b/drivers/mfd/Kconfig
> >>>>>> @@ -524,6 +524,21 @@ config MFD_PM8921_CORE
> >>>>>>  	  Say M here if you want to include support for PM8921 chip as a module.
> >>>>>>  	  This will build a module called "pm8921-core".
> >>>>>>  
> >>>>>> +config MFD_QPNP_SPMI
> >>>>>> +	tristate "Qualcomm QPNP SPMI PMIC"
> >>>>>> +	depends on ARCH_QCOM || COMPILE_TEST
> >>>>>> +	depends on OF
> >>>>>> +	select MFD_CORE
> >>>>>> +	select REGMAP_SPMI
> >>>>>> +	help
> >>>>>> +	  This enables support for the Qualcomm QPNP SPMI PMICs.
> >>>>>> +	  These PMICs are currently used with the Snapdragon 800 series of
> >>>>>> +	  SoCs.  Note, that this will only be useful paired with descriptions
> >>>>>> +	  of the independent functions as children nodes in the device tree.
> >>>>>
> >>
> >> <snip>
> >>
> >>>>>
> >>>>>> + * 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/spmi.h>
> >>>>>> +#include <linux/regmap.h>
> >>>>>> +#include <linux/of_address.h>
> >>>>>> +#include <linux/slab.h>
> >>>>>> +#include <linux/mfd/core.h>
> >>>>>> +
> >>>>>> +#define QPNP_RESOURCE_SIZE	256
> >>>>>> +
> >>>>>> +static const struct regmap_config qpnp_regmap_config = {
> >>>>>> +	.reg_bits	= 16,
> >>>>>> +	.val_bits	= 8,
> >>>>>> +	.max_register	= 0xffff,
> >>>>>> +};
> >>>>>> +
> >>>>>> +static int qpnp_index_to_resource(struct device_node *np, int index,
> >>>>>> +				  struct resource *res)
> >>>>>> +{
> >>>>>> +	const char *name = NULL;
> >>>>>> +	const __be32 *addrp;
> >>>>>> +	u64 addr;
> >>>>>> +
> >>>>>> +	addrp = of_get_address(np, index, NULL, NULL);
> >>>>>> +	if (!addrp)
> >>>>>> +		return -EINVAL;
> >>>>>> +
> >>>>>> +	addr = of_read_number(addrp, 1);
> >>>>>> +	if (addr == OF_BAD_ADDR)
> >>>>>> +		return -EINVAL;
> >>>>>> +
> >>>>>> +	of_property_read_string_index(np, "reg-names", index, &name);
> >>>>>> +
> >>>>>> +	res->start = addr;
> >>>>>> +	res->end = addr + QPNP_RESOURCE_SIZE - 1;
> >>>>>> +	res->flags = IORESOURCE_REG;
> >>>>>> +	res->name = name ? name : np->name;
> >>>>>> +
> >>>>>> +	return 0;
> >>>>>> +}
> >>>>>> +
> >>>>>> +static int qpnp_add_device(struct spmi_device *root, struct device_node *child)
> >>>>>> +{
> >>>>>> +	struct mfd_cell cell = {};
> >>>>>> +	struct resource *res, *r;
> >>>>>> +	int num_resources = 0;
> >>>>>> +	const char *compat;
> >>>>>> +	int ret, i;
> >>>>>> +
> >>>>>> +	compat = of_get_property(child, "compatible", NULL);
> >>>>>> +	if (!compat)
> >>>>>> +		return -ENODEV;
> >>>>>> +
> >>>>>> +	while (of_get_address(child, num_resources, NULL, NULL))
> >>>>>> +		num_resources++;
> >>>>>> +
> >>>>>> +	if (!num_resources)
> >>>>>> +		return -ENODEV;
> >>>>>> +
> >>>>>> +	res = kcalloc(num_resources, sizeof(*res), GFP_KERNEL);
> >>>>>> +	if (!res)
> >>>>>> +		return -ENOMEM;
> >>>>>> +
> >>>>>> +	r = res;
> >>>>>> +	for (i = 0; i < num_resources; i++, r++)
> >>>>>> +		qpnp_index_to_resource(child, i, r);
> >>>>>> +
> >>>>>> +	cell.name = kasprintf(GFP_KERNEL, "%x.%04x.%s", root->usid,
> >>>>>> +			      (u16)res[0].start, child->name);
> >>>>>> +	cell.of_compatible = compat;
> >>>>>> +	cell.num_resources = num_resources;
> >>>>>> +	cell.resources = res;
> >>>>>> +
> >>>>>> +	ret = mfd_add_devices(&root->dev, PLATFORM_DEVID_NONE, &cell, 1,
> >>>>>> +			      NULL, 0, NULL);
> >>>>>> +
> >>>>>> +	kfree(res);
> >>>>>> +	kfree(cell.name);
> >>>>>> +
> >>>>>> +	return ret;
> >>>>>> +}
> >>>>>> +
> >>>>>> +static int qpnp_probe(struct spmi_device *sdev)
> >>>>>> +{
> >>>>>> +	struct device_node *root = sdev->dev.of_node;
> >>>>>> +	struct device_node *child;
> >>>>>> +	struct regmap *regmap;
> >>>>>> +
> >>>>>> +	regmap = devm_regmap_init_spmi_ext(sdev, &qpnp_regmap_config);
> >>>>>> +	if (IS_ERR(regmap))
> >>>>>> +		return PTR_ERR(regmap);
> >>>>>> +
> >>>>>> +	for_each_available_child_of_node(root, child)
> >>>>>> +		qpnp_add_device(sdev, child);
> >>>>>
> >>>>> This entire driver looks like a re-write of of_platform_populate().
> >>>>>
> >>>>> Why?
> >>>>
> >>>> of_platform_populate is not used because the PMIC function resources are
> >>>> non-translatable. You can see that the resources are of type
> >>>> IORESOURCE_REG (qpnp_index_to_resource()) not IORESOURCE_MEM or _IO.
> >>>>
> >>>> The whole point of this mfd driver is to parse devicetree to prepare
> >>>> resources for every child and create platform device for it through
> >>>> mfd_add_devices(). Then the PMIC function driver got its resources and
> >>>> use them as register addresses passed to regmap. These register accesses
> >>>> hits the SPMI controller which is the physical interface between PMIC's
> >>>> and SoC.
> >>>
> >>> I can't help but think that if this is required, it should be part of
> >>> the core OF code, rather than doing your own thing which looks
> >>> frighteningly like existing framework functionality.
> >>
> >> does it make sense to have a common of_mfd code for devicetree parsing?
> >> Is that discussed already? I mean presently the mfd_cell resources and
> >> number of resources are passed by the mfd_add_devices users. Is it
> >> possible to have common code which parses devicetree sub-nodes of the
> >> parent device_node and fill resources/create platform devices for them.
> > 
> > I'm not sure it does.  Normally users _either_ represent devices in
> > MFD cells from within the driver _or_ populate using existing DT
> > interfaces i.e. of_platform_populate().  This is the first time I've
> > seen someone attempt to parse the entire MFD node structure from
> > within a driver.
> 
> Our goal is to use common mfd driver for various Qualcomm PMIC from the
> same PMIC generation (codenamed QPNP). Presently we have good
> abstraction of the register manipulation functions through regmap-spmi
> layer. Using the devicetree to represent different peripherals base
> addresses (reg property) gives us a benefit by avoiding huge header
> files in linux/mfd and make flexible transition to new PMIC's which just
> change the base addresses but keep the IP block the same. This way we
> should change the devicetree binding without touch the drivers.

I'm still not convinced that you need to write your own parser.  If
you're concerned about not translating the reg property, use (or
don't) the 'ranges' property accordingly.

> > After searching for some documentation to try and figure this out, I
> > noticed that you've also missed a patch from your set:
> > 
> >   mfd: qpnp-spmi: document DT bindings for Qualcomm QPNP PMICs
> 
> I sent a new version without RFC tag which includes this binding
> document [1].
> 
> > 
> > ... which might help enlighten the DT guys.
> > 
> 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [RFC PATCH v2 1/5] mfd: qpnp: add support for Qualcomm QPNP PMICs

[Stanimir Varbanov]

On 07/14/2014 05:03 PM, Lee Jones wrote:
> On Mon, 14 Jul 2014, Stanimir Varbanov wrote:
>> On 07/11/2014 12:07 PM, Lee Jones wrote:
>>> On Thu, 10 Jul 2014, Stanimir Varbanov wrote:
>>>>>>>> The Qualcomm QPNP PMIC chips are components used with the
>>>>>>>> Snapdragon 800 series SoC family.  This driver exists
>>>>>>>> largely as a glue mfd component, it exists to be an owner
>>>>>>>> of an SPMI regmap for children devices described in
>>>>>>>> device tree.
>>>>>>>>
>>>>>>>> Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
>>>>>>>> Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
>>>>>>>> ---
>>>>>>>>  drivers/mfd/Kconfig     |   15 ++++++
>>>>>>>>  drivers/mfd/Makefile    |    1 +
>>>>>>>>  drivers/mfd/qpnp-spmi.c |  129 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>  3 files changed, 145 insertions(+), 0 deletions(-)
>>>>>>>>  create mode 100644 drivers/mfd/qpnp-spmi.c
>>>>>>>>
>>>>>>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>>>>>>>> index ee8204c..258b733 100644
>>>>>>>> --- a/drivers/mfd/Kconfig
>>>>>>>> +++ b/drivers/mfd/Kconfig
>>>>>>>> @@ -524,6 +524,21 @@ config MFD_PM8921_CORE
>>>>>>>>  	  Say M here if you want to include support for PM8921 chip as a module.
>>>>>>>>  	  This will build a module called "pm8921-core".
>>>>>>>>  
>>>>>>>> +config MFD_QPNP_SPMI
>>>>>>>> +	tristate "Qualcomm QPNP SPMI PMIC"
>>>>>>>> +	depends on ARCH_QCOM || COMPILE_TEST
>>>>>>>> +	depends on OF
>>>>>>>> +	select MFD_CORE
>>>>>>>> +	select REGMAP_SPMI
>>>>>>>> +	help
>>>>>>>> +	  This enables support for the Qualcomm QPNP SPMI PMICs.
>>>>>>>> +	  These PMICs are currently used with the Snapdragon 800 series of
>>>>>>>> +	  SoCs.  Note, that this will only be useful paired with descriptions
>>>>>>>> +	  of the independent functions as children nodes in the device tree.
>>>>>>>
>>>>
>>>> <snip>
>>>>
>>>>>>>
>>>>>>>> + * 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/spmi.h>
>>>>>>>> +#include <linux/regmap.h>
>>>>>>>> +#include <linux/of_address.h>
>>>>>>>> +#include <linux/slab.h>
>>>>>>>> +#include <linux/mfd/core.h>
>>>>>>>> +
>>>>>>>> +#define QPNP_RESOURCE_SIZE	256
>>>>>>>> +
>>>>>>>> +static const struct regmap_config qpnp_regmap_config = {
>>>>>>>> +	.reg_bits	= 16,
>>>>>>>> +	.val_bits	= 8,
>>>>>>>> +	.max_register	= 0xffff,
>>>>>>>> +};
>>>>>>>> +
>>>>>>>> +static int qpnp_index_to_resource(struct device_node *np, int index,
>>>>>>>> +				  struct resource *res)
>>>>>>>> +{
>>>>>>>> +	const char *name = NULL;
>>>>>>>> +	const __be32 *addrp;
>>>>>>>> +	u64 addr;
>>>>>>>> +
>>>>>>>> +	addrp = of_get_address(np, index, NULL, NULL);
>>>>>>>> +	if (!addrp)
>>>>>>>> +		return -EINVAL;
>>>>>>>> +
>>>>>>>> +	addr = of_read_number(addrp, 1);
>>>>>>>> +	if (addr == OF_BAD_ADDR)
>>>>>>>> +		return -EINVAL;
>>>>>>>> +
>>>>>>>> +	of_property_read_string_index(np, "reg-names", index, &name);
>>>>>>>> +
>>>>>>>> +	res->start = addr;
>>>>>>>> +	res->end = addr + QPNP_RESOURCE_SIZE - 1;
>>>>>>>> +	res->flags = IORESOURCE_REG;
>>>>>>>> +	res->name = name ? name : np->name;
>>>>>>>> +
>>>>>>>> +	return 0;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static int qpnp_add_device(struct spmi_device *root, struct device_node *child)
>>>>>>>> +{
>>>>>>>> +	struct mfd_cell cell = {};
>>>>>>>> +	struct resource *res, *r;
>>>>>>>> +	int num_resources = 0;
>>>>>>>> +	const char *compat;
>>>>>>>> +	int ret, i;
>>>>>>>> +
>>>>>>>> +	compat = of_get_property(child, "compatible", NULL);
>>>>>>>> +	if (!compat)
>>>>>>>> +		return -ENODEV;
>>>>>>>> +
>>>>>>>> +	while (of_get_address(child, num_resources, NULL, NULL))
>>>>>>>> +		num_resources++;
>>>>>>>> +
>>>>>>>> +	if (!num_resources)
>>>>>>>> +		return -ENODEV;
>>>>>>>> +
>>>>>>>> +	res = kcalloc(num_resources, sizeof(*res), GFP_KERNEL);
>>>>>>>> +	if (!res)
>>>>>>>> +		return -ENOMEM;
>>>>>>>> +
>>>>>>>> +	r = res;
>>>>>>>> +	for (i = 0; i < num_resources; i++, r++)
>>>>>>>> +		qpnp_index_to_resource(child, i, r);
>>>>>>>> +
>>>>>>>> +	cell.name = kasprintf(GFP_KERNEL, "%x.%04x.%s", root->usid,
>>>>>>>> +			      (u16)res[0].start, child->name);
>>>>>>>> +	cell.of_compatible = compat;
>>>>>>>> +	cell.num_resources = num_resources;
>>>>>>>> +	cell.resources = res;
>>>>>>>> +
>>>>>>>> +	ret = mfd_add_devices(&root->dev, PLATFORM_DEVID_NONE, &cell, 1,
>>>>>>>> +			      NULL, 0, NULL);
>>>>>>>> +
>>>>>>>> +	kfree(res);
>>>>>>>> +	kfree(cell.name);
>>>>>>>> +
>>>>>>>> +	return ret;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static int qpnp_probe(struct spmi_device *sdev)
>>>>>>>> +{
>>>>>>>> +	struct device_node *root = sdev->dev.of_node;
>>>>>>>> +	struct device_node *child;
>>>>>>>> +	struct regmap *regmap;
>>>>>>>> +
>>>>>>>> +	regmap = devm_regmap_init_spmi_ext(sdev, &qpnp_regmap_config);
>>>>>>>> +	if (IS_ERR(regmap))
>>>>>>>> +		return PTR_ERR(regmap);
>>>>>>>> +
>>>>>>>> +	for_each_available_child_of_node(root, child)
>>>>>>>> +		qpnp_add_device(sdev, child);
>>>>>>>
>>>>>>> This entire driver looks like a re-write of of_platform_populate().
>>>>>>>
>>>>>>> Why?
>>>>>>
>>>>>> of_platform_populate is not used because the PMIC function resources are
>>>>>> non-translatable. You can see that the resources are of type
>>>>>> IORESOURCE_REG (qpnp_index_to_resource()) not IORESOURCE_MEM or _IO.
>>>>>>
>>>>>> The whole point of this mfd driver is to parse devicetree to prepare
>>>>>> resources for every child and create platform device for it through
>>>>>> mfd_add_devices(). Then the PMIC function driver got its resources and
>>>>>> use them as register addresses passed to regmap. These register accesses
>>>>>> hits the SPMI controller which is the physical interface between PMIC's
>>>>>> and SoC.
>>>>>
>>>>> I can't help but think that if this is required, it should be part of
>>>>> the core OF code, rather than doing your own thing which looks
>>>>> frighteningly like existing framework functionality.
>>>>
>>>> does it make sense to have a common of_mfd code for devicetree parsing?
>>>> Is that discussed already? I mean presently the mfd_cell resources and
>>>> number of resources are passed by the mfd_add_devices users. Is it
>>>> possible to have common code which parses devicetree sub-nodes of the
>>>> parent device_node and fill resources/create platform devices for them.
>>>
>>> I'm not sure it does.  Normally users _either_ represent devices in
>>> MFD cells from within the driver _or_ populate using existing DT
>>> interfaces i.e. of_platform_populate().  This is the first time I've
>>> seen someone attempt to parse the entire MFD node structure from
>>> within a driver.
>>
>> Our goal is to use common mfd driver for various Qualcomm PMIC from the
>> same PMIC generation (codenamed QPNP). Presently we have good
>> abstraction of the register manipulation functions through regmap-spmi
>> layer. Using the devicetree to represent different peripherals base
>> addresses (reg property) gives us a benefit by avoiding huge header
>> files in linux/mfd and make flexible transition to new PMIC's which just
>> change the base addresses but keep the IP block the same. This way we
>> should change the devicetree binding without touch the drivers.
> 
> I'm still not convinced that you need to write your own parser.  If
> you're concerned about not translating the reg property, use (or
> don't) the 'ranges' property accordingly.

Unfortunately "ranges" property doesn't help here. The pmic addresses
parsed from "reg" properties are SPMI addresses (similar to I2C addresses).

-- 
regards,
Stan

[RFC PATCH v2 2/5] dt: qcom: msm8974: add qpnp-spmi device nodes

[Stanimir Varbanov]

From: Ivan T. Ivanov <iivanov-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org>

The qpnp-spmi device nodes are childrens of spmi pmic arbiter.
msm8974 SoC using two pmic chips pm8941 and pm8841. Every chip
has two spmi-qpnp bus id's.

Signed-off-by: Stanimir Varbanov <svarbanov-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/boot/dts/qcom-msm8974.dtsi |   45 +++++++++++++++++++++++++++++++++++
 1 files changed, 45 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/qcom-msm8974.dtsi b/arch/arm/boot/dts/qcom-msm8974.dtsi
index 69dca2a..9166b8e 100644
--- a/arch/arm/boot/dts/qcom-msm8974.dtsi
+++ b/arch/arm/boot/dts/qcom-msm8974.dtsi
@@ -3,6 +3,7 @@
 #include "skeleton.dtsi"
 
 #include <dt-bindings/clock/qcom,gcc-msm8974.h>
+#include <dt-bindings/spmi/spmi.h>
 
 / {
 	model = "Qualcomm MSM8974";
@@ -236,5 +237,49 @@
 			#interrupt-cells = <2>;
 			interrupts = <0 208 0>;
 		};
+
+		spmi@fc4cf000 {
+			compatible = "qcom,spmi-pmic-arb";
+			reg-names = "core", "intr", "cnfg";
+			reg = <0xfc4cf000 0x1000>,
+			      <0xfc4cb000 0x1000>,
+			      <0xfc4ca000 0x1000>;
+			interrupt-names = "periph_irq";
+			interrupts = <0 190 0>;
+			qcom,ee = <0>;
+			qcom,channel = <0>;
+			#address-cells = <2>;
+			#size-cells = <0>;
+			interrupt-controller;
+			#interrupt-cells = <4>;
+
+			usid0: pm8941@0 {
+				compatible = "qcom,qpnp-spmi";
+				reg = <0x0 SPMI_USID>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+			};
+
+			usid1: pm8941@1 {
+				compatible = "qcom,qpnp-spmi";
+				reg = <0x1 SPMI_USID>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+			};
+
+			usid4: pm8841@4 {
+				compatible = "qcom,qpnp-spmi";
+				reg = <0x4 SPMI_USID>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+			};
+
+			usid5: pm8841@5 {
+				compatible = "qcom,qpnp-spmi";
+				reg = <0x5 SPMI_USID>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+			};
+		};
 	};
 };
-- 
1.7.0.4

--
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: [RFC PATCH v2 2/5] dt: qcom: msm8974: add qpnp-spmi device nodes

[Lee Jones]

On Thu, 03 Jul 2014, Stanimir Varbanov wrote:

> From: Ivan T. Ivanov <iivanov-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org>
> 
> The qpnp-spmi device nodes are childrens of spmi pmic arbiter.
> msm8974 SoC using two pmic chips pm8941 and pm8841. Every chip
> has two spmi-qpnp bus id's.
> 
> Signed-off-by: Stanimir Varbanov <svarbanov-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org>
> ---
>  arch/arm/boot/dts/qcom-msm8974.dtsi |   45 +++++++++++++++++++++++++++++++++++
>  1 files changed, 45 insertions(+), 0 deletions(-)

Subject should start with 'ARM: ' for all of your arch/arm patches.

Rest of the patch unreviewed by me.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
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: [RFC PATCH v2 2/5] dt: qcom: msm8974: add qpnp-spmi device nodes

[Stanimir Varbanov]

On 07/09/2014 05:10 PM, Lee Jones wrote:
> On Thu, 03 Jul 2014, Stanimir Varbanov wrote:
> 
>> From: Ivan T. Ivanov <iivanov@mm-sol.com>
>>
>> The qpnp-spmi device nodes are childrens of spmi pmic arbiter.
>> msm8974 SoC using two pmic chips pm8941 and pm8841. Every chip
>> has two spmi-qpnp bus id's.
>>
>> Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
>> ---
>>  arch/arm/boot/dts/qcom-msm8974.dtsi |   45 +++++++++++++++++++++++++++++++++++
>>  1 files changed, 45 insertions(+), 0 deletions(-)
> 
> Subject should start with 'ARM: ' for all of your arch/arm patches.

Sure, will correct the subject, thanks.

-- 
regards,
Stan

[RFC PATCH v2 3/5] rtc: add qpnp rtc driver

[Stanimir Varbanov]

A 32bits RTC is housed inside PMIC. The RTC driver uses QPNP
SPMI interface to communicate with the PMIC RTC module.

The RTC device is divided into two sub-peripherals:
 - RTC read-write peripheral having basic RTC registers
 - alarm peripheral for controlling alarm

These two RTC peripherals are childrens of QPNP SPMI bus. They
use regmap to read/write to its registers into PMIC.

Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
---
 drivers/rtc/Kconfig    |    8 +
 drivers/rtc/Makefile   |    1 +
 drivers/rtc/rtc-qpnp.c |  489 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 498 insertions(+), 0 deletions(-)
 create mode 100644 drivers/rtc/rtc-qpnp.c

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 0754f5c..eb97b0a 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -1365,6 +1365,14 @@ config RTC_DRV_XGENE
 	  This driver can also be built as a module, if so, the module
 	  will be called "rtc-xgene".
 
+config RTC_DRV_QPNP
+	tristate "Qualcomm QPNP PMIC RTC"
+	depends on MFD_QPNP_SPMI
+	help
+	  Say Y here if you want to support the Qualcomm QPNP PMIC RTC.
+	  To compile this driver as a module, choose M here: the
+	  module will be called rtc-qpnp.
+
 comment "HID Sensor RTC drivers"
 
 config RTC_DRV_HID_SENSOR_TIME
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 70347d0..52488b5 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -141,3 +141,4 @@ obj-$(CONFIG_RTC_DRV_X1205)	+= rtc-x1205.o
 obj-$(CONFIG_RTC_DRV_XGENE)	+= rtc-xgene.o
 obj-$(CONFIG_RTC_DRV_SIRFSOC)	+= rtc-sirfsoc.o
 obj-$(CONFIG_RTC_DRV_MOXART)	+= rtc-moxart.o
+obj-$(CONFIG_RTC_DRV_QPNP)	+= rtc-qpnp.o
diff --git a/drivers/rtc/rtc-qpnp.c b/drivers/rtc/rtc-qpnp.c
new file mode 100644
index 0000000..ea26e62
--- /dev/null
+++ b/drivers/rtc/rtc-qpnp.c
@@ -0,0 +1,489 @@
+/* Copyright (c) 2012-2014, The Linux Foundation. All rights reserved.
+ *
+ * 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/module.h>
+#include <linux/rtc.h>
+#include <linux/spinlock.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+/* RTC/ALARM register offsets */
+#define REG_ALARM_RW			0x40
+#define REG_ALARM_CTRL1			0x46
+#define REG_ALARM_CTRL2			0x48
+#define REG_RTC_WRITE			0x40
+#define REG_RTC_CTRL			0x46
+#define REG_RTC_READ			0x48
+#define REG_PERP_SUBTYPE		0x05
+
+/* RTC_CTRL register bit fields */
+#define RTC_ENABLE			BIT(7)
+#define RTC_ALARM_ENABLE		BIT(7)
+#define RTC_ABORT_ENABLE		BIT(0)
+#define RTC_ALARM_CLEAR			BIT(0)
+
+/* RTC/ALARM peripheral subtype values */
+#define PERPH_SUBTYPE_RTC		0x1
+#define PERPH_SUBTYPE_ALARM		0x3
+#define NUM_8_BIT_RTC_REGS		0x4
+
+#define TO_SECS(arr)			\
+		(arr[0] | (arr[1] << 8) | (arr[2] << 16) | (arr[3] << 24))
+
+/* rtc driver internal structure */
+struct qpnp_rtc {
+	struct regmap *regmap;
+	struct device *dev;
+	struct rtc_device *rtc;
+	spinlock_t lock;	/* to protect RTC control register */
+	struct rtc_class_ops ops;
+	u8 rtc_ctrl_reg;
+	u8 alarm_ctrl_reg1;
+	u16 rtc_base;
+	u16 alarm_base;
+	int alarm_irq;
+};
+
+static int qpnp_rtc_read(struct qpnp_rtc *rtc, u8 *val, u16 off, int len)
+{
+	return regmap_bulk_read(rtc->regmap, rtc->rtc_base + off, val, len);
+}
+
+static int qpnp_rtc_write(struct qpnp_rtc *rtc, u8 *val, u16 off, int len)
+{
+	return regmap_bulk_write(rtc->regmap, rtc->rtc_base + off, val, len);
+}
+
+static int qpnp_alarm_read(struct qpnp_rtc *rtc, u8 *val, u16 off, int len)
+{
+	return regmap_bulk_read(rtc->regmap, rtc->alarm_base + off, val, len);
+}
+
+static int qpnp_alarm_write(struct qpnp_rtc *rtc, u8 *val, u16 off, int len)
+{
+	return regmap_bulk_write(rtc->regmap, rtc->alarm_base + off, val, len);
+}
+
+static int qpnp_rtc_set_time(struct device *dev, struct rtc_time *tm)
+{
+	struct qpnp_rtc *rtc = dev_get_drvdata(dev);
+	u8 value[4], reg = 0, alarm_enabled = 0, ctrl_reg;
+	u8 rtc_disabled = 0, rtc_ctrl_reg;
+	unsigned long secs, flags;
+	int ret;
+
+	rtc_tm_to_time(tm, &secs);
+
+	value[0] = (secs >>  0) & 0xff;
+	value[1] = (secs >>  8) & 0xff;
+	value[2] = (secs >> 16) & 0xff;
+	value[3] = (secs >> 24) & 0xff;
+
+	dev_dbg(dev, "Seconds value to be written to RTC = %lu\n", secs);
+
+	spin_lock_irqsave(&rtc->lock, flags);
+	ctrl_reg = rtc->alarm_ctrl_reg1;
+
+	if (ctrl_reg & RTC_ALARM_ENABLE) {
+		alarm_enabled = 1;
+		ctrl_reg &= ~RTC_ALARM_ENABLE;
+		ret = qpnp_alarm_write(rtc, &ctrl_reg, REG_ALARM_CTRL1, 1);
+		if (ret)
+			goto rtc_rw_fail;
+	} else {
+		spin_unlock_irqrestore(&rtc->lock, flags);
+	}
+
+	/*
+	 * 32 bit seconds value is coverted to four 8 bit values
+	 *	|<------  32 bit time value in seconds  ------>|
+	 *      <- 8 bit ->|<- 8 bit ->|<- 8 bit ->|<- 8 bit ->|
+	 *       ----------------------------------------------
+	 *      | BYTE[3]  |  BYTE[2]  |  BYTE[1]  |  BYTE[0]  |
+	 *       ----------------------------------------------
+	 *
+	 * RTC has four 8 bit registers for writting time in seconds:
+	 *             WDATA[3], WDATA[2], WDATA[1], WDATA[0]
+	 *
+	 * Write to the RTC registers should be done in following order
+	 * Clear WDATA[0] register
+	 *
+	 * Write BYTE[1], BYTE[2] and BYTE[3] of time to
+	 * RTC WDATA[3], WDATA[2], WDATA[1] registers
+	 *
+	 * Write BYTE[0] of time to RTC WDATA[0] register
+	 *
+	 * Clearing BYTE[0] and writting in the end will prevent any
+	 * unintentional overflow from WDATA[0] to higher bytes during the
+	 * write operation
+	 */
+
+	/* Disable RTC H/w before writing on RTC register*/
+	rtc_ctrl_reg = rtc->rtc_ctrl_reg;
+	if (rtc_ctrl_reg & RTC_ENABLE) {
+		rtc_disabled = 1;
+		rtc_ctrl_reg &= ~RTC_ENABLE;
+		ret = qpnp_rtc_write(rtc, &rtc_ctrl_reg, REG_RTC_CTRL, 1);
+		if (ret)
+			goto rtc_rw_fail;
+		rtc->rtc_ctrl_reg = rtc_ctrl_reg;
+	}
+
+	/* Clear WDATA[0] */
+	reg = 0x0;
+	ret = qpnp_rtc_write(rtc, &reg, REG_RTC_WRITE, 1);
+	if (ret)
+		goto rtc_rw_fail;
+
+	/* Write to WDATA[3], WDATA[2] and WDATA[1] */
+	ret = qpnp_rtc_write(rtc, &value[1], REG_RTC_WRITE + 1, 3);
+	if (ret)
+		goto rtc_rw_fail;
+
+	/* Write to WDATA[0] */
+	ret = qpnp_rtc_write(rtc, value, REG_RTC_WRITE, 1);
+	if (ret)
+		goto rtc_rw_fail;
+
+	/* Enable RTC after writing on RTC register */
+	if (rtc_disabled) {
+		rtc_ctrl_reg |= RTC_ENABLE;
+		ret = qpnp_rtc_write(rtc, &rtc_ctrl_reg, REG_RTC_CTRL, 1);
+		if (ret)
+			goto rtc_rw_fail;
+		rtc->rtc_ctrl_reg = rtc_ctrl_reg;
+	}
+
+	if (alarm_enabled) {
+		ctrl_reg |= RTC_ALARM_ENABLE;
+		ret = qpnp_alarm_write(rtc, &ctrl_reg, REG_ALARM_CTRL1, 1);
+		if (ret)
+			goto rtc_rw_fail;
+	}
+
+	rtc->alarm_ctrl_reg1 = ctrl_reg;
+
+rtc_rw_fail:
+	if (alarm_enabled)
+		spin_unlock_irqrestore(&rtc->lock, flags);
+
+	return ret;
+}
+
+static int qpnp_rtc_read_time(struct device *dev, struct rtc_time *tm)
+{
+	struct qpnp_rtc *rtc = dev_get_drvdata(dev);
+	u8 value[4], reg;
+	unsigned long secs;
+	int ret;
+
+	ret = qpnp_rtc_read(rtc, value, REG_RTC_READ, NUM_8_BIT_RTC_REGS);
+	if (ret)
+		return ret;
+
+	/*
+	 * Read the LSB again and check if there has been a carry over
+	 * If there is, redo the read operation
+	 */
+	ret = qpnp_rtc_read(rtc, &reg, REG_RTC_READ, 1);
+	if (ret)
+		return ret;
+
+	if (reg < value[0]) {
+		ret = qpnp_rtc_read(rtc, value, REG_RTC_READ,
+				    NUM_8_BIT_RTC_REGS);
+		if (ret)
+			return ret;
+	}
+
+	secs = TO_SECS(value);
+	rtc_time_to_tm(secs, tm);
+
+	ret = rtc_valid_tm(tm);
+	if (ret)
+		return ret;
+
+	dev_dbg(dev, "secs = %lu, h:m:s == %d:%d:%d, d/m/y = %d/%d/%d\n",
+		secs, tm->tm_hour, tm->tm_min, tm->tm_sec,
+		tm->tm_mday, tm->tm_mon, tm->tm_year);
+
+	return 0;
+}
+
+static int qpnp_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
+{
+	struct qpnp_rtc *rtc = dev_get_drvdata(dev);
+	unsigned long secs, secs_rtc, irq_flags;
+	struct rtc_time rtc_tm;
+	u8 value[4], ctrl_reg;
+	int ret;
+
+	rtc_tm_to_time(&alarm->time, &secs);
+
+	/*
+	 * Read the current RTC time and verify if the alarm time is in the
+	 * past. If yes, return invalid
+	 */
+	ret = qpnp_rtc_read_time(dev, &rtc_tm);
+	if (ret)
+		return -EINVAL;
+
+	rtc_tm_to_time(&rtc_tm, &secs_rtc);
+	if (secs < secs_rtc)
+		return -EINVAL;
+
+	value[0] = (secs >>  0) & 0xff;
+	value[1] = (secs >>  8) & 0xff;
+	value[2] = (secs >> 16) & 0xff;
+	value[3] = (secs >> 24) & 0xff;
+
+	spin_lock_irqsave(&rtc->lock, irq_flags);
+
+	ret = qpnp_alarm_write(rtc, value, REG_ALARM_RW, NUM_8_BIT_RTC_REGS);
+	if (ret)
+		goto rtc_rw_fail;
+
+	ctrl_reg = alarm->enabled ?
+			(rtc->alarm_ctrl_reg1 |  RTC_ALARM_ENABLE) :
+			(rtc->alarm_ctrl_reg1 & ~RTC_ALARM_ENABLE);
+
+	ret = qpnp_alarm_write(rtc, &ctrl_reg, REG_ALARM_CTRL1, 1);
+	if (ret)
+		goto rtc_rw_fail;
+
+	rtc->alarm_ctrl_reg1 = ctrl_reg;
+
+	dev_dbg(dev, "Alarm Set for h:r:s=%d:%d:%d, d/m/y=%d/%d/%d\n",
+			alarm->time.tm_hour, alarm->time.tm_min,
+			alarm->time.tm_sec, alarm->time.tm_mday,
+			alarm->time.tm_mon, alarm->time.tm_year);
+
+rtc_rw_fail:
+	spin_unlock_irqrestore(&rtc->lock, irq_flags);
+	return ret;
+}
+
+static int qpnp_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alarm)
+{
+	struct qpnp_rtc *rtc = dev_get_drvdata(dev);
+	unsigned long secs;
+	u8 value[4];
+	int ret;
+
+	ret = qpnp_alarm_read(rtc, value, REG_ALARM_RW, NUM_8_BIT_RTC_REGS);
+	if (ret)
+		return ret;
+
+	secs = TO_SECS(value);
+	rtc_time_to_tm(secs, &alarm->time);
+
+	ret = rtc_valid_tm(&alarm->time);
+	if (ret)
+		return ret;
+
+	dev_dbg(dev, "Alarm set for - h:r:s=%d:%d:%d, d/m/y=%d/%d/%d\n",
+		alarm->time.tm_hour, alarm->time.tm_min,
+				alarm->time.tm_sec, alarm->time.tm_mday,
+				alarm->time.tm_mon, alarm->time.tm_year);
+
+	return 0;
+}
+
+static int qpnp_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
+{
+	struct qpnp_rtc *rtc = dev_get_drvdata(dev);
+	unsigned long flags;
+	u8 ctrl_reg;
+	u8 value[4] = {0};
+	int ret;
+
+	spin_lock_irqsave(&rtc->lock, flags);
+
+	ctrl_reg = rtc->alarm_ctrl_reg1;
+	ctrl_reg = enabled ?
+		(ctrl_reg | RTC_ALARM_ENABLE) : (ctrl_reg & ~RTC_ALARM_ENABLE);
+
+	ret = qpnp_alarm_write(rtc, &ctrl_reg, REG_ALARM_CTRL1, 1);
+	if (ret)
+		goto rtc_rw_fail;
+
+	rtc->alarm_ctrl_reg1 = ctrl_reg;
+
+	/* Clear Alarm register */
+	if (!enabled)
+		ret = qpnp_alarm_write(rtc, value, REG_ALARM_RW,
+				       NUM_8_BIT_RTC_REGS);
+
+rtc_rw_fail:
+	spin_unlock_irqrestore(&rtc->lock, flags);
+
+	return ret;
+}
+
+static irqreturn_t qpnp_alarm_trigger(int irq, void *dev_id)
+{
+	struct qpnp_rtc *rtc = dev_id;
+	unsigned long flags;
+	u8 ctrl_reg;
+	int ret;
+
+	rtc_update_irq(rtc->rtc, 1, RTC_IRQF | RTC_AF);
+
+	spin_lock_irqsave(&rtc->lock, flags);
+
+	/* Clear the alarm enable bit */
+	ctrl_reg = rtc->alarm_ctrl_reg1;
+	ctrl_reg &= ~RTC_ALARM_ENABLE;
+
+	ret = qpnp_alarm_write(rtc, &ctrl_reg, REG_ALARM_CTRL1, 1);
+	if (ret) {
+		spin_unlock_irqrestore(&rtc->lock, flags);
+		return IRQ_HANDLED;
+	}
+
+	rtc->alarm_ctrl_reg1 = ctrl_reg;
+	spin_unlock_irqrestore(&rtc->lock, flags);
+
+	/* Set ALARM_CLR bit */
+	ctrl_reg = 0x1;
+	ret = qpnp_alarm_write(rtc, &ctrl_reg, REG_ALARM_CTRL2, 1);
+	if (ret)
+		dev_dbg(rtc->dev, "Write to ALARM control reg failed\n");
+
+	return IRQ_HANDLED;
+}
+
+static int qpnp_rtc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct qpnp_rtc *rtc;
+	struct resource *res;
+	u8 subtype;
+	int ret;
+
+	rtc = devm_kzalloc(dev, sizeof(*rtc), GFP_KERNEL);
+	if (!rtc)
+		return -ENOMEM;
+
+	spin_lock_init(&rtc->lock);
+
+	rtc->regmap = dev_get_regmap(dev->parent, NULL);
+	if (!rtc->regmap)
+		return -ENODEV;
+
+	rtc->dev = dev;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_REG, "rtc");
+	if (!res)
+		return -ENODEV;
+
+	rtc->rtc_base = res->start;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_REG, "alarm");
+	if (!res)
+		return -ENODEV;
+
+	rtc->alarm_base = res->start;
+
+	ret = qpnp_rtc_read(rtc, &subtype, REG_PERP_SUBTYPE, 1);
+	if (ret)
+		return ret;
+
+	if (subtype != PERPH_SUBTYPE_RTC)
+		return -ENODEV;
+
+	ret = qpnp_alarm_read(rtc, &subtype, REG_PERP_SUBTYPE, 1);
+	if (ret)
+		return ret;
+
+	if (subtype != PERPH_SUBTYPE_ALARM)
+		return -ENODEV;
+
+	rtc->alarm_irq = platform_get_irq(pdev, 0);
+	if (rtc->alarm_irq < 0)
+		return rtc->alarm_irq;
+
+	ret = qpnp_rtc_read(rtc, &rtc->rtc_ctrl_reg, REG_RTC_CTRL, 1);
+	if (ret)
+		return ret;
+
+	if (!(rtc->rtc_ctrl_reg & RTC_ENABLE)) {
+		dev_dbg(dev, "RTC h/w disabled, rtc not registered\n");
+		return -ENODEV;
+	}
+
+	ret = qpnp_alarm_read(rtc, &rtc->alarm_ctrl_reg1, REG_ALARM_CTRL1, 1);
+	if (ret)
+		return ret;
+
+	rtc->alarm_ctrl_reg1 |= RTC_ABORT_ENABLE;
+	ret = qpnp_alarm_write(rtc, &rtc->alarm_ctrl_reg1, REG_ALARM_CTRL1, 1);
+	if (ret)
+		return ret;
+
+	ret = devm_request_any_context_irq(dev, rtc->alarm_irq,
+					   qpnp_alarm_trigger,
+					   IRQF_TRIGGER_RISING,
+					   "qpnp_rtc_alarm", rtc);
+	if (ret < 0)
+		return ret;
+
+	device_init_wakeup(dev, 1);
+	enable_irq_wake(rtc->alarm_irq);
+
+	platform_set_drvdata(pdev, rtc);
+
+	rtc->ops.read_time = qpnp_rtc_read_time;
+	rtc->ops.set_alarm = qpnp_rtc_set_alarm;
+	rtc->ops.read_alarm = qpnp_rtc_read_alarm;
+	rtc->ops.alarm_irq_enable = qpnp_rtc_alarm_irq_enable;
+	rtc->ops.set_time = qpnp_rtc_set_time;
+
+	rtc->rtc = rtc_device_register("qpnp-rtc", dev, &rtc->ops, THIS_MODULE);
+	if (IS_ERR(rtc->rtc))
+		return PTR_ERR(rtc->rtc);
+
+	return 0;
+}
+
+static int qpnp_rtc_remove(struct platform_device *pdev)
+{
+	struct qpnp_rtc *rtc = platform_get_drvdata(pdev);
+
+	device_init_wakeup(rtc->dev, 0);
+	free_irq(rtc->alarm_irq, rtc);
+	rtc_device_unregister(rtc->rtc);
+
+	return 0;
+}
+
+static const struct of_device_id qpnp_rtc_table[] = {
+	{ .compatible = "qcom,qpnp-rtc", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, rtc_qpnp_table);
+
+static struct platform_driver qpnp_rtc_driver = {
+	.probe = qpnp_rtc_probe,
+	.remove = qpnp_rtc_remove,
+	.driver = {
+		.name = "qpnp-rtc",
+		.owner = THIS_MODULE,
+		.of_match_table = qpnp_rtc_table,
+	},
+};
+module_platform_driver(qpnp_rtc_driver);
+
+MODULE_ALIAS("platform:" KBUILD_MODNAME);
+MODULE_DESCRIPTION("SMPI PMIC RTC driver");
+MODULE_AUTHOR("The Linux Foundation");
+MODULE_LICENSE("GPL v2");
-- 
1.7.0.4

Re: [RFC PATCH v2 3/5] rtc: add qpnp rtc driver

[Stephen Boyd]

On 07/03/14 06:14, Stanimir Varbanov wrote:
> A 32bits RTC is housed inside PMIC. The RTC driver uses QPNP
> SPMI interface to communicate with the PMIC RTC module.
>
> The RTC device is divided into two sub-peripherals:
>  - RTC read-write peripheral having basic RTC registers
>  - alarm peripheral for controlling alarm
>
> These two RTC peripherals are childrens of QPNP SPMI bus. They
> use regmap to read/write to its registers into PMIC.
>

What happened to using Josh's patch for rtc-pm8xxx.c[1]? That seems
easier than adding an entire new driver for almost the same hardware.

[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-October/207665.html

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [RFC PATCH v2 3/5] rtc: add qpnp rtc driver

[Stanimir Varbanov]

Hi Stephen,

On 07/09/2014 09:07 PM, Stephen Boyd wrote:
> On 07/03/14 06:14, Stanimir Varbanov wrote:
>> A 32bits RTC is housed inside PMIC. The RTC driver uses QPNP
>> SPMI interface to communicate with the PMIC RTC module.
>>
>> The RTC device is divided into two sub-peripherals:
>>  - RTC read-write peripheral having basic RTC registers
>>  - alarm peripheral for controlling alarm
>>
>> These two RTC peripherals are childrens of QPNP SPMI bus. They
>> use regmap to read/write to its registers into PMIC.
>>
> 
> What happened to using Josh's patch for rtc-pm8xxx.c[1]? That seems
> easier than adding an entire new driver for almost the same hardware.
> 

Ups, I completely forgot about Josh's patch, thanks for the reminder.
Nevermind, the intention of the patch set was to get agreement on the
approach of the qpnp-spmi driver. The rtc is just slightly reworked to
platform driver downstream rtc driver. And last but not least I've used
rtc to test the approach and as a example of PMIC function/peripheral
driver.

I will see what efforts are needed to reuse rtc-pm8xxx.

> [1]
> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-October/207665.html
> 

-- 
regards,
Stan

Re: [RFC PATCH v2 3/5] rtc: add qpnp rtc driver

[Bjorn Andersson]

On Thu, Jul 3, 2014 at 6:14 AM, Stanimir Varbanov <svarbanov@mm-sol.com> wrote:
[...]
> +static const struct of_device_id qpnp_rtc_table[] = {
> +       { .compatible = "qcom,qpnp-rtc", },
> +       {}
> +};
> +MODULE_DEVICE_TABLE(of, rtc_qpnp_table);

We have had a lot of discussions related to how to name pm8x41
drivers; as they obviously fall under pm8xxx in most peoples eyes.

As you guys have explained, QPNP is defining the split of address
space and how interrupts are layed out. To me this does however not
say anything related to the actual functionality; e.g. the rtc in this
case.

Are you by this patch saying that this is the one and only rtc
hardware that will ever be spun under the QPNP umbrella?
I would expect the naming to be more specific; and definitely the
compatible to be specific.

This concern goes for all the qpnp drivers.

Regards,
Bjorn

Re: [RFC PATCH v2 3/5] rtc: add qpnp rtc driver

[Stanimir Varbanov]

On 07/10/2014 04:08 PM, Bjorn Andersson wrote:
> On Thu, Jul 3, 2014 at 6:14 AM, Stanimir Varbanov <svarbanov@mm-sol.com> wrote:
> [...]
>> +static const struct of_device_id qpnp_rtc_table[] = {
>> +       { .compatible = "qcom,qpnp-rtc", },
>> +       {}
>> +};
>> +MODULE_DEVICE_TABLE(of, rtc_qpnp_table);
> 
> We have had a lot of discussions related to how to name pm8x41
> drivers; as they obviously fall under pm8xxx in most peoples eyes.

IMO the pm8xxx is misleading already.

> 
> As you guys have explained, QPNP is defining the split of address
> space and how interrupts are layed out. To me this does however not
> say anything related to the actual functionality; e.g. the rtc in this
> case.
> 
> Are you by this patch saying that this is the one and only rtc
> hardware that will ever be spun under the QPNP umbrella?
> I would expect the naming to be more specific; and definitely the
> compatible to be specific.
> 
> This concern goes for all the qpnp drivers.

QPNP "umbrella" includes 11 PMIC chips according to downstream kernel at
[1]. I think that every driver with qpnp in the name will support the
appropriate sub-function IP on every pmic of that list.

Of course the naming convention can be changed and for rtc we could say
rtc-pm8941 in compatible string.

compatible = "qcom,pm8941-rtc", "qcom,pma8084-rtc", "qcom,qpnp-rtc";

I haven't strong opinion on the naming.

Otherwise, I will do evaluate how difficult will be to merge "rtc-qpnp"
and rtc-pm8xxx. Those rtc peripherals looks similar.

-- 
regards,
Stan

[1]
https://www.codeaurora.org/cgit/quic/la/kernel/msm-3.10/tree/drivers/platform/msm/qpnp-revid.c?h=msm-3.10#n27

Re: [RFC PATCH v2 3/5] rtc: add qpnp rtc driver

[Stanimir Varbanov]

Hi Bjorn,

On 07/10/2014 06:43 PM, Stanimir Varbanov wrote:
> On 07/10/2014 04:08 PM, Bjorn Andersson wrote:
>> On Thu, Jul 3, 2014 at 6:14 AM, Stanimir Varbanov <svarbanov@mm-sol.com> wrote:
>> [...]
>>> +static const struct of_device_id qpnp_rtc_table[] = {
>>> +       { .compatible = "qcom,qpnp-rtc", },
>>> +       {}
>>> +};
>>> +MODULE_DEVICE_TABLE(of, rtc_qpnp_table);
>>
>> We have had a lot of discussions related to how to name pm8x41
>> drivers; as they obviously fall under pm8xxx in most peoples eyes.
> 
> IMO the pm8xxx is misleading already.

I guess we can rename pm8921-core.c to pm8921-ssbi.c. And then "QPNP"
variant can become pm8xxx-spmi.c. This way the name reflects the
physical interface used for particular pmic. We need to drop out the
"xx41" because this driver will support PMA8084 as well.

Then we can try to extend the pm8xxx subsystem drivers (pmic8xxx-keypad,
rtc-pm8xxx, pm8xxx-vibrator, pmic8xxx-pwrkey) to support pm8xxx-spmi
(qpnp) depending on compatible string.

-- 
regards,
Stan

[RFC PATCH v2 4/5] dt: msm8974: add qpnp rtc device node

[Stanimir Varbanov]

Add QPNP RTC device tree node. The RTC device resides into pm8941
and is attached on usid0.

Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
---
 arch/arm/boot/dts/qcom-msm8974.dtsi |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/qcom-msm8974.dtsi b/arch/arm/boot/dts/qcom-msm8974.dtsi
index 9166b8e..f6fe27b 100644
--- a/arch/arm/boot/dts/qcom-msm8974.dtsi
+++ b/arch/arm/boot/dts/qcom-msm8974.dtsi
@@ -3,6 +3,7 @@
 #include "skeleton.dtsi"
 
 #include <dt-bindings/clock/qcom,gcc-msm8974.h>
+#include <dt-bindings/interrupt-controller/irq.h>
 #include <dt-bindings/spmi/spmi.h>
 
 / {
@@ -258,6 +259,13 @@
 				reg = <0x0 SPMI_USID>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+
+				pm8941_rtc@6000 {
+					compatible = "qcom,qpnp-rtc";
+					reg = <0x6000 0x6100>;
+					reg-names = "rtc", "alarm";
+					interrupts = <0x0 0x61 0x1 IRQ_TYPE_EDGE_RISING>;
+				};
 			};
 
 			usid1: pm8941@1 {
-- 
1.7.0.4

[RFC PATCH v2 5/5] dt: rtc: add binding document for qpnp rtc

[Stanimir Varbanov]

Add devicetree binding document which describes the qpnp-rtc.

Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
---
 .../devicetree/bindings/rtc/qcom,rtc-qpnp.txt      |   24 ++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/rtc/qcom,rtc-qpnp.txt

diff --git a/Documentation/devicetree/bindings/rtc/qcom,rtc-qpnp.txt b/Documentation/devicetree/bindings/rtc/qcom,rtc-qpnp.txt
new file mode 100644
index 0000000..a64e194
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/qcom,rtc-qpnp.txt
@@ -0,0 +1,24 @@
+Qualcomm RTC QPNP
+
+The RTC device supports 32bit RTC module housed inside QPNP PMIC's.
+The driver utilizes SPMI interface to communicate with the RTC module.
+RTC device is divided into two sub-peripherals one which controls
+basic RTC and other for controlling alarm.
+
+Required properties :
+ - compatible:  Should be "qcom,qpnp-rtc".
+ - reg:         Specify the peripheral id for device. Currently there
+                are two sub-peripherals - rtc base and alarm base.
+ - reg-names:   Specify the reg property as strings. Should be "rtc"
+                and "alarm", defining the rtc and alarm bases.
+ - interrupts:  Specifies alarm interrupt, only for alarm sub-peripheral.
+                For more info about interrupt cells see
+                Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt
+
+Example:
+	rtc@6000 {
+		compatible = "qcom,qpnp-rtc";
+		reg = <0x6000 0x6100>;
+		reg-names = "rtc", "alarm";
+		interrupts = <0x0 0x61 0x1 IRQ_TYPE_EDGE_RISING>;
+	};
-- 
1.7.0.4