[PATCH v4 00/16] R-Car DU: Convert LVDS code to bridge driver

[PATCH v4 00/16] R-Car DU: Convert LVDS code to bridge driver

[Laurent Pinchart]

Hello,

This patch series addresses a design mistake that dates back from the initial
DU support. Support for the LVDS encoders, which are IP cores separate from
the DU, was bundled in the DU driver. Worse, both the DU and LVDS were
described through a single DT node.

To fix the, patches 01/16 and 02/16 define new DT bindings for the LVDS
encoders, and deprecate their description inside the DU bindings. To retain
backward compatibility with existing DT, patches 03/16 to 08/16 then patch the
device tree at runtime to convert the legacy bindings to the new ones.

With the DT side addressed, patch 09/16 converts the LVDS support code to a
separate bridge driver. Patches 11/16 to 16/16 then update all the device tree
sources to the new DU and LVDS encoders bindings.

I decided to go for live DT patching in patch 08/16 because implementing
support for both the legacy and new bindings in the driver would have been
very intrusive, and prevented further cleanups. This version relies more
heavily on overlays to avoid touching the internals of the OF core compared to
v2, even if manual fixes to the device tree are still needed.

Compared to v3, this series uses the OF changeset API to update properties
instead of accessing the internals of the property structure. This removes the
local implementation of functions to look up nodes by path and update
properties. In order to do this, I pulled in Pantelis' patch series titled
"[PATCH v2 0/5] of: dynamic: Changesets helpers & fixes" at Rob's request, and
rebased it while taking two small review comments into account.

Rob, I'd like this series to be merged in v4.17. As the changeset helpers are
now a dependency, I'd need you to merge them early (ideally on top of
v4.16-rc1) and provide a stable branch, or get your ack to merge them through
Dave's tree if they don't conflict with what you have and will queue for
v4.17.

This version also drops the small fix to the Porter board device tree that has
been queued for v4.17 already.

Compared to v2, the biggest change is in patch 03/16. Following Rob's and
Frank's reviews it was clear that modifying the unflattened DT structure of
the overlay before applying it wasn't popular. I have thus decided to use one
overlay source per SoC to move as much of the DT changes to the overlay as
possible, and only perform manual modifications (that are still needed as some
of the information is board-specific) on the system DT after applying the
overlay. As a result the overlay is parsed and applied without being modified.

Compared to v1, this series update the r8a7792 and r8a7794 device tree sources
and incorporate review feedback as described by the changelogs of individual
patches.


Laurent Pinchart (11):
  dt-bindings: display: renesas: Add R-Car LVDS encoder DT bindings
  dt-bindings: display: renesas: Deprecate LVDS support in the DU
    bindings
  drm: rcar-du: Fix legacy DT to create LVDS encoder nodes
  drm: rcar-du: Convert LVDS encoder code to bridge driver
  ARM: dts: r8a7790: Convert to new LVDS DT bindings
  ARM: dts: r8a7791: Convert to new LVDS DT bindings
  ARM: dts: r8a7792: Convert to new DU DT bindings
  ARM: dts: r8a7793: Convert to new LVDS DT bindings
  ARM: dts: r8a7794: Convert to new DU DT bindings
  arm64: dts: renesas: r8a7795: Convert to new LVDS DT bindings
  arm64: dts: renesas: r8a7796: Convert to new LVDS DT bindings

Pantelis Antoniou (5):
  of: dynamic: Add __of_node_dupv()
  of: changesets: Introduce changeset helper methods
  of: changeset: Add of_changeset_node_move method
  of: unittest: changeset helpers
  i2c: demux: Use changeset helpers for clarity

 .../bindings/display/bridge/renesas,lvds.txt       |  56 +++
 .../devicetree/bindings/display/renesas,du.txt     |  31 +-
 MAINTAINERS                                        |   1 +
 arch/arm/boot/dts/r8a7790-lager.dts                |  22 +-
 arch/arm/boot/dts/r8a7790.dtsi                     |  64 ++-
 arch/arm/boot/dts/r8a7791-koelsch.dts              |  10 +-
 arch/arm/boot/dts/r8a7791-porter.dts               |  16 +-
 arch/arm/boot/dts/r8a7791.dtsi                     |  36 +-
 arch/arm/boot/dts/r8a7792.dtsi                     |   1 -
 arch/arm/boot/dts/r8a7793-gose.dts                 |  10 +-
 arch/arm/boot/dts/r8a7793.dtsi                     |  37 +-
 arch/arm/boot/dts/r8a7794.dtsi                     |   1 -
 .../boot/dts/renesas/r8a7795-es1-salvator-x.dts    |   3 +-
 arch/arm64/boot/dts/renesas/r8a7795-h3ulcb.dts     |   3 +-
 arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts |   3 +-
 .../arm64/boot/dts/renesas/r8a7795-salvator-xs.dts |   3 +-
 arch/arm64/boot/dts/renesas/r8a7795.dtsi           |  36 +-
 arch/arm64/boot/dts/renesas/r8a7796-m3ulcb.dts     |   3 +-
 arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts |   3 +-
 arch/arm64/boot/dts/renesas/r8a7796.dtsi           |  36 +-
 drivers/gpu/drm/rcar-du/Kconfig                    |   6 +-
 drivers/gpu/drm/rcar-du/Makefile                   |  10 +-
 drivers/gpu/drm/rcar-du/rcar_du_drv.c              |  21 +-
 drivers/gpu/drm/rcar-du/rcar_du_drv.h              |   5 -
 drivers/gpu/drm/rcar-du/rcar_du_encoder.c          | 175 +------
 drivers/gpu/drm/rcar-du/rcar_du_encoder.h          |  12 -
 drivers/gpu/drm/rcar-du/rcar_du_kms.c              |  14 +-
 drivers/gpu/drm/rcar-du/rcar_du_lvdscon.c          |  93 ----
 drivers/gpu/drm/rcar-du/rcar_du_lvdscon.h          |  24 -
 drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c          | 238 ----------
 drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.h          |  64 ---
 drivers/gpu/drm/rcar-du/rcar_du_of.c               | 307 ++++++++++++
 drivers/gpu/drm/rcar-du/rcar_du_of.h               |  20 +
 .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7790.dts    |  81 ++++
 .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7791.dts    |  55 +++
 .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7793.dts    |  55 +++
 .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7795.dts    |  55 +++
 .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7796.dts    |  55 +++
 drivers/gpu/drm/rcar-du/rcar_lvds.c                | 524 +++++++++++++++++++++
 drivers/i2c/muxes/i2c-demux-pinctrl.c              |  12 +-
 drivers/of/dynamic.c                               | 317 ++++++++++++-
 drivers/of/unittest.c                              |  54 +++
 include/linux/of.h                                 | 337 +++++++++++++
 43 files changed, 2199 insertions(+), 710 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt
 delete mode 100644 drivers/gpu/drm/rcar-du/rcar_du_lvdscon.c
 delete mode 100644 drivers/gpu/drm/rcar-du/rcar_du_lvdscon.h
 delete mode 100644 drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c
 delete mode 100644 drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.h
 create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of.c
 create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of.h
 create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7790.dts
 create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7791.dts
 create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7793.dts
 create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7795.dts
 create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7796.dts
 create mode 100644 drivers/gpu/drm/rcar-du/rcar_lvds.c

-- 
Regards,

Laurent Pinchart

[PATCH v4 03/16] of: dynamic: Add __of_node_dupv()

[Laurent Pinchart]

From: Pantelis Antoniou <pantelis.antoniou@konsulko.com>

Add an __of_node_dupv() private method and make __of_node_dup() use it.
This is required for the subsequent changeset accessors which will
make use of it.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/of/dynamic.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 7bb33d22b4e2..4ffd04925fdf 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -382,8 +382,9 @@ struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags)
 }
 
 /**
- * __of_node_dup() - Duplicate or create an empty device node dynamically.
- * @fmt: Format string (plus vargs) for new full name of the device node
+ * __of_node_dupv() - Duplicate or create an empty device node dynamically.
+ * @fmt: Format string for new full name of the device node
+ * @vargs: va_list containing the arugments for the node full name
  *
  * Create an device tree node, either by duplicating an empty node or by allocating
  * an empty one suitable for further modification.  The node data are
@@ -391,17 +392,15 @@ struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags)
  * OF_DETACHED bits set. Returns the newly allocated node or NULL on out of
  * memory error.
  */
-struct device_node *__of_node_dup(const struct device_node *np, const char *fmt, ...)
+struct device_node *__of_node_dupv(const struct device_node *np,
+		const char *fmt, va_list vargs)
 {
-	va_list vargs;
 	struct device_node *node;
 
 	node = kzalloc(sizeof(*node), GFP_KERNEL);
 	if (!node)
 		return NULL;
-	va_start(vargs, fmt);
 	node->full_name = kvasprintf(GFP_KERNEL, fmt, vargs);
-	va_end(vargs);
 	if (!node->full_name) {
 		kfree(node);
 		return NULL;
@@ -433,6 +432,24 @@ struct device_node *__of_node_dup(const struct device_node *np, const char *fmt,
 	return NULL;
 }
 
+/**
+ * __of_node_dup() - Duplicate or create an empty device node dynamically.
+ * @fmt: Format string (plus vargs) for new full name of the device node
+ *
+ * See: __of_node_dupv()
+ */
+struct device_node *__of_node_dup(const struct device_node *np,
+		const char *fmt, ...)
+{
+	va_list vargs;
+	struct device_node *node;
+
+	va_start(vargs, fmt);
+	node = __of_node_dupv(np, fmt, vargs);
+	va_end(vargs);
+	return node;
+}
+
 static void __of_changeset_entry_destroy(struct of_changeset_entry *ce)
 {
 	of_node_put(ce->np);
-- 
Regards,

Laurent Pinchart

[PATCH v4 04/16] of: changesets: Introduce changeset helper methods

[Laurent Pinchart]

From: Pantelis Antoniou <pantelis.antoniou@konsulko.com>

Changesets are very powerful, but the lack of a helper API
makes using them cumbersome. Introduce a simple copy based
API that makes things considerably easier.

To wit, adding a property using the raw API.

	struct property *prop;
	prop = kzalloc(sizeof(*prop)), GFP_KERNEL);
	prop->name = kstrdup("compatible");
	prop->value = kstrdup("foo,bar");
	prop->length = strlen(prop->value) + 1;
	of_changeset_add_property(ocs, np, prop);

while using the helper API

	of_changeset_add_property_string(ocs, np, "compatible",
			"foo,bar");

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
[Fixed memory leak in __of_changeset_add_update_property_copy()]
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/of/dynamic.c | 222 ++++++++++++++++++++++++++++++++++
 include/linux/of.h   | 328 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 550 insertions(+)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 4ffd04925fdf..85e722ed8631 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -910,3 +910,225 @@ int of_changeset_action(struct of_changeset *ocs, unsigned long action,
 	return 0;
 }
 EXPORT_SYMBOL_GPL(of_changeset_action);
+
+/* changeset helpers */
+
+/**
+ * of_changeset_create_device_node - Create an empty device node
+ *
+ * @ocs:	changeset pointer
+ * @parent:	parent device node
+ * @fmt:	format string for the node's full_name
+ * @args:	argument list for the format string
+ *
+ * Create an empty device node, marking it as detached and allocated.
+ *
+ * Returns a device node on success, an error encoded pointer otherwise
+ */
+struct device_node *of_changeset_create_device_nodev(
+	struct of_changeset *ocs, struct device_node *parent,
+	const char *fmt, va_list vargs)
+{
+	struct device_node *node;
+
+	node = __of_node_dupv(NULL, fmt, vargs);
+	if (!node)
+		return ERR_PTR(-ENOMEM);
+
+	node->parent = parent;
+	return node;
+}
+EXPORT_SYMBOL_GPL(of_changeset_create_device_nodev);
+
+/**
+ * of_changeset_create_device_node - Create an empty device node
+ *
+ * @ocs:	changeset pointer
+ * @parent:	parent device node
+ * @fmt:	Format string for the node's full_name
+ * ...		Arguments
+ *
+ * Create an empty device node, marking it as detached and allocated.
+ *
+ * Returns a device node on success, an error encoded pointer otherwise
+ */
+__printf(3, 4) struct device_node *
+of_changeset_create_device_node(struct of_changeset *ocs,
+	struct device_node *parent, const char *fmt, ...)
+{
+	va_list vargs;
+	struct device_node *node;
+
+	va_start(vargs, fmt);
+	node = of_changeset_create_device_nodev(ocs, parent, fmt, vargs);
+	va_end(vargs);
+	return node;
+}
+EXPORT_SYMBOL_GPL(of_changeset_create_device_node);
+
+/**
+ * __of_changeset_add_property_copy - Create/update a new property copying
+ *                                    name & value
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @value:	pointer to the value data
+ * @length:	length of the value in bytes
+ * @update:	True on update operation
+ *
+ * Adds/updates a property to the changeset by making copies of the name & value
+ * entries. The @update parameter controls whether an add or update takes place.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+int __of_changeset_add_update_property_copy(struct of_changeset *ocs,
+		struct device_node *np, const char *name, const void *value,
+		int length, bool update)
+{
+	struct property *prop;
+	int ret = -ENOMEM;
+
+	prop = kzalloc(sizeof(*prop), GFP_KERNEL);
+	if (!prop)
+		return -ENOMEM;
+
+	prop->name = kstrdup(name, GFP_KERNEL);
+	if (!prop->name)
+		goto out_err;
+
+	/*
+	 * NOTE: There is no check for zero length value.
+	 * In case of a boolean property, this will allocate a value
+	 * of zero bytes. We do this to work around the use
+	 * of of_get_property() calls on boolean values.
+	 */
+	prop->value = kmemdup(value, length, GFP_KERNEL);
+	if (!prop->value)
+		goto out_err;
+
+	of_property_set_flag(prop, OF_DYNAMIC);
+
+	prop->length = length;
+
+	if (!update)
+		ret = of_changeset_add_property(ocs, np, prop);
+	else
+		ret = of_changeset_update_property(ocs, np, prop);
+
+	if (!ret)
+		return 0;
+
+out_err:
+	kfree(prop->value);
+	kfree(prop->name);
+	kfree(prop);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(__of_changeset_add_update_property_copy);
+
+/**
+ * of_changeset_add_property_stringf - Create a new formatted string property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @fmt:	format of string property
+ * ...		arguments of the format string
+ *
+ * Adds a string property to the changeset by making copies of the name
+ * and the formatted value.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+__printf(4, 5) int of_changeset_add_property_stringf(
+		struct of_changeset *ocs, struct device_node *np,
+		const char *name, const char *fmt, ...)
+{
+	va_list vargs;
+	int ret;
+
+	va_start(vargs, fmt);
+	ret = __of_changeset_add_update_property_stringv(ocs, np, name, fmt,
+			vargs, false);
+	va_end(vargs);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(of_changeset_add_property_stringf);
+
+/**
+ * of_changeset_update_property_stringf - Update formatted string property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @fmt:	format of string property
+ * ...		arguments of the format string
+ *
+ * Updates a string property to the changeset by making copies of the name
+ * and the formatted value.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+int of_changeset_update_property_stringf(
+	struct of_changeset *ocs, struct device_node *np,
+	const char *name, const char *fmt, ...)
+{
+	va_list vargs;
+	int ret;
+
+	va_start(vargs, fmt);
+	ret = __of_changeset_add_update_property_stringv(ocs, np, name, fmt,
+			vargs, true);
+	va_end(vargs);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(of_changeset_update_property_stringf);
+
+/**
+ * __of_changeset_add_update_property_string_list - Create/update a string
+ *                                                  list property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @strs:	pointer to the string list
+ * @count:	string count
+ * @update:	True on update operation
+ *
+ * Adds a string list property to the changeset.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+int __of_changeset_add_update_property_string_list(
+		struct of_changeset *ocs, struct device_node *np,
+		const char *name, const char **strs, int count, bool update)
+{
+	int total = 0, i, ret;
+	char *value, *s;
+
+	for (i = 0; i < count; i++) {
+		/* check if  it's NULL */
+		if (!strs[i])
+			return -EINVAL;
+		total += strlen(strs[i]) + 1;
+	}
+
+	value = kmalloc(total, GFP_KERNEL);
+	if (!value)
+		return -ENOMEM;
+
+	for (i = 0, s = value; i < count; i++) {
+		/* no need to check for NULL, check above */
+		strcpy(s, strs[i]);
+		s += strlen(strs[i]) + 1;
+	}
+
+	ret = __of_changeset_add_update_property_copy(ocs, np, name, value,
+			total, update);
+
+	kfree(value);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(__of_changeset_add_update_property_string_list);
diff --git a/include/linux/of.h b/include/linux/of.h
index da1ee95241c1..7aef555f9bc2 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1266,6 +1266,8 @@ enum of_reconfig_change {
 };
 
 #ifdef CONFIG_OF_DYNAMIC
+#include <linux/slab.h>
+
 extern int of_reconfig_notifier_register(struct notifier_block *);
 extern int of_reconfig_notifier_unregister(struct notifier_block *);
 extern int of_reconfig_notify(unsigned long, struct of_reconfig_data *rd);
@@ -1309,6 +1311,23 @@ static inline int of_changeset_update_property(struct of_changeset *ocs,
 {
 	return of_changeset_action(ocs, OF_RECONFIG_UPDATE_PROPERTY, np, prop);
 }
+
+struct device_node *of_changeset_create_device_nodev(
+	struct of_changeset *ocs, struct device_node *parent,
+	const char *fmt, va_list vargs);
+
+__printf(3, 4) struct device_node *
+of_changeset_create_device_node(struct of_changeset *ocs,
+	struct device_node *parent, const char *fmt, ...);
+
+int __of_changeset_add_update_property_copy(struct of_changeset *ocs,
+		struct device_node *np, const char *name, const void *value,
+		int length, bool update);
+
+int __of_changeset_add_update_property_string_list(
+		struct of_changeset *ocs, struct device_node *np,
+		const char *name, const char **strs, int count, bool update);
+
 #else /* CONFIG_OF_DYNAMIC */
 static inline int of_reconfig_notifier_register(struct notifier_block *nb)
 {
@@ -1328,8 +1347,317 @@ static inline int of_reconfig_get_state_change(unsigned long action,
 {
 	return -EINVAL;
 }
+
+static inline struct device_node *of_changeset_create_device_nodev(
+	struct of_changeset *ocs, struct device_node *parent,
+	const char *fmt, va_list vargs)
+{
+	return ERR_PTR(-EINVAL);
+}
+
+static inline __printf(3, 4) struct device_node *
+of_changeset_create_device_node(struct of_changeset *ocs,
+	struct device_node *parent, const char *fmt, ...)
+{
+	return ERR_PTR(-EINVAL);
+}
+
+static inline int __of_changeset_add_update_property_copy(
+	struct of_changeset *ocs, struct device_node *np,
+	const char *name, const void *value, int length, bool update)
+{
+	return -EINVAL;
+}
+
+static inline __printf(4, 5) int of_changeset_add_property_stringf(
+		struct of_changeset *ocs, struct device_node *np,
+		const char *name, const char *fmt, ...)
+{
+	return -EINVAL;
+}
+
+static inline int of_changeset_update_property_stringf(
+	struct of_changeset *ocs, struct device_node *np,
+	const char *name, const char *fmt, ...)
+{
+	return -EINVAL;
+}
+
+static inline int __of_changeset_add_update_property_string_list(
+		struct of_changeset *ocs, struct device_node *np,
+		const char *name, const char **strs, int count, bool update)
+{
+	return -EINVAL;
+}
+
 #endif /* CONFIG_OF_DYNAMIC */
 
+/**
+ * of_changeset_add_property_copy - Create a new property copying name & value
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @value:	pointer to the value data
+ * @length:	length of the value in bytes
+ *
+ * Adds a property to the changeset by making copies of the name & value
+ * entries.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+static inline int of_changeset_add_property_copy(struct of_changeset *ocs,
+	struct device_node *np, const char *name,
+	const void *value, int length)
+{
+	return __of_changeset_add_update_property_copy(ocs, np, name, value,
+			length, false);
+}
+
+/**
+ * of_changeset_update_property_copy - Update a property copying name & value
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @value:	pointer to the value data
+ * @length:	length of the value in bytes
+ *
+ * Update a property to the changeset by making copies of the name & value
+ * entries.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+static inline int of_changeset_update_property_copy(struct of_changeset *ocs,
+	struct device_node *np, const char *name,
+	const void *value, int length)
+{
+	return __of_changeset_add_update_property_copy(ocs, np, name, value,
+			length, true);
+}
+
+/**
+ * __of_changeset_add_update_property_string - Create/update a string property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @str:	string property value
+ * @update:	True on update operation
+ *
+ * Adds/updates a string property to the changeset by making copies of the name
+ * and the given value. The @update parameter controls whether an add or
+ * update takes place.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+static inline int __of_changeset_add_update_property_string(
+	struct of_changeset *ocs, struct device_node *np, const char *name,
+	const char *str, bool update)
+{
+	return __of_changeset_add_update_property_copy(ocs, np, name, str,
+			strlen(str) + 1, update);
+}
+
+/**
+ * __of_changeset_add_update_property_stringv - Create/update a formatted
+ *						string property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @fmt:	format of string property
+ * @vargs:	arguments of the format string
+ * @update:	True on update operation
+ *
+ * Adds/updates a string property to the changeset by making copies of the name
+ * and the formatted value. The @update parameter controls whether an add or
+ * update takes place.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+static inline int __of_changeset_add_update_property_stringv(
+	struct of_changeset *ocs, struct device_node *np, const char *name,
+	const char *fmt, va_list vargs, bool update)
+{
+	char *str;
+	int ret;
+
+	str = kvasprintf(GFP_KERNEL, fmt, vargs);
+	if (!str)
+		return -ENOMEM;
+	ret = __of_changeset_add_update_property_string(ocs, np, name, str,
+			update);
+	kfree(str);
+
+	return ret;
+}
+
+/**
+ * of_changeset_add_property_string_list - Create a new string list property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @strs:	pointer to the string list
+ * @count:	string count
+ *
+ * Adds a string list property to the changeset.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+static inline int of_changeset_add_property_string_list(
+	struct of_changeset *ocs, struct device_node *np, const char *name,
+	const char **strs, int count)
+{
+	return __of_changeset_add_update_property_string_list(ocs, np, name,
+			strs, count, false);
+}
+
+/**
+ * of_changeset_update_property_string_list - Update string list property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @strs:	pointer to the string list
+ * @count:	string count
+ *
+ * Updates a string list property to the changeset.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+static inline int of_changeset_update_property_string_list(
+	struct of_changeset *ocs, struct device_node *np,
+	const char *name, const char **strs, int count)
+{
+	return __of_changeset_add_update_property_string_list(ocs, np, name,
+			strs, count, true);
+}
+
+/**
+ * of_changeset_add_property_string - Adds a string property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @str:	string property
+ *
+ * Adds a string property to the changeset by making copies of the name
+ * and the string value.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+static inline int of_changeset_add_property_string(
+	struct of_changeset *ocs, struct device_node *np,
+	const char *name, const char *str)
+{
+	return __of_changeset_add_update_property_string(ocs, np, name, str,
+			false);
+}
+
+/**
+ * of_changeset_update_property_string - Update a string property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @str:	string property
+ *
+ * Updates a string property to the changeset by making copies of the name
+ * and the string value.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+static inline int of_changeset_update_property_string(
+	struct of_changeset *ocs, struct device_node *np,
+	const char *name, const char *str)
+{
+	return __of_changeset_add_update_property_string(ocs, np, name, str,
+			true);
+}
+
+/**
+ * of_changeset_add_property_u32 - Create a new u32 property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @val:	value in host endian format
+ *
+ * Adds a u32 property to the changeset.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+static inline int of_changeset_add_property_u32(struct of_changeset *ocs,
+		struct device_node *np, const char *name, u32 val)
+{
+	val = cpu_to_be32(val);
+	return __of_changeset_add_update_property_copy(ocs, np, name, &val,
+			sizeof(val), false);
+}
+
+/**
+ * of_changeset_update_property_u32 - Update u32 property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @val:	value in host endian format
+ *
+ * Updates a u32 property to the changeset.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+static inline int of_changeset_update_property_u32(
+	struct of_changeset *ocs, struct device_node *np,
+	const char *name, u32 val)
+{
+	val = cpu_to_be32(val);
+	return __of_changeset_add_update_property_copy(ocs, np, name, &val,
+			sizeof(val), true);
+}
+
+/**
+ * of_changeset_add_property_bool - Create a new u32 property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ *
+ * Adds a bool property to the changeset. Note that there is
+ * no option to set the value to false, since the property
+ * existing sets it to true.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+static inline int of_changeset_add_property_bool(
+	struct of_changeset *ocs, struct device_node *np, const char *name)
+{
+	return __of_changeset_add_update_property_copy(ocs, np, name, "", 0,
+			false);
+}
+
+/**
+ * of_changeset_update_property_bool - Update a bool property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ *
+ * Updates a property to the changeset. Note that there is
+ * no option to set the value to false, since the property
+ * existing sets it to true.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+static inline int of_changeset_update_property_bool(struct of_changeset *ocs,
+		struct device_node *np, const char *name)
+{
+	return __of_changeset_add_update_property_copy(ocs, np, name, "", 0,
+			true);
+}
+
 /**
  * of_device_is_system_power_controller - Tells if system-power-controller is found for device_node
  * @np: Pointer to the given device_node
-- 
Regards,

Laurent Pinchart

[PATCH v4 05/16] of: changeset: Add of_changeset_node_move method

[Laurent Pinchart]

From: Pantelis Antoniou <pantelis.antoniou@konsulko.com>

Adds a changeset helper for moving a subtree to a different place
in the running tree. This is useful in advances cases of dynamic
device tree construction.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/of/dynamic.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of.h   |  9 +++++++
 2 files changed, 75 insertions(+)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 85e722ed8631..27d9057ef360 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -1132,3 +1132,69 @@ int __of_changeset_add_update_property_string_list(
 	return ret;
 }
 EXPORT_SYMBOL_GPL(__of_changeset_add_update_property_string_list);
+
+static struct device_node *
+__of_changeset_node_move_one(struct of_changeset *ocs,
+		struct device_node *np, struct device_node *new_parent)
+{
+	struct device_node *np2;
+	const char *unitname;
+	int err;
+
+	err = of_changeset_detach_node(ocs, np);
+	if (err)
+		return ERR_PTR(err);
+
+	unitname = strrchr(np->full_name, '/');
+	if (!unitname)
+		unitname = np->full_name;
+
+	np2 = __of_node_dup(np, "%s/%s",
+			new_parent->full_name, unitname);
+	if (!np2)
+		return ERR_PTR(-ENOMEM);
+	np2->parent = new_parent;
+
+	err = of_changeset_attach_node(ocs, np2);
+	if (err)
+		return ERR_PTR(err);
+
+	return np2;
+}
+
+/**
+ * of_changeset_node_move_to - Moves a subtree to a new place in
+ *                             the tree
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer to be moved
+ * @to:		device node of the new parent
+ *
+ * Moves a subtree to a new place in the tree.
+ * Note that a move is a safe operation because the phandles
+ * remain valid.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+int of_changeset_node_move(struct of_changeset *ocs,
+		struct device_node *np, struct device_node *new_parent)
+{
+	struct device_node *npc, *nppc;
+
+	/* move the root first */
+	nppc = __of_changeset_node_move_one(ocs, np, new_parent);
+	if (IS_ERR(nppc))
+		return PTR_ERR(nppc);
+
+	/* move the subtrees next */
+	for_each_child_of_node(np, npc) {
+		nppc = __of_changeset_node_move_one(ocs, npc, nppc);
+		if (IS_ERR(nppc)) {
+			of_node_put(npc);
+			return PTR_ERR(nppc);
+		}
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_changeset_node_move);
diff --git a/include/linux/of.h b/include/linux/of.h
index 7aef555f9bc2..76197bc75346 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1328,6 +1328,9 @@ int __of_changeset_add_update_property_string_list(
 		struct of_changeset *ocs, struct device_node *np,
 		const char *name, const char **strs, int count, bool update);
 
+int of_changeset_node_move(struct of_changeset *ocs,
+	struct device_node *np, struct device_node *new_parent);
+
 #else /* CONFIG_OF_DYNAMIC */
 static inline int of_reconfig_notifier_register(struct notifier_block *nb)
 {
@@ -1390,6 +1393,12 @@ static inline int __of_changeset_add_update_property_string_list(
 	return -EINVAL;
 }
 
+static inline int of_changeset_node_move(struct of_changeset *ocs,
+		struct device_node *np, struct device_node *new_parent)
+{
+	return -EINVAL;
+}
+
 #endif /* CONFIG_OF_DYNAMIC */
 
 /**
-- 
Regards,

Laurent Pinchart

[PATCH v4 06/16] of: unittest: changeset helpers

[Laurent Pinchart]

From: Pantelis Antoniou <pantelis.antoniou@konsulko.com>

Add a unitest specific for the new changeset helpers.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/of/unittest.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index 7a9abaae874d..1b21d2c549a8 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -609,6 +609,59 @@ static void __init of_unittest_changeset(void)
 #endif
 }
 
+static void __init of_unittest_changeset_helper(void)
+{
+#ifdef CONFIG_OF_DYNAMIC
+	struct device_node *n1, *n2, *n21, *parent, *np;
+	struct of_changeset chgset;
+
+	of_changeset_init(&chgset);
+
+	parent = of_find_node_by_path("/testcase-data/changeset");
+
+	unittest(parent, "testcase setup failure\n");
+	n1 = of_changeset_create_device_node(&chgset,
+			parent, "/testcase-data/changeset/n1");
+	unittest(n1, "testcase setup failure\n");
+	n2 = of_changeset_create_device_node(&chgset,
+			parent, "/testcase-data/changeset/n2");
+	unittest(n2, "testcase setup failure\n");
+	n21 = of_changeset_create_device_node(&chgset, n2, "%s/%s",
+			"/testcase-data/changeset/n2", "n21");
+	unittest(n21, "testcase setup failure\n");
+
+	unittest(!of_changeset_add_property_string(&chgset, parent,
+				"prop-add", "foo"), "fail add prop\n");
+
+	unittest(!of_changeset_attach_node(&chgset, n1), "fail n1 attach\n");
+	unittest(!of_changeset_attach_node(&chgset, n2), "fail n2 attach\n");
+	unittest(!of_changeset_attach_node(&chgset, n21), "fail n21 attach\n");
+
+	unittest(!of_changeset_apply(&chgset), "apply failed\n");
+
+	/* Make sure node names are constructed correctly */
+	np = of_find_node_by_path("/testcase-data/changeset/n1");
+	unittest(np, "'%s' not added\n", n1->full_name);
+	of_node_put(np);
+
+	/* Make sure node names are constructed correctly */
+	np = of_find_node_by_path("/testcase-data/changeset/n2");
+	unittest(np, "'%s' not added\n", n2->full_name);
+	of_node_put(np);
+
+	np = of_find_node_by_path("/testcase-data/changeset/n2/n21");
+	unittest(np, "'%s' not added\n", n21->full_name);
+	of_node_put(np);
+
+	unittest(!of_changeset_revert(&chgset), "revert failed\n");
+
+	of_changeset_destroy(&chgset);
+
+	of_node_put(parent);
+#endif
+}
+
+
 static void __init of_unittest_parse_interrupts(void)
 {
 	struct device_node *np;
@@ -2363,6 +2416,7 @@ static int __init of_unittest(void)
 	of_unittest_property_string();
 	of_unittest_property_copy();
 	of_unittest_changeset();
+	of_unittest_changeset_helper();
 	of_unittest_parse_interrupts();
 	of_unittest_parse_interrupts_extended();
 	of_unittest_match_node();
-- 
Regards,

Laurent Pinchart

[PATCH v4 07/16] i2c: demux: Use changeset helpers for clarity

[Laurent Pinchart]

From: Pantelis Antoniou <pantelis.antoniou@konsulko.com>

The changeset helpers are easier to use, use them instead of
using the static property.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
Acked-by: Wolfram Sang <wsa@the-dreams.de>
["okay" -> "ok"]
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/i2c/muxes/i2c-demux-pinctrl.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-demux-pinctrl.c b/drivers/i2c/muxes/i2c-demux-pinctrl.c
index 33ce032cb701..0f0046831492 100644
--- a/drivers/i2c/muxes/i2c-demux-pinctrl.c
+++ b/drivers/i2c/muxes/i2c-demux-pinctrl.c
@@ -220,10 +220,7 @@ static int i2c_demux_pinctrl_probe(struct platform_device *pdev)
 
 	priv = devm_kzalloc(&pdev->dev, sizeof(*priv)
 			   + num_chan * sizeof(struct i2c_demux_pinctrl_chan), GFP_KERNEL);
-
-	props = devm_kcalloc(&pdev->dev, num_chan, sizeof(*props), GFP_KERNEL);
-
-	if (!priv || !props)
+	if (!priv)
 		return -ENOMEM;
 
 	err = of_property_read_string(np, "i2c-bus-name", &priv->bus_name);
@@ -241,12 +238,9 @@ static int i2c_demux_pinctrl_probe(struct platform_device *pdev)
 		}
 		priv->chan[i].parent_np = adap_np;
 
-		props[i].name = devm_kstrdup(&pdev->dev, "status", GFP_KERNEL);
-		props[i].value = devm_kstrdup(&pdev->dev, "ok", GFP_KERNEL);
-		props[i].length = 3;
-
 		of_changeset_init(&priv->chan[i].chgset);
-		of_changeset_update_property(&priv->chan[i].chgset, adap_np, &props[i]);
+		of_changeset_update_property_string(&priv->chan[i].chgset,
+						    adap_np, "status", "ok");
 	}
 
 	priv->num_chan = num_chan;
-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 07/16] i2c: demux: Use changeset helpers for clarity

[Wolfram Sang]

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

On Wed, Feb 21, 2018 at 01:10:37AM +0200, Laurent Pinchart wrote:
> From: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> 
> The changeset helpers are easier to use, use them instead of
> using the static property.
> 
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> Acked-by: Wolfram Sang <wsa@the-dreams.de>

My ack still holds. Good luck pushing this series!


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

Re: [PATCH v4 04/16] of: changesets: Introduce changeset helper methods

[Geert Uytterhoeven]

Hi Laurent,

On Wed, Feb 21, 2018 at 12:10 AM, Laurent Pinchart
<laurent.pinchart+renesas@ideasonboard.com> wrote:
> From: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>
> Changesets are very powerful, but the lack of a helper API
> makes using them cumbersome. Introduce a simple copy based
> API that makes things considerably easier.
>
> To wit, adding a property using the raw API.
>
>         struct property *prop;
>         prop = kzalloc(sizeof(*prop)), GFP_KERNEL);
>         prop->name = kstrdup("compatible");
>         prop->value = kstrdup("foo,bar");
>         prop->length = strlen(prop->value) + 1;
>         of_changeset_add_property(ocs, np, prop);
>
> while using the helper API
>
>         of_changeset_add_property_string(ocs, np, "compatible",
>                         "foo,bar");
>
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> [Fixed memory leak in __of_changeset_add_update_property_copy()]
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

You missed one fix I have in my topic/overlays branch
https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git/commit/?h=topic/overlays&id=150f95b9dec77ce371c229f7ac4d6dd8620bef4a

> --- a/include/linux/of.h
> +++ b/include/linux/of.h

> +/**
> + * of_changeset_add_property_u32 - Create a new u32 property
> + *
> + * @ocs:       changeset pointer
> + * @np:                device node pointer
> + * @name:      name of the property
> + * @val:       value in host endian format
> + *
> + * Adds a u32 property to the changeset.
> + *
> + * Returns zero on success, a negative error value otherwise.
> + */
> +static inline int of_changeset_add_property_u32(struct of_changeset *ocs,
> +               struct device_node *np, const char *name, u32 val)
> +{
> +       val = cpu_to_be32(val);

You must use an intermediate, to avoid complaints from sparse:

    __be32 x = cpu_to_be32(val);

> +       return __of_changeset_add_update_property_copy(ocs, np, name, &val,
> +                       sizeof(val), false);

s/val/x/

> +}
> +
> +/**
> + * of_changeset_update_property_u32 - Update u32 property
> + *
> + * @ocs:       changeset pointer
> + * @np:                device node pointer
> + * @name:      name of the property
> + * @val:       value in host endian format
> + *
> + * Updates a u32 property to the changeset.
> + *
> + * Returns zero on success, a negative error value otherwise.
> + */
> +static inline int of_changeset_update_property_u32(
> +       struct of_changeset *ocs, struct device_node *np,
> +       const char *name, u32 val)
> +{
> +       val = cpu_to_be32(val);

Oh, a new one.

> +       return __of_changeset_add_update_property_copy(ocs, np, name, &val,
> +                       sizeof(val), true);
> +}

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v4 03/16] of: dynamic: Add __of_node_dupv()

