[PATCH 0/1] Compact interface for Device-Tree

[PATCH 0/1] Compact interface for Device-Tree

[Gilad Avidov]

Device-Tree compact API
------------------------

Common code seen in driver’s probe reads device tree values and handling
erroneous return codes from all those of_property_read_xxx()  APIs. This
common code is factored out by the of_property_map module which allows
driver’s probe to replace that (often lengthy) code with a concise table:

struct of_prop_map map[] = {
    {"i2c",            &dev->id,        OF_REQ,  OF_ID,  -1},
    {"qcom,clk-freq-out",    &dev->clk_freq_out,    OF_REQ,  OF_U32,  0},
    {"qcom,clk-freq-in",    &dev->clk_freq_in,    OF_REQ,  OF_U32,  0},
    {"qcom,disable-dma",    &dev->disable_dma,    OF_OPT,  OF_BOOL, 0},
    {"qcom,master-id",    &dev->mstr_id,        OF_SGST, OF_U32,  0},
    {NULL,            NULL,            0,       0,       0},
};

Then call populate to read the values into the device’s variables:

ret = of_prop_populate(dev, dev->of_node, map);


An equivalent code snippet using the traditional of_property_read_XXXX()
API. Note that the equivalent is longer and more difficult to follow and
debug:

/* optional property */
dev->disable_dma = of_property_read_bool(node, "qcom,disable-dma");

ret = of_property_read_u32(dev->node, "qcom,clk-freq-out", &dev->clk_freq_out);
if (ret) {
    dev_err(dev, "error: missing 'qcom,clk-freq-out' DT property\n");
    if (!err)
        err = ret;
}

ret = of_property_read_u32(dev->node, "qcom,clk-freq-in", &dev->clk_freq_in);
if (ret) {
    dev_err(dev, "error: missing 'qcom,clk-freq-in' DT property\n");
    if (!err)
        err = ret;
}

/* suggested property */
ret = of_property_read_u32(dev->node, "qcom,master-id", &dev->mstr_id);
if (ret && !err)
    err = ret;


ret = of_alias_get_id(dev->node, "i2c");
if (ret < 0) {
    dev_err(dev, "error: missing '"i2c"' DT property\n");
    if (!err)
        err = ret;
} else {
    dev->id = ret;
}


The Device-Tree node and alias which are read by the above code snippets:

aliases {
    i2c0 = &i2c_0; /* I2C0 controller device */
};

i2c_0: i2c@78b6000 { /* BLSP1 QUP2 */
    compatible = "qcom,i2c-msm-v2";
    reg-names = "qup_phys_addr", "bam_phys_addr";
    reg = <0x78b6000 0x600>,
          <0x7884000 0x23000>;
    qcom,clk-freq-out = <100000>;
    qcom,clk-freq-in  = <19200000>;
    qcom,disable-dma;
    qcom,master-id = <86>;
}; 


Gilad Avidov (1):
  of_propery_map: compact interface for Device-Tree

 Documentation/devicetree/of_property_map.txt |   76 ++++++++++++++++++++++
 drivers/of/Makefile                          |    2 +-
 drivers/of/of_property_map.c                 |   88 ++++++++++++++++++++++++++
 include/linux/of_property_map.h              |   74 ++++++++++++++++++++++
 4 files changed, 239 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/of_property_map.txt
 create mode 100644 drivers/of/of_property_map.c
 create mode 100644 include/linux/of_property_map.h

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

[PATCH 1/1] of_propery_map: compact interface for Device-Tree

[Gilad Avidov]

Compact API for populating variables with values
from Device-Tree properties.

Change-Id: Ib0f17d32941605b0c431a1815cfcf5e8b76fb07c
Signed-off-by: Gilad Avidov <gavidov@codeaurora.org>
---
 Documentation/devicetree/of_property_map.txt |   76 ++++++++++++++++++++++
 drivers/of/Makefile                          |    2 +-
 drivers/of/of_property_map.c                 |   88 ++++++++++++++++++++++++++
 include/linux/of_property_map.h              |   74 ++++++++++++++++++++++
 4 files changed, 239 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/of_property_map.txt
 create mode 100644 drivers/of/of_property_map.c
 create mode 100644 include/linux/of_property_map.h

