[RFC PATCH 0/5] Add an overlay manager to handle board capes

[RFC PATCH 0/5] Add an overlay manager to handle board capes

[Antoine Tenart]

Hi all,

Many boards now come with dips and compatible capes; among others the
C.H.I.P, or Beaglebones. All these boards have a kernel implementing an
out-of-tree "cape manager" which is used to detected capes, retrieve
their description and apply a corresponding overlay. This series is an
attempt to start a discussion, with an implementation of such a manager
which is somehow generic (i.e. formats or cape detectors can be added).
Other use cases could make use of this manager to dynamically load dt
overlays based on some input / hw presence.

The proposed design is a library which can be used by detector drivers
to parse headers and load the corresponding overlay. Helpers are
provided for this purpose. The whole thing is divided into 3 entities:

- The parser which is project-specific (to allow supporting headers
  already into the wild). It registers a function parsing an header's
  data and filling one or more strings which will be used to find
  matching dtbo on the fs.

- The overlay manager helpers allowing to parse a header to retrieve
  the previously mentioned strings and to load a compatible overlay.

- The detectors which are used to detect capes and get their description
  (to be parsed).

An example of parser and detector is given, compatible with what's done
for the C.H.I.P. As the w1 framework is really bad (and we should
probably do something about that) the detector code is far from being
perfect; but that's not related to what we try to achieve here.

The actual implementation has a limitation: the detectors cannot be
built-in the kernel image as they would likely detect capes at boot time
but will fail to get their corresponding dt overlays as the fs isn't
mounted yet. The only case this can work is when dt overlays are
built-in firmwares. This isn't an issue for the C.H.I.P. use case right
now. There was a discussion about making an helper to wait for the
rootfs to be mount but the answer was "this is the driver's problem".

I'd like to get comments, specifically from people using custom cape
managers, to see if this could fill their needs (with I guess some
modifications).

Thanks!

Antoine

Antoine Tenart (5):
  of: introduce the overlay manager
  of: overlay-mgr: add the CHIP format
  w1: report errors returned by w1_family_notify
  w1: add a callback to call slave when a new device is connected
  of: overlay-mgr: add a detector for headers stored on a ds2431 eeprom
    over w1

 drivers/of/Kconfig                           |   2 +
 drivers/of/Makefile                          |   1 +
 drivers/of/overlay-manager/Kconfig           |  29 ++++
 drivers/of/overlay-manager/Makefile          |   2 +
 drivers/of/overlay-manager/format-chip.c     |  72 ++++++++++
 drivers/of/overlay-manager/overlay-manager.c | 199 +++++++++++++++++++++++++++
 drivers/w1/slaves/w1_ds2431.c                |  39 ++++++
 drivers/w1/w1.c                              |  14 +-
 drivers/w1/w1_family.h                       |   2 +
 include/linux/overlay-manager.h              |  51 +++++++
 10 files changed, 410 insertions(+), 1 deletion(-)
 create mode 100644 drivers/of/overlay-manager/Kconfig
 create mode 100644 drivers/of/overlay-manager/Makefile
 create mode 100644 drivers/of/overlay-manager/format-chip.c
 create mode 100644 drivers/of/overlay-manager/overlay-manager.c
 create mode 100644 include/linux/overlay-manager.h

-- 
2.10.1

[RFC PATCH 1/5] of: introduce the overlay manager

[Antoine Tenart]

The overlay manager is an in-kernel library helping to handle dt overlay
loading when using capes.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 drivers/of/Kconfig                           |   2 +
 drivers/of/Makefile                          |   1 +
 drivers/of/overlay-manager/Kconfig           |   6 +
 drivers/of/overlay-manager/Makefile          |   1 +
 drivers/of/overlay-manager/overlay-manager.c | 199 +++++++++++++++++++++++++++
 include/linux/overlay-manager.h              |  38 +++++
 6 files changed, 247 insertions(+)
 create mode 100644 drivers/of/overlay-manager/Kconfig
 create mode 100644 drivers/of/overlay-manager/Makefile
 create mode 100644 drivers/of/overlay-manager/overlay-manager.c
 create mode 100644 include/linux/overlay-manager.h

diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index bc07ad30c9bf..e57aeaf0bf4f 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -116,4 +116,6 @@ config OF_OVERLAY
 config OF_NUMA
 	bool
 
+source "drivers/of/overlay-manager/Kconfig"
+
 endif # OF
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index d7efd9d458aa..d738fd41271f 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -16,3 +16,4 @@ obj-$(CONFIG_OF_OVERLAY) += overlay.o
 obj-$(CONFIG_OF_NUMA) += of_numa.o
 
 obj-$(CONFIG_OF_UNITTEST) += unittest-data/
+obj-y += overlay-manager/
diff --git a/drivers/of/overlay-manager/Kconfig b/drivers/of/overlay-manager/Kconfig
new file mode 100644
index 000000000000..eeb76054dcb8
--- /dev/null
+++ b/drivers/of/overlay-manager/Kconfig
@@ -0,0 +1,6 @@
+config OF_OVERLAY_MGR
+	bool "Device Tree Overlay Manager"
+	depends on OF_OVERLAY
+	help
+	  Enable the overlay manager to handle automatic overlay loading when
+	  devices are detected.
diff --git a/drivers/of/overlay-manager/Makefile b/drivers/of/overlay-manager/Makefile
new file mode 100644
index 000000000000..86d2b53950e7
--- /dev/null
+++ b/drivers/of/overlay-manager/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_OF_OVERLAY_MGR)			+= overlay-manager.o
diff --git a/drivers/of/overlay-manager/overlay-manager.c b/drivers/of/overlay-manager/overlay-manager.c
new file mode 100644
index 000000000000..a725d7e24d38
--- /dev/null
+++ b/drivers/of/overlay-manager/overlay-manager.c
@@ -0,0 +1,199 @@
+/*
+ * Copyright (C) 2016 - Antoine Tenart <antoine.tenart@free-electrons.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/firmware.h>
+#include <linux/list.h>
+#include <linux/of.h>
+#include <linux/of_fdt.h>
+#include <linux/overlay-manager.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+
+struct overlay_mgr_overlay {
+	struct list_head list;
+	char *name;
+};
+
+LIST_HEAD(overlay_mgr_overlays);
+LIST_HEAD(overlay_mgr_formats);
+DEFINE_SPINLOCK(overlay_mgr_lock);
+DEFINE_SPINLOCK(overlay_mgr_format_lock);
+
+/*
+ * overlay_mgr_register_format()
+ *
+ * Adds a new format candidate to the list of supported formats. The registered
+ * formats are used to parse the headers stored on the dips.
+ */
+int overlay_mgr_register_format(struct overlay_mgr_format *candidate)
+{
+	struct overlay_mgr_format *format;
+	int err = 0;
+
+	spin_lock(&overlay_mgr_format_lock);
+
+	/* Check if the format is already registered */
+	list_for_each_entry(format, &overlay_mgr_formats, list) {
+		if (!strcpy(format->name, candidate->name)) {
+			err = -EEXIST;
+			goto err;
+		}
+	}
+
+	list_add_tail(&candidate->list, &overlay_mgr_formats);
+
+err:
+	spin_unlock(&overlay_mgr_format_lock);
+	return err;
+}
+EXPORT_SYMBOL_GPL(overlay_mgr_register_format);
+
+/*
+ * overlay_mgr_parse()
+ *
+ * Parse raw data with registered format parsers. Fills the candidate string if
+ * one parser understood the raw data format.
+ */
+int overlay_mgr_parse(struct device *dev, void *data, char ***candidates,
+		      unsigned *n)
+{
+	struct list_head *pos, *tmp;
+	struct overlay_mgr_format *format;
+
+	list_for_each_safe(pos, tmp, &overlay_mgr_formats) {
+		format = list_entry(pos, struct overlay_mgr_format, list);
+
+		format->parse(dev, data, candidates, n);
+		if (n > 0)
+			return 0;
+	}
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(overlay_mgr_parse);
+
+static int overlay_mgr_check_overlay(struct device_node *node)
+{
+	struct property *p;
+	const char *str = NULL;
+
+	p = of_find_property(node, "compatible", NULL);
+	if (!p)
+		return -EINVAL;
+
+	do {
+		str = of_prop_next_string(p, str);
+		if (of_machine_is_compatible(str))
+			return 0;
+	} while (str);
+
+	return -EINVAL;
+}
+
+/*
+ * _overlay_mgr_insert()
+ *
+ * Try to request and apply an overlay given a candidate name.
+ */
+static int _overlay_mgr_apply(struct device *dev, char *candidate)
+{
+	struct overlay_mgr_overlay *overlay;
+	struct device_node *node;
+	const struct firmware *firmware;
+	char *firmware_name;
+	int err = 0;
+
+	spin_lock(&overlay_mgr_lock);
+
+	list_for_each_entry(overlay, &overlay_mgr_overlays, list) {
+		if (!strcmp(overlay->name, candidate)) {
+			dev_err(dev, "overlay already loaded\n");
+			err = -EEXIST;
+			goto err_lock;
+		}
+	}
+
+	overlay = devm_kzalloc(dev, sizeof(*overlay), GFP_KERNEL);
+	if (!overlay) {
+		err = -ENOMEM;
+		goto err_lock;
+	}
+
+	overlay->name = candidate;
+
+	firmware_name = kasprintf(GFP_KERNEL, "overlay-%s.dtbo", candidate);
+	if (!firmware_name) {
+		err = -ENOMEM;
+		goto err_free;
+	}
+
+	dev_info(dev, "requesting firmware '%s'\n", firmware_name);
+
+	err = request_firmware_direct(&firmware, firmware_name, dev);
+	if (err) {
+		dev_info(dev, "failed to request firmware '%s'\n",
+			 firmware_name);
+		goto err_free;
+	}
+
+	of_fdt_unflatten_tree((unsigned long *)firmware->data, NULL, &node);
+	if (!node) {
+		dev_err(dev, "failed to unflatted tree\n");
+		err = -EINVAL;
+		goto err_fw;
+	}
+
+	of_node_set_flag(node, OF_DETACHED);
+
+	err = of_resolve_phandles(node);
+	if (err) {
+		dev_err(dev, "failed to resolve phandles: %d\n", err);
+		goto err_fw;
+	}
+
+	err = overlay_mgr_check_overlay(node);
+	if (err) {
+		dev_err(dev, "overlay checks failed: %d\n", err);
+		goto err_fw;
+	}
+
+	err = of_overlay_create(node);
+	if (err < 0) {
+		dev_err(dev, "failed to create overlay: %d\n", err);
+		goto err_fw;
+	}
+
+	list_add_tail(&overlay->list, &overlay_mgr_overlays);
+
+	dev_info(dev, "loaded firmware '%s'\n", firmware_name);
+
+	spin_unlock(&overlay_mgr_lock);
+	return 0;
+
+err_fw:
+	release_firmware(firmware);
+err_free:
+	devm_kfree(dev, overlay);
+err_lock:
+	spin_unlock(&overlay_mgr_lock);
+	return err;
+}
+
+int overlay_mgr_apply(struct device *dev, char **candidates, unsigned n)
+{
+	int i, ret;
+
+	for (i=0; i < n; i++) {
+		ret = _overlay_mgr_apply(dev, candidates[i]);
+		if (!ret)
+			return 0;
+	}
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(overlay_mgr_apply);
diff --git a/include/linux/overlay-manager.h b/include/linux/overlay-manager.h
new file mode 100644
index 000000000000..8adcc4f5ddf6
--- /dev/null
+++ b/include/linux/overlay-manager.h
@@ -0,0 +1,38 @@
+/*
+ * Copyright (C) 2016 - Antoine Tenart <antoine.tenart@free-electrons.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#ifndef __OVERLAY_MGR_H__
+#define __OVERLAY_MGR_H__
+
+#include <linux/device.h>
+#include <linux/list.h>
+#include <linux/sizes.h>
+
+#define OVERLAY_MGR_DIP_MAX_SZ		SZ_128
+
+struct overlay_mgr_format {
+	struct list_head list;
+	char *name;
+	int (*parse)(struct device *dev, void *data, char ***candidates,
+		     unsigned *n);
+};
+
+int overlay_mgr_register_format(struct overlay_mgr_format *candidate);
+int overlay_mgr_parse(struct device *dev, void *data, char ***candidates,
+		      unsigned *n);
+int overlay_mgr_apply(struct device *dev, char **candidates, unsigned n);
+
+#define dip_convert(field)                                      \
+        (                                                       \
+                (sizeof(field) == 1) ? field :                  \
+                (sizeof(field) == 2) ? be16_to_cpu(field) :     \
+                (sizeof(field) == 4) ? be32_to_cpu(field) :     \
+                -1                                              \
+        )
+
+#endif /* __OVERLAY_MGR_H__ */
-- 
2.10.1

Re: [RFC PATCH 1/5] of: introduce the overlay manager

[Mathieu Poirier]

Hi Antoine,

Please find my comments below.

On 26 October 2016 at 08:57, Antoine Tenart
<antoine.tenart@free-electrons.com> wrote:
> The overlay manager is an in-kernel library helping to handle dt overlay
> loading when using capes.
>
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> ---
>  drivers/of/Kconfig                           |   2 +
>  drivers/of/Makefile                          |   1 +
>  drivers/of/overlay-manager/Kconfig           |   6 +
>  drivers/of/overlay-manager/Makefile          |   1 +
>  drivers/of/overlay-manager/overlay-manager.c | 199 +++++++++++++++++++++++++++
>  include/linux/overlay-manager.h              |  38 +++++
>  6 files changed, 247 insertions(+)
>  create mode 100644 drivers/of/overlay-manager/Kconfig
>  create mode 100644 drivers/of/overlay-manager/Makefile
>  create mode 100644 drivers/of/overlay-manager/overlay-manager.c
>  create mode 100644 include/linux/overlay-manager.h
>
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index bc07ad30c9bf..e57aeaf0bf4f 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -116,4 +116,6 @@ config OF_OVERLAY
>  config OF_NUMA
>         bool
>
> +source "drivers/of/overlay-manager/Kconfig"
> +
>  endif # OF
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index d7efd9d458aa..d738fd41271f 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -16,3 +16,4 @@ obj-$(CONFIG_OF_OVERLAY) += overlay.o
>  obj-$(CONFIG_OF_NUMA) += of_numa.o
>
>  obj-$(CONFIG_OF_UNITTEST) += unittest-data/
> +obj-y += overlay-manager/
> diff --git a/drivers/of/overlay-manager/Kconfig b/drivers/of/overlay-manager/Kconfig
> new file mode 100644
> index 000000000000..eeb76054dcb8
> --- /dev/null
> +++ b/drivers/of/overlay-manager/Kconfig
> @@ -0,0 +1,6 @@
> +config OF_OVERLAY_MGR
> +       bool "Device Tree Overlay Manager"
> +       depends on OF_OVERLAY
> +       help
> +         Enable the overlay manager to handle automatic overlay loading when
> +         devices are detected.
> diff --git a/drivers/of/overlay-manager/Makefile b/drivers/of/overlay-manager/Makefile
> new file mode 100644
> index 000000000000..86d2b53950e7
> --- /dev/null
> +++ b/drivers/of/overlay-manager/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_OF_OVERLAY_MGR)                   += overlay-manager.o
> diff --git a/drivers/of/overlay-manager/overlay-manager.c b/drivers/of/overlay-manager/overlay-manager.c
> new file mode 100644
> index 000000000000..a725d7e24d38
> --- /dev/null
> +++ b/drivers/of/overlay-manager/overlay-manager.c
> @@ -0,0 +1,199 @@
> +/*
> + * Copyright (C) 2016 - Antoine Tenart <antoine.tenart@free-electrons.com>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/firmware.h>
> +#include <linux/list.h>
> +#include <linux/of.h>
> +#include <linux/of_fdt.h>
> +#include <linux/overlay-manager.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +
> +struct overlay_mgr_overlay {
> +       struct list_head list;
> +       char *name;
> +};

Please use the proper documentation format for structures.

> +
> +LIST_HEAD(overlay_mgr_overlays);
> +LIST_HEAD(overlay_mgr_formats);
> +DEFINE_SPINLOCK(overlay_mgr_lock);
> +DEFINE_SPINLOCK(overlay_mgr_format_lock);
> +
> +/*
> + * overlay_mgr_register_format()
> + *
> + * Adds a new format candidate to the list of supported formats. The registered
> + * formats are used to parse the headers stored on the dips.
> + */
> +int overlay_mgr_register_format(struct overlay_mgr_format *candidate)
> +{
> +       struct overlay_mgr_format *format;
> +       int err = 0;
> +
> +       spin_lock(&overlay_mgr_format_lock);
> +
> +       /* Check if the format is already registered */
> +       list_for_each_entry(format, &overlay_mgr_formats, list) {
> +               if (!strcpy(format->name, candidate->name)) {

This function is public to the rest of the kernel - limiting the
lenght of ->name and using strncpy() is probably a good idea.

> +                       err = -EEXIST;
> +                       goto err;
> +               }
> +       }
> +
> +       list_add_tail(&candidate->list, &overlay_mgr_formats);
> +
> +err:
> +       spin_unlock(&overlay_mgr_format_lock);
> +       return err;
> +}
> +EXPORT_SYMBOL_GPL(overlay_mgr_register_format);
> +
> +/*
> + * overlay_mgr_parse()
> + *
> + * Parse raw data with registered format parsers. Fills the candidate string if
> + * one parser understood the raw data format.
> + */
> +int overlay_mgr_parse(struct device *dev, void *data, char ***candidates,

I'm pretty sure there is another way to proceed than using 3 levels of
references.  It makes the code hard to read and a prime candidate for
errors.

> +                     unsigned *n)
> +{
> +       struct list_head *pos, *tmp;
> +       struct overlay_mgr_format *format;
> +
> +       list_for_each_safe(pos, tmp, &overlay_mgr_formats) {
> +               format = list_entry(pos, struct overlay_mgr_format, list);

Can you use list_for_each_safe_entry() ?

> +
> +               format->parse(dev, data, candidates, n);

->parse() returns an error code.  It is probably a good idea to check
it.  If it isn't needed then a comment explaining why it is the case
would be appreciated.

> +               if (n > 0)
> +                       return 0;
> +       }
> +
> +       return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(overlay_mgr_parse);
> +
> +static int overlay_mgr_check_overlay(struct device_node *node)
> +{
> +       struct property *p;
> +       const char *str = NULL;
> +
> +       p = of_find_property(node, "compatible", NULL);
> +       if (!p)
> +               return -EINVAL;
> +
> +       do {
> +               str = of_prop_next_string(p, str);
> +               if (of_machine_is_compatible(str))
> +                       return 0;
> +       } while (str);
> +
> +       return -EINVAL;
> +}
> +
> +/*
> + * _overlay_mgr_insert()
> + *
> + * Try to request and apply an overlay given a candidate name.
> + */
> +static int _overlay_mgr_apply(struct device *dev, char *candidate)
> +{
> +       struct overlay_mgr_overlay *overlay;
> +       struct device_node *node;
> +       const struct firmware *firmware;
> +       char *firmware_name;
> +       int err = 0;
> +
> +       spin_lock(&overlay_mgr_lock);
> +
> +       list_for_each_entry(overlay, &overlay_mgr_overlays, list) {
> +               if (!strcmp(overlay->name, candidate)) {
> +                       dev_err(dev, "overlay already loaded\n");
> +                       err = -EEXIST;
> +                       goto err_lock;
> +               }
> +       }
> +
> +       overlay = devm_kzalloc(dev, sizeof(*overlay), GFP_KERNEL);

Function devm_kzalloc() can sleep but you're holding a spinlock - I'm
surprised the kernel didn't complain here.  Allocate the memory before
holding the lock.  If the overly is already loaded simply free it on
the error path.

> +       if (!overlay) {
> +               err = -ENOMEM;
> +               goto err_lock;
> +       }
> +
> +       overlay->name = candidate;
> +
> +       firmware_name = kasprintf(GFP_KERNEL, "overlay-%s.dtbo", candidate);
> +       if (!firmware_name) {
> +               err = -ENOMEM;
> +               goto err_free;
> +       }
> +
> +       dev_info(dev, "requesting firmware '%s'\n", firmware_name);
> +
> +       err = request_firmware_direct(&firmware, firmware_name, dev);
> +       if (err) {
> +               dev_info(dev, "failed to request firmware '%s'\n",
> +                        firmware_name);
> +               goto err_free;
> +       }
> +
> +       of_fdt_unflatten_tree((unsigned long *)firmware->data, NULL, &node);
> +       if (!node) {
> +               dev_err(dev, "failed to unflatted tree\n");
> +               err = -EINVAL;
> +               goto err_fw;
> +       }
> +
> +       of_node_set_flag(node, OF_DETACHED);
> +
> +       err = of_resolve_phandles(node);
> +       if (err) {
> +               dev_err(dev, "failed to resolve phandles: %d\n", err);
> +               goto err_fw;
> +       }
> +
> +       err = overlay_mgr_check_overlay(node);
> +       if (err) {
> +               dev_err(dev, "overlay checks failed: %d\n", err);
> +               goto err_fw;
> +       }
> +
> +       err = of_overlay_create(node);
> +       if (err < 0) {
> +               dev_err(dev, "failed to create overlay: %d\n", err);
> +               goto err_fw;
> +       }
> +
> +       list_add_tail(&overlay->list, &overlay_mgr_overlays);
> +
> +       dev_info(dev, "loaded firmware '%s'\n", firmware_name);
> +

out:

> +       spin_unlock(&overlay_mgr_lock);
> +       return 0;

return err;

> +
> +err_fw:
> +       release_firmware(firmware);
> +err_free:
> +       devm_kfree(dev, overlay);

goto out;

> +err_lock:
> +       spin_unlock(&overlay_mgr_lock);
> +       return err;

This code is repeated twice.  See above corrections to fix the situation.

> +}
> +
> +int overlay_mgr_apply(struct device *dev, char **candidates, unsigned n)
> +{
> +       int i, ret;
> +
> +       for (i=0; i < n; i++) {

I'm surprised checkpatch.pl let you get away with this one.

> +               ret = _overlay_mgr_apply(dev, candidates[i]);
> +               if (!ret)
> +                       return 0;
> +       }
> +
> +       return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(overlay_mgr_apply);
> diff --git a/include/linux/overlay-manager.h b/include/linux/overlay-manager.h
> new file mode 100644
> index 000000000000..8adcc4f5ddf6
> --- /dev/null
> +++ b/include/linux/overlay-manager.h
> @@ -0,0 +1,38 @@
> +/*
> + * Copyright (C) 2016 - Antoine Tenart <antoine.tenart@free-electrons.com>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#ifndef __OVERLAY_MGR_H__
> +#define __OVERLAY_MGR_H__
> +
> +#include <linux/device.h>
> +#include <linux/list.h>
> +#include <linux/sizes.h>
> +
> +#define OVERLAY_MGR_DIP_MAX_SZ         SZ_128
> +
> +struct overlay_mgr_format {
> +       struct list_head list;
> +       char *name;
> +       int (*parse)(struct device *dev, void *data, char ***candidates,
> +                    unsigned *n);
> +};

Please use the kernel documentation standard.

> +
> +int overlay_mgr_register_format(struct overlay_mgr_format *candidate);
> +int overlay_mgr_parse(struct device *dev, void *data, char ***candidates,
> +                     unsigned *n);
> +int overlay_mgr_apply(struct device *dev, char **candidates, unsigned n);
> +
> +#define dip_convert(field)                                      \
> +        (                                                       \
> +                (sizeof(field) == 1) ? field :                  \
> +                (sizeof(field) == 2) ? be16_to_cpu(field) :     \
> +                (sizeof(field) == 4) ? be32_to_cpu(field) :     \
> +                -1                                              \
> +        )

Please document your macro definition.  Otherwise reviewers are left guessing...

> +
> +#endif /* __OVERLAY_MGR_H__ */
> --
> 2.10.1
>

Re: [RFC PATCH 1/5] of: introduce the overlay manager

[Thomas Petazzoni]

Hello,

On Wed, 26 Oct 2016 10:29:59 -0600, Mathieu Poirier wrote:

> > +       overlay = devm_kzalloc(dev, sizeof(*overlay), GFP_KERNEL);  
> 
> Function devm_kzalloc() can sleep but you're holding a spinlock - I'm
> surprised the kernel didn't complain here.  Allocate the memory before
> holding the lock.  If the overly is already loaded simply free it on
> the error path.

Actually, I'm not sure using a spinlock here is appropriate. Using a
mutex would probably be better.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [RFC PATCH 1/5] of: introduce the overlay manager

[Antoine Tenart]

[-- Attachment #1: Type: text/plain, Size: 3670 bytes --]

Hello Mathieu,

On Wed, Oct 26, 2016 at 10:29:59AM -0600, Mathieu Poirier wrote:
> 
> Please find my comments below.

Thanks for the comments. I expected more distant reviews, on the overall
architecture to know if this could fit the needs of others. But anyway
your comments are helpful if we ever decide to go with an overlay
manager like this one.

> On 26 October 2016 at 08:57, Antoine Tenart
> <antoine.tenart@free-electrons.com> wrote:
> > +
> > +/*
> > + * overlay_mgr_register_format()
> > + *
> > + * Adds a new format candidate to the list of supported formats. The registered
> > + * formats are used to parse the headers stored on the dips.
> > + */
> > +int overlay_mgr_register_format(struct overlay_mgr_format *candidate)
> > +{
> > +       struct overlay_mgr_format *format;
> > +       int err = 0;
> > +
> > +       spin_lock(&overlay_mgr_format_lock);
> > +
> > +       /* Check if the format is already registered */
> > +       list_for_each_entry(format, &overlay_mgr_formats, list) {
> > +               if (!strcpy(format->name, candidate->name)) {
> 
> This function is public to the rest of the kernel - limiting the
> lenght of ->name and using strncpy() is probably a good idea.

I totally agree. This was in fact something I wanted to do.

> > +
> > +/*
> > + * overlay_mgr_parse()
> > + *
> > + * Parse raw data with registered format parsers. Fills the candidate string if
> > + * one parser understood the raw data format.
> > + */
> > +int overlay_mgr_parse(struct device *dev, void *data, char ***candidates,
> 
> I'm pretty sure there is another way to proceed than using 3 levels of
> references.  It makes the code hard to read and a prime candidate for
> errors.

Sure. I guess we could allocate an array of fixed-length strings which
would be less flexible but I don't think we need something that flexible
here.

> 
> > +
> > +               format->parse(dev, data, candidates, n);
> 
> ->parse() returns an error code.  It is probably a good idea to check
> it.  If it isn't needed then a comment explaining why it is the case
> would be appreciated.

So the point of the parse function is to determine if the data read from
a source is a compatible header of a given format. Returning an error
doesn't mean the header won't be recognized by another one.

We could maybe handle this better, by returning an error iif different
that -EINVAL. Or we could have one function to check the compatibility
and one to parse it, if compatible.

> > +static int _overlay_mgr_apply(struct device *dev, char *candidate)
> > +{
> > +       struct overlay_mgr_overlay *overlay;
> > +       struct device_node *node;
> > +       const struct firmware *firmware;
> > +       char *firmware_name;
> > +       int err = 0;
> > +
> > +       spin_lock(&overlay_mgr_lock);
> > +
> > +       list_for_each_entry(overlay, &overlay_mgr_overlays, list) {
> > +               if (!strcmp(overlay->name, candidate)) {
> > +                       dev_err(dev, "overlay already loaded\n");
> > +                       err = -EEXIST;
> > +                       goto err_lock;
> > +               }
> > +       }
> > +
> > +       overlay = devm_kzalloc(dev, sizeof(*overlay), GFP_KERNEL);
> 
> Function devm_kzalloc() can sleep but you're holding a spinlock - I'm
> surprised the kernel didn't complain here.  Allocate the memory before
> holding the lock.  If the overly is already loaded simply free it on
> the error path.

Right.

Thanks,

Antoine

-- 
Antoine Ténart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [RFC PATCH 1/5] of: introduce the overlay manager

[Mathieu Poirier]

On 27 October 2016 at 08:03, Antoine Tenart
<antoine.tenart@free-electrons.com> wrote:
> Hello Mathieu,
>
> On Wed, Oct 26, 2016 at 10:29:59AM -0600, Mathieu Poirier wrote:
>>
>> Please find my comments below.
>
> Thanks for the comments. I expected more distant reviews, on the overall
> architecture to know if this could fit the needs of others. But anyway
> your comments are helpful if we ever decide to go with an overlay
> manager like this one.

I agree - something like this should have attracted more reviews.  I
suggest providing a better explanation, i.e what you are doing and why
in more details.  I reviewed your set and I'm still not sure of
exactly what it does, hence not being able to comment on the validity
of the approach.

I'm pretty sure there is value in what you are suggesting, it's a
matter of getting people to understand the approach and motivation.

>
>> On 26 October 2016 at 08:57, Antoine Tenart
>> <antoine.tenart@free-electrons.com> wrote:
>> > +
>> > +/*
>> > + * overlay_mgr_register_format()
>> > + *
>> > + * Adds a new format candidate to the list of supported formats. The registered
>> > + * formats are used to parse the headers stored on the dips.
>> > + */
>> > +int overlay_mgr_register_format(struct overlay_mgr_format *candidate)
>> > +{
>> > +       struct overlay_mgr_format *format;
>> > +       int err = 0;
>> > +
>> > +       spin_lock(&overlay_mgr_format_lock);
>> > +
>> > +       /* Check if the format is already registered */
>> > +       list_for_each_entry(format, &overlay_mgr_formats, list) {
>> > +               if (!strcpy(format->name, candidate->name)) {
>>
>> This function is public to the rest of the kernel - limiting the
>> lenght of ->name and using strncpy() is probably a good idea.
>
> I totally agree. This was in fact something I wanted to do.
>
>> > +
>> > +/*
>> > + * overlay_mgr_parse()
>> > + *
>> > + * Parse raw data with registered format parsers. Fills the candidate string if
>> > + * one parser understood the raw data format.
>> > + */
>> > +int overlay_mgr_parse(struct device *dev, void *data, char ***candidates,
>>
>> I'm pretty sure there is another way to proceed than using 3 levels of
>> references.  It makes the code hard to read and a prime candidate for
>> errors.
>
> Sure. I guess we could allocate an array of fixed-length strings which
> would be less flexible but I don't think we need something that flexible
> here.
>
>>
>> > +
>> > +               format->parse(dev, data, candidates, n);
>>
>> ->parse() returns an error code.  It is probably a good idea to check
>> it.  If it isn't needed then a comment explaining why it is the case
>> would be appreciated.
>
> So the point of the parse function is to determine if the data read from
> a source is a compatible header of a given format. Returning an error
> doesn't mean the header won't be recognized by another one.
>
> We could maybe handle this better, by returning an error iif different
> that -EINVAL. Or we could have one function to check the compatibility
> and one to parse it, if compatible.
>
>> > +static int _overlay_mgr_apply(struct device *dev, char *candidate)
>> > +{
>> > +       struct overlay_mgr_overlay *overlay;
>> > +       struct device_node *node;
>> > +       const struct firmware *firmware;
>> > +       char *firmware_name;
>> > +       int err = 0;
>> > +
>> > +       spin_lock(&overlay_mgr_lock);
>> > +
>> > +       list_for_each_entry(overlay, &overlay_mgr_overlays, list) {
>> > +               if (!strcmp(overlay->name, candidate)) {
>> > +                       dev_err(dev, "overlay already loaded\n");
>> > +                       err = -EEXIST;
>> > +                       goto err_lock;
>> > +               }
>> > +       }
>> > +
>> > +       overlay = devm_kzalloc(dev, sizeof(*overlay), GFP_KERNEL);
>>
>> Function devm_kzalloc() can sleep but you're holding a spinlock - I'm
>> surprised the kernel didn't complain here.  Allocate the memory before
>> holding the lock.  If the overly is already loaded simply free it on
>> the error path.
>
> Right.
>
> Thanks,
>
> Antoine
>
> --
> Antoine Ténart, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

Re: [RFC PATCH 1/5] of: introduce the overlay manager

[Antoine Tenart]

[-- Attachment #1: Type: text/plain, Size: 752 bytes --]

On Thu, Oct 27, 2016 at 08:49:29AM -0600, Mathieu Poirier wrote:
> 
> I agree - something like this should have attracted more reviews.  I
> suggest providing a better explanation, i.e what you are doing and why
> in more details.  I reviewed your set and I'm still not sure of
> exactly what it does, hence not being able to comment on the validity
> of the approach.
> 
> I'm pretty sure there is value in what you are suggesting, it's a
> matter of getting people to understand the approach and motivation.

That's what I tried to do in the cover-letter. What do you think is
missing that I should add in it?

Thanks,

Antoine

-- 
Antoine Ténart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [RFC PATCH 1/5] of: introduce the overlay manager

[Matthias Brugger]

On 10/26/2016 04:57 PM, Antoine Tenart wrote:
> The overlay manager is an in-kernel library helping to handle dt overlay
> loading when using capes.
>
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> ---
>  drivers/of/Kconfig                           |   2 +
>  drivers/of/Makefile                          |   1 +
>  drivers/of/overlay-manager/Kconfig           |   6 +
>  drivers/of/overlay-manager/Makefile          |   1 +
>  drivers/of/overlay-manager/overlay-manager.c | 199 +++++++++++++++++++++++++++
>  include/linux/overlay-manager.h              |  38 +++++
>  6 files changed, 247 insertions(+)
>  create mode 100644 drivers/of/overlay-manager/Kconfig
>  create mode 100644 drivers/of/overlay-manager/Makefile
>  create mode 100644 drivers/of/overlay-manager/overlay-manager.c
>  create mode 100644 include/linux/overlay-manager.h
>
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index bc07ad30c9bf..e57aeaf0bf4f 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -116,4 +116,6 @@ config OF_OVERLAY
>  config OF_NUMA
>  	bool
>
> +source "drivers/of/overlay-manager/Kconfig"
> +
>  endif # OF
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index d7efd9d458aa..d738fd41271f 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -16,3 +16,4 @@ obj-$(CONFIG_OF_OVERLAY) += overlay.o
>  obj-$(CONFIG_OF_NUMA) += of_numa.o
>
>  obj-$(CONFIG_OF_UNITTEST) += unittest-data/
> +obj-y += overlay-manager/
> diff --git a/drivers/of/overlay-manager/Kconfig b/drivers/of/overlay-manager/Kconfig
> new file mode 100644
> index 000000000000..eeb76054dcb8
> --- /dev/null
> +++ b/drivers/of/overlay-manager/Kconfig
> @@ -0,0 +1,6 @@
> +config OF_OVERLAY_MGR
> +	bool "Device Tree Overlay Manager"
> +	depends on OF_OVERLAY
> +	help
> +	  Enable the overlay manager to handle automatic overlay loading when
> +	  devices are detected.
> diff --git a/drivers/of/overlay-manager/Makefile b/drivers/of/overlay-manager/Makefile
> new file mode 100644
> index 000000000000..86d2b53950e7
> --- /dev/null
> +++ b/drivers/of/overlay-manager/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_OF_OVERLAY_MGR)			+= overlay-manager.o
> diff --git a/drivers/of/overlay-manager/overlay-manager.c b/drivers/of/overlay-manager/overlay-manager.c
> new file mode 100644
> index 000000000000..a725d7e24d38
> --- /dev/null
> +++ b/drivers/of/overlay-manager/overlay-manager.c
> @@ -0,0 +1,199 @@
> +/*
> + * Copyright (C) 2016 - Antoine Tenart <antoine.tenart@free-electrons.com>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/firmware.h>
> +#include <linux/list.h>
> +#include <linux/of.h>
> +#include <linux/of_fdt.h>
> +#include <linux/overlay-manager.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +
> +struct overlay_mgr_overlay {
> +	struct list_head list;
> +	char *name;
> +};
> +
> +LIST_HEAD(overlay_mgr_overlays);
> +LIST_HEAD(overlay_mgr_formats);

Maybe you can find some better names for this, or rename the structs.
This will make the code more readable.

> +DEFINE_SPINLOCK(overlay_mgr_lock);
> +DEFINE_SPINLOCK(overlay_mgr_format_lock);

As Thomas already said, a mutex should be fine. We are not doing any 
time critical here, right?

> +
> +/*
> + * overlay_mgr_register_format()
> + *
> + * Adds a new format candidate to the list of supported formats. The registered
> + * formats are used to parse the headers stored on the dips.
> + */
> +int overlay_mgr_register_format(struct overlay_mgr_format *candidate)
> +{
> +	struct overlay_mgr_format *format;
> +	int err = 0;
> +
> +	spin_lock(&overlay_mgr_format_lock);
> +
> +	/* Check if the format is already registered */
> +	list_for_each_entry(format, &overlay_mgr_formats, list) {
> +		if (!strcpy(format->name, candidate->name)) {
> +			err = -EEXIST;
> +			goto err;
> +		}
> +	}
> +
> +	list_add_tail(&candidate->list, &overlay_mgr_formats);
> +
> +err:
> +	spin_unlock(&overlay_mgr_format_lock);
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(overlay_mgr_register_format);
> +
> +/*
> + * overlay_mgr_parse()
> + *
> + * Parse raw data with registered format parsers. Fills the candidate string if
> + * one parser understood the raw data format.
> + */
> +int overlay_mgr_parse(struct device *dev, void *data, char ***candidates,
> +		      unsigned *n)
> +{
> +	struct list_head *pos, *tmp;
> +	struct overlay_mgr_format *format;
> +
> +	list_for_each_safe(pos, tmp, &overlay_mgr_formats) {
> +		format = list_entry(pos, struct overlay_mgr_format, list);
> +
> +		format->parse(dev, data, candidates, n);
> +		if (n > 0)
> +			return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(overlay_mgr_parse);
> +
> +static int overlay_mgr_check_overlay(struct device_node *node)
> +{
> +	struct property *p;
> +	const char *str = NULL;
> +
> +	p = of_find_property(node, "compatible", NULL);
> +	if (!p)
> +		return -EINVAL;
> +
> +	do {
> +		str = of_prop_next_string(p, str);
> +		if (of_machine_is_compatible(str))
> +			return 0;
> +	} while (str);
> +
> +	return -EINVAL;
> +}
> +
> +/*
> + * _overlay_mgr_insert()
> + *
> + * Try to request and apply an overlay given a candidate name.
> + */
> +static int _overlay_mgr_apply(struct device *dev, char *candidate)

Should be __overlay_mgr_apply(...)

Cheers,
Matthias

Re: [RFC PATCH 1/5] of: introduce the overlay manager

[Pantelis Antoniou]

Hi Antoine,

> On Oct 26, 2016, at 17:57 , Antoine Tenart <antoine.tenart@free-electrons.com> wrote:
> 
> The overlay manager is an in-kernel library helping to handle dt overlay
> loading when using capes.
> 

All in all a nice idea. Comments inline.

> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> ---
> drivers/of/Kconfig                           |   2 +
> drivers/of/Makefile                          |   1 +
> drivers/of/overlay-manager/Kconfig           |   6 +
> drivers/of/overlay-manager/Makefile          |   1 +
> drivers/of/overlay-manager/overlay-manager.c | 199 +++++++++++++++++++++++++++
> include/linux/overlay-manager.h              |  38 +++++
> 6 files changed, 247 insertions(+)
> create mode 100644 drivers/of/overlay-manager/Kconfig
> create mode 100644 drivers/of/overlay-manager/Makefile
> create mode 100644 drivers/of/overlay-manager/overlay-manager.c
> create mode 100644 include/linux/overlay-manager.h
> 
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index bc07ad30c9bf..e57aeaf0bf4f 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -116,4 +116,6 @@ config OF_OVERLAY
> config OF_NUMA
> 	bool
> 
> +source "drivers/of/overlay-manager/Kconfig"
> +
> endif # OF
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index d7efd9d458aa..d738fd41271f 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -16,3 +16,4 @@ obj-$(CONFIG_OF_OVERLAY) += overlay.o
> obj-$(CONFIG_OF_NUMA) += of_numa.o
> 
> obj-$(CONFIG_OF_UNITTEST) += unittest-data/
> +obj-y += overlay-manager/
> diff --git a/drivers/of/overlay-manager/Kconfig b/drivers/of/overlay-manager/Kconfig
> new file mode 100644
> index 000000000000..eeb76054dcb8
> --- /dev/null
> +++ b/drivers/of/overlay-manager/Kconfig
> @@ -0,0 +1,6 @@
> +config OF_OVERLAY_MGR
> +	bool "Device Tree Overlay Manager"
> +	depends on OF_OVERLAY
> +	help
> +	  Enable the overlay manager to handle automatic overlay loading when
> +	  devices are detected.
> diff --git a/drivers/of/overlay-manager/Makefile b/drivers/of/overlay-manager/Makefile
> new file mode 100644
> index 000000000000..86d2b53950e7
> --- /dev/null
> +++ b/drivers/of/overlay-manager/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_OF_OVERLAY_MGR)			+= overlay-manager.o
> diff --git a/drivers/of/overlay-manager/overlay-manager.c b/drivers/of/overlay-manager/overlay-manager.c
> new file mode 100644
> index 000000000000..a725d7e24d38
> --- /dev/null
> +++ b/drivers/of/overlay-manager/overlay-manager.c
> @@ -0,0 +1,199 @@
> +/*
> + * Copyright (C) 2016 - Antoine Tenart <antoine.tenart@free-electrons.com>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/firmware.h>
> +#include <linux/list.h>
> +#include <linux/of.h>
> +#include <linux/of_fdt.h>
> +#include <linux/overlay-manager.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +
> +struct overlay_mgr_overlay {
> +	struct list_head list;
> +	char *name;
> +};
> +
> +LIST_HEAD(overlay_mgr_overlays);
> +LIST_HEAD(overlay_mgr_formats);
> +DEFINE_SPINLOCK(overlay_mgr_lock);
> +DEFINE_SPINLOCK(overlay_mgr_format_lock);
> +
> +/*
> + * overlay_mgr_register_format()
> + *
> + * Adds a new format candidate to the list of supported formats. The registered
> + * formats are used to parse the headers stored on the dips.
> + */
> +int overlay_mgr_register_format(struct overlay_mgr_format *candidate)
> +{
> +	struct overlay_mgr_format *format;
> +	int err = 0;
> +
> +	spin_lock(&overlay_mgr_format_lock);
> +
> +	/* Check if the format is already registered */
> +	list_for_each_entry(format, &overlay_mgr_formats, list) {
> +		if (!strcpy(format->name, candidate->name)) {
> +			err = -EEXIST;
> +			goto err;
> +		}
> +	}
> +
> +	list_add_tail(&candidate->list, &overlay_mgr_formats);
> +
> +err:
> +	spin_unlock(&overlay_mgr_format_lock);
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(overlay_mgr_register_format);
> +
> +/*
> + * overlay_mgr_parse()
> + *
> + * Parse raw data with registered format parsers. Fills the candidate string if
> + * one parser understood the raw data format.
> + */
> +int overlay_mgr_parse(struct device *dev, void *data, char ***candidates,
> +		      unsigned *n)
> +{
> +	struct list_head *pos, *tmp;
> +	struct overlay_mgr_format *format;
> +
> +	list_for_each_safe(pos, tmp, &overlay_mgr_formats) {
> +		format = list_entry(pos, struct overlay_mgr_format, list);
> +
> +		format->parse(dev, data, candidates, n);
> +		if (n > 0)
> +			return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(overlay_mgr_parse);
> +
> +static int overlay_mgr_check_overlay(struct device_node *node)
> +{
> +	struct property *p;
> +	const char *str = NULL;
> +
> +	p = of_find_property(node, "compatible", NULL);
> +	if (!p)
> +		return -EINVAL;
> +
> +	do {
> +		str = of_prop_next_string(p, str);
> +		if (of_machine_is_compatible(str))
> +			return 0;
> +	} while (str);
> +
> +	return -EINVAL;
> +}
> +
> +/*
> + * _overlay_mgr_insert()
> + *
> + * Try to request and apply an overlay given a candidate name.
> + */
> +static int _overlay_mgr_apply(struct device *dev, char *candidate)
> +{
> +	struct overlay_mgr_overlay *overlay;
> +	struct device_node *node;
> +	const struct firmware *firmware;
> +	char *firmware_name;
> +	int err = 0;
> +
> +	spin_lock(&overlay_mgr_lock);
> +
> +	list_for_each_entry(overlay, &overlay_mgr_overlays, list) {
> +		if (!strcmp(overlay->name, candidate)) {
> +			dev_err(dev, "overlay already loaded\n");
> +			err = -EEXIST;
> +			goto err_lock;
> +		}
> +	}
> +
> +	overlay = devm_kzalloc(dev, sizeof(*overlay), GFP_KERNEL);
> +	if (!overlay) {
> +		err = -ENOMEM;
> +		goto err_lock;
> +	}
> +
> +	overlay->name = candidate;
> +
> +	firmware_name = kasprintf(GFP_KERNEL, "overlay-%s.dtbo", candidate);
> +	if (!firmware_name) {
> +		err = -ENOMEM;
> +		goto err_free;
> +	}
> +
> +	dev_info(dev, "requesting firmware '%s'\n", firmware_name);
> +
> +	err = request_firmware_direct(&firmware, firmware_name, dev);
> +	if (err) {
> +		dev_info(dev, "failed to request firmware '%s'\n",
> +			 firmware_name);
> +		goto err_free;
> +	}
> +
> +	of_fdt_unflatten_tree((unsigned long *)firmware->data, NULL, &node);
> +	if (!node) {
> +		dev_err(dev, "failed to unflatted tree\n");
> +		err = -EINVAL;
> +		goto err_fw;
> +	}
> +
> +	of_node_set_flag(node, OF_DETACHED);
> +
> +	err = of_resolve_phandles(node);
> +	if (err) {
> +		dev_err(dev, "failed to resolve phandles: %d\n", err);
> +		goto err_fw;
> +	}
> +
> +	err = overlay_mgr_check_overlay(node);
> +	if (err) {
> +		dev_err(dev, "overlay checks failed: %d\n", err);
> +		goto err_fw;
> +	}
> +
> +	err = of_overlay_create(node);
> +	if (err < 0) {
> +		dev_err(dev, "failed to create overlay: %d\n", err);
> +		goto err_fw;
> +	}
> +
> +	list_add_tail(&overlay->list, &overlay_mgr_overlays);
> +
> +	dev_info(dev, "loaded firmware '%s'\n", firmware_name);
> +
> +	spin_unlock(&overlay_mgr_lock);
> +	return 0;
> +
> +err_fw:
> +	release_firmware(firmware);
> +err_free:
> +	devm_kfree(dev, overlay);
> +err_lock:
> +	spin_unlock(&overlay_mgr_lock);
> +	return err;
> +}
> +
> +int overlay_mgr_apply(struct device *dev, char **candidates, unsigned n)
> +{
> +	int i, ret;
> +
> +	for (i=0; i < n; i++) {
> +		ret = _overlay_mgr_apply(dev, candidates[i]);
> +		if (!ret)
> +			return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(overlay_mgr_apply);
> diff --git a/include/linux/overlay-manager.h b/include/linux/overlay-manager.h
> new file mode 100644
> index 000000000000..8adcc4f5ddf6
> --- /dev/null
> +++ b/include/linux/overlay-manager.h
> @@ -0,0 +1,38 @@
> +/*
> + * Copyright (C) 2016 - Antoine Tenart <antoine.tenart@free-electrons.com>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#ifndef __OVERLAY_MGR_H__
> +#define __OVERLAY_MGR_H__
> +
> +#include <linux/device.h>
> +#include <linux/list.h>
> +#include <linux/sizes.h>
> +
> +#define OVERLAY_MGR_DIP_MAX_SZ		SZ_128
> +


This should not be here; if it’s general kernel plumbing no mention to
specific boards/archs are relevant.


> +struct overlay_mgr_format {
> +	struct list_head list;
> +	char *name;
> +	int (*parse)(struct device *dev, void *data, char ***candidates,
> +		     unsigned *n);
> +};
> +
> +int overlay_mgr_register_format(struct overlay_mgr_format *candidate);
> +int overlay_mgr_parse(struct device *dev, void *data, char ***candidates,
> +		      unsigned *n);
> +int overlay_mgr_apply(struct device *dev, char **candidates, unsigned n);
> +
> +#define dip_convert(field)                                      \
> +        (                                                       \
> +                (sizeof(field) == 1) ? field :                  \
> +                (sizeof(field) == 2) ? be16_to_cpu(field) :     \
> +                (sizeof(field) == 4) ? be32_to_cpu(field) :     \
> +                -1                                              \
> +        )
> +

Same as above.

> +#endif /* __OVERLAY_MGR_H__ */
> -- 
> 2.10.1

Regards

— Pantelis

Re: [RFC PATCH 1/5] of: introduce the overlay manager

[Pantelis Antoniou]

Hi Antoine,

> On Oct 26, 2016, at 17:57 , Antoine Tenart <antoine.tenart@free-electrons.com> wrote:
> 
> The overlay manager is an in-kernel library helping to handle dt overlay
> loading when using capes.
> 

Code related comments

> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> ---
> drivers/of/Kconfig                           |   2 +
> drivers/of/Makefile                          |   1 +
> drivers/of/overlay-manager/Kconfig           |   6 +
> drivers/of/overlay-manager/Makefile          |   1 +
> drivers/of/overlay-manager/overlay-manager.c | 199 +++++++++++++++++++++++++++
> include/linux/overlay-manager.h              |  38 +++++
> 6 files changed, 247 insertions(+)
> create mode 100644 drivers/of/overlay-manager/Kconfig
> create mode 100644 drivers/of/overlay-manager/Makefile
> create mode 100644 drivers/of/overlay-manager/overlay-manager.c
> create mode 100644 include/linux/overlay-manager.h
> 
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index bc07ad30c9bf..e57aeaf0bf4f 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -116,4 +116,6 @@ config OF_OVERLAY
> config OF_NUMA
> 	bool
> 
> +source "drivers/of/overlay-manager/Kconfig"
> +
> endif # OF
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index d7efd9d458aa..d738fd41271f 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -16,3 +16,4 @@ obj-$(CONFIG_OF_OVERLAY) += overlay.o
> obj-$(CONFIG_OF_NUMA) += of_numa.o
> 
> obj-$(CONFIG_OF_UNITTEST) += unittest-data/
> +obj-y += overlay-manager/
> diff --git a/drivers/of/overlay-manager/Kconfig b/drivers/of/overlay-manager/Kconfig
> new file mode 100644
> index 000000000000..eeb76054dcb8
> --- /dev/null
> +++ b/drivers/of/overlay-manager/Kconfig
> @@ -0,0 +1,6 @@
> +config OF_OVERLAY_MGR
> +	bool "Device Tree Overlay Manager"
> +	depends on OF_OVERLAY
> +	help
> +	  Enable the overlay manager to handle automatic overlay loading when
> +	  devices are detected.
> diff --git a/drivers/of/overlay-manager/Makefile b/drivers/of/overlay-manager/Makefile
> new file mode 100644
> index 000000000000..86d2b53950e7
> --- /dev/null
> +++ b/drivers/of/overlay-manager/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_OF_OVERLAY_MGR)			+= overlay-manager.o
> diff --git a/drivers/of/overlay-manager/overlay-manager.c b/drivers/of/overlay-manager/overlay-manager.c
> new file mode 100644
> index 000000000000..a725d7e24d38
> --- /dev/null
> +++ b/drivers/of/overlay-manager/overlay-manager.c
> @@ -0,0 +1,199 @@
> +/*
> + * Copyright (C) 2016 - Antoine Tenart <antoine.tenart@free-electrons.com>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/firmware.h>
> +#include <linux/list.h>
> +#include <linux/of.h>
> +#include <linux/of_fdt.h>
> +#include <linux/overlay-manager.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +
> +struct overlay_mgr_overlay {
> +	struct list_head list;
> +	char *name;
> +};
> +
> +LIST_HEAD(overlay_mgr_overlays);
> +LIST_HEAD(overlay_mgr_formats);
> +DEFINE_SPINLOCK(overlay_mgr_lock);
> +DEFINE_SPINLOCK(overlay_mgr_format_lock);
> +

Is there a reason for using spinlocks here? OF uses a mutex for
locking since we don’t expect OF manipulation occurring in a
non schedulable context.

> +/*
> + * overlay_mgr_register_format()
> + *
> + * Adds a new format candidate to the list of supported formats. The registered
> + * formats are used to parse the headers stored on the dips.
> + */
> +int overlay_mgr_register_format(struct overlay_mgr_format *candidate)
> +{
> +	struct overlay_mgr_format *format;
> +	int err = 0;
> +
> +	spin_lock(&overlay_mgr_format_lock);
> +
> +	/* Check if the format is already registered */
> +	list_for_each_entry(format, &overlay_mgr_formats, list) {
> +		if (!strcpy(format->name, candidate->name)) {
> +			err = -EEXIST;
> +			goto err;
> +		}
> +	}
> +
> +	list_add_tail(&candidate->list, &overlay_mgr_formats);
> +
> +err:
> +	spin_unlock(&overlay_mgr_format_lock);
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(overlay_mgr_register_format);
> +
> +/*
> + * overlay_mgr_parse()
> + *
> + * Parse raw data with registered format parsers. Fills the candidate string if
> + * one parser understood the raw data format.
> + */
> +int overlay_mgr_parse(struct device *dev, void *data, char ***candidates,
> +		      unsigned *n)
> +{

The two argument format is not very readable. Perhaps use a structure instead?

> +	struct list_head *pos, *tmp;
> +	struct overlay_mgr_format *format;
> +
> +	list_for_each_safe(pos, tmp, &overlay_mgr_formats) {
> +		format = list_entry(pos, struct overlay_mgr_format, list);
> +
> +		format->parse(dev, data, candidates, n);
> +		if (n > 0)
> +			return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(overlay_mgr_parse);
> +
> +static int overlay_mgr_check_overlay(struct device_node *node)
> +{
> +	struct property *p;
> +	const char *str = NULL;
> +
> +	p = of_find_property(node, "compatible", NULL);
> +	if (!p)
> +		return -EINVAL;
> +
> +	do {
> +		str = of_prop_next_string(p, str);
> +		if (of_machine_is_compatible(str))
> +			return 0;

I think this is very similar to the way of_match_node works.
Find a way to use that instead?

> +	} while (str);
> +
> +	return -EINVAL;
> +}
> +
> +/*
> + * _overlay_mgr_insert()
> + *
> + * Try to request and apply an overlay given a candidate name.
> + */
> +static int _overlay_mgr_apply(struct device *dev, char *candidate)
> +{
> +	struct overlay_mgr_overlay *overlay;
> +	struct device_node *node;
> +	const struct firmware *firmware;
> +	char *firmware_name;
> +	int err = 0;
> +
> +	spin_lock(&overlay_mgr_lock);
> +
> +	list_for_each_entry(overlay, &overlay_mgr_overlays, list) {
> +		if (!strcmp(overlay->name, candidate)) {
> +			dev_err(dev, "overlay already loaded\n");
> +			err = -EEXIST;
> +			goto err_lock;
> +		}
> +	}
> +
> +	overlay = devm_kzalloc(dev, sizeof(*overlay), GFP_KERNEL);
> +	if (!overlay) {
> +		err = -ENOMEM;
> +		goto err_lock;
> +	}
> +

spinlock and possibly sleeping?

> +	overlay->name = candidate;
> +
> +	firmware_name = kasprintf(GFP_KERNEL, "overlay-%s.dtbo", candidate);
> +	if (!firmware_name) {
> +		err = -ENOMEM;
> +		goto err_free;
> +	}
> +
> +	dev_info(dev, "requesting firmware '%s'\n", firmware_name);
> +
> +	err = request_firmware_direct(&firmware, firmware_name, dev);
> +	if (err) {
> +		dev_info(dev, "failed to request firmware '%s'\n",
> +			 firmware_name);
> +		goto err_free;
> +	}
> +

Same here.

> +	of_fdt_unflatten_tree((unsigned long *)firmware->data, NULL, &node);
> +	if (!node) {
> +		dev_err(dev, "failed to unflatted tree\n");
> +		err = -EINVAL;
> +		goto err_fw;
> +	}
> +
> +	of_node_set_flag(node, OF_DETACHED);
> +
> +	err = of_resolve_phandles(node);
> +	if (err) {
> +		dev_err(dev, "failed to resolve phandles: %d\n", err);
> +		goto err_fw;
> +	}
> +
> +	err = overlay_mgr_check_overlay(node);
> +	if (err) {
> +		dev_err(dev, "overlay checks failed: %d\n", err);
> +		goto err_fw;
> +	}
> +
> +	err = of_overlay_create(node);
> +	if (err < 0) {
> +		dev_err(dev, "failed to create overlay: %d\n", err);
> +		goto err_fw;
> +	}
> +
> +	list_add_tail(&overlay->list, &overlay_mgr_overlays);
> +
> +	dev_info(dev, "loaded firmware '%s'\n", firmware_name);
> +
> +	spin_unlock(&overlay_mgr_lock);
> +	return 0;
> +
> +err_fw:
> +	release_firmware(firmware);
> +err_free:
> +	devm_kfree(dev, overlay);
> +err_lock:
> +	spin_unlock(&overlay_mgr_lock);
> +	return err;
> +}
> +
> +int overlay_mgr_apply(struct device *dev, char **candidates, unsigned n)
> +{
> +	int i, ret;
> +
> +	for (i=0; i < n; i++) {
> +		ret = _overlay_mgr_apply(dev, candidates[i]);
> +		if (!ret)
> +			return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(overlay_mgr_apply);
> diff --git a/include/linux/overlay-manager.h b/include/linux/overlay-manager.h
> new file mode 100644
> index 000000000000..8adcc4f5ddf6
> --- /dev/null
> +++ b/include/linux/overlay-manager.h
> @@ -0,0 +1,38 @@
> +/*
> + * Copyright (C) 2016 - Antoine Tenart <antoine.tenart@free-electrons.com>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#ifndef __OVERLAY_MGR_H__
> +#define __OVERLAY_MGR_H__
> +
> +#include <linux/device.h>
> +#include <linux/list.h>
> +#include <linux/sizes.h>
> +
> +#define OVERLAY_MGR_DIP_MAX_SZ		SZ_128
> +
> +struct overlay_mgr_format {
> +	struct list_head list;
> +	char *name;
> +	int (*parse)(struct device *dev, void *data, char ***candidates,
> +		     unsigned *n);
> +};
> +
> +int overlay_mgr_register_format(struct overlay_mgr_format *candidate);
> +int overlay_mgr_parse(struct device *dev, void *data, char ***candidates,
> +		      unsigned *n);
> +int overlay_mgr_apply(struct device *dev, char **candidates, unsigned n);
> +
> +#define dip_convert(field)                                      \
> +        (                                                       \
> +                (sizeof(field) == 1) ? field :                  \
> +                (sizeof(field) == 2) ? be16_to_cpu(field) :     \
> +                (sizeof(field) == 4) ? be32_to_cpu(field) :     \
> +                -1                                              \
> +        )
> +
> +#endif /* __OVERLAY_MGR_H__ */
> -- 
> 2.10.1
> 

[RFC PATCH 2/5] of: overlay-mgr: add the CHIP format

[Antoine Tenart]

Support parsing the header used by capes compatible with Nextthing's
C.H.I.P.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 drivers/of/overlay-manager/Kconfig       | 13 ++++++
 drivers/of/overlay-manager/Makefile      |  1 +
 drivers/of/overlay-manager/format-chip.c | 72 ++++++++++++++++++++++++++++++++
 include/linux/overlay-manager.h          | 13 ++++++
 4 files changed, 99 insertions(+)
 create mode 100644 drivers/of/overlay-manager/format-chip.c

diff --git a/drivers/of/overlay-manager/Kconfig b/drivers/of/overlay-manager/Kconfig
index eeb76054dcb8..1a36613c0c53 100644
--- a/drivers/of/overlay-manager/Kconfig
+++ b/drivers/of/overlay-manager/Kconfig
@@ -4,3 +4,16 @@ config OF_OVERLAY_MGR
 	help
 	  Enable the overlay manager to handle automatic overlay loading when
 	  devices are detected.
+
+if OF_OVERLAY_MGR
+
+menu "Dips header formats"
+
+config OF_OVERLAY_MGR_FORMAT_CHIP
+	bool "Nextthing's C.H.I.P. dip header format"
+	help
+	  Enable Nextthing's C.H.I.P. dip header format support.
+
+endmenu
+
+endif
diff --git a/drivers/of/overlay-manager/Makefile b/drivers/of/overlay-manager/Makefile
index 86d2b53950e7..637cc7ba20c2 100644
--- a/drivers/of/overlay-manager/Makefile
+++ b/drivers/of/overlay-manager/Makefile
@@ -1 +1,2 @@
 obj-$(CONFIG_OF_OVERLAY_MGR)			+= overlay-manager.o
+obj-$(CONFIG_OF_OVERLAY_MGR_FORMAT_CHIP)	+= format-chip.o
diff --git a/drivers/of/overlay-manager/format-chip.c b/drivers/of/overlay-manager/format-chip.c
new file mode 100644
index 000000000000..3a3d315dcb5c
--- /dev/null
+++ b/drivers/of/overlay-manager/format-chip.c
@@ -0,0 +1,72 @@
+/*
+ * Copyright (C) 2016 - Antoine Tenart <antoine.tenart@free-electrons.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/device.h>
+#include <linux/overlay-manager.h>
+#include <linux/slab.h>
+
+#define CAPE_CHIP_MAGIC		0x43484950
+#define CAPE_CHIP_VERSION	1
+#define CAPE_CHIP_CANDIDATES	2
+
+static int cape_chip_parse(struct device *dev, void *data, char ***candidates,
+			   unsigned *n)
+{
+	struct chip_header *header = (struct chip_header *)data;
+	char **tmp;
+	int err;
+
+	if (dip_convert(header->magic) != CAPE_CHIP_MAGIC)
+		return -EINVAL;
+
+	if (dip_convert(header->version) > CAPE_CHIP_VERSION)
+		return -EINVAL;
+
+	tmp = devm_kzalloc(dev, CAPE_CHIP_CANDIDATES * sizeof(char *), GFP_KERNEL);
+	if (!tmp)
+		return -ENOMEM;
+
+	tmp[0] = devm_kasprintf(dev, GFP_KERNEL, "%x-%x-%x",
+				dip_convert(header->vendor_id),
+				dip_convert(header->product_id),
+				dip_convert(header->product_version));
+	if (!tmp[0]) {
+		err = -ENOMEM;
+		goto err_free_list;
+	}
+
+	tmp[1] = devm_kasprintf(dev, GFP_KERNEL, "%x-%x",
+				dip_convert(header->vendor_id),
+				dip_convert(header->product_id));
+	if (!tmp[1]) {
+		err = -ENOMEM;
+		goto err_free_0;
+	}
+
+	*candidates = tmp;
+	*n = CAPE_CHIP_CANDIDATES;
+
+	return 0;
+
+err_free_0:
+	devm_kfree(dev, tmp[0]);
+err_free_list:
+	devm_kfree(dev, tmp);
+	return err;
+}
+
+static struct overlay_mgr_format format_chip = {
+	.name	= "Nextthing C.H.I.P. dip header format",
+	.parse	= &cape_chip_parse,
+};
+
+static int __init cape_chip_init(void)
+{
+	return overlay_mgr_register_format(&format_chip);
+}
+device_initcall(cape_chip_init);
diff --git a/include/linux/overlay-manager.h b/include/linux/overlay-manager.h
index 8adcc4f5ddf6..d76c3c9fd863 100644
--- a/include/linux/overlay-manager.h
+++ b/include/linux/overlay-manager.h
@@ -35,4 +35,17 @@ int overlay_mgr_apply(struct device *dev, char **candidates, unsigned n);
                 -1                                              \
         )
 
+/* Nextthing's C.H.I.P. dip header */
+struct chip_header {
+        u32     magic;                  /* rsvd */
+        u8      version;                /* spec version */
+        u32     vendor_id;
+        u16     product_id;
+        u8      product_version;
+        char    vendor_name[32];
+        char    product_name[32];
+        u8      rsvd[36];               /* rsvd for futre spec versions */
+        u8      data[16];               /* user data, per-cape specific */
+} __packed;
+
 #endif /* __OVERLAY_MGR_H__ */
-- 
2.10.1

[RFC PATCH 3/5] w1: report errors returned by w1_family_notify

[Antoine Tenart]

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 drivers/w1/w1.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/w1/w1.c b/drivers/w1/w1.c
index bb34362e930a..80d0cc4e6e7f 100644
--- a/drivers/w1/w1.c
+++ b/drivers/w1/w1.c
@@ -702,7 +702,9 @@ static int __w1_attach_slave_device(struct w1_slave *sl)
 			dev_name(&sl->dev), err);
 		return err;
 	}
-	w1_family_notify(BUS_NOTIFY_ADD_DEVICE, sl);
+	err = w1_family_notify(BUS_NOTIFY_ADD_DEVICE, sl);
+	if (err)
+		return err;
 
 	dev_set_uevent_suppress(&sl->dev, false);
 	kobject_uevent(&sl->dev.kobj, KOBJ_ADD);
-- 
2.10.1

Re: [RFC PATCH 3/5] w1: report errors returned by w1_family_notify

[Mathieu Poirier]

On 26 October 2016 at 08:57, Antoine Tenart
<antoine.tenart@free-electrons.com> wrote:

GKH won't accept an empty commit log.

> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> ---
>  drivers/w1/w1.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/w1/w1.c b/drivers/w1/w1.c
> index bb34362e930a..80d0cc4e6e7f 100644
> --- a/drivers/w1/w1.c
> +++ b/drivers/w1/w1.c
> @@ -702,7 +702,9 @@ static int __w1_attach_slave_device(struct w1_slave *sl)
>                         dev_name(&sl->dev), err);
>                 return err;
>         }
> -       w1_family_notify(BUS_NOTIFY_ADD_DEVICE, sl);
> +       err = w1_family_notify(BUS_NOTIFY_ADD_DEVICE, sl);
> +       if (err)
> +               return err;
>
>         dev_set_uevent_suppress(&sl->dev, false);
>         kobject_uevent(&sl->dev.kobj, KOBJ_ADD);
> --
> 2.10.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH 4/5] w1: add a callback to call slave when a new device is connected

[Antoine Tenart]

This patch adds the possibility for slave drivers to register a
callback, to be called whenever a new device matching the slave ID
is connected.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 drivers/w1/w1.c        | 10 ++++++++++
 drivers/w1/w1_family.h |  2 ++
 2 files changed, 12 insertions(+)

diff --git a/drivers/w1/w1.c b/drivers/w1/w1.c
index 80d0cc4e6e7f..7010ffd1ea93 100644
--- a/drivers/w1/w1.c
+++ b/drivers/w1/w1.c
@@ -659,6 +659,16 @@ static int w1_family_notify(unsigned long action, struct w1_slave *sl)
 				return err;
 			}
 		}
+		if (fops->callback) {
+			err = fops->callback(sl);
+			/*
+			 * Do not return an error as the slave driver correctly
+			 * probed.
+			 */
+			if (err)
+				dev_err(&sl->dev,
+					"callback call failed. err=%d\n", err);
+		}
 
 		break;
 	case BUS_NOTIFY_DEL_DEVICE:
diff --git a/drivers/w1/w1_family.h b/drivers/w1/w1_family.h
index 10a7a0767187..5e165babc6f3 100644
--- a/drivers/w1/w1_family.h
+++ b/drivers/w1/w1_family.h
@@ -55,11 +55,13 @@ struct w1_slave;
  * @add_slave: add_slave
  * @remove_slave: remove_slave
  * @groups: sysfs group
+ * @callback: called when a new device is discovered
  */
 struct w1_family_ops
 {
 	int  (* add_slave)(struct w1_slave *);
 	void (* remove_slave)(struct w1_slave *);
+	int  (* callback)(struct w1_slave *);
 	const struct attribute_group **groups;
 };
 
-- 
2.10.1

Re: [RFC PATCH 4/5] w1: add a callback to call slave when a new device is connected

[Mathieu Poirier]

On 26 October 2016 at 08:57, Antoine Tenart
<antoine.tenart@free-electrons.com> wrote:
> This patch adds the possibility for slave drivers to register a
> callback, to be called whenever a new device matching the slave ID
> is connected.
>
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> ---
>  drivers/w1/w1.c        | 10 ++++++++++
>  drivers/w1/w1_family.h |  2 ++
>  2 files changed, 12 insertions(+)
>
> diff --git a/drivers/w1/w1.c b/drivers/w1/w1.c
> index 80d0cc4e6e7f..7010ffd1ea93 100644
> --- a/drivers/w1/w1.c
> +++ b/drivers/w1/w1.c
> @@ -659,6 +659,16 @@ static int w1_family_notify(unsigned long action, struct w1_slave *sl)
>                                 return err;
>                         }
>                 }
> +               if (fops->callback) {
> +                       err = fops->callback(sl);
> +                       /*
> +                        * Do not return an error as the slave driver correctly
> +                        * probed.
> +                        */

I don't get this part.  What's the point of calling a callback if a
failure is not important - maybe I'm just missing something.

> +                       if (err)
> +                               dev_err(&sl->dev,
> +                                       "callback call failed. err=%d\n", err);
> +               }
>
>                 break;
>         case BUS_NOTIFY_DEL_DEVICE:
> diff --git a/drivers/w1/w1_family.h b/drivers/w1/w1_family.h
> index 10a7a0767187..5e165babc6f3 100644
> --- a/drivers/w1/w1_family.h
> +++ b/drivers/w1/w1_family.h
> @@ -55,11 +55,13 @@ struct w1_slave;
>   * @add_slave: add_slave
>   * @remove_slave: remove_slave
>   * @groups: sysfs group
> + * @callback: called when a new device is discovered
>   */
>  struct w1_family_ops
>  {
>         int  (* add_slave)(struct w1_slave *);
>         void (* remove_slave)(struct w1_slave *);
> +       int  (* callback)(struct w1_slave *);
>         const struct attribute_group **groups;
>  };
>
> --
> 2.10.1
>

Re: [RFC PATCH 4/5] w1: add a callback to call slave when a new device is connected

[Antoine Tenart]

[-- Attachment #1: Type: text/plain, Size: 1310 bytes --]

Hello Mathieu,

On Wed, Oct 26, 2016 at 10:42:28AM -0600, Mathieu Poirier wrote:
> On 26 October 2016 at 08:57, Antoine Tenart
> <antoine.tenart@free-electrons.com> wrote:
> >                 }
> > +               if (fops->callback) {
> > +                       err = fops->callback(sl);
> > +                       /*
> > +                        * Do not return an error as the slave driver correctly
> > +                        * probed.
> > +                        */
> 
> I don't get this part.  What's the point of calling a callback if a
> failure is not important - maybe I'm just missing something.
> 
> > +                       if (err)
> > +                               dev_err(&sl->dev,
> > +                                       "callback call failed. err=%d\n", err);
> > +               }

In our case it can be not that important: if we fail to apply an
overlay, we can still use the w1 interfaces to access the eeprom.

Anyway, all those errors weren't taken into account by the w1 framework
before (see my other patch). Also, the w1 patches are given for the
example and could be improved. Part of the reason is the w1 framework
itself :-)

Antoine

-- 
Antoine Ténart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[RFC PATCH 5/5] of: overlay-mgr: add a detector for headers stored on a ds2431 eeprom over w1

[Antoine Tenart]

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 drivers/of/overlay-manager/Kconfig | 10 ++++++++++
 drivers/w1/slaves/w1_ds2431.c      | 39 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+)

diff --git a/drivers/of/overlay-manager/Kconfig b/drivers/of/overlay-manager/Kconfig
index 1a36613c0c53..ad0a5b8e9e5e 100644
--- a/drivers/of/overlay-manager/Kconfig
+++ b/drivers/of/overlay-manager/Kconfig
@@ -16,4 +16,14 @@ config OF_OVERLAY_MGR_FORMAT_CHIP
 
 endmenu
 
+menu "Overlay Manager detectors"
+
+config OF_OVERLAY_MGR_DETECTOR_DS2431
+	bool "Dip header on a DS2431 EEPROM"
+	depends on W1_SLAVE_DS2431
+	help
+	  Enable dip header DS2431 EEPROM support.
+
+endmenu
+
 endif
diff --git a/drivers/w1/slaves/w1_ds2431.c b/drivers/w1/slaves/w1_ds2431.c
index 80572cb63ba8..760325f9a2bd 100644
--- a/drivers/w1/slaves/w1_ds2431.c
+++ b/drivers/w1/slaves/w1_ds2431.c
@@ -15,6 +15,9 @@
 #include <linux/device.h>
 #include <linux/types.h>
 #include <linux/delay.h>
+#include <linux/slab.h>
+
+#include <linux/overlay-manager.h>
 
 #include "../w1.h"
 #include "../w1_int.h"
@@ -280,7 +283,43 @@ static const struct attribute_group *w1_f2d_groups[] = {
 	NULL,
 };
 
+#if IS_ENABLED(CONFIG_OF_OVERLAY_MGR_DETECTOR_DS2431)
+static int chip_dip_callback(struct w1_slave *sl)
+{
+	char **candidates = NULL;
+	int i, n, err = 0;
+	u8 *data;
+
+	data = kzalloc(OVERLAY_MGR_DIP_MAX_SZ, GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	/* sizeof(struct chip_header) is a mulitple of 8 */
+	for (i = 0; i < OVERLAY_MGR_DIP_MAX_SZ; i += 8) {
+		if (w1_f2d_readblock(sl, i, 8, &data[i])) {
+			err = -EIO;
+			goto end;
+		}
+	}
+
+	overlay_mgr_parse(&sl->dev, data, &candidates, &n);
+	if (!n) {
+		err = -EINVAL;
+		goto end;
+	}
+
+	err = overlay_mgr_apply(&sl->dev, candidates, n);
+
+end:
+	kfree(data);
+	return err;
+}
+#endif
+
 static struct w1_family_ops w1_f2d_fops = {
+#if IS_ENABLED(CONFIG_OF_OVERLAY_MGR_DETECTOR_DS2431)
+	.callback	= chip_dip_callback,
+#endif
 	.groups		= w1_f2d_groups,
 };
 
-- 
2.10.1

Re: [RFC PATCH 5/5] of: overlay-mgr: add a detector for headers stored on a ds2431 eeprom over w1

[Matthias Brugger]

On 10/26/2016 04:57 PM, Antoine Tenart wrote:
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> ---

Please provide a commit message.

>  drivers/of/overlay-manager/Kconfig | 10 ++++++++++
>  drivers/w1/slaves/w1_ds2431.c      | 39 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 49 insertions(+)
>
> diff --git a/drivers/of/overlay-manager/Kconfig b/drivers/of/overlay-manager/Kconfig
> index 1a36613c0c53..ad0a5b8e9e5e 100644
> --- a/drivers/of/overlay-manager/Kconfig
> +++ b/drivers/of/overlay-manager/Kconfig
> @@ -16,4 +16,14 @@ config OF_OVERLAY_MGR_FORMAT_CHIP
>
>  endmenu
>
> +menu "Overlay Manager detectors"
> +
> +config OF_OVERLAY_MGR_DETECTOR_DS2431
> +	bool "Dip header on a DS2431 EEPROM"
> +	depends on W1_SLAVE_DS2431
> +	help
> +	  Enable dip header DS2431 EEPROM support.
> +
> +endmenu
> +
>  endif
> diff --git a/drivers/w1/slaves/w1_ds2431.c b/drivers/w1/slaves/w1_ds2431.c
> index 80572cb63ba8..760325f9a2bd 100644
> --- a/drivers/w1/slaves/w1_ds2431.c
> +++ b/drivers/w1/slaves/w1_ds2431.c
> @@ -15,6 +15,9 @@
>  #include <linux/device.h>
>  #include <linux/types.h>
>  #include <linux/delay.h>
> +#include <linux/slab.h>
> +
> +#include <linux/overlay-manager.h>
>
>  #include "../w1.h"
>  #include "../w1_int.h"
> @@ -280,7 +283,43 @@ static const struct attribute_group *w1_f2d_groups[] = {
>  	NULL,
>  };
>
> +#if IS_ENABLED(CONFIG_OF_OVERLAY_MGR_DETECTOR_DS2431)
> +static int chip_dip_callback(struct w1_slave *sl)
> +{
> +	char **candidates = NULL;
> +	int i, n, err = 0;
> +	u8 *data;
> +
> +	data = kzalloc(OVERLAY_MGR_DIP_MAX_SZ, GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	/* sizeof(struct chip_header) is a mulitple of 8 */
> +	for (i = 0; i < OVERLAY_MGR_DIP_MAX_SZ; i += 8) {
> +		if (w1_f2d_readblock(sl, i, 8, &data[i])) {
> +			err = -EIO;
> +			goto end;
> +		}
> +	}
> +
> +	overlay_mgr_parse(&sl->dev, data, &candidates, &n);
> +	if (!n) {
> +		err = -EINVAL;
> +		goto end;
> +	}
> +
> +	err = overlay_mgr_apply(&sl->dev, candidates, n);
> +
> +end:
> +	kfree(data);
> +	return err;
> +}
> +#endif
> +
>  static struct w1_family_ops w1_f2d_fops = {
> +#if IS_ENABLED(CONFIG_OF_OVERLAY_MGR_DETECTOR_DS2431)
> +	.callback	= chip_dip_callback,
> +#endif
>  	.groups		= w1_f2d_groups,
>  };
>
>

Re: [RFC PATCH 5/5] of: overlay-mgr: add a detector for headers stored on a ds2431 eeprom over w1

[Matthias Brugger]

On 10/26/2016 04:57 PM, Antoine Tenart wrote:
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> ---

Please provide a commit message.

Thanks,
Matthias

Re: [RFC PATCH 5/5] of: overlay-mgr: add a detector for headers stored on a ds2431 eeprom over w1

[Antoine Tenart]

[-- Attachment #1: Type: text/plain, Size: 551 bytes --]

Hello Matthias,

On Thu, Oct 27, 2016 at 11:19:14AM +0200, Matthias Brugger wrote:
> On 10/26/2016 04:57 PM, Antoine Tenart wrote:
> > Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> > ---
> 
> Please provide a commit message.

Sure. There are other modifications I'd like to do in the series if it
happens to be an use case for people. This patch is given as an example
of how we could implement this.

Antoine

-- 
Antoine Ténart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [RFC PATCH 0/5] Add an overlay manager to handle board capes

[Rob Herring]

Please Cc the maintainers of drivers/of/.

+ Frank R, Hans, Dmitry S

On Wed, Oct 26, 2016 at 9:57 AM, Antoine Tenart
<antoine.tenart@free-electrons.com> wrote:
> Hi all,
>
> Many boards now come with dips and compatible capes; among others the
> C.H.I.P, or Beaglebones. All these boards have a kernel implementing an
> out-of-tree "cape manager" which is used to detected capes, retrieve
> their description and apply a corresponding overlay. This series is an
> attempt to start a discussion, with an implementation of such a manager
> which is somehow generic (i.e. formats or cape detectors can be added).
> Other use cases could make use of this manager to dynamically load dt
> overlays based on some input / hw presence.

I'd like to see an input source be the kernel command line and/or a DT
chosen property. Another overlay manager was proposed not to long
ago[1] as well. There's also the Allwinner tablet use case from Hans
where i2c devices are probed and detected. That's not using overlays
currently, but maybe could.

Another thing to consider is different sources of overlays. Besides in
the filesystem, overlays could be built into the kernel (already
supported), embedded in the dtb (as the other overlay mgr did) or we
could extend FDT format to append them.

> The proposed design is a library which can be used by detector drivers
> to parse headers and load the corresponding overlay. Helpers are
> provided for this purpose. The whole thing is divided into 3 entities:
>
> - The parser which is project-specific (to allow supporting headers
>   already into the wild). It registers a function parsing an header's
>   data and filling one or more strings which will be used to find
>   matching dtbo on the fs.
>
> - The overlay manager helpers allowing to parse a header to retrieve
>   the previously mentioned strings and to load a compatible overlay.
>
> - The detectors which are used to detect capes and get their description
>   (to be parsed).

What about things like power has to be turned on first to detect
boards and read their ID? I think this needs to be tied into the
driver model. Though, don't go sticking cape mgr nodes into DT. Maybe
a driver gets bound to a connector node, but we've got to sort out
connector bindings first.

> An example of parser and detector is given, compatible with what's done
> for the C.H.I.P. As the w1 framework is really bad (and we should
> probably do something about that) the detector code is far from being
> perfect; but that's not related to what we try to achieve here.
>
> The actual implementation has a limitation: the detectors cannot be
> built-in the kernel image as they would likely detect capes at boot time
> but will fail to get their corresponding dt overlays as the fs isn't
> mounted yet. The only case this can work is when dt overlays are
> built-in firmwares. This isn't an issue for the C.H.I.P. use case right
> now. There was a discussion about making an helper to wait for the
> rootfs to be mount but the answer was "this is the driver's problem".

I thought there are firmware loading calls that will wait. I think
this all needs to work asynchronously both for firmware loading and
because w1 is really slow.

> I'd like to get comments, specifically from people using custom cape
> managers, to see if this could fill their needs (with I guess some
> modifications).

Having 2 would certainly give a better sense this is generic.

Rob

[1] https://patchwork.ozlabs.org/patch/667805/

Re: [RFC PATCH 0/5] Add an overlay manager to handle board capes

[Antoine Tenart]

[-- Attachment #1: Type: text/plain, Size: 4193 bytes --]

Hi Rob,

On Thu, Oct 27, 2016 at 08:41:56AM -0500, Rob Herring wrote:
> Please Cc the maintainers of drivers/of/.
> 
> + Frank R, Hans, Dmitry S

Yes, sorry for that.

> On Wed, Oct 26, 2016 at 9:57 AM, Antoine Tenart
> <antoine.tenart@free-electrons.com> wrote:
> >
> > Many boards now come with dips and compatible capes; among others the
> > C.H.I.P, or Beaglebones. All these boards have a kernel implementing an
> > out-of-tree "cape manager" which is used to detected capes, retrieve
> > their description and apply a corresponding overlay. This series is an
> > attempt to start a discussion, with an implementation of such a manager
> > which is somehow generic (i.e. formats or cape detectors can be added).
> > Other use cases could make use of this manager to dynamically load dt
> > overlays based on some input / hw presence.
> 
> I'd like to see an input source be the kernel command line and/or a DT
> chosen property.

We now have overlay support in U-Boot so we could modify the device tree
from the bootloader and not use the command line. But you can argue the
boot loader can't always be upgraded (to support overlays, or to improve
it). So I guess we can think of using the command line as a input
source.

> Another overlay manager was proposed not to long ago[1] as well.

Thanks for the hint.

> Another thing to consider is different sources of overlays. Besides in
> the filesystem, overlays could be built into the kernel (already
> supported), embedded in the dtb (as the other overlay mgr did) or we
> could extend FDT format to append them.

Sure. Using this series it should be quite easy to support other
sources. We would need to improve the function loading the overlay, to
try other sources. This could even comes in following up patches.

> > The proposed design is a library which can be used by detector drivers
> > to parse headers and load the corresponding overlay. Helpers are
> > provided for this purpose. The whole thing is divided into 3 entities:
> >
> > - The parser which is project-specific (to allow supporting headers
> >   already into the wild). It registers a function parsing an header's
> >   data and filling one or more strings which will be used to find
> >   matching dtbo on the fs.
> >
> > - The overlay manager helpers allowing to parse a header to retrieve
> >   the previously mentioned strings and to load a compatible overlay.
> >
> > - The detectors which are used to detect capes and get their description
> >   (to be parsed).
> 
> What about things like power has to be turned on first to detect
> boards and read their ID? I think this needs to be tied into the
> driver model. Though, don't go sticking cape mgr nodes into DT. Maybe
> a driver gets bound to a connector node, but we've got to sort out
> connector bindings first.

Right. I don't know yet how to handle this. Do you have an existing
example in mind of such a power requirement?

> > An example of parser and detector is given, compatible with what's done
> > for the C.H.I.P. As the w1 framework is really bad (and we should
> > probably do something about that) the detector code is far from being
> > perfect; but that's not related to what we try to achieve here.
> >
> > The actual implementation has a limitation: the detectors cannot be
> > built-in the kernel image as they would likely detect capes at boot time
> > but will fail to get their corresponding dt overlays as the fs isn't
> > mounted yet. The only case this can work is when dt overlays are
> > built-in firmwares. This isn't an issue for the C.H.I.P. use case right
> > now. There was a discussion about making an helper to wait for the
> > rootfs to be mount but the answer was "this is the driver's problem".
> 
> I thought there are firmware loading calls that will wait. I think
> this all needs to work asynchronously both for firmware loading and
> because w1 is really slow.

There is an asynchronous one, but it will also fail if the rootfs isn't
mounted yet.

Thanks!

Antoine

-- 
Antoine Ténart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [RFC PATCH 0/5] Add an overlay manager to handle board capes

[Hans de Goede]

Hi,

On 27-10-16 15:41, Rob Herring wrote:
> Please Cc the maintainers of drivers/of/.
>
> + Frank R, Hans, Dmitry S
>
> On Wed, Oct 26, 2016 at 9:57 AM, Antoine Tenart
> <antoine.tenart@free-electrons.com> wrote:
>> Hi all,
>>
>> Many boards now come with dips and compatible capes; among others the
>> C.H.I.P, or Beaglebones. All these boards have a kernel implementing an
>> out-of-tree "cape manager" which is used to detected capes, retrieve
>> their description and apply a corresponding overlay. This series is an
>> attempt to start a discussion, with an implementation of such a manager
>> which is somehow generic (i.e. formats or cape detectors can be added).
>> Other use cases could make use of this manager to dynamically load dt
>> overlays based on some input / hw presence.
>
> I'd like to see an input source be the kernel command line and/or a DT
> chosen property. Another overlay manager was proposed not to long
> ago[1] as well. There's also the Allwinner tablet use case from Hans
> where i2c devices are probed and detected. That's not using overlays
> currently, but maybe could.

Actually I'm currently thinking in a different direction, which I
think will be good for the boards where some ICs are frequently
replaced by 2nd (and 3th and 4th) sources, rather then that we're
dealing with an extension connector with capes / daughter boards.

Although there is some overlap I'm starting to think that we need to
treat these 2 cases differently. Let me quickly copy and paste
the basic idea I've for the 2nd source touchscreen / accelerometer
chip case:

"""
The kernel actually already has a detect() method in struct i2c_driver,
we could use that (we would need to implement it in drivers which do not
have it yet). Note on second thought it seems it may be better to use
probe() for this, see below.

Then we could have something like this in dt:

&i2c0 {
     touchscreen1: gsl1680@40 {
         reg = <0x40>;
         compatible = "silead,gsl1680";
         enable-gpios = <&pio 7 1 GPIO_ACTIVE_HIGH>; /* PH1 */
         status = "disabled";
     };

     touchscreen2: ektf2127@15 {
         reg = <0x15>;
         compatible = "elan,ektf2127";
         enable-gpios = <&pio 7 1 GPIO_ACTIVE_HIGH>; /* PH1 */
         status = "disabled";
     };

     i2c-probe-stop-at-first-match-0 = <&touchscreen1>, <&touchscreen2>;
     i2c-probe-stop-at-first-match-1 = <&accelerometer1>, <&accelerometer2>;
}

Which would make the i2c subsys call detect (*) on each device, until
a device is found. Likewise we could have a "i2c-probe-all" property
which also walks a list of phandles but does not stop on the first
match.

...

*) Yes this sounds Linux specific, but it really is just "execute to-be-probed
device compatible specific detection method"
"""

This does not 100% solve all q8 issues (see the "Add Allwinner Q8 tablets
hardware manager" thread), but does solve quite a bit of the use-case
and this matches what many vendor os-images (typically android) are
actually doing for these kind of boards.

As for the bits this does not solve, those are mostly board specific details
which cannot be probed at all, and on x86 are typically solved in the device
driver by doing a dmi check to identify the board and then apply a board
specific workaround in the driver.

I've come to believe that we should similarly delegate dealing this to device
drivers in the devicetree case. Note that dt should still of course fully
describe the hardware for normal hardware, the driver would just need to care
about weird board quirks in certain exceptions.

A more interesting problem here is that dt does not have something like
DMI, there is the machine compatible, but that typically does not contain
board revision info (where as DMI often does). I believe that this is
actually something which should be fixed at the bootloader level
have it prepend a new machine compatible which contains revision info.

Hmm, if we make the bootloader prepend a new machine compatible which contains
revision info, we could then trigger quirks on this and in some cases avoid
the need for dealing with board quirks in the driver ...

Note this is all very specific to dealing with board (revision) variants,
for add-ons having the bootloader add info to the machine compatible does
not seem the right solution.

Regards,

Hans




>
> Another thing to consider is different sources of overlays. Besides in
> the filesystem, overlays could be built into the kernel (already
> supported), embedded in the dtb (as the other overlay mgr did) or we
> could extend FDT format to append them.
>
>> The proposed design is a library which can be used by detector drivers
>> to parse headers and load the corresponding overlay. Helpers are
>> provided for this purpose. The whole thing is divided into 3 entities:
>>
>> - The parser which is project-specific (to allow supporting headers
>>   already into the wild). It registers a function parsing an header's
>>   data and filling one or more strings which will be used to find
>>   matching dtbo on the fs.
>>
>> - The overlay manager helpers allowing to parse a header to retrieve
>>   the previously mentioned strings and to load a compatible overlay.
>>
>> - The detectors which are used to detect capes and get their description
>>   (to be parsed).
>
> What about things like power has to be turned on first to detect
> boards and read their ID? I think this needs to be tied into the
> driver model. Though, don't go sticking cape mgr nodes into DT. Maybe
> a driver gets bound to a connector node, but we've got to sort out
> connector bindings first.
>
>> An example of parser and detector is given, compatible with what's done
>> for the C.H.I.P. As the w1 framework is really bad (and we should
>> probably do something about that) the detector code is far from being
>> perfect; but that's not related to what we try to achieve here.
>>
>> The actual implementation has a limitation: the detectors cannot be
>> built-in the kernel image as they would likely detect capes at boot time
>> but will fail to get their corresponding dt overlays as the fs isn't
>> mounted yet. The only case this can work is when dt overlays are
>> built-in firmwares. This isn't an issue for the C.H.I.P. use case right
>> now. There was a discussion about making an helper to wait for the
>> rootfs to be mount but the answer was "this is the driver's problem".
>
> I thought there are firmware loading calls that will wait. I think
> this all needs to work asynchronously both for firmware loading and
> because w1 is really slow.
>
>> I'd like to get comments, specifically from people using custom cape
>> managers, to see if this could fill their needs (with I guess some
>> modifications).
>
> Having 2 would certainly give a better sense this is generic.
>
> Rob
>
> [1] https://patchwork.ozlabs.org/patch/667805/
>

Re: [RFC PATCH 0/5] Add an overlay manager to handle board capes

[Rob Herring]

On Thu, Oct 27, 2016 at 10:13 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 27-10-16 15:41, Rob Herring wrote:
>>
>> Please Cc the maintainers of drivers/of/.
>>
>> + Frank R, Hans, Dmitry S
>>
>> On Wed, Oct 26, 2016 at 9:57 AM, Antoine Tenart
>> <antoine.tenart@free-electrons.com> wrote:
>>>
>>> Hi all,
>>>
>>> Many boards now come with dips and compatible capes; among others the
>>> C.H.I.P, or Beaglebones. All these boards have a kernel implementing an
>>> out-of-tree "cape manager" which is used to detected capes, retrieve
>>> their description and apply a corresponding overlay. This series is an
>>> attempt to start a discussion, with an implementation of such a manager
>>> which is somehow generic (i.e. formats or cape detectors can be added).
>>> Other use cases could make use of this manager to dynamically load dt
>>> overlays based on some input / hw presence.
>>
>>
>> I'd like to see an input source be the kernel command line and/or a DT
>> chosen property. Another overlay manager was proposed not to long
>> ago[1] as well. There's also the Allwinner tablet use case from Hans
>> where i2c devices are probed and detected. That's not using overlays
>> currently, but maybe could.
>
>
> Actually I'm currently thinking in a different direction, which I
> think will be good for the boards where some ICs are frequently
> replaced by 2nd (and 3th and 4th) sources, rather then that we're
> dealing with an extension connector with capes / daughter boards.
>
> Although there is some overlap I'm starting to think that we need to
> treat these 2 cases differently. Let me quickly copy and paste
> the basic idea I've for the 2nd source touchscreen / accelerometer
> chip case:
>
> """
> The kernel actually already has a detect() method in struct i2c_driver,
> we could use that (we would need to implement it in drivers which do not
> have it yet). Note on second thought it seems it may be better to use
> probe() for this, see below.
>
> Then we could have something like this in dt:
>
> &i2c0 {
>     touchscreen1: gsl1680@40 {
>         reg = <0x40>;
>         compatible = "silead,gsl1680";
>         enable-gpios = <&pio 7 1 GPIO_ACTIVE_HIGH>; /* PH1 */
>         status = "disabled";
>     };
>
>     touchscreen2: ektf2127@15 {
>         reg = <0x15>;

Do you ever have different devices with the same address? That would
be somewhat problematic as really these should be
"touchscreen@<addr>".

>         compatible = "elan,ektf2127";
>         enable-gpios = <&pio 7 1 GPIO_ACTIVE_HIGH>; /* PH1 */
>         status = "disabled";
>     };
>
>     i2c-probe-stop-at-first-match-0 = <&touchscreen1>, <&touchscreen2>;
>     i2c-probe-stop-at-first-match-1 = <&accelerometer1>, <&accelerometer2>;
> }
>
> Which would make the i2c subsys call detect (*) on each device, until
> a device is found. Likewise we could have a "i2c-probe-all" property
> which also walks a list of phandles but does not stop on the first
> match.
>
> ...
>
> *) Yes this sounds Linux specific, but it really is just "execute
> to-be-probed
> device compatible specific detection method"
> """

Yeah, not a fan of these properties at first glance. Why can't you
just fail probe on the non-existent devices?


> This does not 100% solve all q8 issues (see the "Add Allwinner Q8 tablets
> hardware manager" thread), but does solve quite a bit of the use-case
> and this matches what many vendor os-images (typically android) are
> actually doing for these kind of boards.

BTW, I've been meaning to ask you if you are looking at the Android
side of things as well?

> As for the bits this does not solve, those are mostly board specific details
> which cannot be probed at all, and on x86 are typically solved in the device
> driver by doing a dmi check to identify the board and then apply a board
> specific workaround in the driver.
>
> I've come to believe that we should similarly delegate dealing this to
> device
> drivers in the devicetree case. Note that dt should still of course fully
> describe the hardware for normal hardware, the driver would just need to
> care
> about weird board quirks in certain exceptions.

Which is fine IMO, though I do think we should look at those cases
carefully to ensure they stay the exception.

> A more interesting problem here is that dt does not have something like
> DMI, there is the machine compatible, but that typically does not contain
> board revision info (where as DMI often does). I believe that this is
> actually something which should be fixed at the bootloader level
> have it prepend a new machine compatible which contains revision info.
>
> Hmm, if we make the bootloader prepend a new machine compatible which
> contains
> revision info, we could then trigger quirks on this and in some cases avoid
> the need for dealing with board quirks in the driver ...

That would work. Board and chip versions both need better handling in
kernel IMO.

QCom has a whole scheme around version numbering in compatible
strings. (Unfortunately, bootloaders only support their previous way
of doing things.)

> Note this is all very specific to dealing with board (revision) variants,
> for add-ons having the bootloader add info to the machine compatible does
> not seem the right solution.

Agreed.

Rob

Re: [RFC PATCH 0/5] Add an overlay manager to handle board capes

[Hans de Goede]

Hi,

On 27-10-16 19:30, Rob Herring wrote:
> On Thu, Oct 27, 2016 at 10:13 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 27-10-16 15:41, Rob Herring wrote:
>>>
>>> Please Cc the maintainers of drivers/of/.
>>>
>>> + Frank R, Hans, Dmitry S
>>>
>>> On Wed, Oct 26, 2016 at 9:57 AM, Antoine Tenart
>>> <antoine.tenart@free-electrons.com> wrote:
>>>>
>>>> Hi all,
>>>>
>>>> Many boards now come with dips and compatible capes; among others the
>>>> C.H.I.P, or Beaglebones. All these boards have a kernel implementing an
>>>> out-of-tree "cape manager" which is used to detected capes, retrieve
>>>> their description and apply a corresponding overlay. This series is an
>>>> attempt to start a discussion, with an implementation of such a manager
>>>> which is somehow generic (i.e. formats or cape detectors can be added).
>>>> Other use cases could make use of this manager to dynamically load dt
>>>> overlays based on some input / hw presence.
>>>
>>>
>>> I'd like to see an input source be the kernel command line and/or a DT
>>> chosen property. Another overlay manager was proposed not to long
>>> ago[1] as well. There's also the Allwinner tablet use case from Hans
>>> where i2c devices are probed and detected. That's not using overlays
>>> currently, but maybe could.
>>
>>
>> Actually I'm currently thinking in a different direction, which I
>> think will be good for the boards where some ICs are frequently
>> replaced by 2nd (and 3th and 4th) sources, rather then that we're
>> dealing with an extension connector with capes / daughter boards.
>>
>> Although there is some overlap I'm starting to think that we need to
>> treat these 2 cases differently. Let me quickly copy and paste
>> the basic idea I've for the 2nd source touchscreen / accelerometer
>> chip case:
>>
>> """
>> The kernel actually already has a detect() method in struct i2c_driver,
>> we could use that (we would need to implement it in drivers which do not
>> have it yet). Note on second thought it seems it may be better to use
>> probe() for this, see below.
>>
>> Then we could have something like this in dt:
>>
>> &i2c0 {
>>     touchscreen1: gsl1680@40 {
>>         reg = <0x40>;
>>         compatible = "silead,gsl1680";
>>         enable-gpios = <&pio 7 1 GPIO_ACTIVE_HIGH>; /* PH1 */
>>         status = "disabled";
>>     };
>>
>>     touchscreen2: ektf2127@15 {
>>         reg = <0x15>;
>
> Do you ever have different devices with the same address? That would
> be somewhat problematic as really these should be
> "touchscreen@<addr>".

Yes that happens (sometimes).

>
>>         compatible = "elan,ektf2127";
>>         enable-gpios = <&pio 7 1 GPIO_ACTIVE_HIGH>; /* PH1 */
>>         status = "disabled";
>>     };
>>
>>     i2c-probe-stop-at-first-match-0 = <&touchscreen1>, <&touchscreen2>;
>>     i2c-probe-stop-at-first-match-1 = <&accelerometer1>, <&accelerometer2>;
>> }
>>
>> Which would make the i2c subsys call detect (*) on each device, until
>> a device is found. Likewise we could have a "i2c-probe-all" property
>> which also walks a list of phandles but does not stop on the first
>> match.
>>
>> ...
>>
>> *) Yes this sounds Linux specific, but it really is just "execute
>> to-be-probed
>> device compatible specific detection method"
>> """
>
> Yeah, not a fan of these properties at first glance. Why can't you
> just fail probe on the non-existent devices?

That is possible and in the other thread on this there are some
links to some boards which actually already do this, but from a dt
pov it feels wrong. If we know only one of a set of options will
ever be there we ought to describe things like this in the dt.

Functionality wise this has 2 advantages:
1) We stop probing needlessly once a device is found, in some
cases the majority of the board variants has dev a, and some
have dev b / c. Then putting a first in the to-probe list will
save probing b / c on most boards.

2) Not all i2c chips are easily identifiable, so in some cases
one may want to put dev x as last to probe, because the
probe solely consists of: "Does something ack i2c transfers
at this address".

>> This does not 100% solve all q8 issues (see the "Add Allwinner Q8 tablets
>> hardware manager" thread), but does solve quite a bit of the use-case
>> and this matches what many vendor os-images (typically android) are
>> actually doing for these kind of boards.
>
> BTW, I've been meaning to ask you if you are looking at the Android
> side of things as well?

No, I purely use android os images / SDKs as a source of how the
hw works, I do not have any intentions to try and get android up
and running with mainline on these boards.

>> As for the bits this does not solve, those are mostly board specific details
>> which cannot be probed at all, and on x86 are typically solved in the device
>> driver by doing a dmi check to identify the board and then apply a board
>> specific workaround in the driver.
>>
>> I've come to believe that we should similarly delegate dealing this to
>> device
>> drivers in the devicetree case. Note that dt should still of course fully
>> describe the hardware for normal hardware, the driver would just need to
>> care
>> about weird board quirks in certain exceptions.
>
> Which is fine IMO, though I do think we should look at those cases
> carefully to ensure they stay the exception.

Ack.

>> A more interesting problem here is that dt does not have something like
>> DMI, there is the machine compatible, but that typically does not contain
>> board revision info (where as DMI often does). I believe that this is
>> actually something which should be fixed at the bootloader level
>> have it prepend a new machine compatible which contains revision info.
>>
>> Hmm, if we make the bootloader prepend a new machine compatible which
>> contains
>> revision info, we could then trigger quirks on this and in some cases avoid
>> the need for dealing with board quirks in the driver ...
>
> That would work. Board and chip versions both need better handling in
> kernel IMO.
>
> QCom has a whole scheme around version numbering in compatible
> strings. (Unfortunately, bootloaders only support their previous way
> of doing things.)
>
>> Note this is all very specific to dealing with board (revision) variants,
>> for add-ons having the bootloader add info to the machine compatible does
>> not seem the right solution.
>
> Agreed.

Regards,

Hans