[PATCH 0/2] Remove one more platform_device_add_properties() call

[PATCH 0/2] Remove one more platform_device_add_properties() call

[Heikki Krogerus]

Hi,

I originally introduced these as part of my series where I was
proposing PM ops for software nodes [1], but since that still needs
work, I'm sending these two separately.

So basically I'm only modifying dwc3-pci.c so it registers a software
node directly at this point. That will remove one more user of
platform_device_add_properties().

[1] https://lore.kernel.org/lkml/20201029105941.63410-1-heikki.krogerus@linux.intel.com/

thanks,

Heikki Krogerus (2):
  software node: Introduce device_add_software_node()
  usb: dwc3: pci: Register a software node for the dwc3 platform device

 drivers/base/swnode.c       | 69 ++++++++++++++++++++++++++++++++-----
 drivers/usb/dwc3/dwc3-pci.c | 61 +++++++++++++++++++-------------
 include/linux/property.h    |  3 ++
 3 files changed, 100 insertions(+), 33 deletions(-)

-- 
2.29.2

[PATCH 1/2] software node: Introduce device_add_software_node()

[Heikki Krogerus]

This helper will register a software node and then assign
it to device at the same time. The function will also make
sure that the device can't have more than one software node.

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

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 010828fc785bc..6742d8d63764d 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -48,6 +48,19 @@ EXPORT_SYMBOL_GPL(is_software_node);
 				     struct swnode, fwnode) : NULL;	\
 	})
 
+static inline struct swnode *dev_to_swnode(struct device *dev)
+{
+	struct fwnode_handle *fwnode = dev_fwnode(dev);
+
+	if (!fwnode)
+		return NULL;
+
+	if (!is_software_node(fwnode))
+		fwnode = fwnode->secondary;
+
+	return to_swnode(fwnode);
+}
+
 static struct swnode *
 software_node_to_swnode(const struct software_node *node)
 {
@@ -843,22 +856,60 @@ void fwnode_remove_software_node(struct fwnode_handle *fwnode)
 }
 EXPORT_SYMBOL_GPL(fwnode_remove_software_node);
 
