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)
> > 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:
> > > Quoting Matti Vaittinen (2018-11-13 03:55:58)
> > > > -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.
> >
> > > 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.
> > > > devres_add(dev, ptr);
> > > > } else {
> > > > devres_free(ptr);
> [..]
> > >
> > > > +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 ~~~