[Geert Uytterhoeven]

Hi Laurent,

On Wed, Feb 21, 2018 at 12:10 AM, Laurent Pinchart
<laurent.pinchart+renesas@ideasonboard.com> wrote:
> From: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>
> Add an __of_node_dupv() private method and make __of_node_dup() use it.
> This is required for the subsequent changeset accessors which will
> make use of it.
>
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/of/dynamic.c | 29 +++++++++++++++++++++++------
>  1 file changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index 7bb33d22b4e2..4ffd04925fdf 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -382,8 +382,9 @@ struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags)
>  }
>
>  /**
> - * __of_node_dup() - Duplicate or create an empty device node dynamically.
> - * @fmt: Format string (plus vargs) for new full name of the device node
> + * __of_node_dupv() - Duplicate or create an empty device node dynamically.
> + * @fmt: Format string for new full name of the device node
> + * @vargs: va_list containing the arugments for the node full name
>   *
>   * Create an device tree node, either by duplicating an empty node or by allocating
>   * an empty one suitable for further modification.  The node data are
> @@ -391,17 +392,15 @@ struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags)
>   * OF_DETACHED bits set. Returns the newly allocated node or NULL on out of
>   * memory error.
>   */
> -struct device_node *__of_node_dup(const struct device_node *np, const char *fmt, ...)
> +struct device_node *__of_node_dupv(const struct device_node *np,

static, cfr.
https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git/commit/?h=topic/overlays&id=c45324e1807dd708344c9a478b777b68aca11cdf

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v4 03/16] of: dynamic: Add __of_node_dupv()

[Laurent Pinchart]

Hi Geert,

On Wednesday, 21 February 2018 12:26:45 EET Geert Uytterhoeven wrote:
> On Wed, Feb 21, 2018 at 12:10 AM, Laurent Pinchart wrote:
> > From: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> > 
> > Add an __of_node_dupv() private method and make __of_node_dup() use it.
> > This is required for the subsequent changeset accessors which will
> > make use of it.
> > 
> > Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > 
> >  drivers/of/dynamic.c | 29 +++++++++++++++++++++++------
> >  1 file changed, 23 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> > index 7bb33d22b4e2..4ffd04925fdf 100644
> > --- a/drivers/of/dynamic.c
> > +++ b/drivers/of/dynamic.c
> > @@ -382,8 +382,9 @@ struct property *__of_prop_dup(const struct property
> > *prop, gfp_t allocflags)> 
> >  }
> >  
> >  /**
> > 
> > - * __of_node_dup() - Duplicate or create an empty device node
> > dynamically.
> > - * @fmt: Format string (plus vargs) for new full name of the device node
> > + * __of_node_dupv() - Duplicate or create an empty device node
> > dynamically. + * @fmt: Format string for new full name of the device node
> > + * @vargs: va_list containing the arugments for the node full name
> > 
> >   *
> >   * Create an device tree node, either by duplicating an empty node or by
> >   allocating * an empty one suitable for further modification.  The node
> >   data are> 
> > @@ -391,17 +392,15 @@ struct property *__of_prop_dup(const struct property
> > *prop, gfp_t allocflags)> 
> >   * OF_DETACHED bits set. Returns the newly allocated node or NULL on out
> >   of
> >   * memory error.
> >   */
> > 
> > -struct device_node *__of_node_dup(const struct device_node *np, const
> > char *fmt, ...) +struct device_node *__of_node_dupv(const struct
> > device_node *np,
> 
> static, cfr.
> https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git/co
> mmit/?h=topic/overlays&id=c45324e1807dd708344c9a478b777b68aca11cdf

I'll fix that in the next version.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 04/16] of: changesets: Introduce changeset helper methods

[Laurent Pinchart]

Hi Geert,

On Wednesday, 21 February 2018 12:21:50 EET Geert Uytterhoeven wrote:
> On Wed, Feb 21, 2018 at 12:10 AM, Laurent Pinchart wrote:
> > From: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> > 
> > Changesets are very powerful, but the lack of a helper API
> > makes using them cumbersome. Introduce a simple copy based
> > API that makes things considerably easier.
> > 
> > To wit, adding a property using the raw API.
> > 
> >         struct property *prop;
> >         prop = kzalloc(sizeof(*prop)), GFP_KERNEL);
> >         prop->name = kstrdup("compatible");
> >         prop->value = kstrdup("foo,bar");
> >         prop->length = strlen(prop->value) + 1;
> >         of_changeset_add_property(ocs, np, prop);
> > 
> > while using the helper API
> > 
> >         of_changeset_add_property_string(ocs, np, "compatible",
> >         
> >                         "foo,bar");
> > 
> > Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> > [Fixed memory leak in __of_changeset_add_update_property_copy()]
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> 
> You missed one fix I have in my topic/overlays branch
> https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git/co
> mmit/?h=topic/overlays&id=150f95b9dec77ce371c229f7ac4d6dd8620bef4a
> > --- a/include/linux/of.h
> > +++ b/include/linux/of.h
> > 
> > +/**
> > + * of_changeset_add_property_u32 - Create a new u32 property
> > + *
> > + * @ocs:       changeset pointer
> > + * @np:                device node pointer
> > + * @name:      name of the property
> > + * @val:       value in host endian format
> > + *
> > + * Adds a u32 property to the changeset.
> > + *
> > + * Returns zero on success, a negative error value otherwise.
> > + */
> > +static inline int of_changeset_add_property_u32(struct of_changeset *ocs,
> > +               struct device_node *np, const char *name, u32 val)
> > +{
> > +       val = cpu_to_be32(val);
> 
> You must use an intermediate, to avoid complaints from sparse:
> 
>     __be32 x = cpu_to_be32(val);
> 
> > +       return __of_changeset_add_update_property_copy(ocs, np, name,
> > &val,
> > +                       sizeof(val), false);
> 
> s/val/x/

I'll fix both in the next version, thanks.

> > +}
> > +
> > +/**
> > + * of_changeset_update_property_u32 - Update u32 property
> > + *
> > + * @ocs:       changeset pointer
> > + * @np:                device node pointer
> > + * @name:      name of the property
> > + * @val:       value in host endian format
> > + *
> > + * Updates a u32 property to the changeset.
> > + *
> > + * Returns zero on success, a negative error value otherwise.
> > + */
> > +static inline int of_changeset_update_property_u32(
> > +       struct of_changeset *ocs, struct device_node *np,
> > +       const char *name, u32 val)
> > +{
> > +       val = cpu_to_be32(val);
> 
> Oh, a new one.
> 
> > +       return __of_changeset_add_update_property_copy(ocs, np, name,
> > &val,
> > +                       sizeof(val), true);
> > +}

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 04/16] of: changesets: Introduce changeset helper methods

[Rob Herring]

On Wed, Feb 21, 2018 at 4:21 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> Hi Laurent,
>
> On Wed, Feb 21, 2018 at 12:10 AM, Laurent Pinchart
> <laurent.pinchart+renesas@ideasonboard.com> wrote:
>> From: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>>
>> Changesets are very powerful, but the lack of a helper API
>> makes using them cumbersome. Introduce a simple copy based
>> API that makes things considerably easier.
>>
>> To wit, adding a property using the raw API.
>>
>>         struct property *prop;
>>         prop = kzalloc(sizeof(*prop)), GFP_KERNEL);
>>         prop->name = kstrdup("compatible");
>>         prop->value = kstrdup("foo,bar");
>>         prop->length = strlen(prop->value) + 1;
>>         of_changeset_add_property(ocs, np, prop);
>>
>> while using the helper API
>>
>>         of_changeset_add_property_string(ocs, np, "compatible",
>>                         "foo,bar");
>>
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>> [Fixed memory leak in __of_changeset_add_update_property_copy()]
>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>
> You missed one fix I have in my topic/overlays branch
> https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git/commit/?h=topic/overlays&id=150f95b9dec77ce371c229f7ac4d6dd8620bef4a

Are you planning to try to upstream all this? If not, I'll get Frank
to keep changing the overlay API to make carrying it out of tree more
painful. :)

Rob

Re: [PATCH v4 04/16] of: changesets: Introduce changeset helper methods

[Geert Uytterhoeven]

Hi Rob,

