Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201106150747.31273-1-broonie@kernel.org> <CAGETcx-abGAgYrHM0jm6hVkrJ5KvfhK6gCZ4Y6RY0msPJDCuQg@mail.gmail.com>
 <20201106192300.GG49612@sirena.org.uk>
In-Reply-To: <20201106192300.GG49612@sirena.org.uk>
From:   Saravana Kannan <saravanak@google.com>
Date:   Fri, 6 Nov 2020 17:55:10 -0800
Message-ID: <CAGETcx_hoDS1jHxfvrQv_+oMQw6E=AAiPANED+Ob6+a9mohW_A@mail.gmail.com>
Subject: Re: [PATCH RFC] driver core: Ensure DT devices always have fwnode set
To:     Mark Brown <broonie@kernel.org>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Rafael J . Wysocki" <rafael@kernel.org>,
        Daniel Mentz <danielmentz@google.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Frank Rowand <frowand.list@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Nov 6, 2020 at 11:23 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, Nov 06, 2020 at 11:09:17AM -0800, Saravana Kannan wrote:
>
> > If you want to do this in "one common place", then I think the way to
> > do this is have include/linux/of.h provide something like:
>
> > void of_set_device_of_node(dev, of_node)
> > {
> >     dev->of_node = of_node;
> >     dev->fw_node = &of_node->fwnode;
> >    /* bunch of other housekeeping like setting OF_POPULATED and doing
> > proper of_node_get() */
> >    // Passing NULL for of_node could undo all the above for dev->of_node.
> > }
>
> That also sounds good, particularly if we have a coccinelle spatch

I've never used coccinelle. But I can fix 5-10 easily findable ones
like i2c, platform, spi, slimbus, spmi, etc.

> or
> some other mechanism that enforced the usage of the function where
> appropriate, my main concern is making sure that we do something which
> ensures that the boilerplate stuff is handled.

Rob/Frank,

I spent an hour or more looking at this and there are many ways of
doing this. Wanted to know how much you wanted to do inside these
boilerplate functions.

This is the minimum we should do in these helper functions.

+/**
+ * of_unset_dev_of_node - Unset a device's of_node
+ * @dev - device to unset the of_node for
+ *
+ * Use this when you delete a device on which you had called
+ * of_set_dev_of_node() before.
+ */
+static inline void of_unset_dev_of_node(struct device *dev)
+{
+       struct device_node *node = dev->of_node
+
+       if (!node)
+               return;
+
+       dev->fwnode = NULL;
+       dev->of_node = NULL;
+       of_node_put(node);
+}
+
+/**
+ * of_set_dev_of_node - Set a device's of_node
+ * @dev - device to set the of_node for
+ * @node - the device_node that the device was constructed from
+ *
+ * Use this when you create a new device from a device_node. It takes care some
+ * of the housekeeping work that's necessary when you set a device's of_node.
+ *
+ * Use of_unset_dev_of_node() before you delete the device.
+ *
+ * Returns an error if the device already has its of_node set.
+ */
+static inline int of_set_dev_of_node(struct device *dev, struct
device_node *node)
+{
+       if (!node)
+               return 0;
+
+       if (WARN_ON(dev->of_node))
+               return -EBUSY;
+
+       of_node_get(node);
+       dev->of_node = node;
+       dev->fwnode = of_fwnode_handle(node);
+
+       return 0;
+}

But I also had another version that set/cleared OF_POPULATED. But that
meant of_device_alloc() will allocate the device before checking if
the node has already been populated (because I'd delete the check
that's already there and use the one rolled into these helper
functions). I think that inefficiency is okay because I don't think
"trying to populate an already populated node" would be a common
occurrence. But I wasn't sure how you'd feel about it.

Any preferences? Thoughts?

Additional context:
https://lore.kernel.org/lkml/20201104205431.3795207-2-saravanak@google.com/

-Saravana