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
