Re: [PATCH 2/9] clk: Introduce get_parent_hw clk op

From: Stephen Boyd <sboyd@kernel.org>
To: Jerome Brunet <jbrunet@baylibre.com>,
	Michael Turquette <mturquette@baylibre.com>
Cc: linux-kernel@vger.kernel.org, linux-clk@vger.kernel.org,
	Miquel Raynal <miquel.raynal@bootlin.com>,
	Russell King <linux@armlinux.org.uk>
Subject: Re: [PATCH 2/9] clk: Introduce get_parent_hw clk op
Date: Fri, 15 Feb 2019 09:01:48 -0800	[thread overview]
Message-ID: <155025010824.115909.1113764424081649980@swboyd.mtv.corp.google.com> (raw)
In-Reply-To: <9e86b95eb8c9e3c0454d3aeebac15e354298c815.camel@baylibre.com>

Quoting Jerome Brunet (2019-02-13 01:16:18)
> On Tue, 2019-02-05 at 16:01 -0800, Stephen Boyd wrote:
> > > 
> > > I really don't understand why you insist on keeping this special case for
> > > num_parent == 1, when we know it is not coherent.
> > > 
> > > Considering, that I already proposed the fix, what is the effort here ?
> > > If it is fixing the driver that rely this weird thing, I'd be happy to do
> > > it.
> > > 
> > > 
> > 
> > Ok. I'm happy to merge your patch to always call the .get_parent clk op
> > when num_parents > 0, but please fix all the drivers and analyze all the
> > implementations of .get_parent to make sure that they aren't broken by
> > the change in behavior. Furthermore, please add a debug/warning message
> > into the code when .get_parent returns a number outside of the range of
> > [0, num_parents) so that they can be converted to use .get_parent_hw
> > instead.
> 
> Fair enough.
> 
> >  Ideally there wouldn't be anything returning a parent index
> > outside the range of possible parents from .get_parent because this
> > analysis of drivers would find those implementations and migrate them to
> > .get_parent_hw instead.
> > 
> > In parallel, I'd like to convert all drivers to use .get_parent_hw
> > instead of .get_parent and then remove the .get_parent clk op right
> > away. 
> 
> Fine by me. Of course step #1 is not required if you get this is in before.
> As long as things are coherent, I'm happy :)

Ok. So does it mean you want everything to be converted over the
.get_parent_hw and then all problems are solved? I think I can use
Coccinelle to convert the callers to pass the index straight into
clk_hw_get_parent_by_index(). I've also stacked a patch on top of this
series to make that API accept a negative index so I can directly chain
that call after the index is figured out.

> 
> > I'll start a sweep of the users of clk_hw_get_parent_by_index() (I
> > see 50 calls in the tree right now) and see if I can convert them to
> > handle errors returned from that API, probably by just continuing and
> > ignoring errors. I'll start doing the same conversion for .round_rate
> > and .determine_rate so that we can get rid of that duplicate clk op as
> > well. Hopefully that's a mostly mechanical conversion.
> 
> This would be nice !

Great! This isn't as easy to script, but it looks like I'll be bombing
the list with hundreds of patches soon.

> 
> > 
> > For now I'll move this patch to the end of this series so that it
> > doesn't hold things up otherwise.
> 
> It could even be separate series ? with the migration you mentionned above ?
> 

Yes that's fine to be a different series. I've reordered the patches now
so this patch comes last and I've also fixed up all callers of
clk_hw_get_parent_by_index() to handle the error case, with lots of
checks for IS_ERR_OR_NULL(). I've also made that API return an error now
and realized that this patch series needs to only do the fallback to the
'fallback' string when there isn't a direct pointer and when the DT
lookup fails with -ENOENT. We should assume that other errors mean that
something is wrong and the lookup actually failed vs. it being a DT
property that hasn't been implemented yet. Hopefully this is clearer
when I repost this series.