On Tue, Jan 20, 2015 at 11:21:24AM -0800, Stephen Boyd wrote:
> On 01/20/2015 02:48 AM, Thierry Reding wrote:
> >
> > /**
> > + * clk_try_parent - check if a clock can be the parent clock source of another
> > + * @clk: clock source
> > + * @parent: parent clock source
> > + *
> > + * This is like clk_set_parent(), except that it only checks that parent can
> > + * be the parent clock source for clock.
> > + *
> > + * Returns success (0) or negative errno.
> > + */
> > +int clk_try_parent(struct clk *clk, struct clk *parent)
> > +{
> > + int err = 0;
> > +
> > + if (!clk || !parent)
> > + return -EINVAL;
>
> NULL clock should be a nop, so return success in either case.
Okay.
> > +
> > + if ((clk->num_parents > 1) && !clk->ops->set_parent)
> > + return -ENOSYS;
>
> This suffers from the same problem as discussed in another thread where
> the mux is read-only and the parent is the current parent. That case
> shouldn't fail.
Okay, if I do a lookup on the parent names array as you suggested below
I don't need to consider this anyway.
> > +
> > + clk_prepare_lock();
> > +
> > + if (clk->parent == parent)
> > + goto unlock;
> > +
> > + err = clk_fetch_parent_index(clk, parent);
> > + if (err > 0)
> > + err = 0;
> > +
>
> Given that we just throw away the index, perhaps we should just loop
> over the parent_names array searching for a name match on the parent's
> name. If we did that this entire function would be lockless too.
Done.
> > +unlock:
> > + clk_prepare_unlock();
> > +
> > + return err;
> > +}
> > +EXPORT_SYMBOL_GPL(clk_try_parent);
> > +
> > +/**
> > * clk_set_parent - switch the parent of a mux clk
> > * @clk: the mux clk whose input we are switching
> > * @parent: the new input to clk
> > diff --git a/include/linux/clk.h b/include/linux/clk.h
> > index fb1ac65f127c..94da8c68a515 100644
> > --- a/include/linux/clk.h
> > +++ b/include/linux/clk.h
> > @@ -328,6 +328,15 @@ long clk_round_rate(struct clk *clk, unsigned long rate);
> > int clk_set_rate(struct clk *clk, unsigned long rate);
> >
> > /**
> > + * clk_try_parent - check if a clock can be the parent clock source of another
> > + * @clk: clock source
> > + * @parent: parent clock source
> > + *
> > + * Returns success (0) or negative errno.
>
> Why not a bool? Do we really care why we can't set the parent in the
> error case?
A bool should do fine. I guess a negative error code would've been
easier to propagate, but we can probably just return an -EINVAL if
clk_has_parent() fails.
> > + */
> > +int clk_try_parent(struct clk *clk, struct clk *parent);
>
> The name makes me think of mutex_trylock(), so I immediately think this
> tries to set the parent. Perhaps a better name would be
> clk_can_have_parent() or clk_has_parent()?
clk_has_parent() sounds good to me.
Thierry
Attachment:
pgp1aHaZIelrQ.pgp
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
