Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
To:     Jerome Brunet <jbrunet@baylibre.com>,
        Michael Turquette <mturquette@baylibre.com>
In-Reply-To: <9e86b95eb8c9e3c0454d3aeebac15e354298c815.camel@baylibre.com>
User-Agent: alot/0.8
Subject: Re: [PATCH 2/9] clk: Introduce get_parent_hw clk op
Message-ID: <155025010824.115909.1113764424081649980@swboyd.mtv.corp.google.com>
References: <20190129061021.94775-1-sboyd@kernel.org> <20190129061021.94775-3-sboyd@kernel.org> <e303573aefeaf6a7e3a4b1d820e801354ff900b4.camel@baylibre.com> <154879654428.136743.10048771201181501034@swboyd.mtv.corp.google.com> <ff76dcdb4ceb3209619e5348ed7b575a8ae9df8d.camel@baylibre.com> <154888381385.169292.12776041058756822056@swboyd.mtv.corp.google.com> <97c2375f41fe7dabc4d71f66c9808912eb0ce611.camel@baylibre.com> <154941131193.169292.7500057932043013834@swboyd.mtv.corp.google.com> <9e86b95eb8c9e3c0454d3aeebac15e354298c815.camel@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>
From:   Stephen Boyd <sboyd@kernel.org>
Date:   Fri, 15 Feb 2019 09:01:48 -0800
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Quoting Jerome Brunet (2019-02-13 01:16:18)
> On Tue, 2019-02-05 at 16:01 -0800, Stephen Boyd wrote:
> > >=20
> > > I really don't understand why you insist on keeping this special case=
 for
> > > num_parent =3D=3D 1, when we know it is not coherent.
> > >=20
> > > 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 t=
o do
> > > it.
> > >=20
> > >=20
> >=20
> > 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.
>=20
> Fair enough.
>=20
> >  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.
> >=20
> > 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.=20
>=20
> Fine by me. Of course step #1 is not required if you get this is in befor=
e.
> 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.

>=20
> > 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.
>=20
> 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.

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

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.