[PATCH 0/3] software node: Introduce software_node_find_by_name()

[PATCH 0/3] software node: Introduce software_node_find_by_name()

[Heikki Krogerus]

Hi,

This helper makes it much more easier to access "external" nodes.

thanks,


Heikki Krogerus (3):
  software node: Add software_node_find_by_name()
  usb: roles: intel_xhci: Supplying software node for the role mux
  platform/x86: intel_cht_int33fe: Use new API to gain access to the
    role switch

 drivers/base/swnode.c                         | 35 ++++++++++++
 drivers/platform/x86/intel_cht_int33fe.c      | 57 ++++---------------
 .../usb/roles/intel-xhci-usb-role-switch.c    | 13 ++++-
 include/linux/property.h                      |  4 ++
 4 files changed, 61 insertions(+), 48 deletions(-)

-- 
2.20.1

[PATCH 1/3] software node: Add software_node_find_by_name()

[Heikki Krogerus]

Function that searches software nodes by node name.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/base/swnode.c    | 35 +++++++++++++++++++++++++++++++++++
 include/linux/property.h |  4 ++++
 2 files changed, 39 insertions(+)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index e7b3aa3bd55a..0c4e395e7143 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -620,6 +620,41 @@ static const struct fwnode_operations software_node_ops = {
 
 /* -------------------------------------------------------------------------- */
 
+/**
+ * software_node_find_by_name - Find software node by name
+ * @parent: Parent of the software node
+ * @name: Name of the software node
+ *
+ * The function will find a node that is child of @parent and that is named
+ * @name. If no node is found, the function returns NULL.
+ */
+const struct software_node *
+software_node_find_by_name(const struct software_node *parent, const char *name)
+{
+	struct swnode *swnode;
+	struct kobject *k;
+
+	if (!name)
+		return NULL;
+
+	spin_lock(&swnode_kset->list_lock);
+
+	list_for_each_entry(k, &swnode_kset->list, entry) {
+		swnode = kobj_to_swnode(k);
+		if (parent == swnode->node->parent && swnode->node->name &&
+		    !strcmp(name, swnode->node->name)) {
+			kobject_get(&swnode->kobj);
+			break;
+		}
+		swnode = NULL;
+	}
+
+	spin_unlock(&swnode_kset->list_lock);
+
+	return swnode ? swnode->node : NULL;
+}
+EXPORT_SYMBOL_GPL(software_node_find_by_name);
+
 static int
 software_node_register_properties(struct software_node *node,
 				  const struct property_entry *properties)
diff --git a/include/linux/property.h b/include/linux/property.h
index 5a910ad79591..9b3d4ca3a73a 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -421,6 +421,10 @@ bool is_software_node(const struct fwnode_handle *fwnode);
 const struct software_node *to_software_node(struct fwnode_handle *fwnode);
 struct fwnode_handle *software_node_fwnode(const struct software_node *node);
 
+const struct software_node *
+software_node_find_by_name(const struct software_node *parent,
+			   const char *name);
+
 int software_node_register_nodes(const struct software_node *nodes);
 void software_node_unregister_nodes(const struct software_node *nodes);
 
-- 
2.20.1

Re: [PATCH 1/3] software node: Add software_node_find_by_name()

[Andy Shevchenko]

On Thu, Aug 15, 2019 at 2:32 PM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> Function that searches software nodes by node name.

> +/**
> + * software_node_find_by_name - Find software node by name
> + * @parent: Parent of the software node
> + * @name: Name of the software node
> + *
> + * The function will find a node that is child of @parent and that is named
> + * @name. If no node is found, the function returns NULL.

Shouldn't we add that the caller responsible of putting kobject?

> + */
> +const struct software_node *
> +software_node_find_by_name(const struct software_node *parent, const char *name)
> +{
> +       struct swnode *swnode;
> +       struct kobject *k;
> +
> +       if (!name)
> +               return NULL;
> +
> +       spin_lock(&swnode_kset->list_lock);
> +
> +       list_for_each_entry(k, &swnode_kset->list, entry) {
> +               swnode = kobj_to_swnode(k);
> +               if (parent == swnode->node->parent && swnode->node->name &&
> +                   !strcmp(name, swnode->node->name)) {
> +                       kobject_get(&swnode->kobj);
> +                       break;
> +               }
> +               swnode = NULL;
> +       }
> +
> +       spin_unlock(&swnode_kset->list_lock);
> +
> +       return swnode ? swnode->node : NULL;
> +}
> +EXPORT_SYMBOL_GPL(software_node_find_by_name);
> +
>  static int
>  software_node_register_properties(struct software_node *node,
>                                   const struct property_entry *properties)
> diff --git a/include/linux/property.h b/include/linux/property.h
> index 5a910ad79591..9b3d4ca3a73a 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -421,6 +421,10 @@ bool is_software_node(const struct fwnode_handle *fwnode);
>  const struct software_node *to_software_node(struct fwnode_handle *fwnode);
>  struct fwnode_handle *software_node_fwnode(const struct software_node *node);
>
> +const struct software_node *
> +software_node_find_by_name(const struct software_node *parent,
> +                          const char *name);
> +
>  int software_node_register_nodes(const struct software_node *nodes);
>  void software_node_unregister_nodes(const struct software_node *nodes);
>
> --
> 2.20.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] software node: Add software_node_find_by_name()