+/**
+ * device_add_software_node - Assign software node to a device
+ * @dev: The device the software node is meant for.
+ * @swnode: The software node.
+ *
+ * This function will register @swnode and make it the secondary firmware node
+ * pointer of @dev. If @dev has no primary node, then @swnode will become the primary
+ * node.
+ */
+int device_add_software_node(struct device *dev, const struct software_node *swnode)
+{
+	int ret;
+
+	/* Only one software node per device. */
+	if (dev_to_swnode(dev))
+		return -EBUSY;
+
+	ret = software_node_register(swnode);
+	if (ret)
+		return ret;
+
+	set_secondary_fwnode(dev, software_node_fwnode(swnode));
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(device_add_software_node);
+
+/**
+ * device_remove_software_node - Remove device's software node
+ * @dev: The device with the software node.
+ *
+ * This function will unregister the software node of @dev.
+ */
+void device_remove_software_node(struct device *dev)
+{
+	struct swnode *swnode;
+
+	swnode = dev_to_swnode(dev);
+	if (!swnode)
+		return;
+
+	kobject_put(&swnode->kobj);
+}
+EXPORT_SYMBOL_GPL(device_remove_software_node);
+
 int software_node_notify(struct device *dev, unsigned long action)
 {
-	struct fwnode_handle *fwnode = dev_fwnode(dev);
 	struct swnode *swnode;
 	int ret;
 
-	if (!fwnode)
-		return 0;
-
-	if (!is_software_node(fwnode))
-		fwnode = fwnode->secondary;
-	if (!is_software_node(fwnode))
+	swnode = dev_to_swnode(dev);
+	if (!swnode)
 		return 0;
 
-	swnode = to_swnode(fwnode);
-
 	switch (action) {
 	case KOBJ_ADD:
 		ret = sysfs_create_link(&dev->kobj, &swnode->kobj,
diff --git a/include/linux/property.h b/include/linux/property.h
index 2d4542629d80b..3b6093f6bd04c 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -485,4 +485,7 @@ fwnode_create_software_node(const struct property_entry *properties,
 			    const struct fwnode_handle *parent);
 void fwnode_remove_software_node(struct fwnode_handle *fwnode);
 
+int device_add_software_node(struct device *dev, const struct software_node *swnode);
+void device_remove_software_node(struct device *dev);
+
 #endif /* _LINUX_PROPERTY_H_ */
-- 
2.29.2

[PATCH 2/2] usb: dwc3: pci: Register a software node for the dwc3 platform device

[Heikki Krogerus]

By registering the software node directly instead of just
the properties in it, the driver can take advantage of also
the other features the software nodes have.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/dwc3/dwc3-pci.c | 61 ++++++++++++++++++++++---------------
 1 file changed, 37 insertions(+), 24 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
index bae6a70664c80..037bc21bffa66 100644
--- a/drivers/usb/dwc3/dwc3-pci.c
+++ b/drivers/usb/dwc3/dwc3-pci.c
@@ -142,6 +142,18 @@ static const struct property_entry dwc3_pci_amd_properties[] = {
 	{}
 };
 
+static const struct software_node dwc3_pci_intel_swnode = {
+	.properties = dwc3_pci_intel_properties,
+};
+
+static const struct software_node dwc3_pci_intel_mrfld_swnode = {
+	.properties = dwc3_pci_mrfld_properties,
+};
+
+static const struct software_node dwc3_pci_amd_swnode = {
+	.properties = dwc3_pci_amd_properties,
+};
+
 static int dwc3_pci_quirks(struct dwc3_pci *dwc)
 {
 	struct pci_dev			*pdev = dwc->pci;
@@ -222,7 +234,6 @@ static void dwc3_pci_resume_work(struct work_struct *work)
 
 static int dwc3_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
 {
-	struct property_entry *p = (struct property_entry *)id->driver_data;
 	struct dwc3_pci		*dwc;
 	struct resource		res[2];
 	int			ret;
@@ -265,7 +276,7 @@ static int dwc3_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
 	dwc->dwc3->dev.parent = dev;
 	ACPI_COMPANION_SET(&dwc->dwc3->dev, ACPI_COMPANION(dev));
 
-	ret = platform_device_add_properties(dwc->dwc3, p);
+	ret = device_add_software_node(&dwc->dwc3->dev, (void *)id->driver_data);
 	if (ret < 0)
 		goto err;
 
@@ -288,6 +299,7 @@ static int dwc3_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
 
 	return 0;
 err:
+	device_remove_software_node(&dwc->dwc3->dev);
 	platform_device_put(dwc->dwc3);
 	return ret;
 }
@@ -304,75 +316,76 @@ static void dwc3_pci_remove(struct pci_dev *pci)
 #endif
 	device_init_wakeup(&pci->dev, false);
 	pm_runtime_get(&pci->dev);
+	device_remove_software_node(&dwc->dwc3->dev);
 	platform_device_unregister(dwc->dwc3);
 }
 
 static const struct pci_device_id dwc3_pci_id_table[] = {
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_BSW),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_BYT),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_MRFLD),
-	  (kernel_ulong_t) &dwc3_pci_mrfld_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_mrfld_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_CMLLP),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_CMLH),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_SPTLP),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_SPTH),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_BXT),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_BXT_M),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_APL),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_KBP),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_GLK),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_CNPLP),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_CNPH),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_CNPV),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ICLLP),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_EHLLP),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_TGPLP),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_TGPH),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_JSP),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ADLS),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_NL_USB),
-	  (kernel_ulong_t) &dwc3_pci_amd_properties, },
+	  (kernel_ulong_t) &dwc3_pci_amd_swnode, },
 	{  }	/* Terminating Entry */
 };
 MODULE_DEVICE_TABLE(pci, dwc3_pci_id_table);
-- 
2.29.2

Re: [PATCH 0/2] Remove one more platform_device_add_properties() call

[Rafael J. Wysocki]

On Mon, Nov 23, 2020 at 4:32 PM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> Hi,
>
> I originally introduced these as part of my series where I was
> proposing PM ops for software nodes [1], but since that still needs
> work, I'm sending these two separately.
>
> So basically I'm only modifying dwc3-pci.c so it registers a software
> node directly at this point. That will remove one more user of
> platform_device_add_properties().
>
> [1] https://lore.kernel.org/lkml/20201029105941.63410-1-heikki.krogerus@linux.intel.com/
>
> thanks,
>
> Heikki Krogerus (2):
>   software node: Introduce device_add_software_node()
>   usb: dwc3: pci: Register a software node for the dwc3 platform device
>
>  drivers/base/swnode.c       | 69 ++++++++++++++++++++++++++++++++-----
>  drivers/usb/dwc3/dwc3-pci.c | 61 +++++++++++++++++++-------------
>  include/linux/property.h    |  3 ++
>  3 files changed, 100 insertions(+), 33 deletions(-)
>
> --

These look good to me.

If you want me to take them, though, I need an ACK from the dwc3 side.

Re: [PATCH 0/2] Remove one more platform_device_add_properties() call

[Andy Shevchenko]

On Mon, Nov 23, 2020 at 06:06:31PM +0100, Rafael J. Wysocki wrote:
> On Mon, Nov 23, 2020 at 4:32 PM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > Hi,
> >
> > I originally introduced these as part of my series where I was
> > proposing PM ops for software nodes [1], but since that still needs
> > work, I'm sending these two separately.
> >
> > So basically I'm only modifying dwc3-pci.c so it registers a software
> > node directly at this point. That will remove one more user of
> > platform_device_add_properties().
> >
> > [1] https://lore.kernel.org/lkml/20201029105941.63410-1-heikki.krogerus@linux.intel.com/
> >
> > thanks,
> >
> > Heikki Krogerus (2):
> >   software node: Introduce device_add_software_node()
> >   usb: dwc3: pci: Register a software node for the dwc3 platform device
> >
> >  drivers/base/swnode.c       | 69 ++++++++++++++++++++++++++++++++-----
> >  drivers/usb/dwc3/dwc3-pci.c | 61 +++++++++++++++++++-------------
> >  include/linux/property.h    |  3 ++
> >  3 files changed, 100 insertions(+), 33 deletions(-)
> >
> > --
> 
> These look good to me.
> 
> If you want me to take them, though, I need an ACK from the dwc3 side.

Btw, I have tested this on one of the platform with DWC3 and found no
regression, so feel free to add

Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/2] Remove one more platform_device_add_properties() call

[Heikki Krogerus]

Hi Felipe,

On Mon, Nov 23, 2020 at 06:06:31PM +0100, Rafael J. Wysocki wrote:
> On Mon, Nov 23, 2020 at 4:32 PM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > Hi,
> >
> > I originally introduced these as part of my series where I was
> > proposing PM ops for software nodes [1], but since that still needs
> > work, I'm sending these two separately.
> >
> > So basically I'm only modifying dwc3-pci.c so it registers a software
> > node directly at this point. That will remove one more user of
> > platform_device_add_properties().
> >
> > [1] https://lore.kernel.org/lkml/20201029105941.63410-1-heikki.krogerus@linux.intel.com/
> >
> > thanks,
> >
> > Heikki Krogerus (2):
> >   software node: Introduce device_add_software_node()
> >   usb: dwc3: pci: Register a software node for the dwc3 platform device
> >
> >  drivers/base/swnode.c       | 69 ++++++++++++++++++++++++++++++++-----
> >  drivers/usb/dwc3/dwc3-pci.c | 61 +++++++++++++++++++-------------
> >  include/linux/property.h    |  3 ++
> >  3 files changed, 100 insertions(+), 33 deletions(-)
> >
> > --
> 
> These look good to me.
> 
> If you want me to take them, though, I need an ACK from the dwc3 side.

Is this OK?

thanks,

-- 
heikki

Re: [PATCH 0/2] Remove one more platform_device_add_properties() call

[Heikki Krogerus]

On Fri, Dec 04, 2020 at 01:23:22PM +0200, Heikki Krogerus wrote:
> Hi Felipe,
> 
> On Mon, Nov 23, 2020 at 06:06:31PM +0100, Rafael J. Wysocki wrote:
> > On Mon, Nov 23, 2020 at 4:32 PM Heikki Krogerus
> > <heikki.krogerus@linux.intel.com> wrote:
> > >
> > > Hi,
> > >
> > > I originally introduced these as part of my series where I was
> > > proposing PM ops for software nodes [1], but since that still needs
> > > work, I'm sending these two separately.
> > >
> > > So basically I'm only modifying dwc3-pci.c so it registers a software
> > > node directly at this point. That will remove one more user of
> > > platform_device_add_properties().
> > >
> > > [1] https://lore.kernel.org/lkml/20201029105941.63410-1-heikki.krogerus@linux.intel.com/
> > >
> > > thanks,
> > >
> > > Heikki Krogerus (2):
> > >   software node: Introduce device_add_software_node()
> > >   usb: dwc3: pci: Register a software node for the dwc3 platform device
> > >
> > >  drivers/base/swnode.c       | 69 ++++++++++++++++++++++++++++++++-----
> > >  drivers/usb/dwc3/dwc3-pci.c | 61 +++++++++++++++++++-------------
> > >  include/linux/property.h    |  3 ++
> > >  3 files changed, 100 insertions(+), 33 deletions(-)
> > >
> > > --
> > 
> > These look good to me.
> > 
> > If you want me to take them, though, I need an ACK from the dwc3 side.
> 
> Is this OK?

I think this went under you radar, so I'll resend these.

Br,

-- 
heikki