Re: [RFC PATCH 1/6] bus: Extract simple-bus into self-contained driver

Rob Herring
2025-01-08T14:11:41Z

On Tue, Jan 7, 2025 at 7:28 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Extract the simple bus into a self contained driver so that devices are
> still populated when a node has two (or more) compatibles with the least
> specific one being the generic "simple-bus". Allow the driver to be a
> module so that in a fully modular build a driver module for the more
> specific compatible will be loaded first before trying to match this
> driver.
>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Saravana Kannan <saravanak@google.com>
> Cc: <devicetree@vger.kernel.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: "Uwe Kleine-König" <u.kleine-koenig@baylibre.com>
> Cc: Bjorn Andersson <andersson@kernel.org>
> Cc: Konrad Dybcio <konradybcio@kernel.org>
> Cc: <linux-arm-msm@vger.kernel.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
> drivers/bus/Kconfig | 23 +++++++++++
> drivers/bus/Makefile | 3 ++
> drivers/bus/simple-bus.c | 79 +++++++++++++++++++++++++++++++++++++
> drivers/bus/simple-pm-bus.c | 2 +
> drivers/of/platform.c | 50 +++++++++++++++++++++++
> 5 files changed, 157 insertions(+)
> create mode 100644 drivers/bus/simple-bus.c
>
> diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
> index ff669a8ccad9..7c2aa1350578 100644
> --- a/drivers/bus/Kconfig
> +++ b/drivers/bus/Kconfig
> @@ -261,6 +261,29 @@ config DA8XX_MSTPRI
> configuration. Allows to adjust the priorities of all master
> peripherals.
>
> +config ALLOW_SIMPLE_BUS_OVERRIDE
> + bool "Allow simple-bus compatible OF nodes to match other drivers"
> + depends on OF
> + help
> + Allow nodes with the "simple-bus" compatible to use a more specific
> + driver which populates child devices itself.

A kconfig option for this is not going to work. What does a distro kernel pick?

> +
> +config OF_SIMPLE_BUS
> + tristate "OF Simple Bus Driver"
> + depends on ALLOW_SIMPLE_BUS_OVERRIDE || COMPILE_TEST
> + default ALLOW_SIMPLE_BUS_OVERRIDE
> + help
> + Driver for the "simple-bus" compatible nodes in DeviceTree. Child
> + nodes are usually automatically populated on the platform bus when a
> + node is compatible with "simple-bus". This driver maintains that
> + feature but it fails probe to allow other drivers to try to probe
> + with a more specific compatible if possible.
> +
> + Those other drivers depend on this kconfig symbol so that they match
> + the builtin or modular status of this driver. Don't disable this
> + symbol if ALLOW_SIMPLE_BUS_OVERRIDE is set and there isn't another
> + driver for the simple-bus compatible.

Giving users some rope to hang themselves?

> +
> source "drivers/bus/fsl-mc/Kconfig"
> source "drivers/bus/mhi/Kconfig"
>
> diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile
> index cddd4984d6af..f3968221d704 100644
> --- a/drivers/bus/Makefile
> +++ b/drivers/bus/Makefile
> @@ -40,5 +40,8 @@ obj-$(CONFIG_VEXPRESS_CONFIG) += vexpress-config.o
>
> obj-$(CONFIG_DA8XX_MSTPRI) += da8xx-mstpri.o
>
> +# Must be last for driver registration ordering
> +obj-$(CONFIG_OF_SIMPLE_BUS) += simple-bus.o
> +
> # MHI
> obj-y += mhi/
> diff --git a/drivers/bus/simple-bus.c b/drivers/bus/simple-bus.c
> new file mode 100644
> index 000000000000..3e39b9818566
> --- /dev/null
> +++ b/drivers/bus/simple-bus.c
> @@ -0,0 +1,79 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Simple Bus Driver
> + */
> +
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +
> +static struct platform_driver simple_bus_driver;
> +
> +static int has_specific_simple_bus_drv(struct device_driver *drv, void *dev)
> +{
> + /* Skip if it's this simple bus driver */
> + if (drv == &simple_bus_driver.driver)
> + return 0;
> +
> + if (of_driver_match_device(dev, drv)) {
> + dev_dbg(dev, "Allowing '%s' to probe more specifically\n", drv->name);
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> +static int simple_bus_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + const struct of_dev_auxdata *lookup = dev_get_platdata(dev);
> + struct device_node *np = dev->of_node;
> +
> + /*
> + * If any other driver wants the device, leave the device to the other
> + * driver. Only check drivers that come after this driver so that if an
> + * earlier driver failed to probe we don't populate any devices, and
> + * only check if there's a more specific compatible.
> + */
> + if (of_property_match_string(np, "compatible", "simple-bus") != 0 &&
> + bus_for_each_drv(&platform_bus_type, &simple_bus_driver.driver, dev,
> + has_specific_simple_bus_drv))
> + return -ENODEV;

If this driver is built-in and the specific driver is a module, this
doesn't work, right?

I think we're going to have to have a list of specific compatibles
that have or don't have a specific driver. Today, the list with
specific drivers is short, but that could easily change if this
becomes the way to handle things. We should consider if new platforms
should just avoid 'simple-bus' and use something new.

Rob