linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/3] software node: implement reference properties
@ 2019-09-06 22:26 Dmitry Torokhov
  2019-09-06 22:26 ` [PATCH v2 2/3] platform/x86: intel_cht_int33fe: use inline " Dmitry Torokhov
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Dmitry Torokhov @ 2019-09-06 22:26 UTC (permalink / raw)
  To: Rafael J. Wysocki, Heikki Krogerus
  Cc: Andy Shevchenko, Linus Walleij, linux-kernel, platform-driver-x86

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>
---

v1->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


 drivers/base/swnode.c    | 43 ++++++++++++++++++++++++++------
 include/linux/property.h | 54 +++++++++++++++++++++++++++++-----------
 2 files changed, 74 insertions(+), 23 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index ee2a405cca9a..bd720d995123 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -387,6 +387,9 @@ static int property_entry_copy_data(struct property_entry *dst,
 		new = kstrdup(src->value.str, GFP_KERNEL);
 		if (!new && src->value.str)
 			return -ENOMEM;
+	} else if (src->type == DEV_PROP_REF) {
+		/* All reference properties must be arrays */
+		return -EINVAL;
 	} else {
 		new = pointer;
 	}
@@ -568,21 +571,45 @@ 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_args;
 	const struct property_entry *prop;
 	struct fwnode_handle *refnode;
 	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 all references to be stored as arrays via
+		 * a pointer, even single ones.
+		 */
+		if (!prop->is_array)
+			return -EINVAL;
+
+		if (index * sizeof(*ref_args) >= prop->length)
+			return -ENOENT;
+
+		ref_args = &prop->pointer.ref[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;
 
@@ -601,7 +628,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 9b3d4ca3a73a..6658403f6fa9 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -22,7 +22,7 @@ enum dev_prop_type {
 	DEV_PROP_U32,
 	DEV_PROP_U64,
 	DEV_PROP_STRING,
-	DEV_PROP_MAX,
+	DEV_PROP_REF,
 };
 
 enum dev_dma_attr {
@@ -219,6 +219,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.
@@ -240,6 +254,7 @@ struct property_entry {
 			const u32 *u32_data;
 			const u64 *u64_data;
 			const char * const *str;
+			const struct software_node_ref_args *ref;
 		} pointer;
 		union {
 			u8 u8_data;
@@ -284,6 +299,15 @@ struct property_entry {
 	{ .pointer = { .str = _val_ } },			\
 }
 
+#define PROPERTY_ENTRY_REF_ARRAY(_name_, _val_)				     \
+(struct property_entry) {						     \
+	.name = _name_,							     \
+	.length = ARRAY_SIZE(_val_) * sizeof(struct software_node_ref_args), \
+	.is_array = true,						     \
+	.type = DEV_PROP_REF,						     \
+	.pointer.ref = _val_,						     \
+}
+
 #define PROPERTY_ENTRY_INTEGER(_name_, _type_, _Type_, _val_)	\
 (struct property_entry) {					\
 	.name = _name_,						\
@@ -314,6 +338,20 @@ struct property_entry {
 	.name = _name_,				\
 }
 
+#define PROPERTY_ENTRY_REF(_name_, _ref_, ...)				\
+(struct property_entry) {						\
+	.name = _name_,							\
+	.length = sizeof(struct software_node_ref_args),		\
+	.type = DEV_PROP_REF,						\
+	.is_array = true,						\
+	.type = DEV_PROP_REF,						\
+	.pointer.ref = &(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);
 
@@ -377,20 +415,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.23.0.187.g17f5b7556c-goog


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 2/3] platform/x86: intel_cht_int33fe: use inline reference properties
  2019-09-06 22:26 [PATCH v2 1/3] software node: implement reference properties Dmitry Torokhov
@ 2019-09-06 22:26 ` Dmitry Torokhov
  2019-09-07 16:12   ` Andy Shevchenko
  2019-09-06 22:26 ` [PATCH v2 3/3] software node: remove separate handling of references Dmitry Torokhov
  2019-09-07 16:08 ` [PATCH v2 1/3] software node: implement reference properties Andy Shevchenko
  2 siblings, 1 reply; 12+ messages in thread
From: Dmitry Torokhov @ 2019-09-06 22:26 UTC (permalink / raw)
  To: Rafael J. Wysocki, Heikki Krogerus
  Cc: Andy Shevchenko, Linus Walleij, linux-kernel, platform-driver-x86

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.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---

v1-v2:

- rebased on top of linux-next-20190904

 drivers/platform/x86/intel_cht_int33fe.c | 81 ++++++++++++------------
 1 file changed, 41 insertions(+), 40 deletions(-)

diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
index 1d5d877b9582..4177c5424931 100644
--- a/drivers/platform/x86/intel_cht_int33fe.c
+++ b/drivers/platform/x86/intel_cht_int33fe.c
@@ -46,30 +46,6 @@ struct cht_int33fe_data {
 	struct fwnode_handle *dp;
 };
 
-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
@@ -105,8 +81,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),
 	{ }
 };
 
@@ -122,6 +108,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"),
@@ -129,15 +117,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 },
 	{ }
 };
 
@@ -173,9 +167,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) {
@@ -187,25 +182,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.23.0.187.g17f5b7556c-goog


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 3/3] software node: remove separate handling of references
  2019-09-06 22:26 [PATCH v2 1/3] software node: implement reference properties Dmitry Torokhov
  2019-09-06 22:26 ` [PATCH v2 2/3] platform/x86: intel_cht_int33fe: use inline " Dmitry Torokhov
