[PATCH v6 00/12] platform/chrome: Introduce DT hardware prober

[PATCH v6 00/12] platform/chrome: Introduce DT hardware prober

[Chen-Yu Tsai]

Hi everyone,

This is v6 of my "of: Introduce hardware prober driver" [1] series.
v6 mainly addresses comments from Andy.

v2 continued Doug's "of: device: Support 2nd sources of probeable but
undiscoverable devices" [2] series, but follows the scheme suggested by
Rob, marking all second source component device nodes as "fail-needs-probe",
and having a hardware prober driver enable the one of them.


Changes since v5:
- Link to v5:
  https://lore.kernel.org/all/20240822092006.3134096-1-wenst@chromium.org/
- Patch 1 "of: dynamic: Add of_changeset_update_prop_string"
  - Collected Rob's reviewed-by
- Patch 2 "of: base: Add for_each_child_of_node_with_prefix_scoped()"
  - New patch
- Patch 3 "regulator: Move OF-specific regulator lookup code to of_regulator.c"
  - Fix kerneldoc format of of_regulator_dev_lookup()
  - Fix stub compile error for !CONFIG_OF in drivers/regulator/internal.h
- Patch 4 "regulator: Split up _regulator_get()"
  - Fixed kerneldoc "Return" section format for _regulator_get_common()
  - Slightly reworded return value description
- Patch 5 "regulator: Do pure DT regulator lookup in of_regulator_bulk_get_all()"
  - Used "dev_of_node(dev)" instead of "dev->of_node"
  - Replaced "dev_printk" with "dev_printk()" in kerneldoc mentions
  - Fixed kerneldoc "Return" section format for of_regulator_get_optional()
  - Fix @np parameter name in of_regulator_dev_lookup() kerneldoc
- Patch 6 "gpiolib: Add gpio_property_name_length()"
  - Changed function name to "gpio_get_property_name_length()"
  - Changed argument name to "propname"
  - Clarified return value for "*-<GPIO suffix>" case
  - Reworked according to Andy's suggestion
  - Added stub function
- Patch 7 "i2c: core: Remove extra space in Makefile"
  - New patch
- Patch 8 "i2c: Introduce OF component probe function"
  - Fixed indent in Makefile
  - Split regulator and GPIO TODO items
  - Reversed final conditional in i2c_of_probe_enable_node()
- Patch 9 "i2c: of-prober: Add regulator support"
  - Split of_regulator_bulk_get_all() return value check and explain
    "ret == 0" case
  - Switched to of_get_next_child_with_prefix_scoped() where applicable
  - Used krealloc_array() instead of directly calculating size
  - copy whole regulator array in one memcpy() call
  - Drop "0" from struct zeroing initializer
  - Split out regulator helper from i2c_of_probe_enable_res() to keep
    code cleaner when combined with the next patch
  - Added options for customizing power sequencing delay
  - Rename i2c_of_probe_get_regulator() to i2c_of_probe_get_regulators()
  - Add i2c_of_probe_free_regulator() helper
- Patch 10 "i2c: of-prober: Add GPIO support"
  - Renamed "con" to "propname" in i2c_of_probe_get_gpiod()
  - Copy string first and check return value of strscpy() for overflow in
    i2c_of_probe_get_gpiod()
  - Add parenthesis around "enable" and "reset" GPIO names in comments
  - Split resource count debug message into two separate lines
  - Split out GPIO helper from i2c_of_probe_enable_res() to keep code
    cleaner following the previous patch
  - Adopted options for customizing power sequencing delay following
    previous patch
- Patch 11 "platform/chrome: Introduce device tree hardware prober"
  - Adapt to new i2c_of_probe_component() parameters
- Patch 12 "arm64: dts: mediatek: mt8173-elm-hana: Mark touchscreens and
	    trackpads as fail"
  - None

See v5 cover letter for previous change logs.

For the I2C component (touchscreens and trackpads) case from the
original series, the hardware prober driver finds the particular
class of device in the device tree, gets its parent I2C adapter,
and tries to initiate a simple I2C read for each device under that
I2C bus. When it finds one that responds, it considers that one
present, marks it as "okay", and returns, letting the driver core
actually probe the device.

This works fine in most cases since these components are connected
via a ribbon cable and always have the same resources. The prober
will also grab these resources and enable them.

The other case, selecting a display panel to use based on the SKU ID
from the firmware, hit a bit of an issue with fixing the OF graph.
It has been left out since v3.

Patch 1 adds of_changeset_update_prop_string(), as requested by Rob.

Patch 2 adds for_each_child_of_node_with_prefix_scoped(), as suggested
by Andy.

Patches 3 through 5 reorganize the OF-specific regulator core code and
reworks the existing of_regulator_bulk_get_all() function to look up
regulator supplies solely using device tree nodes.

Patch 6 adds a function to the GPIO library that checks whether a
given string (property name) matches the GPIO property pattern, and
if it does, returns the length of the GPIO name.

Patch 7 cleans up some extra spaces in the i2c core Makefile

Patch 8 implements probing the I2C bus for presence of components as
a helper function in the I2C core.

Patch 9 implements regulator supply support for the I2C component
prober.

Patch 10 implements GPIO support for the I2C component prober.

Patch 11 adds a ChromeOS specific DT hardware prober. This initial
version targets the Hana Chromebooks, probing its I2C trackpads and
touchscreens.

Patch 12 modifies the Hana device tree and marks the touchscreens
and trackpads as "fail-needs-probe", ready for the driver to probe.


The patch and build time dependencies for this series is now quite
complicated:

  of_regulator.c cleanups [1] -> regulator patches here ----
							   |
							   v
  gpio patch in -next [2] -> gpiolib patch here  -----> i2c of-prober --
								       |
                      platform/chrome device tree hardware prober <----- 
 
The regulator patches in this series depend on other cleanup patches [1]
I sent earlier. The gpiolib patch depends on a commit in -next [2]
changing the GPIO suffixes array. Patches 7 through 10 introducting i2c
of-prober depend on the first 5 patches. Patch 11, The chrome prober,
depends on patches 7 through 10.

I think it might be easier if the respective maintainers take the first
six patches for -rc1. Patches 7 through 11 can go through either the i2c
or chrome trees either very late in the merge window or right after it.
Patch 12 can go in only after everything else is in. This should be
better than having an immutable branch on top of some commit in -next
for three other trees to consume.

Wolfram, would you be able to handle the late PR? Assuming you get a
chance to look at the patches that is.


Thanks
ChenYu

[1] https://lore.kernel.org/all/20240822072047.3097740-1-wenst@chromium.org/
[2] commit 4b91188dced8 ("gpiolib: Replace gpio_suffix_count with NULL-terminated array")

Chen-Yu Tsai (12):
  of: dynamic: Add of_changeset_update_prop_string
  of: base: Add for_each_child_of_node_with_prefix_scoped()
  regulator: Move OF-specific regulator lookup code to of_regulator.c
  regulator: Split up _regulator_get()
  regulator: Do pure DT regulator lookup in of_regulator_bulk_get_all()
  gpiolib: Add gpio_get_property_name_length()
  i2c: core: Remove extra space in Makefile
  i2c: Introduce OF component probe function
  i2c: of-prober: Add regulator support
  i2c: of-prober: Add GPIO support
  platform/chrome: Introduce device tree hardware prober
  arm64: dts: mediatek: mt8173-elm-hana: Mark touchscreens and trackpads
    as fail

 .../boot/dts/mediatek/mt8173-elm-hana.dtsi    |  13 +
 arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi  |   4 +-
 drivers/gpio/gpiolib.c                        |  25 +
 drivers/i2c/Makefile                          |   7 +-
 drivers/i2c/i2c-core-of-prober.c              | 437 ++++++++++++++++++
 drivers/of/base.c                             |  35 ++
 drivers/of/dynamic.c                          |  44 ++
 drivers/platform/chrome/Kconfig               |  11 +
 drivers/platform/chrome/Makefile              |   1 +
 .../platform/chrome/chromeos_of_hw_prober.c   | 104 +++++
 drivers/regulator/core.c                      | 143 ++----
 drivers/regulator/internal.h                  |  15 +-
 drivers/regulator/of_regulator.c              | 150 +++++-
 include/linux/gpio/consumer.h                 |   7 +
 include/linux/i2c.h                           |  14 +
 include/linux/of.h                            |  13 +
 16 files changed, 918 insertions(+), 105 deletions(-)
 create mode 100644 drivers/i2c/i2c-core-of-prober.c
 create mode 100644 drivers/platform/chrome/chromeos_of_hw_prober.c

-- 
2.46.0.469.g59c65b2a67-goog

[PATCH v6 01/12] of: dynamic: Add of_changeset_update_prop_string

[Chen-Yu Tsai]

Add a helper function to add string property updates to an OF changeset.
This is similar to of_changeset_add_prop_string(), but instead of adding
the property (and failing if it exists), it will update the property.

This shall be used later in the DT hardware prober.

Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
---
Changes since v5:
- Collected Rob's tag

Changes since v4:
- Use modern designated initializer for |prop|

Changes since v3:
- Use new __of_prop_free() helper
- Add new line before header declaration

Changes since v2:
- New patch added in v3
---
 drivers/of/dynamic.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of.h   |  4 ++++
 2 files changed, 48 insertions(+)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 110104a936d9..daa69d160a78 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -1072,3 +1072,47 @@ int of_changeset_add_prop_bool(struct of_changeset *ocs, struct device_node *np,
 	return of_changeset_add_prop_helper(ocs, np, &prop);
 }
 EXPORT_SYMBOL_GPL(of_changeset_add_prop_bool);
+
+static int of_changeset_update_prop_helper(struct of_changeset *ocs,
+					   struct device_node *np,
+					   const struct property *pp)
+{
+	struct property *new_pp;
+	int ret;
+
+	new_pp = __of_prop_dup(pp, GFP_KERNEL);
+	if (!new_pp)
+		return -ENOMEM;
+
+	ret = of_changeset_update_property(ocs, np, new_pp);
+	if (ret)
+		__of_prop_free(new_pp);
+
+	return ret;
+}
+
+/**
+ * of_changeset_update_prop_string - Add a string property update to a changeset
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @prop_name:	name of the property to be updated
+ * @str:	pointer to null terminated string
+ *
+ * Create a string property to be updated and add it to a changeset.
+ *
+ * Return: 0 on success, a negative error value in case of an error.
+ */
+int of_changeset_update_prop_string(struct of_changeset *ocs,
+				    struct device_node *np,
+				    const char *prop_name, const char *str)
+{
+	struct property prop = {
+		.name = (char *)prop_name,
+		.length = strlen(str) + 1,
+		.value = (void *)str,
+	};
+
+	return of_changeset_update_prop_helper(ocs, np, &prop);
+}
+EXPORT_SYMBOL_GPL(of_changeset_update_prop_string);
diff --git a/include/linux/of.h b/include/linux/of.h
index 85b60ac9eec5..046283be1cd3 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1651,6 +1651,10 @@ static inline int of_changeset_add_prop_u32(struct of_changeset *ocs,
 	return of_changeset_add_prop_u32_array(ocs, np, prop_name, &val, 1);
 }
 
+int of_changeset_update_prop_string(struct of_changeset *ocs,
+				    struct device_node *np,
+				    const char *prop_name, const char *str);
+
 int of_changeset_add_prop_bool(struct of_changeset *ocs, struct device_node *np,
 			       const char *prop_name);
 
-- 
2.46.0.469.g59c65b2a67-goog

[PATCH v6 02/12] of: base: Add for_each_child_of_node_with_prefix_scoped()

[Chen-Yu Tsai]

There are cases where drivers would go through child device nodes and
operate on only the ones whose node name starts with a given prefix.

Provide a helper for these users. This will mainly be used in a
subsequent patch that implements a hardware component prober for I2C
busses.

Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
Changes since v5:
- New patch
---
 drivers/of/base.c  | 35 +++++++++++++++++++++++++++++++++++
 include/linux/of.h |  9 +++++++++
 2 files changed, 44 insertions(+)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 20603d3c9931..d3c123b3261a 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -628,6 +628,41 @@ struct device_node *of_get_next_child(const struct device_node *node,
 }
 EXPORT_SYMBOL(of_get_next_child);
 
+/**
+ * of_get_next_child_with_prefix - Find the next child node with prefix
+ * @node:	parent node
+ * @prev:	previous child of the parent node, or NULL to get first
+ *
+ * This function is like of_get_next_child(), except that it automatically
+ * skips any nodes whose name doesn't have the given prefix.
+ *
+ * Return: A node pointer with refcount incremented, use
+ * of_node_put() on it when done.
+ */
+struct device_node *of_get_next_child_with_prefix(const struct device_node *node,
+						  struct device_node *prev,
+						  const char *prefix)
+{
+	struct device_node *next;
+	unsigned long flags;
+
+	if (!node)
+		return NULL;
+
+	raw_spin_lock_irqsave(&devtree_lock, flags);
+	next = prev ? prev->sibling : node->child;
+	for (; next; next = next->sibling) {
+		if (!of_node_name_prefix(next, prefix))
+			continue;
+		if (of_node_get(next))
+			break;
+	}
+	of_node_put(prev);
+	raw_spin_unlock_irqrestore(&devtree_lock, flags);
+	return next;
+}
+EXPORT_SYMBOL(of_get_next_child_with_prefix);
+
 static struct device_node *of_get_next_status_child(const struct device_node *node,
 						    struct device_node *prev,
 						    bool (*checker)(const struct device_node *))
diff --git a/include/linux/of.h b/include/linux/of.h
index 046283be1cd3..acc0d5b98417 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -289,6 +289,9 @@ extern struct device_node *of_get_parent(const struct device_node *node);
 extern struct device_node *of_get_next_parent(struct device_node *node);
 extern struct device_node *of_get_next_child(const struct device_node *node,
 					     struct device_node *prev);
