Re: [PATCH v2] clk: Introduce clk_has_parent()

[Thierry Reding]

On Wed, Jan 21, 2015 at 04:16:05PM -0800, Stephen Boyd wrote:
> On 01/21/2015 08:13 AM, Thierry Reding wrote:
> > From: Thierry Reding <treding@xxxxxxxxxx>
> >
> > This new function is similar to clk_set_parent(), except that it doesn't
> > actually change the parent. It merely checks that the given parent clock
> > can be a parent for the given clock.
> >
> > A situation where this is useful is to check that a particular setup is
> > valid before switching to it. One specific use-case for this is atomic
> > modesetting in the DRM framework where setting a mode is divided into a
> > check phase where a given configuration is validated before applying
> > changes to the hardware.
> >
> > Cc: Russell King <linux@xxxxxxxxxxxxxxxx>
> > Cc: Mike Turquette <mturquette@xxxxxxxxxx>
> > Cc: Stephen Boyd <sboyd@xxxxxxxxxxxxxx>
> > Signed-off-by: Thierry Reding <treding@xxxxxxxxxx>
> > ---
> 
> Reviewed-by: Stephen Boyd <sboyd@xxxxxxxxxxxxxx>
> 
> This will slightly conflict with Tomeu's  patches for per-user clock
> constraints. It would be best if we can take this through the clk tree
> to fix up any conflicts

I had hoped to take this through the drm tree to resolve the build-time.
Another possibility would be for me to include the clk tree (or a subset
thereof) in my pull-request. That way you can still fix things up in the
clock tree if there are any conflicts with other work. We could make
that work two ways: this patch gets applied to the clk tree and I pull
it, or I provide a stable branch that I base my pull request on and that
branch can be pulled into the clk tree.

Yet another alternative would be to split out the clk_has_parent()
change from the series and not use it for now. That way we're going to
miss this check, but we do that anyway currently and it will only be
temporary until v3.21.

Perhaps given where we are in the release cycle the latter would make
the most sense for now.

> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
[...]
> > +	/* Optimize for the case where the parent is already the parent. */
> > +	if (clk->parent == parent)
> > +		return true;
> 
> I worry that we'll need to grab a lock here with Tomeu's patches, but
> maybe I'm wrong.

Why would this need a lock? Worst case somebody concurrently changes the
parent, in which case will not succeed and fallback to the lookup below.

It's been a while since I last looked at Tomeu's series, but I seem to
remember that struct clk was going to be per-user, in which case I guess
the code would have to be modified anyway since ->parent and
->parent_names will likely become properties of the clock structure
shared by all users (was it struct clk_core?).

Thierry

Attachment: pgpe96PaJvIqp.pgp
Description: PGP signature