On Wed, Feb 21, 2018 at 4:23 PM, Rob Herring <robh@kernel.org> wrote:
> On Wed, Feb 21, 2018 at 4:21 AM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>> You missed one fix I have in my topic/overlays branch
>> https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git/commit/?h=topic/overlays&id=150f95b9dec77ce371c229f7ac4d6dd8620bef4a
>
> Are you planning to try to upstream all this? If not, I'll get Frank

Not really. But I do need a way to load DT overlays at runtime, for testing
hardware on expansion connectors.

> to keep changing the overlay API to make carrying it out of tree more
> painful. :)

He already did a very good job w.r.t. that in v4.15-rc1 ;^)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v4 00/16] R-Car DU: Convert LVDS code to bridge driver

[Simon Horman]

On Wed, Feb 21, 2018 at 01:10:30AM +0200, Laurent Pinchart wrote:
> Hello,
> 
> This patch series addresses a design mistake that dates back from the initial
> DU support. Support for the LVDS encoders, which are IP cores separate from
> the DU, was bundled in the DU driver. Worse, both the DU and LVDS were
> described through a single DT node.
> 
> To fix the, patches 01/16 and 02/16 define new DT bindings for the LVDS
> encoders, and deprecate their description inside the DU bindings. To retain
> backward compatibility with existing DT, patches 03/16 to 08/16 then patch the
> device tree at runtime to convert the legacy bindings to the new ones.
> 
> With the DT side addressed, patch 09/16 converts the LVDS support code to a
> separate bridge driver. Patches 11/16 to 16/16 then update all the device tree
> sources to the new DU and LVDS encoders bindings.
> 
> I decided to go for live DT patching in patch 08/16 because implementing
> support for both the legacy and new bindings in the driver would have been
> very intrusive, and prevented further cleanups. This version relies more
> heavily on overlays to avoid touching the internals of the OF core compared to
> v2, even if manual fixes to the device tree are still needed.
> 
> Compared to v3, this series uses the OF changeset API to update properties
> instead of accessing the internals of the property structure. This removes the
> local implementation of functions to look up nodes by path and update
> properties. In order to do this, I pulled in Pantelis' patch series titled
> "[PATCH v2 0/5] of: dynamic: Changesets helpers & fixes" at Rob's request, and
> rebased it while taking two small review comments into account.
> 
> Rob, I'd like this series to be merged in v4.17. As the changeset helpers are
> now a dependency, I'd need you to merge them early (ideally on top of
> v4.16-rc1) and provide a stable branch, or get your ack to merge them through
> Dave's tree if they don't conflict with what you have and will queue for
> v4.17.
> 
> This version also drops the small fix to the Porter board device tree that has
> been queued for v4.17 already.
> 
> Compared to v2, the biggest change is in patch 03/16. Following Rob's and
> Frank's reviews it was clear that modifying the unflattened DT structure of
> the overlay before applying it wasn't popular. I have thus decided to use one
> overlay source per SoC to move as much of the DT changes to the overlay as
> possible, and only perform manual modifications (that are still needed as some
> of the information is board-specific) on the system DT after applying the
> overlay. As a result the overlay is parsed and applied without being modified.
> 
> Compared to v1, this series update the r8a7792 and r8a7794 device tree sources
> and incorporate review feedback as described by the changelogs of individual
> patches.
> 
> 
> Laurent Pinchart (11):
>   dt-bindings: display: renesas: Add R-Car LVDS encoder DT bindings
>   dt-bindings: display: renesas: Deprecate LVDS support in the DU
>     bindings
>   drm: rcar-du: Fix legacy DT to create LVDS encoder nodes
>   drm: rcar-du: Convert LVDS encoder code to bridge driver
>   ARM: dts: r8a7790: Convert to new LVDS DT bindings
>   ARM: dts: r8a7791: Convert to new LVDS DT bindings
>   ARM: dts: r8a7792: Convert to new DU DT bindings
>   ARM: dts: r8a7793: Convert to new LVDS DT bindings
>   ARM: dts: r8a7794: Convert to new DU DT bindings
>   arm64: dts: renesas: r8a7795: Convert to new LVDS DT bindings
>   arm64: dts: renesas: r8a7796: Convert to new LVDS DT bindings

I have marked the dts patches above as deferred as they depend
on the driver changes not to cause a regression. Please repost them
or otherwise ping me once the driver dependencies are present in an rc
release.

I am assuming that the other patches in this series are not targeted
at the renesas tree.

> 
> Pantelis Antoniou (5):
>   of: dynamic: Add __of_node_dupv()
>   of: changesets: Introduce changeset helper methods
>   of: changeset: Add of_changeset_node_move method
>   of: unittest: changeset helpers
>   i2c: demux: Use changeset helpers for clarity

...

Re: [PATCH v4 06/16] of: unittest: changeset helpers

[Rob Herring]

On Tue, Feb 20, 2018 at 5:10 PM, Laurent Pinchart
<laurent.pinchart+renesas@ideasonboard.com> wrote:
> From: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>
> Add a unitest specific for the new changeset helpers.
>
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/of/unittest.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 54 insertions(+)
>
> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> index 7a9abaae874d..1b21d2c549a8 100644
> --- a/drivers/of/unittest.c
> +++ b/drivers/of/unittest.c
> @@ -609,6 +609,59 @@ static void __init of_unittest_changeset(void)
>  #endif
>  }
>
> +static void __init of_unittest_changeset_helper(void)
> +{
> +#ifdef CONFIG_OF_DYNAMIC

I think this can be:

if (!IS_ENABLED(CONFIG_OF_DYNAMIC))
  return;

Otherwise,

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v4 04/16] of: changesets: Introduce changeset helper methods

[Rob Herring]

On Tue, Feb 20, 2018 at 5:10 PM, Laurent Pinchart
<laurent.pinchart+renesas@ideasonboard.com> wrote:
> From: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>
> Changesets are very powerful, but the lack of a helper API
> makes using them cumbersome. Introduce a simple copy based
> API that makes things considerably easier.
>
> To wit, adding a property using the raw API.
>
>         struct property *prop;
>         prop = kzalloc(sizeof(*prop)), GFP_KERNEL);
>         prop->name = kstrdup("compatible");
>         prop->value = kstrdup("foo,bar");
>         prop->length = strlen(prop->value) + 1;
>         of_changeset_add_property(ocs, np, prop);
>
> while using the helper API
>
>         of_changeset_add_property_string(ocs, np, "compatible",
>                         "foo,bar");
>
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> [Fixed memory leak in __of_changeset_add_update_property_copy()]
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/of/dynamic.c | 222 ++++++++++++++++++++++++++++++++++
>  include/linux/of.h   | 328 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 550 insertions(+)

Other than what Geert pointed out,

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v4 03/16] of: dynamic: Add __of_node_dupv()

[Rob Herring]

On Tue, Feb 20, 2018 at 5:10 PM, Laurent Pinchart
<laurent.pinchart+renesas@ideasonboard.com> wrote:
> From: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>
> Add an __of_node_dupv() private method and make __of_node_dup() use it.
> This is required for the subsequent changeset accessors which will
> make use of it.
>
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/of/dynamic.c | 29 +++++++++++++++++++++++------
>  1 file changed, 23 insertions(+), 6 deletions(-)

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v4 05/16] of: changeset: Add of_changeset_node_move method

[Rob Herring]

On Tue, Feb 20, 2018 at 5:10 PM, Laurent Pinchart
<laurent.pinchart+renesas@ideasonboard.com> wrote:
> From: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>
> Adds a changeset helper for moving a subtree to a different place
> in the running tree. This is useful in advances cases of dynamic
> device tree construction.

This one I'm not real clear on when we'd use this and we don't have
any user, so lets drop it for now.

Rob

Re: [PATCH v4 06/16] of: unittest: changeset helpers

[Laurent Pinchart]

Hi Rob,

On Thursday, 22 February 2018 01:10:25 EET Rob Herring wrote:
> On Tue, Feb 20, 2018 at 5:10 PM, Laurent Pinchart wrote:
> > From: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> > 
> > Add a unitest specific for the new changeset helpers.
> > 
> > Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > 
> >  drivers/of/unittest.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 54 insertions(+)
> > 
> > diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> > index 7a9abaae874d..1b21d2c549a8 100644
> > --- a/drivers/of/unittest.c
> > +++ b/drivers/of/unittest.c
> > @@ -609,6 +609,59 @@ static void __init of_unittest_changeset(void)
> > 
> >  #endif
> >  }
> > 
> > +static void __init of_unittest_changeset_helper(void)
> > +{
> > +#ifdef CONFIG_OF_DYNAMIC
> 
> I think this can be:
> 
> if (!IS_ENABLED(CONFIG_OF_DYNAMIC))
>   return;

Not quite, as there are functions used below (such as of_changeset_init()) 
that are not defined if CONFIG_OF_DYNAMIC isn't enabled. We could create stubs 
in that case but I believe that's out of scope for this patch series.

> Otherwise,
> 
> Reviewed-by: Rob Herring <robh@kernel.org>

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 05/16] of: changeset: Add of_changeset_node_move method

[Laurent Pinchart]

Hi Rob,

On Thursday, 22 February 2018 01:20:36 EET Rob Herring wrote:
> On Tue, Feb 20, 2018 at 5:10 PM, Laurent Pinchart wrote:
> > From: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> > 
> > Adds a changeset helper for moving a subtree to a different place
> > in the running tree. This is useful in advances cases of dynamic
> > device tree construction.
> 
> This one I'm not real clear on when we'd use this and we don't have
> any user, so lets drop it for now.

OK, no problem.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 06/16] of: unittest: changeset helpers

[Rob Herring]

On Wed, Feb 21, 2018 at 5:39 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Rob,
>
> On Thursday, 22 February 2018 01:10:25 EET Rob Herring wrote:
>> On Tue, Feb 20, 2018 at 5:10 PM, Laurent Pinchart wrote:
>> > From: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>> >
>> > Add a unitest specific for the new changeset helpers.
>> >
>> > Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>> > Signed-off-by: Laurent Pinchart
>> > <laurent.pinchart+renesas@ideasonboard.com>
>> > ---
>> >
>> >  drivers/of/unittest.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++
>> >  1 file changed, 54 insertions(+)
>> >
>> > diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
>> > index 7a9abaae874d..1b21d2c549a8 100644
>> > --- a/drivers/of/unittest.c
>> > +++ b/drivers/of/unittest.c
>> > @@ -609,6 +609,59 @@ static void __init of_unittest_changeset(void)
>> >
>> >  #endif
>> >  }
>> >
>> > +static void __init of_unittest_changeset_helper(void)
>> > +{
>> > +#ifdef CONFIG_OF_DYNAMIC
>>
>> I think this can be:
>>
>> if (!IS_ENABLED(CONFIG_OF_DYNAMIC))
>>   return;
>
> Not quite, as there are functions used below (such as of_changeset_init())
> that are not defined if CONFIG_OF_DYNAMIC isn't enabled. We could create stubs
> in that case but I believe that's out of scope for this patch series.

Okay. I thought we had the necessary stubs, but didn't check too closely.

>
>> Otherwise,
>>
>> Reviewed-by: Rob Herring <robh@kernel.org>
>
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH v4 00/16] R-Car DU: Convert LVDS code to bridge driver

[Frank Rowand]

On 02/20/18 15:10, Laurent Pinchart wrote:
> Hello,
> 
> This patch series addresses a design mistake that dates back from the initial
> DU support. Support for the LVDS encoders, which are IP cores separate from
> the DU, was bundled in the DU driver. Worse, both the DU and LVDS were
> described through a single DT node.
> 
> To fix the, patches 01/16 and 02/16 define new DT bindings for the LVDS
> encoders, and deprecate their description inside the DU bindings. To retain
> backward compatibility with existing DT, patches 03/16 to 08/16 then patch the
> device tree at runtime to convert the legacy bindings to the new ones.
> 
> With the DT side addressed, patch 09/16 converts the LVDS support code to a
> separate bridge driver. Patches 11/16 to 16/16 then update all the device tree
> sources to the new DU and LVDS encoders bindings.
> 
> I decided to go for live DT patching in patch 08/16 because implementing
> support for both the legacy and new bindings in the driver would have been
> very intrusive, and prevented further cleanups. This version relies more
> heavily on overlays to avoid touching the internals of the OF core compared to
> v2, even if manual fixes to the device tree are still needed.
> 
> Compared to v3, this series uses the OF changeset API to update properties
> instead of accessing the internals of the property structure. This removes the
> local implementation of functions to look up nodes by path and update
> properties. In order to do this, I pulled in Pantelis' patch series titled
> "[PATCH v2 0/5] of: dynamic: Changesets helpers & fixes" at Rob's request, and
> rebased it while taking two small review comments into account.

Wait a minute!  Why are you putting a patch set to modify core devicetree
in the middle of a driver series.  Please pull it out to a separate series.

I'll try to look at the patches, as they are in this series, sometime
tomorrow.  I have a vague memory of unresolved issues from the last
time they were proposed.

Thanks,

Frank


> 
> Rob, I'd like this series to be merged in v4.17. As the changeset helpers are
> now a dependency, I'd need you to merge them early (ideally on top of
> v4.16-rc1) and provide a stable branch, or get your ack to merge them through
> Dave's tree if they don't conflict with what you have and will queue for
> v4.17.
> 
> This version also drops the small fix to the Porter board device tree that has
> been queued for v4.17 already.
> 
> Compared to v2, the biggest change is in patch 03/16. Following Rob's and
> Frank's reviews it was clear that modifying the unflattened DT structure of
> the overlay before applying it wasn't popular. I have thus decided to use one
> overlay source per SoC to move as much of the DT changes to the overlay as
> possible, and only perform manual modifications (that are still needed as some
> of the information is board-specific) on the system DT after applying the
> overlay. As a result the overlay is parsed and applied without being modified.
> 
> Compared to v1, this series update the r8a7792 and r8a7794 device tree sources
> and incorporate review feedback as described by the changelogs of individual
> patches.
> 
> 
> Laurent Pinchart (11):
>   dt-bindings: display: renesas: Add R-Car LVDS encoder DT bindings
>   dt-bindings: display: renesas: Deprecate LVDS support in the DU
>     bindings
>   drm: rcar-du: Fix legacy DT to create LVDS encoder nodes
>   drm: rcar-du: Convert LVDS encoder code to bridge driver
>   ARM: dts: r8a7790: Convert to new LVDS DT bindings
>   ARM: dts: r8a7791: Convert to new LVDS DT bindings
>   ARM: dts: r8a7792: Convert to new DU DT bindings
>   ARM: dts: r8a7793: Convert to new LVDS DT bindings
>   ARM: dts: r8a7794: Convert to new DU DT bindings
>   arm64: dts: renesas: r8a7795: Convert to new LVDS DT bindings
>   arm64: dts: renesas: r8a7796: Convert to new LVDS DT bindings
> 
> Pantelis Antoniou (5):
>   of: dynamic: Add __of_node_dupv()
>   of: changesets: Introduce changeset helper methods
>   of: changeset: Add of_changeset_node_move method
>   of: unittest: changeset helpers
>   i2c: demux: Use changeset helpers for clarity
> 
>  .../bindings/display/bridge/renesas,lvds.txt       |  56 +++
>  .../devicetree/bindings/display/renesas,du.txt     |  31 +-
>  MAINTAINERS                                        |   1 +
>  arch/arm/boot/dts/r8a7790-lager.dts                |  22 +-
>  arch/arm/boot/dts/r8a7790.dtsi                     |  64 ++-
>  arch/arm/boot/dts/r8a7791-koelsch.dts              |  10 +-
>  arch/arm/boot/dts/r8a7791-porter.dts               |  16 +-
>  arch/arm/boot/dts/r8a7791.dtsi                     |  36 +-
>  arch/arm/boot/dts/r8a7792.dtsi                     |   1 -
>  arch/arm/boot/dts/r8a7793-gose.dts                 |  10 +-
>  arch/arm/boot/dts/r8a7793.dtsi                     |  37 +-
>  arch/arm/boot/dts/r8a7794.dtsi                     |   1 -
>  .../boot/dts/renesas/r8a7795-es1-salvator-x.dts    |   3 +-
>  arch/arm64/boot/dts/renesas/r8a7795-h3ulcb.dts     |   3 +-
>  arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts |   3 +-
>  .../arm64/boot/dts/renesas/r8a7795-salvator-xs.dts |   3 +-
>  arch/arm64/boot/dts/renesas/r8a7795.dtsi           |  36 +-
>  arch/arm64/boot/dts/renesas/r8a7796-m3ulcb.dts     |   3 +-
>  arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts |   3 +-
>  arch/arm64/boot/dts/renesas/r8a7796.dtsi           |  36 +-
>  drivers/gpu/drm/rcar-du/Kconfig                    |   6 +-
>  drivers/gpu/drm/rcar-du/Makefile                   |  10 +-
>  drivers/gpu/drm/rcar-du/rcar_du_drv.c              |  21 +-
>  drivers/gpu/drm/rcar-du/rcar_du_drv.h              |   5 -
>  drivers/gpu/drm/rcar-du/rcar_du_encoder.c          | 175 +------
>  drivers/gpu/drm/rcar-du/rcar_du_encoder.h          |  12 -
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c              |  14 +-
>  drivers/gpu/drm/rcar-du/rcar_du_lvdscon.c          |  93 ----
>  drivers/gpu/drm/rcar-du/rcar_du_lvdscon.h          |  24 -
>  drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c          | 238 ----------
>  drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.h          |  64 ---
>  drivers/gpu/drm/rcar-du/rcar_du_of.c               | 307 ++++++++++++
>  drivers/gpu/drm/rcar-du/rcar_du_of.h               |  20 +
>  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7790.dts    |  81 ++++
>  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7791.dts    |  55 +++
>  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7793.dts    |  55 +++
>  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7795.dts    |  55 +++
>  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7796.dts    |  55 +++
>  drivers/gpu/drm/rcar-du/rcar_lvds.c                | 524 +++++++++++++++++++++
>  drivers/i2c/muxes/i2c-demux-pinctrl.c              |  12 +-
>  drivers/of/dynamic.c                               | 317 ++++++++++++-
>  drivers/of/unittest.c                              |  54 +++
>  include/linux/of.h                                 | 337 +++++++++++++
>  43 files changed, 2199 insertions(+), 710 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt
>  delete mode 100644 drivers/gpu/drm/rcar-du/rcar_du_lvdscon.c
>  delete mode 100644 drivers/gpu/drm/rcar-du/rcar_du_lvdscon.h
>  delete mode 100644 drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c
>  delete mode 100644 drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.h
>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of.c
>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of.h
>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7790.dts
>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7791.dts
>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7793.dts
>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7795.dts
>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7796.dts
>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_lvds.c
> 

Re: [PATCH v4 00/16] R-Car DU: Convert LVDS code to bridge driver

[Laurent Pinchart]

Hi Frank,

On Thursday, 22 February 2018 08:07:14 EET Frank Rowand wrote:
> On 02/20/18 15:10, Laurent Pinchart wrote:
> > Hello,
> > 
> > This patch series addresses a design mistake that dates back from the
> > initial DU support. Support for the LVDS encoders, which are IP cores
> > separate from the DU, was bundled in the DU driver. Worse, both the DU
> > and LVDS were described through a single DT node.
> > 
> > To fix the, patches 01/16 and 02/16 define new DT bindings for the LVDS
> > encoders, and deprecate their description inside the DU bindings. To
> > retain backward compatibility with existing DT, patches 03/16 to 08/16
> > then patch the device tree at runtime to convert the legacy bindings to
> > the new ones.
> > 
> > With the DT side addressed, patch 09/16 converts the LVDS support code to
> > a separate bridge driver. Patches 11/16 to 16/16 then update all the
> > device tree sources to the new DU and LVDS encoders bindings.
> > 
> > I decided to go for live DT patching in patch 08/16 because implementing
> > support for both the legacy and new bindings in the driver would have been
> > very intrusive, and prevented further cleanups. This version relies more
> > heavily on overlays to avoid touching the internals of the OF core
> > compared to v2, even if manual fixes to the device tree are still needed.
> > 
> > Compared to v3, this series uses the OF changeset API to update properties
> > instead of accessing the internals of the property structure. This removes
> > the local implementation of functions to look up nodes by path and update
> > properties. In order to do this, I pulled in Pantelis' patch series
> > titled "[PATCH v2 0/5] of: dynamic: Changesets helpers & fixes" at Rob's
> > request, and rebased it while taking two small review comments into
> > account.
> 
> Wait a minute!  Why are you putting a patch set to modify core devicetree
> in the middle of a driver series.  Please pull it out to a separate series.

Because Rob asked for the driver-local implementation of the property add 
function to be replaced by Pantelis' series. I want to get the LVDS changes in 
v4.17 and asked Rob whether I could then take the OF changeset patches merged 
through the DRM tree, and he didn't object. If that causes an issue I'll 
switch back to the driver-local implementation to get the driver changes 
merged, split the OF changeset series out, and then move to the OF changeset 
API once merged. Would you prefer that ?