diff --git a/Documentation/devicetree/of_property_map.txt b/Documentation/devicetree/of_property_map.txt
new file mode 100644
index 0000000..307bf57
--- /dev/null
+++ b/Documentation/devicetree/of_property_map.txt
@@ -0,0 +1,76 @@
+* of_property_map:
+
+of_property_map is a compact API for reading Device-Tree nodes.
+The following snippet demonstrates reading the aliased-ID and four other
+properties of the Device-Tree node defined at the bottom.
+
+Example:
+
+struct of_prop_map map[] = {
+	{"i2c",			&dev->id,		OF_REQ,  OF_ID,  -1},
+	{"qcom,clk-freq-out",	&dev->clk_freq_out,	OF_REQ,  OF_U32,  0},
+	{"qcom,clk-freq-in",	&dev->clk_freq_in,	OF_REQ,  OF_U32,  0},
+	{"qcom,disable-dma",	&dev->disable_dma,	OF_OPT,  OF_BOOL, 0},
+	{"qcom,master-id",	&dev->mstr_id,		OF_SGST, OF_U32,  0},
+	{NULL,			NULL,			0,       0,       0},
+};
+
+ret = of_prop_populate(dev, dev->of_node, map);
+
+
+An equivalent code snippet using the traditional of_property_read_XXXX() API.
+Note that the equivalent is longer and more difficult to follow and debug.
+
+Example:
+
+/* optional property */
+dev->disable_dma = of_property_read_bool(node, "qcom,disable-dma");
+
+ret = of_property_read_u32(dev->node, "qcom,clk-freq-out", &dev->clk_freq_out);
+if (ret) {
+	dev_err(dev, "error: missing 'qcom,clk-freq-out' DT property\n");
+	if (!err)
+		err = ret;
+}
+
+ret = of_property_read_u32(dev->node, "qcom,clk-freq-in", &dev->clk_freq_in);
+if (ret) {
+	dev_err(dev, "error: missing 'qcom,clk-freq-in' DT property\n");
+	if (!err)
+		err = ret;
+}
+
+/* suggested property */
+ret = of_property_read_u32(dev->node, "qcom,master-id", &dev->mstr_id);
+if (ret && !err)
+	err = ret;
+
+
+ret = of_alias_get_id(dev->node, "i2c");
+if (ret < 0) {
+	dev_err(dev, "error: missing '"i2c"' DT property\n");
+	if (!err)
+		err = ret;
+} else {
+	dev->id = ret;
+}
+
+
+The Device-Tree node and alias which are read by the above code snippets.
+
+Example:
+
+	aliases {
+		i2c0 = &i2c_0; /* I2C0 controller device */
+	};
+
+	i2c_0: i2c@78b6000 { /* BLSP1 QUP2 */
+		compatible = "qcom,i2c-msm-v2";
+		reg-names = "qup_phys_addr", "bam_phys_addr";
+		reg = <0x78b6000 0x600>,
+		      <0x7884000 0x23000>;
+		qcom,clk-freq-out = <100000>;
+		qcom,clk-freq-in  = <19200000>;
+		qcom,disable-dma;
+		qcom,master-id = <86>;
+	};
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index ca9209c..c60eeb0 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -1,4 +1,4 @@
-obj-y = base.o device.o platform.o
+obj-y = base.o device.o platform.o of_property_map.o
 obj-$(CONFIG_OF_DYNAMIC) += dynamic.o
 obj-$(CONFIG_OF_FLATTREE) += fdt.o
 obj-$(CONFIG_OF_EARLY_FLATTREE) += fdt_address.o
diff --git a/drivers/of/of_property_map.c b/drivers/of/of_property_map.c
new file mode 100644
index 0000000..8145f1c
--- /dev/null
+++ b/drivers/of/of_property_map.c
@@ -0,0 +1,88 @@
+/* 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/of.h>
+#include <linux/device.h>
+#include <linux/of_property_map.h>
+
+int of_prop_populate(struct device *dev, struct device_node *np,
+						struct of_prop_map *itr)
+{
+	int  ret, err = 0;
+
+	for (; itr->propname; ++itr) {
+		switch (itr->type) {
+		case OF_BOOL:
+			*((bool *) itr->var_ptr) =
+				of_property_read_bool(np, itr->propname);
+			ret = 0;
+			break;
+		case OF_U8:
+			ret = of_property_read_u8(np, itr->propname,
+							(u8 *) itr->var_ptr);
+			break;
+		case OF_U16:
+			ret = of_property_read_u16(np, itr->propname,
+							(u16 *) itr->var_ptr);
+			break;
+		case OF_U32:
+			ret = of_property_read_u32(np, itr->propname,
+							(u32 *) itr->var_ptr);
+			break;
+		case OF_U64:
+			ret = of_property_read_u64(np, itr->propname,
+							(u64 *) itr->var_ptr);
+			break;
+		case OF_STR:
+			ret = of_property_read_string(np, itr->propname,
+								itr->var_ptr);
+			break;
+		case OF_ID:
+			ret = of_alias_get_id(np, itr->propname);
+			if (ret >= 0) {
+				*((int *) itr->var_ptr) = ret;
+				ret = 0;
+			}
+			break;
+		default:
+			dev_err(dev, "error: %d is unknown DT property type\n",
+								itr->type);
+			ret = -EINVAL;
+		}
+
+		dev_dbg(dev, "of_property: name:%s val:%d ret:%d\n",
+				itr->propname, *((int *)itr->var_ptr), ret);
+
+		if (ret) {
+			/* on error set value to default */
+			*((long long int *)itr->var_ptr) = itr->default_val;
+
+			if (itr->criticality < OF_OPT) {
+				dev_err(dev,
+					"error: missing '%s' DT property\n",
+					itr->propname);
+
+				/*
+				 * Do not stop on errors. Keep running to
+				 * dump messages for all missing properties.
+				 * On error return the first one found.
+				 */
+				if (itr->criticality == OF_REQ && !err)
+					err = ret;
+			}
+		}
+	}
+
+	return err;
+}
+EXPORT_SYMBOL(of_prop_populate);
diff --git a/include/linux/of_property_map.h b/include/linux/of_property_map.h
new file mode 100644
index 0000000..0c61673
--- /dev/null
+++ b/include/linux/of_property_map.h
@@ -0,0 +1,74 @@
+/* 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.
+ *
+ */
+/*
+ * Higher level and compact interface for reading Device-Tree nodes. Uses
+ * a direct visual mapping table of the node's property name to the
+ * driver's/module's fields.
+ *
+ * Usage exapmle:
+ * struct of_prop_map map[] = {
+ *	{"i2c",			&dev->id,		OF_REQ,  OF_ID,  -1},
+ *	{"qcom,clk-freq-out",	&dev->clk_freq_out,	OF_REQ,  OF_U32,  0},
+ *	{"qcom,clk-freq-in",	&dev->clk_freq_in,	OF_REQ,  OF_U32,  0},
+ *	{"qcom,bam-disable",	&dev->disable_dma,	OF_OPT,  OF_BOOL, 0},
+ *	{"qcom,master-id",	&dev->mstr_id,		OF_SGST, OF_U32,  0},
+ *	{NULL,			NULL,			0,       0,       0},
+ * };
+ *
+ * ret = of_prop_populate(dev, dev->of_node, map);
+ */
+
+#ifndef _OF_PROPERTY_MAP_H
+#define _OF_PROPERTY_MAP_H
+
+enum of_prop_criticality {
+	OF_REQ,		/* Required : fail if missing       */
+	OF_SGST,	/* Suggested: warn if missing       */
+	OF_OPT,		/* Optional : don't warn if missing */
+};
+
+enum of_prop_type {
+	OF_BOOL,	/* of_property_read_bool()	*/
+	OF_U8,		/* of_property_read_u8()	*/
+	OF_U16,		/* of_property_read_u16()	*/
+	OF_U32,		/* of_property_read_u32()	*/
+	OF_U64,		/* of_property_read_u64()	*/
+	OF_STR,		/* of_property_read_string()	*/
+	OF_ID,		/* of_alias_get_id()		*/
+};
+
+/*
+ * of_prop_map: mapping Device-Tree property name to the dev's variable
+ *
+ * @propname property name in Device-Tree
+ * @var_ptr pointer to the device's variable to store the value in.
+ * @default_val if propname is not found then *var_ptr = default_val
+ */
+struct of_prop_map {
+	const char		*propname;
+	void			*var_ptr;
+	enum of_prop_criticality criticality;
+	enum of_prop_type	 type;
+	long long int		 default_val;
+};
+
+/*
+ * of_prop_populate: read Device-Tree properites and populate the mapped fields
+ *
+ * @node the Device-Tree node corresponding to dev
+ * @itr pointer to a NULL terminated property mapping
+ */
+int of_prop_populate(struct device *dev, struct device_node *node,
+						struct of_prop_map *itr);
+
+#endif /*_OF_PROPERTY_MAP_H*/
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH 0/1] Compact interface for Device-Tree