@ 2019-09-06 22:26 ` Dmitry Torokhov
  2019-09-07 16:13   ` Andy Shevchenko
  2019-09-07 16:08 ` [PATCH v2 1/3] software node: implement reference properties Andy Shevchenko
  2 siblings, 1 reply; 12+ messages in thread
From: Dmitry Torokhov @ 2019-09-06 22:26 UTC (permalink / raw)
  To: Rafael J. Wysocki, Heikki Krogerus
  Cc: Andy Shevchenko, Linus Walleij, linux-kernel, platform-driver-x86

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>
---

v1->v2:

- dropped rename of struct software_node_ref_args ->
	struct software_node_reference

 drivers/base/swnode.c    | 44 +++++++++++++++-------------------------
 include/linux/property.h | 14 -------------
 2 files changed, 16 insertions(+), 42 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index bd720d995123..5dc113de0cae 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -570,8 +570,7 @@ 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_args;
+	const struct software_node_ref_args *ref;
 	const struct property_entry *prop;
 	struct fwnode_handle *refnode;
 	int i;
@@ -580,36 +579,25 @@ 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 all references to be stored as arrays via
-		 * a pointer, even single ones.
-		 */
-		if (!prop->is_array)
-			return -EINVAL;
-
-		if (index * sizeof(*ref_args) >= prop->length)
-			return -ENOENT;
+	if (!prop)
+		return -ENOENT;
 
-		ref_args = &prop->pointer.ref[index];
-	} else {
-		if (!swnode->node->references)
-			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 all references to be stored as arrays via
+	 * a pointer, even single ones.
+	 */
+	if (!prop->is_array)
+		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 = &prop->pointer.ref[index];
 
-	refnode = software_node_fwnode(ref_args->node);
+	refnode = software_node_fwnode(ref->node);
 	if (!refnode)
 		return -ENOENT;
 
@@ -628,7 +616,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 6658403f6fa9..5e4adccd6404 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -415,30 +415,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.23.0.187.g17f5b7556c-goog


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/3] software node: implement reference properties
  2019-09-06 22:26 [PATCH v2 1/3] software node: implement reference properties Dmitry Torokhov
  2019-09-06 22:26 ` [PATCH v2 2/3] platform/x86: intel_cht_int33fe: use inline " Dmitry Torokhov
  2019-09-06 22:26 ` [PATCH v2 3/3] software node: remove separate handling of references Dmitry Torokhov
@ 2019-09-07 16:08 ` Andy Shevchenko
  2019-09-07 16:32   ` Dmitry Torokhov
  2 siblings, 1 reply; 12+ messages in thread
From: Andy Shevchenko @ 2019-09-07 16:08 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Rafael J. Wysocki, Heikki Krogerus, Linus Walleij, linux-kernel,
	platform-driver-x86

On Fri, Sep 06, 2019 at 03:26:09PM -0700, 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),
> 	{ }
> };
> 

Thanks for an update, my comments below.

> +	} else if (src->type == DEV_PROP_REF) {
> +		/* All reference properties must be arrays */
> +		return -EINVAL;

Hmm... What about to duplicate pointer under value union and use is_array to
distinguish which one to use? Because...


> @@ -240,6 +254,7 @@ struct property_entry {
>  			const u32 *u32_data;
>  			const u64 *u64_data;
>  			const char * const *str;
> +			const struct software_node_ref_args *ref;
>  		} pointer;

> +#define PROPERTY_ENTRY_REF(_name_, _ref_, ...)				\
> +(struct property_entry) {						\
> +	.name = _name_,							\

> +	.length = sizeof(struct software_node_ref_args),		\

Is it correct?

> +	.type = DEV_PROP_REF,						\

> +	.is_array = true,						\

I really don't like this "cheating".

> +	.type = DEV_PROP_REF,						\
> +	.pointer.ref = &(const struct software_node_ref_args) {		\
> +		.node = _ref_,						\

> +		.nargs = ARRAY_SIZE(((u64[]){ 0, ##__VA_ARGS__ })) - 1,	\

Seems like you can drop pair of parentheses.

> +		.args = { __VA_ARGS__ },				\
> +	},								\
> +}

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 2/3] platform/x86: intel_cht_int33fe: use inline reference properties
  2019-09-06 22:26 ` [PATCH v2 2/3] platform/x86: intel_cht_int33fe: use inline " Dmitry Torokhov