> I'll try to look at the patches, as they are in this series, sometime
> tomorrow.  I have a vague memory of unresolved issues from the last
> time they were proposed.
> 
> > Rob, I'd like this series to be merged in v4.17. As the changeset helpers
> > are now a dependency, I'd need you to merge them early (ideally on top of
> > v4.16-rc1) and provide a stable branch, or get your ack to merge them
> > through Dave's tree if they don't conflict with what you have and will
> > queue for v4.17.
> > 
> > This version also drops the small fix to the Porter board device tree that
> > has been queued for v4.17 already.
> > 
> > Compared to v2, the biggest change is in patch 03/16. Following Rob's and
> > Frank's reviews it was clear that modifying the unflattened DT structure
> > of the overlay before applying it wasn't popular. I have thus decided to
> > use one overlay source per SoC to move as much of the DT changes to the
> > overlay as possible, and only perform manual modifications (that are
> > still needed as some of the information is board-specific) on the system
> > DT after applying the overlay. As a result the overlay is parsed and
> > applied without being modified.
> > 
> > Compared to v1, this series update the r8a7792 and r8a7794 device tree
> > sources and incorporate review feedback as described by the changelogs of
> > individual patches.
> > 
> > Laurent Pinchart (11):
> >   dt-bindings: display: renesas: Add R-Car LVDS encoder DT bindings
> >   dt-bindings: display: renesas: Deprecate LVDS support in the DU
> >     bindings
> >   drm: rcar-du: Fix legacy DT to create LVDS encoder nodes
> >   drm: rcar-du: Convert LVDS encoder code to bridge driver
> >   ARM: dts: r8a7790: Convert to new LVDS DT bindings
> >   ARM: dts: r8a7791: Convert to new LVDS DT bindings
> >   ARM: dts: r8a7792: Convert to new DU DT bindings
> >   ARM: dts: r8a7793: Convert to new LVDS DT bindings
> >   ARM: dts: r8a7794: Convert to new DU DT bindings
> >   arm64: dts: renesas: r8a7795: Convert to new LVDS DT bindings
> >   arm64: dts: renesas: r8a7796: Convert to new LVDS DT bindings
> > 
> > Pantelis Antoniou (5):
> >   of: dynamic: Add __of_node_dupv()
> >   of: changesets: Introduce changeset helper methods
> >   of: changeset: Add of_changeset_node_move method
> >   of: unittest: changeset helpers
> >   i2c: demux: Use changeset helpers for clarity
> >  
> >  .../bindings/display/bridge/renesas,lvds.txt       |  56 +++
> >  .../devicetree/bindings/display/renesas,du.txt     |  31 +-
> >  MAINTAINERS                                        |   1 +
> >  arch/arm/boot/dts/r8a7790-lager.dts                |  22 +-
> >  arch/arm/boot/dts/r8a7790.dtsi                     |  64 ++-
> >  arch/arm/boot/dts/r8a7791-koelsch.dts              |  10 +-
> >  arch/arm/boot/dts/r8a7791-porter.dts               |  16 +-
> >  arch/arm/boot/dts/r8a7791.dtsi                     |  36 +-
> >  arch/arm/boot/dts/r8a7792.dtsi                     |   1 -
> >  arch/arm/boot/dts/r8a7793-gose.dts                 |  10 +-
> >  arch/arm/boot/dts/r8a7793.dtsi                     |  37 +-
> >  arch/arm/boot/dts/r8a7794.dtsi                     |   1 -
> >  .../boot/dts/renesas/r8a7795-es1-salvator-x.dts    |   3 +-
> >  arch/arm64/boot/dts/renesas/r8a7795-h3ulcb.dts     |   3 +-
> >  arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts |   3 +-
> >  .../arm64/boot/dts/renesas/r8a7795-salvator-xs.dts |   3 +-
> >  arch/arm64/boot/dts/renesas/r8a7795.dtsi           |  36 +-
> >  arch/arm64/boot/dts/renesas/r8a7796-m3ulcb.dts     |   3 +-
> >  arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts |   3 +-
> >  arch/arm64/boot/dts/renesas/r8a7796.dtsi           |  36 +-
> >  drivers/gpu/drm/rcar-du/Kconfig                    |   6 +-
> >  drivers/gpu/drm/rcar-du/Makefile                   |  10 +-
> >  drivers/gpu/drm/rcar-du/rcar_du_drv.c              |  21 +-
> >  drivers/gpu/drm/rcar-du/rcar_du_drv.h              |   5 -
> >  drivers/gpu/drm/rcar-du/rcar_du_encoder.c          | 175 +------
> >  drivers/gpu/drm/rcar-du/rcar_du_encoder.h          |  12 -
> >  drivers/gpu/drm/rcar-du/rcar_du_kms.c              |  14 +-
> >  drivers/gpu/drm/rcar-du/rcar_du_lvdscon.c          |  93 ----
> >  drivers/gpu/drm/rcar-du/rcar_du_lvdscon.h          |  24 -
> >  drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c          | 238 ----------
> >  drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.h          |  64 ---
> >  drivers/gpu/drm/rcar-du/rcar_du_of.c               | 307 ++++++++++++
> >  drivers/gpu/drm/rcar-du/rcar_du_of.h               |  20 +
> >  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7790.dts    |  81 ++++
> >  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7791.dts    |  55 +++
> >  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7793.dts    |  55 +++
> >  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7795.dts    |  55 +++
> >  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7796.dts    |  55 +++
> >  drivers/gpu/drm/rcar-du/rcar_lvds.c                | 524 ++++++++++++++++
> >  drivers/i2c/muxes/i2c-demux-pinctrl.c              |  12 +-
> >  drivers/of/dynamic.c                               | 317 ++++++++++++-
> >  drivers/of/unittest.c                              |  54 +++
> >  include/linux/of.h                                 | 337 +++++++++++++
> >  43 files changed, 2199 insertions(+), 710 deletions(-)
> >  create mode 100644
> >  Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt delete
> >  mode 100644 drivers/gpu/drm/rcar-du/rcar_du_lvdscon.c
> >  delete mode 100644 drivers/gpu/drm/rcar-du/rcar_du_lvdscon.h
> >  delete mode 100644 drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c
> >  delete mode 100644 drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.h
> >  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of.c
> >  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of.h
> >  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7790.dts
> >  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7791.dts
> >  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7793.dts
> >  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7795.dts
> >  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7796.dts
> >  create mode 100644 drivers/gpu/drm/rcar-du/rcar_lvds.c

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 00/16] R-Car DU: Convert LVDS code to bridge driver

[Frank Rowand]

Hi Laurent,

On 02/22/18 02:25, Laurent Pinchart wrote:
> Hi Frank,
> 
> On Thursday, 22 February 2018 08:07:14 EET Frank Rowand wrote:
>> On 02/20/18 15:10, Laurent Pinchart wrote:
>>> Hello,
>>>
>>> This patch series addresses a design mistake that dates back from the
>>> initial DU support. Support for the LVDS encoders, which are IP cores
>>> separate from the DU, was bundled in the DU driver. Worse, both the DU
>>> and LVDS were described through a single DT node.
>>>
>>> To fix the, patches 01/16 and 02/16 define new DT bindings for the LVDS
>>> encoders, and deprecate their description inside the DU bindings. To
>>> retain backward compatibility with existing DT, patches 03/16 to 08/16
>>> then patch the device tree at runtime to convert the legacy bindings to
>>> the new ones.
>>>
>>> With the DT side addressed, patch 09/16 converts the LVDS support code to
>>> a separate bridge driver. Patches 11/16 to 16/16 then update all the
>>> device tree sources to the new DU and LVDS encoders bindings.
>>>
>>> I decided to go for live DT patching in patch 08/16 because implementing
>>> support for both the legacy and new bindings in the driver would have been
>>> very intrusive, and prevented further cleanups. This version relies more
>>> heavily on overlays to avoid touching the internals of the OF core
>>> compared to v2, even if manual fixes to the device tree are still needed.
>>>
>>> Compared to v3, this series uses the OF changeset API to update properties
>>> instead of accessing the internals of the property structure. This removes
>>> the local implementation of functions to look up nodes by path and update
>>> properties. In order to do this, I pulled in Pantelis' patch series
>>> titled "[PATCH v2 0/5] of: dynamic: Changesets helpers & fixes" at Rob's
>>> request, and rebased it while taking two small review comments into
>>> account.
>>
>> Wait a minute!  Why are you putting a patch set to modify core devicetree
>> in the middle of a driver series.  Please pull it out to a separate series.
> 
> Because Rob asked for the driver-local implementation of the property add 
> function to be replaced by Pantelis' series. I want to get the LVDS changes in 
> v4.17 and asked Rob whether I could then take the OF changeset patches merged 
> through the DRM tree, and he didn't object. If that causes an issue I'll 
> switch back to the driver-local implementation to get the driver changes 
> merged, split the OF changeset series out, and then move to the OF changeset 
> API once merged. Would you prefer that ?

You have already created a new version of the R-Car patches without the
set of patches that I was objecting to here.  So this is somewhat just
an academic comment.

As I mentioned in the v6 thread, I am coming back here to clean up loose
ends, and explain why I had the reaction I had.  Basically, this is a
process issue to me.

(1) The patch set from Pantelis is "hidden" in the driver patch series.
When viewing collapsed threads (which is my normal mode to avoid getting
overwhelmed by the vast volume of email I scan), the Pantelis patch set
is totally invisible.  If the R-Car driver patch series had not had me
on the to: list, there is a very good chance I would not have noticed
it.  Or noticed in a more delayed time frame.  And the same applies to
anyone else who might be subscribed to the devicetree mail list.  If
the Pantelis patch series was split out into a separate patch set then
it would be more visible on the list.

(2) There is no good way to indicate in the email subject lines for
the Pantelis patches that they are version 3 of the series, since
they are also version 4 of the R-Car patch series.  If one reads the
patch 0/0 header carefully, and/or the other Pantelis patch comment
headers carefully, and then does a little detective work, it is
possible to find version 2 of the Pantelis patch series, and thus
be able to track the full history.  If just glancing at each
individual patch email subject, scanning the patch comment, and
spending more time reading the body of the patch, it would be
very easy to overlook the existence of the previous versions on
the mail list.

(2b) Small quibble:  if the Pantelis patches were in a separate series,
with v3 in the subject header, then you would have normally put
a "changes since v2" section in the patch comment header, which would
have been more visible and less cryptic than what you wrote, which was:

   ...

   Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
   [Fixed memory leak in __of_changeset_add_update_property_copy()]
   Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
   ---
    drivers/of/dynamic.c | 222 ++++++++++++++++++++++++++++++++++
    include/linux/of.h   | 328 +++++++++++++++++++++++++++++++++++++++++++++++++++
    2 files changed, 550 insertions(+)

My original scan of version 2 and then the email in this series had me
questioning whether the two open issues from patch 2 had been addressed
(yes, patch 0/16 did explicitly mention that 2 review comments had been
taken into account, so you can point at my poor reading comprehension).