[Heikki Krogerus]

On Thu, Aug 15, 2019 at 03:55:40PM +0300, Andy Shevchenko wrote:
> On Thu, Aug 15, 2019 at 2:32 PM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > Function that searches software nodes by node name.
> 
> > +/**
> > + * software_node_find_by_name - Find software node by name
> > + * @parent: Parent of the software node
> > + * @name: Name of the software node
> > + *
> > + * The function will find a node that is child of @parent and that is named
> > + * @name. If no node is found, the function returns NULL.
> 
> Shouldn't we add that the caller responsible of putting kobject?

OK. I'll fix this and the other one too.

Thanks Andy!

-- 
heikki

[PATCH 2/3] usb: roles: intel_xhci: Supplying software node for the role mux

[Heikki Krogerus]

The primary purpose for this node will be to allow linking
the users of the switch to it. The users will be for example
USB Type-C connectors. By supplying a reference to this
node in the software nodes representing the USB Type-C
controllers or connectors, the drivers for those devices can
access the switch.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/roles/intel-xhci-usb-role-switch.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/roles/intel-xhci-usb-role-switch.c b/drivers/usb/roles/intel-xhci-usb-role-switch.c
index 277de96181f9..1e86c01da35c 100644
--- a/drivers/usb/roles/intel-xhci-usb-role-switch.c
+++ b/drivers/usb/roles/intel-xhci-usb-role-switch.c
@@ -39,6 +39,10 @@ struct intel_xhci_usb_data {
 	void __iomem *base;
 };
 
+static const struct software_node intel_xhci_usb_node = {
+	"intel-xhci-usb-sw",
+};
+
 static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role)
 {
 	struct intel_xhci_usb_data *data = dev_get_drvdata(dev);
@@ -122,7 +126,7 @@ static enum usb_role intel_xhci_usb_get_role(struct device *dev)
 	return role;
 }
 
-static const struct usb_role_switch_desc sw_desc = {
+static struct usb_role_switch_desc sw_desc = {
 	.set = intel_xhci_usb_set_role,
 	.get = intel_xhci_usb_get_role,
 	.allow_userspace_control = true,
@@ -133,6 +137,7 @@ static int intel_xhci_usb_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct intel_xhci_usb_data *data;
 	struct resource *res;
+	int ret;
 
 	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
@@ -147,6 +152,12 @@ static int intel_xhci_usb_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, data);
 
+	ret = software_node_register(&intel_xhci_usb_node);
+	if (ret)
+		return ret;
+
+	sw_desc.fwnode = software_node_fwnode(&intel_xhci_usb_node);
+
 	data->role_sw = usb_role_switch_register(dev, &sw_desc);
 	if (IS_ERR(data->role_sw))
 		return PTR_ERR(data->role_sw);
-- 
2.20.1

Re: [PATCH 2/3] usb: roles: intel_xhci: Supplying software node for the role mux

[Andy Shevchenko]

On Thu, Aug 15, 2019 at 2:28 PM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> The primary purpose for this node will be to allow linking
> the users of the switch to it. The users will be for example
> USB Type-C connectors. By supplying a reference to this
> node in the software nodes representing the USB Type-C
> controllers or connectors, the drivers for those devices can
> access the switch.

> -static const struct usb_role_switch_desc sw_desc = {
> +static struct usb_role_switch_desc sw_desc = {

I dunno what is better, but usual approach I'm using in other drivers
is to kmemdup() input constant structures in order to have originals
untouched.

-- 
With Best Regards,
Andy Shevchenko

[PATCH 3/3] platform/x86: intel_cht_int33fe: Use new API to gain access to the role switch

[Heikki Krogerus]

The driver for the Intel USB role mux now always supplies
software node for the role switch, so no longer checking
that, and never creating separate node for the role switch.
From now on using software_node_find_by_name() function to
get the handle to the USB role switch.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/platform/x86/intel_cht_int33fe.c | 57 +++++-------------------
 1 file changed, 10 insertions(+), 47 deletions(-)

diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
index 4fbdff48a4b5..1d5d877b9582 100644
--- a/drivers/platform/x86/intel_cht_int33fe.c
+++ b/drivers/platform/x86/intel_cht_int33fe.c
@@ -34,7 +34,6 @@ enum {
 	INT33FE_NODE_MAX17047,
 	INT33FE_NODE_PI3USB30532,
 	INT33FE_NODE_DISPLAYPORT,
-	INT33FE_NODE_ROLE_SWITCH,
 	INT33FE_NODE_USB_CONNECTOR,
 	INT33FE_NODE_MAX,
 };
@@ -45,7 +44,6 @@ struct cht_int33fe_data {
 	struct i2c_client *pi3usb30532;
 
 	struct fwnode_handle *dp;
-	struct fwnode_handle *mux;
 };
 
 static const struct software_node nodes[];
@@ -139,46 +137,10 @@ static const struct software_node nodes[] = {
 	{ "max17047", NULL, max17047_props },
 	{ "pi3usb30532" },
 	{ "displayport" },
-	{ "usb-role-switch" },
 	{ "connector", &nodes[0], usb_connector_props, usb_connector_refs },
 	{ }
 };
 
-static int cht_int33fe_setup_mux(struct cht_int33fe_data *data)
-{
-	struct fwnode_handle *fwnode;
-	struct device *dev;
-	struct device *p;
-
-	fwnode = software_node_fwnode(&nodes[INT33FE_NODE_ROLE_SWITCH]);
-	if (!fwnode)
-		return -ENODEV;
-
-	/* First finding the platform device */
-	p = bus_find_device_by_name(&platform_bus_type, NULL,
-				    "intel_xhci_usb_sw");
-	if (!p)
-		return -EPROBE_DEFER;
-
-	/* Then the mux child device */
-	dev = device_find_child_by_name(p, "intel_xhci_usb_sw-role-switch");
-	put_device(p);
-	if (!dev)
-		return -EPROBE_DEFER;
-
-	/* If there already is a node for the mux, using that one. */
-	if (dev->fwnode)
-		fwnode_remove_software_node(fwnode);
-	else
-		dev->fwnode = fwnode;
-
-	data->mux = fwnode_handle_get(dev->fwnode);
-	put_device(dev);
-	mux_ref.node = to_software_node(data->mux);
-
-	return 0;
-}
-
 static int cht_int33fe_setup_dp(struct cht_int33fe_data *data)
 {
 	struct fwnode_handle *fwnode;
@@ -211,10 +173,9 @@ static void cht_int33fe_remove_nodes(struct cht_int33fe_data *data)
 {
 	software_node_unregister_nodes(nodes);
 
-	if (data->mux) {
-		fwnode_handle_put(data->mux);
+	if (mux_ref.node) {
+		fwnode_handle_put(software_node_fwnode(mux_ref.node));
 		mux_ref.node = NULL;
-		data->mux = NULL;
 	}
 
 	if (data->dp) {
@@ -235,14 +196,16 @@ static int cht_int33fe_add_nodes(struct cht_int33fe_data *data)
 	/* 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 find
-	 * the mux device and assign our node directly to it. That means we
-	 * depend on the mux driver. This function will return -PROBE_DEFER
-	 * until the mux device is registered.
+	 * 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.
 	 */
-	ret = cht_int33fe_setup_mux(data);
-	if (ret)
+	mux_ref.node = software_node_find_by_name(NULL, "intel-xhci-usb-sw");
+	if (!mux_ref.node) {
+		ret = -EPROBE_DEFER;
 		goto err_remove_nodes;
+	}
 
 	/*
 	 * The DP connector does have ACPI device node. In this case we can just
-- 
2.20.1

Re: [PATCH 3/3] platform/x86: intel_cht_int33fe: Use new API to gain access to the role switch

[Andy Shevchenko]

On Thu, Aug 15, 2019 at 2:28 PM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> The driver for the Intel USB role mux now always supplies
> software node for the role switch, so no longer checking
> that, and never creating separate node for the role switch.
> From now on using software_node_find_by_name() function to
> get the handle to the USB role switch.

Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>

>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>  drivers/platform/x86/intel_cht_int33fe.c | 57 +++++-------------------
>  1 file changed, 10 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
> index 4fbdff48a4b5..1d5d877b9582 100644
> --- a/drivers/platform/x86/intel_cht_int33fe.c
> +++ b/drivers/platform/x86/intel_cht_int33fe.c
> @@ -34,7 +34,6 @@ enum {
>         INT33FE_NODE_MAX17047,
>         INT33FE_NODE_PI3USB30532,
>         INT33FE_NODE_DISPLAYPORT,
> -       INT33FE_NODE_ROLE_SWITCH,
>         INT33FE_NODE_USB_CONNECTOR,
>         INT33FE_NODE_MAX,
>  };
> @@ -45,7 +44,6 @@ struct cht_int33fe_data {
>         struct i2c_client *pi3usb30532;
>
>         struct fwnode_handle *dp;
> -       struct fwnode_handle *mux;
>  };
>
>  static const struct software_node nodes[];
> @@ -139,46 +137,10 @@ static const struct software_node nodes[] = {
>         { "max17047", NULL, max17047_props },
>         { "pi3usb30532" },
>         { "displayport" },
> -       { "usb-role-switch" },
>         { "connector", &nodes[0], usb_connector_props, usb_connector_refs },
>         { }
>  };
>
> -static int cht_int33fe_setup_mux(struct cht_int33fe_data *data)
> -{
> -       struct fwnode_handle *fwnode;
> -       struct device *dev;
> -       struct device *p;
> -
> -       fwnode = software_node_fwnode(&nodes[INT33FE_NODE_ROLE_SWITCH]);
> -       if (!fwnode)
> -               return -ENODEV;
> -
> -       /* First finding the platform device */
> -       p = bus_find_device_by_name(&platform_bus_type, NULL,
> -                                   "intel_xhci_usb_sw");
> -       if (!p)
> -               return -EPROBE_DEFER;
> -
> -       /* Then the mux child device */
> -       dev = device_find_child_by_name(p, "intel_xhci_usb_sw-role-switch");
> -       put_device(p);
> -       if (!dev)
> -               return -EPROBE_DEFER;
> -
> -       /* If there already is a node for the mux, using that one. */
> -       if (dev->fwnode)
> -               fwnode_remove_software_node(fwnode);
> -       else
> -               dev->fwnode = fwnode;
> -
> -       data->mux = fwnode_handle_get(dev->fwnode);
> -       put_device(dev);
> -       mux_ref.node = to_software_node(data->mux);
> -
> -       return 0;
> -}
> -
>  static int cht_int33fe_setup_dp(struct cht_int33fe_data *data)
>  {
>         struct fwnode_handle *fwnode;
> @@ -211,10 +173,9 @@ static void cht_int33fe_remove_nodes(struct cht_int33fe_data *data)
>  {
>         software_node_unregister_nodes(nodes);
>
> -       if (data->mux) {
> -               fwnode_handle_put(data->mux);
> +       if (mux_ref.node) {
> +               fwnode_handle_put(software_node_fwnode(mux_ref.node));
>                 mux_ref.node = NULL;
> -               data->mux = NULL;
>         }
>
>         if (data->dp) {
> @@ -235,14 +196,16 @@ static int cht_int33fe_add_nodes(struct cht_int33fe_data *data)
>         /* 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 find
> -        * the mux device and assign our node directly to it. That means we
> -        * depend on the mux driver. This function will return -PROBE_DEFER
> -        * until the mux device is registered.
> +        * 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.
>          */
> -       ret = cht_int33fe_setup_mux(data);
> -       if (ret)
> +       mux_ref.node = software_node_find_by_name(NULL, "intel-xhci-usb-sw");
> +       if (!mux_ref.node) {
> +               ret = -EPROBE_DEFER;
>                 goto err_remove_nodes;
> +       }
>
>         /*
>          * The DP connector does have ACPI device node. In this case we can just
> --
> 2.20.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/3] software node: Introduce software_node_find_by_name()

[Hans de Goede]

Hi,

On 15-08-19 13:28, Heikki Krogerus wrote:
> Hi,
> 
> This helper makes it much more easier to access "external" nodes.

This series looks good and I've also tested it and it works
as it should (the usb role-sw is still found and controlled
properly) on a device using the intel_cht_int33fe driver
for its TypeC connector:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans