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

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@chromium.org>
---
 drivers/of/Kconfig     |  13 ++++
 drivers/of/Makefile    |   1 +
 drivers/of/hw_prober.c | 154 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 168 insertions(+)
 create mode 100644 drivers/of/hw_prober.c

On Thu, Nov 9, 2023 at 4:06 AM Chen-Yu Tsai <wenst@chromium.org> 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@chromium.org>
> ---
>  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

On Thu, Nov 09, 2023 at 06:05:59PM +0800, Chen-Yu Tsai 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.

...

> +config HW_PROBER

config OF_HW_PROBER // or anything with explicit OF

Don't give a false impression that this is something that may works without
OF support.

...

> +	bool "Hardware Prober driver"

Ditto.

...

> +/*
> + * hw_prober.c - Hardware prober driver

Do not include filename into the file itself.

> + *
> + * Copyright (c) 2023 Google LLC
> + */

...

> +	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);

With

	struct device *dev = &pdev->dev;

this and other lines can be made neater.

...


For better maintenance it's good to have ret assignment be placed here

	ret = 0;

> +	for_each_child_of_node(i2c_node, node) {
> +		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);
> +		if (ret)
> +			kfree(prop);
> +
> +		of_node_put(node);
> +		break;
> +	}

...

> +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" },
> +};

Why can't OF ID table be used for this?

...

> +	for (int i = 0; i < ARRAY_SIZE(hw_prober_platforms); i++)

unsigned?

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

...

> +	pdev = platform_device_register_simple(DRV_NAME, -1, NULL, 0);

-1 is defined in the header, use that definition.

> +	if (!IS_ERR(pdev))
> +		return 0;
> +
> +	platform_driver_unregister(&hw_prober_driver);
> +
> +	return PTR_ERR(pdev);

Can you use standard pattern, i.e. checking for the _error_ condition?

On Fri, Nov 10, 2023 at 1:54 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Nov 09, 2023 at 06:05:59PM +0800, Chen-Yu Tsai 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.
>
> ...
>
> > +config HW_PROBER
>
> config OF_HW_PROBER // or anything with explicit OF
>
> Don't give a false impression that this is something that may works without
> OF support.

Ack.

> ...
>
> > +     bool "Hardware Prober driver"
>
> Ditto.

Ack.

> ...
>
> > +/*
> > + * hw_prober.c - Hardware prober driver
>
> Do not include filename into the file itself.

Ack.

> > + *
> > + * Copyright (c) 2023 Google LLC
> > + */
>
> ...
>
> > +     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);
>
> With
>
>         struct device *dev = &pdev->dev;
>
> this and other lines can be made neater.

Ack.

> ...
>
>
> For better maintenance it's good to have ret assignment be placed here
>
>         ret = 0;

Ack.

> > +     for_each_child_of_node(i2c_node, node) {
> > +             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);
> > +             if (ret)
> > +                     kfree(prop);
> > +
> > +             of_node_put(node);
> > +             break;
> > +     }
>
> ...
>
> > +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" },
> > +};
>
> Why can't OF ID table be used for this?

My intent was to have this accept a probe function, which may take an extra
data argument. So either a new structure like the one here, or use OF ID table,
and then another layer with a struct holding the prober and extra data pointer.

I'm guessing this will change since Rob thinks the next patch that adds a
different prober doesn't belong here.

> ...
>
> > +     for (int i = 0; i < ARRAY_SIZE(hw_prober_platforms); i++)
>
> unsigned?

Ack.

> > +             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;
> > +             }
>
> ...
>
> > +     pdev = platform_device_register_simple(DRV_NAME, -1, NULL, 0);
>
> -1 is defined in the header, use that definition.

Ack.

> > +     if (!IS_ERR(pdev))
> > +             return 0;
> > +
> > +     platform_driver_unregister(&hw_prober_driver);
> > +
> > +     return PTR_ERR(pdev);
>
> Can you use standard pattern, i.e. checking for the _error_ condition?

Ack.


Thanks
ChenYu

On Thu, Nov 9, 2023 at 11:14 PM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Thu, Nov 9, 2023 at 4:06 AM Chen-Yu Tsai <wenst@chromium.org> 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@chromium.org>
> > ---
> >  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