Message-ID: <20240904153746.GA2610341-robh@kernel.org>
Date: Wed, 4 Sep 2024 10:37:46 -0500
From: Rob Herring <robh@...nel.org>
To: Chen-Yu Tsai <wenst@...omium.org>
Cc: Saravana Kannan <saravanak@...gle.com>,
	Matthias Brugger <matthias.bgg@...il.com>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	Wolfram Sang <wsa@...nel.org>, Benson Leung <bleung@...omium.org>,
	Tzung-Bi Shih <tzungbi@...nel.org>, Mark Brown <broonie@...nel.org>,
	Liam Girdwood <lgirdwood@...il.com>,
	chrome-platform@...ts.linux.dev, devicetree@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org,
	Douglas Anderson <dianders@...omium.org>,
	Johan Hovold <johan@...nel.org>, Jiri Kosina <jikos@...nel.org>,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	linux-i2c@...r.kernel.org
Subject: Re: [PATCH v6 08/12] i2c: Introduce OF component probe function

On Wed, Sep 04, 2024 at 05:00:10PM +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 probe. function For a
> 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". 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@...omium.org>
> ---
> Changes since v5:
> - Fixed indent in Makefile
> - Split regulator and GPIO TODO items
> - Reversed final conditional in i2c_of_probe_enable_node()
> 
> Changes since v4:
> - Split code into helper functions
> - Use scoped helpers and __free() to reduce error path
> 
> Changes since v3:
> - Complete kernel-doc
> - Return different error if I2C controller is disabled
> - Expand comment to explain assumptions and constraints
> - Split for-loop finding target node and operations on target node
> - Add missing i2c_put_adapter()
> - Move prober code to separate file
> 
> Rob also asked why there was a limitation of "exactly one touchscreen
> will be enabled across the whole tree".
> 
> The use case this prober currently targets is a component on consumer
> electronics (tablet or laptop) being swapped out due to cost or supply
> reasons. Designs with multiple components of the same type are pretty
> rare. The way the next patch is written also assumes this for efficiency
> reasons.
> 
> Changes since v2:
> - New patch split out from "of: Introduce hardware prober driver"
> - Addressed Rob's comments
>   - Move i2c prober to i2c subsystem
>   - Use of_node_is_available() to check if node is enabled.
>   - Use OF changeset API to update status property
> - Addressed Andy's comments
>   - Probe function now accepts "struct device *dev" instead to reduce
>     line length and dereferences
>   - Move "ret = 0" to just before for_each_child_of_node(i2c_node, node)
> ---
>  drivers/i2c/Makefile             |   1 +
>  drivers/i2c/i2c-core-of-prober.c | 158 +++++++++++++++++++++++++++++++
>  include/linux/i2c.h              |   4 +
>  3 files changed, 163 insertions(+)
>  create mode 100644 drivers/i2c/i2c-core-of-prober.c
> 
> diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
> index f12d6b10a85e..c539cdc1e305 100644
> --- a/drivers/i2c/Makefile
> +++ b/drivers/i2c/Makefile
> @@ -9,6 +9,7 @@ i2c-core-objs			:= i2c-core-base.o i2c-core-smbus.o
>  i2c-core-$(CONFIG_ACPI)		+= i2c-core-acpi.o
>  i2c-core-$(CONFIG_I2C_SLAVE)	+= i2c-core-slave.o
>  i2c-core-$(CONFIG_OF)		+= i2c-core-of.o
> +i2c-core-$(CONFIG_OF_DYNAMIC)	+= i2c-core-of-prober.o
>  
>  obj-$(CONFIG_I2C_SMBUS)		+= i2c-smbus.o
>  obj-$(CONFIG_I2C_CHARDEV)	+= i2c-dev.o
> diff --git a/drivers/i2c/i2c-core-of-prober.c b/drivers/i2c/i2c-core-of-prober.c
> new file mode 100644
> index 000000000000..64d7631f4885
> --- /dev/null
> +++ b/drivers/i2c/i2c-core-of-prober.c
> @@ -0,0 +1,158 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Linux I2C core OF component prober code
> + *
> + * Copyright (C) 2024 Google LLC
> + */
> +
> +#include <linux/cleanup.h>
> +#include <linux/device.h>
> +#include <linux/dev_printk.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/slab.h>
> +
> +/*
> + * Some devices, such as Google Hana Chromebooks, are produced by multiple
> + * vendors each using their preferred components. Such components are all
> + * in the device tree. Instead of having all of them enabled and having each
> + * driver separately try and probe its device while fighting over shared
> + * resources, they can be marked as "fail-needs-probe" and have a prober
> + * figure out which one is actually used beforehand.
> + *
> + * This prober assumes such drop-in parts are on the same I2C bus, have
> + * non-conflicting addresses, and can be directly probed by seeing which
> + * address responds.
> + *
> + * TODO:
> + * - Support handling common regulators.
> + * - Support handling common GPIOs.
> + * - Support I2C muxes
> + */
> +
> +static struct device_node *i2c_of_probe_get_i2c_node(struct device *dev, const char *type)
> +{
> +	struct device_node *node __free(device_node) = of_find_node_by_name(NULL, type);
> +	if (!node)
> +		return dev_err_ptr_probe(dev, -ENODEV, "Could not find %s device node\n", type);
> +
> +	struct device_node *i2c_node __free(device_node) = of_get_parent(node);
> +	if (!of_node_name_eq(i2c_node, "i2c"))
> +		return dev_err_ptr_probe(dev, -EINVAL, "%s device isn't on I2C bus\n", type);
> +
> +	if (!of_device_is_available(i2c_node))
> +		return dev_err_ptr_probe(dev, -ENODEV, "I2C controller not available\n");
> +
> +	return no_free_ptr(i2c_node);
> +}
> +
> +static int i2c_of_probe_enable_node(struct device *dev, struct device_node *node)
> +{
> +	int ret;
> +
> +	dev_info(dev, "Enabling %pOF\n", node);
> +
> +	struct of_changeset *ocs __free(kfree) = kzalloc(sizeof(*ocs), GFP_KERNEL);
> +	if (!ocs)
> +		return -ENOMEM;
> +
> +	of_changeset_init(ocs);
> +	ret = of_changeset_update_prop_string(ocs, node, "status", "okay");
> +	if (ret)
> +		return ret;
> +
> +	ret = of_changeset_apply(ocs);
> +	if (ret) {
> +		/* ocs needs to be explicitly cleaned up before being freed. */
> +		of_changeset_destroy(ocs);
> +	} else {
> +		/*
> +		 * ocs is intentionally kept around as it needs to
> +		 * exist as long as the change is applied.
> +		 */
> +		void *ptr __always_unused = no_free_ptr(ocs);
> +	}
> +
> +	return ret;
> +}
> +
> +/**
> + * i2c_of_probe_component() - probe for devices of "type" on the same i2c bus
> + * @dev: &struct device of the caller, only used for dev_* printk messages
> + * @type: a string to match the device node name prefix to probe for
> + *
> + * Probe for possible I2C components of the same "type" on the same I2C bus
> + * that have their status marked as "fail".
> + *
> + * Assumes that across the entire device tree the only instances of nodes
> + * prefixed with "type" are the ones that need handling for second source
> + * components. In other words, if type is "touchscreen", then all device
> + * nodes named "touchscreen*" are the ones that need probing. There cannot
> + * be another "touchscreen" node that is already enabled.
> + *
> + * Assumes that for each "type" of component, only one actually exists. In
> + * other words, only one matching and existing device will be enabled.
> + *
> + * Context: Process context only. Does non-atomic I2C transfers.
> + *          Should only be used from a driver probe function, as the function
> + *          can return -EPROBE_DEFER if the I2C adapter is unavailable.
> + * Return: 0 on success or no-op, error code otherwise.
> + *         A no-op can happen when it seems like the device tree already
> + *         has components of the type to be probed already enabled. This
> + *         can happen when the device tree had not been updated to mark
> + *         the status of the to-be-probed components as "fail". Or this
> + *         function was already run with the same parameters and succeeded
> + *         in enabling a component. The latter could happen if the user
> + *         had multiple types of components to probe, and one of them down
> + *         the list caused a deferred probe. This is expected behavior.
> + */
> +int i2c_of_probe_component(struct device *dev, const char *type)
> +{
> +	struct i2c_adapter *i2c;
> +	int ret;
> +
> +	struct device_node *i2c_node __free(device_node) = i2c_of_probe_get_i2c_node(dev, type);
> +	if (IS_ERR(i2c_node))
> +		return PTR_ERR(i2c_node);
> +
> +	for_each_child_of_node_scoped(i2c_node, node) {

for_each_available_child_of_node_scoped()


> +		if (!of_node_name_prefix(node, type))
> +			continue;
> +		if (!of_device_is_available(node))
> +			continue;
> +
> +		/*
> +		 * Device tree has component already enabled. Either the
> +		 * device tree isn't supported or we already probed once.
> +		 */
> +		return 0;
> +	}

Powered by blists - more mailing lists