Re: [RFC PATCH v2 2/7] of: Introduce hardware prober driver

[Rob Herring]

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