Setting fwnode->dev is specific to fw_devlink.

In order to avoid having a direct 'fwnode->dev = dev;' in several
place in the kernel, introduce fw_devlink_set_device() helper to perform
this operation.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 include/linux/fwnode.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index a921ca2fe940..a1345e274125 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -231,4 +231,10 @@ void fw_devlink_purge_absent_suppliers(struct fwnode_handle *fwnode);
 void fw_devlink_refresh_fwnode(struct fwnode_handle *fwnode);
 bool fw_devlink_is_strict(void);
 
+static inline void fw_devlink_set_device(struct fwnode_handle *fwnode,
+					 struct device *dev)
+{
+	fwnode->dev = dev;
+}
+
 #endif
-- 
2.49.0

On Fri, Jun 13, 2025 at 6:49 AM Herve Codina <herve.codina@bootlin.com> wrote:
>
> Setting fwnode->dev is specific to fw_devlink.
>
> In order to avoid having a direct 'fwnode->dev = dev;' in several
> place in the kernel, introduce fw_devlink_set_device() helper to perform
> this operation.
>

This should not be set anywhere outside the driver core files. I'll
get to reviewing the series, but until then, NACK to this.

Is there a specific patch that explain why we need to set this outside
driver core?

-Saravana

> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  include/linux/fwnode.h | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
> index a921ca2fe940..a1345e274125 100644
> --- a/include/linux/fwnode.h
> +++ b/include/linux/fwnode.h
> @@ -231,4 +231,10 @@ void fw_devlink_purge_absent_suppliers(struct fwnode_handle *fwnode);
>  void fw_devlink_refresh_fwnode(struct fwnode_handle *fwnode);
>  bool fw_devlink_is_strict(void);
>
> +static inline void fw_devlink_set_device(struct fwnode_handle *fwnode,
> +                                        struct device *dev)
> +{
> +       fwnode->dev = dev;
> +}
> +
>  #endif
> --
> 2.49.0
>

On Fri, Jun 13, 2025 at 02:13:49PM -0700, Saravana Kannan wrote:
> On Fri, Jun 13, 2025 at 6:49 AM Herve Codina <herve.codina@bootlin.com> wrote:

> > Setting fwnode->dev is specific to fw_devlink.
> >
> > In order to avoid having a direct 'fwnode->dev = dev;' in several
> > place in the kernel, introduce fw_devlink_set_device() helper to perform
> > this operation.
> 
> This should not be set anywhere outside the driver core files. I'll
> get to reviewing the series

Strictly speaking I agree with you, but this is not a some driver case,
it's very special and also we have some (ab)users of it.
I can relax the requirement to not set outside of a core functionality,
(like driver core, PCI core) which are tightly related to driver core
one.

Just my 2c.

-- 
With Best Regards,
Andy Shevchenko

Hi Saravana,

On Fri, 13 Jun 2025 14:13:49 -0700
Saravana Kannan <saravanak@google.com> wrote:

> On Fri, Jun 13, 2025 at 6:49 AM Herve Codina <herve.codina@bootlin.com> wrote:
> >
> > Setting fwnode->dev is specific to fw_devlink.
> >
> > In order to avoid having a direct 'fwnode->dev = dev;' in several
> > place in the kernel, introduce fw_devlink_set_device() helper to perform
> > this operation.
> >  
> 
> This should not be set anywhere outside the driver core files. I'll
> get to reviewing the series, but until then, NACK to this.
> 
> Is there a specific patch that explain why we need to set this outside
> driver core?

We need to set it in case of creating device-tree node for PCI.

Usually, fwnode are created (based on DT or ACPI) and then, dev are
created.

In the PCI DT node creation case, device are already created and then, based
on information already computed by the kernel, DT node are created.

You can see that on patch 11 (dev setting was already upstream and it is
replace by a call to the helper for PCI host bridge) and on patch 13 (PCI
device).

Other patches (8, 9 and 10) replace the existing direct setting of the dev
member by a call to the helper.

Best regards,
Hervé

Hi Saravana,

On Mon, 16 Jun 2025 09:04:06 +0200
Herve Codina <herve.codina@bootlin.com> wrote:

> Hi Saravana,
> 
> On Fri, 13 Jun 2025 14:13:49 -0700
> Saravana Kannan <saravanak@google.com> wrote:
> 
> > On Fri, Jun 13, 2025 at 6:49 AM Herve Codina <herve.codina@bootlin.com> wrote:  
> > >
> > > Setting fwnode->dev is specific to fw_devlink.
> > >
> > > In order to avoid having a direct 'fwnode->dev = dev;' in several
> > > place in the kernel, introduce fw_devlink_set_device() helper to perform
> > > this operation.
> > >    
> > 
> > This should not be set anywhere outside the driver core files. I'll
> > get to reviewing the series, but until then, NACK to this.
> > 
> > Is there a specific patch that explain why we need to set this outside
> > driver core?  
> 
> We need to set it in case of creating device-tree node for PCI.
> 
> Usually, fwnode are created (based on DT or ACPI) and then, dev are
> created.
> 
> In the PCI DT node creation case, device are already created and then, based
> on information already computed by the kernel, DT node are created.
> 
> You can see that on patch 11 (dev setting was already upstream and it is
> replace by a call to the helper for PCI host bridge) and on patch 13 (PCI
> device).
> 
> Other patches (8, 9 and 10) replace the existing direct setting of the dev
> member by a call to the helper.
> 

Have you got time to look at the series, patches I pointed out and the reply
from Andy?

Are modifications still nacked on your side?
If so, what kind of modification would you like to see in order to move
forward?

Best regards,
Hervé