[Rob Herring]

On Fri, Oct 31, 2014 at 6:59 AM, Gilad Avidov <gavidov@codeaurora.org> wrote:
>
> Device-Tree compact API
> ------------------------
>
> Common code seen in driver’s probe reads device tree values and handling
> erroneous return codes from all those of_property_read_xxx()  APIs. This
> common code is factored out by the of_property_map module which allows
> driver’s probe to replace that (often lengthy) code with a concise table:
>
> struct of_prop_map map[] = {
>     {"i2c",            &dev->id,        OF_REQ,  OF_ID,  -1},
>     {"qcom,clk-freq-out",    &dev->clk_freq_out,    OF_REQ,  OF_U32,  0},
>     {"qcom,clk-freq-in",    &dev->clk_freq_in,    OF_REQ,  OF_U32,  0},
>     {"qcom,disable-dma",    &dev->disable_dma,    OF_OPT,  OF_BOOL, 0},
>     {"qcom,master-id",    &dev->mstr_id,        OF_SGST, OF_U32,  0},
>     {NULL,            NULL,            0,       0,       0},
> };
>
> Then call populate to read the values into the device’s variables:
>
> ret = of_prop_populate(dev, dev->of_node, map);

Interesting idea. The main concern I have with this is there has been
on-going discussions about how to generalize property handling across
DT and ACPI to make drivers more agnostic, so I'm copying a few folks
involved in that. That may be a bit orthogonal to what this is doing,
but we may want some coordination here.

BTW, there is little point to a 0/1 email. All this info should be in
the patch either in commit msg or documentation as part of the patch.

Rob

> An equivalent code snippet using the traditional of_property_read_XXXX()
> API. Note that the equivalent is longer and more difficult to follow and
> debug:
>
> /* optional property */
> dev->disable_dma = of_property_read_bool(node, "qcom,disable-dma");
>
> ret = of_property_read_u32(dev->node, "qcom,clk-freq-out", &dev->clk_freq_out);
> if (ret) {
>     dev_err(dev, "error: missing 'qcom,clk-freq-out' DT property\n");
>     if (!err)
>         err = ret;
> }
>
> ret = of_property_read_u32(dev->node, "qcom,clk-freq-in", &dev->clk_freq_in);
> if (ret) {
>     dev_err(dev, "error: missing 'qcom,clk-freq-in' DT property\n");
>     if (!err)
>         err = ret;
> }
>
> /* suggested property */
> ret = of_property_read_u32(dev->node, "qcom,master-id", &dev->mstr_id);
> if (ret && !err)
>     err = ret;
>
>
> ret = of_alias_get_id(dev->node, "i2c");
> if (ret < 0) {
>     dev_err(dev, "error: missing '"i2c"' DT property\n");
>     if (!err)
>         err = ret;
> } else {
>     dev->id = ret;
> }
>
>
> The Device-Tree node and alias which are read by the above code snippets:
>
> aliases {
>     i2c0 = &i2c_0; /* I2C0 controller device */
> };
>
> i2c_0: i2c@78b6000 { /* BLSP1 QUP2 */
>     compatible = "qcom,i2c-msm-v2";
>     reg-names = "qup_phys_addr", "bam_phys_addr";
>     reg = <0x78b6000 0x600>,
>           <0x7884000 0x23000>;
>     qcom,clk-freq-out = <100000>;
>     qcom,clk-freq-in  = <19200000>;
>     qcom,disable-dma;
>     qcom,master-id = <86>;
> };
>
>
> Gilad Avidov (1):
>   of_propery_map: compact interface for Device-Tree
>
>  Documentation/devicetree/of_property_map.txt |   76 ++++++++++++++++++++++
>  drivers/of/Makefile                          |    2 +-
>  drivers/of/of_property_map.c                 |   88 ++++++++++++++++++++++++++
>  include/linux/of_property_map.h              |   74 ++++++++++++++++++++++
>  4 files changed, 239 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/of_property_map.txt
>  create mode 100644 drivers/of/of_property_map.c
>  create mode 100644 include/linux/of_property_map.h
>
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> hosted by The Linux Foundation
>

Re: [PATCH 0/1] Compact interface for Device-Tree

[Rafael J. Wysocki]

On Saturday, November 01, 2014 05:13:45 AM Rob Herring wrote:
> On Fri, Oct 31, 2014 at 6:59 AM, Gilad Avidov <gavidov@codeaurora.org> wrote:
> >
> > Device-Tree compact API
> > ------------------------
> >
> > Common code seen in driver’s probe reads device tree values and handling
> > erroneous return codes from all those of_property_read_xxx()  APIs. This
> > common code is factored out by the of_property_map module which allows
> > driver’s probe to replace that (often lengthy) code with a concise table:
> >
> > struct of_prop_map map[] = {
> >     {"i2c",            &dev->id,        OF_REQ,  OF_ID,  -1},
> >     {"qcom,clk-freq-out",    &dev->clk_freq_out,    OF_REQ,  OF_U32,  0},
> >     {"qcom,clk-freq-in",    &dev->clk_freq_in,    OF_REQ,  OF_U32,  0},
> >     {"qcom,disable-dma",    &dev->disable_dma,    OF_OPT,  OF_BOOL, 0},
> >     {"qcom,master-id",    &dev->mstr_id,        OF_SGST, OF_U32,  0},
> >     {NULL,            NULL,            0,       0,       0},
> > };
> >
> > Then call populate to read the values into the device’s variables:
> >
> > ret = of_prop_populate(dev, dev->of_node, map);
> 
> Interesting idea. The main concern I have with this is there has been
> on-going discussions about how to generalize property handling across
> DT and ACPI to make drivers more agnostic, so I'm copying a few folks
> involved in that. That may be a bit orthogonal to what this is doing,
> but we may want some coordination here.

Agreed.