@ 2019-09-07 16:12   ` Andy Shevchenko
  0 siblings, 0 replies; 12+ messages in thread
From: Andy Shevchenko @ 2019-09-07 16:12 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Rafael J. Wysocki, Heikki Krogerus, Linus Walleij, linux-kernel,
	platform-driver-x86

On Fri, Sep 06, 2019 at 03:26:10PM -0700, Dmitry Torokhov wrote:
> 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>
> ---
> 
> v1-v2:
> 
> - rebased on top of linux-next-20190904
> 
>  drivers/platform/x86/intel_cht_int33fe.c | 81 ++++++++++++------------
>  1 file changed, 41 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
> index 1d5d877b9582..4177c5424931 100644
> --- a/drivers/platform/x86/intel_cht_int33fe.c
> +++ b/drivers/platform/x86/intel_cht_int33fe.c
> @@ -46,30 +46,6 @@ struct cht_int33fe_data {
>  	struct fwnode_handle *dp;
>  };
>  
> -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
> @@ -105,8 +81,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),
>  	{ }
>  };
>  
> @@ -122,6 +108,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"),
> @@ -129,15 +117,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 },
>  	{ }
>  };
>  
> @@ -173,9 +167,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) {
> @@ -187,25 +182,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.23.0.187.g17f5b7556c-goog
> 

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 3/3] software node: remove separate handling of references
  2019-09-06 22:26 ` [PATCH v2 3/3] software node: remove separate handling of references Dmitry Torokhov
@ 2019-09-07 16:13   ` Andy Shevchenko
  0 siblings, 0 replies; 12+ messages in thread
From: Andy Shevchenko @ 2019-09-07 16:13 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Rafael J. Wysocki, Heikki Krogerus, Linus Walleij, linux-kernel,
	platform-driver-x86

On Fri, Sep 06, 2019 at 03:26:11PM -0700, Dmitry Torokhov wrote:
> Now that all users of references have moved to reference properties,
> we can remove separate handling of references.
> 

FWIW,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> 
> v1->v2:
> 
> - dropped rename of struct software_node_ref_args ->
> 	struct software_node_reference
> 
>  drivers/base/swnode.c    | 44 +++++++++++++++-------------------------
>  include/linux/property.h | 14 -------------
>  2 files changed, 16 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index bd720d995123..5dc113de0cae 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -570,8 +570,7 @@ 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_args;
> +	const struct software_node_ref_args *ref;
>  	const struct property_entry *prop;
>  	struct fwnode_handle *refnode;
>  	int i;
> @@ -580,36 +579,25 @@ 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 all references to be stored as arrays via
> -		 * a pointer, even single ones.
> -		 */
> -		if (!prop->is_array)
> -			return -EINVAL;
> -
> -		if (index * sizeof(*ref_args) >= prop->length)
> -			return -ENOENT;
> +	if (!prop)
> +		return -ENOENT;
>  
> -		ref_args = &prop->pointer.ref[index];
> -	} else {
> -		if (!swnode->node->references)
> -			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 all references to be stored as arrays via
> +	 * a pointer, even single ones.
> +	 */
> +	if (!prop->is_array)
> +		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 = &prop->pointer.ref[index];
>  
> -	refnode = software_node_fwnode(ref_args->node);
> +	refnode = software_node_fwnode(ref->node);
>  	if (!refnode)
>  		return -ENOENT;
>  
> @@ -628,7 +616,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 6658403f6fa9..5e4adccd6404 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -415,30 +415,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.23.0.187.g17f5b7556c-goog
> 

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/3] software node: implement reference properties
  2019-09-07 16:08 ` [PATCH v2 1/3] software node: implement reference properties Andy Shevchenko