+extern struct device_node *of_get_next_child_with_prefix(const struct device_node *node,
+							 struct device_node *prev,
+							 const char *prefix);
 extern struct device_node *of_get_next_available_child(
 	const struct device_node *node, struct device_node *prev);
 extern struct device_node *of_get_next_reserved_child(
@@ -1468,6 +1471,12 @@ static inline int of_property_read_s32(const struct device_node *np,
 	     child != NULL;						\
 	     child = of_get_next_child(parent, child))
 
+#define for_each_child_of_node_with_prefix_scoped(parent, child, prefix) \
+	for (struct device_node *child __free(device_node) =		\
+	     of_get_next_child_with_prefix(parent, NULL, prefix);	\
+	     child != NULL;						\
+	     child = of_get_next_child_with_prefix(parent, child, prefix))
+
 #define for_each_available_child_of_node(parent, child) \
 	for (child = of_get_next_available_child(parent, NULL); child != NULL; \
 	     child = of_get_next_available_child(parent, child))
-- 
2.46.0.469.g59c65b2a67-goog

[PATCH v6 03/12] regulator: Move OF-specific regulator lookup code to of_regulator.c

[Chen-Yu Tsai]

There's still a bit of OF-specific code in the regulator device lookup
function.

Move those bits of code over to of_regulator.c, and create a new
function of_regulator_dev_lookup() to encapsulate the code moved out of
regulator_dev_lookup().

Also mark of_find_regulator_by_node() as static, since there are no
other users in other compile units.

There are no functional changes. A line alignment was also fixed.

Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
Changes since v5:
- Fix kerneldoc format of of_regulator_dev_lookup()
- Fix stub compile error for !CONFIG_OF in drivers/regulator/internal.h

Changes since v4:
- New patch
---
 drivers/regulator/core.c         |  87 ++----------------------
 drivers/regulator/internal.h     |   9 +--
 drivers/regulator/of_regulator.c | 110 ++++++++++++++++++++++++++++++-
 3 files changed, 119 insertions(+), 87 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index c1d11924d892..835a5531d045 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -421,74 +421,6 @@ static void regulator_lock_dependent(struct regulator_dev *rdev,
 	mutex_unlock(&regulator_list_mutex);
 }
 
-/**
- * of_get_child_regulator - get a child regulator device node
- * based on supply name
- * @parent: Parent device node
- * @prop_name: Combination regulator supply name and "-supply"
- *
- * Traverse all child nodes.
- * Extract the child regulator device node corresponding to the supply name.
- *
- * Return: Pointer to the &struct device_node corresponding to the regulator
- *	   if found, or %NULL if not found.
- */
-static struct device_node *of_get_child_regulator(struct device_node *parent,
-						  const char *prop_name)
-{
-	struct device_node *regnode = NULL;
-	struct device_node *child = NULL;
-
-	for_each_child_of_node(parent, child) {
-		regnode = of_parse_phandle(child, prop_name, 0);
-
-		if (!regnode) {
-			regnode = of_get_child_regulator(child, prop_name);
-			if (regnode)
-				goto err_node_put;
-		} else {
-			goto err_node_put;
-		}
-	}
-	return NULL;
-
-err_node_put:
-	of_node_put(child);
-	return regnode;
-}
-
-/**
- * of_get_regulator - get a regulator device node based on supply name
- * @dev: Device pointer for the consumer (of regulator) device
- * @supply: regulator supply name
- *
- * Extract the regulator device node corresponding to the supply name.
- *
- * Return: Pointer to the &struct device_node corresponding to the regulator
- *	   if found, or %NULL if not found.
- */
-static struct device_node *of_get_regulator(struct device *dev, const char *supply)
-{
-	struct device_node *regnode = NULL;
-	char prop_name[64]; /* 64 is max size of property name */
-
-	dev_dbg(dev, "Looking up %s-supply from device tree\n", supply);
-
-	snprintf(prop_name, 64, "%s-supply", supply);
-	regnode = of_parse_phandle(dev->of_node, prop_name, 0);
-
-	if (!regnode) {
-		regnode = of_get_child_regulator(dev->of_node, prop_name);
-		if (regnode)
-			return regnode;
-
-		dev_dbg(dev, "Looking up %s property in node %pOF failed\n",
-				prop_name, dev->of_node);
-		return NULL;
-	}
-	return regnode;
-}
-
 /* Platform voltage constraint check */
 int regulator_check_voltage(struct regulator_dev *rdev,
 			    int *min_uV, int *max_uV)
@@ -2021,7 +1953,6 @@ static struct regulator_dev *regulator_dev_lookup(struct device *dev,
 						  const char *supply)
 {
 	struct regulator_dev *r = NULL;
-	struct device_node *node;
 	struct regulator_map *map;
 	const char *devname = NULL;
 
@@ -2029,19 +1960,11 @@ static struct regulator_dev *regulator_dev_lookup(struct device *dev,
 
 	/* first do a dt based lookup */
 	if (dev && dev->of_node) {
-		node = of_get_regulator(dev, supply);
-		if (node) {
-			r = of_find_regulator_by_node(node);
-			of_node_put(node);
-			if (r)
-				return r;
-
-			/*
-			 * We have a node, but there is no device.
-			 * assume it has not registered yet.
-			 */
-			return ERR_PTR(-EPROBE_DEFER);
-		}
+		r = of_regulator_dev_lookup(dev, supply);
+		if (!IS_ERR(r))
+			return r;
+		if (PTR_ERR(r) == -EPROBE_DEFER)
+			return r;
 	}
 
 	/* if not found, try doing it non-dt way */
diff --git a/drivers/regulator/internal.h b/drivers/regulator/internal.h
index 77a502141089..8e5506c5ee94 100644
--- a/drivers/regulator/internal.h
+++ b/drivers/regulator/internal.h
@@ -66,7 +66,8 @@ static inline struct regulator_dev *dev_to_rdev(struct device *dev)
 }
 
 #ifdef CONFIG_OF
-struct regulator_dev *of_find_regulator_by_node(struct device_node *np);
+struct regulator_dev *of_regulator_dev_lookup(struct device *dev,
+					      const char *supply);
 struct regulator_init_data *regulator_of_get_init_data(struct device *dev,
 			         const struct regulator_desc *desc,
 				 struct regulator_config *config,
@@ -80,10 +81,10 @@ int of_get_n_coupled(struct regulator_dev *rdev);
 bool of_check_coupling_data(struct regulator_dev *rdev);
 
 #else
-static inline struct regulator_dev *
-of_find_regulator_by_node(struct device_node *np)
+static inline struct regulator_dev *of_regulator_dev_lookup(struct device *dev,
+							    const char *supply)
 {
-	return NULL;
+	return ERR_PTR(-ENODEV);
 }
 
 static inline struct regulator_init_data *
diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index cec8c3647a00..d5dd7a9e577b 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -552,7 +552,75 @@ struct regulator_init_data *regulator_of_get_init_data(struct device *dev,
 	return NULL;
 }
 
-struct regulator_dev *of_find_regulator_by_node(struct device_node *np)
+/**
+ * of_get_child_regulator - get a child regulator device node
+ * based on supply name
+ * @parent: Parent device node
+ * @prop_name: Combination regulator supply name and "-supply"
+ *
+ * Traverse all child nodes.
+ * Extract the child regulator device node corresponding to the supply name.
+ *
+ * Return: Pointer to the &struct device_node corresponding to the regulator
+ *	   if found, or %NULL if not found.
+ */
+static struct device_node *of_get_child_regulator(struct device_node *parent,
+						  const char *prop_name)
+{
+	struct device_node *regnode = NULL;
+	struct device_node *child = NULL;
+
+	for_each_child_of_node(parent, child) {
+		regnode = of_parse_phandle(child, prop_name, 0);
+
+		if (!regnode) {
+			regnode = of_get_child_regulator(child, prop_name);
+			if (regnode)
+				goto err_node_put;
+		} else {
+			goto err_node_put;
+		}
+	}
+	return NULL;
+
+err_node_put:
+	of_node_put(child);
+	return regnode;
+}
+
+/**
+ * of_get_regulator - get a regulator device node based on supply name
+ * @dev: Device pointer for the consumer (of regulator) device
+ * @supply: regulator supply name
+ *
+ * Extract the regulator device node corresponding to the supply name.
+ *
+ * Return: Pointer to the &struct device_node corresponding to the regulator
+ *	   if found, or %NULL if not found.
+ */
+static struct device_node *of_get_regulator(struct device *dev, const char *supply)
+{
+	struct device_node *regnode = NULL;
+	char prop_name[64]; /* 64 is max size of property name */
+
+	dev_dbg(dev, "Looking up %s-supply from device tree\n", supply);
+
+	snprintf(prop_name, 64, "%s-supply", supply);
+	regnode = of_parse_phandle(dev->of_node, prop_name, 0);
+
+	if (!regnode) {
+		regnode = of_get_child_regulator(dev->of_node, prop_name);
+		if (regnode)
+			return regnode;
+
+		dev_dbg(dev, "Looking up %s property in node %pOF failed\n",
+			prop_name, dev->of_node);
+		return NULL;
+	}
+	return regnode;
+}
+
+static struct regulator_dev *of_find_regulator_by_node(struct device_node *np)
 {
 	struct device *dev;
 
@@ -561,6 +629,46 @@ struct regulator_dev *of_find_regulator_by_node(struct device_node *np)
 	return dev ? dev_to_rdev(dev) : NULL;
 }
 
+/**
+ * of_regulator_dev_lookup - lookup a regulator device with device tree only
+ * @dev: Device pointer for regulator supply lookup.
+ * @supply: Supply name or regulator ID.
+ *
+ * Return: Pointer to the &struct regulator_dev on success, or ERR_PTR()
+ *	   encoded value on error.
+ *
+ * If successful, returns a pointer to the &struct regulator_dev that
+ * corresponds to the name @supply and with the embedded &struct device
+ * refcount incremented by one. The refcount must be dropped by calling
+ * put_device().
+ *
+ * On failure one of the following ERR_PTR() encoded values is returned:
+ * * -%ENODEV if lookup fails permanently.
+ * * -%EPROBE_DEFER if lookup could succeed in the future.
+ */
+struct regulator_dev *of_regulator_dev_lookup(struct device *dev,
+					      const char *supply)
+{
+	struct regulator_dev *r;
+	struct device_node *node;
+
+	node = of_get_regulator(dev, supply);
+	if (node) {
+		r = of_find_regulator_by_node(node);
+		of_node_put(node);
+		if (r)
+			return r;
+
+		/*
+		 * We have a node, but there is no device.
+		 * assume it has not registered yet.
+		 */
+		return ERR_PTR(-EPROBE_DEFER);
+	}
+
+	return ERR_PTR(-ENODEV);
+}
+
 /*
  * Returns number of regulators coupled with rdev.
  */
-- 
2.46.0.469.g59c65b2a67-goog

[PATCH v6 04/12] regulator: Split up _regulator_get()

[Chen-Yu Tsai]

_regulator_get() contains a lot of common code doing checks prior to the
regulator lookup and housekeeping work after the lookup. Almost all the
code could be shared with a OF-specific variant of _regulator_get().

Split out the common parts so that they can be reused. The OF-specific
version of _regulator_get() will be added in a subsequent patch.
No functional changes were made.

Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
Changes since v5:
- Fixed kerneldoc "Return" section format for _regulator_get_common()
- Slightly reworded return value description

Changes since v4:
- New patch
---
 drivers/regulator/core.c     | 54 ++++++++++++++++++++++++++++--------
 drivers/regulator/internal.h |  4 +++
 2 files changed, 47 insertions(+), 11 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 835a5531d045..d60c86477ac2 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2103,26 +2103,43 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
 	return ret;
 }
 
-/* Internal regulator request function */
-struct regulator *_regulator_get(struct device *dev, const char *id,
-				 enum regulator_get_type get_type)
+/* common pre-checks for regulator requests */
+int _regulator_get_common_check(struct device *dev, const char *id,
+				enum regulator_get_type get_type)
 {
-	struct regulator_dev *rdev;
-	struct regulator *regulator;
-	struct device_link *link;
-	int ret;
-
 	if (get_type >= MAX_GET_TYPE) {
 		dev_err(dev, "invalid type %d in %s\n", get_type, __func__);
-		return ERR_PTR(-EINVAL);
+		return -EINVAL;
 	}
 
 	if (id == NULL) {
 		dev_err(dev, "regulator request with no identifier\n");
-		return ERR_PTR(-EINVAL);
+		return -EINVAL;
 	}
 
-	rdev = regulator_dev_lookup(dev, id);
+	return 0;
+}
+
+/**
+ * _regulator_get_common - Common code for regulator requests
+ * @rdev: regulator device pointer as returned by *regulator_dev_lookup()
+ *       Its reference count is expected to have been incremented.
+ * @dev: device used for dev_printk messages
+ * @id: Supply name or regulator ID
+ * @get_type: enum regulator_get_type value corresponding to type of request
+ *
+ * Returns: pointer to struct regulator corresponding to @rdev, or ERR_PTR()
+ *	    encoded error.
+ *
+ * This function should be chained with *regulator_dev_lookup() functions.
+ */
+struct regulator *_regulator_get_common(struct regulator_dev *rdev, struct device *dev,
+					const char *id, enum regulator_get_type get_type)
+{
+	struct regulator *regulator;
+	struct device_link *link;
+	int ret;
+
 	if (IS_ERR(rdev)) {
 		ret = PTR_ERR(rdev);
 
@@ -2238,6 +2255,21 @@ struct regulator *_regulator_get(struct device *dev, const char *id,
 	return regulator;
 }
 
+/* Internal regulator request function */
+struct regulator *_regulator_get(struct device *dev, const char *id,
+				 enum regulator_get_type get_type)
+{
+	struct regulator_dev *rdev;
+	int ret;
+
+	ret = _regulator_get_common_check(dev, id, get_type);
+	if (ret)
+		return ERR_PTR(ret);
+
+	rdev = regulator_dev_lookup(dev, id);
+	return _regulator_get_common(rdev, dev, id, get_type);
+}
+
 /**
  * regulator_get - lookup and obtain a reference to a regulator.
  * @dev: device for regulator "consumer"
diff --git a/drivers/regulator/internal.h b/drivers/regulator/internal.h
index 8e5506c5ee94..5b43f802468d 100644
--- a/drivers/regulator/internal.h
+++ b/drivers/regulator/internal.h
@@ -121,6 +121,10 @@ enum regulator_get_type {
 	MAX_GET_TYPE
 };
 
+int _regulator_get_common_check(struct device *dev, const char *id,
+				enum regulator_get_type get_type);
+struct regulator *_regulator_get_common(struct regulator_dev *rdev, struct device *dev,
+					const char *id, enum regulator_get_type get_type);
 struct regulator *_regulator_get(struct device *dev, const char *id,
 				 enum regulator_get_type get_type);
 int _regulator_bulk_get(struct device *dev, int num_consumers,
-- 
2.46.0.469.g59c65b2a67-goog

[PATCH v6 05/12] regulator: Do pure DT regulator lookup in of_regulator_bulk_get_all()

[Chen-Yu Tsai]

The to-be-introduced I2C component prober needs to enable regulator
supplies (and toggle GPIO pins) for the various components it intends
to probe. To support this, a new "pure DT lookup" method for getting
regulator supplies is needed, since the device normally requesting
the supply won't get created until after the component is probed to
be available.

Convert the existing of_regulator_bulk_get_all() for this purpose.
This function has no in-tree users, as the original patch [1] that
used it was never landed. This patch changes the function ABI, but
it is straightforward to convert users.

The underlying code that supports the existing regulator_get*()
functions has been reworked in previous patches to support this
specific case. An internal OF-specific version of regulator_get(),
of_regulator_get_optional(), is added for this.

Also convert an existing usage of "dev && dev->of_node" to
"dev_of_node(dev)".

[1] https://lore.kernel.org/all/20231220203537.83479-2-jernej.skrabec@gmail.com/

Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
Changes since v5:
- Used "dev_of_node(dev)" instead of "dev->of_node"
- Replaced "dev_printk" with "dev_printk()" in kerneldoc mentions
- Fixed kerneldoc "Return" section format for of_regulator_get_optional()
- Fix @np parameter name in of_regulator_dev_lookup() kerneldoc

Changes since v4:
- Restore platform-agnostic regulator consumer code to original state
- Move OF-specific regulator code to of_regulator.c (separate patch)
- Split _regulator_get() into three parts for reuse (separate patch)
- Add OF-specific _of_regulator_get() function
- Rename regulator_of_get_optional() to of_regulator_get_optional() for
  consistency
- Make of_regulator_get_optional static, as it is only used internally
- Convert of_regulator_bulk_get_all()

Changes since v3:
- New patch
---
 drivers/regulator/core.c         |  4 +--
 drivers/regulator/internal.h     |  2 ++
 drivers/regulator/of_regulator.c | 52 +++++++++++++++++++++++++++-----
 3 files changed, 49 insertions(+), 9 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index d60c86477ac2..f94a06ac2109 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1959,8 +1959,8 @@ static struct regulator_dev *regulator_dev_lookup(struct device *dev,
 	regulator_supply_alias(&dev, &supply);
 
 	/* first do a dt based lookup */
-	if (dev && dev->of_node) {
-		r = of_regulator_dev_lookup(dev, supply);
+	if (dev_of_node(dev)) {
+		r = of_regulator_dev_lookup(dev, dev_of_node(dev), supply);
 		if (!IS_ERR(r))
 			return r;
 		if (PTR_ERR(r) == -EPROBE_DEFER)
diff --git a/drivers/regulator/internal.h b/drivers/regulator/internal.h
index 5b43f802468d..f62cacbbc729 100644
--- a/drivers/regulator/internal.h
+++ b/drivers/regulator/internal.h
@@ -67,6 +67,7 @@ static inline struct regulator_dev *dev_to_rdev(struct device *dev)
 
 #ifdef CONFIG_OF
 struct regulator_dev *of_regulator_dev_lookup(struct device *dev,
+					      struct device_node *np,
 					      const char *supply);
 struct regulator_init_data *regulator_of_get_init_data(struct device *dev,
 			         const struct regulator_desc *desc,
@@ -82,6 +83,7 @@ bool of_check_coupling_data(struct regulator_dev *rdev);
 
 #else
 static inline struct regulator_dev *of_regulator_dev_lookup(struct device *dev,
+							    struct device_node *np,
 							    const char *supply)
 {
 	return ERR_PTR(-ENODEV);
diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index d5dd7a9e577b..b00172176d0c 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -590,7 +590,8 @@ static struct device_node *of_get_child_regulator(struct device_node *parent,
 
 /**
  * of_get_regulator - get a regulator device node based on supply name
- * @dev: Device pointer for the consumer (of regulator) device
+ * @dev: Device pointer for dev_printk() messages
+ * @node: Device node pointer for supply property lookup
  * @supply: regulator supply name
  *
  * Extract the regulator device node corresponding to the supply name.
@@ -598,15 +599,16 @@ static struct device_node *of_get_child_regulator(struct device_node *parent,
  * Return: Pointer to the &struct device_node corresponding to the regulator
  *	   if found, or %NULL if not found.
  */
-static struct device_node *of_get_regulator(struct device *dev, const char *supply)
+static struct device_node *of_get_regulator(struct device *dev, struct device_node *node,
+					    const char *supply)
 {
 	struct device_node *regnode = NULL;
 	char prop_name[64]; /* 64 is max size of property name */
 
-	dev_dbg(dev, "Looking up %s-supply from device tree\n", supply);
+	dev_dbg(dev, "Looking up %s-supply from device node %pOF\n", supply, node);
 
 	snprintf(prop_name, 64, "%s-supply", supply);
-	regnode = of_parse_phandle(dev->of_node, prop_name, 0);
+	regnode = of_parse_phandle(node, prop_name, 0);
 
 	if (!regnode) {
 		regnode = of_get_child_regulator(dev->of_node, prop_name);
@@ -632,6 +634,7 @@ static struct regulator_dev *of_find_regulator_by_node(struct device_node *np)
 /**
  * of_regulator_dev_lookup - lookup a regulator device with device tree only
  * @dev: Device pointer for regulator supply lookup.
+ * @np: Device node pointer for regulator supply lookup.
  * @supply: Supply name or regulator ID.
  *
  * Return: Pointer to the &struct regulator_dev on success, or ERR_PTR()
@@ -646,13 +649,13 @@ static struct regulator_dev *of_find_regulator_by_node(struct device_node *np)
  * * -%ENODEV if lookup fails permanently.
  * * -%EPROBE_DEFER if lookup could succeed in the future.
  */
-struct regulator_dev *of_regulator_dev_lookup(struct device *dev,
+struct regulator_dev *of_regulator_dev_lookup(struct device *dev, struct device_node *np,
 					      const char *supply)
 {
 	struct regulator_dev *r;
 	struct device_node *node;
 
-	node = of_get_regulator(dev, supply);
+	node = of_get_regulator(dev, np, supply);
 	if (node) {
 		r = of_find_regulator_by_node(node);
 		of_node_put(node);
@@ -669,6 +672,41 @@ struct regulator_dev *of_regulator_dev_lookup(struct device *dev,
 	return ERR_PTR(-ENODEV);
 }
 
+static struct regulator *_of_regulator_get(struct device *dev, struct device_node *node,
+					   const char *id, enum regulator_get_type get_type)
+{
+	struct regulator_dev *r;
+	int ret;
+
+	ret = _regulator_get_common_check(dev, id, get_type);
+	if (ret)
+		return ERR_PTR(ret);
+
+	r = of_regulator_dev_lookup(dev, node, id);
+	return _regulator_get_common(r, dev, id, get_type);
+}
+
+/**
+ * of_regulator_get_optional - get optional regulator via device tree lookup
+ * @dev: device used for dev_printk() messages
+ * @node: device node for regulator "consumer"
+ * @id: Supply name
+ *
+ * Return: pointer to struct regulator corresponding to the regulator producer,
+ *	   or PTR_ERR() encoded error number.
+ *
+ * This is intended for use by consumers that want to get a regulator
+ * supply directly from a device node, and can and want to deal with
+ * absence of such supplies. This will _not_ consider supply aliases.
+ * See regulator_dev_lookup().
+ */
+static struct regulator *of_regulator_get_optional(struct device *dev,
+						   struct device_node *node,
+						   const char *id)
+{
+	return _of_regulator_get(NULL, node, id, OPTIONAL_GET);
+}
+
 /*
  * Returns number of regulators coupled with rdev.
  */
@@ -886,7 +924,7 @@ int of_regulator_bulk_get_all(struct device *dev, struct device_node *np,
 		} else {
 			memcpy(name, prop->name, i);
 			name[i] = '\0';
-			tmp = regulator_get(dev, name);
+			tmp = of_regulator_get_optional(dev, np, name);
 			if (IS_ERR(tmp)) {
 				ret = PTR_ERR(tmp);
 				goto error;
-- 
2.46.0.469.g59c65b2a67-goog

[PATCH v6 06/12] gpiolib: Add gpio_get_property_name_length()

[Chen-Yu Tsai]

The I2C device tree component prober needs to get and toggle GPIO lines
for the components it intends to probe. These components may not use the
same name for their GPIO lines, so the prober must go through the device
tree, check each property to see it is a GPIO property, and get the GPIO
line.

Instead of duplicating the GPIO suffixes, or exporting them to the
prober to do pattern matching, simply add and export a new function that
does the pattern matching and returns the length of the GPIO name. The
caller can then use that to copy out the name if it needs to.

Andy suggested a much shorter implementation.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>

---
Changes since v5:
- Changed function name to "gpio_get_property_name_length()"
- Changed argument name to "propname"
- Clarified return value for "*-<GPIO suffix>" case
- Reworked according to Andy's suggestion
- Added stub function

Changes since v4:
- new patch
---
 drivers/gpio/gpiolib.c        | 25 +++++++++++++++++++++++++
 include/linux/gpio/consumer.h |  7 +++++++
 2 files changed, 32 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 3903d0a75304..86527cc7991b 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -4295,6 +4295,31 @@ struct gpio_desc *fwnode_gpiod_get_index(struct fwnode_handle *fwnode,
 }
 EXPORT_SYMBOL_GPL(fwnode_gpiod_get_index);
 
+/**
+ * gpio_get_property_name_length - Returns the GPIO name length from a property name
+ * @propname:	name of the property to check
+ *
+ * This function checks if the given property name matches the GPIO property
+ * patterns, and returns the length of the name of the GPIO. The pattern is
+ * "*-<GPIO suffix>" or just "<GPIO suffix>".
+ *
+ * Returns:
+ * The length of the string before '-<GPIO suffix>' if it matches
+ * "*-<GPIO suffix>", or 0 if no name part, just the suffix, or
+ * -EINVAL if the string doesn't match the pattern.
+ */
+int gpio_get_property_name_length(const char *propname)
+{
+	const char *dash = strrchr(propname, '-');
+
+	for (const char *const *p = gpio_suffixes; *p; p++)
+		if (!strcmp(dash ? dash + 1 : propname, *p))
+			return dash ? dash - propname : 0;
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(gpio_get_property_name_length);
+
 /**
  * gpiod_count - return the number of GPIOs associated with a device / function
  *		or -ENOENT if no GPIO has been assigned to the requested function
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index db2dfbae8edb..494dde33ca44 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -56,6 +56,8 @@ enum gpiod_flags {
 
 #ifdef CONFIG_GPIOLIB
 
+int gpio_get_property_name_length(const char *propname);
+
 /* Return the number of GPIOs associated with a device / function */
 int gpiod_count(struct device *dev, const char *con_id);
 
@@ -188,6 +190,11 @@ struct gpio_desc *devm_fwnode_gpiod_get_index(struct device *dev,
 
 #include <asm/bug.h>
 
+static inline int gpio_get_property_name_length(const char *propname)
+{
+	return ERR_PTR(-ENOSYS);
+}
+
 static inline int gpiod_count(struct device *dev, const char *con_id)
 {
 	return 0;
-- 
2.46.0.469.g59c65b2a67-goog

[PATCH v6 07/12] i2c: core: Remove extra space in Makefile

[Chen-Yu Tsai]

Some lines in the Makefile have a space before tabs. Remove those.

Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Closes: https://lore.kernel.org/all/ZsdE0PxKnGRjzChl@smile.fi.intel.com/
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
Changes since v5:
- new patch
---
 drivers/i2c/Makefile | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
index 3f71ce4711e3..f12d6b10a85e 100644
--- a/drivers/i2c/Makefile
+++ b/drivers/i2c/Makefile
@@ -5,10 +5,10 @@
 
 obj-$(CONFIG_I2C_BOARDINFO)	+= i2c-boardinfo.o
 obj-$(CONFIG_I2C)		+= i2c-core.o
-i2c-core-objs 			:= i2c-core-base.o i2c-core-smbus.o
+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_I2C_SLAVE)	+= i2c-core-slave.o
+i2c-core-$(CONFIG_OF)		+= i2c-core-of.o
 
 obj-$(CONFIG_I2C_SMBUS)		+= i2c-smbus.o
 obj-$(CONFIG_I2C_CHARDEV)	+= i2c-dev.o
-- 
2.46.0.469.g59c65b2a67-goog

[PATCH v6 08/12] i2c: Introduce OF component probe function

[Chen-Yu Tsai]

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@chromium.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) {
+		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;
+	}
+
+	i2c = of_get_i2c_adapter_by_node(i2c_node);
+	if (!i2c)
+		return dev_err_probe(dev, -EPROBE_DEFER, "Couldn't get I2C adapter\n");
+
+	ret = 0;
+	for_each_child_of_node_scoped(i2c_node, node) {
+		union i2c_smbus_data data;
+		u32 addr;
+
+		if (!of_node_name_prefix(node, type))
+			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;
+
+		/* Found a device that is responding */
+		ret = i2c_of_probe_enable_node(dev, node);
+		break;
+	}
+
+	i2c_put_adapter(i2c);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(i2c_of_probe_component);
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 388ce71a29a9..c6c16731243d 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -1033,6 +1033,10 @@ const struct of_device_id
 int of_i2c_get_board_info(struct device *dev, struct device_node *node,
 			  struct i2c_board_info *info);
 
+#if IS_ENABLED(CONFIG_OF_DYNAMIC)
+int i2c_of_probe_component(struct device *dev, const char *type);
+#endif
+
 #else
 
 static inline struct i2c_client *of_find_i2c_device_by_node(struct device_node *node)
-- 
2.46.0.469.g59c65b2a67-goog

[PATCH v6 09/12] i2c: of-prober: Add regulator support

[Chen-Yu Tsai]

This adds regulator management to the I2C OF component prober.
Components that the prober intends to probe likely require their
regulator supplies be enabled, and GPIOs be toggled to enable them or
bring them out of reset before they will respond to probe attempts.
GPIOs will be handled in the next patch.

Without specific knowledge of each component's resource names or
power sequencing requirements, the prober can only enable the
regulator supplies all at once, and toggle the GPIOs all at once.
Luckily, reset pins tend to be active low, while enable pins tend to
be active high, so setting the raw status of all GPIO pins to high
should work. The wait time before and after resources are enabled
are collected from existing drivers and device trees.

The prober collects resources from all possible components and enables
them together, instead of enabling resources and probing each component
one by one. The latter approach does not provide any boot time benefits
over simply enabling each component and letting each driver probe
sequentially.

The prober will also deduplicate the resources, since on a component
swap out or co-layout design, the resources are always the same.
While duplicate regulator supplies won't cause much issue, shared
GPIOs don't work reliably, especially with other drivers. For the
same reason, the prober will release the GPIOs before the successfully
probed component is actually enabled.

Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
Changes since v5:
- Split of_regulator_bulk_get_all() return value check and explain
  "ret == 0" case
- Switched to of_get_next_child_with_prefix_scoped() where applicable
- Used krealloc_array() instead of directly calculating size
- copy whole regulator array in one memcpy() call
- Drop "0" from struct zeroing initializer
- Split out regulator helper from i2c_of_probe_enable_res() to keep
  code cleaner when combined with the next patch
- Added options for customizing power sequencing delay
- Rename i2c_of_probe_get_regulator() to i2c_of_probe_get_regulators()
- Add i2c_of_probe_free_regulator() helper

Changes since v4:
- Split out GPIO handling to separate patch
- Rewrote using of_regulator_bulk_get_all()
- Replaced "regulators" with "regulator supplies" in debug messages

Changes since v3:
- New patch

This change is kept as a separate patch for now since the changes are
quite numerous.
---
 drivers/i2c/i2c-core-of-prober.c | 154 +++++++++++++++++++++++++++++--
 include/linux/i2c.h              |  10 +-
 2 files changed, 155 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/i2c-core-of-prober.c b/drivers/i2c/i2c-core-of-prober.c
index 64d7631f4885..56b06ad7aa64 100644
--- a/drivers/i2c/i2c-core-of-prober.c
+++ b/drivers/i2c/i2c-core-of-prober.c
@@ -6,12 +6,14 @@
  */
 
 #include <linux/cleanup.h>
+#include <linux/delay.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/regulator/consumer.h>
 #include <linux/slab.h>
 
 /*
@@ -27,11 +29,119 @@
  * address responds.
  *
  * TODO:
- * - Support handling common regulators.
  * - Support handling common GPIOs.
  * - Support I2C muxes
  */
 
+struct i2c_of_probe_data {
+	const struct i2c_of_probe_opts *opts;
+	struct regulator_bulk_data *regulators;
+	unsigned int regulators_num;
+};
+
+/* Returns number of regulator supplies found for node, or error. */
+static int i2c_of_probe_get_regulators(struct device *dev, struct device_node *node,
+				       struct i2c_of_probe_data *data)
+{
+	struct regulator_bulk_data *tmp, *new_regulators;
+	int ret;
+
+	ret = of_regulator_bulk_get_all(dev, node, &tmp);
+	if (ret < 0) {
+		return ret;
+	} else if (ret == 0) {
+		/*
+		 * It's entirely possible for a device node to not have
+		 * regulator supplies. While it doesn't make sense from
+		 * a hardware perspective, the supplies could be always
+		 * on or otherwise not modeled in the device tree, but
+		 * the device would still work.
+		 */
+		return ret;
+	}
+
+	if (!data->regulators) {
+		data->regulators = tmp;
+		data->regulators_num = ret;
+		return ret;
+	};
+
+	new_regulators = krealloc_array(data->regulators, (data->regulators_num + ret),
+					sizeof(*tmp), GFP_KERNEL);
+	if (!new_regulators) {
+		regulator_bulk_free(ret, tmp);
+		return -ENOMEM;
+	}
+
+	data->regulators = new_regulators;
+	memcpy(&data->regulators[data->regulators_num], tmp, sizeof(*tmp) * ret);
+	data->regulators_num += ret;
+
+	return ret;
+}
+
+static void i2c_of_probe_free_regulators(struct i2c_of_probe_data *data)
+{
+	regulator_bulk_free(data->regulators_num, data->regulators);
+	data->regulators_num = 0;
+	data->regulators = NULL;
+}
+
+static void i2c_of_probe_free_res(struct i2c_of_probe_data *data)
+{
+	i2c_of_probe_free_regulators(data);
+}
+
+static int i2c_of_probe_get_res(struct device *dev, struct device_node *node,
+				struct i2c_of_probe_data *data)
+{
+	struct property *prop;
+	int ret;
+
+	ret = i2c_of_probe_get_regulators(dev, node, data);
+	if (ret < 0) {
+		dev_err_probe(dev, ret, "Failed to get regulator supplies from %pOF\n", node);
+		goto err_cleanup;
+	}
+
+	return 0;
+
+err_cleanup:
+	i2c_of_probe_free_res(data);
+	return ret;
+}
+
+static int i2c_of_probe_enable_regulators(struct device *dev, struct i2c_of_probe_data *data)
+{
+	int ret;
+
+	dev_dbg(dev, "Enabling regulator supplies\n");
+
+	ret = regulator_bulk_enable(data->regulators_num, data->regulators);
+	if (ret)
+		return ret;
+
+	msleep(data->opts->post_power_on_delay_ms);
+
+	return 0;
+}
+
+static void i2c_of_probe_disable_regulators(struct i2c_of_probe_data *data)
+{
+	regulator_bulk_disable(data->regulators_num, data->regulators);
+}
+
+static int i2c_of_probe_enable_res(struct device *dev, struct i2c_of_probe_data *data)
+{
+	int ret;
+
+	ret = i2c_of_probe_enable_regulators(dev, data);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 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);
@@ -78,10 +188,17 @@ static int i2c_of_probe_enable_node(struct device *dev, struct device_node *node
 	return ret;
 }
 
+static const struct i2c_of_probe_opts i2c_of_probe_opts_default = {
+	/* largest post-power-on pre-reset-deassert delay seen among drivers */
+	.post_power_on_delay_ms = 500,
+};
+
 /**
  * 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
+ * @opts: &struct i2c_of_probe_data containing tweakable options for the prober.
+ *	  Defaults are used if this is %NULL.
  *
  * Probe for possible I2C components of the same "type" on the same I2C bus
  * that have their status marked as "fail".
@@ -108,8 +225,9 @@ static int i2c_of_probe_enable_node(struct device *dev, struct device_node *node
  *         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)
+int i2c_of_probe_component(struct device *dev, const char *type, const struct i2c_of_probe_opts *opts)
 {
+	struct i2c_of_probe_data probe_data = { .opts = opts ?: &i2c_of_probe_opts_default };
 	struct i2c_adapter *i2c;
 	int ret;
 
@@ -117,9 +235,7 @@ int i2c_of_probe_component(struct device *dev, const char *type)
 	if (IS_ERR(i2c_node))
 		return PTR_ERR(i2c_node);
 
-	for_each_child_of_node_scoped(i2c_node, node) {
-		if (!of_node_name_prefix(node, type))
-			continue;
+	for_each_child_of_node_with_prefix_scoped(i2c_node, node, type) {
 		if (!of_device_is_available(node))
 			continue;
 
@@ -134,13 +250,33 @@ int i2c_of_probe_component(struct device *dev, const char *type)
 	if (!i2c)
 		return dev_err_probe(dev, -EPROBE_DEFER, "Couldn't get I2C adapter\n");
 
+	/* Grab resources */
+	for_each_child_of_node_with_prefix_scoped(i2c_node, node, type) {
+		u32 addr;
+
+		if (of_property_read_u32(node, "reg", &addr))
+			continue;
+
+		dev_dbg(dev, "Requesting resources for %pOF\n", node);
+		ret = i2c_of_probe_get_res(dev, node, &probe_data);
+		if (ret)
+			return ret;
+	}
+
+	dev_dbg(dev, "Resources: # of regulator supplies = %d\n", probe_data.regulators_num);
+
+	/* Enable resources */
+	ret = i2c_of_probe_enable_res(dev, &probe_data);
+	if (ret) {
+		i2c_of_probe_free_res(&probe_data);
+		return dev_err_probe(dev, ret, "Failed to enable resources\n");
+	}
+
 	ret = 0;
-	for_each_child_of_node_scoped(i2c_node, node) {
+	for_each_child_of_node_with_prefix_scoped(i2c_node, node, type) {
 		union i2c_smbus_data data;
 		u32 addr;
 
-		if (!of_node_name_prefix(node, type))
-			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)
@@ -151,6 +287,8 @@ int i2c_of_probe_component(struct device *dev, const char *type)
 		break;
 	}
 
+	i2c_of_probe_disable_regulators(&probe_data);
+	i2c_of_probe_free_res(&probe_data);
 	i2c_put_adapter(i2c);
 
 	return ret;
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index c6c16731243d..dbcdb8edbf6f 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -1034,7 +1034,15 @@ int of_i2c_get_board_info(struct device *dev, struct device_node *node,
 			  struct i2c_board_info *info);
 
 #if IS_ENABLED(CONFIG_OF_DYNAMIC)
-int i2c_of_probe_component(struct device *dev, const char *type);
+/**
+ * i2c_of_probe_opts - I2C OF component prober customization options
+ * @post_power_on_delay_us: Delay in ms after regulators are powered on. Passed to msleep().
+ */
+struct i2c_of_probe_opts {
+	unsigned int post_power_on_delay_ms;
+};
+
+int i2c_of_probe_component(struct device *dev, const char *type, const struct i2c_of_probe_opts *opts);
 #endif
 
 #else
-- 
2.46.0.469.g59c65b2a67-goog

[PATCH v6 10/12] i2c: of-prober: Add GPIO support

[Chen-Yu Tsai]

This adds GPIO management to the I2C OF component prober.
Components that the prober intends to probe likely require their
regulator supplies be enabled, and GPIOs be toggled to enable them or
bring them out of reset before they will respond to probe attempts.
regulator support was added in the previous patch.

Without specific knowledge of each component's resource names or
power sequencing requirements, the prober can only enable the
regulator supplies all at once, and toggle the GPIOs all at once.
Luckily, reset pins tend to be active low, while enable pins tend to
be active high, so setting the raw status of all GPIO pins to high
should work. The wait time before and after resources are enabled
are collected from existing drivers and device trees.

The prober collects resources from all possible components and enables
them together, instead of enabling resources and probing each component
one by one. The latter approach does not provide any boot time benefits
over simply enabling each component and letting each driver probe
sequentially.

The prober will also deduplicate the resources, since on a component
swap out or co-layout design, the resources are always the same.
While duplicate regulator supplies won't cause much issue, shared
GPIOs don't work reliably, especially with other drivers. For the
same reason, the prober will release the GPIOs before the successfully
probed component is actually enabled.

Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>

---
Changes since v5:
- Renamed "con" to "propname" in i2c_of_probe_get_gpiod()
- Copy string first and check return value of strscpy() for overflow in
  i2c_of_probe_get_gpiod()
- Add parenthesis around "enable" and "reset" GPIO names in comments
- Split resource count debug message into two separate lines
- Split out GPIO helper from i2c_of_probe_enable_res() to keep code
  cleaner following the previous patch
- Adopted options for customizing power sequencing delay following
  previous patch

Changes since v4:
- Split out from previous patch
- Moved GPIO property name check to common function in gpiolib.c in new
  patch
- Moved i2c_of_probe_free_gpios() into for_each_child_of_node_scoped()
- Rewrote in gpiod_*_array-esque fashion
---
 drivers/i2c/i2c-core-of-prober.c | 143 ++++++++++++++++++++++++++++++-
 include/linux/i2c.h              |   2 +
 2 files changed, 144 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-core-of-prober.c b/drivers/i2c/i2c-core-of-prober.c
index 56b06ad7aa64..04242ff86e69 100644
--- a/drivers/i2c/i2c-core-of-prober.c
+++ b/drivers/i2c/i2c-core-of-prober.c
@@ -5,16 +5,19 @@
  * Copyright (C) 2024 Google LLC
  */
 
+#include <linux/bitmap.h>
 #include <linux/cleanup.h>
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/dev_printk.h>
 #include <linux/err.h>
+#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
+#include <linux/types.h>
 
 /*
  * Some devices, such as Google Hana Chromebooks, are produced by multiple
@@ -29,12 +32,14 @@
  * address responds.
  *
  * TODO:
- * - Support handling common GPIOs.
+ * - Support inverted polarity GPIOs, such as electrical high to "disable".
+ *   Seen on some OmniVision camera sensors.
  * - Support I2C muxes
  */
 
 struct i2c_of_probe_data {
 	const struct i2c_of_probe_opts *opts;
+	struct gpio_descs *gpiods;
 	struct regulator_bulk_data *regulators;
 	unsigned int regulators_num;
 };
@@ -85,10 +90,90 @@ static void i2c_of_probe_free_regulators(struct i2c_of_probe_data *data)
 	regulator_bulk_free(data->regulators_num, data->regulators);
 	data->regulators_num = 0;
 	data->regulators = NULL;
+};
+
+/*
+ * Returns 1 if property is GPIO and GPIO successfully requested,
+ * 0 if not a GPIO property, or error if request for GPIO failed.
+ */
+static int i2c_of_probe_get_gpiod(struct device_node *node, struct property *prop,
+				  struct i2c_of_probe_data *data)
+{
+	struct fwnode_handle *fwnode = of_fwnode_handle(node);
+	struct gpio_descs *gpiods;
+	struct gpio_desc *gpiod;
+	char propname[32]; /* 32 is max size of property name */
+	char *con_id = NULL;
+	size_t new_size;
+	int len, ret;
+
+	len = gpio_get_property_name_length(prop->name);
+	if (len < 0)
+		return 0;
+
+	ret = strscpy(propname, prop->name);
+	if (ret < 0) {
+		pr_err("%pOF: length of GPIO name \"%s\" exceeds current limit\n",
+		       node, prop->name);
+		return -EINVAL;
+	}
+
+	if (len > 0) {
+		/* "len < ARRAY_SIZE(propname)" guaranteed by strscpy() above */
+		propname[len] = '\0';
+		con_id = propname;
+	}
+
+	/*
+	 * GPIO descriptors are not reference counted. GPIOD_FLAGS_BIT_NONEXCLUSIVE
+	 * can't differentiate between GPIOs shared between devices to be probed and
+	 * other devices (which is incorrect). If the initial request fails with
+	 * -EBUSY, retry with GPIOD_FLAGS_BIT_NONEXCLUSIVE and see if it matches
+	 * any existing ones.
+	 */
+	gpiod = fwnode_gpiod_get_index(fwnode, con_id, 0, GPIOD_ASIS, "i2c-of-prober");
+	if (IS_ERR(gpiod)) {
+		if (PTR_ERR(gpiod) != -EBUSY || !data->gpiods)
+			return PTR_ERR(gpiod);
+
+		gpiod = fwnode_gpiod_get_index(fwnode, con_id, 0,
+					       GPIOD_ASIS | GPIOD_FLAGS_BIT_NONEXCLUSIVE,
+					       "i2c-of-prober");
+		for (unsigned int i = 0; i < data->gpiods->ndescs; i++)
+			if (gpiod == data->gpiods->desc[i])
+				return 1;
+
+		return -EBUSY;
+	}
+
+	new_size = struct_size(gpiods, desc, data->gpiods ? data->gpiods->ndescs + 1 : 1);
+	gpiods = krealloc(data->gpiods, new_size, GFP_KERNEL);
+	if (!gpiods) {
+		gpiod_put(gpiod);
+		return -ENOMEM;
+	}
+
+	data->gpiods = gpiods;
+	data->gpiods->desc[data->gpiods->ndescs++] = gpiod;
+
+	return 1;
+}
+
+/*
+ * This is split into two functions because in the normal flow the GPIOs
+ * have to be released before the actual driver probes so that the latter
+ * can acquire them.
+ */
+static void i2c_of_probe_free_gpios(struct i2c_of_probe_data *data)
+{
+	if (data->gpiods)
+		gpiod_put_array(data->gpiods);
+	data->gpiods = NULL;
 }
 
 static void i2c_of_probe_free_res(struct i2c_of_probe_data *data)
 {
+	i2c_of_probe_free_gpios(data);
 	i2c_of_probe_free_regulators(data);
 }
 
@@ -104,6 +189,18 @@ static int i2c_of_probe_get_res(struct device *dev, struct device_node *node,
 		goto err_cleanup;
 	}
 
+	for_each_property_of_node(node, prop) {
+		dev_dbg(dev, "Trying property %pOF/%s\n", node, prop->name);
+
+		/* GPIOs */
+		ret = i2c_of_probe_get_gpiod(node, prop, data);
+		if (ret < 0) {
+			dev_err_probe(dev, ret, "Failed to get GPIO from %pOF/%s\n",
+				      node, prop->name);
+			goto err_cleanup;
+		}
+	}
+
 	return 0;
 
 err_cleanup:
@@ -131,6 +228,37 @@ static void i2c_of_probe_disable_regulators(struct i2c_of_probe_data *data)
 	regulator_bulk_disable(data->regulators_num, data->regulators);
 }
 
+static int i2c_of_probe_set_gpios(struct device *dev, struct i2c_of_probe_data *data)
+{
+	int ret;
+	int gpio_i;
+
+	if (!data->gpiods)
+		return 0;
+
+	for (gpio_i = 0; gpio_i < data->gpiods->ndescs; gpio_i++) {
+		/*
+		 * "reset" GPIOs normally have opposite polarity compared to
+		 * "enable" GPIOs. Instead of parsing the flags again, simply
+		 * set the raw value to high.
+		 */
+		dev_dbg(dev, "Setting GPIO %d\n", gpio_i);
+		ret = gpiod_direction_output_raw(data->gpiods->desc[gpio_i], 1);
+		if (ret)
+			goto disable_gpios;
+	}
+
+	msleep(data->opts->post_reset_deassert_delay_ms);
+
+	return 0;
+
+disable_gpios:
+	for (gpio_i--; gpio_i >= 0; gpio_i--)
+		gpiod_set_raw_value_cansleep(data->gpiods->desc[gpio_i], 0);
+
+	return ret;
+}
+
 static int i2c_of_probe_enable_res(struct device *dev, struct i2c_of_probe_data *data)
 {
 	int ret;
@@ -139,7 +267,15 @@ static int i2c_of_probe_enable_res(struct device *dev, struct i2c_of_probe_data
 	if (ret)
 		return ret;
 
+	ret = i2c_of_probe_set_gpios(dev, data);
+	if (ret)
+		goto err_disable_regulators;
+
 	return 0;
+
+err_disable_regulators:
+	i2c_of_probe_disable_regulators(data);
+	return ret;
 }
 
 static struct device_node *i2c_of_probe_get_i2c_node(struct device *dev, const char *type)
@@ -191,6 +327,8 @@ static int i2c_of_probe_enable_node(struct device *dev, struct device_node *node
 static const struct i2c_of_probe_opts i2c_of_probe_opts_default = {
 	/* largest post-power-on pre-reset-deassert delay seen among drivers */
 	.post_power_on_delay_ms = 500,
+	/* largest post-reset-deassert delay seen in tree for Elan I2C HID */
+	.post_reset_deassert_delay_ms = 300,
 };
 
 /**
@@ -264,6 +402,8 @@ int i2c_of_probe_component(struct device *dev, const char *type, const struct i2
 	}
 
 	dev_dbg(dev, "Resources: # of regulator supplies = %d\n", probe_data.regulators_num);
+	dev_dbg(dev, "Resources: # of GPIOs = %d\n",
+		probe_data.gpiods ? probe_data.gpiods->ndescs : 0);
 
 	/* Enable resources */
 	ret = i2c_of_probe_enable_res(dev, &probe_data);
@@ -283,6 +423,7 @@ int i2c_of_probe_component(struct device *dev, const char *type, const struct i2
 			continue;
 
 		/* Found a device that is responding */
+		i2c_of_probe_free_gpios(&probe_data);
 		ret = i2c_of_probe_enable_node(dev, node);
 		break;
 	}
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index dbcdb8edbf6f..0da1edddb5a2 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -1037,9 +1037,11 @@ int of_i2c_get_board_info(struct device *dev, struct device_node *node,
 /**
  * i2c_of_probe_opts - I2C OF component prober customization options
  * @post_power_on_delay_us: Delay in ms after regulators are powered on. Passed to msleep().
+ * @post_reset_deassert_delay_ms: Delay in ms after GPIOs are set. Passed to msleep().
  */
 struct i2c_of_probe_opts {
 	unsigned int post_power_on_delay_ms;
+	unsigned int post_reset_deassert_delay_ms;
 };
 
 int i2c_of_probe_component(struct device *dev, const char *type, const struct i2c_of_probe_opts *opts);
-- 
2.46.0.469.g59c65b2a67-goog

[PATCH v6 11/12] platform/chrome: Introduce device tree hardware prober

[Chen-Yu Tsai]

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". 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>
---
Changes since v5:
- Adapt to new i2c_of_probe_component() parameters

Changes since v4:
- Fix Kconfig dependency
- Update copyright year
- Drop "linux/of.h" header
- Include "linux/errno.h"
- Move |int ret| declaration to top of block
- Return -ENODEV on no match instead of 0
- Unregister platform driver and device unconditionally after previous
  change

Changes since v3:
- Include linux/init.h
- Rewrite for loop in driver probe function as suggested by Andy
- Make prober driver buildable as module
- Ignore prober errors other than probe deferral

Changes since v2:
- Addressed Rob's comments
  - Move remaining driver code to drivers/platform/chrome/
  - Depend on rather than select CONFIG_I2C
  - Copy machine check to driver init function
- Addressed Andy's comments
  - Explicitly mention "device tree" or OF in driver name, description
    and Kconfig symbol
  - Drop filename from inside the file
  - Switch to passing "struct device *" to shorten lines
  - Move "ret = 0" to just before for_each_child_of_node(i2c_node, node)
  - Make loop variable size_t (instead of unsigned int as Andy asked)
  - Use PLATFORM_DEVID_NONE instead of raw -1
  - Use standard goto error path pattern in hw_prober_driver_init()

- Changes since v1:
  - New patch
---
 drivers/platform/chrome/Kconfig               |  11 ++
 drivers/platform/chrome/Makefile              |   1 +
 .../platform/chrome/chromeos_of_hw_prober.c   | 104 ++++++++++++++++++
 3 files changed, 116 insertions(+)
 create mode 100644 drivers/platform/chrome/chromeos_of_hw_prober.c

diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
index 7dbeb786352a..b7dbaf77b6db 100644
--- a/drivers/platform/chrome/Kconfig
+++ b/drivers/platform/chrome/Kconfig
@@ -61,6 +61,17 @@ config CHROMEOS_TBMC
 	  To compile this driver as a module, choose M here: the
 	  module will be called chromeos_tbmc.
 
+config CHROMEOS_OF_HW_PROBER
+	tristate "ChromeOS Device Tree Hardware Prober"
+	depends on OF
+	depends on I2C
+	select OF_DYNAMIC
+	default OF
+	help
+	  This option enables the device tree hardware prober for ChromeOS
+	  devices. The driver will probe the correct component variant in
+	  devices that have multiple drop-in options for one component.
+
 config CROS_EC
 	tristate "ChromeOS Embedded Controller"
 	select CROS_EC_PROTO
diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
index 2dcc6ccc2302..21a9d5047053 100644
--- a/drivers/platform/chrome/Makefile
+++ b/drivers/platform/chrome/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_CHROMEOS_ACPI)		+= chromeos_acpi.o
 obj-$(CONFIG_CHROMEOS_LAPTOP)		+= chromeos_laptop.o
 obj-$(CONFIG_CHROMEOS_PRIVACY_SCREEN)	+= chromeos_privacy_screen.o
 obj-$(CONFIG_CHROMEOS_PSTORE)		+= chromeos_pstore.o
+obj-$(CONFIG_CHROMEOS_OF_HW_PROBER)	+= chromeos_of_hw_prober.o
 obj-$(CONFIG_CHROMEOS_TBMC)		+= chromeos_tbmc.o
 obj-$(CONFIG_CROS_EC)			+= cros_ec.o
 obj-$(CONFIG_CROS_EC_I2C)		+= cros_ec_i2c.o
diff --git a/drivers/platform/chrome/chromeos_of_hw_prober.c b/drivers/platform/chrome/chromeos_of_hw_prober.c
new file mode 100644
index 000000000000..90fc1de16788
--- /dev/null
+++ b/drivers/platform/chrome/chromeos_of_hw_prober.c
@@ -0,0 +1,104 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * ChromeOS Device Tree Hardware Prober
+ *
+ * Copyright (c) 2024 Google LLC
+ */
+
+#include <linux/array_size.h>
+#include <linux/errno.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#define DRV_NAME	"chromeos_of_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 device *dev, const void *data);
+	const void *data;
+};
+
+static int chromeos_i2c_component_prober(struct device *dev, const void *data)
+{
+	const char *type = data;
+
+	return i2c_of_probe_component(dev, type, NULL);
+}
+
+static const struct hw_prober_entry hw_prober_platforms[] = {
+	{ .compatible = "google,hana", .prober = chromeos_i2c_component_prober, .data = "touchscreen" },
+	{ .compatible = "google,hana", .prober = chromeos_i2c_component_prober, .data = "trackpad" },
+};
+
+static int chromeos_of_hw_prober_probe(struct platform_device *pdev)
+{
+	for (size_t i = 0; i < ARRAY_SIZE(hw_prober_platforms); i++) {
+		int ret;
+
+		if (!of_machine_is_compatible(hw_prober_platforms[i].compatible))
+			continue;
+
+		ret = hw_prober_platforms[i].prober(&pdev->dev, hw_prober_platforms[i].data);
+		/* Ignore unrecoverable errors and keep going through other probers */
+		if (ret == -EPROBE_DEFER)
+			return ret;
+	}
+
+	return 0;
+}
+
+static struct platform_driver chromeos_of_hw_prober_driver = {
+	.probe	= chromeos_of_hw_prober_probe,
+	.driver	= {
+		.name = DRV_NAME,
+	},
+};
+
+static struct platform_device *chromeos_of_hw_prober_pdev;
+
+static int chromeos_of_hw_prober_driver_init(void)
+{
+	size_t i;
+	int ret;
+
+	for (i = 0; i < ARRAY_SIZE(hw_prober_platforms); i++)
+		if (of_machine_is_compatible(hw_prober_platforms[i].compatible))
+			break;
+	if (i == ARRAY_SIZE(hw_prober_platforms))
+		return -ENODEV;
+
+	ret = platform_driver_register(&chromeos_of_hw_prober_driver);
+	if (ret)
+		return ret;
+
+	chromeos_of_hw_prober_pdev =
+			platform_device_register_simple(DRV_NAME, PLATFORM_DEVID_NONE, NULL, 0);
+	if (IS_ERR(chromeos_of_hw_prober_pdev))
+		goto err;
+
+	return 0;
+
+err:
+	platform_driver_unregister(&chromeos_of_hw_prober_driver);
+
+	return PTR_ERR(chromeos_of_hw_prober_pdev);
+}
+module_init(chromeos_of_hw_prober_driver_init);
+
+static void chromeos_of_hw_prober_driver_exit(void)
+{
+	platform_device_unregister(chromeos_of_hw_prober_pdev);
+	platform_driver_unregister(&chromeos_of_hw_prober_driver);
+}
+module_exit(chromeos_of_hw_prober_driver_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("ChromeOS device tree hardware prober");
-- 
2.46.0.469.g59c65b2a67-goog

[PATCH v6 12/12] arm64: dts: mediatek: mt8173-elm-hana: Mark touchscreens and trackpads as fail

[Chen-Yu Tsai]

Instead of having them all available, mark them all as "fail-needs-probe"
and have the implementation try to probe which one is present.

Also remove the shared resource workaround by moving the pinctrl entry
for the trackpad interrupt line back into the individual trackpad nodes.

Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
Changes since v4:
- Rebased

Changes since v3:
- Also remove second source workaround, i.e. move the interrupt line
  pinctrl entry from the i2c node back to the components.

Changes since v2:
- Drop class from status
---
 arch/arm64/boot/dts/mediatek/mt8173-elm-hana.dtsi | 13 +++++++++++++
 arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi      |  4 ++--
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/mediatek/mt8173-elm-hana.dtsi b/arch/arm64/boot/dts/mediatek/mt8173-elm-hana.dtsi
index 8d1cbc92bce3..251e084bf7de 100644
--- a/arch/arm64/boot/dts/mediatek/mt8173-elm-hana.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8173-elm-hana.dtsi
@@ -14,6 +14,7 @@ touchscreen2: touchscreen@34 {
 		compatible = "melfas,mip4_ts";
 		reg = <0x34>;
 		interrupts-extended = <&pio 88 IRQ_TYPE_LEVEL_LOW>;
+		status = "fail-needs-probe";
 	};
 
 	/*
@@ -26,6 +27,7 @@ touchscreen3: touchscreen@20 {
 		reg = <0x20>;
 		hid-descr-addr = <0x0020>;
 		interrupts-extended = <&pio 88 IRQ_TYPE_LEVEL_LOW>;
+		status = "fail-needs-probe";
 	};
 
 	/* Lenovo Ideapad C330 uses G2Touch touchscreen as a 2nd source touchscreen */
@@ -47,9 +49,12 @@ &i2c4 {
 	trackpad2: trackpad@2c {
 		compatible = "hid-over-i2c";
 		interrupts-extended = <&pio 117 IRQ_TYPE_LEVEL_LOW>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&trackpad_irq>;
 		reg = <0x2c>;
 		hid-descr-addr = <0x0020>;
 		wakeup-source;
+		status = "fail-needs-probe";
 	};
 };
 
@@ -74,3 +79,11 @@ pins_wp {
 		};
 	};
 };
+
+&touchscreen {
+	status = "fail-needs-probe";
+};
+
+&trackpad {
+	status = "fail-needs-probe";
+};
diff --git a/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi b/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
index b4d85147b77b..eee64461421f 100644
--- a/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
@@ -358,12 +358,12 @@ touchscreen: touchscreen@10 {
 &i2c4 {
 	clock-frequency = <400000>;
 	status = "okay";
-	pinctrl-names = "default";
-	pinctrl-0 = <&trackpad_irq>;
 
 	trackpad: trackpad@15 {
 		compatible = "elan,ekth3000";
 		interrupts-extended = <&pio 117 IRQ_TYPE_LEVEL_LOW>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&trackpad_irq>;
 		reg = <0x15>;
 		vcc-supply = <&mt6397_vgp6_reg>;
 		wakeup-source;
-- 
2.46.0.469.g59c65b2a67-goog

Re: [PATCH v6 11/12] platform/chrome: Introduce device tree hardware prober

[Tzung-Bi Shih]

On Wed, Sep 04, 2024 at 05:00:13PM +0800, Chen-Yu Tsai wrote:
> diff --git a/drivers/platform/chrome/chromeos_of_hw_prober.c b/drivers/platform/chrome/chromeos_of_hw_prober.c
[...]
> +static int chromeos_of_hw_prober_probe(struct platform_device *pdev)
> +{
> +	for (size_t i = 0; i < ARRAY_SIZE(hw_prober_platforms); i++) {
> +		int ret;
> +
> +		if (!of_machine_is_compatible(hw_prober_platforms[i].compatible))
> +			continue;
> +
> +		ret = hw_prober_platforms[i].prober(&pdev->dev, hw_prober_platforms[i].data);
> +		/* Ignore unrecoverable errors and keep going through other probers */
> +		if (ret == -EPROBE_DEFER)
> +			return ret;

Is it harmless if some of the components get probed multiple times?  E.g.:
comp1 probed -> comp2 probed -> comp3 returned -EPROBE_DEFER -> some time
later, chromeos_of_hw_prober_probe() gets called again.

Re: [PATCH v6 02/12] of: base: Add for_each_child_of_node_with_prefix_scoped()

[Andy Shevchenko]

On Wed, Sep 04, 2024 at 05:00:04PM +0800, Chen-Yu Tsai wrote:
> There are cases where drivers would go through child device nodes and
> operate on only the ones whose node name starts with a given prefix.
> 
> Provide a helper for these users. This will mainly be used in a
> subsequent patch that implements a hardware component prober for I2C
> busses.

...

> +#define for_each_child_of_node_with_prefix_scoped(parent, child, prefix) \
> +	for (struct device_node *child __free(device_node) =		\
> +	     of_get_next_child_with_prefix(parent, NULL, prefix);	\
> +	     child != NULL;						\
> +	     child = of_get_next_child_with_prefix(parent, child, prefix))

I'm wondering if we may drop _scoped from day 1. Yeah, probably a bit confusing
as the rest of APIs without that suffix do require reference count handling on
the loop abrupt.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 03/12] regulator: Move OF-specific regulator lookup code to of_regulator.c

[Andy Shevchenko]

On Wed, Sep 04, 2024 at 05:00:05PM +0800, Chen-Yu Tsai wrote:
> There's still a bit of OF-specific code in the regulator device lookup
> function.
> 
> Move those bits of code over to of_regulator.c, and create a new
> function of_regulator_dev_lookup() to encapsulate the code moved out of
> regulator_dev_lookup().
> 
> Also mark of_find_regulator_by_node() as static, since there are no
> other users in other compile units.
> 
> There are no functional changes. A line alignment was also fixed.

...

> +/**
> + * of_get_child_regulator - get a child regulator device node
> + * based on supply name
> + * @parent: Parent device node
> + * @prop_name: Combination regulator supply name and "-supply"
> + *
> + * Traverse all child nodes.
> + * Extract the child regulator device node corresponding to the supply name.
> + *
> + * Return: Pointer to the &struct device_node corresponding to the regulator
> + *	   if found, or %NULL if not found.
> + */
> +static struct device_node *of_get_child_regulator(struct device_node *parent,
> +						  const char *prop_name)
> +{
> +	struct device_node *regnode = NULL;
> +	struct device_node *child = NULL;
> +
> +	for_each_child_of_node(parent, child) {

> +		regnode = of_parse_phandle(child, prop_name, 0);
> +		if (!regnode) {
> +			regnode = of_get_child_regulator(child, prop_name);
> +			if (regnode)
> +				goto err_node_put;
> +		} else {
> +			goto err_node_put;
> +		}

I know this is just a move of the existing code, but consider negating the
conditional and have something like

		regnode = of_parse_phandle(child, prop_name, 0);
		if (regnode)
			goto err_node_put;

		regnode = of_get_child_regulator(child, prop_name);
		if (regnode)
			goto err_node_put;

> +	}
> +	return NULL;
> +
> +err_node_put:
> +	of_node_put(child);
> +	return regnode;
> +}

...

I assume the use of _scoped() macros is in mind for the future changes?

...

> +/**
> + * of_get_regulator - get a regulator device node based on supply name
> + * @dev: Device pointer for the consumer (of regulator) device
> + * @supply: regulator supply name
> + *
> + * Extract the regulator device node corresponding to the supply name.
> + *
> + * Return: Pointer to the &struct device_node corresponding to the regulator
> + *	   if found, or %NULL if not found.
> + */
> +static struct device_node *of_get_regulator(struct device *dev, const char *supply)
> +{
> +	struct device_node *regnode = NULL;
> +	char prop_name[64]; /* 64 is max size of property name */
> +
> +	dev_dbg(dev, "Looking up %s-supply from device tree\n", supply);
> +
> +	snprintf(prop_name, 64, "%s-supply", supply);
> +	regnode = of_parse_phandle(dev->of_node, prop_name, 0);

> +	if (!regnode) {

Similarly here

	snprintf(prop_name, 64, "%s-supply", supply);
	regnode = of_parse_phandle(dev->of_node, prop_name, 0);
	if (regnode)
		return regnode;


> +		regnode = of_get_child_regulator(dev->of_node, prop_name);
> +		if (regnode)
> +			return regnode;
> +
> +		dev_dbg(dev, "Looking up %s property in node %pOF failed\n",
> +			prop_name, dev->of_node);
> +		return NULL;
> +	}
> +	return regnode;

	regnode = of_get_child_regulator(dev->of_node, prop_name);
	if (regnode)
		return regnode;

	dev_dbg(dev, "Looking up %s property in node %pOF failed\n", prop_name, dev->of_node);
	return NULL;

> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 05/12] regulator: Do pure DT regulator lookup in of_regulator_bulk_get_all()

[Andy Shevchenko]

On Wed, Sep 04, 2024 at 05:00:07PM +0800, Chen-Yu Tsai wrote:
> The to-be-introduced I2C component prober needs to enable regulator
> supplies (and toggle GPIO pins) for the various components it intends
> to probe. To support this, a new "pure DT lookup" method for getting
> regulator supplies is needed, since the device normally requesting
> the supply won't get created until after the component is probed to
> be available.
> 
> Convert the existing of_regulator_bulk_get_all() for this purpose.
> This function has no in-tree users, as the original patch [1] that
> used it was never landed. This patch changes the function ABI, but
> it is straightforward to convert users.
> 
> The underlying code that supports the existing regulator_get*()
> functions has been reworked in previous patches to support this
> specific case. An internal OF-specific version of regulator_get(),
> of_regulator_get_optional(), is added for this.
> 
> Also convert an existing usage of "dev && dev->of_node" to
> "dev_of_node(dev)".

> [1] https://lore.kernel.org/all/20231220203537.83479-2-jernej.skrabec@gmail.com/
> 
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>

Make it Link tag.

  Link: https://lore.kernel.org/all/20231220203537.83479-2-jernej.skrabec@gmail.com/ [1]
  Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 03/12] regulator: Move OF-specific regulator lookup code to of_regulator.c

[Andy Shevchenko]

On Wed, Sep 04, 2024 at 04:09:26PM +0300, Andy Shevchenko wrote:
> On Wed, Sep 04, 2024 at 05:00:05PM +0800, Chen-Yu Tsai wrote:

> > +static struct device_node *of_get_child_regulator(struct device_node *parent,
> > +						  const char *prop_name)
> > +{
> > +	struct device_node *regnode = NULL;

> > +	struct device_node *child = NULL;

Btw, redundant assignment here, as child will be assigned anyway AFAIR.

> > +	for_each_child_of_node(parent, child) {
> 
> > +		regnode = of_parse_phandle(child, prop_name, 0);
> > +		if (!regnode) {
> > +			regnode = of_get_child_regulator(child, prop_name);
> > +			if (regnode)
> > +				goto err_node_put;
> > +		} else {
> > +			goto err_node_put;
> > +		}
> 
> I know this is just a move of the existing code, but consider negating the
> conditional and have something like
> 
> 		regnode = of_parse_phandle(child, prop_name, 0);
> 		if (regnode)
> 			goto err_node_put;
> 
> 		regnode = of_get_child_regulator(child, prop_name);
> 		if (regnode)
> 			goto err_node_put;
> 
> > +	}
> > +	return NULL;
> > +
> > +err_node_put:
> > +	of_node_put(child);
> > +	return regnode;
> > +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 06/12] gpiolib: Add gpio_get_property_name_length()

[Andy Shevchenko]

On Wed, Sep 04, 2024 at 05:00:08PM +0800, Chen-Yu Tsai wrote:
> The I2C device tree component prober needs to get and toggle GPIO lines
> for the components it intends to probe. These components may not use the
> same name for their GPIO lines, so the prober must go through the device
> tree, check each property to see it is a GPIO property, and get the GPIO
> line.
> 
> Instead of duplicating the GPIO suffixes, or exporting them to the
> prober to do pattern matching, simply add and export a new function that
> does the pattern matching and returns the length of the GPIO name. The
> caller can then use that to copy out the name if it needs to.

> Andy suggested a much shorter implementation.

No need to have this sentence in the commit message, changelog area is fine.
But if you wish... :-)

> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>

...

> +/**
> + * gpio_get_property_name_length - Returns the GPIO name length from a property name
> + * @propname:	name of the property to check
> + *
> + * This function checks if the given property name matches the GPIO property
> + * patterns, and returns the length of the name of the GPIO. The pattern is
> + * "*-<GPIO suffix>" or just "<GPIO suffix>".
> + *
> + * Returns:
> + * The length of the string before '-<GPIO suffix>' if it matches
> + * "*-<GPIO suffix>", or 0 if no name part, just the suffix, or
> + * -EINVAL if the string doesn't match the pattern.

Should be %-EINVAL as we agreed with Bart when I updated GPIOLIB kernel-doc.

> + */

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 07/12] i2c: core: Remove extra space in Makefile

[Andy Shevchenko]

On Wed, Sep 04, 2024 at 05:00:09PM +0800, Chen-Yu Tsai wrote:
> Some lines in the Makefile have a space before tabs. Remove those.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 02/12] of: base: Add for_each_child_of_node_with_prefix_scoped()

[Rob Herring]

On Wed, Sep 04, 2024 at 04:03:47PM +0300, Andy Shevchenko wrote:
> On Wed, Sep 04, 2024 at 05:00:04PM +0800, Chen-Yu Tsai wrote:
> > There are cases where drivers would go through child device nodes and
> > operate on only the ones whose node name starts with a given prefix.
> > 
> > Provide a helper for these users. This will mainly be used in a
> > subsequent patch that implements a hardware component prober for I2C
> > busses.
> 
> ...
> 
> > +#define for_each_child_of_node_with_prefix_scoped(parent, child, prefix) \
> > +	for (struct device_node *child __free(device_node) =		\
> > +	     of_get_next_child_with_prefix(parent, NULL, prefix);	\
> > +	     child != NULL;						\
> > +	     child = of_get_next_child_with_prefix(parent, child, prefix))
> 
> I'm wondering if we may drop _scoped from day 1. Yeah, probably a bit confusing
> as the rest of APIs without that suffix do require reference count handling on
> the loop abrupt.

Yes, please drop. We have other new ones coming and they don't have 
"_scoped". I was on the fence, but if you use a scoped one like a 
non-scoped one (declaring child outside or using it outside the loop) 
you will get a compiler error.

Rob

Re: [PATCH v6 08/12] i2c: Introduce OF component probe function

[Andy Shevchenko]

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.

...

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

Hmm, but can it be for_each_child_of_node_with_prefix_scoped() now?

> +		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;
> +	}
> +
> +	i2c = of_get_i2c_adapter_by_node(i2c_node);
> +	if (!i2c)
> +		return dev_err_probe(dev, -EPROBE_DEFER, "Couldn't get I2C adapter\n");
> +
> +	ret = 0;
> +	for_each_child_of_node_scoped(i2c_node, node) {

Ditto.

> +		union i2c_smbus_data data;
> +		u32 addr;
> +
> +		if (!of_node_name_prefix(node, type))
> +			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;
> +
> +		/* Found a device that is responding */
> +		ret = i2c_of_probe_enable_node(dev, node);
> +		break;
> +	}
> +
> +	i2c_put_adapter(i2c);
> +
> +	return ret;
> +}

...

> +EXPORT_SYMBOL_GPL(i2c_of_probe_component);

Wonder if we may already use namespaced export from day 1.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 09/12] i2c: of-prober: Add regulator support

[Andy Shevchenko]

On Wed, Sep 04, 2024 at 05:00:11PM +0800, Chen-Yu Tsai wrote:
> This adds regulator management to the I2C OF component prober.
> Components that the prober intends to probe likely require their
> regulator supplies be enabled, and GPIOs be toggled to enable them or
> bring them out of reset before they will respond to probe attempts.
> GPIOs will be handled in the next patch.
> 
> Without specific knowledge of each component's resource names or
> power sequencing requirements, the prober can only enable the
> regulator supplies all at once, and toggle the GPIOs all at once.
> Luckily, reset pins tend to be active low, while enable pins tend to
> be active high, so setting the raw status of all GPIO pins to high
> should work. The wait time before and after resources are enabled
> are collected from existing drivers and device trees.
> 
> The prober collects resources from all possible components and enables
> them together, instead of enabling resources and probing each component
> one by one. The latter approach does not provide any boot time benefits
> over simply enabling each component and letting each driver probe
> sequentially.
> 
> The prober will also deduplicate the resources, since on a component
> swap out or co-layout design, the resources are always the same.
> While duplicate regulator supplies won't cause much issue, shared
> GPIOs don't work reliably, especially with other drivers. For the
> same reason, the prober will release the GPIOs before the successfully
> probed component is actually enabled.

...

> +static int i2c_of_probe_get_regulators(struct device *dev, struct device_node *node,
> +				       struct i2c_of_probe_data *data)
> +{
> +	struct regulator_bulk_data *tmp, *new_regulators;
> +	int ret;
> +
> +	ret = of_regulator_bulk_get_all(dev, node, &tmp);
> +	if (ret < 0) {
> +		return ret;
> +	} else if (ret == 0) {
> +		/*
> +		 * It's entirely possible for a device node to not have
> +		 * regulator supplies. While it doesn't make sense from
> +		 * a hardware perspective, the supplies could be always
> +		 * on or otherwise not modeled in the device tree, but
> +		 * the device would still work.
> +		 */
> +		return ret;
> +	}

	if (ret < 0)
		return ret;

	/*
	 * It's entirely possible for a device node to not have regulator
	 * supplies. While it doesn't make sense from a hardware perspective,
	 * the supplies could be always on or otherwise not modeled in
	 * the device tree, but the device would still work.
	 */
	if (ret == 0)
		return ret;

> +	if (!data->regulators) {
> +		data->regulators = tmp;
> +		data->regulators_num = ret;
> +		return ret;
> +	};
> +
> +	new_regulators = krealloc_array(data->regulators, (data->regulators_num + ret),

Redundant parentheses.

> +					sizeof(*tmp), GFP_KERNEL);
> +	if (!new_regulators) {
> +		regulator_bulk_free(ret, tmp);
> +		return -ENOMEM;
> +	}
> +
> +	data->regulators = new_regulators;
> +	memcpy(&data->regulators[data->regulators_num], tmp, sizeof(*tmp) * ret);

Shouldn't be the size calculated based on the size of the destination?

> +	data->regulators_num += ret;
> +
> +	return ret;
> +}

...

As I said earlier my main concern is that timeout heuristic which seems fragile.
But I have no ideas to propose, leave this to others to comment on / think about.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 10/12] i2c: of-prober: Add GPIO support

[Andy Shevchenko]

On Wed, Sep 04, 2024 at 05:00:12PM +0800, Chen-Yu Tsai wrote:
> This adds GPIO management to the I2C OF component prober.
> Components that the prober intends to probe likely require their
> regulator supplies be enabled, and GPIOs be toggled to enable them or
> bring them out of reset before they will respond to probe attempts.
> regulator support was added in the previous patch.
> 
> Without specific knowledge of each component's resource names or
> power sequencing requirements, the prober can only enable the
> regulator supplies all at once, and toggle the GPIOs all at once.
> Luckily, reset pins tend to be active low, while enable pins tend to
> be active high, so setting the raw status of all GPIO pins to high
> should work. The wait time before and after resources are enabled
> are collected from existing drivers and device trees.
> 
> The prober collects resources from all possible components and enables
> them together, instead of enabling resources and probing each component
> one by one. The latter approach does not provide any boot time benefits
> over simply enabling each component and letting each driver probe
> sequentially.
> 
> The prober will also deduplicate the resources, since on a component
> swap out or co-layout design, the resources are always the same.
> While duplicate regulator supplies won't cause much issue, shared
> GPIOs don't work reliably, especially with other drivers. For the
> same reason, the prober will release the GPIOs before the successfully
> probed component is actually enabled.

...

> +static int i2c_of_probe_get_gpiod(struct device_node *node, struct property *prop,
> +				  struct i2c_of_probe_data *data)
> +{
> +	struct fwnode_handle *fwnode = of_fwnode_handle(node);
> +	struct gpio_descs *gpiods;
> +	struct gpio_desc *gpiod;
> +	char propname[32]; /* 32 is max size of property name */
> +	char *con_id = NULL;
> +	size_t new_size;
> +	int len, ret;
> +
> +	len = gpio_get_property_name_length(prop->name);
> +	if (len < 0)
> +		return 0;
> +
> +	ret = strscpy(propname, prop->name);
> +	if (ret < 0) {
> +		pr_err("%pOF: length of GPIO name \"%s\" exceeds current limit\n",
> +		       node, prop->name);
> +		return -EINVAL;

Any good reason to shadow the error code from strscpy() here?

> +	}
> +
> +	if (len > 0) {
> +		/* "len < ARRAY_SIZE(propname)" guaranteed by strscpy() above */

is guaranteed

> +		propname[len] = '\0';

Please, check if it's guaranteed by strscpy() (IIRC it is, hence redundant line).

> +		con_id = propname;
> +	}
> +
> +	/*
> +	 * GPIO descriptors are not reference counted. GPIOD_FLAGS_BIT_NONEXCLUSIVE
> +	 * can't differentiate between GPIOs shared between devices to be probed and
> +	 * other devices (which is incorrect). If the initial request fails with
> +	 * -EBUSY, retry with GPIOD_FLAGS_BIT_NONEXCLUSIVE and see if it matches
> +	 * any existing ones.
> +	 */
> +	gpiod = fwnode_gpiod_get_index(fwnode, con_id, 0, GPIOD_ASIS, "i2c-of-prober");
> +	if (IS_ERR(gpiod)) {
> +		if (PTR_ERR(gpiod) != -EBUSY || !data->gpiods)
> +			return PTR_ERR(gpiod);
> +
> +		gpiod = fwnode_gpiod_get_index(fwnode, con_id, 0,
> +					       GPIOD_ASIS | GPIOD_FLAGS_BIT_NONEXCLUSIVE,
> +					       "i2c-of-prober");
> +		for (unsigned int i = 0; i < data->gpiods->ndescs; i++)
> +			if (gpiod == data->gpiods->desc[i])
> +				return 1;
> +
> +		return -EBUSY;
> +	}
> +
> +	new_size = struct_size(gpiods, desc, data->gpiods ? data->gpiods->ndescs + 1 : 1);
> +	gpiods = krealloc(data->gpiods, new_size, GFP_KERNEL);
> +	if (!gpiods) {
> +		gpiod_put(gpiod);
> +		return -ENOMEM;
> +	}
> +
> +	data->gpiods = gpiods;
> +	data->gpiods->desc[data->gpiods->ndescs++] = gpiod;
> +
> +	return 1;
> +}

...

> +static int i2c_of_probe_set_gpios(struct device *dev, struct i2c_of_probe_data *data)
> +{
> +	int ret;
> +	int gpio_i;

Why signed? And can it be simply named 'i'?

> +
> +	if (!data->gpiods)
> +		return 0;
> +
> +	for (gpio_i = 0; gpio_i < data->gpiods->ndescs; gpio_i++) {
> +		/*
> +		 * "reset" GPIOs normally have opposite polarity compared to
> +		 * "enable" GPIOs. Instead of parsing the flags again, simply
> +		 * set the raw value to high.
> +		 */
> +		dev_dbg(dev, "Setting GPIO %d\n", gpio_i);
> +		ret = gpiod_direction_output_raw(data->gpiods->desc[gpio_i], 1);
> +		if (ret)
> +			goto disable_gpios;
> +	}
> +
> +	msleep(data->opts->post_reset_deassert_delay_ms);
> +
> +	return 0;
> +
> +disable_gpios:

> +	for (gpio_i--; gpio_i >= 0; gpio_i--)

	while (i--)

> +		gpiod_set_raw_value_cansleep(data->gpiods->desc[gpio_i], 0);
> +
> +	return ret;
> +}


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 08/12] i2c: Introduce OF component probe function

[Rob Herring]

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

Re: (subset) [PATCH v6 00/12] platform/chrome: Introduce DT hardware prober

[Mark Brown]

On Wed, 04 Sep 2024 17:00:02 +0800, Chen-Yu Tsai wrote:
> This is v6 of my "of: Introduce hardware prober driver" [1] series.
> v6 mainly addresses comments from Andy.
> 
> v2 continued Doug's "of: device: Support 2nd sources of probeable but
> undiscoverable devices" [2] series, but follows the scheme suggested by
> Rob, marking all second source component device nodes as "fail-needs-probe",
> and having a hardware prober driver enable the one of them.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next

Thanks!

[03/12] regulator: Move OF-specific regulator lookup code to of_regulator.c
        commit: b8c3255457147162cd713a319a8e2274335449b9

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [PATCH v6 09/12] i2c: of-prober: Add regulator support

[Doug Anderson]

Hi,

On Wed, Sep 4, 2024 at 2:01 AM Chen-Yu Tsai <wenst@chromium.org> wrote:
>
> This adds regulator management to the I2C OF component prober.
> Components that the prober intends to probe likely require their
> regulator supplies be enabled, and GPIOs be toggled to enable them or
> bring them out of reset before they will respond to probe attempts.
> GPIOs will be handled in the next patch.
>
> Without specific knowledge of each component's resource names or
> power sequencing requirements, the prober can only enable the
> regulator supplies all at once, and toggle the GPIOs all at once.
> Luckily, reset pins tend to be active low, while enable pins tend to
> be active high, so setting the raw status of all GPIO pins to high
> should work. The wait time before and after resources are enabled
> are collected from existing drivers and device trees.
>
> The prober collects resources from all possible components and enables
> them together, instead of enabling resources and probing each component
> one by one. The latter approach does not provide any boot time benefits
> over simply enabling each component and letting each driver probe
> sequentially.
>
> The prober will also deduplicate the resources, since on a component
> swap out or co-layout design, the resources are always the same.
> While duplicate regulator supplies won't cause much issue, shared
> GPIOs don't work reliably, especially with other drivers. For the
> same reason, the prober will release the GPIOs before the successfully
> probed component is actually enabled.
>
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
> Changes since v5:
> - Split of_regulator_bulk_get_all() return value check and explain
>   "ret == 0" case
> - Switched to of_get_next_child_with_prefix_scoped() where applicable
> - Used krealloc_array() instead of directly calculating size
> - copy whole regulator array in one memcpy() call
> - Drop "0" from struct zeroing initializer
> - Split out regulator helper from i2c_of_probe_enable_res() to keep
>   code cleaner when combined with the next patch
> - Added options for customizing power sequencing delay
> - Rename i2c_of_probe_get_regulator() to i2c_of_probe_get_regulators()
> - Add i2c_of_probe_free_regulator() helper
>
> Changes since v4:
> - Split out GPIO handling to separate patch
> - Rewrote using of_regulator_bulk_get_all()
> - Replaced "regulators" with "regulator supplies" in debug messages
>
> Changes since v3:
> - New patch
>
> This change is kept as a separate patch for now since the changes are
> quite numerous.
> ---
>  drivers/i2c/i2c-core-of-prober.c | 154 +++++++++++++++++++++++++++++--
>  include/linux/i2c.h              |  10 +-
>  2 files changed, 155 insertions(+), 9 deletions(-)

I never jumped back into looking at this since you started sending new
versions last month (sorry), but I finally did...

At a high level, I have to say I'm not really a fan of the "reach into
all of the devices, jam their regulators on, force their GPIOs high,
and hope for the best" approach. It just feels like it's going to
break at the first bit of slightly different hardware and cause power
sequence violations left and right. If nothing else, regulators often
need delays between when one regulator is enabled and the next. There
may also be complex relationships between regulators and GPIOs, GPIOs,
GPIOs that need to be low, or even GPIO "toggle sequences" (power on
rail 1, wait 1 ms, assert reset, wait 10 ms, deassert reset, power on
rail 2).

IMO the only way to make this reliable is to have this stuff be much
less automatic and much more driven by the board.

I think that, in general, before the board prober checks i2c address
then the prober should be in charge of setting up pinctrl and turning
on regulators / GPIOs. Given that the same regulator(s) and GPIO(s)
may be specified by different children, the prober will just have to
pick one child to find those resources. It should have enough
board-specific knowledge to make this choice. Then the prober should
turn them on via a board-specific power-on sequence that's known to
work for all the children. Then it should start probing.

I think there can still be plenty of common helper functions that the
board-specific prober can leverage, but overall I'd expect the actual
power-on and power-off code to be board-specific.

If many boards have in common that we need to turn on exactly one
regulator + one GPIO, or just one regulator, or whatever then having a
helper function that handles these cases is fine. ...but it should be
one of many choices that a board proper could use and not the only
one.

-Doug

Re: [PATCH v6 11/12] platform/chrome: Introduce device tree hardware prober

[Chen-Yu Tsai]

On Wed, Sep 4, 2024 at 6:08 PM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> On Wed, Sep 04, 2024 at 05:00:13PM +0800, Chen-Yu Tsai wrote:
> > diff --git a/drivers/platform/chrome/chromeos_of_hw_prober.c b/drivers/platform/chrome/chromeos_of_hw_prober.c
> [...]
> > +static int chromeos_of_hw_prober_probe(struct platform_device *pdev)
> > +{
> > +     for (size_t i = 0; i < ARRAY_SIZE(hw_prober_platforms); i++) {
> > +             int ret;
> > +
> > +             if (!of_machine_is_compatible(hw_prober_platforms[i].compatible))
> > +                     continue;
> > +
> > +             ret = hw_prober_platforms[i].prober(&pdev->dev, hw_prober_platforms[i].data);
> > +             /* Ignore unrecoverable errors and keep going through other probers */
> > +             if (ret == -EPROBE_DEFER)
> > +                     return ret;
>
> Is it harmless if some of the components get probed multiple times?  E.g.:
> comp1 probed -> comp2 probed -> comp3 returned -EPROBE_DEFER -> some time
> later, chromeos_of_hw_prober_probe() gets called again.

Yes it is harmless. Components already enabled will not get disabled
in the error path. And the prober that enabled that component will see
that a component was enabled, and skip doing the whole process again.

So something like:

    comp1 probed -> comp2 probed -> comp3 -EPROBE_DEFER ->
        comp1 skip -> comp2 skip -> comp3 probed


ChenYu

Re: [PATCH v6 11/12] platform/chrome: Introduce device tree hardware prober

[Tzung-Bi Shih]

On Thu, Sep 05, 2024 at 11:52:32AM +0800, Chen-Yu Tsai wrote:
> On Wed, Sep 4, 2024 at 6:08 PM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
> >
> > On Wed, Sep 04, 2024 at 05:00:13PM +0800, Chen-Yu Tsai wrote:
> > > diff --git a/drivers/platform/chrome/chromeos_of_hw_prober.c b/drivers/platform/chrome/chromeos_of_hw_prober.c
> > [...]
> > > +static int chromeos_of_hw_prober_probe(struct platform_device *pdev)
> > > +{
> > > +     for (size_t i = 0; i < ARRAY_SIZE(hw_prober_platforms); i++) {
> > > +             int ret;
> > > +
> > > +             if (!of_machine_is_compatible(hw_prober_platforms[i].compatible))
> > > +                     continue;
> > > +
> > > +             ret = hw_prober_platforms[i].prober(&pdev->dev, hw_prober_platforms[i].data);
> > > +             /* Ignore unrecoverable errors and keep going through other probers */
> > > +             if (ret == -EPROBE_DEFER)
> > > +                     return ret;
> >
> > Is it harmless if some of the components get probed multiple times?  E.g.:
> > comp1 probed -> comp2 probed -> comp3 returned -EPROBE_DEFER -> some time
> > later, chromeos_of_hw_prober_probe() gets called again.
> 
> Yes it is harmless. Components already enabled will not get disabled
> in the error path. And the prober that enabled that component will see
> that a component was enabled, and skip doing the whole process again.
> 
> So something like:
> 
>     comp1 probed -> comp2 probed -> comp3 -EPROBE_DEFER ->
>         comp1 skip -> comp2 skip -> comp3 probed

I assume this patch will be applied through the i2c tree:
Acked-by: Tzung-Bi Shih <tzungbi@kernel.org>

Re: [PATCH v6 03/12] regulator: Move OF-specific regulator lookup code to of_regulator.c

[Chen-Yu Tsai]

On Wed, Sep 4, 2024 at 9:16 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Sep 04, 2024 at 04:09:26PM +0300, Andy Shevchenko wrote:
> > On Wed, Sep 04, 2024 at 05:00:05PM +0800, Chen-Yu Tsai wrote:
>
> > > +static struct device_node *of_get_child_regulator(struct device_node *parent,
> > > +                                             const char *prop_name)
> > > +{
> > > +   struct device_node *regnode = NULL;
>
> > > +   struct device_node *child = NULL;
>
> Btw, redundant assignment here, as child will be assigned anyway AFAIR.
>
> > > +   for_each_child_of_node(parent, child) {
> >
> > > +           regnode = of_parse_phandle(child, prop_name, 0);
> > > +           if (!regnode) {
> > > +                   regnode = of_get_child_regulator(child, prop_name);
> > > +                   if (regnode)
> > > +                           goto err_node_put;
> > > +           } else {
> > > +                   goto err_node_put;
> > > +           }
> >
> > I know this is just a move of the existing code, but consider negating the
> > conditional and have something like
> >
> >               regnode = of_parse_phandle(child, prop_name, 0);
> >               if (regnode)
> >                       goto err_node_put;
> >
> >               regnode = of_get_child_regulator(child, prop_name);
> >               if (regnode)
> >                       goto err_node_put;
> >

Looks like Mark already merged this one. I'll send extra patches to clean
this up later.

ChenYu

> > > +   }
> > > +   return NULL;
> > > +
> > > +err_node_put:
> > > +   of_node_put(child);
> > > +   return regnode;
> > > +}
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v6 03/12] regulator: Move OF-specific regulator lookup code to of_regulator.c

[Andy Shevchenko]

On Thu, Sep 05, 2024 at 04:11:18PM +0800, Chen-Yu Tsai wrote:
> On Wed, Sep 4, 2024 at 9:16 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:

...

> Looks like Mark already merged this one. I'll send extra patches to clean
> this up later.

I was OOF, haven't read this and I sent already a patch yesterday evening.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 09/12] i2c: of-prober: Add regulator support

[Chen-Yu Tsai]

On Thu, Sep 5, 2024 at 6:57 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Wed, Sep 4, 2024 at 2:01 AM Chen-Yu Tsai <wenst@chromium.org> wrote:
> >
> > This adds regulator management to the I2C OF component prober.
> > Components that the prober intends to probe likely require their
> > regulator supplies be enabled, and GPIOs be toggled to enable them or
> > bring them out of reset before they will respond to probe attempts.
> > GPIOs will be handled in the next patch.
> >
> > Without specific knowledge of each component's resource names or
> > power sequencing requirements, the prober can only enable the
> > regulator supplies all at once, and toggle the GPIOs all at once.
> > Luckily, reset pins tend to be active low, while enable pins tend to
> > be active high, so setting the raw status of all GPIO pins to high
> > should work. The wait time before and after resources are enabled
> > are collected from existing drivers and device trees.
> >
> > The prober collects resources from all possible components and enables
> > them together, instead of enabling resources and probing each component
> > one by one. The latter approach does not provide any boot time benefits
> > over simply enabling each component and letting each driver probe
> > sequentially.
> >
> > The prober will also deduplicate the resources, since on a component
> > swap out or co-layout design, the resources are always the same.
> > While duplicate regulator supplies won't cause much issue, shared
> > GPIOs don't work reliably, especially with other drivers. For the
> > same reason, the prober will release the GPIOs before the successfully
> > probed component is actually enabled.
> >
> > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > ---
> > Changes since v5:
> > - Split of_regulator_bulk_get_all() return value check and explain
> >   "ret == 0" case
> > - Switched to of_get_next_child_with_prefix_scoped() where applicable
> > - Used krealloc_array() instead of directly calculating size
> > - copy whole regulator array in one memcpy() call
> > - Drop "0" from struct zeroing initializer
> > - Split out regulator helper from i2c_of_probe_enable_res() to keep
> >   code cleaner when combined with the next patch
> > - Added options for customizing power sequencing delay
> > - Rename i2c_of_probe_get_regulator() to i2c_of_probe_get_regulators()
> > - Add i2c_of_probe_free_regulator() helper
> >
> > Changes since v4:
> > - Split out GPIO handling to separate patch
> > - Rewrote using of_regulator_bulk_get_all()
> > - Replaced "regulators" with "regulator supplies" in debug messages
> >
> > Changes since v3:
> > - New patch
> >
> > This change is kept as a separate patch for now since the changes are
> > quite numerous.
> > ---
> >  drivers/i2c/i2c-core-of-prober.c | 154 +++++++++++++++++++++++++++++--
> >  include/linux/i2c.h              |  10 +-
> >  2 files changed, 155 insertions(+), 9 deletions(-)
>
> I never jumped back into looking at this since you started sending new
> versions last month (sorry), but I finally did...
>
> At a high level, I have to say I'm not really a fan of the "reach into
> all of the devices, jam their regulators on, force their GPIOs high,
> and hope for the best" approach. It just feels like it's going to
> break at the first bit of slightly different hardware and cause power
> sequence violations left and right. If nothing else, regulators often
> need delays between when one regulator is enabled and the next. There
> may also be complex relationships between regulators and GPIOs, GPIOs,
> GPIOs that need to be low, or even GPIO "toggle sequences" (power on
> rail 1, wait 1 ms, assert reset, wait 10 ms, deassert reset, power on
> rail 2).
>
> IMO the only way to make this reliable is to have this stuff be much
> less automatic and much more driven by the board.
>
> I think that, in general, before the board prober checks i2c address
> then the prober should be in charge of setting up pinctrl and turning
> on regulators / GPIOs. Given that the same regulator(s) and GPIO(s)
> may be specified by different children, the prober will just have to
> pick one child to find those resources. It should have enough
> board-specific knowledge to make this choice. Then the prober should
> turn them on via a board-specific power-on sequence that's known to
> work for all the children. Then it should start probing.
>
> I think there can still be plenty of common helper functions that the
> board-specific prober can leverage, but overall I'd expect the actual
> power-on and power-off code to be board-specific.
>
> If many boards have in common that we need to turn on exactly one
> regulator + one GPIO, or just one regulator, or whatever then having a
> helper function that handles these cases is fine. ...but it should be
> one of many choices that a board proper could use and not the only
> one.

IIUC we could have the "options" data structure have much more board
specific information:

  - name of node to fetch resources (regulator supplies and GPIOs) from
  - names of the resources for the node given from the previous item
  - delay time after each resource is toggled
  - polarity in the case of GPIOs
  - prober callback to do power sequencing

The "resource collection" step would use the first two items to retrieve
the regulator supplies and GPIOS instead of the bulk APIs used right now.

The power sequencing callback would use the resources combined with the
given delays to enable the supplies and toggle the GPIOs.

For now I would probably only implement a generic one regulator supply
plus one GPIO helper. That is the common case for touchscreens and
trackpads connected over a ribbon cable.

Does that sound like what you have in mind?


This next item would be a later enhancement (which isn't implemented in
this series anyway):

  - optional prober callback that does actual probing

In our case it would only be used for cases where an HID-over-I2C
component shares the same address as a non-HID one, and some extra
work is needed to determine which type it is. I still need to think
about the structure of this.


Thanks
ChenYu

Re: [PATCH v6 09/12] i2c: of-prober: Add regulator support

[Doug Anderson]

Hi,

On Thu, Sep 5, 2024 at 8:10 AM Chen-Yu Tsai <wenst@chromium.org> wrote:
>
> On Thu, Sep 5, 2024 at 6:57 AM Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Wed, Sep 4, 2024 at 2:01 AM Chen-Yu Tsai <wenst@chromium.org> wrote:
> > >
> > > This adds regulator management to the I2C OF component prober.
> > > Components that the prober intends to probe likely require their
> > > regulator supplies be enabled, and GPIOs be toggled to enable them or
> > > bring them out of reset before they will respond to probe attempts.
> > > GPIOs will be handled in the next patch.
> > >
> > > Without specific knowledge of each component's resource names or
> > > power sequencing requirements, the prober can only enable the
> > > regulator supplies all at once, and toggle the GPIOs all at once.
> > > Luckily, reset pins tend to be active low, while enable pins tend to
> > > be active high, so setting the raw status of all GPIO pins to high
> > > should work. The wait time before and after resources are enabled
> > > are collected from existing drivers and device trees.
> > >
> > > The prober collects resources from all possible components and enables
> > > them together, instead of enabling resources and probing each component
> > > one by one. The latter approach does not provide any boot time benefits
> > > over simply enabling each component and letting each driver probe
> > > sequentially.
> > >
> > > The prober will also deduplicate the resources, since on a component
> > > swap out or co-layout design, the resources are always the same.
> > > While duplicate regulator supplies won't cause much issue, shared
> > > GPIOs don't work reliably, especially with other drivers. For the
> > > same reason, the prober will release the GPIOs before the successfully
> > > probed component is actually enabled.
> > >
> > > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > > ---
> > > Changes since v5:
> > > - Split of_regulator_bulk_get_all() return value check and explain
> > >   "ret == 0" case
> > > - Switched to of_get_next_child_with_prefix_scoped() where applicable
> > > - Used krealloc_array() instead of directly calculating size
> > > - copy whole regulator array in one memcpy() call
> > > - Drop "0" from struct zeroing initializer
> > > - Split out regulator helper from i2c_of_probe_enable_res() to keep
> > >   code cleaner when combined with the next patch
> > > - Added options for customizing power sequencing delay
> > > - Rename i2c_of_probe_get_regulator() to i2c_of_probe_get_regulators()
> > > - Add i2c_of_probe_free_regulator() helper
> > >
> > > Changes since v4:
> > > - Split out GPIO handling to separate patch
> > > - Rewrote using of_regulator_bulk_get_all()
> > > - Replaced "regulators" with "regulator supplies" in debug messages
> > >
> > > Changes since v3:
> > > - New patch
> > >
> > > This change is kept as a separate patch for now since the changes are
> > > quite numerous.
> > > ---
> > >  drivers/i2c/i2c-core-of-prober.c | 154 +++++++++++++++++++++++++++++--
> > >  include/linux/i2c.h              |  10 +-
> > >  2 files changed, 155 insertions(+), 9 deletions(-)
> >
> > I never jumped back into looking at this since you started sending new
> > versions last month (sorry), but I finally did...
> >
> > At a high level, I have to say I'm not really a fan of the "reach into
> > all of the devices, jam their regulators on, force their GPIOs high,
> > and hope for the best" approach. It just feels like it's going to
> > break at the first bit of slightly different hardware and cause power
> > sequence violations left and right. If nothing else, regulators often
> > need delays between when one regulator is enabled and the next. There
> > may also be complex relationships between regulators and GPIOs, GPIOs,
> > GPIOs that need to be low, or even GPIO "toggle sequences" (power on
> > rail 1, wait 1 ms, assert reset, wait 10 ms, deassert reset, power on
> > rail 2).
> >
> > IMO the only way to make this reliable is to have this stuff be much
> > less automatic and much more driven by the board.
> >
> > I think that, in general, before the board prober checks i2c address
> > then the prober should be in charge of setting up pinctrl and turning
> > on regulators / GPIOs. Given that the same regulator(s) and GPIO(s)
> > may be specified by different children, the prober will just have to
> > pick one child to find those resources. It should have enough
> > board-specific knowledge to make this choice. Then the prober should
> > turn them on via a board-specific power-on sequence that's known to
> > work for all the children. Then it should start probing.
> >
> > I think there can still be plenty of common helper functions that the
> > board-specific prober can leverage, but overall I'd expect the actual
> > power-on and power-off code to be board-specific.
> >
> > If many boards have in common that we need to turn on exactly one
> > regulator + one GPIO, or just one regulator, or whatever then having a
> > helper function that handles these cases is fine. ...but it should be
> > one of many choices that a board proper could use and not the only
> > one.
>
> IIUC we could have the "options" data structure have much more board
> specific information:
>
>   - name of node to fetch resources (regulator supplies and GPIOs) from
>   - names of the resources for the node given from the previous item
>   - delay time after each resource is toggled
>   - polarity in the case of GPIOs
>   - prober callback to do power sequencing
>
> The "resource collection" step would use the first two items to retrieve
> the regulator supplies and GPIOS instead of the bulk APIs used right now.
>
> The power sequencing callback would use the resources combined with the
> given delays to enable the supplies and toggle the GPIOs.
>
> For now I would probably only implement a generic one regulator supply
> plus one GPIO helper. That is the common case for touchscreens and
> trackpads connected over a ribbon cable.
>
> Does that sound like what you have in mind?

I guess I'd have to see how the code looks to know for sure, but if I
understand it sounds a little awkward. Specifically, the "options"
sound like they might become complicated enough that you're inventing
your own little programming language (with delays, abilities to drive
pins low and high, abilities to turn on/off clocks, and abilities to
turn off/on regulators) and then probers need to code up their
programs in this language. You also need to handle undoing things
properly if there is a failure in the middle. Like your "program"
would look like this (obviously you'd have to play with enums more,
but you get the idea):

{
   { OPCODE_TURN_REGULATOR_ON, "vdd" },
   { OPCODE_DELAY, 10 },
   { OPCODE_GPIO_ASSERT, "reset" },
   { OPCODE_DELAY, 5 },
   { OPCODE_GPIO_DEASSERT "reset" },
   { OPCODE_DELAY, 100 },
   { OPCODE_TURN_REGULATOR_ON, "vddIO" },
}

Why not just expect the board probers to write C code to turn things
on before looking for i2c devices, then provide helpers to the C code?

So there wouldn't be some generic "resource collection" API, but you'd
provide a helper to make it easy to grab regulators from one of the
nodes by name. If you think bulk enabling regulators is common then
you could make a helper that grabs all of the regulators from a node
in a way that is consistent with the bulk APIs, but I wouldn't expect
every driver to use that since devices I've seen expect regulators to
be enabled in a very specific order even if they don't need a delay
between them.

I wouldn't expect a "collect all GPIOs" API because it seems really
weird to me that we'd ever want to jam multiple GPIOs in a state
without knowing exactly which GPIO was what and asserting them in the
right sequence.


> This next item would be a later enhancement (which isn't implemented in
> this series anyway):
>
>   - optional prober callback that does actual probing
>
> In our case it would only be used for cases where an HID-over-I2C
> component shares the same address as a non-HID one, and some extra
> work is needed to determine which type it is. I still need to think
> about the structure of this.

IMO _that_ would be a great option to the i2c prober. It feels like
you could have an optional register read that needs to match to have
the i2c prober succeed. Most people would leave it blank (just the i2c
device existing is enough) but probably a single register read would
be enough to confirm you got the right device. Most i2c devices have
some sort of "version" / "vendor" / "id" type register somewhere.

-Doug

Re: [PATCH v6 09/12] i2c: of-prober: Add regulator support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1155 bytes --]

On Thu, Sep 05, 2024 at 11:14:58AM -0700, Doug Anderson wrote:
> On Thu, Sep 5, 2024 at 8:10 AM Chen-Yu Tsai <wenst@chromium.org> wrote:

> > This next item would be a later enhancement (which isn't implemented in
> > this series anyway):

> >   - optional prober callback that does actual probing

> > In our case it would only be used for cases where an HID-over-I2C
> > component shares the same address as a non-HID one, and some extra
> > work is needed to determine which type it is. I still need to think
> > about the structure of this.

> IMO _that_ would be a great option to the i2c prober. It feels like
> you could have an optional register read that needs to match to have
> the i2c prober succeed. Most people would leave it blank (just the i2c
> device existing is enough) but probably a single register read would
> be enough to confirm you got the right device. Most i2c devices have
> some sort of "version" / "vendor" / "id" type register somewhere.

I'm not so sure about the "most" there - it depends quite a bit on the
class of device.  This also imply that the prober would have a regmap
config as well...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v6 09/12] i2c: of-prober: Add regulator support

[Andy Shevchenko]

On Thu, Sep 05, 2024 at 11:14:58AM -0700, Doug Anderson wrote:
> On Thu, Sep 5, 2024 at 8:10 AM Chen-Yu Tsai <wenst@chromium.org> wrote:

...

> I guess I'd have to see how the code looks to know for sure, but if I
> understand it sounds a little awkward. Specifically, the "options"
> sound like they might become complicated enough that you're inventing
> your own little programming language (with delays, abilities to drive
> pins low and high, abilities to turn on/off clocks, and abilities to
> turn off/on regulators) and then probers need to code up their
> programs in this language.

You beat me up to it. I have the same thought.
However, what is described is exactly what a regular PMIC has.
They already have their own language for exactly this purposes.
(At least I see that in a few Intel SoC-based platforms.)

So, after all it may be not a bad idea But would be good to have it
standardized (if it's even possible :-).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 09/12] i2c: of-prober: Add regulator support

[Chen-Yu Tsai]

On Fri, Sep 6, 2024 at 2:15 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Thu, Sep 5, 2024 at 8:10 AM Chen-Yu Tsai <wenst@chromium.org> wrote:
> >
> > On Thu, Sep 5, 2024 at 6:57 AM Doug Anderson <dianders@chromium.org> wrote:
> > >
> > > Hi,
> > >
> > > On Wed, Sep 4, 2024 at 2:01 AM Chen-Yu Tsai <wenst@chromium.org> wrote:
> > > >
> > > > This adds regulator management to the I2C OF component prober.
> > > > Components that the prober intends to probe likely require their
> > > > regulator supplies be enabled, and GPIOs be toggled to enable them or
> > > > bring them out of reset before they will respond to probe attempts.
> > > > GPIOs will be handled in the next patch.
> > > >
> > > > Without specific knowledge of each component's resource names or
> > > > power sequencing requirements, the prober can only enable the
> > > > regulator supplies all at once, and toggle the GPIOs all at once.
> > > > Luckily, reset pins tend to be active low, while enable pins tend to
> > > > be active high, so setting the raw status of all GPIO pins to high
> > > > should work. The wait time before and after resources are enabled
> > > > are collected from existing drivers and device trees.
> > > >
> > > > The prober collects resources from all possible components and enables
> > > > them together, instead of enabling resources and probing each component
> > > > one by one. The latter approach does not provide any boot time benefits
> > > > over simply enabling each component and letting each driver probe
> > > > sequentially.
> > > >
> > > > The prober will also deduplicate the resources, since on a component
> > > > swap out or co-layout design, the resources are always the same.
> > > > While duplicate regulator supplies won't cause much issue, shared
> > > > GPIOs don't work reliably, especially with other drivers. For the
> > > > same reason, the prober will release the GPIOs before the successfully
> > > > probed component is actually enabled.
> > > >
> > > > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > > > ---
> > > > Changes since v5:
> > > > - Split of_regulator_bulk_get_all() return value check and explain
> > > >   "ret == 0" case
> > > > - Switched to of_get_next_child_with_prefix_scoped() where applicable
> > > > - Used krealloc_array() instead of directly calculating size
> > > > - copy whole regulator array in one memcpy() call
> > > > - Drop "0" from struct zeroing initializer
> > > > - Split out regulator helper from i2c_of_probe_enable_res() to keep
> > > >   code cleaner when combined with the next patch
> > > > - Added options for customizing power sequencing delay
> > > > - Rename i2c_of_probe_get_regulator() to i2c_of_probe_get_regulators()
> > > > - Add i2c_of_probe_free_regulator() helper
> > > >
> > > > Changes since v4:
> > > > - Split out GPIO handling to separate patch
> > > > - Rewrote using of_regulator_bulk_get_all()
> > > > - Replaced "regulators" with "regulator supplies" in debug messages
> > > >
> > > > Changes since v3:
> > > > - New patch
> > > >
> > > > This change is kept as a separate patch for now since the changes are
> > > > quite numerous.
> > > > ---
> > > >  drivers/i2c/i2c-core-of-prober.c | 154 +++++++++++++++++++++++++++++--
> > > >  include/linux/i2c.h              |  10 +-
> > > >  2 files changed, 155 insertions(+), 9 deletions(-)
> > >
> > > I never jumped back into looking at this since you started sending new
> > > versions last month (sorry), but I finally did...
> > >
> > > At a high level, I have to say I'm not really a fan of the "reach into
> > > all of the devices, jam their regulators on, force their GPIOs high,
> > > and hope for the best" approach. It just feels like it's going to
> > > break at the first bit of slightly different hardware and cause power
> > > sequence violations left and right. If nothing else, regulators often
> > > need delays between when one regulator is enabled and the next. There
> > > may also be complex relationships between regulators and GPIOs, GPIOs,
> > > GPIOs that need to be low, or even GPIO "toggle sequences" (power on
> > > rail 1, wait 1 ms, assert reset, wait 10 ms, deassert reset, power on
> > > rail 2).
> > >
> > > IMO the only way to make this reliable is to have this stuff be much
> > > less automatic and much more driven by the board.
> > >
> > > I think that, in general, before the board prober checks i2c address
> > > then the prober should be in charge of setting up pinctrl and turning
> > > on regulators / GPIOs. Given that the same regulator(s) and GPIO(s)
> > > may be specified by different children, the prober will just have to
> > > pick one child to find those resources. It should have enough
> > > board-specific knowledge to make this choice. Then the prober should
> > > turn them on via a board-specific power-on sequence that's known to
> > > work for all the children. Then it should start probing.
> > >
> > > I think there can still be plenty of common helper functions that the
> > > board-specific prober can leverage, but overall I'd expect the actual
> > > power-on and power-off code to be board-specific.
> > >
> > > If many boards have in common that we need to turn on exactly one
> > > regulator + one GPIO, or just one regulator, or whatever then having a
> > > helper function that handles these cases is fine. ...but it should be
> > > one of many choices that a board proper could use and not the only
> > > one.
> >
> > IIUC we could have the "options" data structure have much more board
> > specific information:
> >
> >   - name of node to fetch resources (regulator supplies and GPIOs) from
> >   - names of the resources for the node given from the previous item
> >   - delay time after each resource is toggled
> >   - polarity in the case of GPIOs
> >   - prober callback to do power sequencing
> >
> > The "resource collection" step would use the first two items to retrieve
> > the regulator supplies and GPIOS instead of the bulk APIs used right now.
> >
> > The power sequencing callback would use the resources combined with the
> > given delays to enable the supplies and toggle the GPIOs.
> >
> > For now I would probably only implement a generic one regulator supply
> > plus one GPIO helper. That is the common case for touchscreens and
> > trackpads connected over a ribbon cable.
> >
> > Does that sound like what you have in mind?
>
> I guess I'd have to see how the code looks to know for sure, but if I
> understand it sounds a little awkward. Specifically, the "options"
> sound like they might become complicated enough that you're inventing
> your own little programming language (with delays, abilities to drive
> pins low and high, abilities to turn on/off clocks, and abilities to
> turn off/on regulators) and then probers need to code up their
> programs in this language. You also need to handle undoing things
> properly if there is a failure in the middle. Like your "program"
> would look like this (obviously you'd have to play with enums more,
> but you get the idea):
>
> {
>    { OPCODE_TURN_REGULATOR_ON, "vdd" },
>    { OPCODE_DELAY, 10 },
>    { OPCODE_GPIO_ASSERT, "reset" },
>    { OPCODE_DELAY, 5 },
>    { OPCODE_GPIO_DEASSERT "reset" },
>    { OPCODE_DELAY, 100 },
>    { OPCODE_TURN_REGULATOR_ON, "vddIO" },
> }
>
> Why not just expect the board probers to write C code to turn things
> on before looking for i2c devices, then provide helpers to the C code?
>
> So there wouldn't be some generic "resource collection" API, but you'd
> provide a helper to make it easy to grab regulators from one of the
> nodes by name. If you think bulk enabling regulators is common then
> you could make a helper that grabs all of the regulators from a node
> in a way that is consistent with the bulk APIs, but I wouldn't expect
> every driver to use that since devices I've seen expect regulators to
> be enabled in a very specific order even if they don't need a delay
> between them.
>
> I wouldn't expect a "collect all GPIOs" API because it seems really
> weird to me that we'd ever want to jam multiple GPIOs in a state
> without knowing exactly which GPIO was what and asserting them in the
> right sequence.

So I'm slightly confused, as it sounds like at this point the i2c prober
would be litter more than just a framework, and the heavy lifting is to
be all done by callbacks provided by the board-specific driver?

So the framework becomes something like:

1. find i2c bus node
2. call provided callback with i2c bus node to gather resources;
   let callback handle specifics
3. call provided callback to enable resources
4. for each i2c component, call provided callback to probe

  If the probe succeeded:

    5. call provided callback for early release of resources (GPIOs)
    6. set "status" to "okay"
    7. call provided callback for late release of resources (regulators)

  Otherwise at the end of the loop

8. release resources

The current code can be reworked into helpers for steps 2, 3, 5, 7 for
the single regulator single GPIO case.

> > This next item would be a later enhancement (which isn't implemented in
> > this series anyway):
> >
> >   - optional prober callback that does actual probing
> >
> > In our case it would only be used for cases where an HID-over-I2C
> > component shares the same address as a non-HID one, and some extra
> > work is needed to determine which type it is. I still need to think
> > about the structure of this.
>
> IMO _that_ would be a great option to the i2c prober. It feels like
> you could have an optional register read that needs to match to have
> the i2c prober succeed. Most people would leave it blank (just the i2c
> device existing is enough) but probably a single register read would
> be enough to confirm you got the right device. Most i2c devices have
> some sort of "version" / "vendor" / "id" type register somewhere.

At least for the stuff that we have (touchscreens and trackpads) such
registers typically don't exist, unless it's an HID-over-I2C device,
in which case there's the standard HID descriptor at some address.
But, yeah, reading the HID descriptor was the use case I had in mind.

At least for one Chromebooks it's a bit more tricky because that one
HID-over-I2C component shares the same address as a non-HID one. We
currently have different SKU IDs and thus different device trees for
them, but we could make the prober work with this. It just has be able
to tell if the component it's currently probing needs the special
prober and is it responding correctly. This bit I need to think about.


ChenYu

Re: [PATCH v6 00/12] platform/chrome: Introduce DT hardware prober

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1204 bytes --]

Hi,

this series gets quite some review from people I trust. This is awesome.
So, I will keep to some high level questions:

> undiscoverable devices" [2] series, but follows the scheme suggested by
> Rob, marking all second source component device nodes as "fail-needs-probe",
> and having a hardware prober driver enable the one of them.

Where is this "fail-needs-probe" documented? I neither see it here nor
in the dt-schema repo.

> The other case, selecting a display panel to use based on the SKU ID
> from the firmware, hit a bit of an issue with fixing the OF graph.
> It has been left out since v3.

Does it make sense then to add touchscreens only? Will the panels be
worked on once this is upstream? Or what is the way forward?

> Wolfram, would you be able to handle the late PR? Assuming you get a
> chance to look at the patches that is.

Yes, I can do this, but...

>  drivers/i2c/Makefile                          |   7 +-
>  drivers/i2c/i2c-core-of-prober.c              | 437 ++++++++++++++++++

... this is quite some code. Would you be willing to maintain it (i.e.
please add a MAINTAINERS entry then). Kudos for not touching other parts
of the I2C core!

All the best,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v6 05/12] regulator: Do pure DT regulator lookup in of_regulator_bulk_get_all()

[Chen-Yu Tsai]

On Wed, Sep 4, 2024 at 9:13 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Sep 04, 2024 at 05:00:07PM +0800, Chen-Yu Tsai wrote:
> > The to-be-introduced I2C component prober needs to enable regulator
> > supplies (and toggle GPIO pins) for the various components it intends
> > to probe. To support this, a new "pure DT lookup" method for getting
> > regulator supplies is needed, since the device normally requesting
> > the supply won't get created until after the component is probed to
> > be available.
> >
> > Convert the existing of_regulator_bulk_get_all() for this purpose.
> > This function has no in-tree users, as the original patch [1] that
> > used it was never landed. This patch changes the function ABI, but
> > it is straightforward to convert users.
> >
> > The underlying code that supports the existing regulator_get*()
> > functions has been reworked in previous patches to support this
> > specific case. An internal OF-specific version of regulator_get(),
> > of_regulator_get_optional(), is added for this.
> >
> > Also convert an existing usage of "dev && dev->of_node" to
> > "dev_of_node(dev)".
>
> > [1] https://lore.kernel.org/all/20231220203537.83479-2-jernej.skrabec@gmail.com/
> >
> > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
>
> Make it Link tag.
>
>   Link: https://lore.kernel.org/all/20231220203537.83479-2-jernej.skrabec@gmail.com/ [1]
>   Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>

Ack.

Re: [PATCH v6 06/12] gpiolib: Add gpio_get_property_name_length()

[Chen-Yu Tsai]

On Wed, Sep 4, 2024 at 9:40 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Sep 04, 2024 at 05:00:08PM +0800, Chen-Yu Tsai wrote:
> > The I2C device tree component prober needs to get and toggle GPIO lines
> > for the components it intends to probe. These components may not use the
> > same name for their GPIO lines, so the prober must go through the device
> > tree, check each property to see it is a GPIO property, and get the GPIO
> > line.
> >
> > Instead of duplicating the GPIO suffixes, or exporting them to the
> > prober to do pattern matching, simply add and export a new function that
> > does the pattern matching and returns the length of the GPIO name. The
> > caller can then use that to copy out the name if it needs to.
>
> > Andy suggested a much shorter implementation.
>
> No need to have this sentence in the commit message, changelog area is fine.
> But if you wish... :-)

It does seem out of place without any context. I'll move it to the
changelog area. :D

> > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
>
> ...
>
> > +/**
> > + * gpio_get_property_name_length - Returns the GPIO name length from a property name
> > + * @propname:        name of the property to check
> > + *
> > + * This function checks if the given property name matches the GPIO property
> > + * patterns, and returns the length of the name of the GPIO. The pattern is
> > + * "*-<GPIO suffix>" or just "<GPIO suffix>".
> > + *
> > + * Returns:
> > + * The length of the string before '-<GPIO suffix>' if it matches
> > + * "*-<GPIO suffix>", or 0 if no name part, just the suffix, or
> > + * -EINVAL if the string doesn't match the pattern.
>
> Should be %-EINVAL as we agreed with Bart when I updated GPIOLIB kernel-doc.

Ack.

In the regulator cleanups I did, I used -%EINVAL instead. But then I
realized that constants aren't really cross-referenced. I probably
have to go through all of them to fix those up.


ChenYu

Re: [PATCH v6 08/12] i2c: Introduce OF component probe function

[Chen-Yu Tsai]

On Wed, Sep 4, 2024 at 9:47 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> 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.
>
> ...
>
> > +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) {
>
> Hmm, but can it be for_each_child_of_node_with_prefix_scoped() now?

Looks like at least one of the fixups got squashed into the wrong patch.
I fixed both of them to use for_each_child_of_node_with_prefix().

That isn't compatible with Rob's comment though, and I'm not sure we
want a for_each_available_child_of_node_with_prefix() helper just for
one instance?

> > +             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;
> > +     }
> > +
> > +     i2c = of_get_i2c_adapter_by_node(i2c_node);
> > +     if (!i2c)
> > +             return dev_err_probe(dev, -EPROBE_DEFER, "Couldn't get I2C adapter\n");
> > +
> > +     ret = 0;
> > +     for_each_child_of_node_scoped(i2c_node, node) {
>
> Ditto.
>
> > +             union i2c_smbus_data data;
> > +             u32 addr;
> > +
> > +             if (!of_node_name_prefix(node, type))
> > +                     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;
> > +
> > +             /* Found a device that is responding */
> > +             ret = i2c_of_probe_enable_node(dev, node);
> > +             break;
> > +     }
> > +
> > +     i2c_put_adapter(i2c);
> > +
> > +     return ret;
> > +}
>
> ...
>
> > +EXPORT_SYMBOL_GPL(i2c_of_probe_component);
>
> Wonder if we may already use namespaced export from day 1.

I think that would be a good idea, especially given Doug's request for
exposing more helpers.

Thanks
ChenYu

Re: [PATCH v6 00/12] platform/chrome: Introduce DT hardware prober

[Chen-Yu Tsai]

On Sun, Sep 8, 2024 at 12:58 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> Hi,
>
> this series gets quite some review from people I trust. This is awesome.
> So, I will keep to some high level questions:
>
> > undiscoverable devices" [2] series, but follows the scheme suggested by
> > Rob, marking all second source component device nodes as "fail-needs-probe",
> > and having a hardware prober driver enable the one of them.
>
> Where is this "fail-needs-probe" documented? I neither see it here nor
> in the dt-schema repo.

You are correct that it has not been documented yet. I will send a patch
to the dt-schema repo.

> > The other case, selecting a display panel to use based on the SKU ID
> > from the firmware, hit a bit of an issue with fixing the OF graph.
> > It has been left out since v3.
>
> Does it make sense then to add touchscreens only? Will the panels be
> worked on once this is upstream? Or what is the way forward?

The devices this patch series targets all use eDP panels, which are
more or less directly probe-able.

The panels mentioned for the second phase are MIPI panels, which require
specific power and command sequences and thus are not probe-able. This
will be worked on once this part is done. For the panels it is entirely
handled in the chrome platform device tree prober. See the following
patch for an earlier version of it:

    https://lore.kernel.org/all/20231109100606.1245545-6-wenst@chromium.org/

> > Wolfram, would you be able to handle the late PR? Assuming you get a
> > chance to look at the patches that is.
>
> Yes, I can do this, but...
>
> >  drivers/i2c/Makefile                          |   7 +-
> >  drivers/i2c/i2c-core-of-prober.c              | 437 ++++++++++++++++++
>
> ... this is quite some code. Would you be willing to maintain it (i.e.
> please add a MAINTAINERS entry then). Kudos for not touching other parts
> of the I2C core!

Sure!

> All the best,
>
>    Wolfram

Thanks. There are still some comments to address, in particular making
the prober more of a pluggable framework and exposing more code as
helpers.


ChenYu

Re: [PATCH v6 09/12] i2c: of-prober: Add regulator support

[Doug Anderson]

Hi,

On Thu, Sep 5, 2024 at 8:45 PM Chen-Yu Tsai <wenst@chromium.org> wrote:
>
> > > IIUC we could have the "options" data structure have much more board
> > > specific information:
> > >
> > >   - name of node to fetch resources (regulator supplies and GPIOs) from
> > >   - names of the resources for the node given from the previous item
> > >   - delay time after each resource is toggled
> > >   - polarity in the case of GPIOs
> > >   - prober callback to do power sequencing
> > >
> > > The "resource collection" step would use the first two items to retrieve
> > > the regulator supplies and GPIOS instead of the bulk APIs used right now.
> > >
> > > The power sequencing callback would use the resources combined with the
> > > given delays to enable the supplies and toggle the GPIOs.
> > >
> > > For now I would probably only implement a generic one regulator supply
> > > plus one GPIO helper. That is the common case for touchscreens and
> > > trackpads connected over a ribbon cable.
> > >
> > > Does that sound like what you have in mind?
> >
> > I guess I'd have to see how the code looks to know for sure, but if I
> > understand it sounds a little awkward. Specifically, the "options"
> > sound like they might become complicated enough that you're inventing
> > your own little programming language (with delays, abilities to drive
> > pins low and high, abilities to turn on/off clocks, and abilities to
> > turn off/on regulators) and then probers need to code up their
> > programs in this language. You also need to handle undoing things
> > properly if there is a failure in the middle. Like your "program"
> > would look like this (obviously you'd have to play with enums more,
> > but you get the idea):
> >
> > {
> >    { OPCODE_TURN_REGULATOR_ON, "vdd" },
> >    { OPCODE_DELAY, 10 },
> >    { OPCODE_GPIO_ASSERT, "reset" },
> >    { OPCODE_DELAY, 5 },
> >    { OPCODE_GPIO_DEASSERT "reset" },
> >    { OPCODE_DELAY, 100 },
> >    { OPCODE_TURN_REGULATOR_ON, "vddIO" },
> > }
> >
> > Why not just expect the board probers to write C code to turn things
> > on before looking for i2c devices, then provide helpers to the C code?
> >
> > So there wouldn't be some generic "resource collection" API, but you'd
> > provide a helper to make it easy to grab regulators from one of the
> > nodes by name. If you think bulk enabling regulators is common then
> > you could make a helper that grabs all of the regulators from a node
> > in a way that is consistent with the bulk APIs, but I wouldn't expect
> > every driver to use that since devices I've seen expect regulators to
> > be enabled in a very specific order even if they don't need a delay
> > between them.
> >
> > I wouldn't expect a "collect all GPIOs" API because it seems really
> > weird to me that we'd ever want to jam multiple GPIOs in a state
> > without knowing exactly which GPIO was what and asserting them in the
> > right sequence.
>
> So I'm slightly confused, as it sounds like at this point the i2c prober
> would be litter more than just a framework, and the heavy lifting is to
> be all done by callbacks provided by the board-specific driver?
>
> So the framework becomes something like:
>
> 1. find i2c bus node
> 2. call provided callback with i2c bus node to gather resources;
>    let callback handle specifics
> 3. call provided callback to enable resources
> 4. for each i2c component, call provided callback to probe

I don't think I'd do it as callbacks but just have the HW prober call
the functions directly. AKA, instead of doing:

  i2c_of_probe_component(dev, "touchscreen", ts_opts, ts_callbacks);

Do:

  grab_touchscreen_resources(...);
  power_on_touchscreens(...);
  i2c_of_probe_component(...);
  power_off_touchscreen(...);
  release_touchscreen_resources(...);

Obviously I'm spitballing here, though. Without writing the code it's
hard for me to know that my proposal would be better, but my gut tells
me that trying to write something overly generic with lots of options
/ callbacks would be more confusing.


>   If the probe succeeded:
>
>     5. call provided callback for early release of resources (GPIOs)
>     6. set "status" to "okay"
>     7. call provided callback for late release of resources (regulators)
>
>   Otherwise at the end of the loop
>
> 8. release resources
>
> The current code can be reworked into helpers for steps 2, 3, 5, 7 for
> the single regulator single GPIO case.
>
> > > This next item would be a later enhancement (which isn't implemented in
> > > this series anyway):
> > >
> > >   - optional prober callback that does actual probing
> > >
> > > In our case it would only be used for cases where an HID-over-I2C
> > > component shares the same address as a non-HID one, and some extra
> > > work is needed to determine which type it is. I still need to think
> > > about the structure of this.
> >
> > IMO _that_ would be a great option to the i2c prober. It feels like
> > you could have an optional register read that needs to match to have
> > the i2c prober succeed. Most people would leave it blank (just the i2c
> > device existing is enough) but probably a single register read would
> > be enough to confirm you got the right device. Most i2c devices have
> > some sort of "version" / "vendor" / "id" type register somewhere.
>
> At least for the stuff that we have (touchscreens and trackpads) such
> registers typically don't exist, unless it's an HID-over-I2C device,
> in which case there's the standard HID descriptor at some address.
> But, yeah, reading the HID descriptor was the use case I had in mind.
>
> At least for one Chromebooks it's a bit more tricky because that one
> HID-over-I2C component shares the same address as a non-HID one. We
> currently have different SKU IDs and thus different device trees for
> them, but we could make the prober work with this. It just has be able
> to tell if the component it's currently probing needs the special
> prober and is it responding correctly. This bit I need to think about.

I guess Mark Brown also thought that there wouldn't be some magic
register, but my gut still tells me that most i2c devices have some
way to confirm that they are what you expect even if it's not an
official "vendor" or "version" register. Some type of predictable
register at a predictable location that you could use, at least if you
knew all of the options that someone might stuff.

For instance, in elan trackpads you can see elan_i2c_get_product_id().
That just reads a location (ETP_I2C_UNIQUEID_CMD = 0x0101) that could
theoretically be used to figure out (maybe in conjunction with other
registers) that it's an elan trackpad instead of an i2c-hid one. You'd
have to (of course) confirm that an i2c-hid device wouldn't somehow
return back data from this read that made it look like an elan
trackpad, but it feels like there ought to be some way to figure it
out with a few i2c register reads.

...that being said, I guess my original assertion that you might be
able to figure out with a simple register read was naive and you'd
actually need a function (maybe as a callback) to figure this out.


-Doug

Re: [PATCH v6 09/12] i2c: of-prober: Add regulator support

[Chen-Yu Tsai]

On Wed, Sep 11, 2024 at 8:30 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Thu, Sep 5, 2024 at 8:45 PM Chen-Yu Tsai <wenst@chromium.org> wrote:
> >
> > > > IIUC we could have the "options" data structure have much more board
> > > > specific information:
> > > >
> > > >   - name of node to fetch resources (regulator supplies and GPIOs) from
> > > >   - names of the resources for the node given from the previous item
> > > >   - delay time after each resource is toggled
> > > >   - polarity in the case of GPIOs
> > > >   - prober callback to do power sequencing
> > > >
> > > > The "resource collection" step would use the first two items to retrieve
> > > > the regulator supplies and GPIOS instead of the bulk APIs used right now.
> > > >
> > > > The power sequencing callback would use the resources combined with the
> > > > given delays to enable the supplies and toggle the GPIOs.
> > > >
> > > > For now I would probably only implement a generic one regulator supply
> > > > plus one GPIO helper. That is the common case for touchscreens and
> > > > trackpads connected over a ribbon cable.
> > > >
> > > > Does that sound like what you have in mind?
> > >
> > > I guess I'd have to see how the code looks to know for sure, but if I
> > > understand it sounds a little awkward. Specifically, the "options"
> > > sound like they might become complicated enough that you're inventing
> > > your own little programming language (with delays, abilities to drive
> > > pins low and high, abilities to turn on/off clocks, and abilities to
> > > turn off/on regulators) and then probers need to code up their
> > > programs in this language. You also need to handle undoing things
> > > properly if there is a failure in the middle. Like your "program"
> > > would look like this (obviously you'd have to play with enums more,
> > > but you get the idea):
> > >
> > > {
> > >    { OPCODE_TURN_REGULATOR_ON, "vdd" },
> > >    { OPCODE_DELAY, 10 },
> > >    { OPCODE_GPIO_ASSERT, "reset" },
> > >    { OPCODE_DELAY, 5 },
> > >    { OPCODE_GPIO_DEASSERT "reset" },
> > >    { OPCODE_DELAY, 100 },
> > >    { OPCODE_TURN_REGULATOR_ON, "vddIO" },
> > > }
> > >
> > > Why not just expect the board probers to write C code to turn things
> > > on before looking for i2c devices, then provide helpers to the C code?
> > >
> > > So there wouldn't be some generic "resource collection" API, but you'd
> > > provide a helper to make it easy to grab regulators from one of the
> > > nodes by name. If you think bulk enabling regulators is common then
> > > you could make a helper that grabs all of the regulators from a node
> > > in a way that is consistent with the bulk APIs, but I wouldn't expect
> > > every driver to use that since devices I've seen expect regulators to
> > > be enabled in a very specific order even if they don't need a delay
> > > between them.
> > >
> > > I wouldn't expect a "collect all GPIOs" API because it seems really
> > > weird to me that we'd ever want to jam multiple GPIOs in a state
> > > without knowing exactly which GPIO was what and asserting them in the
> > > right sequence.
> >
> > So I'm slightly confused, as it sounds like at this point the i2c prober
> > would be litter more than just a framework, and the heavy lifting is to
> > be all done by callbacks provided by the board-specific driver?
> >
> > So the framework becomes something like:
> >
> > 1. find i2c bus node
> > 2. call provided callback with i2c bus node to gather resources;
> >    let callback handle specifics
> > 3. call provided callback to enable resources
> > 4. for each i2c component, call provided callback to probe
>
> I don't think I'd do it as callbacks but just have the HW prober call
> the functions directly. AKA, instead of doing:
>
>   i2c_of_probe_component(dev, "touchscreen", ts_opts, ts_callbacks);
>
> Do:
>
>   grab_touchscreen_resources(...);
>   power_on_touchscreens(...);
>   i2c_of_probe_component(...);
>   power_off_touchscreen(...);
>   release_touchscreen_resources(...);
>
> Obviously I'm spitballing here, though. Without writing the code it's
> hard for me to know that my proposal would be better, but my gut tells
> me that trying to write something overly generic with lots of options
> / callbacks would be more confusing.

The helpers would be exported along with the framework, so there's nothing
preventing others from using the helpers directly. The framework provides
boilerplate so that scenarios fitting it won't have to duplicate the code.
Obviously I'm basing it on my scenario here, but I think it works out for
the simpler cases.

I'll send out what I have reworked, and we can continue the discussion
either on the mailing list or in person at ELCE and Plumbers.

> >   If the probe succeeded:
> >
> >     5. call provided callback for early release of resources (GPIOs)
> >     6. set "status" to "okay"
> >     7. call provided callback for late release of resources (regulators)
> >
> >   Otherwise at the end of the loop
> >
> > 8. release resources
> >
> > The current code can be reworked into helpers for steps 2, 3, 5, 7 for
> > the single regulator single GPIO case.
> >
> > > > This next item would be a later enhancement (which isn't implemented in
> > > > this series anyway):
> > > >
> > > >   - optional prober callback that does actual probing
> > > >
> > > > In our case it would only be used for cases where an HID-over-I2C
> > > > component shares the same address as a non-HID one, and some extra
> > > > work is needed to determine which type it is. I still need to think
> > > > about the structure of this.
> > >
> > > IMO _that_ would be a great option to the i2c prober. It feels like
> > > you could have an optional register read that needs to match to have
> > > the i2c prober succeed. Most people would leave it blank (just the i2c
> > > device existing is enough) but probably a single register read would
> > > be enough to confirm you got the right device. Most i2c devices have
> > > some sort of "version" / "vendor" / "id" type register somewhere.
> >
> > At least for the stuff that we have (touchscreens and trackpads) such
> > registers typically don't exist, unless it's an HID-over-I2C device,
> > in which case there's the standard HID descriptor at some address.
> > But, yeah, reading the HID descriptor was the use case I had in mind.
> >
> > At least for one Chromebooks it's a bit more tricky because that one
> > HID-over-I2C component shares the same address as a non-HID one. We
> > currently have different SKU IDs and thus different device trees for
> > them, but we could make the prober work with this. It just has be able
> > to tell if the component it's currently probing needs the special
> > prober and is it responding correctly. This bit I need to think about.
>
> I guess Mark Brown also thought that there wouldn't be some magic
> register, but my gut still tells me that most i2c devices have some
> way to confirm that they are what you expect even if it's not an
> official "vendor" or "version" register. Some type of predictable
> register at a predictable location that you could use, at least if you
> knew all of the options that someone might stuff.
>
> For instance, in elan trackpads you can see elan_i2c_get_product_id().
> That just reads a location (ETP_I2C_UNIQUEID_CMD = 0x0101) that could
> theoretically be used to figure out (maybe in conjunction with other
> registers) that it's an elan trackpad instead of an i2c-hid one. You'd
> have to (of course) confirm that an i2c-hid device wouldn't somehow
> return back data from this read that made it look like an elan
> trackpad, but it feels like there ought to be some way to figure it
> out with a few i2c register reads.
>
> ...that being said, I guess my original assertion that you might be
> able to figure out with a simple register read was naive and you'd
> actually need a function (maybe as a callback) to figure this out.

Yeah, that's my plan for a follow-up series, likely after the SKU ID
based prober work is done. The actual probe function would likely only
target the known cases of I2C address conflicts we have on ChromeOS
devices.

Re: [PATCH v6 06/12] gpiolib: Add gpio_get_property_name_length()

[Chen-Yu Tsai]

On Mon, Sep 9, 2024 at 10:45 AM Chen-Yu Tsai <wenst@chromium.org> wrote:
>
> On Wed, Sep 4, 2024 at 9:40 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Wed, Sep 04, 2024 at 05:00:08PM +0800, Chen-Yu Tsai wrote:
> > > The I2C device tree component prober needs to get and toggle GPIO lines
> > > for the components it intends to probe. These components may not use the
> > > same name for their GPIO lines, so the prober must go through the device
> > > tree, check each property to see it is a GPIO property, and get the GPIO
> > > line.
> > >
> > > Instead of duplicating the GPIO suffixes, or exporting them to the
> > > prober to do pattern matching, simply add and export a new function that
> > > does the pattern matching and returns the length of the GPIO name. The
> > > caller can then use that to copy out the name if it needs to.
> >
> > > Andy suggested a much shorter implementation.
> >
> > No need to have this sentence in the commit message, changelog area is fine.
> > But if you wish... :-)
>
> It does seem out of place without any context. I'll move it to the
> changelog area. :D
>
> > > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> >
> > ...
> >
> > > +/**
> > > + * gpio_get_property_name_length - Returns the GPIO name length from a property name
> > > + * @propname:        name of the property to check
> > > + *
> > > + * This function checks if the given property name matches the GPIO property
> > > + * patterns, and returns the length of the name of the GPIO. The pattern is
> > > + * "*-<GPIO suffix>" or just "<GPIO suffix>".
> > > + *
> > > + * Returns:
> > > + * The length of the string before '-<GPIO suffix>' if it matches
> > > + * "*-<GPIO suffix>", or 0 if no name part, just the suffix, or
> > > + * -EINVAL if the string doesn't match the pattern.
> >
> > Should be %-EINVAL as we agreed with Bart when I updated GPIOLIB kernel-doc.
>
> Ack.
>
> In the regulator cleanups I did, I used -%EINVAL instead. But then I
> realized that constants aren't really cross-referenced. I probably
> have to go through all of them to fix those up.

FTR this patch ended up getting dropped from the series as it was no
longer needed. However if folks think there is still value in this patch,
I can still send a new version.


ChenYu

Re: [PATCH v6 09/12] i2c: of-prober: Add regulator support

[Andy Shevchenko]

On Tue, Sep 10, 2024 at 05:30:07PM -0700, Doug Anderson wrote:
> On Thu, Sep 5, 2024 at 8:45 PM Chen-Yu Tsai <wenst@chromium.org> wrote:

...

> > At least for the stuff that we have (touchscreens and trackpads) such
> > registers typically don't exist, unless it's an HID-over-I2C device,
> > in which case there's the standard HID descriptor at some address.
> > But, yeah, reading the HID descriptor was the use case I had in mind.
> >
> > At least for one Chromebooks it's a bit more tricky because that one
> > HID-over-I2C component shares the same address as a non-HID one. We
> > currently have different SKU IDs and thus different device trees for
> > them, but we could make the prober work with this. It just has be able
> > to tell if the component it's currently probing needs the special
> > prober and is it responding correctly. This bit I need to think about.
> 
> I guess Mark Brown also thought that there wouldn't be some magic
> register, but my gut still tells me that most i2c devices have some
> way to confirm that they are what you expect even if it's not an
> official "vendor" or "version" register. Some type of predictable
> register at a predictable location that you could use, at least if you
> knew all of the options that someone might stuff.

"most" is way too optimistic to say, I believe that not even close to majority
of I²C target devices they are not reliably discoverable.

That's the downside of non-discoverable busses like I²C. Maybe I³C has
a mechanism for that, but I am not an expert, just wondering.

> For instance, in elan trackpads you can see elan_i2c_get_product_id().
> That just reads a location (ETP_I2C_UNIQUEID_CMD = 0x0101) that could
> theoretically be used to figure out (maybe in conjunction with other
> registers) that it's an elan trackpad instead of an i2c-hid one. You'd
> have to (of course) confirm that an i2c-hid device wouldn't somehow
> return back data from this read that made it look like an elan
> trackpad, but it feels like there ought to be some way to figure it
> out with a few i2c register reads.
> 
> ...that being said, I guess my original assertion that you might be
> able to figure out with a simple register read was naive and you'd
> actually need a function (maybe as a callback) to figure this out.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 06/12] gpiolib: Add gpio_get_property_name_length()

[Andy Shevchenko]

On Wed, Sep 11, 2024 at 03:37:35PM +0800, Chen-Yu Tsai wrote:
> On Mon, Sep 9, 2024 at 10:45 AM Chen-Yu Tsai <wenst@chromium.org> wrote:
> > On Wed, Sep 4, 2024 at 9:40 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Wed, Sep 04, 2024 at 05:00:08PM +0800, Chen-Yu Tsai wrote:

...

> > > > +/**
> > > > + * gpio_get_property_name_length - Returns the GPIO name length from a property name
> > > > + * @propname:        name of the property to check
> > > > + *
> > > > + * This function checks if the given property name matches the GPIO property
> > > > + * patterns, and returns the length of the name of the GPIO. The pattern is
> > > > + * "*-<GPIO suffix>" or just "<GPIO suffix>".
> > > > + *
> > > > + * Returns:
> > > > + * The length of the string before '-<GPIO suffix>' if it matches
> > > > + * "*-<GPIO suffix>", or 0 if no name part, just the suffix, or
> > > > + * -EINVAL if the string doesn't match the pattern.
> > >
> > > Should be %-EINVAL as we agreed with Bart when I updated GPIOLIB kernel-doc.
> >
> > Ack.
> >
> > In the regulator cleanups I did, I used -%EINVAL instead. But then I
> > realized that constants aren't really cross-referenced. I probably
> > have to go through all of them to fix those up.
> 
> FTR this patch ended up getting dropped from the series as it was no
> longer needed.

That's great news!

> However if folks think there is still value in this patch,
> I can still send a new version.

Not for me, thanks.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 09/12] i2c: of-prober: Add regulator support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2162 bytes --]

On Wed, Sep 11, 2024 at 05:38:23PM +0300, Andy Shevchenko wrote:
> On Tue, Sep 10, 2024 at 05:30:07PM -0700, Doug Anderson wrote:
> > On Thu, Sep 5, 2024 at 8:45 PM Chen-Yu Tsai <wenst@chromium.org> wrote:

> > > At least for one Chromebooks it's a bit more tricky because that one
> > > HID-over-I2C component shares the same address as a non-HID one. We
> > > currently have different SKU IDs and thus different device trees for
> > > them, but we could make the prober work with this. It just has be able
> > > to tell if the component it's currently probing needs the special
> > > prober and is it responding correctly. This bit I need to think about.

> > I guess Mark Brown also thought that there wouldn't be some magic
> > register, but my gut still tells me that most i2c devices have some
> > way to confirm that they are what you expect even if it's not an
> > official "vendor" or "version" register. Some type of predictable
> > register at a predictable location that you could use, at least if you
> > knew all of the options that someone might stuff.

> "most" is way too optimistic to say, I believe that not even close to majority
> of I²C target devices they are not reliably discoverable.

> That's the downside of non-discoverable busses like I²C. Maybe I³C has
> a mechanism for that, but I am not an expert, just wondering.

There's no standard mechanism for I2C, however it is relatively common
for devices to have some kind of ID registers.  This is especially true
if you're confining yourself to a particular class of device, bigger and
more modern devices are more likely to have this - if you want to pick
your audio CODEC or touchscreen controller it's a lot more likely
that'll work than something like a simple DAC or ADC for example.  You
also have the different I2C addresses that vendors pick to help out.
It's not an actual specified discovery mechanism, but practically
speaking you probably can generalise some of the ID register probing.
Though equally it's good practice for drivers to check this anyway so
it's not clear how much benefit there is over just trying to run probe().

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]