[PATCH] pmdomain: imx: gpc: set fwnode from parent device

[Emil Kronborg Andersen]

On Wed, Oct 25, 2023 at 17:35 +0200, Ulf Hansson wrote:
> On Wed, 25 Oct 2023 at 12:12, Emil Kronborg Andersen <emkan at prevas.dk> wrote:
> >
> > On Tue, Oct 24, 2023 at 14:42 +0200, Ulf Hansson wrote:
> > > I had a bit closer look at this. If I understand things correctly,
> > > this problem is because the imx_gpc_driver's ->probe() (imx_gpc_probe)
> > > is allocating/adding a new platform device, per child-of-node to
> > > represent a power-domain.
> > >
> > > I suspect in most other cases where a platform device is being
> > > allocated per child node, is when the child node has a
> > > compatible-string and the parent driver is calling
> > > of_platform_populate(), or similar. In those cases, the "dev.fwnode"
> > > would be set accordingly, but not in the imx_gpc_driver case as it
> > > needs to be set explicitly.
> > >
> > > That said, it looks like we should move forward with the $subject
> > > patch. However, it looks like the patch [1] from Pengfei Li is more
> > > correct than the $subject patch.
> > >
> > > Kind regards
> > > Uffe
> > >
> > > [1]
> > > https://lore.kernel.org/all/20231020185949.537083-1-pengfei.li_1@nxp.com/
> >
> > That is also how I understand it works.
> >
> > I was unsure if the patch from Pengfei Li [1] solves the same issue as
> > this patch, since there are no errors included in the commit message.
> > However, after testing the patch in [1], I can confirm that it fixes:
> >
> > [    1.039830] imx-gpc 20dc000.gpc: Failed to create device link (0x180) with 20c8000.anatop:regulator-vddpu
> >
> > I based my solution on other patches, where device_set_node() is used.
> > For example, a26cc2934331 ("drm/mipi-dsi: Set the fwnode for
> > mipi_dsi_device") and 6dbe6c07f94f ("gpio: Propagate firmware node from
> > a parent"). I am curious: Why is the approach in [1] considered more
> > correct? It seems logical to set the fwnode as early as possible from
> > the parent its device.
> 
> Why do we want to use the parent's node? The devices (platform-device)
> that we allocate belongs to the corresponding parsed child node,
> doesn't it?
> 
> Kind regards
> Uffe
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

I think you are right about that.

In [1], dev.fwnode is set directly, but I think it is more correct to
use device_set_node() instead, to ensure that dev.of_node is also set
correctly like so:

diff --git a/drivers/pmdomain/imx/gpc.c b/drivers/pmdomain/imx/gpc.c
index 90a8b2c0676f..22d5c29da79e 100644
--- a/drivers/pmdomain/imx/gpc.c
+++ b/drivers/pmdomain/imx/gpc.c
@@ -497,7 +497,7 @@ static int imx_gpc_probe(struct platform_device *pdev)
                        domain->ipg_rate_mhz = ipg_rate_mhz;

                        pd_pdev->dev.parent = &pdev->dev;
-                       pd_pdev->dev.of_node = np;
+                       device_set_node(&pd_pdev->dev, of_fwnode_handle(np));

                        ret = platform_device_add(pd_pdev);
                        if (ret) {

[1]
https://lore.kernel.org/all/20231020185949.537083-1-pengfei.li_1@nxp.com/

Best regards,
Emil