@ 2019-09-07 16:32   ` Dmitry Torokhov
  2019-09-07 17:12     ` Andy Shevchenko
  0 siblings, 1 reply; 12+ messages in thread
From: Dmitry Torokhov @ 2019-09-07 16:32 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Rafael J. Wysocki, Heikki Krogerus, Linus Walleij, linux-kernel,
	platform-driver-x86

On Sat, Sep 07, 2019 at 07:08:19PM +0300, Andy Shevchenko wrote:
> On Fri, Sep 06, 2019 at 03:26:09PM -0700, 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),
> > 	{ }
> > };
> > 
> 
> Thanks for an update, my comments below.
> 
> > +	} else if (src->type == DEV_PROP_REF) {
> > +		/* All reference properties must be arrays */
> > +		return -EINVAL;
> 
> Hmm... What about to duplicate pointer under value union and use is_array to
> distinguish which one to use? Because...

Then we have to special-case copying this entry, similar to the pains we
are going with the strings.

> 
> 
> > @@ -240,6 +254,7 @@ struct property_entry {
> >  			const u32 *u32_data;
> >  			const u64 *u64_data;
> >  			const char * const *str;
> > +			const struct software_node_ref_args *ref;
> >  		} pointer;
> 
> > +#define PROPERTY_ENTRY_REF(_name_, _ref_, ...)				\
> > +(struct property_entry) {						\
> > +	.name = _name_,							\
> 
> > +	.length = sizeof(struct software_node_ref_args),		\
> 
> Is it correct?

Yes, we length is element size * number of elements.

> 
> > +	.type = DEV_PROP_REF,						\
> 
> > +	.is_array = true,						\
> 
> I really don't like this "cheating".

This is not cheating. Any single value can be represented as an array of
one element. Actually, the only reason we have this "is_array" business
is because for scalar values and short strings it is much cheaper to
store single value in-line instead of out of line + pointer, especially
on 64 bit arches.

If you want we can change is_array into is_inline.

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/3] software node: implement reference properties
  2019-09-07 16:32   ` Dmitry Torokhov
@ 2019-09-07 17:12     ` Andy Shevchenko
  2019-09-07 17:37       ` Dmitry Torokhov
  0 siblings, 1 reply; 12+ messages in thread
From: Andy Shevchenko @ 2019-09-07 17:12 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Rafael J. Wysocki, Heikki Krogerus, Linus Walleij, linux-kernel,
	platform-driver-x86

On Sat, Sep 07, 2019 at 09:32:40AM -0700, Dmitry Torokhov wrote:
> On Sat, Sep 07, 2019 at 07:08:19PM +0300, Andy Shevchenko wrote:
> > On Fri, Sep 06, 2019 at 03:26:09PM -0700, Dmitry Torokhov wrote:

> > > +	} else if (src->type == DEV_PROP_REF) {
> > > +		/* All reference properties must be arrays */
> > > +		return -EINVAL;
> > 
> > Hmm... What about to duplicate pointer under value union and use is_array to
> > distinguish which one to use? Because...
> 
> Then we have to special-case copying this entry, similar to the pains we
> are going with the strings.

I can't see it as a pain. Simple do the same kmemdup() for the case when
is_array = false and DEV_TYPE_REF?

By the way, don't we need to update property_entry_{get,set}_pointer()?

> > > +	.is_array = true,						\
> > 
> > I really don't like this "cheating".
> 
> This is not cheating. Any single value can be represented as an array of
> one element. Actually, the only reason we have this "is_array" business
> is because for scalar values and short strings it is much cheaper to
> store single value in-line instead of out of line + pointer, especially
> on 64 bit arches.

Yes, and this is a lot of benefit!

> If you want we can change is_array into is_inline.

Nope, is_array is exactly what it tells us about the content. Its functional
load is to distinguish which union (value vs. pointer) we are using.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/3] software node: implement reference properties
  2019-09-07 17:12     ` Andy Shevchenko
@ 2019-09-07 17:37       ` Dmitry Torokhov
  2019-09-07 18:03         ` Andy Shevchenko
  0 siblings, 1 reply; 12+ messages in thread
From: Dmitry Torokhov @ 2019-09-07 17:37 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Rafael J. Wysocki, Heikki Krogerus, Linus Walleij, linux-kernel,
	platform-driver-x86

On Sat, Sep 07, 2019 at 08:12:51PM +0300, Andy Shevchenko wrote:
> On Sat, Sep 07, 2019 at 09:32:40AM -0700, Dmitry Torokhov wrote:
> > On Sat, Sep 07, 2019 at 07:08:19PM +0300, Andy Shevchenko wrote:
> > > On Fri, Sep 06, 2019 at 03:26:09PM -0700, Dmitry Torokhov wrote:
> 
> > > > +	} else if (src->type == DEV_PROP_REF) {
> > > > +		/* All reference properties must be arrays */
> > > > +		return -EINVAL;
> > > 
> > > Hmm... What about to duplicate pointer under value union and use is_array to
> > > distinguish which one to use? Because...
> > 
> > Then we have to special-case copying this entry, similar to the pains we
> > are going with the strings.
> 
> I can't see it as a pain. Simple do the same kmemdup() for the case when
> is_array = false and DEV_TYPE_REF?

And then you need to make sure it is freed on error paths and when we
remove property entries. This requires more checks and code. In contrast
we already know how to handle out of line objects of arbitrary size.

The only reason we have inline strings is because for shorter strings we
save 4/8 bytes.

> 
> By the way, don't we need to update property_entry_{get,set}_pointer()?

I do not see these, where are they?

> 
> > > > +	.is_array = true,						\
> > > 
> > > I really don't like this "cheating".
> > 
> > This is not cheating. Any single value can be represented as an array of
> > one element. Actually, the only reason we have this "is_array" business
> > is because for scalar values and short strings it is much cheaper to
> > store single value in-line instead of out of line + pointer, especially
> > on 64 bit arches.
> 
> Yes, and this is a lot of benefit!

Yes, nobody argues against it. Here however we are dealing with a larger
structure. There is absolutely no benefit of trying to separate single
value vs array here.

> 
> > If you want we can change is_array into is_inline.
> 
> Nope, is_array is exactly what it tells us about the content. Its functional
> load is to distinguish which union (value vs. pointer) we are using.

No, it signifies whether the value is stored within property entry or
outside. I can fit probably 8 bytes arrays into property entry
structure, in which case is_array will definitely not reflect the data
type.

It is the type-specific accessors that know how to parse and fetch data
from properties.

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/3] software node: implement reference properties
  2019-09-07 17:37       ` Dmitry Torokhov
@ 2019-09-07 18:03         ` Andy Shevchenko
  2019-09-07 18:23           ` Dmitry Torokhov
  0 siblings, 1 reply; 12+ messages in thread
From: Andy Shevchenko @ 2019-09-07 18:03 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Rafael J. Wysocki, Heikki Krogerus, Linus Walleij, linux-kernel,
	platform-driver-x86

On Sat, Sep 07, 2019 at 10:37:24AM -0700, Dmitry Torokhov wrote:
> On Sat, Sep 07, 2019 at 08:12:51PM +0300, Andy Shevchenko wrote:
> > On Sat, Sep 07, 2019 at 09:32:40AM -0700, Dmitry Torokhov wrote:
> > > On Sat, Sep 07, 2019 at 07:08:19PM +0300, Andy Shevchenko wrote:
> > > > On Fri, Sep 06, 2019 at 03:26:09PM -0700, Dmitry Torokhov wrote:
> > 
> > > > > +	} else if (src->type == DEV_PROP_REF) {
> > > > > +		/* All reference properties must be arrays */
> > > > > +		return -EINVAL;
> > > > 
> > > > Hmm... What about to duplicate pointer under value union and use is_array to
> > > > distinguish which one to use? Because...
> > > 
> > > Then we have to special-case copying this entry, similar to the pains we
> > > are going with the strings.
> > 
> > I can't see it as a pain. Simple do the same kmemdup() for the case when
> > is_array = false and DEV_TYPE_REF?
> 
> And then you need to make sure it is freed on error paths and when we
> remove property entries. This requires more checks and code. In contrast
> we already know how to handle out of line objects of arbitrary size.

We can put it one level up to be a sibling to value / pointer unions.
In that case is_array can be anything (we just don't care).

Actually strings aren't inlined.

> > By the way, don't we need to update property_entry_{get,set}_pointer()?
> 
> I do not see these, where are they?

swnode.c. I meant property_{get,set}_pointer().

> > > > > +	.is_array = true,						\
> > > > 
> > > > I really don't like this "cheating".
> > > 
> > > This is not cheating. Any single value can be represented as an array of
> > > one element. Actually, the only reason we have this "is_array" business
> > > is because for scalar values and short strings it is much cheaper to
> > > store single value in-line instead of out of line + pointer, especially
> > > on 64 bit arches.
> > 
> > Yes, and this is a lot of benefit!
> 
> Yes, nobody argues against it. Here however we are dealing with a larger
> structure. There is absolutely no benefit of trying to separate single
> value vs array here.

Thus, moving to upper layer makes more sense. Right?

> > > If you want we can change is_array into is_inline.
> > 
> > Nope, is_array is exactly what it tells us about the content. Its functional
> > load is to distinguish which union (value vs. pointer) we are using.
> 
> No, it signifies whether the value is stored within property entry or
> outside. I can fit probably 8 bytes arrays into property entry
> structure, in which case is_array will definitely not reflect the data
> type.

Nope, since strings are not inlined AFAICS.

> It is the type-specific accessors that know how to parse and fetch data
> from properties.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/3] software node: implement reference properties
  2019-09-07 18:03         ` Andy Shevchenko
