Messages in this thread | | | Subject | Re: [PATCH 2/9] clk: Introduce get_parent_hw clk op | From | Stephen Boyd <> | Date | Fri, 15 Feb 2019 09:01:48 -0800 |
| |
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.
| |