We actually have a patchset adding a unified device property API in linux-next (http://git.kernel.org/cgit/linux/kernel/git/rafael/linux-pm.git/log/?h=device-properties)
and I'd prefer to see the "compactization" to happen at that level, if possible,
rather that for of_ only.

Rafael

Re: [PATCH 0/1] Compact interface for Device-Tree

[Arnd Bergmann]

On Friday 31 October 2014 23:53:28 Rafael J. Wysocki wrote:
> On Saturday, November 01, 2014 05:13:45 AM Rob Herring wrote:
> > On Fri, Oct 31, 2014 at 6:59 AM, Gilad Avidov <gavidov@codeaurora.org> wrote:
> > >
> > > Device-Tree compact API
> > > ------------------------
> > >
> > > Common code seen in driver’s probe reads device tree values and handling
> > > erroneous return codes from all those of_property_read_xxx()  APIs. This
> > > common code is factored out by the of_property_map module which allows
> > > driver’s probe to replace that (often lengthy) code with a concise table:
> > >
> > > struct of_prop_map map[] = {
> > >     {"i2c",            &dev->id,        OF_REQ,  OF_ID,  -1},
> > >     {"qcom,clk-freq-out",    &dev->clk_freq_out,    OF_REQ,  OF_U32,  0},
> > >     {"qcom,clk-freq-in",    &dev->clk_freq_in,    OF_REQ,  OF_U32,  0},
> > >     {"qcom,disable-dma",    &dev->disable_dma,    OF_OPT,  OF_BOOL, 0},
> > >     {"qcom,master-id",    &dev->mstr_id,        OF_SGST, OF_U32,  0},
> > >     {NULL,            NULL,            0,       0,       0},
> > > };
> > >
> > > Then call populate to read the values into the device’s variables:
> > >
> > > ret = of_prop_populate(dev, dev->of_node, map);
> > 
> > Interesting idea. The main concern I have with this is there has been
> > on-going discussions about how to generalize property handling across
> > DT and ACPI to make drivers more agnostic, so I'm copying a few folks
> > involved in that. That may be a bit orthogonal to what this is doing,
> > but we may want some coordination here.
> 
> Agreed.
> 
> We actually have a patchset adding a unified device property API in 
> linux-next (http://git.kernel.org/cgit/linux/kernel/git/rafael/linux-pm.git/log/?h=device-properties)
> and I'd prefer to see the "compactization" to happen at that level, if possible,
> rather that for of_ only.

Agreed, this should definitely use the new generalized API.
I have prototyped a similar concept last year, which actually went much
further and also abstracted high-level properties such as interrupts,
gpios, pwm, dma-engine, etc. I still think we should do something
like that, but I've never had the time to follow up and nobody else
picked up my work from back then.

Would others like to see that?

	Arnd

Re: [PATCH 0/1] Compact interface for Device-Tree

[Sascha Hauer]

On Mon, Nov 03, 2014 at 04:06:03PM +0100, Arnd Bergmann wrote:
> On Friday 31 October 2014 23:53:28 Rafael J. Wysocki wrote:
> > On Saturday, November 01, 2014 05:13:45 AM Rob Herring wrote:
> > > On Fri, Oct 31, 2014 at 6:59 AM, Gilad Avidov <gavidov@codeaurora.org> wrote:
> > > >
> > > > Device-Tree compact API
> > > > ------------------------
> > > >
> > > > Common code seen in driver’s probe reads device tree values and handling
> > > > erroneous return codes from all those of_property_read_xxx()  APIs. This
> > > > common code is factored out by the of_property_map module which allows
> > > > driver’s probe to replace that (often lengthy) code with a concise table:
> > > >
> > > > struct of_prop_map map[] = {
> > > >     {"i2c",            &dev->id,        OF_REQ,  OF_ID,  -1},
> > > >     {"qcom,clk-freq-out",    &dev->clk_freq_out,    OF_REQ,  OF_U32,  0},
> > > >     {"qcom,clk-freq-in",    &dev->clk_freq_in,    OF_REQ,  OF_U32,  0},
> > > >     {"qcom,disable-dma",    &dev->disable_dma,    OF_OPT,  OF_BOOL, 0},
> > > >     {"qcom,master-id",    &dev->mstr_id,        OF_SGST, OF_U32,  0},
> > > >     {NULL,            NULL,            0,       0,       0},
> > > > };
> > > >
> > > > Then call populate to read the values into the device’s variables:
> > > >
> > > > ret = of_prop_populate(dev, dev->of_node, map);
> > > 
> > > Interesting idea. The main concern I have with this is there has been
> > > on-going discussions about how to generalize property handling across
> > > DT and ACPI to make drivers more agnostic, so I'm copying a few folks
> > > involved in that. That may be a bit orthogonal to what this is doing,
> > > but we may want some coordination here.
> > 
> > Agreed.
> > 
> > We actually have a patchset adding a unified device property API in 
> > linux-next (http://git.kernel.org/cgit/linux/kernel/git/rafael/linux-pm.git/log/?h=device-properties)
> > and I'd prefer to see the "compactization" to happen at that level, if possible,
> > rather that for of_ only.
> 
> Agreed, this should definitely use the new generalized API.
> I have prototyped a similar concept last year, which actually went much
> further and also abstracted high-level properties such as interrupts,
> gpios, pwm, dma-engine, etc. I still think we should do something
> like that, but I've never had the time to follow up and nobody else
> picked up my work from back then.
> 
> Would others like to see that?

Do you have a link to your patches? I think I remember that you wrote
something like that, but can't find it anymore in the archives.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 0/1] Compact interface for Device-Tree

[Grant Likely]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 2601 bytes --]

On Mon, 03 Nov 2014 16:06:03 +0100
, Arnd Bergmann <arnd@arndb.de>
 wrote:
> On Friday 31 October 2014 23:53:28 Rafael J. Wysocki wrote:
> > On Saturday, November 01, 2014 05:13:45 AM Rob Herring wrote:
> > > On Fri, Oct 31, 2014 at 6:59 AM, Gilad Avidov <gavidov@codeaurora.org> wrote:
> > > >
> > > > Device-Tree compact API
> > > > ------------------------
> > > >
> > > > Common code seen in driver’s probe reads device tree values and handling
> > > > erroneous return codes from all those of_property_read_xxx()  APIs. This
> > > > common code is factored out by the of_property_map module which allows
> > > > driver’s probe to replace that (often lengthy) code with a concise table:
> > > >
> > > > struct of_prop_map map[] = {
> > > >     {"i2c",            &dev->id,        OF_REQ,  OF_ID,  -1},
> > > >     {"qcom,clk-freq-out",    &dev->clk_freq_out,    OF_REQ,  OF_U32,  0},
> > > >     {"qcom,clk-freq-in",    &dev->clk_freq_in,    OF_REQ,  OF_U32,  0},
> > > >     {"qcom,disable-dma",    &dev->disable_dma,    OF_OPT,  OF_BOOL, 0},
> > > >     {"qcom,master-id",    &dev->mstr_id,        OF_SGST, OF_U32,  0},
> > > >     {NULL,            NULL,            0,       0,       0},
> > > > };
> > > >
> > > > Then call populate to read the values into the device’s variables:
> > > >
> > > > ret = of_prop_populate(dev, dev->of_node, map);
> > > 
> > > Interesting idea. The main concern I have with this is there has been
> > > on-going discussions about how to generalize property handling across
> > > DT and ACPI to make drivers more agnostic, so I'm copying a few folks
> > > involved in that. That may be a bit orthogonal to what this is doing,
> > > but we may want some coordination here.
> > 
> > Agreed.
> > 
> > We actually have a patchset adding a unified device property API in 
> > linux-next (http://git.kernel.org/cgit/linux/kernel/git/rafael/linux-pm.git/log/?h=device-properties)
> > and I'd prefer to see the "compactization" to happen at that level, if possible,
> > rather that for of_ only.
> 
> Agreed, this should definitely use the new generalized API.
> I have prototyped a similar concept last year, which actually went much
> further and also abstracted high-level properties such as interrupts,
> gpios, pwm, dma-engine, etc. I still think we should do something
> like that, but I've never had the time to follow up and nobody else
> picked up my work from back then.
> 
> Would others like to see that?

Absolutely. I also tried to do the same thing and didn't get very far.
And, yes, it should be done at the level of the device properties API.

g.

Re: [PATCH 0/1] Compact interface for Device-Tree

[Arnd Bergmann]

On Tuesday 04 November 2014 15:53:29 Grant Likely wrote:
> On Mon, 03 Nov 2014 16:06:03 +0100
> , Arnd Bergmann <arnd@arndb.de>
>  wrote:
> > On Friday 31 October 2014 23:53:28 Rafael J. Wysocki wrote:
> > > On Saturday, November 01, 2014 05:13:45 AM Rob Herring wrote:
> > > > On Fri, Oct 31, 2014 at 6:59 AM, Gilad Avidov <gavidov@codeaurora.org> wrote:
> > > > >
> > > > > Device-Tree compact API
> > > > > ------------------------
> > > > >
> > > > > Common code seen in driver’s probe reads device tree values and handling
> > > > > erroneous return codes from all those of_property_read_xxx()  APIs. This
> > > > > common code is factored out by the of_property_map module which allows
> > > > > driver’s probe to replace that (often lengthy) code with a concise table:
> > > > >
> > > > > struct of_prop_map map[] = {
> > > > >     {"i2c",            &dev->id,        OF_REQ,  OF_ID,  -1},
> > > > >     {"qcom,clk-freq-out",    &dev->clk_freq_out,    OF_REQ,  OF_U32,  0},
> > > > >     {"qcom,clk-freq-in",    &dev->clk_freq_in,    OF_REQ,  OF_U32,  0},
> > > > >     {"qcom,disable-dma",    &dev->disable_dma,    OF_OPT,  OF_BOOL, 0},
> > > > >     {"qcom,master-id",    &dev->mstr_id,        OF_SGST, OF_U32,  0},
> > > > >     {NULL,            NULL,            0,       0,       0},
> > > > > };
> > > > >
> > > > > Then call populate to read the values into the device’s variables:
> > > > >
> > > > > ret = of_prop_populate(dev, dev->of_node, map);
> > > > 
> > > > Interesting idea. The main concern I have with this is there has been
> > > > on-going discussions about how to generalize property handling across
> > > > DT and ACPI to make drivers more agnostic, so I'm copying a few folks
> > > > involved in that. That may be a bit orthogonal to what this is doing,
> > > > but we may want some coordination here.
> > > 
> > > Agreed.
> > > 
> > > We actually have a patchset adding a unified device property API in 
> > > linux-next (http://git.kernel.org/cgit/linux/kernel/git/rafael/linux-pm.git/log/?h=device-properties)
> > > and I'd prefer to see the "compactization" to happen at that level, if possible,
> > > rather that for of_ only.
> > 
> > Agreed, this should definitely use the new generalized API.
> > I have prototyped a similar concept last year, which actually went much
> > further and also abstracted high-level properties such as interrupts,
> > gpios, pwm, dma-engine, etc. I still think we should do something
> > like that, but I've never had the time to follow up and nobody else
> > picked up my work from back then.
> > 
> > Would others like to see that?
> 
> Absolutely. I also tried to do the same thing and didn't get very far.
> And, yes, it should be done at the level of the device properties API.


This is taken from an old email I sent last year on the subject "Re:
[RFC PATCH dtc] C-based DT schema checker integrated into dtc". I haven't
actually followed up at all, and couldn't find the file now, so this
is all I have.

	Arnd

#if 0
/* allocate drvdata */
DEVM_ALLOC,

/* request hardware properties */
DEVM_IRQ,
DEVM_IOMAP,
DEVM_GPIO,
DEVM_DMA_SLAVE,
DEVM_PINCTRL,
DEVM_REGULATOR,
DEVM_CLK,
DEVM_PWM,
DEVM_USBPHY,

/* auxiliary information */
DEVM_PROP_BOOL
DEVM_PROP_U32
DEVM_PROP_STRING
#endif

#error don't bother compiling this file, it's only proof-of-concept

struct device;

struct devm_probe {
        int (*initfunc)(struct device *dev, const struct devm_probe *probe);
        ptrdiff_t offset;
        unsigned int index;
        const char *name;
        void *arg;
        unsigned int flags;
};

/* like offsetof(), but ensures that MEMBER is of type MEMBERTYPE */
#define offsetof_t(TYPE, MEMBER, MEMBERTYPE) \
        ((typeof(MEMBER)*)0 - typeof(MEMBERTYPE*)0 + offsetof((TYPE), (MEMBER)))

/* cast 'ptr' to void* and cause a build warning if it wasn't of 'type' before */
#define typecheck_ptr(ptr, type) \
        (void *)(ptr - (type)NULL + (type)NULL)

/*
 * This is the main entry point for drivers: 
 *
 * Given an zero-terminated array of 'struct devm_probe' entries,
 * this calls all initfunc pointers in the given order. Each initfunc
 * may manipulate a field in the driver_data, as pointed to by
 * the 'offset' member.
 */
int devm_probe(struct device *dev, const struct devm_probe *probe)
{
        int ret;

        for (ret = 0; !ret && probe->initfunc, probe++)
                ret = probe->initfunc(dev, probe);

        return ret;
}
EXPORT_SYMBOL_GPL(devm_probe);

/*
 * this must be the called before any of the others, or not at
 * all, if dev_set_drvdata() has already been called.
 */
static void devm_probe_alloc_release(struct device *dev, void *res)
{
        dev_set_drvdata(dev, NULL);
}
int devm_probe_alloc(struct device *dev, const struct devm_probe *probe)
{
        void *dr;

        if (dev_get_drvdata)
                return -EBUSY;

        dr = alloc_dr(devm_probe_alloc_release, probe->offset, GFP_KERNEL);
        if (unlikely(!dr))
                return -ENOMEM;

        dev_set_drvdata(dev, dr);
        set_node_dbginfo(&dr->node, "devm_probe_alloc", size);
        devres_add(dev, dr->data);
}
EXPORT_SYMBOL_GPL(devm_probe_alloc);


#define DEVM_ALLOC(_struct) \
        { .initfunc = devm_probe_alloc, .offset = sizeof(struct _struct), }

int devm_probe_irq(struct device *dev, const struct devm_probe *probe)
{
        int ret;
        int irq;
        int *irqp;
        int index;
        const char *name;

        /* come up with a reasonable string for the irq name,
           maybe create devm_kasprintf() to allow arbitrary length? */
        name = devm_kmalloc(dev, 32, GFP_KERNEL);
        if (!name)
                return -ENOMEM;
        if (probe->name)
                snprintf(name, 32 - 1, "%s:%s", dev_name(dev), probe->name);
        else
                snprintf(name, 32 - 1, "%s:%n", dev_name(dev), probe->index);

        /* find IRQ number from resource if platform device */
        irq = 0;
        if (dev->bus_type == &platform_bus) {
                struct platform_device *pdev = to_platform_device(dev);

                if (probe->name)
                        irq = platform_get_irq_byname(pdev, probe->name);
                else
                        irq = platform_get_irq(pdev, probe->index);
        }

        /* try getting irq number from device tree if that failed */
        if (!irq && dev->of_node) {
                if (probe->name)
                        index = of_property_match_string(dev->of_node,
                                                         "interrupt-names",
                                                         probe->name);
                else
                        index = probe->index;

                irq = irq_of_parse_and_map(dev->of_node, index);
        }

        /* copy irq number to driver data */
        irqp = dev_get_drvdata(dev) + probe->offset;
        *irqp = irq;
        
        return devm_request_irq(dev, irq, probe->arg, probe->flags, name, probe->dev);
}
EXPORT_SYMBOL_GPL(devm_probe_irq);

#define DEVM_IRQ(_struct, _member, _index, _handler, _flags) {  \
        .initfunc = devm_probe_irq,                             \
        .offset = offsetof_t(struct _struct, _member, int),     \
        .index = _index,                                        \
        .arg = typecheck_ptr((_handler), irq_handler_t),        \
        .flags = _flags,                                        \
}       

#define DEVM_IRQ_NAMED(_struct, _member, _name, _handler, _flags) { \
        .initfunc = devm_probe_irq,                             \
        .offset = offsetof_t(struct _struct, _member, int),     \
        .name = _name,                                          \
        .arg = typecheck_ptr((_handler), irq_handler_t),        \
        .flags = _flags,                                        \
}       


#define DEVM_IOMAP_NOREQUEST 1

int devm_probe_iomap(struct device *dev, const struct devm_probe *probe)
{
        struct resource res, *resp;
        void __iomem *vaddr;
        void * __iomem *vaddrp;
        int ret;

        /* find mem resource from platform device */
        if (dev->bus_type == &platform_bus) {
                struct platform_device *pdev = to_platform_device(dev);

                if (probe->name)
                        resp = platform_get_resource_byname(pdev,
                                        IORESOURCE_MEM, probe->name);
                else
                        resp = platform_get_resource(pdev,
                                        IORESOURCE_MEM, probe->index);
        }

        /* try getting resource from device tree if that failed */
        if (!resp && dev->of_node) {
                if (probe->name)
                        index = of_property_match_string(dev->of_node,
                                                         "reg-names",
                                                         probe->name);
                else
                        index = probe->index;

                ret = of_address_to_resource(dev->of_node, index, &res);
                if (ret)
                        return ret;
                resp = &res;
        }


        if (probe->flags & DEVM_IOMAP_NOREQUEST) {
                vaddr = devm_ioremap(dev, resp->start, resource_size(resp));
                if (!vaddr)
                        return -EINVAL;
        } else {
                vaddr = devm_ioremap_resource(dev, resp);
                if (IS_ERR(vaddr))
                        return PTR_ERR(vaddr);
        }

        vaddrp = dev_get_drvdata(dev) + probe->offset;
        *vaddrp = vaddr;

        return 0;
}
EXPORT_SYMBOL_GPL(devm_probe_iomap);

#define DEVM_IOMAP(_struct, _member, _index, _flags) {                  \
        .initfunc = devm_probe_iomap,                                   \
        .offset = offsetof_t(struct _struct, _member, void __iomem *),  \
        .index = _index,                                                \
        .flags = _flags,                                                \
}       

#define DEVM_IOMAP_NAMED(_struct, _member, _name, _flags) {             \
        .initfunc = devm_probe_iomap,                                   \
        .offset = offsetof_t(struct _struct, _member, void __iomem *),  \
        .name = _name,                                                  \
        .flags = _flags,                                                \
}       