@ 2019-09-07 18:23           ` Dmitry Torokhov
  2019-09-09 10:09             ` Andy Shevchenko
  0 siblings, 1 reply; 12+ messages in thread
From: Dmitry Torokhov @ 2019-09-07 18:23 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Rafael J. Wysocki, Heikki Krogerus, Linus Walleij, linux-kernel,
	platform-driver-x86

On Sat, Sep 07, 2019 at 09:03:48PM +0300, Andy Shevchenko wrote:
> On Sat, Sep 07, 2019 at 10:37:24AM -0700, Dmitry Torokhov wrote:
> > On Sat, Sep 07, 2019 at 08:12:51PM +0300, Andy Shevchenko wrote:
> > > On Sat, Sep 07, 2019 at 09:32:40AM -0700, Dmitry Torokhov wrote:
> > > > On Sat, Sep 07, 2019 at 07:08:19PM +0300, Andy Shevchenko wrote:
> > > > > On Fri, Sep 06, 2019 at 03:26:09PM -0700, Dmitry Torokhov wrote:
> > > 
> > > > > > +	} else if (src->type == DEV_PROP_REF) {
> > > > > > +		/* All reference properties must be arrays */
> > > > > > +		return -EINVAL;
> > > > > 
> > > > > Hmm... What about to duplicate pointer under value union and use is_array to
> > > > > distinguish which one to use? Because...
> > > > 
> > > > Then we have to special-case copying this entry, similar to the pains we
> > > > are going with the strings.
> > > 
> > > I can't see it as a pain. Simple do the same kmemdup() for the case when
> > > is_array = false and DEV_TYPE_REF?
> > 
> > And then you need to make sure it is freed on error paths and when we
> > remove property entries. This requires more checks and code. In contrast
> > we already know how to handle out of line objects of arbitrary size.
> 
> We can put it one level up to be a sibling to value / pointer unions.
> In that case is_array can be anything (we just don't care).

I think it would be better if you sketched out your proposed data
structure(s) so we are talking about the same things. But please note
that when you are dealing with property arrays we need to keep the easy
way of defining them, which means we should not be splitting individual
entries.

> 
> Actually strings aren't inlined.

Right. Maybe I should clean it up as well.

> 
> > > By the way, don't we need to update property_entry_{get,set}_pointer()?
> > 
> > I do not see these, where are they?
> 
> swnode.c. I meant property_{get,set}_pointer().

Yes, I think you are right about property_set_pointer() at least.

> 
> > > > > > +	.is_array = true,						\
> > > > > 
> > > > > I really don't like this "cheating".
> > > > 
> > > > This is not cheating. Any single value can be represented as an array of
> > > > one element. Actually, the only reason we have this "is_array" business
> > > > is because for scalar values and short strings it is much cheaper to
> > > > store single value in-line instead of out of line + pointer, especially
> > > > on 64 bit arches.
> > > 
> > > Yes, and this is a lot of benefit!
> > 
> > Yes, nobody argues against it. Here however we are dealing with a larger
> > structure. There is absolutely no benefit of trying to separate single
> > value vs array here.
> 
> Thus, moving to upper layer makes more sense. Right?
> 
> > > > If you want we can change is_array into is_inline.
> > > 
> > > Nope, is_array is exactly what it tells us about the content. Its functional
> > > load is to distinguish which union (value vs. pointer) we are using.
> > 
> > No, it signifies whether the value is stored within property entry or
> > outside. I can fit probably 8 bytes arrays into property entry
> > structure, in which case is_array will definitely not reflect the data
> > type.
> 
> Nope, since strings are not inlined AFAICS.

But u64 values are.

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/3] software node: implement reference properties
  2019-09-07 18:23           ` Dmitry Torokhov
@ 2019-09-09 10:09             ` Andy Shevchenko
  0 siblings, 0 replies; 12+ messages in thread
From: Andy Shevchenko @ 2019-09-09 10:09 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Rafael J. Wysocki, Heikki Krogerus, Linus Walleij, linux-kernel,
	platform-driver-x86

On Sat, Sep 07, 2019 at 11:23:35AM -0700, Dmitry Torokhov wrote:
> On Sat, Sep 07, 2019 at 09:03:48PM +0300, Andy Shevchenko wrote:
> > On Sat, Sep 07, 2019 at 10:37:24AM -0700, Dmitry Torokhov wrote:
> > > On Sat, Sep 07, 2019 at 08:12:51PM +0300, Andy Shevchenko wrote:
> > > > On Sat, Sep 07, 2019 at 09:32:40AM -0700, Dmitry Torokhov wrote:
> > > > > On Sat, Sep 07, 2019 at 07:08:19PM +0300, Andy Shevchenko wrote:
> > > > > > On Fri, Sep 06, 2019 at 03:26:09PM -0700, Dmitry Torokhov wrote:
> > > > 
> > > > > > > +	} else if (src->type == DEV_PROP_REF) {
> > > > > > > +		/* All reference properties must be arrays */
> > > > > > > +		return -EINVAL;
> > > > > > 
> > > > > > Hmm... What about to duplicate pointer under value union and use is_array to
> > > > > > distinguish which one to use? Because...
> > > > > 
> > > > > Then we have to special-case copying this entry, similar to the pains we
> > > > > are going with the strings.
> > > > 
> > > > I can't see it as a pain. Simple do the same kmemdup() for the case when
> > > > is_array = false and DEV_TYPE_REF?
> > > 
> > > And then you need to make sure it is freed on error paths and when we
> > > remove property entries. This requires more checks and code. In contrast
> > > we already know how to handle out of line objects of arbitrary size.
> > 
> > We can put it one level up to be a sibling to value / pointer unions.
> > In that case is_array can be anything (we just don't care).
> 
> I think it would be better if you sketched out your proposed data
> structure(s) so we are talking about the same things. But please note
> that when you are dealing with property arrays we need to keep the easy
> way of defining them, which means we should not be splitting individual
> entries.

This one:

        union {
                union {
                        const u8 *u8_data;
                        const u16 *u16_data;
                        const u32 *u32_data;
                        const u64 *u64_data;
                        const char * const *str;
                } pointer;
                union {
                        u8 u8_data;
                        u16 u16_data;
                        u32 u32_data;
                        u64 u64_data;
                        const char *str;
                } value;
		struct ... *ref;
        };

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2019-09-09 10:09 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-06 22:26 [PATCH v2 1/3] software node: implement reference properties Dmitry Torokhov
2019-09-06 22:26 ` [PATCH v2 2/3] platform/x86: intel_cht_int33fe: use inline " Dmitry Torokhov
2019-09-07 16:12   ` Andy Shevchenko
2019-09-06 22:26 ` [PATCH v2 3/3] software node: remove separate handling of references Dmitry Torokhov
2019-09-07 16:13   ` Andy Shevchenko
2019-09-07 16:08 ` [PATCH v2 1/3] software node: implement reference properties Andy Shevchenko
2019-09-07 16:32   ` Dmitry Torokhov
2019-09-07 17:12     ` Andy Shevchenko
2019-09-07 17:37       ` Dmitry Torokhov
2019-09-07 18:03         ` Andy Shevchenko
2019-09-07 18:23           ` Dmitry Torokhov
2019-09-09 10:09             ` Andy Shevchenko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).