(3) This is totally unrelated to your patch series, but I'm leaving a bread
crumb trail here.  Rob pointed you at a patch series that was not the most
recent version.  When this patch series appears again, there already is
a version 3 subset of it, but it is still labeled as being version 2, and
also has outstanding un-addressed issues.  ("[PATCH v2 1/2] of: dynamic:
Add __of_node_dupv()", 11/04/16)

-Frank

Re: [PATCH v4 00/16] R-Car DU: Convert LVDS code to bridge driver

[Laurent Pinchart]

Hi Frank,

On Friday, 23 February 2018 05:20:43 EET Frank Rowand wrote:
> On 02/22/18 02:25, Laurent Pinchart wrote:
> > On Thursday, 22 February 2018 08:07:14 EET Frank Rowand wrote:
> >> On 02/20/18 15:10, Laurent Pinchart wrote:
> >>> Hello,
> >>> 
> >>> This patch series addresses a design mistake that dates back from the
> >>> initial DU support. Support for the LVDS encoders, which are IP cores
> >>> separate from the DU, was bundled in the DU driver. Worse, both the DU
> >>> and LVDS were described through a single DT node.
> >>> 
> >>> To fix the, patches 01/16 and 02/16 define new DT bindings for the LVDS
> >>> encoders, and deprecate their description inside the DU bindings. To
> >>> retain backward compatibility with existing DT, patches 03/16 to 08/16
> >>> then patch the device tree at runtime to convert the legacy bindings to
> >>> the new ones.
> >>> 
> >>> With the DT side addressed, patch 09/16 converts the LVDS support code
> >>> to a separate bridge driver. Patches 11/16 to 16/16 then update all the
> >>> device tree sources to the new DU and LVDS encoders bindings.
> >>> 
> >>> I decided to go for live DT patching in patch 08/16 because implementing
> >>> support for both the legacy and new bindings in the driver would have
> >>> been very intrusive, and prevented further cleanups. This version relies
> >>> more heavily on overlays to avoid touching the internals of the OF core
> >>> compared to v2, even if manual fixes to the device tree are still
> >>> needed.
> >>> 
> >>> Compared to v3, this series uses the OF changeset API to update
> >>> properties instead of accessing the internals of the property structure.
> >>> This removes the local implementation of functions to look up nodes by
> >>> path and update properties. In order to do this, I pulled in Pantelis'
> >>> patch series titled "[PATCH v2 0/5] of: dynamic: Changesets helpers &
> >>> fixes" at Rob's request, and rebased it while taking two small review
> >>> comments into account.
> >> 
> >> Wait a minute!  Why are you putting a patch set to modify core devicetree
> >> in the middle of a driver series.  Please pull it out to a separate
> >> series.
> > 
> > Because Rob asked for the driver-local implementation of the property add
> > function to be replaced by Pantelis' series. I want to get the LVDS
> > changes in v4.17 and asked Rob whether I could then take the OF changeset
> > patches merged through the DRM tree, and he didn't object. If that causes
> > an issue I'll switch back to the driver-local implementation to get the
> > driver changes merged, split the OF changeset series out, and then move
> > to the OF changeset API once merged. Would you prefer that ?
> 
> You have already created a new version of the R-Car patches without the
> set of patches that I was objecting to here.  So this is somewhat just
> an academic comment.
> 
> As I mentioned in the v6 thread, I am coming back here to clean up loose
> ends, and explain why I had the reaction I had.  Basically, this is a
> process issue to me.
> 
> (1) The patch set from Pantelis is "hidden" in the driver patch series.
> When viewing collapsed threads (which is my normal mode to avoid getting
> overwhelmed by the vast volume of email I scan), the Pantelis patch set
> is totally invisible.  If the R-Car driver patch series had not had me
> on the to: list, there is a very good chance I would not have noticed
> it.  Or noticed in a more delayed time frame.  And the same applies to
> anyone else who might be subscribed to the devicetree mail list.  If
> the Pantelis patch series was split out into a separate patch set then
> it would be more visible on the list.
> 
> (2) There is no good way to indicate in the email subject lines for
> the Pantelis patches that they are version 3 of the series, since
> they are also version 4 of the R-Car patch series.  If one reads the
> patch 0/0 header carefully, and/or the other Pantelis patch comment
> headers carefully, and then does a little detective work, it is
> possible to find version 2 of the Pantelis patch series, and thus
> be able to track the full history.  If just glancing at each
> individual patch email subject, scanning the patch comment, and
> spending more time reading the body of the patch, it would be
> very easy to overlook the existence of the previous versions on
> the mail list.
> 
> (2b) Small quibble:  if the Pantelis patches were in a separate series,
> with v3 in the subject header, then you would have normally put
> a "changes since v2" section in the patch comment header, which would
> have been more visible and less cryptic than what you wrote, which was:
> 
>    ...
> 
>    Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>    [Fixed memory leak in __of_changeset_add_update_property_copy()]
>    Signed-off-by: Laurent Pinchart
> <laurent.pinchart+renesas@ideasonboard.com>
>    ---
>     drivers/of/dynamic.c | 222 ++++++++++++++++++++++++++++++++++
>     include/linux/of.h   | 328 +++++++++++++++++++++++++++++++++++++++++++++
>     2 files changed, 550 insertions(+)
> 
> My original scan of version 2 and then the email in this series had me
> questioning whether the two open issues from patch 2 had been addressed
> (yes, patch 0/16 did explicitly mention that 2 review comments had been
> taken into account, so you can point at my poor reading comprehension).

All points dully noted, I'll make sure not to include similar patches in the 
middle of a driver series should I need to rework a patch series previously 
posted by someone else.

I assume your comments also apply to patches to drivers/of/ that are developed 
as part of a drivers series, not merely pulled in it ? I have mixed feelings 
about this, as in many subsystems changes to the core require at least one 
user. Always splitting core and driver changes in separate series would thus 
often be impractical.

I doubt we will find a single process that will suit all subsystem 
maintainers, so maybe this should be a per-subsystem decision ?

> (3) This is totally unrelated to your patch series, but I'm leaving a bread
> crumb trail here.  Rob pointed you at a patch series that was not the most
> recent version.  When this patch series appears again, there already is
> a version 3 subset of it, but it is still labeled as being version 2, and
> also has outstanding un-addressed issues.  ("[PATCH v2 1/2] of: dynamic:
> Add __of_node_dupv()", 11/04/16)

Ah, thank you for the information. I was looking for a v3 but missed that one 
as it was labeled v2.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 00/16] R-Car DU: Convert LVDS code to bridge driver

[Frank Rowand]

On 02/23/18 01:25, Laurent Pinchart wrote:
> Hi Frank,
> 
> On Friday, 23 February 2018 05:20:43 EET Frank Rowand wrote:
>> On 02/22/18 02:25, Laurent Pinchart wrote:
>>> On Thursday, 22 February 2018 08:07:14 EET Frank Rowand wrote:
>>>> On 02/20/18 15:10, Laurent Pinchart wrote:
>>>>> Hello,
>>>>>
>>>>> This patch series addresses a design mistake that dates back from the
>>>>> initial DU support. Support for the LVDS encoders, which are IP cores
>>>>> separate from the DU, was bundled in the DU driver. Worse, both the DU
>>>>> and LVDS were described through a single DT node.
>>>>>
>>>>> To fix the, patches 01/16 and 02/16 define new DT bindings for the LVDS
>>>>> encoders, and deprecate their description inside the DU bindings. To
>>>>> retain backward compatibility with existing DT, patches 03/16 to 08/16
>>>>> then patch the device tree at runtime to convert the legacy bindings to
>>>>> the new ones.
>>>>>
>>>>> With the DT side addressed, patch 09/16 converts the LVDS support code
>>>>> to a separate bridge driver. Patches 11/16 to 16/16 then update all the
>>>>> device tree sources to the new DU and LVDS encoders bindings.
>>>>>
>>>>> I decided to go for live DT patching in patch 08/16 because implementing
>>>>> support for both the legacy and new bindings in the driver would have
>>>>> been very intrusive, and prevented further cleanups. This version relies
>>>>> more heavily on overlays to avoid touching the internals of the OF core
>>>>> compared to v2, even if manual fixes to the device tree are still
>>>>> needed.
>>>>>
>>>>> Compared to v3, this series uses the OF changeset API to update
>>>>> properties instead of accessing the internals of the property structure.
>>>>> This removes the local implementation of functions to look up nodes by
>>>>> path and update properties. In order to do this, I pulled in Pantelis'
>>>>> patch series titled "[PATCH v2 0/5] of: dynamic: Changesets helpers &
>>>>> fixes" at Rob's request, and rebased it while taking two small review
>>>>> comments into account.
>>>>
>>>> Wait a minute!  Why are you putting a patch set to modify core devicetree
>>>> in the middle of a driver series.  Please pull it out to a separate
>>>> series.
>>>
>>> Because Rob asked for the driver-local implementation of the property add
>>> function to be replaced by Pantelis' series. I want to get the LVDS
>>> changes in v4.17 and asked Rob whether I could then take the OF changeset
>>> patches merged through the DRM tree, and he didn't object. If that causes
>>> an issue I'll switch back to the driver-local implementation to get the
>>> driver changes merged, split the OF changeset series out, and then move
>>> to the OF changeset API once merged. Would you prefer that ?
>>
>> You have already created a new version of the R-Car patches without the
>> set of patches that I was objecting to here.  So this is somewhat just
>> an academic comment.
>>
>> As I mentioned in the v6 thread, I am coming back here to clean up loose
>> ends, and explain why I had the reaction I had.  Basically, this is a
>> process issue to me.
>>
>> (1) The patch set from Pantelis is "hidden" in the driver patch series.
>> When viewing collapsed threads (which is my normal mode to avoid getting
>> overwhelmed by the vast volume of email I scan), the Pantelis patch set
>> is totally invisible.  If the R-Car driver patch series had not had me
>> on the to: list, there is a very good chance I would not have noticed
>> it.  Or noticed in a more delayed time frame.  And the same applies to
>> anyone else who might be subscribed to the devicetree mail list.  If
>> the Pantelis patch series was split out into a separate patch set then
>> it would be more visible on the list.
>>
>> (2) There is no good way to indicate in the email subject lines for
>> the Pantelis patches that they are version 3 of the series, since
>> they are also version 4 of the R-Car patch series.  If one reads the
>> patch 0/0 header carefully, and/or the other Pantelis patch comment
>> headers carefully, and then does a little detective work, it is
>> possible to find version 2 of the Pantelis patch series, and thus
>> be able to track the full history.  If just glancing at each
>> individual patch email subject, scanning the patch comment, and
>> spending more time reading the body of the patch, it would be
>> very easy to overlook the existence of the previous versions on
>> the mail list.
>>
>> (2b) Small quibble:  if the Pantelis patches were in a separate series,
>> with v3 in the subject header, then you would have normally put
>> a "changes since v2" section in the patch comment header, which would
>> have been more visible and less cryptic than what you wrote, which was:
>>
>>    ...
>>
>>    Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>>    [Fixed memory leak in __of_changeset_add_update_property_copy()]
>>    Signed-off-by: Laurent Pinchart
>> <laurent.pinchart+renesas@ideasonboard.com>
>>    ---
>>     drivers/of/dynamic.c | 222 ++++++++++++++++++++++++++++++++++
>>     include/linux/of.h   | 328 +++++++++++++++++++++++++++++++++++++++++++++
>>     2 files changed, 550 insertions(+)
>>
>> My original scan of version 2 and then the email in this series had me
>> questioning whether the two open issues from patch 2 had been addressed
>> (yes, patch 0/16 did explicitly mention that 2 review comments had been
>> taken into account, so you can point at my poor reading comprehension).
> 
> All points dully noted, I'll make sure not to include similar patches in the 
> middle of a driver series should I need to rework a patch series previously 
> posted by someone else.
> 
> I assume your comments also apply to patches to drivers/of/ that are developed 
> as part of a drivers series, not merely pulled in it ?

Yes, as a normal process.  Of course we find cases where following normal process
causes problems, and it can be better to do something different in those cases.


> I have mixed feelings 
> about this, as in many subsystems changes to the core require at least one 
> user. Always splitting core and driver changes in separate series would thus 
> often be impractical.

If at all reasonable, I would still prefer to see two patch series for this
case.  I would expect both series to be upstreamed by a single maintainer
(with the two involved maintainers coordinating this process).  I have not
thought this through in any detail, so I don't know if I'm missing a bunch
of issues.  One issue that I can think of is clearly documenting which
version of the subsystem patch series any given version of the driver patch
series depends upon.  That should be easy to note in patch 0 of the driver
series.  Do you have experience with any other issues in this scenario?

This sounds like a good topic to include in the "Guide to Being A Linux Kernel
Maintainer".


> I doubt we will find a single process that will suit all subsystem 
> maintainers, so maybe this should be a per-subsystem decision ?

Totally agree.  And even for me, I see it as a guideline or normal process,
with exceptions made where it makes sense.


>> (3) This is totally unrelated to your patch series, but I'm leaving a bread
>> crumb trail here.  Rob pointed you at a patch series that was not the most
>> recent version.  When this patch series appears again, there already is
>> a version 3 subset of it, but it is still labeled as being version 2, and
>> also has outstanding un-addressed issues.  ("[PATCH v2 1/2] of: dynamic:
>> Add __of_node_dupv()", 11/04/16)
> 
> Ah, thank you for the information. I was looking for a v3 but missed that one 
> as it was labeled v2.
>