Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 14 Oct 2021 15:27:51 +0200
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
Message-ID: <YWgwV3dW3Q9HQhlF@phenom.ffwll.local>
Mail-Followup-To: Stephen Boyd <swboyd@chromium.org>,
        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,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Rob Clark <robdclark@gmail.com>,
        Russell King <rmk+kernel@arm.linux.org.uk>,
        Saravana Kannan <saravanak@google.com>
References: <20211006193819.2654854-1-swboyd@chromium.org>
 <20211006193819.2654854-3-swboyd@chromium.org>
 <YV6HpM8NlO29UjAI@kroah.com>
 <CAE-0n51QPTT3TcgNJf2jDoXaRdxNnnyQY6--UqpNTp9ZKToqbw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAE-0n51QPTT3TcgNJf2jDoXaRdxNnnyQY6--UqpNTp9ZKToqbw@mail.gmail.com>
Precedence: bulk

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