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

[Chen-Yu Tsai]

On Thu, Nov 9, 2023 at 11:14 PM Rob Herring <robh+dt@xxxxxxxxxx> wrote:
>
> 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.

I think we can move the majority of this patch into the I2C core and
export just the one prober function. The remainder and the other SKU
ID based prober could be moved to drivers/platform/chrome.

Not sure how we want to place probers for other platforms though?

> >  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.

Ack.

> > +       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.

Ack.

> > +               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().

I wanted to distinguish between explicitly disabled, i.e. components
known to not exist on some specific variant, vs fail or components
that need probing. I suppose this is overengineered since the previous
of_node_name_prefix() already restricts the check to the class of
components we are interested in.

> > +                       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.

I will look into it.

> > +               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().

Ack. However I don't really like the API, as there is no easy way to free
the allocated property if of_changeset_apply fails and we want to clean up.

> > +               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.

OK. How about drivers/platform/chrome/ for the non-generic bits and the other
prober patch?

> > +};
> > +
> > +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.

Ack.

Thanks
ChenYu