Quoting Jerome Brunet (2019-01-29 01:34:38)
> On Mon, 2019-01-28 at 22:10 -0800, Stephen Boyd wrote:
> > ---
> > drivers/clk/clk.c | 117 ++++++++++++++++++++++++++---------
> > include/linux/clk-provider.h | 9 +++
> > 2 files changed, 96 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 01b36f0851bd..5d82cf25bb29 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -2242,14 +2242,84 @@ struct clk *clk_get_parent(struct clk *clk)
> > }
> > EXPORT_SYMBOL_GPL(clk_get_parent);
> >
> > -static struct clk_core *__clk_init_parent(struct clk_core *core)
> > +static struct clk_core *
> > +__clk_init_parent(struct clk_core *core, bool update_orphan)
> > {
> > u8 index = 0;
> > + struct clk_hw *parent_hw = NULL;
> >
> > - if (core->num_parents > 1 && core->ops->get_parent)
> > - index = core->ops->get_parent(core->hw);
> > + if (core->ops->get_parent_hw) {
> > + parent_hw = core->ops->get_parent_hw(core->hw);
> > + /*
> > + * The provider driver doesn't know what the parent is,
> > + * but it's at least something valid, so it's not an
> > + * orphan, just a clk with some unknown parent.
> > + */
>
> I suppose this is the answer the discussion we had last year. I'm not sure it
> answer the problem. In the case I presented, we have no idea wether the
> setting is valid or not.
>
> We can't assume it is `at least something valid`, the value in the mux is just
> something we can't map.
So if you can't map the value in the mux how is that valid? I would
think the mux knows what indexes it has strings for, and if the index
isn't in there it's invalid. Is that not the case here?
>
> Aslo, could you provide an example of what such callback would be, with clk-
> mux maybe ?
Sounds fair. I can convert the clk-mux API to this op. It may be that we
need to make clk_hw_get_parent_by_index() return an error pointer
instead of NULL if it can't find the clk so that we can move the error
codes through this new API.
>
> I don't get how a clock driver will keep track of the clk_hw pointers it is
> connected to. Is there an API for this ? clk-mux must access to clk_core to
> explore his own parent ... which already does not scale well, expect if we
> plan to expose clk_core at some point ?
No we don't want to expose clk_core to provider drivers. It is only for
the use of the clk framework and it's not exposed even as an opaque
pointer. We have that core member of clk_hw but that's just to traverse
from clk_hw to clk_core, and not for anything else.
>
> > + if (!parent_hw && update_orphan)
> > + core->orphan = false;
> > + } else {
> > + if (core->num_parents > 1 && core->ops->get_parent)
>
> I still get why, when num_parents == 1, it is OK to call get_parent_hw() and
> no get_parent(). It does not seems coherent.
I'd rather not change behavior of existing code in this patch, so I took
the route of adding another callback with semantics that we can define
now because there aren't any users. The difference between the two is
made intentionally.
>
> > + index = core->ops->get_parent(core->hw);
> > +
> > + parent_hw = clk_hw_get_parent_by_index(core->hw, index);
> > + }
> > +
> > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> > index 60c51871b04b..8b84dee942bf 100644
> > --- a/include/linux/clk-provider.h
> > +++ b/include/linux/clk-provider.h
> > @@ -155,6 +155,14 @@ struct clk_duty {
> > * multiple parents. It is optional (and unnecessary) for clocks
> > * with 0 or 1 parents.
> > *
> > + * @get_parent_hw: Queries the hardware to determine the parent of a
> > clock. The
> > + * return value is a clk_hw pointer corresponding to
> > + * the parent clock. In short, this function translates the
> > parent
> > + * value read from hardware into a pointer to the clk_hw for that
> > clk.
> > + * Currently only called when the clock is initialized by
> > + * __clk_init. This callback is mandatory for clocks with
> > + * multiple parents. It is optional for clocks with 0 or 1
> > parents.
> > + *
>
> The comments above could imply that get_parent() and get_parent_hw() are both
> mandatory if num_parent > 1. (I don't think so but) Is this your intent ?
It is not the intent. I'll update the docs. Thanks.