int devm_probe_gpio(struct device *dev, const struct devm_probe *probe)
{
        struct gpio_desc *desc, **descp;

        desc = devm_gpiod_get_index(dev, probe->name, probe->index);
        if (IS_ERR(desc)) {
                /* FIXME: this looks wrong */
                desc = of_get_named_gpiod_flags(dev->of_node, probe->name,
                                                probe->index, NULL);
                if (IS_ERR(desc))
                        return PTR_ERR(desc);
                devm_gpio_request(dev, desc_to_gpio(desc), probe->name);
        }

        descp = dev_get_drvdata(dev) + probe->offset;
        *descp = desc;

        return 0;
}

#define DEVM_GPIO(_struct, _member, _index) {                            \
        .initfunc = devm_probe_iomap,                                    \
        .offset = offsetof_t(struct _struct, _member, struct gpio_desc*),\
        .name = "gpios",                                                 \
        .index = _index,                                                 \
}       

#define DEVM_GPIO_NAMED(_struct, _member, _name) {                       \
        .initfunc = devm_probe_iomap,                                    \
        .offset = offsetof_t(struct _struct, _member, struct gpio_desc*),\
        .name = _name,                                                   \
}       

static void devm_probe_dma_release(struct device *dev, void *chanp)
{
        dma_release_channel(*(struct dma_chan**)chanp);
}

int devm_probe_dma(struct device *dev, const struct devm_probe *probe)
{
        struct dma_chan *chan, **chanp;

        /* there is no devm_dma_request_channel yet, so build our own */
        chanp = devres_alloc(devm_probe_dma_release, sizeof(chanp), GFP_KERNEL);
        if (!chanp)
                return -ENOMEM;

        chan = dma_request_slave_channel(dev, probe->name);
        if (!chan) {
                devres_free(chanp);
                return -EINVAL;
        }

        *chanp = chan;
        devres_add(dev, chanp);

        /* add pointer to the private data */
        chanp = dev_get_drvdata(dev) + probe->offset;
        *chanp = chan;

        return 0;
}

#define DEVM_DMA_SLAVE(_struct, _member, _name)                         \
        .offset = offsetof_t(struct _struct, _member, struct dma_chan*),\
        .name = _name,                                                  \
}

/*
 * simple properties: bool, u32, string
 * no actual need for managed interfaces, just a way to abstract the
 * access to DT or other information source
 */
int devm_probe_prop_bool(struct device *dev, struct devm_probe *probe)
{
        bool *val;

        val = dev_get_drvdata(dev) + probe->offset;
        *val = of_property_read_bool(dev->of_node, probe->name);

        return 0;
}
EXPORT_SYMBOL_GPL(devm_probe_prop_bool);

