Date:   Tue, 25 Aug 2020 20:58:09 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Saravana Kannan <saravanak@...gle.com>
Cc:     Liam Girdwood <lgirdwood@...il.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        John Stultz <john.stultz@...aro.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Android Kernel Team <kernel-team@...roid.com>,
        Stephen Boyd <sboyd@...nel.org>
Subject: Re: [PATCH v3 3/4] regulator: core: Add basic enable/disable support
 for sync_state() callbacks

On Tue, Aug 04, 2020 at 10:10:49PM +0100, Mark Brown wrote:
> On Tue, Jul 28, 2020 at 02:14:52PM -0700, Saravana Kannan wrote:

> > So, say you have a callback that you get for every single consumer
> > binding successfully. What can you do there? For every consumer, you
> > have to parse their firmware (Eg: DT node) to see what all resources
> > they use (Eg: all the -supply properties)

> Again off the top of my head we could also do this the other way around
> and when making a link go and ask the subsystems if it's their link and
> they have a better idea about where to put it.  Actually, having found
> the code that adds the links we don't even need to ask the subsystems if
> it's their link - we already know at the time we're doing the parsing
> which subsystem a link relates to!  Perhaps we could do some of this
> checking/notification at the time links are connected/satisfied rather
> than at parse time, or perhaps when the suppliers register they could
> look at outgoing links.

> We already have a set of links and we already have the ability to figure
> out which resources they're talking about, we just need to join those
> two things up which shouldn't be an intractable problem.

So, having taken a look at the device_link stuff in the driver core it
seems like it should be easy enough to add another parameter to
device_link_add() or a variant thereof so we can get a supplier ID of
some kind to the core (eg, a callback plus ID) along with the link so I
don't see any issue with getting the data in there.

We then need to figure out how to use that in device_links_driver_bound(),
that is currently unconditionally kicking _queue_sync_state() for every
supplier to go check if we need to do the sync_state() so would seem to
be the logical place to schedule a per subsystem callback.  It also
deletes the link if it was a _SYNC_STATE link which does make things a
bit more fun but we can always remember which link we're deleting and
pass that on when scheduling the kick.  Indeed, it occurs to me that we
could be cute here and in _queue_sync_state() have it check while
scanning the consumer links to see if we find a consumer with the same
subsystem callback information and if we don't then that must've been
the last link that just got deleted and we can call the callback.

That's not quite everything, in particular at least for any subsystem
where the core can be modular you'd need to have a layer of indirection
for the callback (it's possibly a good idea to do that anyway), but I'm
not seeing anything new with regard to locking or whatever.  It looks
like the work already done for basic sync_state() should be reusable
unless there's some nasty gotchas I'm not seeing, that was pretty much
what I was expecting to see TBH.

BTW doesn't the fact that we throw away the _SYNC_STATE_ONLY links cause
fun if the provider driver gets unbound then rebound?  We don't reparse
the DT to re-add those links.  I'm also not seeing where we ever clear
the state_synced flag that gets set which seems like it'd break things
if a supplier gets removed and reprobed.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists