linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/3] software node: implement reference properties
@ 2019-09-06  4:38 Dmitry Torokhov
  2019-09-06  4:38 ` [PATCH 2/3] platform/x86: intel_cht_int33fe: use inline " Dmitry Torokhov
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Dmitry Torokhov @ 2019-09-06  4:38 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:

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

const struct property_entry simone_key_enter_props[] __initconst = {
	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    | 34 +++++++++++++++++++------
 include/linux/property.h | 54 +++++++++++++++++++++++++++++-----------
 2 files changed, 65 insertions(+), 23 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index e7b3aa3bd55a..01325705b8e4 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -568,21 +568,39 @@ 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;
+		if (index * sizeof(*ref_args) >= prop->length)
+			return -ENOENT;
+
+		ref_args = prop->is_array ?
+				&prop->pointer.ref[index] : &prop->value.ref;
+	} 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 +619,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 5a910ad79591..b25440344743 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -22,7 +22,8 @@ enum dev_prop_type {
 	DEV_PROP_U32,
 	DEV_PROP_U64,
 	DEV_PROP_STRING,
-	DEV_PROP_MAX,
+	DEV_PROP_REF,
+	DEV_PROP_MAX
 };
 
 enum dev_dma_attr {
@@ -219,6 +220,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 +255,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;
@@ -247,6 +263,7 @@ struct property_entry {
 			u32 u32_data;
 			u64 u64_data;
 			const char *str;
+			struct software_node_ref_args ref;
 		} value;
 	};
 };
@@ -284,6 +301,16 @@ 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 +341,17 @@ 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,					\
+	.value.ref.node = _ref_,				\
+	.value.ref.nargs =					\
+		ARRAY_SIZE(((u64[]){ 0, ##__VA_ARGS__ })) - 1,	\
+	.value.ref.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] 11+ messages in thread

* [PATCH 2/3] platform/x86: intel_cht_int33fe: use inline reference properties
  2019-09-06  4:38 [PATCH 1/3] software node: implement reference properties Dmitry Torokhov
@ 2019-09-06  4:38 ` Dmitry Torokhov
  2019-09-06 11:22   ` Heikki Krogerus
  2019-09-06  4:38 ` [PATCH 3/3] software node: remove separate handling of references Dmitry Torokhov
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 11+ messages in thread
From: Dmitry Torokhov @ 2019-09-06  4:38 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>
---

Heikki, I do not have this hardware, so if you could try this out
it would be really great.

 drivers/platform/x86/intel_cht_int33fe.c | 46 ++++++++++++------------
 1 file changed, 22 insertions(+), 24 deletions(-)

diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
index 4fbdff48a4b5..91f3c8840fd8 100644
--- a/drivers/platform/x86/intel_cht_int33fe.c
+++ b/drivers/platform/x86/intel_cht_int33fe.c
@@ -50,28 +50,8 @@ struct cht_int33fe_data {
 
 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
@@ -107,7 +87,13 @@ static const struct property_entry max17047_props[] = {
 	{ }
 };
 
-static const struct property_entry fusb302_props[] = {
+/* Not const because we need to update "usb-role-switch" property. */
+static struct property_entry fusb302_props[] = {
+	/*
+	 * usb-role-switch property must be first as we rely on fixed
+	 * position to adjust it once we know the real node.
+	 */
+	PROPERTY_ENTRY_REF("usb-role-switch", NULL),
 	PROPERTY_ENTRY_STRING("linux,extcon-name", "cht_wcove_pwrsrc"),
 	{ }
 };
@@ -131,16 +117,22 @@ 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" },
 	{ "usb-role-switch" },
-	{ "connector", &nodes[0], usb_connector_props, usb_connector_refs },
+	{ "connector", &nodes[0], usb_connector_props },
 	{ }
 };
 
@@ -174,7 +166,13 @@ static int cht_int33fe_setup_mux(struct cht_int33fe_data *data)
 
 	data->mux = fwnode_handle_get(dev->fwnode);
 	put_device(dev);
-	mux_ref.node = to_software_node(data->mux);
+
+	/*
+	 * Update "usb-role-switch" property with real node. Note that we
+	 * rely on software_node_register_nodes() to use the original
+	 * instance of properties instead of copying them.
+	 */
+	fusb302_props[0].value.ref.node = to_software_node(data->mux);
 
 	return 0;
 }
-- 
2.23.0.187.g17f5b7556c-goog


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

* [PATCH 3/3] software node: remove separate handling of references
  2019-09-06  4:38 [PATCH 1/3] software node: implement reference properties Dmitry Torokhov
  2019-09-06  4:38 ` [PATCH 2/3] platform/x86: intel_cht_int33fe: use inline " Dmitry Torokhov
@ 2019-09-06  4:38 ` Dmitry Torokhov
  2019-09-06 12:38   ` Andy Shevchenko
  2019-09-06 11:17 ` [PATCH 1/3] software node: implement reference properties Heikki Krogerus
  2019-09-06 12:27 ` Andy Shevchenko
  3 siblings, 1 reply; 11+ messages in thread
From: Dmitry Torokhov @ 2019-09-06  4:38 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>
---
 drivers/base/swnode.c    | 31 +++++++++----------------------
 include/linux/property.h | 26 ++++++--------------------
 2 files changed, 15 insertions(+), 42 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 01325705b8e4..21771b29b641 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -568,7 +568,6 @@ 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;
@@ -577,30 +576,18 @@ 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;
-
-		if (index * sizeof(*ref_args) >= prop->length)
-			return -ENOENT;
-
-		ref_args = prop->is_array ?
-				&prop->pointer.ref[index] : &prop->value.ref;
-	} else {
-		if (!swnode->node->references)
-			return -ENOENT;
+	if (!prop)
+		return -ENOENT;
 
-		for (ref = swnode->node->references; ref->name; ref++)
-			if (!strcmp(ref->name, propname))
-				break;
+	if (prop->type != DEV_PROP_REF)
+		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->is_array ? &prop->pointer.ref[index] : &prop->value.ref;
 
-	refnode = software_node_fwnode(ref_args->node);
+	refnode = software_node_fwnode(ref->node);
 	if (!refnode)
 		return -ENOENT;
 
@@ -619,7 +606,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 b25440344743..005b90d9e608 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -223,12 +223,12 @@ static inline int fwnode_property_count_u64(const struct fwnode_handle *fwnode,
 struct software_node;
 
 /**
- * struct software_node_ref_args - Reference property with additional arguments
+ * struct software_node_reference - Named software node reference property
  * @node: Reference to a software node
  * @nargs: Number of elements in @args array
  * @args: Integer arguments
  */
-struct software_node_ref_args {
+struct software_node_reference {
 	const struct software_node *node;
 	unsigned int nargs;
 	u64 args[NR_FWNODE_REFERENCE_ARGS];
@@ -255,7 +255,7 @@ struct property_entry {
 			const u32 *u32_data;
 			const u64 *u64_data;
 			const char * const *str;
-			const struct software_node_ref_args *ref;
+			const struct software_node_reference *ref;
 		} pointer;
 		union {
 			u8 u8_data;
@@ -263,7 +263,7 @@ struct property_entry {
 			u32 u32_data;
 			u64 u64_data;
 			const char *str;
-			struct software_node_ref_args ref;
+			struct software_node_reference ref;
 		} value;
 	};
 };
@@ -305,7 +305,7 @@ struct property_entry {
 (struct property_entry) {					\
 	.name = _name_,						\
 	.length = ARRAY_SIZE(_val_) *				\
-			sizeof(struct software_node_ref_args),	\
+			sizeof(struct software_node_reference),	\
 	.is_array = true,					\
 	.type = DEV_PROP_REF,					\
 	.pointer.ref = _val_,					\
@@ -344,7 +344,7 @@ struct property_entry {
 #define PROPERTY_ENTRY_REF(_name_, _ref_, ...)			\
 (struct property_entry) {					\
 	.name = _name_,						\
-	.length = sizeof(struct software_node_ref_args),	\
+	.length = sizeof(struct software_node_reference),	\
 	.type = DEV_PROP_REF,					\
 	.value.ref.node = _ref_,				\
 	.value.ref.nargs =					\
@@ -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] 11+ messages in thread

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

On Thu, Sep 05, 2019 at 09:38:07PM -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:
> 
> const struct software_node gpio_bank_b_node = {
> 	.name = "B",
> };
> 
> const struct property_entry simone_key_enter_props[] __initconst = {
> 	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>

This looks really good to me. I'll wait for Andy's comments on the
idea, but to me it makes sense.

Thanks Dmitry!

-- 
heikki

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

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

Hi,

On Thu, Sep 05, 2019 at 09:38:08PM -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.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> 
> Heikki, I do not have this hardware, so if you could try this out
> it would be really great.
> 
>  drivers/platform/x86/intel_cht_int33fe.c | 46 ++++++++++++------------
>  1 file changed, 22 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
> index 4fbdff48a4b5..91f3c8840fd8 100644
> --- a/drivers/platform/x86/intel_cht_int33fe.c
> +++ b/drivers/platform/x86/intel_cht_int33fe.c
> @@ -50,28 +50,8 @@ struct cht_int33fe_data {
>  
>  static const struct software_node nodes[];

I think you can remove that.

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

I'm pretty sure you should now drop that one.

> -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
> @@ -107,7 +87,13 @@ static const struct property_entry max17047_props[] = {
>  	{ }
>  };
>  
> -static const struct property_entry fusb302_props[] = {
> +/* Not const because we need to update "usb-role-switch" property. */
> +static struct property_entry fusb302_props[] = {
> +	/*
> +	 * usb-role-switch property must be first as we rely on fixed
> +	 * position to adjust it once we know the real node.
> +	 */
> +	PROPERTY_ENTRY_REF("usb-role-switch", NULL),
>  	PROPERTY_ENTRY_STRING("linux,extcon-name", "cht_wcove_pwrsrc"),
>  	{ }
>  };
> @@ -131,16 +117,22 @@ 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" },
>  	{ "usb-role-switch" },
> -	{ "connector", &nodes[0], usb_connector_props, usb_connector_refs },
> +	{ "connector", &nodes[0], usb_connector_props },
>  	{ }
>  };
>  
> @@ -174,7 +166,13 @@ static int cht_int33fe_setup_mux(struct cht_int33fe_data *data)
>  
>  	data->mux = fwnode_handle_get(dev->fwnode);
>  	put_device(dev);
> -	mux_ref.node = to_software_node(data->mux);
> +
> +	/*
> +	 * Update "usb-role-switch" property with real node. Note that we
> +	 * rely on software_node_register_nodes() to use the original
> +	 * instance of properties instead of copying them.
> +	 */
> +	fusb302_props[0].value.ref.node = to_software_node(data->mux);

There are other changes to this driver and to swnode.c in Rafael's
tree, so if you send v2 soon, then please rebase on top of his devprop
branch, or alternatively linux-next.


thanks,

-- 
heikki

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

* Re: [PATCH 1/3] software node: implement reference properties
  2019-09-06  4:38 [PATCH 1/3] software node: implement reference properties Dmitry Torokhov
                   ` (2 preceding siblings ...)
  2019-09-06 11:17 ` [PATCH 1/3] software node: implement reference properties Heikki Krogerus
@ 2019-09-06 12:27 ` Andy Shevchenko
  2019-09-06 22:34   ` Dmitry Torokhov
  3 siblings, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2019-09-06 12:27 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Rafael J. Wysocki, Heikki Krogerus, Linus Walleij, linux-kernel,
	platform-driver-x86

On Thu, Sep 05, 2019 at 09:38:07PM -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:
> 
> const struct software_node gpio_bank_b_node = {
> 	.name = "B",
> };

Why this can't be __initconst?

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

So it's basically mimics the concept of phandle, right?

> +		ref_args = prop->is_array ?
> +				&prop->pointer.ref[index] : &prop->value.ref;

Better to do if with explicit 'if ()' as it's done in the rest of the code.

	if (prop->is_array)
		ref_args = ...;
	else
		ref_args = ...;

> -	DEV_PROP_MAX,
> +	DEV_PROP_MAX

It seems it wasn't ever used, so, can be dropped completely.

> @@ -240,6 +255,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;
> @@ -247,6 +263,7 @@ struct property_entry {
>  			u32 u32_data;
>  			u64 u64_data;
>  			const char *str;
> +			struct software_node_ref_args ref;

Hmm... This bumps the size of union a lot for each existing property_entry.
Is there any other way? Maybe we can keep pointer and allocate memory for it
when copying?

>  		} value;

> +#define PROPERTY_ENTRY_REF_ARRAY(_name_, _val_)			\
> +(struct property_entry) {					\
> +	.name = _name_,						\
> +	.length = ARRAY_SIZE(_val_) *				\
> +			sizeof(struct software_node_ref_args),	\

I would rather leave it on one line and shift right all \:s in this macro.

> +	.is_array = true,					\
> +	.type = DEV_PROP_REF,					\
> +	.pointer.ref = _val_,					\
> +}
> +

> +#define PROPERTY_ENTRY_REF(_name_, _ref_, ...)			\
> +(struct property_entry) {					\
> +	.name = _name_,						\
> +	.length = sizeof(struct software_node_ref_args),	\
> +	.type = DEV_PROP_REF,					\
> +	.value.ref.node = _ref_,				\

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

Ditto.

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

-- 
With Best Regards,
Andy Shevchenko



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

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

On Thu, Sep 05, 2019 at 09:38:09PM -0700, Dmitry Torokhov wrote:
> Now that all users of references have moved to reference properties,
> we can remove separate handling of references.

This is good one!

-- 
With Best Regards,
Andy Shevchenko



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

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

On Fri, Sep 06, 2019 at 02:17:44PM +0300, Heikki Krogerus wrote:
> On Thu, Sep 05, 2019 at 09:38:07PM -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:
> > 
> > const struct software_node gpio_bank_b_node = {
> > 	.name = "B",
> > };
> > 
> > const struct property_entry simone_key_enter_props[] __initconst = {
> > 	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>
> 
> This looks really good to me. I'll wait for Andy's comments on the
> idea, but to me it makes sense.

Idea in general is fine. Though, taking into consideration, for example,
drivers/mfd/intel-lpss-pci.c, the size of predefined structures bumps a lot.
I think we always should keep a pointer. In this case we may not add another
property type.

-- 
With Best Regards,
Andy Shevchenko



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

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

On Fri, Sep 06, 2019 at 03:40:43PM +0300, Andy Shevchenko wrote:
> On Fri, Sep 06, 2019 at 02:17:44PM +0300, Heikki Krogerus wrote:
> > On Thu, Sep 05, 2019 at 09:38:07PM -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:
> > > 
> > > const struct software_node gpio_bank_b_node = {
> > > 	.name = "B",
> > > };
> > > 
> > > const struct property_entry simone_key_enter_props[] __initconst = {
> > > 	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>
> > 
> > This looks really good to me. I'll wait for Andy's comments on the
> > idea, but to me it makes sense.
> 
> Idea in general is fine. Though, taking into consideration, for example,
> drivers/mfd/intel-lpss-pci.c, the size of predefined structures bumps a lot.
> I think we always should keep a pointer. In this case we may not add another
> property type.

Yeah, you have a point.

thanks,

-- 
heikki

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

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

On Fri, Sep 06, 2019 at 02:22:43PM +0300, Heikki Krogerus wrote:
> Hi,
> 
> On Thu, Sep 05, 2019 at 09:38:08PM -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.
> > 
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> > 
> > Heikki, I do not have this hardware, so if you could try this out
> > it would be really great.
> > 
> >  drivers/platform/x86/intel_cht_int33fe.c | 46 ++++++++++++------------
> >  1 file changed, 22 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
> > index 4fbdff48a4b5..91f3c8840fd8 100644
> > --- a/drivers/platform/x86/intel_cht_int33fe.c
> > +++ b/drivers/platform/x86/intel_cht_int33fe.c
> > @@ -50,28 +50,8 @@ struct cht_int33fe_data {
> >  
> >  static const struct software_node nodes[];
> 
> I think you can remove that.

Not really, as there is still circular dependency between nodes and
properties. I moved it down closer to properties though.

> 
> > -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;
> 
> I'm pretty sure you should now drop that one.

I ended up reworking things a bit and still using a form of this to
reach in and change data, as properties ended up being constants.

> 
> > -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
> > @@ -107,7 +87,13 @@ static const struct property_entry max17047_props[] = {
> >  	{ }
> >  };
> >  
> > -static const struct property_entry fusb302_props[] = {
> > +/* Not const because we need to update "usb-role-switch" property. */
> > +static struct property_entry fusb302_props[] = {
> > +	/*
> > +	 * usb-role-switch property must be first as we rely on fixed
> > +	 * position to adjust it once we know the real node.
> > +	 */
> > +	PROPERTY_ENTRY_REF("usb-role-switch", NULL),
> >  	PROPERTY_ENTRY_STRING("linux,extcon-name", "cht_wcove_pwrsrc"),
> >  	{ }
> >  };
> > @@ -131,16 +117,22 @@ 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" },
> >  	{ "usb-role-switch" },
> > -	{ "connector", &nodes[0], usb_connector_props, usb_connector_refs },
> > +	{ "connector", &nodes[0], usb_connector_props },
> >  	{ }
> >  };
> >  
> > @@ -174,7 +166,13 @@ static int cht_int33fe_setup_mux(struct cht_int33fe_data *data)
> >  
> >  	data->mux = fwnode_handle_get(dev->fwnode);
> >  	put_device(dev);
> > -	mux_ref.node = to_software_node(data->mux);
> > +
> > +	/*
> > +	 * Update "usb-role-switch" property with real node. Note that we
> > +	 * rely on software_node_register_nodes() to use the original
> > +	 * instance of properties instead of copying them.
> > +	 */
> > +	fusb302_props[0].value.ref.node = to_software_node(data->mux);
> 
> There are other changes to this driver and to swnode.c in Rafael's
> tree, so if you send v2 soon, then please rebase on top of his devprop
> branch, or alternatively linux-next.

Yep, did that, will re-post series shortly.

-- 
Dmitry

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

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

On Fri, Sep 06, 2019 at 03:27:43PM +0300, Andy Shevchenko wrote:
> On Thu, Sep 05, 2019 at 09:38:07PM -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:
> > 
> > const struct software_node gpio_bank_b_node = {
> > 	.name = "B",
> > };
> 
> Why this can't be __initconst?

It may or it may not. I'll remove __inticonst from below as well to not
confuse things.

> 
> > const struct property_entry simone_key_enter_props[] __initconst = {
> > 	PROPERTY_ENTRY_U32("linux,code", KEY_ENTER),
> > 	PROPERTY_ENTRY_STRING("label", "enter"),
> > 	PROPERTY_ENTRY_REF("gpios", &gpio_bank_b_node, 123, GPIO_ACTIVE_LOW),
> > 	{ }
> > };
> 
> So it's basically mimics the concept of phandle, right?

Yes.

> 
> > +		ref_args = prop->is_array ?
> > +				&prop->pointer.ref[index] : &prop->value.ref;
> 
> Better to do if with explicit 'if ()' as it's done in the rest of the code.
> 
> 	if (prop->is_array)
> 		ref_args = ...;
> 	else
> 		ref_args = ...;

OK, it will be gone actually.

> 
> > -	DEV_PROP_MAX,
> > +	DEV_PROP_MAX
> 
> It seems it wasn't ever used, so, can be dropped completely.

OK.

> 
> > @@ -240,6 +255,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;
> > @@ -247,6 +263,7 @@ struct property_entry {
> >  			u32 u32_data;
> >  			u64 u64_data;
> >  			const char *str;
> > +			struct software_node_ref_args ref;
> 
> Hmm... This bumps the size of union a lot for each existing property_entry.
> Is there any other way? Maybe we can keep pointer and allocate memory for it
> when copying?

Right, I think we can always store references as arrays, even when we
only need single entry, thus we do not need to increase the size of the
structure.

I just posted v2 implementing that, please take another look.

Thanks.

-- 
Dmitry

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

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

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-06  4:38 [PATCH 1/3] software node: implement reference properties Dmitry Torokhov
2019-09-06  4:38 ` [PATCH 2/3] platform/x86: intel_cht_int33fe: use inline " Dmitry Torokhov
2019-09-06 11:22   ` Heikki Krogerus
2019-09-06 21:34     ` Dmitry Torokhov
2019-09-06  4:38 ` [PATCH 3/3] software node: remove separate handling of references Dmitry Torokhov
2019-09-06 12:38   ` Andy Shevchenko
2019-09-06 11:17 ` [PATCH 1/3] software node: implement reference properties Heikki Krogerus
2019-09-06 12:40   ` Andy Shevchenko
2019-09-06 13:31     ` Heikki Krogerus
2019-09-06 12:27 ` Andy Shevchenko
2019-09-06 22:34   ` Dmitry Torokhov

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