#define DEVM_PROP_BOOL(_struct, _member, _name)                 \
        .offset = offsetof_t(struct _struct, _member, bool),    \
        .name = _name,                                          \
}

int devm_probe_prop_u32(struct device *dev, struct devm_probe *probe)
{
        u32 *val;
        int ret;

        val = dev_get_drvdata(dev) + probe->offset;
        ret = of_property_read_u32(dev->of_node, probe->name, val);

        return ret;
}
EXPORT_SYMBOL_GPL(devm_probe_prop_bool);

#define DEVM_PROP_U32(_struct, _member, _name)                  \
        .offset = offsetof_t(struct _struct, _member, u32),     \
        .name = _name,                                          \
}

int devm_probe_prop_string(struct device *dev, struct devm_probe *probe)
{
        const char **val;
        int ret;

        val = dev_get_drvdata(dev) + probe->offset;
        ret = of_property_read_string(dev->of_node, probe->name, val);

        return ret;
}
EXPORT_SYMBOL_GPL(devm_probe_prop_bool);

#define DEVM_PROP_STRING(_struct, _member, _name)                       \
        .offset = offsetof_t(struct _struct, _member, const char *),    \
        .name = _name,                                                  \
}

/* example driver */
struct foo_priv {
        spinlock_t lock;
        void __iomem *regs;
        int irq;
        struct gpio_desc *gpio;
        struct dma_chan *rxdma;
        struct dma_chan *txdma;
        bool oldstyle_dma;
};

static irqreturn_t foo_irq_handler(int irq, void *dev);

/*
 * this lists all properties we access from the driver. The list
 * is interpreted by devm_probe() and can be programmatically
 * verified to match the binding.
 */
static const struct devm_probe foo_probe_list[] = {
        DEVM_ALLOC(foo_priv),
        DEVM_IOMAP(foo_priv, regs, 0, 0),
        DEVM_PROP_BOOL(foo_priv, oldstyle_dma, "foo,oldstyle-dma"),
        DEVM_DMA_SLAVE(foo_priv, rxdma, "rx");
        DEVM_DMA_SLAVE(foo_priv, txdma, "tx");
        DEVM_GPIO(foo_priv, gpio, 0);
        DEVM_IRQ_NAMED(foo_priv, irq, foo_irq_handler, "fifo", IRQF_SHARED),
        {},
};

static int foo_probe(struct platform_device *dev)
{
        int ret;

        ret = devm_probe(dev->dev, foo_probe_list);
        if (ret)
                return ret;

        return bar_subsystem_register(&foo_bar_ops, dev);
}