[PATCH v8 0/6] software node: add support for reference properties

[PATCH v8 0/6] software node: add support for reference properties

[Dmitry Torokhov]

These series implement "references" properties for software nodes as true
properties, instead of managing them completely separately.

The first 2 patches do away with separate handling of arrays vs
single-value properties, and treat everything as arrays (which really
matches how we handle OF or ACPI properties). Instead we recognize that
data, if it is small enough, may be embedded into property_entry
structure. As a side effect, properties can be converted from having their
data stored separately to embedding their data when they are being copied.

Patch #3 implements PROPERTY_ENTRY_REF() and friends; patch #4 converts
the user of references to the property syntax, and patch #5 removes the
remains of references as entities that are managed separately.

Patch #6 adds unit tests to verify that the handling of property
entries is correct.

Note that patches #4 and $5 can be postponed.

Changes in v8:
- switch data members of "value" union to be arrays to clearly show
  that even when embedding we may be dealing with more than one element
- when parsing references use property_entry_read_int_array() instead of
  fetching u32 value directly from nargs property

Changes in v7:
- rebased onto next-20191107
- dropped already applied patches
- reworked patch that moved small properties inline on copying to
  avoid temporary allocation
- cleaned up logic for embedding vs storing values out-of-line
- fixed handling of embedded 2-element string array on x32

Changes in v6:
- rebased onto next-20191023
- fixed patch moving small properties inline
- fixed handling boolean properties after is_array -> is_inline
  conversion
- changed comments around is_inline "stored directly" vs embedded
  in one place (Andy)
- added unit tests for property entries based on KUnit framework
- added Any's reviewed-by/acked-by

Changes in v5:
- rebased onto next-20191011

Changes in v4:
- dealt with union aliasing concerns
- inline small properties on copy

Changes in v3:
- added various cleanups before implementing reference properties

Changes in v2:
- reworked code so that even single-entry reference properties are
  stored as arrays (i.e. the software_node_ref_args instances are
  not part of property_entry structure) to avoid size increase.
  From user's POV nothing is changed, one can still use PROPERTY_ENTRY_REF
  macro to define reference "inline".
- dropped unused DEV_PROP_MAX
- rebased on linux-next

Dmitry Torokhov (6):
  software node: rename is_array to is_inline
  software node: allow embedding of small arrays into property_entry
  software node: implement reference properties
  platform/x86: intel_cht_int33fe: use inline reference properties
  software node: remove separate handling of references
  software node: add basic tests for property entries

 drivers/base/swnode.c                         | 154 +++---
 drivers/base/test/Makefile                    |   2 +
 drivers/base/test/property-entry-test.c       | 472 ++++++++++++++++++
 .../platform/x86/intel_cht_int33fe_typec.c    |  81 +--
 include/linux/property.h                      |  98 ++--
 5 files changed, 655 insertions(+), 152 deletions(-)
 create mode 100644 drivers/base/test/property-entry-test.c

-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

[PATCH v8 1/6] software node: rename is_array to is_inline

[Dmitry Torokhov]

We do not need a special flag to know if we are dealing with an array,
as we can get that data from ratio between element length and the data
size, however we do need a flag to know whether the data is stored
directly inside property_entry or separately.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/base/swnode.c    | 12 +++++-------
 include/linux/property.h | 13 ++++++++-----
 2 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index d8d0dc0ca5acf..18a30fb3cc588 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -108,10 +108,7 @@ static const void *property_get_pointer(const struct property_entry *prop)
 	if (!prop->length)
 		return NULL;
 
-	if (prop->is_array)
-		return prop->pointer;
-
-	return &prop->value;
+	return prop->is_inline ? &prop->value : prop->pointer;
 }
 
 static const void *property_entry_find(const struct property_entry *props,
@@ -205,7 +202,7 @@ static void property_entry_free_data(const struct property_entry *p)
 	const char * const *src_str;
 	size_t i, nval;
 
-	if (p->is_array) {
+	if (!p->is_inline) {
 		if (p->type == DEV_PROP_STRING && p->pointer) {
 			src_str = p->pointer;
 			nval = p->length / sizeof(const char *);
@@ -250,7 +247,7 @@ static int property_entry_copy_data(struct property_entry *dst,
 	const void *pointer = property_get_pointer(src);
 	const void *new;
 
-	if (src->is_array) {
+	if (!src->is_inline) {
 		if (!src->length)
 			return -ENODATA;
 
@@ -264,15 +261,16 @@ static int property_entry_copy_data(struct property_entry *dst,
 				return -ENOMEM;
 		}
 
-		dst->is_array = true;
 		dst->pointer = new;
 	} else if (src->type == DEV_PROP_STRING) {
 		new = kstrdup(src->value.str, GFP_KERNEL);
 		if (!new && src->value.str)
 			return -ENOMEM;
 
+		dst->is_inline = true;
 		dst->value.str = new;
 	} else {
+		dst->is_inline = true;
 		dst->value = src->value;
 	}
 
diff --git a/include/linux/property.h b/include/linux/property.h
index 48335288c2a96..dad0ad11b55e2 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -227,15 +227,17 @@ static inline int fwnode_property_count_u64(const struct fwnode_handle *fwnode,
  * struct property_entry - "Built-in" device property representation.
  * @name: Name of the property.
  * @length: Length of data making up the value.
- * @is_array: True when the property is an array.
+ * @is_inline: True when the property value is embedded in
+ *	&struct property_entry instance.
  * @type: Type of the data in unions.
- * @pointer: Pointer to the property (an array of items of the given type).
- * @value: Value of the property (when it is a single item of the given type).
+ * @pointer: Pointer to the property when it is stored separately from
+ *	the &struct property_entry instance.
+ * @value: Value of the property when it is stored inline.
  */
 struct property_entry {
 	const char *name;
 	size_t length;
-	bool is_array;
+	bool is_inline;
 	enum dev_prop_type type;
 	union {
 		const void *pointer;
@@ -262,7 +264,6 @@ struct property_entry {
 (struct property_entry) {						\
 	.name = _name_,							\
 	.length = (_len_) * __PROPERTY_ENTRY_ELEMENT_SIZE(_elem_),	\
-	.is_array = true,						\
 	.type = DEV_PROP_##_Type_,					\
 	{ .pointer = _val_ },						\
 }
@@ -293,6 +294,7 @@ struct property_entry {
 (struct property_entry) {						\
 	.name = _name_,							\
 	.length = __PROPERTY_ENTRY_ELEMENT_SIZE(_elem_),		\
+	.is_inline = true,						\
 	.type = DEV_PROP_##_Type_,					\
 	{ .value = { ._elem_ = _val_ } },				\
 }
@@ -311,6 +313,7 @@ struct property_entry {
 #define PROPERTY_ENTRY_BOOL(_name_)		\
 (struct property_entry) {			\
 	.name = _name_,				\
+	.is_inline = true,			\
 }
 
 struct property_entry *
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

[PATCH v8 2/6] software node: allow embedding of small arrays into property_entry

[Dmitry Torokhov]

We should not conflate whether a property data is an array or a single
value with where it is stored (embedded into property_entry structure or
out-of-line). All single-value properties are in effect 1-element
arrays, and we can figure the amount of data stored in a property by
examining its length and the data type. And arrays can be as easily
stored in property entry instances as single values are, provided that
we have enough space (we have up to 8 bytes). We can embed:

- up to 8 bytes from U8 arrays
- up to 4 words
- up to 2 double words
- one U64 value
- one (on 64 bit architectures) or 2 (on 32 bit) strings.

This change also has an effect of switching properties with small amount
of data to embed it instead of keeping it separate when copying such
properties.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/base/swnode.c    | 115 +++++++++++++++++++--------------------
 include/linux/property.h |  14 ++---
 2 files changed, 62 insertions(+), 67 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 18a30fb3cc588..3d422918a53d9 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -198,93 +198,84 @@ static int property_entry_read_string_array(const struct property_entry *props,
 
 static void property_entry_free_data(const struct property_entry *p)
 {
-	const void *pointer = property_get_pointer(p);
 	const char * const *src_str;
 	size_t i, nval;
 
-	if (!p->is_inline) {
-		if (p->type == DEV_PROP_STRING && p->pointer) {
-			src_str = p->pointer;
-			nval = p->length / sizeof(const char *);
-			for (i = 0; i < nval; i++)
-				kfree(src_str[i]);
-		}
-		kfree(pointer);
-	} else if (p->type == DEV_PROP_STRING) {
-		kfree(p->value.str);
+	if (p->type == DEV_PROP_STRING) {
+		src_str = property_get_pointer(p);
+		nval = p->length / sizeof(*src_str);
+		for (i = 0; i < nval; i++)
+			kfree(src_str[i]);
 	}
+
+	if (!p->is_inline)
+		kfree(p->pointer);
+
 	kfree(p->name);
 }
 
-static const char * const *
-property_copy_string_array(const struct property_entry *src)
+static bool property_copy_string_array(const char **dst_ptr,
+				       const char * const *src_ptr,
+				       size_t nval)
 {
-	const char **d;
-	const char * const *src_str = src->pointer;
-	size_t nval = src->length / sizeof(*d);
 	int i;
 
-	d = kcalloc(nval, sizeof(*d), GFP_KERNEL);
-	if (!d)
-		return NULL;
-
 	for (i = 0; i < nval; i++) {
-		d[i] = kstrdup(src_str[i], GFP_KERNEL);
-		if (!d[i] && src_str[i]) {
+		dst_ptr[i] = kstrdup(src_ptr[i], GFP_KERNEL);
+		if (!dst_ptr[i] && src_ptr[i]) {
 			while (--i >= 0)
-				kfree(d[i]);
-			kfree(d);
-			return NULL;
+				kfree(dst_ptr[i]);
+			return false;
 		}
 	}
 
-	return d;
+	return true;
 }
 
 static int property_entry_copy_data(struct property_entry *dst,
 				    const struct property_entry *src)
 {
 	const void *pointer = property_get_pointer(src);
-	const void *new;
-
-	if (!src->is_inline) {
-		if (!src->length)
-			return -ENODATA;
-
-		if (src->type == DEV_PROP_STRING) {
-			new = property_copy_string_array(src);
-			if (!new)
-				return -ENOMEM;
-		} else {
-			new = kmemdup(pointer, src->length, GFP_KERNEL);
-			if (!new)
-				return -ENOMEM;
-		}
-
-		dst->pointer = new;
-	} else if (src->type == DEV_PROP_STRING) {
-		new = kstrdup(src->value.str, GFP_KERNEL);
-		if (!new && src->value.str)
+	void *dst_ptr;
+	size_t nval;
+
+	/*
+	 * Properties with no data should not be marked as stored
+	 * out of line.
+	 */
+	if (!src->is_inline && !src->length)
+		return -ENODATA;
+
+	if (src->length <= sizeof(dst->value)) {
+		dst_ptr = &dst->value;
+		dst->is_inline = true;
+	} else {
+		dst_ptr = kmalloc(src->length, GFP_KERNEL);
+		if (!dst_ptr)
 			return -ENOMEM;
+		dst->pointer = dst_ptr;
+	}
 
-		dst->is_inline = true;
-		dst->value.str = new;
+	if (src->type == DEV_PROP_STRING) {
+		nval = src->length / sizeof(const char *);
+		if (!property_copy_string_array(dst_ptr, pointer, nval)) {
+			if (!dst->is_inline)
+				kfree(dst->pointer);
+			return -ENOMEM;
+		}
 	} else {
-		dst->is_inline = true;
-		dst->value = src->value;
+		memcpy(dst_ptr, pointer, src->length);
 	}
 
 	dst->length = src->length;
 	dst->type = src->type;
 	dst->name = kstrdup(src->name, GFP_KERNEL);
-	if (!dst->name)
-		goto out_free_data;
+	if (!dst->name) {
+		property_entry_free_data(dst);
+		return -ENOMEM;
+	}
 
 	return 0;
-
-out_free_data:
-	property_entry_free_data(dst);
-	return -ENOMEM;
 }
 
 /**
@@ -484,6 +475,8 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
 	const struct software_node_reference *ref;
 	const struct property_entry *prop;
 	struct fwnode_handle *refnode;
+	u32 nargs_prop_val;
+	int error;
 	int i;
 
 	if (!swnode || !swnode->node->references)
@@ -501,11 +494,13 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
 		return -ENOENT;
 
 	if (nargs_prop) {
-		prop = property_entry_get(swnode->node->properties, nargs_prop);
-		if (!prop)
-			return -EINVAL;
+		error = property_entry_read_int_array(swnode->node->properties,
+						      nargs_prop, sizeof(u32),
+						      &nargs_prop_val, 1);
+		if (error)
+			return error;
 
-		nargs = prop->value.u32_data;
+		nargs = nargs_prop_val;
 	}
 
 	if (nargs > NR_FWNODE_REFERENCE_ARGS)
diff --git a/include/linux/property.h b/include/linux/property.h
index dad0ad11b55e2..c592c286e3394 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -242,11 +242,11 @@ struct property_entry {
 	union {
 		const void *pointer;
 		union {
-			u8 u8_data;
-			u16 u16_data;
-			u32 u32_data;
-			u64 u64_data;
-			const char *str;
+			u8 u8_data[sizeof(u64) / sizeof(u8)];
+			u16 u16_data[sizeof(u64) / sizeof(u16)];
+			u32 u32_data[sizeof(u64) / sizeof(u32)];
+			u64 u64_data[sizeof(u64) / sizeof(u64)];
+			const char *str[sizeof(u64) / sizeof(char *)];
 		} value;
 	};
 };
@@ -258,7 +258,7 @@ struct property_entry {
  */
 
 #define __PROPERTY_ENTRY_ELEMENT_SIZE(_elem_)				\
-	sizeof(((struct property_entry *)NULL)->value._elem_)
+	sizeof(((struct property_entry *)NULL)->value._elem_[0])
 
 #define __PROPERTY_ENTRY_ARRAY_LEN(_name_, _elem_, _Type_, _val_, _len_)\
 (struct property_entry) {						\
@@ -296,7 +296,7 @@ struct property_entry {
 	.length = __PROPERTY_ENTRY_ELEMENT_SIZE(_elem_),		\
 	.is_inline = true,						\
 	.type = DEV_PROP_##_Type_,					\
-	{ .value = { ._elem_ = _val_ } },				\
+	{ .value = { ._elem_[0] = _val_ } },				\
 }
 
 #define PROPERTY_ENTRY_U8(_name_, _val_)				\
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

[PATCH v8 3/6] software node: implement reference properties

[Dmitry Torokhov]

It is possible to store references to software nodes in the same fashion as
other static properties, so that users do not need to define separate
structures:

static const struct software_node gpio_bank_b_node = {
	.name = "B",
};

static const struct property_entry simone_key_enter_props[] = {
	PROPERTY_ENTRY_U32("linux,code", KEY_ENTER),
	PROPERTY_ENTRY_STRING("label", "enter"),
	PROPERTY_ENTRY_REF("gpios", &gpio_bank_b_node, 123, GPIO_ACTIVE_LOW),
	{ }
};

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/base/swnode.c    | 49 ++++++++++++++++++++++++++++------
 include/linux/property.h | 57 +++++++++++++++++++++++++++++-----------
 2 files changed, 82 insertions(+), 24 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 3d422918a53d9..604d7327bba79 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -246,6 +246,13 @@ static int property_entry_copy_data(struct property_entry *dst,
 	if (!src->is_inline && !src->length)
 		return -ENODATA;
 
+	/*
+	 * Reference properties are never stored inline as
+	 * they are too big.
+	 */
+	if (src->type == DEV_PROP_REF && src->is_inline)
+		return -EINVAL;
+
 	if (src->length <= sizeof(dst->value)) {
 		dst_ptr = &dst->value;
 		dst->is_inline = true;
@@ -473,23 +480,49 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
 {
 	struct swnode *swnode = to_swnode(fwnode);
 	const struct software_node_reference *ref;
+	const struct software_node_ref_args *ref_array;
+	const struct software_node_ref_args *ref_args;
 	const struct property_entry *prop;
 	struct fwnode_handle *refnode;
 	u32 nargs_prop_val;
 	int error;
 	int i;
 
-	if (!swnode || !swnode->node->references)
+	if (!swnode)
 		return -ENOENT;
 
-	for (ref = swnode->node->references; ref->name; ref++)
-		if (!strcmp(ref->name, propname))
-			break;
+	prop = property_entry_get(swnode->node->properties, propname);
+	if (prop) {
+		if (prop->type != DEV_PROP_REF)
+			return -EINVAL;
 
-	if (!ref->name || index > (ref->nrefs - 1))
-		return -ENOENT;
+		/*
+		 * We expect that references are never stored inline, even
+		 * single ones, as they are too big.
+		 */
+		if (prop->is_inline)
+			return -EINVAL;
+
+		if (index * sizeof(*ref_args) >= prop->length)
+			return -ENOENT;
+
+		ref_array = prop->pointer;
+		ref_args = &ref_array[index];
+	} else {
+		if (!swnode->node->references)
+			return -ENOENT;
+
+		for (ref = swnode->node->references; ref->name; ref++)
+			if (!strcmp(ref->name, propname))
+				break;
+
+		if (!ref->name || index > (ref->nrefs - 1))
+			return -ENOENT;
+
+		ref_args = &ref->refs[index];
+	}
 
-	refnode = software_node_fwnode(ref->refs[index].node);
+	refnode = software_node_fwnode(ref_args->node);
 	if (!refnode)
 		return -ENOENT;
 
@@ -510,7 +543,7 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
 	args->nargs = nargs;
 
 	for (i = 0; i < nargs; i++)
-		args->args[i] = ref->refs[index].args[i];
+		args->args[i] = ref_args->args[i];
 
 	return 0;
 }
diff --git a/include/linux/property.h b/include/linux/property.h
index c592c286e3394..19b9dcc322763 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -22,6 +22,7 @@ enum dev_prop_type {
 	DEV_PROP_U32,
 	DEV_PROP_U64,
 	DEV_PROP_STRING,
+	DEV_PROP_REF,
 };
 
 enum dev_dma_attr {
@@ -223,6 +224,20 @@ static inline int fwnode_property_count_u64(const struct fwnode_handle *fwnode,
 	return fwnode_property_read_u64_array(fwnode, propname, NULL, 0);
 }
 
+struct software_node;
+
+/**
+ * struct software_node_ref_args - Reference property with additional arguments
+ * @node: Reference to a software node
+ * @nargs: Number of elements in @args array
+ * @args: Integer arguments
+ */
+struct software_node_ref_args {
+	const struct software_node *node;
+	unsigned int nargs;
+	u64 args[NR_FWNODE_REFERENCE_ARGS];
+};
+
 /**
  * struct property_entry - "Built-in" device property representation.
  * @name: Name of the property.
@@ -260,14 +275,20 @@ struct property_entry {
 #define __PROPERTY_ENTRY_ELEMENT_SIZE(_elem_)				\
 	sizeof(((struct property_entry *)NULL)->value._elem_[0])
 
-#define __PROPERTY_ENTRY_ARRAY_LEN(_name_, _elem_, _Type_, _val_, _len_)\
+#define __PROPERTY_ENTRY_ARRAY_ELSIZE_LEN(_name_, _elsize_, _Type_,	\
+					  _val_, _len_)			\
 (struct property_entry) {						\
 	.name = _name_,							\
-	.length = (_len_) * __PROPERTY_ENTRY_ELEMENT_SIZE(_elem_),	\
+	.length = (_len_) * (_elsize_),					\
 	.type = DEV_PROP_##_Type_,					\
 	{ .pointer = _val_ },						\
 }
 
+#define __PROPERTY_ENTRY_ARRAY_LEN(_name_, _elem_, _Type_, _val_, _len_)\
+	__PROPERTY_ENTRY_ARRAY_ELSIZE_LEN(_name_,			\
+				__PROPERTY_ENTRY_ELEMENT_SIZE(_elem_),	\
+				_Type_, _val_, _len_)
+
 #define PROPERTY_ENTRY_U8_ARRAY_LEN(_name_, _val_, _len_)		\
 	__PROPERTY_ENTRY_ARRAY_LEN(_name_, u8_data, U8, _val_, _len_)
 #define PROPERTY_ENTRY_U16_ARRAY_LEN(_name_, _val_, _len_)		\
@@ -278,6 +299,10 @@ struct property_entry {
 	__PROPERTY_ENTRY_ARRAY_LEN(_name_, u64_data, U64, _val_, _len_)
 #define PROPERTY_ENTRY_STRING_ARRAY_LEN(_name_, _val_, _len_)		\
 	__PROPERTY_ENTRY_ARRAY_LEN(_name_, str, STRING, _val_, _len_)
+#define PROPERTY_ENTRY_REF_ARRAY_LEN(_name_, _val_, _len_)		\
+	__PROPERTY_ENTRY_ARRAY_ELSIZE_LEN(_name_,			\
+				sizeof(struct software_node_ref_args),	\
+				REF, _val_, _len_)
 
 #define PROPERTY_ENTRY_U8_ARRAY(_name_, _val_)				\
 	PROPERTY_ENTRY_U8_ARRAY_LEN(_name_, _val_, ARRAY_SIZE(_val_))
@@ -289,6 +314,8 @@ struct property_entry {
 	PROPERTY_ENTRY_U64_ARRAY_LEN(_name_, _val_, ARRAY_SIZE(_val_))
 #define PROPERTY_ENTRY_STRING_ARRAY(_name_, _val_)			\
 	PROPERTY_ENTRY_STRING_ARRAY_LEN(_name_, _val_, ARRAY_SIZE(_val_))
+#define PROPERTY_ENTRY_REF_ARRAY(_name_, _val_)			\
+	PROPERTY_ENTRY_REF_ARRAY_LEN(_name_, _val_, ARRAY_SIZE(_val_))
 
 #define __PROPERTY_ENTRY_ELEMENT(_name_, _elem_, _Type_, _val_)		\
 (struct property_entry) {						\
@@ -316,6 +343,18 @@ struct property_entry {
 	.is_inline = true,			\
 }
 
+#define PROPERTY_ENTRY_REF(_name_, _ref_, ...)				\
+(struct property_entry) {						\
+	.name = _name_,							\
+	.length = sizeof(struct software_node_ref_args),		\
+	.type = DEV_PROP_REF,						\
+	{ .pointer = &(const struct software_node_ref_args) {		\
+		.node = _ref_,						\
+		.nargs = ARRAY_SIZE(((u64[]){ 0, ##__VA_ARGS__ })) - 1,	\
+		.args = { __VA_ARGS__ },				\
+	} },								\
+}
+
 struct property_entry *
 property_entries_dup(const struct property_entry *properties);
 
@@ -379,20 +418,6 @@ int fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode,
 /* -------------------------------------------------------------------------- */
 /* Software fwnode support - when HW description is incomplete or missing */
 
-struct software_node;
-
-/**
- * struct software_node_ref_args - Reference with additional arguments
- * @node: Reference to a software node
- * @nargs: Number of elements in @args array
- * @args: Integer arguments
- */
-struct software_node_ref_args {
-	const struct software_node *node;
-	unsigned int nargs;
-	u64 args[NR_FWNODE_REFERENCE_ARGS];
-};
-
 /**
  * struct software_node_reference - Named software node reference property
  * @name: Name of the property
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

[PATCH v8 4/6] platform/x86: intel_cht_int33fe: use inline reference properties

[Dmitry Torokhov]

Now that static device properties allow defining reference properties
together with all other types of properties, instead of managing them
separately, let's adjust the driver.

Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 .../platform/x86/intel_cht_int33fe_typec.c    | 81 ++++++++++---------
 1 file changed, 41 insertions(+), 40 deletions(-)

diff --git a/drivers/platform/x86/intel_cht_int33fe_typec.c b/drivers/platform/x86/intel_cht_int33fe_typec.c
index 2d097fc2dd465..04138215956bf 100644
--- a/drivers/platform/x86/intel_cht_int33fe_typec.c
+++ b/drivers/platform/x86/intel_cht_int33fe_typec.c
@@ -36,30 +36,6 @@ enum {
 	INT33FE_NODE_MAX,
 };
 
-static const struct software_node nodes[];
-
-static const struct software_node_ref_args pi3usb30532_ref = {
-	&nodes[INT33FE_NODE_PI3USB30532]
-};
-
-static const struct software_node_ref_args dp_ref = {
-	&nodes[INT33FE_NODE_DISPLAYPORT]
-};
-
-static struct software_node_ref_args mux_ref;
-
-static const struct software_node_reference usb_connector_refs[] = {
-	{ "orientation-switch", 1, &pi3usb30532_ref},
-	{ "mode-switch", 1, &pi3usb30532_ref},
-	{ "displayport", 1, &dp_ref},
-	{ }
-};
-
-static const struct software_node_reference fusb302_refs[] = {
-	{ "usb-role-switch", 1, &mux_ref},
-	{ }
-};
-
 /*
  * Grrr I severly dislike buggy BIOS-es. At least one BIOS enumerates
  * the max17047 both through the INT33FE ACPI device (it is right there
@@ -95,8 +71,18 @@ static const struct property_entry max17047_props[] = {
 	{ }
 };
 
+/*
+ * We are not using inline property here because those are constant,
+ * and we need to adjust this one at runtime to point to real
+ * software node.
+ */
+static struct software_node_ref_args fusb302_mux_refs[] = {
+	{ .node = NULL },
+};
+
 static const struct property_entry fusb302_props[] = {
 	PROPERTY_ENTRY_STRING("linux,extcon-name", "cht_wcove_pwrsrc"),
+	PROPERTY_ENTRY_REF_ARRAY("usb-role-switch", fusb302_mux_refs),
 	{ }
 };
 
@@ -112,6 +98,8 @@ static const u32 snk_pdo[] = {
 	PDO_VAR(5000, 12000, 3000),
 };
 
+static const struct software_node nodes[];
+
 static const struct property_entry usb_connector_props[] = {
 	PROPERTY_ENTRY_STRING("data-role", "dual"),
 	PROPERTY_ENTRY_STRING("power-role", "dual"),
@@ -119,15 +107,21 @@ static const struct property_entry usb_connector_props[] = {
 	PROPERTY_ENTRY_U32_ARRAY("source-pdos", src_pdo),
 	PROPERTY_ENTRY_U32_ARRAY("sink-pdos", snk_pdo),
 	PROPERTY_ENTRY_U32("op-sink-microwatt", 2500000),
+	PROPERTY_ENTRY_REF("orientation-switch",
+			   &nodes[INT33FE_NODE_PI3USB30532]),
+	PROPERTY_ENTRY_REF("mode-switch",
+			   &nodes[INT33FE_NODE_PI3USB30532]),
+	PROPERTY_ENTRY_REF("displayport",
+			   &nodes[INT33FE_NODE_DISPLAYPORT]),
 	{ }
 };
 
 static const struct software_node nodes[] = {
-	{ "fusb302", NULL, fusb302_props, fusb302_refs },
+	{ "fusb302", NULL, fusb302_props },
 	{ "max17047", NULL, max17047_props },
 	{ "pi3usb30532" },
 	{ "displayport" },
-	{ "connector", &nodes[0], usb_connector_props, usb_connector_refs },
+	{ "connector", &nodes[0], usb_connector_props },
 	{ }
 };
 
@@ -163,9 +157,10 @@ static void cht_int33fe_remove_nodes(struct cht_int33fe_data *data)
 {
 	software_node_unregister_nodes(nodes);
 
-	if (mux_ref.node) {
-		fwnode_handle_put(software_node_fwnode(mux_ref.node));
-		mux_ref.node = NULL;
+	if (fusb302_mux_refs[0].node) {
+		fwnode_handle_put(
+			software_node_fwnode(fusb302_mux_refs[0].node));
+		fusb302_mux_refs[0].node = NULL;
 	}
 
 	if (data->dp) {
@@ -177,25 +172,31 @@ static void cht_int33fe_remove_nodes(struct cht_int33fe_data *data)
 
 static int cht_int33fe_add_nodes(struct cht_int33fe_data *data)
 {
+	const struct software_node *mux_ref_node;
 	int ret;
 
-	ret = software_node_register_nodes(nodes);
-	if (ret)
-		return ret;
-
-	/* The devices that are not created in this driver need extra steps. */
-
 	/*
 	 * There is no ACPI device node for the USB role mux, so we need to wait
 	 * until the mux driver has created software node for the mux device.
 	 * It means we depend on the mux driver. This function will return
 	 * -EPROBE_DEFER until the mux device is registered.
 	 */
-	mux_ref.node = software_node_find_by_name(NULL, "intel-xhci-usb-sw");
-	if (!mux_ref.node) {
-		ret = -EPROBE_DEFER;
-		goto err_remove_nodes;
-	}
+	mux_ref_node = software_node_find_by_name(NULL, "intel-xhci-usb-sw");
+	if (!mux_ref_node)
+		return -EPROBE_DEFER;
+
+	/*
+	 * Update node used in "usb-role-switch" property. Note that we
+	 * rely on software_node_register_nodes() to use the original
+	 * instance of properties instead of copying them.
+	 */
+	fusb302_mux_refs[0].node = mux_ref_node;
+
+	ret = software_node_register_nodes(nodes);
+	if (ret)
+		return ret;
+
+	/* The devices that are not created in this driver need extra steps. */
 
 	/*
 	 * The DP connector does have ACPI device node. In this case we can just
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

[PATCH v8 5/6] software node: remove separate handling of references

[Dmitry Torokhov]

Now that all users of references have moved to reference properties,
we can remove separate handling of references.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/base/swnode.c    | 48 +++++++++++++++-------------------------
 include/linux/property.h | 14 ------------
 2 files changed, 18 insertions(+), 44 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 604d7327bba79..0b081dee1e95c 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -479,9 +479,8 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
 				 struct fwnode_reference_args *args)
 {
 	struct swnode *swnode = to_swnode(fwnode);
-	const struct software_node_reference *ref;
 	const struct software_node_ref_args *ref_array;
-	const struct software_node_ref_args *ref_args;
+	const struct software_node_ref_args *ref;
 	const struct property_entry *prop;
 	struct fwnode_handle *refnode;
 	u32 nargs_prop_val;
@@ -492,37 +491,26 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
 		return -ENOENT;
 
 	prop = property_entry_get(swnode->node->properties, propname);
-	if (prop) {
-		if (prop->type != DEV_PROP_REF)
-			return -EINVAL;
-
-		/*
-		 * We expect that references are never stored inline, even
-		 * single ones, as they are too big.
-		 */
-		if (prop->is_inline)
-			return -EINVAL;
-
-		if (index * sizeof(*ref_args) >= prop->length)
-			return -ENOENT;
-
-		ref_array = prop->pointer;
-		ref_args = &ref_array[index];
-	} else {
-		if (!swnode->node->references)
-			return -ENOENT;
+	if (!prop)
+		return -ENOENT;
+
+	if (prop->type != DEV_PROP_REF)
+		return -EINVAL;
 
-		for (ref = swnode->node->references; ref->name; ref++)
-			if (!strcmp(ref->name, propname))
-				break;
+	/*
+	 * We expect that references are never stored inline, even
+	 * single ones, as they are too big.
+	 */
+	if (prop->is_inline)
+		return -EINVAL;
 
-		if (!ref->name || index > (ref->nrefs - 1))
-			return -ENOENT;
+	if (index * sizeof(*ref) >= prop->length)
+		return -ENOENT;
 
-		ref_args = &ref->refs[index];
-	}
+	ref_array = prop->pointer;
+	ref = &ref_array[index];
 
-	refnode = software_node_fwnode(ref_args->node);
+	refnode = software_node_fwnode(ref->node);
 	if (!refnode)
 		return -ENOENT;
 
@@ -543,7 +531,7 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
 	args->nargs = nargs;
 
 	for (i = 0; i < nargs; i++)
-		args->args[i] = ref_args->args[i];
+		args->args[i] = ref->args[i];
 
 	return 0;
 }
diff --git a/include/linux/property.h b/include/linux/property.h
index 19b9dcc322763..b28c81af7bb68 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -418,30 +418,16 @@ int fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode,
 /* -------------------------------------------------------------------------- */
 /* Software fwnode support - when HW description is incomplete or missing */
 
-/**
- * struct software_node_reference - Named software node reference property
- * @name: Name of the property
- * @nrefs: Number of elements in @refs array
- * @refs: Array of references with optional arguments
- */
-struct software_node_reference {
-	const char *name;
-	unsigned int nrefs;
-	const struct software_node_ref_args *refs;
-};
-
 /**
  * struct software_node - Software node description
  * @name: Name of the software node
  * @parent: Parent of the software node
  * @properties: Array of device properties
- * @references: Array of software node reference properties
  */
 struct software_node {
 	const char *name;
 	const struct software_node *parent;
 	const struct property_entry *properties;
-	const struct software_node_reference *references;
 };
 
 bool is_software_node(const struct fwnode_handle *fwnode);
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

[PATCH v8 6/6] software node: add basic tests for property entries

[Dmitry Torokhov]

This adds tests for creating software nodes with properties supplied by
PROPERTY_ENTRY_XXX() macros and fetching and validating data from said
nodes/properties.

We are using KUnit framework for the tests.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/base/test/Makefile              |   2 +
 drivers/base/test/property-entry-test.c | 472 ++++++++++++++++++++++++
 2 files changed, 474 insertions(+)
 create mode 100644 drivers/base/test/property-entry-test.c

diff --git a/drivers/base/test/Makefile b/drivers/base/test/Makefile
index 0f1f7277a0139..22143102e5d21 100644
--- a/drivers/base/test/Makefile
+++ b/drivers/base/test/Makefile
@@ -1,2 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_TEST_ASYNC_DRIVER_PROBE)	+= test_async_driver_probe.o
+
+obj-$(CONFIG_KUNIT) += property-entry-test.o
diff --git a/drivers/base/test/property-entry-test.c b/drivers/base/test/property-entry-test.c
new file mode 100644
index 0000000000000..d523a3d9caeda
--- /dev/null
+++ b/drivers/base/test/property-entry-test.c
@@ -0,0 +1,472 @@
+// SPDX-License-Identifier: GPL-2.0
+// Unit tests for property entries API
+//
+// Copyright 2019 Google LLC.
+
+#include <kunit/test.h>
+#include <linux/property.h>
+#include <linux/types.h>
+
+static void pe_test_uints(struct kunit *test)
+{
+	const struct property_entry entries[] = {
+		PROPERTY_ENTRY_U8("prop-u8", 8),
+		PROPERTY_ENTRY_U16("prop-u16", 16),
+		PROPERTY_ENTRY_U32("prop-u32", 32),
+		PROPERTY_ENTRY_U64("prop-u64", 64),
+		{ }
+	};
+
+	struct fwnode_handle *node;
+	u8 val_u8, array_u8[2];
+	u16 val_u16, array_u16[2];
+	u32 val_u32, array_u32[2];
+	u64 val_u64, array_u64[2];
+	int error;
+
+	node = fwnode_create_software_node(entries, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, node);
+
+	error = fwnode_property_read_u8(node, "prop-u8", &val_u8);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_EQ(test, (int)val_u8, 8);
+
+	error = fwnode_property_read_u8_array(node, "prop-u8", array_u8, 1);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_EQ(test, (int)array_u8[0], 8);
+
+	error = fwnode_property_read_u8_array(node, "prop-u8", array_u8, 2);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u8(node, "no-prop-u8", &val_u8);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u8_array(node, "no-prop-u8", array_u8, 1);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u16(node, "prop-u16", &val_u16);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_EQ(test, (int)val_u16, 16);
+
+	error = fwnode_property_read_u16_array(node, "prop-u16", array_u16, 1);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_EQ(test, (int)array_u16[0], 16);
+
+	error = fwnode_property_read_u16_array(node, "prop-u16", array_u16, 2);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u16(node, "no-prop-u16", &val_u16);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u16_array(node, "no-prop-u16", array_u16, 1);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u32(node, "prop-u32", &val_u32);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_EQ(test, (int)val_u32, 32);
+
+	error = fwnode_property_read_u32_array(node, "prop-u32", array_u32, 1);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_EQ(test, (int)array_u32[0], 32);
+
+	error = fwnode_property_read_u32_array(node, "prop-u32", array_u32, 2);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u32(node, "no-prop-u32", &val_u32);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u32_array(node, "no-prop-u32", array_u32, 1);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u64(node, "prop-u64", &val_u64);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_EQ(test, (int)val_u64, 64);
+
+	error = fwnode_property_read_u64_array(node, "prop-u64", array_u64, 1);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_EQ(test, (int)array_u64[0], 64);
+
+	error = fwnode_property_read_u64_array(node, "prop-u64", array_u64, 2);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u64(node, "no-prop-u64", &val_u64);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u64_array(node, "no-prop-u64", array_u64, 1);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	fwnode_remove_software_node(node);
+}
+
+static void pe_test_uint_arrays(struct kunit *test)
+{
+	u8 a_u8[16] = { 8, 9 };
+	u16 a_u16[16] = { 16, 17 };
+	u32 a_u32[16] = { 32, 33 };
+	u64 a_u64[16] = { 64, 65 };
+	const struct property_entry entries[] = {
+		PROPERTY_ENTRY_U8_ARRAY("prop-u8", a_u8),
+		PROPERTY_ENTRY_U16_ARRAY("prop-u16", a_u16),
+		PROPERTY_ENTRY_U32_ARRAY("prop-u32", a_u32),
+		PROPERTY_ENTRY_U64_ARRAY("prop-u64", a_u64),
+		{ }
+	};
+
+	struct fwnode_handle *node;
+	u8 val_u8, array_u8[32];
+	u16 val_u16, array_u16[32];
+	u32 val_u32, array_u32[32];
+	u64 val_u64, array_u64[32];
+	int error;
+
+	node = fwnode_create_software_node(entries, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, node);
+
+	error = fwnode_property_read_u8(node, "prop-u8", &val_u8);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_EQ(test, (int)val_u8, 8);
+
+	error = fwnode_property_read_u8_array(node, "prop-u8", array_u8, 1);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_EQ(test, (int)array_u8[0], 8);
+
+	error = fwnode_property_read_u8_array(node, "prop-u8", array_u8, 2);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_EQ(test, (int)array_u8[0], 8);
+	KUNIT_EXPECT_EQ(test, (int)array_u8[1], 9);
+
+	error = fwnode_property_read_u8_array(node, "prop-u8", array_u8, 17);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u8(node, "no-prop-u8", &val_u8);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u8_array(node, "no-prop-u8", array_u8, 1);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u16(node, "prop-u16", &val_u16);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_EQ(test, (int)val_u16, 16);
+
+	error = fwnode_property_read_u16_array(node, "prop-u16", array_u16, 1);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_EQ(test, (int)array_u16[0], 16);
+
+	error = fwnode_property_read_u16_array(node, "prop-u16", array_u16, 2);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_EQ(test, (int)array_u16[0], 16);
+	KUNIT_EXPECT_EQ(test, (int)array_u16[1], 17);
+
+	error = fwnode_property_read_u16_array(node, "prop-u16", array_u16, 17);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u16(node, "no-prop-u16", &val_u16);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u16_array(node, "no-prop-u16", array_u16, 1);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u32(node, "prop-u32", &val_u32);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_EQ(test, (int)val_u32, 32);
+
+	error = fwnode_property_read_u32_array(node, "prop-u32", array_u32, 1);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_EQ(test, (int)array_u32[0], 32);
+
+	error = fwnode_property_read_u32_array(node, "prop-u32", array_u32, 2);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_EQ(test, (int)array_u32[0], 32);
+	KUNIT_EXPECT_EQ(test, (int)array_u32[1], 33);
+
+	error = fwnode_property_read_u32_array(node, "prop-u32", array_u32, 17);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u32(node, "no-prop-u32", &val_u32);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u32_array(node, "no-prop-u32", array_u32, 1);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u64(node, "prop-u64", &val_u64);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_EQ(test, (int)val_u64, 64);
+
+	error = fwnode_property_read_u64_array(node, "prop-u64", array_u64, 1);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_EQ(test, (int)array_u64[0], 64);
+
+	error = fwnode_property_read_u64_array(node, "prop-u64", array_u64, 2);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_EQ(test, (int)array_u64[0], 64);
+	KUNIT_EXPECT_EQ(test, (int)array_u64[1], 65);
+
+	error = fwnode_property_read_u64_array(node, "prop-u64", array_u64, 17);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u64(node, "no-prop-u64", &val_u64);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_u64_array(node, "no-prop-u64", array_u64, 1);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	fwnode_remove_software_node(node);
+}
+
+static void pe_test_strings(struct kunit *test)
+{
+	const char *strings[] = {
+		"string-a",
+		"string-b",
+	};
+
+	const struct property_entry entries[] = {
+		PROPERTY_ENTRY_STRING("str", "single"),
+		PROPERTY_ENTRY_STRING("empty", ""),
+		PROPERTY_ENTRY_STRING_ARRAY("strs", strings),
+		{ }
+	};
+
+	struct fwnode_handle *node;
+	const char *str;
+	const char *strs[10];
+	int error;
+
+	node = fwnode_create_software_node(entries, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, node);
+
+	error = fwnode_property_read_string(node, "str", &str);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_STREQ(test, str, "single");
+
+	error = fwnode_property_read_string_array(node, "str", strs, 1);
+	KUNIT_EXPECT_EQ(test, error, 1);
+	KUNIT_EXPECT_STREQ(test, strs[0], "single");
+
+	/* asking for more data returns what we have */
+	error = fwnode_property_read_string_array(node, "str", strs, 2);
+	KUNIT_EXPECT_EQ(test, error, 1);
+	KUNIT_EXPECT_STREQ(test, strs[0], "single");
+
+	error = fwnode_property_read_string(node, "no-str", &str);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_read_string_array(node, "no-str", strs, 1);
+	KUNIT_EXPECT_LT(test, error, 0);
+
+	error = fwnode_property_read_string(node, "empty", &str);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_STREQ(test, str, "");
+
+	error = fwnode_property_read_string_array(node, "strs", strs, 3);
+	KUNIT_EXPECT_EQ(test, error, 2);
+	KUNIT_EXPECT_STREQ(test, strs[0], "string-a");
+	KUNIT_EXPECT_STREQ(test, strs[1], "string-b");
+
+	error = fwnode_property_read_string_array(node, "strs", strs, 1);
+	KUNIT_EXPECT_EQ(test, error, 1);
+	KUNIT_EXPECT_STREQ(test, strs[0], "string-a");
+
+	/* NULL argument -> returns size */
+	error = fwnode_property_read_string_array(node, "strs", NULL, 0);
+	KUNIT_EXPECT_EQ(test, error, 2);
+
+	/* accessing array as single value */
+	error = fwnode_property_read_string(node, "strs", &str);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_STREQ(test, str, "string-a");
+
+	fwnode_remove_software_node(node);
+}
+
+static void pe_test_bool(struct kunit *test)
+{
+	const struct property_entry entries[] = {
+		PROPERTY_ENTRY_BOOL("prop"),
+		{ }
+	};
+
+	struct fwnode_handle *node;
+
+	node = fwnode_create_software_node(entries, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, node);
+
+	KUNIT_EXPECT_TRUE(test, fwnode_property_read_bool(node, "prop"));
+	KUNIT_EXPECT_FALSE(test, fwnode_property_read_bool(node, "not-prop"));
+
+	fwnode_remove_software_node(node);
+}
+
+/* Verifies that small U8 array is stored inline when property is copied */
+static void pe_test_move_inline_u8(struct kunit *test)
+{
+	u8 u8_array_small[8] = { 1, 2, 3, 4 };
+	u8 u8_array_big[128] = { 5, 6, 7, 8 };
+	struct property_entry entries[] = {
+		PROPERTY_ENTRY_U8_ARRAY("small", u8_array_small),
+		PROPERTY_ENTRY_U8_ARRAY("big", u8_array_big),
+		{ }
+	};
+	struct property_entry *copy;
+	const u8 *data_ptr;
+
+	copy = property_entries_dup(entries);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, copy);
+
+	KUNIT_EXPECT_TRUE(test, copy[0].is_inline);
+	data_ptr = (u8 *)&copy[0].value;
+	KUNIT_EXPECT_EQ(test, (int)data_ptr[0], 1);
+	KUNIT_EXPECT_EQ(test, (int)data_ptr[1], 2);
+
+	KUNIT_EXPECT_FALSE(test, copy[1].is_inline);
+	data_ptr = copy[1].pointer;
+	KUNIT_EXPECT_EQ(test, (int)data_ptr[0], 5);
+	KUNIT_EXPECT_EQ(test, (int)data_ptr[1], 6);
+
+	property_entries_free(copy);
+}
+
+/* Verifies that single string array is stored inline when property is copied */
+static void pe_test_move_inline_str(struct kunit *test)
+{
+	char *str_array_small[] = { "a" };
+	char *str_array_big[] = { "b", "c", "d", "e" };
+	char *str_array_small_empty[] = { "" };
+	struct property_entry entries[] = {
+		PROPERTY_ENTRY_STRING_ARRAY("small", str_array_small),
+		PROPERTY_ENTRY_STRING_ARRAY("big", str_array_big),
+		PROPERTY_ENTRY_STRING_ARRAY("small-empty", str_array_small_empty),
+		{ }
+	};
+	struct property_entry *copy;
+	const char * const *data_ptr;
+
+	copy = property_entries_dup(entries);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, copy);
+
+	KUNIT_EXPECT_TRUE(test, copy[0].is_inline);
+	KUNIT_EXPECT_STREQ(test, copy[0].value.str[0], "a");
+
+	KUNIT_EXPECT_FALSE(test, copy[1].is_inline);
+	data_ptr = copy[1].pointer;
+	KUNIT_EXPECT_STREQ(test, data_ptr[0], "b");
+	KUNIT_EXPECT_STREQ(test, data_ptr[1], "c");
+
+	KUNIT_EXPECT_TRUE(test, copy[2].is_inline);
+	KUNIT_EXPECT_STREQ(test, copy[2].value.str[0], "");
+
+	property_entries_free(copy);
+}
+
+/* Handling of reference properties */
+static void pe_test_reference(struct kunit *test)
+{
+	const struct software_node nodes[] = {
+		{ .name = "1", },
+		{ .name = "2", },
+	};
+
+	const struct software_node_ref_args refs[] = {
+		{
+			.node = &nodes[0],
+			.nargs = 0,
+		},
+		{
+			.node = &nodes[1],
+			.nargs = 2,
+			.args = { 3, 4 },
+		},
+	};
+
+	const struct property_entry entries[] = {
+		PROPERTY_ENTRY_REF("ref-1", &nodes[0]),
+		PROPERTY_ENTRY_REF("ref-2", &nodes[1], 1, 2),
+		PROPERTY_ENTRY_REF_ARRAY("ref-3", refs),
+		{ }
+	};
+
+	struct fwnode_handle *node;
+	struct fwnode_reference_args ref;
+	int error;
+
+	error = software_node_register_nodes(nodes);
+	KUNIT_ASSERT_EQ(test, error, 0);
+
+	node = fwnode_create_software_node(entries, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, node);
+
+	error = fwnode_property_get_reference_args(node, "ref-1", NULL,
+						   0, 0, &ref);
+	KUNIT_ASSERT_EQ(test, error, 0);
+	KUNIT_EXPECT_PTR_EQ(test, to_software_node(ref.fwnode), &nodes[0]);
+	KUNIT_EXPECT_EQ(test, ref.nargs, 0U);
+
+	/* wrong index */
+	error = fwnode_property_get_reference_args(node, "ref-1", NULL,
+						   0, 1, &ref);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	error = fwnode_property_get_reference_args(node, "ref-2", NULL,
+						   1, 0, &ref);
+	KUNIT_ASSERT_EQ(test, error, 0);
+	KUNIT_EXPECT_PTR_EQ(test, to_software_node(ref.fwnode), &nodes[1]);
+	KUNIT_EXPECT_EQ(test, ref.nargs, 1U);
+	KUNIT_EXPECT_EQ(test, ref.args[0], 1LLU);
+
+	/* asking for more args, padded with zero data */
+	error = fwnode_property_get_reference_args(node, "ref-2", NULL,
+						   3, 0, &ref);
+	KUNIT_ASSERT_EQ(test, error, 0);
+	KUNIT_EXPECT_PTR_EQ(test, to_software_node(ref.fwnode), &nodes[1]);
+	KUNIT_EXPECT_EQ(test, ref.nargs, 3U);
+	KUNIT_EXPECT_EQ(test, ref.args[0], 1LLU);
+	KUNIT_EXPECT_EQ(test, ref.args[1], 2LLU);
+	KUNIT_EXPECT_EQ(test, ref.args[2], 0LLU);
+
+	/* wrong index */
+	error = fwnode_property_get_reference_args(node, "ref-2", NULL,
+						   2, 1, &ref);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	/* array of references */
+	error = fwnode_property_get_reference_args(node, "ref-3", NULL,
+						   0, 0, &ref);
+	KUNIT_ASSERT_EQ(test, error, 0);
+	KUNIT_EXPECT_PTR_EQ(test, to_software_node(ref.fwnode), &nodes[0]);
+	KUNIT_EXPECT_EQ(test, ref.nargs, 0U);
+
+	/* second reference in the array */
+	error = fwnode_property_get_reference_args(node, "ref-3", NULL,
+						   2, 1, &ref);
+	KUNIT_ASSERT_EQ(test, error, 0);
+	KUNIT_EXPECT_PTR_EQ(test, to_software_node(ref.fwnode), &nodes[1]);
+	KUNIT_EXPECT_EQ(test, ref.nargs, 2U);
+	KUNIT_EXPECT_EQ(test, ref.args[0], 3LLU);
+	KUNIT_EXPECT_EQ(test, ref.args[1], 4LLU);
+
+	/* wrong index */
+	error = fwnode_property_get_reference_args(node, "ref-1", NULL,
+						   0, 2, &ref);
+	KUNIT_EXPECT_NE(test, error, 0);
+
+	fwnode_remove_software_node(node);
+	software_node_unregister_nodes(nodes);
+}
+
+static struct kunit_case property_entry_test_cases[] = {
+	KUNIT_CASE(pe_test_uints),
+	KUNIT_CASE(pe_test_uint_arrays),
+	KUNIT_CASE(pe_test_strings),
+	KUNIT_CASE(pe_test_bool),
+	KUNIT_CASE(pe_test_move_inline_u8),
+	KUNIT_CASE(pe_test_move_inline_str),
+	KUNIT_CASE(pe_test_reference),
+	{ }
+};
+
+static struct kunit_suite property_entry_test_suite = {
+	.name = "property-entry",
+	.test_cases = property_entry_test_cases,
+};
+
+kunit_test_suite(property_entry_test_suite);
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

Re: [PATCH v8 1/6] software node: rename is_array to is_inline

[Rafael J. Wysocki]

On Fri, Nov 8, 2019 at 5:22 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> We do not need a special flag to know if we are dealing with an array,
> as we can get that data from ratio between element length and the data
> size, however we do need a flag to know whether the data is stored
> directly inside property_entry or separately.

So the subject is slightly misleading, because it is not a rename.  I
would say "replace x with y" instead.

[Arguably I can change that when applying the patch, but since we are
going to wait for the dependencies to go in, it should not be a big
deal to send an update of this patch alone?]

> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/base/swnode.c    | 12 +++++-------
>  include/linux/property.h | 13 ++++++++-----
>  2 files changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index d8d0dc0ca5acf..18a30fb3cc588 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -108,10 +108,7 @@ static const void *property_get_pointer(const struct property_entry *prop)
>         if (!prop->length)
>                 return NULL;
>
> -       if (prop->is_array)
> -               return prop->pointer;
> -
> -       return &prop->value;
> +       return prop->is_inline ? &prop->value : prop->pointer;
>  }
>
>  static const void *property_entry_find(const struct property_entry *props,
> @@ -205,7 +202,7 @@ static void property_entry_free_data(const struct property_entry *p)
>         const char * const *src_str;
>         size_t i, nval;
>
> -       if (p->is_array) {
> +       if (!p->is_inline) {
>                 if (p->type == DEV_PROP_STRING && p->pointer) {
>                         src_str = p->pointer;
>                         nval = p->length / sizeof(const char *);
> @@ -250,7 +247,7 @@ static int property_entry_copy_data(struct property_entry *dst,
>         const void *pointer = property_get_pointer(src);
>         const void *new;
>
> -       if (src->is_array) {
> +       if (!src->is_inline) {
>                 if (!src->length)
>                         return -ENODATA;
>
> @@ -264,15 +261,16 @@ static int property_entry_copy_data(struct property_entry *dst,
>                                 return -ENOMEM;
>                 }
>
> -               dst->is_array = true;
>                 dst->pointer = new;
>         } else if (src->type == DEV_PROP_STRING) {
>                 new = kstrdup(src->value.str, GFP_KERNEL);
>                 if (!new && src->value.str)
>                         return -ENOMEM;
>
> +               dst->is_inline = true;
>                 dst->value.str = new;
>         } else {
> +               dst->is_inline = true;
>                 dst->value = src->value;
>         }
>
> diff --git a/include/linux/property.h b/include/linux/property.h
> index 48335288c2a96..dad0ad11b55e2 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -227,15 +227,17 @@ static inline int fwnode_property_count_u64(const struct fwnode_handle *fwnode,
>   * struct property_entry - "Built-in" device property representation.
>   * @name: Name of the property.
>   * @length: Length of data making up the value.
> - * @is_array: True when the property is an array.
> + * @is_inline: True when the property value is embedded in
> + *     &struct property_entry instance.
>   * @type: Type of the data in unions.
> - * @pointer: Pointer to the property (an array of items of the given type).
> - * @value: Value of the property (when it is a single item of the given type).
> + * @pointer: Pointer to the property when it is stored separately from
> + *     the &struct property_entry instance.
> + * @value: Value of the property when it is stored inline.

And while at it, can you please try to make the comments shorter so
they each take one line?

Re: [PATCH v8 1/6] software node: rename is_array to is_inline

[Bjørn Mork]

Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:

> We do not need a special flag to know if we are dealing with an array,
> as we can get that data from ratio between element length and the data
> size, however we do need a flag to know whether the data is stored
> directly inside property_entry or separately.

Doesn't a non-null prop->pointer tell you this?

And inverting the flag is unnecessarily risky IMHO. An all-zero prop
might now result in dereferencing a NULL prop->pointer instead of using
the empty prop->value.  Now I haven't looked at the code to see if this
is a real problem.  But I believe it's better not having to do that
anyway...


Bjørn

Re: [PATCH v8 1/6] software node: rename is_array to is_inline

[Dmitry Torokhov]

On Wed, Nov 13, 2019 at 07:52:43AM +0100, Bjørn Mork wrote:
> Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:
> 
> > We do not need a special flag to know if we are dealing with an array,
> > as we can get that data from ratio between element length and the data
> > size, however we do need a flag to know whether the data is stored
> > directly inside property_entry or separately.
> 
> Doesn't a non-null prop->pointer tell you this?

No it does not because pointer is a part of a union.

> 
> And inverting the flag is unnecessarily risky IMHO. An all-zero prop
> might now result in dereferencing a NULL prop->pointer instead of using
> the empty prop->value.  Now I haven't looked at the code to see if this
> is a real problem.  But I believe it's better not having to do that
> anyway...

All-zero property is a terminator and thus we will not dereference
anything.

Thanks.

-- 
Dmitry

Re: [PATCH v8 1/6] software node: rename is_array to is_inline

[Marek Szyprowski]

Dear All,

On 08.11.2019 05:22, Dmitry Torokhov wrote:
> We do not need a special flag to know if we are dealing with an array,
> as we can get that data from ratio between element length and the data
> size, however we do need a flag to know whether the data is stored
> directly inside property_entry or separately.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Today I've noticed that this patch got merged to linux-next as commit 
e6bff4665c595b5a4aff173848851ed49ac3bfad. Sadly it breaks DWC3/xHCI 
driver operation on Samsung Exynos5 SoCs (and probably on other SoCs 
which use DWC3 in host mode too). I get the following errors during boot:

dwc3 12000000.dwc3: failed to add properties to xHCI
dwc3 12000000.dwc3: failed to initialize host
dwc3: probe of 12000000.dwc3 failed with error -61

Here is a full kernel log from Exynos5250-based Snow Chromebook on KernelCI:

https://storage.kernelci.org/next/master/next-20191212/arm/exynos_defconfig/gcc-8/lab-collabora/boot-exynos5250-snow.txt

(lack of 'ref' clk is not related nor fatal to the driver operation).

The code which fails after this patch is located in 
drivers/usb/dwc3/host.c. Let me know if I can help more in locating the bug.

> ---
>   drivers/base/swnode.c    | 12 +++++-------
>   include/linux/property.h | 13 ++++++++-----
>   2 files changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index d8d0dc0ca5acf..18a30fb3cc588 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -108,10 +108,7 @@ static const void *property_get_pointer(const struct property_entry *prop)
>   	if (!prop->length)
>   		return NULL;
>   
> -	if (prop->is_array)
> -		return prop->pointer;
> -
> -	return &prop->value;
> +	return prop->is_inline ? &prop->value : prop->pointer;
>   }
>   
>   static const void *property_entry_find(const struct property_entry *props,
> @@ -205,7 +202,7 @@ static void property_entry_free_data(const struct property_entry *p)
>   	const char * const *src_str;
>   	size_t i, nval;
>   
> -	if (p->is_array) {
> +	if (!p->is_inline) {
>   		if (p->type == DEV_PROP_STRING && p->pointer) {
>   			src_str = p->pointer;
>   			nval = p->length / sizeof(const char *);
> @@ -250,7 +247,7 @@ static int property_entry_copy_data(struct property_entry *dst,
>   	const void *pointer = property_get_pointer(src);
>   	const void *new;
>   
> -	if (src->is_array) {
> +	if (!src->is_inline) {
>   		if (!src->length)
>   			return -ENODATA;
>   
> @@ -264,15 +261,16 @@ static int property_entry_copy_data(struct property_entry *dst,
>   				return -ENOMEM;
>   		}
>   
> -		dst->is_array = true;
>   		dst->pointer = new;
>   	} else if (src->type == DEV_PROP_STRING) {
>   		new = kstrdup(src->value.str, GFP_KERNEL);
>   		if (!new && src->value.str)
>   			return -ENOMEM;
>   
> +		dst->is_inline = true;
>   		dst->value.str = new;
>   	} else {
> +		dst->is_inline = true;
>   		dst->value = src->value;
>   	}
>   
> diff --git a/include/linux/property.h b/include/linux/property.h
> index 48335288c2a96..dad0ad11b55e2 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -227,15 +227,17 @@ static inline int fwnode_property_count_u64(const struct fwnode_handle *fwnode,
>    * struct property_entry - "Built-in" device property representation.
>    * @name: Name of the property.
>    * @length: Length of data making up the value.
> - * @is_array: True when the property is an array.
> + * @is_inline: True when the property value is embedded in
> + *	&struct property_entry instance.
>    * @type: Type of the data in unions.
> - * @pointer: Pointer to the property (an array of items of the given type).
> - * @value: Value of the property (when it is a single item of the given type).
> + * @pointer: Pointer to the property when it is stored separately from
> + *	the &struct property_entry instance.
> + * @value: Value of the property when it is stored inline.
>    */
>   struct property_entry {
>   	const char *name;
>   	size_t length;
> -	bool is_array;
> +	bool is_inline;
>   	enum dev_prop_type type;
>   	union {
>   		const void *pointer;
> @@ -262,7 +264,6 @@ struct property_entry {
>   (struct property_entry) {						\
>   	.name = _name_,							\
>   	.length = (_len_) * __PROPERTY_ENTRY_ELEMENT_SIZE(_elem_),	\
> -	.is_array = true,						\
>   	.type = DEV_PROP_##_Type_,					\
>   	{ .pointer = _val_ },						\
>   }
> @@ -293,6 +294,7 @@ struct property_entry {
>   (struct property_entry) {						\
>   	.name = _name_,							\
>   	.length = __PROPERTY_ENTRY_ELEMENT_SIZE(_elem_),		\
> +	.is_inline = true,						\
>   	.type = DEV_PROP_##_Type_,					\
>   	{ .value = { ._elem_ = _val_ } },				\
>   }
> @@ -311,6 +313,7 @@ struct property_entry {
>   #define PROPERTY_ENTRY_BOOL(_name_)		\
>   (struct property_entry) {			\
>   	.name = _name_,				\
> +	.is_inline = true,			\
>   }
>   
>   struct property_entry *

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v8 1/6] software node: rename is_array to is_inline

[Andy Shevchenko]

On Thu, Dec 12, 2019 at 12:12:36PM +0100, Marek Szyprowski wrote:
> Dear All,
> 
> On 08.11.2019 05:22, Dmitry Torokhov wrote:
> > We do not need a special flag to know if we are dealing with an array,
> > as we can get that data from ratio between element length and the data
> > size, however we do need a flag to know whether the data is stored
> > directly inside property_entry or separately.
> >
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> Today I've noticed that this patch got merged to linux-next as commit 
> e6bff4665c595b5a4aff173848851ed49ac3bfad. Sadly it breaks DWC3/xHCI 
> driver operation on Samsung Exynos5 SoCs (and probably on other SoCs 
> which use DWC3 in host mode too). I get the following errors during boot:
> 
> dwc3 12000000.dwc3: failed to add properties to xHCI
> dwc3 12000000.dwc3: failed to initialize host
> dwc3: probe of 12000000.dwc3 failed with error -61
> 
> Here is a full kernel log from Exynos5250-based Snow Chromebook on KernelCI:
> 
> https://storage.kernelci.org/next/master/next-20191212/arm/exynos_defconfig/gcc-8/lab-collabora/boot-exynos5250-snow.txt
> 
> (lack of 'ref' clk is not related nor fatal to the driver operation).
> 
> The code which fails after this patch is located in 
> drivers/usb/dwc3/host.c. Let me know if I can help more in locating the bug.

Thank you for report.

I think we should not have that patch in the fist place... I used to have
a bad feeling about it and then forgot about it existence.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v8 1/6] software node: rename is_array to is_inline

[Rafael J. Wysocki]

On Thu, Dec 12, 2019 at 12:28 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Dec 12, 2019 at 12:12:36PM +0100, Marek Szyprowski wrote:
> > Dear All,
> >
> > On 08.11.2019 05:22, Dmitry Torokhov wrote:
> > > We do not need a special flag to know if we are dealing with an array,
> > > as we can get that data from ratio between element length and the data
> > > size, however we do need a flag to know whether the data is stored
> > > directly inside property_entry or separately.
> > >
> > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> >
> > Today I've noticed that this patch got merged to linux-next as commit
> > e6bff4665c595b5a4aff173848851ed49ac3bfad. Sadly it breaks DWC3/xHCI
> > driver operation on Samsung Exynos5 SoCs (and probably on other SoCs
> > which use DWC3 in host mode too). I get the following errors during boot:
> >
> > dwc3 12000000.dwc3: failed to add properties to xHCI
> > dwc3 12000000.dwc3: failed to initialize host
> > dwc3: probe of 12000000.dwc3 failed with error -61
> >
> > Here is a full kernel log from Exynos5250-based Snow Chromebook on KernelCI:
> >
> > https://storage.kernelci.org/next/master/next-20191212/arm/exynos_defconfig/gcc-8/lab-collabora/boot-exynos5250-snow.txt
> >
> > (lack of 'ref' clk is not related nor fatal to the driver operation).
> >
> > The code which fails after this patch is located in
> > drivers/usb/dwc3/host.c. Let me know if I can help more in locating the bug.
>
> Thank you for report.
>
> I think we should not have that patch in the fist place... I used to have
> a bad feeling about it and then forgot about it existence.

Well, I think you mean the [2/6].

The $subject one really shouldn't change functionality, we must have
missed something here.

Anyway, I'll drop this branch from the linux-next one for now.

Re: [PATCH v8 1/6] software node: rename is_array to is_inline

[Dmitry Torokhov]

Hi Marek,

On Thu, Dec 12, 2019 at 12:12:36PM +0100, Marek Szyprowski wrote:
> Dear All,
> 
> On 08.11.2019 05:22, Dmitry Torokhov wrote:
> > We do not need a special flag to know if we are dealing with an array,
> > as we can get that data from ratio between element length and the data
> > size, however we do need a flag to know whether the data is stored
> > directly inside property_entry or separately.
> >
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> Today I've noticed that this patch got merged to linux-next as commit 
> e6bff4665c595b5a4aff173848851ed49ac3bfad. Sadly it breaks DWC3/xHCI 
> driver operation on Samsung Exynos5 SoCs (and probably on other SoCs 
> which use DWC3 in host mode too). I get the following errors during boot:
> 
> dwc3 12000000.dwc3: failed to add properties to xHCI
> dwc3 12000000.dwc3: failed to initialize host
> dwc3: probe of 12000000.dwc3 failed with error -61
> 
> Here is a full kernel log from Exynos5250-based Snow Chromebook on KernelCI:
> 
> https://storage.kernelci.org/next/master/next-20191212/arm/exynos_defconfig/gcc-8/lab-collabora/boot-exynos5250-snow.txt
> 
> (lack of 'ref' clk is not related nor fatal to the driver operation).
> 
> The code which fails after this patch is located in 
> drivers/usb/dwc3/host.c. Let me know if I can help more in locating the bug.

Does the following help? If, as I expect, it does, I'll submit it
formally.

---

diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
index 5567ed2cddbec..fa252870c926f 100644
--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c
@@ -88,10 +88,10 @@ int dwc3_host_init(struct dwc3 *dwc)
 	memset(props, 0, sizeof(struct property_entry) * ARRAY_SIZE(props));
 
 	if (dwc->usb3_lpm_capable)
-		props[prop_idx++].name = "usb3-lpm-capable";
+		props[prop_idx++] = PROPERTY_ENTRY_BOOL("usb3-lpm-capable");
 
 	if (dwc->usb2_lpm_disable)
-		props[prop_idx++].name = "usb2-lpm-disable";
+		props[prop_idx++] = PROPERTY_ENTRY_BOOL("usb2-lpm-disable");
 
 	/**
 	 * WORKAROUND: dwc3 revisions <=3.00a have a limitation
@@ -103,7 +103,7 @@ int dwc3_host_init(struct dwc3 *dwc)
 	 * This following flag tells XHCI to do just that.
 	 */
 	if (dwc->revision <= DWC3_REVISION_300A)
-		props[prop_idx++].name = "quirk-broken-port-ped";
+		props[prop_idx++] = PROPERTY_ENTRY_BOOL("quirk-broken-port-ped");
 
 	if (prop_idx) {
 		ret = platform_device_add_properties(xhci, props);


-- 
Dmitry

Re: [PATCH v8 1/6] software node: rename is_array to is_inline

[Marek Szyprowski]

Hi Dmitry,

On 13.12.2019 02:24, Dmitry Torokhov wrote:
> On Thu, Dec 12, 2019 at 12:12:36PM +0100, Marek Szyprowski wrote:
>> On 08.11.2019 05:22, Dmitry Torokhov wrote:
>>> We do not need a special flag to know if we are dealing with an array,
>>> as we can get that data from ratio between element length and the data
>>> size, however we do need a flag to know whether the data is stored
>>> directly inside property_entry or separately.
>>>
>>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Today I've noticed that this patch got merged to linux-next as commit
>> e6bff4665c595b5a4aff173848851ed49ac3bfad. Sadly it breaks DWC3/xHCI
>> driver operation on Samsung Exynos5 SoCs (and probably on other SoCs
>> which use DWC3 in host mode too). I get the following errors during boot:
>>
>> dwc3 12000000.dwc3: failed to add properties to xHCI
>> dwc3 12000000.dwc3: failed to initialize host
>> dwc3: probe of 12000000.dwc3 failed with error -61
>>
>> Here is a full kernel log from Exynos5250-based Snow Chromebook on KernelCI:
>>
>> https://protect2.fireeye.com/url?k=df93ba76-820d14ec-df923139-0cc47a336fae-f751d8b108bc5bdf&u=https://storage.kernelci.org/next/master/next-20191212/arm/exynos_defconfig/gcc-8/lab-collabora/boot-exynos5250-snow.txt
>>
>> (lack of 'ref' clk is not related nor fatal to the driver operation).
>>
>> The code which fails after this patch is located in
>> drivers/usb/dwc3/host.c. Let me know if I can help more in locating the bug.
> Does the following help? If, as I expect, it does, I'll submit it
> formally.

Yes, it fixes the issue. If possible, please add the following tags:

Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

to the final patch.

> ---
>
> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
> index 5567ed2cddbec..fa252870c926f 100644
> --- a/drivers/usb/dwc3/host.c
> +++ b/drivers/usb/dwc3/host.c
> @@ -88,10 +88,10 @@ int dwc3_host_init(struct dwc3 *dwc)
>   	memset(props, 0, sizeof(struct property_entry) * ARRAY_SIZE(props));
>   
>   	if (dwc->usb3_lpm_capable)
> -		props[prop_idx++].name = "usb3-lpm-capable";
> +		props[prop_idx++] = PROPERTY_ENTRY_BOOL("usb3-lpm-capable");
>   
>   	if (dwc->usb2_lpm_disable)
> -		props[prop_idx++].name = "usb2-lpm-disable";
> +		props[prop_idx++] = PROPERTY_ENTRY_BOOL("usb2-lpm-disable");
>   
>   	/**
>   	 * WORKAROUND: dwc3 revisions <=3.00a have a limitation
> @@ -103,7 +103,7 @@ int dwc3_host_init(struct dwc3 *dwc)
>   	 * This following flag tells XHCI to do just that.
>   	 */
>   	if (dwc->revision <= DWC3_REVISION_300A)
> -		props[prop_idx++].name = "quirk-broken-port-ped";
> +		props[prop_idx++] = PROPERTY_ENTRY_BOOL("quirk-broken-port-ped");
>   
>   	if (prop_idx) {
>   		ret = platform_device_add_properties(xhci, props);
>
>
Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v8 1/6] software node: rename is_array to is_inline

[Marek Szyprowski]

Hi Rafael,

On 12.12.2019 17:41, Rafael J. Wysocki wrote:
> On Thu, Dec 12, 2019 at 12:28 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
>> On Thu, Dec 12, 2019 at 12:12:36PM +0100, Marek Szyprowski wrote:
>>> On 08.11.2019 05:22, Dmitry Torokhov wrote:
>>>> We do not need a special flag to know if we are dealing with an array,
>>>> as we can get that data from ratio between element length and the data
>>>> size, however we do need a flag to know whether the data is stored
>>>> directly inside property_entry or separately.
>>>>
>>>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>> Today I've noticed that this patch got merged to linux-next as commit
>>> e6bff4665c595b5a4aff173848851ed49ac3bfad. Sadly it breaks DWC3/xHCI
>>> driver operation on Samsung Exynos5 SoCs (and probably on other SoCs
>>> which use DWC3 in host mode too). I get the following errors during boot:
>>>
>>> dwc3 12000000.dwc3: failed to add properties to xHCI
>>> dwc3 12000000.dwc3: failed to initialize host
>>> dwc3: probe of 12000000.dwc3 failed with error -61
>>>
>>> Here is a full kernel log from Exynos5250-based Snow Chromebook on KernelCI:
>>>
>>> https://storage.kernelci.org/next/master/next-20191212/arm/exynos_defconfig/gcc-8/lab-collabora/boot-exynos5250-snow.txt
>>>
>>> (lack of 'ref' clk is not related nor fatal to the driver operation).
>>>
>>> The code which fails after this patch is located in
>>> drivers/usb/dwc3/host.c. Let me know if I can help more in locating the bug.
>> Thank you for report.
>>
>> I think we should not have that patch in the fist place... I used to have
>> a bad feeling about it and then forgot about it existence.
> Well, I think you mean the [2/6].
>
> The $subject one really shouldn't change functionality, we must have
> missed something here.

Nope, I was really talking about [1/6]. It looks that it revealed an 
issue in the DWC3 driver pointed by Dmitry.

> Anyway, I'll drop this branch from the linux-next one for now.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v8 1/6] software node: rename is_array to is_inline

[Rafael J. Wysocki]

On Fri, Dec 13, 2019 at 7:47 AM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> Hi Rafael,
>
> On 12.12.2019 17:41, Rafael J. Wysocki wrote:
> > On Thu, Dec 12, 2019 at 12:28 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> >> On Thu, Dec 12, 2019 at 12:12:36PM +0100, Marek Szyprowski wrote:
> >>> On 08.11.2019 05:22, Dmitry Torokhov wrote:
> >>>> We do not need a special flag to know if we are dealing with an array,
> >>>> as we can get that data from ratio between element length and the data
> >>>> size, however we do need a flag to know whether the data is stored
> >>>> directly inside property_entry or separately.
> >>>>
> >>>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> >>> Today I've noticed that this patch got merged to linux-next as commit
> >>> e6bff4665c595b5a4aff173848851ed49ac3bfad. Sadly it breaks DWC3/xHCI
> >>> driver operation on Samsung Exynos5 SoCs (and probably on other SoCs
> >>> which use DWC3 in host mode too). I get the following errors during boot:
> >>>
> >>> dwc3 12000000.dwc3: failed to add properties to xHCI
> >>> dwc3 12000000.dwc3: failed to initialize host
> >>> dwc3: probe of 12000000.dwc3 failed with error -61
> >>>
> >>> Here is a full kernel log from Exynos5250-based Snow Chromebook on KernelCI:
> >>>
> >>> https://storage.kernelci.org/next/master/next-20191212/arm/exynos_defconfig/gcc-8/lab-collabora/boot-exynos5250-snow.txt
> >>>
> >>> (lack of 'ref' clk is not related nor fatal to the driver operation).
> >>>
> >>> The code which fails after this patch is located in
> >>> drivers/usb/dwc3/host.c. Let me know if I can help more in locating the bug.
> >> Thank you for report.
> >>
> >> I think we should not have that patch in the fist place... I used to have
> >> a bad feeling about it and then forgot about it existence.
> > Well, I think you mean the [2/6].
> >
> > The $subject one really shouldn't change functionality, we must have
> > missed something here.
>
> Nope, I was really talking about [1/6]. It looks that it revealed an
> issue in the DWC3 driver pointed by Dmitry.

Right, but I was referring to the Andy's comment.

Cheers!

Re: [PATCH v8 3/6] software node: implement reference properties

[Lukasz Stelmach]

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

It was <2019-11-07 czw 20:22>, when Dmitry Torokhov wrote:
> It is possible to store references to software nodes in the same fashion as
> other static properties, so that users do not need to define separate
> structures:
>
> static const struct software_node gpio_bank_b_node = {
> 	.name = "B",
> };
>
> static const struct property_entry simone_key_enter_props[] = {
> 	PROPERTY_ENTRY_U32("linux,code", KEY_ENTER),
> 	PROPERTY_ENTRY_STRING("label", "enter"),
> 	PROPERTY_ENTRY_REF("gpios", &gpio_bank_b_node, 123, GPIO_ACTIVE_LOW),
> 	{ }
> };
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---

I am writing a piece that needs to provide a list of gpios to a
diriver. The above example looks like what I need.

At the moment the driver gets the list from fwnode/of_node. The list
contain references to phandles which get resolved and and the driver
ends up with a bunch of gpio descriptors. Great.

This example looks nice but does the code that reads the reference from
the gpios property and returns a gpiod actually exist? If it doesn't, I
am willing to write it.

At first glance it makes more sense to me to pass (struct gpiod_lookup
*) instead of (struct software_node *) and make gpiolib's gpiod_find()
accept lookup tables as parameter instead of searching the
gpio_lookup_list? Or do you think such temporary table should be
assembled from the above structure and then used in gpiod_find()?

Any other suggestions on how to get a bunch of gpios (the description
for gpios is available in the devicetree) for a device described with a
software nodes?

Kind regards,
-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

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

Re: [PATCH v8 3/6] software node: implement reference properties

[Andy Shevchenko]

On Mon, Nov 09, 2020 at 06:02:29PM +0100, Lukasz Stelmach wrote:
> It was <2019-11-07 czw 20:22>, when Dmitry Torokhov wrote:
> > It is possible to store references to software nodes in the same fashion as
> > other static properties, so that users do not need to define separate
> > structures:
> >
> > static const struct software_node gpio_bank_b_node = {
> > 	.name = "B",
> > };
> >
> > static const struct property_entry simone_key_enter_props[] = {
> > 	PROPERTY_ENTRY_U32("linux,code", KEY_ENTER),
> > 	PROPERTY_ENTRY_STRING("label", "enter"),
> > 	PROPERTY_ENTRY_REF("gpios", &gpio_bank_b_node, 123, GPIO_ACTIVE_LOW),
> > 	{ }
> > };
> >
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> 
> I am writing a piece that needs to provide a list of gpios to a
> diriver. The above example looks like what I need.

Nope.

It mustn't be used for GPIOs or PWMs or whatever that either should come via
lookup tables or corresponding firmware interface.

> At the moment the driver gets the list from fwnode/of_node. The list
> contain references to phandles which get resolved and and the driver
> ends up with a bunch of gpio descriptors. Great.
> 
> This example looks nice but does the code that reads the reference from
> the gpios property and returns a gpiod actually exist? If it doesn't, I
> am willing to write it.
> 
> At first glance it makes more sense to me to pass (struct gpiod_lookup
> *) instead of (struct software_node *) and make gpiolib's gpiod_find()
> accept lookup tables as parameter instead of searching the
> gpio_lookup_list? Or do you think such temporary table should be
> assembled from the above structure and then used in gpiod_find()?
> 
> Any other suggestions on how to get a bunch of gpios (the description
> for gpios is available in the devicetree) for a device described with a
> software nodes?



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v8 3/6] software node: implement reference properties

[Lukasz Stelmach]

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

It was <2020-11-09 pon 19:24>, when Andy Shevchenko wrote:
> On Mon, Nov 09, 2020 at 06:02:29PM +0100, Lukasz Stelmach wrote:
>> It was <2019-11-07 czw 20:22>, when Dmitry Torokhov wrote:
>> > It is possible to store references to software nodes in the same fashion as
>> > other static properties, so that users do not need to define separate
>> > structures:
>> >
>> > static const struct software_node gpio_bank_b_node = {
>> > 	.name = "B",
>> > };
>> >
>> > static const struct property_entry simone_key_enter_props[] = {
>> > 	PROPERTY_ENTRY_U32("linux,code", KEY_ENTER),
>> > 	PROPERTY_ENTRY_STRING("label", "enter"),
>> > 	PROPERTY_ENTRY_REF("gpios", &gpio_bank_b_node, 123, GPIO_ACTIVE_LOW),
>> > 	{ }
>> > };
>> >
>> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> > ---
>> 
>> I am writing a piece that needs to provide a list of gpios to a
>> diriver. The above example looks like what I need.
>
> Nope.
>
> It mustn't be used for GPIOs or PWMs or whatever that either should come via
> lookup tables or corresponding firmware interface.
>

May I ask why? I've read commit descriptions for drivers/base/swnode.c
and the discussion on lkml and I understand software nodes as a way to
provide (synthesize) a description for a device that is missing a
description in the firmware. Another use case seems to be to replace (in
the long run) platform data. That is what I am trying to use it for.

I want my device to be configured with either DT or software_nodes
created at run time with configfs. My device is going to use GPIOs
described in the DT and it is going to be configured via configfs at run
time. I could use platform_data to pass structures from configfs but
software nodes would let me save some code in the device driver and use
the same paths for both static (DT) and dynamic (configfs)
configuration.

Probably I have missed something and I will be greatful, if you tell me
where I can find more information about software nodes. There are few
users in the kernel and it isn't obvious for me how to use software
nodes properly.

>> At the moment the driver gets the list from fwnode/of_node. The list
>> contain references to phandles which get resolved and and the driver
>> ends up with a bunch of gpio descriptors. Great.
>> 
>> This example looks nice but does the code that reads the reference from
>> the gpios property and returns a gpiod actually exist? If it doesn't, I
>> am willing to write it.
>> 
>> At first glance it makes more sense to me to pass (struct gpiod_lookup
>> *) instead of (struct software_node *) and make gpiolib's gpiod_find()
>> accept lookup tables as parameter instead of searching the
>> gpio_lookup_list? Or do you think such temporary table should be
>> assembled from the above structure and then used in gpiod_find()?
>> 
>> Any other suggestions on how to get a bunch of gpios (the description
>> for gpios is available in the devicetree) for a device described with a
>> software nodes?

Kind regards,
-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

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

Re: [PATCH v8 3/6] software node: implement reference properties

[Dmitry Torokhov]

On Mon, Nov 09, 2020 at 07:18:37PM +0100, Lukasz Stelmach wrote:
> It was <2020-11-09 pon 19:24>, when Andy Shevchenko wrote:
> > On Mon, Nov 09, 2020 at 06:02:29PM +0100, Lukasz Stelmach wrote:
> >> It was <2019-11-07 czw 20:22>, when Dmitry Torokhov wrote:
> >> > It is possible to store references to software nodes in the same fashion as
> >> > other static properties, so that users do not need to define separate
> >> > structures:
> >> >
> >> > static const struct software_node gpio_bank_b_node = {
> >> > 	.name = "B",
> >> > };
> >> >
> >> > static const struct property_entry simone_key_enter_props[] = {
> >> > 	PROPERTY_ENTRY_U32("linux,code", KEY_ENTER),
> >> > 	PROPERTY_ENTRY_STRING("label", "enter"),
> >> > 	PROPERTY_ENTRY_REF("gpios", &gpio_bank_b_node, 123, GPIO_ACTIVE_LOW),
> >> > 	{ }
> >> > };
> >> >
> >> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> >> > ---
> >> 
> >> I am writing a piece that needs to provide a list of gpios to a
> >> diriver. The above example looks like what I need.
> >
> > Nope.
> >
> > It mustn't be used for GPIOs or PWMs or whatever that either should come via
> > lookup tables or corresponding firmware interface.
> >
> 
> May I ask why? I've read commit descriptions for drivers/base/swnode.c
> and the discussion on lkml and I understand software nodes as a way to
> provide (synthesize) a description for a device that is missing a
> description in the firmware. Another use case seems to be to replace (in
> the long run) platform data. That is what I am trying to use it for.
> 
> I want my device to be configured with either DT or software_nodes
> created at run time with configfs. My device is going to use GPIOs
> described in the DT and it is going to be configured via configfs at run
> time. I could use platform_data to pass structures from configfs but
> software nodes would let me save some code in the device driver and use
> the same paths for both static (DT) and dynamic (configfs)
> configuration.
> 
> Probably I have missed something and I will be greatful, if you tell me
> where I can find more information about software nodes. There are few
> users in the kernel and it isn't obvious for me how to use software
> nodes properly.

Yeah, I disagree with Andy here. The lookup tables are a crutch that we
have until GPIO and PWM a taught to support software nodes (I need to
resurrect my patch series for GPIO, if you have time to test that would
be awesome).

Thanks.

-- 
Dmitry

Re: [PATCH v8 3/6] software node: implement reference properties

[Andy Shevchenko]

On Mon, Nov 09, 2020 at 07:18:37PM +0100, Lukasz Stelmach wrote:
> It was <2020-11-09 pon 19:24>, when Andy Shevchenko wrote:
> > On Mon, Nov 09, 2020 at 06:02:29PM +0100, Lukasz Stelmach wrote:
> >> It was <2019-11-07 czw 20:22>, when Dmitry Torokhov wrote:

...

> >> I am writing a piece that needs to provide a list of gpios to a
> >> diriver. The above example looks like what I need.
> >
> > Nope.
> >
> > It mustn't be used for GPIOs or PWMs or whatever that either should come via
> > lookup tables or corresponding firmware interface.
> 
> May I ask why? I've read commit descriptions for drivers/base/swnode.c
> and the discussion on lkml and I understand software nodes as a way to
> provide (synthesize) a description for a device that is missing a
> description in the firmware. Another use case seems to be to replace (in
> the long run) platform data. That is what I am trying to use it for.

Yes. Both are correct. They are simply not applicable for everything
(it's not a silver bullet).

> I want my device to be configured with either DT or software_nodes
> created at run time with configfs.

Okay.

> My device is going to use GPIOs
> described in the DT and it is going to be configured via configfs at run
> time.

How is this related to swnodes?
Create GPIO lookup table.

> I could use platform_data to pass structures from configfs but
> software nodes would let me save some code in the device driver and use
> the same paths for both static (DT) and dynamic (configfs)
> configuration.
> 
> Probably I have missed something and I will be greatful, if you tell me
> where I can find more information about software nodes. There are few
> users in the kernel and it isn't obvious for me how to use software
> nodes properly.

gpiod_add_lookup_table().

> >> At the moment the driver gets the list from fwnode/of_node. The list
> >> contain references to phandles which get resolved and and the driver
> >> ends up with a bunch of gpio descriptors. Great.
> >> 
> >> This example looks nice but does the code that reads the reference from
> >> the gpios property and returns a gpiod actually exist? If it doesn't, I
> >> am willing to write it.
> >> 
> >> At first glance it makes more sense to me to pass (struct gpiod_lookup
> >> *) instead of (struct software_node *) and make gpiolib's gpiod_find()
> >> accept lookup tables as parameter instead of searching the
> >> gpio_lookup_list? Or do you think such temporary table should be
> >> assembled from the above structure and then used in gpiod_find()?
> >> 
> >> Any other suggestions on how to get a bunch of gpios (the description
> >> for gpios is available in the devicetree) for a device described with a
> >> software nodes?


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v8 3/6] software node: implement reference properties

[Andy Shevchenko]

On Mon, Nov 09, 2020 at 10:53:05AM -0800, Dmitry Torokhov wrote:
> On Mon, Nov 09, 2020 at 07:18:37PM +0100, Lukasz Stelmach wrote:
> > It was <2020-11-09 pon 19:24>, when Andy Shevchenko wrote:

...

> > Probably I have missed something and I will be greatful, if you tell me
> > where I can find more information about software nodes. There are few
> > users in the kernel and it isn't obvious for me how to use software
> > nodes properly.
> 
> Yeah, I disagree with Andy here. The lookup tables are a crutch that we
> have until GPIO and PWM a taught to support software nodes (I need to
> resurrect my patch series for GPIO, if you have time to test that would
> be awesome).

We don't have support for list of fwnodes, this will probably break things
where swnode is already exist.

I think Heikki may send a documentation patch to clarify when swnodes can and
can't be used. Maybe I'm mistaken and above is a good use case by design.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v8 3/6] software node: implement reference properties

[Lukasz Stelmach]

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

It was <2020-11-09 pon 21:02>, when Andy Shevchenko wrote:
> On Mon, Nov 09, 2020 at 07:18:37PM +0100, Lukasz Stelmach wrote:
>> It was <2020-11-09 pon 19:24>, when Andy Shevchenko wrote:
>> > On Mon, Nov 09, 2020 at 06:02:29PM +0100, Lukasz Stelmach wrote:
>> >> It was <2019-11-07 czw 20:22>, when Dmitry Torokhov wrote:
>
> ...
>
>> >> I am writing a piece that needs to provide a list of gpios to a
>> >> diriver. The above example looks like what I need.
>> >
>> > Nope.
>> >
>> > It mustn't be used for GPIOs or PWMs or whatever that either should come via
>> > lookup tables or corresponding firmware interface.
>> 
>> May I ask why? I've read commit descriptions for drivers/base/swnode.c
>> and the discussion on lkml and I understand software nodes as a way to
>> provide (synthesize) a description for a device that is missing a
>> description in the firmware. Another use case seems to be to replace (in
>> the long run) platform data. That is what I am trying to use it for.
>
> Yes. Both are correct. They are simply not applicable for everything
> (it's not a silver bullet).
>
>> I want my device to be configured with either DT or software_nodes
>> created at run time with configfs.
>
> Okay.
>
>> My device is going to use GPIOs
>> described in the DT and it is going to be configured via configfs at run
>> time.
>
> How is this related to swnodes?

I thought I should mention it, because as far as I can tel mixing and
merging information from different fwnode backends seems to be tricky if
supported at all.

> Create GPIO lookup table.
>
>> I could use platform_data to pass structures from configfs but
>> software nodes would let me save some code in the device driver and use
>> the same paths for both static (DT) and dynamic (configfs)
>> configuration.
>> 
>> Probably I have missed something and I will be greatful, if you tell me
>> where I can find more information about software nodes. There are few
>> users in the kernel and it isn't obvious for me how to use software
>> nodes properly.
>
> gpiod_add_lookup_table().
>

Yes, that is exactly what my POC code does now. But having a lookup
table together with the rest of the device structures has several
advantages.

1) The device may be hotpluggable and there is no
   gpiod_remove_lookup_table().

2) Having the lookup table allocated and managed together with the rest
   of the device seems like a better way to go than on gpio_lookup_list.

3) As of now I've got a minor issue with device naming. I need to set
   dev_id of the table before the device is ready and only after it is
   ready, its name is set (in the hotpluggable use case).

4) Because no other devices would use this lookup table "publishing" it
   rather than keeping together with the device seems at least slightly
   odd.

When the lookup table is attached to the devices and can be passed
around  the final lookup can be done with a function like

static struct gpio_desc *gpiod_find_from_table(struct device *dev,
                             const char *con_id, unsigned int idx,
                 unsigned long *flags, struct gpiod_lookup *table)

>>>> At the moment the driver gets the list from fwnode/of_node. The list
>>>> contain references to phandles which get resolved and and the driver
>>>> ends up with a bunch of gpio descriptors. Great.
>>>> 
>>>> This example looks nice but does the code that reads the reference from
>>>> the gpios property and returns a gpiod actually exist? If it doesn't, I
>>>> am willing to write it.
>>>> 
>>>> At first glance it makes more sense to me to pass (struct gpiod_lookup
>>>> *) instead of (struct software_node *) and make gpiolib's gpiod_find()
>>>> accept lookup tables as parameter instead of searching the
>>>> gpio_lookup_list? Or do you think such temporary table should be
>>>> assembled from the above structure and then used in gpiod_find()?
>>>> 
>>>> Any other suggestions on how to get a bunch of gpios (the description
>>>> for gpios is available in the devicetree) for a device described with a
>>>> software nodes?

-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

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

Re: [PATCH v8 3/6] software node: implement reference properties

[Lukasz Stelmach]

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

It was <2020-11-09 pon 10:53>, when Dmitry Torokhov wrote:
> On Mon, Nov 09, 2020 at 07:18:37PM +0100, Lukasz Stelmach wrote:
>> It was <2020-11-09 pon 19:24>, when Andy Shevchenko wrote:
>> > On Mon, Nov 09, 2020 at 06:02:29PM +0100, Lukasz Stelmach wrote:
>> >> It was <2019-11-07 czw 20:22>, when Dmitry Torokhov wrote:
>> >> > It is possible to store references to software nodes in the same fashion as
>> >> > other static properties, so that users do not need to define separate
>> >> > structures:
>> >> >
>> >> > static const struct software_node gpio_bank_b_node = {
>> >> > 	.name = "B",
>> >> > };
>> >> >
>> >> > static const struct property_entry simone_key_enter_props[] = {
>> >> > 	PROPERTY_ENTRY_U32("linux,code", KEY_ENTER),
>> >> > 	PROPERTY_ENTRY_STRING("label", "enter"),
>> >> > 	PROPERTY_ENTRY_REF("gpios", &gpio_bank_b_node, 123, GPIO_ACTIVE_LOW),
>> >> > 	{ }
>> >> > };
>> >> >
>> >> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> >> > ---
>> >> 
>> >> I am writing a piece that needs to provide a list of gpios to a
>> >> diriver. The above example looks like what I need.
>> >
>> > Nope.
>> >
>> > It mustn't be used for GPIOs or PWMs or whatever that either should come via
>> > lookup tables or corresponding firmware interface.
>> >
>> 
>> May I ask why? I've read commit descriptions for drivers/base/swnode.c
>> and the discussion on lkml and I understand software nodes as a way to
>> provide (synthesize) a description for a device that is missing a
>> description in the firmware. Another use case seems to be to replace (in
>> the long run) platform data. That is what I am trying to use it for.
>> 
>> I want my device to be configured with either DT or software_nodes
>> created at run time with configfs. My device is going to use GPIOs
>> described in the DT and it is going to be configured via configfs at run
>> time. I could use platform_data to pass structures from configfs but
>> software nodes would let me save some code in the device driver and use
>> the same paths for both static (DT) and dynamic (configfs)
>> configuration.
>> 
>> Probably I have missed something and I will be greatful, if you tell me
>> where I can find more information about software nodes. There are few
>> users in the kernel and it isn't obvious for me how to use software
>> nodes properly.
>
> Yeah, I disagree with Andy here. The lookup tables are a crutch that we
> have until GPIO and PWM a taught to support software nodes (I need to
> resurrect my patch series for GPIO, if you have time to test that would
> be awesome).

It will be great to help. I am not sure if I have means to test PWMs,
but I can do GPIOs.

-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

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

Re: [PATCH v8 3/6] software node: implement reference properties

[Andy Shevchenko]

On Mon, Nov 09, 2020 at 08:47:14PM +0100, Lukasz Stelmach wrote:
> It was <2020-11-09 pon 21:02>, when Andy Shevchenko wrote:
> > On Mon, Nov 09, 2020 at 07:18:37PM +0100, Lukasz Stelmach wrote:
> >> It was <2020-11-09 pon 19:24>, when Andy Shevchenko wrote:
> >> > On Mon, Nov 09, 2020 at 06:02:29PM +0100, Lukasz Stelmach wrote:
> >> >> It was <2019-11-07 czw 20:22>, when Dmitry Torokhov wrote:

...

> > Create GPIO lookup table.
> >
> >> I could use platform_data to pass structures from configfs but
> >> software nodes would let me save some code in the device driver and use
> >> the same paths for both static (DT) and dynamic (configfs)
> >> configuration.
> >> 
> >> Probably I have missed something and I will be greatful, if you tell me
> >> where I can find more information about software nodes. There are few
> >> users in the kernel and it isn't obvious for me how to use software
> >> nodes properly.
> >
> > gpiod_add_lookup_table().
> >
> 
> Yes, that is exactly what my POC code does now. But having a lookup
> table together with the rest of the device structures has several
> advantages.
> 
> 1) The device may be hotpluggable and there is no
>    gpiod_remove_lookup_table().

	% git grep -n -w gpiod_remove_lookup_table

Or I did get it wrong? Did you mean that the removal is not being called?

> 2) Having the lookup table allocated and managed together with the rest
>    of the device seems like a better way to go than on gpio_lookup_list.

Nice, what are you going to do with the rest of lookup tables
(PWM, regulators, etc)? If you convert, convert them all at least.

> 3) As of now I've got a minor issue with device naming. I need to set
>    dev_id of the table before the device is ready and only after it is
>    ready, its name is set (in the hotpluggable use case).

Hotpluggable devices are very much supported by ACPI assistance. DT I have
heard has overlays. What's the issue?

> 4) Because no other devices would use this lookup table "publishing" it
>    rather than keeping together with the device seems at least slightly
>    odd.
> 
> When the lookup table is attached to the devices and can be passed
> around  the final lookup can be done with a function like
> 
> static struct gpio_desc *gpiod_find_from_table(struct device *dev,
>                              const char *con_id, unsigned int idx,
>                  unsigned long *flags, struct gpiod_lookup *table)

Something sounds fishy about your case. Why do you need to have board code /
platform data in the first place? Sorry, but I didn't get why you should
reconstruct DT (or ACPI) at run-time without using proper framework / feature
(overlays)?

> >>>> At the moment the driver gets the list from fwnode/of_node. The list
> >>>> contain references to phandles which get resolved and and the driver
> >>>> ends up with a bunch of gpio descriptors. Great.
> >>>> 
> >>>> This example looks nice but does the code that reads the reference from
> >>>> the gpios property and returns a gpiod actually exist? If it doesn't, I
> >>>> am willing to write it.
> >>>> 
> >>>> At first glance it makes more sense to me to pass (struct gpiod_lookup
> >>>> *) instead of (struct software_node *) and make gpiolib's gpiod_find()
> >>>> accept lookup tables as parameter instead of searching the
> >>>> gpio_lookup_list? Or do you think such temporary table should be
> >>>> assembled from the above structure and then used in gpiod_find()?
> >>>> 
> >>>> Any other suggestions on how to get a bunch of gpios (the description
> >>>> for gpios is available in the devicetree) for a device described with a
> >>>> software nodes?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v8 3/6] software node: implement reference properties

[Heikki Krogerus]

On Mon, Nov 09, 2020 at 09:05:51PM +0200, Andy Shevchenko wrote:
> On Mon, Nov 09, 2020 at 10:53:05AM -0800, Dmitry Torokhov wrote:
> > On Mon, Nov 09, 2020 at 07:18:37PM +0100, Lukasz Stelmach wrote:
> > > It was <2020-11-09 pon 19:24>, when Andy Shevchenko wrote:
> 
> ...
> 
> > > Probably I have missed something and I will be greatful, if you tell me
> > > where I can find more information about software nodes. There are few
> > > users in the kernel and it isn't obvious for me how to use software
> > > nodes properly.
> > 
> > Yeah, I disagree with Andy here. The lookup tables are a crutch that we
> > have until GPIO and PWM a taught to support software nodes (I need to
> > resurrect my patch series for GPIO, if you have time to test that would
> > be awesome).
> 
> We don't have support for list of fwnodes, this will probably break things
> where swnode is already exist.
> 
> I think Heikki may send a documentation patch to clarify when swnodes can and
> can't be used. Maybe I'm mistaken and above is a good use case by design.

Yeah, the problem is that I'm not sure that we can limit the swnodes
for any specific purpose, or dictate very strictly how they are used
with other possible fwnodes.

Right now I'm thinking we should simply not talk about the
relationship a software node should have or can have with other
fwnodes (regardless of their type) in the swnode documentation.

Br,

-- 
heikki

Re: [PATCH v8 3/6] software node: implement reference properties

[Rafael J. Wysocki]

On Tue, Nov 10, 2020 at 1:39 PM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> On Mon, Nov 09, 2020 at 09:05:51PM +0200, Andy Shevchenko wrote:
> > On Mon, Nov 09, 2020 at 10:53:05AM -0800, Dmitry Torokhov wrote:
> > > On Mon, Nov 09, 2020 at 07:18:37PM +0100, Lukasz Stelmach wrote:
> > > > It was <2020-11-09 pon 19:24>, when Andy Shevchenko wrote:
> >
> > ...
> >
> > > > Probably I have missed something and I will be greatful, if you tell me
> > > > where I can find more information about software nodes. There are few
> > > > users in the kernel and it isn't obvious for me how to use software
> > > > nodes properly.
> > >
> > > Yeah, I disagree with Andy here. The lookup tables are a crutch that we
> > > have until GPIO and PWM a taught to support software nodes (I need to
> > > resurrect my patch series for GPIO, if you have time to test that would
> > > be awesome).
> >
> > We don't have support for list of fwnodes, this will probably break things
> > where swnode is already exist.
> >
> > I think Heikki may send a documentation patch to clarify when swnodes can and
> > can't be used. Maybe I'm mistaken and above is a good use case by design.
>
> Yeah, the problem is that I'm not sure that we can limit the swnodes
> for any specific purpose, or dictate very strictly how they are used
> with other possible fwnodes.

Generally agreed, but if there are known problems, they need to be
documented at least for the time being until they are addressed.

> Right now I'm thinking we should simply not talk about the
> relationship a software node should have or can have with other
> fwnodes (regardless of their type) in the swnode documentation.

This sounds reasonable to me, with the above exception.