On Thu, Nov 9, 2023 at 4:06 AM Chen-Yu Tsai <wenst@xxxxxxxxxxxx> wrote:
>
> Some devices are designed and manufactured with some components having
> multiple drop-in replacement options. These components are often
> connected to the mainboard via ribbon cables, having the same signals
> and pin assignments across all options. These may include the display
> panel and touchscreen on laptops and tablets, and the trackpad on
> laptops. Sometimes which component option is used in a particular device
> can be detected by some firmware provided identifier, other times that
> information is not available, and the kernel has to try to probe each
> device.
>
> This change attempts to make the "probe each device" case cleaner. The
> current approach is to have all options added and enabled in the device
> tree. The kernel would then bind each device and run each driver's probe
> function. This works, but has been broken before due to the introduction
> of asynchronous probing, causing multiple instances requesting "shared"
> resources, such as pinmuxes, GPIO pins, interrupt lines, at the same
> time, with only one instance succeeding. Work arounds for these include
> moving the pinmux to the parent I2C controller, using GPIO hogs or
> pinmux settings to keep the GPIO pins in some fixed configuration, and
> requesting the interrupt line very late. Such configurations can be seen
> on the MT8183 Krane Chromebook tablets, and the Qualcomm sc8280xp-based
> Lenovo Thinkpad 13S.
>
> Instead of this delicate dance between drivers and device tree quirks,
> this change introduces a simple I2C component prober. For any given
> class of devices on the same I2C bus, it will go through all of them,
> doing a simple I2C read transfer and see which one of them responds.
> It will then enable the device that responds.
>
> This requires some minor modifications in the existing device tree.
> The status for all the device nodes for the component options must be
> set to "failed-needs-probe-xxx". This makes it clear that some mechanism
> is needed to enable one of them, and also prevents the prober and device
> drivers running at the same time.
>
> Signed-off-by: Chen-Yu Tsai <wenst@xxxxxxxxxxxx>
> ---
> drivers/of/Kconfig | 13 ++++
> drivers/of/Makefile | 1 +
> drivers/of/hw_prober.c | 154 +++++++++++++++++++++++++++++++++++++++++
Not sure about having this in drivers/of/, but fine for now... Really,
the I2C bus stuff should be in the I2C core with the rest of the code
that knows how to parse I2C bus nodes.
> 3 files changed, 168 insertions(+)
> create mode 100644 drivers/of/hw_prober.c
>
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index da9826accb1b..269d20d51936 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -102,4 +102,17 @@ config OF_OVERLAY
> config OF_NUMA
> bool
>
> +config HW_PROBER
> + bool "Hardware Prober driver"
> + select I2C
You should not select I2C, but enable/disable I2C functionality based
on it being enabled.
> + select OF_DYNAMIC
> + help
> + Some devices will have multiple drop-in options for one component.
> + In many cases the different options are indistinguishable by the
> + kernel without actually probing each possible option.
> +
> + This driver is meant to handle the probing of such components, and
> + update the running device tree such that the correct variant is
> + made available.
> +
> endif # OF
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index eff624854575..ed3875cdc554 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o
> obj-$(CONFIG_OF_RESOLVE) += resolver.o
> obj-$(CONFIG_OF_OVERLAY) += overlay.o
> obj-$(CONFIG_OF_NUMA) += of_numa.o
> +obj-$(CONFIG_HW_PROBER) += hw_prober.o
>
> ifdef CONFIG_KEXEC_FILE
> ifdef CONFIG_OF_FLATTREE
> diff --git a/drivers/of/hw_prober.c b/drivers/of/hw_prober.c
> new file mode 100644
> index 000000000000..442da6eff896
> --- /dev/null
> +++ b/drivers/of/hw_prober.c
> @@ -0,0 +1,154 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * hw_prober.c - Hardware prober driver
> + *
> + * Copyright (c) 2023 Google LLC
> + */
> +
> +#include <linux/array_size.h>
> +#include <linux/i2c.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +
> +#define DRV_NAME "hw_prober"
> +
> +/**
> + * struct hw_prober_entry - Holds an entry for the hardware prober
> + *
> + * @compatible: compatible string to match against the machine
> + * @prober: prober function to call when machine matches
> + * @data: extra data for the prober function
> + */
> +struct hw_prober_entry {
> + const char *compatible;
> + int (*prober)(struct platform_device *pdev, const void *data);
> + const void *data;
> +};
> +
> +/*
> + * Some devices, such as Google Hana Chromebooks, are produced by multiple
> + * vendors each using their preferred components. This prober assumes such
> + * drop-in parts are on dedicated I2C busses, have non-conflicting addresses,
> + * and can be directly probed by seeing which address responds without needing
> + * regulators or GPIOs being enabled or toggled.
> + */
> +static int i2c_component_prober(struct platform_device *pdev, const void *data)
> +{
> + const char *node_name = data;
> + struct device_node *node, *i2c_node;
> + struct i2c_adapter *i2c;
> + int ret = 0;
> +
> + node = of_find_node_by_name(NULL, node_name);
> + if (!node)
> + return dev_err_probe(&pdev->dev, -ENODEV, "Could not find %s device node\n",
> + node_name);
> +
> + i2c_node = of_get_next_parent(node);
> + if (strcmp(i2c_node->name, "i2c")) {
We have functions for comparing node names, use them and don't access
->name directly.
> + of_node_put(i2c_node);
> + return dev_err_probe(&pdev->dev, -EINVAL, "%s device isn't on I2C bus\n",
> + node_name);
> + }
> +
> + for_each_child_of_node(i2c_node, node) {
> + if (!of_node_name_prefix(node, node_name))
> + continue;
> + if (!of_device_is_fail(node)) {
> + /* device tree has component already enabled */
This isn't quite right if there's a disabled device. To check 'is
enabled', you just need to use of_device_is_available().
> + of_node_put(node);
> + of_node_put(i2c_node);
> + return 0;
> + }
> + }
> +
> + i2c = of_get_i2c_adapter_by_node(i2c_node);
> + if (!i2c) {
> + of_node_put(i2c_node);
> + return dev_err_probe(&pdev->dev, -EPROBE_DEFER, "Couldn't get I2C adapter\n");
> + }
> +
> + for_each_child_of_node(i2c_node, node) {
The I2C core will walk the devices too. Perhaps if that saves off a
list of failed devices, then we don't need to walk the nodes again.
> + struct property *prop;
> + union i2c_smbus_data data;
> + u32 addr;
> +
> + if (!of_node_name_prefix(node, node_name))
> + continue;
> + if (of_property_read_u32(node, "reg", &addr))
> + continue;
> + if (i2c_smbus_xfer(i2c, addr, 0, I2C_SMBUS_READ, 0, I2C_SMBUS_BYTE, &data) < 0)
> + continue;
> +
> + dev_info(&pdev->dev, "Enabling %pOF\n", node);
> +
> + prop = kzalloc(sizeof(*prop), GFP_KERNEL);
> + if (!prop) {
> + ret = -ENOMEM;
> + of_node_put(node);
> + break;
> + }
> +
> + prop->name = "status";
> + prop->length = 5;
> + prop->value = "okay";
> +
> + /* Found a device that is responding */
> + ret = of_update_property(node, prop);
Use the changeset API instead and make an update flavor of
of_changeset_add_prop_string().
> + if (ret)
> + kfree(prop);
> +
> + of_node_put(node);
> + break;
> + }
> +
> + i2c_put_adapter(i2c);
> + of_node_put(i2c_node);
> +
> + return ret;
> +}
> +
> +static const struct hw_prober_entry hw_prober_platforms[] = {
> + { .compatible = "google,hana", .prober = i2c_component_prober, .data = "touchscreen" },
> + { .compatible = "google,hana", .prober = i2c_component_prober, .data = "trackpad" },
Not generic code. Needs to be somewhere else.
> +};
> +
> +static int hw_prober_probe(struct platform_device *pdev)
> +{
> + for (int i = 0; i < ARRAY_SIZE(hw_prober_platforms); i++)
> + if (of_machine_is_compatible(hw_prober_platforms[i].compatible)) {
> + int ret;
> +
> + ret = hw_prober_platforms[i].prober(pdev, hw_prober_platforms[i].data);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static struct platform_driver hw_prober_driver = {
> + .probe = hw_prober_probe,
> + .driver = {
> + .name = DRV_NAME,
> + },
> +};
> +
> +static int __init hw_prober_driver_init(void)
> +{
> + struct platform_device *pdev;
> + int ret;
> +
> + ret = platform_driver_register(&hw_prober_driver);
> + if (ret)
> + return ret;
> +
> + pdev = platform_device_register_simple(DRV_NAME, -1, NULL, 0);
This should be dependent on platforms that need it, not everyone. IOW,
this is where checking for "google,hana" belongs.
Rob
