Re: [PATCH v2 02/34] component: Introduce the aggregate bus_type

From: Daniel Vetter <daniel@ffwll.ch>
To: Stephen Boyd <swboyd@chromium.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	dri-devel@lists.freedesktop.org, freedreno@lists.freedesktop.org,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Rob Clark <robdclark@gmail.com>,
	Russell King <rmk+kernel@arm.linux.org.uk>,
	Saravana Kannan <saravanak@google.com>
Subject: Re: [PATCH v2 02/34] component: Introduce the aggregate bus_type
Date: Thu, 14 Oct 2021 15:27:51 +0200	[thread overview]
Message-ID: <YWgwV3dW3Q9HQhlF@phenom.ffwll.local> (raw)
In-Reply-To: <CAE-0n51QPTT3TcgNJf2jDoXaRdxNnnyQY6--UqpNTp9ZKToqbw@mail.gmail.com>

On Thu, Oct 07, 2021 at 04:42:48PM -0400, Stephen Boyd wrote:
> Quoting Greg Kroah-Hartman (2021-10-06 22:37:40)
> > On Wed, Oct 06, 2021 at 12:37:47PM -0700, Stephen Boyd wrote:
> > >
> > > Let's make the component driver into an actual device driver that has
> > > probe/remove/shutdown functions. The driver will only be bound to the
> > > aggregate device once all component drivers have called component_add()
> > > to indicate they're ready to assemble the aggregate driver. This allows
> > > us to attach shutdown logic (and in the future runtime PM logic) to the
> > > aggregate driver so that it runs the hooks in the correct order.
> >
> > Why are you creating a new bus type and not using the auxiliary bus
> > instead?
> >
> > You have seen Documentation/driver-api/auxiliary_bus.rst, right?
> >
> 
> Nope, but I read it now. Thanks for the pointer.
> 
> My read of it is that the auxiliary bus is a way to slice up a single IP
> block into multiple devices and then have drivers attach to those
> different "pieces" of the IP. It avoids polluting the platform bus with
> devices that don't belong on the platform bus because they are sub
> components of a larger IP block that sits on the platform bus.
> 
> The aggregate bus is solving the reverse problem. It is rounding up a
> collection of IP blocks that live on some bus (platform, i2c, spi,
> whatever) and presenting them as a single aggregate device (sound card,
> display card, whatever) whenever all the component devices call
> component_add(). For example, we don't want to do operations on the
> entire display pipeline until all the devices that make up the display
> are probed and drivers are attached. I suppose the aggregate_device in
> this patch series has a 1:1 relationship with the drm class_type that
> makes up /sys/class/drm/cardN but there's also a couple sound users and
> a power_supply user so I don't know the equivalent there.
> 
> Long term, maybe all of this component code could be placed directly
> into the driver core? That's probably even more invasive of a change but
> I imagine we could make device links with component_add() as we're
> already doing with these patches and then have driver core call some
> class function pointer when all the links are probed. That would
> handle the 'bind/probe' callback for the aggregate device but it won't
> handle the component_bind_all() path where we call bind_component() for
> each component device that makes up the aggregate device. Maybe we can
> add even more devices for the components and then call probe there too.
> 
> Sorry that's a long-winded non-answer. I don't think they're solving the
> same problem so using the same bus type looks wrong. We'd have to take
> two different paths depending on what type of device it is (aggregate
> vs. auxiliary) so there's not much of anything that is shared code-wise.

Yeah component is the reverse of auxiliary, and right now a lot of
subsystems have their own hand-rolled version of this. I do hope that
component.c does become more of a standard (that's why it's in
drivers/base/), but I guess that's a bit tricky if the device model
maintainer hasn't seen it yet ...

Hopefully putting more proper device model concepts into it can fix this
problem :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch