Date:   Mon, 4 Jan 2021 16:51:51 -0800
From:   Dan Williams <dan.j.williams@...el.com>
To:     Jason Gunthorpe <jgg@...dia.com>
Cc:     Mark Brown <broonie@...nel.org>,
        Greg KH <gregkh@...uxfoundation.org>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
        alsa-devel@...a-project.org, Kiran Patil <kiran.patil@...el.com>,
        linux-rdma <linux-rdma@...r.kernel.org>,
        Shiraz Saleem <shiraz.saleem@...el.com>,
        Martin Habets <mhabets@...arflare.com>,
        Liam Girdwood <lgirdwood@...il.com>,
        Ranjani Sridharan <ranjani.sridharan@...ux.intel.com>,
        Fred Oh <fred.oh@...ux.intel.com>,
        Dave Ertman <david.m.ertman@...el.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Netdev <netdev@...r.kernel.org>,
        Leon Romanovsky <leonro@...dia.com>,
        David Miller <davem@...emloft.net>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Parav Pandit <parav@...lanox.com>,
        Lee Jones <lee.jones@...aro.org>
Subject: Re: [resend/standalone PATCH v4] Add auxiliary bus support

On Mon, Jan 4, 2021 at 4:14 PM Jason Gunthorpe <jgg@...dia.com> wrote:
>
> On Mon, Jan 04, 2021 at 09:19:30PM +0000, Mark Brown wrote:
>
>
> > > Regardless of the shortcut to make everything a struct
> > > platform_device, I think it was a mistake to put OF devices on
> > > platform_bus. Those should have remained on some of_bus even if they
> >
> > Like I keep saying the same thing applies to all non-enumerable buses -
> > exactly the same considerations exist for all the other buses like I2C
> > (including the ACPI naming issue you mention below), and for that matter
> > with enumerable buses which can have firmware info.
>
> And most busses do already have their own bus type. ACPI, I2C, PCI,
> etc. It is just a few that have been squished into platform, notably
> OF.
>

I'll note that ACPI is an outlier that places devices on 2 buses,
where new acpi_driver instances are discouraged [1] in favor of
platform_drivers. ACPI scan handlers are awkwardly integrated into the
Linux device model.

So while I agree with sentiment that an "ACPI bus" should
theoretically stand on its own there is legacy to unwind.

I only bring that up to keep the focus on how to organize drivers
going forward, because trying to map some of these arguments backwards
runs into difficulties.

[1]: http://lore.kernel.org/r/CAJZ5v0j_ReK3AGDdw7fLvmw_7knECCg2U_huKgJzQeLCy8smug@mail.gmail.com

> > > are represented by struct platform_device and fiddling in the core
> > > done to make that work OK.
> >
> > What exactly is the fiddling in the core here, I'm a bit unclear?
>
> I'm not sure, but I bet there is a small fall out to making bus_type
> not 1:1 with the struct device type.. Would have to attempt it to see
>
> > > This feels like a good conference topic someday..
> >
> > We should have this discussion *before* we get too far along with trying
> > to implement things, we should at least have some idea where we want to
> > head there.
>
> Well, auxillary bus is clearly following the original bus model
> intention with a dedicated bus type with a controlled naming
> scheme. The debate here seems to be "what about platform bus" and
> "what to do with mfd"?
>
> > Those APIs all take a struct device for lookup so it's the same call for
> > looking things up regardless of the bus the device is on or what
> > firmware the system is using - where there are firmware specific lookup
> > functions they're generally historical and shouldn't be used for new
> > code.  It's generally something in the form
> >
> >       api_type *api_get(struct device *dev, const char *name);
>
> Well, that is a nice improvement since a few years back when I last
> worked on this stuff.
>
> But now it begs the question, why not push harder to make 'struct
> device' the generic universal access point and add some resource_get()
> API along these lines so even a platform_device * isn't needed?
>
> Then the path seems much clearer, add a multi-bus-type device_driver
> that has a probe(struct device *) and uses the 'universal api_get()'
> style interface to find the generic 'resources'.
>
> The actual bus types and bus structs can then be split properly
> without the boilerplate that caused them all to be merged to platform,
> even PCI could be substantially merged like this.
>
> Bonus points to replace the open coded method disptach:
>
> int gpiod_count(struct device *dev, const char *con_id)
> {
>         int count = -ENOENT;
>
>         if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node)
>                 count = of_gpio_get_count(dev, con_id);
>         else if (IS_ENABLED(CONFIG_ACPI) && dev && ACPI_HANDLE(dev))
>                 count = acpi_gpio_count(dev, con_id);
>
>         if (count < 0)
>                 count = platform_gpio_count(dev, con_id);
>
> With an actual bus specific virtual function:
>
>     return dev->bus->gpio_count(dev);
>
> > ...and then do the same thing for every other bus with firmware
> > bindings.  If it's about the firmware interfaces it really isn't a
> > platform bus specific thing.  It's not clear to me if that's what it is
> > though or if this is just some tangent.
>
> It should be split up based on the unique naming scheme and any bus
> specific API elements - like raw access to ACPI or OF data or what
> have you for other FW bus types.

I agree that the pendulum may have swung too far towards "reuse
existing bus_type", and auxiliary-bus unwinds some of that, but does
the bus_type really want to be an indirection for driver apis outside
of bus-specific operations?

Powered by blists - more mailing lists