Hello Again Stephen,

I did already send v5 prior to your reply but I will create v6 today
based on this discussion.

On Mon, Dec 03, 2018 at 03:35:10PM -0800, Stephen Boyd wrote:

Quoting Matti Vaittinen (2018-11-30 02:50:22)

quoted

Hello Stephen,

Thanks a bunch for taking the time and reviewing this!

On Fri, Nov 30, 2018 at 12:54:10AM -0800, Stephen Boyd wrote:

quoted

Quoting Matti Vaittinen (2018-11-13 03:55:58)

quoted

-int devm_of_clk_add_hw_provider(struct device *dev,
+static int __devm_of_clk_add_hw_provider(struct device *dev,
                        struct clk_hw *(*get)(struct of_phandle_args *clkspec,
                                              void *data),
-                       void *data)
+                       struct device_node *of_node, void *data)
 {
-       struct device_node **ptr, *np;
+       struct device_node **ptr;
        int ret;
 
        ptr = devres_alloc(devm_of_clk_release_provider, sizeof(*ptr),

@@ -3906,10 +3906,9 @@ int devm_of_clk_add_hw_provider(struct device *dev,
        if (!ptr)
                return -ENOMEM;
 
-       np = dev->of_node;
-       ret = of_clk_add_hw_provider(np, get, data);
+       *ptr = of_node;
+       ret = of_clk_add_hw_provider(of_node, get, data);
        if (!ret) {
-               *ptr = np;

Why is this moved outside of the if condition?

I completely removed the local variable np and just unconditionally set
the allocated devres to point at the node (if allocation succeeded). We
could of course only do this if the provider registration succeeded and
save one assignment - but I guess I intended to remove the curly braces
and thus decided to go for one liner after if. But apparently I didn't
remove the braces O_o. Well, I can put the assignment inside the
condition if you prefer that.

quoted

In fact, why isn't just
the first line in this hunk deleted and passed to this function as
struct device_node *np?

I am sorry but I don't quite follow your suggestion here. Do you mean we
could just pass the struct device_node *np in devres_add()? I thought
the pointer passed to devress_add() should be allocated using
devres_alloc. Can you please elaborate what you mean?

I'm just trying to reduce the diff in the patch.

Oh, right. I will see how renaming the argument to np would impact to
patch size. iActually, I never consider the patch size at all - I have
only been concentrating on how the resulting file looks like. It didn't
ever cross my mind that patch size matters. But I guess the size of
chanes is really meaningfull when the amount of changes is large.

quoted

quoted

quoted

                devres_add(dev, ptr);
        } else {
                devres_free(ptr);

[..]

quoted

quoted

quoted

+int devm_of_clk_add_hw_provider(struct device *dev,
+                       struct clk_hw *(*get)(struct of_phandle_args *clkspec,
+                                             void *data),
+                       void *data)
+{
+       return __devm_of_clk_add_hw_provider(dev, get, dev->of_node, data);
+}
 EXPORT_SYMBOL_GPL(devm_of_clk_add_hw_provider);
 
+int devm_of_clk_add_parent_hw_provider(struct device *dev,
+                       struct clk_hw *(*get)(struct of_phandle_args *clkspec,
+                                             void *data),
+                       void *data)
+{
+       return __devm_of_clk_add_hw_provider(dev, get, dev->parent->of_node,

I'm wondering if we can somehow auto-detect this in
devm_of_clk_add_hw_provider() by looking for #clock-cells in the node.
If it isn't there, then we go to the parent node and look for a
#clock-cells property there in the DT node for that device. Does that
make sense? Then there isn't any new API and we can attach the lifetime
of the devm registration to the presence of the property indicating this
is a clk controller or not.

Huh. I don't know why but building this kind of logic in core is a bit
scary to me. I guess I can try implementing something like this - but I
am not really a fan of this. (Accidentally) omit the #clock-cells from
node and we go to parent node - I am a novice on this area but this
sounds like a potential hazard to me. I believe the driver should know
if it's properties should be in own or parent node - and if they are
not, then there should be no guessing but error. The lifetime is topic
where I would like to get information from you who know the kernel
better than I do =) But I guess the parent node is there at least as
long as the child device is alive. So for me the life time of
get-callback is more crucial - but as I said, I don't understand the
kernel in details so you probably know it better than me. But please let
me know your final take on this and I will follow the guidance =)

Please do the magic instead of adding another API. It makes things
simpler and will work for this case without having to change anything
besides of_clk_add_provider().

All right. Let's go on this direction then.

If the DT doesn't have the #clock-cells property in the node being
registered then calling clk_get() will fail for any consumer devices
that point to the node with a phandle and clock specifier. I don't
expect us to get very far into development if that's the case.

Makes sense. So only potential thing to break is if someone out there
has broken DT/driver - where they currently see this failure. Eg. they
use node w/o #clock-cells as provider and where they try and fail
controlling this clock - but ignore the error (and system just "works"
with HW defaults). After this change they may actually succeed in
controlling - but do control wrong clock.

Not likely scenario (sure happens somewhere) - and it involves already
broken design. So I agree with you. Besides, you are the maintainer for
clk framework and thus get the most of the rain if **** hits the fan =D

Of course, we don't fail in of_clk_add_provider() if there isn't a
#clock-cells property in the node, we just happily add the node to the
provider list and carry on. I doubt anyone is failing to specify the DT
property, but maybe they are, in which case we could keep not failing
and just add the node of whatever we're called with originally if
neither the parent or the passed node have the #clock-cells property. I
wouldn't try to go any higher than one node above the current node and
look for a #clock-cells though.

I think we should use parent device's node, not the paren node in DT,
right? But I agree, we should only look "one level up in the chain".

If this all still seems scary then don't worry about it, I'll implement
it myself.

It still is somewhat "scary" - but I really would like to use the devm
based provider registration in the bd718x7 driver so I will implement it
in this series. The engineer version of the "living on the edge", you
know =)

Br,
	Matti Vaittinen

-- 
Matti Vaittinen
ROHM Semiconductors

~~~ "I don't think so," said Rene Descartes.  Just then, he vanished ~~~

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel